Fix #1306: github capture: an observedRepos.list() failure during a session_repos tick still aborts the whole tick

[philcunliffe]

Feature or issue

When inventory: "session_repos" is configured, runCaptureTick awaited runtime.observedRepos.list() outside any guard, before the per-repo try/catch that exists precisely so one failure cannot kill a tick. A failing local inventory read (an unreadable cache partition, a sidecar rewrite) therefore escaped runCaptureTick into the tick() catch in source.js: the tick recorded github.poll_tick_failed having captured nothing, no per-failure errors entry reached hyp github sync/backfill, and the source's backlogPending was left at whatever the previous tick happened to set. Deferred hardening finding 2 from PR #1302, which fixed only the all_visible half of the same shape.

Solution

• runCaptureTick now guards the observedRepos.list() call: on a throw it logs github.inventory_resolve_failed with the error's error_kind, and resolves with the failure as a single { repo: '(inventory)' } entry in errors instead of throwing, so the tick completes attributably and the CLI reports the real cause.
• pending on that path is read off the durable cursors rather than reported flat false: the failure itself is not backlog (LLP 0360#cadence), but a tick that never resolved its inventory retired none either, and clearing the flag would push a saved continuation back to a full poll interval (LLP 0361#budget).
• Two regression tests in test/plugins/github-observed-repos.test.js, where the existing runCaptureTick + observedRepos cases live (the issue named github-capture.test.js alongside the all_visible case, but that case is in unmerged PR Fix #1298: github capture: a foreign Link during all_visible enumeration aborts the whole tick instead of one repo #1302 and captureRepos never calls observedRepos.list()). Both fail on master with the throw escaping at tick.js:28 and pass after. Full suite 5990 pass / 0 fail; npm run typecheck clean. No lint or build check is defined beyond build:types.

Code: +25 / -5 lines

Fixes #1306

[philcunliffe]

Review record - head 2956fe8a, effort high

Verdict: findings (3). Two fixed and pushed as 069c6d9e; one recorded, not fixed, with the reason below.

The change is well shaped: the throw is contained where the per-repo isolation already lives, the error kind is carried through to hyp github sync/backfill, and both new tests genuinely fail on master. Every finding is in the one line that derives pending on the new early return, plus one CLI consequence of no longer throwing.

1. High - an unfinished revalidation loses its backlog flag. FIXED

hypaware-core/plugins-workspace/github/src/tick.js:47 (reviewed head)

observedRepos.list() runs update(), which runs the bounded revalidation slice. When that slice throws (the unreadable-partition case this PR's own tests model), createLocalObservedReposIndex never reaches its state = next swap (observed-repos.js:123), so revalidationPending() keeps returning true off the prior pass (observed-repos.js:304). The new early return derived pending from the cursors alone, so on any install whose repos completed it returned false.

Consequences, all regressions against master (where the throw escaped and left backlogPending at the true the previous incomplete-revalidation tick set):

• source.js:47 clears backlogPending, so the retry is deferred a full poll_interval (24h default) instead of BACKLOG_RETRY_MS.
• While a revalidation is incomplete, list() returns only re-confirmed repos (observed-repos.js:302), so capture runs against a truncated inventory for that whole day.
• It also contradicts the inventoryPending rule ten lines below (tick.js:55-58, @ref LLP 0367#bounded-revalidation), which says exactly this state rides the backlog cadence.

Fix: fold runtime.observedRepos.revalidationPending?.() === true into the returned pending. Regression test added in test/plugins/github-observed-repos.test.js ("keeps an unfinished revalidation on the backlog cadence"); verified it fails on the pre-fix tree (not ok 10) and passes after.

2. Medium - the cursor scan is not equivalent to captureRepos' pending. NOT FIXED (recorded)

hypaware-core/plugins-workspace/github/src/tick.js:47

Object.values(cursors.repos).some(c => c?.work) diverges from the real computation in both directions:

• Over-reports. Nothing prunes cursors.repos (capture.js only ever writes entries, lines 117/154). A repo that had a saved continuation and then left the inventory keeps its work forever, so a persistently failing inventory read pins a daily source to the 15-minute cadence. Left as is: master reaches the same outcome by preserving the stale true, so this is not a regression, and the correct answer needs the inventory that just failed to resolve.
• Under-reports. Budget exhaustion at a repo boundary (capture.js:110-113, and visited < repos.length at line 158) sets pending = true with no work saved anywhere; the continuation lives in cursors.next_repo. next_repo is not usable as the missing signal - capture.js:155 sets it after every repo including the last, wrapping to repos[0], so it is populated after every completed rotation too. There is no durable marker that distinguishes "stopped mid-rotation" from "finished a rotation", so this case cannot be recovered from state without a new field, which the task did not call for.

Net: the remaining gap is a narrow under-report on the boundary-exhaustion case only, and closing it would need a new durable field. Worth a follow-up decision if the daily source ever proves to stall on it; not landed here.

3. Low - hyp github backfill owner/repo contradicted itself on an inventory failure. FIXED

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

Because the tick now returns repos: 0 instead of throwing, backfill with positional repos fell into the only && result.repos === 0 branch and printed none of [owner/repo] are in the active repository inventory - a false claim about the user's configuration - one line after reportErrors printed the true cause as ! (inventory): .... Introduced by this PR: before it, the throw was caught by the outer handler and only the real message was printed.

Fix: guard that branch on result.errors.length === 0. No test: runGithubBackfill has no existing harness (requireGithubRuntime is a module-level singleton and nothing under test/ exercises the command), and building one is out of scope for a one-condition message guard.

Checks

• npm test: 5991 pass, 0 fail, 1 skipped.
• npm run typecheck: clean.
• No em dashes, no semicolons introduced.

Pushed as 069c6d9e on fix/issue-1306.

[philcunliffe]

Review record - head 004f2571, effort high

Verdict: findings (3 actionable, all fixed and pushed as 4315d409; 1 carried forward unfixed).

Second round. Head 004f2571 is 069c6d9e (the first round's fixes) plus a merge of origin/master; the merge touched only src/core/cli/spinner.js, src/core/commands/sync.js, test/core/sync-command.test.js and .github/workflows/ci.yml, none of which reach the github plugin, so the PR's own diff is byte-identical to what round 1 reviewed.

The core fix remains sound: containing the observedRepos.list() throw where the per-repo isolation already lives is the right shape, the error kind is carried through to hyp github sync/backfill, and the three regression tests genuinely fail on the pre-fix tree (verified: 7 pass / 3 fail before, 10 pass after). Findings this round are one false load-bearing comment, the test comment repeating it, and one CLI line.

1. Low - the failure-path comment's stated reason is false. FIXED

hypaware-core/plugins-workspace/github/src/tick.js:47-49 (reviewed head)

The comment justified reading revalidationPending() on the failure path with: "its bounded slice runs inside this same list(), so a throw leaves the pass still reported as pending". That is not what the index does. update() builds a local next and assigns state = next only at the very end (observed-repos.js:122-123), with writeState on the line before. So when a revalidation is started by the same call that throws (next.revalidation set at observed-repos.js:96, then revalidateSlice throws inside storage.readRowsSince, which is exactly the unreadable-partition case this PR's tests model), next is discarded: neither state nor the sidecar records the pass, and revalidationPending() returns false.

Proven with a standalone probe against the real createLocalObservedReposIndex (pristine build under fp1, then a fingerprint flip to fp2 with a throwing readRowsSince):

pass 2 list() threw: cache partition unreadable
pass 2 revalidationPending(): false   <- comment claims true
sidecar recorded revalidation: false

The claim only holds for a pass a previous tick already persisted. The code is fine either way (that persisted case is real and is what the line usefully catches); only the stated reason was wrong, and it is the kind of comment a later reader would rely on.

Fix: comment corrected to say only a persisted pass counts, and why. No behavior change.

2. Low - the regression test repeated the same false rationale. FIXED

test/plugins/github-observed-repos.test.js:436-438 (reviewed head)

The third new test's comment asserted the same incorrect mechanism, and was self-contradictory about it: "a throw from it leaves the pass unfinished and still reported by revalidationPending() (the index only swaps its state after a completed update())". The parenthetical is precisely the reason the pass is not reported when it started in the throwing call.

Fix: comment corrected to describe the case the test actually covers (an earlier tick's persisted pass, which the failed call leaves untouched). The test itself is kept as is: it correctly pins that runCaptureTick honors revalidationPending() on the failure path, which is round 1's finding 1.

3. Low - hyp github backfill owner/repo advised a next step after capturing nothing. FIXED

hypaware-core/plugins-workspace/github/src/commands.js:62 (reviewed head)

Introduced by this PR. Because the tick now returns repos: 0 instead of throwing, an inventory failure falls past round 1's errors.length === 0 guard and reaches the unconditional advice line, so the run printed:

github backfill: 0 event(s) across 0 repo(s)
  ! (inventory): cache partition unreadable
run 'hyp graph project' to project github_events into the graph

Exit code is correctly 1, so only the advice line is wrong, but it dresses a total failure up as progress. Before this PR the throw was caught by the outer handler and only the real message printed.

Fix: guarded on result.repos > 0 || result.errors.length === 0.

Test: added the first test to exercise runGithubBackfill at all, closing the coverage gap round 1 recorded as out of scope. setGithubRuntime is exported, so the module-level singleton is straightforward to drive; the test reuses the existing failingInventoryRuntime helper and asserts exit 1, the real cause on stderr, no active repository inventory claim, and no next-step advice. Verified it fails on the pre-fix tree on exactly the offending line (not ok 1 ... run 'hyp graph project' ...) and passes after.

4. Medium - the cursor scan over-reports pending. NOT FIXED (carried forward from round 1)

hypaware-core/plugins-workspace/github/src/tick.js:50-52

Independently re-raised this round, with a sharper argument than round 1 recorded. Object.values(cursors.repos).some((cursor) => Boolean(cursor?.work)) scans every cursor ever written, not the current selection, and nothing prunes cursors.repos (capture.js only ever writes entries, lines 117/154). A repo added to ignore[], contracted out of the session inventory, or permanently failing leaves work behind forever, so a persistently failing inventory read returns pending: true on every tick and nextCaptureDelay pins a 24h source at the 15-minute backlog cadence with no progress possible. capture.js:136-142 documents that exact outcome as the thing to avoid, and capture.js:163 scopes the same predicate correctly as repos.filter((repo) => cursors.repos[repo]?.work).

Round 1 recorded this as "not a regression, master reaches the same outcome by preserving the stale true". That is only true when the previous tick set backlogPending = true; where master would have left it false, this PR can newly pin the source to the 15-minute cadence. So it can be a regression, in a narrow case.

Left unfixed deliberately:

• The correct predicate needs the inventory that just failed to resolve, so it cannot be scoped here.
• The complementary under-report (budget exhaustion at a repo boundary sets pending with no work saved; the continuation lives in cursors.next_repo, which capture.js:155 also sets after every completed rotation) cannot be distinguished from state without a new durable field, which the task did not call for and CLAUDE.md discourages inventing.
• The blast radius is bounded and self-healing: a cheap local read retried every 15 minutes instead of every 24 hours, no GitHub traffic (the inventory fails before any client call), no data loss, and it clears the moment the partition becomes readable.
• Narrowing the scan by config.ignore alone would fix only the reviewer's literal example, not the commoner "contracted out of the session inventory" case, while re-opening pending semantics that round 1 had just settled.

Worth a follow-up decision if a daily source is ever observed stalling on it; not landed here.

Observed, not filed

github.inventory_resolve_failed logs mode: 'session_repos' rather than opts.mode, so a log line cannot say whether an inventory failure happened during a poll or a backfill. Not raised as a finding: this matches its paired success event github.inventory_resolved (capture.js:56-57), which also logs mode: inventory, so the new event is consistent with the convention already in place rather than deviating from it.

Checks

• npm test: 5996 pass, 0 fail, 1 skipped (5995/0/1 before the added test).
• npm run typecheck: clean.
• npm run smoke -- github_local_capture: ok.
• node --test test/plugins/github-observed-repos.test.js: 11 pass.
• CI green at the reviewed head (test 22/24, typecheck 22/24, LLP, cross-branch-numbers, duplicate-numbers).
• No em dashes, no semicolons introduced. Cited anchors LLP 0360#cadence and LLP 0361#budget both exist and match the claims made against them.

Fixes verified present in the committed tree and absent at 004f2571, then pushed as 4315d409 on fix/issue-1306.

[philcunliffe]

Triage at head 4315d409: review rounds exhausted with one residual finding, classified non-blocking and deferred to an issue.

• Finding 1 (medium, round 2 finding 4): the failure-path pending in tick.js:50-52 scans every cursor ever written, so a stale work entry plus a persistently failing inventory pins a 24h source at the 15-minute retry cadence -> github capture: failure-path pending scans every cursor ever written, pinning a failing 24h source at the 15-minute backlog cadence #1316

Worst case verified as a bounded, self-healing local retry-cadence inefficiency: the tick aborts before any GitHub call, nothing is written or lost, and it clears on the first successful inventory read. All three round-2 fixes were verified present at this head.

[philcunliffe]

Ship risk: low

Who could be affected: Only people who deliberately turned on GitHub activity capture. It stays off unless you switch it on, confirmed on the real startup path rather than from a list.

What could happen:

• If the local store this feature reads becomes unreadable, background checking can repeat every 15 minutes instead of once a day until the store recovers. It costs a local read, contacts GitHub not at all, and stops on its own.
• A manual run now prints the real reason it failed instead of a wrong claim about your settings, and no longer suggests a next step after capturing nothing.

Why this level: Nothing is written, changed, or lost when this path runs, and nothing new is read or sent. The only noticeable effect is slightly more frequent background retrying while a local fault lasts, on a feature the person opted into. Access, data, and privacy are untouched, and it clears itself.

What was checked: A test drove the real code through the failure and confirmed it contacts GitHub zero times, records nothing, leaves saved progress byte-for-byte unchanged, never reports a failed run as a success, and cannot widen what is collected. That test fails when any of it stops being true. The project's tests and all automated checks pass on this exact version.