diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..bdf8ccf7a --- /dev/null +++ b/.gitattributes @@ -0,0 +1,21 @@ +# Line endings are a property of the repository, not of the checkout. +# +# Several text assets are consumed byte-for-byte rather than line-by-line: +# `internal/agents/opencode/plugin/gortex.js` and +# `internal/agents/pi/extension/index.ts` are compiled into the binary with +# go:embed and written verbatim into a user's project, and the golden files +# under `cmd/gortex/testdata/` are compared against rendered output. Git for +# Windows installs with `core.autocrlf=true` — the GitHub windows runner +# included — so without this file a Windows checkout rewrites those assets to +# CRLF, which both changes what the binary ships and breaks the comparisons. +# +# The committed blobs are already LF, so this pins the checkout; it does not +# rewrite history or content. +* text=auto eol=lf + +# PowerShell is the one place a CRLF checkout is still the convention. +*.ps1 text eol=crlf + +# Never guess at these. +*.png binary +*.gz binary diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec479a9c1..35413835b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,8 +53,16 @@ jobs: - name: Build all packages run: go build ./... + # ./internal/agents is one package and does not recurse, so the + # opencode adapter is named explicitly. Its plugin asset is embedded + # verbatim with go:embed and TestPluginFailsOpen parses it by + # splitting on "\n}\n", which is the closest thing the suite has to a + # detector for a CRLF checkout of an embedded file — and it can only + # fire on a runner that converts line endings. - name: Test Windows agent integrations - run: go test -timeout=5m -count=1 ./internal/agents + run: > + go test -timeout=5m -count=1 + ./internal/agents ./internal/agents/opencode # Symlink confinement keeps out-of-repo files out of the index, and it # reads differently on Windows: a symlink reparse point maps to diff --git a/internal/agents/claudecode/install_diet_test.go b/internal/agents/claudecode/install_diet_test.go index 8116400b4..e35e560ab 100644 --- a/internal/agents/claudecode/install_diet_test.go +++ b/internal/agents/claudecode/install_diet_test.go @@ -133,9 +133,14 @@ func TestGlobalInstall_FatToSlimReplacement(t *testing.T) { // The block is now the thin pointer: it @-includes the active // profile copy from the hermetic instructions dir… + // Two spellings of one file, and the difference is the point: the + // @-include is document content and stays '/'-spelled on every OS, + // while the ReadFile below is a real filesystem call and needs the + // native path. activePath := filepath.Join(env.InstructionsDir, profiles.ActiveFileName) - if !strings.Contains(text, "@"+activePath) { - t.Errorf("re-installed block does not @-include the active profile (%s)", activePath) + includedPath := filepath.ToSlash(activePath) + if !strings.Contains(text, "@"+includedPath) { + t.Errorf("re-installed block does not @-include the active profile (%s)", includedPath) } if !strings.Contains(text, "gortex instructions switch") { t.Error("re-installed block lost the switch verb") diff --git a/internal/agents/embedded_asset_eol_test.go b/internal/agents/embedded_asset_eol_test.go new file mode 100644 index 000000000..9e5ff33d4 --- /dev/null +++ b/internal/agents/embedded_asset_eol_test.go @@ -0,0 +1,162 @@ +package agents + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// embeddedTextAssets are the text files this tree hands to a user +// verbatim: go:embed compiles them into the binary and the adapters write +// them into a project unchanged. Their bytes are therefore a shipping +// artefact, not a source detail, and a checkout that rewrites their line +// endings changes what users receive. +var embeddedTextAssets = []string{ + "internal/agents/opencode/plugin/gortex.js", + "internal/agents/pi/extension/index.ts", +} + +// TestEmbeddedTextAssetsArePinnedToLF guards the `* text=auto eol=lf` line +// in .gitattributes from both directions. +// +// The byte half catches a checkout that already converted: Git for Windows +// installs with core.autocrlf=true, so without the attribute these files +// arrive CRLF and the binary ships CRLF. The attribute half catches the +// removal itself, and is the half that still binds on a runner configured +// not to convert — where a byte assertion would pass no matter what +// .gitattributes says. +// +// TestPluginFailsOpen detects the same corruption, but only as a side +// effect of splitting the plugin source on "\n}\n"; this states the +// contract directly and covers the pi extension too. +func TestEmbeddedTextAssetsArePinnedToLF(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + root, err := moduleRootFrom(wd) + if err != nil { + t.Fatalf("locating the module root: %v", err) + } + + // The attribute half needs the assets to belong to the checkout whose + // attributes it is about to read. A copied, vendored or nested tree + // can sit inside some *other* work tree, and asking that one about + // these paths answers a different question — or, as this test did + // before, addresses files that are not there at all. + gitRoot, gitErr := gitTopLevel(root) + attributesApply := gitErr == nil && sameDir(gitRoot, root) + + for _, rel := range embeddedTextAssets { + body, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Errorf("%s: %v", rel, err) + continue + } + if i := strings.Index(string(body), "\r\n"); i >= 0 { + t.Errorf("%s: embedded asset has a CRLF line ending at byte %d — "+ + "this checkout converted it and the binary would ship it that way; "+ + "check the `* text=auto eol=lf` line in .gitattributes", rel, i) + } + + if !attributesApply { + continue + } + out, err := exec.Command("git", "-C", root, "check-attr", "eol", "--", rel).Output() + if err != nil { + t.Errorf("%s: git check-attr: %v", rel, err) + continue + } + // `: eol: `; unset attributes report "unspecified". + line := string(out) + if got := strings.TrimSpace(line[strings.LastIndex(line, ":")+1:]); got != "lf" { + t.Errorf("%s: eol attribute is %q, want \"lf\" — a Windows checkout "+ + "will convert this embedded asset to CRLF", rel, got) + } + } + + if !attributesApply { + t.Logf("module root %s is not its own git work tree (%v); "+ + "asserted the embedded bytes only, skipped the attribute half", root, gitErr) + } +} + +// TestModuleRootFromIgnoresAnEnclosingCheckout is the regression case for +// the layout that broke this file: the source placed under some other +// repository, as a vendored copy, a monorepo subdirectory or a packaging +// staging tree. `git rev-parse --show-toplevel` answers for the enclosing +// work tree, which is the wrong tree and often does not contain these +// paths at all, so the root has to come from the module instead. +func TestModuleRootFromIgnoresAnEnclosingCheckout(t *testing.T) { + outer := t.TempDir() + inner := filepath.Join(outer, "src") + start := filepath.Join(inner, "internal", "agents") + + // An enclosing work tree *and* an enclosing module, so neither a git + // lookup nor a sloppy upward walk can accidentally pass. + for _, dir := range []string{filepath.Join(outer, ".git"), start} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + for _, f := range []string{ + filepath.Join(outer, "go.mod"), + filepath.Join(inner, "go.mod"), + } { + if err := os.WriteFile(f, []byte("module x\n"), 0o644); err != nil { + t.Fatal(err) + } + } + + got, err := moduleRootFrom(start) + if err != nil { + t.Fatalf("moduleRootFrom(%s): %v", start, err) + } + if !sameDir(got, inner) { + t.Errorf("moduleRootFrom(%s) = %s, want the nearest module root %s", start, got, inner) + } +} + +// moduleRootFrom walks up from dir to the nearest directory holding a +// go.mod. That is the module this test belongs to, whatever repository +// happens to contain it. +func moduleRootFrom(dir string) (string, error) { + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + return "", fmt.Errorf("no go.mod at or above %s", dir) + } + dir = parent + } +} + +// gitTopLevel reports the work tree dir owns, if any. +func gitTopLevel(dir string) (string, error) { + out, err := exec.Command("git", "-C", dir, "rev-parse", "--show-toplevel").Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + +// sameDir compares two directories by identity rather than by spelling. +// git answers with forward slashes on Windows while filepath produces +// backslashes, and either side may arrive through a symlink or a +// different case, so string equality is the wrong test. +func sameDir(a, b string) bool { + fa, err := os.Stat(a) + if err != nil { + return false + } + fb, err := os.Stat(b) + if err != nil { + return false + } + return os.SameFile(fa, fb) +} diff --git a/internal/agents/instructions.go b/internal/agents/instructions.go index 7ac95d667..e31b86850 100644 --- a/internal/agents/instructions.go +++ b/internal/agents/instructions.go @@ -63,7 +63,13 @@ const ( // CLAUDE.md. The heading stays here as the idempotency sentinel and as // a functional minimum for readers that do not expand @-includes. func GlobalPointerBody(instructionsDir string) string { - active := filepath.Join(instructionsDir, profiles.ActiveFileName) + // The @-include is document content, not a filesystem call: it is + // written into ~/.claude/CLAUDE.md and read back as markdown. Native + // separators would leak a `@C:\Users\me\.gortex\...` line into a file + // whose every other path is '/'-spelled, and the same rendered body + // is compared against a golden that cannot be platform-specific. This + // mirrors shellSafeHookBinary, which normalises for the same reason. + active := filepath.ToSlash(filepath.Join(instructionsDir, profiles.ActiveFileName)) return "## MANDATORY: Use Gortex MCP tools instead of Read/Grep/Glob\n\n" + "The machine-wide Gortex rules load from the active instruction profile, imported below:\n\n" + "@" + active + "\n\n" + diff --git a/internal/agents/instructions_test.go b/internal/agents/instructions_test.go index 406d9ad19..3c5e9212f 100644 --- a/internal/agents/instructions_test.go +++ b/internal/agents/instructions_test.go @@ -1,7 +1,7 @@ package agents import ( - "path/filepath" + "path" "strings" "testing" @@ -82,7 +82,11 @@ func TestBashInstructionsBodyUsesOnlyExplicitCLIMirror(t *testing.T) { func TestGlobalPointerBody_ShapeAndSentinel(t *testing.T) { const dir = "/home/user/.gortex/instructions" body := GlobalPointerBody(dir) - activePath := filepath.Join(dir, "active.md") + // path.Join, not filepath.Join: the @-include is document content and + // stays '/'-spelled on every OS. Building the expectation with + // filepath.Join would assert the native mangling on Windows and pass + // there whether or not the renderer normalises. + activePath := path.Join(dir, "active.md") if !strings.Contains(body, InstructionsSentinel) { t.Error("pointer block lost the idempotency sentinel heading") diff --git a/internal/agents/render.go b/internal/agents/render.go index a3abc3f03..d32ad6522 100644 --- a/internal/agents/render.go +++ b/internal/agents/render.go @@ -359,10 +359,26 @@ func canonicalManifestKey(key string) string { // HOME / repo root and the resolved gortex binary path — with stable // placeholders so the manifest is identical on every machine. func normalizeRender(s, home, root string) string { - s = strings.ReplaceAll(s, home, "$HOME") - s = strings.ReplaceAll(s, root, "$ROOT") + // The values to scrub are native paths — os.Executable, exec.LookPath, + // the sandbox HOME — but a rendered manifest deliberately carries + // '/'-spelled paths even on Windows: shellSafeHookBinary normalises + // unconditionally, and an @-include is document content rather than a + // filesystem call. Replacing only the native spelling therefore leaves + // a machine-specific absolute in the manifest on Windows and the + // golden drifts for every developer who has gortex installed. + // filepath.ToSlash is a no-op on POSIX, so the second pass collapses + // into the first everywhere else. + scrub := func(s, from, to string) string { + if from == "" { + return s + } + s = strings.ReplaceAll(s, from, to) + return strings.ReplaceAll(s, filepath.ToSlash(from), to) + } + s = scrub(s, home, "$HOME") + s = scrub(s, root, "$ROOT") for _, p := range gortexBinaryPaths() { - s = strings.ReplaceAll(s, p, "gortex") + s = scrub(s, p, "gortex") } return s } diff --git a/internal/agents/render_test.go b/internal/agents/render_test.go index 1dfcb391e..145b38718 100644 --- a/internal/agents/render_test.go +++ b/internal/agents/render_test.go @@ -242,3 +242,58 @@ func TestRenderRestoresUnsetEnvironment(t *testing.T) { t.Errorf("CLAUDE_CONFIG_DIR leaked out of the render as %q; it was unset before", v) } } + +// TestNormalizeRenderScrubsTheSlashSpelledPath binds the second +// replacement in normalizeRender, which nothing else can. +// +// A rendered manifest carries '/'-spelled paths even on Windows — +// shellSafeHookBinary normalises unconditionally, and an @-include is +// document content rather than a filesystem call — while the values handed +// to normalizeRender are native. Scrubbing only the native spelling +// therefore leaves a machine-specific absolute in the manifest, and +// TestAgentsRenderGolden catches it only in cmd/gortex, which the Windows +// job does not test. +// +// On linux/macos both spellings are the same string and this passes with +// either implementation; on Windows it fails without the ToSlash pass. +// That asymmetry is the point, and it is why the assertion lives in +// internal/agents, which the Windows job already runs. +func TestNormalizeRenderScrubsTheSlashSpelledPath(t *testing.T) { + home := filepath.Join(string(filepath.Separator)+"sandbox", "home") + root := filepath.Join(string(filepath.Separator)+"sandbox", "repo") + exe := filepath.Join(string(filepath.Separator)+"tools", "bin", "gortex") + + // The binary path is the third thing normalizeRender scrubs, and the + // one an adapter is most likely to have written through + // shellSafeHookBinary. Pinning gortexBinaryPaths keeps the assertion + // about the scrub rather than about whatever gortex happens to be + // installed on the machine running the test. No test in this package + // calls t.Parallel, so the swap cannot race one. + restore := gortexBinaryPaths + t.Cleanup(func() { gortexBinaryPaths = restore }) + gortexBinaryPaths = func() []string { return []string{exe} } + + // The body is what a renderer emits: forward slashes, whatever the OS. + body := "@" + filepath.ToSlash(home) + "/.gortex/instructions/active.md\n" + + "root=" + filepath.ToSlash(root) + "\n" + + `"command": "` + filepath.ToSlash(exe) + ` hook"` + "\n" + + got := normalizeRender(body, home, root) + + if !strings.Contains(got, "@$HOME/.gortex/instructions/active.md") { + t.Errorf("the slash-spelled home survived normalizeRender:\n%s", got) + } + if !strings.Contains(got, "root=$ROOT") { + t.Errorf("the slash-spelled root survived normalizeRender:\n%s", got) + } + if !strings.Contains(got, `"command": "gortex hook"`) { + t.Errorf("the slash-spelled executable survived normalizeRender:\n%s", got) + } + for _, leaked := range []string{ + filepath.ToSlash(home), filepath.ToSlash(root), filepath.ToSlash(exe), + } { + if strings.Contains(got, leaked) { + t.Errorf("machine-specific absolute %q is still in the manifest:\n%s", leaked, got) + } + } +}