diff --git a/CHANGELOG.md b/CHANGELOG.md index 665faac..5c076f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,28 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [0.13.0] - 2026-09-09 + +### Changed + +- `unless` is gone from the engine. The parser has refused the keyword for a while, but `engine.Shell` still carried the guard and the agent read it from a step's free-form arguments — reachable by a forged request, by nothing a plan can write. A capability with no way to express it is a trap (#619). + +### Fixed + +- `archive.extract` checks what its members hold, not only that they are there: their digests are recorded at extract time and compared after, so a file emptied in place is no longer reported converged. It also costs less — an observe no longer opens the archive, where listing it decompressed the whole thing every run (#614). + +- `postgres.config` doubles a single quote in the value, which is how postgres escapes one. Written raw it closed the quote early and the cluster refused the file — at startup, so the failure surfaced on the next restart rather than on the run that caused it. Verified against a real cluster, including that the value still converges (#618). + +- `sshd.config` checks the name it builds a path from, as `sudo.write` and `systemd.unit` already did. A name carrying a `/` wrote into a subdirectory sshd never reads — a drop-in believed installed that the server never sees. The def called itself the same shape as `sudo.write`, having copied its content check and not its name check (#617). + +- `==` refuses a value it cannot compare instead of crashing. Two shell results, or two outcomes, panicked the evaluator. The rule is now a whitelist — strings, ints and booleans compare, the rest is refused by name and says what to write instead. `if r == ok`, silently false before, is refused too (#616). + +- A dead `SSH_AUTH_SOCK` no longer discards a working inventory key. A socket outliving its agent failed the run over an agent the host never needed, so the same plan worked in one terminal and not in another. The agent is skipped and named in the trace; with no other method the error stands (#612). + +- `shellf status` exits non-zero when a host could not be reached. It asked a helper that only inspected block errors, so a sweep printing `unreachable` on every line still exited 0 — the worst answer for the command a monitor runs on a schedule. Drift stays a success: reporting what differs is what `status` is for (#615). + +- `archive.extract-member` extracts to a staged file and renames it. It redirected `tar` straight at the destination, so a member missing from the archive emptied the file that was there — usually an executable, since that is what this def installs. The destination's mode is carried over, which a rename would otherwise drop (#613). + ## [0.12.0] - 2026-09-08 ### Added @@ -361,7 +383,8 @@ agent that evaluates on the host — "raw shell, but idempotent, previewable, fa per-user agent/workdir scoping. - Commands: `run`, `status`, `clean`, and `version`. -[Unreleased]: https://github.com/haribo/shellf/compare/v0.12.0...HEAD +[Unreleased]: https://github.com/haribo/shellf/compare/v0.13.0...HEAD +[0.13.0]: https://github.com/haribo/shellf/compare/v0.12.0...v0.13.0 [0.12.0]: https://github.com/haribo/shellf/compare/v0.11.0...v0.12.0 [0.11.0]: https://github.com/haribo/shellf/compare/v0.10.0...v0.11.0 [0.10.0]: https://github.com/haribo/shellf/compare/v0.9.1...v0.10.0 diff --git a/cmd/shellf/main.go b/cmd/shellf/main.go index 4a72fb4..c911bd3 100644 --- a/cmd/shellf/main.go +++ b/cmd/shellf/main.go @@ -547,25 +547,18 @@ func statusCmd(args []string) { // `status` refuses an unknown target like `run` does. The render stays pure — the // exit code is the caller's call, so a report string keeps one job (#451). reports := orchestrator.Run(plan, inv, self, "status", dial, base, secrets, defsSrc, orchestrator.Options{Parallel: *parallel, Limit: limits, ValidateArgs: validate}) + // The verdict comes from the renderer that produced the report, as it does for `run`: + // asking a second function is how `status` came to exit 0 over a fleet where every host + // was unreachable (#615). if *asJSON { - out, _ := report.JSON(reports) + out, anyErr := report.JSON(reports) fmt.Print(report.RedactJSON(out, secretValues)) - exitFor(anyBlockError(reports)) + exitFor(anyErr) return } - fmt.Print(report.Redact(report.Status(reports), secretValues)) - exitFor(anyBlockError(reports)) -} - -// anyBlockError reports whether any block failed as a whole (an unknown target), as -// opposed to a per-host outcome. -func anyBlockError(reports []orchestrator.BlockReport) bool { - for _, blk := range reports { - if blk.Err != nil { - return true - } - } - return false + text, anyErr := report.Status(reports) + fmt.Print(report.Redact(text, secretValues)) + exitFor(anyErr) } // allUnknownTargets reports whether every block failed on an unknown target — the shape diff --git a/cmd/shellf/main_test.go b/cmd/shellf/main_test.go index f285c2e..a19a81d 100644 --- a/cmd/shellf/main_test.go +++ b/cmd/shellf/main_test.go @@ -10,7 +10,6 @@ import ( "strings" "testing" - "shellf/internal/orchestrator" "shellf/internal/project" ) @@ -285,37 +284,6 @@ func TestCheckParallel(t *testing.T) { // anyBlockError is what makes `status` exit non-zero on an unknown target (#451). It had // no test of its own — the behaviour was only covered end to end, where a change to it // would surface as a puzzling exit code rather than a failing assertion. -// errFake is a minimal error for table cases. internal/report has its own since #491: -// a test helper does not cross a package boundary. -type errFake string - -func (e errFake) Error() string { return string(e) } - -func TestAnyBlockError(t *testing.T) { - none := []orchestrator.BlockReport{ - {Target: "web", Hosts: []orchestrator.HostOutcome{{Host: "h1"}}}, - {Target: "db"}, - } - if anyBlockError(none) { - t.Fatal("no block failed as a whole") - } - // A per-host failure is not a block failure: the block ran, the host did not. - perHost := []orchestrator.BlockReport{{ - Target: "web", - Hosts: []orchestrator.HostOutcome{{Host: "h1", Err: errFake("unreachable")}}, - }} - if anyBlockError(perHost) { - t.Fatal("a host error is not a block error") - } - blocked := []orchestrator.BlockReport{ - {Target: "web", Hosts: []orchestrator.HostOutcome{{Host: "h1"}}}, - {Target: "wbe", Err: &orchestrator.UnknownTargetError{Target: "wbe"}}, - } - if !anyBlockError(blocked) { - t.Fatal("a block that could not run must be reported") - } -} - // `-v` must not undo what the report masks: the tracer is where redaction happens, // because the CLI is what knows the run's secrets (#461). func TestTracer_RedactsAndStaysOffStdout(t *testing.T) { diff --git a/docs/design/orchestration.md b/docs/design/orchestration.md index 59bda2f..22cc45c 100644 --- a/docs/design/orchestration.md +++ b/docs/design/orchestration.md @@ -10,8 +10,8 @@ orchestration plan maps `def` calls onto the inventory. is a singleton group. ``` -on db { postgres-install() } -on web { nginx-install(); nginx-config() } +on db { db.postgres-install() } +on web { web.nginx-install(); web.nginx-config() } ``` The `def` inside stay neutral — `on` composes them, never the reverse. @@ -22,7 +22,7 @@ The `def` inside stay neutral — `on` composes them, never the reverse. |---|---| | Between `on` blocks | **Sequential**, file order. `on db` completes on all its hosts before `on web` starts. | | Hosts within one block | **Parallel** (fan-out) — conflict-free, each host independent. | -| `def` within one block, per host | **Sequential**. `nginx-install` then `nginx-config` on that host. | +| `def` within one block, per host | **Sequential**. `web.nginx-install` then `web.nginx-config` on that host. | One SSH session per host carries the whole block's sequence (the agent is pushed once, not per `def`). @@ -36,16 +36,16 @@ absence of conflict. ``` on web { parallel { - nginx-install() - fetch-assets() + web.nginx-install() + web.fetch-assets() } - nginx-config() // after both branches complete + web.nginx-config() // after both branches complete } ``` - **Result**: aggregate — `err` if any branch is `err`. Branches run to completion before the aggregate; no short-circuit. -- **Halting**: an aggregate `err` halts the rest of the block (`nginx-config` +- **Halting**: an aggregate `err` halts the rest of the block (`web.nginx-config` skipped), per the halting rule. - **Real speedup only without a shared exclusive resource.** shellf does start the branches together, but the system may re-serialize them: two `apt` runs diff --git a/docs/language.md b/docs/language.md index f6c525c..fa73ed7 100644 --- a/docs/language.md +++ b/docs/language.md @@ -350,8 +350,8 @@ So `if dir.exists("/opt/legacy") { … }` still resolves on a host where the dir **A `check` phase must not depend on state the plan itself produces.** That is a rule for writing defs, not something the model can enforce: `systemd.unit` shipped validating its content with `systemd-analyze verify`, which refuses a unit whose `ExecStart` is not yet on disk — so a plan delivering a script and the unit calling it could not be previewed. The fix belonged in the def. ``` -if dir.exists("/opt/app") { // present → then, absent → else — deterministic even in --dry-run - apt.install("nginx") +if dir.exists("/opt/app") { // present → then; absent → the branch is previewed as + apt.install("nginx") // undetermined, not taken as else (ADR-0051) } ``` diff --git a/internal/agent/agent.go b/internal/agent/agent.go index b00b7b3..2283fb3 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -404,7 +404,9 @@ func agentErr(label string, err error) proto.StepResult { func dispatch(step proto.Step) (engine.Instruction, error) { switch step.Instruction { case "shell": - return engine.Shell{Cmd: step.Args["cmd"], Unless: step.Args["unless"], Env: engine.Env(step.Env)}, nil + // No `unless`: it was read from the step's arguments and no parser ever produced + // one, so the only way to set it was a hand-forged request (#619). + return engine.Shell{Cmd: step.Args["cmd"], Env: engine.Env(step.Env)}, nil default: return nil, fmt.Errorf("unknown instruction: %q", step.Instruction) } diff --git a/internal/agent/capture_test.go b/internal/agent/capture_test.go index e5236b6..54d0bdf 100644 --- a/internal/agent/capture_test.go +++ b/internal/agent/capture_test.go @@ -6,12 +6,8 @@ import ( "shellf/internal/proto" ) -func capture(cmd, unless, bind string) proto.Step { - args := map[string]string{"cmd": cmd} - if unless != "" { - args["unless"] = unless - } - return proto.Step{Instruction: "shell", Args: args, Bind: bind} +func capture(cmd, bind string) proto.Step { + return proto.Step{Instruction: "shell", Args: map[string]string{"cmd": cmd}, Bind: bind} } func ifRef(name, test string) proto.Step { @@ -33,7 +29,7 @@ func TestAgentCapture_ChangedRunsThen(t *testing.T) { f.set("doit", "", 0) f.set("thencmd", "", 0) serve(t, f, proto.Request{Mode: "apply", Steps: []proto.Step{ - capture("doit", "", "x"), + capture("doit", "x"), ifRef("x", "changed"), }}) if !f.called("thencmd", "") { @@ -41,19 +37,31 @@ func TestAgentCapture_ChangedRunsThen(t *testing.T) { } } +// A captured result that did not act must not fire `if x.changed { … }`. The producer is a +// def that observes itself already converged: a raw `shell` always acts, so it cannot make +// this state — it used to be made with an `unless` guard, which was removed in #619 because +// no plan could write one. +// +// Both directions matter and both fail silently: losing the flag stops every +// `if x.changed { restart }` downstream, inventing it fires them all for nothing. func TestAgentCapture_NotChangedSkipsThen(t *testing.T) { - // x = shell { doit } unless { guard-ok } → skipped → not changed → then skipped f := newFake() - f.set("guardcmd", "", 0) // guard satisfied → shell skipped - serve(t, f, proto.Request{Mode: "apply", Steps: []proto.Step{ - capture("doit", "guardcmd", "x"), - ifRef("x", "changed"), - }}) - if f.called("doit", "") { - t.Fatalf("guard satisfied → shell must be skipped") + f.set(`test -f "$path"`, "", 0) // observe: already in the desired state + serve(t, f, proto.Request{ + Mode: "apply", + Defs: map[string]string{ + "converged": `def converged(path: str) { observe { return state(there: shell { test -f "$path" }.exit == 0) } apply { shell { touch "$path" } return ok.done } }`, + }, + Steps: []proto.Step{ + {Instruction: "converged", Args: map[string]string{"path": "/tmp/x"}, Bind: "x"}, + ifRef("x", "changed"), + }, + }) + if f.called(`touch "$path"`, "") { + t.Fatal("the def was already converged → its apply must not run") } if f.called("thencmd", "") { - t.Fatalf("x.changed false → then must NOT run") + t.Fatal("x.changed false → then must NOT run") } } @@ -63,7 +71,7 @@ func TestAgentCapture_OkSugar(t *testing.T) { f.set("doit", "", 0) f.set("thencmd", "", 0) serve(t, f, proto.Request{Mode: "apply", Steps: []proto.Step{ - capture("doit", "", "x"), + capture("doit", "x"), ifRef("x", "ok"), }}) if !f.called("thencmd", "") { @@ -72,7 +80,7 @@ func TestAgentCapture_OkSugar(t *testing.T) { } func caughtCap(cmd, bind string) proto.Step { - s := capture(cmd, "", bind) + s := capture(cmd, bind) s.Caught = true return s } @@ -207,7 +215,7 @@ func TestAgentCapture_OutcomePattern(t *testing.T) { return []proto.Step{{Instruction: "shell", Args: map[string]string{"cmd": cmd}}} } serve(t, f, proto.Request{Mode: "apply", Steps: []proto.Step{ - capture("doit", "", "x"), + capture("doit", "x"), {If: &proto.IfBlock{CondRef: &proto.ResultRef{Name: "x", Category: "ok"}, Then: then("yes")}}, {If: &proto.IfBlock{CondRef: &proto.ResultRef{Name: "x", Category: "err"}, Then: then("no")}}, }}) diff --git a/internal/engine/shell.go b/internal/engine/shell.go index 9d92fcb..ebb5874 100644 --- a/internal/engine/shell.go +++ b/internal/engine/shell.go @@ -2,15 +2,19 @@ package engine import "strings" -// Shell runs a raw shell command on the target — the thesis's first-class -// citizen. Idempotence and previewability come only from an optional `unless` -// guard: without it the command always runs (and, like any raw shell, cannot be -// previewed). The user is responsible for the guard, as in Chef `not_if` / -// Puppet `unless`. +// Shell runs a raw shell command on the target — the thesis's first-class citizen. It +// always runs and, like any raw shell, cannot be previewed: a plan that needs a guard +// writes `if !shell { } { shell { } }`, which the language can express and +// preview. +// +// It carried an `Unless` guard until #619. Nothing could set it: the parser refuses the +// keyword by name (`internal/lang/parser.go`), #415 removed the def-side remnant, and the +// only remaining writer read it out of a step's free-form arguments — reachable by a forged +// request and by nothing a plan can produce. A capability with no way to express it is a +// trap for the next reader of this file, not a feature. type Shell struct { - Cmd string - Unless string // read-only guard; empty = no guard - Env Env // per-host variables, injected as $name (injection-safe) (#106) + Cmd string + Env Env // per-host variables, injected as $name (injection-safe) (#106) } func (s Shell) Name() string { return "shell" } @@ -24,17 +28,8 @@ func (s Shell) PreCheck() *Result { return nil } -// Guard: with an `unless`, exit 0 means the desired state already holds → skip. -func (s Shell) Guard(ex Executor) *Result { - if s.Unless == "" { - return nil // no guard → always run - } - if ex.Shell(s.Unless, s.Env).OK() { - r := Ok("alreadySatisfied") - return &r - } - return nil -} +// Guard: a raw shell has none — it always runs (#619). +func (s Shell) Guard(Executor) *Result { return nil } func (s Shell) Apply(ex Executor) Result { r := ex.Shell(s.Cmd, s.Env) diff --git a/internal/engine/shell_test.go b/internal/engine/shell_test.go index c281e2d..5457ad8 100644 --- a/internal/engine/shell_test.go +++ b/internal/engine/shell_test.go @@ -14,20 +14,9 @@ func TestShell_NoGuard_Runs(t *testing.T) { } } -func TestShell_UnlessSatisfied_Skips(t *testing.T) { - f := &fcFake{responses: map[string]ShellResult{netInspect: {Exit: 0}}} // guard satisfied - got := Run(Shell{Cmd: netCreate, Unless: netInspect}, f, Apply).String() - if got != "ok.alreadySatisfied" { - t.Fatalf("got %s, want ok.alreadySatisfied", got) - } - if f.calls[netCreate] { - t.Fatal("command ran despite the guard being satisfied") - } -} - func TestShell_Check_WouldNotMutate(t *testing.T) { - f := &fcFake{responses: map[string]ShellResult{netInspect: {Exit: 1}}} // guard not satisfied - got := Run(Shell{Cmd: netCreate, Unless: netInspect}, f, Check).String() + f := &fcFake{} + got := Run(Shell{Cmd: netCreate}, f, Check).String() if got != "would.ran" { t.Fatalf("got %s, want would.ran", got) } @@ -53,11 +42,3 @@ func TestShell_Apply_InjectsEnv(t *testing.T) { t.Fatalf("Apply must pass Env to the executor (#106), got %+v", f.gotEnv) } } - -func TestShell_Guard_InjectsEnv(t *testing.T) { - f := &envFake{result: ShellResult{Exit: 1}} // guard fails → command would run - Shell{Cmd: "act", Unless: "test -f $path", Env: Env{"path": "/tmp/x"}}.Guard(f) - if f.gotEnv["path"] != "/tmp/x" { - t.Fatalf("Guard must pass Env to the executor (#106), got %+v", f.gotEnv) - } -} diff --git a/internal/lang/bytes_equality_test.go b/internal/lang/bytes_equality_test.go index d68c347..7781a40 100644 --- a/internal/lang/bytes_equality_test.go +++ b/internal/lang/bytes_equality_test.go @@ -71,3 +71,73 @@ func TestBytes_InequalityIsRefusedToo(t *testing.T) { t.Fatal("`!=` on bytes must be refused as `==` is") } } + +// #616. #578 enumerated one uncomparable kind and left the others, which is why this came +// back: `engine.ShellResult` and `engine.Result` both carry slices or maps, so `a == b` +// over two of either **panicked the evaluator** rather than answering. +// +// The rule is now the other way round — `==` accepts the scalar kinds and refuses the rest +// by name — so a kind added later is refused, not fatal. Do not turn this back into a list +// of what is forbidden. + +func TestEquality_RefusesEveryUncomparableKind(t *testing.T) { + fetch := func(string, []byte, map[string]string) ([]byte, error) { return []byte("abc"), nil } + cases := map[string]struct{ src, want string }{ + "two shell results": { + src: `def t() { check { a = shell { true } b = shell { true } if a == b { return ok.same } return err.diff } }`, + want: "shell result", + }, + "two def results": { + src: `def helper() { check { return ok.done } } +def t() { check { a = helper() b = helper() if a == b { return ok.same } return err.diff } }`, + want: "outcome", + }, + // Answered false in silence before: a def author writing this is testing success and + // getting a condition that never fires. ADR-0010 says `if r`, and the message says so. + "a shell result against ok": { + src: `def t() { check { a = shell { true } if a == ok { return ok.same } return err.diff } }`, + want: "if r", + }, + "a shell result against a string": { + src: `def t() { check { a = shell { true } if a == "x" { return ok.same } return err.diff } }`, + want: "shell result", + }, + } + for what, c := range cases { + t.Run(what, func(t *testing.T) { + _, err := evalWithFetch(t, c.src, "t", map[string]string{}, fetch) + if err == nil { + t.Fatal("an uncomparable operand must be refused, not answered or crashed") + } + if !strings.Contains(err.Error(), c.want) { + t.Fatalf("the refusal must say what to do instead, got: %v", err) + } + }) + } +} + +// The scalars keep working — this is a whitelist, and it has to let the ordinary case +// through or every def stops parsing. +func TestEquality_ScalarsStillCompare(t *testing.T) { + src := `def t(k: str, n: str) { + check { + if k == "" { return err.empty } + if k == n { return ok.same } + if k != n { return ok.differ } + return err.unreachable + } +}` + for _, c := range []struct{ k, n, want string }{ + {"", "x", "err.empty"}, + {"a", "a", "ok.same"}, + {"a", "b", "ok.differ"}, + } { + res, err := evalWithFetch(t, src, "t", map[string]string{"k": c.k, "n": c.n}, nil) + if err != nil { + t.Fatalf("%q vs %q: %v", c.k, c.n, err) + } + if got := res.String(); got != c.want { + t.Fatalf("%q vs %q: got %s, want %s", c.k, c.n, got, c.want) + } + } +} diff --git a/internal/lang/eval.go b/internal/lang/eval.go index 385b92f..7ef857f 100644 --- a/internal/lang/eval.go +++ b/internal/lang/eval.go @@ -637,7 +637,7 @@ func (ev *evaluator) evalExpr(e Expr) value { return ev.evalField(x) case Binary: l, r := ev.evalExpr(x.L), ev.evalExpr(x.R) - ev.refuseBytesComparison(x.Op, l, r) + ev.refuseUncomparable(x.Op, l, r) eq := equal(l, r) if x.Op == "==" { return eq @@ -1206,25 +1206,40 @@ func truthy(v value) bool { // operands (#578). func equal(a, b value) bool { return a == b } -// refuseBytesComparison enforces ADR-0034 §4 at the one boundary that never enforced it. -// Bytes are opaque — they go from a primitive to an instruction and nowhere else — and the -// record says in as many words that they cannot be compared. `==` did it anyway, in two -// wrong ways: +// refuseUncomparable holds `==` and `!=` to the kinds that can answer them: a string, an +// int, a bool. Everything else is refused by name. // -// - two Bytes panicked the evaluator, since `a == b` over interfaces holding []byte is a -// runtime error rather than an answer (#578); -// - Bytes against a string answered **false** for any content, because Go compares the -// dynamic types first. Silently false is worse than a panic: it reads as "the contents -// differ", which is the shape of #411. +// A whitelist and not a list of forbidden kinds, deliberately. #578 enumerated `Bytes`, +// and #616 came back with the two it had left — `engine.ShellResult` and `engine.Result` +// both carry a map or a slice, so `a == b` over two of either is a **runtime panic**, not +// an answer. Listing what is refused means the next kind added panics again; listing what +// is allowed means it is refused. // -// The message names the one comparison that is honest — a digest of the content, which the -// caller can compute where the content lives. -func (ev *evaluator) refuseBytesComparison(op string, l, r value) { - _, lb := l.(Bytes) - _, rb := r.(Bytes) - if !lb && !rb { - return +// The refusals say what to write instead, because each wrong form has a right one: +// +// - bytes are opaque (ADR-0034 §4) — compare a digest where the content is; +// - a result is tested, not compared (ADR-0010) — `if r`, `if !r`, `r.exit == 0`. +// +// That second case used to answer **false**, silently, whatever the result: a def writing +// `if r == ok` was testing success and getting a condition that never fires. Silence is +// worse than the panic, since nothing shows it. +func (ev *evaluator) refuseUncomparable(op string, l, r value) { + for _, v := range []value{l, r} { + switch t := v.(type) { + case string, int, bool: + continue + case Bytes: + ev.fail("%s on bytes: content read by a primitive is opaque and cannot be compared "+ + "(ADR-0034 §4) — compare a digest computed where the content is, or hand the "+ + "bytes to an instruction", op) + case engine.ShellResult: + ev.fail("%s on a shell result: a shell result is tested, not compared (ADR-0010) — "+ + "write `if r` / `if !r`, or compare a field: `r.exit == 0`", op) + case engine.Result: + ev.fail("%s on an instruction's outcome: an outcome is tested, not compared "+ + "(ADR-0010) — write `if r`, or match it in a plan: `r == ok`, `r != err.tag`", op) + default: + ev.fail("%s on a value of an unsupported kind (%T)", op, t) + } } - ev.fail("%s on bytes: content read by a primitive is opaque and cannot be compared (ADR-0034 §4) — "+ - "compare a digest computed where the content is, or hand the bytes to an instruction", op) } diff --git a/internal/lang/shell_test.go b/internal/lang/shell_test.go index c3d479b..92b00f0 100644 --- a/internal/lang/shell_test.go +++ b/internal/lang/shell_test.go @@ -52,11 +52,12 @@ func TestParseShell_UnterminatedBlock(t *testing.T) { // #415: `unless { … }` inside a def parsed and was **silently ignored**. Measured: a def // doing `shell { touch "$dst" } unless { true }` created the file — the guard held, the // command ran anyway. The clause was stored in `ShellExpr.Unless` and nothing ever read -// it: `engine.Shell.Unless` is only ever filled from a plan step's argument, and plans +// it: `engine.Shell.Unless` was only ever filled from a plan step's argument, and plans // refuse the keyword outright. // // So it lived in exactly one place, where it did nothing. Refused now, with the message -// plans already give. +// plans already give — and the engine field itself is gone since #619, which leaves this +// refusal and the plan-side one as the only mentions of the word. func TestShell_UnlessInADefIsRefused(t *testing.T) { srcs := map[string]string{ "in an apply": `def t(p: str) { apply { shell { touch "$p" } unless { true } return ok.done } }`, diff --git a/internal/report/report.go b/internal/report/report.go index 4ef437a..a5fa677 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -74,13 +74,23 @@ func Redact(s string, secrets []string) string { // statusReport renders the per-host state report: one line per resource, with a // `current → desired` diff on each field that has drifted. Pure (returns the // text) so it is unit-testable without capturing stdout. -func Status(reports []orchestrator.BlockReport) string { +// Status renders the sweep and reports whether it failed, like Text and JSON — the second +// return is the point of #615: `status` used to decide its exit code from a helper that +// only inspected block errors, so a sweep where every host was unreachable printed +// `unreachable` on every line and exited 0. A monitor cannot tell that from a healthy +// fleet. +// +// Drift is deliberately **not** a failure: reporting what differs is what `status` is for, +// and a caller has to be able to tell "not converged" from "could not be reached". +func Status(reports []orchestrator.BlockReport) (string, bool) { var b strings.Builder + anyErr := false for _, blk := range reports { fmt.Fprintf(&b, "on %s:\n", blk.Target) // Block error and empty block, rendered as in reportText (#451). if blk.Err != nil { fmt.Fprintf(&b, " ! %v\n", blk.Err) + anyErr = true continue } if len(blk.Hosts) == 0 { @@ -90,6 +100,7 @@ func Status(reports []orchestrator.BlockReport) string { for _, h := range blk.Hosts { if h.Err != nil { fmt.Fprintf(&b, " %s: unreachable (%v)\n", h.Host, h.Err) + anyErr = true continue } fmt.Fprintf(&b, " %s:\n", h.Host) @@ -98,7 +109,7 @@ func Status(reports []orchestrator.BlockReport) string { } } } - return b.String() + return b.String(), anyErr } func statusStep(b *strings.Builder, s proto.StepResult, indent string) { diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 4f414b9..2221617 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -44,7 +44,7 @@ func TestStatusReport(t *testing.T) { {Host: "app2", Err: errFake("dial")}, }, }} - got := Status(reports) + got, _ := Status(reports) for _, want := range []string{ "on web:", " app1:", @@ -353,7 +353,7 @@ func TestRedactJSON_IgnoresEmptySecrets(t *testing.T) { // `status` renders block errors and empty blocks like `run` does (#451) — the paths that // only a status sweep reaches. func TestStatusReport_BlockErrorAndEmptyBlock(t *testing.T) { - text := Status([]orchestrator.BlockReport{ + text, _ := Status([]orchestrator.BlockReport{ {Target: "wbe", Err: &orchestrator.UnknownTargetError{Target: "wbe"}}, {Target: "spare"}, }) @@ -492,3 +492,57 @@ func TestStatusStep_ShapesByWhatTheStepIs(t *testing.T) { }) } } + +// #615. `status` exited 0 over a fleet where every host was unreachable: it asked +// `anyBlockError`, which answers about blocks, while the verdict it needed was the one +// `Text` and `JSON` already compute from the hosts. `Status` now returns it too, so the +// three renderers answer the same question the same way and the caller cannot pick the +// wrong one. +func TestStatus_ReportsAHostThatCouldNotBeReached(t *testing.T) { + cases := map[string]struct { + reports []orchestrator.BlockReport + failed bool + }{ + "an unreachable host": { + reports: []orchestrator.BlockReport{{ + Target: "web", + Hosts: []orchestrator.HostOutcome{{Host: "h1", Err: errFake("dial refused")}}, + }}, + failed: true, + }, + "an unknown target": { + reports: []orchestrator.BlockReport{ + {Target: "wbe", Err: &orchestrator.UnknownTargetError{Target: "wbe"}}, + }, + failed: true, + }, + // Drift is what `status` is for, not a failure: a field that differs must still + // exit 0, or a monitor cannot tell "unreachable" from "not converged". + "a host reporting drift": { + reports: []orchestrator.BlockReport{{ + Target: "web", + Hosts: []orchestrator.HostOutcome{{Host: "h1", Response: proto.Response{ + Results: []proto.StepResult{{Label: "dir.ensure(path=/opt)", Fields: []engine.FieldDiff{ + {Name: "present", Current: "false", Desired: "true"}, + }}}, + }}}, + }}, + failed: false, + }, + "a converged host": { + reports: []orchestrator.BlockReport{{ + Target: "web", + Hosts: []orchestrator.HostOutcome{{Host: "h1", Response: proto.Response{}}}, + }}, + failed: false, + }, + } + for what, c := range cases { + t.Run(what, func(t *testing.T) { + _, failed := Status(c.reports) + if failed != c.failed { + t.Fatalf("failed = %v, want %v", failed, c.failed) + } + }) + } +} diff --git a/internal/std/archive/archive.shellf b/internal/std/archive/archive.shellf index 37c2dfb..397f713 100644 --- a/internal/std/archive/archive.shellf +++ b/internal/std/archive/archive.shellf @@ -1,36 +1,48 @@ def extract(src: str, dst: str) { - # Observed: the archive's sha256, recorded in a sentinel under `dst` at extract - # time, equals the current archive's sha256. So a changed archive at `src` - # re-extracts, and an unchanged one is skipped (#259). Both hashes are computed - # target-side, since `src` is a target path. - # The sentinel alone was the whole observe, and it answers the wrong question: it says - # "this archive was extracted here once", not "its contents are here". Deleting what the - # archive delivered while leaving the marker reported `already` over an empty - # destination — #486 in another def (#594, #596). + # The sentinel under `dst` holds two things: the archive's sha256 on the first line, and + # one `sha256sum` line per member below it. # - # So both are asked: the sentinel first, because it is one `cat` and it is what makes a - # *changed* archive re-extract, then the members. Listing the archive costs a pass over - # it on every run; that is the price of an observe that describes the machine rather - # than its own bookkeeping. Directories are skipped — `tar` recreates them implicitly, - # and an archive need not carry entries for them. + # It started as the archive's digest alone, which answers "this archive was extracted + # here once" rather than "its contents are here": deleting what it delivered while + # leaving the marker reported `already` over an empty destination (#594/#596). Checking + # that every member *exists* fixed that much and no more — a member emptied in place + # still passed (#614). + # + # Recording the digests at extract time rather than recomputing them from the archive on + # every run is what makes the honest check affordable: an observe now reads the sentinel + # and hashes what is on disk, and **never opens the archive**. Listing it with `tar tzf` + # decompressed the whole thing on every run. + # + # The digests come from the archive's member list, not from a walk of `dst`: a file that + # was already there and is not in the archive stays out of the sentinel, so removing + # something `tar` never promised to manage does not trigger a re-extraction. observe { return state(extracted: shell { test -f "$dst/.shellf-archive-sha256" || exit 1 - [ "$(cat "$dst/.shellf-archive-sha256" 2>/dev/null)" = "$(sha256sum "$src" | cut -d' ' -f1)" ] || exit 1 - # No `exit` inside the loop: it runs in a subshell of the pipeline, where an - # exit would be lost. The absence is reported as output instead. - missing=$(tar tzf "$src" | while IFS= read -r m; do - case "$m" in */) continue ;; esac - [ -e "$dst/$m" ] || printf 'x' - done) - [ -z "$missing" ] + [ "$(head -n 1 "$dst/.shellf-archive-sha256")" = "$(sha256sum "$src" | cut -d' ' -f1)" ] || exit 1 + # An archive of directories only has no digest lines, and `sha256sum -c` on empty + # input fails with "no properly formatted checksum lines found" — which would + # make such an archive never converge. Nothing to verify is verified. + digests=$(tail -n +2 "$dst/.shellf-archive-sha256") + [ -z "$digests" ] || printf '%s\n' "$digests" | ( cd "$dst" && sha256sum -c --status - ) }.exit == 0) } apply { r = shell { mkdir -p "$dst" tar xzf "$src" -C "$dst" - sha256sum "$src" | cut -d' ' -f1 > "$dst/.shellf-archive-sha256" + # Written in one pass: the archive's digest, then a line per member. Directory + # entries are skipped — `tar` recreates them implicitly and an archive need not + # carry them — and a member the extraction did not produce is skipped rather + # than recorded as missing. + { + sha256sum "$src" | cut -d' ' -f1 + tar tzf "$src" | while IFS= read -r m; do + case "$m" in */) continue ;; esac + [ -f "$dst/$m" ] || continue + ( cd "$dst" && sha256sum "$m" ) + done + } > "$dst/.shellf-archive-sha256" } if !r { return err.runtime(r) } return ok.extracted @@ -53,7 +65,19 @@ def extract-member(src: str, dst: str, member: str) { apply { r = shell { mkdir -p "$(dirname "$dst")" - tar xzOf "$src" "$member" > "$dst" + # Staged, then renamed — `> "$dst"` truncates before tar writes a byte, so an + # archive that is corrupt or a member that is not in it left the destination + # empty, and what this def installs is usually an executable (#613). Measured: + # a missing member emptied a binary that was working. Same repair as #298 for + # `file.write` and #599 for `file.download`. + staged="$dst.shellf.$$" + trap 'rm -f "$staged"' EXIT + tar xzOf "$src" "$member" > "$staged" + # A redirection onto an existing file kept its mode; a rename does not. Carried + # over explicitly, or a second extraction drops the `+x` a plan set with + # `file.mode` — the regression #599 found the first time. + if [ -f "$dst" ]; then chmod "$(stat -c '%a' "$dst")" "$staged"; fi + mv -f "$staged" "$dst" } if !r { return err.runtime(r) } return ok.extracted diff --git a/internal/std/argument_guards_test.go b/internal/std/argument_guards_test.go index f8e6019..96c286b 100644 --- a/internal/std/argument_guards_test.go +++ b/internal/std/argument_guards_test.go @@ -61,3 +61,31 @@ func TestGuards_SudoWriteRefusesANameItCannotFile(t *testing.T) { }) } } + +// #617. `sshd.config` builds `/etc/ssh/sshd_config.d/.conf` from its argument, at +// three places, and checked nothing — while the two defs that do the same thing, +// `sudo.write` and `systemd.unit`, hold their name to a pattern. The def's own comment says +// twice that it is "the same shape as sudo.write": it copied the content validation and +// not the name one. +// +// What is at risk is the path. sshd reads `*.conf` from that directory, so a name with a +// `/` writes into a subdirectory nothing reads — a config the operator believes is +// installed and the server never sees. +func TestGuards_SshdConfigRefusesANameItCannotFile(t *testing.T) { + args := func(name string) map[string]string { + return map[string]string{"name": name, "content": "MaxAuthTries 4"} + } + for _, name := range []string{"", "hard ening", "../../etc/ssh/sshd_config", "sub/dir"} { + t.Run(name, func(t *testing.T) { + got := eval(t, "sshd.config", args(name), &fakeExec{observe: drift, apply: converged}, engine.Apply).String() + if got != "err.badName" { + t.Fatalf("name %q: got %s, want err.badName", name, got) + } + }) + } + // The drop-in convention must keep working: a leading number and a dash are what + // `50-hardening.conf` is made of. + if got := eval(t, "sshd.config", args("50-hardening"), &fakeExec{observe: converged}, engine.Apply).String(); got == "err.badName" { + t.Fatal("a conventional drop-in name must be accepted") + } +} diff --git a/internal/std/postgres/postgres.shellf b/internal/std/postgres/postgres.shellf index f281452..ef6d9ea 100644 --- a/internal/std/postgres/postgres.shellf +++ b/internal/std/postgres/postgres.shellf @@ -170,7 +170,13 @@ def config(key: str, value: str) as root { # # `k` is safe in a pattern: `check` restricts it to [A-Za-z_][A-Za-z0-9_]*. # `\x27` is a single quote, and mawk — Debian's awk — prints it as one. - awk 'BEGIN { k = ENVIRON["key"]; v = ENVIRON["value"] } + # A single quote inside the value is doubled, which is how postgres escapes one + # inside a quoted value. Written raw it closed the quote early — `key = \x27it\x27s\x27` + # — and the server refused the file at **startup**, so the failure surfaced on the + # next restart rather than on the run that caused it (#618). Verified against a + # real cluster: the doubled form restarts cleanly, and `pg_conftool -s show` + # returns the value unescaped, so the observe above still compares equal. + awk 'BEGIN { k = ENVIRON["key"]; v = ENVIRON["value"]; gsub(/\x27/, "\x27\x27", v) } $0 ~ "^" k "[ \t]*=" { if (!done) { printf "%s = \x27%s\x27\n", k, v; done = 1 } next } { print } END { if (!done) printf "%s = \x27%s\x27\n", k, v }' "$f" > "$staged" diff --git a/internal/std/sshd/sshd.shellf b/internal/std/sshd/sshd.shellf index 8b5d3ea..bc935e8 100644 --- a/internal/std/sshd/sshd.shellf +++ b/internal/std/sshd/sshd.shellf @@ -18,6 +18,14 @@ def config(name: str, content: str) as root { return state(synced: shell { test -f "$f" && printf '%s\n' "$content" | cmp -s - "$f" }.exit == 0, secured: shell { [ "$(stat -c '%a' "$f" 2>/dev/null)" = "600" ] }.exit == 0) } check { + # The name becomes a file under /etc/ssh/sshd_config.d, so it is held to what sshd + # will read back — the same check `sudo.write` runs, which this def described itself + # as sharing the shape of and did not have (#617). What is at risk is the path, not + # injection: sshd includes `*.conf` from that directory, so a name carrying a `/` + # writes into a subdirectory nothing reads, and the drop-in is silently never + # applied. `^[A-Za-z0-9_-]+$` still admits the convention — `50-hardening`. + if !~text.matches(name, "^[A-Za-z0-9_-]+$") { return err.badName } + r = shell { tmp=$(mktemp) trap 'rm -f "$tmp"' EXIT diff --git a/internal/transport/ssh.go b/internal/transport/ssh.go index c62b34e..570cb62 100644 --- a/internal/transport/ssh.go +++ b/internal/transport/ssh.go @@ -631,11 +631,24 @@ func (s SSH) authMethods() ([]ssh.AuthMethod, func(), error) { if sock := os.Getenv("SSH_AUTH_SOCK"); sock != "" { conn, err := net.Dial("unix", sock) - if err != nil { + switch { + case err != nil && len(methods) > 0: + // A dead socket removes a method; it does not fail a run that can already + // authenticate. `SSH_AUTH_SOCK` outlives the agent it names — a detached tmux, + // a closed session, an inherited variable — and discarding the inventory key + // over it made the same plan work in one terminal and not in another (#612). + // ADR-0026 §1 ranks `key:` first, "an explicit choice wins", and asks for an + // error only when neither method is available; that case is still below. + // + // Traced rather than silent: being invisible is what made this hard to + // attribute to the environment it comes from. + s.trace("skipping ssh-agent (%s): %v", sock, err) + case err != nil: return nil, noop, fmt.Errorf("connect ssh-agent (%s): %w", sock, err) + default: + methods = append(methods, ssh.PublicKeysCallback(agent.NewClient(conn).Signers)) + noop = func() { _ = conn.Close() } } - methods = append(methods, ssh.PublicKeysCallback(agent.NewClient(conn).Signers)) - noop = func() { _ = conn.Close() } } if len(methods) == 0 { diff --git a/internal/transport/ssh_test.go b/internal/transport/ssh_test.go index e6e3cea..1aff5ea 100644 --- a/internal/transport/ssh_test.go +++ b/internal/transport/ssh_test.go @@ -4,6 +4,7 @@ import ( "crypto/ed25519" "crypto/rand" "encoding/pem" + "fmt" "net" "os" "os/exec" @@ -318,3 +319,42 @@ func TestPosix_DeliversTheScriptVerbatim(t *testing.T) { t.Fatalf("the script must reach sh unchanged, got %q", out) } } + +// #612. A dead `SSH_AUTH_SOCK` — a detached tmux, a closed session, an inherited variable — +// discarded the inventory key that had already been loaded, and failed the run complaining +// about an agent the host never needed. ADR-0026 ranks `key:` first and only requires an +// error when *neither* method is available. +// +// The failure was environmental and invisible: same plan, same inventory, same machine, +// working in one terminal and not in another. +func TestAuthMethods_DeadAgentSocketKeepsTheKey(t *testing.T) { + t.Setenv("SSH_AUTH_SOCK", filepath.Join(t.TempDir(), "nope.sock")) + m, cleanup, err := (SSH{Key: writeKeyFile(t)}).authMethods() + if err != nil { + t.Fatalf("a dead agent must not discard a working key: %v", err) + } + defer cleanup() + if len(m) != 1 { + t.Fatalf("the key remains, the agent is dropped → 1 method, got %d", len(m)) + } +} + +// …and it says so, once, where a run with --trace can see it. Silence is what made the +// defect above hard to attribute. +func TestAuthMethods_DeadAgentSocketIsTraced(t *testing.T) { + sock := filepath.Join(t.TempDir(), "nope.sock") + t.Setenv("SSH_AUTH_SOCK", sock) + var traced []string + s := SSH{Key: writeKeyFile(t), Trace: func(format string, a ...any) { + traced = append(traced, fmt.Sprintf(format, a...)) + }} + if _, cleanup, err := s.authMethods(); err != nil { + t.Fatal(err) + } else { + cleanup() + } + joined := strings.Join(traced, "\n") + if !strings.Contains(joined, sock) { + t.Fatalf("the skipped agent must be named in the trace, got: %q", joined) + } +} diff --git a/test/e2e/adverse-cases.md b/test/e2e/adverse-cases.md index e15d35d..11b2e2c 100644 --- a/test/e2e/adverse-cases.md +++ b/test/e2e/adverse-cases.md @@ -60,7 +60,7 @@ The third is the one `coverage.shellf` can never produce, because it only ever b from an empty target. Its cases are built by hand: a `.env` holding the wanted line **and** a stale duplicate, an archive's destination emptied with its sentinel left behind, a database that exists under the wrong owner, two logins where one is a regex match of the -other. Each was verified to fail before the fix and pass after — a case of this kind that +other, an archive member emptied in place. Each was verified to fail before the fix and pass after — a case of this kind that was never seen red proves nothing at all, since a weak observe passes it by construction. An argument case passes a path holding a space, a single quote and a `&`, or a name at a diff --git a/test/e2e/plans/adverse-archive.extract-emptied.shellf b/test/e2e/plans/adverse-archive.extract-emptied.shellf new file mode 100644 index 0000000..f395e70 --- /dev/null +++ b/test/e2e/plans/adverse-archive.extract-emptied.shellf @@ -0,0 +1,31 @@ +# `archive.extract` over a destination whose member was **emptied in place** (#614). +# +# #594 widened the observe from "the sentinel is there" to "every member is there", which +# is the difference between believing your own bookkeeping and looking at the machine. It +# stopped at existence: a file truncated where it stands keeps its name, so the def still +# reported `already` over content that is gone. +# +# The state is right-shaped and wrong — the third kind of adverse case (adverse-cases.md). +on target { + as root { + unsafe shell { + rm -rf /tmp/adv-extract-emptied + mkdir -p /tmp/adv-extract-emptied/src/inner + printf 'one\n' > /tmp/adv-extract-emptied/src/inner/one.txt + printf 'two\n' > /tmp/adv-extract-emptied/src/two.txt + tar czf /tmp/adv-extract-emptied/a.tar.gz -C /tmp/adv-extract-emptied/src . + } + + archive.extract("/tmp/adv-extract-emptied/a.tar.gz", "/tmp/adv-extract-emptied/dst") + + # The file keeps its name and loses its content: the shape existence cannot see. + unsafe shell { : > /tmp/adv-extract-emptied/dst/two.txt } + + archive.extract("/tmp/adv-extract-emptied/a.tar.gz", "/tmp/adv-extract-emptied/dst") + + shell { + grep -qx 'two' /tmp/adv-extract-emptied/dst/two.txt || exit 1 + grep -qx 'one' /tmp/adv-extract-emptied/dst/inner/one.txt || exit 1 + } + } +} diff --git a/test/e2e/run.sh b/test/e2e/run.sh index e9df7f6..5001be2 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -1443,4 +1443,73 @@ docker exec -u deploy "$cname" grep -qx 'the new content' /tmp/dl-dst.bin \ docker exec -u deploy "$cname" sh -c '[ "$(stat -c "%a" /tmp/dl-dst.bin)" = "700" ]' \ || fail "a download must keep the destination's mode — a staged rename drops it (#599)" -say "PASS — check inert, apply provisioned, re-apply idempotent, status converged, allow-list held, defs declare nothing, bridge relaunched, every def exercised, examples run, remote module used, changed source re-delivered, shell rules enforced, converged previews honest, delete-only reported, foreign agent refused, weak observes fixed, delivery atomic, asset links contained, escalated transfer honoured, links never carry a write out, booleans are booleans, dry-run diffs a change, commands are reported, purged packages reinstalled, defs survive a hostile state, dir.owner sees a missing path, ufw converges while down, a malformed unit is refused, a bad hash leaves the destination alone" +say "29. a failed member extraction leaves the destination untouched (#613)" +# `archive.extract-member` redirected `tar xzO` straight at its destination, so a member +# that is not in the archive emptied the file that was there — and what this def installs is +# usually an executable. Same defect as #298 (file.write) and #599 (file.download), a third +# time. Asserted on the machine: the verdict was already correct. +mkdir -p "$work/mem/plans" "$work/mem/inventories" "$work/mem/assets" "$work/mem/defs" +cp "$work/inventory.shellf" "$work/mem/inventories/inv.shellf" +# As `deploy`, like every plan the harness runs (#591). +docker exec -u deploy "$cname" sh -c ' + rm -rf /tmp/mem29 && mkdir -p /tmp/mem29/src + printf "the member\n" > /tmp/mem29/src/real.txt + tar czf /tmp/mem29/a.tar.gz -C /tmp/mem29/src . + printf "the previous binary\n" > /tmp/mem29/installed.bin + chmod 750 /tmp/mem29/installed.bin' +cat > "$work/mem/plans/plan.shellf" <<'EOF' +on target { + e = archive.extract-member("/tmp/mem29/a.tar.gz", "/tmp/mem29/installed.bin", "./absent.txt")? + if e == err.runtime { + shell { + grep -qx 'the previous binary' /tmp/mem29/installed.bin || exit 1 + [ "$(stat -c '%a' /tmp/mem29/installed.bin)" = "750" ] || exit 1 + ls /tmp/mem29/installed.bin.shellf.* >/dev/null 2>&1 && exit 1 + exit 0 + } + } + archive.extract-member("/tmp/mem29/a.tar.gz", "/tmp/mem29/installed.bin", "./real.txt") + shell { + grep -qx 'the member' /tmp/mem29/installed.bin || exit 1 + [ "$(stat -c '%a' /tmp/mem29/installed.bin)" = "750" ] || exit 1 + } +} +EOF +out="$("$work/shellf" run --inventory "$work/mem/inventories/inv.shellf" --insecure \ + "$work/mem/plans/plan.shellf" 2>&1)" || fail "the extract-member step failed:\n$out" +printf '%s\n' "$out" +docker exec -u deploy "$cname" grep -qx 'the member' /tmp/mem29/installed.bin \ + || fail "a successful extraction must land" +docker exec -u deploy "$cname" sh -c '[ "$(stat -c "%a" /tmp/mem29/installed.bin)" = "750" ]' \ + || fail "extract-member must keep the destination's mode — a staged rename drops it (#613)" + +say "30. a quote in a postgres value does not break the server (#618)" +# `postgres.config` writes `key = \x27value\x27`, so a value carrying a single quote closed +# the quote early and the cluster refused the file — at **startup**, which means on the next +# restart rather than on the run that caused it. The restart below is the assertion: a file +# postgres will not read is a file that stops it coming back. +mkdir -p "$work/pgq/plans" "$work/pgq/inventories" "$work/pgq/assets" "$work/pgq/defs" +cp "$work/inventory.shellf" "$work/pgq/inventories/inv.shellf" +cat > "$work/pgq/plans/plan.shellf" <<'EOF' +on target { + as root { + postgres.config("application_name", "it's a test") + unsafe shell { pg_ctlcluster $(ls /etc/postgresql) main restart } + shell { + grep -q "^application_name = 'it''s a test'$" \ + /etc/postgresql/$(ls /etc/postgresql)/main/postgresql.conf + } + } +} +EOF +out="$("$work/shellf" run --inventory "$work/pgq/inventories/inv.shellf" --insecure \ + "$work/pgq/plans/plan.shellf" 2>&1)" || fail "a quoted value must be written and accepted:\n$out" +printf '%s\n' "$out" +# And it converges: `pg_conftool -s show` returns the value unescaped, so the observe must +# compare equal to what the caller passed rather than to the doubled form. +out="$("$work/shellf" run --inventory "$work/pgq/inventories/inv.shellf" --insecure \ + "$work/pgq/plans/plan.shellf" 2>&1)" || fail "the second run failed:\n$out" +printf '%s' "$out" | grep -q 'postgres.config.*already' \ + || { printf '%s\n' "$out"; fail "a quoted value must converge on a second run (#618)"; } + +say "PASS — check inert, apply provisioned, re-apply idempotent, status converged, allow-list held, defs declare nothing, bridge relaunched, every def exercised, examples run, remote module used, changed source re-delivered, shell rules enforced, converged previews honest, delete-only reported, foreign agent refused, weak observes fixed, delivery atomic, asset links contained, escalated transfer honoured, links never carry a write out, booleans are booleans, dry-run diffs a change, commands are reported, purged packages reinstalled, defs survive a hostile state, dir.owner sees a missing path, ufw converges while down, a malformed unit is refused, a bad hash leaves the destination alone, a failed member extraction leaves it too, a quoted postgres value is accepted"