Repository navigation
fix(e5): distinguish proved cloud downloads from uploads - #891
chrisknvidia wants to merge 25 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.
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 main's E1 command parsing is preserved.Rebased onto
e14c450b; current headbdc072d5. This includes upstream formatting and frozen-dependency updates; all five feature patches remain unchanged.Independent RG, runtime, regular code and structured security delta reviews found no new actionable PR defect. Native typed review bots were unavailable. Full integration was not rerun for this dependency-only refresh; earlier integration had two Claude CLI settings failures reproduced on its unchanged base. No live model inference or customer production run was performed.
Fresh hosted checks are pending. The known upstream PowerShell assertions remain a validation limitation; no test or timeout was weakened. Re-review requires clean current-head checks.