feat(security): add behavioral session tracker with trifecta detection#965
feat(security): add behavioral session tracker with trifecta detection#965gemini2026 wants to merge 13 commits intoNVIDIA:mainfrom
Conversation
Tracks three capability classes per session: read_sensitive, ingested_untrusted, has_egress. When all three appear (trifecta), risk escalates to critical, detecting multi-step exfiltration attacks that per-action gates miss.
Align getExposure with getCapabilities and hasTrifecta by adding an explicit empty-string guard. Add corresponding test case.
|
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 an in-memory SessionStore that records per-session capability events ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/reference/session-tracker.md (2)
3-3: Align the H1 withtitle.page.The H1 does not match the frontmatter page title.
As per coding guidelines: "H1 heading matches the
title.pagefrontmatter value."Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 3, Update the document so the top-level H1 matches the frontmatter title.page value ("Session Tracker — Behavioral Trifecta Detection"): locate the frontmatter key title.page and replace or edit the existing H1 heading (the leading "# ..." line) to exactly match that value, ensuring punctuation and capitalization are identical; also check the second occurrence (line 21) mentioned and make it consistent with title.page as well.
6-8: Fix product-name casing in frontmatter metadata.Use
NemoClaw,OpenClaw, andOpenShellcasing consistently (including keywords/tags).As per coding guidelines: "NemoClaw, OpenClaw, and OpenShell must use correct casing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` around lines 6 - 8, The frontmatter arrays keywords and tags use incorrect product-name casing—update occurrences of "nemoclaw" to "NemoClaw", "openclaw" to "OpenClaw", and "openshell" to "OpenShell" in the keywords and tags entries in docs/reference/session-tracker.md (the frontmatter keys: keywords and tags) so the metadata uses the correct product-name casing consistently.nemoclaw/src/security/session-tracker.test.ts (1)
121-150: Consolidate duplicated event-cap tests.These two blocks validate the same boundary behavior (100 stored, 101st dropped). Merging them into one parameterized/compact suite would keep intent while reducing maintenance overhead.
Also applies to: 329-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/session-tracker.test.ts` around lines 121 - 150, The two test blocks under the "event cap" suite duplicate boundary checks for storing exactly 100 events and dropping the 101st; consolidate them by creating a single parameterized/compact test that covers: (1) inserting N events where N=100 asserts exposure.events length is 100 and last event detail is "/file-99", (2) inserting N+1 events where the 101st is dropped, and (3) after the log is full a subsequent store.record("s1", Capability.HasEgress, ...) still flips getCapabilities("s1")[Capability.HasEgress] to true while exposure.events remains length 100; reuse store.record, getExposure and getCapabilities calls and remove the duplicated block (the similar tests later in the file) so the suite tests the boundary once with clear parameterization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/session-tracker.md`:
- Around line 95-96: Update the docs for getExposure() to state that it returns
null not only for unknown sessions but also when an empty sessionId is passed;
specifically mention the empty-sessionId null behavior so it matches the
implementation of getExposure(sessionId) and references the parameter name
sessionId and the function getExposure().
- Line 101: The sentence about egressAttempts is misleading because the
implementation emits just the tool name when detail is empty; update the wording
to state that egressAttempts records every has_egress event as "tool + ' ' +
detail" when detail is non-empty, and as just the tool name when detail === ""
(i.e., no trailing space), referencing the egressAttempts array and the
has_egress event/detail variable to make the conditional behavior explicit.
In `@nemoclaw/src/security/session-tracker.ts`:
- Around line 222-223: getExposure currently shallow-copies the session.events
array (const eventsCopy: CapabilityEvent[] = [...sess.events]) which still
exposes the internal CapabilityEvent objects to external mutation; change this
to return deep copies of each event instead — for example, replace the shallow
spread with a per-item clone (e.g., map and shallow-clone each event object with
object spread or use structuredClone/JSON deep-clone if events contain nested
state) so callers of getExposure cannot mutate sess.events; update references in
getExposure and any tests to use the cloned events.
---
Nitpick comments:
In `@docs/reference/session-tracker.md`:
- Line 3: Update the document so the top-level H1 matches the frontmatter
title.page value ("Session Tracker — Behavioral Trifecta Detection"): locate the
frontmatter key title.page and replace or edit the existing H1 heading (the
leading "# ..." line) to exactly match that value, ensuring punctuation and
capitalization are identical; also check the second occurrence (line 21)
mentioned and make it consistent with title.page as well.
- Around line 6-8: The frontmatter arrays keywords and tags use incorrect
product-name casing—update occurrences of "nemoclaw" to "NemoClaw", "openclaw"
to "OpenClaw", and "openshell" to "OpenShell" in the keywords and tags entries
in docs/reference/session-tracker.md (the frontmatter keys: keywords and tags)
so the metadata uses the correct product-name casing consistently.
In `@nemoclaw/src/security/session-tracker.test.ts`:
- Around line 121-150: The two test blocks under the "event cap" suite duplicate
boundary checks for storing exactly 100 events and dropping the 101st;
consolidate them by creating a single parameterized/compact test that covers:
(1) inserting N events where N=100 asserts exposure.events length is 100 and
last event detail is "/file-99", (2) inserting N+1 events where the 101st is
dropped, and (3) after the log is full a subsequent store.record("s1",
Capability.HasEgress, ...) still flips
getCapabilities("s1")[Capability.HasEgress] to true while exposure.events
remains length 100; reuse store.record, getExposure and getCapabilities calls
and remove the duplicated block (the similar tests later in the file) so the
suite tests the boundary once with clear parameterization.
🪄 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
Run ID: a4fe7a16-7e3b-4730-8713-6e296e3f559e
📒 Files selected for processing (3)
docs/reference/session-tracker.mdnemoclaw/src/security/session-tracker.test.tsnemoclaw/src/security/session-tracker.ts
- Deep-copy CapabilityEvent objects in getExposure to prevent external mutation of internal session state - Document getExposure null return for empty sessionId - Clarify egressAttempts formatting when detail is empty
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/session-tracker.md`:
- Line 3: The H1 heading "# Session Tracker" does not match the frontmatter key
title.page ("Session Tracker — Behavioral Trifecta Detection"); update the H1 so
it exactly matches title.page (including punctuation and spacing) to satisfy the
page-structure rule and ensure the H1 equals title.page.
🪄 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
Run ID: 209b67da-c891-4e74-a04f-3c003296fd58
📒 Files selected for processing (2)
docs/reference/session-tracker.mdnemoclaw/src/security/session-tracker.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/src/security/session-tracker.ts
Align with the pattern used by other reference pages where H1 is the short name and title.page adds descriptive context.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/reference/session-tracker.md (1)
21-21:⚠️ Potential issue | 🟡 MinorH1 must match
title.pageexactly.The H1 heading "Session Tracker" does not match the
title.pagefrontmatter value "Session Tracker — Detect Multi-Step Exfiltration Attacks" (line 3). Update line 21 to include the full title with the em dash and subtitle.As per coding guidelines, "H1 heading matches the
title.pagefrontmatter value."📝 Proposed fix
-# Session Tracker +# Session Tracker — Detect Multi-Step Exfiltration Attacks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 21, Update the H1 heading so it exactly matches the frontmatter title.page value: replace the current "Session Tracker" H1 with "Session Tracker — Detect Multi-Step Exfiltration Attacks" (ensure you include the em dash and subtitle exactly as in title.page).
🧹 Nitpick comments (1)
docs/reference/session-tracker.md (1)
40-40: Replace colon with em dash or period.The colon in this sentence introduces an explanation rather than a list. The formatting rule states that colons should only introduce lists. Consider using an em dash or splitting into two sentences.
As per coding guidelines, "Colons should only introduce a list. Flag colons used as general punctuation between clauses."
📝 Suggested alternatives
Option 1: Use em dash
-A trifecta indicates a possible exfiltration chain: read a secret, get instructions from an attacker, and send the secret out. +A trifecta indicates a possible exfiltration chain — read a secret, get instructions from an attacker, and send the secret out.Option 2: Two sentences
-A trifecta indicates a possible exfiltration chain: read a secret, get instructions from an attacker, and send the secret out. +A trifecta indicates a possible exfiltration chain. +The agent reads a secret, gets instructions from an attacker, and sends the secret out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 40, Replace the colon in the sentence "A trifecta indicates a possible exfiltration chain: read a secret, get instructions from an attacker, and send the secret out." with an em dash or split into two sentences so it no longer uses a colon as general punctuation; update the line to either "A trifecta indicates a possible exfiltration chain — read a secret, get instructions from an attacker, and send the secret out." or "A trifecta indicates a possible exfiltration chain. It means reading a secret, getting instructions from an attacker, and sending the secret out."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/reference/session-tracker.md`:
- Line 21: Update the H1 heading so it exactly matches the frontmatter
title.page value: replace the current "Session Tracker" H1 with "Session Tracker
— Detect Multi-Step Exfiltration Attacks" (ensure you include the em dash and
subtitle exactly as in title.page).
---
Nitpick comments:
In `@docs/reference/session-tracker.md`:
- Line 40: Replace the colon in the sentence "A trifecta indicates a possible
exfiltration chain: read a secret, get instructions from an attacker, and send
the secret out." with an em dash or split into two sentences so it no longer
uses a colon as general punctuation; update the line to either "A trifecta
indicates a possible exfiltration chain — read a secret, get instructions from
an attacker, and send the secret out." or "A trifecta indicates a possible
exfiltration chain. It means reading a secret, getting instructions from an
attacker, and sending the secret out."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c98d63a2-4938-4370-a148-41e48fce8897
📒 Files selected for processing (1)
docs/reference/session-tracker.md
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/reference/session-tracker.md (3)
102-102: Use active voice.The phrase "are not deduplicated" is passive. Rewrite in active voice.
Suggested revision:
- Egress attempts are not deduplicated. + The tracker does not deduplicate egress attempts.As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 102, Replace the passive sentence "Egress attempts are not deduplicated." with an active-voice version (for example: "We do not deduplicate egress attempts." or "The system does not deduplicate egress attempts.") in docs/reference/session-tracker.md so the statement complies with the project's active-voice guideline.
56-56: Use active voice.The phrase "are dropped" is passive. Rewrite in active voice.
Suggested revision:
-Events beyond the 100th are dropped, but the capability set continues to update. +The tracker drops events beyond the 100th, but continues to update the capability set.As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 56, Replace the passive sentence "Events beyond the 100th are dropped, but the capability set continues to update." with an active-voice version such as "We drop events beyond the 100th, but the capability set continues to update." to satisfy the active voice guideline; locate that exact sentence in session-tracker.md and update it accordingly.
77-77: Use active voice.The phrase "are silently ignored" is passive. Rewrite in active voice.
Suggested revision:
-Empty `sessionId` values are silently ignored. +The method silently ignores empty `sessionId` values.As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` at line 77, Change the passive sentence about `sessionId` to active voice: locate the sentence containing `sessionId` and replace "Empty `sessionId` values are silently ignored." with an active phrasing such as "The system ignores empty `sessionId` values." (or "We ignore empty `sessionId` values.") to meet the active-voice guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/session-tracker.md`:
- Line 102: Replace the passive sentence "Egress attempts are not deduplicated."
with an active-voice version (for example: "We do not deduplicate egress
attempts." or "The system does not deduplicate egress attempts.") in
docs/reference/session-tracker.md so the statement complies with the project's
active-voice guideline.
- Line 56: Replace the passive sentence "Events beyond the 100th are dropped,
but the capability set continues to update." with an active-voice version such
as "We drop events beyond the 100th, but the capability set continues to
update." to satisfy the active voice guideline; locate that exact sentence in
session-tracker.md and update it accordingly.
- Line 77: Change the passive sentence about `sessionId` to active voice: locate
the sentence containing `sessionId` and replace "Empty `sessionId` values are
silently ignored." with an active phrasing such as "The system ignores empty
`sessionId` values." (or "We ignore empty `sessionId` values.") to meet the
active-voice guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4040fa89-6063-4cd9-ab1e-d75c361b8ff5
📒 Files selected for processing (1)
docs/reference/session-tracker.md
wscurran
added
enhancement
|
✨ Thanks for submitting this proposal with a detailed summary, it proposes adding behavioral session tracking to NemoClaw's security toolkit, which could enhance the security of NemoClaw and provide better protection for users. |
There was a problem hiding this comment.
Review: Behavioral Session Tracker
Thanks for the contribution — the trifecta detection concept (read sensitive + ingest untrusted + egress = exfiltration chain) is a sound security pattern and worth exploring for NemoClaw.
A few things we'd need addressed before this can move forward:
Integration required
The SessionStore is self-contained with no call sites anywhere in NemoClaw. Nobody calls record() today, so the tracker would ship as dead code. For this to provide value, it needs to be wired into the agent execution pipeline — likely hooking into OpenClaw's tool-call lifecycle (beforeToolUse/afterToolUse) or the NemoClaw plugin's command dispatch. Without at least one integration point, there's nothing to review from a security effectiveness standpoint.
No consumer for detection results
The PR notes that "consumer decides what to do when trifecta is detected," but there's no callback, event emitter, or hook mechanism for a consumer to subscribe to. Detection without response is just logging — we'd need at minimum a way to register a handler (e.g., log a warning, emit a metric, terminate the session) when risk escalates.
Session lifecycle management
Sessions are stored in a Map with no TTL, eviction policy, or clear() method. In a long-running gateway process, this will grow unbounded. Would need either:
- A max session count with LRU eviction, or
- A TTL-based cleanup (e.g., sessions older than 1h are pruned), or
- Tying session lifetime to OpenClaw's own session lifecycle
Documentation references non-existent pages
The doc at docs/reference/session-tracker.md links to {doc}/reference/injection-scanner and {doc}/reference/audit-chain — these don't exist in the repo. Please either remove these references or note them as future work.
Minor observations
- The 100-event cap is reasonable but would benefit from a brief justification (why 100 vs. 50 or 500?)
- In-memory-only storage means all tracking is lost on container restart — worth calling out explicitly as a known limitation and whether that's acceptable for the threat model
classifyRisk()has a redundant path: ifcount > 2it returns"critical", but that case is already caught by thetrifectacheck above it (3 capabilities = trifecta = true)
Suggested path forward
The implementation quality is solid (good types, thorough tests, clean separation). The gap is entirely about integration. Would suggest:
- Define where
record()gets called (which tool-call hook or middleware) - Define what happens when trifecta is detected (the consumer/response mechanism)
- Add session cleanup
- Then this becomes a meaningful addition to the security stack
Happy to re-review once there's an integration plan.
- Remove dead code: `classifyRisk()` count > 2 branch can never be reached because trifecta is checked first and there are exactly three capability classes — simplify to a ternary - Add `onTrifecta` callback to `SessionStore` constructor so callers can log a warning, emit a metric, or terminate the session when risk escalates; callback fires once per session on first trifecta - Add `clear()` method to release all tracked state; prevents unbounded memory growth in long-running processes - Consolidate duplicate event-cap / boundary-condition test suites into a single test covering all boundary behaviors - Add tests for `onTrifecta` (fires once, per session, not partial) and `clear()` (resets state, allows new sessions) - Fix passive voice in docs: "are dropped" → "The tracker drops", "are silently ignored" → "The method silently ignores", "are not deduplicated" → "The tracker does not deduplicate" - Replace colon with em dash in exfiltration chain sentence - Add justification for 100-event cap (~10 KB per session) - Document in-memory-only limitation explicitly - Annotate Next Steps cross-refs with pending PR numbers (NVIDIA#870/NVIDIA#892) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@prekshivyas good call on all of these. Here's what I pushed:
Re: integration — you're right, should've wired it from the start instead of proposing a follow-up. Pushing integration code next that hooks into |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nemoclaw/src/security/session-tracker.test.ts (1)
92-117: Consider adding a test case for zero capabilities, or document why"clean"is unreachable.Looking at the implementation, a session is only created when
record()is called, which always sets at least one capability. This means the"clean"risk level (count === 0) inclassifyRisk()is currently unreachable code.This isn't necessarily a bug—the API design is valid—but the test suite could benefit from documenting this invariant. Either:
- Add a comment explaining that
"clean"exists for API completeness or future use- Add a test that explicitly verifies a freshly recorded session is never
"clean"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/session-tracker.test.ts` around lines 92 - 117, Add coverage or documentation for the unreachable "clean" branch in classifyRisk(): either add a test that asserts a session created via store.record(...) is never classified as "clean" (use store.record, store.listSessions(), and check session.riskLevel !== "clean") or add an inline comment next to classifyRisk() (or where sessions are created in record()) stating the invariant that sessions are only created via record() and thus always have at least one capability; reference the classifyRisk(), record(), listSessions(), and riskLevel symbols so reviewers can verify the rationale.docs/reference/session-tracker.md (1)
59-61: Passive voice detected.Line 60 uses passive voice ("is lost"). Consider rewriting to active voice.
Suggested revision: "The host process loses all tracking state when it restarts."
📝 Suggested fix
The tracker holds sessions in memory only. -All tracking state is lost when the host process restarts. +The host process loses all tracking state when it restarts. This is an acceptable trade-off for ephemeral sandbox sessions, where the threat window is bounded by the container lifetime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/session-tracker.md` around lines 59 - 61, Replace the passive sentence "All tracking state is lost when the host process restarts." with an active-voice version; update it to say "The host process loses all tracking state when it restarts." so the line reads clearly and directly while preserving the original meaning and context in the session-tracker documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/session-tracker.md`:
- Around line 59-61: Replace the passive sentence "All tracking state is lost
when the host process restarts." with an active-voice version; update it to say
"The host process loses all tracking state when it restarts." so the line reads
clearly and directly while preserving the original meaning and context in the
session-tracker documentation.
In `@nemoclaw/src/security/session-tracker.test.ts`:
- Around line 92-117: Add coverage or documentation for the unreachable "clean"
branch in classifyRisk(): either add a test that asserts a session created via
store.record(...) is never classified as "clean" (use store.record,
store.listSessions(), and check session.riskLevel !== "clean") or add an inline
comment next to classifyRisk() (or where sessions are created in record())
stating the invariant that sessions are only created via record() and thus
always have at least one capability; reference the classifyRisk(), record(),
listSessions(), and riskLevel symbols so reviewers can verify the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4afbb79-2629-4d37-9473-93f4e9d95215
📒 Files selected for processing (3)
docs/reference/session-tracker.mdnemoclaw/src/security/session-tracker.test.tsnemoclaw/src/security/session-tracker.ts
- Wrap onTrifecta callback in try/catch so a broken handler never
disrupts recording — the store state is already mutated by the time
the callback fires, so propagating would leave callers in an
inconsistent state
- Replace {doc} cross-references with plain-text file paths to avoid
Sphinx build errors while injection-scanner (NVIDIA#870) and audit-chain
(NVIDIA#892) pages are still pending
- Add "NemoClaw" prefix to title.page and H1 to match the naming
convention used by other reference pages (NemoClaw Architecture,
NemoClaw Network Policies, etc.)
- Add test: getExposure returns null after clear() — catches partial-
reset bugs that wipe capabilities but not the event log
- Add test: onTrifecta fires again after clear + re-record — verifies
the callback is not permanently suppressed for a session ID
- Add test: throwing onTrifecta callback is swallowed without error
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
More fixes:
|
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
@gemini2026 Thanks for fixing the callback error-bounding, the Sphinx cross-refs, and the H1 mismatch — those are all good.
However, the main blocker from the previous review is still unaddressed: this has no integration points. SessionStore is self-contained with zero call sites in NemoClaw. Nobody calls record() anywhere, so this would ship as dead code.
Before this can move forward, we need:
- At least one integration point — hook into the agent execution pipeline (e.g., OpenClaw's
beforeToolUse/afterToolUseor NemoClaw plugin command dispatch) so capabilities are actually recorded during real agent sessions - A decision on trifecta response — what happens when all three capabilities are detected? Log only? Block the action? Alert the operator? The tracker detects the condition but nothing acts on it
- Shallow-copy mutation fix — CodeRabbit flagged that
getExposure()only shallow-copies arrays, so returned events still alias internal state and can be mutated by consumers
The trifecta concept is solid — we want to land this, but not as a standalone library with no consumers. Please add the wiring and we can re-review.
Address the main blocker from PR review: SessionStore had no call sites and shipped as dead code. Integration: - Add session-integration.ts that wires SessionStore into the OpenClaw plugin lifecycle via before_tool_call and session_end hooks - Classify tool calls into capability classes based on tool name and argument patterns (read_file + sensitive path → ReadSensitive, fetch/curl → IngestedUntrusted, http_post/send_email → HasEgress) - Log warning via api.logger.warn() when trifecta is first detected - Clean up sessions on session_end to prevent unbounded memory growth - Call wireSessionTracker(api) from register() in index.ts SessionStore additions: - Add delete(sessionId) method for per-session cleanup - 3 new tests for delete() Integration test suite: - 22 tests covering classification rules, hook registration, trifecta warning, session cleanup, and edge cases (no sessionId, non-matching tools, non-sensitive paths) Documentation: - Add Integration section to session-tracker.md documenting the hook wiring, classification rules table, and trifecta response behavior - Document delete() method in the API section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Integration pushed (16dcbbe). Hooks into
Cleans up on Rules are conservative on purpose — detection aid, not a gate. Easy to extend. Trifecta fires Re: shallow-copy — already fixed, |
prekshivyas
added
priority: medium
- Frontmatter description: flat string → main/agent structure - Em dash → colon in title.page and H1
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
@gemini2026 Thanks for the persistence through multiple review rounds — the code quality and test coverage are genuinely good, and the trifecta concept (credential read + untrusted ingestion + egress = exfiltration chain) is a valid security pattern worth thinking about.
After reviewing the latest version thoroughly, we've decided not to move forward with this PR. Here's the reasoning:
The fundamental problem: tool-call tracking can't see shell execution
The tracker hooks into before_tool_call and classifies tools like http_post, send_email, fetch. But an agent performing real exfiltration would use bash_execute or exec — running curl, wget, git push, or piping data through a subprocess. These bypass the tool classification entirely because the tracker sees "bash_execute" (one tool) but has no visibility into what the shell command actually does.
Adding bash_execute to the egress list would flag every shell command as potential egress, which makes the signal useless.
Detection-only doesn't fit our threat model
NemoClaw's security model is built on enforcement: Landlock blocks filesystem writes at the kernel level, network policies block unauthorized egress at the L7 proxy, the secret scanner blocks credential writes to memory. Each of these prevents the action.
The session tracker observes after the fact. By the time the trifecta warning fires, the exfiltration has already happened. For operators actively watching logs in real-time, this has some value — but our users run sandboxes unattended, often headless.
False positive volume would cause alert fatigue
Every fetch/curl to any URL (including localhost, internal APIs) is classified as ingested_untrusted. Most legitimate agent sessions make API calls, meaning nearly every session would immediately reach elevated risk. When the baseline is noisy, the trifecta signal gets lost.
Session-boundary evasion is trivial
The tracker is per-session with no cross-session correlation. An agent can read a secret in session A, end the session, and exfiltrate in session B. The trifecta never fires.
Where behavioral detection would actually work
The right place for this kind of detection is OpenShell's L7 proxy and network layer — where egress actually happens, regardless of which tool or shell command initiated it. OpenShell can see every outbound connection, inspect payloads, and correlate with filesystem access patterns at the kernel level (Landlock/audit). That's where behavioral detection can't be bypassed by tool-call indirection.
Summary
| What | Status |
|---|---|
| Code quality | Good |
| Test coverage | Good (67 tests) |
| Core concept | Valid |
| Practical security value for NemoClaw | Low — bypassed by shell execution, detection-only, high false positive rate |
| Recommendation | Close |
Thanks again for the contribution.
4 participants
Closes #964
What this does
Adds behavioral session tracking under
nemoclaw/src/security/. Most agent security gates check each tool call in isolation — an agent that reads a secret, fetches untrusted input, and opens an outbound connection across separate actions slips through. This tracker aggregates those capabilities over a session and flags when all three appear together (the "trifecta").How it works
Three capability classes:
read_sensitive,ingested_untrusted,has_egress. Risk goes fromclean→elevated(1-2 caps) →critical(all 3).The integration module hooks into
before_tool_callto classify tool invocations automatically:read_sensitiveingested_untrustedhas_egressOn trifecta: logs a warning via
api.logger.warn(). Pluggable viaonTrifectacallback if you want to block or alert instead.Sessions clean up on
session_end. Events capped at 100 per session (~10 KB). In-memory only — acceptable for ephemeral sandbox sessions.Wired into
register()in index.ts:wireSessionTracker(api).Files
session-tracker.ts— core SessionStore with trifecta detectionsession-integration.ts— plugin lifecycle hooks + tool classificationsession-tracker.test.ts— 45 testssession-integration.test.ts— 22 testsdocs/reference/session-tracker.md— reference docsindex.tsTest plan
tsc --noEmitclean