feat(devices): synced device-trust propagation (TRUST-01)#132
Merged
Conversation
|
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds synced device-trust propagation: new ChangesDevice trust propagation (TRUST-01)
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sync/trust_apply_test.go (2)
176-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen local-target no-op assertion.
Line 189 only rejects
"revoked"/"lost", so a regression that instead flips local trust to some other unexpected value (e.g."pending") would slip through undetected. Since the local device's initial trust state is known/deterministic fromnewSyncStore, assert equality to the expected unchanged value instead of an exclusion check.Suggested tightening
- if got := deviceTrustState(t, device.ID); got == "revoked" || got == "lost" { - t.Fatalf("local device trust=%q, must never flip from a remote event", got) - } + if got := deviceTrustState(t, st, device.ID); got != "approved" { + t.Fatalf("local device trust=%q, want unchanged approved (must never flip from a remote event)", got) + }🤖 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/trust_apply_test.go` around lines 176 - 192, The test TestApplyDeviceTrustLocalTargetIsNoOp only excludes a couple of bad trust values, so it can miss other unintended changes. Update the assertion to compare deviceTrustState(t, st, device.ID) against the expected unchanged initial trust value from newSyncStore, using the existing identifiers ApplyEventsWithStats, deviceTrustState, and device.ID to keep the no-op check strict.
89-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest exercises cross-type stickiness, not literal event replay/idempotency.
The comment/test name ("replay") implies redelivery of the same event, but this test actually applies a different event (
lost, new IDevt_lost_b) against an already-revokedtarget — it verifies monotonic stickiness across event types, not idempotent handling of a duplicate/re-delivered event with the same ID/content. Per the stack context, this file is expected to cover "replay idempotency" for the trust events; consider adding a companion case that re-applies the exact samerevokeevent (same ID) to confirm the dedup/no-reflag path for genuine redelivery, distinct from this cross-type stickiness case.Suggested additional case
// Re-apply the identical revoke event (redelivery) — must not re-flag rotation. if _, err := ApplyEvents(ctx, st, []state.Event{revoke}); err != nil { t.Fatal(err) } n2, err := st.CountSecretBindingsNeedingRotation(ctx) if err != nil { t.Fatal(err) } if n2 != 0 { t.Fatalf("needs_rotation count=%d, want 0 on literal replay", n2) }🤖 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/trust_apply_test.go` around lines 89 - 131, The current test named TestApplyDeviceTrustReplayDoesNotReflagRotation is validating cross-type stickiness by applying a new EventDeviceLost event, not literal replay/idempotency. Keep this case for monotonic behavior, but add a separate assertion in the same trust flow that re-applies the exact same revoke event (same event ID via signedDeviceTrustEvent and ApplyEvents) and verifies CountSecretBindingsNeedingRotation stays at zero. This will cover true redelivery handling distinct from the existing revoked-to-lost scenario.
🤖 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/07_NAMESPACE_AND_SYNC_MODEL.md`:
- Line 749: Update the TRUST-01 paragraph in the namespace/sync model spec so it
no longer claims no sync/remote path can inject a revoked/lost row. Rephrase the
statement to limit that guarantee to locally created revoked/lost rows, while
acknowledging that propagated device.revoked/device.lost states from the sticky
apply path can reach peers. Keep the fail-closed explanation aligned with the
existing symbols TestHasEnrolledDevicesStickyAfterRevoke,
TestApplyEventsRevokedLastDeviceStaysFailClosed, EnsureRemoteDeviceTx, and
hasEnrolledDevices.
---
Nitpick comments:
In `@internal/sync/trust_apply_test.go`:
- Around line 176-192: The test TestApplyDeviceTrustLocalTargetIsNoOp only
excludes a couple of bad trust values, so it can miss other unintended changes.
Update the assertion to compare deviceTrustState(t, st, device.ID) against the
expected unchanged initial trust value from newSyncStore, using the existing
identifiers ApplyEventsWithStats, deviceTrustState, and device.ID to keep the
no-op check strict.
- Around line 89-131: The current test named
TestApplyDeviceTrustReplayDoesNotReflagRotation is validating cross-type
stickiness by applying a new EventDeviceLost event, not literal
replay/idempotency. Keep this case for monotonic behavior, but add a separate
assertion in the same trust flow that re-applies the exact same revoke event
(same event ID via signedDeviceTrustEvent and ApplyEvents) and verifies
CountSecretBindingsNeedingRotation stays at zero. This will cover true
redelivery handling distinct from the existing revoked-to-lost scenario.
🪄 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: 325ee8e6-d1c0-46d4-9063-e6454a5bca64
📒 Files selected for processing (16)
cmd/devstrap/testdata/script/sync_never_granted_epoch_wedge.txtarcmd/devstrap/testdata/script/sync_trust_propagation.txtarinternal/cli/devices.gointernal/cli/devices_test.gointernal/state/store.gointernal/state/store_test.gointernal/sync/events.gointernal/sync/trust_apply_test.gospec/00_START_HERE.mdspec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/09_SECRETS_AND_ENVIRONMENT.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
devices revoke/lost now emit synced device.revoked/device.lost events (mustVerify, same transaction as the local trust flip, before WCK rotation), so one device's revoke reaches the whole fleet: receiving devices flip the target sticky/monotonically (only pending/approved transition; the local device never flips from a remote event; only the local fingerprint ceremony resurrects) and flag needs_rotation exactly once (gated on an actual change — replays never re-flag cleared rotations). device.approved is deliberately NOT propagated: a synced approval would let one compromised device enroll attackers fleet-wide; approval stays the local P4-SEC-04 ceremony. Mutual revocation converges deterministically within one batch (HLC-earlier revoke wins; the counter-revoke quarantines once its signer flips); cross-window bystander divergence is a documented accepted residual — fail-closed either way, loud, operator re-approves the survivor. Closes the P6-SYNC-01/P6-SYNC-03 trust-propagation residual (spec/07, spec/15). e2e: sync_trust_propagation.txtar (three devices). sync_never_granted_epoch_wedge.txtar expectations updated: the revoke event also rides the post-rotation epoch, so a never-granted device quarantines two carriers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 6, 2026
Closed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/state/store.go`:
- Around line 1770-1792: The sqlclosecheck failure comes from manually closing
rotRows inside UpsertEnvProfileTx after reading secret_bindings rotation flags.
Move the query/scan logic into a small helper such as readBindingRotationFlags
that uses defer Close on the rows and returns the map before the DELETE/INSERT
work continues. Keep the existing ordering in UpsertEnvProfileTx so the result
set is fully consumed and closed before tx.tx.ExecContext runs, and reuse the
helper’s error wrapping for read/scan/close failures.
🪄 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: b1dc702f-3a7a-4c86-af9c-64efc3b49766
📒 Files selected for processing (16)
cmd/devstrap/testdata/script/sync_never_granted_epoch_wedge.txtarcmd/devstrap/testdata/script/sync_trust_propagation.txtarinternal/cli/devices.gointernal/cli/devices_test.gointernal/state/store.gointernal/state/store_test.gointernal/sync/events.gointernal/sync/trust_apply_test.gospec/00_START_HERE.mdspec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/09_SECRETS_AND_ENVIRONMENT.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
✅ Files skipped from review due to trivial changes (6)
- spec/14_MVP_ROADMAP_AND_BACKLOG.md
- spec/00_START_HERE.md
- spec/09_SECRETS_AND_ENVIRONMENT.md
- spec/16_TEST_PLAN.md
- spec/18_WORK_LOG.md
- spec/13_CLI_DAEMON_API.md
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/devstrap/testdata/script/sync_never_granted_epoch_wedge.txtar
- cmd/devstrap/testdata/script/sync_trust_propagation.txtar
- internal/cli/devices.go
- internal/cli/devices_test.go
- spec/15_SECURITY_THREAT_MODEL.md
- internal/sync/trust_apply_test.go
- internal/sync/events.go
…load quarantine - spec/07 shipped-event catalog gains device.revoked/device.lost (they had only been removed from the planned block). - A signed-but-malformed trust payload (decode failure / empty target) now wraps ErrEventVerification so it quarantines-as-consumed instead of aborting the pull batch — only an approved signer can produce one, which is exactly the compromised-device class these events cut off. Pinned by TestApplyDeviceTrustMalformedPayloadQuarantinesWithoutAbort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Dual-review record (complete).
Gates re-run on the final tree: race tests green (sync/state/cli), wedge + propagation txtars green, spec-drift passes. |
…odeRabbit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/sync/events.go`:
- Around line 898-900: The trust event decode path in the JSON unmarshal
handling is formatting the inner error with %v instead of wrapping it, so update
the return in the event payload decode block to preserve the underlying JSON
error in the chain. Use the json.Unmarshal failure branch in the trust event
processing code and change the fmt.Errorf call so the wrapped error is carried
through with %w while keeping the existing state.ErrEventVerification and
event.ID context.
🪄 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: 815e1ce2-d21e-46aa-a672-5df485c2ba60
📒 Files selected for processing (5)
internal/state/store.gointernal/sync/events.gointernal/sync/trust_apply_test.gospec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/18_WORK_LOG.md
✅ Files skipped from review due to trivial changes (1)
- spec/18_WORK_LOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sync/trust_apply_test.go
- spec/07_NAMESPACE_AND_SYNC_MODEL.md
- internal/state/store.go
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reederey87
added a commit
that referenced
this pull request
Jul 6, 2026
Live-R2 validation log for the ENV-SYNC-01 + TRUST-01 wave (PRs #130–#132): pairing-before-capture ordering, byte-identical cross-device hydrate, revocation propagation via sync alone, post-revocation quarantine + epoch wedge-out, rotation-warning propagation, and the dogfood-caught needs_rotation wipe (fixed in PR #132). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 6, 2026
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 standing P6-SYNC-01/P6-SYNC-03 residual (spec/07 "revoke is local-only", spec/15:180):
devices revoke/lostnow emit synceddevice.revoked/device.lostevents, so one device's revoke reaches the whole fleet instead of requiring a manual operator action per device.Semantics (design anchored on Keybase sigchain-revocation analysis — no trust CRDTs/ordering service for a single-user fleet):
mustVerifyEventfor both types (signer must be locally approved); sticky/monotonic — onlypending/approvedrows flip (pending→revoked is the fail-closed direction), revoked↔lost churn no-ops, the local device never flips from a remote event (a hub cannot talk a device into distrusting itself); unknown targets get a revoked placeholder (device records don't sync);needs_rotationflagged only on an actual state change so replays never re-flag cleared rotations.device.approveddeliberately NOT propagated — a synced approval would let one compromised device enroll attackers fleet-wide; approval stays the local P4-SEC-04 fingerprint ceremony.Dual review record
keys rotateremedy, spec/15 documents the residual, fail-closed/auto-retry rotation filed as follow-up. (2) cross-window mutual-revocation split — the accepted residual, but the review exposed an undocumented recovery trap (re-approval replays the counter-revoke), now pinned byTestApplyMutualRevocationCrossWindowDivergesLoudlyand documented as the two-step recovery in spec/07.Tests
internal/sync/trust_apply_test.go: flip+rotation flagging, sticky replay no-reflag, unknown-target placeholder, untrusted-signer quarantine, local-target no-op, single-batch mutual-revocation determinism (both orders), post-revoke same-batch quarantine isolation, cross-window divergence loudness.TestApplyRemoteDeviceTrustTxMatrix(state) — transition matrix incl. remote-approve rejection;TestDeviceRevokeEmitsTrustEvent(cli) — same-tx emission.sync_trust_propagation.txtar: three devices, full mutual pinning; A revokes B → C learns via sync (devices list + doctor rotation flags), B's subsequent push quarantines on C.sync_never_granted_epoch_wedge.txtarexpectations updated: the revoke event also rides the post-rotation epoch, so a never-granted device now quarantines two carriers (recovery still ends at "No open conflicts").Validation
gofmt clean · darwin+linux builds · golangci-lint 0 issues ·
go test -race ./...all ok (rebased tree) · spec-drift passes (16 files). Specs: spec/00/07/09/13/15/16 stale claims retired; spec/14TRUST-01flipped shipped.🤖 Generated with Claude Code
Summary by CodeRabbit
revokedandlostare synced to other devices on their next sync, with fail-closed verification.