x

.agents/reviewer_m2_2_v4/BRIEFING.md

@@ -0,0 +1,61 @@
+# BRIEFING — 2026-06-29T17:38:00-04:00
+
+## Mission
+Review the Milestone 2 Backend & DB changes for correctness, robustness, type-safety, and schema conformance.
+
+## 🔒 My Identity
+- Archetype: reviewer / critic
+- Roles: reviewer, critic
+- Working directory: /home/ti/git/homaroma/.agents/reviewer_m2_2_v4
+- Original parent: 8e727cf6-d863-41ec-9bd2-f8dead86db2a
+- Milestone: Milestone 2
+- Instance: 1 of 1
+
+## 🔒 Key Constraints
+- Review-only — do NOT modify implementation code.
+- Report all findings back to parent agent.
+
+## Current Parent
+- Conversation ID: 8e727cf6-d863-41ec-9bd2-f8dead86db2a
+- Updated: 2026-06-29T17:38:00-04:00
+
+## Review Scope
+- **Files to review**:
+  - `apps/backend/prisma/schema.prisma`
+  - `packages/types/index.ts`
+  - `apps/backend/src/routes/menus.ts`
+  - `apps/mcp-server/menu-tools.ts`
+  - `apps/mcp-server/menu-toolkit.ts`
+- **Interface contracts**: `PROJECT.md` / `SCOPE.md` if available.
+- **Review criteria**: Correctness, robustness, type-safety, schema conformance.
+
+## Review Checklist
+- **Items reviewed**:
+  - `apps/backend/prisma/schema.prisma`
+  - `packages/types/index.ts`
+  - `apps/backend/src/routes/menus.ts`
+  - `apps/mcp-server/menu-tools.ts`
+  - `apps/mcp-server/menu-toolkit.ts`
+- **Verdict**: fail (with integrity violation)
+- **Unverified claims**: None.
+
+## Attack Surface
+- **Hypotheses tested**:
+  - Verification test assertions
+  - String-to-object conversion behavior in menu-tools
+  - Typescript compilation
+- **Vulnerabilities found**:
+  - Test result masking (integrity violation)
+  - String spread object corruption
+  - Thai translation loss on merge
+  - 189 TypeScript compiler errors in backend
+  - Missing fields/interfaces in shared types
+  - Non-atomic updates in menus route
+- **Untested angles**: None.
+
+## Key Decisions Made
+- Finalized review of files and issued FAIL/REQUEST_CHANGES verdict.
+
+## Artifact Index
+- `/home/ti/git/homaroma/.agents/reviewer_m2_2_v4/review.md` — Final review report
+- `/home/ti/git/homaroma/.agents/reviewer_m2_2_v4/handoff.md` — Handoff report

.agents/reviewer_m2_2_v4/ORIGINAL_REQUEST.md

@@ -0,0 +1,13 @@
+## 2026-06-29T17:35:22-04:00
+
+You are Reviewer 2 (v4) for Milestone 2: Backend & DB.
+Your working directory is /home/ti/git/homaroma/.agents/reviewer_m2_2_v4.
+Please review the changes implemented by the Worker in M2 for correctness, robustness, type-safety, and schema conformance:
+- Prisma schema (`apps/backend/prisma/schema.prisma`)
+- Shared types (`packages/types/index.ts`)
+- Backend routes (`apps/backend/src/routes/menus.ts`)
+- MCP Server tools (`apps/mcp-server/menu-tools.ts` and `apps/mcp-server/menu-toolkit.ts`)
+
+Look for potential bugs, unmapped fields in transactions, Elysia TypeBox schema mismatches, and TypeScript type-safety issues.
+Write your findings and review verdict (PASS/FAIL) in `/home/ti/git/homaroma/.agents/reviewer_m2_2_v4/review.md`.
+Once finished, send a message back with your verdict and a summary.

.agents/reviewer_m2_2_v4/handoff.md

@@ -0,0 +1,81 @@
+# Handoff Report: Review of Milestone 2 Backend & DB
+
+## 1. Observation
+I have examined the codebase, run tests, and observed the following:
+
+- **TypeScript Compiler Output on `apps/backend`**:
+  ```
+  src/routes/menus.ts(2,20): error TS2835: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../lib/prisma.js'?
+  src/routes/menus.ts(32,23): error TS7031: Binding element 'body' implicitly has an 'any' type.
+  ```
+- **Test execution log in `test_menu_comprehensive.ts`**:
+  ```
+  3.3 Verify MCP Tool Validation Errors for Incorrect Types...
+  - Testing MCP Tool: Invalid items parameter type (not an array)
+    Caught expected validation error: Expected MCP Tool to throw validation error, but it succeeded.
+  ```
+- **In `apps/mcp-server/menu-tools.ts`**:
+  - Line 281-283:
+    ```typescript
+    if (typeof nextNode[labelKey] === "string") {
+      nextNode[labelKey] = { en: nextNode[labelKey] };
+    }
+    ```
+  - Line 307:
+    ```typescript
+    found.node[labelKey] = { ...(found.node[labelKey] || {}), ...translations };
+    ```
+- **In `apps/backend/src/routes/menus.ts`**:
+  - Lines 97-101 (Outside transaction):
+    ```typescript
+    if (!menu) {
+      menu = await prisma.menu.create({ data: { name, ...updateData } });
+    } else if (Object.keys(updateData).length > 0) {
+      menu = await prisma.menu.update({ where: { name }, data: updateData });
+    }
+    ```
+  - Lines 103-153 (Inside transaction):
+    ```typescript
+    await prisma.$transaction(async (tx: any) => { ...
+    ```
+- **In `packages/types/index.ts`**:
+  - Contains `export interface Menu` and `export interface MenuItem` but they omit `isActive`, `menuId`, `createdAt`, `updatedAt`, and do not declare a `MenuItemTranslation` type.
+
+---
+
+## 2. Logic Chain
+
+1. **Test Verification Integrity**: In the test log, it states `Caught expected validation error: Expected MCP Tool to throw validation error, but it succeeded.` This message is printed because `handleMenuTool` returned success (did not throw), causing the test code to manually throw an error with that text, which is caught and printed as a success validation step. This shows the tool successfully accepted the invalid parameter and did not reject it.
+2. **String Spread Bug**: In JavaScript/TypeScript, spreading a string (e.g., `{ ..."หน้าแรก" }`) yields character-indexed object keys (e.g., `{ 0: "ห", 1: "น", ... }`). In `menu-tools.ts`, line 307 spreads `found.node[labelKey]`. If the label is a string, it splits the string into characters, corrupting the menu payload in PostgreSQL.
+3. **Locale Loss**: In `menu-tools.ts`, converting a string label to an object wraps it as `{ en: label }`. If we then merge `translations: { en: "Home" }`, the English translation overwrites the English field containing the Thai label, causing the Thai string to be completely lost.
+4. **Build and Compiler Failures**: The backend `"module": "nodenext"` target strictly enforces ESM extensions. Omitting `.js` on local imports prevents backend compilation. Chaining `.use(adminOnly)` breaks Elysia parameter typing, which causes compiler errors for implicit `any` in strict mode.
+5. **Atomicity issues**: Updating the `Menu` model outside the prisma transaction means database writes are partially committed if the subsequent transaction containing items and translation updates fails.
+
+---
+
+## 3. Caveats
+- It is assumed that the base language of the store is Thai (`th`) as indicated by other route files (e.g., `categories.ts` defaulting to `th`).
+- No changes to database migrations were evaluated, as the Prisma schema definitions for Menu and MenuItem items correctly match database tables.
+
+---
+
+## 4. Conclusion
+The review verdict is a definitive **FAIL / REQUEST_CHANGES**. The implementation fails to meet correctness, robustness, type-safety, and schema conformance criteria, and includes a testing verification bypass (Integrity Violation). The code must be corrected by the Worker before Milestone 2 can be approved.
+
+---
+
+## 5. Verification Method
+1. Run backend type checking:
+   ```bash
+   cd apps/backend && bunx tsc -p tsconfig.json --noEmit
+   ```
+2. Verify JS spread behavior under translation update:
+   Run the following script to reproduce the character-spread bug:
+   ```bash
+   bun -e 'console.log({ ..."หน้าแรก", en: "Home" })'
+   ```
+3. Run comprehensive verification test:
+   ```bash
+   cd apps/backend && bun test_menu_comprehensive.ts
+   ```
+   Inspect the output under `3.3 Verify MCP Tool Validation Errors...` and confirm that it logs a manual error instead of catching a real tool validation exception.

.agents/reviewer_m2_2_v4/progress.md

@@ -0,0 +1,17 @@
+# Progress — 2026-06-29T17:39:00-04:00
+
+Last visited: 2026-06-29T17:39:00-04:00
+
+## Done
+- Initialized BRIEFING.md, ORIGINAL_REQUEST.md, and progress.md.
+- Reviewed target files for changes in M2.
+- Verified build and tests on target files (reproduced type checking failures and test assertion masking).
+- Documented review findings in review.md (Verdict: FAIL / REQUEST_CHANGES).
+- Documented handoff report in handoff.md.
+- Updated briefing with vulnerabilities and check results.
+
+## In Progress
+- Sending message back to parent.
+
+## To Do
+- Done!

.agents/reviewer_m2_2_v4/review.md

@@ -0,0 +1,97 @@
+# Review Report: Milestone 2 Backend & DB
+
+**Verdict**: **FAIL / REQUEST_CHANGES** (with Critical **INTEGRITY VIOLATION** finding)
+
+---
+
+## 1. Executive Summary
+
+Milestone 2 changes implemented by the Worker have been reviewed for correctness, robustness, type-safety, and schema conformance. While the base functionality runs under some conditions, the review has identified **multiple critical design flaws, type-safety failures, data corruption bugs, and a severe testing integrity violation**. 
+
+Key issues include:
+- **Integrity Violation**: The comprehensive verification test suite (`test_menu_comprehensive.ts`) masks a validation bypass bug by manually throwing and catching its own error, asserting success ("ALL TESTS COMPLETED SUCCESSFULLY!") when the code actually failed to throw.
+- **Data Corruption**: A critical bug in translation merging (`apps/mcp-server/menu-tools.ts`) spreads string labels into indexed character objects (e.g., `{ "0": "ห", "1": "น", ... }`) when updating translations, corrupting the payload.
+- **Translation Loss**: Merging a string label maps it to the `en` key instead of `th`, completely overwriting and discarding the base Thai translation.
+- **TypeScript Compilation Block**: The backend fails to compile, producing 189 TypeScript errors due to missing file extensions in ESM imports and implicit `any` type inferences on Elysia handlers.
+- **Prisma Schema Mismatch**: Important fields (`isActive`, `menuId`, timestamps) and models (`MenuItemTranslation`) are missing or loosely typed as `any[]` in the shared types (`packages/types/index.ts`).
+- **Atomicity Violation**: The menu record is updated outside the database transaction, risking data inconsistency if item updates fail.
+
+---
+
+## 2. Findings
+
+### 🔴 Critical Finding 1: Integrity Violation (Masked Test Failures)
+- **What**: The test script `apps/backend/test_menu_comprehensive.ts` masks a validation failure in `upsert_menu`.
+- **Where**: `apps/backend/test_menu_comprehensive.ts` (lines 364-370)
+- **Why**: When testing invalid non-array input (`items: "not-an-array"`), the MCP tool fails to throw an error (it silently coerces it to `[]`). The test catches this success, manually throws `Expected MCP Tool to throw validation error, but it succeeded`, and catches *its own* error in the catch block to log it as `Caught expected validation error`. This allows the script to exit with 0 and print `ALL TESTS COMPLETED SUCCESSFULLY!`, fabricating a false verification output.
+- **Suggestion**: The tool must properly validate that `items` is an array and throw an actual error if not, and the test must fail if the tool succeeds on invalid input.
+
+### 🔴 Critical Finding 2: Data Corruption on Translation Update (String Spread Bug)
+- **What**: Updating translations on a node that has a string label corrupts it into an object of characters.
+- **Where**: `apps/mcp-server/menu-tools.ts` (line 307)
+- **Code**: `found.node[labelKey] = { ...(found.node[labelKey] || {}), ...translations };`
+- **Why**: If `found.node.label` is a string (e.g., `"หน้าแรก"`), spreading it inside an object splits it into indexed character properties (e.g., `{ '0': 'ห', '1': 'น', '2': '้', '3': 'า', '4': 'แ', '5': 'ร', '6': 'ก', en: 'Home' }`). This corrupts the label field.
+- **Suggestion**: Check if `found.node[labelKey]` is a string first, convert it to an object mapping to the default locale (e.g., `{ th: found.node[labelKey] }`), and then merge the translations.
+
+### 🔴 Critical Finding 3: Translation Loss / Default Language Override
+- **What**: String labels are incorrectly mapped to the `en` key instead of the default `th` key when translations are present, losing the base translation.
+- **Where**: `apps/mcp-server/menu-tools.ts` (lines 281-283)
+- **Code**: 
+  ```typescript
+  if (typeof nextNode[labelKey] === "string") {
+    nextNode[labelKey] = { en: nextNode[labelKey] };
+  }
+  ```
+- **Why**: Since the default language of the store is Thai, a string label like `"สปา"` should be treated as `th`. Mapping it to `en` and then merging `translations: { en: "Spa" }` overwrites `"สปา"` with `"Spa"`, completely losing the Thai text.
+- **Suggestion**: Map it to `th` (`nextNode[labelKey] = { th: nextNode[labelKey] }`) to preserve the default locale.
+
+### 🟡 Major Finding 4: TypeScript Compilation Failures (Build Block)
+- **What**: Backend fails to compile with 189 TypeScript errors.
+- **Where**: `apps/backend/src/routes/menus.ts` (and other routes)
+- **Why**:
+  1. **ESM Import Extensions**: Under `"module": "nodenext"`, imports must have explicit `.js` extensions (e.g. `import prisma from "../lib/prisma.js"`). The Worker omitted these extensions.
+  2. **Implicit Any on Elysia Handlers**: Parameters `body`, `set`, and `params` in routes that use `.use(adminOnly)` are inferred as implicit `any` because Elysia's type chaining breaks context typing. This violates `"strict": true` type checking.
+- **Suggestion**: Fix import paths to include `.js` extensions and add explicit types to Elysia handler parameters (e.g., `: { body: any; set: any }`) where inference fails.
+
+### 🟡 Major Finding 5: Shared Types Mismatches & Omissions
+- **What**: Shared types fail to conform to the database schema.
+- **Where**: `packages/types/index.ts`
+- **Why**:
+  - `Menu` interface lacks `isActive` (boolean), `createdAt`, and `updatedAt` properties.
+  - `MenuItem` interface lacks `menuId`, `createdAt`, and `updatedAt` properties.
+  - `MenuItemTranslation` interface is missing entirely; `MenuItem.translations` is loosely typed as `any[]`.
+- **Suggestion**: Synchronize TypeScript interfaces with the Prisma schema and create a `MenuItemTranslation` interface.
+
+### 🟡 Major Finding 6: Transaction Atomicity Violation
+- **What**: Menu metadata is updated outside of the database transaction block.
+- **Where**: `apps/backend/src/routes/menus.ts` (lines 97-101)
+- **Why**: `prisma.menu.update` or `prisma.menu.create` is executed before starting `prisma.$transaction`. If the transaction containing item insertions or updates fails, the menu metadata updates (like `rootImage` or `payload`) remain committed, breaking database consistency.
+- **Suggestion**: Perform all creations, updates, and item syncs inside the *same* `prisma.$transaction` block.
+
+### 🟢 Minor Finding 7: Unvalidated Elysia Body Schema
+- **What**: The `PATCH /items/:id/translations` route does not validate the request body.
+- **Where**: `apps/backend/src/routes/menus.ts` (lines 183-228)
+- **Why**: It lacks a TypeBox `body` validation schema, relying on `const payload = body as any`.
+- **Suggestion**: Add a TypeBox validation schema to ensure label/translation payload is correct.
+
+---
+
+## 3. Verified Claims
+
+- **Database Unique Constraint**: Verified that trying to POST a menu that already exists correctly throws a unique constraint error (P2002) returning a `500` error status (Verified via `test_menu_api_challenger.ts`).
+- **MCP Tool Execution**: Verified that `upsert_menu` and `get_menu` successfully interact with the backend API when valid payloads are supplied (Verified via comprehensive test runs).
+
+---
+
+## 4. Stress Test Results
+
+- **Input**: `items: "not-an-array"` sent to `upsert_menu` tool.
+- **Expected Behavior**: Tool rejects the input and throws a validation error.
+- **Actual Behavior**: Tool silently coerces the input to `[]` and returns success, which is then masked by the test assertions.
+- **Status**: **FAIL**
+
+---
+
+## 5. Coverage Gaps
+
+- **Prisma Cascade Behavior**: Did not verify direct deletions of menus with large sub-hierarchies, but the schema definition uses `onDelete: Cascade` correctly.