fix: keep runtime-selected printf reconstruction incomplete

[mohgupta-ship-it]

Runtime-selected executable and wrapper names can fall out of bounded printf reconstruction and incorrectly produce a complete/SAFE result. Recognize ambiguous invocations from printf-style operands or destructive outer arguments and report partial analysis so strict CLI and MCP installation gates reject unresolved work.

Preserve ordinary parameter documentation, PowerShell expressions, and the reference-accounting fixes from #507. Operand lookahead remains bounded. Clone monitoring tolerates disappearing Git metadata only while the clone is active and still performs a strict final inspection.

Completeness checks use the Markdown-aware projection, preserving inline-code, JSON-string, table-cell, and paragraph boundaries. Remove the raw-Markdown fallback, keep validated JSON values outside unrelated unclosed spans, avoid reparsing literal backtick bodies, and do not treat shell fence labels without a command string as incomplete execution.

Regression tests distinguish literal shell backticks from Markdown delimiters. Paired CLI/MCP tests exercise static-only and semantic-enabled workflows with deterministic model responses and verify that successful LLM analysis does not erase incomplete static coverage.

Earlier full-suite validation at ceec1b8c9caaeef00e8dc2ab9112937b4df8b68e (Python 3.13.13, locked dependencies):

• make test-ci: 6,992 passed, 14 skipped, 87 deselected, 4 expected failures; 90% coverage. Four warnings: three unregistered timeout markers and one deliberate duplicate ZIP-member fixture.
• Offline graph integration: 62 passed.
• Cache/deadline regression tests confirm consistent views and that warm caches cannot bypass deadlines.
• Ruff lint/format, diff checks, and commit sign-off validation passed.

Security checks after rebasing onto 224ba29:

Check	Result	Details
Secrets	PASS	Gitleaks: final PR diff and commit messages scanned; no findings
Additional container-based secret check	SKIP	Docker daemon unavailable
Static security analysis	PASS	Semgrep: 129 Python rules across four changed production files; one unchanged subprocess audit finding reviewed (argument list, shell=False, validated URL host)
Dependencies	WARN	Three existing advisories in AnyIO 4.14.1; fixes available in 4.14.2; dependencies unchanged
License review	PASS	No dependency or license changes
Container changes	N/A	No container changes in the review follow-up
Security code review	PASS	Ownership boundaries, nested substitution visibility, deadlines, and error handling reviewed

Rebased onto main at 224ba29. Seven commits, no merge commits; final files are identical to the pre-rebase head af385bf. Preserved the cache/deadline conflict resolution and its regression tests.

Validation at 959a606: 1,152 regression tests passed; Ruff lint, formatting, and diff checks passed. Original commit authors and sign-offs are preserved. Hosted CI and reviewer approval remain required.

[yashrajp22]

I checked these seven cases against this commit and the base. The PowerShell issue is new; the other six are existing gaps that still show up here.

[rng1995]

[SkillSpector Review]

Reviewed current head 11f15ab947d8db05c6fb3d8f371eddf946519a28 — REQUEST_CHANGES.

The seven unresolved, non-outdated review threads are reproducible gaps in the fail-closed contract and remain unaddressed on the merge-only head:

• tests/nodes/analyzers/test_security_reconstruction.py:1721: a runtime-selected destructive command inside a triple-backtick Markdown fence is skipped and can yield complete/SAFE. Route fenced code bodies through the incomplete-analysis check and add this regression.
• src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:729: assignment/exec/separator prefixes can make the parser stop before $CMD (for example $(MODE=x $CMD %s r m) -rf /). Continue to the runtime command or fail closed.
• src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:1203: brace-expanded absolute targets such as {/,/tmp} bypass the root-path check. Expand/prove the bounded brace alternatives or retain partial status.
• tests/nodes/test_security_end_to_end.py:1021: declared-marker reconstruction scans findings but does not propagate parse-exhaustion status, allowing a reconstructed runtime command to become complete/SAFE. Carry the exhaustion result into the ledger.
• src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:1180: an empty quoted prefix ("""$($CMD)" -rf /) causes tokenizer uncertainty that is discarded. Preserve the tokenizer's limited flag.
• src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:1186: a destructive target just beyond the 8,192-character lookahead is treated as clean. Reaching the lookahead boundary must produce partial analysis.
• src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:742: ordinary PowerShell replacement expressions containing %TEMP%, %s, or printf are misclassified as printf reconstruction and blocked. Exclude PowerShell value expressions while retaining unresolved shell-command cases.

The head changed during review from d95663d9766ae214a6b6668dac8bda62b41f5c1e to this merge commit; the current threads are still unresolved. The required fixes, unresolved threads, absent checks on the new head, and mergeStateStatus=BLOCKED all block merging.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 2c6a19c99ed8f46cc0ad5e7aa532c7b50b5abe6d.

The Markdown-fence finding from the previous review is resolved by the synchronized #516 changes: fence delimiters are now masked while fenced command bodies still reach the bounded parse-exhaustion check, with Markdown regression coverage.

The other six prior blockers remain on this head: assignment, exec, or separator prefixes can stop before the runtime command; brace-expanded absolute targets are not expanded by the root-path check; declared-marker projections still do not propagate parse exhaustion; tokenizer uncertainty from an empty quoted prefix is discarded; reaching the 8,192-character lookahead boundary is treated as clean; and ordinary PowerShell replacement strings containing %TEMP%, %s, or printf still trigger the printf-reconstruction heuristic. The existing inline threads contain the exact reproductions and requested fixes, so I have not duplicated them.

No hosted checks are reported on this head, and GitHub reports mergeStateStatus=BLOCKED.

[rng1995]

[SkillSpector Review]

Re-reviewed current head b4971cafc36d895ae265d80d576175c4f7a47d56. The commits since the marked review at 2c6a19c99ed8f46cc0ad5e7aa532c7b50b5abe6d only merge #522's configurable static-analysis allowance. That synchronization changes budget configuration, documentation, and tests but does not alter the six remaining parser/completeness blockers.

Assignment, exec, or separator prefixes can still stop before the runtime command; brace-expanded absolute targets remain unexpanded by the root-path check; declared-marker projections still fail to propagate parse exhaustion; tokenizer uncertainty from an empty quoted prefix is discarded; reaching the 8,192-character lookahead boundary is treated as clean; and ordinary PowerShell replacement strings containing %TEMP%, %s, or printf still trigger the printf-reconstruction heuristic. The existing inline threads contain the exact cases and requested fixes, so I have not duplicated them. The Markdown-fence finding remains resolved by the earlier #516 synchronization.

No hosted checks are reported, and GitHub reports mergeStateStatus=BLOCKED.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 0ad0b8e65a002152b8cfbd3054ab3c9b1c2e0464 after the marked review at b4971cafc36d895ae265d80d576175c4f7a47d56. All six PR-owned production and test blobs are byte-for-byte unchanged.

The Markdown-fence bypass remains resolved by the synchronized #516 work. The other six blockers remain: assignment/exec/separator prefixes can stop before the runtime command; brace-expanded absolute paths evade root-target recognition; declared-marker scans do not propagate parse exhaustion; empty quoted prefixes discard tokenizer uncertainty; reaching the bounded lookahead is treated as clean; and ordinary PowerShell replacement expressions are still mistaken for printf reconstruction. The existing threads contain the exact reproductions and expected corrections, so I have not duplicated them.

No required checks are reported on this head, active change requests and unresolved threads remain, and GitHub reports mergeStateStatus=BLOCKED.

[yashrajp22]

Please fix the time-budget issue below before merging.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 4b2584c721179b70443aa1f12739cec1a1cd5f4a. The head change only synchronizes unrelated main work; the PR-owned analyzer and regression files are unchanged. The eight previously reported fail-open and runtime-bound cases are now represented by exact regressions and their threads are resolved. A new regression in the current implementation blocks approval.

At src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:3011-3024, the additive raw-Markdown pass sends the entire unmasked document through _has_shell_command_word_exhaustion. That discards established inline-code, JSON-string, table-cell, and paragraph ownership and marks benign documentation such as $(hostname).example partial. The latest tested implementation failed 172 tests across those contracts and CLI/MCP gates; three parser stress cases also exceeded their 12-second bound. Restrict the raw fallback to proven runtime-selected destructive or backtick evidence (or preserve ownership metadata), restore the completeness contracts, and obtain a green exact-head rerun.

The current head has no attached checks. The correctness/performance regression, missing exact-head validation, active change request, and GitHub BLOCKED state prevent merge.

[rng1995]

[SkillSpector Review]

Re-reviewed exact head 694e8d22d125a7161328cfa361f89ef979856db7. Every commit since the last marked review only synchronizes main; the PR-owned raw-Markdown completeness fallback and its current regression remain unchanged.

src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:3011-3024 still passes the entire unmasked Markdown document to _has_shell_command_word_exhaustion. That loses established inline-code, JSON-string, table-cell, and paragraph ownership, making benign documentation such as $(hostname).example partial. The latest validated implementation produced 172 contract/CLI/MCP regressions and three parser stress overruns. Restrict the raw fallback to proven runtime-selected destructive/backtick evidence, or preserve ownership metadata, and obtain a green exact-head rerun of the affected completeness suites.

No exact-head checks are attached, and GitHub reports conflicts/DIRTY. The correctness/performance regression remains an approval blocker.

[zozozeezee]

Fixed in 2c70363. Removed the extra raw-text scan that was flagging harmless Markdown and fixed the related parsing issues. The eight earlier fixes still pass.

[zozozeezee]

Resolved the conflict in ceec1b8, preserving both caching and timeout checks. The latest main sync keeps the fix, and GitHub reports no merge conflicts.

The full local suite passed: 6,992 tests plus 62 integration tests. Another 371 relevant tests passed after the automatic main update.

Reviewer approval is still required. The earlier CI run is still active; the latest automatic update has no hosted checks yet.

[rng1995]

[SkillSpector Review]

Re-reviewed exact head 54973a2050bd8654a5eb045a32b61be88ead2e31, the complete current diff, relevant surrounding parser/runner code, previous reviews and resolved threads, and added regression tests. The intervening main merge was inspected as well.

The previously reported Markdown raw-fallback regression is fixed: completeness now uses the ownership-aware Markdown projection, with validated JSON string recovery rather than scanning the unmasked document again. The earlier reported fence, assignment/control-prefix, brace-root, declared-marker, empty-quote, lookahead-boundary, PowerShell-value, and overlapping-suffix/deadline issues are addressed by the current paths and corresponding tests.

One required issue remains in the newly added command-string reconstruction: eval combines all operands, but the implementation only re-parses the first one. The inline finding gives a small inert fixture and required regression coverage. Please fix that before approval.

No checks are currently reported for this exact head. Review was static; no contributor-provided code or tests were executed.

[rng1995]

[SkillSpector Review]

Incremental re-review after automatic main synchronization. Only already-reviewed #539/#588/#591/#607/#597 and merge commits were added; the command-reconstruction implementation and its tests are unchanged. The current actionable eval-operand finding in #514 (review) therefore remains open. No duplicate inline finding added. Exact-head checks are unavailable, and the PR remains unmerged.

Static review only; no contributor code/tests executed.
Reviewed head: af385bf9badc0e7adf6fc0803eb483eccad666bd.
Priority: P0 — Runtime-command completeness prevents clean verdicts for unresolved destructive execution.

[rng1995]

[SkillSpector Review]

Re-reviewed exact head e41991665543a1e36e1de1d1e64fc49a2a2864eb: Changes Requested.

The full nine-file diff, relevant surrounding code, prior reviews/replies and thread states, regression tests, and current hosted CI were assessed. Of the ten unique prior findings, eight are resolved, the brace-target finding is partially resolved, and the multi-argument eval finding remains unresolved. There is also a newly identified overbroad PowerShell exemption.

Required corrections:

1. P1 — Multi-argument eval: still unresolved. The implementation at static_patterns_tool_misuse.py:1940–1942 still reads just one operand. Please address the existing inline finding, including split-versus-single operand regression coverage through the static ledger, CLI, and MCP. I have not duplicated that inline comment.
2. P1 — Brace expansion limits: the original two-alternative example is fixed, but unsupported/over-limit brace expansion still loses uncertainty in the dynamic-command path. See the new inline comment.
3. P1 — PowerShell exemption: .ps1 files skip the entire completeness hook, including embedded Bash command strings. See the new inline comment.

The other prior cases—Markdown fences, assignment/control prefixes, declared-marker propagation, empty quotes, lookahead boundaries, benign PowerShell replacements, deadline/overlapping-rescan behavior, and Markdown ownership—have source fixes and relevant regression coverage. The original benign PowerShell cases being fixed does not establish that exempting all .ps1 content is safe.

All six hosted checks pass for this head. CI run 35887171105 reports 7,680 passed, 14 skipped, 134 deselected, 4 expected failures, and 90% coverage. The three blocking cases above are not covered by the current regression tests. Findings are source-traced; no contributor-provided code or tests were executed locally. No merge performed.

[rng1995]

[SkillSpector Review]

Approved commit 1f162694a3e0daa196b144204983278fb80957da after implementing and independently reviewing the three remaining corrections:

• Reconstruct all bounded eval operands while respecting redirections, comments, clause boundaries, and deadlines; unresolved or over-limit reconstruction remains partial.
• Preserve unsupported brace-expansion uncertainty through both dynamic-command completeness paths.
• Check embedded Bash command strings in .ps1 files while preserving the narrow benign PowerShell replacement-expression handling.

All three outstanding review threads now have fix details and are resolved. Validation: 1,606 targeted tests passed across command reconstruction, documentation ownership, public CLI/MCP gates, PowerShell, brace boundaries, input bounds, and predicate caches. Tests include static-only and deterministic successful semantic-analysis workflows. Ruff lint/format and diff checks passed. Local Python 3.13 used dependencies matching the lockfile.

Hosted CI for this commit is running; changes, lint, TypeScript tests, and DCO already pass. The full hosted Python suite and container smoke result are not claimed complete. No merge performed.