Repository navigation
feat(patterns): add pi-coding-agent acq-kit (earendil-works/pi, npm install, unprivileged startup-only) - #399
Conversation
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>
… 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>
ReviewFive findings, most severe first. The supply-chain claims in the PR body check out: 1.
|
…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>
…eat/pi-coding-agent-kit
basiliskus
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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 foropencodeandshell(a convention we lifted fromsbx). 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? 🤔 |
|
@basiliskus — thank you, this was a genuinely useful review; all 5 findings are addressed. Summary, in your original order:
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; |
|
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>
|
@basiliskus — all six follow-up findings are addressed in 5c47a63:
Verification: |
Re-reviewThanks 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. 2. Two pointers now dangle. 3. Narration remnants. The mktemp history at 4. Stale bits from 5c47a63. One note, no change requested: with the |
|
@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.
The actual question you're asking — should 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>
|
@basiliskus — all four items from your Sep 17 re-review are addressed in fc1b5ca:
Verification: 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
left a comment
There was a problem hiding this comment.
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.
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 onlyread/write/edit/bashtools by default.Ships ahead of the in-flight
prime-agentkit (#368) by design — see #398 for the full sequencing rationale.Eligibility & Accountability
@GSA-TTS/agentic-coding-teamcode-owner approval; eligibility is attested here, not authenticated).AI Assistance Disclosure
engines.noderequirement, 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.What it does
openchamber's settled unprivileged pattern.npm install -g --ignore-scripts --prefix "$HOME/.local", pinned to a specific reviewed version,--fetch-timeout/--fetch-retries=0to bound registry-hang risk), deliberately notpi.dev/install.sh.openchamber's own script, rebuilt every boot; a thinpiwrapper carries that bundle into the user's own later invocation (see "Findings addressed" below — this is new since the last review round).registry.npmjs.orgonly.scripts/verify,TROUBLESHOOTING.md, anddocs/decisions/are now present, matching every sibling kit's mandatory structure (peracq-kits/README.md) — new since the last review round.--ignore-scriptsresidual risk, prefix choice, CA-bundle wrapper) is now documented once, indocs/decisions/, rather than duplicated acrossspec.yaml,README.md, and the install script's own comments.Findings addressed (from @basiliskus's review — thank you)
piwas not reachable after install. Root cause: install prefix was$HOME/.npm-global, which is not on the guest's defaultPATH; the review correctly noted this is the same PATH-injection problemopenchamber/paseosolve in their ownscripts/verify, not the kit's install script. Fixed: switched the prefix to$HOME/.local, which is on the default guestPATH— confirmed live via a completely baresh -cprobe from a fresh, separate exec (no PATH massaging of any kind). Seedocs/decisions/local-prefix-not-npm-global.md.export NODE_EXTRA_CA_CERTSnever reaches apiprocess a user launches later, in a separate shell. Fixed:pinow ships behind a thin wrapper at$HOME/.local/bin/pithat 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 hadpifrom before this fix. Seedocs/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.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 probescommand -v pithrough a baresh -cwith zero PATH injection — the exact check the review said would have caught finding 1. Live-ran the full offline +RUN_ACQ=1live suite: 7/7 live checks pass; the one offline failure (validate-kits.pytripping on a__pycache__directory) is a pre-existing, unrelated repo issue, confirmed by the same run showingOK pi-coding-agent.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.ca-decode.*.errfiles after any boot.Verification
validate-kits.py --strict— clean forpi-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 findingsmarkdownlint-cli2— 0 issues across README/TROUBLESHOOTING/docs/decisionspi --versioninvocation, wrapper content inspection (confirms the CA env var and real-binary path are both correct), and the fullscripts/verifylive suite (7/7 pass)Co-authored-by: OpenCode Agent agent@gsa.gov