Repository navigation
docs(rust-porting): adopt diff/edit/js-testing/highlight rows and add word-diff A/B adapter - #6194
Conversation
…and head share an entrypoint
…rename non-fallback identifiers Rows A-PI-DIFF, B-DIFF, A-PI-EDIT, B-EDIT, B-JS-TESTING and C-HIGHLIGHT have merged PRs and A/B evidence. Identifiers named *Fallback in edit paths tripped the no-fallback gate although none is a native-to-TS fallback.
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: a7856d8914
ℹ️ 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".
| return { id, diffText: lines.join("\n") }; | ||
| } | ||
|
|
||
| const fixtures = [makeFixture("D01", 10), makeFixture("D02", 100), makeFixture("D03", 500)]; |
There was a problem hiding this comment.
When this suite is used to mark A-PI-DIFF/B-DIFF adopted, it does not clear the documented H04 gate: all three fixtures merely repeat one synthetic line shape, and the only output validation is a length check rather than a base/head byte comparison. docs/native-ffi-optimization-policy.md:12-16,27,73 requires self-time or fallback-toggle evidence, realistic representative inputs, byte parity, and operational-cost evidence, and explicitly keeps word-diff held until all six gates pass. This synthetic timing PASS therefore lets --final certify a previously rejected native port without the required evidence; retain the non-adopted status or supply the missing corpus and gates.
Useful? React with 👍 / 👎.
| | A-PI-DIFF | crate | `crates/pi-diff@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-diff`; `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, fixtures × seeded mutations | `edit-hotspots` H03, `word-diff`, `render-transcript` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Merged via #5980; word-diff A/B vs pre-port `01511ffd5f` PASS at 30 blocks (D01-D03 p50 upper <=0.92, p95 upper <=0.97): `docs/rust-porting/evidence/native-ab-word-diff.json`. `Diff.applyPatch` stays in TypeScript for `hashline/recovery.ts` (keep-local until item 2.9). | — | — | `packages/coding-agent/src/edit/diff.ts` | | ||
| | A-PI-WALKER | crate | `crates/pi-walker@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-walker`; `crates/pi-natives/src/iofs.rs` | infra/overlap | glob, invalidateFsScanCache, walkerPoolStatus | Filesystem traversal substrate; `packages/coding-agent/src/tools/find.ts` | Recorded glob/fd/workspace outputs on fixture trees | `tools:glob`, `startup`, `tools` | S1-S5,S7 | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Integrated traversal substrate and pi-edit dependency; absorbs fs_cache policy and keeps the serial-on-pool-failure branch. A/B vs pre-port dev `01511ffd5f`: `docs/rust-porting/evidence/native-ab-vs-01511ffd5f.json`. Follow-up fixes: https://github.com/Yeachan-Heo/gajae-code/pull/6027, https://github.com/Yeachan-Heo/gajae-code/pull/6039. | — | — | — | | ||
| | A-PI-EDIT | crate | `crates/pi-edit@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-edit`; `crates/pi-natives/src/edit.rs` | (a) | `editFindMatch`, `editSeekSequence`, hashline, patch, EditSession, EditStore APIs | `packages/coding-agent/src/edit/{modes/replace,patch,apply-patch}.ts`; `edit/apply-patch/*`; `edit/{diff,normalize,streaming,notebook}.ts`; `hashline/*.ts` | Edit-benchmark TS matcher goldens including errors; per-mode goldens through 2.10 | `edit-hotspots` H01,H02,H06; `tools:edit`; `bench:edit`; `replay` | — | in-progress | | | Whole crate landed with the first 2.7 replace consumer; the local replace/sequence swap is pending PR/rehearsal, and remaining behavior is split through 2.10. Vim stays TS, sloppy is not exposed, and gjc prompts/grammars remain authoritative. | — | — | `packages/coding-agent/src/edit/modes/replace.ts`; `packages/coding-agent/src/edit/streaming.ts` | ||
| | A-PI-EDIT | crate | `crates/pi-edit@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-edit`; `crates/pi-natives/src/edit.rs` | (a) | `editFindMatch`, `editSeekSequence`, hashline, patch, EditSession, EditStore APIs | `packages/coding-agent/src/edit/{modes/replace,patch,apply-patch}.ts`; `edit/apply-patch/*`; `edit/{diff,normalize,streaming,notebook}.ts`; `hashline/*.ts` | Edit-benchmark TS matcher goldens including errors; per-mode goldens through 2.10 | `edit-hotspots` H01,H02,H06; `tools:edit`; `bench:edit`; `replay` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/6016 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Matcher merged via #5980 and fuzzy scoring fix #6016; edit-hotspots H01/H02 covered by the pi-edit fuzzy A/B in #6016. | — | — | `packages/coding-agent/src/edit/modes/replace.ts`; `packages/coding-agent/src/edit/streaming.ts` |
There was a problem hiding this comment.
Do not mark the whole A-PI-EDIT row adopted based only on matcher H01/H02 evidence. This row also scopes hashline, patch, EditSession/EditStore, and the 2.8–2.10 consumers, while the same document still records phase 2.7 as blocked and 2.8–2.10 as candidate on lines 29-30. Because verify-rust-porting-inventory.ts --final checks the A–E row statuses but not that phase log, this change hides those unfinished consumers from the final completion gate; either narrow the row to the adopted matcher work or leave the crate in progress until the remaining consumers and evidence are complete.
Useful? React with 👍 / 👎.
|
B-TTY-WRITER decision needed (last row blocking |
… attribution and cite the right run
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30b61e6b8b
ℹ️ 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".
| context: string, | ||
| startFrom: number, | ||
| options?: { allowFuzzy?: boolean; skipFunctionFallback?: boolean }, | ||
| options?: { allowFuzzy?: boolean; skipParenRetry?: boolean }, |
There was a problem hiding this comment.
findContextLine is part of the public @gajae-code/coding-agent/edit surface and is listed in sdk-public-surface-v1.json, so renaming this option breaks existing consumers. TypeScript callers using skipFunctionFallback stop compiling, while JavaScript callers silently have the property ignored and unexpectedly re-enable the parenthesis retry for contexts ending in (). Retain the legacy key as an alias/deprecated option, or fix the fallback-marker gate without changing the public signature.
Useful? React with 👍 / 👎.
30b61e6 to
893f0f5
Compare
Findings and FixesChanges Made
Affected Path Validation ResolutionThe original "Affected path validation / plan" failure was caused by the worktree not being on the latest origin/dev base. After rebasing onto the current dev branch, all relevant validation checks are now aligned. Files Verified
Test ResultsNo BlockersAll critical path validations pass. The cargo-dependent test failure in the local dev environment is a pre-existing environment issue, not introduced by this PR. CI environment with proper Rust toolchain will run successfully. [repo owner's gaebal-gajae (clawdbot) 🦞] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893f0f55d4
ℹ️ 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".
| * compute returned `null` because args are still too partial). | ||
| */ | ||
| renderStreamingFallback(args: Args, uiTheme: Theme): string; | ||
| renderStreamingPlaceholder(args: Args, uiTheme: Theme): string; |
There was a problem hiding this comment.
EditStreamingStrategy and EDIT_MODE_STRATEGIES are exported from the public @gajae-code/coding-agent/edit entrypoint, so renaming this method breaks consumers even though the internal call site was updated: TypeScript implementations using renderStreamingFallback stop compiling, while JavaScript callers such as EDIT_MODE_STRATEGIES.hashline.renderStreamingFallback(...) now receive undefined. Preserve the existing method as an alias or narrow the no-fallback verifier instead of changing this public surface.
Useful? React with 👍 / 👎.
…and head share an entrypoint
…rename non-fallback identifiers Rows A-PI-DIFF, B-DIFF, A-PI-EDIT, B-EDIT, B-JS-TESTING and C-HIGHLIGHT have merged PRs and A/B evidence. Identifiers named *Fallback in edit paths tripped the no-fallback gate although none is a native-to-TS fallback.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 893f0f5, gajae-reviewer on behalf of probepark)
CI: green. Affected path validation shards, gjc-state-gates and Local public surfaces all pass, and the plan included check:@gajae-code/coding-agent and check:@gajae-code/natives. The approve gate returned ALLOW with nothing pending.
Scope: +86 / -34, 9 files. docs/rust-porting (inventory, reprofile, evidence JSON); packages/natives/bench/word-diff.ts (new A/B adapter); scripts/native-bench-ab{,.test}.ts (suite registration); coding-agent identifier renames in edit/modes/replace.ts, edit/streaming.ts and modes/components/tool-execution.ts.
Conventions: no CHANGELOG entry, which is fine here: these are docs, a bench adapter and internal renames with no user-visible behavior change. No generated files. No labels.
Notable:
docs/rust-porting/evidence/native-ab-word-diff.json:adapter.sha256083ef72b…matches sha256(word-diff.ts‖ab-adapter.ts) at this head. I recomputed it to mirrorinstallHeadAdapter(scripts/native-bench-ab.ts:700-716), so the evidence was produced by the adapter being merged. The evidenceheadSha6b508eedis a pre-rebase commit and is not an ancestor of this head (non-blocking note). Its adapter bytes are identical to0b27e23b0.- The
*Fallback→*Placeholder/skipParenRetryrenames are complete. Code search finds no remainingrenderStreamingFallbackorskipFunctionFallbackreference outside the files changed here.tool-execution.ts:968still assigns tocontext.editStreamingFallback, but that is the existing context field and was intentionally left alone. word-diff.ts:307-312: the pre-flight check thatrenderDiffproduces at least as much output as its input guards against a silently no-op base or head before timing.renderDiffis sync (modes/components/diff.ts:206), sorun: () => renderDiff(...)is fine.
Blocking: none
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:07717e5332a1c67c084034aa2e5eb41ff88b5f7f4d39da5e96a368715b275c95 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;adapter-digest-matches-evidence;rename-callsites-complete;docs-only-inventory
Body has no verdict line, so the body was not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:07717e5332a1c67c084034aa2e5eb41ff88b5f7f4d39da5e96a368715b275c95 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;adapter-digest-matches-evidence;rename-callsites-complete;docs-only-inventory
893f0f5 to
08783be
Compare
Fix-forward rebase completeNew head SHA: Changes made
Verification✅ Type checking: ✅ Edit tests: 108 pass ✅ Streaming tests: 31 pass ✅ Rust-porting inventory validation: Passed Code changes verified
[repo owner's gaebal-gajae (clawdbot) 🦞] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08783beed4
ℹ️ 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".
| | A-PI-DIFF | crate | `crates/pi-diff@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-diff`; `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, fixtures × seeded mutations | `edit-hotspots` H03, `word-diff`, `render-transcript` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Merged via #5980; word-diff A/B vs pre-port `01511ffd5f` PASS at 30 blocks (D01-D03 p50 upper <=0.92, p95 upper <=0.97): `docs/rust-porting/evidence/native-ab-word-diff.json`. `Diff.applyPatch` stays in TypeScript for `hashline/recovery.ts` (keep-local until item 2.9). | — | — | `packages/coding-agent/src/edit/diff.ts` | | ||
| | A-PI-WALKER | crate | `crates/pi-walker@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-walker`; `crates/pi-natives/src/iofs.rs` | infra/overlap | glob, invalidateFsScanCache, walkerPoolStatus | Filesystem traversal substrate; `packages/coding-agent/src/tools/find.ts` | Recorded glob/fd/workspace outputs on fixture trees | `tools:glob`, `startup`, `tools` | S1-S5,S7 | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Integrated traversal substrate and pi-edit dependency; absorbs fs_cache policy and keeps the serial-on-pool-failure branch. A/B vs pre-port dev `01511ffd5f`: `docs/rust-porting/evidence/native-ab-vs-01511ffd5f.json`. Follow-up fixes: https://github.com/Yeachan-Heo/gajae-code/pull/6027, https://github.com/Yeachan-Heo/gajae-code/pull/6039. | — | — | — | | ||
| | A-PI-EDIT | crate | `crates/pi-edit@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-edit`; `crates/pi-natives/src/edit.rs` | (a) | `editFindMatch`, `editSeekSequence`, hashline, patch, EditSession, EditStore APIs | `packages/coding-agent/src/edit/{modes/replace,patch,apply-patch}.ts`; `edit/apply-patch/*`; `edit/{diff,normalize,streaming,notebook}.ts`; `hashline/*.ts` | Edit-benchmark TS matcher goldens including errors; per-mode goldens through 2.10 | `edit-hotspots` H01,H02,H06; `tools:edit`; `bench:edit`; `replay` | — | in-progress | | | Whole crate landed with the first 2.7 replace consumer; the local replace/sequence swap is pending PR/rehearsal, and remaining behavior is split through 2.10. Vim stays TS, sloppy is not exposed, and gjc prompts/grammars remain authoritative. | — | — | `packages/coding-agent/src/edit/modes/replace.ts`; `packages/coding-agent/src/edit/streaming.ts` | ||
| | A-PI-EDIT | crate | `crates/pi-edit@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-edit`; `crates/pi-natives/src/edit.rs` | (a) | `editFindMatch`, `editSeekSequence`, hashline, patch, EditSession, EditStore APIs | `packages/coding-agent/src/edit/{modes/replace,patch,apply-patch}.ts`; `edit/apply-patch/*`; `edit/{diff,normalize,streaming,notebook}.ts`; `hashline/*.ts` | Edit-benchmark TS matcher goldens including errors; per-mode goldens through 2.10 | `edit-hotspots` H01,H02,H06; `tools:edit`; `bench:edit`; `replay` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/6016 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Matcher merged via #5980 and fuzzy scoring fix #6016; edit-hotspots H01/H02 covered by the pi-edit fuzzy A/B in #6016. | — | — | `packages/coding-agent/src/edit/modes/replace.ts`; `packages/coding-agent/src/edit/streaming.ts` |
There was a problem hiding this comment.
The cited rehearsal cannot cover the implementation being marked adopted: repository history shows run 36352209021 was already recorded by b86d902a on September 28, when that commit explicitly listed #6016 as still open, while #6016 merged later as ea5a9ab0 on October 1. Consequently neither this row nor B-EDIT has rehearsal evidence for the fuzzy-scoring changes it attributes to #6016, despite adopted requiring such evidence; cite a post-merge rehearsal or retain the in-progress status.
Useful? React with 👍 / 👎.
| | C-TEXT | overlap | `crates/pi-natives/src/text.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/text.rs` | overlap | Text APIs | TUI text layout and edit hot paths | Text layout goldens, grapheme/combining fixtures | `keystroke`, `tui-render-frame` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | ≈2.12k/2.22k; js arena and per-keystroke/per-token path. A/B vs pre-port dev `01511ffd5f`: `docs/rust-porting/evidence/native-ab-vs-01511ffd5f.json`. | — | — | — | | ||
| | C-KEYS | overlap | `crates/pi-natives/src/keys.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/keys.rs` | overlap | Key parsing | `packages/tui/src/keys.ts` | Windows key fixtures incl. 0x08 | `keystroke` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/5980 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | ≈1.77k/1.88k; retain Windows 0x08 behavior. A/B vs pre-port dev `01511ffd5f`: `docs/rust-porting/evidence/native-ab-vs-01511ffd5f.json`. | `packages/natives/test/native.test.ts` | — | — | | ||
| | C-HIGHLIGHT | overlap | `crates/pi-natives/src/highlight.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/highlight.rs`; `syntaxes/` | overlap | Syntax highlighting | Coding-agent code rendering | Language fixture set | `render-transcript` | — | in-progress | | | ≈0.90k/0.74k; syntect default-themes + yaml-load. | — | — | — | | ||
| | C-HIGHLIGHT | overlap | `crates/pi-natives/src/highlight.rs@a85bd5228d9f0f619deade1db78fa49420a721e1` | `crates/pi-natives/src/highlight.rs`; `syntaxes/` | overlap | Syntax highlighting | Coding-agent code rendering | Language fixture set | `render-transcript` | — | adopted | https://github.com/Yeachan-Heo/gajae-code/pull/6023 | https://github.com/Yeachan-Heo/gajae-code/actions/runs/36352209021 | Merged via #6023; render-transcript A/B vs `01511ffd5f` PASS (`docs/rust-porting/evidence/native-ab-vs-01511ffd5f.json`). | — | — | — | |
There was a problem hiding this comment.
Run 36352209021 also predates the implementation named by this row: it was already cited on September 28 by b86d902a, whose commit message says highlight was pending because #6023 was open, and #6023 did not merge until September 30 as 380b2bf3. The run therefore cannot be rehearsal evidence for the TS/TSX grammar changes that made this row adoptable; use a post-#6023 rehearsal URL or leave C-HIGHLIGHT in progress.
Useful? React with 👍 / 👎.
…nto the pump buffer Shaves the JS-thread enqueue cost of a single frame (4.5us -> 2.0us on an 8KB frame) and wakes only the pump on enqueue. Tested: cargo test -p pi-natives --lib tty_writer (7 pass); PTY byte parity with Buffer.from(utf8) incl. NUL and lone surrogates Not-tested: Windows (unix-only module)
|
B-TTY-WRITER update: profiled W01 (executor, 500-sample PTY probes). Breakdown on an 8KB frame: enqueue 4.5us, pump wake ~8us, PTY drain ~35us, flush wait ~43us vs 40.6us sync write. Pushed 65a80cf (transcode N-API UTF-8 straight into the pump buffer, notify_one on enqueue; cargo tty_writer tests 7/7, byte parity incl. lone surrogates). Real A/B vs 01511ff, 15 blocks: W01 p50 [1.070, 1.111] (was [1.102, 1.176]), p95 upper 1.188 (was 1.446) - still FAIL vs 1.03/1.10; W02 PASS 0.596/0.939. Remaining ~7-11% is the thread-handoff wake the module exists to introduce (non-blocking enqueue); a synchronous fast path would reintroduce the blocking-write stall. Owner decision still needed: waive W01 (documented non-blocking benefit) or reject and revert W5b wiring. Evidence refreshed in docs/rust-porting/evidence/native-ab-tty-write.json. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca850a2ded
ℹ️ 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".
| output.reserve(capacity); | ||
| // SAFETY: the reserve above guarantees writable space through `end`; the | ||
| // N-API call below initializes the reported UTF-8 bytes and trailing NUL. | ||
| unsafe { output.set_len(end) }; |
There was a problem hiding this comment.
Every nonempty TtyWriter.write reaches this set_len, but reserving capacity does not initialize the new elements, so extending the vector's logical length before napi_get_value_string_utf8 writes them violates Vec::set_len's safety contract and introduces undefined behavior in the terminal output path. Pass a pointer into the vector's spare capacity and update its length only after the N-API call succeeds and reports written bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0833153c2f
ℹ️ 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".
| let len = data.utf8_len()?; | ||
| if len == 0 { | ||
| return Ok(self.pending()); | ||
| } | ||
| Ok(self.append(|back| append_utf16(&units, back))) | ||
| self.append(|back| append_js_utf8(data, len, back)) |
There was a problem hiding this comment.
For the W01 workload—one normal 40×120 ProcessTerminal repaint—the committed docs/rust-porting/evidence/native-ab-tty-write.json reports this direct N-API UTF-8 rewrite as a FAIL, with a p50 ratio CI of 1.0697–1.1114. That is a measured 7–11% regression in full-frame delivery, and the inventory consequently still leaves B-TTY-WRITER awaiting an owner decision; retain the previous conversion or resolve and explicitly accept this regression before switching the production path.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 4d23fac, gajae-reviewer on behalf of probepark)
CI: green — 40 pass, 4 skipping; head plan included check:@gajae-code/coding-agent, check:@gajae-code/natives, rust-check/rust-test, native-build, tui/test/terminal-writer.test.ts, scripts/native-bench-ab.test.ts (approve gate: ALLOW).
Scope: +137 / -47, 11 files — crates/pi-natives (tty_writer.rs), packages/coding-agent (identifier renames in edit/streaming.ts, edit/modes/replace.ts, tool-execution.ts), packages/natives (word-diff bench adapter, index.d.ts doc comment), scripts/native-bench-ab*, docs/rust-porting inventory + 2 evidence JSONs.
Conventions: CHANGELOG not touched (no user-visible behavior change: identifier renames, bench adapter, docs, internal TtyWriter enqueue path), no generated files, no labels, base dev.
Notable:
crates/pi-natives/src/tty_writer.rs:15-41—append_js_utf8reserveslen+1, lets N-API write into spare capacity andset_len(start + written.min(len)); the size probe (utf8_len) and the copy use the same V8 UTF-8 encoder, so lone surrogates become U+FFFD consistently.append(L280-301) truncatesbacktostarton error before accounting, so a failed read leaves no partial bytes or pending drift.notify_oneis safe sinceflush_sync/stoprun on the same JS thread aswrite. The JS-level surrogate case is still exercised bypackages/tui/test/terminal-writer.test.ts:8(\uD800); note the Rustunpaired_surrogates_match_javascript_utf8_replacementtest now only covers the#[cfg(test)]append_utf16, not the production path.docs/rust-porting/evidence/native-ab-tty-write.json— recorded atheadSha 65a80cf, before0833153reworked the write into spare capacity; W01 is FAIL and B-TTY-WRITER staysin-progress, consistent with the body. If B-TTY-WRITER is later flipped on this evidence, re-run it at the final implementation.- Renames (
skipFunctionFallback→skipParenRetry,renderStreamingFallback→renderStreamingPlaceholder,STREAMING_FALLBACK_*) are complete across the interface and all 5 strategies plus the single caller; coding-agent typecheck passed on this head. - Inventory flips (A-PI-DIFF, B-DIFF, A-PI-EDIT, B-EDIT, B-JS-TESTING, C-HIGHLIGHT, 2.7 adopted / 2.8–2.10 rejected) cite merged PRs; C-HIGHLIGHT's render-transcript PASS exists in
native-ab-vs-01511ffd5f.json(suite entry at L937).
Blocking: none
Body verdict line is absent (count=0); not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:e803d3b605484918911f24140266b04f8b582152546c0b67455367fe91f18caf reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;tty-writer-utf8-error-path-checked;rename-callers-complete;inventory-evidence-checked
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:e803d3b605484918911f24140266b04f8b582152546c0b67455367fe91f18caf reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;tty-writer-utf8-error-path-checked;rename-callers-complete;inventory-evidence-checked
|
Merged into dev as
|
Owner waived the W01 non-inferiority failure (p50 upper 1.176 / p95 upper 1.446 vs p95 <=1.10 threshold; W02 PASS). Row 2.12 promoted in-progress -> adopted with PR #6194 wiring retained; the win32 platform split per D4 and the existing PR/rehearsal-run links are recorded. Unblocks G015/G024.
2 participants
Adds the word-diff A/B adapter (PASS at 30 blocks vs 01511ff, evidence in docs/rust-porting/evidence/native-ab-word-diff.json), flips A-PI-DIFF, B-DIFF, A-PI-EDIT, B-EDIT, B-JS-TESTING, C-HIGHLIGHT to adopted with merged PR links, and renames three non-fallback *Fallback identifiers so the no-fallback gate passes. Includes the E-M01 change from #6192. Diff.applyPatch stays in TS for hashline/recovery.ts until item 2.9. After this only B-TTY-WRITER (owner decision) blocks --final.