Skip to content

chore: correct stale keeperhub/ paths in messages that tell a reader where to look - #2409

Open
subheeksh5599 wants to merge 2 commits into
KeeperHub:stagingfrom
subheeksh5599:chore/stale-keeperhub-paths
Open

chore: correct stale keeperhub/ paths in messages that tell a reader where to look#2409
subheeksh5599 wants to merge 2 commits into
KeeperHub:stagingfrom
subheeksh5599:chore/stale-keeperhub-paths

Conversation

@subheeksh5599

Copy link
Copy Markdown
Contributor

Three user-facing strings name paths that do not exist in this tree, so a reader following them looks somewhere that is not there.

There is no keeperhub/ directory: ec2c17735 ("refactor: KEEP-1561 consolidate remaining keeperhub/ dirs into top-level") moved those directories to the top level. package.json is not named keeperhub, and the only tsconfig path alias is @/* -> ./*, so nothing resolves through a keeperhub/ prefix. No import in the tree uses one.

What changes

file was now
plugins/safe/index.ts:59 Ensure keeperhub/protocols is imported before keeperhub/plugins/safe Ensure @/protocols is imported before @/plugins/safe
plugins/cowswap/index.ts:211 the same, for cowswap the same
scripts/token-audit.js:387 See keeperhub/specs/design-system/tokens.css See specs/design-system/tokens.css
scripts/test-release.sh (7 lines) ./keeperhub/scripts/test-release.sh ./scripts/test-release.sh

The two plugin strings are the registry-miss error, the one place a contributor is told how to fix that failure. The module that actually registers each protocol at import time is the @/protocols barrel (protocols/index.ts, "Imports all protocol definitions and registers them at import time"), reached as import "@/protocols" at 21 call sites including plugins/protocol/index.ts. The old text pointed at a directory that has not existed for months.

token-audit.js is the one I would single out: CLAUDE.md:165 tells contributors to run node scripts/token-audit.js before committing UI changes, and the line printed on every run that finds an error sent them to a spec file at the wrong path. It now prints the path that exists.

Scope

Message text only. No behaviour, no resolver, no registry, no scan change. git diff --stat is 4 files, 10 lines.

Deliberately not touched, because each is correct as written: the @keeperhub/* package names, the KeeperHub/keeperhub URLs, the keeperhub/tap/kh brew tap, the /eks/techops-*/keeperhub/... SSM parameter paths, ~/.keeperhub/wallet.json, and the hyphenated keeperhub-events/ keeperhub-executor/ keeperhub-metrics-collector/ keeperhub-scheduler/ directories. The specs/ and lib/metrics/METRICS_REFERENCE.md occurrences are documentation and are left alone too.

Also not folded in: the same script's SCAN_DIRS and SKIP_PATTERNS still carry three and four entries for paths that no longer exist. Those are functional config rather than message text, and correcting them changes which files the audit scans, so they belong in their own change. For the record I measured it: with them corrected the audit reports the same 2 errors and 164 warnings across one fewer file.

Verification

  • node scripts/token-audit.js -> 745 files scanned, 2 errors, 164 warnings, unchanged, and the closing line now names a path that exists
  • bash scripts/test-release.sh --help -> prints ./scripts/test-release.sh examples that run
  • npx vitest run tests/unit/safe-pending-transactions-projection.test.ts -> 2 passed
  • no test pins any of the changed strings (git grep for each, over tests/)

chore: per ISSUES.md's "not required" list, which names comment and error-message wording.

…where to look

Three user-facing strings still name paths that do not exist in this tree. There
is no keeperhub/ directory (KEEP-1561 consolidated those dirs into the top level,
`ec2c17735`), package.json is not named keeperhub, and the only tsconfig path
alias is `@/*` -> `./*`, so nothing resolves through a keeperhub/ prefix and no
import in the tree uses one.

- plugins/safe/index.ts and plugins/cowswap/index.ts: the registry-miss error told
  a reader to import `keeperhub/protocols`, which cannot resolve. The module that
  actually registers each protocol at import time is the `@/protocols` barrel
  (protocols/index.ts), used at 21 call sites including plugins/protocol/index.ts.
- scripts/token-audit.js: the closing line printed on every run with errors
  pointed at `keeperhub/specs/design-system/tokens.css`; the file is at
  specs/design-system/tokens.css. CLAUDE.md tells contributors to run this before
  committing UI changes, so the wrong path was printed often.
- scripts/test-release.sh: the usage block printed `./keeperhub/scripts/...` as
  copy-pasteable examples. The script is at scripts/test-release.sh.

Message text only: no behaviour, no resolver, no registry change. Verified the
audit still reports the same 745 files, 2 errors and 164 warnings, and that
test-release.sh --help now prints invocations that work. No test pins any of the
changed strings.

Left alone deliberately: the specs/ and lib/metrics/METRICS_REFERENCE.md
occurrences (documentation), the `@keeperhub/*` package names, the
KeeperHub/keeperhub URLs, the SSM parameter paths and the `keeperhub/<service>`
directory names, all of which are correct as written. The token-audit SCAN_DIRS
and SKIP_PATTERNS also still carry three and four stale entries, but those are
functional config rather than message text and correcting them changes what the
script scans, so they are not folded in here.
@github-actions

Copy link
Copy Markdown
Contributor

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

@suisuss suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this changes

Ten single-line string replacements across four files. plugins/cowswap/index.ts:211 and plugins/safe/index.ts:59 swap keeperhub/protocols and keeperhub/plugins/<name> for @/protocols and @/plugins/<name> inside their throw new Error(...) literals; scripts/test-release.sh corrects seven occurrences of ./keeperhub/scripts/test-release.sh; scripts/token-audit.js:387 corrects the tokens.css path in its failure epilogue.

I checked both sides of every line against staging: every old path 404s, every new path resolves, and keeperhub/ is not a top-level tree. No guard, condition or control flow moves - the plugin strings are throw arguments with the modules' real imports on separate static lines above, the shell strings sit in a comment block and a single-quoted heredoc, and nothing anywhere imports a keeperhub/-prefixed specifier, so none of these were resolvable shorthand.

Using @/protocols rather than stripping the prefix is the better fix: tsconfig.json maps @/* to ./*, both files already import through that alias, and the advice the message gives is now literally executable.

The risk scan flagged this as touching an auth path. That is the filename token-audit.js matching on token - the file is a CSS design-token linter with no auth, session or network in it. Recording it so the flag is not re-raised.

Does it match the description

Matches.

Blocking

  • scripts/token-audit.js:32-34 - SCAN_DIRS is ["app", "components", "keeperhub/components", "keeperhub/app", "keeperhub/api"], and getAllFiles(join(ROOT, "keeperhub/components")) resolves to a directory that does not exist, so readdirSync throws into the bare catch {} and returns []. -> Three of the five configured scan directories are silently dead, so the auditor reports clean over a third of what it claims to cover. This diff corrects that file's error message and leaves its configuration broken, which is the one place where fixing the prose rather than the path has a cost. -> Drop the three keeperhub/ entries.

Mechanical - actionable as-is

  • scripts/token-audit.js:48-54 - four SKIP_PATTERNS entries carry the same prefix and are matched with fullPath.includes(p) against an absolute path. They work today only because the checkout directory is itself named keeperhub, so .../keeperhub/lib/palette.ts happens to contain keeperhub/lib/palette.ts. Rename the checkout and four exemptions start firing. keeperhub/api/mcp/schemas/route.ts is doubly stale - the real path is app/api/mcp/schemas/route.ts, so stripping the prefix alone would still miss.

  • README.md:262 is [Metrics Reference](keeperhub/lib/metrics/METRICS_REFERENCE.md), a broken link on the repo front page. #2385 fixes exactly that line and is approved, so leave it - flagging only so the two changes do not collide.

With the team

Nothing.

Verdict

Changes requested on SCAN_DIRS - every line this diff touches is correct, and the file it touches last has three dead scan directories that the corrected message sits directly beneath.

Two things outside this change, for whoever scopes the follow-up. The residue is much larger than the two plugin strings and five spec files I described when I raised this: roughly 169 source-path-shaped keeperhub/ references across 28 files on staging, 18 of them under specs/. And prefix-stripping is not a safe blanket fix there - several targets are stale in both forms, for example keeperhub/lib/abi-cache.ts where lib/abi-cache.ts is also gone, and keeperhub/db/schema-extensions.ts whose real path is lib/db/schema-extensions.ts. Each target needs checking rather than rewriting.

Separately, and not yours: .github/workflows/maintainability.yml:109 greps for from.*[@/]keeperhub/ across app/ components/ lib/ plugins/ to warn about unmarked fork code. I ran it verbatim against staging - zero matches. The convention it enforces was abandoned and the check was never retired, so it has been a no-op. Same rot, sitting in CI rather than prose.

@suisuss suisuss added the changes-requested Triage: reviewed, changes needed from the contributor label Sep 12, 2026
…emptions

Three of the five SCAN_DIRS entries name directories that do not exist, and
getAllFiles swallows the readdirSync throw into a bare catch, so a third of the
configured scan surface has been silently empty.

The four SKIP_PATTERNS entries were live only by accident: they are matched with
fullPath.includes(pattern) against an absolute path, so "keeperhub/lib/palette.ts"
happened to match because the checkout directory is named keeperhub. Measured on
a copy of app/ and components/ in a directory without that name, the unpatched
script reports 747 files and 16 errors against 745 files and 2 errors inside a
keeperhub-named checkout. The 14 extra errors are the four exemptions failing to
apply. Patched, both checkouts report 744 files, 2 errors and 164 warnings, so the
result no longer depends on what the clone directory is called.

keeperhub/api/mcp/schemas/route.ts was stale in both halves: the real path is
app/api/mcp/schemas/route.ts, so stripping the prefix alone would still miss it.
@subheeksh5599

Copy link
Copy Markdown
Contributor Author

Both items fixed, pushed as 1f43078fc.

Blocking: the three dead SCAN_DIRS. Dropped. app and components already cover everything the three keeperhub/ entries were meant to reach, including keeperhub/api, whose real home is app/api.

Mechanical: the four SKIP_PATTERNS. Corrected, including the one that was stale in both halves (keeperhub/api/mcp/schemas/route.ts to app/api/mcp/schemas/route.ts).

You were right that they were live only by coincidence, and it is worth having the number rather than the argument. I copied app/ and components/ into a directory whose path contains no keeperhub and ran both versions:

checkout script result
~/kh-work/keeperhub before 745 files, 2 errors, 164 warnings
directory without keeperhub in the path before 747 files, 16 errors, 164 warnings
directory without keeperhub in the path after 744 files, 2 errors, 164 warnings

So the unpatched script reports 14 phantom errors the moment the clone is called anything else, and the patched script reports the same thing in both places. The audit result no longer depends on the directory name. In this checkout it now reads 744 files scanned, 2 errors, 164 warnings with the corrected epilogue.

node --check passes. scripts/ is outside biome's configured paths, so there is no formatter on this file either way.

On your inventory correction. Taken, and it changes how that follow-up should be scoped: roughly 169 references across 28 files, 18 under specs/, and targets like keeperhub/lib/abi-cache.ts and keeperhub/db/schema-extensions.ts that are stale in both forms. That is a check-each-target job, not a prefix strip, and it is not in this pull request.

On maintainability.yml:109. Agreed it is the same rot in a different medium, and a check that has been a no-op since the KEEP-1561 move is arguably worse than the prose, because prose does not run in CI and claim to have passed. Not mine to retire, but flagging that I agree with the read rather than leaving it hanging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Triage: reviewed, changes needed from the contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants