fix: Collapse post-intake dual execution mode — one execution mode, orchestrators as sequencers#411
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR collapses the post-intake “dual execution mode” into a single model where apply/review/hydrate always run as dispatched sub-agents, and /fab-continue and _pipeline.md act as sequencers that own fab status transitions and interpret returned results/findings.
Changes:
- Rework
fab-continue.mdto make/fab-continuea one-stage sequencer post-intake (dispatch stage block, read results, then run status transitions). - Align
_preamble.md,_pipeline.md, specs, and memory docs to describe the universal “nofab statusin dispatched blocks” contract. - Add a Go regression test asserting rework transition-history shape is caller-identity-blind with respect to
driver.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kit/skills/fab-continue.md | Reframes post-intake stages as always-dispatched blocks; /fab-continue owns transitions as sequencer. |
| src/kit/skills/_preamble.md | Updates per-stage model-tier prose to reflect post-intake always-dispatch. |
| src/kit/skills/_pipeline.md | Clarifies orchestrator as pure sequencer and “universal block contract” framing. |
| src/go/fab/internal/status/mutators_test.go | Adds a history-shape regression test to protect the single-mode invariant. |
| fab/changes/260613-fgxx-collapse-post-intake-dual-mode/plan.md | Captures the change plan, requirements, and acceptance criteria for fgxx. |
| fab/changes/260613-fgxx-collapse-post-intake-dual-mode/.status.yaml | Updates the change’s local pipeline state/metrics for this PR’s change folder. |
| fab/changes/260613-fgxx-collapse-post-intake-dual-mode/.history.jsonl | Appends local change history entries corresponding to pipeline progress. |
| docs/specs/stage-models.md | Updates stage model/tier documentation for the single post-intake dispatch mode. |
| docs/specs/skills/SPEC-fab-continue.md | Mirrors the new /fab-continue sequencer/block model and universal contract. |
| docs/specs/skills/SPEC-_preamble.md | Mirrors _preamble.md per-stage model-tier prose updates. |
| docs/specs/skills/SPEC-_pipeline.md | Mirrors _pipeline.md pure-sequencer/universal-contract phrasing. |
| docs/memory/pipeline/schemas.md | Documents the new caller-identity-blind history-shape invariant + changelog entry. |
| docs/memory/pipeline/execution-skills.md | Updates pipeline execution memory docs + changelog entry for fgxx. |
| docs/memory/distribution/index.md | Updates index “Last Updated” date for distribution docs. |
| docs/memory/_shared/index.md | Updates index row to reflect the fgxx per-stage model-tier wording. |
| docs/memory/_shared/context-loading.md | Updates shared context-loading doc and changelog to reflect fgxx framing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **`active`** (intake) → Generate intake if missing and advance to `ready` (backward compat for interrupted generations) — main session, no dispatch | ||
| - **`active`/`ready`** (post-intake execution) → resolve the stage's model, dispatch the stage's block as a sub-agent, then finish/fail/reset per the returned result | ||
|
|
||
| **Sub-agent dispatch contract (post-intake stages).** For apply / review / hydrate, before dispatching run `fab resolve-agent <stage>` and pass the resolved model/effort into the Agent dispatch (empty model ⇒ omit/inherit; empty effort ⇒ omit) per `_preamble.md` § Subagent Dispatch → Per-Stage Model Resolution. Dispatch `/fab-continue` as a sub-agent (Agent tool, `general-purpose`) for the named Behavior section, and the prompt MUST include **"do NOT run `fab status` commands; return results only"** plus the standard subagent context files. The sequencer (this `/fab-continue` invocation) runs the `finish`/`fail`/`reset` transition after the sub-agent returns. This is the universal block contract — the block never owns its transitions, regardless of caller. |
Collaborator
Author
There was a problem hiding this comment.
Fixed — clarified the dispatch line to say "Dispatch a sub-agent (Agent tool, general-purpose) to execute the named Behavior section of this file" with an explicit "NOT a recursive /fab-continue invocation" note, removing the recursive-execution misread. (4173be2)
Comment on lines
163
to
+165
| ### Verdict | ||
|
|
||
| > The Verdict transitions are run by the **sequencer** (the manual `/fab-continue` invocation in Path A) on the verdict this block returns — not by the dispatched review block. In Paths B/C/D the orchestrator's Auto-Rework Loop (`_pipeline.md`) takes the equivalent actions autonomously. |
Collaborator
Author
There was a problem hiding this comment.
Skipped — the requested "sequencer-only, return findings then stop" guidance already exists in two explicit blockquotes guarding this block: the ## Review Behavior intro ("The block runs no fab status command and takes no §Verdict-style decision itself — it never branches on caller... shown here for the end-to-end picture") and the ### Verdict intro ("The Verdict transitions are run by the sequencer... not by the dispatched review block"). The dispatch prompt also carries the universal block contract "do NOT run fab status commands; return results only." Adding a third disclaimer would be redundant.
Comment on lines
+450
to
+454
| // behind collapsing the post-intake dual execution mode (260613-fgxx): the | ||
| // foreground/manual path (driver="") and the dispatched orchestrator path | ||
| // (driver="fab-fff") issue the SAME fab status call sequence, so the | ||
| // .status.yaml transition history they leave is byte-identical EXCEPT the | ||
| // optional driver annotation. The Go state machine is already caller-agnostic |
Collaborator
Author
There was a problem hiding this comment.
Fixed — reworded the function docstring from "byte-identical EXCEPT the optional driver annotation" to "agree on every caller-blind field (stage/action/from/reason) — equal modulo the per-run ts timestamp and the optional driver annotation," matching the test's actual contract and the transitionEntry struct's own docstring. Also corrected the stale ".status.yaml transition history" phrasing to ".history.jsonl transition entries." (4173be2)
| - **Target-state validation (k4ge)**: `lookupTransition` validates the resolved target state against the stage's allowed states (a `validateTarget` helper applied to both the stage-override and the default resolution path). A schema-forbidden combination — `advance ship`/`advance review-pr` (target `ready`) or `skip intake` (target `skipped`) — exits non-zero with `Cannot {event} stage '{stage}' — target state '{state}' is not allowed for this stage` and writes nothing, instead of writing a state that permanently bricks `fab preflight` ("State 'ready' not allowed for stage ship"). The schema is the single constraint source, so any future forbidden combo is closed automatically. The now-unreachable `stageTransitions["review-pr"]["advance"]` override row in `status.go` is a recorded deletion candidate (k4ge plan) — removing it is byte-identical behavior since the default `advance` row produces the same rejection. tb6f's review widened the candidate: the `advance`/`finish`/`reset` rows in *both* the review and review-pr overrides duplicate `defaultTransitions` byte-for-byte (only `start`/`fail` genuinely differ; `lookupTransition` falls through to the default table for events absent from a stage override), and the exhaustive matrix test now proves any such removal behavior-neutral | ||
| - **Cascade preserves `iterations` (k4ge)**: when the `reset`/`skip` cascade sets a stage to `pending`/`skipped`, a `stage_metrics` entry with `iterations > 0` is kept with only its `iterations` counter (timing fields `started_at`/`driver`/`completed_at` cleared; the next activation rewrites them); zero-iteration entries are still deleted, so no empty `{}` entries linger. This keeps `stage_metrics.review.iterations` truthful across the rework choreography's `fail review` + `reset apply`, making the cycle count `fab pr-meta` reports real. Preservation is uniform across all stages, not review-only — since tb6f it is regression-tested end-to-end through the public `Fail`/`Reset`/`Finish` functions (3-cycle rework choreography, iterations 1→2→3) and across the `skip` cascade; the test passes against the shipped implementation, confirming the PR #402 "1 cycle" anomaly is not a state-machine bug. See [change-lifecycle.md](change-lifecycle.md) for full `stage_metrics` semantics | ||
| - **Exhaustively tested (tb6f)**: `internal/status/transitions_test.go` walks all 216 cells (6 stages × 6 events × 6 from-states) of the two tables above and asserts every cell's outcome — target state, forbidden-target rejection, or no-valid-transition rejection — against hand-written copies of the tables, deliberately NOT the implementation's own vars, so a table regression cannot silently rewrite the test's expectations. The enumerations in this doc and that test pin the same spec | ||
| - **History shape is caller-identity-blind (fgxx)**: the `.status.yaml` / `.history.jsonl` transition history left by a conforming rework run depends only on the **call sequence**, never on caller identity. `driver` flows solely into `applyMetricsSideEffect` (it annotates `stage_metrics.driver` and the transition-log entry) — no state transition reads it — so the manual/foreground path (`driver=""`) and the dispatched-orchestrator path (`driver="fab-fff"`) issue the identical `Finish(intake) → Finish(apply) → [Fail(review) → Reset(apply) → Finish(apply)]×N` sequence and leave **byte-identical** history, differing only in the optional `driver` field. `internal/status/mutators_test.go`'s `TestHistoryShape_IdenticalRegardlessOfDriver` pins this: it drives the rework choreography twice (`driver=""` and `driver="fab-fff"`) and asserts the `.history.jsonl` stage-transition entries match in count/stage/action/from/reason, differing only where `driver` is recorded. This is the structural invariant that made collapsing the post-intake dual execution mode safe (fgxx — both modes already issued the same CLI calls); the test guards against a future skills-layer regression that diverges the two sequences |
Collaborator
Author
There was a problem hiding this comment.
Fixed — reworded the § Transitions bullet from "leave byte-identical history, differing only in the optional driver field" to "leave history of identical shape — agreeing on every caller-blind field (stage/action/from/reason), equal modulo the per-run ts timestamp and the optional driver field." (4173be2)
|
|
||
| | Change | Date | Summary | | ||
| |--------|------|---------| | ||
| | 260613-fgxx-collapse-post-intake-dual-mode | 2026-06-13 | **Collapsed the post-intake dual execution mode — one execution mode, orchestrators as pure sequencers** (skills+specs prose refactor + one Go test; no production Go change, no new CLI command). Guiding principle (newly named): **intake is the sole context boundary** — the main session runs up to and including intake; after intake the intake artifact IS the context and every post-intake stage (apply/review/hydrate) runs as a dispatched sub-agent fed by `intake.md` + `plan.md` + `docs/memory/`. `fab-continue.md`: the three caller-aware "When invoked as a subagent: do NOT run `fab status`" blockquote conditionals (Apply/Review/Hydrate Behavior) are **removed** — with one execution mode there is no second branch; plain `/fab-continue` is now a **one-stage sequencer** that runs `fab resolve-agent <stage>` then dispatches the stage's block exactly as the orchestrators do, and owns the `finish`/`fail`/`reset` transition after it returns. The "do NOT run `fab status`; return results only" prompt instruction is now the **universal block contract** (carried in the dispatch prompt), not a per-caller skip rule in the block; it is NOT re-encoded as any "skip §Verdict when subagent" flag. The interactive § Verdict rework menu (Path A) and `_pipeline.md`'s autonomous Auto-Rework Loop (Paths B/C/D) **remain** as the invocation-level failure-policy fork. Header note reconciled from "foreground = advisory-only" to the one-stage-sequencer framing. `_pipeline.md` Behavior note affirmed as **pure sequencer** (dispatch → read → proceed/loop/stop) with the universal-contract framing; per-cycle rework choreography (fail+reset pair) and the history-shape invariant **unchanged in semantics**. `_preamble.md` § Per-Stage Model Resolution "Foreground is advisory-only" minimally reconciled (residual advisory only for a genuinely no-dispatch run; closes Gap 1a — uniform tiering across apply/review/hydrate; full uniform-tier closure deferred to a follow-up). New Go test `TestHistoryShape_IdenticalRegardlessOfDriver` in `internal/status/mutators_test.go` pins that the manual (`driver=""`) and dispatched (`driver="fab-fff"`) paths leave byte-identical `.history.jsonl` shape, differing only in the optional `driver` field (see [schemas.md](schemas.md)). SPEC mirrors updated same-PR: `SPEC-fab-continue.md` (f006 → universal block contract), `SPEC-_pipeline.md`, `SPEC-_preamble.md`, `docs/specs/stage-models.md` § Foreground limitation. `runtime/operator.md` verified — no foreground-post-intake assumption (no-op). | |
Collaborator
Author
There was a problem hiding this comment.
Fixed — reworded the changelog entry from "leave byte-identical .history.jsonl shape, differing only in the optional driver field" to "leave .history.jsonl entries of identical shape — agreeing on every caller-blind field (stage/action/from/reason), equal modulo the per-run ts timestamp and the optional driver field." (4173be2)
|
|
||
| | Change | Date | Summary | | ||
| |--------|------|---------| | ||
| | 260613-fgxx-collapse-post-intake-dual-mode | 2026-06-13 | **History shape is caller-identity-blind** (test-only Go addition; no production state-machine change). New `TestHistoryShape_IdenticalRegardlessOfDriver` in `internal/status/mutators_test.go` pins that the rework choreography `Finish(intake) → Finish(apply) → [Fail(review) → Reset(apply) → Finish(apply)]×N` leaves byte-identical `.history.jsonl` transition entries (count/stage/action/from/reason) whether driven with `driver=""` (manual/foreground) or `driver="fab-fff"` (dispatched), differing only in the optional `driver` annotation — because `driver` flows only into `applyMetricsSideEffect`, never into a state transition. New "History shape is caller-identity-blind" bullet under § Transitions documents the invariant. This is the structural reason the post-intake dual-execution-mode collapse (fgxx — see [execution-skills.md](execution-skills.md)) was safe: both modes already issue the same CLI call sequence. | |
Collaborator
Author
There was a problem hiding this comment.
Fixed — reworded the changelog row from "leaves byte-identical .history.jsonl transition entries (count/stage/action/from/reason)... differing only in the optional driver annotation" to "entries that agree on every caller-blind field (count/stage/action/from/reason) — equal modulo the per-run ts timestamp and the optional driver annotation." (4173be2)
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.
Meta
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Impact:
impl: +31/−24 (net +7)
tests: +163/−0 (net +163)
total: +194/−24 (net +170) ← excludes
fab/,docs/Summary
The pipeline has exactly one context boundary — intake. Every post-intake stage should run as a dispatched subagent fed by artifacts, never by conversation history. This change eliminates the dual execution mode baked into
fab-continue.md's post-intake blocks: threedo NOT run fab status when dispatchedconditionals caused each block (apply, review, hydrate) to behave differently depending on whether it ran foreground or as a subagent. With one execution mode (always dispatched), orchestrators become pure sequencers — they dispatch a block, read the returned status/findings, and decide proceed/loop/stop — while the block behaviors become single shared definitions that both/fab-continue(manual) and_pipeline(auto) dispatch identically.Changes
/fab-continueALWAYS dispatch a subagent for its stage — same dispatch orchestrators already usedo NOT run fab statusconditionals fromfab-continue.md(apply ~line 100, review ~line 156, hydrate ~line 176)_pipeline.md) to pure sequencers: dispatch block → read returned status/findings → decide proceed/loop/stopmutators_test.goasserting the rework-loop history-shape invariant is caller-identity-blind (same.status.yamlstate regardless ofdriver=""vsdriver="fab-fff")_preamble.md,_pipeline.md, anddocs/specs/stage-models.mdprose to reflect single execution mode (minimal reconcile — full uniform-tier closure deferred to Change C)docs/specs/skills/SPEC-fab-continue.md,SPEC-_pipeline.md,SPEC-_preamble.md, anddocs/specs/stage-models.mdper constitution requirementsdocs/memory/pipeline/execution-skills.md,docs/memory/pipeline/schemas.md,docs/memory/_shared/context-loading.mdto document the new single-mode model