feat(coder/modules/git-clone): add support for extra_args and drop depth#893
Conversation
…th extra_args for flexible git clone options
matifali
left a comment
matifali
left a comment
There was a problem hiding this comment.
Assuming you have tested it. Great addition.
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The design decision to replace depth with a generic extra_args passthrough is good. It prevents flag-proliferation and the encoding scheme (base64 + newline-split into bash array) is sound. The test suite is real infrastructure (Terraform + Docker), not mocks. Two new tests cover the happy path.
Severity summary: 1 P1, 3 P2, 2 P3, 2 Nit, 4 Note.
The P1 is the version bump: 9 of 16 reviewers independently flagged that a self-identified breaking change (checkbox checked in the PR template) ships under a minor version (1.4.0 instead of 2.0.0). Multiple in-tree templates use ~> 1.0 constraints and would auto-upgrade into a broken terraform plan. The P2s are a bash <4.4 compatibility regression on the default code path, missing migration guidance for the removed depth variable, and a gap in the test suite for space-containing arguments (the entire reason the base64+array encoding exists).
Process note: the PR title ("add support for recursive submodule cloning and parallel jobs") describes the abandoned typed-variable approach from commit 6117692, not the shipped extra_args passthrough from 9d2e19d. Worth updating before squash-merge so the changelog entry matches what shipped.
"The old code had
depthas a one-off knob with branching logic in the shell script. Addingrecurse_submodules,jobs,filter, etc. as separate variables would be classic feature creep." (Luffy)
🤖 This review was automatically generated with Coder Agents.
35C4n0r
changed the title
|
/coder-agents-review |
There was a problem hiding this comment.
11 of 13 R1 findings addressed and verified by the panel. The fixes are thorough: the installFakeGit shim is a particularly good testing pattern that proves argv boundary integrity where the echo output cannot. The bash <4.4 set -u fix uses the standard ${arr[@]+"${arr[@]}"} idiom. All naming, documentation, and cosmetic fixes confirmed correct.
Two contested findings re-evaluated by the panel and upheld:
DEREM-5 (P1): Version bump. 5 reviewers evaluated the author's defense ("one variable removed, one added"). 4/5 rated P1, 1/5 P2. The panel concluded that the author's description of the change is itself the CONTRIBUTING.md definition of a breaking change: "removing inputs, changing types." The defense does not cite any policy that overrides CONTRIBUTING.md. Version should be 2.0.0. This needs a human decision from a maintainer.
DEREM-7 (P2): Migration guidance. 5 reviewers evaluated the author's defense ("minimal change"). 4/5 rated P2, 1/5 P3. "Minimal" describes the diff size, not the user's recovery effort. One sentence in the README prevents every affected user from independently debugging the same Terraform error.
No new findings. The code and tests are in good shape.
"'One variable has been removed' is exactly the behavior that CONTRIBUTING.md defines as major." (Leorio)
🤖 This review was automatically generated with Coder Agents.
…icated script paths and logging
|
/coder-agents-review |
|
Review posted | Chat Review historydeep-review v0.5.0 | Round 4 | Last posted: Round 4, 18 findings (1 P1, 3 P2, 6 P3, 3 Nit, 5 Note), REQUEST_CHANGES. Review Finding inventoryFindings
Contested and acknowledgedDEREM-5 (P1, main.tf:59) - Version 1.4.0 for self-identified breaking change
DEREM-7 (P2, README.md:188) - No migration guidance from depth to extra_args
Round logRound 1Panel (16 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Chopper, Gon, Leorio, Ging-TS, Kite, Meruem, Robin, Luffy, Zoro, Knov, Razor). Netero first pass: 3 P3, 1 Nit. Panel: 1 P1, 3 P2, 2 Nit, 4 Note new. 2 dropped. Reviewed against ee219a8..9d2e19d. Round 2Panel (6 reviewers: Mafuuu, Pariston, Bisky, Leorio, Kite, Knov). 11 findings verified fixed. DEREM-5 contested, panel re-raised (4/5 P1). DEREM-7 contested, panel re-raised (4/5 P2). No new findings. Reviewed against ee219a8..cf98844. Round 3BLOCKED. DEREM-5 and DEREM-7 silent. Maintainer @matifali confirmed DEREM-5 should be a major bump but no code change pushed. No review. Round 4DEREM-5 fixed (version bumped to 2.0.0). DEREM-7 contested again with same defense, panel upheld 4/4 P2. 1 P3, 1 P3, 1 P3, 1 Nit, 1 Note new from R4 infrastructure changes. Reviewed against f980245..e679ef8. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Review blocked. Two findings from R1 remain unaddressed after 2 rounds of panel review.
DEREM-5 (P1): Version should be 2.0.0, not 1.4.0. Maintainer @matifali confirmed in the thread: "that was by mistake. I missed the variable removal. This should be a major version bump." No code change has been pushed. All 12 README version references still read 1.4.0.
DEREM-7 (P2): No migration guidance from depth to extra_args. No response from the author after the R2 panel re-raise. No changes to the README.
Further review is blocked until these two findings are addressed or explicitly deferred with a ticket.
🤖 This review was automatically generated with Coder Agents.
addressed version bumped to 2.0.0
The change seems too trivial to be given a migration guide. |
|
/coder-agents-review |
…nvention - Add pre_clone_log_path and post_clone_log_path locals in main.tf - Pass log paths to run.sh as template variables - Pipe pre/post clone script execution through tee to dedicated log files
There was a problem hiding this comment.
DEREM-5 (P1, version bump) verified fixed: all README examples now read 2.0.0.
DEREM-7 (P2, migration guidance) upheld for the third time. Author's R4 defense ("too trivial") is the same argument the panel rejected in R2. 4/4 reviewers in this round re-raised at P2. The project's own CONTRIBUTING.md says: "Don't remove existing variables without clear migration path" and "Document any breaking changes clearly." This needs a maintainer decision from @matifali: add the one-sentence migration note, or explicitly accept the gap.
R4 also introduced infrastructure changes (wrapper script, directory scaffolding, tee-based logging) that the panel flagged 3 P3, 1 Nit, 1 Note. The most notable is the fixed module directory path ($HOME/.coder-modules/coder/git-clone/) which creates a collision when a template uses two git-clone module instances.
CI note: "Validate Terraform output" is failing. All local mechanical checks (terraform validate, shellcheck) pass.
Process note: commit e679ef8's subject reads feat(git-clone): update version from 1.4.0 to 2.0.0 across all modules. Updating version strings is docs or chore, not feat; "across all modules" is misleading since only one README was changed.
"'Trivial' describes the effort to write the migration note. It does not describe the effort a user spends debugging without one." (Leorio)
registry/coder/modules/git-clone/README.md:261
P3 [CRF-3] The new log file path (~/.coder-modules/coder/git-clone/logs/clone.log) is not documented.
"The
claude-codemodule documents its log paths in a troubleshooting section with explicitcatcommands. Same convention should apply here." (Leorio)
Suggested addition:
## Troubleshooting
Clone output is logged to `~/.coder-modules/coder/git-clone/logs/clone.log`:
cat ~/.coder-modules/coder/git-clone/logs/clone.log(Leorio P3)
🤖
registry/coder/modules/git-clone/main.test.ts:64
Note [CRF-5] The "runs with git" test does not assert the expected non-zero exit code. Every other failure test in this file asserts exitCode (line 60: === 1, line 337: === 42, line 448: === 43). This test only checks stdout for "fatal". Adding expect(output.exitCode).not.toBe(0) would prove that git clone errors propagate, consistent with the pattern in the rest of the file.
(Chopper)
🤖
🤖 This review was automatically generated with Coder Agents.
|
@35C4n0r can you resolve threads and merge this when ready? thanks |
- Add migration guidance for depth -> extra_args (DEREM-7) - Use folder_name in module_dir to avoid multi-instance collisions (CRF-1) - Remove diagnostic 'Running:' echo lines (CRF-2) - Add Troubleshooting section documenting log paths (CRF-3) - Use robust base64 regex in test (CRF-4) - Add exit code assertion to 'runs with git' test (CRF-5)
… improve test - Quote all path interpolations in coder_script heredoc to handle spaces and shell metacharacters in folder_name - Update README secrets warning to cite actual exposure vectors (script on disk, Terraform state) after removing echo lines - Replace mechanism-coupled extra_args test with behavioral check using the fake-git shim - Remove stray blank line in log test - Add note about -b/--branch conflict with branch_name
…note into NOTE block
|
@matifali also this is what the path would look like for 
|
35C4n0r
added
version:major
#902) ## Description Fix the troubleshooting section placeholder from `<instance>` to `<folder_name>` to match the actual path component used in `module_dir` (`${local.folder_name}`). ## Type of Change - [x] Documentation ## Module Information **Path:** `registry/coder/modules/git-clone` Follow-up to #893. > 🤖 Generated with [Coder Agents](https://coder.com) Co-authored-by: Jay Kumar <jay.kumar@coder.com>
2 participants
Description
Type of Change
Module Information
Path:
registry/coder/modules/git-cloneNew version:
v2.0.0Breaking change: [x] Yes [ ] No
Testing & Validation
bun test)bun fmt)Related Issues
Closes #74