Repository navigation
Conversation
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
9f116b5 to
c2f6683
Compare
processFullResponseActions applies <dyad-write>/<dyad-delete>/etc. tags straight to disk (fs.writeFileSync, git commit) once a turn finishes. The only existing check before that call was an explicit user-abort check - nothing looked at the stream's own finishReason. A well-formed tag can be followed by a "length" cutoff, a content-filter stop, or a provider error, and none of those currently stop the tags collected so far from being applied with autoApproveChanges on. Adds src/ipc/utils/stream_execution_guard.ts, a thin wrapper around prefix-safe-json's createAiSdkExecutionGuard() that tracks one synthetic call representing "this response" and resolves it against the real finish/error/abort parts observed on fullStream. processStreamChunks now observes every raw part (previously only chunk-producing part types were inspected) and returns a streamSafety verdict alongside fullResponse; the final shouldAutoApply gate only calls processFullResponseActions once that verdict is confirmedSafe. The saved response text itself is unaffected - only the auto-apply side effect is withheld, so nothing is silently discarded from the visible/persisted transcript. Scope is intentionally narrow: only the shouldAutoApply call site in chat_stream_handlers.ts is changed; processFullResponseActions itself is untouched.
fullResponse is cumulative across the initial generation and any search-replace-fix / continuation retry streams within one turn, but the previous change tracked only the most recently resolved stream's safety verdict (lastStreamSafety = result.streamSafety on every retry). A stream that was cut off by a length limit, followed by a clean continuation on the same turn, would have its unsafe content laundered into an executable response: the earlier stream's unsafe verdict was simply overwritten by the later stream's safe one, even though both streams' output ended up in the same fullResponse that gets applied. combineStreamSafety() in stream_execution_guard.ts folds each stream's verdict into an accumulator with sticky-unsafe semantics: once any contributing stream is unsafe, the accumulated verdict for the turn stays unsafe regardless of how safely a later stream in the same turn completes. Only "every contributing stream safe" yields a safe accumulated verdict. chat_stream_handlers.ts's three processStreamChunks call sites now fold into accumulatedStreamSafety via combineStreamSafety instead of overwriting a single lastStreamSafety. Covered by new tests: combineStreamSafety unit tests for the fold semantics, plus four production-path tests driving two real processStreamChunks calls through the real accumulator and the real processFullResponseActions/fs.writeFileSync (unsafe-then-safe, safe-then-unsafe, safe-then-safe, and a three-stream sticky-unsafe case).
Was ^0.0.1-alpha.4, which npm's semver range treats as "no upper bound below the next alpha.N" and would resolve later 0.0.1-alpha.x prereleases without a corresponding review of this integration against them. Pin the exact version actually integrated against and validated. Resolved package is unchanged (0.0.1-alpha.4).
c2f6683 to
6b92633
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b92633529
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
12f1ee4 to
9a235af
Compare
The 0.1.0 entry said the package "had already been integrated against three independent real-world codebases (Dyad, CodePilot, Apache Maka) across separate PRs" - past tense, implying settled, completed integrations. Checked all three directly against their live PRs rather than repeating the claim: - dyad-sh/dyad#4341 and op7418/CodePilot#676 are both real and substantive - genuine PRs pinning [email protected] with real integration code and specific, named test suites - but both are still open, not merged. - apache/maka#3434 solves the identical problem (gating tool execution on raw stream completion, not just JSON validity) but its own PR description states it "adds no new runtime package, no prefix-safe-json dependency" - a Maka-owned native implementation. It should never have been grouped with the other two as a dependency adopter. None of the three amount to "integrated" in past tense, and open PRs are not package adoption regardless of how substantive the patch behind them is. Added a dated correction note directly under the original paragraph rather than editing it - the original text stays exactly as published, readable for what it said and when, with the correction attached immediately after it and sourced to the specific PRs it's about.
…release prefix-safe-json 0.0.1-alpha.4 -> 0.2.0 (exact). The createAiSdkExecutionGuard() API this integration uses is unchanged between the two, so no source changes were needed. Verified: resolved version is exactly 0.2.0, and stream_execution_guard.test.ts (24/24), chat_stream_handlers.test.ts (87/87), chat_stream_handlers.actor_terminal.test.ts (5/5), and response_processor.test.ts (11/11) all still pass unchanged, including the sticky-unsafe accumulation semantics and the canned-QA auto-apply preservation.
|
Replaced the old alpha dependency with the published prefix-safe-json 0.2.0 release and reran the stream-safety-guard and auto-apply suites against it. No architecture or behavior change — same sticky-unsafe semantics, same canned-QA auto-apply handling — this only removes the prerelease dependency. |
|
@canblmz1 - let's open an issue and discuss whether this fix is needed beforehand. i'm also concerned about adding a new dependency here (that I see you've published) as it introduces supply chain risk to our project. |
|
That makes sense. I moved too quickly from a possible boundary gap to proposing an implementation. I agree the correctness question and the dependency decision should be treated separately. I’ve opened #4383 to verify the behavior first against Dyad’s current main and real auto-apply path, without proposing any dependency. If the behavior is not reproducible, there is nothing to fix. If it is reproducible, I’d prefer to discuss the smallest local fix and regression test first; adding another runtime dependency should be a separate decision with its own supply-chain justification. |
Problem
When
autoApproveChangesis enabled, Dyad applies actions parsed from theaccumulated assistant response through
processFullResponseActions().Before this change, the final auto-apply gate checked whether the request had
been explicitly aborted, but it did not verify how the underlying model stream
terminated.
That means a response can contain a syntactically complete
<dyad-write>(oranother actionable Dyad tag) and still belong to a generation that later ends
because of a token limit, provider error, content filter, abort, or an
unrecognized terminal state.
A complete tag is not sufficient evidence that the surrounding generation
finished normally.
Fix
Track the termination state of each AI SDK
fullStreamconsumed while buildingthe response and require positive safe completion before automatic application.
The stream classifier is backed by
[email protected]'s AI SDKstream adapter, which normalizes terminal states fail-closed:
stop/ normal completion -> confirmed safelength-> unsafeThe integration intentionally uses the package here for its AI SDK
terminal-state normalization and fail-closed behavior. Dyad's
<dyad-*>actionsare plain-text response tags rather than JSON tool calls, so this change does
not claim to use the package's streamed JSON-completeness guarantees.
Accumulated responses
fullResponsecan contain output from more than one provider stream, includingsearch/replace repair and continuation attempts.
Safety is therefore accumulated with sticky-unsafe semantics:
later successful continuation does not restore automatic execution.
The generated response is still persisted and visible for manual review; only
the automatic filesystem/SQL/dependency side effect is withheld.
Scope
This patch only changes the existing response-level auto-apply path.
processFullResponseActions()itself is unchanged.Dyad's Local Agent tool-execution path is intentionally out of scope; it has a
different AI SDK multi-step execution lifecycle and would require a separate
design and test surface.
Tests
Added focused coverage for terminal-state classification and the real
auto-apply execution path.
Safety cases include:
Accumulated multi-stream cases include:
Validation performed:
stream_execution_guard.test.ts: 24/24 passingchat_stream_handlersauto-apply integration cases: 8/8 passingchat_stream_handlers.actor_terminal.test.ts: 5/5 passing(
chat_stream_handlers.test.ts,response_processor.test.ts,stream_execution_guard.test.ts):122/122 passing
own diff is dependency-only —
package.json/package-lock.json— sothere is no new source for oxlint to check; the original claim above
still stands unchanged)
The repository-wide TypeScript run currently reports 90 errors under
testing/fake-llm-server/*because that sandbox lacks the correspondingExpress/CORS type dependencies. The exact same error set was reproduced on
pristine upstream before attributing it to this change.
A broader suite comparison was also performed against pristine upstream.
Existing environment failures reproduced on both branches; two additional
full-suite timeout cases on the patched run passed when rerun independently and
were unrelated to the changed paths.
Dependency update
prefix-safe-jsonmoved from the0.0.1-alpha.4prerelease to the publishedexact release:
No source changes were required —
createAiSdkExecutionGuard()and theAiSdkExecutionGuard.push()/.finish()surface this integration uses areunchanged between the two. Re-verified against the real 0.2.0 install
(resolved version confirmed exactly
0.2.0, from the public npm registry,lockfile regenerated with
npm install): all of the above test counts areidentical to the
0.0.1-alpha.4result, including the sticky-unsafeaccumulation semantics and the canned
[dyad-qa=...]auto-apply preservationfrom the prior review round. No vendored or local copy is used.