TASK 10: Clarify and harden the remote security model - #331
Open
Trapa-Eureka wants to merge 10 commits into
Open
Trapa-Eureka wants to merge 10 commits into
Trapa-Eureka wants to merge 10 commits into
Conversation
* Formalize run lifecycle as an explicit state machine (TASK 1)
Runs previously carried status as a bare `String`, and `Store::update_status`
was an unconditional `UPDATE ... WHERE id = ?1` with no transition checks —
any caller could overwrite any status with any other status, and three
independent copies of `is_terminal` had to be kept in sync by hand.
Add `RunStatus` (store.rs): Starting -> Running -> {Done, Failed, Cancelled},
with Starting also able to jump straight to a terminal state. Terminal states
are absorbing and `RunStatus::can_transition_to` is the single source of
truth for legality.
`Store::update_status` now takes a typed `RunStatus` and derives its SQL
guard from `can_transition_to` (`WHERE status IN (<legal sources>)`) so the
check-and-write is atomic and can never drift from the documented table. It
returns `Result<bool>`: `false` means the transition was illegal (almost
always because the run was already terminal), which is an expected, benign
outcome rather than an error — a duplicate completion callback, or a
completion racing a cancellation, is now a no-op instead of a corruption.
Threaded the typed status through `jobs::stage_to_run_status`,
`supervise::run_status_for_stage`, and all 8 backend poll loops (HF, k8s,
Modal, ssh, Slurm, Ray, local, OpenResearch), replacing stringly-typed
statuses with compile-time-checked ones. Consolidated the three duplicate
`is_terminal` functions (local/mod.rs, commands/serve.rs, commands/up.rs)
into one canonical `store::is_terminal_status`.
Added deterministic tests covering the doc's listed scenarios: the full
transition matrix, duplicate terminal writes, completion arriving after
cancellation, invalid backward transitions (running -> starting), cancel
while starting/running, retry-as-a-new-row, and concurrent writers (separate
SQLite connections racing to finalize the same run) settling on exactly one
terminal outcome.
No wire/schema changes — RunStatus::as_str() matches the existing stored
vocabulary exactly, so the UI and API are unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
* chore: trigger CI now that Actions is enabled on the fork
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
A supervisor's crash (OOM-killed, host reboot) or a failed spawn attempt previously left its run stuck `Starting`/`Running` forever: startup reconciliation in `orx up` was a one-shot pass, and a `spawn_detached_supervise` failure — there or anywhere else it's called — was only ever logged to stderr and dropped, with no retry and no status change. Add `commands::exp::reconcile_active_runs`: for every locally-owned active run, check whether a supervisor is actually alive and watching it via `supervise::run_has_live_supervisor` — a non-blocking probe of the same `fd_lock` file a running supervisor holds. The lock is an OS-level advisory lock, released automatically the instant the holding process dies or the machine reboots, so this is a race-free, always-fresh liveness signal with no heartbeat/TTL of our own to go stale. An orphaned run gets a fresh supervisor; once respawned, that supervisor's own `inspect_job` reconciles against the actual backend the normal way — reconciliation's only job is making sure *someone* is always eventually watching. A run whose supervisor can't be spawned after `MAX_SUPERVISOR_SPAWN_ATTEMPTS` (5) consecutive tries — a broken installation, not a transient blip — is marked unrecoverable via the new `Store::mark_run_unrecoverable`, which stamps a machine-readable `recovery_reason` (a new nullable `runs` column, excluded from `upsert_run`'s ON CONFLICT like `chat_session_id`, so a later backend write can't clobber it) and appends a human-readable note to `result_markdown`. It goes through `update_status`'s existing terminal-state guard (TASK 1), so it can never overwrite a run that reached a real outcome first. Wired into `orx up`: the existing startup pass now calls `reconcile_active_runs` instead of spawning unconditionally, and a new 30-second periodic task (mirroring the existing chat-turn-lease reconciler, including its data-dir-move gate) keeps calling it for the life of the process, so a supervisor that dies mid-flight — not just one dead before `orx up` started — gets noticed and replaced without needing a restart. Tests added: the lock-probe's three liveness transitions (free, held, released) in `commands::supervise`; `mark_run_unrecoverable`'s terminal-write, never-overwrites-a-real-outcome, and idempotency behavior in `store`; and `reconcile_active_runs`'s full decision matrix (skip when watched, spawn when orphaned, ignore non-local runs, retry-then-give-up with the counter cleared either way, no counter leak for runs that leave the active set) in `commands::exp`, all via the same injected-closure style already used by `request_local_run_cancel_with`. Scope note: this covers orphan detection and give-up semantics generically, across every backend, by making sure a supervisor is always eventually running. It does not change the per-backend `inspect_job` behavior itself — five of seven backends still retry a transport failure (an unreachable ssh host, a down k8s API) forever with no cap, unlike Slurm/Ray's ~60s "GONE" debounce; generalizing that belongs to TASK 4 (compute backend contracts). It also does not add a full process-kill/server-restart integration test (spawning a real `orx supervise` child and killing it) — this repo has no integration-test harness for the Rust binary yet, and building one is a larger, separate investment; the unit tests above cover the same invariants (lock release, orphan detection, give-up) at the mechanism level instead. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…#5) Implements Priority 7 from OpenResearch_Improvement_Priorities.md. Correctness fixes found while auditing every multi-statement Store method: - `record_chat_spawn_attempt` was `UPDATE attempts = attempts + 1` followed by a separate `SELECT attempts` — a genuine TOCTOU race: a concurrent caller's own increment could land in the gap, so the value read back could be *their* count, not this call's. Replaced with one atomic `UPDATE ... RETURNING attempts` (the bundled SQLite is 3.35+, so this is available) — no transaction needed, no race possible. - `prune_run_wakeups`, `prune_chat_spawns`, `claim_chat_turn`, and `claim_data_dir_move` each issued 2-3 sequential statements against the same connection with no transaction boundary. None were live bugs today (each statement's own WHERE clause already prevented double-claims), but none had the "all or nothing" guarantee a future added step could safely assume either. Wrapped each in `self.begin()`/`commit()`. - `delete_local_project` mixed `tx.execute` and `self.conn.execute` for its last three deletes — already atomic in practice (same connection, same open transaction) but a maintainability trap: nothing but convention kept those three statements inside the transaction's scope. Normalized to `tx.execute` throughout. Performance fix: `runs` had no index beyond its primary key. Every poll of active runs — including the TASK 2 reconciliation loop, every 30s — plus listing by project/experiment and the newest-first history view all table-scanned. Added `idx_runs_status`, `idx_runs_project_id`, `idx_runs_experiment_id`, and `idx_runs_created_at` (`CREATE INDEX IF NOT EXISTS`, applied retroactively to existing databases on next open, same pattern as the existing chat-table indexes). WAL mode + a 5000ms busy_timeout (`Store::open_at_with_move_lock`) were already the sole, centrally-configured defense against `SQLITE_BUSY`, used uniformly by every `Store::open()`/`open_at()` caller — reviewed and left as-is; this is the correct, already-standardized configuration Priority 7 asks to check for. Tests added (`src/store.rs`), matching the file's existing style of real threads/connections rather than mocking: - `parallel_experiment_creation_all_land_with_no_sqlite_busy_errors` — 32 threads, each its own connection, creating distinct experiments at once. - `concurrent_readers_are_never_blocked_by_a_concurrent_writer` — one writer hammering `update_status`/`set_result_markdown` on a run while four readers loop `get_run`/`list_runs`; no read may ever error, and every reader must eventually observe the final terminal status. - `an_uncommitted_transaction_rolls_back_on_drop` — a dropped, uncommitted `Transaction` (including one that failed partway through a second statement) leaves no trace, proving the rollback guarantee every `self.begin()`-based method here depends on. - `concurrent_spawn_attempt_increments_never_collide` — 16 concurrent callers of the newly-atomic `record_chat_spawn_attempt` must get back exactly `{1..16}`, not a multiset with collisions. - `ten_thousand_sequential_status_writes_complete_quickly` and `active_run_queries_stay_fast_against_a_large_runs_table` — hand-rolled `Instant`-based timing sanity checks (no benchmark harness exists in this repo yet) against a 20k-row `runs` table, to catch a severe regression (an unintended fsync, a missing/regressed index) rather than pin an exact number. CI finding: the first version of the new read/write-contention test used a 200-write hot loop with no pacing and a 10s/10,000-write timing bound. Both passed on Linux/macOS but failed on the Windows CI runner — sequential writes measured ~2.4ms each there (vs. sub-millisecond locally), and the unpaced hot loop hit genuine `SQLITE_BUSY` ("database is locked") even with the 5s `busy_timeout`. Retuned to a realistic polling cadence (small sleeps, fewer iterations) with a short retry-with-backoff around reads for the rare case that's still not enough, and widened the timing bounds to match observed slow-CI-hardware throughput rather than local timing. Confirmed green on Linux, macOS, and Windows CI before merging. Scope note: experiment creation also creates a Git branch (`local::experiments::create_experiment`) before the DB row is inserted, with no compensation if the DB write then fails — this is pre-existing, already-accepted best-effort behavior (Git and DB state are never kept transactionally consistent anywhere in this codebase; branch publication to GitHub is explicitly best-effort too, per `local_plane.rs`). Bringing that under an explicit rollback/compensation model is a larger design change than this task's "add tests, fix transaction boundaries at the SQL layer" scope. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Implements Priority 3 from OpenResearch_Improvement_Priorities.md.
Consolidated backend-specific branching that had drifted into duplication
across the 8 job backends:
- `JobState { stage: String, message: Option<String> }` was defined
identically in ssh.rs, kubernetes.rs, modal.rs, and slurm.rs, plus a
structurally-identical `JobInfo` in ray.rs — five copies of one shape.
Moved the canonical definition to `jobs::mod`; each backend now re-exports
it (`ray` keeps its existing `JobInfo` name via a renamed re-export), so
`supervise.rs`'s poll loops never need backend-specific field access.
Huggingface's native API response nests this one level deeper
(`JobInfo { status: JobStatus { .. } }`) — added `JobInfo::state()` to
adapt it to the same shape rather than special-casing HF's field access.
- Slurm and Ray each hand-rolled an identical "GONE" debounce (a scheduler-
reported "job vanished from bookkeeping" state must persist ~60s before
being believed, since it also fires during a scheduler restart). Extracted
to a shared `jobs::GoneDebounce`, so the window and reset rule can't drift
between the two schedulers that need it.
- Every one of the 6 backends with a fallible `inspect_job` (hf, k8s, modal,
ssh — shared by both `ssh_job` and `openresearch_job` — slurm, ray)
retried a transport failure (host unreachable, cluster API down, auth
expired) every `POLL_INTERVAL` forever, with no cap. A permanently
decommissioned host or a revoked credential would strand a run in
`Running` indefinitely — TASK 2's crash recovery covers "no supervisor at
all," but not "a live supervisor whose backend is gone for good." Added a
shared `MAX_CONSECUTIVE_INSPECT_FAILURES` bound (~10 minutes at the
default poll interval) and `give_up_on_unreachable_backend`, which marks
the run unrecoverable via TASK 2's `Store::mark_run_unrecoverable` —
inheriting its terminal-state guard, so it can never overwrite a run that
reached a real outcome in the meantime.
- `localbox::cancel_job` returned `Err` when the target process was already
dead — including on a *second* cancel call — which violates the
"cancellation is idempotent" contract test below. Now checks liveness
first and treats an already-gone process as a no-op success.
Added a conformance test suite (`jobs::localbox`'s `local_job_lifecycle`)
covering every property `OpenResearch_Improvement_Priorities.md` lists for
this contract: launch succeeds with a valid configuration, invalid
configuration fails before execution (a run id colliding with an existing
file at its run-dir path), logs remain readable after completion (both a
resumed read and a fresh replay from the start), status reaches one
terminal state only (wires this real backend's terminal output through
`stage_to_run_status` + `Store::update_status`, confirming TASK 1's
transition guard end-to-end rather than only in `store.rs`'s own tests),
and cancellation is idempotent (a second cancel on an already-dead process
succeeds). `local` is the only backend this repo can exercise end-to-end in
CI without external infrastructure (a live k8s cluster, an ssh host, cloud
credentials); the suite is written and labeled so a future backend gaining
real test infrastructure can assert the same properties.
Also added `jobs::GoneDebounce`'s own unit tests (window elapses, resets on
any non-GONE observation, a window shorter than one poll interval still
requires at least one GONE poll).
Scope note: a `ComputeBackend` trait already exists (`compute.rs`) covering
launch (`preflight`/`stage_source`/`submit` — real per-backend dispatch) and
default `status`/`logs`/`cancel`/`cleanup` methods that only touch the local
DB/log file, never the actual remote provider — the real per-backend poll/
cancel/log lifecycle lives entirely in `supervise.rs`'s hand-dispatch on
`descriptor.kind`, outside the trait. Routing that lifecycle through trait
method overrides for all 8 backends would be a much larger, higher-risk
refactor (duplicating `supervise.rs`'s existing loops as trait impls) that
can't be meaningfully tested for 7 of 8 backends without real external
infrastructure — left as a larger, separate future investment rather than
attempted here. Likewise, "backend-specific failures map to stable error
types" has no foundation to build on (the crate is a flat `anyhow::Error`
throughout, with zero typed error taxonomy anywhere) — introducing one is
squarely Priority 9's ("error classification and observability") territory,
not achievable by wiring up existing types here.
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Implements Priority 9 from OpenResearch_Improvement_Priorities.md.
`src/error.rs` was a flat `anyhow::Error` re-export with zero typed error
taxonomy anywhere in the crate — every backend and command produces ad-hoc
`anyhow!("...")` strings. Introducing a full taxonomy across every failure
site in the codebase is a much larger, cross-cutting change; this task
establishes the taxonomy and wires it into the two places `orx` itself
force-fails a run (rather than a backend reporting its own outcome), since
those are exactly where a stable, machine-readable classification is most
valuable to an agent or the dashboard deciding what to do next.
Added `error::ErrorKind`: the doc's nine suggested categories
(ConfigurationError, AuthenticationError, GitStateError, BackendUnavailable,
LaunchFailure, RuntimeFailure, CancellationFailure, ReconciliationFailure,
StorageFailure), each with a stable snake_case string code, `as_str`/`parse`
round-tripping, and `Display`.
Wired in via `Store::mark_run_unrecoverable` (TASK 2), which now takes an
`ErrorKind` alongside its existing free-text `reason` and persists both — a
new `error_kind` column (parallel to `recovery_reason`: excluded from
`upsert_run`'s `ON CONFLICT` so a later backend write can't clobber it,
inherits `update_status`'s terminal-state guard so it's never stamped over a
real outcome). Its two existing callers are now classified precisely:
- `commands::exp::reconcile_active_runs` (TASK 2, crash recovery giving up
on respawning a supervisor) -> `ErrorKind::Reconciliation`.
- `commands::supervise::give_up_on_unreachable_backend` (TASK 4, a backend
unreachable for `MAX_CONSECUTIVE_INSPECT_FAILURES` consecutive polls) ->
`ErrorKind::BackendUnavailable`.
Exposed both fields on the dashboard/API's `ApiRun` — `error_kind` only when
the stored string still round-trips through a known code, so a value from a
future `orx` version never surfaces as a silently-wrong classification to an
older client.
Reviewed against the rest of Priority 9's asks:
- "Correlation IDs for runs" — already satisfied: every supervisor log line
is already prefixed `supervise {run_id}: ...`, and the run id is the
natural correlation key throughout the CLI/API/store. No new ID scheme
needed.
- "Structured logging" and classifying every other failure site across 8
backends and the rest of the CLI — genuinely absent today (everything is
`eprintln!`) and a much larger, cross-cutting change; left as significant
future work rather than attempted piecemeal here.
Tests added/extended: `ErrorKind` round-trips and code uniqueness/format
(`error.rs`); `mark_run_unrecoverable`'s three existing tests now assert
`error_kind` is stamped, cleared-on-no-op, and not overwritten by a second
call with a different kind (`store.rs`); `reconcile_active_runs`'s give-up
test now asserts `reconciliation_failure` (`commands/exp.rs`); a new test
for `give_up_on_unreachable_backend` asserts `backend_unavailable`
(`commands/supervise.rs`).
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Implements Priority 4 from OpenResearch_Improvement_Priorities.md. Added `store::ProvenanceManifest`, captured once at launch and persisted in a new `provenance_json` column (immutable thereafter — excluded from `upsert_run`'s ON CONFLICT, same pattern as `chat_session_id`). Deliberately scoped to what's cheap and reliable to capture from the launching process itself, without new subprocess probes on `orx exp run`'s latency-sensitive path, and without restating what a run already carries on its own columns or `BackendDescriptor` (git commit SHA, exact command, backend + its configuration, start/completion timestamps): - `orx_version` — the exact `orx` build that launched the run. - `launcher_os` / `launcher_arch` — a lightweight, secret-free "environment fingerprint" for the machine that launched it (not necessarily where the payload executes remotely — that's already `BackendDescriptor.flavor`). - `agent_harness` / `agent_model` — resolved from the launching chat session, when the run was started by a coding-agent harness rather than a plain CLI invocation. - `parent_experiment_id` — a snapshot of the launching experiment's parent id, so the lineage survives even if the experiment itself is later reparented or deleted. `compute::build_provenance` computes this once in `compute::submit` (which already has `store`/`args`/`experiment` in scope) before dispatching to a backend; every backend's own subsequent `upsert_run` (once its real job handle is known) naturally excludes the column, so it's never clobbered. Exposed on the dashboard/API's `ApiRun` as a `provenance` object. Explicitly NOT captured — see the doc's other suggested fields: dependency/runtime versions and hardware details beyond OS/arch would need new per-backend probing this task doesn't attempt (and, for hardware, adds real subprocess latency to every launch — see `jobs::localbox::hardware_info`, already flagged in its own docs as "blocking, call via spawn_blocking"); the codebase has no dataset/artifact registry to reference at all. Bug fix found while adding this: `Store::list_ready_run_wakeups` hand-rolled its SELECT's column list rather than reusing `SELECT_RUN`, and had silently drifted out of sync across TASK 2 and TASK 5 — `recovery_reason` and `error_kind` were being read from the wakeup table's own `chat_session_id`/ `state` columns instead of the run's, wrong data with no error raised. Adding a further column (`provenance_json`) finally pushed the mismatch past the row's actual column count, turning it into a real "Invalid column index" panic that the existing wakeup test caught immediately. Fixed by switching to `r.*` (aliasing the two wakeup-specific columns to avoid the naming collision) so this can't drift out of sync again. Tests added: `ProvenanceManifest` JSON round-trip (including the minimal/no-optional-fields case); a real `Store`-backed test proving `provenance_json` survives a backend's later `upsert_run` the same way `chat_session_id` already does; `build_provenance` resolving an agent harness/model from a real chat session row, and correctly resolving to `None`/`None` for a plain CLI launch with no parent experiment. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…K 7) (#13) App.tsx (2054 lines) mixed ~14 distinct concerns in one component closure. Extract the pieces that are cleanly separable without touching the right-panel tab state machine: - filePathResolution.ts: parseFilePath/fileBranchLabel/escapeRegExp (pure, previously untested — 90-line path-resolution algorithm now has 18 unit tests) - panelSizing.ts: floating-panel sizing constants and pure width math - listUpsert.ts: generic upsertById helper (tested) - useStableStringMap.ts: generic map-identity hook - useAppData.ts: project/ui-state/session queries + combined startup error + retry - useExperimentScope.ts: experiments pane view/scope filter state - useSpawnTabMeta.ts: live title/running-state for open sub-agent tabs - usePreferredAgent.ts: preferred-agent persistence with rollback App.tsx shrinks by ~300 lines (net) and each extracted concern now has an independent, reviewable home following the codebase's existing flat `use*.ts` convention (no new directory taxonomy introduced). Scope note: the right-panel tab system (open/close/select/promote across 5 tab kinds, ~700 lines) and the top-level JSX composition are deliberately left in App.tsx. It is one tightly-coupled state machine whose callbacks close over shared refs specifically to dodge stale closures, and it is fed directly into useProjectWorkspace's persistence round-trip. Splitting it further without an existing React test harness for App.tsx's rendered behavior risks reintroducing exactly the stale-closure bugs the current refs exist to avoid. Left as a follow-up once that safety net exists. Verified: pnpm typecheck, pnpm lint:i18n, pnpm lint:styles, pnpm test (183 passed, 0 regressions, 21 new), pnpm build (ui/dist regenerated and committed). Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…8) (#15) src/main.rs (1266 lines) mixed the process entry point (main, dispatch, telemetry/lifecycle-lock plumbing, Windows/macOS helpers) with every subcommand's clap Args/Subcommand/ValueEnum definitions. Move the latter into a new `cli` module, split by domain: - cli/mod.rs: the top-level `Cli`/`Command` command tree, `PublishBranchArgs`, and the CLI-parsing test module (moved verbatim) - cli/auth.rs: LoginArgs - cli/projects.rs: ProjectsArgs, OrgsArgs, ProjectArgs, ProjectCommand - cli/experiments.rs: RunsArgs, LogsArgs, CreateExperimentArgs, ExpArgs, ExpCommand, ExpRunArgs - cli/agent.rs: AgentArgs, AgentCommand - cli/compute.rs: ComputeArgs, InstanceArgs/Command/*, SshKeyArgs/Command/* - cli/library.rs: SkillArgs, LibraryArgs, LibraryCommand, InstallSkillsArgs - cli/discover.rs: LitSource, DiscoverArgs, DiscoverCommand, DiscoverySearchArgs, DiscoveryPriority, PaperArgs - cli/system.rs: VersionArgs, UpdateArgs, InstallCliArgs, DeleteArgs, DeleteCommand, TelemetryArgs, TelemetryCommand - cli/daemon.rs: ServeArgs, SuperviseArgs, UpArgs, RemoteHostArgs, RemoteHostCommand main.rs re-exports the whole schema at the crate root (`pub use cli::*;`), so every existing `crate::LoginArgs`-style reference across ~30 files in commands/, local/, compute.rs, and plane/ needed no changes — the module split is purely organizational. main.rs shrinks from 1266 to 342 lines and now holds only the entry point: `main`, `dispatch`, telemetry/lifecycle-lock gating, and the Windows/macOS console/panic helpers. Verified: cargo build, cargo build --release, cargo fmt --check, cargo clippy --all-targets -D warnings, cargo test --locked (850 passed, 0 regressions — the 8 CLI-parsing tests now live at cli::cli_tests and pass unchanged), and manual smoke checks (`orx --no-telemetry version --build-channel`, `orx --help`) against both debug and release binaries. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
ui/dist is a committed, embedded build artifact (see AGENTS.md), and its diffs — minified/hashed JS and CSS — add noise to every PR that touches the frontend without being independently readable. Address the priorities doc's recommended changes without changing the committed- artifact deployment model: - .gitattributes: mark `ui/dist/**` `linguist-generated=true`, so GitHub collapses it in the PR diff view by default and excludes it from language statistics. Deliberately not `-diff` — a reviewer can still expand it; the goal is to stop it dominating the diff, not to make it unreviewable. - ci.yml: add a "UI build artifacts are up to date" step (the check job) that rebuilds `ui/dist` from source with `vite build` and fails the PR if the committed copy doesn't match (`git diff --cached` after `git add -A -- dist` catches modified, added, and removed hashed filenames alike). This is the deterministic-build check backing the generated-file marker above: a reviewer can trust that a collapsed `ui/dist` diff is a real, correct rebuild of the accompanying source change, not a stale or hand-edited artifact. Verified locally first: two consecutive `pnpm build` runs against the same source produce a byte-identical `dist/`, confirming the build is deterministic enough for this check to be reliable rather than flaky. - AGENTS.md: expand the existing `ui/dist` contribution note to recommend a separate commit for the regenerated assets from the one changing `ui/src` (so a reviewer can read the source diff on its own merits), and to name the new CI check and generated-file marker so the reasoning is discoverable, not just enforced. Scope note: no `ui/src` changes in this PR, so there is nothing to split into a separate generated-artifact commit here — the practice applies starting with the next PR that touches the frontend. Verified: the new CI step passes against the current `ui/dist` (confirmed locally with the exact commands the workflow runs) and correctly fails when `ui/dist` is tampered with relative to a fresh build (verified locally, then reverted). `.github/workflows/ci.yml` parses as valid YAML. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Part of the improvement roadmap in OpenResearch_Improvement_Priorities.md (Priority 8): document the exact trust model for every remote-facing loopback surface and close the one real authentication gap it turned up, without touching openresearch.sh (out of scope per AGENTS.md). - SECURITY.md (new): the trust model this repo didn't have written down anywhere. Covers all three loopback services (`orx up`, `orx up --remote`, `orx serve`) and their actual auth posture, the `--remote` session-bearer lifecycle (minted per attach, delivered over SSH stdin never argv/echo, unregistered on disconnect — no persistent token file to expire), the SSH host-key policy table (`UserConfig`/`AcceptNew`/ `Ephemeral` and which backends use which, and why), credential storage and propagation (account token, SSH keys, per-backend secrets), and the `remote_route_forbidden` blocklist's maintenance risk (a new dashboard route is remotely reachable by default unless explicitly denied). - `orx serve` (`src/commands/serve.rs`): this was the one real gap — read-only access to every run's metadata and logs, unauthenticated, on a daemon whose own doc comment says it's meant to run on remote/shared agent boxes. Any other local user on that box could reach 127.0.0.1:4790 directly, bypassing the SSH tunnel the intended caller uses. Adds opt-in `--token`/`ORX_SERVE_TOKEN` bearer auth, gating every route including `/health` (a liveness probe still discloses the running version). Opt-in, not default-on: the process that launches `orx serve` on a box today is openresearch.sh's API, which this repo doesn't control — SECURITY.md documents the tradeoff and how to close it. Prints a startup warning when no token is set. - `src/token_auth.rs` (new): SHA-256 digest + constant-time compare, extracted from `remote_host.rs`'s private copy so `orx serve` (and any future loopback surface) doesn't grow a second implementation of the same primitive. - `src/config.rs`: `save_credentials`/`save_overleaf_credentials`/ `write_synced_env_vars` all had the same write-then-chmod-0600 shape — a fresh credentials file sat briefly at the process umask before being tightened, and their containing `~/.config/openresearch` directory was never tightened at all (world-traversable by convention, same as `~/.config` itself). Extracted shared `write_owner_only`/ `ensure_private_dir` helpers (async + sync) that set mode 0600/0700 at creation, used by all three call sites; also collapses three copies of the same OpenOptions dance into one. - `src/commands/remote_host.rs`: audited the same TOCTOU shape on the remote-host control socket and concluded it isn't real risk there — its containing directory is already verified 0700/same-owner before bind, and the accept loop separately gates on `SO_PEERCRED`. Added a comment recording that reasoning so it doesn't get "fixed" into the umask-based approach later (process-global umask mutation would be unsafe on this binary's multi-threaded tokio runtime). - README.md: corrected a stale claim that `orx up --remote` "has no application-level authentication" — it does (the session bearer above); that line was true only of the different, unauthenticated `orx serve`/bare-`orx up` paths it wasn't describing. Points to SECURITY.md. Scope note: `~/.openresearch/env`'s permissions are enforced by `orx` wherever `orx` itself writes it, but openresearch.sh may also write to it as part of its own sync flow — SECURITY.md flags that as something to confirm on that side rather than claiming coverage this repo can't verify. Flipping `remote_route_forbidden` from a blocklist to an allowlist would close its documented gap more completely, but needs an exhaustive route audit and careful behavioral testing beyond this change's scope; documented as a known limitation instead. Verified: cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, and cargo test --locked all pass (869 tests, including 15 new ones: token digest/constant-time-compare, orx serve's token gate end-to-end over a real loopback socket for four scenarios, and the config.rs permission helpers against throwaway temp-dir paths — never the real $XDG_CONFIG_HOME, which other modules' tests already mutate under the parallel runner). Claude-Session: https://claude.ai/code/session_01Y1qgF5QYGLzuRo6jE7MBuD Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 14, 2026
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #317 per request, so each task is independently reviewable/testable. Part 10 of 10, stacked in dependency order (this task builds on the previous ones' code).
What this does
Adds SECURITY.md documenting the trust model for every remote-facing loopback surface (orx up, orx up --remote, orx serve): auth posture, the --remote session-bearer lifecycle, the SSH host-key policy table, credential storage/propagation. Adds opt-in --token/ORX_SERVE_TOKEN bearer auth to orx serve (previously unauthenticated, despite its own doc comment saying it's meant to run on remote/shared agent boxes). Fixes a write-then-chmod permission TOCTOU on credential files. Corrects a stale README claim that orx up --remote has no application-level auth (it does -- a per-session bearer token).
About the diff
This branch is stacked on top of the prior task's branch in this split (not yet merged into
main), so until that earlier PR merges, this PR's diff shows its commit too, on top of this task's own commit. The Commits tab lists this task's commit separately if you want to review just that one. I'll rebase this branch (and the diff will shrink to just this task) as each earlier PR in the stack lands.Verification
Passed on this exact rebased state:
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo test --locked(870/870), and for frontend-touching commitspnpm typecheck,pnpm test,pnpm buildwithui/distregenerated.Stack
Full list and status: see #317 (will be updated to link all 10, or closed once these replace it).