Skip to content

Fix commented-out Codex base URL overrides - #4330

Merged
steipete merged 3 commits into
steipete:mainfrom
lishouxian:fix/codex-commented-base-url
Oct 8, 2026
Merged

steipete merged 3 commits into
steipete:mainfrom
lishouxian:fix/codex-commented-base-url

Conversation

@lishouxian

@lishouxian lishouxian commented Oct 7, 2026 •

Copy link
Copy Markdown

Problem

A commented-out chatgpt_base_url at the start of a line is treated as an active override. For example:

# chatgpt_base_url = "http://127.0.0.1:8788/backend-api/"

CodexBar attempts to fetch usage from that address instead of the default endpoint, resulting in a network error when the old local service is stopped. A commented override can also shadow an active setting later in the file.

Change

Preserve empty subsequences when splitting a line at #, so a full-line comment leaves an empty prefix that is skipped. Existing inline-comment handling is preserved.

Add five regression cases covering full-line comments with and without whitespace, fallback to the default usage endpoint, and an active override following a comment with a trailing inline comment.

Built production-path proof

Verified locally on October 7, 2026 using the actual CLI built from PR head 765869ff644e30d389c1ff526591ab97b29bbb32:

$ swift build --product CodexBarCLI
Build complete! (52.00 seconds)

A temporary CODEX_HOME contained an on-disk config.toml with a commented loopback override followed by an active loopback mock-server override:

# chatgpt_base_url = "http://127.0.0.1:<inactive-port>/backend-api/"
chatgpt_base_url = "http://127.0.0.1:<mock-port>/backend-api/" # mock server

The harness used a synthetic auth.json (fake access/refresh tokens and a fresh timestamp), a separate version-1 CODEXBAR_CONFIG enabling only Codex, a temporary home, and CODEXBAR_DISABLE_KEYCHAIN_ACCESS=1. Both endpoint candidates were loopback addresses. It did not use real credentials, browser cookies, or a live OpenAI endpoint.

The same CLI arguments and fixture were exercised against the installed v0.72.0 binary and the newly built PR binary:

usage --provider codex --source oauth --no-credits --format json

installed v0.72.0:
  exit: 1
  source: oauth
  error: Network error: The request timed out.
  active mock-server requests: []

PR head 765869f (.build/debug/CodexBarCLI):
  exit: 0
  source: oauth
  active mock-server requests:
    GET /backend-api/wham/usage
    GET /backend-api/wham/rate-limit-reset-credits
  usage.primary.usedPercent: 12
  usage.secondary.usedPercent: 34
  usage.dataConfidence: exact

PASS: built production CLI reads config.toml, ignores the commented endpoint,
      and fetches the mock usage values.

The mock server returned fixed 12%/34% usage values; the harness asserted the successful exit, the actual /backend-api/wham/usage request, and the decoded 34% weekly value. This exercises file loading, endpoint resolution, HTTP fetching, and usage decoding in the compiled production CLI, without extracting/reimplementing those paths. The before comparison is the installed v0.72.0 release, not a separately rebuilt base revision.

Validation

  • A standalone Swift harness using the actual parser extracted from the source reproduced three failures across seven cases before the change; all seven pass after the change. This is parser-level validation, not a successful package test run.
  • SwiftFormat lint: passed, including the new test file.
  • SwiftLint strict: passed with the Command Line Tools framework directory supplied through DYLD_FRAMEWORK_PATH.
  • git diff --check: passed.
  • make test / make test-fast: blocked by the installed Python lacking os.waitid required by the test runner.
  • Direct swift test --filter CodexOAuthTests with Scripts/test_environment.sh: build blocked because actool requires full Xcode; only Command Line Tools are installed.
  • make check: stopped in the existing packaged-app launch isolation test because the synthetic ambient home acquired a Library directory. Format and lint checks were subsequently run separately.

No real account credentials or external live provider calls were used in these tests.

The upstream CI run is currently action_required for this fork PR. Maintainer approval is needed before hosted checks can run; full make test / make check success is not claimed.

@clawsweeper

clawsweeper Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added 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. labels Oct 7, 2026
@clawsweeper

clawsweeper Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed October 7, 2026, 8:06 PM ET / October 8, 2026, 00:06 UTC (Revision 4).

ClawSweeper review

What this changes

The PR corrects parsing of commented Codex endpoint overrides, adds isolated regression tests, and documents the behavior.

Merge readiness

⛔ Needs changes before merge - 1 item remains

The fix remains necessary on current main and the latest release. No actionable patch defect was found, and production-path proof is sufficient; repository-required validation remains incomplete.

Priority: P2
Reviewed head: 3d0e4a2b93e8303b3bcab6e5e7ab7057ce82a5a2

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with meaningful production CLI evidence and isolated regression coverage; full repository validation remains outstanding.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The compiled CLI loaded a real configuration file, skipped the commented endpoint, reached a loopback server through the production HTTP client, and decoded the expected quota values. The demonstrated production fetcher blob is unchanged at the current head; no stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The compiled CLI loaded a real configuration file, skipped the commented endpoint, reached a loopback server through the production HTTP client, and decoded the expected quota values. The demonstrated production fetcher blob is unchanged at the current head; no stored-data contract changes.
Evidence reviewed 8 items Introduced repair: The pinned introduction delta changes one split argument, preserving the empty prefix before a leading comment marker so the existing empty-line guard skips it. Active settings and inline-comment handling retain their existing path.
Still necessary on main: Fetched main still uses omittingEmptySubsequences: true in the parser. The file loader and default endpoint resolver remain present, so a commented endpoint can still shadow a later active setting.
Latest release retains the defect: The v0.73.0 source also uses omittingEmptySubsequences: true; this repair is not present in the latest release.
Findings None None.
Security None None.

How this fits together

CodexBar reads Codex configuration to choose the endpoint for authenticated usage requests. Returned quota data feeds the app and CLI usage displays.

flowchart TD
 A[Codex configuration file] --> B[Ignore comments]
 B --> C{Active override?}
 C -->|Yes| D[Configured endpoint]
 C -->|No| E[Default endpoint]
 D --> F[Authenticated HTTP request]
 E --> F
 F --> G[Quota display]
Loading

Before merge

  • Complete next step (P2) - Complete make test and make check in a supported macOS/full-Xcode environment and resolve failures attributable to this PR.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test growth production +1/-1, tests +39 The repair replaces one parser argument and adds focused coverage without growing production code.

Technical review

Best possible solution:

Keep the existing endpoint resolver and apply this focused comment-parsing correction with isolated regression coverage.

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

Yes, current-main source establishes that a leading comment marker loses its empty prefix and lets the commented endpoint shadow active configuration. Contributor CLI evidence confirms the failure on v0.72.0; this review did not execute current main.

Is this the best way to solve the issue?

Yes, preserving the empty prefix is the narrowest repair and retains existing override, fallback, and inline-comment handling.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 844b0e19bbbb.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: A bounded configuration-parser defect can prevent Codex usage fetching when a disabled endpoint remains in the configuration.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The compiled CLI loaded a real configuration file, skipped the commented endpoint, reached a loopback server through the production HTTP client, and decoded the expected quota values. The demonstrated production fetcher blob is unchanged at the current head; no stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The compiled CLI loaded a real configuration file, skipped the commented endpoint, reached a loopback server through the production HTTP client, and decoded the expected quota values. The demonstrated production fetcher blob is unchanged at the current head; no stored-data contract changes.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Yuxin Qiao: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Complete make test and make check in a supported macOS environment with full Xcode, resolving failures attributable to this PR.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (3 earlier review cycles)
  • reviewed 2026-10-07T15:45:41.508Z sha 765869f :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-07T15:53:50.235Z sha 765869f :: needs changes before merge. :: none
  • reviewed 2026-10-07T15:57:41.886Z sha 765869f :: needs changes before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Oct 7, 2026
@lishouxian

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

Merge the reviewed main baseline without rewriting the contributor commit.
Exercise commented and active overrides through isolated config.toml files,
document comment handling, and record contributor credit for steipete#4330.
@steipete
steipete merged commit f8b75cf into steipete:main Oct 8, 2026
1 check passed
@steipete

steipete commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Thanks @lishouxian! Merged in f8b75cf: a commented-out Codex base URL override was still being applied because the parser matched the key inside the comment; commented overrides are now ignored, with parser and on-disk configuration regressions. Ships in the next release.

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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants