[Spec 1286] consult: configurable per-lane models and per-review-type lane selection - #1341
Open
waleedkadous wants to merge 100 commits into
Open
[Spec 1286] consult: configurable per-lane models and per-review-type lane selection#1341waleedkadous wants to merge 100 commits into
waleedkadous wants to merge 100 commits into
Conversation
…sm to plan; record architect note Also adds the initial implementation plan.
Addresses codex iter1: validation invocation point (loadConfig), new listProtocolNames() cross-tier enumeration API, Phase 4 depends on Phase 3. Folds in #1288 two-layer default-preservation tests.
Adds canonicalProtocolName (alias-aware byProtocol lookup), findConfigSource (names the supplying config layer), and the three-part provider-rejection error contract incl. agy output capture.
… resolvers Adds consult.models / reasoningEffort / pricing and porch.consultation modelsByType / byProtocol to CodevConfig, with all validation invoked from loadConfig() so malformed config fails at load time (matching the existing harness-validation precedent). Model ids are validated for syntax only — the provider is the sole authority on existence, so no local id allowlist exists anywhere. Reasoning effort IS validated locally because it is a closed union; REASONING_EFFORTS is bound to the SDK's ModelReasoningEffort via 'satisfies' so an SDK change breaks the build rather than drifting silently (verified: adding a bogus member fails tsc). Adds cross-tier protocol enumeration to skeleton.ts (listProtocolNames, canonicalProtocolName, listReviewTypes). byProtocol keys accept aliases and are canonicalized on both sides, so byProtocol.spider applies to a project running as spir instead of silently no-opping. Review types come from the resolved protocol.json only, so a shadowed skeleton copy's types cannot leak in. Adds findConfigSource() to name which of the five config layers supplied a key, without threading provenance through deepMerge. 65 tests.
…our-tier discovery tests byProtocol.<name>.modelsByType: null passed the object guard (typeof null === 'object') and reached Object.entries(), raising a bare TypeError instead of a keyed config error — defeating the point of load-time validation. The same guard was written correctly one level up; the nested copy omitted the clause. Test gaps closed: discovery is claimed over four tiers but was only exercised over two — added cache-tier and skeleton-tier coverage plus cross-tier shadowing, and a table-driven suite asserting every null position raises a keyed Error rather than a TypeError. 65 -> 75 tests.
… porch next and done porch next and porch done each carried their own copy of the lane precedence logic, and the copies had drifted three ways: done did no lane-name validation, did not normalize a single-string value into a list, and wrapped config loading in a bare catch that turned any config error into a silent fall-back to protocol defaults. That drift is not cosmetic — next emitting one lane set while done demands another is a deadlock the user cannot debug, since neither command prints the set it derived. Both now call one exported resolveConsultationModels in porch/config.ts, which delegates to phase 1's resolveLaneComposition. Placed there rather than in next.ts because both commands already import ./config.js, so consolidating adds no new coupling between them. Removing done's catch is a deliberate behavior change and this phase's only regression risk: a workspace whose porch.consultation config is malformed today limps along on protocol defaults and will now fail loudly. That is the spec's fail-fast rule applied to an existing latent bug. All 453 pre-existing porch tests pass UNMODIFIED — the plan named them as the regression net and warned that needing to change them would be a signal to re-examine the code, not the test. Scenario 8 gets real next()/done() integration rather than the tautology it invited: asserting a shared function equals itself proves nothing. The tests drive both commands end-to-end, so they still fail if a private copy is ever reintroduced — mutation-verified by making done ignore config again, which fails the narrowing test. The paired unconfigured case (done must REJECT two of three review files) exists so the narrowing assertion cannot pass via a done that simply accepts anything.
…ort real lane count codex found that the end-to-end test could not fail: done() reports a missing review via process.exit(1), not a throw, so .rejects.toThrow() had nothing to intercept and the worker just exited. The test passed anyway — a green tick I had cited as evidence. Mocked process.exit following done-verification.test.ts, and made the assertion specific rather than a bare toThrow(), since 'some error' would also match a crash during fixture setup. Mutation-verified after the fix: disabling done's refusal now fails the test; it did not before. claude's two minor points, both taken. The hardcoded '3-way review' string is output this phase made wrong — a 2-lane PIR was told to expect three, with no way to tell a failed lane from one never asked for; it now derives the count from the same resolver. And the deleted catch, this phase's one real behavior change, is now pinned through done() end-to-end rather than only at the resolver, which could not prove the catch was gone from the call site.
…tated test comment claude's two minor iter2 points, both APPROVE-with-notes. The comment fix matters more than the code one. My test claimed to pin the deleted catch, but done() reaches loadCheckOverrides -> loadConfig before lane resolution, so the validator throws at the earlier call either way — the test proves the acceptance criterion (malformed config fails done) without proving anything about the catch site. The narrowing test is what does that. Recorded the distinction in the comment rather than deleting the test, since a comment that overstates a test's reach is exactly how false coverage survives review. Also drops a duplicate getVerifyConfig call; the function-scoped verifyConfig was already in scope.
…, fail-fast contract Documents both config blocks (consult.models / reasoningEffort / pricing, and porch.consultation.modelsByType / byProtocol), the five-level precedence ladder as an ordered list matching resolveLaneComposition's candidate order, and a worked byProtocol example keeping PIR at its CMAP-2 footprint under a widened workspace default — the case where config silently outranks a protocol that was deliberately designed to be cheap. States the asymmetry the plan called out, because it is the thing a reader will otherwise get wrong: reasoningEffort is a closed enum Codev validates locally at config load, while model ids are provider-authoritative and checked only for syntax. Someone who reads 'fails fast on invalid values' will reasonably expect a bad model id to be rejected at config time, and it will not be — what they get instead is a hard lane failure with the provider's error text, the config key named, and no review file written, so porch cannot advance on a lane that never ran. There is no model-id allowlist anywhere in Codev by design. Verified rather than asserted: every constant in the docs (lane names, effort values, pricing keys, id syntax, the medium default) was read from consult-lanes.ts and the codex dispatch site, and the documented example config was run through the real loadConfig and resolver in a temp workspace — PIR resolved to two lanes and SPIR to three, and injecting a typo'd lane name produced exactly the documented load-time error. Skeleton parity is diff-verified empty.
…-id, tighten pricing and gemini fail-fast Both reviewers independently named the same two defects. The config-layer list was wrong: no env layer exists and I had dropped the framework cache. I wrote that line from memory of how config stacks usually look rather than from config.ts, in a document whose whole value is being trusted instead of the code. Every other constant here was read out of the source; the one that wasn't is the one that was wrong. --model-id ships, parses, and appears in --help but was missing from the reference — the same 'registered, documented, inert' failure its own code comment cites as the thing it was built to avoid, one layer out. Now documented with the distinction users trip on (-m picks the lane, --model-id picks the model), precedence, supported lanes, and the hermes hard error. claude's catch on the gemini path was the sharpest, because my text was wrong rather than merely thin: the hard-failure gate keys on the RESOLVED id, not on config, so --model-id arms it too — a reader would have believed the flag left the lane in skip mode. Rewritten around the resolved id, and it now names what still skips even with an id (agy absent, unauthed, timed out, signal-killed), since those are environment causes rather than the model's fault. Also: pricing accepts only the codex lane and requires three finite non-negative rates, and the override outranks the shipped table for every model, not just unknown ones. Skeleton parity diff-verified empty.
…lete the agy skip list claude caught that the new section's only example does not run: cli.ts declares an optional positional subcommand, so a bare prompt string binds to it and anything that isn't 'stats' is rejected. Confirmed by running the command, not just reading the parser. A wrong example is worse than a missing one — the reader has no reason to doubt it and concludes the tool is broken. I had verified this document's config JSON by loading it for real and every constant against source, but never ran the one shell command I wrote. Swept the whole file rather than fixing the reported line: every other example already used --prompt/--prompt-file/--type/stats, so the new section was the lone departure from the file's own convention. Also completes the list of causes that still skip non-blockingly with a model id resolved: an agy run that exits 0 having produced no output skips too, which is the least intuitive member of that set and the one a reader would assume must be a hard failure.
…correct pricing and PIR examples codex found // comments in a .codev/config.json example; config.ts uses a bare JSON.parse, so copying it fails. Same class as iter2's non-running shell command, one format over — an example that can't be copy-pasted is worse than none, because the failure reads as 'Codev's parser is broken'. Annotations moved to prose, fences relabelled json. Swept instead of spot-fixing and found one codex didn't report: a pre-existing integrationBranch example (#1113) with the same defect. Then automated the check rather than re-reading — all six json blocks now verified through json.loads. Eyeballing is what let the first one through. claude's pricing nit is sharper than its label: my invented rates were ~4x below the shipped gpt-5.6-sol ones and unlabelled, so a copy-paste yields a confidently wrong cost — exactly what this key exists to prevent. The doc was teaching the bug it documents. Now uses the real shipped rates with an explicit 'take these from the provider' warning. The two PIR examples disagreed, and the worked one silently changed PIR's composition while its prose claimed to preserve it. PIR's shipped verify pair is [gemini, codex]; both examples now say so.
… concurrency bugs
codex's three PR-review findings. The satisfies binding caught removals and renames but not ADDITIONS — and the comment above it claimed all three. An SDK-added effort value would compile fine while Codev hard-rejected it as invalid, failing open in the direction the spec specifically required to break the build. Added an Exclude<...> extends never assertion for the other direction, and mutation-verified it: dropping a covered member now produces TS2322, which a type-level guard needs or it is indistinguishable from no guard. Its test was circular — it iterated the very list it validated, so it passed for any contents. Values are now pinned as literals at runtime; SDK drift is carried by the compile-time check, since no runtime test can enumerate a compile-time union. And the review's own header was wrong: I wrote 'all 6 phases approved unanimously, 93 commits' when status.yaml records phase_6 force-advanced at the iteration cap with codex still at REQUEST_CHANGES, over 96 commits. I summarized my own project from memory instead of reading the state file — the same habit that caused every phase_6 docs defect, which I had already written up as a lesson in this project's own thread. Force-advance is not approval; the review now says exactly which changes went unreviewed and how I verified them. Also records claude's three non-blocking findings as known limitations rather than fixing them at the PR gate, since each would override a spec requirement or widen scope.
Contributor
Author
|
Post-review update — CMAP-2 at the PR gate ran codex One code fix, and it's a real gap rather than polish: the reasoning-effort list was bound to the Codex SDK union with Two corrections to my own review, which codex caught and which matter for judging this at the gate:
My review header had claimed "6 phases, all approved unanimously". I'd summarized my own project from memory instead of reading Claude's three non-blocking findings are recorded under Known limitations and follow-up candidates in the review rather than fixed here, since each would mean overriding a spec requirement or widening scope at the gate: No behavior change beyond what the PR body already describes. |
…PROVE) Force-advance is not approval and self-verification does not close out a standing REQUEST_CHANGES, so a scoped codex pass covering exactly the un-re-reviewed surface — phase_6's iter3 docs fixes (2cb1e2f) and the three PR-gate CMAP fixes (251c867) — was run at the architect's direction. codex APPROVE (HIGH), no issues: strict-JSON examples parse, doc trees byte-identical, documented values match the implementation, the SDK exhaustiveness guard is bidirectional, replacement tests non-circular. Run with an explicit --output outside the porch project dir so it could not auto-persist and be miscounted as a phase review. Consult .txt outputs are gitignored repo-wide (.gitignore:59), so the verdict is preserved verbatim in a tracked .md rather than force-added as a raw artifact — same convention every other consult output on this project follows. Recorded in the review's per-phase history and in the PR body, where the gate reader sees it without opening the review.
Contributor
Author
|
Confirming pass: codex
Run at the architect's direction because force-advance is not approval and my own verification does not close out a reviewer's standing
Codex independently confirmed: strict-JSON examples parse, the two doc trees are byte-identical, documented values match the implementation, the SDK exhaustiveness guard is bidirectional, the replacement tests are non-circular, and the corrected review metadata matches Run with an explicit The PR body now carries the full review history up front — a table of what was and wasn't reviewer-approved, which specific changes went unreviewed, and this verdict closing them out. Net: every change on this branch is now reviewer-approved, either within its phase or by this pass. Nothing further from me — the |
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
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.
Closes #1286.
Lets a workspace choose which model each consult lane runs, and which lanes run at all — per
protocol and per review type — from
.codev/config.json.This removes the incentive behind the issue: the requesting workspace had been tier-2-shadowing
spir/aspir/pirprotocol.jsoncopies to change lane composition, recreating exactly thestale-shadow-copy rot class PR #1281 had just cleaned up (17 drifted files).
Absent config, nothing changes — ids included.
{ "consult": { "models": { "claude": "claude-opus-5", "codex": "gpt-5.6-sol" } }, "porch": { "consultation": { "models": ["gemini", "codex", "claude"], "byProtocol": { "pir": { "models": ["gemini", "codex"] } } } } }porch doneno longer swallows config errors. It previously wrapped config loading in a barecatchthat turned any error into a silent fall-back to protocol defaults.Who is affected: only a workspace whose
porch.consultationconfig is already malformed. Thatworkspace currently limps along on protocol defaults. After this change,
porch donefails with:The error names the offending key, the value, and the valid alternatives, so the fix is the next
thing the user does. A workspace with valid or absent config sees no change.
Worth noting the previous state was worse than either alternative:
porch nextalready failed thisway, so a typo made
nextrefuse to run whiledonequietly demanded a different lane set — withneither command printing the set it derived. Fail-fast, no fallbacks.
📋 Review history — what was and was not reviewer-approved
Stated in the PR, not only in the review file, so the gate reader can see it without opening
anything.
APPROVEPhase 6 ended iteration 3 with codex
REQUEST_CHANGES/ claudeAPPROVE, hitmax_iterations: 3, and porch force-advanced (force_advancedrecord instatus.yaml). The fixesfor codex's iter3 finding were committed but never re-reviewed. They are docs-only — JSON examples
made parseable, real pricing rates, PIR example consistency (
2cb1e2f7).The spec was likewise force-advanced at iteration 3; codex requested changes on all three passes,
each time on a genuinely different and valid defect, and its iter3 fix was committed post-review. An
architect-required 4th codex pass was run before planning (2 of its 4 findings were factually false
and are rebutted on file).
Force-advance is not approval, and my own verification does not close out a standing
REQUEST_CHANGES. So at the architect's direction a scoped confirming codex pass was run overexactly the un-re-reviewed surface — phase 6's iter3 docs fixes (
2cb1e2f7) plus the three PR-gateCMAP fixes (
251c867f, including the reverse exhaustiveness assertion).Codex independently confirmed the strict-JSON examples parse, the two doc trees are byte-identical,
documented values match the implementation, the SDK exhaustiveness guard is bidirectional, and the
replacement tests are non-circular. The verdict is reproduced verbatim at
codev/projects/1286-consult-configurable-per-lane-/1286-confirming-codex-scoped.md(run with anexplicit
--outputoutside the porch project dir so it could not auto-persist and be miscounted asa phase review; raw consult
.txtoutputs are gitignored repo-wide).Net: every change on this branch is now reviewer-approved, either within its phase or by this
confirming pass.
My first draft of the review said "6 phases, all approved unanimously, 93 commits". That was wrong
on both counts and codex caught it at the gate; corrected in the review, the thread, and here.
🐛 Two real concurrency bugs found and fixed in
metrics.tsBoth produced the same invisible symptom — a silently missing metrics row — because
recordMetricsswallows errors by design (a metrics failure must never take down a consultation).Both trigger on the normal path: a CMAP opens one
MetricsDBconnection per lane, in parallel.1. The migration was check-then-act.
PRAGMA table_infothenALTER TABLE, unserialized. Onthe first consultation after upgrading, all three lanes can see the column as absent; one adds it,
the others fail
duplicate column nameand lose their rows. Fixed with a fast path,BEGIN IMMEDIATE+ re-check inside the lock, and duplicate tolerance.2.
journal_mode = WALthrew SQLITE_BUSY.busy_timeoutwas set after the WAL pragma — anddoes not protect a journal-mode switch anyway, since that needs an exclusive lock no busy-handler
waits for. So concurrent opens of a non-WAL database threw straight out of the
MetricsDBconstructor.
stats.tsandanalytics.tsconstruct it unguarded, where the throw propagates.Fixed in
enableWal(): timeout first, skip the switch when already WAL, treatSQLITE_BUSYassuccess-by-someone-else.
Neither was introduced by this spec; the second surfaced only once reviewers forced the concurrency
test to stop running against a stale
dist/. Both fixes are mutation-verified (migration 5/6 runs,WAL 5/5 deterministic).
What's in it
lib/consult-lanes.ts,lib/skeleton.tshelpers)--model-idflag--modelpassthrough; skip-vs-hard-failure splitmodel_idmetrics column + guarded migration; codex costsnullrather than wrongporch nextandporch donePrecedence (highest first; first level present wins, levels do not merge):
byProtocol[P].modelsByType[T]→byProtocol[P].models→modelsByType[T]→models→the protocol's
verify.models.Design notes
the provider's call, so a new model works the day it ships. Reasoning effort is the deliberate
opposite: a closed enum bound to the Codex SDK type via
satisfies, so SDK drift breaks the build.consultation_metrics.modelstill holds the lane name (consult statsgroups on it); theresolved id went into a new
model_idcolumn.consult.modelsrejectshermes(no model selector → inert), whileporch.consultationlanelists still accept it.
byProtocol/modelsByTypekeys are errors, not warnings — a warning would leaveyou silently on the defaults you were trying to override.
is exactly how a SPIR-tuned 3-lane default silently inflates PIR.
Testing
642 tests across the consult, porch, and lane suites; full unit suite, build, and
tscall green.All 453 pre-existing porch tests pass unmodified — the plan named them as the regression net for
the resolver substitution.
Review with lessons learned:
codev/reviews/1286-consult-configurable-per-lane-.md.Note for the reviewer
Three assertions in this branch were caught by CMAP as structurally unable to fail (a
.rejects.toThrow()against aprocess.exit, a tautology, a file-existence check standing in for"the row landed"), and the docs phase needed four iterations, mostly for examples that didn't run.
Both are written up in the review. Every end-to-end assertion here is now mutation-verified.