fix(observe): readers anchor on the nearest materialized hook; installers on the linked root

[tonychang04]

Follow-up to #178 from its post-merge review (round 4, filed against main after the squash).

Critical taken

auditRoot preferred the link file over the materialized hook, while the hook writes at its own entry path and the installers materialized at cwd. A re-link or insta observe install from a subdirectory of a linked repo minted a second hook under that subdirectory; sessions there wrote to it, and observe report|sync read the link root and printed empty. Two changes so the halves can't diverge:

• Readers anchor on the nearest materialized hook above cwd (what the Codex wrapper and the Claude entry actually run), then the link file, then cwd. nearestHookRoot() / auditRoot() in commands/observe.ts.
• Installers anchor on the linked project root (installRoot(), same rule as writeProject), so a re-link or observe install from a subdirectory refreshes the project's hook instead of creating a second one. observe uninstall uses the same root.

Suggestions taken

• Fourth auditRoot case in the test: link file at the repo root and a hook below it → the hook wins; above the hook the link file still anchors. Plus an installRoot test.
• Codex wrapper exits 0 when the child could not be started or was killed (status === null); a non-zero exit there is reported by Codex as a failed hook after every tool call, which is worse than one missed audit line. The hook's own status still passes through when it ran.
• The git rm -r --cached hint is printed even when every skills add failed: tracked files are independent of today's adds. The ignore entries still wait for a successful add.

Verification

npm run typecheck clean; vitest 715/715 (2 new tests, one extended).

🤖 Generated with Claude Code

https://claude.ai/code/session_0144rBPvDXaGgoje2QnmfxzY

Recorded decision

Cleanup of nested hooks already materialized by 0.0.59 (John-bot, non-blocking) — not added. A stale nested hook is self-consistent (a session below it writes there and observe report from below it reads there), 0.0.59 was the only release that could create one and was live for minutes, and removing it is rm -r <subdir>/.insta/observe plus the two hook entries. A migration that deletes user-visible files from a subdirectory on the next link is more surprising than the state it would fix.

[jwfing]

Summary
The implementation matches the stated root-anchor fix and does not introduce blocking security, performance, or functionality issues.

Requirements Context
I used the PR description, the linked #178 review context on GitHub, local README.md command docs, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md. The repo docs only describe insta observe at a high level as a local credential audit and say project create|link install the observe hook; the precise root precedence comes from this PR’s description and the observe hook comments/tests. I found no local skills/insta/cli-reference.md checkout to inspect, and this PR does not change commands or flags.

Findings

Critical
(none)

Suggestion

1. src/commands/observe.ts:21-24, src/commands/observe.ts:62-75, src/observe/install.ts:92-99 — Consider a cleanup path for already-materialized nested hooks from the bad release. This PR prevents new nested hooks by installing/uninstalling at the linked root, but auditRoot and the Codex wrapper still prefer an existing nearer nested hook. In that stale split state, reinstalling from the nested tree refreshes the linked root while the nested hook can continue to shadow it until manually removed. Non-blocking because the reader/writer now agree and the main regression is fixed.

Information

1. test/observe-install.test.ts:130-168, test/ensure-skills.test.ts:79-90 — Software engineering coverage looks appropriate for the changed behavior: the tests exercise nearest-hook precedence, linked-root install anchoring, and the offline git rm --cached hint case.
2. src/commands/observe.ts:21-43, src/observe/install.ts:92-99, src/ensure-skills.ts:105-113 — No security-relevant changes found: this does not add new dependencies, SQL, HTTP calls, or secret logging paths. No performance issues found: the added ancestor scans are bounded by directory depth and run in CLI/hook setup/report paths.

I did not rerun npm run typecheck or npm test because node_modules is absent and installing dependencies would mutate the checkout; git diff --check main...HEAD is clean.

Verdict
approved per the requested rubric: no Critical findings.

[jwfing]

Summary

Inverts auditRoot's precedence to the nearest materialized hook and anchors the installers on the linked project root — the reader half is correct and test-bound; the installer half and the Codex exit-code change are behaviour changes that no test binds, and the two halves can still diverge in a two-hook tree.

Requirements context

No matching spec/plan found — this repo has no /docs/superpowers/ (no docs/ tree at all; CLAUDE.md is a one-line @AGENTS.md pointer and the only skill is .claude/skills/developing-insta-cli/SKILL.md). Assessed against the PR body and the #178 post-merge review (round 4) it quotes as its source of intent.

Verification I ran in a clean clone at head a7ddca4 (confirmed current via git ls-remote; base 29bb82d):

• npm ci clean, npx tsc --noEmit clean, npx vitest run → 49 files / 715 tests passed. Matches the body's claim.
• Negative controls (revert each behaviour change, re-run the suite):
  • revert auditRoot to #178's link-file-first form → 1 red (auditRoot: nearest materialized hook, …) ✅ bound
  • revert ensure-skills hint back below the installed === 0 return → 1 red ✅ bound
  • revert all three installer call sites to process.cwd() → 715/715 green ❌ unbound
  • revert r.status===null?0: to ?1: → 715/715 green ❌ unbound

Findings

Critical

(none) — the reader fix is correct, does what the PR claims for the reported scenario, and reds under a negative control. Nothing here blocks the merge.

Suggestion

[software engineering] The installer half of the PR is not bound by any test — src/commands/observe.ts:62-76, src/commands/project.ts:23-31

test/observe-install.test.ts:160-168 binds the installRoot helper, but nothing binds its use. Reverting all three call sites to process.cwd() (observe.ts:63, observe.ts:74, project.ts:25) leaves the suite at 715/715 green. That is exactly the shape that let #178 r4 through: a helper proved in isolation while the call site that decides the behaviour goes untested.

The blocker is testability — observeInstall(), observeUninstall() and tryInstallObserve() all read process.cwd() with no injectable cwd, unlike auditRoot(cwd = process.cwd()). Threading an optional cwd through them makes the assertion a three-liner, and test/project-link.test.ts already has the exact idiom to copy (writeProject from a subdirectory updates the root link, not a nested copy — including the expect(existsSync(join(sub, '.insta'))).toBe(false) "no second one minted" assertion this PR is about).

[software engineering] The Codex exit-code change is also unbound — src/observe/install.ts:97

Flipping r.status===null?0:r.status back to ?1: keeps the suite green. test/observe-install.test.ts:96-122 already spawns the real Codex command in a child, so the arm is reachable from a test: materialize a hook path that exists but cannot be executed as a script (e.g. make .insta/observe/hook.js a directory) and assert the wrapper's exit code is 0. Without that, the next person tidying the ternary silently restores the "failed hook after every tool call" behaviour this PR removed.

[functionality] Reader and writer can still diverge — src/commands/observe.ts:14-25

The comment says the nearest materialized hook is "what the Codex wrapper and the Claude entry actually run". That is true of the Codex wrapper (it climbs from process.cwd(), install.ts:92-99) but not of the Claude entry, which resolves "$CLAUDE_PROJECT_DIR/.insta/observe/hook.js" (install.ts:72-74) — an absolute path fixed at session start, unrelated to the reader's cwd. When two hooks exist at different depths the two anchors disagree.

Reproduced against head (scratch vitest, removed afterwards) — linked at <repo>, hooks materialized at both <repo> and <repo>/apps/api, one finding appended at <repo>/.insta/audit.jsonl by a Claude session started at <repo>:

writer root:                      /tmp/probeB-xxxx
reader root from apps/api:        /tmp/probeB-xxxx/apps/api
audit.jsonl at reader root?       false      → `insta observe report` prints "empty"

Under main's precedence the same fixture resolves to /tmp/probeB-xxxx and finds the record — so this is a genuine delta, the mirror image of the bug being fixed. And the state is reachable on head alone, no legacy CLI needed: insta observe install in an unlinked <repo>/apps/api anchors at cwd (installRoot → findProjectRoot() ?? cwd), then insta project link at <repo> mints the second hook. The installer change stops new splits from link/install inside an already-linked tree, but it neither heals nor detects an existing one.

Worth either (a) preferring a hook root that actually has an .insta/audit.jsonl before falling back to the nearest one, or (b) having installObserve clear a now-orphaned nested .insta/observe when it anchors at a parent, or at minimum (c) warning from observe report when a hook exists both at the anchor and elsewhere in the tree. Also worth softening the observe.ts:14-20 comment — "the Claude entry" doesn't climb.

[functionality] observe uninstall now uses the installer's anchor instead of the reader's — src/commands/observe.ts:73-76

uninstallObserve(await installRoot()) unregisters the linked root's harness entries. In the same two-hook fixture, running it from apps/api gives:

parent  PostToolUse after uninstall: []            ← removed
apps/api PostToolUse after uninstall: [{…hook…}]   ← still registered and still recording
reader anchor from apps/api:          apps/api     ← still points at the live one

…while the command prints uninstalled observe hook. On main (uninstallObserve(process.cwd())) the local one was the one removed. For a tool whose whole job is capturing credential material into a local file, "uninstall reported success and the hook that is actually running kept running" is the wrong failure direction. Uninstall is a question about what runs, which is the reader's question — nearestHookRoot(cwd) ?? (await installRoot(cwd)) would keep it symmetric with auditRoot. (Note uninstallObserve also leaves .insta/observe/hook.js on disk, so the directory keeps anchoring auditRoot afterwards — intentional for reading a stale log, but it means the two hooks can never be reduced to one via the CLI.)

[security] The status === null fail-open leaves no signal at all — src/observe/install.ts:89-97

The rationale is sound (a non-zero exit means Codex shouts after every tool call), but the wrapper now discards r.error entirely: a hook that could not be spawned produces no exit code, no stderr, no audit line. On a credential-audit path that is a silent loss of coverage. A single stderr note before exiting 0 keeps the quiet-exit behaviour and makes it diagnosable, and satisfies the character constraint documented at install.ts:86-88 (no $, backtick, %, !, "):

if(r.error)process.stderr.write('insta-observe: hook did not run\n');

Information

• tryInstallObserve's message still hardcodes the relative path — src/commands/project.ts:28 says installed observe hook (credential audit) → ./.insta/observe while observeInstall was correctly updated to print the resolved root (observe.ts:65). Run insta project link from apps/api and the line now names a directory the install did not touch.
• Divergent anchors inside one command — project.ts:85-86 and 104-105: observe now installs at the link root, installSkills({ cwd: process.cwd() }) still uses cwd. Self-consistent (the npx skills add child inherits the process cwd, so the skills and their .gitignore entries do land together), but the two anchors in adjacent lines invite the next reader to "fix" one of them. A one-line comment saying why they differ would earn its keep.
• The untrack hint can now be printed without its ignore entries — src/ensure-skills.ts:105-113. The reasoning is right (tracked files predate today's run), but in the all-adds-failed path the user is told git rm -r --cached skills-lock.json while .gitignore still has no entry for it — following the hint leaves the file untracked and unignored, and the next git add -A re-adds it. The new test at test/ensure-skills.test.ts:79-91 asserts exactly that pairing. Not wrong, just incomplete advice in that one state.
• Ordering side effect of the same move: the hint now prints before the .gitignore += line rather than after, so the "here's what I ignored" and "here's what the ignore can't help" lines swapped. Cosmetic.
• No import cycle from project.ts → commands/observe.ts (observe.ts pulls api/config/util/observe, none of which reach commands/project.ts; only index.ts imports it), and installRoot is byte-for-byte the writeProject rule (config.ts:141-146) — the "same rule as writeProject" claim checks out.

Performance

No concerns. nearestHookRoot is a bounded sync climb to the filesystem root with one existsSync per level — the same shape and cost as the pre-existing findProjectRoot, on a non-hot CLI path. The only new work is alreadyTracked's git ls-files now running on the installed === 0 path too: one extra spawnSync on a command that just spent seconds in npx. No new queries, loops, allocations, or blocking work in a hot path.

Security (rest)

Nothing else notable. No new dependencies (package.json / package-lock.json untouched). No new external input reaches a shell — CODEX_HOOK_SCRIPT is a module constant with no interpolation, and alreadyTracked uses argv-array spawnSync with no shell (gitignore.ts:31). The new observeInstall line prints a local filesystem path, not a credential. No auth or authorization surface is touched.

Verdict

approved — 0 Critical findings. The reader fix is right and proven; the four Suggestions (two missing test bindings, the residual two-hook divergence, and the uninstall anchor) are worth a follow-up but none of them blocks this merge. Informational only — the GitHub approval is a separate human action.

[jwfing]

LGTM - approved.