fix(runs): repair false opencode timeouts (BLO-22922) - #1146
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
🔗 Paperclip issue: BLO-22922 |
1 similar comment
|
🔗 Paperclip issue: BLO-22922 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@allyblockcast please review exact head Focus on the exit-zero timeout invariant across adapter and server, migration |
8d22b70 to
63c0305
Compare
|
@allyblockcast please review rebased exact head The only failure on the obsolete run was ARC runner DNS during checkout ( |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 63c0305
Critical Issues (0)
Important Issues (3)
- [pr-review-toolkit]
server/src/services/heartbeat.ts:8498— The false-timeout exception suppresses everyerrorMessagewhenevertimedOutaccompanies exit code 0, even when the error is unrelated to timeout handling. A validation, publication, or post-processing error can therefore be persisted as success merely because the malformed timeout bit is also set.- Normalize the contradictory timeout marker separately, then continue to honor independent adapter errors. If timeout errors need special handling, identify them through structured timeout metadata or a timeout-specific error code instead of discarding arbitrary error text.
- [native-codex]
server/src/services/heartbeat.ts:8499— The new helper does not establish the promised persisted-success invariant for the exact regression fixture. The added test usesresultJson: null, but heartbeat finalization subsequently appliesisEmptyResultand changes the outcome tofailed; because run error fields were computed before that override, the resulting failed row can also have null error metadata. The helper-only test therefore passes while the persisted result remains wrong.- Normalize and finalize the outcome before deriving status/error fields, and add a service-level persistence test that feeds the contradictory adapter result through heartbeat finalization and asserts status, exit code, error fields, wake status, and result metadata.
- [gstack/review]
packages/db/src/migrations/0212_repair_successful_runs_mislabeled_timed_out.sql:16— The migration repairs anopencode_k8sincident using a predicate that covers every adapter, then unconditionally removes genericmessageandstopReasonkeys.messageis a first-class generated result column and can contain valid successful output, so affected historical runs can be relabeled successful while their useful output is destroyed.- Restrict the repair to rows positively attributable to the affected adapter/version or otherwise prove the global invariant, and remove only timeout-specific keys/values. Preserve generic output unless its value is demonstrably stale timeout text; extend the migration test with preserved successful message content and an unaffected non-
opencode_k8srow.
- Restrict the repair to rows positively attributable to the affected adapter/version or otherwise prove the global invariant, and remove only timeout-specific keys/values. Preserve generic output unless its value is demonstrably stale timeout text; extend the migration test with preserved successful message content and an unaffected non-
Suggestions (0)
Strengths
- The adapter is pinned to a full immutable commit and both Docker pin contracts were updated consistently.
- Genuine timeout cases with null or nonzero exit codes remain covered.
- The migration updates
result_jsonrather than attempting to write generated columns directly, and its matching predicate is mechanically idempotent after status correction.
Recommended Action
- Address the Important issues before merge.
63c0305 to
e619d42
Compare
|
@allyblockcast please review exact head This head addresses the review concerns by narrowing timeout-error suppression to structured canonical timeout evidence, repairing linked wake status atomically, and excluding ambiguous result/error payloads from migration |
e619d42 to
4514874
Compare
|
@allyblockcast please review exact head This head fixes all three Important findings from the |
|
@allyblockcast please review exact head |
|
@allyblockcast correction: please review exact head |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c4363be
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The previous independent-error blocker is resolved: false-timeout normalization now requires
errorCode: timeoutand the exact timeout message shape, so publication or validation errors still fail. - Empty-result finalization now occurs before status and error derivation; persistence tests cover both the malformed timeout success and ordinary empty-result failure paths, including wake status.
- Migration 0212 is limited to
opencode_k8s, exit-zero timeout-shaped rows, leaves ambiguous/non-opencode rows untouched, and preserves generic result content. - The adapter pin is immutable and both pin contracts remain synchronized.
- All PR checks are green; targeted local tests report 13 passing, with Postgres-backed cases covered by CI.
Recommended Action
Merge.
ally-verdict: pass
|
Pre-merge note on the pin move (recorded while verifying this PR against a duplicate, #1160, now closed). This repin is a divergence, not a fast-forward. The outgoing pin So moving to That is inert in production today: the env-guard plugin is canary-gated behind Follow-up: re-land |
Superseded at c4363be: the App review found an unresolved adapter-pin regression on this same head.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c4363be
Prior Findings Dispositioned (3)
- prior:63c0305 important 1 — fixed —
server/src/services/heartbeat.ts:8492— false-timeout normalization now requires both the structuredtimeoutcode and the canonical timeout-message shape, so unrelated adapter errors still fail. - prior:63c0305 important 2 — fixed —
server/src/services/heartbeat.ts:22548— false-timeout and empty-result finalization now occurs before error/status derivation; the persistence test atserver/src/__tests__/heartbeat-timeout-outcome-persistence.test.ts:129verifies run and wake state for the exact contradiction. - prior:63c0305 important 3 — fixed —
packages/db/src/migrations/0212_repair_successful_runs_mislabeled_timed_out.sql:24— the repair is now restricted toopencode_k8s, preserves generic result content, and excludes rows carrying ambiguous result/error payloads.
Critical Issues (0)
Important Issues (1)
- [native-codex]
Dockerfile:356— The repin is not a fast-forward and drops the outgoing pin's env-guard hardening. GitHub reports83197d46...8f427267as diverged with8f427267one commit behind; the omitted83197d46changed shell unwrapping sosh -c env ignoredandbash -c "env" ignoredare classified from the command string rather than positional arguments. At8f427267,SHELL_WRAPPER_REcaptures the entire unquoted remainder, soenv ignoredno longer matchesFULL_ENV_DUMP_REand the guard allows the dump whenenvGuardPluginis enabled. The new Dockerfile comment incorrectly says the pin carries this fix.- Re-land
83197d46on the adapter branch containing8f427267, verify the env-guard corpus, and pin Paperclip to that descendant so the timeout repair does not regress the existing secret-dump guard.
- Re-land
Suggestions (0)
Strengths
- The server now preserves independent errors while rejecting the exact malformed timeout tuple.
- Persistence coverage exercises both corrected success and ordinary empty-result failure, including linked wake state.
- Migration 0212 is guarded, auditable, generated-column-safe, and covered against genuine timeout, ambiguous, unrelated-adapter, and existing-success rows.
- All current PR checks are green.
Recommended Action
- Address the Important issue before merge.
|
Supplementary exact-head finding from a concurrent pass; the formal App review on this head remains the operative verdict.
Exact head: |
c4363be to
3afa217
Compare
|
@allyblockcast please review exact head This head addresses the supplementary finding by refusing false-timeout normalization whenever structured result evidence reports an error ( |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3afa217
Prior Findings Dispositioned (1)
- prior:c4363be important 1 — fixed —
Dockerfile:19— The repin now targets6dca0201547f962dc9ae45576c81c12808b73bb3, a linear descendant of8f4272675db81e95bf393679a912d9037df3d9ab(git compare 8f427267...6dca0201reportsahead_by:3, behind_by:0), and itssrc/server/env-guard-plugin.ts/.test.tspatches are byte-identical to83197d46's (verified via the GitHub commits API — diff of the two patches is empty). Dockerfile comment and both pin-contract tests (scripts/opencode-k8s-runtime-cache-pin.test.js,server/src/__tests__/docker-opencode-runtime-pin.test.ts) now assert the new SHA plus the grace-window and shell-command-string comment text, and assert8f427267is absent. The env-guard hardening and the BLO-22922 timeout fix now ship from one descendant.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The supplementary finding is resolved correctly:
hasStructuredAdapterFailureEvidence(server/src/services/heartbeat.ts:8492) now makesisFalseAdapterTimeoutResultreject the malformed-timeout normalization wheneverresultJsoncarriesis_error/isError/success:false/ok:false, a failure-shapedtype/subtype/status/outcome/stopReason, or a non-emptyerror/errors/errorMessage/errorCode— exactly the{ error: "publication failed" }case the supplementary comment raised. - New persistence coverage (
server/src/__tests__/heartbeat-timeout-outcome-persistence.test.ts) exercises all three shapes end-to-end against a real Postgres instance: the genuine exit-zero timeout contradiction →succeeded, the same contradiction plus structured failure evidence →failedwith the originalerror/errorCodepreserved, and the empty-result path →failed/EMPTY_RESULT, each cross-checked against the linkedagent_wakeup_requestsrow via a pollingwaitForWakeToFinishrather than a fixed delay. - Migration
0212's exclusion list (?| ARRAY['error','errors','errorMessage','errorCode','message','result','summary','is_error','isError','success','ok','type','subtype','status','outcome','stop_reason']) is a superset of the server-side evidence check, so it conservatively leaves any row carrying possible structured content astimed_outfor operator review rather than risking a false repair; migration test coverage coincides with the persistence tests' fixtures (real-timeout, publish-failure,is_errorexecution-failure, and already-good-success rows all left untouched or correctly repaired). effectiveErrorMessage's dead assignment was removed cleanly — no remaining references in the file — and theemptyResultOverridecheck was hoisted above the error-message ternary soEMPTY_RESULTnow flows through botherroranderrorCodeconsistently, whereas before only the message was overridden.- All PR checks other than the still-running CI matrix are green (Helm chart, policy, review, security-review); no code search hits remain for the retired
83197d46/8f427267pins.
Recommended Action
Merge once the pending CI matrix completes.
ally-verdict: pass
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3afa217
Prior Findings Dispositioned (1)
- prior:c4363be important 1 — fixed —
Dockerfile:19— The repin now targets6dca0201547f962dc9ae45576c81c12808b73bb3, a linear descendant of8f4272675db81e95bf393679a912d9037df3d9ab(git compare 8f427267...6dca0201reportsahead_by:3, behind_by:0), and itssrc/server/env-guard-plugin.ts/.test.tspatches are byte-identical to83197d46's (verified via the GitHub commits API — diff of the two patches is empty). Dockerfile comment and both pin-contract tests (scripts/opencode-k8s-runtime-cache-pin.test.js,server/src/__tests__/docker-opencode-runtime-pin.test.ts) now assert the new SHA plus the grace-window and shell-command-string comment text, and assert8f427267is absent. The env-guard hardening and the BLO-22922 timeout fix now ship from one descendant.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The supplementary finding is resolved correctly:
hasStructuredAdapterFailureEvidence(server/src/services/heartbeat.ts:8492) now makesisFalseAdapterTimeoutResultreject the malformed-timeout normalization wheneverresultJsoncarriesis_error/isError/success:false/ok:false, a failure-shapedtype/subtype/status/outcome/stopReason, or a non-emptyerror/errors/errorMessage/errorCode— exactly the{ error: "publication failed" }case the supplementary comment raised. - New persistence coverage (
server/src/__tests__/heartbeat-timeout-outcome-persistence.test.ts) exercises all three shapes end-to-end against a real Postgres instance: the genuine exit-zero timeout contradiction →succeeded, the same contradiction plus structured failure evidence →failedwith the originalerror/errorCodepreserved, and the empty-result path →failed/EMPTY_RESULT, each cross-checked against the linkedagent_wakeup_requestsrow via a pollingwaitForWakeToFinishrather than a fixed delay. - Migration
0212's exclusion list (?| ARRAY['error','errors','errorMessage','errorCode','message','result','summary','is_error','isError','success','ok','type','subtype','status','outcome','stop_reason']) is a superset of the server-side evidence check, so it conservatively leaves any row carrying possible structured content astimed_outfor operator review rather than risking a false repair; migration test coverage coincides with the persistence tests' fixtures (real-timeout, publish-failure,is_errorexecution-failure, and already-good-success rows all left untouched or correctly repaired). effectiveErrorMessage's dead assignment was removed cleanly — no remaining references in the file — and theemptyResultOverridecheck was hoisted above the error-message ternary soEMPTY_RESULTnow flows through botherroranderrorCodeconsistently, whereas before only the message was overridden.- All PR checks other than the still-running CI matrix are green (Helm chart, policy, review, security-review); no code search hits remain for the retired
83197d46/8f427267pins.
Recommended Action
Merge once the pending CI matrix completes.
ally-verdict: pass
2 participants
Thinking Path
Linked Issues or Issue Description
Paperclip work item: BLO-22922
What happened? With
opencode_k8s.timeoutSecset to a positive value, the adapter armed a 30-second Job-condition grace timer at Job launch. A healthy Job that ran longer could later exit 0 while retainingtimedOut: true; Paperclip stored that run astimed_out.Expected behavior: The grace period begins only after pod log streaming exits, and a confirmed exit code 0 is persisted as success even if a malformed adapter also reports a timeout marker.
Steps to reproduce:
opencode_k8sagent with a positivetimeoutSec.heartbeat_runs: the row can havestatus = timed_outandexit_code = 0.Paperclip version or commit: Reproduced on Blockcast Paperclip master before
c794754d6.Deployment mode: Docker, self-hosted server, external Postgres, custom external plugin adapter.
What Changed
opencode_k8sto reviewed adapter commit8f427267, which starts grace only after log-stream exit and rejects timeout on exit 0.timedOut: truecannot override a confirmedexitCode: 0, while preserving genuine timeout and failure outcomes.0212to repair all 1,190 historical contradictory rows, clear stale timeout/error/liveness fields, and annotateresult_json.outcomeCorrection.Verification
pnpm exec vitest run server/src/__tests__/heartbeat-timeout-outcome.test.ts server/src/__tests__/docker-opencode-runtime-pin.test.tsnode --test scripts/opencode-k8s-runtime-cache-pin.test.jspnpm --filter @paperclipai/db typecheckpnpm --filter @paperclipai/server typecheckgit diff --check origin/master...HEADkkroo/paperclip-adapter-opencode-k8s#55: 606 tests, typecheck, build, and diff check pass.UPDATE 3, zero contradictory rows afterward, transaction rolled back.Risks
timed_outrow withexit_code = 0. That pair is contradictory under both the repaired adapter and server invariant; each correction remains auditable inresult_json.Model Used
gpt-5.6-sol, ultra reasoning effort, with repository, shell, GitHub, Kubernetes, and database tool execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template