fix(hive): route mismatched local identity to managed recovery - #91
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughPriority Level: P4/NITNo P0–P3 findings identified. The change correctly classifies readable local identities as The implementation also moves the classifier out of Optional follow-up
These follow-ups are optional and are not required for merge. Confidence: 88% WalkthroughNative identity verification moved into shared binding logic. Login now distinguishes ready, mismatched, unbound, and invalid identities. Resume verification rejects mismatched keys. Tests cover these outcomes. Confidence: 95%. ChangesNative identity handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LoginIdentity
participant IdentityBinding
participant LocalKeys
participant ResumeFlow
LoginIdentity->>IdentityBinding: classify_existing_native_identity(binding, keys)
IdentityBinding->>LocalKeys: compare local key with canonical key
IdentityBinding-->>LoginIdentity: return Ready, Unbound, or Mismatched
ResumeFlow->>IdentityBinding: verify_existing_native_identity(binding, keys)
IdentityBinding-->>ResumeFlow: return false, true, or validation error
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a51217a4c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(matches!( | ||
| classify_existing_native_identity(binding, keys)?, | ||
| ExistingNativeIdentity::Ready | ||
| )) |
There was a problem hiding this comment.
When a first-time managed membership has binding.public_key == None and a healthy local key exists, the classifier returns Unbound, but this match now reports the identity as not ready. complete_login therefore skips bind_identity, reaches its final unbound branch, and returns “could not establish a native identity,” blocking all new-membership enrollment. Treat Unbound as ready when local keys are present, while reserving the recovery path for Mismatched or missing local keys.
Useful? React with 👍 / 👎.
|
Independent identity/security review is in (Claude-side, cross-model against the Codex-authored diff). It returned BLOCK on one mechanical P0 and could not break the identity logic itself — explicit no on "can anything improper reach recovery", no on silent enrollment / unconfirmed rotation / cross-company adoption, yes on the resume path being safe and coherent, no on any accidentally-load-bearing invariant being weakened. P0 — fixed in cd9f547The reviewer reproduced exactly what CI caught: Fixed as prescribed: P1 — test rename, fixed in cd9f547
P1 — the new tests do not run in CI. Confirmed, not fixed here.All four are behind P2 findings — flagged, deliberately not builtPer the program's security posture (flag, don't harden):
Both are consequences of #78's intended design rather than defects introduced here. Recording them so the decision is visible; happy to open issues if you'd rather track them. Verification after cd9f547 (local)
CI is the real gate; watching it now. |
|
Ready to merge — needs a human approver. This is identity/authentication code, so it should not land on AI review alone. Flagging rather than merging. Gate status:
What still is not proven after this merges, per #78's proof boundary: signed artifact, installed fresh-Mac recovery, custody keyring configuration in production, and the primary operator/Benjamin two-person acceptance. See the close-out plan comment on #78 — and note #92 first, since a canary built from the committed workflow would not contain this code at all. |
![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
🤖 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 `@desktop/src-tauri/src/evaos_teams/identity_binding.rs`:
- Around line 37-40: Add a Rust doc comment to classify_existing_native_identity
documenting the classification contract: Unbound when no native identity is
bound, Ready when the existing binding matches the provided keys, and Mismatched
when a binding exists but does not match. Keep the documentation focused on this
pub(super) API.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7d27f54-2c96-4dd5-8a3d-24e8757cd6fe
📒 Files selected for processing (4)
desktop/src-tauri/src/evaos_teams.rsdesktop/src-tauri/src/evaos_teams/identity_binding.rsdesktop/src-tauri/src/evaos_teams/login_identity.rsdesktop/src-tauri/src/evaos_teams/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Rust Lint
- GitHub Check: Desktop Build (macOS)
- GitHub Check: Desktop Smoke E2E (2)
- GitHub Check: Desktop Smoke E2E (4)
- GitHub Check: Windows Rust (x86_64-pc-windows-msvc)
- GitHub Check: Desktop Smoke E2E (1)
- GitHub Check: Desktop Smoke E2E (3)
- GitHub Check: Desktop Core
- GitHub Check: Desktop E2E Relay
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not introduceunsafeRust code.
Do not introduce newunwrap()orexpect()calls in production Rust paths; use?and proper error types.
Add doc comments to every new public Rust API.
Files:
desktop/src-tauri/src/evaos_teams.rsdesktop/src-tauri/src/evaos_teams/tests.rsdesktop/src-tauri/src/evaos_teams/login_identity.rsdesktop/src-tauri/src/evaos_teams/identity_binding.rs
🧠 Learnings (1)
📚 Learning: 2026-07-30T11:25:18.286Z
Learnt from: 100yenadmin
Repo: electricsheephq/evaOS-Hive PR: 82
File: desktop/src-tauri/src/shutdown.rs:258-294
Timestamp: 2026-07-30T11:25:18.286Z
Learning: When reviewing code that concerns the Hive Internal Canary supported architecture, use the supported-release-path model as the baseline: it is a single company, single relay/community, single VM, and single Hermes gateway. Do not treat multi-community runtime-pair behavior (e.g., a single local managed-agent record spanning multiple communities) as a “release-path regression” in review—this multi-community pattern is not a supported release path, so its presence should not trigger regression warnings.
Applied to files:
desktop/src-tauri/src/evaos_teams.rsdesktop/src-tauri/src/evaos_teams/tests.rsdesktop/src-tauri/src/evaos_teams/login_identity.rsdesktop/src-tauri/src/evaos_teams/identity_binding.rs
🔇 Additional comments (1)
desktop/src-tauri/src/evaos_teams/tests.rs (1)
631-632: 📐 Maintainability & Code QualityConfirm
evaos-teams-managedCI coverage.The CI configuration does not establish whether a job runs the managed-feature tests and clippy checks. Add a job if no existing job enables
evaos-teams-managed. Confidence: 50%.
evaOS review status: failedPR: #91 - fix(hive): route mismatched local identity to managed recovery evaOS review failed for this head and needs retry or operator attention. Automation note: agents should wait for this comment to reach PR URL: #91 Details: Review failed; see bot evidence for operator-only details. |
A Mac that completed Electric OAuth while holding a readable but non-canonical native key hard-errored with "This device's native Buzz identity does not match the canonical Hive identity" and returned to Sign In, without ever attempting custody recovery. local_identity_ready_for_login called verify_existing_native_identity with ?, so the mismatch Err propagated straight out of complete_login. Control never reached the recovery arm, making select_legacy_identity_candidate, identity_custody::recover_identity, and the identity_reset_required fallback all unreachable. Classify the local identity as Ready / Unbound / Mismatched and treat only a valid, readable mismatch as a recovery state, so complete_login falls through to its existing recovery arm unchanged. Everything else stays a hard error: invalid membership, invalid canonical key, locked or unreadable Keychain, and IdentityRecoveryError::Other. verify_existing_native_identity keeps its Err on mismatch for the session-resume path, which correctly defers to a fresh OAuth sign-in rather than recovering in place. No silent new-key enrollment path is introduced. Refs #78
evaos_teams.rs sits exactly at the 1000-line desktop ratchet, so the added classifier pushed it to 1018 and CI's Desktop Core job failed at desktop-check before any Rust step ran. Move ExistingNativeIdentity, classify_existing_native_identity, and verify_existing_native_identity into evaos_teams/identity_binding.rs, which already owns IdentityBinding and its validators and is not feature-gated. Net effect on the capped file is negative; behavior is unchanged. Also rename healthy_mismatched_native_identity_is_rejected_without_reset: under this change a healthy mismatch is no longer 'rejected without reset' on managed sign-in, it is routed to recovery. The test now pins the actual invariant - the two callers diverge - instead of comparing an immutable local value against itself and inspecting a default state the code under test never touches.
79cad89 to
3b315ff
Compare
evaOS review status: failedPR: #91 - fix(hive): route mismatched local identity to managed recovery evaOS review failed for this head and needs retry or operator attention. Automation note: agents should wait for this comment to reach PR URL: #91 Details: Review failed; see bot evidence for operator-only details. |
|
Fable sign-off: merged after CI went CLEAN. Independently verified before the force-push that the identity logic was byte-identical to the reviewed commit (identity_binding.rs and login_identity.rs both diff 0 lines vs cd9f547), so the rebase onto current main carried no silent change. Cross-model review record on this PR (Codex authored → Claude reviewed → BLOCK on two real issues → fixed → re-review APPROVE) is the pattern to keep for identity code. Lane E was right to refuse to self-merge this class; the merge came from the program session on the owner's direct authority. |
1 participant
Bounded stabilization for the installed #78 acceptance failure of 2026-07-31.
Observed failure
Signed candidate
38c68e257dc34b7c057fe64a1539d45cc7c72198, artifact SHA-25673d9436b003474fbe984019575119b8f03fd7bd60224d85e7ddeb55522cce733,/Applications/Hive.app: Electric OAuth succeeded and the proof-bound one-time backup-code claim was accepted, then Hive rejected the readable local native key withThis device's native Buzz identity does not match the canonical Hive identityand returned to Sign In — never attempting same-canonical-key custody recovery, never exposing the explicit lost-identity replacement action.Root cause
local_identity_ready_for_logincalledverify_existing_native_identitywith?. On a readable-but-non-canonical key that function returnsErr, so the error propagated straight out ofcomplete_login. Control never reached thebinding.public_key.is_some()recovery arm, which madekeychain_migration::select_legacy_identity_candidate,identity_custody::recover_identity, and theidentity_reset_requiredfallback all unreachable.Change
Adds a typed classifier
ExistingNativeIdentity { Unbound, Ready, Mismatched }.local_identity_ready_for_loginnow returnsOk(false)for a valid readable mismatch, socomplete_loginfalls through to its existing, unchanged recovery arm: legacy-candidate adoption →recover_identity→ onNotAvailable, stagePendingIdentityResetand returnidentity_reset_required.Deliberately not reclassified — these stay hard errors:
valid_public_keyrequire_genuine_native_identity_lossuntouched)IdentityRecoveryError::Other, bind/entitlement/enrollment/persistence failuresverify_existing_native_identitykeeps itsErr-on-mismatch behavior for its other caller, the session-resume path (evaos_teams.rs:752). That path correctly cannot recover in place, so it surfacesreauth_requiredand defers to a fresh OAuth sign-in, which now routes to recovery. No silent new-key enrollment is introduced; the only non-recovery outcome remains the explicit user-confirmed reset.Verification (run locally on this branch, not just reported)
cargo fmt --checkjust desktop-tauri-clippy(CI's exact recipe)cargo test --features evaos-teams-managed evaos_teamsnode --test scripts/check-file-sizes-core.test.mjs(reset-boundary ratchet)Tests added: mismatch is a recovery state (not
Err), matching key is ready, unbound membership is not ready, malformed membership and invalid canonical key remain hard errors.Diff is +52 net across 3 files, entirely inside the
evaos_teamsadapter surface. No new modules, crates, Tauri commands, or UI changes — thin-adapter boundary preserved.Note for reviewers (pre-existing, not fixed here)
cargo clippywith--features evaos-teams-managedreports twoneedless_borrowerrors indesktop/src-tauri/src/evaos_teams/identity_custody.rs(~lines 653, 663). They are present onmainunchanged and are invisible to CI becausejust desktop-tauri-clippybuilds without that feature. Flagged rather than fixed — touching an unrelated file is outside this diff's scope. Worth a separate one-liner, and worth deciding whether CI should lint the managed feature at all.What this does NOT prove
Source and CI only. Merge, signed artifact, installed fresh-Mac recovery, and the primary operator/Benjamin two-person acceptance remain separate claims — see the #78 close-out plan. In particular, recovery still cannot succeed until (a) the custody keyring Edge secrets are confirmed set in production and (b) a custody envelope has actually been enrolled from a device holding the canonical key.
Refs #78