docs(rust-porting): commit reproducible D5 evidence for the pi-vcs rejection - #6116
Conversation
…jection The B-VCS rejection cited numbers that only existed in the ledger. A fresh measurement (upstream a85bd522 vendored into a scratch dev tree, 15 ABAB blocks x 20 iterations per case, paired bootstrap) is now committed. The size gate and the aggregate read win pass (+5.79 MB gz; geometric-mean p50 ratio 0.240, CI upper 0.241), but the row still cannot be adopted: status regresses on both repositories (1.141x, CI [1.121, 1.162]; 1.331x, CI [1.288, 1.375]), which fails the plan's non-inferiority rule for the status-line suite, and native `show` output is not byte-identical to the git CLI output the TS consumers parse. The row stays rejected with this reason; a partial adoption (diff/log native, status/show on the CLI) needs an owner D5 decision. Confidence: high Scope-risk: narrow Tested: verify-rust-porting-inventory (schema valid); verify-rust-porting-inventory.test.ts
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 579af7049b
ℹ️ 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".
| "interleaving": "ABAB per iteration: TypeScript CLI base then native head; each case has 15 blocks x 20 paired iterations.", | ||
| "timerScope": "Native VcsGitRepo handles were discovered once before timing; each sampled operation call and its JS wrapper cost were included. Native show Buffer-to-UTF-8 decoding is included in its timed call.", | ||
| "blocksPerCase": 15, | ||
| "iterationsPerBlock": 20, |
There was a problem hiding this comment.
This evidence records only 20 iterations per block and no warmup or p95 results, while the shared A/B gate in docs/rust-porting-inventory.md specifies 15×200 measured iterations after warmup and requires paired-bootstrap results for both p50 and p95. Consequently, the artifact cannot be treated as a conforming D5 run—particularly for any future decision to adopt the apparently faster diff/log subset—without rerunning it using the documented protocol.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,338 @@ | |||
| { | |||
| "upstreamSha": "a85bd5228d9f0f619deade1db78fa49420a721e1", | |||
| "devSha": "d73818b6ac3ca39ffeab133f4df99b35509490ae", | |||
There was a problem hiding this comment.
The referenced devSha is not the measured native candidate: inspecting d73818b6ac3ca39ffeab133f4df99b35509490ae shows that its tree contains neither crates/pi-vcs nor crates/pi-natives/src/vcs.rs. Because the scratch-tree patch, benchmark harness, and candidate binary hashes are also absent, another checkout cannot reconstruct which code produced the native timings or the 5.79 MB addon delta; record a commit/tree or a complete reproducible patch for the measured candidate.
Useful? React with 👍 / 👎.
| "name": "large-worktree.status", | ||
| "repository": "large-worktree", | ||
| "operation": "status", | ||
| "cliCommand": "git status --porcelain", |
There was a problem hiding this comment.
The recorded baseline command omits the behavior of the TypeScript path being evaluated: packages/coding-agent/src/utils/git.ts runs read-only calls with --no-optional-locks plus -c core.fsmonitor=false and -c core.untrackedCache=false, whereas this artifact says it timed plain git status --porcelain (and similarly simplified commands for the other cases). Those cache settings can materially change status latency, so the reported 1.14×–1.33× regression does not establish performance against the CLI invocation that production actually pays for; invoke the wrapper or include all of its flags.
Useful? React with 👍 / 👎.
| | B-DIFF | native module | `crates/pi-natives/src/diff.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/diff.rs` | (a) | diffLines, diffLineRuns, structuredPatchHunks, diffWords, DiffStream | `packages/coding-agent/src/edit/diff.ts`; `modes/components/diff.ts`; `tools/vim.ts`; `eval/js/shared/helpers.ts`; `hashline/recovery.ts` | jsdiff oracle and seeded mutations | `edit-hotspots` H03, `word-diff`, `render-transcript` | — | in-progress | | | Part of pi-diff item 2.2. | — | — | `packages/coding-agent/src/edit/diff.ts` | | ||
| | B-EDIT | native module | `crates/pi-natives/src/edit.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/edit.rs` | (a) | `editFindMatch`, `editSeekSequence` | `packages/coding-agent/src/edit/modes/replace.ts`; `packages/coding-agent/src/edit/modes/patch.ts`; `packages/coding-agent/src/edit/diff.ts` | Edit-benchmark TS matcher goldens including errors and every confidence | `edit-hotspots` H01,H02 | — | in-progress | | | The 2.7 matcher consumer is implemented locally with an awaited first-use native load and no TS fuzzy fallback; status remains in-progress pending PR/rehearsal. UTF-16 scoring parity is preserved by D-PI-EDIT-UTF16-SCORING. The pinned edit.rs has no matcher wrappers; description/grammar/sloppy exports stay unexposed per D9. | — | — | `packages/coding-agent/src/edit/modes/replace.ts` | ||
| | B-VCS | native module | `crates/pi-natives/src/vcs.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/vcs.rs` | (a), size-gated (D5) | VCS read APIs | `packages/coding-agent/src/utils/git.ts`; listed git callers | Git CLI over fixture repositories | `status-line`, `tools:git` | — | rejected | | | D5 perf gate: large-worktree read suite upper-CI p50 ratio was 0.826 (needs <0.80), and status was 1.20x slower. | — | — | — | | ||
| | B-VCS | native module | `crates/pi-natives/src/vcs.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/vcs.rs` | (a), size-gated (D5) | VCS read APIs | `packages/coding-agent/src/utils/git.ts`; listed git callers | Git CLI over fixture repositories | `status-line`, `tools:git` | — | rejected | | | D5: addon +5.79 MB and aggregate read ratio 0.240 pass, but status regresses (1.14x-1.33x, CI lower >1.12) and native `show` output differs from the git CLI the TS parses. Evidence: `docs/rust-porting/evidence/2.17-pi-vcs-d5.json`. Partial adoption (diff/log native, status/show CLI) needs an owner D5 decision. | — | — | — | |
There was a problem hiding this comment.
Updating only B-VCS leaves A-PI-VCS, the crate row for the same pi-vcs/vcs.rs candidate, claiming the old 0.826 aggregate and 1.20× status result with no evidence link. Inventory readers now receive contradictory D5 explanations depending on whether they inspect the crate or native-module row; update A-PI-VCS to reference this evidence and the same current rejection rationale.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 579af70, gajae-reviewer on behalf of probepark)
CI: green. Approve gate returned ALLOW with 0 pending and 0 failed. Merge approval bootstrap is only waiting on the verdict gate. Planned jobs passed: Affected path validation (plan, native-build, docs-index-lazy test, evidence producer), Virtual integration validation, gjc-state-gates, PR contract bootstrap.
Scope: +339 / -1, 2 files, docs only: docs/rust-porting-inventory.md (B-VCS row reason) and the new docs/rust-porting/evidence/2.17-pi-vcs-d5.json.
Conventions: no CHANGELOG entry, which is fine for a docs-only change with no user-facing effect. No generated files. No labels. The row stays rejected. The table cell has no |, so the column count is unchanged.
Notable:
docs/rust-porting/evidence/2.17-pi-vcs-d5.json: I recomputed the numbers from the head blob. EachheadOverBaseP50equalsp50Ms.head/p50Ms.base. The geometric mean of the 8 ratios is 0.2398066567, which matchesaggregate.headOverBaseP50.addonGzipBytes.after - before == delta(5,786,020 < 8,000,000). The status CI lower bounds (1.121 and 1.288) support the row text "CI lower >1.12".- Aggregate: the 8-case geomean includes both
showcases, even thoughequivalent:false. If you keep only the output-equivalent cases, the geomean is 0.363, still below 0.80. The "aggregate read ratio passes" claim therefore holds either way, and the rejection correctly rests on status and show. (Note, not blocking.) repositories.*.pathrecords absolute local paths (/Users/bellman/...). The small fixture has arecipe. The large worktree can only be reproduced fromheadSha 804436c, so it could be worth stating that explicitly. The file has no trailing newline. (Notes, not blocking.)
Blocking: none.
Body verdict line is owned by pending (author line); not edited. The author's sha256 already matches the exact-head digest I computed (b20b00e6…686e). Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:b20b00e620808e37b6892ccb8bd3a1b6b7be4fc49ff0bd229dfc0a7af208686e reviewer:human reviewer-id:probepark evidence:ci-green;docs-only;evidence-json-ratios-recomputed;approve-gate-allow
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:b20b00e620808e37b6892ccb8bd3a1b6b7be4fc49ff0bd229dfc0a7af208686e reviewer:human reviewer-id:probepark evidence:ci-green;docs-only;evidence-json-ratios-recomputed;approve-gate-allow
2 participants
What
docs/rust-porting/evidence/2.17-pi-vcs-d5.json, a reproducible D5 measurement for pi-vcs. It includes:rejected.Why
B-VCS was rejected on numbers that existed only in the ultragoal ledger. The fresh measurement vendors upstream
a85bd522into a scratch dev tree and compares each native read path against the git CLI command the TS runs today.The gzipped addon grows by 5.79 MB, within the 8 MB budget. The aggregate read-suite ratio (geometric mean) is 0.240, with a CI upper bound of 0.241, well below 0.80. On those two D5 criteria alone, the row would be adopted.
It stays rejected for two reasons:
status-linesuite.showoutput differs. Nativeshowoutput is not byte-identical to the git CLI output that the TS consumers parse.Adopting only diff and log natively, while keeping status and show on the CLI, is a different scope and needs an owner D5 decision.
Testing
bun scripts/verify-rust-porting-inventory.tsreports the schema valid.bun test scripts/verify-rust-porting-inventory.test.tspasses.Risk classification
low-riskregression-riskhigh-riskGJC verdict
devpackages/<pkg>/changelog.d/(if user-facing)