Add Fabric chat session entrypoint - #21
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesSession Identity and Chat Flow
Sequence Diagram(s)See hidden review stack artifact. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
177-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the session-mode requirement explicit in the examples.
The prose says sessions/chat require
runtime.mode: session, but neither snippet shows that field. Copy/paste users can end up with a profile that looks valid and then fail at startup if it isn't session-mode.🤖 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 `@README.md` around lines 177 - 233, Make the session-mode requirement explicit in both the `FabricClient().start(...)` example and the `fabric chat` example by showing that the referenced profile must use `runtime.mode: session`. Update the `chat()` snippet and the CLI snippet so users can see the needed session-mode configuration in context, matching the prose already under the “Multi-Turn SDK Sessions” and “Interactive CLI Chat” sections.
🧹 Nitpick comments (2)
python/tests/smoke_readme_examples.py (1)
36-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the README smoke anchors.
These checks only prove that a few literals exist; they do not protect the chat/session transcript contract described in the README. If the banner, prompt, or
/verbosemetadata output drifts, this test will still pass.Consider anchoring on a couple of stable lines from the actual example (
session.runtime_id,agent>,/info) so the smoke fails when the user-facing flow changes.🤖 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 `@python/tests/smoke_readme_examples.py` around lines 36 - 43, The README smoke test is too loose because it only checks for a few standalone literals and can miss regressions in the chat/session transcript contract. Update the assertions in smoke_readme_examples.py to anchor on stable, user-facing lines from the actual example, such as session.runtime_id, agent>, and /info, so the test fails when the banner, prompt, or verbose metadata flow changes. Keep the checks tied to the example’s transcript rather than generic strings like session_id or fabric chat.tests/test_session.py (1)
167-167: 📐 Maintainability & Code Quality | 🔵 TrivialOptional: silence RUF043 by marking the
match=pattern as raw or escaping the dot.The
.inruntime.modeis a regex metacharacter. It happens to match the literal dot, so behavior is unaffected, but Ruff flags it.♻️ Suggested tweak
- with pytest.raises(RuntimeError, match="requires runtime.mode=session"): + with pytest.raises(RuntimeError, match=r"requires runtime\.mode=session"):Also applies to: 206-206, 613-613
🤖 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 `@tests/test_session.py` at line 167, Update the pytest.raises assertions in test_session to avoid Ruff RUF043 by making the match patterns raw strings or escaping the dot in runtime.mode; locate the affected assertions around the RuntimeError checks in the session tests and adjust the match= values consistently for all mentioned occurrences.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@README.md`:
- Around line 177-233: Make the session-mode requirement explicit in both the
`FabricClient().start(...)` example and the `fabric chat` example by showing
that the referenced profile must use `runtime.mode: session`. Update the
`chat()` snippet and the CLI snippet so users can see the needed session-mode
configuration in context, matching the prose already under the “Multi-Turn SDK
Sessions” and “Interactive CLI Chat” sections.
---
Nitpick comments:
In `@python/tests/smoke_readme_examples.py`:
- Around line 36-43: The README smoke test is too loose because it only checks
for a few standalone literals and can miss regressions in the chat/session
transcript contract. Update the assertions in smoke_readme_examples.py to anchor
on stable, user-facing lines from the actual example, such as
session.runtime_id, agent>, and /info, so the test fails when the banner,
prompt, or verbose metadata flow changes. Keep the checks tied to the example’s
transcript rather than generic strings like session_id or fabric chat.
In `@tests/test_session.py`:
- Line 167: Update the pytest.raises assertions in test_session to avoid Ruff
RUF043 by making the match patterns raw strings or escaping the dot in
runtime.mode; locate the affected assertions around the RuntimeError checks in
the session tests and adjust the match= values consistently for all mentioned
occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2bd9a11d-560d-4141-905b-bed19b60d368
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlREADME.mdadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pycrates/fabric-cli/Cargo.tomlcrates/fabric-cli/src/main.rscrates/fabric-core/src/runtime.rspython/src/nemo_fabric/client.pypython/tests/smoke_readme_examples.pypython/tests/smoke_sdk_sessions.pyschemas/adapter-invocation.schema.jsonschemas/runtime-context.schema.jsontests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.pytests/smoke_cli.pytests/smoke_hermes_session.pytests/test_adapaters_hermes_common.pytests/test_session.py
dagardner-nv
left a comment
dagardner-nv
left a comment
There was a problem hiding this comment.
Approving with questions:
When we resume a session in Fabric, the session id is the runtime id of the initial run correct?
When we resume, the second run has a new runtime id?
It's unclear from the README.md if the user is allowed to supply their own session_id or not.
I think this is OK, but let's just document it.
User is allowed to now supply the session_id yes. I will.make sure its clearer in documentation |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py (1)
55-66: 🗄️ Data Integrity & Integration | 🟠 MajorUse the resolved Hermes session ID for agent resume.
load_runtime_history()resolves compressed sessions, butAIAgent(..., session_id=...)still gets the original Fabric key. That can split history lookup and future writes across different Hermes sessions; resolve once and pass that same Hermes ID through both paths. Also applies to lines 116-118 and 138-140.🤖 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 `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py` around lines 55 - 66, Use the resolved Hermes session ID consistently for resume: in load_runtime_history() resolve the Fabric session key once via resolve_resume_session_id and return/pass that Hermes ID instead of the original fabric_session_id. Update the AIAgent(session_id=...) call sites and the resume path around the referenced adapter logic so both history loading and future writes use the same resolved session identifier, preventing split session state.
🤖 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.
Outside diff comments:
In `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py`:
- Around line 55-66: Use the resolved Hermes session ID consistently for resume:
in load_runtime_history() resolve the Fabric session key once via
resolve_resume_session_id and return/pass that Hermes ID instead of the original
fabric_session_id. Update the AIAgent(session_id=...) call sites and the resume
path around the referenced adapter logic so both history loading and future
writes use the same resolved session identifier, preventing split session state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41a7b3c7-c51b-4506-978a-2a03ccaa4b78
📒 Files selected for processing (3)
adapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- adapters/common/src/nemo_fabric_adapters/common/hermes.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
95-113: 🎯 Functional Correctness | 🟠 MajorUse the Hermes session id returned by
ensure_hermes_session().build_command()still passesharness_session_idto--continue, so a compressed session can resume the wrong record. Threadsession["session_id"]through the command builder and keepharness_session_idonly as the caller-facing id.🤖 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 `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py` around lines 95 - 113, `build_command()` is still using the caller-facing `harness_session_id` for `--continue` instead of the session id returned by `ensure_hermes_session()`, which can resume the wrong record. Update `adapter.py` so the result from `ensure_hermes_session()` is captured and its `session["session_id"]` is threaded into `build_command()`. Keep `harness_session_id` only for external/caller-facing use, and make sure the continuation flag is built from the Hermes session id.
🤖 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.
Outside diff comments:
In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py`:
- Around line 95-113: `build_command()` is still using the caller-facing
`harness_session_id` for `--continue` instead of the session id returned by
`ensure_hermes_session()`, which can resume the wrong record. Update
`adapter.py` so the result from `ensure_hermes_session()` is captured and its
`session["session_id"]` is threaded into `build_command()`. Keep
`harness_session_id` only for external/caller-facing use, and make sure the
continuation flag is built from the Hermes session id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a866ecab-13d5-482d-a996-e3ea01a1c63b
📒 Files selected for processing (2)
adapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
95-113: 🎯 Functional Correctness | 🟠 MajorUse the resolved Hermes session id for
--continue.run_hermes_cli()ignores the session object returned byensure_hermes_session(), andbuild_command()still passes Fabric’s key intochat --continue. If Hermes rotates the underlying session id after compression, resume can hit the wrong session; thread the returned Hermes session id intobuild_command()and keepfabric_session_idonly for Fabric-facing output.🤖 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 `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py` around lines 95 - 113, The `run_hermes_cli()` flow is still using Fabric’s `fabric_session_id` when building the `chat --continue` command, even though `ensure_hermes_session()` can return a resolved Hermes session id after rotation. Update `run_hermes_cli()` to capture the session object/id returned by `ensure_hermes_session()` and pass that resolved Hermes session id into `build_command()` for `--continue`, while keeping `fabric_session_id` only for Fabric-facing output and logging.
🤖 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.
Outside diff comments:
In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py`:
- Around line 95-113: The `run_hermes_cli()` flow is still using Fabric’s
`fabric_session_id` when building the `chat --continue` command, even though
`ensure_hermes_session()` can return a resolved Hermes session id after
rotation. Update `run_hermes_cli()` to capture the session object/id returned by
`ensure_hermes_session()` and pass that resolved Hermes session id into
`build_command()` for `--continue`, while keeping `fabric_session_id` only for
Fabric-facing output and logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 88157d66-07bb-450e-85c2-58b9e2c07587
📒 Files selected for processing (2)
adapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
94efa7c to
7f1e6f2
Compare
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/fabric-core/src/runtime.rs (1)
1224-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilently dropping non-existent relay artifacts may hide adapter contract errors.
promote_relay_artifacts_to_manifestskips any candidate whosepathdoesn't exist via!path.exists(). That's a reasonable guard, but a missing file is just as likely a path-resolution bug in the adapter as it is an intentional omission, and the silentcontinuegives no signal. Consider emitting a debug/warn trace when a declared relay artifact can't be found so misconfigured adapters are diagnosable.🤖 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/fabric-core/src/runtime.rs` around lines 1224 - 1258, The promote_relay_artifacts_to_manifest function is silently skipping relay artifacts whose resolved path does not exist, which makes adapter path bugs hard to diagnose. Update the !path.exists() branch in promote_relay_artifacts_to_manifest to emit a debug or warn trace that includes the missing artifact path and relevant kind before continuing, while keeping the existing duplicate and empty-path guards unchanged.tests/smoke_cli.py (1)
155-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
run,run_with_stdin, andrun_raw.
runandrun_with_stdinshare identical non-zero return-code assertion logic;runcould delegate torun_rawto dedupe. Optional cleanup. (The Ruff S603 / ast-grep subprocess hints are false positives here:COMMANDis a fixed constant, not request-derived input.)🤖 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 `@tests/smoke_cli.py` around lines 155 - 187, `run` and `run_with_stdin` duplicate the same non-zero return-code assertion, so simplify the smoke CLI helpers by centralizing subprocess execution and failure handling. Have `run` delegate to `run_raw` (or extract a shared assertion helper used by both `run` and `run_with_stdin`) and keep `run_raw` responsible only for calling `subprocess.run`; use the existing `run`, `run_with_stdin`, and `run_raw` symbols to update the test helper cleanly.Source: Linters/SAST tools
crates/fabric-cli/src/main.rs (1)
247-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
run_chatlifecycle ordering is correct; clippy?hint is a safe simplification.The current shape guarantees
stop_runtimealways runs even whenchat_loopfails, and prioritizes the chat error. The Clippy hint at Line 250 can be applied without changing semantics sincestop_resultis already evaluated.♻️ Optional simplification
let runtime = start_runtime(&plan)?; let chat_result = chat_loop(&plan, &runtime, session_id.as_deref(), verbose); let stop_result = stop_runtime(&plan, &runtime); - if let Err(error) = chat_result { - return Err(error); - } + chat_result?; stop_result?; Ok(())🤖 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/fabric-cli/src/main.rs` around lines 247 - 254, In run_chat, the lifecycle ordering is already correct, so simplify the manual chat_result error handling by using the `?`-style clippy suggestion only in a way that still evaluates `stop_runtime` before returning; keep the existing behavior where `chat_loop` errors are prioritized after `stop_runtime` runs, and adjust the control flow around `chat_loop`, `stop_runtime`, and the final return to make the intent clearer without changing semantics.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@crates/fabric-cli/src/main.rs`:
- Around line 247-254: In run_chat, the lifecycle ordering is already correct,
so simplify the manual chat_result error handling by using the `?`-style clippy
suggestion only in a way that still evaluates `stop_runtime` before returning;
keep the existing behavior where `chat_loop` errors are prioritized after
`stop_runtime` runs, and adjust the control flow around `chat_loop`,
`stop_runtime`, and the final return to make the intent clearer without changing
semantics.
In `@crates/fabric-core/src/runtime.rs`:
- Around line 1224-1258: The promote_relay_artifacts_to_manifest function is
silently skipping relay artifacts whose resolved path does not exist, which
makes adapter path bugs hard to diagnose. Update the !path.exists() branch in
promote_relay_artifacts_to_manifest to emit a debug or warn trace that includes
the missing artifact path and relevant kind before continuing, while keeping the
existing duplicate and empty-path guards unchanged.
In `@tests/smoke_cli.py`:
- Around line 155-187: `run` and `run_with_stdin` duplicate the same non-zero
return-code assertion, so simplify the smoke CLI helpers by centralizing
subprocess execution and failure handling. Have `run` delegate to `run_raw` (or
extract a shared assertion helper used by both `run` and `run_with_stdin`) and
keep `run_raw` responsible only for calling `subprocess.run`; use the existing
`run`, `run_with_stdin`, and `run_raw` symbols to update the test helper
cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e8cdc96-9156-4c78-87f6-2d63a44cebd4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlREADME.mdadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pycrates/fabric-cli/Cargo.tomlcrates/fabric-cli/src/main.rscrates/fabric-core/src/runtime.rspython/src/nemo_fabric/client.pypython/tests/smoke_readme_examples.pypython/tests/smoke_sdk_sessions.pyschemas/adapter-invocation.schema.jsonschemas/runtime-context.schema.jsontests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.pytests/smoke_cli.pytests/smoke_hermes_session.pytests/test_adapaters_hermes_common.pytests/test_session.py
✅ Files skipped from review due to trivial changes (3)
- crates/fabric-cli/Cargo.toml
- tests/smoke_hermes_session.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- python/tests/smoke_readme_examples.py
- schemas/adapter-invocation.schema.json
- tests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.py
- Cargo.toml
- adapters/common/src/nemo_fabric_adapters/common/hermes.py
- schemas/runtime-context.schema.json
- adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
- python/tests/smoke_sdk_sessions.py
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
2 participants
Summary
fabric chatfor session-mode profiles using the native runtime lifecycle.session_idthrough SDK/CLI session turns into adapter runtime context.runtime.mode=sessionforfabric chatand SDKSessionentrypoints.Validation
cargo testcargo check -p fabric-pythonpython3 tests/smoke_cli.pyPYTHONPATH=python/src pytest -q tests/test_session.pyPYTHONPATH=python/src python3 python/tests/smoke_sdk.pyPYTHONPATH=python/src python3 python/tests/smoke_sdk_sessions.pyPYTHONPATH=python/src python3 python/tests/smoke_readme_examples.pypython3 tests/smoke_hermes_cli.py && python3 tests/smoke_hermes_config_mapping.py && python3 tests/smoke_swebench_style.pyPYTHONPATH=adapters/common/src:adapters/hermes-sdk/src:python/src pytest -q tests/test_adapaters_hermes_common.py tests/test_hermes_sdk_adapter.py tests/test_hermes_cli.pyprintf 'hello from manual check\n/exit\n' | cargo run -q -p fabric-cli -- chat tests/fixtures/hermes-shim-agent --profile env_local --session-id cli-session-123 --verboseexits 0 with empty stdout and chat UI on stderr.cargo run -q -p fabric-cli -- chat examples/code-review-agent --profile hermes_sdk </dev/nullexits 1 withfabric chat requires runtime.mode=session; use \fabric run` for oneshot profiles`.git diff --checkSummary by CodeRabbit
New Features
session_idsupport across the SDK and Hermes adapter flow (including per-turn context propagation).fabric chatcommand for session-mode runtimes, supporting resumable sessions and optional per-turn verbose output.Bug Fixes
session_id, with clear validation/errors when not usingruntime.mode: session.Documentation
Tests / Chores
ctrlcfor clean interrupt handling.