diff --git a/.devin/skills/devx-workflow/SKILL.md b/.devin/skills/devx-workflow/SKILL.md index bbf03e8..11a84fd 100644 --- a/.devin/skills/devx-workflow/SKILL.md +++ b/.devin/skills/devx-workflow/SKILL.md @@ -12,7 +12,6 @@ Quick reference for devx tools when working on this repo. | 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` | @@ -30,6 +29,25 @@ CI runs a `pre-merge-check` job early (after quality + detect-changes) that validates branch format, PR title, and Vikunja task match. This fails fast before expensive molecule tests run. +## 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/.md`, has REQ-IDs, all ACs checked | +| PR size | `devx.ci.check_pr_size` | Max 500 lines / 10 files (excludes CHANGELOG, badges, locks) | + +Full molecule tests still run on every PR (6 scenarios, all platforms). + +## Post-merge Auto-publish + Dependency PR + +After merge to master, `post-merge.yml`: +1. Runs release (git-cliff semver, tags, publishes to Gitea PyPI) +2. Auto-creates an infra dependency PR (`devx.ci.create_dependency_pr`) + to bump the pinned grm version in `infra/pyproject.toml` +3. Syncs wiki, updates Vikunja task, pushes badges + ## Key Rules - Never manually merge via API — always use auto-merge with `ready-to-merge` label diff --git a/.devin/skills/pr-review/SKILL.md b/.devin/skills/pr-review/SKILL.md new file mode 100644 index 0000000..967ec1f --- /dev/null +++ b/.devin/skills/pr-review/SKILL.md @@ -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/.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: + repo: + pull_number: + state: PENDING (accumulate comments before submitting) + body: "" (empty for now, summary added on submit) + comments: [ + { + path: "", + new_line_num: , + body: "**[] []** \n\n**Suggested fix:**\n```\n\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 — ` +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 . 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: + repo: + pull_number: + review_id: + state: COMMENT (or APPROVED if no blocking issues remain) + body: +``` + +### 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: + repo: + issue_number: + labels: [] +``` + +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 `-N: ` 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. diff --git a/.devin/skills/spec-driven-development/SKILL.md b/.devin/skills/spec-driven-development/SKILL.md new file mode 100644 index 0000000..8fef430 --- /dev/null +++ b/.devin/skills/spec-driven-development/SKILL.md @@ -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/.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/.md` (see template below) +3. **Create branch** — `git checkout -b -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 +# : + +## 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 +``` diff --git a/.devin/skills/testing-and-debugging/SKILL.md b/.devin/skills/testing-and-debugging/SKILL.md index 555f0f4..01b4003 100644 --- a/.devin/skills/testing-and-debugging/SKILL.md +++ b/.devin/skills/testing-and-debugging/SKILL.md @@ -35,6 +35,12 @@ produces false failures (missing dependencies, wrong Python version). | All platforms | `make molecule-all` | All 6 scenarios on all 4 OSes | | Parallel | `make molecule-all-parallel` | MOLECULE_JOBS=4 | +### 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 before running expensive jobs. + ## Pre-Push Verification **Before pushing any branch:** diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 6a9f76f..6abfdcd 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -110,14 +110,30 @@ jobs: --pr-title "$PR_TITLE" \ --repo "$REPOSITORY" \ --pr-number "$PR_NUMBER" - - name: Run automated PR review + - name: Validate spec file if: github.event_name == 'pull_request' + env: + DEVX_TASK_PREFIX: GRM + PYTHONPATH: ${{ env.PYTHONPATH }} + HEAD_REF: ${{ github.head_ref }} run: | . .venv/bin/activate 2>/dev/null || true - set -euo pipefail - python3 -m devx.ci.pr_review \ - "${{ github.event.number }}" \ - "${{ github.repository }}" + python3 -m devx.ci.validate_spec \ + --branch "$HEAD_REF" \ + --github-output + - name: Check PR size + if: github.event_name == 'pull_request' + env: + PYTHONPATH: ${{ env.PYTHONPATH }} + CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }} + run: | + . .venv/bin/activate 2>/dev/null || true + python3 -m devx.ci.check_pr_size \ + --base "origin/master" \ + --head "${{ github.event.pull_request.head.sha || github.sha }}" \ + --repo "${{ github.repository }}" \ + --pr-number "${{ github.event.number }}" \ + --github-output # --- release-dry-run step (conditional) --- - name: Release dry-run validation if: steps.detect.outputs.user-facing-changed == 'true' @@ -285,16 +301,17 @@ jobs: env: REVIEWER_GITEA_API_TOKEN: ${{ secrets.REVIEWER_GITEA_API_TOKEN }} PR_NUMBER: ${{ github.event.number }} - REPOSITORY: ${{ github.repository }} + GITHUB_SERVER_URL: ${{ github.server_url }} + GITHUB_REPOSITORY: ${{ github.repository }} run: | . .venv/bin/activate 2>/dev/null || true - python3 -m devx.ci.pr_review \ - "$PR_NUMBER" \ - "$REPOSITORY" \ - --event APPROVE \ - --checklist-confirmed \ - --checklist-categories 1,2,3,4,5,6,7,8,9,10,11,12,13 \ - --body "Auto-approved: all CI checks passed (validate, molecule-tests)." + # Post APPROVE review via Gitea API to satisfy branch protection + curl -s -X POST \ + "${GITHUB_SERVER_URL}/api/v1/repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" \ + -H "Authorization: token ${REVIEWER_GITEA_API_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{"event":"APPROVED","body":"Auto-approved: all CI checks passed (validate, molecule-tests)."}' \ + || echo "::warning::Failed to post approval review (best-effort)." - name: Wait for molecule tests to complete env: CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }} @@ -311,11 +328,11 @@ jobs: import sys,json d=json.load(sys.stdin) statuses={s['context']:s['status'] for s in d.get('statuses',[])} - # Check if all molecule-tests contexts are terminal (success/failure) + # Check if all molecule-tests contexts are terminal (success/failure/skipped) mol_contexts=[k for k in statuses if 'molecule-tests' in k] if not mol_contexts: - print('pending') - elif all(statuses[k] in ('success','failure') for k in mol_contexts): + print('skipped') + elif all(statuses[k] in ('success','failure','skipped') for k in mol_contexts): if any(statuses[k]=='failure' for k in mol_contexts): print('failure') else: @@ -324,8 +341,8 @@ jobs: print('pending') ") echo "Molecule tests status: $STATUS (elapsed: ${ELAPSED}s)" - if [ "$STATUS" = "success" ]; then - echo "All molecule tests passed." + if [ "$STATUS" = "success" ] || [ "$STATUS" = "skipped" ]; then + echo "All molecule tests passed (or skipped — no ansible changes)." break elif [ "$STATUS" = "failure" ]; then echo "ERROR: Molecule tests failed. Aborting auto-merge." diff --git a/.gitea/workflows/post-merge.yml b/.gitea/workflows/post-merge.yml index 7db9d88..bfe2741 100644 --- a/.gitea/workflows/post-merge.yml +++ b/.gitea/workflows/post-merge.yml @@ -148,6 +148,26 @@ jobs: git fetch --tags git checkout "${{ steps.release-tag.outputs.tag }}" python3 -m devx.ci.publish "${{ steps.release-tag.outputs.tag }}" "${{ github.repository }}" --auto-login + - name: Create infra dependency PR + if: steps.release-tag.outputs.tag != '' + env: + CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }} + VIKUNJA_TOKEN: ${{ secrets.VIKUNJA_TOKEN }} + PYTHONPATH: ${{ env.PYTHONPATH }} + DEVX_TASK_PREFIX: GRM + DEVX_VIKUNJA_PROJECT_ID: 6 + run: | + . .venv/bin/activate 2>/dev/null || true + # Extract version from the tag (strip leading 'v') + TAG="${{ steps.release-tag.outputs.tag }}" + VERSION="${TAG#v}" + python3 -m devx.ci.create_dependency_pr \ + --repo oblachno/infra \ + --package grm \ + --new-version "$VERSION" \ + --source-repo "${{ github.repository }}" \ + --source-run-id "${{ github.run_id }}" || \ + echo "::warning::Failed to create infra dependency PR (best-effort)." # --- sync-wiki + vikunja (skip on automated/release commits) --- - name: Sync documentation to wiki if: needs.detect-and-configure.outputs.is-automated == 'false' diff --git a/AGENTS.md b/AGENTS.md index 2a84ace..66cfe82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,8 +61,37 @@ CI also runs a best-effort `make workflow-dryrun` step (skipped if act_runner is - **devx package** (installed from git) — Reusable CI/CD tools: auto-merge, post-merge, release, publishing, molecule distribution, PR reviews, failure notifications - **Versioning** (`cliff.toml`) — git-cliff configuration for automated semver versioning from conventional commits + +## Spec-Driven Development + +Every change starts with a spec. No spec, no code. + +**Workflow:** +1. Create Vikunja task → get `<PREFIX>-N` task ID +2. Write spec at `docs/specs/<TASK-ID>.md` (see template in `.devin/skills/spec-driven-development/SKILL.md`) +3. Create branch, implement with `# Implements: REQ-N` comments +4. Tick all acceptance criteria checkboxes in spec +5. Push and create PR — CI validates spec before expensive jobs + +**CI gates (pre-merge):** +- `devx.ci.validate_spec` — checks spec exists, has required sections, REQ-IDs, all ACs checked +- `devx.ci.check_pr_size` — max 500 lines / 10 files (excludes CHANGELOG, badges, locks) +- `devx.ci.fast_molecule` — converge+verify only for changed roles, single platform + +**Nightly (infra only):** +- Full molecule suite (all scenarios, all platforms) + staging deploy + integration tests +- On failure: sets `NIGHTLY_STATUS=failed`, blocks staging deploys +- Post-merge auto-deploy to staging checks this gate before deploying + +**Post-merge:** +- Infra: auto-deploys to staging (if nightly gate is green) +- GRM/sso-bridge: auto-publishes package, auto-creates infra dependency PR to bump pinned version + +**Skill:** `.devin/skills/spec-driven-development/SKILL.md` — full template and workflow details. + ## PR Workflow (Mandatory) + Every change to master goes through this workflow. No exceptions. ### Branch Protection (Required Gitea Settings) @@ -118,67 +147,40 @@ docs: update README ### 6. Review the PR (Mandatory — Before Adding ready-to-merge Label) -**Review checklist:** Every PR is reviewed against 13 categories covering -architecture, code quality, security, i18n, testing, performance, -UX, documentation, workflow compliance, maintainability, resource -management, backwards compatibility, and logging. +**Review checklist:** Every PR is reviewed against 8 categories covering +functional correctness, completeness, architecture, reliability, +robustness, security, technical excellence, and test quality. -**Automated review (CI `validate` job):** Every PR triggers an automated -review via `python -m devx.ci.pr_review` as a step in the `validate` job. -This posts a review with -`COMMENT` (no issues) or `REQUEST_CHANGES` (issues found) based on -the **[auto]** items in the checklist: +**Deep review (agent-invoked `pr-review` skill):** The agent invokes +the `pr-review` skill to perform a deep, critical review of the PR. +The skill posts inline comments for each issue found via the Gitea MCP, +auto-fixes them, pushes fixes to the PR branch, resolves discussion +threads, and posts a brief summary. When no blocking issues remain, +the PR is marked `ready-to-merge`. -- Architecture compliance (no subprocess in CLI, no hardcoded URLs) -- Best practices (no `print()`, no bare `except`, no `TODO`/`FIXME`, - no functions > 50 lines) -- Security (no hardcoded secrets, no `shell=True`, no `eval`/`exec`) -- i18n (no raw strings in `click.echo()` without `_()` wrapper) -- Resource management (no `open()` without `with`, no `Popen()` without cleanup) -- Documentation (source changes must include doc updates) -- Test coverage (source changes must include test updates) -- Commit conventions (conventional commit format on PR commits) - -The automated review posts inline comments on specific lines and -includes a summary of the checklist categories. The agent **must** address all -`REQUEST_CHANGES` issues before proceeding. - -**Manual review (agent):** After the automated review passes, the agent -must go through **every category** listed above and verify -the **[manual]** items by reviewing the full diff -(`git diff master...HEAD`). - -Post review comments using `devx.ci.pr_review` (run as `python -m devx.ci.pr_review`): -```bash -CI_GITEA_TOKEN=<token> python -m devx.ci.pr_review <pr_number> <owner/repo> \ - --event REQUEST_CHANGES \ - --body "Review summary" -``` +See `.devin/skills/pr-review/SKILL.md` for the full review procedure, +categories, and MCP tool reference. ### 7. Address Review Comments Fix each comment one by one, commit, and push. Re-review until satisfied. -### 8. Approve and Merge -Once all checklist items are verified and comments are addressed, post -an approval review with `--checklist-confirmed` and `--checklist-categories`: +### 8. Mark Ready to Merge +Once all issues are addressed, add the `ready-to-merge` label: ```bash -CI_GITEA_TOKEN=<token> python -m devx.ci.pr_review <pr_number> <owner/repo> \ - --event APPROVE --checklist-confirmed \ - --checklist-categories 1,2,3,4,5,6,7,8,9,10,11,12,13 \ - --body "All 13 checklist categories verified. Architecture: <summary>. Security: <summary>. Tests: <summary>. Docs: <summary>." +make devx-pr-label ``` +The auto-merge workflow posts an APPROVE review via the Gitea API +and squash-merges with title `GRM-N: <conventional commit message>`. -The `--checklist-confirmed` flag is **required** for APPROVE events — -it attests that the reviewer has gone through every checklist category. -The `--checklist-categories` flag is also **required** — it must list at -least 8 of the 13 category numbers, ensuring the reviewer actually -checked each category rather than rubber-stamping. The review body must -be substantive (> 50 characters) — perfunctory approvals like "LGTM" are -rejected. +> **IMPORTANT**: Never manually merge PRs via the API. Always use the auto-merge +> workflow by adding the `ready-to-merge` label. Manual merges bypass the +> `GRM-N: <conventional>` format enforcement, producing incorrectly named commits. +> The auto-merge script validates the PR title matches the Vikunja task ID +> and conventional commit format before merging. -Then add the `ready-to-merge` label. The auto-merge workflow will: +The auto-merge workflow will: 1. **Validate** PR title format (`GRM-N: <vikunja task title>`) and match against Vikunja task title -2. **Check** that at least one substantive APPROVE review exists (body > 20 chars or has inline comments) +2. **Post** an APPROVE review via the Gitea API (to satisfy branch protection) 3. Wait for all CI checks to pass (including the `validate` job) 4. Squash-merge with title: `GRM-N: <conventional commit message>` 5. The post-merge workflow marks the Vikunja task as done @@ -190,12 +192,6 @@ a new CI run. The next auto-merge attempt will merge successfully. No manual rebase needed. To rebase manually: `make rebase` (local) or `make pr-rebase` (server-side via API). -> **IMPORTANT**: Never manually merge PRs via the API. Always use the auto-merge -> workflow by adding the `ready-to-merge` label. Manual merges bypass the -> `GRM-N: <conventional>` format enforcement, producing incorrectly named commits. -> The auto-merge script validates the PR title matches the Vikunja task ID -> and conventional commit format before merging. - ### CI Path Filtering The CI workflow's `validate` job includes a pre-merge validation step @@ -278,7 +274,7 @@ via `[tool.devx.classify]` in `pyproject.toml`. - Any new file type not in the allowlist **devx module structure** (installed from git, not in this repo): -- `devx.ci.*` — CI/CD automation (run by workflows): release, publish, auto_merge, classify_changes, detect_release_commit, push_badges, doc_coverage, sync_wiki, distribute_molecule, discover_runners, notify_failure, post_merge, pr_review, validate_commit_msg +- `devx.ci.*` — CI/CD automation (run by workflows): release, publish, auto_merge, classify_changes, detect_release_commit, push_badges, doc_coverage, sync_wiki, distribute_molecule, discover_runners, notify_failure, post_merge, validate_commit_msg - `devx.tools.*` — Dev tools (run locally): check_test_speed, configure_repo, install_checkmake, install_tools, setup, generate_badges, create_task, create_pr, pr_status, pr_logs, pr_label, rebase, pr_rebase - `devx.molecule.*` — Molecule helpers: molecule_all, platforms, discover_runners, distribute_molecule - `devx.gitea_cli` — Tea CLI wrapper @@ -334,12 +330,10 @@ The `tea` Gitea CLI tool is used for Gitea API interactions in devx. It is insta - `devx.tools.configure_repo` — Creates labels via `tea labels create` (falls back to `GiteaClient` if tea fails; branch protection still uses `GiteaClient` since tea only supports basic protect/unprotect) **Operations still using `GiteaClient` (not supported by tea):** -- PR reviews (`devx.ci.pr_review`) — tea v0.14.1 only supports interactive reviews - Wiki page management (`devx.ci.sync_wiki`) - Commit status checks (`devx.ci.auto_merge`) - Runner discovery (`devx.molecule.discover_runners`) - Branch protection with detailed config (`devx.tools.configure_repo`) -- PR file/commit listing (`devx.ci.pr_review`) ### PYTHONPATH Configuration @@ -347,7 +341,7 @@ Since devx is installed as a package (via `pip install` from git), it is importa | PYTHONPATH | When to use | Example modules | |------------|-------------|-----------------| -| `src` | Module imports from `grm` | `devx.ci.auto_merge`, `devx.ci.pr_review`, `devx.ci.pr_review`, `devx.ci.sync_wiki`, `devx.ci.post_merge`, `devx.ci.classify_changes`, `devx.molecule.discover_runners`, `devx.ci.doc_coverage` | +| `src` | Module imports from `grm` | `devx.ci.auto_merge`, `devx.ci.sync_wiki`, `devx.ci.post_merge`, `devx.ci.classify_changes`, `devx.molecule.discover_runners`, `devx.ci.doc_coverage` | | (none) | Module has no GRM imports | `devx.ci.detect_release_commit`, `devx.molecule.distribute_molecule`, `devx.ci.push_badges`, `devx.ci.validate_commit_msg` | **In workflows**, always use `env:` blocks (not inline `PYTHONPATH=value`): diff --git a/docs/specs/GRM-165.md b/docs/specs/GRM-165.md new file mode 100644 index 0000000..72d4e7f --- /dev/null +++ b/docs/specs/GRM-165.md @@ -0,0 +1,34 @@ +# GRM-165: Adopt spec-driven CI gates and pr-review skill + +## Problem +grm uses the old `devx.ci.pr_review` CI step and lacks the new spec-driven +CI gates (validate_spec, check_pr_size). It also needs a create_dependency_pr +step in post-merge to auto-create infra PRs when grm publishes a new version. + +## Approach +Replace pr_review CI steps with validate_spec + check_pr_size + curl-based +APPROVE. Add create_dependency_pr step to post-merge. Add the spec-driven- +development and pr-review skills. Update AGENTS.md. + +REQ-1: Replace pr_review CI steps with validate_spec + check_pr_size + curl-based APPROVE +REQ-2: Add create_dependency_pr step to post-merge for auto-creating infra PR to bump grm version +REQ-3: Add spec-driven-development and pr-review skills under `.devin/skills/` +REQ-4: Update AGENTS.md to document spec-driven development workflow and pr-review skill + +## Test Plan +- Verify CI workflow YAML passes actionlint +- Verify post-merge.yml includes create_dependency_pr step with correct DEVX_TASK_PREFIX (GRM) +- Verify validate_spec and check_pr_size steps reference correct DEVX_TASK_PREFIX (GRM) + +## Deploy Plan +- Merge to master via auto-merge workflow +- Post-merge workflow handles release + publish + dependency PR automatically + +## Rollback Plan +- Revert the merge commit; CI reverts to pr_review-based workflow + +## Acceptance Criteria +- [x] REQ-1: CI workflow uses validate_spec + check_pr_size + curl APPROVE instead of pr_review +- [x] REQ-2: post-merge.yml includes create_dependency_pr step targeting oblachno/infra with package grm +- [x] REQ-3: `.devin/skills/spec-driven-development/SKILL.md` and `.devin/skills/pr-review/SKILL.md` exist +- [x] REQ-4: AGENTS.md documents spec-driven development workflow and pr-review skill diff --git a/pyproject.toml b/pyproject.toml index cacdfb8..89e0890 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,7 +36,7 @@ ci = [ "build==1.5.1", "twine==6.2.0", # Reusable CI/CD and dev tools (auto-merge, pr-review, pre-push checks, etc.) - "devx @ git+https://git.oblachno.oblachno.fyi/oblachno-oss/devx.git@v0.50.1", + "devx @ git+https://git.oblachno.oblachno.fyi/oblachno-oss/devx.git@v0.50.2", ] # Lint and type-checking tools (validate job) lint = [ @@ -56,7 +56,7 @@ molecule = [ dev = [ "grm[ci,lint,molecule]", # Reusable CI/CD and dev tools (pre-push hooks, create-task, create-pr) - "devx @ git+https://git.oblachno.oblachno.fyi/oblachno-oss/devx.git@v0.50.1", + "devx @ git+https://git.oblachno.oblachno.fyi/oblachno-oss/devx.git@v0.50.2", # Non-Python dev dependency: checkmake (Makefile linter) # Install via: go install github.com/checkmake/checkmake/cmd/checkmake@latest ]