feat: add capsula pull command - #1178
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
+ Coverage 60.41% 62.49% +2.07%
==========================================
Files 44 45 +1
Lines 4661 4994 +333
==========================================
+ Hits 2816 3121 +305
- Misses 1845 1873 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for the PR. Below is the review of this PR by Claude Code Opus 5. I didn't verify each claim, but if they are true, I agree with the conclusion:
Other findings are valid but don't need to be addressed in this PR. Warning This content was written by an AI agent and must be verified by a human developer. After human verification, this alert may be removed. Threat model for this reviewPer maintainer direction, this review assumes:
SummaryThe feature is well shaped. Extracting the run-directory naming into Under the stated trust model the dominant risk is destructive Findings at a glancePaths are relative to
Merge blockers: F1, F2 — and F3 / F6, which share F1's fix. FindingsOrdered by severity under the stated trust model. F1 — [HIGH]
|
| Lost | Why |
|---|---|
| Symlinks | entry.file_type().is_file() is false for symlinks; follow_links defaults to false |
| Empty directories | only files are collected |
Original _capsula/*.json bytes |
_capsula is excluded from upload and reconstructed from DB columns |
Sub-millisecond duration |
server stores duration_ms; restored as secs/nanos at ms precision (pull.rs:199-202) |
| Any file added after the push | never uploaded |
The realistic sequence needs no mistake beyond a reasonable reading of the flag: "I pulled this run, it
says the directory already exists, the error tells me to use --force, so I use --force." The error at
pull.rs:54-57 actively suggests it.
_capsula/pulled.json already exists and is exactly the signal needed to distinguish the two cases — but
nothing reads it (F3).
Fix:
- Refuse
--forcewhen the target lacks_capsula/pulled.json(i.e. it is locally produced), requiring a
separate, explicit opt-in for that case. Reuse the existing marker rather than adding new state. - Reword the "already exists" error so it distinguishes the two cases instead of unconditionally
recommending--force. - Document in
docs/cli-reference.mdthat a restored run is a subset of the original, listing what is lost.
F2 — [HIGH] The temp directory sits at exactly the depth the vault scanners walk
Location: crates/capsula-orchestration/src/pull.rs:69-72
let temp = tempfile::Builder::new().prefix(".pull-tmp-").tempdir_in(date_dir)?;TempDir cleans up on drop, which SIGINT/SIGKILL skips. Ctrl-C during a download — completely routine,
especially for large artifacts — therefore leaves .pull-tmp-XXXX at vault/<date>/.pull-tmp-XXXX. That is
precisely the vault/*/* shape every vault scanner walks:
list_runs—crates/capsula-orchestration/src/vault.rs:46-89find_run_dir/find_run_dir_by_name—vault.rs:128,:173(min_depth(2).max_depth(2))push --all—crates/capsula-cli/src/main.rs:451-471
Two outcomes depending on when the interruption lands:
- During download (the common case): invisible garbage that accumulates silently across retries.
Nothing surfaces it and nothing cleans it up. - After
write_capsula_dir(pull.rs:75) but before the rename (pull.rs:82): the leftover has a
valid_capsula/metadata.json, so it becomes a phantom run —capsula listshows it,
capsula showresolves to it, andpush --alluploads it under a bogus path.
The PR's claim that "an interrupted pull never leaves a half-restored run" holds for the final path only,
not for the vault as a whole.
Fix: create the temp directory directly under target_vault_dir — same filesystem, so the rename stays
atomic, but one level above the depth-2 scan. Optionally sweep stale .pull-tmp-* at the start of a pull.
Filtering the prefix in the scanners is a weaker second line.
F3 — [MEDIUM] pulled.json is written but nothing reads it; push --all re-uploads pulled runs
Location: crates/capsula-orchestration/src/pull.rs:207-212
The marker is the right idea, but no code consumes it — so it currently reads as a safeguard while providing
none (it is also the missing ingredient for F1).
Meanwhile push --all (main.rs:451) pushes every depth-2 directory containing _capsula, which now
includes pulled runs. push --all is a habitual, whole-vault command, so this needs no mistake either.
The reconstruction is explicitly lossy, so the round trip degrades server state:
durationtruncated to millisecond precision (pull.rs:199-202)parse_command's shlex fallback (pull.rs:219-224) rewrites a legacy command string into a JSON array,
changing what the server stores on re-push__meta.hook_indexis injected into the reconstructed hook JSON;run_outputsupserts with
ON CONFLICT ... DO UPDATE(server/src/lib.rs:1224), so rows are rewritten from the reconstruction
POST /api/v1/runs returns 409 for an existing run, so the runs row itself is spared — hook rows and
captured files are not.
Fix: implement the guard now (skip pulled runs in push / push --all unless explicitly overridden).
It closes F1 and F3 together.
F4 — [MEDIUM] pull accepts only a ULID while push accepts an ID or a name
Location: crates/capsula-cli/src/main.rs:99-101 vs :88-92, crates/capsula-orchestration/src/pull.rs:49-50
capsula push [run-id-or-name] resolves either form via find_run_dir. capsula pull <RUN_ID> takes only a
ULID. Run names are the human-facing handle everywhere else — capsula list prints them, run-start emits
one, show and run-end take them — so capsula pull happy-river is the natural thing to type.
It fails with "Run 'happy-river' not found on server", which reads as "that run was never pushed" rather
than "wrong identifier kind". The Ulid::from_string check at pull.rs:49 never even runs, because the
server 404s on the name first.
Fix: at minimum, detect a non-ULID argument up front and say so explicitly. Better, accept names by
resolving them server-side (POST /api/v1/runs/search already filters by vault) so pull matches push.
F5 — [MEDIUM] The new API types are not actually shared with the server, so they can drift silently
Location: crates/capsula-api-types/src/lib.rs:127-178, crates/capsula-server/src/lib.rs:828-858, :924-930
(Unaffected by the trust model.)
The PR adds RunRecord / RunDetailFile / RunDetailResponse to capsula-api-types and describes this as
"following the existing client/server type-sharing convention" — but the server still hand-builds the response
with json!, and fetch_run_files_json hand-builds each file entry. capsula-server never references any
of the three types.
The existing convention is the opposite: VaultsResponse, VaultExistsResponse, and UploadResponse are
constructed by the server and serialized via json_response (server/src/lib.rs:473, :513, :1306).
Consequence: renaming a field in the server's json! is not a compile error. The only guard is the new
integration test, which requires live PostgreSQL and so does not run under a plain cargo test --workspace.
Fix: build RunDetailResponse / RunDetailFile in get_run and return them through json_response,
as the surrounding handlers already do.
F6 — [LOW-MEDIUM] --force guard is checked long before it is used, and the comment claims otherwise
Location: crates/capsula-orchestration/src/pull.rs:53-58, :77-81
if target_dir.exists() {
// Only reachable with --force (checked above).
std::fs::remove_dir_all(&target_dir)?;
}The exists() guard is at line 53, before every network round trip. A directory created during the download
is therefore remove_dir_all'd without --force. Benign trigger: two concurrent capsula pull calls for
the same run, or a pull racing a long-running local operation. Narrow, but the comment asserts the opposite of
what holds and the failure mode is data loss.
Fix: if force && target_dir.exists(); if !force and it now exists, fail rather than clobber. One line,
and it also bounds the blast radius of F7.
F7 — [LOW under this trust model] run.name is used unvalidated as a path component
Location: crates/capsula-orchestration/src/pull.rs:51
run.name arrives from GET /api/v1/runs/{id} and is interpolated into a path component
(format!("{time_str}-{name}"), crates/capsula-core/src/run.rs:59) with no validation. run.id is gated by
Ulid::from_string; run.name has no equivalent.
Why this is low here: run names are generated in exactly one place —
Generator::default().next() in create_and_setup_run (orchestration/src/run.rs:23) — and there is no
user-facing way to set one. Under a trusted server with non-malicious clients, a name containing a path
separator cannot arise in practice.
Recorded for completeness, since the behaviour is confirmed rather than theoretical (throwaway probe,
name ../../../../escaped/evil):
result = Ok(".../test-vault/2016-07-30/235410-../../../../escaped/evil")
→ files written to proj/.capsula/escaped/evil/ (outside the vault)
→ with --force, remove_dir_all fires on the escaped path
→ a literal `235410-..` junk directory is left behind even on success
Recommendation: still add the guard, but as cheap robustness rather than a merge blocker — assert
run.name is a single Component::Normal, and assert the computed target_dir resolves under
target_vault_dir. ensure_within_vault (vault.rs:109) already implements the containment pattern.
If the trust model ever changes — a shared or multi-tenant server, since POST /api/v1/runs performs no
validation of name (server/src/models.rs:23) — this returns to high severity immediately.
F8 — [LOW under this trust model] Vault name joined into a path unvalidated
Location: crates/capsula-cli/src/main.rs:528-540
vault_dir.parent()?.join(&vault_name) — note that Path::join with an absolute argument discards the base
entirely, so --vault /tmp/x would target /tmp/x rather than a sibling.
Why this is low here: the pull only proceeds if vault_name equals the run's recorded vault
(pull.rs:40-47), and recorded vault names come from [vault] name in capsula.toml. A typo that is not a
real vault name fails the mismatch check first. The reachable case is therefore misconfiguration: a
[vault] name containing / (e.g. "team/project", which silently nests) or .. (which escapes).
Fix: validate vault_name as a single normal path component before joining — the same helper as F7.
Consider validating [vault] name at config-load time instead, which catches it earlier and for every command.
F9 — [LOW] hex_encode duplicated three times
Location: crates/capsula-orchestration/src/pull.rs:231-237, crates/capsula-orchestration/tests/pull.rs:23-31
capsula_core::util::hex_encode (crates/capsula-core/src/util.rs:7) already exists, is pub, and is
byte-for-byte the same implementation. capsula-orchestration already depends on capsula-core, and
pull.rs:3 already imports from it. The test helper sha256_hex is a third copy.
Fix: use capsula_core::util::hex_encode; in both; delete the local copies.
F10 — [LOW] Two sanitize_relative_path functions with the same name and different contracts
Location: crates/capsula-orchestration/src/pull.rs:138 vs crates/capsula-server/src/lib.rs:1415
| server | pull | |
|---|---|---|
| Returns | Result<String, &'static str> |
Result<PathBuf> (anyhow) |
./a, a//b |
normalizes and accepts | rejects |
| NUL byte | explicit check | none |
C:\ prefix |
explicit check | none (on Unix) |
They compose safely today only because the server normalizes at upload time, so stored paths never contain the
forms the pull side rejects. The shared name over divergent semantics is a maintenance trap.
Fix: rename the pull-side one (e.g. checked_run_relative_path) or hoist a shared helper into
capsula_core::util next to resolve_relative.
F11 — [LOW] Stringly-typed status and untyped hook payloads
Location: crates/capsula-api-types/src/lib.rs:158-178, crates/capsula-orchestration/src/pull.rs:28-38
status: String matched via match detail.status.as_str() pushes a wire-format concern into a runtime string
compare. An internally-tagged enum expresses the real contract, makes the run: Option<_> / error: Option<_>
correlation type-level, and removes the "Server response is missing the run record" fallback at pull.rs:32:
#[serde(tag = "status", rename_all = "snake_case")]
pub enum RunDetailResponse {
Ok { run: RunRecord, /* ... */ },
NotFound { error: String },
Error { error: String },
}pre_run_hooks: Vec<serde_json::Value> is similarly untyped; models::HookOutputResponse already describes
the shape and could move into capsula-api-types alongside F5.
F12 — [LOW] Whole-file buffering caps artifact size
Location: crates/capsula-client/src/lib.rs (download_file), crates/capsula-orchestration/src/pull.rs:104-129
response.bytes()?.to_vec() buffers the entire body then copies it, roughly doubling peak memory; the caller
holds the same buffer to hash and write. Capsula captures build artifacts, so multi-hundred-MB files are
plausible — and a large download is also the case most likely to be Ctrl-C'd (F2).
Fix: stream into the destination through a hashing writer (response.copy_to(&mut writer) wrapping
Sha256), verifying the digest after the copy and discarding on mismatch.
F13 — [LOW] encode_file_path over-encodes unreserved characters
Location: crates/capsula-client/src/lib.rs (encode_file_path)
The allowlist-polarity reasoning is sound and worth keeping. But NON_ALPHANUMERIC also encodes -, .,
_, ~, which RFC 3986 §2.3 explicitly discourages: URIs differing only in the percent-encoding of unreserved
characters are equivalent and "should not be created". Practical costs: URLs and server logs read as
result%2Etxt, and some proxies normalize or reject %2E.
Fix: keep the polarity, subtract the unreserved set —
NON_ALPHANUMERIC.remove(b'-').remove(b'.').remove(b'_').remove(b'~'). This changes two client unit tests and
the integration mock paths (output/result%2Etxt → output/result.txt).
F14 — [LOW] fetch_run_files_json duplicates an existing query and selects columns it discards
Location: crates/capsula-server/src/lib.rs:828-858
Identical to the query at server/src/lib.rs:424 (HTML run-detail page). The new copy selects storage_path
and content_type purely to satisfy query_as!(models::CapturedFile, ...), then drops them.
Fix: one shared fetch_run_files(pool, id) used by both handlers; narrow the projection (or use a
dedicated row struct) if the extra columns are unwanted.
F15 — [LOW] Silent numeric coercion and unused/divergent numeric types
Location: crates/capsula-orchestration/src/pull.rs:194, crates/capsula-api-types/src/lib.rs
let duration_ms = u64::try_from(run.duration_ms.unwrap_or(0)).unwrap_or(0);A negative duration_ms silently becomes 0. Per the repo's own guidance ("do not use None as an error
signal"), this deserves at least a warn!.
Also: RunDetailFile.size is deserialized but never used — the downloaded length is never compared against the
recorded size (harmless, since the hash covers integrity, but the field is then decorative). And
RunRecord.duration_ms: Option<i64> diverges from the server's Option<i32> (server/src/models.rs:14);
the widening direction is safe, but the mismatch is gratuitous.
F16 — [LOW] Undocumented round-trip fidelity gaps
Location: crates/capsula-orchestration/src/pull.rs:177-188, :219-224
- A locally created run always writes
pre-run.jsonandpost-run.json(orchestration/src/run.rs:71,
:98), even for an empty hook array. A pulled run omits them when the array is empty, so_capsulalayouts
differ by origin. parse_command's shlex fallback rewrites legacy command strings into arrays — deliberate but irreversible
(see F3).
The PR is upfront that reconstruction is best-effort; these two specifics just are not in the list, and they
belong in the F1 documentation.
F17 — [LOW] Test coverage gaps
The existing 7 integration + 6 unit tests are good. Missing, in trust-model priority order:
--forceover a locally produced run — the F1 case; the highest-value missing test.- An abandoned
.pull-tmp-*with a valid_capsula/metadata.json— assertslist_runsdoes not surface
it (F2). - A run with no
exit_code(created byrun-start, never ended) — assertscommand.jsonis absent. - A file entry with
hash: null— exercises thewarn!path atpull.rs:117. --forcewhen the target exists as a file, not a directory —remove_dir_allfails confusingly.run.name/ vault name containing separators — regression tests for F7 / F8.- The server-side test (
api_tests.rs) needs live PostgreSQL, so thefileswire contract is unverified in a
defaultcargo test --workspace. Addressing F5 makes it compile-checked instead.
Implementation plan
Re-ordered for the "trusted server, fallible user" model. Steps 1–2 are the merge blockers.
Step 1 — Make --force non-destructive (F1, F3, F6) — blocking
These three share one mechanism, so do them together:
- Read
_capsula/pulled.jsoninpull_single_run; refuse--forcewhen the existing target lacks it,
requiring a separate explicit opt-in to overwrite a locally produced run. - Move the removal behind
if force && target_dir.exists(); fail rather than clobber if it appeared during
the download. - Reword the "already exists" error so it distinguishes pulled-copy from local-original instead of
unconditionally recommending--force. - Have
push/push --allskip runs carryingpulled.jsonunless explicitly overridden. - Tests:
--forceover a local-origin run is refused; over a previously pulled run it succeeds;
push --allskips pulled runs.
No dependencies. Highest value per line changed.
Step 2 — Move the temp directory out of the scanned depth (F2) — blocking
Create it under target_vault_dir rather than date_dir, keeping the rename on the same filesystem.
Optionally sweep stale .pull-tmp-* at pull start. Test: an abandoned temp dir with a valid
_capsula/metadata.json must not appear in list_runs.
Independent of Step 1.
Step 3 — Accept run names in pull (F4)
At minimum, detect a non-ULID argument before the request and produce a specific error. Preferably resolve
names server-side so pull matches push's [run-id-or-name].
Step 4 — Cheap path-component hardening (F7, F8)
One shared helper asserting a single Component::Normal, applied to run.name in pull_single_run and to
vault_name in main.rs, plus a containment assertion on target_dir reusing the ensure_within_vault
pattern. Consider validating [vault] name at config-load time so every command benefits.
Low severity under the current trust model, but a few lines. Revisit priority if the deployment model ever
becomes multi-tenant.
Step 5 — Make the API types genuinely shared (F5, F11, F14)
- Move
HookOutputResponse/HookMetaResponseintocapsula-api-types(or mirror them). - Rewrite
get_runto constructRunDetailResponseand return it viajson_response. - Fold
fetch_run_files_jsoninto a sharedfetch_run_filesused by both the HTML and JSON handlers. - Convert
statusto an internally-tagged enum and simplify thematchinpull.rs.
Do 5.1–5.2 together; 5.4 touches the client and should follow 5.2 so both sides move at once.
Step 6 — Quality cleanups (F9, F10, F12, F13, F15, F16)
Independent and individually small; batch into one follow-up commit:
- Delete the duplicated
hex_encodes in favour ofcapsula_core::util::hex_encode. - Rename or unify the two
sanitize_relative_pathfunctions. - Stream downloads through a hashing writer instead of buffering whole files.
- Subtract the RFC 3986 unreserved set from the percent-encode set (updates 3 tests).
- Warn rather than silently zero on a negative
duration_ms; drop or useRunDetailFile.size; align
duration_mswidth with the server model. - Document the
pre-run.json/post-run.jsonomission and theparse_commandnormalization as part of the
F1 docs.
Step 7 — Fill the remaining test gaps (F17)
Whatever Steps 1–4 do not already cover.
What is good and should not change
- Extracting
run_dir_relative_pathintocapsula-coreso push/pull naming cannot drift — exactly right, and
the doc comment carries over the original rationale. - Download-to-temp-then-rename for atomicity of the final path (the flaw is where the temp lives, F2,
not the technique). - SHA-256 verification against the server-recorded hash, with the no-hash case surfaced as a warning rather
than silently trusted. - Allowlist-polarity percent-encoding with the reasoning written down, plus a regression test pinning the wire
format (pull_restores_file_with_url_significant_characters_in_name). F13 only narrows the set. - Writing
pulled.jsonat all — it is the right primitive, and Step 1 turns it into a real safeguard. - The server test asserting
storage_pathdoes not leak into the API response. - The PR description's honesty about what is best-effort and what is deferred.
Merge blockers: - F1/F3/F6: --force now only replaces runs carrying the _capsula/pulled.json marker; locally produced runs are never replaced and the already-exists errors distinguish the two cases. push (and push --all) refuses/skips pulled runs so lossy reconstructions cannot degrade the server's copy. The existence check is re-done at replacement time, so a directory appearing during the download is no longer clobbered. - F2: the download temp directory now lives directly under the vault root (same filesystem, atomic rename) instead of vault/<date>/, so an interrupted pull cannot leave a phantom run at the depth the vault scanners walk. Non-blockers taken along: - F4 (minimum): non-ULID pull arguments fail fast with a specific error - F7/F8: run name and vault name are validated as single path components before being joined into paths - F9: reuse capsula_core::util::hex_encode instead of local copies - F10: rename the pull-side sanitizer to checked_run_relative_path to stop sharing a name with the server's normalizing variant - F13: percent-encoding no longer escapes RFC 3986 unreserved chars - F14: shared fetch_run_files used by both run-detail handlers - F15: verify downloaded sizes; warn on negative duration_ms; align duration_ms width with the server model - F16: document the round-trip fidelity gaps in the CLI reference - F17: regression tests for all of the above (16 integration tests)
432d90b to
978ec85
Compare
|
Thank you for the thorough review — the findings all checked out against the implementation. Addressed in 978ec85 (rebased onto latest Merge blockers
Non-blockers taken along: F4 (minimum: non-ULID arguments fail fast with a specific message), F7/F8 (run name and vault name validated as single path components), F9 ( Two deliberate deviations from the suggested fixes
Deferred
|
Pull runs of a non-configured vault into the default vault location. The configured `[vault] path` describes the configured vault only, so restoring another vault's run next to it placed the run in a directory that was never chosen for it (an external directory, for instance). The default `.capsula/<name>` layout only existed as an inline format string inside `VaultConfig`'s Deserialize impl, which is why `pull` could not ask for it. Extract it as `DEFAULT_VAULT_ROOT` plus `default_vault_path()` in capsula-config and use it from both the config default and `pull`. A run of another vault now defaults to `.capsula/<vault>/` under the project root. An explicit `--vault-path` / `CAPSULA_VAULT_PATH` override still wins, so `resolve_vault_path()` reports its source through `VaultPathSource` and `LoadedConfig` carries it. The decision lives in `resolve_pull_vault_dir()` so it is unit-testable. Also centralize the pulled-run check: `is_pulled_run()` is now public and replaces the open-coded `pulled.json` checks in `push_single_run()` and in the `push --all` skip.
Merge blockers: - F1/F3/F6: --force now only replaces runs carrying the _capsula/pulled.json marker; locally produced runs are never replaced and the already-exists errors distinguish the two cases. push (and push --all) refuses/skips pulled runs so lossy reconstructions cannot degrade the server's copy. The existence check is re-done at replacement time, so a directory appearing during the download is no longer clobbered. - F2: the download temp directory now lives directly under the vault root (same filesystem, atomic rename) instead of vault/<date>/, so an interrupted pull cannot leave a phantom run at the depth the vault scanners walk. Non-blockers taken along: - F4 (minimum): non-ULID pull arguments fail fast with a specific error - F7/F8: run name and vault name are validated as single path components before being joined into paths - F9: reuse capsula_core::util::hex_encode instead of local copies - F10: rename the pull-side sanitizer to checked_run_relative_path to stop sharing a name with the server's normalizing variant - F13: percent-encoding no longer escapes RFC 3986 unreserved chars - F14: shared fetch_run_files used by both run-detail handlers - F15: verify downloaded sizes; warn on negative duration_ms; align duration_ms width with the server model - F16: document the round-trip fidelity gaps in the CLI reference - F17: regression tests for all of the above (16 integration tests)
Pull runs of a non-configured vault into the default vault location. The configured `[vault] path` describes the configured vault only, so restoring another vault's run next to it placed the run in a directory that was never chosen for it (an external directory, for instance). The default `.capsula/<name>` layout only existed as an inline format string inside `VaultConfig`'s Deserialize impl, which is why `pull` could not ask for it. Extract it as `DEFAULT_VAULT_ROOT` plus `default_vault_path()` in capsula-config and use it from both the config default and `pull`. A run of another vault now defaults to `.capsula/<vault>/` under the project root. An explicit `--vault-path` / `CAPSULA_VAULT_PATH` override still wins, so `resolve_vault_path()` reports its source through `VaultPathSource` and `LoadedConfig` carries it. The decision lives in `resolve_pull_vault_dir()` so it is unit-testable. Also centralize the pulled-run check: `is_pulled_run()` is now public and replaces the open-coded `pulled.json` checks in `push_single_run()` and in the `push --all` skip.
51aee41 to
b6dc6f4
Compare
Merge blockers: - F1/F3/F6: --force now only replaces runs carrying the _capsula/pulled.json marker; locally produced runs are never replaced and the already-exists errors distinguish the two cases. push (and push --all) refuses/skips pulled runs so lossy reconstructions cannot degrade the server's copy. The existence check is re-done at replacement time, so a directory appearing during the download is no longer clobbered. - F2: the download temp directory now lives directly under the vault root (same filesystem, atomic rename) instead of vault/<date>/, so an interrupted pull cannot leave a phantom run at the depth the vault scanners walk. Non-blockers taken along: - F4 (minimum): non-ULID pull arguments fail fast with a specific error - F7/F8: run name and vault name are validated as single path components before being joined into paths - F9: reuse capsula_core::util::hex_encode instead of local copies - F10: rename the pull-side sanitizer to checked_run_relative_path to stop sharing a name with the server's normalizing variant - F13: percent-encoding no longer escapes RFC 3986 unreserved chars - F14: shared fetch_run_files used by both run-detail handlers - F15: verify downloaded sizes; warn on negative duration_ms; align duration_ms width with the server model - F16: document the round-trip fidelity gaps in the CLI reference - F17: regression tests for all of the above (16 integration tests)
Pull runs of a non-configured vault into the default vault location. The configured `[vault] path` describes the configured vault only, so restoring another vault's run next to it placed the run in a directory that was never chosen for it (an external directory, for instance). The default `.capsula/<name>` layout only existed as an inline format string inside `VaultConfig`'s Deserialize impl, which is why `pull` could not ask for it. Extract it as `DEFAULT_VAULT_ROOT` plus `default_vault_path()` in capsula-config and use it from both the config default and `pull`. A run of another vault now defaults to `.capsula/<vault>/` under the project root. An explicit `--vault-path` / `CAPSULA_VAULT_PATH` override still wins, so `resolve_vault_path()` reports its source through `VaultPathSource` and `LoadedConfig` carries it. The decision lives in `resolve_pull_vault_dir()` so it is unit-testable. Also centralize the pulled-run check: `is_pulled_run()` is now public and replaces the open-coded `pulled.json` checks in `push_single_run()` and in the `push --all` skip.
b6dc6f4 to
59d46b5
Compare
Add a pull command that downloads a run from a Capsula server and restores it under the local vault, as the counterpart of capsula push (#1177): - Extend GET /api/v1/runs/{id} with a files array (path, size, hash); the server-internal storage_path is not exposed - Reconstruct the run directory deterministically from the run's ULID and name via the new capsula_core::run::run_dir_relative_path (shared with local run creation) - Verify downloaded files against server-recorded SHA-256 hashes; restore _capsula metadata best-effort from the server's structured data and write a _capsula/pulled.json origin marker - Download into a temporary directory and rename atomically; existing run directories require --force to replace - Restore only relative paths contained in the run directory (syntactic check); full path-safety hardening follows the ResolvedProjectPath work (#1098, #1146) - Encode URL file paths segment-wise with NON_ALPHANUMERIC (allowlist) so arbitrary filenames round-trip without a curated character list
Merge blockers: - F1/F3/F6: --force now only replaces runs carrying the _capsula/pulled.json marker; locally produced runs are never replaced and the already-exists errors distinguish the two cases. push (and push --all) refuses/skips pulled runs so lossy reconstructions cannot degrade the server's copy. The existence check is re-done at replacement time, so a directory appearing during the download is no longer clobbered. - F2: the download temp directory now lives directly under the vault root (same filesystem, atomic rename) instead of vault/<date>/, so an interrupted pull cannot leave a phantom run at the depth the vault scanners walk. Non-blockers taken along: - F4 (minimum): non-ULID pull arguments fail fast with a specific error - F7/F8: run name and vault name are validated as single path components before being joined into paths - F9: reuse capsula_core::util::hex_encode instead of local copies - F10: rename the pull-side sanitizer to checked_run_relative_path to stop sharing a name with the server's normalizing variant - F13: percent-encoding no longer escapes RFC 3986 unreserved chars - F14: shared fetch_run_files used by both run-detail handlers - F15: verify downloaded sizes; warn on negative duration_ms; align duration_ms width with the server model - F16: document the round-trip fidelity gaps in the CLI reference - F17: regression tests for all of the above (16 integration tests)
Pull runs of a non-configured vault into the default vault location. The configured `[vault] path` describes the configured vault only, so restoring another vault's run next to it placed the run in a directory that was never chosen for it (an external directory, for instance). The default `.capsula/<name>` layout only existed as an inline format string inside `VaultConfig`'s Deserialize impl, which is why `pull` could not ask for it. Extract it as `DEFAULT_VAULT_ROOT` plus `default_vault_path()` in capsula-config and use it from both the config default and `pull`. A run of another vault now defaults to `.capsula/<vault>/` under the project root. An explicit `--vault-path` / `CAPSULA_VAULT_PATH` override still wins, so `resolve_vault_path()` reports its source through `VaultPathSource` and `LoadedConfig` carries it. The decision lives in `resolve_pull_vault_dir()` so it is unit-testable. Also centralize the pulled-run check: `is_pulled_run()` is now public and replaces the open-coded `pulled.json` checks in `push_single_run()` and in the `push --all` skip.
59d46b5 to
6542c96
Compare
3 participants
Summary
Adds a
capsula pullcommand that downloads a run from a Capsula server and restores it under the local.capsuladirectory, as the counterpart ofcapsula push. Implements the design decided in #1177.Closes #1177.
How it works
GET /api/v1/runs/{id}— extended in this PR to include afilesarray (path,size,hash; the server-internalstorage_pathis not exposed). The run's recorded vault is verified against--vault(default: the vault incapsula.toml).{YYYY-MM-DD}/{HHMMSS}-{name}from the run's ULID (UTC timestamp) and name — the naming logic is extracted fromRun::gen_run_dirinto the publiccapsula_core::run::run_dir_relative_path, so it cannot drift.GET /api/v1/runs/{id}/files/{*path}endpoint and verified against the server-recorded SHA-256 hash; a mismatch fails the pull. Files without a stored hash are restored with a warning._capsula/{metadata,pre-run,post-run,command}.jsonare reconstructed from the server's structured data (best-effort as decided in feat: addcapsula pullcommand #1177 — not byte-identical: JSON formatting differs anddurationis restored at millisecond precision). A_capsula/pulled.jsonmarker records the origin server URL and pull time, so pulled runs can be told apart from locally produced ones (e.g. by a future push-side guard).--forceis given, in which case it is replaced.Path containment scope
Restoring writes server-provided file paths to the local disk, so this PR includes a minimal, syntactic containment check (
sanitize_relative_path): only relative paths that stay inside the run directory are restored — absolute paths and paths containing..(or any non-normal component) are rejected as errors. Within a pull, only regular files and directories are ever created, so symlink-based escapes cannot be introduced by the restore itself.This intentionally stops short of full path-safety hardening. #1177 deferred sanitization entirely; this PR ships the cheap syntactic subset because writing unchecked server paths to disk is an obvious footgun. The full treatment should follow the approach of the in-flight path-safety PR stack (#1097 → #1098 → #1146 → #1147): once
ResolvedProjectPath(#1098) and the containment semantics established forcapture-file(#1146) land, the pull-side check should be migrated to that shared mechanism instead of growing its own.Other notes
CapsulaClient::{get_run, download_file, base_url}. File paths are URL-encoded segment by segment withNON_ALPHANUMERIC(allowlist polarity — everything except[0-9A-Za-z]is percent-encoded, keeping/for the wildcard route), so no curated character list needs to be exhaustive. Spaces, URL-significant characters, and non-ASCII filenames all round-trip; server-side path normalization at upload time (e.g.\→/) is untouched, and pull restores exactly what the server recorded.RunRecord/RunDetailFile/RunDetailResponseadded tocapsula-api-types, following the existing client/server type-sharing convention.filesaddition toGET /api/v1/runs/{id}is additive and reuses the exact query already used by the run detail page (no new SQLx offline metadata needed).capsula pullsection added to the CLI reference, including the restore semantics and the path/hash verification behavior.Testing
pull_single_run: successful restore (files, metadata, hooks, marker), hash mismatch leaves no partial run, vault mismatch, unsafe path rejection, existing directory /--forcereplacement, not-found handling, and a regression test for URL-significant characters in file paths (spaces) that pins the percent-encoded wire format; plus unit tests for command parsing, path sanitization, and URL path encoding.GET /api/v1/runs/{id}returnsfileswithpath/size/hashand does not leakstorage_path.cargo test --workspace,cargo clippy --workspace --all-targets --all-features/--no-default-features,cargo fmt --check --all,cargo doc --workspace --no-depsall clean.capsula run→push→ delete local run →pullrestored the captured files byte-identically at the exact original directory path, and the restored run works withcapsula list/show;--forcesemantics verified live.AI assistance
capsula pullcommand #1177Breaking changes
breaking changelabel is applied