Conversation
Multi-agent workflow to implement real-time PTY output streaming across five layers: Rust emitter, TS wrapper parser, protocol types, daemon subscription, and SDK client method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
khaliqgant
left a comment
There was a problem hiding this comment.
Code Review: PTY Output Streaming Workflow YAML
Verdict: REQUEST_CHANGES
Critical Issues (BLOCK)
1. File paths throughout the workflow reference a non-existent directory structure
The workflow assumes a relay-pty/src/main.rs Rust crate and packages/wrapper/, packages/daemon/ directories. None of these exist. The actual codebase layout is:
- Rust PTY code:
src/pty.rs,src/pty_worker.rs,src/main.rs(single crate at repo root viaCargo.toml, binary nameagent-relay-broker) - There is no
packages/wrapper/directory -- there is no separate TS wrapper package - There is no
packages/daemon/directory -- the daemon is the Rust binary itself - Protocol types:
packages/sdk/src/protocol.ts(this one is correct)
Every preflight check, every task description, and every build command referencing relay-pty/ or --manifest-path relay-pty/Cargo.toml will fail. The correct Cargo commands use just cargo build or cargo build --manifest-path Cargo.toml from repo root.
Affected steps: rust-output-emitter, build-rust, final-regression, commit-and-pr preflight checks, integration-test
2. worker_stream already exists -- the feature is largely implemented
The workflow's core premise (issue #390) assumes PTY output is never emitted as events. However, the codebase already has:
- Rust side (
src/pty_worker.rs:441-444, 836-839): PTY output is already emitted asworker_streamframes with{ stream: "stdout", chunk: text }viasend_frame() - Protocol types (
packages/sdk/src/protocol.ts:217-221):worker_streamBrokerEvent already defined with{ kind: 'worker_stream', name, stream, chunk } - SDK side (
packages/sdk/src/relay.ts:1019, 1190):agent.onOutput()callback already exists and filtersworker_streamevents by agent name, with unsubscribe support - Tests (
packages/sdk/src/__tests__/orchestration-upgrades.test.ts:988-1055): Full test suite foronOutputincluding filtering and unsubscribe
The workflow would have agents re-implement functionality that already exists, risking regressions or conflicting implementations.
High Priority
3. packages/wrapper/src/relay-pty-orchestrator.ts does not exist
The ts-wrapper-parser step and ts-wrapper-dev agent are built around this file. The PTY orchestration is handled entirely within the Rust binary (src/pty_worker.rs). There is no TS wrapper layer between Rust and the daemon -- the Rust binary IS the daemon/broker.
4. The SUBSCRIBE_WORKER_OUTPUT subscription model conflicts with existing architecture
The existing architecture uses a push model: the broker broadcasts worker_stream events to all connected SDK clients as BrokerEvents. SDK clients filter locally via agent.onOutput(). The workflow proposes a pull/subscription model (SUBSCRIBE_WORKER_OUTPUT -> WORKER_OUTPUT) that would be a parallel, incompatible mechanism for the same data.
5. daemon-subscription step references non-existent infrastructure
The task mentions packages/daemon/src/spawn-manager.ts, packages/daemon/src/server.ts, __broadcastLogOutput, and onLogOutput. None of these exist. Grepping confirms zero matches for onLogOutput, broadcastLogOutput, or spawn-manager in the codebase. The broker logic lives in src/main.rs and src/pty_worker.rs (Rust).
Medium Priority
6. commit-and-pr step creates a competing PR from within a workflow PR
The final step runs gh pr create to make a new PR. This is confusing when the workflow YAML itself is already in a PR. The step also uses git add relay-pty/src/ packages/wrapper/src/ packages/daemon/src/ -- three of those four paths don't exist.
7. Agent role assignment mismatch
ts-wrapper-dev is assigned to fix Rust build failures in fix-build-failures ("For Rust issues, describe what needs to change and make the fix in relay-pty/src/main.rs directly"). A Rust fix step should be assigned to rust-dev.
8. Build commands won't work
npm run lint-- should verify this exists; the repo usesturbo.jsonsuggestingnpx turbo lintor similarnpx tsc --noEmit -p packages/wrapper/tsconfig.json--packages/wrapper/doesn't existnpx tsc --noEmit -p packages/daemon/tsconfig.json--packages/daemon/doesn't exist
Summary
This workflow YAML is based on an incorrect mental model of the codebase architecture. The repo is a monorepo with a single Rust binary (agent-relay-broker) at the root (not in relay-pty/), no separate TS wrapper or daemon packages, and the core feature (PTY output streaming via worker_stream) is already implemented across Rust, protocol types, and SDK layers.
Recommendation: Before writing a workflow, the actual codebase structure and existing worker_stream implementation need to be audited. If issue #390 is about gaps in the existing streaming (e.g., buffering, subscription filtering at the broker level, or stderr streaming), the workflow should be rewritten targeting the correct files and building on top of the existing worker_stream infrastructure rather than creating a parallel system.
…390) The previous workflow was based on wrong assumptions — it referenced non-existent paths (relay-pty/src/main.rs, packages/wrapper/, packages/daemon/) and tried to implement features that already exist. The full worker_stream pipeline is already implemented: - Rust: src/pty_worker.rs emits worker_stream frames - Protocol: packages/sdk/src/protocol.ts defines worker_stream types - SDK: packages/sdk/src/relay.ts has agent.onOutput() with filtering Rewritten workflow focuses on the actual remaining gaps: 1. Rate-limited 100ms buffering on the Rust side (reduces frame flood) 2. Optional stream-type filtering on the SDK onOutput() API Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g in PTY streaming workflow - Add preflight check to reject execution on main branch - Add set -o pipefail to build-check and run-tests steps so pipe through tail preserves exit codes - Change run-tests failOnError to true so commit-changes is skipped on test failure - Add create-branch step and main-branch guard before commit-changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Agents now use preset: worker (non-interactive, no relay tools) - Removed output_contains verification with tokens in task text (double-occurrence problem) — using exit_code instead - Pre-read context via deterministic steps, inject via step output instead of hardcoded line numbers - Removed channel subscriptions on non-interactive agents - Reduced from 6 phases to 2 (impl + verify/fix) - Removed coordination/barriers/state config (unnecessary for dag) - Shortened task prompts (was 40+ lines, now ~15) - Removed summary step (adds latency, no value) - Removed git branch/commit steps (workflow shouldn't auto-commit) - Added file verification gate between impl and build
Summary
relay.pty-output-streaming.yaml) to implement real-time PTY output streaming per Relay PTY Output Streaming #390Test plan
agent-relay workflow validate relay.pty-output-streaming.yamlRefs #390
🤖 Generated with Claude Code