Skip to content

fix(tests): use native smoke result descriptor - #155

Merged
steipete merged 1 commit into
mainfrom
steipete/native-smoke-descriptor-handling-c68c4f
Aug 28, 2026
Merged

fix(tests): use native smoke result descriptor#155
steipete merged 1 commit into
mainfrom
steipete/native-smoke-descriptor-handling-c68c4f

Conversation

@steipete

Copy link
Copy Markdown
Contributor

What Problem This Solves

Resolves a problem where running the standalone native filesystem diagnostic failed during descriptor identity checking and cleanup. openBeneath() returns { fd, containment }, but this script still treated the entire result as a numeric descriptor. Its closeSync() error also masked the original identity-check failure.

This is a stale standalone script contract, not a broken CI path: current CI uses scripts/native-mode-smoke.mjs.

Why This Change Was Made

Destructure the numeric fd at the call site and retain the existing try/finally ownership. Keep the tiny diagnostic available without changing the binding, containment semantics, Windows lossless identities, or retained-directory staging. The return-contract migration in #65 missed this caller; current documentation already describes the object result.

User Impact

Tooling only. The standalone diagnostic can complete its mkdir, open, identity, hardlink, rename, and collision checks again. No public API, package behavior, security policy, or compatibility change.

Evidence

Actual darwin-arm64 addon, built with pnpm build and pnpm native:build:

  • Before: node scripts/native-smoke.mjs exited 1 with ERR_INVALID_ARG_TYPE: the fd argument received an Object.

  • After: the same command exited 0 with native smoke: PASS, including the later hardlink and rename/collision checks.

  • Existing native descriptor/containment integration regression passed against the built addon.

  • pnpm check: lint, build, and documentation checks passed; tests hit 68 timeouts plus cleanup errors on a heavily loaded host.

  • Full suite retried with pnpm test --maxWorkers=2: 1,282 passed, 112 skipped, three archive-fuzz tests timed out. Test timeouts and repository configuration are unchanged. PR CI remains the full-suite landing gate.

  • git diff --check passed. Independent Codex autoreview reported no actionable findings.

  • Existing regression test and actual native before/after smoke exercised

  • Security and compatibility impact considered

  • CHANGELOG.md updated for the tooling correction

  • No credentials, private paths, private hosts, or sensitive contents included

@steipete
steipete requested a review from a team as a code owner August 28, 2026 15:28
@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 28, 2026
@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 28, 2026, 11:31 AM ET / 15:31 UTC.

ClawSweeper review

What this changes

The PR updates the standalone native diagnostic to extract the numeric file descriptor returned by openBeneath() and records that tooling fix in the changelog.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open: the one-line correction matches the current native binding contract, has recorded real smoke evidence, and remains necessary because fetched main still passes the result object where a numeric descriptor is required.

Priority: P3
Reviewed head: 26c001a62088abe727d755fed76a7adab96a3955

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused contract correction with source-confirmed behavior and recorded native smoke evidence.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.
Evidence reviewed 5 items Current-main defect: The fetched main revision assigns the object returned by openBeneath() to fd, then passes it to descriptor-only identity and close operations.
Binding contract: The native binding declares an object result with numeric fd and containment metadata; current production callers consume the fd member.
Introduced patch: The verified merge-base-to-head diff contains only the matching descriptor extraction and its Unreleased changelog entry.
Findings None None.
Security None None.

How this fits together

The native smoke script opens a test file through the package’s confinement-aware native binding, then uses the returned descriptor for identity checks and cleanup. It verifies native filesystem-boundary behavior separately from the CI mode-smoke path.

flowchart LR
  A[Temporary fixture] --> B[Native smoke script]
  B --> C[Confinement-aware file open]
  C --> D[Open result]
  D --> E[Descriptor identity checks]
  E --> F[Cleanup and PASS result]
Loading

Before merge

  • Complete next step (P2) - No repair lane is needed because the submitted patch has no actionable review finding.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 files affected; 1 script line changed, 1 changelog line added The correction is confined to a standalone diagnostic and its required release-note entry.

Technical review

Best possible solution:

Merge the narrow descriptor-contract correction after the remaining required checks complete.

Do we have a high-confidence way to reproduce the issue?

Yes: current-main source passes an object into descriptor-only operations, and the PR body records a built-addon before/after smoke run that changes from an argument-type failure to native smoke: PASS.

Is this the best way to solve the issue?

Yes: destructuring fd at this stale caller preserves the established { fd, containment } contract and existing cleanup ownership without changing confinement semantics.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a5354d30cde9.

Labels

Label changes:

  • add P3: This is a low-risk standalone tooling correction with no public package behavior change.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.

Label justifications:

  • P3: This is a low-risk standalone tooling correction with no public package behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body records an actual built darwin-arm64 addon before/after run: the diagnostic changes from an object-descriptor error to native smoke: PASS, including later filesystem checks.

Evidence

What I checked:

  • Current-main defect: The fetched main revision assigns the object returned by openBeneath() to fd, then passes it to descriptor-only identity and close operations. (scripts/native-smoke.mjs:21, a5354d30cde9)
  • Binding contract: The native binding declares an object result with numeric fd and containment metadata; current production callers consume the fd member. (src/native-binding.ts:34, 26c001a62088)
  • Introduced patch: The verified merge-base-to-head diff contains only the matching descriptor extraction and its Unreleased changelog entry. (scripts/native-smoke.mjs:21, 26c001a62088)
  • Feature history: History shows the standalone diagnostic was introduced with the bundled-native feature and is now maintained by the same contributor. (scripts/native-smoke.mjs:21, 1db7c7fa4a1c)
  • CI boundary: CI invokes native-mode-smoke.mjs; this separate script remains a useful standalone diagnostic rather than a replacement for CI coverage. (.github/workflows/ci.yml:105, 26c001a62088)

Likely related people:

  • Peter Steinberger: He authored both the bundled-native change that introduced the standalone diagnostic and this contract correction. (role: original diagnostic author and recent native-surface contributor; confidence: high; commits: 1db7c7fa4a1c, 26c001a62088; files: scripts/native-smoke.mjs, src/native-binding.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 2d8875c into main Aug 28, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant