fix(patterns): detect constructed sensitive paths

[deepujain]

Fixes #477.

Resolve literal-only os.path.join(...) calls before matching PE3 credential paths, preserving the detector for constructed paths without evaluating arbitrary expressions.

Validation: python3 -m py_compile src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py tests/unit/test_patterns.py and git diff --check. The isolated checkout lacks the project test environment; hosted CI will run the full suite.

[MohammedAlkindi]

Two things measured by running analyze() at 03e8f56 and at this head.

The new emitter appends without asking whether the existing PE3 loop already matched that line. os.path.join('.ssh/id_rsa', 'x') gives one PE3 on base and two here, the second carrying matched_text .ssh/id_rsa/x — the same line reported twice at HIGH/0.9. .aws/credentials behaves the same way.

Separately, [^()\n]+ in call_pattern excludes newlines, so the recognizer stops seeing the call as soon as it wraps. Split across two lines it gives 0 on both refs, and so do from os.path import join and import os.path as p. The single-line os.path.join form does fire, so the feature works; it is narrower than the description suggests, and any formatter wrapping a long call removes it.

[rng1995]

[SkillSpector Review]

Reviewed current head f850169695b2c11875c574df466a9d8134eec95b. The literal single-line os.path.join('/etc', 'passwd') case is detected and every hosted check passes, but the added recognizer is not yet safe to merge.

First, the new emitter independently appends PE3 after the existing PE3 loop, so a line such as os.path.join('.ssh/id_rsa', 'x') produces two HIGH/0.9 findings for the same line and can inflate scoring. Second, [^()\n]+ makes the feature disappear as soon as an otherwise identical call is formatted across lines; imported/aliased join forms are also outside the advertised construction coverage.

Deduplicate/choose the best PE3 finding per source occurrence and use the shared Python AST to resolve literal join calls without reparsing (or consume the cached tree supplied by the analyzer path). Add duplicate, multiline, and supported alias/import regressions. The inline comments identify both defects.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 9d365eded87f5dae23bf4a3225b35556d7b8f0ba against both prior findings, the complete analyzer/test diff, surrounding PE3 aggregation, and exact-head checks.

Neither required correction is present in the current diff. The recognizer is still a single-line os.path.join(...) regex, so multiline and supported imported/aliased forms remain uncovered. It also still appends a second PE3 independently of the existing PE3 loop, allowing one source occurrence to produce duplicate HIGH findings and inflate scoring. Resolve literal joins through the shared cached AST (or an equivalently complete bounded representation), deduplicate per occurrence, and add multiline/alias/duplicate regressions. I have not duplicated the existing inline findings.

All six exact-head checks pass, but the unresolved coverage and scoring regressions plus active change requests block merging.

Priority: P1 — duplicated HIGH findings and missed path constructions directly affect risk scoring.

[deepujain]

All review comments are addressed on the current head 1782279:

• rng1995 P1 (newline losing the detector and regex unable to resolve aliased joins): addressed by resolving the bounded literal call from the shared/cached Python AST in 1782279 (fix(patterns): preserve shared Python AST cache), with multiline plus import/alias regressions.
• rng1995 P1 (duplicate PE3 emission for one occurrence): addressed with occurrence-level deduplication and a duplicate regression.

CI is green on this head: https://github.com/NVIDIA/SkillSpector/actions/runs/35404568143 (lint, test-unit, docker-smoke, DCO). Per your 2026-09-18 note, this PR is ready for re-review.

[deepujain]

Addressed both open P1 threads in 8262737:

• Multiline/alias coverage: the constructed-path recognizer now resolves literal os.path.join calls from the Python AST instead of the single-line regex, so wrapped calls and supported import spellings (from os.path import join, import os.path as p, from os import path) are recognized. Only fully-literal argument lists resolve; dynamic args stay with the existing pattern loop. Regressions added for multiline, the three alias spellings, and the original single-line case.
• Duplicate PE3: constructed findings now deduplicate against PE3 findings the pattern loop already emitted for the same line, keeping the best-confidence finding per line (same aggregation as PE4/PE5). os.path.join('.ssh/id_rsa', 'x') now yields one PE3, with a pinned regression.

CI is re-running on this head.

[deepujain]

Follow-up: 8262737 broke the parse-once invariant (test_graph_scan_parses_python_once_before_parallel_analyzers) because the AST resolver parsed every Python file unconditionally. 40cea88 gates the local parse behind a cheap join() pre-check so files without a plausible join call never parse; the module stays lexical, preserving windowed and normalized-view scans for all file types. The 329 pattern/runner tests pass locally. CI is re-running on this head.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 40cea883f8019ff0db6a15edd5b7f916aac1ab4f, including the complete two-file diff, both previous review threads and author replies, the shared AST/parser and static-runner integration, the new regression assertions, and current CI.

The original single-line duplicate and the explicit multiline/direct-import examples are addressed. Both earlier threads are marked resolved, but the implementation does not yet satisfy the full corrections:

• A renamed import (from os.path import join as j) is rejected by the new textual precheck before alias resolution runs.
• Wrapping the duplicate example across lines still emits both the raw-literal and constructed-path findings because deduplication compares only the call's starting line.
• Files containing a plausible join still call parse_python_source directly, bypassing the prewarmed cache. The current parse-once graph fixture contains no join, so green CI does not cover this remainder.

The inline comments give the remaining cases and expected regression coverage. All six reported CI checks pass; gh pr checks --required reports no required checks. Tests were inspected, not executed locally. Changes remain required; no approval or merge is submitted.

[rng1995]

[P1] Let renamed join imports reach alias resolution. from os.path import join as j\ntarget = j('/etc', 'passwd')\n contains no join(, so this precheck returns before resolve_call_name can map j to os.path.join. The credential path is split across literals, so the lexical PE3 loop cannot recover it either. Remove this spelling-dependent gate or make it cover imported local names, and add a renamed-import regression alongside the existing direct-import cases.

[deepujain]

Done: the pre-check is now built from the import aliases, so every local name bound to os.path or os.path.join (e.g. j from from os.path import join as j) counts as a plausible join call before alias resolution runs. Regression: test_pe3_aliased_join_imports_are_detected now includes the renamed-import case.

[rng1995]

[P2] Reuse the scan's parsed tree for files that contain join calls. parse_python_source calls ast.parse unconditionally; it does not consult the runtime cache. This module also lacks the runner's shared-AST hook, so the textual gate only preserves parse-once behavior for files with no plausible join. Route this supplemental analysis through the cached AST (while retaining the lexical/windowed scans), and extend test_graph_scan_parses_python_once_before_parallel_analyzers with a literal os.path.join call so it verifies that the graph still performs one parse.

[deepujain]

Done: the module stays lexical so the windowed and normalized views keep running, and the constructed-path analysis now reuses the scan shared parse via the new peek_python_ast helper (exact-content hit only, never parses or writes), published to analyze() by node() through a contextvar. Standalone callers keep one on-demand parse. The extended test_graph_scan_parses_python_once_before_parallel_analyzers now includes a literal os.path.join call and still asserts a single parse.

[rng1995]

[P1] Deduplicate the whole call occurrence, including wrapped arguments. With import os\np = os.path.join(\n '.ssh/id_rsa', 'x'\n)\n, the lexical PE3 result starts on line 3, while the constructed result is anchored to node.lineno on line 2. This equality therefore misses the existing finding and appends a second HIGH/0.9 result for the same credential-path occurrence. Carry the call's source span and match raw findings belonging to that span, preserving unrelated occurrences; add a multiline version of the duplicate regression.

[deepujain]

Done: constructed hits now carry the call (start_line, end_line) span, and dedup matches raw PE3 findings anywhere inside that span, so a wrapped join call yields one PE3 while unrelated occurrences on other lines are preserved. Regression: test_pe3_multiline_constructed_join_is_deduplicated.

[MohammedAlkindi]

Confirmed on 56d121c — the alias-aware gate resolves the renamed import, and a file whose only join is ','.join(parts) now parses once where 40cea88 parsed it twice. One regression comes with it. Routing the constructed-path analysis through peek_python_ast needs a whole-file content match, but above SECURITY_VIEW_WINDOW_CHARS (256,000, static_runner.py:108) the runner hands lexical modules window slices rather than the whole file, so the peek always misses. Same os.path.join('/etc', 'passwd') through the full graph: at 49,992 characters both heads report PE3, while at 376,792 and 436,820 characters 40cea88 reports /etc/passwd and this head reports nothing. MAX_ANALYZABLE_FILE_BYTES is 16 MiB, so files in that range are still in scope.

[deepujain]

Replying to the regression thread (#545 (comment)): fixed in c8d201b.

Root cause: above SECURITY_VIEW_WINDOW_CHARS the runner hands lexical modules window slices instead of the whole file, so peek_python_ast never matches the scan's whole-file cache entry, and the constructed-path analysis returned [] for every windowed fragment. That was the silent drop you measured.

Fix: on a peek miss, parse the provided fragment directly behind the same plain textual join gate (restoring the 40cea88 behavior for window slices), while keeping the 17:45 improvements for whole files: alias-aware gate, shared AST reuse, multiline span dedup, and no parse for join-free fragments.

Verification:

• Your repro through the full graph now reports PE3 /etc/passwd at ~49,992, ~376,792, and ~436,820 chars (the two larger sizes were missing before).
• New regression tests: a unit test pinning windowed-fragment coverage (fails on 56d121c, passes with the fix), a unit test pinning that join-free fragments never trigger a parse, and a graph-level test with a ~376k-char file.
• test_graph_scan_parses_python_once_before_parallel_analyzers still passes (small files still parse once). Focused files green (222 tests), broad run of tests/unit/test_patterns.py + tests/nodes/analyzers green (3204 passed, 4 xfailed), ruff check and ruff format clean on all touched files.