fix(observe,skills): gitignore the local state the CLI writes; make the Codex hook portable

[tonychang04]

Problem

insta project create|link (and insta observe install) leaves three things for the user to discover in git status:

• .insta/observe/ — a copy of this CLI version's hook + scanner, regenerated on every link
• .codex/hooks.json — with the absolute project path baked into the hook command, so committing it ships /Users/<author>/… to every clone, where it fails after each tool call
• skills-lock.json — written by npx skills add; its payload (the skill dirs) was already ignored, the lock itself was not

Only the three skill dirs were ever added to .gitignore. Nothing ignored .insta/audit.jsonl either, and that file holds partial secret fingerprints plus redacted context (hostnames, usernames around the masked span).

Change

• One helper, src/gitignore.ts → ensureGitignore(cwd, entries, comment), used by both installers (moved out of ensure-skills.ts, re-exported there). Rule: the step that writes a regenerable or machine-local file adds its ignore entry, exactly as insta secrets already does for .env.
• observe install ignores .insta/observe/ and .insta/audit.jsonl. Deliberately not .insta/ wholesale: project.json is the team binding the skill tells users to commit.
• skills install also ignores skills-lock.json, and only touches .gitignore when at least one skills add succeeded. The lock pins only a content hash, not the prod/staging source this CLI resolves per environment, and insta project link is the restore path, so a committed lock is a lockfile for nothing.
• Already-tracked paths get a hint. An ignore entry does nothing for a file git already tracks, and the repos that most need these entries are the ones that committed audit.jsonl or skills-lock.json before the CLI ignored them. Both installers now print the one git rm -r --cached … line that fixes it (alreadyTracked / untrackHint in gitignore.ts).
• Codex hook becomes one shell-neutral node -e script that climbs from the session cwd to the nearest .insta/observe/hook.js (the same walk findProjectRoot does) and runs it with stdin passed through; nothing found → silent no-op. No absolute path, no POSIX syntax, no git forks. The script is kept free of every character sh or cmd.exe rewrites inside double quotes ($ ` % ! ") and a test asserts that, so no commandWindows override is needed. .codex/hooks.json is now shareable (Codex still has each user trust it before it runs).
• Both commands print .gitignore += … for what they added; re-runs add nothing.

What is committed vs ignored after this

File	Verdict
.insta/project.json	commit (team binding)
.claude/settings.json hook entry	commit (portable, guarded)
.codex/hooks.json hook entry	commit (now portable, shell-neutral)
.insta/observe/, .insta/audit.jsonl	ignored
.claude/skills/, .agents/skills/, .github/skills/, skills-lock.json	ignored

Existing projects pick the entries up on their next insta project link / insta observe install (idempotent).

Review round 1 → round 2

Both Criticals were on the first Codex entry (git rev-parse --show-toplevel ≠ insta project root in a monorepo → hook silently dead; POSIX guard handed to cmd.exe on Windows → failure after every tool call). Fixed by the shell-neutral climb above; the new tests run the command through the platform shell (shell: true), so the Windows CI job exercises the real form: nested monorepo project from the project dir and a subdirectory, git root above the project, fresh clone, stdin passthrough, character-set assertion.

Suggestions taken: #3 already-tracked hint, #4 no git forks (moot with the new form), #5 nested-project test, #6 gate the skills ignore on a successful add.

Round 2 → round 3

Critical: finding the right hook.js from a nested session cwd was only half of it — the hook then recorded into CLAUDE_PROJECT_DIR || event.cwd, and Codex's event.cwd is the session cwd, so apps/api/src/routes/.insta/audit.jsonl appeared, unignored. Fixed in the hook itself: the materialized entry lives at <root>/.insta/observe/hook.js, so projectRootFor derives the root from its own entry path and only falls back to the harness env / event cwd when not running from a materialized location. observe report|sync read the audit from findProjectRoot() so they work from any subdirectory too. Test: the real hook source (loaded via tsx through NODE_OPTIONS, no build step) driven by the generated Codex command from apps/api/src/routes with a Codex-shaped event carrying a DB password appends a redacted finding to apps/api/.insta/audit.jsonl and writes nothing at the session cwd.

Recorded decisions on the informational items:

• Devel #7 line-equality matching — kept. A redundant entry next to a user's *.jsonl or .insta/observe (no slash) is harmless, and ignore-semantics matching would need a real gitignore engine for no user-visible gain.
• feat: insta upgrade + autoupdate (on by default while pre-1.0) #8 uninstall leaves the entries — deliberate, now stated in a comment: a stale audit.jsonl should stay ignored after observe uninstall.
• Suppress repeated skills-CLI banner on install (+ version bump) #9 .gitignore written outside a git repo — kept as the pre-existing skills-path behaviour; settling it for both installers and insta secrets together is the follow-up below, not this PR.

Round 3 (clean) → final head

Both bots approved at 38743c3 with no Criticals. Taken from r2d2's suggestions: #1 a test for the report-root change, and #2 auditRoot() — report/sync anchor on the link file, else on the materialized hook (a standalone insta observe install in an unlinked directory has no project.json), else cwd, so both halves agree from any subdirectory.

Recorded declines (non-blocking, kept as is):

• usage/billing commands, browser OAuth login, Bun native binaries #3 installed === 0 skips ignore maintenance on an offline re-run — accepted trade-off. The alternative prints .gitignore += … for files that may not exist; the entries land on the next successful link, which is also when the files are (re)written.
• insta services add/list/remove/scale/upgrade #4 bound the ancestor climb — the climb stops at the first .insta/observe/hook.js, and the hook only runs after the user has trusted the project's .codex/hooks.json, which lists no path at all. A world-writable /tmp/.insta/observe/hook.js requires filesystem write access the attacker already has; a depth cap would trade the monorepo fix for hardening against that. Noted as a follow-up if the threat model changes.
• Devel #5 second node process (~+44 ms per tool call) — inside the 15 s timeout; an in-process import() couples the wrapper to the hook's module format and error handling. Follow-up if it ever shows up in practice.
• CLI: source-directory deploy · compute custom domains · auto-install skills #6 shell: true uses /s while Codex uses cmd /C — the pre-PR Windows command (node "C:\…\hook.js") had the same one-double-quoted-argument shape and worked, so the quote regime is unchanged by this PR. Manual Windows check noted for the release.
• Devel #7 legacy absolute-path Codex entry migration relies on _insta marker — the marker is written by every version that ever produced that entry, so it is the intended key; the observe/hook. substring is only a belt-and-braces fallback.

Verification

• npm run typecheck clean; vitest 710/710 (8 new tests across the two files).
• Built CLI run in a throwaway git repo: git status afterwards shows only .codex/ and .gitignore; second run prints no .gitignore += line.

Not in this PR (follow-ups): insta secrets only ignores .env when .git is in cwd (misses pre-git init and subdirectory runs) and the installers do the opposite — pick one rule for all three; ~/.insta/config.json is written 0644 with tokens inside; source deploy has no .env-in-build-context preflight; audit.jsonl stores the last 4 chars of each secret (the hash prefix alone would do for dedup).

🤖 Generated with Claude Code

https://claude.ai/code/session_0144rBPvDXaGgoje2QnmfxzY

[jwfing]

Summary
The ignore-entry changes look well scoped, but the new Codex hook is not portable across the repo’s supported Windows surface.

Requirements context
I based intent on the PR description plus local docs. The repo documents that project create|link have cwd side effects including .insta/, observe hook, and agent skills (CONTRIBUTING.md:17-18, .claude/skills/developing-insta-cli/SKILL.md:17-19), that ./.insta/project.json is the project binding (README.md:223-226), and that project create/link install stack skills plus the observe hook (README.md:238-245). No command or flag surface changed, so the cli-reference mirror requirement does not appear to apply (CONTRIBUTING.md:41-44). I also checked the official Codex hooks docs for the PR’s git rev-parse claim and Windows override support: https://learn.chatgpt.com/docs/hooks.

Findings

Critical

• src/observe/install.ts:76-84 builds the committed Codex hook with POSIX-only shell syntax ([ ! -f ... ], $(...), pwd) and does not emit a commandWindows override. This PR’s stated goal is a portable .codex/hooks.json, and this repo treats Windows as a supported/tested platform (CONTRIBUTING.md:48-53); the current regression test is explicitly skipped on Windows (test/observe-install.test.ts:12-52). On Windows Codex, this is likely to become the same class of after-each-tool hook failure the PR is fixing, just on another platform. Add a Windows-compatible commandWindows entry or generate a shell-neutral command.

Suggestion
(none)

Information

• Software engineering: the shared helper extraction follows the project’s side-effectful-module/test-injection style, and the new tests cover ignore entries, idempotence, and preserving .insta/project.json (src/gitignore.ts:11-20, test/ensure-skills.test.ts:29-90, test/observe-install.test.ts:19-29).
• Functionality: aside from the Windows hook path above, the implementation matches the PR intent for .insta/observe/, .insta/audit.jsonl, and skills-lock.json (src/observe/install.ts:87-97, src/ensure-skills.ts:82-108).
• Security: no new SQL, HTTP, auth, or dependency surface is introduced; ignoring .insta/audit.jsonl reduces accidental check-in risk for local audit findings (src/observe/install.ts:10-15, src/observe/hook.ts:19-27).
• Performance: no hot-path concern found; the new synchronous file I/O runs during install/link flows only (src/gitignore.ts:11-20, src/observe/install.ts:87-97).
• Verification: git diff --check main...HEAD passed. I could not run npm run typecheck or targeted Vitest locally because tsc/local vitest dependencies are not installed in this checkout (package.json:35-40).

Verdict
Request changes: fix the generated Codex hook’s Windows portability before merge.

[jwfing]

Summary

The .gitignore half of this PR is clean, well-tested and a real improvement (the audit.jsonl entry in particular closes a genuine credential-leak path); the Codex-hook half swaps one portability bug for two new ones — the new git rev-parse --show-toplevel form silently disables the hook whenever the insta project root is not the git root, and it hands POSIX [ ! -f … ] syntax to cmd.exe on Windows, where the old absolute-path form worked.

Requirements context

No matching spec/plan found — this repo has no /docs/superpowers/ (and no docs/ tree at all), so the implementation is assessed against the PR description plus the repo's own conventions in AGENTS.md and .claude/skills/developing-insta-cli/SKILL.md. Note AGENTS.md rule 4 (mirror command/flag changes into skills/insta/cli-reference.md) does not apply here: no command or flag surface changes, only two extra info lines.

Gates reproduced locally at head 671a9c7: npm run typecheck clean, vitest run 705/705 passed / 49 files — matches the PR body.

Codex behaviour claims were checked against upstream openai/codex rather than taken on trust. Two are confirmed: hooks.json is loaded from the project layer's <project>/.codex/ folder (codex-rs/config/src/state.rs hooks_config_folder()), and the { hooks: { PostToolUse: [{ matcher, hooks: [{ type, command, timeout }] }] } } shape this installer writes is exactly the accepted schema (codex-rs/config/src/hooks_tests.rs). Also confirmed: hooks carry a trustStatus and content hash, so the "each user trusts it first" argument for making the file shareable holds.

---

Findings

Critical

1. Functionality — the new Codex hook silently no-ops whenever the insta project root is below the git root — src/observe/install.ts:76-85

git rev-parse --show-toplevel resolves the git repository root, but the file it is looking for is written at opts.cwd — the insta project root (src/observe/install.ts:88 → materialize). Those are the same directory only when the project sits at the repo root. This CLI explicitly supports the other case: findProjectRoot (src/config.ts:107-116) is a git-style ancestor climb for .insta/project.json precisely so a project can live anywhere in a tree, and insta project link can be run in apps/api of a monorepo.

Reproduced at this head (git repo at /tmp/mono, installObserve({ cwd: '/tmp/mono/apps/api' }), hook command executed from apps/api):

cmd: [ ! -f "$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.insta/observe/hook.js" ] || node "$(...)/.insta/observe/hook.js"
new form → status 0  stdout=[]      # resolves /tmp/mono/.insta/... which does not exist → silent no-op
old form → status 0  stdout=[RAN]   # node "<abs project path>/.insta/observe/hook.js"

So for nested projects the credential-audit hook goes from working to permanently dead, with no output at all — and re-running insta project link cannot fix it, since the entry is regenerated identically. The || pwd fallback does not help: it only fires when git fails outright, i.e. never inside a repo.

The faithful analogue of findProjectRoot is to climb from $PWD for the hook itself rather than ask git for a different root, e.g.

d="$PWD"; while [ ! -f "$d/.insta/observe/hook.js" ] && [ "$d" != "/" ]; do d=$(dirname "$d"); done
[ ! -f "$d/.insta/observe/hook.js" ] || node "$d/.insta/observe/hook.js"

which also satisfies the "runs from a subdirectory" requirement the current test at test/observe-install.test.ts:37-56 covers, works for non-git projects, and drops the two git forks noted under Suggestions.

2. Functionality (Windows) — the POSIX guard is handed to cmd.exe and fails after every tool call — src/observe/install.ts:82-84

Upstream normalizes hook commands as let command = if cfg!(windows) { command_windows.unwrap_or(command) } else { command }; (codex-rs/hooks/src/engine/discovery.rs) — with no commandWindows override the POSIX string is used verbatim on Windows and executed by cmd.exe, which cannot parse [ ! -f … ] or $( … ). It exits non-zero, and a non-zero hook exit is reported as a failure with hook exited with code {exit_code} (codex-rs/hooks/src/events/stop.rs).

The pre-PR command was node "<abs path>" — shell-agnostic and fine under cmd.exe. So this is a Windows regression on the exact axis the PR is about, in a repo that runs a dedicated test-windows CI job. The new test is posixTest (test/observe-install.test.ts:37), so it is skipped on win32 and cannot catch it; nothing else asserts the command shape. Please emit a commandWindows sibling in the hook object (camelCase in hooks.json, cf. statusMessage/additionalContextLimit in the upstream schema test) with a cmd.exe-parseable equivalent — or pick a guard form that is valid in both shells.

Suggestion

3. Security / functionality — an ignore entry does nothing for files that are already tracked — src/observe/install.ts:14, src/ensure-skills.ts:22

The Problem statement is that users have been discovering these files in git status; some of them will have committed .insta/audit.jsonl, .insta/observe/ or skills-lock.json already. Git ignores .gitignore for tracked paths, so for exactly those repos — the ones where the partial-fingerprint audit log is already in history — this change is a no-op and prints nothing. Cheap remedy: after ensureGitignore returns, git ls-files --error-unmatch <entry> per added entry and, on a hit, print a one-line git rm --cached <path> hint. That is also the only place a user is told history may need attention.

4. Performance — the command substitution is duplicated, so git rev-parse forks twice per tool call — src/observe/install.ts:82-84

${hook} is interpolated into both the [ ! -f … ] test and the node … arm, so every PostToolUse event pays two git process spawns before node even starts (the Claude entry pays none — it reads an env var). Assigning to a shell variable once, as in the fix sketched in finding 1, removes both.

5. Software engineering — the regression in finding 1 is exactly the case the new test does not cover — test/observe-install.test.ts:37-56

The new test builds the git repo at the project dir (git init in cwd), which is the one topology where toplevel and project root coincide. A sibling case — git init in a parent, project in parent/apps/api, hook expected to run — would have failed and is a three-line addition. Per the repo's TDD convention this is the test that belongs with this change.

6. Software engineering — installSkills adds ignore entries even when every npx skills add failed — src/ensure-skills.ts:99-104

The ensureGitignore call is unconditional inside the try, so an offline run that printed three "failed — add manually" lines still writes four ignore entries (now including skills-lock.json) for files that were never created, and prints .gitignore += … as if something happened. Pre-existing for the three dirs; the lock entry inherits it. Gating on at least one successful add would make the printed line honest.

Information

7. ensureGitignore matches entries by exact trimmed line (src/gitignore.ts:14-15). A project that already ignores .insta/ wholesale, .insta/observe without the trailing slash, or *.jsonl gets a redundant-but-harmless duplicate entry. Fine as-is; just noting the helper's contract is line equality, not ignore-semantics equivalence.

8. uninstallObserve (src/observe/install.ts:100-103) is now asymmetric with install: it unregisters both harnesses but leaves the two ignore entries behind. Probably correct — a stale audit.jsonl should stay ignored — but worth being a deliberate decision rather than an omission.

9. installObserve now writes a .gitignore into cwd even when cwd is not in a git repo at all, so insta project create in a plain directory creates a file the user did not ask for. Pre-existing behaviour for the skills path; observe now joins it. This is the mirror image of the insta secrets gap the PR body lists as a follow-up — worth settling one way for both.

10. Deliberately not ignoring .insta/ wholesale, and asserting that with expect(gi).not.toMatch(/^\.insta\/?$/m) (test/observe-install.test.ts:29), is the right call and a nice non-vacuous assertion. Same for routing the new .gitignore += … line through the existing quiet → stderr path in src/commands/project.ts:19-27, which keeps --json stdout single-document.

11. Both new user-visible lines are pure output additions — no command/flag surface change — so no skills/insta/cli-reference.md mirror is required by AGENTS.md rule 4.

---

Verdict

request_changes — findings 1 and 2 are Critical (a silently dead hook for nested projects, and a per-tool-call failure on Windows, both regressions against the current main behaviour). The .gitignore extraction, the skills-lock.json/audit.jsonl entries and their tests are good and I'd take them as-is; the Codex codexEntry() path is what needs another pass.

[cubic-dev-ai]

1 issue found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/gitignore.ts">

<violation number="1" location="src/gitignore.ts:17">
P3: When a new file is created, the template adds a leading newline due to the computed prefix. The unconditional newline should be dropped so a fresh file starts at its comment line.

Proposed fix: remove the unconditional leading newline logic and only add a leading newline when there is existing content that requires separation.

Suggested change:
- Replace the line setting prefix with logic that only adds a leading newline when there is existing content, and then compose the content accordingly.

Example fix (conceptual):

const leading = existing ? '\n' : ''
const separator = existing && !existing.endsWith('\n') ? '\n' : ''
writeFileSync(p, existing + ${leading}${separator}${header}${missing.join('\n')}\n)

</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P3: When a new file is created, the template adds a leading newline due to the computed prefix. The unconditional newline should be dropped so a fresh file starts at its comment line.

Proposed fix: remove the unconditional leading newline logic and only add a leading newline when there is existing content that requires separation.

Suggested change:

• Replace the line setting prefix with logic that only adds a leading newline when there is existing content, and then compose the content accordingly.

Example fix (conceptual):

const leading = existing ? '\n' : ''
const separator = existing && !existing.endsWith('\n') ? '\n' : ''
writeFileSync(p, existing + `${leading}${separator}${header}${missing.join('\n')}\n`)

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/gitignore.ts, line 17:

<comment>When a new file is created, the template adds a leading newline due to the computed prefix. The unconditional newline should be dropped so a fresh file starts at its comment line.

Proposed fix: remove the unconditional leading newline logic and only add a leading newline when there is existing content that requires separation.

Suggested change:
- Replace the line setting prefix with logic that only adds a leading newline when there is existing content, and then compose the content accordingly.

Example fix (conceptual):

const leading = existing ? '\n' : ''
const separator = existing && !existing.endsWith('\n') ? '\n' : ''
writeFileSync(p, existing + ${leading}${separator}${header}${missing.join('\n')}\n)

<file context>
@@ -0,0 +1,21 @@
+  const have = new Set(existing.split('\n').map((l) => l.trim()))
+  const missing = entries.filter((e) => !have.has(e))
+  if (missing.length === 0) return []
+  const prefix = existing && !existing.endsWith('\n') ? '\n' : ''
+  const header = comment && !have.has(comment) ? `${comment}\n` : ''
+  writeFileSync(p, existing + `${prefix}\n${header}${missing.join('\n')}\n`)
</file context>

Superseded: filed on 671a9c7 (POSIX-only Codex hook); fixed in 3dd7ea9 with a shell-neutral node -e form, tested through the platform shell on the Windows job

[jwfing]

Summary
The current head is close, but the Codex subdirectory path still writes audit state outside the ignored project root.

Requirements context
I based intent on the PR description, the linked PR page (#178), and local repo docs. CONTRIBUTING.md:17-18 and .claude/skills/developing-insta-cli/SKILL.md:17-19 document that project create|link write .insta/, observe hooks, and project skills; README.md:223-245 documents .insta/project.json as the project binding and says create/link install project skills plus the observe hook. I also checked the official Codex hooks docs (https://learn.chatgpt.com/docs/hooks), which state command hooks run with the session cwd and receive that cwd in stdin.

Findings

Critical

1. Functionality / Security: Codex subdirectory sessions can still create unignored audit logs — src/observe/install.ts:89-100, src/observe/hook.ts:37-41, test/observe-install.test.ts:56-72.
   The new wrapper climbs from process.cwd() and correctly finds the nearest .insta/observe/hook.js, but it then spawns the hook without telling it which project root was found. The real hook ignores its own script location and records into process.env.CLAUDE_PROJECT_DIR || event.cwd || '.'; for Codex, event.cwd is the session cwd. So if Codex starts in apps/api/src/routes, the wrapper runs apps/api/.insta/observe/hook.js, but any finding is appended to apps/api/src/routes/.insta/audit.jsonl, which is not covered by the project-root .gitignore entry .insta/audit.jsonl and will not be found by insta observe report from the project root. This misses the PR goal of gitignoring the local audit state and leaves the redacted/fingerprinted audit data exposed in git status for exactly the subdirectory case this PR now supports. A fix would be to pass the located root into the child, for example via an explicit env var or by setting CLAUDE_PROJECT_DIR: d, and add a test that runs the generated Codex command from a nested cwd using the real hook and asserts the audit file lands at the linked project root.

Suggestion
(none)

Information

1. Software engineering: the shared helper follows the repo’s side-effectful-module style, and the tests cover idempotent ignore entries, header de-duplication, tracked-file hints, Windows-safe command shape, and monorepo hook lookup — src/gitignore.ts:12-43, test/ensure-skills.test.ts:30-113, test/observe-install.test.ts:19-103.
2. Functionality: aside from the audit destination gap above, the implementation matches the PR intent for ignoring .insta/observe/, root .insta/audit.jsonl, and skills-lock.json, while keeping .insta/project.json committable — src/observe/install.ts:10-15, src/ensure-skills.ts:16-23.
3. Security: no new SQL, HTTP, auth, or dependency surface is introduced; the intended root audit-log ignore reduces accidental exposure risk, but the subdirectory case above still needs closure.
4. Performance: no hot-path concern found; the new synchronous file and git checks run during install/link flows or the small Codex wrapper only — src/gitignore.ts:12-43, src/observe/install.ts:112-124.
5. Verification: I kept this read-only per instruction and did not run npm run typecheck or npm test; git diff --check main...HEAD passed.

Verdict
request_changes — fix the Codex wrapper so the hook records audit state at the located project root before merge.

Superseded: filed on 3dd7ea9 (hook wrote audit.jsonl at the session cwd); fixed in 38743c3 — projectRootFor derives the root from the hook's own entry path, report/sync read from findProjectRoot(), real-hook e2e test from a nested cwd

[jwfing]

Summary
The PR is well-scoped and matches its stated intent: generated observe/skills state is ignored at creation time, tracked-file hints are surfaced, and the Codex hook no longer commits machine-specific absolute paths.

Requirements Context
I based the intent on the PR description at #178, the linked docs mirror at InsForge/insta-skills#74, and local repo docs: AGENTS.md, CONTRIBUTING.md, README.md, and .claude/skills/developing-insta-cli/SKILL.md. Those docs establish that project create|link install project-local side effects, .insta/project.json is the committable project binding, insta observe is a local credential-audit hook, and command/flag changes require cli-reference updates; this PR adds output/behavior only, not new commands or flags.

Findings

Critical

(none)

Suggestion

(none)

Information

• Software engineering / tests (test/observe-install.test.ts:50-130, test/ensure-skills.test.ts:71-90): Coverage is focused on the changed behavior: idempotent ignore entries, skills-lock.json, already-tracked hints, shell-neutral Codex command shape, nested cwd traversal, fresh-clone no-op, stdin passthrough, and project-root audit writes. I did not run vitest because the review request was read-only; npm run typecheck was attempted but failed because tsc is missing from this checkout.
• Functionality (src/observe/install.ts:89-124, src/observe/hook.ts:42-55, src/commands/observe.ts:15-19): The implementation matches the PR contract: the Codex hook is relative/portable, finds the nearest materialized .insta/observe/hook.js, passes stdin through, records findings at the linked root, and reports .gitignore additions plus already-tracked paths.
• Security (src/observe/install.ts:10-15, src/observe/install.ts:89-100, src/gitignore.ts:28-42): No new SQL/HTTP/auth paths or dependencies were added. The change reduces accidental check-in risk for .insta/audit.jsonl; the new shell command is constant and avoids shell-expanded characters, while git path checks are passed as argv rather than through a shell.
• Performance (src/gitignore.ts:28-34, src/observe/install.ts:112-124): No hot-path performance issue found. The added sync filesystem work and git ls-files call run only during install/link flows.
• Minor polish (src/gitignore.ts:18-20): When .gitignore does not exist, the helper writes a leading blank line before the comment. This is harmless and not worth blocking.

Verdict
Approved per the severity rule: no Critical findings.

[jwfing]

Summary
Reviewed at head 38743c3. Both Criticals from the earlier rounds are genuinely closed and I verified the fixes end-to-end with the built CLI — no blocking findings remain; the items below are all non-blocking.

Requirements context
This repo has no /docs/superpowers/ tree (no docs/ directory at all) — no matching spec/plan found, so I assessed against the PR title/body, AGENTS.md (non-negotiable 3: npm run typecheck && npm test; rule 4's cli-reference.md mirror doesn't apply — no command or flag surface changed), and the pre-existing behaviour on main (208d3c5). Codex hook semantics checked against upstream source via context7 (codex-rs/hooks/src/engine/command_runner.rs, codex-rs/config/src/hook_config.rs, codex-rs/hooks/src/engine/discovery.rs).

Adjudicating the earlier rounds (all three prior reviews are now dismissed, so nothing is standing):

• "git rev-parse --show-toplevel is not the insta project root" — closed. Replaced by the node -e climb at src/observe/install.ts:89-96; reverting to the rev-parse form reds 3 tests.
• "POSIX [ ! -f … ] hits cmd.exe on Windows" — closed. Confirmed upstream: default_shell_command() is %COMSPEC% /C on Windows and $SHELL -lc on POSIX. The new script avoids $, backtick, %, ! and inner "; I ran the generated command under sh -lc and bash -lc from a project dir, a nested subdir and an unrelated dir (0 / 0 / 0, correct output each time), and confirmed the dirname climb reaches a fixed point on every Windows path form (C:\, UNC \\server\share, \\?\C:\) — no unbounded loop.
• "Codex subdirectory sessions leave an unignored audit.jsonl" (round-2 review) — mechanism was accurate, but I reproduced the identical behaviour with main's absolute-path command form, so it was pre-existing in src/observe/hook.ts, not a regression introduced here. 38743c3 fixes it anyway, and I confirmed the fix with the real built hook: a Codex-shaped event carrying a credential, run from apps/api/src/routes, now appends to apps/api/.insta/audit.jsonl (fingerprint only, raw value absent), git add -A -n adds nothing under .insta/, and nothing is written at the session cwd.

Findings

Critical

(none)

Suggestion

Software engineering — the one untested behaviour change in the PR

1. src/commands/observe.ts:15-19 — the readAudit switch from process.cwd() to findProjectRoot() has no test. Negative control: replacing line 17 with const root = process.cwd() leaves the suite at 712/712 green, while 7 of the other 8 controls I ran red (see verification). This is the companion half of the 38743c3 fix — without it, an audit written at the project root is invisible to observe report/observe sync from a subdirectory — so it deserves the same coverage every other change here got.

Functionality — the two halves anchor on different files
2. src/commands/observe.ts:17 vs src/observe/hook.ts:42-49 — the hook derives the root from its own entry path (<root>/.insta/observe/hook.js), while readAudit derives it from <root>/.insta/project.json. Those agree for project create|link (which writes both), but not for a standalone insta observe install in an unlinked directory. Reproduced with the built CLI in apps/api (no project.json): findings land in apps/api/.insta/audit.jsonl, and insta observe report from apps/api/src/routes prints Audit log is empty — nothing recorded yet; dropping a project.json into apps/api/.insta/ makes the identical command render the report. Narrow regression vs main (pre-PR the hook wrote beside the session cwd, so report from that same cwd found it). One-line fix: when findProjectRoot() is null, fall back to climbing for .insta/observe/hook.js — the anchor the hook itself uses.
3. src/ensure-skills.ts:107 — if (installed === 0) return skips .gitignore maintenance entirely, including for skill dirs a previous successful run already created. An offline re-run on a machine that has .claude/skills/ + skills-lock.json from an older CLI leaves them un-ignored until the next successful add. The comment ("an offline run that failed every add has no skill dirs or lock to ignore") holds only for a first run.

Security — bound the ancestor climb
4. src/observe/install.ts:89-96 — the wrapper walks from the session cwd all the way to / and executes the first .insta/observe/hook.js it finds, whereas the pre-PR form was pinned to one absolute path. That is a new (small) trust surface: on a shared or CI box, a world-writable /tmp/.insta/observe/hook.js becomes the script that runs after every tool call for any hook-trusting project under /tmp with no closer .insta. .insta/observe/ being gitignored blocks the obvious clone-borne variant, and the attacker already needs filesystem write access, so this is hardening rather than a hole — but stopping the climb at the first .insta/project.json (or a small depth cap) keeps the monorepo fix and removes the walk to root.

Performance — a second node process per tool call
5. src/observe/install.ts:89-100 — every Codex tool call now starts two node processes (wrapper + hook) instead of one. Measured here, 20 iterations under sh -lc: 119 ms/invocation vs 75 ms for main's direct node <abs> (~+44 ms per tool call). Comfortably inside timeout: 15, so not blocking. If you want it back: import(pathToFileURL(h).href) inside the -e script runs the hook in-process (the materialized .insta/observe/package.json already declares type: module) and saves the second startup — dynamic import() works fine from a CJS -e context.

Software engineering — two test gaps
6. test/observe-install.test.ts:48 — runHook uses node's shell: true, which on Windows is %ComSpec% /d /s /c "<cmd>". Codex uses %COMSPEC% /C <cmd> — no /s, and /s is exactly the flag that forces unconditional outer-quote stripping. So a green test-windows job exercises a different quote-stripping regime than production. Codex supports a commandWindows override (hook_config.rs, commandWindows / command_windows, selected on Windows via command_windows.unwrap_or(command)); setting it explicitly, or asserting through cmd /c directly, would remove the guesswork. I could not confirm from the available upstream snippets whether the command string reaches cmd.exe via Rust's .arg() (which would escape the inner " as \" and break it) or .raw_arg() (which would be fine) — worth one manual run on a Windows box before release.
7. test/observe-install.test.ts:186 pins the legacy-Claude-entry migration but there's no equivalent for the legacy absolute-path Codex entry, which is the migration this PR exists for. I verified manually that a pre-0.0.59 .codex/hooks.json re-installs to exactly one entry (not stacked) — but that works via _insta: MARKER alone: isInstaHook's blob.includes('observe/hook.') fallback misses the Windows backslash form (C:\…\observe\hook.js), so the marker is load-bearing and unpinned.

Information

1. src/observe/install.ts:117-118 — installObserve now creates a .gitignore in directories that aren't git repositories at all, while insta secrets gates on .git being present (your own follow-up list notes that gap). The direction here is the better one; just flagging that the two commands now disagree.
2. src/ensure-skills.ts:22 — SKILL_DIRS now holds a file (skills-lock.json) alongside directories; the name no longer describes the contents.
3. src/ensure-skills.ts:14 — the ensureGitignore re-export exists only so the pre-existing test import keeps resolving; the test could import src/gitignore.ts directly and the re-export could go.
4. src/gitignore.ts:20 — appends LF-only lines regardless of the file's existing line endings, so a CRLF .gitignore on Windows ends up mixed. Harmless to git, visible in a diff.

Things I checked that are clean, for the record

• alreadyTracked behaves correctly when the insta project is below the git root: git ls-files emits cwd-relative paths and a pathspec for a non-existent directory exits 0, so from apps/api it returns ['.insta/audit.jsonl'] / ['skills-lock.json'] and empty outside a repo — no silent blindness (src/gitignore.ts:28-38).
• .gitignore entries contain a mid-string /, so they anchor to the project's own .gitignore directory, not the git root — correct for the monorepo case.
• --json discipline preserved: the new .gitignore += and untrack-hint lines route to stderr via tryInstallObserve(opts.json) / skillsPrint(opts.json) (src/commands/project.ts:20-28, 82, 101); stdout stays JSON-only.
• Exit-code propagation through the wrapper (hook exit 7 → wrapper 7), a 256 KB tool_input through the inherited stdin pipe (260,049 bytes delivered, status 0), and silent no-op with empty stdout+stderr when no .insta exists above.
• No new dependencies; no SQL/shell/HTTP surface; the removal of the baked /Users/<author>/… path from a committed .codex/hooks.json and the ignoring of .insta/audit.jsonl are both net security improvements.

Verification
Ran in a clean clone at 38743c3 (npm ci): npm run typecheck clean, vitest 712/712. Eight negative controls, seven red as expected — projectRootFor entry-path branch removed (2 red), Codex command reverted to the rev-parse form (3 red) and to the pre-PR absolute path (4 red), LOCAL_PATHS → ['.insta/'] (2 red), installed === 0 guard dropped (1 red), alreadyTracked → [] (2 red), header dedup removed (1 red), climb removed (1 red) — and the one green control is Suggestion 1. Plus the end-to-end runs described above with the built dist/ hook.

Verdict
approved — no Critical findings. (Informational: the green checkmark is still a separate human action.) Suggestions 1 and 2 are the two I'd most like to see before merge; the rest are take-or-leave.

[jwfing]

LGTM - approved.

[jwfing]

Summary

Round 4 review. The two Criticals I raised in round 1 (git rev-parse --show-toplevel ≠ insta project root; POSIX guard handed to cmd.exe) are genuinely fixed and now bound by tests; one Critical remains — auditRoot() prefers the link file over the materialized hook, so a linked parent + an install in a subdirectory makes observe report|sync silently print [] while findings sit on disk.

The PR merged while I was reviewing (head moved 38743c3 → 80900640, squashed to main as 29bb82d). I re-verified every finding against 29bb82d, not against the head I started on. The REQUEST_CHANGES below is therefore a follow-up against main, not a merge gate — nothing here needs unwinding, one line needs a follow-up PR.

Requirements context

No matching spec/plan found — this repo has no /docs/superpowers/, and in fact no docs/ tree at all (no docs/specs/, no plans/). Assessed against the PR description, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md. External API claims were checked against the real Codex source rather than assumed:

• codex-rs/hooks/src/engine/command_runner.rs — POSIX runs the command as $SHELL -lc <command>, Windows as %COMSPEC% /C <command>. The character-set constraint in the comment block (src/observe/install.ts:86-88) is exactly the right invariant for both, and the not.toMatch(/[$%!]/)` assertion is a real guard, not decoration.
• Same file, run_command: command.current_dir(cwd) — the hook child is explicitly given the session cwd, the same value that lands in the event JSON's cwd. This validates the whole climb-from-process.cwd() premise; it is not a guess, and it holds under codex --cd/-C too.
• env_clear() + session env snapshot, stdout/stderr piped, exit ≠ 0 → HookRunStatus::Failed ("hook exited with code N"). Relevant to the Suggestion on exit codes below.

Gates reproduced in a clean clone of 29bb82d: tsc --noEmit clean, vitest run 713/713.

Negative controls — all four new behaviours are bound by tests that go red on the pre-change form:

Reverted to	Result
projectRootFor → CLAUDE_PROJECT_DIR || eventCwd || '.' (round-2 form)	2 red, incl. the tsx end-to-end test
codexEntry → [ ! -f "$(git rev-parse --show-toplevel …)/…" ] || node … (round-1 form)	3 red
alreadyTracked → always []	2 red
drop if (installed === 0) return	1 red

That is a real TDD trail, and the tsx-loaded end-to-end test that drives the generated Codex command through the platform shell with a Codex-shaped event is the right shape for this bug class.

---

Findings

Critical

[functionality] auditRoot() prefers the link file over the materialized hook, so the two halves still disagree — src/commands/observe.ts:19-29

auditRoot returns findProjectRoot(cwd) whenever a link file exists anywhere above, and only climbs for .insta/observe/hook.js when there is none. But the hook writes at its own entry path (projectRootFor, src/observe/hook.ts:42-48), and the installers materialize at process.cwd() (src/commands/project.ts:22,83,102; src/commands/observe.ts:49) — while writeProject deliberately targets the existing root (src/config.ts:143-147). So the moment those two diverge, the link file wins and the reader looks in a directory the writer never touches.

Failure scenario, all CLI-produced, no hand-editing:

1. cd /repo && insta project link → project.json, hook, .gitignore all at /repo. Fine.
2. Later, from /repo/apps/api, run insta project link again (re-link, switch project) or insta observe install. writeProject correctly updates /repo/.insta/project.json; installObserve({cwd: process.cwd()}) materializes a second hook — plus a new .gitignore, .codex/hooks.json, .claude/settings.json — under apps/api.
3. A Codex/Claude session in apps/api runs /repo/apps/api/.insta/observe/hook.js, which records to /repo/apps/api/.insta/audit.jsonl.
4. insta observe report / insta observe sync, from anywhere → auditRoot() → /repo → [] / "nothing to sync".

Reproduced against 29bb82d in a scratch test:

auditRoot(sub) = /tmp/z178-81ch7u    hook is materialized at /tmp/z178-81ch7u/apps/api
observe report --json  >>>  "[]\n"

(The same scenario on bd22dff (base) prints the finding, because readAudit used process.cwd(). So this is a delta introduced here, not pre-existing.)

This is the exact class of bug round 3 set out to close ("so both halves agree from any subdirectory") — the remedy just landed on the wrong precedence. A credential-audit tool reporting zero findings when findings exist is the worst failure mode it has: it is indistinguishable from a clean run.

Fix is one swap — climb for the materialized hook first, fall back to the link file:

export async function auditRoot(cwd = process.cwd()): Promise<string> {
  let dir = resolve(cwd)
  for (;;) {
    if (existsSync(join(dir, '.insta', 'observe', 'hook.js'))) return dir
    const parent = dirname(dir)
    if (parent === dir) break
    dir = parent
  }
  return (await findProjectRoot(cwd)) ?? cwd
}

This is strictly better in every enumerated case: when the hook is at the link root (the normal case) both orders return the same directory; when they differ, only the hook path names where findings actually are. Alternatively — and arguably the more principled fix — anchor the installers on findProjectRoot() ?? cwd the way writeProject already does, so step 2 never creates the second install in the first place. Either closes it; the second also stops a stray .gitignore/.codex/ appearing in a monorepo subdirectory.

Suggestion

[software engineering] The new auditRoot test enumerates the three cases that pass and skips the one that fails — test/observe-install.test.ts:125-144

The test covers linked, hook-only, and neither. It never constructs the fourth state — link file and materialized hook both present at different depths — which is the only one where the precedence choice is observable, and the one that is wrong. A fourth assertion in the same test would have caught the Critical above:

const mono = realpathSync(mkdtempSync(join(tmpdir(), 'obs-mono-')))
mkdirSync(join(mono, '.insta'), { recursive: true })
writeFileSync(join(mono, '.insta', 'project.json'), '{"projectId":"p","orgId":"o","branch":"main"}')
const project = join(mono, 'apps', 'api')
mkdirSync(project, { recursive: true })
installObserve({ cwd: project, assetDir: fakeAssets() })
expect(await auditRoot(join(project, 'src'))).toBe(project) // where the hook actually writes

More generally: observeReport / observeSync still have no test at any level (nothing in test/ imports src/commands/observe.ts except for auditRoot). The helper is covered; the command that consumes it is not.

[functionality] A spawn failure is reported to Codex as a failed hook after every tool call — src/observe/install.ts:94

process.exitCode = r.status === null ? 1 : r.status maps "the child could not be started, or was killed by a signal" (spawnSync sets status: null and populates error) to exit 1. Per codex-rs/hooks, any non-zero exit is HookRunStatus::Failed, surfaced as hook exited with code 1 — after every tool call, until the user finds it. Every other arm of this install path is deliberately silent-best-effort: the [ ! -f … ] guard on the Claude entry, the fresh-clone no-op, the catch {} around the .gitignore write, catch {} around both registerHarness calls. Exiting 0 when the child never ran would match that contract; the inner hook.js already exits 0 unconditionally, so nothing is lost.

[software engineering] installed === 0 also skips the already-tracked hint — src/ensure-skills.ts:107

Noted that this was consciously declined for the ignore entries, and that reasoning holds (they land on the next successful link, which is also when the files are rewritten). The hint is a different thing though: a repo that committed skills-lock.json months ago and then has one offline run gets no git rm -r --cached line, even though the tracked-file problem is entirely independent of whether this run installed anything. Hoisting untrackHint(alreadyTracked(...)) above the gate is free and doesn't reintroduce the claim the gate exists to prevent.

Information

[security] The "so shareable is safe" claim is slightly stronger than the trust mechanism supports — src/observe/install.ts:88

Codex's trust record (currentHash / trustStatus, per hooks/list) covers the hook entry, not the .insta/observe/hook.js the command resolves at runtime — which is the point the recorded decline on bounding the climb already concedes ("which lists no path at all"). I agree with the disposition: a depth cap would trade away the monorepo fix, and an attacker who can write .insta/observe/hook.js under your tree already has write access. But the comment sentence asserts more than that reasoning does. Worth amending it to say what is actually true — the entry is trusted, the resolved script is whatever is nearest above the session cwd — so the next reader doesn't inherit the stronger claim.

[performance] No hot-path concerns; the costs are all bounded and small.

• Two Node startups per tool call instead of one — already measured and accepted (~+44 ms), well inside timeout: 15. No objection.
• The climb is one existsSync per ancestor directory, so ~5-15 stats from a typical project depth. Negligible.
• alreadyTracked forks git ls-files once per installObserve / installSkills, i.e. once per insta project create|link, not per tool call. Fine — flagging only because "no git forks" is listed as a property of the new Codex form; it holds for the hook, not for the install path.
• No new dependencies, no new queries, no unbounded loops (both climbs terminate on dirname(dir) === dir).

[security] git rm -r --cached does not purge history — src/gitignore.ts:39

The hint's wording ("to stop committing") is accurate and doesn't overclaim. Still, the repos this targets are precisely the ones with audit.jsonl — partial secret fingerprints and redacted context — already in their history, where the untrack leaves every past commit intact. A pointer to that (in the hint or the observe docs) would serve those users.

[software engineering] Cosmetic: ensureGitignore emits a leading blank line when it creates the file from scratch (existing is '', so prefix is '' and the literal \n lands first) — src/gitignore.ts:14-19.

---

What I checked and found clean

• Legacy migration. isInstaHook matches the old absolute-path entry both by _insta marker and by the observe/hook. substring, and upsert filters before pushing — so an upgrade replaces the stale entry rather than stacking a duplicate. Verified by reading upsert/registerHarness, and the new command still contains .insta/observe/hook.js so the fallback key survives (asserted at test/observe-install.test.ts:60).
• Shell neutrality. The generated command holds no $, backtick, %, ! or inner "; it is CJS-shaped (require) so Node's --eval syntax detection can't flip it to ESM; it survives sh -lc, cmd /C, and also fish/csh where the old POSIX form would not.
• Redaction. The end-to-end test asserts the audit contains a fingerprint and not the raw secretpass, and that nothing is written at the session cwd. No secrets, tokens or PII newly logged or returned anywhere in this diff.
• Auth/authorization. Untouched. observeSync still goes through ApiClient.load() + requireProject().
• .gitignore scoping. .insta/observe/ and .insta/audit.jsonl, never .insta/ wholesale — project.json stays committable, and there's an explicit not.toMatch(/^\.insta\/?$/m) guarding it.

Verdict

request_changes — one Critical (auditRoot precedence, src/commands/observe.ts:19-29), reproduced against merged main (29bb82d). Since the PR is already merged, this is a follow-up PR, not a revert: swap the two branches in auditRoot (or anchor the installers on findProjectRoot() ?? cwd) and add the both-present case to the auditRoot test. Everything else above is non-blocking.

[tonychang04]

Re the round-4 (post-merge) review: the remaining Critical (auditRoot preferring the link file over the materialized hook) plus the three suggestions (fourth auditRoot case, wrapper exit 0 on spawn failure, hint independent of the installed gate) are in the follow-up PR fix/observe-root-anchor. Installers now also anchor at the linked root (same rule as writeProject) so a re-link from a subdirectory refreshes the project's hook instead of minting a second one.