[codex] Add harness contact handshake#206
Conversation
|
@senamakel is attempting to deploy a commit to the Vezures Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe SessionEnvelopePublisher now resolves and validates DM recipients through contact-status checks before sending, requesting contact acceptance when needed, and retrying sends on "not a contact" failures. Tests are expanded with a relay harness simulating contact status/request endpoints and covering both auto-accept and pending-contact scenarios. ChangesContact-Aware DM Sending
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Publisher as SessionEnvelopePublisher
participant Contacts as Contacts API
participant Relay
Publisher->>Publisher: resolveRecipientKey(dmRecipient)
Publisher->>Contacts: status()
alt not accepted
Publisher->>Contacts: request()
alt blocked or pending
Publisher-->>Publisher: throw error
end
end
Publisher->>Relay: sendMessage()
alt send fails ("not a contact")
Publisher->>Publisher: clear contactPromise
Publisher->>Contacts: ensureContact() again
Publisher->>Relay: sendMessage() retry
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: 99e7ac6928
ℹ️ 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".
| } | ||
|
|
||
| private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { | ||
| this.contactPromise ??= this.ensureContactNow(ctx); |
There was a problem hiding this comment.
In a long-running harness where the first envelope creates an outgoing contact request and the operator accepts it before later semantic messages, this ??= keeps the rejected “contact request pending” promise in contactPromise. Every later publish reuses that same rejection and never re-reads /contacts/.../status, so DM forwarding stays disabled until the wrapper restarts even though the relationship is now accepted. Clear the cached promise on rejection or cache only successful resolutions.
Useful? React with 👍 / 👎.
| } | ||
| this.contactPromise = undefined; | ||
| const refreshedRecipient = await this.ensureContact(ctx); | ||
| await sendMessage(ctx.client, ctx.signer, refreshedRecipient, JSON.stringify(envelope)); |
There was a problem hiding this comment.
When the relay returns not_a_contact for the first encrypted DM to a peer after ensureContact succeeds, the failed sendMessage has already run MessagesApi.send/SignalSession.encrypt, which stores and advances an outbound Signal session before the PUT response is known. Retrying here then sends a CIPHERTEXT instead of the required PREKEY_BUNDLE; a recipient that never received the rejected pre-key message has no session and will drop the retried envelope. Reset the peer Signal session or avoid encrypting until authorization is confirmed before retrying.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
sdk/typescript/tests/codex-cli.test.ts (2)
567-660: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
makeRelayinto route-handler helpers.The contact-route additions make this helper exceed the 20-line function limit. Consider extracting message, contact, and key handlers. As per coding guidelines, “Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/typescript/tests/codex-cli.test.ts` around lines 567 - 660, `makeRelay` now exceeds the 20-line limit because it contains all route handling inline. Refactor the request-dispatch logic in `makeRelay` into separate helper functions for messages, contacts, and key routes, and have the main relay handler delegate to those helpers while keeping `relay.contactRequests` initialization intact.Source: Coding guidelines
333-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAnnotate the new callbacks and extract setup helpers.
The new test callback and nested callbacks need explicit return types, and the test body is well over 20 lines. As per coding guidelines, “Use explicit return types for all functions in TypeScript” and “Limit functions to a maximum of 20 lines of code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/typescript/tests/codex-cli.test.ts` around lines 333 - 418, The new test in codex-cli.test.ts needs to satisfy TypeScript style rules by adding explicit return types to the main test callback and any nested callback functions (for example the relay spawn microtask and timeout handlers), and by reducing the oversized test body in the request-contact-approval scenario. Extract the repeated setup and session-writing logic into small helper functions near the existing test utilities so the test itself stays under 20 lines, while keeping the behavior anchored around the runTinyPlaceCli, makeRelay, and makeClient flow.Source: Coding guidelines
sdk/typescript/src/cli/harness-wrapper.ts (1)
611-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract part of
ensureContactNowto stay under the function-length limit.This new helper is over 20 lines. Extracting the repeated blocked/accepted status handling keeps the contact flow easier to scan. As per coding guidelines, “Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/typescript/src/cli/harness-wrapper.ts` around lines 611 - 639, `ensureContactNow` is over the function-length limit because it repeats the contact status checks inline; extract the accepted/blocked handling into a small helper and reuse it for both the pre-request and post-request `ctx.client.contacts.status` results. Keep the core flow in `ensureContactNow` focused on resolving the recipient, checking the signer, requesting contact, and delegating status interpretation to the new helper so the logic stays under 20 lines and easier to scan.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/typescript/src/cli/harness-wrapper.ts`:
- Around line 606-611: The new helper parameter name uses an unsupported
abbreviation. Update the `ensureContact` and `ensureContactNow` method
signatures in `harness-wrapper.ts` to use `context` instead of `ctx`, and rename
any matching references inside those helpers so the naming follows the coding
guidelines.
- Around line 606-608: Avoid caching failed contact checks in ensureContact;
contactPromise is currently set from ensureContactNow(ctx) before it settles, so
a rejection gets reused on later publishes. Update ensureContact in
harness-wrapper so only successful contact results are cached, and clear/reset
contactPromise when ensureContactNow rejects or throws. Use ensureContact and
ensureContactNow as the key locations to keep the retry behavior working after
recovery or approval.
In `@sdk/typescript/tests/codex-cli.test.ts`:
- Around line 666-667: The helper contactKey currently uses abbreviated
parameter names a and b, which violates the naming guideline. Rename the
parameters in contactKey to descriptive names that reflect the two contacts
being compared, and update the function body to use those new names
consistently.
---
Nitpick comments:
In `@sdk/typescript/src/cli/harness-wrapper.ts`:
- Around line 611-639: `ensureContactNow` is over the function-length limit
because it repeats the contact status checks inline; extract the
accepted/blocked handling into a small helper and reuse it for both the
pre-request and post-request `ctx.client.contacts.status` results. Keep the core
flow in `ensureContactNow` focused on resolving the recipient, checking the
signer, requesting contact, and delegating status interpretation to the new
helper so the logic stays under 20 lines and easier to scan.
In `@sdk/typescript/tests/codex-cli.test.ts`:
- Around line 567-660: `makeRelay` now exceeds the 20-line limit because it
contains all route handling inline. Refactor the request-dispatch logic in
`makeRelay` into separate helper functions for messages, contacts, and key
routes, and have the main relay handler delegate to those helpers while keeping
`relay.contactRequests` initialization intact.
- Around line 333-418: The new test in codex-cli.test.ts needs to satisfy
TypeScript style rules by adding explicit return types to the main test callback
and any nested callback functions (for example the relay spawn microtask and
timeout handlers), and by reducing the oversized test body in the
request-contact-approval scenario. Extract the repeated setup and
session-writing logic into small helper functions near the existing test
utilities so the test itself stays under 20 lines, while keeping the behavior
anchored around the runTinyPlaceCli, makeRelay, and makeClient flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78c8a93d-9912-4230-aa4b-0b7e5ce1b148
📒 Files selected for processing (2)
sdk/typescript/src/cli/harness-wrapper.tssdk/typescript/tests/codex-cli.test.ts
| private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { | ||
| this.contactPromise ??= this.ensureContactNow(ctx); | ||
| return this.contactPromise; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid caching rejected contact checks.
contactPromise is assigned before ensureContactNow settles, so a transient status/request failure—or a pending-contact throw—gets reused by every later publish without rechecking after recovery or approval.
Proposed fix
private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> {
- this.contactPromise ??= this.ensureContactNow(ctx);
+ this.contactPromise ??= this.ensureContactNow(ctx).catch((error: unknown) => {
+ this.contactPromise = undefined;
+ throw error;
+ });
return this.contactPromise;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { | |
| this.contactPromise ??= this.ensureContactNow(ctx); | |
| return this.contactPromise; | |
| private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { | |
| this.contactPromise ??= this.ensureContactNow(ctx).catch((error: unknown) => { | |
| this.contactPromise = undefined; | |
| throw error; | |
| }); | |
| return this.contactPromise; |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn as spawnChild } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/typescript/src/cli/harness-wrapper.ts` around lines 606 - 608, Avoid
caching failed contact checks in ensureContact; contactPromise is currently set
from ensureContactNow(ctx) before it settles, so a rejection gets reused on
later publishes. Update ensureContact in harness-wrapper so only successful
contact results are cached, and clear/reset contactPromise when ensureContactNow
rejects or throws. Use ensureContact and ensureContactNow as the key locations
to keep the retry behavior working after recovery or approval.
| private ensureContact(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { | ||
| this.contactPromise ??= this.ensureContactNow(ctx); | ||
| return this.contactPromise; | ||
| } | ||
|
|
||
| private async ensureContactNow(ctx: Awaited<ReturnType<typeof makeContext>>): Promise<string> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename ctx in the new helpers.
ctx is not in the abbreviation exception list; use context for the new helper parameter. As per coding guidelines, “Avoid abbreviations … exceptions: db, arg, args, env, fn, prop, props, ref, refs.”
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn as spawnChild } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/typescript/src/cli/harness-wrapper.ts` around lines 606 - 611, The new
helper parameter name uses an unsupported abbreviation. Update the
`ensureContact` and `ensureContactNow` method signatures in `harness-wrapper.ts`
to use `context` instead of `ctx`, and rename any matching references inside
those helpers so the naming follows the coding guidelines.
Source: Coding guidelines
| function contactKey(a: string, b: string): string { | ||
| return [a, b].sort().join("\0"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use descriptive contact-key parameter names.
a and b violate the abbreviation rule.
Proposed fix
-function contactKey(a: string, b: string): string {
- return [a, b].sort().join("\0");
+function contactKey(firstParticipant: string, secondParticipant: string): string {
+ return [firstParticipant, secondParticipant].sort().join("\0");
}As per coding guidelines, “Avoid abbreviations … exceptions: db, arg, args, env, fn, prop, props, ref, refs.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function contactKey(a: string, b: string): string { | |
| return [a, b].sort().join("\0"); | |
| function contactKey(firstParticipant: string, secondParticipant: string): string { | |
| return [firstParticipant, secondParticipant].sort().join("\0"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/typescript/tests/codex-cli.test.ts` around lines 666 - 667, The helper
contactKey currently uses abbreviated parameter names a and b, which violates
the naming guideline. Rename the parameters in contactKey to descriptive names
that reflect the two contacts being compared, and update the function body to
use those new names consistently.
Source: Coding guidelines
1 participant
Summary
not_a_contact.Validation
pnpm exec vitest run tests/codex-cli.test.tspnpm buildNotes
--no-verifyafter the monorepo pre-push hook failed in the unrelated website build:/sitemap.xmltimed out after three 60s attempts.Summary by CodeRabbit