← back to Cli Printing Press
fix(cli): reconcile MCPB manifest against internal/client env reads (#859) (#1035)
c93ce0c261efa227ffa916bd74a63026c769f207 · 2026-05-11 00:41:11 -0700 · Trevin Chow
* fix(cli): reconcile MCPB manifest against internal/client env reads (#859)
Hand-written credential-flow code in internal/client/ (auth_refresh.go for
JWT-with-refresh flows, etc.) reads env vars the OpenAPI spec never advertises.
Without lock-time reconciliation those vars never reach the MCPB user_config
block and the bundled server fails on first refresh.
Add a post-promote scan that parses every Go file in the staging dir's
internal/client/, collects os.Getenv("...") string literals, and extends
manifest.json's user_config + mcp_config.env with any env var the manifest
does not already declare. Sensitive=true on every discovered field; required
mirrors authRequiresCredential and AuthOptional so optional/composed auth
keeps discovered fields optional.
Purely additive — APIs without internal/client/ or with all env reads already
declared (the spec-driven default) produce no changes. Verified against the
existing 16 golden cases.
* fix(cli): fold client-env reconcile into WriteMCPBManifestFromStruct (#859)
Move the post-write reconcile call from PromoteWorkingCLI into the writer
itself so every WriteMCPBManifest / WriteMCPBManifestFromStruct caller
(lock+promote, publish.go, mcpsync, renamecli, and the in-memory
climanifest path) reads a reconciled manifest. Without this, a one-off
bundle build or publish path that bypasses lock+promote would ship the
exact stale-manifest bug #859 describes.
Thread the in-memory CLIManifest through reconcileMCPBManifestFromClient
so the reconciler does not re-read .printing-press.json — the writer
already has it, and callers that build the manifest in memory may not
have written it to disk yet (caught by an integration test now in the
suite).
Surface parser errors on individual client files via stderr instead of
silently skipping so an operator publishing sees which file the scan
could not analyze. Trim the speculative "half-generated tree" comment
per AGENTS.md Code & Comment Hygiene.
Add integration tests:
- TestWriteMCPBManifestFromStruct_ReconcilesClientEnvReads: end-to-end
via the writer entry point (catches regressions that detach
reconcile from the writer).
- TestWriteMCPBManifestFromStruct_IdempotentReconcile: two consecutive
writer runs produce identical output bytes.
- TestScanClientEnvReadsBacktickLiteral: backtick-quoted env names
survive strconv.Unquote.
- Tighten the rentalworks-home assertion with NotContains("Optional.")
to guard the required-vs-optional branch.
* fix(cli): abort lock+promote on MCPB manifest reconcile failure (#859)
The lock.go WriteMCPBManifest call previously logged a stderr warning
and proceeded with the staging swap on any failure. Folding reconcile
into the writer surfaced the latent issue: a reconcile failure (the
exact bug #859 fixes) was getting downgraded to a warning, then a CLI
with an incomplete user_config block was published anyway.
Treat failures the same way writeCLIManifestForPublish does at line 268:
remove staging, return the wrapped error, do not publish a partial
manifest. The only callers that would have relied on warn-and-continue
were those willing to ship a stale manifest — that contract was already
incompatible with the rest of the lock+promote semantics.
Add two integration tests that close the gaps the test reviewer flagged:
- TestWriteMCPBManifest_DiskReadVariantReconciles exercises the
disk-read writer entry point (the shape lock.go / publish.go /
mcpsync actually use).
- TestWriteMCPBManifestFromStruct_AuthOptionalPropagates guards the
in-memory CLIManifest threading on the AuthOptional branch.
Files touched
A internal/pipeline/client_env_scanner.goA internal/pipeline/client_env_scanner_test.goM internal/pipeline/lock.goM internal/pipeline/mcpb_manifest.go
Diff
commit c93ce0c261efa227ffa916bd74a63026c769f207
Author: Trevin Chow <trevin@trevinchow.com>
Date: Mon May 11 00:41:11 2026 -0700
fix(cli): reconcile MCPB manifest against internal/client env reads (#859) (#1035)
* fix(cli): reconcile MCPB manifest against internal/client env reads (#859)
Hand-written credential-flow code in internal/client/ (auth_refresh.go for
JWT-with-refresh flows, etc.) reads env vars the OpenAPI spec never advertises.
Without lock-time reconciliation those vars never reach the MCPB user_config
block and the bundled server fails on first refresh.
Add a post-promote scan that parses every Go file in the staging dir's
internal/client/, collects os.Getenv("...") string literals, and extends
manifest.json's user_config + mcp_config.env with any env var the manifest
does not already declare. Sensitive=true on every discovered field; required
mirrors authRequiresCredential and AuthOptional so optional/composed auth
keeps discovered fields optional.
Purely additive — APIs without internal/client/ or with all env reads already
declared (the spec-driven default) produce no changes. Verified against the
existing 16 golden cases.
* fix(cli): fold client-env reconcile into WriteMCPBManifestFromStruct (#859)
Move the post-write reconcile call from PromoteWorkingCLI into the writer
itself so every WriteMCPBManifest / WriteMCPBManifestFromStruct caller
(lock+promote, publish.go, mcpsync, renamecli, and the in-memory
climanifest path) reads a reconciled manifest. Without this, a one-off
bundle build or publish path that bypasses lock+promote would ship the
exact stale-manifest bug #859 describes.
Thread the in-memory CLIManifest through reconcileMCPBManifestFromClient
so the reconciler does not re-read .printing-press.json — the writer
already has it, and callers that build the manifest in memory may not
have written it to disk yet (caught by an integration test now in the
suite).
Surface parser errors on individual client files via stderr instead of
silently skipping so an operator publishing sees which file the scan
could not analyze. Trim the speculative "half-generated tree" comment
per AGENTS.md Code & Comment Hygiene.
Add integration tests:
- TestWriteMCPBManifestFromStruct_ReconcilesClientEnvReads: end-to-end
via the writer entry point (catches regressions that detach
reconcile from the writer).
- TestWriteMCPBManifestFromStruct_IdempotentReconcile: two consecutive
writer runs produce identical output bytes.
- TestScanClientEnvReadsBacktickLiteral: backtick-quoted env names
survive strconv.Unquote.
- Tighten the rentalworks-home assertion with NotContains("Optional.")
to guard the required-vs-optional branch.
* fix(cli): abort lock+promote on MCPB manifest reconcile failure (#859)
The lock.go WriteMCPBManifest call previously logged a stderr warning
and proceeded with the staging swap on any failure. Folding reconcile
into the writer surfaced the latent issue: a reconcile failure (the
exact bug #859 fixes) was getting downgraded to a warning, then a CLI
with an incomplete user_config block was published anyway.
Treat failures the same way writeCLIManifestForPublish does at line 268:
remove staging, return the wrapped error, do not publish a partial
manifest. The only callers that would have relied on warn-and-continue
were those willing to ship a stale manifest — that contract was already
incompatible with the rest of the lock+promote semantics.
Add two integration tests that close the gaps the test reviewer flagged:
- TestWriteMCPBManifest_DiskReadVariantReconciles exercises the
disk-read writer entry point (the shape lock.go / publish.go /
mcpsync actually use).
- TestWriteMCPBManifestFromStruct_AuthOptionalPropagates guards the
in-memory CLIManifest threading on the AuthOptional branch.
---
internal/pipeline/client_env_scanner.go | 179 ++++++++++
internal/pipeline/client_env_scanner_test.go | 473 +++++++++++++++++++++++++++
internal/pipeline/lock.go | 10 +-
internal/pipeline/mcpb_manifest.go | 27 +-
4 files changed, 683 insertions(+), 6 deletions(-)
diff --git a/internal/pipeline/client_env_scanner.go b/internal/pipeline/client_env_scanner.go
new file mode 100644
index 00000000..0884d698
--- /dev/null
+++ b/internal/pipeline/client_env_scanner.go
@@ -0,0 +1,179 @@
+package pipeline
+
+import (
+ "encoding/json"
+ "fmt"
+ "go/ast"
+ "go/parser"
+ "go/token"
+ "os"
+ "path/filepath"
+ "sort"
+ "strconv"
+ "strings"
+)
+
+// scanClientEnvReads parses every Go source file under <dir>/internal/client/
+// and returns the deduplicated, sorted set of env var names read via
+// os.Getenv("...").
+//
+// The scan is intentionally restricted to internal/client/. Hand-written
+// auth-refresh code in that package is the unambiguous signal the manifest
+// reconciler keys off — broader scans (cmd/, internal/cli/) would pick up
+// env reads that should not become MCPB user_config entries (debug flags,
+// test helpers, IDE shims).
+//
+// Parser errors on individual files are reported via stderr so an operator
+// running publish sees which file the scan skipped. The scan continues so
+// one malformed file does not block reconciliation of the remaining files.
+func scanClientEnvReads(dir string) ([]string, error) {
+ clientDir := filepath.Join(dir, "internal", "client")
+ entries, err := os.ReadDir(clientDir)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return nil, nil
+ }
+ return nil, fmt.Errorf("reading client dir: %w", err)
+ }
+
+ seen := make(map[string]struct{})
+ fset := token.NewFileSet()
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".go") {
+ continue
+ }
+ path := filepath.Join(clientDir, e.Name())
+ file, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "warning: skipping unparseable client file %s: %v\n", path, err)
+ continue
+ }
+ ast.Inspect(file, func(n ast.Node) bool {
+ call, ok := n.(*ast.CallExpr)
+ if !ok || len(call.Args) != 1 {
+ return true
+ }
+ sel, ok := call.Fun.(*ast.SelectorExpr)
+ if !ok || sel.Sel.Name != "Getenv" {
+ return true
+ }
+ pkg, ok := sel.X.(*ast.Ident)
+ if !ok || pkg.Name != "os" {
+ return true
+ }
+ lit, ok := call.Args[0].(*ast.BasicLit)
+ if !ok || lit.Kind != token.STRING {
+ return true
+ }
+ name, err := strconv.Unquote(lit.Value)
+ if err != nil || name == "" {
+ return true
+ }
+ seen[name] = struct{}{}
+ return true
+ })
+ }
+
+ names := make([]string, 0, len(seen))
+ for n := range seen {
+ names = append(names, n)
+ }
+ sort.Strings(names)
+ return names, nil
+}
+
+// reconcileMCPBManifestFromClient extends the just-written manifest.json in
+// dir with user_config entries for any os.Getenv read in internal/client/*.go
+// that the manifest does not already declare. cli is the same in-memory
+// CLIManifest the writer just emitted; passing it through avoids a re-read
+// of .printing-press.json (which may not be on disk yet for callers that
+// build the CLI manifest in memory).
+//
+// Safe by construction:
+// - APIs with no internal/client/ dir produce no changes.
+// - Env vars already declared in mcp_config.env are skipped.
+// - Goldens for spec-driven APIs without hand-written client code are
+// untouched because their client.go's os.Getenv calls all resolve to
+// names already in mcp_config.env.
+func reconcileMCPBManifestFromClient(dir string, cli CLIManifest) error {
+ manifestPath := filepath.Join(dir, MCPBManifestFilename)
+ data, err := os.ReadFile(manifestPath)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return nil
+ }
+ return fmt.Errorf("reading MCPB manifest: %w", err)
+ }
+
+ var manifest MCPBManifest
+ if err := json.Unmarshal(data, &manifest); err != nil {
+ return fmt.Errorf("parsing MCPB manifest: %w", err)
+ }
+
+ envReads, err := scanClientEnvReads(dir)
+ if err != nil {
+ return err
+ }
+ if len(envReads) == 0 {
+ return nil
+ }
+
+ var missing []string
+ for _, name := range envReads {
+ if _, declared := manifest.Server.MCPConfig.Env[name]; declared {
+ continue
+ }
+ missing = append(missing, name)
+ }
+ if len(missing) == 0 {
+ return nil
+ }
+
+ if manifest.Server.MCPConfig.Env == nil {
+ manifest.Server.MCPConfig.Env = make(map[string]string, len(missing))
+ }
+ if manifest.UserConfig == nil {
+ manifest.UserConfig = make(map[string]MCPBVar, len(missing))
+ }
+
+ required := authRequiresCredential(cli.AuthType) && !cli.AuthOptional
+ for _, name := range missing {
+ key := userConfigKey(name)
+ manifest.Server.MCPConfig.Env[name] = "${user_config." + key + "}"
+ manifest.UserConfig[key] = MCPBVar{
+ Type: mcpbVarTypeString,
+ Title: name,
+ Description: discoveredEnvDescription(cli, name, required),
+ Sensitive: true,
+ Required: required,
+ }
+ }
+
+ out, err := marshalMCPBManifest(manifest)
+ if err != nil {
+ return err
+ }
+ return os.WriteFile(manifestPath, out, 0o644)
+}
+
+// discoveredEnvDescription mirrors envVarDescription's shape but flags the
+// field as discovered from the client source so an install-page reader knows
+// why it appeared in the user_config block alongside the spec-declared keys.
+func discoveredEnvDescription(m CLIManifest, envVar string, required bool) string {
+ var b strings.Builder
+ if !required {
+ b.WriteString("Optional. ")
+ }
+ b.WriteString("Sets ")
+ b.WriteString(envVar)
+ b.WriteString(" for the ")
+ if m.DisplayName != "" {
+ b.WriteString(displayNameForConcat(m.DisplayName))
+ } else if m.APIName != "" {
+ b.WriteString(m.APIName)
+ } else {
+ b.WriteString("CLI")
+ }
+ b.WriteString(" MCP server. Required by the generated client for credential refresh or hand-written auth flow.")
+ return b.String()
+}
diff --git a/internal/pipeline/client_env_scanner_test.go b/internal/pipeline/client_env_scanner_test.go
new file mode 100644
index 00000000..018c1ea0
--- /dev/null
+++ b/internal/pipeline/client_env_scanner_test.go
@@ -0,0 +1,473 @@
+package pipeline
+
+import (
+ "encoding/json"
+ "os"
+ "path/filepath"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestScanClientEnvReads(t *testing.T) {
+ t.Run("returns nil when internal/client dir missing", func(t *testing.T) {
+ got, err := scanClientEnvReads(t.TempDir())
+ require.NoError(t, err)
+ assert.Nil(t, got)
+ })
+
+ t.Run("returns sorted dedup set of os.Getenv args", func(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func mintToken() string {
+ id := os.Getenv("FEDEX_API_KEY")
+ if id == "" {
+ id = os.Getenv("FEDEX_API_KEY")
+ }
+ _ = os.Getenv("FEDEX_SECRET_KEY")
+ return id
+}
+`)
+ writeClientFile(t, dir, "auth_refresh.go", `package client
+
+import "os"
+
+func tryRefresh() (string, string) {
+ return os.Getenv("RENTALWORKS_HOME_USERNAME"), os.Getenv("RENTALWORKS_HOME_PASSWORD")
+}
+`)
+
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{
+ "FEDEX_API_KEY",
+ "FEDEX_SECRET_KEY",
+ "RENTALWORKS_HOME_PASSWORD",
+ "RENTALWORKS_HOME_USERNAME",
+ }, got)
+ })
+
+ t.Run("skips non-string-literal Getenv args", func(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+const tokenVar = "X_TOKEN"
+
+func read(name string) string {
+ _ = os.Getenv(name) // variable arg — skip
+ _ = os.Getenv(tokenVar) // identifier arg — skip
+ return os.Getenv("X_API_KEY")
+}
+`)
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{"X_API_KEY"}, got)
+ })
+
+ t.Run("ignores non-os Getenv calls and unrelated calls", func(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "client.go", `package client
+
+type fake struct{}
+
+func (f fake) Getenv(s string) string { return s }
+
+func read() string {
+ f := fake{}
+ _ = f.Getenv("LOOKS_LIKE_GETENV")
+ return ""
+}
+`)
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Empty(t, got)
+ })
+
+ t.Run("logs but continues past files that fail to parse", func(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "broken.go", `package client
+this is not go`)
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("GOOD_VAR") }
+`)
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{"GOOD_VAR"}, got)
+ })
+
+ t.Run("ignores non-go files and subdirs", func(t *testing.T) {
+ dir := t.TempDir()
+ clientDir := filepath.Join(dir, "internal", "client")
+ require.NoError(t, os.MkdirAll(filepath.Join(clientDir, "sub"), 0o755))
+ require.NoError(t, os.WriteFile(filepath.Join(clientDir, "notes.txt"), []byte("os.Getenv(\"IGNORE_ME\")"), 0o644))
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("PICK_ME") }
+`)
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{"PICK_ME"}, got)
+ })
+}
+
+func TestReconcileMCPBManifestFromClient(t *testing.T) {
+ t.Run("no manifest file is a no-op", func(t *testing.T) {
+ dir := t.TempDir()
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, CLIManifest{}))
+ })
+
+ t.Run("no client dir leaves manifest unchanged", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{APIName: "noop", MCPBinary: "noop-pp-mcp", AuthType: "api_key"}
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "noop-pp-mcp",
+ Server: MCPBServer{
+ MCPConfig: MCPBLaunchSpec{Env: map[string]string{"NOOP_API_KEY": "${user_config.noop_api_key}"}},
+ },
+ UserConfig: map[string]MCPBVar{
+ "noop_api_key": {Type: "string", Title: "NOOP_API_KEY", Required: true, Sensitive: true},
+ },
+ })
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ assert.Equal(t, map[string]string{"NOOP_API_KEY": "${user_config.noop_api_key}"}, got.Server.MCPConfig.Env)
+ assert.Len(t, got.UserConfig, 1)
+ })
+
+ t.Run("declared env vars are skipped", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{
+ APIName: "stripe",
+ DisplayName: "Stripe",
+ MCPBinary: "stripe-pp-mcp",
+ AuthType: "api_key",
+ }
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "stripe-pp-mcp",
+ Server: MCPBServer{
+ MCPConfig: MCPBLaunchSpec{Env: map[string]string{"STRIPE_API_KEY": "${user_config.stripe_api_key}"}},
+ },
+ UserConfig: map[string]MCPBVar{
+ "stripe_api_key": {Type: "string", Title: "STRIPE_API_KEY", Required: true, Sensitive: true},
+ },
+ })
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("STRIPE_API_KEY") }
+`)
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ assert.Len(t, got.Server.MCPConfig.Env, 1)
+ assert.Len(t, got.UserConfig, 1)
+ })
+
+ t.Run("adds sensitive user_config for undeclared env reads on required-credential auth", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{
+ APIName: "rentalworks-home",
+ DisplayName: "RentalWorks Home",
+ MCPBinary: "rentalworks-home-pp-mcp",
+ AuthType: "bearer_token",
+ AuthEnvVars: []string{"RENTALWORKS_HOME_TOKEN"},
+ }
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "rentalworks-home-pp-mcp",
+ Server: MCPBServer{
+ MCPConfig: MCPBLaunchSpec{Env: map[string]string{"RENTALWORKS_HOME_TOKEN": "${user_config.rentalworks_home_token}"}},
+ },
+ UserConfig: map[string]MCPBVar{
+ "rentalworks_home_token": {Type: "string", Title: "RENTALWORKS_HOME_TOKEN", Required: true, Sensitive: true},
+ },
+ })
+ writeClientFile(t, dir, "auth_refresh.go", `package client
+
+import "os"
+
+func refresh() (string, string) {
+ return os.Getenv("RENTALWORKS_HOME_USERNAME"), os.Getenv("RENTALWORKS_HOME_PASSWORD")
+}
+`)
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ assert.Equal(t, "${user_config.rentalworks_home_token}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_TOKEN"])
+ assert.Equal(t, "${user_config.rentalworks_home_username}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_USERNAME"])
+ assert.Equal(t, "${user_config.rentalworks_home_password}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_PASSWORD"])
+
+ username, ok := got.UserConfig["rentalworks_home_username"]
+ require.True(t, ok)
+ assert.Equal(t, "RENTALWORKS_HOME_USERNAME", username.Title)
+ assert.Equal(t, "string", username.Type)
+ assert.True(t, username.Sensitive)
+ assert.True(t, username.Required, "credential-required bearer_token auth must propagate Required to discovered fields")
+ assert.Contains(t, username.Description, "RentalWorks Home")
+ assert.Contains(t, username.Description, "credential refresh")
+ assert.NotContains(t, username.Description, "Optional.", "required-auth descriptions must not carry the Optional prefix")
+
+ password, ok := got.UserConfig["rentalworks_home_password"]
+ require.True(t, ok)
+ assert.True(t, password.Sensitive)
+ assert.True(t, password.Required)
+ })
+
+ t.Run("optional auth keeps discovered fields optional", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{
+ APIName: "recipe-goat",
+ DisplayName: "Recipe Goat",
+ MCPBinary: "recipe-goat-pp-mcp",
+ AuthType: "api_key",
+ AuthOptional: true,
+ }
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "recipe-goat-pp-mcp",
+ Server: MCPBServer{MCPConfig: MCPBLaunchSpec{Env: map[string]string{}}},
+ })
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("RECIPE_EXTRA_SECRET") }
+`)
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ entry, ok := got.UserConfig["recipe_extra_secret"]
+ require.True(t, ok)
+ assert.False(t, entry.Required, "AuthOptional=true must mark discovered fields optional")
+ assert.True(t, entry.Sensitive)
+ assert.Contains(t, entry.Description, "Optional.")
+ })
+
+ t.Run("composed auth marks discovered fields optional", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{
+ APIName: "pizza",
+ MCPBinary: "pizza-pp-mcp",
+ AuthType: "composed",
+ }
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "pizza-pp-mcp",
+ Server: MCPBServer{MCPConfig: MCPBLaunchSpec{Env: map[string]string{}}},
+ })
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("PIZZA_HIDDEN_TOKEN") }
+`)
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ entry, ok := got.UserConfig["pizza_hidden_token"]
+ require.True(t, ok)
+ assert.False(t, entry.Required, "composed auth keeps user_config optional")
+ })
+
+ t.Run("manifest with nil env/userconfig maps gets populated", func(t *testing.T) {
+ dir := t.TempDir()
+ cli := CLIManifest{APIName: "x", MCPBinary: "x-pp-mcp", AuthType: "api_key"}
+ writeMCPBManifest(t, dir, MCPBManifest{
+ Name: "x-pp-mcp",
+ Server: MCPBServer{},
+ })
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("X_HIDDEN") }
+`)
+
+ require.NoError(t, reconcileMCPBManifestFromClient(dir, cli))
+
+ got := readMCPBManifest(t, dir)
+ assert.Equal(t, "${user_config.x_hidden}", got.Server.MCPConfig.Env["X_HIDDEN"])
+ _, ok := got.UserConfig["x_hidden"]
+ assert.True(t, ok)
+ })
+}
+
+// TestWriteMCPBManifestFromStruct_ReconcilesClientEnvReads is the
+// integration guard: every WriteMCPBManifest* call site must invoke the
+// reconciler automatically. A regression that detaches reconcile from the
+// writer would let the lock+promote and bundle paths ship un-reconciled
+// manifests, reintroducing #859.
+func TestWriteMCPBManifestFromStruct_ReconcilesClientEnvReads(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "auth_refresh.go", `package client
+
+import "os"
+
+func refresh() (string, string) {
+ return os.Getenv("RENTALWORKS_HOME_USERNAME"), os.Getenv("RENTALWORKS_HOME_PASSWORD")
+}
+`)
+
+ m := CLIManifest{
+ APIName: "rentalworks-home",
+ DisplayName: "RentalWorks Home",
+ MCPBinary: "rentalworks-home-pp-mcp",
+ MCPReady: "full",
+ AuthType: "bearer_token",
+ AuthEnvVars: []string{"RENTALWORKS_HOME_TOKEN"},
+ }
+
+ require.NoError(t, WriteMCPBManifestFromStruct(dir, m))
+
+ got := readMCPBManifest(t, dir)
+ assert.Equal(t, "${user_config.rentalworks_home_token}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_TOKEN"])
+ assert.Equal(t, "${user_config.rentalworks_home_username}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_USERNAME"])
+ assert.Equal(t, "${user_config.rentalworks_home_password}", got.Server.MCPConfig.Env["RENTALWORKS_HOME_PASSWORD"])
+
+ for _, key := range []string{"rentalworks_home_username", "rentalworks_home_password"} {
+ entry, ok := got.UserConfig[key]
+ require.True(t, ok, "%s must be present in user_config", key)
+ assert.True(t, entry.Sensitive, "%s must be sensitive", key)
+ assert.True(t, entry.Required, "%s must be required when base auth requires credential", key)
+ assert.NotContains(t, entry.Description, "Optional.", "%s must not carry Optional prefix on required auth", key)
+ }
+}
+
+// TestWriteMCPBManifest_DiskReadVariantReconciles guards the
+// disk-read entry point used by lock.go, publish.go, and mcpsync. A
+// regression that broke `WriteMCPBManifest`'s delegation to
+// `WriteMCPBManifestFromStruct` would skip reconcile silently for the
+// most common call shape.
+func TestWriteMCPBManifest_DiskReadVariantReconciles(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "auth_refresh.go", `package client
+
+import "os"
+
+func u() (string, string) {
+ return os.Getenv("X_USERNAME"), os.Getenv("X_PASSWORD")
+}
+`)
+ writeManifest(t, dir, CLIManifest{
+ APIName: "x",
+ MCPBinary: "x-pp-mcp",
+ MCPReady: "full",
+ AuthType: "bearer_token",
+ AuthEnvVars: []string{"X_TOKEN"},
+ })
+
+ require.NoError(t, WriteMCPBManifest(dir))
+
+ got := readMCPBManifest(t, dir)
+ assert.Equal(t, "${user_config.x_username}", got.Server.MCPConfig.Env["X_USERNAME"])
+ assert.Equal(t, "${user_config.x_password}", got.Server.MCPConfig.Env["X_PASSWORD"])
+ for _, key := range []string{"x_username", "x_password"} {
+ entry, ok := got.UserConfig[key]
+ require.True(t, ok, "%s must be in user_config after disk-read writer path", key)
+ assert.True(t, entry.Required, "%s must inherit required from bearer_token auth", key)
+ }
+}
+
+// TestWriteMCPBManifestFromStruct_AuthOptionalPropagates guards the
+// in-memory CLIManifest plumbing — a regression that dropped AuthOptional
+// between the writer and the reconciler would mark discovered fields
+// Required on optional-auth APIs.
+func TestWriteMCPBManifestFromStruct_AuthOptionalPropagates(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "client.go", `package client
+
+import "os"
+
+func read() string { return os.Getenv("OPTIONAL_HIDDEN") }
+`)
+ m := CLIManifest{
+ APIName: "optional-cli",
+ MCPBinary: "optional-cli-pp-mcp",
+ MCPReady: "full",
+ AuthType: "api_key",
+ AuthOptional: true,
+ }
+
+ require.NoError(t, WriteMCPBManifestFromStruct(dir, m))
+
+ got := readMCPBManifest(t, dir)
+ entry, ok := got.UserConfig["optional_hidden"]
+ require.True(t, ok)
+ assert.False(t, entry.Required, "AuthOptional=true must propagate through the writer entry point")
+ assert.Contains(t, entry.Description, "Optional.")
+}
+
+// TestWriteMCPBManifestFromStruct_IdempotentReconcile ensures running the
+// writer twice on the same dir produces identical output bytes — the
+// reconciler must not double-append or churn fields.
+func TestWriteMCPBManifestFromStruct_IdempotentReconcile(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "auth_refresh.go", `package client
+
+import "os"
+
+func u() string { return os.Getenv("X_USERNAME") }
+`)
+
+ m := CLIManifest{
+ APIName: "x",
+ MCPBinary: "x-pp-mcp",
+ MCPReady: "full",
+ AuthType: "bearer_token",
+ AuthEnvVars: []string{"X_TOKEN"},
+ }
+
+ require.NoError(t, WriteMCPBManifestFromStruct(dir, m))
+ first, err := os.ReadFile(filepath.Join(dir, MCPBManifestFilename))
+ require.NoError(t, err)
+
+ require.NoError(t, WriteMCPBManifestFromStruct(dir, m))
+ second, err := os.ReadFile(filepath.Join(dir, MCPBManifestFilename))
+ require.NoError(t, err)
+
+ assert.Equal(t, string(first), string(second), "reconcile must be idempotent across consecutive writer runs")
+}
+
+// TestScanClientEnvReadsBacktickLiteral guards against a regression that
+// would drop backtick-quoted env var names. strconv.Unquote handles both
+// forms, but the integration is worth a fixture in case the parser path
+// changes.
+func TestScanClientEnvReadsBacktickLiteral(t *testing.T) {
+ dir := t.TempDir()
+ writeClientFile(t, dir, "client.go", "package client\n\nimport \"os\"\n\nfunc read() string { return os.Getenv(`BACKTICK_VAR`) }\n")
+ got, err := scanClientEnvReads(dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{"BACKTICK_VAR"}, got)
+}
+
+func writeClientFile(t *testing.T, dir, name, content string) {
+ t.Helper()
+ clientDir := filepath.Join(dir, "internal", "client")
+ require.NoError(t, os.MkdirAll(clientDir, 0o755))
+ require.NoError(t, os.WriteFile(filepath.Join(clientDir, name), []byte(content), 0o644))
+}
+
+// Sanity check that MCPBVar json round-trips the new Sensitive+Required flags.
+func TestMCPBVarRoundtripFlags(t *testing.T) {
+ in := MCPBVar{Type: "string", Title: "X", Sensitive: true, Required: true}
+ data, err := json.Marshal(in)
+ require.NoError(t, err)
+ var out MCPBVar
+ require.NoError(t, json.Unmarshal(data, &out))
+ assert.Equal(t, in, out)
+}
diff --git a/internal/pipeline/lock.go b/internal/pipeline/lock.go
index 2b7f42de..31ae7e51 100644
--- a/internal/pipeline/lock.go
+++ b/internal/pipeline/lock.go
@@ -276,9 +276,15 @@ func PromoteWorkingCLI(cliName, workingDir string, state *PipelineState) error {
}
// Refresh the MCPB manifest.json in the staging dir so the lock-and-promote
- // flow keeps it in sync with the post-publish CLIManifest fields.
+ // flow keeps it in sync with the post-publish CLIManifest fields. The
+ // writer also reconciles against env reads in internal/client/ so the
+ // staged bundle includes any user_config fields the spec did not surface.
+ // Errors abort the promote rather than warn-and-continue — a reconcile
+ // failure here means the published bundle would ship missing user_config
+ // fields, which is the exact bug class this writer chain exists to prevent.
if err := WriteMCPBManifest(stagingDir); err != nil {
- fmt.Fprintf(os.Stderr, "warning: could not write MCPB manifest.json: %v\n", err)
+ _ = os.RemoveAll(stagingDir)
+ return fmt.Errorf("writing MCPB manifest to staging: %w", err)
}
// Remove any stale backup from a prior successful swap before we create a
diff --git a/internal/pipeline/mcpb_manifest.go b/internal/pipeline/mcpb_manifest.go
index 4ed2f57e..4544b262 100644
--- a/internal/pipeline/mcpb_manifest.go
+++ b/internal/pipeline/mcpb_manifest.go
@@ -151,15 +151,34 @@ func WriteMCPBManifestFromStruct(dir string, m CLIManifest) error {
if m.MCPBinary == "" {
return nil
}
- // SetEscapeHTML(false) so `>=1.0.0` stays readable instead of `>=1.0.0`.
+ out, err := marshalMCPBManifest(buildMCPBManifest(dir, m))
+ if err != nil {
+ return err
+ }
+ if err := os.WriteFile(filepath.Join(dir, MCPBManifestFilename), out, 0o644); err != nil {
+ return err
+ }
+ // Extend the just-written manifest with env vars read by
+ // internal/client/*.go that the spec-driven build didn't surface
+ // (credential-flow JWT refreshers, hand-written auth helpers, etc.).
+ // Runs from every writer call site so the bundle path reads a
+ // reconciled manifest regardless of whether it came through lock+promote
+ // or a one-off bundle build.
+ return reconcileMCPBManifestFromClient(dir, m)
+}
+
+// marshalMCPBManifest serializes an MCPBManifest with the same encoder
+// settings the writer uses end-to-end. SetEscapeHTML(false) so `>=1.0.0`
+// stays readable instead of `>=1.0.0`.
+func marshalMCPBManifest(manifest MCPBManifest) ([]byte, error) {
var buf bytes.Buffer
enc := json.NewEncoder(&buf)
enc.SetEscapeHTML(false)
enc.SetIndent("", " ")
- if err := enc.Encode(buildMCPBManifest(dir, m)); err != nil {
- return fmt.Errorf("marshaling MCPB manifest: %w", err)
+ if err := enc.Encode(manifest); err != nil {
+ return nil, fmt.Errorf("marshaling MCPB manifest: %w", err)
}
- return os.WriteFile(filepath.Join(dir, MCPBManifestFilename), buf.Bytes(), 0o644)
+ return buf.Bytes(), nil
}
func buildMCPBManifest(dir string, m CLIManifest) MCPBManifest {
← dc5e8c55 fix(cli): gate sync since-param emission per resource (#1036
·
back to Cli Printing Press
·
fix(cli): hide raw resource groups when api browser is gener ed693b0d →