review: dedup config parsing, revert claude/gemini -p - change, bump version#44
review: dedup config parsing, revert claude/gemini -p - change, bump version#44kridaydave wants to merge 9 commits into
Conversation
Part of the review split of PR Shreyan1#39 (kridaydave:fix/good-first-issues). This PR handles concern Shreyan1#1 only. We made a deliberate, documented call in detect.ts and the FAQ: we intentionally do not wire Antigravity capture yet, because Google has not published the finalized hook format, and shipping a capture path that silently never fires would break the transparency/reliability promise in PHILOSOPHY.md ("no half-working features dressed up to look finished"). fromAntigravity guesses the payload shape and AntigravityCliProvider guesses `agy -p -` (which, per Google's own docs, a non-interactive agy also needs --headless/--approve or it hangs). Most importantly, adding antigravity to ALLOWED_ADAPTERS/HOOK_ADAPTERS to satisfy the parity test was the part that should not land: that invariant exists to prove every registered adapter is real and fully wired end-to-end (install + detect + a real fixture), so including it made the test pass without the thing it tests being true. This removes: - src/adapters/antigravity.ts and src/providers/antigravity-cli.ts - the antigravity entries in ALLOWED_ADAPTERS, HOOK_ADAPTERS, PARKED_PROVIDERS - the adapter/provider registration tests The Antigravity *migration detection* in detect.ts stays (it only switches the Gemini setup path to inject-only, it does not register capture hooks). Split from Shreyan1#39; the other three concerns (bounded concurrency, the captureDisabled cache fix, config-parse dedup) land in their own PRs.
…version Part of the review split of PR Shreyan1#39 (kridaydave:fix/good-first-issues). This PR handles concern Shreyan1#4 only. - readConfig and readConfigAsync duplicated the entire YAML-ish parse body. Extract a shared parseProviderConfig() helper and have both call it, so the two paths cannot drift apart. - Revert the claude/gemini CLI providers from `spawn('claude', ['-p', '-'])` back to `spawnSync('claude', ['-p', prompt])`. `-` is a literal positional prompt to claude/gemini, not a stdin sentinel, so the stdin write/end path did not do what the comment claimed. Restoring the original positional form keeps behaviour matching the real binaries. (agy's `-p -` would also hang without --headless/--approve, which is why Antigravity capture is being held off entirely — see the sibling Antigravity PR.) - Bump version 0.9.0 -> 0.9.1 (user-facing change per the PR checklist). Base is the drop-Antigravity PR so providers/index.ts lands cleanly. Split from Shreyan1#39; the other three concerns (drop Antigravity, bounded concurrency, the captureDisabled cache fix) land in their own PRs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Shreyan1
left a comment
Shreyan1
left a comment
There was a problem hiding this comment.
Good cleanup. parseProviderConfig + resolveConfigPath shared between both readers so they cannot drift, and this is the only one of the four that actually bumps the version.
One real bug though, and it is a bit ironic for a "clean up config parsing" PR (inline): the comment stripper truncates any value that contains a #. I ran it against a realistic Ollama config (I have Ollama enabled locally now): ollama_url: http://user:pa#ss@localhost:11434 (an embedded credential with a #) parses to http://user:pa, so extraction would silently connect to the wrong host and fail. A plain sk-a#bc secret becomes sk-a too. The dedup lifted the naive logic into a shared helper, so now both paths corrupt it. Since you are already in here, worth fixing properly and adding a test.
Two smaller notes:
- The PR description says it reverts the claude/gemini
-p -change, but versus current main that is a no-op: main never had-p -(it was only in the #39 blob), soclaude-cli.tsis not even in this diff. Nothing to do, just so the description matches reality. - CodeQL HIGH at
storage.tsfrom the carried #39 base (inline). This one depends on #41, so it also carries #41's blob; once #41 is rebased and slimmed, rebase this on top.
| return cfg; | ||
| } | ||
| const lines = text.split('\n').map(line => { | ||
| const hashIdx = line.indexOf('#'); |
There was a problem hiding this comment.
This is the bug: indexOf('#') strips from the first # anywhere on the line, including inside a value. Verified live against an Ollama config: ollama_url: http://user:pa#ss@localhost:11434 becomes http://user:pa, and openai_api_key: "sk-a#bc" becomes sk-a. Only treat # as a comment when it is at line-start or preceded by whitespace and outside quotes. Please add a test with a # inside a value so it cannot regress.
| return; // absent or unstattable: nothing to trim | ||
| } | ||
| if (size <= LOG_ROTATE_BYTES) return; | ||
| const st = fs.statSync(filePath); |
There was a problem hiding this comment.
Same CodeQL HIGH TOCTOU as the other three (stat here, then readFileSync next line; carried from the #39 trimIfNeeded rewrite, not on current main). Rebasing on main drops it, or read through a single fd like readFileBytesSafe does below.
2 participants
Split out of PR #39 (kridaydave:fix/good-first-issues) to address review concern #4.
readConfigandreadConfigAsyncduplicated the entire YAML-ish parse body. Extract a sharedparseProviderConfig()helper and have both call it, so the two paths cannot drift apart.spawn('claude', ['-p', '-'])back tospawnSync('claude', ['-p', prompt]).-is a literal positional prompt to claude/gemini, not a stdin sentinel, so the stdin write/end path did not do what the comment claimed. Restoring the original positional form keeps behaviour matching the real binaries. (agy's-p -would also hang without--headless/--approve, which is why Antigravity capture is being held off entirely — see review: drop Antigravity capture wiring (keep migration detection) #41.)Depends on / should be merged after #41 (this branch is based on it) so
providers/index.tslands cleanly. It is otherwise independent of #42 and #43.The other concerns (drop Antigravity #1, bounded concurrency #2, the
captureDisabledcache fix #3) land in their own PRs, also split from #39.