Implement PRD-001/002: service registration, telemetry ingestion, SSE stream - #6
Merged
Merged
Conversation
…ream Extends the static registry with an optional telemetryDbPath (PRD-001a), adds a read-only SQLite reader over the service_status/service_metrics/service_logs contract (PRD-001b reader side), a ~1s poll-and-merge loop that probes /health and merges it with each service's telemetry into an in-memory fleet model with per-service fault isolation (PRD-001c), and a single fleet-telemetry SSE stream at GET /events mounted onto the existing status page (PRD-002/002a). The poll loop's schema-tolerant metrics reader and windowed log reads keep memory bounded per PRD-002c. Zero new dependencies: node:sqlite and node:http only.
Security review finding (medium): an unconstrained telemetryDbPath in the registry file let hivedoctor open ANY user-readable SQLite file and, if it had Contract-B-shaped tables, broadcast its contents on the unauthenticated loopback SSE stream (/events). coerceTelemetryDbPath now enforces containment under the trusted ~/.honeycomb/telemetry/ root via the existing assertWithinBase helper (the same defense-in- depth pattern healthUrl's loopback restriction already uses); a path that escapes falls back to undefined (health-probe-only), never a crash, mirroring coerceHealthUrl's fallback-on-invalid-input posture. Updated the test that previously asserted the vulnerable behavior (an arbitrary absolute path 'preserved verbatim') to instead assert containment, and added 3 new tests: an out-of-root path rejected, a '..' traversal rejected, and a legitimate nested subdirectory path preserved. 566/569 tests pass (3 pre-existing, unrelated failures, confirmed earlier this session); typecheck clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds a telemetry ingestion path from per-daemon SQLite files into an SSE ChangesTelemetry Ingestion and SSE Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StatusPageServer
participant telemetryPollLoop
participant TelemetryDbReader
Client->>StatusPageServer: GET /events
StatusPageServer->>telemetryPollLoop: onSnapshot subscribe
telemetryPollLoop->>TelemetryDbReader: readStatus/readMetrics/readNewLogs
TelemetryDbReader-->>telemetryPollLoop: telemetry rows
telemetryPollLoop-->>StatusPageServer: FleetTelemetryEvent
StatusPageServer-->>Client: SSE fleet-telemetry frame
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compose/index.ts`:
- Around line 707-715: The telemetry polling setup in telemetryPollLoop is
bypassing the injected CreateHiveDoctorOptions.probe by hard-coding probeHealth
inside createPollLoop. Update the probe argument in this createPollLoop call to
use the provided injected probe for telemetry entries as well, matching the
existing seam used elsewhere. Keep the wiring consistent so telemetry health
checks respect the caller-supplied probe instead of always calling probeHealth.
In `@src/ingestion/poll-loop.ts`:
- Around line 188-195: The poll loop currently trusts any DB under the root and
attributes its data to entry.name without verifying the DB’s own
ServiceStatusRow.name. In src/ingestion/poll-loop.ts inside the loop that calls
readStatus, readMetrics, and readNewLogs, compare the returned status name
against the registry entry name and treat any mismatch as malformed. If the
names do not match, skip caching and forwarding all rows for that DB so
mispointed telemetryDbPath values cannot cross-wire services.
- Around line 297-305: The reload logic in reload() only removes runtimes for
deleted names, so a same-name entry that changes telemetryDbPath keeps reusing
the old cached runtime and its lastLogId. Update the runtime cache handling in
src/ingestion/poll-loop.ts so reload() also evicts or resets the matching entry
when its telemetryDbPath changes or disappears, forcing the next run to reopen
the DB instead of reusing the stale one. Use the existing reload(), runtime map,
closeHandle(), and the daemon entry’s telemetryDbPath to locate and invalidate
the cached runtime.
In `@src/registry.ts`:
- Around line 204-210: The path guard in registry.ts validates resolve(expanded)
but returns expanded, so a relative telemetryDbPath can later be reopened under
a different cwd in poll-loop.ts. Update the logic in the path
expansion/validation flow around assertWithinBase so the exact returned path is
the same path that was validated, and reject any non-absolute post-~ input
before returning it.
In `@src/status-page/server.ts`:
- Around line 257-265: The 404 fallback in the status-page server no longer
matches the public routes because `/events` is missing from the discovered route
list. Update the 404 response construction in `server.ts` so the route metadata
includes `/events` whenever `onEvents` is wired, keeping the fallback payload in
sync with the `onEvents` handler and the existing `"/"` and `"/status.json"`
entries.
In `@tests/compose/telemetry-wiring.test.ts`:
- Around line 125-137: The telemetry wiring test starts the status page directly
via doctor.statusPage.start() but only calls doctor.stop(), which does not shut
it down unless doctor.start() was used. Update this test to explicitly stop the
status page by calling the status page shutdown/stop method on the
doctor.statusPage instance after the fetch assertion, so the HTTP server is
always cleaned up and doesn’t leak into later tests.
- Around line 159-176: Move the explicit doctor.telemetryPollLoop.close()
cleanup into a finally block around the tick/assertion sequence so it runs even
when expect(event.services) fails; keep the same telemetryPollLoop and
doctor.stop() cleanup path, but ensure the SQLite-backed handle is always closed
before tmp dir teardown to avoid the Windows EBUSY race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8338780c-648b-43a3-b47c-18d99f4ec424
📒 Files selected for processing (11)
src/compose/index.tssrc/ingestion/poll-loop.tssrc/ingestion/sse.tssrc/registry.tssrc/status-page/server.tssrc/telemetry/schema.tssrc/telemetry/sqlite-reader.tstests/compose/telemetry-wiring.test.tstests/ingestion/poll-loop.test.tstests/ingestion/sse.test.tstests/registry.test.ts
Comment on lines
+125
to
+137
| it("wires /events onto the existing status page and it emits a well-formed fleet-telemetry frame", async () => { | ||
| const doctor = buildDoctor(); | ||
| doctor.statusPage.start(); | ||
| const port = await waitForPort(doctor); | ||
|
|
||
| const { statusCode, body } = await fetchEventsFirstFrame(port); | ||
| expect(statusCode).toBe(200); | ||
| expect(body).toContain("event: fleet-telemetry"); | ||
| expect(body).toContain('"services"'); | ||
| expect(body).toContain('"logs"'); | ||
|
|
||
| await doctor.stop(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stop the status page explicitly in this test.
Line 127 starts doctor.statusPage directly, but Line 136 calls doctor.stop(), which is a no-op unless doctor.start() ran. This leaves the HTTP server open across tests.
Suggested fix
it("wires /events onto the existing status page and it emits a well-formed fleet-telemetry frame", async () => {
const doctor = buildDoctor();
doctor.statusPage.start();
- const port = await waitForPort(doctor);
-
- const { statusCode, body } = await fetchEventsFirstFrame(port);
- expect(statusCode).toBe(200);
- expect(body).toContain("event: fleet-telemetry");
- expect(body).toContain('"services"');
- expect(body).toContain('"logs"');
-
- await doctor.stop();
+ try {
+ const port = await waitForPort(doctor);
+ const { statusCode, body } = await fetchEventsFirstFrame(port);
+ expect(statusCode).toBe(200);
+ expect(body).toContain("event: fleet-telemetry");
+ expect(body).toContain('"services"');
+ expect(body).toContain('"logs"');
+ } finally {
+ doctor.statusPage.stop();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("wires /events onto the existing status page and it emits a well-formed fleet-telemetry frame", async () => { | |
| const doctor = buildDoctor(); | |
| doctor.statusPage.start(); | |
| const port = await waitForPort(doctor); | |
| const { statusCode, body } = await fetchEventsFirstFrame(port); | |
| expect(statusCode).toBe(200); | |
| expect(body).toContain("event: fleet-telemetry"); | |
| expect(body).toContain('"services"'); | |
| expect(body).toContain('"logs"'); | |
| await doctor.stop(); | |
| }); | |
| it("wires /events onto the existing status page and it emits a well-formed fleet-telemetry frame", async () => { | |
| const doctor = buildDoctor(); | |
| doctor.statusPage.start(); | |
| try { | |
| const port = await waitForPort(doctor); | |
| const { statusCode, body } = await fetchEventsFirstFrame(port); | |
| expect(statusCode).toBe(200); | |
| expect(body).toContain("event: fleet-telemetry"); | |
| expect(body).toContain('"services"'); | |
| expect(body).toContain('"logs"'); | |
| } finally { | |
| doctor.statusPage.stop(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/compose/telemetry-wiring.test.ts` around lines 125 - 137, The telemetry
wiring test starts the status page directly via doctor.statusPage.start() but
only calls doctor.stop(), which does not shut it down unless doctor.start() was
used. Update this test to explicitly stop the status page by calling the status
page shutdown/stop method on the doctor.statusPage instance after the fetch
assertion, so the HTTP server is always cleaned up and doesn’t leak into later
tests.
Comment on lines
+159
to
+176
| const event = await doctor.telemetryPollLoop.tick(); | ||
| expect(event.services).toEqual([ | ||
| { | ||
| name: "honeycomb", | ||
| health: "ok", | ||
| lastSeen: "2026-07-01T00:00:00.000Z", | ||
| metrics: { actionsTaken: 4, filesProcessed: 2, memoriesCreated: 1 }, | ||
| deeplake: { connected: true, lastCommunicationAt: "2026-07-01T00:00:00.000Z" }, | ||
| telemetryFault: null, | ||
| }, | ||
| ]); | ||
|
|
||
| // stop() without a matching start() is a documented no-op (mirrors every other | ||
| // pre-existing test in this repo that drives a loop directly without the full | ||
| // lifecycle); close the poll loop's cached read-only handle explicitly so the tmp | ||
| // dir cleanup below does not race an open SQLite file on Windows. | ||
| doctor.telemetryPollLoop.close(); | ||
| await doctor.stop(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the SQLite-backed poll loop in a finally block.
The explicit doctor.telemetryPollLoop.close() runs only after the assertion on Line 160. When that assertion fails, the DB handle stays open and Windows cleanup hits the EBUSY unlink reported in CI.
Suggested fix
- const event = await doctor.telemetryPollLoop.tick();
- expect(event.services).toEqual([
- {
- name: "honeycomb",
- health: "ok",
- lastSeen: "2026-07-01T00:00:00.000Z",
- metrics: { actionsTaken: 4, filesProcessed: 2, memoriesCreated: 1 },
- deeplake: { connected: true, lastCommunicationAt: "2026-07-01T00:00:00.000Z" },
- telemetryFault: null,
- },
- ]);
-
- // stop() without a matching start() is a documented no-op (mirrors every other
- // pre-existing test in this repo that drives a loop directly without the full
- // lifecycle); close the poll loop's cached read-only handle explicitly so the tmp
- // dir cleanup below does not race an open SQLite file on Windows.
- doctor.telemetryPollLoop.close();
- await doctor.stop();
+ try {
+ const event = await doctor.telemetryPollLoop.tick();
+ expect(event.services).toEqual([
+ {
+ name: "honeycomb",
+ health: "ok",
+ lastSeen: "2026-07-01T00:00:00.000Z",
+ metrics: { actionsTaken: 4, filesProcessed: 2, memoriesCreated: 1 },
+ deeplake: { connected: true, lastCommunicationAt: "2026-07-01T00:00:00.000Z" },
+ telemetryFault: null,
+ },
+ ]);
+ } finally {
+ doctor.telemetryPollLoop.close();
+ await doctor.stop();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const event = await doctor.telemetryPollLoop.tick(); | |
| expect(event.services).toEqual([ | |
| { | |
| name: "honeycomb", | |
| health: "ok", | |
| lastSeen: "2026-07-01T00:00:00.000Z", | |
| metrics: { actionsTaken: 4, filesProcessed: 2, memoriesCreated: 1 }, | |
| deeplake: { connected: true, lastCommunicationAt: "2026-07-01T00:00:00.000Z" }, | |
| telemetryFault: null, | |
| }, | |
| ]); | |
| // stop() without a matching start() is a documented no-op (mirrors every other | |
| // pre-existing test in this repo that drives a loop directly without the full | |
| // lifecycle); close the poll loop's cached read-only handle explicitly so the tmp | |
| // dir cleanup below does not race an open SQLite file on Windows. | |
| doctor.telemetryPollLoop.close(); | |
| await doctor.stop(); | |
| try { | |
| const event = await doctor.telemetryPollLoop.tick(); | |
| expect(event.services).toEqual([ | |
| { | |
| name: "honeycomb", | |
| health: "ok", | |
| lastSeen: "2026-07-01T00:00:00.000Z", | |
| metrics: { actionsTaken: 4, filesProcessed: 2, memoriesCreated: 1 }, | |
| deeplake: { connected: true, lastCommunicationAt: "2026-07-01T00:00:00.000Z" }, | |
| telemetryFault: null, | |
| }, | |
| ]); | |
| } finally { | |
| doctor.telemetryPollLoop.close(); | |
| await doctor.stop(); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI / 0_HiveDoctor gate (ubuntu-latest).txt
[error] 160-160: Vitest assertion failed (createHiveDoctor telemetry wiring PRD-001/PRD-002: registered entry with telemetryDbPath feeds telemetry poll loop end to end). Expected services health "ok" but received "unreachable".
🪛 GitHub Actions: CI / HiveDoctor gate (ubuntu-latest)
[error] 160-160: vitest run failed: AssertionError in 'a registered entry with telemetryDbPath feeds the telemetry poll loop end to end' (expected services[0].health 'ok', received 'unreachable').
🪛 GitHub Check: HiveDoctor gate (macos-latest)
[failure] 160-160: tests/compose/telemetry-wiring.test.ts > createHiveDoctor telemetry wiring (PRD-001/PRD-002) > a registered entry with telemetryDbPath feeds the telemetry poll loop end to end
AssertionError: expected [ { name: 'honeycomb', …(5) } ] to deeply equal [ { name: 'honeycomb', …(5) } ]
- Expected
- Received
@@ -2,11 +2,11 @@
{
"deeplake": {
"connected": true,
"lastCommunicationAt": "2026-07-01T00:00:00.000Z",
},
-
"health": "ok",
-
"health": "unreachable", "lastSeen": "2026-07-01T00:00:00.000Z", "metrics": { "actionsTaken": 4, "filesProcessed": 2, "memoriesCreated": 1,
❯ tests/compose/telemetry-wiring.test.ts:160:26
🪛 GitHub Check: HiveDoctor gate (ubuntu-latest)
[failure] 160-160: tests/compose/telemetry-wiring.test.ts > createHiveDoctor telemetry wiring (PRD-001/PRD-002) > a registered entry with telemetryDbPath feeds the telemetry poll loop end to end
AssertionError: expected [ { name: 'honeycomb', …(5) } ] to deeply equal [ { name: 'honeycomb', …(5) } ]
- Expected
- Received
@@ -2,11 +2,11 @@
{
"deeplake": {
"connected": true,
"lastCommunicationAt": "2026-07-01T00:00:00.000Z",
},
-
"health": "ok",
-
"health": "unreachable", "lastSeen": "2026-07-01T00:00:00.000Z", "metrics": { "actionsTaken": 4, "filesProcessed": 2, "memoriesCreated": 1,
❯ tests/compose/telemetry-wiring.test.ts:160:26
🪛 GitHub Check: HiveDoctor gate (windows-latest)
[failure] 160-160: tests/compose/telemetry-wiring.test.ts > createHiveDoctor telemetry wiring (PRD-001/PRD-002) > a registered entry with telemetryDbPath feeds the telemetry poll loop end to end
AssertionError: expected [ { name: 'honeycomb', …(5) } ] to deeply equal [ { name: 'honeycomb', …(5) } ]
- Expected
- Received
@@ -2,11 +2,11 @@
{
"deeplake": {
"connected": true,
"lastCommunicationAt": "2026-07-01T00:00:00.000Z",
},
-
"health": "ok",
-
"health": "unreachable", "lastSeen": "2026-07-01T00:00:00.000Z", "metrics": { "actionsTaken": 4, "filesProcessed": 2, "memoriesCreated": 1,
❯ tests/compose/telemetry-wiring.test.ts:160:26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/compose/telemetry-wiring.test.ts` around lines 159 - 176, Move the
explicit doctor.telemetryPollLoop.close() cleanup into a finally block around
the tick/assertion sequence so it runs even when expect(event.services) fails;
keep the same telemetryPollLoop and doctor.stop() cleanup path, but ensure the
SQLite-backed handle is always closed before tmp dir teardown to avoid the
Windows EBUSY race.
Sources: Linters/SAST tools, Pipeline failures
…lemetry poll, name-mismatch guard, reload eviction, path validation, test cleanup - compose: telemetry poll loop honors the injected CreateHiveDoctorOptions.probe seam instead of hard-coding probeHealth (fixes the failing telemetry-wiring CI test) - poll-loop: reject a telemetry DB whose service_status.name does not match the registry entry (treated as malformed; nothing cached or forwarded) - poll-loop: reload() also evicts a same-name runtime whose telemetryDbPath changed or disappeared, so the next tick reopens the new DB from the start - registry: coerceTelemetryDbPath rejects relative post-tilde paths and returns the exact resolved absolute path it validated (no cwd-dependent reopen) - status page: 404 fallback lists /events when the onEvents handler is wired - telemetry-wiring tests: wrap in try/finally so the status page server and the SQLite-backed poll loop handle never leak on a failed assertion (Windows EBUSY)
This was referenced Jul 4, 2026
Merged
Closed
This was referenced Jul 12, 2026
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
Implements hivedoctor as the single source of truth for hive health + telemetry (ADR-0001/0002), part of the portal + telemetry realignment initiative driven by
/the-smokeragainstlibrary/initiatives/portal-and-telemetry-realignment.md(the-apiary superproject).telemetryDbPath, additive, backward-compatible), the runtime SQLite status/metrics/log contract, and the ~1s read-only poll-and-merge loop with per-service fault isolation and registry-reload triggers.GET /eventsSSE stream (fleet-telemetry) on the existing loopback status page, the schema-tolerant metrics reader (works across honeycomb's 3-counter and hivenectar's 5-counter tables), and windowed/bounded reads.Security
security-worker-bee found one medium finding: an unconstrained
telemetryDbPathlet a poisoned registry entry point hivedoctor at an arbitrary user-readable SQLite file and broadcast its contents over the unauthenticated loopback SSE stream. Remediated (ad2174a): containment under~/.honeycomb/telemetry/via the existingassertWithinBasehelper, falling back to health-probe-only on escape. 3 new containment tests added.Quality
quality-worker-bee independently re-ran typecheck/tests and re-verified the security fix by hand-tracing the containment logic. Verdict: PASS. All ACs implemented except one deliberately out-of-scope item (PRD-001a AC-3, plural DB paths, excluded by the pinned cross-repo contract so 3 sibling repos could build in parallel against one literal shape).
Test plan
npm run typecheckcleannpm test: 566/569 passing (3 pre-existing, unrelated failures, confirmed via parent-commit checkout+rerun, not a regression)node:sqlite,node:httpbuilt-ins only)Full acceptance-criteria ledger:
library/ledger/EXECUTION_LEDGER.mdin the-apiary superproject (companion PRs open there and in honeycomb, hivenectar, the-hive).Summary by CodeRabbit
GET /eventsusing Server-Sent Events (fleet-telemetry).