Separate automation run transport status from domain outcome - #1021
Open
amadad wants to merge 1 commit into
Open
Separate automation run transport status from domain outcome#1021amadad wants to merge 1 commit into
amadad wants to merge 1 commit into
Conversation
Collaborator
|
Hey @amadad in the future can you open up an issue before opening a PR. I would love if you could follow the CONTRIBUTING.md. It makes it easier to align on the problem first before coupling it to a solution |
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.
Problem
closeAutomationRunForSettledThreadinplugins/automations/src/run.tscollapses every settled agent thread into a singleautomation_runs.statuscolumn:thread.idlealways maps to"succeeded",thread.failedalways maps to"failed". That status only tells you the transport outcome — did the thread reach a terminal state without crashing — not the domain outcome the automation's prompt was actually trying to report.Concretely,
thread.idlefires whenever the provider turn settles, regardless of whether the agent's own instructions succeeded. The event carrieslastAssistantText, which can contain a terminal token the automation's prompt was told to emit (e.g.TASK_COMPLETE, or a failure sentinel likeREFRESH_FAILED) — but the run-closing code discards it. Today the only way to learn a script-mode run's actual outcome is viaoutput/skipReason; agent-mode runs have no equivalent, so downstream consumers (dashboards, other automations chaining off a run) are forced to scrape thread logs or guess from"succeeded"even when the agent's own prompt reported a domain failure. Theworkflowsplugin already sidesteps this by having its ownbb_workflow_resultconvention; automations had no equivalent.Fix
Separate the two concerns instead of conflating them:
automation_runs.status:running/succeeded/failed/skipped) keeps meaning exactly what it means today — did the run mechanism itself complete.terminal_tokencolumn) captures the last non-empty line of the run's output when it is a bare[A-Z][A-Z0-9_]*token (extractTerminalTokeninplugins/automations/src/terminal-token.ts), so an automation's prompt can report its own outcome without the platform inventing a taxonomy for it.Changes:
terminal-token.ts(new): strict single-line token extractor, capped at 128 chars, only accepted from a successful transport close.run-summary.ts(new):formatRunTransportLabel/formatRunDomainLabel— sharedtransport=…/domain=…formatting for CLI and UI.run.ts:closeAutomationRunForSettledThreadnow takes the caller-determined transportstatusdirectly ("succeeded" | "failed") plus an optionalterminalToken, instead of remapping"idle" | "failed"internally.server.ts: thethread.idlehandler extracts the terminal token fromlastAssistantTextand passes it through;thread.failedexplicitly nulls it (a transport failure has no trustworthy domain signal).script-runner.ts:mapScriptResultToRunextracts a terminal token from script stdout on successful runs (timeouts, non-zero exit, empty output, andwakeAgent:falseskips all stay null).data.ts: adds theterminal_tokencolumn via the plugin's standard sequential migration (bb.storage.migrate, same pattern every other migration in this file uses), threads it through the run row/select/insert/close paths, and nulls it whenever a run is reopened or closes non-successfully.closeAutomationRunnow does its update+read as a singleUPDATE … WHERE status = 'running' RETURNING …instead of an unconditional update followed by a separate read, so calling it twice on an already-closed run is a no-op that returns the first final state instead of silently overwriting it.service.ts: the manual-run failure path explicitly setsterminalToken: null.rpc-types.ts/cli.ts/detail-view.tsx:terminalTokenflows through the RPC response schema; the CLI run table and the run detail view both rendertransport=…anddomain=…labels (domain omitted when null).What's intentionally left out of this PR
The commit this is drawn from also included a ~600-line preflight schema-reconciliation pass (
reconcileTerminalTokenMigrationPreflight) that ran beforebb.storage.migrateon every plugin startup, plus ~350 lines of tests exercising it. That code defends against a specific migration-history drift this deployment's automations DB had accumulated during its own dev/rebase history — it is not something a normal install of this plugin can get into viabb.storage.migrate's existing sequential apply-by-index mechanism (seeapps/server/src/services/plugins/plugin-api.ts), and its schema/index/foreign-key introspection would actively throw on any database shape it doesn't recognize. It's excluded here as operationally-specific scar tissue, not a generalizable fix. The plainALTER TABLE automation_runs ADD COLUMN terminal_token TEXT;migration entry is the only schema change upstream needs, consistent with how every other migration in this file is written.Test coverage
plugins/automations/src/terminal-token.test.ts(new, trimmed from the larger test file above): token-extraction edge cases (strict format, trailing whitespace/CRLF, length cap, wrong-case rejection) andmapScriptResultToRun's success/timeout/non-zero-exit/skip branches; pluscloseAutomationRunstorage behavior — non-success statuses always null out the token, and closing an already-closed run is idempotent and preserves the first terminal state.plugins/automations/src/server-harness.test.ts: end-to-endthread.idle→ closed run assertion thatterminalTokenis populated fromlastAssistantText, plus a CLI-table assertion (Transport/Domaincolumns,transport=succeeded,domain=TASK_COMPLETE).plugins/automations/src/run-status-ui.test.tsx(new): rendersAutomationDetailViewwith a token-bearing and a legacy (null-token) run, asserts accessibletransport=…/domain=…labels and that the domain badge is omitted when there's no token.Ran locally:
pnpm exec turbo run test --filter=bb-plugin-automations— 8 test files, 63/63 passing.pnpm exec turbo run typecheck --filter=bb-plugin-automations— clean.Production note
This fix (in its full form, including the deployment-specific migration-reconciliation code excluded from this PR) is running in production on the author's own bb deployment.