fix(script): route helper error/warning output to stderr

[0xDEnYO]

Which Linear task belongs to this PR?

Fixes EXSC-734

Why did I implement it this way?

error() and warning() in script/helperFunctions.sh printed to stdout. Many of the helpers that call them also return their value on stdout and are consumed via command substitution, so on the failure path the caller captured the rendered ANSI error text as the value instead of an empty string — silently defeating -z/emptiness guards while the message never reached the console. #2148 fixed one such site (getCurrentContractVersion → CURRENT_VERSION, which ended up 127 characters long instead of empty) by rc-checking and blanking at the call site.

I swept the helper and the deploy scripts for the rest of the class and fixed it at the source instead of per call site. The sweep enumerated every function in script/helperFunctions.sh (plus script/deploy/**/*.sh, script/multiNetworkExecution.sh, script/universalCast.sh, script/scriptMaster.sh) whose body calls error/warning, then grepped for $(...) captures of each: within that scope, 79 capture sites across 20 distinct helpers. Because error/warning are pure output helpers with no callers depending on their stdout, redirecting them to stderr once fixes every current and future capture site, whereas rc-check-and-blank would need repeating at every site and would still leave the next new capture broken. That also means #2148's call-site guard stays correct — it is now belt-and-braces rather than load-bearing.

Five related stragglers in the same class — hand-rolled stdout diagnostics inside value-returning helpers — found by the same sweep:

• checkFailure printed Failed to <msg> to stdout — same swallowing problem when it fires inside a capture.
• findContractVersionInTargetState printed its [info] No matching entry found… miss message to stdout, i.e. into the very variable callers read as the target version. Its call sites all rc-check, so none were broken today, but the message was invisible whenever the capture succeeded in the shell's eyes.
• findContractInMasterLogByAddress printed [info] address not found in MongoDB into the captured RESULT/JSON_ENTRY (script/helperFunctions.sh:824, :1097). Both rc-check (:1097 already uses the fix(deploy): make target-state generation fail loudly instead of silently wiping entries #2148-style || JSON_ENTRY="" workaround), so again invisible rather than broken.

The two [info]-level ones go through a new info() helper rather than a hand-rolled echo … >&2, so all four diagnostic levels now sit together and share one explanation of why they are stderr-bound (per CodeRabbit review). Its contract is deliberately narrow — diagnostics emitted from inside a value-returning helper — because the 43 other [info] echoes in this file are progress output that belongs on stdout and must keep using plain echo. .agents/rules/300-bash.md is updated to list it, since that rule enumerated the logging helpers exhaustively.

• doesAddressContainBytecode hand-rolled echo "[warning]: …" on the invalid-address path, which landed in NEW_DEPLOYMENT at script/deploy/deploySingleContract.sh:192. Switched to the warning helper so it follows the same routing and matches [CONV:BASH-HELPERS].
• getCreate3FactoryAddress hand-rolled echo "Error: create3Factory address not found…" into CREATE3_FACTORY_ADDRESS at script/deploy/deploySingleContract.sh:154 (rc-checked by the following checkFailure). Switched to the error helper, same rationale.

Six helpers (getZkSolcVersion, processNetworkLine, getCoreFacetsArray, normalizePrivateKey, install_foundry_zksync, estimatePauseCost) already appended >&2 to individual error/warning calls — 18 call sites in total. Those are now redundant but harmless — a second >&2 on an already-stderr-bound printf is a no-op — so I left them rather than widen the diff.

Deliberately not changed. success() stays on stdout: no captured helper calls it, so there is no live bug, and changing it would widen the diff for no behavioural gain. findContractInMasterLog (script/helperFunctions.sh:280) is the one straggler I left in place — see the escalation comment below; fixing it correctly means also deleting two now-dead string-match blocks, one of which lives in script/playgroundHelpers.sh, outside this PR's scope.

I checked every consumer that reads these scripts' output as text before making the switch, to be sure nothing depends on the messages arriving on stdout: .github/workflows/deploy-smoke-test.yml (2>&1 | tee), .github/workflows/runPendingTimelockTXs.yml (2>&1 | tee execution.log, then greps for Failed to execute), and the per-network capture in script/multiNetworkExecution.sh:1699 (2>&1 | tee). All three already merge stderr into the captured stream, so error text still reaches the logs and the greps they run. There is no | tee without 2>&1 anywhere in the repo, and no TypeScript consumer parses helper output.

Known trade-off, disclosed. Two call sites suppress stderr with 2>/dev/null, and those redirects were written when our diagnostics went to stdout — so they previously suppressed only forge/cast noise and now also swallow our own error detail: script/helperFunctions.sh:2744 (deploySingleContract … 2>/dev/null on the diamond-bootstrap path) and script/deploy/deploySingleContract.sh:525 (verifyContract … 2>/dev/null). Neither becomes a silent success — both are rc-checked, and :2744's checkFailure runs outside the redirect so Failed to deploy contract … still prints — but the reason is now hidden where it used to be visible. Fixing that means removing the redirects, which re-exposes the tool noise they exist to suppress; that is a separate judgment call, so I left them and flagged it in the review comment rather than deciding it here.

Verification. The strongest evidence is end-to-end on a real call path, not on the helper in isolation. Running the real processNetworkLine (script/helperFunctions.sh:1908) against a nonexistent contract with cell value latest, before vs after (Google-Sheets download stubbed with the CSV line it would have produced; helper, guard and JSON writer are unmodified repo code):

• Before: CURRENT_VERSION captured a ~130-char ANSI blob, the -z "$CURRENT_VERSION" guard did not fire, and the ANSI error text was written into the network target-state JSON ("NonexistentFacetXYZ": "�[31m[error] …"), which mergeNetworkResults would then fold into _targetState.json.
• After: capture empty, guard fires, JSON gets "", and both error lines reach stderr.

Per-helper, sourcing the modified file and exercising both paths directly:

Case	Before	After
getCurrentContractVersion on a bad path	rc=1, captured 127 chars (16-char contract name; length is 111 + len(name))	rc=1, captured 0 chars, [error] on stderr
getCurrentContractVersion "SquidFacet" (happy path)	1.0.0	1.0.0 — unchanged
findContractVersionInTargetState miss	rc=1, captured the 125-char [info] line	rc=1, captured 0 chars, [info] on stderr
findContractVersionInTargetState hit (real _targetState.json, DiamondCutFacet)	1.0.0	1.0.0 — unchanged
doesAddressContainBytecode with empty address	rc=1, captured the 61-char warning	rc=1, captured 0 chars, [warning] on stderr
getCreate3FactoryAddress on an unknown network	rc=1, captured the error text	rc=1, captured 0 chars, [error] on stderr
getCreate3FactoryAddress "mainnet" (real networks.json)	0x93FEC2C0…4AE1	0x93FEC2C0…4AE1 — unchanged

Also ran bash -n script/helperFunctions.sh and bash -c 'set -euo pipefail; source script/helperFunctions.sh' — both clean.

No tests are added: these are shell helpers with no existing bash test harness in the repo, and the behaviour is verified by the executed before/after cases above rather than by a committed test.

Checklist before requesting a review

• I have performed a self-review of my code
• This pull request is as small as possible and only tackles one problem
• I have added tests that cover the functionality / test the bug
• For new facets: I have checked all points from this list: https://www.notion.so/lifi/New-Facet-Contract-Checklist-157f0ff14ac78095a2b8f999d655622e
• I have updated any required documentation

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

• I have checked that any arbitrary calls to external contracts are validated and or restricted
• I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
• I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

[coderabbitai]

Walkthrough

Deployment helper diagnostics now write to stderr. Shared error and warning helpers provide consistent stderr routing for lookup failures, invalid addresses, and missing CREATE3 factory configuration.

Changes

Diagnostic stream routing

Layer / File(s)	Summary
Shared diagnostic output script/helperFunctions.sh	The shared error, warning, and checkFailure helpers now emit diagnostics on stderr.
Lookup diagnostic callers script/helperFunctions.sh	Deployment and target-state lookup messages use stderr. Invalid deployment addresses and missing CREATE3 factory configuration use the shared diagnostic helpers.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes routing helper error and warning output to stderr.
Description check	✅ Passed	The description explains the motivation, implementation, scope, verification, trade-offs, and checklist status in detail.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/cool-kare-4e3157

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[0xDEnYO]

/gate-review — escalated findings (not auto-fixed)

Review gate ran 6 parallel agents (rules adherence, out-of-diff bug hunt, git history, prior-PR comments, code comments, falsification-on-real-data). Two agents returned clean. The falsification agent refuted one of this PR's own claims — the sweep-completeness claim — and found three same-class stragglers. Two were safe and have been auto-fixed in this PR (findContractInMasterLogByAddress, getCreate3FactoryAddress). One is escalated:

1. findContractInMasterLog still prints its miss message to stdout — needs a decision

script/helperFunctions.sh:280 — echo "[info] No matching entry found in MongoDB for CONTRACT=…" followed by return 1. Same class as the rest of this PR: the message is swallowed into the captured value at all 7 call sites (helperFunctions.sh:793, :5744; deploySingleContract.sh:245, :469; playgroundHelpers.sh:53, :306, :580). No guard is broken today — every site rc-checks.

Why escalated rather than auto-fixed: two call sites string-match that exact text out of the captured value as a belt-and-braces check on top of the rc-check:

• script/helperFunctions.sh:5748 — if [[ "$LOG_ENTRY" == *"No matching entry found"* ]]; then FIND_RESULT=1; fi
• script/playgroundHelpers.sh:584 — identical block

Moving the echo to stderr silently deadens both blocks (they also capture with 2>/dev/null, so the text disappears entirely). It is safe — both are backstopped by FIND_RESULT=$? and by the downstream -n "$LOG_ENTRY" test — but doing it properly means deleting the two now-dead # Additional check blocks, one of which is in script/playgroundHelpers.sh, outside this PR's stated scope (helperFunctions.sh + script/deploy/**). That is more than one valid fix and touches a file this PR otherwise doesn't, so the gate's conservative floor says escalate.

Options: (a) fix it here and delete both dead blocks, accepting the scope creep into playgroundHelpers.sh; (b) leave as-is — no live bug; (c) separate follow-up ticket. My recommendation is (c): it is genuinely a different change (removing redundant string-matching guards) from "route diagnostics to stderr".

2. #2148's explanatory comment goes stale when this merges — cross-PR, needs sequencing

#2148 (claude/target-state-generator-guards, still open) adds at script/helperFunctions.sh:2038-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.

Once this PR merges that first sentence is factually wrong, and the || CURRENT_VERSION="" it justifies becomes belt-and-braces rather than load-bearing. The two PRs touch disjoint line ranges, so there is no git merge conflict — nothing forces the comment to be updated. Whoever merges second should update or drop that comment. Flagging rather than editing, because the line lives on the other branch.

Lower-confidence — human judgment

• script/deploy/deploySingleContract.sh:192 — NEW_DEPLOYMENT=$(doesAddressContainBytecode …) is assigned and then never read (pre-existing dead code, confirmed by grep; unrelated to this PR). Candidate for removal in a cleanup pass.
• script/emergency/emergencyPauseBreakGlass.sh:201 and script/utils/verifyEmergencyPauseReadinessGitHub.sh:82 — rpcCallWithRetry deliberately returns error text as the captured value. Intentional by design, out of scope, listed so it isn't mistaken for the same bug later.

Nothing here is critical or security-touching; items 1 and 2 need your decision before merge, not before review.

[0xDEnYO]

/gate-review round 2 — re-gate of the fix commit (68a5e4638)

Per the per-push gate rule, re-ran the gate on the two fixes the first round produced (findContractInMasterLogByAddress, getCreate3FactoryAddress). Fix commit is clean — enumerated every call site repo-wide (3 total, all rc- or emptiness-checked), zero string-matches on either message text anywhere, and executed the real deploySingleContract.sh:154-155 pattern to confirm checkFailure $? receives the substitution's exit code and its exit 1 actually kills the script (UNREACHABLE never printed, outer rc=1). Positive control passed (getCreate3FactoryAddress "mainnet" → 0x93FEC2C0…4AE1). bash -n clean on both touched scripts.

One new finding it surfaced — needs your call (LOW severity, no silent failure)

Two call sites suppress stderr with 2>/dev/null. Those redirects predate this PR and were written when our diagnostics went to stdout — so they used to suppress only forge/cast noise, and now also swallow our own error detail:

• script/helperFunctions.sh:2744 — deploySingleContract … 2>/dev/null (diamond-bootstrap path)
• script/deploy/deploySingleContract.sh:525 — verifyContract … 2>/dev/null

Neither turns into a silent success: both are rc-checked, and at :2744 the checkFailure call sits outside the redirect so Failed to deploy contract <X> to network <Y> still prints. What's lost is the reason — previously visible, now discarded.

Attribution, to be straight about it: this is a consequence of this PR's first commit moving error/warning/checkFailure to stderr, not a pre-existing defect. The blanket 2>/dev/null is pre-existing; the interaction is new.

Options: (a) drop the two redirects — restores the detail but re-exposes the forge/cast noise they exist to suppress; (b) leave as-is — failures still surface, just less informatively; (c) follow-up ticket to make those two sites filter noise instead of discarding everything. I'd take (c), but it's a judgment call about deploy-log verbosity that's yours, not mine, so I left the redirects untouched and disclosed the trade-off in the PR description.

Still open from round 1

Items 1 (findContractInMasterLog at helperFunctions.sh:280, coupled to two dead string-match blocks) and 2 (#2148's now-stale comment at helperFunctions.sh:2038-2040, no merge conflict to force the fix) are unchanged and still need decisions before merge.

[0xDEnYO]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

script/helperFunctions.sh (1)

3270-3277: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the stdout/stderr contract.

Cover error(), warning(), checkFailure(), and the lookup helpers with separate stdout and stderr capture. Assert that returned values remain clean on stdout, diagnostics appear on stderr, and failure statuses remain nonzero.

This protects the contract described in the PR objective.

🤖 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 3270 - 3277, Add regression tests for
error(), warning(), checkFailure(), and the lookup helpers that capture stdout
and stderr separately. Verify stdout contains only the expected returned values,
stderr contains diagnostics, and failure cases retain nonzero exit statuses.

🤖 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 3460-3461: Replace the raw stderr echo diagnostics in
findContractInMasterLogByAddress and findContractVersionInTargetState with the
shared stderr-aware info helper. Preserve the existing [info] severity and
message content for normal lookup misses, adding or reusing an info helper if
necessary; do not route these messages through warning().

---

Nitpick comments:
In `@script/helperFunctions.sh`:
- Around line 3270-3277: Add regression tests for error(), warning(),
checkFailure(), and the lookup helpers that capture stdout and stderr
separately. Verify stdout contains only the expected returned values, stderr
contains diagnostics, and failure cases retain nonzero exit statuses.

🪄 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: a77ad201-28e6-45f9-b2be-45107df4a563

📥 Commits

Reviewing files that changed from the base of the PR and between c72ce47 and 68a5e46.

📒 Files selected for processing (1)

• script/helperFunctions.sh