Skip to content

feat(#6464): add the pi runtime (stream parser, Bootstrap/Run, Vertex provider, enablement) - #6467

Merged
waynesun09 merged 59 commits into
mainfrom
agent/6464-pi-runtime-stub
Aug 22, 2026
Merged

feat(#6464): add the pi runtime (stream parser, Bootstrap/Run, Vertex provider, enablement)#6467
waynesun09 merged 59 commits into
mainfrom
agent/6464-pi-runtime-stub

Conversation

@fullsend-ai-coder

@fullsend-ai-coder fullsend-ai-coder Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds pi (earendil-works/pi 0.84.2) as a fullsend agent runtime, following the OpenCode precedent (#6035/#6147) but landing plan steps 2–5 together: the runtime is registered and functional — Bootstrap, Run, transcript handling and the sandbox-hook adapter — and is user-selectable with runtime: pi (org defaults, per-repo config, fullsend admin install --runtime pi). No fleet lifecycle run on Vertex has been recorded yet; docs/runtimes.md lists exactly what is not yet exercised and recommends piloting triage/prioritize on a disposable org first. Includes #6466 (merged in) and supersedes it, and merges in #6468 (fix(#6357): PostToolUse sanitizers honour Claude Code's hook contract — tool_response in, hookSpecificOutput.updatedToolOutput out, one posttool_chain.py driver) so both runtimes land on the same v2 hook contract together.

Security posture is evaluated control-by-control against the Claude Code runtime in docs/runtimes.md; pi is equal on every control (with #6468 merged, PostToolUse sanitizers run through the same posttool_chain.py on both — and, because the v2 contract makes them effective under Claude Code for the first time, their heuristics were scoped in 63a0a7d6/02fcaa3a so they no longer rewrite ordinary source, condense compound commands that failed, or NFKC-rewrite non-ASCII file content; every rewrite now tells the agent via additionalContext, PostToolUseFailure runs canary detection, hooks carry timeout: 30) and stricter on three (failed tool calls are sanitized too, repo-owned runtime config never loads, an unreadable hook manifest blocks every tool call). Credentials use the same WIF external_account + runner-refreshed OIDC token path as Claude Code on Vertex, under the same egress allowlist; for the Vertex provider Run unsets ANTHROPIC_API_KEY/ANTHROPIC_AUTH_TOKEN/ANTHROPIC_BASE_URL/ANTHROPIC_VERTEX_BASE_URL after sourcing .env and pins GOOGLE_CLOUD_PROJECT to ANTHROPIC_VERTEX_PROJECT_ID, so pi cannot be routed past Vertex or at a different project. Whether the hook adapter is loaded is decided from the runner's security signal (not the agent-writable manifest); Run refuses to start when security is enabled but the manifest carries no hook plan, and the run command fails closed (exit 97) if the adapter or manifest is missing or the adapter's SHA-256 differs from the embedded copy — checked before the agent-writable .env is sourced, via command -p sha256sum, because pi silently skips a missing -e path.

Changes

  • PiRuntime (internal/runtime/pi.go, pi_bootstrap.go, pi_run.go, pi_transcript.go, pi_agent.go)
    • ConfigDir() = /sandbox/pi-config (sandbox.SandboxPiConfig, outside the cloned repo tree); EnvExports pins PI_CODING_AGENT_DIR / PI_CODING_AGENT_SESSION_DIR and sets PI_OFFLINE=1, PI_SKIP_VERSION_CHECK=1, PI_TELEMETRY=0.
    • Bootstrap parses the Claude-style agent .md (frontmatter name/description/model/tools, body) and writes APPEND_SYSTEM.md, a locked-down settings.json (defaultProjectTrust: never), skills, the hook scripts + fullsend-hooks.js when security is enabled, and fullsend-manifest.json (pi tool names for --tools, Bash(a,b) allowlist, HookPlan, pi version from a pi --version preflight). Plugins are skipped with a warning.
    • Run executes pi --print --mode json --no-approve --no-extensions --no-prompt-templates --no-themes --session-dir … -e /opt/pi-extensions/anthropic-vertex [-e fullsend-hooks.js] [--tools …] --model <provider/id> [--thinking <effort>] 'Run the agent task', streams through parsePiStream, emits InitEvent with the pi version and bare model id, tees output.jsonl, and returns 1 when pi exited 0 on a stream error. Model: opus|sonnet|haiku → pi catalog ids (claude-opus-4-6, claude-sonnet-4-6, claude-haiku-4-5), provider prefix anthropic-vertex, FULLSEND_PI_MODEL/FULLSEND_PI_PROVIDER overrides; harness model: wins over frontmatter. --debug sends pi's stderr to pi-debug.log.
    • Transcripts: session JSONL extraction; ParseTranscriptFile judges --mode json captures by their ResultEvent and session files by the last assistant stopReason.
  • parsePiStream (pi_progress.go) — NDJSON → AgentEvents, verified against pi v0.84.2 sources. Tool summaries come from call arguments (redacted before any cap); agent_end{willRetry:false} is not terminal (compaction / queued follow-ups continue; agent_settled ends the prompt; one prompt per positional message), so exactly one ResultEvent is emitted at stream end, failing closed on incomplete/mid-compaction/lost streams; auto_retry_startRetryEvent; length is not an error.
  • Hook adapter (internal/runtime/pi_extension/fullsend-hooks.js, embedded) — runs the runtime-neutral hook scripts (ADR 0090) from pi's tool_call/tool_result with Claude tool names/inputs; PreToolUse groups in HookPlan order, block on exit ≠ 0 or decision:block, fail closed on spawn failure; PostToolUse chained sequentially accepting v1 tool_result and v2 updatedToolOutput (PostToolUse sandbox hooks read tool_result but Claude Code sends tool_response — sanitizers are inert under Claude Code #6357/fix(#6357): make PostToolUse sanitizers honor Claude Code's hook contract #6468) replies. Bash(a,b) is advisory by default (Claude Code parity, ADR 0027), FULLSEND_PI_BASH_ALLOWLIST=enforce blocks (first-token check on every ;/|/&&/||/&-separated command — fd redirections such as 2>&1 are not separators — refusing substitution, subshells, binary paths, every VAR= prefix and eval/exec/sh/command/env/xargs-style wrappers). Agents that list Skill or ship skills get pi's read tool, which pi's prompt-driven skills require. node --test suite wired into make script-test.
  • Sandbox imageARG PI_VERSION=0.84.2 (npm install -g --ignore-scripts), image-level PI_* ENV defaults (test-guarded against SandboxPiConfig), and the vendored twoGiants/pi-anthropic-vertex v0.1.13 (commit d3c9d10d, tag + tarball SHA256, npm ci --omit=dev --omit=peer --ignore-scripts, read-only under /opt/pi-extensions) as the interim Claude-on-Vertex provider until upstream feat(ai): add Anthropic Vertex provider earendil-works/pi#5262 ships. Renovate tracks both pins (no automerge).
  • Enablementconfig.ValidRuntimes() += pi (config validation, --runtime help, CLI/layered-config references); skills/analyze-transcript normalizes pi session files (toolCalltool_use, toolResulttool_result, camelCase usage/stop reason) so every subcommand works on <agent>-<timestamp>_<id>.jsonl.
  • Registry/docscase "pi" in Resolve(); docs/runtimes.md pi column (no longer a stub), switching quickstart, a runtime-agnostic run sequence diagram, a trust-zones diagram ahead of the security matrix, config-key rows, sandbox layout, "Pi-specific known constraints" with an at-a-glance table and the pi iteration flowchart (all pi claims cited to v0.84.2); docs/architecture.md "one contract, two runtimes" and claude-vs-pi sandbox diagrams (mermaid, rendered by the site's component in both themes); the site gains click-to-enlarge for every mermaid diagram, every markdown table and long or wide code blocks (shared EnlargeDialog.vue with the theme's styles applied inside; diagrams at natural size with a fit toggle, tables/code reflowed to the full viewport; keyboard-reachable pill, Escape/backdrop close, closes on navigation) after Playwright measurements showed wide diagrams scaled to 0.3 and the security matrix scroll-boxed in the content column, and the runtime diagrams are drawn top-to-bottom; glossary/roadmap mentions; landing page (web/public/index.html) gains a "Multi-runtime / Claude Code · pi" hero stat, and the hero chip/subtitle name GitHub and GitLab (the rest of the page stays GitHub-only until GitLab is fully rolled out); cli-internals.md, images/README.md, topology docs.

Testing

  • go build ./..., go vet, full go test ./... pass; 90+ pi tests (parser, agent parsing, Bootstrap/Run against a fake openshell, transcripts, image-ENV guard)
  • node --test internal/runtime/pi_extension/ — 15 tests (incl. a real python3 hook script); the hook guard fragment exercised under real sh and Debian dash, including function/PATH shadowing of sha256sum
  • pre-commit on all changed files; local podman build of the Vertex extension layer
  • Multi-agent review (Claude + Grok): three parser fix/verify rounds, a full-PR review-squad pass after merging refactor(runtime): register pi as a stub runtime #6466, a re-review of the fix commits and of the Vertex vendoring, and four Claude + Grok fix/verify rounds on the Bootstrap/Run/hook-adapter delta (hash-pinned adapter guard, allowlist separators and env prefixes, frontmatter fence strictness, guard ordering before .env)
  • Effort parity: pi's built-in default thinking level is medium (core/defaults.js) while Claude Code runs at high on Vertex/API-key and the fleet agents set no effort:, so Run now always passes --thinking — the harness effort when it is a pi level, high otherwise (4619aa3e; pi maps the level onto Anthropic adaptive effort and clamps it for non-reasoning models)
  • analyze-transcript: 16 unit tests (make script-test) on pi session files and a Claude-shape transcript; ruff/ty/bandit clean
  • Unattended operation verified against pi v0.84.2 source and on the pinned build in a container: no tool-approval layer, no-op extension UI in print mode, --no-approve keeps a planted .pi/extensions/evil.js unloaded, missing credentials exit 1; the one blocker (print mode reads an open stdin to EOF) is closed with </dev/null in the run command
  • Behaviour suite (per-repo): the triage scenario now asserts the runtime selected from the repo config (metrics.json runtime), and features/runtime/pi.feature runs a minimal tool-using agent on haiku under runtime: pi asserting the runtime, a toolCall in the pi session transcript and token usage — gated on BEHAVIOUR_CAPABILITIES=runtime-pi until fullsend-sandbox:latest carries PI_VERSION; no change to the deprecated org-mode e2e/admin suite
  • Double scrutiny of the Claude Code runtime with the v2 PostToolUse contract (fix(#6357): make PostToolUse sanitizers honor Claude Code's hook contract #6468): the contract verified against the official hooks docs and Claude Code 2.1.234/2.1.240 (tool_response in, hookSpecificOutput.updatedToolOutput out, shape-preserving); the first bot-review run on the merge head shows the v2 path live ([REDACTED PRIVATE KEY] in the review transcripts). An adversarial review then showed the now-effective heuristics rewriting ordinary output — reproduced (Read rewrote 105 of 900 fullsend files; pytest; go test with 3 failures condensed to "passed"; CJK content NFKC-rewritten) and fixed in 63a0a7d6, then hardened again in 02fcaa3a after a second Claude + Grok round (keyword-argument calls, id_token/system-qualified names, human passwords as literals, .txt/your_/dotted evasions, quoted | and comment/continuation command shapes, idempotent db-URL masking, detection copy immune to combining-mark/selector interleaving, key-agnostic PostToolUseFailure scan). a third verify-only round (3316308f) then closed what the exemptions had opened (real glpat-/glrt-/ya29./ASIA tokens get their own prefix patterns, the known-prefix exemption covers word-shaped fakes only, bare-identifier keyword arguments, AWS pagination tokens, an over-escaped quoted-region regex). A Grok round on the five follow-up commits then found the failure path blind to Cf-split canaries (zero-width, bidi, tag characters) that the success path caught, plus four smaller gaps; e20d933b folds format characters into the detection copy, schedules the failure phase whenever the chain has work there (not only when the canary is on), scans the detection copy for credentials on failures, teaches suppression about wrapped invocations (sudo/timeout/env/mise exec --), and stops pagination words vetoing NEXT_SECRET. 56254034 before it closed the failed-call gap as far as Claude Code's contract allows — PostToolUseFailure accepts only additionalContext, so the driver halts on a canary there and otherwise detects, logs (masked) and warns about credentials and control characters it cannot strip. Note the suppressors only ever see zero-exit output under Claude Code (a non-zero exit fires PostToolUseFailure instead), while pi's tool_result does deliver failed calls to the same chain. e7881dff before it adopted the invariant a parallel audit of the live hooks surfaced — condense only on positive evidence a tool printed, never from silence (a no-output pre-commit run had been reported as passed; Claude Code's Bash result carries no exit code to tell a clean run from a hook whose interpreter is missing) — and anchors the command matchers so a command that merely mentions a tool (grep -n scan-secrets …) keeps its output. 215 hook tests (was 160); the 900-file Read sweep went 105 → 14, all of them test files or doc examples holding real-shaped tokens/private keys that should be masked
  • Fleet lifecycle run on Vertex (post-merge pilot, tracked on Track pi (earendil-works/pi) as a supported agent runtime #6464): confirms the bare model ids and copied compat flags on Vertex, the fleet prompts under pi's system-prompt preamble, and records real fixtures via regen.sh

Refs #6464. Supersedes #6466. Includes #6468closes #6357.

Post-script verification

  • Branch is not main/master (agent/6464-pi-runtime-stub)
  • Secret scan passed (gitleaks)
  • PR body secret scan passed (gitleaks — no-git)

Register PiRuntime (earendil-works/pi, CLI `pi`) in runtime.Resolve()
following the OpenCode stub precedent (#6035): resolvable internally for
dev/testing, deliberately NOT added to config.ValidRuntimes() — a selectable
stub would burn pre-script side effects and a sandbox before failing — so it
is not user-selectable via `fullsend admin install --runtime` until the
runtime is functional.

- internal/runtime/pi.go: Name "pi", System "pi" (multi-provider, OpenCode
  precedent), ConfigDir /sandbox/pi-config (new sandbox.SandboxPiConfig,
  outside the agent-writable workspace), EnvExports pinning
  PI_CODING_AGENT_DIR / PI_CODING_AGENT_SESSION_DIR to runner-owned paths
  plus PI_OFFLINE=1 and PI_SKIP_VERSION_CHECK=1; Bootstrap/Run and
  transcript extraction return explicit not-implemented errors (#6464).
- images/sandbox/Containerfile: ARG PI_VERSION=0.84.2, npm install
  --ignore-scripts, with a renovate customManagers regex tracking the pin;
  the install comment records that pin bumps must re-verify the upcoming
  stream-parser fixtures (pi changed its --mode json shape within 0.84).
- docs/runtimes.md: registered-runtimes row, pi (stub) column in the
  security feature matrix and the config-key support matrix.
- Tests: pi_test.go (metadata, env exports, not-implemented, no-ops,
  capability defaults — no CLAUDE.md bridge, default debug-log name);
  registry and config tests extended with the resolvable-but-not-selectable
  cases.

Next steps tracked in #6464: stream parser with recorded fixtures,
Bootstrap/Run with the ADR 0090 hook-adapter extension, transcript
extraction, then ValidRuntimes() enable.

Refs #6464

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 21, 2026 21:33
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:35 PM UTC · Completed 9:50 PM UTC

Commit: 1fd2d7f · View workflow run →

…ract

Read tool_response (fallback tool_result) and replace output via
hookSpecificOutput.updatedToolOutput, with suppress → unicode → redact
enforced in a single posttool_chain.py driver because Claude runs hooks
in parallel. Canary post-tool also redacts leaked tokens, since
decision:block does not hide the original result.

Signed-off-by: Wayne Sun <gsun@redhat.com>
Claude Code Bash payloads always have stdout, so first-key scan_text
missed stderr-only canary leaks. Two PostToolUse hooks also raced on
updatedToolOutput; the chain now owns suppress → unicode → redact →
canary in one process.

Assisted-by: Grok (fix), Claude (review), Gemini (review), Codex (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
FULLSEND_POSTTOOL_SKIP was a test-only env knob the agent could write
into workspace .env and disable the chain. Stages are gated by sibling
files only. Also type the chain metadata map and add Bash-object tests
for unicode and suppress.

Assisted-by: Grok (fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile, skills/analyze-transcript/SKILL.md, skills/analyze-transcript/analyze-transcript.py, skills/analyze-transcript/analyze_transcript_test.py — This PR modifies governance and infrastructure files under protected paths (.github/, images/, skills/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding Node.js setup for the hook extension test suite, adding the pi CLI and Vertex extension to the sandbox image, updating the analyze-transcript skill for pi session normalization). Human approval is always required for protected-path changes, regardless of context.

  • [scope-creep] web/public/index.html — The landing page changes include modifying "GitHub-native" to "GitHub & GitLab" in the hero chip and subtitle. These GitLab mentions are unrelated to the pi runtime feature authorized by issue Track pi (earendil-works/pi) as a supported agent runtime #6464. The PR body acknowledges these changes but does not cite a separate authorizing issue. The "Multi-runtime / Claude Code \u00b7 pi" stat and "agent runtime" mention are in-scope.

Low

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) has no SHA-256 integrity check in piHooksGuard, unlike the hook extension file. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

  • [supply-chain] images/sandbox/Containerfile — The pi-anthropic-vertex extension (v0.1.13) is installed with npm ci from its own vendored package-lock.json. Unlike the top-level project deps managed by renovate, this lockfile is not renovate-managed — advisories in its transitive dependencies surface only when the tag itself is bumped. Mitigated by SHA-256 tarball verification, --ignore-scripts, --omit=dev --omit=peer, and read-only filesystem (chmod a-w).

  • [stale-doc] docs/guides/user/customizing-agents.md:16, docs/guides/user/bring-your-own-agent.md:214, docs/guides/user/building-custom-agents.md:135 — The comment (claude runtime only) on the effort harness key is stale. Pi now supports effort via --thinking, as documented in the updated runtimes.md config key table.

Previous run

Review

Findings

Medium

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile, skills/analyze-transcript/SKILL.md, skills/analyze-transcript/analyze-transcript.py, skills/analyze-transcript/analyze_transcript_test.py — This PR modifies governance and infrastructure files under protected paths (.github/, images/, skills/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding Node.js setup for the hook extension test suite, adding the pi CLI and Vertex extension to the sandbox image, updating the analyze-transcript skill for pi session normalization). Human approval is always required for protected-path changes, regardless of context.

  • [naming-consistency] internal/runtime/pi_run.go:194buildPiRunCommand hardcodes "Run the agent task" instead of using params.Prompt with a fallback to DefaultAgentPrompt. The RunParams.Prompt doc comment (runtime.go:47–55) states "Every Runtime implementation MUST honour this field, falling back to DefaultAgentPrompt when it is empty." ClaudeRuntime.buildRunCommand correctly implements this at claude.go:336–339. The pi runtime ignoring params.Prompt means the validation loop's feedback_mode will silently produce blind retries instead of injecting failure context — indistinguishable from the behavior this field exists to remove (Implement validation loop feedback so agents can self-correct #1050, Run pre-commit inside the code/fix validation loop so a lint block becomes a retry, not a dead end #6494).

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) and hook scripts remain agent-writable between iterations. The adapter's SHA-256 is verified via piHooksGuard (using command -p sha256sum to bypass PATH manipulation), but the manifest itself — which controls bashAllowlistMode, toolNames, and script lists — has no integrity check. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

  • [supply-chain] images/sandbox/Containerfile — The pi-anthropic-vertex extension (v0.1.13) is installed with npm ci from its own vendored package-lock.json. Unlike the top-level project deps managed by renovate, this lockfile is explicitly documented as NOT renovate-managed — advisories in its 29 transitive dependencies surface only when the tag itself is bumped. The images/README.md documents this gap, and renovate.json disables automerge so bumps require manual review and checksum refresh. Between tag bumps there is no automated vulnerability scanning. The --ignore-scripts and --omit=dev --omit=peer flags appropriately reduce the attack surface.

  • [scope-creep] web/public/index.html — The landing page changes include adding a "Multi-runtime / Claude Code · pi" hero stat (related to pi) and changing "GitHub-native" to "GitHub & GitLab" in the hero chip and subtitle. The GitLab mention is unrelated to the pi runtime and is not authorized by issue Track pi (earendil-works/pi) as a supported agent runtime #6464.

Low

  • [edge-case] internal/runtime/pi_extension/fullsend-hooks.js:81 — The SEPARATORS regex for Bash allowlist command splitting uses lookbehind/lookahead to distinguish backgrounding & from fd redirections. Edge cases in complex shell syntax could theoretically bypass the first-token check. The allowlist defaults to advisory mode; enforce mode is opt-in.

  • [stale-doc] docs/guides/user/customizing-agents.md:16, docs/guides/user/bring-your-own-agent.md:214, docs/guides/user/building-custom-agents.md:135 — The comment (claude runtime only) on the effort harness key is stale. Pi now supports effort via --thinking, as documented in the updated runtimes.md config key table.


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

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile, skills/analyze-transcript/SKILL.md, skills/analyze-transcript/analyze-transcript.py, skills/analyze-transcript/analyze_transcript_test.py — This PR modifies governance and infrastructure files under protected paths (.github/, images/, skills/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding Node.js setup for the hook extension test suite, adding the pi CLI and Vertex extension to the sandbox image, updating the analyze-transcript skill for pi session normalization). Human approval is always required for protected-path changes, regardless of context.

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) and hook scripts remain agent-writable between iterations. The adapter's SHA-256 is verified via piHooksGuard (using command -p sha256sum to bypass PATH manipulation), but the manifest itself — which controls bashAllowlistMode, toolNames, and script lists — has no integrity check. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

  • [scope-creep] web/public/index.html — The landing page changes include adding a "Multi-runtime / Claude Code · pi" hero stat (related to pi) and changing "GitHub-native" to "GitHub & GitLab" in the hero chip and subtitle. The GitLab mention is unrelated to the pi runtime and is not authorized by issue Track pi (earendil-works/pi) as a supported agent runtime #6464.

Low

  • [inconsistent-documentation] internal/security/hooks.go:141 — The HookPlan comment says "suppress → unicode → redact → canary must share one process" but posttool_chain.py executes in the order: unicode → canary → suppress → redact. The comment's intent is to justify why stages must share one process, not prescribe order, but the listed order is misleading.

  • [naming-consistency] internal/runtime/pi.go:36PiVertexExtensionPath is exported but only used within the runtime package (in pi_run.go). All other pi-specific constants (piManifestFile, piHooksExtensionFile, etc.) are unexported. Minor API surface hygiene.

  • [naming-consistency] internal/runtime/pi_bootstrap.go:29piDebugLog breaks the naming pattern of its constant group (piManifestFile, piHooksExtensionFile, piAppendSystemFile, piSettingsFile all end in File).

  • [stale-annotation] docs/guides/user/customizing-agents.md:16, docs/guides/user/bring-your-own-agent.md:214, docs/guides/user/building-custom-agents.md:135 — The comment (claude runtime only) on the effort harness key is stale. Pi now supports effort via --thinking, as documented in the updated runtimes.md config key table.

  • [scope-creep] docs/.vitepress/theme/components/EnlargeDialog.vue — The PR adds a general-purpose click-to-enlarge UI framework for the documentation site. While motivated by the pi runtime diagrams being unreadable at narrow widths, this is a site-wide UX feature not scoped to pi and not in issue Track pi (earendil-works/pi) as a supported agent runtime #6464's acceptance criteria. Consider filing a separate tracking issue.

  • [edge-case] internal/runtime/pi_extension/fullsend-hooks.js:60 — The SEPARATORS regex for Bash allowlist command splitting uses lookbehind/lookahead to distinguish backgrounding & from fd redirections. Edge cases in complex shell syntax could theoretically bypass the first-token check. The allowlist defaults to advisory mode; enforce mode is opt-in.

Previous run (3)

Review

Findings

Medium

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile, skills/analyze-transcript/SKILL.md, skills/analyze-transcript/analyze-transcript.py, skills/analyze-transcript/analyze_transcript_test.py — This PR modifies governance and infrastructure files under protected paths (.github/, images/, skills/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding the pi CLI and Vertex extension to the sandbox image, adding Node.js setup for the hook extension test suite, updating the analyze-transcript skill for pi session normalization). Human approval is always required for protected-path changes, regardless of context.

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) and hook scripts remain agent-writable between iterations. The adapter's SHA-256 is verified via piHooksGuard (using command -p sha256sum to bypass PATH manipulation), but the manifest itself — which controls bashAllowlistMode, toolNames, and script lists — has no integrity check. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

  • [scope-creep] web/public/index.html — The landing page changes include adding a "Multi-runtime / Claude Code \u00b7 pi" hero stat (related to pi) and changing "GitHub-native" to "GitHub & GitLab" in the hero chip and subtitle. The GitLab mention is unrelated to the pi runtime and is not authorized by issue Track pi (earendil-works/pi) as a supported agent runtime #6464.

Low

  • [credential-hygiene] internal/runtime/pi_run.go:130 — The ANTHROPIC_* credential clearing uses shell unset after sourcing .env. Since .env is agent-writable, an agent could write ANTHROPIC_API_KEY back into .env to have it re-set after the unset. Mitigated by the sandbox using WIF/OIDC with no real API key available — there is no credential to leak.

  • [edge-case] internal/runtime/pi_extension/fullsend-hooks.js:60 — The SEPARATORS regex for Bash allowlist command splitting uses lookbehind/lookahead to distinguish backgrounding & from fd redirections. Edge cases in complex shell syntax could theoretically bypass the first-token check. The allowlist defaults to advisory mode; enforce mode is opt-in.

  • [scope-creep] docs/.vitepress/theme/components/EnlargeDialog.vue — The PR adds a general-purpose click-to-enlarge UI framework for the documentation site. While motivated by the pi runtime diagrams being unreadable at narrow widths, this is a site-wide UX feature not scoped to pi and not in issue Track pi (earendil-works/pi) as a supported agent runtime #6464's acceptance criteria. Consider filing a separate tracking issue.

Previous run (4)

Review

Findings

Medium

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile, skills/analyze-transcript/SKILL.md, skills/analyze-transcript/analyze-transcript.py, skills/analyze-transcript/analyze_transcript_test.py — This PR modifies governance and infrastructure files under protected paths (.github/, images/, skills/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding the pi CLI and Vertex extension to the sandbox image, adding Node.js setup for the hook extension test suite, updating the analyze-transcript skill for pi session normalization). Human approval is always required for protected-path changes, regardless of context.

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) and hook scripts remain agent-writable between iterations. The adapter's SHA-256 is verified via piHooksGuard (using command -p sha256sum to bypass PATH manipulation), but the manifest itself — which controls bashAllowlistMode, toolNames, and script lists — has no integrity check. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

  • [scope-deviation] internal/config/config.go:194 — Issue Track pi (earendil-works/pi) as a supported agent runtime #6464 acceptance criterion 8 states "ValidRuntimes() enabled + docs finalized only after the lifecycle test passes." This PR adds pi to ValidRuntimes(), making it user-selectable, but the PR body acknowledges "No fleet lifecycle run on Vertex has been recorded yet." The e2e scenario (features/runtime/pi.feature) is gated on BEHAVIOUR_CAPABILITIES=runtime-pi until the sandbox image ships PI_VERSION. The mitigations (capability gating, docs caveats, sandbox-image precondition) are substantial, but this deviates from the issue's stated acceptance sequencing. Consider either removing pi from ValidRuntimes() until the lifecycle test passes, or updating issue Track pi (earendil-works/pi) as a supported agent runtime #6464 criterion 8 to reflect the revised sequencing.

Low

  • [credential-hygiene] internal/runtime/pi_run.go:130 — The ANTHROPIC_* credential clearing uses shell unset before sourcing .env. Since .env is agent-writable, an agent could write ANTHROPIC_API_KEY back into .env to have it re-set after the unset. Mitigated by the sandbox using WIF/OIDC with no real API key available — there is no credential to leak.

  • [stale-runtime-list] docs/glossary.md:23 — The Agent Runtime glossary entry says "Claude Code and OpenCode are the primary runtime candidates" but omits pi, which is now a fully implemented, user-selectable runtime. The PR already updates all operational runtime references; this is a background reference document.

  • [edge-case] internal/runtime/pi_run.go:147 — In buildPiRunCommand, the Vertex extension (-e /opt/pi-extensions/anthropic-vertex) is always appended regardless of provider. When using a direct anthropic provider, the extension is still loaded. This is inert (registers an unused provider) but loads unnecessary Vertex SDK startup work.

  • [edge-case] internal/runtime/pi_extension/fullsend-hooks.js:60 — The SEPARATORS regex for Bash allowlist command splitting uses lookbehind/lookahead to distinguish backgrounding & from fd redirections. Edge cases in complex shell syntax could theoretically bypass the first-token check. The allowlist defaults to advisory mode; enforce mode is opt-in. High-severity bypass vectors (command substitution, subshells, eval/exec wrappers, env prefixes, paths) are correctly blocked.

Previous run (5)

Review

Findings

Medium

  • [protected-path] .github/workflows/lint.yml, images/README.md, images/code/Containerfile, images/sandbox/Containerfile — This PR modifies governance and infrastructure files under protected paths (.github/, images/). The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale (adding the pi CLI and Vertex extension to the sandbox image, adding Node.js setup for the hook extension test suite). Human approval is always required for protected-path changes, regardless of context.

  • [TOCTOU] internal/runtime/pi_run.go — The manifest (fullsend-manifest.json) and hook scripts remain agent-writable between iterations. The adapter's SHA-256 is verified via piHooksGuard (using command -p sha256sum to bypass PATH manipulation), but the manifest itself — which controls bashAllowlistMode, toolNames, and script lists — has no integrity check. A malicious agent could modify the manifest between iterations to weaken the Bash allowlist or alter tool-name mappings. This is a known-accepted risk matching Claude Code's posture (where hooks.json is similarly agent-writable), documented in docs/runtimes.md.

Low

  • [pre-filter inconsistency] internal/runtime/pi_transcript.go:165parsePiSessionEntries uses a compact-only "type":"message" pre-filter while isPiStreamCapture in the same file checks both compact and spaced variants. Pi writes compact JSON in practice, making this a consistency issue rather than a live bug, but worth aligning with isPiStreamCapture's dual-pattern approach for robustness.

  • [misleading-label] PR title — The PR title uses refactor(#6464) for ~3877 lines of net-new code adding a new runtime. Per COMMITS.md, refactor is for internal packages that don't change user-visible behavior — defensible since pi is not yet in ValidRuntimes(). If the intent is to surface this in release notes under Features when pi becomes user-selectable, update the prefix at that time.

  • [shared-mutable-map] internal/runtime/pi_bootstrap.go:179piHooksManifestFor assigns the package-level claudeToolForPi map directly to the manifest struct's ToolNames field. Safe today (the manifest is serialized immediately and never mutated), but fragile if a future change adds mutation to the manifest's tool-name map after construction. Consider copying the map.

Previous run (6)

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile, images/code/Containerfile, images/README.md — This PR modifies files under the images/ protected path (sandbox Containerfile adds pi CLI and pi-anthropic-vertex extension install; code Containerfile updates comments; README.md adds supply-chain pin entries). The PR links to Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale (version-pinning pi into the sandbox image for the Bootstrap/Run work). Human approval is always required for protected-path changes, regardless of context.

Low

  • [supply-chain] images/sandbox/Containerfile — The pi-anthropic-vertex extension is installed from a vendored package-lock.json containing 29 transitive dependency packages that are not managed by Renovate. Security advisories in those dependencies will only surface when the extension tag is bumped. The PR documents this limitation (images/README.md: "that lockfile is not renovate-managed, so advisories in it surface only when the tag is bumped") but relies on manual npm audit during bump reviews.

  • [error-handling-gap] internal/runtime/pi_progress.go — In the oversized-line skip loop (~lines 553–558), when isPrefix is true the inner loop consumes continuation fragments via br.ReadLine(). If that inner call returns an IO error, the error is silently consumed before the outer loop's next br.ReadLine() surfaces the same error. No data loss or incorrect behavior results (the outer loop handles the error on its next iteration), but the intermediate error is not handled directly. Non-actionable — matches the same pattern used by other runtime parsers.


Labels: PR modifies sandbox image (Containerfile, sandbox.go constants) and adds documentation for the new pi runtime

Previous run (7)

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile — This PR modifies files under the protected path images/: images/README.md, images/code/Containerfile, images/sandbox/Containerfile. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and explains the rationale for adding pi to the sandbox image. Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/runtime/pi_progress.go:506 — The compaction state machine coupling between compaction_start and compaction_end handlers is implicit: compaction_start only sets compacting=true when pendingResult is non-nil, and compaction_end unconditionally clears it. While the compaction_end handler has a pendingResult != nil guard preventing actual harm from out-of-order events, the implicit invariant could confuse future maintainers.

  • [sandbox-security-hooks] internal/runtime/pi.go — PiRuntime.Bootstrap is a stub that returns an error without type-asserting SandboxHooksBootstrap. The stub correctly prevents execution, the tracking issue Track pi (earendil-works/pi) as a supported agent runtime #6464 is referenced, and the security matrix documents the gap. Consider adding a TODO or marker test to remind the future implementer to wire hooks when the stub is replaced.

  • [supply-chain] images/sandbox/Containerfile — The pi-anthropic-vertex extension (twoGiants/pi-anthropic-vertex v0.1.13) is a third-party TypeScript extension that runs inside the pi process with sandbox-level privileges. Risk is mitigated by SHA256 tarball pinning, read-only permissions, manual renovate review requirement, and an equivalent trust model to the existing Claude Code npm install.

  • [test-adequacy] internal/runtime/pi_progress_test.go — No test exercises the oversized-line skip path when the oversized line is a terminal event (e.g., agent_end). Adding such a test would verify the incomplete-stream fallback handles this edge case.

  • [stale-doc] docs/glossary.md:23 — The Agent Runtime glossary entry mentions "Claude Code and OpenCode are the primary runtime candidates" but does not mention Pi, which this PR registers with the same stub status as OpenCode.

  • [comment-format] internal/runtime/event.go:3 — The streamBufSize doc comment says "used by both NDJSON stream parsers (Claude and OpenCode)" but Pi's parser now uses it too, making it the third consumer.

  • [error-handling-gap] internal/runtime/pi_progress.go:397 — The finish(lost=true) path has a theoretical scenario where settledResult from a first prompt could mask an incomplete second prompt. Tests confirm this cannot occur because agent_start clears sawAgentEnd when settledResult is set.

Previous run (8)

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile — This PR modifies images/sandbox/Containerfile, which is under a protected path (images/). The change is well-justified (version-pinned pi CLI install for supply-chain safety, matching the existing Claude Code pin pattern) and authorized by the linked issue Track pi (earendil-works/pi) as a supported agent runtime #6464. Human approval is always required for protected-path changes regardless of justification.

Low

  • [edge-case] internal/runtime/pi_progress.go:675 — The combination of multi-prompt execution and a mid-stream read error is not directly tested. The behavior is correct (reports as incomplete via the discardPending() → incomplete-fallback path), but the specific lost=true with prior settledResult scenario is only exercised by inference from adjacent tests (TestParsePiStream_ReadErrorStillEmitsResult and TestParsePiStream_SecondPromptDiesBeforeAgentEnd), not directly.
  • [error-handling] internal/runtime/pi_progress.go:551parsePiStream emits a ResultEvent via the callback even when returning a non-nil error (on read errors). This dual-delivery contract is tested (TestParsePiStream_ReadErrorStillEmitsResult) but not documented in the function's doc comment. A future caller could assume a non-nil error means no ResultEvent was emitted.
  • [comment-trailing-period] internal/sandbox/sandbox.go:29 — The SandboxPiConfig doc comment's first sentence is missing a trailing period, inconsistent with the adjacent SandboxWorkspace and SandboxClaudeConfig constants which both end their synopsis sentences with periods.
  • [incomplete-doc] docs/contributing/sandbox-topology.md:15 — The "Key additions over parent" column for fullsend-sandbox does not mention pi, which this PR adds to the same Containerfile. Consider adding "pi (stub)" to the list.
Previous run (9)

Review

Findings

Low

  • [comment-consistency] internal/runtime/event.go:3 — The streamBufSize comment says "both NDJSON stream parsers (Claude and OpenCode)" but parsePiStream is now a third consumer. Update to include Pi.

  • [session-id-validation] internal/runtime/pi_progress.go:668 — The sessionID returned by parsePiStream is taken from the untrusted NDJSON session event with no validation. Currently not wired into any consumer, but when Bootstrap/Run are implemented, the value should be validated before use in file paths or logs.

  • [scope-boundary] docs/runtimes.md:183 — Date-stamped upstream status claims ("still open as of 2026-08-21", "last updated 2026-07-25") will become stale. Consider removing specific dates or noting they should be updated periodically.

  • [error-message-consistency] internal/runtime/pi.go:48 — Pi stub error messages include issue references (see #6464) while the OpenCode stub does not. Minor inconsistency in error format between stub runtimes.

Info

  • [provenance-warning] — Prior review context discarded: provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments.
Previous run (10)

Review

Findings

Low

  • [edge-case] internal/runtime/pi_progress.go:110piTruncate operates on byte length (len(s)) and byte slice (s[:n]), which can split a multi-byte UTF-8 character and produce invalid UTF-8 output. The codebase has two established rune-safe truncation patterns: utf8.RuneCountInString + []rune slicing in claude_progress.go, and walk-back with utf8.Valid in truncateError. Consider aligning with one of these.
  • [incomplete-doc] docs/glossary.md:23 — The Agent Runtime glossary entry lists "Claude Code and OpenCode" as primary runtime candidates but omits pi, which is now registered at the same stub level as OpenCode.
  • [comment-stale] internal/runtime/event.go:5streamBufSize doc comment says "both NDJSON stream parsers (Claude and OpenCode)" but parsePiStream is now a third consumer.
  • [scope-observation] docs/runtimes.md — Forward-looking implementation notes in the security matrix and config-key table (TypeScript extension API details, upstream PR references) may need updating as upstream pi evolves with its fast release cadence.
Previous run (11)

Review

Findings

Medium

  • [scope-exceeded] — The PR body contains Closes #6464, which will auto-close the tracking issue when merged. Issue Track pi (earendil-works/pi) as a supported agent runtime #6464 has 8 acceptance criteria (documented in the issue body), and this PR addresses items 1-3 only (documentation, Resolve() registration, parsePiStream). Items 4-8 (Bootstrap/Run, transcript handling, tool allowlist, e2e test, ValidRuntimes() enablement) are deferred to subsequent work. Auto-closing will lose visibility into the remaining acceptance criteria.
    Remediation: Change Closes #6464 to Part of #6464 or Toward #6464 in the PR body.

  • [code-organization] internal/runtime/opencode_progress.go:11redactSummary is defined in opencode_progress.go but is now called by both the OpenCode and Pi parsers. Placing a cross-runtime helper in one runtime's file creates implicit coupling: deleting or renaming opencode_progress.go would break pi_progress.go compilation.
    Remediation: Move redactSummary (and its dependency progressRedactor) to a shared file such as progress_helpers.go or redact.go.

Low

  • [misleading-label] — PR title uses feat(#6464) but the change adds no user-visible functionality. PiRuntime is a stub excluded from ValidRuntimes() — users cannot select it via config. Per COMMITS.md, internal packages/helpers/abstractions without user-visible behavior change should use refactor. Using feat will surface this in the Features section of GoReleaser release notes.
    Remediation: Change PR title to refactor(#6464): register pi runtime stub with stream parser.

  • [test-inadequate] internal/runtime/registry_test.goTestResolveFromPerRepoConfig has an explicit test case showing that opencode (a stub not in ValidRuntimes()) is reachable via a hand-written config bypassing validation. An analogous test case for pi was not added, breaking the pattern where each stub runtime has a matching ResolveFromPerRepoConfig test.

Previous run (12)

Review

Findings

Medium

  • [misleading-label] PR title — The PR title uses the feat prefix, but per COMMITS.md, feat is reserved for user-facing features. This stub is excluded from ValidRuntimes() and is not user-selectable. The OpenCode stub precedent (refactor(runtime): register opencode as a stub runtime #6035) used refactor(runtime). Consider changing to refactor(#6464): register pi runtime stub with stream parser.

  • [stale comment] internal/runtime/event.go:3 — The streamBufSize comment says "used by both NDJSON stream parsers (Claude and OpenCode)" but the Pi parser now also uses this constant, making the enumeration incomplete. Consider updating to "used by all NDJSON stream parsers" or listing all three.

Low

  • [data consistency] internal/runtime/pi_progress.go:193 — The agent_end handler uses cumulative values from the event payload while numTurns is locally accumulated. The dual-path design (prefer agent_end when present, fall back to local accumulation for truncated streams) is correct, but an inline comment clarifying that agent_end.Usage carries cumulative totals would improve maintainability.

  • [comment format consistency] internal/runtime/pi.go:17 — PiRuntime struct comment includes upstream repo reference (earendil-works/pi) while OpenCodeRuntime omits its upstream reference in the struct comment. Cosmetic inconsistency; the parenthetical is arguably more informative.

  • [test pattern consistency] internal/runtime/registry_test.goTestResolveFromPerRepoConfig has a documented pattern for opencode showing stubs not in ValidRuntimes() can be reached via hand-written config (lines 67–73). No analogous test case for pi was added.

  • [stale-doc] docs/glossary.md:23 — The Agent Runtime glossary entry lists only Claude Code and OpenCode as runtime candidates. Pi is now also registered. Consider generalizing to avoid hardcoding the list.


Labels: PR adds a new runtime implementation under internal/runtime/ with documentation updates

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/runner Agent runner behavior and lifecycle labels Aug 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:13 PM UTC · Completed 10:28 PM UTC

Commit: 1e2c0b7 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:41 PM UTC · Completed 10:55 PM UTC

Commit: 0037c66 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Aug 21, 2026
Bake pi's safety env vars (PI_OFFLINE, PI_SKIP_VERSION_CHECK,
PI_CODING_AGENT_DIR/SESSION_DIR) into the sandbox image directly, since the
binary ships to every sandbox regardless of runtime selection while
PiRuntime.EnvExports() only runs once Bootstrap is implemented. Add a
build-time Node engine check (pi requires >=22.19.0) so an incompatible base
image fails loudly instead of silently. Disable renovate automerge for pi's
version pin given its documented history of breaking --mode json wire
changes within a minor. Align docs/runtimes.md's pi columns with OpenCode's
existing phrasing for stub hook rows. Add missing pi test parity alongside
existing opencode cases (config validation rejection, SandboxPiConfig
pinning, capability-table coverage) and cite sources for env var/CLI-flag
claims in code comments.

Assisted-by: Claude (fix), Claude (review), Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:22 AM UTC · Completed 12:36 AM UTC

Commit: 120c07d · View workflow run →

Derive PiRuntime.EnvExports() paths from ConfigDir() via fmt.Sprintf
(matching ClaudeRuntime's pattern) instead of a second raw reference to
sandbox.SandboxPiConfig, so a future path change only needs one update.
Pass nil (not ui.New(nil)) for the *ui.Printer param in
TestPiRuntimeRun_NotImplemented, matching OpenCode's equivalent test.

Assisted-by: Claude (fix), Claude (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-coder Bot and others added 3 commits August 21, 2026 20:46
Add PiRuntime as a stub implementation of the Runtime and
TranscriptHandler interfaces for the pi agent runtime
(earendil-works/pi), following the same pattern as
OpenCodeRuntime. The runtime is resolvable via
runtime.Resolve("pi") but intentionally excluded from
ValidRuntimes() until Bootstrap/Run are functional (per
the #6035 precedent).

Key additions:
- PiRuntime stub (pi.go): implements Runtime and
  TranscriptHandler with not-implemented errors, mirroring
  OpenCodeRuntime.
- parsePiStream (pi_progress.go): maps pi's --mode json
  NDJSON event stream to AgentEvent values. Handles session
  header (InitEvent), text, thinking, tool_result,
  message_end (TokensEvent), agent_end (ResultEvent), and
  error events. Detects stop_reason=error/aborted for the
  exit-0-override since --mode json exits 0 on model error.
  Falls back to synthesized ResultEvent on truncated streams.
- Test fixtures (testdata/pi/): recorded from pi 0.84.2
  --mode json output covering basic run, error run,
  reasoning, multi-step, malformed input, empty input, and
  truncated stream scenarios.
- Registry wiring: "pi" case in Resolve() switch.
- Documentation: pi column in docs/runtimes.md security
  feature matrix, config key support table, registered
  runtimes row, and pi-specific known constraints section.

Note: pre-commit could not run (sandbox network policy
blocked git fetch during hook environment init).
golangci-lint not available in sandbox. go vet passes.

Related to #6464
The stub parser and fixtures used invented event names. Map the
documented --mode json schema so tests cover the real contract.

Assisted-by: Grok (fix), Claude (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Retry checkpoints must not sticky-fail a later success, tool summaries
must read content[].text and stay bounded, and pi stays out of
ValidRuntimes until the stub is selectable.

Assisted-by: Grok (fix), Claude (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
pi's built-in default thinking level is "medium" (core/defaults.js
DEFAULT_THINKING_LEVEL) while Claude Code runs at "high" on Vertex and
API-key accounts, and the fleet agents set no `effort:` — so the same
agent reasoned a level lower on pi. The run command now always passes
--thinking: the harness effort when it is a pi level, "high" otherwise
(unset, or an unrecognised value, which is still warned about). pi maps the
level onto Anthropic adaptive effort and clamps it for models without
reasoning.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 7:59 PM UTC · Ended 8:02 PM UTC

Commit: 4619aa3 · View workflow run →

- the known-prefix exemption only covers word-shaped remainders
  (`ghs_maskable`, `glpat-new`); real GitLab (`glpat-`/`glrt-`...),
  Google OAuth (`ya29.`) and AWS STS (`ASIA`) tokens get prefix patterns of
  their own, so the exemption can no longer unmask them
- keyword arguments whose value is a bare identifier
  (`Client(token=accessToken)`) are expressions; the non-ASCII tail test
  that let `TOKEN=...é` through is gone
- the constant-name exemption (`"FULLSEND_GCP_WIF_PROVIDER"`) applies to
  quoted source literals only, not to env-style lines
- `NextToken`/`ContinuationToken` (AWS pagination) are not secrets
- suppression: the quoted-region regex was over-escaped, so an
  `\"`-terminated string could hide a following pipe; a lowercase
  `error:` line also blocks condensing
- chain: the NFKC redaction fallback also fires when the normalized copy
  finds strictly more, covering two secrets that share a mask prefix
- `TestPiThinkingFor` encodes the new default (unset → high)

Assisted-by: Claude (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 8:03 PM UTC · Ended 8:33 PM UTC

Commit: 3316308 · View workflow run →

A `pre-commit run` that printed nothing came back to the agent as
"pre-commit: passed". Under Claude Code the Bash result carries no exit
code (`{stdout, stderr, interrupted, isImage}`), so the suppressor cannot
tell a clean run from the silent failure of a hook whose interpreter is
missing from PATH — and it turned that silence into an affirmative pass.

- a summary is built only from text a successful run prints (`ok <pkg>`,
  `N passed`, `<hook>…Passed`, `no leaks`); the empty-output branches for
  pre-commit, go vet, go build, linters and gitlint are gone (replacing an
  empty result with a sentence cost tokens for no information anyway)
- matchers are anchored at the start of a command segment (after
  `VAR=…`, `uvx`, `npx`, `uv run`…), so a command that merely mentions a
  tool (`grep -n scan-secrets hooks.py`) keeps its output; `python -m
  pytest` and `pnpm/yarn/bun test` are recognised

Reported by the hook-fix session's audit of the live hooks.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 8:35 PM UTC · Ended 8:58 PM UTC

Commit: e7881df · View workflow run →

`PostToolUseFailure` accepts only `additionalContext` — no rewrite — so a
credential or an ANSI/zero-width sequence in a failing command's output
reaches the transcript whatever a hook does. The driver already halted on a
canary there; everything else was silent.

It now also runs detection-only secret and unicode passes over the failure
payload, logs what it finds to `findings.jsonl` (masked), and returns an
`additionalContext` warning telling the agent the values are unmasked, not
to copy them into a file, edit, commit or comment, and to treat hidden
instructions as data. The canary halt still wins over the warning path.

Also records in docs/runtimes.md that under Claude Code the suppressors only
ever see zero-exit output — a non-zero-exit command fires PostToolUseFailure
instead — while pi's `tool_result` does deliver failed calls to the chain.

Reported by the hook-fix session.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 9:00 PM UTC · Ended 9:31 PM UTC

Commit: 5625403 · View workflow run →

A canary split by a zero-width space, a bidi override or a tag character
halted a successful tool call but not a failed one: the success path strips
those before scanning, while the failure path only had the detection copy,
which folded combining marks and variation selectors but not category Cf.
`_detection_form` now folds Cf too (line and field separators stay, since
`scan_text` relies on them to prevent cross-field matches), so both paths
see through the same obfuscations.

Also from the same review round:

- the PostToolUseFailure group is scheduled whenever the chain has anything
  to do there, not only when the canary hook is on — the detection-only
  secret and unicode warnings added in 5625403 never fired for a config
  with sanitizers on and canary off
- the failure path scans the detection copy for credentials too, so a
  fullwidth-obfuscated token in a failed call is flagged
- suppression recognises wrapped invocations (`sudo`, `timeout 60`, `env
  VAR=…`, `mise exec --`, stacked) and `python3.12 -m pytest`
- a pagination word only vetoes a token name (`NextToken`,
  `ContinuationToken`); `NEXT_SECRET` and `PAGE_PASSWORD` are secrets again
- the `docs/runtimes.md` phase bullet said the failure event runs "canary
  detection only", contradicting the caveat two lines below it

Assisted-by: Claude, Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:32 PM UTC · Completed 10:14 PM UTC

Commit: e20d933 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

Resolves the runtimes config-key table: main added a
`validation_loop.feedback_mode` row (#1050/#6494) while this branch added
the Pi column. Kept both, with pi's cell filled in.

Signed-off-by: Wayne Sun <gsun@redhat.com>
`buildPiRunCommand` hardcoded pi's positional prompt, so the validation
loop's `feedback_mode` (#1050, merged from main as #6494) degraded to a
blind retry on pi: the previous iteration's failure was never injected and
the agent re-ran the same task with no idea it had failed. The prompt is
now `params.Prompt` with the documented fallback to `DefaultAgentPrompt`,
matching `ClaudeRuntime.buildRunCommand`, and the runtimes key-support
matrix records pi as honouring the key.

Reported by the review bot on PR #6467.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:20 PM UTC · Completed 10:39 PM UTC

Commit: 3822019 · View workflow run →

@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.

Comment thread internal/runtime/pi_run.go
Comment thread images/sandbox/Containerfile
The detection copy learned format characters last round but still left the
line and paragraph separators (U+2028/U+2029), NUL and ANSI/OSC escapes
that `unicode_posttool` strips on a successful call — so those still split
a canary on a failed one, the same success/failure asymmetry the previous
commit set out to close. `_detection_form` now removes `Cf`, `Zl`, `Zp`,
`Cc` (except the newline `scan_text` uses as its field separator, and the
tab/carriage return beside it) and whole escape sequences, since dropping
only the ESC would leave the parameter bytes splitting the token anyway.

Also from the same review round:

- a pagination word no longer vetoes a qualified key name: `NEXT_API_KEY`
  and `PAGE_ACCESS_KEY` are secrets again, while `NextToken`,
  `nextPageToken` and `NEXT_PUBLIC_API_KEY` stay exempt
- the failure phase is scheduled only when something actually runs there —
  suppression rewrites output, which the event does not allow, so a
  suppress-only configuration no longer schedules a no-op hook
- `_handle_failure` uses the public `hook_io.nfkc` rather than the private
  helper, so it cannot drift from the success path
- `timeout` is matched before `time` in the wrapper prefix instead of
  relying on backtracking
- stale wording: the phase godoc, the sanitizer-scope bullet's prefix list
  and the detection-copy description in docs/runtimes.md

Assisted-by: Claude, Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:43 PM UTC · Completed 11:03 PM UTC

Commit: e0ee28e · View workflow run →

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving after four multi-agent review rounds (Claude + Grok) on the sanitizer/hook work and three on the pi runtime, with every finding either fixed or answered in-thread.

Verified locally on the merged tree: 231 hook tests + 9 subtests, go build ./..., go test ./internal/..., node --test internal/runtime/pi_extension/ (16), ruff/ty/bandit/gitleaks clean. CI green on the merge head that includes main's latest, with the two known flakes (#6489 waitForFork, functional-tests judge) re-run.

Behaviour changes worth knowing at release: PostToolUse sanitizers are effective under Claude Code for the first time (#6468/#6357), so their heuristics were scoped to stop rewriting ordinary source, condensing compound commands that failed, or NFKC-rewriting non-ASCII content — a 900-file Read sweep went from 105 rewrites to 14, all real-shaped fixtures. Suppression now condenses only on positive evidence a tool printed, never from silence. Failed tool calls are wired to PostToolUseFailure for canary halt plus detect-and-warn, which is the ceiling that event's contract allows.

Follow-ups filed rather than folded in: #6502 (validation feedback injected unframed/unsanitized), #6485 (local minimal pi run doc). Accepted risks answered in-thread: manifest TOCTOU (matches Claude Code's posture, sandbox is the boundary) and the interim Vertex extension's vendored lockfile (deleted when upstream pi#5262 lands).

@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 22, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 10:56 PM UTC · Completed 11:51 PM UTC

Commit: e0ee28e · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review skipped — this PR is already merged.

The /fs-review command only reviews open PRs/MRs.

Posted by fullsend post-review check

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6467 — pi runtime implementation

Timeline: Issue #6464 was created by waynesun09 at 21:05 UTC on Aug 21. Triage ran at 21:06. The code agent (run 32527743227) produced 1 commit — a stub runtime with stream parser — in ~16 minutes at $4.25 cost (75 turns, 111 tool calls). The human author then expanded the PR to 84 files / +9,819 lines across 58 additional commits over ~25 hours, implementing Bootstrap/Run, the Vertex provider, the sandbox hook adapter, PostToolUse sanitizer scoping (#6357/#6468), behaviour tests, documentation with click-to-enlarge diagrams, and landing-page updates. The review agent ran 48 times (14 successful, ~34 cancelled by rapid pushes). The human approved after thorough local verification (231 hook tests, go build ./..., go test ./internal/..., node --test 16 tests, ruff/ty/bandit/gitleaks clean) and the PR merged at 22:54 UTC on Aug 22.

Code agent assessment

The code agent performed well within its constraints. It correctly scoped to a deliverable subset (acceptance criteria 1–3 of 8), followed existing runtime patterns (OpenCode precedent), delegated codebase exploration to a sub-agent, and set closes_issue: false. The $4.25 cost for a well-structured starting point is good value. However, its stream parser used event names that didn't match pi's actual --mode json wire format — the human's next commit (e0d4a20) was titled "align parsePiStream with pi 0.84.2 json wire format" and stated the fixtures used "invented event names." This is a known class of code agent limitation where it cannot verify against external tool output.

Review agent assessment

Value delivered: The review agent found genuinely useful issues. The piTruncate UTF-8 boundary edge case was fixed (5be4e92). The Vertex extension loading regardless of provider was fixed (3778e93). The RunParams.Prompt hardcoding was identified as load-bearing by the human and fixed (3822019). Multiple low-severity findings drove incremental improvements. The human's approval noted "every finding either fixed or answered in-thread."

Cost concerns: 12 successful review runs with valid telemetry cost $111.49 total (avg $9.29/run). Two additional runs crashed (exit_code=-1, 256–478 tool calls over ~44 min each) but reported $0 in telemetry. ~34 cancelled runs consumed approximately 6.2 hours of GHA compute time. Conservative total estimate: $150–250 across all 48 runs.

Evidence for existing open issues

  • Review dispatch debounce (#1014, #4069): The heaviest churn window was Aug 22 13:48–17:58 UTC, with 21 review dispatches in ~4 hours as the human iterated rapidly. Most were cancelled within minutes by the next push. This is the most extreme case I've seen — 34 cancelled runs on a single PR.

  • Review finding dedup (#2959, #5139, agents#721, agents#685): The protected-path finding appeared in 11 consecutive review runs (runs 6–13). The TOCTOU manifest finding appeared in 7+ runs. Both were acknowledged by the human as accepted risks but continued to reappear. The human had to re-explain the same decisions across multiple review threads.

  • Severity oscillation (#2993): The TOCTOU finding was rated medium in some runs and low in others with identical substance.

  • Telemetry gap on crash (#5361): Runs 32591362372 and 32599789096 both exited with code -1 on both iterations, ran 256–478 tool calls over ~44 minutes each, but telemetry reported $0 cost and 0 tokens. The workflow job still concluded as "success" despite all iterations failing.

  • Factual verification (agents#420): The review agent made at least one factually incorrect claim about execution order that was corrected by the human reviewer in-thread.

Autonomy assessment

The review agent's useful-finding rate (several findings drove real code changes) demonstrates value, but the 48-run cost burden and repeated findings mean the current pipeline requires active human engagement to filter signal from noise. The human's approval added verification the review agent cannot currently perform: running the full test suite locally, tracing execution order empirically, validating against external tools (Claude, Grok, Cursor, Codex were all used), and filing follow-up issues (#6502, #6485). Closing the dedup gap (agents#721) would be the single highest-impact improvement for this class of PR.

No new proposals filed — all improvement opportunities map to existing open issues. The evidence from this PR is noted above for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/docs User-facing documentation component/runner Agent runner behavior and lifecycle component/sandbox OpenShell sandbox environment ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PostToolUse sandbox hooks read tool_result but Claude Code sends tool_response — sanitizers are inert under Claude Code

1 participant