From c8ebe9a7896425297bfcaf4d7e38388ead1c618f Mon Sep 17 00:00:00 2001 From: viettranx Date: Sat, 11 Apr 2026 12:24:29 +0700 Subject: [PATCH] refactor(comments): remove plan/phase refs from agent identity hardening code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite inline comments added during the agent identity hardening so they explain the code as it stands today, rather than tying to internal plan terminology (phase numbers, FR/NFR/H/M/C codes, PR references, trap zone labels). Commit history already carries the plan archaeology. Comments now keep the non-obvious invariants (cache boundaries, bypass gaps, silent-nil traps, dual-tenant semantics) and drop the scaffolding. Comment-only — no runtime behavior change. --- cmd/gateway_methods.go | 2 +- internal/agent/loop_tracing.go | 4 ++-- internal/agent/memoryflush.go | 7 +++--- internal/agent/memoryflush_test.go | 7 +++--- internal/agent/router.go | 8 +++---- .../agent/router_cache_canonicalize_test.go | 22 +++++++++---------- .../agent/router_invalidate_collision_test.go | 8 +++---- internal/agent/router_isrunning_test.go | 10 ++++----- internal/eventbus/bus_impl.go | 1 - internal/eventbus/validate_agent_id.go | 20 ++++++++--------- internal/eventbus/validate_agent_id_test.go | 5 +++-- internal/gateway/methods/agent_links.go | 4 ++-- .../methods/agent_resolve_cached_test.go | 9 ++++---- internal/gateway/methods/channel_instances.go | 12 +++++----- internal/gateway/methods/teams_members.go | 6 ++--- .../gateway/methods/teams_tasks_mutations.go | 2 +- internal/http/vault_handler_upload.go | 9 ++++---- internal/http/vault_handler_upload_test.go | 22 +++++++++---------- internal/store/pg/memory_docs.go | 4 ++-- internal/store/pg/vault_documents.go | 5 ++--- 20 files changed, 82 insertions(+), 85 deletions(-) diff --git a/cmd/gateway_methods.go b/cmd/gateway_methods.go index bb541f65..054fd8dd 100644 --- a/cmd/gateway_methods.go +++ b/cmd/gateway_methods.go @@ -43,7 +43,7 @@ func registerAllMethods(server *gateway.Server, agents *agent.Router, sessStore // Phase 2: Heartbeat heartbeatMethods := methods.NewHeartbeatMethods(heartbeatStore, msgBus) - // Wire cache-aware resolver (Phase 3): accepts agent_key or UUID input + // Wire cache-aware resolver so heartbeat can accept agent_key or UUID // without a DB roundtrip on the hot path when the agent is router-cached. heartbeatMethods.SetAgentRouter(agents) heartbeatMethods.Register(router) diff --git a/internal/agent/loop_tracing.go b/internal/agent/loop_tracing.go index e8174cee..cd5f5cc6 100644 --- a/internal/agent/loop_tracing.go +++ b/internal/agent/loop_tracing.go @@ -24,12 +24,12 @@ func (l *Loop) emit(event AgentEvent) { // ID returns the agent's identifier (agent_key, e.g. "goctech-leader"). // Use for logs, UI, filesystem paths. NEVER for DB FK or DomainEvent.AgentID. -// See docs/agent-identity-conventions.md (Phase 6). +// See docs/agent-identity-conventions.md. func (l *Loop) ID() string { return l.id } // UUID returns the agent's canonical UUID (DB primary key). // Use for SQL WHERE/JOIN, DomainEvent.AgentID, context propagation. -// See docs/agent-identity-conventions.md (Phase 6). +// See docs/agent-identity-conventions.md. func (l *Loop) UUID() uuid.UUID { return l.agentUUID } // Model returns the model identifier for this agent loop. diff --git a/internal/agent/memoryflush.go b/internal/agent/memoryflush.go index b2016502..d5106100 100644 --- a/internal/agent/memoryflush.go +++ b/internal/agent/memoryflush.go @@ -70,7 +70,7 @@ func ResolveMemoryFlushSettings(compaction *config.CompactionConfig) *MemoryFlus // buildMemoryFlushPromptConfig returns the SystemPromptConfig used by the // memory flush turn. Extracted as a pure function so tests can assert the // config shape (specifically, that AgentUUID is populated) without building -// a full Loop fixture. See Phase 1 Fix A and M7 mitigation. +// a full Loop fixture. func buildMemoryFlushPromptConfig( agentID, agentUUID, model, workspace string, toolNames []string, @@ -131,9 +131,8 @@ func (l *Loop) runMemoryFlush(ctx context.Context, sessionKey string, settings * flushSystemPrompt := strings.ReplaceAll(settings.SystemPrompt, "YYYY-MM-DD", today) // System prompt: combine agent's normal system prompt context with flush system prompt. - // Config construction extracted to buildMemoryFlushPromptConfig for testability — the - // AgentUUID field here mirrors loop_history.go:199-201 and must stay in sync - // (regression: missing AgentUUID caused identity drift in DomainEvents — see PR #826). + // AgentUUID must stay in sync with loop_history.go's SystemPromptConfig — + // missing it here historically caused identity drift in downstream DomainEvents. systemPrompt := BuildSystemPrompt(buildMemoryFlushPromptConfig( l.id, l.agentUUID.String(), diff --git a/internal/agent/memoryflush_test.go b/internal/agent/memoryflush_test.go index ff37897a..1e0f850c 100644 --- a/internal/agent/memoryflush_test.go +++ b/internal/agent/memoryflush_test.go @@ -6,10 +6,11 @@ import ( "github.com/google/uuid" ) -// TestBuildMemoryFlushPromptConfig_AgentUUIDPopulated asserts Fix A — the +// TestBuildMemoryFlushPromptConfig_AgentUUIDPopulated asserts the // SystemPromptConfig returned by buildMemoryFlushPromptConfig carries the -// AgentUUID. Historically missing; loop_history.go set it but memoryflush.go -// did not, leading to identity drift (see PR #826 / brainstorm trap zone 2). +// AgentUUID. loop_history.go always set it but memoryflush.go historically +// did not, which would have caused identity drift in downstream DomainEvents +// if AgentUUID ever reached the stable cache prefix. func TestBuildMemoryFlushPromptConfig_AgentUUIDPopulated(t *testing.T) { u := uuid.New() cfg := buildMemoryFlushPromptConfig( diff --git a/internal/agent/router.go b/internal/agent/router.go index a103a192..af18abfd 100644 --- a/internal/agent/router.go +++ b/internal/agent/router.go @@ -194,10 +194,10 @@ func (r *Router) ListInfo() []AgentInfo { return infos } -// IsRunning checks if a specific agent is currently running (cached in router). -// Ctx carries tenant scope — pre-fix (C6) this function did a bare lookup which -// always returned false in tenant-scoped deployments, causing `agents.list` to -// incorrectly report every live agent as idle. +// IsRunning checks if a specific agent is currently running (cached in +// router). Ctx carries tenant scope — without it, a bare lookup returns false +// in tenant-scoped deployments because cache keys are stored as +// `tenantID:agentKey` after resolution. func (r *Router) IsRunning(ctx context.Context, agentID string) bool { cacheKey := agentCacheKey(ctx, agentID) r.mu.RLock() diff --git a/internal/agent/router_cache_canonicalize_test.go b/internal/agent/router_cache_canonicalize_test.go index 6c6e24f4..9c85dd1a 100644 --- a/internal/agent/router_cache_canonicalize_test.go +++ b/internal/agent/router_cache_canonicalize_test.go @@ -36,10 +36,10 @@ func stubResolver(agentKey string) ResolverFunc { } } -// TestRouterGet_UUIDInputStoresCanonicalKey — Phase 2 FR-1. -// When the caller passes a UUID-like string to Get(), the cache entry must -// land under tenantID:agentKey (canonical), NOT tenantID:uuidStr. -// Exercises the canonicalization path via a real resolver call. +// TestRouterGet_UUIDInputStoresCanonicalKey verifies that when the caller +// passes a UUID-like string to Get(), the cache entry lands under +// tenantID:agentKey (canonical), NOT tenantID:uuidStr. Exercises the +// canonicalization path via a real resolver call. func TestRouterGet_UUIDInputStoresCanonicalKey(t *testing.T) { r := NewRouter() r.SetResolver(stubResolver("goctech-leader")) @@ -72,9 +72,9 @@ func TestRouterGet_UUIDInputStoresCanonicalKey(t *testing.T) { } } -// TestRouterGet_IdempotentCacheUnderKeyOrUUID — Phase 2 FR-4. -// Calling Get() first with agent_key then with UUID should produce exactly -// ONE cache entry — the canonical tenantID:agent_key. +// TestRouterGet_IdempotentCacheUnderKeyOrUUID verifies that calling Get() +// first with agent_key then with UUID produces exactly ONE cache entry — the +// canonical tenantID:agent_key. func TestRouterGet_IdempotentCacheUnderKeyOrUUID(t *testing.T) { r := NewRouter() var resolveCount atomic.Int32 @@ -107,10 +107,10 @@ func TestRouterGet_IdempotentCacheUnderKeyOrUUID(t *testing.T) { } } -// TestRouterGet_UUIDCallerResolvesEveryTime — Phase 2 honest cost (H1). -// A caller that keeps passing the UUID form never hits the canonical key on -// read, so the resolver runs on every call. Document this behavior so future -// refactors don't pretend the cache covers UUID inputs. +// TestRouterGet_UUIDCallerResolvesEveryTime documents the honest cost of +// canonicalization: a caller that keeps passing the UUID form never hits the +// canonical key on read, so the resolver runs on every call. Pin this +// behavior so future refactors don't pretend the cache covers UUID inputs. func TestRouterGet_UUIDCallerResolvesEveryTime(t *testing.T) { r := NewRouter() var resolveCount atomic.Int32 diff --git a/internal/agent/router_invalidate_collision_test.go b/internal/agent/router_invalidate_collision_test.go index 195f9502..0a374624 100644 --- a/internal/agent/router_invalidate_collision_test.go +++ b/internal/agent/router_invalidate_collision_test.go @@ -4,10 +4,10 @@ import ( "testing" ) -// TestInvalidateAgent_NoSubstringCollision — Phase 2 FR-2. -// Before the fix, Router.Remove / InvalidateAgent used strings.HasSuffix -// which would match "tenantX:sub-foo" when invalidating "foo". Verify -// exact-segment match rejects substring collisions. +// TestInvalidateAgent_NoSubstringCollision verifies exact-segment match +// rejects substring collisions. A previous HasSuffix-based matcher would have +// removed "tenantX:sub-foo" when invalidating "foo" — this regression guards +// against that. func TestInvalidateAgent_NoSubstringCollision(t *testing.T) { r := NewRouter() r.agents["tenantX:foo"] = &agentEntry{} diff --git a/internal/agent/router_isrunning_test.go b/internal/agent/router_isrunning_test.go index c71675c9..6cb8dc86 100644 --- a/internal/agent/router_isrunning_test.go +++ b/internal/agent/router_isrunning_test.go @@ -10,11 +10,11 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) -// TestIsRunning_TenantScopedLookup — Phase 2 FR-5 (C6). -// Router.IsRunning must accept ctx and lookup under tenant-scoped cache key. -// Pre-fix: bare `r.agents[agentID]` lookup always returned false for any -// tenant-scoped deployment, so the WS `agents.list` response incorrectly -// showed `isRunning: false` for every live agent. +// TestIsRunning_TenantScopedLookup asserts that Router.IsRunning accepts ctx +// and looks up under the tenant-scoped cache key. A previous bare +// `r.agents[agentID]` lookup always returned false for any tenant-scoped +// deployment, so the WS `agents.list` response incorrectly showed every live +// agent as `isRunning: false`. func TestIsRunning_TenantScopedLookup(t *testing.T) { r := NewRouter() tenantA := uuid.New() diff --git a/internal/eventbus/bus_impl.go b/internal/eventbus/bus_impl.go index bbbb53a9..e81c5015 100644 --- a/internal/eventbus/bus_impl.go +++ b/internal/eventbus/bus_impl.go @@ -64,7 +64,6 @@ func (b *busImpl) Publish(event DomainEvent) { return } // Publish-time observer: warns on non-UUID AgentID drift without blocking. - // See validate_agent_id.go for rationale (PR #826 regression safety net). validateAgentID(event) select { case b.queue <- event: diff --git a/internal/eventbus/validate_agent_id.go b/internal/eventbus/validate_agent_id.go index 2ed21f08..63d666fd 100644 --- a/internal/eventbus/validate_agent_id.go +++ b/internal/eventbus/validate_agent_id.go @@ -6,19 +6,17 @@ import ( "github.com/google/uuid" ) -// validateAgentID is a publish-time observer that logs a warning when a DomainEvent -// carries a non-UUID AgentID. It does NOT block the publish — observability only. +// validateAgentID is a publish-time observer that logs a warning when a +// DomainEvent carries a non-UUID AgentID. It does NOT block the publish — +// observability only. Acts as a safety net catching future drift before the +// event reaches a consumer that parses the field as a UUID and queries the DB +// with it. // -// Motivation: PR #826 fixed multiple call sites that passed agent_key strings -// (e.g. "goctech-leader") into DomainEvent.AgentID, silently corrupting downstream -// consumers. This helper acts as a safety net to catch any future drift BEFORE -// the event reaches a consumer that parses the field as a UUID. +// Log field name is `non_uuid_agent_id` — intentionally distinct from the +// standard `agent_id` field used elsewhere — to avoid collision with +// observability tooling that parses `agent_id` as a UUID. // -// Log field name: `non_uuid_agent_id` (intentionally distinct from the standard -// `agent_id` field used elsewhere) to avoid collision with observability tooling -// that parses `agent_id` as a UUID. See red-team finding H6. -// -// See docs/agent-identity-conventions.md (Phase 6) for the convention. +// See docs/agent-identity-conventions.md for the full convention. func validateAgentID(event DomainEvent) { if event.AgentID == "" { return // legitimate team-owned, tenant-scoped, or anonymous event diff --git a/internal/eventbus/validate_agent_id_test.go b/internal/eventbus/validate_agent_id_test.go index 732d9477..9919fc92 100644 --- a/internal/eventbus/validate_agent_id_test.go +++ b/internal/eventbus/validate_agent_id_test.go @@ -76,8 +76,9 @@ func TestValidateAgentID_NonUUIDWarns(t *testing.T) { } func TestValidateAgentID_DistinctFieldName_NoCollision(t *testing.T) { - // H6 mitigation: the field name must NOT be `agent_id` (which downstream - // observability tooling parses as UUID) — it must be `non_uuid_agent_id`. + // The log field name must NOT be `agent_id` (which downstream + // observability tooling parses as UUID) — it must be `non_uuid_agent_id` + // so the warning never collides with valid UUID-typed agent_id fields. buf, restore := captureSlog(t) defer restore() diff --git a/internal/gateway/methods/agent_links.go b/internal/gateway/methods/agent_links.go index 4d12c0f5..41fc2555 100644 --- a/internal/gateway/methods/agent_links.go +++ b/internal/gateway/methods/agent_links.go @@ -412,7 +412,7 @@ func (m *AgentLinksMethods) invalidateLinkAgentsByID(ctx context.Context, source // its canonical UUID via a DB lookup. Tenant-aware via // store.TenantIDFromContext(ctx) inside agentStore.GetByID/GetByKey. Prefer // resolveAgentUUIDCached in hot-path handlers to avoid the extra DB roundtrip. -// See docs/agent-identity-conventions.md trap zone 5. +// See docs/agent-identity-conventions.md. func resolveAgentUUID(ctx context.Context, agentStore store.AgentStore, keyOrID string) (uuid.UUID, error) { if id, err := uuid.Parse(keyOrID); err == nil { ag, err := agentStore.GetByID(ctx, id) @@ -441,7 +441,7 @@ type agentUUIDProvider interface { // on cache miss or when the input is a UUID string (router cache keys are // canonicalized to `tenantID:agentKey`, so UUID inputs never hit the cache). // If router is nil, delegates straight to resolveAgentUUID. -// See docs/agent-identity-conventions.md trap zone 5 and section 8. +// See docs/agent-identity-conventions.md. func resolveAgentUUIDCached(ctx context.Context, router *agent.Router, agentStore store.AgentStore, keyOrID string) (uuid.UUID, error) { // Fast path: input is agent_key and the agent is cached in the router. if router != nil { diff --git a/internal/gateway/methods/agent_resolve_cached_test.go b/internal/gateway/methods/agent_resolve_cached_test.go index 547c0d5c..e7055ca5 100644 --- a/internal/gateway/methods/agent_resolve_cached_test.go +++ b/internal/gateway/methods/agent_resolve_cached_test.go @@ -59,10 +59,11 @@ func TestResolveAgentUUIDCached_CacheMissFallsBack(t *testing.T) { } } -// TestResolveAgentUUIDCached_UUIDInputTakesDBPath — per Phase 2 H1, a caller -// passing the UUID form is never cached under the raw UUID key, so the helper -// must fall through to the DB path. Verifies this by checking the stub's -// sentinel error surfaces. +// TestResolveAgentUUIDCached_UUIDInputTakesDBPath verifies that a caller +// passing the UUID form falls through to the DB path. Router cache entries +// are canonicalized to `tenantID:agentKey`, so the raw UUID input never hits +// the cache and the helper must delegate to the store stub (whose sentinel +// error surfaces here). func TestResolveAgentUUIDCached_UUIDInputTakesDBPath(t *testing.T) { r := agent.NewRouter() stub := &errorAgentStore{err: errSentinelMiss} diff --git a/internal/gateway/methods/channel_instances.go b/internal/gateway/methods/channel_instances.go index 71d9e9f8..3f06f2c0 100644 --- a/internal/gateway/methods/channel_instances.go +++ b/internal/gateway/methods/channel_instances.go @@ -23,10 +23,8 @@ var channelInstanceAllowed = map[string]bool{ } // ChannelInstancesMethods handles channel instance CRUD via WebSocket RPC. -// -// NFR-2 exception (Phase 3 C2): agentStore is a new struct field required to -// support agent_key input via resolveAgentUUIDCached. Constructor signature -// expanded — single caller at cmd/gateway_channels_setup.go. +// agentStore is held so the create/update handlers can resolve agent_key or +// UUID input via resolveAgentUUIDCached. type ChannelInstancesMethods struct { store store.ChannelInstanceStore agentStore store.AgentStore @@ -127,9 +125,9 @@ func (m *ChannelInstancesMethods) handleCreate(ctx context.Context, client *gate return } - // Accept both agent_key and UUID via resolveAgentUUIDCached (Phase 3 FR-1). - // nil router: channel_instances methods are not wired to the router — falls - // back to a pure DB lookup which is acceptable given create is a rare op. + // Accept both agent_key and UUID via resolveAgentUUIDCached. Router is nil + // here because channel_instances methods are not wired to the router — + // falls back to a pure DB lookup, acceptable given create is a rare op. agentID, err := resolveAgentUUIDCached(ctx, nil, m.agentStore, params.AgentID) if err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "agent_id"))) diff --git a/internal/gateway/methods/teams_members.go b/internal/gateway/methods/teams_members.go index cf058e7d..1f5daff6 100644 --- a/internal/gateway/methods/teams_members.go +++ b/internal/gateway/methods/teams_members.go @@ -52,8 +52,8 @@ func (m *TeamsMethods) handleAddMember(ctx context.Context, client *gateway.Clie return } - // Resolve agent — accepts agent_key or UUID. H10 fix: return i18n error instead - // of leaking raw store error (was: "agent: " + err.Error()). + // Resolve agent — accepts agent_key or UUID. Return an i18n error on + // failure; never leak the raw store error string to WS clients. ag, err := resolveAgentInfo(ctx, m.agentStore, params.Agent) if err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "agent"))) @@ -135,7 +135,7 @@ func (m *TeamsMethods) handleRemoveMember(ctx context.Context, client *gateway.C client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "teamId"))) return } - // Phase 3 FR-1: accept agent_key or UUID via cache-aware resolver. + // Accept agent_key or UUID via cache-aware resolver. agentID, err := resolveAgentUUIDCached(ctx, m.agentRouter, m.agentStore, params.AgentID) if err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "agentId"))) diff --git a/internal/gateway/methods/teams_tasks_mutations.go b/internal/gateway/methods/teams_tasks_mutations.go index c498c1a0..c9e032c3 100644 --- a/internal/gateway/methods/teams_tasks_mutations.go +++ b/internal/gateway/methods/teams_tasks_mutations.go @@ -151,7 +151,7 @@ func (m *TeamsMethods) handleTaskAssign(ctx context.Context, client *gateway.Cli client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "taskId"))) return } - // Phase 3 FR-1: accept agent_key or UUID for assignee. + // Accept agent_key or UUID for assignee. agentID, err := resolveAgentUUIDCached(ctx, m.agentRouter, m.agentStore, params.AgentID) if err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgInvalidID, "agentId"))) diff --git a/internal/http/vault_handler_upload.go b/internal/http/vault_handler_upload.go index c2d7af5e..c0004916 100644 --- a/internal/http/vault_handler_upload.go +++ b/internal/http/vault_handler_upload.go @@ -41,10 +41,11 @@ func (h *VaultHandler) handleUpload(w http.ResponseWriter, r *http.Request) { agentIDStr := r.FormValue("agent_id") teamIDStr := r.FormValue("team_id") - // Boundary UUID validation — Fix B (Phase 1, H9 mitigation). - // validateTeamMembership short-circuits on owner role + lite edition (nil teamAccess), - // leaving downstream `parseUUIDOrNil(*doc.TeamID)` as a silent-nil trap. Validate at - // the HTTP boundary so bad form input is rejected before any store call or event publish. + // Boundary UUID validation. validateTeamMembership below short-circuits + // on owner role + lite edition (nil teamAccess), which would leave a + // downstream parseUUIDOrNil(*doc.TeamID) call as a silent-nil trap. + // Validate at the HTTP boundary so bad form input is rejected before any + // store call or event publish. // See docs/agent-identity-conventions.md. if agentIDStr != "" { if _, err := uuid.Parse(agentIDStr); err != nil { diff --git a/internal/http/vault_handler_upload_test.go b/internal/http/vault_handler_upload_test.go index 83f18188..844a7fe1 100644 --- a/internal/http/vault_handler_upload_test.go +++ b/internal/http/vault_handler_upload_test.go @@ -60,10 +60,10 @@ func assertBadRequest(t *testing.T, rr *httptest.ResponseRecorder, wantFragment } } -// TestHandleUpload_InvalidAgentIDReturns400 asserts Phase 1 Fix B — bad form -// `agent_id` is rejected at the HTTP boundary before any store call. -// Closes the owner/lite edition gap where validateTeamMembership would skip -// the UUID check. +// TestHandleUpload_InvalidAgentIDReturns400 asserts bad form `agent_id` is +// rejected at the HTTP boundary before any store call. Closes the +// owner / lite edition gap where validateTeamMembership would skip the UUID +// check. func TestHandleUpload_InvalidAgentIDReturns400(t *testing.T) { h := &VaultHandler{} // no store wired — boundary check runs first req := buildUploadRequest(t, map[string]string{ @@ -76,9 +76,10 @@ func TestHandleUpload_InvalidAgentIDReturns400(t *testing.T) { assertBadRequest(t, rr, "invalid agent_id") } -// TestHandleUpload_InvalidTeamIDReturns400 asserts Phase 1 Fix B for team_id. -// This is the hole validateTeamMembership leaves open: it short-circuits on -// owner role (line 57) and nil teamAccess (line 60), never parsing the UUID. +// TestHandleUpload_InvalidTeamIDReturns400 asserts bad form `team_id` is +// rejected at the HTTP boundary. validateTeamMembership short-circuits on +// owner role and on nil teamAccess (lite edition), never parsing the UUID — +// the boundary check closes that hole. func TestHandleUpload_InvalidTeamIDReturns400(t *testing.T) { h := &VaultHandler{} // lite edition: teamAccess is nil req := buildUploadRequest(t, map[string]string{ @@ -91,10 +92,9 @@ func TestHandleUpload_InvalidTeamIDReturns400(t *testing.T) { assertBadRequest(t, rr, "invalid team_id") } -// TestHandleUpload_InvalidAgentID_OwnerContext asserts the boundary check fires -// regardless of role. Pre-Fix-B, validateTeamMembership skipped the check for -// owner role — but agent_id was never validated at all upstream, so the UUID -// hole also existed for admins. Boundary check closes it for every caller. +// TestHandleUpload_InvalidAgentID_OwnerContext asserts the boundary check +// fires regardless of role. The UUID hole existed for every caller — the +// boundary check ignores role entirely. func TestHandleUpload_InvalidAgentID_OwnerContext(t *testing.T) { h := &VaultHandler{} req := buildUploadRequest(t, map[string]string{ diff --git a/internal/store/pg/memory_docs.go b/internal/store/pg/memory_docs.go index e56b7d32..f92014d2 100644 --- a/internal/store/pg/memory_docs.go +++ b/internal/store/pg/memory_docs.go @@ -482,7 +482,7 @@ func (s *PGMemoryStore) Close() error { return nil } // either corrupt data or hide bugs as empty reads / zero-row updates. FK // constraints reject bad writes at the driver layer, but errors there come // back as cryptic PG 23503 — parseUUID catches them upstream with a clean -// Go error. See docs/agent-identity-conventions.md trap zone 3. +// Go error. See docs/agent-identity-conventions.md. func parseUUID(s string) (uuid.UUID, error) { id, err := uuid.Parse(s) if err != nil { @@ -496,7 +496,7 @@ func parseUUID(s string) (uuid.UUID, error) { // SELECT WHERE paths where a no-match (empty result) is the correct // semantics on bad input. Do NOT use for writes, updates, deletes, or any // SELECT where an empty result would hide a bug. Prefer parseUUID for new -// code. See docs/agent-identity-conventions.md trap zone 3. +// code. See docs/agent-identity-conventions.md. func parseUUIDOrNil(s string) uuid.UUID { id, err := uuid.Parse(s) if err != nil { diff --git a/internal/store/pg/vault_documents.go b/internal/store/pg/vault_documents.go index 86e5d851..cacb19d2 100644 --- a/internal/store/pg/vault_documents.go +++ b/internal/store/pg/vault_documents.go @@ -57,9 +57,8 @@ func (s *PGVaultStore) Close() error { return nil } // optAgentUUID converts a nullable *string agent_id to *uuid.UUID for SQL. // Returns (nil, nil) when the input is nil or empty — a legitimate SQL NULL. // Returns (nil, error) on a non-empty, non-UUID input — propagating the error -// prevents silent-nil writes that would otherwise corrupt data (pre-Phase-4 -// this helper silently returned nil on garbage input). -// See docs/agent-identity-conventions.md (Phase 6). +// prevents silent-nil writes that would otherwise corrupt data. +// See docs/agent-identity-conventions.md. func optAgentUUID(agentID *string) (*uuid.UUID, error) { if agentID == nil || *agentID == "" { return nil, nil