feat: add JSON API mode for account workflows#150
Draft
Loongphy wants to merge 1 commit into
Draft
Conversation
Owner
Author
|
@greptile review |
@loongphy/codex-auth-darwin-arm64
@loongphy/codex-auth-darwin-x64
@loongphy/codex-auth-linux-arm64
@loongphy/codex-auth-linux-x64
@loongphy/codex-auth-win32-arm64
@loongphy/codex-auth-win32-x64
@loongphy/codex-auth
commit: |
Contributor
Greptile SummaryThis PR adds a structured
Confidence Score: 4/5Safe to merge; the JSON mode is additive, all CLI paths are preserved, and ownership/cleanup patterns across the new workflow helpers are consistent and correct. The core logic — atomic selector resolution before any mutation, snapshot-before-remove ordering, schema-version-aware plan migration — is well-reasoned and handled correctly. The three style-level findings carry no runtime risk. The one logic note — .edu accounts newly entering the workspace account-name refresh scope — is worth confirming against whether that API endpoint supports edu plans. src/registry/account_ops.zig (isWorkspaceAccount .edu expansion) and src/workflows/switch.zig (page_allocator in printJsonMutationError) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GUI
participant CLI as codex-auth CLI
participant Workflow
participant Registry
GUI->>CLI: "codex-auth list --json"
CLI->>Workflow: "handleList opts.json=true"
Workflow->>Registry: loadRegistry + syncActiveAccountFromAuth
Workflow->>Workflow: refreshForegroundUsage API or local
Workflow->>Workflow: buildListResult AccountView array
CLI-->>GUI: "JSON {schema_version, command, accounts, active_account_key}"
GUI->>CLI: "codex-auth switch query --json"
CLI->>Workflow: handleSwitchQueryJson
Workflow->>Registry: resolveSwitchQueryLocally
alt not_found
CLI-->>GUI: "JSON error code account_not_found"
else ambiguous
CLI-->>GUI: "JSON error code ambiguous_query with candidates"
else direct
Workflow->>Registry: activateAccountByKey + saveRegistry
Workflow->>Workflow: buildSwitchResult AccountView
CLI-->>GUI: "JSON {schema_version, command:switch, switched_to}"
end
GUI->>CLI: "codex-auth remove selector --json"
CLI->>Workflow: handleRemoveJson
Workflow->>Workflow: resolveRemoveSelectors
alt hasNotFound or hasAmbiguous
Workflow->>Workflow: buildSelectorResolutionViews
CLI-->>GUI: "JSON error code selector_resolution_failed with resolutions"
else all resolved
Workflow->>Workflow: buildAccountViewsForIndices snapshot before remove
Workflow->>Registry: removeSelectedAccountsAndPersist
CLI-->>GUI: "JSON {schema_version, command:remove, removed, new_active_account_key}"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant GUI
participant CLI as codex-auth CLI
participant Workflow
participant Registry
GUI->>CLI: "codex-auth list --json"
CLI->>Workflow: "handleList opts.json=true"
Workflow->>Registry: loadRegistry + syncActiveAccountFromAuth
Workflow->>Workflow: refreshForegroundUsage API or local
Workflow->>Workflow: buildListResult AccountView array
CLI-->>GUI: "JSON {schema_version, command, accounts, active_account_key}"
GUI->>CLI: "codex-auth switch query --json"
CLI->>Workflow: handleSwitchQueryJson
Workflow->>Registry: resolveSwitchQueryLocally
alt not_found
CLI-->>GUI: "JSON error code account_not_found"
else ambiguous
CLI-->>GUI: "JSON error code ambiguous_query with candidates"
else direct
Workflow->>Registry: activateAccountByKey + saveRegistry
Workflow->>Workflow: buildSwitchResult AccountView
CLI-->>GUI: "JSON {schema_version, command:switch, switched_to}"
end
GUI->>CLI: "codex-auth remove selector --json"
CLI->>Workflow: handleRemoveJson
Workflow->>Workflow: resolveRemoveSelectors
alt hasNotFound or hasAmbiguous
Workflow->>Workflow: buildSelectorResolutionViews
CLI-->>GUI: "JSON error code selector_resolution_failed with resolutions"
else all resolved
Workflow->>Workflow: buildAccountViewsForIndices snapshot before remove
Workflow->>Registry: removeSelectedAccountsAndPersist
CLI-->>GUI: "JSON {schema_version, command:remove, removed, new_active_account_key}"
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+257
to
+267
| fn printJsonMutationError(err: anyerror, operation: []const u8) anyerror { | ||
| if (err == error.OutOfMemory) return err; | ||
| const message = try std.fmt.allocPrint( | ||
| std.heap.page_allocator, | ||
| "the {s} operation could not be completed; stored state may have changed; run `list --json` before retrying", | ||
| .{operation}, | ||
| ); | ||
| defer std.heap.page_allocator.free(message); | ||
| try cli.json_output.printError("state_uncertain", message, null); | ||
| return error.StateUncertain; | ||
| } |
Contributor
There was a problem hiding this comment.
page_allocator used instead of caller's allocator
printJsonMutationError in switch.zig uses std.heap.page_allocator to format the error message, while the equivalent helper in remove.zig avoids allocation entirely by using a static string. Since operation is always the literal "switch", the formatted string is effectively static — the simplest fix is to drop the allocPrint call entirely and inline the complete static message, matching the remove.zig style and eliminating the page_allocator dependency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/workflows/switch.zig
Line: 257-267
Comment:
**`page_allocator` used instead of caller's allocator**
`printJsonMutationError` in `switch.zig` uses `std.heap.page_allocator` to format the error message, while the equivalent helper in `remove.zig` avoids allocation entirely by using a static string. Since `operation` is always the literal `"switch"`, the formatted string is effectively static — the simplest fix is to drop the `allocPrint` call entirely and inline the complete static message, matching the `remove.zig` style and eliminating the `page_allocator` dependency.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+244
to
+250
| const account_idx = registry.findAccountIndexByAccountKey(@constCast(reg), account_key) orelse return error.AccountNotFound; | ||
| const selected = [_]usize{account_idx}; | ||
| const views = try buildAccountViewsForIndices(allocator, reg, usage_state, &selected); | ||
| std.debug.assert(views.len == 1); | ||
| defer allocator.free(views); | ||
| return views[0]; | ||
| } |
Contributor
There was a problem hiding this comment.
@constCast to work around a mutable-pointer API
registry.findAccountIndexByAccountKey is called with @constCast(reg) because its signature takes a mutable *Registry. If that function is ever extended to mutate the registry, the silent const-stripping here would be an unseen hazard. Consider adding a *const overload for findAccountIndexByAccountKey, or at minimum adding a brief comment explaining why the cast is safe here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/workflows/results.zig
Line: 244-250
Comment:
**`@constCast` to work around a mutable-pointer API**
`registry.findAccountIndexByAccountKey` is called with `@constCast(reg)` because its signature takes a mutable `*Registry`. If that function is ever extended to mutate the registry, the silent const-stripping here would be an unseen hazard. Consider adding a `*const` overload for `findAccountIndexByAccountKey`, or at minimum adding a brief comment explaining why the cast is safe here.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
list,switch, andremovewhile keeping their human-facing CLI behavior aligned.Why
The GUI and CLI ship together and need one stable command contract. The CLI now owns plan normalization and workflow decisions, so JSON consumers receive final product semantics instead of reimplementing backend-value interpretation.
The registry schema remains at version 4. This change does not bump the application version or add a schema 5 migration.
Validation
zig fmt --check src testszig build test --summary failureszig build run -- list$autoreview: patch correct, no actionable findings (0.87 confidence)