feat(review): review_version 1.2 with answers, pre-existing, rounds and policy fields
This commit is contained in:
parent
ed67526987
commit
5709957e10
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
16 changed files with 1034 additions and 11 deletions
74
tests/fixtures/review-version/review-1-1.md
vendored
Normal file
74
tests/fixtures/review-version/review-1-1.md
vendored
Normal file
|
|
@ -0,0 +1,74 @@
|
|||
---
|
||||
type: trekreview
|
||||
review_version: "1.1"
|
||||
created: 2026-10-30
|
||||
task: "Add a retry to the HTTP client"
|
||||
slug: http-retry
|
||||
project_dir: .claude/projects/2026-10-30-http-retry/
|
||||
brief_path: .claude/projects/2026-10-30-http-retry/brief.md
|
||||
scope_sha_start: "9c7415404313b37eb41902654837df5c93d2b5d7"
|
||||
scope_sha_end: "801dd95f7372095cc06b9a6a1984f86038c5886b"
|
||||
reviewed_files_count: 2
|
||||
verdict: WARN
|
||||
findings:
|
||||
- 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
---
|
||||
|
||||
# Review: Add a retry to the HTTP client
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Verdict: WARN. One MAJOR finding: the retry loop has no upper bound on its total wait.
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| lib/http.mjs | deep-review | risk |
|
||||
| lib/config.mjs | summary-only | default |
|
||||
|
||||
## Findings (MAJOR)
|
||||
|
||||
### 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:41`
|
||||
- **Title:** The retry loop has no total time limit
|
||||
- **Recommended action:** Stop retrying once the total wait passes the configured limit.
|
||||
|
||||
## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
One MAJOR finding; no BLOCKER, so no remediation step is required.
|
||||
|
||||
```json
|
||||
{
|
||||
"verdict": "WARN",
|
||||
"counts": {
|
||||
"BLOCKER": 0,
|
||||
"MAJOR": 1,
|
||||
"MINOR": 0,
|
||||
"SUGGESTION": 0
|
||||
},
|
||||
"allow_blocked_by": [],
|
||||
"findings": [
|
||||
{
|
||||
"id": "77bc675af2ef7b4524b6a21b6fb23190bb56a32f",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 41,
|
||||
"title": "The retry loop has no total time limit",
|
||||
"detail": "A server that keeps answering 503 keeps the caller waiting without end.",
|
||||
"recommended_action": "Stop retrying once the total wait passes the configured limit.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review"
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
154
tests/fixtures/review-version/review-1-2-bad-answers.md
vendored
Normal file
154
tests/fixtures/review-version/review-1-2-bad-answers.md
vendored
Normal file
|
|
@ -0,0 +1,154 @@
|
|||
---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
created: 2026-11-02
|
||||
task: "Add a retry to the HTTP client"
|
||||
slug: http-retry
|
||||
project_dir: .claude/projects/2026-10-30-http-retry/
|
||||
brief_path: .claude/projects/2026-10-30-http-retry/brief.md
|
||||
scope_sha_start: "9c7415404313b37eb41902654837df5c93d2b5d7"
|
||||
scope_sha_end: "801dd95f7372095cc06b9a6a1984f86038c5886b"
|
||||
reviewed_files_count: 2
|
||||
verdict: WARN
|
||||
round: 1
|
||||
round_base: null
|
||||
review_policy: applied
|
||||
review_policy_note: "max-minor 10, two skip paths"
|
||||
review_policy_sha256: "823412d1eacb67956220e532959f0104603057c88704863ca38e7cd188fda812"
|
||||
finalized: true
|
||||
findings:
|
||||
- 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
- e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
- bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
---
|
||||
|
||||
# Review: Add a retry to the HTTP client
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Verdict: WARN. Two MAJOR findings on lines this range changed, one answered `accepted` and one `fixed`; one
|
||||
pre-existing MAJOR on a line the range did not change.
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| lib/http.mjs | deep-review | risk |
|
||||
| lib/config.mjs | summary-only | default |
|
||||
|
||||
## Findings (MAJOR)
|
||||
|
||||
### 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:41`
|
||||
- **Title:** The retry loop has no total time limit
|
||||
- **Recommended action:** Stop retrying once the total wait passes the configured limit.
|
||||
|
||||
### e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:58`
|
||||
- **Title:** A 429 answer is retried without reading Retry-After
|
||||
- **Recommended action:** Wait at least the Retry-After value before the next attempt.
|
||||
|
||||
### bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/config.mjs:12`
|
||||
- **Title:** The timeout default is read as seconds and used as milliseconds
|
||||
- **Pre-existing:** yes (the range did not add or change this line)
|
||||
|
||||
## Pre-existing
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| MAJOR | 1 |
|
||||
|
||||
- The timeout default is read as seconds and used as milliseconds (`lib/config.mjs:12`)
|
||||
|
||||
## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
Two counting MAJOR findings, both answered; no BLOCKER.
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
### Step R1: Honour Retry-After on a 429 answer
|
||||
|
||||
- **Files:** `lib/http.mjs`
|
||||
- **Source finding:** `e1fa7656c24e23e316f96169884da5f15059beb9`
|
||||
- **Test first:** a 429 with `Retry-After: 2` waits at least two seconds before the next attempt
|
||||
- **Verify:** re-run /trekreview → finding `e1fa7656c24e23e316f96169884da5f15059beb9` absent
|
||||
- **On failure:** revert
|
||||
|
||||
## Answers
|
||||
|
||||
| Finding | Answer | Reason | Record | Carried from |
|
||||
|---|---|---|---|---|
|
||||
| 77bc675af2ef7b4524b6a21b6fb23190bb56a32f | maybe | The caller sets its own deadline; a total limit here is a follow-up. | c97c121a37e664a6ccbe874f9b0afb22e21661305772e0c2939e61ddfdb74924 | - |
|
||||
| e1fa7656c24e23e316f96169884da5f15059beb9 | fixed | Fixed by Step R1. | 191904440c3a567c054e56ee1831204c422553732b255ad562f7034e54543554 | - |
|
||||
|
||||
```json
|
||||
{
|
||||
"verdict": "WARN",
|
||||
"counts": {
|
||||
"BLOCKER": 0,
|
||||
"MAJOR": 3,
|
||||
"MINOR": 0,
|
||||
"SUGGESTION": 0
|
||||
},
|
||||
"pre_existing_counts": {"BLOCKER": 0, "MAJOR": 1, "MINOR": 0, "SUGGESTION": 0},
|
||||
"resolved": [],
|
||||
"ran": {
|
||||
"engines": ["code-review", "security-review"],
|
||||
"reviewers": ["brief-conformance-reviewer", "code-correctness-reviewer"]
|
||||
},
|
||||
"allow_blocked_by": [],
|
||||
"findings": [
|
||||
{
|
||||
"id": "77bc675af2ef7b4524b6a21b6fb23190bb56a32f",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 41,
|
||||
"title": "The retry loop has no total time limit",
|
||||
"detail": "A server that keeps answering 503 keeps the caller waiting without end.",
|
||||
"recommended_action": "Stop retrying once the total wait passes the configured limit.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": false
|
||||
},
|
||||
{
|
||||
"id": "e1fa7656c24e23e316f96169884da5f15059beb9",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 58,
|
||||
"title": "A 429 answer is retried without reading Retry-After",
|
||||
"detail": "The client retries a 429 at once, so a rate-limited server is hit again before it allows it.",
|
||||
"recommended_action": "Wait at least the Retry-After value before the next attempt.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review"
|
||||
},
|
||||
{
|
||||
"id": "bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/config.mjs",
|
||||
"line": 12,
|
||||
"title": "The timeout default is read as seconds and used as milliseconds",
|
||||
"detail": "The default of 30 becomes a 30 ms timeout.",
|
||||
"recommended_action": "Multiply the default by 1000 where it is read.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": true
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
154
tests/fixtures/review-version/review-1-2-bad-policy.md
vendored
Normal file
154
tests/fixtures/review-version/review-1-2-bad-policy.md
vendored
Normal file
|
|
@ -0,0 +1,154 @@
|
|||
---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
created: 2026-11-02
|
||||
task: "Add a retry to the HTTP client"
|
||||
slug: http-retry
|
||||
project_dir: .claude/projects/2026-10-30-http-retry/
|
||||
brief_path: .claude/projects/2026-10-30-http-retry/brief.md
|
||||
scope_sha_start: "9c7415404313b37eb41902654837df5c93d2b5d7"
|
||||
scope_sha_end: "801dd95f7372095cc06b9a6a1984f86038c5886b"
|
||||
reviewed_files_count: 2
|
||||
verdict: WARN
|
||||
round: 1
|
||||
round_base: null
|
||||
review_policy: applied
|
||||
review_policy_note: "max-minor 10, two skip paths"
|
||||
review_policy_sha256: xyz
|
||||
finalized: true
|
||||
findings:
|
||||
- 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
- e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
- bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
---
|
||||
|
||||
# Review: Add a retry to the HTTP client
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Verdict: WARN. Two MAJOR findings on lines this range changed, one answered `accepted` and one `fixed`; one
|
||||
pre-existing MAJOR on a line the range did not change.
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| lib/http.mjs | deep-review | risk |
|
||||
| lib/config.mjs | summary-only | default |
|
||||
|
||||
## Findings (MAJOR)
|
||||
|
||||
### 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:41`
|
||||
- **Title:** The retry loop has no total time limit
|
||||
- **Recommended action:** Stop retrying once the total wait passes the configured limit.
|
||||
|
||||
### e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:58`
|
||||
- **Title:** A 429 answer is retried without reading Retry-After
|
||||
- **Recommended action:** Wait at least the Retry-After value before the next attempt.
|
||||
|
||||
### bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/config.mjs:12`
|
||||
- **Title:** The timeout default is read as seconds and used as milliseconds
|
||||
- **Pre-existing:** yes (the range did not add or change this line)
|
||||
|
||||
## Pre-existing
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| MAJOR | 1 |
|
||||
|
||||
- The timeout default is read as seconds and used as milliseconds (`lib/config.mjs:12`)
|
||||
|
||||
## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
Two counting MAJOR findings, both answered; no BLOCKER.
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
### Step R1: Honour Retry-After on a 429 answer
|
||||
|
||||
- **Files:** `lib/http.mjs`
|
||||
- **Source finding:** `e1fa7656c24e23e316f96169884da5f15059beb9`
|
||||
- **Test first:** a 429 with `Retry-After: 2` waits at least two seconds before the next attempt
|
||||
- **Verify:** re-run /trekreview → finding `e1fa7656c24e23e316f96169884da5f15059beb9` absent
|
||||
- **On failure:** revert
|
||||
|
||||
## Answers
|
||||
|
||||
| Finding | Answer | Reason | Record | Carried from |
|
||||
|---|---|---|---|---|
|
||||
| 77bc675af2ef7b4524b6a21b6fb23190bb56a32f | accepted | The caller sets its own deadline; a total limit here is a follow-up. | c97c121a37e664a6ccbe874f9b0afb22e21661305772e0c2939e61ddfdb74924 | - |
|
||||
| e1fa7656c24e23e316f96169884da5f15059beb9 | fixed | Fixed by Step R1. | 191904440c3a567c054e56ee1831204c422553732b255ad562f7034e54543554 | - |
|
||||
|
||||
```json
|
||||
{
|
||||
"verdict": "WARN",
|
||||
"counts": {
|
||||
"BLOCKER": 0,
|
||||
"MAJOR": 3,
|
||||
"MINOR": 0,
|
||||
"SUGGESTION": 0
|
||||
},
|
||||
"pre_existing_counts": {"BLOCKER": 0, "MAJOR": 1, "MINOR": 0, "SUGGESTION": 0},
|
||||
"resolved": [],
|
||||
"ran": {
|
||||
"engines": ["code-review", "security-review"],
|
||||
"reviewers": ["brief-conformance-reviewer", "code-correctness-reviewer"]
|
||||
},
|
||||
"allow_blocked_by": [],
|
||||
"findings": [
|
||||
{
|
||||
"id": "77bc675af2ef7b4524b6a21b6fb23190bb56a32f",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 41,
|
||||
"title": "The retry loop has no total time limit",
|
||||
"detail": "A server that keeps answering 503 keeps the caller waiting without end.",
|
||||
"recommended_action": "Stop retrying once the total wait passes the configured limit.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": false
|
||||
},
|
||||
{
|
||||
"id": "e1fa7656c24e23e316f96169884da5f15059beb9",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 58,
|
||||
"title": "A 429 answer is retried without reading Retry-After",
|
||||
"detail": "The client retries a 429 at once, so a rate-limited server is hit again before it allows it.",
|
||||
"recommended_action": "Wait at least the Retry-After value before the next attempt.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review"
|
||||
},
|
||||
{
|
||||
"id": "bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/config.mjs",
|
||||
"line": 12,
|
||||
"title": "The timeout default is read as seconds and used as milliseconds",
|
||||
"detail": "The default of 30 becomes a 30 ms timeout.",
|
||||
"recommended_action": "Multiply the default by 1000 where it is read.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": true
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
154
tests/fixtures/review-version/review-1-2.md
vendored
Normal file
154
tests/fixtures/review-version/review-1-2.md
vendored
Normal file
|
|
@ -0,0 +1,154 @@
|
|||
---
|
||||
type: trekreview
|
||||
review_version: "1.2"
|
||||
created: 2026-11-02
|
||||
task: "Add a retry to the HTTP client"
|
||||
slug: http-retry
|
||||
project_dir: .claude/projects/2026-10-30-http-retry/
|
||||
brief_path: .claude/projects/2026-10-30-http-retry/brief.md
|
||||
scope_sha_start: "9c7415404313b37eb41902654837df5c93d2b5d7"
|
||||
scope_sha_end: "801dd95f7372095cc06b9a6a1984f86038c5886b"
|
||||
reviewed_files_count: 2
|
||||
verdict: WARN
|
||||
round: 1
|
||||
round_base: null
|
||||
review_policy: applied
|
||||
review_policy_note: "max-minor 10, two skip paths"
|
||||
review_policy_sha256: "823412d1eacb67956220e532959f0104603057c88704863ca38e7cd188fda812"
|
||||
finalized: true
|
||||
findings:
|
||||
- 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
- e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
- bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
---
|
||||
|
||||
# Review: Add a retry to the HTTP client
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Verdict: WARN. Two MAJOR findings on lines this range changed, one answered `accepted` and one `fixed`; one
|
||||
pre-existing MAJOR on a line the range did not change.
|
||||
|
||||
## Coverage
|
||||
|
||||
| File | Treatment | Reason |
|
||||
|------|-----------|--------|
|
||||
| lib/http.mjs | deep-review | risk |
|
||||
| lib/config.mjs | summary-only | default |
|
||||
|
||||
## Findings (MAJOR)
|
||||
|
||||
### 77bc675af2ef7b4524b6a21b6fb23190bb56a32f
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:41`
|
||||
- **Title:** The retry loop has no total time limit
|
||||
- **Recommended action:** Stop retrying once the total wait passes the configured limit.
|
||||
|
||||
### e1fa7656c24e23e316f96169884da5f15059beb9
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/http.mjs:58`
|
||||
- **Title:** A 429 answer is retried without reading Retry-After
|
||||
- **Recommended action:** Wait at least the Retry-After value before the next attempt.
|
||||
|
||||
### bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb
|
||||
|
||||
- **Rule:** `CODE_REVIEW_DEFECT`
|
||||
- **Location:** `lib/config.mjs:12`
|
||||
- **Title:** The timeout default is read as seconds and used as milliseconds
|
||||
- **Pre-existing:** yes (the range did not add or change this line)
|
||||
|
||||
## Pre-existing
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| MAJOR | 1 |
|
||||
|
||||
- The timeout default is read as seconds and used as milliseconds (`lib/config.mjs:12`)
|
||||
|
||||
## Declined to Judge
|
||||
|
||||
None.
|
||||
|
||||
Trimmed: 0
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
Two counting MAJOR findings, both answered; no BLOCKER.
|
||||
|
||||
## Remediation Steps
|
||||
|
||||
### Step R1: Honour Retry-After on a 429 answer
|
||||
|
||||
- **Files:** `lib/http.mjs`
|
||||
- **Source finding:** `e1fa7656c24e23e316f96169884da5f15059beb9`
|
||||
- **Test first:** a 429 with `Retry-After: 2` waits at least two seconds before the next attempt
|
||||
- **Verify:** re-run /trekreview → finding `e1fa7656c24e23e316f96169884da5f15059beb9` absent
|
||||
- **On failure:** revert
|
||||
|
||||
## Answers
|
||||
|
||||
| Finding | Answer | Reason | Record | Carried from |
|
||||
|---|---|---|---|---|
|
||||
| 77bc675af2ef7b4524b6a21b6fb23190bb56a32f | accepted | The caller sets its own deadline; a total limit here is a follow-up. | c97c121a37e664a6ccbe874f9b0afb22e21661305772e0c2939e61ddfdb74924 | - |
|
||||
| e1fa7656c24e23e316f96169884da5f15059beb9 | fixed | Fixed by Step R1. | 191904440c3a567c054e56ee1831204c422553732b255ad562f7034e54543554 | - |
|
||||
|
||||
```json
|
||||
{
|
||||
"verdict": "WARN",
|
||||
"counts": {
|
||||
"BLOCKER": 0,
|
||||
"MAJOR": 3,
|
||||
"MINOR": 0,
|
||||
"SUGGESTION": 0
|
||||
},
|
||||
"pre_existing_counts": {"BLOCKER": 0, "MAJOR": 1, "MINOR": 0, "SUGGESTION": 0},
|
||||
"resolved": [],
|
||||
"ran": {
|
||||
"engines": ["code-review", "security-review"],
|
||||
"reviewers": ["brief-conformance-reviewer", "code-correctness-reviewer"]
|
||||
},
|
||||
"allow_blocked_by": [],
|
||||
"findings": [
|
||||
{
|
||||
"id": "77bc675af2ef7b4524b6a21b6fb23190bb56a32f",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 41,
|
||||
"title": "The retry loop has no total time limit",
|
||||
"detail": "A server that keeps answering 503 keeps the caller waiting without end.",
|
||||
"recommended_action": "Stop retrying once the total wait passes the configured limit.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": false
|
||||
},
|
||||
{
|
||||
"id": "e1fa7656c24e23e316f96169884da5f15059beb9",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 58,
|
||||
"title": "A 429 answer is retried without reading Retry-After",
|
||||
"detail": "The client retries a 429 at once, so a rate-limited server is hit again before it allows it.",
|
||||
"recommended_action": "Wait at least the Retry-After value before the next attempt.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review"
|
||||
},
|
||||
{
|
||||
"id": "bd517c3b965addbf8db5fdc6c04478e3e0f3c6cb",
|
||||
"severity": "MAJOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/config.mjs",
|
||||
"line": 12,
|
||||
"title": "The timeout default is read as seconds and used as milliseconds",
|
||||
"detail": "The default of 30 becomes a 30 ms timeout.",
|
||||
"recommended_action": "Multiply the default by 1000 where it is read.",
|
||||
"engine": "code-review",
|
||||
"reviewer": "claude-code-review",
|
||||
"pre_existing": true
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
|
|
@ -126,7 +126,7 @@ test('[M4] Handover 6 names the review-not-measured reason', () => {
|
|||
|
||||
test('[M4] Handover 6 lists review_version 1.1 as current', () => {
|
||||
const row = fieldRow('review_version');
|
||||
assert.match(row, /"1\.1"`? \(current\)/, `the review_version row does not list 1.1 as current: ${row}`);
|
||||
assert.match(row, /"1\.2"`? \(current\)/, `the review_version row does not list 1.2 as current: ${row}`);
|
||||
assert.doesNotMatch(row, /"1\.0"`? \(current\)/, `the review_version row still lists 1.0 as current: ${row}`);
|
||||
});
|
||||
|
||||
|
|
|
|||
116
tests/lib/fix-round-4-5b2-b-prose.test.mjs
Normal file
116
tests/lib/fix-round-4-5b2-b-prose.test.mjs
Normal file
|
|
@ -0,0 +1,116 @@
|
|||
// tests/lib/fix-round-4-5b2-b-prose.test.mjs
|
||||
// Prose pins for fix round 4.5b2, part B (plan
|
||||
// .claude/projects/2026-10-31-review-gate-4-5b2): the parts of the round that
|
||||
// are text a session reads. The tag in brackets is the part-B id from the
|
||||
// round's plan; each tagged test fails on an assertion at the step's base.
|
||||
// V12 review_version 1.2 is current: Handover 6's table and field row name
|
||||
// it, no line of docs/HANDOVER-CONTRACTS.md still names 1.1 as current,
|
||||
// the Versioning paragraph says what 1.1 -> 1.2 changed, MIGRATION.md
|
||||
// holds the 1.2 section with its N-1 window, and every other place that
|
||||
// states the current review_version says 1.2.
|
||||
// The helpers below are copied, never imported from an earlier prose test.
|
||||
// Fix the SOURCE a pin guards, not the pin.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { existsSync, readFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
|
||||
const readCache = new Map();
|
||||
|
||||
/** A repo file's text; the file must exist. Read from disk once per run. */
|
||||
// Copied from tests/lib/fix-round-4-5b2-a-prose.test.mjs (read).
|
||||
function read(rel) {
|
||||
if (!readCache.has(rel)) {
|
||||
assert.ok(existsSync(join(ROOT, rel)), `missing file ${rel}`);
|
||||
readCache.set(rel, readFileSync(join(ROOT, rel), 'utf8'));
|
||||
}
|
||||
return readCache.get(rel);
|
||||
}
|
||||
|
||||
/** Whitespace (line breaks too) collapsed to one space. */
|
||||
// Copied from tests/lib/fix-round-4-5b2-a-prose.test.mjs (flat).
|
||||
const flat = (s) => s.replace(/\s+/g, ' ').trim();
|
||||
|
||||
/** The text from `heading` up to the next `## ` heading; the heading must exist. */
|
||||
// Copied from tests/lib/review-prose-pins.test.mjs (section).
|
||||
function section(text, heading) {
|
||||
const start = text.indexOf(heading);
|
||||
assert.ok(start >= 0, `heading missing: ${heading}`);
|
||||
const next = text.indexOf('\n## ', start + heading.length);
|
||||
return text.slice(start, next === -1 ? undefined : next);
|
||||
}
|
||||
|
||||
/** Every sentence of `text` (flattened). */
|
||||
const sentences = (text) => flat(text).split(/(?<=\.)\s+(?=[A-Z`"*(])/);
|
||||
|
||||
const CONTRACTS = 'docs/HANDOVER-CONTRACTS.md';
|
||||
|
||||
// --- V12: review_version 1.2 is current ------------------------------------------
|
||||
|
||||
test('[V12] docs/HANDOVER-CONTRACTS.md names 1.2 as current in the table and the row, and no line names 1.1 as current', () => {
|
||||
const text = read(CONTRACTS);
|
||||
const tableRows = text.split('\n').filter((l) => l.startsWith('| `review.md` | `review_version`'));
|
||||
assert.equal(tableRows.length, 1, `the versioning table has one review.md row; found ${tableRows.length}`);
|
||||
assert.match(tableRows[0], /\|\s*`1\.2`\s*\|\s*$/, `the versioning table's review.md row says 1.2 is current: ${tableRows[0]}`);
|
||||
|
||||
const h6 = section(text, '## Handover 6');
|
||||
const fieldRows = h6.split('\n').filter((l) => l.startsWith('| `review_version` |'));
|
||||
assert.equal(fieldRows.length, 1, `Handover 6 has one review_version row; found ${fieldRows.length}`);
|
||||
assert.match(fieldRows[0], /"1\.2"`? \(current\)/, `the review_version row lists 1.2 as current: ${fieldRows[0]}`);
|
||||
|
||||
const stale = text.split('\n').filter((l) =>
|
||||
/1\.1"?`?\s*\(current\)/.test(l)
|
||||
|| /current is `?1\.1\b/.test(l)
|
||||
|| (l.startsWith('| `review.md` | `review_version`') && /`1\.1`/.test(l)));
|
||||
assert.deepEqual(stale, [], `no line of ${CONTRACTS} may name review_version 1.1 as current`);
|
||||
});
|
||||
|
||||
test('[V12] the Handover 6 Versioning paragraph has the 1.1 -> 1.2 sentence', () => {
|
||||
const h6 = section(read(CONTRACTS), '## Handover 6');
|
||||
const paras = h6.split('\n').filter((l) => l.startsWith('**Versioning:**'));
|
||||
assert.equal(paras.length, 1, `Handover 6 has one Versioning paragraph; found ${paras.length}`);
|
||||
const para = flat(paras[0]);
|
||||
assert.match(para, /current is `1\.2`/, `the Versioning paragraph says current is 1.2: ${para}`);
|
||||
const hits = sentences(para).filter((s) => /1\.1/.test(s) && /1\.2/.test(s));
|
||||
assert.ok(hits.length > 0, `no sentence of the Versioning paragraph names both 1.1 and 1.2: ${para}`);
|
||||
assert.ok(hits.some((s) => /still validates/.test(s)),
|
||||
`the 1.1 -> 1.2 sentence says a 1.1 review still validates: ${hits.join(' | ')}`);
|
||||
assert.ok(hits.some((s) => /never opens/.test(s) && /unanswered MAJOR/.test(s) && /lock/.test(s)),
|
||||
`the 1.1 -> 1.2 sentence says a 1.1 review never opens the lock with an unanswered MAJOR: ${hits.join(' | ')}`);
|
||||
});
|
||||
|
||||
test('[V12] MIGRATION.md has the review_version 1.2 section with its N-1 window', () => {
|
||||
const text = read('MIGRATION.md');
|
||||
const heads = text.split('\n').filter((l) => /^## review_version 1\.2\b/.test(l));
|
||||
assert.deepEqual(heads, ['## review_version 1.2 (from the next release)'],
|
||||
'MIGRATION.md has exactly one "## review_version 1.2 (from the next release)" section');
|
||||
const body = flat(section(text, heads[0]));
|
||||
assert.match(body, /\bN-1\b/, `the 1.2 section names the N-1 window: ${body}`);
|
||||
assert.match(body, /\b1\.1\b/, `the 1.2 section says what happens to a 1.1 review: ${body}`);
|
||||
});
|
||||
|
||||
test('[V12] every current review_version statement names 1.2', () => {
|
||||
const tpl = read('templates/trekreview-template.md');
|
||||
assert.match(tpl.split('\n')[2], /^review_version: "1\.2"$/, 'the template\'s frontmatter says review_version "1.2"');
|
||||
assert.ok(!tpl.includes('review_version: "1.1"'), 'the template no longer writes review_version "1.1"');
|
||||
|
||||
const coord = read('agents/review-coordinator.md');
|
||||
assert.ok(coord.includes('review_version: "1.2"'), 'the coordinator writes review_version "1.2"');
|
||||
assert.ok(!coord.includes('review_version: "1.1"'), 'the coordinator no longer writes review_version "1.1"');
|
||||
|
||||
const cmd = read('commands/trekreview.md');
|
||||
assert.match(cmd, /\*\*The review is a gate \(review_version 1\.2,/, '/trekreview says the review is a gate at review_version 1.2');
|
||||
assert.doesNotMatch(cmd, /\(review_version 1\.1,/, '/trekreview no longer says review_version 1.1 is current');
|
||||
|
||||
const claude = read('CLAUDE.md');
|
||||
assert.ok(claude.includes('**Review is a gate (review_version 1.2).**'), 'CLAUDE.md says the review gate is review_version 1.2');
|
||||
assert.ok(!claude.includes('(review_version 1.1)'), 'CLAUDE.md no longer names review_version 1.1');
|
||||
|
||||
const arch = read('docs/architecture.md');
|
||||
assert.ok(arch.includes('review_version 1.2'), 'docs/architecture.md names review_version 1.2');
|
||||
assert.ok(!arch.includes('review_version 1.1'), 'docs/architecture.md no longer names review_version 1.1');
|
||||
});
|
||||
|
|
@ -76,7 +76,7 @@ test('coordinator — writes ## Declined to Judge with the trim count', () => {
|
|||
// ---- K7: the verdict in the file, and the gate -------------------------------
|
||||
|
||||
test('coordinator — review_version 1.1 frontmatter carries verdict', () => {
|
||||
assert.ok(COORD.includes('review_version: "1.1"'));
|
||||
assert.ok(COORD.includes('review_version: "1.2"'));
|
||||
assert.match(COORD, /\nverdict: \{BLOCK \| WARN \| ALLOW\}\n/);
|
||||
});
|
||||
|
||||
|
|
|
|||
116
tests/lib/review-validator-1-2.test.mjs
Normal file
116
tests/lib/review-validator-1-2.test.mjs
Normal file
|
|
@ -0,0 +1,116 @@
|
|||
// tests/lib/review-validator-1-2.test.mjs
|
||||
// Fix round 4.5b2 part B, Step 2 (tag V12): review_version 1.2 validates, and
|
||||
// a 1.1 review still does (N-1 window). Every new 1.2 field is optional; a
|
||||
// malformed one present is a strict error (a soft warning in --soft) with one
|
||||
// of four codes. Each [V12] case fails at its base on an assertion: the
|
||||
// fixtures are new, so each is read only after `assert.ok(existsSync(p))`,
|
||||
// and the base validator knows no 1.2 field, so a malformed one passes there.
|
||||
// The kept behaviour (a 1.1 review validates, unknown keys are tolerated)
|
||||
// sits inside the tagged tests as "Control:" assertions.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { existsSync, readFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { validateReviewContent } from '../../lib/validators/review-validator.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const FIXTURES = 'tests/fixtures/review-version';
|
||||
|
||||
/** A fixture's text; the fixture must exist (they are new in this step). */
|
||||
function fixture(name) {
|
||||
const p = join(ROOT, FIXTURES, name);
|
||||
assert.ok(existsSync(p), `missing fixture ${FIXTURES}/${name}`);
|
||||
return readFileSync(p, 'utf-8');
|
||||
}
|
||||
|
||||
const codes = (list) => list.map((i) => i.code);
|
||||
|
||||
// The ids the 1.2 fixture uses (written down here, independent of the validator).
|
||||
const FIXED_ID = 'e1fa7656c24e23e316f96169884da5f15059beb9';
|
||||
|
||||
/** `text` with `from` replaced once; `from` must be in it, or the mutation proves nothing. */
|
||||
function mutate(text, from, to) {
|
||||
assert.ok(text.includes(from), `mutation source missing from the fixture: ${JSON.stringify(from)}`);
|
||||
return text.replace(from, to);
|
||||
}
|
||||
|
||||
test('[V12] the 1.2 fixture validates strictly and the 1.1 fixture validates', () => {
|
||||
const v12 = fixture('review-1-2.md');
|
||||
assert.match(v12, /^review_version: "1\.2"$/m, 'the 1.2 fixture declares review_version "1.2"');
|
||||
const r = validateReviewContent(v12, { strict: true });
|
||||
assert.equal(r.valid, true, `the 1.2 fixture is valid in strict mode: ${JSON.stringify(r.errors)}`);
|
||||
assert.deepEqual(r.errors, []);
|
||||
|
||||
// The 1.2 fields of the fixture are read, not merely tolerated: the same file
|
||||
// with an out-of-range round is refused.
|
||||
const badRound = mutate(v12, '\nround: 1\n', '\nround: 4\n');
|
||||
const rb = validateReviewContent(badRound, { strict: true });
|
||||
assert.equal(rb.valid, false, 'round: 4 is outside 1-3 and must be refused at 1.2');
|
||||
assert.ok(codes(rb.errors).includes('REVIEW_ROUND_MALFORMED'), JSON.stringify(rb.errors));
|
||||
|
||||
// Control: a 1.1 review validates as before (N-1 window), strict and soft.
|
||||
const v11 = fixture('review-1-1.md');
|
||||
assert.match(v11, /^review_version: "1\.1"$/m, 'the 1.1 fixture declares review_version "1.1"');
|
||||
assert.match(v11, /^verdict: WARN$/m, 'the 1.1 fixture is a WARN review');
|
||||
const r11 = validateReviewContent(v11, { strict: true });
|
||||
assert.equal(r11.valid, true, `the 1.1 fixture is valid in strict mode: ${JSON.stringify(r11.errors)}`);
|
||||
assert.equal(validateReviewContent(v11, { strict: false }).valid, true, 'the 1.1 fixture is valid in soft mode');
|
||||
|
||||
// Control: at 1.1 the new keys are unknown keys, and unknown keys stay tolerated.
|
||||
const v11Extra = mutate(v11, '\nverdict: WARN\n', '\nverdict: WARN\nround: 9\nreview_policy_sha256: xyz\n');
|
||||
assert.equal(validateReviewContent(v11Extra, { strict: true }).valid, true,
|
||||
'a 1.1 review is not held to the 1.2 field shapes');
|
||||
});
|
||||
|
||||
test('[V12] the bad-answers and bad-policy fixtures fail with their codes', () => {
|
||||
const badAnswers = fixture('review-1-2-bad-answers.md');
|
||||
assert.match(badAnswers, /^\| [0-9a-f]{40} \| maybe \|/m, 'the bad-answers fixture has a row answered "maybe"');
|
||||
const ra = validateReviewContent(badAnswers, { strict: true });
|
||||
assert.equal(ra.valid, false, 'an ## Answers row with answer "maybe" is refused in strict mode');
|
||||
assert.ok(codes(ra.errors).includes('REVIEW_ANSWERS_MALFORMED'), JSON.stringify(ra.errors));
|
||||
const sa = validateReviewContent(badAnswers, { strict: false });
|
||||
assert.equal(sa.valid, true, 'soft mode reads it with a warning, not an error');
|
||||
assert.ok(codes(sa.warnings).includes('REVIEW_ANSWERS_MALFORMED'), JSON.stringify(sa.warnings));
|
||||
|
||||
const badPolicy = fixture('review-1-2-bad-policy.md');
|
||||
assert.match(badPolicy, /^review_policy_sha256: xyz$/m, 'the bad-policy fixture has review_policy_sha256: xyz');
|
||||
const rp = validateReviewContent(badPolicy, { strict: true });
|
||||
assert.equal(rp.valid, false, 'review_policy_sha256: xyz is refused in strict mode');
|
||||
assert.ok(codes(rp.errors).includes('REVIEW_POLICY_MALFORMED'), JSON.stringify(rp.errors));
|
||||
const sp = validateReviewContent(badPolicy, { strict: false });
|
||||
assert.equal(sp.valid, true, 'soft mode reads it with a warning, not an error');
|
||||
assert.ok(codes(sp.warnings).includes('REVIEW_POLICY_MALFORMED'), JSON.stringify(sp.warnings));
|
||||
});
|
||||
|
||||
test('[V12] a `fixed` answer without its R-step fails', () => {
|
||||
const v12 = fixture('review-1-2.md');
|
||||
assert.match(v12, new RegExp(`^\\| ${FIXED_ID} \\| fixed \\|`, 'm'), 'the 1.2 fixture answers one MAJOR "fixed"');
|
||||
// Remove the one ### Step R<n> that names the fixed finding.
|
||||
const step = v12.match(/^### Step R1\b[\s\S]*?(?=^## )/m);
|
||||
assert.ok(step && step[0].includes(FIXED_ID), 'the 1.2 fixture has a ### Step R1 naming the fixed finding');
|
||||
const noStep = v12.replace(step[0], '');
|
||||
const r = validateReviewContent(noStep, { strict: true });
|
||||
assert.equal(r.valid, false, 'a MAJOR answered "fixed" without a ### Step R<n> naming it is refused');
|
||||
const hit = r.errors.find((e) => e.code === 'REVIEW_ANSWERS_MALFORMED' && e.message.includes(FIXED_ID));
|
||||
assert.ok(hit, `the error names the fixed finding: ${JSON.stringify(r.errors)}`);
|
||||
});
|
||||
|
||||
test('[V12] a malformed round, pre-existing or ran field at 1.2 fails with its code', () => {
|
||||
const v12 = fixture('review-1-2.md');
|
||||
const cases = [
|
||||
['round_base not full hex', '\nround_base: null\n', '\nround_base: abc123\n', 'REVIEW_ROUND_MALFORMED'],
|
||||
['finalized not boolean', '\nfinalized: true\n', '\nfinalized: yes\n', 'REVIEW_ROUND_MALFORMED'],
|
||||
['pre_existing not boolean', '"pre_existing": true', '"pre_existing": "yes"', 'REVIEW_PRE_EXISTING_MALFORMED'],
|
||||
['pre_existing_counts not counts', '"pre_existing_counts": {"BLOCKER": 0, "MAJOR": 1, "MINOR": 0, "SUGGESTION": 0}',
|
||||
'"pre_existing_counts": {"MAJOR": "one"}', 'REVIEW_PRE_EXISTING_MALFORMED'],
|
||||
['ran engine unknown', '"engines": ["code-review", "security-review"]', '"engines": ["code-review", "lint"]', 'REVIEW_ROUND_MALFORMED'],
|
||||
['review_policy unknown', '\nreview_policy: applied\n', '\nreview_policy: maybe\n', 'REVIEW_POLICY_MALFORMED'],
|
||||
];
|
||||
for (const [name, from, to, code] of cases) {
|
||||
const r = validateReviewContent(mutate(v12, from, to), { strict: true });
|
||||
assert.equal(r.valid, false, `${name}: refused in strict mode`);
|
||||
assert.ok(codes(r.errors).includes(code), `${name}: ${code} expected, got ${JSON.stringify(r.errors)}`);
|
||||
}
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue