fix(ci): isolate orphan-recovery transcript tests#109826
Conversation
862a654 to
a015350
Compare
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 5:20 AM ET / 09:20 UTC. Summary PR surface: Source +6, Tests +13. Total +19 across 2 files. Reproducibility: yes. The linked main workflow failure demonstrates the shared-worker import-order problem, and the grouped orphan-recovery plus registry test command is the focused reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land the injected reader seam once exact-head CI confirms the combined agents-core shard, keeping the canonical reader as the unchanged production default and the isolated mock confined to tests. Do we have a high-confidence way to reproduce the issue? Yes. The linked main workflow failure demonstrates the shared-worker import-order problem, and the grouped orphan-recovery plus registry test command is the focused reproduction path. Is this the best way to solve the issue? Yes. Dependency injection at the existing orphan-recovery boundary is narrower and more maintainable than preserving a module-cache mock or adding module resets, while leaving production composition unchanged. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0471b293bb5c. Label changesLabel justifications:
Evidence reviewedPR surface: Source +6, Tests +13. Total +19 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles) |
fd7d864 to
9b50fbd
Compare
9b50fbd to
198b414
Compare
What Problem This Solves
Current
mainfailschecks-node-agentic-agents-core-subagentswhen the shared agents-core Vitest worker imports orphan recovery before its test-local transcript-reader module mock.Why This Change Was Made
Inject the transcript reader through the existing orphan-recovery boundary so tests do not depend on module-cache import order, while production keeps the canonical reader.
User Impact
No runtime behavior change. Main CI can consistently validate interrupted subagent resume context.
Evidence
node scripts/run-vitest.mjs src/agents/subagent-registry.test.ts src/agents/subagent-orphan-recovery.test.ts— 140 tests passed, including the cache-order reproducer.pnpm format:check --no-error-on-unmatched-pattern -- src/agents/subagent-orphan-recovery.test.ts src/agents/subagent-orphan-recovery.tsThe independent
check-test-typesfailure from that run was fixed separately onmainbyf656d96fc3eand is not part of this PR.