diff --git a/CHANGELOG.md b/CHANGELOG.md index 46d3649..fb6047f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,55 @@ ## [Unreleased] +## [0.3.0-preview.2] — 2026-09-21 + +**A preview**, on the same terms as the one before it: a GitHub pre-release, so +`godo -e update` does not offer it and neither Homebrew nor Scoop carries it. + +All of it was found by running `v0.3.0-preview.1` on a real Windows host, which +is what the preview was for. The roadmap said Windows shell detection was +written from documentation and unverified; it was also wrong, and so were two +other things. + +Still unverified, and the reason this is preview.2 rather than 0.3.0: the +junction syscall runs only on Windows. Its reparse buffer is built in portable +code and tested field by field, but `DeviceIoControl` itself has been executed +by nobody. + +### Fixed +- **Every PowerShell user on Windows was told they were in `cmd`.** Detection + read the parent process, and something is usually in between: scoop installs + godo as `shims\godo.exe`, which starts the real `godo.exe` as a child, so the + parent of the process asking the question was *godo*. Not a shell, so the + answer fell through to `%ComSpec%`. It now walks up the process tree to the + nearest shell, which also covers npm, bun and make wrappers and an editor's + terminal. +- **A catalog written on Windows lost its decorators.** With CRLF the YAML + parser files the comment above a key as the *previous* key's foot comment, so + `# @dialect matcher` decorated nothing — unless a blank line happened to sit + above it, which is why it read as "no space between the command and the + comment breaks it". Line endings are normalized before parsing, which YAML + already calls the same line break. It also keeps a `\r` out of a block + scalar, where it was being handed to the shell as part of the command. +- **`fs.slink` did nothing on Windows.** It called `os.Symlink`, and a symlink + there is a privilege rather than a file operation, so an ordinary user got + "A required privilege is not held by the client". It now uses the link + Windows actually offers — a **junction** for a directory, a **hard link** for + a file — neither of which needs a privilege. The op is the same op; the + mechanism is the platform's. + +### Added +- A decorator YAML puts somewhere godo does not read is now an error naming + where it belongs, instead of being dropped: `ins: x # @deps y` files the + comment on the value, and a decorator after the last key files as that key's + foot comment. Prose that merely mentions `@deps` is still a comment. + +### Changed +- `godo -e runners` says **how** the default shell was chosen — `your $SHELL`, + `found in the process tree`, `set by GODO_SHELL`, `%ComSpec%, because no + shell is in the process tree`. A wrong answer is only diagnosable if you can + tell a stale `GODO_SHELL` from a fallback, and a path alone cannot. + ## [0.3.0-preview.1] — 2026-09-20 **A preview.** It is a GitHub pre-release, so `godo -e update` does not offer diff --git a/docs/contract.md b/docs/contract.md index 61679db..48720a7 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -195,16 +195,16 @@ Selection, in order: |--|--| | `GODO_SHELL` | Always wins | | Unix | `$SHELL`, else `/bin/sh` | -| Windows | the parent process when it is a shell, else `%ComSpec%` | +| Windows | the nearest shell in the process tree, else `%ComSpec%` | -godo answers "which shell am I in" from the parent process, which is the only -thing that knows. No command run *inside* a shell can report it — it would only +godo answers "which shell am I in" by walking up the process tree, which is +the only thing that knows. No command run *inside* a shell can report it — it would only describe the shell godo just started. To confirm it yourself, in your own terminal: `echo $0` (sh, bash, zsh, dash, ksh), `echo $version` (fish), `$PSVersionTable.PSVersion` (PowerShell), `echo %COMSPEC%` (cmd). `godo -e runners` prints these too. -Windows reads the parent process because the environment cannot answer: +Windows reads the process tree because the environment cannot answer: PowerShell sets `PSModulePath` and everything it starts inherits it, so a `cmd.exe` opened from PowerShell would look like PowerShell. On Unix, `$SHELL` is the login shell rather than the one running right now — bash started inside @@ -330,6 +330,21 @@ that runner's bodies are not expanded either way. YAML comment block **immediately above** the script key. Apply to scripts only. +Above the key is the only placement that works, and godo refuses the two that +look like they should: + +```yaml + ins: bun install # @dialect matcher <- error + # @deps ins <- error, if nothing follows it +``` + +YAML files a trailing comment on the value and a comment after the last key on +that key, neither of which decorates anything. Silently dropping them meant a +catalog ran with a dialect or a dependency list its author believed they had +written, and the symptom arrived far away — a script matching nothing, a +dependency that never ran. A comment that merely mentions `@deps` in prose is +still a comment; only one that *starts* with a decorator is refused. + | Line | | |------|--| | `# text` (no `@`) | Doc | diff --git a/docs/dev/plugin-protocol.md b/docs/dev/plugin-protocol.md index 78c72cc..68ec9b0 100644 --- a/docs/dev/plugin-protocol.md +++ b/docs/dev/plugin-protocol.md @@ -111,6 +111,13 @@ unbuffered, so a plugin that spoke first would deadlock. {"code":0,"ok":true,"stdout":"","stderr":"","error":""} ``` +`slink` makes the link the platform offers: a **symlink** on Unix, and on +Windows a **junction** for a directory or a **hard link** for a file. Windows +treats a symlink as a privilege rather than a file operation, so asking for one +fails for an ordinary user; a junction and a hard link do not. Consequence: on +Windows the target must exist, because which link to make depends on what it +is, and a hard link cannot cross volumes. + `error` is godo refusing — an unknown op, or a capability the catalog did not grant. A command that ran and failed is `code`, not `error`. diff --git a/docs/guide/runners.md b/docs/guide/runners.md index 3056711..efd811e 100644 --- a/docs/guide/runners.md +++ b/docs/guide/runners.md @@ -83,7 +83,8 @@ A shell that is not installed fails when the plan is built, so nothing runs. ``` $ godo -e runners -inherit /bin/zsh [default] +inherit /bin/zsh + the default runner, your $SHELL shells found here: sh /bin/sh @@ -103,12 +104,18 @@ Selection order: |--|--| | `GODO_SHELL` | Always wins | | Unix | `$SHELL`, else `/bin/sh` | -| Windows | the parent process when it is a shell, else `%ComSpec%` | +| Windows | the nearest shell in the process tree, else `%ComSpec%` | -Windows reads the parent process because the environment cannot answer: +Windows reads the process tree because the environment cannot answer: PowerShell sets `PSModulePath` and everything it starts inherits it, so a `cmd.exe` opened from PowerShell would look like PowerShell. +It walks the tree rather than reading the parent alone because something is +usually in between. A package manager's shim is the common one: scoop installs +godo as `shims\godo.exe`, which starts the real `godo.exe` as a child — so the +parent of the process asking the question is *godo*. The same shape appears +with npm, bun and make wrappers, and inside an editor's terminal. + On Unix, `$SHELL` is your *login* shell, not necessarily the one running right now — bash started inside zsh still reports zsh. Every other tool follows that convention; `GODO_SHELL` is how you disagree with it. diff --git a/docs/install.md b/docs/install.md index 9b91bad..48e0ad2 100644 --- a/docs/install.md +++ b/docs/install.md @@ -41,10 +41,10 @@ A preview is published as a GitHub **pre-release**. `godo -e update` reads the arrives on its own — you go and get it: ```bash -go install github.com/my-rv/godo/cmd/godo@v0.3.0-preview.1 +go install github.com/my-rv/godo/cmd/godo@v0.3.0-preview.2 ``` -or download the `v0.3.0-preview.1` assets from +or download the `v0.3.0-preview.2` assets from [Releases](https://github.com/MY-RV/godo/releases). On a preview binary `godo -e update check` reports no update available: the diff --git a/docs/roadmap.md b/docs/roadmap.md index 923bb45..13dd41a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -16,7 +16,7 @@ This is what we **commit to communicate**. Pre-1.0 APIs can still change within **Not promised in v0.1:** Homebrew/Scoop installs, dialects `nscript`/`matchns`, stable Go API. -## v0.3 — previewed as `v0.3.0-preview.1` +## v0.3 — previewed as `v0.3.0-preview.2` Breaking: the shell that runs your scripts changes, and file fields move into `engine:`. It was held until plugin loading worked — `engine:` without a loader @@ -25,7 +25,7 @@ is half a promise, and this is the release where the promise gets made. A preview is a GitHub pre-release: `godo -e update` does not offer it, and neither Homebrew nor Scoop carries it. It is on [Releases](https://github.com/MY-RV/godo/releases) and on -`go install github.com/my-rv/godo/cmd/godo@v0.3.0-preview.1`. +`go install github.com/my-rv/godo/cmd/godo@v0.3.0-preview.2`. | Promise | | |---------|--| @@ -46,7 +46,9 @@ Everything in the table above. ### Before v0.3.0 ships - Preview feedback. Plugin loading is new, and the preview is where it gets - found out. + found out. preview.1 on Windows already produced three fixes. +- **Windows verification.** preview.2 fixes shell detection, CRLF catalogs and + `fs.slink`; the junction syscall behind `slink` has still been run by nobody. - The plugin protocol is **not** frozen by this preview. A plugin is pinned by digest, so a protocol change cannot silently break a catalog — it fails by naming the plugin. diff --git a/go.mod b/go.mod index 1354bf4..5b5ad64 100644 --- a/go.mod +++ b/go.mod @@ -2,9 +2,8 @@ module github.com/my-rv/godo go 1.26.0 -require gopkg.in/yaml.v3 v3.0.1 - require ( - github.com/tetratelabs/wazero v1.12.0 // indirect - golang.org/x/sys v0.44.0 // indirect + github.com/tetratelabs/wazero v1.12.0 + golang.org/x/sys v0.44.0 + gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index d385a7e..42e511f 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,3 @@ -github.com/tetratelabs/wazero v1.9.0 h1:IcZ56OuxrtaEz8UYNRHBrUa9bYeX9oVY93KspZZBf/I= -github.com/tetratelabs/wazero v1.9.0/go.mod h1:TSbcXCfFP0L2FGkRPxHphadXPjo1T6W+CseNNY7EkjM= github.com/tetratelabs/wazero v1.12.0 h1:DuWcpNu/FzgEXgGBDp8J1Spc+CWOvvtvVyjKlaZopYU= github.com/tetratelabs/wazero v1.12.0/go.mod h1:LvKtzl2RqO4gyF27BiXU+nKAjcV8f38U+kP/q2vgxh0= golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= diff --git a/internal/catalog/decorator_test.go b/internal/catalog/decorator_test.go new file mode 100644 index 0000000..4b52994 --- /dev/null +++ b/internal/catalog/decorator_test.go @@ -0,0 +1,102 @@ +package catalog_test + +import ( + "strings" + "testing" + + "github.com/my-rv/godo/internal/catalog" +) + +// A decorator at the end of the line is the failure a Windows user hit on the +// 0.3.0 preview: YAML files it as a line comment, godo read nobody's comment, +// and the catalog ran with a dialect its author believed they had written. +func TestDecorator_trailingOnTheLineIsAnError(t *testing.T) { + dir := t.TempDir() + path := writeCat(t, dir, "version: \"0.1\"\nscripts:\n ins: bun install # @dialect matcher\n b: echo b\n") + + _, err := catalog.LoadFile(path) + if err == nil { + t.Fatal("want an error, got a catalog that quietly ignored the decorator") + } + for _, want := range []string{"@dialect", "not read", "own line above the key"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not say %q", err, want) + } + } +} + +// After the last key there is no next key to decorate, so YAML files it as +// that key's foot comment. Silently dropped before. +func TestDecorator_belowTheLastKeyIsAnError(t *testing.T) { + dir := t.TempDir() + path := writeCat(t, dir, "version: \"0.1\"\nscripts:\n a: echo a\n # @deps a\n") + + _, err := catalog.LoadFile(path) + if err == nil || !strings.Contains(err.Error(), "@deps") { + t.Fatalf("err=%v", err) + } +} + +// The check must not turn ordinary comments into errors. +func TestDecorator_proseMentioningOneIsStillAComment(t *testing.T) { + dir := t.TempDir() + path := writeCat(t, dir, "version: \"0.1\"\nscripts:\n a: echo a # like @deps but not\n b: echo b\n") + + if _, err := catalog.LoadFile(path); err != nil { + t.Fatalf("a comment that merely names a decorator is a comment: %v", err) + } +} + +// The placement that works has to keep working, blank line or not. +func TestDecorator_aboveTheKeyIsRead(t *testing.T) { + for name, doc := range map[string]string{ + "no blank line": "version: \"0.1\"\nscripts:\n a: echo a\n # @dialect matcher\n ${x}: echo ${godo:argv[x]}\n", + "blank line": "version: \"0.1\"\nscripts:\n a: echo a\n\n # @dialect matcher\n ${x}: echo ${godo:argv[x]}\n", + } { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + cat, err := catalog.LoadFile(writeCat(t, dir, doc)) + if err != nil { + t.Fatal(err) + } + if cat.Scripts[1].Dialect != "matcher" { + t.Fatalf("dialect=%q, want matcher", cat.Scripts[1].Dialect) + } + }) + } +} + +// A catalog written on Windows has CRLF, and the YAML parser files comments +// differently for it: the decorator directly above its key became the previous +// key's foot comment and decorated nothing. A blank line above the comment +// happened to hide it, which is why the report was "no space between the +// command and the comment breaks it". +func TestDecorator_crlfCatalogReadsTheDecorator(t *testing.T) { + doc := "version: \"0.1\"\nscripts:\n a: echo a\n # @dialect matcher\n ${x}: echo ${godo:argv[x]}\n" + dir := t.TempDir() + path := writeCat(t, dir, strings.ReplaceAll(doc, "\n", "\r\n")) + + cat, err := catalog.LoadFile(path) + if err != nil { + t.Fatalf("a CRLF catalog is a catalog: %v", err) + } + if cat.Scripts[1].Dialect != "matcher" { + t.Fatalf("dialect=%q, want matcher", cat.Scripts[1].Dialect) + } +} + +// The same normalization keeps \r out of a block scalar, where it would reach +// the shell as part of the command. +func TestDecorator_crlfDoesNotLeakIntoABody(t *testing.T) { + doc := "version: \"0.1\"\nscripts:\n a: |\n echo one\n echo two\n" + dir := t.TempDir() + path := writeCat(t, dir, strings.ReplaceAll(doc, "\n", "\r\n")) + + cat, err := catalog.LoadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(cat.Scripts[0].Commands[0], "\r") { + t.Fatalf("body carries a carriage return: %q", cat.Scripts[0].Commands[0]) + } +} diff --git a/internal/catalog/load.go b/internal/catalog/load.go index 030d871..21cd8ea 100644 --- a/internal/catalog/load.go +++ b/internal/catalog/load.go @@ -1,6 +1,7 @@ package catalog import ( + "bytes" "fmt" "os" "path/filepath" @@ -28,7 +29,7 @@ func Parse(data []byte, path string, dialects ...*DialectRegistry) (*Catalog, er } var root yaml.Node - if err := yaml.Unmarshal(data, &root); err != nil { + if err := yaml.Unmarshal(normalizeBreaks(data), &root); err != nil { return nil, fmt.Errorf("%w: parse yaml: %v", ErrInvalidCatalog, err) } doc := &root @@ -113,8 +114,29 @@ func Parse(data []byte, path string, dialects ...*DialectRegistry) (*Catalog, er return cat, nil } +// normalizeBreaks turns CRLF into LF before the YAML parser sees it. +// +// A catalog written on Windows has CRLF, and the parser files a comment +// differently for it: a decorator directly above its key became the *previous* +// key's foot comment instead of that key's head comment, so it decorated +// nothing. A blank line above the comment happened to hide it, which is why it +// read as "no space between the command and the comment breaks it". +// +// YAML treats CRLF as a line break, so normalizing is what the format already +// says. It also keeps a \r out of block scalars, where it would otherwise be +// handed to the shell as part of the command. +func normalizeBreaks(data []byte) []byte { + if !bytes.Contains(data, []byte("\r\n")) { + return data + } + return bytes.ReplaceAll(data, []byte("\r\n"), []byte("\n")) +} + func scriptFromNodes(key, val *yaml.Node, fileDialect DialectName, reg *DialectRegistry, path string) (Script, error) { s := Script{Key: key.Value} + if err := rejectMisplacedDecorators(key, val); err != nil { + return Script{}, fmt.Errorf("%w: script %q: %v", ErrInvalidCatalog, s.Key, err) + } dec, err := parseDecorators(key.HeadComment) if err != nil { return Script{}, fmt.Errorf("%w: script %q: %v", ErrInvalidCatalog, s.Key, err) @@ -183,6 +205,58 @@ func validateMatcherKey(key string) error { return nil } +// decoratorNames are the @-words parseDecorators answers to. +var decoratorNames = []string{"@deps", "@dependencies", "@dialect", "@runner"} + +// rejectMisplacedDecorators fails on a decorator YAML puts somewhere godo does +// not read. +// +// Only the comment block *above* a key decorates it. A decorator written at +// the end of the line lands on the value as a line comment, and one written +// after the last key lands on that key as a foot comment; both were read by +// nobody and dropped in silence. The catalog then ran with a dialect or a +// dependency list its author believed they had written, which surfaces far +// from the line that caused it — a script matching nothing, or a dependency +// that never runs. +// +// So it is an error, and the error says where the decorator belongs. A comment +// only trips this if it starts with a decorator godo knows: prose mentioning +// "@deps" is a comment, and stays one. +func rejectMisplacedDecorators(key, val *yaml.Node) error { + places := []struct { + comment string + where string + }{ + {key.LineComment, "at the end of the line"}, + {val.LineComment, "at the end of the line"}, + {key.FootComment, "below the key"}, + {val.FootComment, "below the key"}, + } + for _, p := range places { + if name := leadingDecorator(p.comment); name != "" { + return fmt.Errorf("%s is %s, where it is not read; a decorator goes on its own line above the key", name, p.where) + } + } + return nil +} + +// leadingDecorator returns the decorator a comment opens with, or "". +func leadingDecorator(comment string) string { + for _, line := range strings.Split(comment, "\n") { + line = strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "#")) + fields := strings.Fields(line) + if len(fields) == 0 { + continue + } + for _, name := range decoratorNames { + if fields[0] == name { + return name + } + } + } + return "" +} + // decorators is the parsed @-block above a script key. type decorators struct { doc string diff --git a/internal/cli/app.go b/internal/cli/app.go index de1a275..5f0c32a 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -396,7 +396,9 @@ func (a *App) nearestCatalog() *catalog.Catalog { } func (a *App) listRunners(cat *catalog.Catalog) error { - fmt.Fprintf(a.Stdout, "%-10s %s [default]\n", string(catalog.RunnerInherit), execshell.DetectShell()) + shell, why := execshell.DetectedShell() + fmt.Fprintf(a.Stdout, "%-10s %s\n", string(catalog.RunnerInherit), shell) + fmt.Fprintf(a.Stdout, "%-10s the default runner, %s\n", "", why) fmt.Fprintln(a.Stdout) fmt.Fprintln(a.Stdout, "shells found here:") found := false diff --git a/internal/cli/e2e_contract_test.go b/internal/cli/e2e_contract_test.go index a6ba2b2..975f17e 100644 --- a/internal/cli/e2e_contract_test.go +++ b/internal/cli/e2e_contract_test.go @@ -165,7 +165,10 @@ func TestE2E_engineRunnersLists(t *testing.T) { t.Fatal(err) } got := out.String() - for _, want := range []string{"inherit", "[default]", "shells found here", "GODO_SHELL", "Not the shell you expected"} { + // The listing says which shell, and how it was chosen: a wrong shell is + // only diagnosable if the reader can tell a stale GODO_SHELL from a + // fallback. + for _, want := range []string{"inherit", "the default runner", "shells found here", "GODO_SHELL", "Not the shell you expected"} { if !strings.Contains(got, want) { t.Fatalf("missing %q in:\n%s", want, got) } diff --git a/internal/execshell/ancestor.go b/internal/execshell/ancestor.go new file mode 100644 index 0000000..6cbd569 --- /dev/null +++ b/internal/execshell/ancestor.go @@ -0,0 +1,51 @@ +package execshell + +// proc is one row of the process table: who started it, and what it is. +type proc struct { + parent uint32 + name string +} + +// maxAncestry bounds the walk. A shell is a handful of hops away at most; a +// longer chain means the answer is not up there, and the bound is also what +// keeps a corrupt table from spinning. +const maxAncestry = 16 + +// shellAncestor returns the nearest ancestor of pid that is a shell godo +// knows, or "". +// +// It walks rather than reading the parent alone because something is usually +// in between. A package manager's shim is the common one: scoop installs +// godo as shims\godo.exe, which starts the real godo.exe as a child, so the +// parent of the process asking this question is *godo* — and godo is not a +// shell, so reading one level up answered "no shell here" and fell back to +// %ComSpec%, telling every PowerShell user they were in cmd. The same shape +// appears with npm, bun and make wrappers, and inside an editor's terminal. +// +// Walking up is a wider question than "who started me" — it is "what shell am +// I under". That is the question the runner is trying to answer, and it is +// the same looseness $SHELL already has on Unix, where the login shell answers +// even when another shell is running right now. +// +// A pid is visited once. Windows reuses pids, so a table can point a process +// at a "parent" that is really its own descendant, and a cycle would otherwise +// never end. +func shellAncestor(table map[uint32]proc, pid uint32) string { + seen := make(map[uint32]bool, maxAncestry) + for i := 0; i < maxAncestry; i++ { + p, ok := table[pid] + if !ok || p.parent == 0 || seen[p.parent] { + return "" + } + seen[pid] = true + parent, ok := table[p.parent] + if !ok { + return "" + } + if isKnownShell(shellBase(parent.name)) { + return parent.name + } + pid = p.parent + } + return "" +} diff --git a/internal/execshell/ancestor_test.go b/internal/execshell/ancestor_test.go new file mode 100644 index 0000000..e4514f9 --- /dev/null +++ b/internal/execshell/ancestor_test.go @@ -0,0 +1,85 @@ +package execshell + +import "testing" + +func TestShellAncestor(t *testing.T) { + // explorer → WindowsTerminal → powershell → godo-shim → godo + table := map[uint32]proc{ + 4: {parent: 0, name: "System"}, + 100: {parent: 4, name: "explorer.exe"}, + 200: {parent: 100, name: "WindowsTerminal.exe"}, + 300: {parent: 200, name: "powershell.exe"}, + 400: {parent: 300, name: "godo.exe"}, // the scoop shim + 500: {parent: 400, name: "godo.exe"}, // godo itself + } + + t.Run("through a package manager shim", func(t *testing.T) { + // The bug this fixes: reading only the parent found godo.exe, decided + // no shell was there, and reported cmd.exe to a PowerShell user. + if got := shellAncestor(table, 500); got != "powershell.exe" { + t.Fatalf("got %q, want powershell.exe", got) + } + }) + + t.Run("directly from the shell", func(t *testing.T) { + if got := shellAncestor(table, 400); got != "powershell.exe" { + t.Fatalf("got %q, want powershell.exe", got) + } + }) + + t.Run("the nearest shell wins", func(t *testing.T) { + // A cmd started from PowerShell is the shell you are in. + table := map[uint32]proc{ + 300: {parent: 0, name: "powershell.exe"}, + 310: {parent: 300, name: "cmd.exe"}, + 320: {parent: 310, name: "godo.exe"}, + } + if got := shellAncestor(table, 320); got != "cmd.exe" { + t.Fatalf("got %q, want cmd.exe", got) + } + }) + + t.Run("no shell up there", func(t *testing.T) { + table := map[uint32]proc{ + 4: {parent: 0, name: "System"}, + 600: {parent: 4, name: "services.exe"}, + 610: {parent: 600, name: "godo.exe"}, + } + if got := shellAncestor(table, 610); got != "" { + t.Fatalf("got %q, want the caller to fall back", got) + } + }) + + t.Run("a pid missing from the table", func(t *testing.T) { + if got := shellAncestor(table, 9999); got != "" { + t.Fatalf("got %q, want empty", got) + } + }) +} + +// Windows reuses pids, so a table can point a process at a "parent" that is +// really its own descendant. The walk has to end anyway. +func TestShellAncestorTerminatesOnACycle(t *testing.T) { + table := map[uint32]proc{ + 10: {parent: 20, name: "a.exe"}, + 20: {parent: 10, name: "b.exe"}, + } + if got := shellAncestor(table, 10); got != "" { + t.Fatalf("got %q, want empty", got) + } +} + +// A chain of wrappers deeper than the bound is not worth walking, but it must +// not hang either. +func TestShellAncestorStopsAtTheBound(t *testing.T) { + table := map[uint32]proc{1: {parent: 0, name: "powershell.exe"}} + var pid uint32 = 1 + for i := 0; i < maxAncestry+5; i++ { + next := pid + 1 + table[next] = proc{parent: pid, name: "wrapper.exe"} + pid = next + } + if got := shellAncestor(table, pid); got != "" { + t.Fatalf("got %q, want empty past the bound", got) + } +} diff --git a/internal/execshell/inherit.go b/internal/execshell/inherit.go index feb4ec5..8f04fe8 100644 --- a/internal/execshell/inherit.go +++ b/internal/execshell/inherit.go @@ -87,22 +87,33 @@ func (r InheritRunner) Run(command string) error { // now: bash started inside zsh still reports zsh. That is the convention every // other tool follows, and GODO_SHELL is the way to disagree with it too. func DetectShell() string { + shell, _ := DetectedShell() + return shell +} + +// DetectedShell is DetectShell with the reason it landed there. +// +// The reason is not decoration. When the detected shell is wrong, the useful +// question is which rule answered — a stale GODO_SHELL reads nothing like a +// fallback to %ComSpec% because no shell was found — and a reader cannot tell +// those apart from a path alone. +func DetectedShell() (shell, why string) { if s := strings.TrimSpace(os.Getenv("GODO_SHELL")); s != "" { - return s + return s, "set by GODO_SHELL" } if runtime.GOOS == "windows" { if s := callerShell(); s != "" { - return s + return s, "found in the process tree" } if c := strings.TrimSpace(os.Getenv("ComSpec")); c != "" { - return c + return c, "%ComSpec%, because no shell is in the process tree" } - return "cmd.exe" + return "cmd.exe", "the last resort; no shell in the process tree, no %ComSpec%" } if s := strings.TrimSpace(os.Getenv("SHELL")); s != "" { - return s + return s, "your $SHELL" } - return "/bin/sh" + return "/bin/sh", "the last resort; $SHELL is unset" } // shellBase is the shell's name, lowercased and without a .exe suffix. diff --git a/internal/execshell/parent_windows.go b/internal/execshell/parent_windows.go index 43c8457..3dd2e85 100644 --- a/internal/execshell/parent_windows.go +++ b/internal/execshell/parent_windows.go @@ -8,45 +8,50 @@ import ( "unsafe" ) -// callerShell returns the shell that started this process, or "". +// callerShell returns the shell godo is running under, or "". // // Windows has no $SHELL, and the environment cannot answer the question: // PSModulePath is set by PowerShell but inherited by everything it starts, so -// a cmd.exe opened from PowerShell looks exactly like PowerShell. The parent -// process is the only honest answer, so that is what this reads. +// a cmd.exe opened from PowerShell looks exactly like PowerShell. The process +// tree is the only honest answer, so that is what this reads — see +// shellAncestor for why it is the tree and not just the parent. // -// Empty when the parent is not a shell — a build tool, an editor, a CI runner — -// which is the common case and why the caller falls back to %ComSpec%. +// Empty when no shell is up there — a service, a CI runner — which is why the +// caller falls back to %ComSpec%. func callerShell() string { - name, err := processName(uint32(os.Getppid())) - if err != nil || name == "" { - return "" - } - if !isKnownShell(shellBase(name)) { + table, err := processTable() + if err != nil { return "" } - return name + return shellAncestor(table, uint32(os.Getpid())) } -// processName returns the executable name of pid via the process snapshot. -func processName(pid uint32) (string, error) { +// processTable snapshots every running process once. +// +// One snapshot rather than one per hop: walking the tree would otherwise take +// a fresh snapshot at every level, and the tree could change underneath the +// walk between them. +func processTable() (map[uint32]proc, error) { snap, err := syscall.CreateToolhelp32Snapshot(syscall.TH32CS_SNAPPROCESS, 0) if err != nil { - return "", err + return nil, err } defer syscall.CloseHandle(snap) var e syscall.ProcessEntry32 e.Size = uint32(unsafe.Sizeof(e)) if err := syscall.Process32First(snap, &e); err != nil { - return "", err + return nil, err } + table := make(map[uint32]proc, 256) for { - if e.ProcessID == pid { - return syscall.UTF16ToString(e.ExeFile[:]), nil + table[e.ProcessID] = proc{ + parent: e.ParentProcessID, + name: syscall.UTF16ToString(e.ExeFile[:]), } if err := syscall.Process32Next(snap, &e); err != nil { - return "", err + // ERROR_NO_MORE_FILES ends the walk; the table is complete. + return table, nil } } } diff --git a/internal/plugin/invoke.go b/internal/plugin/invoke.go index 063dc51..87bfec9 100644 --- a/internal/plugin/invoke.go +++ b/internal/plugin/invoke.go @@ -320,8 +320,9 @@ func (p *Plugin) slinkOp(op Op, host Host) Result { return Result{Error: fmt.Sprintf("slink: dst: %v", err)} } - // Windows support for os.Symlink is unverified on a real Windows host. - if err := os.Symlink(src, dst); err != nil { + // The mechanism is the platform's: a symlink on Unix, a junction or a hard + // link on Windows. See linkPath. + if err := linkPath(src, dst); err != nil { return Result{Error: fmt.Sprintf("slink: %v", err)} } return Result{Code: 0, OK: true} diff --git a/internal/plugin/reparse.go b/internal/plugin/reparse.go new file mode 100644 index 0000000..8c35ceb --- /dev/null +++ b/internal/plugin/reparse.go @@ -0,0 +1,68 @@ +package plugin + +import ( + "encoding/binary" + "fmt" + "unicode/utf16" +) + +// Sizes of REPARSE_DATA_BUFFER, which Windows defines as: +// +// ULONG ReparseTag; 4 +// USHORT ReparseDataLength; 2 +// USHORT Reserved; 2 +// USHORT SubstituteNameOffset; 2 <- the mount-point body starts here +// USHORT SubstituteNameLength; 2 +// USHORT PrintNameOffset; 2 +// USHORT PrintNameLength; 2 +// WCHAR PathBuffer[]; +const ( + reparseHeader = 8 + mountPointFields = 8 + reparseTagMountPt = 0xA0000003 // IO_REPARSE_TAG_MOUNT_POINT + maxReparseData = 16384 // MAXIMUM_REPARSE_DATA_BUFFER_SIZE +) + +// mountPointBuffer builds the reparse data that turns a directory into a +// junction pointing at target. +// +// It lives here, away from the syscall and without a build tag, because the +// layout is the part that is easy to get wrong and the part worth testing on +// whatever machine you happen to be on. The syscall around it is three lines. +// +// target must be an absolute path. PathBuffer holds both names, each +// NUL-terminated: the substitute name is the NT object path (\??\C:\x), which +// is what the filesystem follows, and the print name is the plain path, which +// is what Explorer shows. +func mountPointBuffer(target string) ([]byte, error) { + sub := utf16.Encode([]rune(`\??\` + target)) + print16 := utf16.Encode([]rune(target)) + + path := make([]byte, 0, (len(sub)+len(print16)+2)*2) + path = appendUTF16(path, sub) + printOffset := len(path) + path = appendUTF16(path, print16) + + if reparseHeader+mountPointFields+len(path) > maxReparseData { + return nil, fmt.Errorf("junction target is too long: %s", target) + } + + buf := make([]byte, 0, reparseHeader+mountPointFields+len(path)) + buf = binary.LittleEndian.AppendUint32(buf, reparseTagMountPt) + buf = binary.LittleEndian.AppendUint16(buf, uint16(mountPointFields+len(path))) + buf = binary.LittleEndian.AppendUint16(buf, 0) // Reserved + buf = binary.LittleEndian.AppendUint16(buf, 0) // SubstituteNameOffset + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(sub)*2)) + buf = binary.LittleEndian.AppendUint16(buf, uint16(printOffset)) + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(print16)*2)) + return append(buf, path...), nil +} + +// appendUTF16 writes code units little-endian and terminates them with a NUL, +// which the lengths above exclude and the offsets count. +func appendUTF16(dst []byte, s []uint16) []byte { + for _, u := range s { + dst = binary.LittleEndian.AppendUint16(dst, u) + } + return binary.LittleEndian.AppendUint16(dst, 0) +} diff --git a/internal/plugin/reparse_test.go b/internal/plugin/reparse_test.go new file mode 100644 index 0000000..6502d6c --- /dev/null +++ b/internal/plugin/reparse_test.go @@ -0,0 +1,76 @@ +package plugin + +import ( + "encoding/binary" + "strings" + "testing" + "unicode/utf16" +) + +// The layout is what Windows reads byte by byte, so it is checked field by +// field. Getting an offset wrong produces a directory that looks like a +// junction and points nowhere. +func TestMountPointBuffer(t *testing.T) { + const target = `C:\Users\me\src\repo` + buf, err := mountPointBuffer(target) + if err != nil { + t.Fatal(err) + } + + tag := binary.LittleEndian.Uint32(buf[0:4]) + if tag != reparseTagMountPt { + t.Fatalf("tag=%#x, want IO_REPARSE_TAG_MOUNT_POINT", tag) + } + dataLen := binary.LittleEndian.Uint16(buf[4:6]) + if int(dataLen) != len(buf)-reparseHeader { + t.Fatalf("ReparseDataLength=%d, want %d (everything after the header)", dataLen, len(buf)-reparseHeader) + } + if r := binary.LittleEndian.Uint16(buf[6:8]); r != 0 { + t.Fatalf("Reserved=%d, want 0", r) + } + + subOff := binary.LittleEndian.Uint16(buf[8:10]) + subLen := binary.LittleEndian.Uint16(buf[10:12]) + printOff := binary.LittleEndian.Uint16(buf[12:14]) + printLen := binary.LittleEndian.Uint16(buf[14:16]) + path := buf[reparseHeader+mountPointFields:] + + if subOff != 0 { + t.Fatalf("SubstituteNameOffset=%d, want 0", subOff) + } + // The substitute name is the NT object path: that prefix is what makes the + // junction resolve at all. + if got := decodeUTF16(path[subOff : subOff+subLen]); got != `\??\`+target { + t.Fatalf("substitute name=%q", got) + } + if got := decodeUTF16(path[printOff : printOff+printLen]); got != target { + t.Fatalf("print name=%q", got) + } + // Each name is NUL-terminated, and the lengths exclude the terminator — + // which is why the print name starts two bytes past the end of the first. + if int(printOff) != int(subLen)+2 { + t.Fatalf("PrintNameOffset=%d, want %d (past the substitute name and its NUL)", printOff, subLen+2) + } + if u := binary.LittleEndian.Uint16(path[subLen : subLen+2]); u != 0 { + t.Fatalf("substitute name is not NUL-terminated") + } + if u := binary.LittleEndian.Uint16(path[printOff+printLen:][:2]); u != 0 { + t.Fatalf("print name is not NUL-terminated") + } +} + +// Windows caps reparse data at 16 KiB, and a refusal here is better than one +// from DeviceIoControl after the directory has already been created. +func TestMountPointBufferRefusesAnEnormousTarget(t *testing.T) { + if _, err := mountPointBuffer(`C:\` + strings.Repeat("x", maxReparseData)); err == nil { + t.Fatal("want an error") + } +} + +func decodeUTF16(b []byte) string { + u := make([]uint16, len(b)/2) + for i := range u { + u[i] = binary.LittleEndian.Uint16(b[i*2:]) + } + return string(utf16.Decode(u)) +} diff --git a/internal/plugin/symlink_other.go b/internal/plugin/symlink_other.go new file mode 100644 index 0000000..85556ee --- /dev/null +++ b/internal/plugin/symlink_other.go @@ -0,0 +1,11 @@ +//go:build !windows + +package plugin + +import "os" + +// linkPath makes dst point at src. +// +// A symlink is an ordinary file operation here, needs no privilege, and may +// dangle. Windows has none of those three, which is why it has its own. +func linkPath(src, dst string) error { return os.Symlink(src, dst) } diff --git a/internal/plugin/symlink_windows.go b/internal/plugin/symlink_windows.go new file mode 100644 index 0000000..3177746 --- /dev/null +++ b/internal/plugin/symlink_windows.go @@ -0,0 +1,91 @@ +//go:build windows + +package plugin + +import ( + "fmt" + "os" + "path/filepath" + + "golang.org/x/sys/windows" +) + +// linkPath makes dst point at src using the link Windows actually offers. +// +// A symlink on Windows is a privilege, not a file operation: without Developer +// Mode or an elevated terminal the OS refuses one, so a plugin that links +// something worked on Unix and failed here. Windows has two links that need no +// privilege, and between them they cover what a symlink is used for: +// +// a directory -> a junction (a mount-point reparse point) +// a file -> a hard link +// +// So that is what this uses. The mechanism is the platform's; the op is the +// same op. +// +// src must exist, because which link to make depends on what it is. That is +// the one place this cannot match Unix, where a link may dangle. +func linkPath(src, dst string) error { + info, err := os.Stat(src) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%s does not exist; on Windows the link is a junction for a directory "+ + "and a hard link for a file, so the target has to be there to say which", src) + } + return err + } + if info.IsDir() { + return createJunction(src, dst) + } + if err := os.Link(src, dst); err != nil { + return fmt.Errorf("%w (a hard link is how a file is linked on Windows; "+ + "it cannot cross volumes, so src and dst must be on the same drive)", err) + } + return nil +} + +// createJunction makes dst a junction pointing at the directory src. +// +// A junction is a directory carrying a mount-point reparse point, so the +// directory is created first and the reparse point written into it. If writing +// it fails the empty directory is removed, because a bare directory where a +// link was asked for is worse than nothing. +func createJunction(src, dst string) error { + target, err := filepath.Abs(src) + if err != nil { + return err + } + if err := os.Mkdir(dst, 0o755); err != nil { + return err + } + if err := writeMountPoint(dst, target); err != nil { + os.Remove(dst) + return err + } + return nil +} + +// writeMountPoint sets the mount-point reparse point on an existing directory. +func writeMountPoint(dir, target string) error { + buf, err := mountPointBuffer(target) + if err != nil { + return err + } + name, err := windows.UTF16PtrFromString(dir) + if err != nil { + return err + } + // BACKUP_SEMANTICS is what lets a directory be opened at all; + // OPEN_REPARSE_POINT opens the directory itself rather than following a + // reparse point that is already on it. + h, err := windows.CreateFile(name, windows.GENERIC_WRITE, 0, nil, windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) + if err != nil { + return err + } + defer windows.CloseHandle(h) + + var returned uint32 + return windows.DeviceIoControl(h, windows.FSCTL_SET_REPARSE_POINT, + &buf[0], uint32(len(buf)), nil, 0, &returned, nil) +}