CTO code review: FRE-5006 VoicePrint quality improvements — rework required
Reviewed commit a653c77 in ShieldAI repo. Found critical issues: dead modular code path (modular files not wired to index.ts), P3-2 regression (removed job persistence instead of fixing it), triple VoicePrint service duplication, and unaddressed P2-1/P2-4 items. Detailed review in plans/FRE-5006-REVIEW-FINDINGS.md. Disposition: REWORK REQUIRED — return to Junior Engineer.
This commit is contained in:
@@ -61,6 +61,20 @@
|
||||
- Updated all 8 agent AGENTS.md files with Repository Rules section
|
||||
- Git commit created for all changes
|
||||
|
||||
## Heartbeat: FRE-5006 — CTO Code Review
|
||||
|
||||
- Woken by issue_assigned for FRE-5006 (in_review, ready for review)
|
||||
- Reviewed commit `a653c77` in ShieldAI repo
|
||||
- Found critical issues:
|
||||
- **Dead modular code**: modular files not wired to index.ts — all P2/P3 fixes unreachable
|
||||
- **P3-2 regression**: removed job persistence instead of fixing it
|
||||
- **Triple duplication**: 3 VoicePrint service copies with different fix states
|
||||
- **P2-4 not addressed**: still uses `new` constructors, no DI
|
||||
- **P2-1 not addressed**: mock logic still in TS, not Python
|
||||
- **LSP errors**: modular files have type errors (schema field mismatches, missing methods)
|
||||
- Wrote detailed review to `plans/FRE-5006-REVIEW-FINDINGS.md`
|
||||
- Disposition: **REWORK REQUIRED** — return to Junior Engineer
|
||||
|
||||
## Facts
|
||||
|
||||
- ShieldAI extension code lives at /home/mike/code/ShieldAI/packages/extension/
|
||||
@@ -69,3 +83,5 @@
|
||||
- Lendair web code lives at ~/code/lendair/web/
|
||||
- Scripter code lives at ~/code/scripter/
|
||||
- Senior Engineer is overloaded: consider workload balancing
|
||||
- VoicePrint service has 3 copies across ShieldAI repo: `services/voiceprint/src/` (modular + monolithic), `packages/api/src/services/voiceprint/` (live copy)
|
||||
- The live API routes import from `packages/api/src/services/voiceprint/` — that copy received zero fixes in FRE-5006
|
||||
|
||||
51
plans/FRE-5006-REVIEW-FINDINGS.md
Normal file
51
plans/FRE-5006-REVIEW-FINDINGS.md
Normal file
@@ -0,0 +1,51 @@
|
||||
# FRE-5006 Code Review: CTO Findings
|
||||
|
||||
Commit: `a653c77959a8291f92209f1d002655fb00025f59`
|
||||
|
||||
## Disposition: REWORK REQUIRED
|
||||
|
||||
## Summary
|
||||
|
||||
Three copies of the VoicePrint service exist, each with different fix status. The modular files received most P2/P3 fixes but are **dead code** — the actual API routes import from `packages/api/src/services/voiceprint/` which received **zero** P2/P3 fixes (only P2-2 hashes were applied to one of the three copies).
|
||||
|
||||
## Per-Item Assessment
|
||||
|
||||
### P2-1 Mock ML consolidation — NOT FIXED
|
||||
Mock logic reorganized within TypeScript but never moved to canonical Python source. Still duplicated across modular and monolithic copies.
|
||||
|
||||
### P2-2 Weak hashes — FIXED (partially)
|
||||
SHA-256 applied to `services/voiceprint/src/voiceprint.service.ts` and modular files. **Not applied** to the live copy at `packages/api/src/services/voiceprint/voiceprint.service.ts`.
|
||||
|
||||
### P2-3 Parallel batch — PARTIAL
|
||||
Modular file uses `Promise.allSettled()` with chunked concurrency (not true semaphore pattern). Live copy still uses sequential `for...of` loop.
|
||||
|
||||
### P2-4 DI pattern — NOT FIXED
|
||||
All three copies still use `new` constructors internally. No DI pattern introduced.
|
||||
|
||||
### P2-5 Structured logging — PARTIAL
|
||||
`logger.ts` created but not used by live code path. Live copy still uses `console.log`/`console.error`.
|
||||
|
||||
### P3-2 Batch jobId persistence — REGRESSION
|
||||
The modular `BatchAnalysisService.ts` **removed** the `prisma.analysisJob.create()` call. jobId is now an unpersisted synthetic string.
|
||||
|
||||
## Additional Issues
|
||||
|
||||
1. **Dead modular code** — `services/voiceprint/src/index.ts` exports from monolithic file, not modular files. All modular fixes unreachable.
|
||||
2. **Triple duplication** — Three copies: modular, monolithic services/, monolithic packages/api/. Each diverging.
|
||||
3. **Unused imports** — `uuid` import and `maxRetries`/`retryDelay` fields in `EmbeddingService.ts`
|
||||
4. **Field mapping bug** — `VoiceEnrollmentService.ts:41`: `embeddingDim: preprocessed.sampleRate` (assigns sample rate to dim)
|
||||
5. **Fragile time-window query** — `getBatchResult` uses hardcoded `+60000ms` window
|
||||
|
||||
## Required Actions
|
||||
|
||||
1. Consolidate to single canonical VoicePrint service copy
|
||||
2. Wire `index.ts` to export from modular files
|
||||
3. Port all fixes to the live API copy
|
||||
4. Fix P3-2 regression (restore `analysisJob.create`)
|
||||
5. Remove unused imports/dead code
|
||||
6. Fix `embeddingDim` data mapping bug
|
||||
7. Replace chunked concurrency with proper semaphore pattern
|
||||
|
||||
## Suggested Reassign
|
||||
|
||||
Return to Junior Engineer for rework with the above checklist.
|
||||
Reference in New Issue
Block a user