feat(localnet): configure sequencer binary and config path

[danisharora099]

Summary

Follow-up to #30, rebased onto the v0.2.0 schema (#95). Adds [localnet].sequencer_binary and [localnet].sequencer_config_path so projects can point scaffold at a non-upstream sequencer (custom fork, alternate LEZ layout) without forking scaffold itself. Defaults match upstream LEZ at the pinned version, so existing scaffold.toml files pick up the new keys with no behaviour change and no schema bump.

What changed

• LocalnetConfig (src/model.rs) gains the two String fields and two helpers — sequencer_bin_path(lez) and sequencer_config_resolved_path(lez) — so the absolute/relative resolution lives in one place.
• setup (src/commands/setup.rs) builds via cargo build -p <sequencer_binary>. The binary file name and the cargo package name share a value upstream and must continue to match in any fork — documented inline.
• localnet start (src/commands/localnet.rs) resolves the binary via the helper, verifies the resolved config path is a regular file before calling patch_sequencer_port, and keeps master's JSON-mutation flow (the pinned LEZ rejects unknown CLI flags, so no --port arg).
• doctor (src/commands/doctor.rs) surfaces both the binary and the config as check_path rows resolved through the same helpers, so absolute and LEZ-relative values render symmetrically.
• README gets a "Pointing localnet at a custom sequencer" section with the example and a scaffold↔LEZ pin alignment caveat.

Validation

• sequencer_binary is constrained to a cargo package name: rejects empty, leading -, ., .., path separators, and absolute paths. Both the cargo -p arg and the target/release/<name> join become impossible to subvert.
• sequencer_config_path allows path separators (the file lives a few directories deep in LEZ) but rejects empty values and leading - so flag-shaped values can't sneak into the spawned sequencer's argv.
• Both validators include the offending value in the error.

Tested

• cargo fmt --all
• cargo check
• cargo test --lib — 8 new unit tests in config.rs (defaults, round-trip, each forbidden form for both keys)
• cargo test --test cli — new doctor --json integration test asserts the configured paths surface end-to-end (replaces the old localnet start smoke test, which had a port-binding race)

Non-changes

• No schema bump — additive within [scaffold].version = "0.2.0", defaults make existing configs work unchanged.
• No re-introduction of a --port CLI flag for the spawned sequencer; master's patch_sequencer_port flow is preserved.

Out of scope

• Prebuilt binary support (the binary still has to live at <lez>/target/release/<name>).
• A separate sequencer_package knob if a fork ever needs the cargo package and runtime binary names to diverge — easy to add later, not needed today.

[Copilot]

Pull request overview

This PR completes the [localnet] config surface by making the sequencer executable name and config path configurable, and then wiring those values through localnet startup, setup, and doctor checks to support newer LSSA/LEZ layouts without patching Scaffold.

Changes:

• Add [localnet].sequencer_binary and [localnet].sequencer_config_path with backwards-compatible defaults.
• Use the configured binary/config path in logos-scaffold setup, localnet start, and doctor.
• Extend doctor’s standalone-support detection to recognize both older and newer sequencer repo layouts, and add a regression test covering the new config wiring.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/model.rs	Extends LocalnetConfig with sequencer_binary and sequencer_config_path defaults.
src/constants.rs	Adds default values for the new localnet sequencer settings.
src/config.rs	Parses/serializes the new [localnet] keys with fallback defaults.
src/commands/setup.rs	Uses configured sequencer identifier when building the standalone sequencer.
src/commands/localnet.rs	Uses configured sequencer binary/config path when spawning the localnet sequencer.
src/commands/doctor.rs	Checks for the configured sequencer binary under target/release.
src/doctor_checks.rs	Expands standalone marker search to include sequencer/service layout.
tests/cli.rs	Adds regression test asserting localnet start uses configured binary and config path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/commands/localnet.rs:138

• sequencer_binary comes from config and is joined onto lssa/target/release. Path::join will ignore the prefix if sequencer_binary is absolute, and .. segments can escape the intended directory, which can lead to executing an unexpected binary when running localnet start. Consider rejecting absolute paths / path separators in sequencer_binary (treat it as a binary name), or explicitly normalize and enforce that the resolved path stays under lssa/target/release before spawning.

    ensure_dir_exists(lssa, "lssa")?;
    let sequencer_bin = lssa.join("target/release").join(sequencer_binary);
    if !sequencer_bin.exists() {
        return Err(LocalnetError::MissingSequencerBinary {
            path: sequencer_bin.display().to_string(),
        }
        .into());

src/commands/localnet.rs:183

• Now that sequencer_config_path is configurable, localnet start will happily spawn the sequencer even if that path is wrong/missing, leading to a harder-to-diagnose readiness timeout/exit. Consider validating the configured config path exists (when relative, check lssa.join(sequencer_config_path); when absolute, check it directly) and returning a targeted error before spawning the process.

    let sequencer_pid = spawn_to_log(
        Command::new(sequencer_bin)
            .current_dir(lssa)
            .arg(sequencer_config_path)
            .arg("--port")
            .arg(localnet_port.to_string())
            .env("RUST_LOG", "info")
            .env("RISC0_DEV_MODE", if risc0_dev_mode { "1" } else { "0" }),
        log_path,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vpavlin]

What Copilot and @fryorcraken said, otherwise looks good:)

[weboko]

can you add doc with example how to use with other than default sequencer?
you can add it to CLI or to the readme or both

otherwise lgtm

[danisharora099]

@logos-co/eco-dev-eng addressed comment

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[weboko]

@danisharora099 you should be able to open your branched in this repo and not your fork

[fryorcraken]

Code review

Critical

C1. Branch is severely stale relative to master. Multiple post-base merges have rewritten the same surfaces this PR touches:

Surface	This PR	Current master
Repo field	[repos.lssa] / cfg.lssa	[repos.lez] (legacy [repos.lssa] accepted) / cfg.lez
Default sequencer binary	sequencer_runner	sequencer_service (SEQUENCER_BIN_REL_PATH, src/constants.rs:13)
Default sequencer config	sequencer_runner/configs/debug (a directory)	sequencer/service/configs/debug/sequencer_config.json (a JSON file, src/constants.rs:14-15)
[localnet] port parsing	Lax if let Ok(p) (silently keeps default)	Strict bail! on parse error (src/config.rs:87-93)
wallet.binary in serializer	Present	Removed

Cannot merge as-is. After rebase the meaningful delta shrinks to ~30 lines. Please re-validate the entire diff post-rebase — most of what's "added" already exists in master under different names.

C2. cmd_localnet_start re-introduces a --port CLI flag the pinned LEZ sequencer doesn't accept. The PR adds:

.arg(sequencer_config_path)
.arg("--port")
.arg(localnet_port.to_string())

Master removed CLI-level port passing because the pinned sequencer reads its port from the JSON config. Master patches that JSON via patch_sequencer_port (src/commands/localnet.rs:443-463). Adding --port back will be ignored at best, or refuse to start. The new test never starts a real sequencer, so it doesn't catch this. On rebase, drop the --port arg and keep patch_sequencer_port.

C3. setup.rs: -p <package> → --bin <name> is a behavioral change. Master uses cargo build --release --features standalone -p sequencer_service (src/commands/setup.rs:37-38). -p and --bin are not equivalent — --bin errors on multiple targets with the same name across packages, and --features standalone may apply to the wrong package. The PR couples "binary I want at runtime" to "cargo build target," which are different concepts. Either keep -p <package> and add a separate sequencer_package knob, or document the constraint explicitly with a comment near the --bin arg.

Important

I1. validate_sequencer_binary_name is partial; sequencer_config_path is unvalidated (src/config.rs:210-221). The binary-name validator rejects /, \, leading -, absolute, empty — but sequencer_config_path has no validation, even though it's joined onto the lez path and passed unmodified as a CLI arg. sequencer_config_path = "../../etc/passwd" would silently traverse outside the LEZ tree. No real exploit risk (operator-owned file), but the inconsistency is surprising. Either drop the binary validator and rely on cargo errors, or add an analogous check on the config path. Whatever you choose, do it symmetrically. Include the offending value in the error message.

I2. No unit tests for new parsing or new validator. The PR adds an end-to-end test for localnet start but no parser-level tests for: round-tripping sequencer_binary / sequencer_config_path, defaults applied when keys are absent, or validate_sequencer_binary_name rejecting each forbidden form. Master's config.rs already has a #[cfg(test)] mod tests block to extend. These are cheap and catch regressions on future renames.

I3. sequencer_config_path semantics conflict with master's patch_sequencer_port (src/commands/localnet.rs:443-463). Master read_to_strings the path, parses as JSON, mutates port, writes back. If a user follows the old default (sequencer_runner/configs/debug — a directory), patch_sequencer_port fails with "Is a directory." After rebase, validate that sequencer_config_path points to a file before spawning, with a clear error. Real foot-gun once this becomes configurable.

I4. README copy needs re-merging. The PR still references the pre-master setup wording and a port-3040 callout the PR explicitly puts out of scope. Re-merge the README on rebase rather than overwriting master's wording.

Suggestions

S1. Asymmetric absolute-path handling between doctor and localnet (src/commands/doctor.rs:115-128). Doctor branches on is_absolute() for sequencer_config_path but always does lssa.join("target/release").join(name) for the binary. Path::join happens to do the right thing if the joined component is absolute, but the inconsistency suggests one of the two cases wasn't fully thought through. Pick a pattern.

S2. doctor_checks.rs adds sequencer/service/Cargo.toml to the standalone-support check (src/doctor_checks.rs:152). After rebase, the sequencer_runner/Cargo.toml line should likely be dropped — verify whether check_standalone_support requires all files or any. If "all," the check now never passes for any single layout.

S3. The new test only asserts on stdout containing the right substrings (tests/cli.rs:780-792). Brittle coupling to logging. Acceptable as a smoke test, but add a comment so a future log-format change doesn't silently break it — or have the test inspect .scaffold/logs/sequencer.log directly.

Security / Performance

• Security: No new attack surface — scaffold.toml is operator-owned. The validator gap (I1) is consistency, not exploit risk.
• Performance: Nothing in a hot path. No concerns.

Recommended next steps

1. Rebase onto master — most of this diff conflicts.
2. Re-scope: the actual new code is two [localnet] keys + small plumbing in setup.rs / localnet.rs / doctor.rs / config.rs. Drop --port (C2). Reconfirm --bin vs -p (C3).
3. Add config-parser and validator tests (I2).
4. Decide on symmetric path validation (I1); validate sequencer_config_path is a file before spawning (I3).
5. Re-verify with the validation flow in CLAUDE.md using a custom sequencer_binary value.

[danisharora099]

Force-pushed a rebase

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.