From e599a16d5fc97eeea440ee3d4ed5ede6d1c221fb Mon Sep 17 00:00:00 2001 From: _david Date: Mon, 28 Sep 2026 00:45:23 +0700 Subject: [PATCH] test(services): add regression coverage for POST .../create returning real _id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #157. The production fix was already live on `staging` since commit f355e2f (2026-08-21) — `baseCreateDocument`'s hookAfterSave result was already propagated correctly — but no regression test ever covered it and the diagram node was never backfilled. - src/__tests__/services/baseCreateDocument.test.ts: direct unit coverage of the hookAfterSave replacement propagation. - src/__tests__/candidate_profile/BaseService.test.ts: spot-checks a real CV section create flow through the unmocked createCrudService/BaseService.ts/services/index.ts code path (only the Mongoose model is faked) — confirms the response's _id is the real persisted id, not null. - agent-hub: backfill evidence (implementer + verifier) and seal node fix-create-response-null-id. npm test: 140 passed, 140 total. npm run build: clean. Node: fix-create-response-null-id (SEALED) Evidence: agent-hub/evidence/implementer/2026-09-28/fix-create-response-null-id-plan.md, agent-hub/evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md Co-Authored-By: Claude Sonnet 5 --- .../fix-create-response-null-id-plan.md | 147 ++++++++++++++++++ .../fix-create-response-null-id-seal.md | 85 ++++++++++ .../haven/diagrams/dev-loop.prime-mermaid.md | 2 +- .../candidate_profile/BaseService.test.ts | 42 +++++ .../services/baseCreateDocument.test.ts | 64 ++++++++ 5 files changed, 339 insertions(+), 1 deletion(-) create mode 100644 agent-hub/evidence/implementer/2026-09-28/fix-create-response-null-id-plan.md create mode 100644 agent-hub/evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md create mode 100644 src/__tests__/candidate_profile/BaseService.test.ts create mode 100644 src/__tests__/services/baseCreateDocument.test.ts diff --git a/agent-hub/evidence/implementer/2026-09-28/fix-create-response-null-id-plan.md b/agent-hub/evidence/implementer/2026-09-28/fix-create-response-null-id-plan.md new file mode 100644 index 0000000..133ac3f --- /dev/null +++ b/agent-hub/evidence/implementer/2026-09-28/fix-create-response-null-id-plan.md @@ -0,0 +1,147 @@ +# 2026-09-28 — fix-create-response-null-id (plan + diff) + +- Worker: implementer +- Version: 0.1.0 +- Node: `fix-create-response-null-id` (`haven/diagrams/dev-loop.prime-mermaid.md`) +- Issue: [#157](https://github.com/datvt243/resume-nodejs-api/issues/157) — POST .../create responses always return `data._id: null` +- Branch: `157-post-create-responses` (base `staging`) +- Task (verbatim): "Fix issue #157 — POST .../create responses always return data._id: null. Root cause: src/candidate_profile/BaseService.ts handlerCreate's hookAfterSave reassigns the local destructured `data` variable, but that reassignment never propagates back through baseCreateDocument's return value. Fix so the caller (and API response) gets the real saved document including its real _id. Acceptance criteria: (1) a regression test on handlerCreate/baseCreateDocument asserts the returned _id matches the actually-persisted document's _id, not null; (2) spot-check at least one real CV section create endpoint end-to-end." + +## Hub bytes before: 86675 + +## Node lookup + +Matched the existing PENDING node `fix-create-response-null-id` directly +(task came from resolving GitHub issue #157, filed against this exact +node). + +## Bookkeeping-gap finding (read before writing anything) + +The production fix already exists on `staging` — same pattern as +`add-pagination-filtering-cv-sections`/#73, `add-logout-all-sessions`/#74, +and `fix-idor-broken-access-control`. `git log -- src/services/index.ts` +shows commit `f355e2f` ("fix: close broken access control, password leak, +and 3 other API bugs", 2026-08-21) already contains: + +```diff ++ /** ++ * callback thực hiện sau khi thêm mới thành công. Nếu hook trả về ++ * (khác undefined), dùng giá trị đó thay _data — trước đây hook nhận ++ * `data` qua destructure-by-value nên gán lại bên trong hook không hề ++ * cập nhật _data ở đây, khiến response luôn trả nguyên kết quả thô của ++ * MODEL.create() ... thay vì list mới đã refetch. ++ */ + if (props?.hookAfterSave) { +- await props?.hookAfterSave?.(document, { success: _success, message: _message, data: _data }); ++ const replacement = await props.hookAfterSave(document, { success: _success, message: _message, data: _data }); ++ if (replacement !== undefined) _data = replacement; + } +``` +(`src/services/index.ts` `baseCreateDocument`, confirmed live on this +branch at lines 300-303 today.) + +That same commit bundled 5 fixes into one, but only +`fix-candidate-password-leak` got its own evidence note at the time +(`evidence/implementer/2026-08-21/fix-candidate-password-leak-diff.md`, +itself still sitting at `sealed_pending_verifier` — never promoted). The +other 4 (IDOR, this one, TOKEN_EXP_IN, v2-register-await) never got a +per-node evidence note. IDOR was already backfilled later +(`fix-idor-broken-access-control`, SEALED 2026-09-08). This note performs +the same backfill for `fix-create-response-null-id`, plus closes the real +gap the issue's acceptance criteria pointed at: **no regression test +existed** for this behavior anywhere in `src/__tests__/`. + +## Diff (smallest diff — no `src/` production code, only new tests) + +`src/services/index.ts` and `src/candidate_profile/BaseService.ts` are +unchanged — the fix is already correct and live on `staging`. The only +diff is 2 new test files: + +- `src/__tests__/services/baseCreateDocument.test.ts` — direct unit + coverage of `baseCreateDocument`'s `hookAfterSave` replacement + propagation (the exact root-cause line): asserts (a) a non-undefined + `hookAfterSave` return value becomes `result.data` instead of the raw + `MODEL.create()` result, (b) `undefined` falls back to the raw create + result, (c) no `hookAfterSave` at all leaves the raw result untouched. +- `src/__tests__/candidate_profile/BaseService.test.ts` — spot-check of a + real CV section's create flow: calls `createCrudService({ model, name: + 'education' }).handlerCreate(...)` with the real, unmocked + `BaseService.ts` + `services/index.ts` code (only the Mongoose model + itself is faked), confirming the final response's `data._id` is the + real persisted id (`real-id-1`), not `null` — this is the exact code + path every CV section's real `POST .../create` endpoint uses. + +## Command + +``` +npm test +``` +Output (verbatim tail): +``` +Test Suites: 26 passed, 26 total +Tests: 140 passed, 140 total +Snapshots: 0 total +Time: 7.101 s +Ran all test suites. +``` +(140 = the 136 total recorded on the last SEALED node, `update-project-docs`, ++ 3 new tests in `baseCreateDocument.test.ts` + 1 new test in +`BaseService.test.ts`. A harness warning — "A worker process has failed to +exit gracefully... Active timers can also cause this" — printed above this +summary; pre-existing, unrelated to this diff (no timer/interval touched +here), and does not affect the pass/fail count.) + +``` +npm run build +``` +Output: `tsc` clean, `cp -R ./src/views ./src/public ./dist/` (the `copy` +step) ran with no errors. + +## Acceptance + +| Criterion | Evidence | +|---|---| +| Trace to exactly one diagram node | `fix-create-response-null-id` | +| Smallest diff | 2 new test files only, 0 production `src/` changes (fix already live since `f355e2f`, 2026-08-21) | +| Regression test asserts `_id` is the real persisted id, not `null` | `src/__tests__/services/baseCreateDocument.test.ts` (root-cause level) + `src/__tests__/candidate_profile/BaseService.test.ts` (CV-section-flow level) | +| Spot-check a real CV section create endpoint end-to-end | See "Live end-to-end — not performed" below; done instead as an unmocked code-path spot-check through the real `createCrudService`/`BaseService.ts`/`services/index.ts`, per `BaseService.test.ts` | +| Exact test command run + output read back | `npm test` → `Tests: 140 passed, 140 total`; `npm run build` clean | +| Evidence note written | This file | + +## Live end-to-end — not performed, said honestly + +This session's sandbox has no `.env`, no local MongoDB (`mongod` not +running, no Mongo Docker container), and the Docker daemon itself is not +reachable (`docker info` fails) — no way to actually start `npm run dev` +against a real database to curl a live `POST /api/v1/education/create`. +Rather than fabricate a live-curl transcript, the "spot-check a real CV +section create endpoint end-to-end" criterion was satisfied instead by +exercising the real, unmocked code path (`BaseService.test.ts` above) — +only the Mongoose model itself is faked, everything else (`BaseService.ts` +handlerCreate → `services/index.ts` baseCreateDocument → +hookAfterSave → baseFindDocument refetch) runs for real. Flagging this gap +honestly rather than claiming a live server round-trip that didn't happen. + +## Noticed, not done + +- `haven/diagrams/dev-loop.prime-mermaid.md` is 39901B, over the 15KB + `/hub-tokens` archive threshold (checked this session, `hub_bytes_before` + above). Not archived here — out of scope for this node, flagged for a + future dedicated archive pass. +- The sibling `fix-candidate-password-leak` node is also already fixed + live on `staging` (same `f355e2f` commit) but still shows PENDING on the + diagram with a stale `sealed_pending_verifier` note from 2026-08-21 that + never got a verifier pass — same backfill pattern as this node, own + `/todo #152` pickup if wanted (issue #152 already filed). +- `TOKEN_EXP_IN`/v2-register-await (issues #155/#156) are also already + fixed in the same bundled commit — not touched here, out of scope for + this node. + +## Seal gate + +No outward-facing action taken (no commit/push). Only local file writes: +2 new test files under `src/__tests__/`. Pending verifier. + +## Status + +`sealed_pending_verifier` diff --git a/agent-hub/evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md b/agent-hub/evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md new file mode 100644 index 0000000..a2418dd --- /dev/null +++ b/agent-hub/evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md @@ -0,0 +1,85 @@ +# 2026-09-28 — fix-create-response-null-id — verifier verdict + +- Worker: verifier (subagent, dispatched via Agent tool) +- Node: `fix-create-response-null-id` (`haven/diagrams/dev-loop.prime-mermaid.md`) +- New PM status: SEALED + +## Isolation proof + +Dispatched as a fresh Agent-tool subagent with task description +"Independent verifier pass for fix-create-response-null-id" — no +conversation history from the implementer's session, no memory of writing +the diff under review. Only source read directly (per recipe step 2's +citation-confirmation exception): `src/services/index.ts` (current + +`git show f355e2f`), the two new test files, `git status`/`git log` on +branch `157-post-create-responses`. Never opened the implementer's +session/diff itself — only its evidence note, plus these independent +citation checks. + +## Reasoning + +- **Command matches doctrine**: `npm test` — matches + `doctrine/MEMORY.md`'s Test row exactly. +- **Output not truncated**: note's `npm test` tail + (`Test Suites: 26 passed, 26 total` / `Tests: 140 passed, 140 total`) + and `npm run build` output are verbatim, no `...`/truncation markers. +- **Criterion (a)** — regression test asserts real persisted `_id`, not + null: confirmed by reading + `src/__tests__/services/baseCreateDocument.test.ts` directly — 3 tests + cover hookAfterSave replacement propagation, undefined fallback, and + no-hook passthrough; the first asserts + `expect((result.data as any)._id).toBe('real-id-123')`. +- **Criterion (b)** — spot-check a real CV section create endpoint + end-to-end: note explicitly discloses the literal live-HTTP + interpretation was NOT performed (no local Mongo/Redis/Docker in that + sandbox — a real, stated constraint, not hidden) and substitutes + `src/__tests__/candidate_profile/BaseService.test.ts`, confirmed by + direct read: calls the real, unmocked `createCrudService(...). + handlerCreate` → real `BaseService.ts` → real `services/index.ts` + `baseCreateDocument` → real `hookAfterSave` refetch path, only the + Mongoose model itself faked. This exercises the exact production code + path every `POST .../create` endpoint uses, asserts + `data._id).toBe('real-id-1')` (not null). Judged: an honest, reasonable + substitution given the disclosed sandbox limit and the fact the + underlying fix has been live and unchanged on `staging` for 5+ weeks + (see next point) — not a REOPEN-worthy gap. +- **Bookkeeping-gap claim independently confirmed**: `git show f355e2f -- + src/services/index.ts` really contains the cited hunk + (`const replacement = await props.hookAfterSave(...); if (replacement + !== undefined) _data = replacement;`), and current + `src/services/index.ts` lines 300-302 match it verbatim — the + production fix is genuinely already live, unchanged since 2026-08-21. + `BaseService.ts` confirmed unchanged (diff is test-only). +- **Proportion**: 2 new test files, 0 production `src/` changes — + proportionate; the production fix already exists, so a regression-test + diff is the smallest diff that closes the real gap (no prior test + coverage). +- **Seal gate**: `git status --short` on branch `157-post-create-responses` + shows only 3 untracked paths — the evidence note dir and the 2 new test + files — nothing staged/committed/pushed. `git log` on that branch shows + no new commit beyond the shared history with `staging`. Matches the + note's "no outward-facing action taken" claim. + +## Forbidden states scan + +- `ADHOC_WORK` — no: task traces to the pre-existing PENDING node + `fix-create-response-null-id` on the diagram (row confirmed before + edit). +- `NO_EVIDENCE` — no: implementer note exists at the cited path. +- `EDIT_UNVERIFIED` — no: `npm test` output is a real, non-truncated, + doctrine-matching command result, consistent with the described diff + (140 = prior 136 + 3 new `baseCreateDocument.test.ts` tests + 1 new + `BaseService.test.ts` test). +- `CODE_IN_HAVEN` — no: `find agent-hub/evidence/implementer/2026-09-28` + shows only the one `.md` note, no runnable code. +- `DIAGRAM_DRIFT` — resolved by this verdict: node row updated PENDING → + SEALED in place. + +## Re-run + +`none` — audit-only, per recipe default for a non-outward-facing, non- +release-gate bug/test-coverage fix. The note's command matched doctrine, +output was verbatim and covered every acceptance criterion, so no partial +or full re-run was warranted; independent checks were limited to +confirming the note's own citations (git history, current source, test +file contents) rather than regenerating its evidence. diff --git a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md index f21d40b..3d04a76 100644 --- a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md +++ b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md @@ -60,7 +60,7 @@ flowchart TD | `fix-candidate-password-leak` | PENDING | `src/candidate/candidate.service.ts` — `handlerGetInformationByEmail` has no `.select()` at all; `handlerGetInformationById` double-wraps `whitelistSelect([select])`, making the select a permanent no-op. Result: `GET /api/v1/candidate/:email` and `PUT/PATCH /candidate/update` return the raw bcrypt password hash in the response. | | `fix-refresh-token-expiry-unused` | PENDING | `TOKEN_EXP_IN` (`.env`, already exported in config) is never used at any `jwtSign()` call site (`auth.service.ts`, `auth.controller.ts`, `api/v1/auth/services/login.ts`) — access and refresh tokens always share the same default 1h expiry, so the refresh token is pointless. | | `fix-v2-register-missing-await` | PENDING | `src/api/v1/auth/services/register.ts:44` — `bcryptGenerateSalt(password)` missing `await`, the Promise gets assigned straight into the Mongoose model's password field → every `POST /api/v2/auth/register` fails with a Promise→string cast error. | -| `fix-create-response-null-id` | PENDING | Minor. `BaseService.ts` `handlerCreate`'s `hookAfterSave` reassigns the local destructured `data` variable, never actually updating what `baseCreateDocument` returns → every `POST .../create` response has `data._id: null` instead of the real new ID. | +| `fix-create-response-null-id` | SEALED | Minor. `BaseService.ts` `handlerCreate`'s `hookAfterSave` reassigns the local destructured `data` variable, never actually updating what `baseCreateDocument` returns → every `POST .../create` response has `data._id: null` instead of the real new ID. Backfill, same pattern as `fix-idor-broken-access-control`: the fix itself was already live since commit `f355e2f` (2026-08-21, `src/services/index.ts` lines ~300-302, `const replacement = await props.hookAfterSave(...); if (replacement !== undefined) _data = replacement;`) — independently confirmed via `git show f355e2f -- src/services/index.ts` and the current file, both match. This node closed the real gap: no regression test existed. 2 new test files added (`src/__tests__/services/baseCreateDocument.test.ts` — root-cause unit coverage; `src/__tests__/candidate_profile/BaseService.test.ts` — unmocked `createCrudService`/`BaseService.ts`/`services/index.ts` spot-check, only the Mongoose model faked), 0 production changes — proportionate. `npm test`: 140/140 passed. Live HTTP end-to-end was not performed (no local Mongo/Redis/Docker in the implementer's sandbox, disclosed honestly, not hidden) — accepted the unmocked code-path spot-check as a reasonable substitute given the fix has been live and stable for 5+ weeks. SEALED 2026-09-28 after independent verifier pass (audit-only, no re-run — note's `npm test` output was verbatim, not truncated, command matched `doctrine/MEMORY.md`; confirmed via `git status --short` on `157-post-create-responses` that nothing was committed/pushed). Evidence: `evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md`. | | `add-candidate-self-delete` | PENDING | Feature (not a bug). No endpoint lets a candidate delete their own account — needed to clean up 2 test accounts created during live-verification of the 5 bug fixes above on production (`livecheck+...@example.com`, `livecheckB+...@example.com`). Requirement: `DELETE /api/v1/candidate`, using only `req.user._id` (never an id from the client — follows the IDOR-safe pattern from `fix-idor-broken-access-control`), cascade-deletes data across all 7 CV section models by `candidateId`. | | `fix-candidate-me-candidateid-not-string` | PENDING | **Critical, found by accident while testing #79.** `candidate_me/index.ts` `handlerGetAboutMe` — `_id` from the raw Mongoose document is an ObjectId instance, passed straight into `idQuerySafe.safeQuery({}, { candidateId: _id })` — `QuerySafe.safeQuery` only accepts `typeof value === 'string'`, so an ObjectId silently fails that check and `candidateId` gets dropped from the filter → `model.find({})` returns CV data (education/experience/award/certificate/project/generalInformation) for **every candidate mixed together**, on every `GET /api/me/:email` request (public, no auth) and `/download-pdf`. Live-tested confirmed: a brand-new candidate profile returned real data belonging to `votan.it@gmail.com`. Fix: `.toString()` on `_id` before passing it in. | | `feat-i18n-api-messages-auth` | PENDING | Feature, GitHub issue #78 (phase 1 of several). i18n infrastructure (hand-rolled `t(key, lang)`, reads `locales/vi.json`/`en.json`, middleware detects `Accept-Language`, defaults `vi`) + fully migrates the auth flow (register/login/logout/refresh). Does NOT migrate Joi validation messages (different architecture — Joi schemas are built once at module load with no request context; needs error TYPE → i18n key mapping, left as a follow-up). Does NOT touch candidate/CV section messages (separate follow-up). | diff --git a/src/__tests__/candidate_profile/BaseService.test.ts b/src/__tests__/candidate_profile/BaseService.test.ts new file mode 100644 index 0000000..a078d7c --- /dev/null +++ b/src/__tests__/candidate_profile/BaseService.test.ts @@ -0,0 +1,42 @@ +/** + * Spot-check for issue #157 — exercises createCrudService's real + * handlerCreate (BaseService.ts) end-to-end through the real, unmocked + * services/index.ts (baseCreateDocument + its hookAfterSave refetch), the + * same code path every CV section's real POST .../create endpoint uses + * (e.g. education). Only the Mongoose model itself is faked. Confirms the + * response's data._id is the real persisted id, not null. + */ +import { createCrudService } from '@/candidate_profile/BaseService'; + +function createFakeEducationModel() { + const docs: Record[] = []; + return { + validate: jest.fn().mockResolvedValue(undefined), + create: jest.fn(async (doc: Record): Promise> => { + // Mirrors real Mongoose behavior for MODEL.create({ _id: null, ... }): + // an explicit `_id: null` is NOT kept as null — Mongoose still + // assigns a real ObjectId-shaped id. + const saved: Record = { ...doc, _id: `real-id-${docs.length + 1}` }; + docs.push(saved); + return saved; + }), + find: jest.fn((query: Record) => ({ + exec: jest.fn().mockResolvedValue(docs.filter((d) => d.candidateId === query.candidateId)), + })), + }; +} + +describe('createCrudService handlerCreate — real CV section create flow (issue #157)', () => { + it('returns the real persisted _id in data, not null, for a section like education', async () => { + const model = createFakeEducationModel(); + const { handlerCreate } = createCrudService({ model, name: 'education' }); + + const result: any = await handlerCreate({ candidateId: 'c1', school: 'MIT' }); + + expect(result.success).toBe(true); + const data = Array.isArray(result.data) ? result.data[0] : result.data; + expect(data._id).toBeDefined(); + expect(data._id).not.toBeNull(); + expect(data._id).toBe('real-id-1'); + }); +}); diff --git a/src/__tests__/services/baseCreateDocument.test.ts b/src/__tests__/services/baseCreateDocument.test.ts new file mode 100644 index 0000000..a714395 --- /dev/null +++ b/src/__tests__/services/baseCreateDocument.test.ts @@ -0,0 +1,64 @@ +/** + * Tests for services/index.ts's baseCreateDocument — regression coverage for + * issue #157 (data._id: null on every POST .../create response). The bug: + * hookAfterSave used to receive `data` by destructured value, so reassigning + * it inside the hook never reached baseCreateDocument's own `_data` variable + * — the caller always got MODEL.create()'s raw result (`_id: null`, since + * `{ _id: null, ...document }` is passed to create()) instead of the + * refetched document with its real id. Fixed by using hookAfterSave's return + * value (`replacement`) as the new `_data` when it isn't undefined. Uses a + * fake Mongoose-shaped model, same style as baseFindDocument.test.ts. + */ +import { baseCreateDocument } from '@/services'; + +function createFakeModel(createdDoc: Record) { + return { + validate: jest.fn().mockResolvedValue(undefined), + create: jest.fn().mockResolvedValue(createdDoc), + }; +} + +describe('baseCreateDocument hookAfterSave propagation (issue #157)', () => { + it("returns hookAfterSave's replacement as data, not the raw create() result with a null _id", async () => { + const model = createFakeModel({ _id: null, candidateId: 'c1' }); + const realDoc = { _id: 'real-id-123', candidateId: 'c1' }; + + const result = await baseCreateDocument({ + document: { candidateId: 'c1' }, + model, + name: 'education', + hookAfterSave: async () => realDoc, + }); + + expect(result.success).toBe(true); + expect(result.data).toBe(realDoc); + expect((result.data as any)._id).toBe('real-id-123'); + }); + + it('falls back to the raw create() result when hookAfterSave returns undefined', async () => { + const createdDoc = { _id: 'created-id', candidateId: 'c1' }; + const model = createFakeModel(createdDoc); + + const result = await baseCreateDocument({ + document: { candidateId: 'c1' }, + model, + name: 'education', + hookAfterSave: async () => undefined, + }); + + expect(result.data).toBe(createdDoc); + }); + + it('returns the raw create() result unchanged when no hookAfterSave is given', async () => { + const createdDoc = { _id: 'created-id-2', candidateId: 'c1' }; + const model = createFakeModel(createdDoc); + + const result = await baseCreateDocument({ + document: { candidateId: 'c1' }, + model, + name: 'education', + }); + + expect(result.data).toBe(createdDoc); + }); +});