Consolidated follow-up for review findings on #2436 and #2448, both merged. Nothing here is a runtime defect and none of it reaches a released artifact before v0.21 cuts.
Grouped by area. Items 1–5 come from #2436, items 6–11 from #2448 (several raised independently by two reviewers).
Docs: migration guidance names a command that does not exist
1. Four published-doc locations and one test comment tell users to pass an overlay with aicr recipe -r. The recipe command has no --recipe/-r flag — the CLI surface baseline committed in #2436 shows --snapshot,-s and no --recipe under aicr recipe — and a live probe returns flag provided but not defined: -r. The flag exists on bundle, validate, mirror list, and evidence digest.
This is the remediation path for exactly the users the #2421 change breaks, which makes it the most urgent item here.
docs/user/deprecations.mddocs/user/api-reference.mddocs/integrator/data-extension.mddocs/design/022-artifact-maturity-and-deprecation.mdpkg/recipe/loader_test.go (comment)
Replace with aicr bundle -r and aicr validate -r. Search by text rather than line number — the docs have shifted since the merge.
CLI surface gate (pkg/cli/surface_test.go)
2. A required flag added to an existing command is reported as compatible. diffLines routes every new baseline line into the additive bucket, including lines carrying required=true, and the failure text reads "Additions are compatible. Regenerate the golden." Adding a required flag to an existing command makes previously valid invocations fail, which the policy table in RELEASING.md classifies as breaking — only a new flag whose default preserves behavior is additive. A required flag on an entirely new command remains additive, so the check must be scoped to commands already present in the baseline.
3. The baseline omits framework-injected surface. renderSurface walks RootCommand() before urfave/cli v3.11.0 performs setup, so none of the injected surface reaches the golden: the completion command and its bash, zsh, fish, and pwsh subcommands, --help across the command tree, and root --version. root.go sets EnableShellCompletion: true and then uses ConfigureShellCompletionCommand to un-hide the completion command and give it a category, so it is deliberately public surface. Removing any of it would pass the gate silently. Render from a post-setup tree; enumerating specific injected entries would close the gap only partly.
Both are gaps in a gate added to prevent this exact class of drift, so they are worth closing before the gate accumulates trust it has not earned.
Deprecation channel (pkg/deprecation)
4. The empty-subject assertion cannot detect the regression it guards. In deprecation_test.go, the condition requires both that the malformed message is present and that --real-flag is absent, but the preceding call always logs --real-flag, so that branch is unreachable. The newline anchor is a second problem: slog's text handler does not place a newline immediately after the message, because structured attributes intervene. The assertion below still catches a suppressed real warning, so the test is not entirely inert — but the regression it is named for would pass. The production guard is correct. Assert that the message appears exactly once.
5. Reconcile the early closure with the central policy. Rejecting a headerless RecipeMetadata on the direct-input path is reachable for externally authored overlays, while the deprecation policy added in the same commit requires two minor releases of notice before a pre-1.0 breaking removal. The decision is documented — ADR-022 records the early closure and its rationale, and the deprecations ledger calls it an early narrowing — so this is not undisclosed behavior. What is missing is reconciliation: RELEASING.md should classify it explicitly as the one-time v0.21 exception the ADR and ledger already imply.
API server conformance (pkg/server)
6. getOnly returns a plain-text 405 while every other 405 in the package returns structured JSON. It calls http.Error(w, "method not allowed", 405). Seven existing call sites — including handleHealth and handleReady, two functions above it in the same file — use WriteError(w, r, http.StatusMethodNotAllowed, aicrerrors.ErrCodeMethodNotAllowed, "Method not allowed", false, map[string]any{keyMethod: r.Method}), which also sets the Allow header the helper sets by hand. The JSON envelope is the documented convention (docs/contributor/api-server.md). A client parsing it gets JSON from /health and a bare string from /metrics for the identical condition. Flagged independently by two reviewers on #2448.
7. The route set is hand-maintained, so a future direct-mux route escapes all three conformance tests. registeredPaths derives "routes served" from s.config.Handlers plus the hand-maintained systemRoutes list, not from the real mux. A future mux.HandleFunc("/debug", …) added directly in New() — the exact place system routes are already wired — would appear in none of the three sources these tests consult and would be invisible to every check. This is a hole in a gate built to catch undocumented endpoints.
8. "not 405" is a weak acceptance oracle. TestOpenAPISpecMethodsAreAccepted fails only on rec.Code == 405, so a declared operation whose handler panics (500 via panicRecoveryMiddleware) or unconditionally 500s still passes. The narrowness is deliberate and well argued in the docstring — asserting success codes would make it a fixture treadmill — but the test name over-promises relative to what it verifies. Either tighten the oracle or rename it.
9. HEAD rejection is now an asserted contract; decide it deliberately. TestOpenAPIUndeclaredMethodsAreRejected requires 405 for every undeclared method on every registered path, so HEAD is pinned as rejected on /, /health, /ready, /metrics, and every v1 and v2 GET endpoint. RFC 9110 §9.1 says general-purpose servers must support GET and HEAD. The rejection is pre-existing on /health and /ready, so #2448 introduced no regression — but it turned a wart into an invariant. Declaring head: alongside get: in the spec would give the same alignment without standing against a MUST; if the deviation is intended, record it in the test's doc comment so the next reader does not "fix" it.
10. Latent: specOperations misreads a $ref-only or parameters-only path item as "declares no operations." Both are legal OpenAPI and yield zero method keys, tripping the len(ops[path]) == 0 failure. All 10 current spec paths use inline get/post, so this is latent until someone refactors the spec.
11. Nit: the spec path is relative to the go test working directory. ../../api/aicr/v1/server.yaml resolves via the per-package CWD that go test sets, which would break under go test -c, Bazel, or a CWD-changing harness. The repo has no such lane today, so this is a non-issue for the current setup.
Suggested sequencing
Item 1 first and on its own if convenient — it is user-facing, lands in published docs at v0.21, and is a text-only change. Items 2, 3, 6, and 7 are the substantive ones. Items 5 and 9 are decisions to record rather than code to write. Items 10 and 11 need no action until the conditions that make them real arrive.
Context
Consolidated follow-up for review findings on #2436 and #2448, both merged. Nothing here is a runtime defect and none of it reaches a released artifact before v0.21 cuts.
Grouped by area. Items 1–5 come from #2436, items 6–11 from #2448 (several raised independently by two reviewers).
Docs: migration guidance names a command that does not exist
1. Four published-doc locations and one test comment tell users to pass an overlay with
aicr recipe -r. Therecipecommand has no--recipe/-rflag — the CLI surface baseline committed in #2436 shows--snapshot,-sand no--recipeunderaicr recipe— and a live probe returnsflag provided but not defined: -r. The flag exists onbundle,validate,mirror list, andevidence digest.This is the remediation path for exactly the users the #2421 change breaks, which makes it the most urgent item here.
docs/user/deprecations.mddocs/user/api-reference.mddocs/integrator/data-extension.mddocs/design/022-artifact-maturity-and-deprecation.mdpkg/recipe/loader_test.go(comment)Replace with
aicr bundle -randaicr validate -r. Search by text rather than line number — the docs have shifted since the merge.CLI surface gate (
pkg/cli/surface_test.go)2. A required flag added to an existing command is reported as compatible.
diffLinesroutes every new baseline line into the additive bucket, including lines carryingrequired=true, and the failure text reads "Additions are compatible. Regenerate the golden." Adding a required flag to an existing command makes previously valid invocations fail, which the policy table inRELEASING.mdclassifies as breaking — only a new flag whose default preserves behavior is additive. A required flag on an entirely new command remains additive, so the check must be scoped to commands already present in the baseline.3. The baseline omits framework-injected surface.
renderSurfacewalksRootCommand()before urfave/cli v3.11.0 performs setup, so none of the injected surface reaches the golden: thecompletioncommand and itsbash,zsh,fish, andpwshsubcommands,--helpacross the command tree, and root--version.root.gosetsEnableShellCompletion: trueand then usesConfigureShellCompletionCommandto un-hide the completion command and give it a category, so it is deliberately public surface. Removing any of it would pass the gate silently. Render from a post-setup tree; enumerating specific injected entries would close the gap only partly.Both are gaps in a gate added to prevent this exact class of drift, so they are worth closing before the gate accumulates trust it has not earned.
Deprecation channel (
pkg/deprecation)4. The empty-subject assertion cannot detect the regression it guards. In
deprecation_test.go, the condition requires both that the malformed message is present and that--real-flagis absent, but the preceding call always logs--real-flag, so that branch is unreachable. The newline anchor is a second problem: slog's text handler does not place a newline immediately after the message, because structured attributes intervene. The assertion below still catches a suppressed real warning, so the test is not entirely inert — but the regression it is named for would pass. The production guard is correct. Assert that the message appears exactly once.5. Reconcile the early closure with the central policy. Rejecting a headerless
RecipeMetadataon the direct-input path is reachable for externally authored overlays, while the deprecation policy added in the same commit requires two minor releases of notice before a pre-1.0 breaking removal. The decision is documented — ADR-022 records the early closure and its rationale, and the deprecations ledger calls it an early narrowing — so this is not undisclosed behavior. What is missing is reconciliation:RELEASING.mdshould classify it explicitly as the one-time v0.21 exception the ADR and ledger already imply.API server conformance (
pkg/server)6.
getOnlyreturns a plain-text 405 while every other 405 in the package returns structured JSON. It callshttp.Error(w, "method not allowed", 405). Seven existing call sites — includinghandleHealthandhandleReady, two functions above it in the same file — useWriteError(w, r, http.StatusMethodNotAllowed, aicrerrors.ErrCodeMethodNotAllowed, "Method not allowed", false, map[string]any{keyMethod: r.Method}), which also sets theAllowheader the helper sets by hand. The JSON envelope is the documented convention (docs/contributor/api-server.md). A client parsing it gets JSON from/healthand a bare string from/metricsfor the identical condition. Flagged independently by two reviewers on #2448.7. The route set is hand-maintained, so a future direct-mux route escapes all three conformance tests.
registeredPathsderives "routes served" froms.config.Handlersplus the hand-maintainedsystemRouteslist, not from the real mux. A futuremux.HandleFunc("/debug", …)added directly inNew()— the exact place system routes are already wired — would appear in none of the three sources these tests consult and would be invisible to every check. This is a hole in a gate built to catch undocumented endpoints.8. "not 405" is a weak acceptance oracle.
TestOpenAPISpecMethodsAreAcceptedfails only onrec.Code == 405, so a declared operation whose handler panics (500 viapanicRecoveryMiddleware) or unconditionally 500s still passes. The narrowness is deliberate and well argued in the docstring — asserting success codes would make it a fixture treadmill — but the test name over-promises relative to what it verifies. Either tighten the oracle or rename it.9. HEAD rejection is now an asserted contract; decide it deliberately.
TestOpenAPIUndeclaredMethodsAreRejectedrequires 405 for every undeclared method on every registered path, so HEAD is pinned as rejected on/,/health,/ready,/metrics, and every v1 and v2 GET endpoint. RFC 9110 §9.1 says general-purpose servers must support GET and HEAD. The rejection is pre-existing on/healthand/ready, so #2448 introduced no regression — but it turned a wart into an invariant. Declaringhead:alongsideget:in the spec would give the same alignment without standing against a MUST; if the deviation is intended, record it in the test's doc comment so the next reader does not "fix" it.10. Latent:
specOperationsmisreads a$ref-only orparameters-only path item as "declares no operations." Both are legal OpenAPI and yield zero method keys, tripping thelen(ops[path]) == 0failure. All 10 current spec paths use inlineget/post, so this is latent until someone refactors the spec.11. Nit: the spec path is relative to the
go testworking directory.../../api/aicr/v1/server.yamlresolves via the per-package CWD thatgo testsets, which would break undergo test -c, Bazel, or a CWD-changing harness. The repo has no such lane today, so this is a non-issue for the current setup.Suggested sequencing
Item 1 first and on its own if convenient — it is user-facing, lands in published docs at v0.21, and is a text-only change. Items 2, 3, 6, and 7 are the substantive ones. Items 5 and 9 are decisions to record rather than code to write. Items 10 and 11 need no action until the conditions that make them real arrive.
Context
54f31b4fc, withb6b242026already resolving three findings from its approving review6452555b9