refactor(agent): derive state handling from definitions - #8143
refactor(agent): derive state handling from definitions#8143jyaunches wants to merge 17 commits into
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces static state-directory inventories with manifest-derived lock plans. ChangesManifest-driven state contract and consumers
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d7bb3cd in the TypeScript / code-coverage/cliThe overall coverage in commit d7bb3cd in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8143.docs.buildwithfern.com/nemoclaw |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/shields/index.ts (1)
3318-3341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
planIssuescan select the wrong recovery guidance.
planIssuesis assigned beforeverify(...)runs. Ifverify(...)throws, the catch block replacesdriftIssueswith the resolve message but leavesplanIssuespopulated. The recovery block at Lines 3365-3368 then tells the operator to rebuild so the plan matches the manifest, although the reported drift is a resolve failure. ClearplanIssuesin the catch block.🐛 Proposed fix
} catch (err) { const msg = err instanceof Error ? err.message : String(err); + planIssues = []; driftIssues = [`unable to resolve agent config target: ${msg}`]; }🤖 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 `@src/lib/shields/index.ts` around lines 3318 - 3341, Clear planIssues in the catch block handling failures around verify(...) so recovery guidance cannot use stale state-lock plan issues when target resolution or verification throws. Keep driftIssues set to the existing resolve-error message and ensure the recovery logic sees an empty planIssues array for this failure path.
🧹 Nitpick comments (8)
test/repro-2681-group-writable.test.ts (1)
719-728: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe subprocess copy of
stateDirGuardActioncan diverge from the host copy.This inline copy duplicates the helper defined at Lines 62-69 and uses
||where the host version uses??. The two definitions must stay in step for the assertions at Lines 802-804 and 856 to mean the same thing. Serialize the single helper into the subprocess source instead of writing it twice.♻️ Serialize one definition
-function stateDirGuardAction(command) { - const installedIndex = command.indexOf(STATE_DIR_GUARD); - if (installedIndex >= 0) return command[installedIndex + 1] || null; - const pythonIndex = command.indexOf("python3"); - return pythonIndex >= 0 && command[pythonIndex + 2] === "-" - ? (command[pythonIndex + 3] || null) - : null; -} +const stateDirGuardAction = ${stateDirGuardAction.toString()};🤖 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 `@test/repro-2681-group-writable.test.ts` around lines 719 - 728, Replace the duplicated inline stateDirGuardAction definition in the subprocess source with a serialized copy of the host helper defined by stateDirGuardAction, so both subprocess and host assertions use identical nullish-value behavior. Keep the existing subprocess constants and invocation flow unchanged.src/lib/shields/policy-transition.test.ts (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
version: 1 as constfor consistency and type safety.The sibling fixture at Lines 114-121 pins the literal type. Here
version: 1widens tonumberunless the enclosing object has a declared type. If any consumer expectsAgentStateLockPlan, the widened type failstsc. Pin the literal in both fixtures.🤖 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 `@src/lib/shields/policy-transition.test.ts` around lines 40 - 47, Update the stateLockPlan fixtures in the relevant test cases to declare version as the literal type 1 using the existing `as const` pattern, including both sibling fixtures. Keep all other fixture fields unchanged.src/lib/shields/state-dir-lock.ts (3)
101-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPlan comparison is order-sensitive.
plansMatchcompares serialized arrays. A manifest edit that only reorders entries inreadOnlyRoots,readOnlyPrefixes, orwritableSubpathsproduces "installed state lock plan differs from the current agent manifest" and blocks every Shields transition until the operator rebuilds, although the policy is unchanged. Compare sets or sorted copies so only semantic differences fail closed.♻️ Order-insensitive comparison
function plansMatch(actual: AgentStateLockPlan, expected: AgentStateLockPlan): boolean { return PLAN_ARRAY_FIELDS.every( - (field) => JSON.stringify(actual[field]) === JSON.stringify(expected[field]), + (field) => + JSON.stringify([...actual[field]].sort()) === JSON.stringify([...expected[field]].sort()), ); }🤖 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 `@src/lib/shields/state-dir-lock.ts` around lines 101 - 105, Update plansMatch to compare readOnlyRoots, readOnlyPrefixes, and writableSubpaths without regard to entry order, using set or sorted-copy semantics while preserving duplicate handling as appropriate. Keep comparisons order-sensitive only where array order is semantically meaningful, and continue returning false for actual policy differences.
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
hasImageRecoveryPlanreintroduces a hard-coded agent path list.This function decides plan-aware behavior from two literal config directories. The PR goal is to derive per-agent state authority from the manifest. An agent that later ships a generated plan, or an agent whose config directory changes, silently keeps the host-injection path and skips installed-plan validation.
Consider passing an explicit flag derived from the agent definition, for example whether the resolved target declares a packaged plan, instead of comparing
configDirstrings.♻️ Sketch of a manifest-derived signal
-function hasImageRecoveryPlan(configDir: string): boolean { - return configDir === "/sandbox/.openclaw" || configDir === "/sandbox/.hermes"; -} +// Callers pass the agent definition's packaged-plan declaration so no +// per-agent path inventory lives in the Shields layer. +function hasImageRecoveryPlan(packagesStateLockPlan: boolean): boolean { + return packagesStateLockPlan; +}🤖 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 `@src/lib/shields/state-dir-lock.ts` around lines 113 - 115, Replace the hard-coded directory comparisons in hasImageRecoveryPlan with an explicit manifest-derived signal from the resolved agent definition, such as whether the target declares a packaged plan. Pass that flag through the callers and use it to select plan-aware behavior, so newly generated plans and configuration-directory changes follow the manifest without updating this function.
252-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState the exit criteria for the historical-image path.
The
historicalbranch keeps a second lock implementation alive: the older container helper runs with its own built-in path inventory instead of the manifest plan. The comment explains the intent but does not bound the window. Add the retirement issue link and the observable exit criterion, for example the minimum image build that always shipsstate-lock-plan.json, so the superseded path can be deleted.As per path instructions: "Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window ... link the retirement issue or PR in GitHub, and state observable exit criteria."
Also applies to: 289-292
🤖 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 `@src/lib/shields/state-dir-lock.ts` around lines 252 - 279, The historical-image fallback in the runtime guard needs an explicit, bounded retirement plan. Update the surrounding comment for the historical branch in the state-dir guard to link the retirement issue or PR and state the observable exit criterion, such as the minimum image build that always includes state-lock-plan.json, so the legacy container-helper path can be removed.Source: Path instructions
src/lib/shields/state-dir-lock.test.ts (1)
205-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection reason per case.
The regex
/installed state lock plan|differs from the current agent manifest/matches every message thatparseInstalledPlanandplansMatchproduce. The three cases therefore cannot distinguish a JSON parse failure from an unknown field or a policy difference, so a regression that reports the wrong reason still passes. Assert a distinct expected message for each case.As per path instructions: "Flag ... conditionals that make a test pass without exercising its claim."
♻️ Per-case expectations
it.each([ - ["malformed JSON", "{"], - ["an unknown field", JSON.stringify({ ...PLAN, registry: [] })], - ["a different policy", JSON.stringify({ ...PLAN, readOnlyRoots: ["hooks"] })], - ])("rejects an installed plan with %s before mutation", (_case, payload) => { + ["malformed JSON", "{", /is not valid JSON/], + ["an unknown field", JSON.stringify({ ...PLAN, registry: [] }), /unknown fields: registry/], + [ + "a different policy", + JSON.stringify({ ...PLAN, readOnlyRoots: ["hooks"] }), + /differs from the current agent manifest/, + ], + ])("rejects an installed plan with %s before mutation", (_case, payload, expected) => { @@ - expect(stateLockPlanCompatibilityIssues(privileged, "/sandbox/.openclaw", PLAN)).toEqual([ - expect.stringMatching(/installed state lock plan|differs from the current agent manifest/), - ]); + expect(stateLockPlanCompatibilityIssues(privileged, "/sandbox/.openclaw", PLAN)).toEqual([ + expect.stringMatching(expected), + ]); });🤖 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 `@src/lib/shields/state-dir-lock.test.ts` around lines 205 - 222, Update the parameterized test around stateLockPlanCompatibilityIssues to include the expected rejection message for each payload case: malformed JSON must assert the parse-failure reason, the unknown field must assert the unknown-field reason, and the different policy must assert the manifest-difference reason. Replace the shared broad regex with the per-case expectation while preserving the existing pre-mutation setup.Source: Path instructions
scripts/state-dir-guard.py (2)
303-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet
strict=Falseexplicitly in_patterns_overlapand record the prefix semantics.
_patterns_overlapcompares only the shared-length prefix on purpose. It mirrorswritablePatternsOverlapinsrc/lib/agent/state-directory-contract.ts(Lines 202-212), soa/banda/b/ccount as overlapping. Ruff reports B905 here. A later contributor who silences B905 withstrict=Truewould convert plan validation into an unhandledValueErrorfor patterns of different length.♻️ Proposed change
def _patterns_overlap(first: tuple[str, ...], second: tuple[str, ...]) -> bool: + # Compare only the shared prefix: a shorter pattern that matches the head of + # a longer one still overlaps it. Never use strict=True here. return all( left == "*" or right == "*" or left == right - for left, right in zip(first, second) + for left, right in zip(first, second, strict=False) )🤖 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 `@scripts/state-dir-guard.py` around lines 303 - 307, Update _patterns_overlap to call zip with strict=False explicitly, preserving its intentional shared-prefix comparison for patterns of different lengths. Add a concise comment documenting that prefix semantics and the alignment with writablePatternsOverlap, without changing the overlap behavior.Source: Linters/SAST tools
2248-2277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind
planon every path inmain, and drop the unreachable-result safeguard.
planis assigned only when_load_plansucceeds. Line 2274 is reachable only whenresult is None, so the current code is correct at runtime. The binding is not visible to the reader or to analysis: the CodeQL check fails with "Local variable 'plan' may be used before it is initialized".Return early on each failure. That makes the plan binding explicit and removes the
RuntimeErrorsafeguard, which no branch can reach.♻️ Proposed restructure
+def _report(result: GuardResult) -> int: + for issue in result.issues: + print(json.dumps(issue.as_json(), sort_keys=True, separators=(",", ":"))) + print(json.dumps(result.summary_json(), sort_keys=True, separators=(",", ":"))) + return 0 if result.ok else 1 + + def main(argv: list[str] | None = None) -> int: args = _parse_args(sys.argv[1:] if argv is None else argv) - result: GuardResult | None = None try: plan = _load_plan(args) except PlanValidationError as exc: result = GuardResult(action=args.action) result.issues.append(Issue("invalid-plan", args.config_dir, str(exc))) - if result is None and os.geteuid() != 0: + return _report(result) + if os.geteuid() != 0: result = GuardResult(action=args.action) result.issues.append( Issue("root-required", args.config_dir, "state-dir guard must run as root") ) - elif result is None: - try: - identity = _production_identity() - except KeyError as exc: - result = GuardResult(action=args.action) - result.issues.append( - Issue( - "identity-unavailable", - args.config_dir, - f"required sandbox account is unavailable: {exc}", - ) - ) - else: - result = run_guard(args.action, args.config_dir, identity, plan) - - if result is None: # All branches above assign a result. - raise RuntimeError("state-dir guard did not produce a result") - for issue in result.issues: - print(json.dumps(issue.as_json(), sort_keys=True, separators=(",", ":"))) - print(json.dumps(result.summary_json(), sort_keys=True, separators=(",", ":"))) - return 0 if result.ok else 1 + return _report(result) + try: + identity = _production_identity() + except KeyError as exc: + result = GuardResult(action=args.action) + result.issues.append( + Issue( + "identity-unavailable", + args.config_dir, + f"required sandbox account is unavailable: {exc}", + ) + ) + return _report(result) + return _report(run_guard(args.action, args.config_dir, identity, plan))🤖 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 `@scripts/state-dir-guard.py` around lines 2248 - 2277, Restructure main so _load_plan and the prerequisite checks return their failure GuardResult immediately, leaving plan definitely bound before the production-identity and run_guard path. Remove the final result-is-None RuntimeError safeguard, while preserving the existing issue types, messages, and successful run_guard behavior.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.
Inline comments:
In `@docs/security/best-practices.mdx`:
- Around line 369-371: Update the Deep Agents lock-plan description so the first
declaration is named `agent`, not `agent/skills`, while retaining the separate
`skills` declaration and the existing behavior descriptions.
In `@docs/security/tcb-boundary.mdx`:
- Around line 139-142: Update the host wiring test coverage sentence in the
final-image validation section to say that host wiring tests “validate the
selection and plan handoff,” preserving the surrounding shared helper test
coverage wording.
In `@src/lib/agent/state-directory-contract.ts`:
- Around line 59-81: Update readWritableSubpaths to inspect the final path
component and reject any entry whose final component is "*", including the
single-component entry "*". Preserve the existing validation and duplicate
detection, while continuing to reject wildcard suffixes such as "runtime/*".
In `@test/snapshot-state-directory-contract.test.ts`:
- Around line 19-26: Remove both added if statements from
snapshot-state-directory-contract.test.ts: move the loadedSandboxState
type-narrowing guard into a shared test helper and reuse it from this test, then
restructure the it.each cases around the accepted value so each test path
performs a linear assertion without an if (!accepted) branch, using an expected
error value or separate accepted/rejected tables.
---
Outside diff comments:
In `@src/lib/shields/index.ts`:
- Around line 3318-3341: Clear planIssues in the catch block handling failures
around verify(...) so recovery guidance cannot use stale state-lock plan issues
when target resolution or verification throws. Keep driftIssues set to the
existing resolve-error message and ensure the recovery logic sees an empty
planIssues array for this failure path.
---
Nitpick comments:
In `@scripts/state-dir-guard.py`:
- Around line 303-307: Update _patterns_overlap to call zip with strict=False
explicitly, preserving its intentional shared-prefix comparison for patterns of
different lengths. Add a concise comment documenting that prefix semantics and
the alignment with writablePatternsOverlap, without changing the overlap
behavior.
- Around line 2248-2277: Restructure main so _load_plan and the prerequisite
checks return their failure GuardResult immediately, leaving plan definitely
bound before the production-identity and run_guard path. Remove the final
result-is-None RuntimeError safeguard, while preserving the existing issue
types, messages, and successful run_guard behavior.
In `@src/lib/shields/policy-transition.test.ts`:
- Around line 40-47: Update the stateLockPlan fixtures in the relevant test
cases to declare version as the literal type 1 using the existing `as const`
pattern, including both sibling fixtures. Keep all other fixture fields
unchanged.
In `@src/lib/shields/state-dir-lock.test.ts`:
- Around line 205-222: Update the parameterized test around
stateLockPlanCompatibilityIssues to include the expected rejection message for
each payload case: malformed JSON must assert the parse-failure reason, the
unknown field must assert the unknown-field reason, and the different policy
must assert the manifest-difference reason. Replace the shared broad regex with
the per-case expectation while preserving the existing pre-mutation setup.
In `@src/lib/shields/state-dir-lock.ts`:
- Around line 101-105: Update plansMatch to compare readOnlyRoots,
readOnlyPrefixes, and writableSubpaths without regard to entry order, using set
or sorted-copy semantics while preserving duplicate handling as appropriate.
Keep comparisons order-sensitive only where array order is semantically
meaningful, and continue returning false for actual policy differences.
- Around line 113-115: Replace the hard-coded directory comparisons in
hasImageRecoveryPlan with an explicit manifest-derived signal from the resolved
agent definition, such as whether the target declares a packaged plan. Pass that
flag through the callers and use it to select plan-aware behavior, so newly
generated plans and configuration-directory changes follow the manifest without
updating this function.
- Around line 252-279: The historical-image fallback in the runtime guard needs
an explicit, bounded retirement plan. Update the surrounding comment for the
historical branch in the state-dir guard to link the retirement issue or PR and
state the observable exit criterion, such as the minimum image build that always
includes state-lock-plan.json, so the legacy container-helper path can be
removed.
In `@test/repro-2681-group-writable.test.ts`:
- Around line 719-728: Replace the duplicated inline stateDirGuardAction
definition in the subprocess source with a serialized copy of the host helper
defined by stateDirGuardAction, so both subprocess and host assertions use
identical nullish-value behavior. Keep the existing subprocess constants and
invocation flow unchanged.
🪄 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: Enterprise
Run ID: 67d4fbd1-0b76-4d94-a53e-f4d1e13a2981
📒 Files selected for processing (65)
Dockerfileagents/hermes/Dockerfileagents/hermes/manifest.yamlagents/hermes/runtime-config-guard.pyagents/hermes/state-lock-plan.jsonagents/langchain-deepagents-code/manifest.yamlagents/openclaw/manifest.yamlagents/openclaw/state-lock-plan.jsonci/source-shape-test-budget.jsonci/test-file-size-budget.jsondocs/index.ymldocs/manage-sandboxes/backup-restore.mdxdocs/security/best-practices.mdxdocs/security/tcb-boundary.mdxpackage.jsonscripts/lib/generate-agent-state-lock-plans.mtsscripts/nemoclaw-start.shscripts/state-dir-guard.pysrc/lib/actions/sandbox/channel-status.test-helpers.tssrc/lib/actions/sandbox/wipe-state.tssrc/lib/agent/definition-types.tssrc/lib/agent/defs.test.tssrc/lib/agent/defs.tssrc/lib/agent/hermes-recovery-boundary-fixtures.tssrc/lib/agent/manifest-readers.tssrc/lib/agent/onboard.test.tssrc/lib/agent/runtime-auth-state-dirs.test.tssrc/lib/agent/runtime.test.tssrc/lib/agent/state-directory-contract.test.tssrc/lib/agent/state-directory-contract.tssrc/lib/onboard/verify-channel-runtime.test.tssrc/lib/sandbox/agent-config.test.tssrc/lib/sandbox/agent-config.tssrc/lib/sandbox/build-context.tssrc/lib/sandbox/config-get.test.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/legacy-hermes-compat.test.tssrc/lib/shields/openclaw-transition.test.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/state-dir-lock.test.tssrc/lib/shields/state-dir-lock.tssrc/lib/state/sandbox.tssrc/lib/state/user-managed-files-probe.test.tstest/destroy-wipe-sandbox-state.test.tstest/e2e/live/state-dir-guard-metadata.test.tstest/helpers/base-image-test-harness.tstest/helpers/shell-source.tstest/hermes-config-transaction-wiring.test.tstest/hermes-final-image-layout.test.tstest/hermes-runtime-config-guard.test.tstest/nemoclaw-start-locked-migration.test.tstest/nemoclaw-start.test.tstest/openclaw-config-transaction-wiring.test.tstest/openclaw-final-image-layout.test.tstest/package-contract/openshell-policy-boundary.test.tstest/rebuild-shields-auto-unlock.test.tstest/repro-2681-group-writable.test.tstest/sandbox-build-context.test.tstest/shields-up-runtime-perms.test.tstest/snapshot-runtime-auth-state.test.tstest/snapshot-state-directory-contract.test.tstest/snapshot.test.tstest/state-dir-guard.test.ts
💤 Files with no reviewable changes (1)
- src/lib/agent/runtime-auth-state-dirs.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/hermes-doctor-config-hash.test.ts (1)
16-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a real YAML parser in the config-hash test.
The hash command calls
_canonical_mcp_servers_digest, which callsyaml.safe_load. The stub returns{}for every input, so the test bypasses YAML parsing and validation. The fixture also lacksmcp_servers. Use the Hermes venv's PyYAML or a fixture-specific parser that asserts the expected mapping.🤖 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 `@test/hermes-doctor-config-hash.test.ts` around lines 16 - 43, The writeYamlStubPython test helper currently makes yaml.safe_load return an empty object, bypassing parsing and validation, and its fixture lacks mcp_servers. Update writeYamlStubPython to use the Hermes virtual environment’s real PyYAML, or a fixture-specific parser that validates and returns the expected mapping, and ensure the test configuration includes mcp_servers.Source: Path instructions
🤖 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 `@test/hermes-doctor-config-hash.test.ts`:
- Around line 16-43: The writeYamlStubPython test helper currently makes
yaml.safe_load return an empty object, bypassing parsing and validation, and its
fixture lacks mcp_servers. Update writeYamlStubPython to use the Hermes virtual
environment’s real PyYAML, or a fixture-specific parser that validates and
returns the expected mapping, and ensure the test configuration includes
mcp_servers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25d566b5-4a65-4686-b1c8-51d34adea2e5
📒 Files selected for processing (50)
agents/hermes/manifest.yamlagents/langchain-deepagents-code/manifest.yamlagents/openclaw/manifest.yamldocs/security/tcb-boundary.mdxscripts/lib/generate-agent-state-lock-plans.mtsscripts/state-dir-guard.pysrc/lib/actions/inference-set.test-support.tssrc/lib/actions/onboard.tssrc/lib/actions/sandbox/gateway-restart-hermes-drift.test.tssrc/lib/agent/definition-types.tssrc/lib/agent/defs.test.tssrc/lib/agent/defs.tssrc/lib/agent/hermes-recovery-boundary-fixtures.tssrc/lib/agent/manifest-readers.tssrc/lib/agent/onboard.test.tssrc/lib/agent/runtime.test.tssrc/lib/agent/state-directory-contract.test.tssrc/lib/agent/state-directory-contract.tssrc/lib/onboard/command-support.tssrc/lib/onboard/dockerfile-remote-dashboard-bind-contract.tssrc/lib/sandbox/agent-config.test.tssrc/lib/sandbox/agent-config.tssrc/lib/sandbox/hermes-dashboard-reseed.test.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/legacy-hermes-compat.test.tssrc/lib/shields/openclaw-transition.test.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/state-dir-lock.test.tssrc/lib/shields/state-dir-lock.tssrc/lib/shields/timer.tssrc/lib/state/sandbox.tssrc/lib/tunnel/allowed-origins.test.tstest/e2e/live/gateway-guard-recovery.test.tstest/e2e/live/hermes-shields-config.test.tstest/e2e/live/rebuild-hermes.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/state-backup-restore.test.tstest/helpers/base-image-test-harness.tstest/hermes-doctor-config-hash.test.tstest/repro-2681-group-writable.test.tstest/sandbox-provisioning-helper-permissions.test.tstest/sandbox-provisioning.test.tstest/sandbox-rlimit-hooks.test.tstest/shields-up-runtime-perms.test.tstest/snapshot-state-directory-contract.test.tstest/state-dir-guard.test.tstest/support/connect-flow-test-harness.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- src/lib/agent/onboard.test.ts
- src/lib/shields/openclaw-transition.test.ts
- agents/langchain-deepagents-code/manifest.yaml
- src/lib/shields/policy-transition.test.ts
- scripts/lib/generate-agent-state-lock-plans.mts
- src/lib/agent/hermes-recovery-boundary-fixtures.ts
- agents/hermes/manifest.yaml
- docs/security/tcb-boundary.mdx
- src/lib/shields/legacy-hermes-compat.test.ts
- src/lib/sandbox/agent-config.test.ts
- test/shields-up-runtime-perms.test.ts
- src/lib/shields/flow.test.ts
- src/lib/agent/state-directory-contract.test.ts
- test/helpers/base-image-test-harness.ts
- src/lib/shields/index.test.ts
- src/lib/agent/state-directory-contract.ts
- src/lib/shields/state-dir-lock.ts
- src/lib/state/sandbox.ts
- src/lib/agent/runtime.test.ts
- src/lib/agent/definition-types.ts
- src/lib/sandbox/agent-config.ts
- src/lib/shields/index.ts
- test/repro-2681-group-writable.test.ts
- scripts/state-dir-guard.py
- src/lib/shields/state-dir-lock.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head f6f0085. Manifest-derived state handling validates paths and overlaps, fails closed, preserves prior-agent semantics, and safely handles snapshot and image restoration. I found no blocking correctness, security, compatibility, or regression issue. The current automated test-fixture suggestion is non-blocking.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current revision after the mainline refresh. The previously reviewed functional commits are unchanged; the only new PR-owned change clarifies the state-guard trust boundary and accurately matches the fail-closed helper/plan selection behavior and its tests. Exact-head correctness, platform, and security checks pass. The remaining dependency-audit gate failures are from existing dependency graphs this PR does not modify, so they are not attributable to this change. No blocking correctness, security, compatibility, or regression defect found.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking on current head 445b9c6: the CLI no longer compiles. npm run build:cli fails with TS2554 at src/lib/shields/index.ts:2444 because the DeepAgents rollback path still calls restoreStateDirLockPosture with three arguments after the merged base changed that API to require five. This same compile error is failing build-typecheck, installer, CLI shards, CLI parity, macOS, and WSL. Please pass requireStateLockPlan(target) and target.stateLockPlanInImage at this call site, matching the other rollback paths, then rerun the build and focused Shields transition tests.
wscurran
added
area: architecture
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/state-dir-guard.py (1)
388-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the comprehension variable to avoid shadowing
value.The generator expression binds
valuefor each writable-subpath string while the outervaluestill holds the parsed plan object. The comprehension scope keeps the behavior correct, but the duplicate name makes the block harder to read and invites a mistake if this code later moves to aforloop.♻️ Proposed rename
writable_subpaths = tuple( - _validate_writable_subpath(value, f"writableSubpaths[{index}]") - for index, value in enumerate(writable_values) + _validate_writable_subpath(entry, f"writableSubpaths[{index}]") + for index, entry in enumerate(writable_values) )🤖 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 `@scripts/state-dir-guard.py` around lines 388 - 391, Rename the generator expression’s comprehension variable in the writable_subpaths assignment, and update its use in _validate_writable_subpath accordingly; preserve the outer parsed-plan value binding and existing validation behavior.
🤖 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 `@scripts/state-dir-guard.py`:
- Around line 388-391: Rename the generator expression’s comprehension variable
in the writable_subpaths assignment, and update its use in
_validate_writable_subpath accordingly; preserve the outer parsed-plan value
binding and existing validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d360b82e-12ad-49ce-983a-edecc847cc81
📒 Files selected for processing (11)
Dockerfileagents/hermes/Dockerfileci/source-shape-test-budget.jsonci/test-file-size-budget.jsondocs/index.ymldocs/manage-sandboxes/backup-restore.mdxdocs/security/best-practices.mdxdocs/security/tcb-boundary.mdxpackage.jsonscripts/nemoclaw-start.shscripts/state-dir-guard.py
🚧 Files skipped from review as they are similar to previous changes (9)
- scripts/nemoclaw-start.sh
- package.json
- ci/test-file-size-budget.json
- ci/source-shape-test-budget.json
- agents/hermes/Dockerfile
- docs/index.yml
- docs/manage-sandboxes/backup-restore.mdx
- docs/security/best-practices.mdx
- docs/security/tcb-boundary.mdx
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
4 participants
Summary
Reimplements #8006 by making each agent manifest the only state declaration.
loadAgent()validates that declaration and derives theAgentDefinitionprojections used by backup, restore, wipe, and Shields.This is the contract foundation for the dependent stack: #8010 (provider/root mutation boundary), then #8009 (generic staged restore), then a replacement for #7806 that synthesizes the useful guarantees from #7871 and #7880. The two existing #7806 PRs should remain open until replacement coverage is visible.
This stack does not promise a net source-line reduction. Its purpose is to replace divergent state authorities with one validated contract and make privileged state mutation safe across agent implementations. Current estimates are:
mainThe only presently defensible deletion estimate is therefore about 100–190 production lines. #8010 intersects the provider work in #7744 and durable receipt work in #7702; its scope and deletion estimate remain provisional until those owners approve or narrow the boundary. #7871 and #7880 together add 2,271 lines and delete 34 across production, tests, and documentation, but they are unmerged alternatives and are not counted as future deletions from
main.The current GitHub diff is +3,942/-727 across production, tests, documentation, and tooling.
The production increase establishes and verifies the shared contract before later PRs consume it. The largest additions are the 308-line TypeScript validation and derivation boundary and the descriptor-safe Python state guard extension. The rest replaces separate behavior in backup, restore, wipe, Shields, startup recovery, and image-version handling.
This PR does not add a policy database or handwritten registry. Each agent manifest contains the declaration, and
AgentDefinitionis the validated runtime authority. The generator callslistAgents()andloadAgent()instead of maintaining an agent list or parsing YAML separately. The generated OpenClaw and Hermes image plans total 51 lines. No code from closed #8084 was transferred.Related Issue
Fixes #8006
Parent epic: #8004
Stacked follow-ups: #8010, then #8009, then #7806
Changes
state_dirswith the independent facts used by current consumers: backup inclusion, Shields mode, declared prefixes, and writable subpaths.AgentDefinitionvalidate those declarations and derive backup, restore, wipe, and Shields projections.listAgents()andloadAgent().state_lock_plan_in_imagedeclares whether an agent image carries that projection.HIGH_RISK_STATE_DIRS,CONFIDENTIALITY_STATE_DIRS,runtime_auth_state_dirs, the fixedagents/*/sessionscarve-out, literalworkspace-*handling, and startup relock lists.AgentDefinitionand an installed current-image plan before a privileged mutation. Older images retain the bounded rebuild compatibility path.The existing backup, restore, wipe, and Shields paths are the consumers required by #8006. A direct change to one consumer would leave the others as separate authorities. The state-directory contract tests, snapshot contract tests, focused consumer tests, and live E2E targets protect the shared definition.
Type of Change
Quality Gates
nemoclaw-maintainer-security-code-reviewcompleted against refreshed base3f7097b4eee982bc6b86f61d9aea24643b12bbffand exact headd7bb3cd259cca72545d63c52cc758499cb5d7532(tree1fd369f090b0fe3aeabfa7b33414e9e6ed6c83a7). All nine categories PASS with no security findings or blocker; the PR Gate remains responsible for exact-head live E2E.Documentation Writer Review
docs-updateddocs/index.yml,docs/manage-sandboxes/backup-restore.mdx,docs/security/best-practices.mdx, anddocs/security/tcb-boundary.mdx; verified OpenClaw, Hermes, and Deep Agents variants against the implementation; exact-head validation passed 1,909 E2E-support tests and 59 state-guard integration tests;npm run validate:prpassed;npm run docspassed with 0 errors and the same 2 unrelated Fern warnings;git diff --checkpassed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all 17 PR commits report valid verification.pre-commit,commit-msg, andpre-pushhooks passed.npm run typecheck:clipassed.npm --prefix nemoclaw run buildpassed.npm run build:clipassed.npm run source-shape:checkpassed.npm run validate:prpassed on exact headd7bb3cd25.npm run docspassed with 0 errors and 2 unchanged Fern warnings.npm run docsbuilds without warnings (doc changes only) — completed with zero errors and two Fern warnings.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
.env.