Skip to content

fix: address late review threads on merged PRs (#368/#373/#374/#377/#378/#379/#385/#390) - #408

Merged
ScriptedAlchemy merged 16 commits into
mainfrom
fix/late-review-threads
Sep 3, 2026
Merged

fix: address late review threads on merged PRs (#368/#373/#374/#377/#378/#379/#385/#390)#408
ScriptedAlchemy merged 16 commits into
mainfrom
fix/late-review-threads

Conversation

@ScriptedAlchemy

Copy link
Copy Markdown
Owner

Summary

Sweep of review threads posted by chatgpt-codex-connector after 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.

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/dispatcher elapsed-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.
  • New tests: 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

  • CI green
  • pnpm test:integration:run locally (running alongside CI)

@changeset-bot

changeset-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d0861a1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
agent-bundle Patch

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T13:06:34.523933Z d0861a1 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@pkg-pr-new

pkg-pr-new Bot commented Sep 3, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle@408
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/create-agent-bundle@408
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/@agent-bundle/runtime@408

commit: d0861a1

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread .changeset/late-review-thread-fixes.md Outdated
---

Address the post-merge review findings on the dev epoch gate, native catalog,
portable validation, and inspection (#408):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b636c32: the changeset is now one user-facing paragraph whose final text is (#408), per .changeset/README.md.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1113 to +1114
for (const entry of staging) await this.#catalogStorage.remove(join(dirname(path), entry), { force: true });
if ((await file.stat()).nlink === 1) return settledOrWithdrawn();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: edec72e881

ℹ️ 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".

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 96 to 100
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 }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1198 to +1200
try { await this.#publicationReceipt(path, publicationIdentity, true, true).rollback(); }
catch (error) { cleanupFailures.push(error); }
created = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 153 to 157
this.#pending = Object.freeze({
contracts,
contracts: this.#contracts(),
epochId,
sequence: this.#sequence,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 7d041c733d

ℹ️ 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".

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1113 to +1116
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));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
…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
@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1135 to +1137
try { await this.#catalogStorage.remove(path, { force: true }); }
catch {
// Nothing further can make this publication unsettled; the caller rejects it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1133 to +1135
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: d0861a12c1

ℹ️ 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".

@ScriptedAlchemy
ScriptedAlchemy merged commit a00273b into main Sep 3, 2026
11 checks passed
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
Matches the README correction in main (#408): the canonical plugin-root
cwd is accepted and omitted; any other token-bearing cwd is rejected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant