feat: tt permissions suggest <workflow> for batch pre-approval - #53
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:
📝 WalkthroughWalkthroughAdds a new CLI subcommand Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Handler
participant Config as Config System
participant Resolver as Workflow Resolver
participant Policy as Permission Policy
participant Output as Output Handler
User->>CLI: permissions suggest <workflow> [--apply] [--json]
CLI->>Config: Load global configuration
Config-->>CLI: Config loaded
CLI->>Resolver: Resolve workflow -> ResolvedSteps
Resolver-->>CLI: ResolvedSteps
loop each command step
CLI->>Policy: evaluate_command_policy(command)
Policy-->>CLI: allowed / blocked
end
CLI->>CLI: Collect blocked commands, deduplicate, build report
alt --apply
CLI->>Config: Acquire write lock, append allow-rules, persist
Config-->>CLI: Persisted
end
alt --json
CLI->>Output: Serialize JSON report
else
CLI->>Output: Format human-readable summary
end
Output-->>User: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
|
Synced with latest |
|
@coderabbitai review\n\nAll CI checks are green after syncing with main; requesting fresh pass for merge readiness. |
|
Synced this branch with latest \ again (commit 1688231) to clear the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/permissions.rs (1)
161-258: Consider adding unit tests forrun_suggest.The new
run_suggestfunction implements substantial logic (workflow resolution, permission evaluation, deduplication, optional config mutation, and dual output modes) but lacks test coverage. The existing tests in this module cover other functionality but not the suggest feature.Consider adding tests that verify:
- Blocked commands are correctly identified and deduplicated
- The
--applyflag correctly modifies the config (usingtempfileper guidelines)- JSON output structure matches
PermissionSuggestReport- Edge cases: empty workflow, no blocked commands, all commands already allowed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/permissions.rs` around lines 161 - 258, Add unit tests for run_suggest that cover detection, deduplication, config mutation, JSON output, and edge cases: write tests that set up a temporary workspace (use tempfile) and create a TuttiConfig and GlobalConfig with a known permissions state, then call run_suggest with varying workflows resolved via WorkflowResolver (or stub/mocked resolution) and assert behavior — verify evaluate_command_policy is exercised by asserting blocked commands appear once (deduplicated) in the returned report/printed output, verify --apply updates GlobalConfig.permissions.allow (and that global.save() persists by inspecting the temp file), verify JSON output matches PermissionSuggestReport shape (workflow, total_commands, blocked, applied_rules) using serde_json, and add edge-case tests for empty workflows, no blocked commands, and when all commands are already allowed; reference run_suggest, WorkflowResolver::resolve, evaluate_command_policy, GlobalConfig::load/save, and PermissionSuggestReport to locate code to test.
🤖 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/permissions.rs`:
- Around line 187-203: The loop in run_suggest only handles
ResolvedStep::Command and ignores ResolvedStep::Workflow, so commands inside
nested workflows are never analyzed; update the loop to recursively inspect
ResolvedStep::Workflow entries (or call a helper that walks a ResolvedStep tree)
and apply the same logic (normalize run, skip empty, evaluate_command_policy,
track seen, and push PermissionSuggestion) to commands found inside nested
workflows; reference ResolvedStep::Workflow and reuse the same
decision/evaluation flow used for ResolvedStep::Command so nested workflow
commands get permission suggestions just like top-level commands.
---
Nitpick comments:
In `@src/cli/permissions.rs`:
- Around line 161-258: Add unit tests for run_suggest that cover detection,
deduplication, config mutation, JSON output, and edge cases: write tests that
set up a temporary workspace (use tempfile) and create a TuttiConfig and
GlobalConfig with a known permissions state, then call run_suggest with varying
workflows resolved via WorkflowResolver (or stub/mocked resolution) and assert
behavior — verify evaluate_command_policy is exercised by asserting blocked
commands appear once (deduplicated) in the returned report/printed output,
verify --apply updates GlobalConfig.permissions.allow (and that global.save()
persists by inspecting the temp file), verify JSON output matches
PermissionSuggestReport shape (workflow, total_commands, blocked, applied_rules)
using serde_json, and add edge-case tests for empty workflows, no blocked
commands, and when all commands are already allowed; reference run_suggest,
WorkflowResolver::resolve, evaluate_command_policy, GlobalConfig::load/save, and
PermissionSuggestReport to locate code to test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 089ca710-c862-4f0d-b491-47cbbfa75dd2
📒 Files selected for processing (2)
src/cli/mod.rssrc/cli/permissions.rs
|
Addressed the actionable CodeRabbit blocker in commit 3a8f1dd.\n\n- now recursively inspects nested entries (not just top-level command steps).\n- Added focused tests for nested-workflow detection/dedup and /JSON report behavior.\n- Validation: running 1 test |
|
Correcting prior comment (shell ate backticks): Addressed the actionable CodeRabbit blocker in commit
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Follow-up: fixed CI format failure by running in commit 94a28ce. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Addressed CI blocker: Clippy warning (denied as error) fixed in commit 83c4815.\n\nValidation rerun locally: and running 1 test |
|
Correction: Clippy failure was due to the too_many_arguments lint being treated as error under -D warnings. Fixed in commit 83c4815. Local validation passed:
|
|
Readiness driver update: dismissed stale CodeRabbit CHANGES_REQUESTED review (3954193362) after follow-up fixes landed and checks are green. Remaining hard blocker is branch protection requiring an approving review from a writer other than the PR author. |
|
@coderabbitai please review now |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/cli/permissions.rs (1)
279-281: Make this CLI error actionable for operators.The message is clear but not actionable. Add a concrete remediation hint (for example: run from a directory containing
tutti.tomlor pass the workspace root).💡 Suggested wording tweak
- TuttiError::ConfigValidation("could not determine workspace root".to_string()) + TuttiError::ConfigValidation( + "could not determine workspace root; run `tt permissions suggest` from a workspace containing tutti.toml".to_string() + )As per coding guidelines: "
src/{cli,error}/**/*.rs: User-facing errors should include actionable guidance".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/permissions.rs` around lines 279 - 281, The error returned when determining project_root via config_path.parent() uses TuttiError::ConfigValidation with an unhelpful message; update that error to provide actionable remediation guidance (e.g., tell the operator to run the command from the directory containing tutti.toml or to pass the workspace root via a flag/environment variable). Locate the project_root assignment and replace the current error string ("could not determine workspace root") with a message that includes concrete steps (for example: "could not determine workspace root: run this command from the directory containing tutti.toml or pass the workspace root via --workspace-root"). Ensure the change still constructs TuttiError::ConfigValidation and preserves the ok_or_else usage so behavior and error type remain the same.
🤖 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/permissions.rs`:
- Around line 166-174: The global seen_workflows check is incorrectly
short-circuiting valid repeated nested invocations; replace the global visited
guard with a recursion-stack (cycle detection) guard so only true cycles are
skipped. Add or rename the parameter (e.g., active_workflows: &mut
std::collections::BTreeSet<String> or Vec<String>) and change the logic in the
function that currently uses seen_workflows.insert(workflow.to_string()) ->
Ok(()) to: if active_workflows.contains(workflow) return Err/cycle-handled
Ok(()); else insert into active_workflows, process the workflow (updating
total_commands, seen_commands, blocked, etc.), then remove it from
active_workflows when done; keep or optionally use a separate seen_workflows
cache only after full processing to avoid re-processing identical workflow
definitions but do not use that cache to skip legitimate repeated nested
invocations without cycle detection; update callers to pass the new
active_workflows parameter.
- Around line 505-556: The test mutates HOME unsafely for parallel runs; mark
the test as serial by adding the serial_test attribute (add use
serial_test::serial; and prepend #[serial] to the test function) or move it into
a single-threaded integration test; ensure serial_test is added to Cargo.toml if
missing so the block that sets/ restores HOME around calls to TuttiConfig::load,
suggest_workflow_permissions, and global_config_path() runs sequentially to
avoid race conditions.
---
Nitpick comments:
In `@src/cli/permissions.rs`:
- Around line 279-281: The error returned when determining project_root via
config_path.parent() uses TuttiError::ConfigValidation with an unhelpful
message; update that error to provide actionable remediation guidance (e.g.,
tell the operator to run the command from the directory containing tutti.toml or
to pass the workspace root via a flag/environment variable). Locate the
project_root assignment and replace the current error string ("could not
determine workspace root") with a message that includes concrete steps (for
example: "could not determine workspace root: run this command from the
directory containing tutti.toml or pass the workspace root via
--workspace-root"). Ensure the change still constructs
TuttiError::ConfigValidation and preserves the ok_or_else usage so behavior and
error type remain the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1798e69-6b8d-42f5-a26d-dbd1fea5d568
📒 Files selected for processing (1)
src/cli/permissions.rs
|
Addressed the fresh CodeRabbit requested change in commit 286d493.\n\n- Replaced manual HOME cleanup in with an RAII that restores HOME in , making cleanup panic-safe.\n- Removed the old manual cleanup block to avoid duplicate/fragile restoration paths.\n\nValidation:\n- running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s\n- running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s\n\n@coderabbitai review |
|
Correction (prior comment had shell formatting issues): Addressed the fresh CodeRabbit requested change in commit 286d493.
Validation:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/permissions.rs`:
- Around line 254-270: The read-modify-write in the apply path (when apply &&
!blocked.is_empty()) mutates global.permissions (PermissionsConfig.allow) and
then calls global.save(), which is vulnerable to lost updates under concurrent
runs; update GlobalConfig::save() to acquire an exclusive config-file lock
before writing and perform an atomic write (write to temp file then rename into
place) so all callers including the code that pushes suggested_rule into
policy.allow (variables: apply, blocked, policy, allow, applied_rules) are
protected from races and concurrent overwrites.
- Around line 172-175: The current cycle-check branch silently returns Ok(())
when active_workflows.contains(workflow) is true; change this to return a
user-facing error: construct and return TuttiError::ConfigValidation (or the
equivalent config validation variant) including the offending workflow name/path
(the workflow variable) and an actionable message telling the operator to remove
or fix the cyclic reference so suggestions can be generated; update the branch
where active_workflows.contains(workflow) is checked to return that error
instead of Ok(()), preserving the surrounding function's Result<T, TuttiError>
signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be3a22ec-c4f0-4f56-9364-1501167f458c
📒 Files selected for processing (1)
src/cli/permissions.rs
|
Addressed the two current CodeRabbit blockers in commit f482629.
Validation:
@coderabbitai review |
|
The validation across ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/config/mod.rs`:
- Around line 1113-1123: Collapse the nested `if let` checks by combining
metadata and modification-time retrieval into one `if let` using `and_then` to
satisfy Clippy: replace the two nested `if let Ok(meta) =
std::fs::metadata(&lock_path)` / `if let Ok(modified) = meta.modified()` with a
single `if let Ok(modified) = std::fs::metadata(&lock_path).and_then(|m|
m.modified())` and keep the existing `modified.elapsed().map(|age| age >
stale_after).unwrap_or(false)` check and the `std::fs::remove_file(&lock_path)`
call unchanged (use the same `lock_path` and `stale_after` symbols).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67dd378f-9c5b-443a-a0f3-e39c0e21f5a6
📒 Files selected for processing (2)
src/cli/permissions.rssrc/config/mod.rs
|
Pushed commit 2e44b2c to address current CI blockers: collapsed nested if-let lock checks in src/config/mod.rs (Clippy collapsible_if), then re-ran cargo fmt, cargo clippy -D warnings, and cargo test (all passing locally). |
Dismissing stale bot CHANGES_REQUESTED after follow-up fixes (f482629,2e44b2c) and green CI.
|
Readiness driver: dismissed stale CodeRabbit CHANGES_REQUESTED reviews (3955594302, 3955708325, 3955762481) after follow-up fixes landed and CI stayed green. Current hard blocker is required approving review from a writer other than PR author. |
|
Readiness driver fix: merged latest \ into this branch (commit 7f00ac6) to resolve merge conflicts and clear the DIRTY/CONFLICTING gate. CI/CodeQL re-runs are now queued; remaining blocker after checks is required approving review from a writer other than the PR author. |
|
Correction: merged latest main into this branch (commit 7f00ac6) to resolve merge conflicts and clear the DIRTY/CONFLICTING gate. CI/CodeQL re-runs are now queued; remaining blocker after checks is required approving review from a writer other than the PR author. |
…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>
2 participants
Summary
Adds workflow-level permission suggestion so operators can pre-approve blocked commands before autonomous runs.
What changed
tt permissions suggest <workflow>subcommand.--jsonoutput with blocked command details.--applyto append suggested wildcard rules into~/.config/tutti/config.toml.WorkflowResolver+evaluate_command_policyto inspect resolved command steps.Validation
cargo test -q(all passing)Fixes #37
Summary by CodeRabbit
New Features
Tests
Chores