refactor: unify RunContract prompt rendering#131
Conversation
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
- Size gate failed: this PR changes 251 lines, which is above the 200-line maximum for kapi-agent approval.
- Please split this into multiple PRs, each under 200 changed lines and scoped to one semantic unit.
- A likely split would be:
- Add the shared
renderRunContractPromptdomain renderer plus focused unit tests. - Route
KapiService.buildWorkflowPromptthrough the renderer and verify prompt equivalence. - Route CLI runtime prompt rendering through the renderer and verify CLI/runtime behavior.
- Add the shared
Because this gate fails, I cannot approve this revision even if the implementation direction looks reasonable.
Warnings / risks
src/cli/kapi-cli.ts: changing unknown workflow handling from fallback prompt construction to throwing may be correct, but it is a behavior change. The split PR should make this explicit and include coverage if intentional.- Prompt rendering refactors are regression-prone because small formatting changes can break workflow/artifact contracts or tests that compare exact prompt text. Keep each semantic change isolated with snapshot/equivalence tests.
Suggestions
- In the follow-up PRs, include before/after prompt equivalence tests for both:
- service workflow prompt rendering
- runtime CLI prompt rendering
- Consider making renderer layout contracts explicit in tests so workflow-only concerns and runtime-only concerns do not leak across layers.
Looks good
- The direction of extracting prompt rendering into
src/domain/run-contract-prompt-renderer.tsis maintainable and should reduce duplicated contract text. - Routing both service and CLI prompt construction through one renderer is a good way to preserve artifact/workflow contract consistency, provided the split PRs keep exact behavior verified.
Verification notes
- Verification gate status: FAIL — size gate failed because this PR has 251 changed lines, exceeding the 200-line limit.
- Size gate status: FAIL — 251 changed lines >= 200.
- Revision-explanation status: not required for this first kapi-agent review; found status is true.
- Claimed author verification:
npm run verify, targeted tests,npm run check,npm run check:unused, andgit diff --check, but approval is blocked by the size gate regardless.
Engine: pi
fd6f9fe to
d1e115e
Compare
|
@kapi-agent review Revision explanation for current head What changed
Why this closes the prior feedback
Verification
|
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
- Revision explanation gate failed. This PR was updated after a prior kapi-agent review, but there is no author comment or PR-body update explaining what changed and why.
- Please add a review response explaining the revision since the prior
CHANGES_REQUESTEDreview, especially how the PR was reduced from 251 changed lines to 159 and what semantic scope remains. - After that, request re-review.
- Please add a review response explaining the revision since the prior
Warnings / risks
- None blocking from the inspected code. Prompt rendering changes are inherently regression-prone, but this revision has focused equivalence/runtime coverage.
Suggestions
- Consider documenting in the PR response whether any behavior changed around unknown workflow prompt fallback versus known workflow rendering, since that was called out in the prior review.
Looks good
src/domain/run-contract-prompt-renderer.tscleanly centralizes workflow/runtime prompt layout without pulling in adapter-specific GitHub/Ragna/Discord semantics.src/application/kapi-service.tsnow delegates workflow prompt construction to the shared renderer while preserving Ralph-specific reviewer guidance and resume notes.src/cli/kapi-cli.tsroutes runtime prompt construction through the same renderer while keeping GitHub issue prompt context appended outside the domain renderer.test/run-contract-prompt-renderer.test.tscovers service prompt equivalence and asserts runtime prompts stay free of adapter-specific semantics.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 159 changed lines, under the 200-line limit.
- Revision-explanation status: FAIL — prior kapi-agent review exists, revision explanation is required, and none was found.
- Local inspection covered:
src/domain/run-contract-prompt-renderer.tssrc/application/kapi-service.tssrc/cli/kapi-cli.tstest/run-contract-prompt-renderer.test.ts
Engine: pi
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the prior feedback:
Verification:
|
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the prior feedback:
Verification:
|
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the prior feedback:
Verification:
|
d1e115e to
9f588f6
Compare
There was a problem hiding this comment.
kapi-agent review
Verdict: APPROVE
Final approval summary
Review journey
The original goal was to unify RunContract prompt rendering so workflow prompts and runtime prompts share one domain-level renderer instead of duplicating prompt layout logic. The semantic scope is now bounded to that renderer extraction plus the two call-site migrations and focused regression coverage.
Earlier kapi-agent reviews requested changes because the PR exceeded the 200-line size gate, then because the revised PR lacked a visible current-head revision explanation. The current revision addresses both: it is under the size gate and includes a current-head explanation of what changed and why.
What changed
- Added
src/domain/run-contract-prompt-renderer.tswithrenderRunContractPrompt. - Routed
KapiService.buildWorkflowPromptthrough the shared renderer. - Routed CLI runtime prompt construction through the shared renderer.
- Preserved unknown-workflow fallback text for runtime prompts.
- Added
test/run-contract-prompt-renderer.test.tscovering service prompt equivalence and runtime prompt rendering boundaries.
Why this is correct
The renderer centralizes shared prompt structure while keeping adapter-specific GitHub/Ragna/Discord semantics outside the core renderer. The service path still injects project context, worker capability summary, Ralph reviewer guidance, resume notes, and workflow summary state. The runtime path still includes runtime metadata and appends GitHub issue prompt context externally, preserving the intended adapter boundary.
The prior unknown-workflow behavior concern is addressed by allowing fallback command/title/contract/completion-gate values when no workflow definition is found.
Evidence
- Verifier gate: PASS — provided evidence shows
npm ci && npm run verifyexited 0. - Size gate: PASS — 159 changed lines, below the 200-line limit.
- Revision-explanation status: PASS — current-head revision explanation is present.
- Local inspection covered:
src/domain/run-contract-prompt-renderer.tssrc/application/kapi-service.tssrc/application/service-domain.tssrc/cli/kapi-cli.tstest/run-contract-prompt-renderer.test.ts
- Local targeted verification also passed for the renderer test path.
Remaining risks and approval rationale
Remaining risk is low and mainly formatting-related: prompt renderers are contract-sensitive, so future edits should keep equivalence/format tests close to call-site changes. This revision is small, tested, preserves the called-out fallback behavior, and maintains adapter/domain separation sufficiently for approval.
Blocking issues
- None.
Warnings / risks
- Prompt text remains a contract surface. Future changes to
src/domain/run-contract-prompt-renderer.tsshould continue using exact/equivalence coverage because small formatting changes can affect workflow/runtime behavior.
Suggestions
- Consider adding a golden-string assertion for the unknown-workflow runtime fallback in a future PR if that path is important operationally.
Looks good
src/domain/run-contract-prompt-renderer.tscleanly factors the shared RunContract layout without importing adapter-specific GitHub/Ragna/Discord concerns.src/application/kapi-service.tspreserves workflow-specific inputs while removing duplicated prompt assembly.src/cli/kapi-cli.tskeeps GitHub issue context appended outside the renderer, preserving the adapter boundary.- Tests cover both service equivalence and runtime renderer boundary expectations.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifypassed in the provided gate evidence. - Size gate: PASS — 159 changed lines < 200.
- Revision-explanation status: PASS — required explanation found for the current head.
- Additional local inspection found no blocking correctness, security, workflow-contract, or maintainability issues.
Engine: pi
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the current gate:
Verification after alignment:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: APPROVE
Final approval summary
Review journey
The original goal was to unify duplicated RunContract prompt rendering behind a shared domain renderer. The semantic scope is now limited to extracting renderRunContractPrompt, routing service/runtime prompt generation through it, and adding focused regression coverage.
Earlier reviews requested changes for the 200-line size gate and then for missing revision explanation. Those gates are now satisfied.
What changed
- Added
src/domain/run-contract-prompt-renderer.ts. - Routed
KapiService.buildWorkflowPromptthrough the shared renderer. - Routed CLI runtime prompt construction through the shared renderer.
- Re-exported the renderer via
src/application/service-domain.ts. - Added
test/run-contract-prompt-renderer.test.ts.
Why this is correct
The renderer centralizes shared prompt structure while keeping workflow and runtime layout differences explicit. It preserves unknown workflow fallback behavior, keeps runtime adapter-specific context outside the core renderer, and preserves Ralph-specific reviewer guidance via explicit additional rules.
Evidence
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 159 changed lines, below the 200-line limit.
- Revision-explanation status: found.
- Inspected files:
src/domain/run-contract-prompt-renderer.ts,src/application/kapi-service.ts,src/application/service-domain.ts,src/cli/kapi-cli.ts, andtest/run-contract-prompt-renderer.test.ts.
Remaining risks and approval rationale
Remaining risk is low and mainly formatting-related: prompt layout is contract-sensitive, so future changes should continue using exact/equivalence tests. Approval is justified because the PR is small enough, verifier passed, prior blocking feedback is addressed, and no blocking correctness/security/workflow-contract issue remains.
Blocking issues
- None.
Warnings / risks
src/domain/run-contract-prompt-renderer.ts: prompt rendering remains regression-sensitive because small formatting changes can affect workflow behavior. Current focused tests reduce this risk.
Suggestions
- Consider adding a future snapshot/golden test if more prompt layouts are added, so accidental whitespace or section-order changes are easier to detect.
Looks good
- The domain renderer does not embed GitHub/Ragna/Discord adapter semantics.
- Workflow and runtime layouts are explicit instead of hidden in call sites.
- Unknown workflow fallback text is preserved.
- Ralph reviewer guidance remains a call-site-specific addition rather than hard-coded into the generic renderer.
- Regression coverage checks both service equivalence and runtime renderer boundaries.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 159 changed lines < 200.
- Revision-explanation status: found.
- CI/local evidence includes full verify run with tests, type/check, unused checks, and quality budgets passing.
Engine: pi
Summary
renderRunContractPromptdomain renderer for workflow and runtime prompt layouts.KapiService.buildWorkflowPromptand CLI runtime prompt dispatch through the shared renderer.Revision explanation for current head
9f588f6173e8121a30824b8f80160baee61e2124What changed
Why this closes the prior feedback
Verification
npm run verifypassed.npm test -- test/run-contract-prompt-renderer.test.ts && npm run check && npm run check:unusedpassed.git diff --checkpassed.Closes #115