Implement PRD-017: service check-in and SQLite telemetry#11
Merged
Conversation
Adds release.yaml mirroring honeycomb's/hivedoctor's OIDC Trusted Publishing model on v* tags (no NPM_TOKEN, id-token: write, npm publish --provenance), adjusted for hivenectar's actual scripts (build is a plain tsc, no aggregate `ci`/`pack:check` script yet, so typecheck+test run as discrete gate steps). Adds the minimum package.json fix needed for a valid scoped-package publish: publishConfig (access: public, provenance) and a prepack build hook. Part of the-apiary PRD-001 (hive-release-manifest-and-ci), Wave 1.
Extends the existing hivedoctor registry writer with a telemetryDbPath field and adds a fail-soft local telemetry surface (src/telemetry/) built on node:sqlite (WAL mode, zero new dependencies), per hivedoctor's ADR-0001/ADR-0002: - checkin.ts: runtime status check-in/heartbeat (binding time, last-seen, health sourced from the same PipelineStatus /health reports) - metrics.ts: the 5-counter since-restart snapshot (files registered, nectars minted, descriptions generated, source-graph versions, embeddings computed), including a store wrapper that increments nectar mint/version-write counters at their real completion point with zero changes to the registration ladder - logs.ts: bounded/rotated log emission with redaction, tapping hivenectar's existing structured log sink - daemon.ts wires check-in/heartbeat and the log tap on every start(), fail-soft throughout so a telemetry failure never blocks boot or the nectar pipeline descriptionsGenerated/embeddingsComputed are wired to the closest real row signal available today (describeStatus === "described" / non-null embedding), since the enricher and embeddings PRDs aren't implemented in this repo yet; both counters read 0 until those land and will start moving automatically with no further wiring.
Security review finding (medium): the telemetry directory was created without a restrictive mode, so on a multi-user host the default umask often yields a world-readable/traversable directory, letting another local user read service_logs (paths, errors, partially-redacted text) and operational metrics. Matches honeycomb's existing fleet-store.ts precedent (mkdirSync with mode: 0o700). Added a POSIX-only test (skipped on Windows, where mode bits don't apply) asserting the created directory is exactly 0o700. 223/225 tests pass (2 skips: 1 pre-existing live-Deep-Lake, 1 this new Windows skip); 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 (5)
📝 WalkthroughWalkthroughThis PR adds a fail-soft SQLite telemetry subsystem for check-ins, metrics, and logs, wires it into daemon and registration flows, exposes it through public exports, and adds a release workflow with npm trusted publishing and updated package scripts. ChangesTelemetry Integration
Package Publishing and Release Workflow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Daemon
participant createTelemetry
participant openTelemetryDb
participant CheckinService
participant MetricsWriter
participant LogWriter
Daemon->>createTelemetry: createTelemetry(opts)
createTelemetry->>openTelemetryDb: openTelemetryDb(dbPath)
alt DB opens
createTelemetry->>CheckinService: startCheckin(health, opts)
createTelemetry->>MetricsWriter: metrics / metricsSnapshot
createTelemetry->>LogWriter: log(level, message)
createTelemetry-->>Daemon: Telemetry (enabled)
else DB open fails
createTelemetry-->>Daemon: createNullTelemetry
end
Estimated code review effort: 4 (Complex) | ~75 minutes 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: 5
🧹 Nitpick comments (3)
.github/workflows/release.yaml (1)
229-235: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer
gh releaseover a third-party release action in this privileged job.The runner already includes GitHub CLI, so replacing
softprops/action-gh-releasereduces third-party action surface while keeping release creation simple.Proposed refactor
- name: Create GitHub Release if: ${{ github.ref_type == 'tag' && steps.mode.outputs.publish == 'true' }} - uses: softprops/action-gh-release@v2.4.1 - with: - tag_name: ${{ github.ref_name }} - name: ${{ github.ref_name }} - generate_release_notes: true + env: + GH_TOKEN: ${{ github.token }} + run: | + if gh release view "$GITHUB_REF_NAME" >/dev/null 2>&1; then + echo "GitHub Release already exists for $GITHUB_REF_NAME." + else + gh release create "$GITHUB_REF_NAME" \ + --title "$GITHUB_REF_NAME" \ + --generate-notes + fi🤖 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 @.github/workflows/release.yaml around lines 229 - 235, The Create GitHub Release step in the release workflow still uses the third-party softprops/action-gh-release action; replace that step with a GitHub CLI-based release command using gh release in the same job. Keep the existing tag_name/name/release-notes behavior equivalent by using github.ref_name and generated notes, and update the release creation logic without changing the surrounding publish gate or mode.outputs.publish condition.Source: Linters/SAST tools
src/telemetry/metrics.ts (2)
100-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the multi-counter flush for a single
appendVersionevent.
appendVersioncan trigger up to three separate synchronousflush()calls (sourceGraphVersions,descriptionsGenerated,embeddingsComputed) for what is logically a single event, tripling the SQLite writes on this pipeline hot path. Consider an internal batched increment (e.g. increment all applicable in-memory counters first, then flush once) to cut this to a single UPSERT perappendVersioncall.♻️ Proposed batching approach
incrementSourceGraphVersions(): void { this.counts.sourceGraphVersions += 1; - this.flush(); + this.flush(); } + + /** Batch several counter bumps behind a single flush. */ + incrementBatch(fn: (counts: MutableCounts) => void): void { + fn(this.counts); + this.flush(); + }if (prop === "appendVersion") { return (row: SourceGraphVersionRow): void => { target.appendVersion(row); - sink.incrementSourceGraphVersions(); - if (row.describeStatus === "described") sink.incrementDescriptionsGenerated(); - if (row.embedding !== null) sink.incrementEmbeddingsComputed(); + sink.incrementSourceGraphVersions({ + describeStatus: row.describeStatus, + embedding: row.embedding, + }); }; }Also applies to: 165-181
🤖 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 `@src/telemetry/metrics.ts` around lines 100 - 108, `appendVersion` is causing multiple synchronous `flush()` calls through `incrementSourceGraphVersions`, `incrementDescriptionsGenerated`, and `incrementEmbeddingsComputed`, which should be batched into one write. Update the `Metrics` counter helpers so they can perform internal in-memory increments without flushing immediately, then have the `appendVersion` flow flush once after all applicable counters are updated. Keep the change localized around the `Metrics` methods and the `appendVersion` call path so a single UPSERT is emitted per event.
110-135: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the prepared statement.
flush()re-prepares the same SQL text on every counter increment. Preparing aservice_metricsupsert once (as an instance field) and just calling.run(...)on subsequent flushes avoids repeated statement compilation on a frequently-hit path.🤖 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 `@src/telemetry/metrics.ts` around lines 110 - 135, The flush() method in Metrics re-prepares the same service_metrics upsert SQL on every call, which adds unnecessary overhead on a hot path. Move the prepared statement creation into a cached instance field on the Metrics class and have flush() reuse that prepared statement by calling .run(...) with the current counts and nowFn() result. Keep the existing flush() behavior and fail-soft catch, but ensure the cached statement is initialized once and reused across increments.
🤖 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 @.github/workflows/release.yaml:
- Around line 70-72: The release workflow currently grants publish-capable
permissions to the entire release job, so move the npm ci, typecheck, test,
build, and guard logic into a separate read-only gate job in release and keep
the privileged contents: write and id-token: write only on a short publish job.
Use the existing release job structure and related steps in the workflow to
split the build/validation path from the publish path, and have the publish job
consume the built artifact rather than rerunning the checks under elevated
permissions.
In `@src/telemetry/checkin.ts`:
- Around line 133-137: Wrap the health sampling in `TelemetryCheckin.start()`
and the timer-tick path inside the same fail-soft handling used by the rest of
the checkin flow, since `this.health()` can throw and currently escapes before
`scheduleNext()`. Update the `start()` and tick callback logic in
`TelemetryCheckin` so `writer.checkin(...)` still runs with a safe fallback or
guarded error handling, and always call `scheduleNext()` even when health
sampling fails.
In `@src/telemetry/db.ts`:
- Line 89: The telemetry directory setup in mkdirSync(dirname(dbPath), {
recursive: true, mode: 0o700 }) only applies permissions when the directory is
newly created, so existing installs may keep broader access. Update the db.ts
telemetry path initialization to explicitly enforce 0o700 on the parent
directory before/after mkdirSync, using the same dirname(dbPath) logic in the
telemetry DB setup so pre-existing directories are corrected too.
- Around line 90-94: Close the SQLite handle on initialization failure in
createTelemetry(): if DatabaseSync(dbPath) succeeds but db.exec("PRAGMA
journal_mode = WAL") or migrateSchema(db) throws, make sure the native db handle
is explicitly closed before returning null telemetry. Update the createTelemetry
flow around loadSqlite, new sqlite.DatabaseSync(dbPath), and migrateSchema so
the opened connection is cleaned up in the error path even when initialization
falls back.
In `@src/telemetry/logs.ts`:
- Around line 39-42: The bearer-token redaction pattern in SENSITIVE_PATTERNS is
too narrow and stops before common opaque token characters like +, /, and =.
Update the Bearer regex in logs.ts so it matches the full token payload after
the Bearer prefix, not just URL-safe characters, while keeping the existing
sensitive-field redactions unchanged. Use the SENSITIVE_PATTERNS constant as the
fix location and ensure the new pattern still works with the log-scrubbing logic
that consumes it.
---
Nitpick comments:
In @.github/workflows/release.yaml:
- Around line 229-235: The Create GitHub Release step in the release workflow
still uses the third-party softprops/action-gh-release action; replace that step
with a GitHub CLI-based release command using gh release in the same job. Keep
the existing tag_name/name/release-notes behavior equivalent by using
github.ref_name and generated notes, and update the release creation logic
without changing the surrounding publish gate or mode.outputs.publish condition.
In `@src/telemetry/metrics.ts`:
- Around line 100-108: `appendVersion` is causing multiple synchronous `flush()`
calls through `incrementSourceGraphVersions`, `incrementDescriptionsGenerated`,
and `incrementEmbeddingsComputed`, which should be batched into one write.
Update the `Metrics` counter helpers so they can perform internal in-memory
increments without flushing immediately, then have the `appendVersion` flow
flush once after all applicable counters are updated. Keep the change localized
around the `Metrics` methods and the `appendVersion` call path so a single
UPSERT is emitted per event.
- Around line 110-135: The flush() method in Metrics re-prepares the same
service_metrics upsert SQL on every call, which adds unnecessary overhead on a
hot path. Move the prepared statement creation into a cached instance field on
the Metrics class and have flush() reuse that prepared statement by calling
.run(...) with the current counts and nowFn() result. Keep the existing flush()
behavior and fail-soft catch, but ensure the cached statement is initialized
once and reused across increments.
🪄 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: 5e7dc2d9-c563-46aa-9a08-b881b29ef87d
📒 Files selected for processing (20)
.github/workflows/release.yamlpackage.jsonsrc/daemon.tssrc/hivedoctor-registry.tssrc/index.tssrc/registration/service.tssrc/telemetry/checkin.tssrc/telemetry/db.tssrc/telemetry/index.tssrc/telemetry/logs.tssrc/telemetry/metrics.tstest/daemon.test.tstest/hivedoctor-registry.test.tstest/telemetry/checkin-service.test.tstest/telemetry/checkin.test.tstest/telemetry/db.test.tstest/telemetry/integration.test.tstest/telemetry/logs.test.tstest/telemetry/metrics.test.tstest/telemetry/test-helpers.ts
Comment on lines
+70
to
+72
| permissions: | ||
| contents: write | ||
| id-token: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow="$(fd -p 'release.yaml' .github/workflows | head -n1)"
sed -n '70,134p' "$workflow"
echo "Package scripts that may run before publish:"
jq '.scripts' package.jsonRepository: legioncodeinc/hivenectar
Length of output: 3237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow="$(fd -p 'release.yaml' .github/workflows | head -n1)"
nl -ba "$workflow" | sed -n '1,220p'Repository: legioncodeinc/hivenectar
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow="$(fd -p 'release.yaml' .github/workflows | head -n1)"
sed -n '70,220p' "$workflow"Repository: legioncodeinc/hivenectar
Length of output: 7300
Split the release workflow into a read-only gate job and a minimal publish job.
id-token: write and contents: write are granted for the entire release job, so npm ci, typecheck, test, build, and the release guards all run with publish-capable permissions. Keep those steps in a separate read-only job and reserve the privileged permissions for a short publish job that consumes the built artifact.
🤖 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 @.github/workflows/release.yaml around lines 70 - 72, The release workflow
currently grants publish-capable permissions to the entire release job, so move
the npm ci, typecheck, test, build, and guard logic into a separate read-only
gate job in release and keep the privileged contents: write and id-token: write
only on a short publish job. Use the existing release job structure and related
steps in the workflow to split the build/validation path from the publish path,
and have the publish job consume the built artifact rather than rerunning the
checks under elevated permissions.
Comment on lines
+90
to
+94
| const sqlite = loadSqlite(); | ||
| const db = new sqlite.DatabaseSync(dbPath); | ||
| db.exec("PRAGMA journal_mode = WAL"); | ||
| migrateSchema(db); | ||
| return db; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the SQLite handle when initialization fails.
If PRAGMA journal_mode or migrateSchema() throws after new DatabaseSync(dbPath), createTelemetry() falls back to null telemetry but this native handle stays open. That can leave a stray descriptor/lock behind on the same DB path.
Suggested fix
mkdirSync(dirname(dbPath), { recursive: true, mode: 0o700 });
const sqlite = loadSqlite();
const db = new sqlite.DatabaseSync(dbPath);
- db.exec("PRAGMA journal_mode = WAL");
- migrateSchema(db);
- return db;
+ try {
+ db.exec("PRAGMA journal_mode = WAL");
+ migrateSchema(db);
+ return db;
+ } catch (err) {
+ try {
+ db.close();
+ } catch {
+ // ignore cleanup failures on the fail-soft path
+ }
+ throw err;
+ }
}📝 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 sqlite = loadSqlite(); | |
| const db = new sqlite.DatabaseSync(dbPath); | |
| db.exec("PRAGMA journal_mode = WAL"); | |
| migrateSchema(db); | |
| return db; | |
| const sqlite = loadSqlite(); | |
| const db = new sqlite.DatabaseSync(dbPath); | |
| try { | |
| db.exec("PRAGMA journal_mode = WAL"); | |
| migrateSchema(db); | |
| return db; | |
| } catch (err) { | |
| try { | |
| db.close(); | |
| } catch { | |
| // ignore cleanup failures on the fail-soft path | |
| } | |
| throw err; | |
| } |
🤖 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 `@src/telemetry/db.ts` around lines 90 - 94, Close the SQLite handle on
initialization failure in createTelemetry(): if DatabaseSync(dbPath) succeeds
but db.exec("PRAGMA journal_mode = WAL") or migrateSchema(db) throws, make sure
the native db handle is explicitly closed before returning null telemetry.
Update the createTelemetry flow around loadSqlite, new
sqlite.DatabaseSync(dbPath), and migrateSchema so the opened connection is
cleaned up in the error path even when initialization falls back.
Comment on lines
+39
to
+42
| const SENSITIVE_PATTERNS: readonly RegExp[] = [ | ||
| /\bBearer\s+[A-Za-z0-9._-]+/gi, | ||
| /"?(authorization|api[_-]?key|apikey|access[_-]?token|refresh[_-]?token|secret|password|client[_-]?secret)"?\s*[:=]\s*"?[^",}\s]+"?/gi, | ||
| ]; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Broaden bearer-token redaction beyond URL-safe characters.
/\bBearer\s+[A-Za-z0-9._-]+/ stops at +, /, or =, which are common in opaque/base64-ish tokens. A line like Bearer abc+/= would still persist part of the credential.
Suggested fix
const SENSITIVE_PATTERNS: readonly RegExp[] = [
- /\bBearer\s+[A-Za-z0-9._-]+/gi,
+ /\bBearer\s+[^\s"',}\]]+/gi,
/"?(authorization|api[_-]?key|apikey|access[_-]?token|refresh[_-]?token|secret|password|client[_-]?secret)"?\s*[:=]\s*"?[^",}\s]+"?/gi,
];📝 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 SENSITIVE_PATTERNS: readonly RegExp[] = [ | |
| /\bBearer\s+[A-Za-z0-9._-]+/gi, | |
| /"?(authorization|api[_-]?key|apikey|access[_-]?token|refresh[_-]?token|secret|password|client[_-]?secret)"?\s*[:=]\s*"?[^",}\s]+"?/gi, | |
| ]; | |
| const SENSITIVE_PATTERNS: readonly RegExp[] = [ | |
| /\bBearer\s+[^\s"',}\]]+/gi, | |
| /"?(authorization|api[_-]?key|apikey|access[_-]?token|refresh[_-]?token|secret|password|client[_-]?secret)"?\s*[:=]\s*"?[^",}\s]+"?/gi, | |
| ]; |
🤖 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 `@src/telemetry/logs.ts` around lines 39 - 42, The bearer-token redaction
pattern in SENSITIVE_PATTERNS is too narrow and stops before common opaque token
characters like +, /, and =. Update the Bearer regex in logs.ts so it matches
the full token payload after the Bearer prefix, not just URL-safe characters,
while keeping the existing sensitive-field redactions unchanged. Use the
SENSITIVE_PATTERNS constant as the fix location and ensure the new pattern still
works with the log-scrubbing logic that consumes it.
…missions, fail-soft checkin, db handle cleanup, chmod enforcement, broader bearer redaction - release.yaml: split into a read-only gate job (contents: read; npm ci, typecheck, test, build, tag/version guards, publish-mode resolution, dist artifact upload) and a minimal publish job that alone holds contents: write + id-token: write, downloads the gate-built dist, and publishes with --ignore-scripts so no repository code runs under the OIDC publish identity. - checkin.ts: wrap health sampling in try/catch in start() and the heartbeat tick so a throwing health() can neither block daemon boot nor permanently stop the heartbeat loop; scheduleNext() always runs. - db.ts: chmodSync the telemetry dir to 0o700 on non-Windows so a pre-existing dir from an older install is tightened too; close the SQLite handle when WAL/migrateSchema throws after open, so the fail-soft fallback never leaks a native descriptor/lock. - logs.ts: broaden the Bearer redaction regex beyond URL-safe chars so base64-ish opaque tokens (+, /, =) are fully redacted. - tests: fail-soft throwing-health-sampler coverage, bearer tokens with +/= chars, and pre-existing-dir chmod enforcement (POSIX-only, skipped on win32).
This was referenced Jul 4, 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
Part of the portal + telemetry realignment initiative driven by
/the-smoker. Extends hivenectar's registry writer to register with hivedoctor and check in (binding time, heartbeat, health) plus writes non-sensitive metrics and redacted logs to a local SQLite database, mirroring honeycomb's PRD-071 against the same hivedoctor ADR-0001/0002 contract.Security
security-worker-bee found one medium finding: the telemetry SQLite directory was created with default
umaskpermissions, potentially world-readable on multi-user systems. Remediated (3fb6f93):mode: 0o700added tomkdirSyncinsrc/telemetry/db.ts, matching honeycomb's existing precedent. New POSIX-only test asserts the permission bit (skipped on Windows, where the concept doesn't apply the same way).Quality
quality-worker-bee independently re-ran the suite and re-verified the permission fix. Verdict: PASS. One Warning carried over from the pre-existing PRD-006 gap: metrics collection isn't yet wired into the live daemon process (already tracked, not introduced by this PR).
Test plan
Note
This branch was authored after a short delay: another agent was actively working on
feature/prd-006-file-registration-completewith uncommitted changes when this initiative first tried to branch here, so this repo's PRD-017 work was deferred and picked up once that work completed, per the work-boundaries rule.Full acceptance-criteria ledger:
library/ledger/EXECUTION_LEDGER.mdin the-apiary superproject (companion PRs open there and in hivedoctor, honeycomb, the-hive).Summary by CodeRabbit
New Features
Bug Fixes
Tests