chore(architecture): map and characterize onboarding lifecycle contracts (#6225) - #6259
Conversation
📝 WalkthroughWalkthroughAdds a lifecycle-contracts.md document mapping onboarding/runtime lifecycle journeys, states, ownership, and known gaps, cross-linked from related READMEs. Adds a new transition-traces.test.ts characterization suite pinning FSM event traces, and extends onboard-session tests for secret-boundary persistence (migratedLegacyValueHashes, credentialEnv). ChangesLifecycle contract documentation
Characterization test suites
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/onboard-session-secret-invariants.test.ts (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommonJS
require()violates the ESM-only test guideline.Lines 41 and 51-52 use
createRequire/require()to defer module loading until after theHOMEreassignment. A top-levelawait import(...)achieves the same deferred-load ordering (it isn't hoisted like a static import) while staying ESM-only, as required for files undertest/.♻️ Proposed fix using dynamic import
-import { createRequire } from "node:module"; import os from "node:os"; import path from "node:path"; import { afterAll, beforeEach, describe, expect, it } from "vitest"; import type { Session } from "../src/lib/state/onboard-session"; -const require = createRequire(import.meta.url); - // SESSION_DIR/SESSION_FILE are derived from HOME at module-load time, so the // temp HOME must be in place before the session module loads. These CJS -// requires (via the integration project's source-require hook) execute here, -// after the assignment — unlike a hoisted static import. +// dynamic imports execute here, after the assignment — unlike a hoisted +// static import. const originalHome = process.env.HOME; const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-session-secret-")); process.env.HOME = tempHome; -const session: typeof import("../src/lib/state/onboard-session") = require("../src/lib/state/onboard-session"); -const stepMutation: typeof import("../src/lib/state/onboard-step-mutation") = require("../src/lib/state/onboard-step-mutation"); +const session: typeof import("../src/lib/state/onboard-session") = await import( + "../src/lib/state/onboard-session" +); +const stepMutation: typeof import("../src/lib/state/onboard-step-mutation") = await import( + "../src/lib/state/onboard-step-mutation" +);As per coding guidelines, "Files under
test/must use ESM imports/exports."🤖 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/onboard-session-secret-invariants.test.ts` around lines 41 - 52, The test currently uses createRequire/require() to delay loading onboard-session and onboard-step-mutation until after HOME is reassigned, but that breaks the ESM-only test rule. Replace the CommonJS require-based loading in this test with deferred dynamic imports using top-level await so the modules still load after tempHome is set. Keep the ordering behavior intact by importing the same symbols from ../src/lib/state/onboard-session and ../src/lib/state/onboard-step-mutation via await import(), and remove the createRequire usage entirely.Source: Coding guidelines
test/onboard-lifecycle-invariants.test.ts (1)
92-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSource-text pinning conflicts with the test-review path instruction, though the rationale is documented.
This section indexes raw source text of
onboard.tsand asserts ordering via string offsets (markerIndex), rather than exercisingcreateSandboxthrough its public boundary. The file's own comment (lines 12-17) explains this is deliberate becausecreateSandboxisn't hermetically testable, but per path instructions for test files, source-text assertions are exactly the pattern to avoid where an observable-outcome alternative exists.Any harmless refactor inside
createSandbox(e.g., reordering an unrelated log line, renaming a local, or reformatting a call) that doesn't touch the described lifecycle ordering could still break these markers/exact snippet matches, making the tests brittle beyond their intended contract. If a longer-term fix is feasible, consider extracting the ordering-relevant steps behind a small internal "step recorder" injectable inonboard.ts(mirroring the pattern already used forsandbox-messaging-preflight,sandbox-create-plan, etc.), so this ordering could be pinned via recorded events instead of literal source-text matching.Separately, the static-analysis path-traversal hint on line 92 is a false positive — the path is built from
import.meta.dirnameplus fixed literal segments, not external input.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 `@test/onboard-lifecycle-invariants.test.ts` around lines 92 - 185, The test is pinning raw source text and exact string offsets inside createSandbox, which makes it brittle and conflicts with the test guidance to prefer observable outcomes. Update onboard-lifecycle-invariants.test.ts to validate the lifecycle through the public boundary or a small injectable step-recorder in createSandbox rather than scanning onboard.ts source text with markerIndex and exact snippet matches. Keep the ordering contract, but assert recorded events/calls from createSandbox, sandboxCreatePlan, sandboxMessagingPreflight, streamSandboxCreate, and sandboxRegistration instead of literal source ordering.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.
Inline comments:
In `@src/lib/onboard/machine/transition-traces.test.ts`:
- Around line 88-92: The updateSession helper in transition-traces.test.ts is
falling back to the outer session when mutator returns void, which drops
in-place edits made on the cloned draft. Update the updateSession function so it
uses the cloned draft as the fallback result instead of session, preserving both
OnboardRuntimeDeps.updateSession call patterns while still cloning the stored
session before returning.
---
Nitpick comments:
In `@test/onboard-lifecycle-invariants.test.ts`:
- Around line 92-185: The test is pinning raw source text and exact string
offsets inside createSandbox, which makes it brittle and conflicts with the test
guidance to prefer observable outcomes. Update
onboard-lifecycle-invariants.test.ts to validate the lifecycle through the
public boundary or a small injectable step-recorder in createSandbox rather than
scanning onboard.ts source text with markerIndex and exact snippet matches. Keep
the ordering contract, but assert recorded events/calls from createSandbox,
sandboxCreatePlan, sandboxMessagingPreflight, streamSandboxCreate, and
sandboxRegistration instead of literal source ordering.
In `@test/onboard-session-secret-invariants.test.ts`:
- Around line 41-52: The test currently uses createRequire/require() to delay
loading onboard-session and onboard-step-mutation until after HOME is
reassigned, but that breaks the ESM-only test rule. Replace the CommonJS
require-based loading in this test with deferred dynamic imports using top-level
await so the modules still load after tempHome is set. Keep the ordering
behavior intact by importing the same symbols from
../src/lib/state/onboard-session and ../src/lib/state/onboard-step-mutation via
await import(), and remove the createRequire usage entirely.
🪄 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: 7dc1530f-c37c-45f1-9753-980a2d0c9ad1
📒 Files selected for processing (6)
AGENTS.mdsrc/lib/onboard/AGENTS.mdsrc/lib/onboard/machine/README.mdsrc/lib/onboard/machine/transition-traces.test.tstest/onboard-lifecycle-invariants.test.tstest/onboard-session-secret-invariants.test.ts
There was a problem hiding this comment.
Thanks for assembling the lifecycle inventory. The contract map contains useful analysis, but this PR currently adds substantially more permanent maintenance surface than the behavior it protects, so I am requesting changes before merge.
Please re-scope this toward the smallest durable deliverable:
- Keep a compact contract map, but prefer stable symbol/module references over volatile line-number anchors. Please also reconsider placing a large current-state architecture inventory in nested
AGENTS.md, where it becomes instruction/context overhead for every onboarding change. - Remove or sharply reduce the 658-line transition suite. The canonical graph, transition kinds, runtime application, retry/branch/failure behavior, and runner behavior are already covered by
transitions.test.ts,runtime.test.ts, andrunner.test.ts. Add only path-level traces that demonstrate a concrete uncovered contract. - Remove the raw-source scanning in
onboard-lifecycle-invariants.test.ts. Exact string offsets and call-site snippets are brittle and conflict with our test guidance. Do not add production scaffolding solely to preserve these characterization assertions; if no behavioral seam exists, document the ordering and leave its executable coverage to the issue that introduces the seam. - Consolidate only genuinely missing session-security cases into the existing
onboard-session.test.ts. That suite already covers endpoint redaction, secret-free provider metadata, nullable credential updates, and failure-message redaction. Do not codify token-shaped secret persistence as an accepted passing contract; track/fix it separately or mark the desired behavior as pending. - Fix the characterization harness correctness bug before retaining any trace test:
mutator(cloneSession(session)) ?? sessiondiscards in-place mutations when the mutator returnsvoid. The fallback must be the cloned draft. Also keep root tests ESM-only rather than usingcreateRequire.
Please favor deletion and consolidation over rewriting all 1,788 test lines. A short map plus a small set of demonstrably missing behavioral tests would satisfy the useful outcome without duplicating the existing test matrix.
…cts (NVIDIA#6225) Add the onboarding lifecycle contract map (src/lib/onboard/AGENTS.md): epic NVIDIA#6224 vocabulary, per-journey contract tables for create, rebuild, re-onboard, and runtime mutation, seven cross-journey divergences with code anchors, and a bug-to-contract-gap table for the epic's evidence issues, with pointers from machine/README.md and the root AGENTS.md. Pin current behavior as an executable characterization baseline before the NVIDIA#6226/NVIDIA#6227 refactors move it: - machine/transition-traces.test.ts: legal-transition surface and full event traces for fresh-run, resume, recreate, and mid-flow failure. Failed-state exit legality and the legacy step-mutation bridge are deliberately not pinned; PR NVIDIA#6253 owns those semantics. - test/onboard-lifecycle-invariants.test.ts: create-path ordering invariants (conflict guard before sandbox delete, deterministic validation before the destructive boundary, cleanup-before-upsert, resume identity per NVIDIA#2753). - test/onboard-session-secret-invariants.test.ts: session persistence secret boundary (credentialEnv name-only, endpointUrl redaction, sha256-only legacy hashes, unset/declined ambiguity pinned as a known NVIDIA#6224 contract gap). Zero production-code changes. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
…n surface (NVIDIA#6225) Address review: replace the nested AGENTS.md inventory with a compact symbol-referenced map at src/lib/onboard/lifecycle-contracts.md and revert the root AGENTS.md registration; reduce transition-traces to the five ordered event-trace pins no existing machine suite covers; drop all raw-source scanning from the lifecycle invariants (the create-path ordering contract is now prose in the map); consolidate the genuinely missing session-security cases into src/lib/state/onboard-session.test.ts, encoding token-shaped-value persistence as pending desired behavior via it.fails rather than an accepted pin; fix the trace-harness updateSession fallback to return the cloned draft so void-returning mutators persist. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
|
Reworked in
|
b997556 to
ab988dd
Compare
wscurran
added
v0.0.76
area: architecture
Remove duplicate coverage and exercise recreate through the real sandbox handler. Align the lifecycle map with current main behavior. Co-authored-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
/ok to test 7ae4ed6 |
|
@cv The requested streamlining is ready for re-review. The revision removes the duplicate 394-line lifecycle-invariants suite, reduces the characterization to four runner-driven traces plus two session-boundary tests and one TODO, exercises recreate through the real sandbox handler, and audits the compact lifecycle map against current main. A synthetic merge onto current main was clean; CLI type-checking, 69 targeted tests (+1 TODO), 86 related onboarding tests, 65 onboard integration tests, changed-file hooks, and pre-push hooks all passed. The original author is credited in the verified commit. |
E2E Target Results — ✅ All requested jobs passedRun: 28899507422
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approving exact head 7ae4ed6a202c47c763cc5e6bcebf38be1be71148.
- All attached CI is green, with no unresolved review threads.
- The required exact-head
cloud-onboardlive proof passed: https://github.com/NVIDIA/NemoClaw/actions/runs/28899507422/job/85732556293 - DCO is present and every commit is GitHub Verified.
- The lifecycle map and focused runner traces are behavior-preserving; there are no production-code changes.
- The prior scope, duplicate raw-source suite, harness fallback, and session-boundary objections are resolved.
I reviewed the primary advisor's request to split the added session-boundary cases out of onboard-session.test.ts. That is nonblocking here: the earlier human review explicitly requested consolidating these cases into the existing session-boundary suite, and the repository's test-size/growth gate passes. The secondary advisor recommends merging as-is.
…cts (NVIDIA#6225) (NVIDIA#6259) ## Summary Delivers NVIDIA#6225 (epic NVIDIA#6224) as a behavior-preserving lifecycle inventory and focused characterization baseline. - `src/lib/onboard/lifecycle-contracts.md` — compact, symbol-referenced contract map covering new, fresh, resume, and recreate onboarding; rebuild and installer upgrade; channel mutations; provider, model, and credential changes; config, policy, resource, port-forward, and runtime contributions; agent differences; persisted-field ownership; effect boundaries; compensation; known gaps; and flow diagrams. - `src/lib/onboard/machine/transition-traces.test.ts` — four runner-driven traces for fresh success, resume, real-handler recreate, and failure. - `src/lib/state/onboard-session.test.ts` — two focused session-boundary characterizations plus one TODO for value-shaped URL secret redaction. - README pointers link the contract map and trace ownership. This revision removes the duplicate 394-line lifecycle-invariants suite and keeps zero production-code changes. ## Testing - Current-main synthetic merge: clean, with no conflicts. - `npm run typecheck:cli` - Targeted characterization: 69 passed, 1 TODO. - Related onboarding machine/create tests: 86 passed. - `test/onboard.test.ts`: 65 passed. - Changed-file prek hooks: all passed. - Pre-push hooks: CLI TypeScript and version sync passed. Closes NVIDIA#6225 Refs NVIDIA#6224 Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a lifecycle contract map for onboarding flows, including journey-level guidance and behavior expectations. * Expanded onboarding docs with a new section that explains how key lifecycle traces are tracked and maintained. * **Tests** * Added broader onboarding session coverage for persistence, reload behavior, and value handling. * Added lifecycle trace coverage to better validate onboarding flow order, resume behavior, repair handling, and failure paths. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
4 participants
Summary
Delivers #6225 (epic #6224) as a behavior-preserving lifecycle inventory and focused characterization baseline.
src/lib/onboard/lifecycle-contracts.md— compact, symbol-referenced contract map covering new, fresh, resume, and recreate onboarding; rebuild and installer upgrade; channel mutations; provider, model, and credential changes; config, policy, resource, port-forward, and runtime contributions; agent differences; persisted-field ownership; effect boundaries; compensation; known gaps; and flow diagrams.src/lib/onboard/machine/transition-traces.test.ts— four runner-driven traces for fresh success, resume, real-handler recreate, and failure.src/lib/state/onboard-session.test.ts— two focused session-boundary characterizations plus one TODO for value-shaped URL secret redaction.This revision removes the duplicate 394-line lifecycle-invariants suite and keeps zero production-code changes.
Testing
npm run typecheck:clitest/onboard.test.ts: 65 passed.Closes #6225
Refs #6224
Signed-off-by: Abhimanyu Kumar abhimanyukumar7290@gmail.com
Summary by CodeRabbit
Documentation
Tests