diff --git a/.omp/commands/fix-issues.md b/.omp/commands/fix-issues.md index 9e9191375..35605140f 100644 --- a/.omp/commands/fix-issues.md +++ b/.omp/commands/fix-issues.md @@ -35,7 +35,7 @@ Each subagent **MUST** follow this exact workflow: #### a. Read everything -1. `github issue_view` (with comments) — comments often carry the real repro and fix hints. +1. Read `issue://` (or `issue:////` for cross-repo) — fetches the issue body plus comments; comments often carry the real repro and fix hints. Append `?comments=0` only if you explicitly want to skip them. 2. `gh search prs` for the issue number to see if a fix is already in flight. - If a PR exists and looks reasonable → switch tracks: review that PR per `.omp/commands/review-prs.md` instead, and report back as `existing-pr`. Do **not** open a competing fix. diff --git a/.omp/commands/review-prs.md b/.omp/commands/review-prs.md index 73ab65c31..f16ab030f 100644 --- a/.omp/commands/review-prs.md +++ b/.omp/commands/review-prs.md @@ -35,7 +35,7 @@ Each subagent **MUST** follow this exact workflow: #### a. Read & decide -1. `github pr_view` (with comments) and `github pr_diff` for the PR. +1. Read `pr://` (with comments by default; append `?comments=0` to skip) and `pr:///diff` for the changed-files listing — use `pr:///diff/all` when you need the full unified diff, or `pr:///diff/` for a single file slice. 2. Check `git log origin/main` and `gh search prs` for whether the same change already landed. 3. Classify into one of: - **slop** — AI-generated noise, broken, off-spec, or net-negative. Drop, write a 1–2 line justification, do not check out. diff --git a/docs/skills.md b/docs/skills.md index 0e046c498..9e89ba094 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -3,7 +3,7 @@ Skills are file-backed capability packs discovered at startup and exposed to the model as: - lightweight metadata in the system prompt (name + description) -- on-demand content via `read skill://...` +- on-demand content via the `read` tool against `skill://...` - optional interactive `/skill:` commands This document covers current runtime behavior in `src/extensibility/skills.ts`, `src/discovery/builtin.ts`, `src/internal-urls/skill-protocol.ts`, and `src/discovery/agents-md.ts`. diff --git a/docs/tools/bash.md b/docs/tools/bash.md index 843c82067..789036cee 100644 --- a/docs/tools/bash.md +++ b/docs/tools/bash.md @@ -100,7 +100,7 @@ Stdout and stderr are merged before the model sees them. Non-zero exit codes are - Uses `session.allocateOutputArtifact()` for spill files. - User-visible prompts / interactive UI - PTY mode opens a TUI overlay titled `Console` and forwards input to the PTY. - - Background start messages direct the agent to `job` and `read jobs://`. + - Background start messages direct the agent to `job` and to read `jobs://`. - Background work / cancellation - Async and auto-background jobs continue after the initial tool return. - Cancellation aborts the native run; PTY overlay dismissal also kills the PTY. diff --git a/docs/tools/job.md b/docs/tools/job.md index ae0e569b8..5b35d3337 100644 --- a/docs/tools/job.md +++ b/docs/tools/job.md @@ -42,8 +42,8 @@ Streaming behavior: - Final return is single-shot after a completion, timeout, abort, or immediate fast path. Related read path: -- `read jobs://` lists all current jobs. -- `read jobs://` renders one job with status, label, start time, duration, and stored result/error text. +- Reading `jobs://` lists all current jobs. +- Reading `jobs://` renders one job with status, label, start time, duration, and stored result/error text. ## Flow 1. `JobTool.createIf(...)` in `packages/coding-agent/src/tools/job.ts` only exposes the tool when `isBackgroundJobSupportEnabled(...)` returns true for either `async.enabled` or `bash.autoBackground.enabled`. @@ -129,15 +129,15 @@ Lifecycle and exact state names: - Cancelling a non-running job is not an exception; it reports `already_completed` even if the actual status is `completed`, `failed`, or `cancelled`. - Tool-call abort during polling stops waiting and returns a final snapshot through `#buildResult(...)`; it does not cancel watched jobs. - Failures inside the underlying async work are stored on the job (`status: "failed"`, `errorText`) and reported in normal tool output, not rethrown by `job`. -- `read jobs://` missing job returns markdown content headed `# Job Not Found` rather than throwing. +- Reading `jobs://` for a missing job returns markdown content headed `# Job Not Found` rather than throwing. ## Notes - `job` waits for the first watched running job to settle, not for all watched jobs. If others remain `running`, they are reported under `## Still Running`; the caller must invoke `job` again to continue waiting. - Delivery suppression is the key difference between snapshot and automatic delivery: - - snapshots (`job`, `read jobs://`) read current manager state; + - snapshots (`job`, reads of `jobs://`) read current manager state; - follow-up delivery comes from `AsyncJobManager.#enqueueDelivery(...)` and `sdk.ts` `onJobComplete`; - watched or acknowledged ids are suppressed via `isDeliverySuppressed(...)`. - `manager.cancel(id)` sets `status = "cancelled"` before the underlying promise settles. The job function may later populate `resultText` or `errorText`; `job-manager.ts` preserves that text but does not transition the status away from `cancelled`. - `jobs://` is implemented by `JobsProtocolHandler` with `immutable = true`, but each resolve call reads live manager state at access time. - `jobs://` shows a cancellation section only when a cancelled job has `errorText`; cancelled jobs with `resultText` are not rendered with a result section there. -- Retention eviction removes the job record, suppression flags, and watch flag together. After eviction, both `job` and `read jobs://` behave as if the id never existed. +- Retention eviction removes the job record, suppression flags, and watch flag together. After eviction, both `job` and reads of `jobs://` behave as if the id never existed. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5957825be..63ba7c24f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,8 +4,8 @@ ### Breaking Changes -- Removed `op: issue_view` and `op: pr_view` from the `github` tool. Read single issues/PRs through `read issue://` / `read pr://` (or the long form `read issue:////` / `read pr:////`); append `?comments=0` to drop the comments section. The `issue` and `comments` parameters were removed from the tool schema since no remaining op consumes them. Mutating ops (`pr_create`, `pr_checkout`, `pr_push`), `repo_view`, `search_*`, and `run_watch` are unchanged. -- Removed `op: pr_diff` (along with the `nameOnly` and `exclude` schema fields) from the `github` tool. Read PR diffs through the new `pr://` URL family: `pr:///diff` for the changed-file listing, `pr:///diff/` for a single file slice (1-indexed), and `pr:///diff/all` for the verbatim unified diff. Long-form `pr://///diff[/…]` works the same way. All three variants share one `gh pr diff` invocation through a new `pr-diff` cache row, so the listing and per-file slices reconstruct from cached bytes without re-shelling. Diff content is served as `text/plain` so `read`'s line selectors (`pr:///diff/all:200-400`) page the cached output without falsely advertising hashline anchors. +- Removed `op: issue_view` and `op: pr_view` from the `github` tool. Read single issues/PRs via the `read` tool against `issue://` / `pr://` (or the long form `issue:////` / `pr:////`); append `?comments=0` to drop the comments section. The `issue` and `comments` parameters were removed from the tool schema since no remaining op consumes them. Mutating ops (`pr_create`, `pr_checkout`, `pr_push`), `repo_view`, `search_*`, and `run_watch` are unchanged. +- Removed `op: pr_diff` (along with the `nameOnly` and `exclude` schema fields) from the `github` tool. Read PR diffs through the new `pr://` URL family: `pr:///diff` for the changed-file listing, `pr:///diff/` for a single file slice (1-indexed), and `pr:///diff/all` for the verbatim unified diff. Long-form `pr://///diff[/…]` works the same way. All three variants share one `gh pr diff` invocation through a new `pr-diff` cache row, so the listing and per-file slices reconstruct from cached bytes without re-shelling. Diff content is served as `text/plain` so the `read` tool's line selectors (e.g. `pr:///diff/all:200-400`) page the cached output without falsely advertising hashline anchors. ### Added diff --git a/packages/coding-agent/src/tools/conflict-detect.ts b/packages/coding-agent/src/tools/conflict-detect.ts index 1a67c6daa..3612d87f4 100644 --- a/packages/coding-agent/src/tools/conflict-detect.ts +++ b/packages/coding-agent/src/tools/conflict-detect.ts @@ -240,7 +240,7 @@ export function getConflictHistory(session: ToolSession): ConflictHistory { return session.conflictHistory; } -/** A side of a conflict block that `read conflict://N/` can render. */ +/** A side of a conflict block that the `read` tool can render via `conflict://N/`. */ export type ConflictScope = "ours" | "theirs" | "base"; const CONFLICT_SCOPES = new Set(["ours", "theirs", "base"]); @@ -440,7 +440,7 @@ function markerLine(prefix: string, label: string | undefined): string { } /** - * Materialise a conflict block for `read conflict://` (and its + * Materialise a conflict block for `conflict://` reads (and their * `/ours` / `/theirs` / `/base` scopes). * * Returns: @@ -534,7 +534,7 @@ export function formatConflictWarning( if (partial) { const hintPath = options.displayPath ?? ""; out.push( - `⚠ ${entries.length} of ${total} unresolved ${word} visible in this window (run \`read ${hintPath}:conflicts\` for the full list).`, + `⚠ ${entries.length} of ${total} unresolved ${word} visible in this window (read \`${hintPath}:conflicts\` for the full list).`, ); } else { out.push(`⚠ ${total} unresolved ${word} detected`); @@ -551,7 +551,7 @@ export function formatConflictWarning( if (theirsLabel) out.push(`- theirs = ${theirsLabel}`); if (anyBase) out.push(`- base = ${baseLabel ?? "(no label)"}`); out.push( - 'NOTICE: Inspect a block with `read conflict://` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://", content })`, or bulk-resolve every registered conflict with `write({ path: "conflict://*", content })`. Writes replace the whole conflict region (markers + all sides).', + 'NOTICE: Inspect a block by reading `conflict://` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://", content })`, or bulk-resolve every registered conflict with `write({ path: "conflict://*", content })`. Writes replace the whole conflict region (markers + all sides).', ); out.push( '`content` shorthand: a line that is exactly `@ours` / `@theirs` / `@base` / `@both` expands to that recorded section. `@both` is ours-then-theirs with no separator. Lines that are not a token pass through verbatim, so `"// keep both\\n@ours\\n@theirs"` literally writes the comment, then ours, then theirs.', @@ -592,7 +592,7 @@ export function formatConflictWarning( /** * Render a single-line-per-block index of every conflict in a file. - * Used by `read :conflicts` to give the agent a cheap overview + * Used by the `:conflicts` read selector to give the agent a cheap overview * of a heavily-conflicted file without dumping every body. */ export function formatConflictSummary( @@ -614,7 +614,7 @@ export function formatConflictSummary( if (theirsLabel) lines.push(`- theirs = ${theirsLabel}`); if (anyBase) lines.push(`- base = ${baseLabel ?? "(no label)"}`); lines.push( - 'NOTICE: Bulk-resolve with `write({ path: "conflict://*", content })`, or address a single block with `write({ path: "conflict://", content })`. Inspect a block with `read conflict://` (add `/ours` / `/theirs` / `/base` for a single side).', + 'NOTICE: Bulk-resolve with `write({ path: "conflict://*", content })`, or address a single block with `write({ path: "conflict://", content })`. Inspect a block by reading `conflict://` (add `/ours` / `/theirs` / `/base` for a single side).', ); lines.push( "`content` shorthand: `@ours` / `@theirs` / `@base` / `@both` lines expand to the recorded sections; `@both` = ours-then-theirs. Non-token lines pass through verbatim.", diff --git a/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts b/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts index 86491c296..2d14ccdda 100644 --- a/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts @@ -198,6 +198,22 @@ describe("pr:// protocol handler", () => { await expect(router.resolve("pr://owner/example/foo/bar")).rejects.toThrow(/Invalid pr:\/\/ URL/); await expect(router.resolve("pr://owner/example/abc")).rejects.toThrow(/Invalid pr:\/\/ number/); }); + + it("rejects empty / dot / dotdot path segments", async () => { + const router = InternalUrlRouter.instance(); + await expect(router.resolve("pr://owner//77")).rejects.toThrow( + /Invalid pr:\/\/ URL: empty or unsafe path segment/, + ); + await expect(router.resolve("pr://owner/repo/77/diff//2")).rejects.toThrow( + /Invalid pr:\/\/ URL: empty or unsafe path segment/, + ); + await expect(router.resolve("pr://owner/../77/diff")).rejects.toThrow( + /Invalid pr:\/\/ URL: empty or unsafe path segment/, + ); + await expect(router.resolve("issue://owner/./repo/1")).rejects.toThrow( + /Invalid issue:\/\/ URL: empty or unsafe path segment/, + ); + }); }); describe("pr://.../diff family", () => { diff --git a/packages/coding-agent/test/tools/conflict-detect.test.ts b/packages/coding-agent/test/tools/conflict-detect.test.ts index 36117d4f3..bea75fdf1 100644 --- a/packages/coding-agent/test/tools/conflict-detect.test.ts +++ b/packages/coding-agent/test/tools/conflict-detect.test.ts @@ -380,7 +380,7 @@ describe("formatConflictWarning", () => { expect(text).toContain(">>> theirs"); expect(text).toContain("\nc"); // NOTICE line with shorthand tokens. - expect(text).toContain("NOTICE: Inspect a block with `read conflict://`"); + expect(text).toContain("NOTICE: Inspect a block by reading `conflict://`"); expect(text).toContain('`write({ path: "conflict://", content })`'); expect(text).toContain('`write({ path: "conflict://*", content })`'); expect(text).toContain("@ours"); diff --git a/packages/coding-agent/test/tools/conflict-integration.test.ts b/packages/coding-agent/test/tools/conflict-integration.test.ts index 7ad178504..f0b3112fa 100644 --- a/packages/coding-agent/test/tools/conflict-integration.test.ts +++ b/packages/coding-agent/test/tools/conflict-integration.test.ts @@ -101,7 +101,7 @@ describe("read surfaces conflicts as a warning footer", () => { expect(text).toContain("──── #1 L2-6 ────"); expect(text).toContain("<<< ours"); expect(text).toContain(">>> theirs"); - expect(text).toContain("NOTICE: Inspect a block with `read conflict://`"); + expect(text).toContain("NOTICE: Inspect a block by reading `conflict://`"); expect(text).toContain('`write({ path: "conflict://", content })`'); expect(text).toContain('`write({ path: "conflict://*", content })`'); expect(text).toContain("@ours"); @@ -164,7 +164,7 @@ describe("read surfaces conflicts as a warning footer", () => { expect(session.conflictHistory?.get(2)).toBeUndefined(); }); - it("renders the full conflict block via `read conflict://`", async () => { + it("renders the full conflict block via reads of `conflict://`", async () => { const filePath = path.join(tempDir, "full.ts"); await Bun.write(filePath, TWO_WAY); const session = createTestSession(tempDir); @@ -182,7 +182,7 @@ describe("read surfaces conflicts as a warning footer", () => { expect(text).not.toContain("⚠"); }); - it("renders only the theirs body via `read conflict:///theirs`", async () => { + it("renders only the theirs body via reads of `conflict:///theirs`", async () => { const filePath = path.join(tempDir, "theirs.ts"); await Bun.write(filePath, TWO_WAY); const session = createTestSession(tempDir); @@ -230,14 +230,14 @@ describe("read surfaces conflicts as a warning footer", () => { await expect(promise).rejects.toThrow(/Conflict #99 not found/); }); - it("rejects `read conflict://*` (wildcard is write-only)", async () => { + it("rejects reads of `conflict://*` (wildcard is write-only)", async () => { const session = createTestSession(tempDir); const read = await getTool(session, "read"); const promise = read.execute("read-wildcard", { path: "conflict://*" }); await expect(promise).rejects.toThrow(/wildcards are write-only/); }); - it("`read :conflicts` lists every conflict in the file with stable ids", async () => { + it("the `:conflicts` read selector lists every conflict in the file with stable ids", async () => { const filePath = path.join(tempDir, "many.ts"); await Bun.write(filePath, TWO_BLOCKS); const session = createTestSession(tempDir); @@ -287,7 +287,7 @@ describe("read surfaces conflicts as a warning footer", () => { const result = await read.execute("read-window", { path: "wide.ts:1-5" }); const text = getText(result); expect(text).toContain("1 of 2 unresolved"); - expect(text).toContain("read wide.ts:conflicts"); + expect(text).toContain("read `wide.ts:conflicts`"); }); });