# Plan Review Panel — samsung-account-intake-m5

**Plan**: `.claude/plans/samsung-account-intake-m5.plan.md` · **Plan version**: `sha256:29be106a6680bbf4fe25c567191a7ba2598f213dca23a1f12cf8feadb9966b69`
**Verdict**: `converged` via `multi-agent`
**Quorum**: 4/3 responses · 4 distinct roles (of 4 fielded) · passed=true
**Layers**: L1 converged · L2 converged · L3 not fired

> Reason: L1 + L2 quorum satisfied (4/3 responses, 4 distinct roles); L3 not fired

## Findings

| Perspective | Severity | Claim | Evidence |
|---|---|---|---|
| security | MEDIUM | The plan never says where confirm gets the member number (memId) it writes to. The lookup returns members from every agency with that login ID, so a naive choice could stop, cancel, revive or re-password a member of a different agency. | agency-fc.repository.ts:359-377 `getIntakeMembersByLoginIds` filters only by `inArray(memLoginId, loginIds)`, not by agency. samsung-intake.service.ts:162-163 has the judge pick `mine` by `m.agcId === ctx.agency.agcId` and keep `others`. Plan Task 3 says only 'IntakeMember 에 memId 를 더하되 판정 행 응답에는 싣지 않는다' and '중지·해지 행의 회원 번호로', with no rule that memId comes from the selected agency's row. The same login ID in s1 and s2 is exactly the case the mismatch warning exists for. None of the #8 counterfactuals (confirm bypass, divergence, mismatch acknowledgement, seat pre-check) covers taking memId from the wrong agency. |
| security | LOW | The `force` and `mismatchAcknowledged` fields are client-set with no server-side proof, so a caller can send `force=true` without ever seeing the seat warning. The plan accepts this on purpose and records the override. | Plan DD10 and NOT Building line 131: the fields are mistake prevention, not authorization; the alarm and log record '의심 확인·강행 여부'. Only admins who pass the allowlist can call the endpoint. |
| test | MEDIUM | Task 3 says a `resign` call on a stop or cancel row counts as failed when it returns `skipped` `not-found` and as applied when it returns `no-subscription`. The plan also marks backlog `samsung-m1-resign-skipped-discarded` as partly absorbed on that basis. No required `it` covers this branch. If the implementation ignored `skipped` and marked every stop row as applied, gates #4, #5 and #8 would all still pass, and the missing member would be reported to the admin as applied. | plan.md Task 3: "반환 `skipped` 가 `not-found` 면 실패, `no-subscription` 은 반영". The plan's carry-over table says "부분 흡수 \| M5 호출처는 소비한다 — `not-found` 는 행 실패". The only stop/cancel `it` in the gate file (samsung-account-intake-m5.gates.mjs:78) is 'confirm: 중지와 해지는 하한을 한 번 판정하고 행마다 강행 사용중지를 부른다', and nothing in it mentions `skipped`. The gate at gates.mjs:81 ('한 행의 실패가 다음 행을 막지 않고') is about exceptions, not the `skipped` return value. The Acceptance line for stop/cancel (plan.md Acceptance, 4th item) never mentions `skipped`. The §NOT Building list of claims left to review does not list this one either. |
| test | LOW | Counterfactual axis 3 only tests setting `mismatchBlocked` to always false, so it only proves the check exists. Nothing tests the other half of the rule: a suspected mismatch with `mismatchAcknowledged` present should go through, and the marker should be recorded. The alarm `it` checks the marker in pure-module output, but no service-level `it` checks that the acknowledged path reaches the writes and logs the marker. | gates.mjs:107 `const mismatchBlocked = false;`. The only mismatch `it` is gates.mjs:73 'confirm: 재판정에서 기업 선택 의심이면 확인 없이 쓰기 없이 거부한다'. DD10 says the override "사실을 알림 문구와 로그에 남긴다", and Task 3 step (11) logs "의심 확인·강행 여부". No required `it` covers the service log or alarm call with acknowledgement. |
| invariant | MEDIUM | The plan says how to handle `resign` skip reasons for stop and cancel rows (`not-found` is a failure, `no-subscription` counts as applied). It says nothing about the revive row's `resign({ids:[memId], useAt: 1}, admin)` call. With two arguments, that call always returns `skipped` reason `not-requested`, and it can also return `not-found`. Nothing says `not-found` on a revive row is a failure, so an unrecognised reason can quietly count as applied. | insuniverse-server/apps/api/src/modules/fc/agency/fc-agency.service.ts:855-857 (restoreSubscribe !== true -> skipped 'not-requested') and :880-883 ('not-found'). Plan Task 3 revive bullet: "→ `resign({ids:[memId], useAt: 1}, admin)` → 재판정 구독이 `active` 가 아니면 `subscribe(...)`" gives no skipped mapping, while the stop bullet does: "반환 `skipped` 가 `not-found` 면 실패, `no-subscription` 은 반영". None of the four #8 counterfactual axes checks this. |
| invariant | LOW | Check #1 decides whether to skip its checks only by whether the baseline file exists. If `baseline.json` exists, the clean-tree and parent-clean checks are skipped, and `baseline-report` only checks that the file matches itself. A stale or hand-made baseline makes #2, #3 and #6 compare against the wrong set. The plan admits this in NOT Building, but #1's expected output still reads like a fresh freeze. | Plan Validation lines 339-349 (`if [ ! -f "$B" ]; then ... fi; node "$LIB" baseline-report ...`); NOT Building: "`baseline.json 변조` 방지 — 기준선은 로컬 파일이라 게이트가 막지 않는다" |
| invariant | LOW | Check #7's expected PRD status can be changed with an environment variable (`M5_PRD_EXPECT="${M5_PRD_EXPECT:-in-progress}"`). Whoever runs the gate can set any status and #7 will pass on it, and nothing prints where the value came from. | Plan Validation line 317 and #7 scope-check argument "$M5_PRD_EXPECT" |

## Refutation attempted

| Perspective | Verdict | What was attacked |
|---|---|---|
| architect | pass | I checked whether the plan's citations of the existing service methods match the code. The plan says it reuses `resign` with two arguments, `assertSeatLowerBound(memIds, admin, force)`, the `subscribe(dto, admId, requestType)` shape, and `excelValidate`/`dataUpload(record, rows, subNextPayDay)`. All of these match insuniverse-server/apps/api/src/modules/fc/agency/fc-agency.service.ts at lines 703, 806, 1318, 1917 and 1938. `resign` does have an optional third argument, `opts.restoreSubscribe`, and the plan deliberately leaves it unused (DD1). I also checked the plan's 'new account' pattern citation, insuniverse-server/apps/api/src/modules/fc/agency/agency-fc.event-handler.ts lines 27-68: `dataUpload` runs first, then `subscribe` with `isNewSubscribe: 1`, `undefined` and `회원등록엑셀`, then `addAlarm`. The plan's create-row sequence mirrors that, and the two writes really are separate (DD4). I then attacked the boundaries. The seat-limit check (DD3) runs once for all rows before any write, while the per-row stop calls pass `force: true`, which the method signature allows, so a later row cannot trigger a mid-run warning. The pure module `samsung-intake-confirm.ts` does no reading or writing of its own. Calls to private methods through brackets are not type-checked because `noImplicitAny` is false. The plan says so itself (GROUND 21) and backs it with a `jest.spyOn` wiring test, so the gap is covered. The per-agency lock (DD8) only works inside one server process. The plan states that limit under NOT Building and names the unique constraint as the backstop, so it is disclosed rather than hidden. Moving the shared re-check into a private `rejudge` step keeps the preview response unchanged and is covered by the preview spec. I found no second owner of state, no layering breach, and no misquoted precedent. Not verified: the `IntakePreview.tsx`/`CorpfcPage.tsx` line citations on the admin side, and the gates script body. |
| security | pass | What I attacked: 1. **Forged selections.** A client sends selections the server never judged. The server re-judges, and `planConfirm` only applies rows whose login ID and verdict match. `parseSelections` rejects verdicts outside the apply set. Not landed. 2. **Password leakage.** Checked the failure-rows Excel columns (no password column), `maskPassword` on failure reasons, the rule that raw error text goes only to the log as a code, and the alarm text (counts only). Observation 23 says duplicate-check messages use the uploaded values, so no other member's data appears. Not landed. 3. **JSON body logged in plaintext.** DD9 sends selections as a multipart field and DD6 returns the Excel in the response. Not landed. 4. **memId in the response.** The plan keeps it out by type. Not landed. 5. **Concurrent confirms.** The per-process Set is a disclosed limit. Duplicate member rows are blocked by the unique constraint. Accepted residual risk. 6. **Override bypass.** Covered in the LOW finding. 7. **Cross-agency writes.** The lookup is not agency-scoped and the plan does not say where memId comes from. This one lands as the MEDIUM finding: it is a gap in the spec, not proven broken code. |
| test | pass | I read the whole plan (lines 1-645) and the SPEC_GROUPS, CF and COUNTERFACTUAL tables in gates.mjs (lines 59-125). I checked each Acceptance claim for gates #4, #5 and #8 against the required `it` names. The plan's counts line up with the gate: 9 required tests for #4 and 20 for #5 (15 confirm/wiring plus access, repository and log). Things I attacked that held up: - Every counterfactual has a matching must-fail `it`, and the must-pass `it`s don't depend on any of the four flipped anchors. - The revive branch where the subscription is still active is tested. - The concurrency `it` blocks the first request with a promise that never resolves, and also checks the lock is released afterwards. - `wiring:` uses `spyOn` to check that the methods called through bracket access still exist. - Every Validate path names spec files from Files to Change. - The M4 preview spec's select-key assertion is updated alongside the code (observation 10), so that old test isn't locking in the old behaviour. The only gap I could back with evidence is that no test covers how stop/cancel rows handle `resign` returning `skipped` (MEDIUM). Nothing I found was HIGH or CRITICAL, so the verdict is pass. |
| invariant | pass | Checked the order of the pre-write blocks. The selection parse, agency lookup, lock, rejudge, mismatch check, contract check and seat check all come before `settleInBatches`. The plan's spec asserts zero write calls for these blocks, and #8 has counterfactuals for three of them: mismatch, drift and seat. Unknown DTO input fails closed: `@IsIn(['true'])` rejects \\"false\\" and junk, and `parseSelections` throws on a non-array, a duplicate row, zero rows or a verdict outside the apply set. The in-process lock is taken with has+add in one tick and released in finally; the multi-task limit is disclosed. Read the real `resign` code (fc-agency.service.ts:806-887) and the `agencyResigned` repository call to see how each skip reason is consumed. The revive mapping is the gap reported above. Traced the #10 seal: under set -e it matches on output text, not just the exit code. Checked that the #8 temp copies are removed by an EXIT trap and that #7 runs before those copies exist. Looked at #1 (skip when the baseline exists) and #7 (env override). Rollback: the plan says there are no schema changes and that data already written is not rolled back by reverting code (it has to be undone with an opposite-intent upload file). This is disclosed, not claimed as a code rollback. Found nothing HIGH: every missing-tool or missing-artifact path I traced in #1-#11 ends in a FAIL. |

## Measurement

<!-- Written by plan-review/cli.js record on EVERY exit path, pass or halt.
     Machine-readable; do not hand-edit. A null field means the axis was
     not observed, never that it was zero. -->

```json
{
  "verdict": "converged",
  "source": "multi-agent",
  "layers": {
    "l1": "converged",
    "l2": "converged",
    "l3": "not fired"
  },
  "quorum": {
    "responded": 4,
    "required": 3,
    "roles": 4,
    "of": 4,
    "passed": true
  },
  "wall_clock_ms": 86862,
  "halt_stage": null,
  "backlog_appended": null,
  "backlog_skipped_nonblocking": null,
  "granted": 4,
  "reviewed_plan_hash": "sha256:29be106a6680bbf4fe25c567191a7ba2598f213dca23a1f12cf8feadb9966b69",
  "plan_path": ".claude/plans/samsung-account-intake-m5.plan.md",
  "recorded_at": "2026-09-17T09:05:21.671Z"
}
```