Skip to content

fix(e5): distinguish proved cloud downloads from uploads - #891

Open
chrisknvidia wants to merge 32 commits into
mainfrom
feat/christopherk/e5-cloud-downloads
Open

chrisknvidia wants to merge 32 commits into
mainfrom
feat/christopherk/e5-cloud-downloads

Conversation

@chrisknvidia

@chrisknvidia chrisknvidia commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #889.

Literal cloud-to-local downloads currently produce E5 findings and fail --fail-on-findings. Suppress only individually proved AWS/GCS downloads through supported cp, non-deleting AWS sync, or gsutil rsync commands. Upload occurrences retain their original locations.

The proof requires bounded, fully observed context and literal operands. Only comments, blank lines, supported literal set flags and simple directory changes are accepted before a proved download. Unknown setup and earlier transfers retain E5. Relative destinations after directory changes, moves, deletion flags, streams, unknown options, dynamic operands and compound commands remain conservative. SDK detection, scoring and exit policy are unchanged.

Current head 9683ba57 integrates main ea5aeef6, preserving the existing branch commits. The import-only conflict is resolved, retaining both upstream E2 flow analysis and E5 direction handling. Independent AST comparison confirms all upstream analyzer definitions and the existing E5 helper are preserved. Current-head CI is running; the older approval-gated run is superseded by this new run.

Validation at this head:

  • 2,363 affected analyzer and shell-ownership tests passed. Full Ruff lint/format and diff checks passed.
  • 94 actual source/noneditable-wheel scans passed: 84 strict CLI reports across 21 fixtures in JSON/SARIF, two default-exit controls, and eight actual MCP scans with initialization, tool discovery and clean shutdown.
  • 47 complete source/wheel behavior pairs matched. Exact E5 locations, strict exits, completeness and MCP/report metadata were verified. All 42 SARIF reports passed project models and the SARIF 2.1.0 JSON schema.
  • All 148 package files and 158 installed distribution versions matched across frozen environments.
  • Independent code/security and retained-output reviews found no actionable findings.

Fixture cloud commands were scanned, never executed. No cloud transfer, live model, customer production, Windows qualification or full integration suite was run. Native typed review bots were unavailable. Initial retained-output audit expectations about MCP format-recovery policy and occurrence representation were corrected against the existing source contract; original audit errors remain retained, with no scanner reruns or source/test weakening.

@chrisknvidia
chrisknvidia marked this pull request as ready for review October 20, 2026 20:10
@chrisknvidia
chrisknvidia requested a review from rng1995 October 20, 2026 20:10
@chrisknvidia
chrisknvidia marked this pull request as draft October 20, 2026 20:17
@chrisknvidia
chrisknvidia marked this pull request as ready for review October 20, 2026 20:24
@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/e5-cloud-downloads branch from 7b64de5 to fa24619 Compare October 21, 2026 03:16

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

Value and readiness

The direction proof is conservative and preserves per-occurrence upload locations. Reviewed code is approvable, although the branch currently conflicts with main and needs reconciliation before merging.

Material findings

No required code changes found. The proof rejects compound context, runtime operands, lookup rebinding, streams, unknown flags and host-language commands. Relative destinations remain conservative after directory changes. No prior blocker threads.

PIC tradeoffs

Only simple AWS/GCS transfers qualify; the 4096-character complete-prefix requirement deliberately limits false-positive relief in long scripts. SDK detection is unchanged.

Verification and gaps

Reviewed exact head 8768a630dcd98ec3ae26db8593165b3cfd1152ca, the full diff, pinned source, description, tests, and all prior review/comment threads. Read the full diff and pinned supply-chain helpers used for logical commands, compound-context ownership and shell tokens. Tests exercise uploads, setup rebinding, streams, CLI strict gates and source locations. No checks are attached to this exact head. Cited hosted run 39537680751 succeeded at fa24619, which is older than this head and is not exact-head CI evidence. Static review only; no contributor code, tests, installs, or external services were executed.

@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/e5-cloud-downloads branch 3 times, most recently from 23ef80e to d526dcb Compare November 2, 2026 19:27
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/e5-cloud-downloads branch from 2349fdc to bdc072d Compare November 2, 2026 20:07

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

Value and readiness

Ready for maintainer review. The per-occurrence proof removes E5 false positives for literal cloud downloads while keeping upload evidence and locations. The prior marked approval had no code blocker; its merge-conflict limitation is resolved at this MERGEABLE head.

Material findings

No required changes found. static_patterns_data_exfiltration.py:190-284 requires complete bounded preceding context, a supported shell host, literal operands, recognized non-deleting flags and a local non-stream destination. Unknown setup, earlier transfers, runtime expansion, command rebinding/compound syntax, and relative destinations after directory changes retain E5. The occurrence loop at lines 664-671 leaves SDK patterns and upload locations intact. Reviewed all 395 added test lines, including strict CLI exits, nearest occurrence locations, lookup/setup changes, context/length boundaries and the shell helpers, streams and host languages. Existing E1 parsing is retained.

PIC tradeoffs

The complete-prefix 4,096-character bound and small preamble/option allowlists deliberately limit false-positive relief. SDK detection and scoring policy remain unchanged. No additional PIC decision identified.

Verification and gaps

Read the complete three-file diff, exact-head analyzer, referenced logical-command/token/compound-context helpers, all marked reviews and comments/threads, and added regression assertions. Exact-head CI run 41516369359: changes, lint, OpenCode TypeScript Tests, DCO and docker-smoke succeeded. test-unit reports 18,047 passed and 15 failed; all failed test IDs exactly match main e14c450b run 41513800480, in PowerShell/quote ownership. There is no additional failing test ID; the overall suite remains red and GitHub reports UNSTABLE. No required-check names are configured. Static review only; no contributor code, tests, imports, builds, setup, regex probes, scanner runs or cloud transfers were executed.

Reviewed head: bdc072d53b6cf66e1b05406f04ed0262ca8c17bd.

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

Decision: Approved

Value and readiness: The scoped direction proof still delivers the requested false-positive correction for #889. Re-reviewed the actual rebased head rather than relying on GitHub's remapped old approval association. No required implementation change remains.

Material findings: The analyzer and all 395 lines of direction tests are byte-identical to persisted reviewed head bdc072d53b6cf66e1b05406f04ed0262ca8c17bd. The complete current three-file diff retains per-occurrence checks, literal supported cloud-to-local operands, complete bounded preceding context, restricted shell hosts and non-deleting options. Dynamic operands, streams, compound/rebound commands, unknown preambles and later uploads retain findings and original locations. The three supply-chain helpers used by the proof are unchanged across the rebase. The shared bounded tokenizer gained upstream diagnostics and optional PowerShell inputs; its existing call contract remains compatible, and the E5 proof excludes those host files. Existing E1 parsing is retained. No prior code blocker has reappeared.

PIC tradeoffs: The 4,096-character complete-prefix bound, literal destination proof and short option/preamble allowlists deliberately retain uncertain transfers. SDK detection, scoring and exit policy remain unchanged. No new PIC decision identified.

Verification and gaps: Read the full current diff, exact-head analyzer/test files, source comparisons to the persisted reviewed head, the changed shared-tokenizer contract, linked #889 and review/discussion/thread evidence. Documentation differences outside this feature come from the synchronized base; the direction documentation remains consistent. No hosted checks attach to this head, so clean exact-head CI is still a merge prerequisite. Static inspection only; no contributor code, tests, snippets, imports, builds, setup, hooks, scanner invocations or cloud transfers executed.

Reviewed head: 95df851b041ae403cc03e2af5f31948d88bea3b6.

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.

E5 labels cloud-to-local downloads as cloud exfiltration

2 participants