Repository navigation
Route terminal CSV row imports through pm close for pm-cli 2026.8.3 - #57
Merged
Merged
Conversation
pm-cli 2026.8.3 enforces governance.require_close_reason on every programmatic transition into `closed`: `pm create --status closed` and `pm update --status closed` are now hard close_reason_required errors, and the 2026.7.29 bypass that invented the reason "Closed via pm update" is gone. Only `pm close --reason` may close an item. A CSV row whose status is `closed` is therefore created `open` and immediately transitioned through `pm close --reason`. The CSV source carries no close-reason or resolution column, so the reason states the import provenance factually — "Imported from <source> (source status: closed)" — rather than inventing an outcome such as "completed" that the source never recorded. `canceled` is not gated by that policy and is still applied directly at create time, unchanged. The transaction-marker semantics are unaffected: presence of the csv-tx row marker, not item status, remains the applied signal, so a legitimately closed imported row is still recognised on resume and never re-imported, while a compensated create still has its markers stripped before closing. Pin moved to ^2026.8.3 with the lockfile refreshed. typecheck, build, tests and release:check (coverage, audit, pack, changelog) all pass against 2026.8.3.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRoutes terminal-status CSV imports through Sequence diagram for CSV import of terminal closed status via pm close --reasonsequenceDiagram
actor CsvSource
participant CsvImporter
participant upsertCreate
participant pm_cli
CsvSource->>CsvImporter: CSV row (status = closed)
CsvImporter->>upsertCreate: upsertCreate(pmRoot, p(status=closed), source, extraTags)
upsertCreate->>pm_cli: pm --path pmRoot create --title p.title --status open --json
pm_cli-->>upsertCreate: { id }
upsertCreate->>pm_cli: pm --path pmRoot close id --reason importCloseReason("closed", source)
pm_cli-->>upsertCreate: closed item
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Summary by CodeRabbit
WalkthroughCSV imports now create closed items as open and transition them with ChangesCSV import status handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CSVImport
participant upsertCreate
participant PMCLI
CSVImport->>upsertCreate: import row with closed status
upsertCreate->>PMCLI: pm create with open status
upsertCreate->>PMCLI: pm close --reason with provenance
PMCLI-->>CSVImport: return imported item
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 |
Greptile SummaryThe PR adapts terminal CSV imports to pm-cli 2026.8.3 and completes the previously requested safeguards around failed create-then-close operations.
Confidence Score: 5/5The PR appears safe to merge. The previously reported open-orphan and unknown-status compensation failures are addressed at the current head, and no blocking failure remains.
|
| Filename | Overview |
|---|---|
| index.ts | Routes closed-row creation through a reasoned close and completes the prior compensation fix by hard-failing missing IDs and positively verifying rollback closure. |
| test/atomic.test.ts | Adds focused regression coverage for successful compensation, missing create IDs, failed compensation, and failed status verification. |
| package.json | Raises the pm-cli peer and development dependency requirements to versions that enforce close reasons. |
| package-lock.json | Refreshes the lockfile for the pm-cli 2026.8.3 dependency update. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CSV row with closed status] --> B[pm create with open status]
B --> C{Create ID recovered?}
C -- No --> D[Report hard failure for manual reconciliation]
C -- Yes --> E[pm close with provenance reason]
E --> F{Close succeeded?}
F -- Yes --> G[Import succeeds]
F -- No --> H[Compensate created item]
H --> I{Status verified closed?}
I -- Yes --> J[Report failed row as compensated]
I -- No --> K[Report actionable failure with item ID]
Reviews (4): Last reviewed commit: "Require positive evidence that the orpha..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@package.json`:
- Around line 57-61: Update the `@unbrained/pm-cli` entry in peerDependencies to
match the tested ^2026.8.3 range used in devDependencies, unless compatibility
evidence for later major versions is added.
🪄 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: 95601fa8-c3d1-494c-8cbe-62012c632d59
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.agents/pm/history/pm-csv-mmi3.jsonl.agents/pm/issues/pm-csv-mmi3.toon.gitattributesindex.tspackage.json
The closed-row create-then-close sequence in upsertCreate is not atomic, and Greptile's P1 review of PR #57 found two distinct defects in that block (index.ts ~1759-1772): 1. Silent skip on an unrecoverable id. The follow-up close was guarded by `if (id && p.status === "closed")`, so when `pm create --json` exited 0 but the id could not be parsed (empty/unparseable JSON), the close was silently skipped and upsertCreate returned "" as if nothing was wrong. A row whose source said `closed` was then reported as imported-and-fine while actually open. For a `closed` row an unrecoverable id is now a hard failure: an error is thrown naming the title, so the row is reported as failed rather than silently imported. The created open orphan cannot be compensated without its id, so the error states that for manual reconciliation. The flat {id,status,changed_field_count} envelope parse is unchanged. 2. No compensation when `pm close` fails. The item was already persisted as open when the throw happened; the non-atomic caller (processImportRow) counted the row as skipped and moved on, leaving an undiscoverable open orphan that a retry without --key-field would duplicate. The created orphan is now compensated (closed) via the same compensateCreate() the atomic transaction path uses, so a failed row is all-or-nothing and a retry reconciles cleanly. Because compensation is an internal rollback rather than a user closure, compensateCreate() now closes with --validate-close off, so a tracker under strict closure-validation governance cannot block the rollback and re-leak the orphan. If compensation also fails, the thrown error carries the created id and states the item was left OPEN, keeping the partial state actionable. Compensation (all-or-nothing) is preferred over a pure id-carrying error because a CSV importer retries failed rows, and without --key-field an open orphan left behind would be duplicated on retry. The id-carrying error is retained as the fallback for the one case compensation cannot fix itself. The upsertCreate docstring is rewritten to state the guarantee the code actually provides; it no longer implies the create-then-close sequence is atomic. Tests (test/atomic.test.ts): - a `closed` row whose `pm close` fails (strict closure-validation governance): asserts the orphan is compensated (closed) on disk and the error states "compensated (closed)", not merely that it throws; - a `closed` row whose create id cannot be recovered (fake pm emits {}): asserts imported is NOT incremented (hard failure, not a silent open import) and the error names the unrecoverable id; - a `closed` row where compensation also fails (fake pm fails all close): asserts the error carries the created id and states the item was left OPEN. The fake-pm wrapper is tracker-scoped (marker-file gated) and transparent to concurrent test files. No existing assertion is weakened. Gates: typecheck, build, test (108 pass), coverage (lines 82.58 / branches 75.86 / funcs 88.42, all above thresholds), audit:prod, pack:dry-run, changelog:check all green. pm item: pm-csv-mmi3
pm create --json emits a flat receipt with no item wrapper; only query commands such as pm read and pm list wrap. A fallback for the wrapper is dead code no real CLI can exercise, and keeping it invites a test that asserts the fallback and so re-creates the failure mode it is meant to guard. The sibling pm-github package shipped a silent production bug from trusting exactly that wrapper. Filed upstream as unbraind/pm-cli#888 asking for the envelope contract to be documented and an SDK parser provided so consumers stop hand-parsing it.
Owner
Author
|
Pushed fixes for both round-1 findings. Summary for re-review:
Gates: typecheck clean, 108/108 tests, @greptileai |
|
✅ 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 20 minutes. |
Review follow-up. `itemStatus()` returns undefined whenever the lookup itself fails — non-zero exit, malformed JSON, or an absent status field — and the verification asked "is it still open?". Undefined therefore fell through to the success path, so a row was reported as compensated (closed) while the orphan might still be open. A retry without --key-field would then duplicate it, which is the exact leak the compensation exists to prevent: the guard was accepting absence of evidence as evidence. The check now demands a status of exactly `closed`. The error text distinguishes all three outcomes — verified closed, still open, and could not be verified — and every one of them carries the created id so the partial state stays actionable. A test drives a fake pm whose status lookup fails while both closes also fail; reverting the check to the old form makes it fail. The existing success assertion is tightened from "compensated (closed)" to "compensated (verified closed)", since an unchecked claim of compensation is what allowed this. typecheck clean, 109/109 tests pass, release:check green.
Owner
Author
|
Round-2 finding fixed (Greptile P1 — unknown status masked failed compensation). The verification now requires a status of exactly Gates: typecheck clean, 109/109 tests, @greptileai |
|
✅ 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 8 minutes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pm-cli 2026.8.3 enforces
governance.require_close_reasonon every programmatic transition intoclosed. Bothpm create --status closedandpm update <id> --status closedare now hardclose_reason_requirederrors — the 2026.7.29 bypass that invented the reason "Closed via pm update" is gone. Onlypm close --reasonmay close an item.What changed
A CSV row whose status is
closedis now createdopenand immediately transitioned throughpm close --reason.The reason text matters here. The CSV import format carries no close-reason or resolution column, so there is no genuine closure evidence to forward. Rather than invent an outcome the source never recorded (e.g.
"completed"), the reason states the import provenance factually:canceledis not gated by that policy and is still applied directly at create time, unchanged.What deliberately did not change
The transaction-marker semantics are untouched: presence of the
csv-tx/csv-txrowrow marker — not item status — remains the applied signal. A legitimately closed imported row is still recognised on resume and never re-imported, and a compensated create still has its markers stripped before closing, so it is correctly not recognised.Verification
typecheck,build,testandrelease:check(coverage gate, prod audit, pack dry-run,changelog:check) all pass against 2026.8.3. Pin moved to^2026.8.3with the lockfile refreshed.pm items
pm-csv-mmi3— Route terminal-status CSV imports throughpm closefor pm-cli 2026.8.3Tracked fleet-wide under the absorption epic
pm-cli-website-uqzw, which lives in the maintainer private companion tracker and is therefore not linkable from here.Summary by Sourcery
Route terminal-status CSV imports through pm-cli’s
pm close --reasonwith provenance-aware close reasons to comply with governance.require_close_reason in pm-cli 2026.8.3.New Features:
Bug Fixes:
closedare created asopenand then closed viapm close --reason, avoiding reason-less terminal transitions rejected by pm-cli 2026.8.3.Enhancements:
closed/canceledviapm close --reasonwith factual import provenance instead of invented outcomes.Build:
Chores:
pm-csv-mmi3change.Summary by cubic
Route terminal CSV imports through
pm close --reasonto comply with@unbrained/pm-cli2026.8.3, and harden the non-atomic create‑then‑close path by compensating failures and only claiming success when closure is verified; also remove a dead JSON parse fallback to prevent silent open orphans.Bug Fixes
pm close --reasonusing "Imported fromcanceled).pm closefails, compensate the orphan (close) with--validate-close off; only treat as compensated when status is verifiedclosed; if verification fails, report "could not be verified" with the created id; if compensation fails, include the id and state it was left open; if the create id can’t be recovered, hard‑fail the row.parsed.item?.idfallback; only parse the flat{id,...}frompm create --jsonto avoid masking failures.Dependencies
@unbrained/pm-clito^2026.8.3(peer>=2026.8.3).Written for commit e390525. Summary will update on new commits.