amadan

carlos / platform Public

deploy: flatten the channels-door pipeline envelope to pipeline_entry/pipeline_channels

Review fix round 1 (Task 11): the /channels door's "pipeline" key
collided with the /pipeline door's own "pipeline" key naming an
incompatible shape (a flat {entry, channels: [strings]} summary vs the
full signed release.Pipeline object) — same JSON path, object on one
door, string-bearing summary on the other. Flattened to two additive
top-level keys, pipeline_entry/pipeline_channels, mirroring the client's
apiChannelsResult fields exactly. Both keys are now omitted together for
BOTH a legacy app and an unreadable pipeline — the null-vs-omitted
distinction the door used to carry is dropped by design; the /pipeline
door remains the sole authority on that distinction, untouched.

Updated: internal/console/apicli.go (handleAPIChannels), apimode.go
(channels decode), and the covering tests in apicli_test.go/apimode_test.go.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev b47a43ad069f4598fadffeffa4fc8bcd4361501f parent 3f2e340
4 files changed, +110 −91
  • apimode.go +19 −17
  • apimode_test.go +18 −17
  • internal/console/apicli.go +32 −24
  • internal/console/apicli_test.go +41 −33
diff --git a/apimode.go b/apimode.go
index 08f3090b4..8ae180e29 100644
--- a/apimode.go
+++ b/apimode.go
@@ -327,8 +327,8 @@ type apiChannel struct {
// whole shape over the wire a second time.
//
// PipelineEntry == "" means "no pipeline" — a legacy app on the implicit
-// edge/beta/stable ladder, OR a console the pipeline read failed against
-// (handleAPIChannels degrades a read failure to omitting the key rather
+// edge/beta/stable ladder, OR a console whose pipeline read failed
+// (handleAPIChannels omits both pipeline_entry/pipeline_channels rather
// than failing the listing; this client-side type does not need to tell
// the two apart, since either way there is no entry channel to fall back
// to and the caller's existing no-pipeline behaviour is exactly right).
@@ -1080,31 +1080,33 @@ func (s *apiSession) channels(ctx context.Context, a apiAcct, app string) (apiCh
Rung string `json:"rung"`
Error string `json:"error"`
} `json:"channels"`
- // Pipeline is nil for both "no pipeline key at all" (an older
- // console) and an explicit JSON null (a confirmed-legacy app) —
- // this client has no need to tell those two apart; see
- // apiChannelsResult's doc comment. Its shape is the REDUCED view
- // handleAPIChannels sends (apicli.go), not the full release.Pipeline
- // object `carlos pipeline` reads.
- Pipeline *struct {
- Entry string `json:"entry"`
- Channels []string `json:"channels"`
- } `json:"pipeline"`
+ // PipelineEntry/PipelineChannels are two flat, top-level keys — NOT
+ // nested under a "pipeline" object — deliberately distinct from the
+ // full release.Pipeline object `carlos pipeline` reads under that
+ // name (review fix round 1: the same JSON key meaning an object on
+ // one door and a string on another was a trap for any future
+ // consumer). Both are absent together for BOTH "no pipeline key at
+ // all" (an older console) and "this app has no pipeline" (a
+ // confirmed-legacy app) AND a pipeline read failure server-side —
+ // this client has no need to tell those apart; see
+ // apiChannelsResult's doc comment.
+ PipelineEntry string `json:"pipeline_entry"`
+ PipelineChannels []string `json:"pipeline_channels"`
}
if err := json.Unmarshal(body, &res); err != nil {
return apiChannelsResult{}, fmt.Errorf("channels: unreadable response: %w", err)
}
- out := apiChannelsResult{Channels: make([]apiChannel, len(res.Channels))}
+ out := apiChannelsResult{
+ Channels: make([]apiChannel, len(res.Channels)),
+ PipelineEntry: res.PipelineEntry,
+ PipelineChannels: res.PipelineChannels,
+ }
for i, ch := range res.Channels {
out.Channels[i] = apiChannel{
Channel: ch.Channel, Version: ch.Version,
PromotedAt: ch.PromotedAt, Rung: ch.Rung, Err: ch.Error,
}
}
- if res.Pipeline != nil {
- out.PipelineEntry = res.Pipeline.Entry
- out.PipelineChannels = res.Pipeline.Channels
- }
return out, nil
}
diff --git a/apimode_test.go b/apimode_test.go
index 1368be30b..ee9d985e4 100644
--- a/apimode_test.go
+++ b/apimode_test.go
@@ -608,29 +608,30 @@ func TestAPISessionChannels(t *testing.T) {
if chans[1].Channel != "broken" || chans[1].Err != "signature mismatch" {
t.Fatalf("row 1 = %+v", chans[1])
}
- // No "pipeline" key at all (an older console, or handleAPIChannels'
- // own degrade-on-read-failure branch): PipelineEntry stays "" — the
- // same zero value a confirmed-legacy null decodes to, since this
- // client has no need to tell the two apart (apiChannelsResult's doc
- // comment).
+ // Neither "pipeline_entry" nor "pipeline_channels" present at all (an
+ // older console, a legacy app, or handleAPIChannels' own
+ // degrade-on-read-failure branch — this client has no need to tell
+ // those apart, apiChannelsResult's doc comment): both decode to their
+ // zero value.
if res.PipelineEntry != "" || len(res.PipelineChannels) != 0 {
- t.Fatalf("no pipeline key = %+v, want a zero-value pipeline", res)
+ t.Fatalf("no pipeline keys = %+v, want a zero-value pipeline", res)
}
}
// TestAPISessionChannelsPipelineEnvelope pins the client-side decode of the
-// additive "pipeline" envelope key (release pipelines v2, Task 11):
-// PipelineEntry is the pipeline's first channel, PipelineChannels is every
-// channel in order — and an explicit JSON null (the confirmed-legacy shape
-// handleAPIChannels answers) decodes to the same zero value as the key
-// being absent altogether.
+// additive "pipeline_entry"/"pipeline_channels" flat top-level keys (release
+// pipelines v2, Task 11, review fix round 1 — NOT nested under a "pipeline"
+// key, since that JSON path already names the full release.Pipeline object
+// on the `/pipeline` door): PipelineEntry/PipelineChannels decode from the
+// two flat fields when present, and — both keys absent, the shape a legacy
+// app OR an unreadable-pipeline app both get, this client has no need to
+// tell them apart — decode to the same zero value.
func TestAPISessionChannelsPipelineEnvelope(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewEncoder(w).Encode(map[string]any{
- "channels": []map[string]any{},
- "pipeline": map[string]any{
- "entry": "edge", "channels": []string{"edge", "production"},
- },
+ "channels": []map[string]any{},
+ "pipeline_entry": "edge",
+ "pipeline_channels": []string{"edge", "production"},
})
}))
defer ts.Close()
@@ -648,7 +649,7 @@ func TestAPISessionChannelsPipelineEnvelope(t *testing.T) {
}
ts2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- json.NewEncoder(w).Encode(map[string]any{"channels": []map[string]any{}, "pipeline": nil})
+ json.NewEncoder(w).Encode(map[string]any{"channels": []map[string]any{}})
}))
defer ts2.Close()
s2 := newTestSession(ts2.URL)
@@ -657,7 +658,7 @@ func TestAPISessionChannelsPipelineEnvelope(t *testing.T) {
t.Fatalf("channels: %v", err)
}
if res2.PipelineEntry != "" || len(res2.PipelineChannels) != 0 {
- t.Fatalf("explicit null pipeline = %+v, want the zero value", res2)
+ t.Fatalf("both keys absent = %+v, want the zero value", res2)
}
}
diff --git a/internal/console/apicli.go b/internal/console/apicli.go
index 7ccc50ba1..8c9de35cc 100644
--- a/internal/console/apicli.go
+++ b/internal/console/apicli.go
@@ -512,8 +512,9 @@ func (s *Server) handleAPIApps(w http.ResponseWriter, r *http.Request) {
// one bad pointer must not hide every other channel's answer from the
// caller.
//
-// The response also carries an ADDITIVE "pipeline" envelope key (release
-// pipelines v2, Task 11) — see the comment at its assignment below.
+// The response also carries two ADDITIVE "pipeline_entry"/"pipeline_channels"
+// keys (release pipelines v2, Task 11) — see the comment at their assignment
+// below.
func (s *Server) handleAPIChannels(w http.ResponseWriter, r *http.Request) {
_, acct, app, ok := s.apiAccountApp(w, r)
if !ok {
@@ -548,35 +549,42 @@ func (s *Server) handleAPIChannels(w http.ResponseWriter, r *http.Request) {
})
}
resp := map[string]any{"channels": out}
- // pipeline: an ADDITIVE envelope key (release pipelines v2, Task 11) —
- // old CLIs decode "channels" and ignore the rest. It is a REDUCED view
- // of the pipeline — {"entry", "channels": [names in order]} — not the
- // full signed release.Pipeline object `carlos pipeline` GETs (Task 6);
- // this door answers "where does a deploy land and what's the order",
- // nothing about bake/rules, so it need not carry the wider object over
- // the wire a second time. The LOAD, though, is shared with
- // handleAPIPipelineGet verbatim (release.GetPipeline) so the two doors
- // can never disagree about what "this app's pipeline" is.
+ // pipeline_entry/pipeline_channels: two ADDITIVE top-level keys (release
+ // pipelines v2, Task 11 — review fix round 1) — old CLIs decode
+ // "channels" and ignore the rest. They are a REDUCED view of the
+ // pipeline — the entry channel and the full order, nothing about
+ // bake/rules — not the full signed release.Pipeline object `carlos
+ // pipeline` GETs (Task 6). Deliberately NOT nested under a "pipeline"
+ // key: the `/pipeline` door already uses that JSON path name for the
+ // full object, and reusing it here for a differently-shaped value (an
+ // object there, these two flat fields here) would be the same key
+ // meaning two incompatible things depending on which door answered it —
+ // a trap for any future consumer, and one that freezes the moment a
+ // released CLI starts decoding it. Two new key NAMES sidestep the
+ // collision entirely. The LOAD is shared with handleAPIPipelineGet
+ // verbatim (release.GetPipeline) so the two doors can never disagree
+ // about what "this app's pipeline" is.
//
- // A read failure here degrades to OMITTING the key rather than failing
- // this otherwise-successful, read-only listing — the channel rows above
- // already carry their own per-row errors, and `carlos pipeline` is one
- // call away for a caller that needs to know why. Crucially this is not
- // the same wire shape as "no pipeline.json": that confirmed-legacy case
- // is `"pipeline": null` (p, err both nil, exactly as handleAPIPipelineGet
- // answers it) — an errored read must never be presented as that, so it
- // leaves the key out instead of forging a null a stale reader would take
- // as "definitely legacy".
+ // Both keys are OMITTED — together, never one without the other — for
+ // BOTH a legacy app (no pipeline.json) AND a pipeline read failure.
+ // This channels door is a convenience summary for deploy resolution,
+ // not the authority on why a pipeline is absent: a caller that needs to
+ // tell "definitely no pipeline" apart from "the console could not read
+ // it" has the dedicated `/pipeline` door for that (its null-vs-502
+ // distinction, Task 6). Collapsing the two here costs nothing a caller
+ // of THIS door needs: deployTargets' entry-channel fallback treats an
+ // absent PipelineEntry as "no fallback" either way, which is the
+ // correct conservative behaviour for a read failure as much as for a
+ // genuinely legacy app.
if p, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app); err != nil {
log.Printf("console: api channels %s/%s: pipeline read: %v", acct.Sqid, app, err)
- } else if p == nil {
- resp["pipeline"] = nil
- } else {
+ } else if p != nil {
names := make([]string, len(p.Channels))
for i, c := range p.Channels {
names[i] = c.Name
}
- resp["pipeline"] = map[string]any{"entry": p.Entry(), "channels": names}
+ resp["pipeline_entry"] = p.Entry()
+ resp["pipeline_channels"] = names
}
writeJSON(w, http.StatusOK, resp)
}
diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go
index cc2e64fb8..18a1fe3b2 100644
--- a/internal/console/apicli_test.go
+++ b/internal/console/apicli_test.go
@@ -261,13 +261,23 @@ func TestAPIChannelsErrorRow(t *testing.T) {
}
}
-// TestAPIChannelsPipelineEnvelope pins the additive "pipeline" envelope key
-// (release pipelines v2, Task 11): a pipelined app's channels response
-// carries the REDUCED view {"entry": ..., "channels": [names in order]} —
-// sharing release.GetPipeline's read with handleAPIPipelineGet (Task 6) so
-// the two doors can never disagree — while a legacy app (no pipeline.json)
-// carries an explicit "pipeline": null. Old CLIs decode "channels" and
-// ignore the rest either way, so both shapes are additive.
+// TestAPIChannelsPipelineEnvelope pins the additive "pipeline_entry"/
+// "pipeline_channels" envelope keys (release pipelines v2, Task 11, review
+// fix round 1): a pipelined app's channels response carries the REDUCED view
+// as two flat top-level keys — "pipeline_entry" and "pipeline_channels":
+// [names in order] — sharing release.GetPipeline's read with
+// handleAPIPipelineGet (Task 6) so the two doors can never disagree. They
+// are deliberately NOT nested under a "pipeline" key: that JSON path already
+// names the full release.Pipeline object on the `/pipeline` door, and
+// reusing it here for a differently-shaped value would mean the same key
+// meaning two incompatible things depending on which door answered.
+//
+// A legacy app (no pipeline.json) gets BOTH keys OMITTED, never present as
+// null — this door does not attempt to distinguish "no pipeline" from "the
+// console could not read one" (see TestAPIChannelsPipelineUnreadableOmitsKey);
+// that distinction belongs to the dedicated `/pipeline` door alone. Old CLIs
+// decode "channels" and ignore the rest either way, so both shapes are
+// additive.
func TestAPIChannelsPipelineEnvelope(t *testing.T) {
fx := newConsoleFixture(t)
token := cliToken(t, fx, "paul@keymail.dev", "code-channels-pipeline")
@@ -289,16 +299,12 @@ func TestAPIChannelsPipelineEnvelope(t *testing.T) {
if status != http.StatusOK {
t.Fatalf("channels (pipelined) = %d %v", status, out)
}
- p, ok := out["pipeline"].(map[string]any)
- if !ok {
- t.Fatalf("channels (pipelined) pipeline = %v, want an object, not null/absent", out["pipeline"])
- }
- if str(t, p, "entry") != "edge" {
- t.Fatalf("pipeline entry = %v, want edge", p["entry"])
+ if str(t, out, "pipeline_entry") != "edge" {
+ t.Fatalf("pipeline_entry = %v, want edge", out["pipeline_entry"])
}
- names, _ := p["channels"].([]any)
+ names, _ := out["pipeline_channels"].([]any)
if len(names) != 2 || names[0] != "edge" || names[1] != "production" {
- t.Fatalf("pipeline channels = %v, want [edge production]", names)
+ t.Fatalf("pipeline_channels = %v, want [edge production]", names)
}
// The channel-rows key is untouched: a fresh app with no promotions has
// none yet.
@@ -308,10 +314,8 @@ func TestAPIChannelsPipelineEnvelope(t *testing.T) {
}
// Legacy app: created directly, bypassing the API create door's
- // default-pipeline write — no pipeline.json exists at all, so the key
- // must be an explicit null, never omitted (an omitted key is what an
- // UNREADABLE pipeline gets, and this app's pipeline is genuinely
- // absent, not unreadable).
+ // default-pipeline write — no pipeline.json exists at all, so BOTH keys
+ // must be absent, together, exactly as the unreadable case below.
if _, err := fx.st.CreateAppIn(OpsAccountID, "legacyapp", "paul@keymail.dev", time.Now()); err != nil {
t.Fatal(err)
}
@@ -319,23 +323,24 @@ func TestAPIChannelsPipelineEnvelope(t *testing.T) {
if status != http.StatusOK {
t.Fatalf("channels (legacy) = %d %v", status, out)
}
- pval, present := out["pipeline"]
- if !present {
- t.Fatalf("channels (legacy) omitted the pipeline key entirely, want an explicit null")
+ if _, present := out["pipeline_entry"]; present {
+ t.Fatalf("channels (legacy) pipeline_entry = %v, want the key OMITTED", out["pipeline_entry"])
}
- if pval != nil {
- t.Fatalf("channels (legacy) pipeline = %v, want null", pval)
+ if _, present := out["pipeline_channels"]; present {
+ t.Fatalf("channels (legacy) pipeline_channels = %v, want the key OMITTED", out["pipeline_channels"])
}
}
// TestAPIChannelsPipelineUnreadableOmitsKey: an app whose pipeline.json
-// exists but fails verification must not be told apart from a legacy app —
-// GetPipeline's error is a genuinely different state (spec-mandated 502 on
-// the dedicated `carlos pipeline` door), and forging "pipeline": null here
-// would tell a caller "definitely no pipeline" when the truth is "the
-// console could not read it". The channels listing itself is unharmed: it
-// is read-only info this door must not fail over a pipeline it doesn't
-// strictly need.
+// exists but fails verification gets the SAME wire shape as a legacy app —
+// both pipeline_entry/pipeline_channels OMITTED. Unlike the dedicated
+// `/pipeline` door (which answers a genuinely-missing pipeline with 200/null
+// and an unreadable one with 502, Task 6), this door does not attempt that
+// distinction at all (review fix round 1) — it is a convenience summary for
+// deploy resolution, and an absent entry channel is the correct conservative
+// answer whether the pipeline is missing or merely unreadable. The channels
+// listing itself is unharmed: it is read-only info this door must not fail
+// over a pipeline it doesn't strictly need.
func TestAPIChannelsPipelineUnreadableOmitsKey(t *testing.T) {
fx := newConsoleFixture(t)
if _, err := fx.st.CreateAppIn(OpsAccountID, "widget", "paul@keymail.dev", time.Now()); err != nil {
@@ -350,8 +355,11 @@ func TestAPIChannelsPipelineUnreadableOmitsKey(t *testing.T) {
if status != http.StatusOK {
t.Fatalf("channels (unreadable pipeline) = %d %v, want 200 — the listing itself is fine", status, out)
}
- if _, present := out["pipeline"]; present {
- t.Fatalf("channels (unreadable pipeline) = %v, want the pipeline key OMITTED, not present (even as null)", out)
+ if _, present := out["pipeline_entry"]; present {
+ t.Fatalf("channels (unreadable pipeline) pipeline_entry = %v, want the key OMITTED, not present (even as null)", out)
+ }
+ if _, present := out["pipeline_channels"]; present {
+ t.Fatalf("channels (unreadable pipeline) pipeline_channels = %v, want the key OMITTED, not present (even as null)", out)
}
}