Adversarial Review

Critical / High

None found. The code is well-structured with appropriate error handling.

Medium

1. Inconsistent collectives handling between login and whoami (auth_login.ts:194-201 vs auth_whoami.ts:58-64)

   • login: ...(collectives ? { collectives } : {}) — omits field if undefined
   • whoami: collectives: collectives ?? [] — saves empty array if undefined

   Breaking example: After logging in via an old server without organization support, auth.json has no collectives field. After running swamp auth whoami against the same server, collectives: [] is saved. The difference is cosmetic since both result in the same behavior (returning only explicit collectives), but it creates inconsistent file state.

   Suggested fix: Use identical patterns in both commands — either always save collectives: [] or always omit when undefined/empty.

2. Server-controlled trust expansion (by design, but worth noting)

   A compromised swamp.club server or a user being added to an organization they don't control would result in automatic extension trust for that collective. The trustMemberCollectives: false opt-out exists, but users may not know to use it until after unexpected extensions are installed.

   Suggested improvement: Consider logging when membership collectives are being used (at debug level) so users can trace unexpected auto-resolution behavior.

Low

1. No validation of collective names from server (swamp_club_client.ts:64-65)

   getCollectives trusts whatever the server returns in organizations[].slug. If a malicious server returns unusual strings (e.g., with special characters), they pass through. However, this is safe because:

   • Collective names are only used for allowedCollectives.includes() string comparison
   • No path construction or command execution uses these values

   The current implementation is secure but adding a simple slug format validation (alphanumeric + hyphen) would add defense-in-depth.

2. Read-modify-write race on auth.json (auth_whoami.ts:40-64)

   load() → API call → save({...credentials, collectives}) pattern could lose concurrent modifications. Practically low risk since the CLI is single-user and atomic write is used.

3. Missing test for empty organizations array from server

   When whoami.organizations is [], getCollectives returns [] which correctly falls through to explicit-only collectives. This behavior is correct but not explicitly tested.

Verdict

PASS — No blocking issues found. The implementation handles edge cases well (graceful degradation when auth unavailable, opt-out mechanism, proper file permissions on auth.json). The inconsistency in Medium #1 is cosmetic and doesn't affect runtime behavior. The security model (trusting user memberships) is explicitly documented in the PR description with appropriate opt-out.

Review Summary

This PR cleanly implements membership-based collective trust with good design decisions. No blocking issues found.

Strengths

• Clean domain modeling: The resolveTrustedCollectives function is a pure function with clear semantics - merge explicit config with membership, deduplicate, respect opt-out
• Comprehensive test coverage: 8 new tests for resolveTrustedCollectives covering defaults, merging, deduplication, opt-out flag, and edge cases (undefined/empty arrays). 2 new tests for auth repository round-trip with collectives
• Graceful degradation: Missing auth.json, old auth.json without collectives, or unreadable files all fall back silently to explicit config only
• Security-conscious design: trustMemberCollectives: false opt-out gives users full control
• Well-documented: Design docs and skills updated with the new behavior

Suggestions (non-blocking)

1. Minor optimization opportunity: AuthRepository is loaded twice in runCli - once in initTelemetryService (for auth token) and once for collectives. Could be extracted to a shared variable, but acceptable since startup isn't a hot path.

2. Documentation note: The different patterns between login (...(collectives ? { collectives } : {})) and whoami (collectives: collectives ?? []) are intentional - login preserves undefined for old servers, whoami refreshes to current state. This nuance could be captured in a code comment for future maintainers.

Domain-Driven Design

The implementation follows DDD principles appropriately:

• AuthCredentials as a Value Object with cached membership data
• Configuration (trustMemberCollectives) in the repo marker where it belongs
• Pure resolution logic separated from persistence concerns

LGTM - ready to merge.