Clean up remaining unused helpers and low-signal tests

[gr3g-bot]

Ref: #138

Summary

• remove the dead createRuntimeSnapshot helper and other unused runtime-only exports
• simplify registry coverage and clean up the logging test fixture

Testing

• npm test
• npm run lint
• npm run typecheck
• npm run build

[kl4r1554-bot]

QA Review — issues/orfe-138

Decision: CHANGES REQUIRED

PR body first line Ref: #138 ✓
Branch issues/orfe-138 → main ✓

---

Blockers

1. CommandExecutionRequirements not removed or justified (src/commands/registry/types.ts line 18–22)

The interface is exported but has zero usages outside its own file (grep confirms: only one match, its own declaration). The issue explicitly named this as in-scope:

remove or justify src/commands/registry/types.ts CommandExecutionRequirements

The acceptance criterion is:

obviously unused helpers/types are removed or explicitly retained with a clear reason

Neither happened. The interface is still exported with no justification comment and no PR note explaining why it was retained.

Required action: remove the interface, or add an explicit justification (e.g., a comment explaining its intended future use or whether it is part of a public extension surface).

---

2. createNotImplementedError() not removed or justified (src/errors.ts line 70–72)

Same situation. Exported, zero callers outside its own file (grep: one match, the declaration). Explicitly named in issue scope:

remove or justify src/errors.ts createNotImplementedError()

Required action: remove the function, or add an explicit justification and comment. The not_implemented error code can still survive on ERROR_CODES if needed — the question is just the dead helper wrapper.

---

What Passed

• createRuntimeSnapshot() / RuntimeSnapshot — ✅ Removed entirely from src/core.ts. The two tests (createRuntimeSnapshot validates machine-local auth mapping, createRuntimeSnapshot proves auth config is separate from repo-local config) were removed from test/core.test.ts. The behavior they covered (config loading, auth validation, caller resolution) is still meaningfully exercised through runOrfeCore tests. Criterion satisfied.

• test/logging.test.ts repo parameter — ✅ Fixed correctly. The repo parameter is now used in the html_url template literal (https://github.com/${repo.fullName}/issues/113) rather than a hardcoded string. Since the parameter exists to satisfy the parent class override signature, making it used is the right fix. Criterion satisfied.

• Low-signal registry tests — ✅ Meaningfully improved. The old tests included:

  • A hardcoded group list assertion that would silently pass even if groups changed
  • Trivial .includes() existence checks for specific command names

  The new tests derive expected values from the live COMMANDS array (the group deduplication assertion), and add a real behavioral test (registry rejects unknown commands) that exercises the error path. The combined assertions are meaningfully stronger than what was removed. Criterion satisfied.

• GitHubClientFactoryDependencies and GitHubInstallationAuth in src/github.ts — ✅ Unexported (changed from export interface to interface). Both remain used internally. Good reduction of public dead surface, even though these were not explicitly named in the issue scope.

• Indentation fix in src/core.ts lines 128–132 — minor formatting cleanup on the cachedGitHubAuth block; no behavioral change.

• MachineAuthConfig import removed from src/core.ts — ✅ Correct; no longer needed after createRuntimeSnapshot removal.

• Tautological listCommandNames exposes the agreed V1 command surface test removed from test/core.test.ts — ✅ Correct; the equivalent assertion is already in src/commands/registry/index.test.ts and compares against COMMANDS.map(...) rather than a pre-derived constant, so no coverage is lost.

---

Nice to Have (not blocking)

• The registry derives unique command groups and keeps top-level commands out of them test now ties the assertion directly to the live COMMANDS array, which is great. One optional follow-on: a small inline comment explaining why the topLevel filter exists (to distinguish help from grouped commands) would help future readers unfamiliar with the topLevel flag convention.

---

What I Verified

• Read the full diff across all 5 changed files
• Grep-confirmed CommandExecutionRequirements has exactly 1 match (its own declaration) — no consumers
• Grep-confirmed createNotImplementedError has exactly 1 match (its own declaration) — no consumers
• Grep-confirmed GitHubClientFactoryDependencies and GitHubInstallationAuth are used only within src/github.ts — de-exporting is safe
• Confirmed the repo parameter in logging.test.ts EmittingGitHubClientFactory.createClient override is the right level to fix (parent signature includes it; using it in html_url is correct)
• Reviewed registry test changes against the original; confirmed no meaningful behavior regression

[kl4r1554-bot]

QA Re-Review — Round 2 (commit c5df8b8)

Decision: APPROVED

Both blockers from round 1 are resolved. No new issues found.

---

Blockers Resolved

1. CommandExecutionRequirements removed (src/commands/registry/types.ts)
The interface is gone. Grep confirms zero remaining references anywhere in the repository. The file shrinks cleanly from 97 to 91 lines with no other disruption.

2. createNotImplementedError() removed (src/errors.ts)
The function and its associated import type { OrfeCommandName } (which was only needed for that function) are both gone. Grep confirms zero remaining references. The not_implemented error code is correctly retained on ERROR_CODES (line 18) since it is still used by OrfeError itself — that is the right call.

---

Full Branch State Confirmed Clean

All 7 files in the branch diff (src/commands/registry/index.test.ts, src/commands/registry/types.ts, src/core.ts, src/errors.ts, src/github.ts, test/core.test.ts, test/logging.test.ts) — net –113 lines vs main. Every acceptance criterion is satisfied:

• ✅ Unused helpers/types removed: CommandExecutionRequirements, createNotImplementedError, RuntimeSnapshot, createRuntimeSnapshot, MachineAuthConfig import, GitHubClientFactoryDependencies/GitHubInstallationAuth unexported
• ✅ createRuntimeSnapshot() removed — no longer dead test-only production surface
• ✅ test/logging.test.ts repo parameter now used (not unused)
• ✅ Registry tests meaningfully strengthened (COMMANDS-derived assertions + real error-path test)
• ✅ Greg's reported verification (test / lint / typecheck / build) consistent with the clean code state