feat: add vibe copy subcommand with Claude Code worktree hook#359
feat: add vibe copy subcommand with Claude Code worktree hook#359
Conversation
Extract copyFiles, copyDirectories, and withConcurrencyLimit from start.ts into a shared module (utils/copy-runner.ts) so the new vibe copy command can reuse the same CoW copy infrastructure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `vibe copy` command that copies files/directories from the main worktree to a secondary worktree using CoW, based on .vibe.toml [copy] configuration. Target resolution: --target flag > stdin JSON .cwd > git repo root. The stdin JSON support allows a simple Claude Code PostToolUse hook (`vibe copy --quiet`) without requiring jq or bash -c wrappers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add English and Japanese documentation for the vibe copy command, including Claude Code hook setup guide. Update command index pages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review:
|
| Area | Status |
|---|---|
Core refactoring (copy-runner.ts) |
✅ Clean |
vibe copy command structure |
✅ Good |
| Bilingual docs (en + ja) | ✅ |
| i18n index updates | ✅ |
| Test coverage | |
| Security (stdin path validation) | validatePath() |
Architecture (start.ts coupling) |
copy-runner.ts |
| Docs diagram style | |
| README update | ❓ Check if needed |
The implementation is solid. The main items to address are path validation for stdin-provided targets and the start.ts coupling. Closing the test gaps and converting the diagram to Mermaid would also be good before merge.
🤖 Generated with Claude Code
- Move resolveCopyConcurrency to copy-runner.ts for better cohesion - Add stdin path validation (isAbsolute, validatePath, MAX_STDIN_SIZE) - Remove non-null assertions on config with optional chaining - Remove 2>/dev/null from hook command for better debuggability - Convert ASCII art to Mermaid diagrams in docs - Expand test coverage with stdin resolution and dry-run tests - Add vibe copy to README command tables Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Development Commands# Run ALL checks for ALL packages (REQUIRED before creating PR)
pnpm run check:all
# Run checks for core package only
pnpm run check:core
# Run checks for docs package only
pnpm run check:docs
# Run checks for video package only
pnpm run check:video
# Run in development mode
pnpm run dev <command>
# Run tests
pnpm run test
# Run a single test file
pnpm run test packages/core/src/path/to/test.ts
# Enable vibe shell function
source .vibedevArchitecture
Source StructureBranch Strategy
PR/Commit Guidelines
Security
|
|
test message please ignore |
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall this is a well-structured addition. The refactoring of shared copy logic into Issues1.
|
| Category | Status |
|---|---|
| Code extraction/refactoring | Clean |
| Test coverage | Good (minor gap noted) |
| Docs (EN + JA) | Both updated and in sync |
| README (EN + JA) | Both updated |
| Security (stdin) | Well-handled |
| Runtime abstraction consistency | process.env / console.warn bypass needs fix |
| Path comparison robustness | Minor concern |
| Settings/docs consistency | Minor inconsistency |
The process.env / console.warn issues in copy-runner.ts are the most worth fixing before merge since they break the runtime abstraction pattern that enables testability.
- Use path.resolve() for worktree path comparison to handle edge cases - Replace process.env with ctx.runtime.env.get() for runtime abstraction - Replace console.warn with warnLog for consistent output handling - Restore 2>/dev/null in hook command to match documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall this is a well-structured PR. The refactoring cleanly extracts shared copy logic, the new command is well-designed for hook usage, and the test/doc coverage is thorough. A few items worth addressing: Issues1.
2. Dead The Minor3. In 4. All existing callers pass 5. Hook command silences stderr entirely "command": "vibe copy --quiet 2>/dev/null || true"With both Strengths
|
- Change console.error to console.warn for debug output consistency - Remove dead code exitCodes field from test helper - Make ctx parameter required in copyDirectories to avoid implicit global context Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall this is a well-structured PR. The extraction of shared copy logic into Issues1. Orphaned JSDoc comment in There are two consecutive JSDoc blocks where the first is orphaned — it describes /**
* Helper to create a mock context for copy command tests.
* Simulates git worktree list with a main worktree and an optional secondary worktree.
*/
/**
* Build a settings.json content that trusts .vibe.toml with skipHashCheck enabled.The first comment should be moved to sit directly above 2. it("executes all items", async () => {
const results: number[] = [];
await withConcurrencyLimit([1, 2, 3, 4, 5], 3, async (item) => {
results.push(item);
});
expect(results).toEqual([1, 2, 3, 4, 5]);
});With 3. Missing test: actual file copy is invoked in non-dry-run mode
Suggestions4. const isMain = resolve(originPath) === resolve(targetPath);
5. const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});The spy is created but not returned or used in any assertion, and each test manually calls 6. Debug log now uses console.warn(`[vibe] Copy strategy: ${dirStrategy.name}`);The old code used Positives
SummaryThe main actionable items are the orphaned JSDoc (#1), the potentially fragile ordering test (#2), and the missing positive-path copy test (#3). The |
- Move orphaned JSDoc comment to correct position above createCopyTestContext - Fix withConcurrencyLimit test ordering fragility by sorting results - Add positive-path test verifying copyFile is called in non-dry-run mode - Remove redundant manual consoleErrorSpy.mockRestore() calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kexi
left a comment
There was a problem hiding this comment.
PR Review: feat: add vibe copy subcommand with Claude Code worktree hook
Task 1: Product Manager Review
Business Value: HIGH. This PR introduces vibe copy as a standalone command, decoupled from vibe start. This is a smart product move because:
- It enables integration with third-party tools (Claude Code) via hooks, expanding the product's ecosystem reach.
- It follows the Unix philosophy of composability — one command, one purpose.
- It solves a real pain point: when a worktree already exists but files from the main worktree need to be re-synced.
User Experience: GOOD. The target resolution priority (--target > stdin > cwd) is well-designed for multiple usage modes (manual, hook, programmatic). The --quiet flag combined with || true in the hook config ensures silent operation in automated flows. The error message "Not in a worktree" is clear.
Strategic Alignment: The Claude Code integration via PostToolUse hook is strategically valuable — it positions vibe as a first-class companion for AI coding assistants. Having a .claude/settings.json shipped with the project is a good demonstration for users.
No actionable issues found.
Task 2: Developer Review
Code Quality & Maintainability: EXCELLENT.
-
Refactoring is clean: The extraction of
copyFiles,copyDirectories,withConcurrencyLimit, andresolveCopyConcurrencyfromstart.tsintocopy-runner.tsis well-executed. Bothstart.tsandcopy.tsnow share the same logic without duplication. -
API design is good: The
copyFilesandcopyDirectoriesfunctions now acceptstring[]patterns instead ofVibeConfig, making them more reusable and testable. This is the right level of abstraction. -
Coding standards: The code follows the project's established patterns:
- Named boolean conditions (e.g.,
const isMain = ...,const hasNoCopyConfig = ...) per CLAUDE.md - Early returns throughout
- Dependency injection via
AppContext - Proper use of
OutputOptionsfor log control
- Named boolean conditions (e.g.,
-
Minor observation —
copy-runner.tsusesconsole.warndirectly on line 143 (console.warn(\[vibe] Copy strategy: ...`)), but the rest of the codebase useswarnLog(). This is in a debug-only code path (VIBE_DEBUG), so it's consistent with the originalstart.ts` behavior. Not a blocker, but worth noting for future cleanup. -
The
readTargetFromStdinfunction is well-implemented with:- TTY detection to avoid blocking on interactive terminals
- Size limit (1 MB) to prevent resource exhaustion
- Path validation via
validatePath()andisAbsolute()check - Graceful fallback on any error
Performance & Scalability: No concerns. The concurrency control is inherited from the existing implementation.
No blocking issues.
Task 3: Quality Engineer Review
Test Coverage: GOOD overall.
-
copy.test.ts(472 lines) covers:- Main worktree detection (3 cases)
- No config scenario
--targetoption--dry-runmode (2 cases)- Actual file copy execution
- stdin target resolution (6 cases: valid JSON, invalid JSON, empty, missing cwd, --target override, terminal)
-
copy-runner.test.tscoverswithConcurrencyLimit(5 cases) -
All 372 tests pass.
Missing test coverage (recommendations):
-
--targetwith relative path: Currently--targetdoes not validate the path withvalidatePath(). The stdin path goes throughvalidatePath(), but the--targetCLI argument does not. This is an inconsistency. While--targetis a locally-provided argument (less risk than stdin), addingvalidatePath()for defense-in-depth would be consistent. See Security section below. -
No test for
readTargetFromStdinwith oversized payload: The 1 MB limit is defined but not tested. A test that sends >1 MB and verifiesundefinedis returned would be valuable. -
No test for
--targetwith relative path: What happens when--target ./some-pathis provided?resolve()will normalize it relative to CWD, but this behavior isn't tested. -
No integration test for the
vibe startrefactoring: The refactoring changedstart.tsto use the sharedcopy-runner.tsfunctions. While existingstart.test.tstests pass, there are no explicit tests verifying the refactoredrunCopyAndPostHooksstill invokes the shared functions correctly.
Regression Risk: LOW. The refactoring preserves the original function signatures and behavior. The start.test.ts import update from ./start.ts to ../utils/copy-runner.ts for resolveCopyConcurrency confirms the move was tracked.
Task 4: Security Engineer Review
-
stdin input handling (GOOD):
- TTY check prevents blocking on interactive terminals
- 1 MB size limit prevents memory exhaustion
JSON.parseis wrapped in try/catch- Path is validated via
isAbsolute()andvalidatePath()(null byte, newline, command substitution checks)
-
--targetoption lacksvalidatePath()call (LOW RISK):- The
--targetCLI argument is provided by the local user (not untrusted input like stdin), so the risk is minimal. - However, for defense-in-depth consistency with stdin handling, consider adding
validatePath(target)before using the--targetvalue. This is especially relevant if--targetis ever used programmatically.
- The
-
Path traversal via glob patterns (EXISTING RISK, NOT INTRODUCED BY THIS PR):
- The
expandCopyPatternsandexpandDirectoryPatternsfunctions use patterns from.vibe.toml, which is already protected by the SHA-256 trust mechanism.
- The
-
Hook command in
.claude/settings.json(ACCEPTABLE):vibe copy --quiet 2>/dev/null || trueis a safe command. The2>/dev/null || truesuppression is appropriate for a hook that shouldn't block the user workflow.- The
timeout: 120is reasonable for large CoW copy operations.
-
No shell string execution (GOOD): All subprocess invocations use
spawnwith argument arrays.
No critical security issues.
Summary
This is a well-executed PR. The refactoring is clean, the new command follows established patterns, tests are comprehensive, and security considerations are well-addressed. The Claude Code integration is a smart strategic addition.
Verdict: APPROVE with the following minor suggestions:
- Consider adding
validatePath()for the--targetoption for defense-in-depth consistency - Consider adding a test for the 1 MB stdin size limit
- Consider adding a test for
--targetwith a relative path
Code ReviewAll CI checks pass (lint, typecheck, tests, e2e, docs, builds on all platforms). Here are my findings. SummaryThis PR cleanly adds Strengths
Issues / Suggestions
validatePath(cwd);
return cwd;If
const copyFileSpy = vi.spyOn(CopyService.prototype, 'copyFile').mockResolvedValue(undefined);This works as long as Hook silences all stderr (
Mermaid diagram shows sequential steps as parallel arrows ( Steps D, E, F (read stdin, load config, copy files) are sequential but fan out from C, implying parallelism. Minor doc nit. Confirm The diff adds No concerns on
Minor nitThe test Overall this is well-implemented. The main item worth a second look before merging is the |
- Add isAbsolute() and validatePath() for --target option (defense-in-depth) - Change console.warn to warnLog for debug output consistency - Add tests for stdin 1MB size limit, relative path rejection, null byte detection, and source/destination path verification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review:
|
| Category | Status |
|---|---|
| Code quality | Good — clean refactoring, consistent style |
| Correctness | Good — target resolution logic is sound |
| Security | Good — stdin validated, path checked, size-limited |
| Tests | Good — edge cases covered; one minor missing scenario |
| Docs | Complete — EN + JA, README updated |
| License | No issues |
The minor issues above are all non-blocking. The most actionable one is the vibe copy ordering inconsistency in README vs help text.
- Align vibe copy ordering in README (EN/JA) with help text - Fix Mermaid diagram to show sequential flow instead of parallel - Add test for stdin providing main worktree path as cwd Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall this is a well-structured PR. The refactoring of shared copy logic into Code Quality & ArchitectureStrengths:
Minor observations:
SecurityStrengths:
Potential concern:
Potential Issues
Test CoverageStrengths:
Gaps:
Documentation
All looks good here. License CompatibilityNo new dependencies introduced. The PR only reorganizes existing code and adds new files using existing internal modules. No license concerns. Hook Configuration (
|
- Use realPath() instead of resolve() for symlink-aware main worktree detection - Add warnLog for stdin payload exceeding 1MB size limit - Remove unused resolve import from node:path - Add test for directory copy with dirs config - Import warnLog from output.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall AssessmentWell-structured PR that cleanly extracts shared copy logic into a reusable module and adds a new Code Quality & ArchitectureStrengths:
Suggestions:
SecurityStrengths:
No concerns found. The stdin input handling is appropriately defensive. No new shell string execution is introduced — Test CoverageStrengths:
Potential gaps:
PerformanceNo concerns. The command reuses the existing DocumentationBoth English and Japanese docs are well-synchronized with identical structure. The Mermaid diagram clearly illustrates the hook flow. The comparison table with README.md and README.ja.md are both updated with the new command in the command table. The command index pages ( Minor note: The License CompatibilityNo new dependencies are introduced. The PR only adds new source files and refactors existing code. No license concerns.
|
- Move resolveCopyConcurrency to only run when dirs config exists - Add error propagation test for git command failures - Add consoleWarnSpy assertion in stdin 1MB size limit test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverviewThis PR adds a Code Quality & ArchitectureStrengths:
Minor observations:
SecurityWell-handled:
One consideration:
Performance
Test CoverageGood coverage (625 lines for copy command, 51 for copy-runner):
Suggestions for additional coverage:
Documentation
All documentation requirements appear met. License CompatibilityNo new dependencies were introduced. The change only reorganizes existing code and adds new source files. No license concerns.
|
- Add 10 tests for resolveCopyConcurrency: default, config, env override, boundary values (1, 32), invalid values (0, -1, 33, non-numeric) - Add test for stdin cwd with relative path rejection - Align start.ts to only call resolveCopyConcurrency when dirs exist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: feat: add vibe copy subcommand with Claude Code worktree hookOverall this is a well-structured PR. The refactoring of shared copy logic into Code Quality & DesignPositives:
Minor suggestions:
Potential Issues
SecurityThe security posture is strong:
No security concerns identified. Performance
Test CoverageTests are comprehensive with 641 lines covering:
Gaps to consider:
Documentation
LicenseNo new dependencies introduced. All changes are internal code refactoring and new command implementation. No license concerns.
|
Summary
copyFiles,copyDirectories,withConcurrencyLimit) fromstart.tsintoutils/copy-runner.tsfor reusevibe copysubcommand that copies files/directories from main worktree to secondary worktree using CoW based on.vibe.toml[copy]configurationPostToolUse(EnterWorktree)hook in.claude/settings.jsonfor automatic copying whenclaude --worktreecreates a worktreevibe copyreads target path from stdin JSON (.cwdfield), enabling a simple hook command withoutjqorbash -cwrappersTest plan
pnpm run check:allpasses (362 tests, lint, typecheck, docs)vibe copy --dry-runin a worktree → shows copy targetsvibe copyin a worktree → CoW copy executesvibe copyin main worktree → error messageclaude --worktree test→ hook auto-copies🤖 Generated with Claude Code