Repository navigation
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
[Low impact] The PR appears safe to merge; the new test covers a distinct cancellation case without changing production behavior. SummaryAdds one regression test for cancellation during an awaited lookup after a previously observed file’s parent becomes a regular file.
No actionable correctness, security, or repository-rule issues were found. This review checked source code; it did not run tests. Reviews (3) · Last reviewed commit: "test(process): preserve exact caller can..." · Reviewed by Greptile |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Compared this PR with main The obstructed-root lookup defect was fixed in e67c364, but the cancellation-during-lookup regression added here is still distinct from main's existing coverage. Please retain the cancellation, bounded completion and cleanup/ownership oracle when rebasing. This is useful fault-boundary coverage and should not be discarded simply because #3238 is closed; the PR can be narrowed to the missing regression. PR head reviewed: |
|
Confirmed against main The retained regression observes the selected file, replaces its parent with a regular file, starts the awaited final lookup, then aborts. It requires No production rewrite or additional harness was added. Current-head CI |
|
TL;DR: I ran the one test file this PR changes, on Node 24.18.0. The new test passes on current main. It also catches the bug: when I removed one line of main's fix by hand, the new test failed. Thanks, xu91102, for the work. I tested head 54b47f3 on top of main 89faa86 (#3050). Results
Not run
Commandsgit -C ~/code/rea-mirror/rea.git fetch origin
git -C ~/code/rea-mirror/rea.git archive 89faa863ed472d8a13a826014331d3b81cfc5de3 | tar -x -C /tmp/t8-1015/main
npm ci # Node 24.18.0, exit 0
git -C ~/code/rea-mirror/rea.git diff 89faa863e...54b47f3e7 > 3263.diff
git apply 3263.diff
npm run test:focused -- src/process/capture/FilesystemSnapshot.test.ts # exit 0, 7 passed 4 skipped
sed -i '67d' src/process/capture/FilesystemSnapshot.ts # drops signal?.throwIfAborted() in the ENOENT/ENOTDIR branch
npm run test:focused -- src/process/capture/FilesystemSnapshot.test.ts # exit 1, 1 failed 6 passed 4 skipped
cp FilesystemSnapshot.ts.orig src/process/capture/FilesystemSnapshot.ts # restored, cmp cleanI ran Test setup🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
Merge current main833 while preserving both the observed-root cancellation regression and main typed read/hash cancellation coverage. Match the shared ProcessCaptureSnapshotFailure owner and retain exact explicit caller reason identity. Refs morluto#3238
Reverse the unpublished merge relative to its included-main parent to restore exact upstream inputs, then reapply only the reviewed cancellation test in a normal commit. Preserve all history; no hook is disabled or bypass flag used. Refs morluto#3238
Reapply only the independently reviewed obstructed-root cancellation regression after restoring upstream bytes changed by the normal merge hook. Keep main typed failures, caller reason identity, partial snapshot and cleanup facts. Refs morluto#3238
Summary
Fixes #3238.
Main already adopted the filesystem-obstruction fix and public lifecycle regression in
e67c3642c8920a2a9c0e231ee2f16f50b245df31, retaining contributor coauthor credit. This PR keeps the distinct cancellation regression: after observing a selected file, its parent becomes a regular file and cancellation arrives during the awaited final lookup.The conflict with current main is confined to
FilesystemSnapshot.test.ts. The normal merge includes833136507dd79d959340b09862a53af344e4f1b3, preserves main's typed read/hash cancellation test and resource owner, and adapts this regression to its existingProcessCaptureSnapshotFailure. It checks the exact explicit caller reason object, cancelled category, partial/truncated snapshot and empty cleanup leases/read failures. Production code, schemas, docs and dependencies match the included main.Validation
Final head:
d1cd67dd128328109df32b933d4d69a17651ba8b; integration merge435620217 has parents previous PR head54b and captured main833; final head retains the complete normal merge/revert/reapply history. No historical CI result is transferred to the new head.npm run check:6 successful tasks/2 cached,0 errors/1795 warnings; module and architecture guards passed. Docs6 tasks/3 cached plus README5; test discovery1704 exactly once.npm pack --dry-run --jsonpassed. These package/runtime inputs are unchanged by the final test-only reason adjustment.The normal merge hook auto-fixed two unrelated upstream files. A normal revert restored upstream bytes and a normal single-file commit reapplied the reviewed test; no history rewrite or hook disable was used. The final diff against included main is one test only. A full check on restored inputs succeeded with all six cached tasks and the architecture guard; prior executed V2 checks remain separately recorded.
GitHub Portable20600PASS/170skip aggregates source and five shards; it is not another execution. The actual Windows capability/package lane passed518 tests/31skip, plus47 filesystem and58 native-boundary tests; the macOS Apple artifact lane passed263/4skip. Linux arm64/x64 and macOS arm64 package baselines also succeeded within their logged scope. The Linux build of bundled Windows controls is a cross-build, separate from actual Windows execution. Other17 conditional jobs remain unexecuted; no broader provider, hardware or performance claim.
The initial raw AbortError assertion failed against current main's typed failure and remains recorded. The first independent review also required an explicit caller abort reason; the final regression supplies and checks that same object. Old inaccessible Codex failure causes remain unknown; no unavailable finding is claimed resolved.
Author readiness
Human owner: @xu91102
The declarations remain for the human owner. This update does not attest to human review or change its external nonDraft state.
Before merge
type(scope): outcome.