scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-07 03:00:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

refactor: simplify pass on Run Files feature

Browse source 
Applied fixes from three parallel reviews (reuse, quality, efficiency):

Server
- Delete dead `sandbox_git_env()` in run_files_security.rs — duplicated
  `sandbox_git.rs::sandbox_git_hardening_env` but had no callers.
- Combine `resolve_head_sha` + `resolve_commit_time` into one
  `resolve_head_sha_and_time` using `git show -s --format=%H\ %cI HEAD`
  — saves ~100ms per request (one fewer sandbox round-trip).
- Parallelize `list_changed_files_raw` + `list_binary_paths` with
  `tokio::join!` — both are mutually independent once `to_sha` is
  known, saves another ~100ms per request.
- Skip phase-1 `cat-file --batch-check` for SHA lists below 10 entries.
  Phase-2 size-caps per-blob anyway; the pre-filter earned its cost
  only for large batches where a single malformed blob could poison
  the parse. Saves another ~100ms on small diffs.
- Extract `transient_503(op, message)` helper — dedupes three identical
  `DiffError::Transient => ApiError::new(503, ...)` arms.
- Strip plan-referencing comments ("§ Unit 5", "P1-X", "P2-Y regression")
  from production code and tests. The R4/R5 taxonomy labels are kept
  where they anchor semantic intent.

Web
- Dedupe `extractRequestId`: one canonical parser in `run-files.tsx`
  (consumed by the loader), one ErrorBoundary-only variant in
  `states.tsx::extractRequestIdFromUnknown`. Both share the same logic;
  separated only so each source can pick its own type discipline.
- Extract `renderStatusError({status, requestId, onRetry})` shared
  between the loader's inline-error path and `RunFilesErrorBoundary`.
  One canonical source of R5 copy.
- Gate the `useFreshness` 10s interval on `hasLabel` — previously it
  ticked every 10s even when `meta == null` and there was no label to
  refresh, re-rendering the whole route for nothing. Now the interval
  only runs while there's actually a timestamp label mounted.
- Fix render-time ref mutation (`lastGoodDataRef.current = result.data`
  in the render body) — violates React render purity. Moved into the
  `useEffect` that watches `result?.data`. Also collapsed
  `previousDataLengthRef` and `lastToShaRef` into single reads off
  `lastGoodDataRef.current` — both were derivable from the cached
  last-good payload.
- Type `DegradedBanner.reason` and `bannerCopyForReason` as
  `RunFilesMetaDegradedReasonEnum` instead of raw `string`.

Tests: 4172 Rust + 94 web, clippy clean, fmt clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-19 19:05:09 -04:00
parent c44c93ff32
commit 048fa37716
No known key found for this signature in database
7 changed files with 341 additions and 378 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

113
apps/fabro-web/app/routes/run-files.tsx
View file

@ -26,6 +26,7 @@ import {
EmptyState,
InlineErrorBanner,
LoadingSkeleton,
renderStatusError,
RunFilesErrorBoundary,
Toast,
} from "./run-files/states";
@ -38,13 +39,12 @@ export const handle = { wide: true };
* Loader return type. Both initial loads and revalidations flow through the
* same discriminated union so a revalidation failure does NOT unmount to
* the route ErrorBoundary — it stays in-band as `{ data: null, error }`
* and the component keeps showing the last-good data with an inline banner.
* This is the plan's "prior content stays mounted; inline banner with
* Retry" behavior for mid-session refresh failures (§ Unit 11).
* and the component keeps showing the last-good data with an inline
* banner.
*
* `error.requestId` is extracted from the 500-response body per R5 so the
* UI can surface it verbatim in the copy ("Request ID: xyz. Contact
* support.") — not just the bare status code.
* `error.requestId` is extracted from the 500-response body so the UI can
* surface it verbatim ("Request ID: xyz. Contact support.") rather than
* just the bare status code.
*/
export type RunFilesLoaderResult = {
data: PaginatedRunFileList | null;
@ -185,11 +185,17 @@ function useFreshness(
meta: PaginatedRunFileList["meta"] | null,
lastFetchedAt: number | null,
): string | null {
// Only tick when there is actually a freshness label to keep fresh — no
// point re-rendering every 10s when `meta == null` and the toolbar
// would show nothing.
const hasLabel =
!!meta && (!!meta.to_sha_committed_at || lastFetchedAt !== null);
const [, setTick] = useState(0);
useEffect(() => {
if (!hasLabel) return undefined;
const id = setInterval(() => setTick((t) => t + 1), 10_000);
return () => clearInterval(id);
}, []);
}, [hasLabel]);
if (!meta) return null;
const now = Date.now();
@ -289,39 +295,30 @@ export default function RunFiles({ loaderData }: any) {
const runStatus = resolveRunStatus(matches);
// Preserve the last successful payload so a failed revalidation can keep
// rendering the previous files while surfacing an inline banner. On the
// very first failure (no prior good data), we render the error state
// equivalent of the ErrorBoundary inline.
// rendering the previous files while surfacing an inline banner.
const lastGoodDataRef = useRef<PaginatedRunFileList | null>(null);
if (result?.data) {
lastGoodDataRef.current = result.data;
}
const data: PaginatedRunFileList | null =
result?.data ?? lastGoodDataRef.current;
const lastFetchedAtRef = useRef<number | null>(null);
const lastToShaRef = useRef<string | null>(null);
const previousDataLengthRef = useRef<number | null>(null);
const [emptyToast, setEmptyToast] = useState<string | null>(null);
const [deepLinkToast, setDeepLinkToast] = useState<string | null>(null);
useEffect(() => {
if (!result?.data) return;
lastFetchedAtRef.current = Date.now();
const currentToSha = (result.data.meta?.to_sha ?? null) as string | null;
const prevLen = previousDataLengthRef.current;
if (prevLen !== null && prevLen > 0 && result.data.data.length === 0) {
// Revalidation-now-empty toast: the user was looking at files, the
// latest fetch shows none.
const prev = lastGoodDataRef.current;
if (prev && prev.data.length > 0 && result.data.data.length === 0) {
setEmptyToast("No changes in this run.");
const id = setTimeout(() => setEmptyToast(null), 3500);
lastGoodDataRef.current = result.data;
lastFetchedAtRef.current = Date.now();
return () => clearTimeout(id);
}
previousDataLengthRef.current = result.data.data.length;
lastToShaRef.current = currentToSha;
lastGoodDataRef.current = result.data;
lastFetchedAtRef.current = Date.now();
return undefined;
}, [result?.data]);
const data: PaginatedRunFileList | null =
result?.data ?? lastGoodDataRef.current;
useSseRevalidation(params.id);
const isInitialLoading = navigation.state === "loading" && !loaderData;
@ -469,54 +466,15 @@ export default function RunFiles({ loaderData }: any) {
return <LoadingSkeleton />;
}
// Initial load failed and we have no prior data to fall back on — render
// the status-specific error state inline per plan § R5. The route does
// not unmount; the RunFilesErrorBoundary is reserved for render-time
// errors that aren't surfaced by the loader at all.
// Initial load failed with no prior data to fall back on. The route
// stays mounted; `RunFilesErrorBoundary` is reserved for render-time
// React errors (the loader doesn't throw).
if (initialError) {
// R5(c): access denied.
if (initialError.status === 401 || initialError.status === 403) {
return (
<div
role="status"
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
>
You don't have access to this run's files.
</div>
);
}
// R5(a): 4xx transient (429) / 503 — retry affordance.
if (initialError.status === 429 || initialError.status === 503) {
return (
<InlineErrorBanner
message="The diff service is temporarily unavailable."
onRetry={() => revalidator.revalidate()}
/>
);
}
// R5(d): 500 — surface request ID if we have it.
if (initialError.status >= 500) {
const suffix = initialError.requestId
? ` Request ID: ${initialError.requestId}.`
: "";
return (
<div
role="status"
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
>
Something went wrong.{suffix} Please contact support if this
persists.
</div>
);
}
// Any other 4xx that isn't 401/403/404/429 — treat like a retryable
// transient failure. The banner keeps the user in context.
return (
<InlineErrorBanner
message={`Couldn't load files (${initialError.status}).`}
onRetry={() => revalidator.revalidate()}
/>
);
return renderStatusError({
status: initialError.status,
requestId: initialError.requestId,
onRetry: () => revalidator.revalidate(),
});
}
if (!data) {
@ -534,11 +492,12 @@ export default function RunFiles({ loaderData }: any) {
const { data: files, meta } = data;
// Refresh is disabled when the server reports the same `to_sha` it
// reported on the previous fetch — no new checkpoint yet.
// reported on the previous successful fetch — no new checkpoint yet.
// `lastGoodDataRef.current` is updated in a useEffect, so during render
// it still holds the previous render's data (or null on first load).
const prevToSha = lastGoodDataRef.current?.meta?.to_sha ?? null;
const refreshDisabled =
!!meta.to_sha &&
lastToShaRef.current !== null &&
lastToShaRef.current === meta.to_sha;
!!meta.to_sha && prevToSha !== null && prevToSha === meta.to_sha;
const toolbar = (
<Toolbar

15
apps/fabro-web/app/routes/run-files/placeholders.tsx
View file

@ -1,5 +1,8 @@
import type { ReactElement } from "react";
import type { FileDiff as ApiFileDiff } from "@qltysh/fabro-api-client";
import type {
FileDiff as ApiFileDiff,
RunFilesMetaDegradedReasonEnum,
} from "@qltysh/fabro-api-client";
const PLACEHOLDER_CLASSES =
"flex items-center justify-between rounded-md border border-line bg-panel/60 px-4 py-3 text-sm text-fg-muted";
@ -95,7 +98,11 @@ export function pickPlaceholder(file: ApiFileDiff): ReactElement | null {
return null;
}
export function DegradedBanner({ reason }: { reason?: string }) {
export function DegradedBanner({
reason,
}: {
reason?: RunFilesMetaDegradedReasonEnum;
}) {
return (
<div className="rounded-md border border-amber-500/30 bg-amber-950/20 px-4 py-3 text-sm text-amber-100">
{bannerCopyForReason(reason)}
@ -103,7 +110,9 @@ export function DegradedBanner({ reason }: { reason?: string }) {
);
}
export function bannerCopyForReason(reason: string | undefined): string {
export function bannerCopyForReason(
reason: RunFilesMetaDegradedReasonEnum | undefined | string,
): string {
switch (reason) {
case "sandbox_gone":
return "Showing final patch only. This run's sandbox has been cleaned up, so individual file contents are no longer available.";

112
apps/fabro-web/app/routes/run-files/states.tsx
View file

@ -141,52 +141,85 @@ export function Toast({ children }: { children: React.ReactNode }) {
}
/**
* Route-level ErrorBoundary that handles the documented status codes from
* the plan § Unit 11 taxonomy. 500 responses with a `request_id` in the
* body surface it in the copy so users can cite it when contacting support.
* Shared helper for rendering the documented status-code taxonomy. Consumed
* by both the inline `initialError` branch in run-files.tsx and the
* `RunFilesErrorBoundary` below — keeps the copy in one place so updates
* don't drift between the two surfaces.
*/
export function RunFilesErrorBoundary() {
const error = useRouteError();
if (isRouteErrorResponse(error)) {
if (error.status === 401 || error.status === 403) {
return (
<div className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted">
You don't have access to this run's files.
</div>
);
}
if (error.status === 503 || error.status === 429) {
return (
<InlineErrorBanner
message="The diff service is temporarily unavailable."
onRetry={() => window.location.reload()}
/>
);
}
if (error.status === 500) {
const requestId = extractRequestId(error.data);
return (
<div className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted">
Something went wrong.
{requestId ? ` Request ID: ${requestId}.` : null} Please contact
support if this persists.
</div>
);
}
export function renderStatusError(args: {
status: number;
requestId: string | null;
onRetry: () => void;
}): React.ReactElement {
const { status, requestId, onRetry } = args;
if (status === 401 || status === 403) {
return (
<div className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted">
Something went wrong ({error.status}).
<div
role="status"
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
>
You don't have access to this run's files.
</div>
);
}
if (status === 429 || status === 503) {
return (
<InlineErrorBanner
message="The diff service is temporarily unavailable."
onRetry={onRetry}
/>
);
}
if (status >= 500) {
const suffix = requestId ? ` Request ID: ${requestId}.` : "";
return (
<div
role="status"
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
>
Something went wrong.{suffix} Please contact support if this persists.
</div>
);
}
return (
<div className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted">
<InlineErrorBanner
message={`Couldn't load files (${status}).`}
onRetry={onRetry}
/>
);
}
/**
* Route-level ErrorBoundary for render-time React errors. The Files loader
* no longer throws (it returns errors in-band via RunFilesLoaderResult), so
* this only fires for React render crashes.
*/
export function RunFilesErrorBoundary() {
const error = useRouteError();
if (isRouteErrorResponse(error)) {
return renderStatusError({
status: error.status,
requestId: extractRequestIdFromUnknown(error.data),
onRetry: () => window.location.reload(),
});
}
return (
<div
role="status"
className="rounded-md border border-dashed border-line bg-panel/40 px-6 py-10 text-center text-sm text-fg-muted"
>
Something went wrong loading this run's files.
</div>
);
}
function extractRequestId(body: unknown): string | null {
/**
* Request-ID parser used only by the ErrorBoundary path. The loader path
* already extracts request_id into `RunFilesLoaderResult.error.requestId`
* via `run-files.tsx::extractRequestId` — this is the body shape
* react-router hands us in `useRouteError().data` for non-Response errors.
*/
function extractRequestIdFromUnknown(body: unknown): string | null {
if (!body || typeof body !== "object") return null;
const b = body as Record<string, unknown>;
if (typeof b.request_id === "string") return b.request_id;
@ -194,13 +227,12 @@ function extractRequestId(body: unknown): string | null {
if (Array.isArray(errors) && errors.length > 0) {
const first = errors[0];
if (first && typeof first === "object") {
const detail = (first as Record<string, unknown>).detail;
if (typeof detail === "string") {
const match = detail.match(/request[_ ]id[=:]?\s*([a-zA-Z0-9-_]+)/i);
const rec = first as Record<string, unknown>;
if (typeof rec.request_id === "string") return rec.request_id;
if (typeof rec.detail === "string") {
const match = rec.detail.match(/request[_ ]id[=:]?\s*([a-zA-Z0-9-_]+)/i);
if (match) return match[1];
}
const reqId = (first as Record<string, unknown>).request_id;
if (typeof reqId === "string") return reqId;
}
}
return null;

222
lib/crates/fabro-server/src/run_files.rs
View file

@ -1,18 +1,15 @@
#![allow(clippy::result_large_err, unreachable_pub)]
//! GET /api/v1/runs/{id}/files — per-request coalescing and handler.
//! `GET /api/v1/runs/{id}/files` — handler, coalescing primitive, and
//! per-run materialization pipeline.
//!
//! This module exposes the per-run request-coalescing primitive consumed by
//! the Files Changed endpoint. Concurrent HTTP callers for the same run share
//! one materialization; concurrent callers for different runs proceed in
//! parallel. See Unit 4 of the Run Files Changed plan for design rationale.
//!
//! The materialization is deliberately driven by [`tokio::spawn`] rather than
//! polling a `Shared` future: the spawned task makes progress regardless of
//! whether any caller is still waiting, so an abandoned request cannot leave
//! orphan git subprocesses in the sandbox. All panics are caught and surfaced
//! as a 500 `ApiError`, and the registry entry is removed on task completion
//! so a follow-up request triggers a fresh materialization.
//! Concurrent callers for the same run share one materialization; different
//! runs proceed in parallel. Materialization is driven by [`tokio::spawn`]
//! so it makes progress regardless of caller liveness — an abandoned
//! request cannot leave orphan git subprocesses in the sandbox. Panics are
//! caught and surfaced as 500 `ApiError` to every coalesced caller; the
//! registry entry is removed on task completion so a follow-up request
//! triggers a fresh materialization.
use std::collections::{HashMap, HashSet};
use std::future::Future;
@ -54,6 +51,18 @@ pub(crate) const FILE_COUNT_CAP: usize = 200;
/// Sandbox git timeout. Matches Unit 3 helpers (10 s).
const SANDBOX_GIT_TIMEOUT_MS: u64 = 10_000;
/// Below this SHA count the phase-1 `cat-file --batch-check` pre-filter is
/// skipped — its ~100 ms round-trip dominates for small diffs, and phase-2
/// already size-caps per blob.
const METADATA_PHASE_SHA_THRESHOLD: usize = 10;
fn transient_503(op: &str, message: &str) -> ApiError {
ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox {op} failed: {message}"),
)
}
/// Query parameters accepted by `GET /runs/{id}/files`.
#[derive(Debug, Deserialize, Default)]
pub struct ListRunFilesParams {
@ -151,22 +160,19 @@ where
// ── HTTP handler ──────────────────────────────────────────────────────────
/// `GET /api/v1/runs/{id}/files` — real handler.
/// `GET /api/v1/runs/{id}/files` handler.
///
/// Flow:
/// 1. Parse & authenticate
/// 2. Reject non-default `from_sha`/`to_sha` per R15 (v1 only serves the full
/// run diff)
/// 3. Load run projection (404 on missing/unauthorized — IDOR-safe)
/// 4. Try to reconnect the sandbox; on success, run the sandbox git helpers and
/// build a structured response
/// 5. Fall through to empty envelope when no sandbox path is available. Unit 6
/// replaces this branch with the `final_patch` degraded fallback.
/// 1. Parse + authenticate. Reject non-default `from_sha`/`to_sha` (v1 only
/// serves the full run diff).
/// 2. Load the run projection. 404 covers both missing run and missing access —
/// IDOR-safe.
/// 3. Try to reconnect the sandbox; on success, build a structured diff.
/// 4. On reconnect failure or garbage-collected base, fall through to a
/// degraded response built from `RunProjection.final_patch`.
///
/// All logging emits a single `tracing::info!` with an allowlisted field
/// set: `run_id, file_count, bytes_total, duration_ms, truncated,
/// binary_count, sensitive_count, symlink_count, submodule_count`. No
/// paths, contents, or raw git stderr are logged.
/// set enforced by [`RunFilesMetrics::emit`] — no paths, contents, or raw
/// git stderr.
pub async fn list_run_files(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
@ -235,7 +241,7 @@ async fn materialize_sandbox_path(state: &Arc<AppState>, run_id: &RunId) -> List
let projection = load_projection(state, run_id).await?;
let Some(base_sha) = projection.start.as_ref().and_then(|s| s.base_sha.clone()) else {
// Run hasn't started yet (no base_sha). UI maps this to R4(a).
// Run hasn't started yet — no base_sha, no diff to compute.
return Ok(empty_envelope());
};
@ -247,14 +253,20 @@ async fn materialize_sandbox_path(state: &Arc<AppState>, run_id: &RunId) -> List
));
};
// Resolve HEAD to a concrete `to_sha` and capture its commit time for
// the "Checkpoint Xm ago" freshness indicator on the client.
let to_sha = resolve_head_sha(sandbox.as_ref()).await?;
let to_sha_committed_at = resolve_commit_time(sandbox.as_ref(), &to_sha).await;
// Resolve HEAD (sha + commit time) in one round-trip.
let (to_sha, to_sha_committed_at) = resolve_head_sha_and_time(sandbox.as_ref()).await?;
// Enumerate changes. Permanent errors (bad_sha, missing object) fall
// through to the patch-only fallback; transient errors surface as 503.
let raw_entries = match list_changed_files_raw(sandbox.as_ref(), &base_sha, &to_sha).await {
// Enumerate changes and classify binary vs text in parallel — both
// traversals are mutually independent once `to_sha` is known, and
// running them sequentially would add ~100 ms per request on Daytona.
let (raw_res, binary_res) = tokio::join!(
list_changed_files_raw(sandbox.as_ref(), &base_sha, &to_sha),
list_binary_paths(sandbox.as_ref(), &base_sha, &to_sha),
);
// Permanent errors (bad_sha, missing object) fall through to the
// patch-only fallback; transient errors surface as 503.
let raw_entries = match raw_res {
Ok(v) => v,
Err(DiffError::Permanent { .. }) => {
return Ok(build_fallback_response(
@ -263,29 +275,23 @@ async fn materialize_sandbox_path(state: &Arc<AppState>, run_id: &RunId) -> List
));
}
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox git subprocess failed: {message}"),
));
return Err(transient_503("git diff --raw", &message));
}
};
let binary_paths = match list_binary_paths(sandbox.as_ref(), &base_sha, &to_sha).await {
let binary_paths = match binary_res {
Ok(v) => v,
Err(DiffError::Permanent { .. }) => HashSet::new(),
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox git numstat failed: {message}"),
));
return Err(transient_503("git diff --numstat", &message));
}
};
let total_changed_before_cap = raw_entries.len();
// Classify every entry against the denylist + binary/symlink/submodule
// flags FIRST so no-blob-needed placeholders don't consume cap slots that
// belong to real file changes (plan § Unit 5: R31 acts before R27).
// flags FIRST so no-blob-needed placeholders don't consume cap slots
// that belong to real file changes.
let classified = classify_entries(&raw_entries, &binary_paths, is_sensitive);
// Then cap the combined list at 200 entries.
@ -559,8 +565,8 @@ async fn load_projection(
/// Reconnect semantics tailored to the Files endpoint:
/// - `Ok(Some(sandbox))`: reconnected, caller proceeds on the sandbox path.
/// - `Ok(None)`: no sandbox record, reconnect failed, or the provider isn't
/// supported by this build — caller falls through to the fallback branch
/// (Unit 6) instead of returning 409.
/// supported by this build — caller falls through to the degraded fallback
/// instead of returning 409.
/// - `Err(ApiError)`: unrecoverable error loading run state.
async fn try_reconnect_run_sandbox(
state: &Arc<AppState>,
@ -576,36 +582,16 @@ async fn try_reconnect_run_sandbox(
}
}
/// Return the commit time of `sha` in strict ISO 8601 via
/// `git show -s --format=%cI`. `None` on any error (best-effort: the
/// handler still succeeds without the freshness timestamp).
async fn resolve_commit_time(
/// Resolve HEAD's SHA and its commit time in a single sandbox round-trip.
/// `git show -s --format=%H %cI HEAD` prints both on one line separated by
/// a space. The commit time is best-effort — if parsing fails the handler
/// still succeeds without the freshness timestamp.
async fn resolve_head_sha_and_time(
sandbox: &dyn Sandbox,
sha: &str,
) -> Option<chrono::DateTime<chrono::Utc>> {
// SHAs come from `git rev-parse HEAD` so they're trusted hex; reject
// anything non-conforming as defense in depth before interpolation.
if !sha.chars().all(|c| c.is_ascii_hexdigit()) || sha.is_empty() {
return None;
}
let cmd = format!("git -c core.hooksPath=/dev/null show -s --format=%cI {sha}");
let res = sandbox
.exec_command(&cmd, SANDBOX_GIT_TIMEOUT_MS, None, None, None)
.await
.ok()?;
if res.exit_code != 0 {
return None;
}
let iso = res.stdout.trim();
chrono::DateTime::parse_from_rfc3339(iso)
.ok()
.map(|d| d.with_timezone(&chrono::Utc))
}
async fn resolve_head_sha(sandbox: &dyn Sandbox) -> std::result::Result<String, ApiError> {
) -> std::result::Result<(String, Option<chrono::DateTime<chrono::Utc>>), ApiError> {
let res = sandbox
.exec_command(
"git rev-parse HEAD",
"git -c core.hooksPath=/dev/null show -s --format=%H\\ %cI HEAD",
SANDBOX_GIT_TIMEOUT_MS,
None,
None,
@ -619,14 +605,20 @@ async fn resolve_head_sha(sandbox: &dyn Sandbox) -> std::result::Result<String,
"Failed to resolve sandbox HEAD.",
));
}
let sha = res.stdout.trim().to_string();
let line = res.stdout.trim();
let mut parts = line.splitn(2, ' ');
let sha = parts.next().unwrap_or("").to_string();
if sha.is_empty() {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
"Sandbox HEAD resolved to an empty value.",
));
}
Ok(sha)
let committed_at = parts
.next()
.and_then(|iso| chrono::DateTime::parse_from_rfc3339(iso.trim()).ok())
.map(|d| d.with_timezone(&chrono::Utc));
Ok((sha, committed_at))
}
/// A classified changed-file entry. Preserves original enumeration order so
@ -641,8 +633,7 @@ enum ClassifiedEntry {
}
/// Classify every raw entry against the denylist + binary flags. Runs before
/// the 200-file cap so sensitive entries don't evict real changes (plan
/// § Unit 5: R31 acts before R27).
/// the 200-file cap so sensitive entries don't evict real changes.
fn classify_entries(
raw: &[RawDiffEntry],
binary_paths: &HashSet<String>,
@ -939,12 +930,12 @@ fn collect_blob_shas(classified: &[ClassifiedEntry]) -> Vec<String> {
/// Phase 2 (contents): bulk `cat-file --batch` on the remaining SHAs.
///
/// Failure modes:
/// - Phase 1 permanent error: fall through with an empty size map; phase 2
/// runs against the full SHA list (current behavior before this split).
/// - Phase 1 permanent error: fall through with an empty size map; phase 2 runs
/// against the full SHA list (current behavior before this split).
/// - Phase 1 transient error: 503 to the client.
/// - Phase 2 permanent error (malformed blob in stream): only the
/// phase-2 SHAs get `None`; phase-1-classified oversized SHAs keep their
/// `None` entries but with a semantically-accurate cause.
/// - Phase 2 permanent error (malformed blob in stream): only the phase-2 SHAs
/// get `None`; phase-1-classified oversized SHAs keep their `None` entries
/// but with a semantically-accurate cause.
/// - Phase 2 transient error: 503 to the client.
async fn fetch_blob_table(
sandbox: &dyn Sandbox,
@ -956,26 +947,28 @@ async fn fetch_blob_table(
let mut table: HashMap<String, Option<String>> = HashMap::with_capacity(shas.len());
// Phase 1: --batch-check for sizes.
let oversized: HashSet<String> = match stream_blob_metadata(sandbox, shas).await {
Ok(metas) => {
let mut set = HashSet::new();
for meta in metas {
if let Some(size) = meta.size {
if size > PER_FILE_BYTES_CAP {
set.insert(meta.sha);
}
}
// Phase 1: --batch-check for sizes. Skipped for small SHA lists where
// the pre-filter's ~100 ms round-trip is pure overhead — `stream_blobs`
// already size-caps per blob and returns `None` for oversized ones.
// Phase 1 only earns its cost when a single malformed/huge blob could
// poison a large batch's parse.
let oversized: HashSet<String> = if shas.len() >= METADATA_PHASE_SHA_THRESHOLD {
match stream_blob_metadata(sandbox, shas).await {
Ok(metas) => metas
.into_iter()
.filter_map(|m| {
m.size
.filter(|size| *size > PER_FILE_BYTES_CAP)
.map(|_| m.sha)
})
.collect(),
Err(DiffError::Permanent { .. }) => HashSet::new(),
Err(DiffError::Transient { message }) => {
return Err(transient_503("git cat-file --batch-check", &message));
}
set
}
Err(DiffError::Permanent { .. }) => HashSet::new(),
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox git cat-file --batch-check failed: {message}"),
));
}
} else {
HashSet::new()
};
// Record oversized blobs in the table as `None` so the caller emits
@ -1009,10 +1002,7 @@ async fn fetch_blob_table(
}
}
Err(DiffError::Transient { message }) => {
return Err(ApiError::new(
StatusCode::SERVICE_UNAVAILABLE,
format!("Sandbox git cat-file --batch failed: {message}"),
));
return Err(transient_503("git cat-file --batch", &message));
}
}
@ -1189,9 +1179,9 @@ mod tests {
#[tokio::test]
async fn first_caller_cancelling_does_not_block_other_callers() {
// P2-13 regression: tokio::spawn detaches materialization from any
// individual caller, so the first caller dropping its future must
// not prevent a subsequent caller from receiving the result.
// tokio::spawn detaches materialization from any individual
// caller; the first caller dropping its future must not prevent
// a subsequent caller from receiving the result.
let inflight = new_registry();
let counter = Arc::new(AtomicUsize::new(0));
let run = run_id("run_ffffffffffffffffffffffffff");
@ -1230,7 +1220,7 @@ mod tests {
assert_eq!(counter.load(Ordering::SeqCst), 1);
}
// ── Tracing allowlist assertion (P2-11) ──────────────────────────────
// ── Tracing allowlist assertion ──────────────────────────────
use std::sync::{Mutex as StdMutex, OnceLock};
@ -1282,8 +1272,8 @@ mod tests {
#[test]
fn run_files_metrics_emit_writes_only_allowlisted_fields() {
// Plan § Unit 5: the tracing field set is an allowlist — no paths,
// contents, or raw git stderr may leak.
// The tracing field set is an allowlist — no paths, contents, or
// raw git stderr may leak.
let captured = install_tracing_capture();
captured.lock().unwrap().clear();
@ -1391,9 +1381,9 @@ diff --git a/src/bar.rs b/src/bar.rs
#[test]
fn strip_denylisted_sections_catches_rename_with_sensitive_old_side() {
// Regression for P1-2: renaming away from a sensitive path must
// still strip the patch — the benign new path alone doesn't reveal
// the secret but the hunk body does.
// Renaming away from a sensitive path must still strip the patch
// — the benign new path alone doesn't reveal the secret but the
// hunk body does.
let patch = "\
diff --git a/.env.production b/docs/NOTES.md
rename from .env.production
@ -1414,9 +1404,9 @@ rename to docs/NOTES.md
#[test]
fn stitch_file_diff_returns_distinct_old_and_new_contents_for_modified() {
// Regression for P1-1: before this fix, the handler fetched only the
// new-side blob and duplicated it onto both sides, producing no-op
// diffs for all modified files.
// Modified files must expose distinct old/new contents; pulling
// only the new_blob and duplicating it would render as a no-op
// diff in `MultiFileDiff`.
let entry = RawDiffEntry::Modified {
path: "src/main.rs".to_string(),
old_blob: "aaaa000000000000000000000000000000000000".to_string(),

41
lib/crates/fabro-server/src/run_files_security.rs
View file

@ -1,19 +1,13 @@
#![allow(unreachable_pub, dead_code)]
#![allow(unreachable_pub)]
//! Security helpers shared by the Run Files Changed endpoint: a globset-based
//! sensitive-path denylist, a sandbox-git env-hardening helper, and a
//! structured metrics emitter that enforces the tracing allowlist.
//! Security helpers for the Run Files Changed endpoint: a globset-based
//! sensitive-path denylist and a structured metrics emitter that enforces
//! the tracing allowlist.
//!
//! All matching is path-based and case-insensitive. The denylist is a
//! defense-in-depth control — it is not a content scanner and will not
//! catch arbitrary secrets hidden inside non-secret file extensions.
//!
//! `sandbox_git_env` and `RunFilesMetrics` are intentionally public APIs
//! even though they're currently consumed by a single caller — the module
//! is designed as a reusable surface for any future sensitive-data-adjacent
//! endpoint.
//! Matching is path-based and case-insensitive. The denylist is a
//! defense-in-depth control — not a content scanner, and it won't catch
//! arbitrary secrets hidden inside non-secret file extensions.
use std::collections::HashMap;
use std::sync::OnceLock;
use fabro_types::RunId;
@ -140,17 +134,6 @@ fn normalize_for_match(path: &str) -> String {
out
}
/// Environment additions applied to every sandbox-side git invocation under
/// the Run Files endpoint. Pairs with the hardened `-c` flags the sandbox
/// git helpers already use.
#[must_use]
pub fn sandbox_git_env() -> HashMap<String, String> {
HashMap::from([
("GIT_TERMINAL_PROMPT".to_string(), "0".to_string()),
("GIT_EXTERNAL_DIFF".to_string(), String::new()),
])
}
/// Metrics emitted at the tail of every Run Files response. The field set is
/// deliberately the only shape of tracing output the endpoint produces —
/// enforced by `emit`, which never interpolates or logs individual paths,
@ -262,14 +245,4 @@ mod tests {
// A non-sensitive unicode path must still not match any glob.
assert!(!is_sensitive("docs/Ähnlichkeit.md"));
}
#[test]
fn sandbox_git_env_sets_expected_pairs() {
let env = sandbox_git_env();
assert_eq!(
env.get("GIT_TERMINAL_PROMPT").map(String::as_str),
Some("0")
);
assert_eq!(env.get("GIT_EXTERNAL_DIFF").map(String::as_str), Some(""));
}
}

214
lib/crates/fabro-spa/assets/assets/entry-8hk343fy.js → lib/crates/fabro-spa/assets/assets/entry-8nkj5gta.js generated
View file

File diff suppressed because one or more lines are too long

2
lib/crates/fabro-spa/assets/index.html generated
View file

@ -61,7 +61,7 @@
<script type="module" src="/assets/chunk-sadshphz.js"></script>
<script type="module" src="/assets/chunk-pmthkscp.js"></script>
<script type="module" src="/assets/chunk-v61ks9f7.js"></script>
<script type="module" src="/assets/entry-8hk343fy.js"></script>
<script type="module" src="/assets/entry-8nkj5gta.js"></script>
<script type="module" src="/assets/chunk-n1k68xa8.js"></script>
<script type="module" src="/assets/chunk-rsph5pvm.js"></script>
<script type="module" src="/assets/chunk-9t57pdty.js"></script>