refactor(quality): thermo-nuclear review — decompose AgentCatalogService, dedup boilerplate

[thomasluizon]

A thermo-nuclear code-quality review of orbit-api (the strict maintainability/structure standard: giant files, spaghetti, missed simplifications, leaky abstractions, duplication). Everything here is behavior-preserving — no behavior, API, or DB-schema changes — and the full suite stays green.

Applied (all verified green)

Change	What	Why
AgentCatalogService 1764 LOC → partial-class split	Moved the ~26 static catalog-builder method bodies verbatim into 4 focused files (.Capabilities / .Operations / .Surfaces / .UserDataCatalog); core file is now ~190 LOC (fields/ctor/getters/helpers).	A 1764-line file was the single worst file-size smell. Now navigable by concern; the 10 catalog-completeness tests prove the emitted catalog is identical.
AiController boilerplate extraction (~−87 LOC)	The 4 metadata GET endpoints (capabilities/operations/data-catalog/surfaces) were ~30-line copies of build policy context → evaluate → forbid-on-deny → audit → return catalog. Extracted a generic ReadCatalogAsync<T>(capabilityId, operationName, description, readCatalog, ct); each endpoint is now a 6-line expression body.	Killed near-total duplication; the policy+audit pattern now lives in one place.
HabitScheduleService dedup	GetUnionScheduledDates re-listed the exact 5-condition streak-eligibility filter that the existing private IsStreakContributingHabit already encapsulates → replaced with a single call.	One source of truth for "does this habit contribute to the streak."
ProcessedExternalEvent base	Extracted an abstract base for the twin ProcessedStripeEvent / ProcessedPlayNotification entities (shared ProcessedAtUtc); each keeps only its provider key. EF model verified byte-identical (has-pending-model-changes → no changes).	Removes drift risk between the two webhook-dedup entities.

Verification: dotnet build → 0 errors. dotnet test → 3719 passed, 0 failed, 0 skipped (baseline preserved exactly). The AgentCatalogService split was additionally script-verified to be byte-identical member content, and the entity-base change confirmed not to shift the EF model.

Deferred (documented follow-ups — real risk, deserve dedicated PRs)

• OrbitDbContext (705 LOC) → IEntityTypeConfiguration<T> + ApplyConfigurationsFromAssembly. The Configure* methods take runtime parameters (encConverter/nullableEncConverter from the instance encryption service, isPostgres from the provider) that branch the mapping. IEntityTypeConfiguration.Configure(builder) can't receive those, and the context deliberately varies the cached model per instance — migrating risks a silent model shift. Needs a careful, snapshot-diffed effort.
• ProcessUserChatCommand (1227 LOC) handler decomposition. Methods are interdependent and share three private accumulator types; clean extraction means new collaborator services + constructor changes + reworking the 1761-LOC mock-heavy test. Recommended order: (1) extract the pure tool-outcome mappers (~294 LOC) to a ToolResultMapper; (2) extract RunBackgroundPostResponseWork + fact-extraction into a PostResponseWorker; (3) a ChatContextLoader. One PR each.
• ServiceCollectionExtensions (723 LOC) is already domain-grouped; the per-tool AddScoped lines and the distinctly-shaped dependency-record factories can't be collapsed without reflection-based auto-registration, which would change registration semantics. Left as-is by design.

🤖 Generated with Claude Code

[claude]

PR Review 250 - APPROVE - 0 Critical, 0 High, 0 Medium findings. Pure structural refactor: AgentCatalogService decomposed from ~1764 to ~190 LOC via partial classes, ReadCatalogAsync generic helper deduplicates 4 identical endpoint bodies, IsStreakContributingHabit extracted, ProcessedExternalEvent base class added. Correctness PASS (behavior-preserving), Dead code PASS (all partials wired), Architecture PASS (domain-aligned split), Security PASS (minor improvement: GetAgentAuthMethod captured once vs twice), Contract PASS (no DTO/route changes), Hard rules PASS (DateTime.UtcNow only in factory Create() methods as permitted).

[thomasluizon]

Paired nuclear-review PR: thomasluizon/orbit-ui-mobile#307 (same thermo-nuclear pass, client side).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.6% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #250

Scope: PR #250 — refactor(quality): thermo-nuclear review — decompose AgentCatalogService, dedup boilerplate
Recommendation: APPROVE

Summary

Behavior-preserving quality refactor across four changes: splitting the 1764-line AgentCatalogService into focused partial-class files, extracting a generic ReadCatalogAsync<T> helper in AiController (~−87 LOC of duplicated pattern), collapsing a duplicated 5-condition habit filter in HabitScheduleService, and extracting a ProcessedExternalEvent abstract base for the two idempotency-ledger entities. No API surface, DB schema, or behavior changes. All four changes are correct.

Findings

Critical

None

High

None

Medium

None

Low / Info

[Info] Audit-before-execute ordering (pre-existing, not introduced here)

• dimension: Security (#12)
• location: src/Orbit.Api/Controllers/AiController.cs:87-99
• issue: ReadCatalogAsync<T> records AgentOperationStatus.Succeeded before calling readCatalog(). The pre-refactor per-endpoint code had the identical ordering — faithfully preserved. The catalog is a frozen in-memory list built at DI startup and cannot throw, so this is not a realistic gap.
• fix (future): call readCatalog() first, then record the audit entry. Out of scope for a behavior-preserving refactor.

Subagents

Agent	Verdict
security-reviewer	PASS — [Authorize] coverage intact; policy evaluation and audit trail preserved correctly in ReadCatalogAsync; catalog responses contain no PII/secrets; ProcessedExternalEvent introduces no new security surface
contract-aligner	N/A — no DTO, route, or packages/shared type changes

Validation

Check	Result
Build (dotnet)	N/A (--skip-build; PR reports 0 errors)
Tests (dotnet)	N/A (--skip-build; PR reports 3719 passed, 0 failed, 0 skipped)

What's good

• AgentCatalogService decomposition: 1764 LOC → 190-LOC core + 4 focused partial-class files by concern. #pragma warning disable suppressions are carried consistently. Helper methods (CreateCapability, CloneJson, ToDisplayName) stay in the core file where they belong.
• ReadCatalogAsync<T> is correctly scoped: extracts exactly the repeated pattern (userId → policy eval → forbid-on-deny → audit → Ok) without over-abstracting. Four genuinely-identical call sites warranted it.
• HabitScheduleService dedup is minimal and correct: inline 5-condition guard → single IsStreakContributingHabit(habit) call, removing the only divergence risk between the two callers.
• ProcessedExternalEvent base is architecturally correct: carries only ProcessedAtUtc; EF Core's per-concrete-type configurations pick up the inherited property without a new abstract registration — no migration required.
• No dead code, no narration comments, no type-safety holes, no contract changes across the entire diff.

Recommendation

Clean approval. The deferred items (OrbitDbContext and ProcessUserChatCommand decomposition) are sensibly scoped to dedicated follow-up PRs.

🤖 Reviewed with Claude Code