mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
feat(skills): show a line diff for a pending tracked-source update (#20155)
## 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>
This commit is contained in:
1 parent
4afdf0493d
commit
28e070f8dc
6 files changed
+492
-1
No files matched your search
@@ -43,6 +43,30 @@ export interface SkillSourceFilePreview {
|
||||
truncated: boolean;
|
||||
commitSha: string;
|
||||
}
|
||||
/** One file that differs between the installed and latest tracked source revisions. */
|
||||
export interface SkillSourceUpdateDiffFile {
|
||||
path: string;
|
||||
/** `not_compared`: the scan left this path out of at least one revision (too large, a symlink, or rejected by the audit). */
|
||||
change: 'added' | 'removed' | 'modified' | 'not_compared';
|
||||
binary: boolean;
|
||||
/** The file's executable bit differs between the two revisions. */
|
||||
executableChanged: boolean;
|
||||
/** Unified line diff ("-" installed, "+" latest); "" when only the executable bit changed; null when unavailable. */
|
||||
diff: string | null;
|
||||
/** Why `diff` is null: binary content, a file too large to diff, a path left out of a snapshot, or the response budget ran out. */
|
||||
diffUnavailable: 'binary' | 'too_large' | 'omitted' | 'budget' | null;
|
||||
}
|
||||
export interface SkillSourceUpdateDiff {
|
||||
supported: boolean;
|
||||
reason: string | null;
|
||||
currentRef: string | null;
|
||||
latestRef: string | null;
|
||||
/** False when the installed revision is already the latest one. */
|
||||
hasUpdate: boolean;
|
||||
files: SkillSourceUpdateDiffFile[];
|
||||
/** Some files were not listed or carry no diff because the response reached its size or work budget. */
|
||||
truncated: boolean;
|
||||
}
|
||||
export interface SkillSourceEntry {
|
||||
id: string;
|
||||
sourceId: string;
|
||||
|
||||
@@ -0,0 +1,126 @@
|
||||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { hoistModuleGraph } from "./helpers/hoist-module-graph.js";
|
||||
|
||||
const INSTALLED = "a".repeat(40);
|
||||
const LATEST = "b".repeat(40);
|
||||
const mockSkills = vi.hoisted(() => ({ getById: vi.fn(), getByRouteRef: vi.fn() }));
|
||||
const mockSources = vi.hoisted(() => ({ sourceForSkill: vi.fn() }));
|
||||
const mockRead = vi.hoisted(() => vi.fn());
|
||||
const mockDiff = vi.hoisted(() => vi.fn());
|
||||
|
||||
function registerModuleMocks() {
|
||||
vi.doMock("../services/skill-sources.js", () => ({ skillSourceService: () => mockSources }));
|
||||
vi.doMock("../services/skill-source-github-access.js", () => ({ skillSourceGitHubReader: () => mockRead }));
|
||||
vi.doMock("../services/skill-update-diff.js", () => ({ diffGitHubSkillUpdate: mockDiff }));
|
||||
vi.doMock("../services/company-skills.js", async () => ({
|
||||
...await vi.importActual<typeof import("../services/company-skills.js")>("../services/company-skills.js"),
|
||||
companySkillService: () => mockSkills,
|
||||
}));
|
||||
vi.doMock("../services/index.js", async () => ({
|
||||
...await vi.importActual<typeof import("../services/index.js")>("../services/index.js"),
|
||||
companySkillService: () => mockSkills,
|
||||
}));
|
||||
}
|
||||
|
||||
const board = { type: "board", userId: "user-1", companyIds: ["company-1"], source: "session", isInstanceAdmin: false };
|
||||
const trackedSource = {
|
||||
enabled: true, connectionId: null, fullName: "example/skills", trackingRef: "main", repositoryUrl: "https://github.com/example/skills",
|
||||
packageMode: "skills", entries: [{ skillId: "skill-1", selection: "selected", path: "design/SKILL.md" }],
|
||||
};
|
||||
|
||||
describe("GET /companies/:companyId/skills/:skillId/update-diff", () => {
|
||||
const routeModules = hoistModuleGraph(registerModuleMocks, async () => {
|
||||
const [{ companySkillRoutes }, { errorHandler }] = await Promise.all([
|
||||
vi.importActual<typeof import("../routes/company-skills.js")>("../routes/company-skills.js"),
|
||||
vi.importActual<typeof import("../middleware/index.js")>("../middleware/index.js"),
|
||||
]);
|
||||
return { companySkillRoutes, errorHandler };
|
||||
});
|
||||
|
||||
function app(actor: Record<string, unknown> = board) {
|
||||
const { companySkillRoutes, errorHandler } = routeModules.value;
|
||||
const server = express();
|
||||
server.use((req, _res, next) => { (req as any).actor = actor; next(); });
|
||||
server.use("/api", companySkillRoutes({} as never));
|
||||
server.use(errorHandler);
|
||||
return server;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
// The real lookup applies the caller's skill visibility; a skill the caller cannot read is not found.
|
||||
mockSkills.getByRouteRef.mockImplementation(async (_companyId: string, ref: string) => ref === "skill-1" ? { id: "skill-1", sourceRef: INSTALLED } : null);
|
||||
mockSkills.getById.mockImplementation(async (_companyId: string, id: string) => id === "skill-1" ? { id: "skill-1", sourceRef: INSTALLED } : null);
|
||||
mockSources.sourceForSkill.mockResolvedValue(trackedSource);
|
||||
mockRead.mockResolvedValue({ sha: LATEST });
|
||||
mockDiff.mockResolvedValue({ supported: true, truncated: false, files: [{ path: "SKILL.md", change: "modified", binary: false, executableChanged: false, diff: "@@ -1,1 +1,1 @@\n-a\n+b", diffUnavailable: null }] });
|
||||
});
|
||||
|
||||
it("returns the per-file diff between the installed and latest revisions", async () => {
|
||||
const response = await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff");
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toMatchObject({ supported: true, hasUpdate: true, currentRef: INSTALLED, latestRef: LATEST, truncated: false });
|
||||
expect(response.body.files).toHaveLength(1);
|
||||
expect(mockDiff).toHaveBeenCalledWith(expect.objectContaining({ installedCommitSha: INSTALLED, latestCommitSha: LATEST, skillPath: "design/SKILL.md" }), mockRead, expect.any(AbortSignal));
|
||||
});
|
||||
|
||||
it("answers a skill with no pending update without scanning either revision", async () => {
|
||||
mockRead.mockResolvedValue({ sha: INSTALLED.toUpperCase() });
|
||||
const response = await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff");
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual({ supported: true, reason: null, currentRef: INSTALLED, latestRef: INSTALLED.toUpperCase(), hasUpdate: false, files: [], truncated: false });
|
||||
expect(mockDiff).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects a caller from another company before reading anything", async () => {
|
||||
const response = await request(app({ ...board, companyIds: ["company-2"] })).get("/api/companies/company-1/skills/skill-1/update-diff");
|
||||
expect(response.status).toBe(403);
|
||||
expect(mockSkills.getByRouteRef).not.toHaveBeenCalled();
|
||||
expect(mockRead).not.toHaveBeenCalled();
|
||||
const agent = await request(app({ type: "agent", agentId: "agent-1", companyId: "company-2", runId: "run-1" })).get("/api/companies/company-1/skills/skill-1/update-diff");
|
||||
expect(agent.status).toBe(403);
|
||||
});
|
||||
|
||||
it("returns 404 for a skill the caller cannot read", async () => {
|
||||
const response = await request(app()).get("/api/companies/company-1/skills/private-skill/update-diff");
|
||||
expect(response.status).toBe(404);
|
||||
expect(mockSources.sourceForSkill).not.toHaveBeenCalled();
|
||||
expect(mockRead).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("explains why a skill without a usable tracked source has no diff", async () => {
|
||||
mockSources.sourceForSkill.mockResolvedValueOnce(null);
|
||||
expect((await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff")).body)
|
||||
.toMatchObject({ supported: false, reason: "Only skills installed from a tracked GitHub source have an update diff.", files: [] });
|
||||
mockSources.sourceForSkill.mockResolvedValueOnce({ ...trackedSource, enabled: false });
|
||||
expect((await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff")).body)
|
||||
.toMatchObject({ supported: false, reason: "Source is disconnected. Reconnect it in Skills → Sources." });
|
||||
mockSkills.getById.mockResolvedValueOnce({ id: "skill-1", sourceRef: "main" });
|
||||
expect((await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff")).body)
|
||||
.toMatchObject({ supported: false, reason: "The installed source revision is unknown.", currentRef: "main" });
|
||||
mockDiff.mockResolvedValueOnce({ supported: false, reason: "The repository is larger than the supported package size, so its revisions cannot be compared file by file." });
|
||||
expect((await request(app()).get("/api/companies/company-1/skills/skill-1/update-diff")).body)
|
||||
.toMatchObject({ supported: false, hasUpdate: true, latestRef: LATEST, files: [] });
|
||||
expect(mockDiff).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("aborts the scans when the caller disconnects", async () => {
|
||||
let seen: AbortSignal | undefined;
|
||||
mockDiff.mockImplementationOnce((_input: unknown, _read: unknown, signal: AbortSignal) => new Promise((_resolve, reject) => {
|
||||
seen = signal;
|
||||
signal.addEventListener("abort", () => reject(signal.reason), { once: true });
|
||||
}));
|
||||
const server = app().listen(0);
|
||||
try {
|
||||
const { port } = server.address() as { port: number };
|
||||
const controller = new AbortController();
|
||||
const pending = fetch(`http://127.0.0.1:${port}/api/companies/company-1/skills/skill-1/update-diff`, { signal: controller.signal }).catch(() => null);
|
||||
await vi.waitFor(() => expect(seen).toBeDefined());
|
||||
controller.abort();
|
||||
await pending;
|
||||
await vi.waitFor(() => expect(seen!.aborted).toBe(true));
|
||||
} finally { server.close(); }
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,122 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
|
||||
type ScanFile = { path: string; content: string; encoding?: "utf8" | "base64"; executable?: boolean };
|
||||
type Scan = { commitSha: string; packageMode?: "skills" | "repository"; repositorySubset?: boolean; repositoryFiles?: ScanFile[];
|
||||
repositoryPackage?: { warnings: string[] }; warnings?: string[]; skills: Array<{ path: string; files: ScanFile[]; warnings?: string[] }> };
|
||||
const scans = vi.hoisted(() => new Map<string, Scan>());
|
||||
const scanSignals = vi.hoisted(() => [] as Array<AbortSignal | undefined>);
|
||||
vi.mock("../services/github-skill-source.js", () => ({
|
||||
scanGitHubSkillsWithPackageFallback: vi.fn(async (input: { commitSha: string }, _read: unknown, options: { signal?: AbortSignal } = {}) => {
|
||||
scanSignals.push(options.signal);
|
||||
return { warnings: [], ...scans.get(input.commitSha) };
|
||||
}),
|
||||
}));
|
||||
import { diffGitHubSkillUpdate, diffSkillPackageFiles, MAX_UPDATE_DIFF_CHARS, MAX_UPDATE_DIFF_FILES, unifiedLineDiff } from "../services/skill-update-diff.js";
|
||||
|
||||
const installed = ["---", "version: \"0.4\"", "---", "", "Run checks so every build or release runs them.", "", "## Notes", "Keep it short."].join("\n");
|
||||
const latest = ["---", "version: \"0.5\"", "---", "", "Run checks so every new build runs them.", "", "## Design", "Breathable layouts.", "", "## Notes", "Keep it short."].join("\n");
|
||||
|
||||
describe("skill update diff", () => {
|
||||
it("marks installed-only lines with - and latest-only lines with +", () => {
|
||||
const diff = unifiedLineDiff(installed, latest)!;
|
||||
const lines = diff.split("\n");
|
||||
expect(lines).toContain("-version: \"0.4\"");
|
||||
expect(lines).toContain("+version: \"0.5\"");
|
||||
expect(lines).toContain("-Run checks so every build or release runs them.");
|
||||
expect(lines).toContain("+Run checks so every new build runs them.");
|
||||
expect(lines).toContain("+## Design");
|
||||
expect(lines).toContain("+Breathable layouts.");
|
||||
expect(lines).not.toContain("-## Notes");
|
||||
expect(lines.filter(line => line.startsWith("@@"))).toEqual(["@@ -1,8 +1,11 @@"]);
|
||||
expect(unifiedLineDiff(installed, installed)).toBe("");
|
||||
});
|
||||
|
||||
it("keeps distant changes in separate hunks with bounded context", () => {
|
||||
const before = Array.from({ length: 30 }, (_, i) => `line ${i + 1}`);
|
||||
const after = [...before]; after[1] = "changed 2"; after[27] = "changed 28";
|
||||
const diff = unifiedLineDiff(before.join("\n"), after.join("\n"))!;
|
||||
expect(diff.split("\n").filter(line => line.startsWith("@@"))).toEqual(["@@ -1,5 +1,5 @@", "@@ -25,6 +25,6 @@"]);
|
||||
expect(diff).not.toContain(" line 15");
|
||||
});
|
||||
|
||||
it("lists added, removed, modified and binary files and skips unchanged ones", () => {
|
||||
const { files, truncated } = diffSkillPackageFiles(
|
||||
[{ path: "SKILL.md", content: installed }, { path: "old.md", content: "gone\n" }, { path: "same.md", content: "same" }, { path: "logo.png", content: "AAA=", encoding: "base64" }],
|
||||
[{ path: "SKILL.md", content: latest }, { path: "new.md", content: "fresh\n" }, { path: "same.md", content: "same" }, { path: "logo.png", content: "AAE=", encoding: "base64" }],
|
||||
);
|
||||
expect(truncated).toBe(false);
|
||||
expect(files.map(file => [file.path, file.change, file.binary, file.diffUnavailable])).toEqual([
|
||||
["SKILL.md", "modified", false, null], ["logo.png", "modified", true, "binary"], ["new.md", "added", false, null], ["old.md", "removed", false, null],
|
||||
]);
|
||||
// An absent side has no lines, so an added or removed file shows only its own lines.
|
||||
expect(files.find(file => file.path === "new.md")!.diff).toBe("@@ -0,0 +1,1 @@\n+fresh");
|
||||
expect(files.find(file => file.path === "old.md")!.diff).toBe("@@ -1,1 +0,0 @@\n-gone");
|
||||
expect(files.find(file => file.path === "logo.png")!.diff).toBeNull();
|
||||
});
|
||||
|
||||
it("does not count a final newline as a line and shows when only the final newline changes", () => {
|
||||
expect(unifiedLineDiff("a\nb\n", "a\nc\n")).toBe("@@ -1,2 +1,2 @@\n a\n-b\n+c");
|
||||
expect(unifiedLineDiff("a\n", "a")).toBe("@@ -1,1 +1,1 @@\n-a\n+a\n\\ No newline at end of file");
|
||||
});
|
||||
|
||||
it("reports a change to only the executable bit", () => {
|
||||
const { files } = diffSkillPackageFiles([{ path: "run.sh", content: "echo hi\n" }], [{ path: "run.sh", content: "echo hi\n", executable: true }]);
|
||||
expect(files).toEqual([{ path: "run.sh", change: "modified", binary: false, executableChanged: true, diff: "", diffUnavailable: null }]);
|
||||
});
|
||||
|
||||
it("does not call a file the scan left out of one revision added or removed", () => {
|
||||
const { files } = diffSkillPackageFiles([{ path: "SKILL.md", content: "a" }], [{ path: "SKILL.md", content: "a" }, { path: "assets/logo.svg", content: "<svg/>" }],
|
||||
{ installed: new Set(["assets/logo.svg"]), latest: new Set() });
|
||||
expect(files).toEqual([{ path: "assets/logo.svg", change: "not_compared", binary: false, executableChanged: false, diff: null, diffUnavailable: "omitted" }]);
|
||||
});
|
||||
|
||||
it("bounds the diff text and the file count of one response", () => {
|
||||
const big = Array.from({ length: 400 }, (_, i) => ({ path: `f${String(i).padStart(3, "0")}.md`, content: `${"x".repeat(5_000)}\n` }));
|
||||
const { files, truncated } = diffSkillPackageFiles(big.map(file => ({ ...file, content: "" })), big);
|
||||
expect(truncated).toBe(true);
|
||||
expect(files.reduce((sum, file) => sum + (file.diff?.length ?? 0), 0)).toBeLessThanOrEqual(MAX_UPDATE_DIFF_CHARS);
|
||||
expect(files.at(-1)!.diffUnavailable).toBe("budget");
|
||||
const many = Array.from({ length: MAX_UPDATE_DIFF_FILES + 5 }, (_, i) => ({ path: `m${i}.md`, content: "x" }));
|
||||
const listed = diffSkillPackageFiles([], many);
|
||||
expect([listed.files.length, listed.truncated]).toEqual([MAX_UPDATE_DIFF_FILES, true]);
|
||||
});
|
||||
|
||||
it("compares the installed commit snapshot with the latest one for the selected skill", async () => {
|
||||
scans.set("a".repeat(40), { commitSha: "a".repeat(40), skills: [{ path: "accessibility/SKILL.md", files: [{ path: "SKILL.md", content: installed }] }] });
|
||||
scans.set("b".repeat(40), { commitSha: "b".repeat(40), skills: [{ path: "accessibility/SKILL.md", files: [{ path: "SKILL.md", content: latest }] }] });
|
||||
const controller = new AbortController();
|
||||
scanSignals.length = 0;
|
||||
const result = await diffGitHubSkillUpdate({ repositoryUrl: "https://github.com/example/skills", skillPath: "accessibility/SKILL.md", installedCommitSha: "a".repeat(40), latestCommitSha: "b".repeat(40) }, {} as never, controller.signal);
|
||||
if (!result.supported) throw new Error(result.reason);
|
||||
expect(result.files).toHaveLength(1);
|
||||
expect(result.files[0]!.diff).toContain("-Run checks so every build or release runs them.");
|
||||
expect(scanSignals).toEqual([controller.signal, controller.signal]);
|
||||
scans.set("c".repeat(40), { commitSha: "d".repeat(40), skills: [] });
|
||||
await expect(diffGitHubSkillUpdate({ repositoryUrl: "https://github.com/example/skills", skillPath: "accessibility/SKILL.md", installedCommitSha: "a".repeat(40), latestCommitSha: "c".repeat(40) }, {} as never)).rejects.toThrow("did not match");
|
||||
});
|
||||
|
||||
it("marks files the scan left out of a skill folder as not compared", async () => {
|
||||
scans.set("e".repeat(40), { commitSha: "e".repeat(40), skills: [{ path: "design/SKILL.md", files: [{ path: "SKILL.md", content: "a" }],
|
||||
warnings: ["design/assets/hero.png — Exceeds the 1 MB file limit; not included."] }] });
|
||||
scans.set("f".repeat(40), { commitSha: "f".repeat(40), skills: [{ path: "design/SKILL.md", files: [{ path: "SKILL.md", content: "a" }, { path: "assets/hero.png", content: "AAA=", encoding: "base64" }] }] });
|
||||
const result = await diffGitHubSkillUpdate({ repositoryUrl: "https://github.com/example/skills", skillPath: "design/SKILL.md", installedCommitSha: "e".repeat(40), latestCommitSha: "f".repeat(40) }, {} as never);
|
||||
expect(result).toEqual({ supported: true, truncated: false, files: [{ path: "assets/hero.png", change: "not_compared", binary: false, executableChanged: false, diff: null, diffUnavailable: "omitted" }] });
|
||||
});
|
||||
|
||||
it("refuses to compare a repository package that a scan could only include in part", async () => {
|
||||
const full = { commitSha: "1".repeat(40), packageMode: "repository" as const, repositoryFiles: [{ path: "README.md", content: "a" }], skills: [] };
|
||||
const input = { repositoryUrl: "https://github.com/example/skills", packageMode: "repository" as const, skillPath: "x/SKILL.md", installedCommitSha: "1".repeat(40), latestCommitSha: "2".repeat(40) };
|
||||
scans.set("1".repeat(40), full);
|
||||
for (const partial of [
|
||||
{ commitSha: "2".repeat(40), packageMode: "repository" as const, repositorySubset: true, repositoryFiles: [], skills: [] },
|
||||
{ commitSha: "2".repeat(40), packageMode: "skills" as const, skills: [] },
|
||||
]) {
|
||||
scans.set("2".repeat(40), partial);
|
||||
const result = await diffGitHubSkillUpdate(input, {} as never);
|
||||
expect(result.supported).toBe(false);
|
||||
}
|
||||
scans.set("2".repeat(40), { ...full, commitSha: "2".repeat(40), repositoryFiles: [{ path: "README.md", content: "b" }] });
|
||||
const result = await diffGitHubSkillUpdate(input, {} as never);
|
||||
expect(result.supported && result.files.map(file => file.path)).toEqual(["README.md"]);
|
||||
});
|
||||
});
|
||||
@@ -6,12 +6,13 @@ import { adoptProjectSkillRepository, projectSkillRepositories, skillSourceSyncS
|
||||
import { skillAccessRequestContext, skillAccessService } from "../services/skill-access.js";
|
||||
import { skillSourceService, type SkillSourceContext } from "../services/skill-sources.js";
|
||||
import { skillSourceGitHubReader } from "../services/skill-source-github-access.js";
|
||||
import { diffGitHubSkillUpdate } from "../services/skill-update-diff.js";
|
||||
import { toolAccessService } from "../services/tool-access.js";
|
||||
import { skillSourceCreateSchema, skillSourceDiscoverySchema, skillSourcePreviewSchema, skillSourceSelectionSchema, skillSourceSettingsSchema, skillSourceBranchesQuerySchema, projectSkillSourceAdoptSchema, skillSourceDisconnectSchema } from "@paperclipai/shared";
|
||||
import type { ActivityPublication } from "../services/activity-log.js";
|
||||
import type { PluginWorkerManager } from "../services/plugin-worker-manager.js";
|
||||
import { once } from "node:events";
|
||||
import type { SkillSourceDiscoveryEvent } from "@paperclipai/shared";
|
||||
import type { SkillSourceDiscoveryEvent, SkillSourceUpdateDiff } from "@paperclipai/shared";
|
||||
import { createHash } from "node:crypto";
|
||||
import { and, eq, sql } from "drizzle-orm";
|
||||
import { activityLog, companySkills, companySkillMutationReceipts } from "@paperclipai/db";
|
||||
@@ -1225,6 +1226,44 @@ export function companySkillRoutes(db: Db, options: { pluginWorkerManager?: Plug
|
||||
res.json(result);
|
||||
});
|
||||
|
||||
// Shows exactly what a pending tracked-source update would change, so a
|
||||
// reviewer never has to compare two full snapshots by eye.
|
||||
router.get("/companies/:companyId/skills/:skillId/update-diff", async (req, res) => {
|
||||
const companyId = req.params.companyId as string;
|
||||
const skillId = req.params.skillId as string;
|
||||
// The skillId parameter handler has already checked company access and that this actor can read the skill.
|
||||
assertCompanyAccess(req, companyId);
|
||||
const unsupported = (reason: string, currentRef: string | null = null, latestRef: string | null = null): SkillSourceUpdateDiff =>
|
||||
({ supported: false, reason, currentRef, latestRef, hasUpdate: false, files: [], truncated: false });
|
||||
const skill = await svc.getById(companyId, skillId);
|
||||
if (!skill) { res.status(404).json({ error: "Skill not found" }); return; }
|
||||
const managed = await sourceSvc.sourceForSkill(companyId, skillId);
|
||||
const entry = managed?.entries.find(row => row.skillId === skillId);
|
||||
if (!managed || !entry) { res.json(unsupported("Only skills installed from a tracked GitHub source have an update diff.", skill.sourceRef ?? null)); return; }
|
||||
if (!managed.enabled || entry.selection !== "selected") { res.json(unsupported("Source is disconnected. Reconnect it in Skills → Sources.", skill.sourceRef ?? null)); return; }
|
||||
const currentRef = skill.sourceRef ?? null;
|
||||
if (!currentRef || !/^[a-f0-9]{40}$/i.test(currentRef)) { res.json(unsupported("The installed source revision is unknown.", currentRef)); return; }
|
||||
// Stop the GitHub reads and both scans when the caller goes away; they hold shared scan and download slots.
|
||||
const controller = new AbortController();
|
||||
const stop = () => { if (!res.writableEnded) controller.abort(); };
|
||||
res.once("close", stop);
|
||||
try {
|
||||
const read = skillSourceGitHubReader(db, companyId, req.actor, managed.connectionId);
|
||||
const latest = await read(`/repos/${managed.fullName}/commits/${encodeURIComponent(managed.trackingRef)}`, controller.signal) as { sha: string };
|
||||
if (latest.sha.toLowerCase() === currentRef.toLowerCase()) {
|
||||
res.json({ supported: true, reason: null, currentRef, latestRef: latest.sha, hasUpdate: false, files: [], truncated: false } satisfies SkillSourceUpdateDiff);
|
||||
return;
|
||||
}
|
||||
const diff = await diffGitHubSkillUpdate({
|
||||
repositoryUrl: managed.repositoryUrl, packageMode: managed.packageMode, skillPath: entry.path, installedCommitSha: currentRef, latestCommitSha: latest.sha,
|
||||
}, read, controller.signal);
|
||||
const result: SkillSourceUpdateDiff = diff.supported
|
||||
? { supported: true, reason: null, currentRef, latestRef: latest.sha, hasUpdate: true, files: diff.files, truncated: diff.truncated }
|
||||
: { ...unsupported(diff.reason, currentRef, latest.sha), hasUpdate: true };
|
||||
res.json(result);
|
||||
} finally { res.off("close", stop); }
|
||||
});
|
||||
|
||||
router.get("/companies/:companyId/skills/:skillId/files", async (req, res) => {
|
||||
const companyId = req.params.companyId as string;
|
||||
const skillId = req.params.skillId as string;
|
||||
|
||||
@@ -8637,6 +8637,16 @@ registry.registerPath({
|
||||
responses: { 200: r.ok(), 401: r.unauthorized },
|
||||
});
|
||||
|
||||
registry.registerPath({
|
||||
method: "get",
|
||||
path: "/api/companies/{companyId}/skills/{skillId}/update-diff",
|
||||
tags: ["skills"],
|
||||
summary: "Show what a pending skill source update changes (diff installed vs latest revision)",
|
||||
description: "For a skill installed from a tracked GitHub source, compares the installed source revision (currentRef) with the latest tracked revision (latestRef) and returns each changed package file with a unified line diff: lines starting with \"-\" exist only in the installed revision and lines starting with \"+\" only in the latest one. hasUpdate is false and files is empty when the installed revision is already the latest. A file with change \"not_compared\" was left out of a revision's scan, and diffUnavailable says why a file has no diff. truncated is true when the response reached its size budget. Read-only; it does not apply the update. Use this to review an update before installing it.",
|
||||
request: { params: z.object({ companyId: z.string(), skillId: z.string() }) },
|
||||
responses: { 200: r.ok(), 401: r.unauthorized, 403: r.forbidden, 404: r.notFound },
|
||||
});
|
||||
|
||||
registry.registerPath({
|
||||
method: "get",
|
||||
path: "/api/companies/{companyId}/skills/{skillId}/files",
|
||||
|
||||
@@ -0,0 +1,170 @@
|
||||
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 }) };
|
||||
}
|
||||
Reference in new issue
Block a user