Skip to content

Add audius.comms() host function to QuickJS sandbox#27

Open
glassBead-tc wants to merge 1 commit intomainfrom
worktree-agent-adb0ecce
Open

Add audius.comms() host function to QuickJS sandbox#27
glassBead-tc wants to merge 1 commit intomainfrom
worktree-agent-adb0ecce

Conversation

@glassBead-tc
Copy link
Copy Markdown
Owner

Summary

  • Adds audius.comms(method, path, options) host function to the QuickJS WASM sandbox, bridging to audiusClient.commsRequest() for comms/messaging endpoints
  • Extracts shared bridgeHostFn helper to eliminate duplication between request and comms host functions
  • Adds commsRequest to the AudiusClient interface with a stub implementation (real implementation comes from Unit 2A)

Test plan

  • TypeScript compilation passes (pnpm build)
  • pnpm test passes
  • E2E testing deferred until merge with Unit 2A (which provides the real commsRequest implementation)

🤖 Generated with Claude Code

Expose commsRequest bridge in the sandbox so LLM-generated code can call
Audius comms/messaging endpoints via audius.comms(method, path, options).
Extracts shared bridgeHostFn helper to eliminate duplication between
request and comms host functions. Adds commsRequest stub to AudiusClient
interface (real implementation comes from Unit 2A).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR adds an audius.comms(method, path, options) host function to the QuickJS sandbox, mirroring the existing audius.request() pattern for comms/messaging endpoints. It achieves this cleanly by extracting a shared bridgeHostFn helper that eliminates the prior duplication, and wires up a stub commsRequest on AudiusClient (always failing with a clear error) to be replaced by Unit 2A's real implementation.

Key changes:

  • AudiusClient interface and AudiusClientLive both gain commsRequest, currently a no-op stub that returns Effect.fail(new Error(\"Comms not implemented - see Unit 2A\")).
  • Sandbox.ts refactors the inline audius.request wiring into a reusable bridgeHostFn(name, clientMethod) helper, then uses it to register both audius.request and audius.comms.
  • The injected VM wrapper code gains the matching audius.comms = function(...) { return _origComms(...).then(JSON.parse) } shim.
  • TypeGenerator.ts is not updated to declare audius.comms() in the generated declare const audius type block — worth considering before Unit 2A ships.

The refactor is structurally sound: the bridgeHostFn closure captures audiusHandle by reference and is only called after audiusHandle is initialised, so there is no temporal-dead-zone issue. Error handling (resolving rather than rejecting with a { error } JSON envelope) is consistent with the existing request behaviour.

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style/documentation suggestions with no blocking correctness issues.

The code change is a clean refactor (extracting bridgeHostFn) plus a clearly-documented stub. No P0 or P1 issues were found: the forward reference to audiusHandle in bridgeHostFn is safe at runtime, error paths are consistent with existing behaviour, and the stub is correctly guarded behind Effect.fail. All comments are P2 (missing type declaration, JSDoc update, and a forward-looking SSRF reminder for Unit 2A).

No files require special attention; TypeGenerator.ts may need a minor update to expose audius.comms() in generated declarations before Unit 2A ships.

Important Files Changed

Filename Overview
src/api/AudiusClient.ts Adds commsRequest to the AudiusClient interface and service with a clearly-documented stub that always fails; clean addition, no issues.
src/sandbox/Sandbox.ts Extracts shared bridgeHostFn helper (good refactor), registers audius.comms alongside audius.request, and adds the matching JSON-parse wrapper in injected VM code; the audiusHandle forward-reference is safe at runtime since bridgeHostFn is only called after audiusHandle is declared.

Sequence Diagram

sequenceDiagram
    participant VM as QuickJS VM (user code)
    participant Bridge as bridgeHostFn (Sandbox.ts)
    participant Effect as Effect.runPromise
    participant Client as AudiusClient

    VM->>Bridge: audius.request(method, path, opts)
    Bridge->>Bridge: ctx.newPromise() → deferred
    Bridge->>Effect: runPromise(audiusClient.request(...))
    Effect->>Client: request(method, path, opts)
    Client-->>Effect: JSON response
    Effect-->>Bridge: resolve(JSON.stringify(result))
    Bridge->>Bridge: deferred.resolve(strHandle)
    Bridge->>VM: executePendingJobs()
    VM-->>VM: .then(JSON.parse) → parsed result

    VM->>Bridge: audius.comms(method, path, opts)
    Bridge->>Bridge: ctx.newPromise() → deferred
    Bridge->>Effect: runPromise(audiusClient.commsRequest(...))
    Effect->>Client: commsRequest (stub → Effect.fail)
    Client-->>Effect: Error("Comms not implemented")
    Effect-->>Bridge: reject handler fires
    Bridge->>Bridge: deferred.resolve({error: "Comms not implemented"})
    Bridge->>VM: executePendingJobs()
    VM-->>VM: .then(JSON.parse) → {error: "..."}
Loading

Comments Outside Diff (2)

  1. src/sandbox/TypeGenerator.ts, line 43-53 (link)

    P2 audius.comms() missing from generated type declarations

    The TypeGenerator hardcodes only audius.request() in the generated declare const audius block (line 51). Now that audius.comms() is a live host function in the sandbox, it should be declared here as well so LLM-generated code can discover and type-check it. Without this, callers relying on the injected __apiDecls__ string won't know audius.comms() exists.

    Suggested addition after line 51:

      comms(method: string, path: string, options?: RequestOptions): Promise<any>;
    

    This is intentionally deferred if the function is purposefully hidden until Unit 2A ships, but it's worth noting.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/sandbox/TypeGenerator.ts
    Line: 43-53
    
    Comment:
    **`audius.comms()` missing from generated type declarations**
    
    The `TypeGenerator` hardcodes only `audius.request()` in the generated `declare const audius` block (line 51). Now that `audius.comms()` is a live host function in the sandbox, it should be declared here as well so LLM-generated code can discover and type-check it. Without this, callers relying on the injected `__apiDecls__` string won't know `audius.comms()` exists.
    
    Suggested addition after line 51:
    
    ```
      comms(method: string, path: string, options?: RequestOptions): Promise<any>;
    ```
    
    This is intentionally deferred if the function is purposefully hidden until Unit 2A ships, but it's worth noting.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. src/sandbox/Sandbox.ts, line 1-17 (link)

    P2 Module JSDoc doesn't mention audius.comms()

    The module-level comment on line 6 still only lists audius.request() host function as a provided capability. Now that audius.comms() is also injected, the description should be updated to keep documentation accurate.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/sandbox/Sandbox.ts
    Line: 1-17
    
    Comment:
    **Module JSDoc doesn't mention `audius.comms()`**
    
    The module-level comment on line 6 still only lists `audius.request() host function` as a provided capability. Now that `audius.comms()` is also injected, the description should be updated to keep documentation accurate.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/sandbox/TypeGenerator.ts
Line: 43-53

Comment:
**`audius.comms()` missing from generated type declarations**

The `TypeGenerator` hardcodes only `audius.request()` in the generated `declare const audius` block (line 51). Now that `audius.comms()` is a live host function in the sandbox, it should be declared here as well so LLM-generated code can discover and type-check it. Without this, callers relying on the injected `__apiDecls__` string won't know `audius.comms()` exists.

Suggested addition after line 51:

```
  comms(method: string, path: string, options?: RequestOptions): Promise<any>;
```

This is intentionally deferred if the function is purposefully hidden until Unit 2A ships, but it's worth noting.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/sandbox/Sandbox.ts
Line: 1-17

Comment:
**Module JSDoc doesn't mention `audius.comms()`**

The module-level comment on line 6 still only lists `audius.request() host function` as a provided capability. Now that `audius.comms()` is also injected, the description should be updated to keep documentation accurate.

```suggestion
 * Provides isolated JavaScript execution with:
 * - audius.request() and audius.comms() host functions bridged to AudiusClient
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/api/AudiusClient.ts
Line: 102-103

Comment:
**Reminder: Unit 2A implementation will need SSRF path validation**

The `request` function guards against SSRF on line 49–51 (`path.startsWith("/")` and `!path.includes("@")`). When Unit 2A replaces this stub with a real implementation targeting a different base URL (e.g. comms endpoint), it should include equivalent path validation to maintain the same security boundary.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Add audius.comms() host function to Quic..." | Re-trigger Greptile

Comment thread src/api/AudiusClient.ts
Comment on lines +102 to +103
const commsRequest = (_method: string, _path: string, _options?: RequestOptions): Effect.Effect<unknown, Error> =>
Effect.fail(new Error("Comms not implemented - see Unit 2A"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Reminder: Unit 2A implementation will need SSRF path validation

The request function guards against SSRF on line 49–51 (path.startsWith("/") and !path.includes("@")). When Unit 2A replaces this stub with a real implementation targeting a different base URL (e.g. comms endpoint), it should include equivalent path validation to maintain the same security boundary.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/api/AudiusClient.ts
Line: 102-103

Comment:
**Reminder: Unit 2A implementation will need SSRF path validation**

The `request` function guards against SSRF on line 49–51 (`path.startsWith("/")` and `!path.includes("@")`). When Unit 2A replaces this stub with a real implementation targeting a different base URL (e.g. comms endpoint), it should include equivalent path validation to maintain the same security boundary.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df91a36de4

ℹ️ 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".

Comment thread src/api/AudiusClient.ts
Comment on lines +102 to +103
const commsRequest = (_method: string, _path: string, _options?: RequestOptions): Effect.Effect<unknown, Error> =>
Effect.fail(new Error("Comms not implemented - see Unit 2A"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Implement commsRequest or keep comms host disabled

This commit exposes audius.comms(...) in the sandbox, but commsRequest is hardcoded to fail for every invocation, so any comms call will never hit a real endpoint and always returns the placeholder error path. In production, this makes the newly added host function non-functional until a follow-up change lands; shipping the API surface before the implementation will break any workflow that starts using audius.comms.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant