Skip to content

fix(agent-hooks): keep a dev run's endpoint file out of the installed profile - #272

Open
miadisabelle wants to merge 2 commits into
johannesjo:mainfrom
miadisabelle:contrib/dev-instance-hook-endpoint
Open

fix(agent-hooks): keep a dev run's endpoint file out of the installed profile#272
miadisabelle wants to merge 2 commits into
johannesjo:mainfrom
miadisabelle:contrib/dev-instance-hook-endpoint

Conversation

@miadisabelle

Copy link
Copy Markdown

The dev-instance cross-wiring you spotted reviewing #270. Your diagnosis was right; two things about it turned out to be worse than the comment said, and the fix wanted to be slightly bigger than one line.

Why it isn't one line

The suffix rule already exists twice, verbatim in shape: getStateDir() in persistence.ts:5 and getKeybindingsDir() in register.ts:769. It is missing from three places — agent-hooks/runtime.ts:18 and both arena handlers. Mirroring it into runtime.ts would have made a third copy of a rule that has now been forgotten more often than it has been remembered.

So electron/user-data-dir.ts instead: resolveUserDataDir(userDataPath, isPackaged) is pure, so the rule is testable without an Electron runtime, and getUserDataDir() is what callers use. After this, app.getPath('userData') appears exactly once in the tree, which makes "someone reached past the helper" a one-line grep instead of an audit.

Same reasoning you used on #270 for the third call site: leaving the copies in place would leave the next one to drift.

Worse than the comment said

The misroute succeeds. The port and the token both come from endpoint.env, so an agent launched by instance A posts to instance B and passes tokenMatches cleanly. Nothing logs on either side — coordinator.ts:318 returns early on an unknown agent id, and applyAgentHookEvent never checks the agent belongs to this instance, so the wrong instance writes phantom status entries.

Quitting one instance breaks hooks for the other. endpoint.env is written only inside startAgentHookServer, at startup. Close the instance that owns the file and it still names a dead port, so the surviving instance's agents post into nothing and fall back to PTY heuristics until that instance is itself restarted.

Observed, not just reasoned

Neither CI nor the tests exercise the dev/packaged separation — the unit tests cover the string rule only. Same gap as the lock in #270, so the same treatment: packaged build under Xvfb, dev run started second, both pointed at the same --user-data-dir.

Before (upstream runtime.ts) — the dev run overwrites the packaged instance's file:

packaged run  → prof/agent-hooks/endpoint.env      PORT=37127
                prof-dev/agent-hooks/endpoint.env  (absent)
dev run       → prof/agent-hooks/endpoint.env      PORT=41559   ← overwritten
                prof-dev/agent-hooks/endpoint.env  (absent)

After — the packaged file is untouched:

packaged run  → prof/agent-hooks/endpoint.env      PORT=42473
                prof-dev/agent-hooks/endpoint.env  (absent)
dev run       → prof/agent-hooks/endpoint.env      PORT=42473   ← unchanged
                prof-dev/agent-hooks/endpoint.env  PORT=46743

Packaged builds are unchanged

resolveUserDataDir returns its input on the first line when isPackaged, and getUserDataDir() is its only caller. All four converted sites resolve to exactly the same paths they did before in a packaged build. agent-hooks needs no migration either — endpoint.env, hook.sh and claude-settings.json are rewritten every launch.

The second commit is droppable

SaveArenaData/LoadArenaData are the same slip without the silent misrouting, found while making the first fix. It is a behavior change for dev runs: arena-presets.json and arena-history.json stay with the installed build, so a dev run picks this up as an empty arena. Split out so you can drop it and keep this PR to agent-hooks.

Checks

npm run compile, npm run check:static, vitest run (2216 passed / 25 skipped) and npm run test:client (36 passed / 6 skipped) all green.

One note in case it is useful to you beyond this PR: npm run typecheck and npm run check:static do not type-check electron/** — root tsconfig.json has "include": ["src", "electron/ipc/channels.ts"], so only npm run compile covers the main process. I had a main-process type error passing both of the checks you used to verify #270 before I caught it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MoJfWuU93BeL2W48mz9bC6

… profile

`getStateDir()` in persistence.ts and `getKeybindingsDir()` in register.ts
carry the same rule, copied twice: take `userData`, append `-dev` when the
build is not packaged, so `npm run dev` does not read or write an installed
build's data. `agent-hooks/runtime.ts` joins raw `userData` and gets no such
separation.

That directory holds `endpoint.env` — the loopback port and the bearer token
the generated hook script posts with. Both instances write it, and it is
written only inside `startAgentHookServer`, so:

- Whichever instance started last owns the file. Every Claude agent launched
  outside Docker re-sources it on every hook event, so agents belonging to the
  *other* instance post their status there. The token comes from the same file,
  so the request authenticates and succeeds — an instance reports status for
  agents it never launched, with nothing logged on either side.
- When that instance quits, the file still names its dead port. Nothing rewrites
  it until an app starts again, so the surviving instance's agents post into
  nothing and fall back to PTY heuristics until that instance is itself
  restarted.

Rather than add a third copy of the suffix rule, extract it:
`electron/user-data-dir.ts` exports `resolveUserDataDir(userDataPath, isPackaged)`
— pure, so the rule is testable without an Electron runtime — and
`getUserDataDir()` for callers. persistence.ts (state.json and custom themes),
the keybindings handlers and agent-hooks now all go through it, and reaching
past it is the thing to look for in review.

Packaged builds resolve exactly as before: `resolveUserDataDir` returns its
input unchanged when `isPackaged`.

3 unit tests on the rule. `npm run compile`, `npm run check:static` and the
full suite pass.
Same class as the agent-hooks fix, found while making that one: SaveArenaData
and LoadArenaData join `app.getPath('userData')` directly, so a dev run reads
and writes the installed build's `arena-*.json`.

No silent misrouting here the way the hook endpoint had — it is the plainer
version of the same slip. Worth knowing before merging: a dev run picks this
change up as an empty arena, since `arena-presets.json` and
`arena-history.json` stay with the installed build.

Separate commit because it is not the bug that prompted the change; drop it if
you would rather keep this PR to agent-hooks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant