Improve code quality, adaptive redraw, and CI coverage

[dakra]

• Extract shared OSC terminator helper (findOscTerminator + OscScanner)
  to deduplicate terminator detection across OSC 7, 52, and 133 parsers
• Check ghostty_terminal_set return values via terminalSet helper instead
  of silently discarding them
• Add adaptive frame rate: shorter initial delay after idle for responsive
  interactive feedback, configurable via ghostel-adaptive-fps
• Add native module build job to CI using Zig 0.14

https://claude.ai/code/session_01DvxJHyLFPaaZTNYcEFN2hQ

[Copilot]

Pull request overview

This PR improves reliability and UX around the Ghostty-backed Emacs terminal by deduplicating OSC parsing logic, handling native terminal option set failures more explicitly, introducing adaptive redraw timing, and expanding CI to build the native module.

Changes:

• Added a terminalSet helper and updated Terminal setter APIs to return errors instead of discarding ghostty_terminal_set results.
• Refactored OSC 7/52/133 parsing to share terminator scanning via findOscTerminator + OscScanner.
• Added native module auto-download/load support and adaptive redraw timing in Elisp, plus a new CI job to build the native module with Zig.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/terminal.zig	Adds terminalSet helper and converts several setters to !void so failures can be handled by callers.
src/module.zig	Updates callback/color setup to handle new error-returning APIs; refactors OSC parsing via shared scanner helpers.
ghostel.el	Adds module auto-download helpers, adaptive redraw delay logic, and updates module loading behavior.
build.zig	Makes Emacs include path configurable and fixes installed artifact suffix based on platform.
.github/workflows/ci.yml	Adds a matrix job to build and upload the native module on Linux/macOS using Zig 0.14.1.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The constructed download URL uses /releases/download/latest/... when ghostel--package-version returns nil. GitHub's 'latest release' download URLs use /releases/latest/download/<asset> (or a concrete tag), so this path will 404 and prevent auto-download for non-package installs (and any case where the version can't be resolved).

Suggested change

	(url (format "%s/download/%s/%s"
	ghostel-github-release-url
	(if version (concat "v" version) "latest")
	asset-name))
	(url (if version
	(format "%s/download/v%s/%s"
	ghostel-github-release-url
	version
	asset-name)
	(format "%s/latest/download/%s"
	ghostel-github-release-url
	asset-name)))

[dakra]

Fixed in 3f7d349 — now uses /releases/latest/download/<asset> for the fallback URL.

---

Generated by Claude Code

[Copilot]

This change introduces automatic download + module-load of a prebuilt native module from the network without any integrity/authenticity verification (checksum/signature/pinned commit). That is a significant supply-chain risk; consider requiring explicit user confirmation before downloading/loading, and/or verifying an expected hash/signature published with the release before calling module-load.

[dakra]

Addressed: auto-download is now opt-in via ghostel-auto-download-module (default nil). Users must explicitly enable it or use M-x ghostel-download-module. Also added HTTPS enforcement — non-HTTPS URLs are rejected before downloading.

---

Generated by Claude Code

[Copilot]

ghostel-module-compile is now defined twice in this file (once here, and again near the end under "Module compilation"). The later definition will overwrite this one, which is confusing and can break expectations (different default-directory, different compile invocation). Please keep a single definition and remove/merge the duplicate.

Suggested change

	(defun ghostel-module-compile ()
	"Build the native module from source using build.sh."
	(interactive)
	(let ((default-directory (file-name-directory
	(or load-file-name
	(locate-library "ghostel")
	buffer-file-name))))
	(compile "./build.sh")))

[dakra]

Fixed in 3f7d349 — removed the duplicate, keeping the pre-existing definition.

---

Generated by Claude Code

[Copilot]

ghostel--download-file doesn't kill the buffer returned by url-retrieve-synchronously, which can leak temporary buffers over time (and during retries). Consider binding the result to a variable and using unwind-protect to ensure the buffer is killed after extracting/writing the payload.

Suggested change

	(with-current-buffer (url-retrieve-synchronously url t t 30)
	(set-buffer-multibyte nil)
	(goto-char (point-min))
	(when (re-search-forward "^HTTP/[0-9.]+ 200" nil t)
	(when (re-search-forward "\r?\n\r?\n" nil t)
	(let ((coding-system-for-write 'binary)
	(start (point)))
	(when (< start (point-max))
	(write-region start (point-max) dest nil 'silent)
	;; Make executable on Unix
	(set-file-modes dest #o755)
	t))))))
	(let ((buf (url-retrieve-synchronously url t t 30)))
	(when buf
	(unwind-protect
	(with-current-buffer buf
	(set-buffer-multibyte nil)
	(goto-char (point-min))
	(when (re-search-forward "^HTTP/[0-9.]+ 200" nil t)
	(when (re-search-forward "\r?\n\r?\n" nil t)
	(let ((coding-system-for-write 'binary)
	(start (point)))
	(when (< start (point-max))
	(write-region start (point-max) dest nil 'silent)
	;; Make executable on Unix
	(set-file-modes dest #o755)
	t)))))
	(when (buffer-live-p buf)
	(kill-buffer buf))))))

[dakra]

Fixed in 3f7d349 — now uses unwind-protect to always kill the buffer.

---

Generated by Claude Code

[Copilot]

time-subtract expects two time values; passing nil here will raise an error on the second and subsequent redraw scheduling when ghostel-adaptive-fps is enabled (breaking redraw). Use (current-time) (or time-since) instead of nil when computing idle-secs.

Suggested change

	(time-subtract nil
	(time-subtract (current-time)

[dakra]

Fixed in 3f7d349 — changed to (current-time).

---

Generated by Claude Code

[Copilot]

Since this code now may auto-download the native module, module-load failures (corrupt download, wrong architecture/suffix, partial write) will throw an error during (require 'ghostel) and can prevent the package from loading at all. Consider wrapping module-load in condition-case and falling back to a warning + instructions, rather than erroring out during load.

[dakra]

Fixed in 3f7d349 — module-load is now wrapped in condition-case, falling back to a display-warning with instructions instead of erroring out.

---

Generated by Claude Code

[Copilot]

This still discards the return value from ghostty_terminal_set when setting OPT_PWD. Since the PR is introducing terminalSet to detect failures, this path should also check the result (and decide how to handle failure) so PWD updates don't silently fail.

Suggested change

	_ = gt.c.ghostty_terminal_set(term.terminal, gt.OPT_PWD, &gs);
	const rc = gt.c.ghostty_terminal_set(term.terminal, gt.OPT_PWD, &gs);
	if (rc != 0) {
	std.log.err("ghostty_terminal_set(OPT_PWD) failed with error code {d}", .{rc});
	}

[dakra]

Fixed in 3f7d349 — added Terminal.setPwd method that checks the return code, used by extractAndSetPwd.

---

Generated by Claude Code

[Copilot]

The build-native job runs ./build.sh but doesn't install Emacs development headers or set EMACS_INCLUDE_DIR. Since src/emacs.zig includes emacs-module.h, this build is likely to be environment-dependent and can fail on fresh runners. Consider installing an Emacs dev package (or using purcell/setup-emacs here and exporting the include dir) so the job is reproducible.

Suggested change

	- name: Install Emacs headers
	run: |
	if [ "${RUNNER_OS}" = "Linux" ]; then
	sudo apt-get update
	sudo apt-get install -y emacs-nox
	echo "EMACS_INCLUDE_DIR=/usr/include" >> "$GITHUB_ENV"
	elif [ "${RUNNER_OS}" = "macOS" ]; then
	brew install emacs
	EMACS_PREFIX="$(brew --prefix emacs)"
	echo "EMACS_INCLUDE_DIR=${EMACS_PREFIX}/include" >> "$GITHUB_ENV"
	else
	echo "Unsupported OS for installing Emacs headers: ${RUNNER_OS}" >&2
	exit 1
	fi

[dakra]

Fixed — CI now uses purcell/setup-emacs and a targeted header search instead of raw apt/brew installs.

---

Generated by Claude Code

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In these error paths, the allocated term (and its ghostty resources) is leaked: after Terminal.init succeeds, returning env.nil() from any of these catch blocks skips term.deinit() / destroy(term). Consider errdefer term.deinit(); errdefer std.heap.c_allocator.destroy(term); after init (or manually cleanup in each catch) before signaling the Emacs error.

Suggested change

	term.setUserdata(term) catch {
	env.signalError("ghostel: failed to set terminal userdata");
	return env.nil();
	};
	term.setWritePty(&writePtyCallback) catch {
	env.signalError("ghostel: failed to set write_pty callback");
	return env.nil();
	};
	term.setBell(&bellCallback) catch {
	env.signalError("ghostel: failed to set bell callback");
	return env.nil();
	};
	term.setTitleChanged(&titleChangedCallback) catch {
	term.setUserdata(term) catch {
	term.deinit();
	std.heap.c_allocator.destroy(term);
	env.signalError("ghostel: failed to set terminal userdata");
	return env.nil();
	};
	term.setWritePty(&writePtyCallback) catch {
	term.deinit();
	std.heap.c_allocator.destroy(term);
	env.signalError("ghostel: failed to set write_pty callback");
	return env.nil();
	};
	term.setBell(&bellCallback) catch {
	term.deinit();
	std.heap.c_allocator.destroy(term);
	env.signalError("ghostel: failed to set bell callback");
	return env.nil();
	};
	term.setTitleChanged(&titleChangedCallback) catch {
	term.deinit();
	std.heap.c_allocator.destroy(term);

[dakra]

Good catch. Fixed — the callback setup now uses a labeled block pattern. If any setter fails, term.deinit() and destroy(term) are called before signaling the error.

---

Generated by Claude Code

[Copilot]

ghostel--package-version uses package-alist without checking it’s bound. When ghostel is loaded outside package.el (e.g., via -L . / straight clone), this can raise void-variable package-alist and make auto-download always fail noisily. Guard with (boundp 'package-alist) (and/or (require 'package)), returning nil when unavailable.

Suggested change

	"Return ghostel package version string, or nil."
	(let ((pkg (and (fboundp 'package-desc-version)
	(car (alist-get 'ghostel package-alist)))))
	(when pkg
	(package-version-join (package-desc-version pkg)))))
	"Return ghostel package version string, or nil.
	If `package.el' is not available or `package-alist' is unbound,
	return nil without signaling an error."
	(when (and (require 'package nil t)
	(boundp 'package-alist))
	(let ((pkg (car (alist-get 'ghostel package-alist))))
	(when pkg
	(package-version-join (package-desc-version pkg))))))

[dakra]

Fixed — now guarded with (require 'package nil t) and (boundp 'package-alist), returning nil when package.el is unavailable.

---

Generated by Claude Code

[Copilot]

Auto-downloading and then module-loading a native binary lacks any integrity/authenticity checks. At minimum, consider enforcing an https:// URL (reject/ignore non-HTTPS if ghostel-github-release-url is customized) and/or verifying a checksum/signature from the release before writing and loading the file.

[dakra]

Addressed with two mitigations:

1. Auto-download is opt-in (ghostel-auto-download-module defaults to nil)
2. HTTPS is now enforced — the download function rejects non-HTTPS URLs before fetching

Checksum/signature verification would be a good future addition but is out of scope for this PR since there are no releases with published checksums yet.

---

Generated by Claude Code

[Copilot]

This step does find / -name emacs-module.h, which can be very slow on GitHub-hosted runners and may hit permission-denied noise/timeouts. Consider limiting the search to likely prefixes (e.g. $(dirname $(which emacs))/../include, /usr/include, /usr/local/include, Homebrew prefix on macOS), or use an Emacs/brew query to derive the include dir directly.

Suggested change

	HEADER=$(find / -name emacs-module.h -print -quit 2>/dev/null || true)
	if [ -z "$HEADER" ]; then
	echo "::error::emacs-module.h not found"
	exit 1
	fi
	set -e
	# Derive candidate include directories from the Emacs binary and common prefixes.
	EMACS_BIN="$(command -v emacs || true)"
	EMACS_BIN_DIR=""
	if [ -n "$EMACS_BIN" ]; then
	EMACS_BIN_DIR="$(dirname "$EMACS_BIN")"
	fi
	CANDIDATES=""
	if [ -n "$EMACS_BIN_DIR" ]; then
	CANDIDATES="$CANDIDATES \"$EMACS_BIN_DIR/../include\""
	CANDIDATES="$CANDIDATES \"$EMACS_BIN_DIR/../share/emacs/include\""
	fi
	# Common system include directories on Linux and macOS.
	CANDIDATES="$CANDIDATES /usr/include /usr/local/include /opt/homebrew/include /opt/local/include"
	HEADER=""
	for dir in $CANDIDATES; do
	# Skip non-existent directories.
	if [ ! -d "$dir" ]; then
	continue
	fi
	# Try direct path first.
	if [ -f "$dir/emacs-module.h" ]; then
	HEADER="$dir/emacs-module.h"
	break
	fi
	# Fall back to a depth-limited search within the candidate directory.
	FOUND="$(find "$dir" -maxdepth 5 -name emacs-module.h -print -quit 2>/dev/null || true)"
	if [ -n "$FOUND" ]; then
	HEADER="$FOUND"
	break
	fi
	done
	if [ -z "$HEADER" ]; then
	echo "::error::emacs-module.h not found"
	exit 1
	fi

[dakra]

Fixed — replaced find / with a targeted search that checks directories relative to the Emacs binary (including Nix store symlinks), then falls back to common system prefixes with maxdepth 5.

---

Generated by Claude Code

[Copilot]

CI pins Zig to 0.15.1, but the PR description (and ghostel.el commentary) mentions Zig 0.14+. Please align the version/documentation: either set the workflow to the intended 0.14.x baseline, or update the PR description/docs and remove/update the inline comments about 0.15.2 being "not yet released" to avoid stale guidance.

[dakra]

Fixed — updated the CI comment to clarify: ghostel itself needs Zig 0.14+, the vendored ghostty submodule needs Zig >= 0.15.2, and CI uses 0.15.1 with zig build check (compile-only, no linking) as a compromise until 0.15.2+ is released.

---

Generated by Claude Code