fix(apps): stop reading a broken metadata path as "app not installed"

[chenmingwei23]

What is the problem?

Two residual gaps around the hook reconciler that #7892 added, both reachable
from a CLI-driven lifecycle change that the gateway only learns about
second-hand.

1. app_enabled_state reads a bad metadata path as "app not installed".

That function exists to keep "not installed" apart from "could not be read",
because the first is a reason to delete an app's runtime and the second is not.
Its own docstring says so. It then led with Path.is_file(), which answers a
silent False for five path shapes that are not absence. Measured against this
interpreter rather than assumed:

path shape	Path.is_file()	base verdict
dangling symlink	False	not installed
directory in the file's place	False	not installed
fifo in its place	False	not installed
symlink loop (ELOOP)	False	not installed
non-directory parent component (ENOTDIR)	False	not installed
unreadable directory (EACCES)	raises	unknown -- already correct

The last row is the boundary and it matters: a genuine stat fault was always
reported correctly, because is_file re-raises it and the existing handler turns
it into None. The bug is path SHAPES, not permissions.

And the verdict cannot be read off the exception class, because one condition does
not produce one class across platforms. A non-directory parent component raises
NotADirectoryError (ENOTDIR) on POSIX but FileNotFoundError on Windows, which
maps ERROR_PATH_NOT_FOUND to ENOENT -- the same class a genuinely missing file
raises. The first version of this fix keyed "definitely not installed" on that
class, so it was correct on Linux and still wrong on Windows; its own Windows test
caught it, failing with assert False is None.

Review then found the same mistake one predicate over, and it is the general form of
this defect: I keyed a decision on something whose meaning changes by platform.
is_symlink is False for a Windows directory junction, so a DANGLING junction
presents as is_dir=False, exists=False, is_symlink=False -- indistinguishable from
nothing at all -- and the ancestor walk stepped over the thing occupying the path and
reported genuine absence. That answer is the escalation this PR exists for: with
#7892's reconciler now a live consumer, a False here is not a cosmetic misreport,
it unloads a RUNNING app's routes and modules and lets apps.backend delete its
materialized agent files. The repo had already built the answer -- is_link_or_junction
in platform_compat, 131 call sites outside its own module, whose docstring states the
premise: "os.path.islink returns False for a junction, so a caller that only checks
islink would treat a junction as a real directory." This was the one call site that
skipped it. The first fix was correct and incomplete, not wrong.

2. A CLI uninstall never drops the app's in-process hook registries.
forget_app_hooks had exactly one caller -- the dashboard uninstall handler --
so a CLI uninstall reached the reconciler's teardown branch and left them behind.

3. The reconciler's own "gone" verdict has the same collapse. It decides teardown
from get_app -> _read_installed, which leads with the identical Path.is_file()
check and additionally folds a corrupt JSON body into None. So a broken path
unloaded a healthy app's routes and modules every tick until the fault cleared -- and
with gap 2's registry drop, would take its disable and slot-close hooks with it.

Why this issue matters to the user

Gap 1's cost is asymmetric, and the callers that already respect the tri-state are
what make it worth fixing. apps.backend reads it before deleting materialized
resources -- _drop_disabled_app_resources on a False, and
_undo_promotion_of_disabled_app likewise -- and its own comments say a None
"must not be collapsed into disabled" and is retried instead. That contract was
already written correctly; this function did not honour it, so a dangling symlink or
a directory in the metadata's place deleted an app's agent files.

Gap 3 is where the unattended harm lives: the 15s reconciler tears a live app's
routes and modules down on a misread, repeatedly. An earlier revision of this PR
attributed that harm to app_enabled_state and was wrong -- the reconciler never
called it. Three reviewers caught the same error; the fix now makes the claim true
rather than deleting it.

Gap 2 leaves the user with a tab they cannot get rid of. The surviving
slot-close hook is a closure over a store the uninstall deleted, so it raises;
notify_slot_closed reports that failure rather than swallowing it, and
api_chat_slot_delete refuses the dismissal on a false return. The app is gone
and its tab stays.

How our fix solves it

• Only nothing being at the path is absence, decided from the path's SHAPE.
  stat() is called directly so it raises instead of lying, and the absent branch
  is then confirmed structurally rather than by error class: _absence_is_genuine
  walks to the nearest existing ancestor and requires it to be a directory. A
  dangling symlink is None (a path that exists whose target cannot be seen), any
  other OSError is None, and a successful stat on a non-regular file is
  None. That makes every one of the six rows above give the same answer on POSIX
  and Windows. No behaviour change for a healthy record, and genuine absence is
  still a definite False -- uninstall depends on it.
• The reconciler confirms absence before acting on it. On get_app returning
  None it reads app_enabled_state off-loop and proceeds only on a definite
  False; unknown defers the whole app to the next tick, the same direction
  _disable_loaded already takes when startup ownership cannot be proven clear.
  _read_installed is deliberately NOT made tri-state -- it has 24 callers and
  get_app/list_apps 63, which is a far wider change than this PR should carry.
• forget_app_hooks on the UNINSTALL shape only, in the reconciler's
  teardown branch where gone is already computed. The asymmetry is the one
  forget_app_hooks documents: these registries are repopulated from each app's
  own watchdog rather than by the gateway, so clearing them on a DISABLE would
  leave a window after a re-enable in which a dismissal silently fails to reach a
  live worker. Gated on a settled teardown, so an app whose code is still running
  keeps its own off-switch and the reconciler drops the registries on the retry
  that settles.

Every path predicate in these two functions, enumerated. Review found this class
twice -- once on the ancestor walk, once on the metadata path itself -- so the useful
statement is not "one more instance is fixed" but "there are four, and here is why
three of them were never wrong":

where	predicate	junction-safe, and why
app_enabled_state	meta_path.stat()	n/a -- it raises, and the handler below decides
app_enabled_state	meta_path.is_symlink() or is_link_or_junction(meta_path)	FIXED -- Path.parents excludes the path itself, so the ancestor walk can never reach a junction sitting ON installed.json
app_enabled_state	stat.S_ISREG(st.st_mode)	safe -- only reached when stat SUCCEEDED, so the path resolved and there is no dangling reparse point left to miss
_absence_is_genuine	ancestor.is_dir()	safe -- an INTACT junction to a directory genuinely leads to a directory, which is the right answer here
_absence_is_genuine	ancestor.exists() or ancestor.is_symlink() or is_link_or_junction(ancestor)	FIXED -- a dangling junction is False for all three ordinary predicates

The general form of the defect, stated once: a decision must not be keyed on
something whose meaning changes by platform. The exception class was the first
instance, is_symlink versus a junction the second and third. Each earlier fix was
correct and incomplete rather than wrong.

What tests we did

test/test_app_manager.py -- five tests, one per measured shape, plus the
already-correct EACCES boundary kept deliberately so the distinction is pinned,
plus a control that a healthy record still reports its own enabled flag.
test/test_hook_reconcile.py -- three tests on main's own reconciler harness:
an uninstall makes a previously-refused slot close succeed, a plain disable
leaves the app's hook reachable and consulted, and an unsettled uninstall keeps
it for the retry.

Ten mutation probes, each killed:

mutant	result
restore Path.is_file()	5 tests red, one per shape
drop the shape check, keying absence on the class again	1 test red, assert False is None -- byte-identical to the Windows CI failure
drop the junction clause from the ancestor walk	1 test red, assert False is None again -- the same signature one predicate over
drop the junction probe from the metadata-path check	1 test red, assert False is None a third time -- the same class on the path itself
shape check never accepts a directory	2 tests red, both controls for real absence
inspect only the immediate parent, never walk up	1 test red
drop forget_app_hooks from the reconciler	1 test red
call it on disable too	1 test red
drop the reconciler's absence confirmation	1 test red -- a healthy app torn down on a broken path
defer on anything but a definite True	12 tests red -- teardown would never run

test_app_manager.py + test_hook_reconcile.py: 179 passed. The FIFO case carries an
explicit skipif(not hasattr(os, "mkfifo")) with its reason -- a FIFO is a POSIX-only
path shape, so there is nothing to assert on Windows; the non-directory-parent case is
NOT skipped, because that shape exists on both platforms and skipping it would have
hidden the defect above. The dangling junction is fed as a SHAPE rather than a real
junction, for the same reason inverted: a junction has no POSIX equivalent at all, so
requiring one would leave the case exercised only on the platform it breaks. A path
that does not exist is already False for all three ordinary predicates, which IS the
dangling junction's shape, so only the junction probe is stood in for. isort, flake8, mypy, the black gate and the sync-io-in-async gate all
clean; test_app_manager.py is in the black baseline, so it was edited without
running black over it (a bare run reformats 39 unrelated pre-existing lines).

Manual verification

#7926's symptom is already fixed on main by #7892, verified before rescoping
this PR: a throwaway worktree at origin/main with none of this branch's code
present, driving hook_reconcile.reconcile_once from a two-process harness
that uses the real CLI as a subprocess against the same KIROCREW_HOME:

verb	after the CLI	after the reconciler's tick	a second, untouched app
app disable	HTTP 200	HTTP 404	200
app uninstall	HTTP 200	HTTP 404	200

That is why this PR says Refs #7926 and not Closes -- #7892 fixed the
reported issue, and this carries only the two gaps that survived it.

Any other suggestions on the work

This PR previously proposed a second, independent reconciliation sweep for the
same symptom. #7892 landed the mechanism first, so that work was dropped rather
than merged alongside it: a fix that is not needed is cheaper to drop than to
maintain. What remains are the two defects that #7892 did not cover, and one of
them is more dangerous because of it.

Pattern harvest

Rule candidate: when a tri-state read exists specifically to separate "absent"
from "unknown", the absent branch must be confirmed from the path's SHAPE, not from
a predicate like Path.is_file() that collapses several failure modes into False
and not from the exception class either -- one filesystem condition does not map to
one class across platforms, so a class-keyed branch is a platform check wearing a
semantic disguise. This repository already learned that for chdir
(_spawn_exec_shim: "the errno is not the thing to key on") and the same trap was
walked into here.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 0db5c243eb0711ac796cca2614185a5dcfeb7f0f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Real, asymmetric harm (agent-file deletion, live-app teardown) fixed at the tri-state contract's owner, with fail-to-unknown confirmation at the one consumer that destroys state.

Watch

• The collapsing reader (_read_installed/get_app) survives for its ~63 callers; the reconciler is patched at the consumer. Any future consumer that treats get_app() is None as "uninstalled" re-walks into this. A follow-up making the tri-state read the primitive would retire the class, not just these instances.

[DESIGN-REVIEWED] 0db5c24

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0db5c243eb0711ac796cca2614185a5dcfeb7f0f and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0db5c24

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0db5c243eb0711ac796cca2614185a5dcfeb7f0f: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0db5c243eb0711ac796cca2614185a5dcfeb7f0f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts and claims verified. The description's numbers hold up (forget_app_hooks had exactly one caller at routes.py:1364; is_link_or_junction is used across 31 files; _read_installed has ~24 callers). The one thing the PR leaves behind is in dashboard/handlers/agents.py, which had already built a call-site patch (_require_present_shape) for this same defect — now superseded at the metadata call site by the cause-level fix. Final review follows.

First-Principles-Verdict: CONCERNS

Cause-level fix, but generalizing app_enabled_state strands its pre-existing call-site patch in agents.py as a weaker second spelling nobody deleted.

What this change ships

Intent: stop a broken-but-present metadata path from being acted on as a deliberate uninstall — a FIX (three declared defects).

1. Broken metadata shapes (dangling link, directory, fifo, loop, bad parent) now read "unknown", not "uninstalled" — justified
2. Genuine absence still answers a definite False, uninstall unchanged — justified control
3. The 15s reconciler defers teardown until absence is confirmed, sparing a live app — justified; symptom-level, declared, with the _read_installed cause deferred on counted scope (24 + 63 callers)
4. CLI uninstall now drops in-process hook registries, making the dead app's tab dismissable — justified, declared second fix
5. Disable and unsettled teardown keep the registries for the retry — justified, documented asymmetry
6. Warning logs naming the broken shape — rides along, harmless
7. Private _absence_is_genuine helper — one consumer, but not a generalized form; fine

Watch

_require_present_shape (agents.py:501) keys "genuinely absent" on FileNotFoundError — the exact platform-varying signal this PR's description names as the general defect. Its one metadata caller is shielded by a prior S_ISDIR screen (agents.py:659), so the Windows hole is race-reachable only; still, it is the 1 counted unfixed sibling of the named root cause (grep: _require_present_shape, 2 call sites).

Subtractions

Drop the _require_present_shape metadata screen at agents.py:670-675: the fixed app_enabled_state now yields None for every present-but-malformed shape and agents.py:677 already raises AppOwnershipUnreadable on None, so the screen (and its now-false comment at agents.py:663 saying app_enabled_state "reaches the metadata through Path.is_file()") duplicates the shared fix. Keep the helper's expect="dir" root call at agents.py:631, its remaining real consumer.

[FIRST-PRINCIPLES-REVIEWED] 0db5c24

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0db5c243eb0711ac796cca2614185a5dcfeb7f0f — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 0db5c24

Verdict parsed from the review's SHA-scoped output markers for commit 0db5c243eb0711ac796cca2614185a5dcfeb7f0f.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0db5c243eb0711ac796cca2614185a5dcfeb7f0f: <one-sentence reason>

[dwu96]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: clear root cause - app_enabled_state led with Path.is_file(), so five path shapes that are not absence (dangling symlink, directory or fifo in the file's place, symlink loop, non-directory parent) read as a definite 'not installed'; the fix decides absence from the path shape rather than the errno class and makes the hook reconciler defer on unknown instead of unloading a healthy app.

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: app_enabled_state led with Path.is_file(), which answers a silent False for five shapes that are not absence (dangling symlink, dir/fifo in the file's place, ELOOP, non-directory parent) -- so a broken metadata path read as uninstalled and the 15s hook reconciler unloaded a healthy app's routes and modules; absence is now decided from the path shape, unknown defers to the next tick, and forget_app_hooks is cleared on uninstall only.