-
Notifications
You must be signed in to change notification settings - Fork 2
feat: integrate background_task system into agents #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… unit tests - Add error logging to replace silent catch blocks (Task 2) - Add TTL-based cleanup to prevent memory leak (Task 3) - Add proper response types for API calls (Task 4) - Add comprehensive unit tests for BackgroundTaskManager (Task 5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 13 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/tools/background-task-error-logging.test.ts">
<violation number="1" location="tests/tools/background-task-error-logging.test.ts:16">
P2: Test doesn't verify what its name claims. The assertion only checks that catch blocks have a named parameter, not that `console.error` is actually called. A catch like `.catch((err) => {})` would pass this test despite not logging anything. Consider parsing the full catch block body and verifying it contains `console.error`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const fs = await import("node:fs/promises"); | ||
| const source = await fs.readFile("src/tools/background-task/manager.ts", "utf-8"); | ||
|
|
||
| // All .catch blocks should have console.error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Test doesn't verify what its name claims. The assertion only checks that catch blocks have a named parameter, not that console.error is actually called. A catch like .catch((err) => {}) would pass this test despite not logging anything. Consider parsing the full catch block body and verifying it contains console.error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/tools/background-task-error-logging.test.ts, line 16:
<comment>Test doesn't verify what its name claims. The assertion only checks that catch blocks have a named parameter, not that `console.error` is actually called. A catch like `.catch((err) => {})` would pass this test despite not logging anything. Consider parsing the full catch block body and verifying it contains `console.error`.</comment>
<file context>
@@ -0,0 +1,23 @@
+ const fs = await import("node:fs/promises");
+ const source = await fs.readFile("src/tools/background-task/manager.ts", "utf-8");
+
+ // All .catch blocks should have console.error
+ const catchBlocks = source.match(/\.catch\s*\([^)]+\)/g) || [];
+ for (const block of catchBlocks) {
</file context>
Summary
Changes
Background Task System Fixes
progress.lastMessage(defined but never populated).catch(() => {})with properconsole.errorloggingAgent Prompt Updates
Testing
Design Document
See
thoughts/shared/designs/2026-01-01-background-task-integration-design.md(gitignored)Summary by cubic
Integrated the background_task system into executor, planner, and project-initializer for real async parallelism, and fixed stability issues in the task manager. This speeds up execution and prevents memory leaks and silent failures.
New Features
Bug Fixes
Written for commit 23f96a5. Summary will update on new commits.