Merge pull request #2538 from dgageot/board/extracting-runtime-components-as-builtin-b822c9ff

feat(hooks): add 6 builtin hooks + widen post_tool_use / before_llm_call contract

Author: dgageot

pkg/hooks/builtins/add_directory_listing.go

@@ -0,0 +1,79 @@
+package builtins
+
+import (
+	"context"
+	"fmt"
+	"log/slog"
+	"os"
+	"sort"
+	"strings"
+
+	"github.com/docker/docker-agent/pkg/hooks"
+)
+
+// AddDirectoryListing is the registered name of the add_directory_listing builtin.
+const AddDirectoryListing = "add_directory_listing"
+
+// maxDirectoryEntries caps how many entries from the cwd top level are
+// included in the listing. A very large project root would otherwise
+// dominate every prompt. Hidden entries (dot-files) are skipped before
+// the cap is applied so the visible budget isn't burned on
+// configuration noise.
+const maxDirectoryEntries = 100
+
+// addDirectoryListing emits a one-shot listing of the working
+// directory's top-level entries as session_start additional context.
+//
+// Hidden entries (names starting with ".") are skipped; the rest are
+// sorted alphabetically with a trailing "/" on directories. Output is
+// capped at [maxDirectoryEntries] with a "... and N more" line when
+// truncated. Read errors are logged at debug and produce a nil Output
+// rather than aborting session start.
+//
+// No-op when Input.Cwd is empty or the listing has no visible entries.
+func addDirectoryListing(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) {
+	if in == nil || in.Cwd == "" {
+		return nil, nil
+	}
+
+	entries, err := os.ReadDir(in.Cwd)
+	if err != nil {
+		slog.Debug("add_directory_listing: read dir failed; skipping", "cwd", in.Cwd, "error", err)
+		return nil, nil
+	}
+
+	var names []string
+	for _, e := range entries {
+		name := e.Name()
+		if strings.HasPrefix(name, ".") {
+			continue
+		}
+		if e.IsDir() {
+			name += "/"
+		}
+		names = append(names, name)
+	}
+	if len(names) == 0 {
+		return nil, nil
+	}
+	sort.Strings(names)
+
+	truncated := 0
+	if len(names) > maxDirectoryEntries {
+		truncated = len(names) - maxDirectoryEntries
+		names = names[:maxDirectoryEntries]
+	}
+
+	var b strings.Builder
+	fmt.Fprintf(&b, "Top-level entries in %s:\n\n", in.Cwd)
+	for _, n := range names {
+		b.WriteString("- ")
+		b.WriteString(n)
+		b.WriteByte('\n')
+	}
+	if truncated > 0 {
+		fmt.Fprintf(&b, "... and %d more\n", truncated)
+	}
+
+	return hooks.NewAdditionalContextOutput(hooks.EventSessionStart, strings.TrimRight(b.String(), "\n")), nil
+}

pkg/hooks/builtins/add_directory_listing_test.go

@@ -0,0 +1,107 @@
+package builtins_test
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/hooks/builtins"
+)
+
+// TestAddDirectoryListingIncludesFilesAndDirs verifies the happy path:
+// regular files appear by name, directories get a trailing "/", and
+// dot-files are excluded. We deliberately don't pin the header line
+// format — the test is robust to header tweaks.
+func TestAddDirectoryListingIncludesFilesAndDirs(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	writeFile(t, dir, "README.md", "")
+	writeFile(t, dir, "main.go", "")
+	writeFile(t, dir, ".env", "secret") // hidden — must be skipped
+	require.NoError(t, os.Mkdir(filepath.Join(dir, "subdir"), 0o755))
+	require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) // hidden dir — skipped
+
+	fn := lookup(t, builtins.AddDirectoryListing)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: dir}, nil)
+	require.NoError(t, err)
+	require.NotNil(t, out)
+	require.NotNil(t, out.HookSpecificOutput)
+	assert.Equal(t, hooks.EventSessionStart, out.HookSpecificOutput.HookEventName,
+		"add_directory_listing must target session_start, not turn_start")
+
+	ctx := out.HookSpecificOutput.AdditionalContext
+	assert.Contains(t, ctx, "README.md")
+	assert.Contains(t, ctx, "main.go")
+	assert.Contains(t, ctx, "subdir/", "directories must be marked with a trailing slash")
+	assert.NotContains(t, ctx, ".env", "hidden files must be skipped")
+	assert.NotContains(t, ctx, ".git", "hidden directories must be skipped")
+}
+
+// TestAddDirectoryListingTruncatesLongList pins the cap behavior: with
+// more than maxDirectoryEntries (100) visible entries, the output ends
+// with a truncation summary. We assert on the "... and N more" line
+// without locking in the exact format of every entry.
+func TestAddDirectoryListingTruncatesLongList(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	for i := range 150 {
+		writeFile(t, dir, fmt.Sprintf("file_%03d.txt", i), "")
+	}
+
+	fn := lookup(t, builtins.AddDirectoryListing)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: dir}, nil)
+	require.NoError(t, err)
+	require.NotNil(t, out)
+	assert.Contains(t, out.HookSpecificOutput.AdditionalContext, "and 50 more",
+		"truncation summary must report the dropped count")
+}
+
+// TestAddDirectoryListingEmptyOrNoCwdIsNoop documents the two no-op
+// paths: an empty Cwd (the safety check) and a Cwd whose only entries
+// are hidden (no visible content to report) both produce nil rather
+// than an empty stanza.
+func TestAddDirectoryListingEmptyOrNoCwdIsNoop(t *testing.T) {
+	t.Parallel()
+
+	fn := lookup(t, builtins.AddDirectoryListing)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+
+	out, err = fn(t.Context(), nil, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+
+	hiddenOnly := t.TempDir()
+	writeFile(t, hiddenOnly, ".hidden", "")
+	out, err = fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: hiddenOnly}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out, "directory with only hidden entries must yield no output")
+}
+
+// TestAddDirectoryListingUnreadableDirIsNoop documents the graceful
+// failure path: a Cwd that doesn't exist (or can't be read) makes the
+// builtin skip rather than abort the session. ReadDir's error is
+// logged at debug level.
+func TestAddDirectoryListingUnreadableDirIsNoop(t *testing.T) {
+	t.Parallel()
+
+	fn := lookup(t, builtins.AddDirectoryListing)
+
+	out, err := fn(t.Context(), &hooks.Input{
+		SessionID: "s",
+		Cwd:       filepath.Join(t.TempDir(), "does-not-exist"),
+	}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+}

pkg/hooks/builtins/add_git_diff.go

@@ -0,0 +1,47 @@
+package builtins
+
+import (
+	"context"
+	"log/slog"
+
+	"github.com/docker/docker-agent/pkg/hooks"
+)
+
+// AddGitDiff is the registered name of the add_git_diff builtin.
+const AddGitDiff = "add_git_diff"
+
+// addGitDiff emits the working-tree diff as turn_start additional
+// context, refreshing every turn.
+//
+// By default it runs `git diff --stat` for a compact summary. Pass the
+// single arg "full" to emit the full unified diff (`git diff`) instead.
+// In either mode the captured output is capped by [maxGitOutputBytes],
+// so a runaway change set can't silently blow up the prompt.
+//
+// No-op when:
+//   - Input.Cwd is empty;
+//   - the directory isn't a git repo (git exits non-zero);
+//   - git isn't installed;
+//   - the diff is empty (clean tree).
+func addGitDiff(ctx context.Context, in *hooks.Input, args []string) (*hooks.Output, error) {
+	if in == nil || in.Cwd == "" {
+		return nil, nil
+	}
+
+	gitArgs := []string{"diff", "--stat"}
+	header := "Current working-tree diff (stat):\n\n"
+	if len(args) > 0 && args[0] == "full" {
+		gitArgs = []string{"diff"}
+		header = "Current working-tree diff:\n\n"
+	}
+
+	out, err := gitOutput(ctx, in.Cwd, gitArgs...)
+	if err != nil {
+		slog.Debug("add_git_diff: git diff failed; skipping", "cwd", in.Cwd, "error", err)
+		return nil, nil
+	}
+	if out == "" {
+		return nil, nil
+	}
+	return hooks.NewAdditionalContextOutput(hooks.EventTurnStart, header+out), nil
+}

pkg/hooks/builtins/add_git_diff_test.go

@@ -0,0 +1,101 @@
+package builtins_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/hooks/builtins"
+)
+
+// TestAddGitDiffStatModeShowsTouchedFile verifies the default mode
+// (`git diff --stat`): after staging an initial commit and modifying a
+// tracked file, the diff output must mention the file. We anchor the
+// assertion on the filename rather than the stat formatting so the
+// test is stable across git versions.
+func TestAddGitDiffStatModeShowsTouchedFile(t *testing.T) {
+	t.Parallel()
+
+	dir := initGitRepo(t)
+	writeFile(t, dir, "README.md", "first")
+	runGit(t, dir, "add", "README.md")
+	runGit(t, dir, "commit", "--quiet", "-m", "init")
+	// Modify the tracked file so `git diff` (working-tree vs HEAD)
+	// has something to report.
+	writeFile(t, dir, "README.md", "second")
+
+	fn := lookup(t, builtins.AddGitDiff)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: dir}, nil)
+	require.NoError(t, err)
+	require.NotNil(t, out)
+	require.NotNil(t, out.HookSpecificOutput)
+	assert.Equal(t, hooks.EventTurnStart, out.HookSpecificOutput.HookEventName,
+		"add_git_diff must target turn_start, not session_start")
+	assert.Contains(t, out.HookSpecificOutput.AdditionalContext, "README.md")
+	assert.Contains(t, out.HookSpecificOutput.AdditionalContext, "stat",
+		"default mode must announce itself as the stat view")
+}
+
+// TestAddGitDiffFullModeShowsHunks pins the args contract: passing the
+// single arg "full" switches from `--stat` to a full unified diff. We
+// assert on the presence of a "+second" line, which only appears in
+// the unified output, not in --stat.
+func TestAddGitDiffFullModeShowsHunks(t *testing.T) {
+	t.Parallel()
+
+	dir := initGitRepo(t)
+	writeFile(t, dir, "README.md", "first\n")
+	runGit(t, dir, "add", "README.md")
+	runGit(t, dir, "commit", "--quiet", "-m", "init")
+	writeFile(t, dir, "README.md", "second\n")
+
+	fn := lookup(t, builtins.AddGitDiff)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: dir}, []string{"full"})
+	require.NoError(t, err)
+	require.NotNil(t, out)
+	assert.Contains(t, out.HookSpecificOutput.AdditionalContext, "+second",
+		"full mode must include the unified-diff hunk lines")
+}
+
+// TestAddGitDiffCleanTreeIsNoop documents that a clean working tree
+// produces a nil Output rather than an empty stanza. The model should
+// not see "Current working-tree diff (stat):\n" with nothing under it.
+func TestAddGitDiffCleanTreeIsNoop(t *testing.T) {
+	t.Parallel()
+
+	dir := initGitRepo(t)
+	writeFile(t, dir, "README.md", "first")
+	runGit(t, dir, "add", "README.md")
+	runGit(t, dir, "commit", "--quiet", "-m", "init")
+
+	fn := lookup(t, builtins.AddGitDiff)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: dir}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out, "clean tree must yield no additional context")
+}
+
+// TestAddGitDiffNoCwdOrNotARepoIsNoop documents the safety / graceful
+// failure paths: empty Cwd, nil input, and a non-repo directory all
+// produce nil rather than aborting the session.
+func TestAddGitDiffNoCwdOrNotARepoIsNoop(t *testing.T) {
+	t.Parallel()
+
+	fn := lookup(t, builtins.AddGitDiff)
+
+	out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+
+	out, err = fn(t.Context(), nil, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+
+	out, err = fn(t.Context(), &hooks.Input{SessionID: "s", Cwd: t.TempDir()}, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+}

pkg/hooks/builtins/add_git_status.go

@@ -0,0 +1,38 @@
+package builtins
+
+import (
+	"context"
+	"log/slog"
+
+	"github.com/docker/docker-agent/pkg/hooks"
+)
+
+// AddGitStatus is the registered name of the add_git_status builtin.
+const AddGitStatus = "add_git_status"
+
+// addGitStatus emits `git status --short --branch` as turn_start
+// additional context, refreshing the model's view of uncommitted
+// changes every turn.
+//
+// No-op when:
+//   - Input.Cwd is empty;
+//   - the directory isn't a git repo (git exits non-zero);
+//   - git isn't installed.
+//
+// Failures are logged at debug and surfaced as a nil Output so an
+// agent configured to receive git status doesn't get the run aborted
+// because the user happened to `cd` outside a repo.
+func addGitStatus(ctx context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) {
+	if in == nil || in.Cwd == "" {
+		return nil, nil
+	}
+	out, err := gitOutput(ctx, in.Cwd, "status", "--short", "--branch")
+	if err != nil {
+		slog.Debug("add_git_status: git status failed; skipping", "cwd", in.Cwd, "error", err)
+		return nil, nil
+	}
+	if out == "" {
+		return nil, nil
+	}
+	return hooks.NewAdditionalContextOutput(hooks.EventTurnStart, "Current git status:\n\n"+out), nil
+}