Recast hooks as general event handlers, not a release-only feature

[rdeago]

Proposed changes

Hooks are a general mechanism — repository-owned event handlers that bv runs at well-known paths of the form .buildvana/hooks/<context>/<event>.cs — but code and docs presented them as a feature of bv release. <context> names the context an event belongs to (today, always the invoking command, though nothing ties a context to being a command) and <event> names the moment of execution that triggers the hook: if Buildvana were an object and hooks were functions, .buildvana/hooks/release/post-release.cs would be the Release_PostRelease handler. release/post-release is merely the first event, not the shape of the feature.

Completing the event-handler parallel, the data a hook receives is its event args, freeing "context" to mean only the <context> path segment:

• PostReleaseHookContext → PostReleaseHookArgs; its Command/Moment constants → Context/Event.
• WellKnownPaths.HookContextsDirectory → HookArgsDirectory, with the on-disk path renamed from .buildvana-temp/hook-contexts/ to .buildvana-temp/hook-args/; GetHookContextFile(command, moment) → GetHookArgsFile(context, event).
• HookRunner.RunHookAsync takes (context, event, args); ReleaseCommand names the hook via the new constants instead of string literals.
• docs/ReleaseHooks.md → docs/Hooks.md, retitled and leading with the general model; the dogfooding motivation moved into the release/post-release section. Generic "release hook" mentions in docs, changelog, and code comments now say just "hooks"; the changelog introduces the mechanism first and release/post-release as its first event.

Everything renamed is unshipped (PublicAPI.Unshipped.txt), so there are no compatibility concerns; user-visible strings (error messages, the Detail log line, the args file path) change accordingly.

Checklist of related issues / discussions

• Fixes #
• Partially fixes #
• Related discussion(s): #

Types of changes

This pull request introduces the following types of changes:

• Bug fix
• New feature
• Test addition / update (no changes to non-test code)
• Refactor (no changes in public API syntax or semantics)
• Performance improvement (no changes in public API syntax or semantics)
• Documentation (docs directory) update
• Dependency addition / update
• Changes to the build scripts
• Changes to CI (workflows, bot / app configurations)
• Other

Other: rename of not-yet-shipped public API (types, constants, method, on-disk path) — a refactor in spirit, but the unshipped API surface does change syntactically, so the "Refactor" box would be inaccurate.

Breaking changes

This pull request introduces breaking changes:

• Yes
• No

Checklist

• For all types of changes:
  • I have read the Code of Conduct and agree to be bound by it
  • I have read the Contributor Guide and agree to follow it
• For code changes only:
  • The project builds on my machine, via the provided build script, with zero warnings
  • I have added tests that prove my feature works / my fix is effective
  • I have added / modified XML documentation according to changes in code
  • I have checked that all the links I added or modified in XML documentation point to their intended destination
• For documentation changes (docs directory) only:
  • I have built and tested documentation locally
  • I have checked that all the links I added or modified point to their intended destination

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.83%. Comparing base (587da50) to head (a02e583).

Files with missing lines	Patch %	Lines
src/Buildvana.Tool/Subcommands/ReleaseCommand.cs	0.00%	6 Missing ⚠️
src/Buildvana.Runtime/PostReleaseHookArgs.cs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
- Coverage   53.86%   53.83%   -0.04%     
==========================================
  Files         151      151              
  Lines        4394     4399       +5     
  Branches      787      787              
==========================================
+ Hits         2367     2368       +1     
- Misses       1927     1931       +4     
  Partials      100      100              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 4m 4s —— View job

---

Review: Recast hooks as general event handlers

• Gather context (read diff, changed files)
• Review code changes for correctness
• Review docs/changelog consistency
• Sweep the tree for missed renames
• Post review

---

Ric, the framing change is a clear win — "context/event/args" reads as a coherent model in a way "command/moment/context" never did, and the Release_PostRelease analogy in docs/Hooks.md earns its place. The mechanical part is almost complete; I found two stale spots and have one API-design call worth making before this ships.

Correctness

No behavioral change found. I checked the things a rename of this shape usually breaks:

• GetHookArgsFile produces .buildvana-temp/hook-args/{context}/{event}.json, matching the changelog, docs/Hooks.md, and DirectoryStructure.md verbatim.
• GlobalSuppressions.cs:29 targets BuildvanaJsonContext.PostReleaseHookArgs — the STJ-generated property name tracks the type name, so that one had to move and did.
• PublicAPI.Unshipped.txt is complete (type, <Clone>$, ctor, all four properties, Equals/GetHashCode/ToString, operators, Load, both consts, GetHookArgsFile, and the ~-prefixed JsonTypeInfo line) and stays ordinally sorted after the rename — Context < Event, HookArgsDirectory < ScratchDirectory, PostReleaseHookArgs() < ProducedPackages.
• ReleaseCommand.cs:247 swapping the "release"/"post-release" literals for PostReleaseHookArgs.Context/.Event is a genuine improvement over the rename: the call site and Load() can no longer drift apart. The wrap is required (the one-liner would be ~162 chars) and follows the parameter-list rule.
• No stale ReleaseHooks.md link anywhere; the Hooks.md#the-hook-args anchor in DirectoryStructure.md:121 matches the retitled ## The hook args heading and its TOC entry.

Two missed renames

1. src/Buildvana.Sdk/Modules/Hooks/Module.targets:7-8 — the SDK-side half of the hook contract still says "hook contexts":

    its own version, so bv, the SDK, and hooks always agree on the shape of configuration and hook
    contexts. An explicitly versioned directive is left alone. [...]

This is the file that pins Buildvana.Runtime for hooks, so it's the one place outside Buildvana.Runtime that describes the same contract — worth having consistent. (It survived my first sweep only because the phrase wraps across the two lines.) Fix this →

2. tests/Buildvana.Tool.Tests/GitServiceTests.cs:57 — GetDirtyFiles_IgnoresScratchDirectory_EvenWhenNotGitignored plants a fixture file named hook-context.json. The name is arbitrary (the test only cares that something under the scratch directory is ignored), so nothing breaks — but it's now the last hook-context string in the tree, and it's exactly the kind of thing that makes a future grep for the old terminology look like there's still work to do. Fix this →

One call to make before this ships

@event in a public signature — WellKnownPaths.GetHookArgsFile(string context, string @event) (src/Buildvana.Runtime/WellKnownPaths.cs:34). It compiles, and CA1716 stays quiet (it targets namespaces, types, and virtual/interface members — not parameters, and not consts on a sealed record, so PostReleaseHookArgs.Event is safe too). But:

• Named-argument callers have to write GetHookArgsFile(context: "release", @event: "post-release").
• This is Buildvana.Runtime's public, additive-only surface. Renaming a parameter after ship is a source-breaking change for those callers, so it's free now and expensive later.
• Both XML docs already say "The name of the context…" / "The name of the event…" — so contextName / eventName would be both more accurate and escape-free.

Your call: keeping context/@event mirrors the path segments exactly, which has its own value. I'd lean eventName, but I don't feel strongly.

Suggestion (optional, fits this PR's thesis)

The hook file path is still built inline — HookRunner.cs:61, Path.Combine(".buildvana", "hooks", context, @event + ".cs") — while the args file path has a WellKnownPaths helper. HookArgsDirectory's own doc comment (WellKnownPaths.cs:21) has to spell out the .buildvana/hooks/{context}/{event}.cs convention in prose precisely because it can't reference it. Since this PR's whole point is that hooks are a general mechanism with more events to come, a sibling WellKnownPaths.GetHookFile(context, event) (plus a HooksDirectory const for CleanBuildCaches at HookRunner.cs:89) would put both halves of the convention in one place — and it's public API that repository-owned tooling could reasonably want. Pre-existing, so entirely fine to defer.

On the Codecov delta

The −0.05% is an artifact, not a regression. The 6 "missing" lines in ReleaseCommand.cs are the same uncovered RunHookAsync call as before, now spread across 5 lines by the required wrap; the partial in PostReleaseHookArgs.cs is the string.IsNullOrEmpty(homeDirectory) ternary, which tests never exercise with the default because hooks-under-test always get an explicit directory. Both pre-date the PR and only surfaced because the files count as changed.

Not verified

I could not run dotnet build / dotnet bv pack in this environment (build commands need approval here), so the zero-warning claim rests on your local run rather than mine. Nothing in the diff looks like it would trip an analyzer, and the two public-API-affecting renames are reflected in PublicAPI.Unshipped.txt, which is where RS0016/RS0017 would otherwise bite.

Also, heads up: my checkout shows .claude/rules/architecture.md modified in the working tree, reverting your hook contexts → hook args edit. That's an artifact of this environment, not of your commit — bf65661 has the correct text.

---

• branch hooks-event-terminology

[rdeago]

Thanks for the review — all four points addressed, one commit each:

• Module.targets "hook contexts" → fixed in 49bb64f: the SDK pin comment now says "the shape of configuration and hook args".
• GitServiceTests fixture hook-context.json → renamed to hook-args.json in 42a4948, so a grep for the old terminology now comes up empty.
• @event in GetHookArgsFile's public signature → keeping it, after weighing the named-argument and post-ship-rename points. The parameters mirror both the path segments and the PostReleaseHookArgs.Context/.Event constants; contextName/eventName would break that symmetry (or force renaming the constants too, which nothing requires). Confirmed by Ric.
• WellKnownPaths.GetHookFile suggestion → taken now rather than deferred, in 926b2ac: HooksDirectory const plus GetHookFile(context, @event), wired into both HookRunner.RunHookAsync and CleanBuildCaches, with matching PublicAPI.Unshipped.txt entries. Doc comments now reference the members instead of spelling the convention out in prose. One deliberate side effect: hook paths in HookRunner messages now use forward slashes on every platform, matching the WellKnownPaths contract, instead of Path.Combine separators.

Sanity gate re-run after the changes: dotnet bv pack green (705/705 tests, 0 warnings, 0 errors, all three packages produced) and ReSharper inspectcode --swea --severity=WARNING reports 0 results.

@claude please take another look.

[claude]

Claude finished @rdeago's task in 4m 15s —— View job

---

Re-review: three follow-up commits

• Read the follow-up commits (49bb64f, 42a4948, 926b2ac)
• Verify the two rename fixes
• Review the new GetHookFile / HooksDirectory public API
• Check PublicAPI.Unshipped.txt and the path-separator side effect
• Sweep the tree for anything still stale

---

Ric, all four points check out, and 926b2ac is better than what I suggested — routing CleanBuildCaches through HooksDirectory too means the hook half of the contract now has exactly one definition. One new finding, on a line this PR already touched.

Verified

49bb64f — Module.targets:7-8 now reads "the shape of configuration and hook args". ✓

42a4948 — fixture renamed. A full-tree sweep for HookContext / hook-context / hook context (all files, case-insensitive) now returns nothing. The single hit left is .claude/rules/architecture.md in my checkout's working tree — the same environment artifact as last time; git show HEAD:.claude/rules/architecture.md says "hook args", so the commit is right.

@event kept — your reasoning is sound, and stronger than I framed it: with PostReleaseHookArgs.Context/.Event as the canonical names, contextName/eventName would make the helper the only place in the contract that spells the two concepts differently. @event at one call site is the cheaper cost.

926b2ac — reviewed as new public API, not just a refactor:

• GetHookFile returns .buildvana/hooks/{context}/{event}.cs, forward slashes, matching the type's "all paths are relative to the home directory and use forward slashes" contract (WellKnownPaths.cs:8).
• PublicAPI.Unshipped.txt has both new lines, and sort -c on the whole file passes in the C locale, so ordinal ordering is intact. Shipped.txt is still empty (1 line), so RS0017 has nothing to say. Parameter renders as string! event, matching the existing GetHookArgsFile line.
• SA1201 is satisfied: all three consts precede both methods.
• Both new members are genuinely covered, and by the good kind of test: HookRunnerTests computes .buildvana/hooks/... independently via Path.Combine (:148, :213) rather than deriving it from the code under test. So a typo in HooksDirectory or GetHookFile fails RunHookAsync_WithHookFile_RunsHookFromHomeDirectory (the File.Exists probe would miss and ran would be false) and CleanBuildCaches_DeletesArtifactsOfEveryHookFileRecursively (artifacts would survive). Nice property to have kept.
• The separator change is safe, not just intended: no test asserts on those two message strings, and Path.GetFullPath(relativePath, base) accepts forward slashes on Windows, so path normalizes exactly as before — only the display text changed.
• No changelog entry needed: CHANGELOG.md:42 already covers "the well-known path constants shared by both sides of the hook contract" generically, and the whole type is unshipped. (Pre-existing nit if you ever revisit that line: it says "constants" while the type also exposes two helper methods.)

One new finding

docs/Hooks.md:117 describes a mechanism that isn't the one implemented. The line says:

bv clean runs dotnet clean on each *.cs file under .buildvana/hooks/ (recursively), clearing its build cache.

HookRunner.CleanBuildCaches (HookRunner.cs:88-111) never invokes dotnet clean. It enumerates **/*.cs under the hooks directory and deletes each file's run-file artifacts directory directly — FileBasedAppHelper.GetArtifactsDirectory, i.e. {temp|LocalAppData}/dotnet/runfile/{name}-{sha256(fullPath.ToUpperInvariant())}.

The claim is pre-existing, but this PR edited the tail of that same sentence ("last hook context included" → "last hook args file included"), so it's a natural place to correct it — and CHANGELOG.md:41 already has the accurate phrasing to borrow: "clears the hooks' file-based-app build caches". It matters a little beyond tidiness: a reader who wants to clear that cache by hand and follows the doc will reach for dotnet clean and not get it. Fix this →

Minor / optional

• .buildvana is now a bare literal in WellKnownPaths. HookArgsDirectory composes from ScratchDirectory, but HooksDirectory = ".buildvana/hooks" spells the segment out — while the same assembly already has BuildvanaConfig.SubdirectoryName = ".buildvana" (BuildvanaConfig-Load.cs:14, private). A public ConfigDirectory = ".buildvana" composed into both would complete the pattern you established, and the config subdirectory is about as well-known as the scratch directory (docs describe it, Sdk.props probes it). Purely additive, so it can land whenever — including never.
• Module.targets wrap. "hook args" is shorter than "hook contexts", so line 8 now starts with a lone args. and the paragraph is unevenly filled. Cosmetic; reflow only if you happen to be in there.
• Discoverability. docs/Hooks.md still describes hook paths purely in prose. Now that HooksDirectory/GetHookFile are public, a clause in "Writing a hook" would tell hook authors the paths are available as API. Optional — the doc reads fine as is.

Not verified

dotnet build and inspectcode are not runnable in this environment (the dotnet build call was denied here), so the zero-warning / zero-inspection claim rests on your local run — bv pack green with 705/705 tests and 0 inspectcode results. Nothing in 926b2ac's diff looks like analyzer bait: the two new public members are documented, in PublicAPI.Unshipped.txt, and member-ordering-clean.

Beyond the docs/Hooks.md:117 line, I have nothing blocking.

--- • branch hooks-event-terminology

[rdeago]

Thanks — all five points taken, one commit each:

• docs/Hooks.md cache-clearing mechanism → fixed in d3c9378: the sentence now says bv clean clears each hook file's build cache by deleting its file-based-app artifacts directory; the dotnet clean claim is gone.
• Bare .buildvana literal → taken in 34593c6, with one deliberate deviation: the const is named BuildvanaDirectory, not ConfigDirectory. The configuration file can also live at the home root, and the directory also holds hooks and repository-supplied Directory.Build files, so DirectoryStructure.md's "grouping directory for Buildvana files" is the identity worth naming. HooksDirectory now composes from it, and BuildvanaConfig.FindFile uses it in place of its private SubdirectoryName const, so the assembly spells the segment exactly once. PublicAPI.Unshipped.txt has the new line, sorted before HookArgsDirectory. Confirmed by Ric, name included.
• Module.targets reflow → 079f85b moves hook down a line; line 8 no longer opens with a lone args..
• Discoverability → 04efb05 adds a sentence to "Writing a hook" pointing hook authors at WellKnownPaths for the directories and the per-hook path helpers.
• Changelog "constants" nit → 5ffde88: the Buildvana.Runtime entry now reads "the well-known paths shared by both sides of the hook contract (directory constants and per-hook path helpers)".

Sanity gate re-run after the changes: dotnet bv pack green (705/705 tests, 0 warnings, 0 errors, all three packages produced) and ReSharper inspectcode --swea --severity=WARNING reports 0 results.

@claude please take another look.

[claude]

Claude finished @rdeago's task in 4m 35s —— View job

---

Re-review: five follow-up commits

• Read the follow-up commits (d3c9378, 34593c6, 079f85b, 04efb05, 5ffde88)
• Verify the docs/Hooks.md cache-clearing correction against HookRunner
• Review the new BuildvanaDirectory const and its two consumers
• Check PublicAPI.Unshipped.txt ordering and the reflow/doc/changelog edits
• Sweep the tree for anything stale

---

Ric, five for five, nothing blocking. One thing I learned while checking 34593c6 is worth passing on, but it's pre-existing and out of scope.

Verified

d3c9378 — docs/Hooks.md:117 now matches CleanBuildCaches (HookRunner.cs:88-111) statement for statement: enumerate **/*.cs under the hooks directory, delete each file's artifacts directory. The method's own XML doc (HookRunner.cs:84-86) already said exactly this, so doc and code now agree in both places. No dotnet clean left anywhere in docs/ or CHANGELOG.md.

34593c6 — BuildvanaDirectory reviewed as new public API:

• HooksDirectory = BuildvanaDirectory + "/hooks" is a compile-time constant expression, so PublicAPI.Unshipped.txt still records the literal ".buildvana/hooks" — unchanged, no API churn beyond the one added line. Whole file passes sort -c in the C locale.
• BuildvanaConfig.FindFile's two subdirectory candidates are already covered by BuildvanaConfigLoadTests.cs:55, which writes a real .buildvana/buildvana.json — so a typo in the const fails a test rather than shipping. Good property for a const that two probes now share.
• BuildvanaDirectory has no slash, so using it inside Path.Combine is correct despite the type's forward-slash contract.
• BuildvanaDirectory is right, and not just defensible: DirectoryStructure.md:33 labels the directory "Optional grouping directory for Buildvana files" — the const's doc comment now repeats that phrase verbatim, so the reference doc and the API say the same thing. ConfigDirectory would have contradicted the very doc it's supposed to name.

079f85b — measured the paragraph: the wrapped lines run 100/101/100 chars, and the reflow takes lines 7-8 from 98/short to 93/98. Moving hook args. up entirely would need ~104, over the paragraph's own ceiling, so this is the best available fill. ✓

04efb05 — accurate on both halves ("hook and args directories" = HooksDirectory/HookArgsDirectory; "per-hook path helpers" = GetHookFile/GetHookArgsFile), and "ship in the package too" picks up the section's opening sentence ("The types a hook needs … ship in the Buildvana.Runtime package") — the antecedent is right there, one screen up.

5ffde88 — "(directory constants and per-hook path helpers)" now covers what the type actually exposes. ✓

Sweep — HookContext / hook-context / hook context, case-insensitive, whole tree: the only hit is .claude/rules/architecture.md in my checkout's working tree, the same environment artifact as the last two rounds. git show HEAD: confirms the commit says "hook args".

Worth knowing: the fold is per-assembly

Your commit message says the assembly spells .buildvana exactly once, and that's exactly true. What I hadn't appreciated until I went looking is that the repository spells the same four-candidate config-file probe four times:

Site	Reaches the const?
src/Buildvana.Runtime/BuildvanaConfig-Load.cs:26-31	✅ now does
src/Buildvana.Sdk.Tasks/Tasks/ComputeVersion-caching.cs:83-84	✅ today — Buildvana.Runtime arrives transitively via Buildvana.Core.Configuration
src/Buildvana.Core.HomeDirectory/HomeDirectoryDiscovery.cs:60-61	needs a ProjectReference; Buildvana.Core.Configuration is precedent
src/Buildvana.Sdk/Sdk/Sdk.props:48-49,81-82	❌ MSBuild, out of reach by construction

These four have to agree or the SDK finds a home directory bv doesn't — the exact drift the const exists to prevent. Entirely pre-existing and not this PR's job; I'm flagging it because the second row costs one identifier and no new dependency. Fix this →

A related rules gap I tripped over on the way there: .claude/rules/architecture.md says a Core-tier project "may depend on other Buildvana.Core.* libraries and ordinary BCL/NuGet dependencies", and spells out the Buildvana.Runtime → Core prohibition — but never says Core → Buildvana.Runtime is allowed, which Buildvana.Core.Configuration.csproj:13 already does. I read the rule as forbidding it and nearly wrote "unreachable" above. Per your own preference for fixing rules over remembering around them: Fix this →

Two doc notes, take or leave

• ScratchDirectory and BuildvanaDirectory now read as a pair they aren't. HooksDirectory = BuildvanaDirectory + "/hooks" and HookArgsDirectory = ScratchDirectory + "/hook-args" sit four lines apart and look like two branches of one tree, but .buildvana-temp is a sibling of .buildvana, not a child — deliberately, since one is checked in and the other is gitignored. Neither doc comment says so, and it's the first question a reader of those two lines will have. Half a sentence on BuildvanaDirectory would settle it.
• WellKnownPaths' class summary is now slightly narrower than its contents: "the well-known paths that make up the contract between bv and repository-owned hooks", while BuildvanaDirectory's own doc is (correctly) written in terms of the configuration file. Nothing misleads — BuildvanaDirectory really is the hooks directory's parent — but "…between bv, the SDK, and repository-owned hooks" would cover both readings.

Not verified

dotnet build / inspectcode remain unrunnable here, so the zero-warning claim is still yours — bv pack green, 705/705, 0 inspectcode results. Nothing in these five commits is analyzer bait: one added const (documented, in PublicAPI.Unshipped.txt, ordered with its siblings), one removed private const with its last use rewritten, and three prose edits.

Ship it.
• branch hooks-event-terminology

[rdeago]

Thanks — all four taken, and the fold went one step further than asked:

• Remaining .buildvana literals → 6729794: ComputeVersion's fingerprint probe and HomeDirectoryDiscovery now compose their candidates from WellKnownPaths.BuildvanaDirectory (Buildvana.Core.HomeDirectory gains the Buildvana.Runtime reference; Core.Configuration was precedent). Since the directory segment was only half the drift protection, BuildvanaConfig.JsonFileName/JsoncFileName are now public API too, so all three C# probe sites spell nothing locally. Sdk.props stays a hand-mirror by construction, fenced by the existing MUST-match warning in HomeDirectoryDiscovery. Two new PublicAPI.Unshipped.txt lines and a changelog touch cover the additions.
• architecture.md gap → fff43f1: the Core-tier bullet now allows Buildvana.Runtime (whose BCL-only closure keeps the tier host-agnostic), and the Runtime bullet states the prohibition is one-directional.
• Sibling clarification → a3e584a: BuildvanaDirectory's doc now says the scratch directory is deliberately a sibling, not a child — one is checked in, the other is gitignored.
• Class summary → a02e583: "…between bv, the SDK, and repository-owned hooks".

Sanity gate re-run after the changes: dotnet bv pack green (705/705 tests, 0 warnings, 0 errors, all three packages produced) and ReSharper inspectcode --swea --severity=WARNING reports 0 results.

Closing the loop here — thanks for three rounds of genuinely useful findings.