feat: agent engine with orchestrator, parallel workers, and workspace intelligence#13
Merged
feat: agent engine with orchestrator, parallel workers, and workspace intelligence#13
Conversation
Covers jam go (interactive) + jam run (one-shot) sharing a new agent engine with orchestrator, parallel workers, tiered permissions, OS sandbox, multimodal image input, workspace intelligence, and context compaction.
16 tasks covering types, errors, config, permissions, sandbox, multimodal, file-lock, provider pool, conventions analyzer, workspace intel, planner, worker, orchestrator, progress reporter, jam run upgrade, and jam go rewrite.
Default path now delegates to the new Orchestrator from src/agent/orchestrator.ts, which supports parallel subtask dispatch, workspace profiling, and progress reporting. The legacy agentic loop is preserved and accessible via JAM_LEGACY_RUN=1 env var. New CLI flags: --auto, --workers, --image, --no-sandbox, --file, --json.
📝 WalkthroughWalkthroughIntroduces a complete agent subsystem: types, planner, orchestrator, worker runtime, file locking, sandboxing, permissioning, provider pool, progress/multimodal utilities, workspace profiling, CLI integrations, config schema/defaults, expanded error codes, and extensive unit/integration tests. Changes
sequenceDiagram
participant CLI as CLI User
participant Orch as Orchestrator
participant WI as WorkspaceIntel
participant Plan as Planner
participant Lock as FileLockManager
participant Pool as ProviderPool
participant Worker as WorkerExec
participant Prov as Provider
participant Progress as ProgressReporter
CLI->>Orch: execute(prompt, options)
Orch->>WI: buildWorkspaceProfile(root)
WI-->>Orch: profile
Orch->>Plan: generateTaskPlan(adapter, prompt, profile)
Plan->>Prov: chatWithTools(...)
Prov-->>Plan: plan JSON
Plan-->>Orch: TaskPlan
Orch->>Progress: onEvent(plan-ready)
Orch->>Lock: assignOwnership(...) for subtasks
Orch->>Orch: topologicalSort(dependencyGraph)
loop subtasks in order
Orch->>Progress: onEvent(worker-started)
Orch->>Pool: acquire()
Pool-->>Orch: lease
Orch->>Worker: executeWorker(subtask, context, signal, deps)
Worker->>Prov: chatWithTools(...)
Prov-->>Worker: toolCalls / responses
Worker->>Orch: WorkerResult
Orch->>Lock: releaseAll(workerId)
Orch->>Pool: release(lease)
Orch->>Progress: onEvent(worker-completed)
end
Orch->>Progress: onEvent(all-done)
Orch-->>CLI: OrchestratorResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (15)
src/agent/config.test.ts (1)
37-39: Consider adding upper bound test formaxWorkers.The schema constrains
maxWorkerstomin(1).max(10), but only the lower bound is tested. Consider adding a test formaxWorkers: 11to ensure the upper bound is also enforced.💡 Suggested additional test case
it('rejects maxWorkers < 1', () => { expect(() => JamConfigSchema.parse({ agent: { maxWorkers: 0 } })).toThrow(); + expect(() => JamConfigSchema.parse({ agent: { maxWorkers: 11 } })).toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/config.test.ts` around lines 37 - 39, Add an upper-bound test for maxWorkers by asserting JamConfigSchema.parse({ agent: { maxWorkers: 11 } }) throws; update the existing test suite (the test named 'rejects maxWorkers < 1' or create a new it block) to include a case that ensures maxWorkers > 10 is rejected via JamConfigSchema.parse, mirroring the lower-bound test that uses expect(...).toThrow(); ensure you reference JamConfigSchema.parse and the agent.maxWorkers field in the new test.src/agent/file-lock.ts (1)
11-20: Note:assignOwnershipsilently overwrites existing ownership.If a file path is already owned by another worker, this will silently reassign it without clearing the old worker's
workerFilesset. This could lead to stale entries inworkerFilesif ownership is reassigned after initial planning.If this is intentional (only called once per worker during initial planning), consider adding a comment to clarify the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/file-lock.ts` around lines 11 - 20, The assignOwnership method currently overwrites ownership in owners without cleaning up the previous owner's workerFiles set; update assignOwnership (in file-lock.ts) to first check const prev = this.owners.get(file.path) and if prev && prev !== workerId then remove file.path from this.workerFiles.get(prev) (and delete the set if it becomes empty) before setting the new owner and adding to this.workerFiles for workerId; alternatively, if reassignment should never occur, add a clear comment stating the method is only called once per worker and consider throwing or asserting on conflicting ownership instead.src/agent/types.ts (1)
144-144: Unused variable:parentis populated but never read.The
parentmap is set during DFS traversal but isn't used in the cycle path reconstruction (which usesstackinstead). This appears to be dead code.♻️ Remove unused variable
const visited = new Set<string>(); const stack = new Set<string>(); - const parent = new Map<string, string>(); function dfs(node: string): string | null { if (stack.has(node)) return node; if (visited.has(node)) return null; visited.add(node); stack.add(node); for (const dep of graph.get(node) ?? []) { - parent.set(dep, node); const cycleNode = dfs(dep); if (cycleNode !== null) return cycleNode; } stack.delete(node); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/types.ts` at line 144, Remove the dead "parent" Map by deleting its declaration and any assignments to it in the DFS/cycle-detection logic (the Map named "parent" that is populated during the DFS but never read); ensure cycle reconstruction continues to use the existing "stack" logic (and update the DFS/detectCycle function to not reference "parent"), leaving behavior unchanged and eliminating the unused variable.src/agent/provider-pool.test.ts (1)
64-72: Timing-based test may be flaky in CI.The test relies on elapsed time measurements which can be unreliable in resource-constrained CI environments. The 10ms slack helps, but consider using
vi.useFakeTimers()for more deterministic testing if this becomes flaky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/provider-pool.test.ts` around lines 64 - 72, The timing-based test in provider-pool.test.ts is flaky; switch to deterministic fake timers: call vi.useFakeTimers() at test start, invoke pool.pauseForRateLimit(50), then call pool.acquire() and advance timers with vi.advanceTimersByTime(50) (or 40ms if you preserve slack) before resolving the promise, then restore timers with vi.useRealTimers(); reference ProviderPool.pauseForRateLimit and ProviderPool.acquire to locate the code to update the test.src/agent/provider-pool.ts (1)
61-73: Consider guarding against double-release.The
release()callback doesn't prevent being called multiple times. If a caller accidentally callsrelease()twice,activecould go negative, corrupting the pool state.🛡️ Proposed fix to guard against double-release
private createLease(): ProviderLease { + let released = false; return { adapter: this.adapter, release: () => { + if (released) return; + released = true; this.active--; const next = this.queue.shift(); if (next) { this.active++; next(this.createLease()); } }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/provider-pool.ts` around lines 61 - 73, The createLease() implementation currently returns a release() that can be invoked multiple times, allowing this.active to go negative; modify createLease() to capture a local boolean (e.g., released = false) in the closure and make release() a no-op if released is already true, only decrement this.active, shift the queue, increment this.active and call next(this.createLease()) on the first invocation, then set released = true; reference the createLease() method, ProviderLease.release, this.active and this.queue.shift() when applying the change.src/agent/integration.test.ts (1)
45-57: Module-level mutable stateworkerCallCountcould cause flaky tests if test isolation fails.While
beforeEachresets this counter, if tests run in parallel or isolation is compromised, this could lead to flaky results. Consider usingvi.fn().mock.calls.lengthinstead.💡 Alternative using mock call tracking
The mock already tracks calls via
vi.fn(). Instead of a separate counter:// In test: const { executeWorker } = await import('./worker.js'); expect((executeWorker as any).mock.calls).toHaveLength(2);This is already done in the second test, so you could remove
workerCallCountentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/integration.test.ts` around lines 45 - 57, Remove the module-level mutable counter workerCallCount and rely on the mock's built-in call tracking for executeWorker instead: delete workerCallCount and its beforeEach reset, import or access the mocked executeWorker from the vi.mock setup (symbol: executeWorker) and replace any assertions or dependency on workerCallCount with checks against (executeWorker as any).mock.calls.length; ensure the vi.fn() mock implementation remains unchanged and tests assert mock.calls length where they previously checked workerCallCount.src/commands/go.ts (2)
181-184:process.exit(0)in close handler may not wait for pending operations.If there are pending async operations (like MCP shutdown or orchestrator cleanup),
process.exit(0)will terminate immediately without waiting for them.💡 Consider graceful shutdown
rl.on('close', () => { process.stderr.write('\nBye!\n'); - process.exit(0); + // Allow event loop to drain before exiting + setImmediate(() => process.exit(0)); });Or handle Ctrl+C separately to ensure
mcpManager.shutdown()is called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/go.ts` around lines 181 - 184, The rl.on('close') handler currently calls process.exit(0) which can kill pending async cleanup; change it to perform a graceful shutdown by invoking the async cleanup functions (e.g., call await mcpManager.shutdown() and any orchestrator cleanup) and then exit when they complete, or instead move termination logic to a SIGINT/SIGTERM handler that runs the shutdown sequence (call mcpManager.shutdown(), orchestrator.shutdown() or similar) and only then calls process.exit(0); locate the rl.on('close') handler and the mcpManager/orchestrator shutdown methods to implement this ordering.
96-100:mcpManager.shutdown()should be awaited before closing readline.The shutdown call is awaited, but
rl.close()is called immediately after within the async handler. Sincerl.on('line', ...)callback is async, ensure the shutdown completes before the readline closes.♻️ Proposed fix
if (input === '/exit' || input === '/quit') { await mcpManager.shutdown(); rl.close(); return; }The current code looks correct since
awaitis present. However, therl.on('line', async (line) => { ... })pattern means errors in the async callback won't be caught by the outer try-catch. Consider wrapping the handler body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/go.ts` around lines 96 - 100, The async readline handler (rl.on('line', async (line) => { ... })) can swallow errors and may call rl.close() before shutdown truly completes; wrap the handler body in a try/catch (or try/finally) so errors are propagated/handled, and ensure you await mcpManager.shutdown() before calling rl.close() (e.g., call await mcpManager.shutdown() inside the try/finally and only invoke rl.close() after that await completes) — adjust the logic around the rl.on('line' ...) callback and the mcpManager.shutdown()/rl.close() calls to guarantee shutdown completes and errors are caught.src/agent/multimodal.test.ts (1)
104-122: Temp files may not be cleaned up if test assertions fail.If
loadImageor assertions throw,unlinkis never called, leaving orphaned temp files. Consider using try/finally or Vitest's hooks for cleanup.♻️ Proposed fix using try/finally
it('loads a PNG file', async () => { const { writeFile, unlink } = await import('node:fs/promises'); const path = '/tmp/test-jam-multimodal.png'; - await writeFile(path, Buffer.from([137, 80, 78, 71, 13, 10, 26, 10])); - const result = await loadImage(path); - expect(result.mediaType).toBe('image/png'); - expect(result.data).toBeTruthy(); - expect(typeof result.data).toBe('string'); // base64 - await unlink(path); + try { + await writeFile(path, Buffer.from([137, 80, 78, 71, 13, 10, 26, 10])); + const result = await loadImage(path); + expect(result.mediaType).toBe('image/png'); + expect(result.data).toBeTruthy(); + expect(typeof result.data).toBe('string'); // base64 + } finally { + await unlink(path).catch(() => {}); // ignore if already deleted + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/multimodal.test.ts` around lines 104 - 122, The tests creating temp files ('loads a PNG file' and 'detects JPEG media type') can leak files if loadImage or assertions throw; wrap the writeFile/loadImage/assert sequence in a try/finally (or use Vitest afterEach) to ensure unlink is always called: for each test that calls writeFile(path, ...) and then await loadImage(path), move the await unlink(path) into a finally block (or register cleanup in afterEach using the same path), referencing the test cases by their descriptions or the used symbols writeFile, unlink, and loadImage so temp files are removed even on failures.src/agent/planner.ts (1)
53-61: Consider throwing a clearer error when adapter doesn't supportchatWithTools.If the adapter lacks
chatWithTools, the function silently returnsnullfor the response, which then throws a generic "empty response" error. A more specific error would aid debugging.♻️ Proposed improvement
// Call provider - const response = adapter.chatWithTools - ? await adapter.chatWithTools(messages, [], { + if (!adapter.chatWithTools) { + throw new JamError( + 'Provider does not support chatWithTools required for planning', + 'AGENT_PLAN_FAILED' + ); + } + + const response = await adapter.chatWithTools(messages, [], { model: options?.model, temperature: options?.temperature ?? 0.3, maxTokens: options?.maxTokens ?? 2000, systemPrompt, - }) - : null; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/planner.ts` around lines 53 - 61, The current call to adapter.chatWithTools sets response to null when the adapter lacks chatWithTools, leading to a generic "empty response" error later; update the logic in the planner function that calls adapter.chatWithTools so that if adapter.chatWithTools is undefined it throws a clear, descriptive error (e.g., "Adapter does not support chatWithTools") instead of returning null; ensure the thrown error is raised before any downstream use of the response variable so callers see the specific failure originating from adapter.chatWithTools.src/agent/planner.test.ts (1)
83-86: Consider asserting the specific error code for invalid JSON.The test verifies that an error is thrown but doesn't assert the error type or message. This could mask regressions if the error behavior changes.
💡 Suggested improvement
it('throws AGENT_PLAN_FAILED on invalid JSON', async () => { const adapter = makeAdapter('not json at all'); - await expect(generateTaskPlan(adapter, 'test', mockProfile)).rejects.toThrow(); + await expect(generateTaskPlan(adapter, 'test', mockProfile)).rejects.toThrow('AGENT_PLAN_FAILED'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/planner.test.ts` around lines 83 - 86, The test currently only checks that generateTaskPlan throws for invalid JSON; update it to assert the specific error code or message (e.g., AGENT_PLAN_FAILED) so regressions are caught. Replace the generic rejects.toThrow() with an assertion that inspects the rejection shape from generateTaskPlan(makeAdapter('not json at all'), 'test', mockProfile), for example using await expect(...).rejects.toMatchObject({ code: 'AGENT_PLAN_FAILED' }) or await expect(...).rejects.toThrow(/AGENT_PLAN_FAILED/) depending on whether the thrown value is an Error or an object; reference generateTaskPlan and makeAdapter when making the change.src/agent/workspace-intel.test.ts (1)
23-28: Caching test verifies value equality but not that the cache was actually used.The current test only confirms that two calls return the same values, which would also pass if caching were broken and the profile was recomputed each time. Consider spying on the underlying detection functions to verify they're only called once.
💡 Suggested improvement to verify cache hit
+import { vi } from 'vitest'; +import * as conventions from '../intel/conventions.js'; + it('returns cached profile on second call', async () => { + const spy = vi.spyOn(conventions, 'analyzeConventions'); const p1 = await buildWorkspaceProfile(ROOT); const p2 = await buildWorkspaceProfile(ROOT); expect(p1.language).toBe(p2.language); expect(p1.testFramework).toBe(p2.testFramework); + // If caching works, analyzeConventions should only be called once (or not at all on second call) + expect(spy).toHaveBeenCalledTimes(1); + spy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/workspace-intel.test.ts` around lines 23 - 28, The test currently only compares values from two buildWorkspaceProfile(ROOT) calls but doesn't assert the cache was used; modify the test to spy on the underlying detection helpers used by buildWorkspaceProfile (e.g., detectLanguage and detectTestFramework or whatever detection functions are imported inside buildWorkspaceProfile) with jest.spyOn (or your test spy utility), call buildWorkspaceProfile(ROOT) twice, then assert each detection helper was invoked only once, and finally restore/clear the spies so other tests are unaffected.src/agent/orchestrator.ts (2)
82-83: Consider logging when subtask lookup fails.If
subtaskIdfrom the dependency graph doesn't match any subtask inplan.subtasks, the loop silently continues. This could mask bugs in plan generation.♻️ Suggested improvement for debuggability
const subtask = plan.subtasks.find(s => s.id === subtaskId); - if (!subtask) continue; + if (!subtask) { + console.warn(`[Orchestrator] Subtask ID "${subtaskId}" in dependency graph not found in plan.subtasks`); + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/orchestrator.ts` around lines 82 - 83, When a dependency subtaskId isn't found, add a debug/warn log so missing mappings don't silently fail: in the loop where you resolve subtask (the lines using plan.subtasks.find and const subtask = ...), emit a log message (e.g., logger.warn or console.warn) including the missing subtaskId and identifying plan context (plan.id or other plan fields) before continuing; keep the existing continue behavior after logging so execution is unchanged.
58-76: Remove or utilize dead code.
estimatedCost(line 59) andestimateDrift(line 76) are computed/declared but never used. The comment suggests these are intended for adaptive budget adjustment.♻️ Suggested cleanup if not needed yet
- // 3. Estimate token cost - const estimatedCost = estimateTokenCost(plan); - // 4. Set up infrastructure const pool = new ProviderPool(adapter, options.maxWorkers); const fileLock = new FileLockManager(); @@ ... @@ const results: WorkerResult[] = []; const completedSummaries = new Map<string, string>(); // subtaskId -> summary - - // Track round estimates for adaptive adjustment - let estimateDrift = 1.0; // multiplierIf you intend to implement adaptive adjustment soon, consider adding a TODO comment to clarify the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/orchestrator.ts` around lines 58 - 76, The variables estimatedCost (from estimateTokenCost(plan)) and estimateDrift are declared but unused; either remove them or make their intent explicit: if you don't need adaptive budgeting yet, delete the call to estimateTokenCost(plan) and the estimateDrift declaration, and remove related comments; if you plan to implement adaptive adjustment soon, keep them but add a TODO comment referencing estimateTokenCost and estimateDrift and wire estimateDrift into the budgeting logic (e.g., use estimateDrift to scale estimatedCost when making cost/labor decisions in the orchestration loop) so the symbols are no longer dead code.src/agent/workspace-intel.ts (1)
46-75: Cache hash only coverssrc/directory; projects withlib/orapp/layouts may have stale caches.
computeProfileHashhashes filenames insrc/(line 58), butdetectSrcLayout(line 174-180) recognizeslib/andapp/as valid source directories. Changes to files in these alternative layouts won't invalidate the cached profile.♻️ Suggested fix to include detected source directory
+ // Determine source directory to hash + const srcDir = await detectSrcLayout(root); + const srcDirName = srcDir === 'flat' ? null : srcDir.replace(/\/$/, ''); + - // Sorted list of file names in src/ (names only, not content) - const srcEntries = await listDir(join(root, 'src')); - hash.update(`src-files:${srcEntries.sort().join(',')}`); + // Sorted list of file names in source dir (names only, not content) + if (srcDirName) { + const srcEntries = await listDir(join(root, srcDirName)); + hash.update(`${srcDirName}-files:${srcEntries.sort().join(',')}`); + }Note: This would require moving
detectSrcLayoutbeforecomputeProfileHashor extracting the source-dir detection logic to avoid circular dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/workspace-intel.ts` around lines 46 - 75, computeProfileHash currently only hashes filenames from join(root, 'src') so changes in alternative layouts like lib/ or app/ won't bust the cache; call or reuse the source-directory detection logic used by detectSrcLayout (or extract that detection into a shared helper e.g., getSourceDirs) so computeProfileHash iterates over the actual detected source dirs (e.g., for each dir returned by detectSrcLayout/getSourceDirs, call listDir and include sorted entries in the hash) instead of hardcoding 'src'; move detectSrcLayout earlier or factor out its detection so computeProfileHash can access it without circular imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/file-lock.test.ts`:
- Around line 56-66: The test needs a way to distinguish deadlock from ordinary
contention: update the FileLockResponse type returned by
FileLockManager.requestFile to include a deadlock flag (e.g., deadlock: boolean
or reason: 'deadlock'), modify the deadlock branch in the
FileLockManager.detectDeadlock/handle-request logic to set that flag when a
cycle is found, and update this test to assert resp.deadlock (or resp.reason ===
'deadlock') in addition to resp.granted === false; change the consumer
logic/tests that relied on the old shape to handle the new deadlock signal.
In `@src/agent/orchestrator.test.ts`:
- Around line 51-90: Add a test that verifies parallel execution by creating a
plan with two independent subtasks and calling Orchestrator.execute with
maxWorkers > 1, ensuring both workers start before the first resolves;
specifically, in orchestrator.test.ts reuse Orchestrator/execute but make the
adapter (or plan returned by mockAdapter) contain two subtasks with no
dependencies, stub executeTool (or executeWorker) so the first task awaits a
controllable Promise while the second can start immediately, collect onProgress
events and assert that two 'worker-started' events occur before any
'worker-completed' event (and that both tasks complete), proving executeWorker
is not awaited serially in a for...of loop.
In `@src/agent/orchestrator.ts`:
- Around line 121-144: The retry branch in orchestrator's worker loop calls
lease.release() before continuing, and the finally block also calls
lease.release(), resulting in double release; remove the redundant
lease.release() inside the "failed -> Retry once" branch (the block that calls
executeWorker and assigns retryResult) so that only the finally block releases
the lease, leaving fileLock.releaseAll(subtaskId) in the finally as-is; ensure
executeWorker, pool.addTokenUsage, results.push, completedSummaries.set logic
remains unchanged.
In `@src/agent/planner.ts`:
- Around line 109-119: In extractJSON, TypeScript warns that fenceMatch[1] (and
similarly objMatch[0]) may be undefined; update the checks to guard the capture
groups before using them (e.g., use fenceMatch?.[1] and objMatch?.[0] or
explicit null checks) so you only call .trim() on a defined string; ensure the
function returns fenceMatch[1].trim() and objMatch[0] only after verifying those
indexed captures are non-null.
In `@src/agent/progress.test.ts`:
- Around line 44-49: Replace the hard-coded en-US token string assertion with a
locale-neutral one: compute the expected formatted token count using
(1500).toLocaleString() (or assert with a regex that matches optional locale
separators) and then assert statusBar contains `${expected} tokens`; update the
assertion that currently expects '1,500 tokens' in the test near
reporter.updateTokenCount/onEvent so the check uses the runtime locale-aware
formatted number from ProgressReporter.
In `@src/agent/progress.ts`:
- Around line 26-30: The TypeScript error comes from using match[1] (typed as
string | undefined under noUncheckedIndexedAccess) with parseInt inside the
'plan-ready' case in progress.ts; update the extraction to safely narrow the
captured group before parsing — e.g., destructure the RegExp match into a named
variable like totalStr (const [, totalStr] = match) and then check if totalStr
is truthy before calling parseInt (or use a non-null assertion only after an
explicit guard), then assign this.total = parseInt(totalStr, 10); keep the
existing this.write(...) and formatPlanReady(event.message) usage.
In `@src/agent/sandbox.ts`:
- Around line 48-99: The sandbox currently treats SandboxConfig as advisory:
buildSeatbeltProfile only denies writes (so workspace-only still allows reads)
and buildSandboxArgs's 'firejail' and 'unshare' branches ignore config.network
and config.filesystem. Fix by updating buildSeatbeltProfile to also deny
file-read* when config.filesystem === 'workspace-only' (so reads outside
workspaceRoot are blocked), and change buildSandboxArgs to honor config for the
'firejail' and 'unshare' cases — e.g., add/adjust firejail flags (like
--net=none when config.network === 'blocked' and use --private or proper
whitelist semantics for workspace-only) and add equivalent unshare/namespace and
mount-bind options when config.network or config.filesystem require
restrictions; modify the 'firejail' and 'unshare' case blocks in
buildSandboxArgs accordingly so they consult the SandboxConfig.
- Around line 101-109: The current permissions-only branch (case
'permissions-only') naively splits the input string by the first space using
spaceIdx/exe/rest which collapses multiple args, quotes, env prefixes and
redirections into a single argument; replace that logic by producing a proper
argv: either tokenize the full command string with a shell-aware parser (e.g.,
use a shell-quote/shellwords style tokenizer) and set command to the first token
and args to the remaining tokens, or if you intentionally want full shell
semantics, return a shell invocation like command '/bin/sh' with args ['-lc',
command] so quoting, env prefixes and redirections behave as in the shell.
Ensure you update the permissions-only branch to use the chosen approach instead
of the current spaceIdx/exe/rest split.
In `@src/agent/worker.ts`:
- Around line 89-96: The current catch around executeTool('run_command', {
command: subtask.validationCommand }) swallows validation errors and allows the
worker to return status: 'completed'; change this so a validation failure is
propagated or explicitly marks the subtask as failed: in the try/catch for
subtask.validationCommand, capture the thrown error, log it, and either rethrow
so the outer handler sets a failure status or set the subtask result/status to a
distinct value (e.g., 'validation_failed') before returning; update any code
paths that inspect the worker result to treat that status as non-completed.
Ensure you modify the block that references subtask.validationCommand and
executeTool so validation failures are not ignored.
- Around line 41-75: The loop only checks signal.aborted between rounds so
long-running adapter.chatWithTools and executeTool calls never abort; update the
calls to be cancellable by racing them against the abort signal (or passing the
abort signal into adapter.chatWithTools if it supports one) and handle immediate
cancellation: when signal.aborted reject/resolve the race and return the
cancelled response (same shape as the existing cancelled return with subtaskId,
status 'cancelled', filesChanged, summary, tokensUsed); apply the same pattern
to executeTool invocations so tool calls can be interrupted, and ensure any
resources/cleanup after a cancellation are handled before returning.
In `@src/commands/go.ts`:
- Around line 70-72: The current assignment to maxWorkers uses
parseInt(options.workers, 10) without validation, so invalid input can produce
NaN; update the logic in the go command (referencing maxWorkers,
options.workers, parseInt and config.agent?.maxWorkers) to: default maxWorkers
from config.agent?.maxWorkers ?? 3, then if options.workers is present parse it
and only override the default when Number.isNaN(parsed) is false and parsed > 0
(otherwise keep the default); ensure the parsed value is an integer and positive
before assigning to maxWorkers.
In `@src/commands/run.ts`:
- Around line 47-56: The --no-sandbox flag is parsed into RunOptions /
GoCommandOptions but never applied; update the call sites in runRun() and
runGo() to forward the noSandbox boolean from the command options into the
orchestrator layer, add a sandbox control field to OrchestratorDeps (or
constructor) and/or OrchestratorOptions (execute method) so Orchestrator.accepts
a noSandbox/sandboxDisabled flag, and implement the execute() logic to respect
that flag (disable OS sandbox when true) so that the noSandbox flag actually
alters runtime behavior; reference RunOptions, GoCommandOptions, runRun(),
runGo(), Orchestrator, OrchestratorDeps, OrchestratorOptions, and execute() when
making these changes.
In `@src/intel/conventions.test.ts`:
- Around line 39-41: The test explicitly asserts
analyzeConventions(ROOT).commitConvention against the live repo which flakes;
change the test to use a deterministic fixture or mock the Git-convention probe
instead of reading the real repository. Either: run analyzeConventions() against
a prepared fixture repository with a known commit history (create a small test
fixture and pass its path to analyzeConventions) or stub/mock the internal Git
probe used by analyzeConventions (the function that detects commit style) to
return 'conventional' for the test; update the test case 'detects conventional
commit style' to use the fixture path or the mocked probe so it no longer
depends on current repo history.
- Around line 5-7: The test uses import.meta.dirname when computing ROOT (const
ROOT = resolve(import.meta.dirname, '../..')) which fails on Node 20.x <20.11;
replace that usage with the fileURLToPath(import.meta.url) fallback: import
fileURLToPath from 'node:url' and compute ROOT using
resolve(fileURLToPath(import.meta.url), '../..') (or update package.json engines
to ">=20.11.0" if you prefer to require the newer runtime); update the code
referencing import.meta.dirname (the ROOT constant) accordingly so tests run on
the declared engine range.
In `@src/intel/conventions.ts`:
- Around line 104-204: The packageManager defaults to 'npm' in detectTooling and
isn't updated for non-JS branches; update packageManager inside the pyproject,
Cargo.toml, and go.mod detection blocks to the correct ecosystem (e.g., set
packageManager = 'pip' when setting language = 'python' in the pyproject.toml
branch, packageManager = 'cargo' in the Cargo.toml branch, and packageManager =
'go' in the go.mod branch) so pure Python/Rust/Go workspaces don't report 'npm';
modify the blocks that set language/testCommand (within function detectTooling)
to also assign the new packageManager values.
- Around line 241-295: The failures come from unchecked array indexing like
lines[i] and lines[i+1]; update the loop to guard those accesses by defaulting
to an empty string when an index may be undefined (e.g., replace const line =
lines[i] with const line = lines[i] ?? '' and similarly compute const trimmed =
(line ?? '').trimStart(), const noStrings = (trimmed ?? '').replace(...), and
when reading the next line use const nextLine = lines[i+1] ?? '' then const
nextTrimmed = nextLine.trim(); ensure all uses of line, trimmed, noStrings, and
nextTrimmed handle the empty-string fallback so no methods are called on
undefined.
---
Nitpick comments:
In `@src/agent/config.test.ts`:
- Around line 37-39: Add an upper-bound test for maxWorkers by asserting
JamConfigSchema.parse({ agent: { maxWorkers: 11 } }) throws; update the existing
test suite (the test named 'rejects maxWorkers < 1' or create a new it block) to
include a case that ensures maxWorkers > 10 is rejected via
JamConfigSchema.parse, mirroring the lower-bound test that uses
expect(...).toThrow(); ensure you reference JamConfigSchema.parse and the
agent.maxWorkers field in the new test.
In `@src/agent/file-lock.ts`:
- Around line 11-20: The assignOwnership method currently overwrites ownership
in owners without cleaning up the previous owner's workerFiles set; update
assignOwnership (in file-lock.ts) to first check const prev =
this.owners.get(file.path) and if prev && prev !== workerId then remove
file.path from this.workerFiles.get(prev) (and delete the set if it becomes
empty) before setting the new owner and adding to this.workerFiles for workerId;
alternatively, if reassignment should never occur, add a clear comment stating
the method is only called once per worker and consider throwing or asserting on
conflicting ownership instead.
In `@src/agent/integration.test.ts`:
- Around line 45-57: Remove the module-level mutable counter workerCallCount and
rely on the mock's built-in call tracking for executeWorker instead: delete
workerCallCount and its beforeEach reset, import or access the mocked
executeWorker from the vi.mock setup (symbol: executeWorker) and replace any
assertions or dependency on workerCallCount with checks against (executeWorker
as any).mock.calls.length; ensure the vi.fn() mock implementation remains
unchanged and tests assert mock.calls length where they previously checked
workerCallCount.
In `@src/agent/multimodal.test.ts`:
- Around line 104-122: The tests creating temp files ('loads a PNG file' and
'detects JPEG media type') can leak files if loadImage or assertions throw; wrap
the writeFile/loadImage/assert sequence in a try/finally (or use Vitest
afterEach) to ensure unlink is always called: for each test that calls
writeFile(path, ...) and then await loadImage(path), move the await unlink(path)
into a finally block (or register cleanup in afterEach using the same path),
referencing the test cases by their descriptions or the used symbols writeFile,
unlink, and loadImage so temp files are removed even on failures.
In `@src/agent/orchestrator.ts`:
- Around line 82-83: When a dependency subtaskId isn't found, add a debug/warn
log so missing mappings don't silently fail: in the loop where you resolve
subtask (the lines using plan.subtasks.find and const subtask = ...), emit a log
message (e.g., logger.warn or console.warn) including the missing subtaskId and
identifying plan context (plan.id or other plan fields) before continuing; keep
the existing continue behavior after logging so execution is unchanged.
- Around line 58-76: The variables estimatedCost (from estimateTokenCost(plan))
and estimateDrift are declared but unused; either remove them or make their
intent explicit: if you don't need adaptive budgeting yet, delete the call to
estimateTokenCost(plan) and the estimateDrift declaration, and remove related
comments; if you plan to implement adaptive adjustment soon, keep them but add a
TODO comment referencing estimateTokenCost and estimateDrift and wire
estimateDrift into the budgeting logic (e.g., use estimateDrift to scale
estimatedCost when making cost/labor decisions in the orchestration loop) so the
symbols are no longer dead code.
In `@src/agent/planner.test.ts`:
- Around line 83-86: The test currently only checks that generateTaskPlan throws
for invalid JSON; update it to assert the specific error code or message (e.g.,
AGENT_PLAN_FAILED) so regressions are caught. Replace the generic
rejects.toThrow() with an assertion that inspects the rejection shape from
generateTaskPlan(makeAdapter('not json at all'), 'test', mockProfile), for
example using await expect(...).rejects.toMatchObject({ code:
'AGENT_PLAN_FAILED' }) or await expect(...).rejects.toThrow(/AGENT_PLAN_FAILED/)
depending on whether the thrown value is an Error or an object; reference
generateTaskPlan and makeAdapter when making the change.
In `@src/agent/planner.ts`:
- Around line 53-61: The current call to adapter.chatWithTools sets response to
null when the adapter lacks chatWithTools, leading to a generic "empty response"
error later; update the logic in the planner function that calls
adapter.chatWithTools so that if adapter.chatWithTools is undefined it throws a
clear, descriptive error (e.g., "Adapter does not support chatWithTools")
instead of returning null; ensure the thrown error is raised before any
downstream use of the response variable so callers see the specific failure
originating from adapter.chatWithTools.
In `@src/agent/provider-pool.test.ts`:
- Around line 64-72: The timing-based test in provider-pool.test.ts is flaky;
switch to deterministic fake timers: call vi.useFakeTimers() at test start,
invoke pool.pauseForRateLimit(50), then call pool.acquire() and advance timers
with vi.advanceTimersByTime(50) (or 40ms if you preserve slack) before resolving
the promise, then restore timers with vi.useRealTimers(); reference
ProviderPool.pauseForRateLimit and ProviderPool.acquire to locate the code to
update the test.
In `@src/agent/provider-pool.ts`:
- Around line 61-73: The createLease() implementation currently returns a
release() that can be invoked multiple times, allowing this.active to go
negative; modify createLease() to capture a local boolean (e.g., released =
false) in the closure and make release() a no-op if released is already true,
only decrement this.active, shift the queue, increment this.active and call
next(this.createLease()) on the first invocation, then set released = true;
reference the createLease() method, ProviderLease.release, this.active and
this.queue.shift() when applying the change.
In `@src/agent/types.ts`:
- Line 144: Remove the dead "parent" Map by deleting its declaration and any
assignments to it in the DFS/cycle-detection logic (the Map named "parent" that
is populated during the DFS but never read); ensure cycle reconstruction
continues to use the existing "stack" logic (and update the DFS/detectCycle
function to not reference "parent"), leaving behavior unchanged and eliminating
the unused variable.
In `@src/agent/workspace-intel.test.ts`:
- Around line 23-28: The test currently only compares values from two
buildWorkspaceProfile(ROOT) calls but doesn't assert the cache was used; modify
the test to spy on the underlying detection helpers used by
buildWorkspaceProfile (e.g., detectLanguage and detectTestFramework or whatever
detection functions are imported inside buildWorkspaceProfile) with jest.spyOn
(or your test spy utility), call buildWorkspaceProfile(ROOT) twice, then assert
each detection helper was invoked only once, and finally restore/clear the spies
so other tests are unaffected.
In `@src/agent/workspace-intel.ts`:
- Around line 46-75: computeProfileHash currently only hashes filenames from
join(root, 'src') so changes in alternative layouts like lib/ or app/ won't bust
the cache; call or reuse the source-directory detection logic used by
detectSrcLayout (or extract that detection into a shared helper e.g.,
getSourceDirs) so computeProfileHash iterates over the actual detected source
dirs (e.g., for each dir returned by detectSrcLayout/getSourceDirs, call listDir
and include sorted entries in the hash) instead of hardcoding 'src'; move
detectSrcLayout earlier or factor out its detection so computeProfileHash can
access it without circular imports.
In `@src/commands/go.ts`:
- Around line 181-184: The rl.on('close') handler currently calls
process.exit(0) which can kill pending async cleanup; change it to perform a
graceful shutdown by invoking the async cleanup functions (e.g., call await
mcpManager.shutdown() and any orchestrator cleanup) and then exit when they
complete, or instead move termination logic to a SIGINT/SIGTERM handler that
runs the shutdown sequence (call mcpManager.shutdown(), orchestrator.shutdown()
or similar) and only then calls process.exit(0); locate the rl.on('close')
handler and the mcpManager/orchestrator shutdown methods to implement this
ordering.
- Around line 96-100: The async readline handler (rl.on('line', async (line) =>
{ ... })) can swallow errors and may call rl.close() before shutdown truly
completes; wrap the handler body in a try/catch (or try/finally) so errors are
propagated/handled, and ensure you await mcpManager.shutdown() before calling
rl.close() (e.g., call await mcpManager.shutdown() inside the try/finally and
only invoke rl.close() after that await completes) — adjust the logic around the
rl.on('line' ...) callback and the mcpManager.shutdown()/rl.close() calls to
guarantee shutdown completes and errors are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62df3bd0-3a00-4ada-ad7f-0a7ed79e9478
⛔ Files ignored due to path filters (2)
docs/plans/2026-03-20-jam-agent-engine.mdis excluded by!docs/**docs/specs/2026-03-20-jam-agent-engine-design.mdis excluded by!docs/**
📒 Files selected for processing (35)
src/agent/config.test.tssrc/agent/errors.test.tssrc/agent/file-lock.test.tssrc/agent/file-lock.tssrc/agent/index.tssrc/agent/integration.test.tssrc/agent/multimodal.test.tssrc/agent/multimodal.tssrc/agent/orchestrator.test.tssrc/agent/orchestrator.tssrc/agent/permissions.test.tssrc/agent/permissions.tssrc/agent/planner.test.tssrc/agent/planner.tssrc/agent/progress.test.tssrc/agent/progress.tssrc/agent/provider-pool.test.tssrc/agent/provider-pool.tssrc/agent/sandbox.test.tssrc/agent/sandbox.tssrc/agent/types.test.tssrc/agent/types.tssrc/agent/worker.test.tssrc/agent/worker.tssrc/agent/workspace-intel.test.tssrc/agent/workspace-intel.tssrc/commands/go.tssrc/commands/run.tssrc/config/schema.tssrc/index.tssrc/intel/conventions.test.tssrc/intel/conventions.tssrc/intel/index.tssrc/providers/base.tssrc/utils/errors.ts
Comment on lines
+56
to
+66
| it('detects deadlock (cycle in wait graph)', () => { | ||
| const mgr = new FileLockManager(); | ||
| mgr.assignOwnership('w1', [{ path: 'src/a.ts', mode: 'modify' }]); | ||
| mgr.assignOwnership('w2', [{ path: 'src/b.ts', mode: 'modify' }]); | ||
| // w1 waits for w2's file | ||
| mgr.requestFile({ workerId: 'w1', path: 'src/b.ts', reason: 'need b' }); | ||
| // Now w2 wants w1's file — this would create a deadlock | ||
| const resp = mgr.requestFile({ workerId: 'w2', path: 'src/a.ts', reason: 'need a' }); | ||
| expect(resp.granted).toBe(false); | ||
| // detectDeadlock should return true internally | ||
| }); |
There was a problem hiding this comment.
Expose deadlocks separately from ordinary lock contention.
In src/agent/file-lock.ts, the deadlock branch returns the same { granted: false, waitForWorker } payload as a normal wait, so the caller cannot react differently and this test cannot prove a cycle was detected. Add a deadlock-specific signal to FileLockResponse and assert it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/file-lock.test.ts` around lines 56 - 66, The test needs a way to
distinguish deadlock from ordinary contention: update the FileLockResponse type
returned by FileLockManager.requestFile to include a deadlock flag (e.g.,
deadlock: boolean or reason: 'deadlock'), modify the deadlock branch in the
FileLockManager.detectDeadlock/handle-request logic to set that flag when a
cycle is found, and update this test to assert resp.deadlock (or resp.reason ===
'deadlock') in addition to resp.granted === false; change the consumer
logic/tests that relied on the old shape to handle the new deadlock signal.
Comment on lines
+41
to
+59
| export function flattenForProvider( | ||
| messages: AgentMessage[], | ||
| supportsVision: boolean | ||
| ): Message[] { | ||
| return messages.map(msg => { | ||
| if (typeof msg.content === 'string') { | ||
| return { role: msg.role, content: msg.content }; | ||
| } | ||
|
|
||
| const textContent = getTextContent(msg); | ||
| const hadImages = hasImages(msg); | ||
|
|
||
| let content = textContent; | ||
| if (!supportsVision && hadImages) { | ||
| content = textContent + '\n[Image provided but this model does not support vision]'; | ||
| } | ||
|
|
||
| return { role: msg.role, content }; | ||
| }); |
There was a problem hiding this comment.
supportsVision still drops the actual image payload.
This helper always reduces ContentPart[] to plain text. Since the returned Message[] can only carry string content, any caller that uses this path for vision-capable providers will silently lose the base64 image data and --image becomes a no-op.
Comment on lines
+51
to
+90
| it('executes a single-subtask plan', async () => { | ||
| const orch = new Orchestrator({ | ||
| adapter: mockAdapter, | ||
| workspaceRoot: '/workspace', | ||
| toolSchemas: [], | ||
| executeTool: vi.fn(), | ||
| }); | ||
|
|
||
| const result = await orch.execute('do something', { | ||
| mode: 'auto', | ||
| maxWorkers: 1, | ||
| }); | ||
|
|
||
| expect(result.plan.goal).toBe('Test goal'); | ||
| expect(result.results).toHaveLength(1); | ||
| expect(result.results[0].status).toBe('completed'); | ||
| expect(result.filesChanged).toContain('src/a.ts'); | ||
| expect(result.totalTokens.totalTokens).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('calls onProgress callbacks', async () => { | ||
| const events: string[] = []; | ||
| const orch = new Orchestrator({ | ||
| adapter: mockAdapter, | ||
| workspaceRoot: '/workspace', | ||
| toolSchemas: [], | ||
| executeTool: vi.fn(), | ||
| }); | ||
|
|
||
| await orch.execute('do something', { | ||
| mode: 'auto', | ||
| maxWorkers: 1, | ||
| onProgress: (e) => events.push(e.type), | ||
| }); | ||
|
|
||
| expect(events).toContain('plan-ready'); | ||
| expect(events).toContain('worker-started'); | ||
| expect(events).toContain('worker-completed'); | ||
| expect(events).toContain('all-done'); | ||
| }); |
There was a problem hiding this comment.
Add a parallel-subtask case; maxWorkers is currently untested.
src/agent/orchestrator.ts awaits each executeWorker() inside a plain for ... of, so independent subtasks never overlap and maxWorkers has no observable effect. These single-subtask tests won't catch that. Add a two-subtask/no-dependency case that proves both workers start before the first one resolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/orchestrator.test.ts` around lines 51 - 90, Add a test that
verifies parallel execution by creating a plan with two independent subtasks and
calling Orchestrator.execute with maxWorkers > 1, ensuring both workers start
before the first resolves; specifically, in orchestrator.test.ts reuse
Orchestrator/execute but make the adapter (or plan returned by mockAdapter)
contain two subtasks with no dependencies, stub executeTool (or executeWorker)
so the first task awaits a controllable Promise while the second can start
immediately, collect onProgress events and assert that two 'worker-started'
events occur before any 'worker-completed' event (and that both tasks complete),
proving executeWorker is not awaited serially in a for...of loop.
Comment on lines
+121
to
+144
| } else if (result.status === 'failed') { | ||
| // Retry once | ||
| options.onProgress?.({ type: 'worker-failed', subtaskId, message: `Failed: ${result.error}. Retrying...` }); | ||
| const retryResult = await executeWorker(subtask, context, signal, { | ||
| lease, | ||
| workspaceRoot, | ||
| workspaceProfile: profile, | ||
| toolSchemas, | ||
| executeTool, | ||
| }); | ||
| pool.addTokenUsage(retryResult.tokensUsed); | ||
| results.push(retryResult); | ||
| if (retryResult.status === 'completed') { | ||
| completedSummaries.set(subtaskId, retryResult.summary); | ||
| } | ||
| lease.release(); | ||
| continue; | ||
| } | ||
|
|
||
| results.push(result); | ||
| } finally { | ||
| lease.release(); | ||
| fileLock.releaseAll(subtaskId); | ||
| } |
There was a problem hiding this comment.
Double release of provider lease in retry path.
When a worker fails and is retried, the code calls lease.release() at line 136 before continue. However, the finally block at line 141-144 will still execute (JavaScript finally always runs, even with continue), causing lease.release() to be called twice.
This could cause issues with the ProviderPool semaphore count if release() isn't idempotent.
🐛 Proposed fix: remove the redundant release in the retry branch
pool.addTokenUsage(retryResult.tokensUsed);
results.push(retryResult);
if (retryResult.status === 'completed') {
completedSummaries.set(subtaskId, retryResult.summary);
}
- lease.release();
continue;
}
results.push(result);
} finally {
lease.release();
fileLock.releaseAll(subtaskId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/orchestrator.ts` around lines 121 - 144, The retry branch in
orchestrator's worker loop calls lease.release() before continuing, and the
finally block also calls lease.release(), resulting in double release; remove
the redundant lease.release() inside the "failed -> Retry once" branch (the
block that calls executeWorker and assigns retryResult) so that only the finally
block releases the lease, leaving fileLock.releaseAll(subtaskId) in the finally
as-is; ensure executeWorker, pool.addTokenUsage, results.push,
completedSummaries.set logic remains unchanged.
Comment on lines
+5
to
+46
| const SAFE_PATTERNS: RegExp[] = [ | ||
| // File inspection | ||
| /^ls(\s|$)/, | ||
| /^cat(\s|$)/, | ||
| /^head(\s|$)/, | ||
| /^tail(\s|$)/, | ||
| /^wc(\s|$)/, | ||
| /^echo(\s|$)/, | ||
| /^pwd(\s|$)/, | ||
| /^whoami(\s|$)/, | ||
| /^date(\s|$)/, | ||
| /^which(\s|$)/, | ||
| /^env(\s|$)/, | ||
| /^find(\s|$)/, | ||
| /^grep(\s|$)/, | ||
| /^rg(\s|$)/, | ||
| /^fd(\s|$)/, | ||
| // Git read-only | ||
| /^git\s+status(\s|$)/, | ||
| /^git\s+diff(\s|$)/, | ||
| /^git\s+log(\s|$)/, | ||
| /^git\s+show(\s|$)/, | ||
| /^git\s+branch(\s|$)/, | ||
| /^git\s+tag(\s|$)/, | ||
| /^git\s+remote(\s|$)/, | ||
| /^git\s+rev-parse(\s|$)/, | ||
| // Test runners | ||
| /^npm\s+test(\s|$)/, | ||
| /^npx\s+vitest(\s|$)/, | ||
| /^npx\s+jest(\s|$)/, | ||
| /^npx\s+tsc(\s|$)/, | ||
| /^npx\s+eslint(\s|$)/, | ||
| /^npx\s+prettier(\s|$)/, | ||
| // Runtimes (read/run only) | ||
| /^node(\s|$)/, | ||
| /^deno(\s|$)/, | ||
| /^bun\s+test(\s|$)/, | ||
| /^bun\s+run(\s|$)/, | ||
| /^cargo\s+test(\s|$)/, | ||
| /^go\s+test(\s|$)/, | ||
| /^python\s+-m\s+pytest(\s|$)/, | ||
| ]; |
There was a problem hiding this comment.
The safe tier is broad enough to bypass the approval boundary.
SAFE_PATTERNS currently marks arbitrary runtimes and test runners as safe, and the match is only on the command prefix. That makes inputs like node -e "..." safe, and even cat foo && git push still inherits a safe classification from the first token. In auto mode, that undermines the whole dangerous-tier gate.
Also applies to: 99-104
Comment on lines
+47
to
+56
| /** Fully autonomous mode (implies --yes). */ | ||
| auto?: boolean; | ||
| /** Max parallel workers for orchestrator. */ | ||
| workers?: string; | ||
| /** Attach image file paths for multimodal input. */ | ||
| image?: string[]; | ||
| /** Disable OS sandbox. */ | ||
| noSandbox?: boolean; | ||
| /** Read prompt from a file. */ | ||
| file?: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All noSandbox / --no-sandbox references:"
rg -n '\bnoSandbox\b|--no-sandbox'Repository: sunilp/jam-cli
Length of output: 1152
🏁 Script executed:
# Check how noSandbox is used in run.ts
echo "=== run.ts references to noSandbox ==="
sed -n '47,200p' src/commands/run.ts | cat -n
echo -e "\n=== go.ts references to noSandbox ==="
sed -n '1,100p' src/commands/go.ts | cat -n
echo -e "\n=== Search for sandbox-related code in the codebase ==="
rg -i 'sandbox' --type ts -A 2 -B 2 | head -100Repository: sunilp/jam-cli
Length of output: 16509
🏁 Script executed:
# Check Orchestrator constructor and execute method
echo "=== Orchestrator class definition ==="
rg -n 'class Orchestrator|constructor|execute\(' src/agent/orchestrator.ts | head -20
echo -e "\n=== Orchestrator execute method signature ==="
sed -n '1,150p' src/agent/orchestrator.ts | grep -A 10 'execute('
echo -e "\n=== Check if Orchestrator uses any sandbox config ==="
rg -n 'sandbox|Sandbox' src/agent/orchestrator.tsRepository: sunilp/jam-cli
Length of output: 932
🏁 Script executed:
# Check OrchestratorDeps and OrchestratorOptions type definitions
echo "=== OrchestratorDeps type definition ==="
rg -n 'interface OrchestratorDeps|type OrchestratorDeps' src/agent/orchestrator.ts -A 6
echo -e "\n=== OrchestratorOptions type definition ==="
rg -n 'interface OrchestratorOptions|type OrchestratorOptions' src/agent/orchestrator.ts -A 6
# Also check if they're defined elsewhere
echo -e "\n=== Search for OrchestratorDeps across codebase ==="
rg -n 'interface OrchestratorDeps|type OrchestratorDeps'
echo -e "\n=== Search for OrchestratorOptions across codebase ==="
rg -n 'interface OrchestratorOptions|type OrchestratorOptions'Repository: sunilp/jam-cli
Length of output: 826
--no-sandbox is currently a no-op and never reaches the execution layer.
The flag is registered in the CLI and passed through RunOptions and GoCommandOptions, but it is never referenced in the runRun() or runGo() implementations. The Orchestrator class does not accept a sandbox parameter in either its constructor (OrchestratorDeps) or its execute() method (OrchestratorOptions). Users can pass --no-sandbox without any behavior change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/run.ts` around lines 47 - 56, The --no-sandbox flag is parsed
into RunOptions / GoCommandOptions but never applied; update the call sites in
runRun() and runGo() to forward the noSandbox boolean from the command options
into the orchestrator layer, add a sandbox control field to OrchestratorDeps (or
constructor) and/or OrchestratorOptions (execute method) so Orchestrator.accepts
a noSandbox/sandboxDisabled flag, and implement the execute() logic to respect
that flag (disable OS sandbox when true) so that the noSandbox flag actually
alters runtime behavior; reference RunOptions, GoCommandOptions, runRun(),
runGo(), Orchestrator, OrchestratorDeps, OrchestratorOptions, and execute() when
making these changes.
Comment on lines
+5
to
+7
| import { resolve } from 'node:path'; | ||
|
|
||
| const ROOT = resolve(import.meta.dirname, '../..'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json engines/volta =="
jq '{engines: .engines, volta: .volta, packageManager: .packageManager}' package.json
echo
echo "== Node version files =="
for f in .nvmrc .node-version; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f"
echo
fi
done
echo "== CI / docs references =="
rg -n -C2 'node-version|setup-node|engines' .github/workflows README.md package.jsonRepository: sunilp/jam-cli
Length of output: 1719
🌐 Web query:
When was import.meta.dirname added to Node.js
💡 Result:
import.meta.dirname was added to Node.js in v20.11.0 (LTS) (released January 9, 2024) and in the Current line as v21.2.0. [1], [2]
Update engines constraint or use fallback pattern for import.meta.dirname.
import.meta.dirname was added in Node.js v20.11.0 (January 2024). The current engines: {"node": ">=20"} allows Node 20.0–20.10.x, where this API does not exist. The test will fail with import.meta.dirname is not defined on those versions.
Either update engines to ">=20.11.0" or use the fileURLToPath(import.meta.url) pattern for compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.test.ts` around lines 5 - 7, The test uses
import.meta.dirname when computing ROOT (const ROOT =
resolve(import.meta.dirname, '../..')) which fails on Node 20.x <20.11; replace
that usage with the fileURLToPath(import.meta.url) fallback: import
fileURLToPath from 'node:url' and compute ROOT using
resolve(fileURLToPath(import.meta.url), '../..') (or update package.json engines
to ">=20.11.0" if you prefer to require the newer runtime); update the code
referencing import.meta.dirname (the ROOT constant) accordingly so tests run on
the declared engine range.
Comment on lines
+39
to
+41
| it('detects conventional commit style', async () => { | ||
| const profile = await analyzeConventions(ROOT); | ||
| expect(profile.commitConvention).toBe('conventional'); |
There was a problem hiding this comment.
Don't assert Git conventions from the live repository history.
analyzeConventions() derives this field from the current repo's Git state, so this expectation will flap with shallow clones, forks, or just a few non-conventional commits. Use a fixture repo or mock the Git-convention probe for this case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.test.ts` around lines 39 - 41, The test explicitly
asserts analyzeConventions(ROOT).commitConvention against the live repo which
flakes; change the test to use a deterministic fixture or mock the
Git-convention probe instead of reading the real repository. Either: run
analyzeConventions() against a prepared fixture repository with a known commit
history (create a small test fixture and pass its path to analyzeConventions) or
stub/mock the internal Git probe used by analyzeConventions (the function that
detects commit style) to return 'conventional' for the test; update the test
case 'detects conventional commit style' to use the fixture path or the mocked
probe so it no longer depends on current repo history.
Comment on lines
+104
to
+204
| async function detectTooling(root: string): Promise<ToolingInfo> { | ||
| let language = 'javascript'; | ||
| let packageManager = 'npm'; | ||
| let testFramework = 'unknown'; | ||
| let testCommand = 'npm test'; | ||
| let linter: string | undefined; | ||
| let formatter: string | undefined; | ||
| let typeChecker: string | undefined; | ||
| let buildTool: string | undefined; | ||
|
|
||
| // package.json — JS/TS | ||
| const pkgText = await readTextFile(join(root, 'package.json')); | ||
| if (pkgText) { | ||
| let pkg: Record<string, unknown> = {}; | ||
| try { pkg = JSON.parse(pkgText); } catch { /* ignore */ } | ||
|
|
||
| const devDeps = (pkg.devDependencies ?? {}) as Record<string, string>; | ||
| const deps = (pkg.dependencies ?? {}) as Record<string, string>; | ||
| const allDeps = { ...deps, ...devDeps }; | ||
| const scripts = (pkg.scripts ?? {}) as Record<string, string>; | ||
|
|
||
| // Language | ||
| if ('typescript' in allDeps) { | ||
| language = 'typescript'; | ||
| typeChecker = 'typescript'; | ||
| } | ||
|
|
||
| // Package manager — check lockfiles | ||
| const [yarnLock, pnpmLock, bunLock] = await Promise.all([ | ||
| readTextFile(join(root, 'yarn.lock')), | ||
| readTextFile(join(root, 'pnpm-lock.yaml')), | ||
| readTextFile(join(root, 'bun.lockb')).then(v => v).catch(() => null), | ||
| ]); | ||
| const packageManagerField = (pkg.packageManager as string | undefined) ?? ''; | ||
| if (packageManagerField.startsWith('yarn') || yarnLock !== null) { | ||
| packageManager = 'yarn'; | ||
| } else if (packageManagerField.startsWith('pnpm') || pnpmLock !== null) { | ||
| packageManager = 'pnpm'; | ||
| } else if (packageManagerField.startsWith('bun') || bunLock !== null) { | ||
| packageManager = 'bun'; | ||
| } | ||
|
|
||
| // Test framework | ||
| if ('vitest' in allDeps) { | ||
| testFramework = 'vitest'; | ||
| } else if ('jest' in allDeps || '@jest/core' in allDeps) { | ||
| testFramework = 'jest'; | ||
| } else if ('mocha' in allDeps) { | ||
| testFramework = 'mocha'; | ||
| } | ||
|
|
||
| // Test command — look at scripts | ||
| const testScript = scripts['test'] ?? ''; | ||
| if (testScript) { | ||
| testCommand = `${packageManager === 'npm' ? 'npm' : packageManager} run test`; | ||
| if (testScript.includes('vitest')) testCommand = 'npx vitest run'; | ||
| else if (testScript.includes('jest')) testCommand = 'npx jest'; | ||
| } | ||
|
|
||
| // Linter | ||
| if ('eslint' in allDeps) linter = 'eslint'; | ||
| else if ('biome' in allDeps || '@biomejs/biome' in allDeps) linter = 'biome'; | ||
| else if ('oxlint' in allDeps) linter = 'oxlint'; | ||
|
|
||
| // Formatter | ||
| if ('prettier' in allDeps) formatter = 'prettier'; | ||
| else if ('biome' in allDeps || '@biomejs/biome' in allDeps) formatter = formatter ?? 'biome'; | ||
|
|
||
| // Build tool | ||
| if ('tsup' in allDeps) buildTool = 'tsup'; | ||
| else if ('vite' in allDeps) buildTool = 'vite'; | ||
| else if ('esbuild' in allDeps) buildTool = 'esbuild'; | ||
| else if ('webpack' in allDeps) buildTool = 'webpack'; | ||
| else if ('rollup' in allDeps) buildTool = 'rollup'; | ||
| } | ||
|
|
||
| // pyproject.toml — Python | ||
| const pyproject = await readTextFile(join(root, 'pyproject.toml')); | ||
| if (pyproject && !pkgText) { | ||
| language = 'python'; | ||
| if (pyproject.includes('pytest')) testFramework = 'pytest'; | ||
| testCommand = 'pytest'; | ||
| } | ||
|
|
||
| // Cargo.toml — Rust | ||
| const cargoToml = await readTextFile(join(root, 'Cargo.toml')); | ||
| if (cargoToml && !pkgText && !pyproject) { | ||
| language = 'rust'; | ||
| testFramework = 'cargo-test'; | ||
| testCommand = 'cargo test'; | ||
| } | ||
|
|
||
| // go.mod — Go | ||
| const goMod = await readTextFile(join(root, 'go.mod')); | ||
| if (goMod && !pkgText && !pyproject && !cargoToml) { | ||
| language = 'go'; | ||
| testFramework = 'go-test'; | ||
| testCommand = 'go test ./...'; | ||
| } | ||
|
|
||
| return { language, packageManager, testFramework, testCommand, linter, formatter, typeChecker, buildTool }; |
There was a problem hiding this comment.
Pure Python/Rust/Go workspaces are still labeled as using npm.
packageManager starts as 'npm' and never gets corrected in the pyproject.toml, Cargo.toml, or go.mod branches. That means the generated WorkspaceProfile can tell downstream prompt builders to use the wrong ecosystem tooling for non-JS repos.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.ts` around lines 104 - 204, The packageManager defaults
to 'npm' in detectTooling and isn't updated for non-JS branches; update
packageManager inside the pyproject, Cargo.toml, and go.mod detection blocks to
the correct ecosystem (e.g., set packageManager = 'pip' when setting language =
'python' in the pyproject.toml branch, packageManager = 'cargo' in the
Cargo.toml branch, and packageManager = 'go' in the go.mod branch) so pure
Python/Rust/Go workspaces don't report 'npm'; modify the blocks that set
language/testCommand (within function detectTooling) to also assign the new
packageManager values.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/intel/conventions.ts (1)
180-202:⚠️ Potential issue | 🟠 MajorPure Python/Rust/Go workspaces will incorrectly report
packageManager: 'npm'.The
packageManagervariable defaults to'npm'and is only updated within thepackage.jsonbranch. Non-JS projects need appropriate values.🛠️ Suggested fix
// pyproject.toml — Python const pyproject = await readTextFile(join(root, 'pyproject.toml')); if (pyproject && !pkgText) { language = 'python'; + packageManager = 'pip'; if (pyproject.includes('pytest')) testFramework = 'pytest'; testCommand = 'pytest'; } // Cargo.toml — Rust const cargoToml = await readTextFile(join(root, 'Cargo.toml')); if (cargoToml && !pkgText && !pyproject) { language = 'rust'; + packageManager = 'cargo'; testFramework = 'cargo-test'; testCommand = 'cargo test'; } // go.mod — Go const goMod = await readTextFile(join(root, 'go.mod')); if (goMod && !pkgText && !pyproject && !cargoToml) { language = 'go'; + packageManager = 'go'; testFramework = 'go-test'; testCommand = 'go test ./...'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/intel/conventions.ts` around lines 180 - 202, The code currently leaves packageManager defaulting to 'npm' for non-JS projects; update the pyproject, Cargo.toml, and go.mod detection blocks to set packageManager appropriately (e.g., set packageManager = 'pip' or 'poetry' for pyproject, packageManager = 'cargo' for Cargo.toml, and packageManager = 'go' for go.mod) alongside setting language, testFramework, and testCommand so pure Python/Rust/Go workspaces don't report npm; adjust the blocks that reference pyproject, cargoToml, and goMod to assign packageManager when those files are found.
🧹 Nitpick comments (1)
src/intel/conventions.ts (1)
132-136: Redundant error handling forbun.lockb.
readTextFilealready returnsnullon error. The additional.then(v => v).catch(() => null)chain is unnecessary.♻️ Suggested simplification
const [yarnLock, pnpmLock, bunLock] = await Promise.all([ readTextFile(join(root, 'yarn.lock')), readTextFile(join(root, 'pnpm-lock.yaml')), - readTextFile(join(root, 'bun.lockb')).then(v => v).catch(() => null), + readTextFile(join(root, 'bun.lockb')), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/intel/conventions.ts` around lines 132 - 136, The Promise.all call redundantly wraps the bun.lockb read with .then(...).catch(...) even though readTextFile already returns null on error; in the array where yarnLock, pnpmLock, bunLock are declared, replace the third element with a direct call to readTextFile(join(root, 'bun.lockb')) so bunLock is resolved the same way as the others (refer to readTextFile, join, root, and the yarnLock/pnpmLock/bunLock destructuring to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/planner.ts`:
- Around line 77-87: The mapping over parsed.subtasks and s.files can throw if
those fields aren't arrays; before mapping, validate and coerce types: ensure
parsed.subtasks is Array (fallback to [] if not) and for each subtask ensure
s.files is Array (fallback to []), then perform the map. Update the code around
the subtasks assignment (variable subtasks and the inner files mapping) to check
Array.isArray(parsed.subtasks) and Array.isArray(s.files) (or use safe coercion)
so malformed model output won't cause a raw TypeError and will preserve JamError
handling.
- Around line 54-64: The planner currently assumes adapter.chatWithTools exists
and throws if missing; update the logic in planner.ts to detect whether the
provider supports tool-calling (use adapter.chatWithTools presence and
ProviderInfo.supportsTools) and, when absent, fall back to a plain text
completion call (e.g., adapter.streamCompletion or the appropriate completion
method) with the same messages/systemPrompt/model/temperature/maxTokens
parameters; only throw the JamError if both tool-capable and plain-completion
paths fail, and ensure references to adapter.chatWithTools, ProviderAdapter,
ProviderInfo.supportsTools, and adapter.streamCompletion are used to locate and
implement the fallback.
- Around line 89-92: The depGraph population blindly trusts parsed.dependencies
entries for each subtask (the loop setting depGraph for st.id), so validate and
normalize before inserting: for each st.id look up parsed.dependencies?.[st.id],
if it's not an array use an empty array; if it is an array, coerce elements to
strings, deduplicate, and filter out any IDs that are not present in the current
subtasks set (build a Set of subtasks.map(s=>s.id)); finally set
depGraph.set(st.id, validatedArray) so only valid, normalized dependency IDs
enter the graph and invalid entries are discarded (optionally log or warn when
invalid types/unknown IDs are encountered).
In `@src/agent/progress.ts`:
- Around line 37-45: The cases for 'worker-completed' and 'worker-failed'
decrement this.activeWorkers unconditionally which can make it go negative;
modify the handlers in the switch (the 'worker-completed' and 'worker-failed'
branches) to only decrement this.activeWorkers when it's > 0, and only increment
this.completed when a decrement actually happened, then proceed to call
write/formatWorkerPrefix/formatStatusBar as before; update references to
activeWorkers, completed, formatWorkerPrefix, formatStatusBar, and write in
those branches.
- Around line 19-24: The onEvent method currently unconditionally appends every
ProgressEvent to this.events causing unbounded memory growth; change onEvent in
the ProgressReporter class so it only pushes events when this.mode === 'json'
(i.e., when getJsonResults() will be used), leaving the existing early returns
for 'quiet' and 'json' behavior intact; update any callers/tests assuming events
are always retained and ensure getJsonResults() still returns the collected list
when mode === 'json'.
In `@src/intel/conventions.ts`:
- Around line 488-498: The test naming is hardcoded to TypeScript patterns via
the naming/hasSpec logic that sets testNaming to '*.spec.ts' or '*.test.ts';
update the logic in the block that checks isDir(join(root, 'src', dir)) (and the
earlier branch that sets testNaming) to detect the repository language (or file
extensions present in root/src) and choose language-appropriate patterns (e.g.,
'test_*.py' for Python, '*_test.go' for Go, Rust conventions like 'tests/' or
inline tests). Keep the existing hasSpec heuristic but extend it to look for
language-specific spec/test extensions and map to testNaming strings accordingly
so testNaming reflects the detected language instead of always using TypeScript
patterns.
- Around line 464-465: The conditional that increments relativeCount currently
matches a literal "..." and should only detect standard relative import
prefixes; update the test used when incrementing relativeCount (the same
conditional that follows the aliasCount check) so it does not include the "..."
pattern and instead only matches "./" or "../" import prefixes (i.e., restrict
the regex to the two valid relative forms).
---
Duplicate comments:
In `@src/intel/conventions.ts`:
- Around line 180-202: The code currently leaves packageManager defaulting to
'npm' for non-JS projects; update the pyproject, Cargo.toml, and go.mod
detection blocks to set packageManager appropriately (e.g., set packageManager =
'pip' or 'poetry' for pyproject, packageManager = 'cargo' for Cargo.toml, and
packageManager = 'go' for go.mod) alongside setting language, testFramework, and
testCommand so pure Python/Rust/Go workspaces don't report npm; adjust the
blocks that reference pyproject, cargoToml, and goMod to assign packageManager
when those files are found.
---
Nitpick comments:
In `@src/intel/conventions.ts`:
- Around line 132-136: The Promise.all call redundantly wraps the bun.lockb read
with .then(...).catch(...) even though readTextFile already returns null on
error; in the array where yarnLock, pnpmLock, bunLock are declared, replace the
third element with a direct call to readTextFile(join(root, 'bun.lockb')) so
bunLock is resolved the same way as the others (refer to readTextFile, join,
root, and the yarnLock/pnpmLock/bunLock destructuring to locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f65ceae-a1b2-4fdf-b66a-de0a7c668ae3
📒 Files selected for processing (4)
src/agent/planner.tssrc/agent/progress.tssrc/config/defaults.tssrc/intel/conventions.ts
✅ Files skipped from review due to trivial changes (1)
- src/config/defaults.ts
Comment on lines
+54
to
+64
| const response = adapter.chatWithTools | ||
| ? await adapter.chatWithTools(messages, [], { | ||
| model: options?.model, | ||
| temperature: options?.temperature ?? 0.3, | ||
| maxTokens: options?.maxTokens ?? 2000, | ||
| systemPrompt, | ||
| }) | ||
| : null; | ||
|
|
||
| if (!response?.content) { | ||
| throw new JamError('AGENT_PLAN_FAILED: Planner received empty response', 'AGENT_PLAN_FAILED'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProviderAdapter contract =="
fd base.ts src/providers --exec sed -n '1,260p' {}
echo
echo "== Planner adapter call sites =="
rg -nP --type=ts -C3 '\badapter\.(chatWithTools|chat)\s*\(' src/agent/planner.ts
echo
echo "== Provider implementations exposing chat/chatWithTools =="
rg -nP --type=ts -C3 'class\s+\w+|chatWithTools\s*\(|chat\s*\(' src/providersRepository: sunilp/jam-cli
Length of output: 12907
🏁 Script executed:
sed -n '50,75p' src/agent/planner.tsRepository: sunilp/jam-cli
Length of output: 792
Add fallback for providers without tool-calling support.
The chatWithTools method is optional in the ProviderAdapter interface, but the planner (lines 54–61) assumes it exists and throws an error if unavailable. While all current provider implementations include chatWithTools, this design is fragile: adding a future provider without tool-calling support will cause planning to fail unconditionally instead of gracefully degrading (e.g., falling back to streamCompletion for plain text generation or introducing a fallback call mechanism).
Consider using the optional supportsTools flag in ProviderInfo to guide planner behavior when tools are unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/planner.ts` around lines 54 - 64, The planner currently assumes
adapter.chatWithTools exists and throws if missing; update the logic in
planner.ts to detect whether the provider supports tool-calling (use
adapter.chatWithTools presence and ProviderInfo.supportsTools) and, when absent,
fall back to a plain text completion call (e.g., adapter.streamCompletion or the
appropriate completion method) with the same
messages/systemPrompt/model/temperature/maxTokens parameters; only throw the
JamError if both tool-capable and plain-completion paths fail, and ensure
references to adapter.chatWithTools, ProviderAdapter,
ProviderInfo.supportsTools, and adapter.streamCompletion are used to locate and
implement the fallback.
Comment on lines
+89
to
+92
| const depGraph = new Map<string, string[]>(); | ||
| for (const st of subtasks) { | ||
| depGraph.set(st.id, parsed.dependencies?.[st.id] ?? []); | ||
| } |
There was a problem hiding this comment.
Validate and normalize dependency entries before building the graph.
Line 91 trusts parsed.dependencies values as-is. Non-array values or unknown IDs can silently enter the graph and cause incorrect scheduling behavior downstream.
Proposed fix
const depGraph = new Map<string, string[]>();
+ const validIds = new Set(subtasks.map((s) => s.id));
for (const st of subtasks) {
- depGraph.set(st.id, parsed.dependencies?.[st.id] ?? []);
+ const rawDeps = parsed.dependencies?.[st.id];
+ if (rawDeps !== undefined && !Array.isArray(rawDeps)) {
+ throw new JamError(`Invalid dependency list for subtask ${st.id}`, 'AGENT_PLAN_FAILED');
+ }
+ const deps = (rawDeps ?? []).map(String);
+ const unknown = deps.find((d) => !validIds.has(d));
+ if (unknown) {
+ throw new JamError(`Unknown dependency "${unknown}" referenced by subtask ${st.id}`, 'AGENT_PLAN_FAILED');
+ }
+ depGraph.set(st.id, deps);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/planner.ts` around lines 89 - 92, The depGraph population blindly
trusts parsed.dependencies entries for each subtask (the loop setting depGraph
for st.id), so validate and normalize before inserting: for each st.id look up
parsed.dependencies?.[st.id], if it's not an array use an empty array; if it is
an array, coerce elements to strings, deduplicate, and filter out any IDs that
are not present in the current subtasks set (build a Set of
subtasks.map(s=>s.id)); finally set depGraph.set(st.id, validatedArray) so only
valid, normalized dependency IDs enter the graph and invalid entries are
discarded (optionally log or warn when invalid types/unknown IDs are
encountered).
Comment on lines
+19
to
+24
| onEvent(event: ProgressEvent): void { | ||
| this.events.push(event); | ||
|
|
||
| if (this.mode === 'quiet') return; | ||
| if (this.mode === 'json') return; // collected, rendered at end | ||
|
|
There was a problem hiding this comment.
Avoid unbounded event retention outside JSON mode.
Line 20 stores every event even when getJsonResults() is never used, which can grow memory indefinitely in long interactive/default runs.
💡 Suggested fix
onEvent(event: ProgressEvent): void {
- this.events.push(event);
+ if (this.mode === 'json') this.events.push(event);
if (this.mode === 'quiet') return;
if (this.mode === 'json') return; // collected, rendered at end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onEvent(event: ProgressEvent): void { | |
| this.events.push(event); | |
| if (this.mode === 'quiet') return; | |
| if (this.mode === 'json') return; // collected, rendered at end | |
| onEvent(event: ProgressEvent): void { | |
| if (this.mode === 'json') this.events.push(event); | |
| if (this.mode === 'quiet') return; | |
| if (this.mode === 'json') return; // collected, rendered at end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/progress.ts` around lines 19 - 24, The onEvent method currently
unconditionally appends every ProgressEvent to this.events causing unbounded
memory growth; change onEvent in the ProgressReporter class so it only pushes
events when this.mode === 'json' (i.e., when getJsonResults() will be used),
leaving the existing early returns for 'quiet' and 'json' behavior intact;
update any callers/tests assuming events are always retained and ensure
getJsonResults() still returns the collected list when mode === 'json'.
Comment on lines
+37
to
+45
| case 'worker-completed': | ||
| this.activeWorkers--; | ||
| this.completed++; | ||
| this.write(`${this.formatWorkerPrefix(event.subtaskId)} ${event.message}\n`); | ||
| this.write(this.formatStatusBar() + '\n'); | ||
| break; | ||
| case 'worker-failed': | ||
| this.activeWorkers--; | ||
| this.write(`${this.formatWorkerPrefix(event.subtaskId)} ${event.message}\n`); |
There was a problem hiding this comment.
Guard activeWorkers from going negative.
Lines 38 and 44 decrement blindly; if completion/failure events are duplicated or reordered, status can show negative active workers.
💡 Suggested fix
case 'worker-completed':
- this.activeWorkers--;
+ this.activeWorkers = Math.max(0, this.activeWorkers - 1);
this.completed++;
this.write(`${this.formatWorkerPrefix(event.subtaskId)} ${event.message}\n`);
this.write(this.formatStatusBar() + '\n');
break;
case 'worker-failed':
- this.activeWorkers--;
+ this.activeWorkers = Math.max(0, this.activeWorkers - 1);
this.write(`${this.formatWorkerPrefix(event.subtaskId)} ${event.message}\n`);
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/progress.ts` around lines 37 - 45, The cases for 'worker-completed'
and 'worker-failed' decrement this.activeWorkers unconditionally which can make
it go negative; modify the handlers in the switch (the 'worker-completed' and
'worker-failed' branches) to only decrement this.activeWorkers when it's > 0,
and only increment this.completed when a decrement actually happened, then
proceed to call write/formatWorkerPrefix/formatStatusBar as before; update
references to activeWorkers, completed, formatWorkerPrefix, formatStatusBar, and
write in those branches.
Comment on lines
+464
to
+465
| if (/from ['"](@\/|~\/|#)/.test(line)) aliasCount++; | ||
| else if (/from ['"](\.\.\.|\.\/|\.\.\/)/.test(line)) relativeCount++; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any import statements using "..." pattern in common JS/TS repos
rg -n "from ['\"]\.\.\./" --type ts --type js || echo "No '...' import patterns found"
# Check what import patterns exist in this codebase
rg -n "^import.*from" --type ts | head -30Repository: sunilp/jam-cli
Length of output: 2506
🏁 Script executed:
sed -n '453,470p' src/intel/conventions.tsRepository: sunilp/jam-cli
Length of output: 665
Remove the \.\.\. pattern from the relative import regex.
Line 465 contains /from ['"](\.\.\.|\.\/|\.\.\/)/.test(line) where the \.\.\. pattern matches literal "..." which is not a valid JavaScript/TypeScript import path syntax. Standard relative imports use only ./ and ../. The pattern should be /from ['"](\./|\..\/)/.test(line) to correctly detect relative imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.ts` around lines 464 - 465, The conditional that
increments relativeCount currently matches a literal "..." and should only
detect standard relative import prefixes; update the test used when incrementing
relativeCount (the same conditional that follows the aliasCount check) so it
does not include the "..." pattern and instead only matches "./" or "../" import
prefixes (i.e., restrict the regex to the two valid relative forms).
Comment on lines
+488
to
+498
| const naming = hasSpec ? '*.spec.ts' : '*.test.ts'; | ||
| return { | ||
| testLocation: dir, | ||
| testNaming: naming, | ||
| testStyle: 'dedicated-directory', | ||
| }; | ||
| } | ||
| if (await isDir(join(root, 'src', dir))) { | ||
| const entries = await listDir(join(root, 'src', dir)); | ||
| const hasSpec = entries.some(e => e.includes('.spec.')); | ||
| const naming = hasSpec ? '*.spec.ts' : '*.test.ts'; |
There was a problem hiding this comment.
Test naming pattern is hardcoded to TypeScript conventions.
The default test naming patterns (*.spec.ts, *.test.ts) are TypeScript-specific. For Python (test_*.py), Go (*_test.go), or Rust (inline or tests/) projects, these patterns would be incorrect. Consider aligning with the detected language.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.ts` around lines 488 - 498, The test naming is
hardcoded to TypeScript patterns via the naming/hasSpec logic that sets
testNaming to '*.spec.ts' or '*.test.ts'; update the logic in the block that
checks isDir(join(root, 'src', dir)) (and the earlier branch that sets
testNaming) to detect the repository language (or file extensions present in
root/src) and choose language-appropriate patterns (e.g., 'test_*.py' for
Python, '*_test.go' for Go, Rust conventions like 'tests/' or inline tests).
Keep the existing hasSpec heuristic but extend it to look for language-specific
spec/test extensions and map to testNaming strings accordingly so testNaming
reflects the detected language instead of always using TypeScript patterns.
Replace all `any` types with proper typed alternatives across agent implementation and test files. Use `as unknown as Type` pattern for mock objects, `unknown` with type narrowing for JSON.parse results, and ToolDefinition[] for tool schema parameters. Fix unused imports, prefer-const violations, and unnecessary type assertions.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (8)
src/agent/multimodal.ts (1)
41-59:⚠️ Potential issue | 🟠 MajorVision-capable path still drops image payloads.
Even when
supportsVisionistrue, this function emits text-onlyMessage.content, so image data is lost before provider dispatch. That makes multimodal input effectively a no-op on the vision path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/multimodal.ts` around lines 41 - 59, flattenForProvider currently strips non-string image payloads by always converting Message.content to text; update it to preserve the original non-string content when supportsVision is true so vision-capable providers receive the image payloads. Specifically, inside flattenForProvider (and where you call getTextContent and hasImages), return { role: msg.role, content: msg.content } for non-string msg.content when supportsVision is true; only convert to text or append the "[Image provided...]" warning when supportsVision is false and hadImages. Ensure you reference the existing functions flattenForProvider, getTextContent, hasImages and the messages/supportsVision args when making the change.src/agent/orchestrator.ts (1)
123-142:⚠️ Potential issue | 🔴 CriticalRemove the extra
lease.release()in the retry branch.The
continueat Line 136 still executes thefinallyblock, so this path releases the same lease twice. IfProviderPoolrelease is not idempotent, its concurrency accounting will drift upward after retries.Suggested fix
pool.addTokenUsage(retryResult.tokensUsed); results.push(retryResult); if (retryResult.status === 'completed') { completedSummaries.set(subtaskId, retryResult.summary); } - lease.release(); continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/orchestrator.ts` around lines 123 - 142, In the retry branch inside orchestrator.ts (the block that handles retryResult from executeWorker), remove the explicit lease.release() call that occurs just before the continue so the lease is only released once in the finally block; locate the code that processes retryResult (references: executeWorker, retryResult, pool.addTokenUsage, completedSummaries, lease.release()) and delete the duplicate lease.release() to avoid double-releasing the ProviderPool lease.src/agent/planner.ts (2)
96-100:⚠️ Potential issue | 🟠 MajorNormalize dependency lists before inserting them into the graph.
rawDeps[st.id]is trusted asstring[], but malformed plans can put non-arrays or unknown IDs here.topologicalSort()will then traverse phantom nodes and let dependants run as if their prerequisites existed.Suggested fix
- const rawDeps = (obj.dependencies ?? {}) as Record<string, string[]>; + const rawDeps = (obj.dependencies && typeof obj.dependencies === 'object' + ? obj.dependencies + : {}) as Record<string, unknown>; + const validIds = new Set(subtasks.map((s) => s.id)); const depGraph = new Map<string, string[]>(); for (const st of subtasks) { - depGraph.set(st.id, rawDeps[st.id] ?? []); + const deps = Array.isArray(rawDeps[st.id]) + ? [...new Set((rawDeps[st.id] as unknown[]).map(String).filter((id) => validIds.has(id)))] + : []; + depGraph.set(st.id, deps); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/planner.ts` around lines 96 - 100, The dependency graph population trusts rawDeps[st.id] as a string[] which can be malformed; before inserting into depGraph in the loop over subtasks ensure the value is normalized to an array of existing subtask IDs: read const deps = rawDeps[st.id], if it's not an array set deps = [], filter deps to include only strings that match ids present in subtasks (e.g. via a Set of subtasks' ids), and then depGraph.set(st.id, deps); this prevents phantom nodes from being introduced and keeps topologicalSort() operating on valid nodes only.
53-64:⚠️ Potential issue | 🟠 MajorPlanner still hard-requires
chatWithToolseven though it asks for no tools.This call passes an empty tool list, so a provider that only implements
streamCompletion()can still generate the plan. Failing straight toAGENT_PLAN_FAILEDhere makes the planner unnecessarily incompatible with any non-tool provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/planner.ts` around lines 53 - 64, The planner currently assumes adapter.chatWithTools must exist and throws AGENT_PLAN_FAILED if not, which breaks providers that only implement streamCompletion; update the logic around adapter.chatWithTools and the response handling in the planner so that if adapter.chatWithTools is missing you call adapter.streamCompletion (or the equivalent single-turn completion method) with the same messages/systemPrompt and options (model, temperature, maxTokens) to obtain response.content; only throw JamError('AGENT_PLAN_FAILED: Planner received empty response', 'AGENT_PLAN_FAILED') if neither path returns a non-empty content. Ensure you reference the existing symbols adapter.chatWithTools, adapter.streamCompletion, messages, systemPrompt, options, and the JamError throw site when making the change.src/agent/worker.ts (2)
89-96:⚠️ Potential issue | 🟠 MajorA failing validation step should not return
completed.Swallowing the
run_commanderror here makes broken output indistinguishable from validated output and can unblock dependent subtasks on an invalid state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/worker.ts` around lines 89 - 96, The validation step is currently swallowing errors in the try/catch around executeTool('run_command', { command: subtask.validationCommand }) which causes failed validation to still mark the worker as completed; change this so a failing validation bubbles up: remove the empty catch (or in the catch rethrow the caught error) so the thrown error from executeTool propagates, or explicitly set the subtask/worker state to failed before rethrowing (reference: subtask.validationCommand and executeTool('run_command', ...)) so dependent subtasks cannot proceed on an invalid result.
41-55:⚠️ Potential issue | 🟠 MajorCancellation still only applies between awaited operations.
signal.abortedis checked at the top of the loop, butmemory.compact(),adapter.chatWithTools(),executeTool('run_command', ...), and per-toolexecuteTool()calls are all awaited directly. A hung provider or tool call will ignore/stopuntil it returns on its own.Also applies to: 75-78, 90-93, 110-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/worker.ts` around lines 41 - 55, The loop only checks signal.aborted before awaiting long operations, so a hung provider/tool will ignore /stop; make each awaited call interruptible by either passing the AbortSignal into APIs that support it or wrapping awaits with an abort-aware helper (e.g., a withAbort/timeout race) and re-checking signal after the race; specifically update calls to memory.compact(messages), adapter.chatWithTools(...), executeTool('run_command', ...), and per-tool executeTool(...) so they accept/observe the signal or are wrapped in an abortable Promise, and ensure you return the cancelled status immediately when the wrapper detects the signal (also add the same pattern around the other instances noted in the comment).src/intel/conventions.ts (2)
105-106:⚠️ Potential issue | 🟠 MajorSet
packageManagerin the non-JS branches.When
pyproject.toml,Cargo.toml, orgo.modwins detection,packageManagernever moves off the initial'npm'. Pure Python/Rust/Go workspaces will therefore surface the wrong ecosystem tool to downstream prompt/command generation.Also applies to: 180-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/intel/conventions.ts` around lines 105 - 106, The bug is that packageManager is initialized to 'npm' and never updated in the non-JS detection branches; update the logic that sets the language variable so it also sets packageManager appropriately when Python, Rust, or Go are detected (e.g., set packageManager to 'pip' or 'pipenv'/'poetry' for pyproject.toml, 'cargo' for Cargo.toml, and 'go' or 'modules' for go.mod) — ensure the same fix is applied to the duplicate detection block later (around the code referencing language and packageManager in the 180-202 area) so pure Python/Rust/Go workspaces don't retain 'npm'.
486-488:⚠️ Potential issue | 🟡 MinorMake test pattern detection language-aware and recursive.
This still hardcodes
*.spec.ts/*.test.ts, and the co-located branch only inspects the immediate children ofsrc/. Even this repo’ssrc/intel/conventions.test.tsis missed there, and Python/Go layouts liketests/test_api.pyorsrc/pkg/foo_test.goget the wrongtestNaming.Also applies to: 495-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/intel/conventions.ts` around lines 486 - 488, The current test-pattern detection (variables entries, hasSpec, naming using listDir(join(root, dir))) only checks immediate children and hardcodes TypeScript patterns; update the logic in the function in conventions.ts that computes testNaming (the block using listDir, join, hasSpec, naming and the related code at 495-521) to perform a recursive search under the repository root for test files and make it language-aware: scan file paths recursively (not just immediate entries), detect language by extension (e.g., .ts/.tsx -> look for *.spec.* vs *.test.*, .py -> tests/ or test_*.py, .go -> *_test.go) and set naming per detected language; stop when any matching pattern is found and prefer language-specific rules so the naming variable reflects the correct test convention for the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/multimodal.ts`:
- Around line 95-98: Replace the incorrect error code used when throwing
JamError for unsupported image formats: in the throw site that constructs new
JamError(`Unsupported image format: ${ext}...`, 'INPUT_FILE_NOT_FOUND'), change
the second argument to 'INPUT_MISSING' (or, if you prefer to introduce a clearer
classification, add and use a new code constant like 'INPUT_INVALID_FORMAT'
across the error enum and tests). Update the JamError usages and any error
enum/type that defines allowed codes (and adjust callers/tests if necessary) so
the format-validation path no longer reports 'INPUT_FILE_NOT_FOUND' but the
chosen, semantically correct code (JamError construction in multimodal.ts: the
Unsupported image format throw).
In `@src/agent/orchestrator.ts`:
- Around line 84-92: The code currently only uses
plan.dependencyGraph.get(subtaskId) to gather context (variables: deps,
priorSummaries, priorFiles) but does not enforce that each dependency finished
successfully; add a gating check before building context/start of the subtask
that verifies every dependency's result exists in results (or
completedSummaries) and has status === 'completed' (inspect results entries'
subtaskId/status and completedSummaries map), and if any dependency has status
'failed' or 'cancelled' then mark this subtask as skipped/cancelled (or fail it)
and do not use its filesChanged or summary in priorFiles/priorSummaries; apply
the same gating logic to the other block mentioned (around lines 117-139) so
dependants never run when prerequisites are not completed.
- Around line 77-144: The dispatch loop currently awaits each subtask serially
so options.maxWorkers is unused; change the logic in orchestrator.ts to collect
all unblocked subtasks (using order, plan.dependencyGraph, plan.subtasks,
completedSummaries) into a queue and run up to options.maxWorkers executeWorker
tasks concurrently instead of a plain for...of; use the ProviderPool
(pool.acquire/release) per task, preserve the existing retry logic, token
accounting (pool.addTokenUsage), results/ completedSummaries updates, and
fileLock.releaseAll(subtaskId) in each task's finally block, and ensure
results.push happens once each subtask completes (or after retry) and onProgress
events are emitted the same way.
In `@src/agent/planner.ts`:
- Around line 26-38: The schema block in src/agent/planner.ts (the prompt string
that begins "Respond with ONLY valid JSON matching this schema:") contains
non-JSON elements (| unions and // comments) which contradict the instruction to
return valid JSON; replace that schema block with strictly valid JSON (no
comments, no TypeScript-style unions) and move any explanatory notes (e.g.,
allowed values or optional field descriptions) outside the JSON block as plain
text or separate comments so the example is parseable and the guidance remains
clear.
- Around line 76-94: Ensure the parser fails fast for malformed planner
payloads: check that `parsed` is a non-null object before casting to `obj`,
validate `obj.subtasks` exists and is a non-empty array (`rawSubtasks`) and for
each subtask `s` ensure required fields (`s.id`, `s.description`, and `s.files`
with each file having `f.path` and valid `f.mode`) are present and of expected
types; if any of these validations fail, throw a `JamError` with a clear message
instead of continuing to build the `Subtask[]` (avoid coercing missing values to
`"undefined"`), otherwise proceed to map `rawSubtasks` into `subtasks` (keep the
existing logic for `estimatedRounds` and optional `validationCommand`).
- Line 91: The value assigned to estimatedRounds (currently set via
Number(s.estimatedRounds) || 10) can be non-finite or unsafe and must be
clamped; replace that expression with logic that parses the model output to a
finite integer (use Number.parseInt/Number and Number.isFinite), apply
Math.floor, enforce a minimum of 1 and a sensible maximum (e.g., 100) and fall
back to 10 if parsing fails, so estimatedRounds is always a finite, bounded
integer before it flows into maxRounds in worker.ts.
In `@src/agent/worker.ts`:
- Around line 53-78: executeWorker currently lets exceptions from
memory.compact() and adapter.chatWithTools() bubble up, which causes the
orchestrator to skip its retry logic; wrap the calls to memory.compact(...) and
adapter.chatWithTools(...) in try/catch blocks inside executeWorker and on any
thrown error return the same shaped failure object the orchestrator expects
(include subtaskId, status: 'failed', filesChanged, summary or error message,
tokensUsed/totalUsage) instead of throwing; specifically guard the
memory.compact(messages) invocation and the adapter.chatWithTools(messages,
toolSchemas, {...}) call so failures result in a consistent returned failure
result rather than an exception.
- Around line 28-35: The WorkingMemory is instantiated with undefined for
systemPrompt so its shouldCompact() call can't account for the full prompt; pass
the computed systemPrompt from buildWorkerSystemPrompt into the WorkingMemory
constructor (i.e., construct new WorkingMemory(adapter, systemPrompt, ...)) so
WorkingMemory.can measure prompt size correctly and compact before provider
turns; ensure references to WorkingMemory.shouldCompact() and any internal size
accounting use that passed systemPrompt.
In `@src/intel/conventions.ts`:
- Around line 291-297: The heuristic only increments vote.trailingCommaCount
when a trailing comma is detected, so trailingCommas becomes true after one
positive sample; modify the logic in the trailing-comma detection (the block
that checks stripped.endsWith(',') and nextTrimmed.startsWith('}',']',')') and
the analogous block around lines 391-392) to also increment
vote.noTrailingCommaCount when the eligible closing-line pattern is present but
the current line does NOT end with a comma; this ensures you track both
positives and negatives before computing the trailingCommas decision.
- Around line 44-45: The naming-vote detection currently only matches JS
keywords (const|let|var|function) so non-JS functions/vars (Python def, Rust fn,
Go func) produce zero votes and cause maxNaming===totals.snakeCase to default to
snake_case; update the declaration-detection regexes (the ones that match
const|let|var|function) to also recognize language-specific keywords "def",
"fn", and "func" and any language-appropriate variable patterns, and adjust the
final selection logic (the code using maxNaming, totals, and totals.snakeCase)
to only treat totals.snakeCase as the winner when maxNaming > 0 (i.e., require
at least one vote before defaulting), so naming detection works for non-JS files
and we don’t force snake_case when no votes exist.
---
Duplicate comments:
In `@src/agent/multimodal.ts`:
- Around line 41-59: flattenForProvider currently strips non-string image
payloads by always converting Message.content to text; update it to preserve the
original non-string content when supportsVision is true so vision-capable
providers receive the image payloads. Specifically, inside flattenForProvider
(and where you call getTextContent and hasImages), return { role: msg.role,
content: msg.content } for non-string msg.content when supportsVision is true;
only convert to text or append the "[Image provided...]" warning when
supportsVision is false and hadImages. Ensure you reference the existing
functions flattenForProvider, getTextContent, hasImages and the
messages/supportsVision args when making the change.
In `@src/agent/orchestrator.ts`:
- Around line 123-142: In the retry branch inside orchestrator.ts (the block
that handles retryResult from executeWorker), remove the explicit
lease.release() call that occurs just before the continue so the lease is only
released once in the finally block; locate the code that processes retryResult
(references: executeWorker, retryResult, pool.addTokenUsage, completedSummaries,
lease.release()) and delete the duplicate lease.release() to avoid
double-releasing the ProviderPool lease.
In `@src/agent/planner.ts`:
- Around line 96-100: The dependency graph population trusts rawDeps[st.id] as a
string[] which can be malformed; before inserting into depGraph in the loop over
subtasks ensure the value is normalized to an array of existing subtask IDs:
read const deps = rawDeps[st.id], if it's not an array set deps = [], filter
deps to include only strings that match ids present in subtasks (e.g. via a Set
of subtasks' ids), and then depGraph.set(st.id, deps); this prevents phantom
nodes from being introduced and keeps topologicalSort() operating on valid nodes
only.
- Around line 53-64: The planner currently assumes adapter.chatWithTools must
exist and throws AGENT_PLAN_FAILED if not, which breaks providers that only
implement streamCompletion; update the logic around adapter.chatWithTools and
the response handling in the planner so that if adapter.chatWithTools is missing
you call adapter.streamCompletion (or the equivalent single-turn completion
method) with the same messages/systemPrompt and options (model, temperature,
maxTokens) to obtain response.content; only throw JamError('AGENT_PLAN_FAILED:
Planner received empty response', 'AGENT_PLAN_FAILED') if neither path returns a
non-empty content. Ensure you reference the existing symbols
adapter.chatWithTools, adapter.streamCompletion, messages, systemPrompt,
options, and the JamError throw site when making the change.
In `@src/agent/worker.ts`:
- Around line 89-96: The validation step is currently swallowing errors in the
try/catch around executeTool('run_command', { command: subtask.validationCommand
}) which causes failed validation to still mark the worker as completed; change
this so a failing validation bubbles up: remove the empty catch (or in the catch
rethrow the caught error) so the thrown error from executeTool propagates, or
explicitly set the subtask/worker state to failed before rethrowing (reference:
subtask.validationCommand and executeTool('run_command', ...)) so dependent
subtasks cannot proceed on an invalid result.
- Around line 41-55: The loop only checks signal.aborted before awaiting long
operations, so a hung provider/tool will ignore /stop; make each awaited call
interruptible by either passing the AbortSignal into APIs that support it or
wrapping awaits with an abort-aware helper (e.g., a withAbort/timeout race) and
re-checking signal after the race; specifically update calls to
memory.compact(messages), adapter.chatWithTools(...), executeTool('run_command',
...), and per-tool executeTool(...) so they accept/observe the signal or are
wrapped in an abortable Promise, and ensure you return the cancelled status
immediately when the wrapper detects the signal (also add the same pattern
around the other instances noted in the comment).
In `@src/intel/conventions.ts`:
- Around line 105-106: The bug is that packageManager is initialized to 'npm'
and never updated in the non-JS detection branches; update the logic that sets
the language variable so it also sets packageManager appropriately when Python,
Rust, or Go are detected (e.g., set packageManager to 'pip' or 'pipenv'/'poetry'
for pyproject.toml, 'cargo' for Cargo.toml, and 'go' or 'modules' for go.mod) —
ensure the same fix is applied to the duplicate detection block later (around
the code referencing language and packageManager in the 180-202 area) so pure
Python/Rust/Go workspaces don't retain 'npm'.
- Around line 486-488: The current test-pattern detection (variables entries,
hasSpec, naming using listDir(join(root, dir))) only checks immediate children
and hardcodes TypeScript patterns; update the logic in the function in
conventions.ts that computes testNaming (the block using listDir, join, hasSpec,
naming and the related code at 495-521) to perform a recursive search under the
repository root for test files and make it language-aware: scan file paths
recursively (not just immediate entries), detect language by extension (e.g.,
.ts/.tsx -> look for *.spec.* vs *.test.*, .py -> tests/ or test_*.py, .go ->
*_test.go) and set naming per detected language; stop when any matching pattern
is found and prefer language-specific rules so the naming variable reflects the
correct test convention for the repo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32f2a47b-43f9-4d55-9923-c8684033da32
📒 Files selected for processing (12)
src/agent/integration.test.tssrc/agent/multimodal.tssrc/agent/orchestrator.test.tssrc/agent/orchestrator.tssrc/agent/planner.test.tssrc/agent/planner.tssrc/agent/progress.test.tssrc/agent/provider-pool.test.tssrc/agent/worker.test.tssrc/agent/worker.tssrc/agent/workspace-intel.tssrc/intel/conventions.ts
✅ Files skipped from review due to trivial changes (2)
- src/agent/progress.test.ts
- src/agent/worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/agent/provider-pool.test.ts
- src/agent/planner.test.ts
- src/agent/workspace-intel.ts
Comment on lines
+95
to
+98
| throw new JamError( | ||
| `Unsupported image format: ${ext}. Supported formats: .png, .jpg, .jpeg, .gif, .webp`, | ||
| 'INPUT_FILE_NOT_FOUND' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify available error codes and current usage sites.
rg -n "type ErrorCode|enum ErrorCode|INPUT_FILE_" src
rg -n "INPUT_FILE_NOT_FOUND" src
# Inspect this file's throw sites in context.
rg -n -C3 "Unsupported image format|Image file not found" src/agent/multimodal.tsRepository: sunilp/jam-cli
Length of output: 2522
🏁 Script executed:
cat -n src/utils/errors.ts | head -50Repository: sunilp/jam-cli
Length of output: 2255
🏁 Script executed:
sed -n '32,100p' src/utils/errors.tsRepository: sunilp/jam-cli
Length of output: 3765
Use a more appropriate error code for unsupported image format.
Line 97 incorrectly uses INPUT_FILE_NOT_FOUND for format validation. This code is semantically wrong since the file exists—the problem is the format is unsupported. The hint for INPUT_FILE_NOT_FOUND says "The specified file does not exist", which will mislead users. Replace with INPUT_MISSING (the least-bad alternative from available codes) or propose adding a dedicated INPUT_INVALID_FORMAT code if format validation should have its own distinct error classification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/multimodal.ts` around lines 95 - 98, Replace the incorrect error
code used when throwing JamError for unsupported image formats: in the throw
site that constructs new JamError(`Unsupported image format: ${ext}...`,
'INPUT_FILE_NOT_FOUND'), change the second argument to 'INPUT_MISSING' (or, if
you prefer to introduce a clearer classification, add and use a new code
constant like 'INPUT_INVALID_FORMAT' across the error enum and tests). Update
the JamError usages and any error enum/type that defines allowed codes (and
adjust callers/tests if necessary) so the format-validation path no longer
reports 'INPUT_FILE_NOT_FOUND' but the chosen, semantically correct code
(JamError construction in multimodal.ts: the Unsupported image format throw).
Comment on lines
+77
to
+144
| // 6. Dispatch workers respecting dependencies | ||
| for (const subtaskId of order) { | ||
| if (signal.aborted) break; | ||
|
|
||
| const subtask = plan.subtasks.find(s => s.id === subtaskId); | ||
| if (!subtask) continue; | ||
|
|
||
| // Build context from completed dependencies | ||
| const deps = plan.dependencyGraph.get(subtaskId) ?? []; | ||
| const priorSummaries = deps | ||
| .map(d => completedSummaries.get(d)) | ||
| .filter(Boolean) | ||
| .join('\n'); | ||
| const priorFiles = results | ||
| .filter(r => deps.includes(r.subtaskId)) | ||
| .flatMap(r => r.filesChanged.map(f => f.path)); | ||
|
|
||
| const context = { | ||
| priorSummary: priorSummaries, | ||
| filesAvailable: priorFiles, | ||
| planReminder: `You are on subtask ${subtask.id} of ${plan.subtasks.length}: ${subtask.description}`, | ||
| }; | ||
|
|
||
| options.onProgress?.({ type: 'worker-started', subtaskId, message: `Starting: ${subtask.description}` }); | ||
|
|
||
| // Acquire provider lease | ||
| const lease = await pool.acquire(); | ||
|
|
||
| try { | ||
| const result = await executeWorker(subtask, context, signal, { | ||
| lease, | ||
| workspaceRoot, | ||
| workspaceProfile: profile, | ||
| toolSchemas, | ||
| executeTool, | ||
| }); | ||
|
|
||
| // Track token usage | ||
| pool.addTokenUsage(result.tokensUsed); | ||
|
|
||
| if (result.status === 'completed') { | ||
| completedSummaries.set(subtaskId, result.summary); | ||
| options.onProgress?.({ type: 'worker-completed', subtaskId, message: `Done: ${subtask.description}` }); | ||
| } else if (result.status === 'failed') { | ||
| // Retry once | ||
| options.onProgress?.({ type: 'worker-failed', subtaskId, message: `Failed: ${result.error}. Retrying...` }); | ||
| const retryResult = await executeWorker(subtask, context, signal, { | ||
| lease, | ||
| workspaceRoot, | ||
| workspaceProfile: profile, | ||
| toolSchemas, | ||
| executeTool, | ||
| }); | ||
| pool.addTokenUsage(retryResult.tokensUsed); | ||
| results.push(retryResult); | ||
| if (retryResult.status === 'completed') { | ||
| completedSummaries.set(subtaskId, retryResult.summary); | ||
| } | ||
| lease.release(); | ||
| continue; | ||
| } | ||
|
|
||
| results.push(result); | ||
| } finally { | ||
| lease.release(); | ||
| fileLock.releaseAll(subtaskId); | ||
| } | ||
| } |
There was a problem hiding this comment.
maxWorkers is a no-op in the current dispatch loop.
Every subtask is awaited inside a plain for...of, so independent branches never overlap and the ProviderPool semaphore is never exercised. To get actual parallel workers, enqueue all currently unblocked subtasks and run up to options.maxWorkers concurrently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/orchestrator.ts` around lines 77 - 144, The dispatch loop currently
awaits each subtask serially so options.maxWorkers is unused; change the logic
in orchestrator.ts to collect all unblocked subtasks (using order,
plan.dependencyGraph, plan.subtasks, completedSummaries) into a queue and run up
to options.maxWorkers executeWorker tasks concurrently instead of a plain
for...of; use the ProviderPool (pool.acquire/release) per task, preserve the
existing retry logic, token accounting (pool.addTokenUsage), results/
completedSummaries updates, and fileLock.releaseAll(subtaskId) in each task's
finally block, and ensure results.push happens once each subtask completes (or
after retry) and onProgress events are emitted the same way.
Comment on lines
+84
to
+92
| // Build context from completed dependencies | ||
| const deps = plan.dependencyGraph.get(subtaskId) ?? []; | ||
| const priorSummaries = deps | ||
| .map(d => completedSummaries.get(d)) | ||
| .filter(Boolean) | ||
| .join('\n'); | ||
| const priorFiles = results | ||
| .filter(r => deps.includes(r.subtaskId)) | ||
| .flatMap(r => r.filesChanged.map(f => f.path)); |
There was a problem hiding this comment.
Dependants still run after a prerequisite fails or is cancelled.
The dependency list is only used to build context; there is no gate that requires each prerequisite to finish with status === 'completed' before starting the next subtask. That means a failed worker can still feed partial filesChanged into a dependant and violate the DAG semantics.
Also applies to: 117-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/orchestrator.ts` around lines 84 - 92, The code currently only uses
plan.dependencyGraph.get(subtaskId) to gather context (variables: deps,
priorSummaries, priorFiles) but does not enforce that each dependency finished
successfully; add a gating check before building context/start of the subtask
that verifies every dependency's result exists in results (or
completedSummaries) and has status === 'completed' (inspect results entries'
subtaskId/status and completedSummaries map), and if any dependency has status
'failed' or 'cancelled' then mark this subtask as skipped/cancelled (or fail it)
and do not use its filesChanged or summary in priorFiles/priorSummaries; apply
the same gating logic to the other block mentioned (around lines 117-139) so
dependants never run when prerequisites are not completed.
Comment on lines
+26
to
+38
| Respond with ONLY valid JSON matching this schema: | ||
| { | ||
| "goal": "one sentence description", | ||
| "subtasks": [ | ||
| { | ||
| "id": "1", | ||
| "description": "what to do", | ||
| "files": [{ "path": "src/file.ts", "mode": "create" | "modify" | "read-only" }], | ||
| "estimatedRounds": 10, | ||
| "validationCommand": "npm test -- --grep pattern" // optional | ||
| } | ||
| ], | ||
| "dependencies": { "2": ["1"], "3": ["2"] } // subtaskId -> [prerequisite IDs] |
There was a problem hiding this comment.
The example “JSON” in the prompt is not valid JSON.
The model is told to return “ONLY valid JSON”, but the schema block still contains | unions and // comments. That contradiction makes parse failures much more likely; keep the example strict JSON and move the explanatory notes outside the block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/planner.ts` around lines 26 - 38, The schema block in
src/agent/planner.ts (the prompt string that begins "Respond with ONLY valid
JSON matching this schema:") contains non-JSON elements (| unions and //
comments) which contradict the instruction to return valid JSON; replace that
schema block with strictly valid JSON (no comments, no TypeScript-style unions)
and move any explanatory notes (e.g., allowed values or optional field
descriptions) outside the JSON block as plain text or separate comments so the
example is parseable and the guidance remains clear.
Comment on lines
+76
to
+94
| // Type guard for the parsed object | ||
| const obj = parsed as Record<string, unknown>; | ||
|
|
||
| // Build TaskPlan | ||
| const rawSubtasks = (Array.isArray(obj.subtasks) ? obj.subtasks : []) as Array<Record<string, unknown>>; | ||
| const subtasks: Subtask[] = rawSubtasks.map((s) => { | ||
| const rawFiles = (Array.isArray(s.files) ? s.files : []) as Array<Record<string, unknown>>; | ||
| return { | ||
| id: String(s.id), | ||
| description: String(s.description ?? ''), | ||
| files: rawFiles.map((f) => ({ | ||
| path: String(f.path), | ||
| mode: f.mode === 'create' || f.mode === 'modify' || f.mode === 'read-only' | ||
| ? f.mode : 'read-only' as const, | ||
| })), | ||
| estimatedRounds: Number(s.estimatedRounds) || 10, | ||
| validationCommand: s.validationCommand ? String(s.validationCommand) : undefined, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Fail closed on malformed or empty planner payloads.
JSON.parse('null') still reaches the property access at Line 80 and throws a raw runtime error, while {} or missing id / path fields quietly become an empty plan or literal "undefined" identifiers. Please reject non-object payloads, empty subtasks, and missing required fields with JamError before constructing Subtask[].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/planner.ts` around lines 76 - 94, Ensure the parser fails fast for
malformed planner payloads: check that `parsed` is a non-null object before
casting to `obj`, validate `obj.subtasks` exists and is a non-empty array
(`rawSubtasks`) and for each subtask `s` ensure required fields (`s.id`,
`s.description`, and `s.files` with each file having `f.path` and valid
`f.mode`) are present and of expected types; if any of these validations fail,
throw a `JamError` with a clear message instead of continuing to build the
`Subtask[]` (avoid coercing missing values to `"undefined"`), otherwise proceed
to map `rawSubtasks` into `subtasks` (keep the existing logic for
`estimatedRounds` and optional `validationCommand`).
| mode: f.mode === 'create' || f.mode === 'modify' || f.mode === 'read-only' | ||
| ? f.mode : 'read-only' as const, | ||
| })), | ||
| estimatedRounds: Number(s.estimatedRounds) || 10, |
There was a problem hiding this comment.
estimatedRounds needs a finite clamp.
Number(s.estimatedRounds) || 10 accepts -1, 1e309, and other unsafe values from model output. Infinity then flows into maxRounds in src/agent/worker.ts, which turns the worker loop into an effectively unbounded run.
Suggested fix
- estimatedRounds: Number(s.estimatedRounds) || 10,
+ estimatedRounds: (() => {
+ const rounds = Number(s.estimatedRounds);
+ if (!Number.isFinite(rounds)) return 10;
+ return Math.min(25, Math.max(1, Math.trunc(rounds)));
+ })(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| estimatedRounds: Number(s.estimatedRounds) || 10, | |
| estimatedRounds: (() => { | |
| const rounds = Number(s.estimatedRounds); | |
| if (!Number.isFinite(rounds)) return 10; | |
| return Math.min(25, Math.max(1, Math.trunc(rounds))); | |
| })(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/planner.ts` at line 91, The value assigned to estimatedRounds
(currently set via Number(s.estimatedRounds) || 10) can be non-finite or unsafe
and must be clamped; replace that expression with logic that parses the model
output to a finite integer (use Number.parseInt/Number and Number.isFinite),
apply Math.floor, enforce a minimum of 1 and a sensible maximum (e.g., 100) and
fall back to 10 if parsing fails, so estimatedRounds is always a finite, bounded
integer before it flows into maxRounds in worker.ts.
Comment on lines
+28
to
+35
| const memory = new WorkingMemory(adapter, undefined, undefined); | ||
|
|
||
| const totalUsage: TokenUsage = { promptTokens: 0, completionTokens: 0, totalTokens: 0 }; | ||
| const filesChanged: FileChange[] = []; | ||
|
|
||
| // Build system prompt with workspace profile + subtask context | ||
| const systemPrompt = buildWorkerSystemPrompt(workspaceProfile, subtask, context); | ||
|
|
There was a problem hiding this comment.
Pass the real systemPrompt into WorkingMemory.
src/utils/memory.ts includes systemPrompt in shouldCompact(), but this worker instantiates memory with undefined and still sends the full prompt on every provider turn. That undercounts context size and can delay compaction until the provider rejects the request.
Suggested fix
- const maxRounds = subtask.estimatedRounds + 5; // budget + 5 bonus
- const memory = new WorkingMemory(adapter, undefined, undefined);
+ const maxRounds = subtask.estimatedRounds + 5; // budget + 5 bonus
@@
// Build system prompt with workspace profile + subtask context
const systemPrompt = buildWorkerSystemPrompt(workspaceProfile, subtask, context);
+ const memory = new WorkingMemory(adapter, undefined, systemPrompt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/worker.ts` around lines 28 - 35, The WorkingMemory is instantiated
with undefined for systemPrompt so its shouldCompact() call can't account for
the full prompt; pass the computed systemPrompt from buildWorkerSystemPrompt
into the WorkingMemory constructor (i.e., construct new WorkingMemory(adapter,
systemPrompt, ...)) so WorkingMemory.can measure prompt size correctly and
compact before provider turns; ensure references to
WorkingMemory.shouldCompact() and any internal size accounting use that passed
systemPrompt.
Comment on lines
+53
to
+78
| // Context compaction check | ||
| if (memory.shouldCompact(messages)) { | ||
| messages.splice(0, messages.length, ...(await memory.compact(messages))); | ||
| } | ||
|
|
||
| // Scratchpad checkpoint | ||
| if (memory.shouldScratchpad(round) && round > 0) { | ||
| messages.push(memory.scratchpadPrompt()); | ||
| } | ||
|
|
||
| // Call provider | ||
| if (!adapter.chatWithTools) { | ||
| return { | ||
| subtaskId: subtask.id, | ||
| status: 'failed', | ||
| filesChanged, | ||
| summary: 'Provider does not support tool calling', | ||
| tokensUsed: totalUsage, | ||
| error: 'Provider does not support chatWithTools', | ||
| }; | ||
| } | ||
|
|
||
| const response = await adapter.chatWithTools(messages, toolSchemas, { | ||
| systemPrompt, | ||
| temperature: 0.2, | ||
| }); |
There was a problem hiding this comment.
Unexpected provider errors bypass the orchestrator retry path.
If memory.compact() or adapter.chatWithTools() throws, executeWorker() rejects instead of returning { status: 'failed' }. src/agent/orchestrator.ts only retries failed results, so one transient provider error aborts the whole run instead of retrying the subtask.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/worker.ts` around lines 53 - 78, executeWorker currently lets
exceptions from memory.compact() and adapter.chatWithTools() bubble up, which
causes the orchestrator to skip its retry logic; wrap the calls to
memory.compact(...) and adapter.chatWithTools(...) in try/catch blocks inside
executeWorker and on any thrown error return the same shaped failure object the
orchestrator expects (include subtaskId, status: 'failed', filesChanged, summary
or error message, tokensUsed/totalUsage) instead of throwing; specifically guard
the memory.compact(messages) invocation and the adapter.chatWithTools(messages,
toolSchemas, {...}) call so failures result in a consistent returned failure
result rather than an exception.
Comment on lines
+44
to
+45
| const sourceExts = new Set(['.ts', '.tsx', '.js', '.jsx', '.py', '.go', '.rs']); | ||
| const skipDirs = new Set(['.git', 'node_modules', '.jam', 'dist', 'build', '.next', '.cache', 'coverage']); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/intel/conventions.ts | head -100Repository: sunilp/jam-cli
Length of output: 3922
🏁 Script executed:
wc -l src/intel/conventions.tsRepository: sunilp/jam-cli
Length of output: 86
🏁 Script executed:
cat -n src/intel/conventions.ts | sed -n '290,310p'Repository: sunilp/jam-cli
Length of output: 1044
🏁 Script executed:
cat -n src/intel/conventions.ts | sed -n '390,410p'Repository: sunilp/jam-cli
Length of output: 1070
🏁 Script executed:
cat -n src/intel/conventions.ts | sed -n '260,310p'Repository: sunilp/jam-cli
Length of output: 2349
🏁 Script executed:
cat -n src/intel/conventions.ts | sed -n '200,250p'Repository: sunilp/jam-cli
Length of output: 1642
🏁 Script executed:
cat -n src/intel/conventions.ts | sed -n '150,210p'Repository: sunilp/jam-cli
Length of output: 2529
Naming vote detection is JS-only and defaults non-JS repos to snake_case.
The regexes at lines 300–302 only match const|let|var|function keywords. Python (def), Rust (fn), and Go (func) function declarations are never captured, leaving all naming vote counts at zero for non-JS files. When maxNaming equals zero, the condition at line 397 (maxNaming === totals.snakeCase) evaluates to true, forcing snake_case as the result regardless of actual conventions in those repos.
Also applies to: 395–398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.ts` around lines 44 - 45, The naming-vote detection
currently only matches JS keywords (const|let|var|function) so non-JS
functions/vars (Python def, Rust fn, Go func) produce zero votes and cause
maxNaming===totals.snakeCase to default to snake_case; update the
declaration-detection regexes (the ones that match const|let|var|function) to
also recognize language-specific keywords "def", "fn", and "func" and any
language-appropriate variable patterns, and adjust the final selection logic
(the code using maxNaming, totals, and totals.snakeCase) to only treat
totals.snakeCase as the winner when maxNaming > 0 (i.e., require at least one
vote before defaulting), so naming detection works for non-JS files and we don’t
force snake_case when no votes exist.
Comment on lines
+291
to
+297
| // Trailing commas — lines ending with , followed by } or ] on next line | ||
| if (stripped.endsWith(',') && i + 1 < lines.length) { | ||
| const nextTrimmed = lines[i + 1]!.trim(); | ||
| if (nextTrimmed.startsWith('}') || nextTrimmed.startsWith(']') || nextTrimmed.startsWith(')')) { | ||
| vote.trailingCommaCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
This trailing-comma heuristic only counts positives.
noTrailingCommaCount is never incremented, so trailingCommas flips to true as soon as one sampled block has a trailing comma, even if most eligible blocks omit them.
Also applies to: 391-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/intel/conventions.ts` around lines 291 - 297, The heuristic only
increments vote.trailingCommaCount when a trailing comma is detected, so
trailingCommas becomes true after one positive sample; modify the logic in the
trailing-comma detection (the block that checks stripped.endsWith(',') and
nextTrimmed.startsWith('}',']',')') and the analogous block around lines
391-392) to also increment vote.noTrailingCommaCount when the eligible
closing-line pattern is present but the current line does NOT end with a comma;
this ensures you track both positives and negatives before computing the
trailingCommas decision.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/commands/run.ts (2)
73-79: Error message loses underlying cause details.When file reading fails, the error message only shows the file path but not why it failed (e.g., ENOENT vs EACCES). Including the error cause would help users diagnose issues.
🛡️ Proposed fix to include error details
try { instruction = (await readFile(options.file, 'utf-8')).trim(); - } catch { - await printError(`Cannot read file: ${options.file}`); + } catch (err) { + const cause = err instanceof Error ? err.message : String(err); + await printError(`Cannot read file: ${options.file}`, cause); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/run.ts` around lines 73 - 79, The catch block for reading the file loses the underlying error; change the catch to capture the thrown error (e.g., catch (err)) and include the error details in the printError call when reading options.file fails (use err.message or JSON.stringify(err) to show code/stack as appropriate), then exit as before; update the block around readFile/options.file/instruction to pass the error context into the error message.
154-154: Hardcoded 10-minute timeout may not suit all use cases.The
AbortSignal.timeout(600000)is hardcoded. Complex multi-step tasks might need more time, while users may want shorter timeouts for quick operations. Consider making this configurable viaconfig.agent?.timeoutor a--timeoutCLI flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/run.ts` at line 154, The hardcoded AbortSignal.timeout(600000) should be replaced with a configurable timeout sourced from the agent config or CLI flag; read a numeric timeout from config.agent?.timeout or a parsed --timeout option (falling back to 600000 ms if absent), validate it is a positive integer, and pass that value into AbortSignal.timeout instead of the literal. Locate the call site in src/commands/run.ts (the object constructing signal: AbortSignal.timeout(...))—update the surrounding function (e.g., runCommand/executeAgent or the command handler) to accept/derive the timeout, wire the CLI config into that parameter, and ensure sensible validation and defaulting.src/commands/go.ts (2)
183-186: Close handler skips cleanup for running tasks and MCP connections.When the user presses Ctrl+D (or Ctrl+C triggers close), this handler exits immediately without aborting any in-progress task or shutting down the MCP manager. The
/exitcommand does this properly, but the close event bypasses it.🛡️ Proposed fix to add cleanup on close
rl.on('close', () => { + if (currentAbort) { + currentAbort.abort(); + } + void mcpManager.shutdown(); process.stderr.write('\nBye!\n'); process.exit(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/go.ts` around lines 183 - 186, The close handler for rl (rl.on('close')) currently calls process.exit(0) directly and skips aborting in-progress tasks and shutting down the MCP manager; refactor the cleanup performed by the `/exit` command into a single reusable function (e.g., gracefulShutdown or shutdownAndExit) that aborts running tasks, closes MCP connections/manager (MCP manager instance), and then exits, then replace the direct process.exit(0) in rl.on('close') with a call to that cleanup function so both `/exit` and the close event perform identical shutdown logic.
91-93: Async IIFE errors may go unhandled.The
void (async () => { ... })()pattern discards the promise, so any rejection not caught inside the IIFE becomes an unhandled promise rejection. The inner try/catch at line 144 only wrapsorchestrator.execute, but errors from other async calls (e.g.,mcpManager.shutdown()at line 98) could escape.🛡️ Proposed fix to catch all async errors
rl.on('line', (line) => { - void (async () => { + (async () => { const input = line.trim(); // ... handler code ... - })(); + })().catch((err) => { + process.stderr.write(`Unexpected error: ${err instanceof Error ? err.message : String(err)}\n`); + rl.prompt(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/go.ts` around lines 91 - 93, The async IIFE used in the rl.on('line', ...) handler currently has its returned promise discarded (void (async () => { ... })()), which allows rejections from any awaited call (e.g., mcpManager.shutdown(), orchestrator.execute()) to become unhandled; wrap the entire async body in a try/catch/finally (or attach a .catch and .finally to the invoked promise) so every error is caught, log the error via the existing logger and ensure mcpManager.shutdown() runs in finally (or is called on error) to prevent unhandled rejections and guarantee cleanup; locate the IIFE inside the rl.on('line', ...) block and modify it accordingly (or replace the void invocation with (async ()=>{...})().catch(...).finally(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/run.ts`:
- Around line 139-141: The code sets maxWorkers using parseInt(options.workers,
10) without validating the result; update the logic around maxWorkers (the
options.workers parse + fallback to config.agent?.maxWorkers ?? 3) to validate
that parsed = parseInt(options.workers, 10) yields a finite, positive integer
(e.g., Number.isFinite(parsed) && parsed > 0); if invalid, fall back to
config.agent?.maxWorkers if that is a valid positive integer, otherwise use 3;
ensure you reference the same symbols (options.workers, maxWorkers,
config.agent?.maxWorkers) so the code enforces a sane default instead of
allowing NaN.
- Line 136: The mode fallback in run.ts is inconsistent with go.ts: update the
assignment for the variable named "mode" (currently set with options.auto ?
'auto' : (config.agent?.defaultMode ?? 'auto')) to use the same default as go.ts
(i.e., use 'supervised' when config.agent?.defaultMode is undefined) so that jam
run and jam go behave consistently; locate the "mode" constant in run.ts and
replace the fallback 'auto' with 'supervised'.
---
Nitpick comments:
In `@src/commands/go.ts`:
- Around line 183-186: The close handler for rl (rl.on('close')) currently calls
process.exit(0) directly and skips aborting in-progress tasks and shutting down
the MCP manager; refactor the cleanup performed by the `/exit` command into a
single reusable function (e.g., gracefulShutdown or shutdownAndExit) that aborts
running tasks, closes MCP connections/manager (MCP manager instance), and then
exits, then replace the direct process.exit(0) in rl.on('close') with a call to
that cleanup function so both `/exit` and the close event perform identical
shutdown logic.
- Around line 91-93: The async IIFE used in the rl.on('line', ...) handler
currently has its returned promise discarded (void (async () => { ... })()),
which allows rejections from any awaited call (e.g., mcpManager.shutdown(),
orchestrator.execute()) to become unhandled; wrap the entire async body in a
try/catch/finally (or attach a .catch and .finally to the invoked promise) so
every error is caught, log the error via the existing logger and ensure
mcpManager.shutdown() runs in finally (or is called on error) to prevent
unhandled rejections and guarantee cleanup; locate the IIFE inside the
rl.on('line', ...) block and modify it accordingly (or replace the void
invocation with (async ()=>{...})().catch(...).finally(...)).
In `@src/commands/run.ts`:
- Around line 73-79: The catch block for reading the file loses the underlying
error; change the catch to capture the thrown error (e.g., catch (err)) and
include the error details in the printError call when reading options.file fails
(use err.message or JSON.stringify(err) to show code/stack as appropriate), then
exit as before; update the block around readFile/options.file/instruction to
pass the error context into the error message.
- Line 154: The hardcoded AbortSignal.timeout(600000) should be replaced with a
configurable timeout sourced from the agent config or CLI flag; read a numeric
timeout from config.agent?.timeout or a parsed --timeout option (falling back to
600000 ms if absent), validate it is a positive integer, and pass that value
into AbortSignal.timeout instead of the literal. Locate the call site in
src/commands/run.ts (the object constructing signal:
AbortSignal.timeout(...))—update the surrounding function (e.g.,
runCommand/executeAgent or the command handler) to accept/derive the timeout,
wire the CLI config into that parameter, and ensure sensible validation and
defaulting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6dd316e-0b37-4985-8b3f-2836c5653382
📒 Files selected for processing (2)
src/commands/go.tssrc/commands/run.ts
| const reporter = createProgressReporter({ quiet: options.quiet, json: options.json }); | ||
|
|
||
| // Determine agent mode | ||
| const mode = options.auto ? 'auto' : (config.agent?.defaultMode ?? 'auto'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the mode defaults in both files
echo "=== run.ts mode default ==="
rg -n "defaultMode.*'auto'|'auto'.*defaultMode" src/commands/run.ts
echo -e "\n=== go.ts mode default ==="
rg -n "defaultMode.*'supervised'|'supervised'.*defaultMode" src/commands/go.ts
echo -e "\n=== Config default mode ==="
rg -n "defaultMode" src/config/Repository: sunilp/jam-cli
Length of output: 433
Mode default in run.ts differs from go.ts — inconsistency causes different behavior.
Line 136 uses 'auto' as the fallback when config.agent?.defaultMode is not set, while go.ts line 69 uses 'supervised' (matching the configured default). This causes jam run and jam go to behave differently when the user has not explicitly configured defaultMode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/run.ts` at line 136, The mode fallback in run.ts is inconsistent
with go.ts: update the assignment for the variable named "mode" (currently set
with options.auto ? 'auto' : (config.agent?.defaultMode ?? 'auto')) to use the
same default as go.ts (i.e., use 'supervised' when config.agent?.defaultMode is
undefined) so that jam run and jam go behave consistently; locate the "mode"
constant in run.ts and replace the fallback 'auto' with 'supervised'.
Comment on lines
+139
to
+141
| const maxWorkers = options.workers | ||
| ? parseInt(options.workers, 10) | ||
| : (config.agent?.maxWorkers ?? 3); |
There was a problem hiding this comment.
parseInt may return NaN for invalid input without validation.
Same issue as in go.ts: if options.workers is an invalid string, parseInt returns NaN, which could cause unexpected behavior in the orchestrator.
🛡️ Proposed fix to validate worker count
- const maxWorkers = options.workers
- ? parseInt(options.workers, 10)
- : (config.agent?.maxWorkers ?? 3);
+ let maxWorkers = config.agent?.maxWorkers ?? 3;
+ if (options.workers) {
+ const parsed = parseInt(options.workers, 10);
+ if (!Number.isNaN(parsed) && parsed > 0) {
+ maxWorkers = parsed;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const maxWorkers = options.workers | |
| ? parseInt(options.workers, 10) | |
| : (config.agent?.maxWorkers ?? 3); | |
| let maxWorkers = config.agent?.maxWorkers ?? 3; | |
| if (options.workers) { | |
| const parsed = parseInt(options.workers, 10); | |
| if (!Number.isNaN(parsed) && parsed > 0) { | |
| maxWorkers = parsed; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/run.ts` around lines 139 - 141, The code sets maxWorkers using
parseInt(options.workers, 10) without validating the result; update the logic
around maxWorkers (the options.workers parse + fallback to
config.agent?.maxWorkers ?? 3) to validate that parsed =
parseInt(options.workers, 10) yields a finite, positive integer (e.g.,
Number.isFinite(parsed) && parsed > 0); if invalid, fall back to
config.agent?.maxWorkers if that is a valid positive integer, otherwise use 3;
ensure you reference the same symbols (options.workers, maxWorkers,
config.agent?.maxWorkers) so the code enforces a sane default instead of
allowing NaN.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New
src/agent/module powering bothjam go(interactive console) andjam run(one-shot) with a shared agent engine.sandbox-exec(macOS) /unshare/firejail(Linux) with permissions-only fallback--imageflag with provider vision detection and fallback.jam/workspace-profile.jsonCommands upgraded
jam run— delegates to orchestrator (legacy fallback viaJAM_LEGACY_RUN=1)jam go— rewritten as interactive console with/stop,/status,/help,/exit--auto,--workers <n>,--image <path>,--no-sandbox,--file <path>Stats
Test plan
npx vitest run src/agent/— 158 tests across 14 filesnpx vitest run— full suite, no regressionsjam run "add a hello world function to src/hello.ts"— verify orchestrator flowjam go— verify interactive console,/status,/stop,/exitJAM_LEGACY_RUN=1 jam run "test"— verify legacy fallback works🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests