feat(40): read both the legacy and DEED launcher metadata dialects - #44
Conversation
Phase 1 of launch-scaffolder#40: the reader accepts a `@launcher-deed` block alongside the `@a2ml-metadata` one every launcher minted to date carries. The emitter is untouched — phase 2 switches `mint`, gated on a tagged release carrying this. Both dialects flatten to the same `MetadataBlock`, so every existing caller is dialect-blind. That equality is asserted directly rather than via two parallel assertion lists: if the scalars and lists match, no caller can tell the forms apart. The DEED form is not re-scanned here. The `#` comment prefix is stripped and the text handed to `deed::parse`, the normative grammar. A second s-expression scanner in this file would be cheaper today and would manufacture a second DEED grammar that nothing forces to agree with `deed.rs` — it would keep passing while being wrong the moment the normative grammar moves. A test proves the delegation is real: a tab inside the block must produce the `HTAB` diagnostic, a rule only the real grammar knows. The new marker deliberately does not say `a2ml`. A2ML is retired, and a marker carrying the name would propagate it into every launcher minted from here on. `rewrite_scalar` refuses a DEED block rather than editing it. Its scanner looks for `key = "value"` and a quoted span; handed a deed form it would match nothing, or worse, the wrong quotes. An error is the honest outcome — the caller never gets a corrupted launcher. A script carrying both marker pairs is rejected rather than silently preferring one: two blocks mean two answers to the same question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPSJ7fBhVAMcpSffCBWUDo Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⚠ Do not read this PR's two green checks as a pass — 14 checks that run on #43 never run here. Measured just now:
The cause is in on:
push:
branches: [main, master]
pull_request:
branches: [main, master]A PR whose base is a feature branch does not match Two consequences, both benign but worth stating rather than discovering:
🤖 Generated with Claude Code |
Correction to my previous comment — "nothing needs doing here" was wrongMy earlier note ended by saying this PR's missing checks self-resolve on merge of #43, so nothing needs doing here. The first half is right; the second is not, and the merge procedure gains steps because of it. Correcting it rather than leaving a false reassurance standing. What I re-measured
It is worse than "14 of 16 missing"The two checks that do run here are CodeRabbit and GitGuardian Security Checks — both third-party GitHub Apps, neither of which honours a workflow base filter. Every first-party workflow in this repo that triggers on # governance.yml pull_request: branches: [main, master]
# codeql.yml pull_request: branches: [main]
# hypatia-scan.yml pull_request: branches: [main, master]So the accurate statement is: no CI defined in this repository has run against this PR at all. The two green checks are entirely external. ⚠ And CodeRabbit's check reports conclusion Merge procedure for this PR
One consequence of step 3If #43 lands as a squash, this branch's head still carries Unchanged from my previous commentIssue #45 still stands and still compounds this: there is no Rust CI in this repository at all, so even the full 16 checks on 🤖 Generated with Claude Code |
c0b1276
into
feat/40-metadata-block-fixture
#40's criteria are all implemented in code shipped by PRs #43, #44 and #46. What the issue still asks for is the write-up: the measurements, taken on a named commit, with the numbers attached. Recorded here: * the fixture predates both phases, and still carries artefacts no post-phase emitter can produce; * PR #44 touched only metadata_block.rs and round_trip.rs — the template is absent from its file list, which is the "phase 1 shipped alone" claim, checked rather than remembered; * the phase-1 fixture test after phase 2, including the one assertion that had to change and why that is not a compatibility break: every value assertion is untouched and still passes, and the restored phase-1/2-era assertion fails naming exactly the four fields #41 started enforcing; * the eight round-trip tests and what each pins; * the mutant kill — reverting the legacy arm of parse_from_text to `return Ok(None)` fails 11 tests, reverting that restores 98 passing. Two things are recorded as still open rather than quietly omitted: #45 AC4 (actions.lock could not be generated here) and the `--version` gap between the generated launcher and the standard's (required-modes). Closes #40 Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Phase 1 of #40: a compat reader that accepts the
@launcher-deeddialectalongside the
@a2ml-metadatablock every launcher minted to date carries.The emitter is untouched. Phase 2 switches
mint, gated on a taggedrelease carrying this — per the owner's ruling that "baked" means a tagged
release, not a merge to
main.Stacked on #43, which supplies the fixture. GitHub retargets this to
mainwhen #43 merges.
⚠ This repo has no Rust CI, so a green check here proves nothing
Measured:
.github/workflows/holds six files —codeql.yml,governance.yml,hypatia-scan.yml,labels.yml,label-triage.yml,push-email-notify.yml—and
grep -rn 'cargo test\|cargo nextest'over that directory returns zero.No workflow builds or tests the crates. Whatever goes green on this PR is
governance and scanning, not Rust.
So the local run below is the only real evidence, and it is quoted rather than
summarised. Filed as #45, with acceptance criteria, per the
standing rule that a new finding is an issue, not a merge blocker.
cargo clippy --all-targets -- -D warningsis clean.A passing suite proves nothing until a mutant dies
#40 asks for exactly this. Eight mutants, each applied to a clean tree and
reverted after:
Ok(None)the_committed_legacy_fixture_still_parsesOk(None)a_deed_dialect_block_parsesapp-displaymapped to the deed's:nameinstead of:displayboth_dialects_flatten_to_the_same_valuesred;both_dialects_agree_on_what_todays_emitter_producesrednode.head != "praxis-deed"check removeda_deed_block_that_is_not_a_praxis_deed_is_rejectedred:standardsstring-count strictness check removed:beholding-chorarequirement removeda_deed_block_without_beholding_chora_is_rejectedredrewrite_scalar's deed guard removedrewrite_scalar_refuses_a_deed_block_rather_than_corrupting_itreda_script_carrying_both_dialects_is_rejectedredMutant E survived the first pass, and that was a real gap.
Value::str_list()skips non-strings silently, so a symbol or integersmuggled into
:standardswould vanish from the compliance claim. The readercompares
str_list().len()againstas_list().len()for exactly that — butnothing tested it, so the comparison was dead code that would have shipped
looking correct. Added
a_non_string_entry_in_standards_is_reported_not_silently_dropped; mutant Enow dies. That test exists because the mutant survived, which is the whole
argument for running them.
Worth recording: removing the both-dialects arm outright (mutant H's first
form) does not compile — the match is exhaustiveness-checked. H was re-run as
a semantic mutant instead, which is the honest test.
Criterion 5 — the round trip, delivered not deferred
mint → parse → realign → parse, on both forms, incrates/launcher-common/tests/round_trip.rs. No CLI needed:realignhas norewrite path in phase 1 —
cmd_realign.rs:156callstemplate::renderand:172writes the result — andtemplate::renderis public, so the test drivesthe identical call
realign_onemakes.mint_parse_realign_parse_is_stable_for_the_legacy_form— also asserts thetwo renders are byte-identical, which is what
Outcome::Unchanged(
cmd_realign.rs:163) is decided on. If that ever stops holding,realignrewrites every launcher on every run.
both_dialects_agree_on_what_todays_emitter_produces— the deed sample isgenerated from the live emitter's own output, not typed by hand, so it
moves when the emitter moves. This is the "on both forms" half.
realign_cannot_edit_a_deed_launcher_in_placephase_one_mint_emits_the_legacy_markers_and_not_the_deed_ones— states theno-emitter-change claim as a test, so phase 2 has to delete it deliberately.
The generator carries a guard that panics if
mintever emits a scalar thedeed dialect has no slot for — silently dropping one is precisely the phase-2
regression this change exists to prevent. Verified non-vacuous: removing
generatorfrom that match list turns two tests red with the intended message.Why the deed form is not re-scanned here
The
#prefix is stripped and the text handed todeed::parse, the normativegrammar. A second s-expression scanner in this file would be cheaper today and
would manufacture a second DEED grammar that nothing forces to agree with
deed.rs— it would keep passing while being wrong the moment the normativegrammar moves. Delegating gets the tab check, escape handling,
#u5literals,#t/#f, and list-vs-clause disambiguation for free.the_real_deed_grammar_is_the_one_enforcing_the_blockproves the delegation isreal: a literal tab inside the block must produce the
HTABdiagnostic, a ruleonly the real grammar knows. A hand-rolled scanner here would accept it.
Scope
Zero
.terafiles. Zero diff incmd_config.rs,config.rs,discovery.rsand
template.rs—config.rs/discovery.rsare standards#960, not this.is_deed()is derived from the captured marker line rather than stored as astruct field, so there is no API break and it cannot desync from
raw_lines.The new marker deliberately does not say
a2ml: A2ML is retired, and a markercarrying the name would propagate it into every launcher minted from here on.
One unrelated observation
cargo fmt --checkis red onmaintoday, independent of this PR — rustfmtcollapses a five-line
app_licenseinsert intemplate.rsto one. Runningcargo fmt --allin any PR silently pulls that into the diff; reverted here sothe "no emitter change" claim stays clean. Not fixed here — it is not this PR's
to carry.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WPSJ7fBhVAMcpSffCBWUDo