Add reproducible PR review loop + enforce merge gate on land - #59
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive PR reproducibility and review-loop enforcement system for Tutti's SDLC automation. It adds merge-gate validation (enforcing required checks and resolved review threads before landing), auto-recovery features for prompt steps, new run tracking CLI commands, milestone-based issue filtering, baseline verification and implementation-result tracking scripts, comprehensive workflow documentation, runtime pattern updates, and infrastructure for deterministic automation workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (Prompt)
participant Session as Tmux Session
participant Executor as Step Executor
participant Automation as Automation Runtime
participant File as Output/State Files
participant Retry as Retry Logic
Executor->>Automation: start_and_wait_ready()
Automation->>Session: Create/start session
Session-->>Automation: Ready
Automation-->>Executor: Session started
Executor->>Executor: Capture baseline pane hash
Executor->>Session: Send prompt
Executor->>Automation: wait_for_prompt_activity_or_output(20s)
alt Activity/Output Detected
Automation->>Session: Poll pane for activity
Session-->>Automation: Output detected
Automation-->>Executor: Success
else Timeout
Automation-->>Executor: Timeout (fail if no output file)
end
Executor->>File: Check for output_json
alt Output File Exists
File-->>Executor: File loaded
Executor->>Executor: Record Success
else File Missing
Executor->>Retry: Prepare retry prompt
Retry->>Session: Auto-start + send retry
Retry->>Automation: wait_for_prompt_activity_or_output(60s)
Automation-->>Retry: Activity detected
Retry->>File: wait_for_prompt_output_file(120s)
File-->>Retry: File appears
Retry-->>Executor: Load and store output
end
sequenceDiagram
participant User as User/Automation
participant Land as Land Command
participant GH as GitHub (gh CLI)
participant Checks as Required Checks
participant Reviews as Review Threads
participant Merge as Merge Gate
User->>Land: tt land (with TT_ENFORCE_MERGE_GATE=1)
Land->>GH: gh pr list --branch <target>
GH-->>Land: PR number
Land->>Checks: gh pr checks <pr> --required
Checks-->>Land: Check statuses
alt Any Check Not Green
Land->>Merge: ❌ BLOCK
Merge-->>User: Landing failed: required checks not green
end
Land->>Reviews: Query GraphQL reviewThreads (paginated)
Reviews-->>Land: Thread list + resolution status
alt Unresolved Threads Exist
Land->>Merge: ❌ BLOCK
Merge-->>User: Landing failed: unresolved review threads
else All Resolved
Land->>Merge: ✅ ALLOW
Merge-->>User: Landing approved
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/land.rs (1)
35-43:⚠️ Potential issue | 🔴 CriticalGate the landed SHA, not just the PR's branch name.
Line 39 validates GitHub state before the
--prearly return and before Line 40 can create a new local commit. That breakstt land <agent> --prunderTT_ENFORCE_MERGE_GATE=1, and the normal land path can still cherry-pick local-only commits — including the auto-commit fromcommit_wip_if_needed()— even though checks/review only covered the older PR head. Skip the gate on the--prpath, and make the gate compare the local branch tip to the PR head SHA before landing.Please also fail closed when `git rev-parse ` does not match the PR head SHA.💡 Safer ordering
if !force { ensure_git_clean(&resolved.project_root)?; } ensure_branch_exists(&resolved.project_root, &branch)?; - maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate)?; let wip_committed = commit_wip_if_needed(&worktree_path, &resolved.agent_name)?; if pr { push_and_open_pr(&resolved.project_root, &branch)?; if wip_committed { @@ } return Ok(()); } + + maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/land.rs` around lines 35 - 43, The merge-gate check is currently run unconditionally before handling the --pr path and before commit_wip_if_needed(), which allows local-only commits to bypass review; modify the flow in land.rs so that when pr is true you skip calling maybe_enforce_merge_gate() before push_and_open_pr(), and instead, when enforcing the merge gate for the normal land path, have maybe_enforce_merge_gate() (or a new helper it calls) fetch the PR head SHA and compare it against the local branch tip (git rev-parse <branch>); if they differ, fail closed with an error; also ensure the check happens after commit_wip_if_needed() for the non-PR path so the gated SHA is the actual commit being landed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/land.rs`:
- Around line 227-237: The GraphQL query assigned to variable `query` only
requests `reviewThreads(first: 100)` which can miss threads on large PRs; change
the query to accept an `after: String` variable and request
`reviewThreads(first: 100, after: $after)`, include `pageInfo { hasNextPage
endCursor }` alongside `nodes { isResolved }`, and then update the calling code
that queries the `pullRequest` (the loop/logic that counts unresolved threads)
to paginate: repeatedly execute the query using `endCursor` while `hasNextPage`
is true, aggregating `nodes` and counting where `isResolved == false` until all
pages are fetched so the unresolved-thread gate is accurate.
In `@src/cli/permissions.rs`:
- Around line 204-209: The current fallback synthesizes "{cmd} *" which is too
broad; change the unwrap_or_else that uses format!("{cmd} *") so it does not
append a wildcard—use cmd.clone() (or preserve None) instead so
PermissionSuggestion.suggested_rule is the command itself rather than a
star-suffixed prefix; update the construction of PermissionSuggestion (and its
suggested_rule type if you want to represent absence) so the code uses
decision.suggested_rule.unwrap_or_else(|| cmd.clone()) instead of the
wildcard-forming string.
In `@src/permissions/mod.rs`:
- Around line 73-85: The suggestion builder currently emits wildcard prefix
rules via suggested_wildcard_prefix_rule which creates entries like "cargo test
*" and thus over-broadens permissions; change suggested_wildcard_prefix_rule to
emit an exact prefix suggestion instead (e.g., return Some(format!("{} {}",
tokens[0], tokens[1])) for two-token commands) so callers get "cargo test"
(which matching_allow_rule already treats as a valid prefix match) rather than a
wildcard rule; leave the function signature and tokenization logic intact but
remove the trailing " *" and ensure it still returns None for short inputs.
---
Outside diff comments:
In `@src/cli/land.rs`:
- Around line 35-43: The merge-gate check is currently run unconditionally
before handling the --pr path and before commit_wip_if_needed(), which allows
local-only commits to bypass review; modify the flow in land.rs so that when pr
is true you skip calling maybe_enforce_merge_gate() before push_and_open_pr(),
and instead, when enforcing the merge gate for the normal land path, have
maybe_enforce_merge_gate() (or a new helper it calls) fetch the PR head SHA and
compare it against the local branch tip (git rev-parse <branch>); if they
differ, fail closed with an error; also ensure the check happens after
commit_wip_if_needed() for the non-PR path so the gated SHA is the actual commit
being landed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb1c405b-6442-4937-b514-83e549ac1c66
📒 Files selected for processing (8)
README.mddocs/CODEX_SDLC_ORCHESTRATION.mddocs/examples/tutti-codex-sdlc.tomldocs/pr-review-loop.mdsrc/automation/mod.rssrc/cli/land.rssrc/cli/permissions.rssrc/permissions/mod.rs
- Paginate reviewThreads GraphQL query in land.rs to handle PRs with
>100 review threads (request pageInfo, loop with cursor)
- Use cmd.clone() instead of format!("{cmd} *") as fallback in
permissions.rs to avoid overly broad suggested rules
- Rename suggested_wildcard_prefix_rule to suggested_prefix_rule and
emit exact prefix suggestions without trailing " *"
- Update all affected test expectations
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/automation/mod.rs (1)
2699-2700: Avoid duplicating the merge-gate env var literal across modules.
"TT_ENFORCE_MERGE_GATE"is now defined in more than one place; centralizing it prevents drift if the key ever changes.♻️ Suggested refactor
diff --git a/src/cli/land.rs b/src/cli/land.rs @@ -const ENFORCE_MERGE_GATE_ENV: &str = "TT_ENFORCE_MERGE_GATE"; +pub(crate) const ENFORCE_MERGE_GATE_ENV: &str = "TT_ENFORCE_MERGE_GATE";diff --git a/src/automation/mod.rs b/src/automation/mod.rs @@ - let run_result = - run_tt_subcommand_with_env(project_root, &args, &[("TT_ENFORCE_MERGE_GATE", "1")]); + let run_result = run_tt_subcommand_with_env( + project_root, + &args, + &[(crate::cli::land::ENFORCE_MERGE_GATE_ENV, "1")], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 2699 - 2700, Extract the environment variable literal "TT_ENFORCE_MERGE_GATE" into a single shared constant and use that constant where it's referenced instead of the string literal; e.g., define a pub(crate) const TT_ENFORCE_MERGE_GATE: &str = "TT_ENFORCE_MERGE_GATE" in a central module (such as a new or existing constants/mod or the parent automation module) and replace the literal in the call to run_tt_subcommand_with_env(project_root, &args, &[("TT_ENFORCE_MERGE_GATE", "1")]) with &[ (TT_ENFORCE_MERGE_GATE, "1") ] so all modules reference the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/automation/mod.rs`:
- Around line 2699-2700: Extract the environment variable literal
"TT_ENFORCE_MERGE_GATE" into a single shared constant and use that constant
where it's referenced instead of the string literal; e.g., define a pub(crate)
const TT_ENFORCE_MERGE_GATE: &str = "TT_ENFORCE_MERGE_GATE" in a central module
(such as a new or existing constants/mod or the parent automation module) and
replace the literal in the call to run_tt_subcommand_with_env(project_root,
&args, &[("TT_ENFORCE_MERGE_GATE", "1")]) with &[ (TT_ENFORCE_MERGE_GATE, "1") ]
so all modules reference the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19b17786-6391-44dc-b085-ac14789a499a
📒 Files selected for processing (4)
src/automation/mod.rssrc/cli/land.rssrc/cli/permissions.rssrc/permissions/mod.rs
Fix dirty worktree contamination, false idle detection, and resource limits that prevented sdlc-auto from completing end-to-end with --strict. Scripts: - create_issue_branch.sh: add worktree safety guard, pre/post-clean, and clean-state assertion to prevent carried dirty state - verify_clean_baseline.sh: new script asserting HEAD==base_sha and clean porcelain after branch creation - wait_coderabbit.sh: treat timeout as soft exit (exit 0), detect CodeRabbit comments and rate-limit responses as fallback Idle detection (health/mod.rs): - Add startup_grace parameter to suppress false saw_activity from initial pane capture during agent initialization - Require idle stability window before acting on completion signals, preventing false completion between Claude Code tool calls - After startup grace, allow stable completion signal to trigger idle detection even without prior saw_activity (fixes case where agent finishes before wait_for_agent_idle starts) Workflow engine (automation/mod.rs): - ensure_running waits for agent to reach idle/ready before declaring success, preventing prompt delivery to uninitialized sessions - Update all wait_for_agent_idle call sites with startup_grace param Workflow config (tutti.toml, docs/examples): - Add verify_clean_baseline step after create_branch - Add stop_idle_agents step before final_review to free max_concurrent slots for the reviewer agent Verified: ISSUE_LABEL=dogfood-now ISSUE_MILESTONE=v0.4.0 tt run sdlc-auto --strict completes all 23 steps successfully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…improvements - issue-claim acquire: add --milestone flag to filter by GitHub milestone - up: preserve agent worktree on auto/issue-* branches instead of resetting, preventing mid-workflow state loss - worktree: add current_branch to WorktreeSnapshot for branch inspection - claude-code runtime: add "Searching for", "Unravelling", "(thinking)" working patterns; add "don't ask on", "shift+tab to cycle" idle and completion patterns for better prompt-bar detection - runtime detection: completion markers only outrank working patterns when no spinner is active - verify_branch_has_changes.py: handle local-only branches and fetch remote before comparison - write_implement_result.py: new script to record implementation output - runs subcommand: add tt runs for listing workflow run history - Test fixtures for claude/codex idle prompt bar detection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use the pub(crate) constant from land.rs instead of duplicating the string literal in automation/mod.rs. Addresses CodeRabbit nitpick on PR #59. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts in permissions (keep non-wildcard suggestion fix), worktree (keep current_branch field + add tests from main), and update test assertions for WorktreeSnapshot.current_branch field. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/automation/verify_branch_has_changes.py`:
- Around line 41-42: The code currently builds branch_ref and calls
git_output(["log", "--oneline", f"origin/main..{branch_ref}"]) which will raise
CalledProcessError if neither origin/<branch> nor local <branch> exist; update
verify_branch_has_changes.py to first check existence of both refs using
ref_exists (for "origin/<branch>" and "<branch>") and if neither exists return a
controlled error (or raise a clear exception) instead of invoking git_output, or
alternatively wrap the git_output call in a try/except that catches
subprocess.CalledProcessError and returns/raises a descriptive error; reference
branch_ref, ref_exists(), and git_output() when making the change.
- Around line 34-39: The dynamic-branch git fetch currently runs with
check=False and ignores all failures; update the subprocess.run call that
fetches ["git", "fetch", "origin", branch] to capture the CompletedProcess
result, inspect result.returncode and result.stderr, and if returncode != 0 and
stderr does not include "couldn't find remote ref" treat it as a hard failure
(log/raise/exit) so network/auth/server errors stop execution while
missing-remote-ref remains acceptable; keep the existing behavior for the
missing-ref case and mirror the stricter handling used for the earlier fetch of
"main".
In `@scripts/automation/write_implement_result.py`:
- Around line 60-66: The current changed_files computation only lists files from
the HEAD commit; update the run_git call that builds changed_files (the list
comprehension using run_git) to use git diff --name-only between base_sha and
HEAD (e.g., run_git(["diff", "--name-only", f"{base_sha}..HEAD"])) so it
collects all files changed in the commit range validated earlier; keep the
filtering (splitlines and if line.strip()) the same and ensure base_sha variable
is referenced in that run_git invocation.
- Around line 45-53: Replace the simple equality check between commit_sha and
base_sha with an explicit ancestor check: call git merge-base --is-ancestor
base_sha HEAD (via the run_git helper or a new helper) before using
run_git(["log", "--oneline", f"{base_sha}..HEAD"]) and if merge-base indicates
base_sha is not an ancestor of HEAD, print an error (include base_branch and
both SHAs) to stderr and return 1; keep the existing early-return for the
equal-SHA case but ensure the new merge-base check runs when they differ to
guard against divergent branches (update variables commit_sha, base_sha,
progress_log where applicable).
In `@src/automation/mod.rs`:
- Around line 815-844: The current auto-start path sleeps for 3 seconds and can
race with agent startup; create a shared helper
start_and_wait_ready(project_root: &Path, session_name: &str, agent: AgentType)
that calls with_project_root(self.project_root, || crate::cli::up::run(...)) to
start the session and then waits for readiness using the existing
wait_for_agent_idle/ensure_running logic (or polling TmuxSession::session_exists
+ wait_for_agent_idle) instead of a fixed sleep; replace the inline match/sleep
blocks in the TmuxSession auto-start site (the block using
with_project_root/crate::cli::up::run) and the other control-DAG startup path
(lines around ensure_running and 1573-1590) to call start_and_wait_ready(...)
and propagate errors into failed_steps/step_results the same way.
- Around line 32-39: The code currently ignores WorkflowBranchState.branch and
treats any commit ahead of base_sha as progress; update
prompt_step_has_branch_progress() to read the current HEAD branch (e.g., from
the worktree or repo) and compare it to WorkflowBranchState.branch, returning
failure/false if they differ, and only then consider base_sha advancement as
success; ensure WorkflowBranchState.branch is referenced to avoid the
unused-field warning and preserve existing semantics for base_branch and
base_sha (use base_branch/default_base_branch and base_sha Option<String> as
before) so CI/clippy warnings are resolved.
- Around line 1201-1247: The retry block for the implement_code step currently
hardcodes the "codex" adapter and fixed timeouts (300s/20s); change the calls
that reference "codex" and the magic timeout values so they use the existing
runtime variable and wait_timeout_secs (and any related timeout values derived
from it) instead; specifically update occurrences in the implement_code retry
path including the wait_for_prompt_activity_or_output call and the
health::wait_for_agent_idle invocation (and any associated sleep/timeout checks)
to pass runtime and wait_timeout_secs rather than the literal "codex" and
hardcoded seconds so the retry behavior matches the agent runtime in use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5530cf32-693c-4c29-b82c-bfa52ca274ee
📒 Files selected for processing (24)
README.mddocs/CODEX_SDLC_ORCHESTRATION.mddocs/examples/tutti-codex-sdlc.tomlscripts/automation/create_issue_branch.shscripts/automation/verify_branch_has_changes.pyscripts/automation/verify_clean_baseline.shscripts/automation/wait_coderabbit.shscripts/automation/write_implement_result.pysrc/automation/mod.rssrc/cli/issue_claim.rssrc/cli/land.rssrc/cli/mod.rssrc/cli/runs.rssrc/cli/send.rssrc/cli/up.rssrc/health/mod.rssrc/main.rssrc/runtime/claude_code.rssrc/runtime/mod.rssrc/worktree/mod.rstests/fixtures/runtime/claude_idle_prompt_bar.txttests/fixtures/runtime/codex_idle_prompt_bar.txttests/fixtures/runtime/codex_idle_prompt_marker.txttutti.toml
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/runtime/codex_idle_prompt_bar.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- docs/CODEX_SDLC_ORCHESTRATION.md
- src/cli/land.rs
Scripts: - verify_branch_has_changes.py: distinguish fetch failures from missing remote refs; guard against missing local+remote branch refs before git log - write_implement_result.py: use merge-base --is-ancestor to validate base_sha ancestry; use git diff --name-only for full range of changed files instead of just HEAD commit Rust: - prompt_step_has_branch_progress: check current branch matches WorkflowBranchState.branch before declaring progress, fixing the unused `branch` field clippy warning - Extract start_and_wait_ready() helper used from all auto-start sites (prompt, ensure_running, implement_code retry, control-DAG) replacing fixed 3-second sleeps - implement_code retry: use agent's actual runtime instead of hardcoded "codex"; use config-driven timeout instead of fixed 300s Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/automation/mod.rs (1)
1143-1148:⚠️ Potential issue | 🟡 MinorFix error message to match actual timeout.
The timeout at line 1134 is 60 seconds, but the error message at line 1146 says "20s". This appears to be a copy-paste error from the earlier activity check at line 902.
🐛 Proposed fix
message: Some( - "implement_code retry did not start activity or produce output within 20s" + "implement_code retry did not start activity or produce output within 60s" .to_string(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 1143 - 1148, The error message for the "implement_code retry" timeout is incorrect (says "20s" but the actual timeout is 60 seconds); update the message associated with the timed_out case (the struct instance that sets exit_code: None, timed_out: true, message: Some(...)) to reflect "60s" (or, better, interpolate the timeout constant/variable used for the 60s check) so the text matches the actual timeout value and avoid future copy-paste discrepancies.
🧹 Nitpick comments (2)
src/automation/mod.rs (2)
1239-1245: Consider usingwait_timeout_secsinstead of hardcoded 3600s for consistency.The first implement_code retry block (lines 1155-1161) uses
Duration::from_secs((*wait_timeout_secs).max(1)), but this block uses a hardcoded 1-hour timeout. This inconsistency could lead to unexpected behavior depending on how the workflow is configured.♻️ Proposed consistency fix
Note: This would require restructuring to access
wait_timeout_secsoutside theif let ResolvedStep::Promptblock, which may not be straightforward. If the 1-hour timeout is intentional for this specific case, consider adding a comment explaining why.+ // Use longer timeout for implement_code without output_json, + // as the agent may need more time to complete complex implementations. let retry_wait = health::wait_for_agent_idle( agent_runtime, session_name, Duration::from_secs(3600),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 1239 - 1245, The call to health::wait_for_agent_idle currently uses a hardcoded Duration::from_secs(3600); replace this with the configured wait_timeout_secs for consistency (e.g., Duration::from_secs((*wait_timeout_secs).max(1))) or otherwise thread the wait_timeout_secs value into the scope where wait_for_agent_idle is invoked (referencing wait_for_agent_idle, agent_runtime, session_name, and wait_timeout_secs) so the timeout behavior matches the other retry block; if the 3600s value is intentional, add a brief comment on why this case must differ.
794-794: Remove unused_auto_started_sessionvariable.This variable is assigned at line 822 but never read. Consider removing it to reduce dead code.
♻️ Suggested removal
- let mut _auto_started_session = false; let rendered = match render_template(text, &outputs, false) {And remove the assignment at line 822:
- Ok(()) => _auto_started_session = true, + Ok(()) => {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` at line 794, Remove the dead variable _auto_started_session: delete its declaration (let mut _auto_started_session = false;) and any subsequent assignments to it (the assignment around where it is set at the later block), since the variable is never read; ensure no other logic depends on that variable so simply remove the declaration and the assignment to eliminate dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/automation/verify_branch_has_changes.py`:
- Around line 40-49: The code allows selecting a stale remote tracking ref
(remote_ref = f"origin/{branch}") even when fetch_branch failed with "couldn't
find remote ref"; update the logic so that when fetch_branch.returncode != 0 and
the stderr contains "couldn't find remote ref" you do NOT consider remote_ref in
the subsequent ref_exists check. Concretely, use the existing fetch_branch error
check to set a flag (or set remote_ref to None) when that specific error occurs,
then only call ref_exists(remote_ref) and assign branch_ref = remote_ref if the
fetch succeeded for that ref; otherwise skip to checking the local branch with
ref_exists(branch) and fail if neither exists, ensuring stale refs aren't used
by ref_exists(remote_ref) after a "couldn't find remote ref" fetch.
In `@scripts/automation/write_implement_result.py`:
- Around line 80-82: The error message incorrectly says "HEAD commit" while the
check uses the computed changed_files for the range base_sha..HEAD; update the
print message in the branch that checks the variable changed_files (the
conditional that returns 1) to reflect the actual range being validated (e.g.,
mention "no changed files in base_sha..HEAD" or interpolate the range variables
used to compute changed_files) so the message matches the check performed by the
function that computes changed_files.
---
Duplicate comments:
In `@src/automation/mod.rs`:
- Around line 1143-1148: The error message for the "implement_code retry"
timeout is incorrect (says "20s" but the actual timeout is 60 seconds); update
the message associated with the timed_out case (the struct instance that sets
exit_code: None, timed_out: true, message: Some(...)) to reflect "60s" (or,
better, interpolate the timeout constant/variable used for the 60s check) so the
text matches the actual timeout value and avoid future copy-paste discrepancies.
---
Nitpick comments:
In `@src/automation/mod.rs`:
- Around line 1239-1245: The call to health::wait_for_agent_idle currently uses
a hardcoded Duration::from_secs(3600); replace this with the configured
wait_timeout_secs for consistency (e.g.,
Duration::from_secs((*wait_timeout_secs).max(1))) or otherwise thread the
wait_timeout_secs value into the scope where wait_for_agent_idle is invoked
(referencing wait_for_agent_idle, agent_runtime, session_name, and
wait_timeout_secs) so the timeout behavior matches the other retry block; if the
3600s value is intentional, add a brief comment on why this case must differ.
- Line 794: Remove the dead variable _auto_started_session: delete its
declaration (let mut _auto_started_session = false;) and any subsequent
assignments to it (the assignment around where it is set at the later block),
since the variable is never read; ensure no other logic depends on that variable
so simply remove the declaration and the assignment to eliminate dead code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d95bc93c-94ec-4693-aa08-c3552a566850
📒 Files selected for processing (3)
scripts/automation/verify_branch_has_changes.pyscripts/automation/write_implement_result.pysrc/automation/mod.rs
- verify_branch_has_changes.py: skip stale origin/<branch> ref when fetch reported "couldn't find remote ref" - write_implement_result.py: fix error message to reflect base_sha..HEAD range instead of "HEAD commit" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/automation/write_implement_result.py (1)
26-27: Use explicit UTF-8 for file I/O in automation artifacts.Relying on implicit locale encoding can make runs nondeterministic across environments.
Proposed fix
- selected_issue = json.loads(Path(sys.argv[1]).read_text()) - branch_info = json.loads(Path(sys.argv[2]).read_text()) + selected_issue = json.loads(Path(sys.argv[1]).read_text(encoding="utf-8")) + branch_info = json.loads(Path(sys.argv[2]).read_text(encoding="utf-8")) @@ - output_path.write_text(json.dumps(payload, indent=2) + "\n") + output_path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8")Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/automation/write_implement_result.py` around lines 26 - 27, The file I/O currently uses Path.read_text() without specifying encoding which can yield nondeterministic results; update the calls that populate selected_issue and branch_info (and the similar call around line where result writing occurs) to read with an explicit UTF-8 encoding (e.g., use Path(...).read_text(encoding="utf-8") or open(..., encoding="utf-8") and .read()) so all artifact reads/writes are deterministic across locales; ensure every Path.read_text()/open usage in this module (including the instance referenced at line ~105) is changed to explicitly specify "utf-8".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/automation/write_implement_result.py`:
- Around line 9-14: Several subprocess.run calls (the ones assigning to result
at the occurrences around lines 9-14, 34-39, 45-50, and 87-92) may hang
indefinitely; add a timeout parameter to each subprocess.run invocation (e.g.,
timeout=30 or an appropriate constant) and catch subprocess.TimeoutExpired
around those calls; on timeout, log an error (including the command and timeout
value) and fail/exit gracefully (e.g., processLogger.error or sys.exit(1)) so
the automation does not stall. Ensure you update every subprocess.run call shown
in write_implement_result.py and handle TimeoutExpired consistently.
---
Nitpick comments:
In `@scripts/automation/write_implement_result.py`:
- Around line 26-27: The file I/O currently uses Path.read_text() without
specifying encoding which can yield nondeterministic results; update the calls
that populate selected_issue and branch_info (and the similar call around line
where result writing occurs) to read with an explicit UTF-8 encoding (e.g., use
Path(...).read_text(encoding="utf-8") or open(..., encoding="utf-8") and
.read()) so all artifact reads/writes are deterministic across locales; ensure
every Path.read_text()/open usage in this module (including the instance
referenced at line ~105) is changed to explicitly specify "utf-8".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66156d60-46fa-404e-8512-1f8c74ec9266
📒 Files selected for processing (2)
scripts/automation/verify_branch_has_changes.pyscripts/automation/write_implement_result.py
Prevent hung automation runs by adding a 60-second timeout to all git subprocess calls (fetch, merge-base, push, and the run_git helper). Addresses CodeRabbit feedback on PR #59. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lt.py Prevents nondeterministic behavior across environments with different locale settings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nutt-adam
previously approved these changes
Mar 19, 2026
Contributor
|
@coderabbitai all requested changes have been made. please re-review |
|
✅ Actions performedFull review triggered. |
nutt-adam
added a commit
that referenced
this pull request
Mar 19, 2026
…choir runs (#71) * feat(health): add startup grace window to wait_for_agent_idle (#67) Prevent fresh prompt steps from falsely completing before the agent has consumed the prompt. The startup grace period (default 30s) gates completion detection until real working activity is observed. Key changes: - wait_for_agent_idle accepts a startup_grace Duration parameter - AgentStatus::Working counts as activity even without pane hash change, requiring 2+ consecutive polls to avoid flicker false positives - First pane capture no longer counts as a hash "change" - Completion signals before any activity are held until grace expires - "Unravelling" added to claude-code working patterns - startup_grace_secs field threaded through config and automation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add 0.3.0 changelog entry for issue #67 and prior unreleased changes Cover startup grace window (#67), persistent memory (#62/#63), merge gate enforcement (#59), permissions suggest (#53), orchestration state machine (#54/#55), and all fixes shipped since 0.2.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reduce startup grace to 10s and validate wait settings - Reduce DEFAULT_STARTUP_GRACE_SECS from 30 to 10 so the completion-before-activity path fires before typical wait timeouts - Validate that wait_timeout_secs/startup_grace_secs are only set when wait_for_idle is true, failing fast with actionable guidance Addresses CodeRabbit feedback on PR #71. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/cli/permissions.rs (1)
587-588: Make the persisted-config assertion exact.
contains("echo blocked")still passes if the file regresses toecho blocked *, so this test no longer proves the on-disk wildcard removal.Suggested fix
- let saved = std::fs::read_to_string(global_config_path()).unwrap(); - assert!(saved.contains("echo blocked")); + let saved = GlobalConfig::load().unwrap(); + assert_eq!( + saved.permissions.unwrap_or_default().allow, + vec!["echo blocked".to_string()] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/permissions.rs` around lines 587 - 588, The test currently asserts that the persisted config "saved" contains "echo blocked", which would still pass if a wildcard slip-in like "echo blocked *" appears; change the assertion to check the exact persisted content instead (e.g., compare saved to the exact expected string or assert equality/trimming rather than using contains). Locate the test that reads the file via global_config_path() (the variable named saved) and replace the loose contains check with an exact match against the expected on-disk config text to ensure wildcard removal is validated precisely.tutti.toml (1)
76-77: Complex shell command is correct but consider extracting to a script.The inline bash command with conditional milestone handling works correctly, but the complexity makes it harder to maintain. Consider extracting this pattern to a reusable script if used frequently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tutti.toml` around lines 76 - 77, The inline complex bash in the run key (the cargo run ... issue-claim acquire command with conditional ISSUE_MILESTONE handling and output to .tutti/state/auto/selected_issue.json) should be extracted to a small executable script (e.g., scripts/acquire-issue.sh) that accepts ISSUE_LABEL, ISSUE_MILESTONE, lease TTL and output path as args or environment variables; update the run value to call that script with the same env vars (ISSUE_LABEL, ISSUE_MILESTONE, ISSUE_MILESTONE check moved into the script) so the conditional logic and command composition live in a reusable, tested script rather than a single long inline bash line.src/automation/mod.rs (1)
794-794: Consider removing unused_auto_started_sessionvariable.This variable is set at line 822 but never read. If it's intended for future use, consider adding a TODO comment; otherwise, it can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` at line 794, The variable `_auto_started_session` is declared and later assigned but never read; remove its declaration and any assignments to `_auto_started_session` (search for `_auto_started_session` in this module) to eliminate the unused variable, or if you intend to keep it for future logic, replace the declaration with a clearly marked TODO comment (e.g., `// TODO: preserve _auto_started_session for future session-tracking`) so linters and reviewers know it's intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/automation/verify_clean_baseline.sh`:
- Line 25: The BASE_SHA assignment interpolates $BRANCH_JSON directly into the
python -c string, risking shell injection; change it so the script passes
BRANCH_JSON as an argument to Python (use sys.argv or argparse inside the
one-liner) and open that argument instead of embedding the variable, e.g. invoke
python3 -c with sys.argv[1] and supply "$BRANCH_JSON" as the external argument;
update the line that sets BASE_SHA to reference BRANCH_JSON only as a shell
argument and parse it inside the Python code to safely read the 'base_sha'
field.
In `@scripts/automation/wait_coderabbit.sh`:
- Around line 31-32: The script currently stores the full PR JSON in DATA and
passes it as a single argv to the embedded Python (via RESULT=$(python3 - <<'PY'
"$DATA")), which can hit argument-length limits for large `comments`; change the
flow to avoid passing the full payload on the command line — either pipe DATA
into Python's stdin or write DATA to a temporary file and have the Python
snippet read from stdin or that file. Update the use-sites of DATA/RESULT and
the python invocation in wait_coderabbit.sh so the Python code reads from
standard input (or reads the temp file path provided) instead of receiving the
full JSON as an argument, and ensure the temporary file (if used) is created
securely and cleaned up.
- Around line 53-65: The fallback loop currently treats any CodeRabbit comment
as a completed review; update the logic in the comments loop (variables:
comments, for c in comments, author, body) to ignore kickoff-only comments
emitted by the bot (e.g. messages containing "review triggered", "Review
triggered.", "kickoff", or other kickoff marker) by checking body
(case-insensitive) and continuing the loop instead of printing PASS/exit for
those; keep the existing rate-limit check ("rate limit") but only treat
non-kickoff CodeRabbit comments as a real review and then print PASS and exit.
In `@src/automation/mod.rs`:
- Around line 1128-1153: The error message in the StepResult created after
wait_for_prompt_activity_or_output incorrectly says "within 20s" while the call
uses Duration::from_secs(60); update the message string in that StepResult (the
one containing "implement_code retry did not start activity or produce output
within 20s") to reflect the actual timeout (e.g., "within 60s" or interpolate
the timeout), referencing the wait_for_prompt_activity_or_output call and the
StepResult construction so both stay consistent.
In `@src/permissions/mod.rs`:
- Around line 78-84: The helper suggested_prefix_rule currently returns None for
single-token command lines because it requires at least two tokens; update
suggested_prefix_rule(command_line: &str) so that if tokens.is_empty() it
returns None, if tokens.len() == 1 it returns Some(tokens[0].to_string()) and
otherwise returns Some(format!("{} {}", tokens[0], tokens[1])); this ensures
single-token suggestions (e.g., "make") are returned while preserving the
two-token behaviour used elsewhere.
In `@src/worktree/mod.rs`:
- Around line 199-212: git_current_branch currently returns the literal "HEAD"
in a detached HEAD state which breaks direct branch comparisons in
automation/mod.rs; change git_current_branch to detect when the output equals
"HEAD" and return an explicit detached indicator (e.g., change its signature to
return Result<Option<String>> where Ok(None) means detached) so callers (like
automation/mod.rs) can handle detached state explicitly (update callers to treat
None as detached or compare against commit SHA if they prefer); reference
git_current_branch, automation/mod.rs, and cli/up.rs when making the
adjustments.
In `@tests/fixtures/runtime/codex_idle_prompt_bar.txt`:
- Around line 1-16: The fixture codex_idle_prompt_bar.txt contains Claude UI
output and is unused by tests; either delete this file or replace its contents
with real Codex idle-prompt output and update/rename it to match intent (or
rename to claude_idle_prompt_bar.txt if it’s intended for Claude) so tests can
reference it; locate the fixture file (codex_idle_prompt_bar.txt) and either
remove it from the fixtures directory and any test fixture lists, or overwrite
its content with authentic Codex output matching the existing
claude_idle_prompt_bar.txt structure and ensure any tests/reference names are
updated accordingly.
In `@tutti.toml`:
- Around line 286-289: The "stop_idle_agents" command uses chained && in the run
string which aborts remaining stops if any "cargo run ... down" fails; update
the run for id "stop_idle_agents" so each agent stop is attempted independently
(e.g., separate sequential commands or use a no-fail postfix like "|| true" or
";" between the four cargo run down invocations) so planner, conductor, tester
and docs-release are each executed even if one fails.
---
Nitpick comments:
In `@src/automation/mod.rs`:
- Line 794: The variable `_auto_started_session` is declared and later assigned
but never read; remove its declaration and any assignments to
`_auto_started_session` (search for `_auto_started_session` in this module) to
eliminate the unused variable, or if you intend to keep it for future logic,
replace the declaration with a clearly marked TODO comment (e.g., `// TODO:
preserve _auto_started_session for future session-tracking`) so linters and
reviewers know it's intentional.
In `@src/cli/permissions.rs`:
- Around line 587-588: The test currently asserts that the persisted config
"saved" contains "echo blocked", which would still pass if a wildcard slip-in
like "echo blocked *" appears; change the assertion to check the exact persisted
content instead (e.g., compare saved to the exact expected string or assert
equality/trimming rather than using contains). Locate the test that reads the
file via global_config_path() (the variable named saved) and replace the loose
contains check with an exact match against the expected on-disk config text to
ensure wildcard removal is validated precisely.
In `@tutti.toml`:
- Around line 76-77: The inline complex bash in the run key (the cargo run ...
issue-claim acquire command with conditional ISSUE_MILESTONE handling and output
to .tutti/state/auto/selected_issue.json) should be extracted to a small
executable script (e.g., scripts/acquire-issue.sh) that accepts ISSUE_LABEL,
ISSUE_MILESTONE, lease TTL and output path as args or environment variables;
update the run value to call that script with the same env vars (ISSUE_LABEL,
ISSUE_MILESTONE, ISSUE_MILESTONE check moved into the script) so the conditional
logic and command composition live in a reusable, tested script rather than a
single long inline bash line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b2f0e6da-23ad-4227-8122-88e975ba34fd
📒 Files selected for processing (27)
README.mddocs/CODEX_SDLC_ORCHESTRATION.mddocs/examples/tutti-codex-sdlc.tomldocs/pr-review-loop.mdscripts/automation/create_issue_branch.shscripts/automation/verify_branch_has_changes.pyscripts/automation/verify_clean_baseline.shscripts/automation/wait_coderabbit.shscripts/automation/write_implement_result.pysrc/automation/mod.rssrc/cli/issue_claim.rssrc/cli/land.rssrc/cli/mod.rssrc/cli/permissions.rssrc/cli/runs.rssrc/cli/send.rssrc/cli/up.rssrc/health/mod.rssrc/main.rssrc/permissions/mod.rssrc/runtime/claude_code.rssrc/runtime/mod.rssrc/worktree/mod.rstests/fixtures/runtime/claude_idle_prompt_bar.txttests/fixtures/runtime/codex_idle_prompt_bar.txttests/fixtures/runtime/codex_idle_prompt_marker.txttutti.toml
Take main's idle detection (startup_grace, consecutive working polls) from merged PR #71. Restore expanded Claude Code completion/idle patterns (don't ask on, shift+tab to cycle) and working patterns (Searching for, (thinking)) needed for reliable prompt-bar detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nutt-adam
force-pushed
the
wren/pr-review-loop-merge-gate
branch
from
March 19, 2026 08:06
c20e9af to
072af0b
Compare
- verify_clean_baseline.sh: pass branch.json path via argv instead of interpolating into Python string (shell injection risk) - wait_coderabbit.sh: pipe PR JSON via stdin instead of argv (arg length limits); filter out kickoff-only CodeRabbit comments before declaring review available - automation/mod.rs: fix stale "20s" in error message (actual timeout is 60s) - permissions/mod.rs: return suggestion for single-token commands instead of None - worktree/mod.rs: handle detached HEAD (returns "HEAD") by returning None instead of the literal string - Remove misnamed unused fixture codex_idle_prompt_bar.txt - tutti.toml: use semicolons instead of && for stop_idle_agents so all agents are stopped even if one is already down Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai I've addressed your requested changed. Re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/land.rs (1)
35-44:⚠️ Potential issue | 🔴 CriticalRun the merge gate against the final branch state.
maybe_enforce_merge_gate()executes beforecommit_wip_if_needed()and before the--prpush path. That lets plainlandauto-commit fresh worktree changes after the PR was already approved/green, and it makesland --prfail before it can create/update the PR the gate expects. Gate after the branch is in its final pushed state, or reject dirty worktrees when the gate is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/land.rs` around lines 35 - 44, The merge gate is being run too early; move maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate) so it runs after commit_wip_if_needed(&worktree_path, &resolved.agent_name) has finalized the worktree, and for the PR flow call maybe_enforce_merge_gate only after push_and_open_pr(&resolved.project_root, &branch) so the gate evaluates the final pushed branch state; additionally ensure that when enforce_merge_gate is true you call ensure_git_clean(&resolved.project_root) (or keep the existing check) before running the gate to reject dirty worktrees rather than allowing auto-commits after the gate check.
♻️ Duplicate comments (1)
src/automation/mod.rs (1)
1199-1205:⚠️ Potential issue | 🟠 MajorReuse the resolved runtime and configured timeout in the fallback retry path.
This retry block still re-derives runtime from
agent.runtimewith a"claude-code"fallback and then waits a fixed 3600s. That diverges from the already-resolvedruntime/wait_timeout_secs, so agents inheriting a non-claude-codedefault runtime can be polled with the wrong adapter, and retries can hang for an hour despite a tighter workflow timeout.Also applies to: 1245-1251, 2273-2278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/mod.rs` around lines 1199 - 1205, The fallback retry path re-derives the adapter runtime and hardcodes a 3600s sleep, which diverges from the already-resolved runtime and configured timeout; update the retry branches to use the previously-resolved runtime variable (agent_runtime or runtime as used earlier) and the resolved wait_timeout_secs instead of recalculating from config.agents and using 3600, so polling uses the correct adapter and honors the configured timeout across the retry logic (apply the same change for the other occurrences referenced around the retry blocks).
🧹 Nitpick comments (2)
src/permissions/mod.rs (1)
78-84: Add a regression test for the single-token branch.Line 82 adds important behavior (
Some(token)), but there isn’t a direct test for a one-token command likemake. Adding one prevents silent regressions.Proposed test addition
#[test] fn evaluate_command_policy_suggests_rule_from_first_two_tokens_only() { let policy = PermissionsConfig { allow: vec!["git status".to_string()], }; let decision = evaluate_command_policy(Some(&policy), "cargo test --quiet --all"); assert_eq!(decision.suggested_rule.as_deref(), Some("cargo test")); } + +#[test] +fn evaluate_command_policy_suggests_single_token_rule() { + let policy = PermissionsConfig { + allow: vec!["git status".to_string()], + }; + let decision = evaluate_command_policy(Some(&policy), "make"); + assert_eq!(decision.suggested_rule.as_deref(), Some("make")); +}As per coding guidelines, "Write unit tests in each module using
#[cfg(test)] mod tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/permissions/mod.rs` around lines 78 - 84, The function suggested_prefix_rule has a single-token branch returning Some(token) but lacks a regression test; add a unit test inside the module's #[cfg(test)] mod tests that calls suggested_prefix_rule with a one-token command (e.g., "make") and asserts it returns Some("make") to prevent future regressions—place the test alongside other tests in the same file and use assert_eq!(suggested_prefix_rule("make"), Some("make".to_string())) (referencing the suggested_prefix_rule function name to locate where to add the test).src/cli/runs.rs (1)
110-124: Add module tests for the new formatting helpers.This file introduces output-shaping logic in
truncate_run_id()andformat_issue()but no local tests. A couple of narrow assertions here would lock the CLI output down cheaply.As per coding guidelines, "Write unit tests in each module using
#[cfg(test)] mod tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/runs.rs` around lines 110 - 124, Add a local test module (#[cfg(test)] mod tests) in the same file that contains unit tests for truncate_run_id and format_issue: write tests that assert truncate_run_id returns the original string when length <= 12 and returns the first 9 chars + "..." when longer; and tests for format_issue that cover: issue_title = None, issue_title = Some("") (or whitespace) and issue_title = Some("Title") to verify outputs "#<number>" and "#<number> <title>" respectively (construct a minimal crate::state::SdlcRunLedgerRecord value for format_issue inputs), using assert_eq! to validate results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 11: The .gitignore entry `.tutti-worktrees/` is incorrect because
worktrees are created under `.tutti/worktrees/<agent_name>` per
src/worktree/mod.rs, and the existing `.tutti/` rule already covers that path;
remove the `.tutti-worktrees/` line from .gitignore (or if a different directory
was intended, replace it with the correct directory name) so the ignore rules
accurately reflect the actual worktree path.
In `@docs/examples/tutti-codex-sdlc.toml`:
- Around line 74-80: The new workflow step with id "verify_clean_baseline" uses
cwd = "agent_worktree" for agent "implementer" but sdlc-smoke never boots or
ensures the implementer worktree exists; update the sdlc-smoke workflow to
bootstrap the implementer before any steps that use agent_worktree (e.g., add an
ensure_running / "tt up" or existing ensure step for implementer prior to the
"create_branch" and "verify_clean_baseline" steps) so src/automation/mod.rs's
agent_worktree resolver finds .tutti/worktrees/implementer.
In `@scripts/automation/create_issue_branch.sh`:
- Around line 33-52: The script's use of git clean -fd leaves ignored files in
place so the worktree can still differ from origin; update both occurrences of
git clean -fd to remove ignored files as well (e.g., git clean -fdx or git clean
-ffdx) so the post-clean truly matches origin/$BASE_BRANCH; keep the existing
git reset --hard and git checkout -B "$BRANCH" "origin/$BASE_BRANCH" flow and
leave the DIRTY check (git status --porcelain) and BASE_SHA assignment
unchanged.
- Around line 11-16: The script's default ISSUE_JSON and OUT_FILE currently
resolve under the agent worktree (breaking the no-arg flow consumed by
src/automation/mod.rs expecting project_root/.tutti/state/auto/branch.json);
update create_issue_branch.sh to compute defaults relative to the repository
top-level (use the existing TOPLEVEL from git rev-parse --show-toplevel) and set
ISSUE_JSON and OUT_FILE to "$TOPLEVEL/.tutti/state/auto/branch.json" (and the
corresponding .out path) while still allowing callers to override via
environment/args; modify any code paths in create_issue_branch.sh that currently
default to agent-relative paths to use these TOPLEVEL-based defaults so
src/automation/mod.rs consumers continue to find the file.
In `@scripts/automation/wait_coderabbit.sh`:
- Around line 31-34: The heredoc Python block is reading stdin but the script
also pipes DATA into python, causing the heredoc to win and json.load(sys.stdin)
to parse the Python source; fix by passing DATA via an environment variable or
temp file instead of piping: set/export the shell variable DATA before invoking
the heredoc (or write DATA to a temp file) and modify the Python here-doc in the
script (the block that calls python3 with <<'PY' and uses json.load(sys.stdin'))
to read os.environ['DATA'] (or open the temp file) and json.loads() so it parses
the actual JSON string from the DATA variable rather than the heredoc source.
In `@scripts/automation/write_implement_result.py`:
- Around line 49-57: The current logic only verifies base_sha is an ancestor of
HEAD, which still allows accidentally being on the wrong branch (e.g., main)
before pushing HEAD into target_branch; after retrieving commit_sha and before
using target_branch, retrieve the current branch name (e.g., via git rev-parse
--abbrev-ref HEAD or run_git equivalent), compare it to target_branch and abort
with an error if they differ, and keep the existing ancestor check
(variables/functions to touch: commit_sha, base_sha, target_branch, run_git,
GIT_TIMEOUT_SECONDS, and the is_ancestor check) so we never push an unintended
HEAD into the issue branch.
In `@src/automation/mod.rs`:
- Around line 2151-2219: The helpers wait_for_prompt_activity_or_output and
wait_for_prompt_output_file currently accept any existing output file via
path.exists(), which can ingest stale JSON on reruns; change them to record a
baseline (file nonexistence or its modified time/hash) before submitting the
prompt (or proactively remove the old file) and then only treat the path as
success if it either did not exist at baseline and now exists, or its mtime/hash
is newer/different than the baseline; update wait_for_prompt_activity_or_output
(use baseline_pane_hash logic pattern) and wait_for_prompt_output_file to check
the file's creation/modification timestamp or content hash against the baseline
and only return Ok when the file is updated after the attempt started.
- Around line 2263-2286: The call to health::wait_for_agent_idle in
start_and_wait_ready currently discards its Result (let _ = ...), allowing
startup failures to be ignored; change that call to propagate errors (e.g.,
remove "let _ =" and append ? to the health::wait_for_agent_idle(...)
invocation) so start_and_wait_ready returns Err when the agent fails to become
ready, ensuring callers cannot proceed on a half-started session.
In `@src/cli/issue_claim.rs`:
- Around line 466-476: The empty-result error in acquire currently reports only
the label and hides the milestone filter; update the error path after calling
gh_list_issues in acquire to include milestone (when Some) in the user-facing
message so it reports both label and milestone (or "no milestone" / None) —
update the error string construction in acquire (referencing variables
milestone, label and the gh_list_issues result) to show the effective filters so
automation misconfiguration is obvious.
In `@src/cli/runs.rs`:
- Around line 2-12: The import and call to load_active_runs in list() is invalid
because crate::state only exports load_sdlc_run_ledger; either add a
load_active_runs function to the state module that returns the same shape
expected by list(), or update list() to call the existing load_sdlc_run_ledger
(and adapt variable names/usage). Locate list(), replace the
load_active_runs(project_root)? call with a call to
load_sdlc_run_ledger(project_root)? (or implement a new pub fn
load_active_runs(...) in crate::state that delegates to load_sdlc_run_ledger) so
the symbol resolves and the return types match.
In `@tutti.toml`:
- Around line 79-93: The workflow fails because steps "create_branch" and
"verify_clean_baseline" run with cwd="agent_worktree" before the implementer
worktree exists; add a preliminary step that ensures/creates the implementer
worktree (call the same logic as ensure_implementer in src/automation/mod.rs)
and place it before the first agent_worktree usage (i.e., before the step with
id="create_branch"); update the workflow entries around ids "create_branch" and
"verify_clean_baseline" (and the similar blocks at the other ranges) to run only
after that new ensure-implementer step so .tutti/worktrees/implementer is
present when those commands run.
---
Outside diff comments:
In `@src/cli/land.rs`:
- Around line 35-44: The merge gate is being run too early; move
maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate) so
it runs after commit_wip_if_needed(&worktree_path, &resolved.agent_name) has
finalized the worktree, and for the PR flow call maybe_enforce_merge_gate only
after push_and_open_pr(&resolved.project_root, &branch) so the gate evaluates
the final pushed branch state; additionally ensure that when enforce_merge_gate
is true you call ensure_git_clean(&resolved.project_root) (or keep the existing
check) before running the gate to reject dirty worktrees rather than allowing
auto-commits after the gate check.
---
Duplicate comments:
In `@src/automation/mod.rs`:
- Around line 1199-1205: The fallback retry path re-derives the adapter runtime
and hardcodes a 3600s sleep, which diverges from the already-resolved runtime
and configured timeout; update the retry branches to use the previously-resolved
runtime variable (agent_runtime or runtime as used earlier) and the resolved
wait_timeout_secs instead of recalculating from config.agents and using 3600, so
polling uses the correct adapter and honors the configured timeout across the
retry logic (apply the same change for the other occurrences referenced around
the retry blocks).
---
Nitpick comments:
In `@src/cli/runs.rs`:
- Around line 110-124: Add a local test module (#[cfg(test)] mod tests) in the
same file that contains unit tests for truncate_run_id and format_issue: write
tests that assert truncate_run_id returns the original string when length <= 12
and returns the first 9 chars + "..." when longer; and tests for format_issue
that cover: issue_title = None, issue_title = Some("") (or whitespace) and
issue_title = Some("Title") to verify outputs "#<number>" and "#<number>
<title>" respectively (construct a minimal crate::state::SdlcRunLedgerRecord
value for format_issue inputs), using assert_eq! to validate results.
In `@src/permissions/mod.rs`:
- Around line 78-84: The function suggested_prefix_rule has a single-token
branch returning Some(token) but lacks a regression test; add a unit test inside
the module's #[cfg(test)] mod tests that calls suggested_prefix_rule with a
one-token command (e.g., "make") and asserts it returns Some("make") to prevent
future regressions—place the test alongside other tests in the same file and use
assert_eq!(suggested_prefix_rule("make"), Some("make".to_string())) (referencing
the suggested_prefix_rule function name to locate where to add the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76982083-61b7-4199-ba81-2e116cd7abf2
⛔ Files ignored due to path filters (1)
docs/social-preview.pngis excluded by!**/*.png
📒 Files selected for processing (27)
.gitignoreREADME.mddocs/CODEX_SDLC_ORCHESTRATION.mddocs/examples/tutti-codex-sdlc.tomldocs/infographic-github.htmldocs/infographic.htmldocs/pr-review-loop.mdscripts/automation/create_issue_branch.shscripts/automation/verify_branch_has_changes.pyscripts/automation/verify_clean_baseline.shscripts/automation/wait_coderabbit.shscripts/automation/write_implement_result.pysrc/automation/mod.rssrc/cli/issue_claim.rssrc/cli/land.rssrc/cli/mod.rssrc/cli/permissions.rssrc/cli/runs.rssrc/cli/up.rssrc/main.rssrc/permissions/mod.rssrc/runtime/claude_code.rssrc/runtime/mod.rssrc/worktree/mod.rstests/fixtures/runtime/claude_idle_prompt_bar.txttests/fixtures/runtime/codex_idle_prompt_marker.txttutti.toml
- .gitignore: remove redundant .tutti-worktrees/ rule (covered by .tutti/) - CLAUDE.md: document CodeRabbit request-changes workflow - create_issue_branch.sh: resolve defaults relative to PROJECT_ROOT not cwd; use git clean -ffdx for exact baseline including ignored files - wait_coderabbit.sh: fix critical stdin conflict — pass JSON via env var instead of piping stdin into heredoc - write_implement_result.py: validate current branch matches target before pushing HEAD - docs/examples: add ensure_running for implementer before worktree steps in sdlc-smoke - automation/mod.rs: delete stale output_json before sending prompt to prevent wait helpers from short-circuiting on leftover files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
land.rs: move merge gate after commit_wip_if_needed and push, so it evaluates the final branch state instead of gating prematurely. automation/mod.rs: use step's resolved runtime and wait_timeout_secs in fallback retry path instead of re-deriving from config; propagate wait_for_agent_idle errors in start_and_wait_ready with warning on timeout; delete stale output_json before prompt send. issue_claim.rs: include milestone in "no issues found" error message. runs.rs: fix load_active_runs reference (add function to state module); add issue_title, branch, failure_message fields to SdlcRunLedgerRecord; add unit tests for truncate_run_id and format_issue. permissions/mod.rs: add regression test for single-token command suggestion. tutti.toml + docs/examples: add ensure_running for implementer before agent_worktree steps in sdlc-smoke workflow. CLAUDE.md: document CodeRabbit request-changes workflow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs/pr-review-loop.mddocumenting the exact Choir loop (resolve CodeRabbit threads -> auto re-review -> required checks green -> approvals -> merge)landsteps: block when required checks are not green or PR review threads are unresolvedlandstepsValidation
cargo test -qNotes
tt landremains unchanged unlessTT_ENFORCE_MERGE_GATE=1is setSummary by CodeRabbit
New Features
Documentation
Improvements