fix(provenance): record why a resolved-secret registry became incomplete - #6478
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Each guard that trips incompleteness now passes a static reason (e.g. Staged temporary registries ( New tests mock the logger and cover log levels, reason attribution, decrypt summarisation, and absence of secret material in log payloads. Reviewed by Cursor Bugbot for commit 8e94bf8. Configure here. |
Greptile SummaryThe PR adds structured diagnostics explaining why a resolved-secret trace registry becomes incomplete without changing its fail-closed behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/utils/resolved-secret-trace-registry.ts | Adds reason-aware incompleteness diagnostics, staged-log suppression, and aggregate decryption-failure reporting while preserving registry state transitions. |
| apps/sim/executor/utils/resolved-secret-trace-registry.test.ts | Adds coverage for reason attribution, severity selection, staged suppression, import-level summarization, and avoidance of logging secret values or names. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Provenance or projection guard] --> B{Registry staged or incompleteness by design?}
B -->|Yes| C[Update incomplete state without summary log]
B -->|No| D{Originating fault reason?}
D -->|Yes| E[Log error with static reason and counts]
D -->|No| F[Log warning with static reason and counts]
C --> G[Fail closed for later projection]
E --> G
F --> G
H[Entry decryption failures] --> I[Aggregate failure count and first error]
I --> J[Emit one detailed import summary]
Reviews (7): Last reviewed commit: "fix(provenance): record why a resolved-s..." | Re-trigger Greptile
|
@cursor review |
0e63f98 to
02bd1f7
Compare
|
@cursor review |
02bd1f7 to
0fc18f7
Compare
0fc18f7 to
8f0cd3c
Compare
|
@cursor review |
8f0cd3c to
9e0b859
Compare
|
@cursor review |
9e0b859 to
fbc6521
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fbc6521. Configure here.
Incompleteness is one-way: once any guard trips, every later model projection in the run fails and the user is left with a single opaque sentence. Every guard could set it and none recorded which, in a file that imported no logger at all, so the cause could not be recovered after the fact. Name each guard with a static reason literal. Originating causes log at error because they permanently fail the run and error is the only level that survives every default the logger falls back to; reasons that merely carry an upstream fault forward log at warn so one fault does not read as several. The decrypt catch no longer discards its cause. No behaviour change. Reasons are static literals and the logged input path is block/field names; no resolved value is recorded.
fbc6521 to
8e94bf8
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8e94bf8. Configure here.
… refusal A refusal fails closed and reaches the user as one fixed sentence. The guard that caused it may have tripped many frames — or a whole process — earlier, and the incompleteness latch is one-way, so by then the causing call has long returned. #6478 recorded the reason when the guard tripped, but marking early-returns once a registry is already incomplete, so a run that inherits an incomplete registry refused with nothing recorded anywhere. That is the case production actually hits. Retain reasons on the registry and report them where the refusal happens. The reason is recorded before the already-incomplete return so a causal chain accumulates, and before the silence checks so a by-design origin that logs nothing when marked is still nameable at refusal. Propagation inherits through markIncomplete's source argument, and copying incomplete input paths carries their reasons, so a fork cannot latch without its cause. Route all 68 refusal sites through one choke point that logs boundary, cause, input path and workspace before throwing. It returns never, so callers still narrow; messages and thrown types are unchanged at every site. Records carry a cause discriminator, since a latched registry and a caller-side cross-check of the projection's own output both arrive here and only the former has reasons. No behaviour change.
… refusal (#6483) * fix(provenance): report why a projection was refused, at the point of refusal A refusal fails closed and reaches the user as one fixed sentence. The guard that caused it may have tripped many frames — or a whole process — earlier, and the incompleteness latch is one-way, so by then the causing call has long returned. #6478 recorded the reason when the guard tripped, but marking early-returns once a registry is already incomplete, so a run that inherits an incomplete registry refused with nothing recorded anywhere. That is the case production actually hits. Retain reasons on the registry and report them where the refusal happens. The reason is recorded before the already-incomplete return so a causal chain accumulates, and before the silence checks so a by-design origin that logs nothing when marked is still nameable at refusal. Propagation inherits through markIncomplete's source argument, and copying incomplete input paths carries their reasons, so a fork cannot latch without its cause. Route all 68 refusal sites through one choke point that logs boundary, cause, input path and workspace before throwing. It returns never, so callers still narrow; messages and thrown types are unchanged at every site. Records carry a cause discriminator, since a latched registry and a caller-side cross-check of the projection's own output both arrive here and only the former has reasons. No behaviour change. * fix(provenance): stop registry-less refusals deduplicating across requests A refusal with no registry aborts the request rather than iterating, so it reaches the reporter at most once per request. Remembering it for the process silenced every later request, including the one being investigated. * improvement(provenance): inherit copied-path reasons once per copy Semantics are unchanged — reasons are still inherited only when at least one incomplete path was actually copied — but the source set is walked once rather than once per copied path.
Why
ResolvedSecretTraceRegistryfails closed: when it can no longer vouch for what it projects, it marks itself incomplete. That state is one-way —isPermanentlyIncomplete()never resets — so a single trip fails every later model projection in the run, and the user gets one opaque sentence (... model input could not be safely projected) with no recourse.Many separate guards can set that state, and none of them recorded which one fired.
resolved-secret-trace-registry.tsimported no logger at all, so after the fact there was no way to tell a genuine containment from a matcher that simply could not decide.What this changes
No behaviour change. Each guard now names itself with a static reason literal:
untrusted-provenanceimportProvenancerejects an untrusted/malformed bundlesource-provenance-incompleteentry-decrypt-failedcatch {}that discarded the causeunverified-resolved-entryprojection-mismatchunresolved-placeholderprovenance-capacity-exceededrestored-checkpoint-unavailabletool-call-scope-mismatchconstructed-incompleteinherited-incomplete-*(×2)value-provenance-*(×3)Log level
Three levels, defaulting to the safe one:
projection-mismatch,entry-decrypt-failed,unverified-resolved-entry,unresolved-placeholder,provenance-capacity-exceeded,tool-call-scope-mismatch,untrusted-provenance, and the twovalue-provenance-*import guards. None is reachable on a healthy run.unspecifiedfrom the ~50 call sites outside this file that have not been audited yet.constructed-incomplete.createIncompleteResolvedSecretTraceRegistrystates outright that no trusted catalog was available; it carries nothing actionable.Defaulting to warn is deliberate. Incompleteness is the designed state wherever there is no catalog to vouch with, and those paths are hot —
background/webhook-execution.ts:586builds an incomplete registry on every webhook execution before replacing it with the real one. Logging that at error would have put a per-webhook error line on a completely healthy path. A reason added later without thought now stays quiet rather than paging someone.Error still matters for reach:
getMinLogLevel()falls back toERRORunderNODE_ENV=productionand undertest, andhelm/sim/templates/deployment-app.yamlsets noLOG_LEVELat all, so on a default self-hosted chart only error survives. The reasons that indicate a real fault — includingprojection-mismatch— are the ones that get it.Decrypt failures are summarised once per import rather than once per entry: one rotated key fails every entry, and a bundle may carry up to
MAX_PROVENANCE_ENTRIESof them.No secret material is logged
Reasons are static literals, the logged
inputPathis block/field names, and the remaining fields are cardinalities. The decrypt path recordsgetErrorMessage(error)and whether the entry was named — never the value, the ciphertext, or the entry name. There is a test asserting the secret value and name are absent from the log record.The failure mode this was built to identify
Driving the real classes reproduces the latch:
A secret whose plaintext is very short (a flag, an emoji, a two-letter code) binds to an input path where those same characters occur only incidentally. The resolver records that path's projection as the literal; the handler's transform records it as a placeholder. Same raw value, different projected value, so
recordStatemarks the path incomplete — and incompleteness is one-way, so the rest of the run's model projections fail.recordResolvedInputProjectionis worth noting: it overwritesrawValue/projectedValuewithout the mismatch guard, so only the handler's laterrecordStatecan observe the disagreement.The behavioural fix is intentionally not in this PR. #6416 solved this class for the logging path with
MIN_UNANCHORED_MATCH_LENGTH+ word-boundary anchoring, and the same policy plausibly belongs where short values bind to input paths. Butresolved-secret-matcher.tsdocuments its wider detection set as deliberate — "a narrow policy can never talk one of them out of failing closed" — so narrowing the wrong guard would convert a fail-closed into a silent leak. That change needs the author's intent and evidence of which guard actually fires, which is exactly what this PR produces.Type of Change
Testing
Registry suite 66/66. Nine new tests cover the error level for genuine faults, the warn default for propagated/unaudited reasons, silence for the by-design construction, the named reason replacing
unspecified, the split between a scope mismatch and an already-incomplete merged child, single-report attribution for an arriving-incomplete bundle, once-per-import decrypt summarisation, and the absence of secret material in the record. Each was verified to fail when its specific change is reverted.Wider run across
executor,lib,background,app/api,providers,tools: 15,486 passing, 0 test failures. Typecheck and biome clean. (83 suites fail to collect in my worktree from a local tailwind v4/v3 resolution artifact — byte-identical list on a clean tree with this change stashed, so unrelated to this PR.)Checklist