Repository navigation
test(sdk): expect retired ready marker after shared owner close - #6287
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. |
probepark
left a comment
There was a problem hiding this comment.
Review (head f248cd7, gajae-reviewer on behalf of probepark) — approve held
CI: green — all planned checks pass (test:packages/coding-agent/test/sdk-machine-lifecycle-topology.test.ts 2m14s, ts-build:coding-agent, gjc-state-gates, Virtual integration validation).
Scope: +2 / -1, 1 file — packages/coding-agent/test/sdk-machine-lifecycle-topology.test.ts (test-only)
Conventions: CHANGELOG none (test-only fix, not required), generated files none, labels none
Notable:
sdk-machine-lifecycle-topology.test.ts:L639-642— the ready-marker assertion aftercloseSharedOwnerflips fromresolves.toBeNull()torejects.toThrow(), matching #6265's intentional retirement of<id>.lifecycle.ready.jsonon close and the equivalent assertion insdk-broker-lifecycle-e2e.test.ts:L5762-5763..lifecycle.jsonstays asserted present (L638), andassertEndpointAndMarkerAbsent(loser, …)still covers the loser. No othercloseSharedOwnercall site in the file asserts the ready marker afterwards (L451-452 do not), so this is the only stale expectation.- Doc nit (non-blocking): the JSDoc on
closeSharedOwner(L343, "its lifecycle marker is retained until session.delete removes it") could mention that the ready marker is retired, to match the new comment at L639.
Blocking: none found in code review.
Approve held: CI plan did not run cd packages/coding-agent && bun run check (biome + check:types; check:@gajae-code/coding-agent is not in the head CI plan) on f248cd7; run it at this head (or add it to the plan) and re-request review.
|
#6287 and #6294 make the same one-line change to — |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This test-only change updates the shared-owner lifecycle topology test to expect the ready marker to be retired after closeSharedOwner while the lifecycle marker remains. That matches the established graceful-close contract and analogous broker lifecycle coverage. No merge-blocking defects were found.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
The updated fs.access(...).rejects.toThrow() assertion confirms access fails, but not specifically that the marker path is absent (a dangling symlink would also reject). That requires external filesystem mutation outside the controlled fixture and is not a blocker; using lstat and checking ENOENT would make the assertion more precise.
CI / Verification
For the exact reviewed head, 15 GitHub check runs succeeded and 8 were skipped; none failed or remained pending. The changed-test affected-path validation and its evidence/aggregate jobs succeeded. No tests were executed locally during this review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The marker lifecycle expectation matches the existing close contract and analogous tests; no intent_projection was found. |
| A2 — Architecture / Correctness / Failure | APPROVED |
The diff changes only the test assertion; the reachable cleanup and close paths support the expected post-close state. |
| A3 — Security / Privacy / Trust | APPROVED |
No production code, attacker-controlled input, authority, or trust boundary changes. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head affected-test validation and aggregate evidence passed; remaining skipped jobs are not failures. |
| A5 — Context / Compatibility / Platform | APPROVED |
Readiness consumers, persisted marker handling, and analogous lifecycle tests are consistent; no material reuse gap was identified. |
|
Merged into dev as — |
Why
Dev has been red since
e29e39e2(#6265 merge). Coding-agent shard 3 fails insdk-machine-lifecycle-topology.test.ts:shared-agent equal saved IDs select one owner without cross-workspace effects in either adapter direction. The test still expects<id>.lifecycle.ready.jsonto exist aftercloseSharedOwner.#6265 (fixes #6261) intentionally retires the ready marker when the owner closes, and it updated the matching assertion in
sdk-broker-lifecycle-e2e.test.tsthe same way (resolves→rejects). This topology test wasn't in #6265's CI plan, so it was missed.What
One assertion: after the owner closes, the ready marker is absent. The
.lifecycle.jsonmarker is still asserted present. Test-only.Evidence
bun test test/sdk-machine-lifecycle-topology.test.ts -t "shared-agent equal saved IDs": fails 3/3 one29e39e2, passes 3/3 on its parent8e709f59.9c54c088: whole file 7/0, 3 runs out of 3. Biome clean.—
[repo owner's gaebal-gajae (clawdbot) 🦞]