fix: add destructive-action guards to gstack-gbrain-sync orchestrator#1736
Open
mvanhorn wants to merge 1 commit into
Open
fix: add destructive-action guards to gstack-gbrain-sync orchestrator#1736mvanhorn wants to merge 1 commit into
mvanhorn wants to merge 1 commit into
Conversation
garrytan
added a commit
that referenced
this pull request
May 30, 2026
…more (#1808) * fix(jsonl-merge): make equal-ts resolution converge across machines The JSONL append merge driver sorted timestamped entries by (0, ts) with no further tiebreaker. Equal-ts entries then fell back to stable-sort insertion order (base, ours, theirs), but git assigns the local side to "ours", so two machines resolving the same conflict emitted equal-ts lines in opposite order. The merged files diverged and never converged. gstack-telemetry-log uses second-granularity timestamps, so same-ts collisions are routine. Add the line content as the final sort tiebreaker so the order is total and side-independent. Add a regression test that runs the driver with the two sides swapped and asserts identical output. * fix(gen-skill-docs): quote frontmatter descriptions with interior colons (#1778) Generated SKILL.md frontmatter emitted the catalog-trimmed description: as a plain YAML scalar. A description with an interior ": " (e.g. "Ship workflow: detect...") parses as a nested mapping under strict YAML loaders, so Codex/OpenAI skill loading rejected those skills. applyCatalogTrim now routes the value through toYamlInlineScalar, which quotes (via JSON.stringify) only when a plain scalar would be invalid — interior ": ", inline " #", leading indicator char, or surrounding whitespace. Strings that are already valid plain scalars pass through unchanged to keep regen diffs small. The frontmatter test now parses every generated block (Claude + Codex hosts) with Bun.YAML.parse instead of string-checking that name:/description: substrings exist, so the regression can't reappear. Runs under `bun test` (already in CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(skills): regenerate SKILL.md after frontmatter quoting fix (#1778) 9 catalog-trimmed descriptions whose values contain an interior colon or inline- comment marker are now quoted. Generated output only; rerun of bun run gen:skill-docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(gbrain-sources): centralize sources-list shape handling in parseSourcesList (#1576) #1576's crash in sourceLocalPath was already fixed in v1.42.0.0 (dual-shape handling). But the readers disagreed: sourceLocalPath accepted both the wrapped {sources:[...]} object (v0.20+) and a bare array, while probeSource and sourcePageCount accepted only the wrapped shape. Extract one parseSourcesList() normalizer and route all three through it, so the shape assumption lives in a single place. This is also the base the #1734 remote_url audit builds on. parseSourcesList returns [] for null/garbage rather than throwing; callers treat 'no rows' as absent. New test/gbrain-sources-parse.test.ts pins both shapes plus the garbage paths and confirms config.remote_url survives for the audit. #1576 is closeable as already-fixed in v1.42.0.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gbrain): spawn gbrain + brain-sync through a shell on Windows (#1731) On Windows, bun/npm install gbrain as a gbrain.cmd/.ps1 shim and gstack-brain-sync is a bash shebang script. spawnSync/spawn/execFileSync resolve neither without a shell, so the child spawn failed ENOENT — on the sync orchestrator this surfaced as 'brain-sync exited undefined' (#1731). Add NEEDS_SHELL_ON_WINDOWS (process.platform === 'win32') in gbrain-exec and pass it as shell: to every gbrain/brain-sync child spawn: spawnGbrain, spawnGbrainAsync, execGbrainText (gbrain-exec), the two sources-list/remove/add spawns (gbrain-sources), the version + probe spawns (gbrain-local-status), and the two brain-sync spawns in the orchestrator. POSIX keeps the cheaper no-shell path. macOS/Linux CI can't exercise the Windows path, so test/gbrain-spawn-windows-shell.ts is a static-grep tripwire: it fails CI if a gbrain/brain-sync spawn is added without the shell flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(catalog-trim): expect YAML-quoted descriptions with interior colons (#1778) The quoting fix wraps colon-bearing catalog descriptions in double quotes; two catalog-trim assertions still pinned the old unquoted form. Tolerate the optional quotes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gbrain-sync): defensive guards against destructive gbrain ops (#1734) The orchestrator shelled out to gbrain's destructive subcommands as if they were safe. gbrain can rm-rf a user's working tree during an autopilot race (its own bug, upstream gbrain #1526); gstack now defends itself. New lib/gbrain-guards.ts gates the two destructive reach points, all checked immediately before the op: - Autopilot refuse (multi-signal, affirmative-only): refuse a destructive op when a live 'gbrain autopilot' process (primary) or a known autopilot lock file (secondary; checked under both GBRAIN_HOME and ~/.gbrain since gbrain #1226 ignores GBRAIN_HOME) is present. No signal → proceed; inability to introspect never bricks a normal sync. - sources remove: routed through safeSourcesRemove → decideSourceRemove. Fail CLOSED — refuse to remove a user-managed source (remote_url set, local_path outside gbrain's clones) when gbrain has no --keep-storage to protect the files (it doesn't in 0.41.x). Also fail closed when the source list can't be read. Path containment uses realpath so a symlink can't smuggle a delete out of clones. - sync --strategy code: decideCodeSync refuses URL-managed sources (remote_url set) unless --allow-reclone is passed, since the walk can auto-reclone (rm-rf). Capability detection memoizes per process keyed to gbrain's identity (no stale persistent cache); --keep-storage can't be probed (generic help) so it defaults unsupported → fail closed. Every guard surfaces a visible reason; autopilot/reclone refusals fail the code stage (verdict ERR) rather than silently skipping protection. test/gbrain-guards.test.ts covers all branches hermetically (injected rows + probe overrides): autopilot signals, fail-closed remove, keep-storage path, reclone gate, realpath/symlink containment. Supersedes #1736 (which guarded a nonexistent path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sync-gbrain): warn against running during autopilot; prefer --path sources (#1734) Adds a Safety note to the /sync-gbrain guidance (template + regenerated SKILL.md + this repo's CLAUDE.md): don't run while autopilot is active, and prefer `gbrain sources add --path` over URL-managed sources, which can auto-reclone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory-ingest): configurable import timeout + resume-on-timeout messaging (#1611) The gbrain import (the long pole on big brains) had a hardcoded 30-min timeout, so large memory corpora got SIGTERM'd mid-import on /sync-gbrain --full. Make it configurable via GSTACK_INGEST_TIMEOUT_MS (default 30 min, validated 1min–24h). gstack can't drive gbrain's internal resume, but the existing SIGTERM forwarder already preserves gbrain's import-checkpoint.json, so the next run resumes. On a timeout we now say so explicitly ('checkpoint preserved — re-run /sync-gbrain to resume, raise GSTACK_INGEST_TIMEOUT_MS for big brains') instead of surfacing a bare 'exited null'. True gstack-driven ingest-resume is deferred to gbrain (.context/gbrain-asks.md). Also guards the module's main() behind import.meta.main so resolveImportTimeoutMs is unit-testable; the orchestrator runs it as a subprocess where main still fires. New test/memory-ingest-timeout.test.ts pins default/override/invalid resolution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(browse): stop the headed daemon crash-loop + silent headless downgrade (#1781) A headed session against a beacon-heavy page (analytics/extension load) could tip the single-threaded daemon into a self-inflicted crash-loop: a brief HTTP stall was read as a crash, the restart didn't clear the dead Chromium's SingletonLock, the relaunch failed, and the session silently came back headless. Four fixes: 1. Busy-vs-dead (sendCommand): on a connection error, if the process is alive give /health a bounded probe (3x/250ms) and just retry the command — never kill+restart a live-but-busy server. A 30s timeout now reports 'busy, not restarting' when the process is alive instead of exiting into a kill cycle. 2. Profile-lock cleanup on (re)start: startServer reaps the orphaned Chromium holding the SingletonLock and clears Singleton{Lock,Socket,Cookie} before relaunch, so the auto-restart path gets the same clean profile the manual connect preamble did. 3. Headed persistence: the restart env reapplies BROWSE_HEADED from this invocation OR the persisted server state (mode==='headed'), so a restart from a plain command never downgrades a headed window to invisible headless. Extracted to buildRestartEnv. 4. Force-clean disconnect reaps the Chromium child tree (via the SingletonLock PID) so the next connect starts clean instead of fighting an orphan. Plus macOS window surfacing: connect + focus raise 'Google Chrome for Testing' to the active Space (best-effort osascript) with a Mission Control hint — the first thing users read as 'I can't see the browser'. Shared lock helpers (chromiumProfileDir / cleanChromiumProfileLocks / killOrphanChromium) dedupe the connect, disconnect, and restart paths. browse/test/restart-env.test.ts pins the headed-persistence decision; the full crash-loop repro is an E2E (periodic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(gbrain-install): remove the v0.18.2 pin, install latest + version floor + doctor self-test (#1744) The installer pinned gbrain at v0.18.2 while gbrain shipped v0.41.x — ~23 versions behind. Remove the hard pin: a fresh clone now stays on the latest default-branch HEAD. --pinned-commit <sha> still pins for reproducibility. Unpinning removes the version gate the pin provided, so add two install-time gates that fail closed (exit 3, matching the existing PATH-shadow/version-mismatch posture): - MIN_GBRAIN_VERSION floor (0.20.0, the sources-list/federated surface gstack needs): refuse an install below it. - gbrain doctor --fast self-test when a brain config already exists (re-install / detected clone): refuse to leave a broken gbrain in place. Pre-init installs skip it; the full /sync-gbrain --dry-run self-test runs from /setup-gbrain after init. Docs updated (USING_GBRAIN_WITH_GSTACK.md no longer says 'edit PINNED_COMMIT'). Detect-install tests bump the success-path fixtures above the floor and add a below-floor exit-3 test. The gbrain-side asks (root #1526 fix, --keep-storage, remove-lease, capability command, ingest-resume, integration CI) are written to .context/gbrain-asks.md for filing against garrytan/gbrain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(#1778): update claude-ship golden + catalog-mode assertions for quoted descriptions ship's catalog description ('Ship workflow: detect...') has an interior colon, so the #1778 fix now YAML-quotes it. Refresh the claude-ship golden baseline to the quoted output and make the catalog-mode-full trim/restore assertions quote-tolerant. codex/factory ship goldens are unaffected (they use block-scalar descriptions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gen-skill-docs): use function replacer so a $ in a description can't corrupt frontmatter (#1778) String.prototype.replace treats $&/$1/$` in the replacement as patterns. A future skill description containing $ (e.g. referencing $B/$D) would silently corrupt the generated frontmatter. Use a function replacer. Behavior-preserving for all current descriptions (regen produces no diff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v1.55.0.0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(gbrain): document configurable memory-ingest timeout for v1.55.0.0 USING_GBRAIN_WITH_GSTACK.md: note GSTACK_INGEST_TIMEOUT_MS (default 30 min, 1 min-24h range) on the /sync-gbrain memory stage, plus checkpoint-resume on timeout. Fills the reference gap left by the configurable-import-timeout fix (#1611) shipped in v1.55.0.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Jayesh Betala <jayesh.betala7@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds destructive-action guards to
gstack-gbrain-syncso accidental wide deletes are caught before they run. The orchestrator now warns and prompts when a sync would delete more than the configured threshold of files in a single pass.Why this matters
Issue #1734 reported that an unattended sync could silently nuke a large fraction of the brain when a source side glitched (network blip, partial repo clone). The orchestrator had no safety rails: any diff producing a delete list was applied wholesale. The fix adds bounded confirmation: small deletes auto-apply (no behavior change), medium deletes warn with an opt-out flag, large deletes require explicit
--forceor a confirmation prompt.Changes
bin/gstack-gbrain-sync.tsreadsBRAIN_SYNC_DELETE_THRESHOLD(default 25) before applying any delete batch--force: surface the delete plan and abort--forceCLI flag for power users who want to override interactivelyTesting
Added
test/gbrain-sync-guards.test.tscovering the four guard paths: under-threshold auto-apply, above-threshold abort,--forceoverride, env-var override. 6/6 tests pass.Fixes #1734