refactor: don't fail on empty files in readYamlOrJson#8008
refactor: don't fail on empty files in readYamlOrJson#8008antigremlin wants to merge 12 commits intomainfrom
Conversation
Tests expect null for empty JSON/YAML files. Malformed JSON still throws.
readJson returns null for empty/whitespace-only files, matching YAML behavior. Malformed JSON still throws.
Remove try-catch blocks in favor of explicit null checks now that readYamlOrJson returns null for empty files instead of throwing. - AltVMFileSubmitter: Check null explicitly, log only for invalid data - EV5FileSubmitter: Same pattern
…n null on empty JSON files, for consistency with YAML behavior and simplified error handling.
antigremlin
requested review from
Xaroz,
ltyu,
paulbalaji,
troykessler,
xeno097 and
yorhodes
as code owners
|
@claude review |
|
Claude finished @paulbalaji's task in 1m 17s —— View job Code Review: PR #8008Summary: This PR makes ✅ What looks good
|
📝 WalkthroughWalkthroughThis change makes file-read helpers return null for empty/whitespace-only JSON/YAML and adds throwing/non-null wrappers and runtime assertions across CLI, infra, rebalancer, and tests, updating call sites to handle nulls or assert non-emptiness. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Fix all issues with AI agents
In `@typescript/utils/src/fs/json.ts`:
- Around line 11-16: readJson currently returns null at runtime but is typed as
T; change its signature to return T | null (export function
readJson<T>(filepath: string): T | null) and keep the runtime behavior returning
null for empty content; then update callers—specifically
writeJsonWithAppendMode—to handle null by guarding merges (do not spread
`existing` when it's null, instead treat null as an empty container appropriate
for the target type: for object merges use `{}`, for array appends use `[]`) and
add a runtime check like `if (existing == null) existing = defaultEmpty` before
merging/appending so empty files no longer cause crashes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript/infra/src/utils/sealevel.ts (1)
503-510:⚠️ Potential issue | 🟡 MinorThe assertion's in the right place, but the error messages might get a bit swampy.
If the file exists but is empty, the
assertthrows "Empty program IDs file at {path}", but then thecatchblock wraps it as "Failed to load program IDs from {path}." - losing the original reason. The outer catch makes sense for handling file-not-found or JSON parse errors, but it'll also swallow the assertion's more specific message.Might want to let the assertion bubble up on its own or include the original error in the catch message.
🔧 Option: Preserve the original error message
try { const programIds = readJson<CoreProgramIds>(programIdsPath); assert(programIds, `Empty program IDs file at ${programIdsPath}`); return programIds; } catch (error) { - throw new Error(`Failed to load program IDs from ${programIdsPath}.`); + throw new Error(`Failed to load program IDs from ${programIdsPath}: ${error}`); }
🤖 Fix all issues with AI agents
In @.changeset/nasty-olives-hide.md:
- Around line 1-7: The changeset claims breaking behavior for readJson() and
readYamlOrJson() now returning T | null; verify the correct semver level for the
three packages '@hyperlane-xyz/deploy-sdk', '@hyperlane-xyz/utils', and
'@hyperlane-xyz/cli' and update the changeset if necessary (patch → minor or
major) per our repository policy; ensure the changeset metadata and any
changelog entries reflect that callers now must handle null from
readJson/readYamlOrJson and bump the package versions accordingly.
🧹 Nitpick comments (3)
typescript/infra/test/balance-alerts.test.ts (1)
54-59: This null check keeps the donkeys from running wild.The guard properly handles the new nullable return from
readJson. Throwing with the alert name gives enough context to track down the problem.One small thing to ponder - other files in this PR use
assert()for this pattern. Using a manual throw works just fine here, but switching toassert(proposedThresholds, ...)would keep things consistent across the swamp.♻️ Optional: Use assert for consistency
const proposedThresholds = readJson<ChainMap<number>>( `${THRESHOLD_CONFIG_PATH}/${alertConfigMapping[alert].configFileName}`, ); - if (!proposedThresholds) { - throw new Error(`Empty thresholds file for ${alert}`); - } + assert(proposedThresholds, `Empty thresholds file for ${alert}`);You'd need to add
assertto the imports from@hyperlane-xyz/utils.typescript/infra/scripts/agent-utils.ts (1)
730-733: Variable name shadows importedpathmodule.Look, the assertion pattern here is solid as a rock — nice defensive coding. But
pathon line 730 shadows thepathmodule imported on line 3. While it works because the module isn't used after this point in the function, it's the kind of thing that'll bite someone later when they try to usepath.join()and get a string instead of a module.Consider renaming to
addressesPathorfilePathto keep things clear in our swamp.✨ Suggested rename
- const path = getInfraLandfillPath(environment, module); - const loadedAddresses = readJson<ChainMap<ChainAddresses>>(path); - assert(loadedAddresses, `Empty addresses file at ${path}`); + const addressesPath = getInfraLandfillPath(environment, module); + const loadedAddresses = readJson<ChainMap<ChainAddresses>>(addressesPath); + assert(loadedAddresses, `Empty addresses file at ${addressesPath}`); addresses = loadedAddresses;typescript/utils/src/fs/format.ts (1)
61-74: The type change is correct; consider updating the docstring.The signature now honestly tells callers what they're getting —
T | null. That's fair play. But the docstring above still says "Reads and parses a YAML or JSON file" without mentioning that empty or whitespace-only files returnnull. Might want to give folks a heads-up in the comments so they don't wander into the swamp unprepared.📝 Suggested docstring update
/** * Reads and parses a YAML or JSON file based on extension or explicit format. + * Returns null if the file is empty or contains only whitespace. */ export function readYamlOrJson<T>( filepath: string, format?: FileFormat, ): T | null {
| --- | ||
| '@hyperlane-xyz/deploy-sdk': patch | ||
| '@hyperlane-xyz/utils': patch | ||
| '@hyperlane-xyz/cli': patch | ||
| --- | ||
|
|
||
| Changed `readJson()` and `readYamlOrJson()` to return `T | null` on empty JSON files, for consistency with YAML behavior and simplified error handling. All call sites updated with explicit null checks using `assert()` or null coalescing. |
There was a problem hiding this comment.
Double-check the version bump level. The PR description calls this a breaking change; a patch bump might understate the impact depending on your changeset policy.
🤖 Prompt for AI Agents
In @.changeset/nasty-olives-hide.md around lines 1 - 7, The changeset claims
breaking behavior for readJson() and readYamlOrJson() now returning T | null;
verify the correct semver level for the three packages
'@hyperlane-xyz/deploy-sdk', '@hyperlane-xyz/utils', and '@hyperlane-xyz/cli'
and update the changeset if necessary (patch → minor or major) per our
repository policy; ensure the changeset metadata and any changelog entries
reflect that callers now must handle null from readJson/readYamlOrJson and bump
the package versions accordingly.
⚙️ Node Service Docker Images Built Successfully
Full image paths |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@typescript/cli/src/tests/ethereum/warp/warp-check-2.e2e-test.ts`:
- Around line 23-27: The CHAIN_3 metadata call currently uses readYamlOrJson
which can return undefined and later causes a TypeError; replace that call with
readYamlOrJsonOrThrow (same import already present) so the code uses the
throwing variant for CHAIN_3 as well (matching the change for other chains) and
surfaces the clear “Empty config file” error instead of a TypeError; update the
reference to the specific variable/usage of CHAIN_3 in warp-check-2.e2e-test.ts
to call readYamlOrJsonOrThrow.
In `@typescript/cli/src/tests/ethereum/warp/warp-read.e2e-test.ts`:
- Around line 81-82: The code awaits a synchronous helper; remove the
unnecessary await when calling the synchronous function readYamlOrJsonOrThrow so
calls like "const exampleWarpConfig: WarpRouteDeployConfig = await
readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE);" become synchronous assignments
(drop await) and do the same for the other occurrences where
readYamlOrJsonOrThrow is awaited (the similar calls creating configs around the
other test variables); ensure the variable names (exampleWarpConfig and any
other config variables using readYamlOrJsonOrThrow) are assigned directly
without await.
In `@typescript/cli/src/utils/files.ts`:
- Around line 65-86: The current use of assert(result, ...) in readJsonOrThrow
and readYamlOrJsonOrThrow will incorrectly throw for valid falsy values like
false, 0, or "". Update both functions (readJsonOrThrow and
readYamlOrJsonOrThrow) to only treat null/undefined as "empty" — e.g. after
calling readJson<T>(filepath) or readYamlOrJson<T>(filepath, format) check if
result == null (or result === null || result === undefined) and throw a proper
Error with the existing message if so; otherwise return result.
🧹 Nitpick comments (3)
typescript/cli/src/tests/ethereum/warp/warp-send.e2e-test.ts (1)
25-29: Consider upgrading remainingreadYamlOrJsonreads to fail‑fast or guard nulls.Now that empty files return
null, switching the other warp‑core reads in this test toreadYamlOrJsonOrThrow(or adding null checks) would keep failures clearer if a config file ends up empty.Also applies to: 64-69
typescript/cli/src/tests/aleo/warp/warp-apply-route-extension.e2e-test.ts (1)
86-91: Oi, something's a wee bit off in the swamp here.These calls to
readYamlOrJsonOrThroware missing the generic type parameter. While the assignment tochain1CoreAddressandchain2CoreAddress(typed asChainAddresses) may still work due to TypeScript's type inference from the variable declaration, explicitly providing the generic improves clarity and ensures the function's internal typing is correct.🧅 Suggested improvement for explicit typing
- chain1CoreAddress = readYamlOrJsonOrThrow( + chain1CoreAddress = readYamlOrJsonOrThrow<ChainAddresses>( `${REGISTRY_PATH}/chains/${TEST_CHAIN_NAMES_BY_PROTOCOL.aleo.CHAIN_NAME_1}/addresses.yaml`, ); - chain2CoreAddress = readYamlOrJsonOrThrow( + chain2CoreAddress = readYamlOrJsonOrThrow<ChainAddresses>( `${REGISTRY_PATH}/chains/${TEST_CHAIN_NAMES_BY_PROTOCOL.aleo.CHAIN_NAME_2}/addresses.yaml`, );typescript/cli/src/tests/ethereum/warp/warp-read.e2e-test.ts (1)
81-82: Also, while we're at it...These call sites are also missing the generic type parameter
<WarpRouteDeployConfig>. While the variable type annotation provides some safety, adding the generic ensures the function's internal typing is correct. Line 217-218 shows the correct pattern withreadYamlOrJsonOrThrow<WarpRouteDeployConfig>(readOutputPath).🧅 Suggested improvement for explicit typing
// Line 81-82 const exampleWarpConfig: WarpRouteDeployConfig = - readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE); + readYamlOrJsonOrThrow<WarpRouteDeployConfig>(WARP_CONFIG_PATH_EXAMPLE); // Line 111-112 const warpReadResult: WarpRouteDeployConfig = - readYamlOrJsonOrThrow(WARP_CONFIG_PATH_2); + readYamlOrJsonOrThrow<WarpRouteDeployConfig>(WARP_CONFIG_PATH_2); // Line 159-160 const warpReadResult: WarpRouteDeployConfig = - readYamlOrJsonOrThrow(readOutputPath); + readYamlOrJsonOrThrow<WarpRouteDeployConfig>(readOutputPath);Also applies to: 111-112, 159-160
| import { | ||
| readYamlOrJson, | ||
| readYamlOrJsonOrThrow, | ||
| writeYamlOrJson, | ||
| } from '../../../utils/files.js'; |
There was a problem hiding this comment.
Use the throwing read for CHAIN_3 metadata too.
Line 69 still uses readYamlOrJson, so an empty file now yields a TypeError instead of a clear “Empty config file” error. Align with the new behavior for consistency.
🛠️ Suggested fix
-import {
- readYamlOrJson,
- readYamlOrJsonOrThrow,
- writeYamlOrJson,
-} from '../../../utils/files.js';
+import {
+ readYamlOrJsonOrThrow,
+ writeYamlOrJson,
+} from '../../../utils/files.js';
@@
- chain3DomainId = (readYamlOrJson(CHAIN_3_METADATA_PATH) as ChainMetadata)
- .domainId;
+ chain3DomainId = readYamlOrJsonOrThrow<ChainMetadata>(
+ CHAIN_3_METADATA_PATH,
+ ).domainId;Also applies to: 66-70
🤖 Prompt for AI Agents
In `@typescript/cli/src/tests/ethereum/warp/warp-check-2.e2e-test.ts` around lines
23 - 27, The CHAIN_3 metadata call currently uses readYamlOrJson which can
return undefined and later causes a TypeError; replace that call with
readYamlOrJsonOrThrow (same import already present) so the code uses the
throwing variant for CHAIN_3 as well (matching the change for other chains) and
surfaces the clear “Empty config file” error instead of a TypeError; update the
reference to the specific variable/usage of CHAIN_3 in warp-check-2.e2e-test.ts
to call readYamlOrJsonOrThrow.
| const exampleWarpConfig: WarpRouteDeployConfig = | ||
| await readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE); |
There was a problem hiding this comment.
Hold up there, donkey - something's not quite right in these parts.
The readYamlOrJsonOrThrow function is synchronous (it just calls readYamlOrJson and uses assert), but these lines are using await on it. While JavaScript will handle awaiting a non-Promise value without error, it's misleading and inconsistent with the usage on line 217-218 in the same file where await is correctly omitted.
🧅 Suggested fix to remove unnecessary await
// Line 81-82
- const exampleWarpConfig: WarpRouteDeployConfig =
- await readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE);
+ const exampleWarpConfig: WarpRouteDeployConfig =
+ readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE);
// Line 111-112
const warpReadResult: WarpRouteDeployConfig =
- await readYamlOrJsonOrThrow(WARP_CONFIG_PATH_2);
+ readYamlOrJsonOrThrow(WARP_CONFIG_PATH_2);
// Line 159-160
const warpReadResult: WarpRouteDeployConfig =
- await readYamlOrJsonOrThrow(readOutputPath);
+ readYamlOrJsonOrThrow(readOutputPath);Also applies to: 111-112, 159-160
🤖 Prompt for AI Agents
In `@typescript/cli/src/tests/ethereum/warp/warp-read.e2e-test.ts` around lines 81
- 82, The code awaits a synchronous helper; remove the unnecessary await when
calling the synchronous function readYamlOrJsonOrThrow so calls like "const
exampleWarpConfig: WarpRouteDeployConfig = await
readYamlOrJsonOrThrow(WARP_CONFIG_PATH_EXAMPLE);" become synchronous assignments
(drop await) and do the same for the other occurrences where
readYamlOrJsonOrThrow is awaited (the similar calls creating configs around the
other test variables); ensure the variable names (exampleWarpConfig and any
other config variables using readYamlOrJsonOrThrow) are assigned directly
without await.
| /** | ||
| * Reads and parses a JSON file, throwing an error if the file is empty. | ||
| * Use this in tests or CLI commands where empty files indicate a configuration error. | ||
| */ | ||
| export function readJsonOrThrow<T>(filepath: string): T { | ||
| const result = readJson<T>(filepath); | ||
| assert(result, `Empty JSON file at ${filepath}`); | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Reads and parses a YAML or JSON file, throwing an error if the file is empty. | ||
| * Use this in tests or CLI commands where empty files indicate a configuration error. | ||
| */ | ||
| export function readYamlOrJsonOrThrow<T>( | ||
| filepath: string, | ||
| format?: FileFormat, | ||
| ): T { | ||
| const result = readYamlOrJson<T>(filepath, format); | ||
| assert(result, `Empty config file at ${filepath}`); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Avoid throwing on valid falsy JSON/YAML values.
assert(result, ...) will fail for legitimate configs like false, 0, or "". Check for null/undefined instead so only empty files trip the error.
🛠️ Suggested fix
export function readJsonOrThrow<T>(filepath: string): T {
const result = readJson<T>(filepath);
- assert(result, `Empty JSON file at ${filepath}`);
+ assert(result !== null && result !== undefined, `Empty JSON file at ${filepath}`);
return result;
}
@@
export function readYamlOrJsonOrThrow<T>(
filepath: string,
format?: FileFormat,
): T {
const result = readYamlOrJson<T>(filepath, format);
- assert(result, `Empty config file at ${filepath}`);
+ assert(result !== null && result !== undefined, `Empty config file at ${filepath}`);
return result;
}🤖 Prompt for AI Agents
In `@typescript/cli/src/utils/files.ts` around lines 65 - 86, The current use of
assert(result, ...) in readJsonOrThrow and readYamlOrJsonOrThrow will
incorrectly throw for valid falsy values like false, 0, or "". Update both
functions (readJsonOrThrow and readYamlOrJsonOrThrow) to only treat
null/undefined as "empty" — e.g. after calling readJson<T>(filepath) or
readYamlOrJson<T>(filepath, format) check if result == null (or result === null
|| result === undefined) and throw a proper Error with the existing message if
so; otherwise return result.
|
The changeset has grown too big. We should take a different approach even though I still think the file-reading utils should be more consistent between YAML/JSON and explicit about throwing or returning nulls. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8008 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
🚀 New features to boost your workflow:
|
2 participants
readJsonnow returnsnullfor empty/whitespace-only JSON files, matching YAML behavior. FileSubmitter classes are simplified by removing try-catch blocks around empty file handling.Backward Compatibility
Breaking change: Code expecting
readYamlOrJson/readJson()to throw on empty files will now receivenull. However, most callers already handle null/undefined (e.g., chain config reader checks!chainMetadata), and the two FileSubmitter classes have been updated to handle this correctly.Testing
Covered by new unit tests.
Summary by CodeRabbit
Bug Fixes
New Features
Tests