fix(review): stop contacting the git remote from the idle freshness poll; add PLANNOTATOR_GIT_REMOTE_CHECK opt-out and kill orphaned git transports on exit - #1585
Merged
Conversation
Three shared pieces the review servers and every host will use for #1553: - `resolveRemoteCheck(cliNoRemoteCheck, config, env)` beside the sibling boolean resolvers, with the usual precedence — the `review --no-remote-check` flag beats `PLANNOTATOR_REMOTE_CHECK`, which beats `{ "remoteCheck": false }` in config.json — and the existing coercion for hand-edited config values. - `--no-remote-check` in `parseReviewArgs`. Every host (Claude Code, OpenCode, Pi) parses review arguments through this one function, so the flag reaches all three rather than being a Bun-CLI-only escape hatch. The field is ABSENT when the flag is not typed: `true` would outrank the env var and config key and make those opt-outs unreachable. - `nextRemoteBaseCheckInterval` plus the two interval constants, so the probe cadence and its failure backoff are one policy both runtimes share instead of duplicated arithmetic that can drift.
Closes the reported half of #1553. `/api/diff/fresh` is polled every 5s for as long as the review page is open, and it carried the 60s remote probe, so an untouched review kept running `git ls-remote --symref origin HEAD` once a minute for the life of the tab. On a smartcard-backed SSH setup that is one hardware-key touch prompt per minute with nothing on screen to explain it — the reporter's YubiKey blinking beside an idle agent. Both runtimes (Bun `packages/server/review.ts` and the Pi `node:http` mirror in `apps/pi-extension/server/serverReview.ts`): - The probe moves off `/api/diff/fresh` and onto `/api/diff/switch`, which backs the diff-type and base pickers AND the "Diff out of date · Refresh" button — the interactions where a reviewer is actually asking for a fresh answer. Startup, `/api/diff` and the explicit `/api/fetch-base` are unchanged. It is kicked off fire-and-forget so a hanging remote can never make a diff switch wait on the network. - `baseBehindRemote` still rides EVERY freshness response from the cached value, so the "behind GitHub" banner neither flickers nor disappears. - Failure backoff: a null probe doubles the interval up to 15 minutes and a success resets it to 60s, through the shared policy helper. Retrying an unreachable remote on the base cadence forever is what made the loop expensive rather than merely chatty. - `--no-remote-check` / `PLANNOTATOR_REMOTE_CHECK` / `{ "remoteCheck": false }` make the session network-free. The startup block is skipped WHOLE, because `detectRemoteDefaultCompareTarget` runs an ls-remote of its own — an opt-out that silenced only the staleness probe would still prompt for authentication when the review opens. The base then stays whatever local ref discovery resolved. `/api/fetch-base` moves to its own predicate and stays reachable: an explicit Fetch is the user asking for the network. Tests are dual-runtime and were each confirmed to fail against the previous behavior: the idle-session guard advances a faked clock past the interval before polling (at real speed the old rate limit would have hidden the bug), the switch guard fails when the new call is removed, and the opt-out guard fails when the startup block is left ungated.
The second half of #1553. Git commands run with `interaction: "forbid"` are spawned detached in their own process group so a timeout can kill the whole git/ssh tree — but that parent-side timer is the ONLY thing that ever reaps them, and it dies with the parent. A server stopped while an ls-remote was still waiting on authentication therefore left the pair orphaned, holding the SSH agent busy long after the review window was gone. Both runtimes now track the process-group leaders of in-flight isolated commands and SIGKILL each group from a synchronous `process.on("exit")` hook, installed lazily on the first such spawn. "exit" is the right hook: it runs on every `process.exit()`, which is where the CLI's SIGINT/SIGTERM handling already routes, and `process.kill` is synchronous. POSIX only, reusing the existing win32 carve-out — the negative-pid group signal has no Windows equivalent, and spawning taskkill from an exit handler is not worth it. The new dual-runtime test gives the command a timeout far longer than the child process's own lifetime, so only the exit hook can account for the transport being dead; it fails with the hook disabled.
Claude Code (both the CLI and the opencode-review stdin path), the OpenCode plugin, and Pi all forward the parsed bit to the review server, which resolves it against PLANNOTATOR_REMOTE_CHECK and config.remoteCheck in one place. The reporter is on OpenCode, so a Bun-CLI-only flag would have missed them. The flag is documented in the `review` subcommand help; the top-level usage line stays the concise summary it already is (it omits --local/--no-local and --json for the same reason).
- AGENTS.md: a PLANNOTATOR_REMOTE_CHECK env-table row spelling out exactly which interactions probe the remote, the 60s rate limit, the failure backoff, and what "off" means; plus the `/api/diff/fresh` row (now strictly local) and the `/api/diff/switch` row (now carries the probe). - README.md: the sentence at line 153 said there is no opt-out. There is one now, and the paragraph also says when the probe runs, so "an idle review page makes no network calls" is checkable rather than implied. - Marketing: an environment-variables row, the code-review guide's "Baseline is behind" section (including the trade-off — a mid-review push is noticed on the next refresh, switch, or reload rather than within the minute), and the blog line that described the ls-remote behavior. - The plannotator skill's review reference, which the freshness test pins against cli.ts.
A rejected stdout/stderr read would otherwise leave the process-group leader in the exit reaper's set, where a recycled pid could be signalled at process exit. The node:http mirror already untracks on both close and error.
The Fetch control is rendered only from the "Baseline is behind" banner (packages/review-editor/App.tsx), and with the remote check off that banner never appears, so "Fetch still works" read as a UI promise the session cannot keep. The /api/fetch-base endpoint does stay reachable, and its post-fetch refreshRemoteBaseInfo() is a no-op under the opt-out; say both.
…eCheck / --no-git-remote-check so it cannot read as remote-session mode
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #1553.
During a code review session that is open but idle — page up, nobody
touching it — the server ran
git ls-remote --symref origin HEADabout once aminute, indefinitely. Closing the page stopped it. Where SSH authentication is
backed by a hardware token, each probe is a physical touch prompt, so the
reporter's YubiKey kept blinking beside an agent that correctly reported itself
as idle; nothing in the UI showed an operation in flight.
Two independent causes, both fixed here:
GET /api/diff/freshevery 5s to keep the "Diff out ofdate" banner honest. That handler called
maybeRefreshRemoteBaseInfo(),whose 60s rate limit was the only thing standing between a 5s poll and a
network round trip — so an idle page produced one remote query per minute
forever, and a remote that could never answer was retried on that same
cadence for the life of the session.
interaction: "forbid"are spawned detached in theirown process group so a timeout can kill the whole git/ssh tree. That
parent-side timer is the only thing that ever reaps them, and it dies with
the parent — so a server stopped mid-probe orphaned the git/ssh pair.
Reproduction (before)
A repo whose
originis an unreachable SSH URL (git@192.0.2.1:x/y.git), withGIT_SSH_COMMANDpointing at a script that logs each invocation with atimestamp and then hangs.
plannotator reviewfrom source,PLANNOTATOR_BROWSER=/usr/bin/true, then/api/diff/freshpolled every 5s forfour minutes with no other interaction:
Six invocations in 4m08s, three of them purely from the idle poll, continuing
for as long as the page stays open. A separate run with no polling at all
isolates the startup window at three, and a stack-trace run identifies each
call site — including that
detectRemoteDefaultCompareTargetitself runs anls-remote, which is why an opt-out has to skip the whole startup block.Orphan check, with SIGTERM delivered while a probe is still hanging (the
fake ssh sleeps far past the server's own 5s transport timeout, so only an
exit-time reap could explain a dead child): the transport survives the server.
Change, per runtime
Both runtimes change together, as the "Server Runtimes" section requires: the
Bun server in
packages/server/and thenode:httpmirror inapps/pi-extension/server/.Where the remote probe lives. Removed from
/api/diff/fresh; added to/api/diff/switch, which backs the diff-type and base pickers and the "Diffout of date · Refresh" button — the interactions where a reviewer is actually
asking for a fresh answer. Startup, the
/api/diffpage load and the explicit/api/fetch-baseare unchanged. The switch call is fire-and-forget, so ahanging remote can never make a diff switch wait on the network: that response
carries the cached staleness and the next one carries the refreshed value.
baseBehindRemotestill rides every/api/diff/freshresponse from thecached value, so the banner neither flickers nor drops out between refreshes.
Negative caching. A failed probe doubles the interval up to a 15-minute
cap; a success resets it to 60s. The policy is one shared helper
(
nextRemoteBaseCheckIntervalinpackages/shared/review-core.ts) so the tworuntimes cannot drift.
Opt-out.
plannotator review --no-git-remote-check,PLANNOTATOR_GIT_REMOTE_CHECK=0(also
false/disabled), or{ "gitRemoteCheck": false }in~/.plannotator/config.json; precedence flag > env > config, resolved byresolveRemoteCheckinpackages/shared/config.tsbeside its siblingresolvers. With it off the session issues zero
ls-remotecalls, startupprobes included — the startup block is skipped whole, because the compare-target
detection is itself an
ls-remoteand an opt-out that only silenced thestaleness probe would still prompt for authentication as the review opens. The
base stays whatever local ref discovery resolved, the banner never shows, and
POST /api/fetch-basestays reachable on its own predicate, because an explicitFetch is the user asking for the network.
The flag is parsed by the shared
parseReviewArgs, so it works on everyhost that forwards review arguments — Claude Code, OpenCode and Pi — not only
the Bun CLI. The reporter is on OpenCode, so a Bun-only flag would have missed
them.
Orphaned transports. Both runtimes track the process-group leaders of
in-flight isolated commands and SIGKILL each group from a synchronous
process.on("exit")hook, installed lazily on the first such spawn. "exit" isthe right hook: it runs on every
process.exit(), which is where the CLI'sSIGINT/SIGTERM handling already routes. POSIX only, reusing the existing win32
carve-out.
Behavior kept
HTTPS and passwordless-SSH users lose nothing they would notice. The remote is
still queried at startup, on
/api/diff, on every diff-type/base switch and onFetch, still rate limited to once a minute, and
baseBehindRemotestill ridesevery diff payload and every freshness response. The banner, the one-click
Fetch, the base picker and the startup upgrade of a bare
maintoorigin/mainall behave as before. Only plain local git sessions were ever affected; PR, jj,
GitButler, P4, workspace and static-patch sessions never ran the check.
Behavior changed
A push that lands on the base branch mid-review is no longer noticed within
the minute on its own. It appears on the next refresh, diff switch or page
reload. That is the deliberate trade: the old within-the-minute detection cost
a network round trip per minute per open tab, whether or not anyone was looking.
Also release-note-worthy: with the opt-out on, the compare target comes from
local refs only, so a repo whose
origin/HEADis unset may resolve a differentdefault than the remote would have reported.
Tests
All dual-runtime where the code is, and each new guard was confirmed to fail
against the previous behavior before being kept:
packages/server/review-remote-check.test.ts(new, Bun + Pi, 6 tests). Countsls-remoteinvocations with agitshim on PATH that logs and then execs thereal git, against a real local bare remote deliberately left ahead of the
tracking ref so
baseBehindRemoteis genuinely true. Guards: an idle sessionissues nothing beyond the startup probes with the clock advanced past the
interval (at real speed the old rate limit would have hidden the bug —
verified: both runtimes fail this test against the pre-fix handler);
baseBehindRemoterides all twelve freshness responses; a switch inside theinterval stays quiet while a switch past it probes exactly once (fails when
the new call is removed); the opt-out is network-free end to end (fails when
the startup block is left ungated).
packages/server/git-background.test.ts(+1 per runtime): exit kills astill-hanging transport, with a command timeout far longer than the child's
lifetime so only the exit hook can account for the kill. Fails with the hook
disabled.
packages/shared/config.test.ts: a 13-rowresolveRemoteCheckprecedencetable plus the shared config-coercion table.
packages/shared/review-core.test.ts: backoff doubling to the cap, reset onsuccess, and no overshoot.
packages/shared/review-args.test.tsandapps/pi-extension/review-args-parity.test.ts: the flag parses, composes withthe other selectors, is absent when not typed (so it cannot outrank the env
var), and is present in the vendored copy Pi parses through.
Counts, on this branch:
bun test packages/server packages/shared apps/pi-extension— 2305 pass,1 skip, 0 fail, 7569 expect() calls, 152 files.
bun test(full) — 4994 pass, 1172 skip, 0 fail, 46594 expect() calls,534 files.
tsc --noEmitacross all nine project configs — clean.Reproduction (after)
Same fixture, same four-minute idle poll, rebuilt from this branch:
Three invocations, all inside the first five seconds. A diff switch past the
interval triggers exactly one more. SIGTERM during an in-flight probe leaves no
surviving child, where the same run with the exit hook disabled orphans it.