refactor(hermes): define path ownership contract - #8084
Closed
jyaunches wants to merge 3 commits into
Closed
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
📝 WalkthroughWalkthroughChangesThe PR adds Hermes filesystem ownership contracts, managed-artifact metadata, topology-aware posture resolution, backup and restore rules, migration handling, path matching, and extensive lifecycle tests. Hermes path ownership
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant findHermesManagedArtifact
participant HermesHomeResolution
participant HermesManagedArtifactCatalog
Caller->>findHermesManagedArtifact: absolute path
findHermesManagedArtifact->>HermesHomeResolution: identify applicable home
HermesHomeResolution-->>findHermesManagedArtifact: home and relative path
findHermesManagedArtifact->>HermesManagedArtifactCatalog: match artifact contract
HermesManagedArtifactCatalog-->>findHermesManagedArtifact: artifact and path role
findHermesManagedArtifact-->>Caller: resolved artifact or null
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 2e40195 in the TypeScript / code-coverage/cliThe overall coverage in commit 2e40195 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
8 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
7 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 5 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/lib/agent/hermes-path-ownership.ts (1)
3599-3653: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache pattern validation and compiled segment regexes.
matchesHermesRelativePatterncallsvalidateHermesRelativePatternon every invocation.findHermesManagedArtifactcalls it once per pattern contract per lookup.matchesPatternSegmentalso builds a newRegExpfor every segment comparison. The catalog is a module-level constant, so both results are stable per pattern.Memoize the validated pattern set and the compiled segment regexes in module-level
Mapinstances. This removes repeated validation and regex compilation from each lookup.Note on the static analysis hints for Line 3611: the regex source comes only from catalog literals that
validateHermesRelativePatternalready rejects when they contain*outside a**segment. Untrusted input reaches the regex input string, not the regex source. Caching the compiled regex also bounds repeated construction.♻️ Proposed caching of validation and compiled regexes
+const VALIDATED_PATTERNS = new Set<string>(); +const SEGMENT_MATCHERS = new Map<string, RegExp | null>(); + function matchesPatternSegment(patternSegment: string, candidateSegment: string): boolean { + let matcher = SEGMENT_MATCHERS.get(patternSegment); + if (matcher === undefined) { const placeholders = [...patternSegment.matchAll(/\{[^{}]+\}/gu)]; - if (placeholders.length === 0) return patternSegment === candidateSegment; - + if (placeholders.length === 0) { + SEGMENT_MATCHERS.set(patternSegment, null); + matcher = null; + } else { let expression = "^"; let cursor = 0; for (const placeholder of placeholders) { const index = placeholder.index ?? 0; expression += patternSegment.slice(cursor, index).replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); expression += ".+"; cursor = index + placeholder[0].length; } expression += patternSegment.slice(cursor).replace(/[.*+?^${}()|[\]\\]/gu, "\\$&") + "$"; - return new RegExp(expression, "u").test(candidateSegment); + matcher = new RegExp(expression, "u"); + SEGMENT_MATCHERS.set(patternSegment, matcher); + } + } + if (matcher === null) return patternSegment === candidateSegment; + return matcher.test(candidateSegment); } function matchesHermesRelativePattern(pattern: string, candidate: string): boolean { - validateHermesRelativePattern(pattern); + if (!VALIDATED_PATTERNS.has(pattern)) { + validateHermesRelativePattern(pattern); + VALIDATED_PATTERNS.add(pattern); + }🤖 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/agent/hermes-path-ownership.ts` around lines 3599 - 3653, Add module-level caches for validated Hermes relative patterns and compiled segment regular expressions, then update matchesHermesRelativePattern to reuse the validation result and matchesPatternSegment to reuse compiled regexes for each pattern segment. Preserve exact-match handling for segments without placeholders and ensure cache keys distinguish different pattern strings or segments.Source: Linters/SAST tools
src/lib/agent/hermes-path-ownership.test.ts (3)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant type assertion.
HERMES_MANAGED_ARTIFACTSis already typed as a readonly array ofHermesManagedArtifact. Theas readonly HermesManagedArtifact[]cast at Line 64 adds no information. A cast also hides a future contract change from the type checker.♻️ Proposed simplification
- for (const entry of HERMES_MANAGED_ARTIFACTS as readonly HermesManagedArtifact[]) { + for (const entry of HERMES_MANAGED_ARTIFACTS) {🤖 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/agent/hermes-path-ownership.test.ts` around lines 63 - 64, Remove the redundant readonly HermesManagedArtifact[] type assertion from the iteration over HERMES_MANAGED_ARTIFACTS, leaving the existing loop and Set logic unchanged so the array’s declared type remains enforced by the compiler.
49-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit this test into separate cases.
This single test covers identity resolution, presence, path shape, lifecycle postures, shield requirements, backup and restore pairing, target uniqueness, and migration sources. The title names two of those claims. A failure in any branch reports one test.
The coding guidelines require low function complexity. Splitting also removes most of the conditionals that the growth guardrail rejects.
Suggested split, one test per invariant:
- producers and readers resolve for every topology
- create posture owner matches the producer
- lifecycle postures use valid modes
- backup and restore stay consistent
- concrete target paths stay unique and resolve back to the artifact
- migration sources resolve with
pathRoleset tomigration-sourceAs per coding guidelines, "Keep function complexity low".
🤖 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/agent/hermes-path-ownership.test.ts` around lines 49 - 158, Split the monolithic test around HERMES_MANAGED_ARTIFACTS into focused cases for identity resolution, create-owner matching, lifecycle posture validity, backup/restore consistency, target uniqueness and resolution, and migration-source resolution. Move each related assertion block into its corresponding test, preserving the existing topology and artifact coverage while keeping each test’s branching and complexity low.Sources: Coding guidelines, Pipeline failures
85-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the property directly instead of collapsing it to a boolean.
These assertions reduce a compound condition to
trueorfalse. Two problems follow.
- The failure message shows only
expected true/false. It does not show the actualpresence,relativePath,backup, orrestorevalue.- Lines 89-97 pass for every
agent-homeartifact because theentry.scope !== "agent-home"conjunct is alreadyfalse. The intended rule, that onlyagent-homeartifacts carry per-homebackupandrestorerules, is never checked for those artifacts.Assert the observable values instead.
♻️ Proposed assertions
- expect(entry.presence === "required" || entry.presence === "optional", entry.id).toBe(true); - expect(typeof entry.relativePath === "string" || entry.scope === "agent-home", entry.id).toBe( - true, - ); - expect( - typeof entry.backup === "object" && - "default" in entry.backup && - entry.scope !== "agent-home", - entry.id, - ).toBe(false); - expect(typeof entry.restore === "object" && entry.scope !== "agent-home", entry.id).toBe( - false, - ); + expect([entry.presence, entry.id]).toEqual([expect.stringMatching(/^(required|optional)$/u), entry.id]); + const homeKeyedBackup = typeof entry.backup === "object" && "default" in entry.backup; + const homeKeyedRestore = typeof entry.restore === "object"; + expect({ id: entry.id, homeKeyedBackup, homeKeyedRestore }).toEqual({ + id: entry.id, + homeKeyedBackup: homeKeyedBackup && entry.scope === "agent-home", + homeKeyedRestore: homeKeyedRestore && entry.scope === "agent-home", + });Apply the same change to Line 125 and Line 137.
As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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/agent/hermes-path-ownership.test.ts` around lines 85 - 97, Update the assertions in the artifact validation test, including the checks near lines 125 and 137, to assert each observable property directly rather than converting compound conditions to booleans. Validate presence and relativePath against their expected values, and assert backup and restore are present only for agent-home artifacts while non-agent-home artifacts have the expected absence; preserve the existing entry.id context in failure messages.Source: Path instructions
src/lib/agent/hermes-path-ownership-lifecycle.test.ts (4)
266-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the expected
artifactClassper artifact.Line 269 accepts three of the five artifact classes for both
tool-homeandkanban-state. The assertion passes if either artifact changes class within that set. The test title claims the state stays writable, so the class matters.♻️ Proposed per-artifact expectation
- for (const id of ["tool-home", "kanban-state"]) { - const entry = artifact(id); - expect(entry.artifactClass, id).toMatch( - /^(credential-reference|durable-state|mutable-runtime-state)$/u, - ); + for (const [id, artifactClass] of [ + ["tool-home", "credential-reference"], + ["kanban-state", "durable-state"], + ] as const) { + const entry = artifact(id); + expect(entry.artifactClass, id).toBe(artifactClass);Replace the placeholder values with the declared classes.
🤖 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/agent/hermes-path-ownership-lifecycle.test.ts` around lines 266 - 275, Update the test case around artifact() so each artifact uses its declared artifactClass explicitly: expect tool-home and kanban-state to their respective fixed classes instead of accepting a shared regular expression. Keep the existing writable-shield and gateway producer/reader assertions unchanged.Source: Path instructions
395-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the narrowing guard before the assertion.
Line 400 uses an optional chain, so
resolvedcan beundefined. Line 401 asserts the id and fails first. Line 402 then throws. The guard is unreachable on the failure path, and the thrown message never appears.Place the guard first, then assert.
♻️ Proposed reordering
- const resolved = findHermesManagedArtifact(target)?.artifact; - expect(resolved?.id, target).toBe(id); - if (!resolved) throw new Error("Missing Hermes artifact for '" + target + "'"); - const posture = resolveHermesPosture(resolved.required.create, "root-separated", "gateway"); + const resolution = findHermesManagedArtifact(target); + expect(resolution?.artifact.id, target).toBe(id); + const resolved = artifact(id); + const posture = resolveHermesPosture(resolved.required.create, "root-separated", "gateway");The
artifact(id)helper already throws a named error, so the local guard is not needed. This also removes one branch that the growth guardrail counts.🤖 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/agent/hermes-path-ownership-lifecycle.test.ts` around lines 395 - 405, Update the loop using findHermesManagedArtifact so the missing-artifact narrowing occurs before accessing or asserting resolved.id, ensuring the intended error is raised first; preferably reuse the existing artifact(id) helper’s named error and remove the redundant local guard if applicable.Source: Pipeline failures
1014-1089: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis assertion copies the production constant verbatim.
HERMES_CONTRACT_GAPSis asserted against a literal that duplicatessrc/lib/agent/hermes-path-ownership.tslines 206-280 field for field. The test passes whenever the two literals stay in sync. It proves no behavior. Any contract edit requires the same edit here, which invites a mechanical copy of the new value.Assert the properties that carry meaning instead. Examples:
- every gap id is unique
- every
targetArtifactIdsentry resolves throughartifact(id)- every
failurevalue isretain-source- every
currentPathsentry resolves to a managed artifact or a declared migration sourceThat set fails when a gap points at a removed artifact. The current literal comparison does not.
The same pattern appears in
src/lib/agent/hermes-path-ownership.test.tsat Lines 264-321 forHERMES_UNSUPPORTED_RESIDUAL_PATHS. That file already pairs the literal withfindHermesManagedArtifactassertions at Lines 322-334, which is the stronger form.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/agent/hermes-path-ownership-lifecycle.test.ts` around lines 1014 - 1089, The test should stop comparing HERMES_CONTRACT_GAPS to a copied production literal and instead validate its semantic invariants: unique gap ids, resolvable targetArtifactIds via artifact, retain-source failure values, and valid managed-artifact or declared migration-source currentPaths. Apply the same principle to HERMES_UNSUPPORTED_RESIDUAL_PATHS in hermes-path-ownership.test.ts, reusing its existing findHermesManagedArtifact assertions rather than duplicating production data.Source: Path instructions
956-982: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the manifest assertions order-independent.
agents/hermes/manifest.yamlis the contract source and currently matches the test. Backup and restore do not requirestateDirsorstateFilesorder, so compare paths and strategies without fixing sequence order.🤖 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/agent/hermes-path-ownership-lifecycle.test.ts` around lines 956 - 982, Update the manifest assertions in the test “records current manifest gaps separately from the target backup contract (`#8006`)” to compare stateDirs and stateFiles order-independently, while still requiring the same directory paths and file path/strategy pairs. Preserve the manifest contract values without enforcing their sequence.Source: Learnings
🤖 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 `@src/lib/agent/hermes-path-ownership-lifecycle.test.ts`:
- Around line 728-747: Update the table-driven expectations in the artifact
ownership test so each staging path entry explicitly provides its expected
shields value. Replace the conditional classification based on id prefixes and
special ids near the stagingPaths assertions, and compare against that per-entry
value while preserving the existing artifact metadata checks and growth
guardrail behavior.
In `@src/lib/agent/hermes-path-ownership.test.ts`:
- Around line 106-120: Remove all if statements from the changed test bodies and
local helpers in src/lib/agent/hermes-path-ownership.test.ts (including the
anchor range 106-120 and the cited ranges 72, 74-78, 120, 127, 152, and 560) by
splitting posture-kind expectations into separate tests and moving other
branches into named narrowing helpers; in
src/lib/agent/hermes-path-ownership-lifecycle.test.ts (346-406, plus 435 and
478), replace narrowing guards with a shared helper that throws and returns the
narrowed value. Move the conditional artifact helper from both files into one
shared non-test module and reuse it.
---
Nitpick comments:
In `@src/lib/agent/hermes-path-ownership-lifecycle.test.ts`:
- Around line 266-275: Update the test case around artifact() so each artifact
uses its declared artifactClass explicitly: expect tool-home and kanban-state to
their respective fixed classes instead of accepting a shared regular expression.
Keep the existing writable-shield and gateway producer/reader assertions
unchanged.
- Around line 395-405: Update the loop using findHermesManagedArtifact so the
missing-artifact narrowing occurs before accessing or asserting resolved.id,
ensuring the intended error is raised first; preferably reuse the existing
artifact(id) helper’s named error and remove the redundant local guard if
applicable.
- Around line 1014-1089: The test should stop comparing HERMES_CONTRACT_GAPS to
a copied production literal and instead validate its semantic invariants: unique
gap ids, resolvable targetArtifactIds via artifact, retain-source failure
values, and valid managed-artifact or declared migration-source currentPaths.
Apply the same principle to HERMES_UNSUPPORTED_RESIDUAL_PATHS in
hermes-path-ownership.test.ts, reusing its existing findHermesManagedArtifact
assertions rather than duplicating production data.
- Around line 956-982: Update the manifest assertions in the test “records
current manifest gaps separately from the target backup contract (`#8006`)” to
compare stateDirs and stateFiles order-independently, while still requiring the
same directory paths and file path/strategy pairs. Preserve the manifest
contract values without enforcing their sequence.
In `@src/lib/agent/hermes-path-ownership.test.ts`:
- Around line 63-64: Remove the redundant readonly HermesManagedArtifact[] type
assertion from the iteration over HERMES_MANAGED_ARTIFACTS, leaving the existing
loop and Set logic unchanged so the array’s declared type remains enforced by
the compiler.
- Around line 49-158: Split the monolithic test around HERMES_MANAGED_ARTIFACTS
into focused cases for identity resolution, create-owner matching, lifecycle
posture validity, backup/restore consistency, target uniqueness and resolution,
and migration-source resolution. Move each related assertion block into its
corresponding test, preserving the existing topology and artifact coverage while
keeping each test’s branching and complexity low.
- Around line 85-97: Update the assertions in the artifact validation test,
including the checks near lines 125 and 137, to assert each observable property
directly rather than converting compound conditions to booleans. Validate
presence and relativePath against their expected values, and assert backup and
restore are present only for agent-home artifacts while non-agent-home artifacts
have the expected absence; preserve the existing entry.id context in failure
messages.
In `@src/lib/agent/hermes-path-ownership.ts`:
- Around line 3599-3653: Add module-level caches for validated Hermes relative
patterns and compiled segment regular expressions, then update
matchesHermesRelativePattern to reuse the validation result and
matchesPatternSegment to reuse compiled regexes for each pattern segment.
Preserve exact-match handling for segments without placeholders and ensure cache
keys distinguish different pattern strings or segments.
🪄 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: f3f1e4a4-434d-41b3-a00c-3bd398a2e7ee
📒 Files selected for processing (3)
src/lib/agent/hermes-path-ownership-lifecycle.test.tssrc/lib/agent/hermes-path-ownership.test.tssrc/lib/agent/hermes-path-ownership.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/agent/hermes-path-ownership.test.ts (1)
35-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
homesForandhomesForResidual.
homesForandhomesForResidualimplement the same scope-to-home mapping. The only difference is thathomesForResidualomits the"dashboard"case, since residual paths never carry that scope.Extract the shared cases into one function and have
homesForResidualcall it after excluding"dashboard", or build a lookup table keyed by scope shared by both functions. This removes the duplicate mapping and avoids a case falling out of sync if a new scope value is added to only one function.♻️ Example consolidation
-function homesFor(artifactRule: HermesManagedArtifact): HermesHome[] { - switch (artifactRule.scope) { - case "agent-home": - return [{ kind: "default" }, { kind: "named-profile", name: "research" }]; - case "default-home": - return [{ kind: "default" }]; - case "dashboard": - return [{ kind: "dashboard" }]; - case "named-profile": - return [{ kind: "named-profile", name: "research" }]; - } -} - -function homesForResidual(residual: HermesUnsupportedResidual): HermesHome[] { - switch (residual.scope) { - case "agent-home": - return [{ kind: "default" }, { kind: "named-profile", name: "research" }]; - case "default-home": - return [{ kind: "default" }]; - case "named-profile": - return [{ kind: "named-profile", name: "research" }]; - } -} +function homesForScope( + scope: "agent-home" | "default-home" | "named-profile", +): HermesHome[] { + switch (scope) { + case "agent-home": + return [{ kind: "default" }, { kind: "named-profile", name: "research" }]; + case "default-home": + return [{ kind: "default" }]; + case "named-profile": + return [{ kind: "named-profile", name: "research" }]; + } +} + +function homesFor(artifactRule: HermesManagedArtifact): HermesHome[] { + return artifactRule.scope === "dashboard" + ? [{ kind: "dashboard" }] + : homesForScope(artifactRule.scope); +} + +function homesForResidual(residual: HermesUnsupportedResidual): HermesHome[] { + return homesForScope(residual.scope); +}🤖 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/agent/hermes-path-ownership.test.ts` around lines 35 - 57, Consolidate the duplicated scope-to-home mapping in homesFor and homesForResidual by introducing one shared mapping helper or lookup table. Preserve the dashboard mapping for HermesManagedArtifact, while ensuring homesForResidual handles only non-dashboard scopes through the shared implementation and remains type-safe if new scope values are added.
🤖 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 `@src/lib/agent/hermes-path-ownership.test.ts`:
- Around line 35-57: Consolidate the duplicated scope-to-home mapping in
homesFor and homesForResidual by introducing one shared mapping helper or lookup
table. Preserve the dashboard mapping for HermesManagedArtifact, while ensuring
homesForResidual handles only non-dashboard scopes through the shared
implementation and remains type-safe if new scope values are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5041513f-0b49-4775-8f32-cf7286c87941
📒 Files selected for processing (3)
src/lib/agent/hermes-path-ownership-lifecycle.test.tssrc/lib/agent/hermes-path-ownership.test.tssrc/lib/agent/hermes-path-ownership.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/agent/hermes-path-ownership-lifecycle.test.ts
- src/lib/agent/hermes-path-ownership.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Collaborator
Security and product-scope review — PASSReviewed commit SHA VerdictPASS. No security findings. Issue #8006 explicitly sequences the typed ownership model and characterization tests as delivery step 1, before production consumers migrate in later steps. This pull request adds only that contract and its tests. Repository search found no production import or caller, so the change does not create a supported integration or alter current runtime behavior. The three changed-file blobs are byte-identical to commit FindingsNo findings. Detailed analysis
Product-scope and review evidence
Files reviewed
|
cv
approved these changes
Aug 3, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Approved for commit 2e40195. Issue #8006 authorizes this contract-only step. The security review passed, the independent documentation writer review found no documentation update was needed, DCO is present, all commits are GitHub Verified, no review threads remain, and the required GitHub checks pass for this commit.
6 tasks
22 tasks
23 tasks
jyaunches
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary Reimplements [#8006](#8006) by making each agent manifest the only state declaration. `loadAgent()` validates that declaration and derives the `AgentDefinition` projections used by backup, restore, wipe, and Shields. This is the contract foundation for the dependent stack: [#8010](#8010) (provider/root mutation boundary), then [#8009](#8009) (generic staged restore). Merged [#7871](#7871) now supplies the cron restore-drain guarantee; this head preserves it while integrating the manifest-derived state contract. [#7880](#7880) and [#7806](#7806) are closed and are not part of the remaining stack. 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: | Follow-up | Estimated production additions | Estimated production lines replaced or deleted from current `main` | | --- | ---: | ---: | | #8010 | About 520–790 for the registered-sandbox slice; 600–930 if created/rebuild flows are included | Pending the owner-approved provider and durable-receipt boundary; it must replace a named existing mutation path before merge | | #8009 | About 530 | About 100–160 | The only presently defensible deletion estimate is therefore about 100–190 production lines. #8010 intersects the provider work in [#7744](#7744) and durable receipt work in [#7702](#7702); its scope and deletion estimate remain provisional until those owners approve or narrow the boundary. [#7871](#7871) is merged and incorporated in this head; [#7880](#7880) is closed. Neither is counted as a future deletion from `main`. The current GitHub diff is +4,847/-948 across 105 production, test, documentation, and tooling files. The production increase establishes and verifies the shared contract before later PRs consume it. The largest additions are the 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 `AgentDefinition` is the validated runtime authority. The generator calls `listAgents()` and `loadAgent()` instead of maintaining an agent list or parsing YAML separately. No code from closed [#8084](#8084) was transferred. ## Related Issue Fixes #8006 Parent epic: #8004 Stacked follow-ups: #8010, then #8009 ## Changes - Extends `state_dirs` with the independent facts used by current consumers: backup inclusion, Shields mode, declared prefixes, and writable subpaths. - Makes `AgentDefinition` validate those declarations and derive backup, restore, wipe, and Shields projections. - Generates each image state-lock plan through `listAgents()` and `loadAgent()`. `state_lock_plan_in_image` declares whether an agent image carries that projection. - Removes `HIGH_RISK_STATE_DIRS`, `CONFIDENTIALITY_STATE_DIRS`, `runtime_auth_state_dirs`, the fixed `agents/*/sessions` carve-out, literal `workspace-*` handling, and startup relock lists. - Rejects drift between the current `AgentDefinition` and an installed current-image plan before a privileged mutation. Older images retain the bounded rebuild compatibility path. - Makes backup discovery and restore authorization fail closed against the target agent definition, including prefix matches and non-backup authentication state. - Routes locked OpenClaw migration through the existing configuration and state-directory guards so a failed relock remains retryable. - Preserves merged Hermes dashboard-profile, cron-restore, truthful Shields rollback, and packaged-service teardown behavior from #7200, #7871, #8198, and #8239. - Corrects the merged #7871 rebuild E2E fixture to read the pinned flat Hermes cron record, reject schema drift, and require newly seeded jobs to be scheduled, then exercises stranded-gate recovery with an overdue one-shot job from the built-in scheduler and validates exactly one post-recovery execution through the pinned Hermes ledger. - Treats the immediate post-restart Hermes `running_pid: null` evidence as a bounded polling transient in the E2E fixture while keeping every other gateway field strict; readiness still requires a new live process identity. - Validates the #7871 stranded-gate receipt after the E2E redaction boundary, accepting only the exact redaction sentinel while separately pinning the raw 32-character token contract. - Keeps the Deep Agents Code live validation lane deterministic with bounded retries for transient status-health failures. That lane is in scope because #8006 changes and validates the Deep Agents manifest alongside OpenClaw and Hermes; the retry remains fail-closed after three attempts and does not change runtime product behavior. - Limits the Dockerfile delta to packaging and protecting the generated OpenClaw state-lock plan; no digest allowlist change remains in the current diff. - Updates security documentation and adds contract, permission, backup, restore, wipe, image-layout, version-skew, and live E2E coverage. 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 - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: the canonical `nemoclaw-maintainer-security-code-review` rubric was applied against exact base `3b208d79e5d3bda4183704145ee5c28d79876ae1` and head `14c334aacfa1897633e5c1b383977a4333a24c1f`. All nine categories PASS with no findings or warnings across all 105 changed files. The review covered secrets, input validation, authorization, dependencies, error handling, data protection, configuration, security tests, and system-level stale-state/recovery/TOCTOU behavior. The nullable E2E PID cannot satisfy readiness and only continues a bounded poll. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: none accepted; required GitHub checks and live E2E remain required before merge. ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed the complete 105-file PR patch against the manifest-derived state contract, snapshot authorization, per-agent Shields plans, historical compatibility, Deep Agents host injection, and the Hermes dashboard carve-out at exact head `14c334aacfa1897633e5c1b383977a4333a24c1f` against base `3b208d79e5d3bda4183704145ee5c28d79876ae1`. The final Hermes restart-transient fixture change is E2E-only and requires no additional user documentation. `npm run docs` passed with 0 errors and 2 existing Fern warnings; route validation passed for 67 guarded pages; `npm run validate:pr` passed. - Agent: Codex Desktop (Writing Style Guide fallback; DORI unavailable) <!-- docs-review-head-sha: 14c334a --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub — all 61 PR commits report valid verification. - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above: - The merged #7871/#8198/#8239 regression set passed 17 files and 407 tests. - The Hermes cron-state, redacted-receipt, schedule, execution-ledger, and restart-gateway-evidence regressions passed 35 focused E2E-support tests; the controller producer suite passed 21 tests; the full E2E-support project passed 193 files and 2,056 tests. - The latest-main rebuild-preflight regression set passed 4 files and 27 tests. - The final #8371 main sync passed 3 CLI files and 96 tests plus 1 integration file and 20 tests on the combined tree. - The canonical #8372 security-rubric and Advisor contract suite passed 6 files and 70 tests. - The production change set passed 399 files and 5,047 tests in the earlier affected-test run; the final fixture-only delta is covered by the exact-head focused and full E2E-support runs above. - The merge-only target fixtures passed 50 focused tests and `npm run typecheck:cli`. - `npm run checks:repository` passed. - `npm run validate:pr` passed on exact head `14c334aac`. - `npm run docs` passed on exact head `14c334aac` with 0 errors and 2 existing Fern warnings; routes and generated variants are current. - Independent exact-head maintainer, documentation-writer, nine-category security, and cross-issue reviews passed with no findings or merge blocker. - [ ] Applicable broad gate passed — exact-head required CI and live E2E remain required. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — completed with zero errors and two Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [x] New doc pages include SPDX header and frontmatter (new pages only) — not applicable; no new documentation page was added. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defines one typed Hermes path ownership contract for configuration, credentials, runtime state, durable state, and disposable artifacts. This is the contract-only first step of #8006; no production consumer changes in this PR.
The follow-up stack is:
Estimated follow-up simplification: the currently measurable follow-up work is expected to delete roughly 650–900 existing production lines. Contract-driven executors and adapters are expected to add about 350–500 production lines, leaving an estimated net reduction of 200–450 production lines. This estimate excludes tests, the contract added here, and compatibility code that cannot be retired until its support window closes; it also does not credit later #8010 or #8007 removals that are not yet measurable.
Related Issue
Part of #8006
Changes
Type of Change
Quality Gates
2e40195291c17ab3db332f019daa3c99b8de152cagainst base SHA4cd4d64fe67143b57707f874afa0b9d269dfeff2.Documentation Writer Review
no-docs-needed2e4019529found the same three internal contract/test files byte-identical to commit04430e2, with no production consumer. No user-facing documentation or Fern change is required.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run test:changed: 2 files and 44 tests passed;npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable to a contract-only source change with focused coverage.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
New Features
Tests