fix: dora tool docstring and sort fallback #6

Merged

ldraney merged 1 commit from fix/dora-tool-nits into main

2026-06-14 11:32:33 +00:00

ldraney commented

2026-06-14 01:56:00 +00:00

Owner

Summary

Fix two nits from PR #5 review: module docstring missing (N tools) suffix convention, and timestamp sort fallback using wrong type.

Changes

src/woodpecker_mcp/tools/dora.py: Added (1 tool) suffix to module docstring to match convention used by all other tool modules (pipelines, repos, secrets, crons, system, queue)
src/woodpecker_mcp/tools/dora.py: Changed sort fallback from 0 (int) to "" (str) to prevent mixed-type comparison when created_at is missing

Test Plan

All 30 tests pass (2 pre-existing skips)
ruff format and lint clean

Nits from post-merge review of PR #5

🤖 Generated with Claude Code

## Summary Fix two nits from PR #5 review: module docstring missing `(N tools)` suffix convention, and timestamp sort fallback using wrong type. ## Changes - `src/woodpecker_mcp/tools/dora.py`: Added `(1 tool)` suffix to module docstring to match convention used by all other tool modules (pipelines, repos, secrets, crons, system, queue) - `src/woodpecker_mcp/tools/dora.py`: Changed sort fallback from `0` (int) to `""` (str) to prevent mixed-type comparison when `created_at` is missing ## Test Plan - [x] All 30 tests pass (2 pre-existing skips) - [x] ruff format and lint clean ## Related - Nits from post-merge review of PR #5 🤖 Generated with [Claude Code](https://claude.com/claude-code)

ldraney added 1 commit

2026-06-14 01:56:00 +00:00

fix: dora tool docstring convention and sort fallback type 552f18bd72

Add (1 tool) suffix to module docstring to match codebase convention
used by all other tool modules. Fix timestamp sort fallback from 0
(int) to "" (str) to prevent mixed-type comparison on missing
created_at values.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented

2026-06-14 02:00:27 +00:00

Author

Owner

PR #6 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI-MCP / Woodpecker CI API integration.

Change 1 -- Docstring (1 tool) suffix: Verified against all other tool modules on main:

Module	Docstring suffix
`pipelines.py`	`(6 tools)`
`repos.py`	`(2 tools)`
`queue.py`	`(1 tool)`
`secrets.py`	`(4) + global secret CRUD (4) = 8 tools`
`crons.py`	`(5 tools)`
`system.py`	`(3 tools)`
`dora.py` (before)	missing

The fix correctly adds (1 tool) to match the established convention. Correct.

Change 2 -- Sort fallback or 0 to or "": This is where it gets interesting.

The created_at values come from p.get("created_at") on the raw Woodpecker API response dict. The Woodpecker CI API (confirmed via the woodpecker-sdk swagger spec in the sibling repo) returns pipeline timestamps under the field name created -- not created_at -- and the value is an integer Unix timestamp.

This means:

In production, p.get("created_at") will always return None (wrong key name), so the fallback fires on every entry.
The original or 0 fallback is actually type-correct for integer Unix timestamps.
The new or "" fallback changes the sort to compare empty strings, which works (all entries sort identically), but is arguably less correct -- if the key name bug were fixed, created_at would be an int, and "" would cause a TypeError on mixed comparison (int vs str).

In other words: this PR fixes a hypothetical type mismatch that does not exist, while leaving an actual field-name bug (created_at vs created) untouched. Fixing the field name would reintroduce the original or 0 as the correct fallback.

BLOCKERS

None. Both changes are low-risk. The sort fallback change is technically a lateral move (neither value works correctly due to the underlying field-name issue), not a regression.

NITS

Field name bug (pre-existing, not introduced by this PR): p.get("created_at") should be p.get("created") to match the Woodpecker API schema. This affects dora.py lines 99-101 and also appears in pipelines.py (lines 34, 73, 270) and crons.py (line 34). Worth a follow-up issue -- the sort fallback fix in this PR becomes moot once the field name is corrected, and the correct fallback for integer timestamps would be or 0, not or "".
Sort order test gap (pre-existing): test_recent_failures_limited verifies the count is capped but does not assert the returned order is descending by timestamp. A one-line assertion on the first vs. last created_at values would close this gap.

SOP COMPLIANCE

PR body has Summary, Changes, Test Plan, Related
No secrets or credentials committed
No scope creep -- exactly 2 targeted line changes
Commit message is descriptive (fix: dora tool docstring and sort fallback)

PROCESS OBSERVATIONS

The created_at vs created field name mismatch across multiple tool modules suggests integration tests against a live Woodpecker instance may not be exercising timestamp fields. The two skipped integration tests in test_dora.py would catch this if run. Consider prioritizing a sweep of all get("created_at") / get("started_at") / get("finished_at") calls against the actual API schema -- this is a class of bug, not an isolated typo.

VERDICT: APPROVED

Both changes are correct in isolation and low-risk. The docstring fix is unambiguously right. The sort fallback change is a lateral move due to the underlying field-name bug, but it does not introduce a regression. Recommend a follow-up issue for the created_at vs created field name mismatch across the codebase.

## PR #6 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI-MCP / Woodpecker CI API integration. **Change 1 -- Docstring `(1 tool)` suffix**: Verified against all other tool modules on `main`: | Module | Docstring suffix | |--------|-----------------| | `pipelines.py` | `(6 tools)` | | `repos.py` | `(2 tools)` | | `queue.py` | `(1 tool)` | | `secrets.py` | `(4) + global secret CRUD (4) = 8 tools` | | `crons.py` | `(5 tools)` | | `system.py` | `(3 tools)` | | `dora.py` (before) | **missing** | The fix correctly adds `(1 tool)` to match the established convention. Correct. **Change 2 -- Sort fallback `or 0` to `or ""`**: This is where it gets interesting. The `created_at` values come from `p.get("created_at")` on the raw Woodpecker API response dict. The Woodpecker CI API (confirmed via the `woodpecker-sdk` swagger spec in the sibling repo) returns pipeline timestamps under the field name `created` -- **not** `created_at` -- and the value is an **integer** Unix timestamp. This means: - In production, `p.get("created_at")` will always return `None` (wrong key name), so the fallback fires on every entry. - The original `or 0` fallback is actually **type-correct** for integer Unix timestamps. - The new `or ""` fallback changes the sort to compare empty strings, which works (all entries sort identically), but is arguably less correct -- if the key name bug were fixed, `created_at` would be an `int`, and `""` would cause a `TypeError` on mixed comparison (`int` vs `str`). In other words: this PR fixes a hypothetical type mismatch that does not exist, while leaving an actual field-name bug (`created_at` vs `created`) untouched. Fixing the field name would **reintroduce** the original `or 0` as the correct fallback. ### BLOCKERS None. Both changes are low-risk. The sort fallback change is technically a lateral move (neither value works correctly due to the underlying field-name issue), not a regression. ### NITS 1. **Field name bug (pre-existing, not introduced by this PR)**: `p.get("created_at")` should be `p.get("created")` to match the Woodpecker API schema. This affects `dora.py` lines 99-101 and also appears in `pipelines.py` (lines 34, 73, 270) and `crons.py` (line 34). Worth a follow-up issue -- the sort fallback fix in this PR becomes moot once the field name is corrected, and the correct fallback for integer timestamps would be `or 0`, not `or ""`. 2. **Sort order test gap (pre-existing)**: `test_recent_failures_limited` verifies the count is capped but does not assert the returned order is descending by timestamp. A one-line assertion on the first vs. last `created_at` values would close this gap. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related - [x] No secrets or credentials committed - [x] No scope creep -- exactly 2 targeted line changes - [x] Commit message is descriptive (`fix: dora tool docstring and sort fallback`) ### PROCESS OBSERVATIONS The `created_at` vs `created` field name mismatch across multiple tool modules suggests integration tests against a live Woodpecker instance may not be exercising timestamp fields. The two skipped integration tests in `test_dora.py` would catch this if run. Consider prioritizing a sweep of all `get("created_at")` / `get("started_at")` / `get("finished_at")` calls against the actual API schema -- this is a class of bug, not an isolated typo. ### VERDICT: APPROVED Both changes are correct in isolation and low-risk. The docstring fix is unambiguously right. The sort fallback change is a lateral move due to the underlying field-name bug, but it does not introduce a regression. Recommend a follow-up issue for the `created_at` vs `created` field name mismatch across the codebase.