From cf94f8879191d992adfeff37881027f7db5590a1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 7 Apr 2026 17:18:42 -0400 Subject: [PATCH] plans --- .../2026-04-07-global-cas-blob-refs-plan.md | 209 ++++++++++++++++ ...-07-store-dump-server-owned-export-plan.md | 235 ++++++++++++++++++ 2 files changed, 444 insertions(+) create mode 100644 docs/plans/2026-04-07-global-cas-blob-refs-plan.md create mode 100644 docs/plans/2026-04-07-store-dump-server-owned-export-plan.md diff --git a/docs/plans/2026-04-07-global-cas-blob-refs-plan.md b/docs/plans/2026-04-07-global-cas-blob-refs-plan.md new file mode 100644 index 000000000..417a0f2e8 --- /dev/null +++ b/docs/plans/2026-04-07-global-cas-blob-refs-plan.md @@ -0,0 +1,209 @@ +# Global CAS Blob Refs Plan + +## Summary + +Replace durable offload pointers with global content-addressed blob refs and keep file paths as an execution-only concern. + +- Automatic large-value offload should persist `blob://sha256/` refs instead of `file://...` paths. +- Blob bytes should be stored in a global CAS namespace rather than under per-run keys. +- Handlers and preamble generation should continue to see file references, but those files should be materialized only in the execution-local environment. +- Host scratch blob cache files should stop being part of the durable contract. +- Garbage collection is explicitly deferred in this pass. + +## Problem Frame + +Large context values are currently offloaded by writing the serialized JSON bytes to the run store, materializing a host-side cache file, and replacing the original value with a `file://` pointer to that file. + +That shape creates the wrong durable boundary: + +- persisted checkpoints and checkpoint-completed events contain host- or sandbox-specific file paths instead of stable storage references +- resume seeds those path strings back into runtime state verbatim +- remote sandbox sync rewrites durable context into sandbox-local `file://` paths +- fork semantics are awkward because the same logical content becomes tied to a specific run and a specific materialized file path + +The durable source of truth should be the blob bytes addressed by content hash. File paths should only exist as a temporary execution detail for agents and command handlers. + +## Key Decisions + +- Use `blob://sha256/` as the only new durable blob reference format. + - `RunBlobId` remains the existing SHA-256 hex content hash type in this pass. + - Durable run state, checkpoints, and checkpoint-completed event payloads should persist only plain JSON values and `blob://` refs. + +- Make blob storage global CAS instead of run-scoped. + - Internally, blob bytes move from per-run keys to global keys such as `blobs#sha256#`. + - Existing run-scoped server endpoints can remain as the API surface for now, but they should read and write the global CAS store under the hood. + +- Keep blob handling invisible to the model. + - The model should continue to receive file references in prompts and preambles. + - `blob://` is a backend durability protocol, not a model-facing protocol. + +- Materialize blobs only in execution-local views. + - Before handler execution and before preamble construction, resolve `blob://` refs into local files for the active sandbox. + - For remote sandboxes, materialize under `{working_directory}/.fabro/blobs/.json`. + - For local execution, materialize under a run-local ephemeral runtime directory such as `runtime/blobs/.json`. + +- Do not persist execution-local `file://` refs back into durable state. + - Managed materialized blob file refs must be normalized back to `blob://sha256/` before context snapshots are emitted or checkpointed. + +- Preserve compatibility with older runs. + - Read paths should continue to recognize legacy blob-backed `file://.../.json` values. + - New writes should use only the `blob://` form. + +- Leave stage artifacts unchanged. + - This change applies only to offloaded run blobs. + - `ArtifactStore` remains the durable system for captured stage artifacts. + +- Defer GC. + - New blob writes are append-only in this pass. + - No mark-and-sweep, refcounting, or retention enforcement is included here. + +## Implementation Changes + +### 1. Blob Storage And Blob Ref Helpers + +- Keep `RunBlobId` unchanged in `lib/crates/fabro-types/src/run_blob_id.rs`. +- Add a shared blob-ref helper module in `fabro-workflow` or `fabro-types` that: + - formats `blob://sha256/` + - parses `blob://sha256/` + - recognizes legacy blob-backed `file://.../.json` + - extracts blob ids from managed materialized blob file paths +- Change `fabro-store` blob key construction from `blobs#{run_id}#{blob_id}` to a global key layout such as `blobs#sha256#`. +- Update `RunDatabase::write_blob` and `RunDatabase::read_blob` to operate on the global CAS namespace. +- Remove blob enumeration from the main architecture path. `list_blobs` should not be part of new feature work; if retained temporarily, it should be treated as legacy/debug-only. + +### 2. Automatic Offload + +- In `lib/crates/fabro-workflow/src/artifact.rs`, change `offload_large_values` so that it: + - serializes the JSON value + - writes the bytes to CAS through the existing run-store handle + - replaces the value with `blob://sha256/` + - does not write a host-side cache file +- Remove the current assumption that `cache/artifacts/values/{blob_id}.json` is part of the durable contract. +- Keep the offload threshold unchanged at 100KB in this pass. + +### 3. Execution-Time Materialization + +- Replace `sync_artifacts_to_env` with an execution-time blob materialization flow that handles both: + - new `blob://` refs + - existing explicit or legacy `file://` refs +- Split this into two responsibilities: + - blob resolution and materialization for managed blob refs + - existing file-copy behavior for explicit `file://` refs that are not blob-backed +- Materialization behavior: + - read the blob bytes from CAS + - write the JSON bytes into a sandbox-usable file path + - return a rewritten execution-local value using `file://` +- Managed materialized paths should use a deterministic layout based on blob id so repeated refs dedupe naturally within an execution. + +### 4. Context And Durable Snapshot Boundaries + +- Introduce a clear split between: + - durable context values + - execution-local resolved context values +- Before handler execution, create a resolved execution view where `blob://` refs are rewritten to materialized `file://` refs. +- After handler execution, normalize any managed materialized blob file refs in handler-produced context changes back to `blob://sha256/`. +- Compatibility reads should normalize legacy blob-backed `file://.../.json` values to `blob://sha256/` in memory before they enter new durable snapshots. +- Update checkpoint creation and checkpoint-completed event emission so they snapshot only durable values, never execution-local materialized paths. +- Treat `current.preamble` as runtime-only derived state and exclude it from persisted context snapshots. This prevents preamble strings containing execution-local file paths from leaking into checkpoints or event payloads. + +### 5. Preamble And Handler Execution + +- Keep blobs invisible to the model. +- Before fidelity builds `current.preamble`, resolve completed-stage outcome values and current context into an execution-local view that contains file refs, not blob refs. +- `build_preamble` should continue to work with file references and should not mention `blob://` or “blobs” in user/model-facing output. +- Prompt and agent handlers should continue to consume `context.preamble()` and file references exactly as they do now. +- The only new behavior for handlers should be that the file refs they receive come from execution-time materialization rather than from durable checkpoint state. + +### 6. Resume And Fork Behavior + +- Resume should seed durable `blob://` values from checkpoint state and let the next execution hop materialize them as needed. +- Legacy checkpoints containing blob-backed `file://.../.json` values should be normalized on read so resumed runs persist the new `blob://` form on the next checkpoint. +- Fork should copy checkpoint and run state without copying blob payloads. +- Child runs should retain the same `blob://sha256/` refs as the source run. + +### 7. CLI And Export Behavior + +- Update CLI final-output rendering so when `response.*` is a blob ref it resolves the blob through the run-store read path before printing markdown. +- Keep explicit non-blob file refs as plain file references in CLI output. +- Refactor `store dump` to become reference-driven for blobs: + - scan exported JSON structures for blob refs + - fetch only referenced blobs + - hydrate them inline in exported JSON + - stop emitting a top-level `blobs/` directory +- Preserve the current export layout for run metadata, nodes, retro output, checkpoints, events, and stage artifacts. + +### 8. Server And API Surface + +- Keep the existing run-scoped blob routes: + - `POST /api/v1/runs/{id}/blobs` + - `GET /api/v1/runs/{id}/blobs/{blobId}` +- Change their implementation to use global CAS storage internally. +- Do not add public blob enumeration or global blob-fetch routes in this pass. +- Do not add GC or blob-membership verification to the API contract in this pass. + +## Test Plan + +### Blob Ref Helpers + +- parse and format `blob://sha256/` +- recognize legacy blob-backed `file://.../.json` +- reject ordinary non-blob `file://` refs +- normalize managed materialized blob file refs back to blob refs + +### Offload And Persistence + +- large values are replaced with `blob://sha256/` +- offload writes the blob bytes to CAS +- offload no longer creates a host scratch cache file +- small values remain inline +- checkpoint and checkpoint-completed payloads persist `blob://` refs, not `file://` +- `current.preamble` is excluded from persisted context snapshots + +### Execution Materialization + +- local execution materializes `blob://` refs to local runtime files +- remote execution materializes `blob://` refs to sandbox files under `.fabro/blobs/` +- preamble generation receives file refs and does not expose `blob://` +- handlers receive file refs and can read them normally +- explicit non-blob `file://` refs keep their existing remote-copy behavior + +### Normalization And Compatibility + +- handler-produced managed materialized blob file refs are normalized back to `blob://` before checkpointing +- legacy blob-backed `file://.../.json` values are normalized to `blob://` on resume +- ordinary explicit `file://` refs are preserved as file refs +- resumed runs re-checkpoint using only the new `blob://` form + +### Fork, CLI, And Export + +- forked runs reuse the same `blob://sha256/` refs with no blob copy +- CLI final-output rendering resolves blob-backed final responses +- `store dump` hydrates referenced blobs inline +- `store dump` emits no top-level `blobs/` directory + +## Important Files + +- `lib/crates/fabro-workflow/src/artifact.rs` +- `lib/crates/fabro-workflow/src/lifecycle/artifact.rs` +- `lib/crates/fabro-workflow/src/lifecycle/fidelity.rs` +- `lib/crates/fabro-workflow/src/node_handler.rs` +- `lib/crates/fabro-workflow/src/handler/llm/preamble.rs` +- `lib/crates/fabro-workflow/src/pipeline/execute.rs` +- `lib/crates/fabro-workflow/src/records/checkpoint.rs` +- `lib/crates/fabro-workflow/src/lifecycle/event.rs` +- `lib/crates/fabro-store/src/keys.rs` +- `lib/crates/fabro-store/src/slate/run_store.rs` +- `lib/crates/fabro-server/src/server.rs` +- `lib/crates/fabro-cli/src/server_client.rs` +- `lib/crates/fabro-cli/src/commands/run/output.rs` +- `lib/crates/fabro-workflow/src/run_dump.rs` +- `docs/execution/context.mdx` +- `docs/agents/outputs.mdx` + +## Assumptions And Defaults + +- The durable blob ref format for this pass is `blob://sha256/`. +- `RunBlobId` remains the current type name even though blobs are no longer run-scoped. +- Global CAS uses the existing durable key-value store rather than introducing a new blob backend. +- Existing run-scoped blob HTTP routes remain the only supported transport surface in this pass. +- Blob lifecycle management and GC are explicitly deferred. diff --git a/docs/plans/2026-04-07-store-dump-server-owned-export-plan.md b/docs/plans/2026-04-07-store-dump-server-owned-export-plan.md new file mode 100644 index 000000000..bf6664111 --- /dev/null +++ b/docs/plans/2026-04-07-store-dump-server-owned-export-plan.md @@ -0,0 +1,235 @@ +# Store Dump Server-Owned Export Plan + +## Summary + +Align `fabro store dump` with the intended architecture by making it a pure server-backed export command. + +- The CLI should resolve runs from server summaries only. +- It should fetch hydrated run state, hydrated event history, and artifacts over HTTP. +- It should stop creating a temporary `Database`. +- It should stop reading local scratch directories or local storage/object-store paths. +- It can keep the current export-style output layout on disk, except blob storage should stay transparent: + - top-level metadata files + - `nodes/**` + - `retro/**` + - `events.jsonl` + - `checkpoints/**` + - `artifacts/**` + +This is a debugging export, not a strict snapshot mechanism. Best-effort consistency is acceptable. + +## Problem Frame + +`fabro store dump` is still implemented as a local store reconstruction flow: + +- it resolves the run through local-storage-aware helpers +- it fetches events over HTTP but replays them into an in-memory `fabro_store::Database` +- it reads artifacts from local storage directly +- it assumes the CLI can see the same storage and scratch directories as the server + +That is now the wrong boundary. The server should own store access; the CLI should only export server-provided run data to disk. + +## Key Decisions + +- `store dump` becomes server-only. + - Use `ServerTargetArgs`, not `StorageDirArgs`. + - Resolve selectors through `ServerSummaryLookup`, not `ServerRunLookup`. + - Do not scan local scratch directories or orphan runs. + +- Keep the current export layout. + - It is fine that the command still writes `events.jsonl`, `checkpoints/`, and `artifacts/`. + - It should not write a `blobs/` directory. + - Blob storage is an internal offload mechanism, not part of the user-facing export model. + +- Do not add blob enumeration. + - Blob-backed values should be hydrated server-side before the CLI receives run state or events. + - The CLI should not need to understand blob pointer conventions. + - The exported files should look as if offloading had never happened. + +- Best-effort race handling is acceptable. + - If a blob or artifact is listed/referenced but disappears before download, skip it and continue. + - Fail on transport or server errors other than `404`. + +- Event pagination should use the server's `meta.has_more` contract. + - Do not stop paging based on "returned fewer than page size". + - The client should retain and use pagination metadata from the API response. + +- Artifact downloads should be concurrent with a fixed upper bound. + - Use a bounded concurrency strategy such as `JoinSet` or `FuturesUnordered` with a small limit. + - Recommended default: `8` concurrent artifact downloads. + +## Implementation Changes + +### 1. Convert `store dump` to standard server targeting + +In `fabro-cli`: + +- change `StoreDumpArgs` to flatten `ServerTargetArgs` +- remove `StorageDirArgs` from this command +- update help text, docs, and snapshots to show `--server` / `FABRO_SERVER` + +Run resolution should follow the same contract as other server-backed inspection commands: + +1. explicit `--server` +2. configured `[server].target` +3. default local server instance if no server target is configured + +The command should no longer derive behavior from a local storage dir. + +### 2. Resolve the run from server summaries only + +- replace `ServerRunLookup` usage with `ServerSummaryLookup` +- resolve `` from server-provided summaries only +- remove any dependence on local scratch-path scanning during selector resolution + +This ensures `store dump` works even when the server runs on a different host. + +### 3. Add server/API support for paginated hydrated reads + +The CLI path depends on the server returning enough information to page correctly and to keep blob handling transparent. + +Add or update the server/API contract for: + +- `GET /runs/{id}/state?hydrate_blobs=true` +- `GET /runs/{id}/events?...&hydrate_blobs=true` + +Follow the existing OpenAPI-first workflow for these API changes: + +- update `docs/api-reference/fabro-api.yaml` +- rebuild Rust API types/client via `cargo build -p fabro-api` +- regenerate the TypeScript client in `lib/packages/fabro-api-client` + +Also update the CLI client shape for event listing so pagination metadata is preserved instead of discarded: + +- add a new paginated event-list helper for `store dump` that returns both `data` and `meta.has_more` +- keep the existing `list_run_events(...) -> Vec` convenience method in place for current callers unless there is a strong reason to migrate them in the same change +- `store dump` must consume the metadata-bearing form + +The existing artifact APIs are already sufficient for this plan: + +- `list_run_artifacts(run_id)` +- `download_stage_artifact(run_id, stage_id, filename)` + +No new artifact endpoint work is required here. + +### 4. Add server-side blob hydration for state and events + +Blob storage is intended to be transparent. `store dump` should not detect, enumerate, or hydrate blob pointers in the CLI. + +Recommended API shape: + +- add `hydrate_blobs=true` as an optional query parameter on both endpoints +- when omitted or `false`, preserve current behavior +- when `true`, the server resolves blob-backed references before serializing the response +- reflect those query parameters in the OpenAPI schema and generated clients + +Hydration scope: + +- all blob-backed values inside `RunProjection` +- all blob-backed values inside returned event payloads +- this includes values that ultimately flow into exported checkpoint JSON because checkpoints come from `RunProjection.checkpoints` + +Hydration mechanism: + +- implement a shared server-side JSON hydrator that walks `serde_json::Value`, detects blob-backed pointer values, reads the referenced blobs from the run store, and replaces the pointer string with the parsed JSON payload +- reuse the same helper for both state and event responses so blob resolution rules stay identical +- keep the helper server-owned rather than attaching it only to `RunProjection`, because event payloads need the same treatment + +If a referenced blob cannot be resolved during hydrated fetch: + +- treat `404` as a race-tolerant miss +- preserve the original pointer value in the hydrated response +- fail on non-`404` server/store errors + +This keeps blob knowledge server-owned, which matches the architecture this plan is trying to enforce. + +### 5. Refactor `RunDump` to accept fetched export data instead of store handles + +`RunDump::store_export` currently assumes: + +- a `RunDatabase` +- an `ArtifactStore` +- store enumeration for blobs and artifacts + +Refactor this into a store-agnostic export builder with a concrete constructor: + +```rust +RunDump::from_export( + state: &RunProjection, + events: &[EventEnvelope], + artifacts: &HashMap<(StageId, String), Bytes>, +) -> Result +``` + +Notes: + +- `state` is already hydrated +- `events` are already hydrated +- blobs are not a separate constructor parameter +- `artifacts` are the only binary payloads the CLI still fetches explicitly + +The new builder should preserve the current file layout and validation rules: + +- top-level metadata files from `RunProjection` +- node files under `nodes//visit-/` +- `retro/prompt.md` and `retro/response.md` +- `events.jsonl` +- `checkpoints/.json` sourced directly from `RunProjection.checkpoints` +- `artifacts/nodes//visit-/` + +Keep the existing staged-directory write behavior: + +- reject non-empty output dirs +- write into a temp dir under the output parent +- rename into place when complete + +### 6. Replace local store reconstruction with HTTP-backed export collection + +In `dump_command`, fetch the export inputs directly from the server: + +- hydrated current run state via `get_run_state(run_id, hydrate_blobs = true)` +- full hydrated event history via paginated `list_run_events(run_id, since_seq, limit, hydrate_blobs = true)` + - use an explicit page size of `1000` + - continue paging based on `meta.has_more` +- run artifacts via `list_run_artifacts(run_id)` +- artifact contents via `download_stage_artifact(run_id, stage_id, filename)` using bounded concurrency + +Do not: + +- create a `Database` +- call `rebuild_run_store` +- open a local `ArtifactStore` +- read anything under local `storage/` or scratch paths + +### 7. Leave shared event-rebuild helpers for other commands + +`rebuild_run_store` is still used by other commands such as `fork`, `rewind`, and `pr create`. + +- remove it from `store dump` +- do not delete it in this change unless those other commands are migrated too + +This plan is specific to aligning `store dump` with server-owned export. + +## Test Plan + +- update the `store dump --help` snapshot for the CLI arg change from `--storage-dir` to `--server` +- re-enable the disabled `store dump` integration coverage and make it exercise the server-backed path only +- add a regression test for a run with more than 100 events to prove pagination exports the full `events.jsonl` +- add a regression test for a run with exactly `1000` events to prove the client performs the additional page fetch and stops on `has_more = false`, not on page-size heuristics +- add server/API coverage for hydrated fetches: + - hydrated `get_run_state(..., hydrate_blobs = true)` replaces blob-backed pointers with original JSON values + - hydrated `list_run_events(..., hydrate_blobs = true)` replaces blob-backed pointers in event payloads + - blob `404` during hydration preserves the original pointer value +- add coverage that no `blobs/` directory is emitted +- add `RunDump` coverage for the new store-agnostic constructor to verify the exported file layout remains unchanged for representative state/events/blobs/artifacts +- keep or restore the non-empty output-dir rejection test +- add a race-tolerance test where a referenced artifact returns `404` and the export completes without that artifact file +- add coverage that artifact downloads run through the bounded-concurrency path without changing output order or file paths + +## Assumptions and Defaults + +- `store dump` should use the standard server-target contract, not `--storage-dir` +- the current export file layout is intentionally preserved +- blob handling is transparent and server-owned; there is no `blobs/` export directory and no need for a blob-list API +- best-effort consistency is sufficient because this command exists for debugging and inspection +- the server remains the only component allowed to access the underlying run store in this architecture