docs(observability): disambiguate audit.ts from recordAuditEvent, document gate-check-publish audit decision#2933
Conversation
…ument gate-check-publish audit decision Two doc-only clarifications from a self-host observability audit: - src/selfhost/audit.ts's logAudit is a stdout-only logger for 4 queue- lifecycle events, not the durable audit_events DB writer -- a naming trap a future reader (including this repo's own prior audit) can easily fall into. Cross-references recordAuditEvent in db/repositories.ts. - The successful gate-check-run publish path writes to check_summaries but not audit_events. Documents this as an intentional design decision (check_summaries is the purpose-built canonical record for this high-frequency event; downstream merge/close/hold actions are already audited separately) rather than a gap needing a fix. No behavior change. The two other items originally flagged in this area are already resolved elsewhere: notification send/suppress/fail audit coverage was added by #2881 (notify-discord.ts's auditExternalNotification), and the observability exports flagged as possibly-dead (registerMetricMeta, DEFAULT_BUCKETS, resetMetrics, flushOpenTelemetry) turned out to all be legitimately in use (extensively by tests, or as internal defaults) or a standalone utility with a distinct purpose from shutdown -- none warrant removal.
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-04 05:56:52 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 4 non-blocking
Concerns raised — review before merging
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2933 +/- ##
=======================================
Coverage 96.05% 96.05%
=======================================
Files 259 259
Lines 28460 28460
Branches 10350 10350
=======================================
Hits 27338 27338
Misses 489 489
Partials 633 633
🚀 New features to boost your workflow:
|
Summary
Re-investigated all four sub-items originally filed under #2908 against current
main(it moved substantially since the audit that filed this issue). Two were already resolved by other merged work; two dead-export candidates turned out not to be dead on closer inspection. What's left is two doc-only clarifications:src/selfhost/audit.ts'slogAuditis a stdout-only logger for exactly 4 queue-lifecycle events (job_complete/job_dead/job_error/job_rate_limited), not the durableaudit_eventsDB writer — a naming trap that's easy to fall into (this repo's own prior audit did). Added a doc-comment cross-reference torecordAuditEventindb/repositories.ts.src/queue/processors.ts'srecordPublishedGateCheckSummary) writes tocheck_summariesbut notaudit_events. Documented this as an intentional decision rather than a gap:check_summariesis the purpose-built canonical record for this specific, very-high-frequency event (repo/PR/headSha/checkRunId/conclusion/detailsUrl), and the downstream actions the verdict triggers (merge/close/hold) are already fully audited elsewhere. A parallelaudit_eventsrow would roughly double that table's volume with no new queryable information.Already resolved by other work (verified, not touched here):
notify-discord.ts'sauditExternalNotification, called for every Discord/Slackcompleted/denied/erroroutcome.Not actually dead (correcting the original audit's classification after re-checking against current
main):registerMetricMeta/resetMetrics(src/selfhost/metrics.ts) — extensively used by test setup/teardown across dozens of test files;resetMetricsin particular is essential test-isolation infrastructure, not dead code.DEFAULT_BUCKETS(src/selfhost/metrics.ts) — used internally asobserve()'s own default parameter value; exported so a caller COULD override with a variant, not unused.flushOpenTelemetry(src/selfhost/otel.ts) — confirmedshutdownOpenTelemetrydoes NOT need to call it:NodeTracerProvider.shutdown()already flushes pending spans as part of its own OTel SDK contract, so this is a legitimate standalone "flush now without shutting down" utility, not a missing step.registerMetricMetagenuinely has zero production callers, meaning/metricscurrently emits no Prometheus HELP/TYPE lines for any metric — real, but authoring HELP text for every metric in the system is a much larger, separate undertaking than this issue's scope; not attempted here.)Resolves #2908. Part of the #1667 self-host review-stack roadmap.
Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run typecheckvitest runon tests touching both changed files (test/unit/queue.test.tsgate-check tests,test/unit/selfhost-sqlite-queue.test.ts,test/unit/selfhost-pg-queue.test.ts) — all passednpm run test:changed— 61 files / 1907 tests passed, 0 failednpm run actionlint/npm run test:workers/npm run build:mcp/npm run test:mcp-pack/npm audit/ui:*— not run locally; no workflow, worker-pool, MCP-package, or UI files touched. CI runs the full gate.If any required check was skipped, explain why:
test:coverage/test:cinot run locally — this diff is 13 added comment lines across 2 files, zero behavior change, so there is no new code path for Codecov's patch-coverage gate to evaluate.Safety