refactor(rebuild): remove superseded rebuild finalization - #8505
Open
harjothkhara wants to merge 4 commits into
Open
refactor(rebuild): remove superseded rebuild finalization#8505harjothkhara wants to merge 4 commits into
harjothkhara wants to merge 4 commits into
Conversation
Contributor
|
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)
📝 WalkthroughWalkthroughThe rebuild finalization module and its test file were removed. Coverage moved to post-restore and recreate observability tests. Retired-selector inventories were updated, and architecture budget thresholds were reduced. ChangesSandbox rebuild recovery
Architecture budget updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
harjothkhara
force-pushed
the
oss-find/nemoclaw-2026-08-06e
branch
from
August 7, 2026 00:39
e393e16 to
04006e5
Compare
rebuild-finalization.ts had no production caller. Its two exports, finalizeRebuildPostRestore and resetRebuildShieldsStateAfterRecreate, were imported only by its own test. rebuild-post-restore-phase.ts is the canonical post-restore implementation, and rebuild-recreate-phase.ts:307 already clears shields state on a recovery recreate. Four assertions in the deleted test covered behavior no other test checked, so they move to the suites that drive the live code: the relock-then-forward order, the forwarding skip after a failed relock, the unverified-forwarding recovery command, and the previously-enabled-shields warning on an unlocked recreate. The recovery-recreate shields reset moves to the recreate-phase suite. Repoint the sandbox-rebuild retired-selector replacement at rebuild-post-restore-phase.test.ts, and lower the source-architecture budgets the deletion frees. Refs: NVIDIA#8283 Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
force-pushed
the
oss-find/nemoclaw-2026-08-06e
branch
from
August 7, 2026 00:41
04006e5 to
e925815
Compare
Cross-review found three gaps in the migration. The deleted test asserted a fixed print order across all six incomplete-recovery lines at once. Each line was individually covered elsewhere, but the order was not, so a reordering of that print block would have gone unnoticed. Assert it against runRebuildPostRestorePhase with every failure dimension set together. Prove the negative half of the recovery-recreate shields reset: a failed recreate must leave prior shields state alone. The success path alone did not pin that. Add rebuild-recreate-observability.test.ts to the sandbox-rebuild retired-selector replacement so the moved shields-reset assertion keeps a compatibility backstop, and make the new recreate suite apply the updateSession mutator instead of discarding it. Refs: NVIDIA#8283 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 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. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
The fixed-order test claimed every incomplete recovery dimension but asserted six of them. The post-restore path also reports an unrefreshed MCP bridge and an incomplete live-policy reconciliation, so a reordering of either would have gone unnoticed (PRA-1). Assert both, and name the test for the OpenClaw path it actually drives. The Hermes gateway report stays out because ensureHermesGatewayAfterStateRestore returns "not-applicable" for OpenClaw, and baseline exclusions already have the NVIDIA#7194 test. Refs: NVIDIA#8283 Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
marked this pull request as ready for review
August 7, 2026 01:05
cv
requested changes
Aug 7, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Please resolve the reported merge conflicts against the current main commit and run the required PR workflows. GitHub reports CONFLICTING, and checks, check-hash, changes, commit-lint, and dco-check are missing for this revision. The dead-module removal and transferred invariant coverage are aligned with the stated refactor; approval remains blocked by the branch and CI state.
23 tasks
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Collaborator
|
Resolved cv's current-main and CI request in revision 46724ac.
Fresh required workflows are running. The branch is ready for re-review once they complete. |
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
3 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
src/lib/actions/sandbox/rebuild-finalization.tshas no production caller. Its two exports were imported only by its own test, whilerebuild-post-restore-phase.tsis the canonical post-restore implementation andrebuild-recreate-phase.ts:307already clears shields state on a recovery recreate. This deletes the superseded module and moves the assertions that were genuinely unique to it onto the suites that drive the live code.Related Issue
Refs: #8283
Changes
src/lib/actions/sandbox/rebuild-finalization.tsandrebuild-finalization.test.ts(355 LOC). Before deleting,grep -rn "finalizeRebuildPostRestore\|resetRebuildShieldsStateAfterRecreate"across the repo returned only the module and its own test.rebuild-post-restore-phase.test.ts, which drives the realrunRebuildPostRestorePhase: the registry → relock → forward order, that host forwarding is never attempted after a failed shields relock, theconnectrecovery command when forwarding is unverified, and the previously-enabled-shields warning on an unlocked recreate.rebuild-recreate-observability.test.ts, which drives the realrunRebuildRecreatePhase. The old test asserted this against a dead helper; it now asserts it againstrebuild-recreate-phase.ts:307.sandbox-rebuildretired-selector replacement fromrebuild-finalization.test.tstorebuild-post-restore-phase.test.tsintools/e2e/retired-selector-compatibility.mtsand its E2E-support test.runRebuildPostRestorePhasedoc comment, which named onlyrebuild-flow.test.tsandrebuild-config-hash.test.tsas boundary coverage. This change makesrebuild-post-restore-phase.test.tsa coverage owner too, so the comment is updated in the same PR rather than left drifting.ci/source-architecture-budget.jsonlimits the deletion frees (branding.ts87→86,terminal-style.ts45→44,registry.ts98→97,src/lib/actions/sandboxroot files 184→183).npm run checks:repositoryfails until these are ratcheted down.Net: 170 insertions, 372 deletions (−202). The issue estimated 330–380; the 92 lines of migrated assertions are the difference, and the mutation proof below is why they were kept rather than dropped.
Which assertions were unique
Grepping each message the deleted test asserted showed most were already covered canonically, so only these five moved:
test/helpers/rebuild-flow-recovery-cases.ts:709:710,:711:722,:747test/rebuild-stale-recovery.test.ts:261rebuild-post-restore-phase.test.ts:233rebuild-post-restore-phase.test.ts:204Review round
A private cross-review (Claude + Codex, independent then cross-examined) found three gaps in the first commit. The second commit fixes all three:
runRebuildPostRestorePhasewith every failure dimension set at once.rebuild-recreate-observability.test.tsis now in thesandbox-rebuildretired-selector replacement, so the moved shields-reset assertion keeps a backstop; and the new recreate suite now applies theupdateSessionmutator instead of discarding it, matching the sibling suite.Both new assertions were mutation-proved the same way as the originals:
npm run typecheck:clialso caught a real type error in the first draft of the ordering test (input()infersfailedPresetsasnever[], so assigning astring[]fails); the vitest run alone did not. Fixed by building the input by spread.Rebase note
First pushed on base
0463c5a21, then rebased ontodd62e1c32.codebase-growth-guardrailshad failed onnemoclaw/src/commands/migration-state.test.ts, a file this PR does not touch: #8489 shrank it to 1301 lines and ratcheted its budget to 1301, so the old base still carried the 1562-line copy and the check compared it against the new budget. The rebase clears it. All checks and budgets below were re-run on the new base.What was deliberately not migrated
One assertion in the deleted test was not carried over:
expect(updateSandbox).toHaveBeenCalledWith("alpha", { agentVersion, policies }). That payload shape belonged to the dead helper. The liverunRebuildPostRestorePhasewrites{ agentVersion, policies, policyTier, policyPresetsFinalized }(rebuild-post-restore-phase.ts:271-276), so copying the old two-field assertion across would have pinned a shape production does not use. The live shape is already asserted intest/helpers/rebuild-flow-recovery-cases.ts:577,716andtest/helpers/rebuild-flow-lifecycle-cases.ts:87,121, andresolveRestoredPolicyRegistryStatehas direct coverage atsrc/lib/actions/sandbox/rebuild-restore-phase.test.ts:641,654.Proof
A deletion has no natural failing test, so each migrated assertion was checked by mutating the live production path and confirming the migrated test fails while the pre-existing suite passes. A mutation the pre-existing suite already caught would mean the assertion was redundant.
M1–M3 mutate
rebuild-post-restore-phase.ts; M4 mutatesrebuild-recreate-phase.ts:307. The "pre-existing suite" column runs theHEADversion of the same test file against the mutated source.Test and check runs:
The 5 failures are pre-existing on this host and reproduce identically with the branch stashed:
gateway-restart-hermes-drift.test.tsfails onzip() takes no keyword arguments(host Python predateszip(strict=)), and 4e2e-supportfailures come from${TRIGGERING_ACTOR,,}needing bash 4 on a macOS bash 3.2 host. Neither touches any file in this change.Coverage note
Deleting 168 lines of fully covered source lowers the CLI coverage ratio slightly, so it is worth bounding.
scripts/check-coverage-ratchet.mts:21allows aTOLERANCEof 0.1 percentage point below each threshold. The coverage include set (src/**/*.tsminus tests, plusbin/**/*.js) is about 306,900 lines. Treating all 168 deleted lines as covered lines removed — the worst case — moveslinesfrom 73.900% to 73.886%, a shift of 0.014 ppt, roughly 7x inside the tolerance. The other three metrics scale the same way.I could not confirm this empirically: the repo's test and coverage workflows are
pull_request_target-gated and are not among the 9 checks running on this unvetted fork PR, and the local run is the one described above. The bound is arithmetic, not a measurement, and a maintainer's vetted CI run supersedes it.Type of Change
Quality Gates
src/production file is modified, and the shields and messaging-forward assertions gain coverage rather than lose it.Documentation Writer Review
no-docs-neededVerification
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 testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run to completion locally.npm run test:coverage:cliwas started but the branch was rebased and amended under it, so its number would not describe this tree, and it was stopped. See the coverage note below for why the ratchet is not at risk. Please treat the vetted CI run as the gate.npm run docsbuilds without warnings (doc changes only)Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests