Support read-only Claude account adapters - #3658
benedictfrancis wants to merge 1 commit into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dfcf784cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| let supportsAccountSwitching: Bool | ||
| if let rawSupportsAccountSwitching = object["supportsAccountSwitching"] { | ||
| guard let value = rawSupportsAccountSwitching as? Bool else { |
There was a problem hiding this comment.
JSONSerialization represents both JSON numbers and booleans as NSNumber, and Swift's conditional bridge lets numeric 0 and 1 pass as? Bool. Consequently, a malformed payload such as "supportsAccountSwitching": 1 is accepted as true, making account snapshots actionable and allowing the switching flow instead of rejecting the malformed capability. Verify that the value's Core Foundation type is CFBooleanGetTypeID(), as the numeric parser below already does.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 15, 2026, 7:40 AM ET / 11:40 UTC. ClawSweeper reviewWhat this changesThe PR adds an optional capability to Claude account adapters that disables account activation while preserving usage display and existing adapters’ default switching behavior. Merge readiness⛔ Blocked before merge - 3 items remain This remains a useful, bounded contribution absent from current main. The previously reported Boolean-validation defect remains unresolved, and the supplied verification does not demonstrate the changed runtime behavior. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads account and usage data from a user-configured Claude adapter executable. It converts that data into account cards whose activation controls can invoke an explicit account-switch command. flowchart TD
A[Configured Claude adapter] --> B[Account list JSON]
B --> C[Validate switching capability]
C --> D[Account snapshots]
D --> E[Usage cards]
E --> F{Activation allowed?}
F -->|No| G[Inspect usage only]
F -->|Yes and clicked| H[Explicit account switch]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the additive capability on the existing adapter path, validate genuine JSON booleans strictly, and preserve legacy switching when the field is omitted. Do we have a high-confidence way to reproduce the issue? Yes, from source on the PR branch: use numeric 1 for supportsAccountSwitching in an otherwise valid list containing an inactive eligible account; the Boolean bridge accepts it and leaves activation enabled. This was not executed, and current main lacks the new field. Is this the best way to solve the issue? Yes in architecture: reusing the existing canActivate path is narrow and preserves legacy defaults. The parser must reject numeric substitutes before the implementation is correct. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 9db44805dc1e. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Respect supportsAccountSwitching=false in schema-v1 account lists while preserving usage, account details, and active markers. Keep the existing switching default when the field is absent and reject nonboolean values. Cover parsing, projection, and suppression of credential commands. Reuse quota parsing and remove single-use projection wrappers to keep the production diff at zero net lines. Adopts #3658. Co-authored-by: benedictfrancis <benedict@techniciti.eu> (cherry picked from commit 86c1bd0)
Respect supportsAccountSwitching=false in schema-v1 account lists while preserving usage, account details, and active markers. Keep the existing switching default when the field is absent and reject nonboolean values. Cover parsing, projection, and suppression of credential commands. Reuse quota parsing and remove single-use projection wrappers to keep the production diff at zero net lines. Adopts #3658. Co-authored-by: benedictfrancis <benedict@techniciti.eu> (cherry picked from commit 86c1bd0)
|
Thanks @benedictfrancis. Your implementation is adopted in #3846 ( |
2 participants
Summary
Verification