Repository navigation
feat(tooling): scripts/preflight.py — contributor environment preflight check (issue #245) - #255
Conversation
…ht check (InfiniteZeroFoundation#245) stdlib-only script that runs before pip install and checks: 1. Python >= 3.12 2. forge / anvil on PATH (MISSING → exit 1) 3. foundry/node_modules present (WARN → npm ci) 4. eth-account, py-multibase, web3 importable (WARN → pip install -e .) 5. torch importable (WARN optional → pip install -e .[test]) 6. --network <name>: .env.<name> exists, RPC chainId reachable, IPFS config resolves via dincli (all WARN, never hard failures) Other changes: - .gitignore: carve out scripts/preflight.py from the /scripts/* exclusion - tests/test_preflight.py: 5 tests; missing-forge → exit nonzero (empty PATH); happy-path → exit zero in a working dev environment - Documentation/public/getting-started.md: preflight section before DIN CLI Installation and Setup - Developer/BACK_LOG.md: BL-32 added as ✅ Fixed — PR #<n>
06f6d77 to
959c012
Compare
|
Cc: @umeradl |
Santiagocetran
left a comment
There was a problem hiding this comment.
Thanks, Robbert—this should make environment setup easier. I reviewed 232d52e against the PR description and issue #245’s approved amendments.
Checks performed:
- Ran preflight and the three regression files listed in the PR: 49 passed, 2 skipped on Python 3.13.5.
- Exercised tool detection with temporary forge/anvil placeholders: all five preflight tests passed; removing either tool produced exit 1 and the expected guidance.
- Tested dotenv parsing against a localhost RPC server and ran the script with Python 3.9.25.
- Simulated import failures, RPC responses/timeouts, and IPFS exceptions.
- Inspected CI (https://github.com/InfiniteZeroFoundation/DevNet/actions/runs/38714826868): all checks green, with 471 Python and 501 Solidity tests passed.
A few findings from the review, ordered by impact:
- P2 — Keep import failures advisory (lines 73–87 ()). A simulated torch OSError escaped the ImportError handler and stopped the checker. Could we handle ordinary import-time exceptions as warnings and continue, consistent with the documented behavior?
Lines 73 to 87 in 232d52e
- P2 — Sanitize RPC diagnostics (lines 147–150 ()). Both outcomes printed full URLs containing fake API credentials. Could we sanitize these URLs and exception messages so credentials don’t end up in shared logs?
Lines 147 to 150 in 232d52e
- P3 — Use dotenv-compatible parsing (lines 126–133 ()). Quotes, export, whitespace around =, and some inline comments cause incorrect parsing or false RPC warnings. Using dotenv-compatible parsing would align this with the configuration syntax dincli accepts.
Lines 126 to 133 in 232d52e
- P3 — Continue independent IPFS checks (lines 96–101 ()). Missing .env. currently skips IPFS entirely. Could we continue that independent check after warning, and reference the existing .env.example in the hint?
Lines 96 to 101 in 232d52e
- P3 — Allow the Python-version diagnostic to run (line 162 ()). Python 3.9.25 raises TypeError on str | None before displaying upgrade guidance. Postponing annotations would allow the intended diagnostic to run.
Line 162 in 232d52e
RPC/IPFS exceptions correctly remained warning-only in the simulated checks.
Local checks covered detection, parsing, and failure handling using placeholders, a localhost server, and simulated errors. Real Foundry and external RPC/IPFS operation were not tested locally; Python 3.12 results come from CI.
The two main things I’d like to see addressed before merging are import-failure handling and credential-safe output. Focused regression tests would help prevent these from coming back. The remaining findings are listed above for discussion. Thanks for putting this together; the preflight check should be a useful addition to onboarding
|
Reviewed against Claims in the PRClaimed: 5/5 tests pass; exit 0 in a working env; exit 1 + Claimed: Claimed: stdlib-only core checks, Santiago's findings: all five reproduced
New findingsNo. 1 (P2): No. 2 (P2): the IPFS check can't fail. No. 3 (P3): any chain passes the RPC check. A stub returning No. 4 (P3): the doc section's audience. The issue 245 approval (amendment 5) pointed at No. 5 (P3): No. 6 (P3, tests): the happy path is gated on forge only and never runs in CI. Nit: Not independently re-verified: a real external RPC and a real Filebase/IPFS node (local stubs and an unreachable host only); Python 3.9 (no interpreter on this machine, see No. 5 in the table). Every claim in the PR description held up as written. The script is a good shape for this, and the stdlib-only/ |
Files changed (5) — as of
|
| Field | Value |
|---|---|
| Change | New |
| Lines | +210/-0 |
| Diff (what exactly is in this PR) | A stdlib-only checker with [OK]/[WARN]/[MISSING] output helpers. Required checks: Python ≥ 3.12, forge, anvil (via shutil.which). Advisory checks: foundry/node_modules, importability of eth_account/multibase/web3, optional torch. --network NAME adds an .env.NAME file check, an eth_chainId POST via urllib, and dincli.services.ipfs.resolve_ipfs_config(). Exits 1 if any required check fails. |
| Functionality — how & why | How: run() sums the 0/1 returns of the three required checks; the advisory checks only print. --network reads <NETWORK>_RPC_URL from <repo root>/.env.<network> with a line-prefix match, POSTs eth_chainId with a 5 s timeout (always WARN-only), then imports dincli lazily to call resolve_ipfs_config(). Why: BL-32 / issue 245. Python version, missing Foundry, missing npm ci and broken venvs have each cost onboarding time, and dincli itself can't diagnose any of them because it won't import in exactly those cases (issue 245 amendment 1). |
Diff vs current develop HEAD |
None (new file) |
| Recommended merge proposal | Needs fixes before merge. P2: S-No. 1 (catch Exception on import, keep it a WARN, never exit 1 for an advisory check); S-No. 2 (redact path/query/userinfo from the URL and the exception text, in both the OK and WARN lines); No. 1 (resolve <NETWORK>_RPC_URL like get_env_key: os.environ → Path.cwd()/.env; hint → .env.example); No. 2 (validate the resolved IPFSConfig's per-provider required fields; fix the IPFS_NODE_URL hint). P3: S-No. 3 (dotenv-compatible parsing), S-No. 4 (don't return before the IPFS check), S-No. 5 (from __future__ import annotations), No. 3 (expected chainId: sepolia_op_devnet → 11155420, local → 1337). Nit: _check_rpc returns nothing useful. |
| Actual merge proposal | Soon |
| Pending proposal | Author fix round for the items above |
| Local merge conflict | No |
| GitHub merge conflict | No |
tests/test_preflight.py
| Field | Value |
|---|---|
| Change | New |
| Lines | +56/-0 |
| Diff (what exactly is in this PR) | 5 subprocess tests: 3 run with PATH=<empty tmp dir> and assert exit ≠ 0, [MISSING], and a Foundry hint; 2 happy-path tests assert exit 0 and the success message, skipped unless forge is on PATH. |
| Functionality — how & why | How: each test runs sys.executable scripts/preflight.py in a subprocess with an overridden env, so the exit code is real, not mocked. Why: issue 245 amendment 5 asked for a test showing a missing tool → non-zero exit. |
Diff vs current develop HEAD |
None (new file) |
| Recommended merge proposal | Needs fixes (No. 6): make the happy path hermetic with forge/anvil placeholders in tmp_path and drop the skipif. Today it fails with forge-but-no-anvil and always skips in CI (492 passed, 2 skipped). Add regressions for S-No. 1, S-No. 2, No. 1 and No. 2. |
| Actual merge proposal | Soon |
| Pending proposal | Author fix round |
| Local merge conflict | No |
| GitHub merge conflict | No |
Documentation/public/getting-started.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +30/-0 |
| Diff (what exactly is in this PR) | New "🩺 Environment Preflight Check" section just before "⚙️ DIN CLI Installation and Setup": usage, a check/pass-condition table, exit-code note, and the --network examples. |
| Functionality — how & why | How: docs only; tells readers to run the script before installing dincli. Why: issue 245 amendment 5 (onboarding doc should reference the command). |
Diff vs current develop HEAD |
None; untouched on develop since the merge-base |
| Recommended merge proposal | Merge with a fix (No. 4): add a one-line scope note that this is for contributors working from a clone. This page's participants install the wheel per setup.md and have neither the script nor Foundry. Once No. 1 lands, update the --network sentence ("verify .env.<network>") to match whatever the script actually reads. |
| Actual merge proposal | Soon |
| Pending proposal | Scope wording; --network sentence follows the No. 1 fix |
| Local merge conflict | No |
| GitHub merge conflict | No |
.gitignore
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-0 |
| Diff (what exactly is in this PR) | !scripts/preflight.py right after /scripts/* |
| Functionality — how & why | How: negation re-includes the one file while every other local scripts/* stays ignored. Why: without it the new script couldn't be committed. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merged as-is. Verified: git check-ignore -v scripts/preflight.py → not ignored; git ls-files scripts lists it. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Developer/BACK_LOG.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff (what exactly is in this PR) | BL-32 row: description trimmed (drops the "Proposal:" sentence), Source cell gains 03-drift-detection and loses the bridge-tx-preflight note, Status 💭 Idea → ✅ Fixed — PR 255 …. |
| Functionality — how & why | How: backlog bookkeeping only. Why: marks BL-32 as resolved by this PR. |
Diff vs current develop HEAD |
None in this row (only the two develop merges touched this file's other rows) |
| Recommended merge proposal | Needs a fix (No. 5): revert the Source cell. 03-drift-detection is BL-33's source; preflight.sh is at the top level of that repo. Keep the bridge-preflight note. Refresh the status text at merge time so it describes what actually lands (test count, --network behavior). |
| Actual merge proposal | Soon |
| Pending proposal | Status wording at merge |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
pytest tests/test_preflight.py → 5 passed (Python 3.12.3); forge-without-anvil → 2 failed, 3 passed; empty PATH → exit 1; default run → exit 0; Python 3.10/3.11 → version [MISSING], exit 1. Every finding was reproduced with local stubs (JSON-RPC server, fake torch, empty HOME). Commands and output are in the previous comment. CI on ba745bc: all four jobs green.
Local vs. GitHub agree: yes. Clean locally (merge-tree exit 0) and MERGEABLE on GitHub; the only blocker is review.
P2 fixes - Catch Exception (not just ImportError) in _check_importable so native-lib OSErrors remain advisory warnings instead of crashing the checker - Sanitize RPC URLs before printing: _sanitize_url() strips path/query/ credentials; exception text is never printed (no credential exposure in either OK or WARN paths) - Resolve <NETWORK>_RPC_URL via os.environ → .env, matching dincli's own get_env_key lookup; dincli never reads .env.<network>; hint now points at .env.example (file that exists) not .env.<network>.example (does not exist) - Extract _check_ipfs_config() and validate per-provider required fields (env: IPFS_API_URL_ADD + IPFS_API_URL_RETRIEVE; filebase: api_key; custom: service_path); fix hint from IPFS_NODE_URL to the real vars P3 fixes - Add from __future__ import annotations so str | None in run() signature is postponed and the Python-version diagnostic runs on 3.9 - Dotenv-compatible parsing in _read_env_value: handles export prefix, spaces around =, single/double quotes, inline comments - IPFS check no longer skipped when env file is missing; both checks run independently after the env file warning - Expected chainId map (sepolia_op_devnet: 11155420, local: 1337); WARN on mismatch - Doc scope note: for contributors from a clone; wheel participants skip - --network paragraph updated to match actual env resolution - BACK_LOG.md BL-32 Source cell: remove 03-drift-detection (that is BL-33's source); restore bridge-preflight note; update test count to 13 Tests - TestHappyPath: drop skipif; use forge/anvil executable stubs in tmp_path prepended to PATH — hermetic and runs in CI - All 13 tests pass
|
All findings from both reviews have been addressed in P2 — S-No. 1 (import-failure handling) P2 — S-No. 2 (credential-safe output) P2 — No. 1 (wrong env file) P2 — No. 2 (IPFS check always passes) P3 — S-No. 3 (dotenv parsing) P3 — S-No. 4 (IPFS skipped on missing env file) P3 — S-No. 5 (Python 3.9 annotation crash) P3 — No. 3 (any chainId passes) P3 — No. 4 (doc audience) P3 — No. 5 (BL-32 Source cell) No. 6 (happy-path tests not hermetic) |
|
Cc: @Santiagocetran changes fixed can you review again |
Santiagocetran
left a comment
There was a problem hiding this comment.
Thanks, Robbert. Rechecked 8cc61f8 against my previous review and Umer’s follow-up. The import-failure handling and RPC credential redaction are fixed. The chain-ID warning, contributor scope note and hermetic happy-path tests are also in place.
Umer’s configuration finding is only partly addressed: the script reads the repository root’s .env, while dincli reads .env from the current working directory. Please use the same environment-variable/current-directory lookup he requested. Also remove the warning and copy hint for .env., since this check doesn’t read that file. A regression with different repository-root and working-directory configurations would cover this.
Two smaller cases remain worth fixing:
- Unknown IPFS providers currently fall through to an OK message, although the IPFS operations reject them. Please report a warning instead.
- A .env read or decoding failure escapes the handler and prevents the independent IPFS check from running. Please keep this advisory, warn safely and continue.
Please add focused regressions and sync with current develop for fresh CI. The configuration mismatch remains the main point before approval; the original import and credential-output blockers are resolved.
This follow-up is based on source inspection, observed CI and existing exact-head offline checks, not live RPC/IPFS testing.
…vider, decode safety)
Configuration mismatch (main finding):
_network_checks now reads .env from Path.cwd() / ".env" instead of
_REPO_ROOT / ".env", matching dincli's own lookup (Path(os.getcwd()) /
".env" in dincli/cli/utils.py). The .env.<network> existence warning is
removed — the script never reads that file.
Unknown IPFS provider:
The else branch in _check_ipfs_config now calls _warn() instead of _ok().
An unrecognised provider will be rejected by dincli's IPFS operations, so
reporting OK was misleading.
.env decode safety:
_read_env_value is now wrapped in a try/except inside _network_checks.
A file-read or encoding error becomes a [WARN] and the independent IPFS
check continues.
Tests:
- test_missing_network_env_still_checks_ipfs: updated to use
monkeypatch.chdir instead of the removed _REPO_ROOT patch; now checks
"No RPC URL found" rather than the removed .env.<network> warning.
- test_env_read_from_cwd_not_repo_root: regression for the cwd vs repo-
root divergence — places .env only in a tmp cwd and asserts the URL
reaches _check_rpc.
- test_env_read_failure_still_runs_ipfs_check: regression for the
decode-safety fix — a binary .env triggers a [WARN] and the IPFS
check still runs.
- test_unknown_ipfs_provider_warns: regression for the provider fallthrough
fix — an unknown provider produces [WARN], not [OK].
All 16 tests pass (Python 3.12).
|
Addressed in Configuration mismatch (main finding)
Unknown IPFS provider The .env decode safety
Regressions added (all pass)
All 16 preflight tests pass. |
|
Cc: @umeradl |
Santiagocetran
left a comment
There was a problem hiding this comment.
Thanks, Robbert. Rechecked e8ad0d3 against the remaining findings. The current-directory .env lookup, removal of the unused .env. warning, unknown-provider warning and safe continuation after .env read/decode failures are all addressed, with focused regressions. The earlier import-failure handling and credential-safe RPC output remain intact.
CI passed with this head merged into current develop. This recheck is based on source inspection and CI evidence, not live RPC/IPFS testing. No blocking findings remain. Approved.
|
Per-file merge proposal for e8ad0d3 This refreshes the earlier proposal. Compared head e8ad0d3 against current develop ee99619: 8 commits ahead, 0 behind. Recommendation: merge the five-file diff as-is using a merge commit.
For every file above: the diff is against current develop, which equals the merge-base, so there is no intervening base-side change to reconcile. GitHub reports the PR mergeable. Local merge-conflict analysis was not rerun for this refresh. Actual merge proposal: Soon, pending the merge; no actual merge SHA or deviation is claimed yet. CI run 40472366162 tested merge d741a77: Python 517 passed/133 integration tests deselected, Forge 516 passed, Hardhat 32 passing, docs 289 links checked, CI OK successful. Advisory lint is not clean. Evidence for this refresh is source inspection, current-base comparison and observed CI. No local tests, local merge simulation or live RPC/IPFS checks were rerun. The actual merge SHA, deviations and post-merge CI results belong in the outcome record after merging. |
06c0af5
into
InfiniteZeroFoundation:develop
|
Merged as-is into develop via 06c0af5. No files deviated from reviewed head e8ad0d3. Post-merge CI 40506732210 passed: Python 517 passed, 133 integration tests deselected; Forge 516 passed; Hardhat 32 passing; docs 289 links checked; CI OK successful. Advisory lint remains non-gating and is not clean. These are observed CI results, not local or live RPC/IPFS tests. |
Closes #245
Summary
scripts/preflight.py: a stdlib-only script contributors run beforepip installto verify their dev environment. Exits 0 (all required checks passed) or 1 (Python version wrong or Foundry not installed).scripts/preflight.pyout of the existing/scripts/*gitignore entry.Documentation/public/getting-started.md.tests/test_preflight.py.What it checks
[MISSING]→ exit 1forgeon PATH[MISSING]→ exit 1anvilon PATH[MISSING]→ exit 1foundry/node_modulespresent[WARN]→cd foundry && npm cieth-account,py-multibase,web3importable[WARN]→pip install -e .torchimportable[WARN]→pip install -e .[test](optional)Pass
--network <name>to also check.env.<name>, the configured RPC endpoint (via stdliburllib), and the IPFS config (viadincli.services.ipfs.resolve_ipfs_config()). All network checks are[WARN], never hard failures.python3 scripts/preflight.py python3 scripts/preflight.py --network sepolia_op_devnet python3 scripts/preflight.py --network localDesign notes (per review amendments)
pip install, before dincli is importable.torchis[WARN](optional test extra), not[MISSING].--network— a fresh clone with no.envfile exits 0 by default.Documentation/public/getting-started.md.solc-selectnot mentioned — Foundry manages solc itself.Test plan
python3.12 -m pytest tests/test_preflight.py -v— 5/5 passpython3.12 -m pytest tests/test_develop_small_backports.py tests/test_dintoken.py tests/test_ipfs_config.py -q— all pass (no regressions)python3 scripts/preflight.pyin a working dev environment — exits 0python3 scripts/preflight.pywith empty PATH — exits 1, prints[MISSING]+ foundryup hintConflicts
No overlapping files with known open PRs. PR No. 72 touches
Documentation/public/getting-started.md— if that PR lands first, rebase before merge; the preflight section is additive and appended before the install section, so merge should be mechanical.Closes #245