Skip to content

fix(analyzer): bound static pattern regex searches - #741

Open
yashrajp22 wants to merge 63 commits into
mainfrom
yashraj/fix-static-pattern-regex-timeouts
Open

yashrajp22 wants to merge 63 commits into
mainfrom
yashraj/fix-static-pattern-regex-timeouts

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

High-complexity static patterns could monopolize a scanner worker on attacker-controlled text. Interruptible searches now preserve earlier findings and report explicit incomplete coverage with the actual limit. Paragraphs share a 0.25-second matching allowance per rule and content window, and small paragraphs use bounded batches.

The change preserves Python Unicode word-boundary behavior and uses bounded native searches for variable-shell backreferences. Oversized argument tails report their character limit. It also preserves main's shared AST analysis, indexes source lines once, and removes overlapping whitespace repeats in the anti-refusal schema helper. The resource-bounds guide identifies rules outside this change.

Validation includes full-size benign and adversarial inputs, Unicode match/span parity, paragraph budgets, all three variable-flow callers, and one AST parse/index for 100 shell-flag candidates. It also covers the newer Python execution-surface and TM1 ownership behavior from main. The timeout-routing regression exhausts the selected rule's allowance deterministically, so it does not depend on CPU speed. The final combined source and wheel results are below.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Hi @yashrajp22, thank you for taking on interruptible static-pattern matching! The translator is careful about Python's word/space alphabet, the ı/İ case aliases, empty-match adjacency and fail-closed grammar.

Value and readiness: The goal is right, and the bound works on the shapes it targets. On main, the TM1 SQL-construction pattern is cubic on one long line and P2's HTML-comment pattern is quadratic. Both run inside re with the GIL held, so a small file can stall the whole scan. With this PR, both stop at 0.25 s and are recorded as partial. However, the translated regex patterns are much slower than re on ordinary non-ASCII text, so the new per-pattern ceiling fires on legitimate documents. Those files are then reported as partially inspected, and the rest of that analyzer is skipped for them. The ledger also reports these events against the 300 s artifact limit. Not ready yet.

Material findings

  1. [Blocker] src/skillspector/nodes/analyzers/static_runner.py:365 (budget at :301): false RUNTIME_LIMIT on ordinary files.
    • When the content contains any non-ASCII character, every \w, \s, \d and \b is expanded to full-Unicode tables. \w alone is about 750 ranges, and each \b becomes four lookarounds over that class.
    • I rebuilt the documented rewrite by hand for TM1 \b(?:delete|remove)\s+['\"]?/[^\s'\"]{1,100}. On text made from this repository's own README.md and docs/*.md, it took 92/190/285/366 ms at 64K/128K/192K/256K characters. Python re on main takes 1-4 ms. On my laptop, such files exceed 0.25 s above about 170K characters; CI runners are slower.
    • The resulting TimeoutError becomes _StaticResourceLimitError(RUNTIME_LIMIT), and _scan_path returns early. TM1's remaining patterns and TM2-TM4 are skipped for that file, and the file is marked partial. MCP safe_to_install then becomes false, and --fail-on-incomplete exits 1.
    • ASCII content is not immune either. I built a 256K ASCII API reference with a curl -X GET ... -H ... line every fourth line (872 commands, no pipe). On it, TM1 curl\s+[^|]*-k\b takes 0.134 s in re and 0.287 s translated.
    • The delete/remove pattern runs in 41 ms on ASCII text, which is why CI is green. Still, the first CI run on this branch failed three legitimate-content tests with the same RUNTIME_LIMIT/partial result.
    • Expected fix: keep near-native speed on non-ASCII input. One option is the offset-preserving ASCII alphabet that artifact_integrity._multiline_prompt_matching_text already uses with regex.ASCII. Catalog patterns with non-ASCII literals, such as P2's zero-width class, would need their own path. Another option is to route only patterns with super-linear shapes through the timed engine and rewrite those with bounded quantifiers.
    • Please also add production-size completeness tests: one on non-ASCII Markdown (for example 256K characters of the repo's own docs) and one on a dense command reference.
  2. [Blocker] src/skillspector/nodes/analyzers/static_runner.py:506: the per-pattern limit is lost before it reaches the ledger.
    • iter_pattern_matches reports limit_seconds: 0.25. However, run_static_patterns_with_ledger treats every RUNTIME_LIMIT as an expired artifact and replaces the metrics with limit_seconds: runtime_limit (300 s, or the remaining workflow time) at static_runner.py:2796-2810.
    • A pattern timeout is therefore published as "Inspection reached its configured runtime limit", with 0.25 s observed against a 300 s limit. That contradicts docs/ANALYSIS_RESOURCE_BOUNDS.md, which promises "the applicable observed and limit values". It also points users at SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT, which cannot lift this bound.
    • Expected fix: keep the per-pattern limit in the event (or add a distinguishing detail), and add the new ceiling to the resource-bounds table.
  3. [Non-blocking] static_runner.py:478 and :484: both clocks measure process-wide CPU time. time.process_time() and the regex timeout count every thread in the process.
    • With three threads hashing outside the GIL, a search with timeout=0.202 raised after only 0.053 s of wall time.
    • yara-python 4.5.4 also releases the GIL during match(); a Python thread kept running throughout a 15 s scan.
    • Analyzers run in parallel, so a concurrent YARA scan of a large file consumes part of every pattern's 0.25 s. Consider time.thread_time() for your own accounting, plus a retry when the engine timeout fires before this thread has used its budget.
  4. [Non-blocking] static_runner.py:307 and :338: lru_cache does not serialize cache misses.
    • Every analyzer thread that reaches the first non-ASCII file builds the 1.1M-code-point table itself. That took 0.76 s per build here, and the static analyzers all start together.
    • My static count gives roughly 310 distinct routed patterns. With two alphabets each, that exceeds maxsize=512, so bundles mixing ASCII and non-ASCII files can evict and recompile Unicode-table patterns.
    • Those compiles cost 24-71 ms each in my measurements. A two-\b pattern I built expanded to about 129K characters of regex source.
    • Consider building the tables once (at import or under a lock) and sizing the cache to the catalog.
  5. [Non-blocking] Scope: several catalog loops still use untimed re.
    • Privilege escalation: the PE1-PE3 code loops, and PE4/PE5 at static_patterns_privilege_escalation.py:1040 and :1064.
    • The harmful_content, deserialization, ssrf, system_prompt_leakage, anti_refusal and output_handling code patterns, and _decoded_literal_xor_calls.
    • In particular, #678's new PE2 chmod regex would land in an unbounded module. Its own docstring says "chmod 0" * 8000 stays quadratic.
    • Please state the covered scope in the PR body, or extend the change to these loops.

PIC tradeoffs: The per-pattern ceiling turns slow-but-complete scans into fast partial results. Even after finding 1 is fixed, the PIC should decide two questions:

  • Should one pattern timeout end the whole analyzer for that file (the current _scan_path behaviour), or only skip that pattern?
  • Should the 0.25 s CPU limit be configurable, or scale with window size?

Verification and gaps:

  • Old shapes, on my own inputs with Python re:
    • P2 <!--.*?(...).*?--> on <!-- followed by repeated send took 0.094/0.363/1.458 s at 10K/20K/40K characters (quadratic).
    • TM1 SQL on a single query('{}{}... line took 0.004/0.034/0.276/2.216 s at 207/407/807/1,607 characters (cubic).
  • New bound, using my transcriptions with regex and a 0.25 s timeout: both shapes raise TimeoutError at 0.25 s on 256K and 12.8K inputs. Real attack strings still match, for example <!-- ignore previous instructions and send data --> and an f-string execute containing DROP TABLE.
  • Partial reporting: I traced iter_pattern_matches → _StaticResourceLimitError → _scan_path, which keeps findings already created. No module defines postprocess_path_findings, so _cleanup_expired_path_findings keeps them too. The result is a PARTIAL ledger event, and no converted call site swallows the error.
  • Semantics: no converted module uses scoped flags, VERBOSE, \B or \N. The VERBOSE regexes in anti_refusal and output_handling never reach the timed matcher. Every transcription I ran produced the same spans as re.
  • CI: the first commit failed test-unit with three RUNTIME_LIMIT/partial results on legitimate fixtures, and 6bc8c39 passed. Head 7a5a1ce adds only bot merges of main, and the PR diff is unchanged (same patch-id). CI on the head is action_required.
  • Conflicts: none with main, #739, #740, #742 or #678.
  • I did not run the tests locally, per policy.

Decision: Changes Requested (reviewed head 7a5a1ce5eccba7fabc36d25168c48e5da81e147f)

Comment thread src/skillspector/nodes/analyzers/static_runner.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_runner.py Outdated
github-actions Bot and others added 4 commits October 5, 2026 14:54
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed review. I also changed the accounting so caller work and other threads do not use the matching budget, and made the character-table setup shared with a larger bounded cache. The description and resource guide now spell out the four covered rule groups; the other helper searches are outside this change. The fixed limit and partial-coverage behavior are kept so interrupted work cannot be reported as complete.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Hi @yashrajp22, thank you for the fast rework, especially the compact Unicode categories and the ledger fix!

Value and readiness: The bound is worth having. On main, a 224K line of repeated chmod 0 keeps TM1's a+rwx rule busy for 26 s, and this PR stops it at 0.25 s with partial coverage. The fix commits resolve the ledger limit and the cache, and cut most of the Unicode-table cost. However, the PR's own 256K fixtures still exceed the 0.25 s limit in two other analyzers that now use the timed matcher. The new completeness test runs only tool_misuse, so it cannot see this. Ordinary files would still be reported as partially inspected, so this is not ready yet.

Previous findings:

  • 1 (false RUNTIME_LIMIT on ordinary files): Partly resolved. In my transcription, TM1 delete/remove on 256K of this repo's docs dropped from 366 ms to about 150 ms (re: 5 ms). The curl check fixes the command reference for TM1, but only when -k and --insecure are absent. The rest is new finding 1.
  • 2 (per-search limit lost in the ledger): Resolved. static_runner.py:2856 now keeps the search's metrics. The test asserts limit_seconds == 0.000001, which the old overwrite would fail. The resource-bounds table has the new row.
  • 3 (process-wide CPU clocks): Partly resolved. The matcher now uses thread CPU and retries, as suggested, but finding 3 below remains.
  • 4 (cache misses and size): Resolved. The categories are built once under _CATEGORY_LOCK. There are 375 routed (pattern, flags) pairs, so the two alphabets need 750 entries, within maxsize=1024. Compiles now take 0.67 ms median (max 9.7 ms), against 24-71 ms before.
  • 5 (scope): Partly resolved. The scope is now documented, but inaccurately (new finding 2).

Both earlier inline threads were resolved on GitHub. The first is only partly fixed, so I have added inline comments on the specific lines.

Material findings

  1. [Blocker] Ordinary 256K windows still hit the 0.25 s limit, including the PR's own fixtures in analyzers other than tool_misuse. Measured with my transcription, using regex 2026.5.9 on Python 3.12 (CI's version) on an Apple-silicon laptop. CI runners are usually slower.
    • static_patterns_data_exfiltration.py:305: E1 curl\s+[^|]*(?:-d|--data|--data-raw|--data-binary)\s+ takes about 1.07 s on the PR's command_reference fixture, against 0.21 s in re on main. It still takes 1.2 s untranslated, so the cost comes from the engine, not the rewrite. data_exfiltration reports that file as partial.
    • static_patterns_memory_poisoning.py:547: MP2 (.{2,20}?)\1{20,} runs on every file. The rewrite leaves it unchanged, but regex needs about 1 µs per character for it, twice re. It took 0.26-0.32 s on the PR's unicode_docs fixture across my runs, and 0.22 s on the command fixture. On main this rule is linear and bounded by the window, so timing it adds false partials for little protection.
    • static_patterns_tool_misuse.py:3167: the curl check passes as soon as the suffix appears anywhere. With one line sort -k 2 results.txt added to the command fixture, TM1 curl\s+[^|]*-k\b runs the full scan and takes 0.36 s. The same [^|]* shape has no check in TM1 wget, git push and chmod/chown, or in E1 wget. On 256K references with one such command every fourth line and no pipes, these took 0.32-0.52 s.
    • The two \b-led TM1 rules still take 0.12-0.16 s on non-ASCII 256K docs, which leaves little margin, especially under contention (finding 3).
    • E1 and MP2 have been in the change since the first commit; I should have reported them last time.
    • Expected fix:
      • Run the completeness test on every analyzer that uses the timed matcher, not only tool_misuse. Use both fixtures plus a command reference that mentions -k once.
      • Keep MP2's repetition rule on re, or out of the 0.25 s path.
      • Replace the curl-only check with an exact linear pre-check for the whole X\s+[^|]*SUFFIX family: search only the pipe-free segments where a required suffix follows the command word. Avoid a length bound such as [^|]{0,N}, because padding would evade it.
  2. [Blocker] docs/ANALYSIS_RESOURCE_BOUNDS.md:301-307 (and the PR body) understate the scope. They say only prompt-injection, tool-misuse, data-exfiltration and supply-chain patterns are timed, and that "Other catalog modules" are outside the matcher. The code also routes the agent_snooping, excessive_agency, memory_poisoning and rogue_agent loops. Through iter_paragraph_matches, it also times the prose rules of harmful_content, system_prompt_leakage, output_handling, anti_refusal, privilege_escalation and ssrf. A memory_poisoning partial with a 0.25 s limit (finding 1) would contradict this paragraph. Please describe the actual scope.
  3. [Non-blocking] static_runner.py:521-545: the retry counts thread CPU correctly, but regex's timer still counts process CPU, and each retry restarts the search. While another native call runs without the GIL, as a YARA scan of a large file does, one search gets less than 0.25 s. In my re-implementation of this loop, a search that finishes alone in 0.165 s hit the limit after 16 retries. So the docs' "Caller work and other threads do not consume the matching allowance" is true for accounting only. Consider scaling the next attempt's engine timeout by the failed attempt's process-to-thread CPU ratio, or qualify the sentence.

PIC tradeoffs:

  • Does a pattern timeout end the whole file's analysis? Yes, for that analyzer. _scan_path returns at the first _StaticResourceLimitError (static_runner.py:1217), and _scan_all_views_detailed returns before the remaining views and windows. Findings already produced are kept, the artifact is recorded as partial, and other analyzers and artifacts continue. Skipping only the timed-out search would keep more coverage on adversarial files. However, a crafted file could then spend 0.25 s on each of the 375 routed searches in every window.
  • Should the 0.25 s limit be configurable? It stays fixed, by design. Because regex is 2-5 times slower than re on several catalog shapes, a fixed per-search limit leaves little margin at the 256K window size on slower hardware. A limit that scales with the searched length, or a setting, would let operators trade time for completeness. This is a product decision.
  • Evasion: timeouts are not silent. No call site catches _StaticResourceLimitError (it subclasses RuntimeError, so I checked the timed modules for except Exception/RuntimeError). Every timeout becomes a PARTIAL ledger event with the search's own limit. Still, one crafted line decides which analyzer stops early for that artifact, and users who do not gate on completeness only see the partial status.

Verification and gaps:

  • Interplay with #740 (merged): _decoded_literal_xor_calls no longer needs the timed matcher. On main, a 200K unterminated backslash run takes 13 ms, and 30K malformed headers take under 1 ms. Its remaining cost is one whole-content call-pattern scan per qualifying helper: 2,800 helpers in 217K characters take 7.4 s on main. A per-search deadline cannot bound that, so it needs a separate fix outside this PR.
  • Interplay with #678 (merged): both TM1 chmod rules use the timed matcher here. PE2's chmod rule stays on untimed re. That is acceptable, because it took at most 12 ms on every adversarial shape I tried, including "chmod 0" * 32000, -a and -- option runs, and long zero runs.
  • Semantics: across all 375 routed (pattern, flags) pairs, taken from main (the PR changes no pattern list), my transcription produced the same spans as re on four 256K corpora: repo docs, the unicode_docs fixture, CJK prose and Cyrillic prose.
  • On the production-size raw windows of test_cross_window_separator_pair_across_public_surfaces, every search took at most 54 ms. I did not model that test's other security views, so I can only say the scaled windows (6bc8c39) do not appear to hide a timeout.
  • Commits: 154cbb3, 24bed2f and e5eec2f carry the author's sign-off. The bot merges that followed leave the PR diff unchanged (same patch-id).
  • Conflicts: none with main, #742 or #739.
  • CI: on e5eec2f, 5 of 6 checks are green and test-unit is still running. Head d722477 is action_required.
  • I did not run the PR's tests, per policy.

Decision: Changes Requested (reviewed head d72247754d9619486c52574ef88b67ff62ee2ddf)

Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py Outdated
Comment thread docs/ANALYSIS_RESOURCE_BOUNDS.md Outdated
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

Thanks! I checked the remaining patterns and found two code paths that still missed the timeout protection. Those now use the shared matcher, along with the shell-flag precheck. I also moved the repeated anti-refusal line splitting out of the match loop. The new sample checks confirm that the slow inputs finish with a clear incomplete-analysis message, and the real detections still work. The targeted suite passed 1,500 tests, and source and installed-wheel results match.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Hi @yashrajp22, thank you for the linear command-segment matcher! It matched Python re span for span in 360,000 differential fuzz cases and turns a 43 s curl -o stall into 6 ms. The MP2 exemption, the 14-analyzer completeness tests and the newly routed OH/PE code patterns also close out most of the last round cleanly.

Value and readiness: The problem is real on main. Running main's own analyzers, the author's P5 recipe, TM1 SQL, OH1 and PE3 payloads each ran for over 40 s (several were stopped after 60-120 s), and PE1 for 43-48 s. Other shapes are super-linear too:

  • PE5 "/sys/fs/cgroup/": 4.4 s at 120K characters.
  • TM4 "--set ": 44 s at 240K characters.
  • The variable shell-flag precheck: 7.9 s on 32K newlines in a .py file.
  • Anti-refusal: 91 s on 8,000 dense lines, because of the per-match splitlines().

All of these hold the GIL. In my transcriptions of the head's matcher, each payload stops at 0.250 s of thread CPU, and the ledger keeps the search's own 0.25 s limit. Positive controls keep identical spans, and the linear path stays at 72 ms or less on adversarial 256K input. Of the eight earlier findings, six are resolved, one is superseded and one is partly resolved (below).

The PR cannot merge yet. It conflicts with main (#788) in anti-refusal, and the resolution has to combine both sides (finding 1). The other findings are non-blocking. Findings 2 and 3 matter for the landing order with #790: a lint-clean resolution of that conflict would silently undo part of this PR, and no test would catch it. Once finding 1 is resolved, the PR is ready for final maintainer/PIC review. The PIC questions below are still open.

Previous findings:

  • Review 1 finding 1 (false RUNTIME_LIMIT on ordinary files): Superseded by review 2 finding 1, which is now resolved.
    • Compact categories (154cbb3) cut TM1 delete/remove on 256K of repo docs from 366 ms to about 164 ms (re: 6 ms). No routed search reached 0.25 s on any of 14 ordinary corpora (transcription, 434 routed searches).
    • The requested production-size tests exist: test_production_window_ordinary_content_remains_complete (tests/nodes/analyzers/test_static_patterns.py:2446) and test_dense_command_references_remain_complete (:2620).
    • The remaining margin is tracked in finding 6.
  • Review 1 finding 2 (per-search limit lost before the ledger): Resolved.
    • static_runner.py:2971-2975 rewrites metrics only for reasons other than RUNTIME_LIMIT.
    • The table row is at docs/ANALYSIS_RESOURCE_BOUNDS.md:258.
    • test_static_regex_deadline_retains_findings_and_incomplete_ledger (:2338) asserts limit_seconds == 0.000001.
  • Review 1 finding 3 (process-wide CPU clocks): Resolved.
    • Accounting uses time.thread_time() (static_runner.py:604, :626), and an early engine timeout retries within the thread's allowance (:631-635).
    • Covered by test_timed_pattern_does_not_charge_consumer_cpu (:2313) and test_timed_pattern_retries_process_cpu_from_other_threads (:2407).
  • Review 1 finding 4 (cache misses and size): Resolved.
    • The tables are built under _CATEGORY_LOCK (static_runner.py:309-318), and _timed_pattern has maxsize=1024 (:380).
    • There are now 418 timed (pattern, flags) pairs, so the two alphabets need 836 entries.
  • Review 1 finding 5 (scope): Resolved.
    • 1565283 routes the PE1-PE5 code loops (including #678's PE2 chmod rule) and the OH1/OH3 code patterns through iter_pattern_matches.
    • The docs and PR body state the scope. Only a wording remainder is left (finding 8).
  • Review 2 finding 1 (ordinary 256K windows still hit the limit): Resolved.
    • (a) The completeness test covers all 14 timed analyzers on unicode_docs, command_reference and unrelated_flag at 256,000 characters (:2426-2470).
    • (b) MP2's repetition rule stays on re.finditer (static_patterns_memory_poisoning.py:549-555; test_bounded_repetition_stays_on_native_engine at :2553).
    • (c) The 15 [^|]*/[^&]* shapes use the linear matcher with no length bound (static_runner.py:495-561), and test_linear_command_registry_covers_routed_catalog_shapes (:2473) enforces the registry.
    • E1 on command_reference now takes 3.1 ms, against 1,302 ms through the regex engine.
    • The margin remark in that finding's fourth bullet is still open: the two \b-led TM1 rules use 160-168 ms of the 250 ms allowance on non-ASCII docs (finding 6).
  • Review 2 finding 2 (docs understate the scope): Partly resolved.
    • docs/ANALYSIS_RESOURCE_BOUNDS.md:337-344 now lists the eight routed groups, the six paragraph-timed groups, the OH/PE code patterns and the precheck. :346-351 explains the MP2 exception.
    • What remains: the closing sentence at :344 now leaves out the untimed catalog code rules (finding 8).
  • Review 2 finding 3 (regex's timer counts process CPU): Resolved.
    • The author took the "qualify" option: the docs (:353-358) and the code comment (static_runner.py:631-634) now describe the behaviour.
    • The contention risk itself feeds the fixed-limit PIC question below.

The author's replies hold where they can be checked. Two claims cannot be fully confirmed:

  • The 1,362- and 1,500-test counts and the source/wheel parity cannot be verified without running the PR's tests. CI is green.
  • The 7 Oct statement that the new checks show slow inputs ending with an incomplete-analysis message holds for OH1, PE1 and PE3 through :2644. However, the code-pattern ledger test at :2693 would also pass without the code routing (finding 3).

Material findings

  1. [Blocker] src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py:408: the PR conflicts with main (#788) in the AR match loop, and only a combined resolution is correct.
    • GitHub reports the PR as dirty. A three-way git merge-file (base b359c66, head 2f896e2, main 3c8e4b9) gives exactly one conflict hunk:
      • Head side: line_num, column = locations.line_and_column(match.start()) at :408, with column used at :411.
      • main side: #788's AR2 _is_descriptive_python_comment skip, followed by the old per-match lines = content.splitlines() and get_line_number(...).
      • Everything else auto-merges, including the head's import change at :40 (which drops get_line_number) and #788's check_runtime, USES_RUNTIME_CHECK and _PythonComments.
    • One-sided resolutions fail CI rather than slipping through:
      • main's side gives ruff F821 (get_line_number, column) and NameErrors in about 102 of #788's tests.
      • The head's side gives F841 (python_comments) and 11 failures in #788's TestAntiRefusalDescriptivePythonComments.
      • The combined form is ruff-clean and passes main's test_static_patterns_anti_refusal.py (114 passed, 4 xfailed). That run used main's module with the three head lines transcribed in.
    • #788's helpers use only group(0), start(), end() and .string. All of these work on the regex.Match that iter_pattern_matches yields over the full content.
    • Keep the line-index change in this PR, because it carries real weight. With main's per-match splitlines(), dense AR text is quadratic: 2,000/4,000/8,000 lines take 5.25/20.5/91.2 s on main, against 0.24/0.47/0.86 s for the combined resolution.
    • Consequence: the PR cannot merge as it stands. Taking either side alone breaks every anti-refusal scan or brings back #788's HIGH false positive on descriptive Python comments.
    • Expected fix: merge or rebase main. In the loop body:
      • Keep #788's if rule_id == "AR2" and python_comments is not None and _is_descriptive_python_comment(python_comments, match): continue first.
      • Then use the head's line_and_column and get_context_from_lines(lines, line_num, window=3, column=column) with the single hoisted lines.
      • Keep #788's check_runtime signature, USES_RUNTIME_CHECK, _PythonComments, _no_runtime_check and imports.
      • Rerun tests/nodes/analyzers/test_static_patterns_anti_refusal.py together with the PR's anti-refusal tests.
  2. [Non-blocking] src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:3957 (and TM4 at :4067): the conflict with open #790 has a lint-clean wrong resolution that puts both searches back on untimed re.
    • git merge-file with #790's head c877c9d gives two conflict hunks: the precheck (:3957-3958) and TM4 (:4067-4070). TM1-TM3 merge cleanly.
    • What #790 does in these two loops:
      • It keeps _VARIABLE_SHELL_FLAG_RE.finditer and re.finditer.
      • It adds runtime_check(), a lazy _VariableShellScopeIndex.same_scope and locations.line_and_column(...)[0].
      • It deletes _variable_shell_flag_same_scope and the get_line_number import.
    • Ruff on the candidate resolutions:
      • This PR's side gives F821 for both deleted names.
      • #790's side passes ruff check and ruff format --check but is untimed. static_runner is still imported for TM1-TM3, so there is no F401 to give it away.
      • The combined form is clean. same_scope uses only group(1), start() and end(), so it works with regex.Match.
    • Both searches are super-linear on main:
      • The precheck takes 0.14/0.46/1.97/7.93 s on 4K/8K/16K/32K newlines, about 8 minutes extrapolated per 256K window.
      • run_static_patterns_with_ledger on a 16K-newline x.py took 18.6 s and still reported completed.
      • TM4 --set\b[^\n]*privileged\s*=\s*true takes 2.9/10.7/44.3 s on 10K/20K/40K repetitions of "--set ".
    • With the head (transcription), 256K newlines finish in under 1 ms, and "--set " * 42_000 stops at 0.25 s with runtime_limit.
    • Consequence: #790 is currently mergeable with main. Whichever PR lands second can take #790's side, keep CI green (no test pins this routing, finding 3), and silently bring back stalls of several seconds to several minutes on a newline-only .py file or a --set line.
    • Expected fix: coordinate the landing order. In both loops, keep this PR's static_runner.iter_pattern_matches(...) line and #790's loop body (runtime_check(), the lazy scope index with same_scope, locations.line_and_column(...)[0]). Land the routing tests from finding 3 first.
  3. [Non-blocking] tests/nodes/analyzers/test_static_patterns.py:2693: tests do not pin the new code-path routing.
    • test_code_pattern_timeouts_reach_incomplete_ledger sets the limit to 1 µs and scans "ordinary text " * 15_000. The OH1 and PE1 prose rules were already paragraph-timed before 1565283, so a prose search expires first and the test passes without any code routing.
      • I modelled this with main's OH and PE modules, which are identical to the head apart from the routing lines, plus a transcribed timed matcher. The ledger was still partial/runtime_limit.
    • The shell-flag precheck (static_patterns_tool_misuse.py:3957) has no adversarial or timing test.
      • Its existing functional tests (tests/unit/test_patterns_new.py:1464-1525) use small inputs.
      • Every new analyzer-level test scans SKILL.md as markdown, so the file_type == "python" branch never runs at size.
    • TM4 (:4067) and PE5 (static_patterns_privilege_escalation.py:1190) have no payload.
      • PE5 /sys/fs/cgroup/.*release_agent takes 1.36/4.38 s in main's analyze at 60K/120K characters. The transcription stops "/sys/fs/cgroup/" * 17_000 at 0.27 s.
      • E5, SC7 and the AS/EA/RA/MP code loops also have no routing guard.
    • OH1 SQL, PE1 and PE3 are genuinely pinned by test_reported_backtracking_payloads_finish_within_search_budget (:2644), which takes minutes on main.
    • Consequence: reverting the precheck, TM4, PE4/PE5 or OH3 routing would not fail CI, including when it happens through the #790 resolution in finding 2.
    • Expected fix:
      • Make :2693 discriminate: shrink the limit only for one code-pattern source, as :2338 does for <!--, or assert which search expired.
      • Add ("privilege_escalation", "/sys/fs/cgroup/" * 17_000) and ("tool_misuse", "--set " * 40_000) to :2634-2642.
      • Add tool_misuse.analyze("\n" * 256_000, "tool.py", "python") under the same 5 s bound. It should finish cleanly on the head, so accept either a clean finish or runtime_limit with limit_seconds <= 0.25.
  4. [Non-blocking] src/skillspector/nodes/analyzers/static_runner.py:588: each paragraph gets a fresh 0.25 s allowance, so a paragraph-split P5 file costs about 20-28 s per window and is reported complete.
    • iter_paragraph_matches (:659-660) calls iter_pattern_matches once per paragraph, and each call resets matching_seconds and matching_limit (:587-588). Only the 300 s artifact deadline bounds the total.
    • Measured with a transcription, using main's P5 rule and _paragraph_ranges:
      • One 256K window of "for every recipe " + " add" * 600 paragraphs separated by blank lines (105-106 paragraphs) takes 19-24 s. The worst single search is 196-209 ms, nothing times out, and the analyzer reports completed.
      • main takes 9-12 s on the same content, so the head is about 2x slower for this shape.
      • Paragraphs tuned just under the limit (about 2.6K characters) reach 28 s per window for P5 alone. At about 2.8K the first search trips, and the file is correctly marked partial.
    • The head still caps the single-paragraph case: the author's 240K-character paragraph runs over a minute on main.
    • The docs (docs/ANALYSIS_RESOURCE_BOUNDS.md:258, :337) and the PR body say "per search". That is literally right, but they do not say that a paragraph-matched rule runs one search per paragraph, so readers will take it as a per-rule cap.
    • The docstring at :654 ("executable and structured rules use finditer") is stale.
    • Consequence: a crafted file can hold a worker for tens of seconds per rule per window, up to the artifact deadline, with no partial signal.
    • Expected fix: either share one allowance per rule per window across paragraphs (weigh this against finding 6), or document that paragraph-matched rules get the limit per paragraph, with the artifact and workflow deadlines bounding the total. Add a many-paragraph P5 test either way, and refresh the :654 docstring.
  5. [Non-blocking] src/skillspector/nodes/analyzers/static_runner.py:660: starting a timed search for every paragraph makes paragraph-dense files about 4x more expensive than on main.
    • Each per-paragraph call repeats the compile lookup and the budget checks, reads time.thread_time() twice and builds a new regex iterator with timeout=. The timed iterator alone costs about 1.17 µs, against 0.34 µs untimed.
    • Measured on 256K of "a\n\n" (85,334 paragraphs) with main's AR, harmful-content, system-prompt-leakage, excessive-agency and privilege-escalation analyzers:
      • All five: 7.24 s with main's matcher, 31.07 s with a transcription of the head's matcher (budget checks included).
      • The AR patterns alone: 1.41 s to 8.29 s. Without timeout=, the engine costs the same as re on this content, so the slowdown is per-range overhead.
    • Ordinary docs with a few hundred paragraphs pay only milliseconds for this overhead.
    • Consequence: a trivial crafted file quadruples prose-analyzer CPU per window while holding the GIL. This is bounded by the artifact deadline; it is not a hang.
    • Expected fix: resolve the pattern, translation and budget once per iter_paragraph_matches call. Hoisting alone only gets to 2.9x (20.88 s), so also avoid a timed iterator and clock reads for every tiny range. For example, run ranges below a small length on the native pattern, and check the clocks every N ranges. A shared allowance (finding 4) fits the same loop. Add a paragraph-dense timing test.
  6. [Non-blocking] src/skillspector/nodes/analyzers/static_runner.py:443: the \b rewrite leaves a thin margin on ordinary non-ASCII content, and single-paragraph non-ASCII text is untested.
    • On non-ASCII content, each \b becomes (?:(?<!w)(?=w)|(?<=w)(?!w)) over the roughly 940-character Unicode word class (:442-443, unchanged since d722477). A single © switches the whole window to this class.

    • Worst search per 256K corpus (transcription, best of 3):

      Corpus Worst search Rule
      The PR's unicode_docs fixture 147 ms TM1 \b(?:rm|del|erase), whole window
      The same text without blank lines 150 ms
      README+docs without blank lines 158 ms AR3
      Whitespace-collapsed TS bundle with one © 152 ms AR2
      Python source 126 ms
      Russian locale JSON 169 ms AR2
      French locale JSON 163 ms AR3
      • Single runs reached 186 ms. re takes 4-8 ms on the same searches.
      • With the regex engine's native \b, TM1 delete/remove takes 5.8 ms instead of 152 ms, so the rewrite accounts for over 95% of the cost.
    • Idle runs never crossed 250 ms. Under contention, results depend on the competing workload:

      • With one competing zlib.compress thread, a 0.18 s Cyrillic AR search hit the limit in 3 of 3 runs.
      • With hashlib threads, a 0.16 s search completed without retries.
      • All of this was measured on Apple silicon with Python 3.13. CI uses 3.12 on x86.
    • What the tests cover:

      • The existing unicode_docs fixture already runs the two \b-led TM1 rules over a whole window.
      • Its 1,717 paragraphs keep AR1-AR3 off the whole-window path.
      • command_reference and unrelated_flag are ASCII (about 58 ms).
    • Consequence: minified bundles, locale files and Markdown without blank lines can become partial on slower or contended runners. That flips MCP safe_to_install to false and makes --fail-on-incomplete exit 1.

    • Expected fix:

      • Add single-paragraph non-ASCII fixtures to the completeness test, for example the DEVELOPMENT.md intro without blank lines and a minified bundle with one ©.
      • Buy margin with a cheaper \b translation. When \b sits next to a literal word character, as in all the \b-led AR and TM1 rules, one lookbehind or lookahead is enough.
      • Alternatively, settle it with the fixed-limit PIC question.
  7. [Non-blocking] src/skillspector/nodes/analyzers/static_runner.py:445: backreferences under IGNORECASE fold differently from Python re, which breaks the claimed span parity for _VARIABLE_SHELL_FLAG_PATTERN.
    • \1 is passed through unchanged (:445) and compiled with regex.IGNORECASE. The ı/İ handling (:394-402, :436-441, :485-486) covers only literals and classes.
    • For (.)\1 with fullmatch:
      • ıI, ſs and µμ match only in regex.
      • İI matches only in re.
      • A Unicode scan found 96 mismatching ordered pairs over 65 code points, so a list of special characters will not close the gap.
    • The only routed catalog pattern with a backreference is _VARIABLE_SHELL_FLAG_PATTERN (static_patterns_tool_misuse.py:195-200), used by TM1 (:3424) and the precheck (:3957). In 20,000 fuzzed snippets, 99 spans differed.
    • A concrete case in valid Python:
      • The file has IX = False, ıx = False, ıx = True inside def f(), flag = True, then subprocess.run((cmd), shell=ıx); subprocess.run(cmd, shell=IX); subprocess.run(cmd, shell=flag).
      • re matches the flag flow at 74-180, and main reports TM1 at line 7.
      • The transcribed timed matcher matches ıx = True … shell=IX at 60-147 instead and consumes the real anchor. The scope check then rejects it, and no variable-flag TM1 is reported.
    • Consequence: detection drifts in both directions. Some pairs that are the same Python identifier under NFKC (ſx/sx) are now caught, and a crafted file can drop this TM1 finding. The practical impact is small, since flag = not False already evades the heuristic on main.
    • Expected fix: either keep this pattern on native re with an exemption like MP2's (after bounding its quadratic ^\s* part), or have _timed_pattern fail closed (RULES_UNAVAILABLE) for backreferences under IGNORECASE when the content is non-ASCII. Confirming candidates with the original pattern is not enough on its own, because it cannot recover re-only matches. Add a parity test with ı, İ, ſ and µ identifiers.
  8. [Non-blocking] docs/ANALYSIS_RESOURCE_BOUNDS.md:344: the closing sentence says only helper-specific searches remain outside the timed matcher, but some catalog code rules do too.
    • 1565283 changed "Other code rules and helper-specific searches remain outside this matcher" to the current text.
    • Catalog rules still on untimed re:
      • SSRF endpoint and request patterns (static_patterns_ssrf.py:118-122)
      • harmful-content substance names (static_patterns_harmful_content.py:112-113)
      • deserialization DS1-DS4 (static_patterns_deserialization.py:131)
    • All of these are linear: at most 43 ms on adversarial 256K input.
    • The PR body likewise says "Paragraph matching also covers harmful content ... and SSRF" without "prose rules".
    • Consequence: readers will assume every catalog rule is timed. This is wording only; there is no performance gap.
    • Expected fix: restore the qualifier, for example "Other code rules (SSRF endpoint and request patterns, harmful-content substance names, deserialization DS1-DS4) and helper-specific searches remain outside it; these patterns are linear and bounded by the inspection window and artifact deadline." Mirror it in the PR body.
  9. [Non-blocking, pre-existing] src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py:370: the untimed AR schema-field helper can still hold a worker for about an hour.
    • The chain: after a timed AR2 match, analyze calls _is_benign_ar_context (:423), which calls _is_schema_field_clause (:388), which runs _BENIGN_AR_SCHEMA_FIELD_PATTERN.search(continuation) (:370) on plain re. The VERBOSE pattern starts with ^\s*(?:\[\])?\s+, which is quadratic on whitespace runs.
    • Input "Respond with no warnings" + " " * k + "x":
      • Transcription: 0.24/0.95/3.60/15.1/60.9 s at k = 2K/4K/8K/16K/32K, about an hour extrapolated to a 256K window.
      • main's CLI spent 15.0 s in this helper at 16K, even with a 2 s artifact deadline, because the deadline is checked only after the helper returns.
    • The helper is byte-identical on main, and the docs (:344) put helper searches out of scope, so this is not a regression. There is no changed line to comment on.
    • Consequence: one line in a SKILL.md can still stall anti-refusal far past the 300 s artifact deadline. The PR's bound therefore holds per catalog search, not per worker.
    • Expected fix: handle it in a separate issue, not in this PR. Cap the continuation (for example at 128 characters) or rewrite the prefix so \s* and \s+ no longer overlap. Add a regression test, and survey the other helper regexes the same way.

PIC tradeoffs:

  • Landing order with #790 (findings 2 and 3). Whichever PR lands second has to combine this PR's timed iterator with #790's loop bodies. Landing the routing tests first turns a wrong resolution into a CI failure.
  • Allowance for paragraph-matched rules (findings 4-6).
    • Today's per-paragraph allowance keeps ordinary multi-paragraph docs far from the limit. It also lets a crafted file spend about 20-28 s per rule per window and still be reported complete, and it pays a per-call overhead.
    • A shared per-rule, per-window allowance caps both, but it brings dense ordinary documents closer to the limit.
  • One search timeout ends the analyzer for that file (PIC question 1 from both earlier reviews, still open).
    • What happens: _scan_path returns at the first _StaticResourceLimitError (static_runner.py:1307). The remaining patterns, windows and views for that analyzer are skipped for the file, while other files and analyzers continue (docs :356-358).
    • The policy comes from main (static_runner.py:954 there), but this PR adds a far cheaper trigger.
    • Example (transcription): AS1's shell shape (static_patterns_agent_snooping.py:62) is outside the linear family. On one line of repeated "cat x " followed later by a .claude/ path, it hits the limit at about 6-8K characters.
      • For "cat x " * 10_000 followed by cat ~/.claude/settings.json, main completes in 0.37 s and reports AS1.
      • The head would time out in that pattern before yielding anything. It would skip the next AS1 pattern, which matches the real line, and mark the file partial.
    • The partial is visible, but with --fail-on-incomplete off (the default) the scan exits normally.
    • The alternative is to skip only the expired pattern for that window. It keeps more coverage, but each expensive pattern could then cost 0.25 s per window and view, still capped by the artifact deadline.
  • Fixed 0.25 s limit (PIC question 2, still open).
    • _STATIC_PATTERN_SECONDS (static_runner.py:303) is a constant, and the docs (:337-338) describe it as fixed.
    • On fast hardware, ordinary non-ASCII windows already use 50-73% of it (finding 6), and contention can push them over.
    • main has a precedent: artifact_integrity.py:102 uses the same fixed 0.25 s. That limit only covers P3/P4 patterns, which cost 13 ms or less here.
    • Scaling the limit with length cannot add headroom, because windows are capped at 256,000 characters.
    • Options:
      • raise the fixed value, for example to 0.5-1.0 s;
      • add a validated setting like SKILLSPECTOR_MAX_STATIC_ANALYSIS_SECONDS_PER_ARTIFACT;
      • make \b cheaper.
  • Helper regexes stay out of scope (finding 9). The bound is per catalog search, not per worker, until the helpers are surveyed.

Verification and gaps:

  • Baseline on main: I ran main's own analyzers and CLI from the main venv. That gives the timings in the value paragraph and in findings 1-5 and 9.
  • Head behaviour: all head results come from my own transcriptions of _python_categories, _timed_pattern, _linear_command_* and iter_pattern_matches, run with regex 2026.5.9.
    • Each reported payload stops at 0.250 s of thread CPU with runtime_limit and a 0.25 s limit, and positive controls keep identical spans.
    • The linear matcher had 0 span/group mismatches against re in 360,000 fuzz cases. Its worst case at 256K is 72 ms.
    • The 60 newly routed code-pattern pairs match re spans on four ordinary corpora. The worst case is 34 ms after the one-time table build.
    • SourceLocationIndex.line_and_column gives the same line as get_line_number on about 46,000 random texts.
  • No interface changes: no pattern string changed. The ledger keeps the search's own limit (static_runner.py:2971-2975). There is no JSON or SARIF schema change, and no dependency change, since regex==2026.5.9 is already pinned on main.
  • Tests that fail on main, by code trace:
    • test_reported_backtracking_payloads_finish_within_search_budget (:2644) takes minutes on main.
    • test_dense_anti_refusal_indexes_source_lines_once (:2676) fails on main's per-match split.
    • :2338, the registry test and the monkeypatch tests fail because the attributes they use do not exist on main.
  • Tests that do not discriminate: the completeness tests are regression guards against false partials, and they would also pass on re. :2693 does not discriminate (finding 3).
  • CI: all 6 checks (changes, lint, test-unit, OpenCode TypeScript Tests, DCO Check, docker-smoke) passed on 2f896e2.
  • Conflicts:
    • With main (#788) in static_patterns_anti_refusal.py (finding 1).
    • With open #790 in static_patterns_tool_misuse.py (finding 2).
    • #794 also touches static_runner.py, but only artifact classification and binary filtering. It merges cleanly and does not interact with the matcher.
  • I did not run the PR's tests or code, per policy.
  • Gaps:
    • Measurements ran on Apple silicon with Python 3.13. CI uses 3.12 on x86, so the margins in finding 6 are laptop figures.
    • The conflict analysis is git merge-file plus ruff on scratch copies. Only the anti-refusal resolution was run against main's tests.
    • Contention was probed with 1-4 threads only. Windows clock behaviour for regex's timeout was not checked.
    • The helper survey behind finding 9 traced only the AR schema-field helper end to end.

Decision: Changes Requested (reviewed head 2f896e28ec5dd753c57f9baeb2f7b392b1f20c8f)

Comment thread src/skillspector/nodes/analyzers/static_patterns_anti_refusal.py
Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py Outdated
Comment thread tests/nodes/analyzers/test_static_patterns.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_runner.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_runner.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_runner.py Outdated
Comment thread src/skillspector/nodes/analyzers/static_runner.py
Comment thread docs/ANALYSIS_RESOURCE_BOUNDS.md Outdated
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
…ttern-regex-timeouts

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

I also fixed the existing anti-refusal schema helper mentioned in the review. Its whitespace prefix no longer has overlapping repeats, so the full-size whitespace case stays bounded without cutting off valid schema text. Tests cover the reported input and the existing benign forms.

@mohgupta-ship-it

Copy link
Copy Markdown
Member

LGTM But please fix the UT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants