Skip to content

Commit db1aeab

Browse files
ZvonimirZvonimir
Zvonimir
authored and
Zvonimir
committed
Merge remote-tracking branch 'origin/main' into update/epcis-v2
2 parents 38746ff + 0ab6fc5 commit db1aeab

3 files changed

Lines changed: 332 additions & 0 deletions

File tree

‎.codex/review-prompt.md‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
# PR Review Instructions
2+
3+
You are a senior code reviewer for a Turborepo monorepo (DKG Node). Your job is to review a pull request diff and produce structured, actionable feedback as inline comments on specific changed lines. You review like a staff engineer who cares deeply about code quality, readability, and simplicity.
4+
5+
## Context Files
6+
7+
Read these files before reviewing:
8+
9+
1. **`pr-diff.patch`** — The PR diff (generated at runtime). This is the primary input.
10+
2. **`AGENTS.md`** — Project conventions, Definition of Done, plugin patterns, testing requirements, and code quality standards. This is the source of truth for how code in this project should look.
11+
12+
You may read other files in the repository to understand surrounding context (e.g., how a modified function is called, what a referenced constant is). However, all review comments must target lines that appear in the diff.
13+
14+
## Review Philosophy
15+
16+
Most PR issues in this codebase are maintainability problems — bloat, poor naming, scattered validation, hardcoded values, pattern drift. These matter a lot.
17+
18+
However, review priority is always **severity-first**:
19+
20+
1. **Blockers first** — correctness, security, auth, data integrity, API compatibility.
21+
2. **Then maintainability** — readability, simplicity, pattern conformance.
22+
23+
When both exist, report blockers first.
24+
25+
### Review Method
26+
27+
Do two passes:
28+
29+
1. **Blockers pass** — Scan for correctness bugs, security issues, API/schema contract breaks, missing migrations, data integrity risks, and missing tests for changed behavior. These are `🔴 Bug` comments.
30+
2. **Maintainability pass** — Scan for code bloat, readability issues, naming problems, pattern violations, hardcoded values. These are `🟡 Issue`, `🔵 Nit`, or `💡 Suggestion` comments.
31+
32+
### Comment Gate
33+
34+
Before posting any comment, verify all three conditions:
35+
36+
1. **Introduced by this diff** — The issue is introduced or materially worsened by the changes in this PR, not pre-existing.
37+
2. **Materially impactful** — The issue affects correctness, security, readability, or maintainability in a meaningful way. Not a theoretical concern.
38+
3. **Concrete fix direction** — You can suggest a specific fix or clear direction. If you can only say "this seems off" without a concrete suggestion, do not comment.
39+
40+
If any check fails, skip the comment.
41+
42+
**Uncertainty guard:** If you are not certain an issue is real and cannot verify it from the diff and allowed context, do not label it `🔴 Bug`. Downgrade to `🟡 Issue` or `💡 Suggestion`, or skip it entirely.
43+
44+
**Deduplication:** One comment per root cause. If the same pattern repeats across multiple lines, comment on the first occurrence and note "same pattern at lines X, Y, Z." Aim for a maximum of ~10 comments, highest impact first.
45+
46+
## What to Review
47+
48+
### Pass 1: Blockers
49+
50+
#### Correctness
51+
- Logic errors, off-by-one, null/undefined handling, incorrect assumptions, race conditions.
52+
- Boundary conditions — empty arrays, null inputs, zero values, maximum values.
53+
- Error handling — swallowed errors, missing error propagation, unhelpful error messages. Do not flag missing error handling for internal code that cannot reasonably fail.
54+
55+
#### Security
56+
- Injection risks (SQL, command, XSS) when handling user input.
57+
- Hardcoded secrets — API keys, passwords, tokens in code.
58+
- Missing input validation at system boundaries (user input, external APIs). Not for internal function calls.
59+
- Auth bypass, privilege escalation, or missing authorization checks.
60+
61+
#### API Compatibility
62+
- Breaking changes to API response schemas or status codes without migration path.
63+
- Removed or renamed API endpoints, query parameters, or response fields that existing consumers depend on.
64+
- Database schema changes that require migration or backfill.
65+
- MCP tool signature changes (renamed tools, changed input schemas) that break existing clients.
66+
67+
#### Tests for Changed Behavior
68+
- New behavior must have corresponding tests covering core functionality and error handling.
69+
- Bug fixes must include a regression test that would have caught the original bug.
70+
- Changed behavior must have updated tests reflecting the new expectations.
71+
- If tests are present but brittle (testing implementation details rather than behavior), flag it.
72+
73+
Missing tests for changed behavior are blockers (`🔴 Bug`) only when the change affects user-facing behavior, API contracts, or data integrity. Missing tests for internal refactors or trivial changes are `🟡 Issue`.
74+
75+
### Pass 2: Maintainability
76+
77+
#### Code Bloat and Unnecessary Complexity
78+
- **Excessive code** — More lines than necessary. Could this be done in fewer lines without sacrificing clarity?
79+
- **Over-engineering** — Abstractions, helpers, or utilities for one-time operations. Premature generalization. Feature flags or config for things that could just be code.
80+
- **Speculative generality** — Code handling hypothetical future requirements nobody asked for.
81+
- **Dead code** — Unused variables, unreachable branches, commented-out code.
82+
- **Duplicate code** — Same logic repeated instead of extracted. But do not suggest extraction for only 2-3 similar lines — that is premature abstraction.
83+
84+
#### Readability and Naming
85+
- **Confusing variable/function names** — Names that don't describe what the thing is or does. Generic names like `data`, `result`, `item`, `temp`, `val` when a specific name would be clearer.
86+
- **Misleading names** — Names that suggest different behavior than what the code does.
87+
- **Inconsistent naming** — Not following conventions in the rest of the codebase.
88+
- **File naming** — Files not following the project's naming conventions.
89+
- **Long functions** — Functions doing too many things. If you need a comment to explain a section, it should probably be its own function.
90+
- **Deep nesting** — More than 2-3 levels. Suggest early returns, guard clauses, or extraction.
91+
- **Unclear control flow** — Complex conditionals that could be simplified or decomposed.
92+
93+
#### Architecture and Pattern Violations
94+
- **Inline validation instead of Zod schemas** — Validation logic written in code (if/else checks, manual type coercion) instead of using Zod schemas in `openAPIRoute()`. All request validation belongs in the schema, not handler code. This applies to both API routes and MCP tool `inputSchema`.
95+
- **Missing `openAPIRoute()` wrapper** — API endpoints defined without the OpenAPI wrapper.
96+
- **Wrong import paths in tests** — Tests importing from `src/` instead of `dist/`.
97+
- **Missing test categories** — Tests without "Core Functionality" and "Error Handling" describe blocks.
98+
- **Mixing concerns** — Route handlers doing business logic, database queries in API handlers, etc.
99+
100+
#### Hardcoded Values and Magic Constants
101+
Flag only when the value is:
102+
- **Reused 3+ times** in touched files or the diff — should be a named constant.
103+
- **Domain-significant** — timeout values, retry counts, port numbers, API URLs, status messages. Even if used once, these belong in constants or environment variables.
104+
105+
Do not flag one-off numeric literals that are self-explanatory in context (e.g., `array.slice(0, 2)`, `Math.round(x * 100) / 100`).
106+
107+
#### Performance (Only Obvious Issues)
108+
- N+1 queries — database queries inside loops.
109+
- Blocking operations in async contexts — synchronous I/O in async code.
110+
- Unnecessary work in hot paths — redundant allocations, repeated computations.
111+
112+
## What NOT to Review
113+
114+
- Formatting or style — Prettier handles this.
115+
- Type annotations for code that already type-checks.
116+
- Things that are clearly intentional design choices backed by existing patterns.
117+
- Pre-existing issues in unchanged code outside the diff.
118+
- Adding documentation unless a public API is clearly undocumented.
119+
120+
## Comment Format
121+
122+
Use severity prefixes:
123+
- `🔴 Bug:` — Correctness error, security issue, API break, data integrity risk. Will cause incorrect behavior.
124+
- `🟡 Issue:` — Code quality problem that should be fixed. Bloated code, bad naming, pattern violation, missing tests.
125+
- `🔵 Nit:` — Minor improvement, optional.
126+
- `💡 Suggestion:` — Alternative approach worth considering.
127+
128+
Be specific, be concise, explain why. One clear sentence with a concrete fix is better than a paragraph of theory.
129+
130+
## Output Format
131+
132+
Return raw JSON only. No markdown fences, no prose before or after the JSON object. Your output MUST be valid JSON matching the provided output schema. Example:
133+
134+
```json
135+
{
136+
"summary": "This PR adds the user settings API but has a potential auth bypass in the update endpoint and several instances of validation logic that should be in Zod schemas instead of handler code.",
137+
"comments": [
138+
{
139+
"path": "packages/plugin-settings/src/index.ts",
140+
"line": 42,
141+
"body": "🔴 Bug: The `authorized()` middleware is missing on this route. Any unauthenticated user can update settings. Add `authorized(['settings:write'])` middleware."
142+
},
143+
{
144+
"path": "packages/plugin-settings/src/index.ts",
145+
"line": 58,
146+
"body": "🟡 Issue: This manual `if (!req.query.id || typeof req.query.id !== 'string')` check should be in the Zod schema passed to `openAPIRoute()`. The schema handles validation automatically and returns 400 with a descriptive error."
147+
}
148+
]
149+
}
150+
```
151+
152+
The `line` field must refer to the line number in the new version of the file (right side of the diff), and it must be a line that actually appears in the diff hunks. Do not comment on lines outside the diff.
153+
154+
## Summary
155+
156+
Write a brief (2–4 sentence) overall assessment in the `summary` field. Lead with blockers if any exist. Mention whether the PR is clean/minimal or has code quality issues. If the PR looks good, say so.

‎.codex/review-schema.json‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
{
2+
"type": "object",
3+
"properties": {
4+
"summary": {
5+
"type": "string",
6+
"description": "Brief overall assessment of the PR (2-4 sentences)"
7+
},
8+
"comments": {
9+
"type": "array",
10+
"description": "Inline review comments on specific changed lines",
11+
"items": {
12+
"type": "object",
13+
"properties": {
14+
"path": {
15+
"type": "string",
16+
"description": "File path relative to repository root"
17+
},
18+
"line": {
19+
"type": "integer",
20+
"minimum": 1,
21+
"description": "Line number in the new version of the file (must be within the diff)"
22+
},
23+
"body": {
24+
"type": "string",
25+
"description": "Review comment with severity prefix"
26+
}
27+
},
28+
"required": ["path", "line", "body"],
29+
"additionalProperties": false
30+
}
31+
}
32+
},
33+
"required": ["summary", "comments"],
34+
"additionalProperties": false
35+
}

‎.github/workflows/codex-review.yml‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
name: Codex PR Review
2+
3+
on:
4+
pull_request:
5+
types: [opened, synchronize, reopened]
6+
7+
concurrency:
8+
group: codex-review-${{ github.event.pull_request.number }}
9+
cancel-in-progress: true
10+
11+
permissions:
12+
contents: read
13+
pull-requests: write
14+
15+
jobs:
16+
review:
17+
name: Codex Review
18+
runs-on: ubuntu-latest
19+
timeout-minutes: 15
20+
# Skip fork PRs — they cannot access repository secrets
21+
if: github.event.pull_request.head.repo.full_name == github.repository
22+
23+
steps:
24+
- name: Checkout PR merge commit
25+
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
26+
with:
27+
ref: refs/pull/${{ github.event.pull_request.number }}/merge
28+
fetch-depth: 0
29+
30+
- name: Generate PR diff
31+
run: git diff ${{ github.event.pull_request.base.sha }}...HEAD > pr-diff.patch
32+
33+
- name: Run Codex review
34+
id: codex
35+
uses: openai/codex-action@f5c0ca71642badb34c1e66321d8d85685a0fa3dc # v1
36+
with:
37+
openai-api-key: ${{ secrets.OPENAI_API_KEY }}
38+
prompt-file: .codex/review-prompt.md
39+
output-schema-file: .codex/review-schema.json
40+
effort: high
41+
sandbox: read-only
42+
43+
- name: Post PR review with inline comments
44+
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7
45+
env:
46+
REVIEW_JSON: ${{ steps.codex.outputs.final-message }}
47+
with:
48+
script: |
49+
let review;
50+
try {
51+
review = JSON.parse(process.env.REVIEW_JSON);
52+
} catch (e) {
53+
console.error('Failed to parse Codex output:', e.message);
54+
console.error('Raw output:', process.env.REVIEW_JSON?.slice(0, 500));
55+
await github.rest.pulls.createReview({
56+
owner: context.repo.owner,
57+
repo: context.repo.repo,
58+
pull_number: context.issue.number,
59+
body: '⚠️ Codex review failed to produce valid JSON output. Check the [workflow logs](' +
60+
`${process.env.GITHUB_SERVER_URL}/${context.repo.owner}/${context.repo.repo}/actions/runs/${process.env.GITHUB_RUN_ID}) for details.`,
61+
event: 'COMMENT',
62+
comments: [],
63+
});
64+
return;
65+
}
66+
67+
// Fetch all changed files (paginated for large PRs)
68+
const files = await github.paginate(
69+
github.rest.pulls.listFiles,
70+
{
71+
owner: context.repo.owner,
72+
repo: context.repo.repo,
73+
pull_number: context.issue.number,
74+
per_page: 100,
75+
}
76+
);
77+
78+
// Build set of valid (path:line) pairs from diff patches
79+
const validLines = new Set();
80+
for (const file of files) {
81+
// Skip binary/large/truncated files with no patch
82+
if (!file.patch) continue;
83+
84+
const lines = file.patch.split('\n');
85+
let currentLine = 0;
86+
for (const line of lines) {
87+
const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)/);
88+
if (hunkMatch) {
89+
currentLine = parseInt(hunkMatch[1], 10);
90+
continue;
91+
}
92+
// Deleted lines don't exist in the new file
93+
if (line.startsWith('-')) continue;
94+
// Context lines and added lines are valid targets
95+
if (!line.startsWith('\\')) {
96+
validLines.add(`${file.filename}:${currentLine}`);
97+
currentLine++;
98+
}
99+
}
100+
}
101+
102+
// Partition comments into valid (on diff lines) and overflow
103+
const comments = Array.isArray(review.comments) ? review.comments : [];
104+
const validComments = [];
105+
const overflowComments = [];
106+
107+
for (const comment of comments) {
108+
const key = `${comment.path}:${comment.line}`;
109+
if (validLines.has(key)) {
110+
validComments.push({
111+
path: comment.path,
112+
line: comment.line,
113+
body: comment.body,
114+
side: 'RIGHT',
115+
});
116+
} else {
117+
overflowComments.push(comment);
118+
}
119+
}
120+
121+
// Build review body: summary + any overflow comments
122+
let body = review.summary || 'Codex review complete.';
123+
if (overflowComments.length > 0) {
124+
body += '\n\n### Additional comments\n';
125+
body += '_These comments target lines outside the diff and could not be posted inline._\n\n';
126+
for (const c of overflowComments) {
127+
body += `- **\`${c.path}:${c.line}\`** — ${c.body}\n`;
128+
}
129+
}
130+
131+
// Post the review
132+
await github.rest.pulls.createReview({
133+
owner: context.repo.owner,
134+
repo: context.repo.repo,
135+
pull_number: context.issue.number,
136+
body,
137+
event: 'COMMENT',
138+
comments: validComments,
139+
});
140+
141+
console.log(`Review posted: ${validComments.length} inline comments, ${overflowComments.length} overflow comments`);

0 commit comments

Comments
 (0)