[piekstra]

Follow-up to the review of #2.

Every error from the token refresh was wrapped as ErrSessionRejected, so a DNS failure, connection reset, timeout, 429 or 5xx from the token endpoint exited 3 and told the user to run granola login, which does not help with any of them.

• persistingTokenSource wraps ErrSessionRejected only when the token endpoint answers with a 4xx other than 429 (oauth2.RetrieveError), such as invalid_grant for a revoked or expired grant. That still exits 3 with the sign-in hint.
• Other refresh failures wrap the new mcpclient.ErrTokenRefresh with their cause, and classifyError maps it to exit 5. Login errors are unchanged. An unreachable server already exited 5 on main, because the MCP transport reports it as a JSON-RPC error, and a refresh that fails the same way now matches.
• The README exit-code table says which failures need a new sign-in.

Tests: TestTransientRefreshFailureIsNotAuthError covers a 503, a 429 and an unreachable server; each exits 5 without the sign-in hint. On main, all three exit 3. TestFailedRefreshIsAuthError (a revoked refresh token) still exits 3. make check passes.

[monit-reviewer]

Automated PR Review

Reviewed commit: aab3e7a797ae
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
documentation:docs	0
architecture:solid-reviewer-agnostic	1
security:code-auditor	0
policies:conventions	0

architecture:solid-reviewer-agnostic (1 finding)

Minor - cmd/errors.go:33

U-L2 / U-I1: two layers now interpret *oauth2.RetrieveError. mcpclient.refreshRejected decides which token-endpoint responses mean the session was rejected, and cmd.classifyError matches the same oauth2 library type to choose exit 5. The cmd layer now relies on a transport-library type crossing the mcpclient boundary, while the rejected-versus-transient decision is owned by mcpclient. The errors.As match is also wider than the refresh path. Any RetrieveError reaching classifyError now exits 5, including one from the granola login code exchange (internal/oauthlogin builds its handler on auth.NewAuthorizationCodeHandler and oauth2.Config), which used to exit 1. That is probably acceptable as upstream, but it was not stated or tested. Suggested fix: have mcpclient return its own typed error for a refresh failure that did not reject the session, for example var ErrTokenRefresh = errors.New(...) wrapped as fmt.Errorf("%w: %w", ErrTokenRefresh, err) in persistingTokenSource.Token. Then classifyError can check errors.Is(err, mcpclient.ErrTokenRefresh) and drop the oauth2 import, matching how ErrSessionRejected and ToolError are already handled. If login exchange failures should also exit 5, say so in the classifyError doc comment and add a test. This is defensible either way and does not block; it is a boundary-hygiene issue, not a behavior bug in the refresh path.

Reviewer Coverage

• documentation:docs — complete (constrained); inspected 1 assigned file (5 inspected across reviewers): README.md; skipped: none; constraints: none
• architecture:solid-reviewer-agnostic — complete (broad); inspected 4 assigned files (5 inspected across reviewers): cmd/cli_test.go, cmd/errors.go, cmd/fake_server_test.go, internal/mcpclient/mcpclient.go; skipped: none; constraints: Ran go test ./... at the PR head; all packages pass. No repo-specific architecture rules found (CLAUDE.md and AGENTS.md only point to external docs), so the universal rules apply.
• security:code-auditor — complete (broad); inspected 2 assigned files (5 inspected across reviewers): cmd/errors.go, internal/mcpclient/mcpclient.go; skipped: none; constraints: none
• policies:conventions — complete (broad); inspected 3 assigned files (5 inspected across reviewers): README.md, cmd/errors.go, internal/mcpclient/mcpclient.go; skipped: none; constraints: This is a public open-cli-collective repo with no Monit-specific tenant, auth, secrets, or logging surfaces; the reviewed change is purely CLI error-classification logic with no cross-tenant or trust-boundary impact.

Inspected files (5)

• README.md
• cmd/cli_test.go
• cmd/errors.go
• cmd/fake_server_test.go
• internal/mcpclient/mcpclient.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 1m 37s | ~$1.90 (est.) | claude-sonnet-5, claude-opus-5-5 | cr 0.10.319

Field	Value
Model	claude-sonnet-5, claude-opus-5-5
Reviewers	documentation:docs, architecture:solid-reviewer-agnostic, security:code-auditor, policies:conventions
Engine	claude_cli · claude-sonnet-5, claude-opus-5-5
Reviewed by	cr · monit-reviewer
Duration	1m 37s wall · 2m 55s compute
Cost	~$1.90 (est.)
Pricing basis	anthropic-public-2026-09-23
Tokens	50 in / 9.0k out

Per-workstream usage

• orchestrator-selection — claude-sonnet-5
  • In: 6
  • Out: 1.8k
  • Cache read: 59.2k
  • Cache create: 68.9k
  • Cost: ~$0.31 (est.)
  • Duration: 23s
• documentation:docs — claude-sonnet-5
  • In: 10
  • Out: 1.3k
  • Cache read: 179.3k
  • Cache create: 72.9k
  • Cost: ~$0.34 (est.)
  • Duration: 22s
• architecture:solid-reviewer-agnostic — claude-opus-5-5
  • In: 14
  • Out: 3.1k
  • Cache read: 276.6k
  • Cache create: 61.2k
  • Cost: ~$0.61 (est.)
  • Duration: 57s
• security:code-auditor — claude-sonnet-5
  • In: 8
  • Out: 1.4k
  • Cache read: 154.6k
  • Cache create: 40.8k
  • Cost: ~$0.21 (est.)
  • Duration: 34s
• policies:conventions — claude-sonnet-5
  • In: 6
  • Out: 909
  • Cache read: 98.4k
  • Cache create: 38.6k
  • Cost: ~$0.18 (est.)
  • Duration: 27s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 421
  • Cache read: 113.3k
  • Cache create: 57.6k
  • Cost: ~$0.26 (est.)
  • Duration: 10s