Behavioral fixture suite + fix warm-reattach descendant walk#16
Closed
Christian-Katzmann wants to merge 1 commit into
Closed
Behavioral fixture suite + fix warm-reattach descendant walk#16Christian-Katzmann wants to merge 1 commit into
Christian-Katzmann wants to merge 1 commit into
Conversation
Add scripts/test-fixtures.sh + scripts/fixtures/ — a hermetic, CI-gated suite that drives the real launcher scripts against tiny project shapes (Vite, Next, static-export, Vite+Express, hardcoded-port, Chrome-fallback, deep-tree) and asserts the headless-automatable rows of SKILL.md's Phase-4 checklist: build, bundle metadata, no placeholder leak, runtime port, server responds, server-belongs-to-the-launcher (descendant-walk ownership), warm-reattach, and clean teardown. Hermetic by design — stand-in $PORT servers, no framework installs, a sandboxed HOME, trap-based teardown — so it never touches the user's real ~/Applications / ~/Library state. A weekly fixtures-real CI lane runs one real `npm run dev -- --port $PORT` Vite app so framework drift can't silently rot the top recipe. Add a documented APP_IT_SMOKE seam to the launcher templates so CI (and SSH debugging) can verify the dev server comes up without opening the GUI window. Fix a real bug the suite caught on first contact with a real framework: the process-ownership descendant walk (the reattach gate in every run-template, and desktop:doctor) only reached the first child generation on macOS, because `pgrep -P` returns nothing for a space-joined PID list. Warm-reattach therefore silently failed for npm/pnpm/Next/Vite (listener at gen 2+), cold-starting a new server on a new port each click instead of reusing the warm one. The walk now expands one PID per call; the deep-tree fixture guards against regression. Adopt a recipe-governance rule in CONTRIBUTING: no new framework recipe merges without a fixture or reproducible smoke test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideAdds a macOS behavioral fixture test suite that drives the real app-it launcher scripts against hermetic project fixtures, introduces a headless APP_IT_SMOKE launch seam, fixes the macOS descendant process walk so warm-reattach and ownership checks work across multi-generation trees, and wires these into CI, docs, and validation tooling. Sequence diagram for the updated descendant walk warm-reattach logicsequenceDiagram
participant Launcher_run_template_sh as run-template.sh
participant pgrep
participant DevServer
Launcher_run_template_sh->>DevServer: start dev server (supervisor PID = EXPECTED_PID)
DevServer-->>Launcher_run_template_sh: writes PORT_FILE, PID_FILE
loop up to 4 generations
Launcher_run_template_sh->>pgrep: pgrep -P EXPECTED_PID
pgrep-->>Launcher_run_template_sh: child PIDs (gen 1)
Launcher_run_template_sh->>pgrep: pgrep -P each_child_PID (one PID per call)
pgrep-->>Launcher_run_template_sh: deeper descendants (gen 2+)
end
Launcher_run_template_sh->>Launcher_run_template_sh: check if runtime-port PID is in DESCENDANTS
alt PID in DESCENDANTS
Launcher_run_template_sh-->>Launcher_run_template_sh: warm reattach to existing DevServer
else PID not in DESCENDANTS
Launcher_run_template_sh-->>Launcher_run_template_sh: start new server on new port
end
Sequence diagram for APP_IT_SMOKE headless launch in the fixture suitesequenceDiagram
actor CI as test-fixtures.sh
participant Launcher_run_template_sh as run-template.sh
participant StubServer as stub-server.js
CI->>Launcher_run_template_sh: APP_IT_SMOKE=1 ./run (bundle run)
Launcher_run_template_sh->>StubServer: start dev server (daemonized)
StubServer-->>Launcher_run_template_sh: listening on chosen PORT
Launcher_run_template_sh-->>Launcher_run_template_sh: write PID_FILE, PORT_FILE
Launcher_run_template_sh-->>CI: print URL and PID (app-it smoke: ...)
Launcher_run_template_sh-->>Launcher_run_template_sh: exit 0 (no GUI wrapper)
CI->>StubServer: curl http://localhost:$PORT
StubServer-->>CI: 200 OK
CI->>Launcher_run_template_sh: desktop-quit.sh (stop server)
Launcher_run_template_sh-->>StubServer: terminate DevServer
StubServer-->>CI: process exits, port freed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The descendant-walk logic for process ownership is now implemented in multiple places (launcher templates, desktop-doctor.sh, test-fixtures.sh); consider extracting this into a single shared helper so future fixes to the walk don’t risk drifting across copies.
- scripts/test-fixtures.sh assumes availability of several external tools (lsof, curl, PlistBuddy, npm/node, etc.); adding an upfront preflight check with clear error messages for missing dependencies would make failures easier to interpret and avoid partial runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The descendant-walk logic for process ownership is now implemented in multiple places (launcher templates, desktop-doctor.sh, test-fixtures.sh); consider extracting this into a single shared helper so future fixes to the walk don’t risk drifting across copies.
- scripts/test-fixtures.sh assumes availability of several external tools (lsof, curl, PlistBuddy, npm/node, etc.); adding an upfront preflight check with clear error messages for missing dependencies would make failures easier to interpret and avoid partial runs.
## Individual Comments
### Comment 1
<location path="scripts/lib/stub-nested.js" line_range="27" />
<code_context>
+ console.error(`stub-nested: failed to spawn node: ${err.message}`);
+ process.exit(1);
+});
+child.on('exit', (code) => process.exit(code == null ? 0 : code));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Propagating a `null` exit code from the child as 0 may mask signal-based failures.
When the child exits due to a signal, Node sets `code` to `null` and passes the reason via the `signal` argument. Converting `null` to `0` makes signal-based terminations (e.g. SIGTERM/SIGKILL) look like a successful exit, hiding underlying launcher/fixture failures. Consider instead treating `null` as a non-zero exit, or inspecting the `signal` and mapping it to an appropriate non-zero code so that unexpected terminations are not reported as success.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| console.error(`stub-nested: failed to spawn node: ${err.message}`); | ||
| process.exit(1); | ||
| }); | ||
| child.on('exit', (code) => process.exit(code == null ? 0 : code)); |
There was a problem hiding this comment.
suggestion (bug_risk): Propagating a null exit code from the child as 0 may mask signal-based failures.
When the child exits due to a signal, Node sets code to null and passes the reason via the signal argument. Converting null to 0 makes signal-based terminations (e.g. SIGTERM/SIGKILL) look like a successful exit, hiding underlying launcher/fixture failures. Consider instead treating null as a non-zero exit, or inspecting the signal and mapping it to an appropriate non-zero code so that unexpected terminations are not reported as success.
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.
What & why
app-it's core promise — double-click → server comes up on a free port → window shows the app → Cmd+Q kills everything → next click is fast — lives in deterministic, env-parameterized shell. Until now the macOS lane had only static CI checks (
bash -n,plutil,swiftc -typecheck), while the Windows lane already did a functional round-trip. This adds the missing behavioral coverage and a rule to keep the support matrix honest.What's in it
scripts/test-fixtures.sh+scripts/fixtures/— a hermetic, CI-gated suite that drives the real scripts (inspect.sh,desktop-build.sh, the generated launcher,desktop-doctor.sh,desktop-quit.sh) against tiny project shapes (Vite, Next, static-export, Vite+Express multiserver, hardcoded-port, Chrome-fallback, deep-tree) and asserts the headless-automatable rows of SKILL.md's Phase‑4 checklist: build, bundle metadata, no placeholder leak, runtime port, server responds, server-belongs-to-the-launcher (descendant-walk ownership), warm-reattach, and clean teardown.$PORTservers (scripts/lib/), no framework installs, a sandboxedHOME, trap-based teardown — so it never touches your real~/Applications/~/Librarystate. A weeklyfixtures-realCI lane runs one realnpm run dev -- --port $PORTVite app so framework drift can't silently rot the top recipe.APP_IT_SMOKEseam in the launcher templates — does everything a real Dock click does except open the GUI window, so CI (and SSH debugging) can verify the server comes up without a display. Inert on a normal launch.CONTRIBUTING.md: no new framework recipe merges without a fixture or reproducible smoke test (directly governs the open Vite/Svelte/Astro recipe issues).docs/RELEASE_CHECKLIST.md.Bug the suite caught on day one (and this PR fixes)
On first contact with a real framework, the suite surfaced a genuine pre-existing bug: the process-ownership descendant walk (the reattach gate in every
run-template*.sh, anddesktop:doctor) only reached the first child generation on macOS —pgrep -Preturns nothing when handed a space-joined PID list, which is what the walk built after generation 1. So for the common case where the dev server's HTTP listener sits deeper than one level (npm run dev→ node → vite;pnpm dev→ node → node → next-server), warm-reattach silently failed: every Dock click cold-started a new server on a new port instead of reusing the warm one. The walk now expands one PID per call; thedeep-treefixture guards against regression.Test plan
./scripts/validate.sh— passes (static gate; also bash-n's the new scripts)../scripts/test-fixtures.sh— 92/92, hermetic, leaves no processes/ports/temp dirs.APP_IT_RUN_REAL=1 ./scripts/test-fixtures.sh— real-Vite lane passes (npm install + real launch + reattach).validate.shpasses with nodesktop-icons-previewcoupling.Scope note
This PR is intentionally limited to the fixture suite + the walk fix + wiring. An in-flight icon-preview feature on the same working tree was deliberately left out so the two land independently.
Summary by Sourcery
Add a macOS behavioral fixture suite for app-it and fix process descendant-walk logic so warm reattach works reliably, along with wiring it into CI and documentation.
New Features:
Bug Fixes:
Enhancements:
CI:
🤖 Generated with Claude Code