diff --git a/.gitignore b/.gitignore index 52dc9c3..e1f2d73 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,6 @@ coverage/ .dolt/ *.db .beads-credential-key + +# Agent context file — role-specific, not shared +CLAUDE.md diff --git a/feedback.md b/feedback.md new file mode 100644 index 0000000..eacd74b --- /dev/null +++ b/feedback.md @@ -0,0 +1,79 @@ +# fleet-e2e-toy Phase 1 - Code Review + +**Reviewer:** reviewer +**Date:** 2026-05-27 12:00:00+00:00 +**Verdict:** APPROVED + +> First review of Phase 1. No prior feedback history. + +--- + +## Build, Lint, and Test + +- `npm run build`: PASS — TypeScript compiles cleanly, no errors. +- `npm run lint`: PASS — no lint warnings or errors. +- `npm test`: PASS — 24/24 tests across 2 suites (validation.test.ts, notes.test.ts). No regressions from the 21 pre-existing tests. + +## Task 1: Add CLAUDE.md to .gitignore + +PASS. `CLAUDE.md` appended to `.gitignore` with a descriptive comment. The doer correctly noted that since CLAUDE.md is already tracked, `git check-ignore` returns non-zero for tracked files — the `.gitignore` entry prevents future accidental adds, which satisfies the intent. + +## Task 2: isBlankOrEmpty validation helper (gh-toy-v6z) + +PASS. `isBlankOrEmpty(s: string): boolean` exported from `src/utils/validation.ts`. Implementation is clean: `s.trim().length === 0`. Three test cases added covering empty string, whitespace-only, and valid non-blank string — matches the plan exactly. All tests pass. + +NOTE: The existing inline checks (`obj.title.trim().length === 0` at validation.ts:23 and :60) were not refactored to use the new helper. This is consistent with the plan's risk register ("does not remove or change existing inline validation") and acceptable — no code change was requested beyond adding the helper. + +## Task 3: --version flag (gh-toy-4ef) + +PASS. In `src/index.ts`, `process.argv.slice(2)` is checked for `--version` or `-v` before any Express setup. Outputs `fleet-e2e-toy v1.0.0` and exits 0. Manually verified: + +- `npx ts-node src/index.ts --version` → `fleet-e2e-toy v1.0.0`, exit 0 ✓ +- `npx ts-node src/index.ts -v` → `fleet-e2e-toy v1.0.0`, exit 0 ✓ + +## Task 4: Help command and --help flag (gh-toy-kbk) + +PASS. Checks for `help`, `--help`, or `-h`. Prints well-formatted usage text listing all subcommands (help), flags (--help/-h, --version/-v), and default behavior (server start). Exits 0. Manually verified: + +- `npx ts-node src/index.ts --help` → usage text, exit 0 ✓ +- `npx ts-node src/index.ts -h` → usage text, exit 0 ✓ +- `npx ts-node src/index.ts help` → usage text, exit 0 ✓ + +## Code Quality + +PASS. The implementation is minimal and correct: + +- Arg parsing uses `process.argv.slice(2)` as planned, avoiding argv[0]/argv[1] pitfalls. +- Version check runs before help check, so `--version` takes precedence — reasonable ordering. +- `process.exit(0)` calls are isolated to `src/index.ts` and don't affect the Jest test environment (index.ts is excluded from test coverage per jest.config.ts). +- No `console.log` in route handlers, no `any` types, no security concerns. + +## Test Quality + +PASS. Three new tests for `isBlankOrEmpty` cover the meaningful cases for a simple trim-and-check function. No redundant tests. CLI flags are verified manually rather than via Jest, which is the correct approach given `process.exit()` usage — documented in the risk register. + +## File Hygiene + +All 7 changed files justified against sprint requirements: + +| File | Justification | +|------|---------------| +| `.beads/issues.jsonl` | Sprint setup — seeds the 3 P1 issues being worked plus 4 P2 backlog items | +| `.gitignore` | Task 1 — add CLAUDE.md to ignore list | +| `plan.md` | Sprint plan replacing previous v2 plan (sprint tracking) | +| `progress.json` | New file — sprint progress tracking | +| `src/index.ts` | Tasks 3 & 4 — --version and help flags | +| `src/utils/validation.ts` | Task 2 — isBlankOrEmpty helper | +| `tests/validation.test.ts` | Task 2 — isBlankOrEmpty unit tests | + +No unexpected files. No secrets, no documentation drift, no extraneous changes. + +## CI + +The CI workflow triggers on `main` and `feature/**` branches only. The sprint branch (`e2e-s1.3-26546133857/sprint`) does not match these patterns, so no CI runs exist. Local build/lint/test all pass cleanly, which is sufficient for this review. + +--- + +## Summary + +All four Phase 1 tasks are complete and meet their done criteria. Build, lint, and all 24 tests pass. CLI flags work correctly with proper exit codes. Code is clean, minimal, and consistent with project conventions. No regressions, no security issues, no file hygiene concerns. Phase 1 is approved. diff --git a/src/index.ts b/src/index.ts index 9e7603c..24b749b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,27 @@ import app from "./app"; +const args = process.argv.slice(2); + +if (args.includes("--version") || args.includes("-v")) { + console.log("fleet-e2e-toy v1.0.0"); + process.exit(0); +} + +if (args.includes("help") || args.includes("--help") || args.includes("-h")) { + console.log(`Usage: ts-node src/index.ts [command|flag] + +Commands: + help Show this help message + +Flags: + --help, -h Show this help message + --version, -v Print version and exit + +Default: + (no args) Start the NoteAPI server on port 3000`); + process.exit(0); +} + const PORT = process.env.PORT ?? 3000; app.listen(PORT, () => { diff --git a/src/utils/validation.ts b/src/utils/validation.ts index 9cfb289..09f2e95 100644 --- a/src/utils/validation.ts +++ b/src/utils/validation.ts @@ -1,5 +1,9 @@ import { CreateNoteInput, UpdateNoteInput } from "../models/note"; +export function isBlankOrEmpty(s: string): boolean { + return s.trim().length === 0; +} + export interface ValidationError { field: string; message: string; diff --git a/tests/validation.test.ts b/tests/validation.test.ts index f55f70f..3a1cdf3 100644 --- a/tests/validation.test.ts +++ b/tests/validation.test.ts @@ -1,4 +1,18 @@ -import { validateCreateInput, validateUpdateInput } from "../src/utils/validation"; +import { validateCreateInput, validateUpdateInput, isBlankOrEmpty } from "../src/utils/validation"; + +describe("isBlankOrEmpty", () => { + it("returns true for empty string", () => { + expect(isBlankOrEmpty("")).toBe(true); + }); + + it("returns true for whitespace-only string", () => { + expect(isBlankOrEmpty(" ")).toBe(true); + }); + + it("returns false for valid non-blank string", () => { + expect(isBlankOrEmpty("hello")).toBe(false); + }); +}); describe("validateCreateInput", () => { it("accepts valid input with all fields", () => {