fix(harness): preserve typed outcomes and original operation bounds - #2801
Conversation
There was a problem hiding this comment.
Changes requested: The new admission scope also rejects abort, interrupt and existing process shutdown after hard stop or expiry.
Warning
Changes requested · head b2cb296 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — harness-pi.md item 6: stop scope blocks abort and process shutdown | src/core/harness/container.ts:1106 |
F1 invariant: Cancellation or exhaustion of work admission must forbid new work without disabling bounded safety shutdown of an already-owned harness process; shutdown must preserve original ownership and must not imply atomic all-descendant termination.
- Pi on the repo-resident factory: RunControl.requestStop('hard') aborts the scope while the launched process is alive. → Deliver the existing abort and bounded owned-process kill; refuse new prompts and launches. Do not swallow admission refusal and leave the process running.
- Pi on the repo-cold factory: the same hard stop occurs while its process is alive. → Deliver the existing abort and bounded owned-process kill through the cold executor without granting new work or treating transport abort as physical termination.
- Pi on the none/host factory: a hard stop occurs after start tracked the child and its piped stdin. → Allow the safety abort and TERM/KILL path for the tracked child despite the aborted work scope; do not retain a live child and its open pipes after end reports completion.
- OpenCode on the repo-resident or repo-cold factory: a hard stop occurs with an assigned live server and tailer. → Allow the existing session interrupt and bounded server/tailer shutdown; refuse model/session work and new launches.
- OpenCode on the none/host factory: a hard stop occurs with an assigned live server and tailer. → Allow the interrupt request and the tracked server/tailer termination despite the work signal; end must not silently turn both kills into no-ops.
- Pi or OpenCode on an exec factory reaches the admitted absolute deadline, or the current RunControl lease expires, while the owned producer remains alive. → Refuse further work but preserve the existing abort/interrupt and bounded shutdown path. The executor's one-second command floor must not suppress safety shutdown.
- Pi or OpenCode on the host factory reaches the admitted deadline or current RunControl lease expires with a tracked live child. → Forbid new work while allowing bounded termination of the tracked child and safe cleanup after termination.
- The runLoop finish-plan factory is constructed with an exhausted carried admission bound and facts naming a leftover process. → Preserve an ownership-checked, bounded leftover-process shutdown path rather than applying model-work admission to the entire find/end lifecycle.
- The original shutdown command encounters a control reset during bounded safety shutdown. → Any permitted idempotent retry consumes the shutdown operation's one original bound; it does not renew work admission or authorize another producer.
- A fresh run is cancelled or expired before root creation or launch, on either exec or host factory. → Continue refusing setup and launch; introducing a safety shutdown path must not reopen work admission.
- OpenCode continuation probe/session/feed collection is unknown before a server is adopted, or facts name another container/harness. → Keep the current no-cleanup/no-rival behavior; a safety shutdown exception must apply only to an already-owned process, never to uncertainty or a foreign process.
Full review
F1 (high confidence): After RunControl.requestStop("hard") aborts the signal, this guard rejects pi’s abort write, OpenCode’s interrupt request, and both harnesses’ subsequent kill calls. Deadline or lease exhaustion has the same effect. BotHostHarnessContainer applies the equivalent guard at lines 112–119 and 333. Both harnesses swallow shutdown failures, so the run can finish without sending any termination command; on the host, the tracked child and its open pipes remain alive with no workspace teardown to rescue them. This contradicts harness-pi item 6’s hard-stop behavior and disables existing shutdown—not merely the explicitly unimplemented atomic descendant-control capability. Separate work admission from a bounded, ownership-checked shutdown path, keeping abort/interrupt and termination deliverable while continuing to refuse new prompts and launches.
b2cb296 to
d9c7551
Compare
There was a problem hiding this comment.
Changes requested: Original stop delivery is repaired, but cleanup failures inside open() still bypass workspace retention.
Warning
Changes requested · head d9c7551 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F2 Spec contradiction — harness.md item 13: failed opening cleanup still authorizes workspace teardown | src/core/dispatch/runLoop.ts:1271 |
F2 invariant: An unconfirmed ending of an already-launched harness must retain its original producer paths and workspace before hard-stop, gate-bypass or in-flight release policy, whether cleanup happens inside open() or after a HarnessSession is returned; absence of a returned session is not confirmation of termination.
- Pi on repo-resident or repo-cold launches, its loop throws with a workspace command in flight, and the local end() invoked by runPiHarnessOpen's catch rejects on kill. → Carry the unconfirmed ending to runLoop before release; retain the original root and workspace rather than treating the undefined harnessSession as a settled end.
- OpenCode on repo-resident or repo-cold launches its server and tailer, driveOpenCode throws, and openOpenCodeRun's catch invokes an end() whose server kill rejects. → Propagate the unconfirmed ending to dispatch and retain the original root and workspace, including when hard stop or an in-flight call otherwise forces release.
- OpenCode on either exec factory encounters an opening-loop failure; server kill completes but tailer kill rejects during local cleanup. → Retain the feed, private root and workspace for the unconfirmed tailer instead of authorizing teardown because open() returned no session.
- Pi or OpenCode on either exec factory encounters an opening-loop failure, and local root removal rejects after the preceding shutdown commands complete. → Carry the incomplete ending to dispatch and retain the original workspace under the same policy used for removal failures from a returned session.
- A returned Pi or OpenCode HarnessSession.end() rejects on producer kill, tailer kill or root removal, with hard-stop, gate-bypass or in-flight release selected. → Set the ending unconfirmed before release policy and retain the original workspace and any remaining producer paths.
- Record draining fails during endHarness after a returned session's end has completed. → Retain the original workspace rather than deriving release authority from an unreadable ending record.
- A finish-plan find or end operation rejects with an unknown or fenced outcome rather than a confirmed container-gone verdict. → Preserve the unconfirmed state through the outer catch and retain the original workspace without a replacement controller or raw-PID fallback.
- An already-launched host Pi or OpenCode fails its local ending inside open() or after handoff. → Retain remaining private paths on unknown shutdown; carry an unconfirmed outcome without bypassing known-child or foreign-port checks.
- Startup is refused before any producer is launched, or the original producer ending and record drain complete normally. → Do not reopen startup or model admission; preserve the existing no-producer or settled-ending release path without fabricating an atomic physical-quiescence receipt.
| Prior finding | Resolution | Evidence at this head |
|---|---|---|
| review:5423096865:F1 | fixed | Verified all 11 prior cases at this head. Cases 1–2: both repo-resident and repo-cold factories still use ExecHarnessContainer with the original executor; PiRpcTransport.write routes only typed abort through cancelInput while prompt/start admission remains guarded. Case 3: host cancelInput reaches the tracked child's stdin and kill uses the existing known-child TERM/KILL path independently of the work scope. Case 4: OpenCode's typed interrupt selects cancelRequest, and server/tailer kills use lifecycle control on both exec factories. Case 5: host cancelRequest accepts the tracked original port and rejects foreign ports; fresh launch and tailer restart both pass the same server port to the tailer, so the shared byDir entry does not break cancellation, and byPid retains both kill targets. Cases 6–7: absolute deadline/current lease exhaustion no longer guards cancellation, observation, kill or safe removal, while work methods remain admission-bound. Case 8: runLoop's finish-plan factory preserves its original executor and find/end use the control observation/shutdown paths despite exhausted admission. Case 9: control captures one OP_TIMEOUT_MS deadline before send and a permitted reset retry subtracts elapsed time. Case 10: host makeRoot/writeFile/start and exec write/start still refuse stopped/expired work; OpenCode readiness keeps its stop/deadline checks. Case 11: foreign harness/container checks precede adoption or cleanup, and unknown recorded probe/session/feed collection still propagates without local kill/remove or rival startup. This closes the admission-blocks-shutdown invariant, not the separate opening-cleanup retention defect reported as F2; no atomic descendant-stop or durable launch ACK claim is inferred. |
Full review
F2 (high confidence): Both harnesses call their local end() when open() fails. If a launched loop fails and that cleanup’s kill returns an unknown outcome, open() rejects before harnessSession is assigned (runLoop.ts:2905). The outer catch then awaits harnessSession?.end()—a no-op—and marks the ending settled. An in-flight command or hard stop can therefore force workspace teardown despite the unconfirmed shutdown, contradicting the new retention contract. Carry the local cleanup outcome explicitly to dispatch and set unconfirmed before release policy; do not treat absence of a returned session as successful termination.
d9c7551 to
35999fd
Compare
There was a problem hiding this comment.
LGTM: F2 is resolved at the exact reviewed head; no remaining major or critical delta findings.
Note
Approved · head 35999fd · no findings
| Prior finding | Resolution | Evidence at this head |
|---|---|---|
| review:5423590120:F2 | fixed | Verified all nine prior cases at this head. Cases 1–4: pi/harness.ts and opencode/harness.ts preserve the original opening error and carry failed producer kill, tailer kill or root removal as HarnessEndingUnconfirmedError before session handoff; runLoop.ts recognizes it before release and does not equate missing harnessSession with confirmed ending. Both repo-resident and repo-cold share the same ExecHarnessContainer and harness paths. Case 5: returned-session end failures still set unconfirmed before mandatory hard-stop, gate-bypass, in-flight or read-only release. Case 6: ending record-drain failure sets unconfirmed through endHarness's outer catch. Case 7: finish-plan find/end unknown or fenced failures set unconfirmed, which the outer catch preserves; confirmed container-gone remains distinct. Case 8: the same opening wrappers protect host failures while BotHostHarnessContainer retains its known-child, original-port and private-root checks. Case 9: clean session end/drain retains settled release, and prelaunch readiness checks continue refusing stopped/expired startup without launching a producer. Expanded sweep: Pi unknown launch/probe and failed recorded kill/removal retain custody and prevent fresh start; transport-replacement cleanup failures are no longer swallowed. OpenCode launchOpenCode retains unknown server/tailer starts, tracks acknowledged children for readiness cleanup and propagates failed cleanup; the outer open catch has no server to end on those failures, so the marker survives unchanged. Fresh create/import failure occurs after server assignment and uses the protected local ending. Recorded probe/session/feed uncertainty, failed tailer restart and refused-continuation server/tailer/root cleanup keep negative custody until handoff or successful cleanup. runLoop unwraps openingError for original typed failure, gate/refusal and interruption handling; hard-stop control and final record/status/card/accounting paths remain unchanged. F1's admission/control separation remains intact, with no controller replacement or raw-PID fallback. This closure establishes negative-custody propagation, not durable launch acknowledgment or physical descendant quiescence. Test-guard dispositions: all ten heuristic groups are refactor, verification intact. RunLoop end-failure retitle retains failed-status/error assertions; loop-plus-end-failure retitle retains original-error precedence and harness_error recording; interruption retitle retains status/card agreement. Their unconditional teardown criterion is explicitly retired by harness.md item 13, with independent end/drain/opening retention and settled-release tests. Host kill retitle retains TERM/KILL, descendant and ended-PID idempotence proofs; unknown-PID no-op is replaced by explicit ownership refusal. Container stdout retitle retains exact streams and named exit failure with truncation proof; runtime-word retitle preserves typed replacement positives while presentation authority is explicitly retired; tail retitle retains alive/kill proofs and independently asserts failed collection. Contract remove retitle retains PID/root/legacy-root assertions and replaces swallowed removal with independently asserted propagation. Process info-timeout retitle retains named timeout and start count; tailer-readiness retitle retains named timeout, early-exit stderr and connected-note byte offset. Post-expiry diagnostic commands are explicitly retired by the original-deadline contract, with setup/expiry controls retaining independent proof. |
Full review
F2 is resolved at 35999fd8; no remaining major or critical delta findings, while durable launch acknowledgment and physical descendant quiescence remain unproven.
Harness operations preserve typed outcomes and original work bounds. Cancellation remains available after admission ends; unconfirmed endings retain custody before and after session handoff.
Why: Rendered command text could decide execution state, retries could renew their timeout, and missing session handoff or unknown OpenCode reads could authorize workspace teardown or a second producer.
Where to look
Feedback wanted: Check uncertainty propagation and the unchanged original lease authority. Report major or critical findings.
Risk: Control completion does not prove atomic descendant quiescence. Unknown end or record drain retains the workspace; durable producer ACK and native capture remain explicit gaps.
Verified: 915 named tests including 8 real local pi/OpenCode fixtures; scoped types/lint/format, consistency, coverage/test guard and 44 screenshot hashes pass.
Decisions (6)
Validation (7 criteria)
For agents
Review this narrow source change only. No runtime replacement, shared RunState/store/Docker change, deployment, production model probe or private-state cleanup. Real local fixtures use fake model endpoints; they do not prove native physical cancellation, durable launch custody or whole lifecycle acceptance.