Skip to content

fix: retain partial source edits in failed patch audits - #211

Merged
steipete merged 1 commit into
mainfrom
fix/phase-five-failed-patch-audit
Sep 12, 2026
Merged

fix: retain partial source edits in failed patch audits#211
steipete merged 1 commit into
mainfrom
fix/phase-five-failed-patch-audit

Conversation

@steipete

Copy link
Copy Markdown
Contributor

What Problem This Solves

A provider can write source files and then fail or exit before returning its fix plan. Clawpatch recorded the attempt as failed but left filesChanged empty, hiding the partial edits from its patch audit.

User Impact

Failed patch attempts retain the source edits observed before the provider failed. The command still fails, and the patch remains failed; no commit, push, or automatic rollback is introduced.

Why This Change Was Made

Use the same before/after source snapshots in the provider-error path as in the successful path. Preserve the original provider error and the finding's patch link.

Evidence

  • Regression fails before the fix: a provider modifies an existing file and creates another, but the failed patch reports an empty file list.
  • Live before proof: a synthetic executable named codex edits src/index.ts then exits 1. The real built CLI records failed, filesChanged: [], despite the changed file.
  • Live after proof: the same executable/CLI fixture still exits 1 with patch status failed, but records filesChanged: ["src/index.ts"] matching the actual edit.
  • pnpm typecheck, pnpm lint, audit/workflow tests (130 passed), and pnpm build pass. The original no-write provider failure case remains intact.
  • Isolated Codex autoreview: scoped-clean at P0–P2.

@steipete
steipete requested a review from a team as a code owner September 12, 2026 23:28
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete merged commit 2ceeaa4 into main Sep 12, 2026
11 checks passed
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 12, 2026
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 7:30 PM ET / 23:30 UTC.

ClawSweeper review

What this changes

Records files changed before a provider failure in the failed patch audit, with regression coverage and documentation.

Merge readiness

Ready for maintainer review

This remains a useful, focused fix: current main and v0.8.0 leave failed provider attempts with an empty file audit. No blocking defect was found in the patch.

Priority: P2
Reviewed head: 8d8e60746be8e677c28f3cd370833af1d63cc9ca

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with relevant before/after CLI observations, regression coverage, and no identified blocking defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.
Evidence reviewed 8 items Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-notes directory was found. Applied the guidance on focused helpers, workflow regression coverage, documentation, and conservative source-file handling.
Introduced fix and current-main defect: The pinned main implementation initializes filesChanged to an empty array and never replaces it in the provider-error catch. The introduced two production lines take the after snapshot and populate that existing field while retaining failed status, the finding link, and rethrowing the provider error.
Existing snapshot and downstream safety contracts: The reused helper excludes project state, fingerprints source changes, and compares sorted path unions. Existing tests cover nested-project isolation and renames. The serialized patch schema is unchanged; open-pr still rejects failed patches unless the operator explicitly supplies force.
Findings None None.
Security None None.

How this fits together

Clawpatch’s fix command sends a selected finding to a provider that can edit the project. Its patch audit records the resulting files, validation results, and failure status for later inspection.

flowchart TD
  A[Selected finding] --> B[Snapshot source changes]
  B --> C[Provider edits project]
  C --> D{Provider succeeds?}
  D -->|Yes| E[Run validation]
  D -->|No| F[Snapshot partial edits]
  F --> G[Save failed audit and return error]
  E --> H[Save validated patch audit]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Use the same project-scoped snapshot comparison for successful and failed provider attempts while preserving failure semantics.

Do we have a high-confidence way to reproduce the issue?

Yes: current main’s catch branch preserves the initial empty file list even when the provider edits files before throwing. Source inspection establishes the defect; this review did not execute it.

Is this the best way to solve the issue?

Yes: reusing the existing snapshot comparison is a narrow repair that keeps the record format and downstream failure checks intact.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ead0ec4703e0.

Labels

Label changes:

  • add P2: Repairs incomplete audit records after provider failures without changing the normal repair workflow.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.

Label justifications:

  • P2: Repairs incomplete audit records after provider failures without changing the normal repair workflow.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports a built-CLI run through the real provider subprocess boundary with an injected edit-then-exit failure: the audit changes from an empty file list to src/index.ts while status and exit remain failed. This directly covers the changed error-path persistence behavior.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-notes directory was found. Applied the guidance on focused helpers, workflow regression coverage, documentation, and conservative source-file handling. (AGENTS.md:1, 8d8e60746be8)
  • Introduced fix and current-main defect: The pinned main implementation initializes filesChanged to an empty array and never replaces it in the provider-error catch. The introduced two production lines take the after snapshot and populate that existing field while retaining failed status, the finding link, and rethrowing the provider error. (src/fix.ts:89, 8d8e60746be8)
  • Existing snapshot and downstream safety contracts: The reused helper excludes project state, fingerprints source changes, and compares sorted path unions. Existing tests cover nested-project isolation and renames. The serialized patch schema is unchanged; open-pr still rejects failed patches unless the operator explicitly supplies force. (src/change-audit.ts:13, 8d8e60746be8)
  • Focused regression coverage: The new workflow case modifies an existing source file, creates another, throws from the provider, and checks the original error plus the failed audit’s exact two-file list. The neighboring no-write failure test retains finding-link coverage. Tests and builds were not executed during this read-only review. (src/workflow.test.ts:5200, 8d8e60746be8)
  • Reported production-path before/after proof: The complete captured PR body, sourceRevision 2dce5ec2d1de2ab8f3f8d6c1f2ea9432abed6cd0a4d61b9874fde1400934e501, reports running the real built CLI against an executable named codex that edits src/index.ts and exits 1. Before the fix the failed audit lists no files; afterward it lists src/index.ts while retaining failed status and exit 1. This exercises Clawpatch’s actual subprocess/provider failure boundary and audit persistence. These are contributor-reported observations, not a reviewer-executed run. (8d8e60746be8)
  • Release remains affected: The latest-release REST response identifies v0.8.0 at e78e718. Reading that release’s fix.ts through the contents API confirms its provider-error branch also retains the initial empty filesChanged array. (src/fix.ts, e78e71836812)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

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

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant