From 4d244b8953202d252bbc89f3cf0eaf09465f3cca Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 00:44:27 -0700 Subject: [PATCH 01/14] fix(config): identify installations by ksuid The control plane mints installation ids as KSUIDs and the edge routes on them, so the canonical-lowercase-UUID grammar rejected every real installation. The check stays a shape check rather than a decode, so this client cannot end up stricter than the router it talks to; a test pins that limit deliberately. --- internal/config/connection.go | 18 ++++++++++---- internal/config/connection_test.go | 38 +++++++++++++++++++++++------- internal/config/oracle_test.go | 6 ++--- internal/execctx/execctx_test.go | 2 +- internal/server/server_test.go | 2 +- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/internal/config/connection.go b/internal/config/connection.go index 866444b..08e409f 100644 --- a/internal/config/connection.go +++ b/internal/config/connection.go @@ -12,10 +12,18 @@ import ( // credentials may be sent is an escape hatch that outlives its reason. const HostedOrigin = "https://cloud.formae.ai" -// installationRE matches the canonical lowercase UUID text form. The constraint -// is syntactic and mirrors what the edge accepts as a routing key; it says -// nothing about UUID version or variant bits. -var installationRE = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) +// installationRE is the routing-key grammar: 27 base62 characters, case +// sensitive, which is the text form of a KSUID. It mirrors byte for byte what +// the edge accepts as a routing key. +// +// It is a shape check and deliberately not a decode. 27 base62 digits span a +// wider range than the 160 bits a KSUID encodes, so a few strings this accepts +// would fail a KSUID parser. Refusing them would make this client stricter than +// the edge that does the routing, so we would refuse an identifier the router +// would have accepted, and gain nothing: nothing mints one that cannot be +// decoded, and a well-formed identifier that is not routable comes back from +// the edge as a 404 that says so. +var installationRE = regexp.MustCompile(`^[0-9A-Za-z]{27}$`) // Connection is where the MCP sends agent requests. It has exactly two arms, so // a resolved configuration cannot be both classic and hosted, and a hosted one @@ -49,7 +57,7 @@ func ValidateHosted(h Hosted) error { return err } if !installationRE.MatchString(h.Installation) { - return fmt.Errorf("installation %q is not a canonical lowercase UUID", h.Installation) + return fmt.Errorf("installation %q is not a well-formed installation id", h.Installation) } return nil } diff --git a/internal/config/connection_test.go b/internal/config/connection_test.go index 2d523dd..90ef209 100644 --- a/internal/config/connection_test.go +++ b/internal/config/connection_test.go @@ -5,7 +5,7 @@ import "testing" func TestValidateHosted_AcceptsCanonicalEndpointAndInstallation(t *testing.T) { h := Hosted{ Endpoint: "https://cloud.formae.ai", - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } if err := ValidateHosted(h); err != nil { t.Fatalf("expected valid hosted connection, got error: %v", err) @@ -29,7 +29,7 @@ func TestValidateHosted_RejectsBadEndpoints(t *testing.T) { t.Run(name, func(t *testing.T) { err := ValidateHosted(Hosted{ Endpoint: endpoint, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", }) if err == nil { t.Fatalf("expected %s endpoint %q to be rejected", name, endpoint) @@ -40,12 +40,18 @@ func TestValidateHosted_RejectsBadEndpoints(t *testing.T) { func TestValidateHosted_RejectsBadInstallations(t *testing.T) { cases := map[string]string{ - "uppercase": "3F2B8C14-0000-4000-8000-000000000000", - "braces": "{3f2b8c14-0000-4000-8000-000000000000}", - "too short": "3f2b8c14-0000-4000-8000-00000000000", - "not a uuid": "default", - "empty": "", - "with spaces": "3f2b8c14-0000-4000-8000-000000000000 ", + // The format installations used to carry. Nothing mints one now, so a + // profile naming one addresses an installation that cannot exist. + "the retired uuid form": "3f2b8c14-0000-4000-8000-000000000000", + "braces": "{3HzFPXfPDGhwLJJVtaHbmFs6vLa}", + "one short": "3HzFPXfPDGhwLJJVtaHbmFs6vL", + "one long": "3HzFPXfPDGhwLJJVtaHbmFs6vLaa", + "a hyphen": "3HzFPXfPDGhwLJJVtaHbmFs6v-a", + "an underscore": "3HzFPXfPDGhwLJJVtaHbmFs6v_a", + "not an installation": "default", + "empty": "", + "trailing space": "3HzFPXfPDGhwLJJVtaHbmFs6vL ", + "a newline": "3HzFPXfPDGhwLJJVtaHbmFs6vLa\n", } for name, id := range cases { t.Run(name, func(t *testing.T) { @@ -56,3 +62,19 @@ func TestValidateHosted_RejectsBadInstallations(t *testing.T) { }) } } + +// The check is the routing key's grammar, not a decode. 27 base62 digits span a +// wider range than the 160 bits a KSUID encodes, so a few well-formed strings +// would fail a KSUID parser. Refusing them here would make this client stricter +// than the edge that does the routing, which validates the same grammar: we +// would refuse an identifier the router accepts and gain nothing, because +// nothing mints one that cannot be decoded. Pinned so the limit is a decision. +func TestValidateHosted_ChecksTheRoutingGrammarNotADecode(t *testing.T) { + err := ValidateHosted(Hosted{ + Endpoint: HostedOrigin, + Installation: "zzzzzzzzzzzzzzzzzzzzzzzzzzz", + }) + if err != nil { + t.Fatalf("a well-formed identifier must be accepted without decoding it: %v", err) + } +} diff --git a/internal/config/oracle_test.go b/internal/config/oracle_test.go index 8191760..8952bb1 100644 --- a/internal/config/oracle_test.go +++ b/internal/config/oracle_test.go @@ -15,7 +15,7 @@ const classicView = `{"schemaVersion":1,"profile":"dev", const hostedView = `{"schemaVersion":1,"profile":"prod", "cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai", - "installation":"3f2b8c14-0000-4000-8000-000000000000"}}}` + "installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}}` func TestDecodeProfileShow_Classic(t *testing.T) { got, err := decodeProfileShow([]byte(classicView)) @@ -41,7 +41,7 @@ func TestDecodeProfileShow_Hosted(t *testing.T) { } want := Hosted{ Endpoint: "https://cloud.formae.ai", - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } if got.Conn != Connection(want) { t.Errorf("connection: want %#v, got %#v", want, got.Conn) @@ -72,7 +72,7 @@ func TestDecodeProfileShow_Rejects(t *testing.T) { {"unknown mode", `{"schemaVersion":1,"cli":{"connection":{"mode":"orbital"}}}`}, {"missing connection", `{"schemaVersion":1,"profile":"dev","cli":{}}`}, {"hosted without installation", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai"}}}`}, - {"hosted with a foreign endpoint", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3f2b8c14-0000-4000-8000-000000000000"}}}`}, + {"hosted with a foreign endpoint", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}}`}, {"not json", `formae: command not found`}, {"classic with an empty url", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","port":49684}}}`}, {"classic with no port", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","url":"http://localhost"}}}`}, diff --git a/internal/execctx/execctx_test.go b/internal/execctx/execctx_test.go index a769a1b..8b2eb5a 100644 --- a/internal/execctx/execctx_test.go +++ b/internal/execctx/execctx_test.go @@ -53,7 +53,7 @@ func TestResolveCarriesTheConnection(t *testing.T) { func TestResolveHosted(t *testing.T) { hosted := config.Hosted{ Endpoint: config.HostedOrigin, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } r := &Resolver{ resolve: func(context.Context, string, string) (config.Resolved, error) { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 12ab851..a96a300 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -835,7 +835,7 @@ func TestClientFor_RefusesHosted(t *testing.T) { ProfileName: "acme-prod", Conn: config.Hosted{ Endpoint: config.HostedOrigin, - Installation: "3f2b8c14-0000-4000-8000-000000000000", + Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", }, }} s := New("") From 19c019221c4c2a3bc8c9b31c350c3218adbf10e6 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 00:46:22 -0700 Subject: [PATCH 02/14] feat(secret): add a credential type that masks in every rendering path The invariant that no path writes a credential into a result, an error, or a log cannot be established by a test, because the next handler someone adds is not covered by it. It becomes a property of the type instead: String, Format, MarshalJSON, MarshalYAML and LogValue all mask, the field is unexported so a reflection-based encoder sees nothing, and Reveal is the single greppable way out. --- internal/secret/secret.go | 73 +++++++++++++++++++ internal/secret/secret_test.go | 129 +++++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 internal/secret/secret.go create mode 100644 internal/secret/secret_test.go diff --git a/internal/secret/secret.go b/internal/secret/secret.go new file mode 100644 index 0000000..509df27 --- /dev/null +++ b/internal/secret/secret.go @@ -0,0 +1,73 @@ +// Package secret holds a credential in a type that cannot be printed by +// accident. +// +// The invariant this exists for — no code path writes a credential into a tool +// result, an error, or a log — is not something a test can establish, because +// the next handler someone adds is not covered by it. So it is a property of +// the type instead: every rendering path masks, and there is exactly one +// accessor, which is greppable. +package secret + +import ( + "fmt" + "log/slog" +) + +// Mask is what a credential renders as. It says a value is present and +// withheld, which is more useful than an empty string that reads as absent. +const Mask = "" + +// Value holds a credential. +// +// Methods take value receivers so a copy cannot lose the masking: a Value +// travels into structs, closures and interface values constantly, and a pointer +// receiver would leave every copy rendering its raw field. +type Value struct { + // v is unexported, so encoding/json and any other reflection-based encoder + // sees a struct with no exported fields even before the marshallers below + // are consulted. + v string +} + +// New wraps a credential. +func New(s string) Value { return Value{v: s} } + +// String masks. Format below covers the verbs String does not. +func (val Value) String() string { return Mask } + +// Format masks every verb, including %#v and %q. +// +// String alone is not enough: fmt consults Stringer only for %v and %s, so %#v +// would otherwise print the struct with its field, which is precisely the +// rendering a developer reaches for when debugging the thing that holds a +// credential. +func (val Value) Format(f fmt.State, verb rune) { + switch verb { + case 'q': + _, _ = fmt.Fprintf(f, "%q", Mask) + default: + _, _ = fmt.Fprint(f, Mask) + } +} + +// MarshalJSON masks. Serialisation is the leak path that matters most, because +// it happens to a whole struct without anyone naming the field. +func (val Value) MarshalJSON() ([]byte, error) { + return []byte(`"` + Mask + `"`), nil +} + +// MarshalYAML masks. Nothing here marshals YAML today; the method costs one +// line and no dependency, and closes the path before something does. +func (val Value) MarshalYAML() (any, error) { return Mask, nil } + +// LogValue masks. slog resolves this instead of formatting the value, so a +// credential passed as a log attribute never reaches the handler. +func (val Value) LogValue() slog.Value { return slog.StringValue(Mask) } + +// Reveal returns the credential. This is the only way out, and the only place +// worth auditing: grep for it. +func (val Value) Reveal() string { return val.v } + +// IsZero reports whether no credential is held, which is what a classic +// connection carries. +func (val Value) IsZero() bool { return val.v == "" } diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go new file mode 100644 index 0000000..a674997 --- /dev/null +++ b/internal/secret/secret_test.go @@ -0,0 +1,129 @@ +package secret + +import ( + "bytes" + "encoding/json" + "fmt" + "log/slog" + "strings" + "testing" +) + +// token is recognisable enough that any leak shows up in a substring check. +const token = "Bearer sup3rs3cr3t-do-not-print-me" + +// assertHidden fails when the rendering leaked the token, and also when it did +// not produce the mask: a rendering that silently produced nothing at all would +// pass a leak check while telling the reader nothing. +// +// The mask is matched in its escaped form too, because encoding/json escapes +// the angle brackets by default. That is the encoder being correct, not the +// mask failing, and a consumer decodes it straight back. +func assertHidden(t *testing.T, what, got string) { + t.Helper() + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("%s leaked the credential: %s", what, got) + } + escaped := strings.NewReplacer("<", `\u003c`, ">", `\u003e`).Replace(Mask) + if !strings.Contains(got, Mask) && !strings.Contains(got, escaped) { + t.Fatalf("%s did not render the mask, got: %s", what, got) + } +} + +func TestValueMasksEveryFormattingVerb(t *testing.T) { + v := New(token) + + assertHidden(t, "Sprint", fmt.Sprint(v)) + assertHidden(t, "%v", fmt.Sprintf("%v", v)) + assertHidden(t, "%s", fmt.Sprintf("%s", v)) + assertHidden(t, "%q", fmt.Sprintf("%q", v)) + assertHidden(t, "%#v", fmt.Sprintf("%#v", v)) + assertHidden(t, "%+v", fmt.Sprintf("%+v", v)) + assertHidden(t, "String", v.String()) +} + +// The realistic leak is a struct reaching an encoder, not a deliberate print. +// Masking String alone does not stop encoding/json walking a field. +func TestValueMasksInsideAStruct(t *testing.T) { + held := struct { + Profile string + Credential Value + }{Profile: "prod", Credential: New(token)} + + out, err := json.Marshal(held) + if err != nil { + t.Fatalf("marshalling a struct holding a credential: %v", err) + } + assertHidden(t, "json.Marshal of a containing struct", string(out)) + + pretty, err := json.MarshalIndent(held, "", " ") + if err != nil { + t.Fatalf("marshal indent: %v", err) + } + assertHidden(t, "json.MarshalIndent of a containing struct", string(pretty)) +} + +func TestValueMasksInJSONAndYAML(t *testing.T) { + v := New(token) + + out, err := json.Marshal(v) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + assertHidden(t, "json.Marshal", string(out)) + + y, err := v.MarshalYAML() + if err != nil { + t.Fatalf("MarshalYAML: %v", err) + } + assertHidden(t, "MarshalYAML", fmt.Sprint(y)) +} + +func TestValueMasksInStructuredLogs(t *testing.T) { + var buf bytes.Buffer + slog.New(slog.NewJSONHandler(&buf, nil)). + Info("resolved", "credential", New(token)) + + assertHidden(t, "slog", buf.String()) +} + +func TestValueMasksWhenWrappedInAnError(t *testing.T) { + err := fmt.Errorf("resolving the connection: %w", + fmt.Errorf("credential %v was refused", New(token))) + + assertHidden(t, "a wrapped error", err.Error()) +} + +func TestRevealIsTheOnlyWayOut(t *testing.T) { + if got := New(token).Reveal(); got != token { + t.Fatalf("Reveal must return the credential verbatim, got %q", got) + } +} + +// The zero value is what a classic connection carries, so it must be safe to +// hold, render, and ask about without anyone reaching for a nil check. +func TestZeroValueIsUsable(t *testing.T) { + var v Value + + if !v.IsZero() { + t.Fatal("the zero value must report IsZero") + } + if got := v.Reveal(); got != "" { + t.Fatalf("the zero value must reveal the empty string, got %q", got) + } + if New(token).IsZero() { + t.Fatal("a value holding a credential must not report IsZero") + } +} + +// A copy is the normal way a Value travels: into a struct, through a channel, +// into a closure. Masking must survive it, which is what value receivers buy. +func TestACopyStillMasks(t *testing.T) { + original := New(token) + copied := original + + assertHidden(t, "a copy", fmt.Sprintf("%v", copied)) + if copied.Reveal() != token { + t.Fatal("a copy must still carry the credential") + } +} From 7f1a6a85ce983d00e1aadd78427c44761cbbd0d6 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 00:51:39 -0700 Subject: [PATCH 03/14] feat(config): resolve the connection and its credential in one read The oracle moves from `formae profile show` to `formae connection resolve`, which produces both from one evaluation of one profile. Two independently timed reads could not: between them the active pointer can move or the profile can be rewritten, and a request would carry one revision's endpoint with another's credential. Three things about the new contract are easy to get wrong and are pinned by tests. --profile is a local flag and follows the subcommand, where profile show took the name positionally. The document is flat rather than nested under cli. A declared failure is an envelope on the same stdout, so a non-zero exit is read rather than reported blind. The producer's free-text message is never surfaced: it is built from an auth plugin's error string or an arbitrary err.Error(), and a Pkl failure quotes profile source lines, which for a classic profile can mean an inline password. Only the code and a validated plugin code cross over. --- internal/config/failure.go | 165 ++++++++++++++ internal/config/oracle.go | 124 +++++++---- internal/config/oracle_test.go | 354 +++++++++++++++++++++++++------ internal/execctx/execctx.go | 26 ++- internal/execctx/execctx_test.go | 67 +++++- internal/server/server.go | 7 +- internal/server/server_test.go | 16 +- 7 files changed, 640 insertions(+), 119 deletions(-) create mode 100644 internal/config/failure.go diff --git a/internal/config/failure.go b/internal/config/failure.go new file mode 100644 index 0000000..d52fcaf --- /dev/null +++ b/internal/config/failure.go @@ -0,0 +1,165 @@ +package config + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "strings" + + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// failureSchemaVersion is the envelope shape this build understands. It is +// checked before any other field, exactly as the success document's is. +const failureSchemaVersion = 1 + +// diagnose is the invitation attached to every declared failure. It is the same +// command, run by hand, and its human output is deliberately not redacted the +// way this consumer is: a person looking at their own profile in their own +// terminal is not the exposure we are closing. +const diagnose = "run `formae connection resolve` to see why" + +// declaredCodes is the closed namespace the producer promises. A code outside +// it is a protocol mismatch, not a message to pass along. +var declaredCodes = map[string]bool{ + "ambiguous_profile": true, + "auth_failed": true, + "untrusted_issuer": true, + "no_connection": true, + "internal": true, +} + +// declaredPluginCodes is what an auth plugin may report. +// +// This is validated rather than trusted, unlike the top-level code, because the +// producer does not close it: the plugin's error code is a bare string alias +// copied through without a membership check, so `details.pluginCode` is an open +// channel wearing a closed channel's name. A plugin that put a token there +// would otherwise route it straight into a tool result. +var declaredPluginCodes = map[string]bool{ + "unsupported": true, + "not_logged_in": true, + "session_expired": true, + "issuer_unreachable": true, +} + +// unrecognisedPluginCode replaces a plugin code we did not declare. It says a +// code was present and withheld, which is all a reader can safely be told. +const unrecognisedPluginCode = "unrecognised" + +// ResolveError is a failure the CLI declared. It carries the code so a caller +// can branch on it, and never the producer's free-text message. +type ResolveError struct { + Code string + // PluginCode is the auth plugin's own code for an auth_failed, validated + // against the declared set. Empty when absent or unrecognised. + PluginCode string +} + +func (e *ResolveError) Error() string { + switch e.Code { + case "auth_failed": + if e.PluginCode != "" { + return fmt.Sprintf("formae could not obtain a credential for this profile (%s); %s", + e.PluginCode, diagnose) + } + return fmt.Sprintf("formae could not obtain a credential for this profile; %s", diagnose) + case "untrusted_issuer": + return fmt.Sprintf( + "this profile's hosted connection names an issuer this build will not authenticate against; %s", + diagnose) + case "no_connection": + return fmt.Sprintf("this profile resolves no connection formae can use; %s", diagnose) + default: + return fmt.Sprintf("formae could not resolve the connection; %s", diagnose) + } +} + +// AmbiguousProfileError is the CLI refusing to guess which installation was +// meant. Its message is the instruction the caller acts on, so a model reading +// it can retry with the argument rather than needing to be told separately. +type AmbiguousProfileError struct { + Candidates []string + Active string +} + +func (e *AmbiguousProfileError) Error() string { + listed := make([]string, 0, len(e.Candidates)) + for _, c := range e.Candidates { + if c == e.Active { + c += " (active)" + } + listed = append(listed, c) + } + return fmt.Sprintf( + "more than one profile exists and none was named, so formae cannot tell which "+ + "installation you meant. Pass the profile argument on this call: %s", + strings.Join(listed, ", ")) +} + +// failureView is the envelope the producer emits on stdout when it fails. +type failureView struct { + SchemaVersion *int `json:"schemaVersion"` + Code string `json:"code"` + Details struct { + Candidates []string `json:"candidates"` + Active string `json:"active"` + PluginCode string `json:"pluginCode"` + } `json:"details"` + // Message is decoded so it is visibly accounted for, and deliberately never + // read: the producer builds it from an auth plugin's error string or an + // arbitrary err.Error(), and a Pkl failure quotes profile source lines, + // which for a classic profile can mean an inline password. + Message string `json:"message"` +} + +// decodeFailure turns a non-zero exit into a typed error. +// +// exitStatus names the failure when the envelope cannot be read at all, which +// is a supported path rather than a defensive one: argv the command cannot +// parse fails before the flags that say how to render a failure exist, so it +// exits non-zero with no envelope. The raw bytes never reach the error. +func decodeFailure(stdout []byte, exitStatus int) error { + unreadable := fmt.Errorf("formae could not resolve the connection (exit %d); %s", + exitStatus, diagnose) + + var v failureView + dec := json.NewDecoder(bytes.NewReader(stdout)) + if err := dec.Decode(&v); err != nil { + return unreadable + } + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return unreadable + } + if v.SchemaVersion == nil || *v.SchemaVersion != failureSchemaVersion { + return unreadable + } + if !declaredCodes[v.Code] { + return unreadable + } + + if v.Code == "ambiguous_profile" { + return &AmbiguousProfileError{ + Candidates: v.Details.Candidates, + Active: v.Details.Active, + } + } + + pluginCode := v.Details.PluginCode + if pluginCode != "" && !declaredPluginCodes[pluginCode] { + pluginCode = unrecognisedPluginCode + } + return &ResolveError{Code: v.Code, PluginCode: pluginCode} +} + +// Resolved is one profile evaluation: the effective profile name the CLI +// reported, the connection it resolved, and the credential that reaches it. +type Resolved struct { + Profile string + Conn Connection + // Credential is the zero value for classic: the MCP sends a self-hosted + // agent none, and that is a non-goal rather than an omission. + Credential secret.Value +} diff --git a/internal/config/oracle.go b/internal/config/oracle.go index e8af92f..2a4484b 100644 --- a/internal/config/oracle.go +++ b/internal/config/oracle.go @@ -12,6 +12,7 @@ import ( "time" "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) // schemaVersion is the machine-view major version this build understands. It is @@ -28,32 +29,34 @@ const oracleTimeout = 10 * time.Second var errOutputTooLarge = errors.New("formae produced more output than expected") -// Resolved is one profile evaluation: the effective profile name the CLI -// reported, and the connection it resolved. -type Resolved struct { - Profile string - Conn Connection -} - // resolveVia runs the CLI as the configuration oracle and decodes its machine // view. Stdout and stderr are captured separately and drained concurrently: // combined capture would put subprocess output into errors, and reading two // pipes serially can deadlock when the child fills the one not being read. -func resolveVia(ctx context.Context, bin, profileName string) (Resolved, error) { +// +// One command produces both the connection and the credential. Two +// independently timed reads could not: between them the active pointer can +// move, the profile can be rewritten, or the auth block can change, and the +// request would carry an endpoint from one revision with a credential from +// another — for hosted, one installation's endpoint with another's credential. +func resolveVia(ctx context.Context, bin, profileName string, forceRefresh bool) (Resolved, error) { if _, ok := ctx.Deadline(); !ok { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, oracleTimeout) defer cancel() } - // `profile show []` takes the name positionally. There is no - // --profile flag on it, and none on the root command either, so a flag form - // would not parse. - args := []string{"profile", "show"} + // --profile is a local flag on `resolve`, registered by AddConfigFlags, so + // it follows the subcommand. This is the opposite of `profile show`, where + // the name is positional and no such flag exists. + args := []string{"connection", "resolve"} if profileName != "" { - args = append(args, profileName) + args = append(args, "--profile", profileName) } args = append(args, "--output-consumer", "machine", "--output-schema", "json") + if forceRefresh { + args = append(args, "--force-refresh") + } cmd := exec.CommandContext(ctx, bin, args...) // Own process group, so a deadline kills the CLI and anything it spawned. @@ -106,35 +109,43 @@ func resolveVia(ctx context.Context, bin, profileName string) (Resolved, error) return Resolved{}, fmt.Errorf("reading configuration from %s: %w", bin, out.err) } if waitErr != nil { - // Reports the failure and the exit status, never the bytes. + // A declared failure is an envelope on this same stdout, so a non-zero + // exit is read rather than reported blind. decodeFailure falls back to + // the exit status when there is no envelope to read, and the bytes + // never reach an error either way. var exitErr *exec.ExitError if errors.As(waitErr, &exitErr) { - return Resolved{}, fmt.Errorf("formae could not resolve the configuration (exit %d)", exitErr.ExitCode()) + return Resolved{}, decodeFailure(out.data, exitErr.ExitCode()) } return Resolved{}, fmt.Errorf("formae could not resolve the configuration: %w", waitErr) } - return decodeProfileShow(out.data) + return decodeResolved(out.data) } -// profileShowView is the subset of the machine view this code reads. Unknown -// fields are ignored so the producer can add fields without a break. -type profileShowView struct { +// resolvedView is the subset of the machine view this code reads. Unknown +// fields are ignored so the producer can add fields without a break — the +// hosted arm's `auth` object is one such field, reduced to a type discriminator +// this consumer has no use for. +// +// The connection is at the top level. `profile show` nests it under `cli`; +// `connection resolve` does not, and reading the wrong shape would silently +// yield no connection at all. +type resolvedView struct { SchemaVersion *int `json:"schemaVersion"` Profile string `json:"profile"` - Cli struct { - Connection *struct { - Mode string `json:"mode"` - URL string `json:"url"` - Port int `json:"port"` - Endpoint string `json:"endpoint"` - Installation string `json:"installation"` - } `json:"connection"` - } `json:"cli"` + Connection *struct { + Mode string `json:"mode"` + URL string `json:"url"` + Port int `json:"port"` + Endpoint string `json:"endpoint"` + Installation string `json:"installation"` + } `json:"connection"` + Credential string `json:"credential"` } -func decodeProfileShow(data []byte) (Resolved, error) { - var v profileShowView +func decodeResolved(data []byte) (Resolved, error) { + var v resolvedView dec := json.NewDecoder(bytes.NewReader(data)) if err := dec.Decode(&v); err != nil { return Resolved{}, errors.New("formae returned output this version cannot read") @@ -152,45 +163,66 @@ func decodeProfileShow(data []byte) (Resolved, error) { "formae output uses schema version %d; this build understands %d", *v.SchemaVersion, schemaVersion) } - if v.Cli.Connection == nil { - return Resolved{}, errors.New("formae output carries no cli.connection") + if v.Connection == nil { + return Resolved{}, errors.New("formae output carries no connection") } - switch v.Cli.Connection.Mode { + switch v.Connection.Mode { case "classic": // Classic gets a minimum check too. An empty URL would produce a // valid-looking connection and defer the failure to request // construction, which reports it far from its cause. - if v.Cli.Connection.URL == "" { + if v.Connection.URL == "" { return Resolved{}, errors.New("formae reported a classic connection with no url") } - if v.Cli.Connection.Port <= 0 { - return Resolved{}, fmt.Errorf("formae reported an unusable port %d", v.Cli.Connection.Port) + if v.Connection.Port <= 0 { + return Resolved{}, fmt.Errorf("formae reported an unusable port %d", v.Connection.Port) + } + // The MCP sends a self-hosted agent no credential. Refusing one here + // rather than dropping it keeps that non-goal out of reach of a + // producer bug, instead of leaving a token in a value that might later + // grow a path to a header. + if v.Credential != "" { + return Resolved{}, errors.New("formae reported a credential for a classic connection") } return Resolved{ Profile: v.Profile, - Conn: Classic{URL: v.Cli.Connection.URL, Port: v.Cli.Connection.Port}, + Conn: Classic{URL: v.Connection.URL, Port: v.Connection.Port}, }, nil case "hosted": h := Hosted{ - Endpoint: v.Cli.Connection.Endpoint, - Installation: v.Cli.Connection.Installation, + Endpoint: v.Connection.Endpoint, + Installation: v.Connection.Installation, } if err := ValidateHosted(h); err != nil { return Resolved{}, fmt.Errorf("hosted connection is not usable: %w", err) } - return Resolved{Profile: v.Profile, Conn: h}, nil + // A hosted connection that cannot be authenticated is not a usable + // connection. The producer refuses to emit one; this refuses to invent + // one rather than deferring the failure into a remote 401. + if v.Credential == "" { + return Resolved{}, errors.New("formae reported a hosted connection with no credential") + } + return Resolved{ + Profile: v.Profile, + Conn: h, + Credential: secret.New(v.Credential), + }, nil default: - return Resolved{}, fmt.Errorf("formae reported an unknown connection mode %q", v.Cli.Connection.Mode) + return Resolved{}, fmt.Errorf("formae reported an unknown connection mode %q", v.Connection.Mode) } } -// Resolve reads a profile's resolved configuration from the CLI. An empty -// profileName lets the CLI resolve the active profile and report which one it -// used, so this package never reasons about what "active" meant. -func Resolve(ctx context.Context, bin, profileName string) (Resolved, error) { +// Resolve reads a profile's resolved connection and credential from the CLI. An +// empty profileName lets the CLI resolve the active profile and report which one +// it used, so this package never reasons about what "active" meant. +// +// forceRefresh asks the auth plugin for a fresh credential rather than the +// stored one. It is for the 401 path, which re-resolves and then checks that +// the target did not move. +func Resolve(ctx context.Context, bin, profileName string, forceRefresh bool) (Resolved, error) { if err := featuregate.GuardFeatureContext(ctx, featuregate.FeatureConnectionOracle, bin); err != nil { return Resolved{}, err } - return resolveVia(ctx, bin, profileName) + return resolveVia(ctx, bin, profileName, forceRefresh) } diff --git a/internal/config/oracle_test.go b/internal/config/oracle_test.go index 8952bb1..9e6ad88 100644 --- a/internal/config/oracle_test.go +++ b/internal/config/oracle_test.go @@ -2,6 +2,7 @@ package config import ( "context" + "encoding/json" "errors" "os" "path/filepath" @@ -10,17 +11,23 @@ import ( "time" ) +// The resolve view is flat: the connection sits at the top level, not under a +// `cli` key the way `profile show` nests it. const classicView = `{"schemaVersion":1,"profile":"dev", - "cli":{"connection":{"mode":"classic","url":"http://localhost","port":49684}}}` + "connection":{"mode":"classic","url":"http://localhost","port":49684}}` const hostedView = `{"schemaVersion":1,"profile":"prod", - "cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai", - "installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}}` + "connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai", + "installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa","auth":{"type":"oidc"}}, + "credential":"Bearer live-token"}` -func TestDecodeProfileShow_Classic(t *testing.T) { - got, err := decodeProfileShow([]byte(classicView)) +// token is recognisable, so a leak into an error shows up in a substring check. +const token = "Bearer sup3rs3cr3t" + +func TestDecodeResolved_Classic(t *testing.T) { + got, err := decodeResolved([]byte(classicView)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Profile != "dev" { t.Errorf("profile: want %q, got %q", "dev", got.Profile) @@ -29,12 +36,15 @@ func TestDecodeProfileShow_Classic(t *testing.T) { if got.Conn != Connection(want) { t.Errorf("connection: want %#v, got %#v", want, got.Conn) } + if !got.Credential.IsZero() { + t.Error("a classic connection must carry no credential") + } } -func TestDecodeProfileShow_Hosted(t *testing.T) { - got, err := decodeProfileShow([]byte(hostedView)) +func TestDecodeResolved_Hosted(t *testing.T) { + got, err := decodeResolved([]byte(hostedView)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Profile != "prod" { t.Errorf("profile: want %q, got %q", "prod", got.Profile) @@ -46,42 +56,199 @@ func TestDecodeProfileShow_Hosted(t *testing.T) { if got.Conn != Connection(want) { t.Errorf("connection: want %#v, got %#v", want, got.Conn) } + if got.Credential.Reveal() != "Bearer live-token" { + t.Errorf("credential: got %q", got.Credential.Reveal()) + } } -// TestDecodeProfileShow_ToleratesUnknownFields pins that the producer can add -// fields without breaking this consumer. -func TestDecodeProfileShow_ToleratesUnknownFields(t *testing.T) { +// The producer can add fields without breaking this consumer. The hosted arm's +// `auth` object is one such field today: it is reduced to a type discriminator +// and this consumer has no use for it. +func TestDecodeResolved_ToleratesUnknownFields(t *testing.T) { view := `{"schemaVersion":1,"profile":"dev","futureField":{"a":1}, - "cli":{"connection":{"mode":"classic","url":"http://localhost","port":49684,"extra":true}}}` - got, err := decodeProfileShow([]byte(view)) + "connection":{"mode":"classic","url":"http://localhost","port":49684,"extra":true}}` + got, err := decodeResolved([]byte(view)) if err != nil { - t.Fatalf("decodeProfileShow: unexpected error: %v", err) + t.Fatalf("decodeResolved: unexpected error: %v", err) } if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { t.Errorf("connection: got %#v", got.Conn) } } -func TestDecodeProfileShow_Rejects(t *testing.T) { +func TestDecodeResolved_Rejects(t *testing.T) { cases := []struct { name string view string }{ - {"unknown schema version", `{"schemaVersion":2,"cli":{"connection":{"mode":"classic","url":"http://x","port":1}}}`}, - {"missing schema version", `{"cli":{"connection":{"mode":"classic","url":"http://x","port":1}}}`}, - {"unknown mode", `{"schemaVersion":1,"cli":{"connection":{"mode":"orbital"}}}`}, - {"missing connection", `{"schemaVersion":1,"profile":"dev","cli":{}}`}, - {"hosted without installation", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai"}}}`}, - {"hosted with a foreign endpoint", `{"schemaVersion":1,"cli":{"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}}`}, + {"unknown schema version", `{"schemaVersion":2,"connection":{"mode":"classic","url":"http://x","port":1}}`}, + {"missing schema version", `{"connection":{"mode":"classic","url":"http://x","port":1}}`}, + {"unknown mode", `{"schemaVersion":1,"connection":{"mode":"orbital"}}`}, + {"missing connection", `{"schemaVersion":1,"profile":"dev"}`}, + {"hosted without installation", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai"},"credential":"Bearer x"}`}, + {"hosted with a foreign endpoint", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"},"credential":"Bearer x"}`}, + // A hosted connection that cannot be authenticated is not a usable + // connection. The producer refuses to emit one; this is the consumer + // refusing to invent one. + {"hosted with no credential", `{"schemaVersion":1,"connection":{"mode":"hosted","endpoint":"https://cloud.formae.ai","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"}}`}, + // The MCP sends a self-hosted agent no credential. Accepting one here + // would make that non-goal reachable by a producer bug. + {"classic with a credential", `{"schemaVersion":1,"connection":{"mode":"classic","url":"http://localhost","port":49684},"credential":"Bearer x"}`}, {"not json", `formae: command not found`}, - {"classic with an empty url", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","port":49684}}}`}, - {"classic with no port", `{"schemaVersion":1,"cli":{"connection":{"mode":"classic","url":"http://localhost"}}}`}, + {"classic with an empty url", `{"schemaVersion":1,"connection":{"mode":"classic","port":49684}}`}, + {"classic with no port", `{"schemaVersion":1,"connection":{"mode":"classic","url":"http://localhost"}}`}, {"trailing document", classicView + "\n" + classicView}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if _, err := decodeProfileShow([]byte(tc.view)); err == nil { - t.Fatalf("decodeProfileShow(%s): expected an error, got nil", tc.name) + if _, err := decodeResolved([]byte(tc.view)); err == nil { + t.Fatalf("decodeResolved(%s): expected an error, got nil", tc.name) + } + }) + } +} + +func envelope(t *testing.T, code, message string, details map[string]any) string { + t.Helper() + e := map[string]any{"schemaVersion": 1, "code": code, "message": message} + if details != nil { + e["details"] = details + } + b, err := json.Marshal(e) + if err != nil { + t.Fatalf("building an envelope: %v", err) + } + return string(b) +} + +func TestDecodeFailure_AmbiguousProfileCarriesTheCandidates(t *testing.T) { + env := envelope(t, "ambiguous_profile", "more than one profile exists", map[string]any{ + "candidates": []string{"prod", "staging"}, + "active": "prod", + }) + + err := decodeFailure([]byte(env), 1) + + var amb *AmbiguousProfileError + if !errors.As(err, &amb) { + t.Fatalf("want an AmbiguousProfileError, got %T: %v", err, err) + } + if strings.Join(amb.Candidates, ",") != "prod,staging" { + t.Errorf("candidates: got %v", amb.Candidates) + } + if amb.Active != "prod" { + t.Errorf("active: got %q", amb.Active) + } + for _, want := range []string{"prod", "staging", "profile"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the message must name %q so the caller can retry: %v", want, err) + } + } +} + +func TestDecodeFailure_DeclaredCodesCarryTheirCode(t *testing.T) { + for _, code := range []string{"auth_failed", "untrusted_issuer", "no_connection", "internal"} { + t.Run(code, func(t *testing.T) { + err := decodeFailure([]byte(envelope(t, code, "some producer prose", nil)), 1) + + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T: %v", err, err) + } + if re.Code != code { + t.Errorf("code: want %q, got %q", code, re.Code) + } + }) + } +} + +// The producer builds these messages out of an auth plugin's error string or an +// arbitrary err.Error(). A Pkl evaluation failure quotes source lines, and a +// classic profile can hold an inline password, so the free text is closed here. +func TestDecodeFailure_NeverSurfacesTheProducersMessage(t *testing.T) { + env := envelope(t, "internal", "cannot read profile: password = \""+token+"\"", nil) + + err := decodeFailure([]byte(env), 1) + + if strings.Contains(err.Error(), token) { + t.Fatalf("the producer's message reached the consumer: %v", err) + } + if strings.Contains(err.Error(), "password") { + t.Fatalf("the producer's message reached the consumer: %v", err) + } + if !strings.Contains(err.Error(), "formae connection resolve") { + t.Fatalf("the fixed text must point somewhere the reader can look: %v", err) + } +} + +// pkg/auth.ErrorCode is a bare string alias and the producer copies whatever the +// plugin returned into details.pluginCode without checking it, so this is an +// open channel wearing a closed channel's name. +func TestDecodeFailure_ValidatesThePluginCode(t *testing.T) { + t.Run("a declared code is kept", func(t *testing.T) { + env := envelope(t, "auth_failed", "session is gone", map[string]any{"pluginCode": "session_expired"}) + + err := decodeFailure([]byte(env), 1) + + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T", err) + } + if re.PluginCode != "session_expired" { + t.Errorf("pluginCode: got %q", re.PluginCode) + } + if !strings.Contains(err.Error(), "session_expired") { + t.Errorf("a declared plugin code carries diagnostic value and should be shown: %v", err) + } + }) + + t.Run("anything else is opaque", func(t *testing.T) { + env := envelope(t, "auth_failed", "refused", map[string]any{"pluginCode": token}) + + err := decodeFailure([]byte(env), 1) + + if strings.Contains(err.Error(), token) { + t.Fatalf("an unvalidated plugin code reached the consumer: %v", err) + } + var re *ResolveError + if !errors.As(err, &re) { + t.Fatalf("want a ResolveError, got %T", err) + } + if re.PluginCode == token { + t.Fatalf("an unvalidated plugin code was kept: %q", re.PluginCode) + } + }) +} + +func TestDecodeFailure_Rejects(t *testing.T) { + cases := []struct { + name string + body string + }{ + {"unknown envelope schema version", `{"schemaVersion":2,"code":"internal","message":"x"}`}, + {"missing envelope schema version", `{"code":"internal","message":"x"}`}, + {"an unregistered code", envelope(t, "teapot", "x", nil)}, + // Argv the command cannot parse fails before the output flags are + // established, so it exits non-zero with no envelope at all. The + // producer pins that, so this is a supported path, not a defensive one. + {"no envelope at all", "Error: unknown flag: --nope"}, + {"empty output", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := decodeFailure([]byte(tc.body), 7) + if err == nil { + t.Fatal("expected an error") + } + var amb *AmbiguousProfileError + if errors.As(err, &amb) { + t.Fatal("an unreadable envelope must not decode as ambiguity") + } + if !strings.Contains(err.Error(), "7") { + t.Errorf("an unreadable failure must name the exit status: %v", err) + } + if strings.Contains(err.Error(), "unknown flag") { + t.Errorf("an unreadable failure must not echo the bytes: %v", err) } }) } @@ -102,63 +269,93 @@ func stubFormae(t *testing.T, script string) string { return path } -// TestResolveVia_ReadsStdoutIgnoringStderr pins that warnings on stderr are -// never part of the parsed value. -func TestResolveVia_ReadsStdoutIgnoringStderr(t *testing.T) { - bin := stubFormae(t, "echo 'warning: cli.api is deprecated' >&2\ncat <<'EOF'\n"+classicView+"\nEOF\n") - - got, err := resolveVia(context.Background(), bin, "") - if err != nil { - t.Fatalf("resolveVia: unexpected error: %v", err) - } - if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { - t.Fatalf("connection: got %#v", got.Conn) - } -} - -// TestResolveVia_PassesExactArgv pins the whole command line, so an -// implementation that invents a flag or drops the machine consumer fails here. -// `profile show` takes the name positionally; there is no --profile flag on it -// and none on the root command either, so a flag form would not even parse. +// TestResolveVia_PassesExactArgv pins the whole command line. --profile is a +// local flag on `resolve`, so it follows the subcommand — the opposite of +// `profile show`, where the name is positional. An implementation that carries +// the old shape over passes against a lenient stub and fails against the real +// binary, which is how slice 1's argv defect got in. func TestResolveVia_PassesExactArgv(t *testing.T) { cases := []struct { - name string - profile string - want string + name string + profile string + forceRefresh bool + want string }{ + { + "active profile", + "", false, + "connection resolve --output-consumer machine --output-schema json", + }, { "named profile", - "prod", - "profile show prod --output-consumer machine --output-schema json", + "prod", false, + "connection resolve --profile prod --output-consumer machine --output-schema json", }, { - "active profile", - "", - "profile show --output-consumer machine --output-schema json", + "named profile with a forced refresh", + "prod", true, + "connection resolve --profile prod --output-consumer machine --output-schema json --force-refresh", + }, + { + "active profile with a forced refresh", + "", true, + "connection resolve --output-consumer machine --output-schema json --force-refresh", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { bin := stubFormae(t, "if [ \"$*\" != '"+tc.want+"' ]; then echo \"unexpected argv: $*\" >&2; exit 9; fi\ncat <<'EOF'\n"+classicView+"\nEOF\n") - if _, err := resolveVia(context.Background(), bin, tc.profile); err != nil { + if _, err := resolveVia(context.Background(), bin, tc.profile, tc.forceRefresh); err != nil { t.Fatalf("resolveVia: unexpected error: %v", err) } }) } } -// TestResolveVia_NonZeroExitDoesNotLeakOutput pins that a failure reports the -// exit status rather than the subprocess bytes, which may carry a credential. +// TestResolveVia_ReadsStdoutIgnoringStderr pins that warnings on stderr are +// never part of the parsed value. +func TestResolveVia_ReadsStdoutIgnoringStderr(t *testing.T) { + bin := stubFormae(t, "echo 'warning: cli.api is deprecated' >&2\ncat <<'EOF'\n"+classicView+"\nEOF\n") + + got, err := resolveVia(context.Background(), bin, "", false) + if err != nil { + t.Fatalf("resolveVia: unexpected error: %v", err) + } + if got.Conn != Connection(Classic{URL: "http://localhost", Port: 49684}) { + t.Fatalf("connection: got %#v", got.Conn) + } +} + +// A declared failure reaches the caller as its typed error, which means reading +// stdout on a non-zero exit rather than reporting the status and stopping. +func TestResolveVia_DecodesADeclaredFailure(t *testing.T) { + env := envelope(t, "ambiguous_profile", "pick one", map[string]any{ + "candidates": []string{"prod", "staging"}, + "active": "prod", + }) + bin := stubFormae(t, "cat <<'EOF'\n"+env+"\nEOF\nexit 1\n") + + _, err := resolveVia(context.Background(), bin, "", false) + + var amb *AmbiguousProfileError + if !errors.As(err, &amb) { + t.Fatalf("want an AmbiguousProfileError, got %T: %v", err, err) + } +} + +// TestResolveVia_NonZeroExitDoesNotLeakOutput pins that a failure the consumer +// cannot read reports the exit status rather than the subprocess bytes, which +// may carry a credential or a profile's contents. func TestResolveVia_NonZeroExitDoesNotLeakOutput(t *testing.T) { - const secret = "sensitive-subprocess-output" - bin := stubFormae(t, "echo '"+secret+"'\necho '"+secret+"' >&2\nexit 3\n") + const leaked = "sensitive-subprocess-output" + bin := stubFormae(t, "echo '"+leaked+"'\necho '"+leaked+"' >&2\nexit 3\n") - _, err := resolveVia(context.Background(), bin, "") + _, err := resolveVia(context.Background(), bin, "", false) if err == nil { t.Fatal("resolveVia: expected an error for a non-zero exit, got nil") } - if strings.Contains(err.Error(), secret) { + if strings.Contains(err.Error(), leaked) { t.Fatalf("error leaks subprocess output: %v", err) } if !strings.Contains(err.Error(), "3") { @@ -166,6 +363,41 @@ func TestResolveVia_NonZeroExitDoesNotLeakOutput(t *testing.T) { } } +// A credential must not reach an error on any path, nor a rendering of the +// value the resolver returns. +func TestResolveVia_ContainsTheCredential(t *testing.T) { + t.Run("a rendering of the resolved value", func(t *testing.T) { + bin := stubFormae(t, "cat <<'EOF'\n"+hostedView+"\nEOF\n") + + got, err := resolveVia(context.Background(), bin, "", false) + if err != nil { + t.Fatalf("resolveVia: %v", err) + } + out, err := json.Marshal(got) + if err != nil { + t.Fatalf("marshalling Resolved: %v", err) + } + if strings.Contains(string(out), "live-token") { + t.Fatalf("a JSON rendering of Resolved leaked the credential: %s", out) + } + }) + + t.Run("a success document that then fails to validate", func(t *testing.T) { + bad := `{"schemaVersion":1,"profile":"prod","connection":{"mode":"hosted",` + + `"endpoint":"https://evil.example.com","installation":"3HzFPXfPDGhwLJJVtaHbmFs6vLa"},` + + `"credential":"` + token + `"}` + bin := stubFormae(t, "cat <<'EOF'\n"+bad+"\nEOF\n") + + _, err := resolveVia(context.Background(), bin, "", false) + if err == nil { + t.Fatal("expected a foreign endpoint to be rejected") + } + if strings.Contains(err.Error(), token) { + t.Fatalf("the rejection leaked the credential: %v", err) + } + }) +} + func TestResolveVia_HonoursCancellation(t *testing.T) { bin := stubFormae(t, "sleep 30\n") @@ -174,7 +406,7 @@ func TestResolveVia_HonoursCancellation(t *testing.T) { done := make(chan error, 1) go func() { - _, err := resolveVia(ctx, bin, "") + _, err := resolveVia(ctx, bin, "", false) done <- err }() @@ -195,7 +427,7 @@ func TestResolveVia_BoundsOutputSizePromptly(t *testing.T) { done := make(chan error, 1) go func() { - _, err := resolveVia(context.Background(), bin, "") + _, err := resolveVia(context.Background(), bin, "", false) done <- err }() diff --git a/internal/execctx/execctx.go b/internal/execctx/execctx.go index 3b4b3a9..ab32584 100644 --- a/internal/execctx/execctx.go +++ b/internal/execctx/execctx.go @@ -9,6 +9,7 @@ import ( "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) // Context is the frozen decision set for one MCP call. @@ -18,6 +19,15 @@ type Context struct { ProfileName string // Conn is where this call sends agent requests. Conn config.Connection + // Credential authenticates this call, and is the zero value for classic. + // + // It sits beside the connection rather than inside the hosted arm on + // purpose: configuration gets logged, compared and rendered, and a secret + // inside it would ride along on all three. The combination this leaves + // representable — a classic connection with a credential — is ruled out + // where it would matter: the decoder refuses that shape, and the client + // holds a credential only in the hosted arm of its routing. + Credential secret.Value // FormaeBin is the formae binary this call shells out to. Resolved once; // callers take it from here rather than resolving again. FormaeBin string @@ -26,7 +36,7 @@ type Context struct { // Resolver builds Contexts. The resolve func is injectable so tests do not // shell out to a CLI. type Resolver struct { - resolve func(ctx context.Context, bin, profileName string) (config.Resolved, error) + resolve func(ctx context.Context, bin, profileName string, forceRefresh bool) (config.Resolved, error) bin formaebin.BinResolver } @@ -47,11 +57,19 @@ func (r *Resolver) Managed() bool { return r.bin.Managed() } // Resolve produces the context for an optional profile name. The CLI is the // configuration authority: it reports which profile it actually used, so this // package never reasons about what "active" meant at the time of the call. -func (r *Resolver) Resolve(ctx context.Context, profileName string) (Context, error) { +// +// forceRefresh is for the 401 path, which re-resolves with a fresh credential +// and then checks the target did not move. +func (r *Resolver) Resolve(ctx context.Context, profileName string, forceRefresh bool) (Context, error) { bin := r.bin.Resolve() - res, err := r.resolve(ctx, bin, profileName) + res, err := r.resolve(ctx, bin, profileName, forceRefresh) if err != nil { return Context{}, err } - return Context{ProfileName: res.Profile, Conn: res.Conn, FormaeBin: bin}, nil + return Context{ + ProfileName: res.Profile, + Conn: res.Conn, + Credential: res.Credential, + FormaeBin: bin, + }, nil } diff --git a/internal/execctx/execctx_test.go b/internal/execctx/execctx_test.go index 8b2eb5a..e79ee15 100644 --- a/internal/execctx/execctx_test.go +++ b/internal/execctx/execctx_test.go @@ -2,10 +2,13 @@ package execctx import ( "context" + "encoding/json" + "strings" "testing" "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" ) func testBin() formaebin.BinResolver { @@ -20,13 +23,16 @@ func testBin() formaebin.BinResolver { // the one that was asked for. func TestResolveCarriesTheConnection(t *testing.T) { r := &Resolver{ - resolve: func(_ context.Context, bin, profileName string) (config.Resolved, error) { + resolve: func(_ context.Context, bin, profileName string, forceRefresh bool) (config.Resolved, error) { if bin != "/usr/bin/formae" { t.Errorf("resolve called with bin %q, want the resolved binary", bin) } if profileName != "dev" { t.Errorf("resolve called with profile %q, want %q", profileName, "dev") } + if forceRefresh { + t.Error("an ordinary resolution must not force a refresh") + } return config.Resolved{ Profile: "dev-effective", Conn: config.Classic{URL: "http://localhost", Port: 49684}, @@ -35,7 +41,7 @@ func TestResolveCarriesTheConnection(t *testing.T) { bin: testBin(), } - ec, err := r.Resolve(context.Background(), "dev") + ec, err := r.Resolve(context.Background(), "dev", false) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -48,6 +54,9 @@ func TestResolveCarriesTheConnection(t *testing.T) { if ec.Conn != config.Connection(config.Classic{URL: "http://localhost", Port: 49684}) { t.Errorf("Conn = %#v", ec.Conn) } + if !ec.Credential.IsZero() { + t.Error("a classic context must carry no credential") + } } func TestResolveHosted(t *testing.T) { @@ -56,17 +65,65 @@ func TestResolveHosted(t *testing.T) { Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa", } r := &Resolver{ - resolve: func(context.Context, string, string) (config.Resolved, error) { - return config.Resolved{Profile: "prod", Conn: hosted}, nil + resolve: func(context.Context, string, string, bool) (config.Resolved, error) { + return config.Resolved{ + Profile: "prod", + Conn: hosted, + Credential: secret.New("Bearer live-token"), + }, nil }, bin: testBin(), } - ec, err := r.Resolve(context.Background(), "") + ec, err := r.Resolve(context.Background(), "", false) if err != nil { t.Fatalf("Resolve: %v", err) } if ec.Conn != config.Connection(hosted) { t.Errorf("Conn = %#v, want the hosted arm", ec.Conn) } + if ec.Credential.Reveal() != "Bearer live-token" { + t.Errorf("Credential = %q", ec.Credential.Reveal()) + } +} + +// The 401 path re-resolves with the credential refreshed, so the flag has to +// reach the CLI rather than being dropped at this seam. +func TestResolveForwardsAForcedRefresh(t *testing.T) { + var saw bool + r := &Resolver{ + resolve: func(_ context.Context, _, _ string, forceRefresh bool) (config.Resolved, error) { + saw = forceRefresh + return config.Resolved{ + Profile: "dev", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + }, nil + }, + bin: testBin(), + } + + if _, err := r.Resolve(context.Background(), "dev", true); err != nil { + t.Fatalf("Resolve: %v", err) + } + if !saw { + t.Fatal("a forced refresh did not reach the resolver") + } +} + +// The context gets logged and serialised in the ordinary course of debugging. +func TestContextDoesNotRenderTheCredential(t *testing.T) { + ec := Context{ + ProfileName: "prod", + Conn: config.Hosted{Endpoint: config.HostedOrigin, Installation: "3HzFPXfPDGhwLJJVtaHbmFs6vLa"}, + Credential: secret.New("Bearer sup3rs3cr3t"), + FormaeBin: "/usr/bin/formae", + } + + out, err := json.Marshal(ec) + if err != nil { + t.Fatalf("marshalling the context: %v", err) + } + if strings.Contains(string(out), "sup3rs3cr3t") { + t.Fatalf("a JSON rendering of the context leaked the credential: %s", out) + } } diff --git a/internal/server/server.go b/internal/server/server.go index cded0cd..dcf4dc9 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -37,7 +37,10 @@ func implementation() *mcp.Implementation { // contextResolver is the seam server tests substitute. The concrete resolver // lives in execctx and its injection points are unexported there. type contextResolver interface { - Resolve(ctx context.Context, profileName string) (execctx.Context, error) + // Resolve produces the frozen context for a call. forceRefresh is for the + // 401 path, which re-resolves with a fresh credential and then checks that + // the target did not move. + Resolve(ctx context.Context, profileName string, forceRefresh bool) (execctx.Context, error) Bin() string // Managed reports whether the resolved formae is the copy we provisioned, // which decides whether an upgrade needs sudo. @@ -109,7 +112,7 @@ func (s *Server) resolveCtx(ctx context.Context, profileName string) (execctx.Co FormaeBin: s.ctxResolver.Bin(), }, nil } - ec, err := s.ctxResolver.Resolve(ctx, profileName) + ec, err := s.ctxResolver.Resolve(ctx, profileName, false) if err != nil { return execctx.Context{}, s.explainIfTooOld(err) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index a96a300..5d6e01a 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -794,10 +794,24 @@ type stubResolver struct { err error managed bool sawProfile string + // calls and refreshes count what the 401 path did, so a test can assert + // that a refresh happened, or that one did not. + calls int + refreshes int + // refreshed, when set, is what a forced refresh resolves to. Nil means a + // refresh returns the same context as the first resolution. + refreshed *execctx.Context } -func (r *stubResolver) Resolve(_ context.Context, profileName string) (execctx.Context, error) { +func (r *stubResolver) Resolve(_ context.Context, profileName string, forceRefresh bool) (execctx.Context, error) { r.sawProfile = profileName + r.calls++ + if forceRefresh { + r.refreshes++ + if r.refreshed != nil { + return *r.refreshed, nil + } + } return r.ec, r.err } From b88eb0971ab9959b53cdb30570be14dcf4cbdfde Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 00:56:44 -0700 Subject: [PATCH 04/14] feat(server): route hosted requests to their installation The client's routing becomes a sum with the base URL inside each arm, so no value can pair one arm's endpoint with the other arm's authentication and a classic client has no field a credential could occupy. Hosted sets Formae-Installation exactly once and carries the credential; classic carries neither and behaves as before. Every redirect is refused on hosted, not only a cross-origin one: Go strips Authorization across a cross-host redirect but forwards custom headers, so the routing header would otherwise follow one. The policy hands the 3xx back rather than erroring, which keeps the status available and leaves nothing wrapped for a caller to unwrap wrongly. Writing the containment test found a hole worth recording: fmt cannot reach a Format method on an unexported field, so a struct holding a masked value prints the credential under %v regardless. The routing masks itself, and the limit is now documented and pinned where the type is defined. --- internal/secret/secret.go | 7 + internal/secret/secret_test.go | 31 +++++ internal/server/client.go | 92 ++++++++----- internal/server/requests.go | 72 ++++++++++- internal/server/requests_test.go | 4 +- internal/server/routing.go | 170 ++++++++++++++++++++++++ internal/server/routing_test.go | 213 +++++++++++++++++++++++++++++++ internal/server/server.go | 38 ++++-- internal/server/server_test.go | 12 +- 9 files changed, 586 insertions(+), 53 deletions(-) create mode 100644 internal/server/routing.go create mode 100644 internal/server/routing_test.go diff --git a/internal/secret/secret.go b/internal/secret/secret.go index 509df27..a21014a 100644 --- a/internal/secret/secret.go +++ b/internal/secret/secret.go @@ -22,6 +22,13 @@ const Mask = "" // Methods take value receivers so a copy cannot lose the masking: a Value // travels into structs, closures and interface values constantly, and a pointer // receiver would leave every copy rendering its raw field. +// +// One limit, and it is not a small one. fmt reaches these methods through +// reflect.Value's Interface, which it cannot call on an *unexported* field, so +// a struct holding a Value in an unexported field is printed reflectively and +// the credential comes straight out under %v. Exported fields, encoding/json, +// and slog are all fine. A type that keeps a Value unexported must mask itself, +// which is a thing to check for rather than assume. type Value struct { // v is unexported, so encoding/json and any other reflection-based encoder // sees a struct with no exported fields even before the marshallers below diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go index a674997..8f3fe8e 100644 --- a/internal/secret/secret_test.go +++ b/internal/secret/secret_test.go @@ -63,6 +63,37 @@ func TestValueMasksInsideAStruct(t *testing.T) { assertHidden(t, "json.MarshalIndent of a containing struct", string(pretty)) } +// holder keeps a credential unexported, the way a routing or a client does. +type holder struct { + name string + credential Value +} + +// The limit, pinned rather than left to be discovered. fmt reaches these +// methods through reflect.Value's Interface, which it cannot call on an +// unexported field, so a struct holding a Value unexported prints the +// credential under %v. A type in that shape has to mask itself, and this test +// exists so that requirement is a documented fact rather than folklore. +func TestAnUnexportedFieldIsNotProtectedByTheTypeAlone(t *testing.T) { + h := holder{name: "prod", credential: New(token)} + + if !strings.Contains(fmt.Sprintf("%v", h), "sup3rs3cr3t") { + t.Skip("fmt no longer prints unexported fields reflectively; " + + "the caveat on Value can be dropped and holders can stop masking themselves") + } + + // The remedy, and the only one: the holder masks. + if strings.Contains(fmt.Sprintf("%v", maskedHolder(h)), "sup3rs3cr3t") { + t.Fatal("a holder that masks itself must not leak") + } +} + +type maskedHolder holder + +func (h maskedHolder) String() string { + return fmt.Sprintf("holder{name:%s credential:%s}", h.name, h.credential) +} + func TestValueMasksInJSONAndYAML(t *testing.T) { v := New(token) diff --git a/internal/server/client.go b/internal/server/client.go index e6ebe8a..9bb15b0 100644 --- a/internal/server/client.go +++ b/internal/server/client.go @@ -15,28 +15,35 @@ import ( ) // FormaeClient is a lightweight HTTP client for the formae agent REST API. +// +// There is no endpoint field: where a request goes and what authenticates it +// are one decision, held by the routing, so neither can be paired with the +// other arm's. type FormaeClient struct { - endpoint string + route routing httpClient *http.Client } +// NewFormaeClient builds a classic client for an endpoint that already carries +// its port. func NewFormaeClient(endpoint string) *FormaeClient { return &FormaeClient{ - endpoint: endpoint, - httpClient: &http.Client{ - Timeout: 30 * time.Second, - }, + route: classicRoute{base: endpoint}, + httpClient: &http.Client{Timeout: 30 * time.Second}, } } -// newClientFromCtx builds a client for a resolved connection. Hosted is -// recognised and refused: this build cannot authenticate, and shipping a -// routing header without a credential would turn an understandable -// "unsupported" into a remote 401. +// newClientFromCtx builds a client for a resolved connection. +// +// The hosted connection is re-validated here rather than trusted from the CLI. +// The MCP exposes tools that write profiles, so a model fed hostile input can +// author a hosted profile naming the real issuer with an attacker's endpoint; +// checking again at the last point before a credential reaches a header is what +// makes that a refusal rather than an exfiltration. // // A classic connection with no port of its own carries one in its URL (a forced // endpoint), so it is used as-is. -func newClientFromCtx(ec execctx.Context) (*FormaeClient, error) { +func newClientFromCtx(ec execctx.Context, refresh refresher) (*FormaeClient, error) { switch conn := ec.Conn.(type) { case config.Classic: endpoint := conn.URL @@ -44,26 +51,47 @@ func newClientFromCtx(ec execctx.Context) (*FormaeClient, error) { endpoint = fmt.Sprintf("%s:%d", conn.URL, conn.Port) } return NewFormaeClient(endpoint), nil + case config.Hosted: - return nil, fmt.Errorf( - "profile %q targets hosted formae, which this build does not support yet", - ec.ProfileName) + if err := config.ValidateHosted(conn); err != nil { + return nil, fmt.Errorf("profile %q resolved an unusable hosted connection: %w", + ec.ProfileName, err) + } + if ec.Credential.IsZero() { + return nil, fmt.Errorf( + "profile %q targets hosted formae but resolved no credential, so it cannot be authenticated", + ec.ProfileName) + } + return &FormaeClient{ + route: &hostedRoute{ + endpoint: conn.Endpoint, + installation: conn.Installation, + credential: ec.Credential, + refreshFn: refresh, + }, + httpClient: &http.Client{ + Timeout: 30 * time.Second, + // Refused rather than followed; see refuseRedirects. + CheckRedirect: refuseRedirects, + }, + }, nil + default: return nil, fmt.Errorf("profile %q resolved no usable connection", ec.ProfileName) } } -func (c *FormaeClient) get(ctx context.Context, path string, query url.Values) ([]byte, int, error) { - return c.do(ctx, request{Method: http.MethodGet, Path: path, Query: query}) +func (c *FormaeClient) get(ctx context.Context, path string, query url.Values, retry retryPolicy) ([]byte, int, error) { + return c.do(ctx, request{Method: http.MethodGet, Path: path, Query: query}, retry) } -func (c *FormaeClient) post(ctx context.Context, path string, query url.Values) ([]byte, int, error) { +func (c *FormaeClient) post(ctx context.Context, path string, query url.Values, retry retryPolicy) ([]byte, int, error) { return c.do(ctx, request{ Method: http.MethodPost, Path: path, Query: query, ContentType: "application/json", - }) + }, retry) } // ListResources queries the agent for resources matching the given query string. @@ -73,7 +101,7 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra q.Set("query", query) } - body, status, err := c.get(ctx, "/api/v1/resources", q) + body, status, err := c.get(ctx, "/api/v1/resources", q, retryOnce) if err != nil { return nil, err } @@ -89,7 +117,7 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra // ListStacks retrieves all stacks from the agent. func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/stacks", nil) + body, status, err := c.get(ctx, "/api/v1/stacks", nil, retryOnce) if err != nil { return nil, err } @@ -105,7 +133,7 @@ func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) // ListPolicies retrieves all standalone policies from the agent. func (c *FormaeClient) ListPolicies(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/policies", nil) + body, status, err := c.get(ctx, "/api/v1/policies", nil, retryOnce) if err != nil { return nil, err } @@ -126,7 +154,7 @@ func (c *FormaeClient) ListTargets(ctx context.Context, query string) (json.RawM q.Set("query", query) } - body, status, err := c.get(ctx, "/api/v1/targets", q) + body, status, err := c.get(ctx, "/api/v1/targets", q, retryOnce) if err != nil { return nil, err } @@ -150,7 +178,7 @@ func (c *FormaeClient) GetCommandStatus(ctx context.Context, commandID string, c Path: "/api/v1/commands/status", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, retryOnce) if err != nil { return nil, err } @@ -179,7 +207,7 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult Path: "/api/v1/commands/status", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, retryOnce) if err != nil { return nil, err } @@ -195,7 +223,7 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult // GetAgentStats retrieves agent statistics. func (c *FormaeClient) GetAgentStats(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.get(ctx, "/api/v1/stats", nil) + body, status, err := c.get(ctx, "/api/v1/stats", nil, retryOnce) if err != nil { return nil, err } @@ -208,7 +236,7 @@ func (c *FormaeClient) GetAgentStats(ctx context.Context) (json.RawMessage, erro // CheckHealth checks if the agent is healthy. func (c *FormaeClient) CheckHealth(ctx context.Context) error { - _, status, err := c.get(ctx, "/api/v1/health", nil) + _, status, err := c.get(ctx, "/api/v1/health", nil, retryOnce) if err != nil { return fmt.Errorf("agent is not reachable: %w", err) } @@ -289,7 +317,7 @@ func (c *FormaeClient) CancelCommands(ctx context.Context, query string, clientI Path: "/api/v1/commands/cancel", Query: q, Headers: map[string]string{"Client-ID": clientID}, - }) + }, noRetry) if err != nil { return nil, err } @@ -306,7 +334,7 @@ func (c *FormaeClient) CancelCommands(ctx context.Context, query string, clientI // ListChangesSinceLastReconcile retrieves modifications since last reconcile for a stack. func (c *FormaeClient) ListChangesSinceLastReconcile(ctx context.Context, stack string) (json.RawMessage, error) { path := fmt.Sprintf("/api/v1/stacks/%s/changes-since-last-reconcile", url.PathEscape(stack)) - body, status, err := c.get(ctx, path, nil) + body, status, err := c.get(ctx, path, nil, retryOnce) if err != nil { return nil, err } @@ -319,7 +347,7 @@ func (c *FormaeClient) ListChangesSinceLastReconcile(ctx context.Context, stack // ForceSync triggers an immediate resource synchronization. func (c *FormaeClient) ForceSync(ctx context.Context) error { - _, status, err := c.post(ctx, "/api/v1/admin/synchronize", nil) + _, status, err := c.post(ctx, "/api/v1/admin/synchronize", nil, noRetry) if err != nil { return err } @@ -332,7 +360,7 @@ func (c *FormaeClient) ForceSync(ctx context.Context) error { // ForceDiscover triggers an immediate resource discovery. func (c *FormaeClient) ForceDiscover(ctx context.Context) error { - _, status, err := c.post(ctx, "/api/v1/admin/discover", nil) + _, status, err := c.post(ctx, "/api/v1/admin/discover", nil, noRetry) if err != nil { return err } @@ -345,7 +373,7 @@ func (c *FormaeClient) ForceDiscover(ctx context.Context) error { // ForceCheckTTL triggers an immediate TTL expiry sweep. func (c *FormaeClient) ForceCheckTTL(ctx context.Context) (json.RawMessage, error) { - body, status, err := c.post(ctx, "/api/v1/admin/check-ttl", nil) + body, status, err := c.post(ctx, "/api/v1/admin/check-ttl", nil, noRetry) if err != nil { return nil, err } @@ -360,7 +388,7 @@ func (c *FormaeClient) ForceCheckTTL(ctx context.Context) (json.RawMessage, erro // but body is also returned so callers can surface the agent's error JSON. func (c *FormaeClient) ForceReconcileStack(ctx context.Context, label string) (json.RawMessage, int, error) { path := fmt.Sprintf("/api/v1/stacks/%s/reconcile", url.PathEscape(label)) - body, status, err := c.post(ctx, path, nil) + body, status, err := c.post(ctx, path, nil, noRetry) if err != nil { return nil, 0, err } @@ -401,5 +429,5 @@ func (c *FormaeClient) postMultipartWithHeaders(ctx context.Context, path string Headers: headers, Body: &buf, ContentType: w.FormDataContentType(), - }) + }, noRetry) } diff --git a/internal/server/requests.go b/internal/server/requests.go index f689f7a..a0063b4 100644 --- a/internal/server/requests.go +++ b/internal/server/requests.go @@ -2,6 +2,7 @@ package server import ( "context" + "errors" "fmt" "io" "net/http" @@ -20,14 +21,63 @@ type request struct { ContentType string } +// retryPolicy says whether a request may be sent a second time after its +// credential has been refreshed. +// +// It is an argument rather than a field on request, so a new call site cannot +// inherit an answer by leaving a zero value alone. Retryability is a property +// of the request and not of its verb: a simulated apply is a POST that may be +// safe, and a nominally read-only tool may issue a POST, so inferring it from +// the method or from a tool annotation would be guessing. +type retryPolicy int + +const ( + // noRetry returns the original failure. Mutations use this: nothing + // establishes that a 401 always precedes dispatch, and duplicating an + // infrastructure mutation to save one error is the wrong trade. + noRetry retryPolicy = iota + // retryOnce resends once, and only once, after a successful refresh. + retryOnce +) + +// errRetryableBody guards a combination that would corrupt a request silently. +var errRetryableBody = errors.New( + "a retryable request may not carry a body: the first attempt consumes the reader, " + + "so a retry would send an empty one") + // do executes a request against the agent and returns the body and status. -func (c *FormaeClient) do(ctx context.Context, r request) ([]byte, int, error) { - u := c.endpoint + r.Path - if len(r.Query) > 0 { - u += "?" + r.Query.Encode() +func (c *FormaeClient) do(ctx context.Context, r request, retry retryPolicy) ([]byte, int, error) { + if retry == retryOnce && r.Body != nil { + return nil, 0, errRetryableBody + } + + body, status, err := c.send(ctx, r) + if err != nil || status != http.StatusUnauthorized { + return body, status, err } - req, err := http.NewRequestWithContext(ctx, r.Method, u, r.Body) + // A 401 has two distinct sources, and only one of them is actionable here. + // A credential command that failed carries the plugin's own code and was + // reported at resolution. This 401 arrived after a successful resolution, + // so there is no plugin error behind it: it can mean expiry, wrong + // audience, wrong issuer, or a malformed credential, and nothing here can + // tell which. Refresh, and say no more than "unauthorized" if it recurs. + // + // 403 has no branch at all, deliberately: a denied installation or tenant + // is an authorization failure, not an expired session. + refreshed, rerr := c.route.refresh(ctx) + if rerr != nil { + return body, status, rerr + } + if !refreshed || retry != retryOnce { + return body, status, nil + } + return c.send(ctx, r) +} + +// send performs exactly one attempt. +func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) { + req, err := http.NewRequestWithContext(ctx, r.Method, c.route.url(r.Path, r.Query), r.Body) if err != nil { return nil, 0, fmt.Errorf("building request: %w", err) } @@ -37,6 +87,9 @@ func (c *FormaeClient) do(ctx context.Context, r request) ([]byte, int, error) { for k, v := range r.Headers { req.Header.Set(k, v) } + // The routing decorates last, so a caller cannot displace the routing + // header or the credential by naming one in Headers. + c.route.decorate(req.Header) resp, err := c.httpClient.Do(req) if err != nil { @@ -44,6 +97,15 @@ func (c *FormaeClient) do(ctx context.Context, r request) ([]byte, int, error) { } defer func() { _ = resp.Body.Close() }() + // A 3xx only reaches here on a connection whose policy refuses to follow + // redirects. Reporting it as routing rather than passing the status up + // keeps a caller from treating a redirect body as an answer. + if resp.StatusCode >= 300 && resp.StatusCode < 400 { + return nil, resp.StatusCode, fmt.Errorf( + "the hosted endpoint answered %d with a redirect, which is refused: "+ + "this is a routing problem, not a response", resp.StatusCode) + } + body, err := io.ReadAll(resp.Body) if err != nil { return nil, resp.StatusCode, fmt.Errorf("failed to read response: %w", err) diff --git a/internal/server/requests_test.go b/internal/server/requests_test.go index 656ebea..cc31578 100644 --- a/internal/server/requests_test.go +++ b/internal/server/requests_test.go @@ -25,7 +25,7 @@ func TestDo_SendsSuppliedHeaders(t *testing.T) { Method: "GET", Path: "/api/v1/health", Headers: map[string]string{"Client-ID": "abc123"}, - }); err != nil { + }, noRetry); err != nil { t.Fatalf("do: unexpected error: %v", err) } if got != "abc123" { @@ -45,7 +45,7 @@ func TestDo_HonoursCancellation(t *testing.T) { cancel() c := newTestFormaeClient(srv) - if _, _, err := c.do(ctx, request{Method: "GET", Path: "/api/v1/health"}); err == nil { + if _, _, err := c.do(ctx, request{Method: "GET", Path: "/api/v1/health"}, noRetry); err == nil { t.Fatal("do with a cancelled context: expected an error, got nil") } } diff --git a/internal/server/routing.go b/internal/server/routing.go new file mode 100644 index 0000000..2a861dc --- /dev/null +++ b/internal/server/routing.go @@ -0,0 +1,170 @@ +package server + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// installationHeader routes a request to one hosted installation. The edge +// selects a backend on this alone, which is why it is set exactly once. +const installationHeader = "Formae-Installation" + +// refresher re-resolves this call's profile with the credential refreshed. It +// is injected rather than reached for directly, so the credential a retry uses +// comes from the same seam the first resolution did. +type refresher func(ctx context.Context) (execctx.Context, error) + +// routing is how one connection addresses and authenticates its requests. +// +// The address lives here with the credential rather than beside them on the +// client: a hosted route holding a classic endpoint, or a classic route holding +// the hosted one, is then not a thing that can be written down. +type routing interface { + // url builds the request URL for a path and query. + url(path string, q url.Values) string + + // decorate adds the headers this connection requires. + decorate(h http.Header) + + // collectionMiss answers a 404 from an endpoint that lists things. + collectionMiss(empty json.RawMessage) (json.RawMessage, error) + + // refresh re-resolves the credential and reports whether anything changed, + // so the caller knows whether a second attempt could differ from the first. + refresh(ctx context.Context) (bool, error) +} + +// classicRoute addresses a self-hosted agent. +type classicRoute struct { + base string +} + +func (r classicRoute) url(path string, q url.Values) string { return joinURL(r.base, path, q) } + +func (classicRoute) decorate(http.Header) {} + +func (classicRoute) collectionMiss(empty json.RawMessage) (json.RawMessage, error) { + return empty, nil +} + +// refresh does nothing. The MCP sends a self-hosted agent no credential, so +// there is nothing a second attempt would do differently. +func (classicRoute) refresh(context.Context) (bool, error) { return false, nil } + +// hostedRoute addresses one installation behind the shared edge. +type hostedRoute struct { + endpoint string + installation string + credential secret.Value + refreshFn refresher +} + +// String and GoString mask, and holding a secret.Value is not enough on its own +// to make that true. +// +// fmt reaches a field's Format or String method only through reflect.Value's +// Interface, which it cannot call on an *unexported* field. So a struct with an +// unexported secret.Value is printed reflectively, field by field, and the +// credential comes straight out — which is exactly what a developer sees when +// they print the routing while debugging the thing that holds it. Any type +// keeping a credential in an unexported field has to mask itself. +func (r *hostedRoute) String() string { + return fmt.Sprintf("hosted{endpoint:%s installation:%s credential:%s}", + r.endpoint, r.installation, r.credential) +} + +func (r *hostedRoute) GoString() string { return r.String() } + +func (r *hostedRoute) url(path string, q url.Values) string { return joinURL(r.endpoint, path, q) } + +// decorate sets the routing header with Set and never Add. The edge rejects a +// duplicated header after it has already selected a router, so a second value +// is not a warning, it is a failure that surfaces somewhere else entirely. +func (r *hostedRoute) decorate(h http.Header) { + h.Set(installationHeader, r.installation) + h.Set("Authorization", r.credential.Reveal()) +} + +// collectionMiss refuses to report a routing failure as an empty list. The +// shared edge answers 404 for an unknown or unrouted installation, and "no +// resources" would hide that behind a plausible answer. +func (r *hostedRoute) collectionMiss(json.RawMessage) (json.RawMessage, error) { + return nil, fmt.Errorf( + "the hosted endpoint did not route this request to installation %s; "+ + "this is more likely a routing problem than an empty result", + r.installation) +} + +// refresh re-resolves with the credential refreshed and refuses to let the +// target move. +// +// The comparison is the point. Without it the refresh path silently +// re-introduces the skew that resolving configuration and credentials together +// exists to prevent, and a retry could read from a different installation than +// the one the call set out to address. +// +// The profile name is deliberately not compared: the target is the endpoint and +// the installation, and a pointer that moved while still resolving to the same +// installation has not moved the target. +func (r *hostedRoute) refresh(ctx context.Context) (bool, error) { + if r.refreshFn == nil { + return false, nil + } + next, err := r.refreshFn(ctx) + if err != nil { + return false, err + } + hosted, ok := next.Conn.(config.Hosted) + if !ok { + return false, errConnectionMoved + } + if hosted.Endpoint != r.endpoint || hosted.Installation != r.installation { + return false, errConnectionMoved + } + if next.Credential.IsZero() { + return false, errors.New("formae refreshed the connection but returned no credential") + } + r.credential = next.Credential + return true, nil +} + +// withEndpoint returns a copy addressing a different origin. It exists for +// tests, which need the validated hosted routing behaviour aimed at a local +// server rather than at the real edge. +func (r *hostedRoute) withEndpoint(endpoint string) *hostedRoute { + copied := *r + copied.endpoint = endpoint + return &copied +} + +var errConnectionMoved = errors.New( + "the connection changed while this request was in flight, so it was abandoned rather than " + + "retried against a different installation") + +func joinURL(base, path string, q url.Values) string { + u := base + path + if len(q) > 0 { + u += "?" + q.Encode() + } + return u +} + +// refuseRedirects is the hosted redirect policy: hand the 3xx back rather than +// follow it, so do can report it as a routing error. +// +// Every redirect is refused, not only a cross-origin one. The agent API has no +// redirect worth following, and refusing outright avoids getting origin +// equivalence subtly right for no benefit. Returning ErrUseLastResponse rather +// than an error keeps the status available and leaves nothing wrapped in a +// *url.Error for a caller to unwrap incorrectly. +func refuseRedirects(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse +} diff --git a/internal/server/routing_test.go b/internal/server/routing_test.go new file mode 100644 index 0000000..a3f76d5 --- /dev/null +++ b/internal/server/routing_test.go @@ -0,0 +1,213 @@ +package server + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +const testInstallation = "3HzFPXfPDGhwLJJVtaHbmFs6vLa" + +// hostedCtx is a resolved hosted context pointing at srv rather than the real +// edge, so the routing behaviour can be exercised against httptest. +func hostedCtx(credential string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: config.HostedOrigin, + Installation: testInstallation, + }, + Credential: secret.New(credential), + FormaeBin: "/usr/bin/formae", + } +} + +// newTestHostedClient builds a hosted client whose requests land on srv. The +// endpoint the routing validated is the real origin; only the transport is +// redirected, so the header and credential behaviour under test is the same +// behaviour that would reach the edge. +func newTestHostedClient(t *testing.T, srv *httptest.Server, credential string, refresh refresher) *FormaeClient { + t.Helper() + c, err := newClientFromCtx(hostedCtx(credential), refresh) + if err != nil { + t.Fatalf("newClientFromCtx: %v", err) + } + c.route = c.route.(*hostedRoute).withEndpoint(srv.URL) + c.httpClient = srv.Client() + c.httpClient.CheckRedirect = refuseRedirects + return c +} + +func TestHostedRequestsCarryTheInstallationExactlyOnce(t *testing.T) { + var values []string + var auth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + values = r.Header.Values("Formae-Installation") + auth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + if _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry); err != nil { + t.Fatalf("do: %v", err) + } + + if len(values) != 1 { + t.Fatalf("Formae-Installation must be set exactly once, got %d values: %v", len(values), values) + } + if values[0] != testInstallation { + t.Errorf("Formae-Installation = %q", values[0]) + } + if auth != "Bearer live-token" { + t.Errorf("Authorization = %q", auth) + } +} + +func TestClassicRequestsCarryNeitherHeader(t *testing.T) { + var installation, auth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + installation = r.Header.Get("Formae-Installation") + auth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + if _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry); err != nil { + t.Fatalf("do: %v", err) + } + + if installation != "" { + t.Errorf("a classic request must carry no routing header, got %q", installation) + } + if auth != "" { + t.Errorf("a classic request must carry no credential, got %q", auth) + } +} + +// Go strips Authorization across a cross-host redirect but forwards custom +// headers, so the routing header would follow one. Every 3xx is refused, and +// the target must never be contacted. +func TestHostedRefusesEveryRedirect(t *testing.T) { + for _, code := range []int{ + http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther, + http.StatusTemporaryRedirect, http.StatusPermanentRedirect, + } { + t.Run(fmt.Sprint(code), func(t *testing.T) { + var followed bool + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + followed = true + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+"/api/v1/health", code) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + + if err == nil { + t.Fatal("a redirect on a hosted request must be an error") + } + if !strings.Contains(err.Error(), "routing") { + t.Errorf("the error should name routing as the cause: %v", err) + } + if followed { + t.Fatal("the redirect target was contacted") + } + }) + } +} + +// A classic connection keeps Go's default redirect handling: nothing about its +// behaviour changes in this slice. +func TestClassicStillFollowsRedirects(t *testing.T) { + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer target.Close() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+"/api/v1/health", http.StatusFound) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + body, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusOK || !strings.Contains(string(body), "ok") { + t.Fatalf("classic redirect handling changed: status %d body %s", status, body) + } +} + +// The MCP re-validates what the CLI resolved, because it exposes tools that +// write profiles: a model fed hostile input can author one. +func TestClientConstructionRevalidatesTheHostedConnection(t *testing.T) { + cases := map[string]config.Hosted{ + "a foreign endpoint": {Endpoint: "https://evil.example", Installation: testInstallation}, + "a malformed installation": {Endpoint: config.HostedOrigin, Installation: "not-an-installation"}, + "the retired uuid form": {Endpoint: config.HostedOrigin, Installation: "3f2b8c14-0000-4000-8000-000000000000"}, + "an http endpoint": {Endpoint: "http://cloud.formae.ai", Installation: testInstallation}, + "an endpoint with a path": {Endpoint: "https://cloud.formae.ai/api", Installation: testInstallation}, + } + for name, conn := range cases { + t.Run(name, func(t *testing.T) { + ec := execctx.Context{ProfileName: "prod", Conn: conn, Credential: secret.New("Bearer x")} + + if _, err := newClientFromCtx(ec, nil); err == nil { + t.Fatalf("%s must be refused before it can reach a header", name) + } + }) + } +} + +// A hosted connection with no credential cannot authenticate, so it is not a +// usable connection. The decoder refuses the shape; this refuses it again at +// the point where a request would otherwise go out unauthenticated. +func TestHostedClientRequiresACredential(t *testing.T) { + ec := hostedCtx("") + + if _, err := newClientFromCtx(ec, nil); err == nil { + t.Fatal("a hosted connection with no credential must be refused") + } +} + +// A credential must not reach a rendering of the client or its routing. +func TestClientDoesNotRenderTheCredential(t *testing.T) { + c, err := newClientFromCtx(hostedCtx("Bearer sup3rs3cr3t"), nil) + if err != nil { + t.Fatalf("newClientFromCtx: %v", err) + } + + for _, rendered := range []string{ + fmt.Sprintf("%v", c.route), + fmt.Sprintf("%+v", c.route), + fmt.Sprintf("%#v", c.route), + } { + if strings.Contains(rendered, "sup3rs3cr3t") { + t.Fatalf("a rendering of the routing leaked the credential: %s", rendered) + } + } + + out, err := json.Marshal(c.route) + if err != nil { + t.Fatalf("marshalling the routing: %v", err) + } + if strings.Contains(string(out), "sup3rs3cr3t") { + t.Fatalf("a JSON rendering of the routing leaked the credential: %s", out) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index dcf4dc9..cbce2c3 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -86,7 +86,29 @@ func (s *Server) clientFor(ctx context.Context, profileName string) (*FormaeClie if err != nil { return nil, err } - return newClientFromCtx(ec) + return s.clientFrom(ec) +} + +// clientFrom builds the client for an already-resolved context. +// +// This is the one place that pairs a context with its refresher, so a 401 on +// any call re-resolves through the same seam the first resolution used rather +// than reaching past it. Handlers that resolve their own context call this +// instead of constructing a client directly. +func (s *Server) clientFrom(ec execctx.Context) (*FormaeClient, error) { + return newClientFromCtx(ec, s.refresherFor(ec)) +} + +// refresherFor re-resolves the same profile with the credential refreshed. +// +// It closes over the effective profile name from the original snapshot, not +// over whatever the active pointer says at refresh time: re-resolving "whatever +// is active now" is exactly the skew that resolving configuration and +// credentials together exists to prevent. +func (s *Server) refresherFor(ec execctx.Context) refresher { + return func(ctx context.Context) (execctx.Context, error) { + return s.ctxResolver.Resolve(ctx, ec.ProfileName, true) + } } // resolveCtx returns the immutable execution context for an optional profile. @@ -375,7 +397,7 @@ func (s *Server) handleGetCommandStatus(ctx context.Context, _ *mcp.CallToolRequ if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } @@ -395,7 +417,7 @@ func (s *Server) handleListCommands(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } @@ -423,7 +445,7 @@ func (s *Server) handleCheckHealth(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } @@ -565,7 +587,7 @@ func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequ } notice := "" - if c, cerr := newClientFromCtx(ec); cerr == nil { + if c, cerr := s.clientFrom(ec); cerr == nil { notice = s.buildSkewNotice(ctx, ec.FormaeBin, c) } return withNotice(textResult(string(content)), notice), nil, nil @@ -652,7 +674,7 @@ func (s *Server) handleApplyForma(ctx context.Context, _ *mcp.CallToolRequest, i return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } @@ -674,7 +696,7 @@ func (s *Server) handleDestroyForma(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } @@ -704,7 +726,7 @@ func (s *Server) handleCancelCommands(ctx context.Context, _ *mcp.CallToolReques if err != nil { return errorResult(err), nil, nil } - c, err := newClientFromCtx(ec) + c, err := s.clientFrom(ec) if err != nil { return errorResult(err), nil, nil } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 5d6e01a..f5e86aa 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -836,8 +836,8 @@ func TestClientFor_ExplicitProfileReachesTheResolver(t *testing.T) { if r.sawProfile != "p" { t.Errorf("resolver saw profile %q, want %q", r.sawProfile, "p") } - if c.endpoint != "http://p-host:7000" { - t.Errorf("endpoint = %q, want the profile endpoint", c.endpoint) + if got := c.route.url("", nil); got != "http://p-host:7000" { + t.Errorf("endpoint = %q, want the profile endpoint", got) } } @@ -875,8 +875,8 @@ func TestClientFor_ClassicBuildsURLPort(t *testing.T) { if err != nil { t.Fatal(err) } - if c.endpoint != "http://localhost:49684" { - t.Errorf("endpoint = %q", c.endpoint) + if got := c.route.url("", nil); got != "http://localhost:49684" { + t.Errorf("endpoint = %q", got) } } @@ -900,8 +900,8 @@ func TestClientFor_ForcedEndpointIsClassicWithoutAPort(t *testing.T) { if err != nil { t.Fatal(err) } - if c.endpoint != "http://forced:1" { - t.Errorf("endpoint = %q, want the forced endpoint", c.endpoint) + if got := c.route.url("", nil); got != "http://forced:1" { + t.Errorf("endpoint = %q, want the forced endpoint", got) } } From 98078c4215b93c96bbbe269b79ce6706676acc1d Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 00:59:35 -0700 Subject: [PATCH 05/14] feat(server): declare retryability and handle a hosted 401 Retryability is a required argument rather than a field with a default, so a new call site cannot inherit an answer by leaving a zero value alone. Every bodyless GET is retryable; every mutation refreshes so the next call succeeds and then returns the original failure. A 401 re-resolves with the credential refreshed and then compares mode, origin and installation against the snapshot the call started from. Without that comparison the refresh path silently re-introduces the skew that resolving configuration and credentials together exists to prevent, and a retry could read from a different installation than the one addressed. 403 has no branch at all: a denied installation is not an expired session. A retryable request may not carry a body. The first attempt consumes the reader, so a retry would send an empty one, which looks like a successful request rather than a corrupted one. --- internal/server/routing_test.go | 19 +- internal/server/unauthorized_test.go | 335 +++++++++++++++++++++++++++ 2 files changed, 351 insertions(+), 3 deletions(-) create mode 100644 internal/server/unauthorized_test.go diff --git a/internal/server/routing_test.go b/internal/server/routing_test.go index a3f76d5..a09f462 100644 --- a/internal/server/routing_test.go +++ b/internal/server/routing_test.go @@ -57,7 +57,20 @@ func TestHostedRequestsCarryTheInstallationExactlyOnce(t *testing.T) { defer srv.Close() c := newTestHostedClient(t, srv, "Bearer live-token", nil) - if _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry); err != nil { + // The request names both headers itself. This is the case that separates + // Set from Add: on a fresh header the two are indistinguishable, and a + // second routing value is not a warning at the edge but a rejection that + // happens after a router has already been chosen, so it surfaces somewhere + // else entirely. It also pins that the routing decorates last and wins, + // rather than a caller being able to redirect a credentialled request. + if _, _, err := c.do(context.Background(), request{ + Method: "GET", + Path: "/api/v1/health", + Headers: map[string]string{ + "Formae-Installation": "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + "Authorization": "Bearer somebody-elses", + }, + }, noRetry); err != nil { t.Fatalf("do: %v", err) } @@ -65,10 +78,10 @@ func TestHostedRequestsCarryTheInstallationExactlyOnce(t *testing.T) { t.Fatalf("Formae-Installation must be set exactly once, got %d values: %v", len(values), values) } if values[0] != testInstallation { - t.Errorf("Formae-Installation = %q", values[0]) + t.Errorf("Formae-Installation = %q, want the routing's own", values[0]) } if auth != "Bearer live-token" { - t.Errorf("Authorization = %q", auth) + t.Errorf("Authorization = %q, want the routing's own", auth) } } diff --git a/internal/server/unauthorized_test.go b/internal/server/unauthorized_test.go new file mode 100644 index 0000000..baedbcb --- /dev/null +++ b/internal/server/unauthorized_test.go @@ -0,0 +1,335 @@ +package server + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" +) + +// recordingRefresher stands in for the resolver seam, counting what the 401 +// path asked for. Counting the calls is the point: an implementation that +// reaches past the seam, or that refreshes when it should not, is invisible to +// an assertion that only looks at the error. +type recordingRefresher struct { + calls int + next execctx.Context + err error +} + +func (r *recordingRefresher) refresh(context.Context) (execctx.Context, error) { + r.calls++ + return r.next, r.err +} + +// hostedAt is what a refresh resolves to: the same installation at the same +// endpoint the route already holds. The tests aim the transport at httptest, so +// the refreshed context has to name that endpoint too — otherwise every refresh +// would look like the target moving, and the binding check would pass these +// tests for the wrong reason. +func hostedAt(endpoint, credential string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: testInstallation, + }, + Credential: secret.New(credential), + FormaeBin: "/usr/bin/formae", + } +} + +// unauthorizedThenOK answers 401 until the request carries wantAuth, then 200. +// It records every Authorization it saw, so a retry can be checked to have used +// the refreshed credential rather than merely to have happened. +type unauthorizedThenOK struct { + wantAuth string + seen []string +} + +func (h *unauthorizedThenOK) ServeHTTP(w http.ResponseWriter, r *http.Request) { + auth := r.Header.Get("Authorization") + h.seen = append(h.seen, auth) + if auth != h.wantAuth { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"ok":true}`)) +} + +func TestRetryableRequestRefreshesAndRetriesWithTheNewCredential(t *testing.T) { + handler := &unauthorizedThenOK{wantAuth: "Bearer refreshed"} + srv := httptest.NewServer(handler) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusOK { + t.Fatalf("status = %d, want the retry to succeed", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want exactly one refresh", rr.calls) + } + if len(handler.seen) != 2 { + t.Fatalf("requests = %d, want the original and one retry", len(handler.seen)) + } + if handler.seen[0] != "Bearer stale" || handler.seen[1] != "Bearer refreshed" { + t.Errorf("the retry did not carry the refreshed credential: %v", handler.seen) + } +} + +// A second 401 is reported as unauthorized without invention. It has no plugin +// error behind it and can mean expiry, wrong audience, wrong issuer or a +// malformed credential, so nothing here can say which. +func TestASecondUnauthorizedIsNotRefreshedAgain(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer also-rejected")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d, want the unauthorized to be reported", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want exactly one refresh", rr.calls) + } + if requests != 2 { + t.Errorf("requests = %d, want exactly one retry", requests) + } +} + +// A mutation refreshes so the next call succeeds, and returns the original +// failure. Nothing establishes that a 401 always precedes dispatch, and +// duplicating an infrastructure mutation to save one error is the wrong trade. +func TestAMutationRefreshesButNeverRetries(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "POST", Path: "/api/v1/commands"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d, want the original failure", status) + } + if rr.calls != 1 { + t.Errorf("resolver calls = %d, want the refresh to still happen", rr.calls) + } + if requests != 1 { + t.Errorf("requests = %d, want no second attempt", requests) + } +} + +// A denied installation or tenant is an authorization failure, not an expired +// session. Asserting the resolver count rather than the error text is what +// makes this a real check: an implementation that refreshed and then reported +// the 403 anyway would pass a text assertion. +func TestForbiddenNeverReauthenticates(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer refreshed")} + c := newTestHostedClient(t, srv, "Bearer live", rr.refresh) + + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusForbidden { + t.Errorf("status = %d", status) + } + if rr.calls != 0 { + t.Fatalf("a 403 triggered %d resolver calls; it must trigger none", rr.calls) + } +} + +// A refresh that comes back pointing somewhere else is abandoned rather than +// retried. Without this the refresh path silently re-introduces the skew that +// resolving configuration and credentials together exists to prevent. +func TestARefreshThatMovesTheTargetAborts(t *testing.T) { + // Each case is built against the endpoint the route actually holds, so it + // fails the check it is named for. Hard-coding the real origin here would + // make every case fail the endpoint comparison first, and the credential + // case in particular would pass without ever exercising its own check. + moved := map[string]func(endpoint string) execctx.Context{ + "a different installation": func(endpoint string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + }, + Credential: secret.New("Bearer refreshed"), + } + }, + "a different origin": func(string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: "https://other.formae.ai", + Installation: testInstallation, + }, + Credential: secret.New("Bearer refreshed"), + } + }, + "a mode that flipped to classic": func(string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + } + }, + "a refresh that returned no credential": func(endpoint string) execctx.Context { + return execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: endpoint, + Installation: testInstallation, + }, + } + }, + } + for name, build := range moved { + t.Run(name, func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: build(srv.URL)} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err == nil { + t.Fatal("a refresh that moved the target must abort") + } + if requests != 1 { + t.Errorf("requests = %d, want no attempt against the moved target", requests) + } + }) + } +} + +// The connection-moved abort names what happened, because "unauthorized" would +// send the reader looking at credentials rather than at their profile. +func TestTheConnectionMovedErrorSaysSo(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{next: execctx.Context{ + ProfileName: "prod", + Conn: config.Hosted{ + Endpoint: config.HostedOrigin, + Installation: "2ZaBcDeFgHiJkLmNoPqRsTuVwXy", + }, + Credential: secret.New("Bearer refreshed"), + }} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if !errors.Is(err, errConnectionMoved) { + t.Fatalf("want errConnectionMoved, got %v", err) + } +} + +// A refresh that fails outright surfaces its own error rather than the 401. +func TestARefreshFailureSurfaces(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + rr := &recordingRefresher{err: errors.New("the auth plugin is not installed")} + c := newTestHostedClient(t, srv, "Bearer stale", rr.refresh) + + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err == nil || !strings.Contains(err.Error(), "auth plugin") { + t.Fatalf("want the refresh failure, got %v", err) + } +} + +// The MCP sends a self-hosted agent no credential, so a 401 from one is +// somebody else's problem and there is nothing a retry would change. +func TestAClassicUnauthorizedIsReturnedAsIs(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + _, status, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + + if err != nil { + t.Fatalf("do: %v", err) + } + if status != http.StatusUnauthorized { + t.Errorf("status = %d", status) + } + if requests != 1 { + t.Errorf("requests = %d, want no retry", requests) + } +} + +// A retryable request carrying a body would replay an already-consumed reader +// and silently send an empty one, which looks like a successful request rather +// than a corrupted one. Refused outright instead. +func TestARetryableRequestMayNotCarryABody(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := newTestFormaeClient(srv) + _, _, err := c.do(context.Background(), request{ + Method: "POST", + Path: "/api/v1/commands", + Body: strings.NewReader("payload"), + }, retryOnce) + + if !errors.Is(err, errRetryableBody) { + t.Fatalf("want errRetryableBody, got %v", err) + } + if requests != 0 { + t.Errorf("the request must be refused before it is sent, got %d requests", requests) + } +} From efabc5385662a23a66b2212df406b4bba9861bfa Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:00:20 -0700 Subject: [PATCH 06/14] feat(server): stop reporting a hosted routing 404 as an empty list The shared edge answers 404 for an unknown or unrouted installation, so the six endpoints that translate 404 into an empty payload would present a routing failure as a plausible answer, which is the failure mode nobody investigates. Under hosted they become an error naming the installation they addressed. GetCommandStatus keeps its 404: that one is an object not-found, and telling edge-routing 404s from agent-object 404s properly needs a stable edge error envelope that does not exist yet, so the narrowing stops where the ambiguity starts. --- internal/server/client.go | 12 +-- internal/server/collection_test.go | 119 +++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 6 deletions(-) create mode 100644 internal/server/collection_test.go diff --git a/internal/server/client.go b/internal/server/client.go index 9bb15b0..2feff74 100644 --- a/internal/server/client.go +++ b/internal/server/client.go @@ -106,7 +106,7 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -122,7 +122,7 @@ func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -138,7 +138,7 @@ func (c *FormaeClient) ListPolicies(ctx context.Context) (json.RawMessage, error return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -159,7 +159,7 @@ func (c *FormaeClient) ListTargets(ctx context.Context, query string) (json.RawM return nil, err } if status == http.StatusNotFound { - return json.RawMessage("[]"), nil + return c.route.collectionMiss(json.RawMessage("[]")) } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -212,7 +212,7 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult return nil, err } if status == http.StatusNotFound { - return json.RawMessage(`{"Commands":[]}`), nil + return c.route.collectionMiss(json.RawMessage(`{"Commands":[]}`)) } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) @@ -322,7 +322,7 @@ func (c *FormaeClient) CancelCommands(ctx context.Context, query string, clientI return nil, err } if status == http.StatusNotFound { - return json.RawMessage(`{"CommandIds":[]}`), nil + return c.route.collectionMiss(json.RawMessage(`{"CommandIds":[]}`)) } if status != http.StatusAccepted { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) diff --git a/internal/server/collection_test.go b/internal/server/collection_test.go new file mode 100644 index 0000000..9e68661 --- /dev/null +++ b/internal/server/collection_test.go @@ -0,0 +1,119 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The six endpoints that translate a 404 into an empty payload. Under hosted +// the shared edge answers 404 for an unknown or unrouted installation, so +// reporting "nothing here" would present a routing failure as a plausible +// answer — the one failure mode nobody investigates. +var collectionCalls = map[string]struct { + call func(context.Context, *FormaeClient) (json.RawMessage, error) + wantEmpty string +}{ + "ListResources": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListResources(ctx, "") }, + wantEmpty: "[]", + }, + "ListStacks": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListStacks(ctx) }, + wantEmpty: "[]", + }, + "ListPolicies": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListPolicies(ctx) }, + wantEmpty: "[]", + }, + "ListTargets": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { return c.ListTargets(ctx, "") }, + wantEmpty: "[]", + }, + "ListCommands": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { + return c.ListCommands(ctx, "", "10", "cid") + }, + wantEmpty: `{"Commands":[]}`, + }, + "CancelCommands": { + call: func(ctx context.Context, c *FormaeClient) (json.RawMessage, error) { + return c.CancelCommands(ctx, "", "cid") + }, + wantEmpty: `{"CommandIds":[]}`, + }, +} + +func notFoundServer(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestClassic404OnACollectionStaysAnEmptyList(t *testing.T) { + for name, tc := range collectionCalls { + t.Run(name, func(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + + got, err := tc.call(context.Background(), c) + if err != nil { + t.Fatalf("%s: a classic 404 must still read as empty: %v", name, err) + } + if string(got) != tc.wantEmpty { + t.Errorf("%s: got %s, want %s", name, got, tc.wantEmpty) + } + }) + } +} + +func TestHosted404OnACollectionIsARoutingError(t *testing.T) { + for name, tc := range collectionCalls { + t.Run(name, func(t *testing.T) { + c := newTestHostedClient(t, notFoundServer(t), "Bearer live-token", nil) + + got, err := tc.call(context.Background(), c) + if err == nil { + t.Fatalf("%s: a hosted 404 must not read as empty, got %s", name, got) + } + if !strings.Contains(err.Error(), "routing") { + t.Errorf("%s: the error should name routing as the likely cause: %v", name, err) + } + if !strings.Contains(err.Error(), testInstallation) { + t.Errorf("%s: the error should name the installation it addressed: %v", name, err) + } + }) + } +} + +// GetCommandStatus is deliberately not in that set. Its 404 is an +// endpoint-specific "no such command" and keeps its meaning under both modes. +// Telling the two apart properly needs a stable edge error envelope, which does +// not exist yet, so the narrowing stops where the ambiguity starts. +func TestCommandStatus404KeepsItsMeaning(t *testing.T) { + t.Run("classic", func(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + + _, err := c.GetCommandStatus(context.Background(), "cmd-1", "cid") + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("want a command-not-found error, got %v", err) + } + }) + + t.Run("hosted", func(t *testing.T) { + c := newTestHostedClient(t, notFoundServer(t), "Bearer live-token", nil) + + _, err := c.GetCommandStatus(context.Background(), "cmd-1", "cid") + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("want a command-not-found error, got %v", err) + } + if strings.Contains(err.Error(), "routing") { + t.Fatalf("an object-not-found must not be reported as a routing failure: %v", err) + } + }) +} From bb95f2e641ab70a38c46e8caab8fe44d52e1206d Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:00:57 -0700 Subject: [PATCH 07/14] test(server): pin that hosted ambiguity reaches the caller as an instruction The CLI decides ambiguity, since it is the only side that can settle it before a credential is minted. This pins that the refusal arrives as a message naming every candidate and the active one, and asking for the profile argument, so a caller can act on it rather than seeing an opaque failure. Elicitation and a remembered per-session profile are deliberately not built: this works in every MCP client, needs no session state and no capability negotiation, and is what an elicitation path falls back to anyway. --- internal/server/ambiguity_test.go | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 internal/server/ambiguity_test.go diff --git a/internal/server/ambiguity_test.go b/internal/server/ambiguity_test.go new file mode 100644 index 0000000..962ed22 --- /dev/null +++ b/internal/server/ambiguity_test.go @@ -0,0 +1,64 @@ +package server + +import ( + "context" + "strings" + "testing" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/tools" +) + +// The CLI decides ambiguity, because it is the only side that can settle it +// before a credential is minted. The MCP's job is to turn that refusal into +// something the caller can act on rather than an opaque failure. +// +// Elicitation and a remembered per-session profile are deliberately not here. +// This message works in every MCP client, needs no session state and no +// capability negotiation, and is what an elicitation path would fall back to +// anyway. +func TestAmbiguityBecomesAnActionableInstruction(t *testing.T) { + s := New("") + s.ctxResolver = &stubResolver{ + ec: execctx.Context{FormaeBin: "/usr/bin/formae"}, + err: &config.AmbiguousProfileError{ + Candidates: []string{"acme-prod", "acme-staging"}, + Active: "acme-prod", + }, + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler returned a transport error: %v", err) + } + if !res.IsError { + t.Fatal("ambiguity must reach the caller as a failed call it can retry") + } + + text := textContent(t, res) + for _, want := range []string{"acme-prod", "acme-staging", "active", "profile"} { + if !strings.Contains(text, want) { + t.Errorf("the instruction must mention %q so the caller can retry: %s", want, text) + } + } +} + +// An explicit profile settles the choice, so the caller's retry has to reach +// the resolver as the named profile rather than being dropped. +func TestAnExplicitProfileIsPassedThroughOnTheRetry(t *testing.T) { + r := &stubResolver{ec: execctx.Context{ + ProfileName: "acme-staging", + Conn: config.Classic{URL: "http://localhost", Port: 49684}, + FormaeBin: "/usr/bin/formae", + }} + s := New("") + s.ctxResolver = r + + if _, err := s.clientFor(context.Background(), "acme-staging"); err != nil { + t.Fatalf("clientFor: %v", err) + } + if r.sawProfile != "acme-staging" { + t.Fatalf("the named profile did not reach the resolver, got %q", r.sawProfile) + } +} From ab4dc7e5e82033c8034e021e5d44c6fed5d42220 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:09:00 -0700 Subject: [PATCH 08/14] feat(server): report which installation answered A hosted result names the effective profile and the installation in a second content block, never by touching the payload a consumer parses. Classic carries none: the user already knows which agent they pointed at. The wording follows how far the call actually got, which only the executor knows. An agent 500 answered. A mutation whose transport failed after dispatch was addressed and may already have acted, and saying it did nothing would be confidently wrong. A forma file that failed to evaluate resolved a destination and sent nothing, and claiming otherwise would send an operator to check an installation for work that cannot exist. The policy planners are agent-backed despite the name, and are the one place where 'answered' cannot be read off the result, because an unreachable agent is deliberately swallowed there. fetchPolicies reports its own reach instead. That also makes a pre-existing gap visible under hosted: those tools take no profile argument, so their inventory read is ambiguous with several profiles and the ambiguity is swallowed like any other failure. The structural check enforces this per return rather than per handler. The first version asked only whether a handler mentioned attribute() anywhere, and a mutation proved it passed on a handler whose success path had none. --- internal/server/attribution.go | 94 +++++++ internal/server/attribution_test.go | 298 +++++++++++++++++++++ internal/server/client.go | 4 + internal/server/policy.go | 11 +- internal/server/policy_standalone_tools.go | 61 ++++- internal/server/requests.go | 14 + internal/server/routing_test.go | 24 +- internal/server/server.go | 194 +++++++++----- 8 files changed, 609 insertions(+), 91 deletions(-) create mode 100644 internal/server/attribution.go create mode 100644 internal/server/attribution_test.go diff --git a/internal/server/attribution.go b/internal/server/attribution.go new file mode 100644 index 0000000..1be2d3a --- /dev/null +++ b/internal/server/attribution.go @@ -0,0 +1,94 @@ +package server + +import ( + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" +) + +// reach is how far one call got toward its destination. +// +// Only the request executor may advance it. Deriving it from IsError, or from +// the context having resolved, is how it stops being true: a forma file that +// fails to evaluate has resolved a destination and contacted nothing, and an +// agent 500 is a failure that was nonetheless answered. +type reach int + +const ( + // reachResolved: a destination is known and nothing was sent. + reachResolved reach = iota + // reachAttempted: bytes left, and the outcome is unknown. + reachAttempted + // reachAnswered: the agent responded, whatever the status. + reachAnswered +) + +// destination is what a result says about where the call went. The zero value +// means no destination was ever resolved, which is the honest answer for a +// failure that happened before resolution. +type destination struct { + ec execctx.Context + reach reach +} + +// resolved names a destination for a call that has not built a client yet. +func resolved(ec execctx.Context) destination { + return destination{ec: ec, reach: reachResolved} +} + +// reached names a destination for a call that has a client, taking how far it +// got from the client itself rather than guessing. +func reached(ec execctx.Context, c *FormaeClient) destination { + if c == nil { + return resolved(ec) + } + return destination{ec: ec, reach: c.reach} +} + +// note renders the attribution line, or "" when there is nothing true to say. +// +// Only hosted calls carry one. A classic connection addresses the agent the +// user pointed at, which they already know; a hosted one addresses a single +// installation behind an endpoint shared with every other installation, and a +// profile name can later be repointed, so the name alone is weak evidence. +// +// The wording differs by reach because the three states are different claims. +// Saying an installation was addressed when nothing was sent would send an +// operator to check for work that cannot exist, which is worse than saying +// nothing at all. +func (d destination) note() string { + hosted, ok := d.ec.Conn.(config.Hosted) + if !ok { + return "" + } + switch d.reach { + case reachAnswered: + return fmt.Sprintf("Installation %s answered, via profile %q.", + hosted.Installation, d.ec.ProfileName) + case reachAttempted: + return fmt.Sprintf( + "This request was sent to installation %s via profile %q, and its outcome is unknown: "+ + "it may already have taken effect.", + hosted.Installation, d.ec.ProfileName) + default: + return fmt.Sprintf("Profile %q resolves to installation %s; nothing was sent.", + d.ec.ProfileName, hosted.Installation) + } +} + +// attribute appends the attribution to a result as its own content block. +// +// A separate block, never a wrapper or a prefix: the first block is the agent's +// payload and a consumer parses it, so changing its content would change the +// result schema. The version-skew notice already works this way. +func attribute(d destination, res *mcp.CallToolResult) *mcp.CallToolResult { + note := d.note() + if note == "" { + return res + } + res.Content = append(res.Content, &mcp.TextContent{Text: note}) + return res +} diff --git a/internal/server/attribution_test.go b/internal/server/attribution_test.go new file mode 100644 index 0000000..a1a8d7c --- /dev/null +++ b/internal/server/attribution_test.go @@ -0,0 +1,298 @@ +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "regexp" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/platform-engineering-labs/formae-mcp/internal/config" + "github.com/platform-engineering-labs/formae-mcp/internal/execctx" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" + "github.com/platform-engineering-labs/formae-mcp/internal/tools" +) + +func blocks(t *testing.T, res *mcp.CallToolResult) []string { + t.Helper() + out := make([]string, 0, len(res.Content)) + for _, c := range res.Content { + tc, ok := c.(*mcp.TextContent) + if !ok { + t.Fatalf("expected TextContent, got %T", c) + } + out = append(out, tc.Text) + } + return out +} + +// hostedServerFor wires a server whose resolved context is hosted but whose +// transport lands on srv, so a handler can be driven end to end. +func hostedServerFor(t *testing.T, srv *httptest.Server) (*Server, execctx.Context) { + t.Helper() + ec := execctx.Context{ + ProfileName: "acme-prod", + Conn: config.Hosted{Endpoint: srv.URL, Installation: testInstallation}, + Credential: secret.New("Bearer live-token"), + FormaeBin: "/usr/bin/formae", + } + s := New("") + s.ctxResolver = &stubResolver{ec: ec} + // The hosted arm validates its endpoint against a compile-time origin, so a + // handler test has to build its client through the seam rather than through + // the guard. The guard itself is covered directly in routing_test.go. + s.newClient = func(ec execctx.Context) (*FormaeClient, error) { + c := newTestHostedClientAt(srv, "Bearer live-token", nil) + return c, nil + } + return s, ec +} + +func TestHostedResultCarriesAttributionWithoutTouchingThePayload(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[{"label":"default"}]`)) + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if res.IsError { + t.Fatalf("unexpected error result: %s", blocks(t, res)) + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the payload and one attribution block, got %d: %v", len(got), got) + } + if got[0] != `[{"label":"default"}]` { + t.Errorf("the first block must still be exactly the agent's payload, got %q", got[0]) + } + if !strings.Contains(got[1], testInstallation) || !strings.Contains(got[1], "acme-prod") { + t.Errorf("attribution must name the installation and the profile: %q", got[1]) + } + if !strings.Contains(got[1], "answered") { + t.Errorf("a call the agent answered should say so: %q", got[1]) + } +} + +func TestClassicResultCarriesNoAttribution(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[{"label":"default"}]`)) + })) + defer srv.Close() + + s := New("") + s.ctxResolver = &stubResolver{ec: execctx.Context{ + ProfileName: "dev", + Conn: config.Classic{URL: srv.URL}, + FormaeBin: "/usr/bin/formae", + }} + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if got := blocks(t, res); len(got) != 1 { + t.Fatalf("a classic result must carry one block, got %d: %v", len(got), got) + } +} + +// An agent error is still an answer, and saying so is what tells an operator +// the installation is reachable and rejecting them rather than unreachable. +func TestAnAgentErrorStillCountsAsAnswered(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a 500 must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 || !strings.Contains(got[1], "answered") { + t.Fatalf("an answered failure must be attributed as answered: %v", got) + } +} + +// The case the attribution exists for. A mutation whose transport fails after +// the request went out does not establish that the agent did nothing. +func TestAMutationThatFailsAfterDispatchSaysItMayHaveActed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Answer nothing and drop the connection, so the failure lands after + // the request has already been sent. + hj, ok := w.(http.Hijacker) + if !ok { + t.Fatal("test server does not support hijacking") + } + conn, _, err := hj.Hijack() + if err != nil { + t.Fatalf("hijack: %v", err) + } + _ = conn.Close() + })) + defer srv.Close() + + s, _ := hostedServerFor(t, srv) + res, _, err := s.handleForceSync(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a dropped connection must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the error and one attribution block, got %v", got) + } + if !strings.Contains(got[1], "may already have taken effect") { + t.Errorf("a post-dispatch failure must not claim the agent did nothing: %q", got[1]) + } + if strings.Contains(got[1], "answered") { + t.Errorf("nothing answered, so the attribution must not say so: %q", got[1]) + } +} + +// Resolved is not addressed. An apply whose forma file fails to evaluate has a +// destination and contacted nothing; claiming otherwise would send an operator +// to check an installation for work that cannot exist. +func TestAFailureBeforeDispatchSaysNothingWasSent(t *testing.T) { + s, _ := hostedServerFor(t, httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }))) + + res, _, err := s.handleApplyForma(context.Background(), nil, tools.ApplyFormaInput{ + FilePath: "/nonexistent/forma.pkl", + Mode: "reconcile", + }) + if err != nil { + t.Fatalf("handler: %v", err) + } + if !res.IsError { + t.Fatal("a missing forma file must be an error result") + } + + got := blocks(t, res) + if len(got) != 2 { + t.Fatalf("want the error and one attribution block, got %v", got) + } + if !strings.Contains(got[1], "nothing was sent") { + t.Errorf("a pre-dispatch failure must say nothing was sent: %q", got[1]) + } +} + +// Before a context resolves there is no destination to name. +func TestAFailureBeforeResolutionCarriesNoAttribution(t *testing.T) { + s := New("") + s.ctxResolver = &stubResolver{ + ec: execctx.Context{FormaeBin: "/usr/bin/formae"}, + err: &config.AmbiguousProfileError{Candidates: []string{"a", "b"}, Active: "a"}, + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + if got := blocks(t, res); len(got) != 1 { + t.Fatalf("an unresolved call has no destination to name, got %v", got) + } +} + +func TestNoResultCarriesTheCredential(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + ec := execctx.Context{ + ProfileName: "acme-prod", + Conn: config.Hosted{Endpoint: srv.URL, Installation: testInstallation}, + Credential: secret.New("Bearer sup3rs3cr3t"), + FormaeBin: "/usr/bin/formae", + } + s := New("") + s.ctxResolver = &stubResolver{ec: ec} + s.newClient = func(execctx.Context) (*FormaeClient, error) { + return newTestHostedClientAt(srv, "Bearer sup3rs3cr3t", nil), nil + } + + res, _, err := s.handleListStacks(context.Background(), nil, tools.ProfileInput{}) + if err != nil { + t.Fatalf("handler: %v", err) + } + for _, b := range blocks(t, res) { + if strings.Contains(b, "sup3rs3cr3t") { + t.Fatalf("a result leaked the credential: %s", b) + } + } +} + +// A structural check, in the spirit of TestNoDirectHTTPConstruction. Every +// result an agent-backed handler returns must be attributed, and a per-handler +// audit is precisely how one of them ends up not being. +// +// It checks each return rather than merely whether the handler mentions +// attribute() anywhere. The weaker form passes on a handler whose client-build +// failure is attributed and whose success path is not, which is the mistake +// most likely to be made. +func TestEveryAgentBackedHandlerAttributes(t *testing.T) { + handler := regexp.MustCompile(`(?s)func \(s \*Server\) (handle\w+)\([^)]*\) \((?:res )?\*mcp\.CallToolResult[^{]*\{(.*?)\n\}`) + returnsResult := regexp.MustCompile(`return (?:attribute\([^,]+, )?(jsonResult|textResult|errorResult|withNotice)\(`) + + for _, file := range []string{ + "server.go", "policy.go", "policy_standalone_tools.go", "profile_tools.go", + } { + src, err := os.ReadFile(file) + if err != nil { + t.Fatalf("reading %s: %v", file, err) + } + for _, m := range handler.FindAllStringSubmatch(string(src), -1) { + name, body := m[1], m[2] + reachesAgent := strings.Contains(body, "s.clientFor(") || + strings.Contains(body, "s.newClient(") || + strings.Contains(body, "s.fetchPolicies(") || + strings.Contains(body, `"extract"`) + if !reachesAgent { + continue + } + // A handler may attribute every path at once with a deferred + // assignment to a named result, which the per-return check cannot + // see and does not need to. + if strings.Contains(body, "defer func() { res = attribute(") { + continue + } + // Only returns after the client exists are checked. Before that + // there is genuinely nothing to name — an input-validation failure + // or a resolution failure has no destination — and the one case in + // between, a forma file that fails to evaluate after resolution, is + // covered behaviourally rather than structurally. + var haveClient bool + for _, line := range strings.Split(body, "\n") { + if strings.Contains(line, "s.newClient(") || strings.Contains(line, "s.fetchPolicies(") { + haveClient = true + continue + } + if !haveClient || !returnsResult.MatchString(line) { + continue + } + if !strings.Contains(line, "attribute(") { + t.Errorf("%s in %s returns an unattributed result once a client exists; "+ + "a hosted caller would not learn which installation acted:\n\t%s", + name, file, strings.TrimSpace(line)) + } + } + } + } +} diff --git a/internal/server/client.go b/internal/server/client.go index 2feff74..b59e448 100644 --- a/internal/server/client.go +++ b/internal/server/client.go @@ -22,6 +22,10 @@ import ( type FormaeClient struct { route routing httpClient *http.Client + // reach is the high-water mark for this call: how far its requests got. + // One client is built per tool call, so there is nothing to reset, and the + // executor is the only thing that advances it. + reach reach } // NewFormaeClient builds a classic client for an endpoint that already carries diff --git a/internal/server/policy.go b/internal/server/policy.go index 2b526bb..572e934 100644 --- a/internal/server/policy.go +++ b/internal/server/policy.go @@ -23,7 +23,12 @@ func currentEvalFunc(bin string) EvalFunc { return makeFormaeEval(bin) } -func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateInlinePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateInlinePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if err := validateCreateInlinePolicyInput(input); err != nil { return errorResult(err), nil, nil } @@ -51,7 +56,9 @@ func (s *Server) handleCreateInlinePolicy(ctx context.Context, _ *mcp.CallToolRe // policies known from the agent" and the source check still runs. var inventory []policyInventoryItem if input.Operation == "set" { - if items, err := s.fetchPolicies(ctx); err == nil { + items, d, err := s.fetchPolicies(ctx) + dest = d + if err == nil { inventory = items } for _, item := range inventory { diff --git a/internal/server/policy_standalone_tools.go b/internal/server/policy_standalone_tools.go index 3407574..13661a8 100644 --- a/internal/server/policy_standalone_tools.go +++ b/internal/server/policy_standalone_tools.go @@ -63,20 +63,38 @@ func mcpPolicyType(agentType string) string { // fetchPolicies reads the agent's standalone policy inventory from the // active/default profile's agent (empty profile = active/default, matching the // server's per-call client resolution). -func (s *Server) fetchPolicies(ctx context.Context) ([]policyInventoryItem, error) { - c, err := s.clientFor(ctx, "") +// +// It reports the destination alongside the inventory because callers +// deliberately swallow its error: a planner's real work is local file +// planning, so an unreachable agent downgrades to "no policies known" rather +// than failing the tool. That makes "the agent answered" underivable from the +// planner's result, and a clean plan claiming an installation had answered +// would assert something false about a real installation. +// +// Under hosted this makes a second thing visible. These tools take no profile +// argument, so the inventory read resolves the active profile; with more than +// one profile that is ambiguous, and the ambiguity is swallowed like any other +// failure. The attribution is what says the installation was never reached. +// Giving the policy tools a profile argument is the actual fix, and belongs +// with whatever revisits that tool surface. +func (s *Server) fetchPolicies(ctx context.Context) ([]policyInventoryItem, destination, error) { + ec, err := s.resolveCtx(ctx, "") if err != nil { - return nil, err + return nil, destination{}, err + } + c, err := s.newClient(ec) + if err != nil { + return nil, resolved(ec), err } body, err := c.ListPolicies(ctx) if err != nil { - return nil, fmt.Errorf("list policies from agent: %w", err) + return nil, reached(ec, c), fmt.Errorf("list policies from agent: %w", err) } var items []policyInventoryItem if err := json.Unmarshal(body, &items); err != nil { - return nil, fmt.Errorf("parse policy inventory: %w", err) + return nil, reached(ec, c), fmt.Errorf("parse policy inventory: %w", err) } - return items, nil + return items, reached(ec, c), nil } // standaloneTypeOf resolves a standalone policy label to its MCP policy type, @@ -136,7 +154,12 @@ func validateStandalonePolicyFields(label, policyType string, ttlSeconds int64, return nil } -func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.CreateStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if err := validateStandalonePolicyFields(input.Label, input.PolicyType, input.TTLSeconds, input.OnDependents, input.IntervalSeconds); err != nil { return errorResult(err), nil, nil } @@ -153,7 +176,9 @@ func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallTo // state. The agent inventory is authoritative for what already exists, so // check it first: a policy the agent already knows must not be re-declared, // even if the current workspace source does not (yet) contain it. - if agentItems, err := s.fetchPolicies(ctx); err == nil { + agentItems, d, err := s.fetchPolicies(ctx) + dest = d + if err == nil { if _, known := findPolicyByLabel(agentItems, input.Label); known { out := tools.CreateStandalonePolicyOutput{ Operation: "noop", @@ -241,7 +266,12 @@ func (s *Server) handleCreateStandalonePolicy(ctx context.Context, _ *mcp.CallTo return jsonResult(body), nil, nil } -func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.AttachStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.AttachStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if input.Stack == "" { return errorResult(fmt.Errorf("stack is required")), nil, nil } @@ -263,7 +293,8 @@ func (s *Server) handleAttachStandalonePolicy(ctx context.Context, _ *mcp.CallTo // before the first apply. Fall back to the workspace source in that case // rather than refusing a documented flow. var notes []string - items, fetchErr := s.fetchPolicies(ctx) + items, d, fetchErr := s.fetchPolicies(ctx) + dest = d if fetchErr != nil { items = nil } @@ -429,7 +460,12 @@ func specFromInventoryItem(item policyInventoryItem) (StandalonePolicySpec, erro }, nil } -func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.DeleteStandalonePolicyInput) (*mcp.CallToolResult, any, error) { +func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallToolRequest, input tools.DeleteStandalonePolicyInput) (res *mcp.CallToolResult, _ any, _ error) { + // Every return from here on carries the destination, whatever path it takes. + // A defer rather than an edit per return: these handlers have many exits and + // a new one must not be able to slip out unattributed. + var dest destination + defer func() { res = attribute(dest, res) }() if input.Label == "" { return errorResult(fmt.Errorf("label is required")), nil, nil } @@ -437,7 +473,8 @@ func (s *Server) handleDeleteStandalonePolicy(ctx context.Context, _ *mcp.CallTo return errorResult(err), nil, nil } - inventory, err := s.fetchPolicies(ctx) + inventory, d, err := s.fetchPolicies(ctx) + dest = d if err != nil { return errorResult(err), nil, nil } diff --git a/internal/server/requests.go b/internal/server/requests.go index a0063b4..277675b 100644 --- a/internal/server/requests.go +++ b/internal/server/requests.go @@ -91,10 +91,16 @@ func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) // header or the credential by naming one in Headers. c.route.decorate(req.Header) + // Advanced before the call rather than after: once Do returns an error we + // cannot tell whether the request reached the agent, and "it may have + // acted" is the answer that keeps an operator safe. + c.advance(reachAttempted) + resp, err := c.httpClient.Do(req) if err != nil { return nil, 0, fmt.Errorf("request failed: %w", err) } + c.advance(reachAnswered) defer func() { _ = resp.Body.Close() }() // A 3xx only reaches here on a connection whose policy refuses to follow @@ -112,3 +118,11 @@ func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) } return body, resp.StatusCode, nil } + +// advance raises the high-water mark, never lowers it: a call that answered +// once has answered, whatever a later attempt does. +func (c *FormaeClient) advance(r reach) { + if r > c.reach { + c.reach = r + } +} diff --git a/internal/server/routing_test.go b/internal/server/routing_test.go index a09f462..f173f79 100644 --- a/internal/server/routing_test.go +++ b/internal/server/routing_test.go @@ -36,14 +36,24 @@ func hostedCtx(credential string) execctx.Context { // behaviour that would reach the edge. func newTestHostedClient(t *testing.T, srv *httptest.Server, credential string, refresh refresher) *FormaeClient { t.Helper() - c, err := newClientFromCtx(hostedCtx(credential), refresh) - if err != nil { - t.Fatalf("newClientFromCtx: %v", err) + return newTestHostedClientAt(srv, credential, refresh) +} + +// newTestHostedClientAt builds the same client without a *testing.T, for the +// places that need one inside a seam closure. +func newTestHostedClientAt(srv *httptest.Server, credential string, refresh refresher) *FormaeClient { + return &FormaeClient{ + route: &hostedRoute{ + endpoint: srv.URL, + installation: testInstallation, + credential: secret.New(credential), + refreshFn: refresh, + }, + httpClient: &http.Client{ + Transport: srv.Client().Transport, + CheckRedirect: refuseRedirects, + }, } - c.route = c.route.(*hostedRoute).withEndpoint(srv.URL) - c.httpClient = srv.Client() - c.httpClient.CheckRedirect = refuseRedirects - return c } func TestHostedRequestsCarryTheInstallationExactlyOnce(t *testing.T) { diff --git a/internal/server/server.go b/internal/server/server.go index cbce2c3..b9cad93 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -54,6 +54,11 @@ type Server struct { forcedEndpoint string // when set, empty-profile calls use this (tests / explicit) ctxResolver contextResolver // resolves per-call execution context clientID *clientid.Resolver // resolves the Client-ID header value + // newClient builds the agent client for a resolved context. It is a field + // for the same reason ctxResolver is: the hosted arm validates its endpoint + // against a compile-time origin, so without a seam no test can exercise a + // hosted handler at all. Production never replaces it. + newClient func(execctx.Context) (*FormaeClient, error) } // New creates a new formae MCP server connected to the given agent endpoint. @@ -72,6 +77,7 @@ func New(endpoint string) *Server { ctxResolver: execctx.NewResolver(formaebin.NewBinResolver()), clientID: clientid.NewResolver(), } + s.newClient = s.clientFrom s.registerTools() s.registerResources() @@ -86,7 +92,7 @@ func (s *Server) clientFor(ctx context.Context, profileName string) (*FormaeClie if err != nil { return nil, err } - return s.clientFrom(ec) + return s.newClient(ec) } // clientFrom builds the client for an already-resolved context. @@ -354,39 +360,51 @@ func (s *Server) registerTools() { // Tool handlers — read-only func (s *Server) handleListResources(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListResourcesInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListResources(ctx, input.Query) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListStacks(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListStacks(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListTargets(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListTargetsInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListTargets(ctx, input.Query) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleGetCommandStatus(ctx context.Context, _ *mcp.CallToolRequest, input tools.GetCommandStatusInput) (*mcp.CallToolResult, any, error) { @@ -397,15 +415,15 @@ func (s *Server) handleGetCommandStatus(ctx context.Context, _ *mcp.CallToolRequ if err != nil { return errorResult(err), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.GetCommandStatus(ctx, input.CommandID, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListCommands(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListCommandsInput) (*mcp.CallToolResult, any, error) { @@ -417,27 +435,31 @@ func (s *Server) handleListCommands(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.ListCommands(ctx, input.Query, maxResults, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleGetAgentStats(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.GetAgentStats(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleCheckHealth(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { @@ -445,18 +467,18 @@ func (s *Server) handleCheckHealth(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } if err := c.CheckHealth(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } msg := "Formae agent is healthy and reachable." if notice := s.buildSkewNotice(ctx, ec.FormaeBin, c); notice != "" { msg += "\n\n" + notice } - return textResult(msg), nil, nil + return attribute(reached(ec, c), textResult(msg)), nil, nil } // buildSkewNotice fetches the agent version and the local formae version and @@ -482,42 +504,50 @@ func (s *Server) buildSkewNotice(ctx context.Context, formaeBin string, c *Forma } func (s *Server) handleListPolicies(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ListPolicies(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp.CallToolRequest, input tools.ListChangesSinceLastReconcileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if input.Stack != "" { result, err := c.ListChangesSinceLastReconcile(ctx, input.Stack) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } // No stack specified: fetch all stacks, then get drift for each stacksJSON, err := c.ListStacks(ctx) if err != nil { - return errorResult(fmt.Errorf("failed to list stacks: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to list stacks: %w", err))), nil, nil } var stacks []struct { Label string `json:"Label"` } if err := json.Unmarshal(stacksJSON, &stacks); err != nil { - return errorResult(fmt.Errorf("failed to parse stacks: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to parse stacks: %w", err))), nil, nil } type stackDrift struct { @@ -529,7 +559,7 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp for _, stack := range stacks { driftJSON, err := c.ListChangesSinceLastReconcile(ctx, stack.Label) if err != nil { - return errorResult(fmt.Errorf("failed to get drift for stack %s: %w", stack.Label, err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to get drift for stack %s: %w", stack.Label, err))), nil, nil } // Parse to check if there are modifications @@ -537,7 +567,7 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp ModifiedResources json.RawMessage `json:"ModifiedResources"` } if err := json.Unmarshal(driftJSON, &drift); err != nil { - return errorResult(fmt.Errorf("failed to parse drift for stack %s: %w", stack.Label, err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to parse drift for stack %s: %w", stack.Label, err))), nil, nil } results = append(results, stackDrift{ @@ -548,9 +578,9 @@ func (s *Server) handleListChangesSinceLastReconcile(ctx context.Context, _ *mcp aggregated, err := json.Marshal(results) if err != nil { - return errorResult(fmt.Errorf("failed to marshal results: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to marshal results: %w", err))), nil, nil } - return jsonResult(aggregated), nil, nil + return attribute(reached(ec, c), jsonResult(aggregated)), nil, nil } func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequest, input tools.ExtractResourcesInput) (*mcp.CallToolResult, any, error) { @@ -576,21 +606,29 @@ func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequ args = append(args, "--profile", ec.ProfileName) } args = append(args, outFile) + // Extract reaches the agent through the CLI, so do never runs and cannot + // advance reach. It is taken from the subprocess outcome instead, and that + // is sound here for a reason rather than by exemption: extract is a read. + // It creates, changes and destroys nothing, so "might this have acted?" + // has one answer however it fails, and neither wording can send an operator + // looking for work that cannot exist. Do not copy this to a mutation. cmd := commandWithContext(ctx, ec.FormaeBin, args...) if output, err := cmd.CombinedOutput(); err != nil { - return errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", err, string(output))), nil, nil + return attribute(resolved(ec), + errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", err, string(output)))), nil, nil } + extracted := destination{ec: ec, reach: reachAnswered} content, err := os.ReadFile(outFile) if err != nil { - return errorResult(fmt.Errorf("failed to read extracted file: %w", err)), nil, nil + return attribute(extracted, errorResult(fmt.Errorf("failed to read extracted file: %w", err))), nil, nil } notice := "" - if c, cerr := s.clientFrom(ec); cerr == nil { + if c, cerr := s.newClient(ec); cerr == nil { notice = s.buildSkewNotice(ctx, ec.FormaeBin, c) } - return withNotice(textResult(string(content)), notice), nil, nil + return attribute(extracted, withNotice(textResult(string(content)), notice)), nil, nil } func (s *Server) handleSearchHubPlugins(_ context.Context, _ *mcp.CallToolRequest, input tools.SearchHubPluginsInput) (*mcp.CallToolResult, any, error) { @@ -671,18 +709,18 @@ func (s *Server) handleApplyForma(ctx context.Context, _ *mcp.CallToolRequest, i } formaJSON, err := evalFormaFile(ctx, ec, input.FilePath) if err != nil { - return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil + return attribute(resolved(ec), errorResult(fmt.Errorf("failed to evaluate forma file: %w", err))), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.SubmitCommand(ctx, "apply", input.Mode, input.Simulate, input.Force, formaJSON, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return withNotice(jsonResult(result), s.buildSkewNotice(ctx, ec.FormaeBin, c)), nil, nil + return attribute(reached(ec, c), withNotice(jsonResult(result), s.buildSkewNotice(ctx, ec.FormaeBin, c))), nil, nil } func (s *Server) handleDestroyForma(ctx context.Context, _ *mcp.CallToolRequest, input tools.DestroyFormaInput) (*mcp.CallToolResult, any, error) { @@ -696,29 +734,29 @@ func (s *Server) handleDestroyForma(ctx context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } if input.Query != "" { result, err := c.DestroyByQuery(ctx, input.Query, input.Simulate, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } formaJSON, err := evalFormaFile(ctx, ec, input.FilePath) if err != nil { - return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("failed to evaluate forma file: %w", err))), nil, nil } result, err := c.SubmitCommand(ctx, "destroy", "", input.Simulate, false, formaJSON, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleCancelCommands(ctx context.Context, _ *mcp.CallToolRequest, input tools.CancelCommandsInput) (*mcp.CallToolResult, any, error) { @@ -726,67 +764,83 @@ func (s *Server) handleCancelCommands(ctx context.Context, _ *mcp.CallToolReques if err != nil { return errorResult(err), nil, nil } - c, err := s.clientFrom(ec) + c, err := s.newClient(ec) if err != nil { - return errorResult(err), nil, nil + return attribute(resolved(ec), errorResult(err)), nil, nil } result, err := c.CancelCommands(ctx, input.Query, s.clientID.Resolve(ec.FormaeBin)) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleForceSync(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if err := c.ForceSync(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return textResult("Resource synchronization triggered successfully."), nil, nil + return attribute(reached(ec, c), textResult("Resource synchronization triggered successfully.")), nil, nil } func (s *Server) handleForceDiscover(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } if err := c.ForceDiscover(ctx); err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return textResult("Resource discovery triggered successfully."), nil, nil + return attribute(reached(ec, c), textResult("Resource discovery triggered successfully.")), nil, nil } func (s *Server) handleForceCheckTTL(ctx context.Context, _ *mcp.CallToolRequest, input tools.ProfileInput) (*mcp.CallToolResult, any, error) { - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } result, err := c.ForceCheckTTL(ctx) if err != nil { - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(result), nil, nil + return attribute(reached(ec, c), jsonResult(result)), nil, nil } func (s *Server) handleForceReconcileStack(ctx context.Context, _ *mcp.CallToolRequest, input tools.ForceReconcileStackInput) (*mcp.CallToolResult, any, error) { if input.Stack == "" { return errorResult(fmt.Errorf("stack is required")), nil, nil } - c, err := s.clientFor(ctx, input.Profile) + ec, err := s.resolveCtx(ctx, input.Profile) if err != nil { return errorResult(err), nil, nil } + c, err := s.newClient(ec) + if err != nil { + return attribute(resolved(ec), errorResult(err)), nil, nil + } body, _, err := c.ForceReconcileStack(ctx, input.Stack) if err != nil { if body != nil { - return errorResult(fmt.Errorf("%s: %s", err.Error(), string(body))), nil, nil + return attribute(reached(ec, c), errorResult(fmt.Errorf("%s: %s", err.Error(), string(body)))), nil, nil } - return errorResult(err), nil, nil + return attribute(reached(ec, c), errorResult(err)), nil, nil } - return jsonResult(body), nil, nil + return attribute(reached(ec, c), jsonResult(body)), nil, nil } // Helpers From 71426e9dd0207ff4a4064329d66d8d5e9270c3c6 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:09:29 -0700 Subject: [PATCH 09/14] docs(changelog): record hosted support --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e79d22d..e24f8da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ Install via the ### Added +- Hosted formae support. A profile whose `cli.connection` is a `Hosted` connection now routes to its installation behind the shared endpoint and carries a credential, so every tool works against a hosted installation the way it does against a self-hosted agent. Requires formae 0.89.0 or newer. +- Hosted results say which installation answered, in a separate block alongside the payload. When a change fails after it was already sent, the result says so rather than implying nothing happened, so you know whether to go and check. - The MCP now warns when the connected formae agent is newer than your local `formae`, so you can tell when authoring may not reflect the agent's latest capabilities. The notice points at `/formae:upgrade`, which fetches the newer `formae` after you confirm (never silently in classic mode). ### Changed @@ -21,7 +23,10 @@ Install via the - Plugin renamed from `formae-mcp` to `formae`; added `/formae:setup` and `/formae:upgrade`. - The plugin now downloads its prebuilt `formae-mcp` and a matched `formae` into `~/.formae-ai/opt` on first run (no build-from-source; set `FORMAE_MCP_DEV=1` for local dev builds). - Commands issued through the MCP (apply, destroy, cancel, status, list) now identify with your CLI's client ID (`~/.pel/formae/cli_client_id`) instead of a fixed `formae-mcp` identity, so the agent attributes them to the same client as your own `formae` runs. When the ID file does not exist yet, the MCP runs `formae --version` once so formae creates it, and falls back to the old `formae-mcp` identity if it still cannot be read. -- Configuration now comes from the formae CLI (`formae profile show`) instead of a text scan of the profile file, so the MCP and your own `formae` runs always agree on where a profile points. Requires formae 0.89.0 or newer. +- Configuration and credentials now come from a single `formae connection resolve` per tool call, replacing `formae profile show`, so the MCP and your own `formae` runs always agree on where a profile points and a request can never combine one profile revision's endpoint with another's credential. Requires formae 0.89.0 or newer. +- An expired hosted credential is refreshed and the call retried once, but only for reads. A change that fails with an expired credential refreshes it for next time and reports the failure rather than being sent twice. +- On a hosted profile, a listing endpoint that answers "not found" is now reported as a routing problem rather than as an empty result, since the shared endpoint answers that way for an installation it cannot route to. +- When several profiles exist and none is named, a hosted call now lists the candidates and asks for the `profile` argument instead of guessing. - Every agent request is built by one internal executor, so cancellation and timeouts apply uniformly across every tool. - The plugin no longer installs a second `formae` alongside one you already have. On launch it looks for yours (`PATH`, then `/opt/pel/bin`, `/usr/local/bin`, `~/.local/bin`, `~/bin`) and uses it; it downloads one into `~/.formae-ai/opt` only when the machine has none. Previously it downloaded a copy on every launch and then ran whichever `formae` came first on `PATH`, so the downloaded one was usually dead weight — and `/formae:upgrade` could upgrade a copy the plugin was not running. - The version-skew notice now says which upgrade applies: `/formae:upgrade` for the copy the plugin installed, or the path of your own install, which the plugin will not change. From 10f614ea74be11de1adc42e440823f3b9238dd13 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:15:22 -0700 Subject: [PATCH 10/14] fix(server): scrub credentials from responses, bound them, and stop over-claiming Three findings from the branch review, all verified in the code. A hosted response body was returned verbatim and interpolated into errors, so an intermediary echoing request headers would have put the bearer token into a tool result and from there into a model's context. The routing knows what it sent, including a credential a retry replaced, and scrubs it on the way back. The attribution claimed the installation answered whenever any response arrived. Every response comes from the shared edge, and a hosted 404 on a collection says two lines away that the edge did not route it, so the two contradicted each other in the same result. It now says the endpoint answered. Attempted comes from the request actually being written rather than from being about to call Do, so a connection refused no longer tells an operator to go and check for work that was never sent. Responses are bounded like the configuration oracle's output already is. An unbounded read from a peer is an unbounded allocation, and that peer is now remote and shared rather than a process on this machine. --- internal/server/attribution.go | 12 +++- internal/server/attribution_test.go | 3 + internal/server/requests.go | 32 ++++++++-- internal/server/routing.go | 38 +++++++++++- internal/server/routing_test.go | 92 +++++++++++++++++++++++++++++ 5 files changed, 167 insertions(+), 10 deletions(-) diff --git a/internal/server/attribution.go b/internal/server/attribution.go index 1be2d3a..cd40a72 100644 --- a/internal/server/attribution.go +++ b/internal/server/attribution.go @@ -66,12 +66,18 @@ func (d destination) note() string { } switch d.reach { case reachAnswered: - return fmt.Sprintf("Installation %s answered, via profile %q.", + // "The hosted endpoint answered", not "the installation answered". + // Every response arrives from the shared edge, and the edge answers for + // itself when it cannot route: a hosted 404 on a collection is reported + // two lines away as a routing miss, so claiming the installation had + // answered would contradict it in the same result. Telling the two + // apart needs a stable edge error envelope, which does not exist yet. + return fmt.Sprintf("The hosted endpoint answered for installation %s, via profile %q.", hosted.Installation, d.ec.ProfileName) case reachAttempted: return fmt.Sprintf( - "This request was sent to installation %s via profile %q, and its outcome is unknown: "+ - "it may already have taken effect.", + "This request was sent for installation %s via profile %q and no response came back, "+ + "so its outcome is unknown: it may already have taken effect.", hosted.Installation, d.ec.ProfileName) default: return fmt.Sprintf("Profile %q resolves to installation %s; nothing was sent.", diff --git a/internal/server/attribution_test.go b/internal/server/attribution_test.go index a1a8d7c..8144755 100644 --- a/internal/server/attribution_test.go +++ b/internal/server/attribution_test.go @@ -164,6 +164,9 @@ func TestAMutationThatFailsAfterDispatchSaysItMayHaveActed(t *testing.T) { if strings.Contains(got[1], "answered") { t.Errorf("nothing answered, so the attribution must not say so: %q", got[1]) } + if strings.Contains(got[1], "was sent to installation") { + t.Errorf("the MCP cannot know the installation itself received it: %q", got[1]) + } } // Resolved is not addressed. An apply whose forma file fails to evaluate has a diff --git a/internal/server/requests.go b/internal/server/requests.go index 277675b..582bbe6 100644 --- a/internal/server/requests.go +++ b/internal/server/requests.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptrace" "net/url" ) @@ -40,6 +41,11 @@ const ( retryOnce ) +// maxResponseBytes bounds one agent response. +const maxResponseBytes = 32 << 20 + +var errResponseTooLarge = errors.New("the agent returned more data than this build will read") + // errRetryableBody guards a combination that would corrupt a request silently. var errRetryableBody = errors.New( "a retryable request may not carry a body: the first attempt consumes the reader, " + @@ -91,10 +97,17 @@ func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) // header or the credential by naming one in Headers. c.route.decorate(req.Header) - // Advanced before the call rather than after: once Do returns an error we - // cannot tell whether the request reached the agent, and "it may have - // acted" is the answer that keeps an operator safe. - c.advance(reachAttempted) + // Attempted means bytes left this process, which is what "it may have + // acted" rests on. Taken from the trace rather than from "we are about to + // call Do", because a DNS or TLS failure sends nothing and telling an + // operator to go and check would be a false alarm in the costly direction. + req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{ + WroteRequest: func(info httptrace.WroteRequestInfo) { + if info.Err == nil { + c.advance(reachAttempted) + } + }, + })) resp, err := c.httpClient.Do(req) if err != nil { @@ -112,11 +125,18 @@ func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) "this is a routing problem, not a response", resp.StatusCode) } - body, err := io.ReadAll(resp.Body) + // Bounded, and scrubbed before it can reach a caller. The bound matches the + // one the configuration oracle already applies to its subprocess: an + // unbounded read from a peer is an unbounded allocation, and under hosted + // that peer is remote and shared rather than a process on this machine. + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) if err != nil { return nil, resp.StatusCode, fmt.Errorf("failed to read response: %w", err) } - return body, resp.StatusCode, nil + if len(body) > maxResponseBytes { + return nil, resp.StatusCode, errResponseTooLarge + } + return c.route.scrub(body), resp.StatusCode, nil } // advance raises the high-water mark, never lowers it: a call that answered diff --git a/internal/server/routing.go b/internal/server/routing.go index 2a861dc..19311b6 100644 --- a/internal/server/routing.go +++ b/internal/server/routing.go @@ -1,12 +1,14 @@ package server import ( + "bytes" "context" "encoding/json" "errors" "fmt" "net/http" "net/url" + "slices" "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/execctx" @@ -37,6 +39,10 @@ type routing interface { // collectionMiss answers a 404 from an endpoint that lists things. collectionMiss(empty json.RawMessage) (json.RawMessage, error) + // scrub removes any credential this connection has sent from bytes the far + // end returned, before they can reach a result, an error, or a log. + scrub(body []byte) []byte + // refresh re-resolves the credential and reports whether anything changed, // so the caller knows whether a second attempt could differ from the first. refresh(ctx context.Context) (bool, error) @@ -55,6 +61,10 @@ func (classicRoute) collectionMiss(empty json.RawMessage) (json.RawMessage, erro return empty, nil } +// scrub does nothing: the MCP sends a self-hosted agent no credential, so a +// response cannot be quoting one. +func (classicRoute) scrub(body []byte) []byte { return body } + // refresh does nothing. The MCP sends a self-hosted agent no credential, so // there is nothing a second attempt would do differently. func (classicRoute) refresh(context.Context) (bool, error) { return false, nil } @@ -65,6 +75,9 @@ type hostedRoute struct { installation string credential secret.Value refreshFn refresher + // used is every credential this route has put on the wire, kept so a + // response quoting one can be scrubbed. It is bounded by one refresh. + used []string } // String and GoString mask, and holding a secret.Value is not enough on its own @@ -90,7 +103,11 @@ func (r *hostedRoute) url(path string, q url.Values) string { return joinURL(r.e // is not a warning, it is a failure that surfaces somewhere else entirely. func (r *hostedRoute) decorate(h http.Header) { h.Set(installationHeader, r.installation) - h.Set("Authorization", r.credential.Reveal()) + sent := r.credential.Reveal() + h.Set("Authorization", sent) + if !slices.Contains(r.used, sent) { + r.used = append(r.used, sent) + } } // collectionMiss refuses to report a routing failure as an empty list. The @@ -136,6 +153,25 @@ func (r *hostedRoute) refresh(ctx context.Context) (bool, error) { return true, nil } +// scrub removes any credential this route has used from bytes the far end +// sent back. +// +// The far end is trusted to route, not to be careful. An error page from an +// intermediary that echoes request headers would otherwise put the bearer +// token into a tool result and from there into a model's context and a +// transcript — a copy of the credential somewhere it can never be withdrawn +// from. Both the current and the previous credential are scrubbed, because a +// retry's response can still be quoting the request that failed. +func (r *hostedRoute) scrub(body []byte) []byte { + for _, used := range r.used { + if used == "" { + continue + } + body = bytes.ReplaceAll(body, []byte(used), []byte(secret.Mask)) + } + return body +} + // withEndpoint returns a copy addressing a different origin. It exists for // tests, which need the validated hosted routing behaviour aimed at a local // server rather than at the real edge. diff --git a/internal/server/routing_test.go b/internal/server/routing_test.go index f173f79..2639608 100644 --- a/internal/server/routing_test.go +++ b/internal/server/routing_test.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" @@ -234,3 +235,94 @@ func TestClientDoesNotRenderTheCredential(t *testing.T) { t.Fatalf("a JSON rendering of the routing leaked the credential: %s", out) } } + +// The far end is trusted to route, not to be careful. An error page from an +// intermediary that echoed the request headers would otherwise put the bearer +// token into a tool result, and from there into a model's context and a +// transcript, where it can never be withdrawn from. +func TestAResponseEchoingTheCredentialIsScrubbed(t *testing.T) { + for _, status := range []int{http.StatusOK, http.StatusBadGateway} { + t.Run(fmt.Sprint(status), func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + _, _ = fmt.Fprintf(w, `{"error":"upstream rejected %s"}`, r.Header.Get("Authorization")) + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer sup3rs3cr3t", nil) + body, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + if err != nil { + t.Fatalf("do: %v", err) + } + if strings.Contains(string(body), "sup3rs3cr3t") { + t.Fatalf("the response body carried the credential back out: %s", body) + } + if !strings.Contains(string(body), secret.Mask) { + t.Errorf("the scrub should leave the mask behind: %s", body) + } + }) + } +} + +// A retry's response can still be quoting the request that failed, so the +// credential the first attempt used has to be scrubbed too. +func TestAScrubCoversTheCredentialARetryReplaced(t *testing.T) { + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts == 1 { + w.WriteHeader(http.StatusUnauthorized) + return + } + _, _ = fmt.Fprint(w, `{"note":"the earlier token Bearer stale-secret was rejected"}`) + })) + defer srv.Close() + + rr := &recordingRefresher{next: hostedAt(srv.URL, "Bearer fresh-secret")} + c := newTestHostedClient(t, srv, "Bearer stale-secret", rr.refresh) + + body, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, retryOnce) + if err != nil { + t.Fatalf("do: %v", err) + } + if strings.Contains(string(body), "stale-secret") { + t.Fatalf("the superseded credential survived the scrub: %s", body) + } +} + +// An unbounded read from a peer is an unbounded allocation, and under hosted +// that peer is remote and shared rather than a process on this machine. +func TestAnOversizedResponseIsRefused(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + chunk := make([]byte, 1<<20) + for i := 0; i <= maxResponseBytes>>20; i++ { + if _, err := w.Write(chunk); err != nil { + return + } + } + })) + defer srv.Close() + + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + _, _, err := c.do(context.Background(), request{Method: "GET", Path: "/api/v1/health"}, noRetry) + + if !errors.Is(err, errResponseTooLarge) { + t.Fatalf("want errResponseTooLarge, got %v", err) + } +} + +// Bytes that never left cannot have taken effect. A connection refused before +// the request is written must not tell an operator to go and check. +func TestATransportFailureBeforeTheRequestIsWrittenClaimsNothingWasSent(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + srv.Close() // nothing is listening now + + _, _, err := c.do(context.Background(), request{Method: "POST", Path: "/api/v1/commands"}, noRetry) + if err == nil { + t.Fatal("expected a transport failure") + } + if c.reach != reachResolved { + t.Fatalf("reach = %v, want resolved: nothing was written, so nothing can have acted", c.reach) + } +} From f7366d59a6db6ea3d35d115c7429ab444643ef44 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:19:05 -0700 Subject: [PATCH 11/14] fix(server): bound and scrub extract output, count a partial write as sent Two findings from the second review pass. Extract reaches the agent through the CLI, so its output never passed through the executor's scrub and went into a tool result whole. Its diagnostics are the value of the failure, so they are kept, bounded, with the credential we handed the process removed. A credential the CLI minted for itself is not ours to know and is the CLI's own to mask, which the comment says rather than implying the output is sanitised. WroteRequest can report an error having already put part of a multipart mutation on the wire. Counting only the clean case told an operator nothing was sent when something may have been. Any invocation of the callback now counts; DNS, connection and TLS failures never reach it, which is the distinction it exists for. --- internal/server/attribution_test.go | 37 +++++++++++++++++++++++++++++ internal/server/requests.go | 11 +++++---- internal/server/server.go | 28 +++++++++++++++++++++- 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/internal/server/attribution_test.go b/internal/server/attribution_test.go index 8144755..3856f32 100644 --- a/internal/server/attribution_test.go +++ b/internal/server/attribution_test.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "context" "net/http" "net/http/httptest" @@ -299,3 +300,39 @@ func TestEveryAgentBackedHandlerAttributes(t *testing.T) { } } } + +// Extract reaches the agent through the CLI, so its output never passes +// through the executor's scrub. Its diagnostics are the whole value of the +// failure and are kept, bounded, with the credential we handed the process +// removed. +func TestExtractFailureOutputIsBoundedAndScrubbed(t *testing.T) { + t.Run("the credential we passed is removed", func(t *testing.T) { + got := safeSubprocessOutput( + []byte("plugin refused: Authorization: Bearer sup3rs3cr3t"), + secret.New("Bearer sup3rs3cr3t")) + + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("extract output leaked the credential: %s", got) + } + if !strings.Contains(got, "plugin refused") { + t.Errorf("the diagnostics are the point and must survive: %s", got) + } + }) + + t.Run("output is bounded", func(t *testing.T) { + got := safeSubprocessOutput(bytes.Repeat([]byte("x"), maxSubprocessOutput*4), secret.Value{}) + + if len(got) > maxSubprocessOutput+len("\n… truncated") { + t.Fatalf("output was not bounded: %d bytes", len(got)) + } + if !strings.Contains(got, "truncated") { + t.Errorf("a truncated result should say so") + } + }) + + t.Run("a classic call has no credential to remove", func(t *testing.T) { + if got := safeSubprocessOutput([]byte("plain diagnostics"), secret.Value{}); got != "plain diagnostics" { + t.Fatalf("got %q", got) + } + }) +} diff --git a/internal/server/requests.go b/internal/server/requests.go index 582bbe6..c829401 100644 --- a/internal/server/requests.go +++ b/internal/server/requests.go @@ -102,10 +102,13 @@ func (c *FormaeClient) send(ctx context.Context, r request) ([]byte, int, error) // call Do", because a DNS or TLS failure sends nothing and telling an // operator to go and check would be a false alarm in the costly direction. req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{ - WroteRequest: func(info httptrace.WroteRequestInfo) { - if info.Err == nil { - c.advance(reachAttempted) - } + WroteRequest: func(httptrace.WroteRequestInfo) { + // Any invocation counts, including one carrying an error. The + // callback runs only after a connection exists and a write was + // attempted, and a write can fail having already put part of a + // multipart mutation on the wire. DNS, connection and TLS failures + // never reach here, which is the distinction this is for. + c.advance(reachAttempted) }, })) diff --git a/internal/server/server.go b/internal/server/server.go index b9cad93..d8333f8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -18,6 +18,7 @@ import ( "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" "github.com/platform-engineering-labs/formae-mcp/internal/formaebin" "github.com/platform-engineering-labs/formae-mcp/internal/profile" + "github.com/platform-engineering-labs/formae-mcp/internal/secret" "github.com/platform-engineering-labs/formae-mcp/internal/tools" "github.com/platform-engineering-labs/formae-mcp/internal/version" ) @@ -615,7 +616,8 @@ func (s *Server) handleExtractResources(ctx context.Context, _ *mcp.CallToolRequ cmd := commandWithContext(ctx, ec.FormaeBin, args...) if output, err := cmd.CombinedOutput(); err != nil { return attribute(resolved(ec), - errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", err, string(output)))), nil, nil + errorResult(fmt.Errorf("formae extract failed: %w\noutput: %s", + err, safeSubprocessOutput(output, ec.Credential)))), nil, nil } extracted := destination{ec: ec, reach: reachAnswered} @@ -843,6 +845,30 @@ func (s *Server) handleForceReconcileStack(ctx context.Context, _ *mcp.CallToolR return attribute(reached(ec, c), jsonResult(body)), nil, nil } +// maxSubprocessOutput bounds the diagnostics a failed CLI invocation may put +// into a tool result. Unlike the configuration oracle, which reports an exit +// status and never the bytes, extract's output is the user's own Pkl and +// plugin diagnostics and is the whole value of the failure, so it is kept — +// bounded, and with the credential we handed the process removed. +const maxSubprocessOutput = 8 << 10 + +// safeSubprocessOutput bounds subprocess output and removes the credential +// this call resolved. +// +// It covers the credential we gave the process. A credential the CLI minted +// for itself is not ours to know, and masking that is the CLI's own job on its +// own output — said plainly here because "output is sanitised" would be a +// wider claim than this makes good. +func safeSubprocessOutput(output []byte, credential secret.Value) string { + if len(output) > maxSubprocessOutput { + output = append(output[:maxSubprocessOutput:maxSubprocessOutput], []byte("\n… truncated")...) + } + if credential.IsZero() { + return string(output) + } + return strings.ReplaceAll(string(output), credential.Reveal(), secret.Mask) +} + // Helpers func evalFormaFile(ctx context.Context, ec execctx.Context, filePath string) ([]byte, error) { From 3686a3b7062191b86d899c16806d42aa20298d48 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 01:20:32 -0700 Subject: [PATCH 12/14] fix(server): scrub extract output before truncating it Truncating first leaves a credential that straddles the cutoff partly intact: the search string is no longer present in the truncated bytes, so nothing is replaced and all but the tail of the token survives. The test walks the credential across the boundary a byte at a time. --- internal/server/attribution_test.go | 22 ++++++++++++++++++++++ internal/server/server.go | 19 ++++++++++++------- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/internal/server/attribution_test.go b/internal/server/attribution_test.go index 3856f32..a65648a 100644 --- a/internal/server/attribution_test.go +++ b/internal/server/attribution_test.go @@ -330,6 +330,28 @@ func TestExtractFailureOutputIsBoundedAndScrubbed(t *testing.T) { } }) + // The order matters: truncating first would leave a credential that + // straddles the cutoff partly intact, because the search string is no + // longer present in the truncated bytes. + t.Run("a credential straddling the cutoff is still removed", func(t *testing.T) { + const cred = "Bearer sup3rs3cr3t-and-then-some-more" + for offset := -len(cred); offset <= 1; offset++ { + start := maxSubprocessOutput + offset + if start < 0 { + continue + } + raw := append(bytes.Repeat([]byte("x"), start), []byte(cred)...) + raw = append(raw, bytes.Repeat([]byte("y"), 128)...) + + got := safeSubprocessOutput(raw, secret.New(cred)) + + if strings.Contains(got, "sup3rs3cr3t") { + t.Fatalf("a credential starting at offset %d survived truncation: %q", + start, got[max(0, len(got)-160):]) + } + } + }) + t.Run("a classic call has no credential to remove", func(t *testing.T) { if got := safeSubprocessOutput([]byte("plain diagnostics"), secret.Value{}); got != "plain diagnostics" { t.Fatalf("got %q", got) diff --git a/internal/server/server.go b/internal/server/server.go index d8333f8..f335d68 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -860,13 +860,18 @@ const maxSubprocessOutput = 8 << 10 // own output — said plainly here because "output is sanitised" would be a // wider claim than this makes good. func safeSubprocessOutput(output []byte, credential secret.Value) string { - if len(output) > maxSubprocessOutput { - output = append(output[:maxSubprocessOutput:maxSubprocessOutput], []byte("\n… truncated")...) - } - if credential.IsZero() { - return string(output) - } - return strings.ReplaceAll(string(output), credential.Reveal(), secret.Mask) + // Scrub first, then truncate. The other order leaves a credential that + // straddles the cutoff partly intact: the search string is no longer + // present in the truncated bytes, so nothing is replaced and all but the + // tail of the token survives. + scrubbed := string(output) + if !credential.IsZero() { + scrubbed = strings.ReplaceAll(scrubbed, credential.Reveal(), secret.Mask) + } + if len(scrubbed) > maxSubprocessOutput { + scrubbed = scrubbed[:maxSubprocessOutput] + "\n… truncated" + } + return scrubbed } // Helpers From 04a7271e6eeb999ca78d2ac7311a5dbd29018f4f Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Sun, 16 Aug 2026 14:39:05 -0700 Subject: [PATCH 13/14] fix(server): explain a hosted 404 everywhere, not just on collections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An installation can disappear underneath a live session: a trial ends, a subscription lapses, someone suspends or destroys it. Sessions stay open for days, so this is an ordinary event, and when it happens every call fails at once. Only the six collection endpoints explained it. The rest asserted their own reading of a 404 — an unhealthy agent, a bare status with the edge's body pasted in, and worst, a command that was not found, which sends the reader hunting for something that was never the problem. The endpoints whose 404 has no object reading now report the routing failure outright, since the agent answers those with 200 or an error and never 404. get_command_status reports both readings, because the status alone cannot separate them and asserting either would be a claim this cannot support. Classic is untouched: a self-hosted agent answers for itself, so its 404 means what the endpoint says it means. --- CHANGELOG.md | 2 +- internal/server/client.go | 64 ++++++++++++++++++++++ internal/server/collection_test.go | 88 ++++++++++++++++++++++++++++-- internal/server/routing.go | 31 +++++++++-- 4 files changed, 174 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e24f8da..3b660aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ Install via the - Commands issued through the MCP (apply, destroy, cancel, status, list) now identify with your CLI's client ID (`~/.pel/formae/cli_client_id`) instead of a fixed `formae-mcp` identity, so the agent attributes them to the same client as your own `formae` runs. When the ID file does not exist yet, the MCP runs `formae --version` once so formae creates it, and falls back to the old `formae-mcp` identity if it still cannot be read. - Configuration and credentials now come from a single `formae connection resolve` per tool call, replacing `formae profile show`, so the MCP and your own `formae` runs always agree on where a profile points and a request can never combine one profile revision's endpoint with another's credential. Requires formae 0.89.0 or newer. - An expired hosted credential is refreshed and the call retried once, but only for reads. A change that fails with an expired credential refreshes it for next time and reports the failure rather than being sent twice. -- On a hosted profile, a listing endpoint that answers "not found" is now reported as a routing problem rather than as an empty result, since the shared endpoint answers that way for an installation it cannot route to. +- On a hosted profile, "not found" from the shared endpoint is now explained rather than passed on. It answers that way for an installation it can no longer route to — one that was suspended or destroyed, or whose subscription lapsed — which can happen part-way through a long session. Every tool now says so, instead of reporting an empty result, an unhealthy agent, or a command that was never missing. - When several profiles exist and none is named, a hosted call now lists the candidates and asks for the `profile` argument instead of guessing. - Every agent request is built by one internal executor, so cancellation and timeouts apply uniformly across every tool. - The plugin no longer installs a second `formae` alongside one you already have. On launch it looks for yours (`PATH`, then `/opt/pel/bin`, `/usr/local/bin`, `~/.local/bin`, `~/bin`) and uses it; it downloads one into `~/.formae-ai/opt` only when the machine has none. Previously it downloaded a copy on every launch and then ran whichever `formae` came first on `PATH`, so the downloaded one was usually dead weight — and `/formae:upgrade` could upgrade a copy the plugin was not running. diff --git a/internal/server/client.go b/internal/server/client.go index b59e448..ebae064 100644 --- a/internal/server/client.go +++ b/internal/server/client.go @@ -112,6 +112,9 @@ func (c *FormaeClient) ListResources(ctx context.Context, query string) (json.Ra if status == http.StatusNotFound { return c.route.collectionMiss(json.RawMessage("[]")) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -128,6 +131,9 @@ func (c *FormaeClient) ListStacks(ctx context.Context) (json.RawMessage, error) if status == http.StatusNotFound { return c.route.collectionMiss(json.RawMessage("[]")) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -144,6 +150,9 @@ func (c *FormaeClient) ListPolicies(ctx context.Context) (json.RawMessage, error if status == http.StatusNotFound { return c.route.collectionMiss(json.RawMessage("[]")) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -165,6 +174,9 @@ func (c *FormaeClient) ListTargets(ctx context.Context, query string) (json.RawM if status == http.StatusNotFound { return c.route.collectionMiss(json.RawMessage("[]")) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -187,8 +199,19 @@ func (c *FormaeClient) GetCommandStatus(ctx context.Context, commandID string, c return nil, err } if status == http.StatusNotFound { + // Both readings, because the status alone cannot separate them: the + // agent answers 404 for a command it does not know, and the edge + // answers 404 for an installation it cannot route. Asserting the + // first sends the reader hunting for a command that was never the + // problem. + if unrouted := c.route.unrouted(); unrouted != nil { + return nil, fmt.Errorf("command %s was not found, or %w", commandID, unrouted) + } return nil, fmt.Errorf("command %s not found", commandID) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -218,6 +241,9 @@ func (c *FormaeClient) ListCommands(ctx context.Context, query string, maxResult if status == http.StatusNotFound { return c.route.collectionMiss(json.RawMessage(`{"Commands":[]}`)) } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -231,6 +257,9 @@ func (c *FormaeClient) GetAgentStats(ctx context.Context) (json.RawMessage, erro if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -244,6 +273,9 @@ func (c *FormaeClient) CheckHealth(ctx context.Context) error { if err != nil { return fmt.Errorf("agent is not reachable: %w", err) } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned unhealthy status: %d", status) } @@ -276,6 +308,9 @@ func (c *FormaeClient) SubmitCommand(ctx context.Context, command string, mode s if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if !isCommandStatusOK(status, simulate) { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -295,6 +330,9 @@ func (c *FormaeClient) DestroyByQuery(ctx context.Context, query string, simulat if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if !isCommandStatusOK(status, simulate) { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -342,6 +380,9 @@ func (c *FormaeClient) ListChangesSinceLastReconcile(ctx context.Context, stack if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -355,6 +396,9 @@ func (c *FormaeClient) ForceSync(ctx context.Context) error { if err != nil { return err } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned status %d", status) } @@ -368,6 +412,9 @@ func (c *FormaeClient) ForceDiscover(ctx context.Context) error { if err != nil { return err } + if err := c.unroutedIf(status); err != nil { + return err + } if status != http.StatusOK { return fmt.Errorf("agent returned status %d", status) } @@ -381,6 +428,9 @@ func (c *FormaeClient) ForceCheckTTL(ctx context.Context) (json.RawMessage, erro if err != nil { return nil, err } + if err := c.unroutedIf(status); err != nil { + return nil, err + } if status != http.StatusOK { return nil, fmt.Errorf("agent returned status %d: %s", status, string(body)) } @@ -396,6 +446,9 @@ func (c *FormaeClient) ForceReconcileStack(ctx context.Context, label string) (j if err != nil { return nil, 0, err } + if err := c.unroutedIf(status); err != nil { + return body, status, err + } if status != http.StatusOK && status != http.StatusAccepted { return body, status, fmt.Errorf("agent returned status %d", status) } @@ -435,3 +488,14 @@ func (c *FormaeClient) postMultipartWithHeaders(ctx context.Context, path string ContentType: w.FormDataContentType(), }, noRetry) } + +// unroutedIf explains a 404 from an endpoint that has no "this object does not +// exist" reading. The agent answers those with 200 or an error, never 404, so +// under hosted a 404 came from the edge rather than from the installation. +// Classic reports nothing and the caller's own status handling stands. +func (c *FormaeClient) unroutedIf(status int) error { + if status != http.StatusNotFound { + return nil + } + return c.route.unrouted() +} diff --git a/internal/server/collection_test.go b/internal/server/collection_test.go index 9e68661..d222d53 100644 --- a/internal/server/collection_test.go +++ b/internal/server/collection_test.go @@ -81,8 +81,8 @@ func TestHosted404OnACollectionIsARoutingError(t *testing.T) { if err == nil { t.Fatalf("%s: a hosted 404 must not read as empty, got %s", name, got) } - if !strings.Contains(err.Error(), "routing") { - t.Errorf("%s: the error should name routing as the likely cause: %v", name, err) + if !strings.Contains(err.Error(), "did not route") { + t.Errorf("%s: the error should say the request was not routed: %v", name, err) } if !strings.Contains(err.Error(), testInstallation) { t.Errorf("%s: the error should name the installation it addressed: %v", name, err) @@ -105,15 +105,91 @@ func TestCommandStatus404KeepsItsMeaning(t *testing.T) { } }) + // Under hosted the same 404 has two readings and the status cannot separate + // them, so it reports both rather than asserting the one that sends the + // reader hunting for a command that was never the problem. t.Run("hosted", func(t *testing.T) { c := newTestHostedClient(t, notFoundServer(t), "Bearer live-token", nil) _, err := c.GetCommandStatus(context.Background(), "cmd-1", "cid") - if err == nil || !strings.Contains(err.Error(), "not found") { - t.Fatalf("want a command-not-found error, got %v", err) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "cmd-1 was not found") { + t.Errorf("the object reading must survive: %v", err) } - if strings.Contains(err.Error(), "routing") { - t.Fatalf("an object-not-found must not be reported as a routing failure: %v", err) + if !strings.Contains(err.Error(), "did not route") { + t.Errorf("the routing reading must be offered too: %v", err) } }) } + +// An installation can disappear underneath a live session: a trial ends, a +// subscription lapses, someone suspends or destroys it. Sessions stay open for +// days, so this is an ordinary event rather than an exotic one, and when it +// happens every call fails at once. Each of them has to say the same true +// thing rather than three different misleading ones. +func TestEveryCallExplainsAGoneInstallation(t *testing.T) { + srv := notFoundServer(t) + c := newTestHostedClient(t, srv, "Bearer live-token", nil) + ctx := context.Background() + + calls := map[string]func() error{ + "ListResources": func() error { _, e := c.ListResources(ctx, ""); return e }, + "ListStacks": func() error { _, e := c.ListStacks(ctx); return e }, + "ListTargets": func() error { _, e := c.ListTargets(ctx, ""); return e }, + "ListPolicies": func() error { _, e := c.ListPolicies(ctx); return e }, + "ListCommands": func() error { _, e := c.ListCommands(ctx, "", "10", "cid"); return e }, + "CancelCommands": func() error { _, e := c.CancelCommands(ctx, "", "cid"); return e }, + "GetCommandStatus": func() error { + _, e := c.GetCommandStatus(ctx, "cmd-1", "cid") + return e + }, + "CheckHealth": func() error { return c.CheckHealth(ctx) }, + "GetAgentStats": func() error { _, e := c.GetAgentStats(ctx); return e }, + "ListChanges": func() error { _, e := c.ListChangesSinceLastReconcile(ctx, "default"); return e }, + "SubmitCommand": func() error { + _, e := c.SubmitCommand(ctx, "apply", "reconcile", false, false, []byte("{}"), "cid") + return e + }, + "DestroyByQuery": func() error { _, e := c.DestroyByQuery(ctx, "stack:x", false, "cid"); return e }, + "ForceSync": func() error { return c.ForceSync(ctx) }, + "ForceDiscover": func() error { return c.ForceDiscover(ctx) }, + "ForceCheckTTL": func() error { _, e := c.ForceCheckTTL(ctx); return e }, + "ForceReconcile": func() error { _, _, e := c.ForceReconcileStack(ctx, "default"); return e }, + } + for name, call := range calls { + t.Run(name, func(t *testing.T) { + err := call() + if err == nil { + t.Fatal("a gone installation must not read as success") + } + if !strings.Contains(err.Error(), testInstallation) { + t.Errorf("must name the installation that could not be reached: %v", err) + } + if !strings.Contains(err.Error(), "subscription") { + t.Errorf("must offer the reason a reader can act on: %v", err) + } + // The edge's body is not an explanation and must not be pasted in. + if strings.Contains(err.Error(), "404 page not found") { + t.Errorf("the endpoint's body reached the caller: %v", err) + } + }) + } +} + +// The same 404 against a self-hosted agent keeps meaning what the endpoint says +// it means. Only the shared edge is ambiguous. +func TestClassicKeepsItsOwn404Meanings(t *testing.T) { + c := newTestFormaeClient(notFoundServer(t)) + ctx := context.Background() + + if _, err := c.GetCommandStatus(ctx, "cmd-1", "cid"); err == nil || + !strings.Contains(err.Error(), "command cmd-1 not found") || + strings.Contains(err.Error(), "subscription") { + t.Errorf("a classic command lookup must report the command, nothing more: %v", err) + } + if err := c.CheckHealth(ctx); err == nil || !strings.Contains(err.Error(), "unhealthy status: 404") { + t.Errorf("classic health keeps its own wording: %v", err) + } +} diff --git a/internal/server/routing.go b/internal/server/routing.go index 19311b6..88d5f89 100644 --- a/internal/server/routing.go +++ b/internal/server/routing.go @@ -39,6 +39,10 @@ type routing interface { // collectionMiss answers a 404 from an endpoint that lists things. collectionMiss(empty json.RawMessage) (json.RawMessage, error) + // unrouted reports the error for a 404 this connection cannot explain as an + // object simply being absent, or nil when it can. + unrouted() error + // scrub removes any credential this connection has sent from bytes the far // end returned, before they can reach a result, an error, or a log. scrub(body []byte) []byte @@ -65,6 +69,10 @@ func (classicRoute) collectionMiss(empty json.RawMessage) (json.RawMessage, erro // response cannot be quoting one. func (classicRoute) scrub(body []byte) []byte { return body } +// unrouted reports nothing. A self-hosted agent answers for itself, so its 404 +// means whatever the endpoint says a 404 means. +func (classicRoute) unrouted() error { return nil } + // refresh does nothing. The MCP sends a self-hosted agent no credential, so // there is nothing a second attempt would do differently. func (classicRoute) refresh(context.Context) (bool, error) { return false, nil } @@ -114,10 +122,25 @@ func (r *hostedRoute) decorate(h http.Header) { // shared edge answers 404 for an unknown or unrouted installation, and "no // resources" would hide that behind a plausible answer. func (r *hostedRoute) collectionMiss(json.RawMessage) (json.RawMessage, error) { - return nil, fmt.Errorf( - "the hosted endpoint did not route this request to installation %s; "+ - "this is more likely a routing problem than an empty result", - r.installation) + return nil, fmt.Errorf("%w; reporting this as an empty result would hide it", r.unrouted()) +} + +// unrouted reports that the shared endpoint may not have reached this +// installation at all. +// +// The edge answers 404 for an installation it cannot route — one that has been +// suspended, destroyed, or reaped when a trial or subscription ended — and that +// is indistinguishable from any other 404 without an edge error envelope, which +// does not exist. It happens on an ordinary day: a session left open for days +// outlives the installation it was working against. +// +// So this never asserts a cause. Callers that could legitimately receive a 404 +// report both possibilities; callers whose endpoint has no such reading report +// this alone. +func (r *hostedRoute) unrouted() error { + return fmt.Errorf( + "the hosted endpoint did not route this request to installation %s, which happens when an "+ + "installation is suspended, destroyed, or no longer covered by a subscription", r.installation) } // refresh re-resolves with the credential refreshed and refuses to let the From e856f833e7ba24b6f1f4c75770994f20b598f2b6 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Mon, 31 Aug 2026 17:32:13 -0700 Subject: [PATCH 14/14] refactor(routing): drop the unused withEndpoint helper Its comment says it exists for tests, but no test calls it: the hosted test clients are built by newTestHostedClientAt. Dead code that fails the unused linter. --- internal/server/routing.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/internal/server/routing.go b/internal/server/routing.go index 88d5f89..e68f5ca 100644 --- a/internal/server/routing.go +++ b/internal/server/routing.go @@ -195,15 +195,6 @@ func (r *hostedRoute) scrub(body []byte) []byte { return body } -// withEndpoint returns a copy addressing a different origin. It exists for -// tests, which need the validated hosted routing behaviour aimed at a local -// server rather than at the real edge. -func (r *hostedRoute) withEndpoint(endpoint string) *hostedRoute { - copied := *r - copied.endpoint = endpoint - return &copied -} - var errConnectionMoved = errors.New( "the connection changed while this request was in flight, so it was abandoned rather than " + "retried against a different installation")