diff --git a/dispatch.go b/dispatch.go index 643e13d..25ddece 100644 --- a/dispatch.go +++ b/dispatch.go @@ -179,6 +179,10 @@ func invoke(ctx context.Context, subject authz.Subject, entry catalog.Entry, inp if errors.Is(outcome.err, catalog.ErrInputTypeMismatch) { return nil, fmt.Errorf("%w: %w", execution.ErrInternal, outcome.err) } + var agentErr *AgentError + if errors.As(outcome.err, &agentErr) && agentErr != nil { + return nil, execution.WithSafeDetail(execution.ErrCapabilityFailure, sanitizeAgentMessage(agentErr.Message)) + } return nil, fmt.Errorf("%w: %w", execution.ErrCapabilityFailure, outcome.err) } diff --git a/docs/docs/explanation/security-model.md b/docs/docs/explanation/security-model.md index 0eae0e9..e2d2eb0 100644 --- a/docs/docs/explanation/security-model.md +++ b/docs/docs/explanation/security-model.md @@ -105,11 +105,13 @@ enabled discovery metadata based on the resolved subject. Detailed causes exist only on the trusted side of the public boundary. Internal packages and host authorizers, resolvers, and handlers can hold or log those causes. A direct call to `authz/rego.Authorize` can return an ordinary error that identifies an undefined or non-Boolean decision, or carries an OPA evaluation or builtin failure. -`codemode.Server.Execute` removes those trusted causes. It returns the documented public sentinel for execution, policy, handler, resource, and internal failures. Request cancellation returns `context.Canceled`. A deadline returns `ErrResourceLimit` and preserves `context.DeadlineExceeded` for `errors.Is`. Root `Error()` strings stay exactly coarse. Approved model-derived parser, resolver, and binding detail may travel with the sentinel for MCP formatting, but it is not part of the root error text. +`codemode.Server.Execute` removes trusted causes. It returns the documented public sentinel for execution, policy, handler, resource, and internal failures. Request cancellation returns `context.Canceled`. A deadline returns `ErrResourceLimit` and preserves `context.DeadlineExceeded` for `errors.Is`. Root `Error()` strings stay exactly coarse. Approved parser, resolver, and binding detail, and explicitly disclosed handler messages, may travel with the sentinel for MCP formatting, but are not part of the root error text. -The MCP adapter narrows the boundary again. It emits the nine fixed error texts in the [MCP tool reference](../reference/mcp-tools.md#errors), plus two stable prefixes that may append approved CodeMode execution detail: `invalid program: ...` for parse and resolve positions and messages, and `invalid capability arguments: ...` for binding diagnostics. Resolver and custom-service details and recovered panic values become coarse responses. SDK input-schema errors are different: they occur before trusted subject resolution and can identify malformed client-owned fields or values. +The MCP adapter narrows the boundary again. The [MCP tool reference](../reference/mcp-tools.md#errors) lists the fixed categories and three suffix forms: `invalid program: ...` for parse and resolve positions and messages, `invalid capability arguments: ...` for binding diagnostics, and `capability failed: ...` for handler-authored messages. Resolver and arbitrary custom-service details and recovered panic values become coarse responses. SDK input-schema errors are different: they occur before trusted subject resolution and can identify malformed client-owned fields or values. -This projection prevents host-derived diagnostic detail from becoming model-visible. MCP responses do not expose budget values, filtered capability identities, unknown requested names, host-derived argument values, Rego decision paths or rule names, handler messages, credentials, panic values, or stack details. The only MCP exceptions are parse or resolve positions and messages and binding argument diagnostics produced by the program that the service executed. With the shipped `*codemode.Server`, that program is the submitted `source`. +Handler text stays hidden by default. A handler can deliberately disclose a message by returning or wrapping [`*codemode.AgentError`](../reference/public-api.md#agenterror). Only its `Message` crosses the worker and MCP boundaries, never surrounding error text or another cause. CodeMode replaces non-printable runes with spaces, converts invalid UTF-8 to replacement characters, and bounds the suffix to 256 UTF-8 bytes, including `...` when truncated. The worker still aborts the program; this does not add Starlark exception handling. Policy errors, panic values, and return-value conversion failures stay bare even if an `AgentError` is involved. + +Sanitization bounds presentation; it does not redact secrets. As with `SearchTerms`, the host author is responsible for treating `AgentError.Message` as model-visible data. Do not put credentials, sensitive tenant information, policy facts, or backend diagnostics in it. A wrong-name or not-ready message should reveal only resource information the caller is allowed to learn. Ordinary host-derived diagnostics, budget values, filtered capability identities, Rego decision paths, panic values, and stack details remain excluded unless a host deliberately puts such information into an agent-facing message. If a host needs detailed diagnostics, its trusted authorizer, resolver, or handler must record them before returning. CodeMode cannot recover a discarded cause after the root or MCP projection. Apply the host's normal access controls and redaction rules to those logs. diff --git a/docs/docs/reference/mcp-tools.md b/docs/docs/reference/mcp-tools.md index d40f7e3..4ac8ae7 100644 --- a/docs/docs/reference/mcp-tools.md +++ b/docs/docs/reference/mcp-tools.md @@ -426,11 +426,12 @@ Only the final converted value from the worker process is exposed in the success ## Authoring and recovery -The listed descriptions above are the model-facing contract. Recovery uses the nine fixed texts and two stable prefixes on this page. When recording or reporting a failed call, keep the error text and the recovery action; do not echo credentials, unknown requested names, or host-derived handler or policy text. +The listed descriptions above are the model-facing contract. Recovery uses the fixed texts and three stable suffix forms on this page. When recording or reporting a failed call, keep the error text and the recovery action; do not add credentials or undisclosed host diagnostics. - Search with task, resource, or exact-name vocabulary. If `truncated` is `true`, use a more specific task/resource query. Pass an exact returned `name` to `describe_api`. - After `capability not found`, search again and pass `describe_api` an exact returned `name`, without whitespace or case changes. - After `invalid capability arguments`, use any suffix after the stable prefix to identify the rejected argument, then compare the call with the published `signature` and `input` field shapes. +- After `capability failed`, use any handler-authored suffix to choose the next action, such as correcting a resource name or waiting for it to become ready. The failed program has aborted; submit a new program to retry. - After `invalid program`, use any suffix after the stable prefix. A parse or resolve suffix includes a `:line:col:` position in the submitted source. Check the program against these requirements: - Write Starlark, not Python: `import`, `while`, f-strings, `filter`, and `map` are unavailable; `sum(iterable)`, `json.decode/encode/indent`, and `math.*` are directly available without import. - Define `main` with zero arguments. @@ -445,7 +446,7 @@ The listed descriptions above are the model-facing contract. Recovery uses the n ## Errors -After a well-formed call reaches the adapter, a resolver or service failure becomes a successful MCP protocol response with `isError` set. Nine texts are fixed. Two classes keep a stable prefix and may append model-derived detail: `invalid program: ...` and `invalid capability arguments: ...`. The adapter removes resolver and custom-service details and recovered panic values. It does not expose budget values, filtered capability identities, unknown requested names, host-derived argument values, Rego decision paths or rule names, handler messages, credentials, panic values, or stack details. Parse and resolve suffixes may include a source position in the submitted program. Binding suffixes may include an argument name from the submitted call. +After a well-formed call reaches the adapter, a resolver or service failure becomes a successful MCP protocol response with `isError` set. Error categories keep stable text. `invalid program: ...` and `invalid capability arguments: ...` may append model-derived detail. `capability failed: ...` may append a message explicitly disclosed by the handler through `codemode.AgentError`. The adapter removes resolver and arbitrary custom-service details and recovered panic values. Parse and resolve suffixes may include a source position in the submitted program. Binding suffixes may include an argument name from the submitted call. | Text | Meaning | | --- | --- | @@ -457,6 +458,7 @@ After a well-formed call reaches the adapter, a resolver or service failure beco | `authorization policy failure` | Policy evaluation failed. | | `resource limit exceeded` | A discovery, execution, depth, per-value, or aggregate intermediate-value budget was exceeded. | | `capability failed` | A handler failed or returned an invalid value, including a non-finite float or an unsigned integer above `math.MaxInt64`. | +| `capability failed: ...` | A handler returned or wrapped `*codemode.AgentError`. Only its `Message` is disclosed: non-printable runes become spaces, invalid UTF-8 becomes replacement characters, and the suffix is truncated on a rune boundary to at most 256 bytes including trailing `...`. An empty message leaves the failure bare. | | `context canceled` | The request context was canceled. | | `context deadline exceeded` | A service returned a bare deadline error. Root CodeMode execution deadlines are projected as `resource limit exceeded`. | | `internal failure` | Any unknown service error or recovered adapter failure. | diff --git a/docs/docs/reference/public-api.md b/docs/docs/reference/public-api.md index 17690f0..6a9c6f3 100644 --- a/docs/docs/reference/public-api.md +++ b/docs/docs/reference/public-api.md @@ -87,6 +87,38 @@ func(context.Context, authz.Subject, Input) (Output, error) The subject is the trusted subject supplied to `Server.Execute`. The input and output are the exact generic types registered for the capability. +#### `AgentError` + +`AgentError` opts a handler failure into agent-visible detail: + +```go +return output, &codemode.AgentError{ + Message: `instance "web" not found in sandbox "demo"`, +} +``` + +`Message string` is the explanation the handler author has chosen to disclose. +Return `*AgentError` directly or wrap it with `fmt.Errorf("lookup: %w", err)`; +CodeMode finds it with `errors.As` and attaches only `Message`, not wrapper +text or other causes. The MCP error is +`capability failed: instance "web" not found in sandbox "demo"`. + +CodeMode replaces control characters and other non-printable runes with spaces, +including newlines and tabs. Invalid UTF-8 bytes become replacement characters. +The sanitized suffix is at most 256 UTF-8 bytes, including a trailing `...` when +truncated; truncation does not split a rune. An empty message or nil +`*AgentError` leaves the failure bare. `AgentError.Error()` returns the original +message (or an empty string for a nil receiver), not the sanitized suffix. + +The failure still aborts the Starlark program. `Server.Execute` retains the +coarse `Error()` text `capability failed` and supports +`errors.Is(err, codemode.ErrCapabilityFailure)`; the MCP adapter formats the +suffix. Ordinary handler errors remain hidden. Panic values, policy errors, +and invalid handler return values cannot opt in through `AgentError`. +The host is responsible for keeping secrets and sensitive data out of `Message`. + +#### Capability identity example + The policy and deployment-filter examples use this explicit capability identity: | Property | Value | diff --git a/errors.go b/errors.go index 6d18225..56fc456 100644 --- a/errors.go +++ b/errors.go @@ -1,6 +1,12 @@ package codemode -import "errors" +import ( + "errors" + "unicode" + "unicode/utf8" + + "github.com/meigma/codemode/internal/execution" +) var ( // ErrInvalidRegistration classifies invalid capability registration, limits, or server construction. @@ -33,3 +39,43 @@ var ( // ErrInternal classifies an unexpected framework failure. ErrInternal = errors.New("internal failure") ) + +// AgentError carries a message the handler author has chosen to expose to the agent. +// +// Return it directly or wrap it using %w. Only Message is +// exposed, never surrounding error text. CodeMode replaces non-printable runes +// with spaces and truncates the message to 256 UTF-8 bytes, including a trailing +// "..." when truncated. Empty messages leave the capability failure bare. +// The host must not put secrets or other sensitive data in Message. +type AgentError struct { + // Message is the handler-authored, agent-facing failure explanation. + Message string +} + +// Error returns the handler-authored message before sanitization. +func (err *AgentError) Error() string { + if err == nil { + return "" + } + return err.Message +} + +// sanitizeAgentMessage bounds work and output independently of handler message size. +func sanitizeAgentMessage(message string) string { + var buffer [execution.MaxAgentErrorBytes]byte + output := buffer[:0] + for _, char := range message { + if !unicode.IsPrint(char) { + char = ' ' + } + if len(output)+utf8.RuneLen(char) > len(buffer) { + end := len(buffer) - len("...") + for end < len(output) && !utf8.RuneStart(output[end]) { + end-- + } + return string(append(output[:end], "..."...)) + } + output = utf8.AppendRune(output, char) + } + return string(output) +} diff --git a/internal/execution/detail.go b/internal/execution/detail.go index 0218855..802208c 100644 --- a/internal/execution/detail.go +++ b/internal/execution/detail.go @@ -2,6 +2,10 @@ package execution import "errors" +// MaxAgentErrorBytes is the maximum UTF-8 size of an approved agent-visible +// capability-failure suffix, including a trailing ASCII ellipsis when truncated. +const MaxAgentErrorBytes = 256 + // safeDetailError attaches one model-derived diagnostic suffix without changing the coarse error text. type safeDetailError struct { // cause is the coarse classified sentinel. @@ -24,7 +28,8 @@ func (err *safeDetailError) Unwrap() error { // WithSafeDetail attaches detail to cause without changing cause.Error. // // Empty detail returns cause unchanged. Callers must pass only model-derived -// suffixes; host-derived text must not be attached. +// suffixes or sanitized explicitly handler-authored detail; host-derived text +// must not be attached. func WithSafeDetail(cause error, detail string) error { if detail == "" { return cause @@ -32,7 +37,7 @@ func WithSafeDetail(cause error, detail string) error { return &safeDetailError{cause: cause, detail: detail} } -// SafeDetail reports the model-derived suffix attached to err, if any. +// SafeDetail reports the approved suffix attached to err, if any. // // Extraction follows the error chain with [errors.As]. func SafeDetail(err error) (string, bool) { diff --git a/internal/execution/detail_test.go b/internal/execution/detail_test.go index 7b3ec4f..e6f751f 100644 --- a/internal/execution/detail_test.go +++ b/internal/execution/detail_test.go @@ -116,3 +116,23 @@ func TestExecuteKeepsGenericRuntimeErrorsCoarse(t *testing.T) { assert.NotContains(t, err.Error(), "db password rejected") assert.NotContains(t, err.Error(), "fail") } + +// TestExecutePreservesApprovedCapabilitySafeDetail proves handler-authored suffixes +// survive classification without changing the coarse Error text. +func TestExecutePreservesApprovedCapabilitySafeDetail(t *testing.T) { + const detail = `instance "web" not found in sandbox "demo"` + _, err := buildEngine(t).Execute( + `def main(): return records.lookup(value="alpha")`, + func(string, map[string]any) (any, error) { + return nil, execution.WithSafeDetail(execution.ErrCapabilityFailure, detail) + }, + defaultExecutionLimits(), + ) + + require.ErrorIs(t, err, execution.ErrCapabilityFailure) + assert.Equal(t, execution.ErrCapabilityFailure.Error(), err.Error()) + got, ok := execution.SafeDetail(err) + require.True(t, ok) + assert.Equal(t, detail, got) + assert.NotContains(t, err.Error(), detail) +} diff --git a/internal/execution/execute.go b/internal/execution/execute.go index 377882a..4a02ce5 100644 --- a/internal/execution/execute.go +++ b/internal/execution/execute.go @@ -204,7 +204,7 @@ func classifyRuntimeError(state *executionState, err error) error { case errors.Is(cause, ErrResourceLimit): return ErrResourceLimit case errors.Is(cause, ErrCapabilityFailure): - return ErrCapabilityFailure + return classifiedSafeDetail(ErrCapabilityFailure, cause) case errors.Is(cause, ErrInternal): return ErrInternal case errors.Is(cause, ErrInvalidProgram): diff --git a/internal/worker/child.go b/internal/worker/child.go index dc9b2af..a88978d 100644 --- a/internal/worker/child.go +++ b/internal/worker/child.go @@ -15,6 +15,25 @@ import ( // remains authoritative because Engine intentionally coarsens unknown errors. var errNativeAbort = errors.New("native abort") +// capabilityAbortError is the Starlark-visible unwind for an approved parent abort suffix. +type capabilityAbortError struct { + // detail is the approved capability-failure suffix. + detail string + + // cause retains classification and detail without allocating during unwrapping. + cause error +} + +// Error returns the Starlark-visible capability-failure text. +func (err *capabilityAbortError) Error() string { + return execution.ErrCapabilityFailure.Error() + ": " + err.detail +} + +// Unwrap preserves [execution.ErrCapabilityFailure] and the approved SafeDetail. +func (err *capabilityAbortError) Unwrap() error { + return err.cause +} + // errChildService classifies a child protocol or internal service failure that // must not become a final_error frame. var errChildService = errors.New("worker service failure") @@ -154,13 +173,24 @@ func nativeForwarder(conn *childConn) execution.NativeCall { case nativeResultFrame: return typed.Result, nil case nativeAbortFrame: - return nil, errNativeAbort + return nil, nativeAbortError(typed.Detail) default: return nil, errChildService } } } +// nativeAbortError maps a decoded abort frame onto the interpreter unwind error. +func nativeAbortError(detail string) error { + if detail == "" { + return errNativeAbort + } + return &capabilityAbortError{ + detail: detail, + cause: execution.WithSafeDetail(execution.ErrCapabilityFailure, detail), + } +} + // writeExecutionError maps an Engine failure onto abort suppression or a final_error. func writeExecutionError(conn *childConn, err error) error { if errors.Is(err, errNativeAbort) { diff --git a/internal/worker/child_test.go b/internal/worker/child_test.go index 55a0757..657b9e6 100644 --- a/internal/worker/child_test.go +++ b/internal/worker/child_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.starlark.net/starlark" "github.com/meigma/codemode/internal/execution" ) @@ -253,6 +254,11 @@ func TestFinalErrorFromExtractsApprovedSafeDetail(t *testing.T) { err: execution.WithSafeDetail(execution.ErrInternal, "hidden"), code: finalErrorInternal, }, + { + name: "capability failure stays child-internal", + err: execution.WithSafeDetail(execution.ErrCapabilityFailure, `instance "web" not found`), + code: finalErrorInternal, + }, } for _, tt := range tests { @@ -333,7 +339,109 @@ func TestServeEngineAbortSuppressesFinalError(t *testing.T) { require.NoError(t, err) _, ok := frame.(nativeCallFrame) require.True(t, ok) - require.NoError(t, parent.writeNativeAbort()) + require.NoError(t, parent.writeNativeAbort("")) + require.NoError(t, <-done) +} + +// TestNativeForwarderAbortClassification proves approved suffixes are Starlark-visible +// and empty aborts stay the private sentinel. +func TestNativeForwarderAbortClassification(t *testing.T) { + const detail = `instance "web" not found in sandbox "demo"` + tests := []struct { + // name identifies the abort payload. + name string + + // detail is written on the parent abort frame. + detail string + + // want is the classified unwind error. + want error + + // text is the Starlark-visible error string. + text string + + // suffix is the approved SafeDetail, if any. + suffix string + }{ + { + name: "approved suffix is Starlark-visible", + detail: detail, + want: execution.ErrCapabilityFailure, + text: execution.ErrCapabilityFailure.Error() + ": " + detail, + suffix: detail, + }, + { + name: "empty abort stays private sentinel", + want: errNativeAbort, + text: errNativeAbort.Error(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exec := validExecFrame() + parent, child, closePipes := newExecPair(t, exec) + defer closePipes() + handshake := make(chan error, 1) + go func() { handshake <- parent.writeExec(exec) }() + _, err := child.read() + require.NoError(t, err) + require.NoError(t, <-handshake) + + done := make(chan error, 1) + go func() { + builtin := starlark.NewBuiltin( + "lookup", + func(*starlark.Thread, *starlark.Builtin, starlark.Tuple, []starlark.Tuple) (starlark.Value, error) { + _, nativeErr := nativeForwarder(child)("cap.lookup", map[string]any{"org": "meigma"}) + return nil, nativeErr + }, + ) + _, callErr := starlark.Call(&starlark.Thread{Name: "codemode"}, builtin, nil, nil) + done <- callErr + }() + + frame, err := parent.read() + require.NoError(t, err) + _, ok := frame.(nativeCallFrame) + require.True(t, ok) + require.NoError(t, parent.writeNativeAbort(tt.detail)) + + err = <-done + require.ErrorIs(t, err, tt.want) + assert.Equal(t, tt.text, err.Error()) + var evalErr *starlark.EvalError + require.ErrorAs(t, err, &evalErr) + assert.Equal(t, tt.text, evalErr.Msg) + got, ok := execution.SafeDetail(err) + if tt.suffix == "" { + assert.False(t, ok) + require.NotErrorIs(t, err, execution.ErrCapabilityFailure) + return + } + require.True(t, ok) + assert.Equal(t, tt.suffix, got) + require.NotErrorIs(t, err, errNativeAbort) + assert.NotContains(t, err.Error(), "db password") + }) + } +} + +// TestServeEngineAbortWithDetailSuppressesFinalError proves a detailed native_abort +// still exits without a child-owned terminal frame. +func TestServeEngineAbortWithDetailSuppressesFinalError(t *testing.T) { + exec := validExecFrame() + exec.Source = "def main():\n return records.lookup(org=\"acme\")\n" + + parent, done, closePipes := startServeExec(t, exec) + defer closePipes() + require.NoError(t, parent.writeExec(exec)) + + frame, err := parent.read() + require.NoError(t, err) + _, ok := frame.(nativeCallFrame) + require.True(t, ok) + require.NoError(t, parent.writeNativeAbort(`instance "web" not found`)) require.NoError(t, <-done) } diff --git a/internal/worker/decode.go b/internal/worker/decode.go index b609244..e85fba5 100644 --- a/internal/worker/decode.go +++ b/internal/worker/decode.go @@ -76,6 +76,15 @@ type rawFinalErrorFrame struct { Detail json.RawMessage `json:"detail"` } +// rawNativeAbortFrame is the raw native_abort object used to distinguish absent detail. +type rawNativeAbortFrame struct { + // Type is the frame discriminator. + Type string `json:"type"` + + // Detail is the optional approved capability-failure suffix token. + Detail json.RawMessage `json:"detail"` +} + // decodeType reads only the type discriminator without rejecting unknown fields. func decodeType(payload []byte) (string, error) { if len(payload) == 0 { @@ -220,6 +229,34 @@ func decodeFinalErrorDetail(code finalErrorCode, raw json.RawMessage) (string, e return detail, nil } +// decodeNativeAbort decodes one parent-owned abort, with optional approved detail. +func decodeNativeAbort(payload []byte) (nativeAbortFrame, error) { + var raw rawNativeAbortFrame + if err := decodeStrict(payload, &raw); err != nil { + return nativeAbortFrame{}, err + } + if raw.Type != frameTypeNativeAbort { + return nativeAbortFrame{}, errMalformedJSON + } + detail, err := decodeAbortDetail(raw.Detail) + if err != nil { + return nativeAbortFrame{}, err + } + return nativeAbortFrame{Type: frameTypeNativeAbort, Detail: detail}, nil +} + +// decodeAbortDetail accepts only a legal non-empty printable in-budget suffix. +func decodeAbortDetail(raw json.RawMessage) (string, error) { + if raw == nil { + return "", nil + } + var detail string + if err := json.Unmarshal(raw, &detail); err != nil || !validAbortDetail(detail) { + return "", errInvalidValue + } + return detail, nil +} + // requireProtocolVersion rejects a missing or mismatched protocol version. func requireProtocolVersion(version *int) error { const mismatch = "%w: protocol version mismatch: child reported %s, parent requires %d" diff --git a/internal/worker/diagnostic_test.go b/internal/worker/diagnostic_test.go new file mode 100644 index 0000000..1e13485 --- /dev/null +++ b/internal/worker/diagnostic_test.go @@ -0,0 +1,344 @@ +package worker + +import ( + "bytes" + "errors" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/meigma/codemode/internal/execution" +) + +// TestEncodeFinalErrorRoundTripsApprovedDetail proves legal suffixes survive encode and decode. +func TestEncodeFinalErrorRoundTripsApprovedDetail(t *testing.T) { + tests := []struct { + // name identifies the approved detail class. + name string + + // code is the child-owned terminal class. + code finalErrorCode + + // detail is the model-derived suffix. + detail string + }{ + { + name: "invalid program", + code: finalErrorInvalidProgram, + detail: ":3:7: got '=', want primary expression", + }, + { + name: "invalid arguments", + code: finalErrorInvalidArguments, + detail: `unknown argument "keu"`, + }, + { + name: "max budget", + code: finalErrorInvalidProgram, + detail: strings.Repeat("x", maxDiagnosticBytes), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload, err := encodeFinalError(tt.code, tt.detail) + require.NoError(t, err) + assert.Contains(t, string(payload), `"detail":`) + + decoded, err := decodePayload(payload) + require.NoError(t, err) + got, ok := decoded.(finalErrorFrame) + require.True(t, ok) + assert.Equal(t, tt.code, got.Code) + assert.Equal(t, tt.detail, got.Detail) + }) + } +} + +// TestDecodeFinalErrorRejectsInvalidDetail proves empty, oversized, and illegal-code detail fail closed. +func TestDecodeFinalErrorRejectsInvalidDetail(t *testing.T) { + tests := []struct { + // name identifies the illegal detail. + name string + + // payload is the unframed JSON object. + payload string + }{ + { + name: "empty detail", + payload: `{"type":"final_error","code":"invalid_program","detail":""}`, + }, + { + name: "null detail", + payload: `{"type":"final_error","code":"invalid_program","detail":null}`, + }, + { + name: "oversized detail", + payload: `{"type":"final_error","code":"invalid_program","detail":"` + strings.Repeat( + "a", + maxDiagnosticBytes+1, + ) + `"}`, + }, + { + name: "illegal resource_limit detail", + payload: `{"type":"final_error","code":"resource_limit","detail":"hidden"}`, + }, + { + name: "illegal internal detail", + payload: `{"type":"final_error","code":"internal","detail":"hidden"}`, + }, + { + name: "null detail on illegal code", + payload: `{"type":"final_error","code":"internal","detail":null}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := decodePayload([]byte(tt.payload)) + + require.Error(t, err) + require.ErrorIs(t, err, errInvalidValue) + }) + } +} + +// TestEncodeNativeAbortRoundTripsApprovedDetail proves legal suffixes survive encode and decode. +func TestEncodeNativeAbortRoundTripsApprovedDetail(t *testing.T) { + tests := []struct { + // name identifies the approved abort suffix. + name string + + // detail is the parent-approved capability-failure suffix. + detail string + }{ + { + name: "quoted resource name", + detail: `instance "web" not found in sandbox "demo"`, + }, + { + name: "max budget", + detail: strings.Repeat("x", execution.MaxAgentErrorBytes), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload, err := encodeNativeAbort(tt.detail) + require.NoError(t, err) + assert.Contains(t, string(payload), `"detail":`) + assert.NotContains(t, string(payload), "db password") + + decoded, err := decodePayload(payload) + require.NoError(t, err) + got, ok := decoded.(nativeAbortFrame) + require.True(t, ok) + assert.Equal(t, tt.detail, got.Detail) + }) + } +} + +// TestEncodeNativeAbortDropsIllegalDetail proves empty and illegal suffixes stay payload-free. +func TestEncodeNativeAbortDropsIllegalDetail(t *testing.T) { + tests := []struct { + // name identifies the omitted detail. + name string + + // detail is rejected before encode. + detail string + }{ + {name: "empty", detail: ""}, + {name: "newline", detail: "a\nb"}, + {name: "control", detail: "a\x01b"}, + {name: "oversized", detail: strings.Repeat("a", execution.MaxAgentErrorBytes+1)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload, err := encodeNativeAbort(tt.detail) + require.NoError(t, err) + assert.JSONEq(t, nativeAbortPayload, string(payload)) + }) + } +} + +// TestDecodeNativeAbortRejectsInvalidDetail proves malformed abort suffixes fail closed. +func TestDecodeNativeAbortRejectsInvalidDetail(t *testing.T) { + tests := []struct { + // name identifies the illegal abort payload. + name string + + // payload is the unframed JSON object. + payload string + + // target is the expected protocol error. + target error + }{ + { + name: "empty detail", + payload: `{"type":"native_abort","detail":""}`, + target: errInvalidValue, + }, + { + name: "null detail", + payload: `{"type":"native_abort","detail":null}`, + target: errInvalidValue, + }, + { + name: "non-string detail", + payload: `{"type":"native_abort","detail":1}`, + target: errInvalidValue, + }, + { + name: "newline detail", + payload: `{"type":"native_abort","detail":"a\u000ab"}`, + target: errInvalidValue, + }, + { + name: "control detail", + payload: `{"type":"native_abort","detail":"a\u0001b"}`, + target: errInvalidValue, + }, + { + name: "oversized detail", + payload: `{"type":"native_abort","detail":"` + strings.Repeat( + "a", + execution.MaxAgentErrorBytes+1, + ) + `"}`, + target: errInvalidValue, + }, + { + name: "unknown field", + payload: `{"type":"native_abort","detail":"x","cause":"db password rejected"}`, + target: errUnknownField, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := decodePayload([]byte(tt.payload)) + + require.Error(t, err) + require.ErrorIs(t, err, tt.target) + }) + } +} + +// TestEncodeFinalErrorFallsBackToCodeOnly proves illegal detail is dropped before write. +func TestEncodeFinalErrorFallsBackToCodeOnly(t *testing.T) { + tests := []struct { + // name identifies the omitted detail. + name string + + // code is the child-owned terminal class. + code finalErrorCode + + // detail is the suffix that must not be written. + detail string + }{ + {name: "absent detail", code: finalErrorInvalidProgram, detail: ""}, + {name: "oversized detail", code: finalErrorInvalidProgram, detail: strings.Repeat("a", maxDiagnosticBytes+1)}, + {name: "illegal code detail", code: finalErrorResourceLimit, detail: "hidden"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload, err := encodeFinalError(tt.code, tt.detail) + require.NoError(t, err) + assert.NotContains(t, string(payload), `"detail"`) + + decoded, err := decodePayload(payload) + require.NoError(t, err) + got, ok := decoded.(finalErrorFrame) + require.True(t, ok) + assert.Equal(t, tt.code, got.Code) + assert.Empty(t, got.Detail) + }) + } +} + +// TestWriteAbortAttachesOnlyApprovedCapabilityDetail proves the abort frame +// carries SafeDetail only for classified capability failures and never raw causes. +func TestWriteAbortAttachesOnlyApprovedCapabilityDetail(t *testing.T) { + const approved = `instance "web" not found in sandbox "demo"` + tests := []struct { + // name identifies the retained parent error. + name string + + // err is retained by writeAbort. + err error + + // detail is the approved suffix expected on the wire. + detail string + }{ + { + name: "approved capability suffix", + err: execution.WithSafeDetail(execution.ErrCapabilityFailure, approved), + detail: approved, + }, + { + name: "wrapped approved suffix omits cause", + err: fmt.Errorf("lookup: %w", execution.WithSafeDetail(execution.ErrCapabilityFailure, "x")), + detail: "x", + }, + { + name: "ordinary capability failure stays bare", + err: execution.ErrCapabilityFailure, + }, + { + name: "permission denied stays bare", + err: execution.ErrPermissionDenied, + }, + { + name: "permission denied hides attached detail", + err: execution.WithSafeDetail(execution.ErrPermissionDenied, "secret"), + }, + { + name: "resource limit stays bare", + err: execution.ErrResourceLimit, + }, + { + name: "raw wrapper text does not traverse", + err: fmt.Errorf("%w: %w", execution.ErrCapabilityFailure, errors.New("db password rejected")), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exec := validExecFrame() + parent, child := newBufferedExecPair(t, exec) + require.NoError(t, parent.writeExec(exec)) + _, err := child.read() + require.NoError(t, err) + require.NoError(t, child.writeNativeCall("cap.lookup", map[string]any{"org": "meigma"})) + _, err = parent.read() + require.NoError(t, err) + + written, ok := parent.w.(*bytes.Buffer) + require.True(t, ok) + written.Reset() + + out := writeAbort(parent, tt.err) + require.Equal(t, tt.err, out.retained) + + payload, err := readFrame(written, parent.writeCap) + require.NoError(t, err) + assert.NotContains(t, string(payload), "db password") + assert.NotContains(t, string(payload), "secret") + if tt.detail == "" { + assert.JSONEq(t, nativeAbortPayload, string(payload)) + assert.NotContains(t, string(payload), `"detail"`) + return + } + decoded, err := decodePayload(payload) + require.NoError(t, err) + frame, ok := decoded.(nativeAbortFrame) + require.True(t, ok) + assert.Equal(t, tt.detail, frame.Detail) + assert.NotContains(t, string(payload), "lookup:") + }) + } +} diff --git a/internal/worker/frame.go b/internal/worker/frame.go index fddba25..6d56995 100644 --- a/internal/worker/frame.go +++ b/internal/worker/frame.go @@ -6,7 +6,10 @@ import ( "encoding/json" "errors" "io" + "unicode" "unicode/utf8" + + "github.com/meigma/codemode/internal/execution" ) const protocolVersion = 1 @@ -79,6 +82,9 @@ const ( finalErrorSuffix = `}` nativeResultPrefix = `{"type":"native_result","result":` nativeResultSuffix = `}` + nativeAbortPrefix = `{"type":"native_abort"` + nativeAbortDetail = `,"detail":` + nativeAbortSuffix = `}` nativeAbortPayload = `{"type":"native_abort"}` emptyJSONString = `""` ) @@ -144,6 +150,9 @@ type nativeResultFrame struct { type nativeAbortFrame struct { // Type is the frame discriminator. Type string `json:"type"` + + // Detail is an optional approved capability-failure suffix. + Detail string `json:"detail,omitempty"` } // finalFrame is the child's successful terminal result. @@ -375,9 +384,18 @@ func encodeNativeResultBytes(encoded []byte) []byte { return buf.Bytes() } -// encodeNativeAbort encodes the payload-free parent abort frame. -func encodeNativeAbort() ([]byte, error) { - return []byte(nativeAbortPayload), nil +// encodeNativeAbort encodes the parent abort frame. +// +// Empty or illegal detail yields the payload-free form. +func encodeNativeAbort(detail string) ([]byte, error) { + detail = sanitizedAbortDetail(detail) + if detail == "" { + return []byte(nativeAbortPayload), nil + } + return marshalFrame(nativeAbortFrame{ + Type: frameTypeNativeAbort, + Detail: detail, + }) } // encodeFinal encodes one successful terminal child result. @@ -421,6 +439,27 @@ func sanitizedFinalErrorDetail(code finalErrorCode, detail string) string { return detail } +// sanitizedAbortDetail keeps only a legal non-empty in-budget printable suffix. +func sanitizedAbortDetail(detail string) string { + if !validAbortDetail(detail) { + return "" + } + return detail +} + +// validAbortDetail reports whether detail is a non-empty printable UTF-8 suffix within budget. +func validAbortDetail(detail string) bool { + if detail == "" || len(detail) > execution.MaxAgentErrorBytes || !utf8.ValidString(detail) { + return false + } + for _, char := range detail { + if !unicode.IsPrint(char) { + return false + } + } + return true +} + // allowsFinalErrorDetail reports whether code may carry a model-derived suffix. func allowsFinalErrorDetail(code finalErrorCode) bool { switch code { @@ -466,10 +505,7 @@ func decodePayload(payload []byte) (any, error) { } return nativeResultFrame{Type: frameTypeNativeResult, Result: result}, nil case frameTypeNativeAbort: - if err := decodeStrict(payload, &nativeAbortFrame{}); err != nil { - return nil, err - } - return nativeAbortFrame{Type: frameTypeNativeAbort}, nil + return decodeNativeAbort(payload) case frameTypeFinal: result, err := decodeResult(payload, frameTypeFinal) if err != nil { @@ -597,11 +633,11 @@ func (c *parentConn) writeNativeResult(result any) error { } // writeNativeAbort writes the terminal parent-owned native failure. -func (c *parentConn) writeNativeAbort() error { +func (c *parentConn) writeNativeAbort(detail string) error { if c == nil || c.kind != connKindExec || c.state != stateAwaitNative { return errIllegalState } - payload, err := encodeNativeAbort() + payload, err := encodeNativeAbort(detail) if err != nil { return err } diff --git a/internal/worker/frame_test.go b/internal/worker/frame_test.go index f806054..d3a3ed8 100644 --- a/internal/worker/frame_test.go +++ b/internal/worker/frame_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/meigma/codemode/internal/binding" + "github.com/meigma/codemode/internal/execution" ) const ( @@ -207,7 +208,11 @@ func TestFrameEncodersProduceCompactDiscriminators(t *testing.T) { encode: func() ([]byte, error) { return encodeNativeResult(nil) }, typ: frameTypeNativeResult, }, - {name: "native_abort", encode: encodeNativeAbort, typ: frameTypeNativeAbort}, + { + name: "native_abort", + encode: func() ([]byte, error) { return encodeNativeAbort("") }, + typ: frameTypeNativeAbort, + }, {name: "final", encode: func() ([]byte, error) { return encodeFinal("ok") }, typ: frameTypeFinal}, { name: "final_error", @@ -230,132 +235,6 @@ func TestFrameEncodersProduceCompactDiscriminators(t *testing.T) { } } -// TestEncodeFinalErrorRoundTripsApprovedDetail proves legal suffixes survive encode and decode. -func TestEncodeFinalErrorRoundTripsApprovedDetail(t *testing.T) { - tests := []struct { - // name identifies the approved detail class. - name string - - // code is the child-owned terminal class. - code finalErrorCode - - // detail is the model-derived suffix. - detail string - }{ - { - name: "invalid program", - code: finalErrorInvalidProgram, - detail: ":3:7: got '=', want primary expression", - }, - { - name: "invalid arguments", - code: finalErrorInvalidArguments, - detail: `unknown argument "keu"`, - }, - { - name: "max budget", - code: finalErrorInvalidProgram, - detail: strings.Repeat("x", maxDiagnosticBytes), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - payload, err := encodeFinalError(tt.code, tt.detail) - require.NoError(t, err) - assert.Contains(t, string(payload), `"detail":`) - - decoded, err := decodePayload(payload) - require.NoError(t, err) - got, ok := decoded.(finalErrorFrame) - require.True(t, ok) - assert.Equal(t, tt.code, got.Code) - assert.Equal(t, tt.detail, got.Detail) - }) - } -} - -// TestDecodeFinalErrorRejectsInvalidDetail proves empty, oversized, and illegal-code detail fail closed. -func TestDecodeFinalErrorRejectsInvalidDetail(t *testing.T) { - tests := []struct { - // name identifies the illegal detail. - name string - - // payload is the unframed JSON object. - payload string - }{ - { - name: "empty detail", - payload: `{"type":"final_error","code":"invalid_program","detail":""}`, - }, - { - name: "null detail", - payload: `{"type":"final_error","code":"invalid_program","detail":null}`, - }, - { - name: "oversized detail", - payload: `{"type":"final_error","code":"invalid_program","detail":"` + strings.Repeat( - "a", - maxDiagnosticBytes+1, - ) + `"}`, - }, - { - name: "illegal resource_limit detail", - payload: `{"type":"final_error","code":"resource_limit","detail":"hidden"}`, - }, - { - name: "illegal internal detail", - payload: `{"type":"final_error","code":"internal","detail":"hidden"}`, - }, - { - name: "null detail on illegal code", - payload: `{"type":"final_error","code":"internal","detail":null}`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := decodePayload([]byte(tt.payload)) - - require.Error(t, err) - require.ErrorIs(t, err, errInvalidValue) - }) - } -} - -// TestEncodeFinalErrorFallsBackToCodeOnly proves illegal detail is dropped before write. -func TestEncodeFinalErrorFallsBackToCodeOnly(t *testing.T) { - tests := []struct { - // name identifies the omitted detail. - name string - - // code is the child-owned terminal class. - code finalErrorCode - - // detail is the suffix that must not be written. - detail string - }{ - {name: "absent detail", code: finalErrorInvalidProgram, detail: ""}, - {name: "oversized detail", code: finalErrorInvalidProgram, detail: strings.Repeat("a", maxDiagnosticBytes+1)}, - {name: "illegal code detail", code: finalErrorResourceLimit, detail: "hidden"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - payload, err := encodeFinalError(tt.code, tt.detail) - require.NoError(t, err) - assert.NotContains(t, string(payload), `"detail"`) - - decoded, err := decodePayload(payload) - require.NoError(t, err) - got, ok := decoded.(finalErrorFrame) - require.True(t, ok) - assert.Equal(t, tt.code, got.Code) - assert.Empty(t, got.Detail) - }) - } -} - func TestProtocolProbeSucceeds(t *testing.T) { parent, child, closePipes := newProbePair(t) @@ -518,7 +397,7 @@ func TestProtocolTerminalFinalErrorAndAbort(t *testing.T) { require.NoError(t, parent.writeExec(validExecFrame())) _, err := parent.read() require.NoError(t, err) - require.NoError(t, parent.writeNativeAbort()) + require.NoError(t, parent.writeNativeAbort("")) wg.Wait() }) } @@ -582,7 +461,7 @@ func TestProtocolRejectsIllegalStateTransitions(t *testing.T) { require.NoError(t, child.writeNativeCall("cap.lookup", map[string]any{"org": "meigma"})) _, err = parent.read() require.NoError(t, err) - require.NoError(t, parent.writeNativeAbort()) + require.NoError(t, parent.writeNativeAbort("")) _, err = child.read() require.NoError(t, err) err = child.writeFinalError(finalErrorInternal, "") @@ -657,13 +536,14 @@ func TestProtocolChildUsesExecCapThenParentCap(t *testing.T) { // native_call, whose payload sits strictly above parentCap and at or below childCap. func TestProtocolParentReadsChildNativeCallAboveParentCap(t *testing.T) { exec := validExecFrame() + exec.Limits.MaxValueBytes = 4096 parentCap, err := parentPayloadCap(exec.Limits.MaxValueBytes) require.NoError(t, err) childCap, err := childPayloadCap(exec.Limits.MaxValueBytes, exec.Manifest) require.NoError(t, err) require.Greater(t, childCap, parentCap) - arguments := map[string]any{"org": strings.Repeat("a", 217)} + arguments := map[string]any{"org": strings.Repeat("a", exec.Limits.MaxValueBytes-10)} payload, err := encodeNativeCall("cap.lookup", arguments) require.NoError(t, err) require.Greater(t, uint32(len(payload)), parentCap) @@ -810,7 +690,7 @@ func TestFrameLimitsCheckedCaps(t *testing.T) { execCap, err := execPayloadCap(32, manifest) require.NoError(t, err) - abort, err := encodeNativeAbort() + abort, err := encodeNativeAbort("") require.NoError(t, err) finalError, err := encodeFinalError(finalErrorInvalidArguments, "") require.NoError(t, err) @@ -820,6 +700,13 @@ func TestFrameLimitsCheckedCaps(t *testing.T) { assert.Greater(t, parentCap, uint32(maxValueBytes)) assert.Greater(t, execCap, uint32(len(manifestJSON(t, manifest)))) + escapedAbort, err := encodeNativeAbort(strings.Repeat("\"", execution.MaxAgentErrorBytes)) + require.NoError(t, err) + assert.GreaterOrEqual(t, parentCap, uint32(len(escapedAbort))) + wantAbort, err := nativeAbortPayloadCap() + require.NoError(t, err) + assert.Equal(t, wantAbort, parentCap) + escapedDetail, err := encodeFinalError(finalErrorInvalidArguments, strings.Repeat("\"", maxDiagnosticBytes)) require.NoError(t, err) assert.GreaterOrEqual(t, childCap, uint32(len(escapedDetail))) diff --git a/internal/worker/limits.go b/internal/worker/limits.go index 243ee0e..55cf45d 100644 --- a/internal/worker/limits.go +++ b/internal/worker/limits.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/meigma/codemode/internal/binding" + "github.com/meigma/codemode/internal/execution" "github.com/meigma/codemode/internal/universe" ) @@ -102,7 +103,26 @@ func parentPayloadCap(maxValueBytes int) (uint32, error) { if err != nil { return 0, err } - return maxUint32(nativeResult, uint32(len(nativeAbortPayload))), nil + abort, err := nativeAbortPayloadCap() + if err != nil { + return 0, err + } + return maxUint32(nativeResult, abort), nil +} + +// nativeAbortPayloadCap is the largest legal detailed native_abort payload. +func nativeAbortPayloadCap() (uint32, error) { + escaped, err := mulConfigUint32(jsonStringEscapeMax, uint32(execution.MaxAgentErrorBytes)) + if err != nil { + return 0, err + } + return addConfigUint32( + uint32(len(nativeAbortPrefix)), + uint32(len(nativeAbortDetail)), + uint32(len(emptyJSONString)), + escaped, + uint32(len(nativeAbortSuffix)), + ) } // execPayloadCap is the largest legal initial exec payload. diff --git a/internal/worker/parent.go b/internal/worker/parent.go index 2b4fcb2..ecd6ad6 100644 --- a/internal/worker/parent.go +++ b/internal/worker/parent.go @@ -757,11 +757,23 @@ func (r *Runner) handleNative( // writeAbort writes native_abort, requires protocol EOF, and retains parentErr. func writeAbort(conn *parentConn, parentErr error) execOutcome { - _ = conn.writeNativeAbort() + _ = conn.writeNativeAbort(approvedAbortDetail(parentErr)) _ = readExecEOF(conn.r) return execOutcome{retained: parentErr} } +// approvedAbortDetail returns SafeDetail only for a classified capability failure. +func approvedAbortDetail(err error) string { + if !errors.Is(err, execution.ErrCapabilityFailure) { + return "" + } + detail, ok := execution.SafeDetail(err) + if !ok { + return "" + } + return sanitizedAbortDetail(detail) +} + // readExecEOF requires a terminal execution stream to close without trailing bytes. func readExecEOF(r io.Reader) error { var extra [1]byte diff --git a/mcpserver/agent_error_test.go b/mcpserver/agent_error_test.go new file mode 100644 index 0000000..72bc49b --- /dev/null +++ b/mcpserver/agent_error_test.go @@ -0,0 +1,329 @@ +package mcpserver_test + +import ( + "context" + "errors" + "fmt" + "math" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/meigma/codemode" + "github.com/meigma/codemode/authz" + authzmocks "github.com/meigma/codemode/authz/mocks" + "github.com/meigma/codemode/mcpserver" +) + +const ( + // failProgram invokes the on-demand failure capability. + failProgram = ` +def main(): + return records.fail() +` + + // scoreProgram invokes the non-finite conversion capability. + scoreProgram = ` +def main(): + return records.score() +` + + // agentErrorByteCap is the contracted AgentError suffix bound, including a trailing "...". + agentErrorByteCap = 256 + + // namedResourceDetail is a handler-authored named-resource failure. + namedResourceDetail = `instance "web" not found in sandbox "demo"` + + // wrappedAgentDetail is the only text a wrapped AgentError may expose. + wrappedAgentDetail = "x" + + // panicAgentDetail must not cross MCP after a recovered handler panic. + panicAgentDetail = `panic-instance "web" not found` + + // policyAgentDetail must not cross MCP after a policy AgentError. + policyAgentDetail = `policy-instance "web" not found` + + // fourByteRune is a printable 4-byte UTF-8 scalar used at the truncation boundary. + fourByteRune = "\U00010000" + + // hiddenFourByteTail is text after a 4-byte rune that truncation must drop. + hiddenFourByteTail = "hidden-4byte-tail" + + // hiddenSplitTail is text after a rune that would be split at byte 253. + hiddenSplitTail = "hidden-split-tail" + + // hiddenTenKiBTail is text at the end of an oversized message that truncation must drop. + hiddenTenKiBTail = "hidden-10kib-tail" +) + +// nanScoreOutput is a handler result that conversion rejects as non-finite. +type nanScoreOutput struct { + // Score is a floating-point field that may be NaN. + Score float64 `json:"score"` +} + +// TestActualMCPAgentErrorDetails proves MCP execute surfaces sanitized handler-authored +// AgentError suffixes and keeps ordinary, panic, policy, and conversion failures hidden. +func TestActualMCPAgentErrorDetails(t *testing.T) { + ascii253 := strings.Repeat("A", 253) + fourByteMessage := ascii253 + fourByteRune + hiddenFourByteTail + fourByteWant := "capability failed: " + ascii253 + "..." + + ascii251 := strings.Repeat("B", 251) + splitMessage := ascii251 + fourByteRune + hiddenSplitTail + splitWant := "capability failed: " + ascii251 + "..." + + tenKiBHead := "instance-web" + tenKiBControls := "\n\r\t\x00\x1f" + tenKiBMessage := tenKiBHead + tenKiBControls + strings.Repeat("x", 10*1024) + hiddenTenKiBTail + tenKiBSanitized := tenKiBHead + strings.Repeat( + " ", + len(tenKiBControls), + ) + strings.Repeat( + "x", + 10*1024, + ) + hiddenTenKiBTail + tenKiBWant := "capability failed: " + tenKiBSanitized[:agentErrorByteCap-len("...")] + "..." + + tests := []struct { + // name identifies the MCP execute failure. + name string + + // policyError is the authorizer failure. Nil selects AllowAll. + policyError error + + // register installs the capability under test. + register func(*codemode.Builder) + + // source is the execute program. + source string + + // want is the exact MCP tool error text. + want string + + // forbidden must not appear in the MCP payload. + forbidden []string + }{ + { + name: "direct named-resource AgentError", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{Message: namedResourceDetail} + }), + source: failProgram, + want: "capability failed: " + namedResourceDetail, + }, + { + name: "wrapped AgentError hides wrapper secret", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, fmt.Errorf( + "lookup: %s: %w", + handlerPasswordCanary, + &codemode.AgentError{Message: wrappedAgentDetail}, + ) + }), + source: failProgram, + want: "capability failed: " + wrappedAgentDetail, + forbidden: []string{handlerPasswordCanary, "lookup:"}, + }, + { + name: "ordinary error stays bare", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, errors.New(handlerPasswordCanary) + }), + source: failProgram, + want: codemode.ErrCapabilityFailure.Error(), + forbidden: []string{handlerPasswordCanary}, + }, + { + name: "sanitized 10KiB controls and newlines", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{Message: tenKiBMessage} + }), + source: failProgram, + want: tenKiBWant, + forbidden: []string{hiddenTenKiBTail}, + }, + { + name: "truncates before 4-byte rune", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{Message: fourByteMessage} + }), + source: failProgram, + want: fourByteWant, + forbidden: []string{fourByteRune, hiddenFourByteTail}, + }, + { + name: "truncation inside multibyte sequence", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{Message: splitMessage} + }), + source: failProgram, + want: splitWant, + forbidden: []string{fourByteRune, hiddenSplitTail}, + }, + { + name: "invalid UTF-8 becomes replacement rune", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{Message: "ok\xffmore"} + }), + source: failProgram, + want: "capability failed: ok\uFFFDmore", + }, + { + name: "empty AgentError stays bare", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, &codemode.AgentError{} + }), + source: failProgram, + want: codemode.ErrCapabilityFailure.Error(), + }, + { + name: "typed-nil AgentError stays bare", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + var typedNil *codemode.AgentError + return lookupResult{}, typedNil + }), + source: failProgram, + want: codemode.ErrCapabilityFailure.Error(), + }, + { + name: "panic AgentError stays hidden", + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + panic(&codemode.AgentError{Message: panicAgentDetail}) + }), + source: failProgram, + want: codemode.ErrInternal.Error(), + forbidden: []string{panicAgentDetail}, + }, + { + name: "policy AgentError stays hidden", + policyError: fmt.Errorf( + "%s: %w", + handlerPasswordCanary, + &codemode.AgentError{Message: policyAgentDetail}, + ), + register: registerRecordsFail(func(context.Context, authz.Subject, struct{}) (lookupResult, error) { + return lookupResult{}, errors.New(handlerPasswordCanary) + }), + source: failProgram, + want: codemode.ErrPolicyFailure.Error(), + forbidden: []string{ + policyAgentDetail, + handlerPasswordCanary, + namedResourceDetail, + }, + }, + { + name: "conversion failure stays bare", + register: func(builder *codemode.Builder) { + codemode.Register(builder, codemode.Capability[struct{}, nanScoreOutput]{ + ID: "records.entry.score", + Name: "records.score", + Summary: "Return a score.", + Description: "Returns one floating-point score.", + Handler: func(context.Context, authz.Subject, struct{}) (nanScoreOutput, error) { + return nanScoreOutput{Score: math.NaN()}, nil + }, + }) + }, + source: scoreProgram, + want: codemode.ErrCapabilityFailure.Error(), + forbidden: []string{ + "not finite", + "unsupported value", + "NaN", + handlerPasswordCanary, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := executeMCPProgram(t, tt.policyError, tt.register, tt.source) + + assertToolError(t, result, tt.want) + assertNoCanary(t, result) + assertNotContainsText(t, result, tt.forbidden...) + require.LessOrEqual(t, agentErrorDetailBytes(t, result), agentErrorByteCap) + }) + } +} + +// executeMCPProgram builds a real CodeMode server and runs source over in-memory MCP. +func executeMCPProgram( + t *testing.T, + policyError error, + register func(*codemode.Builder), + source string, +) *mcp.CallToolResult { + t.Helper() + var authorizer authz.Authorizer = authz.AllowAll() + if policyError != nil { + policy := authzmocks.NewMockAuthorizer(t) + policy.EXPECT().Authorize(mock.Anything, mock.Anything).Return(policyError).Once() + authorizer = policy + } + builder := codemode.New(codemode.Options{ + Authorizer: authorizer, + Limits: codemode.DefaultLimits(), + }) + register(builder) + root, err := builder.Build() + require.NoError(t, err) + + mcpServer, err := mcpserver.New(root, contextResolver{}, mcpserver.Options{}) + require.NoError(t, err) + + trustedCtx := withInvocationIdentity(t.Context(), invocationIdentity{ + Subject: authz.Subject{ID: trustedSubjectID}, + Canary: credentialCanary, + }) + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := mcpServer.Connect(trustedCtx, serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "codemode-e2e", Version: "test"}, nil) + session, err := client.Connect(t.Context(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + result, err := session.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "execute", + Arguments: map[string]any{"source": source}, + }) + require.NoError(t, err) + return result +} + +// registerRecordsFail installs records.fail with the supplied handler. +func registerRecordsFail( + handler func(context.Context, authz.Subject, struct{}) (lookupResult, error), +) func(*codemode.Builder) { + return func(builder *codemode.Builder) { + codemode.Register(builder, codemode.Capability[struct{}, lookupResult]{ + ID: "records.entry.fail", + Name: "records.fail", + Summary: "Fail on demand.", + Description: "Returns one handler-authored failure.", + Handler: handler, + }) + } +} + +// agentErrorDetailBytes returns the MCP suffix length after "capability failed: ", or 0 when bare. +func agentErrorDetailBytes(t *testing.T, result *mcp.CallToolResult) int { + t.Helper() + require.NotNil(t, result) + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "tool error content must be text") + detail, ok := strings.CutPrefix(text.Text, codemode.ErrCapabilityFailure.Error()+": ") + if !ok { + return 0 + } + return len(detail) +} diff --git a/mcpserver/server.go b/mcpserver/server.go index f4ff185..ee97c13 100644 --- a/mcpserver/server.go +++ b/mcpserver/server.go @@ -221,8 +221,8 @@ func resolveSubject(ctx context.Context, resolver InvocationResolver) (authz.Sub } // projectToolError removes wrapped service detail and maps failures to fixed client-visible sentinels. -// Approved SafeDetail for invalid program and invalid capability arguments is formatted as -// ": ". Arbitrary custom-Service wrapper text remains hidden. +// Approved SafeDetail for program, argument, and capability failures is formatted +// as ": ". Arbitrary custom-Service wrapper text remains hidden. func projectToolError(err error) error { if detail, ok := execution.SafeDetail(err); ok { switch { @@ -230,6 +230,8 @@ func projectToolError(err error) error { return fmt.Errorf("%w: %s", codemode.ErrInvalidProgram, detail) case errors.Is(err, codemode.ErrInvalidArguments): return fmt.Errorf("%w: %s", codemode.ErrInvalidArguments, detail) + case errors.Is(err, codemode.ErrCapabilityFailure): + return fmt.Errorf("%w: %s", codemode.ErrCapabilityFailure, detail) } } switch { diff --git a/server.go b/server.go index 164cd0c..5c9d8d7 100644 --- a/server.go +++ b/server.go @@ -138,7 +138,7 @@ func (server *Server) Execute(ctx context.Context, subject authz.Subject, progra // projectExecutionError removes trusted execution causes at the root boundary. // It preserves only safe sentinels and documented context cancellation and deadline wrapping. -// Contracted SafeDetail on invalid-program and invalid-arguments causes is rewrapped +// Contracted SafeDetail on program, argument, and capability failures is rewrapped // onto the public sentinels; Error remains the coarse sentinel text. func projectExecutionError(err error) error { if detail, ok := execution.SafeDetail(err); ok { @@ -147,6 +147,8 @@ func projectExecutionError(err error) error { return execution.WithSafeDetail(ErrInvalidProgram, detail) case errors.Is(err, execution.ErrInvalidArguments): return execution.WithSafeDetail(ErrInvalidArguments, detail) + case errors.Is(err, execution.ErrCapabilityFailure): + return execution.WithSafeDetail(ErrCapabilityFailure, detail) } } switch { diff --git a/server_test.go b/server_test.go index 9c1e4f2..688a2c8 100644 --- a/server_test.go +++ b/server_test.go @@ -371,6 +371,13 @@ func TestServerExecuteProjectsHandlerFailuresWithoutTrustedDetail(t *testing.T) }, target: codemode.ErrCapabilityFailure, }, + { + name: "agent message keeps root error coarse", + handler: func(context.Context, authz.Subject, builderInput) (builderOutput, error) { + return builderOutput{}, &codemode.AgentError{Message: "instance not found"} + }, + target: codemode.ErrCapabilityFailure, + }, { name: "handler panic", handler: func(context.Context, authz.Subject, builderInput) (builderOutput, error) {