From 82ffcdf1ec322185aa5695518a0ce8780ae33493 Mon Sep 17 00:00:00 2001 From: SaladDay <1203511142@qq.com> Date: Wed, 7 Oct 2026 05:14:37 +0000 Subject: [PATCH] Report corrupt stored data as internal errors --- services/core/internal/api/handler_test.go | 8 ++++++- .../internal/api/session_model_defaults.go | 3 +-- .../internal/environmenttemplates/errors.go | 2 +- .../persistence/postgres/sessionpg/devices.go | 10 ++++---- .../sessionpg/execution_environment.go | 4 ++-- .../persistence/postgres/sessionpg/frozen.go | 4 ++-- .../persistence/postgres/sessionpg/rows.go | 8 +++---- .../persistence/postgres/templatepg/seal.go | 3 ++- .../persistence/postgres/templatepg/store.go | 20 +++++++++++----- .../postgres/templatepg/store_test.go | 24 ++++++++++++------- .../integration/session_reads_fixture_test.go | 2 +- .../integration/turn_journal_reads_test.go | 2 +- 12 files changed, 55 insertions(+), 35 deletions(-) diff --git a/services/core/internal/api/handler_test.go b/services/core/internal/api/handler_test.go index 8257827b7..04b28b3f6 100644 --- a/services/core/internal/api/handler_test.go +++ b/services/core/internal/api/handler_test.go @@ -149,6 +149,7 @@ func TestHTTPRejectsUntrustedOrUnsupportedRequests(t *testing.T) { {"initial input", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", valid, 503}, {"stream unavailable", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"agent":`, `"stream":true,"agent":`, 1), 503}, {"unknown saved agent", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"agent":`, `"agent_id":"saved","agent":`, 1), 404}, + {"saved agent without its model provider bundle", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"agent":`, `"agent_id":"saved","agent":`, 1), 500}, {"unknown agent option", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", strings.Replace(valid, `"model":`, `"tools":[{}],"model":`, 1), 400}, {"multiple objects", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", valid + `{}`, 400}, {"null body as empty object", "Bearer test-api-key", "agents=v1", "/v1/agents/sessions", `null`, 400}, @@ -158,10 +159,15 @@ func TestHTTPRejectsUntrustedOrUnsupportedRequests(t *testing.T) { unavailable := 0 h, s, _ := testHandler(t, func(_ *Dependencies, f *testFakes) { f.metrics.recordUnavailable = func() { unavailable++ } - if test.name == "unknown saved agent" { + switch test.name { + case "unknown saved agent": f.agentsReader.getAgentWithModelProvider = func(context.Context, string, string) (agents.Agent, *v1.ModelProviderInput, error) { return agents.Agent{}, nil, agents.ErrNotFound } + case "saved agent without its model provider bundle": + f.agentsReader.getAgentWithModelProvider = func(context.Context, string, string) (agents.Agent, *v1.ModelProviderInput, error) { + return agents.Agent{ID: "saved", Configuration: json.RawMessage(`{"model":"example","x_agents_core":{"model_provider":{"protocol":"responses","base_url":"https://model.invalid"}}}`)}, nil, nil + } } }) r := httptest.NewRequest(http.MethodPost, test.path, strings.NewReader(test.body)) diff --git a/services/core/internal/api/session_model_defaults.go b/services/core/internal/api/session_model_defaults.go index c897bd7c7..93bdffd7d 100644 --- a/services/core/internal/api/session_model_defaults.go +++ b/services/core/internal/api/session_model_defaults.go @@ -7,7 +7,6 @@ import ( v1 "github.com/MiniMax-AI/OpenAgentCore/contracts/agents-api/v1" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/agents" - "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/google/uuid" ) @@ -34,7 +33,7 @@ func (h *Handler) sessionAgentDefaults(ctx context.Context, tenant string, input return nil, nil, err } if inherit && saved.XAgentsCore != nil && saved.XAgentsCore.ModelProvider != nil && provider == nil { - return nil, nil, credentialcrypto.ErrUnavailable + return nil, nil, errors.New("saved agent model provider bundle is missing") } return saved, provider, nil } diff --git a/services/core/internal/environmenttemplates/errors.go b/services/core/internal/environmenttemplates/errors.go index 1591eaeee..7730fa274 100644 --- a/services/core/internal/environmenttemplates/errors.go +++ b/services/core/internal/environmenttemplates/errors.go @@ -7,6 +7,6 @@ var ( // the tenant. Missing, malformed and foreign IDs are indistinguishable. ErrNotFound = errors.New("environment template not found") // ErrInvalidInput reports input that is not a valid Template, or a saved - // Template whose configuration no longer decodes as one. + // Template whose configuration no longer passes validation. ErrInvalidInput = errors.New("invalid environment template") ) diff --git a/services/core/internal/persistence/postgres/sessionpg/devices.go b/services/core/internal/persistence/postgres/sessionpg/devices.go index 9bf5e67e5..7512586b3 100644 --- a/services/core/internal/persistence/postgres/sessionpg/devices.go +++ b/services/core/internal/persistence/postgres/sessionpg/devices.go @@ -16,7 +16,7 @@ import ( ) func (s *Store) GetSessionDevice(ctx context.Context, tenant, session string) (sessions.ExecutionDevice, error) { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return sessions.ExecutionDevice{}, err } @@ -27,7 +27,7 @@ func (s *Store) GetSessionDevice(ctx context.Context, tenant, session string) (s } func (s *Store) GetSessionExecutionBinding(ctx context.Context, tenant, session string) (sessions.ExecutionBinding, error) { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return sessions.ExecutionBinding{}, err } @@ -77,7 +77,7 @@ func (s *Store) ListExecutionDevices(ctx context.Context, tenant string) ([]sess } func (s *Store) GetSessionRuntimeDevice(ctx context.Context, tenant, session string) (sessions.ExecutionDevice, error) { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return sessions.ExecutionDevice{}, err } @@ -176,7 +176,7 @@ func (s *Store) CreateDevice(ctx context.Context, tenant string, registration se } func (s *Store) RevokeDevice(ctx context.Context, tenant, device string) error { - lookup, err := DeviceLookup(tenant, device) + lookup, err := ResourceLookup(tenant, device) if err != nil { return err } @@ -217,7 +217,7 @@ func (s *Store) WithEnrollment(ctx context.Context, environment, credentialHash if err != nil { return err } - lookup, err := DeviceLookup(tenant, current.ID) + lookup, err := ResourceLookup(tenant, current.ID) if err != nil { return err } diff --git a/services/core/internal/persistence/postgres/sessionpg/execution_environment.go b/services/core/internal/persistence/postgres/sessionpg/execution_environment.go index 505f859ec..de4bf3cd5 100644 --- a/services/core/internal/persistence/postgres/sessionpg/execution_environment.go +++ b/services/core/internal/persistence/postgres/sessionpg/execution_environment.go @@ -32,7 +32,7 @@ func (e *Execution) WithConnection(ctx context.Context, tenant, environment stri if err != nil { return err } - lookup, err := DeviceLookup(tenant, current.SessionID) + lookup, err := ResourceLookup(tenant, current.SessionID) if err != nil { return err } @@ -43,7 +43,7 @@ func (e *Execution) WithConnection(ctx context.Context, tenant, environment stri } func (e *Execution) WithDeviceBinding(ctx context.Context, tenant, session string, apply func(context.Context, sessions.DeviceBindingTx, sessions.LockedSession) error) error { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return err } diff --git a/services/core/internal/persistence/postgres/sessionpg/frozen.go b/services/core/internal/persistence/postgres/sessionpg/frozen.go index 40e36f0ef..d7a638f78 100644 --- a/services/core/internal/persistence/postgres/sessionpg/frozen.go +++ b/services/core/internal/persistence/postgres/sessionpg/frozen.go @@ -19,7 +19,7 @@ import ( // data that does not open, decode or validate is corrupt stored data, never // the caller's input, so it is an internal error. func (s *Store) ReadEnvironmentSetup(ctx context.Context, tenant, session string) (environmentconfig.Setup, error) { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return environmentconfig.Setup{}, sessions.ErrNotFound } @@ -51,7 +51,7 @@ func (s *Store) ReadEnvironmentSetup(ctx context.Context, tenant, session string // position. A file that does not open or match its recorded size is corrupt // stored data, so it is an internal error. func (s *Store) ReadInitialEnvironmentFile(ctx context.Context, tenant, session string, position int) (environmentconfig.InitialFileMetadata, []byte, error) { - lookup, err := DeviceLookup(tenant, session) + lookup, err := ResourceLookup(tenant, session) if err != nil { return environmentconfig.InitialFileMetadata{}, nil, err } diff --git a/services/core/internal/persistence/postgres/sessionpg/rows.go b/services/core/internal/persistence/postgres/sessionpg/rows.go index a07c5a342..d635b72bf 100644 --- a/services/core/internal/persistence/postgres/sessionpg/rows.go +++ b/services/core/internal/persistence/postgres/sessionpg/rows.go @@ -42,10 +42,10 @@ func TurnLookup(tenantID, sessionID, turnID string) (sqlc.GetTurnParams, error) return p, err } -// DeviceLookup parses a tenant and the internal identifier of one of its +// ResourceLookup parses a tenant and the internal identifier of one of its // resources, such as a device, Session or Environment; a malformed one is // sessions.ErrInvalidInput. -func DeviceLookup(tenantID, id string) (sqlc.GetDeviceParams, error) { +func ResourceLookup(tenantID, id string) (sqlc.GetDeviceParams, error) { var p sqlc.GetDeviceParams var err error if p.TenantID, err = parseID(tenantID); err != nil { @@ -76,7 +76,7 @@ func sessionFromRow(row sqlc.Session) (sessions.Session, error) { session.Creator = creator configuration, err := jsonobject.Normalize(row.Configuration) if err != nil { - return sessions.Session{}, fmt.Errorf("decode session configuration: %w: %w", sessions.ErrInvalidInput, err) + return sessions.Session{}, fmt.Errorf("decode session configuration: %w", err) } session.Configuration = configuration if err := json.Unmarshal(row.Metadata, &session.Metadata); err != nil { @@ -110,7 +110,7 @@ func environmentFromRow(row sqlc.Environment, tenant pgtype.UUID, configuration } configuration, err = jsonobject.Normalize(configuration) if err != nil { - return sessions.Environment{}, fmt.Errorf("decode environment configuration: %w: %w", sessions.ErrInvalidInput, err) + return sessions.Environment{}, fmt.Errorf("decode environment configuration: %w", err) } return sessions.Environment{ ID: uuid.UUID(row.ID.Bytes).String(), SessionID: uuid.UUID(row.SessionID.Bytes).String(), diff --git a/services/core/internal/persistence/postgres/templatepg/seal.go b/services/core/internal/persistence/postgres/templatepg/seal.go index fd504ecbc..06b2f0749 100644 --- a/services/core/internal/persistence/postgres/templatepg/seal.go +++ b/services/core/internal/persistence/postgres/templatepg/seal.go @@ -2,6 +2,7 @@ package templatepg import ( "encoding/json" + "errors" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/credentialcrypto" "github.com/MiniMax-AI/OpenAgentCore/services/core/internal/environmentconfig" @@ -87,7 +88,7 @@ func (s *Store) openSetup(tenant, id pgtype.UUID, field string, ciphertext []byt return err } if environmentconfig.Decode(plaintext, output) != nil { - return environmenttemplates.ErrInvalidInput + return errors.New("invalid stored environment template " + field) } return nil } diff --git a/services/core/internal/persistence/postgres/templatepg/store.go b/services/core/internal/persistence/postgres/templatepg/store.go index 61f93bb4b..cb3d1d502 100644 --- a/services/core/internal/persistence/postgres/templatepg/store.go +++ b/services/core/internal/persistence/postgres/templatepg/store.go @@ -169,8 +169,10 @@ func (s *Store) List(ctx context.Context, tenantID string, query environmenttemp } // Resolve reads one row, so the metadata and every confidential field come -// from the same committed version. Stored Skill and Plugin metadata must match -// the sealed archives they describe. Any malformed ID resolves to ErrNotFound. +// from the same committed version. Stored data that does not decode or match +// its sealed contents is corrupt, an internal error; configuration that decodes +// but fails current validation is ErrInvalidInput. Any malformed ID resolves to +// ErrNotFound. func (s *Store) Resolve(ctx context.Context, tenantID, templateID string) (environmenttemplates.Resolved, error) { tenant, err := pgunit.ParseID(tenantID) if err != nil { @@ -203,12 +205,15 @@ func (s *Store) Resolve(ctx context.Context, tenantID, templateID string) (envir return environmenttemplates.Resolved{}, err } } - if !matches(metadata.Skills, resolved.Setup.SkillMetadata()) || !matches(metadata.Plugins, resolved.Setup.PluginMetadata()) || resolved.Setup.Validate() != nil { + if !matches(metadata.Skills, resolved.Setup.SkillMetadata()) || !matches(metadata.Plugins, resolved.Setup.PluginMetadata()) { + return environmenttemplates.Resolved{}, errors.New("stored environment template metadata does not match its sealed setup") + } + if resolved.Setup.Validate() != nil { return environmenttemplates.Resolved{}, environmenttemplates.ErrInvalidInput } if len(row.FileContents) == 0 { if len(metadata.Files) > 0 { - return environmenttemplates.Resolved{}, environmenttemplates.ErrInvalidInput + return environmenttemplates.Resolved{}, errors.New("stored environment template files have no contents") } return resolved, nil } @@ -219,7 +224,10 @@ func (s *Store) Resolve(ctx context.Context, tenantID, templateID string) (envir if err != nil { return environmenttemplates.Resolved{}, err } - if json.Unmarshal(plaintext, &resolved.Files) != nil || environmentconfig.ValidateInitialFiles(resolved.Files) != nil { + if json.Unmarshal(plaintext, &resolved.Files) != nil { + return environmenttemplates.Resolved{}, errors.New("invalid stored environment template files") + } + if environmentconfig.ValidateInitialFiles(resolved.Files) != nil { return environmenttemplates.Resolved{}, environmenttemplates.ErrInvalidInput } return resolved, nil @@ -246,7 +254,7 @@ func template(row metadataRow) (environmenttemplates.Template, error) { result.Name = &row.Name.String } if json.Unmarshal(row.Files, &result.Files) != nil || environmentconfig.Decode(row.Packages, &result.Packages) != nil || json.Unmarshal(row.Skills, &result.Skills) != nil || json.Unmarshal(row.Plugins, &result.Plugins) != nil { - return environmenttemplates.Template{}, environmenttemplates.ErrInvalidInput + return environmenttemplates.Template{}, errors.New("invalid stored environment template metadata") } return result, nil } diff --git a/services/core/internal/persistence/postgres/templatepg/store_test.go b/services/core/internal/persistence/postgres/templatepg/store_test.go index ee5b02b82..7710eb447 100644 --- a/services/core/internal/persistence/postgres/templatepg/store_test.go +++ b/services/core/internal/persistence/postgres/templatepg/store_test.go @@ -370,9 +370,10 @@ func TestMissingKeyIsUnavailable(t *testing.T) { } } -// Stored package metadata naming the removed system manager is rejected on -// every read instead of being silently dropped. -func TestStoredSystemPackagesRejected(t *testing.T) { +// Stored package metadata naming the removed system manager no longer decodes, +// so every read fails with an internal error instead of dropping it. Packages +// that decode but fail current validation are an invalid Template. +func TestStoredPackagesRejected(t *testing.T) { f := newFixture(t, pgtest.Open(t)) ctx := t.Context() tenant := uuid.NewString() @@ -383,18 +384,23 @@ func TestStoredSystemPackagesRejected(t *testing.T) { t.Fatal(err) } } + internal := func(err error) bool { return err != nil && !errors.Is(err, environmenttemplates.ErrInvalidInput) } for _, value := range []string{`null`, `[]`, `["jq"]`} { store(`{"npm":[],"python":[],"system":` + value + `}`) - if _, err := f.keyless.Get(ctx, tenant, template.ID); !errors.Is(err, environmenttemplates.ErrInvalidInput) { - t.Fatal("get silently ignored removed system packages", value, err) + if _, err := f.keyless.Get(ctx, tenant, template.ID); !internal(err) { + t.Fatal("get did not fail internally on removed system packages", value, err) } - if _, err := f.keyless.List(ctx, tenant, environmenttemplates.ListQuery{Limit: 1}); !errors.Is(err, environmenttemplates.ErrInvalidInput) { - t.Fatal("list silently ignored removed system packages", value, err) + if _, err := f.keyless.List(ctx, tenant, environmenttemplates.ListQuery{Limit: 1}); !internal(err) { + t.Fatal("list did not fail internally on removed system packages", value, err) } - if _, err := f.keyed.Resolve(ctx, tenant, template.ID); !errors.Is(err, environmenttemplates.ErrInvalidInput) { - t.Fatal("resolve silently ignored removed system packages", value, err) + if _, err := f.keyed.Resolve(ctx, tenant, template.ID); !internal(err) { + t.Fatal("resolve did not fail internally on removed system packages", value, err) } } + store(`{"npm":["-x"],"python":[]}`) + if _, err := f.keyed.Resolve(ctx, tenant, template.ID); !errors.Is(err, environmenttemplates.ErrInvalidInput) { + t.Fatal("resolve accepted packages that fail validation", err) + } store(`{"npm":["semver"],"python":["packaging"]}`) if got, err := f.keyless.Get(ctx, tenant, template.ID); err != nil || !reflect.DeepEqual(got.Packages, v1.EnvironmentPackages{NPM: []string{"semver"}, Python: []string{"packaging"}}) { t.Fatal("supported stored package managers rejected", got.Packages, err) diff --git a/services/core/tests/integration/session_reads_fixture_test.go b/services/core/tests/integration/session_reads_fixture_test.go index 43f097c11..6dbc04a1b 100644 --- a/services/core/tests/integration/session_reads_fixture_test.go +++ b/services/core/tests/integration/session_reads_fixture_test.go @@ -24,7 +24,7 @@ func FixtureEnvironmentDevice(ctx context.Context, pool *pgxpool.Pool, tenant, e if err != nil { return sessions.ExecutionDevice{}, err } - lookup, err := sessionpg.DeviceLookup(tenant, current.SessionID) + lookup, err := sessionpg.ResourceLookup(tenant, current.SessionID) if err != nil { return sessions.ExecutionDevice{}, err } diff --git a/services/core/tests/integration/turn_journal_reads_test.go b/services/core/tests/integration/turn_journal_reads_test.go index 09ee32b68..1bb705a24 100644 --- a/services/core/tests/integration/turn_journal_reads_test.go +++ b/services/core/tests/integration/turn_journal_reads_test.go @@ -50,7 +50,7 @@ type SubagentIdentity struct { // GetSubagentIdentity reads a binding in its authorized, visible Session. Only // tests read bindings back. func (s *Store) GetSubagentIdentity(ctx context.Context, tenantID, sessionID, nativeID string) (SubagentIdentity, error) { - p, err := sessionpg.DeviceLookup(tenantID, sessionID) + p, err := sessionpg.ResourceLookup(tenantID, sessionID) if err != nil { return SubagentIdentity{}, err }