Skip to content

fix: freeze approved feature heads during integration - #674

Merged
krasnoperov-coding-agent[bot] merged 3 commits into
mainfrom
feature/unify-integration-repair-routing
Sep 14, 2026
Merged

krasnoperov-coding-agent[bot] merged 3 commits into
mainfrom
feature/unify-integration-repair-routing

Conversation

@krasnoperov-coding-agent

Copy link
Copy Markdown
Contributor

Summary

  • freeze approved feature heads when main movement produces a dirty PR
  • route approved retries and steward incidents exclusively through candidate integration repair
  • make Queue Health Monitor the sole candidate reader and repair dispatcher
  • consume repair budgets only after a Codex turn actually starts

Invariants

  • an approved feature head is never rewritten to resolve integration drift
  • a missing candidate waits for Merge Steward instead of falling back to feature-branch repair
  • pre-turn infrastructure failures do not consume an agent repair attempt

@krasnoperov-coding-agent
krasnoperov-coding-agent Bot marked this pull request as ready for review September 14, 2026 21:52
review-quill[bot]
review-quill Bot previously requested changes Sep 14, 2026

@review-quill review-quill Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 🛑 Request changes — The new attempt-accounting boundary can both strand pre-turn CI failures and miss completed repair attempts.

Comment thread src/run-orchestrator.ts Outdated
// A repair budget represents an agent attempt, not an enqueue, worktree
// preparation, failed claim, or other pre-turn infrastructure work.
// Consume it only after Codex confirms that the turn actually started.
if (!this.runTaskPlanner.incrementAttemptCounters(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Attempt accounting is not atomic with turn start. claimRun still records lastAttemptedFailure* before pre-turn setup, so a failed CI launch suppresses same-head retry despite consuming no budget; an early completion can also release the lease before this post-await increment, letting retries evade the budget. Record both counter and provenance at a turn-start boundary ordered before completion handling.

@review-quill
review-quill Bot dismissed their stale review September 14, 2026 22:03

Superseded by newer head 91a829a; review-quill will re-review the latest commit separately.

review-quill[bot]
review-quill Bot previously requested changes Sep 14, 2026

@review-quill review-quill Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 🛑 Request changes — The follow-up resolves early-completion accounting, but pre-turn crash recovery and conflict-safe counter updates remain incorrect.

Comment thread src/run-launcher.ts Outdated
lastAttemptedFailureAt: new Date().toISOString(),
}
: {}),
...buildAttemptStartFields(params.runType, fresh, params.effectiveContext),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Claim-time accounting still misses a pre-turn crash window. If PatchRelay exits after persisting the thread but before startTurn, restart reads a valid thread with zero turns; reconciliation never settles it, leaving activeRunId and the consumed budget pinned indefinitely.

Comment thread src/run-launcher.ts Outdated
lastAttemptedFailureAt: new Date().toISOString(),
}
: {}),
...buildAttemptStartFields(params.runType, fresh, params.effectiveContext),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 The counter is derived from fresh, but the claim commit reuses that same update on version conflict. A concurrent head/reset write can therefore be overwritten with a stale counter value instead of recomputing current + 1, prematurely exhausting the repair budget.

@review-quill
review-quill Bot dismissed their stale review September 14, 2026 22:12

Superseded by newer head 4c17901; review-quill will re-review the latest commit separately.

@review-quill review-quill Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve — The follow-up now recomputes claim-time counters on conflicts and settles/refunds persisted pre-turn threads with no turns. No remaining actionable current-head issues found.

@krasnoperov-coding-agent krasnoperov-coding-agent Bot added queue:testing Merge-steward is validating speculative CI queue:merging Merge-steward is merging this PR and removed queue:testing Merge-steward is validating speculative CI labels Sep 14, 2026
@krasnoperov-coding-agent
krasnoperov-coding-agent Bot merged commit 4c17901 into main Sep 14, 2026
2 checks passed
@krasnoperov-coding-agent
krasnoperov-coding-agent Bot deleted the feature/unify-integration-repair-routing branch September 14, 2026 22:17
@krasnoperov-coding-agent krasnoperov-coding-agent Bot removed the queue:merging Merge-steward is merging this PR label Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants