fix(kindle-dedrm): compare firewall rule state against 'True', not truthiness - #3396
Conversation
…uthiness Get-NetFirewallRule's Enabled property is a NetSecurity enum whose members are True = 1 and False = 2, so BOTH are non-zero and BOTH coerce to boolean $true. Confirmed against a live disabled rule on Windows 11 / PowerShell 7.6.5: [int] is 2 and [bool] is True. The enable action's `if (-not $rule.Enabled)` was therefore always false: a disabled Kindle-blocking rule was reported "already enabled" and never re-enabled, which is exactly the state the skill exists to prevent. The disable action's `if ($rule.Enabled)` had the mirror fault and called Disable-NetFirewallRule against rules that were already disabled. Both now compare against 'True', which holds for the enum, for an already-stringified property, and for a plain [bool]. Show-State's `$enabled = $rule.Enabled` was audited under the same suspicion and left alone: it interpolates for display, which renders the member name, so the check action already printed Enabled=False correctly. Regression coverage: the plugin's first Pester suite. Both branches sit behind an elevation gate whose Test-IsElevated is defined inside the script and shadows any injected stand-in, so the suite lifts the two REAL condition expressions out of the script's AST and evaluates them against enum-valued, string-valued, and boolean rule objects. The check action is driven end to end in a child pwsh, because the script's `exit` escapes an in-process call and aborts the whole Pester run. 5 of 10 cases fail against the unfixed script; all 10 pass after. Closes #3368 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 40s —— View job Code review (
|
|
Claude finished @kyle-sexton's task in 1m 24s —— View job Security review complete
No security issues found. This PR changes a local, admin-elevation-gated PowerShell utility
This is a correctness fix to a firewall-state comparison (enum-vs-truthiness bug); it restores the |
|
Last security-reviewed head: |
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a0391cb7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The suite exercised the enum and stringified shapes of the Enabled property but not a plain [bool], which is the third shape the `-eq 'True'` comparison was chosen to tolerate. The new case passes against the UNFIXED script too, and that is the point: a genuine boolean is the one representation the old truthiness test handled correctly, so the case documents the contract rather than pinning the regression. 11/11 after the fix; the same 5 regression cases fail before it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 41s —— View job Security review complete
Skip gate: no new review needed. The only commit since the last security-reviewed head ( That change adds one more Pester test case ( No security issues found, and no new review is warranted for this head per the lane's skip gate. |
|
Claude finished @kyle-sexton's task in 2m 57s —— View job Code review (
|
…e flags The comment-hygiene lane classifies an owner/repo#N form inside a code comment as a tracker-ref:repo-issue violation. The upstream Pester behavior the child process works around is now named in prose instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 42s —— View job Security review (
|
|
Claude finished @kyle-sexton's task in 51s —— View job Code review (
|
…ference The comment-hygiene lane rejects an issue reference in a code comment under two distinct patterns: `owner/repo#N` as tracker-ref:repo-issue, and the prose form "issue <N>" as tracker-ref:issue-reference. The comment now quotes the observed error text, which is what a future reader needs anyway, and cites no tracker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 55s —— View job Code review (
|
|
Claude finished @kyle-sexton's task in 40s —— View job Security review (
|
…he weak case Two review findings, both correct. Join-Path does not split a child argument on `\`, so 'scripts\firewall.ps1' as one segment survives as an ordinary filename on non-Windows pwsh and the whole suite fails at ParseFile. Passed as separate child segments instead. The plain-boolean case cannot discriminate the fix from the defect: with a [bool] on the left, PowerShell converts the right operand to [bool] and every non-empty string converts to $true, so 'True' and 'False' behave identically there. It is now named "does not break when Enabled arrives as a plain boolean" and carries a comment stating the limitation, rather than implying a regression pin it cannot be. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 52s —— View job Security review (
|
|
Claude finished @kyle-sexton's task in 45s —— View job Code review (
|
…oss five plugins (#3459) No linked issue ## Summary A repo-wide `/improvement:find` scan surfaced seven executable scripts across five plugins that mapped to **zero** test suites under `scripts/affected-tests.sh`. The repo contract (AGENTS.md) treats a zero-suite mapping as an error, and this is the same gap class that let the kindle-dedrm firewall truthiness defect (#3396) ship undetected: scripts whose only "coverage" was accidental substring selection of unrelated suites. Writing the tests also forced out two latent defects in the instruction-placement eval harness, fixed here. ## Fix One sibling test suite per unmapped script, plus the two defect fixes: - **wizard 0.2.4**: new `template.test.sh` (144 assertions) covering the wizard library's fail-closed prompt gates, `_drain_tty` paste bypass, `write_env` quoting/atomicity/permissions, `open_url` https-only dispatch, gh secret/variable helpers (secret never on argv), stage framing, and the EXIT trap. - **songwriting 1.4.15**: new `datamuse.test.sh` (93 assertions), fully offline via a curl PATH-shim stub that fails hard if it does not shadow real curl. Covers the mode-to-relation table, TSV contract, family merge/dedupe/re-sort, LIMIT handling, and transport failures. - **prototype 0.9.6**: sibling suites for the canonical `detect-ecosystems.sh` and both skill wrappers (91 assertions): marker detection, glob-order pinning, near-miss plus non-marker manifests (so widening the marker list fails), symlink/anchoring/`CLAUDE_PROJECT_DIR` semantics, and wrapper delegation contracts. The scripts themselves are unchanged. - **kindle-dedrm 0.7.9**: new `sync-prep.test.sh` (73 assertions) proving the script is print-only: PATH shims shadow `rm`/`pwsh`/`netsh` with recording no-ops; "printed the command, ran nothing, deleted nothing" is asserted over a call log plus tree snapshot; the dry-run/live guard is tested in both directions (the firewall-bug class); the printed firewall rule name is checked against `firewall.ps1`'s actual `$RuleName`. - **instruction-placement 0.11.11**: new `adherence-experiment.test.sh` (49 cases via a stub CLI) plus two fixes the tests forced out: the underscore scoring criterion was a constant (it grepped the whole edited file, which always contains the seeded `_unitPrice`, so the published underscore/both columns measured nothing; scoring now scans only the `InvoiceTotal` body with an under-crediting fallback), and the `--filler` flag was undocumented. `adherence-results.md` carries a correction note; the experiment's conclusion is unchanged (it rests on the correctly-scored `sealed` column). ## Verification - Every previously-unmapped script now selects its sibling suite under `scripts/affected-tests.sh --explain` (each was `UNMAPPED`, exit 1, before), and `--run` over the full diff passes (7 suites). - shellcheck (repo rcfile), `shfmt -d`, and `scripts/check-shell-portability.sh` clean on all new suites; changelog parity (`--check`, `--check-bump`, `--check-preserved`) green; fixture-git-isolation, silent-skips, discriminating-skips, orphaned-fixtures, and purged-em-dashes gates green. - Each plugin's work was reviewed by an independent fresh-context verifier: diff review, gate re-runs, adversarial assertion audit, and independent mutation testing with byte-identity restoration proofs (implementer + verifier mutation batteries: wizard 33+9, songwriting 23+10, prototype 19+10, kindle-dedrm 12+9). All verifier findings, including one MEDIUM (the family-dedupe assertion could not distinguish `unique` from `unique_by(.word)`) and several LOW precision gaps, were closed with mutation-kill confirmation. ## Related - #3396 (the shipped defect exemplifying this coverage-gap class) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T --- _Generated by [Claude Code](https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T)_ Co-authored-by: Claude <noreply@anthropic.com>
… bootstrap health (#3487) No linked issue ## Summary Two independent false signals found by an evidence-first improvement scan, both of the same shape: a check that reported healthy when it was not. Each is fixed and each now has a test that fails without the fix. 1. `scripts/affected-tests.sh` claimed coverage a file did not have, because R3/R4 matched a basename as an unanchored substring. A file with no suite of its own could select many suites and exit 0, so the repo's "a changed file that maps to zero suites is an error" contract silently did not apply. This is the gap class behind the kindle-dedrm firewall defect (#3396). 2. `.claude/cloud-bootstrap.sh` reported dozens of failures per session start for a perfectly healthy plugin registry, because `claude plugin install --scope user -y` already leaves a plugin enabled, so the chain's tail `plugin enable` exits 1 with "already enabled at user scope" and the three steps were joined with `&&`. The failure count is the bootstrap's only health signal, and the false alarms buried it. ## Fix **Test selection.** A file now names another only when the basename appears bounded on both sides by a character outside `[A-Za-z0-9_.-]`. `/` is deliberately outside that class, so path-qualified, prose and comment mentions all still count; a trailing run of `.` is sentence punctuation; a basename the rule cannot spell falls back to the old substring test rather than to zero coverage. Matching stays a basename rule because the cross-plugin copy fan-out depends on it. Exposing the false coverage revealed a real gap it had been masking: several Python files whose only suite is `<dir>/tests/test_<stem>.py` were unreachable by any name match, since those suites `import <module>` and never spell the filename, so R2 gained that path arm. **Bootstrap health.** The three subcommand exit statuses are now advisory. Verification reads the end state once per run, over every plugin `enabledPlugins` turns on, and counts a plugin failed only when it is absent at user scope, present without `enabled: true`, or its own directory under `plugins/` changed between the recorded `gitCommitSha` and HEAD. Cases where the snapshot cannot be determined at all (no resolvable HEAD, no registry, absent or null recorded sha, a commit this clone lacks) fail closed with a named reason rather than reading as healthy. An unreadable `plugin list --json` fails the batch in one line instead of one warning per plugin. ## Verification Measured, not asserted: - **Selection hazard, end to end:** an uncovered hook body dropped into a scratch clone selected **131 suites at exit 0** before; it is now **UNMAPPED at exit 1**. - **Sweep over every tracked file (3,455):** 534 files select fewer suites (15.8% fewer selected-suite slots), 4 previously-UNMAPPED files became mapped to the suites that genuinely test them, **nothing became unmapped**. 25 files dropped to an empty selection, every one a markdown context file whose basename had been landing inside a longer one, and every one already covered by a class in `affected-tests-no-suite.txt`, so they report as no-suite at exit 0. - **Bootstrap, live on a real machine:** `72 enabled, 0 newly installed, 0 refreshed, 0 failed`, where the previous code reported every refresh as failed. - Suites: `affected-tests.test.sh` 56 assertions green (47 before); new `.claude/hooks/cloud-bootstrap-plugins.test.sh` 32 assertions green, discovered by the existing `find plugins .claude/hooks` roots with no ci.yml change. Both changes were reviewed by independent fresh-context verifiers that re-derived the evidence rather than trusting it. The selector verifier recomputed the full sweep itself, hand-read all 190 dropped basename/enclosing-token pairs (only two name the real file, and both keep their suites through other bounded mentions), proved a direct-coverage invariant over all 3,455 files with zero missing a suite that genuinely names it, and killed 8 of its own 10 mutations. The bootstrap verifier ran 19 fail-open probes (glob and space-bearing ids, `declare -A` re-entry, duplicate ids, degenerate list JSON and registry shapes, no-git-repo, `set -u` interaction) without constructing a fail-open, and its first pass **rejected** an earlier version that still printed `0 failed` for five bad end states; those five are now pinned by tests, along with the extraction anchors and the block's exit status, since a nonzero exit there would abort the whole bootstrap under `set -e`. ## Related - #3396 (the shipped defect exemplifying the false-coverage class) - A third scan candidate, a 43x speedup of the shell-portability scan, is deliberately **not** in this PR. A verifier found a correctness regression in it, so it is parked unmerged on `wip/portability-perf-unverified` with the defect, the reproducer and the fix shape recorded in its commit message. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T --- _Generated by [Claude Code](https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
The issue asked for the premise to be verified on a real Windows machine before anything changed,
because static analysis alone could not tell a genuine enum hazard from a string-conversion
context that happens to behave. The premise is confirmed. Against a live disabled
NetFirewallRuleon Windows 11 Pro 10.0.26200, PowerShell 7.6.5:Plugin version 0.7.6 -> 0.7.7.
Fix
skills/manage/scripts/firewall.ps1, both branches the issue named:enable(was line 72).if (-not $rule.Enabled)->if ($rule.Enabled -ne 'True'). Theold test was always false, so a disabled Kindle-blocking rule was reported
already enabled, no changeand never re-enabled. That is the failure the skill exists toprevent: after a sync,
enablesilently left Kindle for PC free to fetch its auto-update.disable(was line 100).if ($rule.Enabled)->if ($rule.Enabled -eq 'True'). Themirror fault: always true, so
Disable-NetFirewallRulewas called against rules that werealready disabled and the
already disabled, no changearm was dead.Compared against the string
'True'rather than the fully-qualified[...NetSecurity.Enabled]::Truetype literal, deliberately: the type only resolves once theWindows-only
NetSecuritymodule is loaded, whereas the string comparison holds for the enum, fora CIM path that hands the property back already stringified, and for a plain
[bool].Each of the three has a test case, but they are not of equal strength, and the suite says so
in place rather than letting the count imply otherwise. The enum and string cases discriminate the
fix from the defect. The boolean case cannot: with a
[bool]on the left, PowerShell converts theright operand to
[bool], and every non-empty string converts to$true, so'True','False',and any other literal behave identically there. It proves only that the fix did not break a
boolean-valued property, and it is named and commented to claim exactly that. (Raised by review;
the point is correct and the case was relabelled rather than deleted or oversold.)
On the third acceptance box (
Show-State, was line 55). Audited under the same suspicion anddeliberately left unchanged.
$enabled = $rule.Enabledis a display interpolation, not atruthiness test, and interpolating the enum renders the member name, so
checkalready printedEnabled=Falsefor a disabled rule. Two end-to-end tests pin that, and they pass both before andafter the fix, which is the evidence that it was never part of the defect. No truthiness test on
$rule.Enabledremains anywhere in the script.Verification
New suite:
plugins/kindle-dedrm/skills/manage/tests/firewall.Tests.ps1, the plugin's firstPester file. Run with:
How the guards are reached without elevation.
enableanddisablecallTest-IsElevatedand
exit 2before reaching the guard, andTest-IsElevatedis defined inside the script, soit shadows any stand-in a test could inject, and its
[Security.Principal.WindowsPrincipal]callis not mockable. Rather than assert on a copy of the source text, the suite lifts the two real
condition expressions out of the script's AST and evaluates them against rule objects. What runs
in the test is the same expression that runs in production, so the test cannot pass against a
source that was fixed only in a comment.
The
checkaction needs no elevation and is driven end to end, in a childpwshwith aGet-NetFirewallRulestub in its global scope. Child-process rather than in-process becausefirewall.ps1callsexit, which escapes an in-process& $ScriptPathand aborts the entirePester run (Pester issue 2669) instead of failing one test. Observed, not assumed: the first
attempt at an in-process invocation produced
InvalidOperationException: A 'break' or 'continue' statement … escaped from your codeand zeroresults for the whole file.
The comment naming that upstream behavior originally cited it as
pester/Pester#2669, which thecomment-hygieneCI lane classifies as atracker-ref:repo-issueviolation inside a codecomment. It is now written in prose; that is the only reason for the third commit on this branch.
Red before green. Against the unfixed
firewall.ps1(test file unchanged): 5 failed / 6passed.
enable guard / fires for a disabled rule so the rule is re-enabled— FAILenable guard / is not a bare truthiness test— FAILdisable guard / does not fire for an already-disabled rule— FAILdisable guard / is not a bare truthiness test— FAILboth guards … / reads plain "False"/"True" strings the same way— FAILThe six that passed against the unfixed script did so correctly, and each is named rather than
counted:
checkend-to-end cases, becauseShow-Statewas never part of the defect;enable guard / does not fire for an already-enabled ruleanddisable guard / fires for an enabled rule, because an enabled rule is the case a truthiness test got right by accident;both guards … / does not break when Enabled arrives as a plain boolean, because a genuine[bool]is the one representation-not $rule.Enabledhandled correctly. That case documentsthe contract; it is explicitly not one of the regression pins.
After the fix: 11 passed / 0 failed.
The enum cross-check compares the suite's locally declared stand-in against the real
NetSecuritytype where it is loadable (Falseis 2, non-zero, renders"False"), andself-skips where the module is absent, so the suite is not silently testing a fiction.
Gates
scripts/affected-tests.sh --explainnow selects the new suite forfirewall.ps1(
references firewall.ps1). Before this PR the script's only selection wasplugins/skill-quality/scripts/check-skill.test.sh (references cleanup.sh), which is theunanchored-substring hazard the tool's own header warns about, meaning
firewall.ps1had noreal coverage at all.
scripts/affected-tests.shselects exactly two suites for this change set: the new Pester fileand
plugins/skill-quality/scripts/check-skill.test.sh.--runexecutes the shell one andreports the Pester one as SELECTED / NOT RUN by design, because the runner will not guess a
non-shell lane. The shell suite is covered in CI by the
plugin-gatelane, which runsscripts/run-plugin-tests.shover the whole corpus; the Pester suite was run directly, 11/11.scripts/validate-plugins.sh— passed.CHECK_SKILL_SKILLS_ROOT=plugins/kindle-dedrm/skills scripts/…/check-skill.sh manage—PASS, 0 errors, 1 pre-existing warning (no Gotchas surface), unchanged by this PR.
scripts/check-changelog-parity.sh --checkand--check-bump origin/main— both passed.markdownlint-cli2on the changelog — 0 issues.scripts/check-shell-portability.shis not applicable: this change set contains no shell files.Related
Closes #3368