Gap-hunt audit (PR #31/#32/#33 surface): 18 fixes + catalogued-entry cleanup - #34
Merged
Conversation
GET /api/v1/nodes/agent-release was registered after GET /api/v1/nodes/:nodeId. The static+param collision forces Hono onto the registration-order-sensitive TrieRouter, so the release route was swallowed by the detail handler (404), silently killing the whole PR #32 update-available feature in production while isolated unit tests stayed green. Register the static route first (mirroring /export); make the release service injectable through registerNodeRoutes; add a full-surface route test (red: 404, green: 200). Extract large node-route test fakes to node-routes-helpers.ts to stay under the LOC guard. Co-Authored-By: Claude Opus 4.8 <[email protected]>
A never-contacted provisioning node was reported as reachable by both the rakkr_node_online metric gauge and the dashboard Active Nodes count, because each used a naive status !== "offline" that predates the new provisioning state — inflating the reachable count and masking the RakkrNodeOffline alert. Add a shared isNodeReachable predicate (online/ recording/degraded/alerting) and use it in both sites so they cannot diverge again. Red->green via a metrics test (provisioning -> 0) and a dashboardReportingNodes helper test. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The calendar day-cell create path called defaultDraft(firstNode) without the controller settings, so it always fell back to the built-in profile/ policies and ignored the operator's configured scheduling defaults (the schedules list page passes them). Thread the already-fetched controller settings into the create draft, mirroring the list page. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…(audit S4) The keep-vs-?? merge (omitted field keeps, explicit null clears a default) was only exercised by a DB-gated test, so a regression of keep() to ?? would clear-silently-broken with the whole in-memory suite still green. Add an in-memory route test that sets two defaults independently, confirms an unrelated PATCH preserves them, and confirms explicit null clears exactly one. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…ne (audit N3) The node-lifecycle baseline verifier (shipped to promote node lifecycle to done) never asserted the provisioning-gating invariant PR #31 introduced: its source list omitted node-liveness.ts and watchdog-node-liveness.ts and no phrase/snippet mentioned provisioning or the offline gate. Document the provisioning/offline-liveness contract and extend the verifier to assert the gate source, the isNodeReachable predicate, and the liveness/watchdog test titles. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
useServerPagination only reset offset on filter/page-size change, never when the row total shrank below the current offset. Deleting the last page's rows (bulk delete / retention sweep) stranded the operator on a blank page past the end with only Previous to escape. Add a pure clampedOffset helper (red->green) + a clampToTotal hook method the pages call during render with the server meta.total, mirroring the filter-reset pattern. Wired into all paginated pages. Co-Authored-By: Claude Opus 4.8 <[email protected]>
PR #31 filtered the test-only stub upload policy from settings/scheduling but missed two paths: (H3-1) the recordings page fed the unfiltered list to the per-recording and bulk Queue Upload dropdowns, so the stub was a selectable — and default-first — real upload target; (H3-2) editing a legacy schedule persisted with the stub silently re-saved it (the form's filtered toggles left no way to clear it). Add a selectableUploadPolicies helper for the recordings action dropdowns (labeling keeps the full list), and strip the stub in scheduleToDraft. Red->green helper + draft tests. (recordings.tsx also carries its pagination clampToTotal wiring, H4-1.) Co-Authored-By: Claude Opus 4.8 <[email protected]>
…H1-1) The residual instance of the G1 route-shadow class: the node-auth GET /recording-jobs/:jobId (registered before the operator export route) swallowed GET /recording-jobs/export under TrieRouter, answering it with a node-credential 401. The agent handler must stay registered first (so agent job-reads work), so defer the reserved 'export' segment to the downstream operator handler — a job id is never literally 'export'. Red->green in the agent-job-read harness (production registration order). Co-Authored-By: Claude Opus 4.8 <[email protected]>
…it N4) The heartbeat write persisted the agent-reported status verbatim and the schema accepted 'provisioning'/'offline', so a first heartbeat only promoted a provisioning node because the shipped agent hardcodes 'online' — and any node-credential holder could POST status:'provisioning' to un-promote a live node, which deriveNodeStatus short-circuits, suppressing its offline alert forever. Coerce a heartbeat's provisioning/offline to online in both stores (heartbeatStatus); the controller owns lifecycle state. Red->green transform tests (also closes the H2-2 coverage gap). Co-Authored-By: Claude Opus 4.8 <[email protected]>
PR #31 intended every upload policy to target a real destination, but the create input left destinationId optional and the console eager-created a destination-less policy on New — assignable, and reconciling its recordings to partial (provider_not_configured). Enforce destinationId at the operator create route (the store stays lenient for seeds/tests); client seeds the first destination and disables New until one exists. Red->green route test; fixture updates for the stricter route. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The node inventory status filter omitted 'provisioning', so operators could not filter to the enrolled-but-never-contacted cohort the API fully supports. Add it and lock the dropdown against the full NodeStatus enum. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Each schedule upload policy id fans a recording out to its own upload queue item, but the list was uncapped and un-deduped — a schedule:manage holder could multiply queue work per recording with a long/duplicate list (the sibling switcher-mappings list already caps at 256). Cap the schedule create/update schemas at 32 and dedup the client draft before submit. Red->green dedup test. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…3 coverage) The create route rejects a destination-less policy; add the matching coverage on the update path — an empty destinationId is schema-rejected (400) and an omitted one is preserved — so a future nullable/empty-allowed change to the update schema can't silently reopen the H3-3 hole. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Run 4 convergence attempt: adversary-on-Run-3 + completeness critic (all 85 changed files), data-integrity/concurrency, permission-boundary sweeps — zero code changes; db:verify migration replay green. R4-1/R4-2 logged as suspected/mitigated. First clean run. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Run 5 convergence (fresh angles): end-to-end feature-trace, deploy/ops + docs/baseline consistency, observability/error-surface — all clean, zero code changes. node:test-db 22/22 green. Second consecutive clean run. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…-triage)
Run-6 re-triage found H4-2 was mis-classified as cosmetic for the watchdog
editor: clearing a numeric field yields Number("")===0, and thresholdDbfs
(dbfsSchema [-160,24]) and the score thresholds ([0,1]) ACCEPT 0 — so a
cleared threshold silently persists 0 and arms an always-fire alert
(watchdog-signal fires low-signal for all healthy audio), flooding the
operator with spurious critical health events. Add a shared numericInputCommit
helper (empty/invalid -> no commit, never 0) + a local text buffer in the
shared NumberField (keeps the field editable while typing) so all 16 watchdog
numeric fields are fixed at one point. Red->green helper test. Other editors'
clear->0 is server-rejected (no persist) and stays the catalogued cosmetic.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…reset 2->0) Catalogue re-triage found H4-2 mis-classified (watchdog thresholdDbfs/score thresholds accept 0 -> persisted always-fire alert); fixed as H4-2W. Other 19 catalogued items confirmed correctly triaged. New suspected: R6-ENROLL-DUP, R6-INSTALL-URL-QUOTE. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…dit R7-IPCAP) The agent's collect_ip_addresses() reports every address from 'hostname -I' uncapped, but the heartbeat schema caps ipAddresses at .max(16). A multi-homed node (>16 IPs: IPv6 SLAAC/privacy + Docker/libvirt/VLAN bridges) had every heartbeat rejected 400 — and since the agent freezes the IP list at startup, that desynced the node forever, freezing lastSeenAt and flipping the live, recording node offline. A liveness heartbeat must not fail closed over a cosmetic field: preprocess ipAddresses to truncate to the documented cap and accept the heartbeat (keeping the first 16, the primary addresses). Red->green route test (20 IPs -> 202 + truncated, was 400). Agent-side .take(16) noted as defensive follow-up. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…(streak 0) Rust agent-contract angle found the agent's uncapped ipAddresses vs the heartbeat .max(16) reject -> permanent desync for multi-homed nodes; fixed controller-side (truncate). H4-2W verified sound. New catalogue: R7-IP-AGENT-CAP, R7-NUMCOMMIT-HEX, R7-SEED-LIVENESS. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…R8-RETENTION-SELECT) The schedule form wrapped its recording-profile and watchdog Selects in withSelectedOption (which prepends a synthetic option so a current-but-absent id stays visible), but the retention Select rendered the raw list. A stale/ deleted retention id (a prefilled controller-settings default, or an edited schedule whose policy was removed) made the controlled Select fall back to its placeholder — reading as 'unselected' while the draft silently kept and re-saved the id. Route retention through the same helper (moved to schedule-draft as a shared, now-tested export). Red->green helper test. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…k 0) Broad web sweep found the schedule-form retention Select not wrapped in withSelectedOption (stale id renders blank but re-saves); fixed. R7-IPCAP verified sound+complete. Runs 6-8 each closed one marginal PR-surface defect. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Adversary verified R8 sound; completeness critic over all 85 changed files found no new real defect. Zero code changes. R9-NODEIFACE-SELECT catalogued (pre-existing/bounded). First clean run after the 6-8 dirty streak. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Security-lens + production-scenario edge passes both clean; zero code changes. Cross-replica schedule double-fire investigated -> documented single-writer constraint, rejected. Two consecutive clean runs. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Fresh-eyes dense-logic re-read + test-durability critic (all 18 fixes' tests re-derived to catch a revert) both clean; zero code changes. Three consecutive clean runs. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Diff-as-reviewer (raw PR hunks) + React effect/query-timing passes, both clean (run inline after session limits killed the subagents; diff-reviewer rerun independently confirmed). Zero code changes. Four consecutive clean runs. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Run 13 final convergence: last-chance broad sweep + agent-contract re-confirm both clean; full 'mise run check' (incl. rust+miri+fake-controller-smoke+ db:verify+all baselines) and 'mise run build' green. 18 findings fixed across 13 runs against static base 619b6f1; remaining opens all catalogued cosmetic/ suspected/pre-existing/by-design. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Post-convergence cleanup of the 2026-07-08 gap-hunt catalogue (API items):
- R4-1: dedup uploadPolicyIds server-side in buildSchedule +
sanitizeScheduleUpdate (mirrors uniqueTags) so the server is authoritative.
- R4-2: isolate each node's reconcile in reconcileNodeLivenessEvents with a
per-node try/catch -> {skipped, reconcile_failed} so one failing node no
longer aborts the whole tick.
- H1-3: treat a provisioning node as unavailable in node-action-routes with an
accurate node_provisioning reason (not node_offline).
- N-3A/3B: agent-release fetch now follows Link rel=next (bounded to 5 pages,
same-origin) and reads the body through a bounded reader (Content-Length
pre-check + streaming byte cap); an over-cap body is rejected, last-good kept.
- N-COV-FETCH + R11: lock the GitHub request contract and the uploadPolicyIds
>32 rejection.
Each with a red->green/coverage test.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Post-convergence cleanup of the 2026-07-08 gap-hunt catalogue (web items): - R3-1 (operator decision: Critical): offline node status now reads critical (red); nodeStatusBadgeClass rewritten as an exhaustive switch. - R3-2: toneFillClass gets an explicit neutral (muted) branch. - H4-2/H4-3: extract the buffered NumberField into shared settings-fields and wire it into the recording-profile, upload-policy, and watchdog cards; retention's optionalNumber routes through numericInputCommit. - R7-NUMCOMMIT-HEX: numericInputCommit gates on a decimal-shape regex. - R9: node/interface selects show a synthetic "(unavailable)" option for a stale id (mirrors withSelectedOption). - R6-INSTALL-URL-QUOTE: shell-quote every interpolated install-command value. - H2: nodeStatusLabel (Title Case) replaces raw status on dashboard, room-detail, and the nodes table. - S2: remove the dead schedulingDefaultsFrom export. - S3: defaultDraft validates operator defaults against available lists (threaded from schedules list + calendar via cached query data). - W2: TruncateCell effect depends on [children] (no per-render observer churn). - W3: nodes.tsx omits the Actions column entirely for read-only users. - W4A: withWatchdogDisplayDefaults folds display fallbacks into the draft so the watchdog form is what-you-see-is-what-you-save. - R6-ENROLL-DUP: invalidate ["nodes"] on enroll error too. Unit-tested where a seam exists (node-status, settings-updates, schedule-draft, node-page-helpers); render-only items verified via tsc + oxlint + build. Co-Authored-By: Claude Opus 4.8 <[email protected]>
R7-IP-AGENT-CAP: refactor collect_ip_addresses to a pure parse_ip_addresses helper bounded to MAX_IP_ADDRESSES (16, matching nodeHeartbeatSchema), so a well-behaved agent never emits a payload the controller must truncate. Pure Rust unit test (cap at 16, normal parse, empty). Co-Authored-By: Claude Opus 4.8 <[email protected]>
Document the disposition of the 2026-07-08 gap-hunt catalogue after convergence: 21 items fixed (each with a test where a unit seam exists), 6 deferred with rationale (render-harness-gated or product decisions, plus R7-SEED-LIVENESS as won't-fix). Records the two operator product decisions (offline tone -> Critical; switcher password-clear -> defer) and the green full-check gate at close. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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
1 participant
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.
Summary
An adversarial correctness/security audit of the freshly-landed PR #31/#32/#33
surface (provisioning node state, agent update-available, scheduling defaults,
web-console polish), run as the repeatable loop in
docs/contributing/audit-workflow.md, converged at 5 consecutive clean runs(Runs 9–13, 13 runs total). This PR bundles the 18 audit fixes plus the
post-convergence catalogued-entry cleanup (21 more items) the ledger tracked.
Ledger:
docs/internal/audits/2026-07-08-gap-hunt.md.Audit fixes (18)
Headlines:
G1— a Hono static-vs-param route-registration collision left theentire PR Recorder-agent update-available check; promote node lifecycle to ✅ #32 "update available" feature silently dead in production (its
isolated unit test stayed green). Reproduced empirically, fixed, locked with a
full-surface route test.
N4— a heartbeat could un-promote a live node toprovisioningandsuppress its offline alert; the controller now owns the lifecycle state machine.
R7-IPCAP— a multi-homed node's over-capipAddressesdesynced itpermanently (every heartbeat 400'd → flipped offline while live); the schema now
truncates instead of failing closed.
N1/N2), stub-upload removal completion(
H3-1/H3-2/H3-3), pagination re-clamp (H4-1), UI-honesty fixes(
H4-2W/R8), and route-shadow residual (H1-1).Post-convergence cleanup (21 fixed, 6 deferred)
Worked through the catalogued backlog after convergence. None were confirmed
regressions (that's why it converged) — they are hardening / consistency /
UX-honesty improvements. Highlights:
uploadPolicyIdsdedup (R4-1), per-node liveness errorisolation (R4-2), accurate
node_provisioningaction reason (H1-3), agent-releasepaging + bounded body read (N-3A/3B), request-contract + cap coverage.
buffered
NumberField+ decimal-guardednumericInputCommitacross the settingscards (H4-2/H4-3/R7-NUMCOMMIT-HEX), stale node/interface/default guards
(R9/S3),
nodeStatusLabel(H2), dead-export removal (S2), and W2/W3/W4A/R6.ipAddressescapped at the documented heartbeat limit(R7-IP-AGENT-CAP).
Deferred (6), with rationale in the ledger: switcher password-clear affordance
(R3-3, operator decision) + its coupled coverage (R3-6); render-harness-gated items
(H4-1-CLAMP-HOOK-COV, W1, W4); and R7-SEED-LIVENESS as won't-fix — deriving
liveness on the frozen-timestamp demo seed store would flip the whole demo offline
~120s after startup (production always uses Postgres, which derives).
Two operator product decisions were honored: offline tone → Critical and
switcher password-clear → defer.
Testing
Every fix with a unit seam has a red→green or coverage-lock test; React-render-only
items are verified by tsc + oxlint + build (the web unit harness has no RTL/jsdom).
Full gate green at close:
mise run checkexit 0 — tsc, API + web node tests,oxlint, oxfmt,
check:loc,db:verify(Drizzle replay 0001–0046), all baselineverifiers,
rust:check/clippy/fmt/miri(138 pass / 0 fail / 28 miri-ignored),and
agent:fake-controller-smoke— plusmise run build.🤖 Generated with Claude Code