fix(coding-agent): browser default, patch error path, mcp:// selectors, find timeout/sort, DAP launch races, debugpy diagnostics
- browser tool's existing-tab re-nav defaults to waitUntil: 'load' (matching new-tab path); identical acquireTab() calls no longer hang on dev servers - patch tool error path uses caller-supplied relative path; absolute resolvedPath stays in structured context only ($HOME no longer leaks to TUI) - splitInternalUrlSel keeps mcp:// resource URIs opaque even when they end in ':raw' or '/:1-50' (McpProtocolHandler matches by verbatim URI) - find tool: timeout signal honored by onMatch; partial results sorted by mtime desc; backslash-escaped commas skipped in path-list validation - DAP throwPreferredDapStartError waits up to 50ms for the underlying launch/attach error instead of one microtask - debug tool surfaces 'python missing' and 'pip install debugpy' diagnostics separately when adapter: 'debugpy' is requested
This commit is contained in:
@@ -27,6 +27,15 @@
|
||||
- Fixed web search OAuth-backed providers (including Codex and Gemini) to use broker-managed token retrieval and account metadata, avoiding direct token-store refresh behavior that could cause search authentication failures
|
||||
- Updated Tavily missing-credential feedback to prompt users to configure an API-key provider setting instead of referencing `agent.db` directly
|
||||
- Refreshed expired OpenAI Codex OAuth tokens during `web_search` execution and persisted the updated credentials so searches continue working after token expiry
|
||||
- Fixed the browser tool's existing-tab re-navigation path (`tab-supervisor.acquireTab`) still defaulting to `waitUntil: "networkidle2"` while the new-tab worker switched to `"load"`. Identical `acquireTab({url})` calls behaved differently depending on whether the named tab was fresh or being reused — fresh tabs returned quickly, second invocations hung on dev servers with persistent WebSocket / HMR connections. Both paths now agree on `"load"`.
|
||||
- Fixed the `patch` tool's post-write verification `ToolError` embedding the absolute `resolvedPath` in its user-facing message; the outer composer in `executeSinglePathEntries` then prepended `Error editing ${path}: …`, double-embedding the path and leaking `$HOME` into the TUI. The error message now uses the caller-supplied relative `path` (matching the success branches); `resolvedPath` is retained only in the structured `context` metadata for log correlation.
|
||||
- Fixed `splitInternalUrlSel` keeping `mcp://` resource URIs fully opaque, including selector-shaped suffixes like `:raw`, `:1-50`, and `/:raw`. `McpProtocolHandler` resolves resources by verbatim URI match (`r.uri === uri`), so peeling before resource lookup can make valid server-defined resource IDs unreachable. MCP read-selector support now requires a future resolver-aware path that can try the exact URI before interpreting any suffix as a selector; non-MCP internal URL selectors are unchanged.
|
||||
- Fixed three correctness issues in the `find` tool. (1) `onMatch` guard checked the outer task `signal?.aborted` instead of `combinedSignal.aborted` (which also includes the per-call timeout signal), so late matches accumulated past the timeout. (2) Timeout-drained partial results were emitted in insertion order while the normal path sorts by mtime descending; callers relying on "most recently modified first" got an inconsistent ordering when the call timed out. The timeout drain now tracks per-entry mtime in a parallel array and applies the same comparator. (3) `validateFindPathInputs` now skips backslash-escaped commas (`\,`) when checking for top-level commas, matching `search.ts:containsTopLevelComma`.
|
||||
- Fixed `throwPreferredDapStartError` using a single `await Promise.resolve()` (one microtask) to let a concurrent launch/attach rejection settle before deciding which error to surface. That worked for synchronous-rejection test fakes but not real adapter I/O: the launch failure arrives via socket and lands several ticks after the `configurationDone` failure. `DapStartRequestFailure` now carries a `settled?: Promise<void>` that resolves when the underlying request settles either way, and `throwPreferredDapStartError` races against it with a 50 ms ceiling. The preferred error message (the underlying launch/attach failure rather than the cascade) now surfaces under real I/O.
|
||||
- Fixed `adapter: "debugpy"` over-promising in the `debug` tool. `resolveAdapter("debugpy", cwd)` only checks for `python` in `PATH`; it does not verify that the `debugpy` module is importable. Both failure modes — `python` missing and `debugpy` module missing — used to collapse onto a generic `"No suitable debug adapter found"` error. The launch/attach action now throws a targeted `ToolError` naming `python` when `resolveAdapter` returns null for an explicit `adapter: "debugpy"`, and the `DapSessionManager` spawn-catch detects `"No module named debugpy"` in adapter stderr and surfaces a `pip install debugpy` hint. The Python debug tool documentation lists the install hint so the prompt and runtime diagnostics agree.
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed built-in `explore` agent failing every invocation with `schema_violation: files.0.ref: must not be present` on releases prior to 15.3.2 by renaming the `files[].ref` property to `files[].path` in the agent's output schema; `ref` is a JTD-reserved keyword (RFC 8927) and collides with JSON Type Definition's schema-reference form, so the converter previously dropped it from the generated JSON Schema. Defense-in-depth alongside the 15.3.2 converter fix ([#1379](https://github.com/can1357/oh-my-pi/issues/1379)).
|
||||
- Increased the `yield` tool's schema-validation retry budget from 1 to 3 so subagents whose first structured-output attempt mismatches the declared output schema get up to three retries before the parent's post-mortem `schema_violation` check hard-fails the task. The tool now also surfaces remaining retry attempts and an explicit "call yield again with the corrected shape" directive in each rejection message, giving the model the context it needs to converge — particularly helpful for models like GLM that tend to invent per-element field names instead of following the declared schema.
|
||||
|
||||
|
||||
@@ -108,14 +108,20 @@ function toErrorMessage(value: unknown): string {
|
||||
interface DapStartRequestFailure {
|
||||
rejected: boolean;
|
||||
error?: unknown;
|
||||
settled?: Promise<void>;
|
||||
}
|
||||
|
||||
function trackDapStartRequest<T>(promise: Promise<T>, failure: DapStartRequestFailure): Promise<T> {
|
||||
return promise.catch(error => {
|
||||
const tracked = promise.catch(error => {
|
||||
failure.rejected = true;
|
||||
failure.error = error;
|
||||
throw error;
|
||||
});
|
||||
failure.settled = tracked.then(
|
||||
() => {},
|
||||
() => {},
|
||||
);
|
||||
return tracked;
|
||||
}
|
||||
|
||||
function combineDapStartErrors(command: "launch" | "attach", startError: unknown, configurationError: unknown): Error {
|
||||
@@ -134,7 +140,7 @@ async function throwPreferredDapStartError(
|
||||
startFailure: DapStartRequestFailure,
|
||||
configurationError: unknown,
|
||||
): Promise<never> {
|
||||
await Promise.resolve();
|
||||
await Promise.race([startFailure.settled ?? Promise.resolve(), timers.setTimeout(50)]);
|
||||
if (startFailure.rejected) {
|
||||
throw combineDapStartErrors(command, startFailure.error, configurationError);
|
||||
}
|
||||
@@ -274,6 +280,9 @@ export class DapSessionManager {
|
||||
return buildSummary(session);
|
||||
} catch (error) {
|
||||
await this.#disposeSession(session);
|
||||
if (options.adapter.name === "debugpy" && /No module named ['"]?debugpy['"]?/.test(toErrorMessage(error))) {
|
||||
throw new Error("adapter 'debugpy' is not available: install with 'pip install debugpy'");
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
@@ -330,6 +339,9 @@ export class DapSessionManager {
|
||||
return buildSummary(session);
|
||||
} catch (error) {
|
||||
await this.#disposeSession(session);
|
||||
if (options.adapter.name === "debugpy" && /No module named ['"]?debugpy['"]?/.test(toErrorMessage(error))) {
|
||||
throw new Error("adapter 'debugpy' is not available: install with 'pip install debugpy'");
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1763,7 +1763,7 @@ export async function executePatchSingle(
|
||||
postEditContent.length === preEditContent.length &&
|
||||
postEditContent.every((b, i) => b === preEditContent[i]);
|
||||
if (unchanged) {
|
||||
throw new ToolError(`edit appeared successful but file content did not change on disk: ${resolvedPath}`, {
|
||||
throw new ToolError(`edit appeared successful but file content did not change on disk: ${path}`, {
|
||||
path: resolvedPath,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@ Use for launching or attaching debuggers, setting breakpoints, stepping through
|
||||
- Some adapters require a launched session to receive `configurationDone` before the target actually runs; if the tool says configuration is pending, set breakpoints and then call `continue`.
|
||||
- Adapter availability depends on local binaries. Common built-ins: `gdb`, `lldb-dap`, `python -m debugpy.adapter`, `dlv dap`.
|
||||
- `program` must be an executable file or debug target, not a directory or interpreter name that resolves to a workspace directory.
|
||||
- Python debugging requires `debugpy`; install with `pip install debugpy` if the adapter is unavailable.
|
||||
</caution>
|
||||
|
||||
<examples>
|
||||
|
||||
@@ -105,7 +105,7 @@ export async function acquireTab(
|
||||
await runInTabWithSnapshot(
|
||||
name,
|
||||
{
|
||||
code: `await tab.goto(${JSON.stringify(opts.url)}, { waitUntil: ${JSON.stringify(opts.waitUntil ?? "networkidle2")} });`,
|
||||
code: `await tab.goto(${JSON.stringify(opts.url)}, { waitUntil: ${JSON.stringify(opts.waitUntil ?? "load")} });`,
|
||||
timeoutMs: opts.timeoutMs,
|
||||
signal: opts.signal,
|
||||
},
|
||||
|
||||
@@ -647,6 +647,9 @@ export class DebugTool implements AgentTool<typeof debugSchema, DebugToolDetails
|
||||
await validateLaunchProgram(program, commandCwd);
|
||||
const adapter = selectLaunchAdapter(program, commandCwd, params.adapter);
|
||||
if (!adapter) {
|
||||
if (params.adapter === "debugpy") {
|
||||
throw new ToolError("adapter 'debugpy' is not available: python not found in PATH");
|
||||
}
|
||||
throw new ToolError(
|
||||
`No debugger adapter available. Installed adapters: ${getConfiguredAdapters(commandCwd)}`,
|
||||
);
|
||||
@@ -667,6 +670,9 @@ export class DebugTool implements AgentTool<typeof debugSchema, DebugToolDetails
|
||||
const commandCwd = params.cwd ? resolveToCwd(params.cwd, this.session.cwd) : this.session.cwd;
|
||||
const adapter = selectAttachAdapter(commandCwd, params.adapter, params.port);
|
||||
if (!adapter) {
|
||||
if (params.adapter === "debugpy") {
|
||||
throw new ToolError("adapter 'debugpy' is not available: python not found in PATH");
|
||||
}
|
||||
throw new ToolError(
|
||||
`No debugger adapter available. Installed adapters: ${getConfiguredAdapters(commandCwd)}`,
|
||||
);
|
||||
|
||||
@@ -64,6 +64,10 @@ function validateFindPathInputs(paths: readonly string[]): void {
|
||||
let braceDepth = 0;
|
||||
for (let i = 0; i < entry.length; i++) {
|
||||
const ch = entry.charCodeAt(i);
|
||||
if (ch === 0x5c /* \ */ && i + 1 < entry.length) {
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (ch === 0x7b /* { */) braceDepth++;
|
||||
else if (ch === 0x7d /* } */) {
|
||||
if (braceDepth > 0) braceDepth--;
|
||||
@@ -304,6 +308,7 @@ export class FindTool implements AgentTool<typeof findSchema, FindToolDetails> {
|
||||
|
||||
let matches: natives.GlobMatch[];
|
||||
const onUpdateMatches: string[] = [];
|
||||
const onUpdateMtimes: number[] = [];
|
||||
const updateIntervalMs = 200;
|
||||
let lastUpdate = 0;
|
||||
const emitUpdate = () => {
|
||||
@@ -323,9 +328,10 @@ export class FindTool implements AgentTool<typeof findSchema, FindToolDetails> {
|
||||
});
|
||||
};
|
||||
const onMatch = (err: Error | null, match: natives.GlobMatch | null) => {
|
||||
if (err || signal?.aborted || !match?.path) return;
|
||||
if (err || combinedSignal.aborted || !match?.path) return;
|
||||
const relativePath = formatMatchPath(match.path, match.fileType);
|
||||
onUpdateMatches.push(relativePath);
|
||||
onUpdateMtimes.push(match.mtime ?? 0);
|
||||
emitUpdate();
|
||||
};
|
||||
|
||||
@@ -371,15 +377,18 @@ export class FindTool implements AgentTool<typeof findSchema, FindToolDetails> {
|
||||
// instead of throwing — empty results after a multi-second wait force the
|
||||
// caller to retry blind, which is the worst possible outcome.
|
||||
const seen = new Set<string>();
|
||||
const partial: string[] = [];
|
||||
for (const entry of onUpdateMatches) {
|
||||
const partial: Array<{ p: string; m: number }> = [];
|
||||
for (let i = 0; i < onUpdateMatches.length; i++) {
|
||||
const entry = onUpdateMatches[i];
|
||||
if (seen.has(entry)) continue;
|
||||
seen.add(entry);
|
||||
partial.push(entry);
|
||||
partial.push({ p: entry, m: onUpdateMtimes[i] ?? 0 });
|
||||
}
|
||||
partial.sort((a, b) => b.m - a.m);
|
||||
const sortedPaths = partial.map(e => e.p);
|
||||
const seconds = timeoutMs % 1000 === 0 ? `${timeoutMs / 1000}` : (timeoutMs / 1000).toFixed(1);
|
||||
const notice = `find timed out after ${seconds}s; returning ${partial.length} partial matches — increase timeout or narrow pattern`;
|
||||
return buildResult(partial, { notice, forceTruncated: true });
|
||||
const notice = `find timed out after ${seconds}s; returning ${sortedPaths.length} partial matches — increase timeout or narrow pattern`;
|
||||
return buildResult(sortedPaths, { notice, forceTruncated: true });
|
||||
}
|
||||
|
||||
const relativized: string[] = [];
|
||||
|
||||
@@ -173,10 +173,27 @@ export function splitPathAndSel(rawPath: string): { path: string; sel?: string }
|
||||
*
|
||||
* Falls back to the input unchanged when nothing matches.
|
||||
*/
|
||||
/** MCP resource URIs are server-defined and may legitimately end with
|
||||
* selector-shaped tails like `:raw`, `:conflicts`, `:1-50`, or even `/:raw`.
|
||||
* `McpProtocolHandler` resolves by exact resource URI match (`r.uri === uri`),
|
||||
* so syntactically peeling a selector here can make valid resources
|
||||
* unreachable. Keep mcp:// opaque; selector support for MCP resources needs a
|
||||
* resolver-aware path that can try the exact URI before interpreting a suffix
|
||||
* as a read selector. */
|
||||
function keepOpaqueResourceUri(rawPath: string): { path: string } {
|
||||
return { path: rawPath };
|
||||
}
|
||||
|
||||
export function splitInternalUrlSel(rawPath: string): { path: string; sel?: string } {
|
||||
const schemeMatch = rawPath.match(INTERNAL_URL_SCHEME_RE);
|
||||
if (!schemeMatch) return { path: rawPath };
|
||||
if (!INTERNAL_SCHEMES_WITH_SELECTORS[schemeMatch[1].toLowerCase()]) return { path: rawPath };
|
||||
const scheme = schemeMatch[1].toLowerCase();
|
||||
// `mcp://` resource URIs are server-defined and may legitimately contain
|
||||
// selector-shaped suffixes. Keep them opaque so exact resource lookup wins.
|
||||
if (!INTERNAL_SCHEMES_WITH_SELECTORS[scheme]) {
|
||||
if (scheme === "mcp") return keepOpaqueResourceUri(rawPath);
|
||||
return { path: rawPath };
|
||||
}
|
||||
|
||||
const schemeEnd = schemeMatch[0].length;
|
||||
let path = rawPath;
|
||||
|
||||
@@ -182,6 +182,22 @@ describe("DAP launch failure handling", () => {
|
||||
process.off("unhandledRejection", onUnhandled);
|
||||
}
|
||||
});
|
||||
|
||||
it("surfaces the adapter name and ENOENT when spawn fails", async () => {
|
||||
const manager = new DapSessionManager();
|
||||
spyOn(DapClient, "spawn").mockRejectedValue(new Error("ENOENT: no such file or directory, spawn 'lldb-dap'"));
|
||||
|
||||
let message = "";
|
||||
try {
|
||||
await manager.launch({ adapter: TEST_ADAPTER, program: "/bin/echo", cwd: process.cwd() });
|
||||
} catch (error) {
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
message = (error as Error).message;
|
||||
}
|
||||
|
||||
expect(message).toContain("ENOENT");
|
||||
expect(message).toContain(TEST_ADAPTER.name);
|
||||
});
|
||||
});
|
||||
|
||||
describe("DebugTool launch validation", () => {
|
||||
|
||||
@@ -53,12 +53,60 @@ describe("splitInternalUrlSel", () => {
|
||||
expect(splitInternalUrlSel("agent://1-50")).toEqual({ path: "agent://1-50" });
|
||||
});
|
||||
|
||||
it("leaves mcp:// URLs alone (mcp resource URIs may legitimately contain colons)", () => {
|
||||
it("keeps bare-integer suffixes on mcp:// URLs (could be a port)", () => {
|
||||
expect(splitInternalUrlSel("mcp://some/resource:1234")).toEqual({
|
||||
path: "mcp://some/resource:1234",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server://uri:1-50")).toEqual({
|
||||
path: "mcp://server://uri:1-50",
|
||||
});
|
||||
|
||||
it("treats mcp:// URIs as opaque by default — selector-shaped suffixes are NOT peeled", () => {
|
||||
// MCP resource URIs are server-defined and may legitimately end with `:raw`,
|
||||
// `:1-50`, etc. Without an explicit escape the URI must be forwarded verbatim
|
||||
// to the protocol handler so server-defined resources remain reachable.
|
||||
expect(splitInternalUrlSel("mcp://server/resource:1-50")).toEqual({
|
||||
path: "mcp://server/resource:1-50",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource:raw")).toEqual({
|
||||
path: "mcp://server/resource:raw",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource:L10")).toEqual({
|
||||
path: "mcp://server/resource:L10",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource:conflicts")).toEqual({
|
||||
path: "mcp://server/resource:conflicts",
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps escaped selector-shaped mcp:// suffixes opaque too", () => {
|
||||
// MCP resource URIs are exact server-defined IDs. A resource may
|
||||
// legitimately end in `/:raw` or `/:1-50`; splitting before resolution
|
||||
// would make that resource unreachable with no exact-URI escape hatch.
|
||||
expect(splitInternalUrlSel("mcp://server/resource/:1-50")).toEqual({
|
||||
path: "mcp://server/resource/:1-50",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource/:raw")).toEqual({
|
||||
path: "mcp://server/resource/:raw",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource/:L10")).toEqual({
|
||||
path: "mcp://server/resource/:L10",
|
||||
});
|
||||
expect(splitInternalUrlSel("mcp://server/resource/:conflicts")).toEqual({
|
||||
path: "mcp://server/resource/:conflicts",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not peel when the only slash before the colon is part of the `://` separator", () => {
|
||||
// Guards against degenerate inputs like `mcp://:1-50` — stripping the
|
||||
// scheme's own slash would emit `mcp:/` as the path. The peeler refuses
|
||||
// when the resulting path no longer carries a scheme separator.
|
||||
expect(splitInternalUrlSel("mcp://:1-50")).toEqual({ path: "mcp://:1-50" });
|
||||
expect(splitInternalUrlSel("mcp://:raw")).toEqual({ path: "mcp://:raw" });
|
||||
});
|
||||
|
||||
it("rejects bare-integer suffixes even with the trailing-slash escape", () => {
|
||||
// Could still be a port number after a path; require a richer selector form.
|
||||
expect(splitInternalUrlSel("mcp://server/resource/:1234")).toEqual({
|
||||
path: "mcp://server/resource/:1234",
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user