The no-preauth denylist misses every equivalent flag it does not name

[philcunliffe]

Fixes the three mechanically fixable items from the residual review findings of PR #667.
Items 4-7 are deliberately untouched; see "What this PR does not fix" below.

1. The no-preauth denylist is widened from two exact strings to a pattern (fixed)

test/core/client-launch-manifest.test.js pinned the most security-relevant decision in
#667 (LLP 0198 #no-preauth: the bundled launch never widens what a client may do) with
['--allowedTools', '--dangerously-skip-permissions'] checked by Array.includes. The
decision says "or any equivalent", and the equivalents are open-ended.

Now matched with /(-{1,2})(dangerously|allowedTools|permission-mode|full-auto|yolo|sandbox|ask-for-approval)/i
plus a short-form set for -a, which is too short to pattern-match without false hits.
Unanchored, so a flag embedded in a larger argument string is caught too: a false positive
here is a loud test failure someone reads, a false negative is a session that silently
starts pre-authorized.

The assert.ok(launch, ...) precondition is kept - without it, a client that lost its
launch block has no args to scan and the assertion passes vacuously.

A second test pins the denylist's own reach by example (the twelve forms above, plus six
benign arguments that must not trip it), so a later narrowing fails loudly rather than
silently reopening the gap.

These flag names appear only as string literals to assert the absence of. Nothing in
this PR passes any of them to anything.

Evidence: fails before, passes after

Mutation test. Inject a permission-widening flag into the bundled claude launch spec
(hypaware-core/plugins-workspace/claude/hypaware.plugin.json) and run both versions of
the test against it.

Injection A - "args": ["--permission-mode", "acceptEdits", "{prompt}"]

=== master (two-entry denylist) ===
ok 5 - the bundled CLI clients' launch args never carry a permission-widening flag
# pass 5  # fail 0          <- the gap: a pre-authorizing launch spec ships green

=== this PR (widened) ===
not ok 5 - the bundled CLI clients' launch args never carry a permission-widening flag
  error: 'claude launch args must not carry --permission-mode'
# pass 5  # fail 1

Injection B - "args": ["--dangerously-skip-permissions=true", "{prompt}"]
(an =true suffixed form of a flag master's list does name)

=== master (two-entry denylist) ===
ok 5 - the bundled CLI clients' launch args never carry a permission-widening flag
# pass 5  # fail 0

=== this PR (widened) ===
not ok 5 - the bundled CLI clients' launch args never carry a permission-widening flag
  error: 'claude launch args must not carry --dangerously-skip-permissions=true'
# pass 5  # fail 1

The injection was reverted; the plugin manifest is unchanged in this PR
(git diff --stat touches two test files only).

2. hyp ask --list doing a full status probe (skipped - inspected, no saving available)

Reported as mechanically fixable; on inspection it is not a saving, so the file is left
alone rather than churned.

runAsk hoists askableClients(ctx) and buildWalkthroughClientDescriptorMap() above the
--list early return. But the --list branch calls
resolveLaunchers({ clients, descriptors, env }) to decide the closing line
(writeSuggestedPrompts({ launchable })), and resolveLaunchers needs both: the
attached-client list from the status probe, and the descriptor map for the launch blocks.

All three branches of runAsk (--list, named question, menu) consume exactly the same two
values. Moving the awaits inside the branches would duplicate them three times and save
nothing on any path. There is also no observable behaviour to pin, so no regression test
could satisfy the fail-before/pass-after gate for it - only a call-counting stub, which would
pin an implementation detail rather than a contract.

If the latency of the probe on --list is itself the concern, the real change is to make
--list not report launchability at all, which changes its output and is a design call, not
a mechanical fix.

3. Stray duplicated docblock (fixed)

test/core/cli/wizard/index.test.js had two consecutive /** ... */ blocks. The first
("A catalog whose claude client declares a launch spec...") described launchableCatalog,
which is defined two functions further down, and sat above firstLookWithRows, which it does
not describe. The leftover is removed; the block that describes the following construct is
kept.

No regression test: this is a comment with no runtime behaviour, so the fail-before/pass-after
gate does not apply to it. Verified by the suite staying green.

Checks

• node scripts/run-tests.js: 3726 tests, 3722 pass, 0 fail, 6 skipped
• npx tsc -p tsconfig.json --noEmit: clean (exit 0)
• No semicolons, no em dashes in the diff; @ref LLP 0198#no-preauth verified against the
  live anchor in llp/0198, whose "or any equivalent" wording is what the widening
  implements

What this PR does not fix

Items 4-7 of #671 are untouched, on purpose. They are design calls, not defects: each has
more than one defensible answer, and a fix attempt would have to guess at a decision that
belongs to a human. The issue itself says so.

• 4. hyp ask "<question>" has no TTY check. src/core/commands/ask.js spawns with
  stdio: 'inherit' unconditionally, so in cron/CI/a git hook it can block until the client
  exits. Refusing on non-TTY and documenting the behaviour are both defensible; it needs to be
  a recorded decision either way.
• 5. hyp ask "q" silently picks launchers[0]. Settling it means adding a
  --client <name> flag, which is a CLI surface addition.
• 6. "Exactly one {prompt}" is documented but not enforced. The code's behaviour
  (some() + replaceAll in every element) is the better one, but LLP 0198 carries the same
  "exactly one" wording, so whether this is a mechanical doc fix or needs an extending LLP is
  the call to make - and LLPs are settled once Accepted.
• 7. resolveOnPath's platform seam is host-coupled. path.join uses the host separator
  regardless of the platform argument, and on win32 exts never includes ''. Whether to
  support win32 properly or drop the parameter is a scope decision.

This does not close the whole issue

Fixes #671 will close the issue on merge, but items 4-7 remain open work. They are not
resolved and they are not withdrawn. They need to be re-filed as their own issues (one per
decision, so each can be decided independently), or #671 reopened after this merges.

This matters because it is exactly how these findings were lost the first time: they were
handed back at the two-round review cap on #667 rather than filed, and merging #667 closed
the only artifact holding them. Please do not let the same thing happen twice.

Fixes #671

[philcunliffe]

Review round 1 - 9578b8b - verdict: findings (1 low, fixed in d302003)

The work is sound: the widened denylist is a genuine improvement, the fail-before/pass-after
claim reproduces independently, item 2's skip is correct engineering judgement rather than
laziness, item 3 removed the right block, scope is honest, and the PR body says what needs
saying about items 4-7. One demonstrated bypass remained, now closed.

The widened denylist covers every form #671 named

Each tested against the actual pattern: --permission-mode / =acceptEdits,
--dangerously-bypass-approvals-and-sandbox, --full-auto, --yolo, --sandbox /
=danger-full-access, --dangerously-skip-permissions / =true, --allowedTools,
--ask-for-approval, and -a - all denied. Bare operands (acceptEdits,
danger-full-access, never) pass on their own, which is correct: the flag they follow is
always caught, so the pair never ships.

Independently reproduced, by injecting into the claude manifest and running the PR's test
against git show origin/master:'s version of the same file:

injection	master's test	PR's test
--permission-mode acceptEdits	pass 5 / fail 0	fail 1
--dangerously-skip-permissions=true	pass 5 / fail 0	fail 1
--yolo, --full-auto, -a never	-	fail 1 each

The vacuity precondition is load-bearing, not decorative: deleting the claude launch block
yields fail 2 with claude should be launchable, not a silent pass.

FIXED (low) - -a=never and -anever bypassed the short-flag half

PREAUTH_SHORT_FLAGS = new Set(['-a']) was exact-match, so while -a never (two argv
elements) was caught, the attached-value forms walked straight past. Demonstrated both ways
against the same injection:

-a=never injected, exact-match Set   -> pass 6 / fail 0   <- bypass
-a=never injected, prefix regex      -> fail 1, "claude launch args must not carry -a=never"

Not hypothetical: codex parses with clap, which reads -a, -a=never, and -anever as the
same thing. Narrower than the gap this PR closes, so not a regression - but it is a hole in a
security assertion, and the fix is two lines in a file already open.

Fixed in d302003: PREAUTH_SHORT_FLAG = /^-a/, with -a=never and -anever added to the
deny-list test. Suite 3722 pass / 0 fail, tsc exit 0.

The over-match trade is deliberate and correct

The regex requires 1-2 dashes immediately before the stem, which kills the false positives
worth worrying about: prose containing "sandbox" passes, /var/sandbox/x passes (slash, not
dash), and {prompt}, --print, --model, -p, --cwd, --headless all pass.

It does over-match --no-sandbox, --sandbox-mode=read-only, --no-yolo,
--disable-full-auto, and hyphenated path segments - several of which are permission
narrowing. That is the right trade: the assertion's shape is "these strings must be absent
from two manifests we control", so a false positive is a red suite on a two-line manifest
change, read by the person who just made it. A false negative ships a pre-authorized session.
{prompt} is a placeholder, so no user prose ever reaches the scanned array. The second test
pins the reach in both directions (12 deny, 6 benign), so a later narrowing fails loudly.

Item 2's skip verified correct

runAsk hoists askableClients and buildWalkthroughClientDescriptorMap; the --list
branch calls resolveLaunchers({ clients, descriptors, env }) to compute launchable, and
resolveLaunchers (first_ask.js:147-170) iterates clients and looks each up in
descriptors - it needs both. All three branches consume the same two values, so pushing the
awaits down triples the call sites and saves nothing on any path. No saving was missed.

The only real saving would be dropping the attachment filter for --list, which flips a
PATH-present-but-unattached client from "not launchable" to "launchable" - a behaviour change
and a design call, correctly scoped out.

Items 3-5

Item 3 removed the right block: the deleted comment described launchableCatalog (two
functions down) while sitting above firstLookWithRows; the retained block accurately
describes what it now attaches to.

Scope is honest - git diff --stat is exactly two files, both under test/.
ask.js, first_ask.js, plugin_catalog.js, hypaware-plugin-kernel-types.d.ts, and
llp/0198 are all untouched, so items 4-7 were genuinely not guessed at.

The PR body states that Fixes #671 closes the issue while 4-7 remain open, that they are
"not resolved and not withdrawn", and recommends re-filing one-per-decision or reopening -
naming the reason, which is that these findings were lost exactly this way once already.
Accurate. That merge-time action still needs a human.

Gates

node scripts/run-tests.js → 3722 pass / 0 fail / 6 skipped. npx tsc --noEmit → exit 0.
No em dashes, no semicolons. @ref LLP 0198#no-preauth resolves and its "or any equivalent"
wording is exactly what the widening implements. Flag names appear only as string literals
asserted absent.

One cosmetic nit, not a finding: the body says "3726 tests" where the runner reports
# tests 3728; the pass/fail/skip figures are exact.

---

Correction: this record's marker first named d302003, the head at the moment it was posted, but the review examined 9578b8b. The marker records which head was reviewed, not which head is current, and conflating the two would assert that the -a= bypass fix in d302003 had been reviewed when it was written after the review and examined by nobody. The marker now names 9578b8b, and d302003 is correctly unreviewed pending round 2.

[philcunliffe]

Review round 2 - d302003 - verdict: clean

Round 1 examined 9578b8b and found the short-flag bypass. Neutral wrote the fix itself in
d302003 after that review, so this round exists to review a commit nobody had read.

The bypass is closed, verified independently in both directions

Injecting -a=never into the claude manifest's contributes.client.launch.args:

• at d302003 → subtest 5 fails: claude launch args must not carry -a=never
• git show 9578b8b:'s version of the test, same injection → all 6 subtests pass

So the pre-fix bypass was real and the fix closes it. Manifest reverted,
git status --porcelain clean afterwards.

/^-a/ breadth: broad on purpose, and sound

Verified against the live pattern: -a, -a=never, -anever, -abc, -all, -append,
-agent all match; '', '-', '--a', 'a' do not. No crash on the empty string or a
lone dash - the pattern needs literal characters at positions 0-1, so both degrade to a
clean false.

It would deny a hypothetical future -all or -append. That is the right trade here: the
assertion scans exactly two hardcoded manifests, both currently args: ["{prompt}"], so
there are zero live false positives, and a false positive is a loud test failure read by
whoever just edited the manifest - the same trade already accepted for the long-form pattern
and stated in the code comment. isPreauthFlag is a pure OR, so overlap between the two
patterns is harmless.

The ^ anchor is a deliberate and correct divergence from the long form: an unanchored
/-a/ would false-positive on --full-auto (which contains -auto).

One observation, not a finding: PREAUTH_SHORT_FLAG has no /i, so -A is not caught
while the long form is case-insensitive. That exactly preserves the original
Set(['-a']) behaviour, so it is not a regression introduced here, and neither bundled
client emits -A.

Deny-list reach still pinned both ways

-a=never and -anever are in the enumerated deny set alongside the ten long-form entries
and -a; the benign set ({prompt}, --print, --model, exec, -p, chat) all still
pass, none of them starting -a.

A correction to the scope check itself

Both the round-1 record and this round's first pass reported PR scope using
git diff origin/master..HEAD. That measure became wrong once master advanced. Master
has moved from 920182c (the merge-base) to f0a793f since this branch was cut, so that
diff now reports the divergence between two tips - it briefly read as 7 files, then 19 -
and none of those numbers is this PR's scope.

Measured correctly from the merge-base:

git diff --stat 920182c..HEAD
 test/core/cli/wizard/index.test.js       |  4 --
 test/core/client-launch-manifest.test.js | 70 ++++++++++++++++++++++++++++++--
 2 files changed, 67 insertions(+), 7 deletions(-)

git diff --name-only 920182c..HEAD -- src/ hypaware-core/ is empty. The PR is
test-only, exactly as its body claims, and items 4-7 of #671 are genuinely untouched. The
earlier "2 test files" figure was right; it was right by luck, because master had not yet
moved when it was taken.

Gates

node scripts/run-tests.js → 3722 pass / 0 fail / 6 skipped. npx tsc --noEmit → exit 0.
PR body still carries the "does not close the whole issue" section naming items 4-7.

Note: master has advanced, so this PR now needs a base merge before it can land, and the
merge-time action from the body still needs a human - re-file items 4-7 or reopen #671.