Skip to content

fix(branch): flush staged twig state transitions so workflow reads see fresh ADO (AB#3126) - #339

Merged
PolyphonyRequiem merged 1 commit into
mainfrom
feature/3126
May 12, 2026
Merged

fix(branch): flush staged twig state transitions so workflow reads see fresh ADO (AB#3126)#339
PolyphonyRequiem merged 1 commit into
mainfrom
feature/3126

Conversation

@PolyphonyRequiem

Copy link
Copy Markdown
Owner

Summary

Fixes AB#3126 -- primary_completer failing with ""must be in InProgress state category, but is in Proposed"" in dogfood Run #7.

Root cause (corrected)

The original AB#3126 issue body claimed ""no upstream step transitions the task from Proposed to InProgress"". That was wrong. branch next-impl ALREADY does the begin_implementation transition via twig.SetStateAsync -- the actual bug is that the staged change never reached ADO before primary_completer's polyphony validate ran.

twig state only stages locally; without twig sync the change sits in twig's pending queue. ValidateCommand reads via IWorkItemRepository without syncing, so it saw stale Proposed and refused implementation_complete. Empirically: failure timestamp 14:12:28; ADO state-change timestamp on the same item 14:17 (after my manual sync intervention while filing the issue).

Changes

Verb-level (BranchCommands)

  • branch next-impl -- post-state SyncAsync so the staged transition is durable before return.
  • branch close-scope -- batched post-loop SyncAsync (guarded by closed.Count > 0 so an empty loop doesn't push).

Workflow YAML (implement-merge-group.yaml)

  • primary_completer script: twig sync BEFORE polyphony validate (fresh read), twig sync AFTER twig state (durable write), plus ErrorActionPreference = Stop and PSNativeCommandUseErrorActionPreference = true so a failed sync halts rather than silently propagating stale state.
  • primary_router: explicit error route to a new primary_router_error_gate BEFORE the catch-all. Without this, a failed next-impl would route error through the catch-all to dependency_check, silently treating the failure as ""all items done"" and skipping every remaining task -- a separate latent bug surfaced by rubber-duck critique.
  • New primary_router_error_gate (human_gate, modeled on mg_pr_failed_gate_ado) with retry / abort options.

Convention

.github/skills/polyphony-cli-developer/SKILL.md gains a ""State-mutation durability -- when ADO must hold transition X"" section, sister-pattern to the existing ""Post-condition verification"" section. Verbs that mutate state via ITwigClient.SetStateAsync / PatchFieldsAsync and whose callers need that mutation to be visible to a different process MUST flush via SyncAsync before returning.

Tests

Three new regression tests using runner.Invocations ordering assertions:

  • NextImpl_HappyPath_FlushesStagedTransitionAfterSetState
  • CloseScope_BatchedTransitions_FlushesOnceAfterLoop
  • CloseScope_NoTransitions_DoesNotFlush

Full Polyphony.Tests suite: 3390/3394 pass (4 pre-existing skips). All lints green (jinja-resolver, prose-children, lifecycle-router-coverage, type-agnostic, no-tracked-state). conductor validate confirms the workflow.

Anti-fix

The original issue body proposed adding a primary_starter workflow node before impl_branch_ensure. That would mask the underlying durability bug rather than fix it, and would leave every other staged-but-unflushed mutation site silently broken. The fix belongs at the C# verb layer, with the workflow YAML carrying complementary twig sync calls in script nodes.

Out of scope (filed as follow-ups)

Other unflushed mutation sites discovered during review -- same bug shape, deferred for surgical scope:

  • AB#3127 -- ScopeCommands.TagMutationAsync
  • AB#3128 -- PlanCommands.SeedChildren parent tag/facet
  • AB#3129 -- apex-driver.yaml close_mark_satisfied + apex-item-dispatch.yaml terminal_satisfied workflow scripts
  • AB#3130 -- Polyphony.csproj relative ProjectReference to twig2 fails in per-run worktrees (filed under AB#3085)

Sibling PR

Parent fork session is working on AB#3125 (scope-review deadlock) which also touches implement-merge-group.yaml -- but in the scope-review section (~line 832), not near the changes here (lines 240, 591-619, and the new gate insertion). Should auto-merge.

Verification

  • dotnet build clean
  • dotnet test -- 3390 passed
  • conductor validate -- passes
  • All lints green
  • Rubber-duck critique applied (added explicit error route + pre-validate sync)
  • AB#3126 description updated in ADO with corrected analysis

AB#3126

…e fresh ADO (AB#3126)

Root cause: `twig state` only stages locally; without a follow-up `twig sync`
the change sits in twig's pending queue and never reaches ADO. `polyphony validate`
then reads stale `Proposed` state via IWorkItemRepository (which doesn't sync) and
refuses `implementation_complete` even though `next-impl` already issued the
`begin_implementation` transition.

Verb-level fixes:
* `branch next-impl` -- SyncAsync after SetStateAsync so the staged transition is
  durable before return.
* `branch close-scope` -- batched post-loop SyncAsync (guarded by closed.Count > 0)
  so multiple transitions amortize to a single push.

Workflow YAML fixes (implement-merge-group.yaml):
* `primary_completer` script: pre-validate `twig sync` (fresh read), post-state
  `twig sync` (durable write), plus `ErrorActionPreference = Stop` and
  `PSNativeCommandUseErrorActionPreference = true` so a failed sync halts
  rather than silently propagating stale state.
* `primary_router`: explicit error route to a new primary_router_error_gate
  before the catch-all. Without this, a failed next-impl would route `error`
  through the catch-all to dependency_check, silently treating the failure as
  `all items done` and skipping every remaining task.
* New `primary_router_error_gate` (human_gate, modeled on mg_pr_failed_gate_ado)
  surfaces the failure with retry/abort options.

Convention documented in `.github/skills/polyphony-cli-developer/SKILL.md` --
new `State-mutation durability` section, sister-pattern to the existing
`Post-condition verification` section.

Tests: 3 new regression tests using runner.Invocations ordering (NextImpl
HappyPath flush, close-scope batched flush, close-scope no-op skip). Full
Polyphony.Tests suite still 3390/3394 pass (4 pre-existing skips); all lints
green; conductor validate confirms the workflow.

Follow-ups filed under AB#3104: AB#3127 (ScopeCommands), AB#3128 (SeedChildren
parent tag), AB#3129 (close_mark_satisfied + terminal_satisfied workflow scripts).
AB#3130 filed under AB#3085 for the Polyphony.csproj relative-ProjectReference
quirk discovered while building inside the manual worktree.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PolyphonyRequiem
PolyphonyRequiem merged commit f5f3aed into main May 12, 2026
1 check passed
PolyphonyRequiem added a commit that referenced this pull request May 12, 2026
…8) (#340)

Sister-bug to AB#3126. `PlanCommands.SeedChildren` mutates the parent's
tags via `ITwigClient.PatchFieldsAsync` (stamping `polyphony:planned`
and any `polyphony:facets=...` override) but does NOT call
`SyncAsync` afterward. The change is staged in twig's local cache +
pending queue but never pushed to ADO. The next workflow step that
reads the parent's tag (e.g. `state_detector` in `apex-driver.yaml`
checking `polyphony:planned`) sees stale tag state. This interacts
with the AB#3106 children-seeded heuristic -- the architect emits
`polyphony:planned` but `state_detector` re-entry doesn't always
see it without a sync.

Verb-level fix:
* `plan seed-children` -- `SyncAsync` after `PatchFieldsAsync` so
  the staged parent-tag mutation is durable before the verb returns.
  Guarded inside the existing `if (addPlanned || facetsTagSet)` block
  so the no-op idempotent path still skips the round-trip.

Tests: 2 new regression tests using `runner.Invocations` ordering,
mirroring the AB#3126 pattern (`BranchCommandsNextImplTests` /
`BranchCommandsCloseScopeTests`):
* `SeedChildren_AfterParentTagPatch_FlushesViaSync` -- asserts a
  `twig sync` invocation occurs after the `twig patch` for the
  parent tag.
* `SeedChildren_NoParentTagPatch_DoesNotFlush` -- belt-and-suspenders;
  when no patch was needed (tag already present), no flush either.

`StubPatchOk` extended to also stub the follow-up sync, and a new
`StubSync` helper mirrors the BranchCommands pattern.

Convention: follows the `State-mutation durability` section in
`.github/skills/polyphony-cli-developer/SKILL.md` (added by PR #339).

Tests: full Polyphony.Tests suite 3392/3396 pass (4 pre-existing skips);
2 flaky environmental tests (`ProcessRunnerTests.RunAsync_TimeoutAfter
EmittedOutput_ThrowsProcessCanceledWithBufferedOutput` and
`LifecycleRouterScriptTests.IsRoot_FalseWhenWorkItemDiffersFromApex`)
intermittently fail under suite contention but pass in isolation. All
8 lint scripts green.

Related:
* AB#3126 -- original `BranchCommands.NextImpl` + `close-scope` fix
  (PR #339, commit f5f3aed).
* AB#3127 -- `ScopeCommands` sibling site (separate PR).
* AB#3129 -- `close_mark_satisfied` + `terminal_satisfied` workflow
  scripts (separate PR).
* AB#3104 -- Bucket-C policy controllability umbrella.

Co-authored-by: Daniel Green <dangreen@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PolyphonyRequiem added a commit that referenced this pull request May 12, 2026
…+ terminal_satisfied (AB#3129) (#341)

Sister-fix to AB#3126. Two workflow scripts in
`.conductor/registry/workflows/` invoked `twig state` to transition
work items but did NOT follow up with `twig sync`, leaving the
transition staged in twig's local pending queue rather than pushed to
ADO. Subsequent processes reading ADO without first calling sync saw
stale state.

Fixed sites
-----------
* `apex-driver.yaml` > `close_mark_satisfied` (apex-level close-out):
  added `twig sync` after the `twig state` invocation so the apex's
  terminal-state transition is durable in ADO before the workflow
  terminates. Also added the fail-fast prologue (`ErrorActionPreference`
  + `PSNativeCommandUseErrorActionPreference`) -- reinforces the
  AUDIT AB#3066 fail-loud design for this script.

* `apex-item-dispatch.yaml` > `terminal_satisfied` (per-item wave
  corridor): added `twig sync` after the `twig state` invocation
  inside the existing try/catch, plus the fail-fast prologue. The
  AB#3066 observable-error envelope still applies -- a failed sync
  now surfaces via stderr + the `error` / `error_code` output fields,
  allowing the apex driver outer loop to re-evaluate next-ready on
  the following pass.

No additional unflushed sites found in the other 11 workflow YAMLs;
`implement-merge-group.yaml` > `primary_completer` was fixed by
PR #339, the only other `twig state` call site in workflow scripts.

New lint check
--------------
Added Check 9 to `lint-apex-driver.ps1` to pin this convention going
forward: every PowerShell `script` node in the apex-driver suite that
calls `twig state` MUST declare the fail-fast prologue and follow the
`twig state` with a `twig sync` later in the body. Five new Pester
assertions cover the rule (two real-YAML pins on the two fixed sites,
three synthetic mutation tests for missing-sync / missing-prologue /
twig-note-only-no-trigger).

Tests
-----
* dotnet test Polyphony.slnx -c Release: 3392 pass / 4 pre-existing skip
* Invoke-Pester .conductor/registry/tests: 361 pass / 2 fail (both
  pre-existing on main -- `lint-plan-level` open_questions policy
  check, unrelated to AB#3129)
* Invoke-Pester tests/: 143 pass / 0 fail
* conductor validate apex-driver.yaml: exit 0
* conductor validate apex-item-dispatch.yaml: exit 0

Cross-references: AB#3126 (parent class-of-bug, fixed by PR #339),
AB#3127 + AB#3128 (sibling instances filed under AB#3104; AB#3128
landed in PR #340 just before this PR).

Co-authored-by: Daniel Green <dangreen@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant