Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions pkg/board/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"strings"
"sync"

"github.com/docker/docker-agent/pkg/paths"
"github.com/docker/docker-agent/pkg/userconfig"
)

Expand Down Expand Up @@ -146,7 +145,7 @@ func (a *App) Projects() []Project {
}
projects := make([]Project, 0, len(a.config.Board.Projects))
for _, p := range a.config.Board.Projects {
projects = append(projects, Project{Name: p.Name, Path: p.Path, Agent: p.Agent})
projects = append(projects, Project{Name: p.Name, Path: expandHome(p.Path), Agent: p.Agent})
}
return projects
}
Expand Down Expand Up @@ -178,8 +177,10 @@ func (a *App) AddProject(p Project) error {
}
}
a.config.Board.Projects = append(a.config.Board.Projects, userconfig.BoardProject{
Name: p.Name,
Path: p.Path,
Name: p.Name,
// Stored ~-contracted so the shared config works across
// environments whose home differs (host vs. docker sandbox).
Path: contractHome(p.Path),
Agent: p.Agent,
})
return a.saveConfigLocked()
Expand All @@ -193,10 +194,7 @@ func normalizeProjectPath(path string) (string, error) {
if path == "" {
return "", errors.New("project path is required")
}
if path == "~" || strings.HasPrefix(path, "~/") {
path = filepath.Join(paths.GetHomeDir(), strings.TrimPrefix(path[1:], "/"))
}
abs, err := filepath.Abs(path)
abs, err := filepath.Abs(expandHome(path))
if err != nil {
return "", fmt.Errorf("resolve project path: %w", err)
}
Expand Down Expand Up @@ -237,6 +235,13 @@ func (a *App) CreateCard(project Project, prompt string) (card *Card, err error)
agent = DefaultAgent
}

// Checked before launch because tmux silently falls back to $HOME when
// its start directory is missing, which would surface as a confusing
// worktree error from the agent instead of this one.
if !isGitRepo(a.ctx, project.Path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[MEDIUM] isGitRepo silently passes when project.Path is empty, running git in the process's cwd

CreateCard calls isGitRepo(a.ctx, project.Path) without first validating that project.Path is non-empty. In Go's os/exec, setting cmd.Dir = "" causes the subprocess to inherit the parent's working directory — so if the board process happens to be running inside a git repository, isGitRepo("") returns true, card creation succeeds, and a card with no valid path is committed to state.

An empty project.Path can arise if expandHome is called on a "" stored value (it returns "" unchanged), or if a caller constructs a Project struct without setting Path.

The companion AddProject already guards against this via normalizeProjectPath, which rejects empty paths explicitly. CreateCard needs a matching guard:

if strings.TrimSpace(project.Path) == "" {
    return nil, errors.New("project path is required")
}

This guard should be placed before the isGitRepo call (line 241).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[LOW] Context cancellation or missing git binary produces misleading "not a git repository" error

isGitRepo discards the underlying exec error and returns only a bool. If a.ctx is cancelled (e.g. during app shutdown) or git is not on PATH, the function returns false, and CreateCard reports:

"project path X is not a git repository in this environment; re-add the project here"

…even though the real cause is an execution failure, not an invalid path. This message guides the user to remove and re-add the project, which won't help.

In practice a.ctx is only cancelled at shutdown and git is a hard dependency of this tool, so this is a low-impact UX edge case. Still, distinguishing execution errors from "path is not a repo" would make the error actionable. One option: have isGitRepo return (bool, error) and check errors.Is(err, context.Canceled) before emitting the path-specific message.

return nil, fmt.Errorf("project path %s is not a git repository in this environment; re-add the project here", project.Path)
}

worktreeName := newWorktreeName()
branch := worktreeBranch(worktreeName)
wtPath := worktreeDir(worktreeName)
Expand Down
7 changes: 5 additions & 2 deletions pkg/board/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,14 @@ func removeWorktree(ctx context.Context, repoPath, worktreePath, branch string)
_ = cmd.Run()
}

// isGitRepo reports whether path is inside a git working tree.
// isGitRepo reports whether path is inside a git working tree. The output
// is checked, not just the exit code: inside a bare repo or a .git dir the
// command succeeds but prints "false".
func isGitRepo(ctx context.Context, path string) bool {
cmd := exec.CommandContext(ctx, "git", "rev-parse", "--is-inside-work-tree")
cmd.Dir = path
return cmd.Run() == nil
out, err := cmd.Output()
return err == nil && strings.TrimSpace(string(out)) == "true"
}

// worktreeDiff returns the full diff of all changes in the worktree relative
Expand Down
46 changes: 46 additions & 0 deletions pkg/board/paths.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package board

import (
"path/filepath"
"strings"

"github.com/docker/docker-agent/pkg/paths"
)

// The board's project list and card state live in files shared across
// environments with different home directories — e.g. a host and a docker
// sandbox that mirrors the host's home-relative mounts (~/src, ~/.cagent).
// Paths under home are therefore persisted as "~/…" and expanded against
// the current home on load, so the same state works in both.

// contractHome replaces a current-home prefix with "~", using forward
// slashes so the stored form is platform-neutral.
func contractHome(path string) string {
home := paths.GetHomeDir()
if home == "" || path == "" {
return path
}
if path == home {
return "~"
}
if rel, ok := strings.CutPrefix(path, home+string(filepath.Separator)); ok {
return "~/" + filepath.ToSlash(rel)
}
return path
}

// expandHome resolves a leading "~" against the current home directory.
// Other paths — and "~" paths when the home cannot be determined — are
// returned unchanged, so a failed lookup can never turn a stored path into
// a relative one (which a later save would persist, corrupting the shared
// state).
func expandHome(path string) string {
if path == "~" || strings.HasPrefix(path, "~/") {
home := paths.GetHomeDir()
if home == "" {
return path
}
return filepath.Join(home, strings.TrimPrefix(path[1:], "/"))
}
return path
}
55 changes: 55 additions & 0 deletions pkg/board/paths_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package board

import (
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"

"github.com/docker/docker-agent/pkg/paths"
)

func TestContractHome(t *testing.T) {
t.Parallel()

home := paths.GetHomeDir()

assert.Equal(t, "~/src/repo", contractHome(filepath.Join(home, "src", "repo")))
assert.Equal(t, "~", contractHome(home))
assert.Empty(t, contractHome(""))
// A path outside home is stored as-is.
assert.Equal(t, filepath.FromSlash("/opt/repo"), contractHome(filepath.FromSlash("/opt/repo")))
// A sibling of home must not lose its prefix.
assert.Equal(t, home+"-other", contractHome(home+"-other"))
}

func TestExpandHome(t *testing.T) {
t.Parallel()

home := paths.GetHomeDir()

assert.Equal(t, filepath.Join(home, "src", "repo"), expandHome("~/src/repo"))
assert.Equal(t, home, expandHome("~"))
assert.Empty(t, expandHome(""))
assert.Equal(t, filepath.FromSlash("/opt/repo"), expandHome(filepath.FromSlash("/opt/repo")))
// Only a leading ~/ is expanded, not ~ elsewhere or ~user.
assert.Equal(t, "~user/src", expandHome("~user/src"))
}

// TestExpandHomeNoHome proves a failed home lookup leaves "~" paths
// untouched instead of turning them into relative paths that a later save
// would persist.
func TestExpandHomeNoHome(t *testing.T) {
t.Setenv("HOME", "")
t.Setenv("USERPROFILE", "")

assert.Equal(t, "~/src/repo", expandHome("~/src/repo"))
assert.Equal(t, "~", expandHome("~"))
}

func TestContractExpandRoundTrip(t *testing.T) {
t.Parallel()

p := filepath.Join(paths.GetHomeDir(), ".cagent", "worktrees", "board-abc")
assert.Equal(t, p, expandHome(contractHome(p)))
}
20 changes: 18 additions & 2 deletions pkg/board/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,31 @@ func OpenStore(path string) (*Store, error) {
if err := json.Unmarshal(data, &s.cards); err != nil {
return nil, fmt.Errorf("parse board state %s: %w", path, err)
}
// Drop null entries (hand-edited or corrupted file) rather than panic;
// they carry no data worth preserving.
s.cards = slices.DeleteFunc(s.cards, func(c *Card) bool { return c == nil })
for _, c := range s.cards {
c.RepoPath = expandHome(c.RepoPath)
c.Worktree = expandHome(c.Worktree)
}
return s, nil
}

// save persists the cards. Callers must hold s.mu.
// save persists the cards. Callers must hold s.mu. Paths under the current
// home are written ~-contracted so the state file stays valid across
// environments whose home differs (host vs. docker sandbox).
func (s *Store) save() error {
if err := os.MkdirAll(filepath.Dir(s.path), 0o750); err != nil {
return err
}
data, err := json.MarshalIndent(s.cards, "", " ")
cards := make([]*Card, len(s.cards))
for i, c := range s.cards {
clone := *c
clone.RepoPath = contractHome(clone.RepoPath)
clone.Worktree = contractHome(clone.Worktree)
cards[i] = &clone
}
data, err := json.MarshalIndent(cards, "", " ")
if err != nil {
return err
}
Expand Down
50 changes: 50 additions & 0 deletions pkg/board/store_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package board

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/docker/docker-agent/pkg/paths"
)

func testStore(t *testing.T) *Store {
Expand Down Expand Up @@ -117,3 +120,50 @@ func TestStoreDeleteCard(t *testing.T) {
// Deleting a missing card is a no-op.
require.NoError(t, s.DeleteCard("c1"))
}

// TestOpenStoreSkipsNullCards proves a hand-edited or corrupted state file
// containing null entries loads instead of panicking.
func TestOpenStoreSkipsNullCards(t *testing.T) {
t.Parallel()

path := filepath.Join(t.TempDir(), "cards.json")
require.NoError(t, os.WriteFile(path, []byte(`[null, {"id": "c1", "column": "dev"}]`), 0o644))

s, err := OpenStore(path)
require.NoError(t, err)
cards := s.ListCards()
require.Len(t, cards, 1)
assert.Equal(t, "c1", cards[0].ID)
}

// TestStorePortablePaths proves card paths under home are persisted
// ~-contracted (so shared state files work across environments with
// different home directories) and expanded again on load.
func TestStorePortablePaths(t *testing.T) {
t.Parallel()

path := filepath.Join(t.TempDir(), "cards.json")
s, err := OpenStore(path)
require.NoError(t, err)

home := paths.GetHomeDir()
card := &Card{
ID: "c1",
Column: "dev",
RepoPath: filepath.Join(home, "src", "repo"),
Worktree: filepath.Join(home, ".cagent", "worktrees", "board-abc"),
}
require.NoError(t, s.InsertCard(card))

data, err := os.ReadFile(path)
require.NoError(t, err)
assert.Contains(t, string(data), `"~/src/repo"`)
assert.Contains(t, string(data), `"~/.cagent/worktrees/board-abc"`)

s2, err := OpenStore(path)
require.NoError(t, err)
got, err := s2.GetCard("c1")
require.NoError(t, err)
assert.Equal(t, card.RepoPath, got.RepoPath)
assert.Equal(t, card.Worktree, got.Worktree)
}
Loading