fix: shim spillover #138
fix: shim spillover #138
Conversation
📝 WalkthroughWalkthroughIntroduces a new test port (dup_test) and updates POSIX env cooking to handle env labels, shim spillover, and alias symlinks. Adjusts function signatures to pass optional env_name and to return shim levels. Exposes std_fs via re-export. Adds a test validating duplicate exec handling, spillover, and aliases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test (prov_exec_spillover_bash)
participant Env as EnvsCtx
participant POSIX as posix::cook
participant Shim as shim_link_paths
participant Act as write_activators
participant FS as Filesystem
participant Dup as dup (shimmed exec)
Test->>Env: activate("main")
Env->>POSIX: cook(recipe, env_key, env_name?, env_dir, loaders)
POSIX->>POSIX: derive env_label (env_name or sanitized env_key)
POSIX->>Shim: link bin targets (alias_env_label=env_label)
Shim-->>POSIX: max_bin_level (e.g., 2)
POSIX->>Shim: link lib/include targets
Shim-->>POSIX: max levels
POSIX->>POSIX: build path_vars as Vec per var (spillover dirs)
POSIX->>Act: write_activators(env_vars, path_vars, hooks, aliases)
Act->>FS: write POSIX/Fish activation scripts
Note right of Act: PATH includes bin, bin2, ... per level
Test->>Dup: run dup (via bin/dup)
Dup-->>Test: "one"
Test->>Dup: run dup (via bin2/dup or alias symlink)
Dup-->>Test: "two"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/deno_utils/mod.ts (1)
262-269: Unbounded download request: add a configurable timeout.
.timeout(undefined)can hang indefinitely on network stalls. Add an optional timeout to args and default it to a sane value.Apply this diff:
export type DownloadFileArgs = { downloadPath: string; tmpDirPath: string; url: string; name?: string; mode?: number; headers?: Record<string, string>; + timeout?: dax.Delay; // optional network timeout }; @@ await $.request(url) .header(headers) - .timeout(undefined) + .timeout(args.timeout ?? "10m") .pipeToPath(tmpFilePath, { create: true, mode });Also applies to: 293-296
🧹 Nitpick comments (9)
src/deno_utils/mod.ts (3)
159-167: Harden NotFound handling and avoid swallowing unknown throwables.The current check may accidentally swallow non‑Error throws and relies on
name. Prefer Deno’s typed error.Apply this diff:
async removeIfExists(path: Path | string) { const pathRef = $.path(path); try { await pathRef.remove({ recursive: true }); } catch (err) { - if (err! instanceof Error && err.name != "NotFound") { - throw err; - } + if (err instanceof Deno.errors.NotFound) { + // ignore + } else { + throw err; + } } return pathRef; },
175-178: Deduplicate worker‑context helpers; keep one and add an alias.
inWorker()andisInWorkerContext()are identical. Keep one to avoid drift and provide a back‑compat alias.Apply this diff:
export function inWorker() { return typeof WorkerGlobalScope !== "undefined" && self instanceof WorkerGlobalScope; } +// Back-compat alias; prefer `inWorker`. +export const isInWorkerContext = inWorker; @@ -export function isInWorkerContext() { - return typeof WorkerGlobalScope !== "undefined" && - self instanceof WorkerGlobalScope; -} +// (removed duplicate; use alias above)Also applies to: 503-506
209-216: Be explicit about protocol checks.Use strict comparisons for clarity and to avoid false positives.
Apply this diff:
- if (url.protocol.match(/^http/)) { + if (url.protocol === "http:" || url.protocol === "https:") { let request = $.request(url).timeout(timeout);tests/envs.ts (1)
209-237: Nice spillover coverage; add PATH-execution assertions for aliasesCurrent checks execute alias symlinks via absolute paths. Consider also asserting they resolve on PATH after activation (e.g.,
type dup-main-1and$(dup-main-1)), which future-proofs PATH handling.@@ [ "$(.ghjk/envs/main/shims/bin/dup-main-1)" = "one" ] || exit 107 [ "$(.ghjk/envs/main/shims/bin/dup-main-2)" = "two" ] || exit 108 +type dup-main-1 >/dev/null 2>&1 || exit 109 +type dup-main-2 >/dev/null 2>&1 || exit 110 +[ "$(dup-main-1)" = "one" ] || exit 111 +[ "$(dup-main-2)" = "two" ] || exit 112ports/dup_test.ts (1)
51-57: Create parent dir for bin and make script slightly more robustEnsure
bin/exists before writing and avoid partial writes on error.override async download(args: DownloadArgs) { const conf = confValidator.parse(args.config); - await $.path(args.downloadPath).join("bin", "dup").writeText( - `#!/bin/sh\necho ${conf.output}`, - { mode: 0o700 }, - ); + const binDir = $.path(args.downloadPath).join("bin"); + await binDir.mkdirp(); + await binDir.join("dup").writeText( + `#!/bin/sh +set -e +printf '%s\n' "${conf.output}"`, + { mode: 0o700 }, + ); }src/ghjk/systems/envs/posix.rs (4)
90-103: Alias label sanitization is sensibleAlnum/
-/_only and short-key fallback are reasonable. Consider documenting max label length if you ever enforce one.
175-253: shim_link_paths: deterministic ordering and alias target
- Ordering: expansion via
glob::glob()may not be deterministic across filesystems. If globbing is used for execs, you may see flakiness in which entry lands inbin/vsbin2/. Consider stabilizing with a sort after expansion (only when inputs contain globs), or document that install order determines priority and avoid globs in execs.- Alias target: alias symlinks point to the real entry, not the created shim. That’s fine, but if you later rewrite shim targets (e.g., for wrappers), aliases won’t follow. Linking aliases to the created shim path would keep behavior uniform.
- for entry in glob::glob(path_str)? { - expanded.push(entry?); - } + // NOTE: glob expansion order may vary; collect then sort for stability + let mut matches: Vec<PathBuf> = glob::glob(path_str)?.collect::<Result<_, _>>()?; + matches.sort(); + expanded.extend(matches);
533-547: Use fixed-string grep for PATH cleanup to avoid regex pitfalls
grep -vE "^...$"treats.and other metachars specially. Use-F/-xfor exact fixed-string matching.- writeln!( - buf, - r#"GHJK_CLEANUP_POSIX=$GHJK_CLEANUP_POSIX'{key}=$(echo "${key}" | tr ":" "\n" | grep -vE '\''"^{safe_val}$"'\'' | tr "\n" ":");{key}="${{{key}%:}}";';"# - )?; + writeln!( + buf, + r#"GHJK_CLEANUP_POSIX=$GHJK_CLEANUP_POSIX'{key}=$(echo "${key}" | tr ":" "\n" | grep -vFx '\''{safe_val}'\'' | tr "\n" ":");{key}="${{{key}%:}}";';"# + )?;
713-716: Typo in fish activator headerShould read “should not be executed directly.”
-# it should be executed directly +# it should not be executed directly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ports/dup_test.ts(1 hunks)src/deno_utils/mod.ts(1 hunks)src/ghjk/systems/envs.rs(1 hunks)src/ghjk/systems/envs/posix.rs(13 hunks)tests/envs.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T06:06:50.518Z
Learnt from: Yohe-Am
PR: metatypedev/ghjk#130
File: ports/lade_ghrel.port.ts:22-22
Timestamp: 2025-08-09T06:06:50.518Z
Learning: In ghjk port manifests (files matching pattern `ports/*.port.ts` or `ports/*.ts`), the `version` field in the manifest object represents the version of the port module itself, not the version of the tool/software that the port installs. The actual tool version is typically passed via `args.installVersion` parameter in methods like `downloadUrls()`.
Applied to files:
ports/dup_test.ts
🧬 Code graph analysis (1)
ports/dup_test.ts (3)
src/deno_ports/mod.ts (1)
osXarch(34-41)src/sys_deno/ports/types.ts (3)
InstallConfigSimple(258-260)DownloadArgs(347-350)InstallArgs(352-356)src/deno_utils/mod.ts (2)
$(106-173)std_fs(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-e2e (macos-13, darwin/x86_64)
- GitHub Check: test-e2e (macos-14, darwin/aarch64)
- GitHub Check: test-e2e (ubuntu-22.04, linux/x86_64)
- GitHub Check: test-e2e (ubuntu-22.04-arm, linux/aarch64)
- GitHub Check: test-pre-commit
🔇 Additional comments (11)
src/deno_utils/mod.ts (1)
20-20: Expose std_fs via barrel export — looks good.This aligns with existing
Pathre‑export and helps consumers import from a single module. Please confirm no cycles withdeps.tsand update docs since this expands the public surface.tests/envs.ts (1)
9-9: Good: dedicated test port for duplicate execsImporting the dup_test port isolates spillover behavior and keeps tests deterministic.
ports/dup_test.ts (3)
59-64: Install logic is cleanRemove-if-exists then copy keeps installs idempotent and simple.
41-49: No-env execution is appropriate for duplicate instancesReturning
{}fromexecEnv()avoids cross-instance env leakage.listAll()is fine for this test port.
18-24: Manifest semantics LGTMVersion is for the port module (not tool), platforms cover all OS/arch via
osXarch.src/ghjk/systems/envs/posix.rs (5)
12-15: New env_name parameter: good defaulting behaviorPreferring
env_namefor GHJK_ENV with fallback toenv_keyis the right UX.
37-40: GHJK_ENV assignment uses env_name correctlySimple and explicit—no concerns.
104-108: shim_link_paths return levels are correctly threadedLevels per category enable PATH spillover; passing the label only for bin is appropriate.
124-147: PATH/LIB/INCLUDE spillover construction looks correctThe
dirs_forhelper producesbin,bin2, … and mirrors to lib/include vars. Nice.
164-171: Join multiple path values when returning env_varsJoining late keeps activator generation working on structured data. Good separation.
src/ghjk/systems/envs.rs (1)
429-433: Argument order to posix::cook verified — no other callers foundRepository search shows only the call at src/ghjk/systems/envs.rs:426 (the updated invocation), so no remaining callers use the old parameter order.
| for (key, values) in path_vars { | ||
| // prepend all entries in order (bin, bin2, ...) | ||
| for val in values.iter().rev() { | ||
| let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''"); | ||
| writeln!( | ||
| buf, | ||
| r#"set --global --export --prepend {key} "{safe_val}";"# | ||
| )?; | ||
| } | ||
| writeln!(buf)?; | ||
| } |
There was a problem hiding this comment.
Fish deactivation doesn’t remove injected PATH entries
You prepend spillover dirs but don’t append matching cleanup to GHJK_CLEANUP_FISH. Re-activations will accumulate duplicates in fish shells.
- for (key, values) in path_vars {
- // prepend all entries in order (bin, bin2, ...)
- for val in values.iter().rev() {
+ for (key, values) in path_vars {
+ // prepend all entries in order (bin, bin2, ...)
+ for val in values.iter().rev() {
let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''");
writeln!(
buf,
r#"set --global --export --prepend {key} "{safe_val}";"#
)?;
- }
+ }
+ // add cleanup to filter out each injected value exactly
+ for val in values {
+ let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''");
+ writeln!(
+ buf,
+ r#"set --global --append GHJK_CLEANUP_FISH 'set -l __ghjk_new; for p in ${key}; if test "$p" != "{safe_val}"; set -a __ghjk_new "$p"; end; end; set --global --export {key} $__ghjk_new;';"#
+ )?;
+ }
writeln!(buf)?;
}📝 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.
| for (key, values) in path_vars { | |
| // prepend all entries in order (bin, bin2, ...) | |
| for val in values.iter().rev() { | |
| let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''"); | |
| writeln!( | |
| buf, | |
| r#"set --global --export --prepend {key} "{safe_val}";"# | |
| )?; | |
| } | |
| writeln!(buf)?; | |
| } | |
| for (key, values) in path_vars { | |
| // prepend all entries in order (bin, bin2, ...) | |
| for val in values.iter().rev() { | |
| let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''"); | |
| writeln!( | |
| buf, | |
| r#"set --global --export --prepend {key} "{safe_val}";"# | |
| )?; | |
| } | |
| // add cleanup to filter out each injected value exactly | |
| for val in values { | |
| let safe_val = val.replace("\\", "\\\\").replace("'", "'\\''"); | |
| writeln!( | |
| buf, | |
| r#"set --global --append GHJK_CLEANUP_FISH 'set -l __ghjk_new; for p in ${key}; if test "$p" != "{safe_val}"; set -a __ghjk_new "$p"; end; end; set --global --export {key} $__ghjk_new;';"# | |
| )?; | |
| } | |
| writeln!(buf)?; | |
| } |
1 participant
myExec-myEnv-2Summary by CodeRabbit