feat: remote connect onboarding (ADR 0007) — remote.sh + key.sh - #479
Draft
fujibee wants to merge 18 commits into
Draft
feat: remote connect onboarding (ADR 0007) — remote.sh + key.sh#479fujibee wants to merge 18 commits into
fujibee wants to merge 18 commits into
Conversation
- import: add --identity-stdin as the main path (positional secret arg warned as legacy) — B2, secret no longer exposed via argv/ps - generate/import: atomic identity-file writes (temp+fsync+rename, never truncates or follows a symlink at the destination) — B4 - generate/import: existence-check and epoch write now happen inside the same team-config lock, closing a concurrent-call race that could mint two unrelated epoch-0 keys for one team — B4 - rotate: descoped to NOT READY — the previous_snapshot_sha256 hash chain did not use the age-v1 profile's canonical epoch-snapshot shape and could not detect a wholesale config.json rollback; needs a durable anti-rollback anchor design before it ships — B7
…argv leaks, unsafe auto-generate default, force-revoke ordering, resumable pending commit, strict response validation)
…eak, URL-parse HTTPS bypasses, post-commit idempotency, force+omitted-team gap, UUIDv7 validation)
…rphaned-revoke recovery, credential-file atomicity, duplicate-key/unknown-field rejection, docs)
…S, strict 200-only revoke, JSON-serializer credential write)
…bearing temp files (nonblocking follow-up)
…ype templates SKILL.md documented /agmsg remote ... and /agmsg key ... as routed the same way as every other command, but none of the per-type scripts/drivers/types/*/template.md Execute dispatch sections actually had a branch for them — a user on the normal skill-install path running these slash commands would get no routing to scripts/remote.sh or scripts/key.sh at all. Adds a consistent dispatch block (stdin-first for both the token and the identity, never a literal argument) to all nine per-type templates.
… path for key import
…r bug) The sqlite3 shell's dot-command tokenizer does not honour SQL '' escaping, so .param set :json '<escaped>' silently mis-parses as soon as a team's config already contains a single quote anywhere (confirmed directly: .param set :x 'a''b' errors instead of binding a'b). Splice the escaped JSON blob as a genuine SQL string literal in the actual SELECT statement instead, which does honour '' escaping — same fix as PR #482 applied to the sibling registry scripts.
…ndum)
koit-approved OSS interface addition for cloud/self-hosted crash recovery:
a driver's child 'connect' invocation may die between a successful
server-side exchange and the local commit finishing, leaving it unable to
tell whether that operation actually landed.
- 'remote status --json' emits a strict, secret-free object per team
({local_team, endpoint, server_instance_id, remote_team_id,
credential_id, state: active|disconnected}) a driver correlates against
its own operation-status record.
- 'remote pending list [--json]' / 'remote pending abort <pending_id>'
enumerate and clean up an orphaned exchange that never reached a local
commit. pending_id is the existing sha256(endpoint, token) digest
already used as the pending record's filename — content-derived and
opaque, so no separate generation counter is needed for ABA protection
(reusing an id requires the identical (endpoint, token) retry). list
always reports pending_id/endpoint even for a record whose content
fails strict validation (a legacy/corrupt record), so crash recovery can
still see and abort it; the raw credential is never read into the
listing path at all.
Building this surfaced the same #87-class '.param set' tokenizer bug
already fixed in key.sh (this commit) and PR #482 (registry scripts):
_remote_read_config_field, _remote_local_disconnect, and _remote_commit
all bound the config JSON the same broken way. Fixed identically —
splice the escaped blob as a genuine SQL string literal instead of
binding it via .param set.
Tests: 17 new cases in test_remote.bats covering both JSON schemas,
never-connected/disconnected states, corrupt/legacy pending records, and
a quote-containing team name exercising the .param set fix end to end.
Full test_key.bats + test_remote.bats suite green (73/73).
[P1] pending list read the raw credential into a 0600 temp file and a shell variable purely to discard it (the metadata-only contract's listing output never showed it, but the copies still existed). Added parse-exchange-response.py --metadata-only, which does the same full validation but never emits the credential field at all; the pending-list path now uses it exclusively. [P1] pending abort and connect's resume path had no shared lock, so abort could delete a pending record and report success while connect (already holding its own copy from before the delete) went on to commit a real binding for that same operation — 'aborted' and 'an active binding exists for it' could both become true. Added a per-pending-id lock (_remote_pending_lock_acquire, a dedicated empty dir under PENDING_DIR/.locks/ so it's invisible to pending list's *.json glob) that both cmd_connect's resume path and cmd_pending_abort now claim for the whole span from 'does this record still exist' through to a successful commit or giving up — mutually exclusive, and scoped per pending_id so unrelated operations never serialize against each other. [P1] status --json assembled its 6 fields from 6 independent config.json reads; a concurrent disconnect/reconnect/force-rebind's atomic rename between any two of them could produce a mixed old/new-binding object that never existed as any actual on-disk state — a real defect for a strict ABI a driver correlates fields against (the human-text status path has the same shape but is left as is; it's read by a person, not parsed as a spec). Now reads config.json exactly once under the team lock and derives all 6 fields from that one snapshot via a single python parse. Tests: 2 new deterministic barrier tests (pre-acquire the shared lock, assert the other side blocks and changes nothing, rather than trying to race real concurrent processes). Full test_key.bats + test_remote.bats green (75/75).
The per-pending-id lock added in the previous commit used a plain mkdir-based lock with no owner/liveness tracking at all. If the process holding it died via SIGKILL/OOM/an OS crash — exactly the scenario the whole pending/abort feature exists to help recover from — the lock directory would never be cleaned up, permanently blocking both resume and abort of that exact pending record forever. Replaced with the existing owner_pid-tracked runtime lock (agmsg_runtime_lock_acquire/_verify/_release, lib/storage.sh) — the same primitive and stale-reclaim pattern the codex dispatcher lock already uses (codex-bridge-launcher.sh's acquire_dispatcher_lock): a dead owner_pid (kill -0 fails) is atomically replaced via compare-and-swap instead of blocking forever. No release-on-exit trap is needed for correctness — a crash just leaves a reclaimable dead-owner row for the next attempt; the explicit release added at connect's and abort's normal exit points is purely a promptness optimization. Accepts the same bare-PID reuse risk window that primitive's existing caller already does, matching established precedent rather than inventing a platform-fragile PID+start-time scheme. Tests: the 2 existing barrier tests now pre-insert a runtime-lock row (owned by the test's own live pid) instead of pre-mkdir'ing a lock dir, proving a live owner still blocks. Added a third barrier test that pre-inserts a row owned by a spawned-then-reaped (guaranteed dead) pid and asserts abort reclaims it and succeeds, rather than blocking until timeout — the specific crash-recovery invariant co1 asked to see covered. Full test_key.bats + test_remote.bats green (76/76).
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.
Summary
Implements ADR 0007 (
docs/adr/0007-remote-connect-onboarding-ux.md, included in this PR): team-scoped cloud/self-hosted sync connection (scripts/remote.sh) and onboarding-timeage-v1key management (scripts/key.sh).remote.sh connect|status|disconnect|doctor— connects a local team to a sync endpoint via a single-use exchange token, with--token-stdinas the required path for any programmatic/agent caller (bare positional token kept only as a warned legacy form for a human's own terminal).key.sh generate|show|import— first-epochage-v1key management for a team (single-writer onboarding only).Explicitly NOT in scope for this PR
key rotate— refuses unconditionally, changes no state. Itsprevious_snapshot_sha256design doesn't use the age-v1 profile's pinned canonical epoch-snapshot shape and can't detect a wholesaleconfig.jsonrollback; needs a durable anti-rollback anchor design before it ships.key request/key approve(device-pairing key delivery) — not implemented at all. An adversarial design review found five separate fundamental problems (grinding-attack-vulnerable confirmation code, one-directional auth allowing fake-key injection, missing request/delivery state machine, incompatibility withage-v1's policy gate, undefined broadcast-stream behavior). Deferred to a follow-up design pass.connectonly ever receives one.Note on §8/B3 (encryption-bootstrap default choice)
The originally-approved design let
connectauto-select "generate" as the default when a team's stream looked empty (based on local message count or servercurrent_seq). An adversarial review found this unsafe — neither signal can prove "I am the first writer" (two devices can see an empty stream simultaneously; a malicious/equivocating server can fake it deliberately) — so the shipped behavior never auto-selects generate; it always requires an explicit, deliberateg, defaulting to abort on empty/EOF input. koit approved this default-removal, conditional on the first-time-user wording being unambiguous at a glance — that wording is implemented (the empty-stream hint now states the recommendation directly before the caveat).Addendum: status --json + pending list/abort (koit-approved OSS interface addition)
Two more commits add a strict, secret-free ABI for cloud/self-hosted crash recovery (a driver's child
connectinvocation may die between a successful server-side exchange and the local commit finishing):remote.sh status [<team>] [--json]—{local_team, endpoint, server_instance_id, remote_team_id, credential_id, state: active|disconnected}.remote.sh pending list [--json]/remote.sh pending abort <pending_id>— enumerate and clean up an orphaned exchange that never reached a local commit.pending_idis the existing content-derived sha256(endpoint, token) digest already used as the pending record's filename;liststill reportspending_id/endpointfor a record whose content fails strict validation (a legacy/corrupt record), so crash recovery can still abort it by id.Building this surfaced the same
.param set-vs-tokenizer bug PR #482 fixed in the sibling registry scripts, present inremote.sh/key.shtoo — fixed identically in the first of the two addendum commits.Security review
Went through an initial 7-finding adversarial review (B1-B7) plus three delta re-review rounds (R1-R5, D1-D5, E1-E3) — secret-argv leakage via curl/python3, transport/response validation (HTTPS enforcement bypassable via URL parsing tricks, strict exchange-response schema incl. duplicate-JSON-key detection and UUIDv7 validation per the pinned server spec),
--forcerebind transactionality (CAS races, orphaned-credential recovery), and credential-file write atomicity. Cleared with no blocking findings as of the current head.Test plan
tests/test_key.bats— 19 tests, run against realage/age-keygentests/test_remote.bats— 54 tests (16 new for the addendum), run against a local mock pairing-exchange/revoke HTTP server (tests/helpers/mock_remote_server.py, test-only)ps-sampling verification that no secret ever appears in any process's argv duringconnectDependencies
Base:
main(co2's server-side integration branch isn't set up yet — will rebase ontointegration/remoteonce that lands, per coordination with co2).