Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (2 warning).
packages/harness/src/permissions.ts
Solid redesign of approvals and questions with good test coverage; the main issue is an off-by-one between the frontend answer construction (up to 9 for multi-select + free text) and the backend validation (max 8), which silently drops the user's answer.
- 🟡 bugs: Off-by-one in multi-select answer validation.
resolvePendingrejectsanswer.length > 8(max 8 per question), but the frontend'sPermissionBannerconstructs answers as[...selections, ...writtenText]— if a multi-select question has 8 options (the Zod schema max) and the user selects all 8 plus writes a free-text answer, the array has 9 entries and is silently rejected. The client has already cleared the banner viarespondToPermission, so the user cannot retry; the request times out. Fix: change the limit toanswer.length > 9or cap the combined frontend array at 8.[fixable]
packages/harness/src/permission-handler.ts
Solid redesign of approvals and questions with good test coverage; the main issue is an off-by-one between the frontend answer construction (up to 9 for multi-select + free text) and the backend validation (max 8), which silently drops the user's answer.
- 🟡 bugs: Abort and timeout handlers send duplicate events.
wrappedResolve(called viaresolvePending) sendspermission_resolvedto the transport, but then the calling code in both theonAborthandler and thesetTimeoutcallback also sendspermission_timeoutfor the samepermId. The client is idempotent (both map toPERMISSION_TIMEOUT), so no user-visible breakage, but the server sends two transport messages per resolution in these paths instead of one. Remove thetransportSend(…permission_timeout…)lines from the abort and timeout handlers sincewrappedResolvealready broadcastspermission_resolved.[fixable]
frontend/src/components/__tests__/PermissionBanner.test.tsx
Solid redesign of approvals and questions with good test coverage; the main issue is an off-by-one between the frontend answer construction (up to 9 for multi-select + free text) and the backend validation (max 8), which silently drops the user's answer.
- 🔵 style (L90): The four new test cases (
renders questions as choices…,accepts a free-text answer…,uses the server deadline…,shows full approval input…) are top-levelit()calls outside the existingdescribe('PermissionBanner', …)block which closes at line 89. They will run but won't appear under the 'PermissionBanner' group in test output. Move them inside the describe block.[fixable]
| } | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 style: The four new test cases (renders questions as choices…, accepts a free-text answer…, uses the server deadline…, shows full approval input…) are top-level it() calls outside the existing describe('PermissionBanner', …) block which closes at line 89. They will run but won't appear under the 'PermissionBanner' group in test output. Move them inside the describe block. [fixable]
Centaur ReviewFound 4 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (3 warning).
packages/harness/src/permission-handler.ts
Solid feature addition with good test coverage for the happy paths. Main concerns: unbounded toolInput sent over WebSocket (removed truncation without a replacement cap), and silent failures when server-side answer validation rejects a client submission — the client clears the banner but the server never resolves the permission.
- 🟡 bugs (L168): toolInput is now sent unbounded to the client. Previously
summarizeToolInputtruncated to ~200 chars. A Write tool call with a large file body or a Bash heredoc could produce a multi-MBtoolInputstring sent over the WebSocket and rendered in a<pre>tag. Consider adding a size cap (e.g., truncate after 10KB) to the string sent in the permission request, while keeping the full_toolInputobject for the SDK resolve path.[fixable]
packages/harness/src/permissions.ts
Solid feature addition with good test coverage for the happy paths. Main concerns: unbounded toolInput sent over WebSocket (removed truncation without a replacement cap), and silent failures when server-side answer validation rejects a client submission — the client clears the banner but the server never resolves the permission.
- 🟡 bugs (L53): For single-select questions (
multiSelect: false),answer.length !== 1rejects answers. But the frontend builds answers by concatenating selections + written text. If a user selects a radio option and then types free text without the clearing handler firing (e.g., programmatic state, tests, or a race with React batching), both would be present and the server would silently reject withreturn false. The v1 WS handler atserver/index.ts:893also doesn't passsessionId, soresolvePendingskips session validation for v1 clients — a silent rejection leaves the permission pending until timeout with no feedback to the user.[fixable] - 🔵 unsafe_assumptions (L54): Whitespace-only answer validation mismatch:
resolvePendingrejects whitespace-only strings via!value.trim(), but the Zod schemas inws-schemas-v2.tsandws-schemas.tsusez.string().min(1)which accepts whitespace-only strings like" ". A client-submitted whitespace-only answer passes schema validation but silently fails inresolvePending. Aligning these (e.g., adding.trim().min(1)to the Zod schema or.transform(s => s.trim())) would surface the error earlier.[fixable]
server/ws-handler-v2.ts
Solid feature addition with good test coverage for the happy paths. Main concerns: unbounded toolInput sent over WebSocket (removed truncation without a replacement cap), and silent failures when server-side answer validation rejects a client submission — the client clears the banner but the server never resolves the permission.
- 🟡 bugs (L811):
resolvePendingreturnsfalsewhen validation fails (wrong answer count, whitespace-only values,alwaysdecision on a question, missing session match). The return value is silently ignored here and atserver/index.ts:893. When validation fails, the permission stays deleted (it's not — pending.delete only runs after validation passes, which is correct), but the user sees the card disappear (the client dispatches PERMISSION_TIMEOUT locally inrespondToPermission) while the server-side promise never resolves. The permission hangs until the 120s timeout fires server-side, while the client has already cleared the banner. The user has no way to retry.[fixable]
frontend/src/styles/global.css
Solid feature addition with good test coverage for the happy paths. Main concerns: unbounded toolInput sent over WebSocket (removed truncation without a replacement cap), and silent failures when server-side answer validation rejects a client submission — the client clears the banner but the server never resolves the permission.
- 🔵 regressions: The
.perm-banner--elevatedand.perm-banner--unknownclasses previously setborder-top-colorto visually distinguish elevated/unknown tier banners. The new CSS removes both rules without a replacement visual indicator. Elevated and unknown tier banners now look identical to standard ones aside from the tier badge text. The old desktop-specific.perm-banneroverride (centered, max-width 480px) is also removed — the new base styles handle this, but the max-width changed from 480px to 600px.[fixable]
packages/harness/__tests__/user-questions.test.ts
Solid feature addition with good test coverage for the happy paths. Main concerns: unbounded toolInput sent over WebSocket (removed truncation without a replacement cap), and silent failures when server-side answer validation rejects a client submission — the client clears the banner but the server never resolves the permission.
- 🔵 missing_tests: No test covers the case where
resolvePendingreturnsfalseat the WS handler layer and the user gets no feedback. Also missing: a test for single-select questions where the user provides only a free-text answer (no option selected), which is a valid UX path in the PermissionBanner component.[fixable]
| permId, | ||
| toolName, | ||
| toolInput: inputSummary, | ||
| toolInput: questions |
There was a problem hiding this comment.
🟡 bugs: toolInput is now sent unbounded to the client. Previously summarizeToolInput truncated to ~200 chars. A Write tool call with a large file body or a Bash heredoc could produce a multi-MB toolInput string sent over the WebSocket and rendered in a <pre> tag. Consider adding a size cap (e.g., truncate after 10KB) to the string sent in the permission request, while keeping the full _toolInput object for the SDK resolve path. [fixable]
| !Array.isArray(answer) || | ||
| !answer.length || | ||
| answer.length > 9 || | ||
| (!question.multiSelect && answer.length !== 1) || |
There was a problem hiding this comment.
🟡 bugs: For single-select questions (multiSelect: false), answer.length !== 1 rejects answers. But the frontend builds answers by concatenating selections + written text. If a user selects a radio option and then types free text without the clearing handler firing (e.g., programmatic state, tests, or a race with React batching), both would be present and the server would silently reject with return false. The v1 WS handler at server/index.ts:893 also doesn't pass sessionId, so resolvePending skips session validation for v1 clients — a silent rejection leaves the permission pending until timeout with no feedback to the user. [fixable]
| !answer.length || | ||
| answer.length > 9 || | ||
| (!question.multiSelect && answer.length !== 1) || | ||
| answer.some((value) => typeof value !== 'string' || !value.trim() || value.length > 4000) |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Whitespace-only answer validation mismatch: resolvePending rejects whitespace-only strings via !value.trim(), but the Zod schemas in ws-schemas-v2.ts and ws-schemas.ts use z.string().min(1) which accepts whitespace-only strings like " ". A client-submitted whitespace-only answer passes schema validation but silently fails in resolvePending. Aligning these (e.g., adding .trim().min(1) to the Zod schema or .transform(s => s.trim())) would surface the error earlier. [fixable]
| }, | ||
| () => { | ||
| resolvePending(msg.permId, msg.decision ?? 'deny'); | ||
| resolvePending(msg.permId, msg.decision ?? 'deny', msg.answers, msg.sessionId); |
There was a problem hiding this comment.
🟡 bugs: resolvePending returns false when validation fails (wrong answer count, whitespace-only values, always decision on a question, missing session match). The return value is silently ignored here and at server/index.ts:893. When validation fails, the permission stays deleted (it's not — pending.delete only runs after validation passes, which is correct), but the user sees the card disappear (the client dispatches PERMISSION_TIMEOUT locally in respondToPermission) while the server-side promise never resolves. The permission hangs until the 120s timeout fires server-side, while the client has already cleared the banner. The user has no way to retry. [fixable]
55f3962 to
c962739
Compare
dimakis
changed the title
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (3 warning).
packages/harness/src/permission-handler.ts
The PR improves bounds checking but introduces an unsafe truncated-approval view and two response-path regressions involving terminal error handling and destructive whitespace normalization.
- 🟡 unsafe_assumptions (L47): The approval payload now shows only the first 10,000 characters while
resolvePendingstill executes the complete input. A long Bash command or external-tool payload can therefore hide consequential operations after the truncation marker and receive approval without the user seeing them. The approval boundary should expose the full input (for example via an expandable/detail channel) or reject inputs that cannot be reviewed safely.[fixable]
server/ws-handler-v2.ts
The PR improves bounds checking but introduces an unsafe truncated-approval view and two response-path regressions involving terminal error handling and destructive whitespace normalization.
- 🟡 regressions (L935): A failed/stale/duplicate permission response is emitted as the generic
errorevent. The client handles every such event as terminal: itsERRORreducer setsrunning=falseand clears the current streaming message. A timeout race or double-click can therefore discard the in-progress assistant/tool message even though the permission remains pending or was already resolved. Use a permission-specific nonterminal rejection event (and make the same change in the legacy handler inserver/index.ts) instead oferror.[fixable]
packages/protocol/src/ws-schemas-v2.ts
The PR improves bounds checking but introduces an unsafe truncated-approval view and two response-path regressions involving terminal error handling and destructive whitespace normalization.
- 🟡 regressions (L123): Using
.trim()in the schema transforms valid answer keys and values rather than merely validating them. Question IDs and option labels are not trimmed when requests are registered, so a provider question whose ID or allowed option has leading/trailing whitespace becomes impossible to resolve; free-form answers are also silently changed before reaching the SDK. Validatevalue.trim().length > 0with a refinement while preserving the original strings, and mirror the fix inws-schemas.ts.[fixable]
| toolName === 'Bash' && typeof input.command === 'string' | ||
| ? input.command | ||
| : JSON.stringify(input, null, 2); | ||
| return full.length > PERMISSION_INPUT_MAX_CHARS |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The approval payload now shows only the first 10,000 characters while resolvePending still executes the complete input. A long Bash command or external-tool payload can therefore hide consequential operations after the truncation marker and receive approval without the user seeing them. The approval boundary should expose the full input (for example via an expandable/detail channel) or reject inputs that cannot be reviewed safely. [fixable]
| ); | ||
| if (!resolved) { | ||
| try { | ||
| ctx.connRegistry.get(connectionId)?.transport.send({ |
There was a problem hiding this comment.
🟡 regressions: A failed/stale/duplicate permission response is emitted as the generic error event. The client handles every such event as terminal: its ERROR reducer sets running=false and clears the current streaming message. A timeout race or double-click can therefore discard the in-progress assistant/tool message even though the permission remains pending or was already resolved. Use a permission-specific nonterminal rejection event (and make the same change in the legacy handler in server/index.ts) instead of error. [fixable]
| answers: z | ||
| .record(z.string().min(1).max(4000), z.array(z.string().min(1).max(4000)).min(1).max(9)) | ||
| .record( | ||
| z.string().trim().min(1).max(4000), |
There was a problem hiding this comment.
🟡 regressions: Using .trim() in the schema transforms valid answer keys and values rather than merely validating them. Question IDs and option labels are not trimmed when requests are registered, so a provider question whose ID or allowed option has leading/trailing whitespace becomes impossible to resolve; free-form answers are also silently changed before reaching the SDK. Validate value.trim().length > 0 with a refinement while preserving the original strings, and mirror the fix in ws-schemas.ts. [fixable]
Centaur ReviewFound 3 issue(s) (3 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 1 issue(s) (1 warning).
packages/harness/src/permission-handler.ts
The approval hardening is coherent, but the new payload-size helper introduces a strict TypeScript build failure.
- 🟡 bugs (L49):
JSON.stringify()is typed as returningstring | undefined, sofull.lengthfails strict TypeScript compilation (fullis possibly undefined). Handle an undefined serialization result before reading.length.[fixable]
| toolName === 'Bash' && typeof input.command === 'string' | ||
| ? input.command | ||
| : JSON.stringify(input, null, 2); | ||
| return full.length <= PERMISSION_INPUT_MAX_CHARS ? full : undefined; |
There was a problem hiding this comment.
🟡 bugs: JSON.stringify() is typed as returning string | undefined, so full.length fails strict TypeScript compilation (full is possibly undefined). Handle an undefined serialization result before reading .length. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s) (2 warning).
packages/protocol/src/ws-schemas-v2.ts
Two approval-hardening gaps remain: malformed SSE responses can destroy client stream state, and throwing serialization failures escape the safe-denial path.
- 🟡 bugs (L127): Schema-invalid permission answers are not retry-safe on the default browser SSE transport. POST /api/chat/permission rejects them in validateBody before handlePermissionResponseV2 can emit permission_response_rejected; SseConnection converts that 400 into a generic ERROR action, which clears current streaming content and sets running=false while the server is still waiting on the permission. Handle permission validation failures specially in the REST route/client so they dispatch PERMISSION_REJECTED and preserve stream state.
[fixable]
packages/harness/src/permission-handler.ts
Two approval-hardening gaps remain: malformed SSE responses can destroy client stream state, and throwing serialization failures escape the safe-denial path.
- 🟡 unsafe_assumptions (L48): The new serialization guard only handles JSON.stringify returning undefined; JSON.stringify can also throw for circular objects, BigInt values, or throwing toJSON methods. Because the permission handler is async, such input rejects the handler instead of returning the intended safe denial. Catch serialization errors and return undefined, with a regression test using a throwing/circular input.
[fixable]
| answers: z | ||
| .record(z.string().min(1).max(4000), z.array(z.string().min(1).max(4000)).min(1).max(9)) | ||
| .optional(), | ||
| answers: z.record(z.string(), z.array(NonBlankPermissionText).min(1).max(9)).optional(), |
There was a problem hiding this comment.
🟡 bugs: Schema-invalid permission answers are not retry-safe on the default browser SSE transport. POST /api/chat/permission rejects them in validateBody before handlePermissionResponseV2 can emit permission_response_rejected; SseConnection converts that 400 into a generic ERROR action, which clears current streaming content and sets running=false while the server is still waiting on the permission. Handle permission validation failures specially in the REST route/client so they dispatch PERMISSION_REJECTED and preserve stream state. [fixable]
| const full = | ||
| toolName === 'Bash' && typeof input.command === 'string' | ||
| ? input.command | ||
| : JSON.stringify(input, null, 2); |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The new serialization guard only handles JSON.stringify returning undefined; JSON.stringify can also throw for circular objects, BigInt values, or throwing toJSON methods. Because the permission handler is async, such input rejects the handler instead of returning the intended safe denial. Catch serialization errors and return undefined, with a regression test using a throwing/circular input. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 1 issue(s) (1 warning).
server/chat-rest-handler.ts
The validation and client retry state are generally sound, but semantic permission rejections can be lost on the primary SSE transport because the REST endpoint acknowledges them as successful.
- 🟡 bugs (L247): The REST endpoint always returns
{ok:true}after a schema-valid response, even whenresolvePending()rejected it.handlePermissionResponseV2()reports that failure only through a non-durable SSE event, whileSseConnection.doPost()interprets only non-2xx responses as submission failures. If the SSE stream closes or its write fails during the POST, the client receives a successful HTTP response and no retry error; an expired request will not be replayed, leaving the stale prompt visible. Propagate the resolution result and return a non-2xxpermission_response_rejectedresponse when it is false.[fixable]
| const msg = parsed.data; | ||
| try { | ||
| handlePermissionResponseV2(connectionId, msg, ctx); | ||
| res.json({ ok: true }); |
There was a problem hiding this comment.
🟡 bugs: The REST endpoint always returns {ok:true} after a schema-valid response, even when resolvePending() rejected it. handlePermissionResponseV2() reports that failure only through a non-durable SSE event, while SseConnection.doPost() interprets only non-2xx responses as submission failures. If the SSE stream closes or its write fails during the POST, the client receives a successful HTTP response and no retry error; an expired request will not be replayed, leaving the stale prompt visible. Propagate the resolution result and return a non-2xx permission_response_rejected response when it is false. [fixable]
Centaur ReviewLGTM — no issues found. |
1 participant
Summary
Structured questions and approval-card support reached
mainthrough later protocol and permission work while this PR was open. This branch is rebuilt on currentmainand now contains only the remaining hardening work from the original review:Safety
<pre>contentValidation
npm test -- --maxWorkers=4(304 files; 4,361 passed, 10 skipped)npm run build:allnpm run lint(0 errors; 4 existing warnings)npm run format:checkNo production deployment or model call was made.