fix(ai): bind terminal safety-stop authority to adapter provenance (#4777 P1) - #4782
Conversation
9eceb13 to
70196fc
Compare
![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: 9eceb1314a
ℹ️ 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".
| const finished = await finishResponse(); | ||
| sanitizeProviderSafetyStopProvenance(finished); |
There was a problem hiding this comment.
When a custom stream completes by exhausting its iterator (for example, calling EventStream.end(finalMessage)) without yielding a done or error event, this sanitizer is never reached: execution instead uses the trailing-result path at lines 3524-3528, which still passes the result directly through managedAssistantShell or returns it unchanged. A forged errorKind: "provider_safety_stop" can therefore survive into session state and persistence and suppress downstream retry behavior, leaving the provenance bypass this change is intended to close; apply the same provenance sanitization to the trailing result before rebuilding or returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70196fc333
ℹ️ 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".
| message.stopReason === "error" && | ||
| message.errorKind === "provider_safety_stop" && | ||
| !isProviderSafetyStopAuthenticated(message) | ||
| ) { | ||
| delete message.errorKind; |
There was a problem hiding this comment.
When a custom stream returns a frozen message or a Proxy whose deleteProperty trap rejects deletion, this delete throws before managedAssistantShell can perform its guarded reconstruction. In managed-fallback runs, an unauthenticated safety-stop payload can therefore abort the entire run instead of having its forged label stripped and advancing the fallback chain; sanitize through a mutable rebuilt message or guard the mutation.
Useful? React with 👍 / 👎.
70196fc to
2e0223a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e0223a8b3
ℹ️ 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".
| export function applyProviderSafetyStop(message: AssistantMessage, signal: string): boolean { | ||
| if (!STRUCTURED_REFUSAL_SIGNALS.has(signal)) return false; | ||
| authenticatedProviderSafetyStops.add(message); | ||
| message.errorKind = "provider_safety_stop"; |
There was a problem hiding this comment.
Because this function is publicly exported both from the package barrel and through the existing ./utils/* package export, any custom StreamFn can import it, call applyProviderSafetyStop(message, "refusal"), and obtain the same WeakSet authority as a first-party adapter. managedRetryableFailure then accepts the forged mark and suppresses the configured fallback chain, so the provenance bypass this commit is intended to close remains available to custom stream implementations; the mint needs a capability or module boundary unavailable outside the built-in adapters.
Useful? React with 👍 / 👎.
2e0223a to
a1346f4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1346f4e7a
ℹ️ 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".
| message.stopReason === "error" && | ||
| message.errorKind === "provider_safety_stop" && | ||
| !isProviderSafetyStopAuthenticated(message) |
There was a problem hiding this comment.
When an unmanaged custom StreamFn completes normally with stopReason: "stop" but includes an unauthenticated errorKind: "provider_safety_stop", this guard leaves the forged field on the committed message. The coding-agent session then checks that field without requiring an error in #checkCompaction (agent-session.ts lines 15725-15731), so a successful near-limit response can incorrectly skip threshold compaction and make the next request overflow; strip unauthenticated labels independently of stopReason.
Useful? React with 👍 / 👎.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 4 actionable issues, led by Public API exposes terminal-authority minting and Public API permits forged safety-stop authority. These findings require changes before approval.
Findings / Required Changes
- [P1] Public API exposes terminal-authority minting.
Reference:packages/ai/src/index.ts:57
applyProviderSafetyStopis re-exported publicly, so custom streams or untrusted consumers can call it with a recognized signal and suppress fallback. Keep minting internal to first-party adapters or require an unforgeable private capability. - [P1] Public API permits forged safety-stop authority.
Reference:packages/ai/src/utils/provider-safety-stop.ts:58-63
applyProviderSafetyStopis exported and accepts anyAssistantMessageplus a known signal, so custom integrations can mint the terminal mark despite documentation claiming only first-party adapters can do so; make minting adapter-private or require an unforgeable internal capability, while exposing only verification/transfer as needed. - [P1] Public safety-stop minting bypasses provenance boundary.
Reference:packages/ai/src/utils/provider-safety-stop.ts:52
applyProviderSafetyStopis exported and re-exported from the package index, so untrusted integrations can call it with an allowlisted signal and make arbitrary messages terminal. Make minting private or require an unforgeable adapter-only capability; expose only verification and transfer publicly. - [P1] Trailing completion bypasses provenance sanitization.
Reference:packages/agent/src/agent-loop.ts:3530-3533
The trailing finishResponse path calls managedAssistantShell without sanitizeProviderSafetyStopProvenance, so a forged errorKind provider_safety_stop can be copied and transferred as authenticated. Sanitize the finished message before the trailing shell, and add a regression test for streams ending without done/error.
CI / Verification
- Reviewed the exact remote head:
a1346f4e7a9c299be02d7dfbe14230668f25384c. - CI summary: 19 passing, 3 failing, 11 pending/cancelled/skipped.
- Failing checks:
PR contract bootstrap,Validate exact-head PR contract,Validate exact-head PR contract. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The adapter boundary is not trust-isolated: public access to the provenance mint lets consumers forge terminal provider safety stops. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness risk established: ordinary terminal exits enforce provenance, but the trailing completion path can bypass it; no separate concurrency race was established. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | A3 risk established: the exported mint function lets untrusted consumers forge authenticated terminal provider stops and suppress fallback. |
| A4. Verification / Tests / CI | APPROVED | A4 verification review found passing affected tests but incomplete contract and platform validation; no observable code regression was established. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation support the intended fallback behavior, but the public minting API leaves a trust-boundary risk for custom integrations; Windows compatibility remains unverified. |
Limitations
- PR contract checks are failing in the brokered CI summary, so repository contract compliance cannot be established; affected-path security tests are reported successful.
- PR contract failures have no brokered diagnostic output, so their root cause cannot be determined.
- Windows/platform regressions are not established because the relevant CI jobs were skipped.
- Windows platform behavior cannot be established because the Windows dev:doctor/session-path and Windows safety jobs were skipped.
- The exact-head PR contract cannot be validated because both PR contract validation jobs failed.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
First review at exact head a1346f4e — merge blocked. This closes the residual I documented on #4778 on the normal path, and the design is the right one. One stream exit escapes the sanitizer, and the PR body claims otherwise.
the #4778 residual is genuinely closed on the normal path
agent-loop.ts:359-378 returns false for stopReason === "error" plus errorKind === "provider_safety_stop" plus authenticated provenance, before classifyFallbackTrigger at :374. Both discard gates call it (:2571-2577, :2789-2795).
Binding the kind to minted provenance rather than trusting the field is a better answer than what I asked for. Provenance is minted adapter-side (provider-safety-stop.ts:62-66) and transferred across the managed shell (:1264-1272), so a bare errorKind string on a message is no longer authority by itself.
An authenticated safety stop paired with a retryable class now survives, which was the exact reachable gap I described: provider-safety-stop-hint.e2e.test.ts:183-221 covers 429, asserts one call, zero model_fallback_switched, and the typed assistant retained in session.state.messages. managed-attempt-transaction.test.ts:169-213 covers retryable 500 separately.
No enum or mapping change (types.ts:684-685), and packages/ai/src/models.json is absent from the changed-file list — correct, since it is generated.
major — the trailing stream exit is not sanitized
:3505-3509 sanitizes inside case "done": case "error"::
const finished = await finishResponse();
sanitizeProviderSafetyStopProvenance(finished);:3530-3534 does not:
const trailing = config.fallbackManaged
? managedAssistantShell(await finishResponse(), config.model, managedDegradedFieldDiagnostics)
: await finishResponse();AssistantMessageEventStream supports end(result?: R) (event-stream.ts:183), so a stream can emit start then end(forgedMessage) and reach this branch without ever yielding a done or error event. A forged provider_safety_stop then survives, avoids managed discard because status 400 classifies as other, and suppresses fallback through AgentSession's field-only gate (agent-session.ts:17774-17782).
That is forged authority reaching a trust decision — the thing this PR exists to prevent, on the one exit its sanitizer does not cover.
Fix: sanitize the result before the trailing rebuild, same as the done/error branch. Then pin it with a start + end(message) stream carrying forged provenance.
the body overstates it
The body says every stream exit strips forged labels and that both forged 400 and 429 advance. The trailing exit is a counterexample, and the forged test at provider-safety-stop-hint.e2e.test.ts:223-276 only uses a terminal error event — so it does not cover the start + end(result) shape.
I flag body claims specifically because that field is what a reviewer and the merge gate read to decide whether re-review is needed. Please correct it alongside the fix.
Reviewed by @probepark — method: confirmed the new provenance binding sits ahead of transport classification, then enumerated the stream exits to check which reach managedAssistantShell without sanitization, and verified end(result) is a supported shape rather than assuming the done/error branch is the only path.
gajae.pr-review-verdict.v1 merge-blocked sha256:8928b6e33fa283b9b9b836e6b0d348adf3fed25f5d7f405fe367c2aa8e13ab2c reviewer:human reviewer-id:probepark evidence:exact-head-a1346f4e-normal-path-closes-the-4778-residual-but-the-trailing-stream-exit-rebuilds-without-sanitizing-forged-provenance
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 376a2dfdbb
ℹ️ 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".
| export const PROVIDER_SAFETY_STOP_ADAPTER_CAPABILITY = Object.freeze({ | ||
| [PROVIDER_SAFETY_STOP_ADAPTER_BRAND]: true, | ||
| }) as ProviderSafetyStopAdapterCapability; |
There was a problem hiding this comment.
A custom StreamFn can still import @gajae-code/ai/adapter-internals/provider-safety-stop, because the existing "./*": "./src/*.ts" package export also matches nested subpaths, and obtain both this capability and mintProviderSafetyStop. Fresh evidence at this head is that moving the mint from the public utility into adapter-internals did not make it private—the package subpath resolves through that wildcard—so a forged message can still be authenticated and suppress the configured fallback chain; explicitly block this subpath or place the mint outside every exported pattern.
Useful? React with 👍 / 👎.
| [PROVIDER_SAFETY_STOP_ADAPTER_BRAND]: true, | ||
| }) as ProviderSafetyStopAdapterCapability; | ||
|
|
||
| const authenticatedProviderSafetyStops = new WeakSet<object>(); |
There was a problem hiding this comment.
When model.transport is "pi-native", the first-party adapter runs in the auth-gateway process, while pi-native-server.ts rebuilds and JSON-serializes its terminal event and pi-native-client.ts parses a fresh object. This process-local WeakSet mark therefore never reaches the agent loop, which strips a legitimate Anthropic/OpenAI/Google safety label as unauthenticated; with transport facts such as a 429 or 5xx, the refusal can then advance the fallback chain instead of remaining terminal, and the manual-switch hint is lost. The trusted gateway path needs a provenance representation that can be re-authenticated at the client boundary.
Useful? React with 👍 / 👎.
376a2df to
cd29404
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd2940488f
ℹ️ 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".
| export function transferProviderSafetyStop(from: unknown, to: AssistantMessage): void { | ||
| if (typeof from === "object" && from !== null && authenticatedProviderSafetyStops.has(from)) { | ||
| authenticatedProviderSafetyStops.add(to); | ||
| } |
There was a problem hiding this comment.
When a session has already committed a legitimate safety stop, a custom StreamFn on a later prompt can receive that same authenticated assistant object because convertToLlm returns assistant messages unchanged. This public function then lets the stream copy the authority to an arbitrary forged failure via transferProviderSafetyStop(previous, forged); the sanitizer accepts the new object and the forged stop suppresses the fallback chain. This is a separate bypass even if the mint module's wildcard export is blocked, so transfer must also require a runtime-only capability or remain inaccessible to custom streams.
Useful? React with 👍 / 👎.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 4 actionable issues, led by Public provenance transfer can authenticate forged messages and Public transfer can clone terminal authority. These findings require changes before approval.
Findings / Required Changes
- [P1] Public provenance transfer can authenticate forged messages.
Reference:packages/ai/src/adapter-internals/provider-safety-stop.ts:68-73
transferProviderSafetyStop accepts any destination and publicly exposes this operation, so code holding one authenticated provider message can copy terminal authority onto an altered or fabricated message. Restrict transfer to a private runtime rebuild capability or validate destination ownership. - [P1] Public transfer can clone terminal authority.
Reference:packages/ai/src/adapter-internals/provider-safety-stop.ts:68-70
transferProviderSafetyStop is publicly re-exported and accepts any authenticated source plus arbitrary destination, allowing callers who obtain a genuine adapter-marked message to authenticate a forged message. Keep transfer package-private or require an inaccessible runtime capability for transfers. - [P1] Public transfer can forge terminal authority.
Reference:packages/ai/src/utils/provider-safety-stop.ts:10-12
transferProviderSafetyStopis publicly exported, while its implementation marks any caller-supplied target when given an authenticated source. A consumer can combine an authenticated provider message with a forged target carryingerrorKind: "provider_safety_stop", bypassing the intended adapter-only provenance boundary. Keep transfer package-private, or bind transfer to an internally created rebuild and validate/overwrite the target fields. - [P1] Public transfer can mint authority onto arbitrary objects.
Reference:packages/ai/src/adapter-internals/provider-safety-stop.ts:70-74
transferProviderSafetyStop is re-exported publicly and accepts any destination without runtime ownership proof; a caller holding an authenticated message can mark an attacker-controlled copy as terminal. Keep transfer package-private or require an unforgeable runtime capability/destination token.
CI / Verification
- Reviewed the exact remote head:
cd2940488f055adb9094fc9b0230179846b68602. - CI summary: 19 passing, 3 failing, 11 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Provider safety-stop provenance is protected at minting, but the public transfer boundary can reauthorize arbitrary destination messages. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Concurrency paths preserve authenticated identity across managed rebuilds, but unrestricted public provenance transfer permits terminal-authority forgery. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | The provenance mark blocks unauthenticated labels, but the public transfer API still permits authenticated authority to be copied onto arbitrary messages. |
| A4. Verification / Tests / CI | APPROVED | Verification coverage is strong for affected tests, but repository-level PR contract compliance remains unverified due to failed contract checks. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation changes are mostly coherent, but the public provenance-transfer API leaves a terminal-authority forgery risk; skipped platform jobs leave cross-platform behavior unverified. |
Limitations
- PR contract validation checks are failing, so repository-level contract compliance is not established despite affected-path tests passing.
- PR contract validation checks failed, so repository-level PR contract compliance is not established.
- Exact-head PR contract checks failed, so repository contract compliance is not established.
- PR contract validation failed twice and PR contract bootstrap failed; the broker provided no failure details, so the specific contract defect cannot be determined.
- Windows native/toolchain, Windows safety, and darwin-arm64 smoke jobs were skipped, so platform behavior on those targets is not established; PR contract validation also failed.
Remove public transfer authority from the AI package. The agent loop now records provenance only for its own managed rebuild destinations in a module-private WeakSet, so public consumers cannot copy terminal authority from a genuine adapter-marked source. Closes the remaining P1 trust-boundary finding on #4782. Lore-id: 4777-p1-private-transfer Constraint: public consumers must not mint or transfer provider safety-stop authority Constraint: legitimate managed assistant rebuilds remain terminal Rejected: public transfer helper | arbitrary destinations could become terminal Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: provider, managed-attempt, retry, e2e, and package checks Not-tested: full CI after final push
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head dcc636b8 — merge blocked. My finding is fixed; the fix for it introduced a way to bypass the whole mechanism.
the trailing exit is sanitized
agent-loop.ts:3530-3534 now sanitizes the result before the trailing managedAssistantShell rebuild, matching the done/error branch at :3505-3509. That closes the start + end(result) path I blocked on.
The normal-path properties I confirmed earlier all still hold: authenticated provider_safety_stop checked before classifyFallbackTrigger (:359-378), both discard gates calling it (:2571, :2789), adapter-side minting intact, shell transfer at :1271, and the 400/429 e2e still asserting one call, zero model_fallback_switched, and a retained typed assistant. types.ts and models.json unchanged.
major — the capability module is reachable as a deep import
Making provider-safety-stop package-private is the right instinct, but packages/ai/package.json still carries:
"./*.js": "./src/*.ts"Node subpath patterns are string replacement and * matches across /, so @gajae-code/ai/adapter-internals/provider-safety-stop.js resolves to ./src/adapter-internals/provider-safety-stop.ts. That module exports mintProviderSafetyStop along with PROVIDER_SAFETY_STOP_ADAPTER_CAPABILITY.
So any consumer can import the mint and manufacture authenticated provenance — which is the one thing the design says is unforgeable. The sanitizers you just completed strip unauthenticated labels; they cannot help against a properly minted one.
To be precise about the mechanism, since it is not the obvious ./*: there is no ./adapter-internals/* entry. The exposure comes from the top-level ./*.js pattern, which is easy to read as "only top-level .js files" and is not.
Fix: add an explicit "./adapter-internals/*": null denial ahead of the wildcard, and pin it with a resolution test asserting the deep import fails. Checking only the root index (packages/ai/test/provider-safety-stop.test.ts:31-37) cannot catch this — the root export was never the reachable path.
the new trailing test does not pin the trailing sanitizer
managed-attempt-transaction.test.ts:244-288 does reach start + end(result), which is the right shape. But it uses status 500, which managedRetryableFailure classifies as server — so the attempt is discarded and fallback proceeds whether or not the trailing sanitizer runs. Delete the sanitizer and the test still passes.
Use status 400 through AgentSession's field-only terminal gate — the other class, which is the one that depends on the label surviving — or assert directly that the trailing result is stripped before policy runs.
the body still overclaims
It describes the privacy fix as complete. With ./*.js in place the module is importable, so that statement is not accurate as written. Please reconcile it with the tree.
Reviewed by @probepark — method: confirmed the trailing sanitize landed, then read the exports map rather than trusting the "package-private" framing, and worked out which pattern actually exposes the module. Checked whether the new test's status code exercises the class that depends on the fix.
gajae.pr-review-verdict.v1 merge-blocked sha256:0703f36c163de64672c28346be8d08280fd269c61dd0a7374d1f9f1a2ee937bd reviewer:human reviewer-id:probepark evidence:exact-head-dcc636b8-trailing-exit-sanitized-but-the-star-js-export-pattern-makes-the-mint-capability-a-reachable-deep-import
|
Final trust-boundary fix pushed at exact head
The public Fresh exact-head reviews requested from Signed-off-by: gaebal-gajae (clawdbot) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc636b80e
ℹ️ 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".
| function isManagedProviderSafetyStopAuthenticated(value: unknown): boolean { | ||
| return ( | ||
| isProviderSafetyStopAuthenticated(value) || | ||
| (typeof value === "object" && value !== null && managedProviderSafetyStops.has(value)) |
There was a problem hiding this comment.
When a session continues after a genuine safety stop, the default convertToLlm passes that committed assistant object unchanged to the next custom StreamFn; the stream can mutate and return that same object as an arbitrary error, and this identity-only WeakSet check still authenticates it, allowing the forged failure to suppress the fallback chain. Fresh evidence at this head is that the public transfer helper was removed, but direct reuse of the authenticated source object still transfers its authority; revoke marks after adjudication or bind them to an immutable, single-invocation envelope.
Useful? React with 👍 / 👎.
Owner integration of the exact-head review findings — new head
|
Finding (at dcc636b8) |
Disposition | Commit |
|---|---|---|
[P1] "./*" wildcard export makes adapter-internals/provider-safety-stop deep-importable — mint capability reachable by any custom StreamFn (probepark, merge-blocking) |
./adapter-internals/* is now a null export; @gajae-code/ai/adapter-internals/provider-safety-stop fails ERR_MODULE_NOT_FOUND, barrel + ./utils/provider-safety-stop keep only the verifier; pinned by test |
e26a6d41e |
| [P1] pi-native transport loses the mark across the gateway process boundary — genuine safety stops stripped as unauthenticated, fallback advances, hint lost | restoreProviderSafetyStopFromTrustedTransport re-establishes exactly the authority SSE serialization dropped, gated on a loopback gateway URL (default gjc auth-gateway bind); remote explicitly-configured endpoints stay unauthenticated; 3 new tests |
0a9590847 + 5aa240d3e |
| [P1] committed authenticated objects re-enter later streams with live authority (re-use transfer) | dispatch-entry expiry of both WeakSet marks (expireProviderSafetyStopAuthority) + public revokeProviderSafetyStop (authority-removing only); behavioral test pins forged re-use as fallback-eligible |
4927b402e |
| [P2] frozen/Proxy-trapped final message aborts the run in the strip | strip rebuilds a plain mutable copy on delete failure; test verified failing on the pre-fix loop | 4927b402e |
[P2] forged label with non-error stopReason survives and can skip session compaction checks |
strip is now stopReason-independent; test verified failing on the pre-fix loop |
4927b402e |
Verification on ee5baf16: ai suite 2674 pass (39 failures reproduce identically on pristine 7932f8a8 — broker/gateway/OAuth/token suites plus three Bedrock tests that flake on both trees, verified by diffing full failure lists); agent suite 812 pass / 0 fail; coding-agent safety + session selection 930 pass / 0 fail; bun --cwd=packages/{ai,agent,coding-agent} run check exit 0; verify-gjc-state-writers --fail clean.
Digests: base 7932f8a8acef0d29d7acf76d1388e2a7e448530b, head ee5baf16bb061f63e335b60f32502f973e30808e, git diff --binary --full-index --no-ext-diff base...head sha256 7de9b1a4041dc7429086570ef21daaa8cda33a1e62af6a35d33b56e8d35c7310 (matches the verdict block in the PR body).
Independent exact-head review re-requested from @snowykr and @probepark — any prior review is invalidated by the new head.
Signed-off-by: gaebal-gajae (clawdbot) 🦞
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5baf16bb
ℹ️ 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".
| const url = resolveStreamUrl(model as Model<Api>); | ||
| const trustedTransport = isLoopbackGatewayUrl(url); | ||
| const fetchImpl = options?.fetch ?? globalThis.fetch; |
There was a problem hiding this comment.
A custom StreamFn can import the publicly exported streamPiNative provider (or call streamSimple with transport: "pi-native"), choose a loopback baseUrl, and supply options.fetch that returns a forged SSE safety-stop event. Because trustedTransport depends only on the requested URL, no loopback gateway is contacted, yet lines 224-235 restore the adapter authority and the forged stop can suppress the fallback chain. The new pi-native test itself demonstrates this shape with an injected fetch; provenance restoration needs an authenticated transport signal that callers cannot synthesize, rather than the model URL alone.
Useful? React with 👍 / 👎.
| function sanitizeProviderSafetyStopProvenance(message: AssistantMessage): AssistantMessage { | ||
| if (message.errorKind !== "provider_safety_stop" || isManagedProviderSafetyStopAuthenticated(message)) { |
There was a problem hiding this comment.
When an otherwise valid custom-stream final message has an errorKind accessor or Proxy get trap that throws, this direct read escapes before the try block. In a managed-fallback run that bypasses the existing guarded managedAssistantShell path and aborts the run, whereas nonessential hostile metadata previously degraded safely; read the field through the guarded property helper and rebuild without invoking accessors.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head ee5baf16 — merge blocked.
fixed
The prior deep-import exposure is fixed: packages/ai/package.json denies ./adapter-internals/*, and the package test rejects that deep import.
remaining prior blocker
The trailing sanitizer regression pin is still not fixed. packages/agent/test/managed-attempt-transaction.test.ts:244-291 still uses status 500, so deleting the sanitizer can still pass through the retryable/server path. The separate 400/429 e2e uses a terminal event, not the trailing start + end(result) iterator-completion path.
new major — public pi-native transport is a provenance minting oracle
The delta adds the restore path in packages/ai/src/providers/pi-native-client.ts:194-234. It treats a loopback-looking URL as trusted while accepting a public caller-supplied SimpleStreamOptions.fetch; caller-controlled response data can therefore be converted into authenticated terminal safety-stop provenance. The public streamSimple/provider exports make this reachable. A custom fetch plus loopback base URL can mint a forged typed stop and suppress fallback, defeating the capability boundary.
Do not infer provenance from hostname plus message fields. Fail closed for the pi-native serialization path or require an authenticated gateway identity/envelope unavailable to public callers.
Reviewed by @probepark — method: delta-only comparison to the blocked head, verified the deep-import fix, checked the trailing branch test status, and traced the new public transport path into the private restore capability.
gajae.pr-review-verdict.v1 merge-blocked sha256:7de9b1a4041dc7429086570ef21daaa8cda33a1e62af6a35d33b56e8d35c7310 reviewer:human reviewer-id:probepark evidence:exact-head-ee5baf16-public-pi-native-fetch-can-mint-authenticated-safety-stop-provenance
Remove public transfer authority from the AI package. The agent loop now records provenance only for its own managed rebuild destinations in a module-private WeakSet, so public consumers cannot copy terminal authority from a genuine adapter-marked source. Closes the remaining P1 trust-boundary finding on #4782. Lore-id: 4777-p1-private-transfer Constraint: public consumers must not mint or transfer provider safety-stop authority Constraint: legitimate managed assistant rebuilds remain terminal Rejected: public transfer helper | arbitrary destinations could become terminal Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: provider, managed-attempt, retry, e2e, and package checks Not-tested: full CI after final push
The "./*": "./src/*.ts" export pattern also matches nested subpaths, so @.gajae-code/ai/adapter-internals/provider-safety-stop stayed deep-importable and any custom StreamFn could reach mintProviderSafetyStop plus the adapter capability — the provenance boundary the adapter-internals move was supposed to create did not exist (exact-head review of #4782 at dcc636b). Block ./adapter-internals/* with a null export target so resolution fails closed outside first-party relative imports; public consumers keep only the verifier through the barrel and ./utils/provider-safety-stop. Lore-id: 0e61c2f1 Constraint: public surface stays verify-only; the mint must stay unreachable Tested: bun test packages/ai/test/provider-safety-stop.test.ts (7 pass incl. new export-map pin) Confidence: high Scope-risk: narrow Reversibility: trivial
With transport=pi-native the first-party adapter runs inside the auth-gateway process; encodeStream serializes the terminal event to SSE and the client parses a fresh object, so the process-local WeakSet mark never reached the agent loop. The loop's provenance sanitize then stripped a genuine Anthropic/OpenAI/Google safety label as unauthenticated, and under transport facts (429/5xx) the refusal advanced the fallback chain instead of staying terminal — the manual-switch hint was lost (exact-head review of #4782). restoreProviderSafetyStopFromTrustedTransport re-establishes exactly the authority serialization dropped, and only for the typed error pair a first-party mint could have written on the gateway side. The client gates the restore on a loopback gateway URL (the default gjc auth-gateway bind); remote explicitly-configured endpoints stay unauthenticated and degrade to ordinary fallback-eligible errors. Lore-id: b7c40e9d Constraint: restore must never create authority, only re-establish what a gateway-side mint produced Constraint: remote gateway endpoints must stay unauthenticated Tested: bun test packages/ai/test/pi-native-client.test.ts packages/ai/test/provider-safety-stop.test.ts (21 pass) Confidence: high Scope-risk: narrow Reversibility: trivial
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Public revocation bypasses terminal safety policy. These findings require changes before approval.
Findings / Required Changes
- [P1] Public revocation bypasses terminal safety policy.
Reference:packages/ai/src/utils/provider-safety-stop.ts:8
revokeProviderSafetyStopis publicly exported and can remove authenticated authority from any marked message, allowing integrations or custom provider code holding that object to turn a terminal provider refusal into fallback-eligible data. Keep revocation package-private or require an unforgeable runtime-only capability.
CI / Verification
- Reviewed the exact remote head:
52ae9d4db07a1e159712bfa707c43dbae59e22f6. - CI summary: 27 passing, 4 failing, 20 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | API boundaries preserve provenance and prevent public, cloned, redirected, or serialized messages from minting provider safety-stop authority. |
| A2. Architecture / Correctness / Failure | APPROVED | Provenance transfer, fallback classification, stream finalization, and authority expiry appear concurrency-safe; no correctness defect was established. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | A3: Provenance authentication blocks forged labels, but public authority revocation leaves an actionable path to bypass terminal provider safety-stop policy. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is broadly passing, but repository acceptance and skipped-platform compatibility remain unresolved. |
| A5. Context / Compatibility / Platform | APPROVED | Provider safety-stop provenance and fallback integration appear compatible; no documentation or platform defect was established, though Windows behavior remains unverified. |
Limitations
- Exact-head PR contract checks are failing, so repository-level contract validation cannot be claimed beyond the passing affected-path checks.
- CI summary reports exact-head PR contract and bootstrap failures without diagnostic details, so their security-verification impact cannot be determined.
- Exact-head contract failures prevent establishing repository acceptance for this PR.
- Skipped Windows and macOS jobs leave those platform regressions unverified.
- Windows-specific behavior remains unverified because the Windows validation jobs were skipped.
Remove the public authority-revocation escape hatch while retaining runtime cleanup through the package-private adapter seam.
|
Applied the latest exact-head security fix from snowykrs review: |
|
Fresh exact-head approval requested for |
|
@codex review exact head |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 4 actionable issues, led by Anthropic sensitive refusals lose authentication and Sensitive Anthropic refusals lose terminal authority. These findings require changes before approval.
Findings / Required Changes
- [P1] Anthropic sensitive refusals lose authentication.
Reference:packages/ai/src/providers/anthropic.ts:2478-2484
The safety predicate accepts raw stop_reason="sensitive", but minting passes rawStopReason unless stop_details.type is "refusal"; with a sensitive stop this supplies "sensitive" only if rawStopReason is sensitive, otherwise an accepted sensitive stop can be minted with an unrecognized signal and remain unauthenticated. Pass the recognized structured refusal/sensitive signal explicitly for every accepted safety condition, and add a regression test for stop_details.type="sensitive". - [P1] Sensitive Anthropic refusals lose terminal authority.
Reference:packages/ai/src/providers/anthropic.ts:2484-2492
When stop_details.type is "sensitive", the code recognizes a safety stop but passes rawStopReason to mintProviderSafetyStop; common end_turn/raw values are not an accepted signal, so the authenticated terminal kind is not minted. Pass the recognized stop_details.type (including sensitive) and add regression coverage. - [P2] Caller-transport Anthropic refusals remain terminal instead of fallback-eligible.
Reference:packages/ai/src/providers/anthropic.ts:2474-2491
When a custom fetch/client prevents provenance minting, the adapter still sets stopReason to error and throws an Error without transport facts; the managed loop therefore cannot classify it as retryable. Preserve explicit non-authority transport facts (or otherwise classify the failure) so configured fallback can advance. - [P2] Direct OpenAI adapter callers silently lose safety-stop classification.
Reference:packages/ai/src/providers/openai-completions.ts:834-850
streamOpenAICompletions now requires an internal invocation token, but unlike streamAnthropic it does not establish that token for trusted bundled models. Existing callers using the exported provider adapter with ordinary options will receive an unauthenticated error_kind and changed fallback behavior. Preserve the direct-call contract or explicitly route/deprecate this public boundary with compatibility tests.
CI / Verification
- Reviewed the exact remote head:
095f195b8381bdb378f12c95ac465894f6b92d51. - CI summary: 27 passing, 3 failing, 20 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Adapter provenance is mostly fail-closed, but Anthropic sensitive refusals and direct OpenAI adapter callers have established authority and compatibility regressions. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Concurrency revocation is explicitly performed before and after context transforms; correctness risk remains in Anthropic sensitive-stop provenance minting. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy controls establish fail-closed provenance for provider safety stops and prevent structural or transport-controlled payloads from gaining terminal authority. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | Tests and affected-path CI pass, but Anthropic caller-transport refusals can suppress fallback; platform-specific behavior remains unverified. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and public-surface changes appear coherent; no documentation or platform defect was established, with skipped platform jobs limiting runtime confidence. |
Limitations
- The CI summary reports failures for Validate exact-head PR contract and PR contract bootstrap but provides no diagnostic output, so their root cause cannot be attributed to this change.
- Windows and darwin-specific validation jobs were skipped, so no cross-platform regression claim is established.
- Windows, Telegram, and darwin-arm64 platform-specific CI jobs were skipped, so those behaviors remain unverified.
|
Terminal exact-current status: head |
Recognize Anthropic sensitive stop details, keep caller-transport refusals retryable, and retain direct OpenAI bundled-model behavior.
|
Addressed the latest snowykr findings and reconciled six unrelated Herdr socket commits from current |
|
Fresh exact-head approval requested for |
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c643318f9
ℹ️ 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".
| // Terminal authority comes from the adapter mark, not the wire | ||
| // field: this call site parsed the structured content_filter | ||
| // finish reason from the provider's own response (#4777). | ||
| mintProviderSafetyStop( |
There was a problem hiding this comment.
When options.fetch is caller-controlled and its successful SSE response contains finish_reason: "content_filter" or a refusal delta, this mint returns false, but the return value is ignored and the message remains an untyped error. The subsequent catch replaces transportFailure with facts derived from a generic local error, which yields no retryable status; managed fallback therefore terminates instead of advancing the configured chain. Record the same untrusted_safety_stop transport facts used by the Anthropic and Google adapters whenever this mint is rejected.
Useful? React with 👍 / 👎.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Direct provider safety-stop behavior regresses for custom models. These findings require changes before approval.
Findings / Required Changes
- [P1] Direct provider safety-stop behavior regresses for custom models.
Reference:packages/ai/src/providers/anthropic.ts:1879-1881
Exported direct provider calls only mint provenance when the model is a registered bundled identity; existing callers using custom or manually constructed models now receive unauthenticated safety labels and lose terminal classification. Preserve compatibility via a documented trusted invocation path or an explicit supported adapter API without weakening validation.
CI / Verification
- Reviewed the exact remote head:
7c643318f9558751baf17d9491a0f9031666db6e. - CI summary: 26 passing, 4 failing, 21 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The new provenance boundary blocks forged labels but regresses authenticated safety-stop semantics for direct custom-model provider API callers. |
| A2. Architecture / Correctness / Failure | APPROVED | The safety-stop provenance and managed fallback concurrency changes establish the intended fail-closed behavior without an identified correctness defect. |
| A3. Security / Privacy / Trust | APPROVED | Provider safety-stop authority is provenance-bound and unauthenticated labels cannot suppress fallback or survive managed persistence boundaries. |
| A4. Verification / Tests / CI | APPROVED | A4: Focused tests and builds pass, but overall CI confidence is limited by failed PR-contract checks and pending virtual integration. |
| A5. Context / Compatibility / Platform | APPROVED | Integration boundaries, changelog documentation, package exports, and provider platform paths appear coherent; no concrete compatibility risk was established. |
Limitations
- Virtual integration validation is pending, so integration-only compatibility behavior remains unverified.
- Exact-head PR contract validation failed and virtual integration validation remains pending, so those gates cannot be claimed as passed.
- Full CI health cannot be claimed because Virtual integration validation is pending and PR contract checks are failing.
- Virtual integration validation is PENDING, so virtual-integration coverage is unverified.
- PR contract checks are FAILURE, but the summary provides no diagnostic detail to determine whether this reflects a code regression or repository-policy issue.
- Virtual integration validation is pending, so end-to-end integration behavior is not established by CI.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head 7c643318 — merge blocked.
The prior deep-import, trailing terminal-result sanitizer, and pi-native authority-restoration blockers are fixed. Two new trust-boundary failures remain:
- public direct OpenAI adapter calls mint terminal safety-stop authority when no explicit fetch override is supplied, but execution still uses mutable global
fetch. Caller code can patch global transport, forge a content-filter SSE response, obtain an authenticated provider stop, and suppress fallback. - authenticated message authority is a non-consuming process-wide WeakSet membership on a caller-visible mutable object. An object retained outside current history can be mutated and replayed from a later custom stream; history-only expiry cannot revoke it.
Bind authority to a private, one-shot adapter stream/attempt capability with expected model identity and consume it at finalization. Public calls through replaceable transport must fail closed. Add patched-global-fetch and cross-dispatch retained-object replay regressions.
No separate truncation false-success or duplicate-call blocker was found; pi-native now fails closed on non-terminal stream closure.
Reviewed by @probepark — finite delta review from blocked ee5baf16.
gajae.pr-review-verdict.v1 merge-blocked sha256:61f229208d11f3ac26798140c216ab0ffeedf27297ac46454c09e1d8ccdeeffc reviewer:human reviewer-id:probepark evidence:exact-head-7c643318-global-fetch-mint-and-replayable-message-authority
Keep terminal authority exclusively on dispatcher provenance so mutable global or caller-selected transports cannot mint authenticated safety stops.
|
Addressed both current-head trust-boundary findings by making public low-level Anthropic/OpenAI adapters fail-closed: only the first-party |
|
Fresh exact-head approval requested for |
|
@codex review exact head |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Reconciled the PR verdict metadata to the current state: exact head |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
dd93b4334ffa0afb96017d1fdbd315de59fc6d1c. - CI summary: 27 passing, 4 failing, 34 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The API boundary is fail-closed for unauthenticated safety-stop labels, while bundled dispatcher paths preserve authenticated refusal behavior. |
| A2. Architecture / Correctness / Failure | APPROVED | Correctness and concurrency safeguards appear coherent; no actionable race or state-transition defect was established. |
| A3. Security / Privacy / Trust | APPROVED | No security or privacy risk was established; unauthenticated provider safety-stop labels are fail-closed and cannot acquire terminal authority. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is broadly green for affected tests, but exact-head compliance and skipped platform coverage remain unverified. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and documentation behavior appears consistent; platform parity remains unverified where Windows and darwin-arm64 checks were skipped. |
Limitations
- The exact-head PR contract check failed, so this review cannot claim all current-head CI checks pass.
- Exact-head PR contract compliance cannot be asserted because its validation and bootstrap jobs failed.
- Windows and other skipped platform jobs leave those platform-specific regressions unverified.
- Windows and darwin-arm64 platform smoke jobs were skipped, so cross-platform behavior was not directly verified.
|
Terminal exact-current status: head |
|
Independent exact-head approval from |
3 participants
What
Bind terminal
provider_safety_stopauthority to adapter-minted provenance instead of the wire-assignableerrorKindfield. First-party Anthropic, OpenAI Completions, Google, and pi-native gateway paths now fail closed across caller-controlled fetch/client/prepare seams; managed runtime rebuild authority is module-private and expires at dispatch boundaries. Clones, JSON/persistence round-trips, custom streams, and forged labels cannot become terminal authority.Why
Fixes the linked #4777 trust-boundary issue: an untrusted provider or custom stream could previously self-label
errorKind: "provider_safety_stop"and suppress configured fallback. The current replacement preserves the prior valid hardening and reconciles it onto currentdev.Exact replacement
dd93b4334ffa0afb96017d1fdbd315de59fc6d1c08d002464076c862754561eaa627478d72a55f79sha256:1144d8f52744a82150d2af610f89bb709990fd96dbcfad7939317564e8edd55ffix/issue-4777-safety-stop-provenancedevValidation on the exact replacement
bun --cwd=packages/ai run check: pass with two pre-existing informational lint notices.bun --cwd=packages/agent run check: pass.bun --cwd=packages/coding-agent run check: pass.bun scripts/verify-gjc-state-writers.ts --fail: pass.bun run build: pass.bun run check: blocked by an existing SDK rollback fixture observing Bun 1.3.14 in that subprocess while expecting the pinned 1.4.0; no provider-safety-stop test failed.Risk classification
low-riskregression-riskhigh-risk— security/public trust-boundary change; independent authenticated exact-head domain review is required.GJC verdict
dev