GRM-50: fix: workflow timing, timeouts, status polling, and release classification
This commit is contained in:
@@ -6,7 +6,9 @@ on:
|
||||
|
||||
jobs:
|
||||
merge:
|
||||
if: github.event.label.name == 'ready-to-merge'
|
||||
runs-on: docker
|
||||
timeout-minutes: 5
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- name: Install dependencies
|
||||
|
||||
@@ -8,6 +8,7 @@ on:
|
||||
jobs:
|
||||
quality:
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- name: Set up environment
|
||||
@@ -33,6 +34,7 @@ jobs:
|
||||
needs: [quality, detect-changes]
|
||||
if: needs.detect-changes.outputs.user-facing-changed == 'true'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
@@ -56,6 +58,7 @@ jobs:
|
||||
|
||||
detect-changes:
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
outputs:
|
||||
ansible-changed: ${{ steps.detect.outputs.ansible-changed }}
|
||||
user-facing-changed: ${{ steps.detect.outputs.user-facing-changed }}
|
||||
@@ -91,6 +94,7 @@ jobs:
|
||||
needs: [detect-changes]
|
||||
if: needs.detect-changes.outputs.ansible-changed == 'true'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
outputs:
|
||||
runner-count: ${{ steps.discover.outputs.runner-count }}
|
||||
runner-indices: ${{ steps.discover.outputs.runner-indices }}
|
||||
@@ -118,6 +122,7 @@ jobs:
|
||||
needs: [quality, detect-changes, discover-runners]
|
||||
if: needs.detect-changes.outputs.ansible-changed == 'true'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
strategy:
|
||||
matrix:
|
||||
runner-index: ${{ fromJSON(needs.discover-runners.outputs.runner-indices) }}
|
||||
@@ -147,6 +152,7 @@ jobs:
|
||||
pr-review:
|
||||
if: github.event_name == 'pull_request'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- name: Set up environment
|
||||
|
||||
@@ -21,6 +21,7 @@ on:
|
||||
jobs:
|
||||
detect-type:
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
outputs:
|
||||
is-release: ${{ steps.check.outputs.is-release }}
|
||||
steps:
|
||||
@@ -44,6 +45,7 @@ jobs:
|
||||
needs: [detect-type]
|
||||
if: needs.detect-type.outputs.is-release == 'false'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
@@ -77,6 +79,7 @@ jobs:
|
||||
needs: [detect-type]
|
||||
if: needs.detect-type.outputs.is-release == 'false'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
@@ -95,6 +98,7 @@ jobs:
|
||||
needs: [detect-type]
|
||||
if: needs.detect-type.outputs.is-release == 'false'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
@@ -123,6 +127,7 @@ jobs:
|
||||
needs: [detect-type]
|
||||
if: needs.detect-type.outputs.is-release == 'false'
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
|
||||
@@ -8,6 +8,7 @@ on:
|
||||
jobs:
|
||||
publish:
|
||||
runs-on: docker
|
||||
timeout-minutes: 10
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
with:
|
||||
|
||||
@@ -63,42 +63,31 @@ docs: update README
|
||||
|
||||
### 6. Review the PR (Mandatory — Before Adding ready-to-merge Label)
|
||||
|
||||
**Review checklist:** Every PR is reviewed against
|
||||
[REVIEW_CHECKLIST.md](REVIEW_CHECKLIST.md) — 10 categories covering
|
||||
architecture, code quality, security, i18n, testing, performance,
|
||||
UX, documentation, workflow compliance, and maintainability.
|
||||
|
||||
**Automated review (CI `pr-review` job):** Every PR triggers an automated
|
||||
review via `scripts/ci/pr_review.py`. This job posts a review with
|
||||
`COMMENT` (no issues) or `REQUEST_CHANGES` (issues found) based on:
|
||||
`COMMENT` (no issues) or `REQUEST_CHANGES` (issues found) based on
|
||||
the **[auto]** items in the checklist:
|
||||
|
||||
- **Architecture compliance**: No subprocess calls in CLI (delegate to
|
||||
executor.py), no hardcoded URLs (use config.py)
|
||||
- **Best practices**: No `print()` (use `click.echo`), no bare `except`,
|
||||
no `TODO`/`FIXME` left in merged code, no functions > 50 lines
|
||||
- **Security**: No hardcoded secrets, no `shell=True`, no `eval`/`exec`
|
||||
- **Documentation**: Source changes must include doc updates
|
||||
- **Test coverage**: Source changes must include test updates
|
||||
- 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`)
|
||||
- Documentation (source changes must include doc updates)
|
||||
- Test coverage (source changes must include test updates)
|
||||
|
||||
The automated review posts inline comments on specific lines. The agent
|
||||
**must** address all `REQUEST_CHANGES` issues before proceeding.
|
||||
The automated review posts inline comments on specific lines and
|
||||
includes a link to the full checklist. The agent **must** address all
|
||||
`REQUEST_CHANGES` issues before proceeding.
|
||||
|
||||
**Manual review (agent):** After the automated review passes, review the
|
||||
full diff (`git diff master...HEAD`) focusing on:
|
||||
|
||||
- **Functional completeness**: Does the code do what it claims? Are all requirements met?
|
||||
- **Edge cases**: Are boundary conditions, empty inputs, error paths handled?
|
||||
- **Technical excellence**:
|
||||
- Architecture compliance and evolution
|
||||
- Single Responsibility Principle (SRP)
|
||||
- Deduplication (no copy-paste, single source of truth)
|
||||
- Code smells detection and removal
|
||||
- Best industry practices
|
||||
- Industry-grade code quality
|
||||
- Reusability
|
||||
- Clean code
|
||||
- Readability
|
||||
- Maintainability
|
||||
- Extensibility
|
||||
- **Performance**: No unnecessary allocations, O(n) vs O(n²), efficient data structures
|
||||
- **Security**: No secrets in logs/process list, input validation, no injection vectors
|
||||
- **User experience**: Clear error messages, intuitive CLI flags, helpful output
|
||||
- **Documentation**: Completeness and relevance of docs, CHANGELOG entries, AGENTS.md updates
|
||||
**Manual review (agent):** After the automated review passes, the agent
|
||||
must go through **every category** in `REVIEW_CHECKLIST.md` and verify
|
||||
the **[manual]** items by reviewing the full diff
|
||||
(`git diff master...HEAD`).
|
||||
|
||||
Post review comments using `scripts/ci/review_pr.py`:
|
||||
```bash
|
||||
@@ -112,15 +101,19 @@ REPO_TOKEN=<token> python3 scripts/ci/review_pr.py <pr_number> <owner/repo> \
|
||||
Fix each comment one by one, commit, and push. Re-review until satisfied.
|
||||
|
||||
### 8. Approve and Merge
|
||||
Once all comments are addressed, post a **substantive** approval review
|
||||
(body must be > 20 characters — trivial "LGTM" approvals are rejected
|
||||
by the auto-merge gate):
|
||||
Once all checklist items are verified and comments are addressed, post
|
||||
an approval review with `--checklist-confirmed`:
|
||||
```bash
|
||||
REPO_TOKEN=<token> python3 scripts/ci/review_pr.py <pr_number> <owner/repo> \
|
||||
--event APPROVE \
|
||||
--body "All review comments addressed. Architecture compliance verified, tests pass, docs updated."
|
||||
--event APPROVE --checklist-confirmed \
|
||||
--body "All 10 REVIEW_CHECKLIST.md categories verified. Architecture: <summary>. Security: <summary>. Tests: <summary>. Docs: <summary>."
|
||||
```
|
||||
|
||||
The `--checklist-confirmed` flag is **required** for APPROVE events —
|
||||
it attests that the reviewer has gone through every checklist category.
|
||||
The review body must also be substantive (> 20 characters) — trivial
|
||||
"LGTM" approvals are rejected by the auto-merge gate.
|
||||
|
||||
Then add the `ready-to-merge` label. 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)
|
||||
|
||||
@@ -0,0 +1,100 @@
|
||||
# Review Checklist
|
||||
|
||||
This checklist is **mandatory** for every PR. The automated `pr-review` CI
|
||||
job checks items marked **[auto]**. The agent must verify all items
|
||||
marked **[manual]** before posting an APPROVE review.
|
||||
|
||||
The `review_pr.py` script requires `--checklist-confirmed` for APPROVE
|
||||
events. This flag attests that every category below has been reviewed.
|
||||
|
||||
---
|
||||
|
||||
## 1. Architecture Compliance [auto + manual]
|
||||
|
||||
- [ ] **No business logic in CLI** (`cli.py`): no `subprocess`, no
|
||||
`os.system`, no `ansible-playbook` — delegate to `executor.py`
|
||||
- [ ] **No hardcoded URLs or config values** that belong in `config.py`
|
||||
with env var overrides
|
||||
- [ ] **Layer boundaries respected**: CLI → runner_manager → executor →
|
||||
subprocess/Ansible. No skipping layers.
|
||||
- [ ] **Single Responsibility**: each module/function has one reason to
|
||||
change. If a function does two things, split it.
|
||||
- [ ] **No circular imports** introduced
|
||||
|
||||
## 2. Code Quality and Best Practices [auto + manual]
|
||||
|
||||
- [ ] **No `print()`** in `src/` — use `click.echo()` for user output
|
||||
- [ ] **No bare `except:`** — catch specific exceptions
|
||||
- [ ] **No broad `except Exception:`** without justification
|
||||
- [ ] **No `TODO`/`FIXME`/`HACK`/`XXX`** left in merged code
|
||||
- [ ] **No functions > 50 lines** (excluding docstrings and decorators)
|
||||
- [ ] **No dead code** — unused imports, unreachable branches, commented-out code
|
||||
- [ ] **No copy-paste duplication** — extract shared logic into a helper
|
||||
- [ ] **Idiomatic Python** — use comprehensions, context managers, dataclasses
|
||||
- [ ] **Type hints** on all public functions
|
||||
|
||||
## 3. Security [auto + manual]
|
||||
|
||||
- [ ] **No hardcoded secrets** (tokens, passwords, keys in string literals)
|
||||
- [ ] **No `shell=True`** in subprocess calls — use argument lists
|
||||
- [ ] **No `eval()` or `exec()`** — use `ast.literal_eval` if parsing literals
|
||||
- [ ] **No secrets in logs or process arguments** — pass via env vars or files
|
||||
- [ ] **Input validation** on all external inputs (CLI args, API responses, file contents)
|
||||
- [ ] **No injection vectors** — parameterize subprocess args, SQL queries, etc.
|
||||
|
||||
## 4. Internationalization (i18n) [manual]
|
||||
|
||||
- [ ] **All user-facing strings wrapped in `_()`** — `click.echo(_("..."))`,
|
||||
error messages, help text, prompts
|
||||
- [ ] **No raw English strings** in `click.echo()`, `click.ClickException()`,
|
||||
or `raise` messages visible to users
|
||||
- [ ] **String interpolation uses named placeholders**: `_("Hello {name}", name=x)`
|
||||
not `f"Hello {x}"` for translatable strings
|
||||
|
||||
## 5. Testability and Test Coverage [auto + manual]
|
||||
|
||||
- [ ] **Source file changes include corresponding test updates**
|
||||
- [ ] **100% coverage maintained** (enforced by `pytest-cov`)
|
||||
- [ ] **Tests are fast** (< 10 seconds total, enforced by `check_test_speed.py`)
|
||||
- [ ] **Edge cases tested**: empty inputs, boundary values, error paths
|
||||
- [ ] **No flaky tests** — no `sleep()`, no race conditions, no external dependencies
|
||||
- [ ] **Test names describe the scenario**: `test_<condition>_<expected_result>`
|
||||
|
||||
## 6. Performance [manual]
|
||||
|
||||
- [ ] **No unnecessary allocations** in hot paths (list comprehensions vs generators)
|
||||
- [ ] **Correct data structures** — O(1) lookups use `set`/`dict`, not `list`
|
||||
- [ ] **No N+1 query patterns** in API calls or file I/O
|
||||
- [ ] **No blocking I/O on hot paths** without justification
|
||||
|
||||
## 7. User Experience [manual]
|
||||
|
||||
- [ ] **Clear error messages** — tell the user what went wrong and how to fix it
|
||||
- [ ] **Consistent CLI flag naming** — `--long-name` with `--short` aliases
|
||||
- [ ] **Help text on all commands and options** — `--help` should be useful
|
||||
- [ ] **No silent failures** — if something fails, the user should know
|
||||
- [ ] **Output is actionable** — not just "Error" but "Error: X failed because Y. Try Z."
|
||||
|
||||
## 8. Documentation [auto + manual]
|
||||
|
||||
- [ ] **Source changes include doc updates** — README, wiki, AGENTS.md as needed
|
||||
- [ ] **New functions/classes have docstrings** — Google style
|
||||
- [ ] **Public API changes documented** in CHANGELOG (auto-generated by git-cliff)
|
||||
- [ ] **AGENTS.md updated** if workflow, conventions, or processes changed
|
||||
- [ ] **No stale documentation** — if code changed, docs must reflect it
|
||||
|
||||
## 9. Workflow Compliance [manual]
|
||||
|
||||
- [ ] **PR title matches Vikunja task title** (`GRM-N: <task title>`)
|
||||
- [ ] **Commit messages follow conventional format** (`type: description`)
|
||||
- [ ] **No force-push after review** — creates new commits and re-trigger CI
|
||||
- [ ] **Branch is up to date** with master before merging
|
||||
- [ ] **No merge commits** in the PR branch — use squash merge via auto-merge
|
||||
|
||||
## 10. Extensibility and Maintainability [manual]
|
||||
|
||||
- [ ] **Open/Closed Principle** — code is open for extension, closed for modification
|
||||
- [ ] **No magic numbers** — constants are named and documented
|
||||
- [ ] **Configuration over hardcoding** — use `config.py` with env var overrides
|
||||
- [ ] **Future-proof error handling** — don't catch specific error messages that may change
|
||||
- [ ] **Dependencies are justified** — no new dependency without rationale
|
||||
@@ -35,8 +35,8 @@ from gitea_runner_manager.exceptions import APIError
|
||||
from gitea_runner_manager.i18n import _
|
||||
|
||||
READY_TO_MERGE = "ready-to-merge"
|
||||
MAX_WAIT_SECONDS = 900 # 15 minutes
|
||||
POLL_INTERVAL_SECONDS = 30
|
||||
MAX_WAIT_SECONDS = 180 # 3 minutes max — CI should already be running
|
||||
POLL_INTERVAL_SECONDS = 15 # Poll every 15 seconds
|
||||
|
||||
# PR title: GRM-N: <vikunja task title>
|
||||
PR_TITLE_RE = re.compile(r"^GRM-\d+:\s+.+")
|
||||
@@ -185,6 +185,10 @@ def wait_for_ci(
|
||||
"""Poll commit statuses until all CI checks are complete (not pending).
|
||||
|
||||
Returns True if all checks are successful, False if any failed or timed out.
|
||||
|
||||
Uses the combined status endpoint which returns one entry per context
|
||||
(deduplicated server-side). Filters to "CI /" contexts only, excluding
|
||||
"Auto-merge / merge" and other non-CI contexts.
|
||||
"""
|
||||
elapsed = 0
|
||||
while elapsed < max_wait:
|
||||
@@ -195,14 +199,9 @@ def wait_for_ci(
|
||||
elapsed += poll_interval
|
||||
continue
|
||||
|
||||
# Deduplicate by context — keep the latest status per context.
|
||||
latest: dict[str, dict[str, object]] = {}
|
||||
for s in statuses:
|
||||
ctx = s.get("context", "")
|
||||
if ctx not in latest or s.get("updated_at", "") > latest[ctx].get("updated_at", ""):
|
||||
latest[ctx] = s
|
||||
|
||||
ci_statuses = {ctx: s for ctx, s in latest.items() if ctx.startswith("CI /")}
|
||||
# Combined endpoint already deduplicates — one entry per context.
|
||||
# Filter to CI contexts only (excludes "Auto-merge / merge" etc).
|
||||
ci_statuses = {s.get("context", ""): s for s in statuses if s.get("context", "").startswith("CI /")}
|
||||
if not ci_statuses:
|
||||
click.echo(_("No CI checks found yet, waiting..."))
|
||||
time.sleep(poll_interval)
|
||||
|
||||
@@ -19,10 +19,12 @@ Classification strategy (safe-by-default):
|
||||
- .gitea/workflows/** — Gitea Actions workflows
|
||||
- scripts/** — All scripts (CI/CD, dev tools, setup)
|
||||
- src/gitea_runner_manager/__init__.py — Version file (release artifact)
|
||||
- src/gitea_runner_manager/api_clients.py — Gitea API client (CI/CD only, not used by CLI)
|
||||
- docs/** — Documentation
|
||||
- tests/** — Test files
|
||||
- hooks/** — Git hooks
|
||||
- AGENTS.md — Agent conventions
|
||||
- REVIEW_CHECKLIST.md — Review checklist
|
||||
- README.md — README (lean, links to wiki)
|
||||
- CHANGELOG.md — Changelog (generated)
|
||||
- TROUBLESHOOTING.md — Troubleshooting guide
|
||||
@@ -68,9 +70,12 @@ WORKFLOW_ONLY_PATTERNS = frozenset(
|
||||
# Version file — only contains __version__, not user-facing code.
|
||||
# Version bumps are a release artifact, not a feature.
|
||||
"src/gitea_runner_manager/__init__.py",
|
||||
# Gitea API client — used only by CI/CD scripts, not by the GRM CLI.
|
||||
"src/gitea_runner_manager/api_clients.py",
|
||||
# Documentation
|
||||
"docs/",
|
||||
"AGENTS.md",
|
||||
"REVIEW_CHECKLIST.md",
|
||||
"README.md",
|
||||
"CHANGELOG.md",
|
||||
"TROUBLESHOOTING.md",
|
||||
|
||||
@@ -78,19 +78,32 @@ def main(commit_msg: str, commit_sha: str) -> None:
|
||||
|
||||
task_id = extract_task_id(commit_msg)
|
||||
if not task_id:
|
||||
# Allow release commits without GRM-N prefix
|
||||
first_line = commit_msg.split("\n")[0]
|
||||
if re.match(r"^release: v\d+\.\d+\.\d+", first_line):
|
||||
click.echo(_("Release commit without task ID, skipping Vikunja update."))
|
||||
return
|
||||
# Non-release commits must have GRM-N prefix — fail loudly
|
||||
raise click.ClickException(
|
||||
# Skip gracefully for infrastructure commits that don't follow
|
||||
# the GRM-N convention: release commits, reverts, bot commits, etc.
|
||||
infra_patterns = [
|
||||
r"^release: v\d+\.\d+\.\d+", # release commits
|
||||
r"^revert: ", # git revert commits
|
||||
r"^Merge ", # merge commits
|
||||
r"^\[skip ci\]", # skip-ci commits
|
||||
]
|
||||
for pattern in infra_patterns:
|
||||
if re.match(pattern, first_line):
|
||||
click.echo(
|
||||
_(
|
||||
"Infrastructure commit (no GRM-N task ID), skipping Vikunja update: {msg}",
|
||||
msg=first_line,
|
||||
)
|
||||
)
|
||||
return
|
||||
# Non-infrastructure commits without GRM-N prefix — warn but don't fail
|
||||
click.echo(
|
||||
_(
|
||||
"No task ID (GRM-N) found in commit message: {msg}\n"
|
||||
"All non-release commits on master must follow format: GRM-N <type>: <description>",
|
||||
"Warning: No task ID (GRM-N) found in commit message: {msg}. Skipping Vikunja update.",
|
||||
msg=first_line,
|
||||
)
|
||||
)
|
||||
return
|
||||
|
||||
client = VikunjaClient(VIKUNJA_API_URL, token)
|
||||
vikunja_task_id = 0
|
||||
|
||||
@@ -345,7 +345,13 @@ def build_review_body(result: ReviewResult) -> str:
|
||||
|
||||
lines.append("")
|
||||
lines.append("---")
|
||||
lines.append("*This review is posted by the `pr-review` CI job. The agent must address all issues before merging.*")
|
||||
lines.append("**Manual review required:** Before approving, review every category in")
|
||||
lines.append("[REVIEW_CHECKLIST.md](REVIEW_CHECKLIST.md) and confirm with:")
|
||||
lines.append("```bash")
|
||||
lines.append("python3 scripts/ci/review_pr.py <PR> <owner/repo> \\")
|
||||
lines.append(" --event APPROVE --checklist-confirmed \\")
|
||||
lines.append(' --body "<substantive review summary>"')
|
||||
lines.append("```")
|
||||
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
@@ -252,10 +252,17 @@ def create_and_push_tag(new_version: str, changelog: str, dry_run: bool) -> bool
|
||||
help="Skip lint and test verification (NOT recommended — only for emergency releases).",
|
||||
)
|
||||
def main(dry_run: bool, skip_tests: bool) -> None:
|
||||
# Ensure we're on master
|
||||
# Ensure we're on master (skip this check in dry-run mode for PR validation)
|
||||
branch = run_cmd(["git", "rev-parse", "--abbrev-ref", "HEAD"]).stdout.strip()
|
||||
if branch != "master":
|
||||
if branch != "master" and not dry_run:
|
||||
raise click.ClickException(_("Release must be run on master, currently on '{branch}'.", branch=branch))
|
||||
if branch != "master" and dry_run:
|
||||
click.echo(
|
||||
_(
|
||||
"Dry-run mode: on branch '{branch}' (not master). Some checks may behave differently.",
|
||||
branch=branch,
|
||||
)
|
||||
)
|
||||
|
||||
# Check if any user-facing files changed since the last tag.
|
||||
# If only workflow/infra files changed, skip the release entirely.
|
||||
|
||||
+28
-10
@@ -19,16 +19,11 @@ The comments JSON file is a list of objects with keys:
|
||||
- new_position: line number in the new file (1-based)
|
||||
- old_position: (optional) line number in the old file
|
||||
|
||||
Review focus areas (for the reviewer, not enforced by this script):
|
||||
- Functional completeness
|
||||
- Edge cases
|
||||
- Technical excellence: architecture compliance, SRP, deduplication,
|
||||
code smells, best practices, code quality, reusability, clean code,
|
||||
readability, maintainability, extensibility
|
||||
- Performance
|
||||
- Security
|
||||
- User experience
|
||||
- Documentation completeness and relevance
|
||||
For APPROVE events, --checklist-confirmed is required. This attests
|
||||
that the reviewer has gone through every category in
|
||||
REVIEW_CHECKLIST.md. The review body must also be substantive
|
||||
(> 20 characters) — trivial "LGTM" approvals are rejected by the
|
||||
auto-merge gate.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -93,6 +88,12 @@ def parse_comments(comments_json: str | None, comments_stdin: bool) -> list[dict
|
||||
default=False,
|
||||
help="Read inline comments JSON from stdin.",
|
||||
)
|
||||
@click.option(
|
||||
"--checklist-confirmed",
|
||||
is_flag=True,
|
||||
default=False,
|
||||
help="Required for APPROVE: confirms all REVIEW_CHECKLIST.md categories reviewed.",
|
||||
)
|
||||
def main(
|
||||
pr_number: str,
|
||||
repo: str,
|
||||
@@ -100,6 +101,7 @@ def main(
|
||||
body: str,
|
||||
comments_json: str | None,
|
||||
comments_stdin: bool,
|
||||
checklist_confirmed: bool,
|
||||
) -> None:
|
||||
token = os.environ.get("REPO_TOKEN", "")
|
||||
if not token:
|
||||
@@ -113,6 +115,22 @@ def main(
|
||||
if event != "APPROVE" and not body and not comments:
|
||||
raise click.ClickException(_("Review body or inline comments are required for event '{event}'.", event=event))
|
||||
|
||||
if event == "APPROVE":
|
||||
if not checklist_confirmed:
|
||||
raise click.ClickException(
|
||||
_(
|
||||
"APPROVE requires --checklist-confirmed. "
|
||||
"Review every category in REVIEW_CHECKLIST.md before approving."
|
||||
)
|
||||
)
|
||||
if len(body.strip()) <= 20 and not comments:
|
||||
raise click.ClickException(
|
||||
_(
|
||||
"APPROVE review body must be substantive (> 20 characters) "
|
||||
"or include inline comments. Trivial approvals are rejected."
|
||||
)
|
||||
)
|
||||
|
||||
try:
|
||||
review = client.create_review(pr_number, event=event, body=body, comments=comments)
|
||||
except APIError as e:
|
||||
|
||||
@@ -129,9 +129,16 @@ class GiteaClient:
|
||||
self._request("POST", f"/pulls/{pr_number}/merge", json=payload)
|
||||
|
||||
def get_commit_status(self, sha: str) -> list[dict[str, Any]]:
|
||||
"""Fetch all status check contexts reported for a commit."""
|
||||
r = self._request("GET", f"/commits/{sha}/statuses")
|
||||
return r.json()
|
||||
"""Fetch all status check contexts reported for a commit.
|
||||
|
||||
Uses the combined status endpoint (/commits/{sha}/status) which
|
||||
returns one entry per context (the latest), deduplicated server-side.
|
||||
The plural endpoint (/commits/{sha}/statuses) returns every historical
|
||||
entry including stale "pending" ones that never got updated.
|
||||
"""
|
||||
r = self._request("GET", f"/commits/{sha}/status")
|
||||
data = r.json()
|
||||
return data.get("statuses", [])
|
||||
|
||||
def get_pr(self, pr_number: str | int) -> dict[str, Any]:
|
||||
"""Fetch pull request details including mergeable state."""
|
||||
|
||||
@@ -202,16 +202,17 @@ class TestGiteaClient:
|
||||
)
|
||||
|
||||
def test_get_commit_status(self) -> None:
|
||||
"""Uses combined status endpoint (/status, not /statuses)."""
|
||||
client = GiteaClient("https://git.example.com", "tok", "owner", "repo")
|
||||
client._session.request = MagicMock(
|
||||
return_value=_mock_response([{"context": "CI / quality", "status": "success"}])
|
||||
return_value=_mock_response({"statuses": [{"context": "CI / quality", "status": "success"}]})
|
||||
)
|
||||
|
||||
result = client.get_commit_status("abc123")
|
||||
assert result == [{"context": "CI / quality", "status": "success"}]
|
||||
client._session.request.assert_called_once_with(
|
||||
"GET",
|
||||
"https://git.example.com/repos/owner/repo/commits/abc123/statuses",
|
||||
"https://git.example.com/repos/owner/repo/commits/abc123/status",
|
||||
timeout=DEFAULT_TIMEOUT,
|
||||
)
|
||||
|
||||
|
||||
@@ -321,6 +321,8 @@ class TestWaitForCi:
|
||||
assert wait_for_ci(client, "abc123", max_wait=10) is True
|
||||
|
||||
def test_deduplicates_by_latest(self) -> None:
|
||||
"""Combined endpoint returns one entry per context; if multiple
|
||||
entries appear, the last one wins (dict comprehension)."""
|
||||
client = MagicMock()
|
||||
client.get_commit_status.return_value = [
|
||||
_status("CI / quality (pull_request)", CI_PENDING, "2026-01-01T00:00:00Z"),
|
||||
|
||||
@@ -53,6 +53,14 @@ class TestIsUserFacing:
|
||||
not user-facing code. Version bumps alone should not trigger releases."""
|
||||
assert is_user_facing("src/gitea_runner_manager/__init__.py") is False
|
||||
|
||||
def test_api_clients_is_not_user_facing(self) -> None:
|
||||
"""api_clients.py is used only by CI/CD scripts, not by the GRM CLI."""
|
||||
assert is_user_facing("src/gitea_runner_manager/api_clients.py") is False
|
||||
|
||||
def test_review_checklist_is_not_user_facing(self) -> None:
|
||||
"""REVIEW_CHECKLIST.md is agent infrastructure, not user-facing."""
|
||||
assert is_user_facing("REVIEW_CHECKLIST.md") is False
|
||||
|
||||
def test_docs_are_not_user_facing(self) -> None:
|
||||
assert is_user_facing("docs/user/getting-started.md") is False
|
||||
|
||||
|
||||
@@ -130,12 +130,13 @@ class TestMain:
|
||||
assert "VIKUNJA_TOKEN" in result.output
|
||||
|
||||
@patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"})
|
||||
def test_no_task_id_non_release_fails(self) -> None:
|
||||
"""Non-release commits without GRM-N prefix should fail."""
|
||||
def test_no_task_id_non_release_warns(self) -> None:
|
||||
"""Non-release commits without GRM-N prefix should warn, not fail."""
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["fix: resolve bug"])
|
||||
assert result.exit_code == 1
|
||||
assert result.exit_code == 0
|
||||
assert "No task ID" in result.output
|
||||
assert "Skipping" in result.output
|
||||
|
||||
@patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"})
|
||||
def test_release_commit_without_task_id_skips(self) -> None:
|
||||
@@ -143,9 +144,25 @@ class TestMain:
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["release: v0.3.2"])
|
||||
assert result.exit_code == 0
|
||||
assert "Release commit" in result.output
|
||||
assert "skipping" in result.output
|
||||
|
||||
@patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"})
|
||||
def test_revert_commit_skips(self) -> None:
|
||||
"""Revert commits without GRM-N prefix should skip gracefully."""
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["revert: remove v0.6.0 release"])
|
||||
assert result.exit_code == 0
|
||||
assert "Infrastructure commit" in result.output
|
||||
assert "skipping" in result.output
|
||||
|
||||
@patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"})
|
||||
def test_merge_commit_skips(self) -> None:
|
||||
"""Merge commits without GRM-N prefix should skip gracefully."""
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["Merge pull request #42"])
|
||||
assert result.exit_code == 0
|
||||
assert "Infrastructure commit" in result.output
|
||||
|
||||
@patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"})
|
||||
@patch("scripts.ci.post_merge.VikunjaClient")
|
||||
def test_resolve_failure_propagates(self, mock_client_cls: MagicMock) -> None:
|
||||
|
||||
@@ -367,6 +367,13 @@ class TestBuildReviewBody:
|
||||
body = build_review_body(result)
|
||||
assert "No issues found" in body
|
||||
|
||||
def test_body_contains_checklist_reference(self) -> None:
|
||||
"""Review body must reference REVIEW_CHECKLIST.md for manual review."""
|
||||
result = ReviewResult()
|
||||
body = build_review_body(result)
|
||||
assert "REVIEW_CHECKLIST.md" in body
|
||||
assert "--checklist-confirmed" in body
|
||||
|
||||
|
||||
class TestRunReview:
|
||||
@patch("scripts.ci.pr_review.GiteaClient")
|
||||
|
||||
@@ -302,6 +302,17 @@ class TestMain:
|
||||
assert result.exit_code != 0
|
||||
assert "master" in result.output
|
||||
|
||||
@patch.dict("os.environ", {})
|
||||
@patch("scripts.ci.release.has_user_facing_changes", return_value=False)
|
||||
@patch("scripts.ci.release.run_cmd")
|
||||
def test_dry_run_on_non_master_warns(self, mock_run_cmd: MagicMock, mock_uf: MagicMock) -> None:
|
||||
"""Dry-run mode should not fail on non-master branches."""
|
||||
mock_run_cmd.return_value = MagicMock(returncode=0, stdout="feature-branch\n", stderr="")
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["--dry-run"])
|
||||
assert result.exit_code == 0
|
||||
assert "Dry-run mode" in result.output
|
||||
|
||||
@patch.dict("os.environ", {})
|
||||
@patch("scripts.ci.release.has_user_facing_changes", return_value=True)
|
||||
@patch("scripts.ci.release.has_unreleased_changes", return_value=False)
|
||||
|
||||
@@ -77,14 +77,61 @@ class TestMain:
|
||||
@patch.dict("os.environ", {"REPO_TOKEN": "tok"})
|
||||
@patch("scripts.ci.review_pr.GiteaClient")
|
||||
def test_successful_approve_review(self, mock_client_cls: MagicMock) -> None:
|
||||
"""APPROVE requires --checklist-confirmed and substantive body."""
|
||||
mock_client = MagicMock()
|
||||
mock_client.create_review.return_value = {"id": 7}
|
||||
mock_client_cls.return_value = mock_client
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(main, ["5", "owner/repo", "--event", "APPROVE"])
|
||||
result = runner.invoke(
|
||||
main,
|
||||
[
|
||||
"5",
|
||||
"owner/repo",
|
||||
"--event",
|
||||
"APPROVE",
|
||||
"--checklist-confirmed",
|
||||
"--body",
|
||||
"All 10 checklist categories verified. Architecture OK, tests pass.",
|
||||
],
|
||||
)
|
||||
assert result.exit_code == 0
|
||||
assert "Review #7" in result.output
|
||||
mock_client.create_review.assert_called_once_with("5", event="APPROVE", body="", comments=[])
|
||||
mock_client.create_review.assert_called_once_with(
|
||||
"5",
|
||||
event="APPROVE",
|
||||
body="All 10 checklist categories verified. Architecture OK, tests pass.",
|
||||
comments=[],
|
||||
)
|
||||
|
||||
@patch.dict("os.environ", {"REPO_TOKEN": "tok"})
|
||||
@patch("scripts.ci.review_pr.GiteaClient")
|
||||
def test_approve_without_checklist_confirmed_fails(self, mock_client_cls: MagicMock) -> None:
|
||||
"""APPROVE without --checklist-confirmed is rejected."""
|
||||
mock_client = MagicMock()
|
||||
mock_client_cls.return_value = mock_client
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(
|
||||
main,
|
||||
["5", "owner/repo", "--event", "APPROVE", "--body", "Looks good to me"],
|
||||
)
|
||||
assert result.exit_code != 0
|
||||
assert "checklist" in result.output.lower()
|
||||
mock_client.create_review.assert_not_called()
|
||||
|
||||
@patch.dict("os.environ", {"REPO_TOKEN": "tok"})
|
||||
@patch("scripts.ci.review_pr.GiteaClient")
|
||||
def test_approve_with_trivial_body_fails(self, mock_client_cls: MagicMock) -> None:
|
||||
"""APPROVE with trivial body (< 20 chars) and no comments is rejected."""
|
||||
mock_client = MagicMock()
|
||||
mock_client_cls.return_value = mock_client
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(
|
||||
main,
|
||||
["5", "owner/repo", "--event", "APPROVE", "--checklist-confirmed", "--body", "LGTM"],
|
||||
)
|
||||
assert result.exit_code != 0
|
||||
assert "substantive" in result.output.lower()
|
||||
mock_client.create_review.assert_not_called()
|
||||
|
||||
@patch.dict("os.environ", {"REPO_TOKEN": "tok"})
|
||||
@patch("scripts.ci.review_pr.GiteaClient")
|
||||
|
||||
Reference in New Issue
Block a user