Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 53 additions & 15 deletions crates/pi-natives/src/tty_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,39 @@ use std::{
time::{Duration, Instant},
};

use napi::{Error, JsString, Result};
use napi::{Error, JsString, JsValue, Result};
use napi_derive::napi;
use parking_lot::{Condvar, Mutex};

use crate::js;
fn append_js_utf8(data: JsString<'_>, len: usize, output: &mut Vec<u8>) -> Result<usize> {
let raw = data.value();
let capacity = len
.checked_add(1)
.ok_or_else(|| Error::from_reason("terminal string is too large"))?;
output.reserve(capacity);
let start = output.len();
let mut written = 0;
// SAFETY: `reserve` guarantees `capacity` writable bytes of spare capacity
// past `start`; `data` is a live JS string in this callback and `capacity`
// is its exact UTF-8 length plus the required NUL slot.
let status = unsafe {
napi::sys::napi_get_value_string_utf8(
raw.env,
raw.value,
output.spare_capacity_mut().as_mut_ptr().cast(),
capacity,
&mut written,
)
};
napi::check_status!(status, "Failed to read JavaScript string")?;
// SAFETY: N-API initialized `written` bytes (`<= capacity - 1`) of the spare
// capacity starting at `start`, so extending the length covers only
// initialized bytes.
unsafe { output.set_len(start + written.min(len)) };
Ok(written.min(len))
}

#[cfg(test)]
fn append_valid_utf16_segment(units: &[u16], output: &mut Vec<u8>) {
if units.is_empty() {
return;
Expand All @@ -53,6 +80,7 @@ fn append_valid_utf16_segment(units: &[u16], output: &mut Vec<u8>) {

/// Append UTF-16 as JavaScript's UTF-8 encoding does: valid surrogate pairs
/// become their scalar, while each unpaired surrogate becomes U+FFFD.
#[cfg(test)]
fn append_utf16(units: &[u16], output: &mut Vec<u8>) -> usize {
let start = output.len();
let mut segment_start = 0usize;
Expand Down Expand Up @@ -234,34 +262,42 @@ impl TtyWriter {
/// Enqueue terminal output; never blocks. Returns the total bytes now
/// pending (including this chunk).
///
/// Reads the JS string as UTF-16 and transcodes it with `xutf` straight into
/// the shared back buffer.
/// Reads the JS string as UTF-8 directly into the shared back buffer.
#[napi]
pub fn write(&self, data: JsString) -> Result<u32> {
if self.inner.dead.load(Ordering::Acquire) {
return Ok(self.pending());
}
let units = js::utf16(data)?;
if units.is_empty() {
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))
Comment on lines +271 to +275

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not ship the rewrite after its full-frame gate fails

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 👍 / 👎.

}

/// Append into the back buffer under its lock, account the added bytes,
/// and wake the pump. `fill` returns the byte count it appended.
fn append(&self, fill: impl FnOnce(&mut Vec<u8>) -> usize) -> u32 {
fn append(&self, fill: impl FnOnce(&mut Vec<u8>) -> Result<usize>) -> Result<u32> {
{
let mut back = self.inner.back.lock();
let added = fill(&mut back);
let start = back.len();
let added = match fill(&mut back) {
Ok(added) => added,
Err(error) => {
back.truncate(start);
return Err(error);
},
};
// Publish the pending-byte accounting while `back` is still locked.
// Once this lock is released, the pump may claim and drain the buffer;
// accounting afterward lets its `fetch_sub` win the race and underflow
// the counter, permanently pinning JS-side render backpressure on.
self.inner.pending.fetch_add(added, Ordering::AcqRel);
}
self.inner.cv.notify_all();
self.pending()
// `write` and `flush_sync` run serially on this writer's JS thread, so
// the pump is the only possible waiter while a frame is enqueued.
self.inner.cv.notify_one();
Ok(self.pending())
}

/// Bytes accepted but not yet written to the terminal.
Expand Down Expand Up @@ -372,10 +408,12 @@ mod tests {
}

fn push(writer: &TtyWriter, data: &[u8]) {
writer.append(|back| {
back.extend_from_slice(data);
data.len()
});
writer
.append(|back| {
back.extend_from_slice(data);
Ok(data.len())
})
.unwrap();
}

fn pipe_pair() -> (i32, i32) {
Expand Down
16 changes: 8 additions & 8 deletions docs/rust-porting-inventory.md

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions docs/rust-porting/evidence/native-ab-tty-write.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions docs/rust-porting/evidence/native-ab-word-diff.json

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions packages/coding-agent/src/edit/modes/replace.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* Fuzzy matching utilities for the edit tool.
*
* Provides both character-level and line-level fuzzy matching with progressive
* fallback strategies for finding text in files.
* Provides both character-level and line-level fuzzy matching with staged
* strategies for finding text in files.
*/
import type { AgentToolResult } from "@gajae-code/agent-core";
import { markDesignedError } from "@gajae-code/utils/error-classification";
Expand Down Expand Up @@ -430,7 +430,7 @@ export function findContextLine(
lines: string[],
context: string,
startFrom: number,
options?: { allowFuzzy?: boolean; skipFunctionFallback?: boolean },
options?: { allowFuzzy?: boolean; skipParenRetry?: boolean },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the exported option name

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 👍 / 👎.

): ContextLineResult {
const allowFuzzy = options?.allowFuzzy ?? true;
const trimmedContext = context.trim();
Expand Down Expand Up @@ -570,14 +570,14 @@ export function findContextLine(
};
}

if (!options?.skipFunctionFallback && trimmedContext.endsWith("()")) {
if (!options?.skipParenRetry && trimmedContext.endsWith("()")) {
const withParen = trimmedContext.replace(/\(\)\s*$/u, "(");
const withoutParen = trimmedContext.replace(/\(\)\s*$/u, "");
const parenResult = findContextLine(lines, withParen, startFrom, { allowFuzzy, skipFunctionFallback: true });
const parenResult = findContextLine(lines, withParen, startFrom, { allowFuzzy, skipParenRetry: true });
if (parenResult.index !== undefined || (parenResult.matchCount ?? 0) > 0) {
return parenResult;
}
return findContextLine(lines, withoutParen, startFrom, { allowFuzzy, skipFunctionFallback: true });
return findContextLine(lines, withoutParen, startFrom, { allowFuzzy, skipParenRetry: true });
}

return { index: undefined, confidence: bestScore };
Expand Down
30 changes: 15 additions & 15 deletions packages/coding-agent/src/edit/streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* - compute unified diff previews for the in-flight args
* (`computeDiffPreview`), and
* - render a text placeholder while no diff exists yet
* (`renderStreamingFallback`).
* (`renderStreamingPlaceholder`).
*
* The shared renderer / `ToolExecutionComponent` consult the strategy via
* the injected `editMode` rather than probing argument shape.
Expand Down Expand Up @@ -72,7 +72,7 @@ export interface EditStreamingStrategy<Args = unknown> {
* Rendered inline while the diff hasn't been computed yet (or when the
* compute returned `null` because args are still too partial).
*/
renderStreamingFallback(args: Args, uiTheme: Theme): string;
renderStreamingPlaceholder(args: Args, uiTheme: Theme): string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the exported strategy method name

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 👍 / 👎.

}
export interface EditRequestTargetInventory {
paths: string[];
Expand Down Expand Up @@ -169,7 +169,7 @@ export function getEditRequestTargetInventory(
if (editMode === "hashline") {
const input = typeof values.input === "string" ? values.input : "";
const headerPaths = getHashlineTargetPaths(input);
// `path` is only a fallback for headerless input; explicit §PATH headers are the full target set.
// `path` is only the target for headerless input; explicit §PATH headers are the full target set.
if (headerPaths.length > 0) return { paths: orderedDistinctPaths(headerPaths) };
return { paths: orderedDistinctPaths([topLevelPath]) };
}
Expand Down Expand Up @@ -197,8 +197,8 @@ export function getEditRequestTargetInventory(
return { paths: orderedDistinctPaths([topLevelPath, ...editPaths, partialPath]) };
}

const STREAMING_FALLBACK_LINES = 12;
const STREAMING_FALLBACK_WIDTH = 80;
const STREAMING_PLACEHOLDER_LINES = 12;
const STREAMING_PLACEHOLDER_WIDTH = 80;

function isHashlineHeaderLine(line: string): boolean {
return line.trimEnd().startsWith(HL_FILE_PREFIX);
Expand Down Expand Up @@ -236,15 +236,15 @@ function trimHashlineStreamingSyntax(lines: string[]): string[] {
return lines.slice(index).filter(line => !isHashlineEnvelopeMarkerLine(line));
}

function renderHashlineInputFallback(input: string, uiTheme: Theme): string {
function renderHashlineInputPlaceholder(input: string, uiTheme: Theme): string {
const lines = trimHashlineStreamingSyntax(sanitizeText(input).split("\n"));
if (!lines.some(line => line.trim().length > 0)) return "";

const displayLines = lines.slice(-STREAMING_FALLBACK_LINES);
const displayLines = lines.slice(-STREAMING_PLACEHOLDER_LINES);
const hidden = lines.length - displayLines.length;
let text = "\n\n";
text += displayLines
.map(line => uiTheme.fg("toolOutput", truncateToWidth(replaceTabs(line), STREAMING_FALLBACK_WIDTH)))
.map(line => uiTheme.fg("toolOutput", truncateToWidth(replaceTabs(line), STREAMING_PLACEHOLDER_WIDTH)))
.join("\n");
if (hidden > 0) {
text += uiTheme.fg("dim", `\n… (streaming +${hidden} lines)`);
Expand Down Expand Up @@ -376,7 +376,7 @@ const replaceStrategy: EditStreamingStrategy<ReplaceArgs> = {
ctx.signal.throwIfAborted();
return [toPerFilePreview(args.path, result)];
},
renderStreamingFallback() {
renderStreamingPlaceholder() {
return "";
},
};
Expand Down Expand Up @@ -405,7 +405,7 @@ const patchStrategy: EditStreamingStrategy<PatchArgs> = {
ctx.signal.throwIfAborted();
return [toPerFilePreview(args.path, result)];
},
renderStreamingFallback() {
renderStreamingPlaceholder() {
return "";
},
};
Expand Down Expand Up @@ -552,7 +552,7 @@ const hashlineStrategy: EditStreamingStrategy<HashlineArgs> = {
try {
sections = splitHashlineInputs(input, { cwd: ctx.cwd, path: args.path });
} catch {
// Single-section fallback keeps the original error rendering for the
// Single-section handling keeps the original error rendering for the
// "haven't typed `§ PATH` yet" case.
const result = await computeHashlineDiff({ input, path: args.path }, ctx.cwd, {
autoDropPureInsertDuplicates: ctx.hashlineAutoDropPureInsertDuplicates,
Expand Down Expand Up @@ -590,8 +590,8 @@ const hashlineStrategy: EditStreamingStrategy<HashlineArgs> = {
}
return previews.length > 0 ? previews : null;
},
renderStreamingFallback(args, uiTheme) {
return typeof args.input === "string" ? renderHashlineInputFallback(args.input, uiTheme) : "";
renderStreamingPlaceholder(args, uiTheme) {
return typeof args.input === "string" ? renderHashlineInputPlaceholder(args.input, uiTheme) : "";
},
};

Expand Down Expand Up @@ -642,7 +642,7 @@ const applyPatchStrategy: EditStreamingStrategy<ApplyPatchArgs> = {
}
return previews.length > 0 ? previews : null;
},
renderStreamingFallback() {
renderStreamingPlaceholder() {
return "";
},
};
Expand All @@ -656,7 +656,7 @@ const vimStrategy: EditStreamingStrategy<unknown> = {
async computeDiffPreview() {
return null;
},
renderStreamingFallback() {
renderStreamingPlaceholder() {
return "";
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ export class ToolExecutionComponent extends Container {
if (this.#expanded && !previews?.some(preview => preview.diff)) {
const editMode = this.#editMode;
const strategy = editMode ? EDIT_MODE_STRATEGIES[editMode] : undefined;
const fallback = strategy?.renderStreamingFallback(this.#args, theme);
const fallback = strategy?.renderStreamingPlaceholder(this.#args, theme);
if (fallback) context.editStreamingFallback = fallback;
}
context.renderDiff = renderDiff;
Expand Down
48 changes: 48 additions & 0 deletions packages/natives/bench/word-diff.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// A/B adapter for word-level intra-line diff rendering.
//
// The A/B runner installs this file into the base worktree too, so both sides
// time the same fixtures through the same public entrypoint (`renderDiff`),
// which computes a word diff for every adjacent removed/added line pair. Only
// the implementation behind it differs (jsdiff on base, native on head).
import { Settings } from "../../coding-agent/src/config/settings";
import { renderDiff } from "../../coding-agent/src/modes/components/diff";
import { initTheme } from "../../coding-agent/src/modes/theme/theme";
import { runAbSuite } from "./ab-adapter";

await initTheme("dark");
await Settings.init({ inMemory: true, cwd: process.cwd() });

interface Fixture {
id: string;
diffText: string;
}

function makeFixture(id: string, pairCount: number): Fixture {
const lines: string[] = [];
for (let index = 0; index < pairCount; index++) {
const base = `const record${index} = compute(alpha, amber, gamma, delta, epsilon, zeta, eta, theta, iota, kappa, ${index});`;
const edited = base.replace("amber", "violet").replace("theta", `theta${index % 7}`);
const lineNo = String(index + 1).padStart(4, " ");
lines.push(`-${lineNo}|${base}`);
lines.push(`+${lineNo}|${edited}`);
}
return { id, diffText: lines.join("\n") };
}

const fixtures = [makeFixture("D01", 10), makeFixture("D02", 100), makeFixture("D03", 500)];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep native word-diff held until all FFI gates pass

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 👍 / 👎.


for (const fixture of fixtures) {
const rendered = await renderDiff(fixture.diffText);
if (typeof rendered !== "string" || rendered.length < fixture.diffText.length) {
throw new Error(`${fixture.id}: renderDiff produced unexpectedly short output`);
}
}

await runAbSuite(
"word-diff",
fixtures.map(fixture => ({
id: fixture.id,
run: () => renderDiff(fixture.diffText),
})),
20,
);
3 changes: 1 addition & 2 deletions packages/natives/native/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -615,8 +615,7 @@ export declare class TtyWriter {
* Enqueue terminal output; never blocks. Returns the total bytes now
* pending (including this chunk).
*
* Reads the JS string as UTF-16 and transcodes it with `xutf` straight into
* the shared back buffer.
* Reads the JS string as UTF-8 directly into the shared back buffer.
*/
write(data: string): number
/** Bytes accepted but not yet written to the terminal. */
Expand Down
2 changes: 2 additions & 0 deletions scripts/native-bench-ab.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ describe("native bench A/B contract", () => {
});
expect(parseNativeBenchOptions(["--suite", "grep", "--base", "main", "--rss", "S1,S3,S7", "--allow-baseline-drift"]).rss).toEqual(["S1", "S3", "S7"]);
expect(parseNativeBenchOptions(["--suite", "rss", "--base", "HEAD", "--calibrate"]).suite).toBe("rss");
expect(parseNativeBenchOptions(["--suite", "word-diff", "--base", "HEAD"]).suite).toBe("word-diff");
expect(() => parseNativeBenchOptions(["--suite", "edit-hotspots"])).toThrow("--base");
});
test("uses a suite's default iterations unless --iterations is explicit", () => {
expect(parseNativeBenchOptions(["--suite", "builtins", "--base", "HEAD"]).iterations).toBe(20);
expect(parseNativeBenchOptions(["--suite", "builtins", "--base", "HEAD", "--iterations", "50"]).iterations).toBe(50);
expect(parseNativeBenchOptions(["--suite", "grep", "--base", "HEAD"]).iterations).toBe(200);
expect(parseNativeBenchOptions(["--suite", "word-diff", "--base", "HEAD"]).iterations).toBe(20);
expect(() => parseNativeBenchOptions(["--suite", "builtins", "--base", "HEAD", "--iterations", "0"])).toThrow(
"--iterations",
);
Expand Down
1 change: 1 addition & 0 deletions scripts/native-bench-ab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const SUITES: Record<
{ adapter: string; actualSuite: string; cases: string[]; support?: string[]; defaultIterations?: number }
> = {
"edit-hotspots": { adapter: "packages/natives/bench/edit-hotspots.ts", actualSuite: "edit-hotspots", cases: ["H01", "H02", "H03", "H06"] },
"word-diff": { adapter: "packages/natives/bench/word-diff.ts", actualSuite: "word-diff", cases: ["D01", "D02", "D03"], defaultIterations: 20 },
grep: { adapter: "packages/natives/bench/grep.ts", actualSuite: "grep", cases: ["G01", "G02", "G03", "G04", "G05", "G06", "G07", "G08"] },
"natives-grep": { adapter: "packages/natives/bench/grep.ts", actualSuite: "grep", cases: ["G01", "G02", "G03", "G04", "G05", "G06", "G07", "G08"] },
"render-transcript": { adapter: "packages/natives/bench/render-transcript.ts", actualSuite: "render-transcript", cases: ["R01", "R02", "R03"] },
Expand Down
Loading