feat(hooks): report every launched harness's lifecycle back to Medulla, and give hooks a page - #192
Conversation
…a, and give hooks a page Co-authored-by: Medulla <medulla@tinyhumans.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMedulla now installs built-in lifecycle reporting hooks, injects scoped hook-only grants into spawned harnesses, records sanitized reports through the control socket, persists hook settings, and adds TUI editing and reporting views. ChangesLifecycle hook reporting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c9b95cfe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/sdk/src/harness_hooks/README.md # src/tui/src/app_loop.rs # src/tui/src/event_loop/mod.rs # src/tui/src/event_loop/types.rs # src/tui/src/ui/app/commands/dispatch.rs # src/tui/src/ui/app/mod.rs # src/tui/src/ui/app/state.rs # src/tui/src/ui/app/types.rs
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 074b5cb4eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eport with The hook shim read MEDULLA_MCP_SOCKET/GRANT, which only ever reach the MCP subprocess's own secret_env file, never the harness's own environment the hook command inherits — so every built-in reporting hook installed by default was inert on the normal PTY/CLI launch path (chatgpt-codex-connector P1 on PR #192). Mint a distinct hook-only grant per launch (Grant::hook_only, MEDULLA_HOOK_SOCKET/GRANT), written straight into the harness's own environment: the server's dispatch refuses every op but hook.report for it, so leaking it to a subprocess the harness spawns can do nothing but attribute a fabricated report to its own session. Minted unconditionally in attach_mcp regardless of provider, override, or whether the fleet grant was withheld, since hooks apply to every provider and carry no fleet capability to protect. Co-authored-by: Medulla <medulla@tinyhumans.ai>
persist_hooks went through read_document/write_document directly, which always parse and render TOML, so saving from the Hooks page against a JSON config (medulla.tui.json, or an extensionless config the loader treats as JSON) failed to parse the operator's document and lost the edit (chatgpt-codex-connector P2 on PR #192). Route it through persist_root_setting (now JSON-aware, like persist_setting/persist_section already are) and a new clear_root_setting for the empty-hooks-list case, backed by shared read_json_document/write_json_document helpers. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3690a16867
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Hooks page saved through the app's general config_path, which app_loop resolves to the highest-precedence discovered layer — the project-local .medulla/config.toml or medulla.toml when one exists. load_config strips [[hooks]] from exactly that layer on every non-explicit load (project configuration must not authorize shell commands in the operator's environment), so a hook saved there showed "Hook saved" and applied for the rest of the session while the file Medulla reads on the next launch never carried it (chatgpt-codex-connector P2 on PR #192). Give hook persistence its own App::hooks_config_path, defaulted to config_path by set_config_path and overridden by app_loop to the user-global config (or an explicit --config file, which load_config never strips) whenever the two must differ. persist_hooks now targets it and tells the operator when the save landed somewhere other than the project's own config. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47708f0d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…paths correctly Two built-in-hook correctness bugs (chatgpt-codex-connector P2s on PR #192): - Every built-in serialized the same 5-second timeout regardless of event, but Codex's hook config refuses a SessionEnd handler whose declared timeout exceeds 3 seconds — so a default Codex launch got an unsupported hook definition for the one event it must report on exit. SessionEnd now declares 3s, matching the shim's own internal report budget. - The built-in's command quoted the launching binary's path with POSIX single-quote escaping unconditionally, which is wrong on a native Windows launch where the command runs through cmd.exe/ PowerShell rather than a POSIX shell. shell_quote now quotes for cfg!(windows) (double quotes, doubled embedded quotes), via a platform-parameterized shell_quote_for so both rules are covered by tests regardless of which CI runner executes them. Does not attempt Codex's separate commandWindows override or Claude's Git-Bash-vs-PowerShell distinction for arbitrary operator hook text — this only ever quotes the running binary's own path, and going further needs a real Windows harness to verify against. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1039e08b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Saving or toggling a hook updated loaded.config.hooks, but LocalSessions.hooks — the copy harness_pane::spawn::open_unmanaged actually reads when the operator starts a harness from the Agents pane — was a separate clone taken once at startup and never touched again. The status line claimed "applies to harnesses started from now on" while a session opened before a restart kept launching with the stale set: a newly added hook would not run, and a removed one would keep running (chatgpt-codex-connector P2 on PR #192). Added App::sync_local_sessions_hooks, called from both persist_hooks and toggle_builtin_hooks, so the live launch state always matches what was just saved. The fleet-dispatched task path (EmbeddedDaemonOptions.hooks, baked in at host start) has the same staleness class but needs a materially larger change — threading a shared, mutable hook config through daemon/embedded and daemon/providers across ~24 existing construction sites — so it is called out as a known follow-up rather than folded into this fix. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The shim's 3-second report budget only wrapped hook.report itself; read_payload had already been allowed up to 500ms before it, so the worst case (~3.5s) exceeded Codex's documented 3-second SessionEnd cap — the harness would kill the shim outright instead of it dropping the report quietly (chatgpt-codex-connector P2 on PR #192). One deadline now covers the whole run: declared_timeout mirrors harness_hooks::builtin's per-event value (3s for SessionEnd, 5s otherwise, minus a fixed headroom), read_payload's own budget shrinks to whatever is left of it, and the report is given whatever remains after that — so the total can never exceed what the harness declared, for any event. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
# Conflicts: # src/sdk/src/mcp/mod.rs
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b093379a0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… built-ins Resolving the built-in reporting hooks at config load (85706f3 / this PR's own dd27c79) put the medulla hook <Event> command in every loaded config, but only the pty door (worker::pty::launch::attach_mcp) seeded the MEDULLA_HOOK_SOCKET/MEDULLA_HOOK_GRANT environment that command needs to find anything to report to. The headless one-shot executor (daemon::providers::execute), the interactive session (sessions::interactive), and the medulla <cli> wrapper installed the same hooks and none of them seeded the grant, so every built-in on those doors spawned, found nothing to report to, and exited — a wasted process per firing, and PostToolUse fires once per tool call (chatgpt-codex-connector P2 on PR #192, filed against dd27c79). Chose to plumb the credential into every door rather than gate the built-ins to the one door that already had it: control_socket::active() is the same process-wide handle the ACP spawn path already reads without a registry threaded through it, so every non-ACP door can mint a hook-only grant the same way local_hook_grant already does for pty, at the cost of one seam per door rather than a config change that would make the Hooks page lie about which harnesses actually report. Added harness_hooks::seed_hook_grant (in a new grant.rs, gated behind the workflows feature the same way attach_mcp already is), a HookGrantGuard that revokes the grant when it drops so a short-lived spawn does not leave a live token in the registry, and wired it into execute.rs's headless run, wrapper::run's child spawn (reusing the already-unique wrapper_session_id), and InteractiveSession (stored on the struct so it outlives every turn, not just open()). ACP dispatch remains unfixed by design: Medulla spawns an ACP server that spawns the harness itself, so there is no argv to install either the command or now the grant onto — already reported as a warning rather than silently doing nothing. Updated harness_hooks/README.md, builtin.rs's module docs, and the 'resolved at load' comment in config/load.rs so all three now say a door needs both the installed command and a seeded grant, and that ACP is the one door that gets neither. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc7e16475
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Add an accessor for the resolved `[[hooks]]` configuration so that a one-shot embedded daemon started elsewhere for the same session can install the same built-in and operator hooks, matching the behavior already provided for custom harnesses. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The command dispatcher now retrieves hooks from the local host spawner and forwards them to the workflow spawn function, enabling hooks to be available during workflow execution. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The workflow runner now receives the hooks configuration so that custom harness hooks can be applied during workflow execution. This enables the TUI to support the same hook functionality as the CLI when running workflows. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The workflow runner now receives the hooks configuration so it can apply harness hooks during workflow execution. This enables custom hooks to be triggered as part of the workflow lifecycle. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The one-shot daemon now receives the same lifecycle hooks as the session's primary host, matching the existing behavior for custom harnesses. This ensures the daemon does not start without the built-in and operator hooks it needs. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e924db66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Hook payloads can contain control characters that would be rendered directly into the operator's terminal, so the summary is now filtered to strip all control bytes before it becomes part of the report. This prevents terminal escape sequences from harness-supplied text from being interpreted as commands. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The doc comment on the `sanitize` function now points to the fully qualified `medulla::harness_hooks::HookReport::summary` type instead of the previously ambiguous local reference, clarifying where the sanitized text is used. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add tests asserting that control bytes in notification messages and tool names are stripped from the generated summaries, guarding against terminal escape sequences leaking through to the operator. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11e32e5430
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The hook report summary is now passed through the sanitizer before being truncated, ensuring that any control characters or unsafe content in the summary field are removed prior to storage. This prevents malformed or potentially harmful data from being persisted in the report. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Strip control characters from caller-supplied hook report fields before recording them, since the hook shim is not the only path that can invoke `hook.report` — any subprocess with the grant can call it directly. This prevents terminal escape sequences from reaching the operator's terminal when summaries are rendered via `Span::raw`. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds tests for the control socket server's hook handling, covering the expected behavior of hook registration and invocation. This ensures the hook mechanism works correctly and guards against regressions. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test now verifies that the recorded summary is sanitized by checking it contains no control characters and matches the expected cleaned value, replacing the previous placeholder shadow. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Added a test module declaration for the hooks file, gated behind the test configuration, so the existing hooks tests are compiled and run as part of the test suite. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test module now imports `RunError`, `TaskOutcome`, and `TaskRequest` from the `hub` module instead of the `types` module, reflecting the relocation of these types. This keeps the test imports aligned with the current type definitions. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted the summary assertion in the hook recording test to use a multi-line expression, improving code readability without changing the test's behavior. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d894c2b008
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
What this fixes
[[hooks]]injection worked, but Medulla shipped zero hooks of its own, so aClaude Code or Codex session Medulla launched carried nothing unless the operator
had hand-written a
[[hooks]]section — which is why/hooksin a Medulla-spawnedsession showed nothing at all. A harness on a pty was opaque: "is this waiting for
me?" and "did that turn finish?" were answered by pattern-matching its screen.
What is here
Medulla's own reporting hooks, on by default (
src/sdk/src/harness_hooks/builtin.rs).Resolved at config load, so every spawn door — the pty pane, the executor, the
wrapper, the daemon — installs them without knowing they exist. Each runs
medulla hook <Event>; the shim reads the harness's native payload on stdin andfiles a one-line summary on the control socket that spawn was already handed,
authenticating with that session's grant.
Two properties are deliberate:
PreToolUse, noPermissionRequest— both can deny acall, and a hook Medulla installs everywhere must not change what a session does.
the harness's process tree; the shim summarizes at the source (
used Edit,prompt submitted (412 chars)).Switched off with
[hookDefaults] enabled = false, orbon the new page. Neverwritten to the operator's config file, so a later release can change or withdraw them.
A Hooks page under Hosts (
Hosts → 3 Hooks). Medulla as hook aggregator: oneplace to declare a hook for every harness it launches. Shows what a harness started
right now would carry, marks Medulla's own separately from the operator's, summarizes
per-harness coverage gaps (OpenCode has no declarative hook API; Codex needs one
/hookstrust), and renders the reports arriving live underneath.aadd,eedit,dremove,btoggle Medulla's own — the operator's half round-trips through[[hooks]]in their config file.Validation
cargo test— all suites pass, including a newe2e_hook_shimthat runs the realmedulla hookbinary against a real control socket and asserts the report lands inthe log (and that the tool input does not).
cargo clippy --all-targets -- -D warnings,cargo fmt --check.launch_argsproducesagainst a stand-in control plane:
SessionStart,UserPromptSubmit,PostToolUse(used Bash),Stop,SessionEndall reported, with no payload inany of them.
Codex remains inert until the operator trusts the hooks once (
/hooks) — Medullastill refuses
--dangerously-bypass-hook-trust, and the page says so.Summary by CodeRabbit
medulla hookcommand for lifecycle event reporting with protected, summarized payloads.