fix(deploy): make target-state generation fail loudly instead of silently wiping entries - #2148
Draft
0xDEnYO wants to merge 3 commits into
Draft
fix(deploy): make target-state generation fail loudly instead of silently wiping entries#21480xDEnYO wants to merge 3 commits into
0xDEnYO wants to merge 3 commits into
Conversation
…ntly wiping entries parseTargetStateGoogleSpreadsheet removed all existing entries for the environment before it had parsed anything, so any parse that produced nothing deleted target state and still exited 0. Parse and validate first, then remove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
WalkthroughChangesTarget state regeneration
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
The new guards returned 1 into callers that ignored it, so a refused run printed an error and the menu carried on as if it had succeeded. Also stop a jq failure in the missing-networks check from reading as "no networks would be lost". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Review-gate pass (local trial). Nothing escalated — no findings hit the always-escalate classes (no storage/selector/access-control/event/bridge-math surface; this PR is bash tooling only). Six findings auto-fixed in 052791a and the body correction above. Two lower-confidence items left for human judgment, not acted on:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
script/helperFunctions.sh (1)
1788-1788: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
mktempinstead of a fixed CSV filename.
CSV_FILE_PATH="newTest.csv"is a hardcoded, debug-sounding name written to the working directory; the function already usesmktemp -dforTEMP_DIRlater on. Reusing that pattern here avoids leaving a stray file behind on abnormal exits and avoids collisions if this function is ever invoked twice concurrently.🤖 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 `@script/helperFunctions.sh` at line 1788, Replace the fixed CSV_FILE_PATH assignment with a unique temporary-file path created via mktemp, consistent with the existing TEMP_DIR setup. Ensure the CSV path is used by the surrounding function and remains isolated for concurrent invocations without writing a debug-named file to the working directory.
🤖 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 `@script/helperFunctions.sh`:
- Line 1789: Quote the CSV_FILE_PATH variable in the curl output argument within
the surrounding command, changing the unquoted -o value to preserve paths
containing spaces or glob characters while leaving the existing URL and
error-handling behavior unchanged.
- Around line 1789-1793: Update the curl invocation in the spreadsheet download
error path to include both connection and overall request timeouts, while
preserving the existing URL, output path, and failure handling through the error
message, cleanup, and return statements.
- Line 1897: Update the loop around EXISTING_NETWORKS to iterate with a while
IFS= read -r pattern, preserving each jq output entry verbatim instead of
relying on unquoted word splitting. Keep the existing loop body and
network-processing behavior unchanged.
---
Nitpick comments:
In `@script/helperFunctions.sh`:
- Line 1788: Replace the fixed CSV_FILE_PATH assignment with a unique
temporary-file path created via mktemp, consistent with the existing TEMP_DIR
setup. Ensure the CSV path is used by the surrounding function and remains
isolated for concurrent invocations without writing a debug-named file to the
working directory.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d83c4a2-cb36-405d-a996-ef82c3a91a6d
📒 Files selected for processing (3)
.env.examplescript/helperFunctions.shscript/scriptMaster.sh
…e sheet download getCurrentContractVersion prints its errors to stdout, so the command substitution in processNetworkLine captured ANSI error text on failure and CURRENT_VERSION was never empty -- the missing-version warning (including the case-mismatch hint added on this branch) could never fire. Check the exit code and blank the variable so it does; verified against the real sheet export that a misspelled cell now produces the spelling hint. Also: distinguish "file exists but has no @Custom:version" from a true case mismatch; name the requested network in the empty-rows error when a specific network was asked for; quote the curl output path and add connect/transfer timeouts; iterate jq output with a read loop instead of unquoted word splitting; correct the jq $ENV comment (an object index errors out rather than silently matching nothing). Full-run output is byte-identical to the pre-fix generator on the current production sheet, twice over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@script/helperFunctions.sh`:
- Around line 2038-2042: Update the latest-version handling around
CURRENT_VERSION and addContractVersionToNetworkJSON to reject an empty or
unresolved lookup before writing target state, returning a worker failure.
Ensure the parent network-processing flow captures and aggregates that worker
failure before merging results, so parsing cannot continue with an invalid
version.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac14765e-5022-4f72-bf53-2885edc21dd3
📒 Files selected for processing (1)
script/helperFunctions.sh
Comment on lines
+2038
to
+2042
| # getCurrentContractVersion prints its error messages to stdout, so on failure the | ||
| # command substitution captures error text instead of leaving the variable empty -- | ||
| # blank it explicitly or the emptiness check below can never fire. | ||
| local CURRENT_VERSION | ||
| CURRENT_VERSION=$(getCurrentContractVersion "$CONTRACT") || CURRENT_VERSION="" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject unresolved latest versions before writing target state.
Line 2042 now clears CURRENT_VERSION on lookup failure, but the later latest branch still passes that empty value to addContractVersionToNetworkJSON at Line 2085. A missing or mis-cased contract can therefore generate an empty version while the parser continues. Fail the network before writing it, and ensure the parent aggregates that worker failure before merging.
Proposed guard
CURRENT_VERSION=$(getCurrentContractVersion "$CONTRACT") || CURRENT_VERSION=""
+if [[ "$CELL_VALUE" == "latest" && -z "$CURRENT_VERSION" ]]; then
+ error "[$NETWORK] cannot resolve latest version for contract $CONTRACT"
+ return 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # getCurrentContractVersion prints its error messages to stdout, so on failure the | |
| # command substitution captures error text instead of leaving the variable empty -- | |
| # blank it explicitly or the emptiness check below can never fire. | |
| local CURRENT_VERSION | |
| CURRENT_VERSION=$(getCurrentContractVersion "$CONTRACT") || CURRENT_VERSION="" | |
| # getCurrentContractVersion prints its error messages to stdout, so on failure the | |
| # command substitution captures error text instead of leaving the variable empty -- | |
| # blank it explicitly or the emptiness check below can never fire. | |
| local CURRENT_VERSION | |
| CURRENT_VERSION=$(getCurrentContractVersion "$CONTRACT") || CURRENT_VERSION="" | |
| if [[ "$CELL_VALUE" == "latest" && -z "$CURRENT_VERSION" ]]; then | |
| error "[$NETWORK] cannot resolve latest version for contract $CONTRACT" | |
| return 1 | |
| fi |
🤖 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 `@script/helperFunctions.sh` around lines 2038 - 2042, Update the
latest-version handling around CURRENT_VERSION and
addContractVersionToNetworkJSON to reject an empty or unresolved lookup before
writing target state, returning a worker failure. Ensure the parent
network-processing flow captures and aggregates that worker failure before
merging results, so parsing cannot continue with an invalid version.
8 tasks
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
1 participant
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.
Which Linear task belongs to this PR?
No ticket yet — Linear was unavailable in the session that produced this. Needs an EXSC ID before merge.
Context: fell out of investigating why the nightly Diamond Health Check reported 68/71 networks failing. #2120 fixed the check-side false failures; this fixes one of the two process gaps deferred from that work.
Why did I implement it this way?
What was broken
parseTargetStateGoogleSpreadsheet(scriptMaster.shuse case 10) regeneratesscript/deploy/_targetState.jsonfrom a Google Sheet. It deleted all existing entries forthe environment before it had parsed anything, then repopulated from the parse. So any
parse that produced nothing deleted target state and still printed
Processing completed successfully!and exited 0.Three independent ways to hit that, all observed in one afternoon of use:
read -a, which zsh does not have. Everyrow fails with
read:19: bad option: -a, the arrays stay empty — and the run stillreports success. This collapsed 1995 facet entries to 28. The
#!/bin/bashshebangprotects
./script/scriptMaster.shbut notzsh script/scriptMaster.shand not sourcingthe helpers into an interactive zsh, which is how it was hit.
/export?exportFormat=csv, nogid) only covers the sheet's first tab. The wipe covered all production entries, soanything not on that tab was silently deleted. This wiped Tron's 20 entries to 0 because
Tron had no row. Any future non-EVM chain kept on another tab hits the same thing.
curl -L … 2>/dev/nullwith no status check happily wrote a 404HTML page into the CSV, which then parsed as an empty sheet.
Why this matters even though it's "just a config file":
_targetState.jsondrives deploytooling, and the health check derives its non-core facet and periphery lists from it
(
healthCheckInvariants.tsperiphery-registered,healthCheck.ts:136). A network wipedfrom target state therefore checks fewer contracts and reports green — the failure
mode makes the alarm quieter, not louder. CI only validates that the file is syntactically
valid JSON (
jsonChecker.yml:96).The approach
Parse and validate first, then mutate. Moving the removal to after the parse is the
structural fix — it turns every "parse produced nothing" failure from data loss into a no-op,
including failure modes nobody has thought of yet. The specific guards on top are cheap:
$BASH_VERSIONcheck with the fix in the message.Considered rewriting the
read -acalls to be POSIX-portable instead, but this function isone of many bash-only ones in a 6k-line bash library; making just this one portable would
imply a guarantee the file doesn't keep.
curl -fsSLwith an explicit failure branch, replacing the silenced call. Note thisonly covers hard HTTP failures — a sheet that isn't shared redirects to a login page and
returns 200 with HTML, which
-fdoes not catch; the empty-parse guard below is whatactually covers that case.
shell, never a legitimate "target state is empty now".
networks, with
ALLOW_TARGET_STATE_NETWORK_REMOVAL=trueto override deliberately. This isthe direct fix for the single-tab export deleting Tron. Chose a refuse-with-override over
auto-preserving the missing networks, because silently keeping stale entries for a chain
someone deliberately removed from the sheet is its own failure mode — the operator should
say which they meant.
getContractFilePathusesfind -name, which is case-sensitive regardless of the filesystem, so a sheet cell readingNearIntentsFacetmissesNEARIntentsFacet.sol. The warning now does a case-insensitivelookup and says "no src file named
NearIntentsFacet.sol, butNEARIntentsFacet.solexists — fix the spelling in the Google sheet".
Deliberately not in this PR, to keep it to one problem:
gidparameter for multi-tab exports — needs a decision on which tabs are authoritative.src/**/*.sol; versionabsent from
src; version would downgrade what's deployed). There are 14 live entries in_targetState.jsontoday naming a version older than what is actually deployed, andnothing detects them — but that wants a tested TypeScript script wired into CI, not more
bash. Separate ticket.
exit-inside-$(…)-subshell idiom ingetContractFilePaththat swallows its own harderror — touches every caller in the file.
Verification
No automated test —
parseTargetStateGoogleSpreadsheethas none today and testing it needs astubbed
curlplus a Google Sheet fixture, which is most of the work of the separatevalidator ticket. Verified manually with a stubbed
curlagainst a copy of the real_targetState.json:rc=1, refuses, file untouchedrc=1, lists all 69, file untouched (70 preserved)ALLOW_TARGET_STATE_NETWORK_REMOVAL=trueBlue = Peripheryheader row (incl. an HTML login page)rc=1, refuses, file untouched_targetState.jsonunreadable byjqrc=1, refuses, file untouchedscriptMasteruse case 10checkFailure→ script exits 1The
curlfailure branch was exercised only for hard HTTP failures (stubbed non-zero exit),not for the 302→200-HTML "sheet not shared" case — that path is covered by the empty-parse
guard, verified by feeding the parse loop a real HTML login-page body.
bash -n script/helperFunctions.shclean. No.solor.tstouched, so noforge test/bun test:tsrun. The happy path is unchanged — same parse, same merge, same output.One gotcha worth knowing repo-wide, found while writing the guard:
--arg ENVdoes not workin jq.
$ENVis a jq builtin holding the environment object and shadows the argument, so.value[$ENV]yields nothing instead of erroring usefully — the guard silently passed until Irenamed it to
--arg TARGET_ENV. There's a comment on the line.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)