fix(registry): drop legacy credential hash rows - #6611
Merged
cv merged 7 commits intoJul 11, 2026
Conversation
Signed-off-by: Ho Lim <subhoya@gmail.com>
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)
📝 WalkthroughWalkthroughSandbox persistence now removes legacy ChangesSandbox persistence
Estimated code review effort: 2 (Simple) | ~10 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/state/registry.ts (1)
438-452: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueCall out
providerCredentialHashesin the persistence note. The inline comment only mentions the transient recovery markers, but this strip also removes legacy persistedproviderCredentialHashes; a short wording tweak would make the intent clearer.🤖 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/state/registry.ts` around lines 438 - 452, Update the inline comment above serializeSandboxEntryForDisk to explicitly mention that legacy persisted providerCredentialHashes is also removed, alongside the transient recovery markers. Keep the note concise and clarify that all listed fields are excluded from durable sandboxes.json output.
🤖 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/state/registry.ts`:
- Around line 438-452: Update the inline comment above
serializeSandboxEntryForDisk to explicitly mention that legacy persisted
providerCredentialHashes is also removed, alongside the transient recovery
markers. Keep the note concise and clarify that all listed fields are excluded
from durable sandboxes.json output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d28a08bf-482a-4792-aede-f4fe80185a10
📒 Files selected for processing (2)
src/lib/state/registry.tstest/registry.test.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Collaborator
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Collaborator
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29144624012
|
cv
approved these changes
Jul 11, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Approved on exact head 3a09e94. All five required checks pass; DCO/signatures are verified; CodeRabbit has no unresolved threads; trusted advisor run 29144488094 has both exact-head models at merge_as_is with zero findings. Exact trusted E2E run 29144624012 passed onboard-repair, onboard-resume, and token-rotation. Canceled commit-lint/DCO contexts are superseded duplicates, and advisor comment-provenance formatting does not invalidate the exact artifacts.
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary - Drop legacy `providerCredentialHashes` buckets when sandbox registry rows are rewritten. - Preserve the current messaging plan credential binding as the durable source of truth. - Add a regression test that starts from a legacy Telegram hash row and proves the next registry update removes the stale bucket without losing the current binding. ## Rationale Current main stores messaging credential fingerprints on `messaging.plan.credentialBindings`, but older registry rows can still contain a legacy `providerCredentialHashes` field. If that legacy field survives later registry writes, token rotation can appear to retain duplicate credential-hash state. This keeps the canonical binding and removes the legacy bucket during normal registry serialization. Refs NVIDIA#3631 ## Validation - `npm ci` - `npx vitest run --project integration test/registry.test.ts` - `git diff --check origin/main...HEAD` - `npx prek run --files src/lib/state/registry.ts test/registry.test.ts --stage pre-commit` - `npx prek run --from-ref origin/main --to-ref HEAD --stage pre-commit` - `npx commitlint --from origin/main --to HEAD` Signed-off-by: Ho Lim <subhoya@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * When updating sandbox settings, legacy/transient credential metadata is no longer persisted, preventing stale credential hashes from remaining in saved entries. * Messaging sandbox updates now rewrite credential bindings to retain only the current, expected credential hash. * **Tests** * Added coverage to verify legacy credential metadata is dropped and messaging rows are rewritten correctly during sandbox updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: sandbox
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
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
providerCredentialHashesbuckets when sandbox registry rows are rewritten.Rationale
Current main stores messaging credential fingerprints on
messaging.plan.credentialBindings, but older registry rows can still contain a legacyproviderCredentialHashesfield. If that legacy field survives later registry writes, token rotation can appear to retain duplicate credential-hash state. This keeps the canonical binding and removes the legacy bucket during normal registry serialization.Refs #3631
Validation
npm cinpx vitest run --project integration test/registry.test.tsgit diff --check origin/main...HEADnpx prek run --files src/lib/state/registry.ts test/registry.test.ts --stage pre-commitnpx prek run --from-ref origin/main --to-ref HEAD --stage pre-commitnpx commitlint --from origin/main --to HEADSigned-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit