mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 06:10:44 +01:00
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Skills can come from a tracked GitHub source, and an update moves
the installed copy to a newer upstream revision
> - Before an update, a person asks the agent "what changed in the new
version?"
> - The product has no diff for a pending update. The agent previews two
full SKILL.md snapshots and compares them by eye
> - In two eval runs the agent reversed the same changed line. In a
third run it fetched the files with curl and ran `diff` itself, and the
turn took too long
> - This pull request adds a read-only endpoint that returns the line
diff between the installed and the latest tracked revision
> - The benefit is that review answers come from an exact diff, and they
come faster
## Linked Issues or Issue Description
No public issue exists. Feature description:
**What happened**
An agent asked to show what changed in a pending skill update had only
`POST /skill-sources/preview`, which returns one file at one commit. It
previewed both commits and compared about 9 KB of text by eye. It
reported one changed line in the wrong direction ("every new build" ->
"every build or release" instead of the reverse), and later retracted a
real change.
**Expected behavior**
The agent can request the exact changes of a pending tracked-source
update and report them with the correct direction.
**Steps to reproduce**
1. Install a skill from a GitHub source at an old commit and set the
tracking reference to a newer commit (automatic updates off).
2. Ask the chat agent: "Show me what changed in the new accessibility
skill version before we update it."
3. See that the agent can only preview full files at each commit and
must compare them itself.
## What Changed
- Add `GET /api/companies/{companyId}/skills/{skillId}/update-diff`. For
a skill from a tracked GitHub source, it compares the installed source
revision (`currentRef`) with the latest tracked revision (`latestRef`).
For each changed package file it returns `change` (added, removed,
modified, or `not_compared`), `binary`, `executableChanged`, and a
unified line diff (`-` installed, `+` latest), or `diffUnavailable`
(`binary`, `too_large`, `omitted`, `budget`) that says why there is no
diff. The endpoint is read-only. The `skillId` parameter handler checks
company access and skill visibility, the same checks other skill reads
use.
- `hasUpdate` is false and `files` is empty when the installed revision
is already the latest one. That case reads only the latest commit and
scans nothing.
- Return `supported: false` with a reason for: a skill without a tracked
source, a disconnected source, an unknown installed revision, and a
repository package that a scan could include only in part (a subset, or
skill folders only). In the last case, files that a scan limit left out
would otherwise look removed.
- A path the scan left out of a revision (too large, a symlink, or
rejected by the audit) is listed as `not_compared` instead of added or
removed.
- One response shares a work budget (24M LCS cells), a diff-text budget
(1M characters) and a file-count budget (1,000 files), and sets
`truncated` when it reaches one. The GitHub read and both scans stop
when the caller disconnects.
- Add `server/src/services/skill-update-diff.ts` with a bounded line
diff (shared head and tail trimmed, LCS size capped). An absent side has
no lines, a final newline is not a line, and a missing final newline
shows the unified-diff marker. A replaced line reads `-old` then `+new`.
- Add the shared types `SkillSourceUpdateDiff` and
`SkillSourceUpdateDiffFile`.
- Document the endpoint in the API catalog (`openapi.ts`) so agents find
it with `search_api`.
## Verification
- `cd server && npx vitest run src/__tests__/skill-update-diff.test.ts`
(10 tests):
- direction of `-`/`+` lines, and separate hunks with bounded context
- exact diffs for added and removed files, and final-newline handling
- a change to only the executable bit, and paths left out of a scan
- the response budgets
- the snapshot comparison for the selected skill path, with the abort
signal passed to both scans
- a commit mismatch
- refusal of partial repository scans
- `cd server && npx vitest run
src/__tests__/skill-update-diff-routes.test.ts` (6 tests):
- the diff response
- no pending update (no scan)
- a caller from another company (board user and agent) gets 403 before
any lookup
- a skill the caller cannot read gets 404 before any source read
- every unsupported case
- a client disconnect aborts the scans
- `company-skills-routes.test.ts` and `openapi-routes.test.ts` pass (119
tests across the four files).
- `npx tsc --noEmit -p server`: no errors from this change.
- End to end: the `skills.review-update` chat eval exercises this path.
Its grader checks the quoted hunks against the real pinned diff.
## Risks
- Low risk. The endpoint is new and read-only. Existing routes do not
change.
- When an update is pending, each call scans the source twice (installed
and latest commit). It uses the same GitHub reader, scan limits and scan
slots as preview and refresh. The diff work and response size are capped
per response.
- `/update-status` and this route both read the latest commit of the
tracking ref. They are kept separate because `/update-status` compares
with the source's last scan, while this route compares with the
installed revision.
- No UI uses the endpoint yet.
## Model Used
- Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code
execution in Claude Code.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
171 lines
10 KiB
TypeScript
171 lines
10 KiB
TypeScript
import path from 'node:path';
|
||
import type { CompanySkillVersionFileInventoryEntry, SkillSourceUpdateDiffFile, SkillSourcePackageMode } from '@paperclipai/shared';
|
||
import { unprocessable } from '../errors.js';
|
||
import { scanGitHubSkillsWithPackageFallback, type GitHubRead, type ScannedSkillSource } from './github-skill-source.js';
|
||
|
||
// Bounded LCS work for one file, after trimming the shared head and tail of the two files.
|
||
const MAX_DIFF_CELLS = 4_000_000;
|
||
// One response shares these budgets across all of its files, so a large update
|
||
// cannot hold the event loop or produce an unbounded response body.
|
||
export const MAX_UPDATE_DIFF_CELLS = 24_000_000;
|
||
export const MAX_UPDATE_DIFF_CHARS = 1_000_000;
|
||
export const MAX_UPDATE_DIFF_FILES = 1_000;
|
||
const CONTEXT_LINES = 3;
|
||
const NO_FINAL_NEWLINE = '\n\\ No newline at end of file';
|
||
|
||
type Row = { kind: ' ' | '-' | '+'; text: string; oldLine: number; newLine: number };
|
||
type LineDiff = { diff: string | null; cells: number };
|
||
|
||
/** The lines of a file. An empty file has none; a last line without a final newline carries the unified-diff marker. */
|
||
function fileLines(text: string): string[] {
|
||
if (text === '') return [];
|
||
const lines = text.split('\n');
|
||
if (lines.at(-1) === '') lines.pop();
|
||
else lines[lines.length - 1] += NO_FINAL_NEWLINE;
|
||
return lines;
|
||
}
|
||
|
||
function lineDiff(before: string, after: string, maxCells: number): LineDiff {
|
||
if (before === after) return { diff: '', cells: 0 };
|
||
const a = fileLines(before), b = fileLines(after);
|
||
let head = 0;
|
||
while (head < a.length && head < b.length && a[head] === b[head]) head++;
|
||
let tail = 0;
|
||
while (tail < a.length - head && tail < b.length - head && a[a.length - 1 - tail] === b[b.length - 1 - tail]) tail++;
|
||
const midA = a.slice(head, a.length - tail), midB = b.slice(head, b.length - tail);
|
||
const cells = (midA.length + 1) * (midB.length + 1);
|
||
if (cells > maxCells) return { diff: null, cells };
|
||
const lcs = Array.from({ length: midA.length + 1 }, () => new Uint32Array(midB.length + 1));
|
||
for (let i = midA.length - 1; i >= 0; i--) for (let j = midB.length - 1; j >= 0; j--)
|
||
lcs[i]![j] = midA[i] === midB[j] ? lcs[i + 1]![j + 1]! + 1 : Math.max(lcs[i + 1]![j]!, lcs[i]![j + 1]!);
|
||
const rows: Row[] = [];
|
||
for (let k = 0; k < head; k++) rows.push({ kind: ' ', text: a[k]!, oldLine: k + 1, newLine: k + 1 });
|
||
let i = 0, j = 0;
|
||
while (i < midA.length || j < midB.length) {
|
||
if (i < midA.length && j < midB.length && midA[i] === midB[j]) { rows.push({ kind: ' ', text: midA[i]!, oldLine: head + i + 1, newLine: head + j + 1 }); i++; j++; }
|
||
// On a tie the installed line goes first, so a replaced line reads "-old" then "+new".
|
||
else if (i < midA.length && (j >= midB.length || lcs[i + 1]![j]! >= lcs[i]![j + 1]!)) { rows.push({ kind: '-', text: midA[i]!, oldLine: head + i + 1, newLine: head + j }); i++; }
|
||
else { rows.push({ kind: '+', text: midB[j]!, oldLine: head + i, newLine: head + j + 1 }); j++; }
|
||
}
|
||
for (let k = 0; k < tail; k++) rows.push({ kind: ' ', text: a[a.length - tail + k]!, oldLine: a.length - tail + k + 1, newLine: b.length - tail + k + 1 });
|
||
const changed = rows.map((row, index) => row.kind === ' ' ? -1 : index).filter(index => index >= 0);
|
||
const hunks: Array<[number, number]> = [];
|
||
for (const index of changed) {
|
||
const start = Math.max(0, index - CONTEXT_LINES), end = Math.min(rows.length - 1, index + CONTEXT_LINES);
|
||
const last = hunks.at(-1);
|
||
if (last && start <= last[1] + 1) last[1] = Math.max(last[1], end); else hunks.push([start, end]);
|
||
}
|
||
const diff = hunks.map(([start, end]) => {
|
||
const slice = rows.slice(start, end + 1);
|
||
const oldCount = slice.filter(row => row.kind !== '+').length, newCount = slice.filter(row => row.kind !== '-').length;
|
||
// As in unified diff, a side with no lines starts at the line before the hunk (0 for an empty file).
|
||
const oldStart = oldCount ? slice.find(row => row.kind !== '+')!.oldLine : slice[0]!.oldLine;
|
||
const newStart = newCount ? slice.find(row => row.kind !== '-')!.newLine : slice[0]!.newLine;
|
||
return [`@@ -${oldStart},${oldCount} +${newStart},${newCount} @@`, ...slice.map(row => `${row.kind}${row.text}`)].join('\n');
|
||
}).join('\n');
|
||
return { diff, cells };
|
||
}
|
||
|
||
/** Unified line diff from `before` (installed) to `after` (latest); "" when equal, null when too large. */
|
||
export function unifiedLineDiff(before: string, after: string): string | null {
|
||
return lineDiff(before, after, MAX_DIFF_CELLS).diff;
|
||
}
|
||
|
||
type DiffFile = Pick<CompanySkillVersionFileInventoryEntry, 'path' | 'content' | 'encoding' | 'executable'>;
|
||
/** Paths that a source scan left out of each snapshot (too large, a symlink, or rejected by the audit). */
|
||
export type OmittedPaths = { installed: ReadonlySet<string>; latest: ReadonlySet<string> };
|
||
export type SkillPackageDiff = { files: SkillSourceUpdateDiffFile[]; truncated: boolean };
|
||
|
||
/** Per-file changes between two snapshots of one skill package, sorted by path. */
|
||
export function diffSkillPackageFiles(installed: DiffFile[], latest: DiffFile[], omitted: OmittedPaths = { installed: new Set(), latest: new Set() }): SkillPackageDiff {
|
||
const before = new Map(installed.map(file => [file.path, file])), after = new Map(latest.map(file => [file.path, file]));
|
||
const text = (file: DiffFile) => file.encoding !== 'base64' ? file.content : null;
|
||
const paths = [...new Set([...before.keys(), ...after.keys(), ...omitted.installed, ...omitted.latest])].sort();
|
||
const files: SkillSourceUpdateDiffFile[] = [];
|
||
let cells = 0, chars = 0, truncated = false;
|
||
for (const filePath of paths) {
|
||
const old = before.get(filePath), next = after.get(filePath);
|
||
if (omitted.installed.has(filePath) || omitted.latest.has(filePath)) {
|
||
// Without both snapshots of this path, any added/removed/modified claim would be a guess.
|
||
files.push({ path: filePath, change: 'not_compared', binary: false, executableChanged: false, diff: null, diffUnavailable: 'omitted' });
|
||
} else {
|
||
const sameContent = !!old && !!next && old.content === next.content && (old.encoding ?? 'utf8') === (next.encoding ?? 'utf8');
|
||
const executableChanged = !!old && !!next && !!old.executable !== !!next.executable;
|
||
if (sameContent && !executableChanged) continue;
|
||
const change = !old ? 'added' as const : !next ? 'removed' as const : 'modified' as const;
|
||
const oldText = old ? text(old) : '', newText = next ? text(next) : '';
|
||
const binary = oldText === null || newText === null;
|
||
let diff: string | null = null;
|
||
let diffUnavailable: SkillSourceUpdateDiffFile['diffUnavailable'] = binary ? 'binary' : null;
|
||
if (!binary) {
|
||
const result = lineDiff(oldText, newText, Math.min(MAX_DIFF_CELLS, MAX_UPDATE_DIFF_CELLS - cells));
|
||
if (result.diff === null) {
|
||
diffUnavailable = result.cells > MAX_DIFF_CELLS ? 'too_large' : 'budget';
|
||
truncated ||= diffUnavailable === 'budget';
|
||
} else if (chars + result.diff.length > MAX_UPDATE_DIFF_CHARS) {
|
||
cells += result.cells;
|
||
diffUnavailable = 'budget';
|
||
truncated = true;
|
||
} else {
|
||
cells += result.cells;
|
||
chars += result.diff.length;
|
||
diff = result.diff;
|
||
}
|
||
}
|
||
files.push({ path: filePath, change, binary, executableChanged, diff, diffUnavailable });
|
||
}
|
||
if (files.length >= MAX_UPDATE_DIFF_FILES && filePath !== paths.at(-1)) { truncated = true; break; }
|
||
}
|
||
return { files, truncated };
|
||
}
|
||
|
||
const OMITTED_FILE_WARNING = /^(.+?) — .*(?:not included|couldn’t be included)\.?$/;
|
||
function omittedPaths(warnings: Iterable<string>, toPackagePath: (repositoryPath: string) => string | null) {
|
||
const paths = new Set<string>();
|
||
for (const warning of warnings) {
|
||
const match = OMITTED_FILE_WARNING.exec(warning);
|
||
const packagePath = match ? toPackagePath(match[1]!) : null;
|
||
if (packagePath) paths.add(packagePath);
|
||
}
|
||
return paths;
|
||
}
|
||
|
||
type Snapshot = { files: DiffFile[]; omitted: Set<string> } | { partialReason: string };
|
||
function packageSnapshot(scan: ScannedSkillSource, input: { packageMode?: SkillSourcePackageMode; skillPath: string }): Snapshot {
|
||
if (input.packageMode === 'repository') {
|
||
// A repository that outgrew the package limits is scanned as a subset or as
|
||
// skill folders only; files missing from such a scan may still exist upstream.
|
||
if (scan.packageMode !== 'repository' || scan.repositorySubset || !scan.repositoryFiles) {
|
||
return { partialReason: 'The repository is larger than the supported package size, so its revisions cannot be compared file by file.' };
|
||
}
|
||
return { files: scan.repositoryFiles, omitted: omittedPaths([...scan.warnings, ...(scan.repositoryPackage?.warnings ?? [])], repositoryPath => repositoryPath) };
|
||
}
|
||
const skill = scan.skills.find(candidate => candidate.path === input.skillPath);
|
||
const dir = path.posix.dirname(input.skillPath);
|
||
const prefix = dir === '.' ? '' : `${dir}/`;
|
||
const toPackagePath = (repositoryPath: string) => repositoryPath === input.skillPath ? 'SKILL.md'
|
||
: repositoryPath.startsWith(prefix) ? repositoryPath.slice(prefix.length) : null;
|
||
return { files: skill?.files ?? [], omitted: omittedPaths(skill?.warnings ?? [], toPackagePath) };
|
||
}
|
||
|
||
export type GitHubSkillUpdateDiff = ({ supported: true } & SkillPackageDiff) | { supported: false; reason: string };
|
||
|
||
export async function diffGitHubSkillUpdate(
|
||
input: { repositoryUrl: string; packageMode?: SkillSourcePackageMode; skillPath: string; installedCommitSha: string; latestCommitSha: string },
|
||
read: GitHubRead,
|
||
signal?: AbortSignal,
|
||
): Promise<GitHubSkillUpdateDiff> {
|
||
const snapshotAt = async (commitSha: string) => {
|
||
signal?.throwIfAborted();
|
||
const scan = await scanGitHubSkillsWithPackageFallback({ repositoryUrl: input.repositoryUrl, packageMode: input.packageMode, commitSha, onlySkillPath: input.skillPath }, read, { signal });
|
||
if (scan.commitSha.toLowerCase() !== commitSha.toLowerCase()) throw unprocessable('The source snapshot did not match the requested revision.');
|
||
return packageSnapshot(scan, input);
|
||
};
|
||
const installed = await snapshotAt(input.installedCommitSha);
|
||
if ('partialReason' in installed) return { supported: false, reason: installed.partialReason };
|
||
const latest = await snapshotAt(input.latestCommitSha);
|
||
if ('partialReason' in latest) return { supported: false, reason: latest.partialReason };
|
||
signal?.throwIfAborted();
|
||
return { supported: true, ...diffSkillPackageFiles(installed.files, latest.files, { installed: installed.omitted, latest: latest.omitted }) };
|
||
}
|