Recover relay run issue-1190-20260808121320593-57e9108f - #1192
Conversation
Reason: publish verified #1190 stranded-worktree recovery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDispatch가 브랜치 생성 충돌을 명시적으로 보고합니다. 복구 로직은 stranded worktree의 소유권과 변경 상태를 검증한 뒤 안전한 경우에만 제거합니다. 검사 로직은 조건에 따라 publication 복구 또는 재디스패치를 반환합니다. ChangesStranded worktree 복구
Publication 복구 판정
Estimated code review effort: 4 (복잡) | ~60분 Sequence Diagram(s)sequenceDiagram
participant Dispatch as dispatch.js
participant Git as Git
participant RelayRecover as relay-recover.js
participant Recovery as recoverStrandedWorktree
Dispatch->>Git: 브랜치 생성
Git-->>Dispatch: BRANCH_EXISTS
Dispatch-->>RelayRecover: repository와 branch를 포함한 복구 안내
RelayRecover->>Recovery: stranded worktree 복구 요청
Recovery->>Git: worktree와 branch 상태 재검증
Recovery->>Git: worktree 및 branch 제거
Git-->>Recovery: 복구 결과
Recovery-->>RelayRecover: status와 operation 출력
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f40d825743
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| execGit(current.checkout, ["worktree", "remove", current.worktree]); | ||
| try { | ||
| execGit(current.checkout, ["update-ref", "-d", current.ref, current.branchHead]); |
There was a problem hiding this comment.
When the named branch has committed but unmerged work in a clean worktree under RELAY_WORKTREE_BASE, this path accepts it as stranded and removes both the worktree and its only branch ref; containment, a clean status, and absence of a valid run.json do not prove that dispatch created it. This is especially hazardous because every branch collision now recommends this command, so recovery needs immutable ownership evidence rather than introducing a branch-only recovery authority.
AGENTS.md reference: AGENTS.md:L3-L7
Useful? React with 👍 / 👎.
| // The inspection is intentionally repeated immediately before the first destructive Git operation. | ||
| // There is no run lock in this pre-run-record window, so every ownership predicate is re-proven. | ||
| const current = observeStrandedWorktree({ repository, branch, relayWorktreeBase: initial.base }); | ||
| if (current.status !== "ready" || current.worktree !== initial.worktree || current.branchHead !== initial.branchHead) { | ||
| recoveryFail("STRANDED_WORKTREE_CHANGED", "stranded worktree changed while recovery was revalidating it"); | ||
| } | ||
| execGit(current.checkout, ["worktree", "remove", current.worktree]); |
There was a problem hiding this comment.
When a live dispatch is still between worktree add and createRunRecord, both observations can report no run reference, after which dispatch may publish run.json before this destructive call executes. Because dispatch and this recovery share no lock or atomic claim, the command can remove the worktree and branch of a newly valid run despite the repeated inspection; the ownership check and mutation must be serialized through one shared capability.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
| const match = /^(worktree|HEAD|branch) (.+)$/.exec(line); | ||
| if (!match) recoveryFail("INVALID_WORKTREE_REGISTRY", "git worktree list returned an unsupported record"); |
There was a problem hiding this comment.
When the repository has any detached, bare, locked, or prunable worktree, this parser rejects the entire registry before it can inspect the requested branch because those valid porcelain records are not worktree, HEAD, or branch key/value lines. Git documents these additional attributes in the git-worktree porcelain format, so an unrelated detached worktree currently makes stranded-worktree recovery unusable.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
skills/relay-dispatch/references/recovery-playbook.md (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value멱등성 동작을 문서에 추가하세요.
recoverStrandedWorktree는 브랜치와 worktree가 이미 제거된 경우already_recovered상태를 반환하고 성공합니다. 문서는 거부 조건만 설명합니다. 운영자가 재실행 안전성을 판단할 수 있도록 이 동작을 명시하세요.🤖 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 `@skills/relay-dispatch/references/recovery-playbook.md` around lines 27 - 31, Update the recovery behavior description for recoverStrandedWorktree to state that when the branch and worktree have already been removed, it returns already_recovered and succeeds. Keep the existing fail-closed rejection conditions unchanged.skills/relay/scripts/relay-recover.js (1)
62-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win텍스트 출력에 삭제 대상이 빠져 있습니다.
recoverStrandedWorktree는worktree와removed_parent_directories를 반환합니다. 텍스트 출력은 이 둘을 표시하지 않습니다.--json없이 실행한 운영자는 무엇이 삭제되었는지 알 수 없습니다. 값이 있을 때만 줄을 추가하세요.🔧 제안 수정
if (result.operation === "recover_stranded_worktree") { - return [ + const lines = [ `Repository: ${result.repo}`, `Branch: ${result.branch}`, `Operation: ${result.operation}`, `Status: ${result.status}`, - ].join("\n"); + ]; + if (result.worktree) lines.push(`Removed worktree: ${result.worktree}`); + for (const dir of result.removed_parent_directories || []) lines.push(`Removed directory: ${dir}`); + return lines.join("\n"); }🤖 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 `@skills/relay/scripts/relay-recover.js` around lines 62 - 69, Update the recover_stranded_worktree text formatting branch to include result.worktree and result.removed_parent_directories, adding each output line only when its value is present. Preserve the existing Repository, Branch, Operation, and Status lines and ensure the text output clearly reports what was removed.skills/relay-dispatch/scripts/recover.js (2)
344-351: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win정리 단계의 예외가 성공한 복구를 실패로 보고합니다.
removeEmptyRelayParents는 370번 줄에서 worktree와 branch 삭제가 이미 끝난 뒤에 실행됩니다.EACCES,EPERM,ENOTDIR같은 코드가 발생하면 예외가 전파되고 CLI는 0이 아닌 상태로 종료합니다. 실제 복구는 완료된 상태이므로 운영자가 실패로 오인합니다.빈 상위 디렉터리 제거는 최선 노력 단계입니다. 실패를 결과에 보고하고 복구 자체는 성공으로 처리하세요.
🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 344 - 351, Update removeEmptyRelayParents so cleanup errors are recorded in its result rather than rethrown, including EACCES, EPERM, and ENOTDIR, while continuing to stop when a directory cannot be removed. Ensure the recovery flow treats worktree and branch deletion as successful even when empty-parent cleanup fails, and expose the cleanup failure for reporting.
263-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
recoveryRunsBase에서 기본 런 저장소 해석 규칙을runStore와 일치시키세요.
run-store.js는RELAY_RUNS_BASE가 없으면RELAY_HOME기본값과.relay/runs를 직접 연결합니다.recoveryRunsBase는process.env.RELAY_HOME || path.join(os.homedir(), ".relay")에 다시runs를 붙입니다. 두 기본값 해석 경로가 달라지고 있어RUNS_BASE를 노출하거나 기본 공통 헬퍼를 공유해 주세요.🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 263 - 267, Update recoveryRunsBase to use the same default runs-base resolution as runStore, preferably by reusing its shared helper or exposed RUNS_BASE value instead of constructing the path independently. Preserve the existing absolute-path validation and resolved return behavior.tests/relay-dispatch/scripts/dispatch-vnext.test.js (1)
383-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win핵심 fail-closed 조건 두 가지에 대한 테스트가 없습니다.
현재 테스트는 정상 복구, 멱등성, dirty 거부를 다룹니다.
observeStrandedWorktree의 나머지 두 안전 조건은 검증되지 않습니다.
STRANDED_WORKTREE_REFERENCED: 유효한run.json이 해당 브랜치나 worktree를 참조하면 복구는 거부해야 합니다.STRANDED_WORKTREE_AMBIGUOUS: worktree가 Relay 신뢰 base 밖에 있거나 브랜치가 예상과 다르게 체크아웃된 경우 거부해야 합니다.이 두 조건이 파괴적 삭제를 막는 주된 방어선입니다. 회귀 테스트를 추가하세요.
🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` around lines 383 - 452, Extend the stranded-worktree recovery tests around the existing post-worktree-add scenario to cover both fail-closed outcomes from observeStrandedWorktree: create a valid run.json referencing the target branch or worktree and assert recovery exits nonzero with STRANDED_WORKTREE_REFERENCED, then exercise a worktree outside Relay’s trusted base or with an unexpected branch checkout and assert STRANDED_WORKTREE_AMBIGUOUS. Keep the existing successful recovery and idempotence assertions unchanged.
🤖 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 `@skills/relay-dispatch/scripts/dispatch.js`:
- Around line 91-97: Update branchExistsMessage so the separator between the
recovery instruction and its command does not append a period after the existing
colon; use a separator such as ": " for the final two elements while preserving
the current message text and command formatting.
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 286-296: Update the run-record handling around readRunRecord in
the recovery scan so unreadable existing run.json files cannot be silently
ignored before destructive cleanup. Ignore only the explicit missing-file
condition (RUN_RECORD_MISSING), and propagate parsing, partial-record,
permission, and other read errors; preserve continuing past genuinely absent run
records.
- Around line 230-251: Update parseWorktreeList to accept porcelain attribute
records bare, detached, locked [reason], and prunable [reason] without calling
recoveryFail; ignore them or attach them to the current entry while preserving
existing validation for worktree, HEAD, and branch records.
- Around line 353-370: Serialize recoverStrandedWorktree with dispatch using the
repository-fixed lock or the existing shared serialization mechanism. Acquire it
before the initial observeStrandedWorktree call and hold it through
revalidation, worktree removal, branch deletion, and parent cleanup, ensuring
concurrent dispatch cannot recreate the branch or worktree during recovery.
In `@skills/relay/scripts/relay-recover.js`:
- Around line 105-107: Update skills/relay/scripts/relay-recover.js lines
105-107 to pass the validated reason into the recoverStrandedWorktree result
object. Update lines 62-69 so text output includes reason, worktree, and
removed_parent_directories whenever those values are present.
---
Nitpick comments:
In `@skills/relay-dispatch/references/recovery-playbook.md`:
- Around line 27-31: Update the recovery behavior description for
recoverStrandedWorktree to state that when the branch and worktree have already
been removed, it returns already_recovered and succeeds. Keep the existing
fail-closed rejection conditions unchanged.
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 344-351: Update removeEmptyRelayParents so cleanup errors are
recorded in its result rather than rethrown, including EACCES, EPERM, and
ENOTDIR, while continuing to stop when a directory cannot be removed. Ensure the
recovery flow treats worktree and branch deletion as successful even when
empty-parent cleanup fails, and expose the cleanup failure for reporting.
- Around line 263-267: Update recoveryRunsBase to use the same default runs-base
resolution as runStore, preferably by reusing its shared helper or exposed
RUNS_BASE value instead of constructing the path independently. Preserve the
existing absolute-path validation and resolved return behavior.
In `@skills/relay/scripts/relay-recover.js`:
- Around line 62-69: Update the recover_stranded_worktree text formatting branch
to include result.worktree and result.removed_parent_directories, adding each
output line only when its value is present. Preserve the existing Repository,
Branch, Operation, and Status lines and ensure the text output clearly reports
what was removed.
In `@tests/relay-dispatch/scripts/dispatch-vnext.test.js`:
- Around line 383-452: Extend the stranded-worktree recovery tests around the
existing post-worktree-add scenario to cover both fail-closed outcomes from
observeStrandedWorktree: create a valid run.json referencing the target branch
or worktree and assert recovery exits nonzero with STRANDED_WORKTREE_REFERENCED,
then exercise a worktree outside Relay’s trusted base or with an unexpected
branch checkout and assert STRANDED_WORKTREE_AMBIGUOUS. Keep the existing
successful recovery and idempotence assertions unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d786963d-8b9d-428d-a3c3-2aa838ed710c
⛔ Files ignored due to path filters (2)
tests/ledger/vnext-baseline.generated.jsonis excluded by!**/*.generated.*tests/ledger/vnext-test-sites.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (6)
skills/relay-dispatch/references/recovery-playbook.mdskills/relay-dispatch/scripts/dispatch.jsskills/relay-dispatch/scripts/recover.jsskills/relay/scripts/relay-recover.jstests/relay-dispatch/scripts/dispatch-vnext.test.jstests/relay-dispatch/scripts/docs-defaults.test.js
| function branchExistsMessage(checkout, branch) { | ||
| return [ | ||
| `branch already exists: ${branch}`, | ||
| "If dispatch was killed after its worktree was added and before run.json was created, recover only that stranded Relay worktree with:", | ||
| `node skills/relay/scripts/relay-recover.js recover --repo ${shellQuote(checkout)} --branch ${shellQuote(branch)} --reason 'remove stranded Relay worktree'`, | ||
| ].join(". "); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
오류 메시지 구두점을 정리하세요.
94번 줄 문자열은 이미 콜론으로 끝납니다. join(". ")이 그 뒤에 . 를 추가합니다. 결과 메시지는 ... recover only that stranded Relay worktree with:. node skills/...가 됩니다. 운영자가 복사하는 안내 문구이므로 구분자를 조정하세요.
✏️ 제안 수정
function branchExistsMessage(checkout, branch) {
return [
- `branch already exists: ${branch}`,
- "If dispatch was killed after its worktree was added and before run.json was created, recover only that stranded Relay worktree with:",
- `node skills/relay/scripts/relay-recover.js recover --repo ${shellQuote(checkout)} --branch ${shellQuote(branch)} --reason 'remove stranded Relay worktree'`,
- ].join(". ");
+ `branch already exists: ${branch}`,
+ "If dispatch was killed after its worktree was added and before run.json was created, recover only that stranded Relay worktree with",
+ `node skills/relay/scripts/relay-recover.js recover --repo ${shellQuote(checkout)} --branch ${shellQuote(branch)} --reason 'remove stranded Relay worktree'`,
+ ].join(": ").replace(/^(.*?): /, "$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 `@skills/relay-dispatch/scripts/dispatch.js` around lines 91 - 97, Update
branchExistsMessage so the separator between the recovery instruction and its
command does not append a period after the existing colon; use a separator such
as ": " for the final two elements while preserving the current message text and
command formatting.
| const reason = String(cli.getArg("--reason") || "").trim(); | ||
| if (!reason) throw new Error("recover requires --reason <text>"); | ||
| const record = runStore.readRunRecord({ runDir }); | ||
| const actor = String(cli.getArg("--actor") || getActorName(record.repo.root)).trim(); | ||
| result = await recoverProductionRun({ | ||
| runDir, | ||
| actor, | ||
| reason, | ||
| expectedActionKey: cli.getArg("--expected-action-key") || null, | ||
| verificationFile: cli.getArg("--verification-file") || null, | ||
| breakLock: cli.hasFlag("--break-lock"), | ||
| }); | ||
| result = recoverStrandedWorktree({ repository: path.resolve(cli.getArg("--repo")), branch }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
stranded 복구 결과 페이로드가 운영자 기록으로 불완전합니다. 두 지점의 공통 원인은 하나입니다. 결과 객체가 이 파괴적 작업의 의도와 실제 삭제 대상을 모두 담지 않습니다. --reason은 검증 후 버려지고, 반환된 worktree와 removed_parent_directories는 텍스트 출력에 나타나지 않습니다.
skills/relay/scripts/relay-recover.js#L105-L107: 검증한reason을recoverStrandedWorktree결과 객체에 포함하세요.skills/relay/scripts/relay-recover.js#L62-L69:reason,worktree,removed_parent_directories를 값이 있을 때 텍스트 출력에 추가하세요.
📍 Affects 1 file
skills/relay/scripts/relay-recover.js#L105-L107(this comment)skills/relay/scripts/relay-recover.js#L62-L69
🤖 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 `@skills/relay/scripts/relay-recover.js` around lines 105 - 107, Update
skills/relay/scripts/relay-recover.js lines 105-107 to pass the validated reason
into the recoverStrandedWorktree result object. Update lines 62-69 so text
output includes reason, worktree, and removed_parent_directories whenever those
values are present.
Reason: publish reviewed #1190 correction and changes-requested fold fix
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/relay-dispatch/scripts/inspect.js`:
- Around line 378-385: Update the post-review terminal detection around
latestPostReviewTerminal to require the corresponding attempt’s latestStart
event to occur after latestReview, and derive the terminal through
terminalForAttempt for that attempt rather than selecting any later terminal
event. Preserve the existing completed-status and hasReviewableWork checks, and
add a boundary test covering an attempt that starts before review but finishes
after it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf2f4d13-da6a-4427-bb69-2db668f974f9
⛔ Files ignored due to path filters (2)
tests/ledger/vnext-baseline.generated.jsonis excluded by!**/*.generated.*tests/ledger/vnext-test-sites.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (4)
skills/relay-dispatch/scripts/inspect.jsskills/relay-dispatch/scripts/recover.jstests/relay-dispatch/scripts/dispatch-vnext.test.jstests/relay-dispatch/scripts/run-fold-vnext.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/relay-dispatch/scripts/recover.js
| const latestPostReviewTerminal = known | ||
| .slice(known.indexOf(latestReview) + 1) | ||
| .filter((fact) => fact.type === "attempt_finished" || fact.type === "attempt_interrupted") | ||
| .at(-1) || null; | ||
| if ( | ||
| latestPostReviewTerminal?.type === "attempt_finished" | ||
| && latestPostReviewTerminal.payload.status === "completed" | ||
| && hasReviewableWork(gitFacts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
후속 attempt의 시작 시점을 확인하십시오.
현재 코드는 attempt_finished 이벤트가 latestReview 뒤에 있다는 사실만 확인합니다. 해당 attempt가 검토 전에 시작되고 검토 뒤에 종료된 경우에도 이 분기는 이를 후속 attempt로 처리합니다. 그러면 변경 요청 뒤에 새 attempt가 없는데도 publication_incomplete 복구를 반환합니다.
latestStart가 latestReview 뒤에 있는지 확인하고, 그 attempt의 terminalForAttempt만 사용하십시오. 이 순서를 재현하는 경계 테스트도 추가하십시오.
수정 예시
- const latestPostReviewTerminal = known
- .slice(known.indexOf(latestReview) + 1)
- .filter((fact) => fact.type === "attempt_finished" || fact.type === "attempt_interrupted")
- .at(-1) || null;
+ const latestPostReviewTerminal = (
+ latestStart
+ && latestStartIndex > known.indexOf(latestReview)
+ ) ? terminalForAttempt : null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const latestPostReviewTerminal = known | |
| .slice(known.indexOf(latestReview) + 1) | |
| .filter((fact) => fact.type === "attempt_finished" || fact.type === "attempt_interrupted") | |
| .at(-1) || null; | |
| if ( | |
| latestPostReviewTerminal?.type === "attempt_finished" | |
| && latestPostReviewTerminal.payload.status === "completed" | |
| && hasReviewableWork(gitFacts) | |
| const latestPostReviewTerminal = ( | |
| latestStart | |
| && latestStartIndex > known.indexOf(latestReview) | |
| ) ? terminalForAttempt : null; | |
| if ( | |
| latestPostReviewTerminal?.type === "attempt_finished" | |
| && latestPostReviewTerminal.payload.status === "completed" | |
| && hasReviewableWork(gitFacts) |
🤖 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 `@skills/relay-dispatch/scripts/inspect.js` around lines 378 - 385, Update the
post-review terminal detection around latestPostReviewTerminal to require the
corresponding attempt’s latestStart event to occur after latestReview, and
derive the terminal through terminalForAttempt for that attempt rather than
selecting any later terminal event. Preserve the existing completed-status and
hasReviewableWork checks, and add a boundary test covering an attempt that
starts before review but finishes after it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f45e3ae005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| latestPostReviewTerminal?.type === "attempt_finished" | ||
| && latestPostReviewTerminal.payload.status === "completed" | ||
| && hasReviewableWork(gitFacts) |
There was a problem hiding this comment.
When a post-review executor exits successfully without changing the worktree, this condition still evaluates true in production because observeProduction defines reviewable_work and branch_commit_exists as headSha !== runRecord.git.start_sha (recover.js lines 794-797); any previously published PR normally satisfies that comparison. The run is therefore misclassified as publication_incomplete, recovery only no-ops against the already-recorded PR, and subsequent inspections never return to redispatch even though the requested changes remain unresolved. This check needs to detect work produced after the review, rather than any commit made since the run began.
Useful? React with 👍 / 👎.
Reason: publish review round 2 race correction
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79a14add26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || current.branchHead !== initial.branchHead || current.canonicalHead !== initial.canonicalHead) { | ||
| recoveryFail("STRANDED_WORKTREE_CHANGED", "stranded worktree changed while recovery was revalidating it"); | ||
| } | ||
| execGit(current.checkout, ["worktree", "remove", current.worktree]); |
There was a problem hiding this comment.
If git branch -d fails transiently after this removal—for example because the ref lock is briefly contended—the command reports STRANDED_BRANCH_NOT_REMOVED, but a retry can never converge: observeStrandedWorktree now finds the surviving branch with zero registered holders and returns STRANDED_WORKTREE_NOT_FOUND. The branch therefore keeps blocking dispatch until an operator bypasses the recovery surface and deletes it manually, contrary to the documented idempotent recovery shape. Preserve enough authority to resume the branch-only intermediate state, or avoid removing the worktree before branch deletion can be completed.
AGENTS.md reference: AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
Reason: publish #1190 recovery-loop scope clarification
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b4e98c3e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { record = readRunRecord({ runDir }); } | ||
| catch { continue; } |
There was a problem hiding this comment.
When a valid run references this branch or worktree but readRunRecord fails because of a permission, transient I/O, or immutable-artifact validation error, this blanket catch silently treats the run as nonexistent. Both observations can then pass and delete the run's clean worktree and branch, leaving the immutable run stranded; ignore only the explicitly expected no-record case and propagate other failures so recovery fails closed.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
Reason: publish review round 4 fail-closed corrections
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f6c273d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // An unclaimed directory or an old/structurally-invalid record cannot own this | ||
| // stranded worktree. Everything else is an inspection failure, not evidence of | ||
| // absence, and must stop recovery before it deletes Git state. | ||
| if (IGNORABLE_RUN_RECORD_CODES.has(error?.code)) continue; |
There was a problem hiding this comment.
When a published run's run.json is truncated or otherwise fails schema validation, readRunRecord reports INVALID_RUN_RECORD, this new whitelist treats it as no ownership claim, and cleanup may delete that run's clean worktree and branch. The fresh evidence after the earlier blanket-catch review is that the replacement explicitly includes INVALID_RUN_RECORD and RUN_ID_PATH_MISMATCH; only a genuinely missing, provably unrelated record should be ignorable, while validation failures for a candidate that could own the target must fail closed.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| recoveryFail("STRANDED_BRANCH_NOT_REMOVED", `worktree was removed but branch ${current.branch} was not deleted safely: ${commandFailure(error)}`); | ||
| } | ||
| const removed = removeEmptyRelayParents(current.base, current.worktree); |
There was a problem hiding this comment.
When dispatch is killed after fs.mkdirSync(runDir) in startAttempt but before createRunRecord, stranded recovery ignores the resulting RUN_RECORD_MISSING directory, removes only the worktree-side parents here, and nevertheless reports recovered. The claimed directory under RELAY_RUNS_BASE remains permanently unreadable and causes every retry with the same run ID to fail the duplicate-run check, so recovery needs to identify and remove the matching unpublished claim as well.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
Reason: publish review round 5 TOCTOU corrections
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
skills/relay-dispatch/scripts/recover.js (1)
460-467: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift재검증이 잠금 없이 수행됩니다.
observeStrandedWorktree를 destructive 작업 직전에 반복하지만, 검증과worktree remove사이에 잠금이 없습니다. 동시dispatch가 그 사이에 같은 브랜치로 worktree를 만들면 재검증 결과가 무효화됩니다.host.withRunLock이나 repository 고정 잠금을 획득한 상태에서 재검증부터 branch 삭제까지 유지하세요.코딩 가이드라인에 따라 "Inspect before a write, then re-inspect under the run lock using the same action key"와 "Use
host.withRunLockfor host locks" 규칙을 적용해야 합니다.🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 460 - 467, Protect the final recovery validation and destructive cleanup with host.withRunLock using the same action key as dispatch. Acquire the lock before re-running observeStrandedWorktree, keep it held through the ownership checks, worktree removal, and branch deletion, and preserve the existing STRANDED_WORKTREE_CHANGED failure behavior for mismatches.Source: Coding guidelines
🧹 Nitpick comments (3)
tests/relay-dispatch/scripts/dispatch-vnext.test.js (2)
614-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value환경 변수 저장·복원 보일러플레이트를 헬퍼로 묶는 것을 검토하세요.
두 테스트가 동일한 저장·설정·복원 패턴을 반복합니다. 변수 하나가 추가될 때마다 6줄이 늘어납니다.
withEnv(overrides, fn)같은 헬퍼로 묶으면 누락된 복원으로 인한 테스트 간섭 위험도 줄어듭니다.♻️ 제안 헬퍼
function withEnv(overrides, fn) { const previous = new Map(Object.keys(overrides).map((key) => [key, process.env[key]])); Object.assign(process.env, overrides); try { return fn(); } finally { for (const [key, value] of previous) { if (value === undefined) delete process.env[key]; else process.env[key] = value; } } }Also applies to: 686-705
🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` around lines 614 - 634, Extract the repeated environment-variable save, assignment, and restoration logic used around recovery.recoverStrandedWorktree into a shared withEnv(overrides, fn) helper. Update both affected test blocks to pass their environment overrides and assertions through this helper, preserving restoration of undefined variables by deleting them and restoring existing values afterward.
640-640: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value에러 검증자를 전달해 실패 원인을 좁히세요.
assert.throws(fn[, error][, message])에서 두 번째 인자가 문자열이면 검증자가 생략된 것으로 간주되어 메시지로 처리됩니다. 이 문장은 어떤 실패든 통과하게 합니다. 저장된 복원 경로의git symbolic-ref가 경쟁 지점의 브랜치를 가져오지 않아야 한다면 실패 상태와 관련된 에러 검증을 함께 사용하세요.♻️ 제안 변경
- assert.throws(() => git(worktree, ["symbolic-ref", "--quiet", "--short", "HEAD"]), "the restored path must not steal the competing holder's branch"); + assert.throws( + () => git(worktree, ["symbolic-ref", "--quiet", "--short", "HEAD"]), + (error) => error.status !== 0, + "the restored path must not steal the competing holder's branch", + );🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` at line 640, Update the assert.throws call for the restored path in the relay dispatch test to provide an actual error validator instead of passing a string as the second argument. Verify the git symbolic-ref failure status and retain a message that clearly identifies the competing holder branch condition.skills/relay-dispatch/scripts/recover.js (1)
267-274: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
INVALID_WORKTREE_REGISTRY는 복구 경로에서 과도하게 실패를 유발합니다.
parseWorktreeList가 Git porcelain의 필드 순서를HEAD → branch/detached/bare → locked → prunable로 강제하고, 지원하지 않는 속성 라인이 있으면 복구 전체를 실패합니다. 식별자 중복과 worktree identity 정합성 검증은 유지하고, 순서 검증은 완화하거나 Git 버전별 출력을 기준으로 조정하세요.🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 267 - 274, Update parseWorktreeList to stop failing recovery for valid Git-version-specific field ordering or unsupported attribute lines, while retaining duplicate identifier checks and worktree identity consistency validation. Relax the order validation around the order and prior checks, and handle unrecognized fields without triggering INVALID_WORKTREE_REGISTRY unless they violate the retained identity constraints.
🤖 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 `@skills/relay-dispatch/scripts/recover.js`:
- Around line 31-38: Update the recovery classification around
IGNORABLE_RUN_RECORD_CODES so INVALID_RUN_RECORD is not always ignored: inspect
the raw run.json bytes before treating it as ignorable, and fail closed when
they contain the target branch or worktree path. Remove RUN_ID_PATH_MISMATCH
from the ignorable set so that record remains a protective ownership claim,
while preserving existing handling for genuinely missing or unsupported records.
---
Duplicate comments:
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 460-467: Protect the final recovery validation and destructive
cleanup with host.withRunLock using the same action key as dispatch. Acquire the
lock before re-running observeStrandedWorktree, keep it held through the
ownership checks, worktree removal, and branch deletion, and preserve the
existing STRANDED_WORKTREE_CHANGED failure behavior for mismatches.
---
Nitpick comments:
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 267-274: Update parseWorktreeList to stop failing recovery for
valid Git-version-specific field ordering or unsupported attribute lines, while
retaining duplicate identifier checks and worktree identity consistency
validation. Relax the order validation around the order and prior checks, and
handle unrecognized fields without triggering INVALID_WORKTREE_REGISTRY unless
they violate the retained identity constraints.
In `@tests/relay-dispatch/scripts/dispatch-vnext.test.js`:
- Around line 614-634: Extract the repeated environment-variable save,
assignment, and restoration logic used around recovery.recoverStrandedWorktree
into a shared withEnv(overrides, fn) helper. Update both affected test blocks to
pass their environment overrides and assertions through this helper, preserving
restoration of undefined variables by deleting them and restoring existing
values afterward.
- Line 640: Update the assert.throws call for the restored path in the relay
dispatch test to provide an actual error validator instead of passing a string
as the second argument. Verify the git symbolic-ref failure status and retain a
message that clearly identifies the competing holder branch condition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef346183-f2af-43fe-bc24-7c99e0543c3a
⛔ Files ignored due to path filters (2)
tests/ledger/vnext-baseline.generated.jsonis excluded by!**/*.generated.*tests/ledger/vnext-test-sites.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (5)
skills/relay-dispatch/scripts/dispatch.jsskills/relay-dispatch/scripts/inspect.jsskills/relay-dispatch/scripts/recover.jstests/relay-dispatch/scripts/dispatch-vnext.test.jstests/relay-dispatch/scripts/run-fold-vnext.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/relay-dispatch/scripts/dispatch.js
- tests/relay-dispatch/scripts/run-fold-vnext.test.js
- skills/relay-dispatch/scripts/inspect.js
| // These are the only candidate records that are positively known not to be a | ||
| // usable vNext ownership claim. All read/trust failures fail closed instead. | ||
| const IGNORABLE_RUN_RECORD_CODES = new Set([ | ||
| "RUN_RECORD_MISSING", | ||
| "INVALID_RUN_RECORD", | ||
| "UNSUPPORTED_RUN_VERSION", | ||
| "RUN_ID_PATH_MISMATCH", | ||
| ]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
INVALID_RUN_RECORD를 무조건 무시하면 부분 기록된 run.json이 파괴적 삭제를 막지 못합니다.
이 복구 경로가 다루는 상황은 dispatch가 run 기록을 쓰는 도중 종료된 창입니다. 그 창에서 만들어진 잘린 run.json은 파싱에 실패하고 INVALID_RUN_RECORD로 분류됩니다. 그러면 후보에서 제외되고, recoverStrandedWorktree가 branch와 worktree를 삭제합니다. RUN_ID_PATH_MISMATCH도 기록 자체는 유효한 소유 주장일 수 있습니다.
INVALID_RUN_RECORD는 무시하기 전에 원시 run.json 바이트가 대상 branch 또는 worktree 경로를 포함하는지 확인하고, 포함하면 fail-closed로 거부하세요.
🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 31 - 38, Update the
recovery classification around IGNORABLE_RUN_RECORD_CODES so INVALID_RUN_RECORD
is not always ignored: inspect the raw run.json bytes before treating it as
ignorable, and fail closed when they contain the target branch or worktree path.
Remove RUN_ID_PATH_MISMATCH from the ignorable set so that record remains a
protective ownership claim, while preserving existing handling for genuinely
missing or unsupported records.
Reason: publish review round 6 atomic ref correction
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2fdd32fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isAncestor(checkout, branchHead, canonicalHead)) { | ||
| recoveryFail("STRANDED_WORKTREE_UNMERGED", `branch ${branch} has committed work that is not reachable from the canonical checkout HEAD`); | ||
| } | ||
| const status = gitBytes(worktree, ["--no-optional-locks", "status", "--porcelain=v1", "-z", "--untracked-files=all"]); |
There was a problem hiding this comment.
When the worktree contains an ignored file—for example, one generated by a post-checkout hook—this status call reports no dirt because it omits --ignored (git status -h documents that option as “show ignored files”). git worktree remove nevertheless recursively deletes ignored files without requiring --force, so recovery can silently destroy local artifacts while treating the worktree as clean. Include ignored entries in the cleanliness check before allowing removal.
Useful? React with 👍 / 👎.
Reason: publish review round 7 hidden-data correction
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d35379150
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ignored = safeWorktreeEvidencePaths(worktree, | ||
| parseNulGitPaths(gitBytes(worktree, ["--no-optional-locks", "ls-files", "--others", "--ignored", "--exclude-standard", "-z"]), "git ls-files --ignored"), | ||
| "git ls-files --ignored"); | ||
| indexFlags = parseIndexFlagEntries(gitBytes(worktree, ["--no-optional-locks", "ls-files", "-v", "-z"])); |
There was a problem hiding this comment.
When a tracked path is marked fsmonitor-valid and a stale or misconfigured core.fsmonitor reports no change, git status is empty and this ls-files -v call reports uppercase H, so hiddenIndexFlags misses the modified file; git worktree remove can then trust the same monitor and delete the uncommitted bytes. I reproduced this by marking a file with update-index --fsmonitor-valid, modifying it, and observing recovery return recovered after deleting it. git ls-files -h documents -f as “use lowercase letters for 'fsmonitor clean' files”; detect those entries or disable/refresh fsmonitor for the safety proof.
Useful? React with 👍 / 👎.
Reason: publish review round 8 stranded-worktree safety corrections
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/relay-dispatch/scripts/recover.js (1)
475-479: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
symbolic-ref실패가 typed 복구 오류로 변환되지 않습니다.475행은 worktree의 checked-out 브랜치를 조회합니다. 경쟁으로 worktree가 detached 상태가 되면 이 명령은 0이 아닌 상태로 종료합니다. 그러면
execFileSync의 원시 오류가 그대로 전파되고, CLI는code없는 오류를 출력합니다. 다른 모든 실패 경로는recoveryFail로 typed code를 부여합니다. 이 경로도 동일하게 처리하세요. 삭제는 발생하지 않으므로 fail-closed는 유지됩니다.🛡️ 제안 수정
- const checkedOutBranch = execGit(worktree, ["symbolic-ref", "--quiet", "--short", "HEAD"]); + let checkedOutBranch; + try { checkedOutBranch = execGit(worktree, ["symbolic-ref", "--quiet", "--short", "HEAD"]); } + catch { recoveryFail("STRANDED_WORKTREE_AMBIGUOUS", `registered worktree is not checked out on a branch`); } if (checkedOutBranch !== branch) recoveryFail("STRANDED_WORKTREE_AMBIGUOUS", `registered worktree is not checked out on ${branch}`);🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 475 - 479, Update the checkedOutBranch lookup in the recovery validation flow to catch a failed symbolic-ref command and convert it through recoveryFail using the appropriate typed recovery code, preserving fail-closed behavior and avoiding any deletion. Keep the existing branch-mismatch recoveryFail path unchanged for successful lookups.
🧹 Nitpick comments (3)
skills/relay-dispatch/scripts/recover.js (3)
336-342: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value지역 변수
value가 함수 파라미터value를 섀도잉합니다.338행의
value는 309행 파라미터와 이름이 같습니다. 파라미터는 310행에서bytes로 변환된 뒤 다시 사용되지 않으므로 현재 동작은 정확합니다. 다만 이후 수정에서 혼동을 유발할 수 있습니다. 필드 값 변수 이름을fieldValue로 바꾸세요.🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 336 - 342, Rename the local variable value in the line-parsing logic to fieldValue to avoid shadowing the recover function’s value parameter, and update all references in the match, flag, and annotation expressions accordingly.
396-399: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winretaining ref 선택에 로케일 비의존 정렬을 사용하세요.
398행은
localeCompare로 후보를 정렬합니다.localeCompare의 결과는 런타임 ICU 데이터와 로케일에 따라 달라집니다. 선택된 ref는 671-678행의verify대상이 되므로, 선택 규칙은 결정적이어야 합니다. 코드포인트 비교로 바꾸세요.♻️ 제안 리팩터
- candidates.sort((left, right) => left.ref.localeCompare(right.ref)); + candidates.sort((left, right) => (left.ref < right.ref ? -1 : left.ref > right.ref ? 1 : 0));🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 396 - 399, Update the candidate sorting in the recovery flow around candidates.sort to use a locale-independent deterministic code-point comparison of left.ref and right.ref instead of localeCompare, preserving the existing candidates[0] selection and null fallback.
144-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
UNSAFE_WORKTREE_ENTRY오류 코드가 catch에서 삼켜집니다.
safeWorktreeEvidencePaths는 등록된 worktree 밖 경로를 만나면UNSAFE_WORKTREE_ENTRY를 던집니다. 이 호출이 147-152행에서 try 블록 안에 있으므로, 해당 오류는STRANDED_WORKTREE_SAFETY_PROOF_FAILED로 재분류됩니다. 두 경우 모두 fail-closed이므로 안전성은 유지됩니다. 다만 운영자가 "Git 명령 실패"와 "containment 위반"을 구분할 수 없습니다. containment 위반 코드는 그대로 전파하세요.♻️ 제안 리팩터
} catch (error) { + if (error.code === "UNSAFE_WORKTREE_ENTRY") throw error; recoveryFail("STRANDED_WORKTREE_SAFETY_PROOF_FAILED", `could not prove stranded worktree is safe to remove: ${commandFailure(error)}`); }🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 144 - 155, Update the catch handling around the stranded-worktree safety checks so errors with code UNSAFE_WORKTREE_ENTRY are rethrown unchanged, while all other failures continue to be reported through recoveryFail with STRANDED_WORKTREE_SAFETY_PROOF_FAILED. Preserve the existing fail-closed behavior and use the visible safeWorktreeEvidencePaths and recoveryFail flow.
🤖 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 `@skills/relay-dispatch/scripts/recover.js`:
- Around line 551-557: Update restoreMissingStrandedBranch to perform
conditional ref creation through git update-ref --stdin using the create
command, rather than passing ZERO_OID as the expected old value. Preserve the
behavior that an existing or concurrently recreated ref is not overwritten, and
remove the obsolete ZERO_OID dependency if no longer used.
In `@tests/relay-dispatch/scripts/dispatch-vnext.test.js`:
- Around line 874-896: Replace the 40-character hash-length assertions in
tests/relay-dispatch/scripts/dispatch-vnext.test.js lines 874-896 and 898-923
with branch-existence verification that works for both SHA-1 and SHA-256
repositories, preserving the requirement that the branch survives recovery.
Update the assertions around the branch checks without changing the worktree or
ignored-content expectations.
---
Outside diff comments:
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 475-479: Update the checkedOutBranch lookup in the recovery
validation flow to catch a failed symbolic-ref command and convert it through
recoveryFail using the appropriate typed recovery code, preserving fail-closed
behavior and avoiding any deletion. Keep the existing branch-mismatch
recoveryFail path unchanged for successful lookups.
---
Nitpick comments:
In `@skills/relay-dispatch/scripts/recover.js`:
- Around line 336-342: Rename the local variable value in the line-parsing logic
to fieldValue to avoid shadowing the recover function’s value parameter, and
update all references in the match, flag, and annotation expressions
accordingly.
- Around line 396-399: Update the candidate sorting in the recovery flow around
candidates.sort to use a locale-independent deterministic code-point comparison
of left.ref and right.ref instead of localeCompare, preserving the existing
candidates[0] selection and null fallback.
- Around line 144-155: Update the catch handling around the stranded-worktree
safety checks so errors with code UNSAFE_WORKTREE_ENTRY are rethrown unchanged,
while all other failures continue to be reported through recoveryFail with
STRANDED_WORKTREE_SAFETY_PROOF_FAILED. Preserve the existing fail-closed
behavior and use the visible safeWorktreeEvidencePaths and recoveryFail flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1440534b-7164-4f2f-9c9e-85c1c6351a8e
⛔ Files ignored due to path filters (2)
tests/ledger/vnext-baseline.generated.jsonis excluded by!**/*.generated.*tests/ledger/vnext-test-sites.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (2)
skills/relay-dispatch/scripts/recover.jstests/relay-dispatch/scripts/dispatch-vnext.test.js
| const ZERO_OID = "0".repeat(40); | ||
|
|
||
| function restoreMissingStrandedBranch(current) { | ||
| // Do not overwrite a ref a concurrent user recreated or moved. The all-zero | ||
| // old value makes this creation conditional on the ref still being absent. | ||
| try { | ||
| execGit(current.checkout, ["update-ref", current.ref, current.branchHead, ZERO_OID]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
git update-ref stdin create command vs zero oid old value sha256 repository
💡 Result:
In the context of the git update-ref --stdin command, the create and update subcommands serve distinct purposes regarding the use of zero object IDs (OIDs) [1][2]. The create command is explicitly designed to create a new reference after verifying that it does not already exist [1][2]. Consequently, the provided to create may not be a zero OID, as creating a reference that points to a non-existent (zero) object is generally prohibited by this command [1][3][2]. In contrast, the update command is more flexible [4]. It can be used to set a reference to a new value after optionally verifying its current value [1][2]. Historically, users could create or delete references using the update command by specifying an all-zeroes OID (e.g., 40 zeros for SHA-1 or the equivalent for SHA-256) as the new or old value [4]. Specifically: 1. Zero OID as : Using a zero value for the old OID ensures that the reference does not exist before the update [1][2]. 2. Zero OID as : Using a zero value for the new OID (or omitting it in a delete context) is interpreted as a request to delete the reference [4]. While the update command supports these behaviors, the create command was introduced specifically to provide a more symmetric and intuitive way to create references without requiring users to manually manage zero OIDs [4]. When using the --stdin interface, you can specify a zero value by using either 40 "0" characters (for SHA-1) or the empty string, depending on the specific implementation requirements for your repository's hash algorithm [1][2]. Top results: [1][2][4]
Citations:
- 1: https://git-scm.com/docs/git-update-ref
- 2: https://github.com/git/git/blob/440c705ea63253642c8f4761f80eb7a97a62b614/Documentation/git-update-ref.txt
- 3: https://www.kernel.org/pub/software/scm/git/docs/git-update-ref.html
- 4: git/git@fa4eefe
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Git availability:\n'
git --version || true
if git --version >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
chmod 777 "$tmpdir"
for object_format in sha1 sha256; do
printf '\n--- repository object-format=%s ---\n' "$object_format"
rm -rf "$tmpdir/repo"
mkdir -p "$tmpdir/repo"
git -C "$tmpdir/repo" init --quiet --object-format="$object_format"
git -C "$tmpdir/repo" commit-tree -m init "$(git hash-object -t tree /dev/null)"
git -C "$tmpdir/repo" symbolic-ref HEAD refs/heads/main
branch_sha="$(git -C "$tmpdir/repo" rev-parse "refs/heads/test")"
printf 'expected test SHA length for oid=%s: %s\n' "$object_format" "${`#branch_sha`}"
printf 'try 40 zero old value: '
if git -C "$tmpdir/repo" update-ref refs/heads/test "$branch_sha" "$(printf '%0*s' 40 0)" >/tmp/git_update_$$ 2>&1; then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/test
else
echo FAIL; sed -n '1,80p' /tmp/git_update_$$
fi
printf 'try actual hash-length zero value: '
zero="$(printf '%0*s' "${`#branch_sha`}" 0)"
if git -C "$tmpdir/repo" update-ref refs/heads/test "$branch_sha" "$zero" >/tmp/git_update_$$ 2>&1; then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/test
else
echo FAIL; sed -n '1,80p' /tmp/git_update_$$
fi
printf 'try stdin create command: '
if git -C "$tmpdir/repo" update-ref --stdin >/tmp/git_update_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,80p' /tmp/git_update_$$
fi
done
rm -f /tmp/git_update_$$
rm -rf "$tmpdir"
fi
printf '\nRelevant repository references:\n'
printf 'recover.js exists:'
[ -f skills/relay-dispatch/scripts/recover.js ] && echo yes || echo no
if [ -f skills/relay-dispatch/scripts/recover.js ]; then
wc -l skills/relay-dispatch/scripts/recover.js
rg -n "ZERO_OID|restoreMissingStrandedBranch|independentRetainingRef|update-ref|branchHead" skills/relay-dispatch/scripts/recover.js
printf '\nRelevant lines 360-635:\n'
sed -n '360,635p' skills/relay-dispatch/scripts/recover.js | nl -ba -v360
fiRepository: sungjunlee/dev-relay
Length of output: 572
🌐 Web query:
git documentation git-update-ref update zero oid old value create command stdin create not exist
💡 Result:
In the context of the git-update-ref command, especially when using the --stdin interface, you can ensure that a reference does not exist before performing an operation by using a zero OID (40 zeros) or an empty string [1][2]. When using the --stdin command format, the create command is specifically designed to create a new reference after verifying that it does not already exist [1][2]. According to the official documentation, the provided to the create command may not be zero [2][3]. For other commands (like update or delete) that accept an optional , you can specify a zero OID (40 "0" characters) or an empty string to ensure that the reference does not exist before the update or deletion takes place [1][4][2]. Summary of --stdin command behavior: 1. create : Creates a new reference. The operation fails if the reference already exists. The itself cannot be a zero OID [2][3]. 2. Verifying non-existence: To verify a reference does not exist, use the verify command with an empty string or 40 "0" characters as the , or rely on the inherent non-existence check built into the create command [1][2]. When providing these values in --stdin mode: - Quote fields containing whitespace as if they were strings in C source code (surrounded by double-quotes with backslash escapes) [2][5]. - If using the -z (NUL-terminated) format, use the empty string or 40 "0" characters directly, as quoting is not used in that mode [2][3].
Citations:
- 1: https://git-scm.com/docs/git-update-ref
- 2: https://git-scm.com/docs/git-update-ref.html
- 3: https://github.com/git/git/blob/440c705ea63253642c8f4761f80eb7a97a62b614/Documentation/git-update-ref.txt
- 4: https://git.github.io/htmldocs/git-update-ref.html
- 5: http://linuxman7.org/linux/man-pages/man1/git-update-ref.1.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
trap 'cd "$(mktemp -d)" ; git init -q --bare repo.git; cd repo.git ; exec "$BASH" <<ENDSCRIPT' EXIT
git init -q --bare repo.git
cd repo.git
printf 'Git availability:\n'
git --version
for object_format in sha1 sha256; do
printf '\n--- repository object-format=%s ---\n' "$object_format"
git init -q --bare --object-format="$object_format" repo.git
cd repo.git
tree_obj="$(git hash-object -t tree /dev/null)"
commit_obj="$(echo init | git commit-tree "$tree_obj")"
git update-ref refs/heads/main "$commit_obj"
branch_sha="$(git rev-parse refs/heads/test 2>&1 || true)"
printf 'expected test SHA length for oid=%s: %d\n' "$object_format" "${`#branch_sha`}"
printf 'try 40 zero old value: '
if git update-ref refs/heads/test "$branch_sha" "$(printf '%0*s' 40 0)" >/tmp/git_update_$$ 2>&1; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,12p' /tmp/git_update_$$
fi
printf 'try actual hash-length zero value: '
zero="$(printf '%0*s' "${`#branch_sha`}" 0)"
if git update-ref refs/heads/test "$branch_sha" "$zero" >/tmp/git_update_$$ 2>&1; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,12p' /tmp/git_update_$$
fi
printf 'try stdin create command for non-existing ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,12p' /tmp/git_update_stdin_$$
fi
printf 'try stdin update-existing command for already-existing proposed ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
update refs/heads/proposed 0000000000000000000000000000000000000000 "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,12p' /tmp/git_update_stdin_$$
fi
printf 'try stdin create command for already-existing proposed ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,12p' /tmp/git_update_stdin_$$
fi
cd /tmp
done
rm -f /tmp/git_update_$$ /tmp/git_update_stdin_$$Repository: sungjunlee/dev-relay
Length of output: 661
🌐 Web query:
Git source parse_oid_hex update-ref invalid oid length fatal invalid old value
💡 Result:
The errors you are encountering, such as invalid oid length or invalid old value when using git update-ref, typically stem from providing malformed object IDs or incorrect reference state expectations to the command [1][2]. The function parse_oid_hex is an internal Git helper used to parse hexadecimal object IDs from strings [3][4]. A fatal error involving this function or invalid oid length usually indicates that the input provided as an OID (e.g., a hash or a ref name) is either not a valid 40-character hexadecimal string or is otherwise not recognized as a valid object name by Git in the current context [1][5][3]. When using git update-ref, specifically with the --stdin or --batch-updates modes, the command requires strict adherence to its input format [1][5]. The fatal invalid old value error occurs when you provide an (a verification value) that does not match the actual current state of the reference [6][7]. To troubleshoot and resolve these errors: 1. Validate your OIDs: Ensure any OIDs passed to git update-ref are valid, full 40-character SHA-1 hashes (or the equivalent length if using SHA-256) [1][5][3]. If you are using a placeholder, use 40 zeros to specify a zero value (indicating the ref should not exist) [1][5]. 2. Check Reference State: The fatal invalid old value error often means you are attempting an update assuming the reference points to a specific commit, but it currently points to something else [7]. Use git rev-parse to verify the actual current value of the reference before attempting the update. 3. Batch Mode Strictness: If using --stdin, ensure each command is correctly formatted [1]. Malformed inputs (e.g., missing spaces, invalid characters) will cause the command to fail [1][2]. If working with batch updates, remember that system-level failures will halt the entire batch, whereas individual input errors may be reported individually if --batch-updates is enabled [1][5]. 4. Corruption Check: If you suspect repository corruption (e.g., errors regarding bad objects), verify your object database with git fsck [8]. If objects are missing or corrupt, you may need to re-fetch the references from your remote repository [8]. If you are writing scripts that interface with update-ref, ensure your input strings are properly sanitized and that you are checking the return status of your commands to handle partial failures gracefully [1][9].
Citations:
- 1: https://git-scm.com/docs/git-update-ref
- 2: https://github.com/git/git/blob/ad1641bbcd3c56bafe85ff7e56606b8aa0169211/builtin/update-ref.c
- 3: https://public-inbox.org/git/xmqqy3wyawlu.fsf@gitster.mtv.corp.google.com/T/
- 4: https://code.googlesource.com/git/+/HEAD/hex.c
- 5: https://www.kernel.org/pub/software/scm/git/docs/git-update-ref.html
- 6: https://public-inbox.org/git/1396878498-19887-12-git-send-email-mhagger@alum.mit.edu/
- 7: https://code.googlesource.com/git/+/HEAD/t/t1404-update-ref-errors.sh
- 8: https://swyx.io/solve-git-bad-object-head
- 9: https://www.spinics.net/lists/git/msg516652.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git config --global user.email "coderabbit@example.com"
git config --global user.name "CodeRabbit"
tmpdir="$(mktemp -d)"
chmod 777 "$tmpdir"
for object_format in sha1 sha256; do
printf '\n--- repository object-format=%s ---\n' "$object_format"
rm -rf "$tmpdir/repo"
git -C "$tmpdir" init -q --bare --object-format="$object_format" repo.git
cd "$tmpdir/repo"
tree_obj="$(git hash-object -t tree /dev/null)"
branch_sha="$(git commit-tree "$tree_obj" -m init)"
git update-ref refs/heads/main "$branch_sha"
printf 'expected branch SHA length for object-format=%s: %d\n' "$object_format" "${`#branch_sha`}"
printf 'try 40 zero old value: '
if git update-ref refs/heads/test "$branch_sha" "$(printf '%0*s' 40 0)" >/tmp/git_update_$$ 2>&1; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try actual hash-length zero value: '
zero="$(printf '%0*s' "${`#branch_sha`}" 0)"
if git update-ref refs/heads/test "$branch_sha" "$zero" >/tmp/git_update_$$ 2>&1; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try stdin create command for non-existing ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
printf 'try stdin create command for already-existing proposed ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
cd /tmp
done
rm -f /tmp/git_update_$$ /tmp/git_update_stdin_$$
rm -rf "$tmpdir"Repository: sungjunlee/dev-relay
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
chmod 777 "$tmpdir"
for object_format in sha1 sha256; do
printf '\n--- repository object-format=%s ---\n' "$object_format"
rm -rf "$tmpdir/repo"
git -C "$tmpdir" init --bare --object-format="$object_format" repo.git
echo "repo path: $(ls -la "$tmpdir/repo" | head -n 3)"
mkdir "$tmpdir/work" && cd "$tmpdir/work"
git -C "$tmpdir/repo" symbolic-ref refs/heads/main "$(git -C "$tmpdir/repo" commit-tree "$(git -C "$tmpdir/repo" hash-object -t tree /dev/null)" -m init)"
branch_sha="$(git -C "$tmpdir/repo" rev-parse --verify refs/heads/main)"
printf 'expected branch SHA length for object-format=%s: %d: %s\n' "$object_format" "${`#branch_sha`}" "$branch_sha"
printf 'try 40 zero old value: '
if git -C "$tmpdir/repo" update-ref refs/heads/test "$branch_sha" "$(printf '%0*s' 40 0)" 2>/tmp/git_update_$$; then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try actual hash-length zero value: '
zero="$(printf '%0*s' "${`#branch_sha`}" 0)"
if git -C "$tmpdir/repo" update-ref refs/heads/test "$branch_sha" "$zero" 2>/tmp/git_update_$$; then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try stdin create command for non-existing ref: '
if git -C "$tmpdir/repo" update-ref --stdin 2>/tmp/git_update_stdin_$$ <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
printf 'try stdin create command for already-existing proposed ref: '
if git -C "$tmpdir/repo" update-ref --stdin 2>/tmp/git_update_stdin_$$ <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git -C "$tmpdir/repo" rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
cd /tmp
done
rm -f /tmp/git_update_$$ /tmp/git_update_stdin_$$ >/dev/null 2>&1
rm -rf "$tmpdir"Repository: sungjunlee/dev-relay
Length of output: 1153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
chmod 777 "$tmpdir"
cd "$tmpdir"
for object_format in sha1 sha256; do
printf '\n--- repository object-format=%s ---\n' "$object_format"
rm -rf repo
git init --bare --object-format="$object_format" repo
cd repo
branch_sha="$(git commit-tree "$(git hash-object -t tree /dev/null)" -m init)"
printf 'expected branch SHA length for object_format=%s: %d: %s\n' "$object_format" "${`#branch_sha`}" "$branch_sha"
printf 'try 40 zero old value: '
if git update-ref refs/heads/test "$branch_sha" "$(printf '%0*s' 40 0)" >/dev/null 2>/tmp/git_update_$$; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try actual hash-length zero value: '
zero="$(printf '%0*s' "${`#branch_sha`}" 0)"
if git update-ref refs/heads/test "$branch_sha" "$zero" >/dev/null 2>/tmp/git_update_$$; then
echo OK; git rev-parse refs/heads/test
else
echo FAIL; sed -n '1,8p' /tmp/git_update_$$
fi
printf 'try stdin create command for non-existing ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
printf 'try stdin create command for already-existing proposed ref: '
if git update-ref --stdin >/tmp/git_update_stdin_$$ 2>&1 <<EOF
start
create refs/heads/proposed "$branch_sha"
prepare
commit
EOF
then
echo OK; git rev-parse refs/heads/proposed
else
echo FAIL; sed -n '1,8p' /tmp/git_update_stdin_$$
fi
cd "$tmpdir"
done
rm -f /tmp/git_update_$$ /tmp/git_update_stdin_$$ >/dev/null 2>&1
rm -rf "$tmpdir"Repository: sungjunlee/dev-relay
Length of output: 2672
update-ref의 null OID 비교 방식을 사용하지 않도록 고치세요.
ZERO_OID는 SHA-1/SHA-256 저장소 모두에서 Git이 유효한 old value로 받아들이지 않아 update-ref가 실패합니다. 보상이 실패하면 restoreMissingStrandedBranch가 STRANDED_WORKTREE_CLEANUP_INCOMPLETE 경로로 떨어질 수 있습니다. update-ref --stdin의 create 명령은 ref 존재 여부가 없는 경우에만 실패하므로 같은 조건부 생성 목적에 부합합니다. 이 코드는 현재 동작하지 않는 null OID 비교가 포함된 상태입니다.
🤖 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 `@skills/relay-dispatch/scripts/recover.js` around lines 551 - 557, Update
restoreMissingStrandedBranch to perform conditional ref creation through git
update-ref --stdin using the create command, rather than passing ZERO_OID as the
expected old value. Preserve the behavior that an existing or concurrently
recreated ref is not overwritten, and remove the obsolete ZERO_OID dependency if
no longer used.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5e2f5d21f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| const ZERO_OID = "0".repeat(40); |
There was a problem hiding this comment.
When stranded recovery runs against a SHA-256 repository and needs compensation after deleting the ref—for example, because a competing holder or run reference appears during the post-delete checks—this hard-coded 40-character zero is supplied as <old-val> to git update-ref. git update-ref -h identifies that position as <old-val>, and Git 2.43 rejects this value in a SHA-256 repository as not a valid old SHA1, so restoreMissingStrandedBranch aborts before restoring the original worktree and leaves cleanup incomplete. Derive the zero OID length from current.branchHead, whose surrounding parser already accepts 64-hex OIDs.
AGENTS.md reference: AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
Reason: publish remove-boundary hidden-content revalidation
Reason: publish SHA-256 stranded-branch compensation correction
Reason: publish atomic stranded-worktree quarantine correction
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/relay-dispatch/scripts/dispatch-vnext.test.js (3)
972-981: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
count === 3조건의 근거를 주석으로 남기세요.이 숫자는
recover.js의ls-files -v -z호출 순서에 의존합니다. 복구 단계가 하나만 늘거나 줄어도 테스트는 조용히 다른 시나리오를 검증하게 됩니다. 세 번째 호출이 어떤 단계(quarantine 이후 최종 hidden-content 증명)인지 주석으로 적으세요.As per coding guidelines: "Use meaningful comments to explain complex logic".
🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` around lines 972 - 981, 불투명한 count === 3 조건 바로 앞에 주석을 추가해 해당 세 번째 ls-files -v -z 호출이 quarantine 이후 최종 hidden-content 증명 단계임을 설명하세요. 이 호출 순서 의존성을 명시하고 기존 테스트 동작은 변경하지 마세요.Source: Coding guidelines
1006-1010: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value정규식에 넣기 전에 worktree 경로를 이스케이프하세요.
worktree는 임시 디렉터리 경로입니다. 경로에.이나+같은 정규식 메타문자가 있으면 매칭이 느슨해집니다. 그러면 잘못된 경로도 통과할 수 있습니다. 경로를 이스케이프하면 단정이 정확해집니다.♻️ 제안 변경
+ const escapedWorktree = worktree.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); assert.match( git(value.repo, ["worktree", "list", "--porcelain"]), - new RegExp(`worktree ${worktree}\\.relay-recovery-[0-9a-f]+`), + new RegExp(`worktree ${escapedWorktree}\\.relay-recovery-[0-9a-f]+`), "the clean registered worktree must be preserved at its quarantine path", );🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` around lines 1006 - 1010, 이스케이프되지 않은 worktree 경로를 RegExp 템플릿에 직접 삽입하는 assert.match 검사를 수정하세요. 해당 테스트의 worktree 경로를 정규식 메타문자가 리터럴로 처리되도록 이스케이프한 뒤, 기존의 .relay-recovery 접미사와 16진수 패턴 검증은 유지하세요.Source: Linters/SAST tools
798-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wingit 스텁의 실패 주입 순서를 주석으로 설명하세요.
스텁은
update-ref --stdin호출 이후에 마커 파일을 만들고, 다음worktree list호출에서 한 번만 실패합니다. 이 순서가 "post-delete observation failure" 시나리오를 재현하는 핵심입니다. 코드만으로는 의도를 알기 어렵습니다. 짧은 주석을 추가하세요.추가로 스텁은 실제 git 실패 시 원래 종료 코드를 전달하지 않습니다.
execFileSync가 예외를 던지면 스텁은 코드 1로 끝납니다. 복구 로직이 종료 코드로 분기하면 진단이 흐려집니다.As per coding guidelines: "Use meaningful comments to explain complex logic".
♻️ 제안 변경
const gitStub = path.join(value.root, "sha256-compensation-git.js"); + // 실패 주입 순서: recover.js가 `update-ref --stdin`으로 retaining ref를 기록하면 + // 마커 파일이 생성되고, 바로 다음 `worktree list` 호출이 한 번만 실패한다. + // 이것이 삭제 이후 관찰 실패(post-delete observation failure)를 재현한다. fs.writeFileSync(gitStub, [ `#!${process.execPath}`, 'const { execFileSync } = require("node:child_process");', 'const fs = require("node:fs");', 'const args = process.argv.slice(2);', 'const at = args[0] === "-C" ? 2 : 0;', 'if (args[at] === "worktree" && args[at + 1] === "list" && fs.existsSync(process.env.RELAY_TEST_FAIL_MARKER)) {', ' fs.unlinkSync(process.env.RELAY_TEST_FAIL_MARKER);', ' process.stderr.write("injected post-delete worktree-list failure\\n");', ' process.exit(75);', '}', 'const input = args[at] === "update-ref" && args[at + 1] === "--stdin" ? fs.readFileSync(0) : null;', - 'execFileSync(REAL_GIT, args, input ? { input, stdio: ["pipe", "inherit", "inherit"] } : { stdio: "inherit" });', + 'try {', + ' execFileSync(REAL_GIT, args, input ? { input, stdio: ["pipe", "inherit", "inherit"] } : { stdio: "inherit" });', + '} catch (error) {', + ' process.exit(typeof error.status === "number" ? error.status : 1);', + '}', 'if (input) fs.writeFileSync(process.env.RELAY_TEST_FAIL_MARKER, "fail once\\n");', ].join("\n").split("REAL_GIT").join(JSON.stringify(realGit())), { mode: 0o755 });🤖 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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js` around lines 798 - 813, Update the git stub around the update-ref and worktree-list handling to add a brief comment explaining that the marker is created after update-ref and causes the next worktree list to fail once, simulating a post-delete observation failure. Also catch errors from execFileSync and propagate the underlying git process exit status instead of allowing the stub to exit with a generic code 1.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 `@tests/relay-dispatch/scripts/dispatch-vnext.test.js`:
- Around line 963-983: Update the git stub’s execFileSync invocation in the
late-ignored-git.js setup to pass through the parent process stdin instead of
ignoring it, while preserving the existing stdout and stderr handling. Ensure
recover.js can provide update-ref --stdin input so the retaining-ref assertion
validates the real protection logic.
---
Nitpick comments:
In `@tests/relay-dispatch/scripts/dispatch-vnext.test.js`:
- Around line 972-981: 불투명한 count === 3 조건 바로 앞에 주석을 추가해 해당 세 번째 ls-files -v -z
호출이 quarantine 이후 최종 hidden-content 증명 단계임을 설명하세요. 이 호출 순서 의존성을 명시하고 기존 테스트 동작은
변경하지 마세요.
- Around line 1006-1010: 이스케이프되지 않은 worktree 경로를 RegExp 템플릿에 직접 삽입하는
assert.match 검사를 수정하세요. 해당 테스트의 worktree 경로를 정규식 메타문자가 리터럴로 처리되도록 이스케이프한 뒤, 기존의
.relay-recovery 접미사와 16진수 패턴 검증은 유지하세요.
- Around line 798-813: Update the git stub around the update-ref and
worktree-list handling to add a brief comment explaining that the marker is
created after update-ref and causes the next worktree list to fail once,
simulating a post-delete observation failure. Also catch errors from
execFileSync and propagate the underlying git process exit status instead of
allowing the stub to exit with a generic code 1.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e14ec20-4b93-471d-982c-c5024dd1ee4d
⛔ Files ignored due to path filters (2)
tests/ledger/vnext-baseline.generated.jsonis excluded by!**/*.generated.*tests/ledger/vnext-test-sites.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (2)
skills/relay-dispatch/scripts/recover.jstests/relay-dispatch/scripts/dispatch-vnext.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/relay-dispatch/scripts/recover.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f14cfc8e0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
| try { | ||
| execGit(current.checkout, ["worktree", "remove", quarantine]); |
There was a problem hiding this comment.
If relay-recover is killed or the host crashes after this git worktree remove succeeds but before the ref transaction at lines 734–741, the branch survives with zero registered holders; the next invocation then fails in observeStrandedWorktree with STRANDED_WORKTREE_NOT_FOUND, so the branch continues blocking dispatch and recovery cannot converge. The fresh evidence after the earlier retryability comment is that the final implementation compensates caught deletion failures but still leaves successful worktree removal and ref deletion as separate, unjournaled operations; persist resumable authority before removal or explicitly recover this branch-only intermediate state.
AGENTS.md reference: AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
Reason: publish open-handle-safe worktree evidence preservation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f741c19e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const evidence = `${quarantine}.preserved`; | ||
| try { | ||
| if (fs.existsSync(evidence)) throw new Error(`preservation path already exists: ${evidence}`); | ||
| fs.renameSync(quarantine, evidence); |
There was a problem hiding this comment.
If the process or host stops after this rename but before git worktree remove unregisters the quarantine, Git still records the now-missing quarantine path while the actual tree lives at .preserved; the next recovery fails while resolving the registered path and cannot clear the branch blocking dispatch. Fresh evidence beyond the earlier crash-resumability comment is that the new preservation step introduces this earlier registered-but-missing state, so it needs a durable phase marker or explicit resume handling.
AGENTS.md reference: AGENTS.md:L16-L20
Useful? React with 👍 / 👎.
Reason: publish review 13 cleanup evidence fix after full gate
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1879f69e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { fs.mkdirSync(evidenceBase, { mode: 0o700 }); } | ||
| catch (error) { if (error.code !== "EEXIST") throw error; } | ||
| const stat = fs.lstatSync(evidenceBase); | ||
| if (!stat.isDirectory() || stat.isSymbolicLink() || fs.realpathSync(evidenceBase) !== path.resolve(evidenceBase)) { | ||
| recoveryFail("UNTRUSTED_WORKTREE", `Relay recovery evidence base is not a trusted directory: ${evidenceBase}`); |
There was a problem hiding this comment.
When RELAY_WORKTREE_BASE has a shared writable parent, another user can pre-create <base>.recovery-evidence as a world-writable directory; this validation accepts it because it checks only type and canonical path. Recovery then renames the entire preserved checkout into that attacker-controlled directory, exposing normally readable tracked files. Require the evidence directory to be freshly created with trusted ownership/permissions, or validate and bind those properties before moving the worktree into it.
Useful? React with 👍 / 👎.
Reason: publish review 14 unregister race compensation after full gate
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 703330121f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const evidence = path.join(evidenceBase, `stranded-${crypto.randomBytes(12).toString("hex")}.preserved`); | ||
| try { | ||
| if (fs.existsSync(evidence)) throw new Error(`preservation path already exists: ${evidence}`); | ||
| fs.renameSync(quarantine, evidence); |
There was a problem hiding this comment.
When RELAY_WORKTREE_BASE is itself a mounted volume, the quarantined worktree is on that volume but ${current.base}.recovery-evidence is a sibling on the parent filesystem, so this rename fails with EXDEV. I reproduced this with the worktree base at /dev/shm: recovery restored the original checkout but left the stranded branch in place, meaning every recovery attempt fails and the branch continues blocking dispatch. Select or validate an evidence location with the same device as the quarantined worktree before relying on an atomic rename.
Useful? React with 👍 / 👎.
Reason: publish review 15 dirty submodule safety proof after full gate
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6d690fea9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const quarantine = `${current.worktree}.relay-recovery-${crypto.randomBytes(12).toString("hex")}`; | ||
| if (fs.existsSync(quarantine)) recoveryFail("STRANDED_WORKTREE_CHANGED", `quarantine path already exists: ${quarantine}`); | ||
| try { | ||
| execGit(current.checkout, ["worktree", "move", current.worktree, quarantine]); |
There was a problem hiding this comment.
When the otherwise-clean stranded worktree has an initialized submodule, the safety proof passes but this command cannot quarantine it. Checked with Git 2.43.0: git worktree move exits 128 with “working trees containing submodules cannot be moved or removed,” even with --force, so repositories using submodules can never use the documented recovery path and the branch continues blocking dispatch.
Useful? React with 👍 / 👎.
| repo: current.repoRoot, | ||
| branch: current.branch, | ||
| worktree: current.worktree, | ||
| recovery_evidence: recoveryEvidence, |
There was a problem hiding this comment.
On every successful recovery, preserveAndUnregisterQuarantinedWorktree has moved the entire checkout into a new .recovery-evidence directory, and this terminal return leaves it there permanently; the repo contains no deletion or cleanup path for these directories. Repeated stranded branches therefore accumulate full tracked checkouts outside the worktree inventory, consuming potentially substantial disk while reintroducing the evidence sidecars prohibited by the runtime model.
AGENTS.md reference: AGENTS.md:L3-L7
Useful? React with 👍 / 👎.
1 participant
Recovery Summary
Summary by CodeRabbit
새로운 기능
버그 수정
테스트