refactor: rewrite in rust (cli) #108
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive set of changes across multiple files and modules in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| .await | ||
| .into_iter() | ||
| .collect() | ||
| /* let out = read_files | ||
| .into_co_stream() |
There was a problem hiding this comment.
One of the deadlocks was in upstream code from futures-concurrency.
| return Ok(res); | ||
| }; | ||
|
|
||
| let res = tokio::select! { |
There was a problem hiding this comment.
The other "deadlock" was the callback worker not shutting down properly when the deno runtime is tore down. then2 doesn't reject the promise which I suppose makes sense, there's no eventloop anymore?,
Yohe-Am
changed the title
Yohe-Am
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (54)
src/ghjk/main.rs (2)
54-54: Suppressing Deno UpdatesSetting
DENO_NO_UPDATE_CHECKto1ensures consistent builds but also prevents users from being notified about critical Deno updates. Keep this in mind for potential support/troubleshooting.
59-64: Logging & Blocking the CLI ExecutionLogging the CLI version with
debug!is helpful. The.block_on(cli::cli())call looks straightforward. Consider providing user-facing messages (viainfo!orprintln!) if lengthy tasks or user interactions occur.src/ghjk/systems.rs (2)
49-49:commands_fnField inErasedSystemInstanceStoring an erased closure for commands retrieval is a flexible design, but be cautious of potential runtime errors if any system’s
commands()method returns incompatible types. Thorough integration tests are advised.
129-137:CliCommandActionandSystemCliCommandStructDefining these types in a single place clarifies how commands are stored and executed. This is a neat design. Just watch out for large nested sub-commands that might require deeper hierarchical checks or lazy loading strategies.
src/deno_systems/bindings.ts (2)
16-23: Preventing Event Loop Exit withsetIntervalLeaving an empty
setIntervalto keep the event loop alive can introduce confusion or unexpected resource usage. Consider a more explicit mechanism (e.g., a promise-based wait) so it’s clear why the process remains open.
139-160:commandBinding(...)Execution FlowParsing the raw command, validating with
cliCommand.parse(), and setting theaction_cb_keyinGhjk.callbacksis elegantly structured. If commands can fail or be missing fields, ensure robust error handling.tests/utils.ts (3)
36-36: Creating the.ghjkData DirectoryExplicitly ensuring the directory is created is good practice to avoid runtime errors. Keep track of cleanup steps to prevent leftover directories from accumulating in ephemeral test environments.
54-66: Comprehensive Environment SetupProviding environment variables such as
BASH_ENV,ZDOTDIR, andGHJK_DATA_DIRensures consistent testing. Confirm that ephemeral test environments don’t introduce concurrency issues if multiple tests run in parallel.
172-176: Removing Docker Test RunnerAbandoning Docker-based tests simplifies local testing. If cross-platform coverage is desired, consider reintroducing container-based tests or GitHub Actions matrix builds for broader reliability.
tests/envs.ts (1)
61-61: Capture Exit StatusPrinting
$?before re-activating environment is a good debugging step. If capturing this value becomes critical, consider logging it in the test harness for easier test failure correlation.src/ghjk/systems/deno.rs (5)
8-8: Consider adding a doc comment for theclimodule
A brief explanation of theclimodule's purpose might help future maintainers.
13-14: Potential concurrency considerations
Storing aJoinHandlewithin anArc<Mutex<Option<...>>>may risk blocking other tasks if the lock is frequently taken for extended periods. Consider whether this is the best concurrency pattern or if a more granular approach (like a dedicated channel or watch) might be preferable.
41-41: Consider external configuration for the binding path
Hardcodingsrc/deno_systems/bindings.tscould be limiting. If possible, consider referencing a config or environment variable.
144-160: Error handling injoin_exit_code_watcher
Logging is robust, covering multiple error states. Consider whether you need to re-raise or transform the error for top-level handling if it’s critical.
283-301:commands()method structure
MappingCliCommandDescintoSystemCliCommandis a good approach. Validate untrusted payload carefully since it’s deserialized from external code.src/ghjk/host/hashfile.rs (3)
129-150: Concurrent file digests
Usingfutures::future::join_allis efficient. Ensure no huge memory overhead if read_files is large (watch out for thousands of files).
Line range hint
154-160: Commented-out code
If the commented co-stream approach won’t be used again, consider removing it to reduce clutter.
194-221:SharedFileContentDigestFuture
This shared future structure is a neat optimization. Thedashmapapproach allows concurrency though if the number of files is large, watch out for potential overhead or locking contention.src/ghjk/ext/callbacks.rs (3)
41-41:CallbackLine::newreturning multiple objects.
Returning(Self, CallbacksHandle)is fine. Alternatively, consider a wrapper type in future expansions for clearer conveyance of shared state.
57-57: Clarity in debug logs.
The commented-out debug lines might be useful for diagnosing initialization issues. Consider re-enabling them if needed.Also applies to: 60-61
116-124: ExtractingCallbackCtxfrom config.
Locking ensures concurrency safety. Watch for potential lock contention if many child workers attempt to callmutex.take()concurrently.src/ghjk/host.rs (1)
51-54: Extended function signature forsystems_from_ghjkfile.
Now includesghjkdir_path. This improves clarity and reduces reliance on fields insideHostCtx.src/ghjk/cli.rs (1)
13-21: Initial asynccli()function.
Contains commented-out debug code. May be helpful for diagnosing blocking threads if needed.src/denort/worker.rs (4)
13-17: Consider clarifying concurrency usage in doc comments
This section introduces a dedicated worker thread for Deno tasks. While the doc is clear about single-threaded constraints in Deno internals, it might be helpful to add more context on why a separate thread is chosen and how concurrency is safely managed.
47-53: Surface potential worker preparation errors
Here, errors frommodule_workerare returned to theresponse_channeland then silently handled. Consider logging or tracing these errors more prominently, so that failures in module preparation are not overlooked.
140-185: Validate error handling infrom_config
All errors are currently wrapped withferr!(Box::new(err)). Ensure that this layering adds sufficient context for debugging. In some cases, logging the original error before re-wrapping can make debugging easier.
354-415: Refine event loop exit logic
The code loops onrun_event_loopand checks signals to break from the loop. While it’s clear, consider more explicit handling of partial or graceful exits. For instance, ifbeforeunloadorbeforeexitevents fail, you might want to propagate that error upward or log the event.src/ghjk/js/runtime.js (1)
7-8: Hide global injection from users
ExportingGhjkwhile it’s also injected globally can confuse new contributors. Consider adding a short comment clarifying that global injection is purely an internal convenience, and thatGhjkshould be imported via this file in user code.modules/mod.ts (1)
2-2: Update doc comments to reflect array-based commands
Switching fromRecord<string, cliffy_cmd.Command<any>>toCliCommand[]is a clearer design for command listing. Just confirm that any code referencing the old shape has been updated accordingly.Also applies to: 21-21
deno.jsonc (1)
4-4: Document the updated workflow for running the CLI.
Since"self"now callscargo run -p ghjkinstead ofdeno run ..., users who rely on the olddeno runcommand may need instructions on installing and using Rust. Make sure to communicate or document this crucial change for contributors.install.ts (2)
8-12: Confirm the usefulness of the pre-cached modules.
The newly added imports (./modules/std.ts,./port.ts, and./ports/mod.ts) are pulled in advance to be cached. Ensure these modules are actually needed by the installer. Otherwise, consider removing them for a more concise installer code.
30-31: Handle empty or invalidGHJK_DATA_DIRvalues.
WhenDeno.env.get("GHJK_DATA_DIR")is set but empty or invalid, this logic will fall back todefaultInstallArgs.ghjkDataDir. If you want to detect misconfiguration or prohibit empty strings, consider explicitly verifying the value rather than silent fallback.scripts/dev.ts (1)
32-36: Rename environment variable for clarity.
GHJK_SHARE_DIRenvironment variable is still set to the data directory. To maintain semantic clarity, consider renaming it to something likeGHJK_DATA_DIR, since the code now usesghjkDataDiras the canonical data location.- GHJK_SHARE_DIR: ghjkDataDir.toString(), + GHJK_DATA_DIR: ghjkDataDir.toString(),src/xtask/main.rs (1)
53-61: Evaluate necessity of commented-out code.The
Commands::Runvariant is commented out. If you plan to restore or remove it permanently, please clarify. Leaving it indefinitely commented might confuse future maintainers.modules/tasks/mod.ts (5)
3-3: Check for potential re-exports.
import { zod } from "../../deps/cli.ts";is straightforward. If multiple modules needzod, consider centralizing or re-exporting for consistency.
64-66: Recommend naming clarity.Split out variables with clearer naming, e.g.,
taskNamesSetinstead ofnamedSet. This small improvement could reduce confusion about what's stored in the set.
67-89: Centralize task argument handling.The code appends task arguments in-line. If your tasks grow more complex, consider a helper function that standardizes argument parsing, so other modules or commands can reuse it.
90-92: Typo in error message."unexepected version tag" has a small misspelling.
- throw new Error(`unexepected version tag deserializing lockEntry`); + throw new Error(`unexpected version tag deserializing lockEntry`);
93-93: Use a descriptive return name.
return out;is fine, but consider returning directly from the array definition or naming it more explicitly (e.g.,return commands;).src/ghjk/log.rs (1)
55-57: Commented-out conditional compilation.Commenting out the
#[cfg(not(test))]hook might lead to unexpected behaviors in non-test builds. Make sure it’s intentional.install/mod.ts (2)
99-100: Complete references to new directory properties.
The newly addedghjkDataDirandghjkConfigDirproperties replace older references (e.g.ghjkShareDir). Make sure that all relevant parts of the codebase referencing these properties are updated to avoid confusion or mismatched paths.
153-161: Shell hook installation approach
You are filtering out lines matchingshellHookMarkerand then appending the new source line at the end. This is typically robust. However, if a user’s shell RC has structural requirements (e.g., anexitor other shell logic), you might want to insert the line somewhere more deterministic.ghjk.ts (3)
11-13: Separate documentation for Rust and Deno requirements.
You introduced a clear comment referencing Deno’s requirements right aboveRUST_VERSION. Consider separating or clarifying them a bit more, so it’s obvious how each piece correlates to the overall environment.
29-53: Complex environment variable setup forRUST_LOG.
Using array-based configuration to dynamically build theRUST_LOGstring is powerful, but a bit verbose. Unless you need frequent expansions, consider a simpler approach.
58-61: Installation ordering
Installing cmake, protoc, and Rust together may cause extended build times. Verify that partial failures during these grouped installs do not break the rest of your environment.modules/envs/posix.ts (2)
309-309: Optional check for the 'ghjk' binary.The newly added shim call assumes that 'ghjk' automation is available. If 'ghjk' might not be installed, consider adding a guard or a fallback. Otherwise, an uninstalled
ghjkcould lead to runtime errors.
377-377: Optional check for the 'ghjk' binary in fish script.Similar to the posix version, add a guard in case
ghjkis not installed. This ensures a smoother user experience in fish shells.utils/mod.ts (1)
328-330: Consider implementing XDG directory standards.The comments mention a
FIXMEfor using proper XDG directories. Migrating from hard-coded paths to standard XDG config and data paths increases cross-platform support and user experience..gitignore (1)
16-16: Consider removing redundant ignore patternThe pattern
src/play/*.rsappears redundant since there's already a more genericplay.*pattern above. This could lead to confusion about which files are actually being ignored.Consider either:
- Removing this line since
play.*already covers it, or- Adding a comment explaining why both patterns are needed if there's a specific reason
src/play/Cargo.toml (1)
19-19: Consider limiting tokio featuresThe
fullfeature flag for tokio includes all features, which might be unnecessary and increases compilation time. Consider explicitly listing only the required features.Example of a more targeted feature set:
-tokio = { workspace = true, features = ["full", "parking_lot"] } +tokio = { workspace = true, features = ["rt", "rt-multi-thread", "macros", "parking_lot"] }Cargo.toml (1)
7-7: Consider documenting the pre-release changesThe version change to
0.3.0-rc.1indicates this is a release candidate. Consider documenting the significant changes and any breaking changes that led to this version bump.src/ghjk/Cargo.toml (1)
55-58: Optimize async runtime configurationThe async stack has been updated with new dependencies:
- async-trait
- futures-concurrency
- futures with minimal features
- tokio with full features
Consider optimizing tokio features by replacing
fullwith specific needed features to reduce binary size:-tokio = { workspace = true, features = ["full", "parking_lot", "tracing"] } +tokio = { workspace = true, features = ["rt", "rt-multi-thread", "macros", "io-util", "fs", "parking_lot", "tracing"] }install.sh (1)
15-26: Consider adding version validation.While the version detection logic works, it would be beneficial to validate the version format.
LATEST_VERSION=$(curl "$RELEASE_URL/latest" -s -L -I -o /dev/null -w '%{url_effective}') LATEST_VERSION="${LATEST_VERSION##*v}" + +# Validate version format (e.g., X.Y.Z or X.Y.Z-rc.N) +if ! echo "$LATEST_VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+(-rc\.[0-9]+)?$'; then + echo "Error: Invalid version format detected: $LATEST_VERSION" 1>&2 + exit 1 +fisrc/ghjk/utils.rs (1)
167-194: Consider optimizing buffer size for hash_reader.The hash_reader implementation is good, but the buffer size could be optimized.
- let mut buf = vec![0u8; 4096]; + // Use a larger buffer size for better performance with large files + let mut buf = vec![0u8; 65536]; // 64KB buffer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
-
.ghjk/deno.lockis excluded by!**/*.lock -
Cargo.lockis excluded by!**/*.lock -
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
-
.cargo/config.toml(1 hunks) -
.ghjk/lock.json(17 hunks) -
.github/workflows/nightly.yml(1 hunks) -
.github/workflows/tests.yml(4 hunks) -
.gitignore(1 hunks) -
.pre-commit-config.yaml(0 hunks) -
Cargo.toml(2 hunks) -
README.md(4 hunks) -
deno.jsonc(1 hunks) -
docs/manual.md(3 hunks) -
ghjk.ts(6 hunks) -
host/mod.ts(0 hunks) -
install.sh(1 hunks) -
install.ts(2 hunks) -
install/bash-preexec.sh(1 hunks) -
install/ghjk.sh(0 hunks) -
install/hook.fish(1 hunks) -
install/hook.sh(1 hunks) -
install/mod.ts(4 hunks) -
install/utils.ts(2 hunks) -
main.ts(0 hunks) -
modules/envs/mod.ts(7 hunks) -
modules/envs/posix.ts(2 hunks) -
modules/mod.ts(2 hunks) -
modules/ports/mod.ts(4 hunks) -
modules/ports/worker.ts(1 hunks) -
modules/tasks/mod.ts(3 hunks) -
rust-toolchain.toml(1 hunks) -
scripts/dev.ts(2 hunks) -
src/deno_systems/bindings.ts(3 hunks) -
src/deno_systems/mod.ts(1 hunks) -
src/deno_systems/types.ts(1 hunks) -
src/denort/lib.rs(7 hunks) -
src/denort/promises.rs(1 hunks) -
src/denort/unsync.rs(1 hunks) -
src/denort/worker.rs(1 hunks) -
src/ghjk/Cargo.toml(3 hunks) -
src/ghjk/cli.rs(1 hunks) -
src/ghjk/config.rs(1 hunks) -
src/ghjk/ext.rs(2 hunks) -
src/ghjk/ext/callbacks.rs(8 hunks) -
src/ghjk/host.rs(14 hunks) -
src/ghjk/host/deno.rs(1 hunks) -
src/ghjk/host/hashfile.rs(6 hunks) -
src/ghjk/js/00_runtime.js(2 hunks) -
src/ghjk/js/runtime.js(1 hunks) -
src/ghjk/log.rs(3 hunks) -
src/ghjk/main.rs(1 hunks) -
src/ghjk/systems.rs(5 hunks) -
src/ghjk/systems/deno.rs(9 hunks) -
src/ghjk/systems/deno/cli.rs(1 hunks) -
src/ghjk/utils.rs(4 hunks) -
src/play/Cargo.toml(1 hunks) -
src/play/main.rs(1 hunks) -
src/xtask/main.rs(2 hunks) -
tests/envHooks.ts(1 hunks) -
tests/envs.ts(2 hunks) -
tests/ports.ts(1 hunks) -
tests/reloadHooks.ts(1 hunks) -
tests/test-alpine.Dockerfile(0 hunks) -
tests/test.Dockerfile(0 hunks) -
tests/test.Dockerfile.dockerignore(0 hunks) -
tests/todo.ts(1 hunks) -
tests/utils.ts(5 hunks) -
utils/mod.ts(2 hunks)
💤 Files with no reviewable changes (7)
- tests/test.Dockerfile.dockerignore
- .pre-commit-config.yaml
- main.ts
- tests/test.Dockerfile
- install/ghjk.sh
- tests/test-alpine.Dockerfile
- host/mod.ts
✅ Files skipped from review due to trivial changes (3)
- tests/todo.ts
- rust-toolchain.toml
- src/play/main.rs
👮 Files not reviewed due to content moderation or server errors (3)
- install/bash-preexec.sh
- docs/manual.md
- .ghjk/lock.json
🧰 Additional context used
🪛 Shellcheck (0.10.0)
install/bash-preexec.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 137-137: BP_PIPESTATUS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 241-241: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 286-286: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 312-312: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
🪛 actionlint (1.7.4)
.github/workflows/tests.yml
59-59: label "custom-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 Gitleaks (8.21.2)
.ghjk/lock.json
383-383: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
389-389: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (280)
src/ghjk/main.rs (4)
34-35: Modules Are Properly Organized
Creating separate cli and config modules helps improve maintainability and clarity of responsibilities within the codebase. This structure is beneficial for future scalability.
43-46: Change in main() Return Type
Switching fn main() -> Res<std::process::ExitCode> and handling CLI invocation inline is a clean design. However, ensure that any unexpected CLI parsing errors are gracefully handled and produce a meaningful exit code.
68-68: Shadow Macro Usage
The shadow!(shadow); macro automatically produces build-time constants. Double-check that all references to shadow::PKG_VERSION or other generated constants remain consistent throughout the code and documentation.
72-73: New GhjkCtx Fields
Introducing deno: denort::worker::DenoWorkerHandle and config: config::Config centralizes the context significantly. Confirm that downstream modules now rely on GhjkCtx for consistent context references and eliminate any stale references to old config paths.
src/ghjk/systems.rs (4)
36-37: New commands() Method in SystemInstance
Adding an async commands() method for retrieving CLI commands is a meaningful abstraction. It supports future expansion without complicating the core trait responsibilities. Ensure all implementors handle errors appropriately.
Line range hint 72-95: Config Downcasting & Closures
The load_config_fn and subsequent closure logic for downcasting lock state is well-structured. Just be sure the downcast() usage is always safe and cannot produce partial type mismatches. Where possible, handle errors gracefully instead of calling expect_or_log.
116-118: commands() Wrapper Implementation
Defining a public async commands() that calls the closure is a concise approach. Confirm users of this API have robust error-handling in place, since a failure here can block CLI operations system-wide.
139-147: Custom Debug Implementation
Customizing the debug output to exclude excessive details (like the actual function pointer for action) helps keep logs readable. This approach is a good balance between diagnostic insight and clutter reduction.
src/deno_systems/bindings.ts (2)
27-33: Configuration Object Preservation
The config object redefines critical paths (ghjkfile, ghjkdir, etc.). This is helpful, but ensure you handle edge cases (e.g., missing ghjkfile). The optional properties look correct, but be cautious with forced unwraps in other parts of the code.
127-135: Registering CLI Commands Callback
Lazy-loading commands via the callback is a neat design for asynchronous registration. Verify that every command adheres to the CLI schema (e.g. well-formed sub-commands, no naming collisions).
tests/utils.ts (2)
19-26: E2eTestCase Type
Defining E2eTestCase with optional envVars and multiple endpoints is clear. If concurrency or more advanced orchestration is needed, consider additional fields or specialized test runner logic.
45-52: Creating a Shell Script Wrapper
Generating a shell script to run the local ghjk binary is a nice approach. If different OS or shells are supported, verify permissions and line-endings match the user environment.
tests/envs.ts (2)
50-50: POSIX Test Logic: Checking $SONG
The environment checks are straightforward. Keep an eye on the exit codes (like 1010, 1011) to ensure they’re consistent with bigger test suites if you scale beyond environment tests.
Line range hint 172-172: Extensive Environment Inheritance Cases
The new environment inheritance tests (env_inherit_from_envs, env_inherit_from_tasks, etc.) are thorough. Good job covering diamond inheritance. Keep ensuring that any cyclical or invalid references raise proper errors.
src/ghjk/systems/deno.rs (19)
6-7: Imports look fine
No issues found with these imports.
21-21: Verify backward compatibility
Changing the terminate return type from Res<i32> to Res<()> could break existing dependents if they're expecting an exit code. Please ensure no downstream usage is impacted.
29-30: Error handling in term_signal.send(true)
If sending on this watch channel fails, the code logs but continues. Clarify whether this situation is recoverable and handle errors accordingly if needed.
38-39: Signature expansion is fine
Adding ghjkdir_path to the function parameters seems coherent with the added requirement to return the DenoSystemsContext. Ensure all callers pass valid paths.
51-54: ConfigRef struct initialization
Looks good. Keep in mind the importance of maintaining consistent field naming and usage across the codebase so you don't break other references.
63-63: Config destructuring
Decoupling the destructured fields from gcx.config here is neat. Just make sure any new config fields in the future are also handled.
67-72: Unwrap risk on file_path.parent()
You use file_path.parent().unwrap(). If file_path is at root, .parent() might be None. Consider whether you need a safe guard or if you are certain file_path is not root.
100-100: Callbacks handle retrieval
Straightforward usage. Just confirm it's not empty or invalid.
125-126: Drive-till-exit returns triple
Make sure no resources are left dangling if the tuple _ element is unused.
127-138: Comprehensive error messages in tokio::select!
The code thoroughly logs unexpected terminations and exit codes. This is good for debugging.
139-140: Manifest deserialization
Deserializing from serde_json::Value is straightforward. Keep a close eye on schema changes to avoid runtime errors.
142-142: Cloning gcx.deno
Cloning here is safe for usage in the closure. No issues found.
183-183: Return statement
Clean final return of (manifests, scx). Looks good.
197-197: DenoSystemsContext field
Storing the entire context is simpler than using Arc. This design is more direct but keep an eye on memory usage if DenoSystemManifest is cloned massively.
201-201: Use of #[tracing::instrument]
Great for debugging. No concerns here.
203-203: Debug trace
Adds clarity when diagnosing system initialization steps.
212-212: Deno system initialization
Properly placed trace statement.
227-227: Storing cli_commands_cb_key
No immediate issues. Ensure the remote TS side handles it consistently.
232-232: Context in DenoSystemInstance
Carrying the full context here is consistent with the approach to simpler references.
src/ghjk/host/hashfile.rs (7)
19-26: HashfileError enum
These variants accurately capture parsing vs. generic errors. Consider expanding coverage for filesystem-level exceptions if needed.
28-58: from_result method
Nicely organizes hashing logic. The concurrency pattern (awaiting hash calls) appears correct.
63-69: Error handling in from_file
Returning Ok(None) when file is not found is a good way to handle optional existence. This signature clarifies usage.
Line range hint 72-107: is_stale logic
Great approach to confirm changes in env vars, file presence, or content digests. The staged checks help isolate stale triggers.
115-115: env_var_digests
Straightforward function. The usage of hash_str for environment variables is reliable.
164-170: Skipping non-existent files
Returning Ok(None) for missing files is practical. The approach gracefully ignores them.
174-179: file_content_digest_hash usage
Wrapping the result in a JSON object is flexible. The extra metadata in StatMeta helps detect changes beyond content.
src/denort/unsync.rs (20)
1-3: License and credits
Acknowledging the original MIT license references is compliant. Thanks for preserving licensing details.
4-17: JoinHandle<R> struct
Transparent repr with a wrapped tokio::task::JoinHandle is straightforward. Watch out for code that might rely on standard JoinHandle methods that aren’t directly exposed here.
19-28: Abort patterns
abort() and abort_handle() are symmetrical to tokio::task::JoinHandle. This is consistent with the upstream API.
30-48: Future implementation
The usage of unsafe Pin::into_inner_unchecked is carefully explained. Good job ensuring it’s only used in single-thread contexts.
50-65: spawn non-Send future
Enforcing debug_assert! with a single-thread runtime is a robust check. The approach is consistent with the official deno_unsync logic.
67-82: spawn_blocking
A thin wrapper for blocking tasks is aligned with existing tokio::task::spawn_blocking. This usage is safe.
84-98: MaskResultAsSend
Well documented and properly hidden behind disclaimers. Ensuring it’s only used in single-threaded contexts is crucial.
100-118: MaskFutureAsSend
Same pattern as above. The doc comment clarifies safety constraints.
120-138: Future implementation
Again, be sure the actual code is pinned to the current-thread runtime. The code is consistent with the concept.
140-144: License references
Retaining original references for joinset.rs. No issues.
145-155: JoinSet<T> structure
This extends JoinSet for !Send usage. Watch out for your waker usage to ensure the tasks properly wake up if you have multiple threads.
157-164: Default trait
Constructing an empty JoinSet is straightforward.
166-195: spawn a future
Again, the wake-up logic that notifies a previously-stashed waker is clever. This ensures correct behavior once the set transitions from empty.
197-218: spawn_named
A named variant of spawn. The approach is consistent with the main spawn logic.
220-223: len and is_empty
Good straightforward methods for introspective usage.
225-228: is_empty()
No issues.
230-247: poll_join_next
Stashing the waker upon Ready(None) is a neat solution. Just confirm you don’t have race conditions with multiple tasks.
249-264: join_next
A convenient async approach to poll the next completed task.
265-271: abort_all
Cancels everything. Straightforward.
273-280: detach_all
Let tasks continue in the background. Good option for gracefully ignoring minor tasks.
src/denort/lib.rs (19)
6-7: New modules
Exporting unsync and worker fosters modular growth.
26-27: Importing unsync::JoinHandle
Ensures local usage is consistent with new concurrency abstractions.
61-63: run_sync signature
Swapping import_map_url for config_file clarifies where the config is loaded. Potentially a breaking change—verify external call sites.
69-69: Spawn approach
Creating a new thread with create_and_run_current_thread_with_maybe_metrics. Solid logic if you maintain a single-thread model.
84-86: run parameter changes
Replacing import_map_url with config_file consistently. Make sure doc references in code or README have been updated.
90-104: Flags initialization
Merging unstable_config features is fine as long as usage is fully tested.
110-111: CLI factory
cli_factory.create_cli_main_worker_factory() approach is straightforward after flags are set.
126-128: test_sync signature
Same approach of substituting import_map_url with config logic. Confirm doc references.
131-131: spawn_subcommand usage
Keeping test logic in another thread helps with stack usage.
160-162: test function
Signatures updated for config usage. Ensure the derived test coverage addresses these parameters.
165-165: Permissions prompt
This is correct for test usage.
171-172: Prompt callbacks
Hiding or showing the draw thread is a nice detail for interactive prompts.
175-180: Pattern to string
Helper closure is well-structured. Just watch out for potential collisions with remote URLs.
Line range hint 183-220: test_flags & Flags
Including doc tests, concurrency, coverage_dir, etc. has thorough coverage. The approach is robust.
207-207: Merging permissions
Ensures consistent permission usage in tests.
217-218: Type check mode
Local is a good default for typical workflows.
220-220: Subcommand
DenoSubcommand::Test(...) references the test flags. No concerns.
226-227: CliFactory usage
Reusing custom extension callbacks fosters modular design.
231-231: Partial usage of test options
Ok. Just ensure the rest of the CLI is unaffected.
src/ghjk/config.rs (17)
1-2: New config system
Config struct is a neat approach to unify path references under one structure.
3-11: pub struct Config
Clear fields. The optional ghjkfile and ghjkdir with mandatory deno_dir, data_dir, etc. is well-defined.
13-18: GlobalConfigFile
Optional fields provide flexible global overrides. This is consistent with the approach.
20-25: LocalConfigFile
Flattening the GlobalConfigFile into local config is convenient for merging.
28-38: source() partial logic
Finding GHJK_DIR or .ghjk is a good fallback approach. Logging is thorough, but confirm performance if directories are large.
40-61: Locating ghjkfile
Recursive search for ghjk.ts. Good approach. Just ensure you handle symbolic links carefully (potential infinite recursion).
64-74: Overriding GHJK_DIR
If ghjkfile is found, it redefines GHJK_DIR. This is logical.
76-81: Warning if none found
Friendly user-level notice. Great.
82-103: Default config assignment
Uses fallback XDG paths. The logic is sound. Good job on ensuring the trailing slash for the URL.
105-108: GHJK_CONFIG_DIR override
Allowing override of the global config path matches user expectations.
110-142: Blocking config source
Spawning a blocking task is wise for reading large config files. The code handles layered config merges.
143-156: Ensuring .ghjk directory
Automatically creating the directory if missing helps. The .gitignore snippet is helpful.
161-186: source_global_config
Appropriate usage of config::File with optional reading. The fallback approach is robust.
188-223: source_local_config
Merges local config into existing. Overwrites fields if they exist. This layering is a standard approach.
225-260: source_env_config
Reading environment variables with prefix GHJK_. If deno_lockfile is off, sets it to None. Straightforward.
263-268: resolve_config_path
This ensures the path is absolute, which is safer for referencing user-supplied config paths.
270-286: path_from_env
Graceful handling of invalid UTF-8 and missing variables. This is thorough.
src/ghjk/ext/callbacks.rs (14)
12-12: Use of ExtConfig is appropriate.
No issues with referencing ExtConfig here.
27-30: Struct CallbackCtx design.
Storing both the MPSC receiver and termination signal watch ensures proper control flow for callback tasks. Suggest verifying error paths if the watch channel closes unexpectedly.
36-36: Optional context usage.
Allowing cx to be None is reasonable for worker-less or child thread scenarios.
46-49: Initialization of CallbackCtx.
Looks good. The termination signal from dworker effectively integrates with the Deno worker lifecycle.
55-55: Possibility of uninitialized callback line.
Function returns None if was_set is false, preventing no-op callback calls.
112-115: Doc section clarifies worker creation.
Emphasizing why None might be returned is helpful. No issues.
125-143: Spawning callback worker.
Implementation is standard for asynchronous message consumption. The loop condition gracefully exits as soon as rx.recv() returns None.
160-160: Method signature includes termination signal.
Passing mut term_signal is wise for graceful shutdown.
168-171: Early check for shutdown state.
Proactive check prevents further V8 operations if runtime is shutting down.
190-190: Use of unsafe transmute.
Rely on audience knowledge from the Deno codebase. Prefer robust commentary or references to confirm memory safety.
218-239: Promise watching logic.
Properly handles promise resolution or rejection. The result channel notifies the awaiting call.
240-243: Handling runtime shutdown for promises.
Returning an error is essential to avoid silent failures when the runtime is shutting down.
256-260: Synchronous return vs. tokio::select!.
Provides an immediate result if the callback is non-async. Fallback to waiting for either termination or promise completion is well-structured.
Also applies to: 261-266
Line range hint 304-321: op_callbacks_set properly injects callbacks.
Gracefully warns if callbacks are disabled. The usage of anyhow::bail! ensures the caller sees a short-circuited error.
src/ghjk/host.rs (19)
1-2: Terminal check import.
Using IsTerminal ensures correct interactive user prompts.
9-9: New import for hashfile::HashObj.
Indicates a shift in hashing approach or extended usage.
10-10: use hashfile::HashObj;
No immediate issues. Confirms usage of external hashing logic.
32-32: file_hash_memo type update.
Stores futures for file digests, allowing concurrency. Suggest verifying that unused futures are resolved or canceled properly.
56-57: Concurrent file reading.
Reading hash.json and lock.json concurrently is a performance improvement.
68-77: Discarding corrupt hashfile.
Properly logs the event and sets hash_obj to None. Good for avoiding partial state usage.
79-99: Lockfile corruption logic.
Prompts the user to discard if interactive, else fails. This is a robust approach to preserve data.
111-118: Collecting ghjkfile_exists and ghjkfile_hash.
Confirms existence and calculates a digest. Only used if not locked.
132-142: Staleness check for hashfile.
Discards if stale or if re_serialize is true. The debug messages could be helpful for diagnosing.
Line range hint 154-158: Method write_lockfile_or_log introduced in GhjkfileSystems.
Provides a safer approach than ignoring write errors.
Line range hint 193-203: fresh_serialized check for ghjkfile.
Only re-serializes if no valid lockfile and hashfile are found. Reduces overhead.
Line range hint 213-219: Initializing system instances.
Validates systems from config, ensuring no missing or extraneous IDs.
254-254: sys_instances: make sure usage recognizes changes.
No immediate issues.
263-267: Centralized error logging in write_lockfile_or_log.
Enhances reliability by capturing possible I/O errors.
293-299: Persisting lockfile.
Respects the locked flag, discarding changes if set. Good approach for environments requiring immutability.
310-317: Concurrent approach for writing hashfile.
Ensures a fresh serialization if needed. The direct file write is straightforward.
331-335: Hash object creation.
Creates new HashObj from the serialization result. Good step for consistency.
359-366: New LockfileError enum.
Encapsulates possible lockfile errors, improving maintainability.
371-377: Handling LockObj::from_file.
Returns None if file not found, otherwise attempts to deserialize. Proper fallback behavior.
src/ghjk/systems/deno/cli.rs (18)
1-1: Imports for CLI components.
No immediate issues with new CliCommandAction usage.
3-24: CliCommandDesc struct definition.
Supports optional fields for flexible CLI definitions. Looks good for comprehensive command descriptors.
26-28: CliCommandDesc::convert
Transforms CliCommandDesc into a SystemCliCommand. Helps unify internal representation of subcommands and flags.
32-34: Populating clap::Command from descriptor fields.
Extensive usage of optional fields to build the command. The approach is consistent and tidy.
Also applies to: 35-37, 38-40, 41-43, 44-46, 47-49, 50-52, 53-63
65-73: Handling args in CliCommandDesc.
Avoids naming clashes between flags and args. Terminates if ID conflicts.
74-84: Handling nested subcommands via recursion.
Recursively converts each child, building a hierarchical command structure.
86-99: Creating the command action.
Attaches action_cb_key as a callback. The approach is straightforward.
106-112: Returning the new SystemCliCommand.
Packaging the converted data with subcommands and an optional runtime action. No issues.
115-121: deno_cb_action function.
Efficiently extracts matches and invokes an async callback. Good usage of structural JSON for passing flags and args.
125-130: Iterating over matches.ids().
Collecting the used arguments to ensure each flag or argument is recognized.
131-137: Lookup of argument descriptions.
Panics if an unexpected argument is found. This prevents silent ignoring of undefined arguments.
138-160: Extracting argument values based on ArgActionSerde.
Works properly for string, bool, count, or vector extraction. Has a match statement for each recognized action.
165-172: Deciding whether argument is a flag or an arg.
Separates them into different buckets (flags vs. args).
173-186: Invoking callback with structured JSON.
Executes the user-provided callback key with the constructed flags and args.
188-215: CliArgDesc struct
Allows detailed specification of argument properties like required, hide, etc. This is consistent with Clap's capabilities.
217-285: Conversion from CliArgDesc to clap::Arg.
Methodically sets each property on clap::Arg, ensuring a 1:1 mapping with the descriptor's fields.
288-293: CliFlagDesc struct & its conversion.
Harnesses CliArgDesc internally, then adds custom short/long aliases. A neat composition.
Also applies to: 294-301
331-367: Enums ValueHintSerde & ArgActionSerde.
Mapping to clap equivalents is well done, though all remain as Unknown hints except in ArgActionSerde. Possibly extend them in future.
modules/envs/mod.ts (12)
3-3: Switch from cliffy_cmd to CliCommand.
Indicates a migration to the new deno_systems CLI approach.
23-23: Importing CliCommand.
No immediate issues. In-line with the rest of the new CLI logic.
38-38: Use of override on loadConfig.
Explicit overrides clarify the class hierarchy.
63-63: Implementation of override commands().
Now returns an array of CliCommand, which is consistent with the updated interface.
93-101: Shared flags for environment subcommands.
Encapsulates repeated flag definitions, promoting DRY.
103-108: Shared arguments for environment subcommands.
Parallel approach for reusability.
Line range hint 109-185: envs top-level command and subcommands.
Modular approach for listing, activating, cooking, and showing environment details. Each subcommand includes flags and arguments.
186-217: show subcommand.
Mirrors the pattern in cook and activate but with environment inspection.
Line range hint 221-249: sync subcommand.
Combines environment cooking and activation. Prevents re-initializing an already active environment.
Line range hint 304-321: reduceAndCookEnv logic.
Creates symbolic links for persistent environment usage. Takes a methodical approach.
Line range hint 354-357: Fallback logic if user declines cooking.
(Inside the commented-out code.) It's a future feature to confirm environment diffs.
Line range hint 398-405: Shell detection fallback.
If GHJK_NEXTFILE is present, writes to it; else spawns a new shell. Good user workflow.
src/ghjk/cli.rs (31)
1-1: Importing ExitCode.
Using Rust 1.61+ style exit codes simplifies clean termination paths.
3-3: Adding AnsiColor styling.
Enables colorized CLI output with Clap.
5-7: Config and interlude modules.
No issues referencing the project config components.
8-9: systems usage.
No issues. Ties CLI logic to the systems module for commands.
11-11: Unstable Deno flags array.
Centralizing them here is helpful for controlling new Deno features.
23-31: Capturing current work directory and sourcing config.
Straightforward approach. Terminate early if there's no quick CLI or if success.
33-35: Handling no ghjkdir_path case & setting up Deno.
Gracefully halts if config lacks ghjkdir. Then spawns a specialized Deno worker.
Also applies to: 37-59
62-67: Building GhjkCtx.
Combines loaded config and the newly created Deno context.
68-77: systems_from_deno call.
Loads deno-based systems from a script path. Potential source of file IO or dependency errors.
78-88: Creating HostCtx.
Reuses the config environment variables collected from std::env. Good approach.
90-95: systems_from_ghjkfile return check.
Warns if no ghjkfile found, then relies on the quick CLI error for final exit.
100-102: use clap::*;
No issues; includes all needed Clap items for dynamic subcommands.
104-112: Collecting system commands
Populates subcommands for the root Clap command from each system.
114-117: Attaching subcommands to the root
Iterates over the results of commands_from_systems.
118-126: Argument matching
Attempts to match the user's input or showing an error if matching fails.
128-148: Handling QuickComands
Executes print or automatically bails on invalid subcommands.
150-157: Resolving specific subcommand actions
Finds the correct path and action for the given matches, or errors out gracefully.
159-164: No action found scenario
Prints help and exits with a failure code, preventing silent no-ops.
166-176: Awaiting the final action & terminating contexts
Ensures we close out everything even if an error occurs.
178-210: try_quick_cli
Handles quick subcommands without a full ghjkfile load. Returns early if a recognized subcommand is used.
212-217: CLAP_STYLE
Defines a custom style for the CLI. A nice user experience improvement.
218-227: Cli struct
Root parser with a subcommand for quick commands.
229-241: QuickComands
Groups Print and Deno as subcommands.
243-257: PrintCommands
Set of specialized subcommands for printing config or fundamental paths used by ghjk.
259-301: Implementation of PrintCommands::action
Prints requested data from config or signals that we should proceed if there's no valid config.
303-310: SysCmdAction & SysCmdActions
Holds typed references to each system command and subcommand.
311-356: commands_from_systems
Generates Clap commands from each system's internal definitions. Conflicts are detected.
358-390: action_for_match
Navigates the subcommand structure. Bails on no match. Correctly returns the final action.
392-420: deno_quick_cli
Allows direct user invocation of deno subcommands. Terminates properly.
422-425: exit_with_message
Central location for error logging, ensures consistent output.
427-441: exit_for_error
Handles JavaScript errors gracefully and others with standard error codes.
src/denort/worker.rs (4)
27-40: Validate message loop exit conditions
The tokio::select! block cleanly handles messages and term signals. However, ensure that all resources (e.g., open file handles, incomplete tasks) are gracefully released before breaking from the loop. If any unhandled tasks remain queued, you may want to drain them or provide an explicit flush.
79-87: Check concurrency race conditions when calling prepare_module
This call is asynchronous and spawns further async tasks. If multiple modules are requested in parallel, ensure that shared resources in cx remain thread-safe and that your JoinSet usage won't accidentally reorder tasks that must execute sequentially.
236-258: Promote extension of DenoWorkerMsg
This enum provides a good separation of message types. Keep in mind that future message variants (e.g., for logging or advanced worker controls) should be integrated into this communication pattern.
454-475: Ensure consistency across ModuleWorkerReq
All worker behaviors—run, drive till exit, execute, get modules—should follow a common error-handling approach. Currently, each branch calls expect_or_log("channel error"). Consider unifying how you handle channel closure or partial writes to maintain consistency in error reporting.
src/deno_systems/mod.ts (1)
2-2: Confirm that importing from ./types.ts matches the new structure
Switching from ./bindings.ts to ./types.ts makes sense as long as DenoSystemsRoot is indeed declared in ./types.ts. Verify there are no references in other files that rely on the old path.
install/utils.ts (2)
14-14: Validate environment variable removal
Removing environment variables previously passed may affect user configurations. Confirm that dropping GHJK_SHARE_DIR or DENO_NO_UPDATE_CHECK is intentional and won't break existing setups relying on them.
27-27: Ensure file path quoting is secure
When interpolating Deno.execPath() into string templates, confirm you aren’t opening up shell injection vulnerabilities. The current usage is probably safe, but be mindful if paths can contain special characters.
src/ghjk/js/runtime.js (1)
11-14: Confirm usage of globalThis.____GhjkHost
You’re exposing a property from the global scope. Ensure that you’re not overwriting any existing property and that the name doesn’t conflict with potential user code or future standards.
deno.jsonc (1)
3-3: Consider running tests directly from Cargo as well.
With the new command, cargo build -p ghjk && deno test ..., you're only building with Cargo, not testing with it. If you intend to run both the Rust tests and the Deno tests, consider using cargo test alongside or instead of cargo build to avoid missing potential failures in the Rust codebase.
src/ghjk/js/00_runtime.js (1)
29-29: Watch out for backward-incompatibility when renaming global variables.
Changing globalThis.Ghjk to globalThis.____GhjkHost is a breaking change if external code references the old name. Verify that all references are updated across the codebase, documentation, and scripts to avoid runtime errors.
Also applies to: 43-43
tests/envHooks.ts (1)
63-63: Ensure $BASH_ENV is set or defaults are handled.
Using bash --rcfile $BASH_ENV -si requires $BASH_ENV to be defined. Confirm that $BASH_ENV is always set or that the tests do not fail when $BASH_ENV is missing. You may need to add fallback logic or an assertion.
scripts/dev.ts (5)
17-17: Use caution handling asynchronous directory creation.
You're making a directory with ensureDir() but not handling potential I/O errors. Consider adding basic error handling or logging for this operation to make debugging easier if the directory creation fails.
22-29: Ensure script creation handles edge cases.
You're creating an executable shell script pointing to the ghjk binary. This approach is valid; just confirm the command is executable on all target platforms. Consider adding error handling to ensure writeText completes without failures.
40-42: Confirm JSON schema alignment.
You're writing "data_dir" to config.json. Verify that downstream consumers read from this key and expect the same structure.
47-47: Pass additional install arguments if needed.
You're forwarding ghjkDataDir to install. Confirm that install no longer needs any of the old parameters (e.g., ghjkShareDir), and that this is the correct new reference for data storage.
68-68: Check file existence for fish environment sourcing.
ghjkDataDir.join("env.fish") might not exist if the script that generates it hasn't run yet, or if it's generated under different conditions. Consider verifying that it exists or handle the scenario gracefully.
src/ghjk/host/deno.rs (1)
21-24: Ensure the path to bindings.ts is valid.
You're now deriving the path using repo_root.join("files/deno/bindings.ts"). Confirm that the file structure matches this assumption, and handle the error if bindings.ts is missing or misplaced.
src/xtask/main.rs (3)
12-13: Confirm new styling import usage.
You’ve added AnsiColor; ensure it matches your styling needs and doesn't conflict with other color or style imports in the codebase.
67-72: Leverage custom CLI styling for brand consistency.
Defining CLAP_STYLE is a neat approach to unify CLI styling. Looks good.
74-78: Confirm that custom styles behave as intended on all terminals.
Applying #[clap(styles = CLAP_STYLE)] is powerful, but some terminals with poor color support might render it suboptimally. Consider fallback handling if you need broad compatibility.
modules/tasks/mod.ts (3)
14-14: Validate import path.
import { CliCommand } from "../../src/deno_systems/types.ts"; references a src/deno_systems directory. Ensure that this path remains stable if you reorganize code in the future.
50-50: Overriding a base class method for clarity.
Using override commands() clearly indicates that you're extending or replacing a base class method. Good approach for maintainability.
55-63: Define commands array.
Creating a single CliCommand[] output is simpler to maintain than a deeply chained approach. This is a clear readability improvement.
src/denort/promises.rs (2)
39-48: Good closure refactor for on_fulfilled.
Defining the function as a closure improves readability. The external data retrieval logic has been preserved effectively.
49-57: Ensure consistent error handling in on_rejected.
Everything looks fine; just verify that the function reliably captures all error cases.
src/ghjk/ext.rs (3)
37-41: Parameter addition demands usage verification.
dworker was added to callbacks_handle. Verify existing calls to ensure they pass the new parameter correctly.
48-51: Optional callbacks are well-intentioned.
Changing callbacks to Option allows for flexible usage. Ensure thorough checks for None to avoid runtime panics.
80-80: Optional callbacks field in ExtContext.
Storing callbacks as Option<Callbacks> is consistent with the usage above. No issues found.
src/ghjk/log.rs (4)
28-30: Re-sourcing platform and version references.
Switching to crate::shadow::* is straightforward. Confirm the shadow module values are set correctly to avoid inaccurate logs.
61-61: Simplifying logging environment variable.
Defaulting RUST_LOG to "info" is acceptable. Confirm there's no need for a warning level for specific modules like actix_server.
86-99: Registry layering and console subscriber integration.
Injection of console_subscriber is well-structured with the #[cfg(feature = "console-subscriber")] check. Be sure to handle the feature in Cargo.toml properly.
100-101: Initializing the registry.
Straightforward final initialization using .init(). This approach is clean and clear.
src/deno_systems/types.ts (13)
1-2: Imports and type references.
The zod library is properly imported, and local imports for GhjkCtx and ModuleBase are well-organized.
5-8: Zod schema for denoSystemsRoot.
This schema effectively enforces a record of system constructors. Looks good for typed scaffolding.
9-9: Character schema.
Enforcing a single-character string is a clever approach for short flags.
11-51: CLI argument definitions.
Comprehensive coverage of argument attributes (name, hint, environment, help, etc.). The optional flags and enumerated action types appear well-structured.
53-61: CLI flag extends CLI argument.
Logical extension of argument schema with additional short/long flag details.
63-78: CLI command base schema.
The structure for name, aliases, help, and optional args/flags is properly modular.
80-88: Flags and args aggregator.
Well-defined approach to capturing various data types for flags and args, providing a flexible structure for CLI input.
89-93: CLI action arguments.
The cliActionArgs object merges flags and args, implementing a robust foundation for CLI command callbacks.
94-98: CLI command action base.
Allowing async or sync returns for command actions covers diverse use cases. This is a strong design decision.
100-102: Binding base schema with callback keys.
The optional action_cb_key is flexible for specialized callback scenarios.
104-112: Recursive command definitions.
The use of zod.lazy() for nested subcommands effectively supports hierarchical CLI structures.
113-117: Deno system constructor type.
DenoSystemCtor aligns with the system approach. Storing system references in a record is a neat approach.
119-145: Exporting type definitions and defaults.
Exporting both input and inferred types ensures maximum flexibility for external usage. The default export bundling is well-structured.
install/mod.ts (4)
34-34: Ensure consistent usage of bash-preexec.
It’s great that you're pulling in bash-preexec.sh, but please double-check that this script behaves well in interactive and non-interactive sessions. Also confirm that any custom behaviors (logging, post-command hooks, etc.) are functioning as intended.
109-113: Default directories are well-defined.
Providing clear defaults for ghjkDataDir and ghjkConfigDir simplifies usage. Good move to align them with standard config/data directories.
130-131: Evaluate potential for race conditions in path resolution.
We are resolving paths at runtime using Deno.cwd(). If the current working directory changes between calls, it might lead to unexpected paths. Consider storing an absolute path once or clarifying any assumptions about process working directory stability.
133-137: Informative logging and placeholder substitution
The approach of substituting __GHJK_SHARE_DIR__ with the real location is clear and easy to follow. This is a clean pattern. Be sure to remove any placeholders in partial or leftover files if an error occurs mid-install.
tests/ports.ts (1)
153-153: Double-check environment availability for meta & wasmedge
Using which meta && wasmedge --version is fine, but note that which may behave differently depending on the shell or PATH configuration. Ensure that these commands exist in all relevant test environments or gracefully handle absence.
ghjk.ts (7)
14-20: Good pattern for specifying custom installation.
Defining installs.rust inside a dedicated object is a clean approach that fosters maintainability. Keep verifying that all references to Rust installation components (e.g., profile, version, rust-src) line up with your dev environment needs.
25-25: Allowed build dependencies
You've replaced direct cpy_bs calls with an updated '3.13.1' version and added installs.rust. This is beneficial if you need Rust for building or testing. Make sure to check if any other build dependencies also need to be updated.
68-68: Explicit inheritance
You’re inheriting _rust in env("dev").inherit("_rust"), so that overlapping dependencies are not re-installed. That’s efficient. However, confirm that installing ports.cargobi({ crateName: "tokio-console" }) after _rust is recognized as distinct.
85-85: Updated DENO_VERSION
Upgrading to Deno 2.1.2 is straightforward. Confirm that any new version constraints (e.g., lockfile changes or plugin compatibility) are accounted for.
99-99: Disable inheritance for cache-v8
Setting inherit: false ensures a clean environment for caching V8 builds. That’s likely safer, given the specialized nature of this task.
Line range hint 143-178: Regex-driven updates
The sedLock step for automatically updating versions in multiple files drastically reduces manual overhead. However, keep in mind that wide usage of sedLock can break things if the regex targets are too broad or if future changes in file structure cause false matches.
200-200: Turning off inheritance
Again, disabling inheritance for the lock-sed task is a wise move to avoid polluting the environment with existing dependencies.
tests/reloadHooks.ts (1)
211-212: Interactive bash without system rcfile
Specifying bash --rcfile $BASH_ENV -si is an effective way to isolate testing from system-wide or user configuration. Double-check that $BASH_ENV is set for all test environments, or fallback gracefully if it’s not.
modules/ports/mod.ts (6)
3-3: No issues found with the new import statement.
38-38: No issues found with importing CliCommand.
114-114: No issues found with marking this method as 'override'.
118-165: No major issues detected in the newly introduced commands array.
166-166: No issues detected in the return statement.
168-168: No direct code at this line; no further comments.
utils/mod.ts (1)
46-46: No issues with debug logging.
The updated parameter order still correctly logs debug information, so this change is fine.
.cargo/config.toml (1)
7-9: Document unstable feature usage rationale
While the comment indicates this is for tokio-rs/console, it would be helpful to:
- Document why the unstable feature is necessary
- Note any potential risks or limitations
- Add a link to relevant Tokio documentation
Let's verify if this unstable feature is actually used in the codebase:
✅ Verification successful
Let me gather more context about the console subscriber usage.
Let me check for the feature flag definition and any related dependencies.
Unstable feature is correctly used but needs better documentation
The tokio_unstable flag is necessary as it's being used with the console-subscriber crate (v0.4.1) for debugging and diagnostics. However, there are a few points that need attention:
- The code comment in
src/ghjk/log.rsindicates there might be an issue: "FIXME: this isn't being picked up by tokio-console" - The feature is properly gated behind an optional feature flag
console-subscriberinsrc/ghjk/Cargo.toml - The integration appears to be work-in-progress as there's a commented-out initialization line
Suggested documentation for .cargo/config.toml:
# Enable tokio-console integration for debugging and diagnostics
# Required by console-subscriber crate for runtime instrumentation
# Docs: https://docs.rs/console-subscriber/latest/console_subscriber/
# Note: This is an unstable feature and may change in future Tokio releases
"--cfg",
"tokio_unstable"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for console-related imports or uses in Rust files
rg -t rust "tokio::console" || rg -t rust "console_subscriber"
Length of output: 197
Script:
#!/bin/bash
# Get more context around the console subscriber usage
rg -t rust -B 5 -A 5 "console_subscriber"
# Check if there are any other tokio instrumentation or tracing features used
rg -t rust "tokio::instrument" || rg -t rust "#\[instrument\]"
Length of output: 1016
Script:
#!/bin/bash
# Check Cargo.toml files for console-subscriber dependency and feature definitions
rg -t toml "console-subscriber"
# Check if there are any other files referencing tokio console
rg "tokio-console" --type-not rust
Length of output: 285
src/play/Cargo.toml (3)
1-9: LGTM: Package configuration is well-structured
The package configuration follows workspace conventions and has a clear binary target definition.
14-18: Verify tracing stack configuration
The combination of tracing-related dependencies suggests a logging/debugging setup. Ensure that:
- The tracing subscriber is properly configured in the binary
- Error handling with color-eyre is consistently applied
Let's check the implementation:
✅ Verification successful
Tracing and error handling setup is properly configured
The codebase has a well-implemented tracing and error handling setup:
-
color-eyreis properly installed viacolor_eyre::install() - Tracing is configured with:
- Compact formatting
- Environment-based filter configuration
- Uptime timer
- Default to "info" level if RUST_LOG is not set
The implementation in src/xtask/utils.rs shows proper initialization of both stacks, and the main binary calls log::init() in its main function, indicating correct usage of these dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper tracing setup in main.rs
ast-grep --pattern 'tracing_subscriber::fmt::init()' || \
ast-grep --pattern 'color_eyre::install()'
Length of output: 54
Script:
#!/bin/bash
# Let's search for tracing and color-eyre setup patterns more broadly
rg -l "tracing.*init|color_eyre.*install" --type rust
# Then let's examine the content of main.rs or lib.rs files
fd -e rs "^(main|lib)\.rs$" --type f --exec cat {}
Length of output: 15798
Script:
#!/bin/bash
# Let's check the log module and utils setup
cat src/play/src/log.rs
cat src/xtask/utils.rs
Length of output: 970
20-22: Review security-critical dependency versions
The fixed versions for security-critical dependencies like data-encoding and sha2 should be regularly reviewed for security updates.
Let's check for any known vulnerabilities and latest versions:
✅ Verification successful
Let me try a different approach to check the versions using online package registries.
Let me check the versions used in the other workspace member to ensure consistency.
Security-critical dependencies are up-to-date and consistently versioned
Based on the verification:
-
data-encoding v2.6.0is the latest stable version available -
sha2 v0.10.8is the latest stable version (v0.11.0 is only pre-release) - Both packages are consistently versioned across workspace members (ghjk and play)
- The GitHub Security Advisory shows vulnerabilities only for older versions (0.9.7)
The fixed versions used in the codebase are appropriate and secure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories for data-encoding and sha2
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: RUST, package: "sha2") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
# Check latest versions
cargo search sha2 --limit 1
cargo search data-encoding --limit 1
Length of output: 854
Script:
#!/bin/bash
# Use curl to fetch package information from crates.io API
curl -s https://crates.io/api/v1/crates/sha2 | jq '.crate.max_version, .crate.newest_version'
curl -s https://crates.io/api/v1/crates/data-encoding | jq '.crate.max_version, .crate.newest_version'
# Check if these dependencies are used in other workspace members
fd -e toml | xargs grep -l "sha2\|data-encoding"
Length of output: 337
Script:
#!/bin/bash
# Check the versions used in ghjk's Cargo.toml
cat src/ghjk/Cargo.toml | grep -A 1 -B 1 "sha2\|data-encoding"
Length of output: 126
Cargo.toml (2)
31-34: Ensure clap version consistency
The warning comment correctly emphasizes version matching requirements between clap and deno. The pinned versions look correct:
- clap = "=4.5"
- clap_complete = "=4.5.24"
12-15: Verify version compatibility between deno and deno_core
The warning comment indicates version tracking requirements. Let's verify the compatibility between the specified versions:
- deno: v2.1.2-embeddable
- deno_core: 0.323.0
src/ghjk/Cargo.toml (3)
3-3: LGTM: Added package description
Good addition of a clear and concise package description.
17-26: Review dependency feature flags
Several dependencies have been modified or added. Consider:
-
indexmapwith serde feature - good for maintaining insertion order -
nixwith signal feature - ensure it's necessary for the CLI implementation
Consider auditing and potentially removing unused features to optimize build time and binary size.
✅ Verification successful
Let me check the actual usage of the nix signals to verify if it's necessary for the CLI implementation.
Dependencies and features are appropriately used
The nix dependency with the signal feature is necessary as it's being used in the CLI implementation to handle SIGPIPE signals, which is a common requirement for CLI tools to handle broken pipe scenarios (when the output pipe is closed before the program finishes writing). The indexmap with serde feature is also appropriate for maintaining insertion order in serialized data structures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of nix signal features
rg -l 'use nix::sys::signal' || rg -l 'use nix::sys::Signal'
Length of output: 47
Script:
#!/bin/bash
# Check how nix signals are used in main.rs
rg -A 5 'use nix::sys::signal' src/ghjk/main.rs
Length of output: 212
66-67: Review new feature additions
Good additions for enhanced CLI functionality:
- Config support with async features
- UI improvements with dialoguer and console
- Optional console-subscriber for debugging
Let's verify the usage of these new features:
Also applies to: 73-78
.github/workflows/nightly.yml (1)
7-7: Version consistency verified
The Deno version (2.1.2) matches the version specified in Cargo.toml (v2.1.2-embeddable).
Let's verify this version is used consistently across all workflow files:
✅ Verification successful
Deno version is consistent across workflow files
The version check confirms that DENO_VERSION: "2.1.2" is used consistently in both workflow files:
.github/workflows/tests.yml.github/workflows/nightly.yml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Deno version across all workflow files
rg "DENO_VERSION:" .github/workflows/
Length of output: 143
.github/workflows/tests.yml (4)
13-15: LGTM: Concurrency settings improve CI efficiency.
The addition of concurrency settings with cancellation of in-progress runs helps manage CI resources effectively.
18-18: LGTM: Deno version update.
The update to Deno 2.1.2 aligns with the PR objectives.
78-82: LGTM: Dependencies for Rust build.
The addition of cmake as a build dependency is necessary for building Rust crates with native dependencies.
37-37: Verify Rust toolchain configuration.
The addition of Rust toolchain setup and the change to using deno task self (which runs cargo run) aligns with the Rust rewrite. However, we should verify the Rust toolchain configuration.
Also applies to: 45-45
✅ Verification successful
Rust toolchain configuration is properly set up and aligned with the project requirements
The Rust toolchain configuration is well-defined and appropriate:
- The
rust-toolchain.tomlspecifies Rust version 1.82.0 with necessary components (rustfmt, clippy) and required WASM targets - The
.cargo/config.tomlcontains valid build configuration with tokio-specific flags and useful cargo aliases - The setup aligns with the GitHub Action using
dsherret/rust-toolchain-file@v1which will respect these configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Rust toolchain configuration
# Check for rust-toolchain.toml or rust-toolchain file
fd -t f "^rust-toolchain(\.toml)?$"
# Check for any cargo config files that might affect the build
fd -t f "^config\.toml$" .cargo/
Length of output: 111
Script:
#!/bin/bash
# Let's check the content of both configuration files
echo "=== rust-toolchain.toml ==="
cat rust-toolchain.toml
echo -e "\n=== .cargo/config.toml ==="
cat .cargo/config.toml
Length of output: 561
install/hook.fish (1)
112-112: LGTM: Added safety check for file existence.
The addition of test -e $next_env_dir/activate.fish prevents potential errors by ensuring the activation script exists before checking its modification time.
install/hook.sh (1)
112-112: LGTM: Added safety check for file existence.
The addition of [ -e "$GHJK_LAST_ENV_DIR/activate.sh" ] prevents potential errors by ensuring the activation script exists before checking its modification time. This change maintains consistency with the Fish shell implementation.
README.md (3)
3-3: LGTM! Clear and concise description.
The updated description better reflects the tool's purpose as a set of tools for managing developer environments.
64-66: LGTM! Version references are consistent.
The import statements correctly reference the new version v0.3.0-rc.1, maintaining consistency across the documentation.
154-155: LGTM! Useful test filtering addition.
The addition of test filtering support enhances developer productivity by allowing focused test execution.
install.sh (3)
5-13: LGTM! Essential command checks added.
Good addition of checks for required commands (curl and tar) with clear error messages.
37-77: LGTM! Comprehensive platform detection.
The platform detection logic is thorough and handles various operating systems and architectures.
133-174: LGTM! User-friendly shell configuration.
The shell configuration handling is well-implemented with proper user interaction and support for different shells.
src/ghjk/utils.rs (3)
132-136: LGTM! Proper Debug implementation.
The Debug implementation for CHeapStr is correctly implemented, using the underlying string's formatting.
153-154: LGTM! Good refactoring of hash functions.
The hash_str function now reuses hash_bytes, following the DRY principle.
Also applies to: 156-159
247-253: LGTM! Improved error handling.
The error handling in find_entry_recursive is now more precise, properly distinguishing between "not found" and other errors.
This PR is built on top of #107
Summary by CodeRabbit
Release Notes for Version 0.3.0-rc.1
New Features
Improvements
Bug Fixes
Documentation
Chores