Add report-external-reference CLI subcommand to oz CLI - #14072
Conversation
Add a top-level `oz harness-support report-external-reference` subcommand (sibling of `report-artifact`) that lets third-party agent harnesses (Claude Code, Codex) report URL-addressable work products other than pull requests — filed Linear issues, spec docs, deployed previews — back to Oz as `EXTERNAL_REFERENCE` artifacts. Builds on the server-side `EXTERNAL_REFERENCE` artifact API already merged in warp-server PR #12887. Changes: - crates/warp_cli/src/harness_support.rs: add `ReportExternalReference` variant to `HarnessSupportCommand` with an `as_str_for_tracing` arm, and a `ReportExternalReferenceArgs` struct (`--url`, `--reference-type` both required; `--title` and `--metadata` optional). - app/src/ai/artifacts/mod.rs: add `ExternalReference` variant to the `Artifact` enum (`#[serde(rename = "EXTERNAL_REFERENCE")]`), the matching `ArtifactHelper` variant, and the deserialize arm so run responses carrying `EXTERNAL_REFERENCE` round-trip instead of falling through to the unknown/skip path. - app/src/ai/agent_sdk/harness_support.rs: import `ReportExternalReferenceArgs`, add the dispatch arm in `run()`, and add `report_external_reference()` which validates `--metadata` parses as a JSON object before any network call, builds `Artifact::ExternalReference`, and reports it via the existing `HarnessSupportClient::report_artifact` endpoint, printing `artifact_uid`. - app/src/ai/agent_sdk/mod.rs: add the telemetry mapping arm emitting `HarnessSupportReportArtifact { artifact_type: "external_reference" }`. - app/src/ai/artifacts/buttons.rs: add a no-op arm so the artifact-button match stays exhaustive (external references have no actionable button). - app/src/ai/agent_sdk/ambient.rs: render `ExternalReference` in the CLI run table so deserialized artifacts display. - Tests: 4 CLI parsing tests in crates/warp_cli/src/lib_tests.rs (required args parse, optional title parses, missing --url fails, missing --reference-type fails) and 3 wire-format tests in app/src/server/server_api/harness_support_tests.rs (serialize omits optional fields when None, includes them when present, round-trips). Headless CLI change — no UI; verified via cargo build/test and clippy on the changed crates. REMOTE-2253 Co-Authored-By: Oz <oz-agent@warp.dev>
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds the oz harness-support report-external-reference command, wires it through CLI dispatch and telemetry, adds the EXTERNAL_REFERENCE artifact variant with serialization/deserialization support, and updates CLI artifact display/tests for the new artifact type.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
spec_context.mdreports that no approved or repository spec context was found, so there were no material spec commitments to compare against.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
Adds the oz harness-support report-external-reference subcommand (sibling of report-artifact) plus the Artifact::ExternalReference enum variant, dispatch/telemetry/render arms, and CLI + wire-format tests. The core implementation is clean, mirrors the existing report_artifact handler, and CI is fully green — Linux/macOS/Windows test suites, clippy, and formatting all pass (21 successful, 0 failing). The wire-format tests confirm EXTERNAL_REFERENCE serializes to the server schema (omitting title/metadata when None) and round-trips through deserialize, and the CLI parsing tests cover required/optional args plus the missing---url/--reference-type clap errors — matching spec validation #6 and #7.
Spec alignment
Compared against the approved spec (warp-server PR #12977, agents/specs/REMOTE-2170). Most commitments are met: the top-level sibling command shape (Option A), the Artifact::ExternalReference variant for run-response round-trip deserialization, telemetry reusing HarnessSupportReportArtifact { artifact_type: "external_reference" }, CLI run-table rendering in ambient.rs, and the no-op button arm. The as_str_for_tracing arm returns the full "harness-support report-external-reference" path, matching the sibling commands (the spec's bare "report-external-reference" was a simplification) — correct. The metadata-object validation (reject non-object/malformed JSON before the network call) is implemented correctly for the inline case.
One material gap: spec behavior invariant #3 commits --metadata to accept BOTH an inline JSON object string AND a @file path whose contents are parsed as a JSON object (--metadata @meta.json). The implementation handles inline JSON only; a @file value is parsed as a literal string and rejected with --metadata must be a JSON object, so the documented @file usage is broken. This also leaves spec validation #8 (assert @file and inline both parse to the same object) unmet. The PR body doesn't disclose or justify the omission.
Concerns
The missing @file support is the only blocking issue. Two smaller, non-blocking follow-ons: (1) the metadata-parsing logic is a distinct code path with no test — since the handler needs heavy AppContext/ModelHandle context (and no mock-client handler tests exist for the sibling report_artifact/notify_user commands), the pragmatic path is to extract metadata parsing into a small pure helper (e.g. fn parse_metadata(raw: &str) -> Result<Option<Value>>) and unit-test the @file/inline/non-object/malformed cases directly, which also satisfies the testable half of spec validation #8; (2) spec open question #3 resolved --reference-type (primary) plus a hidden --type clap alias for ergonomics — only --reference-type is wired today. The --type alias is a judgment call since the spec itself flagged a clash risk with other --type flags in the wider CLI, so I'd leave that to the requester's preference.
The full-workspace ./script/presubmit OOMing in the sandbox is acknowledged in the PR body and is acceptable: CI ran the complete gate (build + clippy + nextest on three platforms) green, which is the authoritative signal per factory-verification.
Verdict
Found: 0 critical, 1 important, 2 suggestions
Request changes — add @file metadata support so --metadata @meta.json reads and parses the file (spec behavior invariant #3); the inline comment has a concrete suggestion. The rest of the change is solid and should merge quickly once this is addressed.
There was a problem hiding this comment.
Overview
Re-review of the oz harness-support report-external-reference subcommand against the updated spec (warp-server PR #12977, commit 1a37ceda on agents/specs/REMOTE-2170). The implementation adds the top-level sibling command, the Artifact::ExternalReference enum variant (serialize + deserialize + helper), the report_external_reference() handler with inline-JSON metadata validation, the telemetry arm, the ambient-runner display arm, and the no-op button arm. CI is fully green (Linux/macOS/Windows tests, clippy, formatting, release-flag compilation all pass). The wire-format tests confirm EXTERNAL_REFERENCE serializes to the server schema (omitting title/metadata when None) and round-trips through deserialize; CLI parsing tests cover required/optional args and the missing---url/--reference-type clap errors (spec validation #6 and #7).
Spec alignment (updated spec)
The prior review cycle's single blocking finding — missing --metadata @file support — is rescinded: the spec was updated in commit 1a37ceda to drop @file support. The updated spec now states (Design alternatives): "Metadata input format — Inline JSON string only ... @file support was considered but dropped to keep the implementation minimal"; behavior invariant #3 and open question #4 both commit to inline JSON only; open question #9 requires non-object metadata to be rejected client-side before the network call. The implementation matches exactly: serde_json::from_str + is_object() check, both before runner.update/ctx.spawn (i.e. before any network call), erroring with --metadata must be a JSON object for malformed JSON and for non-object values (arrays, strings, numbers, null). The remaining spec commitments are met: top-level sibling command shape (Option A); Artifact::ExternalReference variant for run-response round-trip deserialization; telemetry reusing HarnessSupportReportArtifact { artifact_type: "external_reference" }; as_str_for_tracing returns the full harness-support report-external-reference path (consistent with siblings); ambient-runner display arm; no-op button arm (reverse-lookup artifacts have no action button).
Concerns
No blocking issues. Two non-blocking suggestions for a follow-up:
-
Metadata-validation unit test (suggestion). The metadata-parsing logic (inline-JSON parse + non-object rejection) is the one piece of novel behavior in this PR and has no direct test — the serialization tests cover the
Artifactstruct with already-parsed values, and the CLI parsing tests only assert the raw--metadatastring is retained. Since the handler needs heavyAppContext/ModelHandlecontext (and no mock-client handler tests exist for the siblingreport_artifact/notify_usercommands), the pragmatic path is to extract metadata parsing into a small pure helper (e.g.fn parse_metadata(raw: &str) -> Result<Option<serde_json::Value>>) and unit-test the inline-object / non-object / malformed cases directly. This would also satisfy the testable half of spec validation #8. Inline comment below. -
Optional
--typehidden alias (suggestion). Spec open question #3 resolved--reference-type(primary) plus a hidden--typeclap alias for ergonomics; only--reference-typeis wired today. The spec itself flagged a clash risk with other--typeflags in the wider CLI, so this is genuinely a judgment call — leaving it to the requester's preference.
Verdict
Found: 0 critical, 0 important, 2 suggestions
Approve — the change is correct against the updated (inline-only) spec, mirrors the existing report_artifact handler, and CI is green. The two suggestions are non-blocking follow-ups. The prior @file finding is rescinded by the spec update (the @file review thread is resolved with the rationale below).
| ) -> Result<()> { | ||
| let metadata = match args.metadata { | ||
| Some(raw) => { | ||
| let parsed: serde_json::Value = serde_json::from_str(&raw) |
There was a problem hiding this comment.
💡 [SUGGESTION] (non-blocking) This metadata-parse + non-object check is the PR's only novel logic and isn't directly tested — the serialization tests cover the Artifact struct with already-parsed values, and the CLI parsing tests only assert the raw --metadata string is retained. Consider extracting this into a small pure helper (e.g. fn parse_metadata(raw: &str) -> Result<Option<serde_json::Value>>) so the inline-object / non-object / malformed cases can be unit-tested directly, without the AppContext/ModelHandle the handler needs (no mock-client handler tests exist for the sibling report_artifact/notify_user commands). This also satisfies the testable half of spec validation #8. Not a merge blocker.
|
|
||
| /// Reference type (required, max 256 chars, e.g. "LINEAR_ISSUE", "GITHUB_PR"). | ||
| #[arg(long = "reference-type")] | ||
| pub reference_type: String, |
There was a problem hiding this comment.
Will there be a fixed set of reference types? If so, we could add a clap enum so that agents can use --help output to discover options. On the other hand, that'd mean we'd need a client release to add new reference types, so maybe not worth it
There was a problem hiding this comment.
There's not a fixed set at the moment. I think the best thing we might be able to do is create a set of reference types associated with our managed MCP implementations that we can have first class support for. Since we want to support the agent being able to produce whatever outputs it needs, there will end up being some open-endedness here. I can noodle more on how to create a more finite set here.
| reference_type: args.reference_type, | ||
| url: args.url, | ||
| title: args.title, | ||
| metadata, |
There was a problem hiding this comment.
do we apply any restrictions on the shape of the object here? or is that something we leave to server-side code?
i guess the same question would apply to reference_type
There was a problem hiding this comment.
I shared some thoughts on reference_type in #14072 (comment).
The metadata object is intentionally unstructured at the moment (other than needing to be a key:value object).
This PR adds a top-level
oz harness-support report-external-referencesubcommand (a sibling ofreport-artifact) that lets third-party agent harnesses (Claude Code, Codex) report URL-addressable work products other than pull requests — filed Linear issues, spec docs, deployed previews — back to Oz asEXTERNAL_REFERENCEartifacts. This is thewarpdotdev/warp(CLI) half of REMOTE-2170; it builds on the server-sideEXTERNAL_REFERENCEartifact API already merged in warp-server PR #12887.Originating thread: https://warpdotdev.slack.com/archives/C0BDQDW8V5E/p1784479516355729
Conversation: https://staging.warp.dev/conversation/4969059e-4545-4008-af98-6b893977e2bb
Run: https://oz.staging.warp.dev/runs/019f82ef-e95d-786e-a61e-1e4a6112db73
This PR was generated with Oz.