chore: remove dead code and unify two repeated patterns - #53
Merged
Conversation
The benchmark integration, the replay harness and the compaction result were only visible in source comments and CLAUDE.md. The README described the product and said nothing about how its defaults were chosen. States the finding that decides the default: compaction cuts peak prompt characters by 36-43% and is off, because implicit caching stopped entirely when it ran - 18.1M cached tokens of 23.2M became 1.1M of 11.6M. Numbers are taken from runtime/compaction.ts, not recomputed. Also states what the harness cannot answer, matching what it prints: cache rates belong to the original recordings and a modified assembly cannot inherit them.
handleSlashCommand returns handled: true for every /-prefixed input - an unknown command included, which is answered with a message rather than falling through. /exit is a registered command (commands/slash/commands.ts), so the literal check for it below the handled guard could never run. Removes it and says at the call site what reaches the agent instead.
About fifty comments named what the next line already said - // Generate unified diff, // Check if path is a directory, // Format output. They carry nothing a reader does not have, and they crowd out the comments that record a decision. Removed, and a few replaced with the reason the code is shaped that way: the write in editFile and writeFile sits below both approval exits, and a logged-out provider cannot be left as the default. Also removes a commented-out console.log in the onDone callback, and hoists glob's dynamic import of statSync to the top of the file.
Three of the store's four modal flows resolve through a private resolvePendingX helper. The edit flow predated it and repeated the same resolver lookup, state clear and emit in approve, reject and clear. Routes all three through resolvePendingEdit. The outcome is named rather than boolean because cancelling rejects where declining resolves false, and editFile reads the difference: a rejection is reported as cancelled, a false as 'Edit rejected ... Do not claim this edit was completed'. Nothing covered that distinction. Mutating the rejection to resolve false left the whole suite green - the overlay tests call clearPendingEdit only as teardown and swallow the rejection. Adds two tests that fail under exactly that mutation, for cancellation and for clearTimeline.
The index read-modify-write was spelled out at five call sites - the sequence that has already lost a row once, per the count check in summariesFor that heals it. updateIndex holds it once. The fallback is a parameter rather than a default because the five did not agree, and the disagreement is load-bearing: saving rebuilds from the files so the new row joins the others, pruning starts from empty, and removing does nothing at all, since writing an index for a project with no sessions would create the directory lazy creation exists to avoid. None of the three was covered. Collapsing rebuild to empty, and skip to empty, both left the whole suite green. Adds a test per fallback, asserted against the index on disk rather than through listSessions, which would have healed all three and hidden them. Each fails under its own mutation. This does not close the window between the read and the write. Two processes still interleave; summariesFor is still what catches it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
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.
Why
This started from a suspicion that the codebase carries too many comments. Measured, it does not: 3,983 comment lines against 19,572 non-test source lines, and almost all of them explain a decision rather than restate the line below. The ratio is unchanged by this branch — 3,981 / 19,552. About fifty comments that named what the next line already said were removed, and roughly the same number added saying why the code is shaped as it is.
The real problems were elsewhere, and most of them were not visible from reading:
runtime/compaction.tsrecords a genuine A/B — compaction cuts peak prompt characters by 36–43% and is off, because implicit caching stopped entirely when it ran — andREADME.mddescribed the product without mentioning any of it.tui/src/store/ui-store.tssolved the same problem two ways in one file. Three of its four modal flows settle through a privateresolvePendingXhelper; the edit flow predated it and repeated the resolver lookup, state clear and emit three times.config/sessions.tsspelled out the index read-modify-write at five call sites — the sequence that has already lost a row once, which is whysummariesForcompares row count to files on disk and rebuilds./exitbranch in the prompt, and a commented-outconsole.login theonDonecallback.What
5149d5dREADME.mdstates how the context work was measured, using the numbers already inruntime/compaction.ts— including that the harness reports cache rates for the original recordings only and a modified assembly cannot inherit them44725bf/exitbranch.handler.tsreturnshandled: truefor every/-prefixed input, unknown commands included, and/exitis registered atcommands/slash/commands.ts:211e4805daglob's dynamicimport("fs")to the top of the filee1c4156ui-store.ts: the edit flow settles through oneresolvePendingEditlike its siblings7721587config/sessions.ts: five index read-modify-writes go through oneupdateIndexTwo shapes are deliberate and worth review.
resolvePendingEdittakes"approved" | "rejected" | "cancelled"rather than a boolean. Cancelling rejects the waiting promise where declining resolves false, andeditFilereads the difference: a rejection is reported as "Edit cancelled", a false as "Edit rejected ... Do not claim this edit was completed". Collapsing them would report an interrupted turn as a refusal the user never made.updateIndextakes the missing-index behaviour as a required parameter rather than defaulting it, because the five call sites did not agree and the disagreement is load-bearing: saving rebuilds from the files so the new row joins the existing ones, pruning starts from empty, and removing does nothing at all — writing an index for a project with no sessions would create the directory lazy creation exists to avoid.updateIndexdoes not close the window between the read and the write. Two processes still interleave;summariesForis still what catches it. The alternative — a lock file or a single-writer queue — buys correctness this store does not need, since the count check already heals the case and costs one directory read.config/sessions.tswas left as one file. Splitting it into modules is a reasonable next step and does not belong in the same diff as a behavioural extraction.Verified
bun run verify --allon the tree as pushed:bun test $(git ls-files '*.test.ts' '*.test.tsx' | sort -r): 1508 pass, 0 fail.bun testruns: 1508 pass, 0 fail in all four.ui-store.test.ts62 pass,sessions.test.ts52 pass,app.overlay.test.tsx11 pass,persistence.e2e.test.ts16 pass.bun run replay:baselinebefore and after: identical — mean peak 126,563 characters, totals −44.4% over 932 iterations. No context change was intended and none happened.Five tests were added, because mutation testing showed four behaviours this branch depends on were covered by nothing. Each mutation was confirmed applied by grep before the run, not assumed:
clearPendingEditresolves false instead of rejecting"rebuild"fallback collapses to empty"skip"fallback collapses to empty"empty"fallback collapses to skipThe
ui-storecase is the one worth knowing about. A test named "resolves a dismissed approval as declined, not as an error" looks like it covers the rejection path and does not — it coversdismissTopModal, which declines. The cancel path was reached only by overlay tests that swallow the rejection as teardown. It read as covered and was not.The three index tests assert against the index file on disk rather than through
listSessions, which heals a wrong index by rebuilding and would have hidden all three.Not verified
config/paths.tsandconfig/sessions.ts, and CI runs both operating systems precisely because the config loader depends on path semantics that differ between them. That check happens on this PR, not before it./exitbranch removal and the modal settlement change are both on the keyboard path. The suite drives the store and mounted components; no real terminal, no real keypress. Ctrl+C over a live diff is unexercised./exititself has no test. Its removal rests on a static reading ofhandler.tsplus a green suite. IfregisterCommands()ever failed to run before a prompt was submitted, the deleted branch was a fallback and no longer is.