refactor(repl): split repl.go, dispatch both interfaces from one command table, and test the session's failure paths - #550
Merged
Conversation
repl.go carried the whole interactive session in one file: assembling it from the flags, the loop, driving a turn, the slash commands, and the record operations behind /seal, /verify, /export and /fork. Four seams come out as pure moves, each file headed by a line naming what belongs in it: repl.go 180 the session state, its memory, the loop repl_start.go 188 the front door and which interface runs repl_turn.go 263 one turn, from the line to a terminal event repl_command.go 184 the slash-command dispatch, /models and /model repl_record.go 140 provenance, seal, verify, export, fork No signature, body or doc comment changed; only the per-file import blocks were recomputed. Sorting every non-blank line of the old file and of the five new ones and diffing them shows the only additions are the package clauses, the imports and the headers. Splitting a file redistributes its cyclomatic total rather than reducing it, so the measures that moved here are lines per file and whether a reader can find what they came for. Signed-off-by: Ion Alpha <contact@ionalpha.io>
The session's slash commands were four hand-kept lists in four files: the
switch in replCommand, the switch in sessionHost.start, the completion
menu's completableCommands, and help.go's sessionCommands. Adding a
command meant editing all four, nothing checked them against each other,
and a command added to three of them would have shipped missing from the
fourth with a green build. help.go said the opposite in a comment, that
keeping the list in one place stopped /help drifting "from what the
dispatch actually handles"; it backed the listing and the footer only.
They agreed as of this commit, so this fixes no live bug. It removes the
way one gets introduced, which is what the duplication rule in AGENTS.md
asks for by the third copy.
A command is now one row of sessionCommands: the name, its alias, the
argument placeholder /help shows, the description, and a handler per
interface. The handlers stay separate because the interfaces genuinely
differ (the line one writes plain text and returns its error to the loop,
the full-screen one renders themed lines and runs the command as a queued
turn); what they no longer each decide is which commands exist. Both now
route through lookupCommand, /help and the completion menu read the same
rows, and TestCommandTableRowsAreComplete fails on a row missing either
handler, which is a nil call at the keystroke rather than a compile error.
replCommand goes from cyclomatic 24 to 2 and sessionHost.start from 20
to 6, because the count in both was the number of commands.
Three behaviour changes, all of them the full-screen interface adopting
what the line interface already did:
- it matches on the first word rather than the whole line, so "/seal x"
runs /seal there instead of going to the model as a prompt
- it matches case-insensitively, so /SEAL is /seal
- the completion menu offers commands in /help's order rather than its
own, which changes which six a broad partial shows
Left as it was: each full-screen handler still echoes its own name as a
literal ("/seal"), one per handler rather than a list, so a rename would
misprint the echo without breaking the command.
Signed-off-by: Ion Alpha <contact@ionalpha.io>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…isible
Splitting repl.go put its untested parts in their own files, where the gap
is legible: the paths that only run when something has already gone wrong.
None of them had a test, and each of them decides whether the session
reports a failure or continues past it into a worse one.
Fifteen tests, over the five behaviours worth pinning:
- the front door refuses a keymap, theme, data directory or harness CLI
it cannot use, before a conversation has begun rather than at the
first turn
- /model applies the switch and saves it as the default, and when only
the saving fails the switch still holds and says so; a switch to an
external harness mid-run is refused, naming what is driving the run
- an externally driven run declares its harness once, reports a
declaration that did not land, and says how many attested events the
record could not hold
- a memory notice reaches the operator through the full-screen notice
line when there is one, and the transcript otherwise
- a turn or a record operation on a run the store no longer holds fails
with that reason and leaves the session on the run it was on
Two of these needed the environment pinned rather than mocked: PATH is
emptied where a harness CLI must be absent, since the machine running the
tests may have one installed, and a data directory is a regular file where
nothing may be creatable under it, which fails the same way on every
platform.
Signed-off-by: Ion Alpha <contact@ionalpha.io>
…re tests Both tests made the data directory a regular file to get a write to fail. That breaks more than the write: with a file vault the credential is read from under the same directory, so on Linux the model failed to resolve and the test failed before reaching what it was about. Each now breaks exactly the path it means to: a directory where the recorded default model belongs, and a directory where the database file belongs. The rest of the data directory stays usable, and a directory cannot be written or opened as a file on any platform, so the failure is the same everywhere. Signed-off-by: Ion Alpha <contact@ionalpha.io>
A turn is assembled against the store and the thing that drives the loop,
and each of those can refuse. None of the three refusals had a test:
- a run whose recorded state cannot be decoded, so the conversation
cannot be reopened from where it left off
- a harness that builds no loop, which is how the external half of
assembly fails
- a record /replay cannot read, which without this reports an empty
transcript and reads as a run that did nothing
The damaged run is written through the store rather than around it, so it
is a state the store will actually hold: the spec is valid and the status
is well-formed JSON of the wrong shape, which is what a version skew or a
half-written checkpoint looks like from here.
Signed-off-by: Ion Alpha <contact@ionalpha.io>
The session's failure paths now have tests, which took the tree from 89.5% to 91.6%. The ratchet exists so coverage that has been earned cannot be spent quietly later, so it moves with it. Signed-off-by: Ion Alpha <contact@ionalpha.io>
ion-alpha-dev
changed the title
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Splits
cmd/flynn/repl.go(890 lines) into five files along the session's seams, replaces the four hand-kept lists of slash commands with one table both interfaces dispatch through, and covers the session's failure paths, which had no tests.repl.gorepl_start.gorepl_turn.gorepl_command.go/modelsand/modelrepl_record.gocommands.goWhy
The session's slash commands lived in four places: the switch in
replCommand, the switch insessionHost.start,completableCommandsincommand_complete.go, andsessionCommandsinhelp.go. Adding a command meant editing all four, nothing checked them against each other, and a command added to three of them would have shipped missing from the fourth with a green build.help.goclaimed the opposite in a comment, that keeping the list in one place stopped/helpdrifting "from what the dispatch actually handles". It backed the listing and the footer hint only; neither dispatcher read it.The four lists agreed as of this branch, so this fixes no live bug. It removes the way one gets introduced, which is what the duplication rule in
AGENTS.mdasks for by the third copy.A command is now one row: the name, its alias, the argument placeholder
/helpshows, the description, and a handler per interface. The two handlers stay separate because the interfaces genuinely differ, the line one writing plain text and returning its error to the loop while the full-screen one renders themed lines and runs the command as a queued turn. What they no longer each decide is which commands exist.Splitting the file also made its untested half legible: with the front door, the turn, the commands and the record operations in separate files, what had no test was all of one kind, the paths that only run once something has already gone wrong. Eighteen tests now cover them, and the tree's coverage goes from 89.5% to 91.6%, with
dev/coverage-floorraised to match so it cannot be spent later.How to verify
go test ./cmd/flynn/covers it. Four tests incommands_test.goare the ones that give the table teeth:TestCommandTableRowsAreCompletefails on a row missing either handler, which would otherwise be a nil call at the keystroke rather than a compile errorTestEveryCommandRunsInLineModedrives every command throughreplCommandand asserts the line was claimed rather than sent to the modelTestEveryCommandIsListedByHelpandTestEveryCommandIsOfferedByCompletionpin the listing and the completion menu to the same rowsTestLookupCommandMatchesHowCommandsAreTypedpins the matching rules the two interfaces now shareThe failure-path tests are in
repl_start_test.go,repl_model_test.go,repl_memory_test.go,repl_provenance_test.goandrepl_record_test.go: the front door refusing a keymap, theme, data store or harness CLI it cannot use;/modelapplying a switch, holding it when only the saved default fails, and refusing a harness swap mid-run; an external run declaring its harness once and reporting what the record could not hold; a memory notice reaching the full-screen notice line when there is one; and a turn,/replay, an export or a fork on a run the store no longer holds.The split half is a pure move. Sorting every non-blank line of the old
repl.goand of the five new files and diffing them shows nothing lost and the only additions being package clauses, imports and the file headers.Notes for reviewers
Three behaviour changes, all of them the full-screen interface adopting what the line interface already did:
/seal xruns/sealthere instead of reaching the model as a prompt/SEALis/seal/help's order rather than its own; with a six-item menu that changes which six a broad partial showssessionCommandsis a function rather than a package variable because/help's own row runsrenderHelpandrenderHelpreads the table, which as a variable is an initialisation cycle the compiler refuses. Between functions the same loop is legal.Left alone on purpose: each full-screen handler still echoes its own name as a literal (
h.echoPrompt("/seal")), one per handler rather than a list, so a rename would misprint the echo without breaking the command.Two of the new tests pin the environment rather than mocking it.
PATHis emptied where a harness CLI has to be absent, since the machine running the tests may have one installed. Where a write has to fail, a directory is created at the exact path a file belongs at, rather than making the whole data directory unwritable: with a file vault the credential is read from under that same directory, so breaking all of it fails the test before it reaches what it is about.Splitting a file redistributes its cyclomatic total rather than reducing it, so the split half should be judged on lines per file and navigability. The counts that did move belong to the table:
replCommandgoes from 24 to 2 andsessionHost.startfrom 20 to 6, because in both the count was the number of commands.This replaces #547, which was cut before #545 and #549 landed and conflicted with them. The one upstream change to
repl.goin that window,s.memory().describeRecall(s.out)in the/memorycase, moves with the rest of that case into the table row; the branch is otherwise identical, verified by diffing the two trees.Three tests fail on my Windows machine before and after this branch, all from one cause outside it: a local
codexnpm install with no vendored binary beside it.TestNewExternalAgent,TestExternalAdapterIsBuiltForEveryBackendandTestAttestedDeclarationMatchesRecordedEvents.