feat(workflow): add repository identity model and SQLite state foundation - #45
Merged
Merged
Conversation
Append repository_sources, repository_instances, and repository_paths tables (schema version 2) for Issue #28 Child 2. Existing migration 1 is untouched.
resolveRepositoryIdentity() derives a RootCommitDigest (a Git-derived hint, not globally unique) and a stable RepositoryInstanceId from a UUID marker file persisted inside the Git common directory, so identity survives repository moves/renames without depending on remote URL, filesystem path, or branch name alone. A corrupted marker file fails closed instead of silently re-minting an id.
New repository-identity-scoped persistence interface, separate from src/state/store.ts's session/read-evidence StateStore, so a future per-repository DB split doesn't change workflow-domain APIs.
WorkflowSqliteStateStore shares the same DB file and migration mechanism as the session-scoped SqliteStateStore. observeRepositoryInstance() is the single write path: it mints a globally unique RepositorySourceId the first time a given RootCommitDigest is seen (reusing it afterward, so root-commit-digest collisions never produce duplicate sources), tracks canonicalized worktree paths per instance, and reports detected moves.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds Git repository identity resolution, migration version 2 for workflow state tables, and a SQLite-backed ChangesRepository workflow state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitRepository
participant resolveRepositoryIdentity
participant WorkflowSqliteStateStore
participant SQLite
GitRepository->>resolveRepositoryIdentity: resolve canonical paths and Git root commits
resolveRepositoryIdentity-->>GitRepository: return repository identity
GitRepository->>WorkflowSqliteStateStore: observe repository identity and worktree path
WorkflowSqliteStateStore->>SQLite: create or reuse source and instance
WorkflowSqliteStateStore->>SQLite: record current and historical paths
SQLite-->>WorkflowSqliteStateStore: commit observation
WorkflowSqliteStateStore-->>GitRepository: return observation result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/state/migrations.ts (1)
79-86: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce one current path per instance in the schema.
repository_pathsallows several rows withis_current = 1for the sameinstance_id.WorkflowSqliteStateStore.observeRepositoryInstancereads the current path with a single-rowget()(src/workflow/state/sqlite-store.tslines 141-143), so a broken invariant produces a non-deterministic current path and wrong move detection. Add a partial unique index while the migration is still unreleased.Also,
idx_repository_paths_instanceduplicates the leftmost column of the primary key(instance_id, canonical_path). SQLite can serveinstance_idlookups from the primary-key index, so the extra index only adds write cost.♻️ Proposed change
CREATE TABLE repository_paths ( instance_id TEXT NOT NULL REFERENCES repository_instances (instance_id), canonical_path TEXT NOT NULL, is_current INTEGER NOT NULL DEFAULT 1, observed_at INTEGER NOT NULL, PRIMARY KEY (instance_id, canonical_path) ); - CREATE INDEX idx_repository_paths_instance ON repository_paths (instance_id); + CREATE UNIQUE INDEX idx_repository_paths_current + ON repository_paths (instance_id) WHERE is_current = 1;🤖 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/state/migrations.ts` around lines 79 - 86, Update the repository_paths schema in the migration to enforce at most one row with is_current = 1 per instance_id using a SQLite partial unique index, and remove the redundant idx_repository_paths_instance index because the primary key already supports instance_id lookups.src/workflow/domain/identity.ts (2)
60-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the marker validation pattern.
INSTANCE_ID_PATTERNaccepts any 36-character string built from hex digits and hyphens, for example"------------------------------------". A stricter UUID shape makes the fail-closed corruption check reliable.Also, if the race winner's content is invalid, the function returns
cannot create instance marker file: EEXIST. That reason mislabels a corrupt marker. Return the corrupt-marker reason in that branch.♻️ Proposed change
-const INSTANCE_ID_PATTERN = /^[0-9a-f-]{36}$/; +const INSTANCE_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/;if ((err as NodeJS.ErrnoException).code === "EEXIST") { try { const raceWinner = fs.readFileSync(markerPath, "utf8").trim(); if (INSTANCE_ID_PATTERN.test(raceWinner)) return { ok: true, instanceId: raceWinner as RepositoryInstanceId }; + return { ok: false, reason: `instance marker file is corrupt: ${markerPath}` }; } catch { // 下の共通エラーへフォールスルー } }Also applies to: 90-98
🤖 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/domain/identity.ts` at line 60, Strengthen INSTANCE_ID_PATTERN to validate the canonical UUID structure, including required hyphen positions and hexadecimal group lengths, rather than accepting any 36-character hex/hyphen string. In the race-winner validation branch of the instance-marker creation flow, return the corrupt-marker reason when the existing marker content is invalid instead of the EEXIST creation error.
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument platform-dependent case handling.
fs.realpathSync.nativeresolves symlinks, but case normalization depends on the operating system and filesystem. Do not describe case normalization as a universal guarantee.🤖 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/domain/identity.ts` around lines 42 - 45, Update the documentation for canonicalizePath to state that it resolves symlinks and normalizes trailing-slash differences, while clarifying that case normalization is platform- and filesystem-dependent rather than guaranteed universally.src/workflow/domain/identity.test.ts (2)
62-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
git worktreecase.
resolveRepositoryIdentitydocuments that two worktrees of one repository sharegitCommonDirand therefore return the sameinstanceId(src/workflow/domain/identity.tslines 102-106). No test covers that path, and the linked issue lists worktree/path behavior as an acceptance item. Add a test that runsgit worktree add, resolves identity from the linked worktree, and asserts an equalinstanceIdwith a differentworktreePath.Do you want me to draft that test?
🤖 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/domain/identity.test.ts` around lines 62 - 79, Add a test alongside the symlink identity test that creates a second checkout using git worktree add, then resolves both repository paths with resolveRepositoryIdentity. Assert both resolutions succeed, their instanceId values match, and their worktreePath values differ, cleaning up the added worktree in a finally block.
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd temp-directory cleanup.
initRepocreates a directory underos.tmpdir()and most tests never remove it. Each run leaves Git repositories behind. Register cleanup with the test context, for examplet.after(() => fs.rmSync(root, { recursive: true, force: true })), or delete the directory at the end of each test.🤖 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/domain/identity.test.ts` around lines 13 - 22, Update initRepo to register cleanup for the temporary repository directory, using the test context’s after hook to remove root recursively with force enabled; adjust the helper’s interface or call sites as needed so the cleanup runs for every test.src/workflow/state/sqlite-store.test.ts (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a file-backed store test.
Every test uses
:memory:, so the file-backed branch ofinit()never runs (src/workflow/state/sqlite-store.tslines 76-91): directory creation,0o600/0o700permissions, and WAL sidecar handling. Add one test that pointsdbPathat a temp file, then asserts that observation and lookups work and that the file mode is owner-only on non-Windows platforms.🤖 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/sqlite-store.test.ts` around lines 6 - 10, Add a file-backed test alongside openStore that creates a temporary database path, initializes WorkflowSqliteStateStore, records an observation, and verifies observation and lookup behavior. On non-Windows platforms, assert the database file uses owner-only permissions, and ensure temporary resources are cleaned up; cover the file-backed init path including directory and WAL handling without changing existing in-memory tests.src/workflow/state/sqlite-store.ts (1)
111-124: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReturn the inserted row without a second SELECT.
The code selects, inserts when missing, then selects again.
RETURNINGremoves the extra query and keeps the record construction in one place. The same applies to the instance block on lines 126-139.🤖 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/sqlite-store.ts` around lines 111 - 124, Update the source and instance persistence blocks around toSourceRecord to replace the pre-check/insert/second-SELECT flow with an INSERT ... ON CONFLICT ... RETURNING query, while preserving existing-row reuse and constructing each record from the returned row in one place.
🤖 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/domain/identity.ts`:
- Around line 129-140: The root commit digest derived in the current
identity-resolution flow is not stable across unrelated histories or orphan
branches. Confirm the intended repository-wide identity behavior, then update
the root commit lookup around runGit and deriveRootCommitDigest to use a stable
reference or deterministic fixed root, ensuring the same repository preserves
one RepositorySourceId as documented.
In `@src/workflow/state/sqlite-store.ts`:
- Around line 130-136: Confirm the intended replacement policy for a reused
git_common_dir, then update the existing-instance handling around the
repository_instances INSERT/UPDATE flow to resolve a stale row whose instance_id
differs but whose git_common_dir matches. Detach or remove the stale mapping
before inserting the new instance so the UNIQUE constraint is not violated,
while preserving normal last_seen_at updates for matching instance_id values.
- Around line 164-167: Update the catch block in applyMigrations to wrap
db.exec("ROLLBACK") in a nested try/catch, ignoring any rollback failure so the
original COMMIT error is rethrown. Match the existing guarded rollback pattern
used in the migrations implementation.
---
Nitpick comments:
In `@src/state/migrations.ts`:
- Around line 79-86: Update the repository_paths schema in the migration to
enforce at most one row with is_current = 1 per instance_id using a SQLite
partial unique index, and remove the redundant idx_repository_paths_instance
index because the primary key already supports instance_id lookups.
In `@src/workflow/domain/identity.test.ts`:
- Around line 62-79: Add a test alongside the symlink identity test that creates
a second checkout using git worktree add, then resolves both repository paths
with resolveRepositoryIdentity. Assert both resolutions succeed, their
instanceId values match, and their worktreePath values differ, cleaning up the
added worktree in a finally block.
- Around line 13-22: Update initRepo to register cleanup for the temporary
repository directory, using the test context’s after hook to remove root
recursively with force enabled; adjust the helper’s interface or call sites as
needed so the cleanup runs for every test.
In `@src/workflow/domain/identity.ts`:
- Line 60: Strengthen INSTANCE_ID_PATTERN to validate the canonical UUID
structure, including required hyphen positions and hexadecimal group lengths,
rather than accepting any 36-character hex/hyphen string. In the race-winner
validation branch of the instance-marker creation flow, return the
corrupt-marker reason when the existing marker content is invalid instead of the
EEXIST creation error.
- Around line 42-45: Update the documentation for canonicalizePath to state that
it resolves symlinks and normalizes trailing-slash differences, while clarifying
that case normalization is platform- and filesystem-dependent rather than
guaranteed universally.
In `@src/workflow/state/sqlite-store.test.ts`:
- Around line 6-10: Add a file-backed test alongside openStore that creates a
temporary database path, initializes WorkflowSqliteStateStore, records an
observation, and verifies observation and lookup behavior. On non-Windows
platforms, assert the database file uses owner-only permissions, and ensure
temporary resources are cleaned up; cover the file-backed init path including
directory and WAL handling without changing existing in-memory tests.
In `@src/workflow/state/sqlite-store.ts`:
- Around line 111-124: Update the source and instance persistence blocks around
toSourceRecord to replace the pre-check/insert/second-SELECT flow with an INSERT
... ON CONFLICT ... RETURNING query, while preserving existing-row reuse and
constructing each record from the returned row in one place.
🪄 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: 5f6f2c23-8ac7-406d-9766-a9a00b9ef0e6
📒 Files selected for processing (8)
AGENTS.mddocs/workflow-policy.mdsrc/state/migrations.tssrc/workflow/domain/identity.test.tssrc/workflow/domain/identity.tssrc/workflow/state/sqlite-store.test.tssrc/workflow/state/sqlite-store.tssrc/workflow/state/store.ts
Address CodeRabbit review on PR #45: - rootCommitDigest now hashes root commits reachable from any ref (`rev-list --max-parents=0 --all`) instead of HEAD only, so switching branches or adding an orphan branch no longer changes the digest of an already-known repository. - INSTANCE_ID_PATTERN now validates the canonical UUID shape instead of accepting any 36-char hex/hyphen string. - A race-winner marker that fails validation now reports the corrupt-marker reason instead of the EEXIST creation error. - canonicalizePath's doc comment no longer claims case normalization is guaranteed (it's platform/filesystem-dependent). Added tests for stability across worktrees and orphan-branch checkout.
…oryInstance Address CodeRabbit review on PR #45: - A reused git_common_dir (marker file deleted, or re-clone into the same path) minting a new instanceId no longer fails on the UNIQUE constraint. The stale instance row is superseded: its git_common_dir is rewritten to a unique tombstone value so the new instance can claim it, while the stale row and its repository_paths history are preserved. - ROLLBACK is now wrapped in try/catch, matching the existing pattern in src/state/migrations.ts, so a rollback failure can't mask the original transaction error. - repository_paths gets a partial unique index enforcing at most one is_current=1 row per instance (this migration is unreleased, so it's edited in place rather than appended as a new version). The redundant non-unique index on instance_id is removed since the primary key's leading column already serves that lookup. Added tests for the supersede path and a file-backed store round trip (directory/file permissions, WAL handling).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflow/domain/identity.ts (1)
78-99: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPublish the instance marker atomically.
fs.writeFileSync(..., { flag: "wx" })provides exclusive creation, but not atomic content publication. A concurrent resolver can read an empty or partial marker, and a crash can leave a corrupt marker that causes later resolutions to fail closed. Write to a temporary file and publish it with exclusive no-overwrite semantics. Add a multi-process first-creation test.🤖 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/domain/identity.ts` around lines 78 - 99, Update the instance-marker creation flow around the resolver’s fs.writeFileSync call to write the generated ID to a unique temporary file, then atomically publish it with exclusive no-overwrite semantics so readers only observe complete content and existing markers are never replaced. Preserve the EEXIST race-winner handling and add a multi-process first-creation test covering concurrent resolution.
🧹 Nitpick comments (3)
src/workflow/domain/identity.test.ts (2)
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace behavior comments with rationale comments.
Lines 14 and 27 describe what each helper does. Keep only Japanese comments that state why the helper exists.
Proposed change
-/** 一時リポジトリを作り、テスト終了時に自動削除する。 */ +// テスト間でリポジトリ状態が残らないようにする。 function initRepo(t: TestContext): string { -/** 一時ディレクトリを作り、テスト終了時に自動削除する（git init しない）。 */ +// 非Gitディレクトリが必要な失敗系を他のテストから隔離する。 function tmpDir(t: TestContext, prefix: string): string {As per coding guidelines, “コメントは日本語で why のみ記述し、what の説明コメントは書かない”.
Also applies to: 27-31
🤖 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/domain/identity.test.ts` around lines 14 - 15, Update the Japanese comments above initRepo and the other helper around lines 27–31 to explain why each helper is needed rather than describing what it does. Keep the comments Japanese and rationale-focused, removing behavior-only wording while preserving the helper implementations.Source: Coding guidelines
72-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a junction for Windows directory links.
When this test runs on Windows,
fs.symlinkSync(..., "dir")can fail withEPERMwithout symlink privileges. Select"junction"onwin32; the current Windows CI job excludes this test, but the repository supports Windows.🤖 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/domain/identity.test.ts` around lines 72 - 77, Update the symlink setup in the test resolving through a symlinked path to use `"junction"` as the link type when process.platform is `"win32"`, while retaining `"dir"` on other platforms. Keep the existing cleanup and identity assertions unchanged.src/workflow/domain/identity.ts (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite this JSDoc as a why-only comment.
The first sentence describes what
canonicalizePathreturns. Replace it with the reason canonicalization is required, or remove it.Proposed wording
- * シンボリックリンク解決・末尾スラッシュ差異を吸収した絶対パスを返す。 + * リポジトリ移動とシンボリックリンク経由の入力を同一パスとして比較するため、 + * 実体パスを基準に正規化する。As per coding guidelines, comments must be written in Japanese and explain why only, not what.
🤖 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/domain/identity.ts` around lines 42 - 46, Rewrite the JSDoc above canonicalizePath as a Japanese why-only comment: remove the description of what the function returns and retain only the reason path canonicalization is needed, including the relevant filesystem case-sensitivity caveat if still useful.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.
Outside diff comments:
In `@src/workflow/domain/identity.ts`:
- Around line 78-99: Update the instance-marker creation flow around the
resolver’s fs.writeFileSync call to write the generated ID to a unique temporary
file, then atomically publish it with exclusive no-overwrite semantics so
readers only observe complete content and existing markers are never replaced.
Preserve the EEXIST race-winner handling and add a multi-process first-creation
test covering concurrent resolution.
---
Nitpick comments:
In `@src/workflow/domain/identity.test.ts`:
- Around line 14-15: Update the Japanese comments above initRepo and the other
helper around lines 27–31 to explain why each helper is needed rather than
describing what it does. Keep the comments Japanese and rationale-focused,
removing behavior-only wording while preserving the helper implementations.
- Around line 72-77: Update the symlink setup in the test resolving through a
symlinked path to use `"junction"` as the link type when process.platform is
`"win32"`, while retaining `"dir"` on other platforms. Keep the existing cleanup
and identity assertions unchanged.
In `@src/workflow/domain/identity.ts`:
- Around line 42-46: Rewrite the JSDoc above canonicalizePath as a Japanese
why-only comment: remove the description of what the function returns and retain
only the reason path canonicalization is needed, including the relevant
filesystem case-sensitivity caveat if still useful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fe99305-a8a3-48b0-bcc7-c8fbb89e1104
📒 Files selected for processing (6)
docs/workflow-policy.mdsrc/state/migrations.tssrc/workflow/domain/identity.test.tssrc/workflow/domain/identity.tssrc/workflow/state/sqlite-store.test.tssrc/workflow/state/sqlite-store.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/state/migrations.ts
- docs/workflow-policy.md
- src/workflow/state/sqlite-store.ts
Address CodeRabbit review on PR #45 (Critical): - resolveOrCreateInstanceId() previously wrote the marker file directly with `wx` (exclusive create). Exclusive creation guards against overwriting an existing marker, but the write itself is not atomic: a concurrent resolver could observe an empty or partial file, and a crash mid-write could leave a corrupt marker that later resolutions then fail closed on. - Now writes the generated id to a unique temp file first, then publishes it via fs.linkSync (a hard link, which fails with EEXIST rather than silently overwriting if the target already exists). Other processes only ever observe "marker absent" or "marker complete" — never partial content. - Added identity-resolve-worker.mjs plus a test that spawns 5 real concurrent processes racing to create the marker for the same repository, asserting they converge on one instance id. Also addressed nitpicks: rewrote two doc comments as why-only (canonicalizePath, test helpers) and made the symlink test use a junction on win32 (plain symlinks can require elevated privileges there).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/workflow/domain/identity.ts (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep failure-path comments rationale-only and correct.
Line 109 describes control flow instead of rationale. Line 117 incorrectly implies that
tmpPathis absent afterfs.linkSync. The temporary hard-link remains untilfs.unlinkSync(tmpPath)succeeds.Proposed fix
} catch { - // 下の共通エラーへフォールスルー + // 競合 winner を検証できない場合に新しい ID を発行しないため、失敗として扱う。 } @@ } catch { - // ベストエフォート（既に自分で publish 済みなら該当ファイルは残らない想定） + // 公開済み marker の整合性を優先するため、削除失敗で解決を失敗させない。 }Also applies to: 114-118
🤖 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/domain/identity.ts` around lines 108 - 110, Update the failure-path comments in the surrounding identity-linking logic to state only the rationale for falling through to the shared error handling, not the control flow itself. Correct the comment near fs.linkSync and tmpPath to reflect that the temporary hard link remains until fs.unlinkSync(tmpPath) succeeds.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/domain/identity.test.ts`:
- Around line 176-179: Update the child-process completion handling in runWorker
to resolve only after the stdout stream closes, using the close event rather
than exit so all output has been received before trimming and returning the
instance ID; preserve rejection for non-zero exit codes.
---
Nitpick comments:
In `@src/workflow/domain/identity.ts`:
- Around line 108-110: Update the failure-path comments in the surrounding
identity-linking logic to state only the rationale for falling through to the
shared error handling, not the control flow itself. Correct the comment near
fs.linkSync and tmpPath to reflect that the temporary hard link remains until
fs.unlinkSync(tmpPath) succeeds.
🪄 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: 6d2be75b-a2c1-4075-86f2-9b52de59b444
📒 Files selected for processing (3)
src/workflow/domain/identity-resolve-worker.mjssrc/workflow/domain/identity.test.tssrc/workflow/domain/identity.ts
Address CodeRabbit review on PR #45: - runWorker() resolved on the child's "exit" event, which can fire before the final stdout "data" event, risking a truncated instance id read. Now waits for "close" instead. - Two failure-path comments in resolveOrCreateInstanceId() described control flow ("フォールスルー") or an incorrect claim (that tmpPath is already gone after linkSync) rather than rationale; reworded to state why, matching the coding guideline.
This was referenced Aug 7, 2026
This was referenced Aug 9, 2026
This was referenced Aug 15, 2026
feat(runtime): launch repository workloads under stable principals with persistent user tooling
#297
Open
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
1 participant
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.
Summary
Add repository/source/instance identity resolution and the SQLite workflow-state foundation: new
repository_sources,repository_instances,repository_pathstables plus a workflow-scopedWorkflowStateStoreinterface and SQLite implementation, undersrc/workflow/domain/andsrc/workflow/state/.Linked issue
Closes #31
Scope
Included
src/workflow/domain/identity.ts:resolveRepositoryIdentity(cwd)resolves, for the Git repository atcwd, aRootCommitDigest(hash of root commit SHA(s) — a Git-derived hint, not a globally unique id) and aRepositoryInstanceId(a UUID persisted in a marker file inside the Git common directory, created on first resolution). Fails closed (structured{ ok: false, reason }) on non-Git directories, unborn HEAD, and corrupted marker files.src/state/migrations.ts: append-only migration (schema version 2) addingrepository_sources,repository_instances,repository_paths. Existing migration 1 is untouched.src/workflow/state/store.ts:WorkflowStateStoreinterface, scoped to repository identity, separate from the existing session/read-evidenceStateStore.src/workflow/state/sqlite-store.ts:WorkflowSqliteStateStoreimplementation, reusingresolveStateDir/resolveStateDbPathand the transactional migration-apply mechanism.observeRepositoryInstance()is the single write path: mints aRepositorySourceId(viacrypto.randomUUID()) the first time aRootCommitDigestis seen, reuses it on subsequent observations, tracks canonicalized worktree paths per instance, and reports detected moves.docs/workflow-policy.md,AGENTS.md: document the identity model and state foundation.Excluded
worktrees,tasks,locks/leases, orpr_recordstables — owned by later child Issues, added when their domain contracts are concrete.node:sqlitevia the existingSqliteStateStorepattern.Implementation
Two design points diverged from the most direct reading of the Issue, discovered while writing tests (both confirmed with the repo owner before implementing):
RepositorySourceIdis not derived purely from root commit SHAs. A hash of root commit SHA(s) alone is not globally unique — two independently initialized repositories with identical content (same tree, same author/timestamp/message) can produce the same root commit SHA, which a test reproduced directly. Soidentity.tsonly computes aRootCommitDigest(a hint/lookup key), andWorkflowSqliteStateStore.observeRepositoryInstance()owns actualRepositorySourceIdissuance: it mints a fresh UUID the first time a digest is observed and reuses it afterward via aroot_commit_digestunique lookup, so a digest collision cannot mint a duplicate source.RepositoryInstanceIdis not derived from the common-dir path. A path-derived id would change when a repository is moved/renamed, which defeats the Issue's own move-detection requirement (an "instance" must stay the same instance after a move). Instead,instanceIdis a UUID written to a marker file inside the Git common directory (<git-common-dir>/mottainai-instance-id) on first resolution; since the common directory moves as a unit with the repository, the id survives renames. A corrupted marker file fails closed rather than silently re-minting an id (which would silently fork identity for a repository that state already tracks).Also fixed during implementation:
git rev-parse --git-common-dirreturns a relative path (e.g..git) for non-worktree repositories, which was being resolved against the calling process's cwd instead of the target repository's cwd before being canonicalized — this collapsed all test repositories to this project's own.gitdirectory until fixed.Behavioral changes
None for existing code paths — this PR only adds new, currently-unused modules under
src/workflow/domain/andsrc/workflow/state/, plus an additive (never-mutated) migration.src/workflow/state/sqlite-store.tsopens the same on-disk SQLite database as the existing session-scopedSqliteStateStore(sameresolveStateDbPath()), so its migration runs automatically the next time any workflow or session state store is initialized.Validation
pnpm test(526/526 pass, including 16 new tests acrossidentity.test.tsandsqlite-store.test.ts),pnpm run typecheck,pnpm run build, andpnpm run governance:test(18/18) all pass. Package check is not applicable: no path inpackageCheckPaths(package.json, pnpm-lock.yaml, tsconfig.build.json, src/index.ts, src/server.ts, src/cli.ts, publish workflow) is touched.Risks
Filesystem path identity varies across OS/symlinks/case-sensitivity;
identity.test.tscovers symlinked paths and repository moves explicitly (both directly and through the SQLite move-detection path) rather than assuming POSIX semantics. The migration is append-only and must never rewrite existingsrc/state/migrations.tsentries — verified this PR only appends a newversion: 2entry.repository_sources.root_commit_digestcollisions (same digest, different actual repositories) are handled by design (see Implementation) rather than assumed away.Breaking changes
No.
Review focus
resolveOrCreateInstanceId()insrc/workflow/domain/identity.ts(marker-file read/create/race-handling logic) andobserveRepositoryInstance()insrc/workflow/state/sqlite-store.ts(source-id issuance and move-detection transaction) — these are the two places where a subtle bug would silently fork or duplicate repository identity.