Skip to content

Clarify Claude OAuth recovery action#2192

Closed
Yuxin-Qiao wants to merge 1 commit into
steipete:mainfrom
Yuxin-Qiao:agent/clarify-claude-oauth-recovery
Closed

Clarify Claude OAuth recovery action#2192
Yuxin-Qiao wants to merge 1 commit into
steipete:mainfrom
Yuxin-Qiao:agent/clarify-claude-oauth-recovery

Conversation

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor

Summary

  • clarify that a suppressed Claude OAuth background repair requires clicking Refresh in the CodexBar menu
  • remove the incorrect claim that merely opening the menu retries the repair
  • cover the user-facing recovery direction in both macOS and Linux delegated-refresh regression tests

Why

With the Keychain prompt policy set to Only on user action, background OAuth repair intentionally fails closed so CodexBar cannot trigger Claude, a Keychain prompt, or a browser launch. Automatic refresh after opening the menu still runs in the background, while the explicit Refresh action is user-initiated. The previous error text incorrectly promised that opening the menu would retry the repair.

Validation

  • swift test --filter 'oauth delegated retry only on user action background suppresses delegation|appOAuthBackgroundRespectsPlatformKeychainPromptPolicy'
  • make check

@Yuxin-Qiao
Yuxin-Qiao marked this pull request as ready for review July 15, 2026 13:32
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 15, 2026, 11:18 PM ET / July 16, 2026, 03:18 UTC.

Summary
The PR changes the suppressed Claude OAuth repair message to require clicking Refresh and adds macOS and Linux assertions for the revised wording.

Reproducibility: yes. at source level: the added Linux assertion requires a message directing users to a macOS menu action that Linux does not provide.

Review metrics: 2 noteworthy metrics.

  • Patch scope: 3 files, 6 changed lines. The narrow copy-and-test change can be corrected without redesigning the OAuth flow.
  • Platform coverage: 2 platform tests changed. Both macOS and Linux expectations are affected, making platform-specific recovery wording necessary.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Make the Linux recovery direction actionable without referring to a macOS menu.
  • [P1] Add a redacted screenshot or terminal/live output showing the corrected recovery message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Focused tests and CI are reported, but no after-fix screenshot, terminal output, or live result shows the changed message; add redacted evidence to the PR body to trigger a fresh review, or ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR supplies tests and CI results but no real app or CLI evidence showing the after-fix recovery message in context.

Maintainer options:

  1. Decide the mitigation before merge
    Provide platform-appropriate recovery guidance—menu Refresh on macOS and an explicit CLI-available retry on Linux—then demonstrate both messages with redacted real output.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The contributor should correct the Linux-visible recovery instruction and add real behavior proof before normal maintainer review.

Security
Cleared: The diff only changes error text and test assertions, with no credential handling, permissions, dependencies, or executable supply-chain changes.

Review findings

  • [P2] Avoid macOS-only recovery text in the Linux path — TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift:68
Review details

Best possible solution:

Provide platform-appropriate recovery guidance—menu Refresh on macOS and an explicit CLI-available retry on Linux—then demonstrate both messages with redacted real output.

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

Yes, at source level: the added Linux assertion requires a message directing users to a macOS menu action that Linux does not provide.

Is this the best way to solve the issue?

No; the macOS direction is appropriate for the app, but a shared Linux-visible message needs platform-appropriate recovery guidance.

Full review comments:

  • [P2] Avoid macOS-only recovery text in the Linux path — TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift:68
    This assertion locks Click Refresh in the CodexBar menu into the shared fetcher's Linux result, but Linux CLI users have no CodexBar menu. Split or rephrase the guidance so macOS points to menu Refresh while Linux describes a CLI-available explicit retry; otherwise this replaces one inaccurate instruction with another.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR addresses misleading OAuth recovery guidance but currently replaces it with an impossible action for Linux CLI users.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Focused tests and CI are reported, but no after-fix screenshot, terminal output, or live result shows the changed message; add redacted evidence to the PR body to trigger a fresh review, or ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: The PR addresses misleading OAuth recovery guidance but currently replaces it with an impossible action for Linux CLI users.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Focused tests and CI are reported, but no after-fix screenshot, terminal output, or live result shows the changed message; add redacted evidence to the PR body to trigger a fresh review, or ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Linux regression expectation: The new Linux assertion requires the shared error message to say “Click Refresh in the CodexBar menu,” although the Linux CLI has no CodexBar menu. (TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift:68, 96956a4d4946)
  • Shared provider message: The recovery instruction is emitted from the shared Claude usage fetcher rather than a macOS-only presentation layer, so the platform-specific wording reaches the Linux-tested path. (Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift:234, 96956a4d4946)
  • Focused validation only: The PR reports focused Swift tests and make check, and all supplied GitHub checks passed, but the body contains no live app or CLI output demonstrating the corrected recovery experience. (96956a4d4946)
  • Refresh-semantics history: Repository release history records later work distinguishing automatic or menu-open refreshes from explicit Refresh actions, supporting the need to correct the stale recovery promise while preserving platform-appropriate guidance. (CHANGELOG.md, d661c1c37d00)

Likely related people:

  • manikv12: Repository release history credits this contributor with the Claude OAuth and Keychain prompt-policy rework across several merged changes. (role: feature owner; confidence: high; files: Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift)
  • zhulijin1991: Recent merged work distinguishing explicit Refresh from automatic and menu-open refresh behavior is directly relevant to this recovery-action wording. (role: adjacent refresh-semantics contributor; confidence: medium; files: Sources/CodexBar/MenuBarController.swift)
  • steipete: Repository and release history show continued integration work around provider refresh, menu behavior, and Claude OAuth changes. (role: merger and recent area contributor; confidence: medium; files: CHANGELOG.md, Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift)
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.

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor Author

Folded this recovery-copy correction into #2191 so the Claude Keychain/OAuth behavior has one canonical merge path.

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

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant