Skip to content

feat: add RunContract evidence criteria#110

Merged
devkade merged 1 commit into
devfrom
feat/issue-99-runcontract-criteria
May 14, 2026
Merged

feat: add RunContract evidence criteria#110
devkade merged 1 commit into
devfrom
feat/issue-99-runcontract-criteria

Conversation

@devkade
Copy link
Copy Markdown
Owner

@devkade devkade commented May 14, 2026

Summary

  • Adds generic EvidenceExpectation and CompletionCriteria primitives to the RunContract domain view.
  • Derives inspectable missing/satisfied criteria from existing ValidationEvidence and required artifacts.
  • Keeps this additive and generic: no policy module, no adapter-specific rules, and no replacement of existing workflow validation.

Issue

Closes #99

Implementation notes

  • RunContractView now includes expectations, completionCriteria, and completion.results.
  • Default expectations include evidence:any and required artifact expectations derived from existing workflow artifacts.
  • evaluateCompletionCriteria() reports satisfiedExpectationIds, missingExpectationIds, and clear missing-evidence reasons.
  • Existing completion summary now surfaces needs-evidence when required generic expectations are missing.

Verification

  • npm test -- test/run-contract.test.ts
  • npm run verify
  • git diff --check
  • grep -RniE 'github|pullRequest|issueNumber|kapiAgent|discord|ragna' src/domain/run-contract.ts test/run-contract.test.ts returned no matches

Notes

  • npm run verify still reports the existing non-failing quality budget warning: code_smells=44, target <=20, status warn.

@devkade
Copy link
Copy Markdown
Owner Author

devkade commented May 14, 2026

kapi-agent review

Verdict: APPROVE

Final approval summary

Review journey

Evidence

  • git diff --check passed before commit.
  • npm test -- test/run-contract.test.ts passed.
  • npm run verify passed.
  • Domain boundary grep returned no adapter/branding matches for src/domain/run-contract.ts and test/run-contract.test.ts.

Notes

  • npm run verify still reports existing non-failing quality budget warning: code_smells=44, target <=20, status warn.
  • This is acceptable for this slice; it is an advisory quality budget warning, not a failing gate.

Decision

Approved for merge into dev.

@devkade devkade merged commit 056a9f6 into dev May 14, 2026
1 check 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: REQUEST_CHANGES

Blocking issues

  • src/domain/run-contract.ts: required artifact expectations are labeled as “Required artifact is tracked”, but the evaluator only satisfies them when some ValidationEvidence record references that artifact via evidence.artifactRefs.
    • This makes the RunContract contract internally inconsistent: state.artifacts can clearly contain the required artifact, while completion.results[].missingExpectationIds still reports artifact:<name> as missing with the reason “Required artifact is tracked: ”.
    • Either change the satisfaction logic so artifact expectations are satisfied by the required artifact being present in state.artifacts, or change the expectation summary/criteria semantics to explicitly require validation evidence for each required artifact.
    • Please add a direct test for the intended behavior with required artifacts present and evidence present but not referencing the artifact.

Warnings / risks

  • test/run-contract.test.ts: the “complete” test only reaches completion by replacing artifacts: [], so it does not verify the normal case where workflows have required artifacts. This leaves the main new criteria path under-tested.
  • src/domain/run-contract.ts: artifactRefs currently means two things: expected artifact existence and evidence-reference matching. That ambiguity is likely to confuse future contract consumers.

Suggestions

  • Consider renaming the field or splitting the model if both concepts are needed:
    • requiredArtifactRefs / requiredTrackedArtifacts
    • evidenceArtifactRefs
  • Consider making missing reasons more explicit, e.g. “No validation evidence references required artifact: verify.md” if evidence linkage is the intended rule.

Looks good

  • The new RunContract fields are additive and stay within the domain layer.
  • Existing evidence projection still avoids aliasing mutable arrays.
  • The completion status now exposes a useful needs-evidence state instead of treating completion as purely lifecycle-derived.

Verification notes

  • Verifier gate status: PASS — npm ci && npm run verify exited 0.
  • Size gate status: PASS — 95 changed lines, under the 200-line limit.
  • Revision-explanation status: not required for this opened PR; found in PR body.
  • I inspected src/domain/run-contract.ts and test/run-contract.test.ts. The blocking issue is semantic/contract correctness, not CI failure.

Engine: pi

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.

feat: model EvidenceExpectation and CompletionCriteria for RunContract

1 participant