Checks tab still visible on zero-check repo after #505 (summary 401 fails open / possible deploy lag)

crueber commented

2026-09-14 12:17:19 +00:00

Owner

What's requested

The #505 fix (merged in 694ff51, PR #510) does not hide the Checks tab on a zero-check repo on the live instance — the tab is still visible, and the page's summary request is 401. Diagnose and fix the deployed gap; static diagnosis from code reading + a live rendered-DOM probe (Playwright). No local repro of the app, no code changes on the filing side.

Evidence (live probe, 2026-09-14 ~12:15 UTC)

Rendered page https://hub.packden.us/crueber/walhub (anonymous session) shows the tab strip:

Code · Commits · Issues · Pulls · Checks · Releases · Settings

Checks is visible while the same page's GET /crueber/walhub/api (the summary endpoint) returns 401 authentication required for the anonymous caller, along with resolve, watch, social, tasks (all 401).

Root-cause analysis (static, current tree)

The #505 mechanism is complete in the tree end-to-end:

Server: internal/api/summary.go — has_checks rides the summary body (summaryBody.HasChecks, line ~51, populated at line ~176 from the Env.ChecksSummary hook); the ETag covers the index version (~k suffix, line ~231) so the first report busts the cache.
Wiring: cmd/walhub/collab.go ~202 sets apiEnv.ChecksSummary from the checks service's HasChecks probe (internal/checks/service.go ~539) — absent index → ok=false → has_checks:false.
Client: web/src/lib/tabs.js showChecksTab(summary) (line ~80) fails OPEN: if (!summary) return true; return summary.has_checks !== false;

The failure is at the boundary between the two: when the summary request fails (401 here), getSummary() is null/undefined, showChecksTab fails open and the tab renders. So every anonymous render of an auth-gated repo (hub.packden.us runs OIDC with anonymous read off at times) shows the Checks tab regardless of has_checks — the #505 gate never gets a summary to gate on. This matches the fail-open design comment in tabs.js (loading/unknown keeps the tab), but on this instance "unknown" is the steady state for anonymous visitors, so the tab is permanently visible there even with zero checks.

Secondary possibilities to rule in/out at fix time:

Deploy lag: main merged the fix ~10h before this probe. If the running binary predates 694ff51, an older SPA bundle serves the unconditional TABS render. Verify the deployed binary/SPA actually contains the #505 build before touching anything else.
Cache: the summary is the mutable-collab class (private, no-cache) with the ~k ETag suffix covering the checks index version — no stale-serve window that would keep has_checks:false after a report, so cache is NOT the mechanism for "tab visible on zero-check repo."

Acceptance criteria

Confirm which mechanism it is (deploy lag vs 401 fail-open) before changing code.
If deploy lag: redeploy and close this with a verification note (tab hidden on the live zero-check view).
If the 401 fail-open: decide and implement the policy for summary-unavailable renders — e.g. the SPA distinguishing "summary loading/absent" from "summary request failed 401" and hiding the Checks tab on an explicit auth failure, or documenting that the tab intentionally stays for gated renders (the fail-open is deliberate for flicker/old-server reasons per the tabs.js comment — if kept, note it as designed and close).
Either way: the hidden state keeps the /api#checks-ci reporting-API pointer in the repo-meta line (Repo.jsx ~731) so external CI authors can still discover the reporting API.
Deep links to /:owner/:name/checks still render the empty state when the tab is hidden (no regression on #505's route guarantee).

## What's requested The #505 fix (merged in 694ff51, PR #510) does not hide the Checks tab on a zero-check repo on the live instance — the tab is still visible, and the page's summary request is 401. Diagnose and fix the deployed gap; static diagnosis from code reading + a live rendered-DOM probe (Playwright). **No local repro of the app, no code changes on the filing side.** ## Evidence (live probe, 2026-09-14 ~12:15 UTC) Rendered page `https://hub.packden.us/crueber/walhub` (anonymous session) shows the tab strip: ``` Code · Commits · Issues · Pulls · Checks · Releases · Settings ``` `Checks` is visible while the same page's `GET /crueber/walhub/api` (the summary endpoint) returns **401 authentication required** for the anonymous caller, along with `resolve`, `watch`, `social`, `tasks` (all 401). ## Root-cause analysis (static, current tree) The #505 mechanism is complete in the tree end-to-end: - Server: `internal/api/summary.go` — `has_checks` rides the summary body (`summaryBody.HasChecks`, line ~51, populated at line ~176 from the `Env.ChecksSummary` hook); the ETag covers the index version (`~k` suffix, line ~231) so the first report busts the cache. - Wiring: `cmd/walhub/collab.go` ~202 sets `apiEnv.ChecksSummary` from the checks service's `HasChecks` probe (`internal/checks/service.go` ~539) — absent index → `ok=false` → `has_checks:false`. - Client: `web/src/lib/tabs.js` `showChecksTab(summary)` (line ~80) fails OPEN: `if (!summary) return true; return summary.has_checks !== false;` The failure is at the boundary between the two: **when the summary request fails (401 here), `getSummary()` is null/undefined, `showChecksTab` fails open and the tab renders.** So every anonymous render of an auth-gated repo (hub.packden.us runs OIDC with anonymous read off at times) shows the Checks tab regardless of `has_checks` — the #505 gate never gets a summary to gate on. This matches the fail-open design comment in `tabs.js` (loading/unknown keeps the tab), but on this instance "unknown" is the *steady state* for anonymous visitors, so the tab is permanently visible there even with zero checks. Secondary possibilities to rule in/out at fix time: 1. **Deploy lag**: main merged the fix ~10h before this probe. If the running binary predates 694ff51, an older SPA bundle serves the unconditional `TABS` render. Verify the deployed binary/SPA actually contains the #505 build before touching anything else. 2. **Cache**: the summary is the mutable-collab class (`private, no-cache`) with the `~k` ETag suffix covering the checks index version — no stale-serve window that would keep `has_checks:false` after a report, so cache is NOT the mechanism for "tab visible on zero-check repo." ## Acceptance criteria - [ ] Confirm which mechanism it is (deploy lag vs 401 fail-open) before changing code. - [ ] If deploy lag: redeploy and close this with a verification note (tab hidden on the live zero-check view). - [ ] If the 401 fail-open: decide and implement the policy for summary-unavailable renders — e.g. the SPA distinguishing "summary loading/absent" from "summary request failed 401" and hiding the Checks tab on an explicit auth failure, or documenting that the tab intentionally stays for gated renders (the fail-open is deliberate for flicker/old-server reasons per the `tabs.js` comment — if kept, note it as designed and close). - [ ] Either way: the hidden state keeps the `/api#checks-ci` reporting-API pointer in the repo-meta line (`Repo.jsx` ~731) so external CI authors can still discover the reporting API. - [ ] Deep links to `/:owner/:name/checks` still render the empty state when the tab is hidden (no regression on #505's route guarantee).

crueber added the

issues

label

2026-09-14 12:17:31 +00:00

crueber added this to the v1 milestone

2026-09-14 12:17:32 +00:00

crueber referenced this issue from a commit

2026-09-14 13:25:58 +00:00

Fix #513: hide Checks tab on explicit summary denial (12_web_ui.md decision)

crueber referenced this issue from a pull request that will close it,

2026-09-14 13:26:00 +00:00

Fix #513: checks tab on auth failure #515

crueber commented

2026-09-14 13:26:04 +00:00

Author

Owner

Diagnosed + fixed in PR #515 (fix/issue-513, unmerged). Verdict: deploy lag RULED OUT — the live bundle (index-NJ0Ao3tk.js) already carries the #505 has_checks gate + reporting-API pointer; the anonymous summary 401 on hub.packden.us is correct server behavior (gated repo), so 401-fail-open was the whole story: getSummary() never settles → showChecksTab(undefined)=true forever. Fix per decided policy: shell records explicit 401 in summaryDenied, tab hides on denial, loading stays fail-open. Tests 1154/1153/1 (1 pre-existing smoke fail, head-to-head verified); vite+esbuild green.

crueber commented

2026-09-14 13:43:50 +00:00

Author

Owner

Review of PR #515 (fix/issue-513) — verified in scratch worktree /tmp/pr515 (removed afterward). No browser (per task rules; node tests + reasoning only — noted explicitly).

VERDICT: ready to merge (no fixes pushed — none needed).

(1) Diagnosis: SOUND. The 401-fail-open mechanism is confirmed straight from the tree: pre-fix tabs.js if (!summary) return true + a 401-denied shared summary that never settles = tab pinned visible forever. Deploy-lag ruled out per author's live-bundle evidence (bundle carries the #505 gate + pointer, container recreated post-merge); the code mechanism alone fully explains the symptom, so the fix is correct either way. 401 is correct server behavior (gated repo, OIDC anonymous-read off) — client-side policy fix is the right layer.

(2) Denied-signal wiring (Repo.jsx:613-623,851): CORRECT. Explicit 401 recorded via SDK err.unauthorized (errors.js:30, status===401 — the documented marker); 404→null via tolerateMissing untouched (data.js:77-82 — 404 resolves before the outer catch ever sees it); all other errors rethrow into the tray path. Resolves undefined (still "loading", never "not found" — 401≠deletion) and spares tray spam. Flag resets at fetch start (line 615), so repo switches (new repo:{full} key → new entry → refetch) never inherit stale denial; sign-in is an OIDC redirect/reload (fresh signals), worst case bounded by next fetch.

(3) Helper contract (tabs.js:84-88): CORRECT. opts.denied hides; non-denied keeps the exact #505 rule; opts={} default keeps every single-arg call backward compatible (verified: checksHidden memo :661, meta pointer :750, all old callers behave byte-identically). Loading fail-open preserved (no strip flicker, old servers not stranded).

(4) Untouched: CONFIRMED via diff. TABS model (:169), activeTab segments, tabBadge wiring (:851 tabBadge(getSummary(), t.id)), <For each={TABS}> render, checks route/empty state, /api#checks-ci meta pointer (:750-752 — still gated on the loaded hidden state, exactly per criterion 4's #505 scope). Note: denied viewers see the "loading…" header (getSummary() stays undefined, :684) with no pointer — expected, there is no summary to anchor it to; doc states "renders only on a loaded hidden state". checksHidden (:661) stays false on denial → no extra ["check"] stream (nothing readable to invalidate) — correct.

(5) Authenticated flows: UNAFFECTED. Flag defaults false; 200/404 paths byte-identical; only the 401 path changes (previously: tray spam + stuck "loading…" + wrongly visible tab; now: silent + stuck "loading…" + hidden tab).

(6) Scope/docs: zero .go files, no SDK/API change, no new deps (law 1 holds), 12_web_ui.md decision appended in-commit (law 12 holds).

TESTS (scratch worktree, node_modules symlinked from main): full node --test web/test/unit/*.test.js → 1153 pass, 1 fail = smoke.test.js /: /setup status — ENVIRONMENTAL, not PR-caused: a live server is up on :8080 here (healthz 200) with setup locked (/setup → 403); the smoke up-gate passes on healthz then asserts /setup 200. A frontend-only diff (0 .go files) cannot change an HTTP status. Focused: checks-tab-513 (4 new) + checks-tab-505 files → 9/9 pass incl. the 12-state denial×summary matrix. vite build + esbuild green (unauthorized gate present in bundle; summaryDenied minified away as expected). pnpm/mise shim unavailable in this shell so vite/esbuild were run directly from web/node_modules/.bin — same scripts as make web.

Acceptance criteria mapping: mechanism confirmed before code change ✓ / 401-fail-open policy implemented (hide on explicit denial, loading fail-open) ✓ / pointer kept on loaded-hidden state ✓ / deep-link empty state untouched ✓.

Review of PR #515 (fix/issue-513) — verified in scratch worktree /tmp/pr515 (removed afterward). No browser (per task rules; node tests + reasoning only — noted explicitly). VERDICT: ready to merge (no fixes pushed — none needed). (1) Diagnosis: SOUND. The 401-fail-open mechanism is confirmed straight from the tree: pre-fix tabs.js `if (!summary) return true` + a 401-denied shared summary that never settles = tab pinned visible forever. Deploy-lag ruled out per author's live-bundle evidence (bundle carries the #505 gate + pointer, container recreated post-merge); the code mechanism alone fully explains the symptom, so the fix is correct either way. 401 is correct server behavior (gated repo, OIDC anonymous-read off) — client-side policy fix is the right layer. (2) Denied-signal wiring (Repo.jsx:613-623,851): CORRECT. Explicit 401 recorded via SDK `err.unauthorized` (errors.js:30, status===401 — the documented marker); 404→null via tolerateMissing untouched (data.js:77-82 — 404 resolves before the outer catch ever sees it); all other errors rethrow into the tray path. Resolves undefined (still "loading", never "not found" — 401≠deletion) and spares tray spam. Flag resets at fetch start (line 615), so repo switches (new `repo:{full}` key → new entry → refetch) never inherit stale denial; sign-in is an OIDC redirect/reload (fresh signals), worst case bounded by next fetch. (3) Helper contract (tabs.js:84-88): CORRECT. `opts.denied` hides; non-denied keeps the exact #505 rule; `opts={}` default keeps every single-arg call backward compatible (verified: checksHidden memo :661, meta pointer :750, all old callers behave byte-identically). Loading fail-open preserved (no strip flicker, old servers not stranded). (4) Untouched: CONFIRMED via diff. TABS model (:169), `activeTab` segments, `tabBadge` wiring (:851 `tabBadge(getSummary(), t.id)`), `<For each={TABS}>` render, checks route/empty state, `/api#checks-ci` meta pointer (:750-752 — still gated on the *loaded* hidden state, exactly per criterion 4's #505 scope). Note: denied viewers see the "loading…" header (getSummary() stays undefined, :684) with no pointer — expected, there is no summary to anchor it to; doc states "renders only on a loaded hidden state". checksHidden (:661) stays false on denial → no extra `["check"]` stream (nothing readable to invalidate) — correct. (5) Authenticated flows: UNAFFECTED. Flag defaults false; 200/404 paths byte-identical; only the 401 path changes (previously: tray spam + stuck "loading…" + wrongly visible tab; now: silent + stuck "loading…" + hidden tab). (6) Scope/docs: zero .go files, no SDK/API change, no new deps (law 1 holds), 12_web_ui.md decision appended in-commit (law 12 holds). TESTS (scratch worktree, node_modules symlinked from main): full `node --test web/test/unit/*.test.js` → 1153 pass, 1 fail = smoke.test.js `/: /setup status` — ENVIRONMENTAL, not PR-caused: a live server is up on :8080 here (healthz 200) with setup locked (/setup → 403); the smoke `up`-gate passes on healthz then asserts /setup 200. A frontend-only diff (0 .go files) cannot change an HTTP status. Focused: checks-tab-513 (4 new) + checks-tab-505 files → 9/9 pass incl. the 12-state denial×summary matrix. vite build + esbuild green (`unauthorized` gate present in bundle; `summaryDenied` minified away as expected). pnpm/mise shim unavailable in this shell so vite/esbuild were run directly from web/node_modules/.bin — same scripts as `make web`. Acceptance criteria mapping: mechanism confirmed before code change ✓ / 401-fail-open policy implemented (hide on explicit denial, loading fail-open) ✓ / pointer kept on loaded-hidden state ✓ / deep-link empty state untouched ✓.

crueber closed this issue

2026-09-14 13:44:41 +00:00

crueber referenced this issue from a commit

2026-09-14 13:44:42 +00:00

Merge pull request 'Fix #513: checks tab on auth failure' (#515) from fix/issue-513 into main

crueber commented

2026-09-14 13:44:42 +00:00

Author

Owner

Fixed by PR #515 (review clean — diagnosis confirmed, denied-signal correct, loading fail-open preserved, authenticated unaffected), merged. Closing.

crueber commented

2026-09-14 13:58:39 +00:00

Author

Owner

Reopening: still reproduces. Root cause found in-code (no fix yet): the ~k ETag suffix is conditional on checksOK, so a zero-check repo's ETag is byte-identical to its pre-#505 ETag — any client holding a pre-#505 cached summary 304-matches forever and never receives has_checks, and the helper fail-opens on the missing field. Fix: always-present ~k suffix. Repro: prime cache with a field-less body + old ETag, revalidate after #505 → 304 (wrong), must become 200.

crueber reopened this issue

2026-09-14 13:58:40 +00:00

crueber referenced this issue from a commit

2026-09-14 14:18:25 +00:00

Fix #513 (reopened): unconditional ~k ETag suffix so pre-#505 cached summaries can never 304-match

crueber referenced this issue from a pull request that will reopen it,

2026-09-14 14:18:32 +00:00

Fix #513 (reopened): unconditional ~k suffix #516

crueber commented

2026-09-14 14:18:33 +00:00

Author

Owner

Root cause found in-code + fix PR: see above (unconditional ~k suffix).

crueber commented

2026-09-14 14:22:05 +00:00

Author

Owner

Review of PR #516 (fix/issue-513-etag) — verified in scratch worktree /tmp/pr516 (removed afterward). No browser per task rules (ETag-only backend change; tests + reasoning only — noted explicitly). No fixes pushed — none needed.

VERDICT: ready to merge.

(1) Root-cause claim: SOUND. Old summary.go appended ~k only when checksOK, so a zero-check repo's ETag was byte-identical to its pre-#505 ETag; writeCached 304s on match, so a pre-#505 cached field-less body revalidated forever and never received has_checks → missing-field fail-open kept the Checks tab visible. Corroborated by the pre-PR handlers_test.go, which asserted bare-sha If-None-Match → 304 (that assertion is exactly the bug, and the PR correctly rewrites it to read the live etag).

(2) ~k0 collision analysis: SAFE, verified in code. updateIndex (service.go:566-574) builds a zero-value IndexDoc then ix.Version++ before the first write, so a present index is always Version >= 1; CompactIndex only ++s existing docs; HasChecks returns the stored version verbatim. Hook-nil path falls into the same else branch (nil hook → checksOK=false → ~k0; covered by the 'nil hook' pin). Theoretical edge only: hand-written index bytes with version:0 would read ok=true/Version=0 and emit a colliding ~k0 — unreachable via any writer, not worth hardening.

(3) No behavior change besides ETag: summary.go diff is a pure else-branch string append; body construction untouched (has_checks rides exactly as #505 left it); Cache-Control untouched (mutable-collab no-cache); zero new store trips — the ChecksSummary probe pre-exists from #505, law 6 holds (summary off the push/sync/checkpoint budgets, sim unaffected).

(4) Regression test: TestSummaryChecksRevalidate (summary505_test.go:95-112) sends a bare-sha pre-#505 token, requires 200 + has_checks present-and-false. Proves the fix. Verified it FAILS on base code (overlaid origin/main summary.go into scratch: etag pin fails bare-vs-~k0; and on old code the bare token IS the current etag so the 200-requirement fails too — the old 304 test proved that path). Restore byte-verified via diff afterward.

(5) Pin updates: every changed pin bare → ~k0 exact-match across handlers/summary235/summary240/summary319/summary424/visibility345/health209/gaps — all justified, none weakened. health209 wantNoETag removal is correct (unborn now emits exact '"~k0"'; equally strong exact assertions). handlers_test 304 path now reads the live etag (stronger). Remaining bare-sha If-None-Match sends are intentional: tree-endpoint immutability (handlers_test.go:564), stale-healthy-must-200 (health209_test.go:157), the new regression probe (summary505_test.go:100). No gaps.

(6) Wording: collab.go checks comment, service.go HasChecks comment, and both 07_api.md passages updated to unconditional-~k0; the new 'Unconditional ~k suffix' decision appended (law 12). Leftover 'byte-identical ETag' strings (collab.go:145,160 re #319/#424, 07_api.md:1223 re ~c, counts_test.go:16) each scope to their own projection's conditional suffix (no ~c/~f added when declined) — still accurate, not stale.

(7) Tests: api -race clean, coverage 95.3% (gate holds); checks -race clean (32s); cmd/walhub clean; gofmt/vet clean; go build ./... green. internal/server shows 10 failures, all 'ui shell missing — run make build' (scratch web/dist unbuilt; PR touches no server/UI-serving code — environmental, not PR-caused). e2e: unaffected by reasoning + grep (no ETag pins outside internal/api; change is header-only on one endpoint).

No small problems found; nothing to fix. Merge recommended as-is. Main worktree left clean (read-only throughout).

Review of PR #516 (fix/issue-513-etag) — verified in scratch worktree /tmp/pr516 (removed afterward). No browser per task rules (ETag-only backend change; tests + reasoning only — noted explicitly). No fixes pushed — none needed. VERDICT: ready to merge. (1) Root-cause claim: SOUND. Old summary.go appended ~k only when checksOK, so a zero-check repo's ETag was byte-identical to its pre-#505 ETag; writeCached 304s on match, so a pre-#505 cached field-less body revalidated forever and never received has_checks → missing-field fail-open kept the Checks tab visible. Corroborated by the pre-PR handlers_test.go, which asserted bare-sha If-None-Match → 304 (that assertion is exactly the bug, and the PR correctly rewrites it to read the live etag). (2) ~k0 collision analysis: SAFE, verified in code. updateIndex (service.go:566-574) builds a zero-value IndexDoc then ix.Version++ before the first write, so a present index is always Version >= 1; CompactIndex only ++s existing docs; HasChecks returns the stored version verbatim. Hook-nil path falls into the same else branch (nil hook → checksOK=false → ~k0; covered by the 'nil hook' pin). Theoretical edge only: hand-written index bytes with version:0 would read ok=true/Version=0 and emit a colliding ~k0 — unreachable via any writer, not worth hardening. (3) No behavior change besides ETag: summary.go diff is a pure else-branch string append; body construction untouched (has_checks rides exactly as #505 left it); Cache-Control untouched (mutable-collab no-cache); zero new store trips — the ChecksSummary probe pre-exists from #505, law 6 holds (summary off the push/sync/checkpoint budgets, sim unaffected). (4) Regression test: TestSummaryChecksRevalidate (summary505_test.go:95-112) sends a bare-sha pre-#505 token, requires 200 + has_checks present-and-false. Proves the fix. Verified it FAILS on base code (overlaid origin/main summary.go into scratch: etag pin fails bare-vs-~k0; and on old code the bare token IS the current etag so the 200-requirement fails too — the old 304 test proved that path). Restore byte-verified via diff afterward. (5) Pin updates: every changed pin bare → ~k0 exact-match across handlers/summary235/summary240/summary319/summary424/visibility345/health209/gaps — all justified, none weakened. health209 wantNoETag removal is correct (unborn now emits exact '"~k0"'; equally strong exact assertions). handlers_test 304 path now reads the live etag (stronger). Remaining bare-sha If-None-Match sends are intentional: tree-endpoint immutability (handlers_test.go:564), stale-healthy-must-200 (health209_test.go:157), the new regression probe (summary505_test.go:100). No gaps. (6) Wording: collab.go checks comment, service.go HasChecks comment, and both 07_api.md passages updated to unconditional-~k0; the new 'Unconditional ~k suffix' decision appended (law 12). Leftover 'byte-identical ETag' strings (collab.go:145,160 re #319/#424, 07_api.md:1223 re ~c, counts_test.go:16) each scope to their own projection's conditional suffix (no ~c/~f added when declined) — still accurate, not stale. (7) Tests: api -race clean, coverage 95.3% (gate holds); checks -race clean (32s); cmd/walhub clean; gofmt/vet clean; go build ./... green. internal/server shows 10 failures, all 'ui shell missing — run make build' (scratch web/dist unbuilt; PR touches no server/UI-serving code — environmental, not PR-caused). e2e: unaffected by reasoning + grep (no ETag pins outside internal/api; change is header-only on one endpoint). No small problems found; nothing to fix. Merge recommended as-is. Main worktree left clean (read-only throughout).

crueber commented

2026-09-14 14:22:42 +00:00

Author

Owner

Fixed by PR #516 (review clean — root cause sound, ~k0 collision-safe, regression fails on base; api 95.3%, -race clean), merged. Closing for the second time.

crueber closed this issue

2026-09-14 14:22:42 +00:00

crueber referenced this issue from a commit

2026-09-14 14:22:43 +00:00

Merge pull request 'Fix #513 (reopened): unconditional ~k suffix' (#516) from fix/issue-513-etag into main

crueber referenced this issue from a commit

2026-09-14 16:06:11 +00:00

Review #519: fix wrong issue number in comments (#513 -> reopened #512)

crueber referenced this issue

2026-09-14 16:06:24 +00:00

Commit-graph lane gutter breaks across rows (per-row margins/borders): margin-free rows + cell padding so the gutter connects #512

crueber referenced this issue from a commit