Skip to content

#41 M1: drift-class collaborator fixes (#3 resolution, #11 re-invite, #12 leave-strip) - #2

Merged
xianxu merged 12 commits into
mainfrom
000041-collaborator-hardening
Jun 3, 2026
Merged

#41 M1: drift-class collaborator fixes (#3 resolution, #11 re-invite, #12 leave-strip)#2
xianxu merged 12 commits into
mainfrom
000041-collaborator-hardening

Conversation

@xianxu

@xianxu xianxu commented Jun 3, 2026

Copy link
Copy Markdown
Owner

xianxu and others added 12 commits June 2, 2026 16:34
@xianxu @claude
#41 M1: drift-class collaborator fixes (#3 resolution, #11 re-invite,…
11a7879 
… #12 leave-strip)

#3 FingerprintForLogin: drop verified.yaml as a login→fp mapping source
(it's offline-sig-verification only per the collaborator-state-machine
target invariant 3); resolve keys-branch <login>.asc → peer sidecar.

#11 InviteCollaborator: make the stale-invitation list/delete failures
hard errors instead of swallowing them — a swallowed failure lets the
PUT silently no-op (no email) against an existing invitation.

#12 LeaveBrain: route through a new shared stripMember helper (extracted
from RemoveRecipient) so leave clears EVERY store — manifest re-key +
verified.yaml + keys-branch <login>.asc + collaborator — not just the
manifest. A lingering keys-branch pubkey let a peer's auto-admit
resurrect the leaver (invariant #2; the #38/#40 bug class on the self
path). LeaveBrain gains a ctx param (RevokePubkey needs it); CLI/TUI
callers + LeaveResult fields updated to surface a failed keys-branch strip.

Tests: new gpg-free TestFingerprintForLogin_IgnoresVerifiedYaml (#3);
leave refusal tests updated for the ctx signature. stripMember's
store-clearing is covered by the existing recipient-remove integration +
brain-vm-e2e paths. go build/vet + full suite green (unsandboxed for the
gpg-dependent brain integration tests).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M1: review fix (stripMember login-resolution comment) + atlas
06535ad 
Fresh-eyes review of M1 (BASE a0b14eb..11a7879) returned no Critical/
Important; one Minor — the stripMember knownLogin doc implied
FingerprintForLogin still consults verified.yaml. Clarified: the verified
login is a removal hint from RemoveVerifiedFor's return, not a canonical
mapping (#3 dropped that). atlas/nous/collaborator-lifecycle.md updated
for #3/#11/#12 + the shared stripMember + what M1 landed vs remains.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M1 milestone-close: review dispositions + Minor fixes
a7c028e 
sdlc milestone-review (fresh-context judge) returned no Critical. Important
findings dispositioned:
- leave has no isolated test → architectural: LeaveBrain is GitHub-coupled
  (refuses on file://), so its full flow can't run in the GitHub-free e2e.
  The shared stripMember IS e2e-proven via the recipient-remove assertion;
  leave's strip is verified by construction + carried to the dogfood (the
  issue done-when already designates GitHub-layer effects for the dogfood).
- fp→login revoke-target prefers the verified.yaml-hint over the canonical
  keys-branch → pre-existing nous#40 best-effort behavior; flagged for M3 #10.

Minor fixes: leave.go error wrap ("leave: %w", not push-specific) + the
stale mid-flow doc comment. M1 log line softened (stripMember is e2e-proven
via recipient-remove; the leave wiring is by-construction + dogfood).

Review-Verdict: FIX-THEN-SHIP
Review-Window: a0b14eb..06535ad
Review-Reason: judge emitted no leading verdict token (recorded 'unknown'); dispositioned above — no Critical, Importants addressed/flagged, Minors fixed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M2: rotation / one-fp-per-login via manifest recipient_logins map (…
3b58449 
…#7/#8)

The durable login→admitted-fp record auto-admit needs to retire a
superseded key on rotation. Lands as an additive inline map in the
manifest — `recipient_logins: {login: FP, ...}` — co-located with
`recipients:` (operator-confirmed design fork: fewer stores than a
separate members.yaml; survives nous#35 compaction unlike git history).

- lib/brain/manifest.go + write.go: RecipientLogins field + parseInlineMap
  + sorted inline render (omitted when empty). Round-trip tests.
- lib/brain/autoadmit.go: on a login whose <login>.asc fp differs from its
  recipient_logins entry, evict the old fp from recipients, admit the new,
  update the map, surface AdmittedRecipient.SupersededFingerprint. The
  verified.yaml drift gate still fires first (an operator-pinned login
  refuses the rotation until re-verified). Sneakernet recipients (no map
  entry, no keys-branch file) are structurally never visited.
- lib/brain/verified.go: FingerprintForLogin reads the map first (it's the
  authoritative admitted-fp record), then keys-branch, then sidecar.
- lib/brainsync/recipient.go: stripMember drops the login's map entry on
  removal so a later re-admit isn't mistaken for a rotation.
- lib/brainsync/watch.go: auto-admit commit/log notes "rotated from <old8>".

Test: TestEndToEnd_RotationSupersede (gpg integration) admits ying@fp1,
rotates her <ying>.asc to fp2, re-runs auto-admit, asserts recipients has
fp2 not fp1, recipient_logins[ying]=fp2, SupersededFingerprint=fp1, and
operator's own recipient is untouched. go build/vet + lib suites green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M2: review fixes — guarded fp slicing + post-rotation idempotence…
2f04f7c 
… test

M2 fresh-eyes review: no Critical, 2 Important (both judged acceptable —
the FingerprintForLogin map iteration matches by login key so a duplicate
fp can't misresolve a given login; security posture unchanged, rotation
gated by the same verified.yaml drift check as first-admit). Applied the
cheap robustness wins:
- watch.go uses the existing guarded shortFpLast8 for the auto-admit fp
  slicing (Minor: a <8-char fp would have panicked; also DRYs a
  pre-existing unguarded slice).
- TestEndToEnd_RotationSupersede gains a post-rotation idempotence
  assertion (re-run auto-admit → 0 admissions, guarding the
  existing-and-not-rotated short-circuit).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M2 milestone-close: SHIP
907c9ad 
sdlc milestone-review (fresh-context judge) returned CLEAN — Review-Verdict
SHIP. One doc-accuracy rec applied: the #3 Spec bullet now records the M2
re-prioritization of FingerprintForLogin to map-first (recipient_logins is
the authoritative admitted-fp record) → keys-branch → peer sidecar. Spec
finding checkboxes ticked for the M1+M2 findings (#3/#11/#12/#7+#8).

Review-Verdict: SHIP
Review-Window: a7c028e..2f04f7c

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M3: membership push-race retry (#6) + login-rename detection (#10)
a6d42ff 
#6 — membership mutations (recipient remove, leave, auto-admit) pushed
directly via AddCommitPush, which returns ErrPushRejected with no retry:
a concurrent operator's push left a committed-but-unpushed membership
drift. New brainsync.pushMembershipChange wraps the commit+push and, on a
rejected push, ResetToRemoteMain (fetch + reset --hard origin/main) and
re-applies the mutation on the merged state (idempotent set-ops converge).
stripMember + autoAdmitBrain route through it. ResetToRemoteMain added to
git.go. Plain-git unit test (TestPushMembershipChange_RetriesOnConcurrentPush)
proves a stale operator's change converges with a concurrent push, no clobber.

This refactor also resolves the M1 review Important #2: stripMember now
resolves the revoke-target login via the canonical keys-branch
(LoginForFingerprint) + sidecar, no longer preferring a stale verified.yaml
hint. The shared manifest param dropped (apply re-reads on each attempt).

#10 — login rename leaves the old login orphaned in keys branch /
recipient_logins while collaborator ops use the new login. gh.ListCollaborators
+ pure brain.DetectLoginDrift flag recorded logins that are no longer current
collaborators; `nous brain recipient list` warns. Detection-only; auto-heal
deferred (per plan). Unit test TestDetectLoginDrift.

go build/vet + lib suites green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M3: review fix — refuse membership push on a dirty tracked tree
458a5d0 
M3 fresh-eyes review flagged one Critical: the retry's `reset --hard
origin/main` discards local tracked changes, and AddCommitPush's `git add
-A` would have bundled unrelated uncommitted edits into the rejected commit
— so a retry could silently roll them back (worst case: the daemon's
auto-admit racing an operator's edits on an autosave-off brain).

Fix protects ALL membership ops (not just auto-admit): pushMembershipChange
now refuses up front if the brain has pre-existing uncommitted *tracked*
changes (SafeToFastForward) — so only the membership change is ever staged
and a reset loses nothing the caller didn't author here. Untracked drafts
are tolerated. Manual CLI ops get a clear "commit or discard first" error;
the daemon skips the tick (soft-logged) and retries when clean.

Also: defensive comment on ResetToRemoteMain re: the never-pushed-brain
first-push case (aborts cleanly, doesn't loop). Other review findings were
all ✓ correct. Build + lib suites (incl. gpg integration) green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M4: collaborator-state-machine target reconciliations (#4/#5/#9)
bbfbbb9 
Doc-only reconciliation of the codex-review findings into the target:
- #4 — invariant 2 reworded to "every brain-side store" + explicit local-keyring
  caveat (operator-managed, not auto-cleared, not a resurrection vector;
  optional --purge-key deliberately non-default since the keyring is shared
  across brains).
- #5 — added the "Recipient — local-ahead" matrix row (footnote ³): manifest
  changed locally but re-key push failed → remote lags. Under-shares, no leak;
  #6's retry + the unpushed-retry path heal it.
- #9 — refined the Accepted-(no pubkey) open question: self-cure capability
  EXISTS (nous brain join re-runs accept+publish; accept no-ops if already
  accepted on web) — the gap is discovery/TUI surfacing, not plumbing.
- Lede crypto-store list + matrix name the recipient_logins map; leave
  transition row notes the full stripMember strip; ## Revisions entry added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M3 milestone-close: SHIP + atlas dirty-tree note
3f4c365 
sdlc milestone-review returned Review-Verdict SHIP. The one Important
(auto-admit behaviour on a dirty tracked tree) is already handled by the
review-fix guard in pushMembershipChange (refuses up front; daemon pauses +
retries when clean). Added the suggested one-line atlas caveat to the
"Concurrency + drift" section. Spec finding checkboxes ticked for #6/#10
(M3) and #4/#5/#9 (M4, doc reconciliations committed in bbfbbb9).

Review-Verdict: SHIP
Review-Window: 907c9ad..458a5d0

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 M4 milestone-close: FIX-THEN-SHIP
6a218cd 
sdlc milestone-review flagged one doc-accuracy fix: the #9 mechanism
description was wrong — `nous brain join OWNER/REPO` cures the
accepted-on-web state via `runBrainJoinRepublish`, which SKIPS invitation
handling and re-publishes the pubkey; it does NOT call gh.AcceptInvitation
(my earlier "AcceptInvitation is a no-op" claim was inaccurate). Verified
against cmd/nous/brain_join.go:82,97 and corrected the target. The
conclusion (capability exists; gap is discovery/TUI) is unchanged.

Review-Verdict: FIX-THEN-SHIP
Review-Window: 458a5d0..3f4c365

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu @claude
#41 close: collaborator-lifecycle hardening — all 12 findings done
323c8df 
status → done, actual 8h (est 9). All four milestones closed + reviewed
(M1 FIX-THEN-SHIP, M2 SHIP, M3 SHIP, M4 FIX-THEN-SHIP). Archived to history.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xianxu
xianxu merged commit 4816b8b into main Jun 3, 2026
1 check passed
@xianxu
xianxu deleted the 000041-collaborator-hardening branch June 3, 2026 00:56
xianxu added a commit that referenced this pull request Jun 7, 2026
@xianxu @claude
#44 #45: file Google OAuth shim (instance #2) + shim-all-externals um…
ba65771 
…brella

#44 shim(google-oauth) — instance #2 of the shim(X) pattern (ariadne#71),
scoped to charon's lib/provider/oauth surface; stresses the async-callback case
+ a harder (interactive-consent) grounding boundary.
#45 shim every nous external dependency + integrated e2e mock harness (the
deterministic-shell demo); deps: [nous#42, nous#44]. Inventory of nous externals
included. ariadne#71 now deps on both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@xianxu