ldraney/forgejo-mcp

Fork 0

fix: epilogue nits -- pagination, test rename, docs, CI #12

Merged

forgejo_admin merged 1 commit from 11-epilogue-nits-pagination-test-rename-doc into main

2026-03-13 20:49:57 +00:00

forgejo_admin commented

2026-03-13 20:41:11 +00:00

Contributor

Summary

Addresses all 4 QA nits from PR #10 review: set_label pagination bug, test file naming, missing documentation for the comment_on_issue/comment_on_pr distinction, and missing pytest CI step.

Changes

src/forgejo_mcp/tools/workflows.py -- Paginate set_label through all repo labels (page-by-page with page_size=50) instead of fetching only the first 100. Added docstrings to comment_on_issue and comment_on_pr documenting they share the same Forgejo API endpoint.
tests/test_new_tools.py -> tests/test_label_comment_repo.py -- Renamed to descriptive filename matching the tools it tests.
tests/test_set_label_pagination.py -- New unit tests (4 cases) verifying pagination works with mocked 150+ labels, single-page repos, missing labels after full pagination, and empty repos.
README.md -- Updated tool list to reflect all current MCP tools. Added section documenting comment_on_issue vs comment_on_pr shared endpoint.
.woodpecker.yml -- Replaced lint-only step with combined test step that installs the package, runs ruff lint+format on src/ and tests/, then runs pytest. Matches the CI pattern used in basketball-api and pal-e-docs.

Test Plan

ruff check src/ tests/ passes
ruff format --check src/ tests/ passes
pytest tests/ -v -- 4 unit tests pass, 8 integration tests correctly skipped (no Forgejo creds)
Woodpecker CI pipeline runs test step on this PR

Review Checklist

Acceptance criteria from issue #11 met (pagination, rename, docs, CI)
No unrelated changes
Existing tests still pass
Lint and format clean

Plan: plan-pal-e-agency (traceability)
Forgejo issue: #11

Closes #11

## Summary Addresses all 4 QA nits from PR #10 review: set_label pagination bug, test file naming, missing documentation for the comment_on_issue/comment_on_pr distinction, and missing pytest CI step. ## Changes - `src/forgejo_mcp/tools/workflows.py` -- Paginate `set_label` through all repo labels (page-by-page with page_size=50) instead of fetching only the first 100. Added docstrings to `comment_on_issue` and `comment_on_pr` documenting they share the same Forgejo API endpoint. - `tests/test_new_tools.py` -> `tests/test_label_comment_repo.py` -- Renamed to descriptive filename matching the tools it tests. - `tests/test_set_label_pagination.py` -- New unit tests (4 cases) verifying pagination works with mocked 150+ labels, single-page repos, missing labels after full pagination, and empty repos. - `README.md` -- Updated tool list to reflect all current MCP tools. Added section documenting `comment_on_issue` vs `comment_on_pr` shared endpoint. - `.woodpecker.yml` -- Replaced lint-only step with combined test step that installs the package, runs ruff lint+format on src/ and tests/, then runs pytest. Matches the CI pattern used in basketball-api and pal-e-docs. ## Test Plan - [x] `ruff check src/ tests/` passes - [x] `ruff format --check src/ tests/` passes - [x] `pytest tests/ -v` -- 4 unit tests pass, 8 integration tests correctly skipped (no Forgejo creds) - [ ] Woodpecker CI pipeline runs test step on this PR ## Review Checklist - [x] Acceptance criteria from issue #11 met (pagination, rename, docs, CI) - [x] No unrelated changes - [x] Existing tests still pass - [x] Lint and format clean ## Related - Plan: `plan-pal-e-agency` (traceability) - Forgejo issue: #11 Closes #11

forgejo_admin added 1 commit

2026-03-13 20:41:11 +00:00

fix: epilogue nits -- pagination, test rename, docs, CI

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

22d906bf2e

- Paginate set_label through all repo labels instead of capping at 100
- Rename test_new_tools.py to test_label_comment_repo.py
- Document comment_on_issue vs comment_on_pr shared endpoint in
  docstrings and README
- Add pytest step to Woodpecker CI pipeline (lint + test in one step)
- Add unit tests for set_label pagination with mocked 150+ labels
- Update README tool list to reflect all current MCP tools

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-13 20:41:42 +00:00

Author

Contributor

Self-Review: LGTM

Verified all 5 acceptance criteria from issue #11 against the diff:

set_label pagination -- Pagination loop with page/limit params correctly iterates through all repo labels. Breaks on empty batch or partial page. No off-by-one risk.
Test file renamed -- Pure rename (test_new_tools.py -> test_label_comment_repo.py), 100% similarity.
Docstring + README docs -- Both comment_on_issue and comment_on_pr docstrings explain the shared Forgejo API endpoint. README has a dedicated subsection.
Woodpecker CI pytest step -- Combined test step: venv, install, ruff lint+format (now covers tests/ too), pytest. Matches platform CI pattern. Publish step preserved.
Pagination unit tests -- 4 cases (multi-page, single-page, not-found, empty-repo) all pass with mocked client.

Additional cleanup: removed duplicate header comments in .woodpecker.yml, updated step syntax to list-of-maps format.

No issues found. Ready for human review.

## Self-Review: LGTM Verified all 5 acceptance criteria from issue #11 against the diff: 1. **set_label pagination** -- Pagination loop with `page`/`limit` params correctly iterates through all repo labels. Breaks on empty batch or partial page. No off-by-one risk. 2. **Test file renamed** -- Pure rename (`test_new_tools.py` -> `test_label_comment_repo.py`), 100% similarity. 3. **Docstring + README docs** -- Both `comment_on_issue` and `comment_on_pr` docstrings explain the shared Forgejo API endpoint. README has a dedicated subsection. 4. **Woodpecker CI pytest step** -- Combined test step: venv, install, ruff lint+format (now covers `tests/` too), pytest. Matches platform CI pattern. Publish step preserved. 5. **Pagination unit tests** -- 4 cases (multi-page, single-page, not-found, empty-repo) all pass with mocked client. Additional cleanup: removed duplicate header comments in `.woodpecker.yml`, updated step syntax to list-of-maps format. No issues found. Ready for human review.

forgejo_admin commented

2026-03-13 20:45:01 +00:00

Author

Contributor

PR #12 Review

BLOCKERS

1. Woodpecker step-level when is redundant and may cause issues

The test step has its own when clause:

    when:
      - event: [push, pull_request]

This duplicates the top-level when filter. While Woodpecker should handle this gracefully (inner when narrows the outer), it is unnecessary duplication. The publish step correctly uses a narrower when (push + main only), which makes sense. The test step's when is identical to the top-level and adds no value. Not a merge blocker, but worth noting -- see NITS.

No blockers found. All four acceptance criteria from issue #11 are met.

NITS

1. Redundant when on the test step (.woodpecker.yml line 14-15)
The top-level when already gates on [push, pull_request]. The step-level when repeats this. The publish step's narrower when (push + main) is the correct pattern for step-level filtering. Consider removing the redundant when from the test step for clarity.

2. Old duplicate comment lines removed -- good cleanup
The original .woodpecker.yml had three identical # Woodpecker CI pipeline for forgejo-mcp comment lines. The PR correctly reduces this to one. Noted as positive.

3. test_single_page_no_extra_calls uses return_value instead of side_effect (test_set_label_pagination.py line 60)
The test uses client.issue_list_labels.return_value = labels which means the mock returns the same list for every call, including the hypothetical second call that the code does NOT make. This still validates the assertion (call_count == 1) correctly, but using side_effect = [labels] would be marginally more precise -- it would raise StopIteration if a second call were accidentally made. Not blocking.

4. Integration test in test_label_comment_repo.py line 29 still uses limit=100
The renamed integration test file still has forgejo_client.issue_list_labels(forgejo_username, repo_name, limit=100) for its own label setup. This is fine -- the integration test calls the SDK directly for test setup, not through the MCP tool. The MCP tool itself now paginates correctly. Just flagging for awareness that the integration test's setup code takes a different approach than the tool.

SOP COMPLIANCE

Branch named after issue -- 11-epilogue-nits-pagination-test-rename-doc references issue #11
PR body has Summary, Changes, Test Plan, Related -- all present and well-structured
Related references plan slug -- plan-pal-e-agency referenced
PR body contains Closes #11 -- present at the bottom of the body
No secrets or .env files committed -- verified clean
No unrelated changes -- all 5 changed files map to the 4 acceptance criteria
Tests exist -- 4 new unit tests for pagination + 8 existing integration tests preserved
Commit scope matches issue -- pagination fix, test rename, docs, CI

ASSESSMENT

Pagination fix is correct. The while True loop with page_size = 50 and len(batch) < page_size break condition properly handles multi-page label sets. The four unit tests cover the key scenarios: multi-page success (150 labels), single-page optimization, label-not-found after full pagination, and empty repo.

Test rename is clean. test_new_tools.py -> test_label_comment_repo.py with similarity index 100%. No broken imports anywhere in the codebase.

Documentation is accurate. Both docstrings and the README correctly explain the shared Forgejo API endpoint. The README tool list now reflects all 14 current tools.

CI step correctly installs .[dev] (which includes pytest + ruff per pyproject.toml), runs ruff lint and format checks on both src/ and tests/, then runs pytest. Uses a venv in /tmp/venv to avoid polluting the workspace.

VERDICT: APPROVED

## PR #12 Review ### BLOCKERS **1. Woodpecker step-level `when` is redundant and may cause issues** The `test` step has its own `when` clause: ```yaml when: - event: [push, pull_request] ``` This duplicates the top-level `when` filter. While Woodpecker should handle this gracefully (inner `when` narrows the outer), it is unnecessary duplication. The `publish` step correctly uses a narrower `when` (push + main only), which makes sense. The `test` step's `when` is identical to the top-level and adds no value. Not a merge blocker, but worth noting -- see NITS. **No blockers found.** All four acceptance criteria from issue #11 are met. ### NITS **1. Redundant `when` on the `test` step** (`.woodpecker.yml` line 14-15) The top-level `when` already gates on `[push, pull_request]`. The step-level `when` repeats this. The `publish` step's narrower `when` (push + main) is the correct pattern for step-level filtering. Consider removing the redundant `when` from the `test` step for clarity. **2. Old duplicate comment lines removed -- good cleanup** The original `.woodpecker.yml` had three identical `# Woodpecker CI pipeline for forgejo-mcp` comment lines. The PR correctly reduces this to one. Noted as positive. **3. `test_single_page_no_extra_calls` uses `return_value` instead of `side_effect`** (`test_set_label_pagination.py` line 60) The test uses `client.issue_list_labels.return_value = labels` which means the mock returns the same list for every call, including the hypothetical second call that the code does NOT make. This still validates the assertion (`call_count == 1`) correctly, but using `side_effect = [labels]` would be marginally more precise -- it would raise `StopIteration` if a second call were accidentally made. Not blocking. **4. Integration test in `test_label_comment_repo.py` line 29 still uses `limit=100`** The renamed integration test file still has `forgejo_client.issue_list_labels(forgejo_username, repo_name, limit=100)` for its own label setup. This is fine -- the integration test calls the SDK directly for test setup, not through the MCP tool. The MCP tool itself now paginates correctly. Just flagging for awareness that the integration test's setup code takes a different approach than the tool. ### SOP COMPLIANCE - [x] **Branch named after issue** -- `11-epilogue-nits-pagination-test-rename-doc` references issue #11 - [x] **PR body has Summary, Changes, Test Plan, Related** -- all present and well-structured - [x] **Related references plan slug** -- `plan-pal-e-agency` referenced - [x] **PR body contains `Closes #11`** -- present at the bottom of the body - [x] **No secrets or `.env` files committed** -- verified clean - [x] **No unrelated changes** -- all 5 changed files map to the 4 acceptance criteria - [x] **Tests exist** -- 4 new unit tests for pagination + 8 existing integration tests preserved - [x] **Commit scope matches issue** -- pagination fix, test rename, docs, CI ### ASSESSMENT **Pagination fix** is correct. The `while True` loop with `page_size = 50` and `len(batch) < page_size` break condition properly handles multi-page label sets. The four unit tests cover the key scenarios: multi-page success (150 labels), single-page optimization, label-not-found after full pagination, and empty repo. **Test rename** is clean. `test_new_tools.py` -> `test_label_comment_repo.py` with `similarity index 100%`. No broken imports anywhere in the codebase. **Documentation** is accurate. Both docstrings and the README correctly explain the shared Forgejo API endpoint. The README tool list now reflects all 14 current tools. **CI step** correctly installs `.[dev]` (which includes pytest + ruff per `pyproject.toml`), runs ruff lint and format checks on both `src/` and `tests/`, then runs pytest. Uses a venv in `/tmp/venv` to avoid polluting the workspace. ### VERDICT: APPROVED