Repository navigation
fix(e5): distinguish proved cloud downloads from uploads - #891
chrisknvidia wants to merge 32 commits into
Conversation
7b64de5 to
fa24619
Compare
rng1995
left a comment
There was a problem hiding this comment.
[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.
23ef80e to
d526dcb
Compare
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>
2349fdc to
bdc072d
Compare
rng1995
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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.
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Fixes #889.
Literal cloud-to-local downloads currently produce E5 findings and fail
--fail-on-findings. Suppress only individually proved AWS/GCS downloads through supportedcp, non-deleting AWSsync, orgsutil rsynccommands. Upload occurrences retain their original locations.The proof requires bounded, fully observed context and literal operands. Only comments, blank lines, supported literal
setflags 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
9683ba57integrates mainea5aeef6, 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:
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.