Skip to content

feat(tooling): scripts/preflight.py — contributor environment preflight check (issue #245) - #255

Merged
Santiagocetran merged 8 commits into
InfiniteZeroFoundation:developfrom
robertocarlous:feat/issue-245-preflight-check
Oct 27, 2026
Merged

Santiagocetran merged 8 commits into
InfiniteZeroFoundation:developfrom
robertocarlous:feat/issue-245-preflight-check

Conversation

@robertocarlous

@robertocarlous robertocarlous commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #245

Summary

  • Adds scripts/preflight.py: a stdlib-only script contributors run before pip install to verify their dev environment. Exits 0 (all required checks passed) or 1 (Python version wrong or Foundry not installed).
  • Carves scripts/preflight.py out of the existing /scripts/* gitignore entry.
  • Adds a preflight section to Documentation/public/getting-started.md.
  • 5 tests in tests/test_preflight.py.

What it checks

# Check Outcome if missing
1 Python ≥ 3.12 [MISSING] → exit 1
2 forge on PATH [MISSING] → exit 1
3 anvil on PATH [MISSING] → exit 1
4 foundry/node_modules present [WARN] → cd foundry && npm ci
5 eth-account, py-multibase, web3 importable [WARN] → pip install -e .
6 torch importable [WARN] → pip install -e .[test] (optional)

Pass --network <name> to also check .env.<name>, the configured RPC endpoint (via stdlib urllib), and the IPFS config (via dincli.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 local

Design notes (per review amendments)

  • stdlib-only for all non-network checks — runs before pip install, before dincli is importable.
  • torch is [WARN] (optional test extra), not [MISSING].
  • Network checks behind --network — a fresh clone with no .env file exits 0 by default.
  • No CI integration claims — the script is advisory/onboarding only; CI is out of scope.
  • Doc path corrected to Documentation/public/getting-started.md.
  • solc-select not mentioned — Foundry manages solc itself.

Test plan

  • python3.12 -m pytest tests/test_preflight.py -v — 5/5 pass
  • python3.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.py in a working dev environment — exits 0
  • python3 scripts/preflight.py with empty PATH — exits 1, prints [MISSING] + foundryup hint

Conflicts

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

robertocarlous added a commit to robertocarlous/DevNet that referenced this pull request Oct 15, 2026
…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>
@robertocarlous
robertocarlous force-pushed the feat/issue-245-preflight-check branch from 06f6d77 to 959c012 Compare October 15, 2026 08:27
@robertocarlous

Copy link
Copy Markdown
Collaborator Author

Cc: @umeradl

@Santiagocetran Santiagocetran left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. P2 — Keep import failures advisory (lines 73–87 (
    def _check_importable(module: str, package: str, optional: bool = False) -> None:
    try:
    importlib.import_module(module)
    _ok(f"{package} importable")
    except ImportError:
    if optional:
    _warn(
    f"{package} not importable (optional — pytest skips torch-dependent tests without it)",
    fix="pip install -e .[test]",
    )
    else:
    _warn(
    f"{package} not importable",
    fix="pip install -e .",
    )
    )). 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?
  2. P2 — Sanitize RPC diagnostics (lines 147–150 (

    DevNet/scripts/preflight.py

    Lines 147 to 150 in 232d52e

    _ok(f"RPC reachable: {url} (chainId {chain_id})")
    return 0
    except Exception as exc:
    _warn(f"RPC not reachable ({url}): {exc}", fix="Check your RPC URL and internet connection")
    )). 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?
  3. P3 — Use dotenv-compatible parsing (lines 126–133 (

    DevNet/scripts/preflight.py

    Lines 126 to 133 in 232d52e

    def _read_env_value(env_file: Path, key: str) -> str:
    prefix = key + "="
    with open(env_file) as fh:
    for line in fh:
    stripped = line.strip()
    if stripped.startswith(prefix):
    return stripped[len(prefix):].strip()
    return ""
    )). 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.
  4. P3 — Continue independent IPFS checks (lines 96–101 (
    if not env_file.exists():
    _warn(
    f".env.{network} not found",
    fix=f"Copy .env.{network}.example and fill in your RPC URL and private keys",
    )
    return failures # can't proceed without the env file
    )). Missing .env. currently skips IPFS entirely. Could we continue that independent check after warning, and reference the existing .env.example in the hint?
  5. P3 — Allow the Python-version diagnostic to run (line 162 (
    def run(network: str | None = None) -> int:
    )). Python 3.9.25 raises TypeError on str | None before displaying upgrade guidance. Postponing annotations would allow the intended diagnostic to run.

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

@umeradl

umeradl commented Oct 21, 2026

Copy link
Copy Markdown
Member

Reviewed against develop in an isolated worktree at PR head ba745bc. The branch is 4 commits ahead of develop tip eb8085d and 0 behind, with no conflicts. Both a local git merge-tree --write-tree and GitHub (mergeable: MERGEABLE, mergeStateStatus: BLOCKED, waiting on review) agree. This builds on @Santiagocetran's review of 232d52e. The only commits since 232d52e are the two develop merges (4003515, ba745bc), and scripts/preflight.py, tests/test_preflight.py, the doc and .gitignore are byte-identical between the two SHAs. So all five of his findings still apply. I re-ran them independently below and then added what I found on top.

Claims in the PR

Claimed: 5/5 tests pass; exit 0 in a working env; exit 1 + [MISSING] + foundryup hint with an empty PATH.
Verified, holds: pytest tests/test_preflight.py → 5 passed (Python 3.12.3, forge/anvil installed). PATH=<empty dir> python scripts/preflight.py → [MISSING] forge, [MISSING] anvil, 2 required check(s) failed, exit 1. A default run in a real env exits 0 (with the expected foundry/node_modules WARN before npm ci). CI run 39658989179 is green: 492 passed, 2 skipped. The 2 skips are this PR's two happy-path tests. The Python job has no forge, so the exit-0 path never runs in CI (see No. 6 below).

Claimed: scripts/preflight.py is carved out of the /scripts/* ignore.
Verified, holds: git check-ignore -v scripts/preflight.py → not ignored (exit 1); git ls-files scripts → scripts/preflight.py.

Claimed: stdlib-only core checks, torch is WARN, network checks behind --network, no CI claims, doc path fixed, no solc-select (issue 245 amendments 1–5).
Verified, holds as written, apart from the gaps below: imports at the top are stdlib only, dincli is imported lazily inside --network, and torch is reported via _warn.

Santiago's findings: all five reproduced

No. Finding My reproduction
1 (P2) Non-ImportError at import crashes the checker Fake torch/__init__.py raising OSError("libtorch_cuda.so: cannot open shared object file") on PYTHONPATH → uncaught traceback, exit 1. That's worse than a crash: exit 1 is the script's "required check failed" code, so an optional dependency fails the run.
2 (P2) Credentials printed https://opt-sepolia.g.alchemy.com/v2/SECRETKEY123 → [WARN] RPC not reachable (https://opt-sepolia.g.alchemy.com/v2/SECRETKEY123): HTTP Error 401. http://user:hunter2@…/v3/SECRETKEY456 prints both secrets. The success path (L147) prints the full URL too.
3 (P3) Hand-rolled .env parsing KEY="http://…" → unknown url type: "http. export KEY=… and KEY = … → No RPC URL. KEY=… # alchemy → passes, but echoes # alchemy into the URL.
4 (P3) Missing env file skips IPFS Confirmed (L101 return). Also the hint names .env.<network>.example, which doesn't exist; only .env.example does.
5 (P3) str | None at def time on < 3.10 Not runnable here (no 3.9 interpreter). On 3.10.20 and 3.11.15 the diagnostic works: [MISSING] Python 3.10.20 is below 3.12, exit 1. On 3.9 a PEP 604 annotation in a def signature is evaluated at definition time and raises TypeError, so Santiago's result is expected. from __future__ import annotations fixes it.

New findings

No. 1 (P2): --network checks a file dincli never reads. dincli resolves <NETWORK>_RPC_URL from the process env, then ./.env in the current directory, then the dincli config's networks.<net>.rpc_url (get_env_key / resolve_network_value, dincli/cli/utils.py L302–L380). Nothing under dincli/ reads .env.<network>. Only hardhat/hardhat.config.ts does. getting-started.md's own "Environment Configuration" section and .env.example both tell users to put SEPOLIA_OP_DEVNET_RPC_URL in .env.
Repro: a dir with only .env containing SEPOLIA_OP_DEVNET_RPC_URL=http://127.0.0.1:18545 (a live local JSON-RPC stub) → [WARN] .env.sepolia_op_devnet not found → Copy .env.sepolia_op_devnet.example …, and the RPC is never contacted. So the check warns a correctly configured user and gives them a hint pointing at a file that doesn't exist. (Issue 245's wording said ".env / .env.", and the approval review didn't catch this either.) Fix: resolve the key the way get_env_key does (os.environ → Path.cwd() / ".env"), and point the hint at .env.example. Using a dotenv-compatible parser for that file also covers Santiago's No. 3.

No. 2 (P2): the IPFS check can't fail. resolve_ipfs_config() (dincli/cli/utils.py L279–L299) only builds an IPFSConfig and never validates it. Repro with an empty HOME (no dincli config), no .env, no IPFS env vars: [OK] resolve_ipfs_config() succeeded, returned IPFSConfig(provider='env', api_url_add=None, api_url_retrieve=None, api_key=None, api_secret=None, service_path=None). The checks that actually raise live in the upload/retrieve paths (dincli/services/ipfs.py: env needs api_url_add (L269) and a retrieve endpoint (L370–L380); filebase needs ipfs_api_key (L286); custom needs ipfs_service_path (L99–L106)). The preflight should check the same fields for the resolved provider and WARN when one is missing. Also, the fix hint Configure IPFS_NODE_URL … names a variable that nothing in the repo reads. The real ones are IPFS_API_URL_ADD/IPFS_API_URL_RETRIEVE, or dincli system configure-ipfs; see Documentation/public/guides/ipfs.md.

No. 3 (P3): any chain passes the RPC check. A stub returning 0xaa36a7 (L1 Sepolia, 11155111) for --network sepolia_op_devnet → [OK] RPC reachable … (chainId 11155111). Pasting an L1 Sepolia URL is an easy mistake, since SEPOLIA_L1_RPC_URL also exists for the bridge. A two-entry expected map (sepolia_op_devnet → 11155420, per dincli/services/bridge.py L36; local → 1337, per foundry/anvil.sh) with a WARN on mismatch would catch it.

No. 4 (P3): the doc section's audience. The issue 245 approval (amendment 5) pointed at Documentation/public/getting-started.md, which was the right file name. But that page's readers are network participants who install the wheel or the pip install git+…#subdirectory=dist package per setup.md (L36/L42). They never clone the repo, so scripts/preflight.py won't exist for them, and Foundry isn't in the page's own "System Requirements"/"Dependencies" list, yet the script makes missing forge/anvil a hard exit 1. A one-line scope note at the top of the section ("For contributors working from a clone of this repo; participants who installed dincli from the wheel can skip this") fixes it without moving anything.

No. 5 (P3): Developer/BACK_LOG.md BL-32 attribution. The row's Source cell now says the preflight.sh pattern is from devops-scripting-labs 03-drift-detection. That's BL-33's source. In that repo preflight.sh sits at the top level (checked the repo tree via the GitHub API). Please revert that cell. The edit also drops the "the only 'preflight' in dincli/cli/system.py is the bridge's tx preflight" note, which is still useful context. The status cell is fine to keep, but it says "✅ Fixed" before review is through, so it'll need refreshing to match whatever lands.

No. 6 (P3, tests): the happy path is gated on forge only and never runs in CI. _needs_forge checks shutil.which("forge") only. With forge on PATH and no anvil, both happy-path tests fail (2 failed, 3 passed) instead of skipping. And in CI they always skip (see above). Suggest making them hermetic instead: drop executable forge/anvil placeholders into tmp_path, prepend that to PATH, remove the skipif, and the exit-0 path is covered in CI too. That's the same placeholder approach Santiago used. Add the regression tests he asked for alongside (an OSError-raising fake module; an RPC URL with a key asserting the key isn't in stdout). Plus one each for No. 1 and No. 2 above.

Nit: _check_rpc always returns 0, so failures += _check_rpc(...) and _network_checks's failures counter are dead. Both could just return None.

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/--network split is exactly what the issue asked for. Not mergeable yet, though: on top of Santiago's two P2s (No. 1 import handling, No. 2 credential-safe output), new No. 1 and No. 2 mean that --network currently gives the wrong answer for a standard dincli setup in both directions. It warns about a missing file that dincli doesn't use, and it reports IPFS [OK] with nothing configured. New No. 3–6 and Santiago's P3s are smaller and could land in the same fix round. Per-file breakdown follows in the next comment.

@umeradl

umeradl commented Oct 21, 2026

Copy link
Copy Markdown
Member

Files changed (5) — as of ba745bc (PR head)

Diffed against merge-base eb8085d (develop tip). The branch already has the latest develop merged in (0 commits behind), so none of the 5 files overlaps anything on develop. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BLOCKED (review pending). A local git merge-tree --write-tree dry run is clean.

Finding numbers below refer to the previous comment: "S-No. N" = Santiago's review, "No. N" = the new findings.

scripts/preflight.py

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.

robertocarlous and others added 2 commits October 24, 2026 10:41
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
@robertocarlous

robertocarlous commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

All findings from both reviews have been addressed in 8cc61f88

P2 — S-No. 1 (import-failure handling)
_check_importable now catches Exception (not just ImportError), so a native-lib OSError like libtorch_cuda.so: cannot open shared object file stays as [WARN] and the checker continues. Covered by test_import_time_oserror_is_advisory_and_does_not_abort.

P2 — S-No. 2 (credential-safe output)
_sanitize_url() strips path, query, and userinfo, leaving scheme://host/***. Both the OK and WARN paths in _check_rpc use the sanitized form; the exception object is never printed. Covered by test_rpc_success_output_does_not_expose_credentials and test_rpc_failure_output_does_not_expose_credentials_or_exception.

P2 — No. 1 (wrong env file)
_network_checks now resolves <NETWORK>_RPC_URL via os.environ then .env — matching dincli's own get_env_key lookup. A warning is still emitted when .env.<network> is absent, but the hint now points at .env.example (the file that actually exists). The --network sentence in the doc is updated accordingly.

P2 — No. 2 (IPFS check always passes)
_check_ipfs_config() is now a standalone function that validates the resolved IPFSConfig per provider: env checks IPFS_API_URL_ADD and IPFS_API_URL_RETRIEVE; filebase checks api_key; custom checks service_path. The hint now names the real env vars (IPFS_API_URL_ADD / IPFS_API_URL_RETRIEVE) and dincli system configure-ipfs.

P3 — S-No. 3 (dotenv parsing)
_read_env_value handles export KEY=val, KEY = val (spaces around =), single/double-quoted values (preserving # inside quotes), and unquoted inline comments. Covered by the three test_env_reader_accepts_dotenv_assignment_forms cases.

P3 — S-No. 4 (IPFS skipped on missing env file)
_check_ipfs_config() always runs after the env file check; the early return is gone. The hint references .env.example. Covered by test_missing_network_env_still_checks_ipfs.

P3 — S-No. 5 (Python 3.9 annotation crash)
from __future__ import annotations added. run()'s annotation is now a string at definition time, so the version diagnostic runs before evaluation. Covered by test_run_annotations_are_postponed_for_python_39_compatibility.

P3 — No. 3 (any chainId passes)
_EXPECTED_CHAIN_IDS maps sepolia_op_devnet → 11155420 and local → 1337. A mismatch emits [WARN] with a hint to verify the URL.

P3 — No. 4 (doc audience)
A scope note now opens the preflight section: "For contributors working from a clone of this repo. Participants who installed dincli from the wheel can skip this section."

P3 — No. 5 (BL-32 Source cell)
03-drift-detection removed (that is BL-33's source). The bridge-preflight note restored. Test count updated to 13.

No. 6 (happy-path tests not hermetic)
TestHappyPath now uses a _make_stubs() helper that writes minimal forge and anvil executables into tmp_path and prepends that to PATH. The skipif is gone; both tests run in CI. All 13 tests pass (pytest tests/test_preflight.py).

@robertocarlous

Copy link
Copy Markdown
Collaborator Author

Cc: @Santiagocetran changes fixed can you review again

@Santiagocetran Santiagocetran left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

robertocarlous and others added 2 commits October 27, 2026 09:09
…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).
@robertocarlous

Copy link
Copy Markdown
Collaborator Author

Addressed in e8ad0d3f.

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, so the warning was misleading.

Unknown IPFS provider

The else branch in _check_ipfs_config now calls _warn() instead of _ok(). An unrecognised provider is rejected by dincli's IPFS operations.

.env decode safety

_read_env_value is now wrapped in try/except inside _network_checks. A file-read or encoding error becomes a [WARN] and the IPFS check continues independently.

Regressions added (all pass)

  • test_env_read_from_cwd_not_repo_root — places .env only in a tmp_path cwd and asserts the URL reaches _check_rpc; would fail against the old _REPO_ROOT lookup.
  • test_env_read_failure_still_runs_ipfs_check — writes a binary (non-UTF-8) .env, asserts [WARN] is printed and the IPFS check still runs.
  • test_unknown_ipfs_provider_warns — injects a fake resolve_ipfs_config returning provider="unknown_backend" and asserts [WARN] (not [OK]).
  • test_missing_network_env_still_checks_ipfs — updated to use monkeypatch.chdir and checks "No RPC URL found" instead of the removed .env.<network> warning.

All 16 preflight tests pass.

@robertocarlous

Copy link
Copy Markdown
Collaborator Author

Cc: @umeradl

@Santiagocetran Santiagocetran left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

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.

  1. scripts/preflight.py — new, +311/−0
    Adds contributor setup checks for Python and Foundry, advisory package checks, and optional RPC/IPFS configuration checks. The reviewed fixes cover cwd .env lookup, credential-safe RPC diagnostics, unknown-provider warnings and continuation after configuration/import errors. Merge as-is. Full dotenv parity is an optional follow-up, not a blocker.

  2. tests/test_preflight.py — new, +216/−0
    Adds missing-tool and happy-path subprocess tests with tool placeholders, plus regressions for import errors, RPC redaction, dotenv forms, cwd lookup, independent IPFS checks, unknown providers and postponed annotations. Merge as-is. These are mocked/subprocess checks, not live RPC/IPFS verification.

  3. Documentation/public/getting-started.md — modified, +34/−0
    Adds contributor-specific usage, checks, exit behavior and optional network examples. Clarifies that wheel-installed network participants do not need this contributor setup. Merge as-is; no pending change.

  4. .gitignore — modified, +1/−0
    Adds an exception for scripts/preflight.py while keeping unrelated scripts ignored. Merge as-is; no pending change.

  5. Developer/BACK_LOG.md — modified, +1/−1
    Updates BL-32 with the implementation and PR/issue links, preserving source attribution. Merge as-is. Optional follow-up: update the stale 13-test count to 16.

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.

@Santiagocetran
Santiagocetran merged commit 06c0af5 into InfiniteZeroFoundation:develop Oct 27, 2026
4 checks passed

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants