feat(cli): report active Norn sessions to HerdR - #18
Conversation
|
Warning Review limit reached
Next review available in: 21 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughNorn adds a Unix HerdR pane integration that reports active sessions, attaches optional session metadata, and releases the pane claim after print or TUI execution. ChangesHerdR integration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Norn
participant PaneClaim
participant HerdR
Norn->>PaneClaim: claim(session_id, session_path)
PaneClaim->>HerdR: pane.report_agent
HerdR-->>PaneClaim: response
Norn->>Norn: run print or TUI session
Norn->>PaneClaim: drop claim
PaneClaim->>HerdR: pane.release_agent
HerdR-->>PaneClaim: response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 12dc1d5ac5
ℹ️ 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".
| LAST_SEQUENCE | ||
| .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |previous| { | ||
| Some(wall_clock.max(previous.saturating_add(1))) | ||
| }) | ||
| .unwrap_or_else(|previous| previous) |
There was a problem hiding this comment.
When HerdR is active, fetch_update returns the previous atomic value, not the value produced by the closure, so the first pane.report_agent in every Norn process is sent with seq = 0 while the atomic is updated to the wall-clock value. After a run releases with a large sequence, launching Norn again in the same pane resets this static and reports 0 again, which can make HerdR treat the new claim as stale; return the stored candidate value instead of the old value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. AtomicU64::fetch_update returns the previous value, so the first request could emit seq = 0. Fixed in 2c870b7 with a compare-exchange loop that returns the newly stored monotonic value; added a regression assertion that the first reported sequence is nonzero. Focused tests and strict clippy pass.
| None => data_dir.join(format!("{}.jsonl", entry.id)), | ||
| }) | ||
| }); | ||
| let herdr_claim = crate::herdr::PaneClaim::claim(&session_id, herdr_session_path.as_deref()); |
There was a problem hiding this comment.
In an interactive HerdR pane, this claim is created once from the startup session_id, but the TUI's /new and /clear path creates a fresh persisted store and updates runtime.session_id in crates/norn-tui/src/app/slash.rs:272-278. After that rotation, HerdR continues to show the retired session id/path until exit even though new events are written under the new session; the claim needs to be renewed or updated when the TUI rotates sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1f17c6e. norn-tui now emits SessionChanged after /new or /clear commits rotation, including the new canonical session ID and JSONL path. The CLI-owned HerdR reporter updates its claim while preserving the current idle/working state.
| pane_id: &self.pane_id, | ||
| source: SOURCE, | ||
| agent: AGENT, | ||
| state: "working", |
There was a problem hiding this comment.
Because the TUI creates this claim before entering run_app and holds it until exit, an interactive Norn pane with no initial prompt—or one sitting between turns waiting for user input—is reported to HerdR as working indefinitely. HerdR uses this semantic state for agent rollups and waits, so idle interactive panes will look busy until the process exits; the TUI path needs to report idle outside active turns or update the state on turn boundaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1f17c6e. The TUI initially claims idle, emits TurnStarted/TurnFinished around the centralized root run_turn, and the CLI reporter maps those to HerdR working/idle. A Drop guard emits TurnFinished on error and cancellation paths as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/norn-cli/src/herdr.rs (1)
106-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winErrors are raw
Strings instead ofthiserror-typed errors.
from_env,report,release, andsendall returnResult<_, String>. Per coding guidelines, library code (outside theaion-serverbinary) should usethiserrorfor domain errors rather than ad-hoc strings.As per coding guidelines, "Use
thiserrorfor library domain errors andanyhowonly in theaion-serverbinary for top-level error reporting."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/norn-cli/src/herdr.rs` around lines 106 - 184, Replace the raw String errors in from_env, report, release, and send with a dedicated thiserror-derived domain error type for HerdR operations. Add variants covering environment validation, socket I/O, serialization/deserialization, and rejected responses, then propagate errors with context while preserving the existing messages and Result behavior.Source: Coding guidelines
crates/norn-cli/src/print/orchestrator.rs (1)
356-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated session→JSONL-path derivation for HerdR reporting. Both call sites independently reimplement the same
rel_pathvs.format!("{}.jsonl", entry.id)fallback logic to compute the HerdR report path.
crates/norn-cli/src/print/orchestrator.rs#L356-L374: extract a shared helper (e.g.crate::herdr::session_report_path(entry, data_dir) -> PathBuf) and call it here instead of the inlinematch.crates/norn-cli/src/tui/driver.rs#L192-L200: call the same shared helper instead of duplicating the match againstpersist_data_dir.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/norn-cli/src/print/orchestrator.rs` around lines 356 - 374, The session-to-JSONL path derivation is duplicated across HerdR reporting call sites. Add a shared `crate::herdr::session_report_path(entry, data_dir)` helper encapsulating the `rel_path` versus `{entry.id}.jsonl` fallback, then use it in `crates/norn-cli/src/print/orchestrator.rs` lines 356-374 and `crates/norn-cli/src/tui/driver.rs` lines 192-200; preserve each caller’s existing data-directory error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/norn-cli/src/herdr.rs`:
- Around line 156-184: Update the HerdR request flow around send and its
claim/Drop callers so synchronous UnixStream connect, read, and write operations
cannot block async executor threads indefinitely. Move the complete blocking
send operation into tokio::task::spawn_blocking and apply an explicit timeout to
the spawned operation, or replace it with tokio::net::UnixStream using bounded
connect/read/write operations; preserve the existing error messages and response
validation.
- Around line 187-199: The next_sequence function currently returns the
pre-update value from LAST_SEQUENCE.fetch_update, producing stale duplicate
sequence numbers. Capture the value computed by the update closure and return
that written successor, while preserving the wall-clock and monotonic-increment
logic; update tests to verify distinct emitted sequences rather than relying
only on monotonic assertions.
---
Nitpick comments:
In `@crates/norn-cli/src/herdr.rs`:
- Around line 106-184: Replace the raw String errors in from_env, report,
release, and send with a dedicated thiserror-derived domain error type for HerdR
operations. Add variants covering environment validation, socket I/O,
serialization/deserialization, and rejected responses, then propagate errors
with context while preserving the existing messages and Result behavior.
In `@crates/norn-cli/src/print/orchestrator.rs`:
- Around line 356-374: The session-to-JSONL path derivation is duplicated across
HerdR reporting call sites. Add a shared
`crate::herdr::session_report_path(entry, data_dir)` helper encapsulating the
`rel_path` versus `{entry.id}.jsonl` fallback, then use it in
`crates/norn-cli/src/print/orchestrator.rs` lines 356-374 and
`crates/norn-cli/src/tui/driver.rs` lines 192-200; preserve each caller’s
existing data-directory error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c29870d4-99d3-4918-ac7b-e1de87a57e84
📒 Files selected for processing (4)
crates/norn-cli/src/herdr.rscrates/norn-cli/src/lib.rscrates/norn-cli/src/print/orchestrator.rscrates/norn-cli/src/tui/driver.rs
Pre-merge lifecycle reviewI did a second adversarial pass after the automated reviews. One confirmed protocol bug is now fixed in 2c870b7:
I recommend holding merge until we choose how to handle the TUI-specific findings below, or narrowing this PR to print/driven mode:
Non-blocking cleanup:
My preferred shape is to keep print/driven reporting as-is, then give the TUI a small lifecycle reporter subscribed to root turn/session-rotation events. That preserves authoritative HerdR semantics instead of competing with HerdR’s process detector using a permanently-working claim. |
Deanofish
left a comment
Deanofish
left a comment
There was a problem hiding this comment.
Reviewed at head 2c870b7 with fresh eyes on both sides of the contract — I administer HerdR-managed agents daily, and I cross-checked every wire-level claim against HerdR 0.7.1's CLI contract and (via an independent first-pass) its source. The core design is right: RAII claim/release, per-request connections, best-effort discipline, field-for-field contract match on the request shapes (verified against herdr pane report-agent/release-agent), and correct explicit-drop placement on both wirings. The findings below are why I'm requesting changes rather than approving: three of them break the PR's own two promises — "never affects the Norn run" and "HerdR shows the real session identity" — on reachable paths.
Major
1. The socket exchange is not actually bounded — a wedged HerdR can hang the run.
send() (herdr.rs:156) installs read/write timeouts only after UnixStream::connect, which has no timeout: a listening-but-wedged server with a full accept backlog blocks indefinitely. The read side is also per-recv, not per-exchange — read_line resets the 500 ms clock on every byte received, so a peer trickling bytes without a newline keeps the call (and the unbounded String) alive forever. Both claim() and Drop call this synchronously from async paths, so the block lands on a runtime thread at startup or exit. Fix shape: one absolute deadline across connect+write+read (nonblocking connect + poll, or socket2), plus a max response frame size. This is the one place the PR's "integration failures are observational and never fail the run" invariant can be violated.
2. TUI /new and /clear leave HerdR advertising the retired session.
The claim is constructed once at startup with that session's id and JSONL path (driver.rs:200) and never touched again. Session rotation (norn-tui/src/app/slash.rs — runtime.session_id = Some(new_id)) has no PaneClaim involvement, verified by grep: the two startup claim sites are the only uses. After a rotation, a long-lived TUI process reports the id/path of the conversation the user explicitly left — an Agents-view consumer resuming from that metadata opens the wrong session. Needs a PaneClaim::update(...) (re-report with new identity, same source) invoked at the rotation commit point; print-mode /clear has the same gap.
3. Catchable-signal termination never releases — the pane stays "working" forever.
Cleanup exists only in Drop. There is no SIGINT/SIGTERM handling anywhere in norn-cli/norn-tui (verified repo-wide); print-mode Ctrl-C and any SIGTERM take the OS default path, which runs no destructors. HerdR retains reported authority until released or superseded — no TTL — so an interrupted print/driven run leaves a dead session displayed as norn/working with a stale id and path, and the pane's previous reporter is never restored. Since restoring the previous reporter is one of this PR's stated goals, catchable signals need a bounded release on the way out (SIGKILL is a fair documented limitation).
4. One fixed authority key across processes: a nested Norn releases its parent's claim.
Every process claims and releases as ("herdr:norn", "norn") with no claim identity in ReleaseParams. HerdR keys sequence acceptance by source and checks release by source+agent only — so a nested/overlapping norn --print in the same pane (inherits the env) claims over the parent, and whichever process exits first clears the survivor's authority. Also: sequences are wall-clock-seeded and HerdR silently ignores seq ≤ last-accepted, so after a clock step-back a fresh process's reports can be dropped while claim() still returns Some — claim/release pairing then operates on a claim that never existed. Smallest fix: outermost-invocation marker (env var set on first claim; descendants skip claiming). A per-process source (herdr:norn:<pid>) also works if pane authority semantics allow it.
Minor
5. Any JSON without a top-level error is treated as acceptance. Response id is never matched to the request (the test server replies with id "ok" against requests named herdr:norn:<seq> — and the tests pass, which is itself the demonstration). A wrong-protocol or stale endpoint reads as success. Deserialize the documented envelope, require the matching id and a result.
6. The Coverage section reads as test coverage, but the paths are covered by live probe only. The three committed tests prove request serialization, in-process sequence ordering, and error surfacing — none construct a PaneClaim, exercise env parsing, drive any of the three execution paths, rotate a session, or test timeout/signal/overlap behavior (cargo test -p norn-cli herdr --lib: 3 tests, 523 filtered out). Reword, or better: fake-socket integration tests around each entry path, which would have caught findings 2–4.
Notes, non-blocking
- The contract offers
idle|working|blocked|unknown; the PR pinsworkingfor the entire session lifetime, so an idle TUI reads as working for hours. Fine as v1 scope — worth a follow-up. - Design question raised by Tom, under discussion with norn's regular reviewer (Sable Nightwick): whether the reporting seam should be general (env-configured presence/lifecycle reporting, HerdR as first consumer) rather than a HerdR-named module. The wire protocol is inherently HerdR's, but the seam — when to claim/release/update and what identity to report — could be neutral. Findings 2 and 3 land in that seam regardless of the answer, so the refactor question shouldn't block fixing them.
Provenance: first pass by an independent Norn session (GPT-5.6 Sol, session fa1da96a-3b09-4577-9631-7e305ec61eb5); every load-bearing claim re-verified by hand at the PR head before posting — rotation path, signal-handler absence, and claim-site census checked at the bytes; HerdR-side semantics checked against the 0.7.1 CLI and source.
Lifecycle follow-up implementedThe pre-merge issues from the second review are addressed in 1f17c6e:
Verification after these changes:
All pass. |
|
Addendum: norn-side review from Sable Nightwick (norn's regular reviewer), folded in with permission — my posted review covered the HerdR side of the contract; this covers what only norn internals knowledge could see. Sable independently verified fmt, strict clippy, and the module tests green at head on norn's pinned toolchain. Strengthens finding 2 (rotation staleness) — and it's worse than the TUI: print/driven Confirms finding 4 (nesting) is real, not hypothetical: norn's bash tool spawns Confirms finding 1 (connect timeout) at major severity with norn-side impact: both drivers run multi-thread tokio, so the blocking I/O doesn't starve the runtime — but the claim sits on the run's own critical path (TUI: before the alternate screen; print: before the first provider call), so a listening-but-not-accepting supervisor hangs norn at startup with a blank terminal, or at exit. Fix shapes: socket2 nonblocking connect + poll deadline, or move all pane I/O to a dedicated plain thread with bounded waits (which also fixes the teardown stall where a wedged HerdR delays the terminal error envelope, since the claim drops before New norn-side items:
Design direction for the revision (answers the generalization question raised earlier): extract the seam, skip the trait. A norn-neutral presence/supervisor module owning the lifecycle facts — claim(identity) at driver start, update(identity) on rotation, optionally set_state at turn boundaries, release at teardown — with the HerdR transport as its single backend behind the Sable has offered a narrow re-read once the rotation-update and connect-timeout revisions land; I'll re-verify the HerdR side of the same revision. |
Status correction after human reviewMarking this draft again while the remaining Deanofish findings are addressed. Current head 1f17c6e resolves:
Still unresolved and considered pre-merge work:
The PR should not merge until those are resolved or deliberately scoped out with documented HerdR behavior. |
2 participants
Summary
Report active Norn sessions to the surrounding HerdR pane so they appear in HerdR's Agents view with their real Norn session identity.
When Norn is launched inside a HerdR-managed pane, HerdR provides
HERDR_ENV,HERDR_PANE_ID, andHERDR_SOCKET_PATH. This change uses HerdR's public pane socket contract to:pane.report_agentagent = "norn"andstate = "working"pane.release_agentwhen the TUI or print/driven run endsThe integration is best-effort: missing/invalid HerdR state, socket timeouts, or server rejection are logged and never fail the Norn run. Non-Unix builds retain a no-op implementation.
Why lifecycle reporting instead of detection alone?
HerdR can detect an executable/process name, and that may already make a pane look like it contains Norn. Active lifecycle reporting adds the information process detection cannot reliably infer: the canonical Norn session ID and session-store path. It also lets Norn release authority cleanly when it exits, so a surrounding/previous pane reporter can become authoritative again.
This intentionally covers active pane-backed sessions, not an archive of stopped sessions from
~/.norn/session-store.Coverage
Verification
cargo test -p norn-clicargo fmt --all -- --checkcargo clippy -p norn-cli --all-targets -- -D warningsagent: "norn",agent_status: "working", and the reported session metadata inherdr agent listSummary by CodeRabbit
New Features
Bug Fixes