fix(goal): let the model hand a goal back at a milestone

[SparkofSpike]

Summary

An operator running Operate reports that an agent can no longer stop at a
milestone. It used to do a stage, stop, and ask what to do next; now it runs
until the goal terminates, which costs a lot of rework when the direction was
wrong.

Both halves of that are real, and they compound:

• Operate tells the model to create_goal for every substantive request
  (crates/tui/src/runtime_handoff.rs, the operate contract). Before that text
  landed, the contract claimed the host created the goal — which the same
  change's own commit message records as "the host never created one, contrary
  to the old text". So goals were rare in practice and the continuation loop
  rarely armed.
• Once a goal is Active, there is no milestone exit. The intra-turn continuation
  re-dispatches on any answer that leaves the goal Active
  (crates/tui/src/core/engine/turn_loop/continuation.rs), a completed turn
  re-arms another one (crates/tui/src/core/engine.rs,
  schedule_goal_continuation), and decide_continuation's default path is
  Continue (crates/runtime/src/goal_loop.rs). The model-side exits are
  complete and blocked only.

blocked is not a usable milestone exit: a blocker the model reported stays
until an explicit /goal resume (GoalState::resume_after_runtime_block only
resumes the runtime-set flag), so reporting a milestone that way strands the
work. And the tool text says to use it when progress cannot continue — not
when a stage is finished.

Changes

• update_goal gains status: "yield" (crates/tui/src/tools/goal.rs).
  The model finished a stage and the next step is the user's call. It pauses the
  goal, so neither continuation dispatcher re-arms it, while leaving it
  unfinished.
• The user's next message resumes it (crates/tui/src/core/engine.rs,
  resume_yielded_goal). This is the same shape as a runtime stop, which is
  already resumable by writing to it: the stop is not a judgement about the
  work. A reported blocker and a user-requested pause still require an explicit
  resume, and automated inputs still resume nothing.
• GoalPauseReason::Yielded (crates/protocol/src/lib.rs), so the pause
  names itself in the UI instead of reading as a user pause or a stall.
• The continuation prompt names the exit (crates/tui/src/prompts/text.rs),
  and the tool description says what a yield means.

Why not just soften blocked

Making a model-reported blocker resumable by the next message would erase the
distinction the runtime already draws: "this is blocked on a decision" versus
"the runtime stopped this". The first should keep needing an explicit resume —
it is the model telling the user something is wrong. A yield is the opposite:
nothing is wrong, the model is handing the turn back. Two states, two meanings.

After review

The adversarial review blocked the first version and the independent review
found the same host-side gap. Three fixes landed on top:

• yield is refused unless the goal is active (mark_yielded). Without
  that guard the tool could overwrite a NoProgress stall, a BudgetLimit
  pause, a user pause, or a blocker into a user-resumable hand-back — widening
  the resume contract this state exists to keep narrow. The guard is what makes
  "two states, two meanings" true at the tool boundary, not just at resume time.
• GoalPauseReason::Unrecognized (crates/protocol/src/lib.rs) with
  #[serde(other)]. The durable host stores the reason as JSON
  (crates/state/src/lib.rs), and a new variant with no fallback made a record
  written by this build fail an older build's goal load. Nothing constructs the
  variant; the resume path reads it as "not a hand-back".
• The pause reason survives a host projection.
  thread_goal_status_projection mapped every Paused status to
  GoalPauseReason::User, and sync_from_host_status hard-coded the same, so a
  host sync between a yield and the user's reply erased the hand-back and the
  goal silently stayed paused. Both now carry the record's own reason
  (sync_from_host_status_with_reason, and the projection takes it as an
  argument).

Tests gained: a_yield_cannot_overwrite_a_pause_reason_someone_else_set,
a_pause_reason_written_by_a_newer_build_still_reads, and two gate-level tests
(a_yielded_goal_is_not_rearmed_by_the_continuation_gate plus its active-goal
control, which proves the gate is not simply dead under test conditions — the
review's point that the earlier unit tests pinned GoalState rather than the
dispatcher).

Known limitations this does not close

A model can yield repeatedly without advancing the goal.
record_not_achieved is the only place the stall counter moves, and a yield
ends the turn before it runs, so the NoProgress net does not catch that
pattern — the user notices instead. Bounding it (for example, counting
consecutive yields that moved nothing) is a separate change.

The host-managed runtime may not persist the resumption in one pass. The
independent review traced this: resume_after_yield calls resume(None), which
mints a new goal_id, while the durable write path is revision-fenced on the
id the host bound at admission — so the settlement write can be refused and the
record stays at the paused revision until a later pass re-syncs it. This is
deliberately not changed here, because it is not specific to a yield: the
existing resume_after_runtime_block uses the identical resume(None) shape and
is subject to the same fencing. Making a hand-back persist in one pass means
teaching the fence about a resumed revision, which changes a contract every goal
transition shares; that belongs in its own change with its own review, not
folded into this one. The engine-local (TUI) path, where the goal state gates
the loop directly, is unaffected.

What this does not do

It does not make a goal stop on its own. A goal still runs to completion unless
the model yields or the user intervenes — [goal] max_continuations,
[goal] max_steps, and /goal pause remain the operator controls. Whether
Operate should create a goal for every substantive request is a product
question this change deliberately leaves alone; it only makes the milestone
that the prompt already asks the model to report a thing the model can actually
do.

Testing

cargo test -p codewhale-tui --lib goal — 147 passed; 0 failed
(14,663 filtered out). New:

• a_yield_stops_continuation_and_the_users_message_resumes_it — a yielded goal
  is Paused with GoalPauseReason::Yielded, is not active, and
  resume_after_yield brings it back.
• a_user_pause_is_not_resumed_by_a_yield_resume — the resume refuses any
  pause that is not a yield.
• a_yield_cannot_overwrite_a_pause_reason_someone_else_set — a user pause, a
  budget stop, a usage stop, a stall, and a blocker all survive a rejected
  yield.
• a_pause_reason_written_by_a_newer_build_still_reads — the durable store's
  forward compatibility, plus the yielded round-trip.
• a_yielded_goal_is_not_rearmed_by_the_continuation_gate and
  an_active_goal_is_still_rearmed_by_the_continuation_gate — the cross-turn
  gate itself, with the control that keeps the first from passing vacuously.
• update_goal_yield_pauses_without_completing_or_blocking — the tool path
  pauses the goal, reports no blocker, and claims no completion.

update_goal_rejects_model_resume still passes (its expected message text was
updated for the new status list), so the model still cannot resume itself.
cargo fmt --all -- --check is clean.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Updated docs or comments as needed
• Added or updated tests where relevant

Related Issues

No-Issue: operator report on 0.10.1; no upstream issue filed for it.

Attribution

🤖 Generated by SpikeBot 000(CodeWhale-LOCAL)

[SparkofSpike]

Adversarial review — PR #10 fix(goal): let the model hand a goal back at a milestone

Verdict: BLOCK. The headline mechanism is sound (a Paused goal does stop both continuation dispatchers), but three gaps are material: (1) the yield tool has no current-state guard and can overwrite an unrelated pause/blocker into a user-resumable Yielded — the resume-widening this PR explicitly says it prevents; (2) the new serializable variant breaks old-client reads of the shared durable store the PR claims to support; (3) the three new tests do not exercise the behavior they attribute to the dispatchers, and the host-side projection can silently erase the Yielded reason so a host-managed resume silently fails.

I could not build codewhale-tui here (25–30 min, out of scope per instructions), so I did not run the goal tests; all findings below are static with crates/tui/src/tools/goal.rs on 8491c12. The -p codewhale-execpolicy suite does not cover this change and I did not run it.

---

1. update_goal(status:"yield") has no state guard and can widen the resume contract it is meant to narrow — confirmed, material

crates/tui/src/tools/goal.rs:1238 routes "yield" straight to mark_yielded() with no check that the goal is currently Active. mark_yielded → mark_paused(Yielded) (goal.rs:560-578) refuses only objective.is_none() and completion_sealed(). It does not refuse a goal that is already Paused for another reason, nor a Blocked goal, and it unconditionally does:

self.pause_reason = Some(Yielded);
self.evidence = None;
self.blocker = None;

The PR's own stated contract is "blockers the model reported and pauses the user asked for stay until an explicit resume." That invariant lives only in resume_after_yield() (goal.rs:517-524), which checks the reason at resume time — not what the yield overwrote:

if !(self.status == Some(Paused) && self.pause_reason == Some(Yielded)) { return false; }

So any tool-visible window where update_goal(status:"yield") can run against a non-Active goal silently converts a NoProgress stall, a BudgetLimit pause, a User pause, or a Blocked blocker into a user-message-resumable yield. The PR says guarding this is the point ("two states, two meanings"); the tool layer has no such guard. Contrast mark_blocked (goal.rs:532-546) which at least clears runtime_blocked and sets a real blocker, and mark_paused(User) which is only reachable from controlled host/UI paths — update_goal is model-reachable and open. Minimal fix: reject yield unless state.is_active() (goal.rs:153).

Severity: high enough to block.

2. New GoalPauseReason::Yielded breaks old-client reads of the shared durable store — confirmed (old→new safe; new→old fails)

crates/protocol/src/lib.rs adds Yielded to a #[serde(rename_all="snake_case")] enum with no #[serde(other)] fallback. The durable host wrote it into SQLite:

• write: crates/state/src/lib.rs:1891-1893 → serde_json::to_string(&reason) → column "yielded".
• read: crates/state/src/lib.rs:2108-2125 → serde_json::from_str::<GoalPauseReason>("yielded") → errors on an old binary (unknown variant), converted to FromSqlConversionFailure.

Contrast the status column this PR touches: thread_goal_status_from_str (state/src/lib.rs:2060-2071) deliberately fails closed on unknown persisted values. The new pause_reason has no such tolerance. The PR body explicitly says the variant is "shared with the durable host"; a store written by this build is unreadable by a prior build's goal load. In a single-version monorepo replay this is low-impact, but if the claim is "durable/host-compatible," this is a real non-round-trip across versions and is asymmetrical with the sibling field's stated policy.

5. The three new tests do NOT test the dispatchers, and one attributes behavior it never asserts — confirmed

• a_yield_stops_continuation_and_the_users_message_resumes_it (goal.rs:1285) calls only GoalState::mark_yielded()/resume_after_yield() and asserts is_active() == false. Its doc comment claims "neither continuation dispatcher re-arms a non-active goal," but neither goal_continuation_allowed (crates/tui/src/core/engine/turn_loop.rs:5161) nor goal_continuation_message_if_needed (turn_loop.rs:5188) nor goal_continuation_if_active (engine.rs:4919) is exercised. The test pins GoalState, not the dispatchers.
• No test drives the resume through handle_admitted_message/resume_yielded_goal (engine.rs:5191, 6120) — so the very case the PR is about (a user's next message resumes) is not covered at engine/turn level.
• update_goal_rejects_model_resume asserts the model cannot send status:"active" — behavior fine, but the changed expected text now pins the message ("complete, blocked, or yield"), which will churn every time the status list grows; the syntax-level behavior (rejection) is unchanged. Fine, but the assertion message is a weak gate.

The "yield stops both dispatchers" claim is true in code, but unpinned. A regression in the dispatchers would ship green.

6. Resume reason can be silently dropped on the host-managed path — suspected (could not confirm timing)

crates/tui/src/core/engine.rs:6120 routes GoalStatus::Paused to resume_yielded_goal, which relies on the in-memory pause_reason == Yielded. But the durable projection thread_goal_status_projection maps ThreadGoalStatus::Paused → (Paused, User) (goal.rs:766-769) — it has no Paused-with-Yielded case, so sync_runtime_goal_control (crates/tui/src/core/engine/handle.rs:299-337) reprojecting a durable Paused into the engine sets pause_reason = User (goal.rs:157-215, sync_from_host_status branch at ~214). mark_yielded's durable record survives (from_thread_goal reads pause_reason directly, goal.rs:735-757), so a clean reload retains Yielded, but any host sync in between (PUT/DELETE/complete/block endpoints → sync_engine_goal_status, crates/tui/src/runtime_threads.rs:8794, runtime_threads.rs:8411/8463/8515) supplies the durable Paused to the engine and erases the reason, so the next user message's resume_after_yield() returns false and the goal silently stays paused. This is resume too narrow (feature silent-fails), the opposite of the stated goal. I did not trace the exact callback ordering between a yield turn and the user's next admitted message to a resident host thread, so this is suspected, not confirmed.

12. Launch-time model abuse: the stall/NoProgress safety net is structurally bypassed by yield — confirmed (design)

record_not_achieved is the only place repeated_gap_count advances (crates/tui/src/goals/goal.rs:461); mark_paused/mark_yielded never touches it, and resume() resets it to 0 (goal.rs:171-174). After a yield the turn ends, so no record_not_achieved can run; after the user's resume the counter is reset. Hence a goal that yields at every milestone with zero progressnever trips MAX_REPEATED_GAP_PASSES/:stall. The only bound is the human answering each yield. That is a defensible product tradeoff (each resume costs a human turn), but the PR body claims the stall machinery scenarios ("the user to notice / compare with NoProgress") — be aware they don't apply here. The tool-gate gap from (1) is what makes this worse than "human answers each time": if a pause from the auto-stall can be re-labeled Yielded by the model, the stall itself no longer binds.

4 / 7 / residues: what I could not verify, and what is clean

• Prompt / cache prefix (attack 5): clean. GOAL_CONTINUATION_PROMPT (text.rs:174) is a runtime-const; its only delivery is render_continuation_prompt → goal_continuation_message_if_needed → add_session_message(Runtime) (engine/turn_loop.rs:5210). It is never spliced into the pinned BASE_PROMPT/system prefix (docs/CACHE.md:12-24). The prompt edit is cache-safe and the "KV-cache effect: none" comment at engine.rs:5991 matches AGENTS.md’s frozen-prefix rule.
• Does yield actually stop the loop? Yes in the versions I traced: goal_continuation_allowed→goal_snapshot_with_current_turn_usage (turn_loop.rs:5161 → ~5144) and goal_continuation_if_active (engine.rs:4919) both early-return on !snapshot.is_active(), and a held scheduled_goal_continuation/queued Op::ContinueGoal is discarded because the handler re-checks live state. I did not find a delayed/queued continuation race that re-arms a Paused goal. This claim holds.
• Host loop-stop: settle_thread_goal_after_turn re-arms only when durable goal.status == Active (runtime_threads.rs:8894); a yielded goal lands durable Paused, so no auto-refarm. Confirmed consistent.
• Test execution: I did not run cargo test -p codewhale-tui --lib goal (build window/prohibited here); the "143 passed" is taken from the PR body.

Fix recommendations (blocks)

1. Add an is_active() precondition to update_goal's "yield" arm (or make mark_yielded refuse non-Active), per (1), and differ from (3) so a yield never reclassifies a stall/budget/user pause.
2. Make the persisted pause_reason tolerant of unknown values (mirror thread_goal_status_from_str's fail-closed policy) — or document the single-version assumption explicitly — and consider a Paused sub-reason in thread_goal_status_projection so the Yielded vs User reason survives every host sync surface, not just clean reloads (2, 6).
3. Add a real engine-level regression test that yields through goal_continuation_message_if_needed/goal_continuation_if_active and through the user-message resume path, since the current tests do not cover the dispatchers (5).

Attribution

🤖 Generated by SpikeBot 003(ClaudeCode-JP)

[SparkofSpike]

Verdict: APPROVE — the follow-up clears my block

The three material findings from the first pass are each fixed soundly, and the
host-side reason-erasure gap the independent review raised is closed at every
durable-reconstruction path I can find. I reviewed this as adversarially as the
first pass. A hard caveat up front: there is no Rust toolchain on this box, so
I could not build or run any of it. My verification is a careful static pass
(lines quoted below), not an execution. I did confirm the state-store JSON claim
against the actual loader.

---

Finding 1 — state guard on yield · resolved (confirmed)

crates/tui/src/tools/goal.rs:549-556. mark_yielded refuses a missing
objective and any status != Some(GoalStatus::Active).

I attacked the boundary for both directions:

• Refused where it should be allowed? No. Every production path that yields a
  live goal — create (goal.rs:373), replace (goal.rs:388),
  from_persisted (goal.rs:280), and resume/resume_after_yield — sets
  status = Some(Active). No path leaves objective set with status == None
  (nothing assigns status = None; nothing constructible reaches the tool with
  objective set but status unset), and even a contrived None-status goal would
  not be re-armed by either dispatcher (is_active() requires Active), so
  refusing a hand-back there is consistent with what a resume could do anyway.
• Lets something through it should not? No. A None status, any Paused
  (User / NoProgress / Usage / Budget / Backoff), Blocked, runtime_blocked,
  Complete, and a sealed completion_sealed goal are all non-Active and all
  rejected before mark_paused runs. completion_sealed and the objective
  guard in mark_paused (goal.rs:588-592) become dead against mark_yielded
  but are still needed by the other pause callers, so the protection isn't
  removed anywhere. I could not find a state the guard admits that it should
  not. Both cited guards return Err before mutating, so a rejected yield
  leaves the prior reason/blocker intact (proven by the new
  a_yield_cannot_overwrite_a_pause_reason_someone_else_set).

Finding 2 — #[serde(other)] on GoalPauseReason · fixed (confirmed)

crates/protocol/src/lib.rs:380-383. The enum is externally tagged with only
unit variants, which is exactly the shape #[serde(other)] is legal for; it
compiles, unknown strings map to Unrecognized, known values round-trip, and
adding Unrecognized changes no existing variant's serialized form (all other
arms are named, including via rename_all).

The forward-compat failure it prevents is real and localized:
crates/state/src/lib.rs:2113-2118 deserializes pause_reason from the SQLite
TEXT column with serde_json::from_str, and without the fallback an unknown
arm from a newer build unwinds the whole goal-row load via ? →
FromSqlConversionFailure → load_goal fails. #[serde(other)] turns that
into Unrecognized and the record loads. That is the correct, narrow fix.

I also confirmed Unrecognized is never constructed: it appears only in the
enum definition, the label() arm (protocol/src/lib.rs:394), and the round
-trip test. No write path constructs it, and every other match on
GoalPauseReason uses a wildcard (e.g. runtime_threads.rs:5993), so no
exhaustive-match breakage.

One suspected (not blocking) note: a forward-compat value loaded as
Unrecognized and re-persisted serializes as "unrecognized", losing the
newer build's original label. Reading that back still yields Unrecognized, so
the "not a hand-back" conservative meaning is stable — it is a normalized
downgrade, not a behavioral leak. I did not verify the re-write path executes in
practice (a Unrecognized-reasonable goal is Paused, so merge_engine_goal _progress's goal.status != Active early-return at runtime_threads.rs:5973
means it is unlikely to be re-emitted).

Finding 3 — host projection erases the reason · fixed (confirmed, one suspected residual)

thread_goal_status_projection now takes the record's own reason
(goal.rs:790), sync_from_host_status_with_reason carries it through
(goal.rs:170), and plain sync_from_host_status delegates with None
(goal.rs:163) so reason-less callers fall back to User as before only where
no reason exists.

I traced every durable→runtime reconstruction site:

• engine/handle.rs:321-326 (sync_runtime_goal_control) — fixed: calls
  with_reason with the projected reason. Both host callers
  (runtime_threads.rs:8779,:8800) pass the durable ThreadGoal, so the
  reason survives.
• runtime_threads.rs:15271-15283 — fixed: GoalSnapshot::from_thread_goal
  carries the reason into from_persisted, which stores it
  (goal.rs:280-303). The scalar .0 at :15278 drops the reason, but that
  is the side-channel goal_status mirror; the authoritative state comes first
  from the snapshot and already carries Yielded.
• runtime_threads.rs:14541 (.0 only) — fine: feeds TurnSpec.goal_status
  (a scalar); the engine's own goal_state is seeded elsewhere with the
  reason, and the resume gate reads the state, not the scalar.
• The engine constructor's reason-less sync_goal_state_from_host
  (engine.rs:1868) is a no-op against an already-seeded Paused goal
  (state.status != Some(status) is false → the assigning block in
  goal.rs:215 is skipped), so it cannot clobber Yielded.

Suspected residual, not confirmed: the interactive TUI dispatch forwards only
the scalar app.goal.status into TurnSpec (dispatch.rs:657), and a
fresh-engine construction could re-derive User if the engine is rebuilt
between a yield and the user's reply. I never confirm whether Engine is
reconstructed there without the state being seeded from the record first; the
normal yield→reply window reuses the cached engine, so it is conditional and
low. I could not exercise it. Everything in scope for this review — the
runtime_threads.rs call sites and engine/handle.rs — preserves the reason.

Finding 4 — gate tests · worth what they claim, with one residual gap

a_yielded_goal_is_not_rearmed_by_the_continuation_gate
(test_cases_12.rs:1586) and its active control (:1621) do prove the cross
-turn gate (goal_continuation_if_active, engine.rs:4937) treats a yielded
goal as Inactive and that the gate is not dead under test conditions. To
answer the prompt directly: yes, the yielded-goal test can pass for a reason
other than the yield — the gate reads only snapshot.is_active(), so any
user pause or budget stop would also produce Inactive. That is inherent in the
gate's contract (it deliberately does not inspect the reason), not a defect in
the test; the reason-specific behaviour is what
resume_after_yield enforces, and that has a dedicated unit test
(a_yield_stops_continuation_and_the_users_message_resumes_it). So the control
(a real control) plus the reason unit test together cover what the gate test
touches; the test does not overclaim.

• Still untested: the intra-turn dispatcher
  goal_continuation_message_if_needed (turn_loop.rs:5188) is not driven by
  any test. It is correctly gated by inspection — it ?es out of non-active
  goals at :5202 and rechecks is_active() under lock before counting a
  pass (:5212) — so the cross-turn result has a static, not executed, twin.
  This remains an honest test gap on the "neither dispatcher re-arms a
  non-active goal" claim, but it is a pre-existing gap the follow-up was not
  asked to close, and the change does not regress the intra-turn path. Not
  blocking; flagging it plainly.

Finding 5 — regressions on earlier probes · none found (static)

mark_paused (goal.rs:582), resume_after_yield (goal.rs:531),
resume_after_runtime_block (goal.rs:515) are unchanged; the dispatchers'
guarding on is_active() is consistent; the tool's update_goal "yield"
branch now rejects a non-active goal with ToolError::invalid_input and, by
virtue of the guard returning Err before mutation, leaves the prior
reason/blocker intact. I could not run the cargo test -p codewhale-tui --lib goal suite the PR body cites (147 passed) — no toolchain here — so those
counts are the author's word, not my verification.

Finding 6 — anything else

• Known-umbrella paragraph: it names the repeated-yield-without-progress
  hole honestly; record_not_achieved is indeed the only stall-counter mover and
  a yield ends the turn before it. I confirm that intended gap exists and is
  out of scope, as written.
• Nit (not blocking): the two gate tests end test_cases_12.rs without a
  trailing newline (\ No newline at end of file in the diff), while
  goal.rs has one. Cosmetic only.

What I could not verify

No Rust toolchain exists on this host, so I could not run
cargo test -p codewhale-execpolicy, the protocol round-trip, the goal unit
suite, or the two new gate tests. Nothing above is a claim of execution; it is
read-of-code. The serde, projection, and guard conclusions are backed by the
actual attribute and callable code cited above.

---

Attribution

🤖 Generated by SpikeBot 003(ClaudeCode-JP)