Skip to content

feat(paseo): bump pinned Paseo CLI to 0.10.2 - #439

Merged
mogul merged 9 commits into
mainfrom
feat/paseo-kit-bump-0.9.1
Sep 30, 2026
Merged

mogul merged 9 commits into
mainfrom
feat/paseo-kit-bump-0.9.1

Conversation

@mogul

@mogul mogul commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Context

Update the paseo acq kit (integrations/isolation/acq-kits/paseo) to install the latest Paseo release, 0.10.2 (previously pinned to 0.7.0, and superseding the earlier 0.9.1 bump in this PR). Verified @getpaseo/cli@0.10.2 is the current latest on the npm registry.

Release-note review

Reviewed the upstream release notes from 0.9.2 through 0.10.2 before keeping the version bump itself as a pin update:

  • 0.9.2 includes daemon startup/config robustness fixes, project/workspace fixes, and OpenCode permission-rule fixes.
  • 0.10.0 adds OpenCode v2 support, password-protected daemon connection handling, and additional daemon robustness fixes.
  • 0.10.1 and 0.10.2 primarily fix OpenCode v2 chat, question-card, context-meter, timeout, and edit-diff behavior.

The kit already uses the 0.9+ daemon approach (paseo daemon run plus persisted daemon config), so 0.10.2 does not require another daemon launch-mode migration.

Plan / Changes

The version pin remains a documented two-value edit:

  • spec.yaml — PASEO_CLI_VERSION="0.10.2" in the startup command
  • files/home/paseo-start.sh — in-script fallback default _ver="${PASEO_CLI_VERSION:-0.10.2}" and its header doc comment

Review feedback from wz-gsa's latest PR comments is also addressed:

  • paseo-start.sh now treats required daemon config persistence as fail-closed: it writes daemon.listen, features.webUi.enabled, and daemon.relay.enabled, reads them back from config.json, and does not start the unauthenticated daemon if the required settings cannot be verified.
  • scripts/verify now has a live assertion that reads config.json and checks those three required daemon settings.
  • paseo-start.sh no longer deletes paseo.pid before every daemon run; Paseo keeps ownership of its double-daemon guard during ordinary respawn.
  • paseo-agent-shim only clears paseo.pid after the fallback path actually kills a daemon process.
  • paseo-start.sh and paseo-set-worktrees-root.mjs now share a coarse $PASEO_HOME/.config-json.lock around config.json writes to prevent startup config writes racing the wrapper's worktree-root write.
  • Added focused Node tests for the worktree-root helper preserving daemon settings and failing closed when the shared config lock is held.

Verification

  • npm view @getpaseo/cli@0.10.2 version && npm view @getpaseo/cli dist-tags.latest -> 0.10.2 / 0.10.2
  • sh -n integrations/isolation/acq-kits/paseo/files/home/paseo-start.sh && sh -n integrations/isolation/acq-kits/paseo/files/home/paseo-agent-shim && bash -n integrations/isolation/acq-kits/paseo/scripts/verify -> OK
  • node --check integrations/isolation/acq-kits/paseo/files/home/paseo-set-worktrees-root.mjs && node --test integrations/isolation/acq-kits/paseo/tests/**/*.test.mjs -> 10 passed, 0 failed
  • python3 -c "import pathlib, yaml; yaml.safe_load(pathlib.Path('integrations/isolation/acq-kits/paseo/spec.yaml').read_text())" && git diff --check -- integrations/isolation/acq-kits/paseo -> OK
  • integrations/isolation/acq-kits/paseo/scripts/verify -> 5 passed, 0 failed; live sandbox checks skipped because RUN_ACQ=1 was not set; neutral kit schema check skipped because this environment lacks jsonschema and pyyaml
  • Search for 0.9.1|0.7.0 across the kit -> no remaining references

make validate was attempted but is blocked in this environment because python is not on PATH. Running the underlying validator with python3 starts but fails before repository validation because the local environment is missing jsonschema; no dependencies were installed as part of this change.

Rollback

Revert this PR to restore the prior 0.7.0 kit pin and the previous daemon startup behavior.

Security Impact

Positive/low risk. The change preserves the existing startup path and network egress shape: startup still installs @getpaseo/cli from registry.npmjs.org with lifecycle scripts disabled. The follow-up hardening makes the relay-disable setting verifiable before launching the unauthenticated daemon and removes an unconditional PID-lock deletion that could undermine Paseo's own double-daemon guard.

AI-assisted change.

Co-authored-by: OpenCode bret.mogilefsky@gsa.gov

@mogul
mogul requested a review from a team as a code owner September 23, 2026 06:47
@wz-gsa

wz-gsa commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

[Deleted review of unrelated PR. -Bret]

@wz-gsa

wz-gsa commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head a6c9e6b, main at 5d1a877. Additional review capacity; I read the existing 3/5-coverage panel comment first and this is deliberately not a re-run of it.

The headline finding is structural, and it changes how the rest of this PR should be read.

Blocker-ish: this PR is titled as a version bump but carries all of draft #420

The title and commit message describe a two-value pin edit. Measured:

$ git diff 1295191..a6c9e6b --shortstat          # #420's head .. #439's head
 2 files changed, 3 insertions(+), 3 deletions(-)

$ git merge-base --is-ancestor 1295191 a6c9e6b && echo CONTAINS
CONTAINS

So #439's own contribution is 3 lines, and the other +1110 is draft PR #420 (feat(paseo): add backup/restore scripts…, still isDraft=true) riding underneath: paseo-backup, paseo-restore, a new ADR, README/TROUBLESHOOTING, and scripts/verify changes.

That matters for three reasons:

  1. A reviewer approving "bump the pin to 0.9.1" would be approving ~1100 lines of new host-side backup/restore tooling that its own author has not yet marked ready.
  2. feat(paseo): add backup/restore scripts to preserve state across sandbox recreation #420 is where the review belongs. Every finding in the existing panel comment is against paseo-restore — i.e. against feat(paseo): add backup/restore scripts to preserve state across sandbox recreation #420's code, filed on feat(paseo): bump pinned Paseo CLI to 0.10.2 #439. If feat(paseo): bump pinned Paseo CLI to 0.10.2 #439 merges, feat(paseo): add backup/restore scripts to preserve state across sandbox recreation #420 becomes empty and its review history is orphaned from the code it reviewed.
  3. Both PRs are BEHIND main, so neither can merge as-is regardless.

Cleanest resolutions, either is fine: rebase #439 onto main so it is genuinely the 3-line bump and let #420 land on its own merits; or mark #420 ready, review it there, and retitle #439 to say it includes it. What shouldn't happen is #1100 lines merging under a bump title.

I could not reproduce the SIGPIPE finding, and I think it's wrong as stated

The existing comment reports that paseo-restore:268

if tar -tf "${BACKUP_TAR}" | grep -Eq '(^|/)daemon\.log(\.|$)|…'; then

silently skips the prune branch under set -euo pipefail because grep -q exits early, tar takes SIGPIPE (141), and pipefail propagates it. I tried hard to reproduce that and could not. Two fixtures, both with the match positioned to maximise the chance:

# 3003 members, daemon.log at position 1670, listing 52924 bytes
set -euo pipefail; tar -tf big.tar | grep -Eq '…daemon\.log…'
  -> MATCHED, prune branch TAKEN, outer rc=0
  PIPESTATUS=(0 0)

# 20003 members, daemon.log FIRST, listing 508953 bytes (~8x the 64KiB pipe buffer)
  -> MATCHED, outer rc=0
  PIPESTATUS=(0 0)

Why: on this host tar is bsdtar 3.5.3 / libarchive 3.7.4, and it does not die on SIGPIPE here. Control checks isolating each link:

yes match | grep -q match      -> PIPESTATUS=(141 0)   # SIGPIPE mechanism works
tar -tf big2.tar | head -1     -> PIPESTATUS=(0 0)     # bsdtar does not
tar -tf big2.tar | (read -r _) -> PIPESTATUS=(0 0)

So the mechanism is real in general but bsdtar specifically doesn't exhibit it, and paseo-restore runs on the host (it pipes base64 … | acq exec), which for this project's primary platform is macOS/bsdtar.

I want to be careful here rather than just contradicting: I had no GNU tar available to test the other side, and GNU tar does exit on SIGPIPE, so the finding is plausibly correct on a Linux host and simply not reproducible on macOS. That makes it platform-conditional, not "confirmed" — and the fix is cheap either way (tar -tf "$TAR" > "$list" then grep -q against the file, or grep -c with the pipeline status captured deliberately). Worth stating the platform in the finding so whoever fixes it knows what they're fixing, and worth a note that a "verified" claim from one platform's tar doesn't generalise.

Verified sound

  • The pin edit itself is complete and consistent. All three places agree on 0.9.1 — spec.yaml:185, the _ver="${PASEO_CLI_VERSION:-0.9.1}" fallback, and the header doc comment. rg '0\.7\.0' over the whole kit returns nothing, so no stale pin was left behind, and the ADR's "two-value edit" contract (install-at-startup.md:63-64) is honoured exactly. This is the part the title describes, and it's right.
  • Install stays npm install -g --prefix "$HOME/.npm-global" — unprivileged, per-user prefix, no sudo.
  • Kit egress is unchanged (registry.npmjs.org only); no new hosts, no wildcards.
  • bin/injection-scan.sh flagged one hit — base64 in the new ADR prose describing the transport — inspected, benign.

What a 0.7.0 → 0.9.1 bump should say

Two minor versions with no note on what changed. The commit body explains the mechanics of the edit but not the risk: no changelog reference, no statement that the daemon's CLI surface (paseo daemon start, the --version parse at paseo-start.sh:81, the 0.0.0.0:6767 listen default) is unchanged across those two minors. The version-detection sed is regex-fragile by design, so "does 0.9.1 still print a parseable --version?" is a real question. The kit's scripts/verify would answer it on a live run — worth saying whether that was done.


Method: static analysis at a6c9e6b; the SIGPIPE non-reproduction used local tar fixtures (3003 and 20003 members) on bsdtar 3.5.3 / BSD grep 2.6.0. No kit executed, no sandbox created, no registry mutation. UNVERIFIED: GNU-tar behaviour (none installed here), and anything requiring a live Paseo 0.9.1.

@mogul
mogul marked this pull request as draft September 23, 2026 22:18
Adopt the latest Paseo release (0.9.1) by updating the two-value version
pin: PASEO_CLI_VERSION in spec.yaml and the matching in-script fallback
default in paseo-start.sh (plus its header doc comment).
@mogul

mogul commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Blocker-ish: this PR is titled as a version bump but carries all of draft #420

Oops, fixing that now!

@mogul
mogul force-pushed the feat/paseo-kit-bump-0.9.1 branch from a6c9e6b to 450d77e Compare September 23, 2026 23:27
@mogul
mogul marked this pull request as ready for review September 23, 2026 23:33
@mogul

mogul commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Ready for re-review.

Changes since the prior review:

  • Removed the unintended PR 420 ancestry from this branch by rebasing onto main; this PR is no longer carrying the backup/restore work.
  • Kept the Paseo 0.9.1 version bump, but expanded it to handle the actual 0.9 daemon CLI change: paseo daemon start --listen/--web-ui/--foreground was removed, so the kit now persists required daemon config and supervises paseo daemon run.
  • Updated the worktree-root bounce path to prefer paseo daemon restart with the existing marker-scoped kill as fallback.
  • Fixed the project registrar / verify API connection for Paseo 0.9 by passing an explicit local daemon target.
  • Added a decision note documenting the 0.9 launch-mode migration and updated related docs/tests.

Verification:

  • Live sandbox smoke: Paseo 0.9.1 running via paseo daemon run --home /home/agent/.paseo; /api/health and / return 200; daemon binds 0.0.0.0:6767; relay is disabled; registrar succeeds against the 0.9 API.
  • User reran the live kit verify and reported all checks passed, including project pre-population.
  • ./scripts/verify offline gate: 6 passed, 0 failed.
  • shellcheck --severity=warning integrations/isolation/acq-kits/paseo/files/home/paseo-start.sh integrations/isolation/acq-kits/paseo/files/home/paseo-agent-shim integrations/isolation/acq-kits/paseo/scripts/verify: passed.
  • make validate: passed, with existing advisory warnings only.

AI-assisted changes by OpenCode.

@wz-gsa

wz-gsa commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Follow-up pass at head 502ff79 (this is the same head as my earlier partial-coverage review on this PR; re-running with a fresh dispatch got full coverage this time). 5/5 coverage, unanimous 5-0 request_changes.

I independently re-verified the panel's top finding before posting; it holds.

High: the migration's central new mechanism (config persistence) is fail-open and has zero test coverage

All 5 voters converge on this independently. Verified directly: paseo-start.sh:359-367 runs 3 paseo daemon config set calls (daemon.listen, features.webUi.enabled, daemon.relay.enabled), and every one is if ! ...; then echo warning >&2; fi — none is fatal, none is retried, none is asserted anywhere. I checked scripts/verify's diff for any readback of config.json and found none — the only new check added (removed_flags=...) is a static grep over the shipped script's own text for old flag strings, which is a lint on a file this same PR just edited, not a behavioral assertion. A sandbox where all three config set calls silently fail (wrong CLI shape, key rename in a future 0.9.x, unwritable $PASEO_HOME) would still report fully green.

The part that makes this more than a robustness nit: daemon.relay.enabled=false is a security control, not just a functional setting — it's what keeps an intentionally-unauthenticated daemon (PASEO_PASSWORD unset by design) off Paseo's cloud relay. Per the PR's own stated premise, managed daemon start mode strips the PASEO_RELAY_ENABLED env override, so config.json is the only remaining source of truth for that setting in the mode where a user might later run paseo daemon restart on a down daemon. A failed config set here degrades a stated security control to whatever upstream defaults to, with the only signal being a stderr warning from a backgrounded startup script that nobody reads, appended to a $STATE_DIR/paseo-config.log that nothing else reads either.

Suggested minimum fix (cheap, per multiple voters): add one assertion in scripts/verify that reads back $PASEO_HOME/config.json after startup and checks the three keys landed, and/or make the daemon.relay.enabled write specifically fatal (or self-correcting) rather than warn-only, since it's the one of the three that's security-relevant rather than purely functional.

Medium: unconditional PID-lock deletion removes the only double-daemon guard

Verified at paseo-start.sh:404 (inside the respawn loop) and mirrored at paseo-agent-shim:277 (the restart-fallback path):

rm -f "${PASEO_HOME:-$HOME/.paseo}/paseo.pid"

runs unconditionally, every iteration, before every relaunch. The comment justifying it says "Paseo treats a dead owner lock as stale" — which is the PR's own admission that Paseo's existing stale-lock handling already covers the case this line claims to guard against. So the line's only actual effect is removing the lock in the case Paseo's own logic does NOT consider stale: a live owner. Three of five voters (architect, security, scope_steward) independently traced the same interaction: this PR also introduces paseo daemon restart as the primary restart path on the worktree-pin-change flow, and if that command brings a replacement worker up under Paseo's own supervision while the outer shell loop's tracked process exits (or is killed via the fallback path), the outer loop's next iteration deletes that live worker's lock and starts a second paseo daemon run against the same PASEO_HOME — contending for port 6767. Net effect ranges from an EADDRINUSE respawn flap to two concurrent writers on shared daemon/worktree state.

scope_steward's framing is the sharpest here: this is added complexity (rung 6 hand-rolled defensive code) justified by an unevidenced race ("avoids repeated lock-acquire races"), at the cost of deleting a real mutual-exclusion mechanism — and it's speculative by the same comment's own admission that upstream already handles the stated case.

Medium, not independently reproduced but well-traced (catfish): two unsynchronized writers to config.json

Before this PR, paseo-agent-shim's paseo-set-worktrees-root.mjs was the sole writer to config.json. This PR adds a second writer — the backgrounded startup script's three config set calls — with no lock or ordering between them. catfish traces a plausible lost-update: if the shim's worktree-root write and the startup script's config-set calls interleave, worktrees.root could get silently reverted by whichever write lands last, while the shim still reports success to the user. I didn't reproduce this live, but the shape (two independent read-modify-write paths on one JSON file, newly introduced by this diff) is real and worth a look — even a coarse flock around both writers would close it.

What holds up (verified independently, not just cited)

  • connectToDaemon({ target: { kind: "instance", home: ... } }) fix is applied consistently in both call sites (paseo-register-mounts.mjs and scripts/verify's own smoke-test) — confirmed by grep across the diff.
  • The supervisor-marker exclusion (supervisor:paseo-daemon) is preserved correctly everywhere the pgrep pattern changed from paseo daemon start to paseo daemon run — checked all 3 sites (shim fallback, verify self-heal test, TROUBLESHOOTING docs) for consistency; all updated in lockstep.
  • Both PASEO_HOME_DIR definitions devex/catfish flagged as "not shown in this diff" are real and correct — I checked the full files at head 502ff79: paseo-agent-shim:62 and the startup script both independently define it before use. Not a bug; just each script defining its own copy of a small local, which is fine.
  • The paseo daemon restart → fallback-kill ordering is the right shape: supported API attempted first, hand-rolled kill only as a documented last resort.

Not gated

  • The inconsistent --string daemon.listen vs. bare features.webUi.enabled true flag shape (architect/pm) is worth a second look but I can't confirm from the diff alone whether that's a real CLI-contract gap or just how the two value types are meant to differ.
  • TROUBLESHOOTING.md hardcodes --home /home/agent/.paseo in one snippet while an adjacent one uses ${PASEO_HOME:-$HOME/.paseo} — cosmetic inconsistency, not a functional issue.

Method: reviewed the full diff at head 502ff79 via a 5-role panel; independently re-verified the config-persistence fail-open claim and the PASEO_HOME_DIR non-issue by reading the actual files at that head rather than trusting the diff hunks alone. No live sandbox was created this pass.

@wz-gsa

wz-gsa commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

This PR's daemon-lifecycle rewrite independently fixes a live, currently-reported bug — #449

AI-assisted, advisory (OpenCode). Not a new review — the tar/pipefail and pid-lock findings from the earlier review still stand and are unaddressed as of this PR's current head. This is a separate, positive finding worth surfacing before it's lost.

#449 (opened today, basiliskus) reports the daemon-bounce logic on main orphaning the Paseo daemon on version 0.7.0: pgrep -f "paseo daemon start" matches nothing against the real process tree (Paseo Supervisor → Paseo Daemon, no matching argv), so the kill-and-respawn bounce kills zero processes, leaves the original daemon running and holding the lock, and the supervisor loop then fails forever trying to start a second one (basiliskus's repro: 6,899 failed restarts over 12 hours).

I live-verified against the real @getpaseo/cli@0.9.1 binary (not just read the diff) that this PR's rewrite of the same code path — paseo daemon restart as the primary path, with the fallback kill now scoped to paseo daemon run instead of the old paseo daemon start — closes the specific mechanism #449 describes: pgrep -f "paseo daemon run" correctly matches 0.9.x's real outer daemon PID, and killing just that PID took the whole daemon+worker tree down cleanly with no orphan, unlike the old pattern.

This wasn't written for #449 (this PR predates it), but both land on the same root cause — the pgrep pattern not matching the real process tree — from different directions (a version bump vs. a live orphaning bug report). Posted the connection on #449 with the verification detail; flagging here too so whoever picks up review on this PR has the context, and so the fix doesn't get lost if this PR's scope changes.

Doesn't change the outstanding blocking findings from the prior review (tar/pipefail inversion, unconditional rm -f paseo.pid in the respawn loop) — those still need addressing before merge.

Update the Paseo acq kit pin from 0.9.1 to 0.10.2 in the startup spec and fallback script default.

Release notes reviewed: 0.10.x keeps the 0.9 daemon-run/config approach while adding OpenCode v2 support and daemon robustness fixes, so no kit wiring changes are needed.

Co-authored-by: OpenCode <bret.mogilefsky@gsa.gov>
@mogul mogul changed the title feat(paseo): bump pinned Paseo CLI to 0.9.1 feat(paseo): bump pinned Paseo CLI to 0.10.2 Sep 30, 2026
Make startup fail closed if required daemon settings cannot be written and verified, add a shared config-json lock for startup and worktree-root writes, and stop deleting the daemon PID lock during ordinary respawn.

Add live verify coverage for the persisted daemon settings and focused helper tests for config preservation and lock contention.

Co-authored-by: OpenCode <bret.mogilefsky@gsa.gov>
Bump markdownlint-cli2 and its markdown-it override so the Markdown Lint job's npm audit step passes with no moderate advisory.

Co-authored-by: OpenCode <bret.mogilefsky@gsa.gov>
@mogul

mogul commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Ready for re-review.

Changes since the last review:

  • Addressed the daemon config persistence finding: startup now writes and reads back daemon.listen, features.webUi.enabled, and daemon.relay.enabled; if they cannot be verified, it does not start the unauthenticated daemon with unknown relay/listen defaults.
  • Added a shared $PASEO_HOME/.config-json.lock around both startup config writes and the worktree-root helper write to prevent concurrent config.json read-modify-write races.
  • Removed unconditional paseo.pid deletion from the supervisor respawn loop; the shim fallback only clears it after actually killing a daemon process.
  • Added live verify coverage for the persisted daemon settings and focused Node tests for config preservation and lock contention.
  • Fixed the failing Markdown Lint CI setup by updating the pinned markdown linter dependency path so npm audit --audit-level=moderate passes.

Verification:

  • GitHub PR checks are all passing as of head e4c7cd9.
  • Local focused checks passed: shell syntax checks, node --check, paseo kit Node tests (10 passed, 0 failed), YAML parse, git diff --check, exact markdownlint CI command, and linter npm audit --audit-level=moderate.
  • Local integrations/isolation/acq-kits/paseo/scripts/verify offline gate passed (5 passed, 0 failed); live sandbox checks were not run locally because RUN_ACQ=1 was not set.

AI-assisted follow-up by OpenCode.

@basiliskus

Copy link
Copy Markdown
Contributor

Reviewed at e4c7cd9.

I reviewed this as a fresh-context change-review pass against main at 5d1a87714b6e500c4c910ab1b0dcf11baba09d13, including the prior advisory threads and the current PR body. The earlier unintended #420 payload is gone, the daemon config path now reads back the three required settings before launch, the unconditional supervisor-loop paseo.pid deletion is gone, and both named config.json writers now use the shared lock.

  1. Major, confirmed: the new daemon config assertion is live-only and does not run in the offline/CI gate (integrations/isolation/acq-kits/paseo/scripts/verify:195, integrations/isolation/acq-kits/paseo/scripts/verify:372)

    The added config.json assertion is in the live section at lines 372-377, but the default/offline path exits at lines 195-203 before reaching it unless RUN_ACQ=1 or RUN_SBX=1 is set. CI runs the offline acq-kit Node tests via make test-kits, not a live RUN_ACQ=1 verify. So the passing checks do not exercise the security-relevant readback for daemon.relay.enabled=false.

    Concrete failure scenario: a future Paseo CLI changes a config key, or paseo daemon config set exits 0 without writing the expected key. The current startup script appears to fail closed by reading back config.json before launch, but that shell readback path is not covered by the offline gate or the current CI signal. A green CI run would not catch a regression that removes or weakens the daemon.relay.enabled=false verification.

Below the bar: I did not find a current code path that still deletes a live PID lock during ordinary respawn. paseo-start.sh now lets Paseo own paseo.pid, and paseo-agent-shim only removes it after a successful fallback kill. I also did not find any remaining backup/restore files or surviving tar | grep pipeline from the earlier #420 payload. The #449 mechanism also looks addressed for the 0.10.x path: the shim now prefers paseo daemon restart and falls back to matching paseo daemon run, not the old paseo daemon start pattern.

Validation: git rev-parse HEAD matched e4c7cd94c72606e749a05982cbc60cc4109f80ab; sh -n passed for paseo-start.sh and paseo-agent-shim; bash -n passed for scripts/verify; node --check passed for both .mjs files; the version regex parsed 0.10.2, v0.10.2, @getpaseo/cli 0.10.2, and paseo 0.10.2 correctly. gh pr checks 439 --repo GSA-TTS/agentic-coding-patterns showed all 7 checks passing. make validate failed locally because python is not on PATH; python3 exists but lacks jsonschema. I did not run the kit Node tests, local markdownlint, scripts/verify, or RUN_ACQ=1 live verification in this plan/read-only session because those commands would create files/directories or require a live sandbox.

Verdict: Comment. The implementation looks substantially fixed, but the PR should not describe the live config.json assertion as CI/offline coverage unless it is moved into an offline testable path or a live gate is actually run.

Assisted-by: OpenCode [gpt_5_5_default_v2] change-review

Move the daemon config readback into a shared helper used by startup and verify, and add offline Node tests for required listen, web UI, and relay settings.

Co-authored-by: OpenCode <bret.mogilefsky@gsa.gov>
….9.1-1

# Conflicts:
#	.github/linters/package-lock.json
#	.github/linters/package.json
@mogul

mogul commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Ready for re-review.

Changes since the latest feedback:

  • Added paseo-assert-daemon-config.mjs, a shared assertion helper for the security-relevant daemon config readback (daemon.listen, features.webUi.enabled, and daemon.relay.enabled=false).
  • Updated paseo-start.sh to call that helper before daemon launch instead of carrying an inline node -e snippet.
  • Updated live scripts/verify to call the same helper, so the live check and startup fail-closed path cannot drift.
  • Added offline Node tests for the helper so CI now covers both success and failure cases, including missing daemon.relay.enabled=false.
  • Updated the branch from origin/main and resolved the linter dependency conflict to main’s patched markdown-it@14.3.1 override.

Verification:

  • GitHub PR checks are all passing at head a9c77ae.
  • make test-kits passed locally, including the new Paseo offline config assertion tests (13 Paseo tests, 0 failed).
  • Focused local checks passed: node --check for Paseo helper files, sh -n / bash -n for shell scripts, YAML parse, git diff --check, markdownlint, and linter npm audit --audit-level=moderate.
  • make validate remains blocked only in this local environment because python is not on PATH; direct python3 scripts/validate_repo.py still lacks local jsonschema. CI Pattern Validation is passing.

AI-assisted follow-up by OpenCode.

basiliskus
basiliskus previously approved these changes Sep 30, 2026

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

Reviewed at a9c77ae.

I re-reviewed the incremental fix after my prior comment on #439. The authored follow-up extracts the daemon config readback into a shipped helper, calls that helper from startup and live verify, and adds offline Node coverage for the security-relevant daemon.relay.enabled=false assertion. I also checked the merge from origin/main; the merged main commits did not touch the Paseo kit.

No findings.

Below the bar: my prior finding is resolved. paseo-start.sh now invokes /home/agent/paseo-assert-daemon-config.mjs before launching the unauthenticated daemon, scripts/verify uses the same helper in the live check, and the new Node tests cover accept/reject behavior for required listen, web UI, and relay-disabled settings. The tests exercise the exported function rather than the CLI entrypoint, but the CLI wrapper is a thin argument parser plus that same function; I traced the production invocation and checked that missing/empty --listen fails closed. Startup always defaults PASEO_LISTEN before invoking the helper and passes --paseo-home explicitly, so I do not see a path-resolution divergence between startup and live verify.

I also looked at the exit 0 question in the config-failure paths. I do not think it is a finding. The important fail-closed behavior is that the daemon is not started with unknown listen/relay defaults. Given the startup command is intentionally backgrounded and the kit documents optional UI failures as not failing sandbox creation, changing those exits to nonzero would not give a reliable backend signal and would be inconsistent with the documented lifecycle posture.

Validation: pinned HEAD is a9c77ae0407dd7bea591676f6279e9c9dbe4bb55; gh pr checks 439 --repo GSA-TTS/agentic-coding-patterns shows all 7 checks passing, including Markdown Lint and acq-kits Tests. I ran sh -n on paseo-start.sh and paseo-agent-shim, bash -n on scripts/verify, node --check on paseo-assert-daemon-config.mjs, paseo-set-worktrees-root.mjs, and paseo-register-mounts.mjs, plus git diff --check -- integrations/isolation/acq-kits/paseo; all produced no output. I confirmed the version pin remains 0.10.2 in spec.yaml and paseo-start.sh, with no 0.7.0/0.9.1 remnants in the kit. I did not run RUN_ACQ=1; live sandbox validation remains the operational gap.

Verdict: approve. Before relying on this in production-like use, a human should still run the live RUN_ACQ=1 integrations/isolation/acq-kits/paseo/scripts/verify path in an environment that can create sandboxes, because CI/offline tests do not prove the real daemon startup, port publish, relay-disabled state, or project pre-population end-to-end.

Assisted-by: OpenCode [gpt_5_5_default_v2] change-review

@mogul

mogul commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Verdict: approve. Before relying on this in production-like use, a human should still run the live RUN_ACQ=1 integrations/isolation/acq-kits/paseo/scripts/verify path in an environment that can create sandboxes, because CI/offline tests do not prove the real daemon startup, port publish, relay-disabled state, or project pre-population end-to-end.

Results of live test:

[...]
5. Paseo daemon + web UI (started by the startup supervisor, on its own)
  FAIL startup script still references removed Paseo 0.9 launch flags:
    | if ! node "$HOME/paseo-assert-daemon-config.mjs" --paseo-home "$PASEO_HOME_DIR" --listen "$PASEO_LISTEN" >>"$STATE_DIR/paseo-config.log" 2>&1; then
[...]

Investigating now.

Avoid flagging the daemon config assertion helper's --listen argument as a removed Paseo daemon launch flag, and add the same launch-surface check to the offline verifier.

Co-authored-by: OpenCode <bret.mogilefsky@gsa.gov>
@mogul

mogul commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Ready for re-review.

Follow-up after live verification found a false positive:

  • The live verifier was rejecting any --listen token in paseo-start.sh, so it incorrectly flagged the new paseo-assert-daemon-config.mjs --listen ... helper call as a removed Paseo 0.9 daemon launch flag.
  • Narrowed the removed-launch-surface detection to actual paseo daemon start usage or removed flags on a paseo daemon start|run command line.
  • Added the same launch-surface assertion to the offline verifier, so this check now runs before live sandbox verification too.

Verification:

  • GitHub PR checks are all passing at head 85b7c7f.
  • Local integrations/isolation/acq-kits/paseo/scripts/verify offline gate now passes: 7 passed, 0 failed.
  • Local make test-kits passed, including the Paseo kit tests: 13 passed, 0 failed.
  • Local syntax checks passed for scripts/verify and paseo-start.sh.

Bret is rerunning the live host-side verification after this verifier fix.

AI-assisted follow-up by OpenCode.

@mogul

mogul commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Bret is rerunning the live host-side verification after this verifier fix.

Summary
  All checks passed.

@basiliskus
basiliskus self-requested a review September 30, 2026 22:05

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

Reviewed at 85b7c7f.

I re-reviewed the follow-up fix for #439 after the live RUN_ACQ=1 verifier failed on the helper’s --listen argument. This change narrows the removed-launch-flag grep so it only targets paseo daemon start or removed flags on a paseo daemon start|run command line, and it adds the same launch-surface check to the offline verifier.

No findings.

Below the bar: I probed the new pattern against representative direct regressions and the helper invocation. It still flags paseo daemon start, paseo daemon run --listen, --web-ui, and --foreground, including with an env VAR=x prefix. It no longer flags node ... paseo-assert-daemon-config.mjs ... --listen, paseo daemon restart, or the current PASEO_LISTEN="$2" paseo daemon run --home "$X" form. The regex is still a line-oriented guard rather than a shell parser: it will not catch variable-supplied flags, flags hidden after a # inside a quoted string, or a removed flag split onto a following continuation line. I do not think that is a blocker here; the current source is clean, the common direct regression forms are covered, and a real live daemon launch regression should still fail later health checks.

I also checked equivalence between the new offline step and the live check. Offline greps files/home/paseo-start.sh; live greps /home/agent/paseo-start.sh; spec.yaml maps that source file to /home/agent/paseo-start.sh, so the offline source check should catch this class before live verification. The remaining gap is deployment fidelity: only a real sandbox run proves the file was copied and executed correctly.

Validation: pinned HEAD is 85b7c7f394ce76b1e58aee2d2e4da63331dd813f; gh pr checks 439 --repo GSA-TTS/agentic-coding-patterns shows all 7 checks passing. I ran bash -n integrations/isolation/acq-kits/paseo/scripts/verify, sh -n integrations/isolation/acq-kits/paseo/files/home/paseo-start.sh, node --check integrations/isolation/acq-kits/paseo/files/home/paseo-assert-daemon-config.mjs, git diff --check -- integrations/isolation/acq-kits/paseo/scripts/verify, and a direct grep of the current paseo-start.sh with the narrowed pattern; all were clean. I could not independently run RUN_ACQ=1 here. The author reports the live host-side verification now passes; treat that as author-reported, not independently verified by this review.

Verdict: approve. Still unverified by me: real sandbox create, shipped-file installation, daemon startup, host port publishing, relay-disabled runtime state, UI asset delivery, daemon self-heal, and project pre-population. Those are covered only by the author-reported live run in this environment.

Assisted-by: OpenCode [gpt_5_5_default_v2] change-review

@mogul
mogul merged commit ef3aa29 into main Sep 30, 2026
7 checks passed
@mogul
mogul deleted the feat/paseo-kit-bump-0.9.1 branch September 30, 2026 22:07
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