chore: fix mentions of read tool in backticks causing confusing against bash read
This commit is contained in:
can1357
@@ -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://<N>` (or `issue://<owner>/<repo>/<N>` 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.
|
||||
|
||||
|
||||
@@ -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://<N>` (with comments by default; append `?comments=0` to skip) and `pr://<N>/diff` for the changed-files listing — use `pr://<N>/diff/all` when you need the full unified diff, or `pr://<N>/diff/<i>` 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.
|
||||
|
||||
+1
-1
@@ -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:<name>` 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`.
|
||||
|
||||
+1
-1
@@ -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://<id>`.
|
||||
- Background start messages direct the agent to `job` and to read `jobs://<id>`.
|
||||
- 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.
|
||||
|
||||
+5
-5
@@ -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://<id>` renders one job with status, label, start time, duration, and stored result/error text.
|
||||
- Reading `jobs://` lists all current jobs.
|
||||
- Reading `jobs://<id>` 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://<id>` missing job returns markdown content headed `# Job Not Found` rather than throwing.
|
||||
- Reading `jobs://<id>` 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://<id>` 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://<id>` 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://<id>` behave as if the id never existed.
|
||||
|
||||
@@ -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://<N>` / `read pr://<N>` (or the long form `read issue://<owner>/<repo>/<N>` / `read pr://<owner>/<repo>/<N>`); 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://<N>/diff` for the changed-file listing, `pr://<N>/diff/<i>` for a single file slice (1-indexed), and `pr://<N>/diff/all` for the verbatim unified diff. Long-form `pr://<owner>/<repo>/<N>/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://<N>/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://<N>` / `pr://<N>` (or the long form `issue://<owner>/<repo>/<N>` / `pr://<owner>/<repo>/<N>`); 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://<N>/diff` for the changed-file listing, `pr://<N>/diff/<i>` for a single file slice (1-indexed), and `pr://<N>/diff/all` for the verbatim unified diff. Long-form `pr://<owner>/<repo>/<N>/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://<N>/diff/all:200-400`) page the cached output without falsely advertising hashline anchors.
|
||||
|
||||
### Added
|
||||
|
||||
|
||||
@@ -240,7 +240,7 @@ export function getConflictHistory(session: ToolSession): ConflictHistory {
|
||||
return session.conflictHistory;
|
||||
}
|
||||
|
||||
/** A side of a conflict block that `read conflict://N/<scope>` can render. */
|
||||
/** A side of a conflict block that the `read` tool can render via `conflict://N/<scope>`. */
|
||||
export type ConflictScope = "ours" | "theirs" | "base";
|
||||
|
||||
const CONFLICT_SCOPES = new Set<ConflictScope>(["ours", "theirs", "base"]);
|
||||
@@ -440,7 +440,7 @@ function markerLine(prefix: string, label: string | undefined): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* Materialise a conflict block for `read conflict://<N>` (and its
|
||||
* Materialise a conflict block for `conflict://<N>` reads (and their
|
||||
* `/ours` / `/theirs` / `/base` scopes).
|
||||
*
|
||||
* Returns:
|
||||
@@ -534,7 +534,7 @@ export function formatConflictWarning(
|
||||
if (partial) {
|
||||
const hintPath = options.displayPath ?? "<file>";
|
||||
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://<N>` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://<N>", 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://<N>` (add `/ours` / `/theirs` / `/base` to render a single side). Resolve with `write({ path: "conflict://<N>", 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 <path>:conflicts` to give the agent a cheap overview
|
||||
* Used by the `<path>: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://<N>", content })`. Inspect a block with `read conflict://<N>` (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://<N>", content })`. Inspect a block by reading `conflict://<N>` (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.",
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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://<N>`");
|
||||
expect(text).toContain("NOTICE: Inspect a block by reading `conflict://<N>`");
|
||||
expect(text).toContain('`write({ path: "conflict://<N>", content })`');
|
||||
expect(text).toContain('`write({ path: "conflict://*", content })`');
|
||||
expect(text).toContain("@ours");
|
||||
|
||||
@@ -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://<N>`");
|
||||
expect(text).toContain("NOTICE: Inspect a block by reading `conflict://<N>`");
|
||||
expect(text).toContain('`write({ path: "conflict://<N>", 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://<N>`", async () => {
|
||||
it("renders the full conflict block via reads of `conflict://<N>`", 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://<N>/theirs`", async () => {
|
||||
it("renders only the theirs body via reads of `conflict://<N>/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 <path>:conflicts` lists every conflict in the file with stable ids", async () => {
|
||||
it("the `<path>: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`");
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user