Skip to content

fix(agent): expose --scope admin + regression test for SEC-A-09#412

Closed
intendednull wants to merge 1 commit into
claude/adoring-euler-3axK7from
auto-fix/issue-311-agent-scope
Closed

fix(agent): expose --scope admin + regression test for SEC-A-09#412
intendednull wants to merge 1 commit into
claude/adoring-euler-3axK7from
auto-fix/issue-311-agent-scope

Conversation

@intendednull
Copy link
Copy Markdown
Owner

Why

PR #389 closed bulk of #311 (SEC-A-09): added --scope {messaging,read,full}, default Messaging, threaded into serve_http. Two gaps remained.

  1. Issue spec listed readonly|messaging|full|admin. admin value not exposed on CLI even though TokenScope::Admin exists in scopes.rs (semantically distinct from Full for audit log consumers).
  2. No test pinned the regression — nothing prevents future drift back to --scope full default.

What

  • Add Admin variant to ScopeArg. Maps to TokenScope::Admin.
  • Doc on ScopeArg now lists per-scope tool sets.
  • New mod tests in crates/agent/src/main.rs:
    • default_scope_is_messaging — pins SEC-A-09 fix.
    • parse coverage every documented value (messaging/read/full/admin).
    • unknown-value-rejected case.
    • ScopeArgTokenScope wiring map.

Lowest tier covering behaviour per CLAUDE.md: clap Cli::parse_from on the bin. No process spawn needed.

Behaviour change

None vs current claude/adoring-euler-3axK7. PR #389 already flipped HTTP default FullMessaging. This PR only adds admin value (additive) and locks the existing default with a test.

Tradeoff considered

Considered flipping TokenScope::default() from Full to Messaging to kill the footgun globally. Rejected. Stdio transport (serve_stdio uses WillowMcpServer::new which reads the Default) is local-process — Full appropriate. Flipping Default would break tests/e2e.rs silently or force every test to with_scope(TokenScope::Full). Worth a follow-up issue to remove the Default impl entirely and force each call site explicit.

Test plan

  • cargo fmt --check clean
  • cargo clippy -p willow-agent --all-targets -- -D warnings clean
  • cargo test -p willow-agent --bin willow-agent 7/7 new tests pass
  • cargo test -p willow-agent --lib 31/31 pass
  • cargo test -p willow-agent --test e2e 32/32 pass

Refs #311

https://claude.ai/code/session_014KLEYzesqZwWdFQnUeXtrJ


Generated by Claude Code

Issue #311 (SEC-A-09) original fix in PR #389 added
`--scope {messaging,read,full}` with Messaging default. Two
gaps remain.

1. Issue spec called for `readonly|messaging|full|admin`. The
   `admin` value is not exposed on the CLI even though
   `TokenScope::Admin` exists in scopes.rs and is semantically
   distinct from Full for audit log consumers. Added Admin
   variant to ScopeArg with mapping to TokenScope::Admin.

2. No test verified that omitting --scope yields the safer
   Messaging default — the regression that was the heart of
   SEC-A-09. Added tests::default_scope_is_messaging plus
   parse coverage for every documented value, an
   unknown-value-rejected case, and the ScopeArg → TokenScope
   wiring map. Covers behaviour at the lowest tier (Rust unit
   on the binary) per CLAUDE.md test-tier guidance.

Doc on ScopeArg now lists per-scope tool sets so operators
can pick without reading scopes.rs.

Refs #311

Tradeoffs:
- Considered changing TokenScope::default() from Full to
  Messaging to remove the footgun globally. Rejected: stdio
  transport (serve_stdio uses WillowMcpServer::new which
  reads the Default) is a local-process channel where Full is
  appropriate; flipping Default would either break stdio
  silently or force every test in tests/e2e.rs to call
  with_scope(TokenScope::Full). Out of scope for this issue.
  Worth a follow-up to make the Default explicit per call
  site and remove the Default impl entirely.
@intendednull
Copy link
Copy Markdown
Owner Author

Is this still needed?

Copy link
Copy Markdown
Owner Author

good catch. #389 already shipped --scope {messaging,read,full} + Messaging default + thread into serve_http. issue auto-close keyword chain didn't fire (closes #301 #304 #305 #311 — GH only closed first id), so #311 stuck open.

what #412 actually adds:

  1. admin CLI variant — issue spec listed readonly|messaging|full|admin, admin was missing. TokenScope::Admin already exists in scopes.rs but unreachable from CLI.
  2. regression test pinning default_scope_is_messaging so default can't drift back to Full.

3 options:

your call. defaulting to close if no preference.


Generated by Claude Code

@intendednull
Copy link
Copy Markdown
Owner Author

Clean up please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants