fix(gsd): repair evidence-backed lifecycle shadows before validation - #2002
Open
pimmink wants to merge 10 commits into
Open
fix(gsd): repair evidence-backed lifecycle shadows before validation#2002pimmink wants to merge 10 commits into
pimmink wants to merge 10 commits into
Conversation
Contributor
🟢 PR Risk Report — LOW
|
Contributor
There was a problem hiding this comment.
Pull request overview
Repairs evidence-backed legacy Task/Slice lifecycle shadows before Milestone validation.
Changes:
- Adds replay-safe, evidence-gated lifecycle repair.
- Processes Tasks before Slices.
- Adds regression tests and architecture documentation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Summary |
|---|---|
src/resources/extensions/gsd/tools/validate-milestone.ts |
Critical: guard repair against ineligible Milestone states. |
src/resources/extensions/gsd/tests/milestone-validation-domain-operation.test.ts |
Adds repair scenario coverage. |
src/resources/extensions/gsd/lifecycle-shadow-repair-domain-operation.ts |
Critical: require explicit passing Task verification evidence. |
docs/dev/architecture.md |
Documents the repair contract. |
Suppressed comments (2)
src/resources/extensions/gsd/lifecycle-shadow-repair-domain-operation.ts:351
- Tasks are listed before Slices, so an unresolved/conflicting Task is already in
unresolvedwhen its parent Slice is visited. This loop nevertheless evaluates and can repair that Slice because descendant evidence is based on legacy Task rows rather than canonical child state, leaving a completed Slice above a nonterminal/conflicting child and committing a partial repair before validation returns an error. Skip or mark a Slice unresolved whenever one of its Tasks was unresolved, or include canonical child state in the Slice candidate before applying this repair.
for (const item of milestoneRepairItems(milestoneId)) {
const candidate = getLifecycleShadowRepairCandidate(item);
if (!candidate || candidate.canonicalStatus === "completed") continue;
src/resources/extensions/gsd/lifecycle-shadow-repair-domain-operation.ts:351
- This unconditional skip hides a conflicting terminal shadow when the legacy row is still
active,pending, orcancelled. Validation does not check descendant parity, so it can record a pass and defer the failure torequireTerminalStateduring closeout instead of returning this unresolved-shadow diagnostic. Only skip a terminal candidate whose normalized legacy status is alsocompleted; report mismatched terminal candidates as unresolved.
if (!candidate || candidate.canonicalStatus === "completed") continue;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+356
to
+358
| if (candidate.targetStatus !== "completed" || !candidate.evidence) { | ||
| unresolved.push(identity); | ||
| continue; |
Comment on lines
+395
to
+398
| if ( | ||
| canonicalInvocation && | ||
| !readMilestoneValidationReplaySource(canonicalInvocation.idempotencyKey) | ||
| ) { |
| milestoneId: string, | ||
| entries: MilestoneRepairEntry[], | ||
| ): string[] { | ||
| const idempotencyKey = `${invocation.idempotencyKey}:lifecycle-shadow-repair:milestone/${milestoneId}/batch`; |
Comment on lines
+480
to
+484
| const singleStepEntries = inScope.filter((entry) => entry.kind === "single-step"); | ||
| const twoPhaseEntries = inScope.filter((entry) => entry.kind === "two-phase-task"); | ||
|
|
||
| if (singleStepEntries.length > 0) { | ||
| repaired.push(...executeMilestoneSingleStepRepairBatch(input.invocation, milestoneId, singleStepEntries)); |
Comment on lines
+500
to
+505
| const singleStepTaskEntries = taskEntries.filter((entry) => entry.kind === "single-step"); | ||
| const twoPhaseTaskEntries = taskEntries.filter((entry) => entry.kind === "two-phase-task"); | ||
| const singleStepSliceEntries = sliceEntries.filter((entry) => entry.kind === "single-step"); | ||
|
|
||
| if (singleStepTaskEntries.length > 0) { | ||
| repaired.push(...executeMilestoneSingleStepRepairBatch(input.invocation, milestoneId, "tasks", singleStepTaskEntries)); |
Comment on lines
+256
to
+276
| function isPassingVerificationResult(verificationResult: string): boolean { | ||
| const normalized = verificationResult.trim().toLowerCase(); | ||
| if (normalized.length === 0) return false; | ||
| if ( | ||
| normalized === "passed" || | ||
| normalized === "pass" || | ||
| normalized === "success" || | ||
| normalized === "succeeded" || | ||
| normalized === "true" | ||
| ) { | ||
| return true; | ||
| } | ||
| if ( | ||
| normalized.startsWith("failed") || | ||
| normalized.startsWith("fail") || | ||
| normalized.startsWith("error") | ||
| ) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
@copilot but then its always passed even if its not right?
Member
|
CI build is red on this branch — 6 failures in the compiled unit suite, first being |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/resources/extensions/gsd/lifecycle-shadow-repair-domain-operation.ts:530
- Slice entries are committed in a second Domain Operation after the task batch has already committed. If a slice candidate fails the stability re-read (or this operation otherwise fails), the repaired Tasks remain durable, contradicting the all-or-nothing/shared-operation contract for single-step descendants. Combine Task and Slice single-step entries into one transaction while preserving Task-first ordering.
if (singleStepSliceEntries.length > 0) {
repaired.push(...executeMilestoneSingleStepRepairBatch(input.invocation, milestoneId, "slices", singleStepSliceEntries));
src/resources/extensions/gsd/db/queries.ts:260
- This still accepts every nonempty value except fail/error prefixes, so the new rejection cases for
inconclusive,needs-attention,true,banana, andNULLdeterministically receive completion evidence and fail. The contract described by this PR requires one exact normalized value; return true only forpassed.
function isPassingVerificationResult(verificationResult: string): boolean {
const normalized = verificationResult.trim().toLowerCase();
if (normalized.length === 0) return false;
if (
normalized === "passed" ||
…n evidence taskCompletionFacts() accepted any non-empty verification_result string (e.g. "failed", "inconclusive", arbitrary prose) as sufficient evidence to forward-repair a missing/ready lifecycle shadow to completed. Only an exact, case-insensitive "passed" verdict should authorize a shadow repair. Added isPassingVerificationResult() predicate (trim + lowercase equality) and regression tests covering normalized-passing variants (PASSED, mixed case, surrounding whitespace) and rejected non-passing variants (failed, inconclusive, needs-attention, arbitrary text, literal "NULL" string).
…adow repair repairMilestoneLifecycleShadowsForward() commits each repairable descendant as its own separate Domain Operation while iterating a milestone's descendants in order. When a later descendant in the same milestone turns out to be unresolved, earlier descendants' repairs have already been durably committed - there is no rollback and no all-or-nothing guarantee. Adds an execution-based (not source-grep) regression test that reproduces this directly: a milestone with one repairable Task (S01/T01) and one unresolved Task (S02/T02) currently commits T01 to completed and leaves T02 unresolved, instead of writing nothing. This is a KNOWN GAP, intentionally NOT fixed in this commit: making the milestone-level repair transactional/atomic is an architecture-level change to lifecycle-shadow-repair-domain-operation.ts's write shape and requires explicit architecture approval per docs/dev/architecture.md before implementation, per this session's governance constraints.
…escendants Supersedes the prior 'KNOWN GAP' commit: repairMilestoneLifecycleShadowsForward() previously committed each repairable descendant as its own separate Domain Operation while iterating. A later unresolved descendant in the same milestone did not roll back earlier, already-committed repairs. Fix: split the function into a read-only planning pass over every descendant, followed by an all-or-nothing write pass. - If any in-scope descendant is unresolved, nothing is written at all, and every in-scope descendant (not just the one that failed) is reported unresolved. - If every in-scope descendant is repairable, single-step repairs (missing- shadow adoption, a ready Slice's direct completion, or a Task's remaining completion step after a prior advance) commit together inside one shared lifecycle.shadow.repair Domain Operation via the new executeMilestoneSingleStepRepairBatch(), with an in-transaction re-read and stability check per descendant before any mutation. A ready Task's advance-then-complete sequence still requires two separately committed Domain Operations: repairLifecycleShadowStep() enforces that a Task's advance and completion edges belong to different, already-committed operations (requirePriorTaskRepairStep), which is an existing, unrelated invariant this change does not touch. Those Tasks are repaired individually, via the existing per-item repairLifecycleShadowForward() path, only after the planning pass has already confirmed no sibling in the milestone is unresolved - so the original bug (an unresolved sibling allowing an earlier repair to durably land) cannot occur for them either. Verified: the existing 'repairs Slice and Milestone shadows...' fixture (one two-phase ready Task + one single-step Slice) still produces exactly 3 lifecycle.shadow.repair operations, matching pre-change behavior. Two new tests assert the fixed all-or-nothing contract directly. 71/71 tests pass across lifecycle-shadow-forward-repair.test.ts and milestone-validation-domain-operation.test.ts. tsc --noEmit clean.
…x test FK fixture - Combines single-step task and slice repair entries into one unified batch Domain Operation instead of separate task/slice operations - Seeds lifecycle row properly via seedLifecycle helper in test fixture to satisfy foreign key constraints
pimmink
force-pushed
the
fix/lifecycle-shadow-authority-repair
branch
from
August 29, 2026 13:42
5a4c617 to
5bbba2b
Compare
Contributor
Author
|
@jeremymcs Thanks for the review!
Ready for review whenever you have time! |
Reverts narrative verification result loosening to adhere strictly to the PR contract requiring an exact case-insensitive 'passed' token.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
What: Repair evidence-backed legacy lifecycle shadows before adopted Milestone validation, then harden the repair gate so only truly passing evidence can authorize repair and mixed repairable/unresolved descendants write nothing.
Why: Partially adopted Milestones can have legacy-complete Tasks/Slices with durable evidence but no canonical
workflow_item_lifecyclesauthority. The original repair also had two stop-ship gaps: non-empty failed evidence could be treated as passing, and an unresolved descendant could be discovered after earlier repairs had already committed.How: Run a narrow, replay-safe, evidence-gated forward repair before recording the validation receipt. Require an exact normalized
passedTask verification result. Classify all descendants before writing; if any in-scope descendant is unresolved, write nothing. When every in-scope descendant is repairable, batch single-step repairs into one sharedlifecycle.shadow.repairDomain Operation.What
repairMilestoneLifecycleShadowsForward()for adopted Milestone validation.verification_resultnormalized exactly topassed.docs/dev/architecture.md.Why
Projects that crossed the canonical lifecycle cutover can be partially adopted: the Milestone has canonical lifecycle authority, while older completed descendants exist only in legacy hierarchy tables. In that state, validation and closeout can fail with missing canonical lifecycle authority even when durable legacy completion evidence exists.
This PR lets validation converge those legacy-complete descendant shadows safely before recording a fresh validation receipt.
How
The repair runs before Milestone validation, not during completion. Descendant lifecycle writes intentionally stale older validation receipts, so a new validation receipt is recorded only after repair has completed.
The follow-up commits harden the original implementation:
taskCompletionFacts()now uses a single exact predicate:verificationResult.trim().toLowerCase() === "passed".failed,inconclusive,needs-attention,true, arbitrary text, empty/whitespace, and literalNULLare rejected as repair evidence.lifecycle.shadow.repairDomain Operation with an in-transaction re-read/stability check.advancethencompleteremains two separate Domain Operations because the existingrepairLifecycleShadowStep()invariant requires a previously committed advance receipt before completion. Those two-phase repairs only run after the planning pass confirms no sibling descendant is unresolved.Root Cause
verification_result, so failed or inconclusive evidence could authorize canonical completion.Regression Coverage
Added or expanded behavioral coverage for:
passed,PASSED, mixed case, surrounding whitespace).failed,inconclusive,needs-attention,true, arbitrary text, empty/whitespace, literalNULL).Verification
Local focused verification:
node node_modules/typescript/bin/tsc --noEmit --project tsconfig.extensions.json: passed.node scripts/compile-tests.mjs: passed.node --test --import tsx dist-test/src/resources/extensions/gsd/tests/lifecycle-shadow-forward-repair.test.js dist-test/src/resources/extensions/gsd/tests/milestone-validation-domain-operation.test.js: 71/71 passed.pnpm run build:core: passed.bash scripts/ci-fast-gates.sh: passed, including 222/222 script tests, strict test-gap audit, strict test-matrix audit, and pi-boundary check.node scripts/lifecycle-shadow-no-cutover-gate.mjs: passed, structural 7/7 and behavioral 11/11.pnpm install --frozen-lockfile --ignore-scripts: passed;pnpm-lock.yamlunchanged.Remote full gate:
pimmink/gsd-pi-ciremote full gate run: https://github.com/pimmink/gsd-pi-ci/actions/runs/32947426378verify-pr (literal): success.verify-merge (literal): success.Upstream PR checks:
4fff959838e44d5ea5da9bdccf5f74547edd1b51.Breaking Changes
None expected. The public Milestone completion guard remains strict and unchanged.
Migration/runtime impact
No migration is added. No manual lifecycle SQL is required or recommended. Existing validation receipts created before repair remain stale after descendant lifecycle writes; rerun Milestone validation once to create a fresh post-repair receipt.
AI-assisted contribution
AI-assisted: implementation and review support were used during development; all submitted changes were reviewed and locally/remote verified by the contributor.
Closes #2055.