Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com> Fixes #1589
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: triepod-ai <199543909+triepod-ai@users.noreply.github.com>
Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com> fix for #1585 (part 2: legacy utils.js pattern)
🦋 Changeset detectedLatest commit: 712a078 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds user-defined Task.metadata with full plumbing (types, entity, storage merge, MCP validation and tools, bridge), introduces verbose streaming loop output with LoopOutputCallbacks and CLI flags, exports FileOperations + modifyJSON, adds MCP bundle/packaging automation, extensive tests, and a forward-port workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LoopService
participant Claude
participant Callbacks
User->>CLI: run loop --verbose
CLI->>LoopService: run(config { verbose: true, callbacks })
LoopService->>LoopService: validate config (reject verbose + sandbox)
LoopService->>Callbacks: onIterationStart(iter, total)
LoopService->>Claude: spawn child (command with --stream-json)
Claude-->>LoopService: stream JSON lines
LoopService->>LoopService: parse lines into events
alt valid event
LoopService->>Callbacks: onText/onToolUse/onStderr/onOutput
LoopService->>LoopService: buffer/aggregate output
else invalid event
LoopService->>Callbacks: onError / log
end
LoopService->>Callbacks: onIterationEnd(iteration)
LoopService-->>CLI: final LoopResult (output / errorMessage)
CLI->>User: display result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com> Fixes #1589
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: triepod-ai <199543909+triepod-ai@users.noreply.github.com>
Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com> fix for #1585 (part 2: legacy utils.js pattern)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
3-52: Changelog version doesn’t match release target (0.43.0).
PR objective says this is the 0.43.0 release, but the top entry is still 0.42.0. Please add a 0.43.0 section (or confirm the release target) and place the new release notes under it so the changelog aligns with the published version.
🤖 Fix all issues with AI agents
In @.github/scripts/sync-manifest-version.mjs:
- Around line 76-85: The spawnSync call to run 'npx mcpb pack' can set
result.error (e.g., ENOENT) while result.status is null; update the error
handling after spawnSync(spawnSync, result, bundleName, rootDir) to treat any
result.error as a failure in addition to non-zero exit codes: if result.error or
result.status !== 0, log a descriptive message including result.error (and
optionally result.stdout/stderr) and call process.exit(1). Ensure you reference
the existing variables result, bundleName, and rootDir when locating the fix.
In `@tests/setup.js`:
- Around line 55-65: The afterAll cleanup currently calls
process.removeAllListeners(signal) which indiscriminately drops all handlers;
change it to preserve baseline listeners captured at suite start and only remove
listeners added by this suite: at setup (before tests run) record original
listeners for each signal via process.listeners(signal) and store them (e.g., in
a module-scope map), then in afterAll iterate signals and remove only listeners
that are not in the saved baseline (or restore by removing all and re-attaching
only the saved originals), updating the existing afterAll block (referencing the
afterAll callback, the listeners array, and the use of
process.removeAllListeners) to use the selective removal/restoration approach.
🧹 Nitpick comments (4)
packages/tm-core/src/modules/loop/services/loop.service.ts (2)
331-337: Consider documenting the 50MB buffer size rationale.The
maxBufferof 50MB is significant. While this aligns with the comment inLoopIteration.outputdocumentation mentioning "up to 50MB per iteration," adding a brief inline comment here would help future maintainers understand why this specific limit was chosen.📝 Suggested documentation
const result = spawnSync(command, args, { cwd: this.projectRoot, encoding: 'utf-8', + // Claude CLI output can be large; matches LoopConfig.includeOutput docs maxBuffer: 50 * 1024 * 1024, stdio: ['inherit', 'pipe', 'pipe'] });
430-436: Redundant JSON check before logging parse error.The condition on line 432 checks
line.trim().startsWith('{')butprocessLinealready exits early if!line.startsWith('{')on line 414. This inner check will always be true when reached.♻️ Simplified error logging
} catch (error) { - // Log malformed JSON for debugging (non-JSON lines like system output are expected) - if (line.trim().startsWith('{')) { - const parseError = `Failed to parse JSON event: ${error instanceof Error ? error.message : 'Unknown error'}. Line: ${line.substring(0, 100)}...`; - this.reportError(callbacks, parseError, 'warning'); - } + // Log malformed JSON for debugging (we only reach here for lines starting with '{') + const parseError = `Failed to parse JSON event: ${error instanceof Error ? error.message : 'Unknown error'}. Line: ${line.substring(0, 100)}...`; + this.reportError(callbacks, parseError, 'warning'); }apps/cli/src/commands/loop.command.spec.ts (1)
253-330: Consider adding test coverage for new--verboseand--no-outputoptions.The execute integration tests don't cover the new
verboseandoutputoptions. While existing tests verify the core loop functionality, tests for the new streaming/output control behavior would improve confidence.Consider adding tests like:
- Verify
callbacksare passed toloop.runconfig- Verify
verbose: truewhen--verboseis provided- Verify
includeOutput: falsewhen--no-outputis provided.github/scripts/sync-manifest-version.mjs (1)
63-71: Consider adding error handling for file deletion.The
unlinkSynccall could throw if the file is locked or permissions are insufficient. While unlikely in a release context, wrapping this in a try-catch would make the script more robust.♻️ Add error handling for file deletion
for (const file of files) { if (file.endsWith('.mcpb')) { const filePath = join(rootDir, file); console.log(`Removing old bundle: ${file}`); - unlinkSync(filePath); + try { + unlinkSync(filePath); + } catch (error) { + console.warn(`Warning: Failed to remove ${file}: ${error.message}`); + } } }
| const result = spawnSync('npx', ['mcpb', 'pack', '.', bundleName], { | ||
| cwd: rootDir, | ||
| encoding: 'utf8', | ||
| stdio: 'inherit' | ||
| }); | ||
|
|
||
| if (result.status !== 0) { | ||
| console.error('Failed to generate MCPB bundle'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Handle spawn error case explicitly.
If npx itself fails to execute (e.g., ENOENT), result.error will be set but result.status may be null. The current check only looks at status !== 0, which would pass when status is null.
🐛 Proposed fix
+if (result.error) {
+ console.error('Failed to execute mcpb:', result.error.message);
+ process.exit(1);
+}
+
if (result.status !== 0) {
console.error('Failed to generate MCPB bundle');
process.exit(1);
}🤖 Prompt for AI Agents
In @.github/scripts/sync-manifest-version.mjs around lines 76 - 85, The
spawnSync call to run 'npx mcpb pack' can set result.error (e.g., ENOENT) while
result.status is null; update the error handling after spawnSync(spawnSync,
result, bundleName, rootDir) to treat any result.error as a failure in addition
to non-zero exit codes: if result.error or result.status !== 0, log a
descriptive message including result.error (and optionally result.stdout/stderr)
and call process.exit(1). Ensure you reference the existing variables result,
bundleName, and rootDir when locating the fix.
| // Clean up signal-exit listeners after all tests to prevent open handle warnings | ||
| // This is needed because packages like proper-lockfile register signal handlers | ||
| afterAll(async () => { | ||
| // Give any pending async operations time to complete | ||
| await new Promise((resolve) => setImmediate(resolve)); | ||
|
|
||
| // Clean up any registered signal handlers from signal-exit | ||
| const listeners = ['SIGINT', 'SIGTERM', 'SIGHUP']; | ||
| for (const signal of listeners) { | ||
| process.removeAllListeners(signal); | ||
| } |
There was a problem hiding this comment.
Avoid removing all signal listeners globally.
process.removeAllListeners(signal) can drop Jest/other cleanup handlers for subsequent suites. Prefer removing only listeners added during this suite (or restoring the original set) to avoid breaking signal handling.
🛠️ Proposed fix (remove only non-baseline listeners)
+const signalExitSignals = ['SIGINT', 'SIGTERM', 'SIGHUP'];
+const baselineSignalListeners = new Map(
+ signalExitSignals.map((signal) => [signal, process.listeners(signal)])
+);
+
// Clean up signal-exit listeners after all tests to prevent open handle warnings
// This is needed because packages like proper-lockfile register signal handlers
afterAll(async () => {
// Give any pending async operations time to complete
await new Promise((resolve) => setImmediate(resolve));
- // Clean up any registered signal handlers from signal-exit
- const listeners = ['SIGINT', 'SIGTERM', 'SIGHUP'];
- for (const signal of listeners) {
- process.removeAllListeners(signal);
- }
+ // Clean up any registered signal handlers from signal-exit
+ for (const signal of signalExitSignals) {
+ const baseline = baselineSignalListeners.get(signal) || [];
+ for (const listener of process.listeners(signal)) {
+ if (!baseline.includes(listener)) {
+ process.removeListener(signal, listener);
+ }
+ }
+ }
});📝 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.
| // Clean up signal-exit listeners after all tests to prevent open handle warnings | |
| // This is needed because packages like proper-lockfile register signal handlers | |
| afterAll(async () => { | |
| // Give any pending async operations time to complete | |
| await new Promise((resolve) => setImmediate(resolve)); | |
| // Clean up any registered signal handlers from signal-exit | |
| const listeners = ['SIGINT', 'SIGTERM', 'SIGHUP']; | |
| for (const signal of listeners) { | |
| process.removeAllListeners(signal); | |
| } | |
| const signalExitSignals = ['SIGINT', 'SIGTERM', 'SIGHUP']; | |
| const baselineSignalListeners = new Map( | |
| signalExitSignals.map((signal) => [signal, process.listeners(signal)]) | |
| ); | |
| // Clean up signal-exit listeners after all tests to prevent open handle warnings | |
| // This is needed because packages like proper-lockfile register signal handlers | |
| afterAll(async () => { | |
| // Give any pending async operations time to complete | |
| await new Promise((resolve) => setImmediate(resolve)); | |
| // Clean up any registered signal handlers from signal-exit | |
| for (const signal of signalExitSignals) { | |
| const baseline = baselineSignalListeners.get(signal) || []; | |
| for (const listener of process.listeners(signal)) { | |
| if (!baseline.includes(listener)) { | |
| process.removeListener(signal, listener); | |
| } | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@tests/setup.js` around lines 55 - 65, The afterAll cleanup currently calls
process.removeAllListeners(signal) which indiscriminately drops all handlers;
change it to preserve baseline listeners captured at suite start and only remove
listeners added by this suite: at setup (before tests run) record original
listeners for each signal via process.listeners(signal) and store them (e.g., in
a module-scope map), then in afterAll iterate signals and remove only listeners
that are not in the saved baseline (or restore by removing all and re-attaching
only the saved originals), updating the existing afterAll block (referencing the
afterAll callback, the listeners array, and the use of
process.removeAllListeners) to use the selective removal/restoration approach.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tm-core/src/common/types/index.ts (1)
258-270: Type guardisTaskStatusis missing 'completed' status.The
TaskStatustype (line 28) includes'completed', but theisTaskStatustype guard only checks for'review'and omits'completed'. This inconsistency could cause valid tasks withstatus: 'completed'to fail the type guard check.Proposed fix
export function isTaskStatus(value: unknown): value is TaskStatus { return ( typeof value === 'string' && [ 'pending', 'in-progress', 'done', 'deferred', 'cancelled', 'blocked', - 'review' + 'review', + 'completed' ].includes(value) ); }mcp-server/src/tools/update-subtask.js (2)
27-51: MakeprojectRootoptional in the MCP tool schema.
Requiring it blocks clients that rely onwithNormalizedProjectRoot/session defaults. Switch to.optional()so the tool works without an explicit path.As per coding guidelines, ...🛠️ Suggested fix
- projectRoot: z - .string() - .describe('The directory of the project. Must be an absolute path.'), + projectRoot: z + .string() + .optional() + .describe('The directory of the project. Must be an absolute path.'),
60-66: Sanitize logged args now that metadata can include sensitive data.
Logging the full args will now include user metadata (and potentially large payloads/PII). Redact metadata (and ideally prompt) before logging.As per coding guidelines, ...🛡️ Suggested fix
- log.info(`Updating subtask with args: ${JSON.stringify(args)}`); + const safeArgs = { + ...args, + metadata: args.metadata ? '[redacted]' : undefined + }; + log.info(`Updating subtask with args: ${JSON.stringify(safeArgs)}`);
🤖 Fix all issues with AI agents
In `@scripts/modules/task-manager/update-subtask-by-id.js`:
- Around line 80-87: The prompt validation currently calls prompt.trim() without
ensuring prompt is a string and also allows forwarding non-string prompts to the
remote update; fix this by introducing a single boolean flag (e.g.,
isPromptEmpty) computed after a type check like typeof prompt === 'string' ?
prompt.trim() === '' : true, use that flag in the existing guard that throws
when both prompt is empty and metadata is falsy, and use the same flag/guard
before sending the remote update so you never forward a non-string prompt; apply
the identical change to the other prompt validation block later in the file (the
second prompt/metadata check).
🧹 Nitpick comments (2)
apps/docs/capabilities/task-structure.mdx (1)
58-60: Clarify the “AI-Safe” claim to match opt‑in metadata updates.
The wording “never overwritten by AI” is absolute, but later you document explicit MCP metadata updates. Suggest softening to “preserved by default unless explicitly updated” to avoid a contradiction.✏️ Suggested tweak
- AI operations preserve your metadata - it's never overwritten by AI. + AI operations preserve metadata by default unless you explicitly update it.packages/tm-core/tests/integration/storage/file-storage-metadata.test.ts (1)
51-156: Consider adding a tagged-format metadata round-trip.
FileStorage supports tags; adding at least onesaveTasks/loadTasks/updateTaskpath with a non-default tag would ensure metadata preservation across tagged task lists.Based on learnings, ...
| // Allow metadata-only updates (no prompt required if metadata is provided) | ||
| if ( | ||
| (!prompt || typeof prompt !== 'string' || prompt.trim() === '') && | ||
| !metadata | ||
| ) { | ||
| throw new Error( | ||
| 'Prompt cannot be empty. Please provide context for the subtask update.' | ||
| 'Prompt cannot be empty unless metadata is provided. Please provide context for the subtask update or metadata to merge.' | ||
| ); |
There was a problem hiding this comment.
Guard prompt type before calling .trim()
With metadata present, the current validation allows non-string prompt, but the metadata-only path calls prompt.trim() and will throw. It also forwards a non-string prompt to the remote path. Add a type guard and reuse a single isPromptEmpty flag.
Proposed fix
// Basic validation - ID must be present
if (!subtaskId || typeof subtaskId !== 'string') {
throw new Error('Subtask ID cannot be empty.');
}
+ if (typeof prompt !== 'undefined' && typeof prompt !== 'string') {
+ throw new Error('Prompt must be a string when provided.');
+ }
+ const isPromptEmpty = !prompt || prompt.trim() === '';
// Allow metadata-only updates (no prompt required if metadata is provided)
- if (
- (!prompt || typeof prompt !== 'string' || prompt.trim() === '') &&
- !metadata
- ) {
+ if (isPromptEmpty && !metadata) {
throw new Error(
'Prompt cannot be empty unless metadata is provided. Please provide context for the subtask update or metadata to merge.'
);
}
@@
- if (metadata && (!prompt || prompt.trim() === '')) {
+ if (metadata && isPromptEmpty) {Also applies to: 177-180
🤖 Prompt for AI Agents
In `@scripts/modules/task-manager/update-subtask-by-id.js` around lines 80 - 87,
The prompt validation currently calls prompt.trim() without ensuring prompt is a
string and also allows forwarding non-string prompts to the remote update; fix
this by introducing a single boolean flag (e.g., isPromptEmpty) computed after a
type check like typeof prompt === 'string' ? prompt.trim() === '' : true, use
that flag in the existing guard that throws when both prompt is empty and
metadata is falsy, and use the same flag/guard before sending the remote update
so you never forward a non-string prompt; apply the identical change to the
other prompt validation block later in the file (the second prompt/metadata
check).
…1612) The MDX parser was interpreting indented </Accordion> tags as list items, causing deployment failures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Ed Sumerfield <esumerfield@stackct.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| (st) => | ||
| String(st.id) === String(updatedSubtask.id) || | ||
| (updatedSubtask.title && st.title === updatedSubtask.title) | ||
| ); |
There was a problem hiding this comment.
Subtask metadata matching uses wrong priority order
Medium Severity
The subtask metadata matching logic uses || which allows title matching to take precedence over ID matching when a title-matching subtask appears earlier in the array. The comment states the intent is to "fall back to title match if IDs don't match," but the || operator doesn't implement a fallback—it treats both conditions as equal alternatives. This can cause metadata from the wrong subtask to be preserved when an updated subtask has the same title as a different original subtask.
6 participants
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Note
Adds one-click Claude Desktop install and introduces durable task metadata with MCP updates, plus a streaming loop UX.
manifest.json,.mcpbignore,icon.png;versionscript syncsmanifest.jsonand builds.mcpbbundle-v/--verbosereal-time streaming,--no-output, improved error handling/validation, progress header tweaks; newLoopOutputCallbacks,includeOutput,briefmetadataon tasks/subtasks preserved across AI ops/storage; MCP toolsupdate_task/update_subtaskacceptmetadata(gated byTASK_MASTER_ALLOW_METADATA_UPDATES=true); validation util addedFileOperations; bridge/API path passesuseResearchandmetadataWritten by Cursor Bugbot for commit 712a078. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.