feat: add task graph readiness projection#232
Conversation
|
@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: REQUEST_CHANGES
Blocking issues
src/domain/task-graph.ts: blocked readiness is only computed for direct failed dependencies, so downstream tasks remain classified aswaitingeven when an upstream failure makes them unexecutable.- Example:
a: failed,b: pending depends on a,c: pending depends on b. - Current behavior:
bisblocked,ciswaiting. - For the stated contract of “blocked/downstream status projection” and “classify downstream tasks blocked by failed dependencies,”
cshould be represented as blocked or otherwise machine-readably blocked by the upstream failure chain. - Please add coverage for an indirect downstream failure case and update
computeTaskReadiness()/projectTaskGraphStatus()accordingly.
- Example:
Warnings / risks
test/task-graph.test.tscovers the happy-path 5-task DAG but does not cover failed dependency propagation through multiple levels. That is the main regression risk for operator-facing status projection.TaskGraphRuntimeEventrecords are intentionally lightweight domain records, not durable runtime envelopes. That is acceptable for this phase, but downstream integration should avoid treating these as persisted execution events without envelope metadata.
Suggestions
- Consider making readiness reasons for downstream blocking explicit, e.g.
blocked by failed upstream dependency a via b, so future CLI/report surfaces can explain why a task is not merely waiting. - Consider adding a small test for deterministic topological ordering when multiple roots are present, since the implementation sorts available nodes lexicographically rather than preserving input order.
Looks good
- The domain-only scope is well bounded and does not prematurely wire worker dispatch, CLI output, persistence, or claim/lease expansion.
- The new surfaces are small and cohesive: readiness projection, topological order, and event records all stay in
src/domain/task-graph.ts. - Existing validation is reused via
assertValidGraph(), so malformed graphs still fail closed before readiness/topology computation. - README alignment is minimal and matches the new runtime-boundary wording.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 141 changed lines, below the 200-line semantic review threshold.
- Revision-explanation status: not required for this initial kapi-agent review; explanation was nevertheless found.
- Ilchul review harness gate: PASS — STANDARD risk, neutral conclusion, no blocking findings.
- PR evidence reports
npm ci, targeted/full tests,npm run check,npm run verify, andgit diff --checkpassing.
Engine: pi
3330dba to
e9d2538
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
The original goal was to add the phase-1 TaskGraph readiness projection surface: topological ordering, inspectable readiness reasons, operator status projection, and lightweight graph/readiness event records. The semantic scope is bounded to the domain layer plus targeted tests and a one-line README alignment update.
A prior kapi-agent review requested changes because blocked readiness only propagated from direct failed dependencies. The current revision includes an author explanation and addresses that feedback by propagating failed upstream dependency blocking through downstream dependency chains.
What changed
src/domain/task-graph.ts- Adds readiness projection APIs and runtime event records.
- Adds deterministic topological ordering.
- Updates blocked-task computation to use readiness projection.
- Propagates failed-dependency blockers transitively.
test/task-graph.test.ts- Adds coverage for the 5-task DAG fixture.
- Adds regression coverage for
failed -> blocked child -> blocked grandchild.
README.md- Aligns the graph execution runtime boundary wording with the new domain surface.
Why this is correct
The previous blocking case is now covered and implemented: a pending task depending on another blocked task is projected as blocked, with machine-readable blockedBy and an explanatory upstream/via reason. Existing repair supersession behavior remains represented through getCompletedOrRepairedTaskIds() / failed blocker filtering.
Evidence
- Verifier gate: PASS.
- Size gate: PASS, 192 changed lines under the 200-line review threshold.
- Revision explanation status: required and found.
- Ilchul review harness gate: PASS, risk profile
STANDARD, blocking findings0. - Verified command evidence:
npm ci && npm run verifyexited 0. - Inspected files:
src/domain/task-graph.ts,test/task-graph.test.ts,README.md.
Remaining risks and approval rationale
Remaining risks are limited to future integration surfaces: durable runtime-event envelopes and CLI/report presentation are intentionally deferred. For this domain-only phase-1 slice, the implementation is bounded, tested, and closes the prior blocking feedback, so approval is justified.
Blocking issues
None.
Warnings / risks
src/domain/task-graph.ts:TaskGraphRuntimeEventremains a lightweight domain event shape rather than a durable runtime envelope. This is acceptable for the stated phase-1 boundary but should be revisited when persistence/reporting integration lands.- CLI/operator presentation is intentionally not added in this PR, so consumers must call the new domain projection APIs directly until a later slice wires them into reports.
Suggestions
- Consider adding a future test for mixed dependency states, e.g. a task with one blocked dependency and one still-pending dependency, to lock down whether
blockedshould suppress or preservewaitingFordetails.
Looks good
- The prior indirect-blocking issue is fixed with explicit regression coverage.
computeBlockedTaskIds()now delegates to the richer readiness projection, reducing duplicated status logic.- Graph validation remains fail-closed before readiness/topological operations.
- README wording matches the implemented domain behavior without overstating CLI/runtime integration.
Verification notes
- Verifier gate status: PASS —
npm ci && npm run verifyexited 0. - Size gate status: PASS — 192 changed lines, below the configured threshold.
- Revision-explanation status: PASS — revision explanation was required and found.
- Ilchul review harness: PASS —
STANDARDrisk profile, neutral/comment verdict, no blocking findings. - Local/CI evidence in PR body includes targeted task-graph testing, full verify gates, and
git diff --check.
Engine: pi
Summary
Linked issue
Closes #194
Problem
Issue #194 asks for the first DAG runtime slice: represent a task graph, validate dependencies, compute ready tasks, explain blocked/waiting tasks, project operator status, and specify events for graph creation/readiness changes.
Before this PR,
src/domain/task-graph.tshad basic task IDs, dependencies, ready-id computation, and validation. It did not expose a first-class runtime task shape with titles/descriptions, topological ordering, inspectable readiness reasons, aggregate projection, or task-graph/readiness event records.Options considered
computeReadyTaskIds()and document the rest as future work.src/domain/task-graph.ts.Selected approach
Selected option: 2 — focused domain-only TaskGraph surface.
Why this one: #194 is the phase-1 runtime primitive. Keeping this slice in the domain layer preserves adapter neutrality and gives later #197/#196 work a stable surface for claim/lease and worker execution without prematurely changing CLI behavior.
Risks/trade-offs: event records here are lightweight domain records, not durable
RuntimeEventEnvelopeinstances. Durable envelope wiring can happen when execution persistence/reporting is connected.Implementation by file/surface
src/domain/task-graph.tstitle/descriptiontoTaskNodeand a strictRuntimeTaskinterface.computeTaskReadiness()withready,waiting,blocked, andnot_pendingreasons.projectTaskGraphStatus()for aggregate operator/reporting projection.topologicalTaskIds()with validation-backed deterministic ordering.createTaskGraphRuntimeEvents()fortask_graph.createdandtask.readyphase-1 event records.test/task-graph.test.tsfailed -> blocked child -> blocked grandchild).README.mdWhy this fixes it
The TaskGraph domain can now reject malformed graphs, order valid DAGs, compute only dependency-complete tasks as ready, classify downstream tasks blocked by failed dependencies across multiple levels, expose machine-readable reasons, and emit phase-1 graph/readiness event records. That covers the issue’s phase-1 acceptance criteria while preserving non-goals: no worker dispatch, claim/lease expansion, integration, repair generation, or policy learning behavior.
QA / Verification
npm ci— pass; installed local dependencies becausetsxwas absent in this fresh worktree.npm test -- test/task-graph.test.ts— pass; package script runs the fulltest/*.test.tssuite plus the explicit file argument (530tests,519pass,11skipped).npm run check— pass.npm run check:unused— pass.npm run quality:budgets— pass with configured non-failingcode_smellswarning.npm run verify— pass before the review-response amendment (npm test,npm run check,npm run check:unused,npm run quality:budgets). The review-response amendment reran the same constituent gates listed above.git diff --check— pass.Anomalies observed
npm test -- test/task-graph.test.tsdoes not run only the named file becausepackage.jsondefinestsx --test test/*.test.ts; it executed the full test suite.[DEP0205] module.register()deprecation warnings during tests.npm run quality:budgetsretains the existing warning:code_smells=60vs target<=20; the command exits 0 under the configured budget policy.Risks / Follow-up
kapi-agent review expectations and current-head merge gate
e9d25381ef0565541b9846f27383a05e3873be39origin/dev(182insertions,10deletions), under the review-size gate.kapi-agentshould verify the domain-only scope, transitive failed-dependency blocking, graph validation interaction, and README alignment before merge.