Skip to content

fix(blueprint): add error handling for JSON.parse in state, config, and migration loaders#638

Open
pjt222 wants to merge 1 commit intoNVIDIA:mainfrom
pjt222:fix/json-parse-error-handling
Open

fix(blueprint): add error handling for JSON.parse in state, config, and migration loaders#638
pjt222 wants to merge 1 commit intoNVIDIA:mainfrom
pjt222:fix/json-parse-error-handling

Conversation

@pjt222
Copy link

@pjt222 pjt222 commented Mar 22, 2026

Summary

  • Add try-catch error handling around bare JSON.parse calls in loadState(), loadOnboardConfig(), and readSnapshotManifest()
  • Follows the existing pattern established by loadConfigDocument() in migration-state.ts (lines 143-153)
  • loadState() recovers silently (returns blank state) — state is machine-written ephemeral data, and the next saveState() call self-heals the file
  • loadOnboardConfig() recovers silently (returns null) — allows re-onboarding
  • readSnapshotManifest() throws a descriptive error with structural validation (matching loadConfigDocument pattern) — migration cannot proceed safely with corrupt data

Related to #553

Test plan

  • cd nemoclaw && npx vitest run — 79 tests pass (4 new)
  • npx tsc --noEmit — no type errors
  • Added test for malformed JSON in state.test.ts
  • Added test for malformed JSON in config.test.ts
  • Added tests for malformed JSON and array input in migration-state.test.ts

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

…nd migration loaders

Three sites used bare JSON.parse(readFileSync(...)) that crash on
corrupted files. The same codebase already has robust error handling
in loadConfigDocument() (migration-state.ts:143-153) — this applies
the same patterns to the remaining unprotected call sites.

Fixes:
- loadState() (state.ts): try-catch returning blankState() — state
  is ephemeral machine-written data, silent recovery matches the
  existing null-on-missing pattern and allows self-healing on next
  saveState() call.
- loadOnboardConfig() (config.ts): try-catch returning null —
  triggers re-onboarding, matches the null-on-missing pattern.
- readSnapshotManifest() (migration-state.ts): structural validation
  (not-null, is-object, not-array) following the loadConfigDocument()
  pattern — throws on corruption because migration cannot proceed
  safely with corrupt manifest data.

Related to NVIDIA#553

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

The PR adds error-handling safeguards for JSON file operations across three modules (state, migration-state, and config). Each loader function now gracefully handles malformed or invalid JSON by returning fallback values instead of throwing exceptions, and validation logic ensures parsed data matches expected types.

Changes

Cohort / File(s) Summary
State Module
nemoclaw/src/blueprint/state.ts, nemoclaw/src/blueprint/state.test.ts
Added try/catch wrapper to loadState() to return blank state on JSON parse failures; new test validates malformed JSON handling.
Migration State Module
nemoclaw/src/commands/migration-state.ts, nemoclaw/src/commands/migration-state.test.ts
Added runtime validation to readSnapshotManifest() ensuring parsed JSON is an object (not array); two new tests cover malformed JSON and non-object parsing scenarios.
Config Module
nemoclaw/src/onboard/config.ts, nemoclaw/src/onboard/config.test.ts
Added try/catch wrapper to loadOnboardConfig() to return null on JSON parse failures; new test validates invalid JSON handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 When JSON stumbles, graceful we stand,
With try/catch shields across the land,
No parse shall crash our precious flow,
We bounce back gently, steady and slow! 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: adding error handling for JSON.parse operations across three loaders (state, config, and migration), which is the primary focus of the changeset.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant