feat: add desktop:doctor self-diagnosis command for generated apps#14
Merged
Conversation
Generated app-it apps can now self-diagnose long after the build session, with no agent in the loop. `scripts/desktop-doctor.sh` (wired as `desktop:doctor`) inspects one launcher and prints a short, issue-ready report on the things app-it actually cares about: config + placeholder leakage, installed/build .app, Info.plist identity, ad-hoc signature, quarantine/iCloud xattrs, preferred-vs-runtime port, stale PID, whether the process on the runtime port is genuinely in the recorded supervisor's descendant tree (reuses the launcher's reattach gate), start-command binary resolution on the launcher's PATH, log/state paths, and template drift (feature-probes the installed wrapper/run against the current templates via the `grep -qboa` idiom — no version stamp needed). `--tail[=N]` appends the launcher log. This is the user-facing embodiment of Core principle #8 (runtime truth beats build-time guess). Hard rules honored structurally: - Diagnostic, not fixer: read-only by default; deterministic and local (no network, no new dependencies); says "probably" when a check can't be certain. - `--fix-safe` is the only mutating mode and is deliberately narrow — it touches ONLY app-it's own generated state (stale pid/port files, this bundle's stale LaunchServices registration, the rebuilt icon, quarantine on the generated .app). Never the user's product code, dependencies, framework config, or a running server. Scoped to the macOS app-it plugin; the app-it-static companion has a different runtime model and would need its own checks. Docs: SKILL.md (templates list, Phase 3, script naming, report, a new "Diagnosing a generated app" section, frontmatter), the user-facing desktop-launcher doc template, docs/TROUBLESHOOTING.md (leads with it), and CHANGELOG. validate.sh requires the new template and bash -n covers it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements a new desktop:doctor diagnostic command for macOS app-it launchers, wiring it into the app-it template/tooling/docs and providing an optional --fix-safe path that only mutates app-it–generated artifacts. 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 2 issues, and left some high level feedback:
- The
LATEST_NVM_NODEselection usessort -V, which is not supported by BSDsorton macOS and will fail on many default setups; consider switching to a portable sort (e.g.,sort -t. -k1,1 -k2,2 -k3,3or usingls -tand picking the first directory) or delegating to a small Node/Python helper. - The
walk_descendantshelper assigns potentially multiple child PIDs intocurrentand then passes that string topgrep -P, butpgrepexpects a comma-separated list, not a space-separated string; this can cause the descendant walk to stop after the first generation, so it would be safer to iterate per PID or normalize to the expected format. - The launcher PATH emulation in
LAUNCHER_PATHappends the current shell’s$PATH, which may differ substantially from the bare PATH used by the real desktop launcher; to avoid false positives/negatives in the start-command resolution check, consider deriving this exactly from therun-template.shimplementation rather than duplicating it with$PATHappended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `LATEST_NVM_NODE` selection uses `sort -V`, which is not supported by BSD `sort` on macOS and will fail on many default setups; consider switching to a portable sort (e.g., `sort -t. -k1,1 -k2,2 -k3,3` or using `ls -t` and picking the first directory) or delegating to a small Node/Python helper.
- The `walk_descendants` helper assigns potentially multiple child PIDs into `current` and then passes that string to `pgrep -P`, but `pgrep` expects a comma-separated list, not a space-separated string; this can cause the descendant walk to stop after the first generation, so it would be safer to iterate per PID or normalize to the expected format.
- The launcher PATH emulation in `LAUNCHER_PATH` appends the current shell’s `$PATH`, which may differ substantially from the bare PATH used by the real desktop launcher; to avoid false positives/negatives in the start-command resolution check, consider deriving this exactly from the `run-template.sh` implementation rather than duplicating it with `$PATH` appended.
## Individual Comments
### Comment 1
<location path="plugins/app-it/skills/app-it/templates/desktop-doctor.sh" line_range="167-169" />
<code_context>
+# walk_descendants PID — echo the PID and up to 4 generations of children,
+# space-separated. Mirrors run-template.sh's reattach gate so "does the running
+# server belong to this launcher" uses the SAME ownership test the launcher does.
+walk_descendants() {
+ local root="$1" current="$1" tree="$1" gen
+ for _ in 1 2 3 4; do
+ gen="$(pgrep -P "$current" 2>/dev/null | tr '\n' ' ')"
+ [ -z "$gen" ] && break
</code_context>
<issue_to_address>
**issue (bug_risk):** walk_descendants may pass multiple PIDs to `pgrep -P`, which expects a single parent PID
After the first iteration, `current` can hold multiple space-separated PIDs, but `pgrep -P` expects a single PID or a comma-separated list, not a space-delimited string. This can make the descendant walk incorrect or brittle and undermine the ownership check. Consider iterating over each PID in `current` and aggregating children, or using a queue-based traversal so you always pass a single parent PID per `pgrep` call.
</issue_to_address>
### Comment 2
<location path="plugins/app-it/skills/app-it/templates/desktop-doctor.sh" line_range="479-50" />
<code_context>
+ if [ -n "$ICON_SRC" ] && [ -x "$SCRIPT_DIR/desktop-icons.sh" ]; then
</code_context>
<issue_to_address>
**suggestion:** The icon rebuild skip message conflates missing source icon with missing desktop-icons.sh
The `else` branch is taken when either `ICON_SRC` is empty or `desktop-icons.sh` is not executable, but the message only mentions a missing source icon. This can misdirect debugging when the script file is the real problem. Consider splitting the checks so each failure case (no `ICON_SRC` vs. non-executable/missing `desktop-icons.sh`) has its own, accurate log message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+167
to
+169
| walk_descendants() { | ||
| local root="$1" current="$1" tree="$1" gen | ||
| for _ in 1 2 3 4; do |
There was a problem hiding this comment.
issue (bug_risk): walk_descendants may pass multiple PIDs to pgrep -P, which expects a single parent PID
After the first iteration, current can hold multiple space-separated PIDs, but pgrep -P expects a single PID or a comma-separated list, not a space-delimited string. This can make the descendant walk incorrect or brittle and undermine the ownership check. Consider iterating over each PID in current and aggregating children, or using a queue-based traversal so you always pass a single parent PID per pgrep call.
| if [ -t 1 ] && [ -z "${NO_COLOR:-}" ]; then | ||
| C_OK=$'\033[32m'; C_WARN=$'\033[33m'; C_FAIL=$'\033[31m' | ||
| C_INFO=$'\033[36m'; C_DIM=$'\033[2m'; C_BOLD=$'\033[1m'; C_OFF=$'\033[0m' | ||
| else |
There was a problem hiding this comment.
suggestion: The icon rebuild skip message conflates missing source icon with missing desktop-icons.sh
The else branch is taken when either ICON_SRC is empty or desktop-icons.sh is not executable, but the message only mentions a missing source icon. This can misdirect debugging when the script file is the real problem. Consider splitting the checks so each failure case (no ICON_SRC vs. non-executable/missing desktop-icons.sh) has its own, accurate log message.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f894ff1cc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Backend (A3.2 multi-server) — only when the config declares one. | ||
| if [ -n "$BACKEND_PORT" ]; then | ||
| BRUNTIME=""; [ -f "$BPORT_FILE" ] && BRUNTIME="$(cat "$BPORT_FILE" 2>/dev/null || true)" | ||
| if [ -n "$BRUNTIME" ] && lsof -ti tcp:"$BRUNTIME" >/dev/null 2>&1; then ok "backend listening on :$BRUNTIME" |
There was a problem hiding this comment.
For A3.2 multi-server launchers, this reports the backend as healthy whenever any process is bound to the recorded backend port. If backend.pid is stale or another local service has taken backend.port, desktop:doctor prints [ ok ] backend listening even though the multi-server launcher would not reattach because its descendant_holds_port gate requires the backend listener to be in the recorded backend supervisor's tree. Please mirror the frontend/launcher ownership check here before marking the backend healthy.
Useful? React with 👍 / 👎.
|
|
||
| # Launch-time binary preflight — catches "works in my terminal, dead from Dock" | ||
| # because Finder launches with a bare PATH. Uses the launcher's augmented PATH. | ||
| CMD="$START_COMMAND" |
There was a problem hiding this comment.
In a multi-server config, BACKEND_START is parsed but never preflighted, so desktop:doctor can report the start command's binary as resolvable when only the frontend binary is present. The generated multi-server launcher checks both START_COMMAND and BACKEND_START_COMMAND before launching, so a missing backend runner (for example uvicorn or python) will still make Dock launch fail while the doctor report misses the actionable PATH problem.
Useful? React with 👍 / 👎.
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
Adds
desktop:doctor— a command that makes any generatedapp-itapp self-diagnosing. A user can runnpm run desktop:doctorlong after the build session (no agent in the loop) and get a short, issue-ready report on one launcher.It's the user-facing embodiment of Core principle #8 — runtime truth beats build-time guess. The same
server.port-first, descendant-walk-ownership, read-the-runtime checks the agent runs in Phase 4, packaged as a command on the user's machine.What it checks
config + placeholder leakage · bundle id shape (rejects
com.$(id -un).*) · installed/build.app· Info.plist identity ·run+ Mach-Owrapper· icon · ad-hoc signature · quarantine / iCloud signature-breaking xattrs · preferred-vs-runtime port · stale PID · whether the process on the runtime port is actually in the recorded supervisor's descendant tree (reuses the launcher's reattach gate) · start-command binary resolution on the launcher's augmented PATH · log/state paths · template drift (feature-probes the installedwrapper/runagainst the current templates via thegrep -qboaidiom — no version stamp needed).--tail[=N]appends the launcher log.Hard rules (honored structurally)
--fix-safeis the only mutating mode and is deliberately narrow — it touches only app-it's own generated state: stale pid/port files (only when the recorded process is dead), this bundle's stale LaunchServices registration, the rebuilt icon, and quarantine on the generated.app. It never touches the user's product code, dependencies, framework config, or anything outside app-it's artifacts, and never kills a running server (that'sdesktop:quit).app-itplugin.app-it-statichas a different runtime model (no dev-server daemon, no PID/port) and would need its own tailored checks — left as a clean follow-up.Design notes
desktop-*.sh— no new dependency.grep -qboa <marker> wrapperidiom.scripts/app-it.config.jsonthe same waydesktop-build.sh/desktop-quit.shdo — no APPS-table drift.Testing
./scripts/validate.shpasses (exit 0; the new template is required and covered by thebash -nsweep). Smoke-tested across 5 scenarios — no-app,--help, unknown-app selector, a fully built/signed/quarantined/stale-PID/markerless-wrapper bundle, and--fix-safe— verifying the fixes land and stay inside generated state, and that uncertain checks hedge with "probably".Docs
SKILL.md(templates list, Phase 3, script naming, report, a new Diagnosing a generated app section, frontmatter), the user-facingdesktop-launcher.md.template,docs/TROUBLESHOOTING.md(leads with it), andCHANGELOG.md.🤖 Generated with Claude Code
Summary by Sourcery
Add a self-diagnosing desktop:doctor command for macOS app-it launchers and document how to use it.
New Features:
Enhancements:
Documentation: