Follow-up from independent review of #50 (v0.9.0, resume=true). The content-substitution path is fully idempotent on resume (a file already correct is a no-op). The rename path (substituteEntry's renamed branch, internal/tools/create_project_from_template.go) is not quite as robust.
Gap
A rename does two separate, non-atomic API calls: write the new path, then delete the old path. If a prior partial run's write succeeded but the paired delete failed (plausible under the same infra#179 flakiness this whole feature exists to work around), a resume will:
See the OLD path still in the tree (delete never happened) and the SAME rename target already correct.
Recompute the identical rename and attempt to upsertRetry (POST-create, no sha) at the new path — which already exists from the prior run → conflict.
partial_failure again, needing yet another resume cycle to notice and (manually) clean up the stray old file.
This is not silent data loss — it fails loud and is themable to another resume attempt — but it can get stuck in a loop needing manual intervention (delete the stray old-path file) rather than genuinely completing on its own.
Fix sketch
In the rename branch, before writing the new path: check if the new path already exists with the correct content (mirror injectDispatchAllow's read-before-write idempotency). If so, skip the write and just retry the delete of the old path (which is the only thing that didn't complete). If the delete's target (old path) is already gone too, the whole rename is done — no-op.
Acceptance
A resume where a prior write-succeeded/delete-failed rename left a stray old-path file completes cleanly (deletes the stray file, doesn't re-attempt the already-done write).
Test coverage for this specific partial-rename-then-resume scenario (currently zero — the existing resume tests only exercise content substitution, not renames).
Refs
Surfaced by independent adversarial review during #50 (v0.9.0). Not a blocker — narrow edge case, fails loud rather than corrupting anything.
## Context
Follow-up from independent review of #50 (v0.9.0, `resume=true`). The content-substitution path is fully idempotent on resume (a file already correct is a no-op). The **rename** path (`substituteEntry`'s `renamed` branch, `internal/tools/create_project_from_template.go`) is not quite as robust.
## Gap
A rename does two separate, non-atomic API calls: write the new path, then delete the old path. If a prior partial run's write succeeded but the paired delete failed (plausible under the same infra#179 flakiness this whole feature exists to work around), a resume will:
1. See the OLD path still in the tree (delete never happened) and the SAME rename target already correct.
2. Recompute the identical rename and attempt to `upsertRetry` (POST-create, no sha) at the new path — which already exists from the prior run → conflict.
3. `partial_failure` again, needing yet another resume cycle to notice and (manually) clean up the stray old file.
This is not silent data loss — it fails loud and is themable to another resume attempt — but it can get stuck in a loop needing manual intervention (delete the stray old-path file) rather than genuinely completing on its own.
## Fix sketch
In the rename branch, before writing the new path: check if the new path already exists with the correct content (mirror `injectDispatchAllow`'s read-before-write idempotency). If so, skip the write and just retry the delete of the old path (which is the only thing that didn't complete). If the delete's target (old path) is already gone too, the whole rename is done — no-op.
## Acceptance
- [ ] A resume where a prior write-succeeded/delete-failed rename left a stray old-path file completes cleanly (deletes the stray file, doesn't re-attempt the already-done write).
- [ ] Test coverage for this specific partial-rename-then-resume scenario (currently zero — the existing resume tests only exercise content substitution, not renames).
## Refs
Surfaced by independent adversarial review during #50 (v0.9.0). Not a blocker — narrow edge case, fails loud rather than corrupting anything.
renameEntry now reads the new path first before writing: if it already holds the correct content (the write half of a prior partial rename succeeded), the write is skipped and only the outstanding delete of the old path runs; if the new path exists but differs, it's updated with the fetched sha instead of a blind create; if the old path is already gone by delete time, that's success, not a failure. Mirrors injectDispatchAllow's (#51) read-before-write idempotency pattern exactly.
Test TestCreateProject_Resume_StrayRenamedOldPath_CompletesCleanly: a stray old path + an already-correct new path resolves to a single delete, zero redundant writes, no partial_failure. All prior create_project tests pass unchanged (backward compatible).
Fixed in v0.9.2 (`51b823a`).
`renameEntry` now reads the new path first before writing: if it already holds the correct content (the write half of a prior partial rename succeeded), the write is skipped and only the outstanding delete of the old path runs; if the new path exists but differs, it's updated with the fetched sha instead of a blind create; if the old path is already gone by delete time, that's success, not a failure. Mirrors `injectDispatchAllow`'s (#51) read-before-write idempotency pattern exactly.
Test `TestCreateProject_Resume_StrayRenamedOldPath_CompletesCleanly`: a stray old path + an already-correct new path resolves to a single delete, zero redundant writes, no `partial_failure`. All prior `create_project` tests pass unchanged (backward compatible).
`task check` green (exit 0, 0 FAIL — verified). Closing #53.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Context
Follow-up from independent review of #50 (v0.9.0,
resume=true). The content-substitution path is fully idempotent on resume (a file already correct is a no-op). The rename path (substituteEntry'srenamedbranch,internal/tools/create_project_from_template.go) is not quite as robust.Gap
A rename does two separate, non-atomic API calls: write the new path, then delete the old path. If a prior partial run's write succeeded but the paired delete failed (plausible under the same infra#179 flakiness this whole feature exists to work around), a resume will:
upsertRetry(POST-create, no sha) at the new path — which already exists from the prior run → conflict.partial_failureagain, needing yet another resume cycle to notice and (manually) clean up the stray old file.This is not silent data loss — it fails loud and is themable to another resume attempt — but it can get stuck in a loop needing manual intervention (delete the stray old-path file) rather than genuinely completing on its own.
Fix sketch
In the rename branch, before writing the new path: check if the new path already exists with the correct content (mirror
injectDispatchAllow's read-before-write idempotency). If so, skip the write and just retry the delete of the old path (which is the only thing that didn't complete). If the delete's target (old path) is already gone too, the whole rename is done — no-op.Acceptance
Refs
Surfaced by independent adversarial review during #50 (v0.9.0). Not a blocker — narrow edge case, fails loud rather than corrupting anything.
Fixed in v0.9.2 (
51b823a).renameEntrynow reads the new path first before writing: if it already holds the correct content (the write half of a prior partial rename succeeded), the write is skipped and only the outstanding delete of the old path runs; if the new path exists but differs, it's updated with the fetched sha instead of a blind create; if the old path is already gone by delete time, that's success, not a failure. MirrorsinjectDispatchAllow's (#51) read-before-write idempotency pattern exactly.Test
TestCreateProject_Resume_StrayRenamedOldPath_CompletesCleanly: a stray old path + an already-correct new path resolves to a single delete, zero redundant writes, nopartial_failure. All priorcreate_projecttests pass unchanged (backward compatible).task checkgreen (exit 0, 0 FAIL — verified). Closing #53.