Data

{
  "owner": "mirendev",
  "repo": "runtime",
  "number": 972,
  "verdict": "ready",
  "event": "comment",
  "summary": "This draft is ready to graduate to human review. The change does exactly what it says: promotes the `sagas` feature flag to on-by-default and keeps `-sagas` as a documented escape hatch, with a clear follow-up ticket (MIR-1460) to remove it entirely.\n\n**What I looked at carefully:**\n\n**The GA flip itself** (`features.yaml` → `labs.gen.go`) is a one-line default change that the codegen faithfully reflects. `TestSagasEnabledByDefault` pins both halves of the contract — no flags needed to get sagas, `-sagas` alone opts back out — and the comment pointing to MIR-1460 makes the intent durable.\n\n**The `Init` logging overhaul** is a real improvement. The old per-flag `if log != nil` guard (which allowed callers to pass `nil`) is replaced by a required non-nil logger — callers that want silence now pass `slog.New(slog.DiscardHandler)`, which is explicit and correct. The new end-of-init summary log records every feature's final state, not just the ones explicitly named on the command line; that's exactly the right call when features start defaulting to `true`. The `sort.Strings` call before building the log attrs gives stable, predictable output.\n\n**The `ContainerSpec` adapter bug fix** in `sandbox_ops.go` is the most substantive correctness fix here. The saga's `bootTask` action was calling `container.Spec(ctx)` directly on an un-namespaced context; containerd looks up the container record using the namespace on the context, so this only worked in production because the server's containerd client happened to have a default namespace configured. The fix routes the call through `sandboxOps.ContainerSpec`, which applies `namespaces.WithNamespace` first. The test in `sandbox_ops_test.go` enforces this by having the `sagaMockContainer.Spec` actually check the namespace and return `errdefs.ErrFailedPrecondition` if it's missing — that's a meaningful, not a trivial, assertion.\n\n**The `cli.go` nil-logger cleanup** replaces the old `labs.Init(nil, ...)` call with `labs.Init(slog.New(slog.DiscardHandler), ...)` and adds a comment explaining why the CLI discards: the server logs the same summary properly at startup.\n\n**Test coverage** is good: `TestSagasEnabledByDefault`, `TestSandboxOpsContainerSpecCarriesNamespace`, and the existing saga harness tests (happy path + every undo path) all run against the changed code paths. The `mockContainerRuntime.ContainerSpec` impl in the test file explicitly namespaces the context before calling `cont.Spec`, mirroring what `sandboxOps` does in production, so the tests aren't accidentally passing for the wrong reason.\n\n**One minor observation worth naming but not blocking:** `mockContainerRuntime.ContainerSpec` in `create_saga_test.go` (line 189–191) applies a hardcoded `\"miren-test\"` namespace and then calls `cont.Spec`. This means the mock is doing work that's really the responsibility of `sandboxOps` — but because `SandboxContainerRuntime` is an interface the test controls, this is intentional and correct for these unit-level saga tests. The `sandbox_ops_test.go` separately pins that `sandboxOps.ContainerSpec` itself does the namespacing, so the separation of concerns is fine.\n\nEverything compiles consistently (the generated `labs.gen.go` matches the template in `labsgen/main.go`), the docs and changelog are updated, and there are no leftover debug statements or TODOs to address before merge.",
  "posted_to_pr": true,
  "draft": true
}