Skip to content

fix: preserve incomplete scans for unsupported input and multiline prompts - #563

Open
mohgupta-ship-it wants to merge 14 commits into
mainfrom
codex/fix-scan-completeness-20260916
Open

mohgupta-ship-it wants to merge 14 commits into
mainfrom
codex/fix-scan-completeness-20260916

Conversation

@mohgupta-ship-it

@mohgupta-ship-it mohgupta-ship-it commented Sep 16, 2026

Copy link
Copy Markdown
Member

Prepared by Codex on behalf of Mohit Gupta.

Unsupported primary content could previously become a passive binary exclusion and leave a complete SAFE report. 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

  • Preserve the identity of an explicitly selected file. Treat cached, in-profile SKILL.md / skill.md basenames 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.
  • Reject unsupported required bytes through a fatal unsupported_primary_content ledger 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.
  • Build a source-mapped ambiguity view by joining alphabetic singleton tokens across one logical newline or one horizontal whitespace character, including mixtures. Keep paragraph breaks, list/code punctuation, ordinary words, and wider gaps. A P3/P4-shaped match must intersect a removed gap before AE6 and obfuscated_instruction_text are recorded. The ordinary semantic view is unchanged.
  • Bound each new multiline regex operation to 0.25 seconds or the smaller remaining workflow allowance. Preserve Python's original character semantics through a length-preserving matching alphabet. A timeout records partial runtime_limit coverage rather than claiming a clean result.

Durable algorithm explanation, worked examples, decision table, observability, bounds, and critique.

flowchart TD
  A["Untrusted selected artifact"] --> B["Path / URL boundary; bounded byte inventory"]
  B --> C["Supported ZIP inspection preserves member paths"]
  B --> P["Required identity: selected file or SKILL.md / skill.md"]
  C --> P
  P --> U["Unsupported bytes / lossy primary decoding"]
  U --> F["Fatal ledger evidence; retain raw bytes"]
  P --> T["Supported text or retained bounded prefix"]
  T --> S["Existing semantic/static checks"]
  T --> N["Singleton projection; preserve structural boundaries and raw offsets"]
  N --> M["Timed P3/P4 matching with original character semantics"]
  M -->|match crosses removed gap| AE["AE6 ambiguity plus partial ledger evidence"]
  M -->|timeout| L["Partial runtime-limit evidence"]
  B -->|byte limit splits final UTF-8 character| L2["Partial size-limit evidence"]
  C -->|archive limit or failure| L3["Explicit archive exception"]
  S --> Q["Findings and ledger finalization"]
  F --> Q
  AE --> Q
  L --> Q
  L2 --> Q
  L3 --> Q
  Q --> R["Reports expose reasons; incomplete SAFE becomes CAUTION"]
  Q --> CLI["CLI: fatal exit 2; strict partial/findings exit 1; risk gate"]
  Q --> MCP["MCP installation safety requires complete successful analysis"]
Loading

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:

Input Evidence / completeness CLI default / strict incomplete / strict findings MCP safe_to_install
Benign UTF-8 or supported benign ZIP Complete 0 / 0 / 0 true
Opaque, UTF-16, malformed UTF-8, or recognized unsupported primary archive Fatal unsupported_primary_content; failed 2 / 2 / 2 false
Unsupported instruction member in normal or renamed ZIP Same fatal event at member path 2 / 2 / 2 false
Pure/mixed newline-spaced never warn the user AE6, source line, partial obfuscated_instruction_text; fixture score 22 0 / 1 / 1 false
Referenced opaque image AE1 and partial opaque-content accounting 0 / 1 / 1 false
Matcher timeout without findings Partial runtime_limit, observed/allowed seconds 0 / 1 / 0 false

Default 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 head c6aa3264954aeadbfef55e5e2f2d843d4e78e6b7; 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: base 4148ab3, head 14fa2278632a7f3a2e46d2771e6d78b403a3c846, 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.

Feedback Disposition
Nested/renamed-ZIP primary identity loss Accepted and fixed; pre-existing gap incompletely closed by the initial draft
Replacement-decoded primary text counted complete Accepted and fixed; pre-existing gap incompletely closed by the initial draft
New newline collapse exposed superlinear wildcard matching and delayed cancellation Accepted and fixed with interruptible matching; introduced by the initial draft
Regex-engine Unicode mismatch during remediation Caught by re-review and fixed before publication; match-span parity and public regressions added
Tiny truncated UTF-8 prefix mislabeled unsupported Fixed; byte-limit explanation remains partial
Mere archive words, benign singleton lists, or a linear provenance walk proving linear matching Rejected with controls; no such general claims remain

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, base 4d52048, Python 3.12.11:

  • Local 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.
  • Ruff lint/format, build/Twine, signed commits, rebuilt local Docker/unmodified smoke, and installed CLI/MCP decision contracts passed. Unicode matching memberships were independently checked across all 1,114,112 code points; the full matcher is not claimed linear.
  • Frozen edge oracle: 43/54 pass, 11/54 fail, with identical failure cases and assertion errors. No expectations changed and no new failures. Optional mypy: 135/135 existing diagnostics against base 4d52048, with zero normalized differences in the same locked environment.
  • Incoming-change checks included 72 input/public-contract tests (2 platform skips), 44 TypeScript wrapper tests, 26 registry tests, and static CLI/MCP registry controls. Windows APIs were simulated, not validated on Windows. Local TypeScript tests used Node 24.5.0; the hosted Node 22.20.0 job passed.

Latest reviewed head 14fa2278632a7f3a2e46d2771e6d78b403a3c846, base 4148ab3, adds an upstream CLI progress update after that full-suite snapshot:

  • 12 focused CLI/progress and explicit stream/invoke integration tests passed. Independent real CLI checks covered direct and forced-terminal streaming for benign, unsupported-primary, and AE6 inputs with strict completeness on/off: valid JSON stdout, progress only on stderr, matching ledger/completeness/risk results, and unchanged exit gates. The explicit integration parity test was run separately because the default CI marker excludes it.
  • Rebuilt exact wheel: 100/100 modules match current source, 20/20 decision cases, 60 installed CLI scans, and 20 real MCP stdio calls pass. Static analysis only, cleared provider/credential inheritance, zero network attempts in guarded validation. Build/Twine and current Ruff lint/format pass.
  • 5/6 current-head hosted checks passed; hosted unit tests still running; scoped technical review READY. Current-head CI. The full-suite totals above belong to 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.

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>
github-actions Bot and others added 2 commits September 16, 2026 18:50
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>
@mohgupta-ship-it

mohgupta-ship-it commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

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:

  • Required instruction identity disappeared below directory and renamed/multi-root ZIP boundaries, allowing unsupported members to be excluded as passive assets.
  • Sparse invalid UTF-8 was replacement-decoded and counted complete. Required content now needs successful interpretation; a byte limit cutting a valid final character remains partial.
  • Newline collapse exposed superlinear wildcard searches between cancellation checks. The new matcher is interruptible and records partial runtime-limit evidence. Unicode matching parity was independently checked across every Unicode code point; the initial engine mismatch was caught and fixed before publication.

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 --fail-on-incomplete yields 1. MCP rejects both through safe_to_install=false. Reports retain reasons, paths, source lines and applicable limit metrics. A 100% component count does not override is_complete=false.

Full-suite snapshot 264ba731cf802ab4f8b96eb18c65325ceec7fb80, base 4d52048, Python 3.12.11:

  • Local 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.
  • Ruff lint/format, build/Twine, signed commits, rebuilt local Docker/unmodified smoke, and installed CLI/MCP decision contracts passed. Unicode matching memberships were independently checked across all 1,114,112 code points; the full matcher is not claimed linear.
  • Frozen edge oracle: 43/54 pass, 11/54 fail, with identical failure cases and assertion errors. No expectations changed and no new failures. Optional mypy: 135/135 existing diagnostics against base 4d52048, with zero normalized differences in the same locked environment.
  • Incoming-change checks included 72 input/public-contract tests (2 platform skips), 44 TypeScript wrapper tests, 26 registry tests, and static CLI/MCP registry controls. Windows APIs were simulated, not validated on Windows. Local TypeScript tests used Node 24.5.0; the hosted Node 22.20.0 job passed.

Latest reviewed head 14fa2278632a7f3a2e46d2771e6d78b403a3c846, base 4148ab3, adds an upstream CLI progress update after that full-suite snapshot:

  • 12 focused CLI/progress and explicit stream/invoke integration tests passed. Independent real CLI checks covered direct and forced-terminal streaming for benign, unsupported-primary, and AE6 inputs with strict completeness on/off: valid JSON stdout, progress only on stderr, matching ledger/completeness/risk results, and unchanged exit gates. The explicit integration parity test was run separately because the default CI marker excludes it.
  • Rebuilt exact wheel: 100/100 modules match current source, 20/20 decision cases, 60 installed CLI scans, and 20 real MCP stdio calls pass. Static analysis only, cleared provider/credential inheritance, zero network attempts in guarded validation. Build/Twine and current Ruff lint/format pass.
  • 5/6 current-head hosted checks passed; hosted unit tests still running; scoped technical review READY. Current-head CI. The full-suite totals above belong to 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.

Initial frozen review (2026-09-16 18:42:55 UTC): base 9e07809, head c6aa326, five hosted checks passed, no reviews/comments. The first remediation at 08f9fd4 passed all five hosted checks (5,348 tests passed, 14 skipped, 38 deselected, 4 expected failures; 90% coverage). Further automatic main synchronizations were preserved and their interaction surfaces reviewed; that earlier green run is not a claim that later heads passed CI. The manual ledger conflict preserves both event lists, and direct/archived coexistence regressions protect the result. Final reviewed head: 14fa2278632a7f3a2e46d2771e6d78b403a3c846. Live refresh at 2026-09-16 21:06:28 UTC: base 4148ab3, head 14fa2278632a7f3a2e46d2771e6d78b403a3c846, 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.

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.

github-actions Bot and others added 9 commits September 16, 2026 19:37
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mohgupta-ship-it
mohgupta-ship-it marked this pull request as ready for review September 17, 2026 15:10

@yashrajp22 yashrajp22 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 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):

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.

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-incomplete

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

Reviewed exact head 183fd554e5b41208f2ea8d9211d084de3972fe3b across the complete production and test diff.

Changes are still required for two independently reproducible regressions already documented inline:

  1. 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.
  2. 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.

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.

4 participants