Review Summary

This PR correctly moves reserved collective validation from the domain layer (manifest schema) to the application layer (push command authorization). This is architecturally sound.

Domain-Driven Design Analysis ✅

The change properly separates concerns:

• Manifest schema (Value Object): Validates structure - "Is this a valid extension manifest format?"
• Push command (Application layer): Validates authorization - "Is this user allowed to publish to this collective?"

Authorization requires external service calls (SwampClub API) and should not be in the domain's parsing logic. This refactoring follows DDD principles correctly.

Code Quality ✅

• TypeScript strict mode: No any types
• Named exports: Used correctly
• Tests updated: Rejection tests converted to acceptance tests appropriately
• License headers: Present
• Error handling: Uses UserError with clear, actionable messages
• Security: Fails closed when server is unreachable for reserved collectives

CI Status ✅

• Lint, Test, and Format Check: pass
• Dependency Audit: pass
• All 2733 tests pass

No Blocking Issues

The implementation is correct:

1. ModelType.isReservedCollective() is well-tested in model_type_test.ts
2. Error message is actionable: tells user to check network and try again
3. Non-members are still rejected via the existing isAllowed check
4. Network failures are handled safely for reserved collectives

🤖 Reviewed with Claude Code

Adversarial Review

Critical / High

None found.

Medium

1. Misleading error message for non-network failures (src/cli/commands/extension_push.ts:141-144)

   The error message claims "Could not verify membership — please check your network connection" but fires for multiple failure modes beyond network issues:

   Breaking scenario:

   • User has expired/invalid API key stored in credentials
   • whoami() returns { authenticated: false } (HTTP 401, no exception thrown)
   • getCollectives({ authenticated: false }) returns undefined (no organizations field)
   • Error fires: "please check your network connection"
   • User checks network, finds nothing wrong, is confused

   Another scenario:

   • Server API version mismatch where /api/whoami succeeds but doesn't include the organizations field
   • Same misleading error

   The security boundary is preserved (push is correctly rejected), but users may waste time debugging the wrong thing.

   Suggested fix: Check whoami.authenticated explicitly and provide differentiated error messages:

   const whoamiResult = await swampClubClient.whoami(credentials.apiKey);
   if (!whoamiResult.authenticated) {
     throw new UserError("API key is invalid or expired. Run 'swamp auth login' to re-authenticate.");
   }
   collectives = getCollectives(whoamiResult);

Low

1. No unit/integration tests for the new authorization logic (src/cli/commands/extension_push.ts:128-145)

   The PR adds 17 lines of authorization logic but the only test changes verify manifest parsing accepts reserved collectives. Missing test coverage for:

   • Reserved collective with successful server membership verification → push proceeds
   • Reserved collective with server unreachable → push rejected with appropriate error
   • Reserved collective where user is not a member → push rejected
   • Reserved collective with invalid API key (401) → current behavior (misleading error)

   The integration test file integration/extension_push_test.ts has no reserved collective test cases.

2. whoami.authenticated is never checked (src/cli/commands/extension_push.ts:130-137)

   The code extracts collectives from the whoami response but never verifies whoami.authenticated === true. For non-reserved collectives with an invalid API key, the code falls through to username matching and attempts the push anyway (which the server would reject, but the CLI could catch this earlier).

   This is existing behavior, not introduced by this PR, but the new reserved collective check makes the gap more visible.

Verdict

PASS — The security model is sound and correctly implemented. Reserved collectives require server verification with fail-closed semantics when verification is impossible. The misleading error message (Medium #1) should be addressed but does not compromise security. The code does what it claims in the PR description.