Enforce repair contract at every checkpoint#340
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 19, 2026, 9:57 AM ET / 13:57 UTC. Summary Reproducibility: not applicable. as an issue reproduction; this is a PR adding optional repair-contract enforcement. For the changed behavior, the PR body includes local dry-run output showing accepted and rejected checkpoint gate cases. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow explicit must-touch checkpoint guard after maintainer review and successful exact-head CI, while keeping conflict-prompt and stale Do we have a high-confidence way to reproduce the issue? Not applicable as an issue reproduction; this is a PR adding optional repair-contract enforcement. For the changed behavior, the PR body includes local dry-run output showing accepted and rejected checkpoint gate cases. Is this the best way to solve the issue? Yes for the stated repair-contract scope. The PR follows the narrower maintainer guidance from #336 by centralizing checkpointing, avoiding conflict-prompt duplication, and not enforcing incomplete AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 471bf8065dd4. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
|
Maintainer deep review found two blocking contract defects.
Required shape before reconsideration:
Relevant code: |
|
Closing after maintainer deep review. The proposed Detailed findings: #340 (comment) The underlying hardening idea can be reconsidered only after defining the artifact contract end to end. Thanks for the investigation. |
Summary
Rebuild the closed repair-contract proposal from current
mainfollowing the maintainer guidance on #336.This version:
Why this is narrower than #336
The previous branch failed because it guarded only the first checkpoint and mixed in conflict-prompt behavior. This PR only changes checkpoint acceptance.
The contract is intentionally explicit: if the artifact does not declare
must_touch/must_touch_files, checkpointing behaves as before. This avoids fail-closing valid repairs whenlikely_filesis stale or incomplete.Validation
Result:
I also started
corepack pnpm run check; it passed build/lint and entered the long unit/coverage portion, but the terminal wrapper timed out before the full command completed. I am not claiming a completed fullpnpm checkrun from this local session.Local repair-worker proof
I ran
dist/repair/execute-fix-artifact.js --dry-runagainst temporary local checkouts withCODEX_BINpointed at a local fake Codex harness. This is local worker proof of the checkpoint gate, not a real model-quality run.Allowed case: worker touches
must_touchFixture:
{ "must_touch": ["docs/repair-contract-proof.md"], "validation_commands": ["git diff --check"] }Fake worker touched:
Observed output:
Observed git result:
The run then reached the later validation/review loop and was blocked by the target repo's default
pnpm check:changedbecause the temporary clone had no installed dependencies. That is after the checkpoint gate had already allowed and committed themust_touchchange.Blocked case: worker touches only an off-contract file
Fixture:
{ "must_touch": ["docs/repair-contract-proof.md"], "validation_commands": ["git diff --check"] }Fake worker touched:
Observed failure before checkpoint:
Observed git result:
Notes
This PR intentionally does not update or reopen #336. If this lands, fork PR Jhacarreiro#2 can be closed as superseded because its porcelain parser fix is included here.