orchestrate: validate_run_state rejects resolvedRouting.fallback: null — non-premium slices should omit it, but the contract is ambiguous #333

Activity

1. rodrigorjsf

   added

   bugSomething isn't workingSomething isn't working

   on Jun 17, 2026
2. rodrigorjsf commented on Jun 21, 2026

   

   rodrigorjsf

   on Jun 21, 2026

   OwnerAuthor

   More actions

   New occurrence — orchestrate plugin v1.6.0, run prd1-20260621-011032

   Hit again on a successful autonomous /orchestrate 1 run — confirms this trap is still live in v1.6.0.

   What happened

   After freezing each slice's resolvedRouting per the SKILL spine (section 3 step 2) for non-premium (standard-tier) slices with no route:* label, I wrote "fallback": null — the natural reading of "no fallback" (and resolve_routing returned fallbacks: []).

   "resolvedRouting": { "model": "...", "variant": "...", "fallback": null, "fallbackTaken": false }

   Failure timing (supports the "fail fast" half of the fix)

   • validate_run_state passed at first-write time — but that checkpoint was written before resolvedRouting existed on the slices, so the bad shape was never in scope at that point.

   • At run end, all three render tools failed:

     run-state.json does not match the expected shape: slices.<N>.resolvedRouting.fallback: Expected object, received null
     

   • Removing the fallback key entirely (omitting it) made validate_run_state pass and all renders succeed.

   This is concrete evidence for suggested-fix option (b)'s second clause: validate_run_state should catch the null case at the first checkpoint write that contains resolvedRouting, not only at render time — otherwise a fully-successful run hits a hard error at the very last step and the operator must hand-edit run-state.json to strip fallback: null from every slice and re-validate before artifacts render.

   Both existing suggested fixes still apply: (1) relax the schema to treat null === absent (.nullish()), and (2) document that resolvedRouting.fallback must be OMITTED (never null) for non-premium slices, showing the no-fallback shape in the reference.
3. rodrigorjsf commented on Jun 21, 2026

   

   rodrigorjsf

   on Jun 21, 2026

   OwnerAuthor

   More actions

   Confirming recurrence — run prd23-20260621-184106 (PRD #23 in rodrigorjsf/ast-grep-for-agents)

   Hit this again on a fresh /orchestrate run, orchestrate v1.6.0. Following references/slice-pipeline.md step 2 ("freeze it into resolvedRouting"), the natural checkpoint for a slice with no route:* label (the common case — resolve_routing returns an empty fallbacks array) is "fallback": null. That validates fine through validate_run_state but then all three render tools fail at end-of-run:

   RUN_STATE_INVALID: slices.<id>.resolvedRouting.fallback: Expected object, received null
   

   Note the two validators disagree: validate_run_state returns valid for the same file the render tools reject — which is exactly why the bad shape survives the whole wave loop and only blows up at render.

   Workaround that worked here: OMIT the fallback key entirely from every slice (do not set it to null), then re-run the render tools.

   Reinforces the suggested fix in the issue body. Concretely the most robust resolution is all of: (a) relax the Zod schema to treat fallback: null === absent, (b) make validate_run_state reject fallback: null too so the inconsistency surfaces at the checkpoint write instead of at final render, and (c) document in references/slice-pipeline.md step 2 that when fallbacks is empty the fallback key must be OMITTED, not set to null.
4. rodrigorjsf commented on Jun 22, 2026

   

   rodrigorjsf

   on Jun 22, 2026

   OwnerAuthor

   More actions

   Confirming this is the exact run that surfaced it: `prd335-20260621-205440` (orchestrate plugin v1.6.0).

   Two additional details from the same run worth folding into the fix:

   1. Schema divergence between validate and render. The checkpoint was written with `"fallback": null` AND, for standard-tier slices, no `investigator` key. `validate_run_state` returned `valid` and the wave loop ran to completion — but `render_dashboard` / `render_graph` / `render_report` then ALL failed with `RUN_STATE_INVALID`:

      ```
      slices.X.resolvedRouting.investigator: Required; slices.X.resolvedRouting.fallback: Expected object, received null
      ```

      So beyond the null-vs-absent `fallback` trap, `validate_run_state` and the render tools enforce different schemas for `resolvedRouting`: a checkpoint that passes the fast-fail guard breaks at the final render step. The render path additionally requires `investigator` (which validate does not).

   2. Manual recovery. The fix was to edit run-state to add `"investigator": null` to every standard slice AND remove every `"fallback": null` line (fallback must be absent, not null).

   Suggested addition to the fix: make `validate_run_state` and the render tools share ONE canonical schema (ideally render calls the same validator), so a state that validates always renders. And clarify run-state.md that `investigator` is a required key (null for non-investigating tiers) while `fallback` must be omitted (never null).
5. rodrigorjsf commented on Jun 28, 2026

   

   rodrigorjsf

   on Jun 28, 2026

   OwnerAuthor

   More actions

   Autonomous pre-grill triage — AI-generated insights to seed a future grilling session. Not a decision.

   Evidence

   • The reported trap is real and still live in the schema. resolvedRoutingSchema.fallback is labelFallbackSchema.optional() (plugins/orchestrate/orchestrate-mcp/src/tools/render.ts:33). Zod .optional() accepts an absent key but rejects a literal null — so "fallback": null fails exactly as the issue states.
   • One of the issue's three claims is already obsolete. The June-22 comment's "two validators disagree / render additionally requires investigator" is no longer true: validate-run-state.ts:4 does import { runStateSchema } from "./render.js" and validateRunState parses against that exact exported schema (validate-run-state.ts:128). The file's own header comment (lines 11–15) explicitly says it "reuses the SAME exported runStateSchema the render tools validate against." Both paths now enforce one schema, including investigator: roleConfigSchemaV2.nullable() (render.ts:29, a required-but-nullable key). This consolidation shipped in PR orchestrate run prd310-20260611-162818 — PRD #310 model routing v2 (8/8 slices) #329 (commit 78ac377, model-routing v2), which is the v1.6.0 build the comments were filed against — so the divergence the operator observed was a real symptom of a now-merged-but-not-yet-cache-refreshed state, not a standing design gap.
   • The remaining true defect is timing, not schema divergence. validate_run_state runs "right after the first write and on resume" (validate-run-state.ts:14–15). But resolvedRouting is frozen per-slice during the wave loop, after that first write — so the bad fallback: null is never in scope when validate runs, and first surfaces at end-of-run render. The June-21 comment confirms exactly this: "validate_run_state passed at first-write time — but that checkpoint was written before resolvedRouting existed on the slices."
   • Doc gap is real and asymmetric. references/run-state.md gives subState an explicit "omit the key entirely; an explicit null is rejected" warning with the Zod rationale (lines 124–127), but fallback only gets "present when a label override carried a fallback, absent otherwise" (lines 148–149) — no "never null" warning. The JSON example (lines 72–79) shows a slice with fallback set; there is no example of the no-fallback shape an author would copy for the common case.
   • references/slice-pipeline.md:49–50 says an unrouted slice should "freeze no fallback" but does not say how (omit vs null). This is the line the operator was following when they wrote null.

   Ambiguities

   • The example at run-state.md:72–79 shows investigator: null (a standard/unrouted slice) but also fallback: {...} set — which contradicts the prose at slice-pipeline.md:50 that unrouted slices freeze no fallback. So the canonical example itself models an arguably-inconsistent combination, muddying what "correct" looks like.
   • The issue conflates three distinct fixes (relax schema, add a fast-fail at first resolvedRouting write, doc the omit-shape) without ranking them. Option (a) "treat null === absent via .nullish()" and option (b) "make validate reject null too" are in direct tension — one accepts null, the other hardens against it. The grill must pick a direction, not "do all of it."
   • "Non-premium slices should omit it" — but is fallback ever meaningfully present for a standard slice? If fallback only exists for premium/route:* slices, the whole field could move under the routed branch and the null case becomes structurally impossible.

   Honest doubts — impact & real value delivery

   • Severity is low and the workaround is trivial. Every reported occurrence ended in a successful run; recovery was "omit the fallback key and re-render." No subagent work is lost, no merge is corrupted — only the final render artifacts are blocked, and only until a one-line hand-edit. This is a polish/ergonomics bug, not a correctness bug.
   • It is self-inflicted by the orchestrator author, not a tool bug. The MCP schema is doing exactly what an optional field should. The recurrences all stem from the human/agent-authored checkpoint writing null against documented guidance. A pure doc fix (mirroring the orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300 subState treatment) may close 100% of real-world occurrences at near-zero cost — making the schema-relax + new-validator work arguably gold-plating.
   • orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300 precedent cuts against the schema change. The closed sibling orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300 (subState: null) was fixed doc-only and apparently has not recurred. If the same minimal fix worked there, the burden is on this issue to justify why fallback needs schema surgery (build + committed dist/ churn) when subState did not.
   • Risk of relaxing to .nullish(): it permanently legitimizes two shapes for the same meaning, which is precisely the kind of "null === absent" ambiguity the rest of run-state.md works hard to forbid (see the subState wording). It trades an author-time error for a latent schema looseness.

   Alternative approaches (>=3 distinct, numbered)

   1. Doc-only, mirror orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300. Add to run-state.md a "fallback must be OMITTED, never null" warning identical in spirit to the subState block, add a no-fallback JSON example, and fix slice-pipeline.md:50 to say "omit the key." Zero code, zero build, matches the precedent that already worked for subState. Lowest cost, closes the observed occurrences.
   2. Tighten the validator's timing, not its schema. Keep fallback strictly .optional(). Instead, call validate_run_state (or add a dedicated guard) at the first checkpoint write that contains resolvedRouting, so null fails in seconds at that write rather than at end-of-run render. This delivers the "fail fast" value the operator explicitly asked for without weakening the schema — and is the only option that addresses the actual root cause (timing), since schema divergence is already gone.
   3. Restructure so the null case is unrepresentable. Move fallback/fallbackTaken into a discriminated sub-object that only exists on premium/routed slices (e.g. resolvedRouting.premiumLane?: { fallback, fallbackTaken }). A standard slice has no place to put fallback, so the trap cannot be written. Larger refactor, but eliminates the class of bug rather than patching one field. Pairs naturally with the ambiguity that fallback may be meaningless for standard slices.
   4. Relax to .nullish() + doc (the issue's "do both"). Accept null and absent as equivalent and document the preferred shape. Most permissive; costs a dist/ rebuild; risks normalizing dual shapes (see doubts).

   Gotchas

   • Any src/ change requires npm run build and committing the rebuilt dist/ — vitest tests src/, so green tests do NOT prove the shipped bundle is fixed (project memory orchestrate-mcp dist/ is committed).
   • Because validateRunState and render now share runStateSchema, any schema edit changes both call sites at once — good for consistency, but it means a .nullish() relaxation also silently makes render accept null, which may not be intended.
   • The issue's comments were filed against a v1.6.0 cache that pre-dates (or coincides with) the orchestrate run prd310-20260611-162818 — PRD #310 model routing v2 (8/8 slices) #329 shared-schema merge; a grill must re-verify against current development before accepting the "two validators disagree" framing — that premise is already stale in the repo I read.
   • fallbackTaken: z.boolean().default(false) (render.ts:34) means the field can be omitted entirely and defaults — worth confirming the freeze guidance doesn't also redundantly write defaults.

   Points to clarify

   • Is fallback ever legitimately present on a non-premium / non-route:* slice? If never, option 3 (make it unrepresentable) is the principled fix.
   • Does the maintainer want null accepted (.nullish()) or rejected earlier (option 2)? These are mutually exclusive directions and the issue asks for both.
   • Should the canonical example at run-state.md:72–79 be split into a routed example (with fallback) and an unrouted example (no fallback, no investigator) to remove the current mixed-signal?
   • Given orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300 was doc-only and stuck, what new evidence justifies code work here beyond "ideally do both"?

   Other analytical lenses

   • Consistency lens: the real systemic issue is that run-state.md treats subState and fallback — two structurally identical optional-rejects-null fields — with different documentation rigor. A one-pass audit of every .optional() field in sliceSchema/resolvedRoutingSchema for the same null-trap (subState ✓ documented, fallback ✗, investigator is nullable so safe) would prevent the next orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300/orchestrate: validate_run_state rejects resolvedRouting.fallback: null — non-premium slices should omit it, but the contract is ambiguous #333 instead of patching the third instance reactively.
   • Cost-of-fix vs cost-of-bug lens: bug cost = one hand-edit at run end, rarely. Fix cost of options 2–4 = code + build + committed dist + review. The economics favor option 1 unless the fail-fast value is independently wanted.
   • Author-ergonomics lens: the deepest fix is making the guidance copyable — authors write null because the only example shows a populated field. A correct no-fallback example may outperform a schema change at preventing recurrence.

   Suggested grilling agenda

   1. Kill the stale premise first: confirm on current development that validate_run_state and render share runStateSchema (they do — validate-run-state.ts:4). Drop the "two validators disagree" framing; reframe the issue as timing + docs, not schema divergence.
   2. Force the direction choice: accept null (.nullish) vs reject-earlier (option 2) vs unrepresentable (option 3). They are mutually exclusive; "do both" is incoherent.
   3. Stress-test "do we need code at all?" against the orchestrate: run-state.md subState wording — say 'omit the key' (an explicit null is rejected) #300 doc-only precedent that has not recurred. Demand the justification for dist/ churn.
   4. Decide scope: patch fallback only, or audit all optional null-traps in the run-state schema in one pass (consistency lens).
   5. Resolve the example contradiction (run-state.md:72–79 shows unrouted-with-fallback vs slice-pipeline.md:50 unrouted-has-no-fallback) — whichever fix wins must ship a coherent canonical example.
6. rodrigorjsf

   added

   grill-primedPrimed with an autonomous pre-grill triagePrimed with an autonomous pre-grill triage

   on Jun 28, 2026
7. rodrigorjsf commented on Aug 3, 2026

   

   rodrigorjsf

   on Aug 3, 2026

   OwnerAuthor

   More actions

   Recurred in orchestrate run prd352-20260803-015333 (PRD #352, twelve slices). I filed #382 before checking the backlog — that was a de-duplication failure on my part; closing it as a duplicate of this issue. Its evidence is worth keeping here.

   What happened. The orchestrator wrote resolvedRouting.fallback: null into all twelve slices, following the spine's own wording — "the returned {model, variant, optional fallback}" — where null is the natural encoding of "there is no fallback", and reads as the companion to the sibling fallbackTaken: false.

   How late it surfaced, and why that is the real cost. Nothing rejected it until the very end, when all three render tools failed at once with twelve identical errors:

   slices.353.resolvedRouting.fallback: Expected object, received null
   ... (all 12)
   

   Twelve slices' worth of work — every implementation, review and merge — had already landed on an invalid checkpoint. It only stayed harmless because nothing else parses run-state.json through the schema: finalize_slice mutates raw JSON surgically rather than round-tripping.

   The structural reason it went unnoticed. validate_run_state's own description recommends calling it "right after writing the first run-state checkpoint … so a mis-shaped checkpoint fails in seconds rather than after expensive subagent work". That advice is exactly right — and the spine only wires it into the resume path. A fresh single-session run therefore never invokes it until something else happens to parse the file. Adding that call after the first slice's resolvedRouting is written would have turned a twelve-slice-late total failure into a two-second one.

   So this issue's ambiguity has a second half worth capturing: it is not only that the contract is unclear about the empty case, it is that nothing checks the contract when it is first written.

   Workaround used: deleted the fallback key wherever it was null, re-ran validate_run_state (valid), re-rendered all three artifacts successfully.
8. rodrigorjsf commented on Aug 3, 2026

   

   rodrigorjsf

   on Aug 3, 2026

   OwnerAuthor

   More actions

   A second, distinct defect in the same schema, found by a cross-slice review of PR #381 (PRD #352). Adding it here rather than opening a separate issue, since it is the same file and the same object — but note it is not the same failure: this issue is a rejected value, the following is a missing key.

   resolvedRoutingSchema does not enumerate slice-executor.

   • src/tools/routing.ts:862-864 — resolve_routing emits a slice-executor entry.
   • skills/orchestrate/references/run-state.md:159-161 — documents it as the first key of resolvedRouting.
   • src/tools/render.ts:26-33 — resolvedRoutingSchema declares only investigator, implementer, reviewer, conflict-resolver, fallback, fallbackTaken.
   • src/tools/validate-run-state.ts:4,128 imports that same schema as canonical.

   Why it matters. Zod strips unknown keys rather than rejecting, so a checkpoint whose resolvedRouting is missing the executor entry validates as valid. That is precisely the field the resume path re-spawns the executor from (SKILL.md:305-307), so the one gate that could catch a malformed checkpoint before a resume is blind to exactly the key delegation depends on. The dashboard also never renders the executor's routing.

   Why it is not P0. Nothing re-serializes parsed run-state: validate-run-state.ts and render.ts only safeParse, and finalize-slice.ts:233-336 edits the raw JSON object — its own comment at :234 says it deliberately avoids parsing through runStateSchema. So no field is dropped from disk today; the exposure is that validation cannot detect the omission.

   Suggested scope for this issue: if the fix here is going to touch resolvedRoutingSchema anyway to accept fallback: null, add the slice-executor key in the same pass. Both are the same class — the schema and its writers disagree about what a well-formed resolvedRouting looks like.