feat(custom-engine): download artifacts.files[].url artifacts into the report#103
Open
zpzjzj wants to merge 2 commits into
Open
feat(custom-engine): download artifacts.files[].url artifacts into the report#103zpzjzj wants to merge 2 commits into
zpzjzj wants to merge 2 commits into
Conversation
…e report Replace the local-transport URL stub in collectArtifacts with a real, best-effort HTTP download: a SessionResult that declares a downloadable `url` is GET'd (http/https only) and written into the case artifact directory under `name`, then registered in GeneratedFiles. This completes the custom-engine artifact contract (PR-C, after the #99 JSON core). The download is bounded (256 MiB cap via io.LimitReader, per-download timeout via context) and best-effort: a non-2xx status, transport error, non-http(s) scheme, or over-cap body is logged and skipped without failing the run, consistent with the rest of artifact handling. A URL embedding the configured API key is refused up front (mirrors errSecretInURL on the http transport) and error strings are scrubbed via maskAPIKey. SSRF stays the engine/operator's responsibility, the same trust posture as the http transport. Handled transport-agnostically in collectArtifacts (any engine may declare a url per docs/design/custom-engine.md), so it is independent of the in-flight multipart PR which only touches custom_http.go.
Collaborator
Author
|
@codex review |
![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: ac643023c0
ℹ️ 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".
…facts The default http.Client follows 3xx redirects, so the archived bytes could come from a Location URL rather than the engine-declared f.URL — breaking the documented "exact declared URL" boundary and the up-front api-key/scheme checks, and letting an artifact host point the fetch at a different (possibly internal) endpoint. Set CheckRedirect to http.ErrUseLastResponse so a 3xx is returned as-is and falls through to the existing non-2xx skip path. Addresses Codex review on #103.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This is PR-C of the Custom Engine HTTP rollout (after #99 JSON core and #102 multipart upload). It implements downloading
SessionResult.artifacts.files[].urlartifacts into the report — the last unimplemented piece of the custom-engine artifact contract.Previously the
case f.URL != ""branch incollectArtifactsonly logged a debug stub ("... url is not downloaded in the local transport"). It now performs a real, best-effort HTTP download.How
downloadURLArtifact(new, incustom.go): GETs the declared URL and writes the body into the case artifact directory (opts.ArtifactDir) underf.Namevia the existingwriteLocalArtifact, then registers the local path inartifacts.GeneratedFiles. Wired intocollectArtifacts.io.LimitReader(maxArtifactDownloadBytes) and a per-download timeout via context (customArtifactDownloadTimeout). Mirrors themaxHTTPResponseBytes/io.LimitReader/maskAPIKeystyle introduced in feat(custom-engine): HTTP transport JSON request/response core #99'scustom_http.go.errSecretInURL), and error strings are scrubbed viamaskAPIKey. The download writes to the artifact output dir, never the workspace, so it bypassesf.Pathworkspace confinement by construction (writeLocalArtifactconfines the name to the dir basename).It is handled transport-agnostically in
collectArtifacts(per the design contract, any engine may declare aurl), not in the transport layer.Independent of #102
This PR touches a different code region (
collectArtifactsincustom.go) than the in-flight multipart PR #102 (custom_http.go). The only overlap is the trivial CHANGELOG[Unreleased]entry; rebase that line if #102 lands first.Docs
docs/design/custom-engine.md— "Agent artifact archiving boundary" and "Result bounds" sections updated; theurlrow is no longer "pending".CHANGELOG.md[Unreleased]— adds the URL-download entry and drops "URL artifact download" from the feat(custom-engine): HTTP transport JSON request/response core #99 follow-up list.Refs the custom-engine HTTP transport design (
docs/design/custom-engine.md).Tests
New
internal/agent/custom_artifact_url_test.go(httptest-driven, reusesnewCustomTestRuntimeetc.):GeneratedFileswith the right contentfile://) → skippedvar)engine.custom.envsecret embedded in the url → masked in the failure warningmake test(race),make verify(0 issues), and themake e2ecustom-engine subset all pass.🤖 Generated with Claude Code