Repository navigation
install: match an entry's whole command, not a suffix of it - #72
Conversation
lutic1
left a comment
There was a problem hiding this comment.
@k4rthiknerella, here's an early review of this draft at eda07c2. It's in good shape and close to approvable.
- Older entries still match. The command format is byte-identical across v0.1.0-alpha.1, v1.1.0 and HEAD, and Owner never inspects the executable. Every entry a released version wrote is still recognised, including Homebrew Cellar paths, symlinks, moved binaries and
~/$HOME. - Quoting round-trips. I fuzzed 200k random executables through
EntryandOwnerfor all six events, and every one round-tripped. - The tests and mutants hold. All three mutants go red.
Before it leaves draft
-
detachnow silently leaves some entries running. These are no longer claimed as ours (reproduced):- entries wrapped as
env FOO=1 … - a double-quoted path containing a space
- two spaces or a tab after the executable
- leading or trailing whitespace
For each,
watchadds a second entry, both fire, and you get duplicate records.detachthen removes ours and says nothing about the wrapped one. Builds from thewindows-native-supportbranch write"C:\Program Files\…exe" hook --install …, which is no longer matched either. Keep the strict match, but have Remove/Present report any entry that contains--install <this id>without matching, so it isn't missed silently. - entries wrapped as
-
Pin the narrowing cases. Two of my throwaway mutants survive every test:
TrimLeft(cmd[end+1:], " ")in place of the exactly-one-space checkstrings.TrimSpace(cmd)inafterFirstWord
Add TestOwner cases for two spaces,
'x'y post, and leading/trailing space.
Smaller
"$HOME/x" post …is claimed, but"/a b/x" post …is not, while the doc says "bare or quoted".- The H-5 mutant is misfiled: H-5 is "first run on a clean machine".
- The LUI-3/4 rows can't be traced anywhere in the repo.
TestLUI4_mutates the package-globallegacyForms, which will break if anyone addst.Parallel.- Run the full mutation sweep before merge. CI skipped it.
Generated by Claude Code
|
@k4rthiknerella, since you don't have Claude Code, I ran the repo's live test
This adds to my review above; it doesn't replace it, and the open items there still stand. Generated by Claude Code |
eda07c2 to
b2f4a90
Compare
a3f9a9e to
af6a631
Compare
lutic1
left a comment
There was a problem hiding this comment.
@k4rthiknerella, I re-checked at b2f4a90. It's a rebase onto the new record/intake, and both commits are byte-identical to eda07c2. Build, vet, -race and --check pass, and Perf gates is red on the base too.
Confirmed fixed: none yet.
Still open, re-reproduced on b2f4a90
-
Ownerstill returns no owner for wrapped entries, so detach skips them without a word:env FOO=1 /…/rashomon post --install <id>"/a b/rashomon" post …and"C:\Program Files\…\rashomon.exe" post …- two spaces, a tab, or leading or trailing whitespace
Keep the strict match, but have Remove/Present report any entry that contains
--install <this id>and doesn't match. -
The narrowing is still unpinned. Add
TestOwnercases for two spaces,'x'y post, and leading and trailing space. -
The smaller items are unchanged: the "bare or quoted" doc, the H-5 filing, the LUI-3/4 rows, the global
legacyForms, and the sweep.
Generated by Claude Code
af6a631 to
2954b7b
Compare
Owner read ownership off the tail of a hook's command line: it searched for " <subcommand> --install " anywhere in the string and took whatever followed as the install id. That is wrong once two entries can share an event with subcommands where one is a suffix of the other -- a "post" entry and a longer one that happens to end the same way would both read as "post"'s. Owner now splits the command at the end of its first shell word (the executable, bare or single-quoted) and compares everything after it exactly against the subcommand, so a longer form is never mistaken for a shorter one it merely ends with. This also adds the hook a subcommand rename needs: a per-event list of older subcommands Owner still recognises as ours, so a release that renames one can refresh an existing entry in place (watch) and still remove it (detach) instead of leaving it as residue. No subcommand has been renamed yet, so every list is empty; the mechanism is proven with a test-supplied legacy form rather than a real one.
afterFirstWord's single-quoted branch had no test of its own. Every Windows install runs through it (shellQuote quotes any path with a backslash), as does any POSIX install under a path with a space or an embedded quote, so a break there would have shown up as every watch adding a duplicate entry and every detach removing nothing, with CI staying green. Add TestOwner cases for an embedded quote, a space, and Windows backslashes, plus an unterminated quote resolving to no owner. Add H-5_install.py, a mutant on the escaped-quote skip, confirmed red by the three new quoted cases. The whole-remainder match also means a command this install did not write itself, but whose tail happens to be byte-identical to ours, no longer reads as ours -- an env-prefixed wrapper, for one. Pin that with its own TestOwner case so a future change back to a looser match has to touch this test, not just add one.
b2f4a90 to
deb31ad
Compare
lutic1
left a comment
There was a problem hiding this comment.
@k4rthiknerella, I re-checked at deb31ad. It's a rebase onto main (e453ed4), and git range-diff shows both commits byte-identical to b2f4a90. Build, vet, -race and --check pass. Perf gates (linux) is red only for main's known issues: the six 1/4/8 MiB rows with no linux/amd64 row, the TestRES28p floor refusal, and the post.edit_314k and recap.stop entry rows. Benchgate and RES-29 pass.
Confirmed fixed: none yet.
Still open (code unchanged)
-
Ownerstill returns no owner for wrapped entries (internal/install/install.go:241), so detach skips them without a word:env FOO=1 /…/rashomon post --install <id>"/a b/rashomon" post …and"C:\Program Files\…\rashomon.exe" post …- two spaces, a tab, or leading or trailing whitespace
Keep the strict match, but have Remove/Present report any entry that contains
--install <this id>and doesn't match. -
The narrowing is still unpinned. Add
TestOwnercases for two spaces,'x'y post, and leading and trailing space. -
The smaller items are unchanged:
- the "bare or quoted" doc (
install.go:236); - the H-5 filing;
- the LUI-3/4 rows;
- the global
legacyForms(install_test.go:153); - the full mutation sweep, which CI still skips.
- the "bare or quoted" doc (
The description still says "Stacked on #61", but the base is now main and #61 has landed.
Generated by Claude Code
Stacked on #61 (intake: one bounded read and one scan of every hook payload); review and merge after it.
What changed
Ownerdecided whether a hook entry belonged to this install by searchingits command line for
" <subcommand> --install "anywhere in the string.That is a suffix check, and a suffix check has a hole: a longer subcommand
that happens to end the same way as a shorter one would read as the shorter
one's entry --
TestLUI3_pins the concrete case, asteer postentryunder
PostToolUseagainst the recorder's ownpostentry on the sameevent. Without this fix, the first entry a future role installs under a
subcommand ending in
postwould have been misread as the recorder's ownPostToolUseentry by both watch and detach.Ownernow splits a command at the end of its first shell word -- theexecutable, bare or single-quoted exactly as it is written -- and compares
everything after it exactly against the subcommand, never as a suffix. It
also now accepts a per-event list of older subcommands as equally ours, so a
future rename can refresh an entry in place instead of leaving the old form
behind for detach to miss it. Nothing has been renamed yet, so every list is
empty;
TestLUI4_supplies its own legacy form to prove the mechanism aheadof a real rename using it.
TestLUI3_needs none -- its case is thewhole-remainder comparison itself, not the legacy lookup.
This is a narrowing, and it has a real consequence: a command this install
did not write itself, but whose tail after the executable is byte-identical
to ours, is no longer recognised. The old suffix search claimed
env FOO=1 /path/rashomon post --install <id>, a double-quoted executable, or anexecutable followed by two spaces before the subcommand, because each still
ends in the right text. The new whole-remainder check does not, because
none of those is "
<word>then exactly one space then the matched form."Watch will now add a second entry beside a hand-wrapped one instead of
refreshing it in place, and detach will leave the hand-wrapped one behind as
a live hook. Nothing on this branch writes a command in any of those shapes,
so nothing regresses today, but it is a real narrowing of what Owner
recognises, not a no-op refactor.
Apply,Remove,RemoveIf,PresentandForeignOwnersare themselvesuntouched -- the fix is entirely inside
Owner-- so everything else aboutwatch and detach (refreshing, collapsing duplicates, leaving foreign entries
and emptied event keys alone) is exactly as it was before, for every command
shape
Ownerstill recognises as ours.What pins it
TestLUI3_OwnerDoesNotClaimSteerPostAsPost: a command ending inpostbutreading
steer postafter its executable is not claimed by thePostToolUseentry, whose own subcommand ispost; the positive twin inthe same test confirms a real
postentry is still read.TestLUI4_WatchThenDetachLeavesNoResidue: an entry written under a(test-supplied) legacy subcommand is recognised, refreshed to the current
form in place rather than duplicated, and removed by a later detach; a
foreign entry under the same event survives byte for byte throughout.
the reason the fix describes, before being run green against it.
TestOwnernow also coversafterFirstWord's single-quoted branchdirectly: an embedded quote, a space, and Windows backslashes (every
Windows install goes through this branch, since
shellQuotequotes anypath containing one), plus an unterminated quote resolving to no owner.
That branch had no case of its own before; a probe that always failed it
(forcing
afterFirstWord's quoted path to return"", false) still leftgo test ./internal/install/...green on the old test file. It also pinsthe narrowing above with a case of its own: a command wrapped in
env FOO=1is not recognised.LUI-3_install.py(revertingOwnerto the suffixsearch),
LUI-4_install.py(dropping the legacy-form list), andH-5_install.py(new -- resuming the escaped-quote skip one byte short,landing inside the escape sequence instead of past it), all three
confirmed red by
sweep.pyitself against this file's mutants, not justcopied out and checked separately. The sweep's static check
(
sweep.py --check) passes across the full 1422-mutant set with everyanchor occurring exactly once; the other mutants that target files
outside this change were not re-run as part of it.
What was measured
Nothing performance-sensitive changed; no
budgets.jsonrow is touched.What is left
A role whose subcommand ends in
post(the shapeTestLUI3_guardsagainst) is not part of this change and is not installed by anything on
this branch.
Owner's fix and the legacy mechanism are ahead of that workon purpose, so the first PR that installs one lands on an identity check
that already handles it correctly.
The narrowing noted above -- a hand-wrapped or hand-edited command no longer
read as ours -- is accepted as-is; nothing on this branch writes a command
in one of those shapes. If that ever needs recovering, it is a new decision
about what Owner should treat as identity, not a bug in this fix.