fix(workflow-runtime): tighten PTY chrome scrubbing, document idle override, tame stale-state warning#930
Merged
Merged
Conversation
…erride, tame stale-state warning
Three quality-of-life fixes surfaced by running a multi-CLI workflow
end-to-end with interactive PTY agents:
1. **Strengthen scrubForChannel for Claude Code TUI noise.**
Interactive-agent step "Output:" blocks were unreadable — the captured
PTY stream included Claude Code's bottom status bar
("Opus 4.7 (1M context) ctx:5% $1.45"), vim-style mode indicators
("--INSERT--⏵⏵bypasspermissionson"), no-whitespace UI hints
("pasteagaintoexpand"), and ambient thinking-status fragments
("↓ 13 tokens · thinking with high effort", "Crunched for 32s").
None of these were caught by the existing regexes.
Added: claudeFooterRe, vimModeRe, thinkingStatusRe; relaxed uiHintRe
to match the no-whitespace TUI rendering variants. New regression test
(scrub-pty-chrome.test.ts) covers the patterns from a real capture and
guards against over-stripping (prose that mentions model names is
preserved).
2. **Document idleThresholdSecs as the per-agent idle escape hatch.**
The option is already plumbed end-to-end (AgentOptions → constraints
→ broker spawn arg) but the JSDoc was one line and didn't mention that
`0` disables idle detection — so users hitting "agent exited before
the awaited event" had no idea how to fix it. Expanded the doc with
when-to-override / when-not-to-override guidance pointing at the
skill.
3. **Stop warning about .agent-relay/ on every ephemeral run.**
The broker warned "stale .agent-relay/ directory found" whenever an
ephemeral run found a .agent-relay/ dir in cwd. But the SDK workflow
runner *always* writes .agent-relay/step-outputs/ and .agent-relay/
team/worker-logs/ regardless of broker mode — those are durable
artifacts, not broker state. The warning fired on virtually every
workflow run as a false positive. Narrowed the check to look for
.agent-relay/state.json specifically, which is the only file that
indicates actual prior broker state worth warning about.
All three are non-breaking. Pre-existing test failures in
verification-traceback.test.ts and verification-custom.test.ts are
unrelated and present on origin/main.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefines broker init stale-state detection, centralizes and hardens PTY/TUI output scrubbing in channel-messenger, delegates runner sanitization to that module, reformats reliability/spawn expressions, adds comprehensive PTY scrubbing tests, expands a builder JSDoc, and updates trajectories metadata. ChangesSDK Workflow Reliability & PTY Output Handling
Broker Initialization State Detection
Trajectories metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // on virtually every workflow run — a noisy false positive. The | ||
| // discriminator is `state.json`, which only the broker writes and only | ||
| // in persist mode. | ||
| let stale_state = runtime_cwd.join(".agent-relay").join("state.json"); |
Contributor
There was a problem hiding this comment.
🟡 Stale state check looks for state.json but persist mode creates state-{name}.json
The new stale-state detection checks for {cwd}/.agent-relay/state.json (init.rs:44), but ensure_runtime_paths in crates/broker/src/runtime/paths.rs:140,176,187 always creates state-{safe_name}.json (e.g., state-my-project.json) in persist mode. The only code that creates a bare state.json is ensure_ephemeral_paths (paths.rs:64), but that writes to a temp directory, not .agent-relay/. As a result, this stale-state warning will never fire for any persist-mode run, silently defeating the intent of the check.
Filename used in persist mode vs stale check
Persist mode (paths.rs:187):
state: root.join(format!("state-{safe_name}.json")),Stale check (init.rs:44):
let stale_state = runtime_cwd.join(".agent-relay").join("state.json");Prompt for agents
The stale state check hardcodes `state.json` but `ensure_runtime_paths` (paths.rs:140,176,187) creates `state-{safe_name}.json` where safe_name is derived from the broker name. This means the stale check never matches files left by a persist-mode broker.
To fix this, the check should look for any file matching the `state-*.json` glob pattern inside `.agent-relay/`, rather than a single hardcoded name. For example, use `std::fs::read_dir` on the `.agent-relay/` directory and check for any entry whose name starts with `state-` and ends with `.json`. Alternatively, just check for `.agent-relay/state-{safe_name}.json` using the same `resolved_name` variable that is already computed above (after sanitizing it the same way `ensure_runtime_paths` does).
The suggested-removal message should also be updated to reference the correct filename(s).
Was this helpful? React with 👍 or 👎 to provide feedback.
Member
Author
There was a problem hiding this comment.
Good catch — fixed in e2ceccf. ensure_runtime_paths writes state-{safe_name}.json (not bare state.json), so my original check would never have fired. Replaced the single-name check with a glob of .agent-relay/state-*.json so it catches state files from any broker name, and the warning now lists every match.
PR feedback from Devin Review on #930: the prior commit checked for .agent-relay/state.json, but ensure_runtime_paths in runtime/paths.rs writes state files as state-{safe_name}.json (where safe_name is the sanitized broker name) — only ensure_ephemeral_paths writes a bare state.json, and that's to a temp dir, never to .agent-relay/. As a result the stale-state warning would never fire for any persist- mode run, silently defeating the intent of the check. Fix: glob .agent-relay/ for any state-*.json entry instead of looking for a single hardcoded name. Surface every match so the user can see exactly what's stale regardless of which broker name produced it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/src/workflows/__tests__/scrub-pty-chrome.test.ts`:
- Around line 23-35: The test currently only ensures tokens are removed but may
allow footer line fragments to remain; update the test around scrub to assert
the entire footer lines are absent by checking that the exact footer strings are
not present as whole lines (e.g., use expect(out.split('\n')).not.toContain(...)
or a multiline regex like not.toMatch(/^[^\n]*Opus.*ctx.*\$\d+\.\d+$/m)) so the
full footer lines produced by Claude are stripped; reference the scrub helper
(and scrubForChannel if used elsewhere) when locating the logic to validate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d077cd2e-9de4-4794-9ff6-92dc68afced2
📒 Files selected for processing (4)
crates/broker/src/runtime/init.rspackages/sdk/src/workflows/__tests__/scrub-pty-chrome.test.tspackages/sdk/src/workflows/builder.tspackages/sdk/src/workflows/runner.ts
Comment on lines
+23
to
+35
| it('strips the Claude Code bottom status bar (model + ctx% + cost)', () => { | ||
| const input = [ | ||
| 'real content line', | ||
| 'workflows git:(main) Opus 4.7 (1M context) ctx:5% $1.45', | ||
| 'Opus4.7(1Mcontext) ctx:6% $1.54', | ||
| 'another real line', | ||
| ].join('\n'); | ||
| const out = scrub(input); | ||
| expect(out).toContain('real content line'); | ||
| expect(out).toContain('another real line'); | ||
| expect(out).not.toMatch(/ctx\s*:\s*\d+%/); | ||
| expect(out).not.toMatch(/\$\d+\.\d+/); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert full footer-line removal, not just token removal.
This test can still pass if scrubForChannel leaves non-token footer fragments behind. Add exact-line absence checks to guard against partial stripping regressions.
Suggested test tightening
const out = scrub(input);
expect(out).toContain('real content line');
expect(out).toContain('another real line');
+ expect(out).not.toContain('workflows git:(main) Opus 4.7 (1M context) ctx:5% $1.45');
+ expect(out).not.toContain('Opus4.7(1Mcontext) ctx:6% $1.54');
expect(out).not.toMatch(/ctx\s*:\s*\d+%/);
expect(out).not.toMatch(/\$\d+\.\d+/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/workflows/__tests__/scrub-pty-chrome.test.ts` around lines
23 - 35, The test currently only ensures tokens are removed but may allow footer
line fragments to remain; update the test around scrub to assert the entire
footer lines are absent by checking that the exact footer strings are not
present as whole lines (e.g., use expect(out.split('\n')).not.toContain(...) or
a multiline regex like not.toMatch(/^[^\n]*Opus.*ctx.*\$\d+\.\d+$/m)) so the
full footer lines produced by Claude are stripped; reference the scrub helper
(and scrubForChannel if used elsewhere) when locating the logic to validate.
runner.ts: packages/sdk/src/workflows/runner.ts runner.ts: packages/sdk/src/workflows/runner.ts channel-messenger.ts: packages/sdk/src/workflows/channel-messenger.ts Co-Authored-By: My Senior Dev <dev@myseniordev.com>
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
1 participant
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.
Summary
Three quality-of-life fixes to the workflow runtime, surfaced by running a multi-CLI workflow end-to-end with interactive PTY agents. All non-breaking; pre-existing test failures in
verification-traceback.test.ts/verification-custom.test.tsare unrelated (also failing onorigin/main).1. Strengthen
scrubForChannelfor Claude Code TUI noiseInteractive-agent step
Output:blocks were unreadable. The captured PTY stream included Claude Code's bottom status bar (Opus 4.7 (1M context) ctx:5% $1.45), vim-style mode indicators (--INSERT--⏵⏵bypasspermissionson), no-whitespace UI hint variants (pasteagaintoexpand), and ambient thinking-status fragments (↓ 13 tokens · thinking with high effort,Crunched for 32s). None were caught by the existing regexes.Added:
claudeFooterRe,vimModeRe,thinkingStatusRe; relaxeduiHintReto match the no-whitespace TUI variants. Newscrub-pty-chrome.test.tscovers the patterns from a real capture and guards against over-stripping (prose that mentions model names is preserved).2. Document
idleThresholdSecsas the per-agent idle escape hatchThe option is already plumbed end-to-end (
AgentOptions→constraints→ broker spawn arg) —0already disables idle detection — but the JSDoc was one line and didn't say so. Users hitting "agent exited before the awaited event" had no obvious way to fix it. Expanded the doc with when-to-override / when-not-to-override guidance, pointing at the updatedwriting-agent-relay-workflowsskill.3. Stop warning about
.agent-relay/on every ephemeral runThe broker warned
stale .agent-relay/ directory foundwhenever an ephemeral run found a.agent-relay/in cwd. But the SDK workflow runner always writes.agent-relay/step-outputs/and.agent-relay/team/worker-logs/regardless of broker mode — those are durable artifacts, not broker state. The warning fired on virtually every workflow run as a false positive. Narrowed the check to look for.agent-relay/state.jsonspecifically, which is the only file that indicates actual prior broker state worth warning about.Test plan
npm run checkinpackages/sdk— cleancargo checkincrates/broker— cleanscrub-pty-chrome.test.ts— 6/6 passorigin/main(unrelated to this change)Companion skill PR
AgentWorkforce/skills#56 — documents theidleThresholdSecs: 0pattern and the new "per-turn interactive spawn" coordination shape that the JSDoc points to.🤖 Generated with Claude Code