Software factory: the instantiate check skips Specs that are not cards#5218
Conversation
Spec discovery skipped only specType 'field', so component and command Specs — which catalog cards routinely ship alongside the card Spec — flowed into instantiation and failed with 'Cannot find card …' (their refs point at Glimmer components and helper functions, not CardDefs). In the first adjust-flow baseline this failed run_instantiate 7 of 8 and the agent's only path to green was deleting the source Specs. Invert the filter to the card/app allowlist — the same one boxel realm ingest-card applies at seed time — so non-instantiable Specs are logged and skipped in both the run_instantiate tool and the pipeline's instantiate step (shared engine). Covered by a discovery unit test over all five specTypes and an e2e seeding the catalog-card shape (card Spec + component module + component/command Specs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes false failures in the software-factory “instantiate” validator by only attempting instantiation for instantiable Spec types (card and app) and skipping non-instantiable Spec types (e.g. field, component, command). This aligns the validator’s behavior with how seeding/ingest treats Specs, preventing adjust-flow baselines from going red due to internal building-block Specs.
Changes:
- Update Spec discovery to allowlist only
card/appspecTypevalues and skip all others with a log entry. - Add a unit test covering discovery behavior across multiple
specTypes (including missingspecType). - Add an e2e test that seeds a “catalog-like” mixed-Spec realm shape and asserts only the card Spec is instantiated.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/software-factory/src/instantiate-execution.ts | Filters discovered Specs to only instantiable specTypes (card/app) and logs skips. |
| packages/software-factory/tests/run-instantiate-in-memory.spec.ts | Adds e2e coverage ensuring non-card/app Specs are skipped (not counted as failures). |
| packages/software-factory/tests/instantiate-discovery.test.ts | Adds unit coverage for discovery filtering across Spec types. |
| packages/software-factory/tests/index.ts | Registers the new discovery unit test. |
| packages/software-factory/tests/helpers/instantiate-test-fixtures.ts | Adds fixtures to seed a realm containing a valid card Spec plus component/command Specs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only card/app specs reference instantiable CardDefs. Field, | ||
| // component, and command specs point at exports the prerenderer | ||
| // cannot instantiate — catalog cards commonly ship them alongside | ||
| // the card spec — so they are skipped rather than failing the run. | ||
| // Same allowlist `boxel realm ingest-card` applies at seed time. | ||
| let specType = attributes.specType as string | undefined; | ||
| if (specType === 'field') { | ||
| log.info(`Spec ${specId} is a field spec — skipping`); | ||
| if (specType !== 'card' && specType !== 'app') { | ||
| log.info( | ||
| `Spec ${specId} has specType ${specType ?? '(none)'} — not instantiable, skipping`, | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Good catch — fixed. defaultSearchSpecs now returns totalSpecCards (raw search hits before the card/app filter) and the retry predicate polls only when the search itself returned zero Spec cards. A realm whose Specs are all non-instantiable returns immediately. Covered by a new unit test asserting exactly one search call in that case.
Discovery's bounded poll treated a zero-spec result as index latency, so a realm whose Specs are all non-instantiable would have polled to the deadline before returning empty. defaultSearchSpecs now reports the raw Spec-card count alongside the filtered list, and the retry predicate only polls when the search itself returned nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd573832a4
ℹ️ 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".
| return retryWithPoll( | ||
| () => searchSpecsFn(options.targetRealm), | ||
| (r) => !r.error && r.specs.length === 0, | ||
| (r) => !r.error && r.specs.length === 0 && (r.totalSpecCards ?? 0) === 0, |
There was a problem hiding this comment.
When a caller injects a searchSpecsFn that returns the previous valid shape { specs: [] } (several tests do this to mean “nothing to validate”, e.g. validation-pipeline.test.ts), totalSpecCards is undefined and the ?? 0 makes this predicate true until the 30s retry deadline expires. Because totalSpecCards is optional, this is not caught statically, and no-spec validation paths now incur the full poll delay per run instead of returning immediately; treat undefined as “no raw-count signal” here, or update the injected API so empty final results can be represented without polling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed — the predicate now polls only on totalSpecCards === 0 (affirmative evidence of an index race); undefined means "no raw-count signal" and is final, so injected { specs: [] } doubles return immediately. Also applied the identical fix to parse-execution's spec-discovery twin, which had the same stall: validation-pipeline.test.ts drops from 31s to 1s. Both cases covered by unit tests asserting a single search call.
…ount
An injected searchSpecsFn returning the legacy { specs: [] } shape has
no totalSpecCards, and the previous '?? 0' treated that as an index
race — polling the full 30s deadline for results that were already
final. The predicate now requires totalSpecCards === 0 (affirmative
evidence the search returned nothing) to poll; undefined means no
signal and is final. defaultSearchSpecs always reports the count, so
production polling on genuine index latency is unchanged.
parse-execution's spec discovery had the identical pattern and the
identical stall (validation-pipeline tests stub it with { specs: [] });
it gets the same treatment. That suite drops from 31s to 1s.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes CS-11425.
The problem
The adjust flow copies an existing card and proves the copy works before changing anything. One of those proofs is the instantiate check: "try to create an instance of every card here."
Catalog cards ship Spec descriptor files not just for the card itself, but also for its internal building blocks — chart components, helper functions. Those building blocks aren't cards, so "create an instance" of them is meaningless. The check didn't know the difference and tried anyway.
Real example from the first adjust run: copying the catalog mortgage-calculator, the check made 8 attempts and failed 7 — every failure a building-block Spec (
LineChart,DonutChart,formatCurrency, …), none of them an actual problem. The agent's only way to a green baseline was deleting those 7 Specs from the copy — damaging what it had just copied before the real work started.The fix
The check now looks at each Spec's type first:
cardandappSpecs get instantiated, everything else (field,component,command) is skipped with a log line. This is the same ruleboxel realm ingest-cardalready applies when copying a card, applied now on the validation side too — so the baseline goes green without touching any source Spec, even when the workspace was seeded some other way (whole-realm pull, agent-written Specs).One safety interaction worth knowing: if skipping ever leaves nothing to check, the zero-coverage guard from #5199 reports that as an error instead of a hollow pass.
Tests
cardandappsurvive.🤖 Generated with Claude Code