Implement uv_thread_self on POSIX #29228
+131
−18
Closed
Claude / Claude Code Review
completed
Apr 12, 2026 in 17m 17s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/napi/uv-stub-stuff/uv_impl.c:109-120 |
test_thread_self only checks self-consistency, not correctness vs pthread_self() |
| 🟡 Nit | test/regression/issue/29223.test.ts:72 |
beforeAll timeout 60_000 still violates no-timeout convention |
Annotations
Check warning on line 120 in test/napi/uv-stub-stuff/uv_impl.c
claude / Claude Code Review
test_thread_self only checks self-consistency, not correctness vs pthread_self()
The `test_thread_self` function in `test/napi/uv-stub-stuff/uv_impl.c` (line 118) checks `pthread_equal(a, b) \!= 0` — verifying only that two consecutive `uv_thread_self()` calls agree with each other — but never checks `pthread_equal(a, pthread_self())`. A bogus implementation returning any constant `pthread_t` value would pass. Fix: add `&& pthread_equal(a, pthread_self()) \!= 0` to the condition on line 118 to match the fuller check already present in the companion addon in `29223.test.ts`.
Check warning on line 72 in test/regression/issue/29223.test.ts
claude / Claude Code Review
beforeAll timeout 60_000 still violates no-timeout convention
The `beforeAll` in `test/regression/issue/29223.test.ts` still closes with `}, 60_000);` (line 72), violating CLAUDE.md's rule: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The previous review comment flagged the original 120,000 ms; commit c5db337 reduced it to 60,000 ms instead of removing it. The companion file `test/napi/uv.test.ts` (added in the same PR, running the identical node-gyp build in its own `beforeAll`) correctly omits the timeout argument, making the two
Loading