[Santiagocetran]

Part of #245

Preflight currently warns about some unusable environments but still exits 0. This change makes missing foundry/node_modules and failed imports of eth-account, py-multibase, or web3 contribute to exit 1. Later checks continue running after import failures, including OSError. Torch remains optional.

With --network, missing RPC configuration, unreachable or invalid RPC responses, and unresolved or incomplete IPFS configuration also fail the run. RPC chain-ID results must be valid hexadecimal quantities; a valid but unexpected chain ID remains advisory. IPFS reporting continues after RPC or .env lookup failures, and diagnostics remain credential-safe.

Updates the exit-behavior explanation in Documentation/public/getting-started.md. Preserves the standalone stdlib bootstrap, read-only behavior, plain output, and opt-in network checks.

Validation:

• 79 targeted preflight tests passed using isolated subprocess fixtures and mocked RPC/IPFS I/O.
• Selected regressions against the original script produced 41 expected failures.
• Non-integration suite: 606 passed, 133 deselected, with three dependency warnings.
• All 290 documentation links resolved; git diff --check and stdlib-only --help passed.
• Independent Astra technical audit found no blocking issues.

Live RPC/IPFS services and Solidity/full-GI integration checks were not exercised. Ruff was unavailable. Full JSON-RPC envelope validation remains outside scope.

[umeradl]

Reviewed against develop in an isolated worktree (branch applies cleanly on top of current tip e9424af, which is also the merge-base; 1 commit, no conflicts — confirmed by both git merge-tree --write-tree locally and GitHub's own mergeable: MERGEABLE / mergeStateStatus: BLOCKED, where BLOCKED is only the missing approval).

CI on 89dc9cc: CI OK green — Python runs tests/test_preflight.py as part of the non-integration suite, and Docs checks the getting-started.md links. Reproduced locally: a break-then-fix run of the new tests, and the real script against a real anvil, a malformed RPC, a dead port and a fresh HOME, because the tests mock all RPC/IPFS I/O.

Claimed: a missing foundry/node_modules and failed imports of eth-account, py-multibase or web3 now make preflight exit 1; torch stays optional.

Verified — correct, on the real script: this review worktree has no foundry/node_modules. python3 scripts/preflight.py there prints [WARN] foundry/node_modules not found, then 1 required check(s) failed, exit 1. develop's script in the same environment prints All required checks passed, exit 0. _check_importable returns 0 if optional else 1, so torch can't add to the count, and run() adds every return value to failures.

Claimed: later checks continue after import failures, including OSError.

Verified — correct: _check_importable still catches Exception, which covers OSError from native libs, and returns a count instead of raising. test_import_time_oserror_is_advisory_and_does_not_abort and test_failed_core_import_still_runs_requested_network_checks cover this, and both pass.

Claimed: with --network, missing RPC configuration, an unreachable or invalid RPC and unresolved IPFS config fail the run; a valid but unexpected chain ID stays advisory.

Verified — correct, against live endpoints (LOCAL_RPC_URL set per run, --network local):

RPC endpoint	Output	Counted as a failure
anvil --chain-id 1337	[OK] RPC reachable … (chainId 1337)	No
anvil, default chain ID 31337	[WARN] … chainId 31337, expected 1337	No (advisory)
HTTP server returning {"result":"0xzz"}	[WARN] RPC not reachable	Yes
closed port	[WARN] RPC not reachable	Yes
unset	[WARN] No RPC URL found for local	Yes

The failure count went up by exactly one in the last three rows compared with the first two. The canonical-quantity regex 0x(?:0|[1-9a-fA-F][0-9a-fA-F]*) matches what anvil returns (0x539).

IPFS, with dincli importable: my real config gives [OK] IPFS config resolved (filebase provider) and no extra failure. A fresh HOME (no config.json) gives [WARN] IPFS env provider: IPFS_API_URL_ADD, IPFS_API_URL_RETRIEVE not set, plus one more failure.

Claimed: diagnostics remain credential-safe.

Verified — correct: all three exception-text interpolations were removed: .env read failed: {exc}, IPFS config not resolved: {exc}, and the provider name in the unknown-provider line. RPC URLs still go through _sanitize_url, so every live row above printed http://127.0.0.1:<port>/***. Covered by test_rpc_failure_output_does_not_expose_credentials_or_exception, test_unresolved_ipfs_fails_without_exposing_exception and test_env_lookup_exception_is_sanitized_and_still_checks_ipfs.

Claimed: the new tests catch the original behaviour (the PR body says 41 expected failures on selected regressions).

Verified — break-then-fix, whole file: with develop's scripts/preflight.py swapped into the worktree, pytest tests/test_preflight.py gives 56 failed, 23 passed. With the PR's script restored it gives 79 passed. My count is higher than the PR's 41 because I ran the whole file, not a selection. Either way, the new tests fail on the old exit behaviour.

Claimed: getting-started.md describes the new exit behaviour.

Verified — correct: the new paragraphs list exactly the conditions that run() and _network_checks() now count, including "unexpected chain ID remains a warning". Links are covered by the green Docs job.

Two observations (updated: the first one is required before merge, see the change-request review):

1. Required — the new failures still print [WARN]. In the real run above, the only problem row says [WARN] while the summary says 1 required check(s) failed. The issue's spec ties failure to the label ("Exits non-zero on any MISSING or ERROR row"), and getting-started.md presents [WARN] as the non-fatal level. My status update on issue No. 245 said "only the exit code changes", so the PR does what was asked, and the gap is my mistake, not the author's. But [WARN] now covers both fatal and harmless rows, so the output can't show which row failed. The four newly fatal paths (node_modules, core imports, RPC, IPFS) need to print with _missing(...) so the label agrees with the exit code. Torch and the chain-ID mismatch stay [WARN]. It's about a six-line change; see the merge-proposal table. (This originally said "would" and called the change optional; corrected.)
2. dincli not importable — skipping IPFS config check now counts as a failure. That's the right call, since --network documents that it needs a working dincli install, but "skipping" reads as if nothing failed. Suggested wording: dincli not importable — IPFS config cannot be checked. In this review's venv, which has no dincli install, that line was the extra failure in every --network run.

Not independently re-verified: a real Sepolia RPC or a live Filebase/Kubo upload. The script only calls eth_chainId and resolve_ipfs_config(), and both paths ran for real above against anvil and the local config.

---

Every checkable claim held up. Updated: not mergeable as-is. Observation 1 is required before merge (changes requested); observation 2 should go in the same push. The PR body says Part of #245, which is correct. This PR delivers all three pending items from the issue's status update. The only thing left is the optional dotenv-parity follow-up, so issue No. 245 can go through din-close-issue after merge, and that item can become its own issue.

[umeradl]

Files changed (3) — as of 89dc9cc (PR head)

Diffed against merge-base e9424af (develop). develop hasn't moved since the branch was cut. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BLOCKED (approval only). Confirmed with a local git merge-tree --write-tree dry run: clean. No other open PR in this batch (No. 304, No. 305) touches these files.

scripts/preflight.py

Field	Value
Change	Modified
Lines	+35/-18
Diff (what exactly is in this PR)	_check_node_modules and _check_importable return int, and run() adds them to failures (torch returns 0 via optional=True). _check_rpc returns 1 from its except, and now validates the response: it must be a dict with no error key, and result must match the canonical hex quantity regex. _check_ipfs_config returns 0 only on the three resolved-provider branches and 1 on every other path, including dincli not importable. _network_checks counts a missing RPC URL and moves Path.cwd() inside the .env try. Exception text and the provider name are removed from the warnings.
Functionality — how & why	How: each check prints its row and returns 0/1, and main() exits 1 if the sum is non-zero. No check raises, so one failure never hides the rows after it. Network checks still run only with --network, and the chain-ID mismatch still prints [WARN] and returns 0. Why: issue No. 245's pending items. Before this PR, a contributor with no npm ci, a broken venv or a dead RPC got All required checks passed and exit 0, then hit the npx race in forge test or a failure in dincli later.
Diff vs current develop HEAD	None — untouched since merge-base.
Recommended merge proposal	Merge after the label fix in Pending (required; changes requested). Verified: on the real script, a missing node_modules exits 1 where develop exits 0. With --network local, a live anvil at 1337 is OK, a 31337 mismatch is advisory, and a malformed 0xzz RPC, a closed port and an unset URL each add one failure. A fresh HOME with no IPFS config adds one failure.
Actual merge proposal	Merged as-is in e25df81 (GitHub merge commit, umeradl). Review items fixed by the author in e22e360; no reviewer fix commits.
Pending proposal	Done in e22e360 (re-review). Failing rows now print [MISSING] via _missing(...), torch and the chain-ID mismatch stay [WARN], and the dincli message is reworded.
Local merge conflict	No
GitHub merge conflict	No

tests/test_preflight.py

Field	Value
Change	Modified
Lines	+339/-60
Diff (what exactly is in this PR)	27 test functions, 79 collected with parametrisation (develop has 14 functions). Standalone subprocess fixtures cover a missing node_modules, a failing required import that continues, an import-time OSError and the torch-optional success path. Mocked urlopen/resolve_ipfs_config cases cover invalid RPC responses (non-dict, error key, non-canonical hex), canonical quantities, mismatch-advisory, per-provider IPFS fields, unimportable dincli, .env lookup exceptions, and network failures reaching main()'s exit code.
Functionality — how & why	How: runs the script both as a subprocess (real exit codes, stdlib-only bootstrap) and in-process with monkeypatch for network I/O, so no live RPC or IPFS is touched. Why: pins every new exit-1 path, and the sanitised-output guarantees, so a later refactor can't make them advisory again unnoticed.
Diff vs current develop HEAD	None — untouched since merge-base.
Recommended merge proposal	Merged as-is. Break-then-fix: against develop's script, 56 failed / 23 passed; against the PR's script, 79 passed. Also green in CI Python.
Actual merge proposal	Merged as-is in e25df81 (GitHub merge commit, umeradl). Review items fixed by the author in e22e360; no reviewer fix commits.
Pending proposal	Done in e22e360 (re-review). Label assertions updated, 79/79 pass.
Local merge conflict	No
GitHub merge conflict	No

Documentation/public/getting-started.md

Field	Value
Change	Modified
Lines	+9/-1
Diff (what exactly is in this PR)	Replaces "exits 1 on Python version or Foundry failures" with the full list of exit-1 conditions, and adds a paragraph on what --network failures do.
Functionality — how & why	How: the text now matches run()/_network_checks() exactly, including torch-optional and mismatch-advisory. Why: the issue's status update asked for this line to change together with the exit code.
Diff vs current develop HEAD	None — untouched since merge-base.
Recommended merge proposal	Merged as-is. If the _missing fix is applied, the sentence above the table ("prints [OK], [WARN], or [MISSING]") already fits.
Actual merge proposal	Merged as-is in e25df81 (GitHub merge commit, umeradl). Review items fixed by the author in e22e360; no reviewer fix commits.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

Verification

CI on 89dc9cc: CI OK green (Python, Solidity, Docs and Integration paths green; Integration (full GI) skipped by path filter). Locally: break-then-fix of tests/test_preflight.py (56 → 0 failures), plus the real script against a live anvil (two chain IDs), a malformed-RPC server, a closed port, an unset URL, and both a configured and an empty IPFS config. Full detail is in the verification comment above.

Local vs. GitHub agree: both clean.

[umeradl]

Approved at 89dc9cc: exit-1 paths verified on the real script (missing node_modules; with --network: live anvil, malformed RPC, closed port, unset URL, empty IPFS config); break-then-fix 56 failed -> 79 passed. Optional at merge: print the newly fatal rows as [MISSING] (see the merge-proposal comment).

[umeradl]

Changing my review: the label fix I called optional is required. The newly fatal checks still print [WARN], the same label as the harmless ones (torch, chain-ID mismatch), so the output doesn't show what failed. That's my mistake: my note on issue No. 245 said "only the exit code changes". Please:

1. Use _missing(...) instead of _warn(...) for the rows that now count as failures: foundry/node_modules, the three core imports, both RPC failure branches, and the IPFS failure branches. Keep [WARN] for torch and the chain-ID mismatch.
2. Reword dincli not importable — skipping IPFS config check to … IPFS config cannot be checked.
3. Update the [WARN] assertions in tests/test_preflight.py for those rows.

Everything else in the PR is verified and fine. I've updated the verification and merge-proposal comments to match.

[Santiagocetran]

Thanks for the thorough review, @umeradl!

Pushed the fixes in e22e360316eb20cf686f8e4e0d5e3c70b3ab9336. Failed checks now print [MISSING], keeping [WARN] for optional torch and unexpected chain IDs. The dincli import message now clearly says the IPFS configuration cannot be checked.

The matching assertions are updated, and all 79 preflight tests passed in an isolated environment with mocked network I/O. Fresh CI and your re-review of the new head are still needed.

[umeradl]

Re-reviewed the fix commit: 89dc9cc (reviewed) → e22e360 (current HEAD). The commit touches only scripts/preflight.py (+10/-10) and tests/test_preflight.py (+67/-17). The branch still merges cleanly into develop: git merge-tree --write-tree exits 0, and GitHub reports MERGEABLE / BEHIND. develop's 2 new commits (the CI workflow split) don't touch this PR's files. CI on e22e360: CI OK green.

Requested 1, _missing(...) on the rows that now fail. Done. git diff 89dc9cc e22e360 -- scripts/preflight.py converts exactly these rows: foundry/node_modules, the core-import branch of _check_importable, RPC not reachable, No RPC URL found, the env/filebase/custom/unknown-provider IPFS branches, and both IPFS except branches. Torch, the chain-ID mismatch and .env lookup failed are untouched and still [WARN], and none of them adds to failures. On the real script in this worktree, which has no foundry/node_modules, the output is now [MISSING] foundry/node_modules not found …, then 1 required check(s) failed., exit 1. The label now matches the exit code.

Requested 2, reword the dincli-import message. Done: dincli not importable — IPFS configuration cannot be checked.

Requested 3, test assertions. Done: pytest tests/test_preflight.py gives 79 passed. Against develop's scripts/preflight.py the same file gives 56 failed / 23 passed, so the tests still catch the old behaviour.

Re-ran on a live anvil (--chain-id 1337, real urllib):

Case	Row	Fails?
LOCAL_RPC_URL=http://127.0.0.1:18545	[OK] RPC reachable: http://127.0.0.1:18545/*** (chainId 1337)	no
Closed port	[MISSING] RPC not reachable: http://127.0.0.1:18599/***	yes
URL unset	[MISSING] No RPC URL found for local …	yes
IPFS_API_URL_RETRIEVE unset (env provider)	[MISSING] IPFS env provider: IPFS_API_URL_RETRIEVE not set	yes. It still ran after the RPC failure.
Chain 1337 under --network sepolia_op_devnet	[WARN] RPC reachable: … (chainId 1337, expected 11155420 …)	no

---

New observation, not blocking this PR: an RPC URL with credentials in it fails preflight even though dincli accepts it. urllib doesn't strip userinfo from the URL, so http://user:pass@host fails DNS resolution, while web3 (and so dincli) connects. Tested against the same anvil:

LOCAL_RPC_URL=http://user:secret@127.0.0.1:18545
preflight → [MISSING] RPC not reachable: http://127.0.0.1:18545/***   (exit 1)
urllib    → URLError <urlopen error [Errno -2] Name or service not known>
web3      → is_connected() == True, chain_id == 1337

This PR didn't cause it: on develop it's already an advisory [WARN], and this PR makes it fatal. It only shows up under --network, with providers that put credentials in the URL. It's a follow-up, along with the dotenv/config-parity item from the earlier review (config.json → networks.<net>.rpc_url is another fallback dincli has and preflight doesn't). The fix: split the userinfo off with urlsplit, send it as an Authorization: Basic header, and add a test that checks the header is sent and the credentials stay out of the output. A nit for the same follow-up: say which provider in IPFS unknown provider, since the name isn't a credential.

Every requested change is in, and every claim holds on e22e360. Good to merge.

[umeradl]

Re-reviewed 89dc9cc → e22e360: all three requested changes are in (failing rows print [MISSING], dincli message reworded, assertions updated). 79/79 tests pass, live-anvil checks pass, CI green. The credentialed-URL case is a follow-up, not a blocker (see the re-review comment).

[umeradl]

@Santiagocetran approved at e22e360, thanks for the quick fixes. One step before merge: the develop ruleset requires the branch to be up to date with develop, and this one is 2 commits behind. Those commits are the CI workflow split, and they don't touch any of this PR's files.

Could you click Update branch (merge commit, not rebase) or merge develop into Santiagocetran/fix-preflight-exit-245 yourself? It needs to come from you: the last-push rule means a push by me would void my own approval. Once CI OK is green on the new head, I'll re-approve and merge.

[umeradl]

Actual outcome: PR No. 303 merged as-is

This supersedes the merge-proposal comment above with what actually happened.

One merge commit, on origin/develop:

1. e25df81: the real merge of this PR (a merge commit, not a squash). umeradl merged it on GitHub at 2026-10-30 16:25 UTC, after umeradl's approval of head 3d5998f. Its parents are 31e3455 (develop) and 3d5998f (PR head). There were 0 conflicts, as predicted. GitHub agrees: the PR shows MERGED.

The reviewer pushed no fix commits. The review items were fixed by the author in e22e360 (re-review). 3d5998f is the author's own "Update branch" merge of develop; it brought in only develop's files and left the PR's diff against develop unchanged (3 files, +447/−93).

Files unchanged from the PR (3 of 3)

• scripts/preflight.py
• tests/test_preflight.py
• Documentation/public/getting-started.md

Verification

• PR CI on the merged head 3d5998f: CI OK green (Python, Docs, Solidity).
• The develop push run for e25df81: CI push run green.

Local vs. GitHub agree: both predicted a clean merge, and it was one.

Follow-ups:

• Issue No. 306: credentialed RPC URLs, config.json / dotenv lookup parity, and the IPFS provider name.
• The issue 245 entry in Developer/GOOD_FIRST_ISSUES.md is now done and should be removed.
• Issue No. 245 itself still has the solc-version check and the ci.yml usage left.