local env usability fixes#1807
Conversation
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bec726a650
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await runDockerCompose({ | ||
| composeFile, | ||
| env, | ||
| profiles: runOptions.profiles, | ||
| detach: true, | ||
| }); |
There was a problem hiding this comment.
In a clean --from-genesis run this starts the base compose file unchanged, but the well-known compose files only pass SEED_PHRASE: $MIDNIGHT_NODE_... (for example devnet.network.yaml), while the current node only imports validator keys from AURA_SEED_FILE, GRANDPA_SEED_FILE, and CROSS_CHAIN_SEED_FILE in node/src/command.rs. As a result, even when users provide the documented MIDNIGHT_NODE_*_SEED env vars, the containers start with empty consensus keystores and the from-genesis network cannot produce/finalize blocks unless stale data already contains keys; this mode needs to generate/mount seed files or update the compose env before docker compose up.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
m2ux
left a comment
m2ux
left a comment
There was a problem hiding this comment.
Review Summary
Structured review pass over this PR (from-genesis restore, stale-data/unset-var warnings, fork-network CI hang fix). Re-verified everything against current HEAD (bec726a650) immediately before posting — no drift since the earlier automated triage.
Overall: Request Changes
Two independently-verified blockers remain unaddressed. Everything else in the diff looks solid.
What's good
--from-genesis, the stale-data warning, and the unset-compose-var warning are implemented cleanly and match the package's existing warn-and-continue conventions.- The
fork-network.ymlCI-hang fix (RPC probe +createApibounded timeout + IPv4-explicit default) is well-diagnosed and correctly addresses the documented Docker-loopback/IPv6-resolution root cause forruntimemode. No defects found here. tsc --noEmitandeslintare clean; docs/changelog additions are accurate.
Blockers
1. Local Environment Tests CI check — still FAILURE, now root-caused.
Traced the job log: +local-env-ci's contract-compiler image build fails on apt-get install — jq : Depends: libjq1 (= 1.6-2.1+deb12u1) but 1.6-2.1+deb12u2 is to be installed. This is a Debian security-repo drift against a pin in local-environment/src/networks/local-env/configurations/contract-compiler/Dockerfile (last touched by an unrelated PR, #1630) — not caused by this diff, and would fail identically on main today. Needs a jq/libjq1 pin bump (or relaxing the exact pin) in a small follow-up before this can go green.
2. From-genesis validator seed wiring — confirmed gap, unchanged.
Re-checked directly against current HEAD: well-known compose files still declare SEED_PHRASE: $MIDNIGHT_NODE_0N_0_SEED per validator, but node/src/command.rs still only imports keys from AURA_SEED_FILE/GRANDPA_SEED_FILE/CROSS_CHAIN_SEED_FILE (file paths — no inline-phrase consumer exists anywhere in node/src). A --from-genesis run with a correctly-populated SEED_PHRASE will start successfully and never produce/finalize a block — and this PR's own collectUnsetComposeVars check can't catch it, since the var is set, just functionally inert. This is a pre-existing latent inconsistency between the well-known compose files and the node's actual keystore contract, newly exposed (not introduced) by --from-genesis being the first mode to let SEED_PHRASE reach a container unmodified — fork-mode's mock override always blanked it before. The changelog/README both describe from-genesis provisioning as complete via --env-file, without flagging this. Recommend either the node consuming SEED_PHRASE directly, or the tooling deriving *_SEED_FILEs from it before bring-up, as a fast-follow — and flagging the gap explicitly in the docs in the meantime.
Additional findings (non-blocking)
- Formatting:
local-environment/src/commands/run.ts's newthrow new Error("--from-genesis and --from-snapshot are mutually exclusive.")failsnpm run lint's Prettier check (needs wrapping to 3 lines). One-line fix. Side note: no CI job in this repo currently lintslocal-environment's TypeScript at all (+checkis Rust-only;+local-env-cifails before reaching any TS execution), which is how this slipped through — might be worth a dedicated lint step for this package at some point. - Test coverage:
local-environmenthas zero automated tests (pre-existing, not this PR's doing).collectUnsetComposeVars, the stale-data check, andcreateApi's timeout path are all pure/near-pure functions that would be cheap to unit-test without Docker — a good candidate for a follow-up. - Minor/latent, not currently triggered:
collectUnsetComposeVars's regex doesn't special-case${VAR:-default}or$$VARcompose syntax (no current well-known compose file uses either, so no live impact);fork-network.yml'sdocker inspectcall for node1's bridge IP would concatenate multiple IPs with no separator if node1 were ever on more than one Docker network (not the case today).
Checked, not escalated
full-upgrade mode's un-fixed RPC-reachability gap is already self-acknowledged in the workflow's own NOTE: comment — treated as confirmation-only here, not a new blocker.
Posted as part of a structured review workflow — happy to re-review once the two blockers above are addressed or explicitly deferred.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
PR Review Summary
PR: #1807 - local env usability fixes
Plan: work package README
Reviewers: Post-Implementation Review · Validate · Strategic Review
Reports: Prior Feedback Triage · Code Review · Test Suite Review · Strategic Review
Date: 2026-07-15
Executive Summary
The change is minimal, correctly scoped to issue #1468 (7/7 files map to the five usability concerns; no scope creep, no over-engineering, no orphaned symbols), and cleanly reuses existing helpers. It is held back by a single unaddressed Critical on the authored surface: from-genesis validator seeds are delivered via SEED_PHRASE, but the node reads seeds only from *_SEED_FILE paths — so a --from-genesis network boots, reports healthy, yet never finalizes a block. The shortfall is completeness, not scope. The previously-blocking red Local Environment Tests CI check is now stale (green at head 98dd8e11 after a main merge pulled the offending jq pin) and is not a current blocker.
Overall Rating: Request Changes
Prior Feedback Triage
Disposition of every prior comment and review on the PR (human and bot), determined before independent analysis.
| # | Prior Comment | Author | Disposition | Reasoning |
|---|---|---|---|---|
| 1 | Local Environment Tests CI job failed (commit bec726a) |
datadog-official (bot) | Superseded — stale | Reported against bec726a650; at head 98dd8e11 the check is green (the jq/libjq1 pin was pulled via a main merge). Not a current blocker. |
| 2 | Review Blocked — insufficient credits | jina-simulation (bot) | Refuted | No substantive finding; the bot could not run. |
| 3 | Codex usage limits reached | chatgpt-codex-connector (bot) | Refuted | Administrative notice, not a code finding. |
| 4 | Wrapper "automated review suggestions" note | chatgpt-codex-connector (bot) | Refuted | Boilerplate wrapper; the substantive Codex finding is inline row 6. |
| 5 | Request Changes — red CI (stale) + from-genesis seed-wiring gap + prettier/no-tests/regex nits | m2ux (human) | Confirmed — caps rating | Seed-wiring gap valid and unaddressed → CR-1. Red CI is stale (see row 1). Non-blocking nits → CR-3/CR-5/TR-1. |
| 6 | P2 — wire validator seed files in from-genesis mode; empty keystores never finalize | chatgpt-codex-connector (bot) | Confirmed — caps rating | Same substance as row 5, reached independently → CR-1. |
Code Review Findings
| # | Finding | Severity | Disposition |
|---|---|---|---|
| CR-1 | From-genesis validator seeds never reach the keystore → network never finalizes (liveness-halt) | Critical | Fix now (design decision) |
| CR-2 | README documents a from-genesis happy path that does not finalize | Medium | Fix now (rides on CR-1) |
| CR-3 | New throw line is 82 chars, exceeds prettier printWidth 80 |
Low | Noted |
| CR-4 | local-environment TypeScript is not linted in CI (why CR-3 slips green) |
Low | Noted (CI follow-up) |
| CR-5 | collectUnsetComposeVars regex misses ${VAR:-default} / $$VAR (latent) |
Low | Noted |
| CR-6 | Changelog fragment has empty PR:/Issue: trailers |
Low | Noted (see SR-1) |
| SA-1 | Unmatched producer: seed channel (SEED_PHRASE) ≠ read channel (*_SEED_FILE) — same defect as CR-1 by the conservation route |
Critical | Fix now (= CR-1) |
| SA-2 | Unset-var guard checks presence, not effect → false "ready" for a set-but-inert seed | Medium | Fix now (part of CR-1 fix) |
Test Review Findings
| # | Gap | Severity | Disposition |
|---|---|---|---|
| TR-1 | local-environment/ package has no automated tests |
Medium | Fix now (recommended) |
| TR-2 | No test exercises the CR-1 from-genesis failure path | Medium | Fix now (recommended) |
| TR-3 | No test pins the mutual-exclusivity guard | Low | Noted |
Strategic Review
Scope-fit is clean — all 7 changed files (+161/-9) map to issue #1468's five usability concerns; no scope creep, no over-engineering, no orphaned symbols. The only shortfall is completeness (CR-1), not scope.
| # | Finding | Severity | Disposition |
|---|---|---|---|
| SR-1 | Changelog fragment lacks a CI-conformant issue reference (same surface as CR-6) | Low | Noted |
Action Items
Must Address (Blocking):
- Wire from-genesis validator seed provisioning so seeds reach the keystore (
*_SEED_FILEor in-tool provisioning) — the network must finalize, not merely boot healthy (CR-1 / SA-1)
Should Address (Recommended):
- Update the README from-genesis section to describe the actual seed-file provisioning, or flag the gap (CR-2)
- Replace/augment the unset-var guard with a seed-provisioning-contract check that measures effect, not presence (SA-2)
- Add a test harness to
local-environment/(start with a pure unit test forcollectUnsetComposeVars) (TR-1) - Add a test covering the from-genesis seed-provisioning precondition (TR-2)
Could Address (Suggested):
- Rewrap the mutual-exclusivity
throwto satisfy prettier printWidth 80 (CR-3) - Add a CI job that lints the
local-environmentTypeScript package (CR-4) - Add a unit test pinning the
--from-genesis/--from-snapshotmutual-exclusivity guard (TR-3)
Nice to Have (Optional):
- Handle
${VAR:-default}/$$VARforms incollectUnsetComposeVarsif compose files gain them (CR-5) - Populate the changelog fragment
PR: #1807/Issue: #1468trailers (CR-6 / SR-1)
Praise
- Clean reuse of existing helpers (
discoverComposeDataMounts/isNonEmptyDirectory/runDockerCompose) — the new path is genuinely thin. - Correct control-flow placement of the mutual-exclusivity guard and the early from-genesis dispatch (the fork-only config load no longer runs on the genesis path).
- Honest user-facing warnings for stale
data/and unset compose vars — the right instrument within the "warn, don't fail" design. - Strong documentation — the fork-network input clarifications and the snapshot-discovery recipe directly close usability gaps from #1468.
Severity Scale
Critical / Major / Minor / Nit / Informational, rendered as Critical / High / Medium / Low. CR-1 is Critical on the liveness-halt axis: behaviourally the flag works and the stack comes up, yet the network cannot finalize — so it renders at Critical, not downgraded.
Posted by an automated review agent on behalf of @m2ux. The recommendation reflects an independent re-verification at head 98dd8e11; the maintainers retain full discretion over disposition.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: Justin Frevert <justinfrevert@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
PR Review Summary
PR: #1807 - local env usability fixes
Plan: work package README
Reviewers: Post-Implementation Review · Validate · Strategic Review
Reports: Prior Feedback Triage · Code Review · Test Suite Review · Validate · Strategic Review
Date: 2026-07-16
Executive Summary
Re-review at head 93eeda644 after the author's seed-wiring follow-up. The prior Critical from-genesis keystore gap is closed (compose override + fail-closed + unit tests); CI is green including Local Environment Tests. Remaining items are Low/Medium nits only.
Overall Rating: Approve
Prior Feedback Triage
Disposition of every prior comment and review on the PR (human and bot), determined before independent analysis.
| # | Prior Comment | Author | Disposition | Reasoning |
|---|---|---|---|---|
| 1 | Local Environment Tests CI job failed |
datadog-official (bot) | Superseded | Reported against bec726a650; check is SUCCESS at head 93eeda644 |
| 2 | Codex usage limits reached | chatgpt-codex-connector (bot) | Refuted | Administrative notice; no code finding |
| 3 | Codex usage limits reached | chatgpt-codex-connector (bot) | Refuted | Same as row 2 |
| 4 | Codex usage limits reached | chatgpt-codex-connector (bot) | Refuted | Same as row 2 |
| 5 | Codex review wrapper | chatgpt-codex-connector (bot) | Superseded | Boilerplate; substance was the inline P2 (row 7) |
| 6 | Request Changes — red CI + seed wiring | m2ux (human) | Superseded | Both blockers addressed at head; nits re-checked below |
| 7 | P2 — wire validator seed files in from-genesis | chatgpt-codex-connector (bot) | Superseded | genesisComposeOverride + fail-closed path in 93eeda644 |
| 8 | Request Changes — prior Critical seed mismatch | m2ux (human) | Superseded | Posted before 93eeda644; seed-wiring blocker closed at new head |
Code Review Findings
| # | Finding | Source | Severity | Disposition |
|---|---|---|---|---|
| CR-1 | README fails prettier on pre-existing tables | README.md:203 | Low | Noted |
| CR-2 | full-upgrade still lacks RPC probe |
fork-network.yml:294 | Low | Noted |
| CR-3 | unset-var regex misses ${VAR:-default} / $$VAR |
run.ts:237 | Low | Noted |
Test Review Findings
| # | Gap | Source | Severity | Disposition |
|---|---|---|---|---|
| TR-1 | No Docker finalize test for seeded from-genesis | package.json:6 | Medium | Noted |
| TR-2 | npm run lint fails prettier on pre-existing README tables |
package.json:47 | Low | Noted |
Action Items
Must Address (Blocking):
- none
Should Address (Recommended):
- Add a Docker-level check that seeded
--from-genesisfinalizes (TR-1)
Could Address (Suggested):
- Prettier-fix pre-existing
local-environment/README.mdtables sonpm run lintis green (CR-1) (TR-2) - Extend RPC probe pattern to
full-upgrade when tooling allows (CR-2) - Handle
${VAR:-default}/$$VARincollectUnsetComposeVarsif compose files gain them (CR-3)
Posted by an automated review agent on behalf of @m2ux. The recommendation reflects an independent re-verification at head 93eeda644; the maintainers retain full discretion over disposition.
2 participants
Overview
Small fixes for issues noted on #1468 (comment).
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links