fix: route closed-issue imports through pm close for pm-cli 2026.8.3 - #28
Conversation
Bump peerDependencies to >=2026.8.3 and devDependencies to ^2026.8.3 so the package tracks the published latest. Required for the close_reason enforcement fix in the follow-up commit.
pm-cli 2026.8.3 enforces governance.require_close_reason: pm create --status closed and pm update --status closed are now hard close_reason_required errors (the old auto-route bypass that defaulted a reason is gone). runImport's non-atomic create path created new closed GitHub issues with pm create --status closed, so importing a not-yet-known closed issue failed. Create such items open, then close them through pm close --reason, mirroring the already-correct atomic path and the re-import reconciliation path. Thread the GitHub closed_at completion timestamp through GhIssue -> PreparedGithubImport -> every close site (create path, reconciliation, and the atomic close mutation) as --completed-at / completedAt, so imported items keep their real completion time instead of the import time. The reason stays factual provenance (GitHub issue #N closed); no outcome is fabricated. The handler-failure test fixture createLinkedItem used the same pm create --status closed pattern to build a closed linked item; switch it to create-then-close so the fixture is still a genuinely closed item under the enforced policy, without changing what the tests assert. Refs: pm-github-rwq9
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideRoutes imports of closed GitHub issues through Sequence diagram for importing a closed GitHub issue via pm closesequenceDiagram
actor Github
participant IndexTs as index_ts
participant PmCli as pm_cli
Github->>IndexTs: runImport(issue)
IndexTs->>IndexTs: prepareGithubImport(issue)
Note over IndexTs: prepared.closedAt = issue.closed_at
IndexTs->>PmCli: pmRun(["create", "--status", "open", "--json", ...])
PmCli-->>IndexTs: created stdout
IndexTs->>IndexTs: parseCreatedItemId(stdout)
alt status was closed
IndexTs->>PmCli: pmRun(["close", createdId, "--reason", ..., "--completed-at", closedAt])
PmCli-->>IndexTs: close result
end
opt syncAnnotations
IndexTs->>IndexTs: syncGithubCommentsToAnnotations(createdId, comments, pmRoot, issue.number)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughGitHub issue imports now preserve ChangesGitHub close import
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Importer
participant pm-cli
GitHub->>Importer: Return issue with closed_at
Importer->>pm-cli: Create issue as open with JSON output
pm-cli-->>Importer: Return created item ID
Importer->>pm-cli: Close item with --completed-at
pm-cli-->>Importer: Return close result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.ts`:
- Around line 2412-2425: Update the parseCreatedItemId failure branch within the
mustClose handling to increment skipped and continue immediately after logging
the missing created item ID. Keep the existing successful close and
close-command failure paths unchanged so an item is counted as imported only
when the required close operation completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ceb0261-0751-4091-8652-6d063f309861
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.agents/pm/history/pm-github-rwq9.jsonl.agents/pm/issues/pm-github-rwq9.toon.gitattributesindex.tspackage.jsontest/handler-failures.test.ts
Greptile SummaryThis PR fixes closed-issue imports under
Confidence Score: 5/5Safe to merge — all three previously broken paths (issue-import create, project-import create, project-import update) are correctly routed through The two issues flagged in the previous review round are both resolved: Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| index.ts | Core fix: parseCreatedItemId reads flat {id} envelope (not {item:{id}}); runImport and runProjectImport create-then-close paths handle all error branches correctly with proper skipped/imported/updated accounting; closedAt threading via githubCloseArgs is clean and applied consistently across reconciliation, create-then-close, and atomic mutation paths. |
| test/comments-sync.test.ts | Unit test updated to pin the real flat envelope shape; adds a contract test that runs actual pm create --json output through parseCreatedItemId and cross-checks the parsed id against the workspace, preventing silent shape drift. |
| test/handler-failures.test.ts | Adds three new end-to-end tests: closed project-item create path, closed board-status update path, and the never-before-covered runImport create-then-close path that catches the always-broken parseCreatedItemId regression; createLinkedItem fixture updated to create-then-close under the enforced policy. |
| package.json | Bumps @unbrained/pm-cli dev dependency and peer requirement to ^2026.8.3 / >=2026.8.3 to match the close-reason enforcement this PR is built against. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GitHub issue / project item fetched] --> B{status === closed?}
B -- No --> C[pm create --status open/in_progress/etc.]
B -- Yes --> D[pm create --status open + --json]
D --> E{parseCreatedItemId\nparsed?.id flat envelope}
E -- id found --> F[pm close --reason\nGitHub provenance\n+ --completed-at closed_at]
E -- id missing --> G[skipped++ continue]
F --> H{close.ok?}
H -- Yes --> I[syncAnnotations if needed\nimported++]
H -- No --> J[skipped++ continue]
C --> K[imported++]
Reviews (8): Last reviewed commit: "Close the create-then-close path the imp..." | Re-trigger Greptile
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new create-then-close flow duplicates close argument construction in several places; consider extracting a small helper to build
closeArgs(including optional--completed-at) to keep behavior consistent and reduce repetition. - In
runImport, the branch wheremustCloseis true butcreatedIdis undefined only logs and leaves the item open; if this represents a hard failure for closed imports, you may want to incrementskippedor return an explicit error to make the behavior clearer. - The GhIssue/PreparedGithubImport
closed_at/closedAthandling is now spread across multiple sites; adding a single normalization point (e.g., a small helper to map GitHub issue objects toPreparedGithubImport) could reduce the risk of future divergence in how completion timestamps are propagated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new create-then-close flow duplicates close argument construction in several places; consider extracting a small helper to build `closeArgs` (including optional `--completed-at`) to keep behavior consistent and reduce repetition.
- In `runImport`, the branch where `mustClose` is true but `createdId` is undefined only logs and leaves the item open; if this represents a hard failure for closed imports, you may want to increment `skipped` or return an explicit error to make the behavior clearer.
- The GhIssue/PreparedGithubImport `closed_at`/`closedAt` handling is now spread across multiple sites; adding a single normalization point (e.g., a small helper to map GitHub issue objects to `PreparedGithubImport`) could reduce the risk of future divergence in how completion timestamps are propagated.
## Individual Comments
### Comment 1
<location path="index.ts" line_range="2413-2422" />
<code_context>
+ skipped++;
+ continue;
+ }
+ } else {
+ console.error(`#${issue.number}: could not parse created item id — left open`);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider treating a failed `createdId` parse as a hard failure when `mustClose` is true.
In the `mustClose` branch, a `parseCreatedItemId` failure currently leaves the item open and the import continues without incrementing `skipped`, so callers won’t see that we failed to mirror the upstream closed status. Align this with how `close` failures are handled (e.g., increment `skipped` and `continue`, or use a safer fallback) to avoid silently ending up with open items that should be closed.
```suggestion
if (mustClose) {
if (createdId) {
const closeArgs = ["--path", pmRoot, "close", createdId, "--reason", `GitHub issue #${issue.number} closed`];
if (closedAt) closeArgs.push("--completed-at", closedAt);
const close = pmRun(closeArgs);
if (!close.ok) {
console.error(`#${issue.number}: close after import failed — ${close.stderr}`);
skipped++;
continue;
}
} else {
console.error(`#${issue.number}: could not parse created item id — treating as failed close`);
skipped++;
continue;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Pushed a follow-up commit addressing all three review findings, plus a root cause found while verifying them. What changed
New tests — the point is that none of them can pass against the old parser:
Gates: typecheck clean, 241/241 tests pass, Please re-review the new commit. @greptileai |
|
✏️ Learnings added
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 34 minutes. |
The initial sweep fixed runImport (issue import) and the createLinkedItem
test fixture, but missed runProjectImport (the github project import
handler). That handler still issued pm create --status closed for project
board items wrapping closed issues, and pm update --status closed for
re-imports whose board Status option maps to closed. Both are hard
close_reason_required errors under pm-cli 2026.8.3 — a user-facing bug:
importing a GitHub project with closed items would fail.
Fix the create path to create open then close via pm close --reason with
factual provenance (the wrapped issue repo#number, or the project ref for
draft issues). Fix the update path to omit --status when the mapped status
is closed, then close via pm close --reason after the update.
Also fix a latent defect in parseCreatedItemId: it looked for
parsed.item.id but pm create --json emits { id: ... } at the top level, so
the create-then-close path in runImport could not read the new id and
silently left the item open instead of closing it. Now accepts both
parsed.id and parsed.item.id.
Added two handler-level tests: a closed upstream issue on the create path,
and a closed board-status mapping on the update path. Updated the
parseCreatedItemId unit test for the real emit shape.
Refs: pm-github-rwq9
Re-applies the three review findings from round 1, which were lost when this
branch was force-updated, and drops the `item` fallback that was added instead.
parseCreatedItemId read `parsed.item.id`, but `pm create --json` emits a flat
receipt — {id, status, changed_field_count} — with no `item` wrapper (mutations
return a flat receipt; only queries such as `pm read`/`pm list` wrap). The id
was therefore always undefined, so the close after create never ran: every
newly-imported closed issue landed OPEN, and every --comments-mode=annotations
sync was silently skipped, while the import still reported success. Nothing
failed loudly because the parser returns undefined rather than throwing.
No `item` fallback is kept. A fallback for a shape the host has never emitted
is dead code no real CLI can exercise, and a test asserting it restores the
exact failure mode that hid the bug: a parser and a test agreeing with each
other rather than with the CLI.
- parseCreatedItemId reads the flat `id`.
- The unit test pins the real shape and asserts the wrapper shape yields
undefined, so the old behaviour cannot return green.
- A contract test parses output from a REAL `pm create --json` run and asserts
there is no `item` wrapper, so host drift fails at the parser rather than
silently downstream.
- A new end-to-end test imports an unlinked, already-closed issue and asserts
the final status is closed. Verified to fail against the old parser.
Also re-applied from review:
- The mustClose branch with no parsed id logged "left open" and then fell
through to imported++, reporting a closed issue as a successful import. It
now counts skipped and continues, matching the adjacent close-failure branch
and the project-import path, which already handled this correctly.
- The two close sites built identical argv; extracted githubCloseArgs(id) so
the reason and --completed-at evidence cannot drift apart.
typecheck clean, 243/243 tests pass.
Summary
pm-cli
2026.8.3enforcesgovernance.require_close_reason:pm create --status closedandpm update --status closedare now hardclose_reason_requirederrors. The old2026.7.29bypass that silently auto-routed these and defaulted a close reason is gone. This package relied on that bypass in one path.Root cause
runImport(non-atomic create path) created new closed GitHub issues withpm create --status closed, so importing a not-yet-known closed issue failed under the enforced policy. The atomic path and the re-import reconciliation path were already correct (they route closure throughpm close --reason); only the plain create path used the now-broken invocation.Changes
index.ts— the create path now creates a closed upstream issue asopen, then closes it viapm close --reason "GitHub issue #N closed", mirroring the atomic and reconciliation paths.closed_atthroughGhIssue→PreparedGithubImport→ every close site (create path, reconciliation, and the atomicclosemutation) as--completed-at/completedAt, so imported items keep their real completion time instead of the import time. The reason stays factual provenance; no outcome is fabricated.test/handler-failures.test.ts— thecreateLinkedItemfixture used the samepm create --status closedpattern to build a closed linked item; switched to create-then-close so the fixture is still a genuinely closed item under the enforced policy, without changing what the tests assert.@unbrained/pm-cliat^2026.8.3(peer>=2026.8.3).pm item
Gates
npm run typechecknpm run buildnpm testnpm run coverageindex.ts88.83% lines / 80.15% branches / 89.94% functions (thresholds 88/79/89)npm run changelog:checkNo governance policy, lint rule, or coverage threshold was weakened.
Summary by Sourcery
Route imports of closed GitHub issues through explicit pm close operations and preserve source completion timestamps under the updated pm-cli governance policy.
Bug Fixes:
Enhancements:
Build:
Tests:
Chores:
Summary by cubic
Fixes imports of closed GitHub issues and project items under
@unbrained/pm-cli2026.8.3 by creating items open, then closing viapm close --reasonwith--completed-at. Also fixes id parsing so create‑then‑close reliably closes the new item.Bug Fixes
pm close --reason; pass GitHubclosed_atas--completed-at.--status closedon create/update; close after with a provenance reason.parseCreatedItemIdnow reads the flat{ id }frompm create --json(drops the unused{ item.id }shape).pm create --jsonenvelope and an end‑to‑end closed‑issue import that asserts the item lands closed.Dependencies
@unbrained/pm-clito^2026.8.3(peer>=2026.8.3).Written for commit e1b8db4. Summary will update on new commits.