Add the parity roadmap and the client rules (MUST/SHOULD/CAN) - #5
Conversation
docs/parity-roadmap.md: the phased plan for closing client-rust's feature gap against client-go — in-flight fixes, dependency modernization (kvproto refresh first; the tonic migration is independent), low-risk plumbing, then the architectural projects. Every gap claim carries file:line evidence at pins.toml's pins. docs/client-rules.md: the normative baseline behind that ordering — 56 MUST/SHOULD/CAN rules grounded in the kvproto contract, with client-rust graded per rule. Five MUST violations, two of them previously uncharacterized: the async-commit min_commit_ts==0 path silently commits at ts 0 (not the panic its FIXME implies), and ServerIsBusy/MaxTimestampNotSynced are surfaced to callers instead of retried. Both documents survived a three-agent adversarial review plus a codex review at high effort; the ~20 verified findings (wrong failure mode, a mislabeled risk tier, a workstream ordering with no real dependency, one already-fixed upstream bug we nearly re-filed, and a batch of shifted proto line refs) are fixed in this commit. Findings header and README updated to reflect that #543/#545 are now filed upstream (PRs #544/#547 in flight). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
💡 Codex Reviewclient-rust-test/crates/parity-runner/src/run.rs Lines 155 to 159 in 800188c  Propagate transient errors before assertion failures
When a step with an ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codex review on PR #5 caught a real hazard in the precondition gate: a region_error on the asserted scan_locks step carries zero locks, so the lock_count assert reported 'setup failed' with transient hard-coded false — turning a retryable cluster event (a split or leader change mid-run) into a fatal scenario failure that never reached the retry loop. The assert is now evaluated only for ADMISSIBLE observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself: region_error stays transient (retried, bounded), driver_error stays fatal, and an OK-but-wrong observation still fails the precondition exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed — good catch. The hazard was real: Fixed in 26a3dc5 on #6 (the open harness-code PR; this PR is docs-only, the flagged code predates it): the precondition is now evaluated only for admissible observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself — |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 912b5ed280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review: crates.io metadata for tonic 0.14.6 and tonic-prost-build 0.14.6 declares rust-version = 1.88 (prost 0.14.4: 1.85), so W4's '1.85 as the goal' was already known-unreachable under the roadmap's own W2 — the proposed MSRV CI job would fail on day one. State the known number instead: the working target is 1.88 once W2 lands, with the CI job reporting the exact floor as dependencies move. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review on PR #5 caught a real hazard in the precondition gate: a region_error on the asserted scan_locks step carries zero locks, so the lock_count assert reported 'setup failed' with transient hard-coded false — turning a retryable cluster event (a split or leader change mid-run) into a fatal scenario failure that never reached the retry loop. The assert is now evaluated only for ADMISSIBLE observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself: region_error stays transient (retried, bounded), driver_error stays fatal, and an OK-but-wrong observation still fails the precondition exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6fd10cf6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…its own checklist Codex review: the rules doc says RE-2 (ServerIsBusy/MaxTimestampNotSynced propagated to callers instead of retried) still needs its upstream issue, and it is a MUST violation — but §9's filing list omitted it while carrying lower-priority filings. The list now leads with the MUST violations (async-commit wrong-commit-ts, RE-2, service GC safepoints) so the roadmap tracks every MUST gap it identifies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review on PR #5 caught a real hazard in the precondition gate: a region_error on the asserted scan_locks step carries zero locks, so the lock_count assert reported 'setup failed' with transient hard-coded false — turning a retryable cluster event (a split or leader change mid-run) into a fatal scenario failure that never reached the retry loop. The assert is now evaluated only for ADMISSIBLE observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself: region_error stays transient (retried, bounded), driver_error stays fatal, and an OK-but-wrong observation still fails the precondition exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codex review on PR #5 caught a real hazard in the precondition gate: a region_error on the asserted scan_locks step carries zero locks, so the lock_count assert reported 'setup failed' with transient hard-coded false — turning a retryable cluster event (a split or leader change mid-run) into a fatal scenario failure that never reached the retry loop. The assert is now evaluated only for ADMISSIBLE observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself: region_error stays transient (retried, bounded), driver_error stays fatal, and an OK-but-wrong observation still fails the precondition exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…luster profile) (#6) * Add G-0002 (raw checksum, the first capability claim) and the 3-store cluster profile The first roadmap-execution slice (docs/parity-roadmap.md §5.6 + §8): - parity protocol: open_raw_client / raw_put / raw_checksum commands, plus a ChecksumObs evidence field on Observation — recorded, never projected: crc64/total_bytes cover prefix-included key bytes and legitimately differ between the two runs of one comparison. - Go driver: rawkv client (same declared router-client deviation as txn), server-side Checksum answered with evidence attached. - Rust driver: raw client + raw_put; raw_checksum answers `unsupported` — client-rust has no such request (rules RAW-4), and the driver must not emulate one. - scenarios/raw-checksum.json + ledger G-0002: expect=diverges, class ok-vs-unsupported plus the unsupported.detail row, declared field-complete from the observed divergence report. - cluster/: a 3-store profile (pd-3store.toml raises max-replicas to 3; wait-ready.sh grows an expected-store-count argument) — the harness prerequisite for roadmap §5.4's replica/stale-read scenarios. Boot-verified; mutually exclusive with the single-node profile by ports. - PR template categories updated to the repo's actual shape (crates/, go/, scenarios/ + ledger.toml, docs/); roadmap §5.6/§8 and rules RAW-4 flipped from prospective to landed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * runner: an inadmissible observation cannot fail a precondition Codex review on PR #5 caught a real hazard in the precondition gate: a region_error on the asserted scan_locks step carries zero locks, so the lock_count assert reported 'setup failed' with transient hard-coded false — turning a retryable cluster event (a split or leader change mid-run) into a fatal scenario failure that never reached the retry loop. The assert is now evaluated only for ADMISSIBLE observations. An inadmissible one passes through to the end-of-run classification, which reports it as itself: region_error stays transient (retried, bounded), driver_error stays fatal, and an OK-but-wrong observation still fails the precondition exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What & why
Two documentation artifacts that turn the merged parity harness into a plan:
docs/parity-roadmap.md— the phased closure plan for client-rust's feature gap against client-go (in-flight fixes → dependency modernization → low-risk/high-impact plumbing → machinery → architectural projects), every claim citedfile:lineatpins.toml's pins.docs/client-rules.md— the normative baseline behind the ordering: 56 MUST/SHOULD/CAN rules grounded in the kvproto wire contract, client-rust graded per rule. Scoreboard: 5 MUST violations, 2 previously uncharacterized (async-commit silently commits at ts 0 on themin_commit_ts == 0fallback path;ServerIsBusy/MaxTimestampNotSyncedsurfaced to callers instead of retried — both to be filed upstream per §9).Also: findings header + README updated — #543/#545 are filed (fix PRs #544/#547 in flight), no longer "filing-ready".
Kind of change
src/,tests/)findings/)cluster/,Makefile)README.md)Checklist
make checkis green (fmt, clippy-D warnings, check)make gateagainst a real TiKV/PD cluster — N/A, docs onlyREADME.md"Gate verdict" / findings updated if behavior or evidence changedEvidence
Both documents were reviewed twice before this PR: a three-agent adversarial pass (citation fact-check against both pinned checkouts + crates.io + the GitHub tracker; technical-judgment attack; cross-document consistency attack) and
codex reviewat high reasoning effort. The ~20 verified findings are fixed in this commit — including a wrong failure-mode characterization (the async-commitunwrap()does not panic; it commits at a garbage ts 0), a mislabeled risk tier (replica read needs a selector core to be correct onDataIsNotReady), a workstream ordering with no real dependency (kvproto refresh runs on the existing codegen pipeline), one already-fixed upstream bug we nearly re-filed (#500, fixed by #501), and a batch of proto line references shifted by extraction-relative numbering. Rule-row count (56) and the scoreboard tallies are machine-checked against the tables.🤖 Generated with Claude Code