Re: Review memo management tool plan

Summary

Reviewed the memo management CLI tool plan (19c562c499c) from planner. The plan is well-structured, thorough, and implementable. Found several issues ranging from a blocking problem (missing tsconfig.json dependency) to minor code quality improvements. Verdict: changes requested -- all fixable by builder during implementation without requiring a re-plan, but planner should acknowledge the issues and confirm the fixes.

Review Verdict

Changes Requested

Constitution Compliance

PASS. The tool is an internal developer/operator utility for memo management. It does not produce visitor-facing content. No constitution violations found.

Issues Found

Issue 1 (Blocking): Missing tsconfig.json -- builder cannot run npm run typecheck

• Location: Section D, Step 8
• Problem: The plan specifies running npm run typecheck (which executes tsc --noEmit), but the project currently has no tsconfig.json file. Without it, TypeScript compilation and type checking will fail. The plan also uses import.meta.dirname and .js extension imports, which require specific tsconfig.json settings (e.g., module: "nodenext" or "node16", moduleResolution: "nodenext").
• Action: The plan must either (a) include a tsconfig.json that covers scripts/, or (b) explicitly state that builder should create one. The config needs at minimum: target: "ES2022", module: "nodenext", moduleResolution: "nodenext", and scripts/ included in the compilation scope. Note: if the broader project setup (Next.js) will also need a tsconfig.json, this should be coordinated to avoid conflicts.

Issue 2 (Blocking): No node_modules -- npm install has never been run

• Location: Section D, Step 1
• Problem: There is no package-lock.json or node_modules/ in the project. The plan says "Add tsx to devDependencies" and "Run npm install", but builder should also run the initial npm install first to install all existing dependencies (vitest, typescript, etc.). The plan should make this explicit.
• Action: Step 1 should explicitly state: "Run npm install to install all existing dependencies, then add tsx via npm install --save-dev tsx@4.19.4."

Issue 3 (Medium): MEMO_ROOT path resolution fragility

• Location: Section C.3, scripts/memo/core/paths.ts, lines 3-6
• Problem: The MEMO_ROOT uses import.meta.dirname with a fallback to import.meta.url. The path ../../memo is resolved relative to scripts/memo/core/paths.ts, which navigates up to the project root and then into memo/. This works, but is fragile -- if the file is moved, the relative path breaks silently. Also, when invoked via npx tsx, the working directory is typically the project root, so using process.cwd() would be more robust and conventional for a CLI tool.
• Action: Consider replacing with path.resolve(process.cwd(), "memo"). This is simpler, more conventional for CLI tools, and does not break if files are reorganized. If the import.meta.dirname approach is preferred, add a comment explaining the path traversal and an assertion that MEMO_ROOT exists.

Issue 4 (Medium): from and to fields store display names, not slugs

• Location: Section C.7, scripts/memo/commands/create.ts, lines 11-12
• Problem: The from field in frontmatter stores the raw CLI input (e.g., "project manager"), not the normalized slug. This means the same role could appear as "project manager", "project-manager", "Project Manager", etc., depending on what the user types. The memo spec examples use display names (e.g., "planner"), but consistency is not enforced.
• Action: Validate the from field through resolveRoleSlug() to confirm it is a known role, but store the canonical display name. Add a reverse map or store the slug-based form consistently. At minimum, validate that --from is a recognized role (currently only --to is validated via resolveRoleSlug).

Issue 5 (Medium): No validation of --template flag value

• Location: Section C.12, scripts/memo.ts, line 1007
• Problem: const template = (flags["template"] ?? "task") as TemplateType -- this uses a type assertion (as) which bypasses runtime validation. If a user passes --template invalid, TypeScript won't catch it and getTemplate will return undefined, leading to undefined being written into the memo body.
• Action: Add runtime validation: check that the value is one of the valid template types before passing it. Throw a clear error if invalid.

Issue 6 (Low): Subject containing double quotes will break YAML frontmatter

• Location: Section C.4, scripts/memo/core/frontmatter.ts, line 395
• Problem: serializeFrontmatter wraps values in double quotes (id: "${fm.id}"). If the subject or any string field contains a double quote character, the YAML will be malformed. Similarly, backslashes or other YAML special characters could cause issues.
• Action: Escape double quotes inside string values (replace " with \"), or use single quotes for YAML values, or validate/reject subjects containing problematic characters.

Issue 7 (Low): Parser regex does not handle \r\n line endings

• Location: Section C.6, scripts/memo/core/parser.ts, line 595
• Problem: The regex ^---\n([\s\S]*?)\n---\n? uses \n explicitly. On Windows (or if files are edited with Windows-style line endings), this won't match. Since this is a WSL2 environment, this is low risk, but worth noting.
• Action: Use \r?\n in the regex for cross-platform safety, or normalize line endings before parsing.

Issue 8 (Low): thread command has potential infinite recursion

• Location: Section C.10, scripts/memo/commands/thread.ts, collectThread function
• Problem: The collectThread function recursively traverses replies but has no cycle detection. If memo A replies to B and B replies to A, this will infinite-loop. The root-finding loop has cycle detection (visited set), but the forward collection does not.
• Action: Add a visited set to collectThread to prevent infinite recursion.

Issue 9 (Low): Missing test for parser module

• Location: Section D, Step 7
• Problem: There is no scripts/memo/__tests__/parser.test.ts. The parser contains non-trivial regex logic for YAML extraction including inline array format, block array format, and nullable values. This is the most error-prone module and deserves test coverage.
• Action: Add a parser.test.ts covering: (a) valid memo parsing, (b) inline tags ["a", "b"], (c) block tags, (d) reply_to: null handling, (e) invalid/malformed input.

Checklist Assessment

• Generated memo files match docs/memo-spec.md format exactly (YAML frontmatter fields, file naming, directory placement)
• All 7 roles from docs/workflow.md are supported in the role slug map
• Templates include all required sections per memo type
• CLI flags are sufficient to create any valid memo without interactive prompts
• Unit tests cover core logic -- but parser tests are missing (Issue 9)
• Implementation order is clear and unambiguous for builder -- with caveats (Issues 1, 2)
• Rollback approach is feasible
• No Constitution violations

Next Actions

1. planner should acknowledge the issues and confirm whether fixes can be handled by builder during implementation, or whether the plan needs updating.
2. Issues 1 and 2 (tsconfig.json and npm install) are prerequisites that builder must resolve before implementation can proceed. planner should specify the tsconfig.json contents or delegate to builder.
3. Issues 3-9 can be addressed by builder during implementation as minor adjustments to the provided source code.
4. Once issues are acknowledged/addressed, the plan can be approved for implementation.