stablyai / stablyai/orca

[Bug]: preflightTrust writers can discard a running agent's config changes (all presets, local and remote)

Open
#18,092 1 comment 0 reactions 1 assignee Claimed by @brennanb2025 View on GitHub
os:macos
Dominant language
TypeScript
Stars
71.3k
Forks
4.7k
Avg merge
14h 54m
Merged PRs (30d)
520

Description

### Operating system

macOS

### Orca version

1.4.194

### Details

**Short summary:** every `preflightTrust` writer does an unguarded read-modify-write of a config file the agent itself owns, so a running agent that writes between Orca's read and Orca's write has its changes discarded. `runExclusivelyForAgentConfigFile` serializes Orca against itself only; it cannot see an external writer.

Raised by CodeRabbit as an outside-diff finding on #18084, where it applies to the new Claude arm. Filing separately because it predates that PR and affects every preset, so a fix for one arm would be an inconsistent mechanism.

## Affected writers

| writer | read | write |
|---|---|---|
| `markRemoteCodexProjectTrusted` | `remote-agent-trust-presets.ts:89` | `:99` |
| `markRemoteCopilotFolderTrusted` | `:133` | `:151` |
| `markRemoteClaudeProjectTrusted` | `:175` | `:195` |
| `markCopilotFolderTrusted` | `agent-trust-presets.ts:84` | `:108` |
| `markClaudeProjectTrusted` | `:183` | `:211` |

`markCursorWorkspaceTrusted` is exempt — it writes a standalone marker file rather than merging into shared config. `markCodexProjectTrusted` goes through `upsertProjectTrustLevel`, which still re-serializes the whole TOML.

## Why the existing guards don't cover it

- `runExclusivelyForAgentConfigFile` is an in-process lane keyed by path. It stops two Orca worktree creations from racing each other, which is what it was added for (#16441), and nothing else.
- `writeFileAtomically` makes the local write atomic, so no reader sees a torn file. It does not make the read-modify-write a transaction, so a lost update still happens.
- Nothing at all guards the remote arms; `IFilesystemProvider.writeFile` is a plain overwrite.

## Impact

Worst case is on `.claude.json`, which is large and hot: every running Claude session rewrites it on each turn (`lastCost`, `lastDuration`, session ids, per-project counters), and it also holds `oauthAccount` and MCP server config. Orca reads it, the agent writes, Orca writes its stale snapshot back, and the agent's turn bookkeeping is gone. Copilot's `config.json` holds `copilotTokens` and has the same shape of exposure.

The timing is narrow but not negligible: preflight runs before Orca spawns the agent *for that workspace*, so the collision is with a **different** session on the same host — which is the normal state for anyone running more than one agent.

## Reproduction

1. Have a Claude session running and active on the host (so it is writing `.claude.json` each turn).
2. Create a new Orca workspace with `claude` as the agent, so `markClaudeProjectTrusted` runs.
3. If the agent's write lands between the preset's `readFileSync` and its `writeFileAtomically`, that write is lost. Same on the remote arms via the SSH filesystem provider.

The window is milliseconds, so this is easier to demonstrate by instrumenting a delay between the read and the write than by racing it naturally.

## Suggested direction

**Correction to an earlier version of this issue,** which said every remedy needs an `IFilesystemProvider` contract change. That was wrong. `FileStat` already exposes `size`, `mtime`, `mtimeMs` and `ino`, on the local provider and the SSH one alike, so *detecting* a concurrent write needs no new contract. Only a genuinely atomic compare-and-swap would — and that is not reachable here at all, for the reason below.

**Locking is not on the table.** `proper-lockfile` is already bundled, but a lock only helps when every writer takes it. The competing writer is the agent's own process, which does not, and cannot be made to. There is also no vendor API to delegate to: `claude project` exposes only `purge`. So detect-and-abort is the ceiling for the local case, not a stepping stone to something stronger.

Given that, in rough order of value:

- **Precondition and abort.** Capture `(size, mtimeMs, ino)` at read time, re-stat immediately before writing, abort if anything moved. Needs no contract change. Shrinks the window from "parse + merge + write" to "final stat → rename", which `writeFileAtomically` already makes atomic. **Abort rather than retry**: the preflight is best-effort by design — every call site swallows errors on the grounds that the user can still accept the trust dialog manually — so declining to write degrades to a visible prompt, while a retry loop only widens exposure for a write we are permitted to skip. Apply to all five writers at once; a single-writer version is the inconsistency both review bots objected to.
- **Host-side merge for the remote arms.** Today they read over SSH, merge on the desktop, and write back, so the window spans two network round trips — tens to hundreds of milliseconds rather than microseconds. Performing the merge on the host in one command collapses it to a local read-modify-write there. This is the largest single reduction available and it applies only to remote.
- **Surgical edit instead of parse-and-reserialize.** `markCodexProjectTrusted` already does this via `upsertProjectTrustLevelInContent`, splicing the TOML text so bytes outside the edited section survive verbatim. The Claude writers `JSON.parse` and re-`stringify`. Measured on a real 93 KB `.claude.json`: not byte-identical (about 156 bytes of formatting drift) but no integers beyond 2^53, so this is cosmetic rather than data loss. Lowest priority of the three; listed for completeness.

None of these makes the read-modify-write transactional. The realistic goal is that Orca never *destroys* a concurrent write — it either wins cleanly or declines and lets the dialog appear.

## Anything else that might help

Worth deciding alongside this: the preflight write is best-effort by design — every call site swallows errors on the grounds that the user can still accept the trust dialog manually. If that stays true, aborting on a detected concurrent write is clearly preferable to retrying into a loop, since the fallback is a visible prompt rather than a broken launch.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.