ralph/feat/loop sandbox post merge#1579
Conversation
|
📝 WalkthroughWalkthroughUpdates test coverage for LoopCommand's sandbox authorization flow, adding tests for flag presence/absence, error handling, and interactive authentication scenarios with mocked ready states and error paths. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (7)**/*.ts📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{spec,test}.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/cli/**/*.{spec,test}.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/cli/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (17)📓 Common learnings📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T22:09:45.455ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.456ZApplied to files:
📚 Learning: 2025-11-24T22:09:45.455ZApplied to files:
📚 Learning: 2025-11-24T22:09:45.455ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.456ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.456ZApplied to files:
📚 Learning: 2025-11-24T22:09:45.455ZApplied to files:
📚 Learning: 2025-11-24T18:01:44.169ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.713ZApplied to files:
🧬 Code graph analysis (1)apps/cli/src/commands/loop.command.spec.ts (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/cli/src/commands/loop.command.spec.ts`:
- Around line 403-411: The test 'should not check sandbox auth when --sandbox
flag is not provided' is passing sandbox: true which contradicts its intent;
update the call to (loopCommand as any).execute so it does not enable the
sandbox (e.g., pass { sandbox: false } or omit the sandbox property) so that
mockTmCore.loop.checkSandboxAuth is not expected to be called; keep references
to the existing execute binding and mockTmCore.loop.checkSandboxAuth assertion
unchanged.
🧹 Nitpick comments (8)
tests/unit/task-manager/tag-management.test.js (1)
116-124: Consider test placement: unit vs integration.These tests perform real file system operations rather than mocking
readJSON/writeJSON, which is appropriate for validating the atomic file locking behavior. However, per coding guidelines, unit tests intests/unit/should mock file operations and test in isolation.Consider either:
- Moving these tests to an integration test directory (e.g.,
tests/integration/)- Adding a comment explaining these are integration-style tests validating file I/O safety
This is a minor organizational concern—the tests themselves are well-written and provide valuable coverage for the concurrency safety features.
packages/tm-core/src/modules/storage/adapters/file-storage/file-operations.ts (1)
69-106: Consider documenting the console.warn behavior.The lock release warning only appears when
DEBUGorTASKMASTER_DEBUG=trueis set. This is reasonable for production noise reduction, but users debugging lock issues may miss this. Consider adding a note in the class-level JSDoc about enabling debug mode for lock diagnostics.Also, the warning uses
console.warninstead of a structured logger. Per coding guidelines, prefer using the central log function instead of direct console calls.apps/cli/src/ui/display/tables.ts (1)
66-73: Edge case: optionalCols > 4 would use fallback ratios.The fallback
COLUMN_WIDTH_RATIOS[4]is used whenoptionalColsexceeds defined keys. Currently only 4 optional columns exist, so this is safe. However, if more optional columns are added in the future, this fallback may produce unexpected results.Consider a defensive fallback
- const ratios = COLUMN_WIDTH_RATIOS[optionalCols] || COLUMN_WIDTH_RATIOS[4]; + const maxDefinedCols = Math.max(...Object.keys(COLUMN_WIDTH_RATIOS).map(Number)); + const ratios = COLUMN_WIDTH_RATIOS[optionalCols] || COLUMN_WIDTH_RATIOS[maxDefinedCols];scripts/modules/utils.js (1)
40-65: Consider the busy-wait CPU impact for longer backoff delays.The busy-wait fallback (lines 61-64) will spin the CPU when
SharedArrayBufferis unavailable. With exponential backoff, the max wait is100ms * 2^4 = 1600ms, which could cause noticeable CPU spikes.For a file-locking utility that may be called frequently, consider using
setTimeoutwith a sync wrapper pattern or documenting this limitation more prominently.packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts (1)
641-645: Silent fallback when copyFrom references unavailable tag in standard format.When in standard format and
options.copyFromis set to something other than'master'(or undefined),tasksToCopywill be an empty array without warning. This is technically correct since standard format only has master tasks, but the silent behavior could surprise callers.Consider adding a warning or throwing when
copyFromreferences a tag that doesn't exist in standard format:// Get tasks to copy (from master in this case) let tasksToCopy: any[] = []; - if (options?.copyFrom === 'master' || !options?.copyFrom) { + if (!options?.copyFrom || options.copyFrom === 'master') { tasksToCopy = JSON.parse(JSON.stringify(masterTasks)); + } else { + // Standard format only has master; requested copyFrom tag doesn't exist + throw new TaskMasterError( + `Cannot copy from tag '${options.copyFrom}' - only 'master' exists in standard format`, + ERROR_CODES.VALIDATION_ERROR + ); }packages/tm-core/src/modules/loop/services/loop.service.ts (1)
278-283: Consider adding a timeout to prevent indefinite hangs.Unlike
checkSandboxAuthwhich has a 30-second timeout,executeIterationhas no timeout. While long iterations are expected, an indefinite hang (e.g., due to network issues or Claude becoming unresponsive) would require manual process termination.Consider adding a configurable timeout (defaulting to a generous value like 30 minutes) to provide a safety net:
- const result = spawnSync(command, args, { + const timeoutMs = 30 * 60 * 1000; // 30 minutes default + const result = spawnSync(command, args, { cwd: this.projectRoot, encoding: 'utf-8', maxBuffer: 50 * 1024 * 1024, // 50MB buffer - stdio: ['inherit', 'pipe', 'pipe'] + stdio: ['inherit', 'pipe', 'pipe'], + timeout: timeoutMs });This could also be exposed as a config option in
LoopConfigfor user control.apps/cli/src/commands/list.command.ts (2)
71-78: Consider discriminated union for clearer type narrowing.The
tasksfield uses a union typeTaskWithBlocks[] | TaskWithTag[], which requires checkingallTagsto determine the actual type. A discriminated union would provide stronger type safety:export type ListTasksResult = { total: number; filtered: number; storageType: Exclude<StorageType, 'auto'>; } & ( | { allTags: false; tag?: string; tasks: TaskWithBlocks[] } | { allTags: true; tasks: TaskWithTag[] } );This allows TypeScript to automatically narrow the
taskstype based onallTags.
417-422: Type cast is safe but could be avoided.The cast
filterReadyTasks(enrichedTasks) as TaskWithTag[]is safe becauseenrichedTasksalready includestagName. However, you could avoid the cast by using a generic or intersection type in the filter function, or by filtering after assigning totasksToAdd:- const tasksToAdd: TaskWithTag[] = options.ready - ? (filterReadyTasks(enrichedTasks) as TaskWithTag[]) - : enrichedTasks; + const tasksToAdd = options.ready + ? filterReadyTasks(enrichedTasks) + : enrichedTasks; - allTaggedTasks.push(...tasksToAdd); + allTaggedTasks.push(...(tasksToAdd as TaskWithTag[]));Or keep the cast as-is since it's demonstrably safe in this context.
- Fix sandbox auth tests to match implementation (sandbox flag is required) - Add test for sandbox not being checked when flag is not provided - Add test for sandbox auth error handling - Add test for interactive auth failure - Fix integration test to check --sandbox option instead of non-existent --json option
Crunchyman-ralph
force-pushed
the
ralph/feat/loop-sandbox-post-merge
branch
from
af49e8e to
634e63d
Compare
1 participant
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.