Repository navigation
fix(analyzer): reuse tool-misuse context indexes and honor deadlines - #790
Conversation
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>
Resolve the conflicts with #577 (5ee48ef) in static_patterns_tool_misuse.py. #577 replaced the per-match reparse with a single AST index (_build_variable_shell_ast_index) and moved variable reconciliation into postprocess_path_findings. That supersedes this branch's _VariableShellScopeIndex, so the resolution keeps main's variable-shell design and drops the index class, its _index_scope method and the deque import. Kept from this branch: - USES_RUNTIME_CHECK and the check_runtime keyword, alongside main's defer_variable_reconciliation keyword. - runtime_check() calls in the TM1-TM4 loops, the Perl helper and the non-deferred reconciliation block. - SourceLocationIndex and get_context_from_lines for TM1-TM4 line and context lookups. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merges main at 2c14261 (#557, #578, #657, #658, #579). Only #578 (stabilize TM1 window identity) conflicted, in static_patterns_tool_misuse.py (7 hunks). Kept main's analyze() logic (_direct_shell_only, per-line context cache, classification_context/_bounded_context, line_number helper) and this PR's USES_RUNTIME_CHECK, check_runtime keyword, runtime_check() calls, and SourceLocationIndex/get_context_from_lines lookups backing line_number/ctx. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks! The newer shell-flow fix in #577 has landed on main and supersedes the AST-reparse change here. I kept that implementation and carried the remaining line-index and runtime safeguards into #741. Its regression confirms that 100 shell-flag matches use one AST parse and one scope index. This PR should not be merged over the newer implementation; I have left it open for tracking. |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @yashrajp22, thank you for this PR. The dense 256,000-character, 4,000-match test you added is what shows the per-line context cost that main still pays, and the deadline plumbing follows the runner's existing USES_RUNTIME_CHECK protocol cleanly.
This is my first review of this PR. The current head, f4432cc, is a maintainer merge of main that resolved the earlier conflict with #577. #577 (5ee48ef) replaced the per-match reparse with a single AST index (_build_variable_shell_ast_index) and moved variable reconciliation into postprocess. That superseded this branch's _VariableShellScopeIndex, so the merge dropped the index class, _index_scope and the deque import, and kept main's design. After that merge, the PR contributes three things:
- Line and context lookups through one
SourceLocationIndexand onecontent.splitlines()peranalyze()call (static_patterns_tool_misuse.py:4726-4734;locations.line_and_columnat 4777, 4823, 4854 and 4877). These replaceget_context/get_line_number, which rescan the whole file on every call. - Deadline plumbing:
USES_RUNTIME_CHECK = True(56), acheck_runtimekeyword (4719, 4723-4724), andruntime_check()calls in the TM1 loop (4769), the TM2-TM4 loops (4822, 4853, 4876), the Perl print helper (4764) and the non-deferred reconciliation block (4745-4752). - Six tests (
tests/unit/test_patterns_new.py:1530-1589).
Value and readiness: The remaining change is worth landing. #578 (dd95368) already replaced the per-finding get_line_number with a bisect and caches context per line on main (main static_patterns_tool_misuse.py:5041-5054). The cache only helps when several findings share a line, though. Each distinct line still calls get_context (main:5052), which runs content.splitlines() and get_line_number over the whole file (main common.py:151-154). On main, analyze() takes 0.99 s, 2.77 s and 9.44 s for 1,000, 2,000 and 4,000 use_shell pairs, so the cost grows quadratically.
I measured on main 2c14261, comparing main's code with a transcription (main's file plus this PR's added lines; no PR code was imported):
- Dense window,
analyze(..., defer_variable_reconciliation=True): 9.28 s on main vs 0.33 s with the change. An independent rerun on a loaded machine gave 8.26 s vs 0.86 s. Context building alone drops from 7.07 s to 0.009 s, and all 4,000 contexts are identical. - The same input through
run_static_patterns_with_ledger: 13.46 s vs 3.66 s (rerun: 9.41 s vs 2.60 s). Findings are identical after stripping IDs. - A Perl file with 12,000
printlines: 22.29 s vs 1.30 s (rerun: 22.70 s vs 1.36 s). The 30,720 findings are identical. - Context anchoring is fixed for files with CR-only, U+2028 or VT line breaks. Main's
get_contextfinds the column withcontent.rfind("\n")(maincommon.py:161). For a TM2 match after a 1,500-character line separated this way, main's 1,000-character context does not contain the match, and the PR's context does. LF and CRLF files are unchanged. Severity and confidence did not change in any case I tested.
The PR is not ready to merge. It now conflicts with main again, this time with #578 (finding 1), and no CI has run on this head. Findings 2-4 are non-blocking.
Material findings
-
[Blocker]
src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:4719: the PR conflicts with #578 (dd95368), and the resolution needs pieces from both sides.-
GitHub reports mergeable=CONFLICTING and mergeStateStatus=DIRTY.
git merge-tree --write-tree origin/main refs/remotes/prcur/790reports 7 conflict hunks, all in this file;tests/unit/test_patterns_new.pymerges cleanly. The result is the same against the newest origin/main, 2f93a86 (#721, which does not touch this file). -
The hunks, with main line numbers at 2c14261:
- Imports (main:53-60): main keeps
get_context; this PR swaps inSourceLocationIndexandget_context_from_lines. - Signature (main:5031-5033): #578's
_direct_shell_onlyagainst this PR'scheck_runtime(head:4719). ctx(main:5041-5054): #578'sline_numberbisect and per-linecontext_by_linecache against this PR's uncachedctx(head:4732-4734).- TM1 line and context (main:5104-5110):
line_number,classification_context, and_bounded_contextfor directshell=True, againstlocations.line_and_column(head:4777). - Hunks 5-7, the TM2, TM3 and TM4 line numbers (main:5246, 5276, 5298):
line_number(match.start())againstruntime_check()pluslocations.line_and_column(head:4822-4823, 4853-4854, 4876-4877).
- Imports (main:53-60): main keeps
-
Neither side works on its own:
- This PR's side of every hunk gives 9 ruff errors: F821
_direct_shell_onlyx3, F821classification_contextx4, F821get_contextat main:6673 (_reconcile_variable_shell_findingsin postprocess still calls it), and F841context_by_line. - It would also make
_DirectShellReplayLexical.analyze(main:6181-6186) raise TypeError, because that method passes_direct_shell_only=True. Ruff does not catch this. - Main's side of every hunk gives F821
check_runtime, F821SourceLocationIndex, F841locationsand F841lines.
- This PR's side of every hunk gives 9 ruff errors: F821
-
Consequence: the PR cannot merge as it stands. A quick resolution would either break postprocess for true-prefixed variable findings (NameError at main:6673), or drop #578's direct-shell replay mode and its bounded direct-shell context.
-
Expected fix: merge current main and resolve the hunks as follows.
- Imports: keep
get_context, which main:6673 still uses, and also importSourceLocationIndexandget_context_from_lines. - Signature: keep
check_runtime,_direct_shell_onlyanddefer_variable_reconciliation. ctx: keep main's per-linecontext_by_linecache. Replace onlyget_context(content, start)withget_context_from_lines(lines, line, column=column), takinglineandcolumnfromlocations.line_and_column(start).- TM1: take main's side unchanged.
- TM2-TM4: keep main's
line_number(match.start())and addruntime_check()before it.
Optionally, back main's
line_numberwithSourceLocationIndexand dropline_starts(main:5041), so only one line index is built per call. That is a cleanup, not a correctness requirement. - Imports: keep
-
Results on my transcription of this resolution:
ruff checkandruff format --checkpass.- Output is identical to main on 1,046
analyze()runs over 253 inputs. An independent check found it identical on 9,489 runs: 3,163 files throughanalyze, deferredanalyzeand the runner. - 4,349 tests from main's 35 tool-misuse test files pass, and all 6 transcribed new tests pass.
Taking this PR's uncached
ctxinstead also passes, but it changes context text (and only context text) when several findings share one long line. That happened only on a synthetic long-line file in both parity runs. Keeping main's cache is the behavior-preserving choice. -
A maintainer may push a prepared merge of main to this branch that follows this plan. If that happens, please pull it before pushing further commits rather than resolving again.
-
-
[Non-blocking]
tests/unit/test_patterns_new.py:1530: the new tests do not guard what this PR changes on current main.- I transcribed the six tests and ran them against main 2c14261. Five pass without the change: 1530, 1540, 1547, 1573 and 1578.
- Test 1530 takes about 9.2 s on main and about 0.8-1.1 s with the change. It only asserts
parse.call_count == 1and 4,000 TM1 findings, and main already parses once since #577. - Test 1559 fails on main only with
TypeError: analyze() got an unexpected keyword argument 'check_runtime'. It callsanalyzewith the defaultdefer_variable_reconciliation=False, so its 20th check fires inside the non-deferred reconciliation loop (head:4751-4752). The runner always defers for this module (headstatic_runner.py:919-920), so that loop never runs in scans. Removing the checks from the TM1 loop (4769) or from the TM2-TM4 loops (4822, 4853, 4876) would still pass the test. - The names
scope_indexandcaches_unparseable_windowrefer to the index that the merge removed. - Consequence: reverting the
get_context_from_lineschange, which is the PR's main remaining value, would pass every new test. The deadline checks on the production (deferred) path are not tested at all. - Expected fix:
- Add a test that fails on main. For example, patch
tm_mod.get_contextwithside_effect=AssertionError("full-file context")and assert thatanalyze(dense_window, "runner.py", "python", defer_variable_reconciliation=True)still returns 4,000 TM1 findings. After the finding 1 resolution,analyze()no longer callsget_context. - Add a deadline test that uses
defer_variable_reconciliation=Truewith an expiringcheck_runtime, or a runner-level test with a patched clock that asserts the module stops at the deadline. - Optionally, add a context-anchoring test for CR-only or U+2028 line breaks.
- Rename the two tests that mention the removed index.
- Add a test that fails on main. For example, patch
-
[Non-blocking]
src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:4726: the title and description still describe the removed scope index.- The title is "index variable shell-flag scopes once per window". The body says the PR builds "one scope index per matching source window", and that a baseline parsed the same source 120 times while the fix parsed it once. After f4432cc the diff contains no index, and main already parses once. I wrapped
parse_python_sourcearound main'sanalyze, and both the 4,000-match test input and a 120-match window parse exactly once. - "Check the artifact deadline while indexing" is only partly accurate. There is no new indexing, and the reconciliation checks at head:4745-4752 run only for direct
analyze()callers. - The description does not mention what the diff actually does: window-sized context lookups (head:4726-4734) with the speedups above, the context-anchoring change for CR, U+2028 and VT line breaks, and the
check_runtimeplumbing. - Consequence: the repository puts the PR title into merge commits (
merge_commit_message=PR_TITLE; squash merges use it as the headline). History would therefore describe work this change does not do, and reviewers would miss the context-text change for files with non-LF line breaks. - Expected fix: retitle, for example "fix(analyzer): reuse a line index for tool-misuse context lookups and honor the runner deadline". Rewrite the body to match the diff after the finding 1 resolution, with numbers measured against current main.
- The title is "index variable shell-flag scopes once per window". The body says the PR builds "one scope index per matching source window", and that a baseline parsed the same source 120 times while the fix parsed it once. After f4432cc the diff contains no index, and main already parses once. I wrapped
-
[Non-blocking]
src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:4822: this PR overlaps with open PR #741.- #741 (head f776f8a) makes the same line and context change in
analyze(). The 17 changed lines that touchSourceLocationIndex,get_context,get_line_number,line_and_columnandsplitlinesare identical in both diffs. - #741 enforces deadlines differently. It uses a
static_runner._ACTIVE_FINDING_BUDGETcontext variable anditer_pattern_matchesinstead of theUSES_RUNTIME_CHECKkeyword (mainstatic_runner.py:774-776, 1018-1019). git merge-tree refs/remotes/prcur/741 refs/remotes/prcur/790conflicts in 3 hunks, all at the TM2-TM4 loop heads, where this PR'sruntime_check()meets #741'siter_pattern_matches. The identical context hunk merges cleanly.- Both PRs also conflict with #578 on the same lines, so the overlap is three-way.
- #741 does not put the Perl helper in
analyze()under a deadline: it keeps_perl_literal_print_shell_text(content, lambda: None)there, as main does at main:5085. - Consequence: whichever PR lands second has to reconcile the deadline lines and the merge with main's
line_numberandcontext_by_line. With both PRs merged, a TM2-TM4 match that creates a finding would be checked up to four times: twice initer_pattern_matches, once by this PR, and once by the runner'sobserve_creation(mainstatic_runner.py:831-833). That is harmless but redundant. - Expected fix: no code change is needed here now; the two PRs need to be sequenced. Beyond #741, this PR's distinct contribution is the Perl helper check (head:4764). Its TM1 loop-head check (head:4769) only partly overlaps #741: #741 routes
TM1_PATTERNSthroughiter_pattern_matchesbut not_tm1_shell_candidates.
- #741 (head f776f8a) makes the same line and context change in
PIC tradeoffs:
- Performance.
SourceLocationIndex(content)andcontent.splitlines()are built on everyanalyze()call, for every file type, even when nothing matches. In return, each context lookup reads a window of about 7 lines instead of the whole file. Inputs without many distinct finding lines showed no measurable overhead: a 256K Perl concatenation file took 0.40 s with and without the change, and 8,000 direct calls on one line took 0.69 s vs 0.70 s. After the merge, main'sline_starts(main:5041) is a second line index unless the two are consolidated. - Integrity (deadline). The checks that run in scans are the TM1-TM4 loop checks and the Perl helper check, because the runner passes
check_runtimewhenUSES_RUNTIME_CHECKis set (mainstatic_runner.py:1018-1019). The four checks at head:4745-4752 run only for direct callers, since the runner always defers reconciliation (mainstatic_runner.py:1020-1021). The runner'sobserve_creation(mainstatic_runner.py:831-833) already checks the deadline whenever a finding is created. What this PR adds is finer granularity: on skipped or deduplicated TM1 candidates, on TM2-TM4 iterations, and inside the Perl print reconstruction (52,203 checks on the 12,000-line Perl file, which still took 1.36 s in total). Whenanalyzeraises at the deadline, the runner's existing partial-findings path handles it (mainstatic_runner.py:1037-1056). - Compatibility.
check_runtimeis keyword-only with a default, and no other caller passes it._DirectShellReplayLexical(main:6174-6186) does not declareUSES_RUNTIME_CHECK, so after the merge its replayedanalyze(_direct_shell_only=True)gets nocheck_runtimeand relies onobserve_creation, as it does on main today. Context text changes only for files with non-LF line breaks, andFinding.fingerprint()(mainmodels.py:165-211) is built from the match and source provenance, not from the context.
Verification and gaps:
- PR state via gh (read-only): head f4432cc, mergeable=CONFLICTING and mergeStateStatus=DIRTY, no check runs on this head, and no reviews.
- Conflicts: with main through #578 (finding 1), unchanged on origin/main 2f93a86; and with open PR #741 (finding 4). #741 also conflicts with main on its own.
- Measurements used main 2c14261's own code in its venv (Python 3.13) and my transcriptions of this PR's added lines and of the six new tests. The resolution I measured in finding 1 is my own transcription. Two separate transcriptions agree on lint, parity and the new tests.
- Gaps:
- I did not build a combined main + #741 + #790 tree.
- The parity corpus is repository files plus synthetic edge cases. I did not run an external skill corpus or the PR body's installed-wheel scans.
- I did not measure end-to-end deadline response with a patched clock.
- Timings are machine-dependent, and CI uses Python 3.12.
- I did not run the PR's tests or code, per policy.
Decision: Changes Requested (reviewed head f4432cc94f5398f07789b7acba18ae865f672a10)
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@yashrajp22 thank you for the dense-window and Perl print measurements behind this branch. They showed that main still pays for a whole-file get_context on every distinct finding line.
Value and readiness
This is a re-review at 5dfbf1b. You have not pushed since my review at f4432cc. The head moved because of a conflict-resolution merge of main pushed from the bot account (960be30), followed by two automated main merges (58a8545 and 5dfbf1b). The resolution is correct, and the only blocker from my last review is gone. The PR now merges cleanly with main, and CI passed on 960be30. The two later merges do not touch static_patterns_tool_misuse.py.
Your 2026-10-09 comment and the current description say this branch should not be merged over the newer implementation. They say it stays open only for tracking, with the line-index and deadline work carried into #741, which is still open, CONFLICTING and has changes requested. My previous review did not account for that. A maintainer needs to pick the vehicle: close #790 and land the work through #741, or adopt #790 and rebase #741 onto it. The bot account pushed 960be30, so this review cannot approve. A human maintainer must approve or close this PR.
Material findings
Prior findings
-
Conflict with #578 (blocker): resolved by 960be30.
- The imports keep
get_context, which postprocess still calls atstatic_patterns_tool_misuse.py:6687, and addSourceLocationIndexandget_context_from_lines.LOGICAL_LINE_BREAKandbisect_rightare still used elsewhere. - The signature keeps
check_runtime,_direct_shell_onlyanddefer_variable_reconciliation._DirectShellReplayLexical(6188-6199) still passes_direct_shell_only=Trueand receives the no-op deadline. line_number()now returnslocations.line_and_column(start)[0].SourceLocationIndexbuilds its line starts withlogical_line_starts, the sameLOGICAL_LINE_BREAKscan that main used forline_starts, and the bisect returns the same 1-based line for every offset. This is the optional single-index consolidation I suggested last time.ctx()keeps main's per-line cache and fills it fromget_context_from_lines(lines, line, column=column).str.splitlines()splits on the same separators asLOGICAL_LINE_BREAK, so the line indices agree. LF and CRLF context is unchanged, and CR-only and U+2028 context is now anchored correctly.- TM1 keeps main's
classification_context/_bounded_contextand adds only the loop-headruntime_check(). TM2-TM4 keepline_number(match.start())withruntime_check()before it.
The inline thread on the signature can be resolved.
- The imports keep
-
Tests do not guard the change: still open. No test has changed since f4432cc.
tests/unit/test_patterns_new.py:1532still asserts only the parse count and the TM1 count. Thecheck_runtimetest still trips inside the non-deferred reconciliation loop, which scans never run. -
Title and description: still open for the title. You edited the body on 10-09 08:45, before my last review, and it now describes the PR as superseded. My earlier quote about 120 parses came from the older body. The title still names the removed scope index, and it would become the merge-commit headline.
-
Overlap with #741: still open. #741 (1821355) still carries the same
SourceLocationIndex/get_context_from_lineschange, with its own deadline mechanism in the TM2-TM4 loop heads.
New: no code defects. Open #783 also edits the TM3 loop head and conflicts textually with this PR there. Keep both lines.
PIC tradeoffs
- Which PR carries the change. If #790 is the vehicle, address finding 2 first. That means a test that fails when
get_context_from_linesis reverted, plus a deadline test on the deferred path. Then fix the title and rebase #741. If #741 is the vehicle, close #790.
Verification and gaps
- Compared the effective diff at f4432cc, 960be30, 58a8545 and 5dfbf1b. Against main 7ee4371, the source diff at 5dfbf1b (blob 0d687a77 to c553fe25) is identical to the one at 960be30; the test hunk only moved down two lines.
git merge-treewith main is clean. - CI was 6/6 green on 960be30. The two automated merges show
action_requiredruns that are waiting for approval. That is an infrastructure state, not a defect. - Read
SourceLocationIndex,logical_line_starts,get_contextandget_context_from_linesincommon.py, and the runner'sUSES_RUNTIME_CHECK/defer_variable_reconciliationwiring. - This was a static review only. I did not run the PR's code or tests, and I did not build a combined #741 + #790 tree.
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Conflict came from main's TM3 umask fix (94547bd/3eb3cd93, PR #783), which added the _is_assigned_value skip to the TM3 match loop that this branch instruments with runtime_check(). Kept both: runtime_check() runs first, then main's assigned-umask skip. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@yashrajp22 thank you for the follow-up in 63a1eab. The new dense deferred-path test (tests/unit/test_patterns_new.py:1542) fails on main with AssertionError: full-file context and passes with this change, so the context-index change now has a real regression guard. The deadline tests at :1580 and :1609 run on the deferred path that scans use, and removing any one of the TM1-TM4 loop checks makes exactly one of them fail. You also renamed the tests that named the removed index and rewrote the title and description to match the diff.
This re-review covers head b99a151. Your only non-merge commit since my last review is 63a1eab, which changes tests only. The source diff is unchanged: one SourceLocationIndex plus one splitlines() per analyze() call, feeding main's per-line context cache (static_patterns_tool_misuse.py:5065-5082), and the USES_RUNTIME_CHECK deadline plumbing (:68, :5057, :5062) with checks in the Perl helper (:5117) and the TM1-TM4 loops (:5133, :5279, :5310, :5333). Nothing I found blocks the change. The remaining items are test-strength gaps and a sequencing note.
Previous findings:
- Review 5473546685, finding 1 [Blocker], conflict with #578 (thread at
:5057): Resolved by maintainer merge 960be30. The signature keepscheck_runtime,_direct_shell_onlyanddefer_variable_reconciliation(:5057-5059).ctx()keeps main'scontext_by_linecache, filled byget_context_from_lines(:5076-5082).get_contextis still imported for postprocess (:59), and_DirectShellReplayLexicalstill passes_direct_shell_only=True. - Review 5473546685, finding 2 [Non-blocking], tests do not guard the change (thread at
:1532): Partly resolved. Both requested tests are in::1542fails on main, and:1580/:1609are deferred-path deadline tests that each guard a TM1-TM4 loop check. Two pieces your reply and the description call covered are still unguarded: the Perl helper deadline and the non-LF anchoring (material findings 1 and 2). - Review 5473546685, finding 3 [Non-blocking], title and description (thread at
:5065): Resolved. The title is now "fix(analyzer): reuse tool-misuse context indexes and honor deadlines". The scope-index and 120-to-1 parse claims are gone. One sentence about test coverage still says too much (see findings 1 and 2). - Review 5473546685, finding 4 [Non-blocking], overlap with #741 (thread at
:5279): Still open. This is a maintainer sequencing decision, and you do not need to change any code. #741 (ca0d62c) still makes the sameSourceLocationIndex/get_context_from_lineschange and still calls_perl_literal_print_shell_text(content, lambda: None).git merge-treeof the two heads conflicts in 3 hunks: thectxcolumn line, the TM3 loop head and the TM4 loop head. Your reply says the combined tests stop the timed routing from being dropped. That holds for TM1-TM4 but not for the Perl helper (finding 1). The Perl line itself merges cleanly, so it survives either landing order unless someone resolves it by hand. - Review 5500976489, "New" note, #783 edits the TM3 loop head: Still open, being resolved by a maintainer merge. #783 landed as 8a4c03a, and the PR now conflicts with main cf5cac7 in exactly one hunk. Main's side is
if skip_assigned and _is_assigned_value(content, match.start()): continue(main:5532-5533). This PR's side isruntime_check()(:5310).tests/unit/test_patterns_new.pymerges cleanly. See Verification for the order the merge needs.
Material findings
-
[Non-blocking]
tests/unit/test_patterns_new.py:1606: the Perl case oftest_deferred_tool_rules_and_perl_helper_honor_deadlinepasses even when the Perl helper's deadline check is removed.- How I checked: I built a transcription (main plus this PR's added lines) and restored
_perl_literal_print_shell_text(content, lambda: None)at:5117. - Result:
"print 'rm -rf /';"x300 still raises at call 20, andcalls == 20still holds. That input creates TM1 candidates, so with the helper check gone the TM1 loop check at:5133supplies the 20 calls. With the helper check in place, the 20th call fires inside_perl_literal_print_shell_text. - Why it matters: the helper check is the part this PR adds beyond #741.
- Suggested fix: use input with no shell text, for example
"print 'hello world';"x300. With the change it raises at call 20 inside the helper. With the helper check removed it makes 1 check call and does not raise. On main it fails with TypeError.
- How I checked: I built a transcription (main plus this PR's added lines) and restored
-
[Non-blocking]
tests/unit/test_patterns_new.py:1553:test_context_anchor_uses_logical_line_breakspasses on main for\r, U+2028 and\v.- What I ran: the test's input through main's own
analyze(). For all three separators main returns line 2 with the needle in the context. - Why it passes on main: main's
get_contexttakes the column fromrfind("\n")and then clamps it to the line length (common.py:161-162). Nothing follows the match, so the window still ends on it. - Suggested fix: append
+ " " * 1500after the needle. With that tail, main's context loses the needle for all three separators, and the transcribed change keeps it. - The windowed lookup is still guarded by
:1542, which fails ifctx()goes back toget_context. The anchoring fix is not. - Until findings 1 and 2 are fixed, the description's sentence "Additional tests cover deferred TM1–TM4 deadlines, the Perl helper, and non-LF context anchoring" holds only for TM1-TM4. Please strengthen the two tests or reword that sentence.
- What I ran: the test's input through main's own
-
[Non-blocking]
src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:68: no test checks theUSES_RUNTIME_CHECK = Trueopt-in.- How the flag is used: the runner passes
check_runtimeonly when this flag is set (mainstatic_runner.py:778,:1018-1019). Without it, every new check is a no-op in scans. - Why no test notices: all the new deadline tests call
analyze()directly withcheck_runtime=. A mutant without the flag passes all 9 transcribed tests. - Measured effect (transcription, clock that expires 0.05 s after
analyze()starts, 250 KB Perl input): with the flag,analyze()stops at 0.05 s; without it, it runs to about 0.42 s. - The impact is small against the 300 s default budget, and
static_patterns_prompt_injection.py:43andstatic_patterns_anti_refusal.py:51already set the same flag without a test. - Optional fix: a one-line
assert tm_mod.USES_RUNTIME_CHECK is True, or a runner-level test that spies on theanalyze()kwargs.
- How the flag is used: the runner passes
Optional cleanup: :1532 (test_tm1_parses_dense_shell_flags_once) still passes on main, where it takes about 13 s. It is now covered by :1542, so you could drop it to save CI time.
Sequencing note for maintainers: if this PR lands first, #741 needs to merge main and keep iter_pattern_matches together with runtime_check() in the TM3 and TM4 loops, and keep skip_assigned in TM3. Separately, your 2026-10-09 conversation comment saying this branch should not be merged over the newer implementation is still on the PR. Please withdraw or update it, so it is clear that #790 is meant to merge.
Verification and gaps:
- Head. b99a151, the newest head I could check. A live
ghre-check at write time failed with network timeouts. Earlier today gh showed this head as CONFLICTING/DIRTY. - Conflict.
git merge-tree --write-tree origin/main refs/remotes/pr4/790gives one conflict hunk, at the TM3 loop head. A maintainer is resolving it with a prepared merge of main. The merge must keep both lines withruntime_check()first:- On
"old = os.umask(0)\n"x3000,runtime_check()first makes 3001 checks. - Putting the
skip_assignedcheck first makes 1 check, so the deadline never fires there.
- On
- Checks on that resolution (transcription):
ruff checkandruff format --checkpass.test_patterns_new.pyandtest_patterns.py: 1,141 passed.tests/nodeswithout integration or provider tests: 9,714 passed, 4 xfailed.- 4,954 tests from main's tool-misuse-related test files pass.
- Over 1,156
analyze()runs (repo files and synthetic inputs, deferred and not), findings match main. The only differences are in context text, on 50 synthetic non-LF inputs.
- CI. No runs exist on b99a151, because the conflict blocks
pull_requestruns. The two automated merges before it, 4500c63 and 590c464, have runs waiting inaction_required. The last run, on 63a1eab (run 38514820846), passed all 6 checks. The PR's patch-id (7f5970499798) is identical at 63a1eab and b99a151, but that run predates #783 and #849 on main. CI needs to pass on the merge head before this merges. - Test guards (mutation runs on the transcription):
- Reverting
ctx()toget_contextfails exactly one test. - Removing the check from any one of the TM1-TM4 loops fails exactly one test.
- Removing the Perl helper check, the
USES_RUNTIME_CHECKflag, or the non-deferred reconciliation checks (:5098-5105) fails none. The reconciliation checks only run for direct callers, because the runner always defers reconciliation.
- Reverting
- Timings (main vs transcription, local Python 3.13; findings identical in each):
- Dense 256K deferred
analyze(): 9.83 s vs 0.44 s. - 12,000-line Perl file: 43.75 s vs 2.23 s.
- Runner on the dense file: 13.81 s vs 3.85 s.
- Dense 256K deferred
- Gaps.
- I did not run the PR's tests or code, per policy. Every result above comes from main's code or from my own transcriptions.
Decision: Approved (reviewed head b99a151754d3c1a13c1c3dd6c7339c52bbd6e161; current head cf07fcb7d56612c807930edc81820380672162ca is a maintainer merge of main resolving the #783 conflict in the TM3 loop, with runtime_check() kept before the skip_assigned check)
…deadlines (#790) Backport the tool-misuse analyzer change that reuses one SourceLocationIndex and split-line buffer for finding context, anchors context after CR/U+2028/VT breaks, and checks the runner deadline (USES_RUNTIME_CHECK) on every scan loop and in the Perl helper. Main-only TM3 umask and references/ triage are not pulled in. (cherry picked from commit fa8043c) Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
List the three security fixes backported onto the v2.13.0 base (#790, document the file-output compatibility notes: POSIX-only no-follow output, 0600 report files, the recursive-scan exit code without #662, and that the gap-fill fix applies to the contributed batch scanner. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve the tool-misuse analyze() conflicts with #790: ctx() keeps main's single line_and_column() lookup (dropping the PR's duplicate column lookup), and the TM4 loop keeps the PR's static_runner.iter_pattern_matches routing plus main's per-match runtime_check(). All other #790 plumbing auto-merged. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dense tool-misuse findings previously rebuilt context from the whole file for each new finding line. This change reuses one line index and split-line buffer while preserving the existing per-line context cache. It also anchors context correctly after CR, U+2028 and VT line breaks and checks the artifact deadline on the deferred scan path and inside the Perl helper.
The current main merge preserves direct-shell replay, classification, and AST reconciliation. This PR overlaps with #741’s line-index work; the combined change must keep #741’s interruptible matching together with these deadline checks.
Validation: 737 tests pass from source and a fresh installed wheel. The new dense deferred-path regression fails on main at the old whole-file context lookup and passes here with all 4,000 findings. Additional tests cover deferred TM1–TM4 deadlines, the Perl helper, and non-LF context anchoring.