internal/common/fileutil/fileutil.go (1)

48-55: IsFile semantics look good; just be aware of error/links behavior

Implementation is straightforward and correct: it returns true only for regular files and false on error. Just keep in mind this treats errors (including permission issues) the same as “not a regular file”, and follows symlinks because it uses os.Stat rather than os.Lstat. If that matches the intended use (e.g., script path checks), this is fine.

internal/runtime/builtin/command/command_builder.go (1)

21-30: Consider guarding against a nil Shell implementation from findShell

Build assumes findShell(b.Shell[0]) always returns a non-nil implementation. If that invariant is ever broken (e.g., future refactor, tests swapping the registry), shell.Build would panic.

You could harden this by returning an explicit error when findShell yields nil:

 func (b *shellCommandBuilder) Build(ctx context.Context) (*exec.Cmd, error) {
 	if len(b.Shell) == 0 {
 		return nil, fmt.Errorf("shell command is required")
 	}
 
-	shell := findShell(b.Shell[0])
-	return shell.Build(ctx, b)
+	shell := findShell(b.Shell[0])
+	if shell == nil {
+		return nil, fmt.Errorf("unknown shell: %s", b.Shell[0])
+	}
+	return shell.Build(ctx, b)
 }

internal/runtime/builtin/command/shell_cmd.go (1)

21-39: Verify working directory propagation and direct-command path

The overall branching looks good:

• When both Command and Script are set, you bypass cmd.exe and invoke the command directly with Args + Script, which matches the “direct command + script file” intent.
• Otherwise you:
  • Clone shell arguments,
  • Append step args,
  • Ensure /c or /C is present,
  • Append the normalized script path via b.normalizeScriptPath(),
  • Then execute cmd.

Two things to double‑check:

1. Working directory: exec.CommandContext is created without setting cmd.Dir here. Unless some higher layer sets Dir on the returned *exec.Cmd, the step’s working directory (b.Dir) may be ignored on Windows for cmdShell. If that’s not handled elsewhere, consider:

• return exec.CommandContext(ctx, cmd, args...), nil // nolint: gosec

• c := exec.CommandContext(ctx, cmd, args...) // nolint: gosec
• c.Dir = b.Dir
• return c, nil

and similarly in the direct‑command branch if needed.

2. **Empty script path:** If neither `b.Script` nor `b.ShellCommandArgs` is set, `b.normalizeScriptPath()` might end up empty. That would add an empty argument to `cmd`. If that’s an impossible state by construction, fine; otherwise it may be worth asserting or failing earlier.

</blockquote></details>
<details>
<summary>internal/runtime/builtin/command/command_script.go (3)</summary><blockquote>

`13-49`: **`setupScript` is robust; consider optional cleanup on failure**

The script setup flow is solid:

- Chooses an extension based on the shell (via `GetScriptExtension`),
- Uses `os.CreateTemp` in `workDir` with a clear pattern,
- Applies preprocessing, writes, syncs, and then `Chmod` to `0750`.

Two optional refinements:

- On write/sync/Chmod failures you currently leave the temp file behind. If those failure paths are likely (e.g., disk full), you might want to `os.Remove(file.Name())` before returning the error.
- The comment says “creates a temporary script file … in the given working directory”; with `workDir == ""`, `os.CreateTemp` falls back to the system temp dir, which is fine but slightly different from the comment.

Both are small polish items; the core behavior looks correct.

---

`64-72`: **Consider always cloning `args` in `createDirectCommand` for consistency**

Logic is correct, but the aliasing behavior differs by branch:

- When `scriptFile != ""`, you clone `args` before appending.
- When `scriptFile == ""`, you pass `args` directly.

For predictable behavior (callers never see their slice modified via `exec.Cmd.Args`), you could always clone:

```diff
func createDirectCommand(ctx context.Context, cmd string, args []string, scriptFile string) *exec.Cmd {
-	if scriptFile != "" {
-		clonedArgs := cloneArgs(args)
-		clonedArgs = append(clonedArgs, scriptFile)
-		return exec.CommandContext(ctx, cmd, clonedArgs...) // nolint: gosec
-	}
-	return exec.CommandContext(ctx, cmd, args...) // nolint: gosec
+	clonedArgs := cloneArgs(args)
+	if scriptFile != "" {
+		clonedArgs = append(clonedArgs, scriptFile)
+	}
+	return exec.CommandContext(ctx, cmd, clonedArgs...) // nolint: gosec
}

---

74-90: validateCommandStep logic is sound; clarify combinations if needed

The validation covers:

• Command + Script (both set),
• Command‑only,
• Script‑only,
• SubDAG‑only,

and otherwise returns core.ErrStepCommandIsRequired. That matches “at least one of Command, Script, or SubDAG must be set”.

If the domain model intends to forbid combinations like Command + SubDAG or Script + SubDAG, you might want to tighten the conditions; otherwise this is fine as‑is.

internal/runtime/builtin/command/shell_unix.go (1)

46-62: Consider adding support for additional Unix shells.

The current list covers the most common Unix shells, but consider whether tcsh or csh should be included. However, since these shells have different syntax for errexit (e.g., csh doesn't support -e), the current approach of excluding them is likely intentional and correct.

internal/runtime/builtin/command/shell_nix.go (2)

26-27: Consider making pure mode optional.

The code always enforces --pure mode, but users might want impure nix-shell environments. Consider making this conditional based on whether the user explicitly specified shell arguments.

Apply this diff to make pure mode conditional:

-	// nix-shell runs in pure mode by default
-	args = append(args, "--pure")
+	// Add pure mode if not already specified
+	if !slices.Contains(args, "--pure") && !slices.Contains(args, "--impure") {
+		args = append(args, "--pure")
+	}

---

35-47: String concatenation for command construction may introduce issues.

Building shell commands via string concatenation (lines 39-43) can be fragile with special characters or whitespace in arguments. While this works for the nix-shell use case, consider if proper shell escaping is needed.

internal/runtime/builtin/command/command_test.go (2)

1800-1814: Consider making the test assertion deterministic.

The comment "This should work since Shell is now a slice" and the conditional if err == nil suggests uncertainty about the expected behavior. For reliable test coverage, consider either:

1. Fixing the input to guarantee a specific outcome
2. Using a more deterministic test case

 	t.Run("shellCommandBuilder error with shell cmd args", func(t *testing.T) {
-		// Test the path where shellCommandBuilder fails due to empty Shell
-		// when ShellCommandArgs is set
 		config := commandConfig{
 			Ctx:              ctx,
-			Shell:            []string{"invalid shell with\ttabs"},
+			Shell:            []string{}, // Empty shell should cause error
 			ShellCommandArgs: "echo hello",
 		}
-		// This should work since Shell is now a slice
 		cmd, err := config.newCmd(ctx, "")
-		// The command might succeed or fail depending on the shell parsing
-		if err == nil {
-			assert.NotNil(t, cmd)
-		}
+		assert.Error(t, err)
+		assert.Nil(t, cmd)
 	})

---

2114-2123: Test assertion is weak and may not provide meaningful coverage.

This test attempts to write to /dev directory but the assertion if err != nil { assert.Error(t, err) } is tautological - it only asserts an error when there is already an error. Consider removing this test or making it more deterministic.

 	if _, err := os.Stat("/dev/full"); err == nil {
 		t.Run("write to full device", func(t *testing.T) {
-			// /dev/full simulates a full disk
-			_, err := setupScript("/dev", "echo hello", []string{"/bin/sh"})
-			// This might fail due to permission or other issues
-			if err != nil {
-				assert.Error(t, err)
-			}
+			t.Skip("Test is non-deterministic and depends on system permissions")
 		})
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/config/env_windows.go (1)

9-25: COMSPEC casing still inconsistent with Windows env / whitelist lookup

The whitelist still uses "COMSPEC" while Windows commonly exposes this variable as "ComSpec". With a case‑sensitive defaultWhitelist lookup, ComSpec from os.Environ() will not be whitelisted, so this base variable is still effectively filtered out.

Recommend aligning with the actual casing (e.g. replace with "ComSpec", or include both "ComSpec" and "COMSPEC" if you want to be extra defensive), or normalize casing in filterEnv before lookup.

-        "COMSPEC",     // cmd.exe
+        "ComSpec",     // cmd.exe (canonical Windows casing)

internal/runtime/builtin/command/shell_unix.go (1)

19-44: Defensively guard against an empty Shell slice in unixShell.Build

Build assumes len(b.Shell) > 0:

cmd := b.Shell[0]
args := cloneArgs(b.Shell[1:])

If shellCommandBuilder ever routes here with an empty Shell slice, this will panic. That may be impossible by construction, but it’s not enforced locally.

Consider adding a small invariant check (returning an error or similar) so a misconfigured builder can’t crash the worker:

if len(b.Shell) == 0 {
    return nil, fmt.Errorf("unixShell.Build: empty Shell slice")
}
cmd := b.Shell[0]
args := cloneArgs(b.Shell[1:])

internal/runtime/builtin/command/command.go (1)

252-305: Exit-code normalization and executor registration are reasonable

exitCodeFromError cleanly centralizes exit-code extraction from *exec.ExitError, returning 0 for nil and 1 for non‑exit errors, and it’s used in both the Start and Wait error paths so ExitCode() won’t remain 0 on failures. NewCommand/NewCommandConfig and the init registration of "", "shell", "command" executors also look coherent with the runtime/env design (Dir and Shell sourced from runtime.GetEnv(ctx) and UserSpecifiedShell derived from step.Shell != "").

If you ever expect wrapped errors around *exec.ExitError, consider switching to errors.As in exitCodeFromError for extra robustness, but it’s not strictly necessary with the current call sites.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/config/env_windows.go (2)

5-7: Relocate the init comment after imports.

The comment about init (lines 5-6) is placed before the import statement, which is unconventional. By Go convention, import statements typically follow immediately after the package declaration, with package-level comments placed after imports.

Apply this diff to improve code organization:

 package config
 
-// init populates the package's defaultWhitelist with common Windows environment
-// variable names so they are treated as whitelisted on Windows builds.
 import "strings"
 
+// init populates the package's defaultWhitelist with common Windows environment
+// variable names so they are treated as whitelisted on Windows builds.
+
 // normalizeEnvKey converts to uppercase on Windows for case-insensitive matching.

---

15-32: LGTM! Addresses the previous casing concern.

The uppercase whitelist keys combined with normalizeEnvKey in filterEnv (env.go:50) correctly resolves case-insensitive Windows environment variable matching. When os.Environ() returns variables like "ComSpec" or "COMSPEC", they are normalized to uppercase before the whitelist lookup, ensuring consistent matching.

The selected environment variables are appropriate for Windows: USERPROFILE for user home, SYSTEMROOT/WINDIR for the system directory, COMSPEC for the command interpreter, PATH/PATHEXT for executables, TEMP/TMP for temporary files, PSMODULEPATH for PowerShell, and HOME for Git/Go compatibility.

However, consider adding explicit tests for Windows-specific behavior: normalizeEnvKey case conversion and whitelist matching with case variations (e.g., verifying "ComSpec=cmd.exe" is properly filtered).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/runtime/builtin/command/shell_cmd.go (2)

26-49: Path resolution is reasonable; consider COMSPEC for extra robustness

The resolveCmdPath flow (default C:\Windows\System32\cmd.exe → SystemRoot\System32 → fall back to original) looks correct and should avoid most Go exec security restrictions around relative paths.

If you want to harden this further for non‑standard installs or customized command interpreters, consider optionally checking the COMSPEC environment variable first and validating it points to an existing cmd.exe before falling back to the hard‑coded/default locations. That would better honor user/OS configuration while keeping the same overall behavior.

---

51-74: Tighten Build arg handling and guard against empty script paths

The overall Build flow looks correct: resolve cmd.exe, preserve any shell args, ensure /c or /C is present, and append the normalized script path.

Two small maintainability/robustness tweaks you might consider:

1. Avoid cloning b.Shell[1:] when the early b.Command != "" && b.Script != "" branch returns immediately.
2. Only append the normalized script path if it’s non‑empty, so a future change that allows “no script” for cmd won’t result in cmd.exe /c "".

A refactor along these lines keeps semantics while tightening things up:

 func (s *cmdShell) Build(ctx context.Context, b *shellCommandBuilder) (*exec.Cmd, error) {
-	cmd := resolveCmdPath(b.Shell[0])
-	args := cloneArgs(b.Shell[1:])
-
-	// When running a command directly with a script, don't include cmd.exe arguments
-	if b.Command != "" && b.Script != "" {
-		args := cloneArgs(b.Args)
-		if b.Script != "" {
-			args = append(args, b.Script)
-		}
-		return exec.CommandContext(ctx, b.Command, args...), nil // nolint: gosec
-	}
-
-	args = append(args, b.Args...)
+	cmd := resolveCmdPath(b.Shell[0])
+
+	// When running a command directly with a script, don't include cmd.exe arguments
+	if b.Command != "" && b.Script != "" {
+		args := cloneArgs(b.Args)
+		args = append(args, b.Script)
+		return exec.CommandContext(ctx, b.Command, args...), nil // nolint: gosec
+	}
+
+	args := cloneArgs(b.Shell[1:])
+	args = append(args, b.Args...)

 	// cmd.exe uses /c instead of -c
 	if !slices.Contains(args, "/c") && !slices.Contains(args, "/C") {
 		args = append(args, "/c")
 	}

-	args = append(args, b.normalizeScriptPath())
+	if scriptPath := b.normalizeScriptPath(); scriptPath != "" {
+		args = append(args, scriptPath)
+	}

 	return exec.CommandContext(ctx, cmd, args...), nil // nolint: gosec
 }

This should keep behavior identical for the current flows while making the function a bit safer against future builder changes.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/runtime/builtin/command/shell_unix.go (1)

68-92: Consider adding tcsh and csh to shell detection.

The isUnixLikeShell function covers common Bourne-compatible shells but omits csh/tcsh. While these shells don't support -e the same way, the function could be renamed or documented to clarify it specifically identifies "errexit-compatible" shells.

-// isUnixLikeShell reports whether the named shell is known to support the -e (errexit) flag.
-// It returns true for "sh", "bash", "zsh", "ksh", "ash", and "dash"; returns false for "fish", an empty string, or any unrecognized shell.
-func isUnixLikeShell(shell string) bool {
+// isErrexitCompatibleShell reports whether the named shell supports the -e (errexit) flag.
+// Returns true for POSIX-compatible shells (sh, bash, zsh, ksh, ash, dash).
+// Returns false for fish, csh, tcsh, empty string, or unrecognized shells.
+func isErrexitCompatibleShell(shell string) bool {

internal/runtime/builtin/command/command_test.go (1)

1380-1420: Consider adding timeout to concurrent test.

The concurrent execution test uses sleep 0.1 which could be flaky. Consider adding a test timeout or using a more deterministic synchronization mechanism.

 func TestCommandExecutor_ConcurrentRun(t *testing.T) {
 	if goruntime.GOOS == "windows" {
 		t.Skip("Skipping Unix-specific test on Windows")
 	}
+	t.Parallel()

 	step := core.Step{
 		Name:    "test",
-		Command: "sleep",
-		Args:    []string{"0.1"},
+		Command: "true", // Use a fast command instead of sleep
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/fileutil/logutil.go (2)

40-176: Encoding decoder helper is comprehensive but could be made more data‑driven

The charset normalization and alias coverage here is extensive and well thought out. Long term, consider refactoring this giant switch into a table‑driven map (normalizedName → encoding) to reduce cognitive load and make it easier to add or tweak aliases.

-func getEncodingDecoder(charset string) *encoding.Decoder {
-    if charset == "" {
-        return nil
-    }
-
-    // Normalize the charset name for comparison
-    normalized := strings.ToLower(strings.ReplaceAll(charset, "_", "-"))
-    normalized = strings.ReplaceAll(normalized, " ", "-")
-
-    switch normalized {
-    // ...
-    }
-}
+var encodingDecoders = map[string]*encoding.Decoder{
+    // utf-8 / default: handled by nil
+    "euc-jp": japanese.EUCJP.NewDecoder(),
+    // (populate with the existing aliases and decoders)
+}
+
+func getEncodingDecoder(charset string) *encoding.Decoder {
+    if charset == "" {
+        return nil
+    }
+
+    normalized := strings.ToLower(strings.ReplaceAll(charset, "_", "-"))
+    normalized = strings.ReplaceAll(normalized, " ", "-")
+
+    if dec, ok := encodingDecoders[normalized]; ok {
+        return dec
+    }
+    // Unknown or utf-8 → nil (no decoding)
+    return nil
+}

---

538-544: Use strings.Builder or Join in ReadLogContent to avoid quadratic concatenation

Building content via repeated += on strings is O(n²) for many lines. Since you already have []string, you can construct the result more efficiently.

-func ReadLogContent(filePath string, options LogReadOptions) (string, int, int, bool, bool, error) {
-    result, err := ReadLogLines(filePath, options)
+func ReadLogContent(filePath string, options LogReadOptions) (string, int, int, bool, bool, error) {
+    result, err := ReadLogLines(filePath, options)
     if err != nil {
         return "", 0, 0, false, false, err
     }
 
-    content := ""
-    for i, line := range result.Lines {
-        content += line
-        if i < len(result.Lines)-1 {
-            content += "\n"
-        }
-    }
+    // Preserve existing behavior (newline-separated, no trailing newline)
+    content := strings.Join(result.Lines, "\n")
 
     return content, result.LineCount, result.TotalLines, result.HasMore, result.IsEstimate, nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/config/loader_windows.go (1)

1-83: Windows code-page→charset mapping looks good; consider tests and documenting fallback behavior

The Windows implementation is clear: you read the system code page once and map it through a focused switch, with a sensible UTF‑8 fallback for anything unknown. That should give good defaults for the vast majority of real-world locales.

As non-blocking follow-ups, you might:

• Add a small table-driven unit test for codePageToCharset (a few known pages plus an unknown) to prevent regressions.
• Consider a brief comment or doc note about the fallback-to-UTF‑8 behavior so it’s explicit how truly unknown/legacy code pages will be treated.

internal/common/config/loader_unix.go (1)

1-131: Unix locale→charset detection is solid; minor hardening could be added later

The Unix-side logic is well-structured: LC_ALL/LANG precedence is correct, explicit encodings are parsed and normalized sensibly, and you fall back to UTF‑8 (via inferEncodingFromLocale) in ambiguous or modern-default cases. The normalization table covers the usual suspects (UTF‑8 variants, EUC/Shift‑JIS, GBK/Big5, ISO‑8859, KOI8).

If you want a bit more robustness later (non-blocking):

• strings.TrimSpace on the initial locale/encoding values would make the parsing more tolerant of stray whitespace.
• A few focused unit tests for normalizeUnixEncoding and getDefaultLogEncodingCharset (e.g., ja_JP.UTF-8, en_US.ISO-8859-1, C, POSIX) would lock in the behavior across platforms.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/fileutil/logutil.go (2)

479-534: Consider consistent handling of very long lines in range reads

readLinesRange now correctly wraps the reader with transform.NewReader when a decoder is present, matching the other helpers. One small inconsistency:

• readFirstLines and readLastLines special‑case bufio.ErrTooLong and return a friendly "Error: Line too long to read." message.
• readLinesRange still propagates scanner.Err() directly, which will surface bufio.ErrTooLong as a raw error to callers of the range API.

If API symmetry matters for your callers, consider matching the head/tail behavior here:

-    if err := scanner.Err(); err != nil {
-        return nil, err
-    }
+    if err := scanner.Err(); err != nil {
+        if errors.Is(err, bufio.ErrTooLong) {
+            return &LogResult{Lines: []string{"Error: Line too long to read."}}, nil
+        }
+        return nil, err
+    }

---

40-176: Consider using ianaindex to reduce charset alias maintenance and improve unknown-encoding transparency

The manual alias table is comprehensive and correct, but long-term maintainability can be improved. Two points:

1. Silent fallback for unknown charsets: Treating unrecognized charset names as UTF-8 without logging creates silent data loss. For troubleshooting garbled logs, either log a warning when charset is not recognized or optionally return an error. This surfaces the problem instead of masking it.

2. Charset alias maintenance: The manual switch handles ~100+ aliases. Consider leveraging golang.org/x/text/encoding/ianaindex.MIME (or .IANA) instead. It provides case-insensitive lookup of IANA-registered charset names and returns encoding.Encoding objects compatible with your .NewDecoder() pattern. This eliminates bookkeeping and ensures consistency with IANA standards. Note: ianaindex.Encoding() returns (nil, nil) for valid but unsupported charsets (distinct from unknown names), so you'll need explicit handling for that case when adopting it.

These are non-blocking suggestions; the current implementation is functionally correct.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/fileutil/logutil.go (1)

315-319: Address the past review comment: mark zero-sample result as an estimate.

This issue was previously flagged: when totalSampleLines == 0, the function returns (1, false, nil), marking the result as exact even though it's a heuristic guess. The IsEstimate flag should be true to indicate that the line count is estimated.

Apply this diff:

 	if totalSampleLines == 0 {
 		// Avoid division by zero; assume 1 line
-		return 1, false, nil
+		return 1, true, nil
 	}

internal/common/fileutil/logutil.go (2)

172-175: Consider logging or returning an error for unknown encodings.

Returning nil for unknown encodings silently treats them as UTF-8, which may mask configuration errors or typos in encoding names. Consider logging a warning to help users detect misconfigurations.

---

12-19: golang.org/x/text v0.29.0 is safe but not the latest version.

The project uses v0.29.0, which is free from known vulnerabilities. However, v0.31.0 is the current latest release (as of December 2025). While the current version is reasonably recent and secure, consider updating to v0.31.0 if newer features or performance improvements are beneficial.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/fileutil/logutil.go (1)

67-68: GB2312 decoder mapping is still incorrect.

This critical issue was previously identified and marked as addressed, but the code still contains the incorrect mapping. GB2312 is an 8-bit encoding, but the code maps it to HZGB2312.NewDecoder(), which decodes HZ-GB-2312 (a 7-bit ASCII-safe encoding). This will cause incorrect decoding of GB2312-encoded log files.

Apply this diff to fix the mapping:

 // Simplified Chinese encodings
 case "gb2312", "gb-2312", "csgb2312":
-    return simplifiedchinese.HZGB2312.NewDecoder()
+    return simplifiedchinese.GBK.NewDecoder()

Note: GBK is a superset of GB2312 and correctly decodes GB2312-encoded text.

internal/common/config/loader_unix.go (1)

14-22: Consider checking LC_CTYPE for complete POSIX precedence.

The standard POSIX locale precedence is LC_ALL > LC_CTYPE > LANG. Since LC_CTYPE specifically controls character encoding behavior, it may be worth including it for completeness:

 func getDefaultLogEncodingCharset() string {
 	// Check LC_ALL first, then LANG (standard precedence)
 	locale := os.Getenv("LC_ALL")
 	if locale == "" {
+		locale = os.Getenv("LC_CTYPE")
+	}
+	if locale == "" {
 		locale = os.Getenv("LANG")
 	}

internal/common/fileutil/logutil.go (1)

167-170: Consider consistent BOM handling for explicitly-specified UTF-16 endianness.

When UTF-16BE is explicitly specified, the code uses IgnoreBOM, but UTF-16LE uses UseBOM. For consistency, consider using IgnoreBOM for UTF-16LE as well, since the endianness is already known:

 // Unicode variants
-case "utf-16", "utf16", "utf-16le", "utf16le":
-    return unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewDecoder()
+case "utf-16", "utf16":
+    return unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewDecoder()
+case "utf-16le", "utf16le":
+    return unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewDecoder()
 case "utf-16be", "utf16be":
     return unicode.UTF16(unicode.BigEndian, unicode.IgnoreBOM).NewDecoder()

This makes the handling symmetric: when endianness is explicit (LE or BE), ignore the BOM; when unspecified, use the BOM to determine endianness.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/common/fileutil/logutil.go (4)

24-38: Encoding option in LogReadOptions/LogResult looks reasonable; consider documenting accepted labels

The added Encoding string on LogReadOptions plus the existing IsEstimate flag on LogResult give callers enough control and feedback. To avoid confusion when users pass OS-specific names (e.g., Windows code pages), it would help to clearly document which charset labels are supported and that ""/UTF‑8 use the “no decoding” fast path.

---

365-405: Decoder plumbing through readFirstLines / readLastLines / readLinesRange is consistent; consider scanner buffer tuning

Passing a single *encoding.Decoder from ReadLogLines into all three helpers and wrapping via transform.NewReader keeps the behavior uniform for non‑UTF‑8 encodings, and error handling for bufio.ErrTooLong is preserved.

One possible follow‑up: if you expect very long log lines (especially in multibyte encodings like UTF‑16 or Shift‑JIS), you may want to raise the scanner’s max token size to avoid tripping the “Line too long to read.” path:

-	scanner := bufio.NewScanner(reader)
+	scanner := bufio.NewScanner(reader)
+	// Optionally allow longer log lines, e.g. up to 1MB.
+	scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024)

You could factor that into a small helper to keep the three functions aligned.

Also applies to: 408-478, 481-539

---

204-205: Decoder integration in ReadLogLines is straightforward; consider whether unknown encodings should be surfaced

Using getEncodingDecoder(options.Encoding) once in ReadLogLines and threading the decoder into all code paths (head, tail, range, default range) is clean and minimizes repeated charset lookups. The only behavioral question is whether silently treating unknown encodings as UTF‑8 (via nil decoder) is what you want operationally; if not, this is the natural place to fail fast once getEncodingDecoder starts returning an error.

Also applies to: 215-216, 225-226, 239-240, 252-253

---

549-550: ReadLogContent join is fine; verify trailing newline semantics vs previous behavior

Switching to strings.Join(result.Lines, "\n") is clearer and avoids manual string concatenation. It does, however, fix the exact placement of newlines to “between lines only”. If the old implementation happened to append a trailing newline after the last line, this would be a subtle behavior change. Please double‑check any callers/tests that might rely on a final newline when returning full‑file content.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro