Skip to content

feat(template): rename lez-framework to spel; delegate scaffolding to spel init - #199

Open
vpavlin wants to merge 7 commits into
masterfrom
feat/spel-template
Open

feat(template): rename lez-framework to spel; delegate scaffolding to spel init#199
vpavlin wants to merge 7 commits into
masterfrom
feat/spel-template

Conversation

@vpavlin

@vpavlin vpavlin commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • lgs new --template spel (new default for framework projects) calls spel init <name> --lez-tag <lez-tag>, then layers scaffold.toml and AI skills on top. Scaffold owns no SPEL template files.
  • lgs new --template lez-framework prints a deprecation warning and maps to spel. Existing scaffold.toml files with framework.kind = "lez-framework" continue to work.
  • FRAMEWORK_KIND_SPEL = "spel" added to constants.
  • lgs build idl on spel projects delegates to spel generate-idl (vendored binary).
  • lgs build client on spel projects delegates to spel ffi-gen.
  • Deploy-cache hashing now requires an IDL for both lez-framework and spel projects.
  • templates/lez-framework/ deleted — the template referenced jimmy-claw/lez-framework (broken) and duplicated logic that belongs in spel.
  • skills/lez-framework-template/ renamed → skills/spel-template/ with updated content (spel vocabulary: #[spel_program], spel.toml, make targets, spel CLI).

Why

The lez-framework template was broken (wrong repo, old macros) and maintained a parallel scaffolding path to what spel init already does correctly. This change makes scaffold's role minimal: version pinning, scaffold.toml, and AI skills — program scaffolding belongs to spel.

Closes logos-co/ecosystem#128 (scaffold now uses released spel for framework projects).

Test plan

  • lgs new my-project --template spel succeeds with spel on PATH and produces a buildable project
  • lgs new my-project --template lez-framework prints deprecation warning and produces same result
  • lgs new my-project (default) still uses the default template (bare LEZ, no change)
  • lgs build idl in a spel project calls spel generate-idl via the vendored binary
  • All 342 unit tests pass (cargo test --lib)

🤖 Generated with Claude Code

vpavlin and others added 2 commits June 2, 2026 09:41
Updates the two pinned defaults in `constants.rs`:
- DEFAULT_LEZ: v0.2.0-rc1 (35d8df0) → v0.1.2 (cf3639d), the first
  published LEZ release.
- DEFAULT_SPEL: v0.2.0-rc.5 (1db7c5f) → v0.5.0 (73fc462), released
  today and aligned with LEZ v0.1.2.

Circuits version (0.4.1) is unchanged — LEZ v0.1.2's flake.lock pins
the same circuits commit (d6cf41f) as before.

Closes logos-co/ecosystem#128 (upgrade spel + scaffold to pin LEZ v0.1.2).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…el init

## What changed

- `lgs new --template spel` (new) calls `spel init <name> --lez-tag
  <DEFAULT_LEZ.tag>` to scaffold the project, then layers scaffold.toml
  and AI skills on top. Scaffold no longer owns any SPEL template files.

- `lgs new --template lez-framework` prints a deprecation warning and
  maps to `spel`. Existing projects with `framework.kind = "lez-framework"`
  in scaffold.toml continue to work unchanged.

- `FRAMEWORK_KIND_SPEL = "spel"` added to constants.

- `lgs build idl` for spel projects delegates to the vendored spel binary
  (`spel generate-idl`) instead of scaffold's internal IDL pipeline.

- `lgs build client` for spel projects delegates to `spel ffi-gen`.

- Deploy-cache IDL hashing treats `spel` projects the same as
  `lez-framework` (IDL is a required deploy artifact).

- `templates/lez-framework/` removed entirely — spel init is the
  authoritative scaffolder for framework projects.

- `skills/lez-framework-template/` renamed to `skills/spel-template/`
  with updated content using spel vocabulary (#[spel_program], spel.toml,
  make targets, spel CLI commands).

## Why

The `lez-framework` template in scaffold was broken (still referenced
jimmy-claw/lez-framework) and duplicated template logic that belongs to
spel. Scaffold's role is now minimal: scaffold.toml, AI skills, and
version pinning — program scaffolding is owned by spel.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vpavlin
vpavlin requested review from a team and Copilot June 2, 2026 07:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Replaces the broken in-tree lez-framework template with a thin delegation to the external spel CLI for scaffolding framework projects. Scaffold now only owns version pinning, scaffold.toml, and AI skills; program scaffolding (Cargo workspace, IDL gen, FFI gen) is handled by spel init/spel generate-idl/spel ffi-gen. The legacy lez-framework template name remains accepted but emits a deprecation warning and is silently mapped to spel.

Changes:

  • New --template spel (and deprecation alias lez-framework) shells out to spel init and layers scaffold state on top; LEZ pin downgraded to v0.1.2, spel pin bumped to v0.5.0.
  • lgs build idl and lgs build client delegate to spel generate-idl / spel ffi-gen for spel projects; deploy-cache hashing now requires an IDL for both lez-framework and spel kinds.
  • Deleted the entire templates/lez-framework/ tree and renamed skills/lez-framework-template/skills/spel-template/ with rewritten content.

Reviewed changes

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

Show a summary per file
File Description
src/commands/new.rs Splits cmd_new_inner into cmd_new_spel (delegates to spel init) and cmd_new_default; adds find_spel_on_path and build_scaffold_config helpers.
src/commands/build.rs Adds FRAMEWORK_KIND_SPEL branch that runs IDL gen but skips client gen (left to project Makefile).
src/commands/idl.rs Routes spel projects to spel generate-idl; updates error message to mention spel.
src/commands/client.rs Routes spel projects to spel ffi-gen and std::process::exit(0)s mid-helper; updates error message.
src/commands/run_state.rs Treats both lez-framework and spel projects as IDL-required when computing program hashes.
src/commands/init.rs Test updated to expect spel-template skill in place of lez-framework-template.
src/constants.rs Adds FRAMEWORK_KIND_SPEL; bumps DEFAULT_SPEL to v0.5.0 and changes DEFAULT_LEZ to v0.1.2.
src/template/project.rs Removes the lez-framework-specific overlay test and drops the variant from the no-self-patch test.
src/template/skills.rs Test updated for renamed skill directory.
templates/lez-framework/** Entire template tree deleted.
skills/lez-framework-template/SKILL.md Deleted.
skills/spel-template/SKILL.md New skill describing the spel-based workflow.

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

Comment thread src/commands/client.rs Outdated
Comment on lines +49 to +54
FRAMEWORK_KIND_SPEL => {
// For spel projects, FFI/client gen is handled by `spel ffi-gen`
// (or the project Makefile's `make ffi`). Delegate directly.
cmd_spel(vec!["ffi-gen".to_string()])?;
std::process::exit(0);
}
vpavlin and others added 2 commits June 2, 2026 09:56
Addressed Copilot review comment: `std::process::exit(0)` inside a
helper returning `DynResult<Project>` was surprising and bypassed
caller cleanup. Spel dispatch now happens at call-site in
`build_clients_for_current_project` and `generate_clients_from_current_idl`
before reaching the lez-framework-only `require_lez_framework_project`
helper.

Also fixes two rustfmt failures flagged by CI on this branch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three CLI tests expected old `lez-framework`-only error strings and the
old `lez-framework-template` skill name. Updated to match the new error
messages (which now mention both `spel` and `lez-framework`) and the
renamed `spel-template` skill.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 2, 2026 08:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.

Comment thread src/commands/new.rs
Comment on lines +113 to +124
let cwd = env::current_dir()?;
let status = std::process::Command::new(&spel_bin)
.arg("init")
.arg(&cmd.name)
.arg("--lez-tag")
.arg(DEFAULT_LEZ.tag)
.current_dir(&cwd)
.status()
.context("failed to launch spel init")?;
if !status.success() {
anyhow::bail!("spel init failed");
}
Comment thread src/commands/new.rs
Comment on lines +299 to 308
fn find_spel_on_path() -> anyhow::Result<std::path::PathBuf> {
let path_var = std::env::var_os("PATH").unwrap_or_default();
for dir in std::env::split_paths(&path_var) {
let candidate = dir.join("spel");
if candidate.is_file() {
return Ok(candidate);
}
}

Ok(())
anyhow::bail!("spel not found on PATH")
}
Comment thread src/commands/new.rs
Comment on lines 44 to 46
other => {
bail!("unsupported template `{other}`. Expected `default` or `lez-framework`.")
bail!("unsupported template `{other}`. Expected `default` or `spel`.")
}
Comment thread src/commands/new.rs
Comment on lines +126 to +133
// spel init created `target/`; layer scaffold state on top.
fs::create_dir_all(target.join(".scaffold/state"))?;
fs::create_dir_all(target.join(".scaffold/logs"))?;

let cfg = build_scaffold_config(cmd, FRAMEWORK_KIND_SPEL, bootstrap_cache);
write_text(&target.join("scaffold.toml"), &serialize_config(&cfg)?)?;
ensure_scaffold_in_gitignore(target)?;
apply_skills(target)?;
vpavlin and others added 2 commits June 2, 2026 10:03
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Error message for unknown template now mentions the deprecated
  `lez-framework` alias so users know it exists.
- `--lez-path` combined with `--template spel` now fails with a clear
  message (was silently recorded in scaffold.toml but never forwarded
  to `spel init`).
- `--vendor-deps` combined with `--template spel` now fails with a clear
  message (was silently writing non-existent paths into scaffold.toml).
- `find_spel_on_path` now also probes `spel.exe` on Windows.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 2, 2026 08:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.

Comment thread src/commands/new.rs
Comment on lines 35 to 49
let template_variant = match cmd.template.as_str() {
FRAMEWORK_KIND_DEFAULT | FRAMEWORK_KIND_LEZ_FRAMEWORK => cmd.template.clone(),
FRAMEWORK_KIND_DEFAULT => cmd.template.clone(),
FRAMEWORK_KIND_SPEL => cmd.template.clone(),
FRAMEWORK_KIND_LEZ_FRAMEWORK => {
eprintln!(
"warning: template `lez-framework` is deprecated; use `--template spel` instead."
);
FRAMEWORK_KIND_SPEL.to_string()
}
other => {
bail!("unsupported template `{other}`. Expected `default` or `lez-framework`.")
bail!(
"unsupported template `{other}`. \
Expected `default` or `spel` (or the deprecated alias `lez-framework`)."
)
}
Comment thread src/commands/new.rs Outdated
Comment on lines +106 to +110
let spel_bin = find_spel_on_path().context(
"spel binary not found on PATH.\n\
Install it first:\n \
cargo install --git https://github.com/logos-co/spel.git --tag v0.5.0 spel",
)?;
Comment thread src/commands/new.rs
Comment on lines +317 to +333
fn find_spel_on_path() -> anyhow::Result<std::path::PathBuf> {
let path_var = std::env::var_os("PATH").unwrap_or_default();
for dir in std::env::split_paths(&path_var) {
let candidate = dir.join("spel");
if candidate.is_file() {
return Ok(candidate);
}
// On Windows executables carry a .exe suffix.
#[cfg(target_os = "windows")]
{
let candidate_exe = dir.join("spel.exe");
if candidate_exe.is_file() {
return Ok(candidate_exe);
}
}
}

Ok(())
anyhow::bail!("spel not found on PATH")
Comment thread src/commands/run_state.rs
Comment on lines +88 to +92
// Both lez-framework and spel projects require an IDL file at deploy time.
let is_lez_framework = matches!(
project.config.framework.kind.as_str(),
FRAMEWORK_KIND_LEZ_FRAMEWORK | FRAMEWORK_KIND_SPEL
);
- Install hint in find_spel_on_path error now uses DEFAULT_SPEL.tag
  instead of a hard-coded version string that would drift on bumps.
- check_spel_version() warns when the installed spel version does not
  match DEFAULT_SPEL.tag, so mismatches surface before `lgs setup`.
- available_templates() now includes "spel" explicitly; after removing
  the lez-framework template directory it was no longer discoverable
  from disk, so --template help advertised only "default".
- Renamed is_lez_framework -> requires_idl in run_state.rs; the old
  name implied the flag only covered lez-framework, but it also matches
  spel. Updated the inline comment at the use site to match.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/commands/new.rs
Comment on lines +332 to 342
let stdout = String::from_utf8_lossy(&output.stdout);
if !stdout.contains(DEFAULT_SPEL.tag) {
eprintln!(
"warning: installed spel version ({}) does not match the expected {} pinned by scaffold.\n\
This may cause unexpected behaviour. Install the pinned version with:\n \
cargo install --git https://github.com/logos-co/spel.git --tag {} spel",
stdout.trim(),
DEFAULT_SPEL.tag,
DEFAULT_SPEL.tag,
);
}
Comment thread src/commands/new.rs
Comment on lines +106 to +129
let spel_bin = find_spel_on_path().with_context(|| {
format!(
"spel binary not found on PATH.\n\
Install it first:\n \
cargo install --git https://github.com/logos-co/spel.git --tag {} spel",
DEFAULT_SPEL.tag
)
})?;
check_spel_version(&spel_bin);

if cmd.lez_path.is_some() {
anyhow::bail!(
"`--lez-path` is not supported with `--template spel`.\n\
`spel init` fetches LEZ via `--lez-tag`; a local path override is not forwarded.\n\
Use `--template default` if you need a local LEZ checkout."
);
}
if cmd.vendor_deps {
anyhow::bail!(
"`--vendor-deps` is not supported with `--template spel`.\n\
Vendoring is managed by `spel init` and `lgs setup`, not by scaffold directly.\n\
Use `--template default` if you need vendored deps."
);
}
Comment thread src/commands/new.rs
anyhow::bail!("spel init failed");
}

// spel init created `target/`; layer scaffold state on top.
@weboko

weboko commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

@vpavlin wow, copilot really tried hard to review your PR

@weboko

weboko commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

is it ready to review?
please, ensure to update dogfooding.md file

quickly tried:

  • lgs build idl / build client / deploy are broken for spel projects and seems due to IDL path & output mismatch
  • seems like circuits pin (0.4.1) needs to be updated, doesn't work with new spel pin for me

@weboko

weboko commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Review + hands-on dogfooding

Reviewed the diff and then actually ran the affected flows end-to-end (built the CLI, installed spel v0.5.0 from the pinned tag, created default + spel projects, ran build idl/build client). The rename/refactor itself is clean and well-commented, and the "scaffold owns pinning, spel owns scaffolding" direction is good. 342 unit tests pass. However, dogfooding surfaced two showstoppers that break the PR's own Test Plan, plus a few smaller issues. None of the new spel delegation path is covered by integration tests — which is exactly where the breakage is.


🔴 Blocker 1 — lgs build idl / build client / deploy are broken for spel projects (IDL path & output mismatch)

The IDL location contract between scaffold and spel doesn't line up:

Producer / consumer Path
scaffold scaffold.toml framework.idl.path idl/ → expects idl/<stem>.json
deploy-cache hashing (run_state.rs ~L118, tightened in this PR) reads idl/<stem>.json, bails if missing for spel
spel spel.toml / make idl <root>/spelproj-idl.json
spel generate-idl (what lgs build idl invokes) prints IDL to stdout, writes no file

Reproduced in a generated spel project (vendored spel binary in place):

  • lgs build idl → runs spel generate-idl, dumps JSON to stdout, creates no file, exits 0. src/commands/idl.rs returns cmd_spel(["generate-idl"]), and cmd_spel uses .status() (inherits stdout) — it never redirects to idl/<stem>.json the way the lez-framework branch writes its IDL.
  • lgs build clientspel ffi-genError reading IDL '…/spelproj-idl.json': No such file or directory, exit 1.
  • lgs deploy / lgs runcompute_program_hashes requires idl/<stem>.json → would bail: "expected IDL file …/idl/spelproj.json … is missing; run lgs build idl first" — and lgs build idl never produces it. Dead end.

So the Test Plan item "lgs build idl … calls spel generate-idl" is true but functionally useless: nothing is persisted, and the deploy gate this PR adds then makes deploy impossible. Fix: capture spel generate-idl stdout → write idl/<stem>.json (as the lez-framework branch does), or point framework.idl.path at spel's <root>/spelproj-idl.json convention and teach run_state to read that name.

🔴 Blocker 2 — circuits pin (0.4.1) is incompatible with the bumped spel/LEZ pins (they need 0.4.2)

constants.rs bumps DEFAULT_LEZ → v0.1.2 and DEFAULT_SPEL → v0.5.0 but leaves DEFAULT_CIRCUITS_VERSION = "0.4.1".

  • Both LEZ v0.1.2's and spel v0.5.0's Cargo.lock pull logos-blockchain rev 1da154c…, whose build script asserts circuits v0.4.2.
  • Reproduced: cargo install --git …/spel.git --tag v0.5.0 spel against circuits 0.4.1 → panic: "The logos-blockchain-circuits directory … is version 'v0.4.1', but version 'v0.4.2' is expected."
  • setup.rs provisions circuits at the 0.4.1 pin and uses it for the LEZ-sequencer, wallet and vendored-spel builds. lgs build was observed downloading …/v0.4.1/… before the (long) sequencer build — it hits the same assertion.

The doc comment derives 0.4.1 from LEZ's flake.lock (nix), but scaffold builds via Cargo, which needs 0.4.2. Net effect: lgs setup / lgs build fail on a clean machine for both default and spel projects. The comment even says "bump this in lock-step with DEFAULT_LB_PIN / DEFAULT_LEZ." Fix: DEFAULT_CIRCUITS_VERSION = "0.4.2" and verify a from-scratch lgs setup.


🟠 Medium

3. check_spel_version warns on every spel project, even when correct. spel --version (v0.5.0) prints usage to stderr and exits 1 with empty stdout; new.rs greps output.stdout for "v0.5.0" → always false. Live output during lgs new --template spel:

warning: installed spel version () does not match the expected v0.5.0 pinned by scaffold.

Note the empty (). Read stderr too (or call a real version subcommand) before deciding to warn.

4. cache_root portability regression — affects default too. The build_scaffold_config extraction changed the no---cache-root default from String::new() to the absolute bootstrap path (new.rs ~L292). Confirmed: a generated default project's scaffold.toml now contains cache_root = "/root/.cache/logos-scaffold". The deleted comment explicitly kept it empty "so scaffold.toml stays portable." This machine-specific path now ships in every committed config, not just spel.

🟡 Low

5. Flag-validation ordering. --vendor-deps / --lez-path with --template spel report "spel not found on PATH" instead of the intended "flag not supported" error, because find_spel_on_path() / check_spel_version run before the flag checks in cmd_new_spel. Validate args first (fail fast regardless of environment).

6. Generated project's spel deps don't match the recorded pin. spel init (called with only --lez-tag) emits spelproj_ffi → spel-framework-core tag="v0.4.0" and examples → branch="main", while scaffold.toml records spel v0.5.0. branch="main" is a moving target that defeats the reproducible pinning that's scaffold's job. Consider passing --spel-tag to match. (Partly upstream, but it's the seam this PR introduces.)

7. No test coverage for the new path. There are no new-command integration tests at all, and cmd_new_spel / the deprecation mapping / the flag-bail paths are untested; the removed lez_framework_overlay unit test wasn't replaced.

8. Doc/UX nits. skills/spel-template/SKILL.md L116-118 uses bare spel -- initialize … (the -- is the lgs passthrough separator), contradicting its own gotcha to use lgs spel -- …; L105 says "lgs build runs make build" (it runs scaffold's workspace build, not make). lgs new --template spel prints two separate "Next steps" blocks (spel's + scaffold's).


Flows exercised

new --template default ✅ (cache_root regression) · new --template spel ✅ delegates (warning #3, IDL #1, deps #6) · new --template lez-framework ✅ deprecation→spel · bad template ✅ correct error · spel + --vendor-deps/--lez-path ⚠️ wrong message (#5) · failure cleanup ✅ no leftover dir · help/available_templates ✅ default, spel · build idl 🔴 stdout, no file · build client 🔴 missing IDL · build/setup 🔴 circuits skew · deploy-cache requires IDL 🔴 would bail · init skills→spel-template ✅ · 342 unit + changed integration tests ✅.

Bottom line: the headline happy path (lgs new --template spel → buildable, deployable project) doesn't work today — it breaks at lgs setup (circuits 0.4.1 vs 0.4.2) and again at lgs build idl/deploy (IDL location). Both look like small, targeted fixes.

🤖 Reviewed via hands-on dogfooding in a fresh environment. Generated with Claude Code.

@danisharora099 danisharora099 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM on the direction - spel owns scaffolding, scaffold owns pins/orchestration. Agree with @weboko: IDL output doesn’t land where deploy expects, and circuits 0.4.1 vs 0.4.2 blocks setup on a clean box. Fix those two (+ cache_root / version warning if easy)

@weboko weboko 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.

Review — reviewed via end-to-end dogfooding (installed pinned spel v0.5.0) + code read

I built the branch, installed the pinned spel v0.5.0 (cargo install --git …/spel.git --tag v0.5.0), and dogfooded the template flows against it.

Verified working ✅

  • lgs new --template spel delegates to spel init <name> --lez-tag v0.1.2, then layers scaffold.toml (framework.kind = "spel") + AI skills (incl. spel-template). Resulting tree is a proper spel project (spel.toml, *_core, *_ffi, methods, Makefile). The exact invocation matches spel v0.5.0's init interface.
  • --template default unchanged; --template lez-framework prints the deprecation warning and maps to spel; unknown templates error clearly.
  • --lez-path / --vendor-deps are rejected for --template spel with actionable messages; missing-spel prints the exact cargo install command.
  • Backward compat is sound: the lez-framework kind is still handled in build/idl/client/run_state (deploy now requires an IDL for both spel and lez-framework — good).
  • Builds clean; 342 lib + 159 CLI tests pass on the branch base.

🐞 Confirmed bug — check_spel_version always warns (reproduced live)

check_spel_version runs spel --version and checks stdout.contains(DEFAULT_SPEL.tag). But spel v0.5.0 has no --version: it exits 1 and prints usage to stderr, leaving stdout empty. So the check is always false and every lgs new --template spel prints — even with the correctly-pinned spel:

warning: installed spel version () does not match the expected v0.5.0 pinned by scaffold.

The empty () makes it self-evidently wrong and will erode trust in the warning. Please detect the version via a command spel actually supports (or drop/redesign the check). A substring match on a v-prefixed tag is also fragile if spel ever prints 0.5.0 without the v.

📄 DOGFOODING.md not updated (required for this change)

This PR renames the primary framework template lez-frameworkspel but does not touch DOGFOODING.md. On current master, DOGFOODING.md has ~12 references to lez-framework — an entire L1L4 scenario series plus E3 and the invalid-template error listing (default, lez-framework). Since this is a user-facing template rename + delegation change, those scenarios need to be migrated to --template spel (including the new spel-on-PATH prerequisite and the spel generate-idl / ffi-gen delegation semantics). Please update DOGFOODING.md as part of this PR.

🔀 Staleness / rebase

The branch is a month old on an old base and conflicts with master in idl.rs, new.rs, constants.rs, tests/cli.rs. Note the constants.rs pin bump (LEZ v0.1.2 / spel v0.5.0) already landed on master via #198, so that portion of the diff is now redundant — the rebase should drop it. Please rebase and re-run the template scenarios.

Minor

  • find_spel_on_path re-implements crate::process::which; consider which("spel") for consistency (it already handles the .exe case).
  • Architecture question for maintainers: --template spel now requires a separately cargo installed spel on PATH, unlike the self-contained default template. Given master vendors + builds spel during lgs setup, is a global spel prerequisite at new time the intended UX, or should new bootstrap/point at the vendored spel? Worth aligning before this lands.

Not approving — comments only. The direction (minimal scaffold role, delegate scaffolding to spel init) is reasonable and the happy path works end-to-end; the version-check bug + DOGFOODING update + rebase are the blockers I'd want resolved first.

@weboko weboko 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.

Independent re-review — spel delegation works, but the PR is not mergeable as-is and the version check is confirmed broken

Built the branch, installed the pinned spel v0.5.0 (cargo install --git …/spel.git --tag v0.5.0, with logos-blockchain-circuits v0.4.1), put it on PATH, and dogfooded the delegation live.

Verified working ✅

  • lgs new --template spel <name> delegates to spel init, producing a proper spel project — spel.toml, <name>_core, <name>_ffi, methods/, Makefile, examples/ — and correctly pins LEZ v0.1.2 (cf3639d8) + spel v0.5.0 (73fc462e), then layers scaffold.toml and AI skills (.claude/skills/, .cursor/rules/, AGENTS.md, incl. the new spel-template skill). The delegation itself is sound.

🚫 Blocker — not mergeable, and superseded-in-part by work that has since landed

  • GitHub reports this PR mergeable: false / mergeable_state: dirty. Its merge-base is 2026-06-02 (#85), i.e. it predates the substantial spel-as-build-tool integration that has since landed on master (DEFAULT_SPEL, [repos.spel], check_spel_lez_alignment, SPEL_BIN_REL_PATH, spel rows in doctor, etc.). This PR deletes templates/lez-framework/** and skills/lez-framework-template/, but on current master the lez-framework variant is still a live code path (FRAMEWORK_KIND_LEZ_FRAMEWORK in new.rs). It needs a rebase onto master and explicit reconciliation of the framework-kind story before it can merge or be meaningfully re-reviewed.

🐞 Confirmed bug (reproduced live) — check_spel_version always warns, with an empty version string

check_spel_version runs spel --version and tests stdout.contains(DEFAULT_SPEL.tag). I confirmed against the real pinned binary that spel v0.5.0 has no --version: it exits 1, prints usage to stderr, and leaves stdout empty. So "".contains("v0.5.0") is always false and every lgs new --template spel prints a spurious warning — even with the exact pinned version installed. Live output:

warning: installed spel version () does not match the expected v0.5.0 pinned by scaffold.

Note the empty () — the message interpolates stdout.trim(), which is empty. There is also no spel version subcommand (it just prints usage). The check needs a real version-detection mechanism (or should be dropped) — as written it is pure false-positive noise on the happy path.

📄 Docs — DOGFOODING.md not updated for the new flow

This PR introduces a whole new scaffolding path (--template spelspel init) but does not add any DOGFOODING.md coverage for it, and master's DOGFOODING.md has no --template spel scenario either. Given the significance of the new flow, a dogfooding scenario should be added.

Recommendation

The core delegation is good and worth landing, but not approvable in its current state: (1) rebase onto current master and reconcile with the already-merged spel plumbing / the still-live lez-framework path; (2) fix check_spel_version (spel has no --version); (3) add a DOGFOODING.md scenario for --template spel.

@weboko

weboko commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Status update — now conflicted, and the template conflicts are modify/delete against the CI fix in #224

No new commits since 47ddccf (2026-06-02), so this is not a re-review of the diff — my 2026-07-20 pass stands. But master has moved to 9fcc376 and the branch no longer merges, and the shape of the conflicts matters more here than the count, so recording it.

CONFLICT (content): Merge conflict in src/commands/build.rs
CONFLICT (content): Merge conflict in src/commands/idl.rs
CONFLICT (content): Merge conflict in src/commands/new.rs
CONFLICT (content): Merge conflict in src/template/project.rs
CONFLICT (modify/delete): templates/lez-framework/Cargo.toml.template
CONFLICT (modify/delete): templates/lez-framework/README.md
CONFLICT (modify/delete): templates/lez-framework/crates/lez-client-gen/Cargo.toml.template
CONFLICT (modify/delete): templates/lez-framework/crates/lez-client-gen/src/main.rs
CONFLICT (modify/delete): templates/lez-framework/methods/guest/Cargo.toml.template
CONFLICT (modify/delete): templates/lez-framework/methods/guest/src/bin/lez_counter.rs
CONFLICT (modify/delete): templates/lez-framework/src/lib.rs

Verified locally with git merge-tree 9fcc376 47ddccf.

The seven modify/delete conflicts are the interesting ones

This PR deletes templates/lez-framework/ (renaming it to spel and delegating scaffolding to spel init). Since this branch's merge base, master has modified every one of those seven files — via #201 (docs: lead onboarding with lgs run) and, more importantly, #224 (ci(template-e2e): make rendered template builds pass end to end).

That is not a formatting collision. #224 is the commit that made the render-and-build (lez-framework) CI leg actually pass — it fixed real compile errors in the rendered template (the AccountId::for_public_pda change, the duplicate nssa_core in the dependency graph). Git has left master's version in the tree for all seven.

So the resolution question is a design question, not a mechanical one: do the fixes #224 made to lez-framework need to be carried into the spel path, or does spel init make them moot?

  • If spel init generates the project (rather than this repo's template rendering it), then #224's fixes live upstream in spel and deleting these files is correct — but someone needs to confirm the upstream spel templates already carry the equivalent of #224, otherwise the render-and-build leg for the new template will fail the same way lez-framework did before #224.
  • If the repo still renders any of it locally, the fixes have to be ported.

I can't answer that from this side without knowing the spel init output. @vpavlin — could you confirm which it is? That determines whether the deletes are safe to accept wholesale.

Also worth knowing

#241 (fix: repair the dogfooding critical path) touched the spel program-id handling on master after this PR was opened. Since this PR is the one introducing spel as the template path, that work may overlap or conflict semantically even where git reports no textual conflict in those files — worth diffing #241's spel-related changes against what this branch does before resolving.

Given the age (opened 2026-06-02, last commit two months ago) and that the four src/ conflicts are in files #221/#241 also rewrote, a rebase onto current master followed by a fresh render-and-build run is the minimum before this can be assessed for merge. Happy to re-run the L-series (L1–L4) end-to-end on the rebased head once that's done — that is the scenario set this PR actually changes, and it is the thing that would catch a missing #224-equivalent fix in the spel output.

(Comment only, not an approval.)


Generated by Claude Code

@weboko

weboko commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Re-review on the new base (7523ee2) — I can now answer my own question to @vpavlin, and the DOGFOODING.md ask from 2026-06-02 is still open

No new commits since 47ddccf, so this is not a re-review of the diff. master moved 9fcc3767523ee2 since my 2026-08-04 note, and this time I built the branch and ran the affected paths on a from-scratch environment (Rust 1.94.1, Nix 2.35.2, risc0 3.0.6 — r0vm/cargo-risczero 3.0.6, guest toolchain 1.97.0, C++ 2024.1.5) rather than only inspecting the merge. That turned up several things the previous passes did not.

First: the open question from 2026-08-04 is partly answered — by #224 itself

I asked whether #224's fixes need porting into the spel path or whether spel init makes them moot. Reading #224's actual diff answers half of it: #224 already migrated templates/lez-framework/ to the spel_framework API. It is not a formatting fix, it is the same rename this PR is doing, done in place:

- use lez_framework::prelude::*;
- use lez_framework::error::{LezError, LezResult};
- use lez_framework_core::types::LezOutput;
- use nssa_core::program::AccountPostState;
+ use spel_framework::prelude::*;

- ) -> LezResult {
-     Ok(LezOutput::states_only(vec![
-         AccountPostState::new_claimed(counter.account.clone()),
-         AccountPostState::new(authority.account.clone()),
-     ]))
+ ) -> SpelResult {
+     Ok(SpelOutput::execute(vec![counter, authority], vec![]))

So master's templates/lez-framework/ is already spel-API content under a legacy directory name, and it is known-good — #224 is the commit that made render-and-build (lez-framework) pass. This branch's copies of those seven files predate that migration.

That sharpens the resolution question rather than removing it. The seven modify/delete conflicts are no longer "old template vs new template" — they are "a working, CI-verified spel-API template that master currently ships" vs "delete it and delegate to spel init". Accepting the deletes is only safe if spel init at the pinned v0.5.0 (73fc462e) emits a program at least as current as post-#224 lez_counter — same SpelOutput::execute shape, same mut counter binding, same dependency graph without the duplicate nssa_core. @vpavlin, that is the specific thing to confirm, and it is now checkable by diffing spel init output against master:templates/lez-framework/.

An eighth modify/delete conflict has also appeared since 2026-08-04, same shape as the other seven:

CONFLICT (modify/delete): skills/lez-framework-template/SKILL.md

The DOGFOODING.md ask from 2026-06-02 has never been actioned

On 2026-06-02 I asked: "please, ensure to update dogfooding.md file". Two and a half months on, this branch touches neither DOGFOODING.md nor README.md — confirmed against the merge base:

git diff <merge-base>...47ddccf --stat -- DOGFOODING.md README.md   ->  (empty)

That is not a documentation nicety here, because DOGFOODING.md is executable procedure and this PR invalidates it in 14 places (README adds 4 more). Concretely, on master:

DOGFOODING.md What this PR breaks
L172–175 The entire L1–L4 scenario table is defined as lez-framework, invoked via new --template lez-framework
L178 E3 (AI skills) runs new --template lez-framework; this PR renames the skill dir to skills/spel-template/
L665, L926 Literal shell lines: "$SCAFFOLD_BIN" new dogfood-lez --template lez-framework
L936 Asserts the invalid-template error lists (default, lez-framework) — now (default, spel), so the documented expectation is simply wrong
L124 Says the lez-framework guest compiles C and therefore needs the risc0 C++ toolchain — unverified for spel init output, and a prerequisite users hit hard if it changed

L1–L4 are exactly the scenarios this PR changes, and they are the ones I offered to run. I could not: see below.

Blocker — --template spel requires a global spel, which contradicts scaffold's core property

This is the finding I'd most like a response on. Verified on a clean machine with the branch built:

$ which spel
  (not found)

$ lgs create demo-spel --template spel
error: spel binary not found on PATH.
Install it first:
  cargo install --git https://github.com/logos-co/spel.git --tag v0.5.0 spel: spel not found on PATH

cmd_new_spel calls find_spel_on_path() (src/commands/new.rs:346), which only walks PATH. But scaffold's stated contract — the one lgs --help advertises and #226 is currently rewriting the README around — is "setup builds the pinned sequencer, wallet, and spel toolchain locally: no global installs, portable across machines and CI." setup puts spel at project-local SPEL_BIN_REL_PATH (target/release/spel), never on PATH.

So there's a genuine ordering gap: create needs spel before setup exists to build it, and because setup's spel is project-local, even a user with ten scaffold projects still has no spel on PATH for the eleventh. The only way through is the global cargo install --git the error suggests — an unpinned-by-scaffold, machine-global toolchain, which is precisely what the rest of the design avoids.

The fix looks like it's already half-wired: cmd_new_spel receives bootstrap_cache but only passes it to build_scaffold_config. cmd_new_default in the same file already demonstrates the pattern — it clones LEZ at DEFAULT_LEZ.sha into ~/.cache/logos-scaffold/repos/lez/<sha> on demand (I watched it do exactly that during dogfooding). Fetching and building spel at DEFAULT_SPEL.sha into the same bootstrap cache would keep the pin authoritative, keep the "no global installs" property, make CI reproducible, and let check_spel_version become an assertion instead of a warning.

As it stands I could not run L1–L4 against the spel path at all without a global install, so the scenario set this PR changes remains unexercised end to end — by me, and (given tests/cli.rs grew only 19 lines and no test covers cmd_new_spel's delegation) by the test suite.

Smaller things

Copy-paste-hostile error. The message above concatenates the anyhow context and its source, so the suggested command runs straight into the error text:

  cargo install --git https://github.com/logos-co/spel.git --tag v0.5.0 spel: spel not found on PATH

A user selecting that line copies ... spel: spel not found on PATH. Since this is the first thing every spel user will hit, it's worth restructuring so the command is on its own line, or dropping the redundant spel not found on PATH source.

check_spel_version is a substring test. src/commands/new.rs:333 does stdout.contains(DEFAULT_SPEL.tag), so v0.5.0 also matches a spel 0.5.01/v0.5.0-rc1 build, and any future v0.5.0 prefix collision. Given the header comment above it explicitly worries about "hard-to-diagnose mismatches", parsing the version and comparing exactly would match the stated intent.

Backward compatibility is handled well — worth saying. --template lez-framework still works and warns:

warning: template `lez-framework` is deprecated; use `--template spel` instead.

and FRAMEWORK_KIND_LEZ_FRAMEWORK is retained with a deprecation comment for existing scaffold.toml files. The --lez-path / --vendor-deps rejections are explicit and explain the alternative. That part of the ergonomics is good.

Build status

The branch builds and tests clean on its own basecargo build --release clean, cargo test --locked --release 501 green (342 lib + 159 CLI), zero failures. So nothing here is about the code being broken in isolation; it's about the rebase, the docs, and the bootstrap gap.

Where this leaves it

Not mergeable as-is, and the gap has widened rather than narrowed since June. Before the next pass I'd want, in rough priority order:

  1. A decision on the global-spel bootstrap — I'd argue for fetching the pinned spel into the bootstrap cache, consistent with how cmd_new_default already handles LEZ.
  2. DOGFOODING.md updated (L1–L4, E3, the two script lines, the L936 assertion, and the L124 C++ prerequisite re-verified against spel init output) plus the 4 README references — the ask from 2026-06-02.
  3. Confirmation that spel init at v0.5.0 emits a program at least as current as post-#224 templates/lez-framework/, before the seven deletes are accepted.
  4. The rebase onto 7523ee2 (12 conflicts, 8 of them modify/delete), then a fresh render-and-build run.
  5. Test coverage for cmd_new_spel — currently none.

Happy to re-run L1–L4 end to end the moment the bootstrap question is settled; that's the check that would actually catch a missing #224-equivalent in the spel output.

(Comment only, not an approval.)


Generated by Claude Code

@weboko

weboko commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

New finding on the current base (834b6b0) — the rebase also breaks template-e2e, which this branch has never run against

No new commits since 47ddccf, so this is not a re-review of the diff; my 2026-08-17 review stands unchanged and I'm not repeating it. I picked this back up because master moved 7523ee2834b6b0 (#251, #252). Neither touches this PR's files, and the merge state is identical in shape — still 12 conflicts, 8 of them modify/delete:

CONFLICT (modify/delete): skills/lez-framework-template/SKILL.md
CONFLICT (content):       src/commands/build.rs, src/commands/idl.rs,
                          src/commands/new.rs, src/template/project.rs
CONFLICT (modify/delete): templates/lez-framework/{Cargo.toml.template, README.md,
                          src/lib.rs, methods/guest/Cargo.toml.template,
                          methods/guest/src/bin/lez_counter.rs,
                          crates/lez-client-gen/Cargo.toml.template,
                          crates/lez-client-gen/src/main.rs}

But re-checking the rebase target surfaced something none of the previous passes caught, and it turns the bootstrap gap from an ergonomics argument into a hard CI failure.

render-and-build (lez-framework) will fail at the render step after the rebase

.github/workflows/template-e2e.yml did not exist at this branch's merge base (7e5ae8e0) — it landed later in #196 — so render-and-build has never run on this PR. It will after the rebase, because the workflow is path-gated on exactly the three paths this PR changes:

# .github/workflows/template-e2e.yml:14-19
on:
  pull_request:
    paths:
      - "src/constants.rs"      # this PR: +20/-... (DEFAULT_SPEL, FRAMEWORK_KIND_*)
      - "templates/**"          # this PR: deletes templates/lez-framework/ entirely
      - "src/template/**"       # this PR: src/template/project.rs, src/template/skills.rs

And its matrix still names the template this PR retires:

# .github/workflows/template-e2e.yml:36-37
      matrix:
        template: [default, lez-framework]
...
# line 90
          "$GITHUB_WORKSPACE/target/release/logos-scaffold" new e2e-app --template "${{ matrix.template }}"

The deprecated alias is kept working (cmd_new, src/commands/new.rs:38-42), so --template lez-framework no longer renders the deleted directory — it warns and routes to cmd_new_spel, which calls find_spel_on_path(). The runner has no global spel, and nothing in the job installs one: the only step that builds spel is Setup (clone + build lez/spel at configured pins) at line 92, which runs after the render step at line 90 — and even when it runs it puts spel at the project-local SPEL_BIN_REL_PATH, never on PATH. So the leg dies at line 90 with the same error I hit by hand on a clean machine:

error: spel binary not found on PATH.
Install it first:
  cargo install --git https://github.com/logos-co/spel.git --tag v0.5.0 spel: spel not found on PATH

Two things follow:

  1. The matrix needs updating as part of this PRtemplate: [default, spel] — otherwise the rebase lands a red required-ish check. Renaming the leg is the easy half.
  2. It cannot be fixed by the matrix alone. Even as spel, the leg still calls new --template spel before any spel exists on the runner. That is the bootstrap gap from my last review, now with a concrete failing consumer: template-e2e cannot be made green for the spel template without either (a) fetching/building the pinned DEFAULT_SPEL into the bootstrap cache the way cmd_new_default already does for LEZ, or (b) adding a global cargo install --git ... --tag v0.5.0 spel step to the workflow. Option (b) would make CI install an unpinned-by-scaffold global toolchain, which is exactly the property setup exists to avoid — so this is CI evidence for option (a), the fix I argued for last time.

This is why I keep coming back to item 1 on my list: it is not a style preference about global installs, it is the thing standing between this PR and a green render-and-build.

Also still open, unchanged

Nothing else has moved since 2026-08-17, so briefly, in priority order:

  1. The global-spel bootstrap decision (above — now CI-blocking).
  2. DOGFOODING.md never updated (the ask from 2026-06-02): L1–L4 are defined as lez-framework, E3 renders it, L665/L926 are literal --template lez-framework shell lines, L936 asserts the error lists (default, lez-framework), and L124's risc0 C++ prerequisite is unverified for spel init output. Plus 4 README references. This is a big-functionality PR whose runbook has not been touched at all — and with the workflow point above, the same rename is now unapplied in three places: the runbook, the README, and CI.
  3. Confirmation that spel init at v0.5.0 (73fc462e) emits a program at least as current as post-#224 templates/lez-framework/ (SpelOutput::execute shape, no duplicate nssa_core), before the seven deletes are accepted.
  4. The rebase itself (12 conflicts).
  5. Test coverage for cmd_new_spel — still none; tests/cli.rs grows 19 lines and none of them exercise the delegation.

I still could not run L1–L4 end to end for the same reason as last time — no global spel, by design — so the scenarios this PR changes remain unexercised by me and by the suite. Happy to run them the moment item 1 is settled.

(Comment only, not an approval.)


Generated by Claude Code

@weboko

weboko commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Re-review: needs a rebase before it can be judged, and two design points to settle first

Environment: Nix 2.35.2, Rust 1.94.1, r0vm 3.0.5 + risc0 guest toolchains (rust r0.1.91.1, cpp 2024.01.05), real LEZ sequencer + wallet from cf3639d8.

I could not evaluate this one the way I evaluated the others, because it does not merge and cannot be built against current master. Flagging the mechanical state first, then two things I'd want settled before a rebase, so the rebase only has to happen once.


The branch is 12 conflicts and 37 master commits behind

Head is 47ddccf (2026-06-02); git merge-tree origin/master pr-199 reports 12 conflicts:

  • 4 content conflicts: src/commands/build.rs, src/commands/idl.rs, src/commands/new.rs, src/template/project.rs
  • 8 modify/delete conflicts: master has since modified files this PR deletesskills/lez-framework-template/SKILL.md and seven files under templates/lez-framework/ (Cargo.toml.template, README.md, the client-gen crate, the guest, src/lib.rs)

The modify/delete class is the awkward one: master has kept actively developing the template this PR removes, so those aren't mechanical resolutions — someone has to decide, per file, whether master's newer content should be ported into spel init's output or dropped. That's a judgement call that belongs to the author, and it's the reason I'm not offering to rebase this myself.

1. DOGFOODING.md is not touched, and this PR invalidates 14 references in it

This is the gap I'd fix first, because it's mechanical and it defines the acceptance criteria for everything else. Master's runbook references lez-framework 14 times; this PR changes zero lines of it:

  • The scenario index rows for L1–L4 are all keyed lez-framework (new --template lez-framework, setup, build idl, build client, deploy, run_lez_counter).
  • E3 lists new --template lez-framework in its command surface.
  • E2 (line 937) asserts the exact error text: "Invalid --template name fails with a clear error listing the available templates (default, lez-framework)" — this PR changes that string to Expected `default` or `spel` (or the deprecated alias `lez-framework`), so E2 fails as written the moment this lands.
  • Line 124 documents the risc0 C++ toolchain requirement as a property of "the lez-framework template's guest", and line 811 documents the HOST_CC/HOST_CXX gotcha for "lez-framework projects". Whether either still holds for a spel init-generated project is exactly the kind of thing only the author knows.

A template rename plus a delegation to an external scaffolder is squarely "major user-facing behavior materially changed" under the runbook's own maintenance note. L1–L4 need to be rewritten against spel, E2's expected error updated, and the provisioning section re-checked — and the new precondition in point 2 below needs to be stated in Shared Preconditions.

2. Requiring spel on PATH contradicts scaffold's "no global installs" contract

let spel_bin = find_spel_on_path().with_context(|| {
    format!("spel binary not found on PATH.\nInstall it first:\n  \
             cargo install --git https://github.com/logos-co/spel.git --tag {} spel", DEFAULT_SPEL.tag)
})?;

Two problems, one practical and one architectural.

Practical — it's a first-run regression. Today lgs new --template lez-framework is self-contained. I verified this on #253's head today on a machine with no spel anywhere: lgs new my-app --template lez-frameworklgs run[1/5][5/5], lez_counter deployed against a real sequencer, lgs wallet -- check-health green. After this PR, that same first command fails until the user runs a cargo install --git from a git tag. That is a hard stop on the very first command of the documented quick start, and it lands on the deprecated alias too — FRAMEWORK_KIND_LEZ_FRAMEWORK maps to FRAMEWORK_KIND_SPEL before cmd_new_spel runs, so --template lez-framework inherits the requirement rather than continuing to work.

Architectural — it inverts the dependency order. setup is what builds spel, and setup runs after new. So new now needs a globally-installed spel in order to create the project whose setup would have built spel locally. That's circular, and it cuts against the principle the README states explicitly for every other dependency: "All binaries are project-local and are not installed to PATH" / "no global installs, portable across machines and CI." Every other external dep here is pinned by sha and built into the cache; spel would become the one that must be ambient.

If delegating to spel init is the right long-term call — and the argument for not maintaining a second copy of the template is a good one — the shape that preserves the contract is for scaffold to sync and build spel at the pinned rev into the cache first (as setup already does), then invoke that binary. new doing its own minimal spel provisioning is more work than shelling out to PATH, but it keeps lgs new a single command.

3. find_spel_on_path / check_spel_version are weaker than the rest of scaffold's pinning

fn find_spel_on_path() -> anyhow::Result<std::path::PathBuf> {
    for dir in std::env::split_paths(&path_var) {
        let candidate = dir.join("spel");
        if candidate.is_file() { return Ok(candidate); }

and the version check is advisory only:

let output = match Command::new(spel_bin).arg("--version").output() {
    Ok(o) => o,
    Err(_) => return,          // unrunnable binary → silently accepted
};
if !stdout.contains(DEFAULT_SPEL.tag) { eprintln!("warning: ..."); }   // mismatch → warning, proceeds

So scaffold will execute the first file named spel on PATH, whatever it is: is_file() doesn't check the executable bit, a binary that fails to run is accepted silently, and a version mismatch only prints a warning to stderr before proceeding to run it. Every other dependency in this codebase is pinned to a commit sha and built from source; this one is "whatever is on PATH." On a developer box with a stale or shadowed spel, the failure mode is a silently wrong project layout, and the warning scrolls past inside spel init's own output.

Minimum I'd want: fail closed on a version mismatch (with --allow-spel-version-mismatch or similar for the deliberate case) rather than warn-and-continue, and treat an unrunnable spel as an error rather than a pass. Better still, resolve spel from the pinned cache per point 2, at which point this function goes away.

Smaller things

  • cmd_new_spel rejects --lez-path and --vendor-deps with good, specific errors — nice, and the right call over silently ignoring them. Worth mirroring in docs/commands.md / the runbook's E2 flag matrix so the restriction is discoverable before you hit it.
  • spel init is invoked with cmd.name as a positional arg while scaffold computes target = cwd.join(&cmd.name) independently. Those two agree for a plain name, but a name containing a path separator relies on spel init resolving it identically to Path::join. Worth either validating name as a single path component up front or deriving the target from what spel init actually created.
  • The refactor into cmd_new_spel / cmd_new_default / build_scaffold_config is a genuine readability win and I'd keep it through the rebase regardless of how the above lands.

Verdict: not approvable in its current state — it doesn't merge, and the two design points above would change the diff substantially, so rebasing first would waste the effort. Concretely, I'd suggest: settle the spel-on-PATH question (point 2) → rebase onto master, resolving the 8 modify/delete conflicts deliberately → update DOGFOODING L1–L4/E2/E3 → then re-request review. The direction (one canonical scaffolder, no duplicated template) is right; it's the provisioning contract and the three-month drift that need resolving.

Happy to re-review promptly once it's rebased — I have a full toolchain provisioned and can run the L-series end-to-end against whatever spel init produces.


Generated by Claude Code

@weboko

weboko commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Delta since my 2026-08-25 review: master's docs were rebuilt under this branch, so the doc-update scope grew

No new commits here — head is still 47ddccf — so this is not a re-review, and my 2026-08-25 review stands in full (the spel-on-PATH contract, find_spel_on_path/check_spel_version weakness, the 8 modify/delete conflicts). I am not restating any of it.

I picked this back up because master moved 834b6b0dc2bddb, and one of those commits (#253, docs: rebuild the public surface around install and first run) landed directly on top of surfaces this PR invalidates. That changes the size of the "update the docs" item, so it is worth recording before the rebase happens rather than after.

Mechanical state: unchanged in shape, further behind

$ git merge-tree origin/master pr-199 | grep -c CONFLICT
12
$ git rev-list --count pr-199..origin/master
40

Still 12 conflicts, still 8 of them modify/delete. 37 → 40 commits behind.

New: this PR now breaks two things in the primary README

#253 rewrote README.md (481 lines changed). Two of the lines it landed depend on exactly what this PR removes.

1. The quick start's first command is the deprecated alias. README.md:15-18 on dc2bddb:

cargo install logos-scaffold
lgs new my-app --template lez-framework

That is now the headline example a new user copies. After this PR, --template lez-framework prints warning: template 'lez-framework' is deprecated; use '--template spel' instead. and routes into cmd_new_spel, which hard-fails with spel binary not found on PATH unless the user has already run a cargo install --git. So the first two lines of the documented quick start stop working together. This is the same first-run regression I raised as point 2 on 2026-08-25, but it has moved from "the L-series in DOGFOODING.md" to "the README's headline example," which raises its severity.

2. A relative link in the README points at a file this PR deletes. README.md:209:

See the [LEZ Framework template README](./templates/lez-framework/README.md)
for details.
$ git diff --name-status $(git merge-base origin/master pr-199)..pr-199 -- templates/lez-framework/README.md
D	templates/lez-framework/README.md

That is a dead link on the repo's front page, not just stale prose. Whatever replaces it has to point somewhere spel init output actually documents itself, which may be upstream rather than in this repo — a question worth answering before the rebase, since it also decides what skills/spel-template/SKILL.md should link to.

Also new: docs/commands.md is a doc surface that did not exist when this was written

#253 moved the exhaustive command reference out of the README into docs/commands.md (195 new lines). Its new/create synopsis is:

logos-scaffold new <name> [--template NAME] [--vendor-deps] [--lez-path PATH] [--cache-root PATH]

--vendor-deps and --lez-path are both listed unconditionally, and this PR makes both a hard error under --template spel. My 2026-08-25 note asked for that restriction to be discoverable; docs/commands.md is now the specific place it belongs, alongside the set of valid --template values.

So the doc checklist for the rebase is DOGFOODING.md (L1–L4, E2's asserted error string, E3, the C++/HOST_CC notes at lines 124 and 812) plus README.md (quick start, the LEZ Framework section and its link) plus docs/commands.md. All three are mechanical once the design point in my point 2 is settled — but all three are now load-bearing user-facing docs rather than internal runbooks.

Verdict unchanged: not approvable as it stands. The direction — one canonical scaffolder instead of a duplicated template — is still right, and the cmd_new_spel/cmd_new_default/build_scaffold_config split is still a readability win worth carrying through the rebase. The blockers are the same ones: settle whether spel is provisioned from the pinned cache or required on PATH, then rebase and resolve the 8 modify/delete conflicts deliberately, then update the three doc surfaces above.

I have not pushed anything to this branch; the modify/delete resolutions are judgement calls that belong to the author. Happy to re-review promptly once it is rebased — I have a full toolchain provisioned (Nix 2.35.2, Rust 1.94.1, r0vm 3.0.5/3.0.6, risc0 guest rust 1.97.0 + cpp 2024.1.5) and can run the L-series end-to-end against whatever spel init produces.


Generated by Claude Code

@weboko

weboko commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

New findings from a code-level pass — two of them change the rebase plan, so they are worth having before the rebase

No new commits here (head is still 47ddccf), and master only moved dc2bddbfb42142 (#257, wallet/deploy connectivity — it touches none of this PR's files, and its DOGFOODING.md edit is confined to the deploy/topup failure-signal lists). So the mechanical picture is unchanged: 12 conflicts, 8 of them modify/delete, now 41 commits behind.

My 2026-08-25 and 2026-08-27 reviews stand in full and I am not restating them. This pass is different: I stopped waiting on the rebase and read the delegation against current master's code. Seven things came out of that, and the first two are the useful ones — one shrinks the rebase, and one supplies the mechanism I asked for on 2026-08-25 without needing a design decision at all.

Environment: Nix 2.35.2, Rust 1.94.1, r0vm 3.0.5, risc0 guest toolchains rust r0.1.91.1 + cpp 2024.01.05.


1. The constants.rs pin bump has already landed on master — that commit is now a no-op

935825f ("bump DEFAULT_SPEL to v0.5.0 and DEFAULT_LEZ to v0.1.2") is fully superseded. Current master and this branch are byte-identical here:

pub(crate) const DEFAULT_LEZ:  GitRef = GitRef { sha: "cf3639d8…", tag: "v0.1.2" };
pub(crate) const DEFAULT_SPEL: GitRef = GitRef { sha: "73fc462e…", tag: "v0.5.0" };

master also independently removed the same "Special note on DEFAULT_SPEL" comment. So the only constants.rs delta this PR still owns is adding FRAMEWORK_KIND_SPEL — and constants.rs is not in the conflict set precisely because both sides converged. One less thing to reason about during the rebase.

2. Scaffold already has the pinned-provisioning mechanism — this PR uses it in three files and then bypasses it in the fourth

This is the one I want to correct myself on. On 2026-08-25 I framed "provision spel from the pinned cache instead of PATH" as a design decision to settle. It is not a decision — the code already exists, and this PR already calls it.

src/commands/spel.rs:31 on master resolves spel from the pinned, project-local cache:

let spel_bin = resolve_repo_path(project, &project.config.spel, "spel")?.join(SPEL_BIN_REL_PATH);

and this PR's own idl.rs and client.rs route through it — cmd_spel(vec!["generate-idl"]), cmd_spel(vec!["ffi-gen"]). So build idl and build client use the pinned binary. Only cmd_new_spel reaches for find_spel_on_path().

The chicken-and-egg objection ("there is no project yet during new") does not hold either, because cmd_new_default — the sibling function in the same file — solves exactly that problem for LEZ, twenty lines up:

// src/commands/new.rs:120-140 (master)
let lez_cached = bootstrap_cache.join("repos/lez").join(DEFAULT_LEZ.sha);
sync_repo_to_pin_at_path_with_opts(lez_source, &lez_cached, DEFAULT_LEZ.sha,)?;

cmd_new_inner has already created bootstrap_cache/{repos,builds,state,logs} before dispatching. So the shape is: sync spel at DEFAULT_SPEL.sha into bootstrap_cache/repos/spel/<sha>, cargo build --release, invoke that binary. find_spel_on_path and check_spel_version both disappear, the first-run regression disappears, and template-e2e can go green without a global cargo install. It is more code than shelling out to PATH, but it is code that already exists in this file.

3. --lez-tag passes a mutable tag where everything else pins a sha — and there is an invariant that depends on it

.arg("--lez-tag").arg(DEFAULT_LEZ.tag)   // "v0.1.2"

DEFAULT_LEZ.sha is in the same struct and is what cmd_new_default and setup use. doctor.rs::check_spel_lez_alignment exists specifically to catch spel and scaffold disagreeing about which LEZ they are on. Resolving the spel side through a tag that upstream can move or replace is the one input that check cannot protect against. Same shape in the install hint (cargo install --git … --tag v0.5.0) and in check_spel_version, which compares --version stdout against DEFAULT_SPEL.tag rather than the sha.

4. spel init succeeding is not checked against target actually existing

if !status.success() { anyhow::bail!("spel init failed"); }
// spel init created `target/`; layer scaffold state on top.
fs::create_dir_all(target.join(".scaffold/state"))?;

target is scaffold's own cwd.join(&cmd.name); the directory is spel init's to create. If the two ever disagree — a name with a path separator, a name beginning with - that spel init parses as a flag, or any future sanitising on spel's side — create_dir_all silently creates the divergent path and scaffold.toml + skills land in a stub next to the real project, with a success message. A if !target.exists() { bail!(…) } between those two statements closes it. (cmd_new_default needs no such check because it creates the directory itself.)

5. Net test coverage goes down

Beyond "no tests for cmd_new_spel" (my item 5 from 2026-08-19), this PR removes existing coverage without replacing it:

  • src/template/project.rs: deletes lez_framework_overlay_converts_template_manifests_to_cargo_toml entirely.
  • same file: narrows the no-unresolved-placeholder loop from ["default", "lez-framework"] to ["default"].
  • tests/cli.rs (+9/-10) only rewrites existing assertions for renamed strings.

There is also an ordering issue that makes the new error paths unreachable in a test environment, which is worth fixing for its own sake: find_spel_on_path() runs before the --lez-path / --vendor-deps rejections, so on any machine without a global spel those two well-written errors are masked by spel binary not found on PATH. Hoisting the two flag checks above the lookup makes them both unit-testable and faster to hit. (Copilot flagged the ordering on 2026-06-02; the testability consequence is the part worth acting on.)

6. lgs build and lgs build client diverge for spel in a way they never did for lez-framework

// build.rs — cmd_build_shortcut
FRAMEWORK_KIND_LEZ_FRAMEWORK => { build_idl_for_current_project()?; generate_clients_from_current_idl()?; }
FRAMEWORK_KIND_SPEL          => { build_idl_for_current_project()?; }   // no client gen

but build client on a spel project does run cmd_spel(["ffi-gen"]). So for lez-framework, lgs build produces clients; for spel it does not, and the user has to know to run make ffi / lgs spel -- ffi-gen. The inline comment explains the intent, but nothing user-facing does. Whatever the call, it needs to be stated in docs/commands.md and in the runbook, because it is a silent behavioural difference between two templates that otherwise present as a rename.

Credit where due on the adjacent surface: adding spel to available_templates() is the right fix and keeps the three cli.rs help strings that consume it accurate. One that got missed, though — src/api/mod.rs:621 still documents CreateProjectOptions::template as Template name (`default` or `lez-framework`). That file is not in this PR's 25, and it is the public library API, so it wants the same rename.

7. On the doc scope — the templates/lez-framework/ deletes are load-bearing, and L4 loses its subject

I can now answer my own open item 3 without a rebase. templates/lez-framework/ is not a standalone template on master — it is an overlay applied on top of the pinned LEZ example:

// src/commands/new.rs:199-215 (master)
let template_root = lez_repo_path.join("examples/program_deployment");
copy_dir_contents(&template_root, target)?;
apply_overlay(target, template_variant, &overlay_ctx)?;      // ← templates/lez-framework/

I verified this by running it on fb42142 with no spel anywhere on the machine: lgs new dogfood-lez --template lez-framework succeeded and produced src/bin/run_lez_counter.rs, crates/lez-client-gen/, methods/guest/src/bin/lez_counter.rs, idl/lez_counter.json — every one of those files comes from the seven overlay files this PR deletes. (That run also re-confirms the first-run regression from point 2, and that E2's asserted error string is still Expected `default` or `lez-framework`. on master.)

Cross-referencing against this PR's own skills/spel-template/SKILL.md, spel init output is a different shape, not a renamed one: <name>_core/, <name>_ffi/, spel.toml, a Makefile — and no crates/lez-client-gen/ and no src/bin/run_lez_counter.rs.

So the DOGFOODING.md item is not the rename I have been describing it as. L4 (deploy, cargo run --bin run_lez_counter) has no counterpart in spel output and has to be re-authored against whatever the spel project's runner actually is, not just re-keyed. Same for L3, whose artefact moves from the client-gen crate to <name>_ffi/. The SKILL.md itself is genuinely good — accurate, well-scoped, no stale paths — it just happens to document how much the generated project changes.


Verdict unchanged: comment only, not an approval. The direction is still right and the cmd_new_spel / cmd_new_default / build_scaffold_config split is still worth carrying through the rebase.

What I would change about the plan I gave on 2026-08-25: point 2 is no longer a design question to settle — sync_repo_to_pin_at_path_with_opts + bootstrap_cache is sitting in the same file, so I would do that first, delete find_spel_on_path/check_spel_version with it, and only then rebase. Drop 935825f while you are there (point 1). The doc work is larger than I said, because of point 7.

Toolchain is provisioned and I can run the L-series end-to-end against whatever spel init produces as soon as this rebases.


Generated by Claude Code

@weboko

weboko commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Follow-up: I finished provisioning and ran the pinned spel directly. Two runtime bugs — the second one means a spel project can never deploy

Addendum to my comment above. lgs setup finished on fb42142, which builds the pinned spel at DEFAULT_SPEL.sha (73fc462e) into the cache:

/root/.cache/logos-scaffold/repos/spel/73fc462eb8f0a4d00f1a846437c627ec2e523f83/target/release/spel

That let me exercise spel init / spel generate-idl at exactly the pinned commit without a rebase. Both findings below are executed, not read.


1. check_spel_version misfires on the correct binary, 100% of the time

Pinned spel v0.5.0 has no version flag at all:

$ spel --version ; echo "exit=$?"
Usage: … spel [OPTIONS] -- <COMMAND> [ARGS]      # ← on stderr
exit=1
$ spel --version 2>/dev/null | wc -c
0                                                 # ← stdout is EMPTY

-V and version behave the same. So in

let output = match Command::new(spel_bin).arg("--version").output() { Ok(o) => o, Err(_) => return };
let stdout = String::from_utf8_lossy(&output.stdout);
if !stdout.contains(DEFAULT_SPEL.tag) { eprintln!("warning: installed spel version ({}) …", stdout.trim()) }

.output() is Ok (the process ran and exited 1 — the exit status is never inspected), output.stdout is "", and "".contains("v0.5.0") is false. Every user who has the exactly correct pinned spel gets:

warning: installed spel version () does not match the expected v0.5.0 pinned by scaffold.

— an empty version string, telling them to install the version they already have. This is not the "tag vs 0.5.0" nit Copilot raised on 2026-06-02; the flag does not exist, so the check cannot validate anything and is wrong on every input. It should be deleted rather than fixed — which point 2 of my comment above already argues for on other grounds.

2. build idl writes no file, and deploy then hard-fails on a file only build idl could have written

This is the blocker. spel init and scaffold disagree about where the IDL lives, and this PR wires them together anyway.

What spel init actually produces (run at the pinned commit):

$ spel init my-app --lez-tag v0.1.2
✅ Project 'my-app' created!

$ cat my-app/spel.toml
[program]
idl = "my-app-idl.json"        # ← a root-level FILE
$ ls my-app/idl
ls: cannot access 'my-app/idl': No such file or directory     # ← no idl/ DIRECTORY

What this PR configures for that same projectbuild_scaffold_config writes the default for spel exactly as for lez-framework:

idl: FrameworkIdlConfig { path: DEFAULT_FRAMEWORK_IDL_PATH.to_string() }   // "idl"

What build idl does for spelidl.rs delegates, and cmd_spel passes stdio straight through (Command::new(&spel_bin).args(args).status()). spel generate-idl prints the IDL to stdout and writes nothing; that is why spel's own Makefile needs a shell redirect (cargo run --bin generate_idl > $(IDL_FILE)):

$ spel generate-idl > /dev/null ; ls *.json idl
1564 bytes to stdout … no *.json, no idl/     # nothing written anywhere

So lgs build idl on a spel project dumps 1.5 KB of JSON into the user's terminal and produces no artifact.

Where that landsrun_state.rs, in the branch this PR extends to spel:

let idl_dir  = project.root.join(&project.config.framework.idl.path);   // <root>/idl
let idl_path = idl_dir.join(format!("{}.json", sanitize_file_stem(&stem)));  // <root>/idl/my_app.json} else if requires_idl {                       // ← this PR added FRAMEWORK_KIND_SPEL here
    anyhow::bail!("expected IDL file {} for program `{stem}` is missing; run `lgs build idl` first …")
}

<root>/idl/my_app.json never exists — spel would have called it my-app-idl.json at the root, and generate-idl doesn't write it regardless. The result is a closed loop:

lgs run / lgs deploy
  → "expected IDL file …/idl/my_app.json is missing; run `lgs build idl` first"
lgs build idl
  → prints JSON to the terminal, writes nothing
lgs run / lgs deploy
  → same error, forever

Before this PR, requires_idl was is_lez_framework, so the loud bail only covered projects whose IDL scaffold itself wrote into idl/. Extending it to spel — whose IDL scaffold never writes — turns a correct guard into an unsatisfiable one. A spel project cannot reach deploy. That is L4, and it is why I could not have validated this by reading alone.

The fix is a decision about who owns the IDL path, and it is not a one-liner either way:

  • have cmd_new_spel write framework.idl.path from the generated spel.toml (and teach run_state that spel's artefact is a file, not <dir>/<stem>.json), or
  • have idl.rs capture spel generate-idl stdout and write it to <idl_dir>/<sanitized_stem>.json itself, so scaffold's existing contract holds.

Either way it needs a test that runs new → build idl → deploy on a spel project, which is the coverage gap from point 5 above — it is exactly the test that would have caught this.


This does not change my verdict, it hardens it: comment only, not an approval. The direction is still right. But the delegation has not been exercised past lgs new, and points 1 and 2 here are both reproducible at the pinned commit today, independent of the rebase — so they are worth fixing before the rebase rather than discovering them after resolving 12 conflicts.

Reproduction environment: Nix 2.35.2, Rust 1.94.1, r0vm 3.0.5, risc0 guest rust r0.1.91.1 + cpp 2024.01.05, lgs setup green on fb42142 (sequencer + wallet + spel all built from the pinned revs).


Generated by Claude Code

@weboko

weboko commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

sorry, my bot went nuts on your PR

and I think this is not needed, no? @vpavlin ?

do you want me to pick it up?

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.

Upgrade spel and scaffold to pin LEZ v0.1.2

4 participants