Conversation
WalkthroughSubstantial refactor introducing descriptor-driven completion source construction in git.ts, establishing new config management infrastructure modules for discovery and TypeScript evaluation, updating the config manager, and centralizing snippet placeholder handling across utility functions. Changes
Sequence DiagramsequenceDiagram
participant Config as Config System
participant Discovery as Discovery
participant Eval as TS Evaluator
participant Merge as Settings Merge
participant Output as Resolved Context
Config->>Discovery: createConfigDiscovery()
Discovery-->>Config: discover(cwd, env, xdgDirs, projectRoot)
Note over Discovery: Scan .zeno, HOME, XDG dirs
Discovery-->>Config: { yamlFiles, tsFiles }
Config->>Eval: createTsConfigEvaluator(logger)
Eval-->>Config: evaluate(files, context)
Note over Eval: Load & execute each file<br/>Cache-bust via mtime
Eval-->>Config: [{ settings, warnings }, ...]
Config->>Merge: mergeSettingsList(allSettings)
Note over Merge: Aggregate & freeze<br/>settings from all sources
Merge-->>Output: Merged Settings
Config-->>Output: ResolveConfigContext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Multiple high-complexity modules (descriptor-driven git.ts refactor, config discovery scanning, TypeScript evaluation with dynamic imports), significant public API expansions, heterogeneous changes across config subsystem and snippet utilities, and intricate control flows for file discovery, config merging, and error handling warrant careful review across all affected areas. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @yuki-yano, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors core parts of the application, primarily focusing on improving the maintainability and organization of configuration management and snippet handling. By breaking down monolithic components into smaller, specialized modules, the changes enhance code clarity and reduce redundancy, particularly in how Git completion sources are defined and how dynamic snippets are processed. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the configuration management and snippet helper logic. The changes successfully centralize responsibilities into dedicated modules, which greatly improves modularity and maintainability. The introduction of reusable option builders and declarative descriptors for git completion sources is a particularly effective way to reduce code duplication. Similarly, centralizing snippet placeholder handling cleans up the snippet command implementations.
Overall, the code quality is high, and the refactoring achieves its goals effectively. I've found one minor potential issue in the new options builder logic, which could be made more robust to handle all override cases. My feedback is included as a specific comment.
| const multi = overrides.multi ?? defaults.multi; | ||
| if (multi) { | ||
| options["--multi"] = true; | ||
| } | ||
|
|
||
| const noSort = overrides.noSort ?? defaults.noSort; | ||
| if (noSort) { | ||
| options["--no-sort"] = true; | ||
| } | ||
|
|
||
| const read0 = overrides.read0 ?? defaults.read0; | ||
| if (read0) { | ||
| options["--read0"] = true; | ||
| } |
There was a problem hiding this comment.
The current logic for handling boolean options like multi, noSort, and read0 only adds them if they evaluate to true. It doesn't handle the case where an option is present in the base options and needs to be disabled via an override (e.g., multi: false). This could lead to unexpected behavior if a base configuration enables a flag that a specific source needs to disable.
To make the builder more robust, it should explicitly handle false overrides by deleting the corresponding key from the options object.
| const multi = overrides.multi ?? defaults.multi; | |
| if (multi) { | |
| options["--multi"] = true; | |
| } | |
| const noSort = overrides.noSort ?? defaults.noSort; | |
| if (noSort) { | |
| options["--no-sort"] = true; | |
| } | |
| const read0 = overrides.read0 ?? defaults.read0; | |
| if (read0) { | |
| options["--read0"] = true; | |
| } | |
| const multi = overrides.multi ?? defaults.multi; | |
| if (multi === true) { | |
| options["--multi"] = true; | |
| } else if (multi === false) { | |
| delete options["--multi"]; | |
| } | |
| const noSort = overrides.noSort ?? defaults.noSort; | |
| if (noSort === true) { | |
| options["--no-sort"] = true; | |
| } else if (noSort === false) { | |
| delete options["--no-sort"]; | |
| } | |
| const read0 = overrides.read0 ?? defaults.read0; | |
| if (read0 === true) { | |
| options["--read0"] = true; | |
| } else if (read0 === false) { | |
| delete options["--read0"]; | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const createConfigContextResolver = (): ResolveConfigContext => { | ||
| return async ({ cwd, env, homeDirectory, projectRoot }) => ({ | ||
| projectRoot, | ||
| currentDirectory: cwd, | ||
| env, | ||
| shell: detectShell(env), |
There was a problem hiding this comment.
The new ContextEnv type is declared as Readonly<Record<string, string | undefined>>, but ConfigContext.env is still a mutable Record. createConfigContextResolver now forwards the readonly env value directly in the returned ConfigContext. TypeScript treats a readonly index signature as non‑assignable to a mutable one, so this code no longer type‑checks (Readonly<Record<...>> can’t be assigned to Record<...>>). Running deno check will fail before any runtime code executes. Either return a mutable copy or widen the types so the resolver can satisfy ConfigContext.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/manager.ts (1)
138-149: Freeze TS result chunks and the merged settings to prevent cache mutation.Currently
result.settingsandmergedcan be mutated by consumers, altering the cached value across calls.- const tsResults = await tsEvaluator(discovered.tsFiles, context); - for (const result of tsResults) { - chunks.push(result.settings); - } + const tsResults = await tsEvaluator(discovered.tsFiles, context); + for (const result of tsResults) { + chunks.push(freezeSettings(result.settings)); + } @@ - const merged = settingsMerger(chunks); - cache = { source: "auto", key, settings: merged }; - return merged; + const merged = freezeSettings(settingsMerger(chunks)); + cache = { source: "auto", key, settings: merged }; + return merged;
🧹 Nitpick comments (16)
src/completion/source/git.ts (2)
46-46: Escape single quotes in prompt labels to avoid shell-quote breakage.If a label ever contains
',--promptbreaks. Escape safely.Apply this diff:
-const formatPrompt = (label: string): string => "'" + label + "> '"; +const escapeSingleQuotes = (s: string): string => + s.replaceAll("'", `'\"'\"'`); +const formatPrompt = (label: string): string => + `'${escapeSingleQuotes(label)}> '`;
78-80: Consider deep‑merging array options (--bind,--expect) instead of overwriting.Current
Object.assignoverwrites arrays. Merging preserves base binds/expect while allowing per‑descriptor additions.Example tweak:
- if (overrides.extra) { - Object.assign(options, overrides.extra); - } + if (overrides.extra) { + const merged = { ...overrides.extra }; + if (Array.isArray((targetBase as any)["--bind"]) && Array.isArray(merged["--bind"])) { + merged["--bind"] = [ ...(targetBase as any)["--bind"], ...merged["--bind"] ]; + } + if (Array.isArray((targetBase as any)["--expect"]) && Array.isArray(merged["--expect"])) { + merged["--expect"] = [ ...(targetBase as any)["--expect"], ...merged["--expect"] ]; + } + Object.assign(options, merged); + }If you prefer stronger types, we can extend
OptionOverridesto carry a typedmergebag and import the bind type. I can draft that.src/config/settings-utils.ts (2)
19-32: Merging keeps duplicates; confirm precedence or dedupe policy.Current merge concatenates arrays, potentially duplicating snippets/completions. If descriptors have identifiers, consider stable dedupe (e.g., last-one-wins) to avoid double execution or UI duplication.
Would you like a keyed-dedupe helper (e.g., by "id" or "name") added here?
34-51: Strengthen normalization with lightweight guards.normalizeSettings trusts array element types from unknown. Add minimal shape checks (filter non-objects) to reduce downstream crashes from user configs.
Example:
- const snippets = Array.isArray(maybe.snippets) - ? maybe.snippets as ReadonlyArray<Snippet> - : []; + const snippets = Array.isArray(maybe.snippets) + ? (maybe.snippets as unknown[]).filter((v) => v && typeof v === "object") as readonly Snippet[] + : []; - const completions = Array.isArray(maybe.completions) - ? maybe.completions as ReadonlyArray<UserCompletionSource> - : []; + const completions = Array.isArray(maybe.completions) + ? (maybe.completions as unknown[]).filter((v) => v && typeof v === "object") as readonly UserCompletionSource[] + : [];If you prefer stricter guards, I can add explicit validators for Snippet/UserCompletionSource. Based on coding guidelines.
src/config/project.ts (1)
4-6: Normalize starting path.Resolve cwd to avoid surprises with relative paths/symlinks.
-export const detectProjectRoot = async (cwd: string): Promise<string> => { - let current = cwd; +export const detectProjectRoot = async (cwd: string): Promise<string> => { + let current = path.resolve(cwd);src/config/context-env.ts (1)
23-41: Return an immutable ContextEnv and freeze it.Expose the stronger ContextEnv type and freeze for safety, aligning with the project’s immutability stance.
-export const collectContextEnv = ( - cwd: string, -): Record<string, string | undefined> => { +export const collectContextEnv = ( + cwd: string, +): ContextEnv => { const rawEnv = Deno.env.toObject(); - const record: Record<string, string | undefined> = {}; + const record: Record<string, string | undefined> = {}; @@ - record.PWD = cwd; - return record; + record.PWD = cwd; + return Object.freeze(record) as ContextEnv; }src/config/ts-evaluator.ts (1)
39-76: Evaluate configs in parallel (Promise.all) for speed.Sequential loads slow down multi-file setups. Parallelize while preserving per-file error handling.
- const results: EvaluateResult[] = []; - - for (const file of files) { - try { - const mod = await importModule(file) as { - default?: unknown; - }; - const configFn = mod.default; - if (typeof configFn !== "function") { - throw new Error( - "TypeScript config must export default defineConfig(() => ...)", - ); - } - const mark = Reflect.get(configFn, CONFIG_FUNCTION_MARK); - if (mark !== true) { - throw new Error( - "TypeScript config must wrap the exported function with defineConfig", - ); - } - const value = await configFn(context); - const settings = normalizeSettings(value); - results.push({ settings, warnings: [] }); - } catch (error) { - const message = `Failed to load TypeScript config ${file}: ${ - error instanceof Error ? error.message : String(error) - }`; - logger.error(message); - results.push({ - settings: freezeSettings(getDefaultSettings()), - warnings: [message], - }); - } - } - - return results; + const results = await Promise.all(files.map(async (file): Promise<EvaluateResult> => { + try { + const mod = await importModule(file) as { default?: unknown }; + const configFn = mod.default; + if (typeof configFn !== "function") { + throw new Error("TypeScript config must export default defineConfig(() => ...)"); + } + const mark = Reflect.get(configFn, CONFIG_FUNCTION_MARK); + if (mark !== true) { + throw new Error("TypeScript config must wrap the exported function with defineConfig"); + } + const value = await configFn(context); + const settings = normalizeSettings(value); + return { settings, warnings: [] }; + } catch (error) { + const message = `Failed to load TypeScript config ${file}: ${error instanceof Error ? error.message : String(error)}`; + logger.error(message); + return { + settings: freezeSettings(getDefaultSettings()), + warnings: [message], + }; + } + })); + return results as readonly EvaluateResult[];src/config/discovery.ts (3)
41-42: Route errors via injected logger; avoid direct console.error.Aligns with project guideline to route output via custom writer and matches ts-evaluator’s DI approach.
- } catch (error) { - console.error(`Failed to scan config dir ${dir}: ${error}`); + } catch (error) { + logger?.error?.(`Failed to scan config dir ${dir}: ${error}`); @@ - } catch (error) { - console.error(`Failed to load config: ${error}`); + } catch (error) { + logger?.error?.(`Failed to load config: ${error}`); @@ - } catch (error) { - console.error(`Failed to load config: ${error}`); + } catch (error) { + logger?.error?.(`Failed to load config: ${error}`); @@ -export const createConfigDiscovery = (): DiscoverConfigFiles => { - return async ({ env, xdgDirs, projectRoot }) => { +export const createConfigDiscovery = ( + logger: Pick<typeof console, "error"> = console, +): DiscoverConfigFiles => { + return async ({ env, xdgDirs, projectRoot }) => {Note: collectFromDir/findLegacyConfig will need access to logger; either pass it as a parameter or inline them inside the factory to capture logger. I can provide a follow-up diff factoring them into the factory if you prefer. Based on coding guidelines.
Also applies to: 57-58, 73-74, 81-83
118-120: Parallelize XDG scans.Speeds up discovery on systems with multiple XDG base dirs.
- for (const baseDir of xdgDirs) { - await tryCollectDir(path.join(baseDir, DEFAULT_APP_DIR)); - } + await Promise.all( + xdgDirs.map((baseDir) => tryCollectDir(path.join(baseDir, DEFAULT_APP_DIR))), + );
122-130: Return readonly arrays (freeze) to match the type.Your type advertises readonly; freeze at runtime for consistency.
- return { yamlFiles, tsFiles }; + return { + yamlFiles: Object.freeze(yamlFiles), + tsFiles: Object.freeze(tsFiles), + } as const;src/config/manager.ts (6)
28-33: Document absolute path expectations in ResolveConfigContext.Clarify (JSDoc or type alias) that
cwd,projectRoot, andhomeDirectoryare absolute, to prevent cache key drift later.
35-40: Normalize paths in cache keys to avoid false cache misses.Trailing slashes/symlinks/case (on Windows) can change
cwd,projectRoot, orhomeDirectorystring forms and bypass the cache.Apply this diff to minimally normalize (strip trailing slashes) when building the key:
@@ -const createCacheKey = (context: ConfigContext): CacheKey => ({ - cwd: context.currentDirectory, - projectRoot: context.projectRoot, - envSignature: createEnvSignature(context.env), - shell: context.shell, - homeDirectory: context.homeDirectory, -}); +const normalizeForKey = (p: string) => p.replace(/[\\/]+$/, ""); +const createCacheKey = (context: ConfigContext): CacheKey => ({ + cwd: normalizeForKey(context.currentDirectory), + projectRoot: normalizeForKey(context.projectRoot), + envSignature: createEnvSignature(context.env), + shell: context.shell, + homeDirectory: normalizeForKey(context.homeDirectory), +});If
std/pathhelpers are exposed viasrc/deps.ts, prefer those (resolve/normalize) instead of regex.Also applies to: 54-60
62-70: Make resolver output explicitly immutable (optional).Consider returning a frozen object to strengthen immutability guarantees.
- return async ({ cwd, env, homeDirectory, projectRoot }) => ({ + return async ({ cwd, env, homeDirectory, projectRoot }) => + Object.freeze({ projectRoot, currentDirectory: cwd, env, shell: detectShell(env), homeDirectory, - }); + });
112-119: Centralize env access; avoid direct Deno.env.get for HOME.Use the injected env record consistently; this improves testability and avoids permission pitfalls.
- const homeDirectory = frozenEnv["HOME"] ?? Deno.env.get("HOME") ?? ""; + const homeDirectory = frozenEnv["HOME"] ?? "";If you still need a fallback, add a
homeDirectoryProvideroption and default it usinggetEnv()instead of callingDeno.envhere.
133-142: Load YAML and evaluate TS in parallel.Reduce latency by running both concurrently.
- if (discovered.yamlFiles.length > 0) { - const yamlSettings = await loadConfigFiles(discovered.yamlFiles); - chunks.push(freezeSettings(yamlSettings)); - } - - const tsResults = await tsEvaluator(discovered.tsFiles, context); + const yamlPromise = + discovered.yamlFiles.length > 0 + ? loadConfigFiles(discovered.yamlFiles).then(freezeSettings) + : Promise.resolve<Settings | undefined>(undefined); + const tsPromise = tsEvaluator(discovered.tsFiles, context); + const [yamlSettings, tsResults] = await Promise.all([ + yamlPromise, + tsPromise, + ]); + if (yamlSettings) chunks.push(yamlSettings); for (const result of tsResults) { - chunks.push(result.settings); + chunks.push(freezeSettings(result.settings)); }
93-101: De‑dupe concurrent loads with an inflight promise.Avoid duplicate disk/eval work when multiple callers request settings simultaneously.
let cache: CacheEntry | undefined; + let inflight: Promise<Settings> | undefined; @@ - const loadSettings = async (): Promise<Settings> => { + const loadSettings = async (): Promise<Settings> => { + if (cache?.source === "manual") return cache.settings; + if (inflight) return await inflight; + inflight = (async () => { const cwd = cwdProvider(); const zenoEnv = envProvider(); @@ - const merged = settingsMerger(chunks); - cache = { source: "auto", key, settings: merged }; - return merged; - }; + const merged = freezeSettings(settingsMerger(chunks)); + cache = { source: "auto", key, settings: merged }; + return merged; + })(); + try { + return await inflight; + } finally { + inflight = undefined; + } + };Also applies to: 147-150
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/completion/source/git.ts(1 hunks)src/config/context-env.ts(1 hunks)src/config/discovery.ts(1 hunks)src/config/manager.ts(4 hunks)src/config/project.ts(1 hunks)src/config/settings-utils.ts(1 hunks)src/config/ts-evaluator.ts(1 hunks)src/snippet/auto-snippet.ts(2 hunks)src/snippet/insert-snippet.ts(2 hunks)src/snippet/next-placeholder.ts(1 hunks)src/snippet/snippet-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use async file operations throughout (avoid sync APIs such as existsSync)
Use async/await for asynchronous code instead of callback-style or then-chains
Handle errors with proper types; avoid using unknown without type guards
Prefer const over let and avoid mutations to preserve immutability
Favor pure functions without side effects where possible
Compose functionality from small functions (function composition) rather than large imperative blocks
Use higher-order functions (map, filter, reduce) instead of imperative loops where appropriate
Use type aliases instead of interface for type definitions
Use Promise.all for parallel async operations
Files:
src/config/context-env.tssrc/snippet/snippet-utils.tssrc/config/ts-evaluator.tssrc/config/discovery.tssrc/snippet/next-placeholder.tssrc/config/project.tssrc/completion/source/git.tssrc/snippet/auto-snippet.tssrc/snippet/insert-snippet.tssrc/config/settings-utils.tssrc/config/manager.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Route all text output through the project’s custom write function
Do not use classes for business logic; use functions/closures (factory pattern) instead
Pass dependencies explicitly as function parameters; avoid global state
Files:
src/config/context-env.tssrc/snippet/snippet-utils.tssrc/config/ts-evaluator.tssrc/config/discovery.tssrc/snippet/next-placeholder.tssrc/config/project.tssrc/completion/source/git.tssrc/snippet/auto-snippet.tssrc/snippet/insert-snippet.tssrc/config/settings-utils.tssrc/config/manager.ts
src/**/!(deps).ts
📄 CodeRabbit inference engine (CLAUDE.md)
Centralize external imports in implementation code via src/deps.ts (implementation files should import through deps)
Files:
src/config/context-env.tssrc/snippet/snippet-utils.tssrc/config/ts-evaluator.tssrc/config/discovery.tssrc/snippet/next-placeholder.tssrc/config/project.tssrc/completion/source/git.tssrc/snippet/auto-snippet.tssrc/snippet/insert-snippet.tssrc/config/settings-utils.tssrc/config/manager.ts
🧬 Code graph analysis (10)
src/snippet/snippet-utils.ts (1)
src/util/exec.ts (1)
executeCommand(32-55)
src/config/ts-evaluator.ts (2)
src/mod.ts (1)
CONFIG_FUNCTION_MARK(48-50)src/config/settings-utils.ts (2)
normalizeSettings(34-51)freezeSettings(11-17)
src/config/discovery.ts (2)
src/deps.ts (1)
exists(1-1)src/config/loader.ts (4)
findYamlFilesInDir(51-52)findTypeScriptFilesInDir(57-59)DEFAULT_CONFIG_FILENAME(5-5)DEFAULT_APP_DIR(6-6)
src/snippet/next-placeholder.ts (3)
src/type/shell.ts (1)
Input(1-6)src/command.ts (1)
normalizeCommand(51-57)src/snippet/snippet-utils.ts (1)
findPlaceholder(27-40)
src/config/project.ts (1)
src/mod.ts (2)
directoryExists(137-138)fileExists(130-131)
src/completion/source/git.ts (4)
src/type/fzf.ts (2)
FzfOptions(1-9)CompletionSource(16-24)src/const/option.ts (2)
DEFAULT_OPTIONS(57-60)GIT_BRANCH_LOG_TAG_REFLOG_OPTIONS(62-67)src/const/preview.ts (5)
GIT_STATUS_PREVIEW(8-13)GIT_BRANCH_LOG_TAG_REFLOG_PREVIEW(19-27)GIT_LOG_PREVIEW(17-17)GIT_STASH_PREVIEW(29-29)GIT_LS_FILES_PREVIEW(15-15)src/const/source.ts (9)
GIT_STATUS_SOURCE(19-19)GIT_STATUS_CALLBACK(20-21)GIT_BRANCH_SOURCE(28-29)GIT_BRANCH_LOG_TAG_REFLOG_CALLBACK(37-37)GIT_LS_FILES_SOURCE(23-23)GIT_STASH_SOURCE(39-40)GIT_STASH_CALLBACK(41-41)GIT_LOG_SOURCE(25-26)GIT_TAG_SOURCE(31-32)
src/snippet/auto-snippet.ts (1)
src/snippet/snippet-utils.ts (1)
extractSnippetContent(7-25)
src/snippet/insert-snippet.ts (1)
src/snippet/snippet-utils.ts (1)
extractSnippetContent(7-25)
src/config/settings-utils.ts (2)
src/completion/completion.ts (1)
completion(5-14)src/config/manager.ts (1)
mergeSettingsList(26-26)
src/config/manager.ts (1)
src/config/context-env.ts (2)
ContextEnv(3-3)detectShell(8-21)
🪛 ast-grep (0.39.6)
src/snippet/snippet-utils.ts
[warning] 4-4: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(PLACEHOLDER_PATTERN)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (10)
src/snippet/next-placeholder.ts (1)
12-20: Excellent refactoring to centralize placeholder detection.The migration to
findPlaceholdereliminates duplicate regex logic while preserving the exact same behavior. The null check and property destructuring are correct.src/snippet/auto-snippet.ts (1)
93-103: Well-executed refactoring with correct cursor positioning.The migration to
extractSnippetContentsuccessfully centralizes snippet evaluation and placeholder detection. The cursor calculation correctly usesplaceholderIndexwhen present, or falls back tosnipText.length + 1to account for the trailing space added at line 107.src/snippet/insert-snippet.ts (1)
33-43: Clean refactoring with improved readability.The migration to
extractSnippetContentand the use of nullish coalescing for cursor offset calculation (line 38) results in cleaner, more maintainable code. The cursor positioning logic correctly accounts for the trailing space added at line 42.src/snippet/snippet-utils.ts (2)
1-40: Excellent centralization of snippet placeholder logic.This new utility module successfully consolidates duplicate placeholder handling across multiple files, improving maintainability and testability. Both
extractSnippetContentandfindPlaceholderare well-designed:
- Clear separation of concerns (async evaluation vs. sync detection)
- Correct use of
replace(regex, "")to remove only the first placeholder match- Proper error handling via
executeCommand(which returns empty string on failure)- Creating fresh RegExp instances is a good defensive pattern
3-5: Static analysis ReDoS warning is a false positive.The placeholder pattern
\\{\\{[^{}\\s]*\\}\\}is safe from Regular Expression Denial of Service attacks:
- Single quantifier (
*) on a negated character class[^{}\\s]that excludes braces and whitespace- No nested quantifiers or overlapping alternations that could cause catastrophic backtracking
- The regex fails fast when it encounters disallowed characters or mismatched braces
- Backtracking depth is O(1), not exponential
The pattern is intentionally restrictive (no nested braces, no whitespace in placeholders) which actually prevents complexity issues.
src/completion/source/git.ts (2)
156-162: Builders registry is clean and type‑safe.Good use of
SourceKindto constrain the map and keep constructors cohesive.
436-438: Descriptor‑drivengitSourcesinit looks good.Simple and extensible; makes adding sources trivial.
src/config/discovery.ts (1)
10-16: Param ‘cwd’ is accepted but unused. Expected?Either use cwd in discovery (e.g., cwd/.zeno) or remove it from the type to avoid confusion.
If intended, I can wire it to check path.join(cwd, ".zeno") in addition to projectRoot.
Also applies to: 82-83
src/config/manager.ts (2)
1-24: Good modularization and deps usage.Imports split by responsibility and re-export of
mergeSettingsListlook clean and align with the project guidelines.Also applies to: 26-26
91-91: Merger behavior verified—no issues found.The verification confirms
defaultMergeSettings(an alias formergeSettingsList) preserves merge order by concatenating snippets and completions via flatMap, and the result is immutably frozen usingObject.freeze(). The code change at line 91 is correct.
| type OptionDefaults = { | ||
| preview?: string; | ||
| multi?: boolean; | ||
| noSort?: boolean; | ||
| read0?: boolean; | ||
| }; | ||
|
|
||
| type OptionOverrides = OptionDefaults & { | ||
| base?: FzfOptions; | ||
| extra?: Partial<FzfOptions>; | ||
| }; |
There was a problem hiding this comment.
Allow explicit disabling of inherited flags and clearing preview (override semantics bug).
Overridden options can’t turn off flags already present in base (e.g., --multi, --no-sort, --read0) and can’t clear an inherited --preview. This can yield incorrect fzf behavior when a descriptor wishes to opt-out.
Fix by:
- Interpreting
overrides.multi/noSort/read0 === falseas “delete corresponding flag.” - Allowing
preview: nullto remove an inherited preview.
Apply this diff:
type OptionDefaults = {
- preview?: string;
+ preview?: string | null;
multi?: boolean;
noSort?: boolean;
read0?: boolean;
};
type OptionOverrides = OptionDefaults & {
base?: FzfOptions;
extra?: Partial<FzfOptions>;
};
const createOptionsBuilder = (
base: FzfOptions,
defaults: OptionDefaults,
) => (label: string, overrides: OptionOverrides = {}): FzfOptions => {
const targetBase = overrides.base ?? base;
const options: Record<string, unknown> = {
...targetBase,
"--prompt": formatPrompt(label),
};
- const preview = overrides.preview ?? defaults.preview;
- if (preview) {
- options["--preview"] = preview;
- }
+ const preview =
+ (overrides.preview !== undefined) ? overrides.preview : defaults.preview;
+ if (preview === null) {
+ delete options["--preview"];
+ } else if (typeof preview === "string" && preview.length > 0) {
+ options["--preview"] = preview;
+ }
- const multi = overrides.multi ?? defaults.multi;
- if (multi) {
- options["--multi"] = true;
- }
+ const multi = overrides.multi;
+ if (multi === true) {
+ options["--multi"] = true;
+ } else if (multi === false) {
+ delete options["--multi"];
+ } else if (defaults.multi) {
+ options["--multi"] = true;
+ }
- const noSort = overrides.noSort ?? defaults.noSort;
- if (noSort) {
- options["--no-sort"] = true;
- }
+ const noSort = overrides.noSort;
+ if (noSort === true) {
+ options["--no-sort"] = true;
+ } else if (noSort === false) {
+ delete options["--no-sort"];
+ } else if (defaults.noSort) {
+ options["--no-sort"] = true;
+ }
- const read0 = overrides.read0 ?? defaults.read0;
- if (read0) {
- options["--read0"] = true;
- }
+ const read0 = overrides.read0;
+ if (read0 === true) {
+ options["--read0"] = true;
+ } else if (read0 === false) {
+ delete options["--read0"];
+ } else if (defaults.read0) {
+ options["--read0"] = true;
+ }
if (overrides.extra) {
Object.assign(options, overrides.extra);
}
return options as FzfOptions;
};Also applies to: 48-83
| patterns: [ | ||
| /git merge(?: .*)? $/, | ||
| ], | ||
| patterns: [/git merge(?: .*)? $/], |
There was a problem hiding this comment.
Regex anchoring is inconsistent; add “^” to avoid accidental matches.
These patterns miss the start anchor and may match mid-line text.
Apply this diff:
- patterns: [/git merge(?: .*)? $/],
+ patterns: [/^git merge(?: .*)? $/],
@@
- patterns: [
- /git stash (?:apply|drop|pop|show)(?: .*)? $/,
- /git stash branch(?=.* [^-]) .* $/,
- ],
+ patterns: [
+ /^git stash (?:apply|drop|pop|show)(?: .*)? $/,
+ /^git stash branch(?=.* [^-]) .* $/,
+ ],Also applies to: 343-345
🤖 Prompt for AI Agents
In src/completion/source/git.ts around line 331 (and also lines 343-345), the
regex patterns lack a start anchor so they can match mid-line text; update each
pattern to include a leading caret (^) so they only match at the beginning of
the line (e.g., change /git merge(?: .*)? $/ to /^git merge(?: .*)? $/ and
similarly add ^ to the other two patterns on lines 343-345), preserving existing
escapes and trailing whitespace handling.
| const gitDir = path.join(current, ".git"); | ||
| if (await directoryExists(gitDir)) { | ||
| return current; | ||
| } | ||
| const packageJson = path.join(current, "package.json"); | ||
| if (await fileExists(packageJson)) { | ||
| return current; | ||
| } |
There was a problem hiding this comment.
Support Git worktrees/submodules where .git is a file, not a dir.
Only checking directoryExists(".git") misses common cases. Treat a .git file as a project root too.
Apply:
- const gitDir = path.join(current, ".git");
- if (await directoryExists(gitDir)) {
+ const gitDir = path.join(current, ".git");
+ if (await directoryExists(gitDir) || await fileExists(gitDir)) {
return current;
}📝 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.
| const gitDir = path.join(current, ".git"); | |
| if (await directoryExists(gitDir)) { | |
| return current; | |
| } | |
| const packageJson = path.join(current, "package.json"); | |
| if (await fileExists(packageJson)) { | |
| return current; | |
| } | |
| const gitDir = path.join(current, ".git"); | |
| if (await directoryExists(gitDir) || await fileExists(gitDir)) { | |
| return current; | |
| } | |
| const packageJson = path.join(current, "package.json"); | |
| if (await fileExists(packageJson)) { | |
| return current; | |
| } |
🤖 Prompt for AI Agents
In src/config/project.ts around lines 7 to 14, the check only treats ".git" as a
directory so it misses worktrees/submodules where ".git" is a file; update the
logic to consider the path a project root if either the .git entry is a
directory OR a file (i.e., await directoryExists(gitDir) || await
fileExists(gitDir)), using the existing fileExists helper (or fs.stat if
preferred), and return current in that case while leaving the package.json check
unchanged.
| export const freezeSettings = (settings: { | ||
| snippets: readonly Snippet[]; | ||
| completions: readonly UserCompletionSource[]; | ||
| }): Settings => Object.freeze({ | ||
| snippets: settings.snippets.map(cloneAndFreezeSnippet), | ||
| completions: settings.completions.map(cloneAndFreezeCompletion), | ||
| }) as Settings; |
There was a problem hiding this comment.
Arrays are not frozen; runtime mutability leaks through.
Object.freeze on the top object doesn’t freeze nested arrays. settings.snippets/settings.completions can still be push/splice’d at runtime, breaking immutability guarantees.
Apply:
-export const freezeSettings = (settings: {
- snippets: readonly Snippet[];
- completions: readonly UserCompletionSource[];
-}): Settings => Object.freeze({
- snippets: settings.snippets.map(cloneAndFreezeSnippet),
- completions: settings.completions.map(cloneAndFreezeCompletion),
-}) as Settings;
+export const freezeSettings = (settings: {
+ snippets: readonly Snippet[];
+ completions: readonly UserCompletionSource[];
+}): Settings => {
+ const frozenSnippets = Object.freeze(
+ settings.snippets.map(cloneAndFreezeSnippet) as readonly Snippet[],
+ );
+ const frozenCompletions = Object.freeze(
+ settings.completions.map(cloneAndFreezeCompletion) as readonly UserCompletionSource[],
+ );
+ return Object.freeze({
+ snippets: frozenSnippets,
+ completions: frozenCompletions,
+ }) as Settings;
+}🤖 Prompt for AI Agents
In src/config/settings-utils.ts around lines 11–17, the top-level Object.freeze
call leaves the nested arrays mutable; freeze the arrays themselves after
mapping so callers cannot push/splice into snippets or completions. Create local
const snippets = Object.freeze(settings.snippets.map(cloneAndFreezeSnippet)) and
const completions =
Object.freeze(settings.completions.map(cloneAndFreezeCompletion)), then return
Object.freeze({ snippets, completions }) as Settings so both the arrays and
their elements are frozen.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f3153b04f883208d71795613cd6972
Summary by CodeRabbit
Release Notes