Conversation
When every auto source fails, keep the most authoritative attempted source's error (app > cli > ide > oauth > offline) instead of the last one written: only .notRunning placeholders still yield to later errors. Add per-source outcome diagnostics: an auto-failure summary line in codexbar usage text output, one debug log line when a pipeline exhausts its strategies, and strategyID + outcome fields in the diagnose export. Part of steipete#3673 (workstream 1); targets the error-masking class behind steipete#3146 and steipete#3662. Source precedence is unchanged.
…r-honesty # Conflicts: # CHANGELOG.md
…r-honesty # Conflicts: # CHANGELOG.md
A set-but-non-executable *_CLI_PATH override was treated as absent and resolution fell through to login-PATH, well-known paths, and shell lookups, so ANTIGRAVITY_CLI_PATH=/nonexistent still resolved the real agy and a background one-shot report fetch could spawn its interactive login. Overrides are now authoritative: unusable overrides fail resolution outright (BinaryLocator.resolveBinary plus the Codex RPC resolver's direct CODEX_CLI_PATH read), and the Claude planner's fallthrough characterization now pins fail-fast. Part of steipete#3673 (workstream 2).
When auto mode has a selected token account or injected OAuth credentials, the identity-free 'agy -p /usage' fallback was suppressed by rethrowing the legacy failure with no trace that the report was skipped. The exclusion now throws a dedicated error whose description keeps the underlying failure and names the skipped identity-free report, so per-source outcomes (usage summary, debug log, diagnose) show it. Placeholder .notRunning failures still pass through unwrapped to keep the first-authoritative fold semantics from steipete#3683 intact. Part of steipete#3673 (workstream 2, conservative branch).
… fails The app/ide account-mismatch error claimed CodexBar is 'using the selected account's OAuth data instead', which is wrong in the all-failed corner where this error stays surfaced while the OAuth source has also failed. State the rejection and the data-source policy instead of promising an OAuth switch. Part of steipete#3673 (workstream 2, copy-only follow-up to steipete#3683).
|
🦞👀 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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 18, 2026, 1:21 PM ET / 17:21 UTC (Revision 4). ClawSweeper reviewWhat this changesThe branch makes invalid Antigravity CLI path overrides stop executable discovery and adds safe explanations when Auto excludes account-unverified usage reports. Merge readiness⛔ Blocked before merge - 3 items remain The PR retains useful work beyond the merged diagnostics changes, and the earlier findings are resolved. No new actionable defect was found; the intentional override compatibility change still needs maintainer acceptance. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s Antigravity provider combines local app, CLI, and OAuth sources into usage results. Executable discovery controls CLI launches, while account-selection rules determine whether fallback reports can supply quotas. flowchart TD
A[Usage refresh] --> B[CLI path resolution]
B --> C{Explicit path usable?}
C -->|Invalid| D[Skip CLI source]
C -->|Valid or normally discovered| E[Try local CLI quota source]
E --> F{Report permitted for selected account?}
F -->|Yes| G[Read usage report]
F -->|No| H[Record safe exclusion and continue fallback]
Decision needed
Why: The current release falls back, while the proposed behavior deliberately stops discovery; source correctness and runtime proof cannot establish acceptance of that upgrade policy. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the exclusion diagnostics, and preserve existing discovery by default with an explicit no-discovery opt-in unless maintainers approve the documented Antigravity-only compatibility change. Do we have a high-confidence way to reproduce the issue? Yes: current main’s resolver falls through an unusable override to ambient discovery, and scoped Auto report exclusions lack the dedicated category. This is source-verified; the reviewer did not execute a current-main reproduction. Is this the best way to solve the issue? Unclear until the compatibility policy is accepted: the implementation is narrow and preserves account guards, but preserving fallback by default would avoid breaking existing invalid-override setups. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1d8cb13d3fba. LabelsLabel 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
HistoryReview history (3 earlier review cycles)
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings and test coverage nits remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens CLI override resolution and makes Antigravity identity-free fallback diagnostics visible.
Changes:
- Makes unusable CLI overrides fail fast.
- Surfaces identity-free report exclusions and improves error messaging.
- Adds per-source diagnostics, tests, documentation, and changelog updates.
File summaries
| File | Reviewed change / final comment |
|---|---|
Tests/CodexBarTests/ProviderDiagnosticExportTests.swift |
Covers diagnostic export behavior. |
Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift |
Updates provider architecture anchors. |
Tests/CodexBarTests/CLIAntigravityFallbackSummaryTests.swift |
Tests CLI fallback summaries. |
Tests/CodexBarTests/ClaudeSourcePlannerTests.swift |
Pins fail-fast Claude override behavior. |
Tests/CodexBarTests/AntigravityCLIUsageReportTests.swift |
Tests identity-free report exclusion. |
Tests/CodexBarTests/AntigravityCLIHTTPSFetchStrategyTests.swift |
Tests fallback behavior and outcomes. |
Tests/CodexBarTests/AntigravityBinaryLocatorTests.swift |
Nit (1 vote): Assert lookup closures are never called. |
Sources/CodexBarCore/Providers/ProviderFetchPlan.swift |
Adds strategy outcomes and diagnostic logging. |
Sources/CodexBarCore/Providers/ProviderDiagnosticExport.swift |
Moderate (1 vote): Preserve an exclusion code or field in diagnose exports. |
Sources/CodexBarCore/Providers/Antigravity/AntigravityStatusProbe.swift |
Moderate (1 vote): Add a stable diagnostic category or flag for identity-free exclusions. |
Sources/CodexBarCore/Providers/Antigravity/AntigravityProviderDescriptor.swift |
Moderate (1 vote): Apply and log resolved errors on terminal strategy returns. |
Sources/CodexBarCore/PathEnvironment.swift |
Moderate (1 vote): Route Kiro resolution through the shared fail-fast resolver or narrow the guarantee. |
Sources/CodexBarCore/CodexExecutableResolver.swift |
Nit (2 votes): Add coverage for an unusable CODEX_CLI_PATH with an ambient executable. |
Sources/CodexBarCLI/CLIUsageCommand.swift |
Displays Antigravity fallback summaries. |
Sources/CodexBarCLI/CLIHelpers.swift |
Formats per-source summaries. |
docs/antigravity.md |
Documents override and fallback behavior. |
CHANGELOG.md |
Records the fixes. |
Review details
Suppressed comments (5)
Sources/CodexBarCore/PathEnvironment.swift:370
- This makes the shared resolver fail-fast only for callers that actually use it. The Kiro production strategy still calls
TTYCommandRunner.which("kiro-cli"), andKiroStatusProbelaunches that result, soKIRO_CLI_PATH=/nonexistentcan still discover and spawn an ambientkiro-cli(and a valid override is ignored). Route Kiro availability and execution throughBinaryLocator.resolveKiroCLIBinaryusing the fetch context environment, or narrow the all-override guarantee.
if let override = env[overrideKey] {
return fileManager.isExecutableFile(atPath: override) ? override : nil
Sources/CodexBarCore/Providers/Antigravity/AntigravityProviderDescriptor.swift:236
- This fold is only used to update
lastAvailableError;ProviderFetchPipeline.fetchstill returns the current error immediately whenshouldFallbackis false. Antigravity's OAuth strategy does that when no offline data exists, and the offline strategy is terminal too, so an OAuth/offline error can still mask an earlier app/CLI/IDE failure and bypass the new per-source debug log. Apply the resolved error on terminal returns (and log that path), with a regression test using the real terminal strategy behavior.
guard (previous as? AntigravityStatusProbeError) == .notRunning else {
return previous
Sources/CodexBarCore/Providers/Antigravity/AntigravityStatusProbe.swift:795
- This wrapper makes the skip visible in the CLI text and debug log, but the diagnose export maps each attempt to only
errorCategory, anderrorCategoryLabelhas no case for this new error. For example, the timed-out exclusion is exported asunknown, socodexbar diagnosestill cannot tell that the identity-free report was intentionally skipped, contrary to the stated visibility goal. Add a stable redacted category/flag for this case (and pin it in the export tests) rather than relying on the raw description.
case let .identityFreeReportExcluded(underlyingDescription):
"\(underlyingDescription) Identity-free CLI usage report skipped: "
+ "it cannot prove which Google account the quota belongs to."
Sources/CodexBarCore/Providers/ProviderDiagnosticExport.swift:295
- The new exclusion reason is discarded when attempts are converted for
codexbar diagnose: this initializer keeps only the coarseerrorCategoryderived fromerrorDescription, so the exported CLI attempt still says onlyfailed/api(ornetwork) and does not reveal that the identity-free report was skipped. Preserve a non-sensitive exclusion code/field in the diagnostic payload (without exporting raw error text) and add a regression test so the diagnose export actually shows this boundary.
outcome: attempt.outcome.rawValue,
wasAvailable: attempt.wasAvailable,
errorCategory: attempt.errorDescription.map(Self.errorCategoryLabel))
Tests/CodexBarTests/AntigravityBinaryLocatorTests.swift:34
- The test name promises that no ambient lookup runs, but both injected lookup closures merely return
nil, so a regression that calls either closure would still pass. Make the closures record a test failure (or count calls and assert zero) so the new no-fallthrough guarantee is actually covered.
let commandV: (String, String?, TimeInterval, FileManager) -> String? = { _, _, _, _ in nil }
let aliasResolver: (String, String?, TimeInterval, FileManager, String) -> String? = { _, _, _, _, _ in nil }
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let override = environment["CODEX_CLI_PATH"] { | ||
| // A set-but-unusable override is authoritative: fail instead of falling | ||
| // through to ambient resolution, which would launch a real binary the | ||
| // override was meant to suppress. | ||
| guard FileManager.default.isExecutableFile(atPath: override) else { return nil } |
![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: 98df78cc66
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard (previous as? AntigravityStatusProbeError) == .notRunning else { | ||
| return previous |
There was a problem hiding this comment.
In Auto mode with OAuth credentials but no offline database, AntigravityOAuthFetchStrategy.shouldFallback returns false when OAuth fails. ProviderFetchPipeline then immediately returns the current OAuth error rather than the lastAvailableError produced by this fold, so an earlier substantive app/CLI failure is still replaced by the less-authoritative OAuth failure in this common all-sources-failed path. Return the folded error on terminal failures, or allow this pipeline to exhaust before returning.
Useful? React with 👍 / 👎.
| let detail = switch attempt.outcome { | ||
| case .failed: | ||
| "failed: \(attempt.errorDescription ?? "unknown error")" |
There was a problem hiding this comment.
When debug logging is enabled, every exhausted provider pipeline now writes raw localizedDescription values to persistent logs. These descriptions are not safe diagnostic fields: for example, Factory parse failures include the first 200 characters of response JSON, and the Claude OAuth code explicitly avoids logging localized descriptions because server bodies can contain identifying information. LogRedactor only covers a limited set of patterns, so log the already-derived safe error category/outcome instead of attempt.errorDescription and surfacedError.localizedDescription.
Useful? React with 👍 / 👎.
| if let override = env[overrideKey] { | ||
| return fileManager.isExecutableFile(atPath: override) ? override : nil |
There was a problem hiding this comment.
When CLAUDE_CLI_PATH is set to an empty or whitespace-only value, ClaudeCLIResolver.resolvedBinaryPath removes the key before calling this shared resolver, so the new authoritative-override branch is bypassed and Claude is still discovered through PATH or well-known locations. This leaves the same unwanted background-spawn behavior that fail-fast overrides are intended to prevent for one of the listed override keys; retain the normalized empty value so resolution returns nil rather than treating it as unset.
Useful? React with 👍 / 👎.
…guard # Conflicts: # CHANGELOG.md # Tests/CodexBarTests/AntigravityCLIHTTPSFetchStrategyTests.swift # docs/antigravity.md
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
…guard # Conflicts: # CHANGELOG.md
…ity/identity-guard # Conflicts: # CHANGELOG.md # Sources/CodexBarCore/Providers/Antigravity/AntigravityStatusProbe.swift # Tests/CodexBarTests/AntigravityCLIHTTPSFetchStrategyTests.swift # docs/antigravity.md
hhh2210
changed the title
clawsweeper
Bot
added
proof: sufficient
…guard # Conflicts: # CHANGELOG.md # Tests/CodexBarTests/CLIEntryTests.swift # Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift # Tests/CodexBarTests/ProviderDiagnosticExportTests.swift
Treat an unusable ANTIGRAVITY_CLI_PATH as authoritative so background refreshes cannot discover and launch a different agy. Preserve other providers' lookup behavior and simplify the final optional fallback. Adopt the scoped override fix from #3689, with synthetic red-to-green regressions, documentation, and the 0.64.1 release note. Document current CLI-only quota behavior separately from desktop-free OAuth setup (#3146). Co-authored-by: hhh2210 <hzy2210@gmail.com> (cherry picked from commit b275fcd)
Treat an unusable ANTIGRAVITY_CLI_PATH as authoritative so background refreshes cannot discover and launch a different agy. Preserve other providers' lookup behavior and simplify the final optional fallback. Adopt the scoped override fix from #3689, with synthetic red-to-green regressions, documentation, and the 0.64.1 release note. Document current CLI-only quota behavior separately from desktop-free OAuth setup (#3146). Co-authored-by: hhh2210 <hzy2210@gmail.com> (cherry picked from commit b275fcd)
|
Adopted the Antigravity-specific explicit-path guard and resolver regression coverage in #3847 (b275fcd). An unusable |
The shared per-source diagnostics have landed in #3683 and are included in 0.61.0. This PR now contains only the remaining Antigravity override and identity-free report boundaries. Refs #3673, #3649, #3650, and #3662.
An unusable
ANTIGRAVITY_CLI_PATHpreviously fell through to an ambientagy. A background usage refresh could therefore launch the CLI and trigger interactive login even when the explicit path was intended to disable discovery.agy -p /usagereport when an account is selected or OAuth credentials are injected. Diagnose exports and debug logs retain the safeidentity_free_report_excludedcategory. Explicit CLI mode keeps its report fallback.notRunningandmissingCSRFTokenplaceholders. The merged fix(antigravity): complete per-source failure diagnostics #3683 pipeline, upstream proof-isolation fix, and released diagnostics note are retained unchanged.Compatibility decision for maintainers: treating an unusable explicit Antigravity path as disabling ambient discovery is intentional and changes main's fallback behavior for that provider. This policy still needs maintainer acceptance; the unrelated providers' contracts are unchanged.
The latest main is merged. The two unshipped changes are documented in a new Unreleased section. Released changelog history exactly matches main, with #3683 correctly recorded under 0.61.0 and the stale duplicate under 0.60.5 removed.
Validation
make check,git diff --check, exact source-anchor checks, and comparison of shared pipeline/CLI files with upstream main passed.05d6b4993: all nine GitHub checks passed, including both macOS test shards and all Linux CLI builds.This update integrates main and fixes changelog placement; the guard/exclusion behavior is unchanged from that validated head. Local Swift/AppKit suites were not rerun for this integration. Current-head CI is pending after the push.