Repository navigation
fix: atomic iteration-field create, PR-item iteration assignment, set_issue_milestone, CI - #27
Conversation
Both lockfiles were committed: pnpm-lock.yaml went stale at v1.4.0 while the 1.5.x device-flow work came in through npm. Nothing arbitrated between them — no CI, and the README said npm while the original setup was pnpm. - drop package-lock.json, refresh pnpm-lock.yaml against current ranges - pin packageManager: pnpm@10.33.0 so a stray npm/yarn install is rejected - allow esbuild's postinstall (pnpm 10 blocks build scripts by default; vitest needs the platform binary) - point the README development steps at pnpm, add pnpm test Consumer-facing install instructions stay on npx/npm -g — those describe the published package, not this working tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #20, closes #25. getProjectItemId resolved the number through `repository.issue(number:)`, so every pull request on a board failed with "Could not resolve to an Issue with the number of N" — the item existed, the lookup just refused to see it. - query the `issueOrPullRequest` union with fragments for both content types - accept `itemId` and `projectId` directly, skipping the lookups, matching the escape hatch `update_item_status` has always had - name the content type in the not-on-board error, and distinguish it from a number that matches nothing at all - validate that the input identifies an item and a project before any request Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #23. create_issue could attach a milestone at creation time and create_milestone could make one, but nothing could change a milestone afterwards — the only way out was `gh issue edit --milestone`. Board fields have always been mutable via update_item_status and assign_issue_to_iteration; milestones were the odd one out. - set_issue_milestone(owner, repo, issueNumber, milestoneNumber), with an explicit null to clear - pull requests carry milestones too, so the number resolves through the issueOrPullRequest union and dispatches to updateIssue or updatePullRequest - reject an omitted milestoneNumber rather than reading it as "clear" - getMilestoneId moves to src/tools/milestones.ts with an injected client, so it is testable and shared with create_issue; it now reports a missing milestone instead of throwing on undefined Scoped narrowly rather than the general update_issue the ticket floats as an alternative — relabel/reassign can follow separately if wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
New tool (set_issue_milestone) plus the PR-item fix in assign_issue_to_iteration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #21, closes #22. The payload bug in these tickets was fixed in #26, but the side effect they both report was not: create_iteration_field created the field, then failed configuring it, leaving an empty field behind and a retry that dead-ended on "Name has already been taken". CreateProjectV2FieldInput accepts iterationConfiguration — confirmed by live introspection and by a real call — so the field and its iterations are now created in one mutation. Either both, or neither. For projects still holding a field stranded by the old two-step path, a duplicate-name failure falls back to adopting that field. The fallback refuses a field that already holds iterations (active or completed): reconfiguring one regenerates every iteration ID and would detach all assignments, which is the exact damage #24 was about. Adopted fields come back flagged `adopted: true`. Verified live against a throwaway ProjectsV2 (created and deleted): create-time configuration mints real iteration IDs, adoption configures the stranded field, and the populated-field guard refuses without touching the existing iterations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- create_iteration_field: single mutation, the adopted-field fallback, and why a populated field is refused - add_iteration / update_iteration: assignments are snapshotted and restored by title, with assignmentsRestored in the result Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
joaodotwork
left a comment
There was a problem hiding this comment.
Reviewed the diff, ran the suite, and checked the load-bearing claims against the live API.
Verified
CreateProjectV2FieldInput.iterationConfigurationexists — confirmed by introspection, so the atomic-create premise holds.pnpm test— 64/64 pass.pnpm run build— clean.- #24 is already closed, so with
closes: 20, 21, 22, 23, 25here, all six iteration/milestone tickets land. - The tests assert real schema shape (
declares the real ProjectV2Iteration input type,queries startDay, never startDate) — the fabricated-mock problem from the pre-#26 suite didn't recur.
The code is good. Four inline notes, one of which I'd fix before merge (the prepare script). The rest are judgment calls, not defects.
| "build": "tsc", | ||
| "dev": "tsc --watch", | ||
| "prepare": "npm run build", | ||
| "prepare": "pnpm run build", |
There was a problem hiding this comment.
Worth fixing before merge.
prepare runs on npm install <git-url> and at publish time, so anyone installing from git without pnpm on PATH now fails with pnpm: command not found. Deleting package-lock.json compounds it — npm ci is gone too.
Consumer impact is bounded (the README points at npx, and the published tarball ships dist, so prepare doesn't run there), but it's a free fix:
"prepare": "tsc"Package-manager agnostic, works identically under both. Worth doing because there's no CI to catch it — the repo has no .github/workflows/.
There was a problem hiding this comment.
Fixed in dd6b2e9 — took your suggestion exactly: "prepare": "tsc".
You're right that it was a real break, and I'd underweighted it: I reasoned about prepare as publish-time only and missed npm install <git-url>. Coupling the build to pnpm being on PATH bought nothing — the packageManager pin is what enforces the policy, and it does that without touching the build path.
Verified it still runs under pnpm:
> @joaodotwork/plantas-github-projects-mcp@1.6.0 prepare
> tsc
Done in 1.2s using pnpm v10.33.0
| }, | ||
| "author": "João Doria de Souza", | ||
| "license": "MIT", | ||
| "packageManager": "pnpm@10.33.0", |
There was a problem hiding this comment.
Scope creep — flagging, not blocking.
The pnpm standardization is unrelated to all six tickets this PR closes, and it's 3,982 of the diff's 5,168 lines.
The justification is sound and it needed doing — two lockfiles committed, pnpm-lock.yaml stale since v1.4.0, package-lock.json advanced by the 1.5.x commits, nothing arbitrating. But it's a repo-wide policy change riding in a bugfix PR, and it changes the contributor workflow.
Either split it into its own PR or call it out in the release notes for 1.6.0.
There was a problem hiding this comment.
Fair flag, and I took the release-notes option rather than the split.
Why not split: the pnpm commit is first on the branch, so extracting it means a second PR plus a rebase of this one, and every commit after it was authored and tested against the single-lockfile tree. The churn buys tidier history at the cost of re-verifying five commits. Given both PRs would be merged by you within minutes of each other, that trade didn't look worth it — but say the word and I'll split it.
What I did instead: the repo publishes via GitHub Releases and has no CHANGELOG, so a changelog file would have been a new convention invented in this PR. There's now a "Release notes draft — v1.6.0" section at the bottom of the PR description, ready to paste at publish time, and it leads with the contributor-facing change rather than burying it under the features:
⚠️ For contributors: this repo now uses pnpm
package-lock.jsonhas been removed andpackageManageris pinned topnpm@10.33.0... Annpm installin a clone will now be rejected by the pin rather than silently producing a second dependency graph.Nothing changes for users of the server:
npx -yandnpm install -gwork exactly as before, and the published tarball ships prebuiltdist.
On the line count: 3,982 of those lines are pnpm-lock.yaml and the deleted package-lock.json. Excluding both lockfiles the PR is roughly 1,190 lines, most of it tests and README. Not a rebuttal to the point — a policy change is a policy change regardless of size — but the diff stat overstates the surface a reviewer has to read.
| "fieldId", | ||
| "iterationId", | ||
| ], | ||
| required: ["fieldId", "iterationId"], |
There was a problem hiding this comment.
required drops from six fields to two. The runtime validation in assignIssueToIteration catches bad input with clear messages, and the ID passthrough is a genuine win that mirrors update_item_status.
The tradeoff: the MCP schema is what a calling model reads first, and it now advertises owner/repo/issueNumber/projectNumber as all optional. The real contract lives in the per-property descriptions, which relies on the model reading them rather than just the required array.
Not a defect and I don't think it blocks — just the one place the PR trades schema-level guidance for flexibility.
There was a problem hiding this comment.
Agreed on the tradeoff, and I'm leaving it as-is. Reasoning, so the choice is on the record:
The alternative is expressing the real contract structurally with anyOf — either {itemId, projectId} or {owner, repo, issueNumber, projectNumber}. That's the accurate schema, but MCP client support for anyOf in tool input schemas is uneven: some clients hand the schema to the model untouched, others validate against it first, and a few flatten or drop combinator keywords. A schema that's correct on paper but rejected or mangled by a client is worse than a permissive one, especially with no CI here to catch a client-specific regression.
So the contract lives in three places instead: the tool description leads with both forms, each property description says what it's required unless, and assignIssueToIteration throws before any network call with a message naming exactly what's missing —
Cannot identify the item: pass either itemId, or owner + repo + issueNumber + projectNumber.
Cannot identify the project: pass either projectId, or owner + projectNumber.
Both paths are covered by tests that assert the client is never even called. That's the mitigation for the case you're pointing at — a model that reads only the required array gets a fast, specific error rather than a confusing GraphQL failure.
Worth revisiting if anyOf handling across clients settles, or if the repo ever grows a schema-conformance test.
| query($projectId: ID!) { | ||
| node(id: $projectId) { | ||
| ... on ProjectV2 { | ||
| fields(first: 100) { |
There was a problem hiding this comment.
Minor: no pagination here. A project with more than 100 fields would miss the stranded field and rethrow the original duplicate-name error.
Degraded rather than wrong, and this fallback only serves boards stranded by pre-1.6.0 versions — since the create is atomic now, no new ones can appear. Fine to leave.
There was a problem hiding this comment.
Checked this rather than leaving it at "fine to leave" — it turns out to be provably unnecessary.
GitHub caps a project at 50 fields in total, and system fields count toward that cap (docs). So fields(first: 100) cannot truncate: a project that could hide the stranded field past the first page can't exist.
That also clears the same pattern at the four other call sites — src/tools/status.ts:35, src/tools/iterations.ts:344, src/index.ts:1116 and :1154 all use fields(first: 100) unpaginated.
Recorded it in the JSDoc in dd6b2e9 so it doesn't get re-litigated:
* Unpaginated on purpose: a project can hold at most 50 fields in total (system fields count
* toward that), so `first: 100` cannot truncate.One correction to my own reasoning while I was here: I'd started to add pagination on the grounds that it would match the rest of the codebase. It wouldn't — snapshotIterationAssignments paginates items, not fields, and every fields query in the repo is unpaginated. Leaving it is the consistent choice, not the lazy one.
Review catch on #27: `prepare` runs on `npm install <git-url>` and at publish time, so "pnpm run build" broke installing from git without pnpm on PATH — and deleting package-lock.json took `npm ci` away as a fallback. There is no CI to catch it either. `tsc` works identically under both package managers and needs neither. Also record why findIterationFieldByName does not paginate: a project holds at most 50 fields including system fields, so first: 100 cannot truncate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #28. The repo had no .github/workflows/, so every regression gate was a human remembering to run pnpm test. That gap did real damage twice in this release: two committed lockfiles drifted for four minor versions with nothing arbitrating, and a `prepare: "pnpm run build"` break was caught by review rather than tooling. - install / build / test on push to main and on every PR, Node 20 and 22 - pnpm install --frozen-lockfile, so a lockfile disagreeing with package.json is a hard failure — the exact drift that went unnoticed through 1.5.x - a separate job that fails if package-lock.json or yarn.lock reappears - concurrency group so superseded PR pushes cancel their own runs Publishing automation is deliberately out of scope; releases stay manual. Validated with @action-validator/cli before pushing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Your review's closing observation — no
It has already run on this PR, which is the end-to-end check: Both of the things your review caught are now caught by tooling instead:
Two follow-ups that are settings rather than code, so they're yours to make:
Publishing automation stayed out of scope deliberately; releases are cut manually and that's a separate call. |
Closes #20, closes #21, closes #22, closes #23, closes #25, closes #28.
create_iteration_fieldis now atomic (#21, #22)#26 fixed the input type these tickets reported, but not the side effect both of them also describe: the tool created the field, then failed configuring it, leaving an empty field behind and a retry that dead-ended on
Name has already been taken.CreateProjectV2FieldInputacceptsiterationConfiguration— confirmed by live introspection and by a real call — so the field and its iterations are created in one mutation. Either both, or neither.For projects still holding a field stranded by the old two-step path, a duplicate-name failure falls back to adopting that field and configuring it (
adopted: truein the result). The fallback refuses a field that already holds iterations, active or completed: reconfiguring one regenerates every iteration ID and would detach all assignments — precisely the damage #24 was about.assign_issue_to_iterationon pull requests (#20, #25)The item lookup resolved the number through
repository.issue(number:), so any pull request on a board failed withCould not resolve to an Issue with the number of N— the item was there, the query just refused to see it. Both tickets are the same bug from different boards.issueOrPullRequestunion with fragments for both content typesitemIdandprojectIdcan be passed directly to skip the lookups, the escape hatchupdate_item_statushas always had.fieldId+iterationIdare the only hard requirements nowset_issue_milestone(#23)create_issuecould attach a milestone at creation time andcreate_milestonecould make one, but nothing could change a milestone afterwards — callers fell back togh issue edit --milestone. Board fields have been mutable all along; milestones were the odd one out.set_issue_milestone(owner, repo, issueNumber, milestoneNumber), with an explicitnullto clearupdateIssueorupdatePullRequestbased on the resolved typemilestoneNumberis rejected rather than silently read as "clear"getMilestoneIdmoved tosrc/tools/milestones.tswith an injected client — testable, shared withcreate_issue, and it now reports a missing milestone instead of throwing on undefinedScoped to the narrow tool rather than the general
update_issue#23 floats as an alternative; relabel/reassign can follow separately.Package manager
The repo had both lockfiles committed:
pnpm-lock.yamlstale since v1.4.0,package-lock.jsonadvanced by the 1.5.x device-flow commits, and no CI to arbitrate. Standardized on pnpm — droppedpackage-lock.json, refreshedpnpm-lock.yaml, pinnedpackageManager: pnpm@10.33.0, allowlisted esbuild's postinstall (pnpm 10 blocks build scripts; vitest needs the binary), and pointed the README dev steps at pnpm.prepareistscrather thanpnpm run build, so installing from a git URL doesn't require pnpm on PATH. Consumer-facingnpx/npm -ginstructions are unchanged.This is a repo-wide policy change riding in a bugfix PR — see the release-notes draft at the bottom, which calls out the contributor-workflow change.
CI (#28)
There was no
.github/workflows/, which is why both problems above went unnoticed until a human looked: the lockfile drift ran for four minor versions, and thepreparebreak was caught in review rather than by tooling. Filed as #28 and fixed here..github/workflows/ci.ymlruns on push tomainand on every PR:pnpm install --frozen-lockfile, so a lockfile disagreeing with package.json is a hard failure — precisely the drift that went unnoticedpackage-lock.jsonoryarn.lockreappears, so the two-lockfile state can't come backPublishing automation is deliberately out of scope; releases stay manual. Validated with
@action-validator/clibefore pushing, and this PR is its first real run.Testing
pnpm test— 64 passing, up from 44. New coverage: atomic create, adoption of a stranded field, the two refusals that protect a populated field, PR-item resolution, both-fragment query shape, item-ID/project-ID passthrough, input validation, and the fullset_issue_milestonesurface.Unit tests mock GraphQL, so they pin query shape. The create-time behaviour was additionally verified live against a throwaway personal ProjectsV2 (created and deleted in the same run), exercising the compiled tool:
Live introspection also confirmed the schema facts behind #21/#22/#24:
Release notes draft — v1.6.0
For the GitHub Release body. The repo uses Releases rather than a CHANGELOG, so this lives here to be pasted at publish time.
package-lock.jsonhas been removed andpackageManageris pinned topnpm@10.33.0. Usepnpm install,pnpm test,pnpm run build. Annpm installin a clone will now be rejected by the pin rather than silently producing a second dependency graph.Nothing changes for users of the server:
npx -y @joaodotwork/plantas-github-projects-mcpandnpm install -gwork exactly as before, and the published tarball ships prebuiltdist.Background: the repo had both lockfiles committed, with
pnpm-lock.yamlstale since v1.4.0 andpackage-lock.jsonadvanced by the 1.5.x releases — no CI arbitrated between them, so localnode_moduleshad drifted. Now there's one lockfile and one tool.New
set_issue_milestone— set, change, or clear the milestone on an existing issue or pull request. Previously onlycreate_issuecould attach one, at creation time (No tool to set a milestone on an existing issue #23).Fixed
assign_issue_to_iterationworks on pull requests. It resolved numbers as issues only, so PR items on a board failed withCould not resolve to an Issue with the number of N(assign_issue_to_iteration fails on pull request items #20, assign_issue_to_iteration fails on pull requests (resolves number as Issue only) #25). It also now acceptsitemId/projectIddirectly, likeupdate_item_status.create_iteration_fieldis atomic. It used to create the field, then fail configuring it, leaving an empty field and a retry that hitName has already been taken(create_iteration_field and add_iteration fail with GraphQL input-type errors #21, add_iteration & create_iteration_field send outdated payload (GitHub schema mismatch) #22). A field of the same name that is empty — stranded by an older version — is now adopted and configured instead; one that already has iterations is refused, since reconfiguring would detach every assignment.update_iterationandadd_iterationpreserve item assignments across the configuration replace that GitHub's API forces, and reportassignmentsRestored: { restored, failed }(update_iteration is broken: queries non-existent 'startDate' field; no warning that config replace wipes item assignments #24, fixed in fix(iterations): correct GraphQL schema misuse and preserve item assignments #26).Internal
🤖 Generated with Claude Code