Good first issues: async capture pipeline, Windows fixes, and Antigravity adapter#39
Good first issues: async capture pipeline, Windows fixes, and Antigravity adapter#39kridaydave 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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Shreyan1
left a comment
Shreyan1
left a comment
There was a problem hiding this comment.
Thanks for this, Kriday, this is a serious chunk of careful work and it shows. I pulled the branch, built it, and ran it live rather than just reading the diff. Build, lint, and the full suite are green for me too (1036 passing), and several changes are genuine improvements: the config parsing hardening (stripping inline # comments + anchoring the regexes) passed every probe I threw at it, the formatDiff de-duplication is nice, the O_NOFOLLOW/fstat-on-fd TOCTOU fix is the right shape, and the Windows path handling is thoughtful. 🙏
Before we can merge, though, I ran the changed paths live and found a few things I'd like to work through. I've left inline notes; the priorities:
1. Please pull the Antigravity adapter + provider out of this PR (blocker). We made a deliberate, documented call in detect.ts and the FAQ: we intentionally do not wire Antigravity capture yet, because Google hasn't 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 -; per Google's own docs a non-interactive agy also needs --headless/--approve or it hangs, so the invocation likely wouldn't work anyway. Adding it to ALLOWED_ADAPTERS/HOOK_ADAPTERS to satisfy the parity test is the part I'd push back on hardest: that invariant exists to prove every registered adapter is real and fully wired end-to-end (install + detect + a real fixture), so this makes the test pass without the thing it tests being true. Let's revisit the moment the format is public.
2. Unbounded process concurrency in runGitCapture (reliability regression). Measured live: on a commit touching 400 files I saw a peak of 104 concurrent git child processes (the sequential version on main peaks at 1). A 60-file commit ran all 60 diffs at once (484ms vs ~12s sequential). The post-commit hook has to stay invisible and fail-open, and on constrained CI or very large commits this risks FD/PID exhaustion. Same Promise.all fan-out shape is in bootstrap.ts (findJsonlFiles/findWireJsonlFiles, recursive) and the codex scan in discoverSessions. If we want concurrency here, let's bound it with a small pool rather than firing everything at once. (Side effect: the captured array lost its documented "newest-file-last" ordering; my run printed file17, file43, file33 in random order.)
3. cachedIgnore module-global is a real bug, reproduced. I wrote a 2-test probe against the real source: test A sets CC_HABITS_DISABLE and calls checkIgnoreAsync() (caches true); test B clears the env and calls captureDisabled() expecting false and got true, the stale cached value. In production a hook is short-lived so it self-limits, but in the shared vitest worker this silently poisons captureDisabled() for any later test depending on order, which is exactly the intermittent flakiness the description already flags. Let's scope the cache to a single invocation, or add a reset-for-tests + a note on its lifetime.
4. Housekeeping: no VERSION/package.json bump (we're on 0.9.0 and this is user-facing, per the PR checklist); the -p - stdin change for claude/gemini should be verified against the real binaries before we rely on it (see inline, claude --help shows - is taken as a literal positional prompt, not a stdin sentinel); and readConfigAsync copies the entire body of readConfig, let's share a parse helper.
Bigger picture: this is really five PRs in one (async refactor, Windows fixes, config parsing, CodeQL, Antigravity). We lean hard on depth-over-breadth, and splitting it would let the strong parts (config parsing, the TOCTOU fix, Windows handling) land fast while we work the async/concurrency questions on their own. Happy to review the pieces quickly. Genuinely appreciate the effort here, this is good work that just needs to be a bit smaller and a bit more bounded.
| @@ -0,0 +1,35 @@ | |||
| import { NormalizedHookInput } from './index'; | |||
|
|
|||
| export function fromAntigravity(raw: any): NormalizedHookInput { | |||
There was a problem hiding this comment.
Blocker, and it's about scope rather than code quality: we deliberately don't wire Antigravity capture yet. See isAntigravityMigrated in detect.ts and the gemini-antigravity-migration FAQ entry: because Google relocated hooks into a format it hasn't finalized publicly, we intentionally hold off on capture "rather than register hooks that quietly never run." This adapter guesses the payload shape (toolCall.arguments, write_file, etc.) against a spec that doesn't exist yet, so we can't verify it's correct. Could you drop fromAntigravity (and its entries in ALLOWED_ADAPTERS/HOOK_ADAPTERS/supported.ts) from this PR? When Google publishes the format we'll add it properly with a real payload fixture and the full install/detect wiring.
|
|
||
| async generate(prompt: string, opts: { maxTokens: number; timeoutMs: number }): Promise<string> { | ||
| return new Promise<string>((resolve, reject) => { | ||
| const child = spawn('agy', ['-p', '-'], { |
There was a problem hiding this comment.
Same blocker as the adapter, plus a correctness concern I verified against Google's docs: agy -p - is a guess. Antigravity's headless docs note a plain non-interactive agy call hangs on an approval prompt unless you pass --headless (and --approve), and -p takes the prompt as an argument, so -p - likely sends a literal -. Since we can't test this against a stable contract yet, let's not ship it. (antigravity.google/docs/cli-using)
| encoding: 'utf-8', | ||
| }); | ||
| return new Promise<string>((resolve, reject) => { | ||
| const child = spawn('claude', ['-p', '-'], { |
There was a problem hiding this comment.
Worth verifying before we rely on this (also applies to gemini-cli.ts:16). claude --help shows the usage as claude [options] [command] [prompt], i.e. - is taken as the positional prompt (literal "-"), not a stdin sentinel. The documented ways to feed a prompt are claude -p "<prompt>" (the original arg form) or piping on stdin with no prompt arg (echo "<prompt>" | claude -p). claude -p - most likely sends prompt="-" with the real prompt appended as context. Codex genuinely documents - as stdin, so codex-cli is fine, but claude/gemini need either the arg form or bare -p + stdin. These are parked so it won't fire by default, but the mocked tests can't catch a wrong flag, so let's confirm against the real binary.
| captured.push({ file, commit: sha.slice(0, 7) }); | ||
| // Concurrently diff the modified files | ||
| await Promise.all( | ||
| files.map(async (file) => { |
There was a problem hiding this comment.
This Promise.all(files.map(...)) is unbounded fan-out, and I measured it live: on a 400-file commit it peaked at 104 concurrent git child processes (main's sequential loop peaks at 1); a 60-file commit ran all 60 diffs at once (484ms vs ~12s). This runs from the post-commit hook, which must stay invisible and fail-open, so on constrained CI or a large refactor commit this risks FD/PID exhaustion. Could we bound the concurrency with a small pool (e.g. process files in batches) instead of spawning one git diff per file simultaneously? Same shape exists in bootstrap.ts (findJsonlFiles/findWireJsonlFiles) and the codex scan. Also note the old "newest-file-last" ordering comment was removed along with the guarantee, captured is now race-ordered (my run printed file17/file43/file33).
| // • the CC_HABITS_DISABLE env var (truthy). | ||
| // When either is set, the PostToolUse and Stop hooks become no-ops: nothing is | ||
| // captured, nothing is sent, no marker is printed. | ||
| let cachedIgnore: boolean | null = null; |
There was a problem hiding this comment.
This module-global cache is a real bug, I reproduced it. checkIgnoreAsync() sets cachedIgnore, and captureDisabled() (line 193) returns it with no reset. Probe: set CC_HABITS_DISABLE, call checkIgnoreAsync() -> caches true; clear the env, call captureDisabled() -> still returns true (stale). A hook process is short-lived so prod mostly self-limits, but in the shared vitest worker this silently poisons captureDisabled() for later tests depending on order, which is the intermittent flakiness the PR description already mentions. Could we scope the cache to a single hook invocation (pass it through), or add a resetIgnoreCacheForTests() + a comment on the cache's intended lifetime? Right now captureDisabled() and checkIgnoreAsync() can also disagree within one process.
| const startBoundary = /^[a-zA-Z0-9_]/.test(term) ? '\\b' : ''; | ||
| const endBoundary = /[a-zA-Z0-9_]$/.test(term) ? 's?\\b' : ''; | ||
| const endBoundary = /[a-zA-Z0-9_]$/.test(term) | ||
| ? (term.toLowerCase().endsWith('s') ? '\\b' : 's?\\b') |
There was a problem hiding this comment.
Scope creep I'd like to separate: this changes which triggers fire (a term ending in "s" now gets \b instead of s?\b), i.e. it alters core habit/memory injection matching. That's a real behavior change landing in the "Windows CLI/UI fixes" commit with no dedicated test and no mention in the description. Could you pull it into its own small PR with a test that pins the before/after and a one-line rationale? Then we can reason about it on its own merits.
| return cfg; | ||
| } | ||
|
|
||
| async function readConfigAsync(): Promise<ProviderConfig> { |
There was a problem hiding this comment.
readConfigAsync duplicates the entire body of readConfig (the comment-strip + anchored-regex parsing, and every read(...) assignment). That parsing logic is exactly the kind of thing that drifts when only one copy gets updated. Could we factor the parse into a shared parseConfigText(text): ProviderConfig and have both the sync and async readers just do their own file read + call it? The parsing improvement itself is great, by the way, it passed all my inline-comment probes.
ci: add gates for the issues found in PR #39's review
PR Shreyan1#39 shipped a plumbing-only Antigravity adapter (gamed the existing HOOK_ADAPTERS/ALLOWED_ADAPTERS parity test by adding the same string to two Sets with no real detection or registration), an unbounded git-diff fan-out (measured live at 104 concurrent git processes on a 400-file commit), a module-global cache with no reset that leaked state between tests, and no VERSION bump on a user-facing change. None of it was caught by the existing green build/lint/test. - tests-ts/adapter-wiring-parity.test.ts: checks each HOOK_ADAPTERS id against the real detect.ts/cli.ts source text, not just Set membership, so a plumbing-only adapter fails CI. - .github/workflows/version-bump-check.yml: fails a PR that touches src/** without a package.json version bump (no-version-bump label as the escape hatch for internal-only changes). - vitest.config.ts: sequence.shuffle: true so order-dependent module state (the exact shape of the leaked cache) fails loudly instead of hiding behind whatever order the suite happens to run in; explicit retry: 0 so a flaky test can never be graduated into a passing one by re-running. - CONTRIBUTING.md: documents the wiring-parity requirement, a bounded fan-out convention, and the new checklist items. Verified all four gates against synthetic reproductions of the original bugs before landing: the parity test fails with the PR's exact set-only change, the version-bump check passes/fails correctly across bumped/unbumped/no-src- change scenarios, and 3 shuffled full-suite runs stayed green at 1052/1052.
3 participants
This PR is a batch of good-first-issue fixes for cc-habits. It makes the capture and bootstrap pipeline asynchronous, adds Windows path handling, converts the CLI providers to async spawn with stdin prompts, hardens persistence with a symlink guard, and fixes several Windows CLI and UI issues. It also adds an Antigravity adapter and CLI provider.
The Antigravity adapter and provider are intentionally parked. Google has not finalized the Antigravity hook format, so capture hooks are not auto-registered yet. The adapter and provider are forward-compatible plumbing and are kept in the adapter sets to satisfy the existing test invariant, so they are not orphaned code.
Changes:
Tests: 1011 passing and 25 skipped. One test, command-surface, is intermittently flaky on Windows in the full suite but passes in isolation and on rerun. This is pre-existing and not a regression.
Not included, to be filed as follow-up good-first-issues: the formatSessionBreakdown one-liner refactor and the runMigration delay. Both are optional cleanups and are not part of this change set.