[potterdigital]

Fix PR

Linked Issue

Fixes #5852

---

What was broken

The secret-read guard's WRAPPERS table (added in #5685) left out three value-taking options that sudo(8) and doas(1) document: sudo's -a/--auth-type and -c/--login-class, and doas's -a. The guard took the option's value as the command word, so sudo -c ls cat .env, sudo --auth-type echo cat .env, sudo -c timeout cat .env and doas -a ls cat .env were allowed (1.18.0 denied them), and the debugger-spawn check read sudo -c ls claude --agent gsd-debugger as an ls command.

What this fix does

Adds a and c to sudo's short set, --auth-type and --login-class to its long list, and a to doas's short set. sudo's entry also lists its no-value long options from sudo(8) (flags), and wrapperOptionWidth gives an exact match on one of them width 1 before it tries prefix matching, the way getopt_long prefers an exact match. Without that, the new valued --login-class would prefix-match sudo's no-value --login (-i), so sudo --login cat .env would take cat as a value and resolve .env as the command word. resolveCommand and its callers are unchanged. The header's documented gap ("a wrapper option taking a separate value the WRAPPERS table does not list") still describes unlisted options accurately, so it is left as is.

Root cause

wrapperOptionWidth gives an option not in the table a width of 1, so skipWrapperArgs stops at the option's value and resolveCommand returns it as the command word. When that word is in NON_READING_COMMANDS the segment is skipped, and timeout swallows cat as its DURATION.

Testing

How I verified the fix

The issue's reproduction script, run against the hook on next 3730c67 and on this branch (Node 24.21, Linux):

                      next 3730c670        this branch
sudo -u root cat .env          2 secret-read   2 secret-read
doas -u root cat .env          2 secret-read   2 secret-read
sudo -c staff cat .env         2 secret-read   2 secret-read
sudo -n cat .env               2 secret-read   2 secret-read
sudo -c ls cat .env            0               2 secret-read
sudo --login-class ls cat .env 0               2 secret-read
sudo -a echo cat .env          0               2 secret-read
sudo --auth-type echo cat .env 0               2 secret-read
sudo -c timeout cat .env       0               2 secret-read
doas -a ls cat .env            0               2 secret-read
sudo -c ls claude --agent gsd-debugger  0      2 debugger-cli-spawn
sudo -c staff ls .env          2 secret-read   0
sudo -n ls .env                0               0
sudo -E ls .env                0               0

The --login rows (base next 3730c67, the first revision 361c66f, this branch bfa3c49):

                                     next   361c66f5   bfa3c49b
sudo --login cat .env                2      0          2 secret-read
sudo --login ls .env                 0      0          0
sudo --login echo .env | xargs cat   2      0          2 secret-read
sudo -c staff echo .env | xargs cat  2      2          2 secret-read

sudo -c staff ls .env flips to allow because -c now consumes staff and the command is ls, which matches the verdict for a bare ls .env. Base also allowed sudo -c staff bash -c 'cat .env' (the nested-shell route failed open for any value, not only non-reading names); this branch blocks it.

Regression test added?

• Yes. tests/gsd-secret-read-guard.test.cjs: a new #5852 block (the six commands from the issue, attached-value forms -cls/-als, the nested-shell route, the still-blocked controls, the allows that show the option consumes exactly one word and that -n/-E/-H still resolve to the next word, and the debugger-spawn case). The bug(hooks): the secret-read guard loses a nested shell behind a flagged or unlisted wrapper — timeout 5 bash -c 'cat .env', nice -n 5 bash -c …, env -i bash -c … are allowed #5627 wrapper list takes sudo -c staff, sudo --login-class staff, sudo -a bsdauth, sudo --auth-type bsdauth and doas -a passwd, so its property test stacks them too. It also pins sudo's no-value --login (sudo --login cat .env, sudo --login bash -c 'cat .env', sudo --login -u root cat .env blocked; sudo --login ls .env allowed; sudo --login and sudo --login -u root in the bug(hooks): the secret-read guard loses a nested shell behind a flagged or unlisted wrapper — timeout 5 bash -c 'cat .env', nice -n 5 bash -c …, env -i bash -c … are allowed #5627 wrapper list) and the xargs-pipe consumer, which runs resolveCommand on the upstream segment (sudo -c staff echo .env | xargs cat, sudo --login echo .env | xargs cat). On this branch the file passes 669/669. With the next hook swapped in, 18 fail; with the first revision's hook (361c66f), 6 fail, all of them the --login rows.

Platforms tested

• macOS
• Windows (including backslash path handling)
• Linux
• N/A (not platform-specific)

Runtimes tested

• Claude Code
• Antigravity
• OpenCode
• Other: ___
• N/A (not runtime-specific): synthetic PreToolUse payloads fed to the hook on stdin

---

Self-test report

{
  "schema": 1,
  "head_sha": "7fe59cf85d0b5c0626207618a83121747b74c66f",
  "tool": "npm-test",
  "failing_first": {
    "test": "tests/gsd-secret-read-guard.test.cjs",
    "fails_on_base": true,
    "passes_on_head": true
  },
  "commands": [
    "node --test tests/gsd-secret-read-guard.test.cjs",
    "npx eslint hooks/gsd-secret-read-guard.js tests/gsd-secret-read-guard.test.cjs",
    "Tests workflow (.github/workflows/test.yml) on this PR at this head: npm run test:coverage:unit:raw -- --shard 1/3, 2/3, 3/3, plus test:integration, test:security, test:install, test:slow"
  ],
  "full_suite": {
    "command": "Tests workflow on this PR at 7fe59cf8, full-scope lanes test (ubuntu-latest, 24, shard 1/3..3/3): https://github.com/open-gsd/gsd-core/actions/runs/42143668096",
    "result": "passed",
    "passed": 39689,
    "failed": 0
  },
  "environment": {
    "os": "GitHub Actions ubuntu-latest (non-WSL); same run green on the windows-latest and macos-latest conformance lanes. Failing-first check: Linux container (node:24-bookworm)",
    "node": "v24.21.0"
  },
  "output_excerpt": "7fe59cf8 is bfa3c49b merged with next f3f3a182; the merge touches none of this PR's files. Run 42143668096 on 7fe59cf8: every job green (3 full ubuntu shards, targeted ubuntu lane, lint-tests, plugin-validate, QA loop walk, coverage gate, 3 windows + 1 macOS conformance lanes, Required tests). Summed over the full ubuntu shards: # tests 39795 / # pass 39689 / # fail 0 / # skipped 105 / # todo 1. tests/gsd-secret-read-guard.test.cjs locally on this head: 669/669; with next f3f3a182's hook swapped in: 651/669 (18 fail). eslint clean on both files."
}

---

Checklist

• Issue linked above with Fixes #NNN
• Linked issue has the confirmed-bug label
• Fix is scoped to the reported bug: no unrelated changes included
• Regression test added
• All existing tests pass: the full-scope Tests workflow on ubuntu-latest at this head is green (see the self-test report)
• .changeset/ fragment added
• No unnecessary dependencies added

Breaking changes

None. The only verdict that loosens is sudo -c <class> <non-reading command> <secret> (and the -a forms), which now matches the bare command's verdict.

[trek-e]

Summary

This PR fixes #5852 by declaring sudo's -a/--auth-type and -c/--login-class, and doas's -a, as value-taking options in the secret-read guard's WRAPPERS table. The fix site and short-option set are right, and the tests cover every acceptance criterion. However, the new long entry --login-class opens a new bypass through the table's prefix matching: sudo --login cat .env was denied before this PR and is allowed after it. The required Self-test report check is also red. Changes are requested.

Classification & gate compliance

• #5852 is labelled confirmed-bug, so this is the Bug-Fix Track. The PR body links it with Fixes #5852.
• The branch is fix/5852-… and the commits are conventional.
• The changeset tidy-birds-tumble.md is Fixed with pr: 5874.
• The branch is behind next. This repo merges through the merge queue, so that is not a finding. It is mergeable.
• CI on 361c66f:
  • Self-test report fails with fail_suite_not_passed (Blocker 1).
  • Reviewed or maintainer is waiting on this review.
  • 9 fork runs are awaiting approval and have not been released, because this review has findings above Nit.
• There is no competing PR for #5852. #5649 edits the same resolveCommand region but not the table.

Root-cause verdict

The root cause is correct. On next, sudo's short set (CDghpRrTtUu) lacks a and c, and its long list lacks --auth-type and --login-class. doas's short set (Cu) lacks a. As a result:

1. wrapperOptionWidth (hooks/gsd-secret-read-guard.js:886) gives those options width 1.
2. skipWrapperArgs stops at the option's value.
3. resolveCommand returns that value as the command word.
4. findSecretRead then skips the segment when that word is a non-reading command.

The table edit is the right fix site. I reproduced this by reading the code on base and head. I did not run the hook.

Blast radius

• get_impact on resolveCommand (next declaration, upstream, depth 2): MEDIUM, 9 affected, all in hooks/gsd-secret-read-guard.js. Direct callers: reconstructedScript, findSecretRead, findDebuggerCliSpawn and scanXargsPipe. At depth 2: scanGitConfig, reconstructedProcsub, findSecretReadUnderOtherReadings and scanShellInterpreter.
• Consequence: every consumer that resolves a command word inherits the new widths, including the bug in Major 1.

Memtrace Evidence

• Index state: stale. The graph is at 894a8c0 and next is at 4dce9e8. Every line reference below was checked against the PR checkout.
• detect_changes (all 3 changed-file blocks):
  • The guard: WRAPPERS at :165-177.
  • The test file: the new describe closures.
  • The changeset: no symbols, because it is not indexed.
• find_cross_module_issues (all 3 blocks): 0 on the tests and changeset blocks. On the guard block it returned 1 issue, inconsistent_type_consumption on a generic .length in scripts/changeset/new.cjs:45. That is a false positive: the file is not in the diff, and the symbol is the built-in length property.
• find_code_review_issues (all 3 blocks, review_mode: online, max_candidates: 100): the same single false positive, otherwise 0.
• find_yaml_rule_matches (all 3 blocks): 0.
• find_ast_review_issues: not run, because the diff has no .py files.
• review_github_pr (post: false): 0 comments.
• get_symbol_context on WRAPPERS: a constant at :165-177. It has one reference, the WRAPPERS.get lookup in resolveCommand (:930), and belongs to community AssertPathTest.
• get_impact on resolveCommand: MEDIUM, 9 affected, as above.
• recall_decision: returned unavailable, because the MemCortex IPC circuit is open. The recorded-decision gate did not run.
• governing_rules on hooks/gsd-secret-read-guard.js: governed: false.

Laws that apply

Subject: the guard's wrapper table now declares sudo -a/-c (and their long forms) and doas -a as value-taking.

• postels-law
  • Fired: WRAPPERS is a parser table. Any option it misclassifies fails open, and the long list is read with prefix matching (:890).
  • Action: new long entries must not prefix-collide with a no-value option (Major 1).
• kerckhoffs-principle
  • Fired: the guard has to stay safe with its table public. sudo --login cat .env is a public bypass created by this edit.
  • Action: fix Major 1 and pin it with a test.
• hyrums-law
  • Cleared: the new denials restore 1.18.0's behaviour. The allow cases (sudo -c staff ls .env, -n, -E, -H) are pinned.

Considered and cleared:

• kernighans-law: a two-line data change.
• knuths-optimization-principle: no performance change.
• leaky-abstractions: no new abstraction.

Findings

Blocker

1. The required Self-test report check is red. Your report records full_suite.result: failed (48,602 passed, 4 failed). None of the failures are in files this PR touches:

   • 2 failures (verify-work-auto-transition, ship-phase-complete-gate): the container has no jq.
   • 1 failure (helpers-cleanup): the clone sat under /tmp.
   • 1 failure: the #5100 win32 install probe. That failure on WSL hosts is now filed as #5888.

   This PR still needs a self-test report with a passing full suite before it can merge. Run it with jq installed, from a clone outside /tmp, on a non-WSL host or after #5888 is fixed.

Major

1. --login-class makes sudo --login cat .env read the secret. This comes from the isolated adversarial pass, and I checked it against the source.
   • Where: wrapperOptionWidth (hooks/gsd-secret-read-guard.js:890) matches long options by prefix: (spec.long || []).some((l) => l.startsWith(word)).
   • How it fails: sudo's --login (the long form of -i) takes no value. With --login-class added to the long list, '--login-class'.startsWith('--login') is true, so --login now gets width 2. In sudo --login cat .env, cat is consumed as its value and .env becomes the command word, so the read is allowed.
   • On next: --login has width 1, cat is the command, and the command is denied.
   • Fix: a word that exactly names an option sudo documents as taking no value must not prefix-match a longer valued one. Either check an exact match against a no-value list first, or only prefix-match when the word is not itself a complete documented long option.
   • Test: add sudo --login cat .env (blocked). Also add sudo --login bash -c 'cat .env' (blocked), since this flag is in common use.

Minor

1. The new options are tested through only one shared consumer. resolveCommand feeds reconstructedScript, scanXargsPipe and findDebuggerCliSpawn, but only the debugger-spawn check gets a dedicated test. Add one xargs-pipe case (for example echo .env | xargs sudo -c ls cat) so the other consumers are pinned too.

Considered and cleared

• Short-option completeness: after this PR, sudo has -a -C -c -D -g -h -p -R -r -T -t -U -u and doas has -a -C -u. That matches the synopses quoted in #5852. The adversarial pass did not re-fetch the man pages.
• Attached and clustered forms: -cls, -nc ls and -Ec ls resolve correctly through the loop at :893-895. --opt=value returns width 1 (:888).
• --a now matching --auth-type: real sudo rejects --a as ambiguous (it also has --askpass), so nothing runs. No action.
• Security pass and code-review pass: both clean. They did not test the --login interaction.

Verdict

Changes requested: 1 Blocker, 1 Major, 1 Minor. The table entries are right, but the long-option prefix match turns --login-class into a new bypass for sudo --login. Fix that and add a test for it, then provide a passing Self-test report.

[potterdigital]

Pushed bfa3c49 for the review.

Major 1 (sudo --login swallowing the next word). sudo's entry now also lists its no-value long options from sudo(8) (flags), and wrapperOptionWidth returns width 1 for an exact match on one of them before it tries prefix matching. getopt_long does the same: an exact match wins. Hook verdicts (exit code) on base next 3730c67, on the first revision 361c66f, and on bfa3c49:

                                     next   361c66f5   bfa3c49b
sudo --login cat .env                2      0          2 secret-read
sudo --login ls .env                 0      0          0
sudo --login echo .env | xargs cat   2      0          2 secret-read
sudo -c staff echo .env | xargs cat  2      2          2 secret-read

New tests: sudo --login cat .env, sudo --login bash -c 'cat .env' and sudo --login -u root cat .env are blocked, sudo --login ls .env is allowed, and sudo --login / sudo --login -u root are added to the #5627 wrapper list. With 361c66f's hook, 6 of the file's 669 tests fail (all of them --login rows). With next's hook, 18 fail. On bfa3c49, 669/669 pass.

Minor 1 (xargs consumer). echo .env | xargs sudo -c ls cat is already denied on next, so it can't pin this change. scanXargsPipe takes xargs's own sub-command word as is, and sudo isn't a non-reading command. It calls resolveCommand only on the upstream segment, so the new tests put the wrapper there: sudo -c staff echo .env | xargs cat and sudo --login echo .env | xargs cat. The second one fails with 361c66f's hook.

Blocker 1 (Self-test report). I ran the full suite on a non-WSL host: this repo's Tests workflow, dispatched on the fork at bfa3c49: https://github.com/potterdigital/gsd-core/actions/runs/41904702557. Every job is green, including the three full-scope ubuntu shards, the windows and macOS conformance lanes, the coverage gate and Required tests. Across the full ubuntu shards: 39,968 tests, 39,856 passed, 0 failed, 111 skipped, 1 todo. The PR body's self-test report is updated to bfa3c49, and scripts/self-test-report-policy.cjs evaluates the new body to ok_report.

[trek-e]

Summary

This re-review covers bfa3c49b, which answers my CHANGES_REQUESTED review of 2026-11-04. All three items are fixed:

• Major 1 (sudo --login): sudo's entry now lists its no-value long options in flags. wrapperOptionWidth returns width 1 for an exact match on one of them before prefix matching, which is getopt_long's own exact-match rule. sudo --login cat .env is denied again. It is pinned along with sudo --login bash -c 'cat .env', sudo --login -u root cat .env, sudo --login ls .env (allowed), and the xargs-upstream form.
• Minor 1 (other consumers): you were right that scanXargsPipe takes xargs's own sub-command word as is and resolves only the upstream segment (hooks/gsd-secret-read-guard.js, scanXargsPipe). The new upstream-wrapper rows pin the path that actually runs.
• Blocker 1 (Self-test report): the report now records the full-scope Tests workflow green at this head, and Self-test report passes.

The fix restores 1.18.0's verdicts for every command #5852 lists, and I found no new bypass.

Classification & gate compliance

• Linked issue: Fixes #5852, OPEN and labelled confirmed-bug, so this is on the Bug-Fix track.
• Merge state: merges cleanly into next. Being behind next is not a finding, because next uses a merge queue.
• Format: Conventional Commits, and a Fixed changeset with pr: 5874 and a bold, symptom-led lead.
• Competing PRs: none.
• CI: the fork runs were held at action_required. I released them after the review found nothing above Nit, and watched them complete green on this head before posting this review.

Root-cause verdict

The root cause is confirmed against the #5852 triage. WRAPPERS (hooks/gsd-secret-read-guard.js:165-170) lacked sudo's -a/--auth-type and -c/--login-class and doas's -a. wrapperOptionWidth therefore gave those options width 1, and resolveCommand returned the option's value as the command word.

The table entries are the right fix site. The flags exact match closes the one prefix collision the new --login-class entry creates.

Anti-paper-over gate: passes.

Blast radius & risk tier

• get_impact on wrapperOptionWidth (upstream, depth 3): MEDIUM, 6 affected, all in hooks/gsd-secret-read-guard.js. The chain is skipWrapperArgs → resolveCommand → reconstructedScript, findSecretRead, findDebuggerCliSpawn and scanXargsPipe.
• Every consumer that resolves a command word inherits the corrected widths. The debugger-spawn and xargs consumers are pinned by the new tests.
• Risk tier: the changed symbols are not in the sweep's top-25 central or bridge symbols.

Co-change completeness

The hook and its test file change together, and the changeset type is Fixed, so no docs update is required. The triage note about the header's accepted-gaps wording still reads accurately after this change.

Memtrace Evidence

The graph is at 894a8c09e, behind next, so graph answers are lower bounds, and line references were checked against the PR checkout.

• get_impact — target wrapperOptionWidth (hooks/gsd-secret-read-guard.js): MEDIUM, 6 affected (skipWrapperArgs, resolveCommand, reconstructedScript, findSecretRead, findDebuggerCliSpawn, scanXargsPipe).
• get_symbol_context — target wrapperOptionWidth: one caller, skipWrapperArgs (:912), community AssertPathTest.
• recall_decision — query "secret-read guard wrapper option table sudo doas value options prefix matching": MemCortex unavailable (MemCortex local IPC deadline elapsed), so I could not check for a recorded decision. governing_rules on the hook returned governed: false.
• find_code_review_issues — run with review_mode: online and max_candidates: 100 on all 3 changed file blocks. It raised one raw issue, inconsistent_type_consumption on length declared in src/fallow-runner.cts with consumer scripts/changeset/new.cjs:45. That is not a defect: the diff touches neither file.
• detect_changes, find_cross_module_issues and find_yaml_rule_matches — run on all 3 blocks. The only hit was that same length item.
• review_github_pr (post: false, reviewMode: online, minSeverity: low): 0 comments.
• There are no .py files, so find_ast_review_issues does not apply.

Laws that apply

Subject: the secret-read guard's WRAPPERS table now declares sudo -a/-c (and their long forms) and doas -a as value-taking, and wrapperOptionWidth gives an exact no-value sudo long option width 1 before prefix matching.

• postels-law — fired: WRAPPERS is a parser table, and an option it misclassifies fails open. Action: none. It is already honored: an exact match on a documented no-value option now wins over a longer valued option it prefixes, which is getopt_long's own rule. A unique abbreviation (--login-c) still prefix-matches.
• kerckhoffs-principle — fired: the guard has to stay safe with its table public, and the first revision's sudo --login cat .env bypass was a public one. Action: none. It is already honored: the bypass is closed and pinned.
• hyrums-law — fired: the guard's verdicts change. Action: none. It is already honored: the new denials restore 1.18.0's behaviour, and the allow cases are pinned.

Considered and cleared:

• kernighans-law: one guarded early return plus table data.
• knuths-optimization-principle: no performance change.
• leaky-abstractions: no abstraction is introduced or bypassed.

Action items: none.

Findings

None.

Considered and cleared

• The abbreviations --lo, --log and --logi still prefix-match --login-class, giving width 2. The isolated adversarial pass raised this. Each of them also prefixes --login, a no-value option with a different has_arg, so getopt_long reports the word as ambiguous and sudo exits without running anything. The hook's verdict on a command that never runs is not a bypass. This is the same reason my earlier review cleared --a.
• No parity test between long and flags, and no new fast-check property. The Standards and adversarial passes raised this. The two lists are complementary, not parallel copies: flags exists to break the one prefix collision, and no valued long option equals a flag. The existing property "any wrapper stack in front of bash -c "cat .env" is blocked" draws its stack from the wrappers list this PR extends ('sudo -c staff', 'sudo --login', 'doas -a passwd' and others), so the new entries are already property-covered.
• The =-attached and clustered forms (--login-class=staff, sudo -ic ls) are not pinned in the new describe. They go through the generic paths (:888 returns width 1 for any --opt=value, and the cluster loop is shared by every wrapper) that this PR does not change.
• The generic spec.flags branch is populated only for sudo. It solves a concrete collision, and other wrappers fall through unchanged.
• /security-review: the change narrows what the guard allows, with no new input surface, and found nothing at confidence ≥ 8.

Verdict

Approved. CI was released and completed green on bfa3c49b before this review was posted.

[trek-e]

@potterdigital this is approved, and it merges as soon as one thing is fixed. That thing comes from me, not from your change.

To refresh a stale Reviewed or maintainer run, I updated this branch with next on the server. That added merge commit 7fe59cf8. The required Self-test report check now fails with fail_stale_sha, because the report in the PR body still says head_sha: bfa3c49b….

Please update the report to the current head 7fe59cf85d0b5c0626207618a83121747b74c66f. The full Tests matrix is running on that commit now, so you can use its run for the full_suite counts. No code change is needed. I'll merge once the report is current and CI is green.

[potterdigital]

@trek-e The self-test report in the PR body now names head 7fe59cf85d0b5c0626207618a83121747b74c66f. Its full_suite uses this PR's Tests run on that commit, 42143668096: all jobs green, 39,689 pass / 0 fail across the three full ubuntu shards.

• The re-run Self-test report check on 7fe59cf passes: run 42147549257. The red run in the list (42143662099) is the earlier one that read the stale bfa3c49b report.
• On 7fe59cf, tests/gsd-secret-read-guard.test.cjs passes 669/669. With next f3f3a18's hook swapped in, 18 of 669 fail. eslint is clean on both files.

No code change.