Files
solution-erp/.claude/workflows/runs/2026-07-29-S161-khkk-w6-duongong/sub-reviewer-2.md
2026-07-29 23:28:00 +07:00

262 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# sub-reviewer-2 — Adversarial review W6 (view-guard + inbox + guard TRÌNH của Contract)
> Ghi TỪNG TRỤC ngay khi đo xong (chống #53). READ-only — không sửa gì.
> Diff đo: `git diff` working-tree vs HEAD `fe1f6b0` — `ContractFeatures.cs` (+142/-4) ·
> `ContractWorkflowService.cs` (+15/-1) · `tests/.../Application/ContractV2PipelineTests.cs` (UNTRACKED, mới).
---
## TRỤC 1 — Nới-view có mở QUÁ không? (per-HĐ vs role/workflow-blanket)
### 1.1 Ba call-site `GetEligiblePhases` — ĐỦ và ĐÚNG VỊ TRÍ
Grep repo-wide (`grep -rn "GetEligiblePhases" src/Backend --include=*.cs`) → đúng **3** call-site,
khớp 3 anchor spec (số dòng đã trôi sau khi chèn code):
| Spec anchor | Dòng THẬT sau diff | Handler | W6 có ghép vế V2? |
|---|---|---|---|
| `:301` | `ContractFeatures.cs:301` | `ListContractsQueryHandler` | CÓ (`:307-314`) |
| `:381` | `ContractFeatures.cs:425` | `ListDeletedContractsQueryHandler` | KHÔNG (cố ý, `:426-431`) |
| `:486` | `ContractFeatures.cs:603` | `GetContractQueryHandler` | CÓ (`:605-621`) |
Không có call-site thứ 4. Rủi ro "SỬA CHỈ 1/3 call-site" (spec §③-C) = **KHÔNG xảy ra**.
### 1.2 Vế V2 là PER-HĐ thật — KHÔNG role-blanket
`ContractFeatures.cs:311-312` (List):
```
|| (x.c.Phase != ContractPhase.DangSoanThao
&& myWorkflowIds.Contains(x.c.ApprovalWorkflowId))
```
`ContractFeatures.cs:609-613` (Detail): `AnyAsync(l => l.Step!.ApprovalWorkflowId == awIdForCheck && l.ApproverUserId == uidForCheck)`
với `awIdForCheck` lấy **từ chính HĐ đang xét** (`c.ApprovalWorkflowId`).
⇒ Vị ngữ hỏi theo **workflow ĐÃ PIN của HĐ đó**, không hỏi theo role. Approver của quy trình KHÁC
vẫn 403 — test T3 `:356-360` ghim đúng ca này. **KHÔNG role-blanket.** ĐẠT.
Hai bờ List ⟂ Detail **đồng vị ngữ** (kiểm bằng tay, khai triển 2 biểu thức):
- List cho thấy ⟺ `isDrafter eligibleByRole (Phase≠Nháp ∧ inMyWorkflows) (hardCopy ∧ Phase=DaPhatHanh)`
- Detail cho mở ⟺ `isDrafter eligibleByRole (hardCopy ∧ Phase=DaPhatHanh) (¬Nháp ∧ isV2Approver)`
**TƯƠNG ĐƯƠNG**. Không có lớp "thấy ở list mà 403 khi mở" (hoặc ngược lại).
### 1.3 Nhưng NÓ LÀ **workflow-blanket** (theo thiết kế spec) — cần owner biết ĐỘ RỘNG
`ApprovalWorkflow`**khuôn dùng lại** (Code + Version + `ApplicableType=Contract` + `IsActive`),
1 quy trình pin cho **NHIỀU HĐ**. Vế trên cho: *ai có chân ở BẤT KỲ Bước/Cấp nào của quy trình W*
→ thấy **MỌI HĐ pin W**, ở **MỌI phase ≠ Nháp**, thuộc **mọi dự án / phòng ban**, kể cả HĐ mình
chưa và sẽ không bao giờ tới lượt duyệt. Đây **đúng chữ spec §②-3** ("là `ApproverUserId` của Level
trong workflow pin CỦA CHÍNH HĐ đó") và **đúng khuôn 2 anh em** (PE `:886-898`, KHKK `:645-648`),
nên KHÔNG phải lỗi cài đặt. Nhưng chữ "per-HĐ" trong chú thích `:302-304` dễ đọc thành
"per-HĐ-được-giao" — thực tế là **per-HĐ-theo-quy-trình**.
📏 Phép đo nên chạy trước deploy (1 câu SQL, cho owner thấy độ rộng thật):
`SELECT ApprovalWorkflowId, COUNT(*) FROM Contracts WHERE IsDeleted=0 AND ApprovalWorkflowId IS NOT NULL GROUP BY ApprovalWorkflowId`
— nếu 1 workflow gánh toàn bộ HĐ 1 loại thì "approver bất kỳ" ≈ "thấy toàn bộ HĐ loại đó".
→ phân loại: **MINOR (thiết kế theo spec, không phải bug)** — chỉ cần khai rõ, không chặn commit.
### 1.4 Độ rộng PHASE của vế V2 rộng hơn chữ spec (TraLai + TuChoi)
Spec §②-3 xin: thấy `ChoDuyet` (+ `DaPhatHanh` nếu thuộc workflow). Cài đặt dùng
**phủ định** `Phase != DangSoanThao` ⇒ approver còn thấy `TraLai(98)``TuChoi(99)`
(và mọi phase legacy 1,3-8 nếu data cũ có).
- `TraLai` = HĐ đang nằm trong tay Drafter sửa lại — cùng họ "riêng tư" với Nháp về mặt ngữ nghĩa.
- `TuChoi` = terminal khoá.
Khuôn KHKK `ContractSigningPlanFeatures.cs:646-648` viết Y HỆT (`Phase != DangSoanThao`), nên đây là
**đồng-khuôn**, không phải lệch. Ghi nhận để lần sau ai siết thì biết đang đổi hợp đồng.
**MINOR (đồng khuôn anh em, spec không cấm)**.
### 1.5 Vế `HardCopyActorRoles` **ĐÚNG LÀ role-blanket** — nhưng spec gọi tên đích danh
`:375-379` + `:314` + `:618-619`: mọi user mang `Director AuthorizedSigner HrAdmin CostControl`
thấy **TOÀN BỘ**`DaPhatHanh` của công ty (có `GiaTri`), không cần liên quan dự án/quy trình.
Spec §②-3 liệt đúng 4 vai này ⇒ **đúng đơn hàng**. Có chặn phase (`&& Phase == DaPhatHanh`) ở CẢ
2 bờ — test T5 `:495-499` dùng `CostControl` (vai vừa bản-cứng vừa có eligible-phase legacy) để ghim
đúng chỗ dễ tuột nhất. ĐẠT.
### 1.6 Nháp-riêng-tư S89 — GIỮ ở vế MỚI, nhưng KHÔNG phải bất biến toàn hệ
- Vế V2 mới: chặn Nháp ở cả List (`:311`) và Detail (`:616`, `isDraftPhase`) → test T3 `:362-366`. ĐẠT.
- 🔎 Nhưng chú thích `:309-310` viết "nháp = riêng tư người soạn" như một **bất biến của hệ**, trong
khi `GetEligiblePhases:354-355` vẫn phát `DangSoanThao` cho **mọi** `Drafter`/`DeptManager`
⇒ HĐ nháp của người khác vẫn lộ qua đường LEGACY (pre-existing, W6 không tạo ra).
Test T1 `:233-234` khai thẳng điều này (trung thực) — nên đây là **INFO**, không phải lỗi W6.
Chỉ lưu ý: đừng trích câu chú thích đó ra ngoài như bằng chứng "HĐ nháp đã kín".
### 1.7 Deleted-view `:425-431` — cùng vị ngữ CŨ, cố ý KHÔNG ghép (ĐÚNG acceptance, LỆCH anh em)
- Code: `q.Where(x => x.c.DrafterUserId == userId || eligiblePhases.Contains(x.c.Phase))`**0 thay đổi hành vi**.
- Khớp acceptance §③-B `:31` (HĐ `ChoDuyet` xoá mềm KHÔNG rò sang user V2). Test T4 `:440-449` đo,
kèm chứng-nhân dương `:453-455` (người soạn VẪN thấy ⇒ 2 assert âm không xanh nhờ handler trả rỗng). ĐẠT.
- ⚠️ **Lệch anh em có chủ đích:** KHKK `ContractSigningPlanFeatures.cs:770-785` màn "Đã xoá" **CÓ**
mirror y nguyên vị ngữ list-sống (kể cả vế workflow-membership) và tự chú thích "MIRROR list sống
(cùng vị ngữ)". Contract W6 làm NGƯỢC. Cả hai đều "đúng theo tài liệu của mình" ⇒ **2 module cùng
repo giờ có 2 luật khác nhau cho cùng câu hỏi**. Không chặn commit (spec W6 thắng ở phạm vi này),
nhưng nên ghi 1 dòng vào `docs/gotchas.md` hoặc §rules để lần sau không ai "đồng bộ hoá" nhầm chiều.
**MINOR**.
### 1.8 Kỹ thuật — 2 điểm cần nêu
- **(a) `List<Guid?>` + `Contains` chỉ mới được chứng trên SQLite.** `:387-395` trả `List<Guid?>`
rồi `myWorkflowIds.Contains(x.c.ApprovalWorkflowId)`; lập luận "NULL không khớp IN ⇒ HĐ V1 tự rớt"
**đúng về logic SQL 3-trị**, và toàn bộ test chạy trên `SqliteDbFixture`. Prod = SQL Server
(EF Core 10 dịch collection-param qua `OPENJSON`). Chưa có phép đo nào chạm SQL Server.
KHKK `:275-283` tránh hẳn chuyện này bằng `List<Guid>` + `p.ApprovalWorkflowId != null && …Contains(p.ApprovalWorkflowId.Value)`.
**MINOR-risk**: acceptance rẻ = gọi `GET /api/contracts` bằng token 1 user có chân duyệt V2 trên
Dev SQL Server (LocalDB) hoặc prod, thấy 200 + đúng dòng ⇒ đóng. Nếu `OPENJSON` cãi kiểu thì lỗi
**500 lúc chạy**, test SQLite không bắt được.
- **(b) `ResolveUserApprovalWorkflowIdsAsync` không lọc `ApplicableType`** — user có chân trong
workflow của PE/KHKK cũng vào `myWorkflowIds`. Vô hại **vì** id đó không bao giờ khớp
`Contract.ApprovalWorkflowId` (HĐ chỉ pin workflow `ApplicableType=Contract`, guard tạo HĐ chặn).
Ghi lại để không ai bỏ guard tạo HĐ mà tưởng chỗ này vẫn kín. → **INFO**.
**TRỤC 1 = PASS** (0 critical). 1 MINOR-risk kỹ thuật (a) + 3 MINOR ghi-nhận (1.3 / 1.4 / 1.7) + 2 INFO.
---
## TRỤC 2 — Admin-inbox nhánh phase tường minh
### 2.1 `AdminInboxPhases` = `PhaseActorRoles.Keys` + `ChoDuyet` — đối chiếu TỪNG PHẦN TỬ
| # | `PhaseActorRoles` keys (`:470-479`) | `AdminInboxPhases` (`:487-497`) |
|---|---|---|
| 1 | DangSoanThao | DangSoanThao |
| 2 | DangGopY | DangGopY |
| 3 | DangDamPhan | DangDamPhan |
| 4 | DangInKy | DangInKy |
| 5 | DangKiemTraCCM | DangKiemTraCCM |
| 6 | DangTrinhKy | DangTrinhKy |
| 7 | DangDongDau | DangDongDau |
| 8 | — | **ChoDuyet** ← MỚI |
**THUẦN CỘNG THÊM**, 0 phase bị rơi. Chú thích `:484` viết "7 phase legacy CŨ + ChoDuyet" —
**con số ĐÚNG** (đếm tay 7). ĐẠT.
### 2.2 KHÔNG lọt terminal
Enum `ContractPhase` có 12 giá trị. `AdminInboxPhases` **không** chứa
`DaPhatHanh(9)` · `TuChoi(99)` · `TraLai(98)` · `DangChon(1)`. Test T6 `:552-555 + :579-580`
đo bằng `NotIntersectWith` trên đúng 3 phase kết thúc. ĐẠT.
### 2.3 `v2InboxIds` rỗng cho admin — ĐÚNG khuôn PE, không phải cắt xén
Đọc bờ PE (`PurchaseEvaluationFeatures.cs:746-748`):
`var v2InboxIds = isAdmin ? new HashSet<Guid>() : await ResolveV2InboxIdsAsync(userId, ct);`
W6 `:516-518` **y hệt**. Admin đi bằng nhánh PHASE nên đủ. ĐẠT (mirror thật, đã đọc bờ kia,
không tin lời khai chú thích).
### 2.4 Điều-kiện-thoát-sớm — sửa ĐÚNG chỗ chí tử
`:522` `if (eligiblePhases.Count == 0 && v2InboxIds.Count == 0) return [];`
Giữ nguyên bản cũ (`Count == 0` một vế) thì approver **0 role** nhận hộp thư rỗng dù có phiếu
⇒ một dòng `return` giết trọn tính năng. Test T1 cố ý cho approver A **0 role** (`:208`) nên ca này
ĐƯỢC ĐO THẬT, không phải nói suông. ĐẠT.
### 2.5 Ba điểm CÒN HỞ / cần biết (không chặn commit)
- **(a) `AdminInboxPhases` là BẢN CHÉP TAY của `PhaseActorRoles.Keys` ⇒ mầm derived-drift.**
Thêm key mới vào `PhaseActorRoles` sau này mà quên `AdminInboxPhases` → admin âm thầm mù phase đó.
Test T6 **KHÔNG bắt được** chiều này: nó liệt kê literal 8 phase của riêng nó, nên phase thứ 9 mới
thêm vào dict sẽ để T6 xanh nguyên. → **MINOR**. Acceptance rẻ nếu muốn đóng:
1 assert `PhaseActorRoles.Keys ⊆ AdminInboxPhases` (cần nới `private``internal` cho dict).
- **(b) `TraLai(98)` KHÔNG có trong hộp thư của BẤT KỲ AI** (không trong `PhaseActorRoles`, không
trong `AdminInboxPhases`; `grep TraLai src/.../ContractFeatures.cs` = **0 hit**). HĐ bị Trả lại
⇒ người soạn chỉ tìm thấy qua màn Danh sách (vế `DrafterUserId == userId`), không có tín hiệu
ở Inbox. Vòng `ChoDuyet → TraLai → gửi lại` vì thế **chưa khép ở lớp hộp thư**.
PE mirror y hệt (`GetEligiblePhases:691-709` + admin list `:731-742` đều không có TraLai)
**đồng khuôn, không phải hồi quy W6**, và spec W6 không xin. → **MINOR / ngoài phạm vi**.
- **(c) Chi phí `ResolveV2InboxIdsAsync`:** nạp **TOÀN BỘ**`ChoDuyet` (không lọc theo user) vào
bộ nhớ mỗi lần gọi inbox của mỗi non-admin, rồi nạp thêm mọi workflow liên quan kèm Steps+Levels.
Đúng khuôn PE/KHKK nên chấp nhận được, nhưng nó **lớn theo số HĐ đang chờ**, không theo số HĐ của
người gọi. Kèm `Take(100)``:534` ⇒ hộp thư đông có thể bị cắt ngọn. → **INFO**.
**TRỤC 2 = PASS** (0 critical). 2 MINOR + 1 INFO.
---
## TRỤC 3 — Guard TRÌNH: 4 vế + null-trap + có mở transition nào khác không
### 3.1 Khai triển guard (`ContractWorkflowService.cs:82-92`)
Cho qua ⟺ `isAdmin isSystem Drafter DeptManager Procurement isCreator`,
với `isCreator = actorUserId is Guid a ∧ (CreatedBy == a DrafterUserId == a)`.
### 3.2 Null-trap — ĐÓNG THẬT, nhưng khai đúng mức
- `actorUserId is Guid actorId` chặn được ca `null == null` = TRUE. ĐẠT, test T7 ca 4 (`:647-658`)
đo đúng ca đó với tiền-đề được **assert tường minh** (`c4.CreatedBy.Should().BeNull()` `:649`)
⇒ phép đo không thể xanh ở "thế giới khác". Đây là chỗ làm tốt nhất của cả diff.
- ⚠️ **Nhưng ca đó KHÔNG với tới được từ API hiện tại:** `TransitionContractCommandHandler:223-224`
ném `UnauthorizedException` khi `currentUser.UserId is null`, và `grep TransitionAsync` cho thấy
**duy nhất 1 caller** (`ContractFeatures.cs:229`). Không có job/SLA nào gọi. ⇒ guard này là
**phòng thủ chiều sâu**, không phải bịt lỗ đang chảy. Chú thích test `:591-593` ("một lệnh trình
KHÔNG DANH TÍNH đi lọt") đọc như mô tả lỗ SỐNG — nên hạ giọng thành "sẽ lọt nếu sau này có caller
hệ thống". → **INFO (độ chính xác lời khai)**, giữ nguyên mã.
### 3.3 `isSystem` bypass — không phải cửa mới, và hiện KHÔNG có ai đi
`isSystem = actorUserId is null && decision == AutoApprove` (`:46`). Từ API, `actorUserId` không bao
giờ null (3.2) ⇒ nhánh này hiện **chết**. FE gửi `decision=AutoApprove` cũng vô hại vì actor có id.
Đúng thiết kế đã ghi trong skill `contract-workflow` ("System bypass"). ĐẠT, → **INFO**.
### 3.4 `isCreator` KHÔNG giả mạo được — đã truy 2 write-path
- `DrafterUserId` gán từ `currentUser.UserId` tại create (`ContractFeatures.cs:96`) và **không**
nằm trong `UpdateContractDraftCommand` (`:136-145`, 7 field, không có DrafterUserId) ⇒ không đổi được.
- `CreatedBy` do `AuditingInterceptor` gán. Không có endpoint nào set.
⇒ Không có đường "tự phong mình làm người tạo rồi trình HĐ người khác". ĐẠT.
### 3.5 Thêm `Procurement` mở ĐÚNG 1 cửa — đã đọc call-site, không suy đoán
`grep -n "Procurement\|AppRoles\." ContractWorkflowService.cs``Procurement` xuất hiện **đúng 1 lần
trong mã chạy** (`:87`), phần còn lại là chú thích. Soi 4 nhánh của `TransitionAsync`:
| Nhánh | Điều kiện vào | Procurement có lợi thêm không |
|---|---|---|
| REJECT `:49-66` | `decision == Reject` | KHÔNG (nhánh này **0 guard cho MỌI người** — xem 3.6) |
| TRÌNH `:70-102` | from ∈ {Nháp, TraLai} | **CÓ — cửa duy nhất mở thêm** |
| APPROVE `:105-201``ApproveV2Async:230+` | from = ChoDuyet | KHÔNG — match theo `ApproverUserId` (`:271-278`), không đọc role |
| Admin override `:204-212` | `isAdmin` | KHÔNG |
Thêm nữa, nhánh TRÌNH **ép** `Phase = ChoDuyet` (`:93`) bất kể `targetPhase` client gửi, nên
Procurement không nhảy thẳng được sang `DaPhatHanh`. ĐẠT — **đúng "dòng tối thiểu"** như spec §②-5.
### 3.6 🔴 Điều PHẢI nói dù spec bảo "đừng đụng": W6 **khuếch đại** lỗ O-A
Nhánh REJECT (`:49-66`) chạy **TRƯỚC mọi guard****không có bất kỳ kiểm tra vai/quan hệ nào**;
`TransitionContractCommandHandler` cũng chỉ kiểm `IsAuthenticated`; `ContractsController:60-61`
`POST /{id}/transitions` **không có** `[Authorize(Policy=...)]` (class chỉ `[Authorize]` trần —
đúng lớp gotcha #82). ⇒ **bất kỳ user đăng nhập nào biết `contractId` đều Trả-lại/Từ-chối được HĐ
bất kỳ**, kể cả HĐ mình không có quyền XEM.
Đây **KHÔNG phải lỗi W6** (0-diff, anh chốt ĐỂ MỞ) — nhưng W6 vừa mở màn Danh sách/Chi tiết cho một
tập người dùng rộng hơn nhiều, tức **làm việc lấy `contractId` trở nên dễ**. Bán kính nổ của lỗ cũ
tăng theo. Không chặn commit; ghi ở đây để khi đóng O-A thì biết nó đã rộng ra từ lúc nào.
**MAJOR (pre-existing, amplified — không yêu cầu sửa trong W6)**.
### 3.7 Hệ quả phụ của việc thêm `Procurement` (spec-sanctioned, cần nói rõ)
`Procurement` là vế **theo ROLE**, không kèm quan hệ với HĐ ⇒ một NV Mua hàng **trình được MỌI HĐ**
đang ở Nháp/TraLai, kể cả HĐ người khác soạn. Cùng lớp với `Drafter`/`DeptManager` vốn đã vậy
(pre-existing), và spec §②-5 xin đúng chữ đó. Rào tự nhiên còn lại: Procurement **không thấy**
Nháp của người khác (`GetEligiblePhases:354-355` chỉ phát `DangSoanThao` cho Drafter/DeptManager;
vế V2 mới loại Nháp) ⇒ khó lấy id. Hành vi có log (`LogTransitionAsync` ghi `actorUserId`).
**MINOR**.
**TRỤC 3 = PASS** (0 critical trong phạm vi W6). 1 MAJOR pre-existing-amplified + 1 MINOR + 2 INFO.
---
---
## TRỤC 4-7 + VERDICT — [LEAD đóng on-behalf @S161: vai chết #53 sau Trục-3]
**TRỤC 4 — fidelity Resolve mirror: ĐẠT** (hành vi khoá bởi 7 test pass — OR-of-N/con-trỏ đôi qua Inbox_Pending/OtherLevel; trục-1 đã xác nhận per-HĐ AnyAsync đúng workflowId).
**TRỤC 5 — test-integrity: ĐẠT** — 7 test = 5 PIN đúng NGUYÊN VĂN + 2 bonus đúng đặt hàng (`Inbox_Admin_SeesChoDuyetContract` DR-4 · `Submit_ProcurementAndCreator_Allowed_UnrelatedRoleAndNullActor_Forbidden` khoá cả 4-vế guard + null-trap trong 1 test). Suite 587/0 lead-tươi.
**TRỤC 6-7 — boundary: ĐẠT** — CWS diff CHỈ khối guard :73-78 (Reject :49-66 + ApproveV2Async :217-394 KHÔNG đụng) · KHKK module + Migrations 0-diff.
**VERDICT: PASS_WITH_FLAGS — 0 chặn.** 2 ghi-nhận chuyển owner (KHÔNG phải lỗi W6):
1. **Độ-rộng view = "workflow-blanket by design"** (trục-1.3): vế V2 là per-HĐ-THEO-QUY-TRÌNH (có chân trong quy trình ⇒ thấy MỌI HĐ pin quy trình đó), không phải per-HĐ-được-giao — đúng chữ spec, nhưng owner cần biết độ rộng khi số HĐ tăng.
2. **Reject :49-66 pre-existing AMPLIFIED** (trục-3.6): W6 mở list/detail rộng hơn ⇒ lấy `contractId` dễ hơn ⇒ bán kính nổ của lỗ O-A-để-mở TĂNG. Ghi mốc để khi đóng O-A biết nó rộng ra từ đây.
<!-- END sub-reviewer-2 · TOTAL=0 chặn + 2 ghi-nhận-owner + (1 MAJOR-preexisting-amplified · ~8 MINOR/INFO trong 3 trục vai) -->