feat(harness): add a generic artifact-offload module - #101
Conversation
Long-horizon runs accumulate context: summarising one oversized payload at a time shrinks each result but never stops the aggregate from growing, and it can never restore full fidelity. The fix is to put large results on disk and hand the next step a path. This lands the generic half of that convention as `harness::artifacts`, ported from OpenHuman's `agent/harness/artifact_offload/` under `plan-agents.md` Phase 5. It is the first family in that phase's order, chosen because its inbound coupling is the lowest of the set. Two host decisions are the whole reason this could not move as written, and both become injected policy rather than imports: - `ArtifactPathPolicy` — which paths are off limits. A host keeps internal state somewhere and an agent write must never land in it; only the host knows where that is. - `ArtifactRedactor` — what gets scrubbed before bytes touch disk. Credential and PII patterns are a host's compliance surface, not a library's. Both are gates the runtime calls, never behaviour the runtime is trusted to have performed (RFC section 2 rule 5). A `None` redactor stores bytes verbatim, which is documented as a security decision rather than an absence of one. Two things deliberately did NOT move: - The prompt contract. It is OpenHuman prompt text naming OpenHuman tools, which `plan-agents.md` section 6 lists as not publishable. It stays host-side, and `render_artifact_pointer` takes the read-tool name as a parameter for the same reason — a hard-coded tool name would put a tool the host may not have into its prompts. - The `is_workspace_internal_path` / `workspace_dir` split. It survives as two separate policy methods producing two distinct errors, because a host wants to tell "under the internal root" apart from "a specific state location" in a log. The specific check runs first so its more precise error wins when a path trips both. The symlink re-validation is preserved as-is and is the subtle part: the resolver's checks are necessarily lexical because the target does not exist when they run, so the real parent is re-checked after `create_dir_all` and before the write. `tokio`'s `fs` feature is now required. It enables more of tokio rather than adding a package, so the kernel-floor package count is unaffected. 44 tests, covering the happy path, the soft-failure path (a refused offload must never cost the caller its content), and the fail-closed hardening. The load-bearing one is that the abstract is built from the redacted body and never from the raw output — the pointer goes straight into a parent's context, so rendering it from raw text would re-expose the credential just scrubbed out of the file, while the file itself still looked correct. Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0856 · 114,057 in / 28,295 out · 73,069 cached (64%) · z-ai/glm-5.2
critique: $0.0413 · 33,570 in / 18,697 out · 24,997 cached (74%) · z-ai/glm-5.2
security: $0.0265 · 33,400 in / 5,481 out · 13,571 cached (41%) · z-ai/glm-5.2
tests: $0.0081 · 22,933 in / 1,691 out · 16,894 cached (74%) · z-ai/glm-5.2
description: $0.0098 · 24,154 in / 2,426 out · 17,607 cached (73%) · z-ai/glm-5.2
What this change touches8 files, +1603 -1 across 3 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/harness/artifacts<br/>6 files +1598 -0"]:::changed
n1["root<br/>1 file +4 -1"]:::changed
n2["src/harness<br/>1 file +1 -0"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p3
1 participant
Lands the generic half of the artifact-offload convention as
harness::artifacts, ported from OpenHuman'sagent/harness/artifact_offload/underplan-agents.mdPhase 5.It is the first family in that phase's order (
artifact_offload→run_queue→parse/tool_calling→subagent_runner→session/turn→session/{builder,runtime,types}), chosen because its inbound coupling is the lowest of the set: 4 call sites host-side.Independent of #100 — no shared files, no ordering constraint.
Why it could not move as written
Two decisions inside the module are host policy, and a redistributed crate cannot make either. They become injected traits rather than imports:
ArtifactPathPolicySecurityPolicy::{is_workspace_internal_path, workspace_dir}ArtifactRedactormemory::store::safety::sanitize_textBoth are gates the runtime calls, never behaviour the runtime is trusted to have performed — RFC §2 rule 5. That is why redaction is a trait returning the stored body rather than a
boolthe caller sets: callers surfacing any part of an artifact back into a model's context must render it from the returned value, and the API makes the raw input the inconvenient thing to reach for.A
Noneredactor stores bytes verbatim. That is a legitimate configuration and it is documented as a security decision rather than an absence of one, with a test pinning it so the default cannot quietly become "scrub something" and give false assurance.What deliberately did NOT move
plan-agents.md§6 lists as not publishable. It stays host-side. For the same reasonrender_artifact_pointertakes the read-tool name as a parameter — a hard-codedfile_readwould put a tool the host may not have into its prompts.is_workspace_internal_pathand theworkspace_dircontainment check survive as two separate policy methods producing two distinct errors, because a host wants to tell "under the internal root" apart from "a specific state location" in a log. The specific check runs first, so its more precise error wins when a path trips both — that ordering is load-bearing when a host has configured its artifact root inside its internal root, and it is pinned by a test.The subtle part, preserved as-is
The resolver's checks are necessarily lexical: the target usually does not exist when they run, so
canonicalizeis unavailable. That means a pre-existing symlink (outputs -> /elsewhere) would still be followed by the write. So the real parent is re-validated aftercreate_dir_alland before the write — the first moment the link-resolved location can be checked at all. This is carried over unchanged, with a test that asserts nothing was written through the link.Naming
action_dir→rootthroughout, since "action dir" is host vocabulary.relative_to_action_dir→relative_to_root;OffloadError::{WorkspaceTarget, WorkspaceInternal}→{InternalRoot, InternalState}, which say what they mean without naming a host concept.Dependency note
tokio'sfsfeature is now required. It enables more of tokio rather than adding a package, so OpenHuman's kernel-floor package count is unaffected.Verification
cargo test --all-features --lib— 1754 passed, 0 failed (44 new)cargo clippy --all-features --all-targets— cleancargo fmt --check— cleanThe tests worth reading, because each pins something that fails silently:
abstract_is_built_from_the_redacted_body_not_the_raw_outputoffload_failure_keeps_the_inline_payload_for_the_host_backstopwrite_refuses_a_parent_that_symlinks_out_of_the_convention_rootrejects_host_internal_state_paths_with_the_specific_errora_policy_free_resolve_still_enforces_containmentpolicy: Nonewidening traversal, not just the host checkswrite_without_a_redactor_stores_bytes_verbatimscratch_resolves_under_the_artifact_root_not_the_internal_rootworkspacethe subdir vsworkspacethe host state root — same word, different placesbuild_abstract_never_splits_a_multibyte_characterextract_artifact_paths_ignores_non_pointer_and_malformed_linespath=yielding the next field as the pathFollow-up
The host-side change — rewriting
agent/harness/artifact_offload/as a thin wrapper that supplies the two policies and keeps the prompt contract, in the same shaperun_queuealready uses — follows inopenhumanonce this merges and the vendor pointer can move.🤖 Generated with Claude Code