Release v1.8.0 — Dogfood Fixes & Bulk Operations #40
Mike Bros
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "release/1.8.0"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Release v1.8.0 — Dogfood Fixes & Bulk Operations
Changes
Highlights
New tools:
bulk_update_work_packages,bulk_transition_work_packages— batch operations that process items independently with 409 retry and partial failure handling.Bug fixes: Dynamic MCP server version, project_id string-to-number coercion, activity caching and project-scoped 404 fallback in log_time.
DX improvements: CLAUDE.md agent guide, CHANGELOG.md, enriched tool descriptions, "Advanced:" prefix on low-usage tools, TTL-based cache expiry (5min default, configurable via CACHE_TTL_MS).
Tests: 253 total (11 new — 9 e2e bulk tool tests with mock API, 2 coercion tests).
Checklist
References
Version: 1.8.0 (Gravity PM)
🤖 PR Review: Release v1.8.0 — Dogfood Fixes & Bulk Operations
Verdict: Acceptable
No blocking findings. The PR is well-engineered with comprehensive error handling and consistent patterns across all changes. Tests pass (253/253). Two important items and several minor items noted below.
Findings Summary
🟡 Important
1. Lint violation:
deleteoperator in log_time 404 fallback (index.js:2329)Biome flags
delete body._links.activityas a performance lint error. The suggested fix isbody._links.activity = undefined. While functionally equivalent for JSON serialization (both omit the key), this will fail CI if lint is enforced in a pipeline.2. Cache TTL feature has zero test coverage
The new
getCached()TTL logic (expiry, pending-flag concurrency prevention, stale-on-error fallback) is untested. This is a meaningful behavioral change — the cache previously never expired, and now it does with three distinct code paths (fresh, stale+pending, stale+refresh). Key scenarios to test:CACHE_TTL_MSenv var is respected🔵 Minor
3. 409 conflict retry logic is untested in bulk operations
Both
bulk_update_work_packagesandbulk_transition_work_packagesimplement single-retry on 409 conflict (fetch fresh lockVersion, re-PATCH). The mock API never returns 409, so this entire code path is untested. Consider adding a mock that returns 409 on first PATCH attempt for a specific WP ID.4. README tool count discrepancy
README line 2 claims "64 tools", but the deployment modes table (line 76) says "API tools (63)". Likely the difference is
admin_command(Docker-only), but this should be clarified with a footnote or consistent count.5. Bulk operation tests use broad assertions
Test assertions like
assert.match(text, /Updated 2\/2/)verify counts but not which specific WP IDs succeeded or failed. More precise assertions would catch regressions in output formatting.6. No test for
zIdArraystring preprocessing inbulk_transitionwork_package_idsaccepts CSV strings ("1,2,3") and JSON string arrays ("[1,2,3]") via preprocessing, but no test exercises these input formats.⚪ Nitpick
7.
cleanStringParaminteger coercion could surprise callerscleanStringParam("8")now returns8(number) instead of"8"(string). This is intentional for project_id union params, but the function name doesn't hint at numeric coercion. A brief JSDoc comment on the function would help future readers.8. Cache TTL design comment
The stale-on-error strategy in
getCached()favors availability over freshness — correct for reference data like statuses/priorities, but worth a one-line comment in the code explaining this design choice.Smoke Test Results
npm testnpm run lintdeleteoperator in index.js:2329)HUMAN REVIEW: Bulk operation batch size limits
The bulk tools accept unbounded arrays. For very large batches (100+ items), sequential processing could time out MCP tool calls. Consider whether a max batch size or warning threshold is warranted. This is a product decision outside the bot's scope.
What Looks Good
log_timehandles the real OpenProject quirk of project-scoped activities gracefully (global → project form → error, with 404 retry)Review generated by Gravity Bot
@ai wrote in #40 (comment):
I do think a max batch size is justified. I think that 100-250 would be plenty high while not being too high that a lot of heavy requests would break the http calls.
PR Review Followup
Responding to review posted on 2026-03-16 (review comment).
Review verdict was Acceptable — no blocking findings.
Triage Summary
Fixes Applied
The following findings have been resolved on this branch:
deleteoperator in log_time 404 fallback — fixed in1b554941b55494Deferred to v1.9.0
The following items have been tracked for the next version:
Human Review Items (Unchanged)
These items require human judgment and were not auto-triaged:
Test Results
This followup was generated by the gr-pr-followup skill.
PR Review Followup (Updated)
Responding to review posted on 2026-03-16 (review comment).
All 8 findings have been resolved on this branch. No items deferred.
Triage Summary
1b55494ea5e4fbea5e4fbFixes Applied
deleteoperator in log_time 404 fallback — fixed in1b554941b55494ea5e4fbea5e4fbea5e4fbea5e4fbea5e4fbea5e4fbAdditional Fix
Human Review Items (Unchanged)
Test Results
This PR should now be ready for re-review.
This followup was generated by the gr-pr-followup skill.
PR Review (Updated)
Verdict: Acceptable
This review was conducted from a fresh context against the current branch state (17 commits, 7 files). Focuses on findings not previously identified in the prior 4 comments.
Findings Summary
Minor
1.
MCP_FOOTERduplication risk in bulk_update_work_packages (index.js:1712)bulk_update_work_packagesappendsMCP_FOOTERunconditionally to descriptions. Sequential MCP updates to the same work package will accumulate duplicate footers. This is a pre-existing pattern shared withcreate_work_packageandupdate_work_package, but bulk operations amplify the risk since batch workflows are more likely to re-update WPs.Suggested fix: Strip existing footer before appending, or check whether the description already ends with it:
Nitpick
2. CSV preprocessing produces opaque errors for non-numeric values (
index.js:1796)In
bulk_transition_work_packages, the CSV string preprocessorval.split(",").map(s => Number(s.trim()))converts non-numeric values toNaN. Zod's.int()validator catches this, but the resulting error message won't indicate which element was invalid. For a bulk tool accepting user input, a more descriptive parse error would improve DX.3. Pre-existing lint warnings (not introduced by this PR)
Biome reports 2
noUselessEscapeInRegexerrors (lines 4814, 5198 —\Zin regex) and 2noTemplateCurlyInStringwarnings (lines 5036, 5040 — intentional YAML template variables in the setup wizard). None are in code changed by this PR. Noting for awareness; these could be cleaned up in a future pass.Test Results
Previously Identified
8 findings from the prior review cycle have been resolved on this branch. These are not repeated as new findings.
Fixed in this branch:
deleteoperator replaced withundefinedassignmentcleanStringParamHuman review item resolved:
dddf39b(responding to reviewer feedback)What Looks Good
MAX_BULK_BATCH_SIZEconstant with Zod.max()enforcement and descriptive error messageupdate_work_packageUpdated review — previous reviews posted on 2026-03-16. This is review cycle 2.
Review generated by Gravity Bot
PR Review Followup
Responding to review posted on 2026-03-16 (review comment).
All 3 findings have been resolved on this branch. No items deferred.
Triage Summary
Fixes Applied
MCP_FOOTERduplication risk inbulk_update_work_packages— fixed inc7cb0cbc7cb0cb\Z) in setup wizard — fixed inc7cb0cbTest Results
This PR should now be ready for merge.
This followup was generated by the gr-pr-followup skill.