Skip to content

fix(cli): classify token and tracking errors#796

Merged
steipete merged 1 commit into
mainfrom
fix/token-tracking-exit-codes
Jun 13, 2026
Merged

fix(cli): classify token and tracking errors#796
steipete merged 1 commit into
mainfrom
fix/token-tracking-exit-codes

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Summary

  • classify malformed auth tokens import JSON and RFC3339 fields as usage errors (exit 2)
  • classify absent/incomplete Gmail tracking setup as configuration errors (exit 10)
  • validate token payload timestamps before client/account resolution
  • document the stable distinction and add changelog coverage

Verification

  • focused auth/tracking tests
  • full go test ./internal/cmd -count=1
  • make docs-check
  • full make ci
  • structured autoreview: clean, no actionable findings
  • rebuilt-binary smoke: malformed JSON, created_at, and access_token_expires_at all exit 2; tracking opens/key paths exit 10
  • 556-path isolated replay: zero timeouts; only intended class changes (auth tokens import 1→2, gmail track opens 1→10); no new file-writing rows
  • live clawdbot@gmail.com Gmail sender preflight with isolated tracking state: auth valid, real Gmail API path reached, exited 10 before send; no message sent

@clawsweeper

clawsweeper Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 13, 2026, 5:57 PM ET / 21:57 UTC.

Summary
This PR changes token-import JSON/RFC3339 failures to usage exit 2, Gmail tracking setup failures to config exit 10, and adds focused tests plus automation docs and changelog text.

Reproducibility: yes. by source inspection: current main returns raw JSON/time parse errors and plain tracking fmt.Errorf values, and ExitCode maps those to generic exit 1. I did not run tests because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Merge-result surface: 10 files changed, 95 added, 18 removed. The actual GitHub merge commit is limited to the token/tracking exit-code patch against current main.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] This deliberately changes a CLI automation contract: callers that special-cased generic exit 1 for malformed token imports or missing tracking setup will now see exit 2 or 10.

Maintainer options:

  1. Accept the exit-code correction (recommended)
    Treat the generic 1 to 2/10 changes for these local failures as the intended stable automation contract, supported by focused tests, docs, and the PR body's replay evidence.
  2. Ask for broader compatibility proof
    If maintainers consider any additional command families protected, ask for a before/after exit-code table before merging.

Next step before merge

  • Human maintainer handling is required because this collaborator-authored PR is protected from cleanup; no automated repair is indicated.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes local error classification, tests, and docs without adding dependency, CI, secret-storage, or credential-exposure risk.

Review details

Best possible solution:

Land the focused exit-code classification once maintainers accept the documented compatibility change and required checks are green.

Do we have a high-confidence way to reproduce the issue?

Yes by source inspection: current main returns raw JSON/time parse errors and plain tracking fmt.Errorf values, and ExitCode maps those to generic exit 1. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

Yes. The PR uses the existing ExitError helpers for usage/config classifications and adds focused regression coverage and docs for the stable distinction.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 965d9691ec39.

Label changes

Label changes:

  • add P2: The PR fixes a user-visible CLI automation contract with limited blast radius across auth token import and Gmail tracking commands.
  • add merge-risk: 🚨 compatibility: Changing exact exit codes can affect scripts that branch on the previous generic failure status.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external proof gate is not applicable because the PR author is a collaborator; the body nevertheless lists rebuilt-binary smoke, isolated replay, and live Gmail preflight evidence.

Label justifications:

  • P2: The PR fixes a user-visible CLI automation contract with limited blast radius across auth token import and Gmail tracking commands.
  • merge-risk: 🚨 compatibility: Changing exact exit codes can affect scripts that branch on the previous generic failure status.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external proof gate is not applicable because the PR author is a collaborator; the body nevertheless lists rebuilt-binary smoke, isolated replay, and live Gmail preflight evidence.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read in full and its PR review guidance was applied: review via gh pr view/diff without switching branches or modifying files. (AGENTS.md:1, 965d9691ec39)
  • Current main token import behavior: Current main returns raw json.Unmarshal and time.Parse errors from auth tokens import, so those paths fall through to generic exit 1 instead of an ExitError usage class. (internal/cmd/auth_tokens.go:267, 965d9691ec39)
  • Current main tracking behavior: Current main returns plain fmt.Errorf for missing Gmail tracking setup/admin/worker configuration, so those paths are not classified as config exit errors. (internal/cmd/gmail_track_opens.go:33, 965d9691ec39)
  • Exit-code contract: ExitCode only preserves explicit ExitError codes; usage errors use code 2 and config is defined as code 10, matching the PR's classification target. (internal/cmd/exit.go:24, 965d9691ec39)
  • PR diff scope: GitHub's merge commit applies only the 10-file token/tracking exit-code patch against current main, avoiding unrelated current-main docs_sed changes despite branch drift. (6793ce1a8a46)
  • Protected author association: GitHub reports the PR author as steipete with COLLABORATOR association, so ClawSweeper cleanup should keep it open for explicit maintainer handling.

Likely related people:

  • steipete: Current blame ties the auth token import and Gmail tracking config paths to commit d9767e2, and recent dd5ca7f exit-code classification work is in the same CLI validation area. (role: feature-history owner and recent area contributor; confidence: high; commits: d9767e2f0688, dd5ca7f7706d; files: internal/cmd/auth_tokens.go, internal/cmd/gmail_track_helpers.go, internal/cmd/gmail_track_key.go)
  • Lachlan Donald: Recent merged auth work in c58b381 touched internal/cmd/auth_tokens.go and token-store behavior adjacent to the token import path. (role: recent auth contributor; confidence: medium; commits: c58b381c97ad; files: internal/cmd/auth_tokens.go, internal/secrets/token.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 13, 2026
@steipete steipete merged commit 37ce921 into main Jun 13, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant