fix: three papercuts the end-to-end run turned up - #204
Merged
Conversation
**Editing the model list from the admin UI deleted the comments inside it.**
Adding two models through `PUT /api/admin/playbook-models` stripped three
comments from the checked-in playbook, including
# Sonnet 5 / Opus 5 reject temperature / top_p / top_k (HTTP 400).
# Opus 5 rejects a token budget ... so depth is `effort`.
which exist to stop someone re-adding a field that 400s. `write_playbook_models`
splices the file around the two model blocks, so comments *outside* survived,
but both blocks were re-rendered from parsed data, so comments *inside* could
not. They are now carried across for every key that survives the rewrite.
Two ruamel details made that harder than it sounds. A comment before the *first*
key of a nested mapping is attached to the parent's key, not to the key it
visually precedes — so the one above `max_tokens:` lives on `params`. And the
heading of the *next* top-level section is parked on the last key of this block;
carrying it forward printed `# Loop bounds.` twice, once where it belongs and
once inside the rewritten block. Column separates them: an inner comment is
indented, a section heading sits at column 0. An unchanged value now also keeps
the scalar ruamel parsed, so editing the list no longer restyles `effort: "high"`
into `effort: high`, and an unchanged rewrite is byte-identical.
**`--model` on `workingset distil` could not switch providers.** It replaced
only the model *name* and kept the tier's provider, so passing the id the API
and UI use — `anthropic/claude-sonnet-5` — sent that whole string to Anthropic
and came back `404 not_found_error`, as a full traceback because only
`NotDistillableError` was caught. It now resolves against the playbook's model
list, accepting either form, refuses an unknown one with the list of valid ids,
and `ProviderError` exits cleanly.
**The API dropped `working_set_id` from every Consultation it returned.** ADR-0036
makes the Working set the chain of Consultations on one problem, and that chain
is what distillation reads, so this was the domain's central relationship
missing from its own API. Nothing was broken at the time — distillation reads
the store directly and the web client never asked for the field — which is
exactly why it survived.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Three smaller defects from the same end-to-end run. Unrelated to each other, all small, all real.
1. Editing the model list from the admin UI deleted the comments inside it
Found by using the product as intended: adding two models through
PUT /api/admin/playbook-modelsstripped three comments from the checked-in playbook, includingwhich exists precisely to stop someone re-adding a field that 400s.
write_playbook_modelssplices the file around themodel:andextra_models:blocks, so comments outside survived — but both blocks are re-rendered from parsed data, so comments inside could not. The function docstring claimed flatly "All other content — comments, key order, formatting — is preserved."Two ruamel details made this harder than it sounds, and I got the first fix wrong before catching it:
max_tokens:is stored onparams, not onmax_tokens.# Loop bounds.twice — once where it belongs and once inside the rewritten block. Column separates them cleanly: an inner comment is indented (col=6), a section heading sits atcol=0.An unchanged value now also keeps the scalar ruamel parsed, so an edit no longer restyles
effort: "high"intoeffort: high. An unchanged rewrite is byte-identical, which is now a test.2.
--modelonworkingset distilcould not switch providersIt replaced only the model name and kept the tier's provider. So passing the id that
GET /api/modelsreturns and the UI uses:— as a full Python traceback, because only
NotDistillableErrorwas caught. And naming another provider's model would have sent it to the wrong API entirely.It now resolves against the playbook's model list, accepting either
provider/nameor the bare name, refuses an unknown one with the list of valid ids, and exits cleanly onProviderError. Both verified against the live instance.3. The API dropped
working_set_idfrom every Consultation_con()serialized id / author / title / created_at / updated_at / pinned_reason / session_id — but notworking_set_id.ADR-0036 makes the Working set the chain of Consultations on one problem, and that chain is what distillation reads, so this was the domain's central relationship missing from its own API. Nothing was broken at the time — distillation calls
consultations.for_working_set()on the store directly and the web client never declared the field — which is exactly why it survived.Verification
6 new tests. The playbook ones use a fixture with comments inside the model blocks; the existing fixture put every comment outside them, which is why it stayed green while the real file lost comments on every edit.
pytest -m "not slow and not requires_ollama and not requires_api_key"— 1317 passed. No protected path touched.🤖 Generated with Claude Code