fix(limits): restore Claude Code usage tracking - #25
Conversation
howdeploy
left a comment
There was a problem hiding this comment.
Thanks for restoring Claude usage tracking for the default macOS profile. Before merge, please align the credential lookup with the credential store that the launched Claude Code process actually uses, so the fix stays correct across macOS profiles and does not regress Linux or Windows.
- The Keychain service cannot always be hard-coded to
Claude Code-credentials. Current Claude Code usesCLAUDE_SECURESTORAGE_CONFIG_DIRwhen it is defined, otherwise the effectiveCLAUDE_CONFIG_DIR/ default config root. On macOS, a non-default root selects a service with a hashed suffix (Claude Code-credentials-${sha256(NFC(root)).slice(0, 8)}), and Claude also queries the current account (security ... -a <username>). CanvasTTY preserves these environment variables for the launched CLI, but this PR always reads the default service. With multiple profiles it can therefore show limits for the wrong account or report a false signed-out state. Please derive the same service/account from the effective credential root, or fail closed instead of falling back to another profile.
Please add coverage for:
- the default macOS profile;
- custom
CLAUDE_CONFIG_DIR; CLAUDE_SECURESTORAGE_CONFIG_DIR, including the explicit empty-string default-store override;- unchanged file-based behavior on Linux and Windows.
-
Please preserve Keychain timeout/error semantics. At the moment every non-parser
execFilefailure, including a killed timeout, becomesnot-authenticated. A timeout should remaintimeout; malformed stored JSON should be a protocol error; only a missing credential item should mean not authenticated. -
Commit
e2aa440adds a separate Fable row, but this feature is not documented in the PR body and changes the current HOME contract from one selected provider row to potentially two Claude rows. Please either split that feature into a separate PR, or update the UI contract and add coverage for the default six-provider plus Fable layout after product confirmation.
The current head passes all 537 tests, typecheck, build, secret audit, and git diff --check. I could not independently run the real macOS Keychain path from the Linux review host.
e2aa440 to
6113048
Compare
|
Thanks for the detailed review — addressed all three points.
Coverage was added first in Validation after the changes: 543/543 tests, typecheck, production build, secret audit, and |
|
Thanks for the update. I re-reviewed head
Please derive the same effective secure-storage root for both file and Keychain backends, preserve Claude’s presence-based suffix semantics, and mirror its account selection with coverage for these cases. Independent validation on this head: all 543 tests pass, typecheck passes, production build passes, and the secret audit passes. I could not execute the native macOS Keychain path from the Linux review host. |
|
Thanks for the update. I rechecked head Please use the following as the acceptance contract for the next revision. 1. Resolve the effective credential store once, before branching by platformPlease introduce one small resolver that returns both:
Both the file-backed path and the macOS Keychain path must consume that same result. The current early non-macOS return at The required precedence is:
The important part is that “default service” is selection metadata, not Expected behavior:
Keep the existing NFC normalization before SHA-256 hashing. 2. Fix Linux and Windows file lookupAfter resolving the store, non-macOS platforms must read: Please replace the current Linux/Windows test named The replacement test should create two different roots with two different OAuth tokens, set 3. Match macOS service selection by presence semanticsPlease do not decide the suffix with: secureStorageRoot === defaultConfigRootInstead, use the resolver's explicit
Both must query the hashed service. Only the implicit default or an explicitly empty secure-storage override may query 4. Match Claude's Keychain account selectionThe production path must mirror Claude's account selection: with the same validation/fallback behavior to Please add coverage for at least:
This matters because CanvasTTY passes What can remain unchanged
Ready-to-merge criteria
Once those cases are fixed, this PR should be in good shape for another review. |
|
Thanks for pushing this forward — the direction is right (Keychain-backed credentials on macOS + percent alongside the reset countdown), and most of the implementation and test coverage is already in good shape. Before this can merge, the remaining three gaps from the last review still need to be closed:
Acceptance contract from the previous review still stands: one shared resolver producing the effective root + a "use default service" flag before platform branching; the 6-scenario env → path/service table in tests; the fixed linux/win32 test; account selection covered on the real path. Also, since there are no CI checks on this branch: please confirm |
Summary
Restores Claude Code usage-limit tracking while keeping credential selection aligned with the Claude Code process launched by CanvasTTY. The HOME limit row displays both used percentage and reset countdown.
Changes
CLAUDE_SECURESTORAGE_CONFIG_DIR, including its explicit empty-string default-store override; otherwise use the effectiveCLAUDE_CONFIG_DIRor default~/.clauderoot.Claude Code-credentialsfor the default root andClaude Code-credentials-${sha256(NFC(root)).slice(0, 8)}for non-default roots.not-authenticated, killed lookup istimeout, and malformed JSON or other Keychain failures areprotocol-error..credentials.jsonbehavior unchanged on Linux and Windows.Coverage
CLAUDE_CONFIG_DIR.CLAUDE_SECURESTORAGE_CONFIG_DIR.Verification
git diff --checkpasses.