Skip to content

feat(harness): allow direct artifact delivery without host buffering - #3031

Open
q827505931 wants to merge 3 commits into
agentscope-ai:mainfrom
q827505931:codex/direct-artifact-delivery
Open

feat(harness): allow direct artifact delivery without host buffering#3031
q827505931 wants to merge 3 commits into
agentscope-ai:mainfrom
q827505931:codex/direct-artifact-delivery

Conversation

@q827505931

Copy link
Copy Markdown

AgentScope-Java Version

2.0.3-SNAPSHOT

Description

deliver_artifact currently downloads every artifact into a host JVM byte[] before calling the application target. A sandbox capable of uploading directly must therefore send large files through the host first.

Add an opt-in DirectArtifactDeliveryTarget and metadata-only ArtifactDeliverySource. The tool dispatches direct targets with the active filesystem and runtime context before any download. Applications can perform the upload inside their sandbox using a backend SDK or shell. Existing byte-based targets, lambdas, and builder configuration remain compatible.

Both paths share filename validation and success/error/conflict rendering. Direct failures never trigger an in-memory fallback. Source checks, backend path resolution (including overlays/routing), destination conflict enforcement, and actual transport remain application responsibilities; this PR does not add a provider-specific uploader. English and Chinese docs include an integration example and explain these boundaries.

Closes #3030

Validation

  • mvn -pl agentscope-harness -am spotless:apply test -Dtest=ArtifactDeliveryToolTest,HarnessAgentTest -Dsurefire.failIfNoSpecifiedTests=false
  • 67 tests passed (18 artifact delivery tests, 49 HarnessAgent tests), zero failures/errors/skips. The compile phase also runs Spotless check.
  • New tests cover direct delivery without filesystem reads, normalized metadata/context propagation, defaults, rejected input, and failure/conflict/null results without fallback. Existing byte-delivery regressions pass.
  • git diff --check passed.
  • No live sandbox/object-store integration test was run; the transport is application-defined.

Checklist

  • Code has been formatted with mvn spotless:apply
  • All tests are passing (mvn test) — focused reactor tests above passed; full repository suite not run
  • Javadoc comments are complete and follow project conventions
  • Related documentation has been updated
  • Code is ready for review

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...scope/harness/agent/tool/ArtifactDeliveryTool.java 85.71% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Introduces an opt-in DirectArtifactDeliveryTarget SPI plus a metadata-only ArtifactDeliverySource, so a sandbox can upload large artifacts without pulling bytes through the host JVM. Clean, backwards-compatible design: the existing byte path is untouched, the tool keeps result == null handling, and no fallback on failure is the right default. Tests cover the no-download guarantee, defaults, and the invalid-input short-circuit. Left two non-blocking notes on SPI discoverability and the source-existence contract.


Automated review by github-manager-bot

@CLAassistant

CLAassistant commented Sep 11, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@q827505931
q827505931 force-pushed the codex/direct-artifact-delivery branch from ce02173 to 399986c Compare September 11, 2026 10:12
@q827505931

Copy link
Copy Markdown
Author

Addressed the two review notes in 399986c: direct targets are now explicitly documented as supported only through the registered deliver_artifact tool, and the source contract now states that filePath is not guaranteed to exist and that implementations must validate existence, size limits, and overwrite/conflict behavior. I also added a regression test for accidental use of the inherited byte-based entry point. The focused suite passes with 68 tests.

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Re-review after the follow-up commits — both points from the previous pass are resolved, and resolved in the way that fits an SPI best: the contract is now written down instead of being left to inference. DirectArtifactDeliveryTarget's type javadoc states that direct targets are supported only by the framework's deliver_artifact tool and that the inherited byte-based deliver returns a failure because it has no source filesystem, the @param source javadoc says the normalized path is not guaranteed to exist and puts existence/size/force-conflict enforcement on the implementation, HarnessAgent.Builder#artifactDeliveryTarget repeats the restriction where a target is wired, and both the EN and ZH sandbox.md sections carry the same caveats (no automatic byte fallback after a failure, normalized path ≠ sandbox-native path, quote paths for shell uploads, keep credentials out of tool output). The added directTarget_byteBasedEntryPointReturnsFailure test pins the documented behavior rather than just the happy path.

Findings

None blocking. Nothing in the new delta changes the dispatch logic itself, so the previously-reviewed behavior (instanceof dispatch, no fallback, validation before either branch) stands as reviewed.

Suggestions

Two optional follow-ups, neither worth holding the PR for:

  • If the direct path turns out to need a size cap in practice, the framework has no lever today since it never sees the bytes — a long maxBytesHint() on the SPI (defaulting to unlimited, documented as advisory) would give a later enforcement point without breaking this contract.
  • A //noinspection-style pointer from ArtifactDeliveryTarget's javadoc to DirectArtifactDeliveryTarget would make the two implementations discoverable from either side; right now only the direct interface cross-references both.

CI is green on the current head and the CLA is signed. Thanks for the quick, thorough turnaround on the feedback, and for keeping the docs and ZH/EN guides in step with the code.


Automated review by github-manager-bot

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.

feat(harness): support sandbox-side artifact delivery without host byte buffering

3 participants