From ef686fac48dde0b57f7401f9707f539c455fb96a Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Tue, 11 Aug 2026 13:35:55 -0700 Subject: [PATCH] improvement(provenance): make the incompleteness reason set closed and complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reason a resolved-secret registry latched is the only thing that names which guard tripped, and a refusal surfaces many frames later as one fixed sentence. Two gaps had opened in that set. `workspace-scope-missing` no longer has a producer: the `!context.workspaceId` guard in the copilot table tool went away when `importRowsForModel` was rewritten, and every operation now returns early on a missing workspace before provenance import is reachable. The literal and its warn-classification test case go with it — the test constructed the reason itself, so it asserted on something nothing emits. ResolvedSecretTraceProvenanceAccumulator had no reason concept at all, so its three guards latched anonymously. That matters more there than on the registry: the wire format carries only `complete`, so the consumer can only ever say `source-provenance-incomplete`, and the guard is unrecoverable. Give it the same required `reason` and name all three — a file source with no workspace identity, a workspace file whose sidecar reads unknown, and an MCP tool that timed out. A latch from `record()` stays silent, since it reflects a bundle whose own registry already reported and subflow aggregation runs it per iteration. Fold the error/warn/by-design split into one `reportIncompleteness`. It was copied across both registry latches and would have been copied a third time here, and a copy that can be updated alone lets one reason be a fault in one place and routine in another. Also give the async workflow tool path its own import origin instead of latching with none, and close `UnrecordedDurableProvenanceCause`, which was a free-form string carrying a TSDoc claim that it was always a static literal. --- apps/sim/app/api/mcp/tools/execute/route.ts | 2 +- apps/sim/app/api/tools/file/manage/route.ts | 4 +- .../resolved-secret-trace-registry.test.ts | 38 +++++++++- .../utils/resolved-secret-trace-registry.ts | 69 ++++++++++++++----- apps/sim/lib/copilot/request/tools/client.ts | 13 +++- .../durable-secret-provenance-enforcement.ts | 15 +++- 6 files changed, 113 insertions(+), 28 deletions(-) diff --git a/apps/sim/app/api/mcp/tools/execute/route.ts b/apps/sim/app/api/mcp/tools/execute/route.ts index eabe79c70ba..d045b407a54 100644 --- a/apps/sim/app/api/mcp/tools/execute/route.ts +++ b/apps/sim/app/api/mcp/tools/execute/route.ts @@ -327,7 +327,7 @@ export const POST = withRouteHandler( return successResponse(transformedResult) } catch (error) { if (getErrorMessage(error) === 'Tool execution timeout') { - resolvedSecretTraceProvenance?.markIncomplete() + resolvedSecretTraceProvenance?.markIncomplete('mcp-tool-execution-timeout') } const bodyErrorResponse = mcpBodyReadErrorResponse(error, request) if (bodyErrorResponse) return bodyErrorResponse diff --git a/apps/sim/app/api/tools/file/manage/route.ts b/apps/sim/app/api/tools/file/manage/route.ts index d22d90f8389..3df9ac4ffd3 100644 --- a/apps/sim/app/api/tools/file/manage/route.ts +++ b/apps/sim/app/api/tools/file/manage/route.ts @@ -295,12 +295,12 @@ async function getFileContentProvenance( for (const source of sources) { if (!source.identity || !source.ownerUserId) { - accumulator.markIncomplete() + accumulator.markIncomplete('file-source-unidentified') continue } const provenance = await getBoundWorkspaceFileSecretProvenance(workspaceId, source.identity) if (provenance.status === 'unknown') { - accumulator.markIncomplete() + accumulator.markIncomplete('workspace-file-provenance-unknown') continue } accumulator.record({ diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts index 3d41d848a93..9ca2188c414 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts @@ -144,6 +144,40 @@ describe('ResolvedSecretTraceProvenanceAccumulator', () => { accumulator.markIncomplete('unspecified') expect(accumulator.exportProvenance().entries).toEqual([]) }) + + /** + * The exported bundle carries only `complete`, so an importer can never say more than + * `source-provenance-incomplete`. If this line does not name the guard, nothing does. + */ + it('names the first guard that latched, and stays quiet for the rest of the invocation', () => { + vi.clearAllMocks() + const accumulator = new ResolvedSecretTraceProvenanceAccumulator(scope) + + accumulator.markIncomplete('file-source-unidentified') + accumulator.markIncomplete('workspace-file-provenance-unknown') + + expect(mockLogger.warn).toHaveBeenCalledTimes(1) + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Resolved secret provenance accumulator marked incomplete', + expect.objectContaining({ + reason: 'file-source-unidentified', + scopeWorkspaceId: 'workspace-1', + }) + ) + expect(mockLogger.error).not.toHaveBeenCalled() + }) + + /** A merge of already-reported bundles adds nothing; subflow aggregation runs it per iteration. */ + it('stays silent when a recorded report is what latched it', () => { + vi.clearAllMocks() + const accumulator = new ResolvedSecretTraceProvenanceAccumulator(scope) + + accumulator.record({ version: 1, complete: false, entries: [], scope }) + + expect(accumulator.exportProvenance().complete).toBe(false) + expect(mockLogger.warn).not.toHaveBeenCalled() + expect(mockLogger.error).not.toHaveBeenCalled() + }) }) describe('ResolvedSecretTraceRegistry', () => { @@ -1474,9 +1508,11 @@ describe('incompleteness diagnostics', () => { 'knowledge-result-provenance-unavailable', 'knowledge-response-capacity-exceeded', 'memory-crossing-capacity-exceeded', - 'workspace-scope-missing', 'table-result-provenance-unavailable', 'mounted-file-provenance-unavailable', + 'workspace-file-provenance-unknown', + 'file-source-unidentified', + 'mcp-tool-execution-timeout', 'table-snapshot-unsafe-for-mount', 'restored-provenance-untrusted', 'backfill-checkpoint-absent', diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.ts index 24d91499987..8556e8e38cf 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.ts @@ -43,6 +43,7 @@ export type ResolvedSecretIncompletenessReason = | 'durable-provenance-malformed' | 'tool-input-not-enumerable' | 'tool-params-transform-failed' + | 'mcp-tool-execution-timeout' | 'structural-input-projection-incomplete' | 'structural-input-root-unprojected' | 'mothership-provenance-invalid' @@ -60,17 +61,20 @@ export type ResolvedSecretIncompletenessReason = | 'knowledge-row-missing' | 'knowledge-row-content-mismatch' | 'memory-crossing-capacity-exceeded' - | 'workspace-scope-missing' | 'table-result-provenance-unavailable' | 'mounted-file-provenance-unavailable' + | 'workspace-file-provenance-unknown' + | 'file-source-unidentified' | 'table-snapshot-unsafe-for-mount' | 'restored-provenance-untrusted' | 'backfill-checkpoint-absent' | 'backfill-checkpoint-unusable' | 'log-creation-skipped' /** - * Only for a caller that has not been given a reason yet. A refusal reporting this names no - * guard, which is the state that made a production latch untraceable — prefer adding a literal. + * No production caller uses this, and none should: a refusal reporting it names no guard, which + * is the state that made a production latch untraceable. It survives for tests that need a + * latched registry and have no guard to name, where a borrowed real reason would read as a claim + * about which one tripped. A new caller wanting it wants a new literal instead. */ | 'unspecified' @@ -122,6 +126,23 @@ const BY_DESIGN_INCOMPLETENESS_REASONS = new Set +): void { + if (BY_DESIGN_INCOMPLETENESS_REASONS.has(reason)) return + if (ORIGINATING_FAULT_REASONS.has(reason)) logger.error(message, { reason, ...details }) + else logger.warn(message, { reason, ...details }) +} + /** * Origins are caller-supplied strings rather than a closed union, so they carry an explicit bound; * one run reaching this many distinct importers already tells the whole story. @@ -624,6 +645,7 @@ export function isResolvedSecretTraceProvenanceV1( export class ResolvedSecretTraceProvenanceAccumulator { private readonly scope?: ResolvedSecretTraceScopeV1 private provenance: ResolvedSecretTraceProvenanceV1 + private reportedGuard = false constructor(scope?: ResolvedSecretTraceScopeV1) { this.scope = scope ? cloneProvenanceScope(scope) : undefined @@ -677,9 +699,26 @@ export class ResolvedSecretTraceProvenanceAccumulator { return true } - /** Marks the invocation incomplete and discards entries that can no longer be trusted. */ - markIncomplete(): void { + /** + * Marks the invocation incomplete and discards entries that can no longer be trusted. + * + * `reason` is required for the same purpose it is on {@link ResolvedSecretTraceRegistry}, and + * matters more here: the wire format carries only `complete`, so the consumer that imports this + * bundle can only latch with `source-provenance-incomplete` and can never name the guard. This + * line is the sole record of which one tripped. + * + * Only the first guard reports. Later ones restate an invocation that already cannot vouch, and + * a caller walking a list of sources would otherwise emit a line per remaining source. A latch + * from {@link record} does not report at all: it reflects a bundle whose own registry already + * reported, so this would only restate it with less context. + */ + markIncomplete(reason: ResolvedSecretIncompletenessReason): void { this.provenance = this.emptyProvenance(false) + if (this.reportedGuard) return + this.reportedGuard = true + reportIncompleteness('Resolved secret provenance accumulator marked incomplete', reason, { + scopeWorkspaceId: this.scope?.workspaceId, + }) } exportProvenance(): ResolvedSecretTraceProvenanceV1 { @@ -1583,17 +1622,13 @@ export class ResolvedSecretTraceRegistry { if (!this.complete) return this.complete = false this.modelEgressRevision += 1 - if (this.staged || BY_DESIGN_INCOMPLETENESS_REASONS.has(reason)) return - const details = { - reason, + if (this.staged) return + reportIncompleteness('Resolved secret registry marked incomplete', reason, { ...(context.origin ? { origin: context.origin } : {}), scopeWorkspaceId: this.scope?.workspaceId, activeEntryCount: this.activeEntries.size, incompleteInputPathCount: this.incompleteInputPaths.size, - } - const message = 'Resolved secret registry marked incomplete' - if (ORIGINATING_FAULT_REASONS.has(reason)) logger.error(message, details) - else logger.warn(message, details) + }) } /** @@ -2038,17 +2073,13 @@ export class ResolvedSecretTraceRegistry { if (this.incompleteInputPaths.has(key)) return this.incompleteInputPaths.set(key, [...path]) this.modelEgressRevision += 1 - if (this.staged || BY_DESIGN_INCOMPLETENESS_REASONS.has(reason)) return - const details = { - reason, + if (this.staged) return + reportIncompleteness('Resolved secret input path marked incomplete', reason, { ...(origin ? { origin } : {}), inputPath: path.join('.'), scopeWorkspaceId: this.scope?.workspaceId, activeEntryCount: this.activeEntries.size, - } - const message = 'Resolved secret input path marked incomplete' - if (ORIGINATING_FAULT_REASONS.has(reason)) logger.error(message, details) - else logger.warn(message, details) + }) } private copyIncompleteInputPathsTo( diff --git a/apps/sim/lib/copilot/request/tools/client.ts b/apps/sim/lib/copilot/request/tools/client.ts index 88cf40cd35e..de792abd5fc 100644 --- a/apps/sim/lib/copilot/request/tools/client.ts +++ b/apps/sim/lib/copilot/request/tools/client.ts @@ -308,7 +308,9 @@ export async function waitForWorkflowToolCompletion({ !trustedExecution.provenance.complete ) { if (!trustedExecution.provenance.complete) - toolRegistry?.markIncomplete('source-provenance-incomplete') + toolRegistry?.markIncomplete('source-provenance-incomplete', { + origin: 'copilotToolClient.workflowExecution', + }) return structuralWorkflowCompletion( getWorkflowToolConfirmationStatus(trustedExecution.status), workflowId, @@ -328,9 +330,14 @@ export async function waitForWorkflowToolCompletion({ }, { trusted: true } ) - if (!imported) toolRegistry.markIncomplete('value-provenance-import-failed') + if (!imported) + toolRegistry.markIncomplete('value-provenance-import-failed', { + origin: 'copilotToolClient.workflowExecution', + }) } catch (error) { - toolRegistry.markIncomplete('value-provenance-import-failed') + toolRegistry.markIncomplete('value-provenance-import-failed', { + origin: 'copilotToolClient.workflowExecution', + }) logger.warn('Failed to import bound workflow provenance', { toolCallId, workflowId, diff --git a/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts b/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts index 8489ab66424..5058cfa3b2a 100644 --- a/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts +++ b/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts @@ -72,10 +72,21 @@ export function isDurableSecretProvenanceEnforced( return enforcedSurfaces.has(surface) } +/** + * What a surface could not vouch for. + * + * A closed union rather than a free-form string, for the reason the resolved-secret registry's + * reason set is one: a surface stays open on the strength of these lines trending to zero, and a + * cause that a call site can spell freely cannot be aggregated or alerted on. + */ +export type UnrecordedDurableProvenanceCause = + | 'durable-provenance-unknown' + | 'row-sidecar-not-exact' + | 'stored-memory-provenance-unknown' + export interface UnrecordedDurableProvenanceReport { surface: DurableSecretProvenanceSurface - /** What the surface could not vouch for, e.g. `sidecar-status-unknown`. Always a static literal. */ - cause: string + cause: UnrecordedDurableProvenanceCause /** How many records in this one read were unrecorded, when the caller reads a page at a time. */ affectedCount?: number workspaceId?: string