Add ghostel-pre-spawn-hook for env injection at spawn time

[dakra]

Summary

• New ghostel-pre-spawn-hook defcustom that fires inside ghostel--spawn-pty immediately before make-process, with process-environment dynamically bound to the about-to-be-spawned env.
• Hook functions call setenv to inject or override entries that the spawned process inherits at fork time.
• One firing site covers both spawn paths (ghostel--start-process and ghostel-exec). ghostel-compile has its own make-process and is not covered, matching today's separation.

Replaces the earlier post-spawn ghostel-exec-hook design

The first revision of this branch added a post-spawn hook that fired after make-process, intended to be paired with with-editor-export-editor (which sends export EDITOR=…\n to a live shell). On review, pre-spawn env injection turned out to be strictly better:

	post-spawn ghostel-exec-hook (v1)	pre-spawn ghostel-pre-spawn-hook (this PR)
User config	one add-hook	one add-hook
with-editor patch	new cond branch in with-editor-export-editor	one new public function (with-editor-setup-environment)
Buffer noise (export … line)	needs HISTCONTROL/leading-space dance	none
Race with shell rc files	yes (bytes queue before rc finishes)	none — env set at fork
Fires harmfully into TUIs (htop, claude via ghostel-exec)	yes — bytes interpreted as keystrokes	no — env in fork is harmless

Motivation

with-editor needs to set EDITOR so Magit's commit-edit / rebase-edit flows pop their files in the running Emacs. The value can't be put in ghostel-environment (it depends on the live Emacs server's server-name / server-process / server-auth-dir, computed at runtime), and a mode-hook fires too early — before the PTY exists.

Intended user config (after the matching with-editor patch)

(add-hook 'ghostel-pre-spawn-hook #'with-editor-setup-environment)

The with-editor-setup-environment side is a small upstream change — a public wrapper around the existing private with-editor--setup — and will be proposed separately.

Test plan

• make -j4 all clean (build + lint + test + test-native, 219/219).
• ghostel-test-pre-spawn-hook-injects-into-process-environment drives a real /bin/sh through ghostel--start-process, has the hook setenv a sentinel value, and asserts the value reached make-process's process-environment (also addresses the v1 review's NITs about real-process coverage).
• Live-verify with the matching with-editor change: git commit --allow-empty from a ghostel buffer pops COMMIT_EDITMSG; C-c C-c returns the shell with exit 0.

[dakra]

@claude review

[claude]

Claude finished @dakra's task in 2m 42s —— View job

---

Claude Code review

• Read PR diff and changed files
• Read CLAUDE.md and src/CLAUDE.md for project conventions
• Inspect existing hook patterns and ghostel--start-process / ghostel-exec
• Inspect new ERT test for correctness
• Post review

---

Overall: small, self-contained, and well-motivated. The two spawn paths and the docstring contract line up; integration shape matches term-exec-hook. A few observations, mostly NIT.

Findings

1. Test coverage skips ghostel--start-process (NIT)
test/ghostel-test.el:8318 only exercises ghostel-exec. The motivating use case (with-editor-export-editor in a regular M-x ghostel shell) goes through ghostel--start-process (lisp/ghostel.el:3657), which is the more complex of the two paths (remote integration, temp-paths cleanup running before the hook). Other tests in this file already drive ghostel--start-process with a real /bin/sh (e.g. test/ghostel-test.el:7514), so a second case verifying ghostel--process is bound and live when the hook fires there is straightforward and worth adding.

2. Hook order in ghostel--start-process interacts with remote temp-path cleanup (QUESTION)
At lisp/ghostel.el:3653, ghostel--cleanup-temp-paths runs synchronously before run-hooks. For remote integration the temp .zshenv / inject script may not have been sourced yet by the just-spawned shell; this race is pre-existing, not introduced here. Worth confirming you're OK with the new hook landing on the post-cleanup side. If users want to send shell-side commands that need integration env in place, the hook still fires fast enough that the bytes queue in the PTY before the shell has read its rc — so the practical answer is probably "fine", same as for term-exec-hook. Just calling it out since the PR places the hook deliberately at the very end.

3. Docstring at lisp/ghostel.el:381 could caution about non-shell ghostel-exec (NIT)
Because the hook fires from both ghostel--start-process and ghostel-exec, adding with-editor-export-editor globally will also blast export EDITOR=...\n into anything spawned via (ghostel-exec buf "htop"), claude, etc. — those programs receive the bytes as keystrokes. term-exec-hook has the same property, so this matches precedent, but a one-liner in the docstring suggesting hook functions gate on buffer/program when needed (and noting that the hook fires for all spawn paths, not only the shell path) would save users a footgun. The example of export EDITOR specifically only makes sense for shell spawns.

4. process-live-p stub in the new test (NIT)
test/ghostel-test.el:8341 cl-letfs process-live-p to recognize the symbol 'fake-proc. That means the captured captured-live only verifies "the stub returned t", not "this is actually a live OS process". The ordering claim (ghostel--process is set before the hook fires) is what the test really pins down, and that part is valid. If you add the ghostel--start-process case in (1) using a real /bin/sh like the existing tests do, you'll get an end-to-end check of the live-process contract for free and can leave this unit-style test as is.

5. CHANGELOG not updated (NIT)
CHANGELOG.md has an [Unreleased] section and recent additions of new defcustoms (e.g. ghostel-spinner-progress) get an ### Added entry. Consider adding one for ghostel-exec-hook.

Non-issues I checked

• Bare run-hooks (vs. the ghostel--run-hook-safely pattern at lisp/ghostel.el:2614) is correct here. That helper exists to isolate per-handler errors in parser-path hooks that fire on every byte; a once-per-spawn lifecycle hook should signal normally, matching term-exec-hook.
• Both call sites correctly run the hook with the ghostel buffer current and ghostel--process already set by ghostel--spawn-pty (lisp/ghostel.el:3553), so the docstring contract holds.
• ghostel-exec returning the proc value is preserved by the let/proc rewrite at lisp/ghostel.el:4298-4302.
  • Branch: with-editor-compat

[dakra]

@claude review

[claude]

Claude finished @dakra's task in 3m 46s —— View job

---

Claude Code review

• Read PR diff and changed files
• Read CLAUDE.md and check repo conventions
• Inspect new defcustom and hook firing sites (ghostel--spawn-pty, ghostel--start-process, ghostel-exec)
• Inspect ERT test for correctness
• Compose review

---

Overall: this v2 redesign is a meaningful win over the v1 post-spawn-hook approach — env injection at fork time sidesteps the byte-stream race against shell rc, keeps TUI spawns clean, and the firing site (inside ghostel--spawn-pty, between the process-environment let* and make-process) is exactly where it needs to be. The previous review's NITs are largely addressed:

• NIT Improve code quality, adaptive redraw, and CI coverage #1 (test hits ghostel--start-process): addressed at test/ghostel-test.el:8318.
• NIT Replace manual lint CI with melpazoid #3 (caution about ghostel-exec blasting bytes into TUIs): no longer applicable — env injection is harmless to TUIs.
• NIT Fix bottom lines cut off when TUI apps fill the screen #4 (process-live-p stub): no longer applicable — test now uses a real /bin/sh.
• NIT Add errdefer fix, heap fallback, ERT migration, and theme sync #5 (CHANGELOG): added at CHANGELOG.md:7-16.

A few remaining observations, all NIT.

Findings

1. Test exercises ghostel--start-process but not ghostel-exec (NIT)
test/ghostel-test.el:8318 covers the more complex spawn path. Since the hook fires inside the shared ghostel--spawn-pty, a ghostel-exec test would mostly be redundant — but it is the only path that documentably forwards user-supplied program args, and asserting the hook still fires for it would lock the docstring contract ("Fires for both spawn paths") into the suite. A 5-line companion test that calls (ghostel-exec test-buffer "/bin/sh" '("-c" "exit 0")) and asserts setenv lands in captured-env would do it.

2. A pre-spawn-hook error leaks remote temp paths (NIT)
lisp/ghostel.el:3665-3670 calls ghostel--cleanup-temp-paths only after ghostel--spawn-pty returns successfully. If a user's pre-spawn hook signals (e.g. a buggy with-editor-setup-environment), the temp .zshenv/inject scripts under remote-integration :temp-files are never cleaned up. The make-process-fails case has the same leak today, so this PR doesn't introduce the bug — but it materially widens the failure surface (any user hook can trip it). An unwind-protect around the spawn-pty call would close it; out of scope for this PR but worth a follow-up. Fix this →

3. Docstring could note that ghostel--process is unbound/stale when the hook fires (NIT)
At lisp/ghostel.el:381-395, the hook is documented to run "in the buffer that will host the new process" — accurate, but on a buffer where a prior ghostel-exec died, ghostel--process is still bound to the old dead proc at hook time (see the live-only guard at lisp/ghostel.el:4285). Hook authors who reach for ghostel--process will get the stale value. One sentence — "the buffer-local ghostel--process is not the new process (which does not yet exist) and may be a dead handle from a prior spawn; do not rely on it from this hook" — would prevent the footgun. (FYI: my own previous-review note that ghostel--process is "already set by ghostel--spawn-pty" was wrong — it's set after the hook fires, at line 3567. The current PR docstring correctly does not promise otherwise.)

4. Test asserts on a process that hasn't been awaited (NIT)
test/ghostel-test.el:8349-8356: the test calls ghostel--start-process (which spawns /bin/sh -c "stty ...; printf '\033[H\033[2J'; exec /bin/sh") and immediately runs assertions, then delete-process. The assertions only depend on captured-env (set synchronously in the wrapped make-process), so the test is correct regardless of whether the shell has finished initializing — but the comment block hints at a "real-process" intent. If the goal is to demonstrate the env actually reached the child (vs. just make-process), feeding a shell that prints $GHOSTEL_PRE_SPAWN_TEST and reading it out of the buffer would be a stronger end-to-end check. As written, this is really a make-process-call-site assertion, which is fine and probably what you want for hermeticity.

Non-issues I checked

• Bare run-hooks (vs. ghostel--run-hook-safely at lisp/ghostel.el:2623) is the right call: that helper isolates per-handler errors for parser-path hooks that fire on every byte; a once-per-spawn lifecycle hook should signal normally so a buggy integration is loud.
• setenv inside the hook does mutate the let-bound process-environment (it does setq on the dynamically bound symbol), so the spawn picks up the change — confirmed against the let* at lisp/ghostel.el:3531-3553.
• The let/proc rewrite at lisp/ghostel.el:3559-3576 preserves the outer-let* scoping; extra-env, tramp-terminal-type, etc. are still in scope where needed, and ghostel-exec's return value (proc) is unchanged.
• process-environment ordering: ghostel-environment → INSIDE_EMACS=ghostel+terminal-env → extra-env → inherited env, then hook-injected entries get prepended by setenv. So a hook setenv overrides ghostel-environment. That's the right precedence.
• Buffer-local hook via (add-hook ... nil :local) works because the hook fires with the spawn buffer current at lisp/ghostel.el:3558.
  • Branch: with-editor-compat