Repository navigation
feat(paseo): bump pinned Paseo CLI to 0.10.2 - #439
Conversation
|
[Deleted review of unrelated PR. -Bret] |
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head 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 #420The title and commit message describe a two-value pin edit. Measured: So #439's own contribution is 3 lines, and the other +1110 is draft PR #420 ( That matters for three reasons:
Cleanest resolutions, either is fine: rebase #439 onto I could not reproduce the SIGPIPE finding, and I think it's wrong as statedThe existing comment reports that if tar -tf "${BACKUP_TAR}" | grep -Eq '(^|/)daemon\.log(\.|$)|…'; thensilently skips the prune branch under Why: on this host So the mechanism is real in general but bsdtar specifically doesn't exhibit it, and 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 ( Verified sound
What a 0.7.0 → 0.9.1 bump should sayTwo 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 ( Method: static analysis at |
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).
Oops, fixing that now! |
a6c9e6b to
450d77e
Compare
|
Ready for re-review. Changes since the prior review:
Verification:
AI-assisted changes by OpenCode. |
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Follow-up pass at head 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 coverageAll 5 voters converge on this independently. Verified directly: The part that makes this more than a robustness nit: Suggested minimum fix (cheap, per multiple voters): add one assertion in Medium: unconditional PID-lock deletion removes the only double-daemon guardVerified at 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 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.jsonBefore this PR, What holds up (verified independently, not just cited)
Not gated
Method: reviewed the full diff at head |
This PR's daemon-lifecycle rewrite independently fixes a live, currently-reported bug — #449AI-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 I live-verified against the real 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 |
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>
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>
|
Ready for re-review. Changes since the last review:
Verification:
AI-assisted follow-up by OpenCode. |
|
Reviewed at e4c7cd9. I reviewed this as a fresh-context
Below the bar: I did not find a current code path that still deletes a live PID lock during ordinary respawn. Validation: Verdict: Comment. The implementation looks substantially fixed, but the PR should not describe the live 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
|
Ready for re-review. Changes since the latest feedback:
Verification:
AI-assisted follow-up by OpenCode. |
basiliskus
left a comment
There was a problem hiding this comment.
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
Results of live test: 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>
|
Ready for re-review. Follow-up after live verification found a false positive:
Verification:
Bret is rerunning the live host-side verification after this verifier fix. AI-assisted follow-up by OpenCode. |
|
basiliskus
left a comment
There was a problem hiding this comment.
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
Context
Update the
paseoacq 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.2is the currentlateston 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.2includes daemon startup/config robustness fixes, project/workspace fixes, and OpenCode permission-rule fixes.0.10.0adds OpenCode v2 support, password-protected daemon connection handling, and additional daemon robustness fixes.0.10.1and0.10.2primarily fix OpenCode v2 chat, question-card, context-meter, timeout, and edit-diff behavior.The kit already uses the 0.9+ daemon approach (
paseo daemon runplus 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 commandfiles/home/paseo-start.sh— in-script fallback default_ver="${PASEO_CLI_VERSION:-0.10.2}"and its header doc commentReview feedback from wz-gsa's latest PR comments is also addressed:
paseo-start.shnow treats required daemon config persistence as fail-closed: it writesdaemon.listen,features.webUi.enabled, anddaemon.relay.enabled, reads them back fromconfig.json, and does not start the unauthenticated daemon if the required settings cannot be verified.scripts/verifynow has a live assertion that readsconfig.jsonand checks those three required daemon settings.paseo-start.shno longer deletespaseo.pidbefore everydaemon run; Paseo keeps ownership of its double-daemon guard during ordinary respawn.paseo-agent-shimonly clearspaseo.pidafter the fallback path actually kills a daemon process.paseo-start.shandpaseo-set-worktrees-root.mjsnow share a coarse$PASEO_HOME/.config-json.lockaroundconfig.jsonwrites to prevent startup config writes racing the wrapper's worktree-root write.Verification
npm view @getpaseo/cli@0.10.2 version && npm view @getpaseo/cli dist-tags.latest->0.10.2/0.10.2sh -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-> OKnode --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 failedpython3 -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-> OKintegrations/isolation/acq-kits/paseo/scripts/verify-> 5 passed, 0 failed; live sandbox checks skipped becauseRUN_ACQ=1was not set; neutral kit schema check skipped because this environment lacksjsonschemaandpyyaml0.9.1|0.7.0across the kit -> no remaining referencesmake validatewas attempted but is blocked in this environment becausepythonis not on PATH. Running the underlying validator withpython3starts but fails before repository validation because the local environment is missingjsonschema; 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/clifromregistry.npmjs.orgwith 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