Skip to content

Commit 08b9eb2

Browse files
Bojan131Bojan131
committed
Merge remote-tracking branch 'origin/main' into test-ui-fixes
2 parents fdb406f + 4ef0e13 commit 08b9eb2

60 files changed

Lines changed: 8299 additions & 394 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.codex/review-prompt.md‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
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 three passes:
28+
29+
1. **Context + risk-map pass (mandatory)** — Read full touched files (not only hunk lines) and identify high-risk zones: platform/runtime boundaries, event handlers, async cleanup (`finally`), auth/validation boundaries, and repeated predicates/guards.
30+
2. **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.
31+
3. **Maintainability pass** — Scan for code bloat, readability issues, naming problems, pattern violations, hardcoded values, and architecture drift in touched areas. These are `🟡 Issue`, `🔵 Nit`, or `💡 Suggestion` comments.
32+
33+
### Comment Gate
34+
35+
Before posting any comment, verify all three conditions:
36+
37+
1. **Introduced by this diff** — The issue is introduced or materially worsened by the changes in this PR, not pre-existing.
38+
2. **Materially impactful** — The issue affects correctness, security, readability, or maintainability in a meaningful way. Not a theoretical concern.
39+
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.
40+
41+
If any check fails, skip the comment.
42+
43+
**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.
44+
45+
**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.
46+
47+
## What to Review
48+
49+
### Pass 1: Blockers
50+
51+
#### Correctness
52+
53+
- Logic errors, off-by-one, null/undefined handling, incorrect assumptions, race conditions.
54+
- Boundary conditions — empty arrays, null inputs, zero values, maximum values.
55+
- Error handling — swallowed errors, missing error propagation, unhelpful error messages. Do not flag missing error handling for internal code that cannot reasonably fail.
56+
- Unsafe runtime assumptions hidden by type assertions (`as ...`, `as any`, non-null `!`) when values come from events, external I/O, or platform-specific APIs.
57+
- Platform/runtime compatibility assumptions — usage of globals/APIs (`window`, `document`, `Node`, `process`, browser-only APIs) in cross-runtime code paths must be guarded.
58+
59+
#### Security
60+
61+
- Injection risks (SQL, command, XSS) when handling user input.
62+
- Hardcoded secrets — API keys, passwords, tokens in code.
63+
- Missing input validation at system boundaries (user input, external APIs). Not for internal function calls.
64+
- Auth bypass, privilege escalation, or missing authorization checks.
65+
66+
#### API Compatibility
67+
68+
- Breaking changes to API response schemas or status codes without migration path.
69+
- Removed or renamed API endpoints, query parameters, or response fields that existing consumers depend on.
70+
- Database schema changes that require migration or backfill.
71+
- MCP tool signature changes (renamed tools, changed input schemas) that break existing clients.
72+
73+
#### Tests for Changed Behavior
74+
75+
- New behavior must have corresponding tests covering core functionality and error handling.
76+
- Bug fixes must include a regression test that would have caught the original bug.
77+
- Changed behavior must have updated tests reflecting the new expectations.
78+
- If tests are present but brittle (testing implementation details rather than behavior), flag it.
79+
- Prefer tests that validate production behavior directly. If a test re-implements production decision logic locally, and could stay green while runtime behavior regresses, flag it and suggest importing shared runtime logic or testing via a higher-level behavior path.
80+
81+
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`.
82+
83+
### Pass 2: Maintainability
84+
85+
#### Architecture Direction (Touched Area)
86+
87+
- Evaluate whether the diff makes the touched area more or less maintainable (coupling, cohesion, readability of control flow).
88+
- Flag **architecture drift** when business decisions become more scattered (same guard/predicate duplicated across multiple call paths, UI/state/network logic further entangled, or test/runtime logic diverging).
89+
- When the same invariant-like predicate appears repeatedly in changed code, prefer a named helper/shared utility if it clearly reduces divergence risk.
90+
91+
#### Code Bloat and Unnecessary Complexity
92+
93+
- **Excessive code** — More lines than necessary. Could this be done in fewer lines without sacrificing clarity?
94+
- **Over-engineering** — Abstractions, helpers, or utilities for one-time operations. Premature generalization. Feature flags or config for things that could just be code.
95+
- **Speculative generality** — Code handling hypothetical future requirements nobody asked for.
96+
- **Dead code** — Unused variables, unreachable branches, commented-out code.
97+
- **Duplicate code** — Same logic repeated instead of extracted. Do not suggest extraction for only 2-3 similar lines unless the repeated logic encodes a correctness invariant across multiple paths (e.g., identical guard logic in multiple `finally` blocks).
98+
99+
#### Readability and Naming
100+
101+
- **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.
102+
- **Misleading names** — Names that suggest different behavior than what the code does.
103+
- **Inconsistent naming** — Not following conventions in the rest of the codebase.
104+
- **File naming** — Files not following the project's naming conventions.
105+
- **Long functions** — Functions doing too many things. If you need a comment to explain a section, it should probably be its own function.
106+
- **Deep nesting** — More than 2-3 levels. Suggest early returns, guard clauses, or extraction.
107+
- **Unclear control flow** — Complex conditionals that could be simplified or decomposed.
108+
109+
#### Architecture and Pattern Violations
110+
111+
- **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`.
112+
- **Missing `openAPIRoute()` wrapper** — API endpoints defined without the OpenAPI wrapper.
113+
- **Wrong import paths in tests** — Tests importing from `src/` instead of `dist/`.
114+
- **Missing test categories** — Tests without "Core Functionality" and "Error Handling" describe blocks.
115+
- **Mixing concerns** — Route handlers doing business logic, database queries in API handlers, etc.
116+
117+
#### Hardcoded Values and Magic Constants
118+
119+
Flag only when the value is:
120+
121+
- **Reused 3+ times** in touched files or the diff — should be a named constant.
122+
- **Domain-significant** — timeout values, retry counts, port numbers, API URLs, status messages. Even if used once, these belong in constants or environment variables.
123+
124+
Do not flag one-off numeric literals that are self-explanatory in context (e.g., `array.slice(0, 2)`, `Math.round(x * 100) / 100`).
125+
126+
#### Performance (Only Obvious Issues)
127+
128+
- N+1 queries — database queries inside loops.
129+
- Blocking operations in async contexts — synchronous I/O in async code.
130+
- Unnecessary work in hot paths — redundant allocations, repeated computations.
131+
132+
## What NOT to Review
133+
134+
- Formatting or style — Prettier handles this.
135+
- Type annotations for code that already type-checks.
136+
- Things that are clearly intentional design choices backed by existing patterns.
137+
- Pre-existing issues in unchanged code outside the diff.
138+
- Adding documentation unless a public API is clearly undocumented.
139+
140+
## Comment Format
141+
142+
Use severity prefixes:
143+
144+
- `🔴 Bug:` — Correctness error, security issue, API break, data integrity risk. Will cause incorrect behavior.
145+
- `🟡 Issue:` — Code quality problem that should be fixed. Bloated code, bad naming, pattern violation, missing tests.
146+
- `🔵 Nit:` — Minor improvement, optional.
147+
- `💡 Suggestion:` — Alternative approach worth considering.
148+
149+
Be specific, be concise, explain why. One clear sentence with a concrete fix is better than a paragraph of theory.
150+
151+
## Output Format
152+
153+
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:
154+
155+
```json
156+
{
157+
"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.",
158+
"comments": [
159+
{
160+
"path": "packages/plugin-settings/src/index.ts",
161+
"line": 42,
162+
"body": "🔴 Bug: The `authorized()` middleware is missing on this route. Any unauthenticated user can update settings. Add `authorized(['settings:write'])` middleware."
163+
},
164+
{
165+
"path": "packages/plugin-settings/src/index.ts",
166+
"line": 58,
167+
"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."
168+
}
169+
]
170+
}
171+
```
172+
173+
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.
174+
175+
## Summary
176+
177+
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. Include one sentence on maintainability direction in touched areas (improved / neutral / worsened, and why). 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/check-package-lock.yml‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
name: Check Package Lock File
2+
3+
permissions:
4+
contents: read
5+
6+
concurrency:
7+
group: check-package-lock-${{ github.ref }}
8+
cancel-in-progress: true
9+
10+
on:
11+
push:
12+
branches:
13+
- main
14+
paths:
15+
- 'package.json'
16+
- 'package-lock.json'
17+
- '**/package.json'
18+
pull_request:
19+
branches:
20+
- "**"
21+
paths:
22+
- 'package.json'
23+
- 'package-lock.json'
24+
- '**/package.json'
25+
26+
jobs:
27+
verify-package-lock:
28+
name: Verify package-lock.json exists
29+
runs-on: ubuntu-latest
30+
timeout-minutes: 5
31+
32+
steps:
33+
- name: Checkout repository
34+
uses: actions/checkout@v4
35+
36+
- name: Check if package-lock.json exists
37+
run: |
38+
if [ ! -f "package-lock.json" ]; then
39+
echo "ERROR: package-lock.json file is missing from the repository"
40+
echo "This file is required to ensure consistent dependency versions across all environments"
41+
echo "Please ensure package-lock.json is committed with your changes"
42+
exit 1
43+
fi
44+
echo "SUCCESS: package-lock.json file is present"
45+
46+
- name: Verify package-lock.json is not empty
47+
run: |
48+
if [ ! -s "package-lock.json" ]; then
49+
echo "ERROR: package-lock.json file exists but is empty"
50+
echo "Please run 'npm install' to regenerate the lock file"
51+
exit 1
52+
fi
53+
echo "SUCCESS: package-lock.json file is valid and not empty"
54+
55+
- name: Setup Node.js
56+
uses: actions/setup-node@v4
57+
with:
58+
node-version: '22'
59+
60+
- name: Validate package-lock.json is valid and in sync
61+
run: npm ci --dry-run --ignore-scripts

0 commit comments

Comments
 (0)