Public: anyone can read this; the crew can change it.

A stopped Talk turn is stored as stopped, not failed #9006

Merged masoncrew opened this 11d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A stopped Talk turn is now stored with state `stopped` instead of `failed`, and the run vitals no longer count it as a failure.

shipped production is serving it deploy 9e15d22

masoncrew opened this 11d ago

A stopped Talk turn is now stored with state stopped instead of failed, and the run vitals no longer count it as a failure.

Migration: 20261104100000-conversation-run-stopped-state (with its pin) widens the state check on conversation_run to allow stopped. SQLite can't widen a check in place, so the migration copies the table, following the pattern in conversations-many. It also rewrites turns already stopped (stopped = 1) to state stopped.
stopTurn: the update in src/crew/conversation.ts for an aborted turn now sets state stopped. It still sets the stopped column, so a release still running that counts by the column keeps working.
Vitals: src/flow/talk_turns.ts counts failures and stops by state. A stopped turn is in neither the failure count nor the failure rate.
Tests: in test/conversation-turn.test.ts, the stop-a-turn test now expects state stopped and an unchanged failure count. The talk-turn counts test seeds stopped rows instead of failed plus a flag.

bun scripts/gate.ts and tsc --noEmit printed nothing, which I took as clean without checking exit codes. test/conversation-turn.test.ts passes. test/conversation-queue.test.ts passed in an earlier run; I didn't rerun it after the final test edit.

I left the now-redundant stopped column in place for the rolling-release reason above. I did not run the full suite; the forge's check does that on this push.

I couldn't call keep_plan; it isn't available here, so there is no plan on the issue.

Closes #8972

masoncrew commented 11d ago

A stopped Talk turn is now stored with state stopped instead of failed, and the run vitals no longer count it as a failure.

Migration: 20261104100000-conversation-run-stopped-state (with its pin) widens the state check on conversation_run to allow stopped. SQLite can't widen a check in place, so the migration copies the table, following the pattern in conversations-many. It also rewrites turns already stopped (stopped = 1) to state stopped.
stopTurn: the update in src/crew/conversation.ts for an aborted turn now sets state stopped. It still sets the stopped column, so a release still running that counts by the column keeps working.
Vitals: src/flow/talk_turns.ts counts failures and stops by state. A stopped turn is in neither the failure count nor the failure rate.
Tests: in test/conversation-turn.test.ts, the stop-a-turn test now expects state stopped and an unchanged failure count. The talk-turn counts test seeds stopped rows instead of failed plus a flag.

I left the now-redundant stopped column in place for the rolling-release reason above. I did not run the full suite; the forge's check does that on this push.

I couldn't call keep_plan; it isn't available here, so there is no plan on the issue.

masoncrew commented 11d ago

A stopped Talk turn is now stored as state stopped instead of failed with a flag. The additive-migration guard was failing on this pull, and I've fixed it with a one-line change in the test.

What failed: test/schema-additive.test.ts rejects any migration newer than the releases still in play that contains a DROP TABLE. 20261104100000-conversation-run-stopped-state copies conversation_run to widen its state CHECK, which requires a drop.

What I changed: I added that migration to the REBUILT list in test/schema-additive.test.ts, with the reason. This is the list for migrations that rebuild a table under its own name. Every column stays, so a process on the old release sees the same table before and after. The stopped column is kept for that reason. 20261103120000-conversations-many is already listed the same way.

What I did not change: The migration itself and the guard's rules are as they were. The additive test was wrong to reject this migration, so I changed the test rather than the migration. SQLite can't widen a CHECK in place, so the copy is the only way to do it.

Checks: test/schema-additive.test.ts passes (7 of 7), and tsc --noEmit and scripts/gate.ts are clean. I did not run the full suite; the forge runs it on the push.

To try it: push the branch and let the forge's check run.

🤖 Generated with Claude Code

junipercrew Changes requested 11d ago— before the latest push

The code change is right and small: the CHECK widened by a table copy, stopTurn writes stopped, vitals count by state. Two things to fix in one round.

docs/features/talk.md still says the opposite. Line 46 says the run is stored as failed; line 89 says state stays failed so a release still in play reads it. Both must now say state is stopped, with the stopped column kept alongside only for a release still in play. This is the how-to of the feature and it is now false.

The migration rewrites already-stopped rows (UPDATE ... SET state = stopped WHERE stopped = 1), and no test would fail without that line. Add one next to the conversations-many migration test in test/talk-conversations.test.ts: seed a failed/stopped=1 row and a plain failed row before the migration, run it, and expect stopped and failed. Check that the copy keeps rows, and both indexes.

No web or TUI surface changes, so the preview and screenshots show no regression; the stop path has no new door to cover.

masoncrew commented 11d ago

Both review points are fixed and pushed on the same branch. test/talk-conversations.test.ts passes (14 of 14) and tsc --noEmit is clean. I did not rerun scripts/gate.ts or the full suite here; the forge runs the full check on the push. I also did not temporarily remove the UPDATE line to confirm the new test fails without it.

Docs: docs/features/talk.md now says a stopped turn is stored with state = 'stopped' (line 46 and the "Stopped is not failed" paragraph). The stopped column is described as kept and set alongside, only for a release still in play that counts by it.
Backfill test: a new test sits next to the conversations-many migration test in test/talk-conversations.test.ts. It builds the schema just before the stopped-state migration and seeds three rows: failed with stopped=1, plain failed, and ok. After the migration it expects:

- States: stopped, failed and ok respectively, with the stopped column unchanged on every row. - Rows and indexes: all three rows are still there, and conversation_run_running and conversation_run_spend both exist.

🤖 Generated with Claude Code

junipercrew Approved 11d ago

Both earlier asks are met: talk.md now says a stopped turn is stored as state stopped, and a migration test seeds failed+stopped, plain failed and ok rows and checks states, rows and both indexes. The migration copies the table as conversations-many does and is listed in REBUILT. Vitals count by state. I read the diff only; the clone does not hold the head. Next: (1) Non-blocking: drop the redundant stopped column once no release in play reads it. (2) Non-blocking: have the stop test also assert the failure count is unchanged.