Skip to content

feat(hooks): support multiple client-scoped commands - #758

Open
1jehuang wants to merge 1 commit into
masterfrom
fix/herdr-client-hooks
Open

feat(hooks): support multiple client-scoped commands#758
1jehuang wants to merge 1 commit into
masterfrom
fix/herdr-client-hooks

Conversation

@1jehuang

@1jehuang 1jehuang commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • accept either a string or an array for Jcode lifecycle hooks while preserving string compatibility
  • execute each configured hook directly and independently
  • propagate the initiating client's terminal environment into hook processes, including shared-server sessions
  • clear stale terminal environment state when no client context exists

This enables Herdr to append its lifecycle observer without wrapping or replacing an existing user hook. It also lets multiple Jcode clients sharing one server report the correct pane-local identity.

Closes #759.

Validation

  • focused Jcode hook suite: 11 passed
  • concurrent two-client shared-server environment isolation test passed
  • direct multiple-hook execution test passed
  • cargo check -p jcode-app-core passed
  • validated live with two Jcode panes in Herdr on one shared socket, each mapped to a distinct native session ID

Downstream

Required by herdrdev/herdr#2248 for native lifecycle hook composition and correct multi-pane routing.

@1jehuang
1jehuang force-pushed the fix/herdr-client-hooks branch from 577053c to 3c57514 Compare August 3, 2026 23:19
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds native support for multiple lifecycle hook commands and client-scoped terminal identity. The main changes are:

  • Hook config now accepts either a single command string or an ordered command array.
  • Env overrides can disable hooks, preserve legacy string behavior, or provide multiple commands.
  • Observer hooks and pre_tool gates now execute each configured command independently.
  • Client terminal environment snapshots are applied to hook processes during shared-server sessions.

Confidence Score: 5/5

Safe to merge with low risk.

The changes preserve legacy hook behavior, add ordered multi-command execution, and include focused tests for config parsing and hook execution.

Files Needing Attention: No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P1 finding and linked it to the corresponding review comment.
  • T-Rex ran cargo test -p jcode-app-core client_lifecycle from /home/user/repo and captured the full log.
  • Two tests in server::client_lifecycle::tests failed: cancel_without_local_task_still_signals_session_control and deferred_cancel_reset_does_not_erase_newer_cancel.
  • A log artifact containing the full test run was uploaded to support validation of the failing tests.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
crates/jcode-app-core/src/server/client_lifecycle.rs Propagates the active client terminal environment through session creation/resume, message processing, clear-session, and cleanup hook execution.
crates/jcode-base/src/hooks.rs Adds task-local client terminal env propagation and executes every configured observer/pre-tool hook independently in declaration order.
crates/jcode-config-types/src/lib.rs Implements backward-compatible hook config deserialization/serialization for either a string or array of command strings.
crates/jcode-terminal-launch/src/lib.rs Adds helper to replace inherited terminal-identifying environment variables with a client-authoritative snapshot and aliases.
crates/jcode-base/src/config/env_overrides.rs Extends lifecycle hook env overrides to accept empty disables, legacy strings, or TOML-style arrays of command strings.
crates/jcode-base/src/config_tests.rs Adds config tests for hook command arrays, legacy first-command compatibility, serialization round trips, and env array overrides.
crates/jcode-app-core/src/server/client_lifecycle_tests.rs Updates lifecycle tests for the new terminal environment parameter on message processing helpers.
crates/jcode-base/src/terminal_launch.rs Re-exports terminal environment application support from the terminal launch crate.

Sequence Diagram

sequenceDiagram
participant Client
participant Server as handle_client
participant Hooks as hooks task-local
participant Config as HooksConfig
participant Proc as Hook processes

Client->>Server: Subscribe(terminal_env)
Server->>Server: store active_terminal_env
Server->>Hooks: with_client_terminal_env(active_terminal_env)
Server->>Config: hook_commands(event)
Config-->>Server: command list
loop each configured command
    Server->>Proc: build process with client terminal env + hook env
    alt observer hook
        Server->>Proc: spawn detached
    else pre_tool gate
        Server->>Proc: wait for exit status
        Proc-->>Server: allow/block/other
    end
end
Loading

Comments Outside Diff (1)

  1. General comment

    P1 Focused client lifecycle Rust test suite fails

    • Bug
      • cargo test -p jcode-app-core client_lifecycle fails with 2 failing tests out of 19 selected tests. Both failures assert that stop_signal.is_set() should be true but it is not.
    • Cause
      • The client lifecycle cancel/session-control behavior under test is not setting the stop signal in the failing scenarios: cancel_without_local_task_still_signals_session_control at crates/jcode-app-core/src/server/client_lifecycle_tests.rs:323 and deferred_cancel_reset_does_not_erase_newer_cancel at line 387.
    • Fix
      • Investigate the cancel/session-control signal path in crates/jcode-app-core/src/server/client_lifecycle.rs and restore the expected behavior so cancellation without a local task and deferred cancel reset handling both leave the relevant stop signal set as the tests expect.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "feat(hooks): support multiple client-sco..." | Re-trigger Greptile

@1jehuang

1jehuang commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

CI note: the remaining failing all-target/build jobs reproduce on the unchanged base commit 156ae4092.

Both fail in the pre-existing CLI confidence migration (src/cli/commands/menubar.rs and src/cli/commands_tests.rs). This PR changes only eight hook/config/lifecycle files and is reported mergeable by GitHub. Its formatting, linked-issue, PowerShell, installer, TypeScript SDK, release automation, and Windows cross-target checks pass.

The integration-specific local suite and live two-pane Herdr validation are documented in the PR body.

@factnest365-ops

Copy link
Copy Markdown

Independent verification of this PR (2026-08-04), run against a fresh clone on macOS aarch64 with cargo 1.96.0.

Verdict: no PR-introduced test failures. Every failing test in the crates this PR touches fails identically on the base commit (156ae4092, v0.67.0).

Test matrix (head 3c5751474 vs base 156ae4092):

Crate / suite Head Base
jcode-app-core client_lifecycle (19 tests) 17 pass, 2 fail 17 pass, 2 fail — identical tests, identical lines (323 / 387), identical stop_signal.is_set() assertion
jcode-base hooks 11/11 pass 8/8 pass — the PR adds 3 new hook tests, all pass
jcode-config-types (14 tests) 14/14 pass 14/14 pass
jcode-terminal-launch (22 tests) 19 pass, 3 fail 19 pass, 3 fail — identical 3 tests

The two failures flagged by automated review (cancel_without_local_task_still_signals_session_control, deferred_cancel_reset_does_not_erase_newer_cancel) are pre-existing on master, not introduced here. The three terminal-launch failures are also pre-existing and environment-sensitive (ghostty detection resolves to the host terminal; the two PoisonErrors are a cascade from the first panic on a shared lock).

Separate note for maintainers: the remaining red CI (Build & Test + Quality Guardrails on all three OSes) comes from a pre-existing src/cli/commands/menubar.rs / src/cli/commands_tests.rs compile mismatch that reproduces on the base commit too. It is unrelated to this PR's eight files. Once that is fixed on master, this PR should show green.

This PR is merge-ready from a test standpoint; the pre-existing failures should be tracked separately.

@factnest365-ops

Copy link
Copy Markdown

Addendum — root causes of the 5 pre-existing test failures (follow-up to the verification comment above; same 2026-08-04 run, head 3c5751474).

The two jcode-app-core failures are a stale test contract, not a runtime cancel bug:

  • Both tests build a bare SessionControlHandle::cancel_only with an empty turn_cancel_registry and call cancel_processing_message with task: None.
  • Since commit 9ee29b66 ("fix(interrupt): make an idle cancel a true no-op instead of arming the next turn"), that path deliberately does not arm the stop signal (has_active_turn false → IDLE_NOOP, client_lifecycle.rs:3073) — arming it would only kill the next turn during the 500ms deferred-reset window.
  • The failing assertions (lines 323/387) date from the app-core crate extraction (4dd91a9c, 2026-05-29) and predate that fix.
  • Fix (15 min, separate PR): register an active turn so the tests exercise the "cancel a turn owned elsewhere" path, or assert the noop contract — signal NOT set, Interrupted + Done events still emitted.

The three jcode-terminal-launch failures are environmental + cascade:

  • detected_resume_terminal_recognizes_ghostty_env panics at lib.rs:906 with left: Some("herdr"), right: Some("ghostty") — this host runs inside herdr, and herdr context is preferred over outer-emulator detection by design. Test assumes a non-herdr host.
  • The panic happens while holding the shared ENV_LOCK: Mutex<()> (lib.rs:825), poisoning it; the other two tests then fail at their .lock().unwrap() (lines 875/989) with PoisonError. Both pass in isolation.

None of the five touches this PR's eight files.

@factnest365-ops

Copy link
Copy Markdown

Ready-to-merge fix for the master CI break (issue #768) — a one-command PR request.

The menubar compile fix is complete, verified, and pushed to my fork. It's currently blocked from PR creation only because my account (factnest365-ops) is pull-only on 1jehuang/jcode — GitHub returns FORBIDDEN on CreatePullRequest. Requesting the PR be opened from a write-capable account.

Branch: factnest365-ops:fix/menubar-ci (commit 93c5cc9)
Baseline: 156ae4092 (v0.67.0) — pre-existing compile break, exactly the 7 errors documented in #768
Verification: cargo check --all-targets passes with the fix

One command to open it (run as any maintainer):

gh pr create --repo 1jehuang/jcode --base master --head factnest365-ops:fix/menubar-ci \
  --title "fix(cli): correct menubar command type mismatches on master" \
  --body "Fixes the pre-existing master compile break (#768). Replaces legacy ConfidenceState numeric scores with the current enum construction in menubar.rs and commands_tests.rs. cargo check --all-targets verified. Independent of PR #758; unblocks its CI."

This unblocks PR #758's red Build & Test and lets the jcode-herdr integration ship.

@factnest365-ops

Copy link
Copy Markdown

Done on our side. Independent verification (fresh clone, macOS aarch64, cargo 1.96.0) confirms this PR introduces zero new test failures — every failure in the touched crates reproduces identically on the base commit (v0.67.0). Ready for merge whenever you are. Note: the separate pre-existing master compile break (#768) has a verified fix ready at factnest365-ops:fix/menubar-ci (one-command PR open posted above) that will turn this PR's CI green once landed.

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.

Support composable client-scoped lifecycle hooks

2 participants