feat: Phase 3 Five-Alarm staged recovery - #4
Conversation
Implement design §8.2 Five-Alarm state machine: gate on current containment failure, intensified Firebreak, alternate model clean-room (no prior implementation code), verify/rank all candidates, apply smallest verified, retain rejects. CLI `tif five-alarm --run` + orchestrator integration; tests per stage; ROADMAP/CHANGELOG.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f03f72d17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || run | ||
| .firebreak | ||
| .as_ref() | ||
| .is_some_and(|fb| !fb.applied && (!fb.success || !fb.candidate_ready)); |
There was a problem hiding this comment.
When a run is already Contained and a previous manual Firebreak failed or produced no ready candidate, this clause makes current_failure true even though the current score/assessment still says the implementation is contained. That bypasses the Five-Alarm gate (“current containment failure required”) and can let tif five-alarm --run escalate and potentially auto-apply a recovery candidate for a contained run; gate this Firebreak outcome on an actual out-of-control score/assessment instead of treating any failed Firebreak as a containment failure.
Useful? React with 👍 / 👎.
| last_err_msg = e.to_string(); | ||
| run.events.push(format!( | ||
| "five-alarm stage1 attempt {attempt}/{max_attempts} error: {e}" | ||
| )); | ||
| continue; |
There was a problem hiding this comment.
If the selected Stage 1 reviewer errors before returning a BackendGenerateResult (for example an uncompiled provider, missing credential, or context-build failure), this branch records only the message and retries the same reviewer; used_reviewer_ids remains empty, so Stage 2 select_excluding can select that same failing reviewer again instead of the configured alternate. In mixed reviewer pools this prevents Five-Alarm from reaching the different authorized model even though one is available, so record the attempted reviewer before continuing or exclude it on Stage 2 after repeated errors.
Useful? React with 👍 / 👎.
| if apply_outcome.requires_approval && apply_outcome.candidate_ready { | ||
| plan.stage = FiveAlarmStage::Complete; | ||
| plan.applied = false; | ||
| plan.original_preserved = true; |
There was a problem hiding this comment.
For approval-required winners, the plan is persisted as Complete with applied = false, but the generic approve_firebreak path later applies the candidate without updating run.five_alarm or the winner candidate flags. Any run that requires approval will therefore keep an audit timeline saying the Five-Alarm candidate was only pending/not applied even after tif approve succeeds; defer marking the plan complete or update the stored Five-Alarm plan in the approval path.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| .clone() | ||
| .filter(|s| Some(&s.id) == winner.isolation_session_id.as_ref()) | ||
| }) | ||
| .or_else(|| run.isolation_session.clone()); |
There was a problem hiding this comment.
🔥 The Roast: The previous three filter callbacks politely decline the wrong session, and then the last or_else quietly hands the apply whatever session was lying around — the stage-3 clean-room tree, the run's last-known session, anything. You fixed up candidate_path afterwards, but session.id is what the audit/rollback story will read back, and you'll be telling future-you it applied fa-stage1-fa-primary-a1 when it actually painted the clean-room tree onto the worktree.
🩹 The Fix: Drop the final or_else(|| run.isolation_session.clone()) and let the Some(session) else { … } branch return the "winner selected but isolation session missing" abort you already wrote. If you genuinely want a fallback, only use it when the candidate path matches the winner's candidate_path byte-for-byte.
| .or_else(|| run.isolation_session.clone()); | |
| .or_else(|| run.isolation_session.clone()) | |
| .filter(|s| { | |
| winner | |
| .candidate_path | |
| .as_ref() | |
| .is_some_and(|p| Some(&s.candidate_path.as_ref().unwrap_or(&PathBuf::new())) == Some(p)) | |
| }); |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| impl Default for FiveAlarmRunOptions { | ||
| fn default() -> Self { | ||
| Self { | ||
| authorize_apply: true, |
There was a problem hiding this comment.
🔥 The Roast: The default for FiveAlarmRunOptions is authorize_apply: true, user_approved: false. So FiveAlarmRunOptions::default() is secretly "skip the approval gate and apply me". Every test that does … ::default() (and there are several) is silently green-lighting an apply — and any future caller who forgets to set the bool gets the same free pass. This is the typedef cousin of DANGEROUS: true.
🩹 The Fix: Flip the default. authorize_apply: false is the safe baseline; let the CLI explicitly OR it with cfg.approval.auto_apply_firebreak (which it already does), and have test harnesses use the same opt-in.
| authorize_apply: true, | |
| authorize_apply: false, |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| // Also enforce FireLevel gate for consistency. | ||
| let _ = FireLevel::escalate_to_five_alarm(true)?; |
There was a problem hiding this comment.
🔥 The Roast: let _ = FireLevel::escalate_to_five_alarm(true)?; — you already verified current_containment_failure is true thirty lines above, and now you're calling the same gate with a literal true and discarding the result. This is guarding the door with a mirror.
🩹 The Fix: Delete the line. The gate is enforced by FiveAlarmPlan::begin_escalation; calling it again with true adds nothing but a ? and a riddle for the next reader.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Ok(plan) | ||
| } | ||
|
|
||
| fn push_timeline( |
There was a problem hiding this comment.
🔥 The Roast: push_timeline (private) and push_timeline_pub (public) are the same function with two names. The private one exists for the sole purpose of calling the public one. That's not encapsulation, that's a conjoined twin.
🩹 The Fix: Pick one name. Either inline push_timeline_pub into the two begin_escalation call sites and delete the private wrapper, or (cleaner) make push_timeline_pub private and rename it push_timeline. The orchestrator can call the same method as the gate — there's no API reason for the public/private split.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| run.five_alarm = Some(plan.clone()); | ||
| self.finalize_firebreak(config, run, apply_outcome.clone())?; | ||
| run.five_alarm = Some(plan.clone()); |
There was a problem hiding this comment.
🔥 The Roast: run.five_alarm = Some(plan.clone()) — and again one line after finalize_firebreak returns. The setter that wraps the work and then the setter that wraps the setter. finalize_firebreak doesn't touch five_alarm, so the second assignment is a no-op wearing a costume.
🩹 The Fix: Delete line 1609. The set on 1607 is the one that actually persists.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: Request changes | Recommendation: Address the session-id fallback and the Overview
Issue Details (click to expand)
🏆 Best part: The clean-room invariants are enforced in three places (context builder, marker assertion, and orchestrator's explicit 💀 Worst part: The session.resolve chain in 📊 Overall: Like a fire drill that everyone runs perfectly except the last person who walks past the exit. The state machine, the gate, the clean-room mode, the multi-reviewer selection — all genuinely good. But the apply path has a "eh, whatever session" escape hatch that turns the audit trail into a fiction. Files Reviewed (12 files)
Correctness / Safety Findings
Ponytail Review
Ponytail net: ~15 lines. Suggested Minimal Patch
Final Merge GuidanceDo not merge until the two warnings are fixed (the session-id fallback and the Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 78.1K · Output: 14.3K · Cached: 1.8M |
Summary
Implements Phase 3: Five-Alarm staged recovery (design §8.2).
Behavior
Surface
FiveAlarmPlanstate machine + audit timeline onRunRecord.five_alarmRunOrchestrator::run_five_alarmtif five-alarm --plan/tif five-alarm --run <id> [--apply]Safety
Test plan