Fix plugin build reproducibility and generated scaffold validation#2052
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. |
WalkthroughThe PR updates generated plugin scaffolds and module wiring, adds deterministic vendor archive metadata writing, and configures Sharp builds to ignore global libvips installations. ChangesPlugin generator updates
Vendor archive determinism
Sharp build configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BuildTxz
participant VendorArchiveWriter
participant FileSystem
BuildTxz->>VendorArchiveWriter: writeVendorArchiveInfo(configPath, configData)
VendorArchiveWriter->>FileSystem: inspect and create vendor archive metadata
VendorArchiveWriter->>FileSystem: conditionally write JSON and set epoch mtime
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)api/DockerfileTraceback (most recent call last): packages/unraid-api-plugin-generator/package.jsonTraceback (most recent call last): packages/unraid-api-plugin-generator/tsconfig.build.jsonTraceback (most recent call last):
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/unraid-api-plugin-generator/src/create-plugin.test.ts`:
- Line 5: Update the createPlugin import in create-plugin.test.ts to use the .js
extension, preserving the existing local module target and NodeNext-compatible
ESM resolution.
In `@plugin/builder/build-txz.ts`:
- Line 14: Update the local ESM imports in plugin/builder/build-txz.ts (lines
14-14) and plugin/builder/utils/vendor-archive.test.ts (lines 5-5) to include
the .js extension, targeting writeVendorArchiveInfo and the vendor-archive
import respectively; no other changes are needed.
In `@plugin/builder/utils/vendor-archive.test.ts`:
- Around line 16-24: Add a distinct second configuration in the
writeVendorArchiveInfo test with changed version and path values, then use it
for the second write and assert the existing file contents reflect the updated
configuration rather than the original metadata.
- Around line 9-10: Update the test using mkdtemp to wrap its body in a
try/finally block, and remove the created directory in the finally cleanup via
the existing filesystem cleanup utility. Ensure cleanup runs whether the test
passes or fails.
In `@plugin/builder/utils/vendor-archive.ts`:
- Around line 33-36: After writing vendor_archive.json in the archive-generation
flow, explicitly set its permissions to the fixed 0o644 mode before or alongside
the deterministic timestamp update. Update the code around writeFile and utimes
so builds produce consistent file metadata regardless of the process umask.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cb7c0c8-3f27-4a53-bb88-0490f4ab5bcc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
api/Dockerfilepackages/unraid-api-plugin-generator/package.jsonpackages/unraid-api-plugin-generator/src/create-plugin.test.tspackages/unraid-api-plugin-generator/src/create-plugin.tspackages/unraid-api-plugin-generator/src/templates/config.persistence.tspackages/unraid-api-plugin-generator/src/templates/index.tspackages/unraid-api-plugin-generator/src/templates/plugin-name.resolver.tspackages/unraid-api-plugin-generator/tsconfig.build.jsonplugin/builder/build-txz.tsplugin/builder/utils/vendor-archive.test.tsplugin/builder/utils/vendor-archive.tsplugin/package.json
💤 Files with no reviewable changes (1)
- packages/unraid-api-plugin-generator/src/templates/config.persistence.ts
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { afterEach, describe, expect, it } from "vitest"; | ||
| import { createPlugin } from "./create-plugin"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists =="
git ls-files | rg '(^|/)create-plugin(\.test)?\.ts$|package(-lock)?\.json$|tsconfig.*\.json$|vitest.config.*\.(ts|js|mjs|cjs)$' || true
echo "== imports of create-plugin in test =="
if [ -f packages/unraid-api-plugin-generator/src/create-plugin.test.ts ]; then
nl -ba packages/unraid-api-plugin-generator/src/create-plugin.test.ts | sed -n '1,40p'
fi
echo "== matching local import styles in same package tests =="
rg -n 'import\s*\{\s*createPlugin\s*\}\s*from' packages/unraid-api-plugin-generator/src -g '*.test.ts' || true
echo "== tsconfig/package config relevant fields =="
fd -a '^(tsconfig|package(-lock)?|vitest.config).*$' packages/unraid-api-plugin-generator . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
case "$f" in
*.json) python3 - <<'PY' "$f"
import json, sys
p=sys.argv[1]
try:
data=json.load(open(p))
except Exception as e:
print(e); raise SystemExit(0)
for k in ["type","scripts","moduleResolution","compilerOptions"]:
v=data
for sub in k.split("."):
if isinstance(v, dict): v=v.get(sub)
if v is not None:
print(f"{k}: {v}")
PY
;;
*.ts|*.js|*.mjs|*.cjs) sed -n '1,140p' "$f" | nl -ba ;;
*) true ;;
esac
done
echo "== repo-wide extensionless TS imports under packages/unraid-api-plugin-generator/src tests? =="
python3 - <<'PY'
import os, re
root = "packages/unraid-api-plugin-generator/src"
pattern = re.compile(r'import\s*\{([^}]*)\}\s*from\s*[\'"]((?!https?://).+)[\'"];\s*$')
for dirpath, _, files in os.walk(root):
for file in files:
if not file.endswith(".test.ts"):
continue
path = os.path.join(dirpath, file)
with open(path, "r", encoding="utf-8", errors="ignore") as f:
for lineno, line in enumerate(f, 1):
m = pattern.match(line.strip())
if not m:
continue
imp = m.group(2)
if not imp.lower().endswith((".js", ".js.map")):
print(f"{path}:{lineno}:{line.strip()}")
PYRepository: unraid/api
Length of output: 1150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test import and surrounding lines =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/unraid-api-plugin-generator/src/create-plugin.test.ts")
for i, line in enumerate(p.read_text().splitlines(), 1):
if i <= 45 or "createPlugin" in line:
print(f"{i}: {line}")
PY
echo "== local createPlugin import usages in create-plugin.test.ts =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/unraid-api-plugin-generator/src/create-plugin.test.ts")
text = p.read_text()
for line in text.splitlines():
if "createPlugin" in line:
print(line)
PY
echo "== config json relevant fields =="
for f in package.json packages/unraid-api-plugin-generator/package.json packages/unraid-api-plugin-generator/tsconfig.json packages/unraid-api-plugin-generator/tsconfig.build.json; do
[ -f "$f" ] || continue
echo "--- $f"
python3 - <<'PY' "$f"
import json, sys
p = sys.argv[1]
data = json.load(open(p))
for k in ["type", "scripts", "module", "main"]:
print(k, ":", data.get(k))
print("compilerOptions:")
co = data.get("compilerOptions", {})
for key in ["module", "moduleResolution", "target", "esModuleInterop", "allowSyntheticDefaultImports", "resolveJsonModule"]:
print(" " + key + ":", co.get(key))
PY
done
echo "== tsconfig vite/vitest related fields =="
for f in packages/unraid-api-plugin-generator/tsconfig.json packages/unraid-api-plugin-generator/tsconfig.build.json; do
[ -f "$f" ] || continue
echo "--- $f"
sed -n '1,120p' "$f"
done
echo "== root/package vitest references =="
rg -n 'vitest|typescript|moduleResolution|module:|type: "module"' -g 'package.json' -g 'tsconfig*.json' -g 'vitest.*' . || trueRepository: unraid/api
Length of output: 10016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package lock tsconfig typescript vitest version snippets =="
python3 - <<'PY'
from pathlib import Path
p = Path("package-lock.json")
if not p.exists():
print("NO package-lock.json")
raise SystemExit(0)
import json
data=json.load(open(p))
pkgs=data.get("packages", {})
for key in ("node_modules/typescript","node_modules/vitest"):
if key in pkgs:
print(key)
for k in ["version","files","dependencies","peerDependencies"]:
print(f" {k}: {pkgs[key].get(k)}")
print()
for key, pkg in pkgs.items():
if not (key.startswith("packages/unraid-api-plugin-generator") or key == "node_modules/@unraid/api-plugin-generator"):
continue
for k in ["packageJSON","version","peerDependencies","dependencies","resolved"]:
v=pkg.get(k)
if k == "packageJSON" and isinstance(v, dict):
print(f"{key}.packageJSON:")
for subk in ["type","module","scripts","dependencies","devDependencies"]:
print(f" {subk}: {v.get(subk)}")
else:
print(f"{key}.{k}: {v}")
PY
echo "== lockfile create-plugin package path =="
python3 - <<'PY'
from pathlib import Path
import json
p=Path("package-lock.json")
if not p.exists(): raise SystemExit(0)
data=json.load(open(p))
for key,pkg in data.get("packages",{}).items():
if "create-plugin" in key or "api-plugin-generator" in (key or ""):
print(key)
if isinstance(pkg.get("packageJSON"), dict):
for k,v in pkg["packageJSON"].items():
if k in ["type","module","main","exports","scripts","dependencies","devDependencies"]:
print(f" {k}: {v}")
elif k in ("version","resolved","dependencies","peerDependencies"):
print(f" {k}: {v}")
PY
echo "== lock metadata top lockfileVersion =="
python3 - <<'PY'
from pathlib import Path
import json
print(json.load(open("package-lock.json"))["lockfileVersion"])
PYRepository: unraid/api
Length of output: 463
Use the .js extension for the local ESM import.
packages/unraid-api-plugin-generator/tsconfig.json uses module: NodeNext / moduleResolution: nodenext, so relative imports in src/**/*.test.ts need file extensions.
Proposed fix
-import { createPlugin } from "./create-plugin";
+import { createPlugin } from "./create-plugin.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createPlugin } from "./create-plugin"; | |
| import { createPlugin } from "./create-plugin.js"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/unraid-api-plugin-generator/src/create-plugin.test.ts` at line 5,
Update the createPlugin import in create-plugin.test.ts to use the .js
extension, preserving the existing local module target and NodeNext-compatible
ESM resolution.
Source: Coding guidelines
| import { getVendorBundleName, getVendorFullPath } from "./build-vendor-store"; | ||
| import { getAssetUrl } from "./utils/bucket-urls"; | ||
| import { validateStandaloneManifest, getStandaloneManifestPath } from "./utils/manifest-validator"; | ||
| import { writeVendorArchiveInfo } from "./utils/vendor-archive"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if rg -nP 'from "\./(?:utils/)?vendor-archive";' \
plugin/builder/build-txz.ts \
plugin/builder/utils/vendor-archive.test.ts; then
exit 1
fiRepository: unraid/api
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files:\n'
git ls-files plugin/builder/build-txz.ts plugin/builder/utils/vendor-archive.ts plugin/builder/utils/vendor-archive.test.ts
printf '\nRelevant imports (with file context):\n'
for f in plugin/builder/build-txz.ts plugin/builder/utils/vendor-archive.test.ts; do
echo "--- $f"
sed -n '1,30p' "$f"
done
printf '\nPackage / TS config hints:\n'
for f in package.json tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,220p' "$f"
fi
doneRepository: unraid/api
Length of output: 6134
Add .js extensions to the local ESM imports.
These imports follow the repository’s ESM convention, where TypeScript imports should include the .js extension to resolve correctly at runtime.
plugin/builder/build-txz.ts#L14-L14: import from./utils/vendor-archive.js.plugin/builder/utils/vendor-archive.test.ts#L5-L5: import from./vendor-archive.js.
📍 Affects 2 files
plugin/builder/build-txz.ts#L14-L14(this comment)plugin/builder/utils/vendor-archive.test.ts#L5-L5
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/builder/build-txz.ts` at line 14, Update the local ESM imports in
plugin/builder/build-txz.ts (lines 14-14) and
plugin/builder/utils/vendor-archive.test.ts (lines 5-5) to include the .js
extension, targeting writeVendorArchiveInfo and the vendor-archive import
respectively; no other changes are needed.
Source: Coding guidelines
| const directory = await mkdtemp(join(tmpdir(), "vendor-archive-")); | ||
| const configPath = join(directory, "config", "vendor_archive.json"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the temporary directory after the test.
mkdtemp leaves a directory behind on every run. Wrap the test body in try/finally and remove the directory to prevent repeated CI/local runs from accumulating temporary files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/builder/utils/vendor-archive.test.ts` around lines 9 - 10, Update the
test using mkdtemp to wrap its body in a try/finally block, and remove the
created directory in the finally cleanup via the existing filesystem cleanup
utility. Ensure cleanup runs whether the test passes or fails.
| await writeVendorArchiveInfo(configPath, config); | ||
| const firstContents = await readFile(configPath); | ||
| const firstStat = await stat(configPath); | ||
|
|
||
| await writeVendorArchiveInfo(configPath, config); | ||
| const secondContents = await readFile(configPath); | ||
| const secondStat = await stat(configPath); | ||
|
|
||
| expect(secondContents).toEqual(firstContents); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the existing-file update path.
Both writes use the same configuration, so a regression that fails to update stale metadata still passes. Add a second configuration with a changed version/path and assert that the existing file is updated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/builder/utils/vendor-archive.test.ts` around lines 16 - 24, Add a
distinct second configuration in the writeVendorArchiveInfo test with changed
version and path values, then use it for the second write and assert the
existing file contents reflect the updated configuration rather than the
original metadata.
| await writeFile(configPath, contents, "utf8"); | ||
| } | ||
|
|
||
| await utimes(configPath, DETERMINISTIC_MTIME, DETERMINISTIC_MTIME); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'writeFile|chmod|mode|makepkg|chown|tar' plugin/builder apiRepository: unraid/api
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
wc -l plugin/builder/utils/vendor-archive.ts
cat -n plugin/builder/utils/vendor-archive.ts
echo "== focused plugin/builder files =="
git ls-files 'plugin/builder/**' | sed -n '1,200p'
rg -n "vendor_archive|vendor-archive|txz|tar|cpio|compress|makepkg|chmod|chmod|utimes|writeFile|appendFile|mkdir" plugin/builder -S --glob '!**/node_modules/**' --glob '!**/dist/**'
echo "== focused api scripts references =="
rg -n "vendor_archive|vendor-archive|txz|makepkg|plugin.*builder|builder/utils/vendor-archive" api -S --max-count 100Repository: unraid/api
Length of output: 25756
🌐 Web query:
Node.js fs.promises writeFile default mode permissions umask documentation
💡 Result:
In Node.js, the fs.promises.writeFile method uses a default file mode of 0o666 when creating a new file [1][2][3]. This mode specifies that the file is readable and writable by the owner, group, and others [4]. Important considerations regarding this mode include: 1. Interaction with umask: The mode you specify (or the default 0o666) is not necessarily the final permission set for the file [4]. When a file is created, the operating system applies the process umask, which removes specific permission bits [4]. For example, if the process umask is 0o022, a file created with the default 0o666 mode will result in final permissions of 0o644 (read/write for the owner, and read-only for the group and others) [4]. 2. Scope of the mode option: The mode option only affects the file if it is newly created [1][3]. If the file already exists and is being overwritten by writeFile, the existing file permissions remain unchanged [1][3]. 3. Thread-safety warning: Relying on the current process umask or modifying it using process.umask can be problematic [4]. Node.js has deprecated the no-argument form of process.umask (DEP0139) because it is not thread-safe; calling it involves a temporary change to the process mask that could affect concurrent file operations in a multi-threaded or asynchronous environment [4]. Instead of modifying the umask, it is generally recommended to explicitly pass the desired mode in the options object if specific permissions are required for the new file [4].
Citations:
- 1: https://nodejs.org/docs/v22.11.0/api/fs.html
- 2: https://nodejs.org/api/fs.html
- 3: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 4: https://www.thenodebook.com/file-system/permissions-metadata-edge-cases
Normalize vendor_archive.json permissions to a fixed mode.
Normalize the file permissions after writing, for example by setting the mode to 0o644, so TXZ builds do not embed differing metadata for a newly created JSON file when builders run under different umasks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/builder/utils/vendor-archive.ts` around lines 33 - 36, After writing
vendor_archive.json in the archive-generation flow, explicitly set its
permissions to the fixed 0o644 mode before or alongside the deterministic
timestamp update. Update the code around writeFile and utimes so builds produce
consistent file metadata regardless of the process umask.
2 participants
Summary
Verification
CI=1 pnpm --dir plugin test(35 Vitest, 62 extractor, shell detection)pnpm --dir packages/unraid-api-plugin-generator testpnpm --dir packages/unraid-api-plugin-generator buildnpm installandnpm run validateSummary by CodeRabbit