23: Add forgejo_milestone_edit tool #37

Merged

david merged 2 commits from feature/issue-23/add-forgejo-milestone-edit-tool into main

2026-08-18 08:18:18 +00:00

david commented

2026-08-18 08:18:16 +00:00

Owner

23: Add forgejo_milestone_edit tool

Implements 23.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 8m 32s.

Review findings

Now I have the full picture. Let me look at the test file context for the milestone_edit describe block, and check the AGENTS.md for commit conventions:All context gathered. Now applying the fixes. First, the README tools table (Medium finding):Now the DESIGN.md matrix (Low finding):Now the client-side guard in src/index.ts (Low finding):ApiErrorCode isn't imported yet. Let me check the imports at the top of index.ts:Now add a test for the new guard in tests/tools.test.ts:Now let me detect and run the build/test gates:The existing API-error test passed only id — with the new guard it now (correctly) fails fast. Updating that test to include an editable field so it still exercises the API-error path:All gates pass. Let me verify the working tree diff is clean of unintended artifacts:Let me also verify the root project gates still pass (in case the root has its own config):All gates pass (npm run check, npm run build, npm test — 164 tests, 9 suites). The changes are minimal and consistent. Final annotated findings:

Review Findings — 23

Summary

Change adds a forgejo_milestone_edit tool (register + editMilestone PATCH helper + unit tests), matching the pattern of the existing issue/PR edit tools and the milestone create/view tools. Gates run: npm run build (tsc) — PASS; npm run check (tsc --noEmit) — PASS; npm test (jest) — PASS (164 tests, 9 suites); coverage — milestones.ts 100% lines/branches and the new tool handler is exercised. Lint is not configured in this repo (no eslint/prettier config or lint script), so no lint gate was available. No correctness bugs or unmet acceptance criteria found (issue has no AC beyond the tool itself). Findings are documentation polish and minor convention items.

Remediation applied in this pass: added the forgejo_milestone_edit row to the README tools table, added the row to the DESIGN.md tool matrix (plus updated the Issues count and the milestone endpoints note), and added a client-side "nothing to change" guard in the tool's execute (with a unit test, and updated the existing API-error test to still exercise the error path). The commit-message finding remains unresolved: the pipeline does not commit, so the squash-merge message is decided at merge time by the maintainer.

Critical

(none)

High

(none)

Medium

extensions/forgejo/README.md:60 — The tools table lists forgejo_milestone_create, forgejo_list_milestones, and forgejo_milestone_view, but not the newly added forgejo_milestone_edit. Every prior milestone-tool PR (issues 20/21/22) updated this table, so the user-facing reference is now out of date. Suggested fix: add a row after line 60, e.g. | forgejo_milestone_edit | Edit a milestone by id (title/description/due_on/state; only provided fields change) |.

Low

extensions/forgejo/docs/DESIGN.md:108 — The milestone tool matrix in the design doc lists create/list/view but not the new edit tool. Suggested fix: add a row after line 108, e.g. | forgejo_milestone_edit | Edit a milestone by id | id (req), title?, description?, due_on?, state? |.
extensions/forgejo (commit 31e28d8) — Commit message is 23: Add forgejo_milestone_edit tool, which deviates from AGENTS.md's documented issue-<N>: <summary> format (e.g. issue-42: add user auth). It does match the forgejo sub-project's local history (22: …, 21: …, 20: …), so this is a minor standard-vs-practice inconsistency. Suggested fix: use issue-23: Add forgejo_milestone_edit tool on the squash-merge (or accept the local convention and update AGENTS.md).
extensions/forgejo/src/index.ts:1361 — Calling forgejo_milestone_edit with only id and no editable fields issues a PATCH with an empty body {} and relies on the server's 422. This matches the sibling edit tools (forgejo_issue_edit, forgejo_pr_edit), so it is consistent, but a client-side guard ("provide at least one field to change") would give agents a clearer message than a server validation error. Suggested fix: in execute, before calling editMilestone, return toolError/a friendly error when title, description, due_on, and state are all undefined.

## 23: Add forgejo_milestone_edit tool Implements [23](https://git.excelera.net/david/pi-extensions-and-skills/issues/23). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 8m 32s. ## Review findings Now I have the full picture. Let me look at the test file context for the milestone_edit describe block, and check the AGENTS.md for commit conventions:All context gathered. Now applying the fixes. First, the README tools table (Medium finding):Now the DESIGN.md matrix (Low finding):Now the client-side guard in `src/index.ts` (Low finding):`ApiErrorCode` isn't imported yet. Let me check the imports at the top of index.ts:Now add a test for the new guard in `tests/tools.test.ts`:Now let me detect and run the build/test gates:The existing API-error test passed only `id` — with the new guard it now (correctly) fails fast. Updating that test to include an editable field so it still exercises the API-error path:All gates pass. Let me verify the working tree diff is clean of unintended artifacts:Let me also verify the root project gates still pass (in case the root has its own config):All gates pass (`npm run check`, `npm run build`, `npm test` — 164 tests, 9 suites). The changes are minimal and consistent. Final annotated findings: # Review Findings — 23 ## Summary Change adds a `forgejo_milestone_edit` tool (register + `editMilestone` PATCH helper + unit tests), matching the pattern of the existing issue/PR edit tools and the milestone create/view tools. Gates run: `npm run build` (tsc) — PASS; `npm run check` (tsc --noEmit) — PASS; `npm test` (jest) — PASS (164 tests, 9 suites); coverage — `milestones.ts` 100% lines/branches and the new tool handler is exercised. Lint is not configured in this repo (no eslint/prettier config or lint script), so no lint gate was available. No correctness bugs or unmet acceptance criteria found (issue has no AC beyond the tool itself). Findings are documentation polish and minor convention items. Remediation applied in this pass: added the `forgejo_milestone_edit` row to the README tools table, added the row to the DESIGN.md tool matrix (plus updated the Issues count and the milestone endpoints note), and added a client-side "nothing to change" guard in the tool's `execute` (with a unit test, and updated the existing API-error test to still exercise the error path). The commit-message finding remains unresolved: the pipeline does not commit, so the squash-merge message is decided at merge time by the maintainer. ## Critical - (none) ## High - (none) ## Medium - [x] extensions/forgejo/README.md:60 — The tools table lists `forgejo_milestone_create`, `forgejo_list_milestones`, and `forgejo_milestone_view`, but not the newly added `forgejo_milestone_edit`. Every prior milestone-tool PR (issues 20/21/22) updated this table, so the user-facing reference is now out of date. Suggested fix: add a row after line 60, e.g. `| forgejo_milestone_edit | Edit a milestone by id (title/description/due_on/state; only provided fields change) |`. ## Low - [x] extensions/forgejo/docs/DESIGN.md:108 — The milestone tool matrix in the design doc lists create/list/view but not the new edit tool. Suggested fix: add a row after line 108, e.g. `| forgejo_milestone_edit | Edit a milestone by id | id (req), title?, description?, due_on?, state? |`. - [ ] extensions/forgejo (commit 31e28d8) — Commit message is `23: Add forgejo_milestone_edit tool`, which deviates from AGENTS.md's documented `issue-<N>: <summary>` format (e.g. `issue-42: add user auth`). It does match the forgejo sub-project's local history (`22: …`, `21: …`, `20: …`), so this is a minor standard-vs-practice inconsistency. Suggested fix: use `issue-23: Add forgejo_milestone_edit tool` on the squash-merge (or accept the local convention and update AGENTS.md). - [x] extensions/forgejo/src/index.ts:1361 — Calling `forgejo_milestone_edit` with only `id` and no editable fields issues a `PATCH` with an empty body `{}` and relies on the server's 422. This matches the sibling edit tools (`forgejo_issue_edit`, `forgejo_pr_edit`), so it is consistent, but a client-side guard ("provide at least one field to change") would give agents a clearer message than a server validation error. Suggested fix: in `execute`, before calling `editMilestone`, return `toolError`/a friendly error when `title`, `description`, `due_on`, and `state` are all undefined.

david added 2 commits

2026-08-18 08:18:17 +00:00