Repository navigation
Render every Workflow stage chat line in full - #1510
Merged
Merged
Conversation
CHAT_PREVIEW (240) and chatPreview() clipped every stage chat message, so the owner's brief and each narrate line that quotes a Bot reply lost its tail and the rest was unreachable (regression from #1456). Render message.text in the system, user and Bot bubbles again, keep #1473's title and word-wrap, and keep the 240-character cap on taskSummary, which is the summary surface. Fixes #1460 Docs-not-needed: display-only restore of text that the stage chat already receives; docs/subsystems/workflow.md describes the message flow, not the truncation, and no flow or API changes.
Collaborator
Author
|
Verdict: PASS at e76c222 Ran at this tip (2026-10-28T01:00Z): 审查要点:
|
Birfy
added a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 28, 2026
…eep (ThinkFlowLab#1425) (ThinkFlowLab#1427) Owner: Chief Reviewer: Reviewer Fixes ThinkFlowLab#1425 Tip SHA: `04655e4a91770f40918c17ca6b56f4f34a1b11c2` Base: `92a6aabd0e4bbe679476cd62438ed5bae13405fd` (`main`) Tree: `fa59bf2ca67a6d42ab39e6c210bf50d574203f0d` ## Reproduce `plugins/workflow/index.js:92` starts reap without awaiting it (`void service.reap?.()`), so `apply()` returns while the release is still in flight. The test paid for that with one fixed sleep and then asserted: ```js await new Promise((resolve) => setTimeout(resolve, 20)) assert.deepEqual(released, ['eph-plan']) ``` 20 ms is a wall-clock dependency, not a condition. On a loaded runner the release lands later than 20 ms and the test reports "nothing was released" / "the run is still running" — the exact symptoms of the bug the test exists to catch. I made that concrete by keeping the new wait out and slowing the release to 150 ms: **1 test fails** (`# fail 1`), `Expected values to be strictly deep-equal: + [] - [ 'eph-plan' ]`. ## What changed One file, tests only; no product code and no `patches/ds-bot/**` (that tree does not cover `plugins/workflow`). - New local helper `untilRun(dir, workflowId, settled, { timeout = 2000, interval = 10 })` polls through the file's existing `persistedRun(dir, workflowId)` until the predicate holds. The deadline exists only to bound a failure, and its message names the state it last saw: `the run was not reaped within ${timeout} ms; last state: …`. - The `ThinkFlowLab#1388` plugin-load test now waits for what reap actually writes — `run.status === 'failed' && run.ephemeral === undefined` — then asserts `'handed' in settled === false`, `released === ['eph-plan']`, and the run's final `failed`/`ephemeral: undefined`. - Before/after for the user: nothing visible. The test stops being a timer race and starts being a guard: slow release passes, a missing reap still fails. No `changelog.d/` fragment: this is a test-only change with no user-visible behavior (AGENTS.md item 4). Docs-not-needed: test-only change; the plugin-load reap behavior described in `docs/subsystems/workflow.md` is unchanged, only the test's wait condition moved from a fixed 20 ms sleep to the state the test asserts. ## Rework at `04655e4a` (Written-by: Rework) Written-by: Rework main moved three commits past the base this PR last sat on (`4d92f4f7` → `92a6aabd`: ThinkFlowLab#1510, ThinkFlowLab#1430, ThinkFlowLab#1512), and ThinkFlowLab#1512 rewrote the very test file this change watches, so the reviewed wait is replayed onto current main as one commit whose parent is `92a6aabd`. `compare` `main...04655e4` lists exactly one file, `plugins/workflow/test/ephemeral.test.js` (+18/−1), `ahead_by 1`, `behind_by 0`; every other blob in the tree is main's, because the commit's tree is main's tree with that one entry replaced. The commit carries a `Docs-not-needed:` trailer in its message as well as in this body: `npm run lint` (`arch-docs`) red-flagged the previous replay with `plugins/workflow/test/ephemeral.test.js → workflow (docs/subsystems/workflow.md)` because it reads the trailer from `git log <base>..HEAD`, not from the body. The tree is byte-identical to that earlier replay (`fa59bf2c`); only the commit message changed. The fix itself is unchanged: the same `untilRun` helper (deadline 2000 ms, interval 10 ms, failure message naming the last state) and the same wait on `status === 'failed' && ephemeral === undefined` instead of the fixed 20 ms sleep. It composes with ThinkFlowLab#1512's node-Bot grant assertions in the same file, which are untouched. The comment's line reference now reads `index.js:92`, which is where main keeps the unawaited `void service.reap?.()`. The body's earlier claim mismatch is fixed and not re-touched here: the checkpoint `claim` is `Chief`, matching the `Owner:` line. ## What I ran At tip `04655e4a91770f40918c17ca6b56f4f34a1b11c2` (base `92a6aabd0e4bbe679476cd62438ed5bae13405fd`), in `plugins/workflow`: - `node --test test/ephemeral.test.js` → **13/13 pass**, 0 fail (226 ms) - `npm test` → **90/90 pass**, 0 fail (323 ms) main's version of the file now holds 13 tests and the workflow suite 90; the counts differ from the previous tip because ThinkFlowLab#1512 added tests, not because this change added or dropped any. ## Red without the source change Each knife is the same tree with one thing reverted, run from a copy: - **New wait + the plugin-load Bot removal slowed to 150 ms** → **13/13 pass** (368 ms). The old 20 ms could not have survived this. - **Old 20 ms wait + the same 150 ms removal** → **`not ok 13`**, `# fail 1`, 12 pass. This is the flake the issue describes, reproduced on demand. - **New wait + the `void service.reap?.()` call removed from `index.js:92`** → **`not ok 13`**, `# fail 1`, 12 pass: `the run was not reaped within 2000 ms; last state: {"id":"run-4c5302fc",…"status":"running","nodeStatus":{"plan":"running"},"ephemeral":{"plan":"eph-plan"},"handed":{"plan":true},…}` So the guard still fires when the behavior it protects actually disappears — it is not merely tolerant. ## Not run - Full `core` suite and `npm run verify`: not runnable on this box (no git, no oxlint, no esbuild; `verify` needs `origin/main`). `core` is untouched by this diff. - `node scripts/ds-bot-patches.mjs generate`/`check`: not needed — no file under `core/`, `ds-bot/`, or `patches/ds-bot/` is in this diff. - CI: left to the tip's checks. ## Risk Low. `plugins/workflow/test/ephemeral.test.js` is the only changed file and no production path moves. The trade-off is deliberate: the test can now wait up to 2 s before failing instead of 20 ms, so a broken reap costs ~2 s of test time rather than a false green. ## Checkpoint <!-- flybot:checkpoint --> ```json { "pr": 1427, "claim": "Chief", "issue": 1425, "summary": "The ThinkFlowLab#1388 plugin-load reap test slept a fixed 20 ms after apply(), which starts reap without awaiting it; on a loaded runner the release landed later and the test flaked like the bug it guards. It now polls workflows.json through a local untilRun helper until the run is failed with its ephemeral Bot released, bounded by a 2 s deadline that names the last state seen. Tests only; no product code, nothing under core/ or patches/ds-bot/. Replayed by Rework onto main 92a6aab.", "files": ["plugins/workflow/test/ephemeral.test.js"], "checks": { "ran": "node --test test/ephemeral.test.js 13/13 pass (226 ms); npm test 90/90 pass (323 ms) in plugins/workflow; knives: new wait + 150 ms slow removal 13/13 pass; old 20 ms + 150 ms removal 1 fail; reap call removed 1 fail with the 2000 ms deadline message", "result": "green", "at": "04655e4a91770f40918c17ca6b56f4f34a1b11c2" }, "next": "none — ready for review" } ``` <!-- /flybot:checkpoint -->
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.
Fixes #1460
Owner: Chief
Reviewer: Reviewer
Written-by: Dev
Tip SHA:
e76c222626f2d4320edb475ec41b071c60148d14Base:
fe6c90693f5ff32c1147d0b618041b1acaaa0d76— livemainat claim time.What changed
Every Workflow stage chat line was clipped at 240 characters ending in
…:Bubbleran thesystem,userand Bot branches throughchatPreview()(plugins/workflow/client.js:428,:429,:435). An owner brief and each narrate line that quotes a Bot reply lost its tail, and after #1452 those narrate lines are the only place a node's words appear. The same line rendered in full before #1456, so this is a regression; #1473'stitleonly made the tail reachable on hover.Bubblerendersmessage.textfor thesystem,userandbotbranches again; thetitleprop and Polish workflow UI: compact assign rows, fix status badges, improve text display, align controls #1473's word-wrap stay.CHAT_PREVIEWandchatPreview()are deleted, including the export (a repo-wide search found no other caller); the 240-character cap stays ontaskSummary(), the summary surface.changelog.d/1460.fixed.mdadded.Verification at the tip
Tip SHA:
e76c222626f2d4320edb475ec41b071c60148d14Ran:
fe6c9069with only the new tests copied in (source: base blob7e678c82e26cc9bf0bdfbe2e09d1939c705920b8):node --test test/client.test.js→# tests 29,# pass 26,# fail 3; the failures area long user line in the stage chat renders in full,a long system line …,a long roster Bot line …(defect tests). At the tip: 29/29. The fourth new test,the stage strip still clips a long task title as a summary, is a guardrail — green on base and at the tip.npm --prefix plugins/workflow test→ 89/89 pass (base 85 + 4 new).npm --prefix plugins/social test→ 525/525;tree→ 125/125;timeline→ 125/125.node --test ".github/scripts/*.test.mjs"→ 543 tests, 541 pass, 0 fail, 2 skipped.npm run lint:interop+npm run test:interop→ lint OK, 37/37;lint:live-e2e+test:live-e2e→ lint OK, 24/24.node core/scripts/build-client.mjs --check→ pass;node scripts/ds-bot-patches.mjs check→ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/(skipped pin (no git)).node core/scripts/lint.mjs→ oxlint62 warnings and 0 errors;node scripts/lint-plugins.mjs→6 warnings and 0 errors,packed @flybot/workflow imported.Not run (and why):
TZ=UTC npm run verifyas one command: this box has nogitbinary, socheck-arch-docs.mjscannot diff and exits the chain (arch-docs: FAIL — cannot diff against origin/main). Both the branch commit and this body carryDocs-not-needed: display-only restore of text the stage chat already receives; docs/subsystems/workflow.md describes the message flow, not the truncation.coresuite: run separately; it is not touched by this diff and its result is in the review thread (CI runs it in full).index.jsorcore/src/host/change) and the CI-onlypackagejob.Risk
T2:
plugins/workflow/client.jsmatchesplugins/**; the test file and the changelog fragment are T1. Norisk:T*label is hand-set — the tier comes fromrisk-gateon the diff. T2 needs a PASS at this tip plus green CI; nothing is enqueued by this node.Docs-not-needed: display-only restore of text the stage chat already receives; docs/subsystems/workflow.md describes the message flow, not the truncation.
Checkpoint
{ "pr": 1510, "claim": "Chief", "issue": 1460, "summary": "The Workflow stage chat renders every message line in full again: the system, user and Bot bubbles no longer pass message.text through chatPreview(), so an owner brief or a narrate line that quotes a Bot reply is readable to the end. CHAT_PREVIEW and chatPreview() are deleted because taskSummary is their only remaining caller, and the 240-character cap stays on that summary surface. The title tooltip and word-wrap are kept. New tests cover long user, system and persistent-roster Bot lines on the mounted stage, and one guardrail pins the strip clipped summary.", "files": ["plugins/workflow/client.js", "plugins/workflow/test/client.test.js", "changelog.d/1460.fixed.md"], "checks": { "ran": "npm --prefix plugins/workflow test; node --test plugins/workflow/test/client.test.js at base and tip; plugins social/tree/timeline, .github/scripts, interop and live-e2e suites; build-client --check; ds-bot-patches check; oxlint", "result": "green", "at": "e76c222626f2d4320edb475ec41b071c60148d14" }, "next": "none — ready for review" }