mirror of
https://github.com/simstudioai/sim.git
synced 2026-08-31 01:11:53 +08:00
improvement(provenance): make the incompleteness reason set closed and complete (#6559)
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.
This commit is contained in:
committed by
GitHub
parent
81e04a8e41
commit
ae1f62d5ef
@@ -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
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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<ResolvedSecretIncompletenessRea
|
||||
'log-creation-skipped',
|
||||
])
|
||||
|
||||
/**
|
||||
* Sole owner of the report level, shared by every latch that reports one.
|
||||
*
|
||||
* The registry, its input paths, and the accumulator each latch for their own reasons but classify
|
||||
* them identically, and a copy of the split per latch is a copy that can be updated alone — which
|
||||
* would let the same reason be a fault in one place and routine in another.
|
||||
*/
|
||||
function reportIncompleteness(
|
||||
message: string,
|
||||
reason: ResolvedSecretIncompletenessReason,
|
||||
details: Record<string, unknown>
|
||||
): 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(
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user