fix(context-blocks): XML sanitization, size limits, and naming

[dimakis]

Summary

• Sanitize XML attribute values in context block headers (escape &, ", <, >)
• Add 100KB size limit with truncation for oversized context blocks
• Normalize context block naming in prompt assembly

Builds on #118 with hardening that wasn't included in the initial merge.

Test plan

• Verify context blocks with special characters in names render correctly
• Test with a context block file >100KB — should truncate with notice
• Existing context block tests still pass

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 57 issue(s) (3 critical) (26 warning).

frontend/src/components/ChatInput.tsx

Solid feature wiring; main concerns are missing tests, no input validation in this layer, and ensuring all parent call sites match the updated callback signatures.

• 🟡 regressions: The onSend and onInterrupt callback signatures changed (added contextBlocks parameter). All call sites passing these props must be updated or they will silently ignore context blocks. Verify parent components match the new signature. [fixable]
• 🟡 unsafe_assumptions: contextBlocks are passed as raw string[] with no validation or size limit. The PR title mentions 'size limits and XML sanitization' but this file applies neither — sanitization/limits must be enforced elsewhere or context blocks could carry unbounded/malicious content. [fixable]
• 🔵 bugs (L184): onInterrupt does not check its return value before clearing state (unlike onSend which checks if (sent)). If the interrupt fails, contextBlocks/images/text are still cleared. This is pre-existing behavior for text and images but now extends to contextBlocks — consider guarding the reset. [fixable]
• 🟡 missing_tests: No tests cover: (1) contextBlocks passed through onSend/onInterrupt, (2) toggle logic adding/removing from the array, (3) state reset after send. These are the core behaviors introduced by this diff. [fixable]
• 🔵 style (L266): The template literal for the className is getting long and hard to read. Consider extracting to a variable or using a classnames/clsx utility if one is already in the project. [fixable]

frontend/src/components/ContextPicker.tsx

Solid component structure, but needs response error handling, runtime validation of API data, and test coverage.

• 🟡 bugs (L34): No error handling for non-OK HTTP responses. If the server returns a 4xx/5xx, r.json() will either throw or parse an error body, leading to unexpected behavior. Should check r.ok before parsing. [fixable]
• 🟡 unsafe_assumptions (L34): The response type assertion (data: { contextBlocks?: ... }) is unchecked at runtime. If the API shape changes or returns unexpected data, Object.entries could operate on non-object values. Consider validating typeof data.contextBlocks === 'object'. [fixable]
• 🔵 bugs (L55): onClose is used inside the effect but listed as a dependency. If onClose is not stable (not wrapped in useCallback by the parent), this effect will re-register the listener on every render, which is wasteful and could cause stale-closure issues.
• 🟡 missing_tests: No tests accompany this new component. Key cases to cover: rendering with empty blocks, rendering with blocks, toggle callback, click-outside closing, fetch failure fallback, and formatSize unit tests.
• 🔵 style (L22): formatSize uses toFixed(1) which can produce trailing zeros (e.g. '1.0 KB'). Minor, but worth noting if clean display is a goal. [fixable]
• 🔵 unsafe_assumptions (L34): The fetch URL /api/config is hardcoded. If the app is served under a base path or the API host differs, this will break. Consider using a configured base URL. [fixable]

frontend/src/components/MessageBubble.tsx

Straightforward prop addition; main concerns are missing tests and ensuring sanitization happens before values reach this component.

• 🟡 unsafe_assumptions (L13): contextBlocks values are rendered directly into the DOM without sanitization. Given the PR title mentions 'XML sanitization', ensure these strings are escaped upstream — if not, this is an XSS vector. [fixable]
• 🟡 missing_tests: No tests added for the new contextBlocks rendering path — should cover: empty array, single block, multiple blocks, and blocks containing special characters. [fixable]
• 🔵 style (L13): The '@ ' prefix is a magic string with no explanation. Consider extracting it to a constant or adding a brief comment explaining what it signifies to the user. [fixable]

frontend/src/hooks/useChatMessages.ts

Clean mechanical addition, but verify the message type accepts contextBlocks and add reducer test coverage.

• 🟡 regressions: The contextBlocks property is added to the USER_SEND action and passed through to the message object, but there's no visible type definition update for the message object itself. If the message type doesn't already include contextBlocks?: string[], this will cause a TypeScript error. [fixable]
• 🔵 missing_tests: No tests covering the new contextBlocks flow through the reducer — e.g., verifying contextBlocks are stored on the message when provided, and that undefined/empty arrays are handled correctly. [fixable]

frontend/src/pages/ChatView.tsx

Main concern is the new USER_SEND dispatch in interruptMessage potentially duplicating user bubbles; the rest is cleanup and missing tests.

• 🔴 regressions (L191): interruptMessage now dispatches USER_SEND and calls forceScrollToBottom(), but the caller likely already does this (or the old behavior intentionally omitted it). This could cause duplicate user bubbles if the caller also dispatches USER_SEND after calling interruptMessage. [fixable]
• 🟡 bugs (L171): The two branches of the if/else (msgState.running vs not) are now identical — both call wsSend, dispatch USER_SEND, and forceScrollToBottom. The only original difference (the comment about server queuing) doesn't justify the duplication. Consider collapsing them. [fixable]
• 🟡 unsafe_assumptions (L167): contextBlocks is attached to payload via direct mutation (payload.contextBlocks = contextBlocks). If buildSendPayload returns a frozen object or if the payload type doesn't include contextBlocks, this will silently fail or throw. Prefer spreading into a new object. [fixable]
• 🟡 missing_tests: No tests cover the new contextBlocks plumbing through sendMessage, interruptMessage, or the UserBubble rendering. At minimum, verify that contextBlocks are forwarded in the WebSocket payload and rendered in the bubble.
• 🔵 style (L167): Inconsistent conditional spread style: sendMessage uses direct assignment (payload.contextBlocks = contextBlocks) while interruptMessage uses spread (...(contextBlocks?.length ? { contextBlocks } : {})). Pick one pattern for consistency. [fixable]

frontend/src/styles/global.css

Clean CSS addition; minor concerns around hardcoded accent colors and border-radius clipping on list items.

• 🔵 unsafe_assumptions (L2088): Hardcoded rgba color rgba(108, 99, 255, 0.08) for selected state (and 0.1 for pills) instead of using a CSS variable. If --accent changes, these won't follow. Consider deriving from var(--accent) or adding a dedicated --accent-bg variable. [fixable]
• 🔵 style (L2070): .context-picker-item uses border-bottom for separators but the parent .context-picker has border-radius: 12px. The items themselves have no border-radius, so background-color on hover/selected for the first/last items will bleed into the rounded corners. Consider adding overflow: hidden on .context-picker-list or border-radius on first/last items. [fixable]
• 🔵 missing_tests: No visual regression or snapshot tests for the new context picker and pill components. Acceptable if the project doesn't have CSS-level testing, but worth noting.

frontend/src/types/chat.ts

Low-risk additive change; consider a more specific type than string[] for clarity.

• 🔵 style (L51): The type string[] is vague — consider a more descriptive type or alias (e.g., ContextBlockId[] or a structured type) to clarify what each string represents (raw content? serialized XML? block IDs?). [fixable]
• 🔵 regressions (L51): Since this is optional and additive to an existing interface, it won't break existing consumers. However, confirm that serialization/deserialization paths (e.g., session restore, history persistence) handle the new field gracefully when absent.

server/__tests__/assemble-prompt.test.ts

Solid test coverage for the new feature; main concern is the fixed temp directory path risking CI flakiness, plus a few missing edge-case tests.

• 🟡 unsafe_assumptions (L5): TMP_DIR uses a fixed path relative to the source tree (.test-assemble-prompt in the repo root). Parallel CI runs or leftover directories from a crashed run could cause flaky tests. Consider using os.tmpdir() + a unique suffix or mkdtemp. [fixable]
• 🔵 missing_tests: No test for XML-unsafe characters appearing in file content (e.g., <script> inside the .md file). If the implementation escapes content too, that should be verified; if it intentionally doesn't, a test documenting that decision would be useful. [fixable]
• 🔵 missing_tests: No test for a context block file that exists but is empty (0 bytes). Worth verifying whether it produces an empty <context> tag or is skipped. [fixable]
• 🔵 style (L30): for (const key of Object.keys(mockContextBlocks)) delete mockContextBlocks[key]; — clearing a shared mutable object via delete-loop is fragile. Reassigning to a fresh object (or using a wrapper) would be clearer, though the current approach works given the mock structure. [fixable]
• 🔵 unsafe_assumptions (L149): The images object is cast inline without a type annotation. If the assemblePrompt signature changes the image schema (e.g., requires width/height), this test will silently pass a wrong shape. Consider importing and using the actual type. [fixable]
• 🔵 missing_tests: Edge case: duplicate names in the contextBlocks array (e.g., ['Workflow', 'Workflow']). Should it inject the block once or twice? [fixable]

server/__tests__/repo-config.test.ts

Good coverage of the happy path and basic validation, but missing tests for security-relevant edge cases (path traversal) and the XML sanitization/size limits/naming constraints mentioned in the PR title.

• 🟡 missing_tests: No test for path traversal in contextBlock values (e.g., ../../etc/passwd). The implementation should reject or sanitize paths that escape the repo root. [fixable]
• 🔵 missing_tests: No test for invalid contextBlock keys (e.g., empty string, keys with special characters, or keys containing XML-unsafe characters like < or &). Given the PR title mentions XML sanitization, key sanitization should be covered. [fixable]
• 🔵 missing_tests: No test for size limits on contextBlocks. The PR title mentions size limits but no test enforces a maximum number of blocks or maximum path length. [fixable]
• 🔵 missing_tests: No test for the naming constraints mentioned in the PR title (e.g., block names with spaces, very long names, duplicate names with different casing). [fixable]
• 🔵 style: The filters out non-string contextBlock values test uses 42 and null but could also cover [] and {} to be thorough about non-string types that pass typeof checks differently. [fixable]

server/__tests__/routes.test.ts

Mock and assertion updates are correct but test coverage is shallow relative to the PR's stated scope (XML sanitization, size limits, naming).

• 🔵 missing_tests: Tests only verify contextBlocks is present and is an object, but don't test any contextBlocks content, XML sanitization, size limits, or naming — the features mentioned in the PR title. Consider adding tests with actual contextBlocks data to validate the sanitization and size-limit behavior. [fixable]

server/app.ts

Likely crash when contextBlocks is absent from config; also has path traversal and sync-IO concerns.

• 🔴 bugs (L284): No null/undefined guard on repoConfig.contextBlocks. If the config omits contextBlocks, Object.entries(undefined) throws a TypeError. [fixable]
• 🟡 unsafe_assumptions (L285): path comes directly from user-controlled config and is passed to statSync without validation. A crafted path (e.g. /etc/shadow, symlinks) could leak file-size information for arbitrary files on the host. Consider resolving and constraining paths to BASE_REPO. [fixable]
• 🟡 unsafe_assumptions (L286): statSync is a blocking call inside a request handler. If many context blocks exist or the filesystem is slow (network mount), this blocks the event loop. Consider using stat (async) with Promise.all. [fixable]
• 🟡 missing_tests: No tests cover the new contextBlocks field in the /api/config response — neither the happy path (files exist with known sizes) nor the error path (missing files returning 0). [fixable]

server/chat.ts

UTF-8 truncation bug and unescaped XML body content are the main risks; the feature logic is otherwise sound but needs tests.

• 🔴 bugs (L224): Truncating a UTF-8 buffer at an arbitrary byte offset with subarray(0, CONTEXT_BLOCK_MAX_BYTES) can slice in the middle of a multi-byte character (e.g. emoji, CJK). The subsequent .toString('utf-8') will produce a replacement character (U+FFFD) or corrupt the last character. Use a decoder with { fatal: false } or find the nearest valid codepoint boundary before slicing. [fixable]
• 🟡 unsafe_assumptions (L233): The content inside context blocks is not XML-escaped. If a context file contains </context> or similar XML-like closing tags, it will prematurely terminate the block, corrupting the prompt and potentially allowing prompt injection via file contents. Either escape the content body or use a CDATA section. [fixable]
• 🟡 unsafe_assumptions (L213): repoConfig.contextBlocks is accessed without a guard. If repoConfig or repoConfig.contextBlocks is undefined (e.g. missing config file, startup race), this will throw a runtime error. Add optional chaining or a fallback. [fixable]
• 🟡 bugs (L220): The byte-length check uses Buffer.byteLength(content) but then truncates using Buffer.from(content).subarray(). For very large files this allocates the full buffer in memory before truncating. Consider using fs.openSync + fs.readSync with a fixed-size buffer, or at minimum stream/slice before fully loading the file, to avoid a transient memory spike for files much larger than 100 KB. [fixable]
• 🟡 missing_tests: No tests cover the new assemblePrompt context-block logic: file-not-found fallback, truncation behavior, XML escaping, or the interaction between context blocks and images. These are all branching paths that should have unit coverage. [fixable]
• 🔵 style (L237): ---CONTEXT_END--- is an ad-hoc sentinel that could collide with file content. Since you're already using XML-style <context> tags, a closing </context-blocks> wrapper (with an opening tag) would be more consistent and less fragile. [fixable]

server/index.ts

Mechanical plumbing change looks consistent across all branches; main risk is whether downstream functions accept the new parameter as optional.

• 🟡 unsafe_assumptions (L199): msg.contextBlocks is passed through without validation. If contextBlocks is undefined, verify that sendToChat, startChat, and interruptChat handle undefined gracefully (e.g., optional parameter or default). If they expect an array, passing undefined could cause runtime errors downstream. [fixable]
• 🔵 regressions (L199): The function signatures of sendToChat and interruptChat are being extended with a new positional parameter. Confirm all other call sites (if any) have been updated, and that the parameter is optional to avoid breaking existing callers. [fixable]
• 🟡 missing_tests: No tests visible for the new contextBlocks plumbing. At minimum, verify that contextBlocks are correctly forwarded in both the active (sendToChat) and inactive (startChat) paths, as well as the interrupt path.

server/repo-config.ts

Structurally sound addition that mirrors existing patterns, but lacks input validation on keys/paths and has no test coverage.

• 🟡 unsafe_assumptions (L139): No validation on name keys — arbitrary keys from user config are accepted as-is. If downstream code uses these names in file paths, shell commands, or XML tags, this could enable injection. Consider validating that names match a safe pattern (e.g., /^[a-zA-Z0-9_-]+$/). [fixable]
• 🔵 unsafe_assumptions (L140): Resolved paths are not checked for path traversal (e.g., ../../etc/passwd). If context blocks are read and injected into prompts, an attacker-controlled config could exfiltrate sensitive files. Consider resolving and validating that the final path stays within the repo root. [fixable]
• 🟡 missing_tests: No tests visible for contextBlocks parsing — should cover: valid entries, non-string values skipped, array rejected, absolute vs relative path resolution, missing/undefined contextBlocks field. [fixable]
• 🔵 style (L132): The pattern obj.X && typeof obj.X === 'object' && !Array.isArray(obj.X) is repeated for repos and now contextBlocks. Consider extracting a small helper like isStringRecord(val) to reduce duplication. [fixable]

server/ws-schemas.ts

Schema additions are structurally correct but lack size/content constraints that the PR title implies should exist.

• 🟡 unsafe_assumptions (L24): contextBlocks accepts arbitrary strings with no validation on content or size. Given the PR title mentions 'XML sanitization, size limits', consider adding .max() on the array and/or individual string length constraints at the schema level rather than relying solely on downstream validation. [fixable]
• 🔵 style (L24): Consider extracting a shared contextBlocks schema fragment (e.g. const ContextBlocksSchema = z.array(z.string()).optional()) to avoid duplication between SendMessage and InterruptMessage. [fixable]

[dimakis]

Already merged to main — all commits including the polish (XML sanitization, size limits) were cherry-picked.

[dimakis]

Centaur has opened PR #147 with fixes for the flagged issues.