Skip to content

fix(ai): direct require('node:fs') in getProcEnv breaks browser bundlers#102955

Closed
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix/ai-proc-env-bundlers
Closed

fix(ai): direct require('node:fs') in getProcEnv breaks browser bundlers#102955
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix/ai-proc-env-bundlers

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

In packages/ai/src/env-api-keys.ts, the file carefully avoids top-level imports of Node.js modules to ensure browser and Vite builds do not fail.
However, inside the getProcEnv helper, it performs a direct, un-guarded require("node:fs"):

function getProcEnv(key: string): string | undefined {
  // ...
  if (procEnvCache === null) {
    procEnvCache = new Map();
    try {
      const { readFileSync } = require("node:fs") as typeof import("node:fs");
      const data = readFileSync("/proc/self/environ", "utf-8");

Impact

Vite and other static bundlers scan source code for occurrences of require("node:fs"). Even if wrapped in a try...catch block and guarded by runtime checks, this direct literal string import will trigger static dependency resolution warnings or compile/bundle failures in non-Node browser environments.

Suggested Fix

Use the existing loadNodeBuiltinModule wrapper function which handles dynamic require safely without exposing raw string literals to static bundlers:

      const fsModule = loadNodeBuiltinModule("node:fs") as typeof import("node:fs") | null;
      if (!fsModule) {
        return undefined;
      }
      const data = fsModule.readFileSync("/proc/self/environ", "utf-8");

@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 11:41 AM ET / 15:41 UTC.

Summary
Replaces the direct require("node:fs") in packages/ai getProcEnv with the existing loadNodeBuiltinModule(NODE_FS_SPECIFIER) helper before reading /proc/self/environ.

PR surface: Source +2. Total +2 across 1 file.

Reproducibility: Partially: current main still contains the exact static require("node:fs") literal inside getProcEnv, which matches the reported bundler trigger. I did not run a Vite/browser bundler repro, so this is source-reproducible rather than runtime-reproduced.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #102934
Summary: The canonical user problem is the open issue about the getProcEnv static require("node:fs"); this PR and the sibling PR are candidate fixes for that same line.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix real behavior proof is present; the contributor should add redacted Vite/browser bundler output, terminal output, logs, screenshot, or recording showing the changed path succeeds, then update the PR body to trigger re-review.

Risk before merge

  • [P1] The PR lacks real browser/Vite bundler proof, so the static-source fix is plausible but not yet proven in the reported consumer environment.
  • [P1] There is another open candidate PR for the same issue; maintainers should land one canonical fix rather than both branches.

Maintainer options:

  1. Decide the mitigation before merge
    Land one narrow PR that removes the static Node builtin require literal via the existing helper after real bundler proof is added and the duplicate candidate branch is resolved.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No automated repair job is appropriate because the remaining gate is contributor-supplied real behavior proof plus duplicate candidate coordination, not a narrow code defect for ClawSweeper to patch.

Security
Cleared: The diff only changes how an existing Node builtin is loaded in the AI env helper and does not alter dependencies, workflows, permissions, secrets, or package resolution.

Review details

Best possible solution:

Land one narrow PR that removes the static Node builtin require literal via the existing helper after real bundler proof is added and the duplicate candidate branch is resolved.

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

Partially: current main still contains the exact static require("node:fs") literal inside getProcEnv, which matches the reported bundler trigger. I did not run a Vite/browser bundler repro, so this is source-reproducible rather than runtime-reproduced.

Is this the best way to solve the issue?

Yes, likely: using the existing loadNodeBuiltinModule(NODE_FS_SPECIFIER) helper is the narrowest local fix and preserves the module's existing Node builtin loading pattern. Merge readiness still depends on real bundler proof and duplicate-PR coordination.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal package compatibility bug with limited blast radius for browser bundler consumers of @openclaw/ai.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix real behavior proof is present; the contributor should add redacted Vite/browser bundler output, terminal output, logs, screenshot, or recording showing the changed path succeeds, then update the PR body to trigger re-review.

Label justifications:

  • P2: This is a normal package compatibility bug with limited blast radius for browser bundler consumers of @openclaw/ai.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix real behavior proof is present; the contributor should add redacted Vite/browser bundler output, terminal output, logs, screenshot, or recording showing the changed path succeeds, then update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source +2. Total +2 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 8 6 +2
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 8 6 +2

What I checked:

Likely related people:

  • steipete: Authored and merged the publishable @openclaw/ai extraction that introduced packages/ai/src/env-api-keys.ts as a package surface. (role: feature-history owner; confidence: high; commits: 062f88e3e3af; files: packages/ai/src/env-api-keys.ts, docs/reference/openclaw-ai.md, packages/ai/package.json)
  • wm0018: Opened the sibling candidate PR that is linked as closing the same issue and changes the same getProcEnv Node builtin loading path. (role: adjacent candidate-fix author; confidence: medium; commits: 310867db6a63; files: packages/ai/src/env-api-keys.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by #102947, now landed on main as 50cb0d66b8e63e5c2baea8f7ce065007d6659521.

The canonical PR applies the same owner-boundary fix and adds a browser-bundle regression. Exact-head proof included all 350 packages/ai tests, check:changed, green canonical CI 29056319772, and Vite 8.1.3 production builds showing the node:fs externalization warning drop from 1 before to 0 after.

Thank you @aniruddhaadak80 for identifying and proposing the fix for this browser-bundling gap.

@steipete steipete closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aniruddhaadak80 @steipete