Skip to content

fix: preserve launcher errors when package probes fail - #136

Merged
unbraind merged 2 commits into
mainfrom
fix/launcher-probe-errors
Oct 4, 2026
Merged

unbraind merged 2 commits into
mainfrom
fix/launcher-probe-errors

Conversation

@unbraind

@unbraind unbraind commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Fixes #135.

Canonical launcher catches inconclusive filesystem probe errors and preserves the original resolution diagnostic. Existing global lookup behavior is deliberately unchanged and documented.

PM item: ops-85at (available on main after merge).

Validation: 19 launcher cases pass on Node22.18/24.19/26.10; eight new regression cases fail before the fix. Full coverage on Node22.18 and26.10:455 passed,0failed,2existing skips,100% statements/branches/functions/lines across23sources. Typechecks,lint,duplication,docstrings,package,changelog,release-policy and PM health pass. Coverage used cached optional metadata and a temporary outside-repo launcher disabling implicit npm audits; no repository gate changes.

Pending: explicit production audit awaits user approval to transmit dependency metadata to npm; hosted CI/reviews still required. No release or downstream template propagation yet. Independent gate PRs #132/#134 are untouched.

Summary by Sourcery

Preserve launcher resolution errors when package presence checks cannot conclusively determine whether pm-ops is installed.

Bug Fixes:

  • Preserve the original launcher resolution diagnostic when package presence probes encounter inconclusive filesystem errors instead of incorrectly treating the package as absent.
  • Retain global package lookup behavior, including NODE_PATH resolution, while distinguishing absent packages from broken or uncertain installations.

Documentation:

  • Document launcher behavior for unreadable or malformed lookup paths and global package lookup.

Tests:

  • Add regression coverage for malformed, inaccessible, and global package lookup paths, including absent, broken, and working global installations.

Summary by cubic

Fixes #135 by preserving the original installer resolution diagnostic when the launcher's package presence probe cannot conclusively determine whether pm-ops is installed.

Previously, an unreadable or malformed lookup path (e.g. ENOTDIR, ELOOP, EACCES, EPERM) raised a secondary filesystem error that replaced the original MODULE_NOT_FOUND message and could incorrectly trigger the omit-dev skip path. The probe now fails closed on inconclusive filesystem errors; Node's global lookup behavior, including NODE_PATH, is intentionally unchanged and documented.

Tests

  • Adds regression coverage for local, hoisted, and global ENOTDIR/ELOOP failures, controlled EACCES/EPERM probe failures, and global absent, broken, and working package behavior.
  • All 19 launcher cases pass on Node 22.18, 24.19, and 26.10; the eight new cases fail before the fix.

Written for commit e3e9618. Summary will update on new commits.

Review in cubic

Fail closed on filesystem probe errors without replacing the original installer diagnostic. Preserve and test global lookup semantics. Add real malformed-path and controlled permission regressions for issue #135.
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: be61f295-825f-4974-b22f-cc2f009df11e
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Oct 3, 2026

Copy link
Copy Markdown

Reviewer's Guide

The canonical launcher now fails closed when package-presence probes encounter inconclusive filesystem errors, preserving the original installer resolution diagnostic while retaining global Node lookup behavior. Documentation and comprehensive launcher regressions cover malformed permissions, path traversal failures, and NODE_PATH cases.

Sequence diagram for preserving launcher resolution errors

sequenceDiagram
    participant Launcher
    participant Resolver
    participant Filesystem

    Launcher->>Resolver: resolve(pm-ops/package.json)
    Resolver-->>Launcher: resolution error
    Launcher->>Resolver: resolve.paths(pm-ops/package.json)
    Resolver-->>Launcher: Node lookup paths including NODE_PATH
    loop each lookup path
        Launcher->>Filesystem: lstatSync(path to pm-ops, throwIfNoEntry false)
        alt package path exists
            Filesystem-->>Launcher: filesystem entry
        else path is absent
            Filesystem-->>Launcher: undefined
        else probe is inconclusive
            Filesystem-->>Launcher: filesystem error
            Launcher-->>Launcher: retain packagePresent
        end
    end
    alt package is present or probe is inconclusive
        Launcher-->>Launcher: rethrow original resolution error
    else package is absent
        Launcher-->>Launcher: skip optional package
    end
Loading

File-Level Changes

Change Details Files
Preserve the installer’s original module-resolution diagnostic when package-presence probing is inconclusive.
  • Treat filesystem probe errors other than absence as evidence of uncertain presence and fail closed.
  • Retain Node global lookup paths, including NODE_PATH, so local absence does not override installer resolution behavior.
  • Document the revised launcher behavior and its distinction from unchanged global lookup semantics.
templates/prepare-merge-driver.ts
README.md
Add regression coverage for malformed, inaccessible, and global package lookup paths.
  • Cover ENOTDIR, ELOOP, EACCES, and EPERM probe failures across local, hoisted, and global locations.
  • Verify the original MODULE_NOT_FOUND resolution error is preserved and skip notices are not emitted.
  • Verify absent, broken, and working packages discovered through NODE_PATH retain their expected behavior.
  • Extend the test runner helper to pass environment variables and Node preload arguments.
test/merge-driver-launcher.test.ts
Record the associated PM work item and history metadata.
  • Add the ops-85at issue and JSONL history entries.
.agents/pm/issues/ops-85at.toon
.agents/pm/history/ops-85at.jsonl

Assessment against linked issues

Issue Objective Addressed Explanation
#135 Make the package-presence probe robust to filesystem errors such as EACCES, EPERM, ENOTDIR, and ELOOP, treating an inconclusive probe as evidence of presence so the original resolution error is preserved. ✅
#135 Explicitly define and document whether Node global module lookup paths participate in the presence probe, while keeping the probe behavior consistent with installer resolution. ✅
#135 Propagate the corrected canonical launcher template through publication and downstream consumer copies that require byte-identical templates. ❌ The canonical template and README are updated, but the PR explicitly states that no release or downstream template propagation has occurred yet.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@unbraind
unbraind marked this pull request as ready for review October 3, 2026 13:23

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @unbraind, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 2 days and 18 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Fixes error handling in the package launcher probe logic.

The PR appears safe to merge based on the reviewed changes.

Summary

This PR makes the merge-driver launcher preserve its original resolution error when a filesystem presence probe is inconclusive, while retaining the absent-package skip and Node global lookup behavior.

  • Adds local, hoisted, global, and controlled permission-error regression cases.
  • Documents the launcher’s behavior for unreadable or malformed lookup paths.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Resolve preparation entry] -->|Success| B[Run installer]
  A -->|Failure| C[Probe package presence]
  C -->|Confirmed absent| D[Print notice and skip]
  C -->|Present or uncertain| E[Rethrow original resolution error]
Loading

Reviews (2) · Last reviewed commit: "Merge branch 'main' into fix/launcher-pr..."

@unbraind

unbraind commented Oct 4, 2026

Copy link
Copy Markdown
Owner Author

@sourcery-ai the review-budget notice is acknowledged (down-voted only because it carries no finding). @greptile-apps / @coderabbitai thanks — the 5/5 confidence summary and walkthrough match the launcher change; branch updated onto main after #132/#134 merged, waiting for checks.

@greptile-apps

greptile-apps Bot commented Oct 4, 2026

Copy link
Copy Markdown

Thanks for the update. The refreshed branch and the #132/#134 merge context are noted. No additional findings from this review; the remaining prerequisites are hosted checks and approval for the production audit.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@unbraind Thanks for the update. With the branch updated onto main, the remaining step is to wait for checks on the updated branch.

You are interacting with an AI system.

@unbraind
unbraind merged commit b97fd99 into main Oct 4, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant