fix: close PI-090 review findings
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { spawn } from 'node:child_process';
|
||||
import { readFile, stat } from 'node:fs/promises';
|
||||
import { open, stat } from 'node:fs/promises';
|
||||
import path from 'node:path';
|
||||
|
||||
const MAX_CHANGED_FILES = 200;
|
||||
@@ -72,7 +72,7 @@ interface StatusEntry {
|
||||
}
|
||||
|
||||
interface FileState extends StatusEntry {
|
||||
content: string;
|
||||
identity: string;
|
||||
preview?: string;
|
||||
truncated?: boolean;
|
||||
}
|
||||
@@ -152,7 +152,7 @@ function boundedText(value: string, maxBytes: number): { text: string; truncated
|
||||
async function untrackedPreview(
|
||||
projectPath: string,
|
||||
relativePath: string,
|
||||
): Promise<{ preview?: string; content: string; truncated?: boolean }> {
|
||||
): Promise<{ preview?: string; identity: string; truncated?: boolean }> {
|
||||
const target = path.resolve(projectPath, ...relativePath.split('/'));
|
||||
const relative = path.relative(path.resolve(projectPath), target);
|
||||
if (!relative || relative.startsWith('..') || path.isAbsolute(relative)) {
|
||||
@@ -160,19 +160,44 @@ async function untrackedPreview(
|
||||
}
|
||||
try {
|
||||
const file = await stat(target);
|
||||
if (!file.isFile()) return { content: 'not-file' };
|
||||
const data = await readFile(target);
|
||||
const bounded = data.subarray(0, MAX_UNTRACKED_PREVIEW_BYTES);
|
||||
if (bounded.includes(0)) return { content: `binary:${data.byteLength}` };
|
||||
if (!file.isFile()) return { identity: `not-file:${file.mtimeMs}` };
|
||||
const byteLength = Math.min(file.size, MAX_UNTRACKED_PREVIEW_BYTES);
|
||||
const data = Buffer.alloc(byteLength);
|
||||
const handle = await open(target, 'r');
|
||||
let bytesRead = 0;
|
||||
try {
|
||||
({ bytesRead } = await handle.read(data, 0, byteLength, 0));
|
||||
} finally {
|
||||
await handle.close();
|
||||
}
|
||||
const bounded = data.subarray(0, bytesRead);
|
||||
const identity = `${file.size}:${file.mtimeMs}`;
|
||||
if (bounded.includes(0)) return { identity: `binary:${identity}` };
|
||||
const preview = bounded.toString('utf8');
|
||||
return {
|
||||
preview,
|
||||
content: `text:${data.byteLength}:${preview}`,
|
||||
...(data.byteLength > bounded.byteLength ? { truncated: true } : {}),
|
||||
identity: `text:${identity}`,
|
||||
...(file.size > bounded.byteLength ? { truncated: true } : {}),
|
||||
};
|
||||
} catch (error) {
|
||||
if (error && typeof error === 'object' && 'code' in error && error.code === 'ENOENT') {
|
||||
return { content: 'missing' };
|
||||
return { identity: 'missing' };
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
async function trackedIdentity(projectPath: string, entry: StatusEntry): Promise<FileState> {
|
||||
const target = path.resolve(projectPath, ...entry.path.split('/'));
|
||||
try {
|
||||
const file = await stat(target);
|
||||
return {
|
||||
...entry,
|
||||
identity: `${file.isFile() ? 'file' : 'not-file'}:${file.size}:${file.mtimeMs}`,
|
||||
};
|
||||
} catch (error) {
|
||||
if (error && typeof error === 'object' && 'code' in error && error.code === 'ENOENT') {
|
||||
return { ...entry, identity: 'missing' };
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
@@ -246,18 +271,29 @@ export class ConversationChangeTracker {
|
||||
}
|
||||
|
||||
private async capture(projectPath: string): Promise<ChangeBaseline> {
|
||||
const repository = await this.git.run(projectPath, ['rev-parse', '--is-inside-work-tree']);
|
||||
let repository: GitCommandResult;
|
||||
try {
|
||||
repository = await this.git.run(projectPath, ['rev-parse', '--is-inside-work-tree']);
|
||||
} catch {
|
||||
return { git: false, head: null, files: new Map() };
|
||||
}
|
||||
if (repository.code !== 0 || repository.stdout.trim() !== 'true') {
|
||||
return { git: false, head: null, files: new Map() };
|
||||
}
|
||||
const [headResult, statusResult] = await Promise.all([
|
||||
this.git.run(projectPath, ['rev-parse', 'HEAD']),
|
||||
this.git.run(projectPath, ['status', '--porcelain=v2', '-z', '--untracked-files=all', '--', '.']),
|
||||
]);
|
||||
let headResult: GitCommandResult;
|
||||
let statusResult: GitCommandResult;
|
||||
try {
|
||||
[headResult, statusResult] = await Promise.all([
|
||||
this.git.run(projectPath, ['rev-parse', 'HEAD']),
|
||||
this.git.run(projectPath, ['status', '--porcelain=v2', '-z', '--untracked-files=all', '--', '.']),
|
||||
]);
|
||||
} catch {
|
||||
return { git: false, head: null, files: new Map() };
|
||||
}
|
||||
if (statusResult.code !== 0) return { git: false, head: null, files: new Map() };
|
||||
const entries = parsePorcelainV2(statusResult.stdout);
|
||||
const files = new Map<string, FileState>();
|
||||
for (const entry of entries) files.set(entry.path, await this.readState(projectPath, entry));
|
||||
const states = await Promise.all(entries.map((entry) => this.readState(projectPath, entry)));
|
||||
const files = new Map(states.map((state) => [state.path, state]));
|
||||
return {
|
||||
git: true,
|
||||
head: headResult.code === 0 ? headResult.stdout.trim() || null : null,
|
||||
@@ -265,31 +301,37 @@ export class ConversationChangeTracker {
|
||||
};
|
||||
}
|
||||
|
||||
private async currentStates(projectPath: string): Promise<Map<string, FileState>> {
|
||||
const status = await this.git.run(
|
||||
projectPath,
|
||||
['status', '--porcelain=v2', '-z', '--untracked-files=all', '--', '.'],
|
||||
);
|
||||
if (status.code !== 0) return new Map();
|
||||
const result = new Map<string, FileState>();
|
||||
for (const entry of parsePorcelainV2(status.stdout)) {
|
||||
result.set(entry.path, await this.readState(projectPath, entry));
|
||||
private async currentEntries(projectPath: string): Promise<Map<string, StatusEntry> | null> {
|
||||
try {
|
||||
const status = await this.git.run(
|
||||
projectPath,
|
||||
['status', '--porcelain=v2', '-z', '--untracked-files=all', '--', '.'],
|
||||
);
|
||||
if (status.code !== 0) return null;
|
||||
return new Map(parsePorcelainV2(status.stdout).map((entry) => [entry.path, entry]));
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
private async readState(projectPath: string, entry: StatusEntry): Promise<FileState> {
|
||||
if (entry.status === 'untracked') {
|
||||
return { ...entry, ...await untrackedPreview(projectPath, entry.path) };
|
||||
}
|
||||
return await trackedIdentity(projectPath, entry);
|
||||
}
|
||||
|
||||
private async readDiff(projectPath: string, relativePath: string): Promise<{
|
||||
diff?: string;
|
||||
truncated?: boolean;
|
||||
}> {
|
||||
const [working, staged] = await Promise.all([
|
||||
this.git.run(projectPath, ['diff', '--no-ext-diff', '--no-color', '--relative', '--', entry.path]),
|
||||
this.git.run(projectPath, ['diff', '--cached', '--no-ext-diff', '--no-color', '--relative', '--', entry.path]),
|
||||
this.git.run(projectPath, ['diff', '--no-ext-diff', '--no-color', '--relative', '--', relativePath]),
|
||||
this.git.run(projectPath, ['diff', '--cached', '--no-ext-diff', '--no-color', '--relative', '--', relativePath]),
|
||||
]);
|
||||
const bounded = boundedText(`${staged.stdout}${working.stdout}`, MAX_DIFF_BYTES);
|
||||
return {
|
||||
...entry,
|
||||
content: bounded.text,
|
||||
...(bounded.text ? { diff: bounded.text } : {}),
|
||||
...(bounded.truncated ? { truncated: true } : {}),
|
||||
};
|
||||
}
|
||||
@@ -301,20 +343,37 @@ export class ConversationChangeTracker {
|
||||
const current = await untrackedPreview(record.projectPath, filePath);
|
||||
files.push({
|
||||
path: filePath,
|
||||
status: current.content === 'missing' ? 'deleted' : 'modified',
|
||||
status: current.identity === 'missing' ? 'deleted' : 'modified',
|
||||
...(current.preview !== undefined ? { preview: current.preview } : {}),
|
||||
...(current.truncated ? { truncated: true } : {}),
|
||||
});
|
||||
}
|
||||
return { ...record.snapshot, files };
|
||||
}
|
||||
const current = await this.currentStates(record.projectPath);
|
||||
const current = await this.currentEntries(record.projectPath);
|
||||
if (!current) {
|
||||
const files: ConversationChangedFile[] = [];
|
||||
for (const filePath of [...record.touchedPaths].sort().slice(0, MAX_CHANGED_FILES)) {
|
||||
const state = await untrackedPreview(record.projectPath, filePath);
|
||||
files.push({
|
||||
path: filePath,
|
||||
status: state.identity === 'missing' ? 'deleted' : 'modified',
|
||||
...(state.preview !== undefined ? { preview: state.preview } : {}),
|
||||
...(state.truncated ? { truncated: true } : {}),
|
||||
});
|
||||
}
|
||||
return { ...record.snapshot, git: false, files };
|
||||
}
|
||||
const candidates = projectWide
|
||||
? [...current.keys(), ...record.baseline.files.keys()]
|
||||
: [...record.touchedPaths];
|
||||
const files: ConversationChangedFile[] = [];
|
||||
for (const filePath of [...new Set(candidates)].sort().slice(0, MAX_CHANGED_FILES)) {
|
||||
const next = current.get(filePath);
|
||||
const candidatePaths = [...new Set(candidates)].sort().slice(0, MAX_CHANGED_FILES);
|
||||
const states = await Promise.all(candidatePaths.map(async (filePath) => {
|
||||
const entry = current.get(filePath);
|
||||
return { filePath, next: entry ? await this.readState(record.projectPath, entry) : undefined };
|
||||
}));
|
||||
for (const { filePath, next } of states) {
|
||||
const baseline = record.baseline.files.get(filePath);
|
||||
if (!next) {
|
||||
if (baseline) {
|
||||
@@ -325,13 +384,16 @@ export class ConversationChangeTracker {
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (baseline?.signature === next.signature && baseline.content === next.content) continue;
|
||||
if (baseline?.signature === next.signature && baseline.identity === next.identity) continue;
|
||||
const diff = next.status === 'untracked'
|
||||
? {}
|
||||
: await this.readDiff(record.projectPath, next.path);
|
||||
files.push({
|
||||
path: next.path,
|
||||
status: next.status,
|
||||
...(next.status === 'untracked' && next.preview !== undefined ? { preview: next.preview } : {}),
|
||||
...(next.status !== 'untracked' && next.content ? { diff: next.content } : {}),
|
||||
...(next.truncated ? { truncated: true } : {}),
|
||||
...diff,
|
||||
...(next.truncated || diff.truncated ? { truncated: true } : {}),
|
||||
});
|
||||
}
|
||||
return { ...record.snapshot, files };
|
||||
|
||||
@@ -88,7 +88,20 @@ interface ChangeRefreshBridgeRequest {
|
||||
resourceId: string;
|
||||
}
|
||||
|
||||
type BridgeRequest = LeaseBridgeRequest | SubagentBridgeRequest | ProductToolBridgeRequest | ChangeRefreshBridgeRequest;
|
||||
interface ChangeTouchedBridgeRequest {
|
||||
action: 'changes.touched';
|
||||
conversationId: string;
|
||||
workerGeneration: number;
|
||||
runId: string;
|
||||
resourceId: string;
|
||||
paths: string[];
|
||||
}
|
||||
|
||||
type BridgeRequest = LeaseBridgeRequest
|
||||
| SubagentBridgeRequest
|
||||
| ProductToolBridgeRequest
|
||||
| ChangeRefreshBridgeRequest
|
||||
| ChangeTouchedBridgeRequest;
|
||||
|
||||
export interface PiExtensionSubagentBridge {
|
||||
scheduler: PiSubagentScheduler;
|
||||
@@ -111,6 +124,12 @@ function bridgeRequest(value: unknown): value is BridgeRequest {
|
||||
return typeof value.toolName === 'string' && 'input' in value;
|
||||
}
|
||||
if (value.action === 'changes.bash') return true;
|
||||
if (value.action === 'changes.touched') {
|
||||
return Array.isArray(value.paths)
|
||||
&& value.paths.length > 0
|
||||
&& value.paths.length <= 200
|
||||
&& value.paths.every((filePath) => typeof filePath === 'string');
|
||||
}
|
||||
return (value.action === 'lease.acquire' || value.action === 'lease.release')
|
||||
&& (value.leaseId === undefined || typeof value.leaseId === 'string');
|
||||
}
|
||||
@@ -304,6 +323,19 @@ export class PiManagedExtensionHost {
|
||||
this.respond(response, 200, { marked: true });
|
||||
return;
|
||||
}
|
||||
if (value.action === 'changes.touched') {
|
||||
if (!this.productTools) {
|
||||
this.respond(response, 503, { error: 'Conversation change tracker is unavailable' });
|
||||
return;
|
||||
}
|
||||
await this.productTools.recordTouchedPaths(
|
||||
record.conversationId,
|
||||
value.runId,
|
||||
value.paths,
|
||||
);
|
||||
this.respond(response, 200, { recorded: true });
|
||||
return;
|
||||
}
|
||||
if (value.action === 'product.invoke') {
|
||||
if (record.role !== 'parent') {
|
||||
this.respond(response, 403, { error: 'Child workers cannot invoke parent product tools' });
|
||||
|
||||
@@ -29,7 +29,7 @@ export async function reportChangedFiles(
|
||||
const paths = snapshot.files.map((file) => file.path);
|
||||
const details: ChangedFileDetailsV1 = { schema: 'changed-file.v1', paths };
|
||||
return {
|
||||
content: [{ type: 'text' as const, text: JSON.stringify({ paths, changes: snapshot.files }) }],
|
||||
content: [{ type: 'text' as const, text: `${paths.length} changed path(s) recorded` }],
|
||||
details,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -306,7 +306,10 @@ export default function makeloreRuntime(pi) {
|
||||
const paths = touchedPaths.get(event.toolCallId);
|
||||
touchedPaths.delete(event.toolCallId);
|
||||
if (paths) {
|
||||
await invokeProduct(event.toolCallId, 'changed_file', { paths, refresh: true }).catch(() => undefined);
|
||||
await bridge('changes.touched', {
|
||||
resourceId: event.toolCallId,
|
||||
paths,
|
||||
}).catch(() => undefined);
|
||||
}
|
||||
});
|
||||
pi.on('agent_end', releaseAll);
|
||||
|
||||
@@ -69,6 +69,10 @@ export class PiProductTools {
|
||||
await this.changeTracker.markProjectRefresh(conversationId, runId);
|
||||
}
|
||||
|
||||
recordTouchedPaths(conversationId: string, runId: string, paths: readonly string[]) {
|
||||
return this.changeTracker.recordTouchedPaths(conversationId, runId, paths);
|
||||
}
|
||||
|
||||
async execute(
|
||||
toolName: PiProductToolName,
|
||||
context: PiProductToolContext,
|
||||
|
||||
@@ -598,11 +598,11 @@ export class PiConversationRuntime implements CodingConversationRuntime {
|
||||
};
|
||||
const generation = this.pool.getState(input.conversationId)?.generation;
|
||||
if (generation) this.extensionUi.beginRun(input.conversationId, generation, runId);
|
||||
if (this.extensionHost && generation) {
|
||||
await this.extensionHost.bindRun(input.conversationId, generation, runId);
|
||||
}
|
||||
let ticket;
|
||||
try {
|
||||
if (this.extensionHost && generation) {
|
||||
await this.extensionHost.bindRun(input.conversationId, generation, runId);
|
||||
}
|
||||
ticket = this.pool.startTopLevel({
|
||||
conversationId: input.conversationId,
|
||||
runId,
|
||||
@@ -744,11 +744,11 @@ export class PiConversationRuntime implements CodingConversationRuntime {
|
||||
const runId = this.id('run');
|
||||
const generation = this.pool.getState(conversationId)?.generation;
|
||||
if (generation) this.extensionUi.beginRun(conversationId, generation, runId);
|
||||
if (this.extensionHost && generation) {
|
||||
await this.extensionHost.bindRun(conversationId, generation, runId);
|
||||
}
|
||||
let ticket;
|
||||
try {
|
||||
if (this.extensionHost && generation) {
|
||||
await this.extensionHost.bindRun(conversationId, generation, runId);
|
||||
}
|
||||
ticket = this.pool.startTopLevel({
|
||||
conversationId,
|
||||
runId,
|
||||
|
||||
Reference in New Issue
Block a user