Repository navigation
feat(import): --atomic CSV import via commitWorkspaceTransaction (all-or-nothing, resumable) [pm-cli 2026.7.19] - #35
Merged
Conversation
…-or-nothing, resumable) [pm-cli 2026.7.19] Add an opt-in --atomic flag to (and the csv-import importer) that wraps the bounded in-memory create path in a single workspace writer-locked, crash-recoverable transaction using pm-cli 2026.7.19's commitWorkspaceTransaction primitive. - All-or-nothing: on any row create failure, every already-applied create is compensated (pm close, not delete) and the command exits non-zero; no committed (open) items from the import remain. - Crash-recoverable/resumable: a stable transaction id (csv-import-<sha1(absFilePath)>) plus a csv-tx:<transactionId> ownership marker let an interrupted run resume via inspect() without duplicating. - Default (non-atomic) path is byte-identical (proven via before/after diff). - --atomic + --stream fails fast (an unbounded stream cannot be one tx). - Bumps @unbrained/pm-cli dependency floor to >=2026.7.19. - 7 new integration tests (happy path, mid-import failure compensation, non-atomic contrast, resumability x2, --atomic+--stream rejection); 97/97 tests green. Refs: pm-csv-4bvs
Reviewer's GuideAdds an opt-in --atomic mode to Sequence diagram for atomic CSV import via commitWorkspaceTransactionsequenceDiagram
actor User
participant pm_csv_command as pm_csv_run
participant importCSV
participant importCSVAtomic
participant commitTransaction
participant pm as pm_binary
User->>pm_csv_command: pm csv import tasks.csv --atomic
pm_csv_command->>importCSV: importCSV(pm_root, filePath, opts)
importCSV->>importCSVAtomic: importCSVAtomic(pm_root, filePath, opts)
importCSVAtomic->>importCSVAtomic: deriveTransactionId(resolve(filePath))
importCSVAtomic->>importCSVAtomic: loadAppliedByTransaction(pm_root, transactionId)
importCSVAtomic->>importCSVAtomic: loadKeyIndex(pm_root)
importCSVAtomic->>importCSVAtomic: build WorkspaceTransactionStep[]
alt ctx.sdk.commitWorkspaceTransaction provided
importCSVAtomic->>commitTransaction: commitTransaction({transactionId, author, steps})
else dynamic SDK resolution
importCSVAtomic->>commitTransaction: resolveCommitWorkspaceTransaction(pm_root)
importCSVAtomic->>commitTransaction: commitTransaction({transactionId, author, steps})
end
loop for each WorkspaceTransactionStep
commitTransaction->>importCSVAtomic: inspect()
alt [state pending]
commitTransaction->>importCSVAtomic: apply()
alt row is create
importCSVAtomic->>pm: upsertCreate(pm_root, parsedRow, keyValue, source, [ownershipTag])
else row is update
importCSVAtomic->>pm: upsertUpdate(pm_root, existingId, parsedRow, source, [ownershipTag])
end
else [state applied]
note over commitTransaction,importCSVAtomic: step already applied (resume)
end
end
alt commitTransaction throws error
commitTransaction-->>importCSVAtomic: Error
importCSVAtomic->>importCSVAtomic: compensate() on each step
importCSVAtomic->>pm: compensateCreate(pm_root, id)
importCSVAtomic-->>importCSV: CommandError
else commitTransaction succeeds
commitTransaction-->>importCSVAtomic: WorkspaceTransactionCommitResult
importCSVAtomic->>importCSVAtomic: tally imported/updated from committed.results
importCSVAtomic-->>importCSV: ImportResult
importCSV-->>pm_csv_command: ImportResult
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughAdds ChangesAtomic CSV import
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CSVImportCommand
participant importCSVAtomic
participant commitWorkspaceTransaction
participant Tracker
User->>CSVImportCommand: run csv import --atomic
CSVImportCommand->>importCSVAtomic: pass atomic options
importCSVAtomic->>Tracker: inspect per-row transaction markers
importCSVAtomic->>commitWorkspaceTransaction: submit create/update steps
commitWorkspaceTransaction->>Tracker: apply workspace transaction
importCSVAtomic->>Tracker: compensate current-run creates on failure
🚥 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 |
Owner
Author
|
@coderabbitai full review |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 52 minutes. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Using
loadAppliedByTransactionto scanpm list-alland then build maps by title/key could become quite expensive on large trackers; if the CLI supports more selective querying by tag or transaction id, consider narrowing the query tocsv-tx:<transactionId>at the pm level rather than filtering client-side. - The resumability logic relies on matching rows by
titleor normalized key within a transaction, butloadAppliedByTransactionuses aMapkeyed only by title/key and will overwrite prior entries when multiple rows share the same value; if duplicate titles/keys per transaction are possible, consider storing and disambiguating multiple ids (e.g., by row index or additional metadata) to avoid incorrectly skipping or compensating rows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `loadAppliedByTransaction` to scan `pm list-all` and then build maps by title/key could become quite expensive on large trackers; if the CLI supports more selective querying by tag or transaction id, consider narrowing the query to `csv-tx:<transactionId>` at the pm level rather than filtering client-side.
- The resumability logic relies on matching rows by `title` or normalized key within a transaction, but `loadAppliedByTransaction` uses a `Map` keyed only by title/key and will overwrite prior entries when multiple rows share the same value; if duplicate titles/keys per transaction are possible, consider storing and disambiguating multiple ids (e.g., by row index or additional metadata) to avoid incorrectly skipping or compensating rows.
## Individual Comments
### Comment 1
<location path="index.ts" line_range="1330-1339" />
<code_context>
+ for (let rowIndex = 0; rowIndex < dataRows.length; rowIndex++) {
+ const lineNo = opts.skipHeaders ? rowIndex + 1 : rowIndex + 2;
+ const row = dataRows[rowIndex];
+ const { parsed } = rowFields(headers, row);
+
+ if (!parsed.title) {
+ console.error(`Row ${lineNo}: skipping — 'title' is empty`);
+ result.skipped++;
+ continue;
+ }
+ if (!rowMatchesFilter(parsed, opts.filter)) {
+ result.skipped++;
+ result.filtered++;
+ continue;
+ }
+ const keyValue = opts.keyField ? (rowFields(headers, row).get(opts.keyField) ?? "") : "";
+ const existingId = keyValue ? keyIndex.get(normalizeKeyValue(keyValue)) : undefined;
+ planned.push({ rowIndex, lineNo, parsed, keyValue, existingId, isUpdate: Boolean(existingId) });
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recomputing `rowFields` for `keyValue` when it was just computed.
Within the planning loop, `rowFields(headers, row)` is invoked twice: once to get `parsed` and again to derive `keyValue`. This adds unnecessary per-row overhead. Consider reusing the initial result (e.g., returning the key field alongside `parsed` or exposing the underlying map) so each row is parsed only once.
Suggested implementation:
```typescript
for (let rowIndex = 0; rowIndex < dataRows.length; rowIndex++) {
const lineNo = opts.skipHeaders ? rowIndex + 1 : rowIndex + 2;
const row = dataRows[rowIndex];
const fields = rowFields(headers, row);
const { parsed } = fields;
if (!parsed.title) {
console.error(`Row ${lineNo}: skipping — 'title' is empty`);
result.skipped++;
continue;
}
if (!rowMatchesFilter(parsed, opts.filter)) {
result.skipped++;
result.filtered++;
continue;
}
const keyValue = opts.keyField ? (fields.get(opts.keyField) ?? "") : "";
const existingId = keyValue ? keyIndex.get(normalizeKeyValue(keyValue)) : undefined;
planned.push({ rowIndex, lineNo, parsed, keyValue, existingId, isUpdate: Boolean(existingId) });
}
```
If `rowFields` currently does not return an object that both:
1) has a `parsed` property, and
2) supports `.get(key: string)`,
you will need to adjust its return type/implementation so that a single call can be used for both purposes (e.g., return a `Map` extended with `parsed`, or an object that wraps the map and exposes `.get`).
</issue_to_address>
### Comment 2
<location path="index.ts" line_range="1451-1460" />
<code_context>
+ * dynamic `import("@unbrained/pm-cli/sdk")` so a standalone-installed extension
+ * still works when the SDK package is resolvable.
+ */
+async function resolveCommitWorkspaceTransaction(
+ pmRoot: string,
+): Promise<(opts: {
+ transactionId: string;
+ author: string;
+ steps: readonly WorkspaceTransactionStep[];
+ lockTtlSeconds?: number;
+ lockWaitMs?: number;
+}) => Promise<WorkspaceTransactionCommitResult>> {
+ const mod: typeof import("@unbrained/pm-cli/sdk") = await import("@unbrained/pm-cli/sdk");
+ const commit = mod.commitWorkspaceTransaction;
+ return (opts) => commit({ pmRoot, ...opts });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Dynamic SDK import on each atomic call lacks explicit failure handling and caching.
This helper re-imports `@unbrained/pm-cli/sdk` on every call and surfaces missing/mis-resolved SDK as a generic module load error. Wrap the import in `try/catch` and rethrow a `CommandError` with a clearer message (e.g., required pm-cli version/installation). You could also cache `commitWorkspaceTransaction` after the first import to avoid repeated dynamic imports for `--atomic` calls.
Suggested implementation:
```typescript
let cachedCommitWorkspaceTransaction:
| (typeof import("@unbrained/pm-cli/sdk"))["commitWorkspaceTransaction"]
| undefined;
/**
* Dynamically resolve the SDK commit coordinator bound to a tracker root, for
* the importer path (which has no host-injected `ctx.sdk`). Falls back to a
* dynamic `import("@unbrained/pm-cli/sdk")` so a standalone-installed extension
* still works when the SDK package is resolvable.
*/
async function resolveCommitWorkspaceTransaction(
pmRoot: string,
): Promise<(opts: {
transactionId: string;
author: string;
steps: readonly WorkspaceTransactionStep[];
lockTtlSeconds?: number;
lockWaitMs?: number;
}) => Promise<WorkspaceTransactionCommitResult>> {
if (cachedCommitWorkspaceTransaction) {
return (opts) => cachedCommitWorkspaceTransaction({ pmRoot, ...opts });
}
try {
const mod: typeof import("@unbrained/pm-cli/sdk") = await import("@unbrained/pm-cli/sdk");
const commit = mod.commitWorkspaceTransaction;
if (typeof commit !== "function") {
throw new CommandError(
"Loaded pm-cli SDK does not expose commitWorkspaceTransaction; please ensure @unbrained/pm-cli is correctly installed and matches the CLI version.",
);
}
cachedCommitWorkspaceTransaction = commit;
return (opts) => commit({ pmRoot, ...opts });
} catch (error) {
// Wrap dynamic import failures in a more actionable CLI error
throw new CommandError(
"Unable to load pm-cli SDK required for atomic workspace transactions. Ensure @unbrained/pm-cli is installed and compatible with this CLI.",
{ cause: error },
);
}
}
```
1. Ensure `CommandError` is imported at the top of `index.ts` from the existing error utility module used in this codebase (e.g., wherever other command/CLI errors are defined).
2. If `CommandError` does not accept an options object with `{ cause }`, adjust the constructor calls to match the existing signature (for example, `new CommandError("message", error)` or similar).
3. If your project’s TypeScript configuration does not support `typeof import("...")` in type positions, you may need to replace the `cachedCommitWorkspaceTransaction` type with an explicit function type matching `commitWorkspaceTransaction`’s signature from the SDK.
</issue_to_address>
### Comment 3
<location path="test/atomic.test.ts" line_range="237-246" />
<code_context>
+test("--atomic resumability: partial application resumes without duplicating", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for key-based upserts in the atomic path (create+update behavior and resumability via csv-key)
Existing atomic tests only cover title-keyed creates and never exercise `--key`-based upserts, which follow separate logic (`byKey` lookup, `normalizeKeyValue`, and non-compensated updates). Please add tests that:
- Import a CSV using `csv-key` (or the configured `--key` field) in atomic mode so some rows create items and others update existing ones.
- Verify that on a mid-import failure, newly created items are compensated but pre-existing updated items are not rolled back.
- Cover resumability where a partially applied run leaves key-tagged items with the transaction tag, and a resumed atomic run correctly detects them via key-based lookup and avoids duplicate creates/updates.
This will validate the `applied.byKey` path in `inspect`, the "updates are not compensated" behavior, and key-based resumability for `--atomic --key`.
Suggested implementation:
```typescript
} finally {
rmSync(root, { recursive: true, force: true });
}
});
// --atomic --key: key-based upserts with partial application and non-compensated updates
test("--atomic --key: key-based upserts compensate creates but not updates on failure", async () => {
const root = freshTracker();
const file = join(root, "atomic-key-upsert.csv");
// We use a dedicated key column (`csv-key`) to exercise the byKey/normalizeKeyValue path.
// Rows 1-2 update pre-existing items, rows 3-4 create new items.
writeFileSync(
file,
[
"csv-key,title,status,priority",
"KEY-1,Existing 1,open,2",
"KEY-2,Existing 2,open,3",
"KEY-3,New 3,open,1",
"KEY-4,New 4,open,2",
].join("\n") + "\n",
);
try {
// Seed pre-existing items that will be updated by the atomic import.
// These items must NOT receive the transaction tag so that updates are
// treated as non-compensated changes in the atomic path.
const seed = join(root, "atomic-key-upsert-seed.csv");
writeFileSync(
seed,
[
"csv-key,title,status,priority",
"KEY-1,Existing 1,open,1",
"KEY-2,Existing 2,open,1",
].join("\n") + "\n",
);
// Non-atomic baseline import creates the pre-existing items.
const seeded = await runImport(root, seed, { atomic: false, key: "csv-key" });
assert.ifError(seeded.error);
assert.equal(seeded.result.imported, 2, "baseline run imports 2 pre-existing items");
assert.equal(listItems(root).length, 2, "tracker contains 2 items before atomic run");
// Now run atomic --key import and simulate a mid-import failure. The importer
// should:
// - update KEY-1 and KEY-2 (non-compensated updates)
// - create KEY-3 and KEY-4 (compensated creates)
//
// The failure must occur after at least one create has been applied so we can
// validate compensation of newly-created items without rolling back updates.
const first = await runImport(root, file, {
atomic: true,
key: "csv-key",
// NOTE: this option name is illustrative; adapt it to whatever the test
// harness uses to simulate partial application / failure.
failAfterApplied: 3,
});
assert(first.error, "atomic run should fail mid-import");
assert.ok(
first.result.imported > 0,
"atomic run applies some rows before failing (creates + updates)",
);
const afterFailure = listItems(root);
// We expect:
// - Updated existing items (KEY-1, KEY-2) to remain, reflecting the atomic
// run's changes.
// - Newly created items (KEY-3, KEY-4) to be compensated/rolled back.
const keysAfterFailure = new Set(afterFailure.map((item: any) => item["csv-key"]));
assert.ok(keysAfterFailure.has("KEY-1"), "KEY-1 exists after failed atomic run");
assert.ok(keysAfterFailure.has("KEY-2"), "KEY-2 exists after failed atomic run");
assert.ok(!keysAfterFailure.has("KEY-3"), "KEY-3 is compensated and removed");
assert.ok(!keysAfterFailure.has("KEY-4"), "KEY-4 is compensated and removed");
} finally {
rmSync(root, { recursive: true, force: true });
}
});
// --atomic --key resumability: key-based lookup detects previously applied rows
// and avoids duplicate creates/updates when resuming a partially applied run.
test("--atomic --key resumability: key-based partial application resumes without duplicating", async () => {
const root = freshTracker();
const file = join(root, "atomic-key-resume.csv");
// Mixed create/update set using csv-key:
// - KEY-1, KEY-2: pre-existing items that will be updated
// - KEY-3, KEY-4: new items that will be created
writeFileSync(
file,
[
"csv-key,title,status,priority",
"KEY-1,Existing 1 updated,open,2",
"KEY-2,Existing 2 updated,open,3",
"KEY-3,New 3,open,1",
"KEY-4,New 4,open,2",
].join("\n") + "\n",
);
try {
// Seed pre-existing items without transaction tag. These will be updated by
// the atomic run and must survive compensation as non-compensated updates.
const seed = join(root, "atomic-key-resume-seed.csv");
writeFileSync(
seed,
[
"csv-key,title,status,priority",
"KEY-1,Existing 1 original,open,1",
"KEY-2,Existing 2 original,open,1",
].join("\n") + "\n",
);
const seeded = await runImport(root, seed, { atomic: false, key: "csv-key" });
assert.ifError(seeded.error);
assert.equal(seeded.result.imported, 2, "baseline run imports 2 pre-existing items");
assert.equal(listItems(root).length, 2, "tracker contains 2 items before atomic run");
// Simulate a prior interrupted atomic run that applied only the first 3 rows
// (KEY-1 update, KEY-2 update, KEY-3 create) and tagged them with the
// transaction ownership tag derived from the absolute file path.
const transactionId = `csv-import-${sha1(join(root, "atomic-key-resume.csv")).slice(0, 12)}`;
// Mark KEY-1, KEY-2, and KEY-3 as applied for the transaction. The
// implementation of applyTransactionTag should mirror whatever the atomic
// import code uses when tagging applied rows for inspect/resume.
const items = listItems(root);
for (const item of items) {
if (item["csv-key"] === "KEY-1" || item["csv-key"] === "KEY-2") {
item.transaction = transactionId;
}
}
// KEY-3 does not exist yet; simulate its creation with the transaction tag so
// that the resumed atomic run sees it as already applied via byKey lookup.
items.push({
"csv-key": "KEY-3",
title: "New 3",
status: "open",
priority: 1,
transaction: transactionId,
});
// Persist simulated state in whatever way the tracker expects.
saveItems(root, items);
// Resumed atomic --key run should:
// - skip KEY-1, KEY-2, and KEY-3 (already applied via byKey/transaction tag)
// - apply only KEY-4 as a new create
const resumed = await runImport(root, file, { atomic: true, key: "csv-key" });
assert.ifError(resumed.error);
assert.equal(
resumed.result.imported,
1,
"resumed atomic run imports only the remaining unapplied row (KEY-4)",
);
const finalItems = listItems(root);
const keys = new Set(finalItems.map((item: any) => item["csv-key"]));
assert.ok(keys.has("KEY-1"), "KEY-1 still present after resume");
assert.ok(keys.has("KEY-2"), "KEY-2 still present after resume");
assert.ok(keys.has("KEY-3"), "KEY-3 still present after resume");
assert.ok(keys.has("KEY-4"), "KEY-4 created by resumed run");
} finally {
rmSync(root, { recursive: true, force: true });
}
});
test("--atomic resumability: partial application resumes without duplicating", async () => {
```
The above tests assume several helpers and behaviors that you may need to align with your existing test harness:
1. **Failure simulation in atomic import**
- The `{ failAfterApplied: 3 }` option is illustrative. Replace it with the actual mechanism your `runImport` helper uses to simulate partial application / mid-import failure in existing atomic tests (e.g. `maxApplied`, `interruptAfter`, or a dedicated failure flag).
2. **Key configuration for imports**
- If your CLI uses a different option name than `key: "csv-key"` for `--key`, adjust the options passed to `runImport` accordingly.
- Ensure the CSV header (`csv-key`) matches the configured key field used by the atomic importer.
3. **Tracker item shape and persistence**
- The code assumes tracker items are plain objects with fields like `"csv-key"`, `title`, `status`, `priority`, and that you can mutate `item.transaction` and then call `saveItems(root, items)`:
- Replace `transaction` with the actual field name used to store the transaction ownership tag.
- Replace `saveItems(root, items)` with your real helper to persist modified items (e.g. `writeTracker`, `saveTrackerState`, or similar).
- If `listItems(root)` returns a different shape (e.g. `{ key, data, meta }`), adjust attribute access to align with your implementation.
4. **Transaction id derivation**
- The `sha1(...)` call is illustrative. If your atomic implementation uses a specific helper to derive the transaction id from the file path, replace the `transactionId` computation with that helper to ensure tests match the real behavior.
5. **TypeScript typings**
- If your tests are strictly typed, replace `(item: any)` casts with your concrete item type or update type definitions to include the `transaction` and `csv-key` fields.
These adaptations will make the new tests properly validate:
- the `applied.byKey` path in `inspect`,
- the "updates are not compensated" behavior for `--atomic --key`, and
- key-based resumability for `--atomic --key` without duplicate creates/updates.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an --atomic flag for the CSV import command, allowing all-or-nothing imports under a single workspace transaction using commitWorkspaceTransaction (introduced in pm-cli version 2026.7.19). The implementation includes crash-recoverable/resumable logic that skips already-applied rows by tagging created items with a transaction-specific marker, as well as comprehensive integration tests. The reviewer feedback highlights a correctness issue where duplicate titles in a CSV could cause incorrect step skipping on resume when no unique key is specified. To resolve this, the reviewer suggests stamping each item with a row-specific tag (csv-tx:<transactionId>-<rowIndex>) and updating the transaction steps, tallying logic, and integration tests accordingly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Greptile SummaryThis PR adds opt-in atomic CSV imports through the pm-cli transaction API. The main changes are:
Confidence Score: 4/5This change has one contained correctness issue in the new atomic resume path. The default import path is preserved, and common atomic success and rollback cases are covered. Duplicate-title create rows can be skipped after an interrupted atomic import, which can produce an incomplete import result. index.ts
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant CSV as pm csv import --atomic
participant SDK as commitWorkspaceTransaction
participant PM as pm CLI/store
User->>CSV: import file with --atomic
CSV->>CSV: derive transactionId from absolute file path
CSV->>PM: list-all --json for csv-tx marker and csv-key index
CSV->>SDK: commitWorkspaceTransaction(transactionId, steps)
loop each planned CSV row
SDK->>CSV: inspect()
CSV-->>SDK: pending or applied
alt pending create/update
SDK->>CSV: apply()
CSV->>PM: pm create/update with csv-tx tag
PM-->>CSV: item id
CSV-->>SDK: result item id
end
end
alt apply failure
SDK->>CSV: compensate applied create steps
CSV->>PM: pm close created item(s)
SDK-->>CSV: throw rollback error
CSV-->>User: non-zero rolled-back result
else success
SDK-->>CSV: committed step results
CSV-->>User: imported/updated/skipped counts
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant CSV as pm csv import --atomic
participant SDK as commitWorkspaceTransaction
participant PM as pm CLI/store
User->>CSV: import file with --atomic
CSV->>CSV: derive transactionId from absolute file path
CSV->>PM: list-all --json for csv-tx marker and csv-key index
CSV->>SDK: commitWorkspaceTransaction(transactionId, steps)
loop each planned CSV row
SDK->>CSV: inspect()
CSV-->>SDK: pending or applied
alt pending create/update
SDK->>CSV: apply()
CSV->>PM: pm create/update with csv-tx tag
PM-->>CSV: item id
CSV-->>SDK: result item id
end
end
alt apply failure
SDK->>CSV: compensate applied create steps
CSV->>PM: pm close created item(s)
SDK-->>CSV: throw rollback error
CSV-->>User: non-zero rolled-back result
else success
SDK-->>CSV: committed step results
CSV-->>User: imported/updated/skipped counts
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
index.ts:1374-1375
**Duplicate titles break resume**
`inspect()` keys create rows only by `parsed.title`, so a crash after applying the first of two same-title rows makes the next run treat every row with that title as already applied. `loadAppliedByTransaction()` stores one `byTitle` entry per title, so the pending duplicate step is skipped and the atomic import finishes with fewer items than rows.
Reviews (2): Last reviewed commit: "feat(import): --atomic CSV import via co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… SDK resolve hardening (2026.7.19-1) Addresses all PR #35 review findings (Greptile 3/5 blocker + Gemini + Sourcery). Version: 2026.7.19 -> 2026.7.19-1 (same-day re-release; this feature commit lands after the 06:10 UTC daily release of 2026.7.19. Precedent: pm-github 2026.7.18-1). CORE CORRECTNESS (resume/compensation per-row-precise): Resume/compensation matched created rows by title (applied.byTitle) and update rows by key (applied.byKey). This is wrong for CSVs with duplicate titles or duplicate keys: a resumed 2nd same-titled row found the 1st row's item via byTitle and was WRONGLY skipped (only one item instead of two); two rows sharing a new key both planned as create (keyIndex not updated during planning) -> duplicate items. Fix: stamp every item this transaction writes with a per-row ownership marker `csv-txrow:<transactionId>#<rowIndex>` (source of truth) in addition to the batch-level `csv-tx:<transactionId>` marker (kept for scanning). loadAppliedBy Transaction now returns byRowIndex: Map<number,string> parsed from the per-row marker; byTitle/byKey dropped. inspect() for EVERY row (create AND update) resolves prior application via byRowIndex.get(row.rowIndex); compensate() targets appliedId ?? byRowIndex.get(row.rowIndex); the final tally uses byRowIndex.has(row.rowIndex). Duplicate titles/keys are now irrelevant to resume/compensation. The marker round-trips through pm tag storage (pm lower- cases tags; transactionId is csv-import-<hex> and rowIndex is numeric). In-batch duplicate-KEY guard: During planning, when --key is set, track keys claimed by an earlier planned CREATE in THIS run (claimedKeys). A later row repeating a not-yet-existing key is SKIPPED with a clear per-row warning + result.skipped++ (simplest correct behavior; documented in README + code). Pre-existing-in-tracker keys still update normally. Sourcery perf: rowFields(headers,row) was computed twice per row in the planning loop (once for `parsed`, once for `keyValue`). Now computed once and reused. Sourcery bug_risk: resolveCommitWorkspaceTransaction dynamic `import("@unbrained/pm-cli/sdk")` wrapped in try/catch; throws an actionable CommandError (USAGE) if the import fails or commitWorkspaceTransaction is not a function (requires @unbrained/pm-cli>=2026.7.19 installed). The resolved function is cached in a module-level var so repeated --atomic calls don't re-import. Export: Now strips csv-tx: and csv-txrow: provenance tags (previously only csv-key:/ csv-source: were stripped) so round-trip exports stay clean. The non-atomic import path is byte-identical. Tests (test/atomic.test.ts, 12 atomic tests total, 102 suite total): - duplicate-title creates: two rows same title -> TWO items; resume skips both via per-row marker (not byTitle). - duplicate-title partial resume: only row 0 tagged -> resume creates row 1 (the bug case: old byTitle would wrongly skip row 1). - --key upsert mid-import failure: created rows compensated (closed), pre- existing updated rows NOT rolled back (still open, retains updated priority). - resume detection via per-row tag (not title/key): sparse partial run (rows 0 and 2 applied) resumes creating exactly rows 1 and 3. - in-batch duplicate-key guard: a repeated NEW key is skipped, not double- created. - existing partial-application test updated to stamp the new per-row marker. pm items: closed pm-csv-4bvs (atomic task) and filed+closed sub-item pm-csv-j3pi documenting the fixes (author pi-glm). CHANGELOG regenerated for 2026.7.19-1. release:check: typecheck + build + build:test + node --test (102 pass) + audit:prod (0 vuln) + pack:dry-run + changelog:check all clean. Co-Authored-By: pi-glm <pi-glm@users.noreply.github.com>
Owner
Author
|
Pushed 83125ab addressing all prior review findings:
@greptileai |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 56 minutes. |
![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 `@index.ts`:
- Around line 1304-1311: Initialize the dry-run keyIndex in the opts.dryRun
branch by loading loadKeyIndex(pmRoot) when opts.keyField is set, while
retaining an empty map when no key field is configured. Pass this seeded index
to processImportRow so previews match the actual upsert outcome.
🪄 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
Run ID: cfd7c8a8-e73d-4814-92ec-3a3745310eff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.agents/pm/history/pm-csv-4bvs.jsonl.agents/pm/history/pm-csv-j3pi.jsonl.agents/pm/tasks/pm-csv-4bvs.toon.agents/pm/tasks/pm-csv-j3pi.toonCHANGELOG.mdREADME.mdindex.tsmanifest.jsonpackage.jsontest/atomic.test.ts
…tection
Greptile round-2 P1 "Compensated rows block retry": compensateCreate() rolls a
create back with `pm close` (not delete, to avoid history resurrection), which
leaves the `csv-txrow:<txId>#<rowIndex>` tag on the now-closed item. Because
loadAppliedByTransaction() recorded any tagged item regardless of status, those
compensated items counted as "applied" — so re-running the same file after a
full rollback skipped every rolled-back row and imported only the rows after the
original failure, instead of redoing the whole batch.
loadAppliedByTransaction() now skips closed items. A post-rollback retry
re-creates the rolled-back rows (closed tombstones are left in place);
genuinely interrupted runs (crash before compensation) leave their items OPEN,
so real resume detection is unaffected.
+1 regression test ("post-rollback retry re-imports the FULL batch") → 103 tests
pass; release:check clean. Documented in pm-csv-j3pi.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round-3 push @greptileai |
|
✏️ Learnings added
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 24 minutes. |
Owner
Author
|
All prior findings are now resolved and replied inline:
103 tests pass, |
…" signal
Greptile round-3 P1 ("Closed rows re-import" / "Closed rows duplicate"): the
round-2 fix excluded *all* closed items from resume detection, but a CSV row
whose status is `closed`/`canceled` is legitimately imported as a closed item
(upsertCreate/upsertUpdate pass the row's status). Excluding closed items made
those rows re-import (duplicate) on a resumed run.
Fix: match on marker PRESENCE, not status. loadAppliedByTransaction() records
any item carrying this transaction's `csv-txrow:<txId>#<rowIndex>` marker
regardless of status; compensateCreate() now STRIPS the csv-tx/csv-txrow markers
before closing a rolled-back row (best-effort; runs whether the item is open or
already closed, so a closed-status create being rolled back also loses its
marker). Net: marker present => applied (resume idempotently, including
closed-status rows); marker absent => rolled back (re-import on retry).
+1 regression test ("closed-status rows resume idempotently") → 104 tests pass;
release:check clean. The earlier post-rollback-retry test still passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round-4 push @greptileai |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- changed files since parent ---'
git diff --name-status 19a34b2^ 19a34b2
echo '--- focused Round-4 diff ---'
git diff --unified=80 19a34b2^ 19a34b2 -- index.ts test/atomic.test.ts
echo '--- module/test outlines ---'
ast-grep outline index.ts --items all --type function,class --view compact || true
ast-grep outline test/atomic.test.ts --items all --type function,class --view compact || true
echo '--- marker, compensation, and transaction call sites ---'
rg -n -C 5 -e 'csv-txrow:|csv-tx:' -e '\bcompensate\b' -e '\binspect\b' \
-e 'loadAppliedByTransaction' -e 'commitWorkspaceTransaction' \
index.ts test/atomic.test.tsLength of output: 49774 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- atomic helper and create/update implementations ---'
sed -n '1180,1585p' index.ts
sed -n '1620,1745p' index.ts
echo '--- atomic test setup, transaction injection, and rollback tests ---'
sed -n '1,240p' test/atomic.test.ts
sed -n '240,570p' test/atomic.test.ts
echo '--- workspace transaction type declarations / installed SDK references ---'
rg -n -C 4 \
-e 'interface WorkspaceTransactionStep\b' \
-e 'type WorkspaceTransactionStep\b' \
-e 'commitWorkspaceTransaction\b' \
-e 'WorkspaceTransactionCommitResult\b' \
-e 'remove-tags' \
-e 'function upsertCreate\b' \
-e 'function upsertUpdate\b' \
-g '*.ts' -g '*.d.ts' .
echo '--- package metadata relevant to SDK version and test scripts ---'
sed -n '1,220p' package.jsonLength of output: 60995
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 58 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
1253-1287: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSilent failure on tag-strip is not surfaced.
The
--remove-tagsspawnSyncresult at Lines 1271-1275 is never checked. If it fails (non-zero exit, or a transport/pminvocation error), the markers are not actually stripped, but the function proceeds as if they were. Since marker-presence is now the sole "applied" signal (per the doc comment at Lines 1257-1260), a silent failure here would make a rolled-back row look "still applied" on resume — reintroducing exactly the bug this round's fix (and thetest/atomic.test.ts:469-504regression test) targets, with no error logged for diagnosis. Theclosecall right below (Lines 1278-1286) does checkr.statusand log on unexpected failure; the strip call should follow the same pattern.🔧 Proposed fix
if (markers.length > 0) { // Best-effort: remove the tx markers so this row is no longer "applied". - spawnSync( + const untag = spawnSync( "pm", ["--path", pmRoot, "update", id, "--remove-tags", markers.join(",")], { encoding: "utf-8" }, ); + if (untag.status !== 0) { + console.error( + `atomic import: failed to strip tx markers for ${id}: ${untag.stderr?.trim() || untag.stdout?.trim()}`, + ); + } }🤖 Prompt for 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. In `@index.ts` around lines 1253 - 1287, Update compensateCreate’s marker-removal spawnSync call to inspect its result, treating non-zero status and invocation or transport errors as failures. Log the marker-strip failure with the item ID and available stderr/stdout details, matching the existing compensation close failure handling, before proceeding to the close step.
🤖 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 @.agents/pm/tasks/pm-csv-j3pi.toon:
- Around line 15-18: Synchronize the release metadata in
.agents/pm/tasks/pm-csv-j3pi.toon lines 15-18 by updating resolution,
expected_result, actual_result, and close_reason to reflect the final closed
state and 104 tests. Append a corrective update/closure record to
.agents/pm/history/pm-csv-j3pi.jsonl lines 5-6 documenting the round-2/round-3
fixes and final 104-test result.
---
Outside diff comments:
In `@index.ts`:
- Around line 1253-1287: Update compensateCreate’s marker-removal spawnSync call
to inspect its result, treating non-zero status and invocation or transport
errors as failures. Log the marker-strip failure with the item ID and available
stderr/stdout details, matching the existing compensation close failure
handling, before proceeding to the close step.
🪄 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
Run ID: f9070387-b00b-4f7b-9548-c670f7357c05
📒 Files selected for processing (4)
.agents/pm/history/pm-csv-j3pi.jsonl.agents/pm/tasks/pm-csv-j3pi.toonindex.tstest/atomic.test.ts
…stale skips)
Greptile round-4 P1 ("Path-only transaction id" / "Transaction id stales"):
deriveTransactionId hashed only resolve(filePath), and resume matches on
rowIndex. So editing a file in place and re-running `pm csv import --atomic`
reused the previous import's csv-txrow:#<n> markers — inspect() treated the
changed rows as already applied and skipped them, reporting a successful import
that changed nothing (silent stale data / data loss; Greptile reproduced it).
The transaction id now folds a SHA-1 fingerprint of the exact parsed content
(rawHeaders + dataRows) via fingerprintContent(). Same content re-run after a
crash keeps the same id (resumes from the journal, no duplicates); editing the
file yields a fresh id (a clean import of the new contents) — never a stale skip.
Also exports atomicTransactionId(filePath, opts) as the single source of truth
for the derivation; the three partial-resume tests now compute the real id via
it instead of a hardcoded path-only formula.
New regression test (edit-in-place re-imports new content) + a marker-stripped
assertion on the mid-import-failure test. 105 tests pass; release:check clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round-5 push @greptileai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR change overview ---'
git diff --stat "$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD^)" HEAD || true
git log --oneline -12
echo '--- changed source/test candidates ---'
fd -t f -e ts -e json -e md . | sort
echo '--- exported/module structure ---'
ast-grep outline index.ts --items all --type function,class --view compact || true
ast-grep outline test/atomic.test.ts --items all --type function,class --view compact || true
echo '--- atomic flow, transaction-ID, compensation, and marker references ---'
rg -n -C 8 \
-e 'atomicTransactionId\b' \
-e 'importCSVAtomic\b' \
-e 'loadAppliedByTransaction\b' \
-e 'compensateCreate\b' \
-e 'remove-tags\b' \
-e 'csv-txrow:|csv-tx:' \
-e 'commitWorkspaceTransaction\b' \
index.ts test/atomic.test.ts
echo '--- current atomic-related diff ---'
git diff --unified=60 "$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD^)" HEAD -- index.ts test/atomic.test.ts package.json README.md CHANGELOG.md || true
echo '--- transaction coordinator declarations and mocked CLI behavior ---'
rg -n -C 5 \
-e 'WorkspaceTransactionStep\b' \
-e 'commitWorkspaceTransaction\b' \
-e 'compensate\s*:' \
-e 'remove-tags\b' \
-g '*.ts' -g '*.d.ts' .Length of output: 152157
|
…nds) CodeRabbit review nit: the tracking task recorded a stale 102-test count and was closed before the round-2..5 follow-ups. actual_result now reflects the final shipped state (105 tests, all Greptile/Sourcery/CodeRabbit findings resolved). Metadata-only; no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 28, 2026
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.
What
Adds an opt-in
--atomicmode topm csv importthat commits an entire CSV batch through the pm-cli 2026.7.19 public SDK primitivecommitWorkspaceTransaction— all-or-nothing, crash-recoverable, and multi-agent-safe. Ships as2026.7.19-1(this feature lands after today's 06:10 UTC daily release of2026.7.19).Today the importer creates items with a per-row
pm createshell-out: if row N fails mid-batch, rows1..N-1are already written (partial, non-atomic state), and two concurrent agents/imports can interleave writes. With--atomic:WorkspaceTransactionStep(inspect/apply/compensate), committed under a single workspace writer lock.transactionIdderived from the input file makes an interrupted import resumable.Default (non-
--atomic) behaviour is byte-identical to before. Requires@unbrained/pm-cli >= 2026.7.19.--atomic+--streamis rejected (an unbounded stream cannot be one atomic transaction).Review-round-2 hardening (commit
83125ab)Addresses every finding from Greptile (3/5 blocker), Gemini, and Sourcery:
csv-txrow:<transactionId>#<rowIndex>ownership marker (the batch-levelcsv-tx:marker is kept only for scanning; the oldbyTitle/byKeymatching is dropped). A CSV with duplicate titles or duplicate keys can no longer trickinspect()into skipping the wrong row orcompensate()into closing the wrong item.--keyguard: a repeated not-yet-existing key is skipped with a clear per-row warning instead of double-creating.rowFields()is computed once per row (was twice).resolveCommitWorkspaceTransactionwraps the dynamic SDK import in try/catch → an actionableCommandError, and caches the resolved function across--atomiccalls.Testing
npm run release:checkclean (typecheck, build, tests,audit:prod,pack:dry-run,changelog:check).--atomic --keyupsert with mid-import failure (creates compensated, pre-existing updates not rolled back); resume detection via per-row tag; in-batch duplicate-key guard.PM tracking
pm-csv-4bvs(feature, closed) ·pm-csv-j3pi(review-round-2 hardening)pm-cli-website-s4wz→ childpm-cli-website-v571; upstream trackerpm-cli-website-uvws(pm-cli#613)🤖 Generated with Claude Code