Repository navigation
Conversation
Why: Installed plugins need an opt-in action pointer without inferring addressability from IM subscriptions or changing the existing delivery envelope and receipt. SDK activation must require the declared handler and revoke it on stop. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: Pairing, signed cloud summons, source-bound replies, original images and MCP belong to the installed package. Preserve legacy credentials and history through a fenced non-UI importer; use ordinary Host messaging, storage, streams and lifecycle instead of a protected cat or dedicated callback API. Keep older plugins on their pinned SDK while testing the new package and exact dependency closure. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: The new beta.39 artifact carries addressed delivery and dormant migration while retaining signed broker protocol 0.1.0. The release assertion must verify that exact new package identity. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: Dot retains its existing 10 MiB original-image protocol, while unrelated anonymous webhooks must keep the 1 MiB default. Expose a positive bounded per-contribution budget and generated 15 MiB hard cap for Host pre-parser enforcement. Include migrated TLS/proxy security fixtures and preserve old consumers own exact SDK identities in package checks. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: The Dot contract upgrade must not force unchanged GenOffice artifacts to adopt a new dependency. Compare the actual installed Contract against its declared exact pin instead of the independently advancing workspace source. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: Host execution diagnostics can change when a queue attempt retries. Only the stable deliveryId should identify the accepted plugin input, so the same summon cannot be posted twice after an execution retry. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: Host messaging materializes pmr_ plugin references and degrades legacy locator strings. Publish a stable PMR backed by the retained Dot asset, preserve the legacy publication ID separately, and stream owner HTTP reads so large originals do not hit the buffered response cap. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
…umer Why: Installing a new direct SDK must not rewrite an unchanged bundled plugin. Verify Feishu against its own source manifest while independently checking the current direct SDK/Contract pair. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: Returning cloud text must not implicitly select new recipients or re-summon Dot. The original Host message may identify a member to wake; otherwise persist the source-bound reply without routing from its body. Proactive asks retain their separate explicit recipient validation. [砚砚/gpt-6.1-sol🐾] Thread-Context: threadId=thread_mvgmbp6scs7nd1ro catId=cat-eqdvbcxw
Why: the legacy authority can be revoked between migration checks. Import must CAS a later revocation into the same connection without restoring it on stale retries or losing newer runtime evidence, including after a receipt write crash.
Why: concurrent legacy imports could pass the absence check and rename conflicting PNGs over a completed original before reporting an immutable-record conflict. Share atomic no-replace publication with runtime images; conflicting bytes fail closed and matching retries reuse the winner.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: adedd1ea8d
ℹ️ 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 identity = context.requestIdentity; | ||
| if (identity && (identity.kind !== 'owner_http' || identity.pluginInstanceId !== context.pluginInstanceId || identity.userId !== context.userId || identity.pluginId !== 'official.chatgpt-dot' || identity.contributionId !== 'dot-mcp' || identity.method !== 'POST' || identity.declaredPath !== 'mcp')) | ||
| throw new Error('dot_owner_required'); | ||
| await runtime.connection.owner(context); |
There was a problem hiding this comment.
For shared-relay requests without x-dot-credential, the owner HTTP context is checked only here and then reduced to the stored credential. If context.signal aborts or assertCurrent() begins rejecting while a tool is running (for example during PNG validation or a Host send), read, reply, image, and ask never consult that context again and can finish after the authenticated owner request has been revoked. Thread the context fence through the operation and recheck it around side effects rather than treating this one admission check as durable authority.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. Each active action now retains its Host context in a private async request scope. Existing storage, messaging and thread ports check signal/assertCurrent before and after every asynchronous call; captured streams keep the same fence after the handler returns. Shared read/reply/image/ask tests revoke authority during awaited Host work, and verify no publication/read evidence escapes. Independent requests retain their own authority. Fixed at 407d565973e2479bb3784e35cfc4de10eb9631f3. Regression evidence: test/authority-lifecycle.test.mjs failed before the fix and now passes. The full canonical gate passed 36/36 configured steps, including all 51 Dot tests and actual offline archive installation/lifecycle, before this push.
| const reservation = { requestDigest, attemptId: state.attemptId, threadId: input.thread_id, recipientCatIds: targets, messageId: null as string | null }; | ||
| if (!prior && !((await this.host.storage.compareAndSet(key, null, reservation)).applied)) | ||
| throw new Error('dot_state_conflict'); | ||
| await this.connection.ready(secret); |
There was a problem hiding this comment.
When dotCredential is configured, reconnecting reuses the same secret. An ask can therefore pass the initial attempt_id check under the old pairing, pause while the owner reconnects and the new subscription becomes ready, and then pass this second ready(secret) call because its returned state is discarded; the code proceeds to post and subscribe using the old state.attemptId. Compare the re-read attempt (or revision) with the originally admitted state before writing so reconnect actually revokes in-flight requests from the old pairing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. The admitted pairing attempt is compared again before ask reservation/send and before completion recording/subscription; a configured credential cannot substitute for attempt identity. A regression holds getCats, reconnects with the same configured secret and a newly ready subscription, then resumes the old ask and proves it posts nothing. Fixed at 407d565973e2479bb3784e35cfc4de10eb9631f3. Regression evidence: test/authority-lifecycle.test.mjs failed before the fix and now passes. The full canonical gate passed 36/36 configured steps, including all 51 Dot tests and actual offline archive installation/lifecycle, before this push.
| const {['dot.import']: migrationOnly, ...liveSettings} = settings.actions; | ||
| Object.assign(actions, liveSettings); | ||
| return { actions, streams: runtime.streams, configRefresh: refresh, stop() { | ||
| stop ??= (async () => { controller.abort(new Error('dot_stopped')); await runtime.settle(); await settings.dispose?.(); if (running === runtime) |
There was a problem hiding this comment.
In the inspected start/stop flow, settle() waits only tasks registered by serial(), while ask() and testRoundtrip() never enter that pending map. If disable occurs while an ask's host.messaging.send() is unresolved, stop() can return immediately; the send may then durably post before the subsequent lifecycle assertion throws, leaving a visible question with no recorded message ID or reply subscription after the plugin is disabled. Track these mutating RPC operations so stop does not complete until they settle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. All admitted active actions are tracked, including proactive asks and settings roundtrip actions. Stop aborts the runtime/settings lifetime and waits for those action promises as well as serialized deliveries. Separate regressions hold an ask and a roundtrip Host send and prove stop cannot finish until the admitted operation settles. Fixed at 407d565973e2479bb3784e35cfc4de10eb9631f3. Regression evidence: test/authority-lifecycle.test.mjs failed before the fix and now passes. The full canonical gate passed 36/36 configured steps, including all 51 Dot tests and actual offline archive installation/lifecycle, before this push.
| this.assertLive(); | ||
| if (!state) | ||
| throw new Error('dot_connection_missing'); | ||
| return { ...state, revision: state.revision + 1, revokedAt: this.now() }; |
There was a problem hiding this comment.
If migration has already imported a non-null legacy revokedAt, an owner invoking disconnect again overwrites that first revocation with the current clock value. Authority remains revoked, but the original revocation evidence that the migration is intended to preserve is lost; retain state.revokedAt when it is already set (or make the repeated disconnect a no-op).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. Disconnect now keeps state.revokedAt ?? now(). The regression starts with an imported non-null timestamp and disconnects twice, retaining the original value. Fixed at 407d565973e2479bb3784e35cfc4de10eb9631f3. Regression evidence: test/authority-lifecycle.test.mjs failed before the fix and now passes. The full canonical gate passed 36/36 configured steps, including all 51 Dot tests and actual offline archive installation/lifecycle, before this push.
Why: shared owner admission was reduced to a reusable secret, proactive writes escaped stop supervision, and reconnect/disconnect could lose pairing or revocation boundaries. Keep request fences across existing async Host ports and captured streams, drain all admitted actions, recheck ask attempts, and preserve first revocation. Eight regressions failed before the fix and now pass.
Why: request fencing wraps the required async SDK ports at activation. The fresh consumer supplied no thread port despite the manifest declaring it; make its Host fixture explicit and fail unexpected calls instead of weakening runtime fencing.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: Re-review after R1: all four findings have targeted RED→GREEN coverage in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 407d565973
ℹ️ 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 entry = await host.storage.get(CONNECTION_KEY); | ||
| if (!entry) { | ||
| if (!(await host.storage.compareAndSet(CONNECTION_KEY, null, next)).applied) |
There was a problem hiding this comment.
If the settings carrier is disposed or the owner fence is revoked while host.storage.get(CONNECTION_KEY) is pending, this path resumes directly into compareAndSet and can import the legacy connection (including its credential) after authority has ended; the outer settings guard only rejects after the handler returns and cannot undo that write. The same raw-host pattern recurs in retain and the receipt CAS, so use the context-fenced storage port or recheck the combined signal/assertCurrent() after each awaited read and around each write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. Fixed in 71a93bcda354c4d32583564fbccac2b6374c5f92: all importer storage reads and writes now use the existing async owner-context fence, including the connection CAS, dispatch/image retain loops and cached/committed receipts. The combined signal is checked before and after assertCurrent, and each port call checks again on both sides. Five new regressions in test/migration-authority.test.mjs reproduced expired-authority writes or receipt returns and now pass; existing migration journeys remain green. Full canonical gate passed 36/36 configured steps, including all 56 Dot tests and the actual offline archive/lifecycle proof, before this push.
Why: owner revocation or settings disposal during a storage read could resume into raw CAS writes or return a cached receipt. Reuse the same async port fence for connection, dispatch, image and receipt storage, preserving existing migration data semantics. Five new regressions failed before the change and pass with existing migration journeys.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: Re-review after R1: all four findings have targeted RED→GREEN coverage in R3: the new R2 finding #95 (comment) is addressed using the same fenced async storage port for the entire dormant importer. Five new regressions cover paused connection/dispatch/image reads, settings disposal and cached receipt reads; all failed before the fix and pass with existing migration journeys. No schema or Host API changes. Please recheck the actual new head and verify this remaining authority boundary. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a93bcda3
ℹ️ 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".
| if (ask.attemptId === state.attemptId && ask.threadId === source.threadId && ask.messageId === replyTo && ask.recipientCatIds.includes(source.catId)) | ||
| return this.deliver(input); |
There was a problem hiding this comment.
When reconnect completes while storage.list() is pending, this condition can match an ask from attempt A using the stale state, but deliver() then reloads the connection and creates/sends the event under ready attempt B. A reply to a question from the revoked pairing can therefore be forwarded to the newly paired callback; pass the matched attempt into deliver() (or recheck it immediately before dispatch) so the follow-up cannot cross pairing generations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f71599: followup passes the matched ask attempt into deliver admission; deliver rejects a changed attempt and uses sameAttempt rather than ignoring ready's current connection. Two deferred-list tests reproduced a new callback receiving the retired question's reply, both with rotated and reused credentials. Both now reject without creating a dispatch or sending a callback, while the existing successful follow-up/restart journey remains green.
| }, | ||
| }; | ||
| // Settings actions exist in the stopped Manager path and the active action table. | ||
| const settings = await definition.settings!(host); |
There was a problem hiding this comment.
If this second settings-side config read rejects, or webhookProxyUrl changes to an invalid value during startup, start() rejects after running was assigned but outside the preceding cleanup catch. The caller receives no activation/stop handle, the controller remains live, and retries on the same definition fail with dot_already_running; include settings initialization in the startup cleanup path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f71599: all startup work after running is assigned is inside the cleanup boundary, including settings config initialization. Failure aborts the controller, disposes any acquired settings, settles runtime work and releases the running slot before propagating the original error. Two tests first reproduced dot_already_running on retry after config read rejection or invalid proxy configuration; both now retry successfully and stop normally.
Why: an ask lookup can outlive reconnect, and settings initialization can reject after runtime ownership is assigned. Preserve the matched pairing at dispatch admission and clean every failed activation before retry; add RED-to-GREEN coverage for rotated/reused credentials and settings read/proxy failures.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: Re-review after R1: all four findings have targeted RED→GREEN coverage in R3: the new R2 finding #95 (comment) is addressed using the same fenced async storage port for the entire dormant importer. Five new regressions cover paused connection/dispatch/image reads, settings disposal and cached receipt reads; all failed before the fix and pass with existing migration journeys. No schema or Host API changes. Please recheck the actual new head and verify this remaining authority boundary. R4 after R3: #95 (comment) and #discussion_r4326252601 are addressed. Four new regressions first failed on the actual old source: both rotated and reused credentials forwarded an old question reply into the newly paired callback; failed second config reads/invalid proxy configuration left dot_already_running on retry. The same tests and existing journeys pass on the new source. Failure-Mode Sweep: repeated findings concern authority continuity across async work. Scanned connection.ts, runtime.ts, rpc.ts, migration.ts and plugin-entrypoint.ts against the prior R1/R2 findings. Owner/request authority remains scoped/fenced across every SDK call; dormant import includes cached receipts. Old record reads/replies/images use source/dispatch attempt validation, ask and roundtrip check sameAttempt, and followup now explicitly passes its matched ask attempt into deliver admission. Ordinary delivery also checks sameAttempt instead of ignoring the result of ready(). Existing startup cleanup covered only restoreSubscriptions; it now covers settings initialization and action construction, revokes transport/settings, settles pending work and releases the running slot before rethrow. No fallback was added (one existing cleanup catch expanded, no new fallback layer), public schema/SDK unchanged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f71599401
ℹ️ 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 thread = await this.host.threads.get(record.threadId); | ||
| const source = await this.host.messaging.get(record.sourceMessageId); | ||
| this.connection.assertLive(); | ||
| if (!thread || !source || source.threadId !== record.threadId || source.userId !== record.ownerUserId || record.sourceRevision && source.sourceRevision !== record.sourceRevision) |
There was a problem hiding this comment.
With an external RPC admitted on attempt A, if reconnect or disconnect completes while either of these Host reads is pending, source() returns after having checked ready() only before the awaits. The final authorize() in reply(), image(), sendEvent(), and streams.read() can therefore be followed by a Host/webhook write or byte yield for the revoked attempt; the later evidence/storage check only turns the call into an error after the side effect. Re-run ready() and the attempt check after both reads before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e57c2d3: source() now re-runs sameAttempt (including ready/revoked/expiry checks) after both Host reads and before returning authorization. The same sweep found streams can suspend after source() at file reads or chunk boundaries; every chunk now rechecks the pairing after the captured owner fence. Ten new regressions were all RED before the fix: reconnect with a reused credential, and disconnect during final reply/image/callback/stream message lookups, final thread lookup, and between original PNG chunks. GREEN confirms zero late Host messages/callbacks/bytes, not merely an eventual error. The existing successful source/reply/image/lifecycle journeys still pass. Public Contract/SDK bytes and shapes remain unchanged.
Why: owner fences do not revoke a pairing snapshot when reconnect or disconnect completes during awaited source reads. Recheck ready state and the original attempt after both Host reads and before every original-image chunk; regressions prove no later reply, image, callback or byte yield survives the retired pairing.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: Re-review after R1: all four findings have targeted RED→GREEN coverage in R3: the new R2 finding #95 (comment) is addressed using the same fenced async storage port for the entire dormant importer. Five new regressions cover paused connection/dispatch/image reads, settings disposal and cached receipt reads; all failed before the fix and pass with existing migration journeys. No schema or Host API changes. Please recheck the actual new head and verify this remaining authority boundary. R4 after R3: #95 (comment) and #discussion_r4326252601 are addressed. Four new regressions first failed on the actual old source: both rotated and reused credentials forwarded an old question reply into the newly paired callback; failed second config reads/invalid proxy configuration left dot_already_running on retry. The same tests and existing journeys pass on the new source. Failure-Mode Sweep: repeated findings concern authority continuity across async work. Scanned connection.ts, runtime.ts, rpc.ts, migration.ts and plugin-entrypoint.ts against the prior R1/R2 findings. Owner/request authority remains scoped/fenced across every SDK call; dormant import includes cached receipts. Old record reads/replies/images use source/dispatch attempt validation, ask and roundtrip check sameAttempt, and followup now explicitly passes its matched ask attempt into deliver admission. Ordinary delivery also checks sameAttempt instead of ignoring the result of ready(). Existing startup cleanup covered only restoreSubscriptions; it now covers settings initialization and action construction, revokes transport/settings, settles pending work and releases the running slot before rethrow. No fallback was added (one existing cleanup catch expanded, no new fallback layer), public schema/SDK unchanged. R5 after R4: #95 (comment) is fixed. The shared source() gate now rechecks sameAttempt/ready after the awaited thread and message lookups. Its callers therefore receive no source authorization after an intervening disconnect/reconnect. The same failure-mode sweep found the original-byte stream could resume after revocation between chunk requests or a file read; every chunk now rechecks sameAttempt after its captured owner fence. Ten new regressions were all RED on the previous source, GREEN on this revision: reconnect/disconnect during the final reply/image/callback/stream message lookup (8), disconnect during the final thread lookup (1), and disconnect between PNG chunks (1). They assert no later Host publication, callback invocation or byte output, rather than merely asserting an eventual RPC error. Existing lifecycle/source/stream journeys are retained. Pattern summary across prior rounds: owner/request lifetime (R1/R2) and pairing lifetime (R1/R3/R4) are distinct. Owner context remains request-local and fenced across existing SDK calls; pairing changes require fresh connection/attempt evidence after asynchronous Host reads. All reply/image/callback callers share source(); the explicit stream chunk guard covers suspension after source() returns. Delivery and followup retain their matched attempt; ask/testRoundtrip and connection evidence preserve their existing explicit attempt checks. No Host API, schema, dependency, fallback, key-registry or external permission change is introduced. If further substantive findings remain after this fifth review, the next step will be a systemic review of the complete failure pattern before any sixth request, not another isolated patch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e57c2d3262
ℹ️ 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 body = JSON.stringify({ eventId: record.eventId, name: 'dot.summoned', timestamp: new Date(record.occurredAt).toISOString(), | ||
| data: { event_id: record.eventId, thread_id: record.threadId, source_message_id: record.sourceMessageId, dispatch_invocation_id: record.dispatchInvocationId }, cursor: null }); | ||
| try { | ||
| const result = await this.connection.send(subscription.url, dotWebhookHeaders(record.eventId, subscription.id, subscription.secret, body, Date.now(), subscription.rotationUntil && subscription.rotationUntil > Date.now() ? subscription.previousSecret ?? undefined : undefined), body, this.connection.operationSignal); |
There was a problem hiding this comment.
When reconnect or disconnect completes while the direct sender is awaiting DNS (or the proxy sender is awaiting DNS/CONNECT), this call still holds the old subscription and its signal is only tied to request/runtime/transport lifetime, so it proceeds to POST the summon to the revoked callback after the pairing change. The later authorize() can make the action fail but cannot retract the disclosed event; bind the transport's pre-write point to state.attemptId or abort in-flight sends when that pairing is revoked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b59275e. A captured owner-and-pairing fence now reaches the actual transport write point. DotWebhookSend carries beforeWrite through the runtime/configured sender facade. Direct transport waits for TLS readiness and calls it before end/HTTP headers/body; proxy transport calls it on the verified TLS socket before creating the callback request. Both summons and verification challenges bind their original attempt (including unpaired expiry for verification). RED→GREEN integration tests revoke during callback preparation, and real direct/proxy TLS tests observe zero request starts/headers/body after fence rejection.
| await retainImageFile(this.host.dataPath, id, checked.bytes, () => this.connection.assertCurrent(), 'dot_publication_conflict'); | ||
| const inserted = await this.host.storage.compareAndSet(key, null, asset); |
There was a problem hiding this comment.
When a publish_image admitted on attempt A is waiting in retainImageFile()'s temporary-file write, reconnect or disconnect can finish before that helper resumes; the callback here checks only request/runtime lifetime, not the captured pairing, so it can then link the canonical file and this CAS persists metadata for the retired attempt. The later authorization before message publication only rejects after those retained side effects, leaving up to 10 MiB of inaccessible data per in-flight event; carry sameAttempt(state) through retention and recheck it before the metadata CAS.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b59275e. The captured owner-and-pairing fence is passed through retainImageFile after its temporary-file write and before canonical linking, and repeated before the metadata CAS. RED→GREEN covers both reconnect/disconnect during temp writes (no canonical file or metadata) and after an already-authorized link but before metadata persistence (no later metadata or message; the already-authorized original is retained unchanged). The full recurring failure-pattern audit was completed before requesting R6.
Why: five review rounds exposed the same authority-lifetime mistake below runtime checks. Capture owner and pairing authority and require it at TLS-ready HTTP writes, verification challenges, immutable file publication and metadata CAS. Record the complete failure-pattern audit and nine RED-to-GREEN regressions that observe actual requests, files and metadata rather than eventual errors.
Why: the new TLS-ready authority check is asynchronous and can stall before a request owns the socket. Race that read-only check against the existing abort signal so deadline/disable closes resources without sending HTTP; do not abandon or weaken draining of mutating Host calls. A RED-to-GREEN timeout regression proves the boundary.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: R6 is requested only after the five-round loop brake: reread all ten findings and original scope, and record the complete failure-pattern audit in docs/plans/2026-10-20-f202-dot-plugin.md#review-loop-brake-complete-failure-pattern-audit-after-r5. This is an explicit continuation after the audit, not an automatic isolated patch cycle. Recurring root: Host request authority, enabled-runtime lifetime and pairing attempt lifetime differ. Upper-layer source checks cannot authorize lower asynchronous DNS/CONNECT/TLS or file writers indefinitely. R5 findings #95 (comment) and #95 (comment) are addressed by carrying a captured owner-and-pairing fence to actual effect boundaries. The private DotWebhookSend seam now carries optional beforeWrite. Direct transport invokes it only after TLS readiness, before req.end/any HTTP headers or body. Proxy transport invokes it on the already verified TLS socket before creating the callback request. DNS pinning, TLS verification, no redirects, existing deadlines and signal aborts remain. Both event delivery and verification challenges pass their original pairing fence; verification uses the unpaired attempt, with expiry checks. No SDK/Contract/Host API changes. Image retention invokes the same fence after temporary bytes are written and before linking a new canonical original; the caller repeats the fence before metadata CAS. Existing canonical originals are never overwritten or deleted. Dormant migration intentionally imports historical/revoked records under owner-operation authority, so it retains its own owner fence rather than requiring an active pairing. Nine effect-boundary regressions were demonstrated RED before their fixes and GREEN here: reconnect/disconnect during callback preparation (2), during temporary PNG creation (2), after an already-authorized original was linked but before metadata CAS (2), verification challenge across reconnect (1), and real direct/proxy TLS writers (2). Actual loopback TLS tests observe zero HTTP request starts/headers/body when the authority fence fails; file tests observe no new canonical file/metadata after revocation before publication, and keep an original already linked while authorized. Previous owner/request/source/chunk/startup/import regressions remain. Please judge whether the complete authorized package outcome and these carried writer fences now hold, including cancellation/resource cleanup and unexpected gaps after asynchronous work. Whole Host/account acceptance remains C-owned and pending. This does not authorize npm/catalog publication or merge before other-party CODEOWNER and experience/soak gates. An additional RED→GREEN regression caught the newly asynchronous proxy fence waiting beyond the transport deadline. That read-only authority check now races the existing abort signal; socket cleanup and no-HTTP guarantees hold even when it stalls. Admitted Host writes remain supervised/drained, not abandoned. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b59275ea9a
ℹ️ 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 dotSubscriptionArguments = z | ||
| .object({ attempt_id: z.string().min(1).max(512), pairing_code: z.string().min(16).max(128) }) | ||
| .strict(); | ||
| export const dotReplyArguments = dotEventRefSchema.extend({ content: z.string().min(1).max(50000) }); |
There was a problem hiding this comment.
When content contains enough multibyte or JSON-escaped text (for example, 22,000 CJK characters), it passes this 50,000-code-unit limit and the matching public tool schema, but textPayload() produces one element whose serialized payload exceeds MESSAGING_BOUNDS.maxElementPayloadBytes (65,536 bytes in packages/plugin-contract/src/generated/contract.generated.ts). The Host then rejects messaging.send, after reply() has already persisted replyDigest and ask() has already persisted its reservation, so a shorter retry under the same event/client ID conflicts and the otherwise accepted operation cannot recover. Validate the serialized payload byte length (including the mentions prepended by ask) before either reservation and expose the same bound in the tool schemas.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c9e7ec5: final text payloads are validated with the public Contract validateMessagingSemantics and generated MESSAGING_BOUNDS before replyDigest/ask reservations. Ask schema validation and runtime share the same mention formatter, including prefix/newline/JSON-escape bytes. Standard JSON Schema cannot encode serialized UTF-8 bytes; its content descriptions now expose the canonical 65,536-byte limit and mention overhead, and Zod/runtime enforce it. Six targeted checks first failed (CJK/escaped reply and ask, prefix overflow, schema disclosure), then passed; shorter retries keep the same event/client ID, and exact-limit reply/ask payloads still succeed. Sibling caption/fixed-nonce send paths were checked; no Contract/SDK/Host API changes.
Why: code-unit length admits multibyte or JSON-escaped text beyond the Host element budget, poisoning reply/ask retries before rejection. Use the public Contract validator and a shared mention formatter before reservations, expose the exact byte budget in tools, and cover shortened retries plus exact-limit payloads.
|
@codex review Please independently review the exact PR head Please prioritize security/owner admission, source-linked reply authorization, image streams and package closure, lifecycle revocation, non-UI migration governance, and monotonic legacy revocation during CAS retries or receipt failure. The Host owns principal authentication/routing/legacy-media materialization; the package must not reintroduce a Pro AgentKey, protected member or Host source dependency. Check that declared body budgets change no authority and that old consumers keep their own pinned dependency identities. Accepted-Source-Ref: Author evidence: full canonical Review-Subject-Ref: R7 after R6: #95 (comment) is addressed. This is a new precondition-ordering family, not another pairing/owner lifetime gap. The complete audit in the implementation plan has been extended: reply/ask must validate their final serialized text before durable idempotency reservations; image caption and fixed-size nonce text paths were checked against the same bound. The package uses the existing public Contract validateMessagingSemantics('MessageDraft', ...) and generated MESSAGING_BOUNDS. A shared payload builder and ask mention formatter serve Zod admission and runtime construction. No Host schema/API/dependency changes or duplicated byte budget are introduced. Both UTF-8 multibyte text and JSON escaping count; ask includes its sorted/deduplicated mention prefix. Text payloads are validated before replyDigest or ask reservation writes, leaving the event/client ID usable for a shorter retry. JSON Schema cannot express the byte count of JSON serialization; the public Contract already treats this as semantic validation. The standard content.description exposes the generated 65,536 UTF-8 JSON byte bound and ask prefix overhead, while Zod and runtime enforce it. Existing 50,000-character caps remain; exact-limit payloads continue to work. Six tests were RED before the fix and GREEN here: CJK/JSON-escaped reply and ask, ask prefix overflow, and public schema budget disclosure. Two exact-65,536-byte reply/ask cases were already green and remain as compatibility checks. Existing successful request/reply/pairing/import/stream/lifecycle tests remain. Full canonical gate: 36 configured stages, Dot 88 tests; the same old Feishu lifecycle exemption is disclosed in the PR. The five-round brake remains honored: all R1-R5 findings and original scope were reread before R6, and this new R6 family was audited before R7. Please judge the complete authorized package and these input-precondition semantics on the exact new head. No production activation, source deletion, npm/catalog publication, self-approval or feature-close is claimed; real Host/account acceptance remains C-owned. |
|
Codex Review: Didn't find any major issues. 🎉 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". |
Why: plugin-private data survives uninstall while installed instance IDs change. Scope migration receipts to the target installation and allow only owner-fenced CAS rebinding of matching connection identity, retaining runtime evidence and first revocation. Exercise the dormant importer across stop/uninstall/reinstall, old credentials, legacy receipts, conflicts and interrupted retries.
|
@codex review Please review exact HEAD C's real Host installer/generic-webhook integration passed initial migration and old-credential tools/list, then failed disable -> uninstall -> reinstall (new instanceId) -> identical legacy import: batch-only receipt returned the old target; bypassing it would fail connection instance equality. Private storage intentionally survives uninstall. This change binds receipt lookup to target instance plus unchanged batchDigest and permits only current owner-operation-fenced CAS rebinding of matching owner/attempt/credential/key provenance/pairing code. Current runtime evidence and first revocation are preserved. Legacy unscoped receipts remain retained but cannot certify a new target. No connection in a batch means no relocation; public batch/receipt, Contract and SDK are unchanged. Ten new regression/compatibility cases in migration-reinstall.test.mjs exercise actual module settings/start/stop/uninstall/recreate/new instance, same source and original PNG/history, old credential tools/list (200 when active, 403 when retained revoked), old receipt retention, dispatch-only ordering, CAS contention, receipt crash/concurrent retries, immutable identity conflicts and obsolete owner fences. Initial three behavioral regressions failed on old code for the expected old-target/connection-conflict reasons; final migration/authority suite23/23 passed. Full gate36/36, Dot98/98 passed, including actual offline canonical archive installation/lifecycle. Canonical archives are regenerated for this exact source under pinned Node24.18.0/npm11.16.0; darwin/arm64 target only. Failure-mode sweep and original scope are in docs/plans/2026-10-20-f202-dot-plugin.md, including the complete R1-R5 authority audit, R6 text precondition audit and new retained-storage/instance-lifetime audit. No authority fallback, Host source edit, production catalog activation, runtime restart or legacy source deletion. Real Host re-acceptance belongs to C after independently reviewed bytes; formal CODEOWNER/operator/soak gates remain separate. PR stays draft. Review-Subject-Ref: pr:#95 [砚砚/gpt-6.1-sol🐾] |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
Why: recreating a module from one import graph does not exercise Host package rematerialization. Validate retained image import and a new MCP image publication through two physical copies of the canonical archive in one process. Use checksum-valid PNG fixtures; production implementation and archive bytes remain unchanged.
|
@codex review R9 exact HEAD: Please focus on the regression's truth: the canonical archive is installed offline into an empty consumer and also independently extracted into a second directory. One fresh Node process imports each physical root's DEFAULT entrypoint with its bundled dependency graph. It imports connection/dispatch/original PNG, enables and authenticates, stops/uninstalls, reimports with a new instance and current-target receipt, then enables and authenticates the same credential. The second materialization publishes a DIFFERENT new PNG via MCP and reads the exact original through plugin streams. This closes the previous test's gap of constructing a new module from one imported graph. The reported second-Sharp failure was withdrawn by C: its fixture had an incorrect IDAT CRC and invalid zlib checksum. Independently generated valid old/new PNGs pass both roots. The runtime was NOT weakened or redesigned to accommodate corrupt fixture bytes. This coverage passes on existing reviewed production bytes; no new product RED-to-GREEN is claimed. Evidence: pinned full gate36/36, Dot98/98, canonical offline test including both physical materializations. Implementation plan contains original scope, prior failure-pattern audits and this diagnosis. Risks: no production behavior/data/security/contract change and no irreversible operation; only new test assertions and evidence documentation. Real Manager HTTP acceptance was independently reported in thread_mvgmbp6scs7nd1ro#0001792538489772-002585-94878ac0. Broader Host/account/operator/soak and CODEOWNER gates remain separate. Keep PR draft; no production activation, restart, source deletion or self-merge. Review-Subject-Ref: pr:#95 [砚砚/gpt-6.1-sol🐾] |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
1 participant
Dot's pairing, cloud callbacks, source-bound replies, image publication and MCP tools currently belong to the Host. This adds the installable
official.chatgpt-dotpackage so the plugin owns that business while the Host owns authentication, installation identity, routing and media persistence.The private
@clowder-ai/chatgpt-dot@0.2.0-alpha.0package uses public SDK messaging, storage, secrets, lifecycle and streams. Its canonical archive physically bundles the exact production dependency closure. It adds no catalog entry or publishing workflow. The bundled native image dependency is validated on darwin/arm64; other platforms and a real paired Host/account journey still need acceptance.The coordinated public contract is Contract
0.1.0-beta.40with SDK0.2.0-beta.22:addressedDelivery.methodexplicitly opts an installation into addressed delivery through an existing action. The SDK validates and exposes that handler; absent declarations remain absent. Delivery envelopes and receipts are unchanged. Instance IDs, aliases and icons remain Host-owned.migration.methoddeclares a non-UI, Host-only dormant settings action.dot.importaccepts owner-bound legacy records and PNG bytes, never filesystem paths. It retains credentials, pairing, historical attempts and evidence while rebinding the installed instance, returns resumable receipts, and deletes no source data. Its storage port rechecks owner authority before and after every read/write, including cached receipts. CAS merges preserve newer runtime evidence and allow only active-to-revoked strengthening, retaining the first revocation even across contention and receipt failure.requestBodyLimitBytesdefaults to 1 MiB and permits positive integers up to the fixed 15 MiB Host cap.dot-mcpdeclares 15 MiB. The matching Host must enforce the per-contribution stream budget and retain header stripping; this PR changes declarations only.The declared MCP contribution is
dot-mcp, pathmcp, actiondot.rpc. Independent calls verifyx-dot-credential. Shared calls require the Host-created third-argument owner HTTP write context, bound to the current instance and owner. No Pro AgentKey or registry enters the package. Replies require reading the exact source first; receipt acknowledgement is distinct from model response. Images usepmr_references and verified streams into Host media storage. Request authority remains in a private async scope: existing Host ports check the context fence before and after calls, and streams capture it for later reads. Disable aborts transport and drains every admitted action, including asks and roundtrip tests. Pairing attempts are checked again across asynchronous work, follow-up dispatch remains bound to its matched question's pairing, and repeated disconnect preserves the first revocation. A failed start also revokes and settles settings/runtime resources before releasing ownership, allowing a clean retry. Source authorization rechecks pairing after both awaited Host lookups, and each original-image chunk rechecks the pairing again before yielding. A captured owner-and-pairing fence also reaches direct/proxy TLS-ready HTTP writes, verification challenges, immutable image publication and metadata CAS. No HTTP bytes precede the transport fence. Restart restores durable pairing and only subscriptions for the plugin's own questions. Uninstall retains user data.Existing plugins retain their published SDK/Contract pins. Catalog and fresh-consumer checks now validate each consumer's exact lock closure rather than assuming every installed plugin shares the newest workspace libraries. No existing catalog artifact has changed.
Verification: full
pnpm gate:ciunder pinned Node 24.18.0/npm 11.16.0; RED→GREEN regression evidence for schema/handler admission, migration retries and monotonic revocation, shared admission, stable delivery IDs and reply targeting. Dot tests include transport security, PNG streams, actual offline empty-cache installation of the canonical archive, default manifest entrypoint setup/enable/stop/restart/uninstall, and retained data. Component evidence does not claim real Host/ChatGPT acceptance.Architecture ownership: plugin package/external protocol moves here; A owns Host mention/queue delivery and C owns cutover, old-authority equivalence checks, shared remote relay, legacy Host media materialization and real acceptance. Neither Host source nor production data/runtime is changed by this PR. Keep this draft until independent review, other-party CODEOWNER approval, isolated experience acceptance and fork soak are satisfied. Do not publish or activate the production catalog.
Independently reviewed Head-Sha:
4798d4133d47d32c3b0d448ee88408b4c11e239aAccepted-Source-Ref:
thread_mu8dg6h7l2x4ohsk#0001792496572158-000196-0a0473acAccepted-Revision:
thread_mu8dg6h7l2x4ohsk#0001792496572158-000196-0a0473acImplementation and interface evidence:
docs/plans/2026-10-20-f202-dot-plugin.md.Review focus: credential/owner authority boundaries, monotonic migration under retries, source-linked replies and images, concurrent immutable PNG retention, declared lifecycle admission, and physical package closure. Behavior, persisted data, security and public contract all change; no irreversible operation is requested.
Canonical archives at the test-only source (darwin/arm64; byte-identical to independently reviewed 163dcc8; new exact-head review passed):
@clowder-ai/chatgpt-dot@0.2.0-alpha.0a5b71b4711df59c454fc0ae9120ac5fd43c9e78c5b98f4667a5d206b9ed5d8a2@clowder-ai/plugin-contract@0.1.0-beta.4037f765206d7a994942948088e772aee5448009c91818eb979a444b5d3acb5d30@clowder-ai/plugin-sdk@0.2.0-beta.22752f6c6dfa05ca53d6a6cca9ce0a3d48046bc02542baa08eaa78bce38d7d9b4dFull canonical gate: 36/36 configured steps; Dot: 98/98. The existing Feishu meeting-intake lifecycle pack exemption remains; Dot passed its real offline archive installation test.
After five review rounds, the full finding-pattern audit and original scope were reread before the sixth request; the recorded decision is in the implementation plan. Ten new RED→GREEN cases observe actual HTTP requests, canonical images and metadata, including real direct/proxy TLS, rather than only eventual errors.
Reply/ask now validate the final text element through the canonical Contract UTF-8/JSON byte validator before reserving retry identity. Ask uses one shared mention formatter for schema admission and publication. Public tool descriptions disclose the generated 65,536-byte bound and prefix overhead; no new Host schema is added. Six RED→GREEN failures and two exact-limit compatibility cases verify that invalid text remains retryable with the same event/client ID.
C integration found a retained-storage lifecycle gap after the previous reviewed HEAD: initial migration and old-credential tools/list passed, but uninstall/reinstall creates a new instance ID and an identical legacy batch returned the previous target receipt. The fix scopes receipts to target instance plus unchanged batchDigest and CAS-rebinds matching retained connections only under the current Host owner-operation fence. It preserves runtime evidence and first revocation; legacy receipts and original PNGs remain retained. Dispatch/image-only batches require an already rebound connection.
Ten added regression/compatibility cases exercise dormant settings plus start/stop/uninstall/new module/new instance, original credentials and data, old receipt retention, revoked credentials (403), CAS contention, receipt failure, stale retries and immutable identity conflicts. Three initial behavioral cases failed on the old code for the reported reasons; the final migration/authority suite passes23/23. The full failure-mode sweep is recorded in the implementation plan. Public Contract/SDK bytes and batch/receipt shape are unchanged. C must rerun its actual Host installer/generic-webhook journey against independently reviewed new archive bytes; no such acceptance is claimed here.
Previous independent no-findings at c9e7ec5 is historical evidence only and cannot approve this new fix. New exact-head review and CI passed as recorded below. Keep draft pending real Host/account acceptance, operator experience/soak and other-party CODEOWNER approval.
Independent cloud code review completed at 2026-10-20T22:54:24Z on exact HEAD
163dcc8d14cbd01d918dae73fdfdb8c5480f878cwith no findings and no unresolved findings: #95 (comment) . Both current-head CI runs passed: https://github.com/zts212653/clowder-ai-plugins/actions/runs/39510816806 and https://github.com/zts212653/clowder-ai-plugins/actions/runs/39510816662 . This R8 result covers prior source163dcc8 only. C subsequently passed its real Manager HTTP reinstall journey on those bytes; current test-only source4798d41 has now passed its own exact-head review and CI. CODEOWNER/account/operator/soak gates remain pending. PR stays draft.C's second-Sharp reload hypothesis was withdrawn after independent CRC/DEFLATE verification found an invalid PNG fixture. Valid PNGs pass both independently materialized packages. C corrected only its test fixture, retained package validation, and passed the actual old builtin -> owner Manager update/dormant import -> enable -> old credential tools/list200 -> disable/uninstall -> catalog reinstall/new instance -> reimport/new target receipt -> old credential200 journey. No runtime workaround was added.
The only delta after R8 is test/evidence coverage: the canonical archive is loaded through default entrypoints from two physical directories with bundled dependencies in one Node process. After first stop/uninstall, the second import retains original PNG/credential and processes a DIFFERENT new image through MCP publication and verified stream read. Production source, manifest, dependencies, Contract/SDK and all three canonical archive byte hashes are unchanged. This test already passes on reviewed production bytes; no additional product RED-to-GREEN is claimed. Full gate36/36 and Dot98/98 were rerun for this test-only source. The new coverage has passed independent exact-head review and current CI; the business-review evidence is retained through verified archive byte continuity.
Final-source R9 cloud review: exact HEAD
4798d4133d47d32c3b0d448ee88408b4c11e239a, completed2026-10-20T23:40:54.994557Z, no findings or unresolved findings, #95 (comment) . Current CI success: https://github.com/zts212653/clowder-ai-plugins/actions/runs/39515560131 and https://github.com/zts212653/clowder-ai-plugins/actions/runs/39515560108 . These are code/component/Host Manager acceptance facts, not formal CODEOWNER approval, full external-account acceptance or production release authority. PR remains draft.