Polish /permissions command output into a bounded card - #205
Conversation
Replace status: ok style with commandCard format used by /tools and /context. Summary shows mode and grant count; grants render as bullet rows. Co-Authored-By: Claude <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesPermissions card transcript rewrite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/command_views.go (1)
283-301: ⚡ Quick winAdd test coverage for the grant list error case.
The error path when
sandboxStore.List()returns an error is not covered by tests. The nil store and empty grants cases are tested inTestPermissionsCommandCardHandlesNilStoreAndEmptyGrants, but the error case should also be validated to ensure the error card renders correctly.🧪 Example test to add
func TestPermissionsCommandCardHandlesGrantListError(t *testing.T) { store := &stubGrantStore{listErr: errors.New("storage failure")} text := model{permissionMode: agent.PermissionModeAsk, sandboxStore: store}.permissionsText() for _, want := range []string{ "Permissions", "ask permissions", "grants error", "mode ask", "error: storage failure", } { assertContains(t, text, want) } assertNotContains(t, text, "status: warning") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/command_views.go` around lines 283 - 301, Add test coverage for the error case in the grant list retrieval path. Create a new test function TestPermissionsCommandCardHandlesGrantListError that uses a stubGrantStore with a listErr field set to a test error, then calls permissionsText() and verifies that the error card renders correctly by asserting that expected strings like "Permissions", "ask permissions", "grants error", and "error: storage failure" appear in the output, while also asserting that unexpected strings like "status: warning" do not appear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/command_views.go`:
- Around line 283-301: Add test coverage for the error case in the grant list
retrieval path. Create a new test function
TestPermissionsCommandCardHandlesGrantListError that uses a stubGrantStore with
a listErr field set to a test error, then calls permissionsText() and verifies
that the error card renders correctly by asserting that expected strings like
"Permissions", "ask permissions", "grants error", and "error: storage failure"
appear in the output, while also asserting that unexpected strings like "status:
warning" do not appear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1f26d6b-7dc9-4c91-962b-e210965092e9
📒 Files selected for processing (3)
internal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/model_test.go
Adds TestPermissionsCommandCardHandlesGrantListError and a grantLister interface so tests can inject a stub store without a real file path. Co-Authored-By: Claude <noreply@anthropic.com>
Tabs alignment only; no functional change. Co-Authored-By: Claude <noreply@anthropic.com>
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review — Polish /permissions into a bounded card
Verdict: Approve. Clean, well-scoped, well-tested, and the security-sensitive bit (grant-reason redaction) is preserved. CI green across all platforms; TUI-only, 3 files.
What it does
Converts /permissions from the legacy commandOutput (status: ok dump) to the commandCard format already used by /tools and /context: a bold summary line, an aligned mode field, and bulleted grant rows. The nil-store / List() error / empty-grants cases now render as cards (state in the summary) instead of status: warning|blocked.
Strengths
- Redaction preserved (defense in depth). Grant reasons are redacted in
SandboxGrantSnapshots(sandbox_snapshot.go:50) and every card line/row passes throughredaction.RedactStringviacompactCommandOutputText(command_output.go:118,123). The tests assert[REDACTED]present andsk-proj-sensitiveabsent — no leak through the new path. ✅ - Good testability seam.
permissionsTextWithStore(grantLister)+ the smallgrantListerinterface let the new error-path test inject a stub store without reaching for a real filesystem path. Idiomatic. - Coverage. New
TestPermissionsCommandCardHandlesNilStoreAndEmptyGrantsexercises nil-store, empty-grants, andList()-error; existing tests are updated to the card format and addassertNotContainsfor the oldstatus: ok/Permission mode:markers (guards against silent regression). formatGrantCountpluralizes 0/1/n correctly. Provider-neutral, no new deps, no network/fs/exec.
Minor (non-blocking) suggestions
- Lines vs Rows in the Grants section. The success path uses
Rows(renders- text), the nil/error paths useLines(no bullet), and the empty case usesRows{Text:"none"}(- none). Harmless, but a single shape for the Grants section across all states would read more uniformly. - Status semantics for nil/error. The card has no status badge, so the old
warning/blockedstates are now conveyed only by the summary phrase (grants unavailable/grants error). That's the intended alignment with/tools//context— just confirm the error state still reads as clearly "something's wrong".
Risk: low
Rendering-only; no change to permission enforcement and no new surface. CI green (smoke macos/ubuntu/windows, perf, security, CodeRabbit).
2 participants
What
Converts
/permissionsfrom the oldstatus: okplain-text dump into the boundedcommandCardformat already used by/toolsand/context.Before
After
Changes
internal/tui/command_views.go—permissionsText()now renders acommandCardwith a bold summary line, alignedmodefield, and bullet rows for each persistent sandbox grant.status: warning/status: blocked.internal/tui/command_polish_test.goandinternal/tui/model_test.goassertions.TestPermissionsCommandCardHandlesNilStoreAndEmptyGrants.Validation
go test ./internal/tui -count=1✅go vet ./internal/tui✅go run ./cmd/zero-release build✅go run ./cmd/zero-release smoke✅Pre-existing unrelated failure:
TestRunExecUsesProjectConfigAndOpenAICompatibleProviderininternal/clifails on cleanmainwithanthropic provider anthropic requires official baseURL https://api.anthropic.com.Summary by CodeRabbit
/permissionsoutput with structured, card-based sections for clearer State and Grants details, including mode and persistent grant counts./permissionsrendering tests to cover nil, empty, and failure cases, and to ensure outdated flat status markers are no longer shown.