[auto] #65 phase 1: deterministic planner-to-implementer handoff in SDLC workflows - #66
Closed
nutt-adam wants to merge 7 commits into
Closed
[auto] #65 phase 1: deterministic planner-to-implementer handoff in SDLC workflows#66nutt-adam wants to merge 7 commits into
nutt-adam wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds permission-suggestion hints and an environment-driven PR merge gate for land operations, updates automation to inject the gate, and adds documentation and workflow examples describing the PR review loop and enforcement behavior. Changes
Sequence DiagramsequenceDiagram
participant Automation as Automation (runner)
participant LandCLI as Land CLI (run_with_options)
participant GH as GitHub CLI (gh)
participant API as GitHub GraphQL API
Automation->>LandCLI: run_with_options(..., enforce_merge_gate=true)
activate LandCLI
LandCLI->>GH: find_open_pr_number (gh pr list)
GH-->>LandCLI: PR number / none
alt PR found
LandCLI->>GH: ensure_required_checks_green (gh pr checks)
GH-->>LandCLI: checks status
LandCLI->>API: ensure_all_review_threads_resolved (gh api graphql)
API-->>LandCLI: unresolved thread count
alt all checks green & no unresolved threads
LandCLI-->>Automation: allow land → proceed
else gate fails
LandCLI-->>Automation: fail closed with error details
end
else no PR found
LandCLI-->>Automation: fail closed (no open PR)
end
deactivate LandCLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
…ule hints Adds changelog, version bump, and README updates for v0.2.4 release covering permissions suggest subcommand, land-step merge gate enforcement, allow-rule hints on blocked commands, dry-run command field, and dependency security fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add changelog entry and version bump for permission allow-rule hints and PR merge gate enforcement on land steps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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 after materializing the branch state.
maybe_enforce_merge_gate()runs beforecommit_wip_if_needed()and before the--prpush, so auto-committed or newly pushed changes are not the ones whose checks and review threads were validated. That lets fresh, unreviewed code slip past the new gate.🔒 Gate the exact head that will be landed
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 - 44, The merge gate is being run too early (maybe_enforce_merge_gate is called before commit_wip_if_needed and before push_and_open_pr), so change the call site to run the gate after the branch state is fully materialized: remove the existing maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate)?; call maybe_enforce_merge_gate only after commit_wip_if_needed(&worktree_path, &resolved.agent_name)? has returned and, if pr is true, after push_and_open_pr(&resolved.project_root, &branch)? completes (so the gate validates the exact commit that will be landed); keep ensure_git_clean and ensure_branch_exists where they are.
🤖 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/permissions/mod.rs`:
- Around line 78-85: The function suggested_wildcard_prefix_rule currently
returns a broad wildcard rule (format!("{} {} *")) which is too permissive;
change suggested_wildcard_prefix_rule to return the exact two-token prefix
(e.g., format!("{} {}", tokens[0], tokens[1])) so it emits the bounded prefix
that matching_allow_rule() expects, and update the CLI fallback behavior that
persists suggestions (the "suggest --apply" path) to use the same narrower
two-token prefix instead of a wildcard to avoid unintentionally granting extra
permissions.
---
Outside diff comments:
In `@src/cli/land.rs`:
- Around line 35-44: The merge gate is being run too early
(maybe_enforce_merge_gate is called before commit_wip_if_needed and before
push_and_open_pr), so change the call site to run the gate after the branch
state is fully materialized: remove the existing
maybe_enforce_merge_gate(&resolved.project_root, &branch, enforce_merge_gate)?;
call maybe_enforce_merge_gate only after commit_wip_if_needed(&worktree_path,
&resolved.agent_name)? has returned and, if pr is true, after
push_and_open_pr(&resolved.project_root, &branch)? completes (so the gate
validates the exact commit that will be landed); keep ensure_git_clean and
ensure_branch_exists where they are.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b11ee65-51ae-4d25-a319-4e2fdecce78d
📒 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
Comment on lines
+224
to
+275
| let query = r#"query($owner: String!, $name: String!, $number: Int!) { | ||
| repository(owner: $owner, name: $name) { | ||
| pullRequest(number: $number) { | ||
| reviewThreads(first: 100) { | ||
| nodes { | ||
| isResolved | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }"#; | ||
|
|
||
| let output = Command::new("gh") | ||
| .args(["api", "graphql", "-f", &format!("query={query}")]) | ||
| .args(["-F", &format!("owner={owner}")]) | ||
| .args(["-F", &format!("name={name}")]) | ||
| .args(["-F", &format!("number={pr_number}")]) | ||
| .current_dir(project_root) | ||
| .output()?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| return Err(TuttiError::Git(format!( | ||
| "merge gate failed to query review threads for PR #{}: {}", | ||
| pr_number, | ||
| stderr.trim() | ||
| ))); | ||
| } | ||
|
|
||
| let payload: Value = serde_json::from_slice(&output.stdout).map_err(|e| { | ||
| TuttiError::State(format!( | ||
| "merge gate failed to parse review thread payload for PR #{}: {e}", | ||
| pr_number | ||
| )) | ||
| })?; | ||
|
|
||
| let unresolved = unresolved_review_thread_count(&payload).ok_or_else(|| { | ||
| TuttiError::State(format!( | ||
| "merge gate could not determine unresolved review thread count for PR #{}", | ||
| pr_number | ||
| )) | ||
| })?; | ||
|
|
||
| if unresolved == 0 { | ||
| Ok(()) | ||
| } else { | ||
| Err(TuttiError::Git(format!( | ||
| "merge gate blocked: PR #{} has {} unresolved review thread(s). Resolve all threads before land/merge.", | ||
| pr_number, unresolved | ||
| ))) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'reviewThreads\(first:\s*100\)|pageInfo|hasNextPage|endCursor' src/cli/land.rsRepository: nutthouse/tutti
Length of output: 227
Paginate review-thread checks before declaring the gate green.
The GraphQL query only fetches reviewThreads(first: 100) and does not follow pagination, so a PR with more than 100 review threads can pass the gate even if later threads are unresolved.
🔁 Query sketch with pagination
-query($owner: String!, $name: String!, $number: Int!) {
+query($owner: String!, $name: String!, $number: Int!, $after: String) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
- reviewThreads(first: 100) {
+ reviewThreads(first: 100, after: $after) {
+ pageInfo {
+ hasNextPage
+ endCursor
+ }
nodes {
isResolved
}
}
}
}
}Loop until hasNextPage is false before computing the unresolved count.
Comment on lines
+78
to
+85
| fn suggested_wildcard_prefix_rule(command_line: &str) -> Option<String> { | ||
| let tokens: Vec<&str> = command_line.split_whitespace().collect(); | ||
| if tokens.len() < 2 { | ||
| return None; | ||
| } | ||
|
|
||
| Some(format!("{} {} *", tokens[0], tokens[1])) | ||
| } |
There was a problem hiding this comment.
Suggest the bounded prefix, not a wildcard rule.
A plain rule like cargo test already authorizes cargo test --quiet via matching_allow_rule(). Emitting cargo test * here is broader than needed, and tt permissions suggest --apply will persist that wider rule automatically. Please return the exact prefix here and align the CLI fallback with the same narrower form.
🔧 Narrower suggestion
-fn suggested_wildcard_prefix_rule(command_line: &str) -> Option<String> {
- let tokens: Vec<&str> = command_line.split_whitespace().collect();
- if tokens.len() < 2 {
- return None;
- }
-
- Some(format!("{} {} *", tokens[0], tokens[1]))
+fn suggested_prefix_rule(command_line: &str) -> Option<String> {
+ let tokens: Vec<&str> = command_line.split_whitespace().collect();
+ match tokens.as_slice() {
+ [] => None,
+ [command] => Some((*command).to_string()),
+ [command, subcommand, ..] => Some(format!("{command} {subcommand}")),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn suggested_wildcard_prefix_rule(command_line: &str) -> Option<String> { | |
| let tokens: Vec<&str> = command_line.split_whitespace().collect(); | |
| if tokens.len() < 2 { | |
| return None; | |
| } | |
| Some(format!("{} {} *", tokens[0], tokens[1])) | |
| } | |
| fn suggested_prefix_rule(command_line: &str) -> Option<String> { | |
| let tokens: Vec<&str> = command_line.split_whitespace().collect(); | |
| match tokens.as_slice() { | |
| [] => None, | |
| [command] => Some((*command).to_string()), | |
| [command, subcommand, ..] => Some(format!("{command} {subcommand}")), | |
| } | |
| } |
🤖 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 - 85, The function
suggested_wildcard_prefix_rule currently returns a broad wildcard rule
(format!("{} {} *")) which is too permissive; change
suggested_wildcard_prefix_rule to return the exact two-token prefix (e.g.,
format!("{} {}", tokens[0], tokens[1])) so it emits the bounded prefix that
matching_allow_rule() expects, and update the CLI fallback behavior that
persists suggestions (the "suggest --apply" path) to use the same narrower
two-token prefix instead of a wildcard to avoid unintentionally granting extra
permissions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
305-305: Clarify that the merge gate is environment-driven.The documentation states that workflow land steps "enforce" a GitHub merge gate, but the implementation in
src/cli/land.rs:121-142shows this is controlled by anenabledparameter (via theTT_ENFORCE_MERGE_GATEenvironment variable). Users reading this might assume the gate is always enforced.📝 Suggested clarification
-- Workflow `land` steps enforce a GitHub merge gate (required checks green + all PR review threads resolved) +- Workflow `land` steps support an optional GitHub merge gate (enabled via `TT_ENFORCE_MERGE_GATE` env var) that ensures required checks are green and all PR review threads are resolved before allowing the land operationAlternatively, if the behavior is documented elsewhere in the README or in a dedicated configuration section:
-- Workflow `land` steps enforce a GitHub merge gate (required checks green + all PR review threads resolved) +- Workflow `land` steps enforce a GitHub merge gate when enabled (required checks green + all PR review threads resolved)As per coding guidelines context, the error message in
src/cli/land.rsexplicitly mentions theTT_ENFORCE_MERGE_GATEenvironment variable, confirming this is a configurable feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 305, The README line claiming workflow `land` steps "enforce a GitHub merge gate" is misleading; update the wording to state the merge gate is environment-driven and only enforced when the TT_ENFORCE_MERGE_GATE variable is enabled (see implementation in src/cli/land.rs where the `enabled` / `TT_ENFORCE_MERGE_MATE` check controls behavior). Change the sentence to explicitly mention TT_ENFORCE_MERGE_GATE as the toggle, and optionally add a short note or pointer to the `src/cli/land.rs` logic (the `enabled` parameter) so users know how to enable/disable the merge gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 305: The README line claiming workflow `land` steps "enforce a GitHub
merge gate" is misleading; update the wording to state the merge gate is
environment-driven and only enforced when the TT_ENFORCE_MERGE_GATE variable is
enabled (see implementation in src/cli/land.rs where the `enabled` /
`TT_ENFORCE_MERGE_MATE` check controls behavior). Change the sentence to
explicitly mention TT_ENFORCE_MERGE_GATE as the toggle, and optionally add a
short note or pointer to the `src/cli/land.rs` logic (the `enabled` parameter)
so users know how to enable/disable the merge gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5de0763c-e528-4f58-9e5a-452081cd4932
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
CHANGELOG.mdCargo.tomlREADME.md
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
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.
Automated SDLC cycle for #65.
Summary by CodeRabbit
Documentation
New Features