Repository navigation
Snapshot submodule contents as plain files instead of refusing the repo - #489
Conversation
The independent-snapshot lane threw borrowed_snapshot_submodules_unsupported on any index carrying a gitlink, so borrowed review was impossible on every repo using a submodule — including kcap-server, which pins its CLI that way and so could not be reviewed by any snapshot vendor at all. Submodules now arrive as PLAIN CONTENT. Borrowing means the reviewer sees the developer's working tree, and for a submodule that is the checked-out files — not the pinned commit a fresh clone would give. Dirty and untracked files in a submodule are carried for exactly the same reason they are in the superproject. Their .git is never copied: it is a gitlink file pointing into the superproject's .git/modules, which this snapshot deliberately does not have, so carrying it would leave a dangling reference. The submodule gains no git identity inside the snapshot. Every submodule path is prefixed to be relative to the SUPERPROJECT before it enters the manifest loop, so it flows through the same containment, symlink, reserved-path, encoding and capacity guards as a superproject path — a submodule is not a second, weaker lane. Prefixes stay raw bytes until that loop's own decode, preserving its classify-before-decode guarantee, and the submodule directory is resolved through ContainedPath before its git is invoked, so an index-supplied path cannot steer `git -C` out of the tree. An uninitialized submodule (nothing checked out) is skipped rather than failing the snapshot.
PR Summary by QodoSnapshot submodule working-tree files in borrowed snapshots (no submodule .git)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Review found a real escape. ContainedPath is LEXICAL — Path.GetFullPath does not resolve links — so a gitlink whose working-tree directory is a symlink passed containment while `git -C` followed it and enumerated a tree outside the source. The per-file symlink guards cannot undo a directory-level traversal that already happened. The new test proves the traversal rather than merely asserting a message: before the fix it failed with "git ls-files ... fatal: not a git repository", i.e. git had already run inside the symlink target. Had that directory been a repo, its files would have been snapshotted. Also from review: - the prefix decode now raises the coded borrowed_snapshot_invalid_path_encoding instead of a bare DecoderFallbackException - nested submodules FAIL with a coded error rather than being silently omitted; a silently incomplete snapshot is invisible to the reviewer, who would then report on source it never saw
|
Copilot review flow: clean at round 2 (context-only). Round 1 found a real security escape — the best finding of the run — plus two correctness gaps. All three fixed in The escape (reviewer's #1/#6)
The test was written first, and its RED output proves the traversal rather than merely asserting a message: git had already been invoked inside the symlink target. Had that directory been a git repo, its files would have been enumerated and copied into the snapshot. Fix: Also fixed
Deliberately unchangedAn uninitialized submodule still skips silently. The asymmetry with the nested case is intentional: an uninitialized submodule has no checked-out content in the developer's own tree either, so the snapshot faithfully mirrors what they have — whereas nested content exists and would be dropped. Reviewer agreed on round 2.
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo caught a real bug in my own guard. `git submodule` leaves an EMPTY directory for an uninitialized submodule, so Directory.Exists is true there, `git -C` fails, and the whole snapshot aborts — the exact opposite of the skip that guard was written to perform. Gated on the submodule's own .git instead. Also from review: - dropped the now-dead duplicate `ls-files --deleted` call, whose output stopped being read when deletedRecords replaced it - the submodule test now removes the helper-created upstream/clone repos, not just the worktree root - trimmed the narrative comment blocks per the repo's minimal-comments rule
|
All Qodo findings addressed. 🐞 #1 Uninitialized submodule aborts the snapshot — real, and my bug. 🐞 #5 Redundant 🐞 #6 Test leaked temp repos — real, fixed. The submodule test now removes both helper-created upstream/clone pairs alongside the worktree root. 🐞 #4 Unguarded submodule path decode — already fixed before this run. Qodo reviewed 📘 #2, #3 Verbose comments — trimmed, in both the production block and the test's doc comment.
|
1 participant
Fixes AI-1794
The problem
The
IndependentSnapshotborrowed-review lane refused any repository whose index carried a gitlink:kcap-serverpinssrc/clias a submodule, so our own primary repo could not be borrowed-reviewed by any snapshot vendor. Measured effect across vendors on a kcap-server changeset:IndependentSnapshotIndependentSnapshotApproach
Submodules are snapshotted as plain content. Borrowing means the reviewer sees the developer's working tree, and for a submodule that is the checked-out files — not the pinned commit a fresh clone gives. Dirty and untracked files inside a submodule are carried for the same reason they are in the superproject.
Its
.gitis never copied: that is a gitlink file pointing into the superproject's.git/modules, which the snapshot deliberately does not have, so copying it would leave a dangling reference. The submodule gains no git identity inside the snapshot.Security-relevant details
This lane has history (AI-1675 was a credential-exfiltration-via-symlinks finding), so the new path source deliberately gets no shortcuts:
EnsureNoLinkedComponentsnow walks the submodule directory component too, so a symlinked submodule dir is caught.ClassifyReservedPath.ContainedPathbefore its git is invoked, so an index-supplied path cannot steergit -Coutside the source tree.Testing
BorrowedSnapshot_CarriesSubmoduleFilesAsPlainContentWithoutItsGit— watched failing first with exactlyborrowed_snapshot_submodules_unsupported. It asserts the submodule's dirty tracked file and its untracked file both arrive (so it can't pass on a pinned-commit checkout), and that no.gitexists at the submodule path in either file or directory form, while the superproject's own independent.gitstill does.Worth noting: the guard being removed had no test at all —
rg submodules_unsupported test/returned nothing.Local:
WorktreeManagerTests22 total / 0 failed,BorrowedReviewContextTests19/0,AgentOrchestratorVendorTests207/0.Out of scope
Option 4 from the issue — making the claude owned-worktree fallback refuse or warn on a submodule repo instead of silently reviewing at the stale pin — is deliberately not in this PR. It's a different mechanism (no snapshot involved) and separable, but it is the more dangerous half of the original report, since it fails silently rather than loudly. Worth its own issue.
🤖 Generated with Claude Code