feat: define runtime event taxonomy#231
Conversation
|
@kapi-agent review Revision explanation for current head What changed:
Why this is bounded:
Verification:
|
kapi-agent review completedThe formal GitHub PR review and required check were submitted separately. This request comment is kept concise to avoid duplicating the full review body. kapi-agent request lifecycle:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
src/domain/runtime-events.ts:validateRuntimeEvents()does not enforce the documented monotonicseq/ replay-order contract.- README says runtime event envelopes have monotonic
seq. RUNTIME_EVENT_REPLAY_RULESsays replay applies by ascending sequence and that equalseqreplay is invalid unless it is the same idempotent event.- Current validation only checks that each
seqis a positive integer. It accepts out-of-order streams and accepts two different events with the sameseqas long as theireventIds differ. - This weakens the artifact/workflow contract because future replay/projection code could receive an apparently valid stream whose ordering is ambiguous or non-append-only.
- README says runtime event envelopes have monotonic
Please add fail-closed validation and tests for:
- decreasing/out-of-order
seq; - duplicate
seqwith differenteventId; - the allowed duplicate/idempotent case, if intended.
Warnings / risks
src/domain/runtime-events.ts: duplicateeventIdequivalence usesJSON.stringify(event). That is acceptable if the contract is “byte-equivalent serialized object in this in-memory representation,” but it may reject semantically identical events with different property insertion order. Not blocking, but worth documenting or normalizing before this becomes persisted replay input.
Suggestions
- Consider making the sequence rule explicit in one helper, e.g. track prior
seqand aMap<number, eventId/fingerprint>, so the validator mirrors the replay rules directly. - Add a test that a second identical duplicate event does not fail, to lock down idempotent replay behavior.
Looks good
- The taxonomy is bounded and domain-local.
- The contract explicitly models recovery through events rather than destructive cleanup.
run.sealedterminal behavior is represented in the spec and has direct test coverage.- README was updated alongside the implementation, which keeps the workflow contract discoverable.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 151 changed lines, below the 200-line semantic review threshold.
- Revision-explanation status: not required; found anyway.
- Ilchul review harness gate: PASS — STANDARD risk, no blocking harness findings.
- Local/file inspection found the blocking sequence-validation gap above despite passing tests.
Engine: pi
2317037 to
9d181df
Compare
|
@kapi-agent review Revision explanation for current head What changed:
Why this closes the prior feedback:
Verification:
|
kapi-agent review completedThe formal GitHub PR review and required check were submitted separately. This request comment is kept concise to avoid duplicating the full review body. kapi-agent request lifecycle:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: APPROVE
Final approval summary
Review journey
This PR defines the runtime event taxonomy and validation contract for #186. The semantic scope is bounded to event envelope/types, replay rules, validation, README documentation, and focused tests; it does not implement storage, worker dispatch, migrations, or destructive recovery.
A prior kapi-agent review requested changes because validateRuntimeEvents() documented monotonic replay semantics but did not enforce monotonic seq ordering or conflicting duplicate sequence handling. The current revision directly addresses that feedback.
What changed
The current revision updates:
src/domain/runtime-events.ts- Adds monotonic sequence validation.
- Tracks seen sequence numbers.
- Rejects duplicate
seqvalues when they refer to a different event. - Preserves exact duplicate/idempotent replay when the repeated event is byte-equivalent.
test/runtime-events.test.ts- Adds regression coverage for out-of-order sequences.
- Adds duplicate sequence conflict coverage.
- Adds allowed exact duplicate/idempotent replay coverage.
README.md- Documents the runtime event envelope and replay contract.
Why this is correct
The validator now matches the documented replay contract: streams must be append/replay ordered by ascending seq, ambiguous same-sequence events fail closed, and exact duplicate replay remains valid for idempotent retry behavior. This closes the prior artifact/workflow contract integrity issue without expanding scope into event persistence or projection implementation.
Evidence
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 172 changed lines, below the 200-line semantic review threshold.
- Revision explanation status: PASS — author provided a current-head explanation for
9d181df6325ecf50511d9601ea7928e5c92e211e. - Ilchul review harness gate: PASS — STANDARD risk profile, no blocking findings.
- Inspected files:
src/domain/runtime-events.ts,test/runtime-events.test.ts, and README diff/context. - Test evidence includes focused runtime-event tests plus full verify suite passing.
Remaining risks and approval rationale
Remaining risks are limited to future integration choices, especially how byte-equivalent duplicate detection is represented once events are serialized/persisted. That is acceptable for this taxonomy/contract slice and is not blocking. Approval is justified because the prior blocking issue is fixed, the scope is bounded, verification passes, and no remaining blocking correctness or safety issue was found.
Blocking issues
None.
Warnings / risks
src/domain/runtime-events.ts: duplicate/idempotent equivalence is based onJSON.stringify(event), which is stricter than checking onlyidempotencyKeyandpayload. This is acceptable for the current in-memory validator, but future persisted-event code should define canonical serialization to avoid property-order or representation drift.
Suggestions
- Consider adding a small comment near the fingerprint logic clarifying that byte-equivalent duplicate replay currently means the full in-memory event object, not just payload/idempotency fields.
Looks good
- The runtime event taxonomy is explicit and versioned.
- Validation fails closed for malformed envelopes, unsupported types, category mismatches, missing required payload fields, lease timestamp issues, conflicting duplicate event ids, sequence conflicts, and mutation after
run.sealed. - The new tests cover the prior review’s requested monotonic and duplicate-sequence cases.
- README is aligned with the new runtime-event contract.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 172 changed lines < 200.
- Revision-explanation status: PASS — current-head explanation found.
- Ilchul review harness: PASS — STANDARD risk, no blocking findings, verdict COMMENT.
- Local/CI evidence provided: focused runtime-event tests and full
npm run verifypassed.
Engine: pi
|
@kapi-agent review --force Revision explanation for current head What changed
Why this closes the prior feedback
Verification
|
9d181df to
06efc55
Compare
kapi-agent review completedThe formal GitHub PR review and required check were submitted separately. This request comment is kept concise to avoid duplicating the full review body. kapi-agent request lifecycle:
|
There was a problem hiding this comment.
kapi-agent review
Verdict: APPROVE
Final approval summary
Review journey
This PR defines the runtime event taxonomy and validation contract for #186. The semantic scope remains bounded to event envelope/types, replay rules, validation, README documentation, and focused tests. It does not add event storage, worker dispatch, migrations, or destructive recovery behavior.
Earlier kapi-agent feedback requested fail-closed enforcement for monotonic seq replay ordering and duplicate sequence conflicts. That feedback was addressed and previously approved. The latest revision was a rebase onto current dev, with no intentional source behavior change.
What changed
The PR adds:
src/domain/runtime-events.ts- Runtime event types, categories, replay actions, specs, and schema version.
- Validation for envelope shape, required payload fields, type/category alignment, timestamps, duplicate event ids, monotonic sequence ordering, conflicting duplicate sequence values, idempotent duplicate replay, and terminal
run.sealed.
test/runtime-events.test.ts- Catalog/replay-rule coverage.
- Envelope and payload validation coverage.
- Duplicate id, duplicate sequence, monotonic ordering, idempotent duplicate, and post-seal mutation regression coverage.
README.md- Runtime event taxonomy and replay contract documentation.
Why this is correct
The implementation now matches the documented replay contract: events are append-only, ordered by monotonic seq, duplicate event ids must be byte-equivalent, duplicate sequence values are only accepted for the same idempotent event, and non-seal events after run.sealed are rejected. The tests cover the previously blocking gap around ambiguous replay ordering.
Evidence
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 172 changed lines, below the 200-line reviewability threshold.
- Ilchul review harness gate: PASS — STANDARD risk, no blocking findings.
- Revision explanation status: not required for this re-review, but found. The author explained the latest head was a rebase onto current
devpreserving the previously approved implementation. - Inspected files:
src/domain/runtime-events.ts,test/runtime-events.test.ts, and README runtime-event documentation context.
Remaining risks and approval rationale
Remaining risk is low and acceptable for this slice. The duplicate-equivalence check uses JSON.stringify(event), so equivalence is representation/order-sensitive; that is acceptable for the current in-memory validation contract and was already called out as a non-blocking concern. Approval is justified because all gates pass, prior blocking feedback is closed, tests cover the contract risks, and the latest revision only rebases the approved implementation.
Blocking issues
None.
Warnings / risks
src/domain/runtime-events.ts: duplicate event equivalence depends onJSON.stringify(event), so semantically identical objects with different property insertion order would not be treated as equivalent. This is acceptable for now if byte/representation equivalence is intended.
Suggestions
- If these events will later be persisted or accepted from multiple serializers, consider introducing a canonical event fingerprint helper so idempotent duplicate comparison is stable across serialization sources.
Looks good
- Runtime event specs are explicit and compact.
- Validation fails closed for the main artifact/workflow contract risks.
run.sealedterminal behavior is enforced.- Tests directly cover the prior monotonic sequence and duplicate sequence feedback.
- README stays aligned with the implementation.
Verification notes
Verifier gate status: PASS — npm ci && npm run verify exited 0.
Size gate status: PASS — 172 changed lines under the semantic review threshold.
Revision-explanation status: not required now; found. The latest explanation states this was a rebase onto current dev with no intentional source changes.
Additional gate evidence: Ilchul review harness PASS, STANDARD risk profile, no blocking findings.
Engine: pi
Summary
run.sealed.Scope
Non-goals / deliberately not included
Validation behavior
schemaVersion: 1.eventId,runId,type,category, timestampat, positive integerseq,idempotencyKey, and typedpayload.eventIdis accepted only when the serialized event is byte-equivalent; conflicting duplicates fail validation.run.sealedis terminal; later non-seal events for the same validated stream are rejected.Verification
npm ci— passnpx tsx --test test/runtime-events.test.ts— pass, 3 testsnpm run check— passnpm run check:unused— passnpm run quality:budgets— pass with existing non-failing warning:code_smells=56 > 20; coverage not configurednpm run verify— pass, 508 passed / 11 skippedgit diff --check— passSize
Closes #186
Refs #167