Conversation
Signed-off-by: Steven Moy <smoy@exaforce.com>
smoy
force-pushed
the
smoy/anchor-unanchored-patterns
branch
from
September 15, 2026 19:20
b30afa3 to
f354595
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
P5matcheskillinside skill -- in a scanner whose entire input domain is skills -- andPE3matches.envinsideprocess.envin a front-end style guide. Both shipped as 100 / CRITICAL /DO_NOT_INSTALL; neither row contains a security defect.exaforce/, not as edits to the upstream analyzers, so an upstream merge cannot silently leave a stale correction applied or shadow an upstream fix --replace_patternraisesPatchDriftErrorif either pattern is rewritten.Tests: 34 in
tests/exaforce(19 new), 1820 intests/unit+tests/nodes,ruff check src/ tests/clean.Details
Evidence. From a re-adjudication of 111 shipped findings (100 unique skill content hashes, 50 repos, scanned 2026-09-11).
P5emits at CRITICAL with confidence 0.95 -- 47 of the 50 available CRITICAL points before any other rule contributes -- so a single unanchored match nearly maxes the unit.PE3fired twice on the same style guide, including on the line reading "Access environment variables safely" and on the CSV's anti-pattern column.The regexes.
\bbefore P5's verb alternation;(?<![\w.])before PE3's dot. Verified in both directions:how to kill someone/poison a person/murder peoplestill fire, andcat .env,~/.env,./.env,app/.env,.env.production,.env.localand a line-terminal.envall still fire.PE3's narrowing does not create a blind spot.
process.env[...KEY|SECRET|TOKEN|PASSWORD...]andObject.keys(process.env)remain covered bystatic_patterns_data_exfiltration.py.The boundary-anchoring is domain-neutral and worth offering upstream to NVIDIA/skillspector. This module is the interim, and
PatchDriftErroris what makes dropping it safe once upstream lands its own fix.Self-review summary
/code-review medium, 4 findings, all fixed.ruffB009ongetattrwith a constant attribute.analyze(). If upstream refactored to pre-compile patterns at import,replace_patternwould still swap the string, every test would still pass, and the patch would be silently inert in a real scan. Rewritten end-to-end throughanalyze().DOTALLwhere the PE3 analyzer does not, making the positive-direction guarantee weaker than it read.replace_patterndocstring promised a raise that does not happen when upstream's fix is byte-identical to ours (that branch is what keepsapply()idempotent); it now says so.Added after the review:
test_the_patch_is_what_removes_the_finding.skillspectorapplies fork patches at package import, so a test that only asserts "no finding" reads identically whether the patch did the work or the analyzer never fired. It restores the upstream pattern, asserts the false positive appears, and puts ours back -- confirmed both fire unpatched and neither fires patched.