Repository navigation
fix: apply the omp audit — card before model, tools only when registered - #270
Merged
Merged
Conversation
Seven findings from a quality pass over the session-model and oh-my-pi changes. No behaviour anybody asked for changes; three of these are promises the code was making and not keeping. **The card is announced before the model is recorded.** `spawn.Open` wrote the model between the row and the `session-opened` event, so a failed write returned with a row nobody could see — exactly the state the comment beside it forbids, and a retry would have added a second row. The model is an override read back at spawn, so losing it costs the provider's default and nothing else. **`HasTools` no longer promises tools no install could register.** Crush and oh-my-pi get theirs from an MCP server written into their config by this binary's path, and both installs leave that registration out when the path cannot be resolved — while `HasTools` kept answering yes off the plugin version alone, sending a relayed message to name a tool absent from that session's list. The condition now matches the install's (`registersServerAtInstall`), which fixes Crush and oh-my-pi in one place rather than the reported one. `lichBinary` moves onto the service so a test can say "this binary cannot be resolved" — the same reason `lookPath` is injected. **The oh-my-pi profile branch is tested.** `OMP_PROFILE` beats `PI_CODING_AGENT_DIR`, and that resolver exists twice by the house pattern (as the Claude Code pair does); only the resume copy was pinned. The install now proves it too, through a real install, because getting it backwards writes the extension where omp never scans and every report goes missing in silence. Also: `flagValue` replaces the trim-and-refuse-a-dash guard written twice in `nameArgs` and `modelArgs`; `ompExtensionsDir` names the directory literal the install and the version read shared; and the CHANGELOG no longer claims a card renames itself from the conversation title without qualification — the wiring is there, but no run has been observed generating one.
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.
A quality pass over #257 and #258 turned up seven findings; this applies all of them. No feature changes — three are promises the code was making and not keeping.
The three that matter
The card is announced before the model is recorded (
internal/spawn/spawn.go).Openwrote the model betweenAddSessionand thesession-openedevent, so a failed write returned with a row and no card — the exact state the comment beside it forbids ("a session only the database knows about is one the user cannot reach to see what went wrong"), and a retry would have left a second row behind. The model is an override read back at spawn, so a failure there now costs the provider's default and nothing else. Pinned by a test that injects the write failure and asserts the card still went out.HasToolsno longer promises tools no install could register (internal/agentplugin/agentplugin.go). Crush and oh-my-pi get their tools from an MCP server written into their config by this binary's path, and both installs deliberately leave that registration out when the path cannot be resolved (crushrcBlock,ompMCPDocument) — whileHasToolskept answering yes off the plugin version alone. A relayed message would then name a tool absent from that session's list. Fixed at the root, so Crush is covered too, not only the provider the audit reported.lichBinarymoves onto the service for the same reasonlookPathis injected: a test has to be able to say "this cannot be resolved".The oh-my-pi profile branch is tested (
internal/agentplugin/files_test.go).OMP_PROFILEbeatsPI_CODING_AGENT_DIR, and the resolver exists in two packages by the house pattern (the Claude Code pair does the same) — but only the resume copy was pinned. The install proves it now, through a real install into a redirected home, because getting the order backwards writes the extension where omp never scans and every report goes missing in silence.The rest
flagValuereplaces the trim-and-refuse-a-leading-dash guard thatnameArgsandmodelArgseach carried.ompExtensionsDirnames the"extensions"literal the install and the version read shared.Test plan
gofmt -l .clean,go vet ./...cleango test ./internal/...green (1352, +8),-raceonagentplugin,spawnandterminalgo build+go vetclean on eachbiome check,tsc --noEmit,vitest run(875) greenregistersServerAtInstallpinned as a list on both sides;HasToolsfalse for an installed omp with no resolvable binary and unchanged for opencode, whose plugin defines its own tools; the install followingOMP_PROFILEover the override