feat(reviews): /release-review periodic full-codebase review#7719
Draft
JohnMcLear wants to merge 26 commits into
Draft
feat(reviews): /release-review periodic full-codebase review#7719JohnMcLear wants to merge 26 commits into
JohnMcLear wants to merge 26 commits into
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Review Summary by Qodo(Agentic_describe updated until commit a83054c)feat(reviews): /release-review periodic full-codebase review system with three-phase orchestration
WalkthroughsDescription• Implements /release-review — a comprehensive periodic full-codebase review system for Claude Code, designed to run once per release version • **Three-phase orchestrator architecture:** - Phase 1: Deterministic tools sweep (pnpm audit, osv-scanner, semgrep, eslint, madge, depcheck) via single subagent - Phase 2: Four parallel AI subagents audit assigned subsystems (auth/sessions, realtime-api, pad-changeset, db-supply-chain) - Phase 3: Aggregation, deduplication, suppression filtering, auto-triage classification, and interactive walkthrough • **Core utilities in src/node/utils/releaseReview/:** - fingerprint.ts: Stable sha256 hashing over ruleId::file::5-line-context survives line-shift drift but breaks on logic changes - aggregate.ts: Merges findings, deduplicates by fingerprint, applies severity floor and suppression filtering - triage.ts: Auto-classifies findings into Fix-now/Issue/Suppress buckets using heuristics - suppression.ts: Loads/validates known-findings.yml with strict schema and date coercion for js-yaml - cli.ts: Orchestrates phases with commands: aggregate, triage, append-suppression, summary - runDir.ts, summary.ts: Run directory management and markdown summary generation • **Comprehensive test coverage:** 53 unit tests validating fingerprint stability, aggregation deduplication, triage classification, suppression handling, and CLI commands • **Documentation:** Design specification, implementation plan, operator guide, and five subagent prompts (tools, auth-sessions, realtime-api, pad-changeset, db-supply) • **Dependencies:** Adds js-yaml@^4.1.1 as direct dependency for YAML parsing • **Slash command:** .claude/commands/release-review.md orchestrates the full workflow with --resume support for continuing interrupted runs Diagramflowchart LR
Setup["Setup: Allocate run-id"]
Phase1["Phase 1: Tools Sweep<br/>pnpm audit, osv-scanner,<br/>semgrep, eslint, madge, depcheck"]
Phase2["Phase 2: AI Subagents<br/>auth-sessions, realtime-api,<br/>pad-changeset, db-supply"]
Aggregate["Aggregate: Deduplicate<br/>by fingerprint, apply<br/>suppression & severity floor"]
Triage["Triage: Auto-classify<br/>into Fix-now/Issue/Suppress"]
Walkthrough["Walkthrough: Interactive<br/>decision & suppression update"]
Summary["Summary: Generate<br/>markdown report"]
Setup --> Phase1
Phase1 --> Phase2
Phase2 --> Aggregate
Aggregate --> Triage
Triage --> Walkthrough
Walkthrough --> Summary
File Changes1. src/tests/backend/specs/releaseReview-utils.ts
|
Code Review by Qodo
1. Fingerprint collides on missing context
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+35
to
+50
| const readLines = (file: string): string[] => { | ||
| const abs = path.isAbsolute(file) ? file : path.join(repoRoot, file); | ||
| if (!fileLineCache.has(abs)) { | ||
| fileLineCache.set( | ||
| abs, | ||
| fs.existsSync(abs) ? fs.readFileSync(abs, 'utf8').split('\n') : [], | ||
| ); | ||
| } | ||
| return fileLineCache.get(abs)!; | ||
| }; | ||
| const enrich = (raw: any): Finding => { | ||
| // Subagent JSON may be top-level array OR {findings: [...]}. | ||
| if (raw.fingerprint) return raw; | ||
| const lines = readLines(raw.file); | ||
| const fp = computeFingerprint(raw.ruleId, raw.file, raw.line, lines); | ||
| return {...raw, fingerprint: fp}; |
There was a problem hiding this comment.
1. Path traversal file read 🐞 Bug ⛨ Security
The aggregate CLI reads file contents for fingerprinting using the file field from subagent JSON, allowing absolute paths and .. segments without verifying the resolved path stays inside repoRoot. A malformed/malicious findings JSON can cause the CLI to read arbitrary host files when computing fingerprints.
Agent Prompt
## Issue description
`cli.ts aggregate` reads `raw.file` from subagent JSON and uses it to read file contents for fingerprint computation. Because the path is not normalized/validated, findings can reference absolute paths or use `..` to escape `repoRoot`, leading to arbitrary file reads.
## Issue Context
Even if subagents are “internal”, their output is effectively untrusted input. The aggregator should defensively ensure it only reads files inside the repository root.
## Fix Focus Areas
- src/node/utils/releaseReview/cli.ts[31-50]
## Suggested fix
- Resolve paths via `path.resolve()`.
- Compute `repoRootAbs = path.resolve(repoRoot)` once.
- For each `raw.file`:
- `abs = path.resolve(repoRootAbs, raw.file)` (even if `raw.file` is absolute, consider rejecting it or normalizing it to a repo-relative path)
- Reject if `path.relative(repoRootAbs, abs).startsWith('..'+path.sep)` or equals `..` (escape attempt).
- Use the validated `abs` for reading, and use a normalized repo-relative (POSIX) path for fingerprinting/suppression stability.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Three-phase orchestrator (tools sweep + 4 parallel AI subsystem sweeps + interactive auto-triage walkthrough) with fingerprinted finding suppression in docs/reviews/known-findings.yml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
17 tasks: TDD-style helper modules (types, fingerprint, suppression, aggregate, triage, runDir, summary), CLI entry point, five subagent prompts (tools + 4 subsystems), slash command orchestrator, README, end-to-end smoke test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… module Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…var persistence) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JohnMcLear
force-pushed
the
chore/release-review-design
branch
from
9812852 to
a83054c
Compare
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit a83054c |
Comment on lines
+25
to
+30
| const idx = line - 1; | ||
| const start = Math.max(0, idx - 2); | ||
| const end = Math.min(lines.length, idx + 3); | ||
| const context = lines.slice(start, end).map((l) => l.trim()).join('\n'); | ||
| const payload = `${ruleId}::${file}::${context}`; | ||
| return createHash('sha256').update(payload).digest('hex'); |
There was a problem hiding this comment.
1. Fingerprint collides on missing context 🐞 Bug ≡ Correctness
computeFingerprint() hashes only the trimmed context window; if the file is missing/unreadable (or the reported line is far out of range such that the slice is empty), the context becomes empty and all such findings collapse to the same fingerprint for a given ruleId+file. This breaks dedupe/suppression by merging unrelated findings into one.
Agent Prompt
## Issue description
`computeFingerprint()` can produce identical fingerprints when it cannot extract any context (e.g., file missing/unreadable => `lines=[]`, or `start>=end`), because the payload becomes `${ruleId}::${file}::`.
## Issue Context
This happens today because the CLI’s `readLines()` returns an empty array when a file doesn’t exist, and `computeFingerprint()` does not add any other distinguishing input when the context slice is empty.
## Fix Focus Areas
- src/node/utils/releaseReview/fingerprint.ts[25-30]
## Implementation notes
- Detect the “no context” case (`lines.length===0` or `start>=end`).
- In that case, incorporate a deterministic fallback into the payload (e.g., include `line` and a sentinel like `__missing_file__` / `__out_of_range__:${line}/${lines.length}`) so distinct findings don’t collide.
- Keep the existing context-based behavior when context is present, to preserve the intended line-shift stability.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+45
| const winner = SEVERITY_RANK[annotated.severity] > SEVERITY_RANK[existing.severity] | ||
| ? annotated | ||
| : existing; | ||
| const sources = new Set([existing.source, annotated.source].flatMap((s) => s.split(','))); | ||
| byFingerprint.set(annotated.fingerprint, { | ||
| ...winner, | ||
| source: [...sources].join(','), | ||
| }); |
There was a problem hiding this comment.
2. Dedupe drops higher category 🐞 Bug ≡ Correctness
aggregate() dedupes by fingerprint but chooses the merged representative only by severity; when severities tie, the chosen category/message/remediationHint becomes order-dependent and can downgrade a CVE-category finding to bug/perf/etc. This mis-prioritizes findings and can push security-relevant items into the wrong downstream triage/sort order.
Agent Prompt
## Issue description
When merging two findings with the same fingerprint, `aggregate()` selects a winner based only on severity. If the severities are equal but categories differ (e.g., `cve` vs `bug`), the result keeps whichever category appeared first, even though a category rank exists.
## Issue Context
`CATEGORY_RANK` is defined and used for sorting, but not for deciding which finding’s fields to keep during dedupe.
## Fix Focus Areas
- src/node/utils/releaseReview/aggregate.ts[38-45]
## Implementation notes
- Update the winner selection to:
- Prefer higher severity.
- If equal severity, prefer higher `CATEGORY_RANK`.
- Consider also merging/selecting other key fields deterministically (e.g., prefer a non-empty `remediationHint`, and possibly keep the “most informative” `message`) so the merged record is not order-dependent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
/release-review— a Claude Code slash command for periodic full-codebase Medium+ reviews intended to run once per release version, complementing the existing CodeQL + dependency-review CI.Three-phase orchestrator:
pnpm audit,osv-scanner,semgrep,eslint,madge,depcheckvia a single subagent that emits normalized findings JSON.src/node/utils/releaseReview/deduplicate by stable fingerprint, apply suppression fromdocs/reviews/known-findings.yml, classify findings into Fix-now / Issue / Suppress buckets, and walk through them interactively.What lands in the repo
.claude/commands/release-review.md— the slash command (the orchestrator)src/node/utils/releaseReview/— types, fingerprint, suppression, aggregate, triage, runDir, summary, CLI entrydocs/reviews/prompts/{tools,auth-sessions,realtime-api,pad-changeset,db-supply}.md— subagent promptsdocs/reviews/known-findings.yml— suppression file (starts empty)docs/reviews/README.md— operator guidedocs/superpowers/specs/2026-05-09-release-review-design.md— design rationaledocs/superpowers/plans/2026-05-09-release-review.md— implementation plansrc/tests/backend/specs/releaseReview-utils.tsDesign notes
ruleId::file::5-line-trimmed-context-window, so suppression survives line-shift drift but breaks on real logic changes.aggregateCLI command computes them deterministically from the file's current content, with arepoRootargument so paths resolve correctly underpnpm exec.Dateobjects.Test plan
pnpm run ts-check— clean (verified)pnpm --filter ep_etherpad-lite run test-utils— 53 passing (verified)cli.ts aggregatefilters by severity floor, computes fingerprints from real file content, classifies into buckets (verified)cli.ts append-suppressionpreserves header comments inknown-findings.yml(verified)/release-reviewend-to-end on develop (deferred to first real use)🤖 Generated with Claude Code