From ff5fbd09ee5945524762dd30611a51258228b209 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:26:14 +0000 Subject: [PATCH 001/207] docs: approved severe fix wave plan (Oct 2026) --- docs/designs/SEVERE_FIX_WAVE_2026_10.md | 295 ++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 docs/designs/SEVERE_FIX_WAVE_2026_10.md diff --git a/docs/designs/SEVERE_FIX_WAVE_2026_10.md b/docs/designs/SEVERE_FIX_WAVE_2026_10.md new file mode 100644 index 0000000000..a7ac57c3bb --- /dev/null +++ b/docs/designs/SEVERE_FIX_WAVE_2026_10.md @@ -0,0 +1,295 @@ + +# gstack severe fix wave (October 3, 2026): no silent failures, nothing lost, every host works + +## Implementation plan + +### Context + +Base: origin/main `ac20ef1e1` (v1.91.16.0). Main CI is green. The weekly periodic eval lane and gate census have been red for five straight weeks (#2755, #2756). + +Triage on 2026-10-03 covered all 299 open issues and 493 open PRs against current main. Each S1/S2 claim was checked with a code read or a local probe, and the evidence is recorded in `~/.capy/work/wave/triage-*.md`. 48 issues were already fixed on main and have been closed with evidence. About 190 open PRs are superseded or junk. + +The previous two waves (#3013, #3014) fixed install/upgrade safety, worktree skill start, Codex model precedence and review gates. This wave targets what is left. The remaining severe bugs fall into four patterns: + +1. **Silent loss.** gstack drops user data and reports success: transcripts never ingested, paid design variants overwritten, project state split across worktrees. +2. **False "all clear".** A check that did not run reports ready or pass: Codex reviews whose sandbox could not start, a retired Codex model, a design binary that dies on launch, search scripts that exit 0 when bun is missing. +3. **Non-Claude hosts half-work.** Codex and the other env-var hosts lose every `$GSTACK_*` path between shell calls, so skill-end, browse, design and state-root all break after the first block. +4. **Push blockers and platform gaps.** Redaction false positives block legitimate pushes, Windows /freeze denies every edit, and /ship stops in repos without a VERSION file. + +### Release promises (the finish line) + +The wave is done when each promise holds and is backed by a test that fails on main today: + +- **P1. Nothing is silently lost.** A transcript, design variant or learning that gstack could not store is reported and retried, never marked done. +- **P2. No false all-clear.** Every gate that could not run says "not run" or "unavailable" with the reason, never "ready", "pass" or "no results". +- **P3. Every generated skill resolves its paths on every env-var host.** Every fenced bash block of every generated skill passes a fresh-shell conformance test on all 10 env-var hosts, and Codex (the one host we certify live) runs a multi-block skill end to end. Host tiers stay as declared in `scripts/host-config.ts` (Claude full, Codex experimental, others best-effort); this wave does not promise feature parity beyond paths, sections and discovery. +- **P4. Legitimate work is not blocked, and real secrets still are.** Known redaction false positives, Windows drive-letter paths and versionless repos stop blocking /ship, /freeze and pushes. Every redaction relaxation ships with true-positive controls that must still flag. +- **P5. Local control surfaces accept only gstack's own clients.** The browse terminal-agent socket rejects foreign extensions, a project's `.env` cannot loosen security settings, and navigation (including redirects) rejects link-local and metadata addresses (D0, D1, D2). Connection-time DNS rebinding is not covered by this release. + +### Three invariants first (they turn the promises into enforced properties) + +Most bugs in this plan are instances of three missing invariants. Build these first; the instance fixes then land against them. + +- **INV-1. Gate outcome contract.** One typed outcome for every gate and outside review: `ran(findings) | not_run(reason) | unavailable(reason)`. "pass" requires positive evidence the check executed. All Codex invocation sites route through `lib/outside-review-result.ts`, which takes text, stderr and exit status (B1). A free lint fails on `2>/dev/null || exit 0` in `gstack-learnings-search` and `gstack-timeline-read`, and on calls to an explicit list of gate binaries wrapped in `|| true`; best-effort tails elsewhere carry a same-line reason. + - **States.** `not_run` means gstack chose not to run the check (no plan bound, feature disabled, prerequisite missing on purpose). `unavailable` means it tried and could not (sandbox, retired model, binary died, bun missing, network). Every outcome this wave adds or changes maps to one state, a reason code and a fix action in a table in `docs/troubleshooting.md`. + - **What users see.** Every outcome that is not `ran` prints one line: `: not run (). Fix: ` or `: unavailable (). Fix: `, with the real values (exit code, path, host). Existing machine markers (`OUTSIDE_STATUS`, `CODEX_MODE`, `DESIGN_READY`) keep their names. + - **What callers do.** A new `classifyOutsideReview({text, stderr, exit})` returns three separate fields: execution (`ran | unavailable(reason)`), findings (highest severity, or none), and verdict (`clean | findings | unverified | unavailable`); `validateOutsideReview` stays as a wrapper so the three direct importers in `claude-code/SKILL.md.tmpl:130,198,268` keep working. The two-argument CLI form keeps today's exit codes exactly (0 completed, 1 unavailable, 2 usage), because `lib/` runs live from the checkout and older rendered skills call it with `|| exit 1`. With `--verdict` (implied by `--stderr` or `--exit`) it prints `VERDICT: ...` and exits 0 / 3 / 4 / 1; every generated caller moves to that form in the same commit and branches on it explicitly. `unverified` covers a completed structured review with neither severity tags nor an explicit no-findings line (/codex review's existing UNVERIFIED state). /codex consult and challenge use a new `execution` gate that checks only execution evidence, not review markers. Reason codes live in `lib/gate-outcomes.ts`, imported by the validator and by the troubleshooting anchor test. A test runs an old rendered caller against the new lib. /ship and /review treat `unavailable` as missing coverage: they continue, show it in the readiness dashboard and the PR body, and never count it as a pass. A `findings` verdict with P0/P1 blocks exactly as a native P0/P1 does. Tested through the real CLI and the generated caller together. +- **INV-2. Write, verify, then stamp.** Storage marks work done only after confirming it landed: memory ingest (A1), design variant rounds (F4), learnings/question logs (G3). Staged, published and indexed are separate states. +- **INV-3. Host conformance test (free suite).** For every host with `usesEnvVars: true`, render all generated SKILL.md and section files, run `bash -n` on every fenced bash block, and execute each block's root-resolution lines in a fresh `env -i`-style shell against a temp install root with stub binaries. Fail on any empty expansion or a path starting with `/bin/`, `/browse`, `/design` or `/gstack-`; fail if any Codex SKILL.md is a symlink or any host SKILL.md exceeds 160,000 bytes. A generated-bash lint in the same suite fails on a `~/` path inside double quotes and on `mktemp` without a `${TMPDIR:-/tmp}` template. Both fail on origin/main today. Details: install-level checks extend the existing `test/host-conformance.test.ts` (codex, kiro, factory, opencode, cursor, copilot: no installed SKILL.md is a symlink, design/make-pdf dist present, every section pointer resolves to a readable file); render-level checks cover all 10 env-var hosts in one new test that renders once per host. Each fence that uses `GSTACK_*`, `$B` or `$D` must carry the shared prelude exactly once, and the prelude is executed under `env -i` and `set -u` with stub binaries, including the B/D consumers. Angle-bracket placeholders are normalized before `bash -n` (231 of 10,136 fences fail today only because of them). The lint tokenizes quotes and fails on: a `~/` word inside double quotes; `mktemp` with no template or a literal `/tmp/` template; `cd`, `rm -r*`, `mv` or `git -C` on a variable not assigned in the same fence unless written `cd -- "${VAR:?...}" || exit 1`. The lint and the size gate run on every host's render, Claude included. Sizes are UTF-8 bytes. + +### Delivery tiers (one PR; the tiers set order and what drops if it slips) + +- **Tier 1, must ship:** C9 (first: it can damage a user's repo today), INV-1/2/3, I1 baseline, A1 (with reconcile), A3, A4, A9, B1 (with the P0 gate bug), B11, C1-C4, D1, D2, E1, F3, F4, G1, G3, H, plus the dotenv hardening below. Each release promise lists the Tier 1 items and tests that prove it (a promise-to-test matrix in the PR body). +- **Tier 2, ship if green:** the rest of A-H. +- **Tier 3, moved to TODOS.md if it slips:** E3 (Windows Node CLI lane), C8's routing-wording change (the disabled-skill half is Tier 2), G6 riders. Nothing in Tier 3 blocks the PR. +- Cuts happen by dependency-complete group (for example "all of A1+A4+reconcile" or none), never half a group; cutting a group rewrites its release promise in the CHANGELOG. +- Land C1 before C4, and C2/C3/F5 before the live Codex check. Rebase on main daily. + +### Workstream A: data loss and privacy in memory ingest and gbrain (P1) + +- **A1. #2778 (S1, data loss):** `bin/gstack-memory-ingest.ts:1127-1140` only reads `~/.gbrain/sync-failures.jsonl`, which gbrain writes only for git import dirs. A page gbrain skipped from the staging dir is stamped ingested and never retried (field reports: 37 and 69 lost entries). Main's count guard (`memory-ingest.ts:2507-2535`) counts gbrain's exit-0 "skipped" as landed, and gbrain stops printing per-file warnings after 5 per class, so stderr parsing alone is not enough. Fix: stamp a page only when (a) no per-file failure names it (gbrain `--json` failures when available, else stderr `Skipped :` lines) and (b) a landing check confirms the page exists with the staged content hash (batched lookup if the installed gbrain supports one, else a bounded number per run with the rest carried). An unnamed failure refuses the whole batch. Remote-HTTP mode records "staged" rather than "ingested" (`memory-ingest.ts:2302`). A `--reconcile` pass (run once automatically after upgrade, then on demand) re-checks stamped entries and un-stamps pages absent from the brain so already-lost transcripts re-ingest. Refused and re-queued pages print even under `--quiet`. If the installed gbrain has no page lookup, pages that passed (a) are stamped `unverified`, not `ingested`, and are re-checked when a lookup becomes available; check gbrain's capability before implementation and record the minimum version in the plan. `--reconcile` is a fourth mode beside `--probe|--incremental|--bulk`, listed in `--help`, supports `--dry-run` and `--limit N`, is bounded, resumable and idempotent, backs up the state file first, and prints `reconcile: checked N, present M, re-queued K, not yet checked L (run again to continue)`. The upgrade migration only records that a reconcile is pending; the next ingest run does one bounded pass and prints the summary even under `--quiet`. Setup never calls gbrain for reconcile. Engineering contract: (1) a versioned state schema with an explicit transition table (`staged → imported_unverified → ingested`, plus `refused`, `quarantined`, `unrecoverable`), migrated from today's file; (2) the source fingerprint is captured from the exact bytes parsed, so a transcript that grows during import is not stamped as its newer self; (3) each entry records its gbrain `source_id` (`default` for entries stamped before this release), and the landing check and `--reconcile` query only the recorded source, so pre-wave pages are never re-ingested into new sources; (4) a lookup error or `pglite_busy` means "not yet checked", never absent; reconcile re-queues a page only when it is missing from its recorded source and the transcript file still exists, otherwise it counts it `unrecoverable`; (5) presence is checked with one `gbrain list` per source per run, content with `gbrain get --json` for at most 50 pages per run against a `gstack_content_sha256` frontmatter field gstack writes (gbrain stores its own hash), and pages over the cap are `imported_unverified` and never re-imported; (6) after 3 refused batches the batch is split in halves to find the bad page, which is quarantined with a reason printed even under `--quiet`; (7) one lock protects every writer (ingest, reconcile, migration flags) and a state-save failure fails the run; (8) this logic lives in a new `lib/memory-ingest-landing.ts` (ratchet new module, ≤800 lines). Capability facts: gbrain installs at latest HEAD (floor 0.20.0); `import --json` reports `failures` from 0.48.5.0 and `source_id` from 0.50.1.0; managed-import `Pending:` receipts count as unchanged, so the landing check is needed even with `failures`; capability is detected by behavior. Fix the stale comments at `memory-ingest.ts:912-919,1127-1135`. Fold in PR #2778 with co-author credit. +- **A2. #2884:** a slug starting with a dot (`.claude`) is pruned by gbrain's walker, so the ingest refuses forever (`memory-ingest.ts:1055`). Normalize staged filenames, and map state recorded under the old filename so no page is duplicated or re-ingested. Also `lib/gbrain-local-status.ts:484` treats DNS failure (`ENOTFOUND`, offline sandbox) as `broken-db` and routes users to "move your config aside". Classify network errors as a new `db-unreachable` status with the reason `database host unreachable ( ); your gbrain config is unchanged. Fix: check network or VPN, then re-run /sync-gbrain.` For skill rendering it counts as configured-but-transient like `timeout`, so running setup offline does not strip brain-aware blocks. Fold in PR #2884. +- **A3. #1917:** with a PGLite brain, `lib/gbrain-exec.ts:111-125` returns the caller's env unchanged, so a project's `.env` `DATABASE_URL` (Bun autoloads it) reaches gbrain: false `broken-db` at best, gbrain connecting to the app's own Postgres at worst. Strip inherited `DATABASE_URL`/`GBRAIN_DATABASE_URL` when the config is PGLite, and run the gbrain child from a working directory that is not the user's project so gbrain cannot reload the project's `.env` / `.env.local`; keep honoring the explicit `GSTACK_RESPECT_ENV_DATABASE_URL` override. Tested with a real child launched from a project containing conflicting dotenv files. PR #1931; the same fix exists in Garry's stale PR #2971. +- **A4. #2140:** `gbrain import` at `memory-ingest.ts:1967-1973` passes no `--source-id`, so transcripts can land in the shared/federated default source. The ingest walks transcripts across all projects (`memory-ingest.ts:439`), so a single destination id would misattribute other projects' transcripts. Partition the batch by each transcript's originating repository and import each partition into that repository's own transcript source, registered before import as machine-local and non-federated (id via the existing `constrainSourceId`) unless the user gave publication consent; if the installed gbrain cannot register sources, the partition stays local and the run says why; transcripts with no attributable repository stay local and are reported, never sent to the default or federated source. The existing `--include-unattributed` flag (which today sends them to the default source) instead imports them into a machine-local source `gstack-transcripts-unattributed` that is never federated, refuses when the brain is remote (Postgres or HTTP), and keeps those pages out of the publishable `transcripts/**` staging that brain-sync pushes; `--help` says so. Take the source-id half of PR #2232. +- **A5. #2670:** brain-sync reports "curated artifacts pushed" on git success with zero indexed pages (`bin/gstack-gbrain-sync.ts:1648`). Fail loud when the source has zero pages. PR #2670. +- **A6. #2688:** a deleted Conductor worktree leaves a `gstack-code-` gbrain source, so autopilot fails on it forever. Source ids deliberately distinguish machines sharing a federated brain (`gstack-gbrain-sync.ts:540`), so local absence is not proof a source is dead. First, gstack's sync stage skips sources whose path is missing locally and reports "path unavailable" once instead of failing forever (gstack only detects gbrain's external autopilot and cannot reschedule it, so the plan relies on the skip in gstack's own stage). Indexed pages of a deleted worktree may be the only copy of work that was never committed, so this wave never deletes them automatically. A new explicit `gstack-gbrain-sync --prune-gone-worktrees` (with `--dry-run`, which is also what it prints first) removes a source only when recomputing `deriveCodeSourceId()` for its recorded path on this host yields its id (so it was created on this machine), the path has been absent on two consecutive syncs, the repository's git common dir still exists and no longer lists the worktree, and the parent directory is readable. It re-checks existence immediately before `safeSourcesRemove` and logs each removal. The sync output names the command when unavailable sources exist. +- **A7. #2783 (cost):** `/sync-gbrain` forces a full `dream` cycle (~35 min, LLM phases) when it only needs `resolve_symbol_edges`. Scope it, only when the installed gbrain supports the phase-scoped dream (detected, not assumed); otherwise skip the forced dream and say the full cycle costs about 35 minutes and how to run it. PR #2783. +- **A8. #1437:** the v1.27.0.0 migration passes a bare repo name to `gh repo rename`, which always fails, then points the artifacts remote at the new name anyway. Rewrite the remote only after the rename succeeded; on upgrade, restore the old URL when only the old repo exists, and say so. Adopt #1437 with credit. +- **A9. New (found in DX review): `memory-ingest --no-write` destroys future ingestion.** Its help calls it a dry run, but it stamps every prepared page `ingested_at` and saves state (`bin/gstack-memory-ingest.ts:2054`), so a later real run skips those pages forever. `--no-write` leaves state untouched; help and docs say what it does. Test: a `--no-write` run followed by a real run imports every eligible page. About a dozen existing tests use `GSTACK_MEMORY_INGEST_NO_WRITE=1` to seed state; they move to an explicit test seam (`--stamp-only` or a fake gbrain) in the same commit, because the contract changes, and the CHANGELOG lists it under behavior changes. + +### Workstream B: no false all-clear (P2) + +- **B1. #1892 (S1):** when Codex's bwrap sandbox cannot start (containers, devcontainers), every Codex command fails, but `lib/outside-review-result.ts` accepts "could not run commands... no issues found" as `completed:true, gate:pass` (probe). Under INV-1, `lib/outside-review-result.ts` takes text, stderr and exit status and returns `unavailable(sandbox)` when stderr (or a non-zero exit) shows sandbox setup failure (bwrap, user namespaces, landlock, seccomp), or the review text contains an exact execution-failure phrase (commands could not run, the diff could not be read). Sandbox keywords in the review text alone never trigger it, so a real review of sandbox-related code still passes (fixture required). Positive evidence: Codex runs with `--json`, and a review counts as executed only when at least one command or diff read succeeded in its event stream; phrase matching is the last fallback. `codex sandbox` exists in codex-cli 0.160.0; captured fixtures (Docker default seccomp: exit 1 `bwrap: No permissions to create new namespace`; bubblewrap missing: exit 101) map to `sandbox_unavailable`, so the free preflight ships. It runs before the paid model probe, and an inconclusive model probe (timeout, network) reports `CODEX_MODE: unverified` instead of `ready`. A 404 message names both causes: a retired model or a wrong `base_url` on a custom provider. The message reads `Codex outside review unavailable: Codex's sandbox could not start here (). No review ran; this is missing coverage, not a pass. Fix: enable unprivileged user namespaces for this container, or set GSTACK_CODEX_NO_SANDBOX=1.` Events `codex_sandbox_unavailable` and `codex_model_retired` are logged. Every Codex site routes through it: `scripts/resolvers/outside-voice.ts`, `codex/sections/review-mode` (which today has its own prose gate at `review-mode.md.tmpl:124-139`), `challenge-mode`, `consult-mode` and the /review adversarial pass; a static test fails if any template invokes `codex exec`/`codex review` without it. `GSTACK_CODEX_NO_SANDBOX=1` (exactly `1`, like `GSTACK_CHROMIUM_NO_SANDBOX`, env var only) is honored only when the user sets it; Codex has no read-only mode without a sandbox, so it switches every site to `sandbox_mode="danger-full-access"` and prints on every use `WARNING: GSTACK_CODEX_NO_SANDBOX=1: Codex runs this review without a sandbox and can read and write anything your user can. Use it only inside a container you trust.` It is recorded in the review log; the hint names it only after a sandbox failure. On Linux, a free `codex sandbox ... true` preflight (10-second bound) runs before the first paid Codex call; a matching failure sets `CODEX_MODE: sandbox_unavailable` and skips the paid call; any other result defers to the post-run classification. Fixtures are checked in under `test/fixtures/codex-sandbox/` with their capture command. A clean "I did not find any issues" with empty stderr still passes. +- **B1b. New (found in CEO review):** the `structured` gate fails only on `[P1]` findings (`lib/outside-review-result.ts:20`), so a review reporting `[P0]` findings passes. Fail on P0 and P1. +- **B2. #2843:** a retired-model 404 ("does not exist") falls through the probe classifier to fail-open `ready` (`bin/gstack-codex-probe:165`). Classify 404 model errors as `model_unusable`. PR #2843. +- **B3. #2192:** the Codex auth probe ignores custom OpenAI-compatible providers (`config.toml` `env_key`), so /codex reports AUTH_FAILED for working setups. PR #2192. +- **B4. #1076 + #1254:** DESIGN SETUP declares `DESIGN_READY` when the binary is merely executable (`scripts/resolvers/design.ts:1124-1127`); a binary that SIGKILLs on launch still passes. Print DESIGN_READY only when `"$D" --version` exits 0 within 10 seconds (using the existing portable timeout wrapper, since macOS has no `timeout`; a success is cached by the binary's inode and mtime so Gatekeeper's first-launch scan is not re-paid), else `DESIGN_NOT_AVAILABLE: --version exited ` (or `timed out after 10s`), adding on exit 137 `(killed at launch; on macOS usually an invalid code signature). Fix: cd && ./setup`. Setup's arm64 re-sign keeps a backup, restores it when signing fails, and reports design unavailable when the post-sign exec probe returns 137 (today it exits 0). +- **B5. #2790:** `gstack-learnings-search` and `gstack-timeline-read` end with `2>/dev/null || exit 0`, so a missing bun looks like "nothing recorded". Exit non-zero with `bun not found` when bun is missing and keep stderr with a non-zero exit when the embedded script fails; only a successful run with no matches exits 0 with empty output. The message reads `