amadan
services: fix the deliver command (C43) and its systemic test (C44/C45)
C43 (Critical, reviewer's own error, upstream): the "deliver it" line
printed by `carlos services create`/`rotate` told an operator to run
`carlos env set -app <app> -secret CARLOS_SERVICE_TOKEN=<secret>`. That
flag does not exist — env set has -app/-environment/-account/-account-id/
-registry/-console and nothing else — so a copy-pasted deliver command hit
"flag provided but not defined" while the secret it named could never be
shown again. The fix is `carlos secrets set -app <app>
CARLOS_SERVICE_TOKEN=<secret>` (sealed storage, appconfig.Var{Kind:
"secret"}), not a bare env set with -secret dropped (which would be valid
but write UNSEALED plaintext). Same wrong command fixed in
internal/console/apiinstances.go's CARLOS_INSTANCE_TOKEN delivery comment;
docs/superpowers/plans/2026-08-18-service-principals.md already carried the
reviewer's own fix.
New services.go `deliverLine` composes this command in one place for both
create and rotate, so the two cannot drift apart on it again — app is
interpolated when the caller already knows it (create, from -app) and left
as a generic "<app>" placeholder when it does not (rotate: the rotate
door's response carries name/account/token only, no app).
C45 (Important): `printServiceRotated` now prints the same deliver-it line
create's output does — rotation is the moment a running service's secret
stops working, and the old output never told the operator where to put the
new one.
C44: TestServicesDeliverLineIsARealCommand closes the class of bug C43 was
an instance of. It extracts the verb and flag names from BOTH create's and
rotate's printed "deliver it" line and checks each flag against the real,
subprocess-built binary's own `-h` output for that verb — a test that asks
whether the printed command runs, not just what token it contains.
Five minors from the same review, all addressed:
- rotate's revoke hint now names the real resolved account (acct.Seg),
not a generic "<sqid|name>" placeholder — create already did.
- the matched=false revoke message no longer contains the substring
"revoked" anywhere ("already gone" replaces "already revoked"), so a
script doing `| grep -q revoked` cannot misread it as success either.
- createService's unreadable-201-body error now tells the operator the
credential exists server-side with its plaintext lost, and points at
`carlos services ls` + rotate to recover.
- the two services_cli_test.go subprocess env-builders that didn't
already set CARLOS_DEPLOYMENT_DIR now also mask CARLOS_DEPLOYMENT_BUCKET/
CARLOS_DEPLOYMENT_DIR explicitly through carlosTestEnv's extras, matching
deploy_test.go's own subprocess helpers.
- (validAppName scope note, no change: reviewer confirmed leaving -app's
client-side validation as-is is correct — existence needs the server
anyway.)
Full gate clean: go build/vet ./..., gofmt -l (touched files), go test
./... (all 21 packages), GOOS=darwin go build ./.... No smart quotes
(U+201C/201D) introduced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
5 files changed,
+204
−17
apimode.go+8 −1docs/superpowers/plans/2026-08-18-service-principals.md+1 −1internal/console/apiinstances.go+1 −1services.go+47 −12services_cli_test.go+147 −2
diff --git a/apimode.go b/apimode.go| index cdb4f6afd..fdec85bdb 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -2484,7 +2484,14 @@ func (s *apiSession) createService(ctx context.Context, a apiAcct, name, role, a |
| Doors []string `json:"doors"` |
| } |
| if err := json.Unmarshal(body, &res); err != nil { |
| - return "", nil, fmt.Errorf("services create: unreadable response: %w", err) |
| + // The 201 already happened: the credential exists server-side and |
| + // its ONLY plaintext copy — never stored anywhere, only its hash is |
| + // — was in this unreadable body. Silence here would leave the |
| + // operator not knowing the mint even landed. Point at the one way |
| + // to recover a working secret for it. |
| + return "", nil, fmt.Errorf("services create %s: minted, but the response was unreadable (%v) — "+ |
| + "the credential exists and its only plaintext copy is lost; check `carlos services ls` "+ |
| + "and rotate it", name, err) |
| } |
| return res.Token, res.Doors, nil |
| } |
diff --git a/docs/superpowers/plans/2026-08-18-service-principals.md b/docs/superpowers/plans/2026-08-18-service-principals.md| index efc051f82..6750eb606 100644 |
| --- a/docs/superpowers/plans/2026-08-18-service-principals.md |
| +++ b/docs/superpowers/plans/2026-08-18-service-principals.md |
| @@ -1869,7 +1869,7 @@ it may: |
| POST /api/cli/apps/{account}/{app}/releases |
| … |
| -deliver it: carlos env set -app <app> -secret CARLOS_SERVICE_TOKEN=<secret> |
| +deliver it: carlos secrets set -app <app> CARLOS_SERVICE_TOKEN=<secret> |
| revoke it: carlos services revoke -account bdm amadan-hub |
| ``` |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index 445b30597..76d078ae9 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -882,7 +882,7 @@ func (s *Server) handleAPIInstanceToken(w http.ResponseWriter, r *http.Request) |
| // Shown exactly once: only the hash is stored, so there is no second |
| // copy to hand out and no way to recover this one. Delivery to the app |
| // is the config-env layer, like any other app secret — |
| - // `carlos env set -app <app> -secret CARLOS_INSTANCE_TOKEN=...`. |
| + // `carlos secrets set -app <app> CARLOS_INSTANCE_TOKEN=...`. |
| writeJSON(w, http.StatusCreated, map[string]any{ |
| "token": raw, "account": acct.Sqid, "app": app, |
| }) |
diff --git a/services.go b/services.go| index dc2a2ded6..44eac45c8 100644 |
| --- a/services.go |
| +++ b/services.go |
| @@ -198,18 +198,40 @@ func roleNames() []string { |
| return out |
| } |
| +// deliverLine composes the ONE command this file ever tells an operator to |
| +// run to get a minted secret into a running app — shared by create and |
| +// rotate so the two cannot drift apart on it again. C43 (review): the |
| +// original text here was `carlos env set -app <app> -secret |
| +// CARLOS_SERVICE_TOKEN=<secret>` — env set has no -secret flag at all, so |
| +// an operator who copied it verbatim got "flag provided but not defined" |
| +// while holding a secret that had just been printed and could never be |
| +// shown again. The fix is not to drop -secret and keep env set: that turns |
| +// it into a VALID command that writes UNSEALED plaintext config |
| +// (appconfig.Var{Kind:"env"}). The sealing verb is `carlos secrets set` |
| +// (Kind:"secret", Envelope) — the only correct destination for a |
| +// credential. |
| +// |
| +// appHint is the real app name when the caller already has it (create, |
| +// straight from -app) and "" when it does not (rotate: handleAPIServiceRotate's |
| +// response carries name/account/token only, no app) — in which case a |
| +// generic "<app>" placeholder is printed instead, exactly like the secret |
| +// itself. Never the token: only ever the literal "<secret>" placeholder, |
| +// so the secret appears exactly once in the surrounding print, on its own |
| +// CARLOS_SERVICE_TOKEN= line, not a second time inside a copy-paste command. |
| +func deliverLine(appHint string) string { |
| + app := "<app>" |
| + if appHint != "" { |
| + app = appHint |
| + } |
| + return "carlos secrets set -app " + app + " CARLOS_SERVICE_TOKEN=<secret>" |
| +} |
| + |
| // printServiceCreated is the ONE place a service credential's secret |
| // reaches stdout on create — printed exactly once, immediately, with the |
| // scope it was minted into stated in words (requirement: an app-scoped |
| // credential's door list must not read as "any app"; the {app} wildcard in |
| // every releases/promote/etc. pattern below is only ever true for THIS app), |
| // the doors it reaches, how to deliver it, and how to revoke it. |
| -// |
| -// The deliver line shows a literal "<secret>" placeholder rather than the |
| -// token itself: the secret appears exactly once in this output, on the |
| -// CARLOS_SERVICE_TOKEN= line above, not a second time inside a copy-paste |
| -// command. There is only one plaintext copy of this credential ever |
| -// (only its hash is stored), and this print is it. |
| func printServiceCreated(acctSeg, name, role, app, token string, doors []string) { |
| scope := "account-wide" |
| if app != "" { |
| @@ -233,7 +255,7 @@ func printServiceCreated(acctSeg, name, role, app, token string, doors []string) |
| fmt.Fprintf(stdout, " %-6s %s\n", method, path) |
| } |
| fmt.Fprintln(stdout) |
| - fmt.Fprintln(stdout, "deliver it: carlos env set -app <app> -secret CARLOS_SERVICE_TOKEN=<secret>") |
| + fmt.Fprintln(stdout, "deliver it: "+deliverLine(app)) |
| fmt.Fprintf(stdout, "revoke it: carlos services revoke -account %s %s\n", acctSeg, name) |
| } |
| @@ -306,7 +328,7 @@ func runServicesRotate(args []string) { |
| if err != nil { |
| log.Fatalf("services rotate: %v", err) |
| } |
| - printServiceRotated(name, token) |
| + printServiceRotated(acct.Seg, name, token) |
| } |
| // printServiceRotated is the ONE place a rotated secret reaches stdout — |
| @@ -314,15 +336,22 @@ func runServicesRotate(args []string) { |
| // printFleetToken carries for fleets, in this family's own words: rotation |
| // refuses the credential's NEXT call with the old secret, but does not stop |
| // anything already in flight, and it is not what a compromised credential |
| -// needs — that is revoke. |
| -func printServiceRotated(name, token string) { |
| +// needs — that is revoke. C45 (review): a credential nobody can deliver is |
| +// not finished, and rotation is exactly the moment a RUNNING service's |
| +// secret stops working — this now carries the same deliver-it line create's |
| +// output does, using deliverLine's own generic "<app>" placeholder: unlike |
| +// create, this verb never learns which app the credential is scoped to |
| +// (handleAPIServiceRotate's response carries name/account/token only). |
| +func printServiceRotated(acctSeg, name, token string) { |
| fmt.Fprintf(stdout, "rotated %s\n\n", name) |
| fmt.Fprintf(stdout, " CARLOS_SERVICE_TOKEN=%s\n\n", token) |
| fmt.Fprintln(stdout, " ^ shown once. Only its hash is stored — a lost secret means rotate again, not a re-run.") |
| fmt.Fprintln(stdout) |
| fmt.Fprintln(stdout, "ROTATION IS NOT REVOCATION: this refuses the credential's NEXT call with the OLD") |
| fmt.Fprintln(stdout, "secret, but does not stop work already in flight — a credential that may be") |
| - fmt.Fprintf(stdout, "COMPROMISED needs `carlos services revoke -account <sqid|name> %s`, not another rotate.\n", name) |
| + fmt.Fprintf(stdout, "COMPROMISED needs `carlos services revoke -account %s %s`, not another rotate.\n", acctSeg, name) |
| + fmt.Fprintln(stdout) |
| + fmt.Fprintln(stdout, "deliver it: "+deliverLine("")) |
| } |
| func runServicesRevoke(args []string) { |
| @@ -359,5 +388,11 @@ func runServicesRevoke(args []string) { |
| fmt.Fprintf(stdout, "revoked %s — its next call fails, whatever secret it holds\n", handle) |
| return |
| } |
| - fmt.Fprintf(stdout, "no live credential of %q — nothing to revoke (already revoked, or never existed)\n", handle) |
| + // "already revoked" would work for a human reader but leaves the |
| + // substring "revoked" in a matched=false line — a diluted version of |
| + // the exact failure `matched` exists to prevent, this time against a |
| + // script doing `carlos services revoke $n | grep -q revoked` rather |
| + // than a person reading the line. "gone" says the same thing without |
| + // it (review finding, minor). |
| + fmt.Fprintf(stdout, "no live credential of %q — nothing to revoke (already gone, or never existed)\n", handle) |
| } |
diff --git a/services_cli_test.go b/services_cli_test.go| index 06eb0b060..c27bf4d17 100644 |
| --- a/services_cli_test.go |
| +++ b/services_cli_test.go |
| @@ -237,6 +237,33 @@ func TestAPISessionServiceDoorsSurfaceServerErrors(t *testing.T) { |
| } |
| } |
| +// TestAPISessionServiceCreateUnreadableBodyTellsOperatorToRecover: a 201 |
| +// whose body createService cannot parse still means the mint happened |
| +// server-side — the credential exists and its only plaintext copy was in |
| +// that unreadable body. Silence here would leave an operator not knowing a |
| +// credential now exists with a secret nobody can ever see; the error must |
| +// point at the recovery (review, minor). |
| +func TestAPISessionServiceCreateUnreadableBodyTellsOperatorToRecover(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + w.WriteHeader(http.StatusCreated) |
| + w.Write([]byte("not json")) |
| + })) |
| + defer ts.Close() |
| + s := newTestSession(ts.URL) |
| + |
| + _, _, err := s.createService(context.Background(), apiAcct{Seg: "bab"}, "amadan-hub", "publish", "") |
| + if err == nil { |
| + t.Fatal("createService with an unreadable 201 body must error") |
| + } |
| + msg := strings.ToLower(err.Error()) |
| + if !strings.Contains(msg, "carlos services ls") { |
| + t.Fatalf("error missing the recovery pointer to `carlos services ls`: %v", err) |
| + } |
| + if !strings.Contains(msg, "rotate") { |
| + t.Fatalf("error missing the recovery pointer to rotate: %v", err) |
| + } |
| +} |
| + |
| // ---- RunServices-level: full verb, good args, real printing --------------- |
| // fakeServicesConsole answers the four CLI endpoints RunServices reaches, |
| @@ -368,6 +395,12 @@ func TestServicesCreateAppScopedSaysSoNextToTheDoors(t *testing.T) { |
| if strings.Contains(out, "account-wide") { |
| t.Fatalf("stdout claims account-wide for an -app-scoped credential:\n%s", out) |
| } |
| + // Review finding (minor 1): the deliver line itself should interpolate |
| + // the real app rather than leave the generic "<app>" placeholder when |
| + // it is already known — only the secret stays a placeholder. |
| + if got := deliverLineFromOutput(t, out); !strings.Contains(got, "-app titogo") { |
| + t.Fatalf("deliver line = %q, want it to name the real app (-app titogo)", got) |
| + } |
| } |
| // TestServicesCreateRefusesABadNameBeforeCalling — a shape the server is |
| @@ -391,7 +424,13 @@ func TestServicesCreateRefusesABadNameBeforeCalling(t *testing.T) { |
| scratch := t.TempDir() |
| cmd := exec.Command(bin, "services", "create", "-role", "publish", "-account", "bab", "Bad Name") |
| - cmd.Env = carlosTestEnv(t, "CARLOS_CREDENTIALS="+creds) |
| + // CARLOS_DEPLOYMENT_BUCKET/DIR masked explicitly too (review, minor): |
| + // carlosTestEnv does not clear them on its own, and deploy_test.go's own |
| + // subprocess helpers always pin both alongside CARLOS_CREDENTIALS for |
| + // exactly this reason — not a live hazard here (nothing in this |
| + // process's own env sets them), but this is the constraint where "not a |
| + // hazard today" is not the bar. |
| + cmd.Env = carlosTestEnv(t, "CARLOS_CREDENTIALS="+creds, "CARLOS_DEPLOYMENT_BUCKET=", "CARLOS_DEPLOYMENT_DIR=") |
| cmd.Dir = scratch |
| out, err := cmd.CombinedOutput() |
| if err == nil { |
| @@ -448,6 +487,102 @@ func TestServicesRotateSaysWhatRotationIsNot(t *testing.T) { |
| if !strings.Contains(out, "revoke") { |
| t.Fatalf("stdout missing the pointer to revoke for a COMPROMISED credential:\n%s", out) |
| } |
| + // Review finding (minor 2): the revoke hint must name the REAL account |
| + // this session resolved, not a generic "<sqid|name>" placeholder — |
| + // create already did; rotate has acct in hand too. |
| + if !strings.Contains(out, "carlos services revoke -account bab amadan-hub") { |
| + t.Fatalf("stdout revoke hint does not name the real account (bab):\n%s", out) |
| + } |
| + // C45 (review): rotation is the moment a RUNNING service's secret stops |
| + // working, so this output must also say how to deliver the fresh one — |
| + // a credential nobody can deliver is not finished, the same principle |
| + // create's own output states. deliverLineFromOutput itself fails the |
| + // test if the line is missing. |
| + deliverLineFromOutput(t, out) |
| +} |
| + |
| +// deliverLineFromOutput pulls the "deliver it:" line out of printed |
| +// RunServices output — the shared parsing TestServicesDeliverLineIsARealCommand |
| +// uses to check what create's and rotate's output actually tells an |
| +// operator to type. |
| +func deliverLineFromOutput(t *testing.T, out string) string { |
| + t.Helper() |
| + for _, line := range strings.Split(out, "\n") { |
| + trimmed := strings.TrimSpace(line) |
| + if strings.HasPrefix(trimmed, "deliver it:") { |
| + return strings.TrimSpace(strings.TrimPrefix(trimmed, "deliver it:")) |
| + } |
| + } |
| + t.Fatalf("no \"deliver it:\" line in output:\n%s", out) |
| + return "" |
| +} |
| + |
| +// parseDeliverCommand splits a printed command line ("carlos secrets set |
| +// -app titogo CARLOS_SERVICE_TOKEN=<secret>") into the verb tokens that |
| +// precede its first flag ("secrets", "set") and the flag tokens themselves |
| +// ("-app") — everything needed to ask the real binary's own -h output |
| +// whether the command it just told an operator to run actually exists. |
| +func parseDeliverCommand(t *testing.T, line string) (verb, flags []string) { |
| + t.Helper() |
| + line = strings.TrimPrefix(strings.TrimSpace(line), "carlos ") |
| + for _, f := range strings.Fields(line) { |
| + if strings.HasPrefix(f, "-") { |
| + flags = append(flags, f) |
| + continue |
| + } |
| + if len(flags) == 0 { |
| + verb = append(verb, f) |
| + } |
| + } |
| + if len(verb) == 0 { |
| + t.Fatalf("deliver command %q named no verb", line) |
| + } |
| + return verb, flags |
| +} |
| + |
| +// TestServicesDeliverLineIsARealCommand — C44 (review), the systemic fix |
| +// for C43: a printed "deliver it" command whose flag did not exist |
| +// (`carlos env set ... -secret`, env set has no such flag) sent an operator |
| +// who copied it verbatim to a `flag provided but not defined` refusal while |
| +// holding a plaintext secret that could never be shown again. Counting |
| +// token occurrences (this file's other create/rotate tests) never asked |
| +// whether the printed line RUNS — this test does, for both create's and |
| +// rotate's deliver line: it extracts the verb and flag names the line |
| +// names and asks the real, subprocess-built binary's own `-h` output for |
| +// that verb whether each flag actually exists. This is a test about the |
| +// COMMAND, so the check that runs it is itself a subprocess, built through |
| +// carlosTestEnv and run from a scratch cmd.Dir — the constraint this whole |
| +// suite is written around. |
| +func TestServicesDeliverLineIsARealCommand(t *testing.T) { |
| + bin := buildCarlos(t) |
| + scratch := t.TempDir() |
| + env := carlosTestEnv(t, "CARLOS_DEPLOYMENT_BUCKET=", "CARLOS_DEPLOYMENT_DIR=") |
| + |
| + checkDeliverLine := func(t *testing.T, out string) { |
| + t.Helper() |
| + line := deliverLineFromOutput(t, out) |
| + verb, flags := parseDeliverCommand(t, line) |
| + cmd := exec.Command(bin, append(append([]string{}, verb...), "-h")...) |
| + cmd.Env = env |
| + cmd.Dir = scratch |
| + helpOut, _ := cmd.CombinedOutput() // -h exits 0 for a real verb; output is what matters either way |
| + for _, f := range flags { |
| + if !strings.Contains(string(helpOut), f) { |
| + t.Fatalf("deliver line %q names flag %q, which `carlos %s -h` does not list:\n%s", |
| + line, f, strings.Join(verb, " "), helpOut) |
| + } |
| + } |
| + } |
| + |
| + servicesCLIFixture(t) |
| + |
| + createBuf := captureStdout(t) |
| + RunServices([]string{"create", "-role", "publish", "-app", "titogo", "-account", "bab", "amadan-hub"}) |
| + checkDeliverLine(t, createBuf.String()) |
| + |
| + rotateBuf := captureStdout(t) |
| + RunServices([]string{"rotate", "-account", "bab", "amadan-hub"}) |
| + checkDeliverLine(t, rotateBuf.String()) |
| } |
| // TestRunServicesRevokeMatchedTrue: an ordinary revoke of a live credential |
| @@ -490,6 +625,13 @@ func TestRunServicesRevokeMatchedFalseDoesNotPrintSuccess(t *testing.T) { |
| if !strings.Contains(lower, "nothing to revoke") && !strings.Contains(lower, "no live credential") { |
| t.Fatalf("stdout missing the matched=false wording:\n%s", out) |
| } |
| + // Review finding (minor): the substring "revoked" must not appear |
| + // anywhere in a matched=false line — not even inside a word like |
| + // "already revoked" — because a script checking `| grep -q revoked` |
| + // would read a typo as success just as a human skimming "revoked" would. |
| + if strings.Contains(lower, "revoked") { |
| + t.Fatalf("stdout contains the substring \"revoked\" for matched=false, which a naive grep would misread as success:\n%s", out) |
| + } |
| } |
| // ---- subprocess: the usage-error / bad-argument paths --------------------- |
| @@ -507,7 +649,10 @@ func TestRunServicesRevokeMatchedFalseDoesNotPrintSuccess(t *testing.T) { |
| func TestServicesCLIUsageErrors(t *testing.T) { |
| bin := buildCarlos(t) |
| scratch := t.TempDir() |
| - env := carlosTestEnv(t) |
| + // CARLOS_DEPLOYMENT_BUCKET/DIR masked explicitly too (review, minor) — |
| + // see TestServicesCreateRefusesABadNameBeforeCalling's comment on the |
| + // same fix. |
| + env := carlosTestEnv(t, "CARLOS_DEPLOYMENT_BUCKET=", "CARLOS_DEPLOYMENT_DIR=") |
| run := func(args ...string) (string, error) { |
| cmd := exec.Command(bin, args...) |