fix: preserve incomplete scans for unsupported input and multiline prompts - #563
mohgupta-ship-it wants to merge 14 commits into
Conversation
Preserve explicit input identity and report unsupported primary content through completeness accounting. Keep supported ZIPs and passive assets unchanged, and validate archive headers without decompression. Implemented by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Account for pure and mixed newline-spaced prompt instructions with source-preserving AE6 ambiguity detection. Preserve structural and benign controls, and keep provenance lookup linear and cancellable. Implemented by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Preserve required instruction identity through directory and ZIP members, reject lossy primary decoding, and keep truncated UTF-8 prefixes explicitly partial. Interrupt multiline pattern searches while preserving Python character semantics and source provenance. Document the algorithm, decision table, bounds, and public verdict contracts with a Mermaid flow diagram. Implemented and reviewed by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
|
Codex on behalf of Mohit Gupta — scoped adversarial/council review of #563 Algorithm verdict: sound within the declared bounded profile after remediation. Rating: critical fix. Disposition: BLOCKED_BY_CI_OR_CONFLICT (hosted unit tests still running; scoped technical review READY). This is technical review, not maintainer approval; the PR remains a draft. Three independent specialists covered specification/regressions, security/trust boundaries, and runtime/design; root reproduced the high-impact candidates and a separate evidence-bounded judge reviewed the fixes. The five council lenses were specification, reachability, scope, design, and standards/tests. Affected layers: primary-input classification, normalization/provenance, completeness/reporting, and CLI/MCP consumers. Accepted and fixed:
The first two were pre-existing gaps incompletely closed by the initial draft; the new regex exposure was introduced by it. Ordinary archive words and benign list/code controls remain valid. AE6 is ambiguity evidence, not a proven semantic P3/P4 instruction. No claim of linear regex execution or universal semantic safety is made. Algorithm explanation, worked examples, decision table, Mermaid flow, and boundary critique. The observable contract matters: unsupported primary content is fatal (CLI 2); AE6 or timeout can be partial with default CLI 0, while Full-suite snapshot
Latest reviewed head
All authored commits carry DCO sign-off; preserved GitHub-generated synchronization merges use the existing CI exemption. The updated Mermaid diagram rendered successfully. Policy exclusions and the same 11 known edge expectations remain explicit limits. Initial frozen review (2026-09-16 18:42:55 UTC): base Profile boundary: excluded dependency/VCS metadata is not interpreted instruction content. Unreferenced non-executable instructions under those existing exclusions can coexist with a complete result; explicitly selecting such a file still invokes required-content checks. The durable explanation now makes this distinction explicit. Still open outside this patch: the same 11 frozen edge expectations, pre-existing optional type-check diagnostics, broader reference/version/BOM gaps, whole-program real-time behavior and unbenchmarked peak resident memory. No remaining substantiated blocker was found in this changed algorithm. No other PR, release, or production pin was changed. |
Retain both primary-content failures and excluded-content audit events when integrating main. Add a coexistence regression and clarify the distinction between analyzed instruction bytes and exclusion-audit metadata. Resolved and reviewed by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Cover both direct and ZIP-contained excluded executables, asserting each exact source path survives alongside the fatal primary-content event. Implemented and reviewed by Codex on behalf of Mohit Gupta. Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
| *reference_events, | ||
| *cache_events, | ||
| *nested.ledger_events, | ||
| *primary_content_events, |
There was a problem hiding this comment.
[P2] Preserve primary-content failures when the ledger is truncated
Could we preserve the fatal primary-content outcome independently of the capped detail list? primary_content_events is appended after exclusion_audit_events, so _bounded_ledger_output() can discard it once the 10,000-record limit is reached. finalize_ledger() then derives execution_successful from the surviving exceptions, even though the primary artifact still has disposition="failed".
Reproduced on 14fa2278632a7f3a2e46d2771e6d78b403a3c846 with default limits and scan --no-llm --format json: a UTF-16 SKILL.md containing # Instructions\nSummarize text.\n produces status="failed", execution_successful=false, and exit 2. Adding 5,000 files named node_modules/example/0000.py through 4999.py, each containing pass\n, changes the same scan to status="partial", execution_successful=true, and exit 1; unsupported_primary_content disappears from ledger_exceptions.
The result remains incomplete, so this is not an installation-safety bypass, but it loses the primary failure reason and violates the documented fatal-error/exit-code contract. Please retain a fatal summary across ledger truncation and add a regression combining unsupported primary content with ledger overflow.
yashrajp22
left a comment
There was a problem hiding this comment.
The scoped review found one new timeout regression on ordinary prose. The existing ledger-truncation comment remains applicable and is not duplicated here.
| started_at = time.monotonic() | ||
| reconstruction_index = 0 | ||
| try: | ||
| for match in pattern.finditer(matching_text, timeout=timeout): |
There was a problem hiding this comment.
Could we avoid spurious timeouts during parallel scans here? The new projection also activates on ordinary contractions such as it's a, and regex.finditer() releases the GIL by default (documented behavior). On HEAD 183fd554, a 6.8 KB prose document that is complete on the base becomes partial in both source and installed-wheel scans, with runtime_limit on this file and the remaining files. The first pattern takes under 0.14 ms alone but times out after about 320 ms in the normal parallel graph. Changing only this call to concurrent=False in a diagnostic control restores complete coverage without raising the timeout.
An independently generated benign fixture also returned strict CLI exit 1, zero coverage and no findings:
text = "# Reading notes\n\n" + (
"It's a short book about a village library. The chapter describes shelves, windows, "
"and reading tables. A visitor returns a borrowed volume and reads the next chapter.\n\n"
) * 40
# Put text in SKILL.md and notes.md, and json.dumps({"examples": [text]}) in examples.json.
# Run: skillspector scan <directory> --no-llm --format json --fail-on-incompleteThis is scheduling-sensitive: the same generated fixture completed in an MCP call. Please retain interruptible matching while preventing ordinary parallel analyzer activity from producing these false partial results, and cover this through the real parallel scan graph. Tested with CPython 3.12.13 on Linux arm64, with 2 CPUs and 4 GiB RAM.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed exact head 183fd554e5b41208f2ea8d9211d084de3972fe3b across the complete production and test diff.
Changes are still required for two independently reproducible regressions already documented inline:
- The 10,000-row ledger cap can discard
unsupported_primary_content, changing a fatal primary-input failure into a merely partial public result and changing the CLI exit contract. Preserve a bounded fatal summary independently of event ordering and add the overflow regression described in the thread. - The new timeout-enabled
regex.finditer()path can produce false runtime-limit failures on ordinary prose under the real parallel graph because wall-clock timeout expires while another analyzer runs. Retain interruptibility without turning normal scheduling into incomplete coverage, and cover this through the parallel workflow.
I verified both findings against the current implementation. I did not repeat the existing inline comments.
Prepared by Codex on behalf of Mohit Gupta.
Unsupported primary content could previously become a passive binary exclusion and leave a complete
SAFEreport. Pure or mixed newline-spaced instructions could also lose deterministic evidence without recording incomplete interpretation. This draft makes those missing-analysis decisions explicit and carries them through CLI and MCP verdicts.The algorithm is conceptually sound within the documented bounded profile: preserve required identity, retain source evidence, and prevent missing or ambiguous analysis from becoming installation safety. It does not establish universal format recognition or semantic safety.
Algorithm
SKILL.md/skill.mdbasenames as required instructions through directories and virtual ZIP members; ordinary incidental assets retain existing reference policy. Exclusion-audit metadata does not bring excluded dependency/VCS trees into ordinary source analysis; explicit file selection and references retain their own gates.unsupported_primary_contentledger event. Use byte classification, successful UTF-8 decoding, and conservative bounded archive-header recognition. A byte limit splitting the final UTF-8 character remains partial. Header recognition examines at most 512 bytes and does not extract or decompress containers; supported ZIP inspection is a separate existing path.obfuscated_instruction_textare recorded. The ordinary semantic view is unchanged.runtime_limitcoverage rather than claiming a clean result.Durable algorithm explanation, worked examples, decision table, observability, bounds, and critique.
The initial boundary was too narrow: archive parsing introduced member paths before primary classification, losing required identity. Conversely, applying rejection before supported ZIP delegation would reject valid containers. The reconstruction boundary must preserve structural separators so it cannot manufacture commands from unrelated prose. Source maps and gap overlap bind ambiguity evidence to the original content. The full explanation includes incidental-asset and no-match branches omitted from this compact diagram.
Observable behavior
Static-only examples with otherwise benign content:
safe_to_installunsupported_primary_content; failednever warn the userobfuscated_instruction_text; fixture score 22runtime_limit, observed/allowed secondsDefault CLI exit 0 is not installation safety. Fatal execution takes precedence; otherwise strict flags or score above 50 cause exit 1. MCP requires complete successful analysis, no entirely uninspected files, score at most 50, and fulfillment of any requested LLM analysis. Validation explicitly uses
--no-llm/use_llm=false.Existing JSON, terminal, Markdown, SARIF, and MCP completeness fields expose the reason, path, fatality, available source lines, and limit metrics. Component coverage may still be 100% when a system-level AE6 interpretation exception makes
is_complete=false; consumers must use completeness, not that percentage alone.Review feedback and validation
Frozen independent review (2026-09-16 18:42:55 UTC): base
9e078093eb8e621852e937cdc1757dca1c41ad05, initial headc6aa3264954aeadbfef55e5e2f2d843d4e78e6b7; draft, five passing hosted checks, no reviews/comments. Three read-only specialists covered specification/regressions, security/trust boundaries, and runtime/design. Root reproduced high-impact candidates, then an independent evidence-bounded judge re-reviewed the remediation across all five council lenses. The live base advanced repeatedly during review. Existing automatic synchronizations were preserved without force-pushing. The batch cache, JSON recovery, LLM deadline/provider, excluded-content ledger, and later input/CLI-consumer interactions were checked within this PR’s scope. The sole manual conflict was final ledger assembly: both primary-content failures and excluded nested-content events are retained, with direct and archived coexistence regressions. Live refresh at 2026-09-16 21:06:28 UTC: base4148ab3, head14fa2278632a7f3a2e46d2771e6d78b403a3c846, open draft, mergeable without conflicts, 0 maintainer reviews. 5/6 current-head hosted checks passed; hosted unit tests still running; scoped technical review READY. Current-head CI.Rating: critical fix. Disposition: BLOCKED_BY_CI_OR_CONFLICT. Hosted unit tests still running; scoped technical review ready. This is technical review, not maintainer approval. The PR remains a draft.
Full-suite snapshot
264ba731cf802ab4f8b96eb18c65325ceec7fb80, base4d52048, Python 3.12.11:make test-ci: 5,497 passed, 15 skipped, 38 deselected, 4 expected failures; 90% coverage. All six hosted checks passed, including 5,498 hosted tests passed, 14 skipped, 38 deselected, 4 expected failures, 90% coverage. Live integration/provider markers are excluded; coverage has no configured fail-under threshold.4d52048, with zero normalized differences in the same locked environment.Latest reviewed head
14fa2278632a7f3a2e46d2771e6d78b403a3c846, base4148ab3, adds an upstream CLI progress update after that full-suite snapshot:264ba73; they are not presented as a completed full-suite run on this newer head.All authored commits carry DCO sign-off; preserved GitHub-generated synchronization merges use the existing CI exemption. The updated Mermaid diagram rendered successfully. Policy exclusions and the same 11 known edge expectations remain explicit limits.
Remaining limits: grammar-specific ambiguity detection, bounded format recognition, cooperative whole-workflow timing, and memory overhead from derived views. Projection/provenance is O(n); regex matching is operationally timed, not claimed linear. The cached-byte cap is not a resident-memory cap. Existing same-line matching and unrelated reference/version/BOM gaps are outside this change. No release, production pin, or other PR is changed.