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

[yashrajp22]

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]

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]

[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)

[rng1995]

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