refactor: implement dependency injection in controllers

[DGouron]

Summary

• GitLab controller: Dependencies injected via GitLabWebhookDependencies interface (3 gateways + 6 use cases)
• GitHub controller: Dependencies injected via GitHubWebhookDependencies interface (3 gateways + 2 use cases)
• Composition root: All instantiations moved to routes.ts where they belong
• Imports: Migrated from relative paths to @/ alias with .js extension
• Tests: Updated to verify use case calls (deps.trackAssignment.execute) instead of gateway internals
• CLAUDE.md: Added project guide rewritten for ReviewFlow (was a copy from MentorGoal frontend)
• .claude/ config: Tracked agents, rules, roles, and commands in git

Test plan

• yarn typecheck passes with 0 errors
• yarn test:ci — 917 tests pass (117 files)
• yarn lint — 0 issues
• GitHub controller: 10/10 tests pass after DI migration
• Manual test: trigger a GitHub webhook review and verify inline comments post correctly

Fixes #74

🤖 Generated with Claude Code

[DGouron]

Code Review - PR #113 (feat/74-dependency-injection-controllers)

Date: 2026-03-06
Reviewer: Claude Code (Mentor Mode)
Branch: feat/74-dependency-injection-controllers
Modified files: 41 (+2271/-981 lines)
Production .ts files changed: 10 (+ 6 test files)

---

Executive Summary

Audit	Score	Verdict
Clean Architecture	9/10	DI refactoring correctly applied; minor import regression
Strategic DDD	9/10	Clean bounded context boundaries maintained
Testing	8/10	Tests properly updated; missing PR-close path coverage
SOLID	9/10	DIP improvement is the core value of this PR
Security	9/10	No vulnerabilities; CLAUDECODE guard removal worth verifying

Overall Score: 8.8/10 — Well-executed refactoring that aligns the GitHub controller with the GitLab controller's dependency injection pattern. Clean removal of unused code. Minor import regression to fix.

---

Important Corrections (before merge)

1. Import regression in reviewAction.gitlab.cli.gateway.ts

Files: src/interface-adapters/gateways/cli/reviewAction.gitlab.cli.gateway.ts

Problem:
The file's imports were changed FROM @/ aliases TO relative ../../../ paths — going against the project convention. This is especially inconsistent because github.controller.ts in the same PR was correctly modernized from ../../../ to @/.

"Consistency is a key ingredient of software quality."
— Robert C. Martin, Clean Code, 2008

Fix:

import type { ReviewAction } from '@/entities/reviewAction/reviewAction.js'
import type { ReviewActionGateway, ExecutionContext } from '@/entities/reviewAction/reviewAction.gateway.js'
import { ExecutionGatewayBase, type CommandInfo } from '@/shared/foundation/executionGateway.base.js'

---

2. CLAUDECODE env var guard removed without explanation

Files: src/frameworks/claude/claudeInvoker.ts

Problem:
The code previously unset CLAUDECODE from the child process environment to prevent nested Claude spawning issues. This guard was removed and process.env is now passed directly. If the review tool is invoked within a Claude session, the child process could inherit CLAUDECODE and behave unexpectedly.

"When you remove something, make sure you know why it was there in the first place."
— Martin Fowler, Refactoring, 2018

Fix: Either restore the guard or add a comment explaining why it's no longer needed.

---

Improvements (backlog)

3. Missing test coverage for PR-close cleanup with injected deps

The PR close path now uses deps.reviewContextGateway.delete(...) but no test verifies this behavior.

4. normalizeGitUrl simplification loses SSH URL support

The normalizeGitUrl function was replaced with a simple .replace(/\.git$/, ''). The old function converted SSH URLs (git@host:org/repo.git) to HTTPS (https://host/org/repo). Verify that findRepositoryByRemoteUrl still works correctly with SSH remote URLs.

5. Feature removal: commentLinkEnricher clickable links

The commentLinkEnricher.ts service (50 lines + 105 lines of tests) was completely removed. This service turned file.py:42 references in review comments into clickable blob links. If the feature is still desired, consider tracking its reimplementation as a separate concern.

---

Positive Observations

Aspect	Observation
DI Pattern	GitHubWebhookDependencies mirrors the GitLab controller pattern — consistency achieved
Composition Root	Instantiation correctly moved to routes.ts
Type-only imports	Use cases imported with type keyword — controller depends on abstractions
Test refactoring	Tests assert on injected mocks, not internal implementations
Data source rules	New MCP prompt rules prevent stale local state during concurrent reviews
Dead code removal	baseUrl cleanly removed from ExecutionContext and all call sites

---

Pre-Merge Checklist

[ ] Fix import regression in reviewAction.gitlab.cli.gateway.ts (use @/ aliases)
[ ] Verify CLAUDECODE env var removal is intentional
[ ] yarn verify passes without error

---

Pedagogical Resources

Dependency Inversion Principle (DIP)

"Depend upon Abstractions. Do not depend upon concretions."
— Robert C. Martin, Agile Software Development, 2002

This PR is a textbook application of DIP. The GitHub controller now receives abstractions through GitHubWebhookDependencies instead of instantiating concrete classes. This makes the controller testable, flexible, and decoupled.

Composition Root Pattern

"A Composition Root is a (preferably) unique location in an application where modules are composed together."
— Mark Seemann, Dependency Injection in .NET, 2011

Moving instantiation to routes.ts follows this pattern correctly — the composition root wires concrete pieces together, keeping the rest of the application framework-agnostic.

[REVIEW_STATS:blocking=0:warnings=2:suggestions=3:score=8.8]

[DGouron]

Follow-up Review - PR #113

Date: 2026-03-06
Previous review: 2026-03-05

---

Blocking Issues Verification

The initial review raised 2 warnings and 3 suggestions. Here is the verification:

#	Issue	Status	Comment
1	Import regression in reviewAction.gitlab.cli.gateway.ts	⚠️ FALSE POSITIVE	This file is not part of the PR diff. The difference between branches is due to newer commits on master (PR #106 merged after this branch was created). Will auto-resolve on rebase.
2	CLAUDECODE env var guard removed	⚠️ FALSE POSITIVE	Same situation — the guard was added to master after this branch diverged. The PR does not remove it; the branch simply predates it. Will auto-resolve on rebase.
3	Missing test for PR-close cleanup path	💡 Suggestion (backlog)	No change — this was a suggestion, not blocking.
4	normalizeGitUrl simplification	💡 N/A	File not part of this PR.
5	commentLinkEnricher removal	💡 N/A	File not part of this PR.

Note: The initial review appears to have analyzed files that are NOT in the PR diff. The reviewAction.gitlab.cli.gateway.ts and CLAUDECODE differences exist only because master received new commits (from PR #106) after this branch was created. A rebase onto master will resolve both discrepancies.

---

New Problems Detected

No new problems detected in the modifications.

Quick scan results:

• ✅ No any types introduced
• ✅ No type assertions in production code (1 new as unknown as in test mock — acceptable)
• ✅ No Law of Demeter violations
• ✅ All production imports use @/ aliases correctly
• ✅ GitHubWebhookDependencies interface properly typed with gateway + use case abstractions
• ✅ Composition root wiring in routes.ts is clean
• ✅ Tests updated to verify use case calls via injected mocks

Minor observation (non-blocking): The test file github.controller.test.ts adds a new relative import (../../../../../interface-adapters/...) instead of @/. This is consistent with the existing test file's pre-existing pattern but diverges from the project convention. Consider normalizing all test imports to @/ in a separate cleanup.

---

Final Verdict

Criterion	Status
Blocking issues fixed	N/A (0 valid blockers — both warnings were false positives)
New blockers	0
Ready to merge	✅ Yes

Recommendation

1. Rebase onto master before merging — this will pick up the @/ imports from PR feat: enrich review comments with clickable file:line links #106 and the CLAUDECODE guard, resolving the apparent discrepancies noted in the initial review.
2. After rebase, run yarn verify to confirm clean build.

---

[REVIEW_STATS:blocking=0:warnings=0:suggestions=1:score=9]