Skip to content

refactor: rename PR review state helpers#214

Merged
devkade merged 1 commit into
devfrom
refactor/issue-209-pr-review-state-names
May 17, 2026
Merged

refactor: rename PR review state helpers#214
devkade merged 1 commit into
devfrom
refactor/issue-209-pr-review-state-names

Conversation

@devkade
Copy link
Copy Markdown
Owner

@devkade devkade commented May 17, 2026

Summary

  • Renames the internal PR review state surface from KapiAgentPrReview* to generic PullRequestReview* names.
  • Renames internal review-state fields from latestKapiReview / kapiAgentReviewCheckConclusion to latestAgentReview / reviewCheckConclusion while preserving literal kapi-agent text where it names the external reviewer bot.
  • Updates CLI event/report tests and GitHub RunContract adapter tests to use the generic internal review-state shape.

Linked issue

Refs #209

Problem

Issue #209 is removing legacy kapi product-name leakage from reusable internal implementation identifiers. The PR-review event/report surface still used KapiAgentPrReviewState, KapiAgentPrReviewView, formatKapiAgentPrReview, latestKapiReview, and kapiAgentReviewCheckConclusion as internal names.

That mixed an external integration name (kapi-agent, which is still the real GitHub App/reviewer name) with reusable internal PR-review state concepts that should be generic.

External reviewer literal: kapi-agent
Internal state/helper names: pull request / agent review / review check

Options considered

  1. Rename only the exported type names — change KapiAgentPrReviewState and KapiAgentPrReviewView.
    • Pros: smallest diff.
    • Cons: leaves latestKapiReview and kapiAgentReviewCheckConclusion as reusable internal field names.
  2. Rename the type, helper, and internal fields together — use PullRequestReviewState, PullRequestReviewView, formatPullRequestReview, latestAgentReview, and reviewCheckConclusion.
    • Pros: removes the whole small internal PR-review naming cluster while keeping literal external kapi-agent strings intact.
    • Cons: touches the JSON object shape used by supervisor report/event internals and therefore requires test updates.
  3. Broaden the slice into all PR/review adapter wording — also rename event type strings and user-facing report labels.
    • Pros: larger residual-count reduction.
    • Cons: risks changing intentional external contracts such as kapi.pr.* event names and kapi-agent reviewer labels outside this small slice.

Selected approach

Selected option 2.

This keeps the slice small and semantic: internal state/helper names become generic, while external compatibility/event names and actual GitHub App labels remain untouched. It deliberately avoids changing kapi.pr.* event type contracts or the visible kapi-agent PR review report label in this PR.

Implementation

  • src/cli/worker-events.ts
    • Renamed KapiAgentPrReviewState to PullRequestReviewState.
    • Renamed review-state fields to latestAgentReview and reviewCheckConclusion.
    • Preserved kapi-agent event names and reviewer literals as external integration contracts.
  • src/cli/runctl-cli.ts
    • Updated PR review inspection/normalization return types and field serialization to the new generic names.
  • src/presentation/runctl-formatters.ts
    • Renamed KapiAgentPrReviewView to PullRequestReviewView and formatKapiAgentPrReview to formatPullRequestReview.
    • Kept human report text that explicitly identifies the external kapi-agent reviewer.
  • src/application/github-run-contract-adapter.ts
    • Updated GitHub adapter review-state field reads to the generic names.
  • test/cli-args.test.ts, test/cli-worker-events.test.ts, test/github-run-contract-adapter.test.ts
    • Updated fixtures/assertions for the generic review-state object shape.

Why this fixes it

The old internal PR-review naming cluster no longer contains KapiAgent* type/helper names or latestKapiReview / kapiAgentReviewCheckConclusion field names. Remaining kapi-agent references in this touched surface now denote the external reviewer bot, check name, or event contract rather than reusable implementation vocabulary.

Residual scan for this slice:

rg -n 'KapiAgentPrReviewState|KapiAgentPrReviewView|formatKapiAgentPrReview|latestKapiReview|kapiAgentReviewCheckConclusion|latestKapi\b' src test
# no matches

Changed-line size: 74 total changed lines (37 insertions + 37 deletions).

QA / Verification

  • npm test -- test/cli-worker-events.test.ts test/github-run-contract-adapter.test.ts test/cli-args.test.ts — pass; package script ran the full test/*.test.ts suite plus repeated args: 521 tests, 510 pass, 11 skipped.
  • npm run check — pass.
  • npm run check:unused — pass.
  • npm run quality:budgets — pass with existing non-failing warning: code_smells=52.
  • git diff --check — pass.
  • rg -n 'KapiAgentPrReviewState|KapiAgentPrReviewView|formatKapiAgentPrReview|latestKapiReview|kapiAgentReviewCheckConclusion|latestKapi\b' src test — no matches.

Manual smoke, if applicable:

  • n/a — this is a focused internal rename slice covered by CLI/report/event tests and typecheck.

Anomalies observed

Symptom Evidence Impact Action
Package test script ignores targeted-looking file args and runs the full suite. npm test -- test/cli-worker-events.test.ts test/github-run-contract-adapter.test.ts test/cli-args.test.ts invoked tsx --test test/*.test.ts ...; 521 tests ran. Longer than a targeted run, but stronger coverage. Reported here; no code action.
Test/check commands flipped src/cli/kapi-review-cli.ts executable bit locally. git diff -- src/cli/kapi-review-cli.ts showed only old mode 100644 / new mode 100755. Unrelated mode-only churn would pollute the PR. Reset mode to 100644 before staging.
quality:budgets still warns on code_smells=52. npm run quality:budgets output. Existing non-failing budget warning; unchanged by this slice. No action in this rename PR.

Risks / Follow-up

  • This PR intentionally does not rename larger residual surfaces such as KapiRegistryEntry, KapiService, or KapiStore; those remain follow-up slices under Remove remaining kapi identifiers from internal codebase #209.
  • This PR intentionally preserves kapi-agent strings and kapi.pr.* event names where they are external integration/contract identifiers.

kapi-agent review

  • Expected reviewer-only bot behavior: automatic formal review after PR open; if absent after a short wait, request with @kapi-agent review.
  • Merge gate: latest kapi-agent approval/check must apply to the current head before merge.
  • Re-review rule: after stale approval, REQUEST_CHANGES, or COMMENT, request re-review with a same-comment current-head revision explanation:
    • @kapi-agent review
    • Revision explanation for current head <HEAD_SHA>
    • What changed
    • Why this closes the prior feedback
    • Verification

@devkade
Copy link
Copy Markdown
Owner Author

devkade commented May 17, 2026

@kapi-agent review

Revision explanation for current head 49a877b51b302c7a12087d4d0614e5d4c893871e:

What changed:

  • Renamed the internal PR review state type/view/helper from KapiAgentPrReview* to PullRequestReview* / formatPullRequestReview.
  • Renamed internal review-state fields from latestKapiReview and kapiAgentReviewCheckConclusion to latestAgentReview and reviewCheckConclusion.
  • Updated CLI event/report and GitHub RunContract adapter tests.

Why this closes the prior feedback:

  • This removes the small internal reusable KapiAgent* PR-review naming cluster while preserving literal kapi-agent strings only where they refer to the external reviewer bot/check/event contract.
  • Residual scan for the renamed cluster returns no matches in src or test.

Verification:

  • npm test -- test/cli-worker-events.test.ts test/github-run-contract-adapter.test.ts test/cli-args.test.ts — pass; package script ran 521 tests / 510 pass / 11 skipped.
  • npm run check — pass.
  • npm run check:unused — pass.
  • npm run quality:budgets — pass with existing non-failing code_smells=52 warning.
  • git diff --check — pass.

Copy link
Copy Markdown

@kapi-agent kapi-agent Bot left a comment

Choose a reason for hiding this comment

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

kapi-agent review

Verdict: APPROVE

Final approval summary

Review journey

This PR addresses issue #209 by removing a focused internal naming cluster that leaked KapiAgent* terminology into reusable PR-review state helpers. The semantic scope is narrow: it renames internal PR review types, helper names, and JSON field names while preserving external contract strings such as kapi-agent, kapi-agent/review, and kapi.pr.* event names. There was no prior kapi-agent review on this PR, so no feedback ping-pong was required.

What changed

The current revision updates:

  • src/cli/worker-events.ts
  • src/cli/runctl-cli.ts
  • src/presentation/runctl-formatters.ts
  • src/application/github-run-contract-adapter.ts
  • Related tests in test/cli-args.test.ts, test/cli-worker-events.test.ts, and test/github-run-contract-adapter.test.ts

The internal surface now uses PullRequestReviewState, PullRequestReviewView, formatPullRequestReview, latestAgentReview, and reviewCheckConclusion.

Why this is correct

The rename is internally consistent across state production, event payloads, report formatting, and GitHub workflow adapter mapping. External reviewer identity and workflow/event contract strings remain unchanged where they appear to be observable behavior, so the change avoids unnecessary compatibility risk while fixing the targeted implementation-name leakage.

Evidence

  • Verifier gate: PASS.
  • Size gate: PASS, 74 changed lines under the semantic review threshold.
  • Revision-explanation status: not required; author explanation present in the PR body.
  • Ilchul review harness gate: PASS, neutral/comment result with no blocking findings.
  • Verified evidence includes npm ci && npm run verify passing, which ran tests, type/check gates, unused checks, and quality budgets.
  • I also inspected the PR diff and searched for the renamed legacy symbols across src and test; no old targeted names remained.

Remaining risks and approval rationale

Remaining risk is low and limited to consumers of the internal JSON/report shape expecting the old field names. The PR body explicitly frames these as internal supervisor report/event internals, and the relevant tests were updated. Approval is justified because the scope is focused, verification passed, external strings/contracts were preserved, and I found no blocking correctness or safety issue.

Blocking issues

None.

Warnings / risks

  • pr_review payload shape changes from latestKapiReview / kapiAgentReviewCheckConclusion to latestAgentReview / reviewCheckConclusion. If any out-of-repo consumer parses these internal JSON fields, it will need to be updated. This appears intentional and covered by tests.

Suggestions

  • Consider documenting the internal pr_review JSON shape if it is consumed by tooling outside this repository.

Looks good

  • The rename is consistent across producer, formatter, adapter, and tests.
  • External contract identifiers such as kapi-agent, kapi-agent/review, and kapi.pr.* were intentionally preserved.
  • The residual search for the targeted legacy names came back clean.

Verification notes

  • Verifier gate status: PASS — npm ci && npm run verify exited 0.
  • Size gate status: PASS — 74 changed lines, below the 200-line semantic threshold.
  • Revision-explanation status: not required; explanation found.
  • Ilchul review harness: PASS with no blocking findings.

Engine: pi

Copy link
Copy Markdown

@kapi-agent kapi-agent Bot left a comment

Choose a reason for hiding this comment

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

kapi-agent review

Verdict: APPROVE

Final approval summary

Review journey

This PR is a focused refactor for issue #209. The semantic scope is narrow: rename the internal PR review state/helper surface away from KapiAgent*-specific names while preserving external contract literals such as kapi-agent, kapi-agent/review, and kapi.pr.* event names. A prior kapi-agent approval already exists on this same current head, and the revision explanation is present.

What changed

The PR renames internal state/view/helper fields and types across:

  • src/cli/worker-events.ts
  • src/cli/runctl-cli.ts
  • src/presentation/runctl-formatters.ts
  • src/application/github-run-contract-adapter.ts
  • Related tests in test/cli-args.test.ts, test/cli-worker-events.test.ts, and test/github-run-contract-adapter.test.ts

Key internal names now use PullRequestReviewState, PullRequestReviewView, latestAgentReview, and reviewCheckConclusion.

Why this is correct

The change is mechanically consistent across production code and tests. The renamed fields flow from PR inspection, through event payloads and reports, into the GitHub workflow adapter without changing the external reviewer identity or event/check contracts. Residual scans show the targeted legacy internal names are gone from src and test.

Evidence

Verifier gate: PASS. npm ci && npm run verify exited 0.
Size gate: PASS, 74 changed lines under the 200-line semantic review threshold.
Revision-explanation status: not required now, but present.
Ilchul review harness gate: PASS with neutral/comment output and no blocking findings.
I inspected the relevant changed files and ran targeted searches for the renamed legacy symbols.

Remaining risks and approval rationale

Remaining risk is low and limited to consumers expecting the old internal JSON field names. The PR body explicitly frames these as supervisor report/event internals, and tests were updated to cover the new shape. Approval is justified because the scope is bounded, verification passes, no blocking correctness or contract issue remains, and external kapi-agent contracts are preserved.

Blocking issues

None.

Warnings / risks

  • pr_review payload internals now use latestAgentReview / reviewCheckConclusion; any out-of-tree consumer of the old internal JSON shape would need to update. This appears intentional and covered by tests.

Suggestions

  • Consider documenting this internal event/report shape as non-stable if external consumers are not intended to rely on it.

Looks good

  • External contract strings are preserved while internal reusable names are generalized.
  • The rename is consistent across inspection, formatting, event emission, adapter mapping, and tests.
  • The residual scan for the targeted old identifiers is clean.

Verification notes

Verifier gate: PASS — npm ci && npm run verify exited 0.
Size gate: PASS — 74 changed lines.
Revision-explanation status: not required now; found.
Ilchul harness: PASS — no blocking findings.
Local inspection included git show of changed files and rg checks for the retired internal names.


Engine: pi

@devkade devkade merged commit f153dd7 into dev May 17, 2026
3 checks passed
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.

1 participant