Skip to content

Port core runtime paths and shell execution to support native Windows builds#1

Merged
giveen merged 7 commits into
copilot/examine-codebase-compilation-issuesfrom
copilot/port-late-to-windows
Apr 21, 2026
Merged

Port core runtime paths and shell execution to support native Windows builds#1
giveen merged 7 commits into
copilot/examine-codebase-compilation-issuesfrom
copilot/port-late-to-windows

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 21, 2026

This PR ports the project’s core OS-sensitive runtime behavior toward native Windows support by removing hardcoded Unix shell assumptions and centralizing platform-aware path resolution. It introduces build-tagged execution paths and aligns config/session storage behavior with OS conventions.

  • Execution abstraction (no hardcoded Unix shell)

    • Replaces direct bash -c invocation in BashTool with a platform-specific command builder.
    • Adds build-tagged implementations:
      • internal/tool/shell_command_windows.gocmd.exe /C
      • internal/tool/shell_command_unix.gobash with cached fallback to sh
    • Keeps process spawning explicit and platform-resolved rather than Linux-assumed.
  • Centralized path handling

    • Adds internal/common/path_utils.go to define cross-platform path conventions in one place.
    • Introduces shared helpers for:
      • config root (LateConfigDir)
      • session storage (LateSessionDir)
      • MCP config locations (project-local and user-level)
  • Adoption in runtime config/session loaders

    • Refactors path construction in:
      • internal/config/config.go
      • internal/session/models.go
      • internal/mcp/config.go
    • Removes duplicated, Linux-specific directory assumptions from these call sites.
  • Build-constraint structure for future OS-specific behavior

    • Establishes *_windows.go / *_unix.go split where behavior cannot be cleanly unified, starting with command execution.
    • Creates a clear extension point for additional Windows-specific adaptations.

Example of the shell abstraction now used by command execution:

//go:build windows
func newShellCommand(ctx context.Context, command string) *exec.Cmd {
	return exec.CommandContext(ctx, "cmd.exe", "/C", command)
}

//go:build !windows
func newShellCommand(ctx context.Context, command string) *exec.Cmd {
	return exec.CommandContext(ctx, getUnixShellPath(), "-c", command)
}

Copilot AI and others added 7 commits April 21, 2026 00:39
Agent-Logs-Url: https://github.com/giveen/late/sessions/37f198b0-316d-4095-967b-b1545d7b069b

Co-authored-by: giveen <1180939+giveen@users.noreply.github.com>
@giveen giveen marked this pull request as ready for review April 21, 2026 00:46
Copilot AI review requested due to automatic review settings April 21, 2026 00:46
@giveen giveen merged commit d2882cd into copilot/examine-codebase-compilation-issues Apr 21, 2026
1 check passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports OS-sensitive runtime behavior toward native Windows support by introducing platform-aware shell execution and centralizing config/session path resolution.

Changes:

  • Added build-tagged shell command construction (cmd.exe /C on Windows; bash with sh fallback on non-Windows).
  • Centralized app config/session/MCP config path building in internal/common/path_utils.go.
  • Refactored config/session/MCP loaders to use the shared path helpers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/tool/shell_command_windows.go Adds Windows-specific shell command builder using cmd.exe /C.
internal/tool/shell_command_unix.go Adds non-Windows shell command builder with cached bashsh fallback.
internal/tool/implementations.go Switches BashTool execution to the platform-specific shell wrapper.
internal/common/path_utils.go Introduces centralized helpers for config, session, and MCP config paths.
internal/config/config.go Loads config via common.LateConfigDir() instead of manual path building.
internal/session/models.go Resolves session storage via common.LateSessionDir().
internal/mcp/config.go Uses shared helpers for project/user MCP config discovery and defaults.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +19 to +24
// Use UserConfigDir on Windows to keep all app state under AppData.
configDir, err := LateConfigDir()
if err != nil {
return "", err
}
return filepath.Join(configDir, "sessions"), nil
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Windows branch, the local variable is named configDir but LateConfigDir() already appends the late subdirectory, so it’s actually the app-specific config dir. Renaming it (e.g., lateConfigDir) or adjusting the comment would avoid confusion when extending these helpers.

Suggested change
// Use UserConfigDir on Windows to keep all app state under AppData.
configDir, err := LateConfigDir()
if err != nil {
return "", err
}
return filepath.Join(configDir, "sessions"), nil
// Use the app-specific config dir on Windows to keep all app state under AppData.
lateConfigDir, err := LateConfigDir()
if err != nil {
return "", err
}
return filepath.Join(lateConfigDir, "sessions"), nil

Copilot uses AI. Check for mistakes.
Comment on lines +449 to 451
// Execute command using a platform-specific shell wrapper.
cmd := newShellCommand(ctx, params.Command)
cmd.Dir = params.Cwd
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BashTool now executes via newShellCommand (which uses cmd.exe on Windows or sh fallback on Unix), but the tool’s user-facing naming/messaging still assumes “bash” (e.g., tool description and some error text). This can be confusing on Windows and when sh is selected; consider making the description/errors shell-agnostic (or explicitly mention the platform-specific shell).

Copilot uses AI. Check for mistakes.
Comment on lines +449 to 451
// Execute command using a platform-specific shell wrapper.
cmd := newShellCommand(ctx, params.Command)
cmd.Dir = params.Cwd
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a new OS-specific execution path (newShellCommand) but there’s no direct unit test asserting the constructed command/args per platform (Windows: cmd.exe /C, Unix: <shell> -c). Adding a small test that inspects the returned *exec.Cmd would help prevent regressions without needing to actually run a shell.

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +32
func LateConfigDir() (string, error) {
configDir, err := os.UserConfigDir()
if err != nil {
return "", err
}
return filepath.Join(configDir, "late"), nil
}

func LateSessionDir() (string, error) {
if runtime.GOOS == "windows" {
// Use UserConfigDir on Windows to keep all app state under AppData.
configDir, err := LateConfigDir()
if err != nil {
return "", err
}
return filepath.Join(configDir, "sessions"), nil
}

homeDir, err := os.UserHomeDir()
if err != nil {
return "", err
}
return filepath.Join(homeDir, ".local", "share", "late", "sessions"), nil
}
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LateConfigDir/LateSessionDir introduce new cross-platform path semantics (notably the Windows branch). Given internal/common already has unit tests, it would be valuable to add tests for these helpers (e.g., validate directory suffixes and behavior under t.Setenv for config/home vars where supported) to lock in the intended conventions.

Copilot uses AI. Check for mistakes.
@giveen giveen deleted the copilot/port-late-to-windows branch April 25, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants