fix: address late review threads on merged PRs (#368/#373/#374/#377/#378/#379/#385/#390) - #408
Conversation
🦋 Changeset detectedLatest commit: d0861a1 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. |
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd1d299b8
ℹ️ 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".
| await new Promise<void>((resolvePromise) => { setTimeout(resolvePromise, stagingPublicationPollMs); }); | ||
| const current = await file.stat(); | ||
| if (current.nlink < 1) return 'withdrawn'; | ||
| if (current.nlink === 1) return (await this.#sidecarStillLinked(path, metadata)) ? 'settled' : 'withdrawn'; |
There was a problem hiding this comment.
Wait for rollback before treating the sidecar as settled
When directory fsync fails after the hard link is created, #persistSnapshot removes the staging link in its finally block before rolling the sidecar back, so a concurrent reader can observe nlink === 1 here, return settled, and cache a catalog whose path is removed moments later. The staging-link disappearance alone is therefore not a commit signal; the failure/rollback ordering must prevent readers from adopting during this window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a68eb7a: #persistSnapshot now rolls the sidecar back inside the finally before removing the staging link whenever the primary failure happened after link() (directory fsync included), so a concurrent reader keeps observing an in-progress doubly linked publication until the path is withdrawn and returns to discovery instead of adopting a briefly singly linked file. New test withdraws a sidecar whose directory fsync fails before releasing its staging link, so a waiting reader never adopts it stalls the winner's directory fsync after the link, asserts the reader stays pending, fails the fsync, and verifies the reader never adopted the rolled-back inode and republished its own singly linked sidecar.
| export const contractProgressObserver = ( | ||
| client: ContractMatrixClient, | ||
| ): ContractMatrixProgressSource['observeProgress'] => { | ||
| if (typeof client.observeProgress === 'function') return client.observeProgress; |
There was a problem hiding this comment.
Preserve the custom progress observer's receiver
When a public ContractMatrixClient implements observeProgress as a normal method that reads instance state through this, returning the property directly causes executeLifecycleTransitions to invoke it as an unbound function, where this is undefined. Such lifecycle matrices fail before running their transitions; return a wrapper that calls client.observeProgress(...) or bind the method to client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a68eb7a: contractProgressObserver now returns (listener) => observeProgress.call(client, listener) so a class-based ContractMatrixClient whose observeProgress reads instance state keeps its receiver. Covered by the new invokes a custom observeProgress method with the client as its receiver test in dev-contract-runner.test.ts.
86f5f33 to
e14d36f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a68eb7a520
ℹ️ 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".
| --- | ||
|
|
||
| Address the post-merge review findings on the dev epoch gate, native catalog, | ||
| portable validation, and inspection (#408): |
There was a problem hiding this comment.
End the changeset summary with the PR suffix
The changeset’s PR suffix is followed by a colon and an extended bullet list, so the summary ends with serialization. rather than the required (#408). Reformat the user-facing summary so its final text is the PR suffix, as required for publishable-package changesets.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b636c32: the changeset is now one user-facing paragraph whose final text is (#408), per .changeset/README.md.
a68eb7a to
b636c32
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b636c32f62
ℹ️ 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".
| if (current.nlink === 1) return (await this.#sidecarStillLinked(path, metadata)) ? 'settled' : 'withdrawn'; | ||
| if (current.nlink !== 2 || !(await this.#stagingLinkAccountsFor(path, metadata))) throw invalid(); | ||
| if (!(await this.#sidecarStillLinked(path, metadata))) return 'withdrawn'; | ||
| if (Date.now() >= deadline) throw invalid(); |
There was a problem hiding this comment.
Recover abandoned staging links after the settle deadline
If the publisher process exits after creating and syncing the sidecar but before removing its staging link, the sidecar permanently retains nlink === 2 with a matching .stage-* sibling. Every subsequent Workbench process waits five seconds here and then rejects the catalog, and the repository has no startup recovery path for these catalog staging files, so Native Playground remains unavailable for that epoch until manual cleanup or retention removes it. Once the deadline establishes that publication is abandoned, recover or withdraw the orphaned publication rather than classifying it as a hostile alias forever.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a49115: once the settle deadline passes, the reader inspects each matching .stage-<pid>-* sibling. If every publisher pid has exited (process.kill(pid, 0) → ESRCH; the current process and any live/unsignalable pid count as alive), the publication is abandoned — the sidecar was fsynced before it was linked — so the reader withdraws the orphaned staging link(s) and adopts the now singly linked sidecar. A staging link whose publisher is still running keeps being rejected rather than yanked. Test recovers a staging link abandoned by an exited publisher after the settle deadline, but never one whose publisher is alive covers both branches via the new @internal catalogStagingSettleDeadlineMs seam.
b636c32 to
8a49115
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a491154c1
ℹ️ 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".
| for (const entry of staging) await this.#catalogStorage.remove(join(dirname(path), entry), { force: true }); | ||
| if ((await file.stat()).nlink === 1) return settledOrWithdrawn(); |
There was a problem hiding this comment.
Fsync the directory after recovering an orphan
When the publisher exits after link() but before its directory fsync, this recovery path unlinks the abandoned staging name and immediately treats the sidecar as settled without syncing the directory. A machine crash before that metadata is flushed can therefore lose the sidecar while retaining an older staging entry, causing the old epoch to fall back to discovery from mutable current eval sources on restart. Call #syncCatalogDirectory(dirname(path)) after removing the orphaned links and before returning settled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in edec72e: after withdrawing the orphaned staging link(s) the reader calls #syncCatalogDirectory(dirname(path)) before returning settled, so a crash cannot retain the stale staging entry while losing the sidecar. The abandoned-staging test now asserts exactly one directory fsync during recovery.
8a49115 to
edec72e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
edec72e to
956eb21
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956eb21cd7
ℹ️ 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".
| callTool: async (params, options) => session.callTool({ | ||
| arguments: params.arguments ?? {}, | ||
| name: params.name, | ||
| signal: options?.signal === undefined ? signal : AbortSignal.any([signal, options.signal]), | ||
| signal: requestSignal(session, options), | ||
| ...(options?.timeout === undefined ? {} : { timeoutMs: options.timeout }), |
There was a problem hiding this comment.
Forward progress metadata through the dev matrix client
When a development lifecycle fixture expects progress, callToolResult supplies its generated token in params._meta, but this adapter forwards only arguments and name; McpSession.callTool likewise reconstructs the wire request without _meta. Generated routes enable sendProgress only when context.mcpReq._meta.progressToken is present, so these calls emit no matching notifications and every lifecycle fixture requiring progress fails the matrix, preventing the epoch from being adopted. Extend the session call options and wire request to preserve _meta.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee9ec2f: McpSessionToolCallOptions and McpClient.callTool params gain _meta (McpRequestMeta, with progressToken), McpSession.#callToolEffect forwards it to the SDK client's callTool params, and the dev matrix adapter passes params._meta through. Tests: dev-contract-runner.test.ts asserts the adapter forwards the lifecycle progress token (and omits _meta when absent); the McpSession timeout test now also asserts the wire params carry _meta only when supplied.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956eb21cd7
ℹ️ 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".
| try { await this.#publicationReceipt(path, publicationIdentity, true, true).rollback(); } | ||
| catch (error) { cleanupFailures.push(error); } | ||
| created = false; |
There was a problem hiding this comment.
Keep the staging link when rollback fails
When the primary publication error triggers rollback and rollback itself fails before moving or removing the sidecar (for example, rename returns EBUSY), this catch records the error but still sets created = false, after which the staging link is removed. The surviving sidecar then has nlink === 1, so concurrent or future readers treat the failed publication as settled and adopt it. Fresh evidence beyond the earlier ordering report is that the new rollback failure path is swallowed before the unconditional staging unlink; only release that link after confirming the owned sidecar was withdrawn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 681fd8d: after a failed publication the staging link is released only when #sidecarStillLinked(path, staged) confirms the owned sidecar was withdrawn; if the rollback could not move or remove it (e.g. EBUSY on rename) the staging link stays, so the sidecar remains doubly linked and readers keep treating it as in progress. New test keeps the staging link when a failed publication cannot roll its sidecar back injects a failing move seam and asserts nlink stays 2, the stage entry survives, and a fresh reader rejects rather than adopts.
There was a problem hiding this comment.
Correction: the fix commit is 7d041c7 (the SHA in my previous reply was main's tip at the time of a rebase, not this change).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee9ec2f6d5
ℹ️ 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".
| this.#pending = Object.freeze({ | ||
| contracts, | ||
| contracts: this.#contracts(), | ||
| epochId, | ||
| sequence: this.#sequence, | ||
| }); |
There was a problem hiding this comment.
Restart the drain when an epoch arrives during finalization
When contracts are disabled, this change now queues every artifact.available event through #processing instead of adopting synchronously. If a second event is delivered after #drain() has observed an empty queue but before its .finally() clears #processing, this assignment leaves the new candidate in #pending while ??= retains the already-resolved promise; no drain is restarted, so hosts keep serving the previous epoch until another rebuild happens. The processing completion path needs to recheck/restart when #pending was populated during that handoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 681fd8d: #schedule() owns the ??=; the drain's completion handler clears #processing and, if #pending was populated during the handoff, calls #schedule() again, and settled() now loops until no drain is running. Regression test publishes epoch-2 from the epoch-1 adoption listener at microtask depths 0–8 and requires epoch-2 to be adopted at every depth (it fails at depth 3 without the restart).
There was a problem hiding this comment.
Correction: the fix commit is 7d041c7 (the SHA in my previous reply was main's tip at the time of a rebase, not this change).
|
@codex review |
3df1756 to
7d041c7
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
80fc95d to
0e719ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e719ec55e
ℹ️ 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".
| for (const entry of staging) await this.#catalogStorage.remove(join(dirname(path), entry), { force: true }); | ||
| // The exited publisher may never have fsynced the directory after link(): | ||
| // flush it here so a crash cannot keep the orphan and lose the sidecar. | ||
| await this.#syncCatalogDirectory(dirname(path)); |
There was a problem hiding this comment.
Preserve the staging guard when recovery fsync fails
When recovering an exited publisher, the staging link is removed before the directory is synced. If this fsync fails (for example with EIO), the current read rejects, but the sidecar is already singly linked; another Workbench process can therefore immediately treat it as settled, even though the staging removal was never made crash-durable and a reboot may lose the sidecar. Keep the publication visibly unsettled or withdraw the sidecar when this recovery fsync cannot complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b5a3c7: if the recovery's directory fsync throws, #restoreStagingGuard re-links the staging entries it just removed (so the sidecar is doubly linked again and every reader keeps treating it as in progress), and if even that fails it withdraws the sidecar; the read then rejects as invalid with the fsync error as cause. The abandoned-staging test now injects an EIO directory fsync during recovery and asserts the reader rejects, nlink is back to 2, and the orphan name aliases the sidecar's inode.
|
@codex review |
cd71d02 to
153fe48
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 153fe480e9
ℹ️ 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".
| } | ||
| let lease: EpochAdoptionLease | undefined; | ||
| if (evaluation === undefined || evaluation.state === 'passed') { | ||
| const leased = await this.#acquireLease(candidate); |
There was a problem hiding this comment.
Recheck supersession after awaiting the lease
When a newer artifact.available event is queued immediately after the lease promise settles, #acquireLease can check candidate.sequence before that event runs, while this outer await resumes afterward. Because the caller does not recheck supersession, it can publish and adopt the obsolete candidate; if the newer candidate subsequently fails its contracts, hosts remain on an epoch that should have been discarded. Recheck the sequence after this await and close the newly acquired lease when it has been superseded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b641117: after await this.#acquireLease(candidate) the drain rechecks candidate.sequence !== this.#sequence (and #closed), closes the freshly acquired lease, and continues, so an obsolete candidate is neither published nor adopted. Regression test publishes epoch-2 (which then fails its contracts) from microtask depths 0–8 inside epoch-1's lease and asserts epoch-1 is never adopted after epoch-2 exists and its lease is released; it fails at depth 2 without the recheck.
#379/#385/#390 - dev: lease the adopted epoch in EpochAdoptionPolicy until replaced or closed; select the contract-matrix target from the server's own target list; apply the session timeout per matrix request; observe lifecycle progress through the session trace via a new ContractMatrixClient.observeProgress seam (#385) - playground: wait for a hard-link catalog publisher to release its staging link before adopting the sidecar; return to discovery when the publication is rolled back (#377) - build: run the Agent Plugins byte lane over portable/ during ordinary artifact validation; reject every forbidden control character in header values (#373) - events/hooks: Codex PostToolUse accepts any present JSON tool_response (#378) - api: project only contract fields of adapter capability rows in inspect (#390) - tests/support: digest the real Claude home in the live session guard; isolate USERPROFILE alongside HOME (#374) - docs: Claude plugin-root cwd exception, parked-pin trigger independence, provider typing contract, portable validation moments (#368/#379/#382/#373)
…h the portable byte lane - EpochAdoptionPolicy acquires the epoch lease before publishing a passed dev.contract.status and announces adoption synchronously with it, so a status reader never sees "passed" for an epoch that is not yet adopted - hooks.test: Codex PostToolUse accepts a string tool_response and rejects a missing one; Claude keeps the object check - mcp-session-service/public-api-packed fixtures: Agent Plugins forbids placeholders in headers and non-bare/non-./ commands, and ordinary artifact validation now enforces the standard, so the fixtures carry a literal header and a bare `node` command
…leasing its staging link; bind custom observeProgress - #persistSnapshot rolls the sidecar back while the staging link still exists when the post-link directory fsync fails, so a concurrent reader keeps seeing an in-progress publication until the path is withdrawn instead of adopting a briefly singly linked file - contractProgressObserver invokes a client's observeProgress method with the client as receiver
…d publisher After the settle deadline, a matching .stage-<pid>-* link whose publisher pid no longer exists is an abandoned publication of an already fsynced sidecar: withdraw the orphan and adopt the sidecar instead of rejecting the epoch forever. A live publisher's staging link is still never yanked. Adds the @internal catalogStagingSettleDeadlineMs seam for deterministic tests.
…ndoned staging link
…nd the dev matrix client Lifecycle fixtures pass their generated progressToken as params._meta; the session adapter and McpSession.callTool dropped it, so generated routes never enabled sendProgress and every progress-gated lifecycle fixture failed the dev matrix. McpSessionToolCallOptions and McpClient.callTool now carry _meta.
… keep the staging link when a sidecar rollback fails - EpochAdoptionPolicy reschedules its drain from the completion handler when a candidate arrived between the loop's last empty check and #processing being cleared, and settled() waits through restarts - #persistSnapshot releases the staging link after a failed publication only once the owned sidecar is confirmed withdrawn, so a rollback failure never leaves a singly linked sidecar for readers to adopt
…he _meta assertions
… aliasing the sidecar) during recovery
153fe48 to
b641117
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b641117d17
ℹ️ 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".
| try { await this.#catalogStorage.remove(path, { force: true }); } | ||
| catch { | ||
| // Nothing further can make this publication unsettled; the caller rejects it. |
There was a problem hiding this comment.
Keep the sidecar guarded if fallback withdrawal fails
When recovery directory fsync fails, relinking the removed staging guard also fails, and remove(path) rejects (for example with EBUSY, EPERM, or EIO), this catch swallows the withdrawal failure after the staging name has already been removed. The canonical sidecar is then left with nlink === 1, so another reader bypasses #awaitStagedPublication and adopts a publication whose recovery was not made durable. Fresh evidence beyond the earlier guard-restoration report is this fallback-removal failure path; retain or recreate a guard unless withdrawal is confirmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7cfbc67: when re-linking the removed staging names fails and the sidecar cannot be unlinked either, #restoreStagingGuard creates a fresh guard .<epoch>.stage-<this pid>-guard-<nonce> aliasing the sidecar, so it stays doubly linked (readers reject it while this process is alive and recover it once the pid is gone); only if that link also fails does it throw an AggregateError of all three failures. The abandoned-staging test now injects EPERM on the re-link plus EBUSY on the sidecar unlink and asserts nlink stays 2, exactly one pid-owned guard aliases the sidecar, and the next reader still rejects.
…er re-link nor withdraw the sidecar
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cfbc6797b
ℹ️ 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".
| const restored = await Promise.allSettled(staging.map((entry) => this.#restoreStagingLink(path, entry))); | ||
| const failures = restored.flatMap((outcome) => (outcome.status === 'rejected' ? [outcome.reason] : [])); | ||
| if (failures.length === 0) return; |
There was a problem hiding this comment.
Fsync the restored staging guard before returning
When orphan recovery's directory fsync fails after unlinking the staging entry, a successful #restoreStagingLink makes the guard visible only in memory and this method immediately returns without syncing the directory again. If the machine crashes, the earlier unlink may persist while the compensating link does not, leaving the canonical sidecar singly linked and eligible for adoption after reboot despite the failed recovery. Fresh evidence beyond the earlier guard-restoration report is that neither the restored original link nor the fallback fresh guard is made crash-durable; sync the directory after recreating a guard before relying on it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0861a1: #restoreStagingGuard now treats each compensating step as trusted only after a directory fsync — re-link the removed names → fsync; else withdraw the sidecar → fsync; else create a fresh pid-owned guard → fsync; else throw an AggregateError of every failure. The abandoned-staging test counts fsync failures so the recovery fsync fails while the guard fsync succeeds (asserting exactly two directory syncs), and adds a persistent-fsync-failure scenario where the sidecar ends up withdrawn (never singly linked) and is republished from discovery.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Matches the README correction in main (#408): the canonical plugin-root cwd is accepted and omitted; any other token-bearing cwd is rejected.
Summary
Sweep of review threads posted by
chatgpt-codex-connectorafter merge on PRs landed 2026-09-02 evening. Each actionable thread on a merged PR is fixed here (replies with this PR + SHA follow on every thread); threads on still-open PRs were answered with the owning lane.EpochAdoptionPolicyholds anEpochReferencelease for the adopted epoch until it is replaced or the policy closes (retention could otherwise delete the advertised last-good epoch after six failing rebuilds); a candidate that cannot be leased is not adopted and is reported asAB7211. The contract-matrix runner selects the target from the configured server's own target intersection (atargets: ['claude']server is absent from the portable manifest), appliessession.timeoutMsper request instead of one signal for the whole matrix, and exposesobserveProgressbacked by the session trace so lifecycle fixtures no longer reach into the SDK client's private_notificationHandlersmap (which the non-SDK adapter never had).AB6035–AB6037) runs overportable/during ordinarybuild/validate --artifact, so standard-invalid documents fail before publication rather than only under--host-validation; header values reject every C0 control character except HTAB, plus DEL.PostToolUseaccepts any present JSONtool_response(pinned schema"tool_response": true) in bothvalidateNativeEventEnvelopeand the generated native hook wrapper; Claude keeps the object check.nameor break--json.HOMEis swapped);USERPROFILEis isolated alongsideHOME.Evidence
pnpm typecheck✅,pnpm lint✅ (0 errors),pnpm build✅pnpm test:unit: 2716 passed; 3 failures were 5 s timeouts under load avg 65–125 in untouched files (inspect-state,native-claude-contract,rsc-runtime/dispatcherelapsed-deadline). Rerun in isolation:inspect-state✅,dispatcher✅,native-claude-contract✅ with--testTimeout 60000(17/17; slow, not hung).pnpm test:route-unit: 1 timeout (lifecycle-replay) under load; rerun in isolation ✅ 5/5.pnpm test:projection✅ 63/63.api.test.ts(integration) ✅ 36/36 including the new portable byte-lane build test and capability projection test.dev-contract-runner.test.ts(target selection, per-request signals, trace progress, observer seam), adoption lease + lease-failure, native-playground withdrawn-winner, Codex tool_response, header control chars.Test plan
pnpm test:integration:runlocally (running alongside CI)