From 761a22b38b5445c7a4cd3585819399cb5e3d629f Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 16:17:56 +0000 Subject: [PATCH] fix: adversarial review fixes Trailer fix after a skipped upgrade is one runnable command (the refusal prose stays on the WARN behind row); the floor reader is whitespace-insensitive and names a non-version value instead of calling it absent. Found by /ship Step 11 (Claude adversarial, Codex adversarial + structured). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 4 +++- bin/gstack-capy-install | 5 ++++- bin/gstack-doctor-check.sh | 9 +++++++-- test/gstack-capy-install.test.ts | 5 +++-- test/gstack-doctor.test.ts | 6 ++++++ 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73bba91f5..1d3a4b03f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,12 @@ - **A Node-only pin mismatch is advisory** (`bin/gstack-doctor-check.sh`): when the project's Node pin is unmet by the Node on PATH while every Bun pin is met, `pins` is `WARN pins — ... (node pin only; bun pins pass); fix: ... (PROJECT_NODE_PIN_UNMET)` and the trailer stays ok. gstack runs on Bun and the project's tests choose their Node (nvm, the CI matrix). A Bun pin that is unmet stays `FAIL pins` (`PROJECT_PIN_MISMATCH`). - **WARN rows print their action as given** (`_dc_row`): `owner action: ...` for the Capy skill copy, `fix: ...` for the floor, a skipped upgrade and a Node pin, so a lane knows which WARNs it can clear itself. - **A Drive copy below the wrapper minimum is a WARN in a direct run too** (`bin/gstack-doctor-check.sh`): with no bootstrapper provenance (`bin/gstack-upgrade`'s installer run, a direct `gstack-doctor --check`), every Drive copy at a bootstrapper revision below `GSTACK_CAPY_MIN_WRAPPER` is `WARN wrapper — every Capy skill copy is below the minimum wrapper 3 (it still works): ...` with the owner action and an ok trailer; only a pre-bootstrapper copy (no `GSTACK_CAPY_WRAPPER=` line) still fails. Found by /ship's red team: raising the minimum to 3 would otherwise have failed every direct run on a machine whose Drive copy is the deployed wrapper 2 until the owner recopied the skill. -- **A `--for none` session never self-upgrades** (`bin/gstack-capy-session`): an older bootstrapper (wrapper 1 or 2) hands `--for none` to the session in install mode; the session now treats it like `--check` (reports `behind=`, moves nothing) before the installer prints `ok not-needed`. Wrapper 3 answers `--for none` before the session. The session's header comment describes the `WARN behind` outcome (it said `fail behind`). +- **A `--for none` session never self-upgrades** (`bin/gstack-capy-session`): an older bootstrapper (wrapper 1 or 2) hands `--for none` to the session in install mode; the session now treats it like `--check` (reports `behind=`, moves nothing) before the installer prints `ok not-needed`; the old bootstrapper's own clone or fast-forward of a below-minimum checkout, before the handover, is unchanged. Wrapper 3 answers `--for none` before the session. The session's header comment describes the `WARN behind` outcome (it said `fail behind`). - **/cso** (`cso/SKILL.md.tmpl`): the always-loaded startup block gains two rules. Environment hygiene: prerequisite checks read a variable as set or unset only, and `env`, `printenv`, `set -x`, a bare `set` or `export`, `export $(...)` and `echo "$NAME"` never appear in a tool fence. Tooling errors fail loudly: a non-zero exit with no helper status is retried at most twice (5 s, 30 s) and then ends **not assessed** for whatever the helper never recorded, quoting the command, exit code and stderr tail; never a quiet partial, never another review offered as the missing coverage. - **Capy skill and docs** (`capy/skill/gstack/SKILL.md`, `docs/capy-install.md`): the session-start steps cover `--for none`, `floor=`, `behind=` as informational, the Node WARN and the in-place review-voice upgrade; run directories and scratch on Capy live under `~/.capy/work/`, never `/tmp`, which Capy wipes when a machine sleeps. - **Result codes** (`lib/result-codes-capy.ts`): `PROJECT_FLOOR_UNMET`, `PROJECT_NODE_PIN_UNMET` and `UPGRADE_SKIPPED`, each with a `docs/troubleshooting.md` anchor. +- **The trailer's `fix:` after a skipped upgrade is one runnable command** (`bin/gstack-capy-install`): the session's rerun or `bin/gstack-upgrade` (which prints its own refusal), never the refusal's prose; that prose, with its `GSTACK_UPGRADE_FORCE=1` escape hatch for a live lease, stays on the `WARN behind` row (found by /ship's adversarial review: an agent told to run the `fix:` command was being handed prose with the note separator in it). +- **The floor reader is whitespace-insensitive** (`bin/gstack-doctor-check.sh`): `min_gstack_version` on one line and its value on the next, which `lib/ship-policy.ts` accepts, is read; a present value that is not a dotted version SKIPs as such instead of as "null or absent" (found by Codex in /ship's adversarial review). ### Tests - `test/gstack-doctor.test.ts`: the `floor` row's four outcomes (absent file, null key, met, unmet with the wrapper fix and without it) and the Node-only WARN beside a Bun-plus-Node FAIL; every required-row list gains `floor`. diff --git a/bin/gstack-capy-install b/bin/gstack-capy-install index 20b20af02..62975197f 100755 --- a/bin/gstack-capy-install +++ b/bin/gstack-capy-install @@ -612,7 +612,10 @@ fi if [ -n "${GSTACK_CAPY_BEHIND:-}" ]; then note "behind=$GSTACK_CAPY_BEHIND (latest: ${GSTACK_CAPY_BEHIND_TO:-${REMOTE_VERSION:-unknown}})" [ -n "$BLOCKED" ] && note "upgrade skipped: $BLOCKED_REASON ($BLOCKED_CODE)" - FIX_NOTE="${BLOCKED:+$BLOCKED_FIX}"; FIX_NOTE="${FIX_NOTE:-${GSTACK_CAPY_BEHIND_FIX:-bash /scripts/install.sh}}" + # The trailer's fix is always one runnable command (the session's: the rerun + # or bin/gstack-upgrade, which prints its own refusal); the refusal's own + # prose stays on the WARN behind row. + FIX_NOTE="${GSTACK_CAPY_BEHIND_FIX:-${BLOCKED:+$ROOT/bin/gstack-upgrade}}"; FIX_NOTE="${FIX_NOTE:-bash /scripts/install.sh}" elif [ -n "$REMOTE_VERSION" ] && [ "$REMOTE_VERSION" != "$INSTALLED" ] \ && [ "$(printf '%s\n%s\n' "$INSTALLED" "$REMOTE_VERSION" | sort -V | head -1)" = "$INSTALLED" ]; then note "latest: $REMOTE_VERSION available" diff --git a/bin/gstack-doctor-check.sh b/bin/gstack-doctor-check.sh index 1054a3c84..0851a6370 100644 --- a/bin/gstack-doctor-check.sh +++ b/bin/gstack-doctor-check.sh @@ -492,9 +492,14 @@ _dc_check_floor() { _dc_row SKIP floor "$project declares none ($GSTACK_PROJECT_FLOOR_FILE absent; set $GSTACK_PROJECT_FLOOR_FIELD there to require a gstack version)" return fi - floor=$(sed -n 's/.*"'"$GSTACK_PROJECT_FLOOR_FIELD"'"[[:space:]]*:[[:space:]]*"\([0-9][0-9.]*\)".*/\1/p' "$file" 2>/dev/null | head -1 | clamp '0-9.' 32) + # Whitespace-insensitive: the value may sit on the line after its field. + floor=$(tr '\n\r\t' ' ' < "$file" 2>/dev/null | sed -n 's/.*"'"$GSTACK_PROJECT_FLOOR_FIELD"'" *: *"\([0-9][0-9.]*\)".*/\1/p' | head -1 | clamp '0-9.' 32) if [ -z "$floor" ]; then - _dc_row SKIP floor "$file sets no $GSTACK_PROJECT_FLOOR_FIELD (null or absent)" + if tr '\n\r\t' ' ' < "$file" 2>/dev/null | grep -q '"'"$GSTACK_PROJECT_FLOOR_FIELD"'" *: *"'; then + _dc_row SKIP floor "$file sets $GSTACK_PROJECT_FLOOR_FIELD to something other than a dotted version (gstack-ship-queue rejects it)" + else + _dc_row SKIP floor "$file sets no $GSTACK_PROJECT_FLOOR_FIELD (null or absent)" + fi return fi installed=$(head -1 "$ROOT/VERSION" 2>/dev/null | clamp '0-9A-Za-z.+-' 40) diff --git a/test/gstack-capy-install.test.ts b/test/gstack-capy-install.test.ts index a04a1a2f8..d40b86ed4 100644 --- a/test/gstack-capy-install.test.ts +++ b/test/gstack-capy-install.test.ts @@ -287,7 +287,8 @@ describe('gstack-capy-install', () => { const busy = { GSTACK_CAPY_BEHIND: '3', GSTACK_CAPY_BEHIND_TO: '1.2.6', GSTACK_CAPY_BEHIND_FIX: `${f.root}/bin/gstack-upgrade`, GSTACK_CAPY_BEHIND_BLOCKED: 'live gstack sessions hold this installation: pid 379 (124m dbus-daemon); fix: wait for those sessions to finish, or re-run with GSTACK_UPGRADE_FORCE=1 when you know they are idle (UPGRADE_BUSY)' }; const r = run(f, [], busy); expect(r.status, r.stdout + r.stderr).toBe(0); - expect(lastLine(r.stdout)).toBe('gstack: ok 1.2.3 for=autoplan (behind=3 (latest: 1.2.6); upgrade skipped: live gstack sessions hold this installation: pid 379 (124m dbus-daemon) (UPGRADE_BUSY); fix: wait for those sessions to finish, or re-run with GSTACK_UPGRADE_FORCE=1 when you know they are idle)'); + // The trailer's fix is the session's one runnable command; the refusal's own prose (and its GSTACK_UPGRADE_FORCE=1 escape hatch) stays on the WARN row. + expect(lastLine(r.stdout)).toBe(`gstack: ok 1.2.3 for=autoplan (behind=3 (latest: 1.2.6); upgrade skipped: live gstack sessions hold this installation: pid 379 (124m dbus-daemon) (UPGRADE_BUSY); fix: ${f.root}/bin/gstack-upgrade)`); expect(r.stdout).toMatch(/^WARN behind — installed 1\.2\.3 is 3 commits behind origin \(latest 1\.2\.6\) and the session-start upgrade was skipped: live gstack sessions hold this installation: pid 379 \(124m dbus-daemon\); fix: wait for those sessions to finish, or re-run with GSTACK_UPGRADE_FORCE=1 when you know they are idle \(UPGRADE_BUSY; .*#upgrade-busy\)$/m); expect(r.stdout).toMatch(/^GSTACK_LAUNCH_FILE: /m); const bare = run(f, [], { ...busy, GSTACK_CAPY_BEHIND_BLOCKED: 'gstack-upgrade exit 9' }); @@ -296,7 +297,7 @@ describe('gstack-capy-install', () => { // A code the installer does not know collapses to UPGRADE_SKIPPED, whose anchor exists; the reason and fix are kept. const other = run(f, [], { ...busy, GSTACK_CAPY_BEHIND_BLOCKED: 'origin unreachable; fix: retry when online (UPGRADE_FETCH_FAILED)' }); expect(other.status).toBe(0); - expect(lastLine(other.stdout)).toBe('gstack: ok 1.2.3 for=autoplan (behind=3 (latest: 1.2.6); upgrade skipped: origin unreachable (UPGRADE_SKIPPED); fix: retry when online)'); + expect(lastLine(other.stdout)).toBe(`gstack: ok 1.2.3 for=autoplan (behind=3 (latest: 1.2.6); upgrade skipped: origin unreachable (UPGRADE_SKIPPED); fix: ${f.root}/bin/gstack-upgrade)`); expect(other.stdout).toMatch(/^WARN behind — .*; fix: retry when online \(UPGRADE_SKIPPED; .*#upgrade-skipped\)$/m); expect(lastLine(run(f, [], { ...busy, STUB_DOCTOR_FAIL: '1' }).stdout)).toBe('gstack: fail codex'); }); diff --git a/test/gstack-doctor.test.ts b/test/gstack-doctor.test.ts index 836d74e77..581957fec 100644 --- a/test/gstack-doctor.test.ts +++ b/test/gstack-doctor.test.ts @@ -672,9 +672,15 @@ describe('gstack-doctor --check', () => { expect(none.rows.floor).toEqual({ status: 'SKIP', rest: `(${project} declares none (.gstack/ship-policy.json absent; set min_gstack_version there to require a gstack version))` }); write(path.join(project, '.gstack/ship-policy.json'), '{\n "bump": "patch",\n "min_gstack_version": null\n}\n'); expect(row(f, ['--project', project]).rows.floor).toEqual({ status: 'SKIP', rest: `(${project}/.gstack/ship-policy.json sets no min_gstack_version (null or absent))` }); + // A present value that is not a dotted version is named as such, never read as "absent"; the shell never evaluates it. + write(path.join(project, '.gstack/ship-policy.json'), '{\n "min_gstack_version": "1.2.3-rc1; $(touch /tmp/pwned)"\n}\n'); + expect(row(f, ['--project', project]).rows.floor).toEqual({ status: 'SKIP', rest: `(${project}/.gstack/ship-policy.json sets min_gstack_version to something other than a dotted version (gstack-ship-queue rejects it))` }); write(path.join(project, '.gstack/ship-policy.json'), `{\n "min_gstack_version": "${VERSION}"\n}\n`); const met = row(f, ['--project', project]); expect(met.rows.floor).toEqual({ status: 'PASS', rest: `${VERSION} declared by ${project}/.gstack/ship-policy.json (installed ${VERSION})` }); + // Valid JSON may put the value on the line after its field (found by Codex in /ship's adversarial review). + write(path.join(project, '.gstack/ship-policy.json'), `{\n "bump": "patch",\n "min_gstack_version":\n "${VERSION}"\n}\n`); + expect(row(f, ['--project', project]).rows.floor.status).toBe('PASS'); write(path.join(project, '.gstack/ship-policy.json'), '{\n "min_gstack_version": "999.0.0.0"\n}\n'); const wrapper = '/drive/skills/gstack/scripts/install.sh'; const unmet = row(f, ['--project', project, '--for', 'health'], { GSTACK_CAPY_WRAPPER_PATH: wrapper });