feat(runtime): render agent event stream live during execution

[ralphbean]

Summary

Renders the agent's event stream live during fullsend run, replacing the old progress output that only showed periodic heartbeats and tool names.

• Define normalized AgentEvent types (InitEvent, ThinkingEvent, TextEvent, ToolUseEvent, TokensEvent, ResultEvent, ErrorEvent, RetryEvent) that bridge runtime-specific NDJSON parsing and runtime-agnostic rendering
• Add OnEvent callback to RunParams for custom event handling
• Add EventRenderer that renders structured terminal output: thinking (🧠 italic/muted), text (💬), tool calls (⚙ blue), heartbeats (⏳ muted), token usage, errors, retries, and result summaries
• Refactor Claude Code NDJSON parser to emit normalized events via callback, with assistant message fallback for non-streaming output
• Wire it all up in ClaudeRuntime.Run

→ Agent: claude-opus-4-6 (v2.1.91)
  🧠 Let me start by reading the AGENTS.md file...
  💬 I'll read the project instructions and fetch the issue details.
  ⚙ Read: /sandbox/workspace/fullsend-2/AGENTS.md
  ⚙ Bash: gh pr list --repo fullsend-ai/fullsend --state open
  ⚙ Skill: issue-labels
  ⏳ Agent running (30s elapsed, 9m30s remaining)
→ Result: success
    Turns: 15
    Cost: $0.3592
    Tokens: in=12 out=4110 cache_create=28320 cache_read=158872

Closes #3170.

Test plan

• All runtime and UI tests pass (87 tests)
• Verified with live triage run against Add a 🧠 emoji to the readme. appdumpster/test-repo#25

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:55 PM UTC · Ended 9:56 PM UTC
Commit: e8381e3 · View workflow run →

[github-actions]

Site preview

Preview: https://5f3fdfee-site.fullsend-ai.workers.dev

Commit: 85d773420f14709ea72e699e99b855851541e642

[ralphbean]

/fs-review

[qodo-code-review]

PR Summary by Qodo

feat(runtime): improve agent event stream rendering

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Introduce normalized AgentEvent interface and 8 concrete event types (InitEvent,
 ThinkingEvent, TextEvent, ToolUseEvent, TokensEvent, ResultEvent, ErrorEvent,
 RetryEvent) as a runtime-agnostic bridge between Claude's NDJSON parser and terminal rendering.
• Replace progressParser with parseClaudeStream that processes full stream-json deltas
 (thinking, text, tool input JSON, token usage, errors, retries) and emits events via a callback;
 keep progressParser as a thin backward-compatible wrapper.
• Add EventRenderer that consumes AgentEvent values and renders structured colored output:
 suppresses duplicate init headers, renders thinking (🧠) and text (💬) content, differentiates tool
 calls (⚙ blue) from heartbeat timer (⏳ muted), and shows full Bash commands truncated at 120 chars.
• Add OnEvent func(AgentEvent) to RunParams so callers can inject custom event handlers; wire
 default EventRenderer in ClaudeRuntime.Run when OnEvent is nil.
• Remove allowedTools blocklist — all tool names are now shown; extractSafeContext returns empty
 for unrecognized tools, and add context extraction for Agent (prompt) tool.
• Add ToolProgress printer method with blue ⚙ icon; change Heartbeat icon from ⟳ to ⏳ for timer
 events.
• Add comprehensive tests: renderer unit tests, parseClaudeStream tests for all event types,
 duplicate-init suppression, assistant fallback text/thinking, token throttling, and
 stream-vs-fallback deduplication.

Diagram

graph TD
    A["ClaudeRuntime.Run"] --> B["parseClaudeStream"]
    B --> C{"onEvent callback"}
    C -->|"OnEvent set"| D["Custom Handler"]
    C -->|"OnEvent nil"| E["EventRenderer"]
    E --> F["ui.Printer"]
    B --> G["AgentEvent types"]
    G --> H["InitEvent / ThinkingEvent / TextEvent"]
    G --> I["ToolUseEvent / TokensEvent"]
    G --> J["ResultEvent / ErrorEvent / RetryEvent"]
    E --> K["RunMetrics"]
    F --> L["Terminal / GHA log"]

    subgraph Legend
      direction LR
      _proc["Process"] ~~~ _data["Data"] ~~~ _out[("Output")]
    end

Loading

High-Level Assessment

The three-layer approach (normalized event types → parser callback → renderer) is the right design for this problem. It cleanly separates Claude-specific NDJSON parsing from runtime-agnostic rendering, keeps the callback synchronous (no channels/goroutines needed), and leaves a clear extension point for future runtimes (opencode) to plug in their own parser. Alternatives considered: (1) channels instead of callbacks — adds goroutine complexity with no benefit since the parser already blocks on I/O; (2) embedding rendering directly in the parser — would couple Claude-specific logic to terminal output and make testing harder; (3) a shared transcript-based approach — deferred rendering loses the live streaming benefit. The PR's approach is optimal.

Files changed (11) +2895 / -163

Enhancement (3) +23 / -3

claude.goWire OnEvent callback and default EventRenderer in ClaudeRuntime.Run +12/-1

Wire OnEvent callback and default EventRenderer in ClaudeRuntime.Run

• Replaces the 'progressParser' call with 'parseClaudeStream(r, handler)'. When 'params.OnEvent' is nil, creates a default 'EventRenderer' and wraps it to also populate 'metrics.Model' from 'InitEvent'. When 'OnEvent' is set, delegates directly to the caller's handler.

internal/runtime/claude.go

runtime.goAdd OnEvent func(AgentEvent) field to RunParams +2/-1

Add OnEvent func(AgentEvent) field to RunParams

• Adds an optional 'OnEvent func(AgentEvent)' field to 'RunParams'. When non-nil, the runtime calls it with normalized events during execution; when nil, the default 'EventRenderer' is used.

internal/runtime/runtime.go

ui.goAdd ToolProgress printer method; change Heartbeat icon to ⏳ +9/-1

Add ToolProgress printer method; change Heartbeat icon to ⏳

• Adds 'ToolProgress(text string)' that renders with blue 'ColorInfo' and a ⚙ icon for tool invocations. Changes 'Heartbeat' icon from ⟳ to ⏳ to visually distinguish timer/waiting events from active tool calls.

internal/ui/ui.go

Refactor (1) +264 / -109

claude_progress.goRefactor: replace progressParser with parseClaudeStream emitting normalized AgentEvents +264/-109

Refactor: replace progressParser with parseClaudeStream emitting normalized AgentEvents

• Rewrites the NDJSON parser as 'parseClaudeStream(r, onEvent)' that processes all stream-json event types: system init/retry, stream_event deltas (text, thinking, tool input JSON accumulation, token usage), result, error, and assistant fallback. Removes 'allowedTools' blocklist and 'emitToolProgress'/'parseAssistantToolUse' helpers. Replaces 'extractBinaryName' with 'collapseCommand' that shows full commands truncated at 120 chars. Adds 'Agent' prompt context extraction. Keeps 'progressParser' as a thin wrapper.

internal/runtime/claude_progress.go

Tests (3) +573 / -51

event_test.goNew: compile-time interface satisfaction test for all AgentEvent types +21/-0

New: compile-time interface satisfaction test for all AgentEvent types

• Adds a single test that appends all 8 concrete event types to an '[]AgentEvent' slice, verifying at compile time that each satisfies the interface.

internal/runtime/event_test.go

renderer_test.goNew: comprehensive unit tests for EventRenderer covering all event types +225/-0

New: comprehensive unit tests for EventRenderer covering all event types

• Tests each event type handler, duplicate init suppression, block transitions (thinking→tool), CI '::notice::' annotation, result metrics population, error/retry rendering, and token event display.

internal/runtime/renderer_test.go

claude_progress_test.goUpdate: align existing tests with new behavior; add parseClaudeStream test suite +327/-51

Update: align existing tests with new behavior; add parseClaudeStream test suite

• Updates 'TestExtractBinaryName' → 'TestCollapseCommand' with full-command expectations. Updates 'TestExtractSafeContext' to reflect that Bash commands are no longer redacted. Renames 'TestProgressParserIgnoresStreamEvents' → 'TestProgressParserStreamEventsProcessed'. Removes 'TestProgressParserUnknownToolAllowlisted' and 'TestProgressParserUnknownToolAssistantNoContext' (replaced by new behavior). Adds 12 new 'TestParseClaudeStream*' tests covering all event types, token throttling, and assistant fallback deduplication.

internal/runtime/claude_progress_test.go

Documentation (2) +1819 / -0

2026-07-06-agent-event-stream-rendering-design.mdNew: design spec for agent event stream rendering +304/-0

New: design spec for agent event stream rendering

• Documents the three-layer architecture (normalized event types, runtime-specific parser, EventRenderer), event type table, Claude stream parser mapping, renderer rendering table, ClaudeRuntime wiring, information disclosure considerations, and future opencode integration path.

docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md

2026-07-06-agent-event-stream-rendering.mdNew: six-task TDD implementation plan for agent event stream rendering +1515/-0

New: six-task TDD implementation plan for agent event stream rendering

• Step-by-step implementation plan with code blocks, test commands, and commit messages for each task: event types, OnEvent callback, EventRenderer, Claude parser refactor, ClaudeRuntime wiring, and lint/verification.

docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md

Other (2) +216 / -0

event.goNew: normalized AgentEvent interface and 8 concrete event types +82/-0

New: normalized AgentEvent interface and 8 concrete event types

• Defines the 'AgentEvent' sealed interface with a no-op marker method and 8 concrete structs: 'InitEvent', 'ThinkingEvent', 'TextEvent', 'ToolUseEvent', 'TokensEvent', 'ResultEvent', 'ErrorEvent', 'RetryEvent'. These serve as the runtime-agnostic bridge between Claude's NDJSON parser and the renderer.

internal/runtime/event.go

renderer.goNew: EventRenderer that renders AgentEvent values to terminal with block state tracking +134/-0

New: EventRenderer that renders AgentEvent values to terminal with block state tracking

• Implements 'EventRenderer' with a 'Handle(AgentEvent)' dispatcher. Suppresses duplicate 'InitEvent' headers via 'seenInit' flag, renders thinking (italic/dim 🧠) and text (💬) as streaming deltas with block state tracking, emits tool calls via 'printer.ToolProgress' (blue ⚙), logs token stats, and populates 'RunMetrics' from 'ResultEvent'. CI mode emits '::notice::' annotations to stderr for tool calls.

internal/runtime/renderer.go

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:57 PM UTC · Ended 9:58 PM UTC
Commit: e8381e3 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Command context leaks secrets ✓ Resolved 🐞 Bug ⛨ Security

Description

extractSafeContext now returns the full Bash command (and Agent prompt) via collapseCommand, so
secrets embedded in env var assignments or arguments can be printed to terminal output and CI
annotations. This is a direct information disclosure risk in CI logs (e.g., API keys/tokens in
commands).

Code

internal/runtime/claude_progress.go[R360-363]

		if err := json.Unmarshal(raw, &cmd); err != nil {
			return ""
		}
-		return extractBinaryName(cmd)
+		return collapseCommand(cmd)

Relevance

⭐⭐⭐ High

Repo historically avoids leaking tool args; progress context was “safe” (binary-only) in #284, so
full-command logging likely rejected.

PR-#284

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Bash context now returns the whole command string (not just a binary), and tests demonstrate that
secret-looking values are expected to appear in the extracted context.

internal/runtime/claude_progress.go[342-405]
internal/runtime/claude_progress_test.go[42-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tool context extraction now displays full Bash commands and Agent prompts. These strings often contain secrets (env vars like `API_KEY=...`, tokens in headers, etc.) and will be rendered to logs/CI notices.

## Issue Context
Tests explicitly validate that an `API_KEY=secret123 ...` prefix is preserved by `extractSafeContext`, confirming the new behavior can surface secrets.

## Fix Focus Areas
- Restore a safer default for Bash context (e.g., binary-only), OR
- Implement redaction for common secret patterns before returning command/prompt strings:
 - redact `KEY=VALUE` where KEY matches `(?i)(token|key|secret|password|auth)`
 - redact common flags (`--token`, `--password`, `Authorization:` header values)
- Keep truncation behavior after redaction.
- Update tests to assert secrets are redacted.

### Code pointers
- internal/runtime/claude_progress.go[342-425]
- internal/runtime/claude_progress_test.go[42-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Untrusted text not sanitized ✓ Resolved 🐞 Bug ⛨ Security

Description

EventRenderer prints ThinkingEvent/TextEvent (and the InitEvent label) directly to the terminal via
Printer.Raw/Header without sanitizeOutput, allowing ANSI/control sequences or GitHub Actions
workflow command markers from untrusted model output to reach logs. This can enable log/annotation
injection and make CI output misleading or unsafe.

Code

internal/runtime/renderer.go[R41-66]

+	case InitEvent:
+		if r.seenInit {
+			return
+		}
+		r.seenInit = true
+		r.endBlock()
+		label := e.Model
+		if e.Version != "" {
+			label = fmt.Sprintf("%s (v%s)", e.Model, e.Version)
+		}
+		r.printer.Header("Agent: " + label)
+	case ThinkingEvent:
+		if !r.inThinking {
+			r.endBlock()
+			r.printer.Raw(thinkingStyle.Render("  \U0001f9e0 "))
+			r.inThinking = true
+		}
+		r.printer.Raw(thinkingStyle.Render(e.Text))
+	case TextEvent:
+		if !r.inText {
+			r.endBlock()
+			r.printer.Raw("  \U0001f4ac ")
+			r.inText = true
+		}
+		r.printer.Raw(e.Text)
+	case ToolUseEvent:

Relevance

⭐⭐⭐ High

Team previously pushed sanitizing untrusted output to prevent GHA/ANSI injection (accepted in #764;
partially in #284).

PR-#764
PR-#284

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Renderer emits model-provided text via Raw without sanitization, while the repo already has a
dedicated sanitization function for untrusted sandbox output (including GHA workflow command
markers).

internal/runtime/renderer.go[39-66]
internal/runtime/sanitize.go[11-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`EventRenderer.Handle` writes untrusted model text (thinking/text deltas) directly via `printer.Raw(...)` without calling `sanitizeOutput`, bypassing existing defenses against ANSI/control characters and GHA workflow command markers.

## Issue Context
`sanitizeOutput` is explicitly intended to strip ANSI sequences, control characters, and GitHub Actions workflow command markers from **untrusted sandbox output**. Tool rendering already uses it, but thinking/text does not.

## Fix Focus Areas
- Apply `sanitizeOutput` to all untrusted strings before rendering:
 - ThinkingEvent.Text
 - TextEvent.Text
 - InitEvent.Model / InitEvent.Version (defense-in-depth)
- Keep renderer styling by sanitizing *before* applying lipgloss style.

### Code pointers
- internal/runtime/renderer.go[39-66]
- internal/runtime/sanitize.go[11-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. ToolProgress() lacks unit test ✓ Resolved 📘 Rule violation ▣ Testability

Description

The PR adds Printer.ToolProgress() and changes Printer.Heartbeat() output, but no tests were
added/updated to exercise these behaviors. This violates the requirement to include tests for
modified Go logic and increases risk of regressions in CLI rendering.

Code

internal/ui/ui.go[R140-148]

+	styled := lipgloss.NewStyle().Foreground(ColorMuted).Render("⏳ " + text)
+	fmt.Fprintf(p.w, "  %s\n", styled)
+}
+
+// ToolProgress prints a tool invocation progress line.
+func (p *Printer) ToolProgress(text string) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	styled := lipgloss.NewStyle().Foreground(ColorInfo).Render("⚙ " + text)

Relevance

⭐⭐ Medium

Mixed history on test-coverage enforcement: UI output changes had tests updated in #1493, but
coverage requests were rejected in #2860.

PR-#1493
PR-#2860

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062049 requires tests for new or modified Go logic. The diff adds a new
ToolProgress() method and changes Heartbeat() output in internal/ui/ui.go, but the existing UI
tests do not include coverage for these methods.

Rule 1062049: Require tests for new or modified Go logic
internal/ui/ui.go[140-148]
internal/ui/ui_test.go[1-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`internal/ui/ui.go` introduces a new `ToolProgress()` method and changes the `Heartbeat()` glyph, but the UI package tests do not cover these new/changed behaviors.

## Issue Context
Compliance requires tests for new or modified Go logic. This change affects user-visible terminal output and should be protected by unit tests in the `internal/ui` package.

## Fix Focus Areas
- internal/ui/ui.go[140-148]
- internal/ui/ui_test.go[1-124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. OnEvent bypasses RunMetrics ✓ Resolved 🐞 Bug ≡ Correctness

Description

When RunParams.OnEvent is provided, ClaudeRuntime.Run passes it directly to parseClaudeStream and
skips the default wrapper that populates RunMetrics (model + tool count + result stats). This yields
missing/incorrect metrics for callers using custom event handlers and can break downstream telemetry
aggregation.

Code

internal/runtime/claude.go[R108-119]

+	handler := params.OnEvent
+	if handler == nil {
+		renderer := NewEventRenderer(printer, start, metrics)
+		handler = func(evt AgentEvent) {
+			if init, ok := evt.(InitEvent); ok && metrics.Model == "" {
+				metrics.Model = init.Model
+			}
+			renderer.Handle(evt)
+		}
+	}
+
+	if parseErr := parseClaudeStream(r, handler); parseErr != nil {

Relevance

⭐⭐ Medium

No close precedent for OnEvent needing metrics wrapper; team cares about metrics though (metrics
persistence accepted in #1682).

PR-#1682

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The default path wraps events to set metrics.Model and render (which also updates tool/result
metrics); the custom handler path skips that wrapper entirely, and RunMetrics is expected to be
collected from parsing.

internal/runtime/claude.go[89-123]
internal/runtime/runtime.go[11-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Providing a custom `RunParams.OnEvent` currently disables the default metrics-populating behavior (and model capture), because the wrapper is only installed when `OnEvent == nil`.

## Issue Context
`RunMetrics` is documented as collecting execution statistics from stream parsing, but `parseClaudeStream` only emits events; metrics mutation is done by the default renderer path.

## Fix Focus Areas
- Always update `metrics` regardless of whether `params.OnEvent` is set:
 - On `InitEvent`: set `metrics.Model` if empty
 - On `ToolUseEvent`: increment `metrics.ToolCalls`
 - On `ResultEvent`: set cost/tokens/turns/cache fields
- Then forward the event to the caller-provided handler.

### Code pointers
- internal/runtime/claude.go[89-123]
- internal/runtime/runtime.go[11-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:59 PM UTC · Completed 10:10 PM UTC
Commit: e53532a · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 79.80132% with 61 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/runtime/claude_progress.go	85.71%	17 Missing and 9 partials ⚠️
internal/runtime/claude.go	0.00%	21 Missing ⚠️
internal/runtime/event.go	0.00%	8 Missing ⚠️
internal/ui/ui.go	0.00%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Prior findings status

#	Prior finding	Status
1	server_tool_use fallback gap (low)	❌ Still present
2	Missing server_tool_use tests (low)	❌ Still present
3	Design spec stale references (low)	❌ Still present (partially updated)
4	Implementation plan stale references (low)	❌ Still present

Changes since prior review

Commit 0786900 ("fix(runtime): address review feedback on metrics, token reset, and bounds") addresses multiple review concerns:

1. Metrics decoupled from renderer — EventRenderer is now a pure rendering concern. Metrics capture moved to a wrapper in claude.go (production) and progressParser (test wrapper). Both implementations are identical. Custom OnEvent handlers now get correct metrics automatically.

2. NewEventRenderer simplified — No longer takes start time.Time or *RunMetrics. Dead parameter removed.

3. Tool input bounds — maxToolInputSize = 1 << 20 (1MB) cap prevents unbounded memory growth from large tool input JSON.

4. Token output reset — totalOutput = 0 at message_start prevents stale output token values from persisting across messages.

5. Design spec updated — progressParser signature, EventRenderer struct, and wiring code updated to reflect the new architecture.

All changes are clean and correct. No new issues introduced.

Findings

Low

• [edge-case] internal/runtime/claude_progress.go — Stream path handles tool_use || server_tool_use at content_block_start, but the assistant fallback path only matches case "tool_use":. A server_tool_use block in a complete assistant message (non-streaming path) would be silently dropped.

• [missing-test] internal/runtime/claude_progress_test.go — No test exercises the server_tool_use content block type in either the stream event path or the assistant fallback path. All existing tests use "tool_use" exclusively.

• [stale-reference] docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md — Design spec still references extractBinaryName and allowedTools as "reused" (line 164) and states "The existing allowedTools safeguard carries over unchanged" (line 256). Both were replaced/removed. Does not describe SecretRedactor or sanitizeStreamText.

• [stale-reference] docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md — Implementation plan references extractBinaryName and allowedTools with code examples showing the old masking behavior. Additionally, this file is gitignored (docs/superpowers/plans/*.md) but is being committed.

Previous run

Review

Prior findings status

#	Prior finding	Status
1	TextEvent/ThinkingEvent sanitization bypass (medium)	✅ Fixed — sanitizeStreamText() applied
2	collapseCommand env var/secret exposure (medium)	✅ Fixed — SecretRedactor integration
3	e.Subtype/e.Model/e.Version unsanitized (low)	✅ Fixed — sanitizeOutput() applied
4	Stale ToolUseEvent doc comment (low)	✅ Fixed
5	server_tool_use fallback gap (low)	❌ Still present
6	Missing server_tool_use tests (low)	❌ Still present
7	Design spec stale references (low)	❌ Still present
8	allowedTools removal (low)	Accepted — conscious design choice

Findings

Low

• [edge-case] internal/runtime/claude_progress.go — Stream path handles tool_use || server_tool_use at content_block_start, but the assistant fallback path only matches case "tool_use":. A server_tool_use block in a complete assistant message (non-streaming path) would be silently dropped.

• [missing-test] internal/runtime/claude_progress_test.go — No test exercises the server_tool_use content block type in either the stream event path or the assistant fallback path. All existing tests use "tool_use" exclusively.

• [stale-reference] docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md — Design spec (introduced in this PR) still references extractBinaryName and allowedTools as "reused" and states "The existing allowedTools safeguard carries over unchanged." Both were replaced/removed. Does not describe SecretRedactor or sanitizeStreamText.

• [stale-reference] docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md — Implementation plan references extractBinaryName and allowedTools as code to reuse and shows code examples with allowedTools masking. Implementation diverged during development.

Previous run (2)

Review

This is a re-review following commit e2744dac which fixes gofmt alignment and removes trailing newlines. No substantive code changes since the prior review at SHA 147a4089. The two medium-severity findings from the prior review remain unresolved.

Prior findings status

#	Prior finding	Status
1	TextEvent/ThinkingEvent sanitization bypass	❌ Still present
2	collapseCommand env var/secret exposure	❌ Still present
3	e.Subtype/e.Model/e.Version unsanitized	❌ Still present
4	allowedTools removal / Agent prompt exposure	❌ Still present
5	Dead start parameter	❌ Still present
6	server_tool_use fallback gap	❌ Still present
7	Duplicated model-capture	❌ Still present
8	Custom OnEvent skips metrics	❌ Still present
9	Missing server_tool_use test coverage	❌ Still present
10	Stale ToolUseEvent doc comment	❌ Still present
11	Design spec stale references	❌ Still present

Findings

Medium

• [GHA-workflow-command-injection] internal/runtime/renderer.go:47,53 — TextEvent writes e.Text via printer.Raw(e.Text) without sanitizeOutput(). ThinkingEvent similarly passes e.Text through thinkingStyle.Render() without sanitization. All other event types correctly sanitize their string fields. In production, the Printer writes to os.Stdout. GitHub Actions interprets workflow commands (::set-env::, ::add-mask::) on stdout. If prompt injection causes the model to emit these sequences in text/thinking output, they would be interpreted by the GHA runner.
  Remediation: r.printer.Raw(sanitizeOutput(e.Text)) for TextEvent; r.printer.Raw(thinkingStyle.Render(sanitizeOutput(e.Text))) for ThinkingEvent.

• [secret-exposure] internal/runtime/claude_progress.go — extractBinaryName (returned only binary name, e.g. "make" from "API_KEY=secret123 make test") was replaced by collapseCommand which returns the full command up to 120 characters with no env var redaction. The prior review accepted a redactEnvVar mitigation that stripped leading KEY=VALUE assignments, but the force push removed that redaction. Tests explicitly expect "API_KEY=secret123 curl https://api.example.com" as output. This flows to ::notice:: CI annotations visible to anyone with repo read access.
  Remediation: Restore the redactEnvVar mitigation that strips leading KEY=VALUE assignments.

Low

Finding	File	Description
e.Subtype unsanitized	renderer.go	ResultEvent.Subtype interpolated into printer.Header() without sanitizeOutput(). Also e.Model and e.Version in InitEvent. Lower risk since these flow through Header() not ::notice::, but inconsistent with other fields.
allowedTools removed	claude_progress.go	The allowedTools safeguard was removed. Previously unknown tool names were masked to "tool". Now all tool names pass through. extractSafeContext also handles the Agent tool, exposing up to 120 chars of prompt text via collapseCommand.
Dead start parameter	renderer.go	NewEventRenderer accepts start time.Time but never stores it. The struct has no start field.
server_tool_use fallback gap	claude_progress.go	Stream path handles tool_use || server_tool_use but assistant fallback only matches case "tool_use":.
Duplicated model-capture	claude.go / claude_progress.go	Identical metrics.Model capture logic in default handler and progressParser wrapper.
Custom OnEvent skips metrics	claude.go	Custom handlers don't get automatic RunMetrics population.
Missing server_tool_use tests	claude_progress_test.go	No test exercises server_tool_use in either stream or fallback path.
Stale ToolUseEvent doc comment	event.go	Says "tool" for unknown/non-allowlisted tools but allowlist was removed.
Design spec stale	specs/...design.md	Three stale references to extractBinaryName, allowedTools as "reused" and "unchanged" when both were removed.

Previous run (3)

Review

Verdict: Comment

This is a re-review following a force push that rewrote the branch history from SHA 9d6ee4e7 (10 prior review iterations) into a clean 10-commit series ending at 147a4089. The three-layer architecture (normalized events → runtime-specific parser → runtime-agnostic renderer) remains well-designed. However, the rebase lost several fixes that were applied in response to prior review feedback. Two medium-severity findings require attention.

Force push impact

The prior review approved at SHA 9d6ee4e7 with all findings resolved except one low-severity item. The force push reorganized commits into:

1. 2e27728e docs: add design spec
2. 2be6859c docs: add implementation plan
3. da9bc6c5 refactor: add normalized AgentEvent types
4. cab06bfb refactor: add OnEvent callback
5. 61199e3a refactor: add EventRenderer
6. cf933c5b refactor: replace progressParser with parseClaudeStream
7. 851e767a feat: wire event stream rendering
8. 7f825898 fix: populate ResultEvent.ErrorMessage
9. e61585e7 fix: improve rendering
10. 147a4089 style: fix gofmt alignment

Several fixes from prior commits (f4903b25, 8e1f5db8, b2971172, 0141bbee) were not carried forward into the rebase.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	❌ Regressed — sanitizeOutput() not applied
2	Dead start parameter	⚠ Partially addressed — struct field removed, constructor parameter remains
3	Custom OnEvent skips metrics	❌ Regressed — no metrics wrapper for custom handlers
4	collapseCommand secret exposure	❌ Regressed — env var redaction removed
5	Design spec stale references	❌ Regressed — still claims allowedTools preserved
6	Stale ToolUseEvent doc comment	❌ Regressed — still says \"tool\" for unknown/non-allowlisted tools
7	server_tool_use not in fallback path	❌ Regressed — only tool_use in assistant fallback
8	Duplicated metrics handler logic	❌ Regressed — model-capture duplicated in claude.go and progressParser
9	Missing server_tool_use test coverage	❌ Still present
10	e.Subtype unsanitized in renderer	❌ Regressed — not passed through sanitizeOutput()

Medium-severity findings

1. TextEvent/ThinkingEvent bypass sanitizeOutput — GHA workflow command injection — In renderer.go, the TextEvent handler writes e.Text via printer.Raw(e.Text) without sanitizeOutput(). The ThinkingEvent handler similarly passes e.Text through thinkingStyle.Render() without sanitization. In production, the Printer writes to os.Stdout (run.go:232). GitHub Actions interprets workflow commands (::error::, ::warning::, ::add-mask::) on stdout. If prompt injection causes the model to emit ::set-env:: or ::add-mask:: sequences in its text/thinking output, they would be interpreted by the GHA runner.

All other event types correctly sanitize their string fields: ToolUseEvent sanitizes msg, ErrorEvent sanitizes e.ErrorType and e.Message, RetryEvent sanitizes e.Error, ResultEvent sanitizes e.ErrorMessage. The gap on TextEvent/ThinkingEvent is an inconsistency in sanitization coverage.

Remediation: Apply sanitizeOutput() to e.Text before rendering. For ThinkingEvent: r.printer.Raw(thinkingStyle.Render(sanitizeOutput(e.Text))). For TextEvent: r.printer.Raw(sanitizeOutput(e.Text)).

2. collapseCommand exposes env vars and command arguments — extractBinaryName (which returned only the binary name, e.g. \"make\" from \"API_KEY=secret123 make test\") was replaced by collapseCommand which returns the full command up to 120 characters. The prior review accepted this at the "partially fixed" level where env var assignments (KEY=VALUE prefixes) were redacted via redactEnvVar. The force push removed that redaction. Tests now explicitly expect \"API_KEY=secret123 curl https://api.example.com\" as output. This output flows to ::notice:: CI annotations visible to anyone with repo read access.

Remediation: At minimum, restore the redactEnvVar mitigation that strips leading KEY=VALUE assignments before the binary name. The prior review accepted this as a reasonable trade-off between debuggability and information minimization.

Low-severity findings

Finding	File	Description
e.Subtype unsanitized	renderer.go	ResultEvent.Subtype interpolated into printer.Header() without sanitizeOutput(). Also e.Model and e.Version in InitEvent. Lower risk since these flow through Header() not ::notice::, but inconsistent with other fields.
Dead start parameter	renderer.go	NewEventRenderer accepts start time.Time but never stores it. The struct has no start field.
server_tool_use fallback gap	claude_progress.go	Stream path handles tool_use || server_tool_use but assistant fallback only matches case \"tool_use\":.
Duplicated model-capture	claude.go / claude_progress.go	Identical metrics.Model capture logic in default handler and progressParser wrapper.
Design spec stale	specs/...design.md	Claims \"The existing allowedTools safeguard carries over unchanged\" but allowedTools was removed.
ToolUseEvent doc comment	event.go	Says \"tool\" for unknown/non-allowlisted tools but no masking occurs.
Custom OnEvent skips metrics	claude.go	Custom handlers don't get automatic RunMetrics population.
Agent prompt exposure	claude_progress.go	extractSafeContext shows up to 120 chars of agent prompts via collapseCommand(prompt).
Plan file committed	docs/superpowers/plans/...	Committed despite .gitignore rule docs/superpowers/plans/*.md.
Missing server_tool_use tests	claude_progress_test.go	No test exercises server_tool_use in either stream or fallback path.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (4)

Review

Verdict: Approve

This is a re-review following commit 9d6ee4e7 which removes the implementation plan file docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md (1515 lines). The removal aligns with repository policy — plan files are gitignored (docs/superpowers/plans/*.md in .gitignore) and are not kept in the repo. No code changes occurred in this commit.

The overall PR — normalized AgentEvent types, Claude-specific NDJSON parser, runtime-agnostic EventRenderer — remains well-designed across six review iterations. All prior critical, high, and medium findings were resolved.

Changes since prior review

Commit 9d6ee4e7 makes one change:

1. Plan file removal: Deletes docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md. No references to this file exist elsewhere in the repository. The design spec (docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md) is a separate file and remains in the PR.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args shown (conscious trade-off)
5	Design spec stale references	✅ Fixed (commit 0141bbee)
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)
7	server_tool_use not in fallback path	✅ Fixed (commit b2971172)
8	Duplicated metrics handler logic	✅ Fixed (commit b2971172)
9	Missing server_tool_use test coverage	❌ Still present (low)
10	e.Subtype unsanitized in renderer	✅ Fixed (commit 0141bbee)

Remaining low-severity finding

Missing server_tool_use test coverage — No test exercises the server_tool_use content block type in either the stream event path or the assistant fallback path. All existing tests use "tool_use" exclusively. If a future refactor accidentally removes "server_tool_use" from one of the case branches, no test will catch it.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (5)

Review

Verdict: Approve

This is a re-review following commit 9d6ee4e7 which removes the implementation plan file docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md (1515 lines). The removal aligns with repository policy — plan files are gitignored (docs/superpowers/plans/*.md in .gitignore) and are not kept in the repo. No code changes occurred in this commit.

The overall PR — normalized AgentEvent types, Claude-specific NDJSON parser, runtime-agnostic EventRenderer — remains well-designed across six review iterations. All prior critical, high, and medium findings were resolved.

Changes since prior review

Commit 9d6ee4e7 makes one change:

1. Plan file removal: Deletes docs/superpowers/plans/2026-07-06-agent-event-stream-rendering.md. No references to this file exist elsewhere in the repository. The design spec (docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md) is a separate file and remains in the PR.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args shown (conscious trade-off)
5	Design spec stale references	✅ Fixed (commit 0141bbee)
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)
7	server_tool_use not in fallback path	✅ Fixed (commit b2971172)
8	Duplicated metrics handler logic	✅ Fixed (commit b2971172)
9	Missing server_tool_use test coverage	❌ Still present (low)
10	e.Subtype unsanitized in renderer	✅ Fixed (commit 0141bbee)

Remaining low-severity finding

Missing server_tool_use test coverage — No test exercises the server_tool_use content block type in either the stream event path or the assistant fallback path. All existing tests use "tool_use" exclusively. If a future refactor accidentally removes "server_tool_use" from one of the case branches, no test will catch it.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (6)

Review

Verdict: Approve

This is a re-review following commit 0141bbee which sanitizes ResultEvent.Subtype in the renderer and updates the design spec to match the implementation. Both changes are correct and address the prior review's remaining fixable findings. The overall PR — normalized AgentEvent types, Claude-specific NDJSON parser, runtime-agnostic EventRenderer — remains well-designed across five review iterations.

Changes since prior review

Commit 0141bbee makes two focused changes:

1. e.Subtype sanitization: Adds sanitizeOutput() to both e.Subtype interpolation sites in renderer.go (lines 88 and 93). The Subtype originates from the Claude API's result event and is now treated consistently with other untrusted fields (e.ErrorType, e.Message, e.Error, e.Model, e.Version, e.Text).

2. Design spec updates: Corrects three stale claims in docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md: (a) line 163 area now accurately describes collapseCommand replacing extractBinaryName and the removal of allowedTools; (b) line 253 area now correctly states the allowedTools blocklist was removed with env var redaction via collapseCommand; (c) line 289 area now correctly describes the testing strategy (sanitizeOutput + collapseCommand, not allowedTools filtering).

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args shown (conscious trade-off)
5	Design spec stale references	✅ Fixed (commit 0141bbee)
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)
7	server_tool_use not in fallback path	✅ Fixed (commit b2971172)
8	Duplicated metrics handler logic	✅ Fixed (commit b2971172)
9	Missing server_tool_use test coverage	❌ Still present (low)
10	e.Subtype unsanitized in renderer	✅ Fixed (commit 0141bbee)

Remaining low-severity finding

Missing server_tool_use test coverage — No test exercises the server_tool_use content block type in either the stream event path (claude_progress.go line 186) or the assistant fallback path (line 268). All existing tests use "tool_use" exclusively. If a future refactor accidentally removes "server_tool_use" from one of the case branches, no test will catch it.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (7)

Review

Verdict: Approve

This is a re-review following commit b2971172 which consolidates the duplicated metrics handler logic into RunMetrics.Update() and adds server_tool_use handling to the assistant fallback path. Both changes are clean and correct. The overall PR architecture — normalized events, runtime-specific parser, runtime-agnostic renderer — is well-designed, and the iterative feedback loop across four review rounds has resolved all critical and medium findings.

Changes since prior review

Commit b2971172 makes three focused changes:

1. Metrics consolidation: Extracts the duplicated metricsHandler closure from claude.go and inline metrics handling from progressParser into a single RunMetrics.Update() method on runtime.go. Both call sites now use metrics.Update(evt), eliminating the divergence risk flagged in the prior review.

2. server_tool_use fallback handling: Adds "server_tool_use" alongside "tool_use" in the assistant fallback path's content item switch (line 268 of claude_progress.go). This matches the stream event path (line 180) which already handled both types. Both paths now consistently handle server_tool_use.

3. progressParser wrapper: Updated to use metrics.Update(evt) instead of its own inline metrics handling, making it consistent with the production path in claude.go.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args shown (conscious trade-off)
5	Design spec stale references	❌ Still present (low)
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)
7	server_tool_use not in fallback path	✅ Fixed (commit b2971172)
8	Duplicated metrics handler logic	✅ Fixed (commit b2971172)
9	Missing server_tool_use test coverage	❌ Still present (low)

Remaining low-severity findings

Design spec stale references — The spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md still contains three claims that contradict the implementation: (1) line 163 references extractBinaryName and allowedTools as "reused" — extractBinaryName was replaced by collapseCommand and allowedTools was removed; (2) line 250 states "The existing allowedTools safeguard carries over unchanged" — it was removed; (3) line 284 references allowedTools filtering in testing — no such tests exist. Since the spec is a Draft introduced in this PR, it should be updated to match the implementation.

Missing server_tool_use test coverage — No test exercises the server_tool_use content block type in either the stream path or the assistant fallback path. All existing tests use "tool_use" exclusively. If a future refactor accidentally removes "server_tool_use" from one of the case branches, there is no test to catch it.

e.Subtype unsanitized in ResultEvent label — In renderer.go, the ResultEvent rendering interpolates e.Subtype into the header label (fmt.Sprintf("Result: ERROR (%s)", e.Subtype)) without passing through sanitizeOutput. The Subtype originates from the Claude API's result event. While it flows through printer.Header (not directly to ::notice:: annotations), applying sanitizeOutput would be consistent with the treatment of other untrusted fields.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (8)

Review

Verdict: Comment

This is a re-review following commit 8e1f5db8 which fixes the stale ToolUseEvent doc comment flagged in the prior review. The overall architecture remains sound — three clean layers (normalized events, runtime-specific parser, runtime-agnostic renderer), comprehensive test coverage (87+ tests), and proper sanitization on all output paths. The latest commit resolves the last of the three fully-fixable findings from the prior review.

Changes since prior review

The author's response commit (8e1f5db8) fixes the stale ToolUseEvent doc comment. The comment now correctly reads "Name is the raw tool name from the runtime" and "Summary is empty for tools not recognized by extractSafeContext" — matching the implementation where allowedTools was removed.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args still shown
5	Design spec not updated	❌ Still present
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)

Remaining findings

Design spec inaccurately describes allowedTools and extractBinaryName — The design spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md contains three claims that contradict the implementation: (1) line 163 states extractSafeContext, extractBinaryName, and allowedTools are reused — but extractBinaryName was replaced by collapseCommand and allowedTools was removed; (2) line 250 states "The existing allowedTools safeguard carries over unchanged" — it was removed entirely; (3) line 284 references allowedTools filtering in the testing strategy — no such tests exist. Since the spec was introduced in this same PR, it should reflect the actual implementation.

server_tool_use not handled in assistant fallback path — The stream_event path at line 186 handles both tool_use and server_tool_use content block types, but the assistant fallback path at line 317 only matches "tool_use". If an older Claude Code version emits a complete assistant message containing a server_tool_use block (instead of streaming), that tool invocation will be silently dropped. This is an edge case — server_tool_use via the non-streaming path is rare — but the inconsistency is worth noting.

Duplicated metrics handler logic — The progressParser wrapper (lines 333-350) duplicates the metricsHandler closure from claude.go (lines 110-126). progressParser is no longer called by production code — only by tests. If a new event type is added to one but not the other, the test path and production path will silently diverge.

Missing test coverage for server_tool_use and Agent tool — No test exercises the server_tool_use content block type in either the stream path or the fallback path. The Agent tool case in extractSafeContext (line 391) also lacks a dedicated test — it calls collapseCommand on the prompt field but no test fixture exercises this.

Previous run (9)

Review

Verdict: Comment

This is a re-review following the author's response commit (f4903b25) which addresses the prior review's feedback. The latest commit resolves three of the prior findings fully, partially mitigates the highest-severity finding, and leaves two low-severity documentation issues. The overall design remains sound — well-layered architecture with normalized events, callback-based integration, and clean separation of parsing/rendering/metrics.

Changes since prior review

The author's response commit addresses the prior review directly:

1. ThinkingEvent/TextEvent sanitization (prior low) → Fully fixed. Both events now pass through sanitizeOutput before rendering. Tests (TestRendererSanitizesThinkingText, TestRendererSanitizesTextEvent) verify GHA marker and ANSI escape stripping.

2. Dead start parameter (prior medium) → Fully fixed. NewEventRenderer no longer accepts start time.Time; the struct has no start field.

3. Custom OnEvent skips metrics (prior low) → Fully fixed. A metricsHandler wrapper is now applied to both default and custom event handlers, ensuring RunMetrics is always populated regardless of callback.

4. collapseCommand secret exposure (prior high) → Partially fixed. Leading KEY=VALUE env var assignments are now redacted via redactEnvVar. However, non-env-var command arguments are still shown verbatim (see remaining finding below).

Remaining findings

collapseCommand still exposes non-env-var secrets in command arguments — The redactEnvVar mitigation handles the most common secret leakage vector (env vars), but inline secrets in command arguments pass through. The test at TestExtractSafeContext("bash with args") explicitly expects curl -H 'Bearer token123' https://api.example.com as output — the bearer token is visible. The old extractBinaryName returned only "curl". This output flows to ::notice:: CI annotations and terminal. While the risk is lower than the prior review's assessment (env vars are now handled, sandbox provides isolation, and log readers already have repo access), this remains a regression from the prior information-minimization posture. Consider whether bare binary name + env-var-redacted args would provide a better security/debuggability tradeoff (e.g., redacting all flag values, not just leading env vars).

Design spec not updated to reflect allowedTools removal — The spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." The implementation removes allowedTools entirely — all tool names pass through, and the commit message explains the rationale ("tool names are not sensitive and the trajectory is public"). The spec should be updated to match.

Stale ToolUseEvent doc comment — The doc comment on ToolUseEvent in event.go still reads Name is the tool name ("tool" for unknown/non-allowlisted tools), but the allowlist no longer exists. All tool names pass through as-is; only extractSafeContext returns empty for tools it doesn't recognize.

Previous run (10)

Review

Verdict: Request changes

This PR implements a well-structured event stream rendering architecture for fullsend run — normalized AgentEvent types, a Claude-specific NDJSON parser, and a runtime-agnostic EventRenderer. The layered design is clean and the test coverage is substantial (87 tests). However, there are security regressions in the information disclosure posture that need resolution before merge.

High-severity findings

collapseCommand exposes secrets in Bash command arguments — extractBinaryName was deliberately designed to return only the binary name (e.g., "make" from "API_KEY=secret123 make test"). The replacement collapseCommand returns the full command up to 120 characters, including inline secrets, environment variable assignments, and auth tokens. The test updates confirm this is intentional (want: "API_KEY=secret123 curl https://api.example.com"). This output flows to GHA ::notice:: annotations visible to anyone with repo read access. This is a regression from an explicit security control.

Medium-severity findings

1. allowedTools guard removed despite design spec — The design spec included in this PR states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." But the implementation removes allowedTools entirely. All tool names from untrusted sandbox output are now displayed verbatim.

2. Dead start parameter in NewEventRenderer — The constructor accepts start time.Time but never stores it (the struct has no start field). The plan document shows elapsed time in tool progress, but the implementation omits it. This is dead code and a plan-implementation mismatch.

3. Stale ToolUseEvent doc comment — The doc comment says Name is the tool name ("tool" for unknown/non-allowlisted tools) but the implementation passes real tool names through without masking.

Low-severity findings

4. Test integrity concern — TestProgressParserUnknownToolAllowlisted was renamed and its assertion inverted to match the guard removal. The companion test TestProgressParserUnknownToolAssistantNoContext was deleted. Security-relevant test assertions were weakened alongside the production change.

5. Custom OnEvent skips model capture — When params.OnEvent is caller-supplied, the model-capture logic (metrics.Model = init.Model) is not applied. This is an undocumented requirement for custom handlers.

6. ThinkingEvent/TextEvent bypass sanitizeOutput — These events are rendered via printer.Raw() without sanitization. If a prompt-injected model emits GHA workflow commands (e.g., ::error::...) in thinking text, they could create spurious annotations. The ToolUseEvent path correctly sanitizes.

Previous run (11)

Review

Verdict: Comment

This is a re-review following commit 8e1f5db8 which fixes the stale ToolUseEvent doc comment flagged in the prior review. The overall architecture remains sound — three clean layers (normalized events, runtime-specific parser, runtime-agnostic renderer), comprehensive test coverage (87+ tests), and proper sanitization on all output paths. The latest commit resolves the last of the three fully-fixable findings from the prior review.

Changes since prior review

The author's response commit (8e1f5db8) fixes the stale ToolUseEvent doc comment. The comment now correctly reads "Name is the raw tool name from the runtime" and "Summary is empty for tools not recognized by extractSafeContext" — matching the implementation where allowedTools was removed.

Prior findings status

#	Prior finding	Status
1	ThinkingEvent/TextEvent sanitization	✅ Fixed (commit f4903b25)
2	Dead start parameter	✅ Fixed (commit f4903b25)
3	Custom OnEvent skips metrics	✅ Fixed (commit f4903b25)
4	collapseCommand secret exposure	⚠ Partially fixed — env vars redacted, non-env-var args still shown
5	Design spec not updated	❌ Still present
6	Stale ToolUseEvent doc comment	✅ Fixed (commit 8e1f5db8)

Remaining findings

Design spec inaccurately describes allowedTools and extractBinaryName — The design spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md contains three claims that contradict the implementation: (1) line 163 states extractSafeContext, extractBinaryName, and allowedTools are reused — but extractBinaryName was replaced by collapseCommand and allowedTools was removed; (2) line 250 states "The existing allowedTools safeguard carries over unchanged" — it was removed entirely; (3) line 284 references allowedTools filtering in the testing strategy — no such tests exist. Since the spec was introduced in this same PR, it should reflect the actual implementation.

server_tool_use not handled in assistant fallback path — The stream_event path at line 186 handles both tool_use and server_tool_use content block types, but the assistant fallback path at line 317 only matches "tool_use". If an older Claude Code version emits a complete assistant message containing a server_tool_use block (instead of streaming), that tool invocation will be silently dropped. This is an edge case — server_tool_use via the non-streaming path is rare — but the inconsistency is worth noting.

Duplicated metrics handler logic — The progressParser wrapper (lines 333-350) duplicates the metricsHandler closure from claude.go (lines 110-126). progressParser is no longer called by production code — only by tests. If a new event type is added to one but not the other, the test path and production path will silently diverge.

Missing test coverage for server_tool_use and Agent tool — No test exercises the server_tool_use content block type in either the stream path or the fallback path. The Agent tool case in extractSafeContext (line 391) also lacks a dedicated test — it calls collapseCommand on the prompt field but no test fixture exercises this.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (12)

Review

Verdict: Comment

This is a re-review following the author's response commit (f4903b25) which addresses the prior review's feedback. The latest commit resolves three of the prior findings fully, partially mitigates the highest-severity finding, and leaves two low-severity documentation issues. The overall design remains sound — well-layered architecture with normalized events, callback-based integration, and clean separation of parsing/rendering/metrics.

Changes since prior review

The author's response commit addresses the prior review directly:

1. ThinkingEvent/TextEvent sanitization (prior low) → Fully fixed. Both events now pass through sanitizeOutput before rendering. Tests (TestRendererSanitizesThinkingText, TestRendererSanitizesTextEvent) verify GHA marker and ANSI escape stripping.

2. Dead start parameter (prior medium) → Fully fixed. NewEventRenderer no longer accepts start time.Time; the struct has no start field.

3. Custom OnEvent skips metrics (prior low) → Fully fixed. A metricsHandler wrapper is now applied to both default and custom event handlers, ensuring RunMetrics is always populated regardless of callback.

4. collapseCommand secret exposure (prior high) → Partially fixed. Leading KEY=VALUE env var assignments are now redacted via redactEnvVar. However, non-env-var command arguments are still shown verbatim (see remaining finding below).

Remaining findings

collapseCommand still exposes non-env-var secrets in command arguments — The redactEnvVar mitigation handles the most common secret leakage vector (env vars), but inline secrets in command arguments pass through. The test at TestExtractSafeContext("bash with args") explicitly expects curl -H 'Bearer token123' https://api.example.com as output — the bearer token is visible. The old extractBinaryName returned only "curl". This output flows to ::notice:: CI annotations and terminal. While the risk is lower than the prior review's assessment (env vars are now handled, sandbox provides isolation, and log readers already have repo access), this remains a regression from the prior information-minimization posture. Consider whether bare binary name + env-var-redacted args would provide a better security/debuggability tradeoff (e.g., redacting all flag values, not just leading env vars).

Design spec not updated to reflect allowedTools removal — The spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." The implementation removes allowedTools entirely — all tool names pass through, and the commit message explains the rationale ("tool names are not sensitive and the trajectory is public"). The spec should be updated to match.

Stale ToolUseEvent doc comment — The doc comment on ToolUseEvent in event.go still reads Name is the tool name ("tool" for unknown/non-allowlisted tools), but the allowlist no longer exists. All tool names pass through as-is; only extractSafeContext returns empty for tools it doesn't recognize.

---

Labels: component/runner already applied — no additional labels needed.

Review

Verdict: Request changes

This PR implements a well-structured event stream rendering architecture for fullsend run — normalized AgentEvent types, a Claude-specific NDJSON parser, and a runtime-agnostic EventRenderer. The layered design is clean and the test coverage is substantial (87 tests). However, there are security regressions in the information disclosure posture that need resolution before merge.

High-severity findings

collapseCommand exposes secrets in Bash command arguments — extractBinaryName was deliberately designed to return only the binary name (e.g., "make" from "API_KEY=secret123 make test"). The replacement collapseCommand returns the full command up to 120 characters, including inline secrets, environment variable assignments, and auth tokens. The test updates confirm this is intentional (want: "API_KEY=secret123 curl https://api.example.com"). This output flows to GHA ::notice:: annotations visible to anyone with repo read access. This is a regression from an explicit security control.

Medium-severity findings

1. allowedTools guard removed despite design spec — The design spec included in this PR states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." But the implementation removes allowedTools entirely. All tool names from untrusted sandbox output are now displayed verbatim.

2. Dead start parameter in NewEventRenderer — The constructor accepts start time.Time but never stores it (the struct has no start field). The plan document shows elapsed time in tool progress, but the implementation omits it. This is dead code and a plan-implementation mismatch.

3. Stale ToolUseEvent doc comment — The doc comment says Name is the tool name ("tool" for unknown/non-allowlisted tools) but the implementation passes real tool names through without masking.

Low-severity findings

4. Test integrity concern — TestProgressParserUnknownToolAllowlisted was renamed and its assertion inverted to match the guard removal. The companion test TestProgressParserUnknownToolAssistantNoContext was deleted. Security-relevant test assertions were weakened alongside the production change.

5. Custom OnEvent skips model capture — When params.OnEvent is caller-supplied, the model-capture logic (metrics.Model = init.Model) is not applied. This is an undocumented requirement for custom handlers.

6. ThinkingEvent/TextEvent bypass sanitizeOutput — These events are rendered via printer.Raw() without sanitization. If a prompt-injected model emits GHA workflow commands (e.g., ::error::...) in thinking text, they could create spurious annotations. The ToolUseEvent path correctly sanitizes.

---

Labels: PR modifies internal/runtime event streaming and internal/ui printer

Previous run (13)

Review

Verdict: Comment

This is a re-review following the author's response commit (f4903b25) which addresses the prior review's feedback. The latest commit resolves three of the prior findings fully, partially mitigates the highest-severity finding, and leaves two low-severity documentation issues. The overall design remains sound — well-layered architecture with normalized events, callback-based integration, and clean separation of parsing/rendering/metrics.

Changes since prior review

The author's response commit addresses the prior review directly:

1. ThinkingEvent/TextEvent sanitization (prior low) → Fully fixed. Both events now pass through sanitizeOutput before rendering. Tests (TestRendererSanitizesThinkingText, TestRendererSanitizesTextEvent) verify GHA marker and ANSI escape stripping.

2. Dead start parameter (prior medium) → Fully fixed. NewEventRenderer no longer accepts start time.Time; the struct has no start field.

3. Custom OnEvent skips metrics (prior low) → Fully fixed. A metricsHandler wrapper is now applied to both default and custom event handlers, ensuring RunMetrics is always populated regardless of callback.

4. collapseCommand secret exposure (prior high) → Partially fixed. Leading KEY=VALUE env var assignments are now redacted via redactEnvVar. However, non-env-var command arguments are still shown verbatim (see remaining finding below).

Remaining findings

collapseCommand still exposes non-env-var secrets in command arguments — The redactEnvVar mitigation handles the most common secret leakage vector (env vars), but inline secrets in command arguments pass through. The test at TestExtractSafeContext("bash with args") explicitly expects curl -H 'Bearer token123' https://api.example.com as output — the bearer token is visible. The old extractBinaryName returned only "curl". This output flows to ::notice:: CI annotations and terminal. While the risk is lower than the prior review's assessment (env vars are now handled, sandbox provides isolation, and log readers already have repo access), this remains a regression from the prior information-minimization posture. Consider whether bare binary name + env-var-redacted args would provide a better security/debuggability tradeoff (e.g., redacting all flag values, not just leading env vars).

Design spec not updated to reflect allowedTools removal — The spec at docs/superpowers/specs/2026-07-06-agent-event-stream-rendering-design.md states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." The implementation removes allowedTools entirely — all tool names pass through, and the commit message explains the rationale ("tool names are not sensitive and the trajectory is public"). The spec should be updated to match.

Stale ToolUseEvent doc comment — The doc comment on ToolUseEvent in event.go still reads Name is the tool name ("tool" for unknown/non-allowlisted tools), but the allowlist no longer exists. All tool names pass through as-is; only extractSafeContext returns empty for tools it doesn't recognize.

---

Labels: component/runner already applied — no additional labels needed.

Previous run (14)

Review

Verdict: Request changes

This PR implements a well-structured event stream rendering architecture for fullsend run — normalized AgentEvent types, a Claude-specific NDJSON parser, and a runtime-agnostic EventRenderer. The layered design is clean and the test coverage is substantial (87 tests). However, there are security regressions in the information disclosure posture that need resolution before merge.

High-severity findings

collapseCommand exposes secrets in Bash command arguments — extractBinaryName was deliberately designed to return only the binary name (e.g., "make" from "API_KEY=secret123 make test"). The replacement collapseCommand returns the full command up to 120 characters, including inline secrets, environment variable assignments, and auth tokens. The test updates confirm this is intentional (want: "API_KEY=secret123 curl https://api.example.com"). This output flows to GHA ::notice:: annotations visible to anyone with repo read access. This is a regression from an explicit security control.

Medium-severity findings

1. allowedTools guard removed despite design spec — The design spec included in this PR states "The existing allowedTools safeguard carries over unchanged" and "unknown tools get name "tool" and empty summary." But the implementation removes allowedTools entirely. All tool names from untrusted sandbox output are now displayed verbatim.

2. Dead start parameter in NewEventRenderer — The constructor accepts start time.Time but never stores it (the struct has no start field). The plan document shows elapsed time in tool progress, but the implementation omits it. This is dead code and a plan-implementation mismatch.

3. Stale ToolUseEvent doc comment — The doc comment says Name is the tool name ("tool" for unknown/non-allowlisted tools) but the implementation passes real tool names through without masking.

Low-severity findings

4. Test integrity concern — TestProgressParserUnknownToolAllowlisted was renamed and its assertion inverted to match the guard removal. The companion test TestProgressParserUnknownToolAssistantNoContext was deleted. Security-relevant test assertions were weakened alongside the production change.

5. Custom OnEvent skips model capture — When params.OnEvent is caller-supplied, the model-capture logic (metrics.Model = init.Model) is not applied. This is an undocumented requirement for custom handlers.

6. ThinkingEvent/TextEvent bypass sanitizeOutput — These events are rendered via printer.Raw() without sanitization. If a prompt-injected model emits GHA workflow commands (e.g., ::error::...) in thinking text, they could create spurious annotations. The ToolUseEvent path correctly sanitizes.

---

Labels: PR modifies internal/runtime event streaming and internal/ui printer

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:25 PM UTC · Completed 4:34 PM UTC
Commit: f4903b2 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:20 PM UTC · Completed 5:30 PM UTC
Commit: 8e1f5db · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:47 PM UTC · Completed 7:59 PM UTC
Commit: b297117 · View workflow run →

[rh-hemartin]

LGTM but not sure if this will be too verbose. Also you are submitting plans and specs. Remove those. As per the verbosity, we can always go back to a less verbose thing.

[waynesun09]

Security-focused review. Three unique medium-and-above findings inline, all stemming from the switch to live-rendering the untrusted agent stream:

• HIGH — text/thinking deltas rendered without sanitizeOutput → ANSI terminal-escape injection and GitHub Actions ::command:: injection.
• MEDIUM — full Bash command (and Agent prompts) now displayed instead of the binary name, leaking secrets into terminal and persisted CI logs.
• MEDIUM — allowedTools allowlist removed and event.go doc comments left claiming masking/"safe for display" that no longer holds.

The common fix is to treat every rendered string from the stream as untrusted: sanitize text/thinking/model/subtype (newline-preserving variant), and restore the binary-name/allowlist disclosure controls the pre-PR code and this PR's own design spec require. Lower-severity notes (unsanitized model/version/subtype headers, unbounded toolInputJSON accumulation) not posted inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:43 PM UTC · Completed 2:57 PM UTC
Commit: 85d7734 · View workflow run →

[waynesun09]

Review Squad — #3186

Agents: 5 dispatched (2x Claude coder, 1x Claude researcher, 1x Codex; Gemini timed out)
Models: Claude, Codex

4 unique findings after dedup, verification, and filtering out intentional design decisions (allowedTools removal, collapseCommand, sanitization — all addressed in prior fix commits).

• 2 HIGH — OnEvent bypasses metrics population; totalOutput not reset on message_start
• 2 MEDIUM — dead start parameter / lost elapsed time; unbounded toolInputJSON growth

Assisted-by: Claude (review), Codex (review)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:43 PM UTC
Commit: 0786900 · View workflow run →

[waynesun09]

All 4 review findings from the prior round are addressed in 07869001:

1. OnEvent metrics wrapping — handler always wrapped to capture InitEvent.Model, ResultEvent fields, and ToolUseEvent count regardless of custom/default path. EventRenderer is now pure rendering.
2. totalOutput reset — reset to 0 on message_start.
3. Dead start parameter — removed from NewEventRenderer and progressParser signatures.
4. toolInputJSON unbounded growth — capped at 1MB (maxToolInputSize).

Metrics logic is correctly duplicated in both claude.go:Run and progressParser. Tests updated to match new signatures. No new issues introduced.

Assisted-by: Claude (review), Codex (review)

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:16 PM UTC · Completed 6:27 PM UTC
Commit: 0786900 · View workflow run →

[fullsend-ai-retro]

PR #3186 added live event stream rendering to fullsend run, a 3000+ line feature PR authored by ralphbean and merged after 4 days. The review process was extensive: 14 review agent dispatches (10 successful), plus qodo-code-review, plus two human reviewers (rh-hemartin, waynesun09). The most significant workflow issue was a security fix (sanitizeOutput for text/thinking events) that was correctly applied on Jul 7 but silently lost during a rebase. The review agent approved the post-rebase code on Jul 8 and Jul 9 without detecting the regression. It took until Jul 10 for both the review agent and a human security reviewer to independently re-discover the issue, causing 2-3 days of delay and 4+ additional review runs. The human security review (waynesun09) also found state-management bugs (metrics bypass, token counter reset, unbounded memory growth) that automated reviews missed across all runs. Three proposals filed targeting the agents repo and platform.

Proposals filed

• Review agent should detect when previously-fixed findings regress after a rebase (in fullsend-ai/agents)
• Review agent correctness sub-agent should verify state initialization completeness in streaming and event-processing code (in fullsend-ai/agents)
• Reduce review dispatch frequency during rapid fix-commit iterations (in fullsend-ai/fullsend)