diff --git a/docs/docs/explanation/security-model.md b/docs/docs/explanation/security-model.md index 1b81a62..b7ba099 100644 --- a/docs/docs/explanation/security-model.md +++ b/docs/docs/explanation/security-model.md @@ -88,11 +88,11 @@ Static filtering is useful for deployment-wide availability, but it is not dynam 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`; no other execution cause remains wrapped at the root API. +`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. -The MCP adapter narrows the boundary again. It emits only the fixed error texts in the [MCP tool reference](../reference/mcp-tools.md#errors). 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. 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. -This projection prevents trusted diagnostic detail from becoming model-visible. In particular, MCP responses do not expose budget values, filtered capability identities, unknown requested names, argument names or values, source locations or text, Rego decision paths or rule names, handler messages, credentials, panic values, or stack details. +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`. 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 e1ab1f5..9632f0e 100644 --- a/docs/docs/reference/mcp-tools.md +++ b/docs/docs/reference/mcp-tools.md @@ -237,7 +237,7 @@ There is no compatibility suffix, alias, or alternate signature field. ## `execute` -Execute one Starlark program that defines `def main():` with zero arguments, calls only names confirmed through `search_api` and `describe_api` inside `main`, and returns `main`'s final result. +Execute one Starlark program that defines `def main():` with zero arguments, calls only names confirmed through `search_api` and `describe_api` inside `main`, and returns `main`'s final result. Starlark is not Python: `sum`, `import`, `while`, and f-strings are unavailable; `load` is disabled; `print` is discarded. ### Input @@ -254,7 +254,7 @@ Execute one Starlark program that defines `def main():` with zero arguments, cal } ``` -The source must define `def main():` as a function with zero arguments. Top-level source loading cannot make native calls; calls are accepted only while `main` runs, and only for names confirmed through `search_api` and `describe_api`. Module loading is disabled. +The source must define `def main():` as a function with zero arguments. Starlark is not Python: `sum`, `import`, `while`, and f-strings are unavailable. Top-level source loading cannot make native calls; calls are accepted only while `main` runs, and only for names confirmed through `search_api` and `describe_api`. Module loading is disabled. `print` output is discarded. Capabilities are available by dotted name. The sample native call is `records.lookup(key="alpha", limit=2)`. Native calls accept keyword arguments only. Duplicate keyword syntax is rejected by the Starlark parser as `invalid program` before authorization or handler dispatch. Positional, unknown, missing, incorrectly typed, and out-of-range arguments reach binding and map to `invalid capability arguments`. For the sample, `key` is required and `limit` can be omitted, `None`, or an integer in the signed 64-bit range. @@ -324,12 +324,13 @@ 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 same fixed coarse errors on this page. When recording or reporting a failed call, keep the coarse text and the recovery action; do not echo the failed source, arguments, credentials, or unknown requested name. +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. - Search with a short literal substring over enabled names and summaries. If the result is empty, retry with a shorter term. - After `capability not found`, search again and pass `describe_api` an exact returned `name`, without whitespace or case changes. -- After `invalid capability arguments`, compare the call with the published `signature` and `input` field shapes. -- After `invalid program`, check the program against these requirements: +- 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 `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: `sum`, `import`, `while`, and f-strings are unavailable. - Define `main` with zero arguments. - Call only names confirmed through `search_api` and `describe_api`, and call them only inside `main`. - Return the final value from `main`. @@ -342,14 +343,14 @@ The listed descriptions above are the model-facing contract. Recovery uses the s ## Errors -After a well-formed call reaches the adapter, a resolver or service failure becomes a successful MCP protocol response with `isError` set and one of the eleven fixed text values below. The adapter removes resolver and custom-service details and recovered panic values. It does not expose budget values, filtered capability identities, unknown requested names, argument names or values, source locations or text, Rego decision paths or rule names, handler messages, credentials, panic values, or stack details. +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. | Text | Meaning | | --- | --- | | `unauthenticated` | The resolver failed or returned an empty subject ID. | | `capability not found` | `describe_api` did not find an enabled exact name. | -| `invalid program` | Source, including duplicate keyword syntax, entry point, runtime behavior, or final-value conversion was invalid. | -| `invalid capability arguments` | A native call failed binding: positional, unknown, missing, incorrectly typed, or out-of-range arguments. | +| `invalid program` | Fixed prefix. Source, including duplicate keyword syntax, entry point, runtime behavior, or final-value conversion was invalid. A parse or resolve failure may append `:line:col: message`. | +| `invalid capability arguments` | Fixed prefix. A native call failed binding: positional, unknown, missing, incorrectly typed, or out-of-range arguments. A binding failure may append the model-derived argument diagnostic. | | `permission denied` | Policy returned a recognized denial. | | `authorization policy failure` | Policy evaluation failed. | | `resource limit exceeded` | A discovery, execution, depth, per-value, or aggregate intermediate-value budget was exceeded. | diff --git a/internal/execution/classify_test.go b/internal/execution/classify_test.go new file mode 100644 index 0000000..bfc3b13 --- /dev/null +++ b/internal/execution/classify_test.go @@ -0,0 +1,25 @@ +package execution + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.starlark.net/syntax" +) + +// TestClassifyRuntimeErrorOmitsParserInternalError proves parser bug text stays coarse. +func TestClassifyRuntimeErrorOmitsParserInternalError(t *testing.T) { + filename := "" + err := classifyRuntimeError(&executionState{}, syntax.Error{ + Pos: syntax.MakePosition(&filename, 1, 1), + Msg: "internal error: parser panic", + }) + + require.ErrorIs(t, err, ErrInvalidProgram) + assert.Equal(t, ErrInvalidProgram.Error(), err.Error()) + _, ok := SafeDetail(err) + assert.False(t, ok) + assert.NotContains(t, err.Error(), "internal error:") + assert.NotContains(t, err.Error(), "parser panic") +} diff --git a/internal/execution/detail.go b/internal/execution/detail.go new file mode 100644 index 0000000..0218855 --- /dev/null +++ b/internal/execution/detail.go @@ -0,0 +1,44 @@ +package execution + +import "errors" + +// safeDetailError attaches one model-derived diagnostic suffix without changing the coarse error text. +type safeDetailError struct { + // cause is the coarse classified sentinel. + cause error + + // detail is the model-derived suffix excluded from Error. + detail string +} + +// Error returns only the coarse cause text. +func (err *safeDetailError) Error() string { + return err.cause.Error() +} + +// Unwrap returns the coarse cause. +func (err *safeDetailError) Unwrap() error { + return err.cause +} + +// 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. +func WithSafeDetail(cause error, detail string) error { + if detail == "" { + return cause + } + return &safeDetailError{cause: cause, detail: detail} +} + +// SafeDetail reports the model-derived suffix attached to err, if any. +// +// Extraction follows the error chain with [errors.As]. +func SafeDetail(err error) (string, bool) { + var wrapped *safeDetailError + if !errors.As(err, &wrapped) { + return "", false + } + return wrapped.detail, true +} diff --git a/internal/execution/detail_test.go b/internal/execution/detail_test.go new file mode 100644 index 0000000..a37c84d --- /dev/null +++ b/internal/execution/detail_test.go @@ -0,0 +1,118 @@ +package execution_test + +import ( + "fmt" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/meigma/codemode/internal/execution" +) + +// TestWithSafeDetailPreservesCoarseCause proves the wrapper never changes Error or [errors.Is]. +func TestWithSafeDetailPreservesCoarseCause(t *testing.T) { + t.Run("empty detail returns cause", func(t *testing.T) { + got := execution.WithSafeDetail(execution.ErrInvalidProgram, "") + + assert.Equal(t, execution.ErrInvalidProgram, got) + _, ok := execution.SafeDetail(got) + assert.False(t, ok) + }) + + t.Run("Error stays coarse", func(t *testing.T) { + const detail = ":1:1: got '='" + got := execution.WithSafeDetail(execution.ErrInvalidProgram, detail) + + require.ErrorIs(t, got, execution.ErrInvalidProgram) + assert.Equal(t, execution.ErrInvalidProgram.Error(), got.Error()) + gotDetail, ok := execution.SafeDetail(got) + require.True(t, ok) + assert.Equal(t, detail, gotDetail) + }) + + t.Run("extracts through genuine wrapper", func(t *testing.T) { + const detail = `unknown argument "keu"` + got := fmt.Errorf("worker: %w", execution.WithSafeDetail(execution.ErrInvalidArguments, detail)) + + require.ErrorIs(t, got, execution.ErrInvalidArguments) + gotDetail, ok := execution.SafeDetail(got) + require.True(t, ok) + assert.Equal(t, detail, gotDetail) + }) +} + +// TestExecuteAttachesApprovedProgramDiagnostics proves parse and resolve suffixes stay model-derived. +func TestExecuteAttachesApprovedProgramDiagnostics(t *testing.T) { + engine := buildEngine(t) + tests := []struct { + // name identifies the invalid program. + name string + + // source is the submitted Starlark program. + source string + + // contains is the required model-derived suffix fragment. + contains string + }{ + { + name: "ordinary syntax", + source: "def main():\n return =\n", + contains: ":", + }, + { + name: "undefined sum", + source: "def main():\n return sum([1, 2])\n", + contains: "undefined: sum", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := engine.Execute(tt.source, echoNativeCall(), defaultExecutionLimits()) + + require.ErrorIs(t, err, execution.ErrInvalidProgram) + assert.Equal(t, execution.ErrInvalidProgram.Error(), err.Error()) + detail, ok := execution.SafeDetail(err) + require.True(t, ok) + assert.Contains(t, detail, tt.contains) + assert.Contains(t, detail, ":") + assert.NotContains(t, err.Error(), detail) + }) + } +} + +// TestExecuteAttachesOneBindingDiagnostic proves BindShape suffixes omit the coarse prefix. +func TestExecuteAttachesOneBindingDiagnostic(t *testing.T) { + var nativeCalls atomic.Int64 + _, err := buildEngine(t).Execute( + `def main(): return records.lookup(keu="alpha")`, + countingNativeCall(&nativeCalls), + defaultExecutionLimits(), + ) + + require.ErrorIs(t, err, execution.ErrInvalidArguments) + assert.Equal(t, execution.ErrInvalidArguments.Error(), err.Error()) + detail, ok := execution.SafeDetail(err) + require.True(t, ok) + assert.Equal(t, `unknown argument "keu"`, detail) + assert.NotContains(t, detail, execution.ErrInvalidArguments.Error()) + assert.Zero(t, nativeCalls.Load()) +} + +// TestExecuteKeepsGenericRuntimeErrorsCoarse proves evaluator messages stay hidden. +func TestExecuteKeepsGenericRuntimeErrorsCoarse(t *testing.T) { + _, err := buildEngine(t).Execute( + "def main():\n fail(\"db password rejected\")\n", + echoNativeCall(), + defaultExecutionLimits(), + ) + + require.ErrorIs(t, err, execution.ErrInvalidProgram) + assert.Equal(t, execution.ErrInvalidProgram.Error(), err.Error()) + _, ok := execution.SafeDetail(err) + assert.False(t, ok) + assert.NotContains(t, err.Error(), "db password rejected") + assert.NotContains(t, err.Error(), "fail") +} diff --git a/internal/execution/execute.go b/internal/execution/execute.go index 4ed30d4..377882a 100644 --- a/internal/execution/execute.go +++ b/internal/execution/execute.go @@ -3,7 +3,9 @@ package execution import ( "errors" "fmt" + "strings" + "go.starlark.net/resolve" "go.starlark.net/starlark" "go.starlark.net/syntax" @@ -176,7 +178,7 @@ func callCapability( canonical, bindingErr := binding.BindShape(input, args, kwargs) if bindingErr != nil { if errors.Is(bindingErr, binding.ErrInvalidArguments) { - return nil, fmt.Errorf("%w: %w", ErrInvalidArguments, bindingErr) + return nil, invalidArgumentDetail(bindingErr) } return nil, fmt.Errorf("%w: %w", ErrInternal, bindingErr) } @@ -188,22 +190,69 @@ func classifyRuntimeError(state *executionState, err error) error { if state.stepLimited { return ErrResourceLimit } + if detail, ok := programDetail(err); ok { + return WithSafeDetail(ErrInvalidProgram, detail) + } + cause := unwrapEvalError(err) switch { - case errors.Is(err, ErrInvalidArguments): - return ErrInvalidArguments - case errors.Is(err, ErrPermissionDenied): + case errors.Is(cause, ErrInvalidArguments): + return classifiedSafeDetail(ErrInvalidArguments, cause) + case errors.Is(cause, ErrPermissionDenied): return ErrPermissionDenied - case errors.Is(err, ErrPolicyFailure): + case errors.Is(cause, ErrPolicyFailure): return ErrPolicyFailure - case errors.Is(err, ErrResourceLimit): + case errors.Is(cause, ErrResourceLimit): return ErrResourceLimit - case errors.Is(err, ErrCapabilityFailure): + case errors.Is(cause, ErrCapabilityFailure): return ErrCapabilityFailure - case errors.Is(err, ErrInternal): + case errors.Is(cause, ErrInternal): return ErrInternal - case errors.Is(err, ErrInvalidProgram): - return ErrInvalidProgram + case errors.Is(cause, ErrInvalidProgram): + return classifiedSafeDetail(ErrInvalidProgram, cause) default: - return fmt.Errorf("%w: %w", ErrInvalidProgram, err) + return ErrInvalidProgram + } +} + +// unwrapEvalError returns the evaluator cause without reading EvalError.Msg. +func unwrapEvalError(err error) error { + evalErr, ok := err.(*starlark.EvalError) //nolint:errorlint // Exact type excludes unrelated wrappers. + if !ok { + return err + } + return evalErr.Unwrap() +} + +// classifiedSafeDetail reattaches an approved suffix to sentinel, or returns sentinel unchanged. +func classifiedSafeDetail(sentinel error, err error) error { + detail, ok := SafeDetail(err) + if !ok { + return sentinel + } + return WithSafeDetail(sentinel, detail) +} + +// programDetail reports a model-derived parse or resolve suffix for a direct evaluator error. +func programDetail(err error) (string, bool) { + if syntaxErr, ok := err.(syntax.Error); ok { //nolint:errorlint // Exact type is the provenance boundary. + if strings.HasPrefix(syntaxErr.Msg, "internal error:") { + return "", false + } + return syntaxErr.Error(), true + } + list, ok := err.(resolve.ErrorList) //nolint:errorlint // Exact type is the provenance boundary. + if !ok || len(list) == 0 { + return "", false + } + return list[0].Error(), true +} + +// invalidArgumentDetail attaches the binding suffix after the exact sentinel prefix, or stays coarse. +func invalidArgumentDetail(err error) error { + prefix := binding.ErrInvalidArguments.Error() + ": " + suffix, ok := strings.CutPrefix(err.Error(), prefix) + if !ok { + return ErrInvalidArguments } + return WithSafeDetail(ErrInvalidArguments, suffix) } diff --git a/internal/worker/child.go b/internal/worker/child.go index f5d8c15..dc9b2af 100644 --- a/internal/worker/child.go +++ b/internal/worker/child.go @@ -10,11 +10,9 @@ import ( "github.com/meigma/codemode/internal/execution" ) -// errNativeAbort is the private interpreter-unwind sentinel returned when the -// parent answers a native call with native_abort. -// -// It does not wrap or equal any execution.Err* sentinel, so Engine's default -// classifyRuntimeError branch preserves it through multi-%w wrapping. +// errNativeAbort is the private sentinel used to unwind the interpreter after +// the parent answers a native call with native_abort. The connection state +// remains authoritative because Engine intentionally coarsens unknown errors. var errNativeAbort = errors.New("native abort") // errChildService classifies a child protocol or internal service failure that @@ -95,7 +93,7 @@ func serveExec(r io.Reader, w io.Writer, frame execFrame) error { hasLimits: true, } if len(frame.Source) > frame.Limits.MaxSourceBytes { - return conn.writeFinalError(finalErrorResourceLimit) + return conn.writeFinalError(finalErrorResourceLimit, "") } engine, err := execution.New(manifestBindings(frame.Manifest)) if err != nil { @@ -106,6 +104,9 @@ func serveExec(r io.Reader, w io.Writer, frame execFrame) error { nativeForwarder(conn), executionLimits(frame.Limits), ) + if conn.state == stateDone { + return nil + } if err != nil { return writeExecutionError(conn, err) } @@ -168,14 +169,15 @@ func writeExecutionError(conn *childConn, err error) error { if errors.Is(err, errChildService) { return err } - return conn.writeFinalError(finalErrorFrom(err)) + code, detail := finalErrorFrom(err) + return conn.writeFinalError(code, detail) } // writeFinalResult writes a successful value or classifies a legal value-byte overflow. func writeFinalResult(conn *childConn, result any) error { if err := conn.writeFinal(result); err != nil { if code, ok := finalWriteCode(err); ok { - return conn.writeFinalError(code) + return conn.writeFinalError(code, "") } return err } @@ -195,15 +197,27 @@ func finalWriteCode(err error) (finalErrorCode, bool) { } // finalErrorFrom maps a classified Engine error onto a child-owned terminal code. -func finalErrorFrom(err error) finalErrorCode { +func finalErrorFrom(err error) (finalErrorCode, string) { switch { case errors.Is(err, execution.ErrInvalidProgram): - return finalErrorInvalidProgram + return finalErrorInvalidProgram, approvedSafeDetail(finalErrorInvalidProgram, err) case errors.Is(err, execution.ErrInvalidArguments): - return finalErrorInvalidArguments + return finalErrorInvalidArguments, approvedSafeDetail(finalErrorInvalidArguments, err) case errors.Is(err, execution.ErrResourceLimit): - return finalErrorResourceLimit + return finalErrorResourceLimit, "" default: - return finalErrorInternal + return finalErrorInternal, "" + } +} + +// approvedSafeDetail extracts contracted detail only for the matching approved code. +func approvedSafeDetail(code finalErrorCode, err error) string { + if !allowsFinalErrorDetail(code) { + return "" + } + detail, ok := execution.SafeDetail(err) + if !ok { + return "" } + return sanitizedFinalErrorDetail(code, detail) } diff --git a/internal/worker/child_test.go b/internal/worker/child_test.go index 288ebd1..0723c73 100644 --- a/internal/worker/child_test.go +++ b/internal/worker/child_test.go @@ -4,6 +4,7 @@ import ( "bytes" "io" "math" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -205,6 +206,65 @@ func TestServeFinalAndFinalErrorMapping(t *testing.T) { } } +// TestFinalErrorFromExtractsApprovedSafeDetail proves only matching approved codes carry a suffix. +func TestFinalErrorFromExtractsApprovedSafeDetail(t *testing.T) { + tests := []struct { + // name identifies the mapped execution error. + name string + + // err is the classified Engine failure. + err error + + // code is the expected child-owned terminal class. + code finalErrorCode + + // detail is the suffix written for approved codes. + detail string + }{ + { + name: "invalid program keeps suffix", + err: execution.WithSafeDetail(execution.ErrInvalidProgram, ":1:1: undefined: sum"), + code: finalErrorInvalidProgram, + detail: ":1:1: undefined: sum", + }, + { + name: "invalid arguments keeps suffix", + err: execution.WithSafeDetail(execution.ErrInvalidArguments, `unknown argument "keu"`), + code: finalErrorInvalidArguments, + detail: `unknown argument "keu"`, + }, + { + name: "code-only fallback for empty detail", + err: execution.ErrInvalidProgram, + code: finalErrorInvalidProgram, + }, + { + name: "code-only fallback for oversized detail", + err: execution.WithSafeDetail(execution.ErrInvalidProgram, strings.Repeat("a", maxDiagnosticBytes+1)), + code: finalErrorInvalidProgram, + }, + { + name: "resource limit stays bare", + err: execution.WithSafeDetail(execution.ErrResourceLimit, "hidden"), + code: finalErrorResourceLimit, + }, + { + name: "internal stays bare", + err: execution.WithSafeDetail(execution.ErrInternal, "hidden"), + code: finalErrorInternal, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + code, detail := finalErrorFrom(tt.err) + + assert.Equal(t, tt.code, code) + assert.Equal(t, tt.detail, detail) + }) + } +} + // TestServeFinalWriteClassification proves only a legal value-byte overflow is // resource_limit; unsupported final values stay invalid_program. func TestServeFinalWriteClassification(t *testing.T) { diff --git a/internal/worker/decode.go b/internal/worker/decode.go index 33c2df9..b609244 100644 --- a/internal/worker/decode.go +++ b/internal/worker/decode.go @@ -71,6 +71,9 @@ type rawFinalErrorFrame struct { // Code is the child-owned error class. Code *finalErrorCode `json:"code"` + + // Detail is the optional model-derived suffix token. + Detail json.RawMessage `json:"detail"` } // decodeType reads only the type discriminator without rejecting unknown fields. @@ -197,7 +200,24 @@ func decodeFinalError(payload []byte) (finalErrorFrame, error) { if raw.Code == nil || !knownFinalError(*raw.Code) { return finalErrorFrame{}, errInvalidValue } - return finalErrorFrame{Type: frameTypeFinalError, Code: *raw.Code}, nil + detail, err := decodeFinalErrorDetail(*raw.Code, raw.Detail) + if err != nil { + return finalErrorFrame{}, err + } + return finalErrorFrame{Type: frameTypeFinalError, Code: *raw.Code, Detail: detail}, nil +} + +// decodeFinalErrorDetail accepts only a legal non-empty in-budget suffix. +func decodeFinalErrorDetail(code finalErrorCode, raw json.RawMessage) (string, error) { + if raw == nil { + return "", nil + } + var detail string + if err := json.Unmarshal(raw, &detail); err != nil || + detail == "" || len(detail) > maxDiagnosticBytes || !allowsFinalErrorDetail(code) { + return "", errInvalidValue + } + return detail, nil } // requireProtocolVersion rejects a missing or mismatched protocol version. diff --git a/internal/worker/frame.go b/internal/worker/frame.go index 86ac1d9..fddba25 100644 --- a/internal/worker/frame.go +++ b/internal/worker/frame.go @@ -32,6 +32,9 @@ const ( finalErrorInternal finalErrorCode = "internal" ) +// maxDiagnosticBytes is the private raw-byte bound for final_error detail. +const maxDiagnosticBytes = 4 << 10 + // knownFinalError reports whether code is one child-owned terminal class. func knownFinalError(code finalErrorCode) bool { switch code { @@ -71,7 +74,9 @@ const ( nativeCallSuffix = `}` finalPrefix = `{"type":"final","result":` finalSuffix = `}` - finalErrorLongest = `{"type":"final_error","code":"invalid_arguments"}` + finalErrorPrefix = `{"type":"final_error","code":"invalid_arguments"` + finalErrorDetail = `,"detail":` + finalErrorSuffix = `}` nativeResultPrefix = `{"type":"native_result","result":` nativeResultSuffix = `}` nativeAbortPayload = `{"type":"native_abort"}` @@ -157,6 +162,9 @@ type finalErrorFrame struct { // Code is one child-owned final error class. Code finalErrorCode `json:"code"` + + // Detail is an optional model-derived suffix for approved codes. + Detail string `json:"detail,omitempty"` } type connKind uint8 @@ -392,11 +400,37 @@ func encodeFinalBytes(encoded []byte) []byte { } // encodeFinalError encodes one child-owned terminal failure. -func encodeFinalError(code finalErrorCode) ([]byte, error) { +// +// Absent, oversized, or illegal detail is dropped before any write. +func encodeFinalError(code finalErrorCode, detail string) ([]byte, error) { if !knownFinalError(code) { return nil, errInvalidValue } - return marshalFrame(finalErrorFrame{Type: frameTypeFinalError, Code: code}) + return marshalFrame(finalErrorFrame{ + Type: frameTypeFinalError, + Code: code, + Detail: sanitizedFinalErrorDetail(code, detail), + }) +} + +// sanitizedFinalErrorDetail keeps only a legal non-empty in-budget suffix. +func sanitizedFinalErrorDetail(code finalErrorCode, detail string) string { + if detail == "" || len(detail) > maxDiagnosticBytes || !allowsFinalErrorDetail(code) { + return "" + } + return detail +} + +// allowsFinalErrorDetail reports whether code may carry a model-derived suffix. +func allowsFinalErrorDetail(code finalErrorCode) bool { + switch code { + case finalErrorInvalidProgram, finalErrorInvalidArguments: + return true + case finalErrorResourceLimit, finalErrorInternal: + return false + default: + return false + } } // decodePayload decodes exactly one UTF-8 JSON frame object. @@ -712,11 +746,11 @@ func (c *childConn) writeFinal(result any) error { } // writeFinalError writes one child-owned terminal failure. -func (c *childConn) writeFinalError(code finalErrorCode) error { +func (c *childConn) writeFinalError(code finalErrorCode, detail string) error { if c == nil || c.kind != connKindExec || c.state != stateReady { return errIllegalState } - payload, err := encodeFinalError(code) + payload, err := encodeFinalError(code, detail) if err != nil { return err } diff --git a/internal/worker/frame_test.go b/internal/worker/frame_test.go index 236ec5e..5f86b66 100644 --- a/internal/worker/frame_test.go +++ b/internal/worker/frame_test.go @@ -211,7 +211,7 @@ func TestFrameEncodersProduceCompactDiscriminators(t *testing.T) { {name: "final", encode: func() ([]byte, error) { return encodeFinal("ok") }, typ: frameTypeFinal}, { name: "final_error", - encode: func() ([]byte, error) { return encodeFinalError(finalErrorInvalidProgram) }, + encode: func() ([]byte, error) { return encodeFinalError(finalErrorInvalidProgram, "") }, typ: frameTypeFinalError, }, } @@ -230,6 +230,132 @@ 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) @@ -270,7 +396,7 @@ func TestProtocolAcceptsChildResourceLimit(t *testing.T) { t.Errorf("child.read() error = %v", err) return } - if err := child.writeFinalError(finalErrorResourceLimit); err != nil { + if err := child.writeFinalError(finalErrorResourceLimit, ""); err != nil { t.Errorf("child.writeFinalError() error = %v", err) } }) @@ -351,7 +477,7 @@ func TestProtocolTerminalFinalErrorAndAbort(t *testing.T) { t.Errorf("child.read() error = %v", err) return } - if err := child.writeFinalError(finalErrorInvalidArguments); err != nil { + if err := child.writeFinalError(finalErrorInvalidArguments, ""); err != nil { t.Errorf("child.writeFinalError() error = %v", err) } }) @@ -441,7 +567,7 @@ func TestProtocolRejectsIllegalStateTransitions(t *testing.T) { require.NoError(t, child.writeFinal(nil)) _, err = parent.read() require.NoError(t, err) - err = child.writeFinalError(finalErrorInternal) + err = child.writeFinalError(finalErrorInternal, "") require.Error(t, err) require.ErrorIs(t, err, errIllegalState) }, @@ -459,7 +585,7 @@ func TestProtocolRejectsIllegalStateTransitions(t *testing.T) { require.NoError(t, parent.writeNativeAbort()) _, err = child.read() require.NoError(t, err) - err = child.writeFinalError(finalErrorInternal) + err = child.writeFinalError(finalErrorInternal, "") require.Error(t, err) require.ErrorIs(t, err, errIllegalState) }, @@ -661,7 +787,7 @@ func TestFrameLimitsCheckedCaps(t *testing.T) { abort, err := encodeNativeAbort() require.NoError(t, err) - finalError, err := encodeFinalError(finalErrorInvalidArguments) + finalError, err := encodeFinalError(finalErrorInvalidArguments, "") require.NoError(t, err) assert.GreaterOrEqual(t, childCap, uint32(len(finalError))) assert.GreaterOrEqual(t, parentCap, uint32(len(abort))) @@ -669,15 +795,26 @@ func TestFrameLimitsCheckedCaps(t *testing.T) { assert.Greater(t, parentCap, uint32(maxValueBytes)) assert.Greater(t, execCap, uint32(len(manifestJSON(t, manifest)))) + escapedDetail, err := encodeFinalError(finalErrorInvalidArguments, strings.Repeat("\"", maxDiagnosticBytes)) + require.NoError(t, err) + assert.GreaterOrEqual(t, childCap, uint32(len(escapedDetail))) + wantFinalError, err := finalErrorPayloadCap() + require.NoError(t, err) + assert.Equal(t, wantFinalError, childCap) + + largeValueBytes := maxDiagnosticBytes * jsonStringEscapeMax + baseCap, err := childPayloadCap(largeValueBytes, manifest) + require.NoError(t, err) + longer := []manifestEntry{{ ID: strings.Repeat("a", 40), Name: "records.lookup", Input: manifest[0].Input, }} require.NoError(t, validateManifest(longer)) - longerCap, err := childPayloadCap(maxValueBytes, longer) + longerCap, err := childPayloadCap(largeValueBytes, longer) require.NoError(t, err) - assert.Greater(t, longerCap, childCap) + assert.Greater(t, longerCap, baseCap) escaped := []manifestEntry{{ ID: strings.Repeat("\"", 40), @@ -685,7 +822,7 @@ func TestFrameLimitsCheckedCaps(t *testing.T) { Input: manifest[0].Input, }} require.NoError(t, validateManifest(escaped)) - escapedCap, err := childPayloadCap(maxValueBytes, escaped) + escapedCap, err := childPayloadCap(largeValueBytes, escaped) require.NoError(t, err) assert.Greater(t, escapedCap, longerCap) diff --git a/internal/worker/limits.go b/internal/worker/limits.go index f1d957c..c22fdd5 100644 --- a/internal/worker/limits.go +++ b/internal/worker/limits.go @@ -68,7 +68,26 @@ func childPayloadCap(maxValueBytes int, manifest []manifestEntry) (uint32, error if err != nil { return 0, err } - return maxUint32(nativeCall, final, uint32(len(finalErrorLongest))), nil + finalError, err := finalErrorPayloadCap() + if err != nil { + return 0, err + } + return maxUint32(nativeCall, final, finalError), nil +} + +// finalErrorPayloadCap is the largest legal detailed final_error payload. +func finalErrorPayloadCap() (uint32, error) { + escaped, err := mulConfigUint32(jsonStringEscapeMax, maxDiagnosticBytes) + if err != nil { + return 0, err + } + return addConfigUint32( + uint32(len(finalErrorPrefix)), + uint32(len(finalErrorDetail)), + uint32(len(emptyJSONString)), + escaped, + uint32(len(finalErrorSuffix)), + ) } // parentPayloadCap is the largest legal parent-originated execution payload. diff --git a/internal/worker/parent.go b/internal/worker/parent.go index 2732fa8..2b4fcb2 100644 --- a/internal/worker/parent.go +++ b/internal/worker/parent.go @@ -710,7 +710,7 @@ func (r *Runner) runExecExchange( if err := readExecEOF(stdout); err != nil { return execOutcome{err: execution.ErrInternal, kill: true} } - return execOutcome{err: mapFinalError(typed.Code)} + return execOutcome{err: mapFinalError(typed)} default: return execOutcome{err: execution.ErrInternal, kill: true} } @@ -778,8 +778,17 @@ func readExecEOF(r io.Reader) error { return errIllegalState } -// mapFinalError projects a child-owned terminal code onto an execution sentinel. -func mapFinalError(code finalErrorCode) error { +// mapFinalError projects a child-owned terminal frame onto an execution sentinel. +func mapFinalError(frame finalErrorFrame) error { + cause := mapFinalErrorCode(frame.Code) + if !allowsFinalErrorDetail(frame.Code) { + return cause + } + return execution.WithSafeDetail(cause, sanitizedFinalErrorDetail(frame.Code, frame.Detail)) +} + +// mapFinalErrorCode projects a child-owned terminal code onto an execution sentinel. +func mapFinalErrorCode(code finalErrorCode) error { switch code { case finalErrorInvalidProgram: return execution.ErrInvalidProgram diff --git a/internal/worker/parent_test.go b/internal/worker/parent_test.go index f67389c..c75cfb4 100644 --- a/internal/worker/parent_test.go +++ b/internal/worker/parent_test.go @@ -506,7 +506,7 @@ func TestRunnerFinalErrorTrailingByteKills(t *testing.T) { runner := newTestRunner(t, testLimits(), nopDispatch()) var parentWrites bytes.Buffer var childOut bytes.Buffer - payload, err := encodeFinalError(finalErrorInvalidProgram) + payload, err := encodeFinalError(finalErrorInvalidProgram, "") require.NoError(t, err) require.NoError(t, writeFrame(&childOut, payload, runner.readCap)) require.NoError(t, childOut.WriteByte('x')) @@ -524,6 +524,71 @@ func TestRunnerFinalErrorTrailingByteKills(t *testing.T) { assert.NoError(t, out.retained) } +// TestMapFinalErrorReconstructsApprovedDetail proves only approved codes regain a suffix. +func TestMapFinalErrorReconstructsApprovedDetail(t *testing.T) { + tests := []struct { + // name identifies the mapped terminal frame. + name string + + // frame is the child-owned terminal failure. + frame finalErrorFrame + + // want is the coarse execution sentinel. + want error + + // detail is the reconstructed suffix, if any. + detail string + }{ + { + name: "invalid program reconstructs suffix", + frame: finalErrorFrame{ + Code: finalErrorInvalidProgram, + Detail: ":3:7: got '=', want primary expression", + }, + want: execution.ErrInvalidProgram, + detail: ":3:7: got '=', want primary expression", + }, + { + name: "invalid arguments reconstructs suffix", + frame: finalErrorFrame{Code: finalErrorInvalidArguments, Detail: `unknown argument "keu"`}, + want: execution.ErrInvalidArguments, + detail: `unknown argument "keu"`, + }, + { + name: "code-only invalid program", + frame: finalErrorFrame{Code: finalErrorInvalidProgram}, + want: execution.ErrInvalidProgram, + }, + { + name: "resource limit stays bare", + frame: finalErrorFrame{Code: finalErrorResourceLimit, Detail: "hidden"}, + want: execution.ErrResourceLimit, + }, + { + name: "internal stays bare", + frame: finalErrorFrame{Code: finalErrorInternal, Detail: "hidden"}, + want: execution.ErrInternal, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := mapFinalError(tt.frame) + + require.ErrorIs(t, err, tt.want) + assert.Equal(t, tt.want.Error(), err.Error()) + detail, ok := execution.SafeDetail(err) + if tt.detail == "" { + assert.False(t, ok) + assert.Empty(t, detail) + return + } + require.True(t, ok) + assert.Equal(t, tt.detail, detail) + }) + } +} + // TestRunnerProtocolViolationWritesNoAbort proves ErrProtocol kills without writing native_abort. func TestRunnerProtocolViolationWritesNoAbort(t *testing.T) { runner := newTestRunner( diff --git a/mcpserver/e2e_test.go b/mcpserver/e2e_test.go index 65ac594..e69081c 100644 --- a/mcpserver/e2e_test.go +++ b/mcpserver/e2e_test.go @@ -7,6 +7,7 @@ import ( "fmt" "maps" "strconv" + "strings" "sync" "sync/atomic" "testing" @@ -44,6 +45,9 @@ const ( // allowedLookupLimit is the optional integer argument used by the successful program. allowedLookupLimit int64 = 2 + + // handlerPasswordCanary is host handler text that must not cross MCP. + handlerPasswordCanary = "db password rejected" ) // invocationContextKey is the typed trusted-context key for e2e identity. @@ -628,6 +632,80 @@ def main(): assert.False(t, hasLabel, "omitted optional string must not appear in the canonical map") } +// TestActualMCPModelDerivedDiagnostics proves MCP execute surfaces only approved +// parser, resolver, and binding detail and keeps handler text hidden. +func TestActualMCPModelDerivedDiagnostics(t *testing.T) { + builder := codemode.New(codemode.Options{ + Authorizer: authz.AllowAll(), + Limits: codemode.DefaultLimits(), + }) + codemode.Register(builder, codemode.Capability[lookupInput, lookupResult]{ + ID: "records.entry.lookup", + Name: "records.lookup", + Summary: "Look up one record by key.", + Description: "Returns one deterministic record for the supplied key.", + Handler: func(context.Context, authz.Subject, lookupInput) (lookupResult, error) { + return lookupResult{}, errors.New(handlerPasswordCanary) + }, + }) + root, err := builder.Build() + require.NoError(t, err) + + mcpServer, err := mcpserver.New(root, contextResolver{}) + 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() }) + + syntax, err := session.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "execute", + Arguments: map[string]any{ + "source": "def main():\n return =\n", + }, + }) + require.NoError(t, err) + require.NotNil(t, syntax) + require.True(t, syntax.IsError) + require.Len(t, syntax.Content, 1) + syntaxText, ok := syntax.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.True(t, strings.HasPrefix(syntaxText.Text, "invalid program: :"), syntaxText.Text) + assert.NotEqual(t, codemode.ErrInvalidProgram.Error(), syntaxText.Text) + assertNoCanary(t, syntax) + + binding, err := session.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "execute", + Arguments: map[string]any{ + "source": "def main():\n return records.lookup(keu=\"alpha\")\n", + }, + }) + require.NoError(t, err) + assertToolError(t, binding, `invalid capability arguments: unknown argument "keu"`) + assertNoCanary(t, binding) + + failed, err := session.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "execute", + Arguments: map[string]any{ + "source": "def main():\n return records.lookup(key=\"secret\")\n", + }, + }) + require.NoError(t, err) + assertToolError(t, failed, codemode.ErrCapabilityFailure.Error()) + assertNoCanary(t, failed) + assertNotContainsText(t, failed, handlerPasswordCanary) +} + // withInvocationIdentity stores trusted identity on the server-owned context. func withInvocationIdentity(ctx context.Context, identity invocationIdentity) context.Context { return context.WithValue(ctx, invocationContextKey{}, identity) diff --git a/mcpserver/server.go b/mcpserver/server.go index 15df710..f4c336d 100644 --- a/mcpserver/server.go +++ b/mcpserver/server.go @@ -11,6 +11,7 @@ import ( "github.com/meigma/codemode" "github.com/meigma/codemode/authz" + "github.com/meigma/codemode/internal/execution" ) // searchInput is the exact search_api tool argument object. @@ -123,7 +124,7 @@ func New(service Service, resolver InvocationResolver) (*mcp.Server, error) { }, bound.describe) mcp.AddTool(server, &mcp.Tool{ Name: "execute", - Description: "Execute one Starlark program that defines def main(): with zero arguments, calls only names confirmed through search_api and describe_api inside main, and returns main's final result.", + Description: "Execute one Starlark program that defines def main(): with zero arguments, calls only names confirmed through search_api and describe_api inside main, and returns main's final result. Starlark is not Python: sum, import, while, and f-strings are unavailable; load is disabled; print is discarded.", OutputSchema: executeOutputSchema, }, bound.execute) return server, nil @@ -199,7 +200,17 @@ 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. func projectToolError(err error) error { + if detail, ok := execution.SafeDetail(err); ok { + switch { + case errors.Is(err, codemode.ErrInvalidProgram): + return fmt.Errorf("%w: %s", codemode.ErrInvalidProgram, detail) + case errors.Is(err, codemode.ErrInvalidArguments): + return fmt.Errorf("%w: %s", codemode.ErrInvalidArguments, detail) + } + } switch { case errors.Is(err, codemode.ErrResourceLimit): return codemode.ErrResourceLimit diff --git a/mcpserver/server_test.go b/mcpserver/server_test.go index 49cdfbc..495b2aa 100644 --- a/mcpserver/server_test.go +++ b/mcpserver/server_test.go @@ -16,6 +16,7 @@ import ( "github.com/meigma/codemode" "github.com/meigma/codemode/authz" "github.com/meigma/codemode/authz/rego" + "github.com/meigma/codemode/internal/execution" "github.com/meigma/codemode/mcpserver" "github.com/meigma/codemode/mcpserver/mocks" ) @@ -102,6 +103,13 @@ func TestNewRegistersExactlyThreeTools(t *testing.T) { "inside main", "confirmed through search_api and describe_api", "final result", + "Starlark is not Python", + "sum", + "import", + "while", + "f-strings", + "load is disabled", + "print is discarded", }, assertOutput: requireExecuteOutputSchema, }, @@ -437,6 +445,36 @@ func TestToolsProjectStableServiceErrors(t *testing.T) { }, want: codemode.ErrInvalidArguments.Error(), }, + { + name: "invalid program safe detail", + tool: "execute", + arguments: map[string]any{"source": "broken"}, + configure: func(service *mocks.MockService) { + service.EXPECT(). + Execute(mock.Anything, authz.Subject{ID: "subject-1"}, codemode.Program("broken")). + Return(nil, fmt.Errorf("trusted parse: %w", execution.WithSafeDetail( + codemode.ErrInvalidProgram, + ":1:1: got '=', want primary expression", + ))). + Once() + }, + want: "invalid program: :1:1: got '=', want primary expression", + }, + { + name: "invalid arguments safe detail", + tool: "execute", + arguments: map[string]any{"source": "args"}, + configure: func(service *mocks.MockService) { + service.EXPECT(). + Execute(mock.Anything, authz.Subject{ID: "subject-1"}, codemode.Program("args")). + Return(nil, fmt.Errorf("trusted args: %w", execution.WithSafeDetail( + codemode.ErrInvalidArguments, + "unknown argument \"keu\"", + ))). + Once() + }, + want: "invalid capability arguments: unknown argument \"keu\"", + }, { name: "permission denied", tool: "execute", diff --git a/server.go b/server.go index 70f6fd0..b499668 100644 --- a/server.go +++ b/server.go @@ -135,7 +135,17 @@ 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 +// onto the public sentinels; Error remains the coarse sentinel text. func projectExecutionError(err error) error { + if detail, ok := execution.SafeDetail(err); ok { + switch { + case errors.Is(err, execution.ErrInvalidProgram): + return execution.WithSafeDetail(ErrInvalidProgram, detail) + case errors.Is(err, execution.ErrInvalidArguments): + return execution.WithSafeDetail(ErrInvalidArguments, detail) + } + } switch { case errors.Is(err, execution.ErrInvalidProgram): return ErrInvalidProgram diff --git a/server_test.go b/server_test.go index 975823b..4b31bce 100644 --- a/server_test.go +++ b/server_test.go @@ -171,6 +171,51 @@ def main(): assert.Zero(t, handlerCalls.Load()) } +// TestServerExecuteProjectsSyntaxAndBindingWithoutDetail proves parser and +// binding failures stay coarse at the root API while remaining classified. +func TestServerExecuteProjectsSyntaxAndBindingWithoutDetail(t *testing.T) { + tests := []struct { + // name identifies the model-derived failure. + name string + + // source is the submitted Starlark program. + source string + + // target is the expected public classification. + target error + }{ + { + name: "syntax error", + source: "def main():\n return =\n", + target: codemode.ErrInvalidProgram, + }, + { + name: "unknown argument", + source: "def main():\n return records.lookup(keu=\"alpha\")\n", + target: codemode.ErrInvalidArguments, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + authorizer := authzmocks.NewMockAuthorizer(t) + var handlerCalls atomic.Int64 + capability := validBuilderCapability("cap.lookup", "records.lookup") + capability.Handler = func(context.Context, authz.Subject, builderInput) (builderOutput, error) { + handlerCalls.Add(1) + return builderOutput{}, nil + } + server := buildTestServer(t, authorizer, codemode.DefaultLimits(), capability) + + _, err := server.Execute(t.Context(), authz.Subject{ID: "subject-1"}, codemode.Program(tt.source)) + + require.ErrorIs(t, err, tt.target) + assert.Equal(t, tt.target.Error(), err.Error()) + assert.Zero(t, handlerCalls.Load()) + }) + } +} + // TestServerExecuteFailsClosedBeforeHandlerDispatch proves denial and policy failures have no handler side effects. func TestServerExecuteFailsClosedBeforeHandlerDispatch(t *testing.T) { tests := []struct {