feat(env): cross-device env-profile exchange (ENV-SYNC-01)#131
Merged
Conversation
Captured and provider-bound env profiles now sync fleet-wide: a new env.profile.updated event (mustVerify, enc.v2-sealed) carries the profile metadata + age_blob ref over the existing blob plane; apply is LWW by source-event coordinate with soft-skip on unknown/tombstoned projects so a stale env pointer can never abort a pull batch; capture/bind emit in the same transaction as the row upsert (P6-DATA-03). - Migration 00023: env_profiles source-event coordinates. - Store: Tx.UpsertEnvProfileTx (shared transactional core), source-coord reader, EnvProfilesForBlobRef. - Revoke rewrap inversion (P5-SEC-04 rationale retired): env blobs rewrap like draft blobs — superseding env.profile.updated first, then hub cleanup; catch-all UpdateBlobRef keeps tombstoned-entry bindings pointed. - Snapshot plane: SnapshotEnv pointer per entry, merged on import by its OWN coordinate (a losing entry row can still deliver a winning env pointer); recovery pulls imported env blobs alongside draft blobs — env profiles survive event-log compaction and snapshot bootstrap. - e2e: env_exchange.txtar (two devices, capture on A, hydrate on B). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds cross-device environment-profile synchronization: a new ChangesEnv Profile Sync
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as env capture/bind
participant State as internal/state
participant Sync as internal/sync
participant Hub as hub
CLI->>State: WithTx insert env profile event + UpsertEnvProfileTx
CLI->>Hub: sync encrypted blob and event
Hub->>Sync: deliver env.profile.updated
Sync->>State: applyEventTx / ReplayPendingEnvProfileConflicts
Sync->>CLI: hydrate cached blob into .env.local
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…event rewrap ordering Codex review on ENV-SYNC-01 surfaced two correctness gaps: - A verified env.profile.updated whose project had not applied yet was silently consumed (cursor advanced, profile lost even after the project later replayed). Apply now tombstone-drops only on a winning delete; an absent path without a tombstone quarantines as a replayable env_pending_project conflict, and ReplayPendingEnvProfileConflicts (after every pull apply + after devices-approve replay) recovers it once the project lands. - rewrapHubCleanup pushed the superseding event before the rewrapped blob was durable, so a peer could apply the event and fail to fetch the ciphertext it names. Blob now uploads first; a failed event push self-heals on the next sync. Also fixes the pre-existing draft-rewrap ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Dual-review record. Coordinator line-by-line review applied two fixes pre-push (tombstoned-binding catch-all
Gates re-run on the final tree: gofmt clean, darwin+linux builds, golangci-lint 0 issues, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/blob_gc.go (1)
55-68: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDon't abort the revoke rewrap pass on the first env error.
rewrapBlobsOnRevokereturns immediately when anyrewrapEnvBlobcall fails, so a transient failure inemitSupersedingEnvProfileorUpdateBlobRefcan stop the draft loop entirely and leave draft blobs unrewrapped under the revoked key. Continue through both loops, collect the first error, and return it after processing everything.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/blob_gc.go` around lines 55 - 68, The revoke rewrap pass in rewrapBlobsOnRevoke currently stops at the first failure from rewrapEnvBlob, which can leave later draft and env blobs unprocessed. Update the EnvBlobRefs loop (and keep the draft loop behavior aligned) so failures from emitSupersedingEnvProfile or UpdateBlobRef are recorded but do not return immediately; continue processing all refs, track the first error encountered, and return that error after all rewrap attempts finish.
🧹 Nitpick comments (3)
internal/cli/env.go (1)
79-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated payload/params construction.
captureEnvProfile(Lines 82-99) andnewEnvBindCommand(Lines 383-398) both build an almost-identicaldssync.EnvProfilePayloadandstate.EnvProfileParamspair from the same fields (Name/Profile, Provider, Mode, BlobRef/VarNames or Refs). The same pattern is repeated a third time inemitSupersedingEnvProfile(internal/cli/blob_gc.go, Lines 234-260). Any future field addition/rename risks the wire payload and persisted params drifting out of sync since they're maintained by hand in three call sites.Consider a small helper (e.g.
newEnvProfileEventAndParams(...) (dssync.EnvProfilePayload, state.EnvProfileParams)) that derives both from one input, and have all three sites call it.Also applies to: 380-406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/env.go` around lines 79 - 107, `captureEnvProfile`, `newEnvBindCommand`, and `emitSupersedingEnvProfile` duplicate construction of matching `dssync.EnvProfilePayload` and `state.EnvProfileParams`, which can drift over time. Refactor the shared field mapping into a small helper such as `newEnvProfileEventAndParams(...)` that derives both objects from one input source, and update each call site to use it so payload and persisted params stay in sync.internal/sync/snapshot_import_test.go (1)
317-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid LWW coverage; consider adding a provider-profile (Refs) snapshot case.
Both new tests only exercise the
devstrap_encryptedshape (VarNames/BlobRef). A round-trip/LWW test using the provider-ref shape (Refsmap,runtime_onlymode) would close the coverage gap forimportEnvTx/UpsertEnvProfileTx's other branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sync/snapshot_import_test.go` around lines 317 - 445, Add a snapshot round-trip/LWW test for the provider-ref env profile shape in addition to the existing devstrap_encrypted coverage. Extend the snapshot import tests around TestSnapshotRoundTripsEnvProfile and TestImportSnapshotEnvLWW to build an Env profile using Refs with runtime_only mode, then verify BuildSnapshot and ImportSnapshot preserve it and that the env pointer LWW behavior still works through importEnvTx and UpsertEnvProfileTx. Ensure the new case asserts the refs survive import/export and idempotent re-import.internal/state/store.go (1)
1787-1816: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInconsistent "unsynced" definition vs.
snapshotEnvForProject.
EnvProfileSourceCoordstreats a profile as having no source coordinates only when all three ofsource_event_hlc/device_id/idare NULL/empty (AND):if !nHLC.Valid && (!nDeviceID.Valid || nDeviceID.String == "") && (!nEventID.Valid || nEventID.String == "") { return 0, "", "", false, nil }
internal/state/snapshot_reads.go'ssnapshotEnvForProjecttreats it as unsynced when any one of the three is missing/empty (OR):if !nHLC.Valid || !nDev.Valid || nDev.String == "" || !nEvt.Valid || nEvt.String == "" { return nil, nil }
UpsertEnvProfileTxstamps all three columns together only whenevent.HLC != 0 || event.DeviceID != "" || event.ID != ""is true — so a partially-populatedEvent(e.g.HLC=0butDeviceID/IDset) would produce non-NULL-but-empty/zero values in one or two columns. In that scenario,EnvProfileSourceCoordswould reportok=true(using the zero/empty value in the LWW compare), whilesnapshotEnvForProjectwould treat the same row as never-synced and drop the env pointer from snapshots. Since both are meant to express the identical "has real source-event coordinates" concept for the same LWW/snapshot exchange, this divergence is a latent correctness risk if a caller ever passes a partially-stampedEvent(current callers pass either a fully-stamped event orEvent{}, so it isn't triggered today, but it's a fragile invariant).Consider extracting a single shared predicate (e.g.
hasSourceCoords(hlc sql.NullInt64, dev, evt sql.NullString) bool) and using it in both places.♻️ Suggested consolidation
+func hasSourceCoords(hlc sql.NullInt64, dev, evt sql.NullString) bool { + return hlc.Valid && dev.Valid && dev.String != "" && evt.Valid && evt.String != "" +} + func (tx *Tx) EnvProfileSourceCoords(ctx context.Context, namespaceID string) (hlc int64, deviceID, eventID string, ok bool, err error) { ... - if !nHLC.Valid && (!nDeviceID.Valid || nDeviceID.String == "") && (!nEventID.Valid || nEventID.String == "") { + if !hasSourceCoords(nHLC, nDeviceID, nEventID) { return 0, "", "", false, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/state/store.go` around lines 1787 - 1816, `EnvProfileSourceCoords` and `snapshotEnvForProject` use different rules for deciding whether an env profile has valid source coordinates, which can make the same row be treated as synced in one path and unsynced in the other. Align the logic in `EnvProfileSourceCoords`, `snapshotEnvForProject`, and the `UpsertEnvProfileTx` stamping flow by introducing one shared helper/predicate for “has real source-event coordinates” and using it everywhere. Make the helper reflect the intended invariant for `source_event_hlc`, `source_event_device_id`, and `source_event_id`, so both read and snapshot paths make the same decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/00_START_HERE.md`:
- Line 159: The shipped list in the START_HERE overview is overstating the
feature set by including production remote device registration. Update the
bullet to match the later env/security docs by removing remote registration and
keeping only the fingerprint-confirmation/manual-approval path, while preserving
references to the local trust plane, synced encrypted env/draft bundle exchange,
and device-revoke rewrap if they are still shipped.
In `@spec/16_TEST_PLAN.md`:
- Around line 279-287: Narrow the “sync” test-plan bullet in Env profile sync so
it only claims the behavior already covered by the implementation and work log.
Update the wording in the relevant test-plan entry to keep the unknown-project
path limited to the soft-skip behavior, and remove any implication that
batch-abort handling is included unless you split it into a separate bullet tied
to the sync flow. Refer to the existing env.profile.updated and unknown-project
cases when rewriting the scope.
---
Outside diff comments:
In `@internal/cli/blob_gc.go`:
- Around line 55-68: The revoke rewrap pass in rewrapBlobsOnRevoke currently
stops at the first failure from rewrapEnvBlob, which can leave later draft and
env blobs unprocessed. Update the EnvBlobRefs loop (and keep the draft loop
behavior aligned) so failures from emitSupersedingEnvProfile or UpdateBlobRef
are recorded but do not return immediately; continue processing all refs, track
the first error encountered, and return that error after all rewrap attempts
finish.
---
Nitpick comments:
In `@internal/cli/env.go`:
- Around line 79-107: `captureEnvProfile`, `newEnvBindCommand`, and
`emitSupersedingEnvProfile` duplicate construction of matching
`dssync.EnvProfilePayload` and `state.EnvProfileParams`, which can drift over
time. Refactor the shared field mapping into a small helper such as
`newEnvProfileEventAndParams(...)` that derives both objects from one input
source, and update each call site to use it so payload and persisted params stay
in sync.
In `@internal/state/store.go`:
- Around line 1787-1816: `EnvProfileSourceCoords` and `snapshotEnvForProject`
use different rules for deciding whether an env profile has valid source
coordinates, which can make the same row be treated as synced in one path and
unsynced in the other. Align the logic in `EnvProfileSourceCoords`,
`snapshotEnvForProject`, and the `UpsertEnvProfileTx` stamping flow by
introducing one shared helper/predicate for “has real source-event coordinates”
and using it everywhere. Make the helper reflect the intended invariant for
`source_event_hlc`, `source_event_device_id`, and `source_event_id`, so both
read and snapshot paths make the same decision.
In `@internal/sync/snapshot_import_test.go`:
- Around line 317-445: Add a snapshot round-trip/LWW test for the provider-ref
env profile shape in addition to the existing devstrap_encrypted coverage.
Extend the snapshot import tests around TestSnapshotRoundTripsEnvProfile and
TestImportSnapshotEnvLWW to build an Env profile using Refs with runtime_only
mode, then verify BuildSnapshot and ImportSnapshot preserve it and that the env
pointer LWW behavior still works through importEnvTx and UpsertEnvProfileTx.
Ensure the new case asserts the refs survive import/export and idempotent
re-import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d627da1-2fff-45f2-9e0a-43bb500f0445
📒 Files selected for processing (29)
cmd/devstrap/testdata/script/env_exchange.txtarinternal/cli/blob_gc.gointernal/cli/blob_gc_test.gointernal/cli/devices.gointernal/cli/env.gointernal/cli/env_test.gointernal/cli/root_test.gointernal/cli/snapshot_recovery.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/state/migrations/00023_env_profile_source_events.sqlinternal/state/snapshot_reads.gointernal/state/store.gointernal/state/store_test.gointernal/sync/apply_test.gointernal/sync/events.gointernal/sync/snapshot.gointernal/sync/snapshot_build.gointernal/sync/snapshot_import.gointernal/sync/snapshot_import_test.gospec/00_START_HERE.mdspec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/09_SECRETS_AND_ENVIRONMENT.mdspec/12_DATA_MODEL_SQLITE.mdspec/13_CLI_DAEMON_API.mdspec/14_MVP_ROADMAP_AND_BACKLOG.mdspec/15_SECURITY_THREAT_MODEL.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/blob_gc_test.go (1)
118-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cleanup step on the success path.
This event-bearing case already checksPutBlob -> Push; addinghub.Deletes == 1here would also lock in the final cleanup order for this scenario.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/blob_gc_test.go` around lines 118 - 137, The event-bearing cleanup test only asserts that rewrapHubCleanup calls PutBlob before Push and keeps old ciphertext on push failure, but it does not verify the success-path cleanup. Update TestRewrapHubCleanupUploadsBlobBeforeEvent to also assert that the successful run on faultHub performs exactly one DeleteBlob after the push, using the existing rewrapHubCleanup and faultHub symbols to lock in the final cleanup order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/blob_gc_test.go`:
- Around line 118-137: The event-bearing cleanup test only asserts that
rewrapHubCleanup calls PutBlob before Push and keeps old ciphertext on push
failure, but it does not verify the success-path cleanup. Update
TestRewrapHubCleanupUploadsBlobBeforeEvent to also assert that the successful
run on faultHub performs exactly one DeleteBlob after the push, using the
existing rewrapHubCleanup and faultHub symbols to lock in the final cleanup
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c374ce9b-89e5-45b1-9ed1-c07f5fbf6b00
📒 Files selected for processing (9)
internal/cli/blob_gc.gointernal/cli/blob_gc_test.gointernal/cli/devices.gointernal/cli/sync.gointernal/sync/apply_test.gointernal/sync/events.gospec/09_SECRETS_AND_ENVIRONMENT.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
✅ Files skipped from review due to trivial changes (2)
- spec/16_TEST_PLAN.md
- spec/18_WORK_LOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/blob_gc.go
- spec/09_SECRETS_AND_ENVIRONMENT.md
…cal (CodeRabbit) The bullet lists what is NOT implemented; out-of-band fingerprint confirmation shipped with P4-SEC-04 and belongs in the shipped list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 6, 2026
Closed
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
Closes the last killer-loop gap in spec/00: env profiles captured on one device now hydrate on every other device.
envRecipientsalready encrypted captured values to all approved devices' age recipients — this PR adds the missing carrier event and synced metadata.env.profile.updated(single type for captured + provider-bound profiles), added tomustVerifyEvent— env payloads feed hydrated files and lifecycle-script environments, so they are trust-affecting.env_profilessource-event coordinates; apply is LWW by the highest(HLC, deviceID, eventID)(same ordering asreconcileSamePath), idempotent on replays; soft-skips an unknown/tombstoned project so a stale env pointer can never abort a pull batch (deliberately unlike the draft handler — that batch-abort is being filed as a follow-up finding).env capture/env bindinsert the event and upsert the rows in one transaction via the newTx.UpsertEnvProfileTx;env rotatere-captures therefore propagate automatically.blobRefFromEventrecognizes env payloads — push/pull, content-hash verification, andmaterialize's EAGER-03 auto-hydrate work unchanged. Hydrate-before-blob-arrival errors now carry arun 'devstrap sync'remedy.env.profile.updatedemitted before hub cleanup, plus a catch-allUpdateBlobRefso bindings on tombstoned entries repoint too.SnapshotEnvpointer per entry, merged on import by its own coordinate — a losing entry row can still deliver a winning env pointer — andrecoverFromSnapshotpulls imported env blobs alongside draft blobs. Env profiles survive event-log compaction and snapshot bootstrap.Flips the
ENV-SYNC-01backlog row (spec/14) to shipped; spec/00/07/09/12/13/15/16 reconciled.Tests
TestUpsertEnvProfileTxLWWCoordsStamped,TestEnvProfilesForBlobRef, legacy-wrapper NULL-coord coverage, schema-version bumps (22→23).TestSnapshotRoundTripsEnvProfile;TestImportSnapshotEnvLWW(older pointer never regresses; newer pointer wins on a losing entry).blobRefFromEventmatrix,TestRewrapEnvBlobEmitsSupersedingProfileEvent.env_exchange.txtar— two homes, mutual fingerprint enrollment, capture on A →sync→ hydrate on B byte-identical, no conflicts.Validation
gofmtclean ·go build(darwin+linux) ·golangci-lint run0 issues (after cache clean for the known sibling-worktree gosec ghosts) ·go test -race ./...all ok ·spec-drift --base origin/mainpasses (29 files).Implementation: gpt-5.5 (Codex) from a line-level spec; coordinator line-by-line review applied two fixes (tombstoned-binding repoint, unreachable-branch cleanup) and added the snapshot slice. Codex dual review to follow in-thread.
🤖 Generated with Claude Code
Summary by CodeRabbit
env capture/binding now emits environment profile updates so other devices can hydrate consistently.devstrap syncwhen synced encrypted data is missing.