Suppress sink_plugin_not_active warnings on zero-plugin lifecycle/read-only commands#222
Suppress sink_plugin_not_active warnings on zero-plugin lifecycle/read-only commands#222philcunliffe wants to merge 1 commit into
Conversation
Lifecycle/read-only commands (`daemon`, `status`, `smoke`, `version`) boot
with `decideBootProfile` returning `{ activate: [] }`, so they deliberately
load no plugins. Dispatch nonetheless ran `materializeSinks` over every
configured sink, and with zero plugins active every sink failed with
`sink_plugin_not_active` — one guaranteed-noise warning per sink on every
invocation (e.g. `hyp daemon restart` warned about iceberg/central/parquet
while the running daemon was exporting through all three).
Gate the dispatch-path `materializeSinks` pass on the boot profile actually
intending to activate plugins. The warning now only fires on the `config`
profile, where a declared sink whose writer/destination plugin isn't enabled
is a real misconfiguration. The daemon's own materialization
(src/core/daemon/runtime.js) is untouched, so a sink that genuinely fails to
materialize in the running daemon still surfaces.
Reword the `sink_plugin_not_active` hint: the old "expected for read-only
commands" framing is now inaccurate since read-only commands no longer emit
the warning.
Regression test: a zero-plugin lifecycle command (`status`) emits no
`sink_plugin_not_active`, while a `config`-profile command over the same
config still warns. Fails on master, passes with the fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dual-agent review —
|
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Claude | minor — hint imprecise for all-available profile (dispatch.js:57) | Targets (sinkWarningHint) / Risks |
| Claude | nit — gate suppresses all sink error kinds on 4 lifecycle cmds (dispatch.js:187) | Targets (bootProfileActivatesPlugins) |
| Codex | no new issues; fix validations correct; daemon signal preserved | Direct callers (materializeSinks two-site split) |
Codex review
Fix Validations
Zero-plugin lifecycle/read-only commands emitted guaranteed sink_plugin_not_active warnings
- Status: correct
- Evidence:
src/core/cli/dispatch.js:73,src/core/cli/dispatch.js:187,src/core/cli/dispatch.js:424,test/core/command-dispatch.test.js:507 - Assessment:
daemon/status/smoke/versionstill resolve to{ activate: [] }, and dispatch now skipsmaterializeSinksfor that empty activation profile. The regression test coversstatusand also proves the same sink config still warns under a normal config-profile command.
Daemon sink materialization signal is preserved
- Status: correct
- Evidence:
src/core/daemon/runtime.js:419,src/core/daemon/runtime.js:425 - Assessment: The daemon runtime still calls
materializeSinksindependently and logsdaemon.sink_materialize_failed, so real daemon materialization failures are not hidden by the CLI dispatch gate.
Findings
No new issues found.
No Finding
- Behavioral Correctness
- Contract & Interface Fidelity
- Change Impact / Blast Radius
- Concurrency, Ordering & State Safety
- Error Handling & Resilience
- Security Surface
- Resource Lifecycle & Cleanup
- Release Safety
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Evidence Bundle
- Changed hot paths:
src/core/cli/dispatch.js:52,src/core/cli/dispatch.js:73,src/core/cli/dispatch.js:167,src/core/cli/dispatch.js:187,src/core/cli/dispatch.js:424 - Impacted callers:
bin/hypaware.js:45,src/core/cli/integration.js:102,src/core/cli/dispatch.js:345, smoke/test directdispatch()callers fromhypaware-core/smoke/flows/* - Impacted tests:
test/core/command-dispatch.test.js:468,test/core/command-dispatch.test.js:507, existing materializer contract tests intest/core/sink-materialize.test.js:246,test/core/sink-materialize.test.js:398,test/core/sink-materialize.test.js:418 - Unresolved uncertainty: I did not rerun the test suite in this read-only review pass; I relied on the supplied green test/build result plus targeted tracing.
Claude review
Claude review
Reworded sink_plugin_not_active hint is imprecise for the all-available boot profile
- Severity: minor
- Confidence: 82
- Evidence: src/core/cli/dispatch.js:57
- Why it matters: The new hint reads
sink '<x>' names a writer/destination plugin that isn't enabled in the active config. Add it to plugins[] or remove the sink. This is exactly right for the dominant'config'profile (ordinary commands activateconfig.plugins[]). But the gate also lets the'all-available'profile through (hyp init/ no-arghyp), and under all-available every installed plugin loads regardless ofplugins[]— so the only way to reachsink_plugin_not_activethere is a sink plugin that isn't installed at all, for which "Add it to plugins[]" won't help and "isn't enabled in the active config" is slightly off. Bites only on init/no-arg with a wholly-uninstalled sink plugin; the "or remove the sink" clause stays valid in every case. - Suggested fix: Broaden the actionable clause to cover the install case, e.g.
Install/enable that plugin (add it to plugins[]) or remove the sink. Optional; low severity.
Gate suppresses all sink error kinds (not only sink_plugin_not_active) on the 4 lifecycle commands
- Severity: nit
- Confidence: 80
- Evidence: src/core/cli/dispatch.js:187
- Why it matters: The gate skips the entire
materializeSinkspass for{ activate: [] }, so a genuinely malformed sink that would raisesink_config_invalid(src/core/sinks/materialize.js) is also silenced ondaemon/status/smoke/version. In practice this is harmless — config-profile commands and config validation still surface such errors, and suppressing sink chatter on these 4 diagnostic commands is precisely the PR's intent — so it is not a defect, only a scope note. - Suggested fix: None required. If ever desired, gate only the
sink_plugin_not_activekind rather than the whole pass. Not recommended for this PR.
Reports: /Users/phil/workspace/hypaware/.git/worktrees/tmp.mLKAjfoySc/dual-review/pr-222
🧭 Decision map — where to spend your attentionCompanion to the dual-review verdict. This casts no verdict — it points at the 3 forks where the author made a real choice, so you can skim the rest. Scanned: ~4 hunks across 2 files. Most is mechanical: a doc-comment rewrite on 1. Gate on the profile's intent to activate plugins, not on how many actually loaded · unhappy-path policyreturn bootProfile.activate.length > 0
2. Skip the whole
|
🤖 neutral: reviewed (approve / low), green, mergeable — held for your mergeFixes #219 (zero-plugin lifecycle/read-only commands spammed
Two optional non-blocking nits (not fixed — your call): (1) the reworded hint "Add it to Merge is yours — neutral never merges. Held. |
Problem
Lifecycle and read-only CLI commands (
daemon restart,daemon start,daemon stop,status,version) print onesink_plugin_not_activewarning per configured sink on every invocation, even thoughdecideBootProfilereturns{ activate: [] }for them — they deliberately load no plugins. With zero plugins active, every configured sink fails to materialize, so the warning is structurally guaranteed and carries no signal. It also contradicts the command's own output (hyp statusreports the daemon healthy with those exact sinks) and would ship to every user, training them to ignore warnings.Root cause
src/core/cli/dispatch.jsranmaterializeSinks(...)unconditionally after boot and printed a warning for every sink whose writer/destination plugin isn't active. For an{ activate: [] }boot profile no plugin is loaded, somaterializeRequest/materializeBlob(src/core/sinks/materialize.js) throwsink_plugin_not_activefor every sink.Fix
Gate the dispatch-path
materializeSinkspass on the boot profile actually intending to activate plugins (bootProfileActivatesPlugins): skip it entirely for the lifecycle/read-only{ activate: [] }profiles. Sink materialization only carries signal when the command intended to load the writer/destination plugins.src/core/daemon/runtime.js) is untouched, so a sink that genuinely fails to materialize in the running daemon still surfaces (viadaemon.sink_materialize_failed). Real signal preserved.config-profile path still warns when a declared sink names a writer/destination plugin that isn't enabled — a genuine misconfiguration.sink_plugin_not_activehint (it previously said "expected for read-only commands", which no longer emit it).Test
New regression in
test/core/command-dispatch.test.js: a zero-plugin lifecycle command (status) emits nosink_plugin_not_active, while aconfig-profile command (noop) over the same sink config still warns. Fails onmaster, passes with the fix. Full suite green (1670 pass),npm run build:typesclean.Fixes #219