diff --git a/docs/tools/github.md b/docs/tools/github.md index 4ce0551bd..268cbfc6f 100644 --- a/docs/tools/github.md +++ b/docs/tools/github.md @@ -17,12 +17,10 @@ | Field | Type | Required | Description | | --- | --- | --- | --- | -| `op` | `"repo_view" \| "pr_create" \| "pr_diff" \| "pr_checkout" \| "pr_push" \| "search_issues" \| "search_prs" \| "search_code" \| "search_commits" \| "search_repos" \| "run_watch"` | Yes | Dispatch selector. `GithubTool.execute()` switches only on this field. | +| `op` | `"repo_view" \| "pr_create" \| "pr_checkout" \| "pr_push" \| "search_issues" \| "search_prs" \| "search_code" \| "search_commits" \| "search_repos" \| "run_watch"` | Yes | Dispatch selector. `GithubTool.execute()` switches only on this field. | | `repo` | `string` | No | `owner/repo` override. Ignored when the identifier argument is already a full GitHub URL. Required in practice when `gh` cannot infer repo context from the current checkout. | | `branch` | `string` | No | Used by `repo_view`, `pr_push`, and `run_watch`. `run_watch` falls back to current git branch when `run` is omitted; `pr_push` falls back to current branch. | -| `pr` | `string \| string[]` | No | Used by `pr_diff`, `pr_checkout`. Each item may be a PR number, branch name, or GitHub PR URL. Array form enables batching. Omitted means current branch PR. | -| `nameOnly` | `boolean` | No | Used only by `pr_diff`; adds `--name-only`. | -| `exclude` | `string[]` | No | Used only by `pr_diff`; each entry becomes `--exclude `. Empty strings are rejected. | +| `pr` | `string \| string[]` | No | Used by `pr_checkout`. Each item may be a PR number, branch name, or GitHub PR URL. Array form enables batching. Omitted means current branch PR. | | `force` | `boolean` | No | Used only by `pr_checkout`. Defaults to `false`; allows resetting an existing `pr-` local branch to the PR head commit. | | `forceWithLease` | `boolean` | No | Used only by `pr_push`; passed through to git push. | | `title` | `string` | No | Used only by `pr_create`. Required unless `fill` is `true`. | @@ -63,7 +61,7 @@ The tool returns a single text result built by `buildTextResult()` in `packages/ - maps common auth/repo-context failures into tool-facing `ToolError` messages; - `json()` rejects empty or invalid JSON. 5. Read-style ops (`repo_view`, `search_*`) fetch JSON and format Markdown-like text summaries. Single-issue and single-PR views were moved out of the tool and now resolve through the `issue://` / `pr://` internal URL schemes, which share the same SQLite cache. -7. `pr_diff` fetches raw text from `gh pr diff`; multi-PR batches are handled with `Promise.all()`. +7. PR diffs moved out of the tool. `pr:///diff` lists changed files, `pr:///diff/` slices a single file, and `pr:///diff/all` returns the full unified diff — see `docs/tools/read.md`. All three variants share one `gh pr diff` invocation through the `pr-diff` cache row. 8. `pr_checkout` resolves PR metadata first, then enters `git.withRepoLock()` before any git mutation so parallel checkout calls for the same primary repo do not race on shared `.git` state. 9. `pr_push` reads PR head metadata back from git branch config, derives a refspec, then pushes with `git.push()`. 10. `pr_create` shells out once, then best-effort re-reads the created PR for a richer summary. @@ -84,7 +82,7 @@ The tool returns a single text result built by `buildTextResult()` in `packages/ If `repo` is omitted, `gh` repository resolution is used. -Single-issue and single-PR reads live in the `issue://` / `pr://` URL schemes (see `docs/tools/read.md`). They share `~/.omp/cache/github-cache.db` (override via `OMP_GITHUB_CACHE_DB`) and the `github.cache.softTtlSec` / `github.cache.hardTtlSec` / `github.cache.enabled` settings. Root and repo-scoped reads (`issue://`, `pr://owner/repo`) issue a live `gh issue list` / `gh pr list` for browsing; query params `state`, `limit`, `author`, `label` pass through to `gh`. +Single-issue and single-PR reads live in the `issue://` / `pr://` URL schemes (see `docs/tools/read.md`). They share `~/.omp/cache/github-cache.db` (override via `OMP_GITHUB_CACHE_DB`) and the `github.cache.softTtlSec` / `github.cache.hardTtlSec` / `github.cache.enabled` settings. The cache retains rendered Markdown plus the raw JSON payload returned by `gh`, including private bodies, comments, reviews, and review comments when comments are enabled; rows are scoped by the local GitHub credential fingerprint. Root and repo-scoped reads (`issue://`, `pr://owner/repo`) issue a live `gh issue list` / `gh pr list` for browsing; query params `state`, `limit`, `author`, `label` pass through to `gh` (`issue://` accepts `state=open|closed|all`; `pr://` also accepts `merged`). PR diffs ride the same cache under `pr:///diff[/…]`: the listing, full diff, and per-file slices all share one `pr-diff` row keyed by repo and PR number. ### `pr_create` @@ -101,18 +99,6 @@ Branches: - Non-empty `body` is written under a temp dir `gh-pr-body-*` in `os.tmpdir()`, passed as `--body-file`, then removed in `finally`. - After creation, the tool parses the returned URL and best-effort runs `gh pr view --repo --json `; failures there are swallowed. -### `pr_diff` - -| Aspect | Value | -| --- | --- | -| Required fields | `op` | -| Optional fields | `repo`, `pr`, `nameOnly`, `exclude[]` | -| `gh` command | For each requested PR: `gh pr diff [] [--repo ] --color never [--name-only] [--exclude ...]` | -| Batching | Yes. `pr` normalization mirrors `pr_checkout`; each requested PR is launched with `Promise.all()`. | -| Output | Single PR: `# Pull Request Diff` or `# Pull Request Files` followed by raw CLI output. Batched: `# Pull Request Diffs` / `File Lists` plus labeled sections. | - -Diff stdout is preserved without trimming. Empty output becomes `No diff output.` or `No changed files.`. - ### `pr_checkout` | Aspect | Value | @@ -255,7 +241,7 @@ Watch flow: - PR review comments page size: `100` (`REVIEW_COMMENTS_PAGE_SIZE`). - Actions jobs page size: `100` (`RUN_JOBS_PAGE_SIZE`). - Search and tail numeric inputs are floored with `Math.floor()`, clamped to the max, and rejected when non-finite or `<= 0`. -- `pr_diff`/`pr_checkout` batch fan-out is unbounded in tool code; all requested PRs are launched with `Promise.all()`. +- `pr_checkout` batch fan-out is unbounded in tool code; all requested PRs are launched with `Promise.all()`. ## Errors - Tool creation is skipped entirely when `gh` is not installed. @@ -266,11 +252,10 @@ Watch flow: - otherwise stderr/stdout text, or fallback `GitHub CLI command failed: gh ...` - `json()` also throws on empty stdout or invalid JSON. - Local validation errors throw `ToolError`, including: - - missing required per-op fields (`issue`, `query`, `title unless fill=true`) + - missing required per-op fields (`query`, `title unless fill=true`) - invalid numeric `limit` / `tail` - invalid `run` format - `fill` combined with `title` or `body` - - empty exclude patterns - missing git repo / branch / HEAD context for checkout, push, or watch - `pr_push` on a branch without `ompPrHeadRef` metadata - conflicting existing worktree path or branch without `force` diff --git a/docs/tools/read.md b/docs/tools/read.md index 0b2e9fb92..d71e93320 100644 --- a/docs/tools/read.md +++ b/docs/tools/read.md @@ -201,7 +201,7 @@ URL selectors are parsed separately in `packages/coding-agent/src/tools/fetch.ts - otherwise paginates the resolved text in memory - passes `immutable` through to `resolveFileDisplayMode()` so anchors are suppressed for immutable resources such as artifacts, skills, memory, and agent outputs - sets `ignoreResultLimits: true` for `skill://` so the full skill text is paginated only by explicit selectors, not by the normal default line limit -- `issue://` / `pr://` (and the long form `issue:////` / `pr:////`) route through the same SQLite cache the `github` tool writes to; `?comments=0` selects the no-comments rendering. Bare `issue://` / `pr://` (and `issue:///` / `pr:///`) issue a live `gh issue list` / `gh pr list` for browsing, accepting `?state=`, `?limit=`, `?author=`, `?label=`. Soft TTL `github.cache.softTtlSec` (default 5 minutes), hard TTL `github.cache.hardTtlSec` (default 7 days). Stale-hit returns the cached row and schedules a background refresh. +- `issue://` / `pr://` (and the long form `issue:////` / `pr:////`) route through the same SQLite cache the `github` tool writes to; `?comments=0` selects the no-comments rendering. Bare `issue://` / `pr://` (and `issue:///` / `pr:///`) issue a live `gh issue list` / `gh pr list` for browsing, accepting `?state=`, `?limit=`, `?author=`, `?label=`. PR diffs share the same cache through `pr:///diff` (numbered file listing with per-file hints), `pr:///diff/` (single file slice; 1-indexed), and `pr:///diff/all` (verbatim unified diff); the listing and per-file slices are reconstructed from the cached unified-diff payload, so all three variants share one `gh pr diff` invocation per PR. Diff content is served as `text/plain`. Soft TTL `github.cache.softTtlSec` (default 5 minutes), hard TTL `github.cache.hardTtlSec` (default 7 days). Stale-hit returns the cached row and schedules a background refresh. ### Web URLs - `parseReadUrlTarget()` accepts `http://`, `https://`, or `www.` targets. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 316ffd0f6..b0075f6cc 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,29 +1,37 @@ # Changelog ## [Unreleased] - ### 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`), `pr_diff`, `repo_view`, `search_*`, and `run_watch` are unchanged. +- 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. ### Added -- Added `issue://` / `pr://` internal-URL schemes that share a SQLite-backed cache with the rest of the `github` tool. Single-item reads (`issue://`, `issue:////`) return rendered markdown and within `github.cache.softTtlSec` (default 5 minutes) skip the `gh` round-trip entirely; within `github.cache.hardTtlSec` (default 7 days) the cached row is returned and a background refresh is scheduled. Root and repo-scoped reads (`issue://`, `pr://owner/repo`) issue a live `gh issue list` / `gh pr list` for browsing, supporting `?state=open|closed|merged|all`, `?limit=`, `?author=`, `?label=` query params. Rendered output lands in `~/.omp/cache/github-cache.db` (override via `OMP_GITHUB_CACHE_DB`); disable the cache entirely with `github.cache.enabled = false`. Cwd→default-repo lookups (`gh repo view`) are memoized per-process. - +- Added markdown rendering for `read` results when content type is `text/markdown`, so GitHub internal-URL outputs are shown as formatted markdown instead of plain code blocks +- Added `pr:///diff`, `pr:///diff/`, and `pr:///diff/all` internal-URL shapes covering changed-file listings, per-file slices, and the full unified diff. They share one `pr-diff` SQLite cache row with the same TTL knobs as `pr://` views (`github.cache.softTtlSec` / `github.cache.hardTtlSec` / `github.cache.enabled`). Single PR views now advertise the diff entry point via a `Diff: pr://///diff` note. Cache schema bumped to `user_version = 2`; older rows are dropped on first open to relax the `kind` CHECK constraint. +- Added `issue://` / `pr://` internal-URL schemes that share a SQLite-backed cache with the rest of the `github` tool. Single-item reads (`issue://`, `issue:////`) return rendered markdown and within `github.cache.softTtlSec` (default 5 minutes) skip the `gh` round-trip entirely; within `github.cache.hardTtlSec` (default 7 days) the cached row is returned and a background refresh is scheduled. Root and repo-scoped reads (`issue://`, `pr://owner/repo`) issue a live `gh issue list` / `gh pr list` for browsing, supporting `?state=open|closed|all` for issues, `?state=open|closed|merged|all` for PRs, and `?limit=`, `?author=`, `?label=` query params. Rendered output lands in `~/.omp/cache/github-cache.db` (override via `OMP_GITHUB_CACHE_DB`); disable the cache entirely with `github.cache.enabled = false`. Cwd→default-repo lookups (`gh repo view`) are memoized per-process. - Added new `Approve and compact context` choice to the ExitPlanMode approval selector. Sits between `Approve and execute` (purge session) and `Approve and keep context` (full transcript) — runs `/compact` on the plan-mode transcript with a planning-specific summarization hint, then dispatches the plan-approved execution turn so it lands on a fresh cache anchor with the summarized rationale carried over. Cancelling the compaction (Esc or any other abort source) defers the execution dispatch and surfaces a warning so the operator can resubmit manually; non-abort failures proceed best-effort. - Added `CompactionCancelledError` typed sentinel and `CompactionOutcome` (`"ok" | "cancelled" | "failed"`) return type to `@oh-my-pi/pi-coding-agent/session/compaction`. `CommandController.executeCompaction` and `handleCompactCommand` now return the outcome instead of `void` so callers can discriminate user-driven aborts from generic failures without inspecting error messages. - Added a `credential_disabled` extension event so extensions can subscribe via `pi.on("credential_disabled", handler)` and react when `AuthStorage` automatically soft-disables a credential (e.g. OAuth `invalid_grant`). Replaces the current `agent_end` errorMessage regex pattern downstream extensions have to match against. Handler payload is `{ type, provider, disabledCause }`. `createAgentSession()` subscribes the per-session extension runner to the shared `AuthStorage` via `authStorage.onCredentialDisabled(...)` at the very top of session creation — before any startup model probes run — so events fire on every disable regardless of whether the embedder also has a constructor `onCredentialDisabled` handler attached. The SDK forwards through `ExtensionRunner.emitCredentialDisabled(event)`, which buffers events until `runner.initialize(...)` runs in the mode controller and then flushes them through `emit()` so extension handlers see populated UI/runtime context (rather than the constructor's no-op default with `hasUI=false`, an unset model, and no-op runtime actions). On `session.dispose()` the subscription is unsubscribed; the embedder's constructor-attached listener keeps firing through its own permanent subscription. The outer `createAgentSession()` catch also releases the subscription if startup throws before the dispose-wrap is wired, so repeated retries don't accumulate dead listeners. ### Changed +- Changed issue and pull-request list entries to link to repository-qualified URLs (for example `issue://owner/repo/`) so list items open correctly outside the default repo - Aligned prompt instruction language by defining `NEVER` and `AVOID` as strict aliases for `MUST NOT` and `SHOULD NOT` in the system prompt, and standardized agent, tool, and system prompt templates to use those terms consistently ### Fixed +- Fixed GitHub view caching to account for active credential identity and avoid serving cached issue/PR data across different account/token contexts +- Fixed `read` call tracking so calls without an explicit path or URL target no longer appear as regular file reads in the execution tracker - Fixed `createAgentSession()` subscribing the `credential_disabled` bridge to a freshly discovered `AuthStorage` orphan when an embedder supplied only `options.modelRegistry` (no `options.authStorage`). Refresh failures emitted by `modelRegistry.getApiKey()` flow through `modelRegistry.authStorage`, so a divergent local instance silently swallowed every disable event and also leaked into the `mcpManager` and session result. The SDK now reconciles `authStorage` to `modelRegistry.authStorage` up front and rejects mismatched `options.authStorage`/`options.modelRegistry.authStorage` pairs at session construction. - Fixed `runSubagent` (subagent task executor) carrying the same latent `AuthStorage`/`ModelRegistry` divergence as `createAgentSession()`: when only `options.modelRegistry` was supplied, the executor previously fell through to a fresh `discoverAuthStorage()` and handed that orphan into `createAgentSession()` alongside a registry whose `.authStorage` was a different instance. The executor now reconciles to `modelRegistry.authStorage` before any further work and rejects mismatched `options.authStorage`/`options.modelRegistry.authStorage` pairs the same way the SDK does, so subagents can no longer silently observe a different storage view than their parent. - Fixed `github` tool's `search_issues`/`search_prs`/`search_code`/`search_commits`/`search_repos` ops always returning 0 results when the query contained more than one qualifier (e.g. `is:merged is:pr`, `is:open author:foo`). `gh search …` since the `advanced_search=true` rollout in gh 2.92 silently wraps multi-token positional queries in parentheses and quotes everything after the first qualifier as that qualifier's value (`is:"merged is:pr"`), which GitHub then matches as a literal state filter that no PR can satisfy. The tool now calls `gh api -X GET /search/ -f q=… -F per_page=…` directly so the qualifiers reach GitHub's search API verbatim. `is:issue`/`is:pr` and `repo:/` are appended internally to preserve the previous CLI-flag behavior; the user-facing query string in the formatted output is unchanged. `state` for merged PRs is derived from `pull_request.merged_at` so the rendered `State:` line stays `merged`/`closed`/`open` as before. +### Security + +- Secured the GitHub cache store with strict file permissions (`0700` directories and `0600` files) to reduce local cache exposure + ## [14.9.9] - 2026-05-12 ### Added diff --git a/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts b/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts index ae53b303c..92d0ce11a 100644 --- a/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts +++ b/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts @@ -19,7 +19,14 @@ */ import type { Settings } from "../config/settings"; import { AgentRegistry } from "../registry/agent-registry"; -import { getOrFetchIssue, getOrFetchPr, parsePositiveDecimalInt, resolveDefaultRepoMemoized } from "../tools/gh"; +import { + getOrFetchIssue, + getOrFetchPr, + getOrFetchPrDiff, + type PrDiffFile, + parsePositiveDecimalInt, + resolveDefaultRepoMemoized, +} from "../tools/gh"; import { formatFreshnessNote } from "../tools/github-cache"; import * as git from "../utils/git"; import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext } from "./types"; @@ -33,6 +40,19 @@ interface ParsedSingle { comments: boolean; } +interface ParsedPrDiff { + kind: "pr-diff"; + repo?: string; + number: number; + /** + * `list` → enumerate changed files. + * `all` → full unified diff. + * `slice`→ single file's diff section (1-indexed `index`). + */ + mode: "list" | "all" | "slice"; + index?: number; +} + interface ParsedList { kind: "list"; repo?: string; @@ -42,7 +62,7 @@ interface ParsedList { label: string | undefined; } -type Parsed = ParsedSingle | ParsedList; +type Parsed = ParsedSingle | ParsedList | ParsedPrDiff; const LIST_LIMIT_DEFAULT = 30; const LIST_LIMIT_MAX = 100; @@ -78,42 +98,83 @@ function parseUrl(url: InternalUrl, scheme: Scheme): Parsed { const pathname = (url.rawPathname ?? url.pathname).replace(/^\/+/, ""); const parts = pathname ? pathname.split("/").filter(Boolean) : []; - // scheme:// → list default repo - // scheme://N → single item, default repo - // scheme://owner/repo → list specific repo - // scheme://owner/repo/N → single item, specific repo + // Shapes: + // scheme:// → list default repo + // scheme://N → single item, default repo + // scheme://owner/repo → list specific repo + // scheme://owner/repo/N → single item, specific repo + // pr://N/diff[/] → diff family, default repo + // pr://owner/repo/N/diff[/] → diff family, specific repo let repo: string | undefined; - let tail: string | undefined; + let numberPart: string | undefined; + let diffParts: string[] = []; if (!host && parts.length === 0) { return parseListOptions(url, scheme, undefined); } if (host && parts.length === 0) { // scheme://N (numeric) or scheme://owner (host-only, no repo segment) - tail = host; + numberPart = host; + } else if (host && parts[0] === "diff") { + // pr://N/diff[/] — short form with diff suffix + numberPart = host; + diffParts = parts; } else if (host && parts.length === 1) { // scheme://owner/repo → list repo = `${host}/${parts[0]}`; return parseListOptions(url, scheme, repo); - } else if (host && parts.length === 2) { - // scheme://owner/repo/N → single + } else if (host && parts.length >= 2) { + // scheme://owner/repo/N[/diff[/]] repo = `${host}/${parts[0]}`; - tail = parts[1]; + numberPart = parts[1]; + diffParts = parts.slice(2); } else { throw new Error( `Invalid ${scheme}:// URL. Expected ${scheme}://, ${scheme}://, ${scheme}:///, or ${scheme}:////`, ); } - const num = parsePositiveDecimalInt(tail); - if (num === undefined) { - throw new Error(`Invalid ${scheme}:// number: ${tail ?? "(missing)"}`); + // Reject unrecognized trailing segments before parsing the number so + // shapes like `issue://owner/repo/foo/bar` surface as "Invalid URL" + // rather than the misleading "Invalid number: foo". + if (diffParts.length > 0) { + if (scheme === "issue") { + throw new Error( + `Invalid issue:// URL. Issue views do not have a diff; use pr://///diff for pull requests.`, + ); + } + if (diffParts[0] !== "diff" || diffParts.length > 2) { + throw new Error( + `Invalid pr:// URL. Expected pr://, pr:///diff, pr:///diff/all, or pr:///diff/`, + ); + } } - const commentsParam = url.searchParams.get("comments"); - const comments = commentsParam === null ? true : !(commentsParam === "0" || commentsParam.toLowerCase() === "false"); + const num = parsePositiveDecimalInt(numberPart); + if (num === undefined) { + throw new Error(`Invalid ${scheme}:// number: ${numberPart ?? "(missing)"}`); + } - return { kind: "single", repo, number: num, comments }; + if (diffParts.length === 0) { + const commentsParam = url.searchParams.get("comments"); + const comments = + commentsParam === null ? true : !(commentsParam === "0" || commentsParam.toLowerCase() === "false"); + return { kind: "single", repo, number: num, comments }; + } + + // diffParts has already been validated above; scheme is `pr`. + if (diffParts.length === 1) { + return { kind: "pr-diff", repo, number: num, mode: "list" }; + } + const sub = diffParts[1] ?? ""; + if (sub === "all") { + return { kind: "pr-diff", repo, number: num, mode: "all" }; + } + const idx = parsePositiveDecimalInt(sub); + if (idx === undefined) { + throw new Error(`Invalid pr:// diff sub-path '${sub}'. Use 'all' or a 1-indexed file number.`); + } + return { kind: "pr-diff", repo, number: num, mode: "slice", index: idx }; } /** @@ -179,7 +240,7 @@ interface PrListItem extends IssueListItem { headRefName?: string; } -function formatListItem(scheme: Scheme, item: IssueListItem | PrListItem): string { +function formatListItem(scheme: Scheme, repo: string, item: IssueListItem | PrListItem): string { const number = item.number ?? "?"; const title = item.title ?? "(no title)"; const state = item.state?.toLowerCase() ?? "?"; @@ -191,7 +252,8 @@ function formatListItem(scheme: Scheme, item: IssueListItem | PrListItem): strin .filter(Boolean) .join(", "); const labelSuffix = labels ? ` labels: ${labels}` : ""; - return `- [${state}${draftSuffix}] #${number} @${author} ${updated}\n ${title}${labelSuffix}\n ${scheme}://${number}`; + const itemUrl = number === "?" ? `${scheme}://${repo}` : `${scheme}://${repo}/${number}`; + return `- [${state}${draftSuffix}] #${number} @${author} ${updated}\n ${title}${labelSuffix}\n ${itemUrl}`; } async function fetchAndRenderList( @@ -240,7 +302,8 @@ async function fetchAndRenderList( scheme === "issue" ? `# Issues in ${repo} (${options.state}, up to ${options.limit})` : `# Pull Requests in ${repo} (${options.state}, up to ${options.limit})`; - const body = items.length === 0 ? "_No matches._" : items.map(item => formatListItem(scheme, item)).join("\n\n"); + const body = + items.length === 0 ? "_No matches._" : items.map(item => formatListItem(scheme, repo, item)).join("\n\n"); const footer = `\n\n---\nRead a specific item: \`${scheme}://${repo}/\` (or \`${scheme}://\` for the current repo).`; const rendered = `${header}\n\n${body}${footer}`; @@ -255,15 +318,31 @@ async function fetchAndRenderList( interface BuildSingleArgs { url: InternalUrl; + scheme: Scheme; parsed: ParsedSingle; rendered: string; status: "miss" | "fresh" | "stale" | "disabled"; fetchedAt: number; + /** Resolved repo (post short-form expansion) — used for the PR-only diff hint. */ + repo?: string; } -function buildSingleResource({ url, parsed, rendered, status, fetchedAt }: BuildSingleArgs): InternalResource { +function buildSingleResource({ + url, + scheme, + parsed, + rendered, + status, + fetchedAt, + repo, +}: BuildSingleArgs): InternalResource { const notes: string[] = [formatFreshnessNote(status, fetchedAt)]; if (!parsed.comments) notes.push("Comments disabled"); + if (scheme === "pr") { + const repoSegment = repo ?? parsed.repo; + const diffUrl = repoSegment ? `pr://${repoSegment}/${parsed.number}/diff` : `pr://${parsed.number}/diff`; + notes.push(`Diff: ${diffUrl}`); + } return { url: url.href, content: rendered, @@ -273,6 +352,95 @@ function buildSingleResource({ url, parsed, rendered, status, fetchedAt }: Build }; } +function formatFileLine(idx: number, file: PrDiffFile, repo: string, prNumber: number): string { + const stats = file.changeType === "binary" ? "(binary)" : `+${file.additions} -${file.deletions}`; + const rename = file.oldPath ? ` (renamed from ${file.oldPath})` : ""; + return `${idx}. ${file.path} ${stats} [${file.changeType}]${rename}\n pr://${repo}/${prNumber}/diff/${idx}`; +} + +async function fetchAndRenderPrDiff( + url: InternalUrl, + parsed: ParsedPrDiff, + context: ResolveContext | undefined, +): Promise { + const cwd = resolveCwd(context); + let repo = parsed.repo; + if (!repo) { + try { + repo = await resolveDefaultRepoMemoized(cwd, context?.signal); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error( + `pr://${parsed.number}/diff could not resolve a default repo from the current session: ${message}\nUse pr:////${parsed.number}/diff.`, + ); + } + } + const lookup = await getOrFetchPrDiff({ + cwd, + repo, + number: parsed.number, + signal: context?.signal, + settings: settingsFromContext(context), + }); + const files = lookup.payload.files; + const freshness = formatFreshnessNote(lookup.status, lookup.fetchedAt); + + if (parsed.mode === "all") { + const content = lookup.payload.unified; + return { + url: url.href, + content, + contentType: "text/plain", + size: Buffer.byteLength(content, "utf-8"), + notes: [ + freshness, + `Full diff for pr://${repo}/${parsed.number} (${files.length} file${files.length === 1 ? "" : "s"})`, + ], + }; + } + + if (parsed.mode === "slice") { + const index = parsed.index ?? 0; + if (index < 1 || index > files.length) { + throw new Error( + `pr://${repo}/${parsed.number}/diff/${index} is out of range; PR has ${files.length} file${files.length === 1 ? "" : "s"}. Use pr://${repo}/${parsed.number}/diff to list available indices.`, + ); + } + const file = files[index - 1]; + if (!file) { + throw new Error(`pr://${repo}/${parsed.number}/diff/${index} resolved to a missing slice (parser bug).`); + } + const content = lookup.payload.unified.slice(file.startOffset, file.endOffset); + return { + url: url.href, + content, + contentType: "text/plain", + size: Buffer.byteLength(content, "utf-8"), + notes: [ + freshness, + `Showing file ${index}/${files.length}: ${file.path}`, + `Read all: pr://${repo}/${parsed.number}/diff/all`, + ], + }; + } + + // mode === "list" + const header = `# Pull Request Diff: ${repo}#${parsed.number} (${files.length} file${files.length === 1 ? "" : "s"})`; + const body = + files.length === 0 + ? "_No file changes._" + : files.map((f, i) => formatFileLine(i + 1, f, repo, parsed.number)).join("\n\n"); + const footer = `\n\n---\nRead all: \`pr://${repo}/${parsed.number}/diff/all\`. Each file is also available as \`pr://${repo}/${parsed.number}/diff/\`.`; + const content = `${header}\n\n${body}${footer}`; + return { + url: url.href, + content, + contentType: "text/markdown", + size: Buffer.byteLength(content, "utf-8"), + notes: [freshness, `File listing for pr://${repo}/${parsed.number}`], + }; +} + /** * Handler for `issue://` URLs. */ @@ -290,6 +458,11 @@ export class IssueProtocolHandler implements ProtocolHandler { throw new Error(`issue:// listing failed: ${message}`); } } + // parseUrl already rejects `issue://.../diff`; this guard is a belt-and- + // suspenders catch in case the union grows. + if (parsed.kind !== "single") { + throw new Error(`Invalid issue:// URL: unexpected variant '${parsed.kind}'`); + } try { const lookup = await getOrFetchIssue({ cwd: resolveCwd(context), @@ -301,6 +474,7 @@ export class IssueProtocolHandler implements ProtocolHandler { }); return buildSingleResource({ url, + scheme: "issue", parsed, rendered: lookup.rendered, status: lookup.status, @@ -330,6 +504,14 @@ export class PrProtocolHandler implements ProtocolHandler { throw new Error(`pr:// listing failed: ${message}`); } } + if (parsed.kind === "pr-diff") { + try { + return await fetchAndRenderPrDiff(url, parsed, context); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`pr:// diff resolution failed: ${message}`); + } + } const cwd = resolveCwd(context); let repo = parsed.repo; if (!repo) { @@ -353,10 +535,12 @@ export class PrProtocolHandler implements ProtocolHandler { }); return buildSingleResource({ url, + scheme: "pr", parsed, rendered: lookup.rendered, status: lookup.status, fetchedAt: lookup.fetchedAt, + repo, }); } catch (err) { const message = err instanceof Error ? err.message : String(err); diff --git a/packages/coding-agent/src/modes/components/read-tool-group.ts b/packages/coding-agent/src/modes/components/read-tool-group.ts index c748345d3..81e6208d8 100644 --- a/packages/coding-agent/src/modes/components/read-tool-group.ts +++ b/packages/coding-agent/src/modes/components/read-tool-group.ts @@ -12,15 +12,22 @@ import type { ToolExecutionHandle } from "./tool-execution"; * resolved content is visible. `path` is the canonical arg; `file_path` is the * legacy alias still tolerated by the read tool schema. */ -export function readArgsTargetInternalUrl(args: unknown): boolean { - if (!args || typeof args !== "object" || Array.isArray(args)) return false; +function readArgsTarget(args: unknown): string | undefined { + if (!args || typeof args !== "object" || Array.isArray(args)) return undefined; const record = args as Record; - const target = - typeof record.path === "string" - ? record.path - : typeof record.file_path === "string" - ? record.file_path - : undefined; + return typeof record.path === "string" + ? record.path + : typeof record.file_path === "string" + ? record.file_path + : undefined; +} + +export function readArgsHaveTarget(args: unknown): boolean { + return readArgsTarget(args) !== undefined; +} + +export function readArgsTargetInternalUrl(args: unknown): boolean { + const target = readArgsTarget(args); if (!target) return false; return InternalUrlRouter.instance().canHandle(target); } diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 1de7204fc..9b7b783a0 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -3,7 +3,11 @@ import type { AssistantMessage, ImageContent } from "@oh-my-pi/pi-ai"; import { type Component, Loader, TERMINAL, Text } from "@oh-my-pi/pi-tui"; import { settings } from "../../config/settings"; import { AssistantMessageComponent } from "../../modes/components/assistant-message"; -import { ReadToolGroupComponent, readArgsTargetInternalUrl } from "../../modes/components/read-tool-group"; +import { + ReadToolGroupComponent, + readArgsHaveTarget, + readArgsTargetInternalUrl, +} from "../../modes/components/read-tool-group"; import { TodoReminderComponent } from "../../modes/components/todo-reminder"; import { ToolExecutionComponent } from "../../modes/components/tool-execution"; import { TtsrNotificationComponent } from "../../modes/components/ttsr-notification"; @@ -273,7 +277,11 @@ export class EventController { for (const content of this.ctx.streamingMessage.content) { if (content.type !== "toolCall") continue; - if (content.name === "read" && !readArgsTargetInternalUrl(content.arguments)) { + if ( + content.name === "read" && + readArgsHaveTarget(content.arguments) && + !readArgsTargetInternalUrl(content.arguments) + ) { this.#trackReadToolCall(content.id, content.arguments); const component = this.ctx.pendingTools.get(content.id); if (component) { @@ -384,7 +392,7 @@ export class EventController { async #handleToolExecutionStart(event: Extract): Promise { this.#updateWorkingMessageFromIntent(event.intent); if (!this.ctx.pendingTools.has(event.toolCallId)) { - if (event.toolName === "read" && !readArgsTargetInternalUrl(event.args)) { + if (event.toolName === "read" && readArgsHaveTarget(event.args) && !readArgsTargetInternalUrl(event.args)) { this.#trackReadToolCall(event.toolCallId, event.args); const component = this.ctx.pendingTools.get(event.toolCallId); if (component) { diff --git a/packages/coding-agent/src/modes/utils/ui-helpers.ts b/packages/coding-agent/src/modes/utils/ui-helpers.ts index 3905dc62a..64f7a7f5b 100644 --- a/packages/coding-agent/src/modes/utils/ui-helpers.ts +++ b/packages/coding-agent/src/modes/utils/ui-helpers.ts @@ -9,7 +9,11 @@ import { CompactionSummaryMessageComponent } from "../../modes/components/compac import { CustomMessageComponent } from "../../modes/components/custom-message"; import { DynamicBorder } from "../../modes/components/dynamic-border"; import { EvalExecutionComponent } from "../../modes/components/eval-execution"; -import { ReadToolGroupComponent, readArgsTargetInternalUrl } from "../../modes/components/read-tool-group"; +import { + ReadToolGroupComponent, + readArgsHaveTarget, + readArgsTargetInternalUrl, +} from "../../modes/components/read-tool-group"; import { SkillMessageComponent } from "../../modes/components/skill-message"; import { ToolExecutionComponent } from "../../modes/components/tool-execution"; import { UserMessageComponent } from "../../modes/components/user-message"; @@ -302,7 +306,11 @@ export class UiHelpers { continue; } - if (content.name === "read" && !readArgsTargetInternalUrl(content.arguments)) { + if ( + content.name === "read" && + readArgsHaveTarget(content.arguments) && + !readArgsTargetInternalUrl(content.arguments) + ) { if (hasErrorStop && errorMessage) { if (!readGroup) { readGroup = new ReadToolGroupComponent({ diff --git a/packages/coding-agent/src/prompts/tools/github.md b/packages/coding-agent/src/prompts/tools/github.md index d0fa0ee90..021f71b6a 100644 --- a/packages/coding-agent/src/prompts/tools/github.md +++ b/packages/coding-agent/src/prompts/tools/github.md @@ -1,10 +1,9 @@ -GitHub CLI tool with a single op-based dispatch. Wraps `gh` for repositories, pull requests, search, checkout, push, and Actions watch workflows. For reading a single issue or PR view, use the `issue://` or `pr://` URL schemes (cached automatically) — they replace what used to be `op: issue_view` and `op: pr_view`. +GitHub CLI tool with a single op-based dispatch. Wraps `gh` for repositories, pull requests, search, checkout, push, and Actions watch workflows. For reading a single issue or PR view, use the `issue://` or `pr://` URL schemes (cached automatically) — they replace what used to be `op: issue_view` and `op: pr_view`. For reading PR diffs, use `pr:///diff` (changed-file listing), `pr:///diff/` (single file slice, 1-indexed), or `pr:///diff/all` (full unified diff) — they replace what used to be `op: pr_diff`. Pick the operation via `op`. Each op uses a subset of the parameters: - `repo_view` — Read repository metadata. Optional `repo` (owner/repo) and `branch`. Falls back to the current checkout or default `gh` repo. - `pr_create` — Create a pull request. Either provide `title` (and optional `body`) or set `fill: true` to auto-fill from commits. Optional `base` (target, defaults to repo default), `head` (source, defaults to current branch), `draft`, `repo`, `reviewer[]`, `assignee[]`, `label[]`. Returns the new PR URL plus a summary. -- `pr_diff` — Read one or more pull request diffs. Optional `pr` (single identifier or array for batch). Optional `repo`. Set `nameOnly: true` for changed file names. Use `exclude` to drop generated paths from the diff. - `pr_checkout` — Check one or more pull requests out into dedicated git worktrees. Optional `pr` (number, URL, branch, or array of any of those — pass an array to batch-check-out multiple PRs in one call), `repo`, `force` (reset existing local branch). - `pr_push` — Push a checked-out PR branch back to its source branch. Requires the branch to have been checked out via `op: pr_checkout` (carries push metadata). Optional `branch`; defaults to the current checked-out git branch. Optional `forceWithLease`. - `search_issues` — Search issues using normal GitHub issue search syntax. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`, `dateField`. diff --git a/packages/coding-agent/src/tools/gh-renderer.ts b/packages/coding-agent/src/tools/gh-renderer.ts index d4923eb54..cb01261ea 100644 --- a/packages/coding-agent/src/tools/gh-renderer.ts +++ b/packages/coding-agent/src/tools/gh-renderer.ts @@ -39,7 +39,6 @@ const FALLBACK_WIDTH = 80; const OP_TITLES: Record = { repo_view: "GitHub Repo", - pr_diff: "GitHub PR Diff", pr_checkout: "GitHub PR Checkout", pr_push: "GitHub PR Push", search_issues: "GitHub Search Issues", @@ -82,7 +81,6 @@ function buildOpMeta(args: GithubToolRenderArgs): string[] { const meta: string[] = []; const op = args.op; switch (op) { - case "pr_diff": case "pr_checkout": case "pr_push": { const id = formatPrIdentifier(args.pr); diff --git a/packages/coding-agent/src/tools/gh.ts b/packages/coding-agent/src/tools/gh.ts index 2024560fe..68bcfe887 100644 --- a/packages/coding-agent/src/tools/gh.ts +++ b/packages/coding-agent/src/tools/gh.ts @@ -10,7 +10,7 @@ import githubDescription from "../prompts/tools/github.md" with { type: "text" } import * as git from "../utils/git"; import type { ToolSession } from "."; import { formatShortSha } from "./gh-format"; -import { type CacheStatus, getOrFetchView } from "./github-cache"; +import { type CacheStatus, getOrFetchView, resolveGithubCacheAuthKey } from "./github-cache"; import type { OutputMeta } from "./output-meta"; import { ToolError, throwIfAborted } from "./tool-errors"; import { toolResult } from "./tool-result"; @@ -201,7 +201,6 @@ const githubSchema = Type.Object({ [ "repo_view", "pr_create", - "pr_diff", "pr_checkout", "pr_push", "search_issues", @@ -235,16 +234,10 @@ const githubSchema = Type.Object({ ], { description: - "pr number, url, or branch (pr_diff, pr_checkout); pass an array to batch-process multiple pull requests in one call", + "pr number, url, or branch (pr_checkout); pass an array to batch-process multiple pull requests in one call", }, ), ), - nameOnly: Type.Optional(Type.Boolean({ description: "return file names only (pr_diff)" })), - exclude: Type.Optional( - Type.Array(Type.String({ description: "glob to exclude" }), { - description: "file globs to exclude (pr_diff)", - }), - ), force: Type.Optional(Type.Boolean({ description: "reset existing local branch (pr_checkout)" })), forceWithLease: Type.Optional(Type.Boolean({ description: "force-with-lease push (pr_push)" })), title: Type.Optional( @@ -2416,8 +2409,6 @@ export class GithubTool implements AgentTool return executeRepoView(this.session, params, signal); case "pr_create": return executePrCreate(this.session, params, signal); - case "pr_diff": - return executePrDiff(this.session, params, signal); case "pr_checkout": return executePrCheckout(this.session, params, signal); case "pr_push": @@ -2477,6 +2468,7 @@ export interface IssueViewLookupOptions { includeComments?: boolean; signal?: AbortSignal; settings?: Settings; + cacheAuthKey?: string | null; } export interface PrViewLookupOptions { @@ -2486,6 +2478,7 @@ export interface PrViewLookupOptions { includeComments?: boolean; signal?: AbortSignal; settings?: Settings; + cacheAuthKey?: string | null; } export interface ViewLookupResult { @@ -2538,6 +2531,7 @@ async function fetchPrViewFresh( export async function getOrFetchIssue(options: IssueViewLookupOptions): Promise> { const identifier = requireNonEmpty(options.issue, "issue"); const includeComments = options.includeComments ?? true; + const authKey = options.cacheAuthKey === undefined ? (resolveGithubCacheAuthKey() ?? null) : options.cacheAuthKey; const urlParse = parseIssueUrl(identifier); // Prefer the URL's repo when the identifier is a full URL; fall back to the // explicit `repo` option, then to the cwd's default repo. @@ -2570,6 +2564,7 @@ export async function getOrFetchIssue(options: IssueViewLookupOptions): Promise< number: cacheNumber, includeComments, settings: options.settings, + authKey, fetchFresh: doFetch, }); return { @@ -2588,6 +2583,7 @@ export async function getOrFetchIssue(options: IssueViewLookupOptions): Promise< */ export async function getOrFetchPr(options: PrViewLookupOptions): Promise> { const includeComments = options.includeComments ?? true; + const authKey = options.cacheAuthKey === undefined ? (resolveGithubCacheAuthKey() ?? null) : options.cacheAuthKey; const doFetch = () => fetchPrViewFresh(options.cwd, options.repo, options.number, includeComments, options.signal); const lookup = await getOrFetchView({ repo: options.repo, @@ -2595,6 +2591,7 @@ export async function getOrFetchPr(options: PrViewLookupOptions): Promise> { - const repo = normalizeOptionalString(params.repo); - const prList = normalizePrIdentifierList(params.pr); - const prRefs: (string | undefined)[] = prList.length > 0 ? prList : [undefined]; +// ──────────────────────────────────────────────────────────────────────────── +// PR diff fetcher +// +// Used by the `pr:///diff[/…]` internal-URL family. Stores the verbatim +// `gh pr diff` text plus a parsed file index so the listing, full-diff, and +// per-file slice variants all share one cache row. +// ──────────────────────────────────────────────────────────────────────────── - const diffs = await Promise.all( - prRefs.map(async prRef => { - const args = ["pr", "diff"]; - if (prRef) args.push(prRef); - appendRepoFlag(args, repo, prRef); - args.push("--color", "never"); - if (params.nameOnly) args.push("--name-only"); - for (const pattern of params.exclude ?? []) { - args.push("--exclude", requireNonEmpty(pattern, "exclude pattern")); - } - const output = await git.github.text(session.cwd, args, signal, { - repoProvided: Boolean(repo), - trimOutput: false, - }); - return { prRef, output }; - }), - ); +export interface PrDiffFile { + /** Display path. Prefers the post-image (`b/`) when present. */ + path: string; + additions: number; + deletions: number; + changeType: "modified" | "added" | "deleted" | "renamed" | "binary"; + /** Pre-image path for renames/deletes; same as `path` otherwise. */ + oldPath?: string; + /** Byte offset of the section's `diff --git` line in the unified diff. */ + startOffset: number; + /** Byte offset of the next section (or end-of-text). */ + endOffset: number; +} - const singleTitle = params.nameOnly ? "# Pull Request Files" : "# Pull Request Diff"; - const emptyBody = params.nameOnly ? "No changed files." : "No diff output."; +export interface PrDiffPayload { + /** Full unified diff text as returned by `gh pr diff --color never`. */ + unified: string; + files: PrDiffFile[]; +} - if (diffs.length === 1) { - const [diff] = diffs; - const body = diff.output.length > 0 ? diff.output : emptyBody; - return buildTextResult(`${singleTitle}\n\n${body}`); +export interface PrDiffLookupOptions { + cwd: string; + repo: string; + number: number; + signal?: AbortSignal; + settings?: Settings; + cacheAuthKey?: string | null; +} +/** + * Split `gh pr diff` output on `^diff --git ` boundaries and parse per-file + * metadata. The unified diff is preserved verbatim so callers can slice it by + * byte offsets without re-running gh. + */ +export function parsePrUnifiedDiff(text: string): PrDiffPayload { + const files: PrDiffFile[] = []; + if (text.length === 0) { + return { unified: text, files }; } - const header = params.nameOnly - ? `# ${diffs.length} Pull Request File Lists` - : `# ${diffs.length} Pull Request Diffs`; - const sections = diffs.map(diff => { - const label = diff.prRef ? `PR ${diff.prRef}` : "PR (current branch)"; - const body = diff.output.length > 0 ? diff.output : emptyBody; - return `## ${label}\n\n${body}`; + // Walk match positions manually so we capture each section's byte range. + const sectionStarts: number[] = []; + const re = /^diff --git /gm; + let m: RegExpExecArray | null = re.exec(text); + while (m !== null) { + sectionStarts.push(m.index); + // Avoid zero-length match infinite loop (regex has fixed prefix, but + // be explicit). + if (re.lastIndex === m.index) re.lastIndex += 1; + m = re.exec(text); + } + + for (let i = 0; i < sectionStarts.length; i += 1) { + const startOffset = sectionStarts[i] ?? 0; + const endOffset = sectionStarts[i + 1] ?? text.length; + const section = text.slice(startOffset, endOffset); + files.push(parsePrDiffSection(section, startOffset, endOffset)); + } + return { unified: text, files }; +} + +function parsePrDiffSection(section: string, startOffset: number, endOffset: number): PrDiffFile { + const lines = section.split("\n"); + const header = lines[0] ?? ""; + // `diff --git a/ b/` — paths may contain spaces, but gh emits + // them quoted with a leading `"`. We accept the common unquoted shape and + // fall back to the whole tail for quoted/exotic forms. + let oldPath: string | undefined; + let newPath: string | undefined; + const trail = header.slice("diff --git ".length); + const aIdx = trail.indexOf("a/"); + const bIdx = trail.indexOf(" b/"); + if (aIdx === 0 && bIdx > 0) { + oldPath = trail.slice(2, bIdx); + newPath = trail.slice(bIdx + 3); + } + + let changeType: PrDiffFile["changeType"] = "modified"; + let isBinary = false; + let additions = 0; + let deletions = 0; + + for (let li = 1; li < lines.length; li += 1) { + const line = lines[li] ?? ""; + if (line.startsWith("new file mode")) { + changeType = "added"; + continue; + } + if (line.startsWith("deleted file mode")) { + changeType = "deleted"; + continue; + } + if (line.startsWith("rename from ")) { + changeType = "renamed"; + oldPath = line.slice("rename from ".length); + continue; + } + if (line.startsWith("rename to ")) { + newPath = line.slice("rename to ".length); + continue; + } + if (line.startsWith("Binary files ") && line.endsWith(" differ")) { + isBinary = true; + continue; + } + // `+++ b/` / `--- a/` are headers, not content. + if (line.startsWith("+++") || line.startsWith("---")) continue; + if (line.startsWith("+")) { + additions += 1; + } else if (line.startsWith("-")) { + deletions += 1; + } + } + + if (isBinary) { + if (changeType === "modified") changeType = "binary"; + additions = 0; + deletions = 0; + } + + const displayPath = + changeType === "deleted" ? (oldPath ?? newPath ?? "(unknown)") : (newPath ?? oldPath ?? "(unknown)"); + const file: PrDiffFile = { + path: displayPath, + additions, + deletions, + changeType, + startOffset, + endOffset, + }; + if (oldPath && oldPath !== displayPath) { + file.oldPath = oldPath; + } + return file; +} + +async function fetchPrDiffFresh( + cwd: string, + repo: string, + number: number, + signal: AbortSignal | undefined, +): Promise<{ rendered: string; sourceUrl: string | undefined; payload: PrDiffPayload }> { + const args = ["pr", "diff", String(number), "--color", "never"]; + appendRepoFlag(args, repo, String(number)); + const text = await git.github.text(cwd, args, signal, { repoProvided: true, trimOutput: false }); + const payload = parsePrUnifiedDiff(text); + return { rendered: text, sourceUrl: undefined, payload }; +} + +/** + * Cache-aware PR diff fetcher. Stores the full unified diff plus a parsed + * file index in a single `pr-diff` cache row so the listing, full-diff, and + * per-file slice variants of `pr:///diff` share one `gh pr diff` + * invocation. + */ +export async function getOrFetchPrDiff(options: PrDiffLookupOptions): Promise> { + const authKey = options.cacheAuthKey === undefined ? (resolveGithubCacheAuthKey() ?? null) : options.cacheAuthKey; + const doFetch = () => fetchPrDiffFresh(options.cwd, options.repo, options.number, options.signal); + const lookup = await getOrFetchView({ + repo: options.repo, + kind: "pr-diff", + number: options.number, + includeComments: false, + settings: options.settings, + authKey, + fetchFresh: doFetch, }); - const text = [header, "", ...joinSections(sections)].join("\n").trim(); - return buildTextResult(text); + return { + rendered: lookup.rendered, + sourceUrl: lookup.sourceUrl, + payload: lookup.payload, + status: lookup.status, + fetchedAt: lookup.fetchedAt, + }; } function joinSections(sections: string[]): string[] { diff --git a/packages/coding-agent/src/tools/github-cache.ts b/packages/coding-agent/src/tools/github-cache.ts index d84721257..972555406 100644 --- a/packages/coding-agent/src/tools/github-cache.ts +++ b/packages/coding-agent/src/tools/github-cache.ts @@ -17,6 +17,7 @@ import { Database } from "bun:sqlite"; import * as fs from "node:fs"; +import * as os from "node:os"; import * as path from "node:path"; import { getGithubCacheDbPath, logger } from "@oh-my-pi/pi-utils"; import type { Settings } from "../config/settings"; @@ -25,9 +26,12 @@ import type { Settings } from "../config/settings"; // Storage layer // ──────────────────────────────────────────────────────────────────────────── -export type CacheKind = "issue" | "pr"; +export type CacheKind = "issue" | "pr" | "pr-diff"; + +const DEFAULT_CACHE_AUTH_KEY = "default"; export interface CachedView { + authKey: string; repo: string; kind: CacheKind; number: number; @@ -39,6 +43,7 @@ export interface CachedView { } interface Row { + auth_key: string; repo: string; kind: CacheKind; number: number; @@ -57,12 +62,30 @@ let openAttempted = false; function ensureParentDir(filePath: string): void { try { - fs.mkdirSync(path.dirname(filePath), { recursive: true }); + const dir = path.dirname(filePath); + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + fs.chmodSync(dir, 0o700); } catch (err) { - logger.debug("github cache: failed to create parent dir", { err: String(err) }); + logger.debug("github cache: failed to create private parent dir", { err: String(err) }); } } +function chmodIfExists(filePath: string, mode: number): void { + try { + fs.chmodSync(filePath, mode); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + logger.debug("github cache: chmod failed", { err: String(err), path: filePath }); + } + } +} + +function protectDbFiles(dbPath: string): void { + chmodIfExists(dbPath, 0o600); + chmodIfExists(`${dbPath}-wal`, 0o600); + chmodIfExists(`${dbPath}-shm`, 0o600); +} + export function openDb(): Database | null { if (cachedDb) return cachedDb; if (openAttempted) return null; @@ -75,20 +98,32 @@ export function openDb(): Database | null { PRAGMA journal_mode=WAL; PRAGMA synchronous=NORMAL; PRAGMA busy_timeout=5000; + `); + // Migrate any pre-existing table whose key/check constraint predates + // the current schema. The cache is regenerable, so we drop rows rather + // than running an in-place ALTER dance. + const userVersion = (db.prepare("PRAGMA user_version").get() as { user_version?: number } | undefined) + ?.user_version; + if (userVersion !== undefined && userVersion < 3) { + db.run("DROP TABLE IF EXISTS github_view_cache"); + } + db.run(` CREATE TABLE IF NOT EXISTS github_view_cache ( + auth_key TEXT NOT NULL, repo TEXT NOT NULL, - kind TEXT NOT NULL CHECK (kind IN ('issue','pr')), + kind TEXT NOT NULL CHECK (kind IN ('issue','pr','pr-diff')), number INTEGER NOT NULL, include_comments INTEGER NOT NULL, fetched_at INTEGER NOT NULL, payload TEXT NOT NULL, rendered TEXT NOT NULL, source_url TEXT, - PRIMARY KEY (repo, kind, number, include_comments) + PRIMARY KEY (auth_key, repo, kind, number, include_comments) ); CREATE INDEX IF NOT EXISTS idx_github_view_cache_fetched ON github_view_cache(fetched_at); - PRAGMA user_version = 1; + PRAGMA user_version = 3; `); + protectDbFiles(dbPath); cachedDb = db; // One-shot eviction on open. The default `DEFAULT_HARD_TTL_SEC` is a // coarse backstop only — when settings load with a stricter @@ -128,6 +163,51 @@ function sweepIfDue(hardTtlMs: number): void { evictExpired(db, hardTtlMs); } +function getGhConfigDir(): string { + const override = process.env.GH_CONFIG_DIR; + if (override) return override; + const xdg = process.env.XDG_CONFIG_HOME; + if (xdg) return path.join(xdg, "gh"); + return path.join(os.homedir(), ".config", "gh"); +} + +function hashCacheIdentity(parts: string[]): string { + return Bun.hash(parts.map(part => `${part.length}:${part}`).join("|")).toString(36); +} + +/** + * Best-effort local fingerprint for the active GitHub CLI credentials. + * + * Cache hits must not cross account/token boundaries, but doing a `gh api user` + * probe before every cached read would defeat the soft-TTL contract that cache + * hits avoid a gh round-trip. Instead, key rows by credential material that the + * GitHub CLI itself consumes: token environment variables and/or hosts.yml. + * The DB stores only a hash, never the token or hosts.yml contents. If no + * credential source is visible, callers should pass `null` to bypass caching. + */ +export function resolveGithubCacheAuthKey(host: string = process.env.GH_HOST || "github.com"): string | undefined { + const parts: string[] = [`host:${host}`]; + let hasCredentialMaterial = false; + for (const name of ["GH_TOKEN", "GITHUB_TOKEN", "GH_ENTERPRISE_TOKEN", "GITHUB_ENTERPRISE_TOKEN"]) { + const value = process.env[name]; + if (!value) continue; + hasCredentialMaterial = true; + parts.push(`${name}:${value}`); + } + try { + const hostsPath = path.join(getGhConfigDir(), "hosts.yml"); + const hosts = fs.readFileSync(hostsPath, "utf8"); + hasCredentialMaterial = true; + parts.push(`hosts:${hosts}`); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + logger.debug("github cache: failed to read gh hosts config for cache identity", { err: String(err) }); + } + } + if (!hasCredentialMaterial) return undefined; + return `${host}:${hashCacheIdentity(parts)}`; +} + function normalizeRepo(repo: string): string { return repo.toLowerCase(); } @@ -137,15 +217,16 @@ export function getCached( kind: CacheKind, number: number, includeComments: boolean, + authKey: string = DEFAULT_CACHE_AUTH_KEY, ): CachedView | null { const db = openDb(); if (!db) return null; try { const row = db .prepare( - "SELECT repo, kind, number, include_comments, fetched_at, payload, rendered, source_url FROM github_view_cache WHERE repo = ? AND kind = ? AND number = ? AND include_comments = ?", + "SELECT auth_key, repo, kind, number, include_comments, fetched_at, payload, rendered, source_url FROM github_view_cache WHERE auth_key = ? AND repo = ? AND kind = ? AND number = ? AND include_comments = ?", ) - .get(normalizeRepo(repo), kind, number, includeComments ? 1 : 0) as Row | undefined; + .get(authKey, normalizeRepo(repo), kind, number, includeComments ? 1 : 0) as Row | undefined; if (!row) return null; let payload: T; try { @@ -155,6 +236,7 @@ export function getCached( return null; } return { + authKey: row.auth_key, repo: row.repo, kind: row.kind, number: row.number, @@ -171,6 +253,7 @@ export function getCached( } export interface PutCachedInput { + authKey?: string; repo: string; kind: CacheKind; number: number; @@ -188,8 +271,9 @@ export function putCached(input: PutCachedInput): void { const fetchedAt = input.fetchedAt ?? Date.now(); const payloadJson = JSON.stringify(input.payload); db.prepare( - "INSERT OR REPLACE INTO github_view_cache (repo, kind, number, include_comments, fetched_at, payload, rendered, source_url) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + "INSERT OR REPLACE INTO github_view_cache (auth_key, repo, kind, number, include_comments, fetched_at, payload, rendered, source_url) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", ).run( + input.authKey ?? DEFAULT_CACHE_AUTH_KEY, normalizeRepo(input.repo), input.kind, input.number, @@ -199,26 +283,34 @@ export function putCached(input: PutCachedInput): void { input.rendered, input.sourceUrl ?? null, ); + protectDbFiles(getGithubCacheDbPath()); } catch (err) { logger.debug("github cache: write failed", { err: String(err) }); } } /** Drop a specific cache entry. */ -export function invalidate(repo: string, kind: CacheKind, number: number, includeComments?: boolean): void { +export function invalidate( + repo: string, + kind: CacheKind, + number: number, + includeComments?: boolean, + authKey: string = DEFAULT_CACHE_AUTH_KEY, +): void { const db = openDb(); if (!db) return; try { if (includeComments === undefined) { - db.prepare("DELETE FROM github_view_cache WHERE repo = ? AND kind = ? AND number = ?").run( + db.prepare("DELETE FROM github_view_cache WHERE auth_key = ? AND repo = ? AND kind = ? AND number = ?").run( + authKey, normalizeRepo(repo), kind, number, ); } else { db.prepare( - "DELETE FROM github_view_cache WHERE repo = ? AND kind = ? AND number = ? AND include_comments = ?", - ).run(normalizeRepo(repo), kind, number, includeComments ? 1 : 0); + "DELETE FROM github_view_cache WHERE auth_key = ? AND repo = ? AND kind = ? AND number = ? AND include_comments = ?", + ).run(authKey, normalizeRepo(repo), kind, number, includeComments ? 1 : 0); } } catch (err) { logger.debug("github cache: invalidate failed", { err: String(err) }); @@ -268,6 +360,12 @@ export interface CacheLookupOptions { kind: CacheKind; number: number; includeComments: boolean; + /** + * Auth/credential namespace for cache rows. Omit only in storage-layer + * tests; pass `null` when production code cannot determine an identity and + * must bypass persistent cache reads/writes. + */ + authKey?: string | null; fetchFresh: () => Promise>; settings?: Settings | undefined; now?: number; @@ -324,6 +422,7 @@ export function resolveCacheTtl(settings?: Settings): CacheTtl { } function storeResult( + authKey: string, repo: string, kind: CacheKind, number: number, @@ -332,6 +431,7 @@ function storeResult( fetchedAt: number, ): void { putCached({ + authKey, repo, kind, number, @@ -344,6 +444,7 @@ function storeResult( } function scheduleBackgroundRefresh( + authKey: string, repo: string, kind: CacheKind, number: number, @@ -354,7 +455,7 @@ function scheduleBackgroundRefresh( const promise = fetchFresh(); promise .then(fresh => { - storeResult(repo, kind, number, includeComments, fresh, Date.now()); + storeResult(authKey, repo, kind, number, includeComments, fresh, Date.now()); }) .catch(err => { logger.debug("github cache: background refresh failed", { @@ -370,8 +471,9 @@ function scheduleBackgroundRefresh( export async function getOrFetchView(options: CacheLookupOptions): Promise> { const ttl = resolveCacheTtl(options.settings); const now = options.now ?? Date.now(); + const authKey = options.authKey === undefined ? DEFAULT_CACHE_AUTH_KEY : options.authKey; - if (!ttl.enabled) { + if (!ttl.enabled || authKey === null) { const fresh = await options.fetchFresh(); return { ...fresh, status: "disabled", fetchedAt: now }; } @@ -386,11 +488,17 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise options.kind, options.number, options.includeComments, + authKey, ); if (cached) { const age = now - cached.fetchedAt; - if (age <= ttl.softMs) { + if (age > ttl.hardMs) { + // Past hard TTL: drop the row eagerly so the on-disk exposure window + // is bounded even if `fetchFresh()` then fails (network down, gh + // auth lapse, etc.) and we never get to overwrite it. + invalidate(options.repo, options.kind, options.number, options.includeComments, authKey); + } else if (age <= ttl.softMs) { return { rendered: cached.rendered, sourceUrl: cached.sourceUrl, @@ -398,9 +506,9 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise status: "fresh", fetchedAt: cached.fetchedAt, }; - } - if (age <= ttl.hardMs) { + } else { scheduleBackgroundRefresh( + authKey, options.repo, options.kind, options.number, @@ -415,15 +523,11 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise fetchedAt: cached.fetchedAt, }; } - // Past hard TTL: drop the row eagerly so the on-disk exposure window - // is bounded even if `fetchFresh()` then fails (network down, gh - // auth lapse, etc.) and we never get to overwrite it. - invalidate(options.repo, options.kind, options.number, options.includeComments); } const fresh = await options.fetchFresh(); const fetchedAt = Date.now(); - storeResult(options.repo, options.kind, options.number, options.includeComments, fresh, fetchedAt); + storeResult(authKey, options.repo, options.kind, options.number, options.includeComments, fresh, fetchedAt); return { ...fresh, status: "miss", fetchedAt }; } diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index eaaad6b93..4fd8f0848 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -26,7 +26,7 @@ import { truncateHead, truncateHeadBytes, } from "../session/streaming-output"; -import { renderCodeCell, renderStatusLine } from "../tui"; +import { renderCodeCell, renderMarkdownCell, renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; import { resolveFileDisplayMode } from "../utils/file-display-mode"; import { ImageInputTooLargeError, loadImageInput, MAX_IMAGE_INPUT_BYTES } from "../utils/image-loading"; @@ -1707,7 +1707,7 @@ export class ReadTool implements AgentTool { cwd: this.session.cwd, settings: this.session.settings, }); - const details: ReadToolDetails = { resolvedPath: resource.sourcePath }; + const details: ReadToolDetails = { resolvedPath: resource.sourcePath, contentType: resource.contentType }; // If extraction was used, return directly (no pagination) if (hasExtraction) { @@ -1807,7 +1807,7 @@ export const readToolRenderer = { renderResult( result: { content: Array<{ type: string; text?: string }>; details?: ReadToolDetails }, - _options: RenderResultOptions, + options: RenderResultOptions, uiTheme: Theme, args?: ReadRenderArgs, ): Component { @@ -1815,7 +1815,7 @@ export const readToolRenderer = { if (urlDetails?.kind === "url" || isReadableUrlPath(args?.file_path || args?.path || "")) { return renderReadUrlResult( result as { content: Array<{ type: string; text?: string }>; details?: ReadUrlToolDetails }, - _options, + options, uiTheme, ); } @@ -1896,28 +1896,45 @@ export const readToolRenderer = { const n = details.conflictCount; title += ` ${uiTheme.fg("warning", `(⚠ ${n} conflict${n === 1 ? "" : "s"})`)}`; } + const isMarkdown = details?.contentType === "text/markdown"; let cachedWidth: number | undefined; + let cachedExpanded: boolean | undefined; let cachedLines: string[] | undefined; return { render: (width: number) => { - if (cachedLines && cachedWidth === width) return cachedLines; - cachedLines = renderCodeCell( - { - code: contentText, - language: lang, - title, - status: "complete", - output: warningLines.length > 0 ? warningLines.join("\n") : undefined, - expanded: true, - width, - }, - uiTheme, - ); + const expanded = options.expanded; + if (cachedLines && cachedWidth === width && cachedExpanded === expanded) return cachedLines; + cachedLines = isMarkdown + ? renderMarkdownCell( + { + content: contentText, + title, + status: "complete", + output: warningLines.length > 0 ? warningLines.join("\n") : undefined, + expanded: true, + width, + }, + uiTheme, + ) + : renderCodeCell( + { + code: contentText, + language: lang, + title, + status: "complete", + output: warningLines.length > 0 ? warningLines.join("\n") : undefined, + expanded, + width, + }, + uiTheme, + ); cachedWidth = width; + cachedExpanded = expanded; return cachedLines; }, invalidate: () => { cachedWidth = undefined; + cachedExpanded = undefined; cachedLines = undefined; }, }; diff --git a/packages/coding-agent/src/tui/code-cell.ts b/packages/coding-agent/src/tui/code-cell.ts index 999566603..5d4791eca 100644 --- a/packages/coding-agent/src/tui/code-cell.ts +++ b/packages/coding-agent/src/tui/code-cell.ts @@ -1,7 +1,8 @@ /** - * Render a code cell with optional output section. + * Render a code or markdown cell with optional output section. */ -import { highlightCode, type Theme } from "../modes/theme/theme"; +import { Markdown } from "@oh-my-pi/pi-tui"; +import { getMarkdownTheme, highlightCode, type Theme } from "../modes/theme/theme"; import { formatDuration, formatExpandHint, @@ -116,3 +117,70 @@ export function renderCodeCell(options: CodeCellOptions, theme: Theme): string[] return renderOutputBlock({ header: title, headerMeta: meta, state, sections, width }, theme); } + +export interface MarkdownCellOptions { + content: string; + index?: number; + total?: number; + title?: string; + status?: "pending" | "running" | "warning" | "complete" | "error"; + spinnerFrame?: number; + duration?: number; + output?: string; + outputMaxLines?: number; + contentMaxLines?: number; + expanded?: boolean; + width: number; +} + +export function renderMarkdownCell(options: MarkdownCellOptions, theme: Theme): string[] { + const { content, output, expanded = false, outputMaxLines = 6, contentMaxLines = 12, width } = options; + const codeOptions: CodeCellOptions = { + code: "", + index: options.index, + total: options.total, + title: options.title, + status: options.status, + spinnerFrame: options.spinnerFrame, + duration: options.duration, + width, + }; + const { title, meta } = formatHeader(codeOptions, theme); + const state = getState(options.status); + + // Markdown component manages its own wrapping at the inner content width. + // `renderOutputBlock` adds a `│ ` prefix + `│` suffix → 3 visible columns. + const innerWidth = Math.max(20, width - 3); + const allLines = content.trim() ? new Markdown(content, 0, 0, getMarkdownTheme()).render(innerWidth) : []; + const maxContentLines = expanded ? allLines.length : Math.min(allLines.length, contentMaxLines); + const contentLines = allLines.slice(0, maxContentLines); + const hiddenContentLines = allLines.length - maxContentLines; + if (hiddenContentLines > 0) { + const hint = formatExpandHint(theme, expanded, hiddenContentLines > 0); + const moreLine = `${formatMoreItems(hiddenContentLines, "line")}${hint ? ` ${hint}` : ""}`; + contentLines.push(theme.fg("dim", moreLine)); + } + + const outputLines: string[] = []; + if (output?.trim()) { + const rawLines = output.split("\n"); + const maxLines = expanded ? rawLines.length : Math.min(rawLines.length, outputMaxLines); + const displayLines = rawLines + .slice(0, maxLines) + .map(line => (line.includes("\x1b[") ? replaceTabs(line) : theme.fg("toolOutput", replaceTabs(line)))); + outputLines.push(...displayLines); + const remaining = rawLines.length - maxLines; + if (remaining > 0) { + const hint = formatExpandHint(theme, expanded, remaining > 0); + const moreLine = `${formatMoreItems(remaining, "line")}${hint ? ` ${hint}` : ""}`; + outputLines.push(theme.fg("dim", moreLine)); + } + } + + const sections: Array<{ label?: string; lines: string[] }> = [{ lines: contentLines }]; + if (outputLines.length > 0) { + sections.push({ label: theme.fg("toolTitle", "Output"), lines: outputLines }); + } + + return renderOutputBlock({ header: title, headerMeta: meta, state, sections, width }, theme); +} 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 0d371a7e3..08826e5f1 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 @@ -16,10 +16,13 @@ import * as git from "@oh-my-pi/pi-coding-agent/utils/git"; let tempDir: string; let originalEnv: string | undefined; +let originalGhToken: string | undefined; beforeEach(async () => { tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "issue-pr-protocol-")); originalEnv = process.env.OMP_GITHUB_CACHE_DB; process.env.OMP_GITHUB_CACHE_DB = path.join(tempDir, "github-cache.db"); + originalGhToken = process.env.GH_TOKEN; + process.env.GH_TOKEN = "test-token"; resetCacheForTests(); InternalUrlRouter.resetForTests(); }); @@ -32,6 +35,11 @@ afterEach(async () => { } else { process.env.OMP_GITHUB_CACHE_DB = originalEnv; } + if (originalGhToken === undefined) { + delete process.env.GH_TOKEN; + } else { + process.env.GH_TOKEN = originalGhToken; + } vi.restoreAllMocks(); await fs.rm(tempDir, { recursive: true, force: true }); }); @@ -78,6 +86,40 @@ function prPayload(number: number, body: string) { }; } +interface DiffFileSpec { + name: string; + adds?: number; + dels?: number; + mode?: "modified" | "added" | "deleted"; + oldName?: string; + binary?: boolean; +} + +function makePrDiff(files: DiffFileSpec[]): string { + return files + .map(f => { + const oldPath = f.oldName ?? f.name; + const lines: string[] = [`diff --git a/${oldPath} b/${f.name}`]; + if (f.mode === "added") lines.push("new file mode 100644"); + if (f.mode === "deleted") lines.push("deleted file mode 100644"); + if (f.oldName) { + lines.push(`rename from ${oldPath}`, `rename to ${f.name}`); + } + lines.push("index 0000000..1111111 100644"); + lines.push(`--- a/${oldPath}`); + lines.push(`+++ b/${f.name}`); + if (f.binary) { + lines.push(`Binary files a/${oldPath} and b/${f.name} differ`); + } else { + lines.push("@@ -1,1 +1,1 @@"); + for (let i = 0; i < (f.dels ?? 0); i += 1) lines.push(`-old line ${i}`); + for (let i = 0; i < (f.adds ?? 0); i += 1) lines.push(`+new line ${i}`); + } + return lines.join("\n"); + }) + .join("\n"); +} + describe("issue:// protocol handler", () => { it("resolves issue://owner/repo/ through the shared cache", async () => { const spy = vi.spyOn(git.github, "json").mockResolvedValue(issuePayload(42, "issue body", ["c1"]) as never); @@ -140,6 +182,7 @@ describe("pr:// protocol handler", () => { expect(first.contentType).toBe("text/markdown"); expect(first.content).toContain("# Pull Request #77: PR #77"); expect(first.immutable).toBe(true); + expect(first.notes).toContain("Diff: pr://owner/example/77/diff"); // First call hits gh twice (view JSON + review-comments page). expect(spy).toHaveBeenCalledTimes(2); @@ -157,6 +200,90 @@ describe("pr:// protocol handler", () => { }); }); +describe("pr://.../diff family", () => { + const diffText = makePrDiff([ + { name: "src/one.ts", adds: 3, dels: 1 }, + { name: "src/two.ts", adds: 2, dels: 0, mode: "added" }, + ]); + + it("pr://owner/repo//diff lists files with per-file hint URLs", async () => { + const textSpy = vi.spyOn(git.github, "text").mockResolvedValue(diffText); + + const router = InternalUrlRouter.instance(); + const resource = await router.resolve("pr://owner/example/77/diff"); + + expect(resource.contentType).toBe("text/markdown"); + expect(resource.content).toContain("# Pull Request Diff: owner/example#77 (2 files)"); + expect(resource.content).toContain("1. src/one.ts +3 -1 [modified]"); + expect(resource.content).toContain("pr://owner/example/77/diff/1"); + expect(resource.content).toContain("2. src/two.ts +2 -0 [added]"); + expect(resource.content).toContain("pr://owner/example/77/diff/2"); + expect(resource.notes?.[0]).toBe("Fetched live"); + expect(textSpy).toHaveBeenCalledTimes(1); + }); + + it("pr://owner/repo//diff renders an empty-file body when the PR has no changes", async () => { + vi.spyOn(git.github, "text").mockResolvedValue(""); + + const router = InternalUrlRouter.instance(); + const resource = await router.resolve("pr://owner/example/77/diff"); + expect(resource.content).toContain("# Pull Request Diff: owner/example#77 (0 files)"); + expect(resource.content).toContain("_No file changes._"); + }); + + it("pr://owner/repo//diff/all returns the verbatim unified diff as text/plain", async () => { + vi.spyOn(git.github, "text").mockResolvedValue(diffText); + + const router = InternalUrlRouter.instance(); + const resource = await router.resolve("pr://owner/example/77/diff/all"); + expect(resource.contentType).toBe("text/plain"); + expect(resource.content).toBe(diffText); + }); + + it("pr://owner/repo//diff/ slices the i-th file (1-indexed) as text/plain", async () => { + vi.spyOn(git.github, "text").mockResolvedValue(diffText); + + const router = InternalUrlRouter.instance(); + const first = await router.resolve("pr://owner/example/77/diff/1"); + expect(first.contentType).toBe("text/plain"); + expect(first.content.startsWith("diff --git a/src/one.ts b/src/one.ts")).toBe(true); + expect(first.content).not.toContain("src/two.ts"); + expect(first.notes).toEqual( + expect.arrayContaining(["Showing file 1/2: src/one.ts", "Read all: pr://owner/example/77/diff/all"]), + ); + + const second = await router.resolve("pr://owner/example/77/diff/2"); + expect(second.content.startsWith("diff --git a/src/two.ts b/src/two.ts")).toBe(true); + expect(second.content).not.toContain("src/one.ts"); + }); + + it("rejects out-of-range and non-decimal diff indices with friendly errors", async () => { + vi.spyOn(git.github, "text").mockResolvedValue(diffText); + + const router = InternalUrlRouter.instance(); + await expect(router.resolve("pr://owner/example/77/diff/9")).rejects.toThrow(/out of range/); + await expect(router.resolve("pr://owner/example/77/diff/foo")).rejects.toThrow(/Invalid pr:\/\/ diff sub-path/); + }); + + it("shares one `gh pr diff` invocation across /diff, /diff/all, and /diff/ reads", async () => { + const textSpy = vi.spyOn(git.github, "text").mockResolvedValue(diffText); + + const router = InternalUrlRouter.instance(); + await router.resolve("pr://owner/example/77/diff"); + await router.resolve("pr://owner/example/77/diff/all"); + await router.resolve("pr://owner/example/77/diff/1"); + // One row services all three variants — `gh pr diff` runs once. + expect(textSpy).toHaveBeenCalledTimes(1); + }); +}); + +describe("issue://.../diff rejection", () => { + it("issue://owner/example/9/diff rejects with 'Invalid issue:// URL'", async () => { + const router = InternalUrlRouter.instance(); + await expect(router.resolve("issue://owner/example/9/diff")).rejects.toThrow(/Invalid issue:\/\/ URL/); + }); +}); + describe("issue:// / pr:// listing", () => { it("issue://owner/repo issues a live `gh issue list` and renders entries", async () => { const spy = vi.spyOn(git.github, "json").mockResolvedValue([ @@ -190,7 +317,7 @@ describe("issue:// / pr:// listing", () => { expect(resource.content).toContain("#1"); expect(resource.content).toContain("Hello"); expect(resource.content).toContain("labels: bug"); - expect(resource.content).toContain("issue://1"); + expect(resource.content).toContain("issue://owner/example/1"); expect(resource.notes?.[0]).toContain("Live listing for owner/example"); expect(spy).toHaveBeenCalledTimes(1); diff --git a/packages/coding-agent/test/tools/gh.test.ts b/packages/coding-agent/test/tools/gh.test.ts index 34153c026..3e22b7287 100644 --- a/packages/coding-agent/test/tools/gh.test.ts +++ b/packages/coding-agent/test/tools/gh.test.ts @@ -7,7 +7,6 @@ import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { buildSearchDateQualifier, GithubTool, parseSearchDateBound } from "@oh-my-pi/pi-coding-agent/tools/gh"; -import { wrapToolWithMetaNotice } from "@oh-my-pi/pi-coding-agent/tools/output-meta"; import * as git from "@oh-my-pi/pi-coding-agent/utils/git"; import { getAgentDir, setAgentDir } from "@oh-my-pi/pi-utils"; @@ -36,7 +35,7 @@ function createSession( }; } -function createToolContext(settings: Settings): AgentToolContext { +function _createToolContext(settings: Settings): AgentToolContext { return { sessionManager: SessionManager.inMemory(), settings, @@ -572,46 +571,6 @@ describe("github tool", () => { expect(reposArgs.some(arg => typeof arg === "string" && arg.includes("repo:ignored/value"))).toBe(false); }); - it("returns diff output under a stable heading without rewriting patch content", async () => { - vi.spyOn(git.github, "text").mockResolvedValue("diff --git a/Makefile b/Makefile\n+\tgo test ./... \n"); - - const tool = new GithubTool(createSession()); - const result = await tool.execute("pr-diff", { op: "pr_diff", pr: "7", repo: "owner/repo" }); - const text = result.content[0]?.type === "text" ? result.content[0].text : ""; - - expect(text).toContain("# Pull Request Diff"); - expect(text).toContain("diff --git a/Makefile b/Makefile"); - expect(text).toContain("+\tgo test ./... "); - expect(text).not.toContain("+ go test ./... "); - }); - - it("lets wrapped GitHub diff output spill to an artifact tail instead of head-truncating", async () => { - const diffOutput = Array.from({ length: 400 }, (_, index) => `diff line ${index + 1}`).join("\n"); - vi.spyOn(git.github, "text").mockResolvedValue(diffOutput); - - const settings = Settings.isolated({ - "github.enabled": true, - "tools.artifactSpillThreshold": 1, - "tools.artifactTailBytes": 1, - "tools.artifactTailLines": 20, - }); - const tool = wrapToolWithMetaNotice(new GithubTool(createSession("/tmp/test", settings))); - const result = await tool.execute( - "pr-diff", - { op: "pr_diff", pr: "7", repo: "owner/repo" }, - undefined, - undefined, - createToolContext(settings), - ); - const text = result.content[0]?.type === "text" ? result.content[0].text : ""; - - expect(text).toContain("diff line 400"); - expect(text).not.toContain("diff line 1"); - expect(text).toContain("Read artifact://"); - expect(text).not.toContain("Use offset="); - expect(result.details?.meta?.truncation?.direction).toBe("tail"); - }); - it("checks out a pull request into a worktree and configures contributor push metadata", async () => { const fixture = await createPrFixture(); const tempHome = await setupTempHome(); @@ -773,24 +732,6 @@ describe("github tool", () => { } }); - it("aggregates multiple pull request diffs when pr is an array", async () => { - vi.spyOn(git.github, "text") - .mockResolvedValueOnce("diff --git a/one.ts b/one.ts\n+content one\n") - .mockResolvedValueOnce("diff --git a/two.ts b/two.ts\n+content two\n"); - - const tool = new GithubTool(createSession()); - const result = await tool.execute("pr-diff", { op: "pr_diff", pr: ["10", "20"], repo: "owner/repo" }); - const text = result.content[0]?.type === "text" ? result.content[0].text : ""; - - expect(text).toContain("# 2 Pull Request Diffs"); - expect(text).toContain("## PR 10"); - expect(text).toContain("## PR 20"); - expect(text).toContain("content one"); - expect(text).toContain("content two"); - // Sections are separated by a horizontal rule. - expect(text.match(/\n---\n/g)?.length).toBe(1); - }); - it("rejects PR pushes from branches without checkout metadata", async () => { const fixture = await createPrFixture(); try { diff --git a/packages/coding-agent/test/tools/github-cache.test.ts b/packages/coding-agent/test/tools/github-cache.test.ts index 220bc9a9c..326dd5ffe 100644 --- a/packages/coding-agent/test/tools/github-cache.test.ts +++ b/packages/coding-agent/test/tools/github-cache.test.ts @@ -22,6 +22,7 @@ import { import * as git from "@oh-my-pi/pi-coding-agent/utils/git"; const TEST_REPO = "owner/example"; +const TEST_AUTH_KEY = "test-auth"; let tempDir: string; let originalEnv: string | undefined; @@ -137,6 +138,32 @@ describe("github-cache db layer", () => { expect(noComments?.rendered).toBe("no-comments-rendering"); }); + it("keys rows by GitHub auth identity", () => { + putCached({ + authKey: "identity-a", + repo: TEST_REPO, + kind: "issue", + number: 12, + includeComments: true, + payload: issuePayload(12, "a"), + rendered: "from-a", + fetchedAt: 1000, + }); + putCached({ + authKey: "identity-b", + repo: TEST_REPO, + kind: "issue", + number: 12, + includeComments: true, + payload: issuePayload(12, "b"), + rendered: "from-b", + fetchedAt: 1000, + }); + + expect(getCached(TEST_REPO, "issue", 12, true, "identity-a")?.rendered).toBe("from-a"); + expect(getCached(TEST_REPO, "issue", 12, true, "identity-b")?.rendered).toBe("from-b"); + }); + it("clearAll wipes every row but the schema survives", () => { putCached({ repo: TEST_REPO, @@ -256,6 +283,40 @@ describe("getOrFetchView (TTL semantics)", () => { expect(fetchFresh).toHaveBeenCalledTimes(1); }); + it("does not return soft-fresh rows past a shorter hard TTL", async () => { + const settings = Settings.isolated({ + "github.cache.softTtlSec": 300, + "github.cache.hardTtlSec": 10, + }); + const fetchFresh = vi.fn(async () => ({ + rendered: "fresh-after-hard-expiry", + sourceUrl: undefined, + payload: { number: 88 }, + })); + putCached({ + repo: TEST_REPO, + kind: "issue", + number: 88, + includeComments: true, + payload: { number: 88 }, + rendered: "hard-expired", + fetchedAt: Date.now() - 20_000, + }); + + const result = await getOrFetchView({ + repo: TEST_REPO, + kind: "issue", + number: 88, + includeComments: true, + fetchFresh, + settings, + }); + + expect(result.status).toBe("miss"); + expect(result.rendered).toBe("fresh-after-hard-expiry"); + expect(fetchFresh).toHaveBeenCalledTimes(1); + }); + it("bypasses the cache entirely when github.cache.enabled = false", async () => { const settings = Settings.isolated({ "github.cache.enabled": false }); const fetchFresh = vi.fn(async () => ({ @@ -296,6 +357,7 @@ describe("getOrFetchIssue (gh-wired wrapper)", () => { repo: TEST_REPO, issue: "123", includeComments: true, + cacheAuthKey: TEST_AUTH_KEY, }); expect(first.status).toBe("miss"); expect(spy).toHaveBeenCalledTimes(1); @@ -305,6 +367,7 @@ describe("getOrFetchIssue (gh-wired wrapper)", () => { repo: TEST_REPO, issue: "123", includeComments: true, + cacheAuthKey: TEST_AUTH_KEY, }); expect(second.status).toBe("fresh"); expect(second.rendered).toBe(first.rendered); @@ -315,12 +378,12 @@ describe("getOrFetchIssue (gh-wired wrapper)", () => { const spy = vi.spyOn(git.github, "json").mockResolvedValue(issuePayload(7, "from-url") as never); const url = `https://github.com/${TEST_REPO}/issues/7`; - await getOrFetchIssue({ cwd: "/tmp/test", issue: url }); + await getOrFetchIssue({ cwd: "/tmp/test", issue: url, cacheAuthKey: TEST_AUTH_KEY }); // Second hit by plain number + explicit repo must read the same row. - await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "7" }); + await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "7", cacheAuthKey: TEST_AUTH_KEY }); expect(spy).toHaveBeenCalledTimes(1); - const cached = getCached(TEST_REPO, "issue", 7, true); + const cached = getCached(TEST_REPO, "issue", 7, true, TEST_AUTH_KEY); expect(cached).not.toBeNull(); expect(cached?.rendered).toContain("Issue #7"); }); @@ -328,14 +391,38 @@ describe("getOrFetchIssue (gh-wired wrapper)", () => { it("caches comments-on and comments-off separately", async () => { const spy = vi.spyOn(git.github, "json").mockResolvedValue(issuePayload(5, "no-comments-body") as never); - await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "5", includeComments: false }); - await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "5", includeComments: true }); + await getOrFetchIssue({ + cwd: "/tmp/test", + repo: TEST_REPO, + issue: "5", + includeComments: false, + cacheAuthKey: TEST_AUTH_KEY, + }); + await getOrFetchIssue({ + cwd: "/tmp/test", + repo: TEST_REPO, + issue: "5", + includeComments: true, + cacheAuthKey: TEST_AUTH_KEY, + }); // Different keys → two underlying fetches. expect(spy).toHaveBeenCalledTimes(2); // Each subsequent same-key call hits the cache. - await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "5", includeComments: false }); - await getOrFetchIssue({ cwd: "/tmp/test", repo: TEST_REPO, issue: "5", includeComments: true }); + await getOrFetchIssue({ + cwd: "/tmp/test", + repo: TEST_REPO, + issue: "5", + includeComments: false, + cacheAuthKey: TEST_AUTH_KEY, + }); + await getOrFetchIssue({ + cwd: "/tmp/test", + repo: TEST_REPO, + issue: "5", + includeComments: true, + cacheAuthKey: TEST_AUTH_KEY, + }); expect(spy).toHaveBeenCalledTimes(2); }); }); @@ -349,6 +436,7 @@ describe("getOrFetchPr (gh-wired wrapper)", () => { repo: TEST_REPO, number: 77, includeComments: false, + cacheAuthKey: TEST_AUTH_KEY, }); expect(first.status).toBe("miss"); expect(spy).toHaveBeenCalledTimes(1); @@ -358,6 +446,7 @@ describe("getOrFetchPr (gh-wired wrapper)", () => { repo: TEST_REPO, number: 77, includeComments: false, + cacheAuthKey: TEST_AUTH_KEY, }); expect(second.status).toBe("fresh"); expect(second.rendered).toBe(first.rendered);