fix(copilot): say whether a withheld tool call changed anything - #7178
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36664464 | Triggered | Generic High Entropy Secret | 0d88a11 | apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
Greptile SummaryThe PR makes withheld Copilot workflow results disclose whether execution was not attempted, attempted, or performed while preserving the secret-egress boundary.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/application/run-workflow-from-copilot.ts | Associates caught workflow failures with the child execution ID and contains recovery failures so the original marked error is preserved. |
| apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts | Maps workflow rejection and settlement outcomes to retry-relevant effect phases and execution IDs. |
| apps/sim/lib/copilot/request/tools/resolved-secret-result.ts | Preserves validated effect disclosures when content is withheld and reports structured withholding causes. |
| apps/sim/app/api/copilot/tools/execute/route.ts | Refuses execution before dispatch when no egress registry is available and logs unsafe projections consistently. |
| apps/sim/executor/utils/errors.ts | Adds identity-based attempted-execution ID storage that supports frozen and sealed thrown objects. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Copilot tool call] --> B{Egress registry available?}
B -- No --> C[Refuse before dispatch<br/>not_attempted]
B -- Yes --> D[Execute tool]
D --> E{Result projects safely?}
E -- Yes --> F[Return projected result]
E -- No --> G{Declared effect phase}
G --> H[Return withheld disclosure<br/>phase and validated execution IDs]
Reviews (15): Last reviewed commit: "docs(copilot): state that a named run ma..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
icecrasher321
force-pushed
the
fix/withheld-run-effect-disclosure
branch
from
August 27, 2026 22:33
3147be5 to
978cc36
Compare
icecrasher321
force-pushed
the
fix/withheld-run-effect-disclosure
branch
from
August 27, 2026 22:46
978cc36 to
9cf9a89
Compare
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
icecrasher321
force-pushed
the
fix/withheld-run-effect-disclosure
branch
from
August 27, 2026 23:05
bdda9f4 to
671f88e
Compare
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ue sentinel
A tool result the egress projection cannot vouch for is reduced to a bare
success or to `TOOL_RESULT_UNAVAILABLE_ERROR`. Both drop the execution id
with the payload, and the sentinel also overwrites the real error text, so
a call rejected on its own arguments and a run that already executed come
back byte-identical. Those need opposite retry decisions.
Reproduced against real code by latching a registry the way production
latches one — a child run that returned no provenance envelope — and
driving the real handler and the real projection: a pre-dispatch rejection
and a post-dispatch failure were identical, and a completed run arrived as
`{"success":true}` with nothing to look it up by. Two distinct shapes for
three outcomes.
The registry is right to fail closed; the boundary was discarding facts it
never needed to redact. A tool may now declare a `ToolCallEffect` — a phase
and server-minted ids — which the projection preserves when it withholds
content, because neither is derived from that content. The exemption is
enforced rather than asserted: ids must match the identifier shape this
system mints, and one that does not voids the whole disclosure.
The phase is attached in the application layer from dispatch onward and
nowhere earlier, which is what makes the id's absence the positive
statement that nothing was created rather than an admission of not
knowing. Withholding also now reports its cause — a latched registry names
the guard that tripped, an absent one means no catalog was built, and a
content refusal means the registry was fine — so the next occurrence is
diagnosable from the logs it already writes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The withheld-run fixture was shaped like a live provider key, which is exactly what a secret scanner is built to catch — it flagged the test file itself. The value only has to clear the eight-character substitution floor, so it says what it is instead. The id-shape guard likewise no longer needs a credential-looking string to prove it refuses one. Also routes the test's error-message mock through getErrorMessage rather than reimplementing it inline, which check:utils bans. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found the attempted-run id attached around the whole of executeWorkflow, which validates workspace and billing attribution before it can create anything. A preflight refusal therefore reported a run that never existed, telling a caller to resolve an id with nothing behind it and to skip a retry that was safe — the mirror of the defect this branch fixes. Move the attachment inside executeWorkflow, at the point it enters the execution core, which is the first moment a row may exist. Everything above it now correctly carries nothing, and the copilot layer keeps only the window executeWorkflow cannot see: a failure after the run already returned, where the crossing import threw and an execution certainly exists. Also from review: - Attach to any thrown object rather than only an Error, and normalize a thrown primitive past the dispatch boundary. Restricting to Error made the invariant silently invert for a thrown plain object — the id would not attach, its absence would read as "nothing started", and the caller would duplicate a real run. - Void the disclosure when an id would take one of the record's own field names. A valid uuid under `effect` overwrote the phase the retry decision reads, on the same all-or-nothing terms as an unvouchable id. - Drop `effect` from the provider model response. That path spreads every non-output field through verbatim, so the type's claim that the disclosure reaches the model only through the withheld-result projection was true by accident rather than by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vailable Production shows 88 of these in fourteen days, every one from this route and every one caused by a workspace id that no longer exists reaching the in-band lane. The handler ran anyway, which is the worst pair of outcomes available: the side effect happened, and because the projection can vouch for nothing without a catalog, the caller got a bare success or an opaque sentinel naming neither the cause nor whether anything had changed. It is also where the reported "cannot tell whether the mutation occurred" came from — of the tools affected, read and grep dominate, and the runs were bursts inside single sessions. Refuse before dispatch instead. Nothing runs, so there is nothing to be uncertain about, and the caller is told which workspace and why. A missing workspace also reported itself as an access denial, which sent every deleted-workspace call down a permissions path nobody could reproduce. `checkWorkspaceAccess` already distinguishes the two, so say which one it was. The refusal log now carries the user and workspace it refused; without them the only way to find the cause was to join by timestamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he core Review was right that entering the execution core is too early. The core loads custom blocks, workflow state, and the environment before `safeStart` writes a row, so a setup failure — which ran nothing and is safely retryable — still reported a dispatched run and sent the caller looking for it. Move the marker to `loggingStarted`, read before the catch's own recovery `safeStart` writes a row for the failure itself. That is the first point blocks may have executed, so it is the honest line, and it lets executeWorkflow go back to a plain rethrow. The thrown value stays exactly as received, including a non-Error one: the core's finalization guard identifies it, and three existing tests pin that. A thrown primitive therefore carries no id, which costs nothing today because every throw site past `safeStart` raises an Error — noted in the code rather than papered over. Also read the marker with `Object.hasOwn` rather than `in`, so an id reached through a prototype chain can never disclose an unrelated run, and cover the reserved-key branch of the disclosure guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uccess The cause was written only on the failure branch, but a withheld success keeps `projected.success` true — so the one case that leaves no other trace, where the model reads a bare success and nothing says why, was also the only one whose cause was never recorded. Report it on its own, as the resume driver already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found the logging session wrong in both directions, and it is: the result of `safeStart` is never checked, so blocks execute even when it fails — reporting that nothing started for a run that did, which is the direction that duplicates work — and it flips before trigger resolution and serialization, reporting a run for failures that never reached a block. A resume whose conditional update matches no row returns true and named a run that does not exist. Entering the executor is the only honest answer to "could a side effect have occurred", because side effects come from blocks rather than from log rows. Moving the marker there settles all three at once. Also from review: - Stop returning the thrown environment or database error to the model when the egress catalog is unavailable. Nothing there can project it — the catalog it would need is the very thing that is missing — so the reason stays in the log and the response carries fixed text plus the workspace id the caller itself supplied. - Guard the attach against a frozen failure, which would otherwise throw and replace the original error partway through cleanup, making a diagnostic aid the thing that loses the diagnosis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iting to it Guarding the write was trading one failure for the worse one: a frozen or sealed error kept the process alive but dropped the marker, which turns "this run exists" into "nothing started" — the single direction that duplicates work. Record the id in a WeakMap keyed by the thrown value, the same shape markExecutionFinalizedByCore already keeps for the same reason. Nothing is written to the error, so a non-extensible one is recorded like any other and there is no throw to guard. Identity keying also retires the prototype-chain concern, and the error's own surface stays clean, so a serialized failure no longer carries a stray field. A thrown primitive still cannot be keyed, which costs nothing today because every throw site past the dispatch boundary raises an Error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review was right that entering `execute` is still too early: DAG construction, snapshot restoration and pipeline assembly all happen inside it and reject a malformed graph having changed nothing, so a validation failure reported a run to resolve. Only the executor knows where that line falls, so it reports it. A `onBlocksMayRun` context extension fires immediately before `engine.run` on both entry points, and execution-core records the run from there rather than guessing at it from outside. A rejected graph now correctly says nothing started. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…erase it Two findings, both real. Firing before `engine.run` was still one step early: the engine's cancellation subscription is fallible and rejects having run nothing, so that failure claimed a run. The signal now fires inside the engine, immediately before the loop that processes blocks and past every startup step that can refuse a request — DAG construction, pipeline assembly and the subscription. The executor no longer guesses at the line from outside; the engine states it. Separately, the copilot catch path could throw while recording the failed crossing or releasing the execution slot. Either one propagated a different error — one the dispatched-run id was never recorded against — so an existing run reported itself as never started and invited the duplicate the id exists to prevent. Both are recovery work and neither may replace the failure it is describing, so both are contained and logged. Contained with try/catch rather than a rejection handler, since a synchronous throw has to be caught too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Once `executeWorkflowCore` returns, the run happened. Everything after it in `executeWorkflow` — analytics, pause persistence, post-execution settling — is bookkeeping that can still throw, and the core's own catch no longer runs, so those failures named no run. The copilot handler then reported `not_attempted` for an execution that had already produced side effects, which is the one direction that duplicates work. Mark it as soon as the core settles, so any later failure carries it. The `finally` had the same shape and is now contained: a throw there replaces whatever the function was about to do, turning a successful run into an error or an error that names its run into one that does not. Settling post-execution work is bookkeeping and must not be able to do either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`performed` claims the run reached the end of its work, so a caller reads it as "never retry, just read the outcome". Every returned result carried it, including a cancelled or paused one — which stopped partway and may have run every block, one, or none. Those are `attempted`: an execution exists under this id, resolve it before deciding anything. That is true whether the cancellation landed before the first block or after the last. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e result's shape Nine review rounds found the same class of defect, which makes it a design problem rather than nine bugs. "Did a side effect occur" had two sources that disagreed: a precise marker on the thrown path, and on the returned path an inference from whatever the outcome happened to look like. Every property used for that inference is a proxy that breaks on the paths that matter — an engine failing before its first block still carries an ExecutionResult, and a run that ends without one still ran every block it had — so each round found another path where the proxy lied. There is now one source. The engine reports the moment a block handler is first about to run, which is terminal: no fallible step remains between it and the handler, so there is nothing left for a later reviewer to find in front of it. The signal is threaded to the caller and recorded against the outcome, and the copilot adapter reads it on every exit path instead of inspecting status or the presence of an attached result. The phase then follows from two stated facts rather than a guess: nothing dispatched is not_attempted whatever the result looks like, a run that stopped partway is attempted, and one that reached the end is performed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I claimed last round that nothing could precede the signal. That was wrong: `executeNode` returns early on a cache hit, initializes loop and parallel scopes, and handles a sentinel that never reaches a handler — all after the point it fired. Both reviewers found the same thing. Move it to the line before `blockExecutor.execute`, which is the handler call. Nothing separates the two, so unlike every previous position this one cannot have something in front of it. Fired per block rather than once, since observers record a boolean and repeats cost nothing. Also accept functions as carriers of the run markers. They key a WeakMap exactly as objects do, so excluding them dropped the record for a thrown function and lost the distinction the markers exist to make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n the executor Ten review rounds chased the same question — when exactly may a side effect have occurred — through six positions in the executor, ending with a callback on every block of every execution in the product. Against 543k executions a week, serving a disclosure read about fifty times a week. The precision was never the point: `attempted` and `performed` both mean an execution exists under this id, and the caller was already handed the id that resolves it. Revert all of it. The engine, the orchestrator, both context types and the callback threading through execute-workflow and execution-core go back to staging untouched; the executor's only remaining change is the id carrier in utils/errors.ts. The phase now comes from what the copilot layer already holds. Its `try` opens on the executor call, so everything it catches is post-dispatch by construction while authorization, admission and provenance export throw past it having created nothing — no id means nothing exists, an id means resolve it. A result in hand says how the run ended, which separates cancelled and paused from completed. The harness that motivated this is now in the diff: every outcome the run path can produce, driven through the real handler and the real projection, asserted on the retry decision a caller can reach and on no run content crossing. Six mutations were used to confirm it fails for the right reasons; one of them found the dispatch flag this refactor introduced was already dead, and it is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both reviewers read an id on a preflight failure as a defect. It is the one place this contract is deliberately coarse, so say so where each of them was looking rather than leave it to be rediscovered. `attempted` already means "zero or one executions exist under this id" — the id is a correlation key, not a promise that a row exists. A caller resolves it, finds nothing, and retries, which is the right outcome at the cost of one lookup. Buying that lookup back means an executor-side dispatch marker: a callback on every block of every execution in the product, which this branch just reverted for that reason. It would also gain nothing, since all four preflight throws are invariant violations — no workspace id, no billing attribution, no principal, attribution mismatch — and a retry fails identically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
icecrasher321
force-pushed
the
fix/withheld-run-effect-disclosure
branch
from
August 28, 2026 03:23
ba86210 to
4d2ba50
Compare
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What a caller saw
A Copilot tool whose result the secret-egress boundary could not vouch for came back as either a bare
{"success": true}with no payload, or one fixed sentence:Both drop the execution id along with the payload, and the sentence also overwrites the tool's real error. So a call rejected on its own arguments — nothing created — and a run that had already executed came back byte-identical. No caller could build a retry policy from that: retrying risks duplicate side effects, not retrying silently drops the work.
Reproduced before designing anything
A test latches a real
ResolvedSecretTraceRegistrythe way production latches one (a child run that returned no provenance envelope) and drives the real handler through the real projection:{"success":false,"error":"…could not be returned safely…"}effect: "not_attempted"effect: "attempted"+executionId{"success":true}, no ideffect: "performed"+executionIdNo run content crosses the boundary in either column.
Two changes
1. A withheld result now states how far the call got.
A tool may declare a
ToolCallEffect— a phase (not_attempted/attempted/performed) plus server-minted ids — which the projection preserves when it withholds content, because neither is derived from that content. The exemption is enforced rather than asserted: ids must match the identifier shape this system mints, may not take one of the disclosure record's own field names, and a single violation voids the whole disclosure.The phase is recorded at the point a block could first have run — immediately before
executorInstance.execute— so its absence is the positive statement that nothing started rather than an admission of not knowing. It lives in aWeakMapkeyed by the thrown value rather than written onto it, matching whatmarkExecutionFinalizedByCorealready keeps nearby, and for the same reason: a thrown value is not reliably writable.2. The in-band route no longer runs tools it cannot report on.
This is where the reported symptom actually originated. Over fourteen days in production, every occurrence of an unavailable egress catalog came from
/api/copilot/tools/execute, and every one was a workspace id that no longer exists reaching that lane — concentrated in a small number of workspaces. The route logged a warning and then ran the tool anyway, which is the worst pair of outcomes available: the side effect happened, and because the projection can vouch for nothing without a catalog, the caller got a bare success or the sentinel.It now refuses before dispatch. Nothing runs, so there is nothing to be uncertain about, and the response names the workspace. The thrown reason stays in the log — it is an environment failure that nothing here can project, since the catalog that would vouch for it is the very thing missing.
A missing workspace also reported itself as an access denial, sending every deleted-workspace call down a permissions path nobody could reproduce.
checkWorkspaceAccessalready distinguishes the two, so it now says which.Telemetry
Withholding reports its cause on the warning and span it already wrote:
registry-incomplete(carrying the closed reason and origin enums that name the guard which tripped),registry-absent, orcontent-refused— three causes with three different fixes. A withheld success is now reported as well; it previously logged nothing, which made the one case that leaves no other trace also the only one with no record of why.For the reviewer
apps/sim/lib/workflows/executor/execution-core.tsis the file to read closely. The change is a single flag set immediately before the executor runs, but it sits on the path of every workflow execution, not only Copilot ones.Verification
tscclean; 37 repo audits passGitGuardian flags a test fixture in an early commit's diff — a synthetic string, never a real credential, already replaced and absent from the final tree.
🤖 Generated with Claude Code