Skip to content

fix(pi): activate grep/find/ls and ship rg + fd in the sandbox image - #6752

Merged
waynesun09 merged 3 commits into
mainfrom
pi-default-tools-rgfd
Sep 1, 2026
Merged

fix(pi): activate grep/find/ls and ship rg + fd in the sandbox image#6752
waynesun09 merged 3 commits into
mainfrom
pi-default-tools-rgfd

Conversation

@waynesun09

@waynesun09 waynesun09 commented Aug 29, 2026

Copy link
Copy Markdown
Member

Summary

pi's built-in grep, find and ls tools have never worked on the fleet's pi runs. Two causes stack:

  1. Never activated. pi 0.84.x starts with only read, bash, edit, write active (packages/coding-agent/src/core/sdk.ts defaultActiveToolNames); grep, find and ls are registered but inactive unless settings.json sets defaultTools or the run passes --tools. piSettingsJSON() seeded neither, and buildPiRunCommand only emits --tools for agents with tools: frontmatter — so code, fix, review and retro never had the search tools (the 2026-08-23 local review transcript was bash ×22 / read ×13, nothing else).
  2. No binaries. Even when active, grep shells out to rg and find to fd (utils/tools-manager.ts). The sandbox image ships neither; the runtime exports PI_OFFLINE=1, which makes pi skip its GitHub-release download ("ripgrep not found. Offline mode enabled, skipping download."), and the egress policy would block that download anyway. Both tools fail with <tool> is not available and could not be downloaded.

Changes:

Verified against pi 0.84.4 source

  • defaultTools is the settings key (docs/settings.md; core/sdk.ts:256-263: options.tools ?? (options.noTools ? [] : (configuredDefaultToolNames ?? defaultActiveToolNames)), defaultActiveToolNames = ["read","bash","edit","write"]), read from PI_CODING_AGENT_DIR/settings.json in print mode too.
  • --tools (main.ts:533options.tools) replaces the set; --no-builtin-tools short-circuits before defaultTools is consulted — so agents with tools: frontmatter keep their strict allowlist (existing TestBuildPiRunCommand_* cover the emitted flags; TestPiRuntimeBootstrap_NoSecurityNoHooks now asserts the no-tools: path).
  • Stream fixtures: the pi bump itself landed via chore(deps): update dependency @earendil-works/pi-coding-agent to v0.84.4 #6745, where json-event.ts was diffed 0.84.3→0.84.4 (byte-identical) and the adapter's node tests passed; nothing in this PR changes the wire format.
  • Renovate: RIPGREP_VERSION/FD_VERSION now have customManagers entries (github-releases); the SHA256 ARGs stay manual (ripgrep publishes .sha256 assets, fd publishes none) and the build fails on a mismatch until refreshed.

Test plan

  • go test ./internal/runtime/... (the two TestDummyRuntime_* failures are pre-existing whenever an OpenShell gateway is running)
  • Host pi 0.84.3: with rg/fd hidden from PATH + PI_OFFLINE=1, grep/find return the "not available and could not be downloaded" errors; with the binaries present both succeed; defaultTools makes the model report all seven tools.
  • podman build of the sandbox image (arm64): rg 15.2.0, fd 10.5.0, pi 0.84.4 present; SHA256 checks pass.
  • In-image end-to-end (real Vertex inference, --model anthropic-vertex/claude-haiku-4-5): grepinternal/runtime/pi_run.go:183: func buildPiRunCommand(...), findinternal/runtime/pi_run.go internal/runtime/pi_run_test.go, both isError=false.
  • fullsend run review --runtime pi on a fullsend-code image built from this branch against a real PR: schema-valid result, defaultTools present in the sandbox's /sandbox/pi-config/settings.json, hooks active; a Grok orchestrator on the same image used the grep tool 5× (opus prefers bash grep).
  • Gemini regression reproduced on 0.84.3 (sub-agent on google-vertex/gemini-3.7-flash under the sandbox proxy → HttpsProxyAgent is not a constructor); 0.84.4 image builds and pi loads both vendored extensions.
  • Fleet: repin fullsend-sandbox/fullsend-code digests in fullsend-ai/agents after the next image publish.

Refs #6464 (pi tracker). Rebased on #6745 (pi 0.84.4).

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Enable pi search tools, bundle rg/fd, and upgrade pi to 0.84.4

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

Grey Divider

AI Description

• Activate all seven pi built-ins by default for agents without tool frontmatter.
• Bundle checksum-pinned ripgrep and fd binaries for offline amd64 and arm64 sandboxes.
• Upgrade pi to 0.84.4 to restore proxied Vertex Gemini requests.
Diagram

graph TD
  B["Pi Bootstrap"] --> S["settings.json"] --> A["Pi Agent"] --> T["Search Tools"] --> R["rg and fd"]
  I["Sandbox Image"] --> R
  I --> P["pi 0.84.4"] --> V["Vertex Gemini"]
Loading
High-Level Assessment

The chosen approach directly fixes both independent failure modes: settings activate registered tools, and image-baked binaries make them usable offline. Always emitting --tools would complicate explicit agent allowlists, while runtime downloads are incompatible with PI_OFFLINE and sandbox egress controls.

Files changed (4) +56 / -3

Bug fix (1) +11 / -1
pi_bootstrap.goActivate every pi built-in by default +11/-1

Activate every pi built-in by default

• Defines the seven built-in pi tools and writes them to defaultTools in the locked-down settings. Explicit --tools arguments continue to replace this default for agents with tool frontmatter.

internal/runtime/pi_bootstrap.go

Tests (1) +1 / -0
pi_bootstrap_test.goVerify pi default tool configuration +1/-0

Verify pi default tool configuration

• Extends bootstrap coverage to assert that generated settings activate read, bash, edit, write, grep, find, and ls.

internal/runtime/pi_bootstrap_test.go

Documentation (1) +2 / -1
pi_agent.goDocument default activation of mapped pi tools +2/-1

Document default activation of mapped pi tools

• Clarifies that all mapped pi built-ins are activated through settings.json because pi's native defaults include only the first four tools.

internal/runtime/pi_agent.go

Other (1) +42 / -1
ContainerfileBundle search backends and upgrade pi +42/-1

Bundle search backends and upgrade pi

• Upgrades pi from 0.84.3 to 0.84.4 to fix HTTPS-proxied Vertex Gemini requests. Adds checksum-pinned ripgrep 15.2.0 and fd 10.5.0 static musl binaries for amd64 and arm64, with build-time version checks.

images/sandbox/Containerfile

@qodo-code-review

qodo-code-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. pi guide update omitted ✓ Resolved 📘 Rule violation ⛨ Security
Description
The PR changes the pi runtime’s default tool capability and bumps its pinned version, but neither
updates nor explicitly references docs/contributing/runtime-implementation.md. This leaves the
required backend-change review and capability documentation unrecorded.
Code

internal/runtime/pi_bootstrap.go[235]

+var piDefaultTools = []string{"read", "bash", "edit", "write", "grep", "find", "ls"}
Relevance

●●● Strong

Recent runtime reviews accepted explicit documentation and comments for backend behavior changes; no
close rejection precedent found.

PR-#2727
PR-#1780

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed bootstrap line introduces a new default capability for the existing pi runtime. The
implementation guide identifies itself as the required reference for changing a runtime.Runtime
backend and its pi section documents version-sensitive verification and tool behavior, while the
supplied PR description does not reference that guide and the PR contains no update to it.

Rule 2889480: Consult runtime implementation guide when modifying runtime.Runtime backends
internal/runtime/pi_bootstrap.go[229-249]
docs/contributing/runtime-implementation.md[1-19]
docs/contributing/runtime-implementation.md[364-425]

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 `pi` backend now activates all seven built-in tools by default, but the runtime implementation guide was not updated and the PR description does not state that the guide was consulted.

## Issue Context
PR Compliance ID 2889480 requires runtime backend behavior and capability changes to be checked against `docs/contributing/runtime-implementation.md`, with the consultation explicitly recorded and applicable runtime documentation updated.

## Fix Focus Areas
- docs/contributing/runtime-implementation.md[364-425]
- docs/runtimes/pi.md[152-165]

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


Grey Divider

Context sources
✅ Compliance rules (platform): 62 rules
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/runtime/pi_bootstrap.go
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 3:55 PM UTC · Ended 3:59 PM UTC

Commit: fb8fa2c · View workflow run →

@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@waynesun09 waynesun09 changed the title fix(pi): activate grep/find/ls, ship rg + fd, and bump pi to 0.84.4 fix(pi): activate grep/find/ls and ship rg + fd in the sandbox image Aug 29, 2026
@waynesun09
waynesun09 force-pushed the pi-default-tools-rgfd branch from fb8fa2c to 8a0018d Compare August 29, 2026 15:59
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 4:01 PM UTC · Ended 4:05 PM UTC

Commit: 8a0018d · View workflow run →

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown

Site preview

Preview: https://96c5340f-site.fullsend-ai.workers.dev

Commit: 480f00523f3a469bf25702f6e39af08c0bf5ccf9

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:07 PM UTC · Completed 4:24 PM UTC

Commit: 9475ef0 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.73

@fullsend-ai-review fullsend-ai-review Bot added the risk/elevated PR risk: elevated label Aug 29, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

Risk Assessment: elevated (3/5)

Details

Elevated risk preserved from prior assessment: Tier 1 signals are unchanged (two protected-path files under images/, medium blast radius, moderate test ratio), high git churn persists across Containerfile (28 commits/30d with fix/reverts) and pi runtime files (pi_run.go: 21 commits/30d, pi_bootstrap.go: 10 commits/30d), partially offset by well-scoped changes tracing to issue #6464 and a known MEMBER contributor.

Previous run

Risk Assessment: elevated (3/5)

Details

Elevated risk preserved from prior assessment: Tier 1 signals are unchanged (two protected-path files under images/, medium blast radius, moderate test ratio), high git churn persists across Containerfile (28 commits/30d with 6 fix/reverts) and pi runtime files (pi_run.go: 21 commits/30d with 11 fix/reverts, pi_bootstrap.go: 10 commits/30d with 7 fix/reverts), partially offset by well-scoped changes tracing to issue #6464 and a known MEMBER contributor.

Previous run (2)

Risk Assessment: elevated (3/5)

Details

Elevated risk preserved from prior assessment: Tier 1 signals are unchanged (two protected-path files under images/, medium blast radius, moderate test ratio), high git churn persists across Containerfile (26 commits/30d with 6 fix/reverts) and pi runtime files (21 commits/30d on pi_run.go with 11 fix/reverts), partially offset by well-scoped changes tracing to issue #6464 and a known MEMBER contributor.

Previous run (3)

Risk Assessment: elevated (3/5)

Details

Elevated risk preserved from prior assessment: Tier 1 signals are unchanged (two protected-path files under images/, medium blast radius, moderate test ratio), high git churn persists across Containerfile (26 commits/30d with 7 fix/reverts) and pi runtime files (21 commits/30d on pi_run.go with 11 fix/reverts), partially offset by well-scoped changes tracing to issue #6464 and a known MEMBER contributor.

Previous run (4)

Risk Assessment: elevated (3/5)

Details

Elevated risk driven by two protected-path files (images/ prefix), high git churn across Containerfile and pi runtime files (15-26 commits in 30 days with frequent fixes), and medium blast radius, partially offset by well-scoped changes tracing to issue #6464 and a known contributor.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile, images/README.md — This PR modifies files under the protected images/ path prefix. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale for shipping ripgrep and fd binaries as pi's grep/find tool backends. Human approval is always required for protected-path changes, regardless of context.
Previous run

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile, images/README.md — This PR modifies files under the protected images/ path prefix. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale for shipping ripgrep and fd binaries as pi's grep/find tool backends. Human approval is always required for protected-path changes, regardless of context.

Low

  • [test-inadequate] internal/runtime/pi_bootstrap_test.goTestPiDefaultTools_CoversToolMap checks that every pi tool in piToolForClaude is present in piDefaultTools (map → defaults direction), but does not check the reverse: that every entry in piDefaultTools has a corresponding entry in claudeToolForPi. If a tool were added to piDefaultTools without updating claudeToolForPi, the hook adapter would receive an unmapped pi tool name instead of the expected Claude name. Remediation: add a reverse check iterating piDefaultTools and asserting each has an entry in claudeToolForPi.

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] images/sandbox/Containerfile, images/README.md — This PR modifies files under the protected images/ path prefix. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale for shipping ripgrep and fd binaries as pi's grep/find tool backends. Human approval is always required for protected-path changes, regardless of context.

Low

  • [test-inadequate] internal/runtime/pi_bootstrap_test.goTestPiDefaultTools_CoversToolMap checks that every pi tool in piToolForClaude is present in piDefaultTools (map → defaults direction), but does not check the reverse: that every entry in piDefaultTools has a corresponding entry in claudeToolForPi. If a tool name is added to piDefaultTools without a claudeToolForPi mapping, the hook adapter would pass the raw pi name to hook scripts, potentially causing silent allowlist mismatches. Remediation: add a reverse check asserting each piDefaultTools entry exists as a key in claudeToolForPi.
  • [fail-open] internal/runtime/pi_bootstrap.godefaultTools is populated from a hardcoded slice literal (piDefaultTools) with no conditional path; if settings.json upload fails, Bootstrap returns an error. Fail-closed design confirmed.
  • [architectural-fit] internal/runtime/pi_bootstrap.gopiDefaultTools seeded into piSettingsJSON() centralizes all pi settings coherently alongside defaultProjectTrust, quietStartup, retry, and compaction.
  • [architectural-fit] images/sandbox/Containerfile — ripgrep and fd installation follows the established supply chain pattern (version ARG + per-arch SHA256 + sha256sum -c + version assertion at build time), matching the gitleaks and tirith blocks.
  • [scope-completeness] internal/runtime/pi_bootstrap.godefaultTools sets the initial tool set when no --tools flag is emitted; when an agent explicitly declares tools: frontmatter, buildPiRunCommand emits --tools which fully replaces the defaultTools set (confirmed against pi 0.84.4 source). Tool restriction contract is preserved.

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

  • [protected-path] images/sandbox/Containerfile, images/README.md — This PR modifies files under the protected images/ path prefix. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale for shipping ripgrep and fd binaries as pi's grep/find tool backends. Human approval is always required for protected-path changes, regardless of context.

Low

  • [fail-open] internal/runtime/pi_bootstrap.godefaultTools is populated from a hardcoded slice literal with no conditional path; if settings.json upload fails, Bootstrap returns an error. Fail-closed design confirmed.
  • [architectural-fit] internal/runtime/pi_bootstrap.gopiDefaultTools seeded into piSettingsJSON() centralizes all pi settings coherently alongside defaultProjectTrust, quietStartup, retry, and compaction.
  • [architectural-fit] images/sandbox/Containerfile — ripgrep and fd installation follows the established supply chain pattern (version ARG + per-arch SHA256 + sha256sum -c + version assertion at build time), matching the gitleaks and tirith blocks.
  • [scope-completeness] internal/runtime/pi_bootstrap.godefaultTools sets the initial tool set when no --tools flag is emitted; when an agent explicitly declares tools: frontmatter, buildPiRunCommand emits --tools which fully replaces the defaultTools set (confirmed against pi 0.84.4 source). Tool restriction contract is preserved.
Previous run (4)

Review

Findings

Medium

  • [protected-path] images/sandbox/Containerfile, images/README.md — This PR modifies files under the protected images/ path prefix. The PR links to issue Track pi (earendil-works/pi) as a supported agent runtime #6464 and the description explains the rationale for shipping ripgrep and fd binaries as pi's grep/find tool backends. Human approval is always required for protected-path changes, regardless of context.

Labels: PR modifies sandbox image (images/sandbox/Containerfile), pi runtime code (internal/runtime/), and documentation

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/sandbox OpenShell sandbox environment labels Aug 29, 2026
@waynesun09
waynesun09 force-pushed the pi-default-tools-rgfd branch from 9475ef0 to f888e33 Compare August 29, 2026 16:26
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:28 PM UTC · Completed 4:47 PM UTC

Commit: f888e33 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.39

Comment thread docs/contributing/runtime-implementation.md Outdated

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

Why do we set the tools without mentioning the new tools installed? It shouldn't be installing rg (and friends) or setting the list of tools on pi settings?

waynesun09 added a commit that referenced this pull request Sep 1, 2026
Split the single-line "Hardening levers in use" bullet in
runtime-implementation.md into sub-bullets so it can be reviewed as a
diff, and state next to the defaultTools setting that pi's grep and
find tools shell out to rg and fd, which is why the sandbox image ships
them (PI_OFFLINE=1 and the egress policy block pi's own download). No
facts moved or removed.

Review feedback on #6752.

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

Copy link
Copy Markdown
Member Author

Why do we set the tools without mentioning the new tools installed? It shouldn't be installing rg (and friends) or setting the list of tools on pi settings?

Two different layers, and the mapping between them is pi's, not ours.

defaultTools lists pi's tool names (grep, find, ls), not binaries. The model only ever sees a grep tool and a find tool. pi's own tool implementation decides how to execute them: grep shells out to rg and find to fd (packages/coding-agent/src/utils/tools-manager.ts). The harness doesn't configure that coupling and can't change it, so there is nothing to name on the settings side beyond the tool itself.

Both parts are needed because they fail independently:

  • Activation. pi's upstream default is read, bash, edit, write only (core/sdk.ts defaultActiveToolNames); grep/find/ls are registered but inactive until settings.json defaultTools or --tools turns them on. Nothing else in the harness sets that, so every agent without tools: frontmatter had no search tools.
  • Backends. On first use pi tries to download rg/fd from GitHub releases into its config dir. PI_OFFLINE=1 skips that download and the egress policy would block it anyway, so the tool errors with <tool> is not available and could not be downloaded. Baking the binaries into the image is the only way to satisfy it offline, and it also gives us the SHA-pinned supply chain instead of a runtime fetch.

So: settings turn the tools on, the image supplies what pi's implementation of those tools runs. I've made that coupling explicit in docs/contributing/runtime-implementation.md next to the defaultTools line (181fceb), and reflowed that bullet into sub-bullets per your inline comment.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 1, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:47 PM UTC · Ended 6:56 PM UTC

Commit: 181fceb · View workflow run →

pi 0.84.x starts with only read, bash, edit and write active
(core/sdk.ts defaultActiveToolNames); grep, find and ls are registered
but inactive unless settings.json sets defaultTools or --tools is
passed. The runtime seeded neither, so every agent without `tools:`
frontmatter (code, fix, review, retro) never had the search tools.

Even when active, grep and find shell out to rg and fd, which the
sandbox image does not ship; PI_OFFLINE=1 makes pi skip its release
download and the egress policy would block it anyway, so both tools
fail with "<tool> is not available and could not be downloaded".

Seed defaultTools with all seven built-ins and bake ripgrep 15.2.0 and
fd 10.5.0 (static musl builds, per-arch SHA256 pinned like gitleaks)
into images/sandbox/Containerfile. Also records why pi 0.84.4 (already
on main via #6745) is the floor: 0.84.3 failed every google-vertex
request under the sandbox proxy (earendil-works/pi#8610).

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
Split the single-line "Hardening levers in use" bullet in
runtime-implementation.md into sub-bullets so it can be reviewed as a
diff, and state next to the defaultTools setting that pi's grep and
find tools shell out to rg and fd, which is why the sandbox image ships
them (PI_OFFLINE=1 and the egress policy block pi's own download). No
facts moved or removed.

Review feedback on #6752.

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

fullsend-ai-review Bot commented Sep 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:58 PM UTC · Completed 7:18 PM UTC

Commit: 262ba04 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.28

fullsend-ai-review[bot]

This comment was marked as outdated.

TestPiDefaultTools_CoversToolMap only checked map → defaults. Add the
reverse: each piDefaultTools entry has a claudeToolForPi key, so a tool
added to the defaults without a mapping fails here instead of reaching
hook scripts under its raw pi name.

Review feedback on #6752.

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

fullsend-ai-review Bot commented Sep 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:26 PM UTC · Completed 7:38 PM UTC

Commit: 480f005 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.46

@waynesun09
waynesun09 dismissed stale reviews from fullsend-ai-review[bot] and fullsend-ai-review[bot] September 1, 2026 19:35

Addressed in 480f005 (reverse tool-map check); human-approved by rh-hemartin, all required checks green.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Sep 1, 2026
@waynesun09
waynesun09 added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit f34eae9 Sep 1, 2026
60 of 62 checks passed
@waynesun09
waynesun09 deleted the pi-default-tools-rgfd branch September 1, 2026 20:24
@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 1, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:26 PM UTC · Completed 8:46 PM UTC

Commit: 480f005 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.52

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6752 — fix(pi): activate grep/find/ls and ship rg + fd in the sandbox image

Verdict: Workflow went well. The review agent surfaced a genuine, actionable finding that improved the codebase. No novel proposals — identified gaps are tracked by existing issues.

Timeline

  1. Aug 29 15:53 — PR created by waynesun09. Two independent failures prevented pi's built-in grep/find/ls tools from working: never activated in settings, and no rg/fd binaries in the sandbox image.
  2. Aug 29 15:53–16:05 — Three rapid pushes. First two review runs correctly cancelled by cancel-in-progress.
  3. Aug 29 16:07–20:31 — Three successful review runs ($7.73, $7.39, $7.16). Run 3 posted a [low] test-inadequate finding: TestPiDefaultTools_CoversToolMap only checked the map-to-defaults direction, not the reverse. This was a high-quality, actionable finding.
  4. Aug 29 15:55 — qodo-code-review[bot] separately caught that runtime-implementation.md was not updated alongside the behavioral change. Also actionable.
  5. Aug 31 07:02–07:05 — rh-hemartin approved with two pieces of feedback: (a) a conceptual question about the two-layer architecture (tool activation vs. binary backends), and (b) a docs formatting request ("Break this into multiple lines, it is unreviewable this way").
  6. Sep 1 18:45 — Author pushed docs reflow commit (262ba04) addressing human feedback. Review run 4 ($6.28) re-discovered and re-posted the same [test-inadequate] finding as a new inline comment.
  7. Sep 1 19:24 — Author pushed the reverse test fix (480f005). Review run 5 ($3.46) correctly recognized the fix, dropped the finding from the sticky comment, and changed verdict from request-changes to comment.
  8. Sep 1 20:24 — Merged via merge queue.

What went well

  • Review agent's [test-inadequate] finding was high-quality. It identified a genuine coverage gap (one-direction-only test assertion) that the author addressed in commit 480f005. This is exactly the kind of mechanical completeness check the review agent excels at.
  • Final run correctly verified the fix. Run 5 recognized that 480f005 resolved the prior finding, dropped it from output, and changed its verdict. The prior-review-aware pipeline is working for the sticky comment layer.
  • Cancel-in-progress working correctly. Three cancelled runs were by design — new pushes superseded stale reviews. No wasted agent time.
  • Multi-bot coverage complementary. qodo-code-review caught the docs update gap; fullsend-ai-review caught the test coverage gap. Together they covered more than either alone.

Evidence for existing issues

Review quality assessment

Source Findings Signal Notes
fullsend-ai-review [test-inadequate] High Led to commit 480f005
fullsend-ai-review [protected-path] Medium Procedural gate, worked as intended
fullsend-ai-review [fail-open], [architectural-fit] Low False positives, confirmed good patterns
qodo-code-review docs update omitted High Led to runtime-implementation.md update
rh-hemartin conceptual architecture question High Surfaced need for clearer docs on the two-layer design
rh-hemartin docs formatting Medium Improved readability of runtime-implementation.md

The human reviewer's conceptual question ("Why do we set the tools without mentioning the new tools installed?") surfaced a design-understanding gap that no automated reviewer caught. This type of architectural-level feedback — questioning whether the abstraction is clear to future readers — remains a human-review strength.

Total review agent cost: ~$32 across 5 successful runs. Approximately $6.28 was avoidable (run 4 on a docs-only commit). The final run's low cost ($3.46) suggests re-review scoping is partially working for cost, even if it still produces duplicate inline comments.

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

Labels

component/sandbox OpenShell sandbox environment requires-manual-review Review requires human judgment risk/elevated PR risk: elevated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants