fix: [ENG-2881] tidy oclif client cache after brv update; rename hook…

[leehpham]

… to after-update.

Summary

• Problem:
• Why it matters:
• What changed:
• What did NOT change (scope boundary):

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes #
• Related #

Root cause (bug fixes only, otherwise write N/A)

• Root cause:
• Why this was not caught earlier:

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
• Key scenario(s) covered:

User-visible changes

List user-visible changes (including defaults, config, or CLI output).
If none, write None.

Evidence

Attach at least one:

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable)
• No breaking changes (or clearly documented above)
• Branch is up to date with main

Risks and mitigations

List real risks for this PR. If none, write None.

• Risk:
  • Mitigation:

[github-actions]

Claude finished @leehpham's task in 6m 31s —— View job

---

PR Review — fix: [ENG-2881] tidy oclif client cache after brv update; rename hook to after-update

• Read CLAUDE.md and understand repo conventions
• Read changed files (hook + tests)
• Analyze diff vs origin/main
• Review for correctness, security, code quality
• Post inline comments for any issues
• Post final summary with score

---

Summary

Clean, well-scoped refactor: renames restart-after-update → after-update (file + tests + package.json hook path), and adds a tidyUpdatePluginClientCache step to the same hook so the oclif plugin-client cache (<dataDir>/client/<version>-<sha>) doesn't accumulate stale version dirs across updates. Existing restart behavior is preserved verbatim — handleRestartAfterUpdate is unchanged.

Strengths

• praise: Hook is purely a thin composition over two pure, dependency-injected functions — easy to unit-test, easy to reason about. Matches the project's outside-in / testable style well.
• praise: Test coverage is genuinely thorough — symlink semantics (relative, dangling, regular-file-instead-of-symlink), missing root, single active version, and a partial-failure case where one rm throws but the others still proceed. Injectable rmFn for the failure path is a clean touch.
• praise: Conservative safety posture — if current can't be resolved to a real candidate, the function returns without deleting anything. That's the right default ("when in doubt, don't delete").
• praise: Error handling is appropriately best-effort: readdir/rm failures are logged via logFn and don't throw, so a cache-tidy failure can never break the update command itself.
• praise: Conforms to repo conventions — type for DTO shapes, ?? for nullish defaults, no as / any / !, .js import extensions in tests, no console logging from the hook.

Issues / questions

• question (correctness): tidyUpdatePluginClientCache runs unconditionally on update, but handleRestartAfterUpdate skips for --autoupdate. That asymmetry is probably intentional (cache cleanup is safe to do for background auto-updates too), but worth confirming and either documenting or aligning. See inline comment.
• suggestion (safety): Active-version resolution relies on basename(readlink(...)). Works for today's oclif-plugin-update layout, but a realpath + parent-equals-root check would harden against future plugin layout changes / absolute symlinks. See inline comment.
• nit (style): Empty line at top of tidyUpdatePluginClientCache body + deps destructured twice — easy consolidation.
• nit (perf, micro): specialCacheEntries Set can be hoisted to module scope (it's a fixed two-entry set).
• nit (test): Prefer fs.mkdtemp over Date.now() + Math.random() for tmp-dir naming.

Correctness notes I did not flag as issues but worth being aware of

• On Linux/macOS, deleting the currently-running CLI's own version directory mid-process is fine (open file handles stay valid). On Windows the rm will fail with EBUSY and is silently logged — the orphan dir will be cleaned up on the next update. Acceptable.
• Concurrent brv update runs are prevented by oclif-plugin-update's own file locking, so tidy can't race with an active install.

Security

No new attack surface. spawn('brv restart', {shell: true}) is unchanged from main. All filesystem paths are join(root, name) where name comes from readdir(root), so no traversal vector.

---

Overall score: 4 / 5

Solid, well-tested PR with a small surface area. Drop to addressing the autoupdate semantics question and the resolver hardening would push it to 5/5; the rest are nits.
• Branch: fix/ENG-2881