feat(harness)!: pi extensions under one plugins: key

[waynesun09]

Summary

Stacked on #6752 (base branch pi-default-tools-rgfd; retarget to main once that merges).

Lets a harness ship pi extensions under the existing plugins: key — one list of directories, each loaded by whichever runtime reads its format — with no manifest, no tool-mapping table and no allowlist bookkeeping. Breaking: plugins: entries become PluginSpec (string form unchanged) and a local plugin directory must now be a plugin.json bundle or a pi-loadable directory.

plugins:
  - plugins/gopls-lsp                         # plugin.json → Claude Code loads it, pi names and skips it
  - extensions/go-diagnostics                 # pi entry point → pi loads it, Claude Code names and skips it
  - path: extensions/pi-fff                   # object form only when env or a pi flag is needed
    env:
      PI_FFF_MULTIGREP: "1"
    pi:
      args: ["--fff-mode", "override"]

Trust story (ADR 0094). Extensions are harness-repo content with the same trust as plugins:/scripts:/skills: (org-allowlisted URL base, content-addressed fetch, injection scan of every text file incl. node_modules). They never come from the target repository: defaultProjectTrust: never, --no-approve and --no-extensions stay exactly as they are — the runner appends vetted -e paths. Because --no-extensions + explicit -e already closes the set of code that can register tools, every non-built-in tool the model sees comes from a declared extension, which is why no per-tool declaration is needed. URLs, npm:/git:/ssh: sources (pi would install them from the network at startup) and .. segments are rejected at validation, as are env keys that would shadow runtime pins (PI_*, FULLSEND_*, provider/proxy variables).

Mechanics.

• harness: extensions: (string or {path, args, env} form), validation that the directory is something pi can load (index.js/index.ts, package.json pi.extensions, or top-level .js/.ts — otherwise pi would load nothing), URL-base resolution generalised from the plugin fetcher (fetchBaseDir with per-kind validation).
• Bootstrap uploads each directory to /sandbox/pi-config/extensions/<name>/ and records name/path/tree-hash/args/env in fullsend-manifest.json; the Claude Code runtime prints Extension "<name>": skipped … and continues (the mirror of pi's plugins: warning).
• Run preflight, in the same guarded block as the hook-adapter SHA check and before the agent-writable .env is sourced: each directory must exist, contain nothing but regular files and directories (symlinks are rejected at validation and fail the sandbox guard — pi's -e <dir> loader follows them), and hash (sorted sha256sum lines over find -type f plus the sorted directory list, identical algorithm in Go and in the shell snippet, cross-tested) to the value recomputed from the host copy — mismatch → exit 96 with fullsend: pi extension "<name>" is missing or was modified. Load order: provider extension → fullsend-hooks.js → declared extensions in harness order (pi runs tool_call handlers in -e order, first block wins); args follow each -e; env is exported last so it can never override the runtime's own exports.
• Hook adapter: extension tools get no special treatment from the hooks — every PreToolUse/PostToolUse group runs on them by name, and orgs that enable the optional tool_allowlist_pretool.py list extension tool names in FULLSEND_TOOL_ALLOWLIST verbatim (like mcp__* names); first use is logged as [fullsend-hooks] extension tool: <name> and the session_start roster line ends with extensions=<names>.
• Live progress: unknown tools show their first string arg among path|file|pattern|query|command (redacted) instead of nothing.
• Docs: docs/runtimes/pi.md "Extensions", key-support matrix (extensions ✓ pi / ignored Claude Code), harness reference/fields, runtime-implementation interface notes, ADR 0094 (0093 is claimed by docs(adr): adopt gh-aw safe outputs reference model #6614).

Review round 8 → fix (head ed414664)

Codex (gpt-5.6-sol) re-reviewed rounds 6–7. One real gap: a Claude plugin is claimed by its marker without a tree walk, so a symlink inside it passed ValidatePluginDirs/fullsend lock and was only caught by the injection scan, which runs with security enabled. pluginformat.TreeEntriesProblem now applies the shared entry rule to the Claude kind at validation (the pi detector already walked). Also: scan messages say "plugin" for both kinds, and three tests were tightened so they pin what they claim (benign manifest in the dispatch test, unknown-kind error, recorded dependency URL on the legacy-key path).

Review round 7 → fixes (head 568d0af5)

Grok re-reviewed the round-6 commit. Fixed: a symlink under a Claude plugin was skipped by the scan but still uploaded — the no-symlink tree rule now applies to both kinds and Detect uses Lstat so a symlinked marker cannot claim a directory; the manifest-only scanPluginDir and the unused ClaudeMarkerFiles helper are gone (dead code); PluginSpec.SameOptions treats absent and empty env/pi alike so env: {} is not a conflict; fullsend lock runs the on-disk plugin checks on the no-URL path too (base-composed plugins); stale "manifest-only" comments and two doc rows corrected.

Review round 6 → fixes (head e9852390)

A three-model pass (Claude, Grok, Codex) over the fold commits. Fixed:

• Claude marker (Claude, HIGH): Claude Code's own manifest is .claude-plugin/plugin.json; root plugin.json is fullsend's historical marker. Both now mark a Claude plugin, so a plugin in Claude Code's layout is neither refused nor read as a pi extension. ADR/docs say which marker is whose (Codex discovers .codex-plugin, .claude-plugin and .cursor-plugin manifests).
• Claude-kind injection scan (Claude, MEDIUM): the whole tree is scanned (commands/, agents/, skills/, hooks/, .mcp.json), not just two manifest files; symlinks are skipped for that kind rather than refused. Unknown kind → scan error (Grok).
• fullsend lock skipped the on-disk plugin checks (Codex + Claude, MEDIUM): the checks are now Harness.ValidatePluginDirs(), run by ValidateFilesExist and by lock after resolution, and they re-check duplicate sandbox basenames across every resolved entry, URL-sourced ones included (Grok, Claude).
• Legacy lock key (Claude, MEDIUM): base-composed plugin fetches fall back to the old <dir>/plugin.json index key, so an existing offline cache keeps working until the next online fullsend lock.
• Same tree, different options (Codex, MEDIUM / Claude, LOW): two URL spellings resolving to one cached tree with different env/pi are a resolve error, not a silent drop; lock replay warns.
• DummyPlaybackRuntime names and skips plugins like the other runtimes (Grok); phantom extensions references in a merge table, comments and test names retired (Grok, Codex); unused PluginPaths dropped (Claude).

Not changed, with reasons: pi.extensions negation-pattern evaluation and the ≥1 MiB / binary scan skips are pre-fold design already settled in rounds 2–4; the cross-kind same-basename over-rejection at Validate() is a deliberate over-approximation (documented as "no duplicate basenames across entries"); the repeated tree walks per run are a known cost, not a correctness issue.

Review round 5 → one plugins: key (head dfe61101)

Per the review discussion, the separate extensions: key is gone. The runtime is chosen by org/per-repo config, not the harness, so one list has to serve every runtime:

• internal/pluginformat (new leaf package): Detect/DetectTree classify a directory — plugin.json at the root → Claude plugin (checked first, so a Claude plugin that bundles a Node MCP server with a package.json is not misread), otherwise pi's loader rule (moved here from harness/extension_spec.go) → pi extension; neither → validation error.
• Harness: plugins: []PluginSpec, entries string | {path, env, pi: {args}}. Syntax checks stay in Validate(); the format verdict and the kind-gated checks (env/pi: only on a pi-format entry, pi reserved names) run in ValidateFilesExist after URL entries are fetched, so pinned-URL plugins follow the skills: rule for both families. Duplicate paths/basenames are a load error. Base composition uses one fetch kind for both (lock key <dir>/ instead of <dir>/plugin.json; old lock entries still map, an offline run against an old lock needs fullsend lock once).
• Runtime contract: BootstrapInput.Plugins() []PluginInput (name, path, Kind, env, pi args) replaces PluginDirs() + Extensions(). Claude Code loads its kind through the unchanged marketplace path and prints Plugin "<name>": skipped — the Claude Code runtime does not load pi extensions; pi loads its kind through the unchanged upload/preflight/-e path and prints Plugin "<name>": skipped — pi does not support Claude plugins. Injection scan is per kind (manifest files vs whole tree).
• Docs/ADR: ADR 0094 rewritten as "Plugins are runtime-scoped harness resources" (one key, detection per directory, namespaced runtime options, skills: sourcing, no polyglot directories, directories only — single-file -e entries are a follow-up); harness reference, runtimes matrix, pi.md "Plugins (pi extensions)", runtime-implementation notes.

Fleet check: the only plugins: entry in fullsend-ai/agents (plugins/gopls-lsp) has plugin.json and is unaffected.

Review round 1 → fixes (head 0d0bfb8d)

Claude + Grok found one HIGH (symlinks/directories invisible to the tree hash — an agent-added index.js symlink or an empty skills/ directory hijacked or disabled the extension with the hash unchanged) and the MEDIUMs listed in the review; all addressed: symlinks rejected at validation and by the sandbox guard, directories folded into the hash (Go/shell equivalence re-tested), loadability rule now mirrors pi 0.84.4's resolveLocalExtensionSource/collectPackageResources (package-layout dirs and package.json main handled; pi exits 1 Failed to load extension on a broken one — docs corrected), allowlist bypass removed, env denylist widened to PATH/HOME/LD_*/PYTHON*/NODE_*/SSL_*/*_API_KEY/*_PROXY/PI_*/provider prefixes, args restricted to --name[=value] forms excluding pi's own options and values not starting with -/@, dummy runtime warns-and-skips, \r names refused, scan size caps + sentinel error, duplicate basenames caught at validation, slash-key normalisation, ADR trimmed and reworded (the deny-list, not export order, is the control), threat-model pointer.

Review round 2 → fixes (head 529410d3)

Round 2 found a second HIGH that predates this PR but that the PR's guarantee depends on: pi loads every -e module through jiti, whose transpile cache is /tmp/jiti in the sandbox (pi is root-installed, so jiti falls back to os.tmpdir()), writable by the sandbox user and persistent across iterations; jiti validates only a /* v9-<hash(source)> */\n trailer, so a poisoned cache body runs while the source — and the hook adapter — hash unchanged. Fixed by exporting JITI_FS_CACHE=false from PiRuntime.EnvExports() (re-exported after .env is sourced) and reserving JITI_* in harness env:; internal/runtime/testdata/pi/jiti-cache-check.sh reproduces the bypass and proves the fix against the pinned pi (a PI_VERSION-bump re-check, like regen.sh). Also from round 2: the loadability rule now follows collectPackageResources exactly (a package.json with a pi object is judged only by pi.extensions; entries validated and confined to the directory), the args grammar matches pi's parser (one value per --flag, none after --flag=value, -/@ refused on both sides), one shared non-regular-entry rule for validation, scan and hash, size-skipped files count toward the scan cap, extra env names reserved (TIRITH_*, IFS, HOSTALIASES, OPENSSL_CONF, *_CA_BUNDLE, GOPROXY/GOFLAGS, CLOUDSDK_*, GIT_SSL_*/GIT_CONFIG*), docs reconciled.

Review round 3 → fixes (head 774cb97b)

Round 3 closed the rest of the loader-environment class: the bundled cli.js creates jiti without an explicit alias, so a .env-exported JITI_ALIAS swapped the hook adapter/extension modules with every hash intact (reproduced), and NODE_OPTIONS=--import=… ran code before any extension on the Vertex path (the unset existed only on the OpenAI branch). The run command now unsets NODE_OPTIONS NODE_PATH and the whole JITI_* family jiti reads (18 names, extracted into jiti-cache-check.sh from the Go source so they cannot drift) right after .env on every provider path, then re-exports JITI_FS_CACHE=false. Also: env deny-list now covers GIT_*, SSLKEYLOGFILE, CDPATH, PROMPT_COMMAND, JAVA_TOOL_OPTIONS, RUBYOPT, PERL5OPT with a drift test against sandbox.reservedCredentialKeys; glob/! pi.extensions entries are matched against the tree instead of accepted blindly; BOM-prefixed package.json, resource-named files, and nested-manifest escapes are handled; reserved sandbox extension names fail at harness validation; docs (architecture.md org-mode wording, harness-fields/harness-reference "top level only", the args sentence).

Review round 4 (verification) → one fix (head 79a47c6f, rebased onto #6752 / main)

Every round-3 item confirmed fixed (the loader-env unset reproduced-and-blocked in the sandbox image, including a .env that tries to redefine unset — dash rejects it). One correctness nit fixed: pi treats an entry as a glob only when it contains * or ? (hasGlobPattern) and expands braces through globSync, so the mirror now uses the same predicate — a bracket-only entry such as [mn]ain.js is a literal path (rejected when absent, as pi would load nothing) and a brace pattern with */? is accepted unevaluated rather than wrongly refused.

Test plan

• go test ./internal/harness/... ./internal/cli/... ./internal/runtime/... ./internal/security/... — new: extension spec unmarshal/validation, base resolution, bootstrap scan of extension trees, TestPiBootstrap_Extensions, run-command goldens (preflight, -e order, args quoting, env order), Go-vs-shell tree-hash equivalence, Claude warn-and-skip. Pre-existing failures only: TestDummyRuntime_* (local OpenShell gateway) and TestResolveAgentSource_OverrideOnlyEntryUsesAgentsRepoFallback (macOS /var vs /private/var).
• node --test internal/runtime/pi_extension/*.test.mjs — 21 pass (extension-tool allowlist branch, roster line)
• make lint clean
• Local fullsend run --runtime pi (this branch's CLI, fullsend-code image from fix(pi): activate grep/find/ls and ship rg + fd in the sandbox image #6752) with a scratch harness declaring extensions: [extensions/hello] (pre-round-5 key; now plugins: [extensions/hello]) (a 12-line ESM extension registering a hello tool, no tools: frontmatter): Extension "hello": uploaded to sandbox, roster line ends extensions=hello, [fullsend-hooks] extension tool: hello on first use, the tool returned HELLO fullsend FROM EXTENSION and the agent finished (haiku, $0.01). In the kept sandbox the find | sort | sha256sum pipeline reproduced the manifest's host-computed hash exactly, and appending one line to index.js changed it (the preflight's exit-96 condition).

Design origin: research on pi extensions for the fleet (audited pi-fff, pi-subagents, workflow engines, pi-web-access, pi-background-tasks, pi-lens — none fleet-default; this key is the opt-in mechanism). Refs #6464, #6550, #6527.

[qodo-code-review]

PR Summary by Qodo

Add secure harness-managed extensions for the pi runtime

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add harness extensions: entries with flags, environment, composition, and loadability
 validation.
• Secure remote resolution, injection scanning, sandbox upload, and host-backed integrity preflight.
• Integrate extension tools with pi hooks, progress output, tests, and documentation.

Diagram

graph TD
  A["Harness config"] --> B["Validate and resolve"] --> C["Scan and bootstrap"] --> D["Sandbox extension"] --> E{"Tree hash valid"}
  E -->|Yes| F["Pi runtime"] --> G["Hook adapter"]
  E -->|No| H["Exit 96"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reuse the plugins field

• ➕ Avoids adding another top-level harness key.
• ➕ Reuses existing directory-fetch concepts.
• ➖ Claude marketplace plugins and pi extensions have incompatible layouts.
• ➖ One field would silently change meaning between runtimes.

2. Use pi package sources

• ➕ Uses pi's native npm and git extension installation model.
• ➕ Reduces custom upload logic in the runner.
• ➖ Requires startup network access unavailable in the sandbox.
• ➖ Reopens the trusted-code set beyond explicit vetted paths.

3. Require per-tool manifests

• ➕ Makes every extension tool explicit for policy and name mapping.
• ➕ Could support finer-grained cross-runtime tool controls.
• ➖ Adds substantial author bookkeeping.
• ➖ Duplicates guarantees provided by --no-extensions plus explicit -e paths.

Recommendation: Keep the PR's harness-resource approach. It gives extensions the established trust, caching, and scanning model used by other harness assets while preserving a closed executable-code set and simple path-list UX; the alternatives either blur runtime semantics, require sandbox networking, or add redundant manifests.

Files changed (33) +2392 / -91

Enhancement (14) +1030 / -49

bootstrap_input.goCarry harness extensions into runtime bootstrap +22/-5

Carry harness extensions into runtime bootstrap

• Adds extension inputs to the CLI bootstrap adapter and maps resolved harness specs into the portable runtime contract.

internal/cli/bootstrap_input.go

bootstrap_scan.goScan all text files in extension trees +80/-1

Scan all text files in extension trees

• Extends runtime content scanning to extension directories, including vendored dependencies, while skipping binary files and honoring fail-open or fail-closed policy.

internal/cli/bootstrap_scan.go

run.goExpose and pass extensions during agent runs +4/-0

Expose and pass extensions during agent runs

• Prints configured extension paths and supplies the same resolved extension inputs to runtime execution that bootstrap receives.

internal/cli/run.go

compose.goCompose and securely fetch extension directories +157/-35

Compose and securely fetch extension directories

• Concatenates extension lists across bases and resolves URL-sourced harness extension directories. Generalizes plugin directory fetching into a kind-aware, allowlisted, content-addressed fetch path with extension-specific loadability validation.

internal/harness/compose.go

extension_spec.goDefine and validate harness extension specifications +345/-0

Define and validate harness extension specifications

• Introduces string and object YAML forms with path, argument, environment, reserved-name, and reserved-variable validation. Mirrors pi entry-point discovery for both local directories and fetched trees.

internal/harness/extension_spec.go

harness.goIntegrate extensions into the harness lifecycle +29/-0

Integrate extensions into the harness lifecycle

• Adds the 'extensions' field and wires it into semantic validation, relative path resolution, directory existence checks, and pi-compatible entry-point validation.

internal/harness/harness.go

bootstrap.goExtend the portable bootstrap contract +24/-0

Extend the portable bootstrap contract

• Adds extension inputs to 'BootstrapInput' and defines shared name, host path, argument, environment, and sandbox-name behavior.

internal/runtime/bootstrap.go

claude.goWarn when Claude Code skips pi extensions +9/-0

Warn when Claude Code skips pi extensions

• Names each configured pi extension and explicitly warns that Claude Code does not upload or load it.

internal/runtime/claude.go

pi_bootstrap.goUpload extensions and record pi manifest metadata +28/-2

Upload extensions and record pi manifest metadata

• Resolves and hashes extensions before sandbox changes, creates the extension directory, uploads each tree, and records paths, hashes, arguments, and environment in the manifest.

internal/runtime/pi_bootstrap.go

fullsend-hooks.jsRecognize and safely authorize extension tools +39/-1

Recognize and safely authorize extension tools

• Identifies tools from declared extensions, logs first use, bypasses only the identity allowlist for agents without explicit tools, and includes extension names in the session roster.

internal/runtime/pi_extension/fullsend-hooks.js

pi_extensions.goImplement pi extension hashing and run wiring +244/-0

Implement pi extension hashing and run wiring

• Adds extension resolution, collision checks, deterministic Go and shell tree hashes, fail-closed sandbox guards, ordered '-e' arguments, and deterministic environment exports.

internal/runtime/pi_extensions.go

pi_progress.goShow useful context for extension tools +13/-1

Show useful context for extension tools

• Falls back to conventional path, file, pattern, query, or command arguments for non-built-in tools while retaining redaction and display caps.

internal/runtime/pi_progress.go

pi_run.goPreflight and load declared pi extensions +31/-3

Preflight and load declared pi extensions

• Recomputes host hashes at run time, verifies sandbox trees before sourcing '.env', appends extension environment and ordered '-e' arguments, and reports exit 96 tampering failures.

internal/runtime/pi_run.go

runtime.goPass extensions through runtime run parameters +5/-1

Pass extensions through runtime run parameters

• Adds resolved extension inputs to 'RunParams' so pi can recompute trusted host hashes for every iteration.

internal/runtime/runtime.go

Tests (11) +1159 / -37

bootstrap_input_test.goTest extension propagation through bootstrap input +28/-0

Test extension propagation through bootstrap input

• Verifies paths, names, arguments, environment, security wrappers, and nil behavior when converting harness extensions.

internal/cli/bootstrap_input_test.go

bootstrap_scan_test.goTest extension injection scanning behavior +89/-5

Test extension injection scanning behavior

• Covers critical findings in nested dependencies, fail-open warnings, benign and binary content, empty entries, and unreadable directories.

internal/cli/bootstrap_scan_test.go

compose_extensions_test.goTest extension composition and remote caching +247/-0

Test extension composition and remote caching

• Covers base-child concatenation, fresh and offline cache fetches, allowlist and path validation, loadability checks, dependency recording, and source URL resolution.

internal/harness/compose_extensions_test.go

extension_spec_test.goTest extension schema and loadability rules +237/-0

Test extension schema and loadability rules

• Exercises YAML round trips, malformed entries, source and environment restrictions, relative resolution, entry-point discovery, missing directories, and helper behavior.

internal/harness/extension_spec_test.go

claude_test.goUpdate Claude bootstrap fixtures for extensions +7/-5

Update Claude bootstrap fixtures for extensions

• Extends the shared bootstrap test input to implement the new extension contract.

internal/runtime/claude_test.go

dummy_test.goUpdate dummy bootstrap fixture contract +6/-5

Update dummy bootstrap fixture contract

• Adds an empty extension implementation to the dummy runtime test bootstrap stub.

internal/runtime/dummy_test.go

pi_bootstrap_test.goUpdate pi bootstrap directory expectations +1/-1

Update pi bootstrap directory expectations

• Verifies bootstrap creates the new pi extensions directory alongside skills, sessions, and hooks.

internal/runtime/pi_bootstrap_test.go

fullsend-hooks.test.mjsTest extension-tool hook policy +93/-0

Test extension-tool hook policy

• Verifies selective allowlist bypass, continued pre/post hook execution, built-in exclusions, strict declared-tools behavior, missing-manifest behavior, one-time logging, and roster output.

internal/runtime/pi_extension/fullsend-hooks.test.mjs

pi_extensions_test.goTest extension integrity and runtime lifecycle +417/-0

Test extension integrity and runtime lifecycle

• Cross-tests Go and POSIX shell hashes, guard failures, collision handling, command ordering, bootstrap manifests, upload behavior, tamper errors, and Claude skip warnings.

internal/runtime/pi_extensions_test.go

pi_progress_test.goTest extension-tool progress summaries +18/-5

Test extension-tool progress summaries

• Covers conventional argument selection, truncation, secret redaction, absent context, and the rule that successful tool output never becomes the summary.

internal/runtime/pi_progress_test.go

pi_run_test.goAdapt pi command tests to extension inputs +16/-16

Adapt pi command tests to extension inputs

• Updates existing command-construction tests for the new resolved-extension parameter while preserving provider, security, tool, prompt, and quoting assertions.

internal/runtime/pi_run_test.go

Documentation (8) +203 / -5

0094-pi-extensions-are-harness-resources.mdRecord the harness-managed pi extension decision +116/-0

Record the harness-managed pi extension decision

• Adds ADR 0094 defining extension trust boundaries, configuration, integrity checks, load order, tool policy, rejected alternatives, and operational consequences.

docs/ADRs/0094-pi-extensions-are-harness-resources.md

architecture.mdAdd pi extensions to the implementation architecture +1/-0

Add pi extensions to the implementation architecture

• Documents harness extensions as trusted resources uploaded and loaded by pi after host-backed tree-hash verification.

docs/architecture.md

harness-fields.mdDocument extension portability and merge semantics +2/-0

Document extension portability and merge semantics

• Classifies 'extensions' as forge-agnostic harness content and specifies base-plus-child concatenation while preserving per-entry options.

docs/contributing/harness-fields.md

runtime-implementation.mdExtend runtime implementation guidance for extensions +2/-1

Extend runtime implementation guidance for extensions

• Documents the new bootstrap contract, non-pi warning requirement, integrity preflight, load ordering, environment handling, and extension-tool hook behavior.

docs/contributing/runtime-implementation.md

harness-reference.mdAdd the extensions harness field reference +11/-3

Add the extensions harness field reference

• Adds YAML examples, validation and trust constraints, runtime behavior, and composition semantics for string and object extension entries.

docs/reference/harness-reference.md

roadmap.mdLink pi extension support from the runtime roadmap +1/-1

Link pi extension support from the runtime roadmap

• Updates the pi runtime pilot entry to include harness-managed extensions and ADR 0094.

docs/roadmap.md

runtimes.mdCompare extension support across runtimes +1/-0

Compare extension support across runtimes

• Adds the 'extensions' compatibility row, noting Claude Code warnings and pi upload, preflight, and loading behavior.

docs/runtimes.md

pi.mdDocument pi extension configuration and operations +69/-0

Document pi extension configuration and operations

• Adds a comprehensive guide covering configuration, trust, upload and hash verification, load order, tool frontmatter, vendoring, and troubleshooting.

docs/runtimes/pi.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:42 PM UTC · Completed 5:01 PM UTC

Commit: 18f0e1e · View workflow run →

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

[github-actions]

Site preview

Preview: https://bae257f9-site.fullsend-ai.workers.dev

Commit: 179d42a58554b290e2ed61637a4d966c2c2a6e98

[qodo-code-review]

Code Review by Qodo

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

1. Symlinks bypass extension vetting ✓ Resolved 🐞 Bug ⛨ Security

Description

piExtensionTreeHash, the sandbox guard, and the content scanner omit symlinks even though
extension uploads preserve them, so a symlinked entry point or dependency can resolve to mutable,
unscanned code outside the vetted tree without changing the guarded hash. Because the agent can
write to pi's config tree, a pre-existing link can be retargeted between iterations and loaded by a
later -e invocation while preflight still reports the extension intact instead of exiting with
code 96.

Code

internal/runtime/pi_extensions.go[R137-138]

+		if !d.Type().IsRegular() {
+			return nil

Relevance

●●● Strong

This is a concrete integrity-boundary bypass involving symlinks; security hardening findings in
runtime code are typically accepted.

PR-#1780

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The host hashing implementation returns early for non-regular directory entries, and the sandbox
guard uses find -type f, so both hashes exclude symlinks and compare only the same incomplete
view. The upload path preserves directory contents including symlinks, while the scanner reads only
regular DirEntry children; combined with the documented agent write access to pi's config tree and
subsequent -e loading, this allows a symlink target to cross neither scanning nor integrity
boundaries before execution.

internal/runtime/pi_extensions.go[127-160]
internal/runtime/pi_extensions.go[176-206]
internal/cli/bootstrap_scan.go[62-84]
internal/sandbox/sandbox.go[1413-1425]
internal/runtime/pi_run.go[327-333]
internal/runtime/pi_extensions.go[108-114]
internal/runtime/pi_extensions.go[133-138]
internal/runtime/pi_extensions.go[188-208]
internal/runtime/pi.go[68-74]

Agent prompt

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

## Issue description
Extension uploads preserve child symlinks, but host and sandbox hashing and injection scanning ignore them. A symlinked entry point or dependency can therefore be retargeted to mutable, agent-writable code outside the vetted extension tree without changing the expected or actual preflight hash.

## Issue Context
Apply one consistent symlink policy across harness validation, scanning, hashing, upload, and sandbox guard generation. Prefer rejecting symlinks anywhere in declared extension trees; alternatively, permit only targets contained within the extension root, ensure host and sandbox hashing include equivalent link paths, targets, and verified content, scan resolved content, and upload a materialized copy so runtime resolution cannot escape the verified tree.

## Fix Focus Areas
- internal/runtime/pi_extensions.go[108-166]
- internal/runtime/pi_extensions.go[176-208]
- internal/cli/bootstrap_scan.go[62-84]
- internal/sandbox/sandbox.go[1413-1425]
- internal/harness/extension_spec.go[307-338]
- internal/runtime/pi.go[68-74]

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

---

2. Extension fetch bypasses SSRF ✗ Dismissed 🐞 Bug ⛨ Security

Description

A declared extension tool with a non-Claude name such as fetch is loaded, but its name is left
unchanged when selecting hook groups, so it never matches the SSRF hook's WebFetch matcher.
Consequently a harness-provided network/fetch extension can receive model-controlled URLs without
ssrf_pretool.py running, despite sandbox security being enabled.

Code

internal/runtime/pi_run.go[R330-333]

+	// Declared extensions come after the hook adapter: pi runs tool_call
+	// handlers in -e order and the first block wins, so the adapter's
+	// PreToolUse hooks see every call before any declared extension does.
+	parts = append(parts, piExtensionArgs(exts)...)

Relevance

●●● Strong

This is a concrete SSRF protection bypass in newly added extension execution; security behavior
defects are typically accepted.

PR-#6536

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new command construction loads every declared extension with -e. The hook adapter preserves
names outside its built-in mapping and matches groups only when that preserved name occurs in the
group's Claude-name list; the security plan binds SSRF protection exclusively to Bash and
WebFetch.

internal/runtime/pi_run.go[327-333]
internal/runtime/pi_extension/fullsend-hooks.js[33-38]
internal/runtime/pi_extension/fullsend-hooks.js[135-138]
internal/security/hooks.go[127-132]

Agent prompt

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

## Issue description
Declared pi extension tools use their pi-native names, while tool-specific sandbox hooks are selected using Claude tool names. As a result, a fetch-capable extension tool does not run the SSRF pre-tool hook unless it happens to be named `WebFetch`.

## Issue Context
`HookPlan` registers `ssrf_pretool.py` only for `Bash` and `WebFetch`, while the pi adapter selects groups with the unmapped extension tool name. Add explicit extension-tool hook metadata/mapping, or use a fail-closed routing policy for extension tools that can perform network actions, so fetch-capable extensions are covered by SSRF validation.

## Fix Focus Areas
- internal/runtime/pi_run.go[330-333]
- internal/runtime/pi_extension/fullsend-hooks.js[33-38]
- internal/security/hooks.go[127-132]

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

---

3. Go changes lack test gate ✗ Dismissed 📘 Rule violation ▣ Testability

Description

This PR modifies internal/ code, but no repository workflow or hook gates merges on `go test
./... or make go-test`. As a result, these runtime contract changes can merge without the required
Go unit-test enforcement.

Code

internal/runtime/bootstrap.go[R20-22]

+	// Extensions returns the harness's declared pi extensions (ADR 0094).
+	// Only the pi runtime loads them; other runtimes warn and skip.
+	Extensions() []ExtensionInput

Relevance

●● Moderate

No close gate precedent found; repository may intentionally rely on CI checks outside workflows,
making enforcement expectation uncertain.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1062046 applies because the PR adds the Extensions() method to code under internal/. The
Makefile defines go-test, but repository workflow searches show no workflow or pre-commit gate
invoking either make go-test or go test ./...; the existing E2E workflow only invokes `make
e2e-test`.

Rule 1062046: Require Go unit tests to pass before committing changes in cmd/ or internal/
internal/runtime/bootstrap.go[20-22]
.github/workflows/e2e.yml[160-175]

Agent prompt

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

## Issue description
Changes under `internal/` are not gated by an automatically enforced Go unit-test job.

## Issue Context
PR Compliance ID 1062046 requires `go test ./...` or `make go-test` to run and succeed before changes under `cmd/` or `internal/` can merge. Add the command to an applicable required CI workflow without suppressing failures.

## Fix Focus Areas
- .github/workflows/e2e.yml[160-175]

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

---

4. ADR exceeds line limit 📜 Skill insight ⚙ Maintainability

Description

The new ADR has 104 substantive lines after its frontmatter, exceeding the 100-line maximum. Its
final four lines are entirely beyond the allowed content limit.

Code

docs/ADRs/0094-pi-extensions-are-harness-resources.md[R113-116]

+  and PostToolUse scripts see Claude vocabulary for extension tools; the
+  Track E sub-agent tool (#6527); `--tools` union with extension tools.
+  Per-agent runtime selection remains
+  [ADR 0091](0091-per-agent-runtime-model-effort.md).

Relevance

● Weak

A close ADR line-limit precedent rejected shortening an overlong ADR; this team has declined the
same maintainability rule.

PR-#2582

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1062092 limits ADR content to 100 lines excluding frontmatter. In the newly added file,
frontmatter ends at line 11 and non-frontmatter content spans lines 13-116, so lines 113-116 exceed
the limit.

docs/ADRs/0094-pi-extensions-are-harness-resources.md[13-116]
Skill: writing-adrs

Agent prompt

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

## Issue description
ADR 0094 exceeds the maximum of 100 content lines after frontmatter.

## Issue Context
The frontmatter ends at line 11 and substantive content runs from line 13 through line 116, totaling 104 lines. Condense repeated detail or move evolving implementation detail into the appropriate living documentation while preserving the decision and consequences.

## Fix Focus Areas
- docs/ADRs/0094-pi-extensions-are-harness-resources.md[13-116]

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

---

Context sources

✅ Compliance rules (platform): 62 rules

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

Review mode: 🧠 Deep: This is a large, security-sensitive cross-cutting runtime and harness change with substantial new logic across validation, fetching, scanning, hashing, sandbox bootstrap/preflight, command construction, hooks, and multiple runtimes, creating many independent defect opportunities.

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

Risk Assessment: elevated (3/5)

Details

Large feature addition (41 files, ~4486 lines). High blast radius (change size composite = 5) mitigated by no protected paths, no security-sensitive files, strong test coverage (32% test ratio), and 7 of 41 files being new/additive. Tier 2 highlights compose.go (24 commits/30d, 7 authors/90d, 32 fix/revert commits), harness.go (23 commits/30d, 10 authors/90d, 26 fix/revert commits), and run.go (85 commits/30d, 14 authors/90d, 189 fix/revert commits) as high-churn hotspots. Preserving prior score of 3 per anchoring rules: Tier 1 signals are unchanged from prior assessment, and Tier 2/3 confirm the same hotspot and feature-driven patterns without introducing new reasons to deviate.

Previous run

Risk Assessment: elevated (3/5)

Details

Large feature addition (41 files, ~4484 lines). High blast radius (change size composite = 5) mitigated by no protected paths, no security-sensitive files, strong test coverage (32% test ratio), and predominantly additive new files. Tier 2 highlights compose.go as a high-churn hotspot (24 commits/30d, 7 authors/90d, 32 fix/revert commits) and harness.go (23 commits/30d, 10 authors/90d, 15 fix/revert commits). Preserving prior score of 3 per anchoring rules since Tier 1 signals are unchanged and Tier 2/3 provide no specific reason to deviate.

Previous run (2)

Risk Assessment: elevated (3/5)

Details

Large feature addition (41 files, ~4394 lines). High blast radius (change size composite = 5) mitigated by no protected paths, no security-sensitive files, strong test coverage (32% test ratio), and predominantly additive new files. Tier 2 highlights compose.go as a high-churn hotspot (24 commits/30d, 7 authors/90d). Preserving prior score of 3 per anchoring rules since Tier 1 signals are unchanged.

Previous run (3)

Risk Assessment: elevated (3/5)

Details

Large feature addition (40 files, ~3792 lines) introducing pi extension harness resources across runtime, harness, CLI, and documentation layers. Tier 1 signals are materially unchanged from prior assessment (files 35 to 40, lines 3034 to 3792, same scoring bands). High blast radius (change size composite = 5) mitigated by no protected paths, no security-sensitive files, strong test coverage (30% test ratio), and predominantly additive new files. Tier 2 highlights compose.go as a high-churn hotspot (24 commits/30d, 7 authors/90d). Preserving prior score of 3 per anchoring rules since Tier 1 signals are unchanged.

Previous run (4)

Risk Assessment: elevated (3/5)

Details

Large feature addition (35 files, ~3034 lines) introducing pi extension harness resources across runtime, harness, CLI, and documentation layers. High blast radius (change size composite = 5) mitigated by strong test coverage (34% test ratio), no protected paths, no security-sensitive files, and additive-only changes with majority new files. Re-review anchoring preserves prior elevated score as Tier 1 signals are largely unchanged from prior assessment.

Previous run (5)

Risk Assessment: elevated (3/5)

Details

Large feature addition (32 files, ~2477 lines) introducing pi extension harness resources across runtime, harness, CLI, and documentation layers. High blast radius mitigated by strong test coverage (36% test ratio), thorough ADR, and no protected paths.

[fullsend-ai-review]

Review

Findings

Low

• [pattern-inconsistency] internal/runtime/pi_extensions.go:102 — cloneStringMap duplicates maps.Clone from the standard library. The same package already uses maps.Clone in pi_bootstrap.go for toolNames. Both preserve nil input as nil output.
  Remediation: Replace cloneStringMap(in.Env) with maps.Clone(in.Env) and remove the cloneStringMap function.

• [naming-inconsistency] internal/runtime/pi_extensions.go:56 — The piReservedExtensionNames package-level variable shadows the exported harness.PiReservedExtensionNames it is assigned from. It is only used as an argument to duplicateDestinationNameError. Other cross-package references in this file use the qualified name directly.
  Remediation: Use harness.PiReservedExtensionNames directly at the call site and remove the package-level alias, matching the pattern used for other harness symbols in the same file.

• [stale-doc] docs/glossary.md:35 — The "Base Composition" glossary entry lists the concatenated fields as plugins, providers, openshell.profiles, and api_servers but omits extensions, which this PR adds with the same concatenation merge semantics.
  Remediation: Add extensions to the list of concatenated fields in the glossary entry.

• [non-deterministic iteration] internal/harness/extension_spec.go:363 — Map iteration in validateExtensions over e.Env makes error messages non-deterministic when an extension has multiple invalid env keys. Only the first error is returned, but which key surfaces first depends on map iteration order.

• [edge-case] internal/harness/extension_spec.go:598 — extensionGlobMatches accepts patterns containing ** or {} unconditionally (returns true without evaluation), and also returns true on any path.Match parse error. Deliberately accepting direction — safe because tree hash protects runtime.

• [test-fragility] internal/cli/bootstrap_scan_test.go — TestScanRuntimeContent_ExtensionScanBounds mutates the package-level maxExtensionScanFiles variable without synchronization. Safe because test is not t.Parallel(), but fragile if parallel is added later.

• [interface-change] internal/runtime/bootstrap.go:20 — BootstrapInput interface gains Extensions() []ExtensionInput. All implementations updated. internal/ package path limits blast radius to forks.

• [Injection Scan Completeness] internal/cli/bootstrap_scan.go — The maxExtensionScanFileBytes limit (1 MiB) means files larger than 1 MiB are skipped by the injection scanner. Mitigated by file count cap, tree-hash integrity check, and scan being heuristic not security boundary.

• [scope-creep] internal/runtime/pi_progress.go:131 — Adding a default: case that probes conventional argument names for extension tools in progress display is a minor scope extension beyond issue authorization. Small (10 lines) and a reasonable quality-of-life follow-through.

• [scope-creep] internal/runtime/pi_run.go — Loader environment hygiene (piLoaderEnvNames, piLoaderEnvUnset, JITI_FS_CACHE=false) goes beyond what linked issues explicitly authorize. However, ADR 0094 documents these as security prerequisites for the extension design.

---

Next steps:

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

Previous run

Review

Findings

Low

• [pattern-inconsistency] internal/runtime/pi_extensions.go:102 — cloneStringMap duplicates maps.Clone from the standard library. The same package already uses maps.Clone in pi_bootstrap.go for toolNames.
  Remediation: Replace cloneStringMap(in.Env) with maps.Clone(in.Env) and remove the cloneStringMap function.

• [stale-doc] docs/glossary.md:35 — The "Base Composition" glossary entry lists the concatenated fields as plugins, providers, openshell.profiles, and api_servers but omits extensions, which this PR adds with the same concatenation merge semantics. Other documentation (harness-reference.md, harness-fields.md, customizing-agents.md) has been updated.
  Remediation: Add extensions to the list of concatenated fields in the glossary entry.

• [maintenance hazard] internal/runtime/pi_extension/fullsend-hooks.js:55 — isExtensionTool uses negative-space classification: a tool is an extension tool if its name is NOT in the toolNames map and NOT in PI_BUILTIN_TOOLS_OUTSIDE_MAP. A future pi built-in added without updating either set would be misclassified in the transcript log. The function is explicitly informational only (decides logging, never hook dispatch).

• [non-deterministic iteration] internal/harness/extension_spec.go:363 — Map iteration in validateExtensions makes error messages non-deterministic when an extension has multiple invalid env keys. Validation is correct (all invalid keys are caught), but the first-reported key varies between runs.

• [edge-case] internal/harness/extension_spec.go:598 — extensionGlobMatches accepts ** and {} patterns without evaluation (returns true unconditionally). A pi.extensions entry like **/*.invalid that matches nothing at runtime is accepted. Deliberate trade-off favoring availability over precision, documented in docs/runtimes/pi.md.

• [test-fragility] internal/cli/bootstrap_scan_test.go:121 — TestScanRuntimeContent_ExtensionScanBounds mutates the package-level maxExtensionScanFiles variable. The test is correctly non-parallel (per comment), but fragile if a future parallel test calls scanExtensionDir.

• [interface-change] internal/runtime/bootstrap.go:20 — BootstrapInput interface gains Extensions() []ExtensionInput. All in-repo implementations are updated. The internal/ package path prevents external import, limiting blast radius to forks.

• [struct-change] internal/runtime/runtime.go:47 — RunParams struct gains Extensions []ExtensionInput field. All in-repo call sites use named fields. The internal/ path limits exposure.

• [struct-change] internal/harness/harness.go:329 — Harness struct gains Extensions []ExtensionSpec YAML field with omitempty. Additive, backward-compatible.

• [new-type] internal/runtime/bootstrap.go:27 — New exported type ExtensionInput in internal/runtime.

• [new-type] internal/harness/extension_spec.go:41 — New exported type ExtensionSpec in internal/harness with cross-package coupling via PiReservedExtensionNames.

• [manifest-schema] internal/runtime/pi_bootstrap.go:57 — piManifest struct gains Extensions field with json:"extensions,omitempty". Additive, backward-compatible. Used for roster logging only.

---

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

• [consumer completeness] docs/contributing/harness-fields.md:49 — The documentation states that extensions: under forge: or overlays: is "ignored, not an error," but the code provides no validation warning when a harness author places extensions: in an overlay. Since ForgeConfig has no Extensions field, YAML unmarshalling silently drops it. A harness author writing overlays: - when: '...'; extensions: [...] gets no feedback — the extensions simply do not load.
  Remediation: Add a check in overlay resolution or validation that warns when an extensions: key appears in an overlay entry, since the field is top-level only and is silently dropped.

Low

• [pattern-inconsistency] internal/runtime/pi_extensions.go:91 — cloneStringMap duplicates maps.Clone from the standard library. The same package already uses maps.Clone in pi_bootstrap.go for toolNames.
  Remediation: Replace cloneStringMap(in.Env) with maps.Clone(in.Env) and remove the cloneStringMap function.

• [stale-doc] docs/glossary.md:35 — The "Base Composition" glossary entry lists the concatenated fields as plugins, providers, openshell.profiles, and api_servers but omits extensions, which this PR adds with the same concatenation merge semantics. Other documentation (harness-reference.md, harness-fields.md, customizing-agents.md) has been updated.
  Remediation: Add extensions to the list of concatenated fields in the glossary entry.

• [maintenance hazard] internal/runtime/pi_extension/fullsend-hooks.js:55 — The isExtensionTool function uses negative-space classification: a tool is an extension tool if its name is NOT in the toolNames map and NOT in PI_BUILTIN_TOOLS_OUTSIDE_MAP. A future pi built-in added without updating either set would be misclassified in the transcript log. The function is explicitly informational only (decides logging, never hook dispatch).

• [toctou-window] internal/runtime/pi_run.go:1359 — Residual TOCTOU window between the extension tree-hash preflight and pi's import. A background process from a prior agent iteration could rewrite the extension tree between the check and pi's load. Documented explicitly, tracked in issue fix(runtime): terminate stray sandbox processes between iterations #6753, bounded by sandbox containment, and shared with the existing hook-adapter guard.

• [non-deterministic iteration] internal/harness/extension_spec.go:363 — Map iteration in validateExtensions makes error messages non-deterministic when an extension has multiple invalid env keys. Validation is correct (all invalid keys are caught), but the first-reported key varies between runs.

• [edge-case] internal/harness/extension_spec.go:598 — extensionGlobMatches accepts ** and {} patterns without evaluation (returns true unconditionally). A pi.extensions entry like **/*.invalid that matches nothing at runtime is accepted. Deliberate trade-off favoring availability over precision, documented in docs/runtimes/pi.md.

• [error-handling] internal/cli/bootstrap_scan.go:84 — In scanExtensionDir, the filepath.EvalSymlinks error at line 80 propagates correctly, but the control flow is non-obvious: err from EvalSymlinks is reused as the WalkDir condition and again at line 143.

• [test-fragility] internal/cli/bootstrap_scan_test.go:121 — TestScanRuntimeContent_ExtensionScanBounds mutates the package-level maxExtensionScanFiles variable. The test is correctly non-parallel (per comment), but fragile if a future parallel test calls scanExtensionDir.

• [interface-change] internal/runtime/bootstrap.go:20 — BootstrapInput interface gains Extensions() []ExtensionInput. All in-repo implementations are updated. The internal/ package path prevents external import, limiting blast radius to forks.

• [struct-change] internal/runtime/runtime.go:47 — RunParams struct gains Extensions []ExtensionInput field. All in-repo call sites use named fields. The internal/ path limits exposure.

• [struct-change] internal/harness/harness.go:30 — Harness struct gains Extensions []ExtensionSpec YAML field with omitempty. Additive, backward-compatible.

• [new-type] internal/runtime/bootstrap.go:27 — New exported type ExtensionInput in internal/runtime.

• [new-type] internal/harness/extension_spec.go:41 — New exported type ExtensionSpec in internal/harness with cross-package coupling via PiReservedExtensionNames.

• [manifest-schema] internal/runtime/pi_bootstrap.go:57 — piManifest struct gains Extensions field with json:"extensions,omitempty". Additive, backward-compatible. Used for roster logging only.

---

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

• [documentation correctness] docs/runtimes/pi.md:433 — The sentence in the "args and env" paragraph has a broken structure with mismatched parentheses and a duplicated verb. The text reads: "pi's own option names (--extension, --approve, --model, --tools, ...) are / --use-theme, --tui-mode, ...) are rejected". This has an extra closing parenthesis without a matching opening parenthesis, and the verb "are" appears twice.
  Remediation: Merge the lines into a single grammatical sentence, e.g.: "pi's own option names (--extension, --approve, --model, --tools, --use-theme, --tui-mode, ...) are rejected, and a value may not start with - or @ in either spelling."

Low

• [pattern-inconsistency] internal/runtime/pi_extensions.go:91 — cloneStringMap duplicates maps.Clone from the standard library. The same package already uses maps.Clone in pi_bootstrap.go for toolNames.
  Remediation: Replace cloneStringMap(in.Env) with maps.Clone(in.Env) and remove the cloneStringMap function.

• [stale-doc] docs/glossary.md:35 — The "Base Composition" glossary entry lists the concatenated fields as plugins, providers, openshell.profiles, and api_servers but omits extensions, which this PR adds with the same concatenation merge semantics.
  Remediation: Add extensions to the list of concatenated fields.

• [maintenance hazard] internal/runtime/pi_extension/fullsend-hooks.js:55 — The isExtensionTool function uses negative-space classification: a tool is an extension tool if its name is NOT in the toolNames map and NOT in PI_BUILTIN_TOOLS_OUTSIDE_MAP. A future pi built-in added without updating either set would still be misclassified in the transcript log. The function is explicitly informational only (decides logging, never hook dispatch).

---

Next steps:

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

Previous run (4)

Review

Findings

Low

• [Env deny-list coverage gap] internal/harness/extension_spec.go — The env deny-list covers credential-shaped suffixes (_API_KEY, _TOKEN, _SECRET*, _PROXY) and specific families (PI_*, FULLSEND_*, GOOGLE_*, etc.), but does not cover _PASSWORD or _PRIVATE_KEY suffixes. An extension env entry with a name like DOCKER_PASSWORD or GITHUB_APP_PRIVATE_KEY would pass validation. While extension env values come from the harness (org-controlled YAML, not attacker input), broadening the deny-list to cover these common credential shapes would be a defense-in-depth improvement.
  Remediation: Add "_PASSWORD" and "_PRIVATE_KEY" to reservedExtensionEnvSuffixes.

• [stale-doc] docs/contributing/runtime-implementation.md:269 — The fullsend-manifest.json annotation in the workspace layout diagram reads "Agent tools/allowlist, HookPlan, pi version" but the manifest now also carries an extensions array (informational for the hook adapter).
  Remediation: Append ", extensions" after "pi version".

• [stale-doc] docs/contributing/runtime-implementation.md:414 — The hardening levers paragraph says "only the explicit -e paths load (at most one vendored provider extension plus the hook adapter)". With declared harness extensions also loading via -e after the hook adapter, this parenthetical is now incomplete.
  Remediation: Update the parenthetical to "(the vendored provider extension, the hook adapter and any declared harness extensions)".

• [stale-doc] docs/contributing/runtime-implementation.md:374 — The pi runtime Mermaid flow diagram omits extension upload in Bootstrap, the tree-hash preflight (exit 96) in the guard node, and -e <declared extensions> in the pi command node. The prose added below the diagram documents this, but the diagram itself is stale.
  Remediation: Update the three Mermaid nodes: Bootstrap to mention extensions upload, the guard to add extension tree-hash check, and the pi command to add [-e <declared>...].

• [stale-doc] docs/architecture.md — The architecture Mermaid diagram for runtime: pi does not list extensions/ in the pi-config directory contents, does not mention the extension hash check in the guard node, and does not include -e <declared> entries in the pi command node.
  Remediation: Add extensions to the relevant Mermaid diagram nodes.

• [stale-doc] docs/contributing/runtime-implementation.md:44 — The host-side security Mermaid SCAN node text lists "context · agent def · skills · plugins" but not extensions. The table row below the diagram (line 82) IS updated by this PR to mention extension directories, but the Mermaid node itself is not.
  Remediation: Add "extensions" to the SCAN node text.

• [maintenance hazard] internal/runtime/pi_extension/fullsend-hooks.js:55 — The isExtensionTool function uses negative-space classification: a tool is an extension tool if its name is NOT in the toolNames map and NOT in PI_BUILTIN_TOOLS_OUTSIDE_MAP. The addition of PI_BUILTIN_TOOLS_OUTSIDE_MAP (currently only powershell) partially mitigates the prior review's finding, but a future pi built-in added without updating either set would still be misclassified in the transcript log. The function is explicitly informational only (decides logging, never hook dispatch).

• [Extension scan bypass via large file count] internal/cli/bootstrap_scan.go — Files over 1 MiB are skipped by the injection scanner with a stderr warning. An extension could theoretically distribute content across oversized files to avoid scanning. Risk is low: extensions are harness-repo content (org-allowlisted, content-addressed), the skip is observable, and the scan is heuristic rather than a security boundary.

• [pattern-inconsistency] internal/runtime/pi_extensions.go — cloneStringMap duplicates functionality that maps.Clone provides. The existing pi_bootstrap.go in the same package already imports the maps package for maps.Clone of toolNames.

• [iteration-order] internal/harness/extension_spec.go — Map iteration in validateExtensions makes error messages non-deterministic when an extension has multiple invalid env keys. The rest of the validation in this function and across the harness package uses indexed slices for deterministic errors.

---

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 (5)

Review

Findings

Medium

• [stale-doc] docs/contributing/runtime-implementation.md:263 — Sandbox workspace layout diagram under pi-config/ is missing the new extensions/<name>/ subdirectory that Bootstrap uploads declared harness extensions to. The PR adds this directory in pi_bootstrap.go but does not update the layout diagram.
  Remediation: Add a line like extensions/<name>/ Declared pi extensions (ADR 0094; tree-hash preflight each iteration) between skills/ and hooks/.

• [stale-doc] docs/contributing/runtime-implementation.md:414 — Hardening levers paragraph says "only the explicit -e paths load (at most one vendored provider extension plus the hook adapter)". With this PR, declared harness extensions also load via -e after the hook adapter. The parenthetical is now stale and incomplete.
  Remediation: Update the parenthetical to mention declared harness extensions alongside the vendored provider and hook adapter.

Low

• [tool-allowlist-bypass] internal/runtime/pi_extension/fullsend-hooks.js:241 — The isExtensionTool function uses negative-space classification: a tool is an extension tool if its name is NOT in the toolNames map (neither as key nor value). While the closure argument (--no-extensions + explicit -e) is sound, a future pi built-in added without a corresponding toolNames entry would be classified as an extension tool and bypass tool_allowlist_pretool.py for agents without tools: frontmatter. This is a maintenance hazard, not a current vulnerability.

• [pattern-inconsistency] internal/runtime/pi_extensions.go — cloneStringMap duplicates functionality that maps.Clone provides. The existing pi_bootstrap.go already imports the maps package for maps.Clone of toolNames, so this package already depends on it.
  Remediation: Replace cloneStringMap(in.Env) with maps.Clone(in.Env) and remove the cloneStringMap function.

• [stale-doc] docs/contributing/runtime-implementation.md:82 — Security feature matrix row "Host-side runtime content scan" describes scanning "agent def, SKILL.md, plugin JSON before upload" but scanRuntimeContent now also scans every text file in each declared pi extension directory (node_modules included).
  Remediation: Add "extension directories" to the parenthetical.

• [stale-doc] docs/contributing/runtime-implementation.md:269 — The fullsend-manifest.json description says "Agent tools/allowlist, HookPlan, pi version" but the manifest now also carries an extensions array (informational for the hook adapter).
  Remediation: Append "extensions" to the description.

---

Next steps:

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

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:11 PM UTC · Completed 6:30 PM UTC

Commit: 0d0bfb8 · View workflow run →

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

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:40 PM UTC

Commit: 529410d · View workflow run →

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

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 7:53 PM UTC · Ended 8:12 PM UTC

Commit: 774cb97 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 4:10 PM UTC · Completed 4:31 PM UTC

Commit: 179d42a · View workflow run →

Runtime: claude · Model: opus → claude-opus-5 · Effort: high · Cost: $16.62

Outdated: superseded by later heads (review rounds 5–8 applied); maintainer-approved by rh-hemartin.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 5:58 PM UTC · Completed 6:13 PM UTC

Commit: 179d42a · View workflow run →

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

[fullsend-ai-retro]

Retro: PR #6754 — pi extensions under one plugins: key

Overview: Human-authored feature PR by waynesun09 (53 files, 5,436 additions, 15 commits, Aug 29 – Sep 4). Major mid-lifecycle refactor in rounds 5–8 folded extensions into the plugins: key. Agents repo: fullsend-ai/agents@main.

Review activity: 14 review agent runs triggered by 11 force pushes. 6 completed successfully, 5 cancelled by subsequent pushes, 2 failed validation after 2 retry iterations, 1 failed at setup. Estimated total cost: $110–140. Models: claude-opus-4-6 (runs 1–13), claude-opus-5 (run 14).

Finding quality: ~40 comment instances across 10 review rounds containing ~21 unique findings. Zero findings led to code changes. The sole HIGH finding (SSRF hook bypass via extension tools) was a false positive — the defense-in-depth design is documented in ADR 0094 added in the same PR. 38% of unique findings (8/21) were stale, referencing code replaced during the PR's own rounds 5–8 refactor. Key duplicates: cloneStringMap finding raised 4× (rounds 3–6), isExtensionTool finding raised 4× (rounds 1–4).

Cascading no-op triggers: The review bot's COMMENTED and DISMISSED review submissions on Sep 4 fired 27 pull_request_review workflow runs in a 1-minute window, all routed to no-ops (~27 min wasted CI time).

Validation failures: Runs 13 and 14 both failed after 2 validation iterations, but run 14 still posted 22+ inline comments despite the failure — the workflow reports failure while comments remain visible on the PR.

All candidate proposals filtered — existing issues provide coverage

• Bot-authored event filtering (27 no-op runs): #1271, #2994, #4984, #5967 directly cover filtering bot-submitted pull_request_review events. New evidence: 27 cascading no-op runs from a single review round on this PR.
• Finding deduplication (4× repeats): #2959, #2816, #5760 track cross-iteration dedup. New evidence: same findings raised 4 times across successive rounds with no acknowledgment of prior findings.
• ADR-aware security review (HIGH false positive): agents#638 and agents#734 cover following ADR references during review. New evidence: the agent did not read ADR 0094 (added in-PR) before raising a security finding the ADR directly addresses.
• Security false-positive suppression: agents#632 covers suppressing findings when the agent's own analysis shows no exploitation vector. New evidence: the SSRF finding was self-retracted after the agent concluded the network boundary mitigates the concern.
• Overlapping bot findings: #4107 covers fullsend-ai-review and qodo-code-review overlap. New evidence: both bots independently flagged the same SSRF false positive on this PR.
• Metrics on validation failure: #6806 covers metrics zeroing on error exit. New evidence: run 14 posted inline comments but the workflow reported failure, which may confuse downstream processes.