fix: stop failing live proof before finite commands finish - #1278
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 28, 2026, 4:58 AM ET / 08:58 UTC. ClawSweeper reviewWhat this changesThe PR makes finite terminal proof commands wait for their configured completion budget before evaluating output assertions, and preserves usable PTY standard I/O for detached child processes. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessKeep open: this is a necessary, narrowly scoped repair to terminal live-proof execution, and the reviewed introduced changes have no concrete correctness or security blocker. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherClawSweeper's live-proof driver runs a reviewed repository command in a controlled terminal and turns its output, exit state, and cleanup result into a verification record. The driver feeds the review/publication workflow, while the prompt context tells the planner what terminal behavior and budget it may rely on. flowchart LR
A[Reviewed proof plan] --> B[Terminal proof driver]
B --> C{Completion contract}
C -->|Finite command| D[Exit status and sealed output]
C -->|Long-running command| E[Readiness and liveness]
D --> F[Verification result]
E --> F
D --> G[Cleanup watchdog]
E --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the existing overall proof budget while using authoritative finite-command completion and sealed output for exit-zero plans, with readiness/liveness behavior unchanged for long-running plans. Do we have a high-confidence way to reproduce the issue? Yes—the source and supplied proof describe a deterministic terminal-driver scenario where a finite command emits its required summary after the former 30-second observation window; this review did not execute it. Is this the best way to solve the issue? Yes—the branch reuses the existing completion and budget owner instead of introducing a second timeout, while retaining failure for nonzero exits and missing output. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 777f3211cd5b. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
@clawsweeper re-review Final head: The bounded real-PTY command passed on this head. For this demonstration, use this exact plan; its observations deliberately exclude cosmetic reporter prefixes: {
"status": "recommended",
"surface": "terminal",
"terminalCompletion": "exit_zero",
"reason": "The selected regression executes the real terminal driver and verifies detached-child PTY descriptors and cleanup.",
"payoff": {"kind": "static_text", "justification": "The observed test result is a concise terminal summary."},
"entry": "pnpm run build:all && node --test --test-reporter=spec --test-name-pattern='terminal proof preserves inherited PTY descriptors in a detached Node child' test/live-proof-review-environment.test.ts",
"steps": [
{"action": "expect_output", "text": "terminal proof preserves inherited PTY descriptors in a detached Node child"},
{"action": "expect_output", "text": "pass 1"}
]
}This retains both a real observed outcome and successful terminal completion; no timeout, heap limit, missing-output check, or proof gate is relaxed. The main body includes the full before/after evidence and final-head CI. |
|
🦞👀 Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
ba73c41 to
d5780ab
Compare
|
Land-ready verification for The current-head ClawSweeper review reports no correctness/security findings, sufficient real-behavior proof, and
The hosted command was: pnpm run build:all && node --test --test-reporter=spec --test-name-pattern='terminal proof preserves inherited PTY descriptors in a detached Node child' test/live-proof-review-environment.test.tsThe combined local boundary/sibling command was: node --test --test-reporter=spec test/live-proof.test.ts test/review-prompt-context.test.ts test/review-prompt-policy.test.ts test/live-proof-package-version.test.ts test/live-proof-report.test.tsThe main body retains the real 35-second before/after fixture and actual OpenClaw-wrapper positive/negative/verbose replays. The original slow-test names arrived at file completion; the old 30-second observation stopped too early. This fix reuses the existing completion/budget owner. Missing output after exit zero remains nonpassing; readiness/liveness, cleanup fencing, heap limits, test timeouts, and the proof budget are unchanged. Integration with the newer pnpm-output fix required a documentation-only conflict resolution plus two test corrections: align the new negative case's diagnostic and remove a redundant wall-clock-dependent sleep assertion while retaining the stronger deterministic timing/cleanup test. The terminal driver remains byte-identical to the completed process proof (SHA-256 The unrelated local action-ledger concurrency hang remains separately owned and honestly disclosed; its unchanged case passed hosted Linux CI. Publication recovered through the normal queue—no workflow disabling, manual label, credential substitution, or gate override was used. Bay is unaffected. This PR does not modify or merge the OpenClaw migration PR. |
Additional instructions
This PR uses a branch in the same repository, which maintainers with repository write access can edit. GitHub reports
maintainerCanModify: false; no fork permission setting was changed.What Problem This Solves
Fixes an issue where ClawSweeper could report a failed live verification while a healthy finite command was still running. The observed OpenClaw migration probe declared
terminalCompletion: exit_zero, but its output assertion stopped after 30 seconds, before the test file and reporter finished.The original premise needs an important qualification: the normal Vitest reporter did emit both slow test names at file completion. The complete Linux log records 49,727 ms for the memory-test file, with batch/deep cases taking 24,499/24,869 ms. The focused run passed 125 tests with two existing skips. The defect was premature observation, not proof that those names never appear or that the migration failed.
This PR does not edit, review for landing, or change the limits of the OpenClaw migration PR.
Why This Change Was Made
Reuse the existing terminal completion owner rather than adding another timeout or supervisor. Expectations for finite commands now wait for authoritative exit zero and sealed, controller-observed output within the existing overall proof budget. This includes finite setup commands before a final long-running command; multiple expectations reuse the completed capture. The final
ready_while_runningcommand retains its existing readiness window, stability hold, and liveness checks.The planner receives the existing effective terminal execution limit and the trusted fact that terminal standard I/O is a PTY, and must inspect the exact command/wrapper/reporter contract. It prefers final summaries for finite tests, distinguishes late slow-test-name reporting from progress, and reports a concrete harness limitation when a meaningful scenario cannot fit. A successful command never waives a missing assertion: the proof remains unverified with an assertion-mismatch diagnostic, not an inferred product defect.
Real-wrapper validation also exposed a small supervisor bug on macOS: inherited
/dev/ttydescriptors became unusable when Node started a detached child session. Child standard I/O now opens the already-bound concrete PTY path. PID/TTY/lease/nonce validation, capture, and cleanup ownership remain unchanged. The test process-discovery helper filters before capture so unrelated long host command lines cannot overflow its buffer.No new configuration, dependency, schema, public verification format, heap allowance, or execution/test timeout. OpenClaw's original 256 MiB heap and 180-second case limits are untouched.
The final
git diff --numstat origin/main...HEADseparates a small runtime change from its regression coverage: production TypeScript is 30 additions / 2 deletions across two files; tests are 200 additions / 34 deletions across five files. The production planner prompt is behavior-bearing text, counted separately at 19 additions / 3 deletions. Documentation and changelog account for 34 additions / 6 deletions. The larger test share exercises parsed plans through the driver/verifier and detached-child PTY behavior.User Impact
Healthy finite commands can finish and provide their real result instead of failing an unrelated 30-second progress-text wait. Missing observations, nonzero exits, signals, expired budgets, and failed cleanup remain nonpassing. Detached Node subprocesses retain usable terminal descriptors on macOS.
OpenClaw Bay Impact
Bay is unaffected. The verification format, publication identity, and observer-only lifecycle surfaces are unchanged; this corrects existing execution semantics and planning guidance.
Documentation Impact
Updated the active
docs/live-proof.mdreference and production review prompt with completion-aware/reporting-aware planning, the existing execution limit, and concrete PTY descriptor semantics. The reference retains its review/publication-maintainer ownership and source-of-truth/update triggers. The unreleased ClawSweeper changelog entry now describes strict observation semantics rather than the retired missing-marker success behavior.Evidence
Current mechanical refresh targets
d5780abb16ffebe2bea30da382f4a0d4354f95a6, rebased onto777f3211cd5beaf2dce3b3ecca08cd5d3a0818e8. The sole manual resolution was indocs/live-proof.md, preserving upstream pnpm 11 structured-version guidance alongside finite-completion, reporter, and PTY guidance. The original three commit messages are unchanged; their range-diff shows only integration context changes. A separate test-only integration commit aligns the upstream negative-case diagnostic and removes a redundant timing assertion; no runtime edits were made. This PR's production TypeScript files are byte-identical toba73c412, including driver SHA-2562b8656cfdd824b1c82b27aa92be229e1e4ae4223aed3aa66202e941313c05cfb. The full upstream range also brings the already-merged report parser/renderer fix from PR 1277; those two production files exactly match main. No runtime, schema, dependency, budget, heap, or Bay contract was changed by the refresh.On the final tree, Node 24.20.0 and pinned pnpm 11.10.0 passed
pnpm run build:all,pnpm run check:static(including format/docs checks),pnpm run lint, and diff checks. All 253 combined executor/context/prompt-policy/package-version/report cases passed, with zero failures or skips, in 40.318 seconds. The explicit-spec real detached-child PTY command below passed on committed headd5780abb16ffebe2bea30da382f4a0d4354f95a6: 1 test, 0 failures, 0 skips, in 9.768 seconds (selected case: 9.419 seconds). Fresh isolated Codex autoreviews of the uncommitted test correction and then the committed branch againstorigin/mainboth passed with no actionable P0 findings; mandatory reviewer isolation, guard, and outgoing-pack credential scanning were retained.The first mechanically rebased run exposed two test integration issues (251/253 passed; a 215-case retry still failed the timing assertion). The package-version negative case correctly rejected the absent literal but expected the retired diagnostic. It now checks the canonical
proof-plan assertion mismatchdiagnostic while retaining failed status, unsatisfied expectation, actual pnpm display, child-failure checks, and all negative cases. The generic completion test redundantly required an exactsleep 6, although the production hold subtracts real elapsed recording time. Only that duplicate hold variable/assertion was removed; completed status, respawn, andUsageremain. The stronger frozen-clock test still verifies authoritative completion before the exact six-second hold, recorder finalization, and cleanup ordering. No tolerance, timeout, clock, or production behavior changed. The earlier Node 26.7.0 PTY proof also passed (8.501 seconds); those initial failed test runs are preserved as history, not counted as passing gates.The previous hosted review/proof passed five selected cases and published its review for the prior head. The current-main refresh addresses its sole rank-up request; previous CI and hosted review are historical continuity, not approvals for this changed head/body. See current PR Checks for new-head status. No new CI or hosted-review success is claimed.
pnpm run build:all,pnpm run check:static(including documentation and formatting),pnpm run lint, andgit diff --check origin/main...HEADpassed on that earlier publication head. An initialcorepack pnpm run build:allinvocation stopped before compilation because nested pnpm resolved a conflicting global version; retrying with directpnpm, which resolves the repository's existing 11.10.0 pin, passed without source, configuration, or dependency changes.cb1fbbd0fa2856ad73555b29b15b6a95c9f233ff: delayed default-summary proof passed in 43.371 s, the deliberately missing fast-test-name assertion was correctly rejected despite 11 passing tests, and explicit-verbose proof passed in 6.320 s. No heap, test timeout, execution budget, or cleanup limit changed.a2f044f819andba73c412; it covers the trusted-PTY-context correction and is historical continuity, not CI for the refreshed head.pnpm run checkdid not pass: an untouched action-ledger opposing-parent-edge concurrency test stalled and the owned run was stopped. An independent helper-repair lane reproduced the same local stall. That same unchanged case passed in the hosted full check in 397 ms. The local hang remains separately owned; no ledger code, timeout, or assertion was changed here.Previous publication validation targeted head
ba73c4126a46a5d91d009ebaeda85a4ff61c7919on baseaf8f89fe69e1e35009591a640c55d12a4a083cf9. That earlier rebase was conflict-free and preserves the separate invalid-live-proof-artifact refresh on main. Follow-ups after thecb1fbbdreal-wrapper replay remove one obsolete example assertion and add the trusted PTY planning fact with documentation/tests; production driver bytes still match the proof hash below. The PTY fact regression was run red before its producer fix; all 215 focused executor/context/policy tests, builds, static checks, and lint then passed, as did fresh precommit review. The explicit-spec real-PTY command below passed on that previous head. Exact-head CI, fresh committed review, and the current ClawSweeper disposition control landing; no stale review or failed check is treated as a pass.Current focused command:
Earlier real PTY sibling command (not rerun in the publication pass):
Real Behavior Proof
Narrow hosted verification target
The ClawSweeper code reviews found no code or security defects and judged the supplied process proof sufficient. The first automated probe nevertheless selected the entire multi-minute suite. The next hosted probe selected the correct real-PTY case, which passed in 2.991 seconds with a successful command exit, but mixed a TAP-specific expectation with Node's normal spec-reporter output. That formatting mismatch is not a product or test failure.
This exposed one missing producer fact: the trusted planner context now explicitly supplies
terminalStdio: "pty", alongside the existing execution budget. Reporter defaults can differ from redirected logs, and wrappers can pipe their own child streams. The extended context regression first failed because this fact was absent and also rejects PR-authored claims that standard I/O is a pipe.For a bounded hosted run, this exact command selects one real production-driver, detached-child PTY case, rather than every setup/environment fixture. It previously passed on head
ba73c4126a46a5d91d009ebaeda85a4ff61c7919; the selected case took 11.291 seconds locally, after the build. The spec reporter is explicit. Use exactly these format-independent observations:terminal proof preserves inherited PTY descriptors in a detached Node childandpass 1, withterminalCompletion: exit_zero. Cosmetic reporter prefixes are not part of the assertions. No fake marker, repeat invocation, timeout increase, or proof override is needed.The same selected real-PTY command was rerun successfully on current head
d5780abb16ffebe2bea30da382f4a0d4354f95a6with Node 24.20.0, taking 9.768 seconds overall. The recorded successful output contains the selected test name,pass 1,fail 0, andskipped 0; its decorative prefix is deliberately excluded from the expected substrings.The test launches a real detached Node child through the production
driveTerminalandmediaProofCommandRunner, verifies character-device/TTY stdin, stdout, and stderr plus clean child exit, observes real output, and checks process/capture cleanup. The delayed-output change itself additionally has the real 35-second before/after and real OpenClaw-wrapper evidence below. This addresses the review's request for after-fix terminal proof without asking the old outer driver to finish a multi-minute suite within its 30-second observation window.Completed terminal and wrapper proof
Claim and surface: the production terminal driver waits for a finite command's real completion and output, without turning successful exit alone into proof. Controlled local macOS execution used private tmux sockets, an isolated HOME/TMPDIR for the OpenClaw replay, no provider/channel credentials, and no recording or external media upload.
Before/after fixture: a harmless finite process emitted startup output, remained quiet for 35 seconds, then emitted its final summary and exited zero. The pre-fix production driver failed its 30-second expectation; the repaired driver passed in 40.804 seconds including supervision/cleanup, using the unchanged 90-second proof budget. The fixture bytes were identical before/after (SHA-256
bd0f6fec467eae871c1bf18a50ecf879f7399b50f7f7e1bca0cf930c6a7507f7).Real OpenClaw wrapper replay: plans were parsed and executed through the production terminal driver/verifier from the clean published ClawSweeper head
cb1fbbd0fa2856ad73555b29b15b6a95c9f233ff, against OpenClaw9458d8b18105a9aa18ef334d50c6974c83f55acd, Node 24.20.0, tmux 3.7c, and Vitest 4.1.11. Each command ran the real repository wrapper with its original test behavior:Test Files,Tests, and[test] passedassertions satisfied after process completion; whole proof 43.371 s--reporter=verbose, requesting the emitted name plus final wrapper summaryRepresentative captured default-reporter result:
Additional integration evidence: a separate helper-repair lane replayed actual Codex-generated fast and 34-second delayed plans against this exact driver source (SHA-256
2b8656cfdd824b1c82b27aa92be229e1e4ae4223aed3aa66202e941313c05cfb). Both passed seven controls: generated success, duplicate invocation rejection, intentional rerun, exit 7, zero exit with missing output, finite deadline, and readiness timeout. That integration covers the driver, not this PR's updated planner prompt/runtime context; those have separate focused coverage. The helper changes are not included here.Limits: this is real local process/PTY and wrapper proof, not a new migration, authenticated-provider, browser, media, or production-deployment test. Initial concurrent macOS PTY attempts hit the existing cleanup cap; serial replays passed without raising it. Raw host logs and agent transcripts are not published. Public execution evidence is the sanitized observations above; the existing terminal verification artifact schema is unchanged.