Skip to content

F: a draft names the budget, not an unsaved safety finding from preparation #127

Closed

Description

@mchwang

Problem

A draft PR can name the wrong reason when a task's safety finding was never saved.

A run that finds a safety violation saves the finding on its attempt, then moves the task to needs human. If the save fails (a real SQLite error) and the move succeeds, the attempt row has no safetyFinding. The finding text exists only in memory, and SafetyFindings.settle drops it once the move commits.

TaskPublishing.#problems (runner/publishing.ts:330) then picks the draft's problems in this order:

  1. A finding remembered in #escalations. Only #payOwed fills this, so a run's own escalation is never in it.
  2. The current attempt's safetyFinding. This is empty, because the save failed.
  3. A diagnostic that starts with Safety violation: (SAFETY_VIOLATION).
  4. "The task's time budget ran out…" when the budget has passed.
  5. The attempt's diagnostic.

Step 3 covers findings from the audit, whose diagnostic is the finding text. It misses two other sources:

  • A finding raised during preparation. Its diagnostic is Preparation failed: "Safety violation: …", which does not start with the prefix.
  • A finding from the audit of a failed run. Its diagnostic is the agent's quoted stderr.

Failure scenario

  1. A task's budget has passed.
  2. A run raises a safety finding during preparation, and recordSafetyFinding fails.
  3. The run still moves the task to needs human.
  4. The draft PR says the time budget ran out, and does not mention the safety finding.

The contract in docs/implementation/pull-request-opening.md (Mode) says a budget that ran out must not hide a finding.

How likely

Very unlikely today. The save goes through the shutdown capability, so only a storage failure triggers it, and it also needs one of the two finding sources above. The independent review of PR #110 (round 10) found it and rated it below the bar for that PR.

Proposed fix

Make ItemExecutor remember the reason for every escalation it makes, keyed by task and tied to the attempt that is current after the move. That covers a run's own #escalate as well as payOwed. Then have TaskPublishing.#problems read that, instead of only its own #escalations map and the SAFETY_VIOLATION prefix check.

Use the same rule as now: the remembered reason applies only while the task is still in needs human on that attempt (see the round 8 fix in #110).

Done when

  • A draft for each finding source names the finding, even after the budget has passed: audit, preparation, and the audit of a failed run.
  • A test for each source fails when the remembered reason is removed.
  • pull-request-opening.md describes where the reason comes from.

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

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions