Add harness-neutral sub-agent thread support - #119
Conversation
Model harness-neutral sub-agent links with immutable parent/child ownership, spawn-turn provenance, nested navigation, readable agent names, running state, and direct child controls. Keep malformed ownership records visible for recovery. Map both Codex collaboration protocols, including legacy lifecycle tools and current activity events. Preserve delegated prompts when available, explicitly track fallback prompts, stream command-only turns immediately, and persist terminal output when native turn events are absent. Resolve transcript links from trusted server-side parent and item records so the browser speaks only Giskard identifiers. Redact native thread IDs from summaries and sub-agent wire payloads, and reject client-asserted ownership or lifecycle metadata on the general thread-open endpoint. Require identity-preserving resume for linked children, with bounded retries for newly advertised Codex rollouts. Keep primary-thread recovery unchanged. Serialize lifecycle mutations per project, bound HTTP lock contention, process linked evidence through a per-parent FIFO, materialize children off the parent forwarder, reuse existing native bindings, and prevent duplicate imports. Coordinate passive monitors and interactive turns through one ownership gate. Use a renewable active idle bound, atomically adopt terminal evidence, persist fallback output once, suppress duplicate turnless broadcasts, and await monitor cancellation through completion signals during deletion. Delete complete child subtrees leaf-first after repeated active-work preflights. Treat only exact missing-rollout deletion responses as idempotent native success. Share forwarder state through RegistryShared, isolate passive pre-turn waiting, use typed lifecycle policies and native-open results, and consolidate duplicated route, prompt, item, and frontend helpers. Recover stale live buffers on genuine new starts without dropping new events. Document both protocols, trusted link opening, navigation, monitoring, prompt, recovery, and deletion behavior in the README, adapter guide, sub-agent guide, and specification.
📝 WalkthroughWalkthroughThis PR introduces harness-neutral sub-agent thread linking across the stack: new ChangesSub-agent thread linking
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/giskard-server/static/app.js (1)
1114-1128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCascade-delete view clearing relies on a possibly stale client cache.
descendants/deletedIdsare computed from the cachedknownProjectThreads(pid)before the request is sent. If a descendant was spawned after the last sidebar refresh, it won't be in this set — if the user is currently viewing that untracked descendant when an ancestor is deleted, the transcript view won't be cleared even though the server actually cascaded-deleted it, leaving a stale/orphaned view.♻️ Suggested fix: verify against the freshly reloaded list instead of the stale pre-computed set
try { await api("DELETE", `/api/projects/${pid}/threads/${tid}`); - const deletedIds = new Set([String(tid), ...descendants]); - if (state.threadId && deletedIds.has(String(state.threadId))) clearThreadView(state.threadId); - await loadThreads(pid); + const priorActiveThread = state.threadId ? String(state.threadId) : null; + await loadThreads(pid); + if (priorActiveThread && !knownThreadForId(pid, priorActiveThread)) clearThreadView(priorActiveThread); } catch (e) {🤖 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/giskard-server/static/app.js` around lines 1114 - 1128, Update deleteThread so the active view is validated against the freshly reloaded thread list after the DELETE completes, rather than relying on the pre-request descendants/deletedIds cache. Reload via loadThreads(pid), then clearThreadView(state.threadId) when the current thread no longer exists in the refreshed results, preserving the existing confirmation and error handling.
🧹 Nitpick comments (2)
crates/giskard-server/src/routes.rs (1)
567-589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the
ThreadKind::Subagentopen dispatch into a shared helper. Theif thread_file.kind == ThreadKind::Subagent { open_linked_thread(...) } else { open_thread(...) }branch is duplicated across four call sites with only argument differences. A single helper (e.g.open_thread_for_kind(®istry, kind, config, ws_root, thread_id, harness_thread_id, model)) would remove the copy/paste and keep the linked-vs-primary resume contract in one place as kinds evolve.
crates/giskard-server/src/routes.rs#L567-L589: replace the inline branch inopen_thread(existing-thread path) with the helper.crates/giskard-server/src/routes.rs#L671-L693: replace the inline branch in the resume-existing path with the helper.crates/giskard-server/src/routes.rs#L3302-L3324: replace the inline branch inensure_thread_openwith the helper.crates/giskard-server/src/routes.rs#L3604-L3626: replace the inline branch inswitch_provider_coldwith the helper.🤖 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/giskard-server/src/routes.rs` around lines 567 - 589, Extract the duplicated ThreadKind::Subagent dispatch into a shared open_thread_for_kind helper that selects open_linked_thread for subagents and open_thread otherwise, preserving the existing arguments and behavior. Replace the inline branches at crates/giskard-server/src/routes.rs lines 567-589, 671-693, 3302-3324, and 3604-3626 with calls to the helper.crates/giskard-server/static/app.js (1)
663-681: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCycles among visible (non-managed) threads silently vanish from the sidebar.
appendThreadRowsonly renders nodes reachable from a "root" (a thread whose parent isn't in the visible set). A genuine cycle among threads that aren't classified as managed sub-agents (e.g. corruptedkind/parent_thread_idcombos) forms a subgraph with no root, so those threads are dropped entirely rather than shown — this contradicts the documented guarantee that malformed/dangling records "remain visible in the main sidebar so they can be repaired or deleted" (docs/subagents.md, spec SA1). Since ownership is meant to be immutable/cycle-free server-side, this is a defense-in-depth gap for corrupted on-disk data rather than a normal-path bug.🤖 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/giskard-server/static/app.js` around lines 663 - 681, Update appendThreadRows so every visible thread is rendered, including nodes in parent-child cycles with no root. Track rendered thread IDs during appendOne, then append any unrendered threads after processing roots while preserving the existing recursive hierarchy behavior and avoiding duplicate rows or infinite recursion.
🤖 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/giskard-server/src/bin/giskard-server-replay.rs`:
- Line 277: Update the conditional in the replay flow around
parent_harness_thread_id to avoid the unstable if-let chain syntax for the Rust
1.85 MSRV. Nest an if let binding inside the existing is_new check, preserving
the current behavior of handling only new entries with a present parent thread
ID.
---
Outside diff comments:
In `@crates/giskard-server/static/app.js`:
- Around line 1114-1128: Update deleteThread so the active view is validated
against the freshly reloaded thread list after the DELETE completes, rather than
relying on the pre-request descendants/deletedIds cache. Reload via
loadThreads(pid), then clearThreadView(state.threadId) when the current thread
no longer exists in the refreshed results, preserving the existing confirmation
and error handling.
---
Nitpick comments:
In `@crates/giskard-server/src/routes.rs`:
- Around line 567-589: Extract the duplicated ThreadKind::Subagent dispatch into
a shared open_thread_for_kind helper that selects open_linked_thread for
subagents and open_thread otherwise, preserving the existing arguments and
behavior. Replace the inline branches at crates/giskard-server/src/routes.rs
lines 567-589, 671-693, 3302-3324, and 3604-3626 with calls to the helper.
In `@crates/giskard-server/static/app.js`:
- Around line 663-681: Update appendThreadRows so every visible thread is
rendered, including nodes in parent-child cycles with no root. Track rendered
thread IDs during appendOne, then append any unrendered threads after processing
roots while preserving the existing recursive hierarchy behavior and avoiding
duplicate rows or infinite recursion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a03ad8d-aff9-49cd-b34e-d3924950d9f0
⛔ Files ignored due to path filters (2)
docs/screenshots/ide-desktop.pngis excluded by!**/*.png,!docs/screenshots/**docs/screenshots/ide-mobile.pngis excluded by!**/*.png,!docs/screenshots/**
📒 Files selected for processing (42)
.dockerignoreREADME.mdcrates/giskard-core/src/item.rscrates/giskard-core/src/lib.rscrates/giskard-core/src/text.rscrates/giskard-core/src/thread.rscrates/giskard-harness-codex/README.mdcrates/giskard-harness-codex/src/lib.rscrates/giskard-harness-codex/src/mapping.rscrates/giskard-harness-replay/src/lib.rscrates/giskard-harness-replay/tests/replay_integration.rscrates/giskard-harness/src/lib.rscrates/giskard-persist/src/store.rscrates/giskard-persist/tests/giskard_admin.rscrates/giskard-proto/src/lib.rscrates/giskard-proto/src/wire.rscrates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/src/hub.rscrates/giskard-server/src/lib.rscrates/giskard-server/src/live_buffer.rscrates/giskard-server/src/registry.rscrates/giskard-server/src/routes.rscrates/giskard-server/src/running_commands.rscrates/giskard-server/src/thread_graph.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/static/index.htmlcrates/giskard-server/tests/e2e_smoke.rscrates/giskard-server/tests/interrupt.rscrates/giskard-server/tests/override_propagation.rscrates/giskard-server/tests/provider_switch.rscrates/giskard-server/tests/read_only_thread.rscrates/giskard-server/tests/running_tasks.rscrates/giskard-server/tests/server_requests.rscrates/giskard-server/tests/thread_lifecycle.rscrates/giskard-server/tests/turn_controls.rscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/README.mdtests/e2e/tests/helpers.tstests/e2e/tests/subagents.spec.ts
| drop(threads); | ||
|
|
||
| let parent_harness_thread_id = Self::subagent_parent(&harness_thread_id); | ||
| if is_new && let Some(parent) = parent_harness_thread_id.clone() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm declared MSRV / edition and existing let-chain usage.
fd -H -t f 'rust-toolchain(.toml)?$' -x cat {}
fd -H -t f '.tool-versions$' -x cat {}
rg -n 'rust-version|edition' -g 'Cargo.toml'
# Any pre-existing if/while let-chains in the crates?
rg -nP '\b(if|while)\b[^\n{]*\b&&\s+let\b' -g 'crates/**/*.rs' -C1 | head -50Repository: marmeladema/Giskard
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== toolchain files =="
fd -HI -t f 'rust-toolchain(.toml)?$|\.tool-versions$' . -x sh -c 'echo "--- {}"; cat "{}"'
echo "== Cargo.toml rust-version/edition =="
rg -n '^(edition|rust-version)\s*=' -g 'Cargo.toml' .
echo "== let-chain usage in crates =="
rg -nP '\b(if|while)\b[^\n{]*\b&&\s+let\b' crates -C 2 || trueRepository: marmeladema/Giskard
Length of output: 320
🌐 Web query:
Rust if let chains stabilized version 1.88 let chains stable
💡 Result:
Yes, let chains were stabilized in Rust 1.88.0 [1][2]. This feature allows you to chain multiple let expressions using the && operator within if and while conditions, and to intermix them with boolean expressions [1][3]. Crucially, let chains are only available in the Rust 2024 edition [4][1]. This limitation exists because the feature relies on edition-specific changes to temporary scope and drop order, which ensure consistent behavior that could not be reliably implemented for older editions [1][5]. If you attempt to use let chains in a project configured for an older edition (e.g., 2021 or 2018), the compiler will emit an error and advise you to upgrade your crate's edition [6][1][5].
Citations:
- 1: https://blog.rust-lang.org/2025/06/26/Rust-1.88.0/
- 2: Tracking issue for release notes of #132833: Stabilize let chains in the 2024 edition rust-lang/rust#139951
- 3: Stabilize let chains in the 2024 edition rust-lang/rust#132833
- 4: https://doc.rust-lang.org/edition-guide/rust-2024/let-chains.html
- 5: https://github.com/rust-lang/rust/pull/132833/files
- 6: Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" rust-lang/rust#53667
Replace the if let-chain here
Cargo.toml pins rust-version = "1.85", but if/while let-chains are stabilized in Rust 1.88. This will not compile on the declared MSRV; nest the binding instead.
🤖 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/giskard-server/src/bin/giskard-server-replay.rs` at line 277, Update
the conditional in the replay flow around parent_harness_thread_id to avoid the
unstable if-let chain syntax for the Rust 1.85 MSRV. Nest an if let binding
inside the existing is_new check, preserving the current behavior of handling
only new entries with a present parent thread ID.
Sources: Coding guidelines, Path instructions
1 participant
Model harness-neutral sub-agent links with immutable parent/child ownership, spawn-turn provenance, nested navigation, readable agent names, running state, and direct child controls. Keep malformed ownership records visible for recovery.
Map both Codex collaboration protocols, including legacy lifecycle tools and current activity events. Preserve delegated prompts when available, explicitly track fallback prompts, stream command-only turns immediately, and persist terminal output when native turn events are absent.
Resolve transcript links from trusted server-side parent and item records so the browser speaks only Giskard identifiers. Redact native thread IDs from summaries and sub-agent wire payloads, and reject client-asserted ownership or lifecycle metadata on the general thread-open endpoint.
Require identity-preserving resume for linked children, with bounded retries for newly advertised Codex rollouts. Keep primary-thread recovery unchanged.
Serialize lifecycle mutations per project, bound HTTP lock contention, process linked evidence through a per-parent FIFO, materialize children off the parent forwarder, reuse existing native bindings, and prevent duplicate imports.
Coordinate passive monitors and interactive turns through one ownership gate. Use a renewable active idle bound, atomically adopt terminal evidence, persist fallback output once, suppress duplicate turnless broadcasts, and await monitor cancellation through completion signals during deletion.
Delete complete child subtrees leaf-first after repeated active-work preflights. Treat only exact missing-rollout deletion responses as idempotent native success.
Share forwarder state through RegistryShared, isolate passive pre-turn waiting, use typed lifecycle policies and native-open results, and consolidate duplicated route, prompt, item, and frontend helpers.
Recover stale live buffers on genuine new starts without dropping new events. Document both protocols, trusted link opening, navigation, monitoring, prompt, recovery, and deletion behavior in the README, adapter guide, sub-agent guide, and specification.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation