Skip to content

fix(analyzer): reuse tool-misuse context indexes and honor deadlines - #790

Merged
rng1995 merged 27 commits into
mainfrom
yashraj/cache-shell-flag-scope-analysis
Oct 19, 2026
Merged

rng1995 merged 27 commits into
mainfrom
yashraj/cache-shell-flag-scope-analysis

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

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.

yashrajp22 and others added 7 commits October 7, 2026 12:43
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>
rng1995 and others added 2 commits October 8, 2026 16:02
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>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

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 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 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 SourceLocationIndex and one content.splitlines() per analyze() call (static_patterns_tool_misuse.py:4726-4734; locations.line_and_column at 4777, 4823, 4854 and 4877). These replace get_context/get_line_number, which rescan the whole file on every call.
  • Deadline plumbing: USES_RUNTIME_CHECK = True (56), a check_runtime keyword (4719, 4723-4724), and runtime_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 print lines: 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_context finds the column with content.rfind("\n") (main common.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

  1. [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/790 reports 7 conflict hunks, all in this file; tests/unit/test_patterns_new.py merges 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:

      1. Imports (main:53-60): main keeps get_context; this PR swaps in SourceLocationIndex and get_context_from_lines.
      2. Signature (main:5031-5033): #578's _direct_shell_only against this PR's check_runtime (head:4719).
      3. ctx (main:5041-5054): #578's line_number bisect and per-line context_by_line cache against this PR's uncached ctx (head:4732-4734).
      4. TM1 line and context (main:5104-5110): line_number, classification_context, and _bounded_context for direct shell=True, against locations.line_and_column (head:4777).
      5. Hunks 5-7, the TM2, TM3 and TM4 line numbers (main:5246, 5276, 5298): line_number(match.start()) against runtime_check() plus locations.line_and_column (head:4822-4823, 4853-4854, 4876-4877).
    • Neither side works on its own:

      • This PR's side of every hunk gives 9 ruff errors: F821 _direct_shell_only x3, F821 classification_context x4, F821 get_context at main:6673 (_reconcile_variable_shell_findings in postprocess still calls it), and F841 context_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, F821 SourceLocationIndex, F841 locations and F841 lines.
    • 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.

      1. Imports: keep get_context, which main:6673 still uses, and also import SourceLocationIndex and get_context_from_lines.
      2. Signature: keep check_runtime, _direct_shell_only and defer_variable_reconciliation.
      3. ctx: keep main's per-line context_by_line cache. Replace only get_context(content, start) with get_context_from_lines(lines, line, column=column), taking line and column from locations.line_and_column(start).
      4. TM1: take main's side unchanged.
      5. TM2-TM4: keep main's line_number(match.start()) and add runtime_check() before it.

      Optionally, back main's line_number with SourceLocationIndex and drop line_starts (main:5041), so only one line index is built per call. That is a cleanup, not a correctness requirement.

    • Results on my transcription of this resolution:

      • ruff check and ruff format --check pass.
      • 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 through analyze, deferred analyze and 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 ctx instead 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.

  2. [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 == 1 and 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 calls analyze with the default defer_variable_reconciliation=False, so its 20th check fires inside the non-deferred reconciliation loop (head:4751-4752). The runner always defers for this module (head static_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_index and caches_unparseable_window refer to the index that the merge removed.
    • Consequence: reverting the get_context_from_lines change, 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_context with side_effect=AssertionError("full-file context") and assert that analyze(dense_window, "runner.py", "python", defer_variable_reconciliation=True) still returns 4,000 TM1 findings. After the finding 1 resolution, analyze() no longer calls get_context.
      • Add a deadline test that uses defer_variable_reconciliation=True with an expiring check_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.
  3. [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_source around main's analyze, 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_runtime plumbing.
    • 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.
  4. [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 touch SourceLocationIndex, get_context, get_line_number, line_and_column and splitlines are identical in both diffs.
    • #741 enforces deadlines differently. It uses a static_runner._ACTIVE_FINDING_BUDGET context variable and iter_pattern_matches instead of the USES_RUNTIME_CHECK keyword (main static_runner.py:774-776, 1018-1019).
    • git merge-tree refs/remotes/prcur/741 refs/remotes/prcur/790 conflicts in 3 hunks, all at the TM2-TM4 loop heads, where this PR's runtime_check() meets #741's iter_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_number and context_by_line. With both PRs merged, a TM2-TM4 match that creates a finding would be checked up to four times: twice in iter_pattern_matches, once by this PR, and once by the runner's observe_creation (main static_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_PATTERNS through iter_pattern_matches but not _tm1_shell_candidates.

PIC tradeoffs:

  • Performance. SourceLocationIndex(content) and content.splitlines() are built on every analyze() 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's line_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_runtime when USES_RUNTIME_CHECK is set (main static_runner.py:1018-1019). The four checks at head:4745-4752 run only for direct callers, since the runner always defers reconciliation (main static_runner.py:1020-1021). The runner's observe_creation (main static_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). When analyze raises at the deadline, the runner's existing partial-findings path handles it (main static_runner.py:1037-1056).
  • Compatibility. check_runtime is keyword-only with a default, and no other caller passes it. _DirectShellReplayLexical (main:6174-6186) does not declare USES_RUNTIME_CHECK, so after the merge its replayed analyze(_direct_shell_only=True) gets no check_runtime and relies on observe_creation, as it does on main today. Context text changes only for files with non-LF line breaks, and Finding.fingerprint() (main models.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)

Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py
Comment thread tests/unit/test_patterns_new.py
Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py
Comment thread src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py

@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]

@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

  1. Conflict with #578 (blocker): resolved by 960be30.

    • The imports keep get_context, which postprocess still calls at static_patterns_tool_misuse.py:6687, and add SourceLocationIndex and get_context_from_lines. LOGICAL_LINE_BREAK and bisect_right are still used elsewhere.
    • The signature keeps check_runtime, _direct_shell_only and defer_variable_reconciliation. _DirectShellReplayLexical (6188-6199) still passes _direct_shell_only=True and receives the no-op deadline.
    • line_number() now returns locations.line_and_column(start)[0]. SourceLocationIndex builds its line starts with logical_line_starts, the same LOGICAL_LINE_BREAK scan that main used for line_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 from get_context_from_lines(lines, line, column=column). str.splitlines() splits on the same separators as LOGICAL_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_context and adds only the loop-head runtime_check(). TM2-TM4 keep line_number(match.start()) with runtime_check() before it.

    The inline thread on the signature can be resolved.

  2. Tests do not guard the change: still open. No test has changed since f4432cc. tests/unit/test_patterns_new.py:1532 still asserts only the parse count and the TM1 count. The check_runtime test still trips inside the non-deferred reconciliation loop, which scans never run.

  3. 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.

  4. Overlap with #741: still open. #741 (1821355) still carries the same SourceLocationIndex/get_context_from_lines change, 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_lines is 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-tree with main is clean.
  • CI was 6/6 green on 960be30. The two automated merges show action_required runs that are waiting for approval. That is an infrastructure state, not a defect.
  • Read SourceLocationIndex, logical_line_starts, get_context and get_context_from_lines in common.py, and the runner's USES_RUNTIME_CHECK/defer_variable_reconciliation wiring.
  • 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.

@yashrajp22 yashrajp22 changed the title fix(analyzer): index variable shell-flag scopes once per window fix(analyzer): reuse tool-misuse context indexes and honor deadlines Oct 14, 2026
github-actions Bot and others added 4 commits October 14, 2026 11:22
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 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]

@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 keeps check_runtime, _direct_shell_only and defer_variable_reconciliation (:5057-5059). ctx() keeps main's context_by_line cache, filled by get_context_from_lines (:5076-5082). get_context is still imported for postprocess (:59), and _DirectShellReplayLexical still 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: :1542 fails on main, and :1580/:1609 are 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 same SourceLocationIndex/get_context_from_lines change and still calls _perl_literal_print_shell_text(content, lambda: None). git merge-tree of the two heads conflicts in 3 hunks: the ctx column 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 is runtime_check() (:5310). tests/unit/test_patterns_new.py merges cleanly. See Verification for the order the merge needs.

Material findings

  1. [Non-blocking] tests/unit/test_patterns_new.py:1606: the Perl case of test_deferred_tool_rules_and_perl_helper_honor_deadline passes 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, and calls == 20 still holds. That input creates TM1 candidates, so with the helper check gone the TM1 loop check at :5133 supplies 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.
  2. [Non-blocking] tests/unit/test_patterns_new.py:1553: test_context_anchor_uses_logical_line_breaks passes 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_context takes the column from rfind("\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 + " " * 1500 after 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 if ctx() goes back to get_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.
  3. [Non-blocking] src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:68: no test checks the USES_RUNTIME_CHECK = True opt-in.

    • How the flag is used: the runner passes check_runtime only when this flag is set (main static_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 with check_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:43 and static_patterns_anti_refusal.py:51 already 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 the analyze() kwargs.

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 gh re-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/790 gives 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 with runtime_check() first:
    • On "old = os.umask(0)\n" x3000, runtime_check() first makes 3001 checks.
    • Putting the skip_assigned check first makes 1 check, so the deadline never fires there.
  • Checks on that resolution (transcription):
    • ruff check and ruff format --check pass.
    • test_patterns_new.py and test_patterns.py: 1,141 passed.
    • tests/nodes without 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_request runs. The two automated merges before it, 4500c63 and 590c464, have runs waiting in action_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() to get_context fails 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_CHECK flag, or the non-deferred reconciliation checks (:5098-5105) fails none. The reconciliation checks only run for direct callers, because the runner always defers reconciliation.
  • 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.
  • Gaps.
    • The combined #741 + #790 check was a git merge-tree analysis only. I did not build or test a combined tree.
    • I could not verify the description's "737 tests pass from source and a fresh installed wheel".
    • CI uses Python 3.12; my runs used 3.13.
  • 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)

@rng1995
rng1995 merged commit fa8043c into main Oct 19, 2026
5 checks passed
@rng1995
rng1995 deleted the yashraj/cache-shell-flag-scope-analysis branch October 19, 2026 10:19
rng1995 added a commit that referenced this pull request Oct 19, 2026
…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>
rng1995 added a commit that referenced this pull request Oct 19, 2026
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>
rng1995 added a commit that referenced this pull request Oct 19, 2026
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>
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