fix: preserve incomplete scans for unsupported input and multiline prompts - #563
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>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head 65799615dd3f49a97b04fdd3137a76a10d49ffcf, including the complete current production, documentation, and test diff and relevant surrounding source.
Both previously reported blockers are addressed:
- Fatal primary-content outcomes are recovered from canonical artifact inventory when bounded ledger detail is truncated, and manifest processing no longer overwrites an existing failed disposition. The added bundle/workflow overflow regressions exercise fatal reason retention, unsuccessful execution, and failed completeness.
- Timed multiline matching now uses
concurrent=Falsewhile retaining the timeout and workflow deadline. The competing-thread and real parallel workflow/MCP regressions cover benign prose; actual-timeout coverage remains present.
Also inspected required-input identity through nested/renamed ZIPs, strict decoding and bounded UTF-8 prefixes, reconstruction provenance, unsupported-format recognition, and benign controls. No remaining required code/test/documentation change found.
The intervening main-branch merge was also checked: it adds the already-reviewed joined reflective-sink fix and tests without changing this PR's completeness implementation.
Approved on code review. There are currently no hosted check results reported for this exact head, so merge readiness remains contingent on the repository's check/protection gates. Review was static; I did not execute contributor-provided code or tests.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Incrementally re-reviewed the new synchronized head after the earlier full approval. The delta contains only the already-reviewed/merged #539/#588/#591/#607/#597 and merge commits. The original unsupported-input fatal-inventory fix and bounded multiline/concurrency behavior remain intact. The build-context interaction from #597 only widens the executable probe from four to eight bytes; its finalizer preserves incomplete ledger outcomes while narrowly suppressing AE1 for validated passive PNG images. No new required change found in this integration.
No current-head checks are reported, and other reviewers' Changes Requested still block merging. This approval does not override them or authorize an unknown/blocked merge state.
Static review only; no contributor code/tests executed.
Reviewed head: dfcde7b91726db062a72622bb6c8ac48c249b430.
Priority: P0 — Prevents unsupported inputs and interrupted scanning from appearing complete.
|
@yashrajp22 Both issues from your review are fixed in the current PR:
The review threads are resolved, and another reviewer has approved the fixes. The current hosted unit tests are still running. Could you re-review and update your Changes Requested review if these fixes address your concerns? |
@yashrajp22 Both issues from your review are fixed in the current PR:
Normal prose no longer causes a false timeout during parallel scans. The timed search now uses concurrent=False, with regression tests for the parallel workflow.
A failed primary skill file stays failed even when the detailed report reaches its size limit. Regression tests cover that limit and the original case.
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.