Conversation
|
Codex review: found issues before merge. What this changes: The PR adds SDK-facing Gateway Maintainer follow-up before merge: Protected maintainer-labeled MEMBER PR with a P2 behavioral blocker and an overlapping open artifact RPC alternative; this needs normal maintainer PR review and a chosen provenance contract, not an automated ClawSweeper repair lane. Security review: Security review cleared: Security review cleared: the diff adds read-scoped transcript artifact RPCs and SDK/docs/types without new dependencies, workflow changes, secrets handling, lifecycle hooks, package resolution changes, or server-side URL fetching. Review findings:
Review detailsBest possible solution: Land one canonical maintainer-approved artifact RPC implementation that keeps the read-scope gating, SDK/docs/protocol synchronization, and unsafe download guardrails, while either persisting and reading real run/task provenance before filtering or narrowing the public contract until that provenance exists. Do we have a high-confidence way to reproduce the issue? Yes. On PR head, read ordinary session messages through current Is this the best way to solve the issue? No, not as-is. The RPC direction is maintainable, but the advertised run/task query surface is not backed by persisted transcript provenance, so the safer fix is to add that provenance or defer/narrow those scopes. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e47a7448e90f. |
There was a problem hiding this comment.
Pull request overview
This PR hardens and completes the SDK-facing Gateway artifact RPC surface by enforcing explicit transcript provenance for artifact queries, improving download-source safety, and wiring the corresponding App SDK helpers/types/docs.
Changes:
- Adds Gateway RPC handlers for
artifacts.list/get/downloadthat project artifacts from session transcript media blocks with safer URL handling. - Extends Gateway protocol schemas/types, method discovery, and scope gating for the new artifact RPCs.
- Wires
oc.artifacts.*in@openclaw/sdk, updates docs, and updates generated Swift protocol models.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gateway/server-methods/artifacts.ts | New artifact RPC handlers + transcript artifact collection + download/source normalization/guardrails |
| src/gateway/server-methods/artifacts.test.ts | Adds unit coverage for list/get/download + runId scoping + URL safety behaviors |
| src/gateway/server-methods.ts | Registers artifactsHandlers into core gateway handler map |
| src/gateway/server-methods-list.ts | Adds artifact RPCs to advertised method list |
| src/gateway/method-scopes.ts | Gates artifact RPCs behind operator read scope |
| src/gateway/protocol/schema/artifacts.ts | Introduces protocol schemas for artifact query params and results |
| src/gateway/protocol/schema.ts | Re-exports artifact schema module |
| src/gateway/protocol/schema/protocol-schemas.ts | Registers artifact schemas into the protocol schema registry |
| src/gateway/protocol/schema/types.ts | Adds artifact-related SchemaType<> aliases |
| src/gateway/protocol/index.ts | Adds artifact param validators and exports types/schemas for protocol consumers |
| packages/sdk/src/types.ts | Adds SDK artifact query/result types and extends ArtifactSummary fields |
| packages/sdk/src/index.ts | Re-exports new SDK artifact types |
| packages/sdk/src/client.ts | Implements oc.artifacts.list/get/download() via RPC calls |
| packages/sdk/src/index.test.ts | Adds SDK-level test asserting artifact RPC calls are issued correctly |
| docs/concepts/openclaw-sdk.md | Documents oc.artifacts readiness and usage examples |
| apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift | Updates generated Swift models for new artifact protocol types |
| apps/macos/Sources/OpenClawProtocol/GatewayModels.swift | Updates generated Swift models for new artifact protocol types |
| CHANGELOG.md | Notes Gateway/SDK artifact RPC + helper availability |
|
@copilot apply changes based on the comments in this thread |
Applied all changes from the review thread in commit
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Fix taskId resolution: use getTaskById() from task registry to look up
requesterSessionKey instead of calling resolveSessionKeyForRun(taskId).
Falls back to resolveSessionKeyForRun(task.runId) when requesterSessionKey
is absent. Fixes artifacts.*({ taskId }) failing to find a session when
the taskId does not equal a sessionId/runId.
- Fix broken taskId test: replace wrong mock (loadSessionEntry with taskId),
incorrect field names (label → title), and stale messageTaskId sentinel.
Add getTaskById mock via vi.mock('../../tasks/task-registry.js').
- Add test for non-base64 data: URL in the content field to guard the
resolveBlockDownload() fix (directBase64 excludes data: strings).
- Make ArtifactsNamespace.list/get/download params required so SDK consumers
get a compile-time error instead of a runtime INVALID_REQUEST from the
Gateway when no scope is provided.
Agent-Logs-Url: https://github.com/openclaw/openclaw/sessions/6c0d8f8e-4a4e-4c73-8456-22f289ee0e09
Co-authored-by: BunsDev <68980965+BunsDev@users.noreply.github.com>
188a500 to
ef3d9c1
Compare
Summary
Duplicate triage
Validation
Closes #74706.
Refs #74898, #74769, #74804, #74786.