Fix #1299: Picker platforms values are not checked against the known process.platform set: a manifest typo silently withholds the row everywhere

[philcunliffe]

Feature or issue

A picker row's platforms gate is compared against process.platform by string equality, but nothing checks the declared values against the set process.platform can report. A plugin author's typo ("macos", "Darwin", "win") passes validatePickerContributions, loads, and then matches no platform, so the row is offered nowhere with no error and no diagnostic. The author's own machine gives no signal either, because the symptom is an absent row, which is what a row nobody wrote looks like.

Solution

• loadManifest now emits one WARN per offending row, manifest.picker_platform_unrecognized, naming the manifest path, the plugin, the row, and the unrecognized values, mirrored to stderr so it reaches an install with no telemetry configured.
• Validation is deliberately unchanged: a manifest rejection is fatal to the whole plugin, so a closed enum would trade one withheld row for a dead plugin and would misfire on any process.platform value Node adds later. LLP 0369 records that loosened-but-warned shape and is noted on LLP 0368's Extended-by: line.
• test/core/picker-platform-unknown-warning.test.js pins the warning: it fails on the pre-fix head (no line emitted) and passes after, asserting the plugin still loads, that a real gate and the bundled catalog stay silent, and that the line carries manifest, row, and value.

Code: +41 / -0 lines

Fixes #1299

[philcunliffe]

Review round 1 (head 11251e30)

Verdict: findings (2, both low). Both fixed and pushed; new head 2f2f2876.

The fix addresses the defect in #1299 as specified by its acceptance condition: an unrecognized platforms value now produces a visible diagnostic without killing the plugin, a test pins it, and the loosened-but-warned shape is recorded in a new LLP extending 0368.

Verification performed

• npm test on 11251e30: 5989 pass, 0 fail, 1 skipped (26s). Re-run after my fixes: identical, 5989 pass, 0 fail, 1 skipped. The unattributed failure a previous run reported did not reproduce in either run here.
• npm run typecheck: clean, before and after.
• The regression test genuinely fails without the production change. Reverting only src/core/manifest.js to origin/master and re-running test/core/picker-platform-unknown-warning.test.js gives # pass 2 / # fail 1: test 1 (a picker platforms value outside the known set warns on stderr and still loads the plugin) fails at line 77, the assertion that exactly one warning line was emitted. Restoring the file makes it pass. Not vacuous.
• node scripts/llp-numbers.js check: 1 LLP number minted against refs/remotes/origin/master, no collision.
• The warn is reachable on an install with no telemetry provider: getLogger('manifest', { mirrorStderr: true }) writes to process.stderr unconditionally in emit, beside the OTel emit rather than behind it (src/core/observability/logger.js:193-196), and the test reads the line off the real descriptor rather than off a provider. Adding a fifth per-site mirror is consistent with LLP 0329 #not-every-warn, which rejected blanket CLI mirroring and left per-site opt-in as the mechanism.
• The warn/reject reasoning holds. A validateManifest failure returns a FailedManifest and drops every contribution the plugin makes, so a closed enum would trade one withheld picker row for a dead plugin, and would misfire on any process.platform value Node adds later. Warning is the right side of that trade.

LLP convention check: pass

• llp/0368-picker-rows-declare-their-platforms.decision.md is +1 / -0: only the **Extended-by:** LLP 0369 ... forward-ref line was appended. Nothing else in the Accepted doc changed, which is exactly what CLAUDE.md permits.
• llp/0369's header carries Type: Decision, Status: Accepted, Systems: Plugins, Onboarding, Author, Date, Related:, Extends: LLP 0368, matching the sibling decisions' shape (compare 0368's own header).
• Every anchor cited resolves. @ref LLP 0369#warn-not-reject exists as ## Warn, do not reject {#warn-not-reject} (0369:23); #known-set and #problem and #consequences likewise. The Related: line's outbound anchors resolve too: LLP 0368 #platform-gate (0368:45), LLP 0130 #picker-block, LLP 0329 #stderr-mirror (0329:77). The test's LLP 0329#dark-substrate resolves (0329:40).

Findings

1. Low: KNOWN_PLATFORMS was missing three platforms that exist today, so a correct gate would have warned. src/core/manifest.js:95-98 (was 89-91 on 11251e30).

The set held the eight values the Node prose docs list. process.platform also reports netbsd, cygwin, and haiku right now; all three are in this repo's own NodeJS.Platform union (node_modules/@types/node/process.d.ts:281-292). A plugin author correctly gating a picker row to ["netbsd"] would get manifest.picker_platform_unrecognized ... which no platform reports, so the row is offered nowhere printed on every manifest load, on every hyp command, while the row rendered perfectly well. A false diagnostic on correct data is the one cost this warning must not pay, and LLP 0369 #known-set only budgets for "a platform Node adds later", not for three that already exist.

Fixed: the set is now the full 11-value union; llp/0369 #known-set says so and why; the quiet-case loop in the test now covers ['netbsd'].

2. Low: the diagnostic sat inside the try whose catch rejects the manifest. src/core/manifest.js:61 on 11251e30.

warnUnrecognizedPickerPlatforms was called inside the withSpan callback, which loadManifest wraps in try { ... } catch, and that catch converts anything thrown into { ok: false, errorKind: 'manifest_invalid' }. So a throw from the diagnostic would have rejected the whole plugin, with a message naming JSON/validation, which is precisely the outcome LLP 0369 #warn-not-reject exists to prevent. The seam is narrow but real: getLogger() calls logs.getLogger('hypaware.manifest') at src/core/observability/logger.js:158, outside the try that emit puts around otelLogger.emit (that inner guard is documented as being "for a globally installed provider that is not ours", and it does not cover the provider's getLogger).

Fixed: manifest is now a let assigned inside the try, the catch returns the failure, and the warn plus the success return sit after the try/catch, with a comment saying why. The invariant is structural rather than incidental.

Not findings, recorded for the reader

• The warning fires once per loadManifest, so a third-party plugin with a typo prints one line per row per hyp process rather than once ever. That is the intended nagging shape for a diagnostic aimed at the plugin author, and the bundled catalog is silent (pinned by the third test).
• LLP 0369 cites LLP 0329 under Related: rather than Extends:. Correct as written: it uses 0329's per-site mirror mechanism unchanged, it does not amend 0329's decision, so no forward-ref is owed there.
• Style rules are respected throughout: no semicolons, no em dashes, JSDoc types with @import at the top, no new dependencies.

Fixes pushed

2f2f2876 on fix/issue-1299. Verified by reading origin/fix/issue-1299 back after the push: KNOWN_PLATFORMS carries all eleven values at src/core/manifest.js:95-98; warnUnrecognizedPickerPlatforms(manifest, manifestPath) is after the closing brace of the catch; llp/0369 #known-set names the eleven; the test's quiet-case loop contains ['netbsd']. Full suite green on the pushed tree.

[philcunliffe]

Review round 2 (head 2f2f2876)

Verdict: findings (2, both low). Both fixed and pushed; new head ca6cd2ae.

Round 1's two fixes are correct and complete, and both of this round's findings are new material: one is a message-accuracy bug that was always there and neither round had looked at, the other is a consequence of round 1's own restructuring.

Round 1's fixes, re-checked

Fix 1 (the platform set) is exactly right, verified mechanically rather than by eye. I parsed KNOWN_PLATFORMS out of src/core/manifest.js and diffed it as a set against this repo's own bundled NodeJS.Platform union at node_modules/@types/node/process.d.ts:281-292 (@types/node 26.4.0):

KNOWN_PLATFORMS  (11): aix,android,cygwin,darwin,freebsd,haiku,linux,netbsd,openbsd,sunos,win32
NodeJS.Platform  (11): aix,android,cygwin,darwin,freebsd,haiku,linux,netbsd,openbsd,sunos,win32
IDENTICAL: true    extras: []    missing: []

Nothing extra, nothing missing. LLP 0369 #known-set (llp/0369:51-56) spells out the same eleven in prose, so doc and code agree literally, and the test's quiet-case loop covers ['netbsd'].

Fix 2 (the warn moved off the reject path) preserved every return shape. I did not take this on inspection, because a control-flow move is where a return path goes missing. I ran loadManifest on five directories against both origin/master's loader and the PR head's, and compared the results key by key:

case	master	head 2f2f2876
success, warning fires	ok:true keys [manifest,manifestPath,ok,rootDir]	identical
success, no warning	ok:true keys [manifest,manifestPath,ok,rootDir]	identical
invalid JSON	ok:false manifest_invalid keys [errorKind,manifestPath,message,ok,rootDir]	identical
invalid shape	ok:false manifest_invalid same keys	identical
missing file	ok:false manifest_invalid same keys	identical

Byte-identical on all five. manifest_invalid is the only member of ManifestErrorKind (src/core/types.d.ts:132), so there is no other kind to lose, and the let manifest hoist is definitely assigned because the catch returns rather than falling through.

Findings

1. Low: the warning claimed "the row is offered nowhere" for a gate that was only partly wrong. src/core/manifest.js:122 on 2f2f2876.

The detail string was built from unrecognized, but asserted something about the whole gate. For "platforms": ["darwin", "win"] (the author meant win32, and this is squarely the class of typo #1299 is about) the row still renders on macOS, and the line told its author it was offered nowhere. That sends them hunting for a row that is visible on their own machine, which is the confusion the diagnostic exists to remove. The "offered nowhere" claim only holds when unrecognized.length === platforms.length.

Fixed: the message now splits on exactly that condition. An end-to-end unrecognized gate keeps the original wording; a partial one gets picker row names <values> in its gate, which no platform reports, so the row is offered only where the rest of the gate matches. test/core/picker-platform-unknown-warning.test.js:85 pins it, and I confirmed it is not vacuous: against 2f2f2876's src/core/manifest.js that test is not ok 2, and the other three still pass.

2. Low: round 1's fix traded a contained failure for an uncontained one. src/core/manifest.js:82 on 2f2f2876.

Round 1 was right that the warn did not belong inside the try whose catch rejects the manifest. But moving it outside with no guard at all left it on no seam: loadManifest's documented contract is Promise<LoadedManifest|FailedManifest> and it has never rejected on the success path, so a throw from the diagnostic now escaped as a rejection instead. The seam is narrow but real, and it is the same one round 1 identified: logs.getLogger('hypaware.manifest') runs at src/core/observability/logger.js:158, ahead of the try that emit puts around the OTel call, and the mirror's process.stderr.write at logger.js:194 is the one step in emit with no guard around it at all. The blast radius is larger than the bug being avoided: loadManifests fans out over Promise.all (src/core/manifest.js:136) and src/core/runtime/installed.js:41 awaits it unguarded, so one throw would fail all of plugin discovery rather than the single plugin the original placement would have lost.

Fixed: the call now sits outside the manifest try and inside a try {} catch {} of its own, with a comment naming both hazards. LLP 0369 #warn-not-reject now holds on both sides: a diagnostic can neither reject the manifest nor reject the promise.

No LLP edit was needed for either. Neither changes a documented decision: finding 1 is message text the LLP does not specify (#warn-not-reject requires the line to name "the manifest path, the plugin, the row, and the unrecognized values", which it still does), and finding 2 makes the existing @ref LLP 0369#warn-not-reject [implements] more literally true rather than differently true.

Verification on the new head ca6cd2ae

• npm test: 5990 pass, 0 fail, 1 skipped (25.7s), up from 5989/0/1 by the one test added.
• npm run typecheck: clean.
• node scripts/llp-numbers.js check: 1 LLP number minted against refs/remotes/origin/master, no collision.
• llp/0368 is still +1 / -0 against master (git diff --numstat on the pushed tree), the Extended-by: forward-ref and nothing else, which is what CLAUDE.md permits on an Accepted doc.
• Style: no semicolons, no em dashes in either changed file.
• KNOWN_PLATFORMS is not a missed reuse. The only other platform list in src/ is ['darwin', 'linux'] at src/core/commands/daemon.js:502, which is a deliberately narrower thing (the platforms the daemon supports), not the set process.platform can report.
• claude-desktop's ["darwin"] is still the only platforms gate in the repo, so the third test is meaningful rather than vacuous, and discoverBundledPlugins does route through loadManifests to loadManifest.

Not findings, recorded for the reader

• The Related: lines cite LLP 0130#picker-block, LLP 0011#autodetect-vs-default and LLP 0139#macos-only, none of which exist as literal {#anchor} marks. Pre-existing and corpus-wide (0198, 0202, 0297 and 0368 itself all cite 0130#picker-block on master today), and ref-check validates @ref in source, not prose Related: lines. Not this PR's to fix.
• The warning still fires once per loadManifest rather than once ever, so a third-party plugin with a typo prints a line per offending row per hyp process. Intended for a diagnostic aimed at the plugin author; the bundled catalog is silent.
• No test was added for finding 2's guard. Proving it needs a globally installed throwing logger provider, and logs.setGlobalLoggerProvider is process-wide state that would leak into the rest of the suite. The guard is structural and reads as its own proof.

Fixes pushed

ca6cd2ae on fix/issue-1299. Verified by reading origin/fix/issue-1299 back after the push, not from the local tree: the try { warnUnrecognizedPickerPlatforms(...) } catch {} block sits after the catch's closing brace; the detail ternary branches on unrecognized.length === platforms.length; KNOWN_PLATFORMS still carries all eleven values; test/core/picker-platform-unknown-warning.test.js:85 contains the new case. Full suite green on the pushed tree.

[philcunliffe]

Triage at head ca6cd2ae

The review-round cap (2) is exhausted with the last review verdict findings, so this head arrives at triage. Reconstructing the finding ledger against the current head:

• Round 1 (head 11251e30) found 2 findings, both low; both were fixed and pushed in 2f2f2876.
• Round 2 (head 2f2f2876) found 2 new findings, both low; both were fixed and pushed in ca6cd2ae, the current head, and the round 2 review verified all four fixes against the pushed tree (full suite 5990 pass / 0 fail, typecheck clean, LLP number check clean).

Residual findings at ca6cd2ae: none. Every listed finding is resolved at this head; the remaining review remarks are explicitly recorded as not-findings (per-load warning cadence, prose Related: anchors that are pre-existing corpus-wide, the deliberate absence of a test for the structural guard). Verified by reading the head diff directly: KNOWN_PLATFORMS carries all eleven NodeJS.Platform values, the warn sits outside the manifest try inside its own try {} catch {}, the detail message branches on a fully versus partly unrecognized gate, and the four tests pin loading, wording, the quiet cases, and the bundled catalog.

No deferred-finding issues are needed. The PR can merge safely at this head.

[philcunliffe]

Ship risk: low

Who could be affected: Almost nobody. People setting up HypAware see exactly the same setup choices as before; only someone building their own plugin sees anything new.

What could happen:

• A plugin author whose setup file names an operating system that does not exist (a typo like "macos" for "darwin") now gets one warning line when the plugin loads, telling them their entry will not appear where they expected. That message is the whole point of the change.
• Nothing changes about which setup choices are offered, on any operating system.

Why this level: The change only adds a message. It does not add, hide, or move any setup option, does not touch anyone's recorded data, privacy, or access, and nothing about it is hard to undo. The worst realistic outcome is an extra line of text for a plugin that is already misconfigured.

What was checked: The riskiest question was whether that new message could, if it ever failed to print, stop every plugin from loading at once. It was proved it cannot: with printing deliberately broken, all plugins still loaded, and the same check fails when the safety guard is removed. The full test suite (5990 tests), the type check, and a real load of the built-in plugins all passed, with no warnings from any shipped plugin.

[philcunliffe]

What neutral was doing

Rung enqueue on PR #1304 (fix/issue-1299, head ca6cd2aee5f1bed9c82ad5b72f21d617a4b7e0d7).
The head is mergeable, green, reviewed clean (triage closed with zero residual findings), and
carries a ship-risk record of low e4 v1, within this repository's configured maxAutomerge: low.

This PR was not enqueued and has not been evicted. Neutral is holding it deliberately:
the two PRs that were enqueued just ahead of it (#1302 and #1300) were both evicted within
minutes for the repo-wide reason below, so enqueueing this one would be evicted identically and
would only burn CI minutes. It is held here so the same decision covers all three.

Why it cannot proceed

The merge queue is evicting PRs for failed_checks, but the checks did not fail on
their merits: the merge-group jobs are hitting the timeout-minutes: 5 cap in
.github/workflows/ci.yml, and GitHub reports a timed-out job as cancelled, which
fails the CI required gate job, which makes the queue evict the PR.

Evidence, two consecutive merge groups, both evicted:

merge group	job	duration	result
48e3ae71 (run 33813167719)	test (22)	5m16s	cancelled at the 5m cap
48e3ae71	test (24)	5m15s	cancelled at the 5m cap
561e0644 (run 33813613314)	test (22)	5m11s	cancelled at the 5m cap
561e0644	test (24)	5m09s	cancelled at the 5m cap
561e0644	typecheck (24)	5m11s	cancelled at the 5m cap

For comparison, the same jobs on ordinary pull_request runs of these very heads:

run	job	duration	result
33809390061 (cdf51d2a, PR 1302)	test (22)	1m21s	success
33808386811 (98c9a81a, PR 1300)	test	~1m30s	success
33811297276 (dd769988, PR 1309)	test (24)	2m16s	success
33809855762 (merge group 57438f44)	test (22)	1m13s	success

So the suite normally finishes in 1 to 2 minutes and merge groups succeeded as recently
as 57438f44 at 21:47. In these two groups every job ran roughly 3 to 4 times slower
(typecheck (22) alone took 3m00s against its usual well under a minute), which points
at the runner or npm i, not at the code under test. No test assertion failed: the logs
show only CI required: Process completed with exit code 1 after the jobs were killed.

This is repo-wide infrastructure, not a defect in this pull request. The change itself
is mergeable, green on its own head, reviewed clean, and assessed at ship risk low.

What it needs from you

A call on the merge-queue CI budget. The options, as neutral sees them:

1. Raise timeout-minutes for the test and typecheck jobs in
   .github/workflows/ci.yml (5 minutes leaves almost no headroom over a 1 to 2 minute
   suite, so any runner slowdown evicts every queued PR). This is the smallest change
   that unblocks landing.
2. Investigate why merge-group runs are 3 to 4 times slower than pull_request runs
   for the same tree, if the slowdown is itself the bug worth fixing.
3. Something else, for example landing these by hand this once.

Neutral has deliberately not re-enqueued anything: with the cap unchanged, every
enqueue is evicted the same way, so retrying would just loop and burn CI minutes.

How to unstick

Reply with a comment on this PR (or push to the branch); neutral monitors this thread and
will re-engage with your guidance on its next tick. If you fix the workflow timeout, say
so here and neutral will re-enqueue this PR.