Restructure setup phases to improve failure handling and fix bugs#1883
Restructure setup phases to improve failure handling and fix bugs#1883DavidMiserak wants to merge 2 commits into
Conversation
Restructures setup into four phases ordered by failure probability so a bad network day or missing sudo never leaves users with zero skills: Phase A — trivial filesystem ops (always succeed) Phase B — local compute: binary build, skill linking, migrations Phase C — settings.json mutation (reversible) Phase D — network/sudo/package managers (best-effort, warn not exit) Playwright/Chromium failures are now non-fatal warnings with a named $_PW_FAIL_REASON. Skills, hooks, and migrations are already installed before Phase D runs; re-running ./setup retries only the network step. Also fixes two bugs found in code review: - Use bun_cmd wrapper (not bare bun) in Codex/Factory/OpenCode skill generation helpers — fixes silent failures on Windows with non-ASCII bun paths - Remove | tail -3 pipe in GBrain regen so bun exit code is preserved and the error warning actually fires on failure
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Thanks for digging into the failure-handling here — the two bug fixes are real and worth landing. Bug fixes (correct, please split out):
These two are tiny and independently testable — a focused PR landing just them (with a test that actually exercises the exit-code path, not only a source-string grep) would merge fast. The phase restructure is hard to land as-is. It moves ~200 lines and renumbers nearly every step, which (a) can't be reviewed as a behavior-preserving move because it also folds in real behavior changes — Playwright Suggested split: (1) the two bug fixes, (2) the Playwright warn-don't-exit change as its own issue-linked PR coordinated with #1838, (3) the phase reorder last, against fresh |
|
Thanks @jbetala7 — agreed on the split. Carved into three PRs as you
Closing this PR — superseded by the three above. Recommended merge order: #1898 first (tiny, independent), then |
Summary
setupinto four phases ordered by failure probability, so a bad network day or missingsudonever leaves users with zero skills installedexit 1, now a named warning with$_PW_FAIL_REASON; skills, hooks, and migrations are already in place before Phase D runsPhase structure
~/.gstack/, welcome message, GBrain detectionsettings.jsonmutation: team/plan-tune hooksPreviously, a Playwright install failure (network issue, bad mirror, missing sudo) would
exit 1mid-run, leaving skills unregistered. Now Phase D is best-effort; re-running./setupretries only that phase.Bug fixes
link_codex_skill_dirs,link_factory_skill_dirs, andlink_opencode_skill_dirswere callingbun rundirectly instead ofbun_cmd, bypassing the Windows non-ASCII path workaround. Silent failures masked by subsequent directory-existence checks.bun_cmd run gen:skill-docs:user ... | tail -3causedtail's exit 0 to swallow bun's failure, so the|| log "warning..."clause never fired. Removed the pipe.Test plan
./setupon a clean install — confirm skills link, hooks install, and Playwright failure (if any) prints a warning instead of aborting./setup --host codexon Windows with non-ASCII username — confirm Codex skill docs generate./setupwith GBrain installed — confirm warning fires ifgen:skill-docs:userfails