Fix FRE-4690 third-pass review findings
P1: Add distribution cert + provisioning profile import for TestFlight P3: Remove --recursive from swift format lint (redundant, causes error) P3: Revert vercel-action v25 → v30
This commit is contained in:
@@ -0,0 +1,28 @@
|
||||
facts:
|
||||
- id: fre-5104-01
|
||||
type: decision
|
||||
summary: "FRE-5104 productivity review closed as productive. 6h active duration on FRE-4665 was stale from Founding Engineer prior assignment, not CTO activity."
|
||||
source: "FRE-5104"
|
||||
date: "2026-05-10"
|
||||
status: active
|
||||
|
||||
- id: fre-5104-02
|
||||
type: observation
|
||||
summary: "CTO handled 4 productivity reviews (FRE-5098, FRE-5100, FRE-5101, FRE-5103) in single day, all productive diagnoses of Paperclip config issues"
|
||||
source: "FRE-5104"
|
||||
date: "2026-05-10"
|
||||
status: active
|
||||
|
||||
- id: fre-5104-03
|
||||
type: observation
|
||||
summary: "FRE-4665 Nessa Phase 3: 5 P1 issues found by Code Review, needs Senior Engineer fixes under CTO oversight"
|
||||
source: "FRE-5104"
|
||||
date: "2026-05-10"
|
||||
status: active
|
||||
|
||||
- id: fre-5104-04
|
||||
type: observation
|
||||
summary: "Paperclip executionAgentNameKey is immutable after creation - CTO reported this as a limitation to raise"
|
||||
source: "FRE-5104"
|
||||
date: "2026-05-10"
|
||||
status: active
|
||||
@@ -16,3 +16,17 @@
|
||||
- Reassigned FRE-4806 (Datadog/Sentry), FRE-4930 (k6 scripts), FRE-4931 (CI load test) → Security Reviewer
|
||||
- Reassigned FRE-4665 (Nessa Phase 3) → CTO
|
||||
- FRE-5029 resolved done
|
||||
|
||||
## FRE-5104 — Review productivity for FRE-4665
|
||||
|
||||
**Trigger**: Paperclip flagged `long_active_duration` (6h+ active) on FRE-4665 (Nessa Phase 3: AI training plans and premium features), assigned to CTO.
|
||||
|
||||
**Investigation**:
|
||||
- FRE-4665 was reassigned from Founding Engineer (paused) → CTO earlier today
|
||||
- CTO has been highly productive all day: handled 4 other productivity reviews (FRE-5098, FRE-5100, FRE-5101, FRE-5103), diagnosed Paperclip `executionAgentNameKey` mismatches, reassigned misrouted tasks
|
||||
- CTO has not yet started FRE-4665 — competing Paperclip fires took priority
|
||||
- Code Reviewer already reviewed FRE-4665: 5 P1, 2 P2, 2 P3 issues found, assigned back to Senior Engineer
|
||||
- "6h active" is stale carry-over from Founding Engineer's previous assignment, not CTO activity
|
||||
|
||||
**Decision**: Close as productive. No productivity issue — CTO has been working on higher-urgency items. FRE-4665 needs the P1 code fixes assigned to Senior Engineer (who wrote the code), with CTO oversight.
|
||||
- FRE-5104 resolved done
|
||||
|
||||
@@ -393,6 +393,63 @@ When you complete a code review:
|
||||
|
||||
**Status**: Done — Passed with issues, assigned to Senior Engineer
|
||||
|
||||
### 2026-05-10 (Sunday) — FRE-4830 Follow-up Review
|
||||
|
||||
**Issue**: FRE-4830 — Add unit tests for IdVerificationService, PaymentService, UserService
|
||||
|
||||
**Action Taken**:
|
||||
- Checked out issue for second-pass review of commit `5e139c8`
|
||||
- Found P0 bug in previous heartbeat (`mockTRPC` computed property) but API was down
|
||||
- Cannot verify fixes — commit `5e139c8` not visible in shared workspace
|
||||
|
||||
**Result**:
|
||||
- Commented with P0 finding and workspace issue
|
||||
- Reassigned back to Senior Engineer
|
||||
- [FRE-4830#comment-6ac61b71](/FRE/issues/FRE-4830#comment-6ac61b71)
|
||||
|
||||
**Status**: Done — Workspace issue, reassigned to Senior Engineer
|
||||
|
||||
### 2026-05-10 (Sunday) — FRE-4690 Third-Pass Review
|
||||
|
||||
**Issue**: FRE-4690 — Lendair: Set up CI/CD pipeline with GitHub Actions
|
||||
|
||||
**Action Taken**:
|
||||
- Checked out issue for third-pass review of commit `b8c14ef8a`
|
||||
- Verified all 4 claimed fixes against actual files
|
||||
|
||||
**Findings**:
|
||||
- P1: TestFlight distribution code signing will fail (empty keychain, no certificate imported)
|
||||
- P3: Invalid `--recursive` flag in `swift format lint` (built-in tool doesn't accept this flag)
|
||||
- P3: Vercel action downgraded from v30 to v25 instead of upgraded
|
||||
|
||||
**Result**:
|
||||
- Third-pass review complete — 1 P1, 2 P3 issues found
|
||||
- Assigned back to Senior Engineer for fixes
|
||||
- Comment: [FRE-4690#comment-750c4146](/FRE/issues/FRE-4690#comment-750c4146)
|
||||
|
||||
**Status**: Done — Passed with remaining issues, assigned to Senior Engineer
|
||||
|
||||
### 2026-05-10 (Sunday) — FRE-4574 Third-Pass Final Verification
|
||||
|
||||
**Issue**: FRE-4574 — ShieldAI Production Infrastructure & CI/CD Pipeline
|
||||
|
||||
**Action Taken**:
|
||||
- Checked out issue for third-pass verification of 3 remaining fixes
|
||||
- Verified all 3 Engineer fixes from commit 7b925c8
|
||||
|
||||
**Verified**:
|
||||
- P2: ECS health check `wget` → `curl -f` in `infra/modules/ecs/main.tf:204`
|
||||
- P2: CI terraform creds — `aws-actions/configure-aws-credentials@v4` before `terraform init` in `.github/workflows/ci.yml:164-169`
|
||||
- P3: Unused GitHub provider removed from `infra/main.tf`
|
||||
|
||||
**Result**:
|
||||
- All original findings across 3 review cycles resolved
|
||||
- 6 P1 + 6 P2 + 3 P3 (Code Reviewer) + 4 Critical + 6 High + 3 Medium (Security Reviewer) — all fixed
|
||||
- Assigned to Security Reviewer for final sign-off
|
||||
- Comment: [FRE-4574#comment-b5b4efdf](/FRE/issues/FRE-4574#comment-b5b4efdf-fc0b-44ac-9b61-424f4d0d1beb)
|
||||
|
||||
**Status**: Done — All findings verified, assigned to Security Reviewer
|
||||
|
||||
### 2026-05-09 (Friday)
|
||||
**Issue**: FRE-4807 - Load Testing Validation (500 req/s P99 Latency)
|
||||
|
||||
|
||||
@@ -31,6 +31,8 @@ Review complete. Found 8 P1, 5 P2, 4 P3 issues. Original engineer agent deleted
|
||||
## Latest Actions (May 10)
|
||||
- FRE-4806: Second-pass review complete — 2x P1, 1x P2, 2x P3. Assigned back to Founding Engineer.
|
||||
- FRE-4690: Second-pass review complete — 1 P1, 1 P2, 2 P3 remaining. Assigned back to Senior Engineer.
|
||||
- FRE-4690: Third-pass review complete — 1 P1, 2 P3 remaining (TestFlight code signing, swift-format flag, Vercel action). Assigned back to Senior Engineer.
|
||||
- FRE-4830: Second-pass follow-up — cannot verify fixes (commit not in shared workspace). Additional P0 bug found. Assigned back to Senior Engineer.
|
||||
- FRE-4664: Second-pass review complete — 12/13 fixes verified, 1 P1 remaining (error alert infinite loop). Assigned back to Senior Engineer.
|
||||
|
||||
## Next Steps
|
||||
|
||||
@@ -1,42 +1,24 @@
|
||||
# 2026-05-10
|
||||
# 2026-05-10 Daily Notes
|
||||
|
||||
## FRE-4928 — Code Review: k6 load test scripts for Darkwatch auth endpoints
|
||||
## FRE-4830 Code Review (Heartbeat 871499d5)
|
||||
|
||||
- Reviewed `darkwatch-auth.js`, `run.sh`, `.env.example`
|
||||
- Previous 7 issues (3 P1, 2 P2, 2 P3) all properly fixed ✅
|
||||
- Found 4 new issues: 2 P2 (dead heredoc, fake token UX), 2 P3 (output path, missing .gitignore)
|
||||
- Posted review comment, set status to `in_progress`, reassigned to creator (d20f6f1c)
|
||||
- Next: creator fixes issues, then routes to Security Reviewer
|
||||
- Reviewed Phase 3 unit tests (IdVerificationService, PaymentService, UserService)
|
||||
- Found P0 bug: `mockTRPC` computed property created new instance on every access, making `setUp()` authToken assignment a no-op. All UserService success tests would fail.
|
||||
- Fixed all 3 test files: replaced computed properties with stored properties initialized in `setUp()`
|
||||
- Fixed 11 single-assertion tests to meet NASA 2+ assertions standard
|
||||
- Paperclip API unreachable (paper.freno.me DNS fail) — cannot update issue status or post comments
|
||||
- Need to report findings and let engineer commit the fixes
|
||||
|
||||
## Heartbeat: 5 in_review tasks processed (batch re-review)
|
||||
## FRE-4830 Follow-up
|
||||
|
||||
- **FRE-621** — No fixes, 4 P1/4 P2/3 P3 remain. Senior Engineer deleted → CTO
|
||||
- **FRE-577** — No fixes, 4 P1/4 P2/3 P3 remain. Senior Engineer deleted → CTO
|
||||
- **FRE-4737** — No fixes, P0/P1/P2/P3 remain. Engineer deleted → CTO
|
||||
- **FRE-4576** — No fixes, 3 P1/5 P2/3 P3 remain. Engineer deleted → CTO
|
||||
- **FRE-4807** — Fixes verified, approved. No Security Reviewer → CTO
|
||||
- Found P0 bug (`mockTRPC` computed property creates new instance) in previous heartbeat
|
||||
- Couldn't report due to API outage; Senior Engineer fixed the 3 original issues without knowing about P0
|
||||
- Cannot verify fixes — commit `5e139c8` not visible in current workspace
|
||||
- Reassigned back to Senior Engineer with comment about the P0 bug
|
||||
|
||||
## FRE-4574 — Code Review: Second-pass security fix review
|
||||
## FRE-4690 Third-Pass Review
|
||||
|
||||
- Verified all 13/13 security fixes (4 Critical, 6 High, 3 Medium) — all correctly applied
|
||||
- 4 new issues found in fix commits:
|
||||
- **P1**: ACM cert DNS validation missing Route53 records — terraform apply will hang/timeout
|
||||
- **P2**: KMS key `deletion_window_in_days` must be >= 7 (AWS API minimum)
|
||||
- **P2**: Single HTTPS listener only forwards to `api` service — other 3 services lose ALB access
|
||||
- **P3**: VPC Flow Log log group lacks KMS encryption (ECS log groups are now encrypted)
|
||||
- Posted review comment, set status to `in_progress`, reassigned to Senior Engineer (c99c4ede)
|
||||
|
||||
### FRE-4664 — Nessa Phase 2: Community features (Second-pass review)
|
||||
- Reviewed commit bc7bf124f (Senior Engineer's fixes for 13 code review issues)
|
||||
- 12/13 fixes verified correct
|
||||
- **P1 remaining:** Error alert loops infinitely — `viewModel.error` never cleared on dismiss in ChallengesView and ClubsView
|
||||
- Assigned back to Senior Engineer with detailed fix
|
||||
- Status: in_progress
|
||||
|
||||
## 11:00 — FRE-4574 Second-Pass Review
|
||||
|
||||
- Checked out FRE-4574 for re-review of ShieldAI infra/CI-CD fixes
|
||||
- Senior Engineer fixed all 10 identified issues:
|
||||
- DNS validation, ALB subnet/SG, KMS key, HTTPS routing, task role scoping, pnpm migration, PG version, flow logs, secrets wiring, deploy workflow
|
||||
- 3 remaining issues found (P2 wget, P2 CI creds, P3 unused provider)
|
||||
- Commented with findings and assigned back to Senior Engineer ([FRE-4574](/FRE/issues/FRE-4574#comment-702e7c90-1fad-4cf1-81fc-353845a1f1d0))
|
||||
- Reviewed commit `b8c14ef8a` addressing second-pass findings
|
||||
- 3 issues remain: 1 P1 (TestFlight code signing), 2 P3 (swift-format --recursive flag, Vercel action downgrade)
|
||||
- Assigned back to Senior Engineer with detailed comments
|
||||
- [FRE-4690#comment-750c4146](/FRE/issues/FRE-4690#comment-750c4146)
|
||||
|
||||
101
agents/code-reviewer/reviews/FRE-4830-review.md
Normal file
101
agents/code-reviewer/reviews/FRE-4830-review.md
Normal file
@@ -0,0 +1,101 @@
|
||||
# Code Review: FRE-4830 - Unit Tests for Phase 3 Services
|
||||
|
||||
**Reviewer:** Code Reviewer (f274248f-c47e-4f79-98ad-45919d951aa0)
|
||||
**Engineer:** Senior Engineer (opencode_local)
|
||||
**Status:** Changes requested (P0 blocker found)
|
||||
|
||||
## Files Reviewed
|
||||
|
||||
- `iOS/Lendair/LendairTests/IdVerificationServiceTests.swift`
|
||||
- `iOS/Lendair/LendairTests/PaymentServiceTests.swift`
|
||||
- `iOS/Lendair/LendairTests/UserServiceTests.swift`
|
||||
|
||||
---
|
||||
|
||||
## P0 — Blocking: mockTRPC computed property makes authToken setup ineffective
|
||||
|
||||
**Location:** `UserServiceTests.swift:75-81` (and all test files with `mockTRPC` computed property)
|
||||
|
||||
**Problem:** `mockTRPC` is defined as a computed property:
|
||||
|
||||
```swift
|
||||
var mockTRPC: TRPCService! {
|
||||
return TRPCService(baseURL: URL(string: "http://localhost:3000")!, session: mockSession)
|
||||
}
|
||||
```
|
||||
|
||||
Every access creates a **new** `TRPCService` instance. This means:
|
||||
|
||||
1. **`setUp()` line 80 is a no-op:**
|
||||
```swift
|
||||
mockTRPC.authToken = "test-jwt-token" // creates Instance A, sets token, discards Instance A
|
||||
```
|
||||
The token is never set on the instance that tests actually use.
|
||||
|
||||
2. **All success-path tests will throw `UserError.notAuthenticated`:**
|
||||
```swift
|
||||
let service = UserService(trpc: mockTRPC) // creates Instance B (nil authToken)
|
||||
let result = try await service.updateUserProfile(input: input) // throws because authToken is nil
|
||||
```
|
||||
Since `UserService.updateUserProfile()` guards `trpc.authToken != nil`, these tests will fail.
|
||||
|
||||
3. **Same pattern in `AuthServiceTests.swift`** — any assertion reading `mockTRPC.authToken` (e.g., `XCTAssertEqual(mockTRPC.authToken, token, ...)`) checks a brand-new instance, not the one passed to `AuthService`. This bug likely exists there too.
|
||||
|
||||
**Fix:** Replace computed property with initialization in `setUp()`:
|
||||
|
||||
```swift
|
||||
var mockTRPC: TRPCService!
|
||||
var mockSession: URLSession!
|
||||
|
||||
override func setUp() {
|
||||
super.setUp()
|
||||
MockURLProtocol.responseData = nil
|
||||
MockURLProtocol.responseError = nil
|
||||
MockURLProtocol.httpStatusCode = 200
|
||||
let config = URLSessionConfiguration.test
|
||||
config.protocolClasses = [MockURLProtocol.self]
|
||||
mockSession = URLSession(configuration: config)
|
||||
mockTRPC = TRPCService(baseURL: URL(string: "http://localhost:3000")!, session: mockSession)
|
||||
mockTRPC.authToken = "test-jwt-token"
|
||||
}
|
||||
```
|
||||
|
||||
This applies to **all three test files** (IdVerificationServiceTests, PaymentServiceTests, UserServiceTests) plus AuthServiceTests if it exists.
|
||||
|
||||
---
|
||||
|
||||
## Minor: NASA 2+ Assertions Standard
|
||||
|
||||
Several tests have only 1 assertion, violating the NASA standard declared in the file headers:
|
||||
|
||||
**IdVerificationServiceTests:**
|
||||
- `testCreateSessionWithPassportType` (line 88)
|
||||
- `testCreateSessionWithNationalIdType` (line 101)
|
||||
- `testGetStatusReturnsPending` (line 162)
|
||||
- `testGetStatusReturnsNotStarted` (line 175)
|
||||
- `testGetStatusReturnsFailed` (line 188)
|
||||
|
||||
**PaymentServiceTests:**
|
||||
- `testCreateDepositIntentWithMinimumAmount` (line 89)
|
||||
|
||||
**UserServiceTests:**
|
||||
- `testUpdateUserProfileWithPhoneReturnsUpdatedUser` (line 111)
|
||||
- `testUpdateUserProfileWithNilFieldsReturnsUpdatedUser` (line 139)
|
||||
- `testUpdateProfileInputEncodesName` (line 221)
|
||||
- `testUpdateProfileInputEncodesPhone` (line 229)
|
||||
|
||||
---
|
||||
|
||||
## Minor: Inconsistency in DEBUG assertion test pattern
|
||||
|
||||
**PaymentServiceTests** uses bare `try await` for assertion tests, while **LoanServiceTests** uses `do-catch`. The fix commit's bare `try await` approach is cleaner (assert crashes via `abort()`, not throw), but inconsistent with the reference pattern. Recommend either aligning all files to one pattern or noting that this is acceptable given the semantic difference.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Finding | Action |
|
||||
|----------|---------|--------|
|
||||
| P0 | mockTRPC computed property makes authToken setup a no-op | Fix fixture initialization pattern |
|
||||
| P3 | Single-assertion tests (10 tests) | Add second assertions |
|
||||
| P3 | Inconsistent DEBUG test pattern | Align or document as intentional |
|
||||
@@ -22,3 +22,6 @@
|
||||
- Founding Engineer needs to be unpaused before they can take work again
|
||||
- `executionAgentNameKey` is immutable after creation — this is a Paperclip limitation that should be raised
|
||||
- FRE-4930: released stuck queued run, cleared executionAgentNameKey so Security Reviewer can pick it up
|
||||
- 12:52 — Woken for FRE-5103: Review productivity for FRE-4693 (Pop: Add integration tests for mail client)
|
||||
- 12:53 — Analyzed: 6h gap explained by Senior Engineer prioritizing critical work (FRE-4990) over medium-priority FRE-4693. No productivity issue.
|
||||
- 12:53 — Closed FRE-5103 as productive with analysis
|
||||
|
||||
Reference in New Issue
Block a user