review: drop Antigravity capture wiring (keep migration detection)#41
review: drop Antigravity capture wiring (keep migration detection)#41kridaydave wants to merge 7 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Shreyan1
left a comment
There was a problem hiding this comment.
Thanks Kriday, splitting the #39 review into four PRs was exactly the right move and I appreciate how fast you turned this around. The Antigravity removal itself is spot on: adapter, provider, ALLOWED_ADAPTERS, HOOK_ADAPTERS, PARKED_PROVIDERS all gone cleanly, and the migration detection correctly kept. Merged with current main the suite is green (1052).
What's holding it up is scope and freshness, not the removal:
- This branch is 7 commits behind main and carries the whole #39 async refactor (28 files), so the actual "drop Antigravity" change is buried under bootstrap/storage/migrate/menu/Windows edits. A reviewer should be able to open the diff and see just the removal. Could you rebase onto current
origin/mainand bring over only the Antigravity removal? It should be about 6 files. - Two CI failures, both of which clear on rebase: the version-bump gate (this is user-facing, so bump
package.json+VERSIONinsrc/cli.ts), and CodeQL flags a HIGH atstorage.ts(see inline) that is not on current main, you are reintroducing it by carrying the old #39trimIfNeeded.
Concretely: git fetch origin main, branch fresh off it, cherry-pick just the removal. Everything else here belongs in #42/#43. I left a couple of inline notes, plus a shared process note (same root cause across all four) in a separate comment below.
| 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.
CodeQL flags this as a HIGH TOCTOU: you stat here, then readFileSync on the next line, and the file can change in between. Heads up, current main does not have this, it comes from the #39 rewrite this branch carries (main still wraps the statSync in its own try/return). Rebasing on main drops the alert. If you do want to keep this shape, read through a single fd (open then fstatSync then read), which is the exact pattern readFileBytesSafe already uses further down this file.
| // Render the menu as a plain string (no cursor control) so tests can assert it. | ||
| export function renderMenu(items: MenuItem[], selected: number): string { | ||
| const width = Math.max(...items.map(it => it.label.length)); | ||
| let width = 0; |
There was a problem hiding this comment.
Example of the scope issue: this renderMenu rewrite has nothing to do with dropping Antigravity, it is only in the diff because the branch carries all of #39. Not wrong, just not reviewable in a PR titled "drop Antigravity". It should ride in whatever PR actually owns the async refactor.
|
One note that applies to all four (#41, #42, #43, #44), since it is the same root cause each time, and a bit about what I hit running them locally. First, credit where it is due: every one of the four fixes is technically correct. You read the review precisely. The problems are git hygiene and test coverage, not the engineering. What actually happened when I pulled and ran these:
If you are driving these with a Codex agent, five instructions fix all of it at once:
Cleanest path from here: rebase all four on current main, stack #42/#43/#44 on #41, slim each to its real diff, add the two missing tests (concurrency primitive + cache regression), and fix the config- |
e93f532 to
4a53364
Compare
Split out of PR #39 (kridaydave:fix/good-first-issues) to address review concern #1.
We made a deliberate, documented call in
detect.tsand 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 inPHILOSOPHY.md("no half-working features dressed up to look finished").fromAntigravityguesses the payload shape andAntigravityCliProviderguessesagy -p -(which, per Google's own docs, a non-interactiveagyalso needs--headless/--approveor it hangs).Most importantly, adding
antigravitytoALLOWED_ADAPTERS/HOOK_ADAPTERSto 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.tsandsrc/providers/antigravity-cli.tsALLOWED_ADAPTERS,HOOK_ADAPTERS,PARKED_PROVIDERSThe Antigravity migration detection in
detect.tsstays (it only switches the Gemini setup path to inject-only, it does not register capture hooks).The other three concerns (bounded concurrency #2, the
captureDisabledcache fix #3, config-parse dedup #4) land in their own PRs, also split from #39.