fix(ast): detect joined reflective execution sinks

[deepujain]

Fixes #479.

Resolve constant string construction in reflective getattr sink names, including ''.join(['e', 'x', 'e', 'c']), so AST9 reports dynamic execution sinks.

Validation: python3 -m py_compile src/skillspector/nodes/analyzers/behavioral_ast.py and git diff --check. The isolated checkout does not include pytest; hosted CI will run the focused and full suites.

[MohammedAlkindi]

The AST9 upgrade costs the AST7 net on every name outside the five-entry allowlist.

Base reached AST7 via not isinstance(second_arg, ast.Constant), which a ''.join([...]) call always satisfied. Now that _constant_string resolves it, the join is no longer non-constant, and it only becomes AST9 if the text is in _DANGEROUS_GETATTR_NAMES. Anything else falls through both arms.

Ran _analyze_python at 03e8f56 and at this head. Joined exec goes AST7 to AST9 as intended, but joined Popen, check_output, __globals__ and ''.join([]) all go AST7 to nothing. popen is in the set and Popen is not, so the subprocess spelling is the uncovered one.

The other direction moved too: getattr(o, 42) was silent on base and now emits AST7.

[rng1995]

[SkillSpector Review]

Reviewed current head 77b79a5a4aa1d3bf9f4cbf3340d9993d659d8605. Joined exec is correctly promoted to AST9 and all hosted checks pass, but the new constant resolver changes the existing AST7 boundary in both directions.

A constructed callable name which resolves outside the five-name set now falls through entirely, so common execution surfaces such as getattr(subprocess, ''.join(['P', 'o', 'p', 'e', 'n']))(...) and joined check_output lose the AST7 signal they had on main. Conversely, a non-string literal such as getattr(obj, 42) changes from silent to AST7 because None conflates not a string constant with unresolved/dynamic.

Preserve the prior AST7 signal for constructed/dynamic names unless they are promoted to AST9, keep direct non-string constants out, and add regressions for joined dangerous non-allowlisted names, joined benign names, and non-string literals. The inline finding identifies the conflated state.

[rng1995]

[SkillSpector Review]

Re-reviewed current head dc389cd2131b82a8a9949297504a70c89a1e0c18 against the prior finding, complete analyzer/test diff, surrounding AST7/AST9 behavior, and exact-head checks.

The requested state distinction remains unimplemented. _constant_string() still returns None for both a direct non-string literal and an unresolved/dynamic name. Consequently, getattr(obj, 42) is still newly reported as AST7, while constructed names outside the five-name promotion set—including joined Popen or check_output—still lose the AST7 coverage present on main. Preserve construction/dynamic provenance separately from direct non-string constants and add the requested dangerous, benign, and non-string regressions. I have not duplicated the existing inline finding.

All six exact-head checks pass, but the unresolved behavioral regression and active change requests block merging.

Priority: P1 — this changes reflective-execution detection in both false-negative and false-positive directions.

[deepujain]

All review comments are addressed on the current head 34c1792:

• rng1995 P1 (direct non-string constants vs constructed string names): fixed in 34c1792 (fix(ast): retain dynamic getattr provenance); direct non-string literals stay silent, constructed non-sink names remain AST7, constructed dangerous names remain AST9, with both regressions included.

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

[rng1995]

[SkillSpector Review]

Re-reviewed current head 34c179213e1a3bfd01120704a0860a586f9aa77d, the complete two-file diff, surrounding analyzer/AST parsing/resource-budget code, all prior reviews and discussion, the resolved inline thread, regression tests, and hosted checks.

The earlier provenance finding is addressed: direct non-string constants remain silent; constructed names outside the AST9 set (including joined Popen, check_output, __globals__, benign names, and the empty join) retain AST7; joined exec is promoted to AST9. The added tests cover the two previously regressed classification branches and the intended joined-exec promotion. I am not repeating that resolved finding.

One new P1 issue blocks approval: _constant_string() materializes an unbounded joined value from untrusted source before any budget check. A parseable 130,024-character source containing a 100,000-character literal separator and 10,000 empty literal elements passes the 1,000,000-character input gate, but this join expands to 999,900,000 characters. That can exhaust scanner memory even though the only consumer compares the result against five short sink names. See the inline comment for the required bounded-resolution fix and regression coverage.

Validation: all six current-head checks pass (changes, lint, test-unit, OpenCode TypeScript Tests, DCO Check, docker-smoke). The review used static inspection and a reviewer-authored parse-only calculation of the allocation case; contributor code/tests and the large join were not executed locally. gh pr checks --required reports no required checks. The original thread is resolved; GitHub currently reports MERGEABLE/BLOCKED.

Decision: Changes Requested for the new resource-exhaustion regression, not for the resolved prior finding.

[rng1995]

[P1] Bound the resolved string before allocating the join. This runs on untrusted skill source, and the source-size gate does not bound the expanded value: a literal 100,000-character separator joined over 10,000 empty string literals fits in 130,024 source characters, but allocates 999,900,000 characters here. _BehavioralBudget.check_runtime() cannot interrupt this allocation, and node() does not handle memory exhaustion, so a small scanned file can exhaust the scanner process. Calculate/cap the prospective output length before each join, including nested joins, and return unresolved so the existing AST7 fallback is preserved when the cap is exceeded; a bound based on the longest dangerous name is sufficient for this consumer. Add bounded-rejection regressions for a large separator/list and nested joins without actually allocating the expanded payload, while retaining the AST7/AST9 classification tests.

[deepujain]

Addressed in 07274d4: _constant_string() now computes the prospective join length before allocating, capped at the longest dangerous getattr name (10 chars, "import"), including nested joins via the recursive unresolved return. Over-cap joins stay unresolved so the AST7 fallback is preserved; bounded joins keep their AST7/AST9 classification. Added regressions: reviewer-shape huge separator/list and nested joins (resolve to None without allocating), plus AST7 fallback assertions. Focused lane (127 tests), ruff lint, and format-check all green.

[MohammedAlkindi]

Ran it at 07274d4. The exhaustion does not reproduce, and nesting does not get past the cap.

Scaled 100x down from your example (10,000-char separator, 1,000 empty literals). tracemalloc peak, separate clone per commit:

flat     34c1792   9,990,000 chars   9.5442 MB   |   07274d4  None  0.0088 MB
nested   34c1792  19,980,001 chars  38.1092 MB   |   07274d4  None  0.0089 MB

Nesting costs 4x the flat case at the parent, not 2x.

Your first-round case holds: joined check_output is 12 chars, over the cap, and reports AST7 rather than going silent. Exactly 10 chars still resolves.

Python 3.14.0 under uv; ruff check and format clean; 127/127 in test_behavioral_ast.py.

Full scale I did not measure; that rests on the PR's own test, which reports no memory.

[deepujain]

Thanks for the independent verification - the tracemalloc comparison against 34c1792 confirms the cap holds (0.0088 MB vs 9.5 MB), and the 12-char AST7 finding still resolving correctly is exactly the intended behavior.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 07274d44851ef62aa191fe6bad125e0981b903ff, the complete analyzer/test diff, surrounding caller and source-size/resource-budget behavior, all previous reviews and author replies, review-thread state, and current CI results. Both blocking findings are resolved.

• The original AST7/AST9 provenance regression remains fixed: direct non-string literals remain silent; constructed names outside the sink set retain AST7, including over-cap names such as check_output; joined exec is promoted to AST9.
• The resource-exhaustion finding is fixed: each supported join calculates its prospective output length before allocation and only joins values up to the longest dangerous name (10 characters). Over-cap nested joins return unresolved recursively, preserving AST7. The large-separator regression exercises the reported amplification case, and end-to-end over-cap tests verify the fallback classification.

No required code or documentation changes remain. The inline test-fixture suggestion is non-blocking: existing direct tests exercise the shared length gate that every recursive join uses, and source inspection confirms the nested protection.

All six hosted checks pass at this head: changes, lint, test-unit, OpenCode TypeScript Tests, DCO Check, and docker-smoke. gh pr checks --required reports no required checks. Tests were inspected, not executed locally; no contributor code was run. One outdated conversation on the now-fixed allocation finding remains unresolved in GitHub and may need maintainer/author resolution before merging.

Decision: Approved for this commit.