COR-1766: report dirty worktree state with corgea scan uploads - #150
COR-1766: report dirty worktree state with corgea scan uploads#150leenk7991 wants to merge 1 commit into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) | ||
| ); | ||
| // Read dirty/sha after packaging so the flag matches the uploaded archive. | ||
| let repo_info = utils::generic::get_repo_info("./").unwrap_or_default(); |
There was a problem hiding this comment.
src/scanners/blast.rs:170 finishes reading the archive before this metadata sample. If another process completes a commit between those points (a normal possibility in CI/watch workflows), this reads the new clean HEAD and upload_zip sends that new SHA with dirty=false, even though the zip contains the previous/mixed snapshot. That makes the backend's commit-diff incremental decision unsound and can skip analysis of files whose uploaded bytes do not match the advertised commit. Capture repo state both immediately before and after packaging, and only advertise a clean snapshot when both samples are clean and have the same SHA; otherwise fail safe to dirty=true. Please cover the reconciliation logic with before/after SHA-change cases.
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Post-packaging Git state may not match archived files
Repository metadata is sampled only after packaging. If HEAD changes while the archive is being created, the upload can advertise the new SHA with dirty=false even though the archive contains files from the previous or a mixed state. Compare state before and after packaging, marking the upload dirty unless both clean samples have the same SHA.
Proof or reproduction:
Start packaging clean commit A, pause after one file is archived, commit changes as B, then resume. The new code observes clean commit B and sends sha=B, dirty=false, although the archive is not commit B.
| // Always send dirty: omitted field = old CLI; "false" = clean tree. | ||
| form = form.part( | ||
| "dirty", | ||
| multipart::Part::text(if info.dirty { DIRTY_TRUE } else { DIRTY_FALSE }), |
There was a problem hiding this comment.
This always equates “clean Git status” with “archive represents clean HEAD,” but blast.rs:87-90 can build a --target archive and blast.rs:170 can omit user---exclude files. Neither selection is sent to the server. Therefore a clean corgea scan blast --target src/a.py (or a clean scan with --exclude) now sends dirty=false for a partial archive and opts it into commit-diff incremental behavior as though it were the full commit. Results can be reused or diffed outside the requested archive scope. Keep the user notice based on actual worktree status, but force the upload's effective dirty/“not exact HEAD” state to true whenever target_str.is_some() or exclude.is_some(); add clean-worktree E2E cases for both options.
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Partial archives are incorrectly marked as clean HEAD
The multipart dirty value depends exclusively on Git worktree status. A clean scan using --target or --exclude therefore sends dirty=false even though its archive is not a complete representation of HEAD. The effective upload state must be dirty when packaging options omit tracked content.
Proof or reproduction:
In a clean repository containing src/a.py and src/b.py, run `corgea scan blast --target src/a.py`. `info.dirty` is false, so the changed code sends `dirty=false`, while the archive omits the tracked src/b.py.
| opts.include_untracked(true) | ||
| .recurse_untracked_dirs(true) | ||
| .include_ignored(false) | ||
| .exclude_submodules(true); |
There was a problem hiding this comment.
exclude_submodules(true) hides a dirty checked-out submodule from this status, while the archive walker at generic.rs:77 still traverses that directory and packages its source files. Thus modified submodule bytes can be uploaded with the parent SHA and dirty=false, allowing an incorrect incremental scan. Include submodule status (or, less usefully, exclude submodule contents from packaging); the minimal safe fix is:
| opts.include_untracked(true) | |
| .recurse_untracked_dirs(true) | |
| .include_ignored(false) | |
| .exclude_submodules(true); | |
| opts.include_untracked(true) | |
| .recurse_untracked_dirs(true) | |
| .include_ignored(false); |
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Modified submodule contents are hidden from dirty detection
exclude_submodules(true) prevents submodule changes from making the status nonempty. Because packaging traverses submodule contents, modified bytes can consequently be uploaded with the parent SHA and dirty=false. Include submodule status or exclude submodule content from the archive.
Proof or reproduction:
Create and commit a submodule, modify a tracked file inside its checkout without updating the parent index, then scan. The status query excludes that submodule and returns empty, causing dirty=false despite the modified file being packaged.
There was a problem hiding this comment.
Automated review risk: 4/5.
The new dirty flag can incorrectly identify partial or inconsistent archives as clean HEAD snapshots, making incremental scan results unsound.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
2 participants
Summary
dirty=true|falsenext tosha) so doghouse can skip commit-diff incremental scans.Test plan
./harness checkcorgea scan— no dirty notice; multipartdirty=falsedirty=true; with COR-1765, full scan (not stale copy)