feat(kanban): add agent-neutral worker bridge - #508
Conversation
Patchbot
CI Checks (1/1 passed)
Updated 2026-07-14T07:32:16.276Z · Run #29314876387 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds worker-aware Kanban routing, agent gateway contracts and handlers, granular agent tools, an authenticated HTTP/MCP bridge, MCP tool annotations, and documentation for scheduled Codex Desktop and ChatGPT Work automation. ChangesKanban automation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant Agent
participant KanbanBridge
participant Gateway
participant Repository
Scheduler->>Agent: Start scheduled worker run
Agent->>KanbanBridge: Request status and pick-next
KanbanBridge->>Gateway: kanban.status and kanban.agent.pickNext
Gateway->>Repository: Select card for worker
Repository-->>Gateway: Card or null pickup
Gateway-->>KanbanBridge: RPC response
KanbanBridge-->>Agent: Status and card result
Agent->>KanbanBridge: Complete or handoff with evidence
KanbanBridge->>Gateway: kanban.agent.complete or kanban.agent.handoff
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the daisy-kanban-operator skill and a private HTTP bridge server (daisy-kanban-bridge) to expose granular Kanban tools and agent-neutral task pickup for Codex Desktop and ChatGPT Work, including worker-specific routing labels. Feedback on these changes highlights several issues: flat PATCH requests to /v1/cards/:cardId will fail validation due to forbidden properties in the fallback updates object; rate limiting using req.socket.remoteAddress will fail behind reverse proxies; the rate limiter's buckets Map has a potential memory leak; and kanban_archive_card is incorrectly marked as idempotent despite requiring an expectedVersion check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (method === "kanban.cards.update") { | ||
| const updates = params.updates; | ||
| params.updates = isRecord(updates) ? updates : params; | ||
| } |
There was a problem hiding this comment.
Flat PATCH requests to /v1/cards/:cardId will fail validation at the gateway. When updates is not explicitly provided in the request body, the bridge falls back to using params as the updates object. However, params contains cardId and expectedVersion (and potentially boardId), which are strictly forbidden inside the updates object by the gateway's KanbanCardsUpdateParamsSchema (which has additionalProperties: false).
if (method === "kanban.cards.update") {
const updates = params.updates;
if (isRecord(updates)) {
params.updates = updates;
} else {
const { cardId, expectedVersion, boardId, ...rest } = params;
params.updates = rest;
}
}References
- When processing untyped JSON objects (e.g., parsed from external sources), use a type guard like
isRecordto verify that entries are non-null objects before accessing their properties, rather than checking individual properties for null/undefined.
| mutation: boolean, | ||
| importRequest: boolean, | ||
| ): boolean { | ||
| const key = req.socket.remoteAddress ?? "unknown"; |
There was a problem hiding this comment.
The rate limiter uses req.socket.remoteAddress as the unique key for rate limit buckets. If the bridge is deployed behind a reverse proxy, load balancer, or HTTPS reverse tunnel (as mentioned in the documentation), remoteAddress will always resolve to the proxy's loopback IP (e.g., 127.0.0.1). This will cause all clients to share a single rate limit bucket, leading to accidental rate-limiting of legitimate users. Consider checking the x-forwarded-for header to identify the real client IP.
const key = (req.headers["x-forwarded-for"] as string)?.split(",")[0].trim() ?? req.socket.remoteAddress ?? "unknown";| function rateLimit( | ||
| req: IncomingMessage, | ||
| buckets: Map<string, RateBucket>, | ||
| now: number, | ||
| mutation: boolean, | ||
| importRequest: boolean, | ||
| ): boolean { | ||
| const key = req.socket.remoteAddress ?? "unknown"; | ||
| const existing = buckets.get(key); | ||
| const bucket = !existing || now - existing.startedAt >= WINDOW_MS | ||
| ? { startedAt: now, reads: 0, writes: 0, imports: 0 } | ||
| : existing; | ||
| if (mutation) bucket.writes += 1; | ||
| else bucket.reads += 1; | ||
| if (importRequest) bucket.imports += 1; | ||
| buckets.set(key, bucket); | ||
| return bucket.reads <= READ_LIMIT && bucket.writes <= WRITE_LIMIT && bucket.imports <= 5; | ||
| } |
There was a problem hiding this comment.
The buckets Map grows indefinitely with every unique client IP address, creating a potential memory leak over long server runtimes. Since there is no mechanism to prune expired or inactive buckets from the map, a large number of unique IPs could eventually exhaust server memory. Consider periodically pruning expired entries (e.g., when the map size exceeds a threshold).
function rateLimit(
req: IncomingMessage,
buckets: Map<string, RateBucket>,
now: number,
mutation: boolean,
importRequest: boolean,
): boolean {
const key = (req.headers["x-forwarded-for"] as string)?.split(",")[0].trim() ?? req.socket.remoteAddress ?? "unknown";
if (buckets.size > 1000) {
for (const [ip, b] of buckets.entries()) {
if (now - b.startedAt >= WINDOW_MS) {
buckets.delete(ip);
}
}
}
const existing = buckets.get(key);
const bucket = !existing || now - existing.startedAt >= WINDOW_MS
? { startedAt: now, reads: 0, writes: 0, imports: 0 }
: existing;
if (mutation) bucket.writes += 1;
else bucket.reads += 1;
if (importRequest) bucket.imports += 1;
buckets.set(key, bucket);
return bucket.reads <= READ_LIMIT && bucket.writes <= WRITE_LIMIT && bucket.imports <= 5;
}| return { | ||
| readOnlyHint: readOnly.has(name), | ||
| destructiveHint: name === "kanban_archive_card", | ||
| idempotentHint: readOnly.has(name) || name === "kanban_archive_card", |
There was a problem hiding this comment.
kanban_archive_card is marked as idempotent in toolAnnotations. However, the tool requires an expectedVersion parameter for optimistic concurrency control. A second attempt to archive the same card with the same expectedVersion will fail with a version conflict error. Therefore, this tool is not truly idempotent and should not have idempotentHint set to true.
idempotentHint: readOnly.has(name),There was a problem hiding this comment.
Pull request overview
This PR adds an agent-neutral Kanban “worker” routing layer (Codex vs Work) and introduces a local authenticated HTTP/MCP bridge so both the private Site and Secure MCP Tunnel can interact with the same Kanban gateway surface without direct MongoDB access.
Changes:
- Add worker routing labels + normalization and update repository pickup logic to filter eligible cards by worker.
- Introduce agent-neutral gateway RPC method aliases (
kanban.agent.*) while preserving legacy Codex method names. - Add a loopback-only authenticated REST/MCP bridge plus granular Kanban MCP tools, along with contract/tests and operator/scheduling docs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mcp/daisy-kanban-mcp/server.ts | Exports gateway caller and adds MCP tool annotations metadata. |
| src/mcp/daisy-kanban-bridge/server.ts | New authenticated loopback REST + MCP bridge with routing and rate limits. |
| src/mcp/daisy-kanban-bridge/server.test.ts | Adds basic bridge auth/routing/body-limit tests. |
| src/kanban/types.ts | Introduces worker label constants/types and label normalization with conflict detection. |
| src/kanban/repository.ts | Normalizes persisted labels and adds worker-aware pickNextCard selection. |
| src/kanban/repository.test.ts | Tests worker label normalization behavior. |
| src/gateway/server-methods/kanban.ts | Adds kanban.agent.* handlers and aliases legacy Codex method names. |
| src/gateway/protocol/schema/types.ts | Exposes agent-neutral Kanban schema types. |
| src/gateway/protocol/schema/protocol-schemas.ts | Registers the new agent-neutral Kanban schemas. |
| src/gateway/protocol/schema/kanban.ts | Adds worker schema + agent-neutral pickNext schemas; aliases Codex pickNext to them. |
| src/gateway/protocol/schema/kanban.contract.test.ts | Adds contract coverage for agent-neutral pickup params + defaults. |
| src/gateway/protocol/index.ts | Adds Ajv validators for agent-neutral Kanban params. |
| src/gateway/method-scopes.ts | Grants operator write scope to the new agent-neutral Kanban methods. |
| src/gateway/method-scopes.test.ts | Updates scope tests to include agent-neutral methods. |
| src/agents/tools/kanban-tool.ts | Adds granular Kanban tools and extends pick schema with worker/agent identity fields. |
| skills/daisy-kanban-operator/SKILL.md | New operator skill documenting routing/identity/safety workflows. |
| docs/automation/daisy-kanban-workers.md | Documents worker schedule configuration and bridge environment requirements. |
| docs/automation/codex-desktop-kanban.md | Updates automation docs to use new operator skill and worker routing. |
| package.json | Adds a kanban:bridge script entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const callGateway = deps?.callGatewayTool ?? callGatewayTool; | ||
| const invoke = async (method: string, params: Record<string, unknown>) => | ||
| jsonResult(await callGateway(method, readGatewayOpts(params), params)); |
| return { | ||
| readOnlyHint: readOnly.has(name), | ||
| destructiveHint: name === "kanban_archive_card", | ||
| idempotentHint: readOnly.has(name) || name === "kanban_archive_card", | ||
| openWorldHint: false, | ||
| }; |
| const server = createServer(async (req, res) => { | ||
| const url = new URL(req.url ?? "/", `http://${req.headers.host ?? DEFAULT_HOST}`); | ||
| const parts = routeParts(url.pathname); | ||
| const isMcp = url.pathname === MCP_PATH; |
| async function readBody(req: IncomingMessage, limit: number): Promise<unknown> { | ||
| const contentLength = Number(req.headers["content-length"] ?? 0); | ||
| if (Number.isFinite(contentLength) && contentLength > limit) { | ||
| throw Object.assign(new Error("request body exceeds limit"), { bridgeCode: "rate-limit" }); | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 909ada74ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function routeParts(pathname: string): string[] { | ||
| return pathname.split("/").filter(Boolean).map((part) => decodeURIComponent(part)); |
There was a problem hiding this comment.
When an unauthenticated request contains malformed percent-encoding, such as /v1/cards/%E0%A4%A, this decodeURIComponent call throws before the authorization check and before the route handler's try block. That leaves the async request listener rejected instead of returning a JSON 400/404, and under default Node unhandled-rejection behavior it can terminate the bridge process; decode failures should be caught and mapped to a normal error response.
Useful? React with 👍 / 👎.
| } | ||
| if (method === "kanban.cards.update") { | ||
| const updates = params.updates; | ||
| params.updates = isRecord(updates) ? updates : params; |
There was a problem hiding this comment.
When PATCH /v1/cards/:id receives a flat REST body without an updates object, for example { "expectedVersion": 1, "title": "New" }, this fallback assigns params.updates to params itself and creates a circular object. The gateway caller then cannot serialize the request frame, so the bridge returns an internal failure instead of applying the flat update or cleanly rejecting it; build a separate updates object rather than pointing it back at the full params object.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/mcp/daisy-kanban-mcp/server.ts (1)
262-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded tool-name list risks drifting from the actual tool definitions.
toolAnnotationsre-derives read-only/destructive hints from a name list maintained here, separate from each tool's own definition inkanban-tool.ts. If a future granular tool is added or renamed there without updating this list, MCP clients get an incorrectreadOnlyHint/destructiveHint— which can affect whether a client asks for user confirmation before an irreversible action.Consider carrying a
readOnly/destructiveflag on each tool definition itself (increateKanbanTools) and deriving these hints from that metadata instead of a name-based lookup here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/daisy-kanban-mcp/server.ts` around lines 262 - 276, The toolAnnotations function hardcodes tool names separately from their definitions, risking stale MCP hints. Add readOnly/destructive metadata to each tool definition created by createKanbanTools, then have toolAnnotations derive readOnlyHint, destructiveHint, and idempotentHint from that metadata instead of maintaining a separate name-based Set.src/mcp/daisy-kanban-bridge/server.ts (2)
59-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse constant-time comparison for bridge tokens.
bearer(req) === expectedshort-circuits on the first differing byte, leaking timing information about the secret. Given the bridge may be exposed over a tunnel (per PR objectives), prefercrypto.timingSafeEqualguarded by a length check.🔒 Proposed fix
+import { timingSafeEqual } from "node:crypto"; + +function safeEqual(a: string, b: string): boolean { + const bufA = Buffer.from(a); + const bufB = Buffer.from(b); + return bufA.length === bufB.length && timingSafeEqual(bufA, bufB); +} + function authorized(req: IncomingMessage, expected: string | undefined): boolean { - return Boolean(expected && bearer(req) === expected); + const token = bearer(req); + return Boolean(expected && token && safeEqual(token, expected)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/daisy-kanban-bridge/server.ts` around lines 59 - 66, Update authorized to compare bridge tokens with crypto.timingSafeEqual after verifying equal byte lengths, returning false for missing or mismatched lengths while preserving bearer extraction through bearer. Convert both token strings to compatible byte buffers before comparison and import the required crypto utility.
138-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
mapUpstreamErrornever produces theauthorizationcode it declares.
errorBody's type union includes"authorization"(Lines 43-57), but none of the regexes here match authorization/scope-denial messages from the gateway — those will fall through to the generic 500"internal"bucket, hiding a 403-class failure behind an opaque server error. Also, broad terms likerequiredandnot foundrisk misclassifying unrelated messages given no anchoring/word boundaries.♻️ Add an authorization branch
if (/version conflict/i.test(message)) { return { status: 409, body: errorBody("version-conflict", "The card changed; reread it and retry with its current version.") }; } + if (/forbidden|not authorized|insufficient scope|permission denied/i.test(message)) { + return { status: 403, body: errorBody("authorization", "The DAISy gateway rejected this request due to insufficient authorization.") }; + } if (/unavailable|timeout|closed|gateway/i.test(message)) {Since the exact wording of scope-denial errors is defined in the gateway/method-scopes layer, please confirm the actual message text used there so the regex matches it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/daisy-kanban-bridge/server.ts` around lines 138 - 153, Update mapUpstreamError to recognize the exact authorization/scope-denial message emitted by the gateway or method-scopes layer and return status 403 with errorBody code "authorization". Confirm and use that upstream message text when defining the regex, and tighten the existing validation and not-found patterns with appropriate boundaries or anchoring to avoid classifying unrelated errors.src/kanban/repository.ts (1)
1683-1704: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWorker routing filter is correct; consider indexing
labelsfor this hot pickup path.The
workerFilterlogic correctly distinguishes "work" (explicitworker:work/worker:anylabel required) from the codex/default path (unlabeled,worker:codex, orworker:any). No functional issue. Sincelabelsisn't part ofKANBAN_INDEX_DEFINITIONS.cards, this$or/$infilter is applied in-memory after theboardId+readyForCodexindex prefix narrows the candidate set — likely fine at typical board sizes, but worth keeping in mind if pickup polling volume or card counts grow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kanban/repository.ts` around lines 1683 - 1704, Consider adding a suitable index for the cards collection’s labels field in KANBAN_INDEX_DEFINITIONS.cards to support the workerFilter used by pickNextCard, while preserving the existing worker-routing logic and index definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agents/tools/kanban-tool.ts`:
- Around line 551-552: Update the invoke flow to read gateway options from the
original raw args rather than the RPC payload, preserving gatewayUrl,
gatewayToken, and timeoutMs for all granular tools including kanban_status. Keep
passing only the RPC-shaped params to callGateway, and adjust invoke’s callers
around buildReadRequest, buildCardCreateParams, and buildWriteRequest
accordingly.
In `@src/mcp/daisy-kanban-bridge/server.ts`:
- Around line 171-185: Update the isMcp request branch to invoke rateLimit()
before processing MCP requests, matching the /v1/* flow. Adjust readBody() to
use the import-specific larger limit when bridgeCode identifies an import tool,
otherwise retain NORMAL_BODY_LIMIT. Align the MCP catch handling with the /v1/*
path so oversized bodies return 413 while other invalid requests remain 400.
- Around line 202-227: Update the kanban.cards.update fallback in the
route-dispatch logic so flat PATCH bodies use the original validated body record
rather than the mutated params object when no updates wrapper is present.
Preserve wrapped updates unchanged, while ensuring the fallback excludes
route-added cardId from the updates payload and cannot create a circular
reference.
- Around line 68-91: Update readBody so exceeding the limit during for-await
does not implicitly destroy req before the caller can send its error response;
explicitly drain the remaining request body or close the connection in a
controlled manner, while preserving the existing rate-limit error and JSON
parsing behavior.
---
Nitpick comments:
In `@src/kanban/repository.ts`:
- Around line 1683-1704: Consider adding a suitable index for the cards
collection’s labels field in KANBAN_INDEX_DEFINITIONS.cards to support the
workerFilter used by pickNextCard, while preserving the existing worker-routing
logic and index definitions.
In `@src/mcp/daisy-kanban-bridge/server.ts`:
- Around line 59-66: Update authorized to compare bridge tokens with
crypto.timingSafeEqual after verifying equal byte lengths, returning false for
missing or mismatched lengths while preserving bearer extraction through bearer.
Convert both token strings to compatible byte buffers before comparison and
import the required crypto utility.
- Around line 138-153: Update mapUpstreamError to recognize the exact
authorization/scope-denial message emitted by the gateway or method-scopes layer
and return status 403 with errorBody code "authorization". Confirm and use that
upstream message text when defining the regex, and tighten the existing
validation and not-found patterns with appropriate boundaries or anchoring to
avoid classifying unrelated errors.
In `@src/mcp/daisy-kanban-mcp/server.ts`:
- Around line 262-276: The toolAnnotations function hardcodes tool names
separately from their definitions, risking stale MCP hints. Add
readOnly/destructive metadata to each tool definition created by
createKanbanTools, then have toolAnnotations derive readOnlyHint,
destructiveHint, and idempotentHint from that metadata instead of maintaining a
separate name-based Set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8ccfe4c-96d2-4dbf-ac27-17cc802d646d
📒 Files selected for processing (19)
docs/automation/codex-desktop-kanban.mddocs/automation/daisy-kanban-workers.mdpackage.jsonskills/daisy-kanban-operator/SKILL.mdsrc/agents/tools/kanban-tool.tssrc/gateway/method-scopes.test.tssrc/gateway/method-scopes.tssrc/gateway/protocol/index.tssrc/gateway/protocol/schema/kanban.contract.test.tssrc/gateway/protocol/schema/kanban.tssrc/gateway/protocol/schema/protocol-schemas.tssrc/gateway/protocol/schema/types.tssrc/gateway/server-methods/kanban.tssrc/kanban/repository.test.tssrc/kanban/repository.tssrc/kanban/types.tssrc/mcp/daisy-kanban-bridge/server.test.tssrc/mcp/daisy-kanban-bridge/server.tssrc/mcp/daisy-kanban-mcp/server.ts
| const invoke = async (method: string, params: Record<string, unknown>) => | ||
| jsonResult(await callGateway(method, readGatewayOpts(params), params)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A15 'function readGatewayOpts|const readGatewayOpts' src/agents/tools/kanban-tool.ts
rg -n -B2 -A20 'function buildReadRequest|function buildCardCreateParams|function buildCardUpdates|GatewayFields =' src/agents/tools/kanban-tool.tsRepository: hughdidit/DAISy-Agency
Length of output: 4034
🏁 Script executed:
sed -n '520,620p' src/agents/tools/kanban-tool.ts
sed -n '620,700p' src/agents/tools/kanban-tool.ts
sed -n '1,120p' src/agents/tools/kanban-tool.tsRepository: hughdidit/DAISy-Agency
Length of output: 11768
🏁 Script executed:
sed -n '330,470p' src/agents/tools/kanban-tool.ts
sed -n '470,520p' src/agents/tools/kanban-tool.tsRepository: hughdidit/DAISy-Agency
Length of output: 7387
🏁 Script executed:
sed -n '300,470p' src/agents/tools/kanban-tool.tsRepository: hughdidit/DAISy-Agency
Length of output: 6263
🏁 Script executed:
sed -n '1,220p' src/agents/tools/gateway.tsRepository: hughdidit/DAISy-Agency
Length of output: 5641
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/agents/tools/kanban-tool.ts')
text = p.read_text()
# Show relevant granular handlers and whether they forward raw args or transformed params.
for needle in [
'execute: async (_id, args) => invoke("kanban.cards.create"',
'execute: async (_id, args) => invoke("kanban.cards.move"',
'execute: async (_toolCallId, args) => {',
'return jsonResult(\n await callGateway(request.method, readGatewayOpts(params), request.params),\n );',
]:
print(f"\n--- {needle} ---")
idx = text.find(needle)
if idx != -1:
print(text[max(0, idx-180): idx+420])
PYRepository: hughdidit/DAISy-Agency
Length of output: 2872
🏁 Script executed:
rg -n -A40 -B10 'KanbanStatusToolSchema|KanbanListCardsToolSchema|KanbanGetCardToolSchema|KanbanActivityToolSchema|KanbanCreateCardToolSchema|KanbanUpdateCardToolSchema|KanbanMoveCardToolSchema|KanbanCommentCardToolSchema|KanbanArchiveCardToolSchema' src/agents/tools/kanban-tool.tsRepository: hughdidit/DAISy-Agency
Length of output: 9913
Preserve raw tool args when reading gateway options
The granular tools drop gatewayUrl, gatewayToken, and timeoutMs: invoke() reads options from the RPC payload, but buildReadRequest / buildCardCreateParams / buildWriteRequest strip those fields first. kanban_status is affected too because it passes {} into invoke(). Read gateway options from the original args, and pass only the RPC-shaped params to callGateway.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agents/tools/kanban-tool.ts` around lines 551 - 552, Update the invoke
flow to read gateway options from the original raw args rather than the RPC
payload, preserving gatewayUrl, gatewayToken, and timeoutMs for all granular
tools including kanban_status. Keep passing only the RPC-shaped params to
callGateway, and adjust invoke’s callers around buildReadRequest,
buildCardCreateParams, and buildWriteRequest accordingly.
| async function readBody(req: IncomingMessage, limit: number): Promise<unknown> { | ||
| const contentLength = Number(req.headers["content-length"] ?? 0); | ||
| if (Number.isFinite(contentLength) && contentLength > limit) { | ||
| throw Object.assign(new Error("request body exceeds limit"), { bridgeCode: "rate-limit" }); | ||
| } | ||
| const chunks: Buffer[] = []; | ||
| let size = 0; | ||
| for await (const chunk of req) { | ||
| const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); | ||
| size += buffer.byteLength; | ||
| if (size > limit) { | ||
| throw Object.assign(new Error("request body exceeds limit"), { bridgeCode: "rate-limit" }); | ||
| } | ||
| chunks.push(buffer); | ||
| } | ||
| if (!size) { | ||
| return {}; | ||
| } | ||
| try { | ||
| return JSON.parse(Buffer.concat(chunks).toString("utf8")); | ||
| } catch { | ||
| throw Object.assign(new Error("request body must be valid JSON"), { bridgeCode: "validation" }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'src/mcp/daisy-kanban-bridge/server.ts'
wc -l src/mcp/daisy-kanban-bridge/server.ts
sed -n '1,260p' src/mcp/daisy-kanban-bridge/server.tsRepository: hughdidit/DAISy-Agency
Length of output: 11210
Avoid throwing from for await here. On chunked requests, the size-limit path can destroy req before the surrounding catch writes the 413/422 JSON response, so clients may only see a connection reset. Drain the body or close the connection explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/daisy-kanban-bridge/server.ts` around lines 68 - 91, Update readBody
so exceeding the limit during for-await does not implicitly destroy req before
the caller can send its error response; explicitly drain the remaining request
body or close the connection in a controlled manner, while preserving the
existing rate-limit error and JSON parsing behavior.
| if (isMcp) { | ||
| if (req.method !== "POST") { | ||
| json(res, 405, errorBody("validation", "MCP requires POST.")); | ||
| return; | ||
| } | ||
| try { | ||
| const request = await readBody(req, NORMAL_BODY_LIMIT); | ||
| const response = await mcp(request as Parameters<typeof mcp>[0]); | ||
| if (response) json(res, 200, response); | ||
| else res.statusCode = 202, res.end(); | ||
| } catch { | ||
| json(res, 400, errorBody("validation", "Invalid MCP request.")); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
/mcp bypasses rate limiting and reuses the tight 1MB body limit even for import tools.
rateLimit() is only invoked in the /v1/* branch (Line 192); the isMcp branch returns at Line 184 without ever calling it, so an authenticated MCP client can call kanban_import_trello_run or other write tools without any throttling, unlike the equivalent REST route. Separately, readBody(req, NORMAL_BODY_LIMIT) (Line 177) is fixed at 1MB regardless of which tool is invoked, so an MCP-driven Trello import (which gets 10MB via /v1/imports/*/run) will be rejected here. The blanket catch (Lines 181-183) also discards the bridgeCode distinction, so an oversized MCP body returns 400 instead of the 413 the REST path returns for the same condition.
Apply the same rateLimit() gate to MCP requests, and either raise the MCP body limit or branch on bridgeCode the same way the /v1/* catch block does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/daisy-kanban-bridge/server.ts` around lines 171 - 185, Update the
isMcp request branch to invoke rateLimit() before processing MCP requests,
matching the /v1/* flow. Adjust readBody() to use the import-specific larger
limit when bridgeCode identifies an import tool, otherwise retain
NORMAL_BODY_LIMIT. Align the MCP catch handling with the /v1/* path so oversized
bodies return 413 while other invalid requests remain 400.
| const params = { ...(req.method === "GET" ? queryParams(url) : body) }; | ||
| const cardId = parts[2]; | ||
| let method = ""; | ||
| if (req.method === "GET" && parts[1] === "status") method = "kanban.status"; | ||
| else if (req.method === "GET" && parts[1] === "board") method = "kanban.board.get"; | ||
| else if (req.method === "GET" && parts[1] === "cards" && !cardId) method = "kanban.cards.list"; | ||
| else if (req.method === "GET" && parts[1] === "cards" && cardId) method = "kanban.cards.get", params.cardId = cardId; | ||
| else if (req.method === "GET" && parts[1] === "activity") method = "kanban.activity.list"; | ||
| else if (req.method === "POST" && parts.length === 2 && parts[1] === "cards") method = "kanban.cards.create"; | ||
| else if (req.method === "PATCH" && parts[1] === "cards" && cardId) method = "kanban.cards.update", params.cardId = cardId; | ||
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "move") method = "kanban.cards.move", params.cardId = cardId; | ||
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "comments") method = "kanban.cards.comment", params.cardId = cardId; | ||
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "archive") method = "kanban.cards.archive", params.cardId = cardId; | ||
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "preview") method = "kanban.import.trello.preview"; | ||
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "run") method = "kanban.import.trello.run"; | ||
| else if (req.method === "POST" && parts[1] === "tasks" && parts[2] === "pick-next") method = "kanban.agent.pickNext"; | ||
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "handoff") method = "kanban.agent.handoff", params.cardId = cardId; | ||
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "complete") method = "kanban.agent.complete", params.cardId = cardId; | ||
| else { | ||
| json(res, 404, errorBody("not-found", "Unknown bridge route.")); | ||
| return; | ||
| } | ||
| if (method === "kanban.cards.update") { | ||
| const updates = params.updates; | ||
| params.updates = isRecord(updates) ? updates : params; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
kanban.cards.update fallback creates a self-referential object, breaking any PATCH without an updates wrapper.
params.cardId = cardId (Line 211) mutates params in place, then when the PATCH body has no updates key, params.updates = isRecord(updates) ? updates : params (Line 226) assigns params to its own updates property — params now contains itself. Any downstream JSON serialization of this object (e.g. the gateway client's frame construction, whose validateRequestFrame(frame) traverses params, followed by JSON.stringify(frame) before sending over the websocket) will throw on the circular reference. So a natural PATCH /v1/cards/:id request with flat fields (e.g. {"title": "..."}) instead of a nested {"updates": {...}} wrapper will fail with an opaque 500, and this path isn't covered by server.test.ts.
🐛 Proposed fix
if (method === "kanban.cards.update") {
const updates = params.updates;
- params.updates = isRecord(updates) ? updates : params;
+ params.updates = isRecord(updates) ? updates : body;
}body is the pre-mutation record (already validated by isRecord(body) at Line 198) and doesn't carry the cardId/circular contamination that params now does.
📝 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.
| const params = { ...(req.method === "GET" ? queryParams(url) : body) }; | |
| const cardId = parts[2]; | |
| let method = ""; | |
| if (req.method === "GET" && parts[1] === "status") method = "kanban.status"; | |
| else if (req.method === "GET" && parts[1] === "board") method = "kanban.board.get"; | |
| else if (req.method === "GET" && parts[1] === "cards" && !cardId) method = "kanban.cards.list"; | |
| else if (req.method === "GET" && parts[1] === "cards" && cardId) method = "kanban.cards.get", params.cardId = cardId; | |
| else if (req.method === "GET" && parts[1] === "activity") method = "kanban.activity.list"; | |
| else if (req.method === "POST" && parts.length === 2 && parts[1] === "cards") method = "kanban.cards.create"; | |
| else if (req.method === "PATCH" && parts[1] === "cards" && cardId) method = "kanban.cards.update", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "move") method = "kanban.cards.move", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "comments") method = "kanban.cards.comment", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "archive") method = "kanban.cards.archive", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "preview") method = "kanban.import.trello.preview"; | |
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "run") method = "kanban.import.trello.run"; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[2] === "pick-next") method = "kanban.agent.pickNext"; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "handoff") method = "kanban.agent.handoff", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "complete") method = "kanban.agent.complete", params.cardId = cardId; | |
| else { | |
| json(res, 404, errorBody("not-found", "Unknown bridge route.")); | |
| return; | |
| } | |
| if (method === "kanban.cards.update") { | |
| const updates = params.updates; | |
| params.updates = isRecord(updates) ? updates : params; | |
| } | |
| const params = { ...(req.method === "GET" ? queryParams(url) : body) }; | |
| const cardId = parts[2]; | |
| let method = ""; | |
| if (req.method === "GET" && parts[1] === "status") method = "kanban.status"; | |
| else if (req.method === "GET" && parts[1] === "board") method = "kanban.board.get"; | |
| else if (req.method === "GET" && parts[1] === "cards" && !cardId) method = "kanban.cards.list"; | |
| else if (req.method === "GET" && parts[1] === "cards" && cardId) method = "kanban.cards.get", params.cardId = cardId; | |
| else if (req.method === "GET" && parts[1] === "activity") method = "kanban.activity.list"; | |
| else if (req.method === "POST" && parts.length === 2 && parts[1] === "cards") method = "kanban.cards.create"; | |
| else if (req.method === "PATCH" && parts[1] === "cards" && cardId) method = "kanban.cards.update", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "move") method = "kanban.cards.move", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "comments") method = "kanban.cards.comment", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "cards" && parts[3] === "archive") method = "kanban.cards.archive", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "preview") method = "kanban.import.trello.preview"; | |
| else if (req.method === "POST" && parts[1] === "imports" && parts[3] === "run") method = "kanban.import.trello.run"; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[2] === "pick-next") method = "kanban.agent.pickNext"; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "handoff") method = "kanban.agent.handoff", params.cardId = cardId; | |
| else if (req.method === "POST" && parts[1] === "tasks" && parts[3] === "complete") method = "kanban.agent.complete", params.cardId = cardId; | |
| else { | |
| json(res, 404, errorBody("not-found", "Unknown bridge route.")); | |
| return; | |
| } | |
| if (method === "kanban.cards.update") { | |
| const updates = params.updates; | |
| params.updates = isRecord(updates) ? updates : body; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/daisy-kanban-bridge/server.ts` around lines 202 - 227, Update the
kanban.cards.update fallback in the route-dispatch logic so flat PATCH bodies
use the original validated body record rather than the mutated params object
when no updates wrapper is present. Preserve wrapped updates unchanged, while
ensuring the fallback excludes route-added cardId from the updates payload and
cannot create a circular reference.
Summary\n- add agent-neutral pickup, handoff, and completion RPC aliases with worker routing\n- add authenticated loopback REST/MCP bridge and granular Kanban tools\n- add operator skill, worker schedule documentation, and bridge contract tests\n\n## Verification\n- git diff --check\n- Sites build, rendered HTML tests, and lint pass in the companion Sites workspace\n- DAISy runtime tests/build require the project GCP container workflow per AGENTS.md and are pending CI/staging\n\n## Risk\n- bridge credentials and tunnel deployment remain environment-specific; no secrets are committed\n
Summary by CodeRabbit
New Features
Documentation
Compatibility