fix: preserve incomplete scans for unsupported input and multiline prompts - #563
mohgupta-ship-it wants to merge 18 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>
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.
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.
Preserve primary-file identity alongside the upstream selected-source identity and retain the cached text-view predicate. Signed-off-by: Chandrashekar Ramachandran <cramachandra@nvidia.com>
Keep failed artifact dispositions authoritative when ledger details are truncated, including when manifest parsing also fails. Preserve byte-recognized ZIP identity at every nesting level and reject unsupported primary bytes even with an archive suffix. Keep bounded prompt matching from yielding its timeout to parallel Python analyzers. Add overflow, archive, and competing-thread regressions. Signed-off-by: Chandrashekar Ramachandran <cramachandra@nvidia.com>
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.