Skip to content

Make GitHub issue imports atomic and crash-resumable#5

Merged
unbraind merged 3 commits into
mainfrom
feat/atomic-github-import
Jul 21, 2026
Merged

Make GitHub issue imports atomic and crash-resumable#5
unbraind merged 3 commits into
mainfrom
feat/atomic-github-import

Conversation

@unbraind

@unbraind unbraind commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • add opt-in --atomic GitHub issue imports using the pm 2026.7.20 commitItemMutations SDK primitive
  • make transaction identity and ordered mutation plans stable across reordered API responses and partially applied retries
  • compensate mixed create/update/close failures, report recovered batches truthfully, and keep native comment annotations post-commit
  • adopt the field-aware PM merge driver and convert the last JavaScript test helper to TypeScript

Why

The previous importer issued one pm mutation per GitHub issue. A failure or concurrent agent could expose partially imported state, while a reordered retry could not safely resume a durable batch. This change makes the full tracker write atomic, crash-resumable, and workspace-writer-locked without changing the default non-atomic path.

PM context

Validation

  • npm run release:check
  • npm test — 134/134 passing
  • real local-package installation and two consecutive atomic imports of current unbraind/pm-cli issues; the second run recovered the same transaction with no duplicates
  • mixed-batch failure test proves pre-existing updates and closes are snapshot-restored
  • production dependency audit: 0 vulnerabilities
  • npm pack dry-run and changelog check: clean
  • full 110-commit credential-signature scan: 0 findings; current PM records use portable workspace paths
  • Greptile pre-push reviews a895c1d7-69a7-4b83-b679-e2ab9d022b71, e7e6bbcc-8d7f-470f-85fd-ca677873e6ee, and 3f9186b2-a856-47bf-b436-733bdf4056fc; final review: 5/5, safe to merge, no comments

Summary by Sourcery

Add an opt-in atomic mode for GitHub issue imports that uses pm-cli bulk item mutations to ensure crash-resumable, non-partial tracker updates while keeping the legacy import path unchanged.

New Features:

  • Introduce an --atomic flag on the GitHub import commands that commits all issue changes as a single workspace-writer-locked transaction.
  • Expose additional TypeScript types and helpers for GitHub issues, comments, import options, and atomic import configuration.
  • Provide a new atomic import pipeline that derives stable transaction and item identities from GitHub issue content and provenance.

Enhancements:

  • Improve native comment annotation handling so comments are synced only after a successful atomic commit while preserving existing deduplication behavior.
  • Update CLI help text, README usage examples, and option documentation to describe atomic imports and their failure semantics.
  • Raise the minimum pm-cli version and manifest requirement to align with the newer commitItemMutations SDK primitive.

Build:

  • Bump pm-cli and pm-changelog dev/peer dependency versions to match the new atomic import capabilities.

Documentation:

  • Document the new --atomic flag, its behavior, and how it interacts with provenance, retries, and native comment annotations in the README.

Tests:

  • Add a comprehensive atomic import test suite covering transaction identity stability, crash-resumable behavior, rollback guarantees for failed batches, and actionable SDK-version errors.
  • Adjust existing concurrent comment-sync tests to use the new TypeScript helper entrypoint instead of the old JavaScript module.

Summary by cubic

Adds an opt-in --atomic mode to GitHub issue imports that commits all creates/updates/closes in one crash-resumable transaction. Dry-run previews now mirror the atomic plan exactly, and recovery is hardened to avoid collisions across retries.

  • New Features

    • --atomic for both pm github import and pm gh-issues import, backed by commitItemMutations in @unbrained/pm-cli 2026.7.20.
    • Deterministic item IDs and a transaction identity that fingerprints the exact ordered mutation plan and provenance; safe across reordered/partial retries.
    • --dry-run routes through the same rendering and provenance matching as commit for truthful previews (the SDK commit path remains disabled).
    • On failure, creates are rolled back and updates/closes snapshot-restored, with clear reporting for interrupted and compensated transactions; native comment annotations sync only post-commit with cross-process dedupe; README documents --atomic.
  • Migration

    • Requires @unbrained/pm-cli >= 2026.7.20.
    • Default (non-atomic) behavior is unchanged.
    • To use it: run imports with --atomic; --dry-run works as before.

Written for commit 0735f7a. Summary will update on new commits.

Review in cubic

Add opt-in crash-resumable bulk imports using the pm 2026.7.20 commitItemMutations SDK. Canonicalize transaction plans across reordered and partially applied retries, derive IDs from stable GitHub issue keys, and compensate create/update/close mutations on failure.

Adopt the field-aware PM merge driver, upgrade the SDK/changelog toolchain, convert the remaining child-process fixture to TypeScript, document the workflow, add real tracker recovery/rollback coverage, and redact legacy absolute host paths from current PM records.
@sourcery-ai

sourcery-ai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Reviewer's Guide

Adds an opt-in atomic GitHub issue import path using pm-cli’s commitItemMutations, with stable transaction/item identity, robust rollback semantics, and updated docs/tests, while keeping the legacy non-atomic importer behavior unchanged.

Sequence diagram for the new atomic GitHub issue import flow

sequenceDiagram
  actor User
  participant PmCli as pm_github_runImport
  participant Atomic as importGithubAtomic
  participant SDK as pm_cli_sdk_commitItemMutations
  participant Tracker as pm_workspace
  participant Comments as syncGithubCommentsToAnnotations

  User->>PmCli: pm github import owner/repo --atomic
  PmCli->>PmCli: parseImportOptions (atomic=true)
  PmCli->>Atomic: importGithubAtomic(pmRoot, repo, preparedEntries)

  Atomic->>SDK: resolveCommitItemMutations()
  Atomic->>SDK: commitItemMutations({ pmRoot, transactionId, mutations })
  SDK-->>Atomic: CommitItemMutationsResult(recovered?)
  Atomic-->>PmCli: { transactionId, recovered, imported, updated, itemIds }

  loop for each entry with syncAnnotations
    PmCli->>Comments: syncGithubCommentsToAnnotations(itemId, comments, pmRoot, issueNumber)
    Comments->>Tracker: write native comments
  end

  alt result.recovered
    PmCli->>User: report recovered transaction and item count
  else fresh commit
    PmCli->>User: report imported/updated/skipped counts
  end

  opt commitItemMutations failure
    SDK-->>Atomic: error
    Atomic-->>PmCli: CommandError (rolled back)
    Atomic-->>PmCli: no partial state
    PmCli->>User: report atomic failure with transactionId
  end
Loading

File-Level Changes

Change Details Files
Introduce an atomic, crash-resumable GitHub issue import pipeline that uses pm-cli’s bulk mutation SDK primitive.
  • Define new exported types and helpers (PreparedGithubImport, AtomicImportOptions, CommitItemMutations) to describe the rendered import state and atomic options.
  • Implement lazy resolution of commitItemMutations, normalizeItemId, and readSettings with clear version/usage errors and caching.
  • Derive stable transaction ids and item ids from repository and issue content, independent of fetch order, using SHA-256 digests and deterministic canonicalization.
  • Build reversible mutation plans (create/update/close) that are crash-resumable and compatible with partially applied prior attempts via deterministic item ids and provenance matching.
  • Implement importGithubAtomic to assemble ordered bulk mutations, commit them as one transaction with createCompensation=delete, report recovered vs fresh batches accurately, and wrap failures in a rollback-assured CommandError.
index.ts
Wire the new atomic mode into the GitHub import command while preserving the default non-atomic behavior.
  • Extend ImportOptions with an atomic flag and parseImportOptions to recognize --atomic without affecting defaults.
  • Update runImport to branch to the atomic pipeline when --atomic and not dry-run, including preparation of issue entries, comment fetching, and post-commit native comment annotation sync.
  • Adjust dry-run output to include atomic:true when appropriate and update usage strings, flags, and help text for both pm github import and pm gh-issues import commands to document --atomic behavior.
index.ts
.agents/pm/tasks/pm-github-0y40.toon
.agents/pm/features/pm-github-9eei.toon
.agents/pm/history/pm-github-0y40.jsonl
.agents/pm/history/pm-github-9eei.jsonl
.agents/pm/history/pm-github-1jez.jsonl
Update SDK and CLI version requirements to pm-cli 2026.7.20+ and align manifest and lockfile.
  • Bump peerDependency and devDependency versions for @unbrained/pm-cli and pm-changelog to the required minimum supporting commitItemMutations.
  • Raise pm_min_version in manifest.json to 2026.7.20 to match the atomic import SDK requirement.
  • Regenerate package-lock.json to reflect the new dependency versions.
package.json
manifest.json
package-lock.json
Document atomic import semantics and usage in the README.
  • Add --atomic to README’s example commands list.
  • Extend the flags table with a detailed description of --atomic semantics, including workspace-writer locking, rollback behavior, and version requirements.
  • Add prose explaining how transaction ids and item ids are derived for atomic mode and how native comment annotations are applied post-commit with deduplication.
README.md
Expand and refactor tests to cover atomic import behavior and convert the remaining helper to TypeScript.
  • Add atomic.test.ts with integration-style tests that exercise transaction id stability, deterministic item ids and mutation plans, successful atomic imports and resumption, rollback of pure-create batches, mixed create/update/close rollback, and SDK resolution error messaging.
  • Convert comment-sync-child helper from .mjs to TypeScript (.ts), adjusting import paths and usage text accordingly, and update import-lock.test to use the compiled .js helper path.
  • Add new pm history/task artifacts for atomic feature tracking in the PM agent configuration.
test/atomic.test.ts
test/import-lock.test.ts
test/helpers/comment-sync-child.ts
.agents/pm/tasks/pm-github-1jez.toon
.gitattributes

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@unbraind

Copy link
Copy Markdown
Owner Author

@greptileai full review

@coderabbitai full review

@unbraind

Copy link
Copy Markdown
Owner Author

@sourcery-ai review

Please focus on crash-recovery plan determinism, mixed create/update/close compensation, and truthful recovered-batch output. The package uses the official pm 2026.7.20 SDK contract and includes real temporary-tracker rollback tests.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@unbraind, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e7b01c9e-6a7c-47d9-8eec-2f3a9baf65ef

📥 Commits

Reviewing files that changed from the base of the PR and between a433ddd and 0735f7a.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (16)
  • .agents/pm/features/pm-github-9eei.toon
  • .agents/pm/history/pm-github-0y40.jsonl
  • .agents/pm/history/pm-github-1jez.jsonl
  • .agents/pm/history/pm-github-9eei.jsonl
  • .agents/pm/tasks/pm-github-0y40.toon
  • .agents/pm/tasks/pm-github-1jez.toon
  • .gitattributes
  • README.md
  • index.ts
  • manifest.json
  • package.json
  • test/atomic.test.ts
  • test/helpers/barrier.ts
  • test/helpers/comment-sync-child.mjs
  • test/helpers/comment-sync-child.ts
  • test/import-lock.test.ts

Walkthrough

Adds an opt-in --atomic GitHub issue import mode using deterministic, crash-resumable transactions with compensation and comment synchronization. It also adds tests and documentation, raises CLI version requirements, updates PM records, and adjusts the comment-sync test helper.

Changes

Atomic GitHub import workflow

Layer / File(s) Summary
Atomic import contracts and execution
index.ts, manifest.json, package.json
Adds atomic import types, deterministic transaction and item IDs, reversible mutation planning, SDK resolution, compensation, recovery reporting, CLI parsing, help text, and the required pm-cli version floor.
Atomic behavior and concurrency validation
test/atomic.test.ts, test/helpers/comment-sync-child.ts, test/import-lock.test.ts
Tests deterministic planning, resume behavior, update/close transitions, rollback of creates and existing items, SDK errors, and concurrent comment synchronization.
Atomic import documentation
README.md, .agents/pm/tasks/pm-github-1jez.toon
Documents the atomic flag, transaction identity, resume behavior, comment synchronization, acceptance criteria, and verification records.
Release and tracker records
.agents/pm/**, .gitattributes
Updates workspace paths, verification history, lifecycle records, redaction events, atomic implementation history, and PM-specific merge drivers.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runImport
  participant importGithubAtomic
  participant commitItemMutations
  participant syncGithubCommentsToAnnotations
  runImport->>importGithubAtomic: prepared GitHub issue entries
  importGithubAtomic->>commitItemMutations: deterministic mutation journal
  commitItemMutations-->>importGithubAtomic: transaction and recovery result
  importGithubAtomic-->>runImport: imported item IDs and counts
  runImport->>syncGithubCommentsToAnnotations: native comments for committed items
Loading

Possibly related PRs

  • unbraind/pm-github#33: Adds the native comment synchronization path used by the atomic import flow and tests.
  • unbraind/pm-github#4: Relates to the concurrent comment-sync test helper and marker deduplication behavior.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: atomic, crash-resumable GitHub issue imports.
Description check ✅ Passed The description is clearly related and accurately summarizes the atomic import changes and supporting updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/atomic-github-import

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

@unbraind: I’ll perform a full review of #5, with particular attention to deterministic recovery plans, mixed-operation compensation, recovered-batch reporting, and post-commit comment annotation behavior.

✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 58 minutes.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 4 issues, and left some high level feedback:

  • The atomic import path in runImport duplicates much of the existing per-issue preparation logic (title/description/tags/comments handling); consider extracting a shared helper to reduce divergent behavior between atomic and non-atomic imports.
  • SDK resolution and error handling for commitItemMutations, normalizeItemId, and readSettings are implemented in multiple places; centralizing this into a single utility would keep the version/usage messaging consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The atomic import path in `runImport` duplicates much of the existing per-issue preparation logic (title/description/tags/comments handling); consider extracting a shared helper to reduce divergent behavior between atomic and non-atomic imports.
- SDK resolution and error handling for `commitItemMutations`, `normalizeItemId`, and `readSettings` are implemented in multiple places; centralizing this into a single utility would keep the version/usage messaging consistent and easier to maintain.

## Individual Comments

### Comment 1
<location path="index.ts" line_range="1610-1619" />
<code_context>
+  if (opts.atomic && !opts.dryRun) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Atomic dry-run mode currently falls back to legacy import semantics, which may diverge from the atomic path.

Because of the `opts.atomic && !opts.dryRun` guard, `--atomic --dry-run` still uses the legacy per-issue import path instead of the atomic batching logic, so its counts and behavior won’t match a real atomic run. If `--atomic --dry-run` is meant to preview the atomic plan, it should go through the same preparation/matching flow (just skipping `commitItemMutations`) so the preview reflects true atomic behavior.

Suggested implementation:

```typescript
  if (opts.atomic) {

```

To fully implement the suggested behavior (atomic dry-run goes through the same matching/preparation flow but skips mutations), you will also need to:
1. Locate the atomic-path commit logic (likely where `commitItemMutations` or equivalent is called for the `prepared` batch) and wrap that call in a `if (!opts.dryRun) { ... }` guard so dry-run does not actually mutate anything.
2. Ensure that any reporting of counts or plans in the atomic path is executed regardless of `opts.dryRun`, so `--atomic --dry-run` prints the same plan and statistics as a real atomic run, just without committing.
3. Verify that the legacy per-issue import path is only used when `!opts.atomic`, so that `--atomic --dry-run` no longer falls back to the legacy path.
</issue_to_address>

### Comment 2
<location path="test/atomic.test.ts" line_range="77-80" />
<code_context>
+  return spawnSync(PM_BIN, ["--path", root, "validate"], PM_SPAWN_OPTS).status === 0;
+}
+
+test("parseImportOptions enables atomic mode without changing the default", () => {
+  assert.strictEqual(parseImportOptions({}).atomic, false);
+  assert.strictEqual(parseImportOptions({ atomic: true }).atomic, true);
+  assert.strictEqual(parseImportOptions({ atomic: "1" }).atomic, true);
+});
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for CLI dry-run/atomic behavior to ensure no SDK access and correct result shape

The current test only covers `atomic` flag parsing. Please also add an integration-style test for `runImport` with `--atomic --dry-run` (e.g., `pm github import ... --atomic --dry-run`) that asserts:
- successful command exit,
- dry-run summary output is correct, and
- the structured output includes `atomic: true` and does not invoke the pm SDK.
This will validate the new short-circuit path and avoid unintentionally requiring a newer CLI in dry-run mode.

Suggested implementation:

```typescript
function validateOk(root: string): boolean {
  return spawnSync(PM_BIN, ["--path", root, "validate"], PM_SPAWN_OPTS).status === 0;
}


import {
  CommandError,
  EXIT_CODE,
  buildAtomicImportMutations,
  deriveAtomicItemId,
  deriveAtomicTransactionId,
  importGithubAtomic,
  parseImportOptions,
  resolveCommitItemMutations,
} from "../dist/index.js";

test("parseImportOptions enables atomic mode without changing the default", () => {
  assert.strictEqual(parseImportOptions({}).atomic, false);
  assert.strictEqual(parseImportOptions({ atomic: true }).atomic, true);
  assert.strictEqual(parseImportOptions({ atomic: "1" }).atomic, true);
});

test("CLI dry-run with atomic mode short-circuits, returns correct summary, and does not invoke SDK", () => {
  // Run the CLI in atomic + dry-run mode and request structured JSON output.
  // The path used here should point at a minimal valid project path for the tests.
  const root = PM_FIXTURE_ROOT; // reuse whatever root the other atomic tests use

  const result = spawnSync(
    PM_BIN,
    [
      "--path",
      root,
      "github",
      "import",
      "--atomic",
      "--dry-run",
      "--json",
    ],
    PM_SPAWN_OPTS,
  );

  // 1. successful command exit
  assert.strictEqual(
    result.status,
    0,
    `expected pm github import --atomic --dry-run to exit successfully, got ${result.status}`,
  );

  // 2. dry-run summary output is correct (human-readable on stdout)
  const stdout = result.stdout.toString("utf8");
  assert.ok(
    /dry[- ]run summary/i.test(stdout),
    `expected dry-run summary in CLI output, got: ${stdout}`,
  );

  // 3. structured output includes atomic: true
  const jsonLine = stdout
    .split("\n")
    .map((line) => line.trim())
    .filter((line) => line.startsWith("{") && line.endsWith("}"))
    .pop();

  assert.ok(jsonLine, "expected a JSON structured output line from CLI dry-run");

  const structured = JSON.parse(jsonLine);
  assert.strictEqual(structured.atomic, true, "expected structured output.atomic === true");
  assert.strictEqual(
    structured.dryRun,
    true,
    "expected structured output.dryRun === true for --dry-run",
  );

  // 4. ensure the pm SDK is not invoked on dry-run/atomic short-circuit path
  // In this test we assert that no SDK-level mutations are present, which indicates
  // that the CLI short-circuited before talking to the SDK.
  assert.ok(
    !structured.mutations || structured.mutations.length === 0,
    "expected no SDK mutations in atomic dry-run structured output",
  );
});

```

1. Replace `PM_FIXTURE_ROOT` with the appropriate fixture path or variable already used by the other atomic tests (e.g., a repository path that makes `pm github import` valid in your test environment).
2. If the CLI does not use `--json` for structured output, adjust the arguments and the parsing logic to match the actual flag and output format (some CLIs use `--output json` or print JSON to stderr instead of stdout).
3. If the structured output uses different property names (for example `isAtomic`, `dry_run`, or nests fields under `result`), update the assertions on `structured.atomic`, `structured.dryRun`, and `structured.mutations` accordingly.
4. To make the “does not invoke the pm SDK” guarantee stronger, you can additionally expose a debug counter or spy mechanism from the SDK side (e.g., incrementing a counter when SDK import is invoked) and assert that it remains zero when running with `--atomic --dry-run`. That hook will need to be wired into the CLI code and imported in this test file.
</issue_to_address>

### Comment 3
<location path="test/helpers/comment-sync-child.ts" line_range="19-28" />
<code_context>
-  process.exit(2);
-}
-
-const barrier = process.env.BARRIER_FILE;
-if (barrier) {
-  const deadline = Date.now() + 10_000;
-  while (!existsSync(barrier)) {
-    if (Date.now() > deadline) {
-      console.error("barrier file never appeared");
-      process.exit(2);
-    }
-    await delay(5);
-  }
-}
-
-const comments = JSON.parse(process.env.FAKE_COMMENTS || "[]");
-const result = await syncGithubCommentsToAnnotations(itemId, comments, pmRoot, 1);
-process.stdout.write(`${JSON.stringify(result)}\n`);
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the barrier wait/timeout behavior in the concurrent comment-sync child helper

This barrier logic is central to the concurrent lock-serialization test but is currently untested. Please add or extend a test that runs the child with `BARRIER_FILE` set and covers both paths:
- barrier file appears before the deadline and the child proceeds; and
- barrier file never appears, leading to the expected error message and exit status.
That way, changes to the wait loop or timeout behavior are less likely to introduce hangs or mis-detected failures in the concurrency tests.

Suggested implementation:

```typescript
import { existsSync } from "node:fs";
import { setTimeout as delay } from "node:timers/promises";

import { syncGithubCommentsToAnnotations } from "../../dist/index.js";
import type { GhComment } from "../../dist/index.js";

```

```typescript
export async function waitForBarrier(
  barrierPath: string,
  {
    now = () => Date.now(),
    delayMs = (ms: number) => delay(ms),
    deadlineMs = 10_000,
    exists = existsSync,
    logError = (msg: string) => console.error(msg),
    exit = (code: number) => process.exit(code),
  } = {}
): Promise<void> {
  const deadline = now() + deadlineMs;

  // Tight loop with short sleep to keep the barrier wait predictable and testable.
  while (!exists(barrierPath)) {
    if (now() > deadline) {
      logError("barrier file never appeared");
      exit(2);
      return; // for testability / type narrowing; process.exit will terminate in real child
    }
    await delayMs(5);
  }
}

async function main() {
  const [itemId, pmRoot] = process.argv.slice(2);
  if (!itemId || !pmRoot) {
    console.error("usage: comment-sync-child.js <itemId> <pmRoot>");
    process.exit(2);
  }

  const barrier = process.env.BARRIER_FILE;
  if (barrier) {
    await waitForBarrier(barrier);
  }

  const comments = JSON.parse(process.env.FAKE_COMMENTS || "[]") as GhComment[];
  const result = await syncGithubCommentsToAnnotations(itemId, comments, pmRoot, 1);
  process.stdout.write(`${JSON.stringify(result)}\n`);
}

void main();

```

To fully implement the requested coverage:
1. Add a new test (or extend an existing one) that imports `waitForBarrier` from `test/helpers/comment-sync-child.ts` and verifies both behaviors by passing test doubles for `exists`, `now`, `delayMs`, `logError`, and `exit`:
   - Case 1: `exists` returns `false` until after some iterations but before `deadlineMs`, ensuring `waitForBarrier` resolves without calling `exit` and without logging an error.
   - Case 2: `exists` always returns `false` and `now` advances beyond `deadlineMs`, asserting that `logError` is called with `"barrier file never appeared"` and `exit` is called with code `2`.
2. Optionally, add integration-style tests that actually spawn the `comment-sync-child` helper with `BARRIER_FILE` set:
   - For the success path, create the barrier file from the parent process and assert the child exits successfully.
   - For the timeout path, do not create the barrier file and assert the child exits with status `2` and the expected stderr output.
These tests should live in the existing concurrency test suite that uses this helper, reusing the current test harness for spawning child processes.
</issue_to_address>

### Comment 4
<location path="README.md" line_range="57" />
<code_context>
 | `--skip-drafts` | boolean | Exclude draft pull requests (only meaningful with `--include-prs`) |
 | `--with-comments` | boolean | Fetch issue comments and append them to the item body |
 | `--comments-mode <mode>` | `body`\|`annotations`\|`both` | How fetched GitHub comments are persisted (default `body`). `annotations` syncs comments into the pm item's native comments collection via the SDK; `both` writes the body section AND native comments. `annotations`/`both` are idempotent on re-import (dedupe by GitHub comment id) |
+| `--atomic` | boolean | Commit every create, update, close, and reopen in one workspace-writer-locked, crash-resumable transaction (requires pm CLI/SDK >=2026.7.20). A failed batch restores all updated/closed items and deletes all creates, so agents never observe partial import state. |
 | `--dry-run` | boolean | Preview without writing |
 | `--type <type>` | string | Override pm item type (default: Issue) |
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing "deletes all creates" for clearer grammar.

"Creates" as a noun is unusual and could be misread. Consider alternatives like "deletes all created items" or "removes all newly created items" to keep the meaning clear.

```suggestion
| `--atomic` | boolean | Commit every create, update, close, and reopen in one workspace-writer-locked, crash-resumable transaction (requires pm CLI/SDK >=2026.7.20). A failed batch restores all updated/closed items and deletes all newly created items, so agents never observe partial import state. |
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread index.ts Outdated
Comment thread test/atomic.test.ts
Comment thread test/helpers/comment-sync-child.ts Outdated
Comment thread README.md Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an opt-in --atomic flag for the pm github import and legacy gh-issues import commands, enabling crash-resumable, transaction-backed imports using the @unbrained/pm-cli/sdk commitItemMutations primitive. It includes stable ID derivation, transaction planning, and rollback compensation to prevent partial tracker states. Additionally, absolute host paths in task logs and test files have been redacted to /workspace/pm-github, and comprehensive unit tests have been added to verify the atomic behavior and recovery. As there are no review comments provided, I have no further feedback to offer.

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-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds atomic, crash-resumable GitHub issue imports. The main changes are:

  • Adds --atomic to both GitHub import commands.
  • Builds stable transaction IDs from ordered mutation plans.
  • Restores state after failed batches and reports incomplete compensation.
  • Syncs native comment annotations only after commit.
  • Updates SDK requirements, documentation, and tests.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the updated transaction identity, empty-plan handling, or compensation reporting.

T-Rex T-Rex Logs

What T-Rex did

  • The run completed with exit code 0 after performing the TypeScript builds and the verbose focused test suite.
  • The tracker-backed recovery and rollback tests ran in temporary workspaces using the real local pm binary, with no GitHub credentials or external services involved.
  • All work was validated under the general contract scope, confirming no external dependencies were invoked.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
index.ts Adds atomic import planning, stable transaction identity, empty-plan failure handling, and accurate compensation reporting.
test/atomic.test.ts Covers changed mutation targets, wholly skipped imports, transaction recovery, and incomplete compensation.
package.json Raises the pm SDK dependency floor for atomic mutation support.

Reviews (4): Last reviewed commit: "fix(import): harden atomic recovery edge..." | Re-trigger Greptile

Comment thread index.ts
Comment thread index.ts Outdated
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds atomic, crash-resumable GitHub issue imports. The main changes are:

  • Stable transaction and item identities for bulk imports.
  • SDK-backed create, update, close, and rollback handling.
  • Post-commit native comment synchronization.
  • New CLI flags, documentation, dependency floors, and tests.
  • TypeScript conversion of the concurrent comment-sync helper.

Confidence Score: 4/5

Transaction recovery can fail after an item-prefix fallback, and incomplete compensation can be reported as a clean rollback.

Normal commits, reordered retries, and mixed rollback paths have strong test coverage.

The transaction identity does not include the prefix used to derive mutation targets. The failure path promises complete compensation for every SDK rejection.

T-Rex T-Rex Logs

What T-Rex did

  • Reproduced the transaction identity behavior by running the compiled importer with the local pm-cli SDK in a temporary workspace, confirming the transaction ID remained constant while mutation targets shifted.
  • Executed the atomic-import incomplete-compensation harness to exercise the compensation path and observed a CommandError that included COMPENSATION_INCOMPLETE while implying full compensation and no partial state.
  • Observed an initial run that created two items in a transaction, followed by a reordered retry that recovered the same transaction with zero imports/updates, two recovered items, and two total item counts with unique IDs.
  • Cross-validated the three repros to confirm consistent transaction identity behavior and consistent reporting of mutation counts and recovery outcomes.
  • Collected and organized the artifacts from all proofs to support reviewer inspection.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
index.ts Adds the atomic import pipeline; transaction recovery can break when the resolved item prefix changes, and failure reporting assumes compensation always completes.
test/atomic.test.ts Covers stable ordering, successful recovery, create rollback, and restoration of updated and closed items.
test/import-lock.test.ts Updates the child-process fixture path for the compiled TypeScript helper.
test/helpers/comment-sync-child.ts Replaces the JavaScript concurrency fixture with an equivalent typed helper.
package.json Raises dependency versions to the SDK release that provides atomic item mutations.
manifest.json Raises the minimum host version to match the atomic SDK requirement.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as GitHub import
participant GH as GitHub API
participant SDK as pm transaction SDK
participant WS as Workspace
CLI->>GH: Fetch and render issues
CLI->>CLI: Derive transaction and item IDs
CLI->>SDK: Commit ordered mutation plan
SDK->>WS: Apply mutations under writer lock
alt Commit or recovery succeeds
    SDK-->>CLI: Committed result
    CLI->>WS: Sync native comments
else Mutation fails
    SDK->>WS: Compensate applied steps
    SDK-->>CLI: Error
end
Loading
%%{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 CLI as GitHub import
participant GH as GitHub API
participant SDK as pm transaction SDK
participant WS as Workspace
CLI->>GH: Fetch and render issues
CLI->>CLI: Derive transaction and item IDs
CLI->>SDK: Commit ordered mutation plan
SDK->>WS: Apply mutations under writer lock
alt Commit or recovery succeeds
    SDK-->>CLI: Committed result
    CLI->>WS: Sync native comments
else Mutation fails
    SDK->>WS: Compensate applied steps
    SDK-->>CLI: Error
end
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
index.ts:729-754
**Transaction Omits Item-ID Prefix**

When workspace settings temporarily fall back to `pm-` and later return a custom `id_prefix`, identical issues keep the same transaction ID but produce different mutation target IDs. The SDK requires an exact ordered plan to resume a journal, so the retry is rejected instead of recovering the interrupted import.

### Issue 2 of 2
index.ts:933-939
**Failed Compensation Reports Clean State**

If `commitItemMutations` rejects because compensation itself could not finish, this catch still states that every mutation was compensated and no partial state remains. Operators can therefore treat a workspace with partially applied creates or updates as clean instead of retrying or repairing the transaction.

Reviews (2): Last reviewed commit: "feat(import): make GitHub issue sync ato..." | Re-trigger Greptile

Comment thread index.ts
Comment thread index.ts
unbraind added 2 commits July 21, 2026 09:41
Route atomic previews through the same issue rendering and provenance matching used by committed imports, while keeping the SDK commit path unreachable during dry-run. Centralize atomic SDK resolution so version errors and lazy loading remain consistent.

Add orchestration coverage for create/update/skip summaries, deterministic barrier success and timeout tests, clearer rollback documentation, and durable pm review evidence. The full release gate passes with 137 tests.
Fingerprint the exact ordered mutation plan in transaction identities so changed prefixes or provenance targets cannot collide with an incompatible recovery journal. Preserve legacy failure semantics when every issue is skipped.

Report interrupted, normally compensated, and incompletely compensated transactions truthfully; document the operator recovery contract and add focused regressions. The full release gate passes with 139 tests.
@unbraind

Copy link
Copy Markdown
Owner Author

Review feedback ledger for the completed round:

  • @sourcery-ai: all four inline findings were accepted and fixed in 5e66c6f/0735f7a; each thread now has a commit-specific reply and is resolved. Shared preparation and SDK loading were also consolidated as suggested.
  • @greptileai: the mutation-target/prefix reports (duplicates of the same root cause), all-skipped behavior, and incomplete-compensation wording were fixed in 0735f7a; each thread has a focused regression and direct reply. Local pre-push review 10a0037e-7eee-41a4-a11a-32929e410c2d completed 5/5 with no comments on the final branch.
  • @gemini-code-assist: the no-finding review was read and positively acknowledged; the subsequent review fixes preserve the behavior it summarized and the full release gate is still green.
  • @coderabbitai: the completed review/trigger acknowledgement and updated walkthrough were read and positively acknowledged. It posted no actionable inline code comments. The docstring-coverage warning is not a repository gate; the new public atomic helpers retain TSDoc and the internal/test seams have explanatory comments. The automatic follow-up attempt responded with a temporary plan rate limit, so an explicit retrigger follows below.

Verification after every accepted finding: 139/139 tests, typecheck, zero production audit vulnerabilities, package dry-run, changelog check, and pm history validation all pass.

@unbraind

Copy link
Copy Markdown
Owner Author

@greptileai full review

@coderabbitai full review

@sourcery-ai review

Please perform the required full follow-up review of commits 5e66c6f and 0735f7a. Focus on atomic dry-run parity/no-SDK behavior, exact mutation-plan transaction identity, all-skipped failure parity, incomplete compensation reporting, and barrier timeout coverage.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@unbraind Thanks for the thorough ledger. Noted that the accepted findings are covered by focused regressions in 5e66c6f and 0735f7a, and that the complete release gate is green. The reported atomic-import invariants and verification results provide solid context for any subsequent review round.

🐇 ✅

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

@unbraind: Starting a full follow-up review of #5, focused on the specified commits and atomic-import recovery/compensation edge cases.

✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 26 minutes.

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Ledger confirmed. All eight threads are resolved with commit-specific replies, and the state matches what I see:

  • 4 Sourcery findings (atomic dry-run path, dry-run orchestration test, barrier wait/timeout coverage, README wording) — all resolved in 5e66c6f.
  • 4 Greptile findings (transaction ID omits mutation targets × 2 root-cause duplicates, all-skipped exit behavior, incomplete-compensation wording) — all resolved in 0735f7a, with focused regressions for each.

Test count grew from 134 → 139 across the two fix commits, gating passes clean, and no open threads remain. This round is complete on my end. The PR is clear to merge.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@unbraind

Copy link
Copy Markdown
Owner Author

@coderabbitai Follow-up acknowledgement: I read and positively reacted to both new replies. The ledger acknowledgement confirms the regression evidence was useful; the requested full review was accepted but the external plan quota reports a 26-minute availability delay. No CodeRabbit code finding is outstanding, and I will not treat the rate-limit response as a code approval or hide the failed check.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@unbraind Thanks for the precise clarification. Agreed: the quota delay is an operational limitation, not a completed follow-up review or an approval. There are no outstanding CodeRabbit findings from the completed round, and the deferred full review remains distinct from that status.

@unbraind

Copy link
Copy Markdown
Owner Author

@greptileai @coderabbitai Final acknowledgement for the latest replies: both were read and positively reacted to. Greptile confirms all eight threads resolved, 139 tests, clean gates, and clear-to-merge status. CodeRabbit confirms there are no outstanding findings from its completed round while correctly keeping the quota-deferred follow-up distinct from approval. I am continuing to hold the merge solely for that external red quota check.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@unbraind Acknowledged. Your merge hold remains appropriate: CodeRabbit has no outstanding findings from its completed review, but the quota-deferred full follow-up has not run and must not be interpreted as approval or a passing review.

🐇 ✅

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Acknowledged. All eight threads are resolved from my side, and my clear-to-merge assessment stands. The external red quota check is the only remaining gate — once that clears, you're good to go.

@unbraind

Copy link
Copy Markdown
Owner Author

Final bot-response acknowledgement before the review timeout: the latest CodeRabbit and Greptile replies were both read and positively reacted to. They agree on the exact state—no outstanding findings, all eight threads resolved, Greptile clear to merge, and the quota-deferred CodeRabbit follow-up is not approval. The 20-minute maximum wait has now elapsed with that external check still red, so the PR remains intentionally unmerged.

@unbraind

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Independent post-implementation verification (orchestrator hand-off check).

Beyond the automated release gate, I ran a real-data functional test of the atomic path against unbraind/pm-changelog (16 live issues) in a throwaway workspace on the built dist/index.js:

  1. First --atomic importAtomically imported 16 new under one transaction (github-import-f4b1c13b…); 16 items created.
  2. Re-run (idempotence)recovered: true, imported: 0, recoveredItems: 16, zero duplicates (still exactly 16 items). The stable transaction id + external-issue-keyed item ids resume correctly.
  3. Default non-atomic import of the same repo in a fresh workspace → identical 16 items, confirming byte-compatible default behavior.
  4. Durable journal persisted at .agents/pm/transactions (SDK-managed).
  5. Full release gate green: 139/139 tests, typecheck, prod audit (0 vulns), pack dry-run, changelog:check all pass. SDK exports commitItemMutations/normalizeItemId/readSettings confirmed present at runtime.

Please perform the quota-deferred follow-up review now (the earlier round reported only a ~26-minute availability delay, not a finding). All eight prior Sourcery/Greptile threads remain resolved with commit-specific replies, Greptile's re-review is clear-to-merge, and Gemini reported no findings.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

@unbraind: Starting the deferred full follow-up review of #5. I’ll independently assess the atomic import path, with particular attention to deterministic transaction plans, retry/recovery reporting, mixed-operation compensation, dry-run/default-path parity, and comment-sync behavior, using the provided live-workspace verification as supporting context.

✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 13 minutes.

@unbraind

Copy link
Copy Markdown
Owner Author

Final review-loop resolution — merging

@coderabbitai's walkthrough now reports "Review limit reached … you've reached your PR review limit, so we couldn't start this review." This is a free-tier quota cap, not a code finding — the deferred full follow-up physically cannot run, and CodeRabbit's completed pre-limit round left zero outstanding findings. Its red status check is CodeRabbit's own advisory context and is not a required status check on main (branch protection: required_status_checks: none).

All actual review gates are satisfied:

  • Sourcery — 4/4 findings fixed in 5e66c6f, threads resolved, re-review clean ✅
  • Greptile — 4/4 P1 findings fixed in 0735f7a, re-review clear-to-merge, check SUCCESS ✅
  • Gemini — no findings ✅ (consumer review sunset 2026-07-17)
  • CI / semgrep / cubic — green/neutral ✅
  • Orchestrator functional verification — real-data atomic import of 16 live issues, idempotent resume with 0 duplicates, byte-compatible default path, 139/139 tests, 0 prod vulns ✅

All eight review threads are resolved, every bot comment has been read and reacted to. The 20-minute maximum re-review window has elapsed with CodeRabbit's limit still exhausted. Proceeding to merge.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes.

@unbraind
unbraind merged commit c1a3c76 into main Jul 21, 2026
5 of 6 checks passed
@unbraind
unbraind deleted the feat/atomic-github-import branch July 21, 2026 12:06
unbraind added a commit that referenced this pull request Jul 21, 2026
…2026.7.21 (#6)

The .gitattributes merge-driver fence adopted in PR #5 is inert without the
per-clone merge.<name>.driver git config. Add a guarded `prepare`
(`if command -v pm; then pm merge install; fi`) so npm install/ci wires it
automatically when pm is present and skips cleanly otherwise (e.g. --omit=dev);
plus a merge:install script and a README note. Bump @unbrained/pm-cli devDep
to ^2026.7.21 (driver requires 2026.7.20+) and sync the lockfile.

Verified: with config unset, npm ci restores merge.pm-item-toon.driver;
typecheck/test/changelog:check green.

Co-authored-by: SteveBot <1153461+unbraind@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant