Skip to content

Suppress sink_plugin_not_active warnings on zero-plugin lifecycle/read-only commands#222

Open
philcunliffe wants to merge 1 commit into
masterfrom
fix/issue-219
Open

Suppress sink_plugin_not_active warnings on zero-plugin lifecycle/read-only commands#222
philcunliffe wants to merge 1 commit into
masterfrom
fix/issue-219

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Problem

Lifecycle and read-only CLI commands (daemon restart, daemon start, daemon stop, status, version) print one sink_plugin_not_active warning per configured sink on every invocation, even though decideBootProfile returns { 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 status reports the daemon healthy with those exact sinks) and would ship to every user, training them to ignore warnings.

Root cause

src/core/cli/dispatch.js ran materializeSinks(...) 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, so materializeRequest/materializeBlob (src/core/sinks/materialize.js) throw sink_plugin_not_active for every sink.

Fix

Gate the dispatch-path materializeSinks pass 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.

  • 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 (via daemon.sink_materialize_failed). Real signal preserved.
  • The config-profile path still warns when a declared sink names a writer/destination plugin that isn't enabled — a genuine misconfiguration.
  • Reworded the now-inaccurate sink_plugin_not_active hint (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 no sink_plugin_not_active, while a config-profile command (noop) over the same sink config still warns. Fails on master, passes with the fix. Full suite green (1670 pass), npm run build:types clean.

Fixes #219

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>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Dual-agent review — approve

  • Verdict: approve
  • Risk class: low
  • Auto-merge advisory: 👎 thumbs down — requires human merge gate

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

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/version still resolve to { activate: [] }, and dispatch now skips materializeSinks for that empty activation profile. The regression test covers status and 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 materializeSinks independently and logs daemon.sink_materialize_failed, so real daemon materialization failures are not hidden by the CLI dispatch gate.

Findings

No new issues found.

No Finding

  1. Behavioral Correctness
  2. Contract & Interface Fidelity
  3. Change Impact / Blast Radius
  4. Concurrency, Ordering & State Safety
  5. Error Handling & Resilience
  6. Security Surface
  7. Resource Lifecycle & Cleanup
  8. Release Safety
  9. Test Evidence Quality
  10. Architectural Consistency
  11. 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 direct dispatch() callers from hypaware-core/smoke/flows/*
  • Impacted tests: test/core/command-dispatch.test.js:468, test/core/command-dispatch.test.js:507, existing materializer contract tests in test/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 activate config.plugins[]). But the gate also lets the 'all-available' profile through (hyp init / no-arg hyp), and under all-available every installed plugin loads regardless of plugins[] — so the only way to reach sink_plugin_not_active there 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 materializeSinks pass for { activate: [] }, so a genuinely malformed sink that would raise sink_config_invalid (src/core/sinks/materialize.js) is also silenced on daemon/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_active kind rather than the whole pass. Not recommended for this PR.

Reports: /Users/phil/workspace/hypaware/.git/worktrees/tmp.mLKAjfoySc/dual-review/pr-222

@philcunliffe

Copy link
Copy Markdown
Contributor Author

🧭 Decision map — where to spend your attention

Companion 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 sinkWarningHint, and a straight two-branch regression test. The decisions worth your eyes, in order:

1. Gate on the profile's intent to activate plugins, not on how many actually loaded · unhappy-path policy

src/core/cli/dispatch.js:75

return bootProfile.activate.length > 0
  • Decision: Suppress the sink_plugin_not_active warning based on the boot profile declaring zero plugins ({ activate: [] }), i.e. the command's intent.
  • Alternative not taken: Gate on the resultactivePlugins.length === 0, or on whether each sink's own writer/destination plugin loaded. Either would also suppress the warning for a config-profile command that loads some plugins but declares a sink whose writer is genuinely disabled — a real misconfig that should still surface.
  • Check: Confirm this is the intended split: a config command with a mis-declared sink still warns; only the four lifecycle commands (daemon/status/smoke/version) go quiet. (The added test pins both halves.)

2. Skip the whole materializeSinks pass vs. suppress only the noisy error kind · unhappy-path policy

src/core/cli/dispatch.js:187

if (bootProfileActivatesPlugins(bootProfile)) {
  • Decision: For zero-plugin profiles, skip materialization entirely rather than run it and filter the output.
  • Alternative not taken: Still call materializeSinks and suppress only sink_plugin_not_active, preserving other error kinds (e.g. sink_config_invalid from materialize.js). Skipping the pass silences all sink-materialization errors on those four commands.
  • Check: Is silencing every sink error kind on daemon/status/smoke/version acceptable? (In practice yes — config-profile commands and config validation still surface a malformed sink — but it's the real fork here.)

3. Fail-safe fallthrough returns true (keep warning) for any non-{activate:[]} shape · unhappy-path policy

src/core/cli/dispatch.js:77

return true
  • Decision: 'config', 'all-bundled', 'all-available', and any unrecognized profile all fall through to "materialize + warn."
  • Alternative not taken: Default to false (suppress) for unknown shapes. The chosen direction errs toward noise over silently hiding a real misconfig.
  • Check: Correct default; new BootProfile variants added later inherit "warn," not "suppress."

Honorable mentions (real but lower-stakes): src/core/cli/dispatch.js:57 — the hint's meaning flips from "expected, ignore" to "misconfig, fix it"; slightly imprecise under the all-available profile (hyp init/no-arg), where the real fix is install the plugin, not add it to plugins[]. test/core/command-dispatch.test.js:557 — the assertion pins the residual sink_plugin_not_active contract for the config profile (the "still warn" half).

Generated by /decision-map. Advisory — directs attention, casts no verdict.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

🤖 neutral: reviewed (approve / low), green, mergeable — held for your merge

Fixes #219 (zero-plugin lifecycle/read-only commands spammed sink_plugin_not_active). Dual-review (Codex + Claude): approve, risk low, zero blocker/major.

  • Gate correctness verified: bootProfileActivatesPlugins suppresses only for { activate: [] } (the daemon/status/smoke/version profiles that load no plugins by design); config/all-* and any non-empty activate set still materialize + warn; unknown shapes fail safe toward warning.
  • Real signal preserved: the running-daemon materialization path (daemon/runtime.jsdaemon.sink_materialize_failed) is a separate call site, untouched — a genuine sink failure still surfaces. The gated dispatch-path materializeSinks discards its handles and only prints warnings, so suppression loses no behavior.
  • State: head 931cac8, MERGEABLE/CLEAN, CI green (1670 pass), tsc clean.

Two optional non-blocking nits (not fixed — your call): (1) the reworded hint "Add it to plugins[]" is imprecise for the all-available profile (hyp init/no-arg), where an uninstalled plugin is the only trigger — could broaden to "install/enable the plugin or remove the sink"; (2) gating the whole materializeSinks pass also silences other error kinds (e.g. sink_config_invalid) on those four commands — harmless, matches intent. Happy to tweak (1) if you want it.

Merge is yours — neutral never merges. Held.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suppress 'sink_plugin_not_active' warnings on zero-plugin lifecycle/read-only commands

1 participant