feat(cli): orca push and orca pull - #35
Open
yi-here wants to merge 9 commits into
Open
Conversation
Move a run between this machine and an OrcaRouter gateway. The gateway records what a local `orca record` cannot see — every key, every colleague, every CI job, and the routing decision behind each call — while the local recorder sees the shell, the filesystem and MCP. Both already write orca-trace v0, so the only thing missing was the transfer. orca push [run] POST /api/replay/runs (multipart, field `file`) orca pull <run> GET /api/replay/runs/:k/export Neither command converts, re-orders or re-serialises anything. The gateway recomputes the integrity root over events.jsonl and refuses a push whose root does not match its manifest, so the only correct thing to send is the bytes already on disk: push reads files and zips them, pull unzips and writes them. `archive.test.ts` pins that with a case a re-serialiser would fail (CRLF, key order, a blank line) — an archiver that produced equivalent JSON would look right locally and be rejected on arrival. Written against the rules of the road: - NO NEW RUNTIME DEPENDENCIES. The zip subset a trace needs is hand-rolled over zlib, a builtin; the multipart body likewise. CRC-32 is computed here rather than taken from zlib.crc32, which landed in node 20.15 — the published CLI supports node 20.0, so using it would break the floor package.json advertises on a patch release nobody would think to test. - THE REDACTOR IS UPSTREAM OF THIS SINK, BY CONSTRUCTION. push is a new sink, and it deliberately does not re-run redaction: TraceWriter puts every event through "spill, redact, validate, append" on the way to disk precisely so no caller can bypass it, so the bytes push reads are already redacted — and re-scrubbing them would rewrite bytes the integrity root covers, which is the one thing that makes an archive unpushable. The gateway's own scan is the second, independent line, which is why --force exists and is opt-in. - SECRETS NEVER REACH A TTY. The key travels in headers only — never the URL, never a form field, never a log line — and a test asserts all four, including over every emitted log entry. A push with no key is REFUSED rather than attempted anonymously: an unauthenticated POST is exactly what a misconfigured public endpoint accepts, and the user would learn their run went somewhere unowned from a 200. - NO DEFAULT DESTINATION, unlike model traffic. `resolveUpstream` may fall back to OrcaRouter because proxying a call the agent was already making is not a disclosure; sending a recording of someone's source code to a host they never named is. A run holds source, shell output and workspace snapshots, so push.packed reports the file count and size before the request goes out. Tests: an in-process HTTP server rather than a stubbed fetch, because what these commands get wrong is the wire — the multipart framing, which header carries the key, what a 4xx body means — and a stub only asserts that the code calls the function the test already believes it calls. Each test was verified red for the reason expected, and one of them exists because a probe found it was not: hard-coding the zip base offset to zero passed the whole suite, so that arithmetic was carrying no weight. It now has a prepended-bytes case. Untested cleverness is worse than none, because it reads as deliberate. npm run check, conformance.mjs and check-neutrality.mjs all pass. The Python SDK job was not run — this change does not touch it. Signed-off-by: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
![orcacode-review[bot]](https://avatars.githubusercontent.com/in/4254738?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 212 calls · 8M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
…destroying Two findings from review on the push/pull PR, both real. THE CONFIGURED KEY WENT TO WHATEVER --gateway NAMED. resolveGateway merged the override URL into config.gateway and asked gatewayHeaders for the result, which keeps api_key/api_key_env while swapping the origin: `orca push --gateway https://somewhere-else` sent the credential for the user's real gateway to that host, with a recording of their source attached. upstreamPlan already carries a long note about this exact defect for model traffic, and its resolution is the right one here too — the stored key applies only when the resolved origin IS the configured one. So sameOrigin moves into config.ts and both credential gates share one comparator, rather than two that can drift apart. A key passed in the environment alongside the override is a deliberate pairing by the person running the command and still goes where they pointed it. `pull --force` DESTROYED THE LOCAL RUN BEFORE IT COULD REPLACE IT. rm(dest) followed by a per-file write leaves anything that fails in between — a full disk, ^C, an archive naming both `x` and `x/y` — with the run neither the old one nor the new one. The old comment argued a partial write is recoverable by pulling again; that is true only if the gateway still has it, and the recording may be the only copy of a crash someone spent a day reproducing. Now staged in a sibling directory and swapped in by rename, with the original moved aside rather than deleted and put back if the swap itself fails. Neither scratch name matches RUN_ID_PATTERN, so litter from a crash mid-swap is invisible to `orca list` instead of presenting itself as a run. Both tests were written first and confirmed red for the right reason: the push resolved (key sent) rather than refusing, and the local events.jsonl had already become the partial replacement. A third test — pointing --gateway at the gateway you already configured still uses the stored key — keeps the first fix from being satisfied by refusing everything. Full suite: 94 files, 1627 passed. typecheck and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 180 calls · 7.7M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
…a throw CI red on the previous commit: `prettier --check .` rejected sync.test.ts. My fault and my process gap — I ran lint and typecheck before pushing but not fmt:check, which is a separate step in the workflow. The review finding in the same file is the substantive half, and it is right. The swap moves the old run aside and the new one into place: two renames with a gap in which `dest` does not exist. try/catch covers a thrown error and nothing else — SIGKILL, ^C and power loss all land in that gap — and my comment claimed the state could not happen. Because the scratch names were RANDOMISED and neither matches RUN_ID_PATTERN, list, show, gc, scrub and resolveRunSelector all skipped them: the run had not failed to update, it had vanished, with a possibly secret-bearing copy stranded where scrub could not reach it and nothing that would ever sweep it. The exception path was defeated under concurrency too — a raced recreation of `dest` made the revert fail into a swallowed catch. Fixed the way the reviewer describes. Scratch names are now deterministic per destination (`<run>.incoming`, `<run>.replaced`), which is what turns the half-done state from invisible into recoverable, and recoverInterruptedSwap finishes or rolls it back: new copy present and dest missing, finish the swap; only the old copy present, put it back; dest present, sweep the litter. Every branch renames before it removes, so no state deletes the only copy. The move-aside also comes inside the try now, so a raced recreation removes staging and rethrows instead of stranding it. Determinism costs one thing — two concurrent pulls of the same run share the scratch names — and buys back a run no command can reach. Those two pulls already raced over `dest` itself; a race is recoverable, a disappearance is not. Recovery runs BEFORE the fetch as well as after it, and the test I wrote for the rollback case is what found that: keyed only on the archive's run id, a pull that fails early (unreachable gateway, 404, empty archive) left a half-swapped store half-swapped, and no other command would ever reclaim it. The pre-fetch pass is keyed on the CLI selector when that is itself a run id, which is the common case — you pull the run you just failed to pull. Three tests stage the on-disk state a killed process leaves behind, which is the only honest way to test it: the failure is the absence of any further code running, so there is no exception to inject. Known residual, stated rather than implied: `orca list` does not self-heal. A user who crashes mid-swap and never pulls that run again still sees nothing. Recovery in listRuns would make a read-only operation mutate the store and would race a concurrent pull, so it belongs in packages/core with its own design, not bolted on here. fmt:check, lint, typecheck all clean; full suite 94 files, 1630 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 197 calls · 9M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
A README section for the two new commands, and a card generated from a real
round trip rather than written by hand.
The card follows the rule render-demo.mjs states for the hero animation and
states for a reason — "every line is real output ... no line is invented or
reworded", because a README that illustrates a debugger with output no code
produced is advertising the one failure the tool exists to prevent. So
scripts/render-sync-card.mjs reads docs/media/sync-transcript.txt, and that
transcript is a captured sequence: `orca record` against this repo's own fake
agent, `orca push`, `orca pull` into a SECOND empty workspace, then `orca show`
reading the pulled copy. The gateway is a local stub speaking the two real
endpoints and returning the uploaded zip verbatim on export, so the pull reads
exactly the bytes the push sent — which is why the address in the card is a
loopback one and is left as printed.
Two things I changed after looking at the rendered result rather than assuming:
- The card is sized to the output instead of the output being clipped to the
card. A fixed 960px width cut `orca show`'s DETAIL column mid-JSON, which
would have been an abbreviation of real output presented as a transcript.
- `orca show` is trimmed to 104 columns AT CAPTURE, because one Bash call in
this run carries a full JSON argument and auto-sizing alone produced a
3050px card that is unreadable at README scale. Same "trimmed for width and
nothing else" rule; done in the capture rather than the renderer so the file
on disk is exactly what the card shows, and recorded in docs/media/README.md
along with how to regenerate both.
The prose covers what the two halves each see (the gateway sees every key,
colleague and CI job; the local recorder sees the shell, filesystem and MCP),
what push needs (a replay-scoped key, uploads enabled per workspace), and four
things it deliberately will not do — no default destination, no stored key sent
to a host named on the command line, no anonymous push, no partial replace —
each with the reason rather than just the rule. It also states plainly that push
does not re-run redaction and why re-running it would break the integrity root
the gateway checks on arrival.
fmt:check, lint, typecheck clean; full suite 94 files, 1630 passed;
check-neutrality 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f
Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 3 issues in this PR: 🟠 3 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 198 calls · 8.8M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
record, attach and replay all create `.orca/runs` through ensureRunsDir. pull
wrote into it with a bare recursive mkdir and inherited none of what that
function exists for, which cost two things — both of them worst in exactly the
case pull is for, a fresh clone with no recording yet, where pull is the command
that CREATES the store:
- The 0700 directory and 0600 files SECURITY.md states as a promise ("Trace
files and blobs are written mode 0600, run directories 0700"). Under a
default umask the pull landed 0755/0644, so the gateway's copy of someone's
source, shell output and workspace snapshots was world-readable on a shared
machine. A pulled run is exactly as sensitive as a recorded one.
- `.orca/.gitignore` containing `*`, git's idiom for a directory that excludes
itself. Without it the whole store shows up in `git status` as untracked,
one `git add -A` from being committed and pushed — the accident ensureRunsDir
was written to prevent.
Fixed by calling ensureRunsDir before anything writes, and by passing the modes
core/src/writer.ts and core/src/blobs.ts use. The explicit chmod after each
create is not redundant: the process umask masks bits out of a creation mode, so
passing `mode` to mkdir/writeFile alone does not make the promise true.
The test asserts the modes actually on disk and the .gitignore's content, from an
EMPTY workspace rather than a seeded one, so it exercises the store-creating path
rather than inheriting a directory some earlier command made correctly. Verified
by reverting the fix and watching it fail.
fmt:check, lint, typecheck clean; full suite 94 files, 1631 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f
Signed-off-by: Claude <noreply@anthropic.com>
…nd a race that deleted both copies Both found by review on the fix I pushed an hour ago, and both are right. `.replaced` IS THE EVIDENCE THAT `.incoming` IS COMPLETE, and recoverInterrupted Swap did not know it. It promoted `.incoming` whenever `dest` was absent, on the reasoning that the interrupted pull "had already written every byte and was one rename from done". That holds for a REPLACE — staging finishes before `dest` is moved aside, so a replace crash always leaves BOTH siblings. It does not hold for a FIRST pull, which writes entries straight into `.incoming` and renames once at the end: a crash there leaves a PARTIAL `.incoming` and no `.replaced`. The recovery promoted it, so a truncated recording became the run — `show`, `scrub` and `push` would all treat it as whole — and the next pull, the one that would have fetched the good copy, was refused with "already exists locally" and a flag the user has no reason to reach for. The rule is now "only a copy something PROVES complete wins", not "newest wins": `.incoming` is promoted only alongside `.replaced`, and a lone `.incoming` with no `dest` is discarded so this pull refetches. The archive is still on the gateway; a partial installed as the truth is not recoverable at all. AND THE RACE I ARGUED WAS ACCEPTABLE PRODUCES THE DISAPPEARANCE I SAID IT COULD NOT. I made the scratch names deterministic to make a half-done swap recoverable, and wrote that the resulting sharing was a fair trade because "a race is recoverable, a disappearance is not". Interleave a recovery's "dest present, so these siblings are litter" cleanup with another pull's two renames and both copies go: the staged new one and the retired old one are deleted, and the rollback finds nothing to restore. Verifying siblings immediately before each removal does not fix that — it is the same check-then-act one instruction later. Exclusion is the fix, so there is now an O_EXCL lock file per run held across recovery, staging and the swap together; locking only the swap would leave recovery free to delete what staging just wrote. A lock older than ten minutes is broken and taken, because a lock file with no answer for the process that dies holding one makes the run permanently unpullable — a worse failure than the one being fixed. Two things the tests caught rather than my reading. Only EEXIST means "held": catching every error made a fresh workspace spin the whole retry budget and take six seconds to fail, because `.orca/runs` did not exist yet — the ordinary first-pull case, and visible only as the suite going from 1s to 19s. And ensureRunsDir has to run before the first lock, since a lock file needs its parent directory. Both fixes probed by reverting them: promoting any `.incoming` fails the partial test, and removing the lock check fails the concurrency test. fmt:check, lint, typecheck clean; full suite 94 files, 1633 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from 3ef0423 to 4746d19 during the review; these findings describe a diff the PR no longer has.
These findings describe3ef04230577c. Re-run the review to check the PR as it stands now.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 205 calls · 10M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 253 calls · 14.9M tokens · 98% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
…he stored one
I defended this asymmetry on the thread one round ago: the stored key is checked
against the configured gateway's origin, while a key from ORCA_GATEWAY_KEY was
sent wherever --gateway pointed, on the reasoning that an env key beside an
override is "a deliberate pairing made in one invocation".
Review answered it with the README from this same PR:
$ read -rs ORCA_GATEWAY_KEY && export ORCA_GATEWAY_KEY
An export PERSISTS. So the pairing was an assumption nothing enforced, and my own
documentation is what makes it false. A user who exports the key for their real
gateway and later runs `orca push --gateway <other-host>` — a typo, a stale
alias, a URL someone sent them — was attaching that key and the whole recording
(source, shell output, workspace snapshot) to the other host, while the identical
request with the key in config was refused. An asymmetry that turns on where a
credential is STORED rather than where it is GOING is not a security boundary.
Every credential now has a home — the origin it was configured for — and travels
only there:
stored key -> config.gateway.url
env key -> ORCA_GATEWAY_URL, or the configured gateway when that is unset
One case is deliberately left through: an env key with NO home, meaning nothing
but this invocation named a URL at all, so the flag and the key arrived together
and there is no earlier association for the flag to contradict. That is the CI
shape the env key exists for, and it has its own test so the rule cannot be
"tightened" into breaking it.
The README's rule text is corrected with the code — it stated the old asymmetry
as a feature — and now says plainly that both exports persist, which is why a
later push to another host in the same shell is refused.
Three tests: the exported-key-to-an-override refusal (verified red by reverting
the gate), the CI shape, and the ordinary exported pair with no flag.
fmt:check, lint, typecheck clean; full suite 94 files, 1636 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f
Signed-off-by: Claude <noreply@anthropic.com>
The example set ORCA_GATEWAY_URL to a `https://your-gateway` placeholder while the `orca setup` walkthrough forty lines earlier shows the real default (`https://api.orcarouter.ai`, which is `ORCAROUTER_URL` in the CLI). Same document, same variable, two different answers — and the section is the one about pushing runs to OrcaRouter, so the placeholder was the less useful of the two. `/api/replay/runs` and `/api/replay/runs/:key/export` are served by the same gateway that serves the model APIs, so the setup host is the push host; there is no second address to learn. Naming it does not give push a default destination, which is the promise the next section makes. That promise is about the CLI never filling in a host of its own, so the bullet now says so in those terms rather than leaning on the example being anonymous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from 565b0f5 to 27184c7 during the review; these findings describe a diff the PR no longer has.
These findings describe565b0f59e14d. Re-run the review to check the PR as it stands now.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 228 calls · 12.1M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 261 calls · 15.1M tokens · 98% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
writeArchive put entries.length straight into the EOCD's 16-bit count fields. The damage is graded, and the middle band is the dangerous one: 65535 entries the count IS the zip64 sentinel, so readArchive refuses it 65536 wraps to 0, so readArchive returns nothing 70000 wraps to 4464, so readArchive SUCCEEDS with 4464 of them In that last band `orca push` transmits a structurally corrupt archive and the matching `orca pull` installs a fraction of the run and reports success. Silent partial loss is precisely what this file's governing property — entry bytes round-trip EXACTLY — exists to rule out. A run directory holds one file per unique content-addressed blob, so a long session with heavy tool and file output reaches these counts without anyone trying. Refusing rather than emitting zip64, because readArchive refuses zip64 too: writing it would trade a silent truncation for a confident file this CLI cannot read back. A bound that says so is the honest answer until both halves learn the format. The 32-bit fields get the same treatment, which the review noted in passing: a per-entry compressed or original size at 4 GiB, a local-header offset past 4 GiB, and the trailer's own central-directory size and offset. Each is checked where it is about to be written, so the message names the entry that overflows rather than the total. Test written first and confirmed red for the right reason — writeArchive resolved instead of rejecting. It pins all three bands, and pins that 0xfffe entries still WRITE and read back complete: a bound that refuses valid input would be a different bug in the same place. `npm run check`: 94 files, 1637 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 216 calls · 11.1M tokens · 97% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| try { | ||
| return await fn(); | ||
| } finally { | ||
| await rm(lock, { force: true }).catch(() => undefined); |
There was a problem hiding this comment.
🟠 P1 Release the run lock only when it is still this process's, not unconditionally
withRunLock's finally does await rm(lock, {force:true}) with no ownership check. The acquisition loop breaks and takes a lock whose mtime is older than STALE_LOCK_MS (10 min). If holder P1 is merely slow or suspended (laptop sleep, SIGSTOP, VM pause) past 10 minutes while its fn is mid-critical-section, waiter P2 breaks the lock and re-acquires it. When P1 resumes and finishes, its finally deletes P2's lock. A third pull P3 then sees no lock, acquires, and runs recoverInterruptedSwap + staging + swap concurrently with P2, both using the SAME deterministic .incoming/.replaced scratch names. That is precisely the interleaving commit 4746d19 says the lock exists to exclude: one process's recovery ("dest present, so these siblings are litter") deletes the other's staged new copy and retired old one, and the rollback finds nothing to restore — the run disappears (data loss). The write side stores only the pid (writeFile writes ${process.pid}\n), so the fix is to make the finally read the lock and remove it only when its contents are this process's pid (or unlink-then-check); otherwise leave the successor's lock alone.
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
2 participants
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.
Orca-Code-Review — push 7
❌ 1 finding blocks merge
What this changes
Two new commands move a run between this machine and an OrcaRouter gateway:
pushdefaults to the last run, like every other run-taking command.pullrequires a run id, because "last" means nothing on a machine that has not seen it yet. Both take--gatewayand--force.Why
The gateway records what a local
orca recordcannot see — every key, every colleague, every CI job, and the routing decision behind each call — while the local recorder sees the shell, the filesystem and MCP. Both already writeorca-trace v0, so the two halves were already interoperable and the only thing missing was the transfer: pull a run a colleague's CI produced andorca showit, or push a local reproduction to where the rest of the team can read it.Today that round trip is a browser download and a form upload.
Tests
Written first, and each one verified red for the reason expected.
archive.test.tsfirst, because the format is the part that fails silently. The one that mattered most ispreserves bytes that a re-serialiser would change(CRLF, key order, a trailing blank line): the gateway recomputes the integrity root overevents.jsonland refuses a push whose root does not match its manifest, so an archiver that produced equivalent JSON would look correct locally and be rejected on arrival. Nothing in these commands parses an entry's content.One test exists because a probe proved it was needed. Hard-coding the zip base offset to zero passed the entire suite — the arithmetic that lets a prepended archive be read was carrying no weight at all. It now has a
reads an archive with bytes prepended to itcase, and that case fails againstbase = 0. Untested cleverness is worse than none, because it reads as deliberate.sync.test.tsstands up a real HTTP server rather than stubbingfetch. What these commands get wrong is the wire — the multipart framing, which header carries the key, what a 4xx body means — and a stub only asserts that the code calls the function the test already believes it calls. Probes confirmed the security assertions bite: putting the key in the URL fails three tests, and removing the no-key guard fails the one that exists for it.Not run: the Python SDK job. This change does not touch it.
Notes for review
Three decisions worth disagreeing with if you see them differently:
The redactor is upstream of this sink by construction, and push deliberately does not re-run it. CONTRIBUTING says a new sink goes through the redactor, and push is one.
TraceWriterputs every event through spill, redact, validate, append on the way to disk precisely so no caller can bypass it — so the bytes push reads are already redacted, and re-scrubbing them would rewrite bytes the integrity root covers, which is the one thing that makes an archive unpushable. The gateway's own scan on arrival is the second, independent line; that is what--forceoverrides, and why it is opt-in.No default destination, unlike model traffic.
resolveUpstreammay fall back to OrcaRouter because proxying a call the agent was already making is not a disclosure. Sending a recording of someone's source code to a host they never named is. A run holds source, shell output and workspace snapshots (as content-addressed blobs, so they travel with it), sopush.packedreports the file count and byte size before the request goes out rather than after.A push with no key is refused, not attempted anonymously. An unauthenticated POST is exactly what a misconfigured public endpoint accepts, and the user would learn their run went somewhere unowned from a
200.Checklist
npm run checkpasses locally — 94 files, 1624 passed, 1 skipped.conformance.mjs(38 events, 0 failures) andcheck-neutrality.mjsalso pass.zlib, a builtin. CRC-32 is computed here rather than taken fromzlib.crc32, which landed in node 20.15: the published CLI supports node 20.0, so using it would break the floorpackage.jsonadvertises, on a patch release nobody would think to test.git commit -s)🤖 Generated with Claude Code
https://claude.ai/code/session_01BgagqGMVfthG1Vyk9dN99f
Generated by Claude Code