Skip to content

feat(python): classify executable source surfaces - #579

Open
chrisknvidia wants to merge 2 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/python-execution-surface-classification
Open

chrisknvidia wants to merge 2 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/python-execution-surface-classification

Conversation

@chrisknvidia

@chrisknvidia chrisknvidia commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Adds consistent Python execution-surface classification from raw bytes across .py, .pyw, PEP 263 encodings, extensionless shebangs, and uv launchers. The classification flows through artifact inventory, nested and transitive analysis, static and behavioral analyzers, inspection ledgers, and provider-cache boundaries.

Ambiguous or platform-dependent inputs remain explicitly incomplete. Analysis retains the decoded Python source while provider views remain bounded and respect local-only, hidden, and nested-content boundaries. This is the Python execution-surface split requested in #497.

The latest update at 59a8d9f incorporates #577’s receiver-invalidation fix through the updated #578 stack. Later calls through a potentially replaced receiver no longer produce the reported false-positive TM1 findings. Regression coverage includes expression, assignment, annotated assignment, and subsequent function-call paths. The previously reviewed bare-uv and execution-type-aware parser-limit fixes are preserved, and the merge conflicts are resolved.

Dependency state: #576 is merged. #577 at 995d746 and #578 at 1c03702 remain open; both fixes are included here. The Python-specific review range is git diff 1c03702..59a8d9f.

Validation at 59a8d9f: all six hosted checks pass, including the unit suite with coverage and Docker scans of a local directory and a GitHub URL. Independent review of the dependency update and merge resolution found no remaining blocker. Fresh static CLI checks in JSON, SARIF, and Markdown verified .pyw, encoded Python, extensionless Python, and uv script inputs.

Ready for re-review of the updated head.

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

Reviewed exact draft head 266d42e050493dad869fe50be420ace97914ea82. I found no additional blocker in the focused Python execution-surface range (d856d88..266d42e): 1,118 affected tests passed with 1 skip, 732 current-main merged-tree regressions passed, and lint/format/diff checks are clean.

I am requesting changes because the current combined tree still contains the confirmed #576/#577 blockers: a three-row ledger cap drops a second distinct fatal fact, and a definitely true bound shell= value evades TM1 when a later argument is effectful. This PR also inherits draft #578 and explicitly requires the dependency stack to land, rebase onto current main, fresh exact-head CI, and current-head review.

Please propagate the dependency fixes and rebase. If the focused range remains semantically unchanged and CI stays green, I found it otherwise suitable for approval.

@chrisknvidia
chrisknvidia marked this pull request as ready for review September 21, 2026 05:23
@chrisknvidia

chrisknvidia commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

@rng1995 The final #578/#577 stack is included, conflicts are resolved, and the bare-uv/parser-limit fixes remain intact. PTAL.

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

The focused #579 bare-uv and execution-type parse-limit changes look correct, but this stacked head inherits the exact #577 receiver-invalidation blocker: an effectful argument after shell= may replace the trusted subprocess receiver, yet later proxy calls are still classified as subprocess and receive false-positive TM1 findings. Please update the stack after #577 clears trusted_names on this effectful path and add the regression. The current head is otherwise conflict-free and its affected/integration suites are clean.

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

Changes requested on exact head 59a8d9fd60b150b27457254979c256bc80b82e37 for a confirmed remaining issue inherited from #577.

The earlier effectful-argument fix is present: direct subprocess calls now invalidate receiver trust after unsafe arguments, including expression, assignment, and annotated-assignment paths. However, an ordinary effectful function call can still replace the receiver without invalidating that trust; a later call through the replacement object is then reported as HIGH TM1. The inline comment provides the source-traced fixture and expected correction. Please propagate the #577 correction through this stack and cover this case.

Scope: I inspected the prior review history, current dependency fix, receiver-trust collector and both scan passes, its caller integration, tests for the direct-argument correction, and current checks. This is a focused blocking review, not approval or certification of the entire large combined stack. The remaining combined diff still needs complete current-head assessment after the dependency issue is addressed. Six hosted checks pass, but passing checks do not establish this untested receiver semantic.

No contributor-provided code or tests were executed locally.

Comment thread src/skillspector/nodes/analyzers/static_python_shell_truthiness.py Outdated
@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/python-execution-surface-classification branch 2 times, most recently from c1fbda5 to d38a3d4 Compare September 23, 2026 12:38
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/python-execution-surface-classification branch from d38a3d4 to 3ee7334 Compare September 23, 2026 12:59
…tion

Signed-off-by: Christopher Kevin <christopherk@nvidia.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.

2 participants