Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The detector can mistake prerequisite setup commands for generator work and approve a vacuous target.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces an SDK_LANGUAGES profile for generated SDKs and validates generation targets.
Changes:
- Makes aggregate Make targets profile-aware.
- Adds generator wiring validation.
- Adds documentation and Bats CI coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
seed/scripts/check-generate-targets.sh |
Validates generator artifacts and target plans. |
seed/Makefile.tmpl |
Adds profile-aware aggregates and validation target. |
seed/CONTRIBUTING.md.tmpl |
Documents profile-aware contributor workflow. |
seed/AGENTS.md.tmpl |
Updates generated repository guidance. |
prompts/seed-sdk.md |
Integrates profiles and validation into bootstrapping. |
MAKEFILE-CONVENTION.md |
Defines the language-profile contract. |
hack/test/check-generate-targets.bats |
Tests generator validation behavior. |
AGENTS.md |
Documents repository maintenance requirements. |
.github/workflows/ci.yml |
Runs the new Bats coverage. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c937a52ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b69881bdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unanchored invocation matching can still approve an echo-only generator target, and several documents overstate Swift conformance coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
prompts/seed-sdk.md:28
- This says
make conformancefollows every profile entry, butswiftis intentionally unmapped in the Makefile, so a full profile still omits Swift from this aggregate. Mention the macOS CI exception here; otherwise the bootstrap instructions overstate what the local gate covers.
seed/AGENTS.md.tmpl:38 - This overstates the profile contract because Swift is listed in
SDK_LANGUAGESbut intentionally omitted fromCONFORMANCE_RUNNERS. Call out that exception here so generated agent guidance does not claimmake checkruns Swift conformance.
seed/CONTRIBUTING.md.tmpl:35 - The “nothing else” claim is inaccurate for Swift:
SDK_LANGUAGEScan includeswift, but the aggregate deliberately has no Swift conformance mapping. Document that Swift conformance remains CI-only so contributors do not mistakemake checkfor covering it.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a72ae91a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 017fbd6fce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review threads: 7 resolved (4 fixed, 3 declined with the reasoning in each thread). Declined:
|
Closes #7.
Two things the seed's Makefile could not say: which languages an SDK actually ships, and whether a
<prefix>-generate-servicestarget does anything when called. Both items from the issue, one change.Profile:
SDK_LANGUAGESOne variable at the top of the rendered Makefile, by prefix, defaulting to the full set:
SDK_LANGUAGES := go ts rb swift kt rscheck,check-full,generate-services,generate-services-check,cleanandconformancederive from it;check-mvp'sgo-checkand the Go-embeddedprovenance-checkare included only whengois listed. Conformance runners are named by language, so aCONFORMANCE_RUNNER_<prefix>map turnstsintoconformance-typescript; Swift has no mapping because the aggregate did not run the Swift runner before this change either (the seededtest.ymlruns it on a macOS leg), andconformance-buildis split intoconformance-build-{go,typescript,kotlin}so it too only builds the profile's runners. Per-language targets are untouched:make ts-checkon ago rsprofile still exists and still fails on the missing tree, which is the point.On the full profile the aggregates expand to the same commands in the same order as before. I diffed
make -nofcheck,check-full,check-mvp,generate-services,conformance,conformance-buildandcleanbetween a render ofmainand a render of this branch: the only differences are the newgenerate-services-checkstep incheckand the per-runner==>banners underconformance-build. (generate-servicesnow listsswift-generate-servicesinstead ofswift-generate; both expand to$(MAKE) -C swift generate.)Rejected alternative: infer the profile from the directory tree.
sync-api-version-checkandsync-version.shalready skip a language whose file is absent, so ranging the aggregates over$(wildcard go typescript ruby ...)would have needed no instantiation step at all. It was rejected because it makes the gate narrow itself: a scaffold step that silently failed to creatego/would remove Go fromcheckinstead of failing it, which is the same class of silent no-op item 1 exists to catch. The file-presence skips in the version sync stay, because a constant that does not exist has nothing to be out of sync with; the gate's scope is declared, not observed.Not covered here, and named in
MAKEFILE-CONVENTION.md:make release's version-constant guards still grep all six languages' files, so a narrower profile cannot yet release. That is a separate change to the release target.Coverage:
scripts/check-generate-targets.shShips in
seed/scripts/, so it lands in every seeded repo. For each prefix in the profile it asserts three things:go/cmd/generate-services,typescript/scripts/generate-services.ts,ruby/scripts/generate-services.rb,swift/Makefile,kotlin/generator/build.gradle.kts,rust/generator/Cargo.toml);make -n <prefix>-generate-servicessucceeds and its plan, after dropping echoes and make's own recursion and "Nothing to be done" lines, contains the invocation of that generator (go run ./cmd/generate-services,scripts/generate-services.ts,ruby scripts/generate-services.rb,gradlew :generator:run,cargo run .*-generator, and for Swift aswift runline). A prerequisite's commands do not satisfy it:ts-installstill plansnpm ciafter the ts recipe is gone, and that must fail.make -nfollows$(MAKE) -C swift generateinto the sub-Makefile, so Swift's delegation is covered by the same rule: ageneratetarget with no recipe there, or a root alias that lost itsswift-generateprerequisite, both come back as "never invokes the generator";--dry-run,make <prefix>-generate-servicesruns and exits 0.make generate-services-checkruns the dry form and is part ofmake check(it is non-mutating; the full form rewrites generated trees, whichcheckmust not do, so the full form is the bootstrap checkpoint inprompts/seed-sdk.md). The check does not assert what the generator wrote: that is what<prefix>-check-driftis for, and some generators legitimately skip unchanged files.This repo cannot run the full form: the seed ships no generator for any language, so on a pristine render all six targets fail, exactly as the issue describes. What CI pins instead is the detector, in
hack/test/check-generate-targets.bats(newbatsstep in the "Seed templates" job): the pristine render fails forgo ts rb swift kt rs; stubbed artifacts pass, Swift through its sub-Makefile; a sub-Makefilegeneratewith no recipe fails; a root alias with no prerequisite fails; a recipe removed while its prerequisites still plan work fails; a generator named only inside an echo fails; ago rsprofile is checked forgo rsonly; run mode (a fakeswifton PATH) reports the target's exit status; an unknown prefix is rejected. The "temporarily break one target" proof from the issue is tests 4 through 7.Running it locally
In a seeded repo:
make generate-services-check(dry),scripts/check-generate-targets.sh(runs every generate target in the profile),scripts/check-generate-targets.sh swift(one language).Gates run locally:
hack/render-seed.sh,actionlinton the rendered workflows,zizmor 1.30.0 --config hack/zizmor-seed.yml --min-severity highon them,shellcheckon the rendered scripts (the new script included),actionlint+zizmoron.github/workflows/ci.yml, andbats hack/test.seed/rust/is untouched, so the rendered Rust scaffold job is unaffected.Docs
MAKEFILE-CONVENTION.mdgains a Language Profile section and agenerate-services-checkrow;AGENTS.mda section on what adding a language to the seed touches and what the bats file pins;seed/AGENTS.md.tmplandseed/CONTRIBUTING.md.tmplreplace the interim "all six unconditionally, trim at instantiation" notes from #5 with the contract;prompts/seed-sdk.mdsetsSDK_LANGUAGESin Phase 3, runsscripts/check-generate-targets.sh <prefix>as each language's step 4, trims the conformance matrix in Phase 5, and adds the full run to post-bootstrap verification.