Skip to content

fix(continue-workflow): lead the load-steering block with the token, not the payload - #729

Open
comdaze wants to merge 1 commit into
awslabs:v2from
comdaze:steering-token-first
Open

fix(continue-workflow): lead the load-steering block with the token, not the payload#729
comdaze wants to merge 1 commit into
awslabs:v2from
comdaze:steering-token-first

Conversation

@comdaze

@comdaze comdaze commented Aug 9, 2026

Copy link
Copy Markdown

The defect

continuationReason() in the continue-workflow hook emits the rules_content payload before the continue "<token>" command. Measured on v2 at 2.5.59, with the existing t121 fixture:

token at offset 332, payload at offset 160

A rule bundle runs to many KB — 16,583 chars across 37 entries on a stock install — and a harness may truncate hook output. The token is therefore the part that gets cut.

Why that is a stuck loop rather than a lost message

Without the token there is nothing to act on. The reader sees a wall of rules, notes correctly that they are already on disk and ambient, and does nothing. The engine re-emits load-steering on the next turn, and again on the one after.

Observed live as seven identical deliveries with no progress, each one paying the full payload in context.

And re-running next is not a recovery: the chain's position lives in the token, so next returns the head of the chain with the same token every time. Draining a stuck chain meant reading the token out of the directive JSON by hand.

The fix is ordering, not size

Truncated rule text is recoverable — the method files are on disk and reach the model through each harness's always-on include regardless. A truncated token is not. So the actionable instruction leads and the bulk context follows.

Every clause keeps its existing wording; only the order changes. A comment records why, so a future tidy-up does not quietly restore the old order.

Test

t121's existing (a) load-steering reason carries exact content and continues the exact token case gains an ordering assertion:

expect(tokenAt).toBeLessThan(payloadAt);

Verified failing on unmodified v2 before the source change (the offsets above), passing after. Its existing toContain assertions on both clauses are untouched and still pass, which is what pins that this is a reorder and not a rewrite.

Verification

  • bun scripts/package.ts --check — clean
  • bun run typecheck — clean
  • bun test tests/unit/t68-version-changelog-sync.test.ts — 7/7 (version, badge, CHANGELOG agree at 2.5.60)
  • bunx biome check --error-on-warnings core harness scripts plugins tests — three pre-existing infos in tests/unit/t267-usage.test.ts, none in the files touched here (left alone deliberately — unrelated scope)
  • bash tests/run-tests.sh --smoke --unit — 205 files, two failing: t248-codekb-scope-diff and t255-workspace-sync, both reproducing identically on an unmodified v2 worktree

Version

Bumps to 2.5.60 with the README badge and a matching CHANGELOG.md entry, per the CONTRIBUTING.md checklist and t68. At the current release cadence this patch number will probably be taken before review completes — I will rebase and re-bump on request.


Context, since you reviewed #726: that one I closed rather than reworked — its premise was false, the resume menu it claimed was missing is fully implemented in the engine, and all four of your findings traced to that. This PR is a different shape on purpose: one behaviour, one file, an assertion verified red before the fix, and no claim that anything is absent.

…not the payload (2.5.60)

`continuationReason()` emitted the `rules_content` payload first and the
`continue "<token>"` command after it. A rule bundle runs to many KB —
16,583 chars across 37 entries measured on a stock install — and a harness
may truncate hook output, so the token was the part that got cut.

What that costs is not a lost message, it is a stuck loop. Without the
token there is nothing to act on: the reader sees a wall of rules, notes
correctly that they are already on disk and ambient, and does nothing. The
engine then re-emits load-steering on the next turn. Observed live as seven
identical deliveries with no progress, each one paying the full payload in
context.

Re-running `next` is not a recovery either — the chain's position lives in
the token, so `next` returns the head of the chain with the same token every
time. Draining a stuck chain meant reading the token out of the directive
JSON by hand.

The fix is ordering, not size. Truncated rule text is recoverable: the
method files are on disk and reach the model through each harness's
always-on include regardless. A truncated token is not. So the actionable
instruction leads and the bulk context follows. Every clause keeps its
existing wording; only the order changes.

`t121` gains an ordering assertion (index of token < index of payload).
Verified failing against the previous order before the fix was applied —
token at offset 332, payload at 160 — and passing after.

Verification: package.ts --check clean, typecheck clean, t68 version sync
7/7, biome reports only three pre-existing infos in
tests/unit/t267-usage.test.ts and none in the files touched here.
smoke+unit 205 files with two failing, both reproducing identically on an
unmodified v2 worktree (t248-codekb-scope-diff, t255-workspace-sync).

@apackeer apackeer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sean, I reviewed this change to the continue-workflow Stop-hook recovery for load-steering. It moves the opaque continue command ahead of the serialized rule payload so host-side truncation is less likely to strand the chain. The change is implemented in the shared core hook, projected across every harness distribution, and covered by a new t121 ordering assertion plus the 2.5.60 release metadata.

Directionally, this is sound. On origin/v2, the hook puts the large rule payload before the only actionable continuation token, and a fresh next restarts delivery at part one rather than recovering the lost position. The shared core hook is the right layer, the scope is proportionate, and bun scripts/package.ts --check confirms the dist/ edits are generated rather than hand-maintained.

I am requesting changes for one blocking semantic regression:

  1. core/hooks/aidlc-continue-workflow.ts:943 now tells the conductor to Run ... continue before it tells it to apply the current rules_content, changing the old apply ... before continuing / Then run sequence to Run / as you go. The steering contract requires the current chunk to be applied and retained before its opaque token is invoked (core/aidlc-common/protocols/stage-protocol.md:743, core/tools/aidlc-directive.ts:75). If the new imperative order is followed literally, the chain advances past the current chunk without retaining it. Please keep the literal command/token ahead of the payload, but phrase it as the command to run only after applying the payload below. The t121 assertion at tests/integration/t121-stop-hook-enforce.test.ts:789 should also pin that semantic order instead of checking only character offsets.
  2. CHANGELOG.md:6 says the wording is unchanged, but this patch changes before continuing / Then run to Run / as you go. Please make the release note match the corrected message.

Focused verification passed at tests/logs/2026-08-09T04-03-55Z with RESULT: PASS. The version sync test passed, all five generated harness trees are in sync, typecheck and the focused Biome check passed, and no new test number was introduced. The passing test does not clear the finding because it checks token position, not the required apply-before-continue instruction.

UX impact is otherwise narrow: there are no new or renamed commands, flags, stages, defaults, or structured output fields. The only runtime surface is the agent-visible Stop-hook nudge; putting the token earlier is consistent with the intended recovery, while telling the agent to run it before applying the current payload is not. The documented upgrade remains to re-copy the relevant dist/<harness>/ shell, with no CI or script break.

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.

2 participants