Reorganize tests by runtime surface and move test-only artifacts out of production paths

[gr3g-bot]

Ref: #150

Summary

• split mixed-surface command tests into explicit core, tool, and CLI files where applicable
• move test-only GitHub mocks and cross-cutting template tests out of production paths and update testing guidance

Testing

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

[kl4r1554-bot]

QA Review — PR #155 / Issue #150

Decision: APPROVED

---

What I verified

• PR body first line: Ref: #150 ✅
• Branch: issues/orfe-150 ✅
• Commit under review: 512f49c
• src/commands/**/mocks/** — no files remain ✅
• All handler.test.ts files removed and replaced by surface-specific files ✅
• test/wrapper/ — correctly absent (removed in a prior issue) ✅
• test/support/github/{auth,issue,pr,project}.ts — all four created ✅
• src/templates.test.ts removed; replaced by test/templates/runtime.test.ts ✅
• tsconfig.json include updated from test/**/*.test.ts → test/**/*.ts (required to type-check support files like test/support/github/*.ts) ✅
• docs/architecture/overview.md updated with an explicit runtime-surface test convention ✅
• 314 tests pass, 63 test files (npm test) ✅
• Build clean (npm run build) ✅
• Typecheck clean (npm run typecheck) ✅
• Lint clean (npm run lint) ✅
• No production source files modified ✅
• issue set-state has core.test.ts + cli.test.ts only — correctly explained (no tool coverage existed to move) ✅

---

Blockers

None.

---

Important

1. definition.test.ts files were not created for the split commands.

The issue scope lists the expected split as definition.test.ts, core.test.ts, tool.test.ts, cli.test.ts. The implementation delivered the last three but no new definition.test.ts files were created for any of the split commands (auth/token, issue/*, pr/*, project/*). The only definition.test.ts in the repo is the pre-existing src/commands/help/definition.test.ts, which predates this issue and still mixes surface concerns in a single file.

Practically:

• The acceptance criteria do not explicitly require definition.test.ts files.
• Line counts in the split files account for roughly all lines removed from the old handler.test.ts files, so no behavioral coverage appears lost.
• The docs update (overview.md line 218) now documents what definition.test.ts is for, so future contributors will know where to add slice-local definition/validation tests.

Recommendation: Create a follow-up issue to add definition.test.ts for each command slice (testing option shapes, validation hooks, validInputExample, etc.). This is not a re-open of #150 — it's the next natural step once contributors start following the documented convention.

2. docs/orfe/spec.md section 13.1 uses stale "wrapper" terminology and does not mention the surface-split convention.

Line 1669 still says "contract tests for wrapper/core separation". The "wrapper" → "opencode" rename happened in earlier issues and that stale ref was not addressed here. The surface-split pattern (core/tool/cli) is also not listed in section 13.

This is pre-existing drift, not introduced by this PR. It was not in scope for #150 (which limited docs impact to overview-level testing conventions). Recommend a follow-up issue to align spec.md section 13 with current terminology and practices.

---

Nice to have

• src/commands/help/definition.test.ts mixes core, tool, and CLI tests in a single file — consistent with the old pre-split pattern. Applying the same surface split to help would make it consistent with all other commands. Out of scope for #150.

---

Docs / invariants

• docs/architecture/invariants.md — no invariants were changed by this PR; no updates needed. ✅
• docs/architecture/overview.md — updated correctly. The single-line expansion at line 218 is clear and actionable. ✅
• ADR — correctly omitted; test organization restructure does not constitute a new architecture decision. ✅
• Debt — no new debt introduced; pre-existing spec.md drift noted above as a follow-up item. ✅

[kl4r1554-bot]

Follow-up QA Review — commit d9f979a

This is the second review pass for PR #155 / issue #150, following up on the Important and Nice-to-have findings from the first round.

Decision: APPROVED — all prior findings resolved.

---

Prior findings and their resolution

Important #1 — definition.test.ts files not created for the reorganized command slices
✅ Resolved. 17 new definition.test.ts files now exist, one for each command slice reorganized under #150:

• auth/token
• issue/{get,create,comment,update,set-state,validate}
• pr/{get,get-or-create,comment,reply,submit-review,update,validate}
• project/{get-status,set-status}

Quality of the new tests is good. They are focused, purposeful, and non-mechanical:

• assertDefinitionIdentity verifies name, group, leaf, and execution — the full command contract identity.
• assertOption spot-checks the options that matter for each command (required flags, enums, multi-value types).
• assertValidInputExample runs the real validateCommandInput path, ensuring the example the definition ships is itself valid — a useful regression safeguard.
• Commands with bespoke validation rules (e.g., issue set-state duplicate constraints, pr submit-review event enum) have dedicated test blocks that exercise those validation paths directly.
• Commands with notable contract semantics (issue validate being auth-free, auth token requiring repo) have those properties explicitly asserted.

The shared test/support/definition-test.ts helper is tight and well-typed. It avoids over-generalizing while still eliminating boilerplate across all 17 files.

Important #2 — stale "wrapper" terminology and missing surface-split convention in docs/orfe/spec.md
✅ Resolved. The spec.md diff is comprehensive:

• All "wrapper" occurrences replaced with "tool entrypoint" or "OpenCode tool/plugin entrypoint" consistently throughout sections 2, 4, 5, 9, 10, 12, and 13.
• Section 13.1 now explicitly documents:
  • definition.test.ts for slice-local metadata and validation coverage
  • core.test.ts, tool.test.ts, cli.test.ts as the runtime-surface split convention
  • "contract tests for OpenCode tool/core separation" replacing the stale "wrapper/core" wording

Nice-to-have — mixed concerns in src/commands/help/definition.test.ts
✅ Resolved. The help command is now fully split into four focused files:

• definition.test.ts (57 lines) — definition identity, option shape, help-data construction, error case
• core.test.ts (37 lines) — runOrfeCore root help and targeted help
• tool.test.ts (22 lines) — executeOrfeTool targeted help
• cli.test.ts (79 lines) — runCli root help, targeted help, representative commands, usage error formatting

This is the cleanest result — the help command now follows the exact same structural pattern as every other command slice in the repo.

---

Verification (follow-up commit d9f979a)

Check	Result
npm test	✅ 338 tests, 82 test files, all pass
npm run build	✅ Clean
npm run typecheck	✅ Clean
npm run lint	✅ Clean
Production source files changed	✅ None — test and docs only

Test count increase: 314 → 338 tests (+24), 63 → 82 test files (+19). All new tests are in definition.test.ts and the help surface-split files.

---

Blockers

None.

Remaining findings

None. All findings from the first review round are fully resolved. No new issues introduced.