Skip to content

refactor: rename local workflow service factory#212

Merged
devkade merged 1 commit into
devfrom
refactor/issue-209-local-workflow-service-factory
May 17, 2026
Merged

refactor: rename local workflow service factory#212
devkade merged 1 commit into
devfrom
refactor/issue-209-local-workflow-service-factory

Conversation

@devkade
Copy link
Copy Markdown
Owner

@devkade devkade commented May 17, 2026

Summary

  • Renames the local service factory export from createLocalKapiService to createLocalWorkflowService.
  • Updates Pi presentation wiring and service factory tests to use the semantic factory name.
  • Adds an architecture guard so the old factory export name does not re-enter the implementation.

Linked issue

Refs #209

Problem

Issue #209 is removing historical kapi product-name leakage from reusable internal code while preserving intentional compatibility/product surfaces.

After PR #211 moved the service/factory files to semantic workflow-service filenames, the local factory export still used the product-prefixed name:

createLocalKapiService()

That name describes a generic local workflow service factory, not an external kapi-agent integration or serialized workflow contract.

Options considered

  1. Rename only the local factory export.
    • Pros: tiny slice, low compatibility risk, keeps service class/store renames separate.
    • Cons: KapiService, KapiStore, and registry type names remain for later slices.
  2. Rename KapiService and factory export together.
    • Pros: more complete semantic service boundary.
    • Cons: larger diff, many more call sites, and higher risk of mixing constructor/API compatibility with this simple factory alias cleanup.
  3. Leave the export name until the whole service boundary is renamed.
    • Pros: no churn now.
    • Cons: keeps an easy-to-remove internal product-name leak after the file rename slice already made the factory boundary semantic.

Selected approach

Selected option 1.

This PR only renames the local factory export and its direct consumers. The service class/store/type names stay intentionally unchanged for later bounded slices.

Implementation by file/surface

  • src/adapters/workflow-service-factory.ts
    • createLocalKapiServicecreateLocalWorkflowService.
  • src/presentation/pi-extension.ts
    • Imports and invokes the semantic factory name.
  • test/presentation-command-behavior.test.ts
    • Updates presentation behavior setup to use createLocalWorkflowService.
  • test/deep-interview-child-rpc.test.ts
    • Updates child RPC service setup to use createLocalWorkflowService.
  • test/architecture.test.ts
    • Adds a direct guard proving the old factory export is absent and the semantic export is present.
  • docs/product-name-audit.md
    • Records that the local factory export has moved while remaining core service/store names are follow-up work.

Why this fixes it

The active implementation no longer exports or imports createLocalKapiService. The remaining old-name references are limited to documentation and a negative architecture assertion, while runtime code uses createLocalWorkflowService.

This is still a partial #209 slice, so this PR uses Refs #209 rather than Closes #209.

QA / Verification

  • npm test -- test/architecture.test.ts test/presentation-command-behavior.test.ts test/deep-interview-child-rpc.test.ts — pass; package script ran the full suite, 520 tests, 509 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.

Residual active-source scan:

rg -n 'createLocalKapiService' src
# no matches

Changed-line count:

git diff --shortstat origin/dev...HEAD
6 files changed, 17 insertions(+), 10 deletions(-)

Independent review:

  • Static/security scan — no hardcoded secrets, shell injection keywords, eval/exec, unsafe deserialization, or SQL injection findings.
  • Independent reviewer subagent — passed; no blocking logic/security issues.

Anomalies observed

Symptom Evidence Impact Action
Targeted-looking npm test -- <files> runs the full suite because the package script is tsx --test test/*.test.ts. Test output showed 520 tests. QA was broader than the named files; runtime cost only. Reported exact scope above.
src/cli/kapi-review-cli.ts executable-bit churn recurred during verification. git status showed mode-only churn before cleanup. Could add unrelated mode churn. Reset mode before staging; no mode-only churn committed.

Risks / Follow-up

  • Exported names such as KapiService, KapiStore, KapiRegistryEntry, and KapiAgentPrReviewState remain for later Remove remaining kapi identifiers from internal codebase #209 slices.
  • Human-facing test descriptions that mention “local Kapi service” remain non-blocking until the broader service class rename slice.
  • kapi-review, kapi-agent, /kapi-*, and serialized kapi-* workflow IDs remain intentional compatibility/external integration surfaces.

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.

@devkade
Copy link
Copy Markdown
Owner Author

devkade commented May 17, 2026

@kapi-agent review

Revision explanation for current head 29d15f7979cec802d4d65cfbd333400e4cbca5af:

What changed:

  • Renamed the local service factory export from createLocalKapiService to createLocalWorkflowService.
  • Updated Pi presentation wiring and tests that instantiate the local service factory.
  • Added an architecture guard proving the old factory export name is absent from the implementation.
  • Updated the product-name audit to record this bounded Remove remaining kapi identifiers from internal codebase #209 slice.

Why this closes the prior feedback:

  • This is a small follow-up after the service filename rename slice, focused only on the local factory export boundary.
  • KapiService, KapiStore, registry names, kapi-agent, kapi-review, and /kapi-* serialized workflow IDs are intentionally left for later or external/compatibility surfaces.
  • Active source has no createLocalKapiService matches; old-name mentions remain only in docs and the negative architecture assertion.

Verification:

  • npm test -- test/architecture.test.ts test/presentation-command-behavior.test.ts test/deep-interview-child-rpc.test.ts — pass; package script ran the full suite, 520 tests, 509 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.
  • Independent reviewer subagent — passed.

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 bounded refactor for issue #209. The semantic scope is intentionally narrow: rename the internal local workflow service factory export from createLocalKapiService to createLocalWorkflowService, update direct consumers, and add a guard against reintroducing the old export name. There was no prior kapi-agent review on this head, so no ping-pong fixes were required.

What changed

  • src/adapters/workflow-service-factory.ts now exports createLocalWorkflowService.
  • src/presentation/pi-extension.ts imports and calls the new factory name.
  • Tests using the local service factory were updated.
  • test/architecture.test.ts now guards against the old export name.
  • docs/product-name-audit.md records this as a completed bounded slice while leaving remaining product-prefixed service/store names for follow-up work.

Why this is correct

The change is a mechanical internal export rename with direct call-site updates. It does not alter factory construction behavior, persistence contracts, serialized workflow IDs, public CLI names, or GitHub integration surfaces. The added architecture test makes the intended boundary explicit and helps prevent regression.

Evidence

  • Verifier gate: PASS.
  • Size gate: PASS, 27 changed lines under the semantic review threshold.
  • Revision-explanation status: not required; found.
  • Ilchul review harness gate: PASS, neutral/comment, no blocking findings.
  • npm ci && npm run verify passed.
  • Inspected source references confirm no active src usage of createLocalKapiService; remaining mentions are documentation and the negative architecture assertion.

Remaining risks and approval rationale

Remaining risk is low. Existing product-prefixed identifiers such as KapiService, KapiStore, and related serialized/external Kapi surfaces remain intentionally out of scope. Approval is justified because the PR is narrowly scoped, tested, documented, and does not appear to change runtime behavior beyond the internal factory export name.

Blocking issues

None.

Warnings / risks

  • The review workspace showed mode-only churn on src/cli/kapi-review-cli.ts after verification. This appears consistent with the PR’s noted existing verification anomaly rather than this diff’s semantic change, but it remains worth cleaning up separately so local verification does not leave tracked files dirty.

Suggestions

  • Consider a follow-up issue or small PR to prevent verification/setup scripts from toggling the executable bit on src/cli/kapi-review-cli.ts.

Looks good

  • The rename is applied consistently across production and test call sites.
  • The architecture guard is appropriate for preventing the old factory export name from returning.
  • Documentation accurately frames this as a partial product-name cleanup slice, not a broad compatibility migration.

Verification notes

Verifier gate status: PASS. Size gate status: PASS. Revision-explanation status: not required; found. PR evidence reports npm ci && npm run verify passed, including tests, type checks, unused checks, and quality budgets; Ilchul harness reported no blocking findings.


Engine: pi

@devkade devkade merged commit 2654250 into dev May 17, 2026
2 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