forgejo_admin/westside-contracts
1
0
Fork 0
Code Issues 2 Pull requests Projects Releases Packages Wiki Activity Actions

Duplicated validation logic + unguarded JSON parse #5

New issue
Closed
opened 2026-03-24 15:54:29 +00:00 by forgejo_admin · 3 comments
forgejo_admin commented 2026-03-24 15:54:29 +00:00
Owner
Copy link

Type

Bug

Lineage

plan-wkq → Phase 14 → QA nits from PR #2

Repo

forgejo_admin/westside-contracts

What Broke

Three issues found in QA review of PR #2:

  1. validation.ts has tested validation functions but +server.ts duplicates the logic inline — they will diverge
  2. request.json() in the sign endpoint is unguarded — malformed JSON body returns 500 instead of 400

Repro Steps

  1. Send malformed JSON to POST /contract/{token}/sign → returns 500
  2. Edit validation.ts logic → +server.ts inline checks don't update → divergence

Expected Behavior

  1. Malformed JSON returns 400
  2. Single source of validation logic (import from validation.ts)

Environment

  • Cluster/namespace: westside-contracts (prod)
  • Service version: latest
  • Files: src/routes/contract/[token]/sign/+server.ts, src/lib/validation.ts, .woodpecker.yaml

Acceptance Criteria

  • Sign endpoint imports validateSignRequest from validation.ts
  • Inline validation removed from +server.ts
  • Malformed JSON returns 400
  • New test added for malformed JSON body returning 400
  • Existing 12 tests pass

Related

  • project-westside-basketball
  • westside-app #72
  • westside-contracts PR #2 QA review
### Type Bug ### Lineage `plan-wkq` → Phase 14 → QA nits from PR #2 ### Repo `forgejo_admin/westside-contracts` ### What Broke Three issues found in QA review of PR #2: 1. `validation.ts` has tested validation functions but `+server.ts` duplicates the logic inline — they will diverge 3. `request.json()` in the sign endpoint is unguarded — malformed JSON body returns 500 instead of 400 ### Repro Steps 1. Send malformed JSON to `POST /contract/{token}/sign` → returns 500 2. Edit `validation.ts` logic → `+server.ts` inline checks don't update → divergence ### Expected Behavior 1. Malformed JSON returns 400 2. Single source of validation logic (import from validation.ts) ### Environment - Cluster/namespace: westside-contracts (prod) - Service version: latest - Files: `src/routes/contract/[token]/sign/+server.ts`, `src/lib/validation.ts`, `.woodpecker.yaml` ### Acceptance Criteria - [ ] Sign endpoint imports `validateSignRequest` from validation.ts - [ ] Inline validation removed from +server.ts - [ ] Malformed JSON returns 400 - [ ] New test added for malformed JSON body returning 400 - [ ] Existing 12 tests pass ### Related - `project-westside-basketball` - westside-app #72 - westside-contracts PR #2 QA review
forgejo_admin commented 2026-03-27 21:09:03 +00:00
Author
Owner
Copy link

Scope Review: NEEDS_REFINEMENT

Review note: review-313-2026-03-27

Three issues found during scope review:

  • Wrong arch label: Board item #313 has arch:westside-app but all work is in westside-contracts. Should be arch:contract-flow (consistent with other contract items on the board).
  • CI sub-issue already resolved: .woodpecker.yaml (note: issue says .yml, actual extension is .yaml) already contains npm test at line 23. Acceptance criterion #4 is pre-satisfied. Remove from scope and update title to reflect two remaining bugs (validation duplication + JSON parse).
  • Missing acceptance criterion: No criterion requiring a test for the malformed JSON 400 response. The fix should include a new test case, not just the try/catch guard.

File targets verified: +server.ts line 30 has unguarded request.json(), lines 32-46 duplicate validation.ts logic, and validateSignRequest is never imported. 12 existing tests pass. No blast radius to sibling services.

## Scope Review: NEEDS_REFINEMENT Review note: `review-313-2026-03-27` Three issues found during scope review: - **Wrong arch label**: Board item #313 has `arch:westside-app` but all work is in `westside-contracts`. Should be `arch:contract-flow` (consistent with other contract items on the board). - **CI sub-issue already resolved**: `.woodpecker.yaml` (note: issue says `.yml`, actual extension is `.yaml`) already contains `npm test` at line 23. Acceptance criterion #4 is pre-satisfied. Remove from scope and update title to reflect two remaining bugs (validation duplication + JSON parse). - **Missing acceptance criterion**: No criterion requiring a test for the malformed JSON 400 response. The fix should include a new test case, not just the try/catch guard. File targets verified: `+server.ts` line 30 has unguarded `request.json()`, lines 32-46 duplicate `validation.ts` logic, and `validateSignRequest` is never imported. 12 existing tests pass. No blast radius to sibling services.
forgejo_admin commented 2026-03-27 21:24:52 +00:00
Author
Owner
Copy link

Scope Review Corrections Needed

From review-313-2026-03-27:

Fix 1: Board label corrected: arch:westside-app → arch:contract-flow (done).

Fix 2: Remove CI sub-issue from title and body. .woodpecker.yaml already has npm test at line 23 — this was fixed since the ticket was filed. Also fix file extension: .yml → .yaml.

Fix 3: Add acceptance criterion: "New test added for malformed JSON body returning 400."

Once fixed → READY for next_up.

## Scope Review Corrections Needed From `review-313-2026-03-27`: **Fix 1:** Board label corrected: `arch:westside-app` → `arch:contract-flow` (done). **Fix 2:** Remove CI sub-issue from title and body. `.woodpecker.yaml` already has `npm test` at line 23 — this was fixed since the ticket was filed. Also fix file extension: `.yml` → `.yaml`. **Fix 3:** Add acceptance criterion: "New test added for malformed JSON body returning 400." Once fixed → READY for next_up.
forgejo_admin changed title from Duplicated validation logic + missing test step in CI + unguarded JSON parse to Duplicated validation logic + unguarded JSON parse 2026-03-27 21:48:15 +00:00
forgejo_admin commented 2026-03-27 21:48:36 +00:00
Author
Owner
Copy link

Issue body updated per scope review corrections.

Issue body updated per scope review corrections.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
29-add-age-based-team-reassignment-clause-t
23-fix-kaniko-push-permission-check-timeout
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/westside-contracts#5
No description provided.