feat(workflow): add provider-neutral Issue/PR models and GitHub integration - #100
Conversation
openPullRequest previously took a free-form body string, bypassing renderPullRequestBody's policy-driven section rendering at the adapter boundary. Route it through PullRequestBodyDraft + policy instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds provider-neutral issue and pull-request models, policy-driven pull-request rendering, GitHub workflow operations, and SQLite persistence for pull-request records. ChangesPull request contracts and rendering
Pull request persistence
GitHub workflow integration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as openWorkflowPullRequest
participant Adapter as GithubAdapter
participant GitHub as gh CLI
participant Store as WorkflowSqliteStateStore
participant TaskState as Task state
Workflow->>Store: Check existing pull-request record
Workflow->>Adapter: Validate and create pull request
Adapter->>GitHub: Run gh pr create
GitHub-->>Adapter: Return pull-request metadata
Adapter-->>Workflow: Return provider pull request
Workflow->>Store: Persist pull-request metadata
Workflow->>TaskState: Transition task to pull-request-open
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
yohnark
left a comment
yohnark
left a comment
There was a problem hiding this comment.
Review — blocking issues found
最新の CI / Governance 成功は確認済みです。provider mutation と local lifecycle の境界として見ると、merge 前に潰すべき問題があります。
[BLOCKING] RepositoryIdentity を owner/name に解決できない場合、別 repository に PR を作成し得る
RepositoryIdentity は provider / id だけで成立し、namespace / name は optional です。ところが repositoryArgument() は namespace + name または / を含む id の場合しか owner/name を返しません。
openPullRequest() は GitHub provider + non-empty id までは検証しますが、repositoryArgument() が undefined でも失敗せず、そのまま gh pr create を実行し、--repo を付けません。
つまり { provider: "github", id: "opaque-node-id" } のような provider-neutral identity が入ると、要求した repository ではなく workspaceRoot/cwd が指す repository に mutation が落ちる可能性があります。read ならまだしも、PR creation で暗黙 target fallback は不可です。
修正案: mutation では explicit owner/name が解決できなければ fail closed にするか、--repo を省略する場合は cwd repository identity を取得して requested identity と一致することを厳密に検証してください。
必須テスト: opaque repository ID + Git repo cwd の組み合わせで gh pr create が呼ばれず invalid-input になること。
[BLOCKING] PR record 書込み後に task transition が失敗すると、retry で状態不整合を修復できない
openWorkflowPullRequest() は provider success 後に:
recordPullRequest()transitionTask(..., "pull-request-open")
の順で 2 つの local mutation を行っています。
1 が成功して 2 が失敗した場合、PR record は残ったまま local-state-write-failed を返します。次回 retry では冒頭の existingRecords[0] に入り、task lifecycle を確認・遷移せず reused: true の success を返します。結果として PR record は存在するのに task は pushed 等のまま、という不整合が恒久化できます。
provider と SQLite を globally atomic にできない点とは別問題で、ここは SQLite/local state 同士 なので修復可能です。
修正案: record + lifecycle transition を 1 transaction にまとめるか、existing record reuse path で lifecycle を検査して、未遷移なら idempotently transition/reconcile してください。
必須テスト: record 成功後に transition failure を注入 → retry → task が pull-request-open に収束すること。
[MAJOR] closing Issue parser が通常の英文を closing reference と誤認する
closingIssues() は close/fix/resolve + 次の token を無条件に closing Issue と数えています。したがって required section に Fix parser behavior や Resolve ambiguity があるだけで parser / ambiguity が closing Issue として検出されます。
closingIssue: "exactly-one" なら、本物の Closes #36 と合わせて 2 件扱いになり valid body を reject できます。none でも普通の prose を reject します。
修正案: GitHub が closing reference として解釈する形式（少なくとも #123, owner/repo#123, 対応 URL）に限定して parse してください。
必須テスト: Fix parser behavior は 0 件、Fixes #123 は 1 件になること。
[MAJOR] Issue parser が assignees/milestone を扱うのに gh request が取得していない
parseGithubIssueOutput() と IssueMetadata は assignees / milestone を持っていますが、viewIssue() の --json は number,title,state,labels,url,repository のみです。実 provider では常に欠落します。
現在の test fixture は mock stdout に assignees を含める一方、呼び出し args を検証していないため、この不整合を見逃しています。--json field list と model/parser contract を一致させ、args も test してください。
全体の provider-neutral shape / non-throwing parse / structured body render の方向は良いです。上の 2 blocking は mutation target と state machine の整合性に関わるため、修正後に再レビューしたいです。
… transitions, and tighten closing-Issue parsing Addresses review feedback on PR #100: - openPullRequest now requires an explicit owner/name for the target repository and refuses to run `gh pr create` when repositoryArgument() cannot resolve one, instead of silently falling back to gh's cwd-derived repository for a mutation. - openWorkflowPullRequest's existing-record reuse path now reconciles a task still stuck at `pushed` (record written, transitionTask previously failed) to `pull-request-open` on retry, instead of returning reused success without checking lifecycle state. - pr-render.ts's closingIssues() now requires a real GitHub closing reference (#123, owner/repo#123, or an issues URL) instead of matching any token after close/fix/resolve, which previously misdetected closing Issues inside ordinary prose section headings. - gh issue view --json now requests assignees and milestone, matching what parseGithubIssueOutput/IssueMetadata already parse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
レビュー対応完了、4件とも修正・テスト追加 (5fc0b3b): [BLOCKING] repository fallback: [BLOCKING] record/transition 不整合: [MAJOR] closing Issue 誤検出: [MAJOR] assignees/milestone 欠落:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/workflow/providers/github.ts (2)
158-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the shadowing between the
fallbackRepositoryhelper and the parameter.Line 158 declares a module-level
fallbackRepositoryhelper. Line 186 declares a parameter with the same name inparseGithubIssueOutput, which shadows it. Lines 197-198 then inline{ provider: GITHUB_PROVIDER, id: "unknown" }, which is the exact value the helper produces. A reader at Line 198 cannot tell which binding is in scope, and the default identity is now expressed in two places.Rename the parameter and call the helper.
♻️ Proposed fix
export function parseGithubIssueOutput( stdout: string, - fallbackRepository?: RepositoryIdentity, + defaultRepository?: RepositoryIdentity, ): { ok: true; issue: Issue } | { ok: false; reason: string } { @@ - const repository = repositoryFromGithub(parsed.value.repository) ?? - fallbackRepository ?? { provider: GITHUB_PROVIDER, id: "unknown" }; + const repository = + repositoryFromGithub(parsed.value.repository) ?? fallbackRepository(defaultRepository);
parseGithubPullRequestRecordat Line 225 uses the same parameter name. Rename it there as well for consistency.Also applies to: 184-198
🤖 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 `@src/workflow/providers/github.ts` around lines 158 - 160, Rename the shadowing repository parameter in parseGithubIssueOutput and parseGithubPullRequestRecord, then replace the inline unknown identity fallback in parseGithubIssueOutput with the module-level fallbackRepository helper. Preserve the existing parameter behavior while ensuring both functions consistently use the helper without duplicate fallback values.
752-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe pull-request body is rendered twice per workflow call.
openWorkflowPullRequestcallsrenderPullRequestBodyat Line 752. It then callsadapter.openPullRequest, which callsrenderPullRequestBodyagain at Line 608 with the samedraftand the samepolicy. The workflow returns its ownrendered.bodyat Line 828, butghreceives the body produced by the adapter's render.Both renders are pure and take identical inputs, so they agree today. The duplication still splits one responsibility across two layers. Any future render input that the workflow resolves but does not forward, such as a repository template, would make the returned
renderedBodydiffer from the body that GitHub received.Consider letting the workflow pass its already-rendered body to the adapter while the adapter keeps rendering for direct callers.
♻️ Sketch of the contract change
export interface GithubCreatePullRequestInput { repository: RepositoryIdentity; title: string; head: RevisionIdentity; base: RevisionIdentity; draft: PullRequestBodyDraft; policy?: PullRequestRenderPolicy | WorkflowPolicyDocument["pullRequest"]; providerDraft?: boolean; + /** workflow 側で検証済みの body。再 render による差異を避けるため、あればそのまま使う。 */ + renderedBody?: string; }Then in
openPullRequest, skip the render wheninput.renderedBodyis present, and inopenWorkflowPullRequest, passrenderedBody: rendered.body.🤖 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 `@src/workflow/providers/github.ts` around lines 752 - 776, Update the contract between openWorkflowPullRequest and openPullRequest so the workflow passes its already-rendered body via a renderedBody input. In openPullRequest, use renderedBody when provided and retain the existing renderPullRequestBody fallback for direct callers; update openWorkflowPullRequest to pass rendered.body to ensure the submitted and returned bodies match.src/workflow/providers/model.ts (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider narrowing
numberandurlonPullRequest.
PullRequestinheritsreference,number?,title?, andurl?fromIssueReference. Every construction site insrc/workflow/providers/github.tssetsnumberandurl. Because the type keeps them optional,openWorkflowPullRequestmust add a runtime guard at Lines 780-791 and map that guard tolocal-state-write-failed, which is a misleading reason for a provider-shape problem.Redeclaring both fields as required moves the check to the type system and lets the adapter parsers stay the single validation point.
♻️ Proposed narrowing
export interface PullRequest extends IssueReference { identity: ProviderEntityIdentity; + number: number; + url: string; state: PullRequestState; lifecycleState: PullRequestLifecycleState; repository: RepositoryIdentity; head: RevisionIdentity; base: RevisionIdentity; }🤖 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 `@src/workflow/providers/model.ts` around lines 58 - 65, Redeclare number and url as required fields on the PullRequest interface, overriding their optional definitions inherited from IssueReference. Update openWorkflowPullRequest to remove the corresponding runtime guard and local-state-write-failed mapping, while leaving provider adapter validation responsible for constructing valid PullRequest values.src/workflow/providers/github.test.ts (1)
216-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
head-sha-requiredandlifecycle-blockedfailure reasons.
WorkflowPullRequestFailureReasoninsrc/workflow/providers/github.tsdeclares six values. These tests coverprovider-failedand the success paths. Two reasons stay untested:
lifecycle-blockedat Lines 748-751, reached when the task lifecycle state does not permitpull-request-open.head-sha-requiredat Lines 760-766, reached whenhead.revisionis empty. This guard runs before the provider call, so a regression there would create a pull request that cannot be recorded.💚 Proposed additional test
+test("missing head revision fails before the provider is called", async () => { + const { store, taskId } = pushedTaskStore(); + try { + const calls: string[][] = []; + const adapter = adapterWith([runResult("https://github.com/org/repository/pull/36")], calls); + const input = workflowInput(store, taskId, adapter); + const result = await openWorkflowPullRequest({ ...input, head: { name: "feature/36" } }); + assert.equal(result.ok, false); + if (!result.ok) assert.equal(result.reason, "head-sha-required"); + assert.equal(calls.length, 0, "gh pr create must not run without a head revision"); + assert.deepEqual(store.listPullRequestRecordsForTask(taskId), []); + } finally { + store.close(); + } +});🤖 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 `@src/workflow/providers/github.test.ts` around lines 216 - 224, Extend the GitHub workflow tests around openWorkflowPullRequest to cover both untested failure reasons: verify lifecycle-blocked when the task lifecycle disallows pull-request-open, and head-sha-required when head.revision is empty. Assert each result is unsuccessful, confirm no provider call or pull-request record is created where applicable, and preserve the task state expectations.src/workflow/state/pr-records.test.ts (1)
37-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
recordPullRequestidempotency and conflict branches.This test covers only the insert path.
WorkflowSqliteStateStore.recordPullRequestinsrc/workflow/state/sqlite-store.ts(Lines 460-468) has two further branches that callers depend on: it returns the existing record when the identity matches, and it throws whenheadShaortaskIddiffer.openWorkflowPullRequestcatches that throw and converts it tolocal-state-write-failedwithproviderCreated: true, so the branch is on a critical path and is currently untested.💚 Proposed additional test
+test("recording the same pull request twice is idempotent and rejects a conflicting identity", () => { + const store = new WorkflowSqliteStateStore({ dbPath: ":memory:" }); + store.init(); + try { + const input = { + provider: "github", + repositoryId: "org/repository", + prNumber: 36, + url: "https://github.com/org/repository/pull/36", + headSha: "abc123", + lifecycleState: "open" as const, + }; + const first = store.recordPullRequest(input); + const second = store.recordPullRequest(input); + assert.equal(second.recordId, first.recordId); + assert.throws(() => store.recordPullRequest({ ...input, headSha: "def456" }), /different identity/); + } finally { + store.close(); + } +});🤖 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 `@src/workflow/state/pr-records.test.ts` around lines 37 - 58, Extend the “PR records are queryable and lifecycle state can be reconciled” test to cover recordPullRequest idempotency and conflicts: call recordPullRequest again with the same provider, repositoryId, prNumber, headSha, and taskId and assert it returns the existing record, then assert calls with a different headSha or taskId throw. Keep the existing insert and lifecycle assertions intact, and use the existing record identity fields and error behavior from recordPullRequest.src/workflow/state/store.ts (1)
210-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the idempotency and conflict contract of
recordPullRequest.
WorkflowSqliteStateStore.recordPullRequestinsrc/workflow/state/sqlite-store.ts(Lines 454-468) returns the existing record whenprovider,repositoryId,prNumber,headSha, andtaskIdall match, and throws when the identity conflicts.openWorkflowPullRequestdepends on that behavior: it wraps the call intry/catchand maps the throw tolocal-state-write-failed. The interface comment describes only the excluded fields, so any second implementation ofWorkflowStateStorecould diverge without breaking the type.♻️ Proposed contract documentation
- /** 外部 provider 成功後に、body/raw response を含めず PR の照合用 metadata だけ記録する。 */ + /** + * 外部 provider 成功後に、body/raw response を含めず PR の照合用 metadata だけ記録する。 + * (provider, repositoryId, prNumber) が既存行と一致し headSha/taskId も同じ場合は、 + * timeout 後の再試行を二重登録にしないため既存 record をそのまま返す。 + * 同じ PR に異なる headSha/taskId が来た場合は、取り違えを検出するため throw する。 + */ recordPullRequest(input: RecordPullRequestInput): PullRequestRecord;🤖 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 `@src/workflow/state/store.ts` around lines 210 - 211, Update the documentation for the WorkflowStateStore.recordPullRequest contract and its implementation WorkflowSqliteStateStore.recordPullRequest: state that matching provider, repositoryId, prNumber, headSha, and taskId must return the existing record, while any identity conflict must throw. Preserve the existing openWorkflowPullRequest error mapping and ensure the interface contract is explicit enough for alternate implementations to follow.src/workflow/policy/schema.ts (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRevise source comments to use Japanese and state only the reason.
src/workflow/policy/schema.ts#L55-L55: Remove the implementation description. Keep only a Japanese compatibility reason if the comment is needed.src/workflow/domain/pr-render.ts#L15-L16: Replace the English comment with a Japanese reason-only comment.src/workflow/domain/pr-render.ts#L214-L220: Remove redundant alias descriptions. If a comment remains for policy conversion, write only its reason in Japanese.As per coding guidelines, comments are Japanese and explain only why.
🤖 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 `@src/workflow/policy/schema.ts` at line 55, Revise comments at src/workflow/policy/schema.ts lines 55-55, src/workflow/domain/pr-render.ts lines 15-16, and src/workflow/domain/pr-render.ts lines 214-220 to use Japanese and explain only the reason: remove the implementation and alias descriptions, retain only the Japanese compatibility reason in the schema comment if needed, replace the English comment with a Japanese reason-only comment, and remove the redundant alias comment unless a Japanese policy-conversion reason is necessary.Source: Coding guidelines
🤖 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 `@src/workflow/providers/github.test.ts`:
- Around line 178-189: Wrap each of the three tests using
pushedTaskStore—including “provider success writes PR metadata and transitions
the task”—in a try/finally block, placing the existing test body in try and
store.close() in finally so the WorkflowSqliteStateStore is always released,
including when assertions fail.
In `@src/workflow/state/pr-records.test.ts`:
- Around line 8-35: Update the migration assertion in the “pr_records migration
is append-only and stores no body or credential columns” test to verify that
MIGRATIONS contains a migration with version 5, rather than requiring version 5
to be the final entry. Keep the existing schema and prohibited-column assertions
unchanged.
In `@src/workflow/state/sqlite-store.ts`:
- Around line 454-468: Move the existence SELECT in recordPullRequest inside the
BEGIN IMMEDIATE transaction, keeping the identity validation and idempotent
return behavior unchanged. Ensure the transaction performs the serialized check
before the INSERT, matching reserveTask’s uniqueness-handling pattern and
existing rollback behavior.
---
Nitpick comments:
In `@src/workflow/policy/schema.ts`:
- Line 55: Revise comments at src/workflow/policy/schema.ts lines 55-55,
src/workflow/domain/pr-render.ts lines 15-16, and
src/workflow/domain/pr-render.ts lines 214-220 to use Japanese and explain only
the reason: remove the implementation and alias descriptions, retain only the
Japanese compatibility reason in the schema comment if needed, replace the
English comment with a Japanese reason-only comment, and remove the redundant
alias comment unless a Japanese policy-conversion reason is necessary.
In `@src/workflow/providers/github.test.ts`:
- Around line 216-224: Extend the GitHub workflow tests around
openWorkflowPullRequest to cover both untested failure reasons: verify
lifecycle-blocked when the task lifecycle disallows pull-request-open, and
head-sha-required when head.revision is empty. Assert each result is
unsuccessful, confirm no provider call or pull-request record is created where
applicable, and preserve the task state expectations.
In `@src/workflow/providers/github.ts`:
- Around line 158-160: Rename the shadowing repository parameter in
parseGithubIssueOutput and parseGithubPullRequestRecord, then replace the inline
unknown identity fallback in parseGithubIssueOutput with the module-level
fallbackRepository helper. Preserve the existing parameter behavior while
ensuring both functions consistently use the helper without duplicate fallback
values.
- Around line 752-776: Update the contract between openWorkflowPullRequest and
openPullRequest so the workflow passes its already-rendered body via a
renderedBody input. In openPullRequest, use renderedBody when provided and
retain the existing renderPullRequestBody fallback for direct callers; update
openWorkflowPullRequest to pass rendered.body to ensure the submitted and
returned bodies match.
In `@src/workflow/providers/model.ts`:
- Around line 58-65: Redeclare number and url as required fields on the
PullRequest interface, overriding their optional definitions inherited from
IssueReference. Update openWorkflowPullRequest to remove the corresponding
runtime guard and local-state-write-failed mapping, while leaving provider
adapter validation responsible for constructing valid PullRequest values.
In `@src/workflow/state/pr-records.test.ts`:
- Around line 37-58: Extend the “PR records are queryable and lifecycle state
can be reconciled” test to cover recordPullRequest idempotency and conflicts:
call recordPullRequest again with the same provider, repositoryId, prNumber,
headSha, and taskId and assert it returns the existing record, then assert calls
with a different headSha or taskId throw. Keep the existing insert and lifecycle
assertions intact, and use the existing record identity fields and error
behavior from recordPullRequest.
In `@src/workflow/state/store.ts`:
- Around line 210-211: Update the documentation for the
WorkflowStateStore.recordPullRequest contract and its implementation
WorkflowSqliteStateStore.recordPullRequest: state that matching provider,
repositoryId, prNumber, headSha, and taskId must return the existing record,
while any identity conflict must throw. Preserve the existing
openWorkflowPullRequest error mapping and ensure the interface contract is
explicit enough for alternate implementations to follow.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fbba8370-8e4d-432e-95f6-eb59ec069926
📒 Files selected for processing (13)
src/state/migrations.test.tssrc/state/migrations.tssrc/workflow/domain/pr-render.test.tssrc/workflow/domain/pr-render.tssrc/workflow/policy/schema.test.tssrc/workflow/policy/schema.tssrc/workflow/providers/github.test.tssrc/workflow/providers/github.tssrc/workflow/providers/model.test.tssrc/workflow/providers/model.tssrc/workflow/state/pr-records.test.tssrc/workflow/state/sqlite-store.tssrc/workflow/state/store.ts
yohnark
left a comment
yohnark
left a comment
There was a problem hiding this comment.
Re-review — previous findings resolved
前回の 2 Blocking + 2 Major を再確認しました。
- PR mutation は repository identity が explicit
owner/nameに解決できない場合、gh pr create自体を実行せず fail-closed になっています。 - PR record 書込み後に task transition だけ失敗した状態は、retry 時に既存 record を検出して
pushed -> pull-request-openを再実行する reconciliation path が追加されています。 - closing Issue 抽出は GitHub Issue reference 形式に限定され、一般文中の
fix/close/resolveを誤認しにくくなっています。 gh issue view --jsonにassignees,milestoneが追加され、parse している metadata と取得 field が一致しました。- それぞれ回帰テストも追加されています。
前回指摘は解消済みと判断します。CI / Governance も最新 head で成功を確認済みです。
※ author と reviewer が同一 GitHub identity のため APPROVE は付けられず、COMMENT として記録しています。
…en types Addresses CodeRabbit review on PR #100: - recordPullRequest's existence check now runs inside the same BEGIN IMMEDIATE transaction as the insert, matching reserveTask's established pattern. Previously the SELECT ran before the transaction, so two concurrent callers could both observe no existing row and race the INSERT. - pr_records migration test now asserts a version-5 migration exists instead of requiring it to be the last entry in MIGRATIONS. - Tests using pushedTaskStore() now close the store in a finally block so a failing assertion doesn't leak the SQLite handle. - PullRequest.number/url are now required (every construction site already set them); removed the now-redundant runtime guard in openWorkflowPullRequest. - Renamed shadowing `fallbackRepository` parameters to `defaultRepository` so the module-level fallbackRepository() helper is unambiguous, and reused the helper in parseGithubIssueOutput. - Removed the unused renderPrBody/validatePrBody aliases; rewrote two comments to state only the reason in Japanese, per coding guidelines. New tests: lifecycle-blocked and head-sha-required failure paths in openWorkflowPullRequest, and recordPullRequest idempotency/conflict behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
CodeRabbit のレビュー対応完了 (ce2be57): [Critical] recordPullRequest の TOCTOU: existence check を [Minor] migration version 5 テストの脆弱性: [Minor] store leak: Nitpick 対応:
二重 render（
|
1 participant
Summary
Adds provider-neutral
Issue/PullRequestdata models and a GitHub adapter that passes through the same policy/lifecycle contracts as local Git operations, including structured-field-based PR body generation and a newpr_recordsstate table.Linked issue
Closes #36
Scope
Included
src/workflow/providers/model.ts: provider-neutralIssue/PullRequeststructured types, independent of GitHub.src/workflow/providers/github.ts:gh-backedGithubAdapterreusing the non-throwing structured-parse convention fromparseIssueViewOutput; mutations (e.g. opening a PR) route through the same policy/lifecycle contracts as the rest ofsrc/workflow/.src/workflow/domain/pr-render.ts: renders PR bodies from structured fields plus repository policy/templates, so the shape can consume Mottainai's own governance schema as one configuration without hard-coding it (per Epic: add configurable Git workflow guardrails and isolated task workspaces #28).src/workflow/policy/schema.ts: adds apullRequestpolicy block (required Issue, exactly-one-closing-Issue, required sections, checklist acceptance criteria).src/state/migrations.ts: appends apr_recordstable recording provider, repository identity, PR number, URL, head SHA, and lifecycle state — never the full PR body.Excluded
mottainai_issue_viewMCP tool's public behavior; only its internal parsing pattern is reused, not its tool registration.pr_recordsstate queryable for it (Child Issue 8).model.tsstays provider-agnostic in shape but onlygithub.tsimplements it.Implementation
GithubAdapter.openPullRequesttakes a structuredPullRequestBodyDraftplus an optional render policy instead of a free-form body string, and callsrenderPullRequestBody()before ever invokinggh pr create, so policy-required sections are enforced at the adapter boundary rather than left to the caller.ghsubprocess calls go through the existingrunProgram()wrapper, matching themottainai_issue_viewconvention already in the codebase.parseCreatePullRequestOutputand related parsing follow the same non-throwing structured-parse pattern asparseIssueViewOutput: malformedghJSON produces a typed error result, not a thrown exception.pr_recordsmigration only stores provider/repository-identity/number/URL/head-SHA/lifecycle-state columns; no body text or credentials are persisted.Behavioral changes
WorkflowPolicyDocumentgains an optionalpullRequestfield; documents without it behave exactly as before.Validation
npm run typecheck,npm run lint, andnpm test(499 passing) all pass locally, including new suites:providers/model.test.ts,providers/github.test.ts(mockedghoutput),domain/pr-render.test.ts,state/pr-records.test.ts, plus updatedstate/migrations.test.tsandpolicy/schema.test.ts.Risks
Provider mutation (
gh pr create) and the local SQLitepr_recordswrite are not globally atomic — aghfailure after PR creation but before the local write is idempotent-retryable, not silently duplicated, since creation itself is not auto-retried (a timeout may mean the PR already exists provider-side). No full Issue/PR body text, credentials, or tokens are ever persisted to SQLite state, only structured metadata.Breaking changes
No. This PR only adds new modules, a new optional policy field, and a new database migration; no existing behavior is modified.
Review focus
renderPullRequestBody's required-section enforcement at theGithubAdapterboundary is the right layering, versus enforcing it further upstream.pr_recordsmigration's column set is sufficient for the reconciliation work planned in Child Issue 8.