feat: Phase 2 automatic isolated Firebreak closed loop - #3
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 826e1573ac
ℹ️ 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".
| authorize_apply: authorize, | ||
| user_approved: false, | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
Persist auto Firebreak state before propagating apply errors
In the auto reviewer branch, the ? here exits before the post-call record_run when run_firebreak_auto returns an apply error. That method can already have preserved a baseline and set restore_pending on the session, so the audit DB still contains only the pre-call record without isolation_session/baseline_path; a subsequent tif rollback cannot locate the recovery data for a partially modified workspace. Capture the result, record the mutated run, then propagate the error as the explicit-candidate path does.
Useful? React with 👍 / 👎.
| // Phase 2 automatic closed loop when --auto or reviewers are configured. | ||
| // Persist before apply is handled inside the closed loop (session attach). | ||
| store.record_run(&run)?; | ||
| let authorize = authorize_apply || auto || cfg.approval.auto_apply_firebreak; |
There was a problem hiding this comment.
Do not let --auto override disabled auto-apply
When a repo sets [approval].auto_apply_firebreak = false, tif run complete --auto-firebreak honors it by passing authorize_apply=false, but tif firebreak --auto sets authorization to true because auto is ORed in here. A smaller non-sensitive candidate will therefore be applied even though the repository explicitly disabled automatic Firebreak apply; --auto should start the closed loop, not bypass the approval policy.
Useful? React with 👍 / 👎.
| let requires_approval = !req.user_approved | ||
| && (req.force_approval | ||
| || req.policy.require_firebreak_approval | ||
| || touches_sensitive(&req.policy, &req.candidate_metrics)); |
There was a problem hiding this comment.
Hold verified candidates for approval when auto-apply is disabled
For automatic flows that pass authorize_apply = config.approval.auto_apply_firebreak, setting auto_apply_firebreak=false produces a smaller verified candidate with apply authorization turned off, but this approval predicate still returns false unless the candidate is sensitive or approval is globally required. The outcome becomes candidate_ready/requires_approval=false, so finalize_firebreak immediately restores instead of entering AwaitingApproval, leaving tif approve unable to apply the held candidate.
Useful? React with 👍 / 👎.
| .unwrap_or("unknown"), | ||
| reviewer_id: fb.and_then(|f| f.reviewer_id.as_deref()), | ||
| }; | ||
| eng.record_run(outcome); |
There was a problem hiding this comment.
Make adaptation recording idempotent per run
This appends aggregate counters every time a run is recorded, but the new CLI calls it after run complete, again after firebreak, and again after approve/rollback for the same run. A single run that goes OutOfControl → AwaitingApproval → Applied is counted as multiple total runs and multiple outcomes, skewing the fire-level and reviewer recommendations; persist per-run adaptation state or update the aggregate idempotently by run id.
Useful? React with 👍 / 👎.
Summary
Implements Phase 2 of the production plan: automatic isolated Firebreak closed loop with approval queue.
When a run completes OutOfControl (correct but exceeds containment) with authorized reviewers configured:
generate_with_backend)require_firebreak_approval→ AwaitingApproval (tif approve|reject)auto_apply_firebreak(default true) → apply with baseline + sticky restoreKey changes
RunOrchestrator::run_firebreak_autoproduction path (not permanently fail-closed when backends exist)approval.auto_apply_firebreak(default true), optionalapproval_ttl_hourstif firebreak --auto,tif approve <run_id>,tif reject <run_id>session.candidate_path(reviewer output); skips.tif-candidateon overlaytif adaptation)Test plan
cargo fmt --allcargo clippy --workspace --all-features -- -D warningscargo test --workspace --all-features(114 tests)Manual smoke (optional)
Safety