feat: add jcode integration - #2248
Conversation
📝 WalkthroughWalkthroughThe pull request adds Jcode as a supported agent and integration. It adds terminal detection, session-start reporting, installation and uninstall flows, native session resume, configuration support, CLI targets, schemas, tests, and documentation. ChangesJcode integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Jcode
participant HerdrHook
participant HerdrSocket
Jcode->>HerdrHook: Invoke session_start hook
HerdrHook->>HerdrHook: Derive session metadata
HerdrHook->>HerdrSocket: Send pane.report_agent_session
HerdrHook->>Jcode: Forward the previous hook
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @1jehuang, thanks for your interest in contributing! Herdr automatically admits focused bug fixes from contributors who are not maintainers when the title uses Contributors who are not maintainers may submit only focused bug fixes. If this pull request fixes a bug, rename it to use a conventional Feature requests, behavior changes, and other proposals belong in GitHub Discussions and require maintainer approval before a pull request. If this gate classified the pull request incorrectly, reply and tag a maintainer listed in Patch size: 26 changed files, 756 changed lines. See https://github.com/herdrdev/herdr/blob/master/CONTRIBUTING.md for the contribution policy. |
|
Hi @ogulcancelik, the intake gate closed this approved feature PR after the follow-up schema commit. Could you reopen it to grant the PR-specific scope override? This is the Jcode integration you invited in Discussion #1848: #1848 (reply in thread) |
kangal-bot
added
ai-review
Greptile SummaryThe PR adds end-to-end Jcode support for screen-based state detection, native session reporting and restoration, and integration lifecycle management.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the two previously reported observer-forwarding issues are eliminated by registering Herdr and user observers as independent native Jcode hook-array entries.
|
| Filename | Overview |
|---|---|
| src/integration/assets/jcode/herdr-agent-state.sh | Adds a session-only socket reporter whose optional Python dependency does not gate independently registered user hooks. |
| src/integration/config_edit.rs | Adds TOML-preserving helpers that normalize, append, and remove only Herdr's Jcode hook-array entry. |
| src/integration/targets.rs | Adds validated, idempotent Jcode installation and targeted uninstallation without creating managed state before config validation. |
| src/detect/manifests/jcode.toml | Defines screen evidence for Jcode composer-ready and processing states. |
| tests/cli/jcode_lifecycle.rs | Exercises Jcode integration lifecycle transitions and failure recovery. |
| tests/cli/hooks.rs | Extends the reusable hook conformance harness for independent Jcode hook-array execution. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Install["herdr integration install jcode"] --> Validate["Parse and validate config.toml"]
Validate --> Hook["Write executable Herdr hook"]
Validate --> Config["Append independent session_start entry"]
Jcode["Jcode session_start"] --> UserHooks["Existing user hooks"]
Jcode --> Hook
Hook --> Socket["pane.report_agent_session"]
Socket --> Restore["Persist native session identity"]
Restore --> Resume["jcode --resume <id>"]
Screen["Jcode terminal output"] --> Manifest["Jcode screen manifest"]
Manifest --> State["Working / idle state"]
Reviews (5): Last reviewed commit: "test: model jcode integration lifecycle ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/agent_resume.rs (1)
444-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend Jcode coverage to the negative and adversarial cases.
Add Jcode to
planner_rejects_path_refs_for_id_only_agentsandids_are_data_not_shell_text. The current test covers only the happy-path argument vector.Source: Coding guidelines
src/integration/config_edit.rs (1)
814-825: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider rejecting a non-string
session_startvalue.
jcode_session_start_commandreturnsOk(None)whenhooks.session_startexists but is not a string.install_jcodethen treats the slot as empty, skips the backup, and overwrites the value. The parse path and the table path both return descriptive errors, so an explicit error here would keep the behavior consistent and prevent silent loss of a user value.♻️ Proposed change
pub(crate) fn jcode_session_start_command( content: &str, config_path: &Path, ) -> io::Result<Option<String>> { let document = parse_jcode_config(content, config_path)?; - Ok(document + let Some(entry) = document .get("hooks") .and_then(Item::as_table_like) .and_then(|hooks| hooks.get("session_start")) - .and_then(Item::as_str) - .map(str::to_string)) + else { + return Ok(None); + }; + match entry.as_str() { + Some(command) => Ok(Some(command.to_string())), + None => Err(io::Error::other(format!( + "jcode hooks.session_start at {} must be a string", + config_path.display() + ))), + } }src/integration/registry.rs (1)
461-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a short comment for the Jcode downgrade rule.
The Grok branch above explains why a valid hook file with a broken config counts as outdated. The Jcode branch repeats the pattern without that context. One comment line, or a combined condition for both targets, keeps the rule clear.
src/integration/assets/jcode/herdr-agent-state.sh (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit empty
CDPATHassignment.
CDPATH= cdis intentional, but ShellCheck reports SC1007. UseCDPATH='' cd ...to make the assignment explicit.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e05c73e5-123b-4ea1-9a62-7267d241f10d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.tomldocs/next/api/herdr-api.schema.jsondocs/next/website/src/content/docs/agents.mdxdocs/next/website/src/content/docs/integrations.mdxdocs/next/website/src/content/docs/session-state.mdxdocs/next/website/src/data/config-reference.jsonsrc/agent_resume.rssrc/api/schema/integrations.rssrc/cli/integration.rssrc/config/model.rssrc/config/sidebar.rssrc/config/sound.rssrc/detect/manifest.rssrc/detect/manifests/jcode.tomlsrc/detect/mod.rssrc/integration/actions.rssrc/integration/assets/jcode/herdr-agent-state.shsrc/integration/config_edit.rssrc/integration/env.rssrc/integration/mod.rssrc/integration/registry.rssrc/integration/targets.rssrc/integration/tests.rssrc/integration/types.rswebsite/agent-detection/index.tomlwebsite/agent-detection/jcode.toml
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97d851c7-3803-469e-bf8e-8eb79105c4f8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.tomldocs/next/api/herdr-api.schema.jsondocs/next/website/src/content/docs/agents.mdxdocs/next/website/src/content/docs/integrations.mdxdocs/next/website/src/content/docs/session-state.mdxdocs/next/website/src/data/config-reference.jsonsrc/agent_resume.rssrc/api/schema/integrations.rssrc/cli/integration.rssrc/config/model.rssrc/config/sidebar.rssrc/config/sound.rssrc/detect/manifest.rssrc/detect/manifests/jcode.tomlsrc/detect/mod.rssrc/integration/actions.rssrc/integration/assets/jcode/herdr-agent-state.shsrc/integration/config_edit.rssrc/integration/env.rssrc/integration/mod.rssrc/integration/registry.rssrc/integration/targets.rssrc/integration/tests.rssrc/integration/types.rswebsite/agent-detection/index.tomlwebsite/agent-detection/jcode.toml
🚧 Files skipped from review as they are similar to previous changes (25)
- src/config/model.rs
- src/detect/manifest.rs
- website/agent-detection/index.toml
- src/detect/manifests/jcode.toml
- docs/next/api/herdr-api.schema.json
- docs/next/website/src/data/config-reference.json
- Cargo.toml
- docs/next/website/src/content/docs/agents.mdx
- docs/next/website/src/content/docs/session-state.mdx
- src/config/sound.rs
- docs/next/website/src/content/docs/integrations.mdx
- src/cli/integration.rs
- src/integration/actions.rs
- src/api/schema/integrations.rs
- src/integration/env.rs
- src/config/sidebar.rs
- website/agent-detection/jcode.toml
- src/integration/registry.rs
- src/integration/mod.rs
- src/integration/targets.rs
- src/integration/types.rs
- src/integration/tests.rs
- src/integration/config_edit.rs
- src/detect/mod.rs
- src/agent_resume.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/integration/tests.rs (1)
4149-4174: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the socket accept/join and the output-polling window.
Two robustness concerns in this new end-to-end test:
- The polling loop waits at most 1 second (100 × 10ms) for
previous_outputto appear before asserting its contents. Under a loaded CI runner, this window can be too short, causing an intermittent, hard-to-diagnose failure rather than a clear one.- The spawned server thread calls
listener.accept()with no timeout, andserver.join()later waits on it unconditionally. If the hook script fails to connect to the socket for any reason (a regression in the hook, environment issue, etc.), this test hangs indefinitely instead of failing with a clear timeout error.Consider widening the polling budget and adding a bounded wait around the
accept()/join()(for example, a channel-basedrecv_timeoutguard) so a broken hook produces a clear test failure instead of a stalled CI job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61912794-59cf-4a93-a443-d746c899aa95
📒 Files selected for processing (7)
src/agent_resume.rssrc/integration/actions.rssrc/integration/assets/jcode/herdr-agent-state.shsrc/integration/config_edit.rssrc/integration/registry.rssrc/integration/targets.rssrc/integration/tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/integration/config_edit.rs
- src/integration/registry.rs
- src/integration/targets.rs
- src/agent_resume.rs
- src/integration/actions.rs
|
hey jeremy, thanks for the pr. the detection part looks good, but the session restore hook has a problem. jcode runs hooks from its shared server, so they use the server’s original herdr pane id. the first pane may work, but later panes can report their session to the wrong pane. if the server started outside herdr, they report nothing. the current test does not cover this. existing hooks are also run differently after installation: jcode normally runs them directly, but this adapter uses could we fix these in jcode first and test two panes using the same server? native support for multiple hooks would make the herdr integration much safer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cli/hooks.rs (1)
173-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert
params["source"]in the shared contract table.The header comment describes this table as the check for "Herdr's stable session-report contract".
assert_reportintests/cli/jcode_lifecycle.rs(lines 690) assertsparams["source"] == "herdr:jcode", but this table does not checksourceat all. A regression that drops or renames thesourcefield in one adapter would pass here.Add a
sourcefield toCaseand assert it per agent.♻️ Proposed refactor
struct Case<'a> { name: &'a str, invocation: ShellHookInvocation<'a>, agent: &'a str, + source: &'a str, session_id: &'a str, }assert_eq!(request["params"]["agent"], case.agent, "{}", case.name); + assert_eq!(request["params"]["source"], case.source, "{}", case.name);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c49c7833-8dcc-4951-8f30-ad5c5b9f2e81
📒 Files selected for processing (10)
docs/next/website/src/content/docs/integrations.mdxsrc/integration/assets/jcode/herdr-agent-state.shsrc/integration/config_edit.rssrc/integration/mod.rssrc/integration/registry.rssrc/integration/targets.rssrc/integration/tests.rstests/cli/hooks.rstests/cli/jcode_lifecycle.rstests/cli/mod.rs
💤 Files with no reviewable changes (2)
- src/integration/assets/jcode/herdr-agent-state.sh
- src/integration/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/next/website/src/content/docs/integrations.mdx
- src/integration/registry.rs
- src/integration/tests.rs
| pub(super) fn start(expected_requests: usize) -> Self { | ||
| let base = unique_test_dir(); | ||
| fs::create_dir_all(&base).unwrap(); | ||
| let socket_path = base.join("herdr.sock"); | ||
| let listener = UnixListener::bind(&socket_path).unwrap(); | ||
|
|
||
| let server = thread::spawn(move || { | ||
| listener.set_nonblocking(true).unwrap(); | ||
| let deadline = Instant::now() + Duration::from_millis(700); | ||
| let mut requests = Vec::with_capacity(expected_requests); | ||
| while requests.len() < expected_requests && Instant::now() < deadline { | ||
| match listener.accept() { | ||
| Ok((mut stream, _)) => { | ||
| let mut line = String::new(); | ||
| let mut reader = BufReader::new(stream.try_clone().unwrap()); | ||
| reader.read_line(&mut line).unwrap(); | ||
| let _ = stream.write_all(br#"{"id":"test","result":{"type":"ok"}}"#); | ||
| let _ = stream.write_all(b"\n"); | ||
| let _ = stream.flush(); | ||
| requests.push(serde_json::from_str(&line).unwrap()); | ||
| } | ||
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | ||
| thread::sleep(Duration::from_millis(10)); | ||
| } | ||
| Err(err) => panic!("accept failed: {err}"), | ||
| } | ||
| } | ||
| requests | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Raise the collector deadline to remove a timing race.
The deadline starts when start() runs, not when the hooks run. Callers then spawn bash (and, for the Jcode asset, a Python interpreter) sequentially. run_shell_hooks invokes every hook after start(), so the 700 ms budget covers all process startups. On a loaded machine two sequential hook invocations can exceed that budget. The collector then returns fewer requests, and tests/cli/jcode_lifecycle.rs assertions such as assert_eq!(requests.len(), 2, ...) fail intermittently.
The equivalent production-path test in src/integration/tests.rs (lines 4146-4229) uses a 5-second deadline for two hooks.
The loop already exits as soon as expected_requests arrive, so a longer deadline does not slow the positive cases. It does slow the expected-empty cases (run_outside_herdr, run_without_python), which always wait for the full deadline. Consider making the deadline a parameter so those cases keep a short wait.
🐛 Proposed fix
impl FakeHookSocket {
pub(super) fn start(expected_requests: usize) -> Self {
+ Self::start_with_timeout(expected_requests, Duration::from_secs(5))
+ }
+
+ pub(super) fn start_with_timeout(expected_requests: usize, timeout: Duration) -> Self {
let base = unique_test_dir();
fs::create_dir_all(&base).unwrap();
let socket_path = base.join("herdr.sock");
let listener = UnixListener::bind(&socket_path).unwrap();
let server = thread::spawn(move || {
listener.set_nonblocking(true).unwrap();
- let deadline = Instant::now() + Duration::from_millis(700);
+ let deadline = Instant::now() + timeout;Then use start_with_timeout(1, Duration::from_millis(700)) in the tests that expect no report.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(super) fn start(expected_requests: usize) -> Self { | |
| let base = unique_test_dir(); | |
| fs::create_dir_all(&base).unwrap(); | |
| let socket_path = base.join("herdr.sock"); | |
| let listener = UnixListener::bind(&socket_path).unwrap(); | |
| let server = thread::spawn(move || { | |
| listener.set_nonblocking(true).unwrap(); | |
| let deadline = Instant::now() + Duration::from_millis(700); | |
| let mut requests = Vec::with_capacity(expected_requests); | |
| while requests.len() < expected_requests && Instant::now() < deadline { | |
| match listener.accept() { | |
| Ok((mut stream, _)) => { | |
| let mut line = String::new(); | |
| let mut reader = BufReader::new(stream.try_clone().unwrap()); | |
| reader.read_line(&mut line).unwrap(); | |
| let _ = stream.write_all(br#"{"id":"test","result":{"type":"ok"}}"#); | |
| let _ = stream.write_all(b"\n"); | |
| let _ = stream.flush(); | |
| requests.push(serde_json::from_str(&line).unwrap()); | |
| } | |
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | |
| thread::sleep(Duration::from_millis(10)); | |
| } | |
| Err(err) => panic!("accept failed: {err}"), | |
| } | |
| } | |
| requests | |
| }); | |
| pub(super) fn start(expected_requests: usize) -> Self { | |
| Self::start_with_timeout(expected_requests, Duration::from_secs(5)) | |
| } | |
| pub(super) fn start_with_timeout(expected_requests: usize, timeout: Duration) -> Self { | |
| let base = unique_test_dir(); | |
| fs::create_dir_all(&base).unwrap(); | |
| let socket_path = base.join("herdr.sock"); | |
| let listener = UnixListener::bind(&socket_path).unwrap(); | |
| let server = thread::spawn(move || { | |
| listener.set_nonblocking(true).unwrap(); | |
| let deadline = Instant::now() + timeout; | |
| let mut requests = Vec::with_capacity(expected_requests); | |
| while requests.len() < expected_requests && Instant::now() < deadline { | |
| match listener.accept() { | |
| Ok((mut stream, _)) => { | |
| let mut line = String::new(); | |
| let mut reader = BufReader::new(stream.try_clone().unwrap()); | |
| reader.read_line(&mut line).unwrap(); | |
| let _ = stream.write_all(br#"{"id":"test","result":{"type":"ok"}}"#); | |
| let _ = stream.write_all(b"\n"); | |
| let _ = stream.flush(); | |
| requests.push(serde_json::from_str(&line).unwrap()); | |
| } | |
| Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { | |
| thread::sleep(Duration::from_millis(10)); | |
| } | |
| Err(err) => panic!("accept failed: {err}"), | |
| } | |
| } | |
| requests | |
| }); |
3 participants
Summary
jcode --resume <id>Approved in #1848 (reply in thread). Screen evidence and prior reports are in #2018.
Jcode dependency
Requires 1jehuang/jcode#758, which adds backward-compatible lifecycle hook arrays and propagates each initiating client's terminal environment through shared-server hook execution.
Validation
cargo fmt --checkcargo checkpython scripts/agent_detection_manifest_check.pyThe lifecycle graph also found and fixed an installation side effect: malformed Jcode config is now validated before Herdr creates its managed hook directory.
A broader
cargo nextest runpreviously reached an existingapi_pingfailure that reproduces unchanged onorigin/master:new_terminal_cwd_follow_ignores_nonleader_group_member_cwdtimes out waiting for helper-ready on this host. The other initially observed API test passes in isolation.