feat: add RunContract quality dimensions#130
Conversation
|
@kapi-agent review Please review current head |
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
-
src/domain/run-contract.ts:dimensionsToHints()drops all passing dimensions, which removes the previous positiveevidence-readinesshint when required evidence is satisfied. Before this PR, a completed/evidence-backed workflow produced anokscoring hint:id: "evidence-readiness"level: "ok"- reason explaining evidence expectations are satisfied
The PR body says existing compact quality hints are preserved/derived for current presentation surfaces, but the implementation now returns
quality.hints: []for the ready case. The updated test intest/run-contract.test.tsalso stops asserting the old hint and checks the dimension instead, so this regression is not covered.Please preserve the existing compact hint contract while adding dimensions. For example, derive an
okevidence-readinesshint from the passing Evidence dimension, or otherwise keep the prior ready-state hint behavior if downstream presentation surfaces rely on it.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown", but the new dimension model cannot currently produce an unknown hint/status. That may be intentional, but it is a contract change worth making explicit or testing if any callers distinguish unknown from attention.src/domain/run-contract.ts: multiple failing/warning dimensions can now produce multiple compact hints with new IDs likealignment-dimensionandsimplicity-dimension. That is probably acceptable, but presentation consumers that assume a small fixed hint ID set may need compatibility consideration.
Suggestions
- Add a regression test for the completed/evidence-backed case asserting that
quality.hintsstill includes the compactevidence-readinessokhint. - Consider documenting whether
quality.dimensionsis the richer canonical advisory surface andquality.hintsis a backward-compatible compact projection.
Looks good
- The five requested dimensions are added in the domain layer without importing GitHub/Kapi-specific presentation concerns.
- Completion authority remains in
summarizeCompletion()/ completion criteria rather than being driven by advisory dimensions. - The new tests cover warning/failing dimension behavior and the separation between advisory quality and completion status.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 134 changed lines, under the 200-line limit.
- Revision-explanation status: not required for this first kapi-agent review; explanation was present in the PR body.
- I inspected
src/domain/run-contract.tsand the providedtest/run-contract.test.tsdiff/content.
Engine: pi
b911372 to
0dee370
Compare
Review responsePrior blocker
What changed
Why changed
Verification
@kapi-agent review |
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
REQUEST_CHANGESreview, but the PR body or author comments do not explain what changed and why. Please add a review response summarizing the current revision and how it addresses the prior feedback before requesting re-review. src/domain/run-contract.ts: the compact hint contract is still not fully preserved. The prior implementation emittedpending-decisionaslevel: "blocked", but this revision maps the completeness dimension’s pending-decision warning tolevel: "attention". The PR states existing compact quality hints are preserved, and downstream presentation surfaces may rely onpending-decisionbeing blocking. Please either restore the previouspending-decisionhint level or explicitly justify and document this as an intentional contract change with matching downstream updates.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown", but the new dimension model has no"unknown"dimension status anddimensionsToHints()no longer appears to produce unknown hints. This may be fine, but it leaves a potentially stale state in the public type surface.src/domain/run-contract.ts:quality.statusis derived from filtered compact hints, not directly from dimensions. This works for the current evidence-pass preservation, but it makes future dimension additions easier to accidentally hide from the rollup.
Suggestions
- Add a focused regression assertion that
pending-decisionkeeps the intended compact hint level, whether that remains"blocked"or is intentionally migrated with documented caller impact. - Consider documenting the mapping from dimension statuses to compact hints near
dimensionsToHints()since it is now the compatibility boundary.
Looks good
- The prior
evidence-readinessready-state regression appears addressed: passing Evidence dimensions still produce anevidence-readinesshint withlevel: "ok". - The implementation keeps the five quality dimensions advisory and does not move completion authority out of completion criteria.
- The added tests cover dimension presence, evidence pass/warn behavior, risk/simplicity warnings, and blocked lifecycle failures.
Verification notes
- Verifier gate status: PASS for
npm ci && npm run verify. - Size gate status: PASS, 135 changed lines < 200.
- Revision-explanation status: FAIL; required now, not found.
- Local inspection focused on
src/domain/run-contract.tsandtest/run-contract.test.ts.
Engine: pi
0dee370 to
242df5a
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 prior kapi-agent
REQUEST_CHANGESreviews, but the current PR body or author comments do not explain what changed and why for this revision. Please add a review response summarizing the latest changes and how they address the prior feedback, then request re-review.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown", while the new dimension model only emitspass | warn | failanddimensionsToHints()does not appear to generateunknown. This is not blocking, but it leaves a stale-looking contract branch that future consumers may misunderstand.
Suggestions
- Consider adding an explicit regression assertion for the pending-decision compact hint level staying
blocked, since that was prior review feedback and is now encoded inhintLevelForDimension().
Looks good
- The current code preserves the positive
evidence-readinesscompact hint for passing evidence dimensions. - The current code restores
pending-decisioncompact hints tolevel: "blocked". - The new dimensions remain advisory and do not replace completion status calculation.
- The RunContract implementation stays generic and does not introduce GitHub-specific domain coupling.
Verification notes
- Verifier gate status: FAIL due to revision-explanation gate failure.
- Size gate status: PASS — 136 changed lines, under the 200-line limit.
- Revision-explanation status: FAIL — required now, not found for the current revision.
- CI/local evidence provided:
npm ci && npm run verifyexited 0. - Inspected:
src/domain/run-contract.tsand the provided test diff/content fortest/run-contract.test.ts.
Engine: pi
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the prior feedback:
Verification:
|
|
@kapi-agent review |
kapi-agent reviewVerdict: REQUEST_CHANGES Blocking issues
Please preserve Warnings / risks
Suggestions
Looks good
Verification notes
Engine: kapi-agent request lifecycle:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
src/domain/run-contract.ts: compact hint compatibility is still not fully preserved for workflows that are both lifecycle-blocked/failed and havestate.pendingDecision.- Previous behavior always emitted
pending-decisionwheneverstate.pendingDecisionexisted. - Current
buildQualityDimensions()prioritizeshasBlockingLifecyclein the Completeness dimension reason, sohintIdForDimension()no longer sees a reason starting with"Pending decision"and emitscompleteness-dimensioninstead ofpending-decision. - Since the PR explicitly preserves existing compact
quality.hints, this is a regression for downstream consumers that key onpending-decision.
- Previous behavior always emitted
Please preserve pending-decision whenever state.pendingDecision exists, including blocked/failed lifecycle states, or explicitly document and update downstream consumers if this is intended.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown", but the new dimension projection does not appear to generate unknown statuses. Not blocking, but this is stale-looking contract surface.
Suggestions
- Add a regression test for a blocked workflow with a pending decision asserting
quality.hints.find(h => h.id === "pending-decision")?.level === "blocked".
Looks good
- The five advisory dimensions are added in a generic RunContract domain surface without GitHub/Kapi-review coupling.
- Completion authority remains separate from advisory quality dimensions.
- The prior evidence-readiness passing
okhint regression is fixed. - The active pending-decision case keeps
pending-decisionasblocked.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 136 changed lines, under the 200-line limit.
- Revision-explanation status: PASS — the author provided a current-head explanation.
- Local inspection focused on
src/domain/run-contract.tsandtest/run-contract.test.ts; the remaining blocking issue is a compact-hint contract edge case not covered by current tests.
Engine: pi
242df5a to
2a15556
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
src/domain/run-contract.ts: compact hint compatibility is still not fully preserved for lifecycle-blocked/failed workflows that also have recorded risks.- Previous behavior emitted
unresolved-riskwheneverstate.risks.lengthwas non-zero, independent of lifecycle status. - Current
buildQualityDimensions()sets the Risk dimension tofailwhenhasBlockingLifecycleis true, andhintIdForDimension()maps failed Risk dimensions tolifecycle-blocked. - Result: a blocked/failed workflow with risks now loses the legacy
unresolved-riskcompact hint id, even though the PR states existing compactquality.hintsare preserved. - Please preserve
unresolved-riskwheneverstate.risks.length > 0, including blocked/failed lifecycle states, or explicitly document this as an intentional contract change with downstream updates.
- Previous behavior emitted
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown", but the new dimension projection does not appear to generate unknown statuses. This is not blocking, but it leaves a stale-looking contract branch.
Suggestions
- Add a regression assertion for the blocked/failed + risks case, e.g.
quality.hints.find(hint => hint.id === "unresolved-risk")?.level === "attention"whenstate.risks.length > 0. - Consider avoiding duplicate or overloaded compact hint ids from dimension projection. The current mapping can make Risk lifecycle failures look like lifecycle-only blockers, which obscures risk-specific presentation behavior.
Looks good
- The revised code preserves the positive
evidence-readinesscompact hint for passing evidence dimensions. - The revised code restores
pending-decisioncompact hints tolevel: "blocked", including the blocked lifecycle + pending decision case. - The new five quality dimensions remain advisory and do not replace completion criteria authority.
- The added tests cover the main dimension surface and prior evidence/pending-decision regressions.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 138 changed lines, under the 200-line limit.
- Revision-explanation status: PASS — the author provided a current revision explanation.
- Local/CI evidence provided: full verify passed; targeted RunContract tests, TypeScript checks, diff whitespace check, and boundary grep were reported as passing.
Engine: pi
2a15556 to
0a86642
Compare
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes prior feedback:
Verification:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: APPROVE
Final approval summary
Review journey
This PR’s original goal was to add five advisory RunContract quality dimensions while preserving existing compact quality.hints behavior and keeping completion authority in the existing workflow validation path. The semantic scope is limited to RunContract projection/quality modeling plus regression tests in test/run-contract.test.ts.
Prior kapi-agent reviews requested fixes for compact hint compatibility regressions: missing positive evidence-readiness, changed pending-decision severity, and lost unresolved-risk hint identity in blocked/risk workflows. The current revision includes an author explanation and addresses those compatibility points.
What changed
src/domain/run-contract.ts- Adds
quality.dimensionswith Alignment, Evidence, Completeness, Simplicity, and Risk. - Keeps compact
quality.hintsgenerated through the legacy-compatible path. - Preserves
pending-decisionasblocked,unresolved-riskasattention, and passing evidence asevidence-readiness/ok.
- Adds
test/run-contract.test.ts- Adds/updates assertions for dimension presence/status.
- Adds regression coverage for compact hint compatibility in blocked workflows with pending decisions and risks.
Why this is correct
The revised implementation no longer derives compact hints by lossy dimension-to-hint mapping. That avoids the earlier regressions where dimension priority changed legacy hint IDs or levels. The new dimensions remain advisory metadata, while summarizeCompletion() and completion criteria evaluation remain separate and authoritative.
Evidence
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 100 changed lines, under the 200-line limit.
- Revision explanation status: PASS — required and found.
- Inspected files:
src/domain/run-contract.ts,test/run-contract.test.ts. - Tests include regression assertions for evidence readiness, pending decisions, unresolved risks, and advisory dimensions not changing completion authority.
Remaining risks and approval rationale
Remaining risk is low and mostly semantic: the new dimension heuristics are intentionally simple and may need future refinement. Approval is justified because the PR is small, verified, keeps the core generic, preserves the legacy compact hint contract, and closes the prior blocking review feedback.
Blocking issues
- None.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown"for compact hint compatibility. This is acceptable, but future dimension consumers should not assume dimensions can also be"unknown".
Suggestions
- Consider documenting the advisory-only nature of
quality.dimensionsnear the exported type definitions if external consumers start relying on this shape.
Looks good
- Compact hint compatibility is now preserved independently from the new dimension model.
- Completion status remains derived from lifecycle and completion criteria, not from advisory dimensions.
- Regression tests cover the previously flagged edge cases.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 100 changed lines < 200.
- Revision-explanation status: PASS — current revision explanation is present.
- PR evidence reports full verify passing, targeted RunContract tests passing, TypeScript/unused checks passing, and diff whitespace checks passing.
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
This PR adds five advisory RunContract quality dimensions while preserving the existing compact quality.hints contract. The semantic scope is limited to src/domain/run-contract.ts and regression coverage in test/run-contract.test.ts.
Earlier kapi-agent reviews flagged compatibility regressions in compact hints, especially pending-decision, evidence-readiness, and unresolved-risk. The current revision includes explanations and closes those issues.
What changed
src/domain/run-contract.ts- Adds
quality.dimensionsfor Alignment, Evidence, Completeness, Simplicity, and Risk. - Keeps compact hints generated through the legacy-compatible path.
- Preserves previous hint IDs and severities.
- Adds
test/run-contract.test.ts- Adds assertions for dimension ordering/status.
- Adds regression coverage for compact hint compatibility.
Why this is correct
The new dimensions are advisory only and do not replace completion criteria or lifecycle validation. Existing compact hint consumers remain protected because buildCompactQualityHints() still emits the legacy hint IDs independently of dimension projection.
Evidence
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 100 changed lines, under the 200-line limit.
- Revision explanation status: not required now; found.
- Inspected files:
src/domain/run-contract.ts,test/run-contract.test.ts.
Remaining risks and approval rationale
Remaining risk is limited to future consumers potentially treating advisory dimensions as authoritative, but this PR’s wording and implementation keep completion authority separate. Approval is justified because the PR is small, verified, and resolves the prior compatibility blockers.
Blocking issues
- None.
Warnings / risks
src/domain/run-contract.ts:RunContractQualityStatusandScoringHintLevelstill include"unknown"for the compact hint/status surface. This is compatible with existing behavior, but future dimension work should avoid confusing compact-hint states with dimension statuses.
Suggestions
- Consider documenting that
quality.dimensionsare advisory-only wherever RunContract output is consumed by presentation/rendering code.
Looks good
- Compact hint compatibility is preserved for
pending-decision,evidence-readiness, andunresolved-risk. - Completion authority remains in completion criteria and lifecycle status, not the new advisory dimensions.
- Tests cover both the new dimension surface and the legacy hint contract.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 100 changed lines < 200.
- Revision-explanation status: not required now; found.
- Local inspection matched the PR diff and test coverage.
Engine: pi
Summary
Closes #116
Refs #114
Problem
RunContract had advisory quality hints, but not the five-dimensional vocabulary requested by the RunContract Track A design. That made future rendering/scoring work more likely to collapse quality into an opaque aggregate or blur advisory quality with authoritative completion validation.
Issue #115 is still open in GitHub and there is no open PR #115, so this branch was built in parallel from current
origin/dev(87f20d4) rather than waiting for a separately landed #115 branch.Options considered
quality.hintswith only dimension objects.quality.hints.quality.dimensionsand derive existing hints from them.Selected approach
Selected option: add
quality.dimensionsand derive the existing hint surface from those dimensions.Why this one: it satisfies the issue without forcing presentation churn or changing completion semantics. Each dimension is
pass | warn | failwith reason strings andevidenceBacked, whilequality.statusremains the existing rollup used by current UI code.Implementation by file/surface
src/domain/run-contract.tsRunContractQualityDimensionId,RunContractQualityDimensionStatus, andRunContractQualityDimension.alignment,evidence,completeness,simplicity,risk.quality.hintsfrom dimensions for backward-compatible presentation behavior.taskis missing, so report/probe surfaces continue to fail closed instead of crashing.test/run-contract.test.tsWhy this fixes it
RunContract now exposes the requested five advisory dimensions directly, with status and reasons. Missing evidence degrades the Evidence dimension, pending decisions/risk affect Completeness/Risk, and lifecycle blockers produce advisory failures without replacing completion validation.
QA / Verification
npx tsx --test test/run-contract.test.ts— pass (5 tests)npx tsx --test test/cli-args.test.ts --test-name-pattern "artifact is a directory"— pass; regression for malformed partial state/report crash pathnpm run check && npm run check:unused— passnpm run verify— pass (397 tests, 386 pass, 11 skipped; quality budgets non-failing with existingcode_smellswarning)git diff --check— pass! grep -RniE 'github|pullRequest|issueNumber|kapiAgent|discord|ragna' src/domain/run-contract.ts test/run-contract.test.ts— passAnomalies observed
npm test -- test/run-contract.test.tsruns the repo test glob plus the named file because the package script istsx --test test/*.test.ts; usednpx tsx --test test/run-contract.test.tsfor the targeted RunContract check.src/cli/kapi-review-cli.tsto executable mode on this machine; reset it to100644before staging.origin/dev.Risks / Follow-up
kapi-agent review expectations and current-head merge gate
@kapi-agent review
Revision explanation for current head
242df5a2acecee833a3400eb9514eb2d11657775:What changed:
pending-decisionhint compatibility contract by mapping pending-decision-derived hints back tolevel: "blocked".evidence-readinesshint for passing evidence dimensions aslevel: "ok".Why this closes the prior feedback:
pending-decisionblocking for existing compact-hint consumers while the new five dimensions remain advisory.Verification: