diff --git a/internal/install/install.go b/internal/install/install.go index 567243e1..90e56603 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -221,9 +221,23 @@ func (s Spec) Entry(event string) (json.RawMessage, error) { return b, nil } +// legacyForms maps an event to older subcommands a previous release wrote +// for it. Owner recognises a listed form as ours, for an entry a release +// renames: it still refreshes and still detaches, not lost until the user's +// next watch happens to touch it and not left behind for detach to miss +// (LUI-3, LUI-4). Nothing has been renamed yet, so every list here is empty. +var legacyForms = map[string][]string{} + // Owner returns the install id an entry claims for an event, or "" for an // entry that is not ours. Ownership is carried in the command line itself, // which survives every round trip a settings file can take. +// +// A command matches only as a WHOLE: everything after its first shell word +// (the executable, bare or quoted) must read exactly "
--install " +// for event's current subcommand or one of legacyForms[event] -- never +// merely END with that text. A suffix check is the trap: "post" is also the +// tail of "steer post", so a future entry installed under that longer +// subcommand would have misread as this one's (LUI-3). func Owner(raw json.RawMessage, event string) string { var g struct { Hooks []struct { @@ -233,17 +247,76 @@ func Owner(raw json.RawMessage, event string) string { if json.Unmarshal(raw, &g) != nil { return "" } - suffix := " " + subcommand(event) + " " + Marker + " " + forms := append([]string{subcommand(event)}, legacyForms[event]...) for _, h := range g.Hooks { - if i := strings.LastIndex(h.Command, suffix); i >= 0 { - if id := h.Command[i+len(suffix):]; isInstallID(id) { - return id - } + if id := ownerOf(h.Command, forms); id != "" { + return id + } + } + return "" +} + +// ownerOf matches cmd against the whole of one of forms, not a suffix of it: +// see Owner. It returns the claimed install id, or "" if none of forms +// matches or the command does not parse as " ". +func ownerOf(cmd string, forms []string) string { + rest, ok := afterFirstWord(cmd) + if !ok { + return "" + } + for _, form := range forms { + prefix := form + " " + Marker + " " + if !strings.HasPrefix(rest, prefix) { + continue + } + if id := rest[len(prefix):]; isInstallID(id) { + return id } } return "" } +// afterFirstWord splits cmd at the end of its first shell word -- the +// executable, exactly as shellQuote writes it, bare or single-quoted -- and +// returns what follows the one space after it. It does not reconstruct the +// word itself: Owner does not care what the executable is (a moved binary is +// what watch exists to refresh), only where it ends, so a caller can compare +// everything after it exactly rather than searching for a trailing match. +// +// ok is false when cmd does not have the shape " " at all: no +// separating space for a bare word, or shellQuote's own escaping of an +// embedded quote -- close quote, backslash, quote, open quote -- never +// closes. +func afterFirstWord(cmd string) (rest string, ok bool) { + if cmd == "" { + return "", false + } + if cmd[0] != '\'' { + i := strings.IndexByte(cmd, ' ') + if i < 0 { + return "", false + } + return cmd[i+1:], true + } + for i := 1; ; { + j := strings.IndexByte(cmd[i:], '\'') + if j < 0 { + return "", false + } + end := i + j + if strings.HasPrefix(cmd[end:], `'\''`) { + // An embedded quote in the executable, escaped as close, + // backslash, quote, open -- not the terminator. + i = end + 4 + continue + } + if end+1 < len(cmd) && cmd[end+1] == ' ' { + return cmd[end+2:], true + } + return "", false + } +} + // Apply ensures exactly one entry of ours per event, refreshing it if the // binary has moved and collapsing duplicates. It reports whether the document // changed, so a no-op watch writes nothing. An entry of ours that is no longer diff --git a/internal/install/install_test.go b/internal/install/install_test.go index ab55f177..7909050e 100644 --- a/internal/install/install_test.go +++ b/internal/install/install_test.go @@ -45,6 +45,58 @@ func TestOwner(t *testing.T) { `{"type":"command","command":"` + spec(oneID).Command(EventSessionStart) + `","timeout":5}]}`, oneID, }, + // The four cases below exercise afterFirstWord's single-quoted + // branch, the one shellQuote sends every path through that is not + // made of bare-safe characters: a backslash (so every Windows + // install), an embedded single quote, or a space. Without these, + // the branch that every Windows install and any POSIX install + // under a path with a space or a quote actually runs had no + // coverage at all -- a break in it would have shown as "every + // watch adds a duplicate, every detach removes nothing" with no + // test going red first. + { + "quoted executable with an embedded single quote", + EventPreToolUse, + string(mustEntry(t, Spec{Executable: "/home/o'brien/bin/rashomon", InstallID: oneID}, EventPreToolUse)), + oneID, + }, + { + "quoted executable with a space", + EventPreToolUse, + string(mustEntry(t, Spec{Executable: "/a b/rashomon", InstallID: oneID}, EventPreToolUse)), + oneID, + }, + { + "quoted executable with Windows backslashes", + EventPreToolUse, + string(mustEntry(t, Spec{Executable: `C:\x\rashomon.exe`, InstallID: oneID}, EventPreToolUse)), + oneID, + }, + { + // shellQuote never writes this shape -- it always closes the + // quote it opens -- but afterFirstWord has to resolve any + // command a settings file might actually hold, including one + // hand-edited into an invalid state. An unclosed quote has no + // "end of the first word" to split at, so Owner must read it + // as nobody's rather than guess. + "unterminated quote never resolves to an owner", + EventPreToolUse, + `{"hooks":[{"type":"command","command":"'/no/closing/quote post --install ` + oneID + `"}]}`, + "", + }, + { + // LUI-2026-10-17: the narrowing to a whole-remainder match (see + // TestLUI3_) also means a hand-wrapped command -- a env-prefix + // here -- is no longer recognised as ours, even though the + // wrapped tail is otherwise byte-identical to an owned entry. + // Pinned here so the choice stays visible: a future change that + // restores suffix matching to recover this case would have to + // change this test, not just add one. + "a command wrapped in an env prefix is not ours", + EventPostToolUse, + `{"hooks":[{"type":"command","command":"env FOO=1 ` + spec(oneID).Command(EventPostToolUse) + `"}]}`, + "", + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -55,6 +107,107 @@ func TestOwner(t *testing.T) { } } +// TestLUI3_OwnerDoesNotClaimSteerPostAsPost pins LUI-3: identity is read off +// the whole of the command after its executable, not a suffix of it. +// +// "post" is PostToolUse's own subcommand, and it is also the tail of "steer +// post" -- a longer subcommand a future entry under a different role +// installs on the very same event. The old implementation searched for +// " post --install " anywhere in the command, so a "steer post" entry would +// have read, wrongly, as this install's post entry: detach would have +// removed somebody's steer entry under the recorder's name, and a second +// watch would have found "ours" already present and left a stray steer +// entry untouched. The fix is a whole-remainder comparison, so a longer +// subcommand that merely ends the same way is never mistaken for a shorter +// one that is a true suffix of it. +func TestLUI3_OwnerDoesNotClaimSteerPostAsPost(t *testing.T) { + notOurs := json.RawMessage(`{"matcher":"Bash","hooks":[{"type":"command",` + + `"command":"/usr/local/bin/rashomon steer post --install ` + oneID + `","timeout":5}]}`) + if got := Owner(notOurs, EventPostToolUse); got != "" { + t.Errorf("Owner(%s) = %q, want %q: a steer post entry is not the recorder's post entry", + notOurs, got, "") + } + + // The positive twin, so the fix is a narrowing and not a break: the + // recorder's own post entry, same executable and id, is still read. + ours := json.RawMessage(mustEntry(t, spec(oneID), EventPostToolUse)) + if got := Owner(ours, EventPostToolUse); got != oneID { + t.Errorf("Owner(%s) = %q, want %q", ours, got, oneID) + } +} + +// TestLUI4_WatchThenDetachLeavesNoResidue pins LUI-4's legacy half: Owner +// recognises a listed legacy subcommand as ours, so an entry a previous +// release wrote under an older subcommand is refreshed to the current one in +// place -- never duplicated -- and a later detach removes it exactly as it +// would the current form. A foreign entry under the same event, and every +// event watch never touches, stay byte for byte as found; that half of the +// promise is TestH06_DetachLeavesTheFileByteForByteAsFound's and is +// unchanged here. +// +// No real subcommand has been renamed yet (legacyForms is empty for every +// production event), so this test supplies its own legacy form rather than +// one shipped: it proves the mechanism a rename rides on, not a rename. +func TestLUI4_WatchThenDetachLeavesNoResidue(t *testing.T) { + const legacy = "recap-legacy-v1" + legacyForms[EventStop] = []string{legacy} + t.Cleanup(func() { delete(legacyForms, EventStop) }) + + exe := "/usr/local/bin/rashomon" + legacyEntry := `{"hooks":[{"type":"command","command":"` + + exe + " " + legacy + " " + Marker + " " + oneID + `","timeout":` + fmt.Sprint(RecapTimeout) + `}]}` + doc, err := settings.Parse([]byte(`{"hooks":{"Stop":[` + foreignHook + `,` + legacyEntry + `]}}`)) + if err != nil { + t.Fatalf("seeding: %v", err) + } + + if _, err := Apply(doc, Spec{Executable: exe, InstallID: oneID}); err != nil { + t.Fatalf("Apply: %v", err) + } + + stopEntries := entriesOf(t, doc, EventStop) + if len(stopEntries) != 2 { + t.Fatalf("Stop carries %d entries after refresh, want 2 (foreign, plus ours refreshed in place)", + len(stopEntries)) + } + var refreshed json.RawMessage + for _, e := range stopEntries { + if Owner(e, EventStop) == oneID { + refreshed = e + } + } + if refreshed == nil { + t.Fatal("no entry under Stop reads as ours after Apply refreshed the legacy form") + } + if detail := intact(refreshed, EventStop); detail != "" { + t.Errorf("the refreshed entry is not intact: %s", detail) + } + if strings.Contains(string(refreshed), legacy) { + t.Errorf("the refreshed entry still carries the legacy subcommand %q: %s", legacy, refreshed) + } + + removed, modified, err := Remove(doc, Spec{InstallID: oneID}, false) + if err != nil { + t.Fatalf("Remove: %v", err) + } + if removed == 0 { + t.Error("Remove removed nothing; the refreshed entry should have been recognised and removed") + } + if len(modified) != 0 { + t.Errorf("Remove reported entries it had to leave edited: %v", modified) + } + remainingStop := entriesOf(t, doc, EventStop) + if len(remainingStop) != 1 { + t.Fatalf("under Stop, %d entries remain, want 1 (the foreign one)", len(remainingStop)) + } + if id := Owner(remainingStop[0], EventStop); id != "" { + t.Errorf("an owned entry survived detach under Stop, reading as %q", id) + } + if !strings.Contains(string(doc.Bytes()), foreignHook) { + t.Errorf("the foreign entry did not survive byte for byte:\n%s", doc.Bytes()) + } +} + // TestRemoveIfAnyInstallID is the detach --all path. With the store gone there // is no id left to match against, so the predicate accepts every install -- and // the entry belonging to no install still has to survive it. diff --git a/test/mutation/mutants/H-5_install.py b/test/mutation/mutants/H-5_install.py new file mode 100644 index 00000000..56978852 --- /dev/null +++ b/test/mutation/mutants/H-5_install.py @@ -0,0 +1,25 @@ +# afterFirstWord's single-quoted branch has no requirement-row test of its +# own -- TestLUI3_ and TestLUI4_ cover the whole-remainder comparison and the +# legacy-form lookup, both downstream of this split, but neither one's +# command is ever quoted. Every Windows install goes through this branch +# (shellQuote quotes any path with a backslash), as does any POSIX install +# under a path with a space or an embedded quote. This mutant breaks the +# escaped-quote skip: resuming the search one byte past the closing quote, +# instead of four, lands inside the backslash-quote-open sequence itself and +# finds a false terminator there, so a quoted executable with an embedded +# quote never resolves to an owner. +m("afterFirstWord resumes inside the escaped-quote sequence instead of past it", + "internal/install/install.go", + "\t\tif strings.HasPrefix(cmd[end:], `'\\''`) {\n" + "\t\t\t// An embedded quote in the executable, escaped as close,\n" + "\t\t\t// backslash, quote, open -- not the terminator.\n" + "\t\t\ti = end + 4\n" + "\t\t\tcontinue\n" + "\t\t}", + "\t\tif strings.HasPrefix(cmd[end:], `'\\''`) {\n" + "\t\t\t// An embedded quote in the executable, escaped as close,\n" + "\t\t\t// backslash, quote, open -- not the terminator.\n" + "\t\t\ti = end + 1\n" + "\t\t\tcontinue\n" + "\t\t}", + "TestOwner/quoted_executable_with_an_embedded_single_quote") diff --git a/test/mutation/mutants/LUI-3_install.py b/test/mutation/mutants/LUI-3_install.py new file mode 100644 index 00000000..02165419 --- /dev/null +++ b/test/mutation/mutants/LUI-3_install.py @@ -0,0 +1,29 @@ +# LUI-3: entry identity is the whole command after the executable, not a +# suffix of it. The old implementation searched h.Command for +# " --install " anywhere, so a longer subcommand sharing a +# shorter one's tail -- "steer post" ends in "post" -- would have read as the +# shorter one's entry. TestLUI3_ pins the exact-match replacement directly; +# TestLUI4_ also goes red, because the same suffix search never finds a +# legacy form that is not itself a bare suffix of the command (e.g. +# "recap-legacy-v1" against " recap --install "). +m("LUI-3 Owner matches a suffix of the command instead of the whole of it", + "internal/install/install.go", + "\tforms := append([]string{subcommand(event)}, legacyForms[event]...)\n" + "\tfor _, h := range g.Hooks {\n" + "\t\tif id := ownerOf(h.Command, forms); id != \"\" {\n" + "\t\t\treturn id\n" + "\t\t}\n" + "\t}\n" + "\treturn \"\"\n" + "}", + "\tsuffix := \" \" + subcommand(event) + \" \" + Marker + \" \"\n" + "\tfor _, h := range g.Hooks {\n" + "\t\tif i := strings.LastIndex(h.Command, suffix); i >= 0 {\n" + "\t\t\tif id := h.Command[i+len(suffix):]; isInstallID(id) {\n" + "\t\t\t\treturn id\n" + "\t\t\t}\n" + "\t\t}\n" + "\t}\n" + "\treturn \"\"\n" + "}", + "TestLUI3_") diff --git a/test/mutation/mutants/LUI-4_install.py b/test/mutation/mutants/LUI-4_install.py new file mode 100644 index 00000000..f9a3992a --- /dev/null +++ b/test/mutation/mutants/LUI-4_install.py @@ -0,0 +1,10 @@ +# LUI-4: Owner recognises a listed legacy subcommand as ours, so a rename +# refreshes an old entry in place instead of leaving it behind as residue +# (detach would otherwise never recognise it either). Dropping legacyForms +# from the candidate list is the whole regression: TestLUI4_ seeds a +# legacy-shaped entry, so the refresh never fires, and Stop ends up with +# three entries where it should have two. +m("LUI-4 Owner never tries a listed legacy subcommand", "internal/install/install.go", + "\tforms := append([]string{subcommand(event)}, legacyForms[event]...)\n", + "\tforms := []string{subcommand(event)}\n", + "TestLUI4_")