diff --git a/internal/adapters/source/local/local.go b/internal/adapters/source/local/local.go index 66b8f462..aed46d47 100644 --- a/internal/adapters/source/local/local.go +++ b/internal/adapters/source/local/local.go @@ -58,13 +58,16 @@ func (s *Source) Schemes() []string { return []string{Scheme} } // one transport verify from another, and is worth the extra unpack of a bundle // measured in kilobytes. func (s *Source) Resolve(ctx context.Context, ref ports.Ref) (ports.ResolvedRelease, error) { - dir, cleanup, err := s.materialise(ref) + dir, source, cleanup, err := s.materialise(ref) if err != nil { return ports.ResolvedRelease{}, err } defer cleanup() - rel, err := release.Load(dir) + // Named for the archive the operator passed, not for the directory it + // was unpacked into. `source` is empty for a bundle already on disk as + // a directory, where the two are the same path anyway. + rel, err := release.LoadAs(dir, source) if err != nil { return ports.ResolvedRelease{}, err } @@ -104,7 +107,22 @@ func (s *Source) Fetch(ctx context.Context, ref ports.Ref, destDir string) (port if err := atomicfs.ExtractTarZst(path, destDir, s.limits); err != nil { return "", err } - if _, err := release.Load(destDir); err != nil { + // Named for the archive, not for where it was just unpacked. A + // plan unpacks into a temporary directory and a real install + // into the release store, and neither is a path the operator + // chose or can act on -- this is the only read of the bundle a + // caller of Fetch does not make itself, so it has to carry the + // name too. + if _, err := release.LoadAs(destDir, path); err != nil { + // Extracted before it could be validated, so a refusal here + // is a refusal to a directory this call already filled. + // Neither caller cleans it up -- `fetchRelease` returns + // straight out on a Fetch error, and `stepStageRelease`'s + // compensation keys off the release in engine state, which + // a failed Fetch never put there -- and an unusable release + // left in the store is one `update --to` away from being + // installed by somebody who never saw this error. + _ = atomicfs.RemoveAll(destDir) return "", err } return ports.BundlePath(destDir), nil @@ -172,26 +190,29 @@ func (s *Source) List(ctx context.Context, ref ports.Ref) ([]domain.Version, err // // The returned cleanup is always non-nil, so callers can defer it // unconditionally rather than branching on how the bundle arrived. -func (s *Source) materialise(ref ports.Ref) (dir string, cleanup func(), err error) { +// The returned `source` is what the operator named, and is empty when that is +// the directory being returned -- so a caller can name the archive in an error +// about its contents without having to know whether one was unpacked. +func (s *Source) materialise(ref ports.Ref) (dir, source string, cleanup func(), err error) { path, err := s.locate(ref) if err != nil { - return "", func() {}, err + return "", "", func() {}, err } if !atomicfs.IsTarZst(path) { - return path, func() {}, nil + return path, "", func() {}, nil } tmp, err := os.MkdirTemp("", "morzer-resolve-") if err != nil { - return "", func() {}, domain.Internal(err, "cannot create a temporary directory") + return "", "", func() {}, domain.Internal(err, "cannot create a temporary directory") } cleanup = func() { _ = atomicfs.RemoveAll(tmp) } if err := atomicfs.ExtractTarZst(path, tmp, s.limits); err != nil { cleanup() - return "", func() {}, err + return "", "", func() {}, err } - return tmp, cleanup, nil + return tmp, path, cleanup, nil } // locate resolves the reference to a bundle directory or archive file. diff --git a/internal/adapters/source/local/local_test.go b/internal/adapters/source/local/local_test.go index 81d0f3ab..7d9aa4c8 100644 --- a/internal/adapters/source/local/local_test.go +++ b/internal/adapters/source/local/local_test.go @@ -6,10 +6,13 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/morzecrew/morzer/internal/adapters/source/local" "github.com/morzecrew/morzer/internal/domain" + "github.com/morzecrew/morzer/internal/infra/atomicfs" "github.com/morzecrew/morzer/internal/ports" + "github.com/morzecrew/morzer/internal/release" ) // This adapter is the reference implementation of ReleaseSource: resolve @@ -355,3 +358,33 @@ func copyTree(src, dst string) error { return os.WriteFile(target, data, info.Mode().Perm()) }) } + +// Fetch extracts before it validates, so a bundle it refuses is a bundle it +// has already written. Leaving it there puts an unusable release in the store, +// one `update --to` away from being installed by somebody who never saw the +// error -- and neither caller cleans it up: `ops.fetchRelease` returns straight +// out on a Fetch error, and `stepStageRelease`'s compensation keys off the +// release in engine state, which a failed Fetch never put there. +func TestFetchRemovesAnArchiveItRefuses(t *testing.T) { + src := t.TempDir() + if err := os.WriteFile(filepath.Join(src, release.ManifestFileName), + []byte("schema_version: 1\nmetadata:\n name: demo\n version: not-a-version\n"), + 0o644); err != nil { + t.Fatal(err) + } + archive := filepath.Join(t.TempDir(), "demo-1.2.0.tar.zst") + if err := atomicfs.WriteTarZst(archive, src, + []string{release.ManifestFileName}, time.Unix(0, 0)); err != nil { + t.Fatal(err) + } + + dest := filepath.Join(t.TempDir(), "releases", "1.2.0") + if _, err := local.New().Fetch(context.Background(), ref(archive), dest); err == nil { + t.Fatal("expected the invalid bundle to be refused") + } + + entries, err := os.ReadDir(dest) + if err == nil && len(entries) > 0 { + t.Fatalf("Fetch left %d entries in the destination it refused", len(entries)) + } +} diff --git a/internal/lifecycle/ops/ops.go b/internal/lifecycle/ops/ops.go index eaacb9d2..95eead34 100644 --- a/internal/lifecycle/ops/ops.go +++ b/internal/lifecycle/ops/ops.go @@ -315,7 +315,13 @@ func (d *Deps) checkPlannedRelease(ctx context.Context, releasePath string, } // LoadManifest validates: the answer this plan needs is already computed // here, and used to be discarded with a bare return. - m, err := release.LoadManifest(filepath.Join(bundle.String(), release.ManifestFileName)) + // + // Named for what the operator passed, not for the copy above. The real + // path names the source without trying, because Resolve reads a local + // bundle in place -- so a plan that named its own scratch directory + // disagreed with the run about a path they were both refusing. + m, err := release.LoadManifestAs( + filepath.Join(bundle.String(), release.ManifestFileName), releasePath) if err != nil { return false, err } diff --git a/internal/release/load.go b/internal/release/load.go index 6e2c3ac6..8ea4b1bb 100644 --- a/internal/release/load.go +++ b/internal/release/load.go @@ -42,27 +42,56 @@ const ReleaseNotesFileName = "RELEASE.md" // The digest is computed over the whole tree, so identity is content-based // rather than trusting what the manifest claims about itself. func Load(dir string) (domain.Release, error) { + return loadFrom(dir, "") +} + +// LoadAs is Load naming `source` in whatever it refuses, instead of the +// directory it actually read. +// +// For the same reason LoadManifestAs exists, one level up: a source that has to +// unpack an archive before it can read the bundle reads an extracted copy, and +// naming that copy points the operator at a directory they never chose and +// which is removed on the way out. The archive is the shape a vendor publishes, +// so that is the install path most operators take. +// +// An empty `source` means "name what you read", which is what Load passes. The +// distinction from LoadManifestAs is deliberate: that one is handed a source by +// a caller that has decided to override, and an empty string there is a bug, +// while here the empty case is the un-overridden default and has a caller. +func LoadAs(dir, source string) (domain.Release, error) { + return loadFrom(dir, source) +} + +func loadFrom(dir, source string) (domain.Release, error) { + named := func(real string) string { + if source != "" { + return source + } + return real + } + abs, err := filepath.Abs(dir) if err != nil { - return domain.Release{}, domain.ValidationError(err, "cannot resolve %s", dir) + return domain.Release{}, domain.ValidationError(err, "cannot resolve %s", named(dir)) } info, err := os.Stat(abs) if err != nil { if errors.Is(err, fs.ErrNotExist) { return domain.Release{}, domain.ValidationError(domain.ErrReleaseNotFound, - "no release bundle at %s", abs). + "no release bundle at %s", named(abs)). WithHint("check the path, or run `morzer release list` to see installed releases") } - return domain.Release{}, domain.ValidationError(err, "cannot read %s", abs) + return domain.Release{}, domain.ValidationError(err, "cannot read %s", named(abs)) } if !info.IsDir() { return domain.Release{}, domain.ValidationError(nil, - "%s is not a directory", abs). + "%s is not a directory", named(abs)). WithHint("point at an unpacked bundle directory containing %s", ManifestFileName) } - manifest, err := LoadManifest(filepath.Join(abs, ManifestFileName)) + manifestPath := filepath.Join(abs, ManifestFileName) + manifest, err := LoadManifestAs(manifestPath, named(manifestPath)) if err != nil { return domain.Release{}, err } @@ -95,16 +124,40 @@ func Load(dir string) (domain.Release, error) { // LoadManifest reads, decodes, defaults and validates one manifest file. func LoadManifest(path string) (domain.Manifest, error) { + return LoadManifestAs(path, path) +} + +// LoadManifestAs is LoadManifest naming `source` in whatever it refuses, +// instead of the file it actually read. +// +// The two differ only where the bytes came from a copy. A plan stages the +// bundle into a temporary directory to read it, and every message naming that +// directory points the operator at a path they never chose and which is gone +// before they can go and look at it. ParseManifest prefixes the source so an +// author with several bundles open knows which one is being complained about, +// and a temp path answers that question with a path they cannot place. +// +// Callers reading a file the operator named pass LoadManifest and get the two +// arguments equal, which is the honest default: naming the file you read is +// right everywhere except when you read a copy on their behalf. +// +// `source` must not be empty, and there is deliberately no fallback for it. An +// empty one renders as `error: : manifest is invalid:`, which is loud, wrong in +// a way nobody can mistake for intended, and caught by the assertions in +// `TestAFirstInstallRefusesADeprecatedBundle`. Quietly substituting `path` +// would turn a caller's mistake into the exact defect this function exists to +// remove, and leave it to be found by an operator instead of by a test. +func LoadManifestAs(path, source string) (domain.Manifest, error) { data, err := os.ReadFile(path) if err != nil { if errors.Is(err, fs.ErrNotExist) { return domain.Manifest{}, domain.ValidationError(domain.ErrReleaseNotFound, - "no manifest at %s", path). + "no manifest at %s", source). WithHint("every release bundle must contain a %s at its root", ManifestFileName) } - return domain.Manifest{}, domain.ValidationError(err, "cannot read %s", path) + return domain.Manifest{}, domain.ValidationError(err, "cannot read %s", source) } - return ParseManifest(data, path) + return ParseManifest(data, source) } // managerVersion is the running manager's own version, recorded once at diff --git a/logs/wave-39.md b/logs/wave-39.md index 1fc4dbdc..ceba9fe8 100644 --- a/logs/wave-39.md +++ b/logs/wave-39.md @@ -209,3 +209,48 @@ than the missing value. **Drift count: still 0.** Both entries are gaps in what this wave built, found before it merged, and neither contradicts a document. + +## Correction — 2026-08-27, the exit code depends on a flag this entry did not name + +**The entry above, and the "Carried into the next unit" item it fed, say the +real `init` refuses a legacy bundle at exit 11. It does that only when +`--product` is passed.** The claim is left standing, as this collection's +corrections are. + +Measured on wave 41's branch against a real binary and a legacy directory +bundle: + +``` +morzer init --release … → exit 2 +morzer init --product demo --release … → exit 11 +morzer init --product demo --release … --dry-run → exit 2 +``` + +Without `--product` the CLI reads the manifest at the source to learn the +product name (`internal/cli/commands.go:64`), and validation comes with the +read — so the refusal lands before any operation is built. With it, that read is +skipped, three steps run and are rolled back, and the failure is the step's. + +The shape the entry described is real and worse than it says. On the exit-11 +path the top-level error is `code: compensated, category: system` — a system +category for what is the operator's input — and the manifest problem appears +exactly once in the whole `--json` stream, inside `record.steps[3].error`. A +consumer reading `.error` is told the operation rolled back and nothing about +why. + +**The cause is general, not this command's.** `domain.ExitCode` tests +`ErrCompensated` (`internal/domain/errors.go:336`) before `ErrUsage` and +`ErrValidation` (line 354), so the compensation wrapper outranks every cause in +every compensated operation. Nothing records that precedence as a decision: no +RFC decision row covers it, `docscheck`'s `checkExitCodes` only asserts each +constant has a row in the published table, and `internal/domain` has no direct +test of `ExitCode` at all. A change to that ordering passes every gate in +`just ci`. + +Carried to an RFC rather than fixed. The published table names "an invalid +manifest" under 2 *and* "back where it started" under 11, and both rows describe +that run truthfully — which is a question about what an exit code reports, not a +defect to patch inside a task. + +**Drift count: unchanged.** This corrects a claim's scope and names its cause; +no entry's class changes. diff --git a/logs/wave-41.md b/logs/wave-41.md new file mode 100644 index 00000000..d1646c66 --- /dev/null +++ b/logs/wave-41.md @@ -0,0 +1,198 @@ +# Wave 41 · The path the operator typed + +Executed against branch `wave-41-the-path-the-operator-typed`. Carried out of +wave 40's scoping pass, which measured four carried items and found that two +were ready, one was blocked on an RFC nobody has written, and one was smaller +than it looked. + +**Drift count: 0.** Nothing a document settled was built otherwise. The one +departure below is from this wave's own execution plan, announced before any +code was written against it, and RFC 0030's destroyed grades were already +counted as drift by wave 37. + +Where building this disagreed with the plan for it, written at the moment it +happened. Nothing here is revised afterwards to agree with what was later +settled, and nothing here has been folded back into any RFC's own text. + +| Class | Test | Meaning | +|---|---|---| +| `discovery` | Could not have been known before code existed | Healthy — the spec was right to be silent | +| `spec-gap` | Could have been known; the spec was silent or at the wrong altitude | The design process missed something | +| `drift` | The spec covered it and it was built otherwise anyway | **A defect** | +| `irreducible` | No amount of design settles it | Stop and spike | + +```divergence +decision: unlisted +grade: UNLISTED +class: spec-gap +at: 2026-08-27T10:05:00Z +attempt: 1 +claim: a plan reads the bundle from a copy it stages under /tmp, and ParseManifest prefixes whatever file it was handed, so a refusal named a morzer-plan- directory the operator never chose and which is removed before they can look at it +evidence: internal/lifecycle/ops/ops.go:305 +action: decided +proposal: ASSUMED — a refusal names what the operator passed. `release.LoadManifestAs` names a source it did not read from, and the plan passes `--release`. The real path already names the source without trying, because Resolve reads a local bundle in place. +``` + +The prefix is not decoration. `ParseManifest` puts the source in front of every +validation failure so "an author with several bundles open knows which file is +being complained about" — and a path under `/tmp` answers that question with one +they cannot place, which is worse than no prefix at all. + +`--product` is what kept this out of review. Without it the CLI reads the +manifest at the source to learn the product name, refuses there, and names the +real path by accident; the temp path is reachable only when the operator +supplies the name themselves. + +```divergence +decision: unlisted +grade: UNLISTED +class: discovery +at: 2026-08-27T10:20:00Z +attempt: 1 +claim: this wave's own plan proposed moving the plan's copy under the managed staging dir, and that directory does not exist while a plan runs — creating it would be a change, which is the one thing a plan may not make +evidence: `find $ROOT -mindepth 1 | wc -l` prints 0 after `morzer --root $ROOT init --product demo --release … --dry-run` exits 0 printing `this is a plan; nothing was changed` +action: decided +proposal: the system temp directory is correct here and the plan item is withdrawn. `update.go:187` has to `MkdirAll(StagingDir)` before using it, which is exactly the change a plan is forbidden. +``` + +**The plan asserted a location without checking it exists at the moment it would +be used.** Escaping `--root` is a real property of the copy and it is the price +of a plan that creates nothing; the alternative buys tidiness by breaking the +guarantee the flag is for. + +```divergence +decision: unlisted +grade: UNLISTED +class: discovery +at: 2026-08-27T10:55:00Z +attempt: 1 +claim: nothing in the tree asserted that a manifest refusal names any file at all — replacing LoadManifest's source with an empty string degrades every refusal to `error: : manifest is invalid:` and passes the whole suite +evidence: `go test ./internal/release/ ./test/clitest/ ./internal/lifecycle/ops/` printed `ok` for all three with `LoadManifestAs(path, "")` substituted, and the built binary then printed `error: : manifest is invalid:` for a legacy bundle +action: decided +proposal: ASSUMED — `TestAFirstInstallRefusesADeprecatedBundle` asserts the bundle path alongside the field name. The claim the prefix exists to serve is now guarded on the path an operator hits most often. +``` + +**The fix and the gap are the same claim at two altitudes.** This wave made the +plan name the right path; the sweep then asked whether anything pinned that a +path was named, and nothing did. Found by sabotage, after the fix was already +green — which is the order that finds it, since a passing suite is exactly when +the question stops being asked. + +## Found by the audit, after the entries above — 2026-08-27 + +```divergence +decision: unlisted +grade: UNLISTED +class: discovery +at: 2026-08-27T12:40:00Z +attempt: 2 +claim: the fix above covered a directory bundle and left the archive — the shape a vendor publishes — still naming a temp path, on the real install as well as the plan, because Resolve unpacks into morzer-resolve-* and Fetch loads what it just extracted before the plan's own check runs +evidence: `morzer --root $R init --product demo --release demo-1.2.0.tar.zst …` printed `failed: /tmp/morzer-resolve-2896715448/manifest.yaml: manifest is invalid:`, and the same invocation with `--dry-run` printed `error: /tmp/morzer-plan-45132326/manifest.yaml: manifest is invalid:` +action: decided +proposal: ASSUMED — `release.LoadAs` carries a name the way `LoadManifestAs` does, `materialise` returns what the operator named, and `Fetch` names the archive it unpacked. Three call sites, one rule: the reader of a copy names the original. +``` + +**The wave fixed the case it was reported against and stopped.** That is the +mistake wave 39 distilled a rule about and wave 40 was written to undo, arriving +here in its own turn — and it was found by auditing the fix, not by any gate. +`TestAnInstallFromAnArchiveIsRefusedToo` was passing throughout; it asserted the +refusal happened and never what it named. + +```divergence +decision: unlisted +grade: UNLISTED +class: discovery +at: 2026-08-27T13:05:00Z +attempt: 2 +claim: `git checkout ` to revert a sabotage ate an uncommitted doc comment in the same file, because the fix had been committed and the comment written afterwards +evidence: `git diff --stat` printed nothing after `git checkout internal/release/load.go`, on a file carrying an uncommitted paragraph documenting LoadManifestAs's contract +action: decided +proposal: the rule already exists — commit before you sabotage — and it is the *second* commit that this wave needed, not the first. A sabotage sweep that starts after new work has accumulated on top of the last commit eats that work, and the sweep is exactly when nobody is looking at the diff. +``` + +## Ruling — 2026-08-27, RFC 0030's grades + +The author graded all five rows. Rows 1, 3 and 4 `LOCKED`, rows 2 and 5 +`ASSUMED`; the answers move to a column of their own and the Grade column +carries grades again. + +| RFC | row | outcome | grade | decision | from | +|---|---|---|---|---|---| +| 0030 | 1 | accepted | LOCKED | Reopening silently re-enables units an operator disabled | wave 37, this wave | +| 0030 | 2 | accepted | ASSUMED | Diagnostic; the row argues its own live risk is worth revisiting | wave 37, this wave | +| 0030 | 3 | accepted | LOCKED | Moving them undoes every `systemctl disable`; the path is pinned by a test | wave 37, this wave | +| 0030 | 4 | accepted | LOCKED | State-schema field that bumped the version to 8; removal needs a migration | wave 37, this wave | +| 0030 | 5 | accepted | ASSUMED | `doctor`'s behaviour; depart-if-wrong is proportionate | wave 37, this wave | + +Wave 37 carried the open question inside this as *what a grade means on a +decision in an RFC that has already shipped*, since `LOCKED`, `ASSUMED` and +`OPEN` all describe what an executor does on conflict. The last three waves +answered it by doing it: wave 38 conflicted with RFC 0001 row 12 and wave 39 +with RFC 0023 decision 23, both in RFCs marked Complete. **A Complete RFC is +what later units collide with**, so the vocabulary needs nothing added and no +shared skill has to change. + +`rfc-index` goes from 27 problems to 22. What is left is the 22 RFCs whose +decision tables carry no Grade column at all. + +## Rules distilled + +- **A measurement from an earlier turn is a memory, not a measurement.** The + scratchpad was cleaned between turns, and two probes re-run against paths that + no longer existed returned answers that looked like findings: an empty `find` + read as "a plan creates nothing" and a missing fixture read as a sabotage + killing a message. Both were re-taken before anything was concluded from them. +- **Naming the file you read is right until you read a copy.** Every path in an + error is a claim about where the reader should go and look, and a stage-then- + read helper breaks it silently, because the read still succeeds. +- **A flag that changes which layer refuses changes the whole error contract.** + `--product` decides whether `init` refuses before the operation or inside it, + and with it the exit code, the category, and whether the cause is in the + top-level error at all. +- **Sabotage after green, not instead of it.** The missing path assertion was + invisible while the new test passed; the sweep found it by asking what the + suite would still accept. +- **A plan item can assert a location that does not exist yet.** Checking the + premise cost one command and withdrew half the change. +- **A path in a message is a claim about where to go and look**, so every + stage-then-read helper owes the original name. The rule generalises past this + wave: `Resolve`, `Fetch`, the plan's own check and the https download cache + are four readers of a copy, and three of them were leaking. +- **Correcting the success path is where the failure path gets forgotten.** + `https.Resolve` fixes the reference it returns and not the error it returns, + with a comment naming exactly the problem it does not solve. +- **Commit again before the *second* sweep.** The rule is "commit before you + sabotage", and the case it misses is a sweep that starts after new work has + landed on top of the commit it is sweeping. + +## Carried into the next unit + +- **What an exit code reports when a cause-code and an outcome-code both apply.** + `domain.ExitCode` answers "the outcome" for every compensated operation, in a + switch nothing documents, no RFC row covers, `docscheck` does not check, and no + test in `internal/domain` exercises. Needs an RFC; see wave 39's correction. +- **The 354 ungraded decision rows across 22 RFCs.** All 22 are Complete. The + vocabulary question that blocked them is answered above, so what remains is + volume and judgment, not a missing word. Proposed as a standing rule rather + than a unit: a wave that conflicts with an ungraded table grades that table. +- **The remote sources name their download cache, not the URL.** The same defect + this wave fixed for `file`, one layer out and worse: measured 2026-08-27, an + `https` bundle whose manifest does not validate is refused with + `/tmp/morzer-download-2453913850/bundle-0.tar.zst is not a valid manifest:`, + and the URL the operator typed appears nowhere. `https.Resolve` already + corrects this on the *success* path — `resolved.Ref = ref`, with a comment + saying "not the temp path we resolved through" — and leaves the failure path + carrying it. **Not fixed here because it is a port change:** `https` delegates + by constructing `ports.Ref{Scheme: local.Scheme, Location: }`, so there + is nowhere to put the operator's name without adding one to the reference, and + a change to `ports.Ref` re-opens the shared `ReleaseSource` conformance battery + for all three sources. `oci` is presumed to have it too and was not measured. +- **Whether a plan should verify signatures** (wave 39), and it is cheaper than + wave 40 assessed. A plan already fetches a local bundle into a temporary + directory to read its manifest, so verification there costs a hash over bytes + already on disk; it declines to look only for non-`file` schemes. +- **`rfc-index` is not wired into `ci`**, now failing on 22 problems. +- **`FieldRemovalRelease` is a single-member design with no members** (D-052). +- **`release.draft: true` means a human publishes**, and that human is the last + reader of the notes (release 0.3.0). +- ~~**RFC 0030's four destroyed grades**~~ (waves 37–38) — closed by this wave. diff --git a/rfcs/0030-unit-enablement-is-the-operators.md b/rfcs/0030-unit-enablement-is-the-operators.md index 29faa052..e3131434 100644 --- a/rfcs/0030-unit-enablement-is-the-operators.md +++ b/rfcs/0030-unit-enablement-is-the-operators.md @@ -150,13 +150,21 @@ could not be answered until the move was priced — which closes this RFC. The trade-off column is left as it was written, so that what was traded is legible next to what was chosen. -| # | Question | Grade | The trade-off | -| --- | --- | --- | --- | -| 1 | Does the manager re-assert *enablement* on a unit that already exists? | ✅ ANSWERED — no (§8.1) | Enabling only units this run newly wrote makes `systemctl disable` stick — and means a unit left disabled by a half-finished install is never repaired, which is the case `init --repair` exists for. Enablement is either the operator's or the manager's; it cannot be both, and today it is the manager's — announced, since row 2, but still the manager's. | -| 2 | Should `UnitState.Enabled` be reported? | ✅ ANSWERED — yes, in [#42](https://github.com/morzecrew/morzer/pull/42) | Shipped as `: not enabled` on a loaded unit the supervisor asked to have enabled, with the oneshot services exempt. It was the smallest useful step and it decided nothing about row 1. Its risk was recorded here as hypothetical and is now live: an operator who *meant* to disable a unit gets a warning on every run, clearable only by letting the next reconciliation overrule them. That is the antipattern 0026's audit removed from this very check, arriving by a different door — and it is the strongest argument for answering row 1 rather than leaving it. | -| 3 | Do the generated units belong in `/etc/systemd/system`? | ✅ ANSWERED — yes, they stay (§8.4) | Answered by what moving them would cost, which was never priced when this row was written. systemd loads `/etc/systemd/system` in preference to `/usr/lib/systemd/system` (systemd.unit(5), measured on systemd 261), so on every machine that already has units the old copy would keep winning and the manager would write files that have no effect. Worse, `InstallUnits` computes freshness from the file's presence in the unit directory, so a move makes every unit fresh at once and `EnableNew` re-enables the lot — undoing every `systemctl disable` an operator made, which is precisely the harm row 1 shipped to prevent, arriving through a migration. Against: `systemctl mask` stays unavailable (§3.2), and drop-in overrides stay awkward. That cost is now bounded rather than open-ended, because rows 1 and 4 gave the operator two working ways to say "off" — a `disable` that sticks, and `policy.skip_scheduled_backups` — so masking is a mechanism for an intent that is already expressible. Consequence: the path is now a decided value rather than a default, and is pinned by a test, because relocating it is what makes every other test in that adapter runnable without root. Added by execution 2026-08-17 — see logs/wave-31.md (D-026). | -| 4 | Should an installation be able to declare "no scheduled backups"? | ✅ ANSWERED — yes, `policy.skip_scheduled_backups` (§8.2) | A declarative flag (`policy.skip_scheduled_backups`, named for the unsafe direction as `SkipBackupBeforeUpdate` is) puts the fact in the installation, where `init --repair` reproduces it and 0027's desired-state story can carry it. Against: it is a *second* way to say something the operator can already say, and adding it without answering row 1 leaves the first way broken and the two disagreeing. | -| 5 | If backups are handled elsewhere, what does `doctor` say? | ✅ ANSWERED — it honours the declaration (§8.3) | A machine backed up at the storage layer never updates the last-backup timestamp, so `StaleBackupAfter` warns for ever. Suppressing that means the manager asserting a fact it cannot verify. Not suppressing it means a permanent warning — which is precisely how a check stops being read. | +**The Grade column was destroyed, and is restored here.** It carried `OPEN` on +rows 1, 3, 4 and 5 at `6d3752d`; answering each question overwrote its grade in +place, one careful edit at a time, until the heading was the last evidence the +column had ever held one. The answers now have a column of their own. The grades +say what a later unit does on meeting a conflict with a row — a question a +Complete RFC still poses, as waves 38 and 39 both found by conflicting with one. +Graded by execution 2026-08-27 — see logs/wave-41.md (unlisted, 2026-08-27T11:20:00Z). + +| # | Question | Grade | Answer | The trade-off | +| --- | --- | --- | --- | --- | +| 1 | Does the manager re-assert *enablement* on a unit that already exists? | LOCKED | ✅ no (§8.1) | Enabling only units this run newly wrote makes `systemctl disable` stick — and means a unit left disabled by a half-finished install is never repaired, which is the case `init --repair` exists for. Enablement is either the operator's or the manager's; it cannot be both, and today it is the manager's — announced, since row 2, but still the manager's. | +| 2 | Should `UnitState.Enabled` be reported? | ASSUMED | ✅ yes, in [#42](https://github.com/morzecrew/morzer/pull/42) | Shipped as `: not enabled` on a loaded unit the supervisor asked to have enabled, with the oneshot services exempt. It was the smallest useful step and it decided nothing about row 1. Its risk was recorded here as hypothetical and is now live: an operator who *meant* to disable a unit gets a warning on every run, clearable only by letting the next reconciliation overrule them. That is the antipattern 0026's audit removed from this very check, arriving by a different door — and it is the strongest argument for answering row 1 rather than leaving it. | +| 3 | Do the generated units belong in `/etc/systemd/system`? | LOCKED | ✅ yes, they stay (§8.4) | Answered by what moving them would cost, which was never priced when this row was written. systemd loads `/etc/systemd/system` in preference to `/usr/lib/systemd/system` (systemd.unit(5), measured on systemd 261), so on every machine that already has units the old copy would keep winning and the manager would write files that have no effect. Worse, `InstallUnits` computes freshness from the file's presence in the unit directory, so a move makes every unit fresh at once and `EnableNew` re-enables the lot — undoing every `systemctl disable` an operator made, which is precisely the harm row 1 shipped to prevent, arriving through a migration. Against: `systemctl mask` stays unavailable (§3.2), and drop-in overrides stay awkward. That cost is now bounded rather than open-ended, because rows 1 and 4 gave the operator two working ways to say "off" — a `disable` that sticks, and `policy.skip_scheduled_backups` — so masking is a mechanism for an intent that is already expressible. Consequence: the path is now a decided value rather than a default, and is pinned by a test, because relocating it is what makes every other test in that adapter runnable without root. Added by execution 2026-08-17 — see logs/wave-31.md (D-026). | +| 4 | Should an installation be able to declare "no scheduled backups"? | LOCKED | ✅ yes, `policy.skip_scheduled_backups` (§8.2) | A declarative flag (`policy.skip_scheduled_backups`, named for the unsafe direction as `SkipBackupBeforeUpdate` is) puts the fact in the installation, where `init --repair` reproduces it and 0027's desired-state story can carry it. Against: it is a *second* way to say something the operator can already say, and adding it without answering row 1 leaves the first way broken and the two disagreeing. | +| 5 | If backups are handled elsewhere, what does `doctor` say? | ASSUMED | ✅ it honours the declaration (§8.3) | A machine backed up at the storage layer never updates the last-backup timestamp, so `StaleBackupAfter` warns for ever. Suppressing that means the manager asserting a fact it cannot verify. Not suppressing it means a permanent warning — which is precisely how a check stops being read. | **Row 4 is downstream of row 1**, and answering it first is the mistake this RFC exists to prevent. It was answered second, and §8.2 records what that bought. diff --git a/test/clitest/deprecation_test.go b/test/clitest/deprecation_test.go index b536c3e1..73cabe2c 100644 --- a/test/clitest/deprecation_test.go +++ b/test/clitest/deprecation_test.go @@ -40,15 +40,23 @@ func TestTheCurrentSpellingVerifies(t *testing.T) { // them with nothing to ask for. func TestAFirstInstallRefusesADeprecatedBundle(t *testing.T) { r := clitest.New(t) + bundle := r.LegacyBundle() + // The path is asserted, not incidental. `ParseManifest` prefixes the + // source so an author with several bundles open knows which one is + // being complained about, and until this line nothing checked that any + // path was named at all: replacing the source with an empty string + // degrades every refusal to `error: : manifest is invalid:` and passed + // the whole suite. Found by sabotage while fixing the plan's half of + // the same claim. r.Run("init", - "--release", r.LegacyBundle(), + "--release", bundle, "--profile", "embedded", "--domain", "demo.example", "--no-recovery-recipient", "--install-units=false", ).ExitCode(2). - StderrContains("is no longer read", "runtimes.compose") + StderrContains("is no longer read", "runtimes.compose", bundle) } // The update path is asserted in the suite, where an update runs to completion @@ -92,6 +100,37 @@ func TestAPlannedInstallRefusesADeprecatedBundle(t *testing.T) { StderrContains("is no longer read") } +// The plan reads a copy, and the copy's path is no answer to "which file?". +// +// `checkPlannedRelease` stages the bundle into a temporary directory to read +// it, and `ParseManifest` prefixes whatever file it was handed -- so a refusal +// named `/tmp/morzer-plan-3095461798/manifest.yaml`, a directory the operator +// never chose and which is removed before they can go and look at it. That +// prefix exists so "an author with several bundles open knows which file is +// being complained about"; a temp path answers that question with a path they +// cannot place, which is worse than not prefixing at all. +// +// `--product` is what makes this reachable, and it is the whole reason the +// defect survived review: without it the CLI reads the manifest at the source +// to learn the product name and refuses there, naming the real path by +// accident. With it, the plan's copy is the only manifest anybody read. +func TestAPlannedRefusalNamesTheBundleTheOperatorPassed(t *testing.T) { + r := clitest.New(t) + bundle := r.LegacyBundle() + + r.Run("init", + "--product", "demo", + "--release", bundle, + "--profile", "embedded", + "--domain", "demo.example", + "--no-recovery-recipient", + "--install-units=false", + "--dry-run", + ).ExitCode(2). + StderrContains("is no longer read", bundle). + NoOutputContains("morzer-plan-") +} + // The warning-is-one-sentence assertion that stood here has moved to // TestTheDeprecationMachineryStillRendersASentenceThatCanBeActedOn in // internal/domain. It drove the field-deprecation join through a real install, @@ -162,13 +201,15 @@ func TestAPlansJSONNamesTheProductAndNoInstallation(t *testing.T) { // // Measured both ways while writing this. Recorded rather than fixed here: a // plan that validates is RFC 0001 decision 12's territory and its own change. -func TestAnInstallFromAnArchiveIsRefusedToo(t *testing.T) { - r := clitest.New(t) +// legacyArchive packs the legacy bundle the way a vendor ships one. +// +// Packed directly rather than with `release archive`, which now refuses the +// same bundle for the same reason -- so after the removal this project can no +// longer produce a legacy archive through its own commands, and a test for one +// has to build it. +func legacyArchive(t *testing.T, r *clitest.Runner) string { + t.Helper() - // Packed directly rather than with `release archive`, which now refuses - // the same bundle for the same reason -- so after the removal this - // project can no longer produce a legacy archive through its own - // commands, and a test for one has to build it. bundle := r.LegacyBundle() var entries []string if err := filepath.WalkDir(bundle, func(path string, e fs.DirEntry, err error) error { @@ -194,6 +235,33 @@ func TestAnInstallFromAnArchiveIsRefusedToo(t *testing.T) { if err := atomicfs.WriteTarZst(archive, bundle, entries, time.Unix(0, 0)); err != nil { t.Fatal(err) } + return archive +} + +// A plan over an archive reads it through `Fetch`, which loads the bundle it +// just unpacked before the plan's own check gets to it -- so the directory case +// and the archive case leaked different temporary paths, and fixing one left +// the shape a vendor actually publishes still naming `morzer-plan-*`. +func TestAPlannedRefusalNamesTheArchiveTheOperatorPassed(t *testing.T) { + r := clitest.New(t) + archive := legacyArchive(t, r) + + r.Run("init", + "--product", "demo", + "--release", archive, + "--profile", "embedded", + "--domain", "demo.example", + "--no-recovery-recipient", + "--install-units=false", + "--dry-run", + ).ExitCode(2). + StderrContains("is no longer read", archive). + NoOutputContains("morzer-plan-", "morzer-resolve-") +} + +func TestAnInstallFromAnArchiveIsRefusedToo(t *testing.T) { + r := clitest.New(t) + archive := legacyArchive(t, r) r.Run("init", "--product", "demo", @@ -203,7 +271,12 @@ func TestAnInstallFromAnArchiveIsRefusedToo(t *testing.T) { "--no-recovery-recipient", "--install-units=false", ).ExitCode(11). - StderrContains("is no longer read") + StderrContains("is no longer read", archive). + // The archive is the shape a vendor publishes, so this is the + // primary install path -- and `Resolve` extracts it into + // `morzer-resolve-*` before reading, which is the same defect + // the plan had, on the path more operators take. + NoOutputContains("morzer-resolve-") } // `--repair` restores an installation that is already there, and both summaries