Skip to content

fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd - #7208

Draft
ascerra wants to merge 7 commits into
mainfrom
feat/6936-pricing-estimation
Draft

fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd#7208
ascerra wants to merge 7 commits into
mainfrom
feat/6936-pricing-estimation

Conversation

@ascerra

@ascerra ascerra commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Problem

Cancelled agent runs report total_cost_usd: 0 in telemetry. The authoritative cost comes from Claude Code's result event — the terminal NDJSON event in the stream. When GitHub Actions cancels a run, SIGINT reaches the fullsend process but never propagates through openshell's gRPC boundary to the claude process inside the sandbox. Claude never flushes its result event, so cost is lost.

Root cause

openshell sandbox exec does not propagate signals from the CLI process to the in-sandbox process. SIGINT to the openshell CLI just kills the relay — the sandboxed claude process keeps running (orphaned) until the sandbox is torn down, never emitting its result event.

Fix

Three coordinated changes make SIGINT reach claude inside the sandbox:

1. PID file + kill-exec in ExecStreamReader (internal/sandbox/sandbox.go)

  • PID file: Prepend echo $$ > /tmp/.fullsend_exec.pid to the command so the in-sandbox process records its PID at startup
  • Setsid: Put openshell in its own session (SysProcAttr.Setsid = true) so GHA's process-group SIGINT doesn't kill the relay before we can use it
  • cmd.Cancel: On context cancellation, spawn a separate openshell sandbox exec that reads the PID file and sends kill -INT to the in-sandbox process
  • Do NOT signal openshell directly: The relay must stay alive to pipe claude's result event (with cost data) back through stdout
  • WaitDelay (8s): Fallback — if claude doesn't exit within 8s after Cancel returns, Go kills openshell with SIGKILL

2. exec claude in buildRunCommand (internal/runtime/claude.go)

Replace claude with exec claude so the shell process is replaced by claude. This makes the PID file point directly to claude's PID rather than the parent shell.

3. exec fullsend run in action.yml

Replace bash with fullsend so GHA's cancellation signals go directly to the fullsend process instead of bash, which would kill fullsend before the cancel mechanism can run.

Timeline on cancellation

GHA cancel → SIGINT to fullsend process group
  → signalContext() catches SIGINT, cancels context
  → cmd.Cancel fires (openshell is in its own session, untouched by GHA's SIGINT)
  → kill-exec goroutine: openshell sandbox exec → kill -INT <claude-pid>
  → claude receives SIGINT, flushes result event with total_cost_usd
  → openshell relays result event through stdout pipe
  → parseClaudeStream reads ResultEvent, sets metrics.TotalCostUSD
  → cmd.Wait() returns, metrics written to artifact

E2E verification

Tested on fullsend-playground/go-hello (run 34513766953):

sandbox cancel: sending SIGINT into fs-rev-0c8de9ccb3b9
sandbox-cancel: pid=678
sandbox-cancel: kill=0
stream ended: parseErr=<nil> ctxErr=context canceled cost=0.7879
wait done: exitCode=0 waitErr=context canceled

Before: total_cost_usd: 0 on every cancelled run
After: total_cost_usd: 0.7879 — claude flushed its result event within ~25ms of receiving SIGINT

Files changed

  • internal/sandbox/sandbox.go — PID file, Setsid, kill-exec cancel mechanism, WaitDelay
  • internal/sandbox/sandbox_test.go — test with fake openshell verifying SIGINT→flush→output
  • internal/runtime/claude.goexec claude, diagnostic logging at stream end and wait
  • internal/cli/run.go — updated handleRunCancellation docstring to reflect SIGINT mechanism
  • action.ymlexec fullsend run

Review squad results (3 agents: Claude x2, Grok)

  • 0 critical, 0 high findings (5 false positives removed after verification)
  • Docstring mismatch fixed, PID-file diagnostics improved, thread-safety and single-use constraints documented
  • All 17 prior review comments resolved (targeted removed rate-cache code)

Test plan

  • Unit test: TestExecStreamReader_SendsSIGINTOnCancel — fake openshell, trap SIGINT, verify flushed output
  • E2E test: cancelled run on go-hello reports total_cost_usd: 0.7879 (was 0)
  • Existing TestExecStreamReader_OpenshellNotInPath still passes
  • Build compiles clean
  • 3-agent review squad passed

When a run is cancelled, Claude Code never emits its terminal result
event (which carries total_cost_usd), so TotalCostUSD stays zero in
telemetry. This adds a client-side pricing table that estimates cost
from captured token counts and the model's published per-MTok rates.

The estimation runs per-iteration before aggregation, so the estimated
cost flows into the agent span, root span, metrics.json, and status
comment — all four telemetry sinks. On successful runs where the
authoritative cost is already present, the estimation is a no-op.

lookupRates handles provider prefixes (anthropic-vertex/...) and
iteratively strips trailing date/experiment suffixes to match model IDs
against the rate table.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:29 AM UTC · Completed 11:47 AM UTC

Commit: 4549445 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.92

@qodo-code-review

qodo-code-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Three agents miss cancellation signals 🔗 Cross-repo conflict ☼ Reliability ⭐ New
Description
ExecStreamReader records the wrapper shell's $$ and later sends SIGINT only to that PID, but
buildPiRunCommand launches Pi as a child without exec and the Codex builder uses a pipeline
rather than replacing the shell as Claude does. Cancellation of Codex or the agents repository's
Pi-based code, prioritize, and review agents can therefore leave the real agent running without
flushing terminal output until the relay fallback timeout, WaitDelay, or sandbox teardown.
Code

internal/sandbox/sandbox.go[1394]

+	wrapped := fmt.Sprintf("echo $$ > %s; %s", execPIDFile, command)
Relevance

●● Moderate

Potential cancellation gap is plausible, but extending exec semantics to other runtimes is a broader
architectural change.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The cancellation wrapper writes the outer sh -c process's PID and later signals exactly that PID,
so its PID invariant holds only when the runtime replaces the wrapper shell. Claude explicitly does
so, but Codex runs through a pipeline and Pi invokes its binary without exec; both use the
modified generic helper, and the agents repository assigns its code, prioritize, and review agents
to Pi, demonstrating that these consumers do not receive the intended in-sandbox cancellation
signal.

internal/sandbox/sandbox.go[1390-1400]
internal/sandbox/sandbox.go[1417-1438]
internal/runtime/pi_run.go[497-505]
internal/sandbox/sandbox.go[1390-1422]
internal/runtime/claude.go[355-358]
internal/runtime/codex_run.go[365-370]
internal/runtime/codex_run.go[483-483]
internal/runtime/pi_run.go[788-788]
External repo: fullsend-ai/agents, .fullsend/config.yaml [22-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
`ExecStreamReader` assumes the PID written from `$$` becomes the agent PID, but that is only true when the supplied command replaces the wrapper shell. Codex uses a pipeline and Pi launches without `exec`, so cancellation targets `sh` rather than the actual agent process.

## Fix Focus Areas
- internal/sandbox/sandbox.go[1390-1440]
- internal/runtime/codex_run.go[365-370]
- internal/runtime/pi_run.go[497-505]
- internal/sandbox/sandbox_test.go[306-363]

## Recommended Fix
Change the launch protocol so the PID file reliably identifies the actual agent process or its dedicated process group for every `ExecStreamReader` caller. Launch Pi with `exec` after completing its setup and guard commands, update Codex command construction as needed, and add cancellation tests covering all three runtime command shapes to verify that each agent receives SIGINT, exits gracefully, and flushes its final output rather than relying on `WaitDelay`.

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


2. Cancelled thinking runs understate spend 🐞 Bug ≡ Correctness
Description
estimateRunMetricsCost prices m.ReasoningTokens, but Claude's cancellation path neither
preserves TokensEvent.ReasoningTokens in RunMetrics nor includes them in its deferred final
token snapshot. When a Claude run using extended thinking is cancelled before its result event, the
estimator receives zero reasoning tokens and every downstream cost sink records an estimate that
omits them.
Code

internal/cli/pricing.go[R102-104]

+	m.TotalCostUSD = estimateCostFromTokens(m.Model,
+		m.InputTokens, m.OutputTokens, m.ReasoningTokens,
+		m.CacheCreationInputTokens, m.CacheReadInputTokens)
Relevance

●●● Strong

Recent Claude token-accounting precedents accepted preserving reasoning tokens across cancellation
and final snapshots.

PR-#6907
PR-#6924

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The estimator explicitly adds RunMetrics.ReasoningTokens at output-token rates, but Claude only
copies input, output, and cache fields from cancellation-preserving token events. Although
message-delta events contain reasoning tokens, the deferred event used when no result arrives also
omits that field, proving that cancelled runs reach the new estimator with reasoning set to zero;
the same stream area previously required a reasoning-specific correction.

internal/cli/pricing.go[59-67]
internal/runtime/claude.go[149-173]
internal/runtime/claude_progress.go[135-155]
internal/runtime/claude_progress.go[285-308]
PR-#6907

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

## Issue description
Cancelled Claude runs discard reasoning-token usage before the new estimator reads `RunMetrics.ReasoningTokens`, causing their estimated cost and reasoning telemetry to be understated.

## Fix Focus Areas
- internal/runtime/claude_progress.go[135-155]
- internal/runtime/claude_progress.go[285-308]
- internal/runtime/claude.go[157-163]
- internal/cli/pricing.go[102-104]

## Recommended Fix
Make incremental and deferred `TokensEvent` values carry the cumulative reasoning-token total, and copy `TokensEvent.ReasoningTokens` into `RunMetrics.ReasoningTokens` alongside the other cumulative counters. Add a cancellation-style regression test with thinking tokens and no result event, asserting that the final metrics retain reasoning tokens and the estimated cost includes them.

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



Remediation recommended

3. Signal routing can regress untested 📘 Rule violation ▣ Testability ⭐ New
Description
buildRunCommand now relies on exec claude for correct signal delivery, but no modified Claude
runtime test asserts that the generated command includes the shell replacement. If a later command
refactor removes exec, existing tests still pass while the PID file targets the intermediate shell
instead of the process that must flush final telemetry.
Code

internal/runtime/claude.go[R355-358]

+	// exec replaces the shell so ExecStreamReader's PID file points to
+	// claude, not sh — required for the SIGINT-into-sandbox cancel mechanism.
	parts := []string{
-		fmt.Sprintf("cd %s && . %s && claude", params.RepoDir, envFile),
+		fmt.Sprintf("cd %s && . %s && exec claude", params.RepoDir, envFile),
Relevance

●●● Strong

Exact precedent accepted restoring command-construction test coverage; behavioral assertion for exec
is directly relevant.

PR-#1780

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 1062049 requires modified Go logic to have a corresponding changed test with a
behavioral assertion. The focused change introduces the required exec claude behavior, while the
existing basic command test checks the repository path and flags but not the shell replacement.

Rule 1062049: Require tests for new or modified Go logic
internal/runtime/claude.go[355-358]
internal/runtime/claude_test.go[217-223]

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

## Issue description
The new signal-delivery mechanism depends on `buildRunCommand` producing `exec claude`, but the corresponding runtime tests do not assert this behavior.

## Fix Focus Areas
- internal/runtime/claude_test.go[217-223]

## Recommended Fix
Update `TestBuildRunCommand_Basic` to assert that the generated command contains `exec claude`, ensuring removal of the required shell replacement causes a test failure.

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


4. Runtime change skips guide review 📘 Rule violation ⛨ Security ⭐ New
Description
ClaudeRuntime.Run and buildRunCommand change an existing runtime backend's execution behavior,
but the PR description does not state that docs/contributing/runtime-implementation.md was
consulted. Because direct process execution participates in the documented egress-binary contract,
reviewers cannot verify from the change record that the runtime checklist was evaluated.
Code

internal/runtime/claude.go[R355-358]

+	// exec replaces the shell so ExecStreamReader's PID file points to
+	// claude, not sh — required for the SIGINT-into-sandbox cancel mechanism.
	parts := []string{
-		fmt.Sprintf("cd %s && . %s && claude", params.RepoDir, envFile),
+		fmt.Sprintf("cd %s && . %s && exec claude", params.RepoDir, envFile),
Relevance

●● Moderate

The rule is explicit, but historical evidence for requiring PR-description guide consultation is
insufficient.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2889480 requires explicit evidence that the runtime implementation guide was
consulted when an existing runtime.Runtime backend changes. ClaudeRuntime identifies itself as a
runtime implementation, the focused command change alters how its model process is executed, and the
guide specifically documents directly executed runtime binaries.

Rule 2889480: Consult runtime implementation guide when modifying runtime.Runtime backends
internal/runtime/claude.go[22-25]
internal/runtime/claude.go[355-358]
docs/contributing/runtime-implementation.md[37-41]

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

## Issue description
The Claude runtime backend now directly replaces its shell process, but the PR does not record consultation of the runtime implementation guide as required for backend behavior changes.

## Fix Focus Areas
- internal/runtime/claude.go[355-358]

## Recommended Fix
Update the PR description to explicitly state that `docs/contributing/runtime-implementation.md` was consulted and that the existing Claude egress-binary mapping, wire protocol, and workspace layout remain unchanged.

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


5. Users misread cancelled-run costs ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
runAgent now invokes estimateRunMetricsCost even though the CLI and tracing references still
describe total_cost_usd as runtime-reported with no pricing-table fallback. When a cancelled
Claude run has token counts but no result event, the estimate reaches spans, metrics.json, and the
status comment without documentation identifying it as approximate.
Code

internal/cli/run.go[2274]

+		estimateRunMetricsCost(&metrics)
Relevance

●●● Strong

Recent precedents accept documentation updates when user-visible CLI behavior or cost semantics
change.

PR-#6938
PR-#5763
PR-#5976

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed call enables Fullsend-side estimation before aggregation, while the existing user
documentation explicitly says total_cost_usd is runtime-reported, token counts are never used in a
Fullsend calculation, and absent costs propagate as zero. This directly violates the requirement to
update relevant documentation when user-facing output behavior changes.

Rule 2748504: Update docs/ references when changing user-facing behavior
internal/cli/run.go[2271-2274]
internal/cli/pricing.go[70-104]
docs/cli/run.md[97-113]
docs/guides/infrastructure/distributed-tracing.md[210-217]
docs/guides/infrastructure/distributed-tracing.md[252-271]

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

## Issue description
Cancelled Claude runs now use token-based cost estimation, but the CLI and tracing documentation still says missing runtime costs remain zero and that no pricing-table fallback exists.

## Fix Focus Areas
- docs/cli/run.md[97-113]
- docs/guides/infrastructure/distributed-tracing.md[200-217]
- docs/guides/infrastructure/distributed-tracing.md[252-271]
- docs/guides/user/tracing-with-mlflow.md[74-81]

## Recommended Fix
Update the cost contract and CLI reference to explain when Fullsend estimates cost, which token categories and rate table it uses, how unknown models behave, and that estimated values propagate to telemetry and status comments. Distinguish these approximate Fullsend values from authoritative runtime-reported costs and independent backend estimates.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 70 rules
✅ Cross-repo context — repo relationships
  Explored: repo: fullsend-ai/.fullsend (sha: 7c163cad)
  Explored: repo: fullsend-ai/agents (sha: 883141b9)
Review mode: ⚖️ Balanced: Downgraded extended -> standard: change is below the extended eligibility bar (hunks 10/18, lines 446/200; both must reach the floor). Router rationale: This push introduces cancellation, signal propagation, subprocess lifecycle, shell/PID-file, and CI behavior across multiple independent paths, making subtle defects materially likely to require redundant review passes.

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit d419f66 ⚖️ Balanced

Results up to commit 4549445 ⚖️ Balanced


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Cancelled thinking runs understate spend 🐞 Bug ≡ Correctness
Description
estimateRunMetricsCost prices m.ReasoningTokens, but Claude's cancellation path neither
preserves TokensEvent.ReasoningTokens in RunMetrics nor includes them in its deferred final
token snapshot. When a Claude run using extended thinking is cancelled before its result event, the
estimator receives zero reasoning tokens and every downstream cost sink records an estimate that
omits them.
Code

internal/cli/pricing.go[R102-104]

+	m.TotalCostUSD = estimateCostFromTokens(m.Model,
+		m.InputTokens, m.OutputTokens, m.ReasoningTokens,
+		m.CacheCreationInputTokens, m.CacheReadInputTokens)
Relevance

●●● Strong

Recent Claude token-accounting precedents accepted preserving reasoning tokens across cancellation
and final snapshots.

PR-#6907
PR-#6924

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The estimator explicitly adds RunMetrics.ReasoningTokens at output-token rates, but Claude only
copies input, output, and cache fields from cancellation-preserving token events. Although
message-delta events contain reasoning tokens, the deferred event used when no result arrives also
omits that field, proving that cancelled runs reach the new estimator with reasoning set to zero;
the same stream area previously required a reasoning-specific correction.

internal/cli/pricing.go[59-67]
internal/runtime/claude.go[149-173]
internal/runtime/claude_progress.go[135-155]
internal/runtime/claude_progress.go[285-308]
PR-#6907

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

## Issue description
Cancelled Claude runs discard reasoning-token usage before the new estimator reads `RunMetrics.ReasoningTokens`, causing their estimated cost and reasoning telemetry to be understated.

## Fix Focus Areas
- internal/runtime/claude_progress.go[135-155]
- internal/runtime/claude_progress.go[285-308]
- internal/runtime/claude.go[157-163]
- internal/cli/pricing.go[102-104]

## Recommended Fix
Make incremental and deferred `TokensEvent` values carry the cumulative reasoning-token total, and copy `TokensEvent.ReasoningTokens` into `RunMetrics.ReasoningTokens` alongside the other cumulative counters. Add a cancellation-style regression test with thinking tokens and no result event, asserting that the final metrics retain reasoning tokens and the estimated cost includes them.

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



Remediation recommended
2. Users misread cancelled-run costs ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
runAgent now invokes estimateRunMetricsCost even though the CLI and tracing references still
describe total_cost_usd as runtime-reported with no pricing-table fallback. When a cancelled
Claude run has token counts but no result event, the estimate reaches spans, metrics.json, and the
status comment without documentation identifying it as approximate.
Code

internal/cli/run.go[2274]

+		estimateRunMetricsCost(&metrics)
Relevance

●●● Strong

Recent precedents accept documentation updates when user-visible CLI behavior or cost semantics
change.

PR-#6938
PR-#5763
PR-#5976

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed call enables Fullsend-side estimation before aggregation, while the existing user
documentation explicitly says total_cost_usd is runtime-reported, token counts are never used in a
Fullsend calculation, and absent costs propagate as zero. This directly violates the requirement to
update relevant documentation when user-facing output behavior changes.

Rule 2748504: Update docs/ references when changing user-facing behavior
internal/cli/run.go[2271-2274]
internal/cli/pricing.go[70-104]
docs/cli/run.md[97-113]
docs/guides/infrastructure/distributed-tracing.md[210-217]
docs/guides/infrastructure/distributed-tracing.md[252-271]

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

## Issue description
Cancelled Claude runs now use token-based cost estimation, but the CLI and tracing documentation still says missing runtime costs remain zero and that no pricing-table fallback exists.

## Fix Focus Areas
- docs/cli/run.md[97-113]
- docs/guides/infrastructure/distributed-tracing.md[200-217]
- docs/guides/infrastructure/distributed-tracing.md[252-271]
- docs/guides/user/tracing-with-mlflow.md[74-81]

## Recommended Fix
Update the cost contract and CLI reference to explain when Fullsend estimates cost, which token categories and rate table it uses, how unknown models behave, and that estimated values propagate to telemetry and status comments. Distinguish these approximate Fullsend values from authoritative runtime-reported costs and independent backend estimates.

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


Grey Divider

Qodo Logo

Comment thread internal/cli/run.go Outdated
Comment thread internal/cli/pricing.go Outdated
Comment on lines +102 to +104
m.TotalCostUSD = estimateCostFromTokens(m.Model,
m.InputTokens, m.OutputTokens, m.ReasoningTokens,
m.CacheCreationInputTokens, m.CacheReadInputTokens)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Cancelled thinking runs understate spend 🐞 Bug ≡ Correctness

estimateRunMetricsCost prices m.ReasoningTokens, but Claude's cancellation path neither
preserves TokensEvent.ReasoningTokens in RunMetrics nor includes them in its deferred final
token snapshot. When a Claude run using extended thinking is cancelled before its result event, the
estimator receives zero reasoning tokens and every downstream cost sink records an estimate that
omits them.
Agent Prompt
## Issue description
Cancelled Claude runs discard reasoning-token usage before the new estimator reads `RunMetrics.ReasoningTokens`, causing their estimated cost and reasoning telemetry to be understated.

## Fix Focus Areas
- internal/runtime/claude_progress.go[135-155]
- internal/runtime/claude_progress.go[285-308]
- internal/runtime/claude.go[157-163]
- internal/cli/pricing.go[102-104]

## Recommended Fix
Make incremental and deferred `TokensEvent` values carry the cumulative reasoning-token total, and copy `TokensEvent.ReasoningTokens` into `RunMetrics.ReasoningTokens` alongside the other cumulative counters. Add a cancellation-style regression test with thinking tokens and no result event, asserting that the final metrics retain reasoning tokens and the estimated cost includes them.

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

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.64516% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/runtime/claude.go 20.00% 4 Missing ⚠️
internal/sandbox/sandbox.go 92.30% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 10, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Tier 1 dropped sharply from the prior review's context (11 files/827 lines) to 5 files/151 lines with no protected, security, CI, or dependency signals, yielding a low Tier 1 composite; this is weighted against a high Tier 2 composite driven by very hot churn/author/fix-revert history on run.go, sandbox.go, and claude.go, and a Tier 3 composite reflecting good scope alignment offset by an unflagged change to existing signal-propagation behavior with no rollback flag. The weighted composite rounds to 2 (moderate), matching the prior score but for a materially smaller blast radius — file/line reduction lowered Tier 1's contribution while sustained hotspot churn on the same files kept Tier 2 elevated.

Previous run

Risk Assessment: moderate (2/5)

Details

Score holds at 2 (moderate), matching the prior assessment: fresh Tier 1 evaluation of the now-11-file/827-line diff still finds no protected/security/CI/dependency signals, keeping the heavily-weighted Tier 1 composite low despite the larger blast radius; this outweighs high Tier 2 churn/author/fix-revert history on the perennially hot run.go/sandbox.go/claude.go files and a rollback-safety concern from unflagged existing-behavior changes, offset by scope-aligned issue coverage.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Composite = 0.50(1.75) + 0.30(3.29) + 0.20(2.6) = 2.38 -> 2 (moderate), matching the prior assessment: re-evaluating Tier 1 fresh for the now-10-file, 761-line diff still finds no protected/security/CI/dependency signals, so the heavily-weighted Tier 1 composite stays low (1.75) despite the larger blast radius; this outweighs high Tier 2 churn/coupling/fix-revert history on the perennially hot run.go/sandbox.go/tracing.md files (3.29) and a rollback-safety concern from unflagged existing-behavior changes plus largely-covered issue-scope alignment (2.6), so the score is preserved at 2 rather than escalated.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Tier 1 signals changed materially since the prior assessment (2 new files, larger blast radius) and were re-evaluated fresh rather than anchored; despite high Tier 2 churn/coupling on the touched run.go/run.md/tracing.md hotspots and a rollback-safety concern (no feature flag on a change to existing telemetry behavior), the heavily-weighted Tier 1 composite stays low (good test ratio, no protected/security/CI/dependency signals, experienced non-bot author), yielding a moderate composite score of 2.

Previous run (4)

Risk Assessment: moderate (2/5)

Details

Small, well-tested change touching one high-churn, multi-author file with a recurring fix history, paired with a narrowly-scoped follow-up to an already-substantially-resolved closed issue that lacks a rollback flag — combining to an overall moderate risk score.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [runtime-mechanism] internal/sandbox/sandbox.go:1421ExecStreamReader records echo $$ > /tmp/.fullsend_exec.pid and on cancel sends SIGINT only to that PID (kill -INT $PID). Claude's exec claude replaces the wrapping shell, so $$ is the runtime and SIGINT is delivered correctly. Codex still ends in a pipeline (printf ... | "$codex" exec in internal/runtime/codex_run.go) — a shell cannot last-exec a pipeline, so $$ is the wrapper shell and kill -INT $PID never reaches the Codex process. Pi launches "$FULLSEND_PI_BIN" --print without exec (internal/runtime/pi_run.go); it may happen to inherit $$ via shell last-command-exec, but that is implicit and untested. Both non-Claude callers also now pay the new 8s WaitDelay on every cancel/timeout before the openshell relay is SIGKILLed. TestExecStreamReader_SendsSIGINTOnCancel only covers a cooperative same-PID child, so this gap is untested. This mechanism is squarely this PR's stated purpose ("send SIGINT into sandbox so cancelled runs report total_cost_usd"), and as implemented it verifiably works for Claude but not for two of the three supported runtimes.
    Remediation: Either exec the payload in every ExecStreamReader caller (so $$ is always the runtime process, matching the Claude pattern) or deliver SIGINT to the recorded PID's process group (kill -INT -- -$PID) so wrapper-shell children receive it. Extend the cancellation test to a wrapping shell with a non-exec'd child and a pipeline child.

Low

  • [permission-expansion] internal/sandbox/sandbox.go:1406Setsid: true moves the openshell relay into a new session, and cmd.Cancel returns nil without ever signaling that host process directly — the only host-side kill is cmd.WaitDelay = 8s, with no Pdeathsig. If the parent fullsend process is SIGKILLed before WaitDelay fires, the relay can be reparented to init and keep running. An 8s WaitDelay also sits at or past GitHub Actions' own ~7.5s SIGINT-to-SIGKILL step window, so the intended force-kill of the relay may never run on a cancelled GHA job. Blast radius is limited to an already-sandboxed relay process (bounded by the existing --timeout, and torn down with the GHA VM), so this is a lifecycle-hardening gap rather than a genuine privilege expansion.
    Remediation: If Setsid is required, also set SysProcAttr.Pdeathsig = syscall.SIGKILL (Linux) so a killed parent cannot leave the relay orphaned. Shrink WaitDelay (or the internal 5s kill-exec timeout) so cleanup still fits inside GitHub Actions' cancellation budget.

  • [fail-open] internal/sandbox/sandbox.go:1421ExecStreamReader's cancel script still treats the in-sandbox PID file as the sole SIGINT target. The new [ -z "$PID" ] check (added since the prior review) closes the absent/empty-file case, but the script still doesn't validate the content is a bare positive integer, and kill -INT $PID remains unquoted — a malformed PID file could still pass multiple IFS-split operands or a negative value. Blast radius stays inside the sandbox, and a bad PID still degrades to the WaitDelay fallback rather than granting host access, so this is a partial fix of the prior finding rather than a new issue.
    Remediation: Reject non-numeric PID file contents (e.g. case "$PID" in ''|*[!0-9]*) exit 1;; esac) before signaling, and use kill -INT -- "$PID".

  • [logging-conventions] internal/runtime/claude.go:181fmt.Fprintf(os.Stderr, " stream ended: parseErr=%v ctxErr=%v cost=%.4f\n", ...) (line 181) and fmt.Fprintf(os.Stderr, " wait done: exitCode=%d waitErr=%v\n", ...) (line 193) execute unconditionally on every ClaudeRuntime.Run, including normal successful runs, adding stderr noise beyond the existing error-only " progress parser: %v\n" pattern. parseErr is also printed with a raw %v here (unsanitized) before being sanitized again on the error-handling path a few lines later.
    Remediation: Remove the unconditional diagnostic prints, or gate them behind an explicit debug/verbose flag if they are meant to persist as permanent diagnostics.

  • [test-idioms] internal/sandbox/sandbox_test.go:356 — In TestExecStreamReader_SendsSIGINTOnCancel, waitErr := cmd.Wait() assigns the error to a named variable that is immediately discarded via _ = waitErr — dead code / unnecessary indirection that also misses an opportunity to assert the wait outcome.
    Remediation: Replace with _ = cmd.Wait() // wait for subprocess cleanup after cancellation, or assert the expected outcome.

These findings should be addressed before merge — the medium-severity SIGINT-targeting gap means this PR's stated fix does not yet work for the pi and codex runtimes, only for claude.


Since the prior review: this PR pivoted away from the previously-reviewed "cost-estimation via historical rate cache" approach — internal/cli/pricing.go, internal/cli/pricing_test.go, and the related doc changes to docs/guides/infrastructure/distributed-tracing.md/docs/cli/run.md were fully removed/reverted (both docs are now byte-identical to the base branch). All of the prior review's findings tied to that code (rate-cache persistence, Codex cost fabrication, missing pricing tests, both stale-doc findings) are now moot. The PR title/body were also updated to correctly describe the SIGINT/cancellation-lifecycle scope, resolving the prior scope-authorization-partial finding, and the prior doc-comment mismatch (WaitDelay "5-second" vs. actual 8 * time.Second) has been fixed.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run

Review

Findings

Medium

  • [runtime-mechanism] internal/cli/pricing.go:28 — The rate cache is not persisted across GitHub Actions jobs. rateCachePath writes <fullsendDir>/.fullsend-cache/pricing-rates.json; the reusable dispatch workflow only restores/saves ${{ runner.temp }}/fullsend-cache (the CLI binary), and .gitignore excludes .fullsend-cache/. recordModelRates is a no-op when TotalCostUSD is 0, so a cancelled run can't seed its own cache either. On the exact environment issue Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936 describes, estimateRunMetricsCost is inert on every job unless a prior successful iteration already wrote the file within the same job, and cancelled runs still report $0.00. (Unchanged since the prior review.)
    Remediation: Add an actions/cache restore/save (or equivalent) for <fullsend-dir>/.fullsend-cache/pricing-rates.json in the dispatch workflow(s), or state in the cost-contract docs that the estimator is inert on ephemeral GitHub Actions runners.

  • [logic-error] internal/cli/run.go:2278estimateRunMetricsCost is invoked after every iteration with no cancellation check; the only guard is TotalCostUSD > 0 inside pricing.go. Codex always sets TotalCostUSD to 0, including on success. If a prior cost-reporting runtime stored a rate under the same normalized model key, a successful Codex run gets a fabricated cost, contradicting docs/cli/run.md and the Cost data contract, which describe the fallback as cancelled-runs only. No source marker distinguishes an estimate from a runtime-reported value. (Unchanged since the prior review.)
    Remediation: Gate estimateRunMetricsCost to the cancelled/incomplete-stream path so successful zero-cost ResultEvents (Codex) stay at 0. Consider adding a cost-source marker.

  • [missing-test] internal/cli/pricing_test.go:82 — The new tests cover isolated record/estimate arithmetic but not the Codex-shaped case (tokens present, cost 0, seeded cache under the same normalized key), and never run cancelled-shaped metrics through the real estimateRunMetricsCostaggregateRunMetrics sequence, so they would not catch the Codex fabricated-cost bug above. (Unchanged since the prior review.)
    Remediation: Add (1) a test that seeds the cache then runs cancelled-shaped metrics through estimateRunMetricsCost and aggregateRunMetrics, asserting the estimate is aggregated; (2) a Codex-shaped successful RunMetrics test with a seeded same-model cache pinning whether TotalCostUSD must stay 0.

  • [runtime-mechanism] internal/sandbox/sandbox.go:1418ExecStreamReader records echo $$ > /tmp/.fullsend_exec.pid and on cancel sends SIGINT to only that PID. buildRunCommand's exec claude makes $$ the Claude process, so Claude receives SIGINT correctly. But pi launches "$pi" --print ... as a child of the wrapping shell (no exec), and Codex's last step is a pipeline printf | "$codex" exec, so in both cases $$ is the wrapping shell, not the runtime process. SIGINT is delivered to the shell, which the runtime child never sees — pi and Codex runs get no graceful-cancellation opportunity and rely entirely on the 8s WaitDelay force-kill, defeating this PR's own stated purpose (flushing the terminal cost-bearing result event) for two of the three supported runtimes. TestExecStreamReader_SendsSIGINTOnCancel only covers a cooperative same-PID child, so this gap is untested.
    Remediation: Either exec the payload in every ExecStreamReader caller (so $$ is always the runtime process, matching the Claude pattern), or deliver SIGINT to the recorded PID's process group instead of the single PID. Extend the cancellation test to a wrapping shell with a non-exec'd child.

  • [permission-expansion] internal/sandbox/sandbox.go:1400Setsid: true moves the openshell relay process into a new session, and cmd.Cancel returns nil without ever signaling that process directly — the 8s WaitDelay is the only host-side mechanism that can still kill it. This detaches openshell from the GHA step's process group, so a group-wide SIGINT/SIGTERM/SIGKILL no longer reaches it directly the way it did before this PR. There is no Pdeathsig; if the parent fullsend process is SIGKILLed before WaitDelay fires, the relay is reparented to init and can keep running. An 8s WaitDelay also sits at or past GitHub Actions' own ~7.5s SIGINT-to-SIGKILL grace window, so the intended force-kill of the relay may never run on a cancelled GHA job.
    Remediation: If Setsid is required to protect the relay from a premature group-wide SIGINT before the in-sandbox result event is flushed, also set SysProcAttr.Pdeathsig = syscall.SIGKILL (Linux) so a killed parent cannot leave the relay orphaned. Shrink WaitDelay (or the internal kill-exec timeout) so cleanup still fits inside GitHub Actions' cancellation budget.

  • [stale-doc] docs/guides/infrastructure/distributed-tracing.md:195 — The Cost data contract still opens with "Fullsend does not calculate inference cost from token counts or maintain a model-price table" (line 195) and "They are not inputs to any fullsend-side cost calculation" (line 225); the later "Distinction from backend-derived cost estimates" subsection still states the authoritative cost is always fullsend.cost_usd/total_cost_usd (line 273). All three passages contradict the new "Estimated cost on cancelled runs" subsection added in this PR, which derives per-token rates from historical cost and multiplies by token counts into those same fields, with no marker distinguishing estimate from runtime-reported cost. (Unchanged since the prior review.)
    Remediation: Revise the opening rule, the token-counts sentence, and the "always authoritative" claim to state: fullsend records the runtime-reported cost as-is when present; when absent, it estimates from token counts using the cached per-model rate — and note there is no field-level marker distinguishing the two.

Low

  • [logic-error] internal/cli/pricing.go:183recordModelRates excludes ReasoningTokens from the rate denominator, but estimateRunMetricsCost's non-PerModelUsage path includes m.ReasoningTokens in the total it multiplies by that rate. Cancelled Claude TokensEvents typically have reasoning 0 so this is unaffected today, but it would overstate cost if estimation is ever applied to Codex-shaped metrics. (Unchanged since the prior review.)
    Remediation: Pass 0 for reasoning in estimateRunMetricsCost's top-level sumTokens call to match recordModelRates, or include reasoning consistently on both sides.

  • [logic-error] internal/sandbox/sandbox.go:1378ExecStreamReader's doc comment says "A 5-second WaitDelay allows the graceful shutdown to complete," but cmd.WaitDelay is actually set to 8 * time.Second a few lines later, and the comment doesn't mention that the kill-exec goroutine runs concurrently with WaitDelay rather than before it.
    Remediation: Update the comment to state 8 seconds and describe that WaitDelay starts as soon as Cancel returns, running concurrently with the async kill-exec call.

  • [fail-open] internal/sandbox/sandbox.go:1411ExecStreamReader's cancel path treats the in-sandbox PID file (/tmp/.fullsend_exec.pid) as the sole SIGINT target and is fail-open when that file is absent, empty, or malformed: PID=$(cat ... 2>/dev/null); kill -INT $PID with $PID unquoted. An empty/missing PID makes kill -INT run with no operand, so SIGINT is silently never delivered (WaitDelay is the fallback). Unquoted expansion could pass multiple operands, or a value like -1, if the well-known path were ever overwritten with malformed content. The blast radius is confined to the sandbox, not the GHA host, and the missing-PID case already degrades to the existing WaitDelay fallback rather than granting broader access.
    Remediation: Validate that the PID file's content is a bare positive integer before using it (e.g. case "$PID" in ''|*[!0-9]*) exit 1;; esac) and quote kill -INT "$PID".

  • [scope-authorization-partial] action.yml:449 — Issue Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936's acceptance criteria explicitly ask to "verify the signal/cancellation lifecycle across the Review workflow, the fullsend action, ClaudeRuntime.Run, and the run finalization/telemetry writer" — this authorizes the substance of the changes to action.yml (exec fullsend run, cited inline as referencing Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936), internal/sandbox/sandbox.go (the SIGINT-relay rewrite), and internal/runtime/claude.go (exec claude). This resolves the prior medium-severity "unauthorized scope" concern for the code itself. What remains is a release-note/PR-metadata accuracy gap: the PR title ("feat(Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936): estimate USD cost on cancelled runs via historical rate cache") and the PR body's own "Files changed" list omit any mention of the signal/cancellation-lifecycle rewrite, even though that rewrite grew more extensive since the prior review. GoReleaser builds release notes from PR titles per this repo's COMMITS.md, so this process-signal-handling change would not surface there. (Downgraded from the prior medium-severity scope-creep finding now that the issue's authorization is established.)
    Remediation: Update the PR title/body to explicitly claim both the cost-estimation and signal/cancellation-lifecycle scopes, and update the body's "Files changed" list to include action.yml, internal/runtime/claude.go, and internal/sandbox/sandbox.go.

  • [leftover-debug-code] internal/runtime/claude.go:181 — Two new unconditional fmt.Fprintf(os.Stderr, ...) debug lines (" stream ended: parseErr=%v ctxErr=%v cost=%.4f" at line 181 and " wait done: exitCode=%d waitErr=%v" at line 193) execute on every agent run, including normal successful runs. These read as developer debug output left over from the signal-lifecycle debugging work rather than intentional, permanent diagnostics, and are not mentioned in the PR's stated design decisions.
    Remediation: Remove the leftover fmt.Fprintf(os.Stderr, ...) statements at lines 181 and 193, or gate them behind an explicit debug/verbose flag if they are meant to persist.

  • [leftover-debug-code] internal/sandbox/sandbox.go:1408 — The killScript constructed in ExecStreamReader's Cancel function contains debug echo statements (echo "sandbox-cancel: pid=$PID" >&2 and echo "sandbox-cancel: kill=$?" >&2) that look like leftover troubleshooting output. See also: [fail-open] finding on this file — the raw $PID echoed here is the same value flagged there for lacking validation.
    Remediation: Remove the debug echo commands from killScript, or route their content through existing output sanitization if they are meant to persist as operator-facing diagnostics.

  • [incorrect-doc] docs/guides/infrastructure/distributed-tracing.md:258 — The new subsection says rates are stored in .fullsend-cache/pricing-rates.json. The code uses filepath.Join(fullsendDir, ".fullsend-cache", "pricing-rates.json"), i.e. .fullsend/.fullsend-cache/pricing-rates.json when --fullsend-dir is .fullsend. The workspace-root .fullsend-cache/ is a different, pre-existing harness fetch cache, so the unqualified path is easy to misread. (Unchanged since the prior review.)
    Remediation: Document the path as <fullsend-dir>/.fullsend-cache/pricing-rates.json (typically .fullsend/.fullsend-cache/pricing-rates.json).

These findings should be addressed before merge — the medium-severity items (rate-cache persistence, Codex cost fabrication, missing test coverage, the SIGINT-targeting gap for pi/Codex, and the openshell relay's weakened kill guarantees) are functional gaps in the cancellation/cost-estimation mechanism this PR is meant to fix, not stylistic nits.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

Medium

  • [runtime-mechanism] internal/cli/pricing.go:27 — The rate cache is not persisted across GitHub Actions jobs. rateCachePath writes <fullsendDir>/.fullsend-cache/pricing-rates.json; the standard per-repo dispatch flow checks out fresh each run and the only actions/cache step restores ${{ runner.temp }}/fullsend-cache (the CLI binary), not this file — .gitignore also excludes .fullsend-cache/. recordModelRates is a no-op when TotalCostUSD is 0, so a cancelled run can't seed its own cache either. On the exact environment issue Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936 describes, estimateRunMetricsCost is a no-op on every job unless a prior successful iteration already wrote the file within the same job, and cancelled runs still report $0.00. (Unchanged since the prior review — this finding still applies.)
    Remediation: Add an actions/cache restore/save (or equivalent) for <fullsend-dir>/.fullsend-cache/pricing-rates.json in the dispatch workflow(s), or state in the cost-contract docs that the estimator is inert on ephemeral GitHub Actions runners and only helps persistent/local workspaces and same-job later iterations.

  • [logic-error] internal/cli/run.go:2278estimateRunMetricsCost is invoked after every iteration with no cancellation check; the only guard is TotalCostUSD > 0 inside pricing.go. Codex always sets TotalCostUSD to 0, including on success. If a prior cost-reporting runtime stored a rate under the same normalized model key (e.g. a pi run recording openai/gpt-4o, later reused by a Codex run on gpt-4o), a successful Codex run gets a fabricated cost, even though both docs/cli/run.md and the Cost data contract describe the fallback as cancelled-runs only. The estimate is written in place into TotalCostUSD/PerModelUsage[].CostUSD with no source marker, so consumers cannot tell it apart from a runtime-reported value. (Unchanged since the prior review.)
    Remediation: Gate estimateRunMetricsCost to the cancelled/incomplete-stream path so successful zero-cost ResultEvents (Codex) stay at 0. Consider adding a cost-source marker if estimates continue to live in the same telemetry fields.

  • [missing-test] internal/cli/pricing_test.go:82 — The new tests cover isolated record/estimate arithmetic but not the Codex-shaped case (tokens present, cost 0, seeded cache under the same normalized key) and never run cancelled-shaped metrics through the real estimateRunMetricsCostaggregateRunMetrics sequence. The existing Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936 cancellation tests in run_test.go still call aggregateRunMetrics/writeMetricsJSON directly and assert TotalCostUSD == 0, so they don't exercise the new estimator and won't catch a regression. (Unchanged since the prior review.)
    Remediation: Add (1) a test that seeds the cache, then runs cancelled-shaped metrics through estimateRunMetricsCost and aggregateRunMetrics, asserting the estimate is aggregated; (2) a Codex-shaped successful RunMetrics test with a seeded same-model cache that pins whether TotalCostUSD must stay 0.

  • [scope-creep] action.yml:449 — The PR title/body frame this change as cost estimation ("estimate USD cost on cancelled runs via historical rate cache"), but this revision also (1) replaces fullsend run with exec fullsend run so GHA signals reach the process directly — affecting every agent invocation through this action, not just cancelled Review runs — and (2) changes ExecStreamReader's default cancellation signal from SIGKILL to SIGINT + a 5s WaitDelay for all sandboxed CLI invocations (Claude, pi, Codex). GoReleaser builds release notes from PR titles (per COMMITS.md), so a signal-handling behavior change hidden inside a cost-estimation title won't surface there, and the risk/moderate label similarly understates the process-lifecycle blast radius (ExecStreamReader is shared across all sandbox executions, not just Review-agent cancellation).
    Remediation: Update the PR title/body to explicitly claim both scopes (cost estimation and graceful cancellation / signal delivery) so release notes and reviewers aren't misled, and re-evaluate the risk label given the shared blast radius. Splitting into two PRs is optional; retitling to reflect the actual scope is the minimum.

  • [stale-doc] docs/guides/infrastructure/distributed-tracing.md:195 — The Cost data contract still opens with "Fullsend does not calculate inference cost from token counts or maintain a model-price table" (lines 194-195) and "They are not inputs to any fullsend-side cost calculation" (line 217); the later "Distinction from backend-derived cost estimates" subsection still states the authoritative cost is always fullsend.cost_usd/total_cost_usd (lines 273-274). All three passages contradict the new "Estimated cost on cancelled runs" subsection, which derives per-token rates from historical cost and multiplies by token counts into those same fields, with no marker distinguishing estimate from runtime-reported cost. (Unchanged since the prior review.)
    Remediation: Revise the opening rule, the token-counts sentence, and the "always authoritative" claim to state: fullsend records the runtime-reported cost as-is when present; when absent, it estimates from token counts using the cached per-model rate — and note there is no field-level marker distinguishing the two.

Low

  • [logic-error] internal/cli/pricing.go:183recordModelRates excludes ReasoningTokens from the rate denominator, but estimateRunMetricsCost's non-PerModelUsage path includes m.ReasoningTokens in the total it multiplies by that rate (the PerModelUsage branch already passes 0). Cancelled Claude TokensEvents typically have reasoning 0, so the advertised cancelled-run path is unaffected today, but this would overstate cost if estimation is ever applied to Codex-shaped metrics (which do populate ReasoningTokens). (Unchanged since the prior review.)
    Remediation: Pass 0 for reasoning in estimateRunMetricsCost's top-level sumTokens call to match recordModelRates and the PerModelUsage estimate path, or include reasoning consistently on both sides.

  • [runtime-mechanism] internal/sandbox/sandbox.go:1384ExecStreamReader now sends SIGINT (via cmd.Cancel) instead of the Go default SIGKILL on context cancellation, with a 5s WaitDelay. Go's os/exec runs this cancellation/kill logic in a background goroutine started from Cmd.Start() (verified against the stdlib source), independent of whether the caller has called Wait() — so the kill does fire even while parseClaudeStream's blocking read is still in progress. The residual gap is narrower: because ExecStreamReader uses StdoutPipe() with no internal io.Copy goroutine, the stdlib's orphaned-pipe-fallback (closeDescriptors, meant for a descendant process that inherited the stdout fd and kept it open — see go.dev/issue/23019) does not run here. If a descendant of openshell's exec target keeps the pipe's write end open after the direct child is SIGKILLed, the blocking read could still hang past the 5s WaitDelay. TestExecStreamReader_SendsSIGINTOnCancel only covers a cooperative child that traps SIGINT and exits promptly — it doesn't cover an unresponsive child or an orphaned descendant holding the pipe open.
    Remediation: Add a test where the child (or a descendant) ignores SIGINT and keeps the stdout write end open, and confirm the read still unblocks within a bounded time — or close the pipe explicitly from the caller on cancellation. Also worth confirming openshell's actual signal-forwarding behavior for the sandboxed exec case.

  • [incorrect-doc] docs/guides/infrastructure/distributed-tracing.md:257 — The new subsection says rates are stored in .fullsend-cache/pricing-rates.json. The code uses filepath.Join(fullsendDir, ".fullsend-cache", "pricing-rates.json"), i.e. .fullsend/.fullsend-cache/pricing-rates.json when --fullsend-dir is .fullsend. The workspace-root .fullsend-cache/ is a different, pre-existing harness fetch cache, so the unqualified path is easy to misread. (Unchanged since the prior review.)
    Remediation: Document the path as <fullsend-dir>/.fullsend-cache/pricing-rates.json (typically .fullsend/.fullsend-cache/pricing-rates.json).


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

Medium

  • [runtime-mechanism] internal/cli/pricing.go:27 — The rate cache is not persisted across GitHub Actions jobs. rateCachePath writes <fullsendDir>/.fullsend-cache/pricing-rates.json; reusable-dispatch.yml sets --fullsend-dir to .fullsend on a fresh checkout each run, so the file lives at .fullsend/.fullsend-cache/pricing-rates.json in an ephemeral workspace. The only actions/cache step in reusable-dispatch.yml restores ${{ runner.temp }}/fullsend-cache (the CLI binary), not this file, and .gitignore already excludes .fullsend-cache/. recordModelRates is a no-op when TotalCostUSD is 0, so a cancelled run can't seed its own cache either. On the standard per-repo dispatch flow — the exact environment issue Persist cost and token telemetry when Review agents are cancelled by GitHub Actions #6936 describes — estimateRunMetricsCost is therefore a no-op on every job unless a prior successful iteration already wrote the file within the same job, and cancelled runs still report $0.00.
    Remediation: Add an actions/cache restore/save (or equivalent) for <fullsend-dir>/.fullsend-cache/pricing-rates.json in the dispatch workflow(s), or state in the cost-contract docs that the estimator is inert on ephemeral GitHub Actions runners and only helps persistent/local workspaces and same-job later iterations.

  • [logic-error] internal/cli/run.go:2278estimateRunMetricsCost is invoked after every iteration with no cancellation check; the only guard is TotalCostUSD > 0 inside pricing.go. Codex always sets TotalCostUSD to 0, including on success. If a prior cost-reporting runtime stored a rate under the same normalized model key (e.g. a pi run recording openai/gpt-4o, later reused by a Codex run on gpt-4o), a successful Codex run gets a fabricated cost, even though both docs/cli/run.md and the updated Cost data contract describe the fallback as cancelled-runs only. The estimate is written in place into TotalCostUSD/PerModelUsage[].CostUSD with no source marker, so consumers cannot tell it apart from a runtime-reported value.
    Remediation: Gate estimateRunMetricsCost to the cancelled/incomplete-stream path so successful zero-cost ResultEvents (Codex) stay at 0. Consider adding a cost-source marker if estimates continue to live in the same telemetry fields.

  • [missing-test] internal/cli/pricing_test.go:82 — The new tests cover isolated record/estimate arithmetic but not the Codex-shaped case (tokens present, cost 0, seeded cache under the same normalized key) and never run cancelled-shaped metrics through the real estimateRunMetricsCostaggregateRunMetrics sequence. The existing #6936 cancellation tests in run_test.go still call aggregateRunMetrics/writeMetricsJSON directly and assert TotalCostUSD == 0, so they don't exercise the new estimator and won't catch a regression.
    Remediation: Add (1) a test that seeds the cache, then runs cancelled-shaped metrics through estimateRunMetricsCost and aggregateRunMetrics, asserting the estimate is aggregated; (2) a Codex-shaped successful RunMetrics test with a seeded same-model cache that pins whether TotalCostUSD must stay 0.

  • [stale-doc] docs/guides/infrastructure/distributed-tracing.md:195 — The Cost data contract still opens with "Fullsend does not calculate inference cost from token counts or maintain a model-price table" (lines 194-195) and "They are not inputs to any fullsend-side cost calculation" (line 217); the later "Distinction from backend-derived cost estimates" subsection still states the authoritative cost is always fullsend.cost_usd/total_cost_usd (line 273). All three passages contradict the new "Estimated cost on cancelled runs" subsection, which derives per-token rates from historical cost and multiplies by token counts into those same fields.
    Remediation: Revise the opening rule, the token-counts sentence, and the "always authoritative" claim to state: fullsend records the runtime-reported cost as-is when present; when absent, it estimates from token counts using the cached per-model rate — and note there is no field-level marker distinguishing the two.

Low

  • [logic-error] internal/cli/pricing.go:183recordModelRates excludes ReasoningTokens from the rate denominator, but estimateRunMetricsCost's non-PerModelUsage path includes m.ReasoningTokens in the total it multiplies by that rate. Cancelled Claude TokensEvents typically have reasoning 0, so the advertised cancelled-run path is unaffected today, but this would overstate cost if estimation is ever applied to Codex-shaped metrics (which do populate ReasoningTokens).
    Remediation: Pass 0 for reasoning in estimateRunMetricsCost's top-level sumTokens call to match recordModelRates and the PerModelUsage estimate path, or include reasoning consistently on both sides.

  • [incorrect-doc] docs/guides/infrastructure/distributed-tracing.md:257 — The new subsection says rates are stored in .fullsend-cache/pricing-rates.json. The code uses filepath.Join(fullsendDir, ".fullsend-cache", "pricing-rates.json"), i.e. .fullsend/.fullsend-cache/pricing-rates.json when --fullsend-dir is .fullsend. The workspace-root .fullsend-cache/ is a different, pre-existing harness fetch cache (ADR 0038), so the unqualified path is easy to misread.
    Remediation: Document the path as <fullsend-dir>/.fullsend-cache/pricing-rates.json (typically .fullsend/.fullsend-cache/pricing-rates.json).


This PR follows up on closed issue #6936, replacing the earlier static-pricing-table approach (flagged in the prior review) with a self-calibrating per-model rate cache — a real improvement in design direction. However, the cache as wired has no persistence mechanism across the ephemeral GitHub Actions runs that motivated the original issue, so the estimator is a no-op in the primary target environment; the estimation call site is still not gated to the cancellation path the docs describe; and both doc files were only partially updated, leaving several passages that contradict the new behavior. These should be addressed before merge.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (4)

Review

Findings

High

  • [backward-incompatible] internal/cli/pricing.go:73estimateRunMetricsCost writes token-derived list-price USD into TotalCostUSD/PerModelUsage.CostUSD whenever the runtime-reported cost is 0 and tokens are present for a known Claude model prefix, then flows that value into all four telemetry sinks (agent span, root span, metrics.json, status comment). This inverts the repository's own "Cost data contract" (docs/guides/infrastructure/distributed-tracing.md, established via docs: define how runtime-reported inference cost is aggregated and traced #6735/docs(#6735): define cost data contract for runtime-reported inference cost #6736 about two weeks before this PR), which explicitly states fullsend does not maintain a model-price table and that a missing runtime cost propagates as $0.00 on all surfaces with no token-based fallback. docs/cli/run.md's total_cost_usd row repeats the same guarantee verbatim ("no fullsend-side pricing-table fallback"). No field or attribute distinguishes an estimated value from a runtime-reported one, so downstream consumers of these documented-as-authoritative fields (dashboards, MLflow reconciliation, other repos' automation) will silently receive blended data.
    Remediation: Either keep total_cost_usd/fullsend.cost_usd strictly runtime-reported and surface the estimate on a distinct, clearly-labeled field (e.g. estimated_cost_usd, a cost_usd_source marker), or get explicit sign-off to revise the Cost data contract and update both doc files to describe the new fallback behavior precisely.

Medium

  • [logic-error] internal/cli/run.go:2274estimateRunMetricsCost is called unconditionally on every iteration before handleRunCancellation, not only on the cancelled/incomplete-stream path the PR title and linked issue describe. The only guard is TotalCostUSD > 0, so any completed iteration whose runtime genuinely reports zero cost (not just a cancelled one) will get a fabricated estimate. The adjacent comment's claim that this "is a no-op" on successful runs is only true incidentally (because such runs usually report non-zero cost), not by design.
    Remediation: Gate the estimation call to the cancellation/incomplete-stream path rather than any zero-cost iteration.

  • [missing-test] internal/cli/pricing_test.go:174 — The 15 new tests cover the pricing-table arithmetic but do not test the actual production wiring: no test exercises a Codex-shaped (tokens present, unrecognized model, cost 0) RunMetrics through estimateRunMetricsCost, and no test exercises the real estimateRunMetricsCostaggregateRunMetrics call order used in run.go. Existing cancellation tests in run_test.go still assert total_cost_usd == 0 without calling the new estimator, so they would not catch a regression in the new wiring.
    Remediation: Add a Codex/unknown-model no-op test at the RunMetrics level and an end-to-end test through the actual run.go call order.

Low

  • [idiomatic-go] internal/cli/pricing.go:20knownModelRates uses unkeyed composite literals for a four-field, all-float64 struct (Input, Output, CacheWrite, CacheRead), making cache-write/cache-read swaps easy to introduce and hard to spot in review.
    Remediation: Key the struct literal fields explicitly.

This PR follows up on closed issue #6936 (already substantially resolved by the separately-merged #6938), using the issue's "add a separate cost-reconciliation mechanism" acceptance-criterion alternative as authorization. However, as implemented, the mechanism is not "separate" — it overwrites the same fields the repository's recently-established Cost data contract designates as authoritative and guarantees have no pricing-table fallback. That contract conflict, combined with the overly-broad (non-cancellation-gated) trigger condition, should be resolved before merge.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

Replace the hardcoded Anthropic pricing table with a self-calibrating
rate cache that derives effective $/token rates from successful runs.
Rates are stored in .fullsend-cache/pricing-rates.json and updated
via exponential moving average. This handles any model (Anthropic,
OpenAI, etc.) without going stale when prices change.

Fixes from review:
- Restrict suffix-stripping to date suffixes and known tags (exp,
  preview, etc.) to prevent false matches like gpt-4o-mini → gpt-4o
- Exclude ReasoningTokens from rate denominator since the deferred
  TokensEvent on cancelled runs does not capture them
- Surface saveRateCache errors via printer.StepWarn instead of
  silently discarding
- Update cost data contract docs and run.md field description

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ascerra ascerra changed the title feat(#6936): estimate USD cost from tokens on cancelled runs feat(#6936): estimate USD cost on cancelled runs via historical rate cache Sep 10, 2026
@ascerra
ascerra marked this pull request as draft September 10, 2026 14:31
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:32 PM UTC · Completed 2:55 PM UTC

Commit: 43f5e6e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.25

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

Layer 1 — action.yml: use `exec fullsend run` so signals go directly to
fullsend instead of bash. Without exec, bash exits on SIGINT, fullsend
becomes an orphan, and GHA kills it before cleanup can flush telemetry.

Layer 2 — ExecStreamReader: send SIGINT (not SIGKILL) to openshell on
context cancellation with a 5s WaitDelay, giving Claude Code time to
flush its result event through the pipe.

Layer 3 — parseClaudeStream: extract token usage from assistant-event
messages (stream-json format) as fallback when stream_event wrappers
are absent. Without this, the deferred TokensEvent on cancelled runs
reports zero tokens.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:20 PM UTC · Completed 4:46 PM UTC

Commit: 1ea267e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.74

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

The prior Cancel function only SIGINTs the openshell CLI, which does
not propagate through gRPC to in-sandbox processes. Claude Code never
receives SIGINT and never flushes the result event with total_cost_usd.

Four changes fix this:

1. Setsid: isolate openshell from GHA's process-group SIGINT so the
   relay stays alive to pipe the result event back.

2. PID file: record the in-sandbox process PID at startup so the
   cancel function can target it directly.

3. Kill-exec: on context cancellation, spawn a separate openshell
   sandbox exec to send kill -INT to the PID inside the sandbox.

4. exec claude: replace the wrapper shell with claude so the PID
   file points to claude's process, not the shell.

Includes diagnostic stderr output at cancel and stream-end points
to verify the mechanism in e2e testing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:15 PM UTC · Ended 6:21 PM UTC

Commit: d708592 · View workflow run →

cmd.Process.Signal(SIGINT) kills openshell immediately, closing the
stdout pipe before the kill-exec goroutine can send SIGINT to claude
inside the sandbox. Claude's result event (with total_cost_usd) has
nowhere to go because the pipe is already dead.

Remove the direct SIGINT to openshell. The kill-exec goroutine is the
sole mechanism: it sends SIGINT to claude via a separate openshell
sandbox exec. Openshell stays alive to relay claude's result event
through the pipe. WaitDelay (8s) is the fallback if claude doesn't
exit.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:23 PM UTC · Completed 6:49 PM UTC

Commit: 7262bad · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.03

…sandbox fix (#6936)

Remove pricing.go, pricing_test.go, and rate-cache wiring in run.go.
Remove assistant-message usage tracking from claude_progress.go (separate concern).
Revert doc changes that described the rate-cache approach.
Update handleRunCancellation docstring to reflect the SIGINT mechanism.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

)

- Fix WaitDelay docstring: "5-second" → "8-second" to match code
- Add empty-PID diagnostic in killScript so logs distinguish
  "pid-file missing" from "kill delivered"
- Document stderrW thread-safety requirement and single-use invariant
- Add comment on exec claude noting PID-file dependency

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ascerra ascerra changed the title feat(#6936): estimate USD cost on cancelled runs via historical rate cache fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd Sep 10, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:52 PM UTC · Ended 7:12 PM UTC

Commit: d419f66 · View workflow run →

@ascerra
ascerra marked this pull request as ready for review September 10, 2026 19:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Propagate cancellation SIGINT to preserve Claude cost telemetry

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Propagates cancellation SIGINT into sandboxes so Claude emits authoritative cost results.
• Keeps OpenShell relays alive briefly to return terminal events before forced shutdown.
• Directs Actions signals to fullsend and verifies graceful cancellation with a regression test.
Diagram

sequenceDiagram
    actor GHA as GitHub Actions
    participant FS as Fullsend CLI
    participant ER as Exec Reader
    participant OR as OpenShell Relay
    participant KE as Kill Exec
    participant CC as Claude Process
    participant MP as Metrics Parser
    FS->>ER: Start sandbox run
    ER->>OR: Launch isolated session
    OR->>CC: exec claude
    GHA->>FS: Send SIGINT
    FS->>ER: Cancel context
    ER->>KE: Start kill command
    KE->>CC: Send SIGINT
    CC-->>OR: Emit result event
    OR-->>MP: Relay NDJSON
    MP-->>FS: Record total cost
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Native OpenShell signal forwarding
  • ➕ Eliminates PID files and the secondary sandbox exec call.
  • ➕ Provides generic signal handling for every sandboxed command.
  • ➖ Requires an upstream OpenShell capability that is currently unavailable.
  • ➖ Delays the fix until compatible OpenShell releases are deployed.
2. Estimate cost from captured tokens
  • ➕ Avoids dependence on graceful process shutdown.
  • ➕ Can provide a fallback when terminal events remain unavailable.
  • ➖ Produces non-authoritative costs that may diverge from provider billing.
  • ➖ Requires continuously maintained or calibrated model pricing data.
  • ➖ Cannot reliably account for every provider-specific pricing rule.
3. In-sandbox process supervisor
  • ➕ Provides a stable lifecycle and signaling endpoint.
  • ➕ Could support multiple concurrent commands without a singleton PID file.
  • ➖ Introduces a persistent control protocol and additional sandbox infrastructure.
  • ➖ Is substantially broader than the telemetry cancellation defect.

Recommendation: Use the PR's PID-file and secondary-exec approach as the pragmatic immediate fix because it preserves Claude's authoritative result event without terminating the stdout relay. Native OpenShell signal forwarding would be the preferred long-term replacement, while token-based estimation is best retained only as a fallback because it cannot guarantee billing accuracy.

Files changed (5) +139 / -12

Bug fix (2) +71 / -6
claude.goMake Claude the PID-tracked process +7/-2

Make Claude the PID-tracked process

• Uses 'exec claude' so the startup shell is replaced and the recorded sandbox PID belongs directly to Claude. It also adds stream completion and process wait diagnostics including parse errors, context state, exit status, and captured cost.

internal/runtime/claude.go

sandbox.goForward cancellation SIGINT through a secondary sandbox exec +64/-4

Forward cancellation SIGINT through a secondary sandbox exec

• Records the in-sandbox process PID, isolates the OpenShell relay in its own session, and overrides command cancellation to send SIGINT through a separate OpenShell invocation. The relay remains alive to return final stdout, with five-second kill-command and eight-second forced-shutdown safeguards.

internal/sandbox/sandbox.go

Tests (1) +58 / -0
sandbox_test.goVerify sandbox cancellation flushes final process output +58/-0

Verify sandbox cancellation flushes final process output

• Adds a Unix regression test using a fake OpenShell executable and a SIGINT-trapping command. The test confirms cancellation reaches the sandbox process, its final output is relayed, and the eight-second wait delay is configured.

internal/sandbox/sandbox_test.go

Documentation (1) +5 / -5
run.goDocument authoritative cancellation cost reporting +5/-5

Document authoritative cancellation cost reporting

• Updates cancellation handling documentation to explain that sandbox SIGINT delivery allows Claude's result event to populate 'TotalCostUSD'. It retains zero cost as the fallback when graceful signal delivery fails.

internal/cli/run.go

Other (1) +5 / -1
action.ymlDeliver workflow cancellation signals directly to fullsend +5/-1

Deliver workflow cancellation signals directly to fullsend

• Replaces the action step's shell process with 'fullsend run' via 'exec'. This prevents an intermediate Bash process from consuming cancellation signals before fullsend can execute its telemetry cleanup path.

action.yml

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 7:14 PM UTC · Ended 7:37 PM UTC

Commit: d419f66 · View workflow run →

Comment on lines +355 to +358
// exec replaces the shell so ExecStreamReader's PID file points to
// claude, not sh — required for the SIGINT-into-sandbox cancel mechanism.
parts := []string{
fmt.Sprintf("cd %s && . %s && claude", params.RepoDir, envFile),
fmt.Sprintf("cd %s && . %s && exec claude", params.RepoDir, envFile),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

3. Signal routing can regress untested 📘 Rule violation ▣ Testability

buildRunCommand now relies on exec claude for correct signal delivery, but no modified Claude
runtime test asserts that the generated command includes the shell replacement. If a later command
refactor removes exec, existing tests still pass while the PID file targets the intermediate shell
instead of the process that must flush final telemetry.
Agent Prompt
## Issue description
The new signal-delivery mechanism depends on `buildRunCommand` producing `exec claude`, but the corresponding runtime tests do not assert this behavior.

## Fix Focus Areas
- internal/runtime/claude_test.go[217-223]

## Recommended Fix
Update `TestBuildRunCommand_Basic` to assert that the generated command contains `exec claude`, ensuring removal of the required shell replacement causes a test failure.

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

Comment on lines +355 to +358
// exec replaces the shell so ExecStreamReader's PID file points to
// claude, not sh — required for the SIGINT-into-sandbox cancel mechanism.
parts := []string{
fmt.Sprintf("cd %s && . %s && claude", params.RepoDir, envFile),
fmt.Sprintf("cd %s && . %s && exec claude", params.RepoDir, envFile),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

4. Runtime change skips guide review 📘 Rule violation ⛨ Security

ClaudeRuntime.Run and buildRunCommand change an existing runtime backend's execution behavior,
but the PR description does not state that docs/contributing/runtime-implementation.md was
consulted. Because direct process execution participates in the documented egress-binary contract,
reviewers cannot verify from the change record that the runtime checklist was evaluated.
Agent Prompt
## Issue description
The Claude runtime backend now directly replaces its shell process, but the PR does not record consultation of the runtime implementation guide as required for backend behavior changes.

## Fix Focus Areas
- internal/runtime/claude.go[355-358]

## Recommended Fix
Update the PR description to explicitly state that `docs/contributing/runtime-implementation.md` was consulted and that the existing Claude egress-binary mapping, wire protocol, and workspace layout remain unchanged.

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

// in-sandbox process. Callers that use "exec" in their command (e.g.
// buildRunCommand) replace the shell, so the PID file points to the
// final process.
wrapped := fmt.Sprintf("echo $$ > %s; %s", execPIDFile, command)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Three agents miss cancellation signals 🔗 Cross-repo conflict ☼ Reliability

ExecStreamReader records the wrapper shell's $$ and later sends SIGINT only to that PID, but
buildPiRunCommand launches Pi as a child without exec and the Codex builder uses a pipeline
rather than replacing the shell as Claude does. Cancellation of Codex or the agents repository's
Pi-based code, prioritize, and review agents can therefore leave the real agent running without
flushing terminal output until the relay fallback timeout, WaitDelay, or sandbox teardown.
Agent Prompt
## Issue description
`ExecStreamReader` assumes the PID written from `$$` becomes the agent PID, but that is only true when the supplied command replaces the wrapper shell. Codex uses a pipeline and Pi launches without `exec`, so cancellation targets `sh` rather than the actual agent process.

## Fix Focus Areas
- internal/sandbox/sandbox.go[1390-1440]
- internal/runtime/codex_run.go[365-370]
- internal/runtime/pi_run.go[497-505]
- internal/sandbox/sandbox_test.go[306-363]

## Recommended Fix
Change the launch protocol so the PID file reliably identifies the actual agent process or its dedicated process group for every `ExecStreamReader` caller. Launch Pi with `exec` after completing its setup and guard commands, update Codex command construction as needed, and add cancellation tests covering all three runtime command shapes to verify that each agent receives SIGINT, exits gracefully, and flushes its final output rather than relying on `WaitDelay`.

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

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit d419f66

@waynesun09

Copy link
Copy Markdown
Member

This bypasses OpenShell’s supported stop lifecycle: sandbox exec does not reliably signal the agent process tree, and Setsid can orphan the relay; please use sandbox stop or add runtime-specific integration tests proving safe cancellation.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

"PID=$(cat %s 2>/dev/null); "+
"if [ -z \"$PID\" ]; then echo \"sandbox-cancel: pid-file missing or empty\" >&2; exit 1; fi; "+
"echo \"sandbox-cancel: pid=$PID\" >&2; "+
"kill -INT $PID 2>&1; echo \"sandbox-cancel: kill=$?\" >&2",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] runtime-mechanism

ExecStreamReader records echo $$ > /tmp/.fullsend_exec.pid and on cancel sends SIGINT only to that PID (kill -INT $PID). Claude's exec claude replaces the wrapping shell, so $$ is the runtime and SIGINT is delivered correctly. Codex still ends in a pipeline (printf ... | "$codex" exec in internal/runtime/codex_run.go) — a shell cannot last-exec a pipeline, so $$ is the wrapper shell and kill -INT $PID never reaches the Codex process. Pi launches "$FULLSEND_PI_BIN" --print without exec (internal/runtime/pi_run.go); it may happen to inherit $$ via shell last-command-exec, but that is implicit and untested. Both non-Claude callers also now pay the new 8s WaitDelay on every cancel/timeout before the openshell relay is SIGKILLed. TestExecStreamReader_SendsSIGINTOnCancel only covers a cooperative same-PID child, so this gap is untested. This mechanism is squarely this PR's stated purpose, and as implemented it verifiably works for Claude but not for two of the three supported runtimes.

Suggested fix: Either exec the payload in every ExecStreamReader caller (so $$ is always the runtime process, matching the Claude pattern) or deliver SIGINT to the recorded PID's process group (kill -INT -- -$PID) so wrapper-shell children receive it. Extend the cancellation test to a wrapping shell with a non-exec'd child and a pipeline child.

// (SIGINT from GHA cancellation) don't kill it before cmd.Cancel
// has a chance to send SIGINT into the sandbox and relay the
// result event through stdout.
cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] permission-expansion

Setsid: true moves the openshell relay into a new session, and cmd.Cancel returns nil without ever signaling that host process directly — the only host-side kill is cmd.WaitDelay = 8s, with no Pdeathsig. If the parent fullsend process is SIGKILLed before WaitDelay fires, the relay can be reparented to init and keep running. An 8s WaitDelay also sits at or past GitHub Actions' own ~7.5s SIGINT-to-SIGKILL step window, so the intended force-kill of the relay may never run on a cancelled GHA job. Blast radius is limited to an already-sandboxed relay process (bounded by the existing --timeout, and torn down with the GHA VM), so this is a lifecycle-hardening gap rather than a genuine privilege expansion.

Suggested fix: If Setsid is required, also set SysProcAttr.Pdeathsig = syscall.SIGKILL (Linux) so a killed parent cannot leave the relay orphaned. Shrink WaitDelay (or the internal 5s kill-exec timeout) so cleanup still fits inside GitHub Actions' cancellation budget.

"PID=$(cat %s 2>/dev/null); "+
"if [ -z \"$PID\" ]; then echo \"sandbox-cancel: pid-file missing or empty\" >&2; exit 1; fi; "+
"echo \"sandbox-cancel: pid=$PID\" >&2; "+
"kill -INT $PID 2>&1; echo \"sandbox-cancel: kill=$?\" >&2",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] fail-open

ExecStreamReader's cancel script still treats the in-sandbox PID file as the sole SIGINT target. The new [ -z "$PID" ] check (added since the prior review) closes the absent/empty-file case, but the script still doesn't validate the content is a bare positive integer, and kill -INT $PID remains unquoted — a malformed PID file could still pass multiple IFS-split operands or a negative value. Blast radius stays inside the sandbox, and a bad PID still degrades to the WaitDelay fallback rather than granting host access, so this is a partial fix of the prior finding rather than a new issue.

Suggested fix: Reject non-numeric PID file contents (e.g. case "$PID" in ''|[!0-9]) exit 1;; esac) before signaling, and use kill -INT -- "$PID".


if parseErr := parseClaudeStream(r, handler); parseErr != nil {
parseErr := parseClaudeStream(r, handler)
fmt.Fprintf(os.Stderr, " stream ended: parseErr=%v ctxErr=%v cost=%.4f\n", parseErr, ctx.Err(), metrics.TotalCostUSD)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] logging-conventions

fmt.Fprintf(os.Stderr, " stream ended: parseErr=%v ctxErr=%v cost=%.4f\n", ...) (line 181) and fmt.Fprintf(os.Stderr, " wait done: exitCode=%d waitErr=%v\n", ...) (line 193) execute unconditionally on every ClaudeRuntime.Run, including normal successful runs, adding stderr noise beyond the existing error-only " progress parser: %v\n" pattern. parseErr is also printed with a raw %v here (unsanitized) before being sanitized again on the error-handling path a few lines later.

Suggested fix: Remove the unconditional diagnostic prints, or gate them behind an explicit debug/verbose flag if they are meant to persist as permanent diagnostics.

}
assert.Contains(t, string(all), "flushed-after-sigint",
"in-sandbox process should receive SIGINT via the kill-exec mechanism and flush output")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-idioms

In TestExecStreamReader_SendsSIGINTOnCancel, waitErr := cmd.Wait() assigns the error to a named variable that is immediately discarded via _ = waitErr — dead code / unnecessary indirection that also misses an opportunity to assert the wait outcome.

Suggested fix: Replace with _ = cmd.Wait() // wait for subprocess cleanup after cancellation, or assert the expected outcome.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:14 PM UTC · Completed 7:37 PM UTC

Commit: d419f66 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.63

@ascerra
ascerra marked this pull request as draft September 10, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants