feat: replace pr_review with spec-driven CI gates and pr-review skill
CI / validate (pull_request) Failing after 41s
CI / auto-merge (pull_request) Skipped

Add validate_spec, check_pr_size, fast_molecule, nightly_gate, and
create_dependency_pr CI modules. Remove the monolithic pr_review module
and its tests. Replace pr_review CI steps with validate_spec + check_pr_size
+ curl-based APPROVE. Add spec-driven-development and pr-review skills.
Remove dead get_reviewer_token. Update translations and AGENTS.md.

Closes DEVX-155

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
emil
2026-08-24 19:56:08 +02:00
co-authored by Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
parent a06caa0e88
commit aefc22de57
26 changed files with 3376 additions and 2035 deletions
+9 -1
View File
@@ -12,7 +12,6 @@ Quick reference for devx tools when working on the devx repo itself.
| Check CI status | `make devx-pr-status` or `make devx-pr-status PR=42 WAIT=1` |
| Fetch CI failure logs | `make devx-pr-logs` or `make devx-pr-logs PR=42 JOB=quality TAIL=50` |
| Add ready-to-merge label | `make devx-pr-label` or `make devx-pr-label PR=42` |
| Post PR review | `make devx-pr-review PR=42 EVENT=APPROVE BODY="..." CHECKLIST=1,2,3,4,5,6,7,8,9,10,11,12,13` |
| Rebase current branch | `make rebase` |
| Rebase PR via API | `make pr-rebase` or `make pr-rebase PR=42` |
@@ -24,6 +23,15 @@ When the `ready-to-merge` label is added and all CI checks pass:
3. The rebase triggers a new CI run; the next auto-merge attempt merges
4. No manual rebase needed unless the API rebase fails
## Spec-Driven CI Gates (Pre-merge)
Every PR must pass these gates before merge:
| Gate | Module | What it checks |
|------|--------|----------------|
| Spec validation | `devx.ci.validate_spec` | Spec file exists at `docs/specs/<TASK-ID>.md`, has REQ-IDs, all ACs checked |
| PR size | `devx.ci.check_pr_size` | Max 500 lines / 10 files (excludes CHANGELOG, badges, locks) |
## Key Rules
- Never manually merge via API — always use auto-merge with `ready-to-merge` label
+272
View File
@@ -0,0 +1,272 @@
# pr-review
Deep, critical PR review with auto-fix. This skill guides the agent
through a thorough review of a pull request, posting inline comments
for each issue found, auto-fixing them, resolving the discussion threads,
and marking the PR as ready-to-merge when no blocking issues remain.
## When to Invoke
Invoke this skill when asked to review a PR, or when a PR is open and
needs review before merge. Do NOT invoke automatically on every PR —
this is an on-demand deep review, not a CI gate.
## Prerequisites
- The PR must be open in a Gitea repo
- The agent needs Gitea MCP access (gitea server)
- The agent needs git push access to the PR's head branch
- The PR should have passed CI (validate job) before deep review
## Review Categories
Review every PR against these 8 categories. For each issue found, post
an inline comment on the specific line, then auto-fix it.
### 1. Functional Correctness
- Does the code actually do what the spec/PR title claims?
- Are edge cases handled? (empty input, null, boundary values, concurrent access)
- Are error paths tested? Not just happy path.
- Does the code handle all return values? (ignored errors, unchecked None)
- Are there off-by-one errors, wrong comparisons, inverted conditions?
- Do loops terminate correctly? (no infinite loops, correct break/continue)
- Are regex patterns correct? (anchored, escaped, non-greedy where needed)
- Are API responses validated before use? (status codes, response shape)
### 2. Completeness
- Are all requirements from the spec implemented? (check each REQ-ID)
- Are all acceptance criteria in the spec checked off?
- Are tests written for all new code paths?
- Are error messages user-facing (wrapped in `_()`)?
- Are new CLI commands documented in `docs/user/cli-commands.md`?
- Are new modules added to architecture docs?
- Are CHANGELOG entries added for user-facing changes?
- Are translations added for new user-facing strings?
### 3. Architecture
- Does the code follow the repo's layer separation? (no business logic in CLI, no direct subprocess in CLI)
- Are new dependencies justified? (no unnecessary new packages)
- Is configuration via env vars / config.py, not hardcoded?
- Are new modules placed in the correct directory? (ci/ vs tools/ vs molecule/)
- Does the code reuse existing utilities? (no reimplemented helpers)
- Are imports circular? (check import chains)
- Is the code testable? (injectable dependencies, no hidden global state)
- Does the code follow existing patterns in the codebase?
### 4. Reliability
- Are external API calls retried with backoff?
- Are timeouts set on all network operations?
- Are file operations atomic? (write to temp, rename)
- Are database operations transactional where needed?
- Are there race conditions? (check shared mutable state)
- Are resources cleaned up in all paths? (finally blocks, context managers)
- Can the code handle partial failures? (one service down, others up)
- Are idempotency guarantees maintained? (safe to retry)
### 5. Robustness
- Does the code fail gracefully? (meaningful error messages, not stack traces)
- Are unexpected inputs handled? (type checking, validation)
- Are there any crash-on-bad-input paths?
- Does the code degrade under load? (backpressure, queue limits)
- Are there resource leaks? (file handles, connections, memory)
- Does the code survive network partitions? (retry, circuit breaker)
- Are there any unhandled exceptions that could crash the process?
- Is logging sufficient to diagnose production issues?
### 6. Security
- Are there hardcoded secrets, tokens, or passwords?
- Is `shell=True` used with user input? (command injection)
- Is `eval()` or `exec()` used? (code injection)
- Are SQL queries parameterized? (no string concatenation)
- Are file paths validated? (no path traversal)
- Are user inputs sanitized before display? (XSS in web contexts)
- Are SSL/TLS verifications disabled without justification?
- Are secrets logged in error messages or debug output?
- Are permissions checked before privileged operations?
- Is sensitive data in memory longer than necessary?
### 7. Technical Excellence
- Are functions under 50 lines? (refactor if longer)
- Is cyclomatic complexity reasonable? (no deeply nested if/else chains)
- Are names meaningful? (no single-letter vars, no misleading names)
- Is dead code removed? (no commented-out blocks, no unused imports)
- Are comments explaining WHY, not WHAT?
- Is the code DRY? (no copy-pasted blocks that should be shared)
- Is the code SOLID? (single responsibility, open/closed)
- Are magic numbers extracted to named constants?
- Is the code formatted per the repo's linter config?
- Are type hints present on all function signatures?
### 8. Test Quality
- Do tests actually test the behavior? (not just that code runs)
- Are tests independent? (no shared mutable state, no order dependency)
- Are tests fast? (no real sleeps, no real network calls, mocked)
- Are edge cases tested? (empty, None, boundary, error paths)
- Are test names descriptive? (test_what_condition_expected_result)
- Are mocks set up correctly? (mocking the right object, not too broad)
- Is coverage 100% for new code? (every branch, every line)
- Are integration tests added for cross-module changes?
- Do tests clean up after themselves? (tmp_path, fixtures)
## Review Procedure
### Step 1: Gather Context
```
1. Read the PR spec (if exists): docs/specs/<TASK-ID>.md
2. Fetch PR details via Gitea MCP: pull_request_read (get_pr, list_pr_files)
3. Read the full diff: git diff origin/master...HEAD
4. Read the PR description and any existing review comments
5. Identify the repo's task prefix (OBL-INFRA, GRM, SSO, DEVX)
```
### Step 2: Review Each File
For each changed file in the PR:
1. Read the full file (not just the diff) to understand context
2. Go through all 8 review categories
3. For each issue found, note: file path, line number, category, severity, description, suggested fix
### Step 3: Post Inline Comments
For each issue found, post an inline review comment using the Gitea MCP:
```
mcp_call_tool: gitea / pull_request_review_write
method: create
owner: <owner>
repo: <repo>
pull_number: <PR number>
state: PENDING (accumulate comments before submitting)
body: "" (empty for now, summary added on submit)
comments: [
{
path: "<file path>",
new_line_num: <line number>,
body: "**[<category>] [<severity>]** <description>\n\n**Suggested fix:**\n```<lang>\n<fixed code>\n```"
}
]
```
Comment format:
```
**[Security] [error]** `shell=True` used with user input — command injection risk.
**Suggested fix:**
```python
subprocess.run(["git", "log", commit], check=True)
```
```
Severity levels:
- `error` — must fix before merge (security, correctness, crash)
- `warning` — should fix before merge (reliability, best practice)
- `info` — consider fixing (style, minor improvement)
### Step 4: Auto-Fix Issues
For each issue that can be safely auto-fixed:
1. Edit the file using the `edit` tool
2. Commit with message: `fix: address review comment — <short description>`
3. Push to the PR's head branch: `git push origin HEAD`
4. Wait for CI to re-run on the push
Auto-fix ALL issues unless:
- The fix requires an architectural decision (ask the user)
- The fix changes public API behavior (ask the user)
- The fix is ambiguous (multiple valid approaches, ask the user)
### Step 5: Resolve Discussion Threads
After auto-fixing an issue and CI passes:
1. Find the review comment thread for that issue
2. Post a reply: `Fixed in <commit-sha>. Closing this thread.`
3. Resolve the discussion (if Gitea supports it via API)
4. If resolving via API is not available, the reply comment serves as resolution
### Step 6: Submit Final Review
After all issues are addressed (fixed or discussed):
```
mcp_call_tool: gitea / pull_request_review_write
method: submit
owner: <owner>
repo: <repo>
pull_number: <PR number>
review_id: <from step 3 create>
state: COMMENT (or APPROVED if no blocking issues remain)
body: <summary — see below>
```
### Step 7: Post Summary
Post a brief summary as a PR comment (via `issue_write / add_comment`):
```
## Deep Review Summary
- **Files reviewed:** N
- **Issues found:** N (N auto-fixed, N require attention)
- **Categories:** security (N), correctness (N), architecture (N), ...
**Outcome:** ✅ Ready to merge — all issues addressed.
**OR**
**Outcome:** ⚠️ N blocking issue(s) remain — see inline comments.
```
Keep the summary to 5-10 bullet points. Do not paste the full review.
### Step 8: Mark PR Ready
If all issues are addressed and no blocking issues remain:
```
mcp_call_tool: gitea / issue_write
method: add_labels
owner: <owner>
repo: <repo>
issue_number: <PR number>
labels: [<label_id for "ready-to-merge">]
```
If blocking issues remain, do NOT add the label. Post a comment
explaining what needs to be resolved before the PR can merge.
## Gitea MCP Tools Reference
| Action | MCP tool | Method |
|--------|----------|--------|
| Get PR details | `pull_request_read` | `get_pr` |
| List PR files | `pull_request_read` | `list_pr_files` |
| Get PR diff | `pull_request_read` | `get_pr_diff` |
| Create review (pending) | `pull_request_review_write` | `create` (state: PENDING) |
| Submit review | `pull_request_review_write` | `submit` (state: APPROVED/COMMENT/REQUEST_CHANGES) |
| Post PR comment | `issue_write` | `add_comment` |
| Add label | `issue_write` | `add_labels` |
| List labels | `label_read` | `list_repo_labels` |
| Merge PR | `pull_request_write` | `merge` (do NOT use — auto-merge handles this) |
## Important Rules
- **Never merge the PR yourself.** Add the `ready-to-merge` label and let
the auto-merge workflow handle it. This ensures CI passes and the
commit message follows the `<PREFIX>-N: <conventional>` format.
- **Never approve your own PR.** If the agent created the PR, post
COMMENT state, not APPROVED.
- **Always push fixes to the PR branch**, not directly to master.
- **Wait for CI after each push** before resolving the discussion thread.
- **Post one review with all comments**, not multiple reviews.
- **The summary must be brief** — 5-10 bullet points max.
- **Severity matters**: only `error` severity blocks the `ready-to-merge` label.
@@ -0,0 +1,130 @@
# Spec-Driven Development
## Overview
Every change starts with a spec. No spec, no code. No code, no PR.
The spec is a markdown file at `docs/specs/<TASK-ID>.md` in the repo.
It contains structured requirements (REQ-IDs) and acceptance criteria
(AC checklist) that CI validates before merge.
## Workflow
1. **Create Vikunja task**`make create-task -- --title "Title" --description "..."`
2. **Write spec** — Create `docs/specs/<TASK-ID>.md` (see template below)
3. **Create branch**`git checkout -b <PREFIX>-N-short-description`
4. **Implement** — Write code with `# Implements: REQ-N` comments
5. **Check ACs** — Tick all acceptance criteria checkboxes in the spec
6. **Push and create PR**`make push-with-pr`
7. **CI validates** — Spec validation, PR size check, fast molecule, lint, tests
8. **Auto-merge** — Add `ready-to-merge` label after review
9. **Auto-deploy** — Post-merge deploys to staging (if nightly gate is green)
## Spec Template
```markdown
# <TASK-ID>: <Title>
## Problem
<What is broken or missing? Why does this change exist?>
## Approach
<How will you solve it? What are the key design decisions?>
REQ-1: <First requirement description>
REQ-2: <Second requirement description>
REQ-3: <Third requirement description>
## Test Plan
- <How will you verify each REQ is implemented correctly?>
- <Include unit tests, molecule scenarios, integration tests>
## Deploy Plan
- <How will this change be deployed?>
- <What order do components need to deploy in?>
- <Are there migrations or one-time operations?>
## Rollback Plan
- <How do you revert if something goes wrong?>
- <What data/state changes are irreversible?>
## Acceptance Criteria
- [ ] REQ-1: <criterion that proves REQ-1 is done>
- [ ] REQ-2: <criterion that proves REQ-2 is done>
- [ ] REQ-3: <criterion that proves REQ-3 is done>
```
## CI Validation
The `devx.ci.validate_spec` module checks:
1. **Spec file exists** at `docs/specs/<TASK-ID>.md` (TASK-ID from branch name)
2. **Required sections present**: Problem, Approach, Test Plan, Deploy Plan, Rollback Plan, Acceptance Criteria
3. **At least one REQ-ID** line (format: `REQ-N: <description>`)
4. **All AC checkboxes checked** (`- [x]`, not `- [ ]`)
If any check fails, CI blocks the PR before expensive jobs run.
## PR Size Limits
CI enforces max 500 lines / 10 files changed (excluding CHANGELOG.md,
README.md, badges, lock files). Oversized PRs are rejected. Split your
work into smaller PRs.
## Code-to-Spec Linking
Each function, task, or template that implements a requirement should
have a comment:
```python
# Implements: REQ-1
def install_sso_bridge():
...
```
```yaml
# Implements: REQ-2
- name: Clone infra repo
git:
...
```
## Fast Molecule (Pre-merge)
CI runs molecule only for **changed roles** (detected via git diff),
with converge + verify only, single platform. This gives quick feedback
(~5-10 min) without the full molecule suite.
## Full Molecule (Nightly)
The complete molecule suite (all scenarios, all platforms) runs nightly
at 02:00 CET on master. If it fails:
- A Gitea issue is created with the `feedback` label
- The `NIGHTLY_STATUS` repo variable is set to `failed:<run_id>`
- All staging deploys are blocked until nightly passes again
## Auto-Deploy on Merge
Every merged PR auto-deploys to staging (if nightly gate is green).
No manual trigger needed. The deploy runs the full pipeline:
provision → deploy-observability → deploy-customer → configure-oidc.
For grm/sso-bridge: post-merge publishes the package, then auto-creates
an infra PR to bump the pinned version. That infra PR auto-deploys when
merged.
## Key Commands
```bash
# Validate spec locally (before pushing)
python -m devx.ci.validate_spec --branch <PREFIX>-N-description
# Check PR size locally
python -m devx.ci.check_pr_size --base origin/master --head HEAD
# See which roles need fast molecule
python -m devx.ci.fast_molecule --base origin/master --head HEAD
# Check nightly gate status
python -m devx.ci.nightly_gate --repo oblachno/infra --action check
```
@@ -45,6 +45,13 @@ This runs `lint-all` + `pytest-cov`. The pre-push git hook only
validates the Vikunja task exists — it does NOT run tests. You must
run `make pre-push` manually.
### Spec-Driven Workflow
Every PR requires a spec file at `docs/specs/<TASK-ID>.md`. See the
`spec-driven-development` skill for the full workflow and template.
CI validates the spec (via `devx.ci.validate_spec`) and checks PR size
(via `devx.ci.check_pr_size`) before running expensive jobs.
## CI Failure Investigation
When investigating a CI failure: