Skip to content

refactor(action): extract runReview from index.ts and test it (#169 slice 1) - #278

Merged
mmcky merged 2 commits into
mainfrom
refactor/169-extract-run-review
Aug 19, 2026
Merged

refactor(action): extract runReview from index.ts and test it (#169 slice 1)#278
mmcky merged 2 commits into
mainfrom
refactor/169-extract-run-review

Conversation

@mmcky

@mmcky mmcky commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

First slice of #169, the W1 gate. Deliberately the smallest one that proves the pattern: runReview was named in the issue's plan as the starting block because it changed twice in the last two releases.

The problem this starts on

src/index.ts is 1,579 lines (1,314 when the audit measured it — it has grown 265 lines since) with no usable exports, a bare run() at module scope, and import.meta.url at line 38. Importing any part of it executes the whole Action, so no test has ever reached it — verified again today: nothing under src/ imports it, and its one export (fetchBibliographies) has no importer anywhere. It is simultaneously the highest-churn file in the repo (24 commits in six months) and the only code that ships in dist-action/.

What moved

runReviewsrc/action/review.ts, with the glossary directory passed in as an argument. That argument is the whole trick: the import.meta.url derivation moves to src/runtime-paths.ts — the one module tests never import — and the entry point calls getBuiltInGlossaryDir() at the call site, so the extracted runner loads cleanly under Jest's CJS registry (F123). detectTargetLanguage becomes a pure function of the repo name instead of reading github.context, and the coreLogger adapter moves to src/action/core-logger.ts so the entry point and the extracted runners share one copy rather than growing a second. Sync and rebase keep their behaviour and now call the same glossary-path helper instead of repeating the path.join(__dirname, '..', 'glossary') incantation three times.

src/index.ts loses 124 lines and both the path and url imports.

Tests

17 new tests — the first ever written against entry-point logic: language detection (including the near-misses that must not match — three-letter suffixes, digits, uppercase), glossary resolution through the shared loader with the empty-string-to-undefined mapping for the custom path, the no-detectable-language warning path, reviewer construction and reviewPR argument threading, every action output including the shadow-gate would-auto-merge conditional and the #164 usage outputs, and error propagation to the shim's catch. Suite goes 1,516 → 1,533, all green, lint and format clean, and check-dev-refs passes.

Why this is safe to land

The extraction is semantics-preserving by construction, and the claim is checked rather than asserted:

  • The set of core.setOutput calls in dist-action/index.js is identical before and after (diffed against a bundle built from the parent commit); the size delta is 261 bytes of module-wrapper overhead.
  • action.yml's entry contract is untouched — still dist-action/run.cjs.
  • import.meta.url resolves through the same esbuild banner as before. Both the old and new bundles derive it from the bundle's own __filename, so ../glossary still resolves to the repo-root glossary/ — worth stating explicitly, because a silent mis-resolution here would be exactly the success-shaped failure the tracker is about.

The module map in docs/developer/architecture.md gains the three new modules — its completeness guard (#168) caught their absence, which is the guard doing its job.

What this does not do

The remaining F<n> findings on #169 stay open: the two divergent FileToSync[] builders (F44, now at :381 and :1038), the error coercion (F36), the metadata.mode dispatch (F19) and the tri-state metadata parser (F139, F140). Those land in the following slices, with src/github-content.ts next. No behaviour change ships here, so nothing about the release gate changes.

Part of #169 · gates W1 (#259)

🤖 Generated with Claude Code

…lice 1)

First slice of #169: the Action entry point had no usable exports, a bare run() at module scope and import.meta.url at line 38, so importing any part of it executed the whole Action — 1,579 lines of mode routing, PR-file classification and rebase branch surgery reachable by zero tests while being the highest-churn file in src/.

runReview moves to src/action/review.ts with the glossary directory passed in as an argument, which is what makes it loadable under Jest's CJS registry: the import.meta.url derivation moves to src/runtime-paths.ts, the one module the tests never import, and the entry point calls getBuiltInGlossaryDir() at the call site. detectTargetLanguage becomes a pure function of the repo name rather than reading github.context, and the coreLogger adapter moves to src/action/core-logger.ts so both the entry point and the extracted runners share one copy. Sync and rebase keep their behaviour and now call the same helper for the glossary path.

17 new tests, the first ever written against entry-point logic: language detection, glossary resolution through the shared loader with the custom-path mapping, the no-language warning path, reviewer construction and reviewPR argument threading, every action output including the shadow-gate conditional, and error propagation to the shim's catch.

Semantics-preserving by construction and checked against the bundle: the set of core.setOutput calls is byte-identical before and after, action.yml's entry contract is untouched, and import.meta.url resolves through the same esbuild banner as before (both derive from the bundle's own __filename, so ../glossary still resolves to the repo-root glossary).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 19, 2026 04:15

Copilot AI 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.

Pull request overview

Refactors the GitHub Action entry point by extracting the REVIEW-mode runner into an importable module, enabling direct Jest coverage of previously untestable entry-point logic while keeping runtime-only import.meta.url usage isolated.

Changes:

  • Extracts runReview (and detectTargetLanguage) from src/index.ts into src/action/review.ts, threading the built-in glossary directory as an explicit argument.
  • Centralizes Action runtime glossary-path derivation in src/runtime-paths.ts and reuses it from sync/rebase code paths.
  • Adds a focused Jest suite covering review-mode behavior (language detection, glossary loading, reviewer wiring, outputs, and error propagation) and updates the module map docs.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/runtime-paths.ts Introduces Action runtime path derivation helpers (module dir + built-in glossary dir).
src/index.ts Removes inline REVIEW runner and import.meta.url path logic; delegates to extracted runner and shared helpers.
src/action/review.ts New exported REVIEW-mode runner + pure detectTargetLanguage, extracted for testability.
src/action/core-logger.ts New shared @actions/coreLogger adapter used by action runners.
src/action/tests/review.test.ts Adds first direct tests for extracted entry-point review logic and outputs.
docs/developer/architecture.md Updates module map to include the new Action/runtime modules.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/runtime-paths.ts Outdated
Copilot's review caught that runtime-paths.ts overclaimed: it said it was the one module allowed to contain import.meta.url, but src/cli/index.ts holds its own copy, deliberately and for the same CJS-registry reason. Narrowed to the Action side, with the CLI's parallel case named so neither reads as a duplicate to consolidate.

Verifying that claim surfaced two comments this PR had itself invalidated. rebase-siblings.ts explained that it lives outside index.ts because index.ts uses import.meta.url — true when written, false as of the previous commit; the conclusion survives for the real reasons (run() at module scope, no usable exports), which is what the comment now gives. sync-orchestrator.ts described formatGlossaryTerms as living beyond the reach of tests in runReview, in the present tense, when the new suite covers exactly that wiring.

Comments only: dist-action/index.js is byte-identical (esbuild strips comments), so the semantics argument on this PR is unaffected; only the source map moves.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Aug 19, 2026
The checklist said slice 1 was unreviewed; Copilot has since reviewed it and its comment is addressed in 05ce0ce, along with two comments the extraction had made false elsewhere. Carries the standing lesson forward: every slice invalidates prose in other files, so grep for explanations of whatever moved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mmcky
mmcky merged commit 228a317 into main Aug 19, 2026
1 check passed
@mmcky
mmcky deleted the refactor/169-extract-run-review branch August 19, 2026 04:32
mmcky added a commit that referenced this pull request Aug 19, 2026
…me checklist

Records the 2026-08-19 session end: the #276 repairs merged and byte-verified (mismatch set zero), #169 slice 1 open as #278 with the pattern it establishes for later slices, and the #169 body figures re-verified against main. The Next section becomes an ordered resume-here checklist — merge #278, then slice 2 (github-content.ts, branched from main after the squash-merge), then the rest of #169 — plus the stale-dist cli-smoke gotcha that makes a bare npm test look broken.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Aug 19, 2026
The checklist said slice 1 was unreviewed; Copilot has since reviewed it and its comment is addressed in 05ce0ce, along with two comments the extraction had made false elsewhere. Carries the standing lesson forward: every slice invalidates prose in other files, so grep for explanations of whatever moved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Aug 19, 2026
Copilot flagged that slice 1 appeared under both In flight and Recently landed, with the latter pointing back at the former — right about the duplication, but #278 merged (228a317) between the review and the fix, which inverts which copy survives: slice 1 is no longer in flight.

In flight now covers #169 the work item — what remains and the pattern each later slice follows, including the grep-for-stale-comments lesson slice 1 produced. Recently landed carries a self-contained slice-1 bullet with no back-pointer. The completed checklist item goes, slice 2 becomes step 1 and gains the concrete rename-path divergence it has to resolve, and the 1,533/65 figure moves onto main where it now belongs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky added a commit that referenced this pull request Aug 19, 2026
…me checklist (#279)

* dev: STATE.md — #276 fallout closed out, #169 slice 1 in flight, resume checklist

Records the 2026-08-19 session end: the #276 repairs merged and byte-verified (mismatch set zero), #169 slice 1 open as #278 with the pattern it establishes for later slices, and the #169 body figures re-verified against main. The Next section becomes an ordered resume-here checklist — merge #278, then slice 2 (github-content.ts, branched from main after the squash-merge), then the rest of #169 — plus the stale-dist cli-smoke gotcha that makes a bare npm test look broken.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* dev: STATE.md — record #278's Copilot round in the resume checklist

The checklist said slice 1 was unreviewed; Copilot has since reviewed it and its comment is addressed in 05ce0ce, along with two comments the extraction had made false elsewhere. Carries the standing lesson forward: every slice invalidates prose in other files, so grep for explanations of whatever moved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* dev: STATE.md — slice 1 has landed; split the two entries by tense

Copilot flagged that slice 1 appeared under both In flight and Recently landed, with the latter pointing back at the former — right about the duplication, but #278 merged (228a317) between the review and the fix, which inverts which copy survives: slice 1 is no longer in flight.

In flight now covers #169 the work item — what remains and the pattern each later slice follows, including the grep-for-stale-comments lesson slice 1 produced. Recently landed carries a self-contained slice-1 bullet with no back-pointer. The completed checklist item goes, slice 2 becomes step 1 and gains the concrete rename-path divergence it has to resolve, and the 1,533/65 figure moves onto main where it now belongs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants