Skip to content

review: Keep incomplete execution out of completed review caches #35

Description

@idy

Background

In GizClaw/gizos#257, review run 34315694559 reported supplied stage inputs under the runner temporary directory as unreadable without attempting to read them. The rollout's read-only permissions allowed reading /; this is not evidence of an actual sandbox denial. The model returned structurally valid policy blockers, which the orchestrator saved as completed evidence. A new review request reused that result with zero model tokens, so the review could not recover.

Goal

Distinguish completion of review execution from the review's pass/fail verdict. A failed attempt must keep eligibility blocked while allowing a new request to retry unfinished work. Existing cached results from the earlier output contract must not perpetuate the failure.

Non-goals

Do not broaden sandbox permissions, enable network access, change the review model, bypass eligibility checks, or retry legitimate completed findings merely because they block merging. Consumer pin updates and GizOS product changes are separate work.

Code Changes Tree

.github/scripts/pr-review/
├── common.mjs                   # increment the ledger schema to invalidate legacy completed evidence and chunk checkpoints
├── run.mjs                      # preflight explicit inputs, instruct read attempts, and reject incomplete execution before checkpointing
├── stage-output-schema.json     # require execution status and reason for PR and Issue model outputs
├── review-output-schema.json    # require execution status and reason for code and aggregation model outputs
└── test.mjs                     # cover failures and same-head recovery for every stage, valid blocker reuse, and legacy invalidation
README.md                       # document execution status, fail-closed recovery, and one-time cache invalidation

Design

Execution contract

Each model response includes execution: {status: "completed" | "incomplete", reason: string}. Completed execution requires an empty reason and may still return legitimate findings or policy blockers. Incomplete execution reports the concrete inability to perform the requested review, rather than encoding that inability as a policy blocker. Missing or malformed execution status fails validation.

Before each turn, the orchestrator checks readability of the exact supplied input paths. Trusted orchestration instructions identify these paths as explicit read-only inputs even outside the working directory and require an actual read attempt before declaring access unavailable. Their contents remain untrusted; the existing restrictions on credentials, network, writes, and executing PR code remain in force. Host preflight does not guarantee model-side access or replace model-side failure reporting.

Persistence and recovery

Reject incomplete execution before saving the turn as completed stage evidence or a completed chunk. Preserve earlier successfully completed checkpoints. The existing execution-failure path reports failure and prevents a final eligibility verdict; a new request retries unfinished work. Completed reviews with real blockers continue to reuse evidence with zero model tokens when their identities match.

Ledger schema v4 rejects the prior ledger, including completed generations, stage evidence, and chunk checkpoints, and forces full review inputs once. Keep the external published review payload unchanged; the execution field is internal control data, not a readiness category.

Test And Acceptance Criteria

Acceptance Criteria

  • PR, Issue, code-chunk, and aggregation execution failures do not publish a final review or cache the failed turn as complete.
  • A new request for the same head retries the failed turn and can finish; earlier successful turns remain checkpointed.
  • Missing execution status fails closed.
  • A completed review containing a genuine blocker remains failed and reusable without new model tokens.
  • Legacy schema v3 evidence cannot produce a reused generation or retain completed chunk checkpoints.
  • Read-only and network restrictions and the external review payload remain unchanged.

Validation

Run node .github/scripts/pr-review/test.mjs, including the mocked Codex failure/retry matrix and legacy-state test. Confirm the new failure regression fails against the old runner and passes with the fix. Run the adjacent suites: node .github/scripts/pr-readiness/test.mjs, node .github/scripts/issue-review/test.mjs, and node .github/scripts/review-request/test.mjs, plus git diff --check.

Request automated review on the implementation PR. Live use of this fix by a pinned consumer requires adopting the merged upstream revision; mocked tests verify orchestration behavior, not model obedience.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions