feat(basecamp): opt into the inspector build via setup --inspector

[fryorcraken]

Closes #265.

What this does

bin-bundle-dir-inspector — the Basecamp output with the QML inspector compiled in — could not be selected through scaffold, leaving a sitometres UI end-to-end suite as the last thing reaching for raw nix build in an otherwise fully lgs-driven project.

The issue proposed appending the attr to BASECAMP_PORTABLE_ATTRS. That fixes the build selection but leaves the opt-in as a flake attr name the user has to know exists and spell correctly — for an output upstream deliberately marks as not a release artifact. This surfaces it as an intent instead:

$ lgs basecamp setup --inspector
inspector build selected (.#bin-bundle-dir-inspector) — test-only, not a release artifact
cloning basecamp at aa237766...

• --inspector selects the build and persists the choice, so later runs stay on it without repeating the flag.
• --no-inspector restores the default. It only undoes our attr — a hand-set bin-appimage survives, and scaffold says so rather than appearing to act.
• Neither flag leaves the configured attr alone, so a plain setup re-run never silently drops the opt-in.
• Both edit only the current host's entry. setup builds for the machine it runs on, so a Linux developer's toggle must not discard the project's aarch64-darwin mapping.

What this unblocks

The consumer is a sitometres UI end-to-end suite, which drives Basecamp headlessly through the QML inspector (via logos-qt-mcp). The inspector is a compile-time feature, deliberately off in every shipping output, so no release artefact can be driven this way.

Downstream context: fryorcraken/logos-radicle-module#4 converted both CI workflows to lgs, with its UI-test job pointing sitometres' --app-dir at .scaffold/basecamp/portable/ and running 18/18 steps green. The single raw nix build it could not convert is the inspector bundle.

The classification was the actual bug

Confirmed against the pinned flake (aa23776, tag 0.2.3): bin-bundle-dir-inspector is dirBundler appDistributedWithInspector, built from the same portable = true derivation as bin-bundle-dir and differing only by enableInspector. It is the portable stack by construction — the issue's reasoning holds.

Classified as dev it picked LogosBasecampDev for the XDG subpath and the cli lgpm variant, so Basecamp declined to load every module with one variant ... not supported on this platform line and opened to an empty UI.

Three adjacent instances of that same quiet failure, all reachable from the new flag and fixed here:

1. Stale [repos.lgpm].attr. Only defaulted when empty, so toggling the stack on an already-set-up project left cli against a portable build. The two are one choice and now move together; an attr scaffold doesn't manage is preserved but warned about.
2. Choice lost on a failed build. Persisted only after the long basecamp build, so an interrupt reverted it. An explicit flag now writes both attrs before the build.
3. --no-inspector was a silent no-op on a project with a per-platform attr map — it compared the scalar, not what effective_attr resolves. Now compares the effective attr, and reports when it declines.

Also included

From the issue's follow-up comment: --print-output on basecamp build / build-portable. Reuses the flag install already exposes rather than adding --print-build-logs as a second name for the same thing. As the follow-up notes, there was no log-carrying module build left in CI: a failing build reported the derivation failure without the compiler output explaining why.

Docs also record .scaffold/state/basecamp.state and its three keys. setup has always written it and nothing documented it — which left --inspector telling a harness author to build something while leaving them to derive its path, and that path is exactly what they must not derive (the correct entry point under the nix output varies by basecamp generation and stack).

That contract now lives in docs/basecamp-module-requirements.md — the doc basecamp docs prints — rather than only in docs/commands.md. Dogfooding this PR downstream showed why: a consumer looking for "where does setup leave the binary" goes to basecamp docs, finds the module contract, and reconstructs a cache path by hand when it isn't there. The section covers the three things a consumer cannot infer — read it rather than reconstruct it, it is a read-only nix out-link whose real ELF is a dot-prefixed sibling on portable builds, and it moves whenever the stack moves (so --inspector invalidates a cached path).

Breaking change for library embedders

BasecampCommand::Setup goes from a unit variant to a struct variant so it can carry the tri-state. BasecampCommand is a public enum on a published crate, so any embedder constructing it stops compiling. The CLI is unaffected, and nothing in tests/ exercises the API surface, so CI does not catch this.

// before
project.basecamp(BasecampCommand::Setup)?;

// after — `None` is the previous behaviour: leave the configured stack alone
project.basecamp(BasecampCommand::Setup { inspector: None })?;

Recorded in ADR.md and in the rustdoc on the variant itself. No version bump in this PR — this repo bumps in standalone chore: commits, so one is expected to follow separately. #[non_exhaustive] is deliberately not added: it would itself be a breaking change and is a separate decision.

Split out of this PR

The issue's other aside — setup preserving scaffold.toml comments — is a change to the config writer that every scaffold command goes through, so it is on its own branch (fix/preserve-scaffold-toml-comments) with its own PR. It is unrelated to selecting a basecamp build, and reviewing them together would let a problem in either block the other.

Testing

• 796 tests green: 596 lib + 192 CLI + 3 API + 5 doc, 0 failures.
• cargo fmt --check clean; cargo clippy --all-targets warning count unchanged from the base commit (62 pre-existing, 0 new — an earlier revision of this description said 64, which was wrong). None of the 62 is in a file this PR touches.
• Unit tests cover the attr selection round trip, per-host map editing (including that a sibling platform's entry survives both directions), the lgpm realignment both ways, and the inspector attr's stack classification — asserted via the XDG subpath and variant key, not just a boolean.
• Verified end to end locally: --inspector persists attr = "bin-bundle-dir-inspector" and lgpm.attr = "cli-portable" even when the build is interrupted.

The earlier LOGOS_SCAFFOLD_E2E_NIX=1 gate is gone (review feedback). ensure_nix_present is only a which("nix"), so a stub nix on PATH satisfies it and an unresolvable [repos.basecamp] source fails the clone in milliseconds — which is well inside the pre-build window the test is actually about. basecamp_setup_inspector_persists_both_attrs_before_the_build now runs unconditionally in CI in 0.01s with no network and no nix store, alongside a sibling covering the per-platform-map case.

Docs updated per CONTRIBUTING: docs/commands.md for the flags, docs/basecamp-module-requirements.md (what basecamp docs prints) for the .scaffold/state/basecamp.state contract, and DOGFOODING.md for B1 / B5 plus the rerun-guidance rows.

🤖 Generated with Claude Code

[fryorcraken]

Dogfooded this in the downstream consumer the PR description names, fryorcraken/logos-radicle-module#5. Both flags work as described, and the last raw nix build for a Basecamp is gone from that repo's CI.

Evidence

Locally: lgs basecamp setup --inspector in a fresh worktree, then browse.yaml 18/18 green against the resulting bundle.

In CI, all four sitometres specs (browse, branches, source, sync) pass against a real Basecamp built by setup --inspector, alongside the four fast-layer jobs. That run also exercises build-portable --print-output.

The classification is the part I checked hardest, since it is the half that would fail silently:

• lgs basecamp paths alice reports module_root under Logos/LogosBasecamp, not …Dev.
• [repos.lgpm].attr moved to cli-portable in the same run.

Confirming the lockstep matters: with lgpm left on cli, Basecamp declines every module with one warning line and opens to an empty UI. That is precisely the failure shape this repo has been bitten by before, and it is invisible until a spec times out on its first step with nothing useful in the runner's output.

Comment preservation also verified against a real case — this repo's scaffold.toml carries load-bearing notes about a sun_path overflow that made every module segfault. A setup --inspector run changed only the two attr lines and left the comments intact.

Three notes from the integration

None are blockers; the first is the only one I would consider acting on.

1. setup does more than a CI consumer needs. The e2e job wants the inspector bundle and nothing else, but setup also builds lgpm and seeds the alice/bob profiles. Cheap next to a Basecamp build, and I would rather have that than hand-roll a flake ref — but if a --no-profiles-shaped flag ever appears, this is a consumer for it. Not worth adding speculatively.

2. The pin is now load-bearing in a place that can drift. With setup doing the build, the rev comes from [repos.basecamp].pin — but this workflow's Nix store cache is keyed on its own BASECAMP_REV env var. Bump one and not the other and every run silently goes cold. That is the consumer's problem, not scaffold's, and I have added a step asserting the two are equal. Flagging it because any repo doing this conversion inherits the same trap.

3. basecamp_bin in .scaffold/state/basecamp.state is the useful output, and I did not find it documented. basecamp docs covers the module contract thoroughly but does not mention where setup leaves the binary or that the path is recorded there. A consumer that needs the binary — which is anything driving Basecamp headlessly — either reads that file or reconstructs ~/.cache/logos-scaffold/basecamp/<pin>/app-result/bin/… by hand. Worth a line in docs.

What still blocks full lgs coverage, and it is not scaffold

The bundle still cannot be handed to sitometres directly. basecamp_bin sits inside a nix out-link — read-only, with the real ELF at .LogosBasecamp.elf — and sitometres 0.1.0's hasInspector() probes [binPath, .<base>], never .<base>.elf. So a copy-and-symlink step still sits between lgs and the spec run.

I checked whether this PR removed the need for it and it does not: pointing --basecamp at scaffold's own binary produces the identical "no Basecamp with the QML inspector compiled in". Scaffold changed where the bundle comes from, not what is inside it, and there is nothing scaffold can do about it. Tracked upstream as paradoxcomputer/sitometres#1 — a one-line fix to that candidate list.

Worth stating plainly since the PR description points at this consumer: with #266, scaffold is no longer what stands between lgs and a fully-lgs e2e pipeline.

[weboko]

Reviewed at a806193. Verified on Linux (x86_64-linux) with Nix 2.35.2 and a real nix on PATH.

What I checked out as correct. The classification argument holds against the pinned flake. In logos-basecamp@aa23776, flake.nix:230 is binBundleDirInspector = withMainProgram (dirBundler appDistributedWithInspector), and appDistributedWithInspector (flake.nix:180-186) is the same ./nix/app.nix import as appDistributed with portable = true; enableInspector = true. nix/app.nix:201 shows portable is what drives -DLOGOS_PORTABLE_BUILD, and it is also what selects the unwrapped $out/bin/LogosBasecamp layout — so adding bin-bundle-dir-inspector to BASECAMP_PORTABLE_ATTRS (and with it Logos/LogosBasecamp rather than Logos/LogosBasecampDev, and cli-portable) is right, not a guess. nix eval .#bin-bundle-dir-inspector.drvPath resolves cleanly at that pin.

The flag itself behaves as documented end to end: setup --inspector writes attr = "bin-bundle-dir-inspector" and [repos.lgpm].attr = "cli-portable" before the build starts, a later --no-inspector moves both back to app/cli, --no-inspector on a project that never opted in declines with a note instead of silently doing nothing, and --inspector --no-inspector resolves last-flag-wins through clap's overrides_with. 797 tests pass (600 lib / 189 CLI / 3 API / 5 doc), cargo fmt --check is clean, and cargo clippy --all-targets reports zero warnings in any file this PR touches.

The findings below are all in the save_project_config half of the change.

---

1. save_project_config now panics on a scaffold.toml that parse_config accepts (blocking)

Switching the writer from "render into an empty document" to "merge over the parsed original" means write_config_into now inherits whatever shape the user's file has, and the .expect("… table") calls in it stop being structurally unreachable. Previously serialize_config always started from DocumentMut::new(), so every doc.entry(k).or_insert(Item::Table(..)) genuinely inserted a Item::Table and the expects could not fire. They can now.

The trigger is a root-level inline table for any section whose parser tolerates a non-table value by falling back to defaults. parse_config reads these through Item::as_table, which returns None for an inline table, so the file parses fine — and then the writer reaches for as_table_mut() on the inline value and aborts.

Repro (x86_64-linux, PR head):

wallet = { home_dir = ".scaffold/wallet" }

[scaffold]
version = "0.2.0"
# ... otherwise minimal, valid v0.2.0 config

$ lgs basecamp setup --inspector
inspector build selected (.#bin-bundle-dir-inspector) — test-only, not a release artifact

thread 'main' panicked at src/config.rs:807:27:
wallet table

Confirmed for four sections, each aborting at its own expect:

inline key at document root	panic site
wallet = { … }	src/config.rs:807 expect("wallet table")
framework = { … }	src/config.rs:815 expect("framework table")
localnet = { … }	src/config.rs:827 expect("localnet table")
circuits = { … }	src/config.rs:839 expect("circuits table")

src/config.rs:821 (expect("idl table")) is the same shape via [framework] + idl = { … }, and ensure_subtable's expect("parent is a table") becomes reachable the same way for modules = { … } once the config actually carries a module (basecamp modules rather than setup).

Two things make this worth fixing before merge rather than filing as a follow-up. It is an abort with a backtrace, not a scaffold-level error — exactly the "raw stack trace with no scaffold-side hint" that DOGFOODING.md B1 calls a UX regression. And it lands on hand-edited files specifically, which is the population this change exists to be kind to; the file that gets a comment written into it is the file most likely to have been hand-shaped in other ways too.

The unparseable-input fallback doesn't cover it, since the file parses. Either coerce a table-like item into a Table before writing, or treat "modelled key exists but isn't a Table" as the same case as unparseable input and fall back to a fresh render — but please make it a returned error or an explicit fallback rather than an expect, since these are now reachable from file content.

2. [basecamp.profiles] is the one conditional emitter the sweep missed (src/config.rs:932)

env and env_append both got the else { basecamp_table.remove(...) } branch; profiles did not. Same defect the PR describes: clearing the profiles while [basecamp] still exists leaves the old tables stranded in the file.

Confirmed with a test in config::tests:

let original = minimal_v0_2_0()
    + "\n[basecamp]\nport_base = 41000\n\n[basecamp.profiles.alice]\nruntime_dir = \"/run/user/1000\"\n";
let mut cfg = parse_config(&original).unwrap();
cfg.basecamp.as_mut().unwrap().profiles.clear();
let updated = update_config(&original, &cfg).unwrap();
// fails: `[basecamp.profiles.alice]` is still in `updated`;
// serialize_config(&cfg) emits only `[basecamp]\nport_base = 41000`.

update_config_clears_keys_reset_to_defaults doesn't catch it because it sets cfg.basecamp = None, which short-circuits to doc.remove("basecamp") at line 979 and never exercises "section kept, profiles emptied". Worth extending that test with the mixed case — it is the vacuous-coverage shape the PR write-up already identified once.

While in there: write_repo_ref is only called under if let Some(repo) = &cfg.basecamp_repo / cfg.lgpm_repo with no else removing a stale [repos.basecamp] / [repos.lgpm]. Nothing sets either back to None today, so it is latent rather than a live bug, but it is the last gap in the "every conditional emitter has a removal branch" invariant the PR states.

3. An unparseable or unreadable scaffold.toml is silently overwritten

update_config does existing.parse().unwrap_or_default(), and save_project_config does fs::read_to_string(&path).unwrap_or_default(). The fallback policy is defensible, but it is silent and there is no backup, so a file that was one stray bracket away from valid is replaced by a from-scratch render — every comment and unmodelled section gone, no message. That is the exact loss the rest of this PR is built to prevent, and the user gets no signal it happened.

unwrap_or_default() on the read also collapses "no file yet" and "file exists but I couldn't read it" (permissions, I/O error) into the same path; the second case silently truncates a file that was there.

A one-line eprintln! naming the fallback would be enough for the parse case. For the read, distinguishing NotFound from other errors and propagating the rest would be better. init's migration already writes a scaffold.toml.bak; matching that here would be the belt-and-braces option.

4. DOGFOODING.md isn't updated, and CONTRIBUTING requires it

CONTRIBUTING.md:44 — "If the change affects user-facing behavior, update README.md and DOGFOODING.md in the same PR." docs/commands.md is updated (thank you — the --inspector paragraph is genuinely good), but nothing in the runbook is.

Concretely missing:

• B1 drives basecamp setup with no flags. Nothing exercises --inspector / --no-inspector, the [repos.lgpm].attr realignment, or the persist-before-build ordering — and B1's "Expected Success Signals" already enumerate per-platform attr behavior, so the inspector attr belongs alongside it.
• B5 covers basecamp build / build-portable without mentioning --print-output.
• Nothing anywhere asserts the new comment-preservation contract. This one matters most: it silently changes the behavior of every command that rewrites scaffold.toml, not just the ones this PR touches, and the failure mode it introduced mid-review (keys reset to defaults being stranded) is invisible unless a scenario looks for it. B1 already says to capture a scaffold.toml excerpt as evidence — a check that a comment placed before setup is still there afterward, and that a value reset to its default is actually gone, would be cheap and would have caught findings 1–3.

The "Minimum Rerun Guidance" table should probably also gain a row for changes to the config writer, since the blast radius is every config-writing command rather than just basecamp.

5. Minor: basecamp_setup_inspector_persists_both_attrs_before_the_build does real network + Nix work in cargo test

The doc comment frames this as local-only coverage that no-ops in CI, and that's accurate for CI. But on any developer machine that does have Nix — the machines most likely to be working on this code — a plain cargo test now shells out to basecamp setup --inspector, which clones the pinned basecamp repo into the shared cache root and starts a Nix build of a Qt-heavy closure. It's the only test in tests/cli.rs that reaches the network or the Nix store, and it uses the real default cache root rather than a temp one.

The assertion is worth keeping. Gating it behind an explicit opt-in env var, and pointing LOGOS_SCAFFOLD_CACHE_ROOT at the test's own tempdir, would make it safe to leave in the default run.

---

Findings 1 and 2 are code defects with reproductions; 3 and 4 are behavior/documentation gaps that follow from the same change. I'd hold approval on 1 in particular — an abort on a scaffold.toml that parses is a worse outcome than the comment loss the change fixes.

I could not complete a full basecamp setup here: the container's proxy answers 403 for Nix's github: flake inputs, and a direct-egress run gets past evaluation but then builds the Qt closure from source, which is the environment limitation DOGFOODING.md B1 documents as the usual B-series blocker. Everything above the Nix build — attr selection, lgpm realignment, persist ordering, config round-tripping — was verified against the real binary.

---

Generated by Claude Code

[weboko]

Follow-up to my review above: the Nix build did finish after all, so the caveat I ended on ("could not complete a full basecamp setup") no longer applies. Bypassing the proxy for GitHub specifically got Nix past the 403 on github: flake inputs, and the closure then built from source.

basecamp setup --inspector completed green on x86_64-linux:

inspector build selected (.#bin-bundle-dir-inspector) — test-only, not a release artifact
  ✓ building basecamp (.#bin-bundle-dir-inspector) (24m24s)
  ✓ building lgpm (github:…/logos-package-manager/202af6fa#cli-portable) (16.2s)
seeded profiles: alice, bob
setup complete

Everything the PR claims about the artefact holds against the real output:

• The inspector is genuinely compiled in. $out/bin/.LogosBasecamp.elf carries InspectorServer, QML_INSPECTOR_PORT, and [QmlInspector] Inspector server listening on port, and logos-qt-mcp (1fea5094) is in the recorded build inputs. nix/app.nix:202 confirms the path: -DENABLE_QML_INSPECTOR=ON plus -DLOGOS_QT_MCP_ROOT. This is the build a sitometres harness needs.
• The portable classification is right in practice, not just in the flake source. Profiles seeded under xdg-{data,cache,config}/Logos/LogosBasecamp — not LogosBasecampDev — and $out/bin/ has the unwrapped portable layout. lgpm built at cli-portable, matching.
• basecamp doctor → 5 PASS, 0 WARN, 0 FAIL, "Ready".
• The persistence claim holds where it matters most: a subsequent plain basecamp setup with no flag stayed on bin-bundle-dir-inspector / cli-portable and completed in 20s off cache with no rebuild. That's the behavior the tri-state was designed for, working.
• Both comments I planted in scaffold.toml survived the write, with [repos.basecamp] / [repos.lgpm] inserted around them.

So the feature does what it says end to end. That doesn't change findings 1–5 — the panic in particular is on a different input shape than the one exercised here — but the headline functionality is verified rather than assumed, and I wanted the record to say so.

---

Generated by Claude Code

[fryorcraken]

@weboko — thank you for this. The verification half in particular (checking flake.nix:230 and nix/app.nix:201 rather than taking the classification argument on trust, then coming back with the completed 24-minute build) is the review that actually tests the claim, and the follow-up confirming $out/bin/.LogosBasecamp.elf carries InspectorServer is the evidence this PR needed and did not have.

One structural note before the findings: you reviewed at a806193, when this branch still carried the scaffold.toml comment-preservation work. That has since been split to #267 (fix/preserve-scaffold-toml-comments). git diff master...HEAD -- src/config.rs is now empty here, so findings 1–3 — all of which you correctly scoped to "the save_project_config half of the change" — describe code that lives in the other PR. I have not implemented them here; I have verified their status against #267's current head instead. Findings 4 and 5 are this PR's and are fixed.

---

1. save_project_config panics on an inline table — correct, and still open in #267

I want to be unambiguous about this one, because it was previously disputed. You are right. #267's earlier analysis claimed or_insert appends a separate [wallet] table when the document already holds an inline wallet = { … }, making the output a cosmetic duplicate stanza rather than a panic. That is wrong. I checked it against the crate's own toml_edit 0.22 rather than reasoning about it:

input:  wallet = { home_dir = ".scaffold/wallet" }
        [scaffold]
        version = "0.2.0"

doc.get("wallet").as_table()               -> None      (parse falls back to default, file parses)
doc.entry("wallet").or_insert(Item::Table) -> the existing inline item
  .as_table_mut()                          -> None      <- `.expect("wallet table")` aborts here

Entry::or_insert returns the occupied value; it does not append alongside it. So the abort is real, and your mechanism — Item::as_table returning None for an inline table on the read side is exactly what lets the file through the parser to reach the writer — is precisely right.

Status in #267 at 7f32892: unfixed, verbatim. All five expects you tabulated are unchanged (config.rs:870, 878, 884, 890, 902), ensure_subtable's expect("parent is a table") is unchanged at 1472, parse_config still reads wallet through Item::as_table, and there is no inline-table coercion anywhere in the file. This should block #267, not this PR.

It is not reachable on this branch: serialize_config here still starts from DocumentMut::new(), so as you say the expects remain structurally unreachable until the merge-over-original writer lands. Your framing was right — this is a defect the comment-preservation change introduces.

2. [basecamp.profiles] removal branch + write_repo_ref gaps — fixed in #267

Both halves. #267 added a bc.profiles.is_empty() branch that walks an existing [basecamp.profiles] table, clears the keys scaffold owns, drops entries that end up empty, and removes the parent when nothing survives — with a update_config_drops_a_cleared_basecamp_profile test for exactly the "section kept, profiles emptied" case you noted update_config_clears_keys_reset_to_defaults short-circuits past. The write_repo_ref gap is closed too: basecamp and lgpm now go through a match repo { Some(..) => write, None => repos.remove(name) }, with update_config_drops_a_dropped_lgpm_repo pinning it. The comment there ("no caller sets these to None today, but [basecamp] had no such caller either right up until it did") is your latent-vs-live point taken.

Worth saying that #267 went further than the finding on the same theme, and for a reason worth flagging: several removals cannot be driven by the model at all, because the corresponding parsers are not total. A fully-default profile is dropped at parse, and an empty env_append list is skipped — so "absent from the model" does not mean "absent from the file", and a blanket remove there would delete user data rather than merely stale it. The removal branches are written as clear-what-scaffold-owns-and-keep-the-rest for that reason.

3. Silent overwrite of an unparseable / unreadable scaffold.toml — half fixed in #267

• Unreadable (your second paragraph, the one that silently truncates a file that is on disk): fixed. save_project_config now matches on ErrorKind::NotFound for the legitimate from-scratch case and propagates every other read error with context, explicitly refusing to render fresh over a file whose comments it could not see.
• Unparseable: still open. update_config is still existing.parse().unwrap_or_default() with no diagnostic. Your minimum ask — a one-line eprintln! naming the fallback — is not there, and neither is the scaffold.toml.bak that init's migration already writes. I would suggest raising this on fix(config): preserve scaffold.toml comments on every rewrite #267 directly; the argument that the user gets no signal that the thing this PR exists to prevent just happened to them stands.

4. DOGFOODING.md — fixed here

B1 gains the inspector round trip: --inspector, a no-flag re-run that must stay on it, the portable module_root check (LogosBasecamp, not …Dev), --no-inspector, and the rejected flag pair — each with a grep beside it as the evidence, since the attrs are written before the build and so are checkable even where the build is out of reach. B5 gains --print-output on both structs, with a note to verify against a failing build. The Minimum Rerun Guidance gains a row for the stack selector.

Your third bullet — that nothing asserts the comment-preservation contract — I have deliberately not actioned here, because the contract it names does not exist on this branch any more. It belongs on #267, and it is the sharpest thing in your review: a runbook check that a planted comment survives a setup and that a value reset to its default is actually gone would have caught 1, 2 and 3 between them. I would rather see it land there than see a scenario here describing behavior this PR does not have.

5. Nix/network work in a plain cargo test — fixed here

Gated behind LOGOS_SCAFFOLD_E2E_NIX=1, and when it does run LOGOS_SCAFFOLD_CACHE_ROOT points at the test's own tempdir so the clone never reaches the developer's shared ~/.cache/logos-scaffold. The doc comment says why on both counts. You were right that "no-ops in CI" was the wrong bar — the machines that would have paid for this are the ones working on this code.

---

Also in this push

@fryorcraken's dogfooding found that the .scaffold/state/basecamp.state documentation was in the wrong file: docs/commands.md had it, but a consumer asking "where does setup leave the binary" goes to basecamp docs, which prints docs/basecamp-module-requirements.md — where it was absent. The contract now lives there, covering the three things that cannot be inferred from a path: read basecamp_bin rather than reconstructing it, it is a read-only out-link whose real ELF is a dot-prefixed sibling on portable builds, and it moves whenever the stack does (so --inspector invalidates a cached path). B1 reads the file before and after --inspector so that last claim is evidenced.

Also corrected the PR description's clippy count: 62, not 64. --all-targets reports 58 lib plus 62 lib-test of which 58 are duplicates. All pre-existing; none in a file this PR touches, which matches what you measured.

Findings 1 and 3-unparseable are the open ones, both on #267.

---

Generated by Claude Code

[danisharora099]

Reviewed at f207de3.

The in-memory logic in apply_inspector_selection and align_lgpm_attr is careful and the unit tests around it are good. The problems are at the seams: where that logic meets the config writer, the build path, and the docs. Two of them undercut the PR's own goal, so I am holding on those.

Blocking

1. --inspector is silently not persisted when the project has a per-platform attr map without an entry for this host. The config writer drops the scalar whenever a map exists. Inline comment on basecamp.rs:354.
2. --print-output on build / build-portable does nothing. The module build path never reads the flag. Inline comment on basecamp.rs:141.

Should fix here, all about the same state file

3. Docs and runbook say basecamp_bin moves when the stack moves. It does not; the out-link is keyed by pin only. Inline on DOGFOODING.md:1073 and basecamp-module-requirements.md:39.
4. The --inspector then --no-inspector round trip replaces a hand-set portable attr with the default. Inline on basecamp.rs:376.
5. Persisting before the build can leave config and basecamp.state on different stacks, and doctor cannot tell. Inline on basecamp.rs:248.

Minor

• docs/basecamp-module-requirements.md:249: the LOGOS_DATA_DIR row lists three portable attrs. The inspector attr now qualifies too.
• BasecampCommand::Setup changed from a unit variant to a struct variant on the public API with no version bump. Inline on api/mod.rs:648.
• The E2E test could run in CI with a stub nix on PATH. Inline on tests/cli.rs:3405.
• The last line of the new DOGFOODING block has no || true. Inline on DOGFOODING.md:1079.

The existing verification on this thread ran on a project with a plain scalar attr, so it does not exercise point 1, and nobody checked that --print-output actually streamed anything. Happy to re-review once 1 and 2 are in.

[danisharora099]

Blocking: this write is lost whenever the project already has a per-platform map.

write_repo_ref (src/config.rs:1053) emits only the inline map when attr_platform is non-empty and drops the scalar. So on a project like:

[repos.basecamp]
attr = { aarch64-darwin = "bin-macos-app" }

a Linux developer running setup --inspector gets lgpm.attr = "cli-portable" on disk but never bin-bundle-dir-inspector. The next plain setup defaults the scalar back to app and builds a dev basecamp against portable lgpm, which is the exact empty-UI mismatch this flag exists to prevent. A later --no-inspector reports "nothing to undo".

The unit tests only cover a map that already contains the current host, so this path is untested.

Suggested fix: when the map is non-empty, always write this host's key into the map and never touch the scalar. That also turns the --no-inspector branch below into a plain remove(system) with no scalar-repair heuristic.

[danisharora099]

Blocking: --print-output is a no-op on this path.

The only nix spawn reachable from cmd_basecamp_build is run_build_portable_nix (around line 2087), which calls cmd.output() and never consults print_output_enabled(). Output is captured and discarded on success, and only trimmed stderr surfaces on failure, exactly as before the flag existed. The help text is also wrong on the other half: build never writes a .scaffold/logs file.

basecamp_build_accepts_print_output_flag only checks --help, so nothing catches this.

Either route that spawn through run_logged the way setup and install do, or drop the flag from this PR.

[danisharora099]

This assertion is false on Linux, and a dogfooder following it will file a bug that is not one.

build_basecamp_app always writes the out-link to <cache>/basecamp/<pin>/app-result regardless of attr, and both the 0.2.x dev launcher and the portable bundle resolve to bin/LogosBasecamp. So basecamp_bin is byte-identical before and after --inspector.

The real hazard is the inverse of what the docs describe: the shared symlink gets re-pointed, so a consumer holding the old path silently execs the new build, and the displaced stack loses its GC root so the next toggle back rebuilds. Keying the out-link by attr (app-result-<attr>) would make this line true and let both stacks coexist.

[danisharora099]

Same as the DOGFOODING comment: the path does not change between stacks today, because the out-link is keyed by pin only and both stacks resolve to bin/LogosBasecamp. Either key the out-link by attr so this becomes true, or reword this bullet to say the symlink is re-pointed in place.

[danisharora099]

The round trip loses a hand-set portable attr, silently.

Start from attr = "bin-appimage", lgpm.attr = "cli-portable". --inspector overwrites the scalar. --no-inspector then lands here, sets app, and align_lgpm_attr drags lgpm to cli. The user is now on the dev stack with no note, because the "left alone" note above only fires when the effective attr was not the inspector. A per-host map entry gets the same treatment: deleted rather than restored. inspector_flags_touch_only_the_current_host_entry asserts this outcome as correct, and docs/commands.md says hand-set values survive.

Restoring the previous value would mean remembering it somewhere, so I am not asking for that here. But the docs should not claim the opposite, and a note when --no-inspector lands on a different stack than the user started from would help.

[danisharora099]

The comment above says nothing downstream depends on the build having finished. That is not quite right: launch, paths, and doctor derive the XDG subpath and variant key from config, but take the binary from basecamp.state, which does not record the attr.

After an interrupted setup --inspector, config says portable while the binary on disk is still the dev launcher. launch seeds portable profiles and execs the dev app, and doctor reports PASS because the path exists.

Recording the built attr in basecamp.state and having doctor compare it to effective_attr would close this, and also answers the "which build is this?" question from the basecamp_bin comments.

Smaller: on a toggle this early save plus the unconditional save at the end write scaffold.toml twice with identical bytes. Guarding the trailing save with !stack_changed would keep it to one.

[danisharora099]

Two corrections in this paragraph:

• lgs doctor does not print the binary path. Only lgs basecamp doctor reads basecamp.state.
• The --inspector bullet says the flag persists "overriding a per-platform attr map, since the inspector output is built for every supported system", and a few sentences later says both flags "edit only the current host's entry". The code does the second. The first reads as if the whole map is replaced.

[danisharora099]

This changes BasecampCommand::Setup from a unit variant to a struct variant on the public API, so any embedder writing BasecampCommand::Setup stops compiling. Fine if intended, but it deserves a version bump or at least a note, since nothing in tests/ exercises the API surface and CI will not notice.

[danisharora099]

This could run unconditionally in CI. ensure_nix_present is only a which("nix"), and the file already stubs binaries on PATH via write_wallet_stub. A stub nix plus a [repos.basecamp] source that fails to clone immediately would prove the write ordering in milliseconds, and the two contains assertions stay as they are. Then the opt-in gate and the nix caveat in the PR description both go away.

[danisharora099]

This line is meant to exit non-zero, but unlike the other expected-failure lines in this scenario it has no || true. Under a set -e capture script the block aborts here. Same for the setup --inspector line above on a host where the clone fails: the comment says to run the greps regardless, but the greps come after a command that may exit non-zero.

[fryorcraken]

Thanks — this was a genuinely good catch list. Both blocking findings were real, and one of them meant the feature silently did nothing on a common project shape. Pushed as 6b71700.

Blocking 1 — the attr write is lost when a per-platform map exists — fixed

Confirmed exactly as described. write_repo_ref (config.rs:1053) emits the inline map instead of the scalar whenever attr_platform is non-empty, so on a project whose map lists only a colleague's platform, --inspector persisted nothing while still moving [repos.lgpm].attr to cli-portable. The next plain setup defaulted the scalar back to app and built a dev basecamp against portable lgpm — the exact mismatch the flag exists to prevent.

Took your suggested fix. The decision now keys on whether the project uses a map at all, not on whether that map happens to mention this host:

• --inspector: map non-empty → write this host's key; otherwise → write the scalar.
• --no-inspector: the mirror. With a map it is a plain remove(system), and the scalar-repair heuristic is gone, as you predicted.

One wrinkle worth flagging: --no-inspector on a map can uncover an empty scalar, which would build a bare nix build .#. setup defaults the scalar before calling in, so no real run reaches it, but the undo now lands on a buildable attr on its own rather than relying on its caller. Your review actually surfaced this — an existing unit test constructed that shape and started failing.

Tests: inspector_flag_writes_into_a_map_that_lacks_this_host covers both directions at the unit level, and basecamp_setup_inspector_persists_into_a_platform_map_lacking_this_host covers it end to end through the binary, since the defect lives in the seam between the selector and the serializer where neither side's unit tests can see it. Verified it fails against the previous code with the reported symptom.

src/config.rs is untouched — the fix sits entirely at the call site, so there is no overlap with #267.

Blocking 2 — --print-output is a no-op on build / build-portable — fixed

Confirmed. run_build_portable_nix called cmd.output() and never consulted print_output_enabled().

Chose to fix rather than drop the flag, but not by routing through run_logged — that redirects both streams, and this function parses store paths from --print-out-paths on stdout, so handing it the whole spawn would leave store_paths empty and fail every build with "returned no output paths". Only stderr is forwarded, which is where nix writes its build log. That gets the flag's actual purpose (a failed CI build explaining itself) without breaking the command.

Help text corrected on both verbs — you were right that build never writes a .scaffold/logs file.

Test: basecamp_build_print_output_actually_streams_nix_output asserts both directions. It has to distinguish the marker arriving on its own line (streamed) from the same marker quoted inside scaffold's nix build … failed (…) summary, which echoes captured stderr by design — a naive contains would pass with the flag deleted. Verified that stubbing the implementation out fails it.

The docs regression I introduced — fixed, and you were right

Plainly: I asserted basecamp_bin moves between stacks without checking, and it did not. build_basecamp_app wrote <cache>/basecamp/<pin>/app-result regardless of attr, and both the 0.2.x dev launcher and the portable bundle resolve to bin/LogosBasecamp, so the path was byte-identical before and after --inspector. A dogfooder following that line would have filed a bug that wasn't one.

Went with keying the out-link by attr (app-result-<attr>) rather than only rewording. Blast radius is one source line and one docs path example — nothing else in src/, tests/, or the docs referenced app-result. It makes the claim true and is the better behaviour regardless, for the reason you gave: a shared link is re-pointed by every toggle, so a consumer holding the old path silently execs the new build and the displaced stack loses its GC root, forcing a rebuild on the way back. Both stacks now coexist under one pin. Both docs updated to describe this accurately.

basecamp.rs:376 — the lossy --no-inspector round trip — partially actioned

Verified. Taking you at your word that restoring the previous value is out of scope, so:

• docs/commands.md no longer claims hand-set values survive. It now says --no-inspector restores the default stack, not what was configured beforehand, and that scaffold does not remember a displaced value.
• --no-inspector now prints a note whenever it lands on something other than the default, so the move is not silent.
• inspector_flags_touch_only_the_current_host_entry reassessed. It still asserts the same outcome, because that is what the code does — but the comment now states plainly that the round trip is lossy by construction and that this is a known limitation rather than the desired end state, instead of implying the outcome is correct.

basecamp.rs:248 — attr not recorded in basecamp.state — deferred, with the smaller half actioned

The scenario is real: after an interrupted setup --inspector, config says portable while the binary on disk is the dev launcher, and doctor passes because the path exists. My "nothing downstream depends on the build having finished" comment was too broad.

Deferring the basecamp.state attr field deliberately. It changes a documented consumer contract that this PR just finished writing down, and doctor gaining a config-vs-built comparison is a new check with its own semantics. Both belong in a follow-up that can treat the state-file schema as its subject rather than smuggling it in here. Worth noting the attr-keyed out-link above narrows the window considerably: the recorded basecamp_bin now contains the attr it was built at, so the two stacks no longer share a path, and a stale record points at the older build rather than being silently overwritten by the newer one.

The smaller point is fixed: the trailing save_project_config is now guarded with !stack_changed, so a toggle writes scaffold.toml once.

docs/commands.md:180 — two factual corrections — both fixed

Both confirmed. lgs doctor does not read basecamp.state; only lgs basecamp doctor does (it prints found <path>), so the text now says that. And the "overriding a per-platform attr map" phrasing contradicted the later sentence — the code edits only the current host's entry. Rewritten to describe where the write actually lands and why (the map is what gets serialized), which also documents the Blocking 1 fix.

src/api/mod.rs:648 — breaking public-API change — maintainer call, not actioned

Confirmed breaking: BasecampCommand is a public enum without #[non_exhaustive], so BasecampCommand::Setup stops compiling for embedders, and nothing in tests/ exercises the API surface.

Not bumping the version here, because I don't think it's mine to decide. CONTRIBUTING.md prescribes no versioning policy, there's no CHANGELOG, and the history shows version bumps landing as standalone chore: commits rather than inside feature PRs (fbde925, 6b5f125). The crate is published, so this is a real semver event on a 0.x line. Flagging for a maintainer: either bump to 0.4.0 before release, or add #[non_exhaustive] to the enum so future variants stop being breaking. Happy to do either in this PR if you say which.

tests/cli.rs:3405 — run it unconditionally in CI — worked, gate removed

Your suggestion was right and it was straightforward. ensure_nix_present is only a which("nix"), so a stub nix on PATH satisfies it, and an unresolvable [repos.basecamp] source fails the clone immediately — well inside the pre-build window the test is actually about. Both contains assertions unchanged.

The LOGOS_SCAFFOLD_E2E_NIX=1 gate is gone, the test runs in CI in 0.01s with no network and no nix store, and I've added the map-variant sibling on the same harness. I'll drop the nix caveat from the PR description.

DOGFOODING.md:1079 — missing || true — fixed

Confirmed. Added || true to the --inspector --no-inspector line and to the three setup invocations above it, so a set -e capture script reaches the greps that are the actual evidence — which was the stated intent of the block.

---

cargo fmt --check, cargo test (796 passing), and cargo clippy --all-targets all clean locally; clippy is still at the 62-warning baseline with none added.

[fryorcraken]

@danisharora099 — resolution on the public-API point you raised, pushed as 27ffa8b.

The breaking change is now documented rather than implicit, in three places:

• ADR.md — a new entry with the one-line migration. That is where this repo already records consumer-facing surfaces and their migrations ("Hook Env Contract is a Documented Public Surface", the 0.2.0 schema-migration entries), and CONTRIBUTING.md says to add to it rather than rewrite it. There is no CHANGELOG and no dedicated API doc, so this follows the existing convention instead of introducing a new file.
• Rustdoc on the variant itself, so it shows up in the generated docs without anyone having to find this PR.
• The PR description.

The migration, for the record:

// before
project.basecamp(BasecampCommand::Setup)?;

// after — `None` is the previous behaviour
project.basecamp(BasecampCommand::Setup { inspector: None })?;

No version bump in this PR, deliberately. The repo's convention is standalone chore: commits for bumps (fbde925, 6b5f125) rather than folding them into feature PRs, so one is expected to follow separately before release. Flagging that as the remaining action rather than something silently dropped.

#[non_exhaustive] also deliberately not added — it would itself be a breaking change, and it is a separate design decision that shouldn't ride along here.

Docs-only commit; CI green on all four checks.

[danisharora099]

Re-reviewed at 27ffa8b. Both blockers are fixed and covered by tests that fail against the previous code, and the out-link change makes the docs true rather than just rewording them. Thanks for taking the E2E test suggestion further than I asked, running it in CI unconditionally is the right outcome.

Verified locally: cargo fmt --check clean, lib tests green, the three new CLI tests pass in about a second. One CLI test failed on my first full run (deploy_program_path_json_includes_program_id, a ureq panic) but passes on rerun and on master, so that is a pre-existing flake unrelated to this PR.

Two non-blocking notes inline. Deferring the basecamp.state attr field to a follow-up is fine by me. The version bump is a maintainer call, and the ADR entry gives them what they need to make it.

[danisharora099]

Non-blocking. This condition fires in the opposite case from the one it was added for. The lossy round trip lands on the default after displacing a hand-set value, and there restored == BASECAMP_ATTR, so the note stays silent. In the scalar branch above attr is always set to the default, so the note never fires there. In the map branch it fires only when the scalar is non-default, which cannot be serialized alongside a map. In practice it is close to unreachable.

Printing the note unconditionally on a successful undo would be simpler and would actually cover the case it describes. Fine as a follow-up.

[danisharora099]

Non-blocking. lgpm-result in build_lgpm just below is still one shared link across cli and cli-portable, so it has the same re-pointing behaviour this change fixes for basecamp: a toggle replaces the other stack's lgpm and drops its GC root. Much smaller cost since lgpm builds in seconds, but keying it the same way (lgpm-result-<attr>) would keep the two consistent and make lgpm_bin in basecamp.state distinct per stack too. One-liner for a follow-up.

[fryorcraken]

One finding from @danisharora099's review body was missed in my earlier responses — flagging it rather than leaving it silently dropped.

docs/basecamp-module-requirements.md:249 — the LOGOS_DATA_DIR row of the env table still listed the portable [repos.basecamp].attr values as bin-macos-app, bin-appimage, bin-bundle-dir. This branch added a fourth entry to BASECAMP_PORTABLE_ATTRS (BASECAMP_ATTR_INSPECTOR = bin-bundle-dir-inspector, src/constants.rs:212-217), and the LOGOS_DATA_DIR export is gated on is_portable_basecamp, which reads that constant — so on macOS after setup --inspector the variable is exported, contrary to the table. The prose at line 37 had already been updated, leaving the file inconsistent with itself, and since basecamp docs prints an embedded copy the stale list shipped offline too.

Fixed in 231889c (docs-only, one line; the code was already correct).

[fryorcraken]

Further dogfooding on this, and I've hit something the review discussion hasn't covered: --inspector breaks basecamp install and basecamp launch for as long as it is set. Not subtly — it is a hard error.

I don't think this is a bug in the implementation. I think it's the flag's shape.

What happened

Working in a module repo, I ran setup --inspector to get a bundle for a sitometres spec. Later in the same session I wanted to look at the module in a normal Basecamp window:

$ lgs basecamp install
...
error: the `.lgx` has no variant for this stack — a dev basecamp loads
`<host>-dev` variants and a portable one loads bare `<host>` variants.
Rebuild with the matching flake output (`#lgx` for dev, `#lgx-portable`
for portable) or align `[repos.basecamp].attr`.

Correct behaviour, given the state. --inspector had moved the profiles to the portable stack, so the dev #lgx artefacts no longer match. The fix is setup --no-inspector, then install, then launch — and to go back to running specs, setup --inspector again.

So the two things a module developer does most often — run the app, run the e2e specs — are now mutually exclusive modes, and switching between them means re-running setup each time. setup also reseeds the alice/bob profiles on the way through, which is its own small cost.

Why I think it's the shape, not the implementation

The PR's strongest argument is that [repos.basecamp].attr and [repos.lgpm].attr must move together, because a half-switched stack fails silently — Basecamp declines every module with one warning line and opens empty. That's right, and the lockstep is a genuine improvement.

But it's also the tell. If selecting this build has to drag lgpm's attr with it, and changes the XDG subpath the profiles are seeded under, and invalidates the recorded basecamp_bin, then --inspector isn't selecting a build. It's switching the whole stack into a different mode, and every one of those consequences follows from that.

A flag on setup reads like "same command, one option different". This is closer to "a different setup". The current design has already needed:

• lgpm's attr moved in lockstep (this PR)
• the out-link keyed by attr (app-result-<attr>) so the two stacks stop sharing a path (from the review)
• a persisted tri-state so a later plain setup doesn't silently drop the choice
• docs warning that --inspector invalidates a cached basecamp_bin

Each fix is right, and each one is scaffold absorbing another consequence of two modes wearing one command's clothes.

What I'd suggest

@fryorcraken's framing, and I agree with it: make it a separate verb rather than a flag —

$ lgs basecamp setup             # the dev stack: install, launch, doctor
$ lgs basecamp setup-inspector   # the portable+inspector stack: specs

That would change the shape of the problem in three ways:

1. The mode is visible at the call site. A CI workflow or a README line says which stack it wants. Today setup means "whatever this project was last toggled to", which is why the tri-state had to exist at all.
2. The persistence question mostly dissolves. Much of the review — the platform-map write, the lossy --no-inspector round trip, an interrupted setup leaving config and disk disagreeing — is about correctly remembering a mode across invocations. A verb that states its mode each time has far less to remember.
3. install / launch / doctor can say something useful. Rather than the variant-mismatch error above, they could say "this project is set up for the inspector stack; run lgs basecamp setup for a dev stack" — which is the sentence I actually needed.

If the two stacks could coexist on disk (the attr-keyed out-link from this PR is most of the way there), the verbs could even be idempotent enough to alternate without a full re-setup. That's a bigger change and I'm not asking for it here.

Not asking to block

This PR delivered what it set out to: the last raw nix build for a Basecamp is gone from my repo's CI, and it works. I'd rather see it merge than hold it for a rename.

But I'd flag setup-inspector-as-a-verb as worth deciding before the flag ships in a release and the name is load-bearing. Renaming a flag after consumers pin to it is the expensive version of this conversation.

One smaller thing that would help regardless of the outcome: install and launch could name the fix in the error. They already know both the configured attr and the artefact's variant, so "run lgs basecamp setup to switch to the dev stack" is derivable, and it turns a two-command detour into a one-line one.

[fryorcraken]

This is actually likely to be unnecessary, will close once confirmed.

[fryorcraken]

Closing this, together with #265 which it implements — the motivating issue turned out to rest on a false premise of mine. Detail in #265's closing comment; the short version is that bin-bundle-dir-inspector is not the only Basecamp output with the QML inspector compiled in. The dev #app has it too — enableInspector = false is set on appDistributed, which feeds the shipping outputs, not on app. Upstream's own inspector-driven checks use appPkg = app.

The consuming repo now runs its whole end-to-end suite against #app on released scaffold, with no --inspector and no attr changes at all. So the dev-vs-portable classification this PR exists to fix never needed fixing for that use case — it only arose from picking the bundle in the first place.

What is being discarded, and what I would keep

Two separable things landed here. I want to be explicit that only one of them is moot:

• setup --inspector — moot, per the above. The flag, the BASECAMP_PORTABLE_ATTRS addition and the lockstep [repos.lgpm].attr move all go with it.

• --print-output on basecamp build / build-portable — not moot, and genuinely useful. It is only unnecessary for us: print_output_enabled() is read inside run_logged, which every build path goes through regardless of subcommand, so LOGOS_SCAFFOLD_PRINT_OUTPUT=1 as a job-level env var already gives the whole behaviour on v0.3.1. Both our workflows now do that.

  The flag is still the better ergonomics — install already has it, and a user who has just watched a build fail with a one-line status and a log file on a runner about to be destroyed should not have to know an env var exists. If you want that half, it is worth a small standalone PR rather than resurrecting this branch; it would be a handful of lines against current master, without the inspector machinery or the ~230 lines of CLI tests that were testing the flag I no longer need. I am happy to send it if it is wanted — say the word. I am not opening it unprompted, since I no longer have a use for it and an unused feature is a maintenance cost you would be carrying, not me.

One thing worth recording

The revert of save_project_config's comment preservation out of this branch — sensible on its own terms, since ~500 lines of config-writer surgery had nothing to do with selecting a basecamp build — silently reintroduced scaffold.toml comment stripping in a repo that had already written it down as fixed. That is not a complaint about the revert; it is the hazard of a downstream consumer pinning cargo install --git --rev to an open PR branch, which is what we were doing and have now stopped. Worth knowing that pinning to a PR branch is a thing people do to your repo.

basecamp setup preserving comments remains a real open ask, independent of everything above.

Apologies for the churn, and thanks for the review time already spent on this.

[fryorcraken]

Link for anyone arriving here later: the downstream change that removed the need for this is fryorcraken/logos-radicle-module#38, now merged.

It switches that repo's end-to-end layer to the dev #app and returns both CI workflows to released logos-scaffold 0.3.1 — no --inspector, no --print-output flag, no --git --rev pin. LOGOS_SCAFFOLD_PRINT_OUTPUT=1 as a job-level env var covers the logging half on the release, as described above.