Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo

[philcunliffe]

Feature or issue

GitHub capture is built so one bad repository never kills a whole tick, but captureRepos resolved its inventory outside that per-repo try/catch. With inventory = "all_visible", enumeration reaches the network through client.listViewerRepos(), and since the origin pin landed a foreign Link header there throws github_foreign_origin. The throw escaped captureRepos entirely, so no repository was captured that tick and it surfaced only as a whole-tick github.poll_tick_failed.

Solution

• Wrap the resolveRepos call in captureRepos, record the failure in the same errors array per-repo failures use, and log github.inventory_resolve_failed with the inventory mode and error_kind; the tick returns normally with zero repos and pending: false, matching LLP 0360#cadence (a failure retries on the ordinary cadence, it is not bounded backlog).
• The duplicated hypErrorKind extraction became one errKind(err) helper, shared with the per-repo error log.
• Regression test in test/plugins/github-capture.test.js: enumeration throws github_foreign_origin, and the tick completes with the error recorded and attributable. It fails on master and passes here; full npm test (5987 pass) and npm run typecheck are green.

Code: +34 / -5 lines

Fixes #1298

[philcunliffe]

Review round 1 - head 374fda80

Verdict: two findings (one MEDIUM, one LOW), both fixed and pushed. New head 99284fcd.

The change does what issue #1298 asks: resolveRepos now runs inside captureRepos's failure handling, an enumeration throw becomes one recorded error instead of a dead tick, and the failure stays attributable via error_kind on a dedicated github.inventory_resolve_failed log line. Both findings are in the blast radius of that new early-return path.

Verification performed

• The regression test genuinely fails without the production change. I reverted capture.js to origin/master in my worktree and re-ran test/plugins/github-capture.test.js: not ok 23 - a failed all_visible enumeration is recorded, not thrown out of the tick, 22 pass / 1 fail. Restored: 23/23. The test is not self-agreeing.
• npm test: 5988 pass, 0 fail. npm run typecheck: clean. Both re-run after each fix.
• No em dashes, no semicolons, no new dependencies.
• (Note for anyone reproducing: npm ci fails in a fresh worktree of this repo and its exit code is easily masked by a pipe. npm install works. Without it ~305 files fail with bogus ERR_MODULE_NOT_FOUND.)

---

Finding 1 (MEDIUM, fixed) - the failure path retires backlog it never touched

hypaware-core/plugins-workspace/github/src/capture.js:107 (at 374fda80)

return { repos: 0, events: 0, requests: 0, pending: false, errors }

Returning false here is not merely declining to claim backlog: it clears backlog that is still saved on disk.

On master, an enumeration throw propagated past source.js:47 (backlogPending = result.pending) into the catch at source.js:59, which never touches backlogPending - so the flag survived a failed tick. Now the tick takes the success path and assigns false. tick.js:35-38 cannot rescue it: inventoryPending is ORed in only when inventory === 'session_repos', and this path is reached from all_visible.

Concrete regression, inventory: all_visible, default poll_interval: 24h:

1. Tick N exhausts the 400-request budget (capture.js:172) with repos still holding cursor.work. pending is true, so nextCaptureDelay (source.js:147) schedules the retry at BACKLOG_RETRY_MS = 15 min.
2. Tick N+1's listViewerRepos() fails transiently - a secondary rate limit, a network blip, or the github_foreign_origin case this PR is about.
3. Capture returns pending: false, backlogPending is cleared, and the source schedules the next attempt 24 hours out. The saved continuations sit untouched for a day. On master they retried in 15 minutes.

This inverts LLP 0361#budget's purpose: the continuation exists so bounded work resumes promptly, and here a one-off network error postpones it by a full interval.

Fix (99284fcd): the tick captured nothing, so the honest answer is already on disk. Derive it - hasSavedWork(cursors), some((cursor) => Boolean(cursor?.work)), the same set github.capture_budget_exhausted counts as pending_repos at capture.js:177. The PR's own principle is preserved exactly: an error alone is still not backlog, and the existing fresh-cursor test still asserts pending === false.

New test a failed enumeration does not retire backlog the cursors still hold (test/plugins/github-capture.test.js:657). Verified it fails against capture.js at ba20fe04 (not ok 24, 23 pass / 1 fail) and passes with the fix.

Credit: this one is the code-review pass's find, not mine - I had cleared pending: false on the "an error is not backlog" reasoning without checking what the source did with the flag on the old throwing path.

Finding 2 (LOW, fixed) - backfill prints a false diagnosis under the real one

hypaware-core/plugins-workspace/github/src/commands.js:54

if (only && result.repos === 0) {
  ctx.stderr.write(`hyp github backfill: none of [${only.join(', ')}] are in the active repository inventory\n`)

Before this PR the branch was unreachable for an enumeration failure: the throw exited through the outer catch at commands.js:62. Now such a failure returns repos: 0 with a recorded error, so hyp github backfill owner/repo behind a foreign-origin proxy prints:

github backfill: 0 event(s) across 0 repo(s)
  ! (inventory): GitHub continuation URL refused: it does not address the configured API base ...
hyp github backfill: none of [owner/repo] are in the active repository inventory

The last line asserts something false and sends the reader to their ignore[]/inventory config instead of the proxy they were just shown. The exit code was 1 either way, so this is diagnosis quality only.

Fix (ba20fe04): guard the branch with result.errors.length === 0. Exact rather than approximate: repos === 0 means the filtered set was empty, so the per-repo loop never ran and a non-empty errors can only be the inventory failure. The real error line and exit 1 are unchanged.

---

Checked and correct

• Attributability is preserved, not lost. The tick no longer reaches github.poll_tick_failed (source.js:60-68), which carried error_kind. The replacement github.inventory_resolve_failed is error-level and carries the same kind; source.js:49 still lifts the message into lastError for hyp status, and lastSuccessAt is correctly left unadvanced (source.js:50). LLP 0360#cadence's "status and structured logs expose ... error kind" still holds.
• No cursor damage. The early return precedes rotateTo, so cursors.next_repo is untouched and the round-robin resumes where it was. writeCursors still runs via tick.js's finally.
• requests: 0 under-reports HTTP calls listViewerRepos may have made before throwing, but that is pre-existing: enumeration requests were never charged to the budget on the success path either (budget is built after resolveRepos on master too). Not a regression; not worth widening the change.
• errKind extraction is a true dedup of two identical hypErrorKind reads and is behavior-preserving for null, primitive, and kind-less errors.
• let repos assigned only inside try with a returning catch narrows correctly under the repo's --strict --checkJs.
• errors[].repo = '(inventory)' is a sentinel, not a slug, but the only consumers are reportErrors (prints it) and source.js (reads .error and .length), so nothing parses it as owner/repo and it cannot collide with a real key.
• No existing test asserted the old throwing behavior.

Observation, not a finding (no change requested)

tick.js:26-30 awaits runtime.observedRepos.list() outside captureRepos, so for the default session_repos inventory a failure there still aborts the whole tick in exactly the shape issue #1298 describes. It is a local read rather than a network call, and #1298's acceptance condition is explicitly scoped to listViewerRepos(), so widening this PR would be out of scope. Noting it so the next reader does not mistake it for something this PR was meant to cover.

[philcunliffe]

Review round 2 (final) - head 99284fcd

Verdict: ship-ready. Round 1's two fixes are correct; the second one was incomplete and is now finished. One LOW finding fixed and pushed (new head cdf51d2a), one MEDIUM weighed and deliberately accepted with the trade-off written into the code.

Round 1's fixes, re-checked at this head

Finding 1 (hasSavedWork) - correct. I traced every case the reviewer asked about:

state	cursors.repos	hasSavedWork	right answer?
first-ever run	{} from readCursors's fallback (cursors.js:43), and repos is a non-optional Record in CursorState (types.d.ts:248), so Object.values cannot throw inside the catch	false	yes: nothing on disk, nothing to lose
exhausted but complete	capture.js:310 delete cursor.work on the comments-phase boundary, so a drained repo keeps only since/etag/pull_numbers	false	yes
partially consumed	a repo interrupted mid-phase keeps work (set at capture.js:210, persisted by tick.js's finally)	true	yes: this is the continuation the 15-minute cadence exists for
cursor entry created but untouched	capture.js:143 seeds {} before capture, work absent	false	yes, no false positive from the seed

The flag is read before any mutation on that path (the early return precedes rotateTo), so it reports pre-tick durable state, which is exactly the quantity a tick that captured nothing should be reporting.

Where it does not agree with pending_repos. The JSDoc claimed it is "the same set the budget-exhausted log line counts as pending_repos" (capture.js:735 vs capture.js:187). That claim is false in both directions, and the divergence is the substance of the MEDIUM below. I rewrote the doc rather than the code; see the reasoning there.

Finding 2 (commands.js guard) - correct, and the condition is exact. result.repos === 0 means the filtered selection was empty, so the per-repo loop never ran and a non-empty errors can only be the inventory failure. It was, however, incomplete: see LOW below.

Finding 3 (LOW, fixed in cdf51d2a) - the same false diagnosis, one line further down

hypaware-core/plugins-workspace/github/src/commands.js:64

Round 1 stopped the "none of [...] are in the active repository inventory" line, but the same run still falls through to the success tail:

github backfill: 0 event(s) across 0 repo(s)
  ! (inventory): GitHub continuation URL refused: it does not address the configured API base ...
run 'hyp graph project' to project github_events into the graph

Before this PR a bad token or a foreign-origin proxy threw out of captureRepos and produced one clear stderr line at commands.js:67. Now the reader is handed a next step that projects a table this run never touched. Exit code was already 1, so this is diagnosis quality, same class as round 1's finding 2 and reachable by the same input, which is why I finished it rather than leaving it.

Fixed with the same exact condition:

if (result.repos === 0 && result.errors.length > 0) return 1

New tests, and they are load-bearing. test/plugins/github-commands.test.js is the first test coverage the github commands have had at all (round 1's commands.js fix landed untested, which is how this one survived). Two cases: a failed enumeration prints the real cause and neither bad line; a genuinely absent repository still gets the inventory diagnosis, so the guards cannot silently over-suppress. Verified not ok 1 / ok 2 against commands.js at both origin/master and 99284fcd, and ok 1 / ok 2 with the fix.

Finding 4 (MEDIUM, accepted, not fixed) - hasSavedWork also counts failure residue

hypaware-core/plugins-workspace/github/src/capture.js:114

hasSavedWork counts any cursor holding work, which includes work a repository left behind by failing. The per-repo catch at capture.js:162 deliberately refuses to count that ("treating an error as pending pins a daily source at the 15-minute backlog cadence for as long as one repository keeps failing", LLP 0360#cadence). So: a token that lost scope for one private repo throws every tick and never clears its work, and while all_visible enumeration is also failing, nextCaptureDelay clamps a 24h source to BACKLOG_RETRY_MS for as long as both persist. It is symmetric in the other direction too: a rotation that exhausted its budget exactly at a repository boundary leaves un-visited repos with no work at all (capture.js:182 sets pending from visited < repos.length, which no cursor records), so hasSavedWork can under-report real backlog.

I am not fixing it, deliberately, and this is the round's judgment call:

• An exact answer is not on disk. Budget residue and failure residue are the same bytes. Distinguishing them means a new cursor field, which CLAUDE.md rules out for a bug fix ("do not invent columns, config keys, or schema fields"). The alternative, a pending: undefined "this tick learned nothing" contract so source.js keeps its previous flag (which is exactly what master's throw path did), ripples through the shared return type in tick.js, commands.js, and source.js at the final review round of a +34/-5 fix.
• Both residual errors are bounded and non-destructive. The worst case is a mis-timed retry: one extra failing listViewerRepos call per 15 minutes against an already-broken endpoint, or one delayed backlog resume. Neither loses rows or corrupts a cursor, and both self-heal the moment enumeration succeeds, because the success path recomputes pending from the real signal.
• It is still strictly better than the alternative it replaced. A flat false discards a real continuation; this at worst mis-times one.
• The pathological combination also needs enumeration to be permanently broken, which is the condition hyp status surfaces via lastError anyway.

What I did change is the claim: capture.js's JSDoc now says the cursors are the only evidence this path has and are approximate on both sides, instead of asserting an equivalence with pending_repos that does not hold. A future reader meets the trade-off where they meet the code.

Also checked, no action

• tick.js:28 still awaits runtime.observedRepos.list() outside any guard, so the default session_repos inventory keeps the whole-tick failure shape github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1298 describes. Unchanged from round 1's read of it: it is a local cache read, not a network call, and github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1298's acceptance condition names listViewerRepos() specifically. Out of scope, recorded so the next reader does not mistake it for covered.
• Exit codes are unchanged by both guards: an inventory failure was exit 1 through the old catch and is exit 1 now, via errors.length > 0. runGithubSync has no next-step line and needed no equivalent.
• errKind remains a faithful extraction (null, primitive, and kind-less errors all yield {}).
• No cursor damage: the early return precedes rotateTo, next_repo is untouched, writeCursors still runs in tick.js's finally.
• npm test 5990 pass / 0 fail (was 5988 before my two tests), npm run typecheck clean, both re-run after the fix. No em dashes, no semicolons, no new dependencies.
• Independent code-review pass at high effort raised exactly the three items above (MEDIUM hasSavedWork residue, LOW next-step line, tick.js observation) and found no null-deref, missing await, or off-by-one. It surfaced no fourth issue.

[philcunliffe]

Triage at head cdf51d2a

Two review rounds left two residual findings. Both re-verified at this head (worktree at cdf51d2a, the touched test files re-run: 26/26 pass; git diff --stat origin/master...HEAD confirms tick.js is untouched). Neither is a production blocker; each is deferred to its own issue:

• Finding 1 (MEDIUM, accepted in round 2): hasSavedWork (capture.js:745, used at :114) approximates backlog in both directions after a failed enumeration - it counts failure residue the per-repo catch refuses to call backlog, and misses budget-boundary rotations no cursor records. Worst case is a mis-timed retry (one extra failing request per 15 min, or one resume delayed up to a poll interval), never lost rows, self-healing on the next successful enumeration, and strictly better than the flat false it replaced. -> github capture: hasSavedWork counts failure residue as backlog and misses budget-boundary rotations, mis-timing the retry cadence after a failed enumeration #1305
• Finding 2 (LOW, noted out-of-scope in both rounds): tick.js:26-30 resolves the default session_repos inventory via observedRepos.list() outside captureRepos's guard, so a failure there still aborts the whole tick in the shape github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1298 described. Pre-existing on master (this PR does not touch tick.js), local cache read rather than a network call, attributable via github.poll_tick_failed and lastError, and backlogPending survives the catch. -> github capture: an observedRepos.list() failure during a session_repos tick still aborts the whole tick #1306

PR can merge safely.

[philcunliffe]

Ship risk: low

Who could be affected: Only people who have turned GitHub capture on and set it to watch every repository they can see. It is off by default, and the default setting watches only repositories your own sessions touched, which this change does not reach.

What could happen:

• Today, one bad response while listing repositories stops that whole round of GitHub capture. After this change it is recorded as a single failure, so people should see fewer silently empty rounds.
• Rarely (a round that stopped at its request limit, immediately followed by a failed listing) the next attempt could wait a full polling interval instead of fifteen minutes. Nothing is lost: the saved position is untouched and the same activity arrives on the next attempt, just later.

Why this level: Nobody loses data, access, or privacy. The affected setting is opt-in, and the only downside is GitHub activity showing up later than it would have.

What was checked: The new code was run side by side with the current version across twelve capture situations and nine command situations, and behaved identically everywhere the listing succeeds. The full test suite (5990 tests) and type checks pass.

[philcunliffe]

What neutral was doing

Rung enqueue on PR #1302 (fix/issue-1298, head cdf51d2a5517e3214cf2e60cd8a41bcc95a742c4).
The head is mergeable, green, reviewed clean, and carries a ship-risk record of low e4 v1,
which is within this repository's configured maxAutomerge: low, so neutral enqueued it into
GitHub's merge queue at position 1 (22:27:37 UTC). GitHub removed it again at 22:33:22 UTC
with reason failed_checks.

Why it cannot proceed

The merge queue is evicting PRs for failed_checks, but the checks did not fail on
their merits: the merge-group jobs are hitting the timeout-minutes: 5 cap in
.github/workflows/ci.yml, and GitHub reports a timed-out job as cancelled, which
fails the CI required gate job, which makes the queue evict the PR.

Evidence, two consecutive merge groups, both evicted:

merge group	job	duration	result
48e3ae71 (run 33813167719)	test (22)	5m16s	cancelled at the 5m cap
48e3ae71	test (24)	5m15s	cancelled at the 5m cap
561e0644 (run 33813613314)	test (22)	5m11s	cancelled at the 5m cap
561e0644	test (24)	5m09s	cancelled at the 5m cap
561e0644	typecheck (24)	5m11s	cancelled at the 5m cap

For comparison, the same jobs on ordinary pull_request runs of these very heads:

run	job	duration	result
33809390061 (cdf51d2a, PR 1302)	test (22)	1m21s	success
33808386811 (98c9a81a, PR 1300)	test	~1m30s	success
33811297276 (dd769988, PR 1309)	test (24)	2m16s	success
33809855762 (merge group 57438f44)	test (22)	1m13s	success

So the suite normally finishes in 1 to 2 minutes and merge groups succeeded as recently
as 57438f44 at 21:47. In these two groups every job ran roughly 3 to 4 times slower
(typecheck (22) alone took 3m00s against its usual well under a minute), which points
at the runner or npm i, not at the code under test. No test assertion failed: the logs
show only CI required: Process completed with exit code 1 after the jobs were killed.

This is repo-wide infrastructure, not a defect in this pull request. The change itself
is mergeable, green on its own head, reviewed clean, and assessed at ship risk low.

What it needs from you

A call on the merge-queue CI budget. The options, as neutral sees them:

1. Raise timeout-minutes for the test and typecheck jobs in
   .github/workflows/ci.yml (5 minutes leaves almost no headroom over a 1 to 2 minute
   suite, so any runner slowdown evicts every queued PR). This is the smallest change
   that unblocks landing.
2. Investigate why merge-group runs are 3 to 4 times slower than pull_request runs
   for the same tree, if the slowdown is itself the bug worth fixing.
3. Something else, for example landing these by hand this once.

Neutral has deliberately not re-enqueued anything: with the cap unchanged, every
enqueue is evicted the same way, so retrying would just loop and burn CI minutes.

How to unstick

Reply with a comment on this PR (or push to the branch); neutral monitors this thread and
will re-engage with your guidance on its next tick. If you fix the workflow timeout, say
so here and neutral will re-enqueue this PR.