fix(planning): pin WP0 validation to immutable merge evidence - #417
fix(planning): pin WP0 validation to immutable merge evidence#417AlexStocks wants to merge 2 commits into
Conversation
PR #414 was squash-merged after PR #412, so comparing the original SDD baseline with the moving main branch made the WP0 artifact gate absorb unrelated storage and Raft paths. Record immutable PR and squash-merge evidence, keep WP0 at implemented while exact-main verification is pending, and validate the historical merge-parent diff independently of the current work package. Add regression coverage for concurrent merges, invalid refs, ancestry, and premature lifecycle promotion. Constraint: Keep WP1 and Issue #415 blocked until WP0 has passed exact-main evidence. Confidence: high Scope-risk: narrow Tested: Windows and Ubuntu WSL SDD self-test, validator, Python compile, and git diff checks. Not-tested: GitHub Actions on the pushed Head. Co-authored-by: OmX <omx@oh-my-codex.dev> Signed-off-by: Xin.Zh <alexstocks@foxmail.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates the SDD to version 2 and records merged WP0 evidence. The validator now checks immutable PR and merge references, exact-main verification metadata, Git ancestry, changed paths, and merge markers. Self-tests cover invalid references and lifecycle promotion rules. ChangesWP0 validation and planning state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SDD
participant validate_sdd
participant Git
SDD->>validate_sdd: provide WP0 evidence fields
validate_sdd->>Git: inspect fixed PR and merge refs
Git-->>validate_sdd: return ancestry, paths, whitespace, and subject data
validate_sdd-->>SDD: accept or report validation errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
scripts/validate_sdd.py (4)
1189-1194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the replacement count, as the neighbouring mutations do.
break_wp0_pr_numberuses a plainstr.replacewith the literal414.break_baselineandbreak_wp0_evidence_refboth usere.subnwith a count assertion, so a stale literal fails with a clear message. Ifwp0_pr_numberchanges, this mutation becomes a no-op and the failure appears as an unrelated assertion. Align it with the existing pattern.♻️ Proposed refactor
def break_wp0_pr_number(candidate: Path) -> None: path = candidate / ".planning/SDD.md" - path.write_text( - read_text(path).replace("wp0_pr_number: 414", "wp0_pr_number: 999", 1), - encoding="utf-8", - ) + text, replacements = re.subn( + r"(?m)^wp0_pr_number: [1-9][0-9]*$", + "wp0_pr_number: 999", + read_text(path), + count=1, + ) + if replacements != 1: + raise AssertionError( + "WP0 PR mutation requires exactly one wp0_pr_number field" + ) + path.write_text(text, encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_sdd.py` around lines 1189 - 1194, Update break_wp0_pr_number to use the same counted-replacement pattern as break_baseline and break_wp0_evidence_ref: capture the replacement count, assert exactly one occurrence of the expected wp0_pr_number value was changed, then write the resulting text.
371-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated Implementation PR pattern.
The regex at lines 371-374 is identical to the one at lines 359-362. Two copies of the same contract must stay in sync by hand. Move the pattern to a module-level constant and reuse it in both places.
♻️ Proposed refactor
+IMPLEMENTATION_PR_PATTERN = ( + r"(?m)^Implementation PR：\[#(?P<pr>\d+)\]" + r"\(https://github\.com/arana-db/kiwi/pull/(?P=pr)\)。$" +)- pr_matches = re.findall( - r"(?m)^Implementation PR：\[#(?P<pr>\d+)\]\(https://github\.com/arana-db/kiwi/pull/(?P=pr)\)。$", - current_block, - ) + pr_matches = re.findall(IMPLEMENTATION_PR_PATTERN, current_block)- wp0_pr_matches = re.findall( - r"(?m)^Implementation PR：\[#(?P<pr>\d+)\]\(https://github\.com/arana-db/kiwi/pull/(?P=pr)\)。$", - blocks.get("WP0", ""), - ) + wp0_pr_matches = re.findall(IMPLEMENTATION_PR_PATTERN, blocks.get("WP0", ""))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_sdd.py` around lines 371 - 374, Extract the duplicated Implementation PR regex used by the WP0 matching logic and the earlier matching logic into a module-level constant, then replace both inline patterns with that shared constant. Preserve the existing pattern and matching behavior.
908-951: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated evidence-mutation blocks into a helper.
The three blocks at lines 908-951 follow one shape: copy
fields, override one key, callvalidate_wp0_git_evidence, assert a fragment. A small helper removes the duplication and makes new regression cases cheap to add.♻️ Proposed refactor
+ def expect_evidence_failure(overrides: dict[str, str], fragment: str, message: str) -> None: + mutated = dict(fields) + mutated.update(overrides) + mutation_errors: list[str] = [] + validate_wp0_git_evidence( + root, + mutated, + set(EXPECTED_WP0_ARTIFACTS), + mutation_errors, + ) + if not any(fragment in error for error in mutation_errors): + raise AssertionError(f"{message}: {mutation_errors}") + + expect_evidence_failure( + {"wp0_merge_parent_ref": fields["wp0_pr_base_ref"]}, + "squash-merge evidence", + "wrong merge parent must fail the immutable lineage check", + ) + expect_evidence_failure( + {"wp0_merge_ref": fields["wp0_merge_parent_ref"]}, + "squash-merge evidence", + "wrong merge ref must fail the immutable lineage check", + ) + expect_evidence_failure( + {"wp0_pr_base_ref": "0" * 40}, + "PR base ref is not available", + "wrong PR base ref must fail even when the PR head object is unavailable", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_sdd.py` around lines 908 - 951, Extract the repeated mutation, validation, and error-fragment assertion logic from the three cases into a local helper near the surrounding regression checks. Have the helper accept the field name, replacement value, expected error fragment, and failure message, then copy fields, override the requested key, call validate_wp0_git_evidence, and assert the fragment; replace the wrong-parent, wrong-merge, and wrong-base blocks with helper calls while preserving their existing values and messages.
587-594: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the PR head check is intentionally optional.
When
pr_head_checkfails, the code records no error and skips the whole PR base→head validation. That is correct for a squash merge, because the PR head object can be absent from a clone ofmainafter the branch is deleted. The code does not state this. A future change can turn the silent skip into a hard failure and break CI.Add a short comment that explains the optional path.
📝 Proposed comment
+ # The PR head object is only present when the source branch still exists. + # A squash merge does not put it on main, so treat this range as optional + # evidence and rely on the merge-parent range below for the mandatory check. pr_head_check = subprocess.run(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_sdd.py` around lines 587 - 594, In the validation block immediately before the condition using pr_base_check and pr_head_check, add a short comment documenting that pr_head_check is intentionally optional because squash merges may leave the PR head object absent after branch deletion, so the base-to-head validation is skipped in that case..planning/SDD.md (1)
916-922: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider binding the 合并证据 SHAs to the front matter fields.
Lines 918-919 repeat
wp0_pr_base_ref,wp0_pr_head_ref,wp0_merge_parent_ref, andwp0_merge_refas prose. The validator does not compare these strings with the front matter. The prose can drift from the machine-readable evidence without failing any gate. Line 922 has the same gap forwp0_exact_main_verification_status.Add an assertion in
validate_current_statethat the WP0 block contains the exact ranges built from the front matter fields. That keeps the human-readable evidence and the immutable fields in one contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/SDD.md around lines 916 - 922, Update validate_current_state to assert that the WP0 merge-evidence prose exactly contains the PR and merge ranges constructed from wp0_pr_base_ref, wp0_pr_head_ref, wp0_merge_parent_ref, and wp0_merge_ref, and that its verification status matches wp0_exact_main_verification_status. Keep the front matter as the source of truth and fail validation when the human-readable evidence drifts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/SDD.md:
- Around line 916-922: The WP0 front matter replicas are not validated against
their immutable source fields. At .planning/SDD.md lines 916-922, extend
validate_current_state to assert the 合并证据 ranges use wp0_pr_base_ref,
wp0_pr_head_ref, wp0_merge_parent_ref, and wp0_merge_ref, and that the line 922
status matches wp0_exact_main_verification_status; at .planning/SDD.md line
1609, add the “WP0 exact-main verification” entry to table_expectations using
verification_status.
- Line 1410: Update the Issue `#143` entry in the Issue index to replace the stale
“支持轨道，等待 PR `#412`” status with wording that reflects PR `#412` being merged and
Issue `#143` being closed, consistent with the status recorded near line 1410.
In `@scripts/validate_sdd.py`:
- Around line 390-398: Update the `verification_status == "passed"` branch to
require `verification_ref` to equal `wp0_merge_ref`, rather than only validating
its 40-character SHA format. Preserve the existing `verification_run`
validation, and ensure the recorded passed verification remains bound to the WP0
merge commit.
- Around line 621-642: Update the ancestry-check condition around pr_base_check
and merge_ancestry to also require that merge_parent_ref resolves to a valid
commit, using the existing object-validation result from
validate_expected_git_diff. Skip merge-base when the merge-parent object is
unavailable so only the missing-object diagnostic is reported; preserve the
existing ancestry error for valid objects that are not ancestors.
---
Nitpick comments:
In @.planning/SDD.md:
- Around line 916-922: Update validate_current_state to assert that the WP0
merge-evidence prose exactly contains the PR and merge ranges constructed from
wp0_pr_base_ref, wp0_pr_head_ref, wp0_merge_parent_ref, and wp0_merge_ref, and
that its verification status matches wp0_exact_main_verification_status. Keep
the front matter as the source of truth and fail validation when the
human-readable evidence drifts.
In `@scripts/validate_sdd.py`:
- Around line 1189-1194: Update break_wp0_pr_number to use the same
counted-replacement pattern as break_baseline and break_wp0_evidence_ref:
capture the replacement count, assert exactly one occurrence of the expected
wp0_pr_number value was changed, then write the resulting text.
- Around line 371-374: Extract the duplicated Implementation PR regex used by
the WP0 matching logic and the earlier matching logic into a module-level
constant, then replace both inline patterns with that shared constant. Preserve
the existing pattern and matching behavior.
- Around line 908-951: Extract the repeated mutation, validation, and
error-fragment assertion logic from the three cases into a local helper near the
surrounding regression checks. Have the helper accept the field name,
replacement value, expected error fragment, and failure message, then copy
fields, override the requested key, call validate_wp0_git_evidence, and assert
the fragment; replace the wrong-parent, wrong-merge, and wrong-base blocks with
helper calls while preserving their existing values and messages.
- Around line 587-594: In the validation block immediately before the condition
using pr_base_check and pr_head_check, add a short comment documenting that
pr_head_check is intentionally optional because squash merges may leave the PR
head object absent after branch deletion, so the base-to-head validation is
skipped in that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffd1058c-d5bb-4254-9215-489ab031d617
📒 Files selected for processing (2)
.planning/SDD.mdscripts/validate_sdd.py
Keep the human-readable WP0 ranges, exact-main status, and current-state table synchronized with the sole machine-readable front matter. Reject duplicate or conflicting projections and stale Issue tracking. Require a passed exact-main ref to exist after the WP0 merge and within the recorded baseline main history, while avoiding misleading ancestry errors when merge objects are unavailable. Add regressions for unrelated verification refs and conflicting evidence lines. Constraint: Preserve offline validation without requiring the squash-merged PR Head object. Confidence: high Scope-risk: narrow Tested: Windows and Ubuntu WSL SDD self-test, validator, and git diff checks. Not-tested: GitHub Actions on this new Head. Co-authored-by: OmX <omx@oh-my-codex.dev> Signed-off-by: Xin.Zh <alexstocks@foxmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the WP0 planning/traceability validator so WP0 artifacts are validated against immutable merge evidence (PR/base/head, merge-parent→merge) rather than a floating working HEAD, preventing concurrent merges from being misattributed to WP0. It also updates the SDD to record the fixed refs/evidence and marks WP0 as implemented (but not verified/accepted) pending exact-main verification.
Changes:
- Extend
scripts/validate_sdd.pyto require and validate immutable WP0 evidence fields (PR number, PR base/head, merge parent/merge, and exact-main verification status), including ancestry/lineage checks and fixed-range diff validation. - Update
.planning/SDD.mdfront matter and WP0 section to record the immutable refs and post-merge CI failure evidence, and to reflect WP0 status asimplemented. - Expand validator self-tests to cover concurrent-merge regressions and evidence-projection drift.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/validate_sdd.py | Adds immutable WP0 evidence fields and validates fixed PR/merge ranges, lineage, and exact-main verification gating. |
| .planning/SDD.md | Records WP0 fixed refs and post-merge evidence; updates WP0 status to implemented and documents next safe action under #416. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SDD Traceability
9820162ebdf2d26aa6349e704efe8737b2e73e4aDescription
PR #414 在 PR #412 之后 squash merge。旧 validator 用 SDD 事实 baseline 到浮动 working HEAD 的累计 Diff 验证 WP0，导致 main push CI 把 #412 的 7 个 Storage/Raft 路径误计为 WP0 产物。
本 PR 将 WP0 状态精确更新为
implemented，记录 PR、squash merge 和失败 run/job 的不可变证据，并把历史产物校验固定到 merge-parent→merge 区间。PR Head 对象存在时额外验证 PR base→head；离线门禁不依赖网络 fetch。Validator 同时强制校验 base ancestry、squash lineage、不可变 PR 编号，以及verified/accepted/released所需的 exact-main 成功 ref/run。本 PR不启动 WP1，也不实现 #415 的 Redis Oracle 代码。
Type of Change
Scope Completion
Verification
python scripts/validate_sdd.py --self-testpython scripts/validate_sdd.pypython -m py_compile scripts/validate_sdd.pygit diff --checkgit diff --checkenvironment, result, and residual risk in
.planning/SDD.mdbefore markingthe work package
verifiedoraccepted.Checklist
Additional Context
verified或accepted。Summary by CodeRabbit
Documentation
Bug Fixes