feat: implement act apply-model and report baseline tripwire (#607)#616
Conversation
iamtoruk
left a comment
There was a problem hiding this comment.
The engine, thresholds, and apply/undo path all check out against #607 and the design comment: the six guardrails match, the write is a single model string into the project settings through the journaled runAction path, and the apply/undo round trip is byte-identical. Good work there.
One real defect blocks merge: the baseline tripwire compares incompatible scopes. recommendModelDefault captures preApplyRate per project (aggregateModelStats([project])), but modelDefaultRow in src/act/report.ts builds its mockProject from sessionsInWindow across ALL projects, so the post-apply rate is global. Consequences: a false quality regression warning when the candidate model is used worse in unrelated projects, and worse, a real per-project regression can be masked by good numbers elsewhere, which defeats the safety net. The under-20-edit-turns gate has the same problem, it counts global turns. The fix looks cheap: rec.changes[0].path already encodes the project root, so filter the sessions to that project before aggregating.
Related gap: the entire modelDefaultRow / tripwire path has no tests, and it is the least obvious code in the PR. Please add report.ts coverage for the tripwire firing, the clean case, and the not-measurable case.
Two minor nits, non-blocking: the realized cell renders formatTokens(0) for a measured row which reads as a token claim, a correlation marker would be clearer; and the candidate is identified via Object.keys(baseline.metrics)[0] insertion order, a labeled field would be safer.
Review fixes for #616: - modelDefaultRow and the under-20-edit-turns gate now aggregate only the target project's sessions (derived from changes[0].path, separators normalized before dirname), matching the per-project baseline captured at apply time instead of comparing against all projects. - baseline.candidateModel labels the candidate explicitly, with a backward-compatible fallback to metrics key order for existing journals. - measured model-default rows render a correlation marker instead of a formatTokens(0) token claim. - zero-matching-projects now reports an honest project-not-found note; clean rows route through confidenceFor like every other kind. - tests: tripwire fires on a same-project regression that global aggregation would mask (fails on pre-fix code), clean and not-measurable cases, Windows-separator journal paths with an excluded masking project.
|
Pushed the review fixes in 7cb1bd9:
Full suite green locally (the branch-local |
iamtoruk
left a comment
There was a problem hiding this comment.
Re-reviewed the update in depth. The per-project scoping fix is real (modelDefaultSessionsInWindow recovers the project root from the journal path and both the post-apply rate and the 20-edit-turn gate now count only that project), and the new report tests are exact-value pins that fail if the cross-project bug is reintroduced (traced the arithmetic: the masking-project fixture flips the note under global aggregation). Both nits fixed, the three small extras are coherent with the fixes. Ready to merge.
Summary
Implements the design-gated #607 path for model default recommendations from real compare data.
This PR adds the no-proxy version of model routing: CodeBurn can now notice when a cheaper same-provider model has matched the current dominant model's edit reliability for a project, show the evidence in
compareandoptimize, and let the user explicitly apply that recommendation withcodeburn act apply-model <project>.The implementation intentionally stays conservative:
effortLevelrecommendation in v1Closes #607.
Why this exists
The acting-layer epic deliberately avoided live routing and provider interception. #607 is the safe middle ground: if local history already shows that a project can use a cheaper model without losing edit reliability, CodeBurn can recommend a project-level default model change.
That keeps the decision reviewable and reversible. The user still controls the session and can override with
--modelwhen a particular task needs a different model.Design followed
This PR follows the thresholds and rollout described in the #607 design thread:
modelsetting.effortLevelis intentionally deferred because compare does not have per-effort evidence yet.What changed
Recommendation engine
Adds
src/act/model-defaults.ts.The engine consumes the existing compare stats pipeline and produces one recommendation per qualifying project. Each recommendation includes the evidence reviewers need to judge it:
The engine refuses to recommend when any guardrail is missing, including insufficient volume, stale observations, cost not actually lower, reliability below threshold, or provider mismatch.
Explicit apply command
Adds:
The command:
<project>/.claude/settings.jsonmodelkeyexpectedHashwhen the settings file already existsrunAction()askind: model-defaultThis command is intentionally explicit. It is not wired into
optimize --apply --yes.Compare and optimize surfaces
Adds low-key recommendation blocks in both places where users already inspect model and waste evidence:
codeburn comparecodeburn optimizeThese blocks are informational. They show the evidence and point to
codeburn act apply-model <project>instead of mutating anything.Act report integration
Extends
codeburn act reportformodel-defaultactions.Model default rows are not token or dollar claims. They are correlation-only quality checks:
quality regression, consider undoif post-apply one-shot rate drops more than 5 percentage pointscorrelation, not attributionThis keeps the accounting honest. We do not claim savings from a model-default change in v1 because the causal story is weaker than for config-token removals.
Reviewer guide
Suggested review order:
tests/act-model-defaults.test.tssrc/act/model-defaults.tssrc/act/cli.tsact apply-model <project>command.src/compare.tsxandsrc/optimize.tssrc/act/report.tsSafety model
This PR keeps the acting-layer invariants intact:
expectedHashmodel, nevereffortLeveloptimize --apply --yesdoes not apply model defaultsFiles changed
src/act/model-defaults.tssrc/act/cli.tscodeburn act apply-model <project>.src/act/report.tssrc/compare.tsxsrc/optimize.tstests/act-model-defaults.test.tsVerification
Local verification on branch
feat/issue-607-model-defaults:Result: clean.
Result: 8 tests passing.
Result: successful build, including dashboard build.
Result: 1500 of 1503 tests passing locally.
The 3 failing tests reproduce on
main, so they are not introduced by this PR:tests/cli-status-menubar.test.ts, 2 failures onmaintests/cli-proxy-path.test.ts, 1 failure onmainAlso verified before opening the PR:
._*files remainorigin/feat/issue-607-model-defaultsNotes for maintainers
The main thing to scrutinize is not whether the command works, but whether the recommendation should exist at all under borderline data. The implementation is deliberately biased toward silence. If volume, reliability, recency, cost, provider family, or debugging-heavy safety is unclear, it returns no recommendation.
That is intentional. A missing recommendation is much cheaper than a bad model default.