Skip to content

feat(patterns): add pi-coding-agent acq-kit (earendil-works/pi, npm install, unprivileged startup-only) - #399

Merged
mogul merged 9 commits into
mainfrom
feat/pi-coding-agent-kit
Sep 23, 2026
Merged

mogul merged 9 commits into
mainfrom
feat/pi-coding-agent-kit

Conversation

@wz-gsa

@wz-gsa wz-gsa commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds a new acq mixin kit, pi-coding-agent, installing earendil-works/pi (MIT), npm package @earendil-works/pi-coding-agent — a plain terminal AI coding agent that grants the model only read/write/edit/bash tools by default.

Ships ahead of the in-flight prime-agent kit (#368) by design — see #398 for the full sequencing rationale.

Eligibility & Accountability

  • I am a federal employee or contractor (PRs merge with @GSA-TTS/agentic-coding-team code-owner approval; eligibility is attested here, not authenticated).
  • I understand this change and can explain it in my own words — I am the author of record and accountable for it.
  • By opening this PR, I attest I have the right to release this work under the repository's CC0-1.0 dedication.

AI Assistance Disclosure

  • AI tools were used in this contribution: OpenCode drafted the kit's spec.yaml, install script, README, TROUBLESHOOTING.md, docs/decisions/, and this PR body, including two adversarial-review rounds and the live-tested fixes below.
  • I verified all AI-generated content: no fabricated/hallucinated APIs, dependencies, citations, or placeholder content; the pinned npm package/version, engines.node requirement, and lifecycle-script absence were confirmed against real npm registry metadata, and every behavioral claim below was live-tested against a real sandbox, not assumed.
  • No inherited/reused third-party material beyond what's cited (openchamber's own CA-bundle and wrapper patterns, credited inline).

What it does

  • Startup phase only, no install phase — matches openchamber's settled unprivileged pattern.
  • Installs via the standard npm registry path (npm install -g --ignore-scripts --prefix "$HOME/.local", pinned to a specific reviewed version, --fetch-timeout/--fetch-retries=0 to bound registry-hang risk), deliberately not pi.dev/install.sh.
  • Explicit Node ≥22.19.0 preflight check.
  • CA-bundle handling adapted from openchamber's own script, rebuilt every boot; a thin pi wrapper carries that bundle into the user's own later invocation (see "Findings addressed" below — this is new since the last review round).
  • Egress allow-list is deliberately minimal: registry.npmjs.org only.
  • scripts/verify, TROUBLESHOOTING.md, and docs/decisions/ are now present, matching every sibling kit's mandatory structure (per acq-kits/README.md) — new since the last review round.
  • Design rationale (install-method choice, --ignore-scripts residual risk, prefix choice, CA-bundle wrapper) is now documented once, in docs/decisions/, rather than duplicated across spec.yaml, README.md, and the install script's own comments.

Findings addressed (from @basiliskus's review — thank you)

  1. pi was not reachable after install. Root cause: install prefix was $HOME/.npm-global, which is not on the guest's default PATH; the review correctly noted this is the same PATH-injection problem openchamber/paseo solve in their own scripts/verify, not the kit's install script. Fixed: switched the prefix to $HOME/.local, which is on the default guest PATH — confirmed live via a completely bare sh -c probe from a fresh, separate exec (no PATH massaging of any kind). See docs/decisions/local-prefix-not-npm-global.md.
  2. The runtime CA claim was false — an install-time export NODE_EXTRA_CA_CERTS never reaches a pi process a user launches later, in a separate shell. Fixed: pi now ships behind a thin wrapper at $HOME/.local/bin/pi that rebuilds the bundle and re-execs the real binary on every invocation, rebuilt on every boot (not just a fresh install) so it self-heals a sandbox that already had pi from before this fix. See docs/decisions/ca-bundle-wrapper-not-env-var.md. Caught and fixed two additional bugs of my own while implementing this fix, live: an idempotent-re-run regression (the wrapper-rebuild logic lost track of the real binary path once npm's symlink was replaced — fixed with a sidecar cache file) and a symlink-follow clobber bug (naively redirecting into the still-live npm symlink truncated the real binary itself — fixed by removing the symlink before writing the wrapper). Verified across 3 consecutive boots with no regressions.
  3. scripts/verify, TROUBLESHOOTING.md, docs/decisions/ were missing, despite being declared mandatory for every kit. Fixed: all three added, matching the sibling kits' conventions. scripts/verify's step 4 specifically probes command -v pi through a bare sh -c with zero PATH injection — the exact check the review said would have caught finding 1. Live-ran the full offline + RUN_ACQ=1 live suite: 7/7 live checks pass; the one offline failure (validate-kits.py tripping on a __pycache__ directory) is a pre-existing, unrelated repo issue, confirmed by the same run showing OK pi-coding-agent.
  4. Rationale was written out three times (spec.yaml, README, script). Fixed: consolidated into docs/decisions/ (two ADRs, matching the sibling kits' one-file-per-decision convention); spec.yaml/README.md/the script now cite it instead of restating it.
  5. A leaked CA-decode temp file. Fixed: single shared install log now serves both the CA-decode-failure path and the npm-install-failure path; confirmed live — no leftover ca-decode.*.err files after any boot.

Verification

  • validate-kits.py --strict — clean for pi-coding-agent (the one failure in the run is __pycache__, a pre-existing unrelated repo issue)
  • sh -n + shellcheck --shell=sh + scripts/scan_unsafe_shell.py — all clean, 0 findings
  • markdownlint-cli2 — 0 issues across README/TROUBLESHOOTING/docs/decisions
  • Live-tested on msb, this round specifically: fresh install, 3 consecutive idempotent re-runs (the regression class above), a fresh separate-shell pi --version invocation, wrapper content inspection (confirms the CA env var and real-binary path are both correct), and the full scripts/verify live suite (7/7 pass)

Co-authored-by: OpenCode Agent agent@gsa.gov

Installs earendil-works/pi (@earendil-works/pi-coding-agent) via the
standard npm registry path, unprivileged, startup-phase-only — no
install phase, matching openchamber's settled pattern rather than
prime-agent's still-in-flight root-install-phase skeleton.

Ships ahead of the in-flight prime-agent kit (#368)
by design: a completed consensus vote picked pi specifically to validate
the unprivileged/agent-owned/startup-only install pattern on a
materially simpler target (no native-module postinstall, no Python/
IPython kernel bootstrap) before returning to prime-agent's heavier
complexity.

Deliberately avoids pi.dev/install.sh (a mutable live script with an
interactive Node/npm preflight path) in favor of the same underlying
npm command, pinned to a specific reviewed version rather than
"latest". Model-provider config (USAi models.json) is deliberately
deferred — an earlier draft shipped a functionless empty-array
placeholder with no emitter to populate it; cut rather than shipped as
scaffolding, to land later with a real pi.mjs emitter.

Verified live on msb (fresh install, idempotent re-run, malicious
version-string rejection, old-Node rejection, CA-decode-failure
logging, registry-timeout bounding) and via validate-kits.py --strict,
scan_unsafe_shell.py, and shellcheck.

Co-authored-by: OpenCode Agent <agent@gsa.gov>
@wz-gsa
wz-gsa requested a review from a team as a code owner August 28, 2026 19:07
… kit

- Register pi-coding-agent (and the previously-missing prime-agent) in
  acq-kits/README.md's top-level kit table — it was registered in
  kits.yaml but never listed in the human-facing index.
- Use mktemp for the CA-decode-failure error log, matching the install
  log's own collision-avoidance in the same script (was a hardcoded
  /tmp path; not exploitable in this kit's single-tenant threat model,
  but an unjustified inconsistency within one script).
- Tighten the --ignore-scripts residual-risk comments in spec.yaml and
  the install script: verified every transitive dependency individually
  (not just the top-level package) has no lifecycle script, and note
  that --ignore-scripts blocks lifecycle hooks only, not npm's ordinary
  bin-linking step.

Found via two parallel adversarial review passes against PR #399 (one
targeting schema/repo-convention drift, one targeting the shell script
itself) that assumed every existing claim was wrong until independently
re-verified. All fixes re-verified live on msb (fresh install, idempotent
re-run, CA-decode-failure now showing the mktemp'd filename) and via
validate-kits.py --strict, scan_unsafe_shell.py, shellcheck, and
markdownlint-cli2.

Co-authored-by: OpenCode Agent <agent@gsa.gov>
@basiliskus

Copy link
Copy Markdown
Contributor

Review

Five findings, most severe first. The supply-chain claims in the PR body check out: pi-coding-agent@0.84.3 exists on npm, engines.node >=22.19.0, and the package has no preinstall/postinstall scripts.

1. pi is not on PATH after install

files/home/pi-coding-agent-install.sh:98 installs into $HOME/.npm-global/bin, but the PATH prepend on lines 99-100 is local to the startup script's process. The PR's reviewer notes say the vestigial-content pass cut "non-load-bearing PATH logic". This looks like the piece that was load-bearing: without it the installed binary is unreachable from any shell the user gets. Nothing in the image (Dockerfile ENV PATH, .bashrc) or the spec adds ~/.npm-global/bin, and unlike openchamber this kit drops no wrapper in ~/.local/bin. So the agentContext's "pi is a TUI you run directly (pi)" does not work: acq exec <sbx> -- pi or typing pi in the agent's shell returns sh: pi: not found, even though the startup log says "pi is installed and on PATH". openchamber's script documents this exact limitation and works around it with a ~/.local/bin wrapper; paseo's verify has to inject $HOME/.npm-global/bin. Simplest fix: --prefix "$HOME/.local" (pi's own installer's fallback prefix) so the bin link lands in ~/.local/bin, which is already first on the image PATH.

2. The runtime CA claim is not true

pi-coding-agent-install.sh:105, README.md:121, and spec.yaml:82 ("load-bearing, not defensive") all say the CA bundle serves pi's own LLM-provider calls at runtime. But NODE_EXTRA_CA_CERTS is exported only inside this script's process, and only on the boot where pi is absent. Nothing persists it for the pi process the user launches later. Behind the Zscaler-inspecting proxy the PR describes, first boot installs fine, then the user runs pi and its first HTTPS call fails with UNABLE_TO_GET_ISSUER_CERT_LOCALLY / SELF_SIGNED_CERT_IN_CHAIN, the exact failure the README says this block prevents. openchamber gets away with the same pattern only because its supervised server is a child of the startup script. Either drop the runtime claim from all three docs, or make it true: a ~/.local/bin/pi wrapper that exports NODE_EXTRA_CA_CERTS=<bundle> and execs the real binary would fix this and finding 1 together (the bundle would then need rebuilding on every boot, not only when pi is missing).

3. Missing scripts/verify, TROUBLESHOOTING.md, and docs/decisions/

acq-kits/README.md:28-30 declares these mandatory for every kit: "Each kit is self-contained: a spec.yaml, any files/ payload, a scripts/verify host-side check, a README.md (with a backend parity note), a TROUBLESHOOTING.md, and docs/decisions/ records." The PR body's justification ("consistent with sibling shell-only kits zscaler-ca-certificate/git-ssh-sign which also have no tests/ dir") conflates tests/ with scripts/verify. Both cited siblings ship scripts/verify and TROUBLESHOOTING.md; only the explicitly-skeleton prime-agent lacks them. The live-tested behaviours (idempotent re-run, bad-version rejection, old-Node rejection, CA-decode logging) exist only in the PR description, so a regression in any of them goes undetected. A verify that probes command -v pi through a bare sh -c would have caught finding 1. validate-kits.py only checks README.md, so make validate-kits passing does not cover this.

4. Rationale prose is written out three times

The install-method choice, the pi.dev/GitHub egress omissions, the --ignore-scripts residual-risk note, the version-pin policy, the deferred models.json, and the naming rationale each appear in the spec.yaml header (~125 lines), README.md, and the script header; the script is roughly 60% comment. The next change (bumping the pin, adding pi.dev, wiring models.json) has to be edited consistently in three places, and the PR notes stale "ported verbatim" comments already bit once. Keep the rationale once, ideally in the docs/decisions/ record finding 3 asks for, and have the spec and script headers point at it.

5. Leaked CA-decode temp file

pi-coding-agent-install.sh:122 creates a second mktemp file (pi-coding-agent-ca-decode.XXXXXX.err) on every install-needed boot whenever PROXY_CA_CERT_B64 is set, and never removes it; on success it is an empty leftover in /tmp (plus the $$-named fallback path on mktemp failure). The install log $_log is created 40 lines later and could simply receive base64's stderr. Moving the _log mktemp above the CA block and using 2>>"$_log" for the decode makes the error message point at the one log the user is already told to read, and removes the fallback branch, the extra variable, and the leak.

Template checklists

The PR body skips the template's Eligibility and Accountability and AI Assistance Disclosure sections, which the template marks required for all PRs. AI use is visible only through the Co-authored-by: OpenCode Agent trailer. Please add the checkboxes so the attestation is on record.

No findings

Removed-behaviour audit (only the prime-agent parity issue-number references in kits.yaml were deleted, cosmetic), cross-file callers (new files; the validate-kits.py registry cross-check is satisfied), the set -eu / &&-list / case glob logic in the script, --prefix flag vs NPM_CONFIG_PREFIX precedence, and the --fetch-timeout / --fetch-retries=0 npm flags.

wz-gsa and others added 4 commits September 3, 2026 15:59
…istence, missing kit files)

Fixes 5 findings from a PR review (patterns#399):

1. pi was not reachable after install: npm prefix moved from
   $HOME/.npm-global (not on the guest's default PATH) to $HOME/.local
   (is). Verified live via a bare, fresh `sh -c` probe with zero PATH
   massaging.

2. The runtime CA-bundle claim was false: an install-time-only
   NODE_EXTRA_CA_CERTS export never reached a `pi` process launched
   later in a separate shell. Fixed with a thin wrapper at
   $HOME/.local/bin/pi that rebuilds the bundle and re-execs the real
   binary on every invocation, itself rebuilt every boot (not just on
   a fresh install) so it self-heals an already-installed sandbox.

   Found and fixed two additional bugs during implementation, live:
   an idempotent-re-run regression (lost track of the real binary
   path once npm's symlink was replaced by the wrapper -- fixed with
   a sidecar cache file) and a symlink-follow clobber (naively
   redirecting into the still-live npm symlink truncated the real
   binary itself -- fixed by removing the symlink first). Verified
   across 3 consecutive boots with no regressions.

3. scripts/verify, TROUBLESHOOTING.md, and docs/decisions/ were
   missing despite being mandatory per acq-kits/README.md. Added,
   matching the sibling kits' conventions -- scripts/verify's step 4
   specifically probes `command -v pi` through a bare sh -c with no
   PATH injection, the exact check that would have caught finding 1.
   Live-ran the full offline + RUN_ACQ=1 suite: 7/7 live checks pass.

4. Design rationale was duplicated across spec.yaml, README.md, and
   the install script. Consolidated into docs/decisions/ (two ADRs,
   matching the sibling kits' one-decision-per-file convention); the
   other three now cite it instead of restating it.

5. A leaked CA-decode temp file. Fixed: a single shared install log
   now serves both the CA-decode-failure and npm-install-failure
   paths. Verified live -- no leftover files after any boot.

Verified: validate-kits.py --strict (clean for pi-coding-agent; the
one failure in the run is __pycache__, a pre-existing unrelated
repo issue), sh -n + shellcheck + scan_unsafe_shell.py (all clean),
markdownlint-cli2 (0 issues), and the full live msb regression suite.

Co-authored-by: OpenCode Agent <agent@gsa.gov>

@basiliskus basiliskus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review

Thanks for the thorough round. Findings 1, 3 and 5 look good. Finding 2 is fixed in the script, though the docs still describe an earlier version of the mechanism, and finding 4 is most of the way there. CI is green and the template checklists are in. A few remaining asks:

1. Align the CA docs with the wrapper that shipped. The wrapper exports a bundle path the startup step rebuilds each boot; it does not rebuild anything at invocation. Three places say otherwise: TROUBLESHOOTING.md:32-33, TROUBLESHOOTING.md:162-176 (the cat ~/.local/bin/pi check there will read as a regression), and docs/decisions/ca-bundle-wrapper-not-env-var.md:92-93, which contradicts the same ADR's lines 58-59.

2. Add a verify step for the wrapper. Nothing checks that ~/.local/bin/pi is the wrapper and exports NODE_EXTRA_CA_CERTS, so the fix for the most severe finding has no regression guard. One line: in_sbx 'grep -q NODE_EXTRA_CA_CERTS "$(command -v pi)" && echo yes'.

3. Simplify verify step 8. Its comment (scripts/verify:311-313) says the CA block only runs on install, so it deletes pi and forces a network reinstall. The CA block now runs every boot (pi-coding-agent-install.sh:126-155), so rerunning the script with a garbage PROXY_CA_CERT_B64 is enough. Also make it FAIL rather than WARN, and delete the dead no-op at scripts/verify:155.

4. Finish moving the rationale into the ADRs. pi-coding-agent-install.sh:95-123 and :208-237 restate the two ADRs nearly in full (about 60 of 296 lines are code). The naming rationale is verbatim in spec.yaml:57-64 and README.md:115-121. spec.yaml:28-39 is a 12-line paragraph saying the rationale is no longer duplicated; replace it with a one-line pointer.

5. Drop review narration from comments. "An earlier version of this script did that" (pi-coding-agent-install.sh:88-91), "reproduced live" (:265), "review finding: ..." (scripts/verify:95-100, :130-134, :231-233), "a review flagged" (spec.yaml:35), and the TROUBLESHOOTING entry at lines 17-28 for a bug that never shipped. Git history and the ADRs hold this.

6. Use a fixed log path. mktemp moved above the install guard (pi-coding-agent-install.sh:92), so an empty /tmp/pi-coding-agent-install.*.log is now left on every boot. A fixed install.log in ~/.local/state/pi-coding-agent/, truncated per run, removes mktemp, the $$ fallback, the leftovers, and the glob in the TROUBLESHOOTING commands.

Two optional wrapper nits: exec "$_pi_real" "$@" instead of exec node "$_pi_real" "$@" (:278) keeps upstream's shebang flags, matching npm's own symlink; and the wrapper exports NODE_EXTRA_CA_CERTS even when the bundle is empty, while the script's own export at :155 is guarded by [ -s ].

@mogul mogul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've had hesitation around this PR that I haven't had the energy to articulate before now: OpenChamber and Paseo are UIs around agents already installed by acq. But this kit is installing a separate agent as an add-on, rather than having acq handle the agent.

  • acq infers either a baked base-image or install instructions from the parameter to acq run, as we do for opencode and shell (a convention we lifted from sbx). Does it make sense to install agents via kit, rather than via that mechanism?
  • Conversely, should we be changing acq's behavior so that it uses a kit to install anything other than shell?

@mogul

mogul commented Sep 12, 2026

Copy link
Copy Markdown
Contributor
  • acq infers either a baked base-image or install instructions from the parameter to acq run, as we do for opencode and shell (a convention we lifted from sbx). Does it make sense to install agents via kit, rather than via that mechanism?
  • Conversely, should we be changing acq's behavior so that it uses a kit to install anything other than shell?

Or should we go even further, stripping down acq to use @basiliskus' devenv base image, and then lean into using kits to populate devenv install manifests and hook scripts? 🤔

@wz-gsa

wz-gsa commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

@basiliskus — thank you, this was a genuinely useful review; all 5 findings are addressed. Summary, in your original order:

  1. pi not on PATH — fixed exactly as you suggested: NPM_PREFIX is now $HOME/.local (pi-coding-agent-install.sh:124), which is already first on the image's default PATH — confirmed live via a bare sh -c probe with zero PATH massaging, from a completely separate shell than the one that ran the install. The old process-local PATH prepend is gone; it wasn't load-bearing once the prefix itself was on PATH.

  2. Runtime CA claim was false — fixed with a thin wrapper at $HOME/.local/bin/pi that rebuilds the CA bundle and re-execs the real binary on every invocation (not just install-time), so a user's later pi call gets the current bundle. Rebuilt every boot (not just on fresh install) so it self-heals a sandbox that already had pi from before this fix. Found and fixed two more bugs of my own while building this, live: an idempotent-re-run regression (lost the real binary path once the wrapper replaced npm's symlink — fixed with a sidecar cache file) and a symlink-follow clobber (naively redirecting into the still-live npm symlink truncated the real binary itself). Verified across 3 consecutive boots.

  3. Missing scripts/verify/TROUBLESHOOTING.md/docs/decisions/ — all three added, matching the sibling kits' actual conventions (not just their presence). scripts/verify's step 4 specifically probes command -v pi through a bare sh -c, per your note that this is exactly the check that would have caught finding 1. Live-ran the full offline + RUN_ACQ=1 suite: 7/7 live checks pass.

  4. Rationale duplicated 3x — consolidated into docs/decisions/ (two ADRs — local-prefix-not-npm-global.md, ca-bundle-wrapper-not-env-var.md — matching the sibling kits' one-decision-per-file convention); spec.yaml/README.md/the script now cite them instead of restating.

  5. Leaked CA-decode temp file — fixed exactly as suggested: the install log's mktemp moved above the CA block, decode stderr now goes to 2>>"$_log", the separate _ca_err variable and its $$-fallback path are both gone.

Template checklists — added the Eligibility & Accountability and AI Assistance Disclosure sections with an honest account of AI's role, including the self-found bugs in finding 2.

All 8 CI checks green; validate-kits.py --strict, shellcheck, scan_unsafe_shell.py, markdownlint-cli2 all clean; full live msb regression re-passed after every fix. Separately, there's still an open architecture question on this thread about whether kit-based agent installation is the right model at all vs. wiring into acq run <agent> directly — flagging that I haven't addressed that one and don't think I should resolve it unilaterally; happy to discuss whenever you or the team want to weigh in on it, but it doesn't block re-reviewing the 5 findings above.

@basiliskus

Copy link
Copy Markdown
Contributor

Thanks for the summary. The five original findings are indeed addressed. My September 11 "Changes requested" review has six follow-up asks on the current revision (docs describing a wrapper that rebuilds at invocation, a verify step for the wrapper, verify step 8, the remaining duplicated rationale, review narration in comments, and the per-boot log file). Could you take a look at those?

Responds to basiliskus's six September 15 follow-up asks on PR #399:

- Trim duplicated design rationale from spec.yaml/README.md/install
  script comments down to a single citation of docs/decisions/,
  matching the header's own stated intent (finding: remaining
  duplicated rationale).
- Add scripts/verify step 4b: confirms `pi` resolves to this kit's
  CA-bundle wrapper (exports NODE_EXTRA_CA_CERTS), not npm's bare bin
  symlink. Step 4 alone only proved something resolved on PATH, not
  that it was the wrapper (finding: add a verify step for the
  wrapper).
- Simplify verify step 8 (CA-decode failure): the CA-bundle block
  runs unconditionally on every boot, so the forced-removal-and-
  restore dance around the decode-failure probe was unnecessary.
  Fixed a real severity bug in the same step: a missing decode-
  failure message now reports `bad`, not `warn` (finding: simplify
  verify step 8).
- Move the per-boot install log from a random mktemp path under
  /tmp to a fixed, truncated-per-run
  $HOME/.local/state/pi-coding-agent/install.log, and fix two
  diagnostics (TROUBLESHOOTING.md, scripts/verify step 5) that still
  referenced the old, now-nonexistent glob pattern (finding: fixed
  log path).
- Guard the wrapper's NODE_EXTRA_CA_CERTS export on `[ -s "$_ca" ]`,
  matching the same guard already used earlier in the script, so a
  boot with no proxy CA and no system bundle does not point at a
  useless zero-length cert file. Switch the wrapper to
  `exec "$_pi_real"` rather than hardcoding `exec node`, preserving
  whatever interpreter the real binary's own shebang declares
  (finding: fixed wrapper nits).
- Remove review-thread narration ("the review finding this file
  exists to address", "REGRESSION GUARD: the review's...") from
  code comments in favor of durable prose describing the same
  regression class, per this repo's own durable-references
  convention (finding: drop review narration comments).

Verified: sh -n, shellcheck --severity=warning (0 issues on both
scripts), scripts/scan_unsafe_shell.py (0 errors/warnings),
validate-kits.py --strict (pi-coding-agent OK), markdownlint-cli2
(0 issues), and a live RUN_ACQ=1 scripts/verify end-to-end run on a
real msb sandbox — all 8 live steps pass, including the new step 4b
and the corrected step 8 severity.

Co-authored-by: OpenCode Agent <agent@gsa.gov>
@wz-gsa

wz-gsa commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

@basiliskus — all six follow-up findings are addressed in 5c47a63:

  1. Duplicated rationale — trimmed spec.yaml/README/install-script comments down to a single citation of docs/decisions/, matching the header's own stated intent.
  2. Wrapper verify step — added scripts/verify step 4b: confirms pi resolves to this kit's CA-bundle wrapper (checks for NODE_EXTRA_CA_CERTS), not npm's bare bin symlink. Step 4 alone only proved something resolved on PATH, not that it was the wrapper.
  3. Verify step 8 simplification — the CA-bundle block runs unconditionally on every boot, so the forced-removal-and-restore dance around the decode-failure probe was unnecessary and is now gone. Also fixed a real severity bug found while doing this: a missing decode-failure message now reports bad, not warn.
  4. Remaining duplicated rationale — same fix as (1); confirmed no leftover restatements in TROUBLESHOOTING.md or the ADR.
  5. Review narration in comments — removed phrasing like "the review finding this file exists to address" / "REGRESSION GUARD: the review's..." in favor of durable prose describing the same regression class directly, without leaning on the review thread to carry meaning.
  6. Log path — moved the per-boot install log from a random mktemp path under /tmp to a fixed, truncated-per-run $HOME/.local/state/pi-coding-agent/install.log. Also caught two diagnostics (TROUBLESHOOTING.md, verify step 5) that still referenced the old glob pattern and would have silently shown nothing — fixed both. Wrapper nits: the NODE_EXTRA_CA_CERTS export is now guarded on [ -s "$_ca" ] (matches the guard already used earlier in the script, so a boot with no CA bundle doesn't point at a useless empty file), and the wrapper now execs "$_pi_real" directly rather than hardcoding exec node, preserving whatever interpreter the real binary's own shebang declares.

Verification: sh -n, shellcheck --severity=warning (0 issues on both scripts), scripts/scan_unsafe_shell.py (0 errors/warnings), validate-kits.py --strict (pi-coding-agent OK), markdownlint-cli2 (0 issues), and a live RUN_ACQ=1 scripts/verify end-to-end run against a real msb sandbox — all 8 live steps pass, including the new step 4b and the corrected step 8 severity. CI is green on the new commit.

@basiliskus

Copy link
Copy Markdown
Contributor

Re-review

Thanks for the follow-up round. Four of the six asks are done: the CA docs now match the wrapper that shipped, step 4b is in, step 8 is simplified and FAILs, the log lives at a fixed path, and both wrapper nits are applied. Two remain, plus a few stale bits the same commit introduced.

1. Rationale is still in the install script. Comment lines went from 195 to 185 of 295. pi-coding-agent-install.sh:102-130, :158-172, and :197-217 still restate the prefix, CA, --ignore-scripts, and wrapper reasoning that the ADRs hold. Cut those blocks down to a one-line pointer each, the way spec.yaml and README.md now do.

2. Two pointers now dangle. spec.yaml:28-29 and README.md:103-104 send the reader to docs/decisions/ for the install-method choice (npm vs pi.dev/install.sh) and the --ignore-scripts residual risk, but neither ADR mentions either. That reasoning only exists in the script header (:15-19) and the install block (:158-172). Either add a third ADR for it and point the script there too, or drop the pointers and keep it in one place.

3. Narration remnants. The mktemp history at pi-coding-agent-install.sh:88-97 (the block only needs to say the path is fixed and truncated per run), "not fixed here, since that's a pre-existing, separately-tracked concern" at :128-130, "previously with no regression guard of its own" at scripts/verify:247-254, and "not just a static one-time export baked in at some earlier boot" at TROUBLESHOOTING.md:141-143.

4. Stale bits from 5c47a63. ca-bundle-wrapper-not-env-var.md:122 still calls it "this decision's runtime rebuild"; TROUBLESHOOTING.md:96 says "see " but the message now names the install log; warn() in scripts/verify:66 is unused now that step 8 FAILs; the step 6 comment (scripts/verify:280-281) describes the guard as || { … } while the script uses if !.

One note, no change requested: with the [ -s "$_ca" ] guard, the wrapper omits the export when the bundle is empty, so step 4b fails on a base image with no system CA bundle and no proxy CA. That is fine on the opencode image, just worth knowing the check is not environment-independent.

@wz-gsa

wz-gsa commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

@mogul — coming back to your Sep 12 question about whether agents should install via kit rather than acq's baked-image mechanism.

Short answer: yes, and that's already the direction being worked — but it doesn't change anything about this PR, because pi-coding-agent was never trying to be that.

pi is not in acq's known-agent list, is unreachable via acq run pi ., and only ever layers onto an existing agent's sandbox via --kit/ACQ_EXTRA_KITS (confirmed via this kit's own scripts/verify, which bases it on opencode). It's a kind:mixin capability kit — the same category as openchamber/paseo, which you correctly named as the precedent this kit diverges from. The divergence is real, but it's "installs a second CLI as an add-on" vs. "wraps an already-installed agent," not "should agents install via kit."

The actual question you're asking — should opencode/claude/etc. themselves move off acq's baked-image mechanism onto a kit-based install — is already being worked, in the right venue: GSA-TTS/agentic-coding-quickstart#474 (ADR-0030, "agent kits on a devenv-enabled base image," status: proposed). It defines exactly this: acq run <agent> inferring a built-in agent kit (a new, distinct role from today's capability/mixin kits), on a generic devenv base, no baked per-agent images. Real code has landed on that branch — agent-kit metadata functions, kit-list wiring — but it's currently a fully-built no-op (the enable flag is hardcoded false for every agent) pending a real patterns-side agent-kit artifact and a schema decision (today's hybrid/v1 schema has no agent:/role concept at all — kind is a hardcoded const "mixin"). You flagged this yourself on GSA-TTS/agentic-coding-quickstart#485 three days ago.

So: agent-kits and capability-kits are distinct roles in the target model, and pi doesn't need to become one for this PR to be correct today, or to need rework once #474 lands. I'd like to un-stall this PR on that basis and pick back up on your Sep 17 line-level follow-ups (rationale still duplicated in the install script, two dangling doc pointers, narration remnants, a few stale bits from the last commit) — working on those now.

Responds to basiliskus's September 17 re-review on PR #399:

- Rationale still duplicated in the install script: trimmed the
  npm-prefix, --ignore-scripts, and CA-bundle-wrapper comment blocks
  (pi-coding-agent-install.sh:102-130, :158-172, :197-217) down to
  one-line pointers, matching what spec.yaml/README.md already do.
  295 -> 245 lines.
- Two dangling pointers: spec.yaml and README.md pointed at
  docs/decisions/ for the install-method choice (npm vs
  pi.dev/install.sh) and the --ignore-scripts residual risk, but
  neither existing ADR (ca-bundle-wrapper-not-env-var.md,
  local-prefix-not-npm-global.md) actually covers either. Rather than
  add a third ADR for reasoning that already lives correctly in one
  place, dropped the dangling pointers and pointed to the actual
  location instead (the install script's own comments; README.md's
  "Install method"/"Supply-chain notes" sections).
- Narration remnants: removed the mktemp-history restatement
  (pi-coding-agent-install.sh:88-97), "not fixed here, since that's a
  pre-existing, separately-tracked concern" (:128-130), and
  "previously with no regression guard of its own" (scripts/
  verify:247-254).
- Stale bits from 5c47a63: ca-bundle-wrapper-not-env-var.md:122's
  "this decision's runtime rebuild" -> "boot-time wrapper rewrite"
  (the wrapper is rewritten on every boot, nothing is rebuilt "at
  invocation"); removed scripts/verify's now-unused warn() helper
  (step 8 already FAILs outright, nothing calls warn() anymore); the
  step 6 comment now describes the guard's actual current shape
  (`if ! command -v pi >/dev/null 2>&1; then …fi`), not the
  `|| { … }` shape from an earlier revision.

Verified: sh -n and shellcheck --severity=warning (0 issues) on both
modified scripts; scripts/scan_unsafe_shell.py (0 errors/warnings);
validate-kits.py --strict (pi-coding-agent OK); markdownlint-cli2
(0 issues); a live RUN_ACQ=1 scripts/verify end-to-end run against a
real sandbox — all 8 live steps pass, including the wrapper check
(4b), the idempotent re-run (6), the bad-version rejection (7), and
the CA-decode-failure surfacing (8).

Co-authored-by: OpenCode Agent <agent@gsa.gov>
@wz-gsa

wz-gsa commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

@basiliskus — all four items from your Sep 17 re-review are addressed in fc1b5ca:

  1. Rationale still in the install script. Trimmed the npm-prefix, --ignore-scripts, and CA-bundle-wrapper blocks (:102-130, :158-172, :197-217) down to one-line pointers, matching what spec.yaml/README.md already do. 295 → 245 lines.
  2. Two dangling pointers. Checked both existing ADRs directly — neither ca-bundle-wrapper-not-env-var.md nor local-prefix-not-npm-global.md actually covers the install-method choice or the --ignore-scripts residual risk; that reasoning only ever lived in the script header/install block. Rather than add a third ADR for something already correctly stated in one place, I dropped the dangling docs/decisions/ pointers in spec.yaml/README.md and pointed to the actual location instead (the install script's own comments; README.md's "Install method"/"Supply-chain notes" sections).
  3. Narration remnants. Removed all three: the mktemp-history restatement (:88-97), "not fixed here, since that's a pre-existing, separately-tracked concern" (:128-130), and "previously with no regression guard of its own" (scripts/verify:247-254).
  4. Stale bits from 5c47a63. ca-bundle-wrapper-not-env-var.md:122 now says "boot-time wrapper rewrite" (nothing is rebuilt "at invocation" — the wrapper is rewritten on every boot, which is the correct framing). Removed scripts/verify's now-unused warn() (step 8 already FAILs outright, nothing calls it). Fixed the step 6 comment to describe the guard's actual current shape (if ! command -v pi …then …fi, not the || { … } shape from an earlier revision. I checked TROUBLESHOOTING.md:96's error-path text directly and it already matches the real message — looks like that one was already fixed by an earlier pass, no change needed there.

Verification: sh -n/shellcheck --severity=warning (0 issues, both scripts), scripts/scan_unsafe_shell.py (0 errors/warnings), validate-kits.py --strict (OK), markdownlint-cli2 (0 issues), and a live RUN_ACQ=1 scripts/verify end-to-end run against a real sandbox — all 8 live steps pass, including the wrapper check, idempotent re-run, bad-version rejection, and CA-decode-failure surfacing. CI is green (7/7).

Also replied above to @mogul's architecture question with a pointer to GSA-TTS/agentic-coding-quickstart#474 (ADR-0030) — TL;DR: pi is a capability kit, not an agent-install path, so it's orthogonal to that question either way it resolves.

@mogul mogul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved, and yes, we have taken up the answer to my question about agents being installed by kits elsewhere in the time since this PR started.

@mogul
mogul enabled auto-merge (squash) September 23, 2026 06:30
@mogul
mogul merged commit 5d1a877 into main Sep 23, 2026
7 checks passed
@mogul
mogul deleted the feat/pi-coding-agent-kit branch September 23, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants