Settle the dead design-system surface policy - #944
Merged
Conversation
Three buckets of unreferenced design-system code have been deferred twice (PR #807, PR #934) because they are a judgement call rather than clear-cut dead application code. This settles the policy and applies it. Delete what is dead weight: - apps/web/src/components/ui/sheet.tsx (134 lines, zero importers) - five unused tokens in lib/glass.ts (glassDivider, glassHover, glassActive, glassBadgeBg, primaryGlow) Keep, and say so in a comment, what is a deliberately complete vendored API surface: DialogTrigger/DialogClose in ui/dialog.tsx and SelectGroup/SelectLabel/SelectSeparator in ui/select.tsx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
selfcontained
added a commit
that referenced
this pull request
Aug 12, 2026
Two follow-ups from the same audit. Repo tools returned their stdout twice: RepoToolResult carries both `stdout` and `message`, and `message` is set to `result.stdout`. Both went into the structured payload, so every repo_dev_up/repo_dev_logs result arrived doubled. The MCP boundary now sends `message` as the text content only, and strips it — along with the agentId and repoRoot the caller supplied — from the structured payload. repo_dev_status: 145 bytes of structured payload, output present once. get_agent_history is removed. It returned 12.6KB at default args on a nearly empty database (include_feedback/include_reviews default true), no job prompt on this server calls it, and get_activity_summary / get_feedback_summary cover the same ground in aggregate. Removing the tool orphaned telemetry's getAgentHistory — 270 lines reachable only through it — so that and its tests go too, per the dead-surface policy settled in #944. The /api/v1/history routes behind the Activity pane are a separate path and are untouched. tools/list drops from 70,079 to 67,776 chars. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
selfcontained
added a commit
that referenced
this pull request
Aug 13, 2026
* Trim MCP tool responses to what a calling agent needs Every byte an MCP tool returns stays in the calling agent's context for the rest of its session. Several tools returned far more than a caller needs — list_templates overflowed the tool-output token limit outright at 108K chars, and a one-field brain_store_object update echoed the whole multi-KB object back. Two rules, applied across the tool surface: - JSON is emitted compact. Pretty-printing paid tokens for indentation no model needs (32 call sites). - List tools return a lean projection; the matching single-item tool (get_template, get_job, brain_get_object) remains the way to get everything. Per tool: - list_templates / list_jobs: prompt bodies replaced by promptChars; templates keep promptArgs. list_jobs also drops webhookSecret, a credential a listing has no use for. - brain_list_objects: strings inside listed values truncated at 400 chars, each marked with the count dropped. - brain_store_object: confirms the write (name, revision, timestamp) instead of echoing the stored value. - dispatch_pin: acknowledges created/updated without echoing the stored pin. - dispatch_list_pins: pin values truncated at 500 chars. - dispatch_review_list_feedback: drops the per-item diff hunk — a copy of code the caller can read at the reported path. Measured against a live dev instance, same requests before and after: list_templates 10,460 → 536 bytes, brain_list_objects 14,564 → 1,635, brain_store_object 14,461 → 335, dispatch_pin 292 → 120. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Walk values iteratively when truncating list responses truncateLongStrings recursed over agent-supplied JSON — a stored brain object or pin whose nesting depth nothing on this server bounds. A deeply nested value exhausted the call stack and failed the whole brain_list_objects request (verified: the recursive version throws RangeError at 50k depth). An explicit work stack has no such ceiling; the copy still preserves key order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Confirm entity writes instead of echoing the stored record create/update/delete on jobs, templates, and personalities echoed the whole entity back — mostly re-transmitting the caller's own request, and carrying the prompt it had just sent. update_template on a 9.8KB-prompt template returned ~10,400 chars; it now returns 108. Writes confirm identity: id, name, updatedAt, plus templateId on a created job (the auto-created backing template is the one id a caller cannot know). get_template / get_job / list_personalities remain the way to read the record back — including any server-applied defaults. Same treatment for two brain writes whose payload is caller-supplied: brain_append_event returns the assigned id and placement rather than the event value, and brain_list_set confirms index/length/revision rather than the item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Stop repeating repo tool output, and drop get_agent_history Two follow-ups from the same audit. Repo tools returned their stdout twice: RepoToolResult carries both `stdout` and `message`, and `message` is set to `result.stdout`. Both went into the structured payload, so every repo_dev_up/repo_dev_logs result arrived doubled. The MCP boundary now sends `message` as the text content only, and strips it — along with the agentId and repoRoot the caller supplied — from the structured payload. repo_dev_status: 145 bytes of structured payload, output present once. get_agent_history is removed. It returned 12.6KB at default args on a nearly empty database (include_feedback/include_reviews default true), no job prompt on this server calls it, and get_activity_summary / get_feedback_summary cover the same ground in aggregate. Removing the tool orphaned telemetry's getAgentHistory — 270 lines reachable only through it — so that and its tests go too, per the dead-surface policy settled in #944. The /api/v1/history routes behind the Activity pane are a separate path and are untouched. tools/list drops from 70,079 to 67,776 chars. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Split the remaining list-shaped tools from their detail reads Applies the pattern the rest of this PR established to the tools that still inlined their bulk, adding a detail read wherever one did not already exist. dispatch_review_list_feedback dropped message threads, which were 71% of a five-item listing (7,426 → 2,160 bytes), and reports messageCount instead. The new dispatch_review_get_feedback returns one item whole — its thread and the diff hunk the earlier commit removed from the listing, so that hunk is reachable again rather than merely gone. brain_query_events truncates strings inside event values like brain_list_objects already did, and the new brain_get_event (BrainStore.getEvent) reads one back in full. brain_list_get truncates the same way; its detail read is the offset and limit it already accepts. get_parent_context truncates the parent's pin values at the cap dispatch_list_pins uses. Left alone deliberately: persona_templates, whose instruction bodies are the point — it exists to show an agent what a persona file looks like, so trimming them would defeat it. list_personas and list_personalities are small enough in absolute terms (1.3KB, 193B) that a detail tool would cost more in schema, paid every session, than the listing costs today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Serve the two detail reads without lying or scanning Review feedback: two findings on the list/detail split. brain_list_get claimed offset/limit was the way to read an item in full, but truncation still applied to the narrowed window, so the documented full read could not actually return one. A window of exactly one item is now returned untruncated — that request is the detail read, not a smaller listing — and the description says so. dispatch_review_get_feedback resolved its item by loading every feedback item and thread message for every review the agent participates in. A review can carry up to 100 items, so the detail read cost more than the listing it was meant to relieve. New getFeedbackItemForAgent fetches the one row plus its thread, scoped by the same ownership predicate listFeedbackItemsForAgent uses (r.agent_id / assigned_agent_id / reviewer_agent_id), wired through as getReviewFeedbackItem. Verified live: an unowned item id returns "not found among this agent's reviews", and brain_list_get with limit 1 returns an 800-char value whole where the unbounded listing truncates it at 400. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop get_parent_context, add a single-pin detail read get_parent_context is removed. It handed a review agent its parent's pins and media as an unscoped snapshot, and nothing asked it to: no persona file, no review injection prompt, and no job references it — only the docs did. A reviewer's briefing already carries what it needs. If this comes back it should follow the rules inter-agent messaging follows rather than reading another agent's state directly. dispatch_list_pins takes an optional id, returning that one pin untruncated. Verified live: a 1,200-char shortcut prompt comes back whole by id where the listing caps it at 500. That gives dispatch_pins a real detail read to point at — its own response already returns thin summaries (id, label, group) from upsertPins, so it needed nothing else, and the value cap added there during the merge with main was a no-op on data that carries no values. Reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make get_feedback_summary a summary, with per-group detail Its groups carried the full text of every top finding — 85% of the response, growing with the corpus — when a caller reading a summary wants to know where the patterns are, not to read five descriptions per group. Groups now report topFindingCount alongside their counts and severity breakdown, and passing `group` with a group's key returns that one group's findings in full. 3,012 → 457 bytes on the dev instance; the detail call returns the same 3KB for the single group asked about. An unknown key errors rather than returning an empty result, so a typo cannot read as "no findings". The persona-review job doc now describes the two-step read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Report the true distinct-finding count, and validate the aggregate Review feedback on the summary split. topFindingCount counted the capped topFindings slice, so any group with five or more distinct findings reported 5 — a group of 40 was indistinguishable from a group of 5, while the description claimed it was the distinct count. The aggregate now carries `distinctFindings` (descCounts.size, computed before the slice) and the summary reports that. The boundary also took the callback's Record<string, unknown> on trust: a result without `groups` passed through as-is, so a malformed `{}` became an empty-looking answer, and a null entry in the array threw during destructuring. readGroups now validates the array and its entries, and a bad shape becomes a visible tool error. An unknown group key also names the keys that do exist and which group_by they belong to, so a typo — or a key from a different grouping — is recoverable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Settle one list/detail policy, and stop overloading limit Architecture review on the PR as a whole. `limit: 1` no longer means "untruncated". limit is a cardinality control — a caller passing 1 is asking for a small response, so making that the one request that returns an unbounded item inverts its intent and ties the response budget to a pagination knob. brain_list_get truncates every page now, and the new brain_get_list_item(collection, name, index) is the explicit read. response.ts states the policy rather than leaving each tool to invent one: a list returns a lean projection and there is always an explicit way to read one entry, in exactly two sanctioned shapes — a separate single-item tool, or an identity selector on the list tool (dispatch_list_pins `id`, get_feedback_summary `group`) that names one entry and can mean nothing else. A cardinality control never doubles as a detail read. One exported LIST_STRING_MAX replaces the per-module 400 and 500 caps, so pins and brain values now truncate alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Let a single list-item read assert the list has not moved Brad asked whether reading one entry is obvious for a caller. It is — but a brain list item is addressed by position, not by an id, and removeListItem reindexes everything after the removed item. So the two-step read this PR now recommends had a race the other detail reads do not: pins, events, objects and feedback items are all named by something stable. Demonstrated on the dev instance: read a 4-item list at revision 1, note index 2 is "C", let another agent remove index 0, then read index 2 again — it silently returns "D". brain_get_list_item now takes an optional expectedRevision, the revision brain_list_get already returns, and raises the store's existing revision_conflict instead of a different item. Same optimistic-concurrency idiom the brain's write tools use. The description says plainly that an index is a position, not an id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert the list-item revision guard Brad's call: reading a single list item is rare enough, and an agent that gets a shifted item will work it out, so the optimistic-concurrency guard was more machinery than the case warrants. Recording why it existed, for whoever meets it later: a brain list item is addressed by position and removeListItem reindexes everything after the removed item, so a list-then-read pair can return a different item than the one the caller saw — reproduced on a 4-item list, where reading index 2 after another agent removed index 0 returned "D" where "C" had been. brain_list_get returns the list revision, so a caller that cares can compare it itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Serve the whiteboard drawing guide from a tool, not every tool list Brad's idea. whiteboard_update's description carried a ~6KB Excalidraw element reference plus its workflow prose — 8,321 chars, 11% of the entire tools/list, charged to every agent on every session whether or not it ever drew anything. The guide now comes from whiteboard_howto, a 360-char tool that returns 6,807 chars on demand. whiteboard_update keeps what a caller needs to decide whether to call it (what it does, merge semantics) and points at the howto; a failed update appends the same pointer, so an agent that guessed at the element shape is told where the format lives rather than left to guess again. tools/list: 74,490 → 67,623 chars, the first cut below where this PR started. Reviewers keep whiteboard_get and never see the authoring guide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three buckets of unreferenced design-system code have been flagged by two prior audits and deliberately deferred both times (PR #807 and again in #934) because they are a judgement call, not clear-cut dead application code. This run settles the policy and applies it, so it does not get re-litigated a fourth time.
Deleted — dead weight:
apps/web/src/components/ui/sheet.tsx(134 lines). Zero importers repo-wide. The only hit anywhere is a string inside a DB seed fixture (db/seed/agents.ts:102, an example pin value that happens to say "shadcn Sheet").apps/web/src/lib/glass.ts:glassDivider,glassHover,glassActive,glassBadgeBg,primaryGlow. Their four siblings (glassPanel,glassSurface,glassOverlay,glassInset) are used by 11 modules and stay. Five of nine unused is past the point where a "complete palette" reading holds up.Kept, and annotated so the next audit stops flagging them:
DialogTrigger+DialogCloseinui/dialog.tsxSelectGroup+SelectLabel+SelectSeparatorinui/select.tsxThese are one-line re-exports of a vendored shadcn primitive whose surface is meant to mirror Radix's. Dropping parts of it just pushes the next consumer toward reaching into
DialogPrimitive/SelectPrimitivedirectly. Each site now carries a short comment saying so.Why this is tech debt
Unreferenced exports left behind by past refactors are the single largest debt category in this codebase (per the running audit notes). Dead code costs on every grep, every "is this used?" question, and every subsequent audit that re-discovers and re-defers it. The keep/delete line is now written down at the two places that matter.
Verification
Every symbol was re-verified with a fresh repo-wide grep covering
apps/,e2e/,bin/,scripts/,docs/, tests, and markdown — not justapps/web/src. For the glass tokens I additionally grepped their class-string values (hover:bg-white/[0.08],bg-white/[0.06],0_0_24px_hsl) in case they were interpolated intoclassNametemplates rather than imported as symbols, and confirmed there is no namespace import (import * as glass) that would defeat symbol-name search.class-variance-authoritysurvivessheet.tsx—badge.tsxandbutton.tsxstill use it, so no dependency is orphaned.Deliberate exclusions
glassOverlay's doc comment was the one adjacent edit: it listed "sheets" among its consumers, which stopped being true with this deletion. Retargeted to "command palette", which is a real consumer.sheet.tsxis not a pristine CLI file — it carries local customizations (hideCloseButton,overlayClassName, and an inlined copy of theglassOverlayclass string in itscvabase). So "just re-add it with the shadcn CLI" would not restore it byte-for-byte;git revertwould. Noting this explicitly since the cheap-to-re-add argument is what makes the deletion safe.Checks
pnpm run check,pnpm run finalize:web, web vitest (798 passed / 59 files), andpnpm run test:e2e(179 passed / 12 skipped) all green. Server vitest skipped — no backend files changed.Next run
The backlog is down to 6 items after this one and the remainder are small filler or judgement calls, which per the audit cadence means a fresh broad audit is due within a run or two.
🤖 Generated with Claude Code