refactor: standardize code formatting and update test structure#7
Conversation
Co-authored-by: DevOps Bot <dev@bl1nk.note>
- Add Context7Client class for MCP server communication - Add 12 LSP tools for IDE-like capabilities (hover, definitions, references, etc.) - Add 6 notepad tools for managing project notes and context - Add comprehensive test suites with 54 passing tests covering error handling, mocking, and edge cases All tools include proper TypeScript types, Zod validation schemas, and follow project conventions.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive suite of AI agent tooling, UI components, and supporting infrastructure. Changes include a new code-block component with Shiki syntax highlighting, new agent tools (delegate task, GitHub commit/PR, LSP operations, notepad management), client libraries for Kilo API and Context7, a bot prompt builder, UI component updates, and expanded test coverage. Dependencies are bumped and workspace cleanup scripts added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Component
participant CodeBlock as CodeBlock Component
participant Shiki as Shiki Tokenizer
participant Cache as Token Cache
Client->>CodeBlock: Render with code & language
CodeBlock->>Cache: Check cached tokens
alt Cache Hit
Cache-->>CodeBlock: Return cached tokens
else Cache Miss
CodeBlock->>Shiki: Request tokenization
Shiki-->>CodeBlock: Return tokens (async)
CodeBlock->>Cache: Store in cache
end
CodeBlock->>CodeBlock: Render with tokens + styling
CodeBlock-->>Client: Display syntax-highlighted code
sequenceDiagram
participant Agent as ToolLoopAgent
participant DelegateTask as Delegate Task Tool
participant Sandbox as Sandbox Executor
participant Subagent as Subagent Stream
participant Aggregator as Result Aggregator
Agent->>DelegateTask: Execute with task list
DelegateTask->>Sandbox: Launch subagent for each task
Sandbox->>Subagent: Stream execution
Subagent->>Subagent: Process messages & tools
Subagent-->>Sandbox: Emit finish event (usage)
Subagent-->>Sandbox: Final message array
Sandbox->>Aggregator: Collect task output & usage
Aggregator->>Aggregator: Aggregate tool call counts
Aggregator-->>DelegateTask: Return aggregated result
DelegateTask-->>Agent: Yield final snapshot
sequenceDiagram
participant Tool as GitHub Commit/PR Tool
participant SandboxExec as Sandbox Executor
participant Git as Git Commands
participant GitHub as GitHub CLI
Tool->>SandboxExec: Execute with branch & message
SandboxExec->>Git: git checkout -b (branch)
SandboxExec->>Git: git add -A
SandboxExec->>Git: git commit -m (message)
SandboxExec->>Git: git push -u origin (branch)
Git-->>SandboxExec: Success
SandboxExec->>GitHub: gh pr create (title, body)
alt PR Creation Success
GitHub-->>SandboxExec: PR URL
else PR Creation Fails
GitHub-->>SandboxExec: Error details
end
SandboxExec-->>Tool: Return { success, pr, errorDetails }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sandbox/vercel/sandbox.ts (1)
110-131:⚠️ Potential issue | 🟠 MajorReplace the polynomial REDOS-vulnerable regex with an anchored alternation pattern.
The regex
/^.*?github\.com[/:]([^/]+?)\/([^/]+?)(?:\.git)?$/contains a lazy prefix (.*?) combined with lazy capture groups ([^/]+?), which CodeQL flags as polynomial-time backtracking on adversarial inputs—for example, long strings with repeatedgithub.com:prefixes that don't ultimately match. The lazy quantifiers force the engine to backtrack through multiple positions.Replace with an explicitly anchored pattern that eliminates ambiguity:
🛡️ Suggested fix
function buildAuthenticatedGitHubUrl( repoUrl: string, token: string, ): string | null { - // Match GitHub URL patterns to avoid REDOS vulnerability - // Matches: github.com/:owner/:repo or github.com/:owner/:repo.git - const githubUrlMatch = repoUrl.match( - /^.*?github\.com[/:]([^/]+?)\/([^/]+?)(?:\.git)?$/, - ); + const match = repoUrl.match( + /^(?:https?:\/\/(?:[^@/]*@)?github\.com\/|git@github\.com:)([^/]+)\/([^/]+?)(?:\.git)?\/?$/, + ); - if (!githubUrlMatch) { + if (!match) { return null; } - const [, owner, repo] = githubUrlMatch; + const [, owner, repo] = match; - // Validate that owner and repo are non-empty if (!owner || !repo) { return null; } return `https://x-access-token:${token}@github.com/${owner}/${repo}.git`; }This pattern avoids exponential backtracking by anchoring to known GitHub URL prefixes (
https://with optional auth orgit@) and using a single non-overlapping alternation with no leading wildcard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sandbox/vercel/sandbox.ts` around lines 110 - 131, The regex in buildAuthenticatedGitHubUrl (operating on repoUrl) uses a leading ".*?" and lazy captures that can cause polynomial backtracking; replace that pattern with an anchored alternation that explicitly matches allowed GitHub URL forms (e.g., ^(?:https?:\/\/(?:[^@\/]+@)?github\.com[:\/]|git@github\.com:)([^\/]+)\/([^\/]+?)(?:\.git)?$) so you no longer rely on a wildcard prefix or lazy quantifiers; update the match logic in buildAuthenticatedGitHubUrl to use the new anchored pattern, keep the owner/repo extraction (owner, repo) and the same null-return checks when no match is found, and leave the final URL construction using token unchanged.
🟡 Minor comments (13)
.github/workflows/codeql.yml-14-20 (1)
14-20:⚠️ Potential issue | 🟡 MinorCodeQL triggers are scoped to a single non-default branch.
Both
pushandpull_requesttriggers (and the implicit baseline used by the schedule) only targetb/devcontainer-setup. Once this work merges back tomain/the default branch, CodeQL will no longer run on push/PR events there, leaving the default branch without code scanning coverage. If this scoping is intentional only for the duration of the devcontainer setup effort, please track restoringmain(and any other long‑lived branches) before merge-back; otherwise consider includingmainhere now.🔧 Suggested adjustment
on: push: - branches: ["b/devcontainer-setup"] + branches: ["main", "b/devcontainer-setup"] pull_request: - branches: ["b/devcontainer-setup"] + branches: ["main", "b/devcontainer-setup"] schedule: - cron: "16 2 * * 6"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeql.yml around lines 14 - 20, The CodeQL workflow currently only triggers on the non-default branch "b/devcontainer-setup" for push and pull_request, which will stop scanning once merged; update the workflow triggers in the CodeQL YAML (the on.push.branches and on.pull_request.branches entries) to include "main" (and any other long‑lived branches you want scanned) instead of or in addition to "b/devcontainer-setup", and ensure the schedule/baseline will run against the default branch by adding "main" to the branches list so CodeQL continues to run after merge.packages/agent/tools/notepad.ts-213-215 (1)
213-215:⚠️ Potential issue | 🟡 Minor"auto-pruned after 7 days" is misleading.
There's no automatic pruning;
notepad_prunemust be invoked explicitly. Consider rewording, e.g. "Entries are timestamped and can be pruned vianotepad_prune(default threshold: 7 days)." Otherwise the agent may rely on entries disappearing on their own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/notepad.ts` around lines 213 - 215, Update the description for the action identified by name "notepad_write_working" to remove the misleading phrase "auto-pruned after 7 days" and instead state that entries are timestamped and must be pruned explicitly via the "notepad_prune" tool (mention the default threshold of 7 days). Locate the action definition for notepad_write_working and replace the description text accordingly so callers know pruning is manual and reference the "notepad_prune" symbol.packages/agent/tools/notepad.ts-144-156 (1)
144-156:⚠️ Potential issue | 🟡 MinorDescription vs. schema mismatch (500 vs 2000 chars).
The tool description tells callers "Keep under 500 chars" while the schema enforces
.max(2000). Either tighten the schema to 500 (hard limit) or reword the description to match the actual cap so the LLM agent isn't given conflicting guidance. Today a model could reasonably submit up to 2000 chars and still receive only a soft warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/notepad.ts` around lines 144 - 156, The tool definition for notepad_write_priority has a description saying "Keep under 500 chars" but the Zod schema uses .max(2000) for content; update them to be consistent—either change the schema on the content field (in the notepad_write_priority tool definition) from .max(2000) to .max(500) to enforce the described hard limit, or alter the description to match .max(2000); ensure the content field's .describe text also aligns with the chosen limit so callers and the schema agree.packages/agent/tools/kilo.test.ts-191-194 (1)
191-194:⚠️ Potential issue | 🟡 MinorNarrow
resultbefore accessing nested fields.
client.call(...)returnsPromise<unknown>, soresult.result.toolsis not type-safe and violates the guideline to narrowunknownwith type guards. In TS strict mode this is aTS18046error and won't compile.- const result = await client.call("tools/list"); - expect(result.result.tools).toHaveLength(2); - expect(result.result.tools[0].name).toBe("query-docs"); + const result = (await client.call("tools/list")) as { + result: { tools: Array<{ name: string }> }; + }; + expect(result.result.tools).toHaveLength(2); + expect(result.result.tools[0].name).toBe("query-docs");A cleaner alternative is to introduce a Zod schema for the JSON-RPC envelope and
parsethe result inside the test (or inContext7Client.callitself), which would also satisfy the Zod guideline.As per coding guidelines: "Never use
anytype; useunknowninstead and narrow with type guards".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/kilo.test.ts` around lines 191 - 194, The test reads an untyped Promise result from client.call("tools/list") and directly accesses nested fields (result.result.tools), which is unsafe because client.call returns Promise<unknown); fix by narrowing the unknown before property access: create and use a Zod schema for the expected JSON-RPC envelope (or write an explicit type guard) to parse/validate the response returned by client.call, assign the parsed value to a properly typed variable, and then assert on parsed.result.tools and parsed.result.tools[0].name; alternatively implement this parsing inside Context7Client.call so tests receive a typed response.packages/mcp/context.ts-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorUse the recommended
CONTEXT7_API_KEYheader instead ofAuthorization: Bearer.Both
packages/mcp/context.ts(lines 5-7) andpackages/agent/tools/kilo.ts(lines 15-18) call the same Context7 endpoint athttps://mcp.context7.com/mcp, but use different auth headers: the first sendsAuthorization: Bearer <key>, while the second sends the customCONTEXT7_API_KEY: <key>header. The Context7 MCP endpoint supports both methods, butCONTEXT7_API_KEYis the primary and recommended approach. Alignpackages/mcp/context.tsto useCONTEXT7_API_KEYfor consistency and to follow the recommended auth scheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp/context.ts` around lines 5 - 7, The headers assignment in the context client uses Authorization: Bearer but should use the recommended custom header; update the headers object where headers is set (the code checking process.env.CONTEXT7_API_KEY) to send { CONTEXT7_API_KEY: process.env.CONTEXT7_API_KEY } instead of Authorization: `Bearer ${process.env.CONTEXT7_API_KEY}`, matching the approach used in kilo.ts and ensuring the MCP endpoint uses the primary recommended auth header.packages/agent/tools/kilo.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorRemove unused
node-fetchimport and use globalfetch.
node-fetchis imported but not declared as a dependency inpackage.json. The sibling modulepackages/agent/kilo/client.tsalready uses the globalfetch, and since the project targets Bun 1.2.14 (which has built-in fetch), switching toglobalThis.fetcheliminates the inconsistency and the undeclared dependency.Update the test mock strategy in
kilo.test.ts: replacemock.module("node-fetch", ...)with direct mocking ofglobalThis.fetchusingspyOn(globalThis, "fetch")or by assigningglobalThis.fetch = fetchMock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/kilo.ts` at line 1, Remove the unused node-fetch import from packages/agent/tools/kilo.ts and switch any usage to the global fetch (use globalThis.fetch or plain fetch) so the module relies on the runtime-provided fetch instead of the undeclared dependency; then update tests in kilo.test.ts to stop mocking "node-fetch" and instead mock the runtime fetch by using spyOn(globalThis, "fetch") or by assigning globalThis.fetch = fetchMock to mirror the change (search for the import line and any references to "node-fetch" mock.module to locate what to change).packages/agent/tools/github.ts-61-64 (1)
61-64:⚠️ Potential issue | 🟡 Minor
git add -Afailure is silently swallowed.The result of staging is discarded, so a non-zero
git add(e.g., index lock, permissions) won't surface here — the subsequent commit will fail with a less informative error. Mirror the pattern used for the other steps and short-circuit on failure.🛡️ Suggested fix
- // 2. Stage all changes - await sandbox.exec("git add -A", cwd, TIMEOUT_MS, { - signal: abortSignal, - }); + // 2. Stage all changes + const addRes = await sandbox.exec("git add -A", cwd, TIMEOUT_MS, { + signal: abortSignal, + }); + if (!addRes.success) { + return { + success: false, + error: `Failed to stage changes: ${addRes.stdout} ${addRes.stderr}`, + }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/github.ts` around lines 61 - 64, The git staging step using sandbox.exec("git add -A", cwd, TIMEOUT_MS, { signal: abortSignal }) currently discards its result; change it to capture the exec result, check for non-zero exitCode (or failure), and short-circuit the flow like the other steps by throwing or returning an error that includes stdout/stderr details and context ("git add -A" failed). Use the same result-handling pattern as the surrounding calls in this file so failures such as index locks or permissions surface immediately rather than causing a later, less informative commit error.packages/agent/tools/lsp.ts-74-97 (1)
74-97:⚠️ Potential issue | 🟡 MinorFragile substring match on
"not found".Branching error formatting on
message.includes("not found")will misclassify any unrelated error containing those words (e.g.,"Symbol not found at position","Document not found in workspace") as an installation/server-missing hint, hiding the real failure. Consider using a typed error (or error code/class) emitted from the LSP client manager so the install-hint path is selected deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/lsp.ts` around lines 74 - 97, The catch block in packages/agent/tools/lsp.ts currently branches on message.includes("not found") which is fragile; update the LSP client/manager to throw a deterministic error (e.g., a ServerNotInstalledError subclass or an error with a specific code like error.code === "SERVER_NOT_FOUND") and then change this catch to detect that specific type/code (check instanceof ServerNotInstalledError or error.code) instead of substring-matching the message; if you cannot change the manager right now, replace the loose includes("not found") with a much stricter pattern (e.g., a targeted regex) as a temporary fallback and document the new ServerNotInstalledError check around the existing operation/context variables used in the catch.apps/web/app/terms-of-service/page.tsx-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorWrong product name in user-facing copy.
The placeholder references
bl1nk-note, but this workspace isopen-agents(rootpackage.jsonline 2). Shipping this string to a public Terms-of-Service route would be visible to end users and mislabels the service. Also worth addingmetadata(title/description) so the page has an accurate<title>.📝 Proposed fix
+import type { Metadata } from "next"; + +export const metadata: Metadata = { + title: "Terms of Service", + description: "Terms of Service for open-agents.", +}; + export default function TermsOfServicePage() { return ( <div className="container mx-auto p-8 max-w-3xl pt-24"> <h1 className="text-3xl font-bold mb-6">Terms of Service</h1> <p className="mb-4 text-muted-foreground"> - By using bl1nk-note, you agree to these Terms of Service. This is a - placeholder page. Please update with your actual Terms of Service. + By using open-agents, you agree to these Terms of Service. This is a + placeholder page. Please update with your actual Terms of Service. </p> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/terms-of-service/page.tsx` around lines 5 - 8, Replace the incorrect product name in the paragraph that currently reads "By using bl1nk-note…" with the correct workspace product name "open-agents" and update the copy to a non-placeholder TOS line; also add an export named metadata (export const metadata = { title: "Terms of Service — open-agents", description: "Terms of Service for open-agents" }) to the page component (page.tsx) so the page has an accurate title and description in the HTML head.apps/web/components/ai-elements/code-block.tsx-384-418 (1)
384-418:⚠️ Potential issue | 🟡 MinorPossible "stuck on rawTokens" race when Shiki resolves between render and effect.
Flow on first render of a not-yet-cached
(code, language):
useMemo(Line 388) callshighlightCode(code, language)with no callback → cache miss → returnsnull,syncTokens = rawTokens. The fire-and-forget Shiki promise starts.- Render commits.
- The effect (Line 406) calls
highlightCode(code, language, cb). If the promise already resolved between step 1 and step 3 (warm highlighter, or a prior identical block),highlightCodereturns the cached result early at Line 195 — before theif (callback)block that registerscb. The subscriber is never added,setAsyncTokensis never called, and there is no other render trigger, so the component stays onrawTokensindefinitely.Two minimal fixes:
🛠️ Suggested fix
Option A — return-and-deliver: if
highlightCodefinds a cached result while a callback is provided, schedule the callback so the caller still receives it.const cached = tokensCache.get(tokensCacheKey); if (cached) { + if (callback) { + queueMicrotask(() => callback(cached)); + } return cached; }Option B — read the cache in the effect and seed
asyncTokensdirectly when a sync result is already available, instead of relying solely on the subscription.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/ai-elements/code-block.tsx` around lines 384 - 418, The race causes components to stay on rawTokens when highlightCode resolves between render and the effect; fix by seeding asyncTokens when a cached result already exists in the effect: inside the useEffect that currently calls highlightCode(code, language, cb) first read highlightCode(code, language) synchronously (or call a new API like highlightCodeSync) and if it returns a TokenizedCode immediately call setAsyncTokens(result) before registering the callback; keep the existing subscription behavior so new async results still update asyncTokens, and continue to use asyncKeyRef, setAsyncTokens, syncTokens and rawTokens as before.package.json-30-32 (1)
30-32:⚠️ Potential issue | 🟡 Minor
@vercel/analyticsversion mismatch between root andapps/web.
apps/web/package.json(line 39) pins@vercel/analytics: ^1.4.1and imports it inapps/web/app/layout.tsx. The rootpackage.json(line 31) now adds^2.0.1as a runtime dependency, creating a version mismatch in the dependency graph. The root is a private workspace orchestrator with no app code, so this runtime dep should not exist there.Either upgrade
apps/webto^2.0.1and remove the duplicate from root, or remove this addition entirely from the root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 30 - 32, There is a version mismatch for the dependency "@vercel/analytics" between the root package.json and apps/web; either remove the runtime dependency from the root or align versions by upgrading apps/web to ^2.0.1 and removing the root entry. To fix: edit the root package.json and delete the "@vercel/analytics" entry (preferred, since root is a private workspace orchestrator), or alternatively update apps/web/package.json to "^2.0.1" and adjust imports in apps/web/app/layout.tsx if any API changes exist, then run workspace install/lockfile update to keep the monorepo consistent.apps/web/components/ai-elements/code-block.tsx-144-148 (1)
144-148:⚠️ Potential issue | 🟡 MinorCache-key collision risk in
getTokensCacheKey.The key is built from
language + total length + first 100 + last 100characters of the source. Two distinct snippets that share the same prefix/suffix and length (very plausible for templated/generated code or diffed variants) will collide and the second snippet will render with the first snippet's tokens.A content hash (e.g., a quick FNV-1a / cyrb53 over the full string, or SubtleCrypto for browsers) is cheap relative to a Shiki tokenization and removes the collision class entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/ai-elements/code-block.tsx` around lines 144 - 148, getTokensCacheKey currently composes a key from language, length, and first/last 100 chars which can collide for different snippets; replace this with a deterministic content hash of the entire code string (e.g., cyrb53 or FNV-1a implementation or SubtleCrypto digest in browser) and include that hash in the returned key (e.g., `${language}:${code.length}:${hash}`) inside getTokensCacheKey so the cache key uniquely represents the full source while remaining fast and deterministic.package.json-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorRemove unused
@shelve/clidevDependency.
@shelve/cliis declared as a devDependency but is not referenced anywhere in the codebase—no npm scripts, bash scripts, imports, or configuration files use it. Remove it fromdevDependenciesto clean up the lockfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 34, Remove the unused devDependency entry "@shelve/cli" from devDependencies in package.json and update the lockfile by reinstalling dependencies (e.g., run npm install or yarn install) so the package is removed from node_modules and package-lock.json/yarn.lock; ensure no npm scripts or imports reference "@shelve/cli" before removing and commit the updated package.json and lockfile.
🧹 Nitpick comments (15)
packages/agent/tools/notepad.test.ts (2)
65-78: Avoid shadowing the importedmock.The arrow parameter
mockshadows themocksymbol imported frombun:teston line 1. It compiles, but obscures readability and could trip up someone adding amock(...)call inside this block later.♻️ Proposed fix
- }).forEach((mock) => mock.mockClear()); + }).forEach((m) => m.mockClear());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/notepad.test.ts` around lines 65 - 78, The parameter name "mock" in the forEach callback shadows the imported mock from bun:test; rename the callback parameter (e.g., to "m" or "entry") in the block that iterates over Object.values({ mockGetPriorityContext, mockGetWorkingMemory, mockGetManualSection, mockSetPriorityContext, mockAddWorkingMemoryEntry, mockAddManualEntry, mockPruneOldEntries, mockGetNotepadStats, mockFormatFullNotepad, mockGetWorktreeNotepadPath, mockEnsureOmcDir, mockValidateWorkingDirectory }) and call m.mockClear() (or entry.mockClear()) so the imported mock symbol remains unshadowed and readable.
119-200: Prefer name-based lookup over array indices.All handler tests reference
notepadTools[0]…notepadTools[5], which silently break if the export array is reordered (the registration test would still pass, but every handler test would target the wrong tool and produce confusing diffs). Consider building a name→tool map once and using it throughout.♻️ Proposed pattern
const { notepadTools } = await import("./notepad"); +const toolByName = Object.fromEntries( + notepadTools.map((t) => [t.name, t]), +); … - const result = await notepadTools[0].handler({ + const result = await toolByName.notepad_read.handler({ section: "all", workingDirectory: testRoot, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/notepad.test.ts` around lines 119 - 200, Tests currently index into the exported notepadTools array (notepadTools[0] … notepadTools[5]) which is brittle if the export order changes; create a stable name→tool map once (e.g. const toolsByName = Object.fromEntries(notepadTools.map(t => [t.name, t])) ) and then update each test to call toolsByName['<toolName>'].handler(...) instead of notepadTools[index].handler so tests target tools by their unique name (referencing the notepadTools array and each tool's handler property to locate replacements).packages/agent/tools/kilo.test.ts (2)
28-30: Top-levelawait importworks in Bun but is fragile.The dynamic
await import("./kilo")on line 29 is necessary becausemock.modulemust be registered before the source module evaluates. This is fine in Bun, but it requires the test file to be treated as an ESM top-level-await module by every tool that touches it (type-check, IDE, future test runners). Consider usingmock.module(...)followed by abeforeAll(async () => { ({ Context7Client } = await import("./kilo")); })pattern — easier to read and avoids module-level side effects ordering pitfalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/kilo.test.ts` around lines 28 - 30, Replace the top-level dynamic import with a pattern that registers mock.module first and performs the import inside a lifecycle hook: declare a top-level mutable binding for Context7Client, call mock.module(...) before test evaluation, then in a beforeAll(async () => { ({ Context7Client } = await import("./kilo")); }) import the module so the mock is registered before the module evaluates; update any tests to use the Context7Client binding after beforeAll completes.
156-165: Tighten the malformed-JSON assertion.
rejects.toThrow()with no argument passes for any thrown error, so this test would still pass ifcallthrew for an unrelated reason (e.g., a network error introduced by a future refactor). Assert on the underlyingSyntaxError(or a substring of its message) to keep the test pinned to the intended behavior.- await expect(client.call("tools/list")).rejects.toThrow(); + await expect(client.call("tools/list")).rejects.toThrow(SyntaxError);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/kilo.test.ts` around lines 156 - 165, The test "should handle malformed JSON response" currently uses await expect(client.call("tools/list")).rejects.toThrow() which is too broad; change the assertion to expect a JSON parse failure specifically by asserting the thrown error is a SyntaxError or matches the parser message (e.g., use rejects.toThrow(SyntaxError) or rejects.toThrow(/Unexpected token|Unexpected end of JSON/)) so the test fails only when parsing succeeds but other unrelated errors occur; update the assertion around the client.call("tools/list") invocation in kilo.test.ts accordingly.packages/agent/kilo/client.ts (2)
1-98: Use Zod schemas for the request/response models and derive TS types viaz.infer.The coding guideline requires Zod schemas for validation with types derived via
z.infer. The current declarations are plain TS types, which means responses fromhttps://api.kilo.aiaren't validated at runtime — a malformed payload would surface as undefined fields deep in callers rather than a clear error at the boundary.Recommended: define
chatCompletionRequestSchema,chatCompletionResponseSchema,modelSchema, etc., andexport type ChatCompletionRequest = z.infer<typeof chatCompletionRequestSchema>. Then haverequest<T>accept a parser (z.ZodType<T>) and callschema.parse(await response.json())before returning.This also gives a natural place to validate the error envelope in the catch block above.
As per coding guidelines: "Use Zod schemas for validation and derive types with
z.infer".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/kilo/client.ts` around lines 1 - 98, The file exposes plain TS types (e.g., ChatCompletionRequest, ChatCompletionResponse, Model) but must use Zod schemas and derive types via z.infer; create zod schemas like chatCompletionRequestSchema, chatCompletionResponseSchema, modelSchema (and any ContentPart/Tool/ToolCall schemas), export types as export type ChatCompletionRequest = z.infer<typeof chatCompletionRequestSchema>, and update the request<T> utility to accept a parser argument (z.ZodType<T>) and call parser.parse(await response.json()) before returning; also use a Zod schema to validate the error envelope in the request catch/response error handling so malformed responses fail fast.
100-169: Add a request timeout /AbortSignalto outbound HTTP calls.
request<T>callsfetchwith no timeout. A hung connection toapi.kilo.aiwill block the caller indefinitely, which is a reliability hazard for any request-thread caller. Consider accepting anAbortSignal(or applyingAbortSignal.timeout(...)) and threading it intofetch.- private async request<T>(path: string, options?: RequestInit): Promise<T> { + private async request<T>( + path: string, + options?: RequestInit & { timeoutMs?: number }, + ): Promise<T> { + const { timeoutMs = 30_000, ...rest } = options ?? {}; const response = await fetch(`${this.baseUrl}${path}`, { - ...options, + ...rest, + signal: rest.signal ?? AbortSignal.timeout(timeoutMs), headers: { Authorization: `Bearer ${this.apiKey}`, "Content-Type": "application/json", - ...options?.headers, + ...rest.headers, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/kilo/client.ts` around lines 100 - 169, Update KiloClient to support request timeouts by accepting and using an AbortSignal (or a timeout in milliseconds) and passing it into fetch from request; modify the method signature for request(path: string, options?: RequestInit, signal?: AbortSignal | number) or accept an options.signal and, if given a numeric timeout, create an AbortController and call setTimeout to abort it (or use AbortSignal.timeout when available), then pass controller.signal into fetch; thread this through the public methods chatCompletions, fimCompletions, listModels, and listProviders so callers can provide a signal or timeout; ensure any created timers/controllers are cleaned up after the request completes.packages/agent/tools/delegate.ts (1)
137-139: One failed subagent loses results from successful peers.
Promise.allrejects on the first error, so a single subagent failure propagates out ofexecuteand the partial snapshot (including completed peers) is never yielded. ConsiderPromise.allSettledand recording failures on the per-taskstateso partially successful runs still surface useful results.♻️ Sketch
- const promises = tasks.map((t, i) => runTask(t, i)); - await Promise.all(promises); + const settled = await Promise.allSettled( + tasks.map((t, i) => runTask(t, i)), + ); + settled.forEach((r, i) => { + if (r.status === "rejected") { + const state = taskStates[i]; + if (state) { + state.final = [ + { + role: "assistant", + content: + r.reason instanceof Error ? r.reason.message : String(r.reason), + } as ModelMessage, + ]; + } + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/delegate.ts` around lines 137 - 139, The current use of Promise.all in execute causes one failing subagent to abort and drop results; switch to Promise.allSettled for the parallel run in execute (replace Promise.all(promises) with Promise.allSettled(promises)), then iterate the settled results and for each index map back to tasks/runTask and record success or failure on the per-task state object (e.g., set state.error or state.status for failed entries and state.result for fulfilled ones) so you still yield the partial snapshot of completed peers; ensure runTask continues to update state for in-progress completion and that execute aggregates settled outcomes before returning/yielding.packages/agent/tools/tools.test.ts (1)
472-476: TightenMock<unknown>to the actual exec signature for safer assertions.
Mock<unknown>provides no typing for arguments/return value, weakening thetoHaveBeenCalledWith(...)assertions below (TypeScript can't catch arity drift). Use the sandboxexecshape explicitly.- let mockSandboxExec: Mock<unknown>; - let mockSandbox: { - workingDirectory: string; - exec: Mock<unknown>; - }; + type ExecMock = Mock< + ( + command: string, + cwd?: string, + timeoutMs?: number, + options?: { signal?: AbortSignal }, + ) => Promise<{ success: boolean; exitCode: number; stdout: string; stderr: string }> + >; + let mockSandboxExec: ExecMock; + let mockSandbox: { workingDirectory: string; exec: ExecMock };As per coding guidelines: "Never use
anytype; useunknowninstead and narrow with type guards" — narrowing the mock signature is the natural follow-through of that rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/tools.test.ts` around lines 472 - 476, The mocks are declared as Mock<unknown>, which loses the actual exec signature and weakens toHaveBeenCalledWith assertions; change mockSandboxExec and mockSandbox to use the real exec function type instead (e.g. Mock<(...args: Parameters<typeof <module>.exec>) => ReturnType<typeof <module>.exec>>), or derive the signature via Parameters/ReturnType of the real sandbox exec function so mockSandbox.exec matches the real exec signature for safer assertions; update references to mockSandboxExec and mockSandbox.exec accordingly.packages/agent/tools/github.ts (1)
109-121: AddoutputSchemawith Zod discriminated union for stable output contract.The
executefunction returns three structurally different shapes ({ success: true, message, prUrl },{ success: true, message, errorDetails },{ success: false, error }). Following the pattern established by other tools in the codebase (seefetch.tsanddelegate.ts), define anoutputSchemausing a Zod discriminated union onsuccessto ensure downstream consumers can safely narrow the return type and detect shape changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/github.ts` around lines 109 - 121, The execute function currently returns three different shapes; add an outputSchema using Zod as a discriminated union on the `success` boolean to lock the output contract: define an exported `outputSchema` (importing z from 'zod') that is z.discriminatedUnion('success', [ z.object({ success: z.literal(true), message: z.string(), prUrl: z.string().optional(), errorDetails: z.string().optional() }), z.object({ success: z.literal(false), error: z.string() }) ]) or split true-cases into two distinct true-variants if you prefer explicit keys, then validate/annotate the function's return against `outputSchema` and export it alongside `execute` so callers can safely narrow the return shape (refer to `execute` in github.ts and follow patterns used in `fetch.ts`/`delegate.ts`).packages/agent/tools/lsp.test.ts (3)
196-206:mockparameter shadows thebun:testimport.Inside
forEach((mock) => mock.mockClear()), the parameter name shadows themocksymbol imported on line 1. It happens to work here, but is confusing and a footgun if the closure later needs the importedmock. Rename the parameter (e.g.,mormockedFn), or just iterate over an array literal instead ofObject.values({...}).♻️ Proposed change
- Object.values({ - mockFormatHover, - mockFormatLocations, - mockFormatDocumentSymbols, - mockFormatWorkspaceSymbols, - mockFormatDiagnostics, - mockFormatCodeActions, - mockFormatWorkspaceEdit, - mockCountEdits, - }).forEach((mock) => mock.mockClear()); + for (const m of [ + mockFormatHover, + mockFormatLocations, + mockFormatDocumentSymbols, + mockFormatWorkspaceSymbols, + mockFormatDiagnostics, + mockFormatCodeActions, + mockFormatWorkspaceEdit, + mockCountEdits, + ]) { + m.mockClear(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/lsp.test.ts` around lines 196 - 206, The forEach callback parameter shadows the imported mock symbol; change the closure in the block that iterates over Object.values({ mockFormatHover, mockFormatLocations, mockFormatDocumentSymbols, mockFormatWorkspaceSymbols, mockFormatDiagnostics, mockFormatCodeActions, mockFormatWorkspaceEdit, mockCountEdits }) so it doesn't use the name "mock" (e.g., use "m" or "mockedFn") or replace Object.values({...}) with an explicit array [mockFormatHover, mockFormatLocations, ...] and then call .forEach(m => m.mockClear()) to avoid shadowing the imported mock from bun:test.
208-227: Index-based tool lookup is brittle.Every handler test uses positional access (
lspTools[0],lspTools[5],lspTools[7],lspTools[9],lspTools[10], etc.) tied to the order inlspTools. Reordering or inserting a tool inpackages/agent/tools/lsp.ts:611-624would silently re-route assertions to the wrong handler — for instance, the "should handle unsupported file type" test at lines 417-428 would pass even while exercising a different tool. Resolve tools by name to make tests order-independent.♻️ Proposed change
+const getTool = (name: string) => { + const tool = lspTools.find((t) => t.name === name); + if (!tool) throw new Error(`Tool ${name} not found`); + return tool; +}; @@ - const result = await lspTools[0].handler({ + const result = await getTool("lsp_hover").handler({ file: "test.ts", line: 1, character: 5, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/lsp.test.ts` around lines 208 - 227, Tests rely on positional access into the lspTools array (e.g., lspTools[0], lspTools[5], lspTools[7], lspTools[9], lspTools[10]) which is brittle; update those tests to resolve tools by name instead of index by using a lookup like finding the element in lspTools where t.name === "<tool_name>" (keep the existing overall array assertions if desired) and use the found tool's handler for assertions, ensuring every test references tools by their unique names (e.g., "lsp_hover", "lsp_diagnostics", "lsp_servers", "lsp_rename", "lsp_code_actions") rather than fixed positions.
190-447: Missing handler-level tests for three tools.The suite asserts the array contains 12 tools (line 210) but only exercises the handlers of 9. There are no tests for:
lsp_diagnostics_directory— therunDirectoryDiagnosticsmock at lines 179-185 is set up but never invoked.lsp_prepare_renamelsp_code_action_resolve— particularly worth covering given the resolve concern flagged inlsp.ts.Worth adding at least happy-path + one error path for each.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/agent/tools/lsp.test.ts` around lines 190 - 447, The test suite claims 12 LSP tools but never exercises handlers for lsp_diagnostics_directory, lsp_prepare_rename, and lsp_code_action_resolve; add unit tests that call the corresponding handlers (locate by lspTools.find(t => t.name === "lsp_diagnostics_directory") / "lsp_prepare_rename" / "lsp_code_action_resolve") to cover a successful case and an error case for each: for lsp_diagnostics_directory invoke the handler and assert runDirectoryDiagnostics (or its mock) is called and the response content contains the expected diagnostics summary, then simulate runDirectoryDiagnostics throwing or returning [] and assert isError / “No diagnostics” message; for lsp_prepare_rename call the handler and assert the underlying LSP client prepareRename mock is invoked and the returned range/name is formatted, then simulate the client returning null/error and assert proper error response; for lsp_code_action_resolve call the handler to resolve an action (assert mockResolveCodeAction or the client.resolveCodeAction was called and formatted via mockFormatWorkspaceEdit), then simulate resolve failing/throwing and assert the tool returns an isError result with the expected message.package.json (1)
28-28:clean:workspaceis bash-only.Invoking via
bash scripts/clean-workspace.shwon't work on native Windows shells (PowerShell/cmd). If Windows is a supported dev environment, consider porting to a Bun/Node script (bun run scripts/clean-workspace.ts) to remain portable — this also aligns with the project's "Run project checks through package scripts ... Preferbun run <script>over invoking tool binaries directly" convention.Based on learnings: "Run project checks through package scripts (e.g.,
bun run ci,bun run --cwd apps/web db:check). Preferbun run <script>over invoking tool binaries directly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 28, The "clean:workspace" npm script currently invokes a bash-only entry ("bash scripts/clean-workspace.sh") which breaks on Windows; port the shell script at scripts/clean-workspace.sh to a cross-platform Bun/Node script (e.g., scripts/clean-workspace.ts) and update the "clean:workspace" npm script to run it via Bun (e.g., "bun run scripts/clean-workspace.ts" or a short alias like "bun run clean:workspace" that calls the TS entry). Ensure the new TypeScript script uses Node/Bun APIs or cross-platform libs (fs, rimraf/fast-glob or built-in recursive rm) instead of shell commands, and keep the original cleanup behavior and exit codes so callers of the clean:workspace script see identical results.apps/web/components/ai-elements/code-block.tsx (1)
1-564: Split this module per the colocated-concerns guideline.This file currently bundles five distinct concerns: (1) Shiki highlighter + token cache + subscriber registry (
getHighlighter,highlightCode,getTokensCacheKey,createRawTokens), (2) keyed-token transformation +TokenSpan/LineSpanrendering, (3) the memoizedCodeBlockBody+CodeBlockContentconsumer, (4)CodeBlockCopyButton, and (5)CodeBlockLanguageSelector*wrappers. Splitting them into colocated files (e.g.code-block/highlighter.ts,code-block/tokens.tsx,code-block/copy-button.tsx,code-block/language-selector.tsx, plus the publicindex.tsx) keeps each file focused and makes the highlight cache easier to test in isolation.Based on learnings: "Applies to **/*.{ts,tsx} : Prefer creating a new colocated file for distinct concerns (components, hooks, utilities, schemas, data-access helpers). Before adding code, assess if the behavior is a separate concern".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/ai-elements/code-block.tsx` around lines 1 - 564, The file bundles multiple distinct concerns; split it into colocated modules and move related symbols: extract the Shiki/highlight logic (getHighlighter, highlightCode, getTokensCacheKey, createRawTokens, highlighterCache, tokensCache, subscribers) into code-block/highlighter.ts, extract token/key helpers and render primitives (addKeysToTokens, KeyedToken/KeyedLine, TokenSpan, LineSpan) into code-block/tokens.tsx, move CodeBlockBody and CodeBlockContent into code-block/body.tsx, move CodeBlockCopyButton and its context usage into code-block/copy-button.tsx, and move the LanguageSelector wrappers (CodeBlockLanguageSelector*, CodeBlockLanguageSelectorTrigger/Value/Content/Item) into code-block/language-selector.tsx; keep only the public CodeBlock, CodeBlockContainer/Header/Title/Filename/Actions and exports in index.tsx and update imports/exports accordingly so each file has a single responsibility and tests can import the highlighter in isolation.apps/web/components/ui/select.tsx (1)
5-5: Consolidate Radix UI imports to the umbrella package.The change in
select.tsximports fromradix-uiumbrella, but 13 other UI component files (dialog,switch,tabs,tooltip,sheet,sidebar,separator,scroll-area,popover,label,dropdown-menu,button-group,avatar) still import from individual@radix-ui/react-*packages. Meanwhile,apps/web/package.jsonretains both the scoped packages and the umbrella (radix-ui^1.4.3), causing both to be bundled together and increasing the JS payload unnecessarily.Migrate all remaining UI primitives to the umbrella package and remove the individual
@radix-ui/react-*dependencies frompackage.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/components/ui/select.tsx` at line 5, Several UI components still import individual Radix packages while select.tsx was migrated to the umbrella "radix-ui", causing duplicate packages to be bundled; update the other primitives to match. For each component file listed (dialog, switch, tabs, tooltip, sheet, sidebar, separator, scroll-area, popover, label, dropdown-menu, button-group, avatar) replace imports from "@radix-ui/react-*" with the umbrella "radix-ui" and keep the same exported symbols (e.g., Dialog, Switch, Tabs, Tooltip, Sheet, Sidebar, Separator, ScrollArea, Popover, Label, DropdownMenu, ButtonGroup, Avatar or their primitve aliases like DialogPrimitive) so code references (component names and any primitives) remain unchanged; finally remove the now-unused "@radix-ui/react-*" entries from apps/web/package.json and run a reinstall/build to verify no import paths break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b082c68b-457a-4aba-a20b-d95c237f2732
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/codeql.yml.gitigapps/web/app/terms-of-service/page.tsxapps/web/components/ai-elements/code-block.tsxapps/web/components/ui/button.tsxapps/web/components/ui/select.tsxapps/web/gitapps/web/package.jsonpackage.jsonpackages/agent/kilo/client.tspackages/agent/open-agent.tspackages/agent/tools/delegate.tspackages/agent/tools/github.tspackages/agent/tools/index.tspackages/agent/tools/kilo.test.tspackages/agent/tools/kilo.tspackages/agent/tools/lsp.test.tspackages/agent/tools/lsp.tspackages/agent/tools/notepad.test.tspackages/agent/tools/notepad.tspackages/agent/tools/tools.test.tspackages/mcp/context.tspackages/prompt/bot.tspackages/sandbox/vercel/sandbox.tsrenovate.jsonscripts/clean-workspace.sh
|
CodeAnt AI finished reviewing your PR. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
User description
This pull request includes:
These changes improve code consistency and test coverage without altering functionality.
Summary by CodeRabbit
New Features
Improvements
CodeAnt-AI Description
Add code blocks, agent tools, and notepad support for the web app
What Changed
Impact
✅ Clearer code snippets✅ Faster agent handoffs✅ Fewer manual commit and PR steps🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.