feat(canvas): persist improvement actions and synchronization by lukasoppermann · Pull Request #41 · github/replica · GitHub
Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/extensions/replica-prototype/README.md

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import { mkdtemp, rm } from "node:fs/promises"
import os from "node:os"
import path from "node:path"

import { runCommand } from "../run-command/index.mjs"
import { gitShaPattern } from "../identity-patterns/index.mjs"

export async function captureReviewRevision(context) {
const directory = await mkdtemp(path.join(os.tmpdir(), "replica-review-"))
const gitDirectory = await runCommand(
"git",
["rev-parse", "--absolute-git-dir"],
context.repositoryPath,
)
if (!gitDirectory.ok) throw new Error("Could not locate the local review Git directory.")
const directory = await mkdtemp(path.join(gitDirectory.stdout.trim(), "replica-review-"))
const env = { ...process.env, GIT_INDEX_FILE: path.join(directory, "index") }
const config = ["-c", "core.fsmonitor=false", "-c", "core.hooksPath=/dev/null"]
async function git(args, optional = false) {
Expand All @@ -26,7 +32,7 @@ export async function captureReviewRevision(context) {
const head = (await git(["rev-parse", "HEAD"])).stdout.trim()
const branch = (await git(["branch", "--show-current"])).stdout.trim()
const headTree = (await git(["rev-parse", "HEAD^{tree}"])).stdout.trim()
if (!/^[0-9a-f]{40}$/.test(head) || !branch)
if (!gitShaPattern.test(head) || !branch)
throw new Error("Open a committed, named branch before commenting.")
await git(["read-tree", head])
const files = (
Expand Down Expand Up @@ -83,7 +89,7 @@ export async function captureReviewRevision(context) {
const untracked = (await git(["ls-files", "--others", "--exclude-standard"])).stdout
if (currentHead !== head || currentBranch !== branch || !remaining.ok || untracked)
throw new Error("The checkout changed while capturing this review revision. Try again.")
if (!/^[0-9a-f]{40}$/.test(treeSha))
if (!gitShaPattern.test(treeSha))
throw new Error("Could not verify the local review source tree.")
return { treeSha, commitSha: head, clean: treeSha === headTree }
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"
import path from "node:path"

import { readRepositoryName } from "../read-repository-name/index.mjs"
import { gitShaPattern } from "../identity-patterns/index.mjs"
import { runCommand } from "../run-command/index.mjs"

export async function createCanvasContext({
Expand All @@ -11,6 +12,7 @@ export async function createCanvasContext({
branch,
commitSha,
publishedUrl,
improvementThreadsSample = false,
sessionId,
sessionWorkspacePath,
repositoryPath,
Expand All @@ -27,6 +29,8 @@ export async function createCanvasContext({
}

const normalizedUrl = normalizePrototypeUrl(prototypeUrl)
if (typeof improvementThreadsSample !== "boolean")
throw new Error("improvementThreadsSample must be a boolean.")
const stateUrl = new URL(normalizedUrl)
stateUrl.hash = ""
const resolvedRepositoryPath = path.resolve(repositoryPath)
Expand Down Expand Up @@ -62,6 +66,7 @@ export async function createCanvasContext({
stateFile: path.join(stateDirectory, `${stateId}.json`),
stateId,
urlHost: new URL(normalizedUrl).host,
...(improvementThreadsSample ? { improvementThreadsSample: true } : {}),
}
}

Expand All @@ -88,6 +93,7 @@ export async function currentCanvasContext(context) {
repository: currentRepository,
branch: currentBranch,
publishedUrl: context.target.publishedUrl,
improvementThreadsSample: context.improvementThreadsSample,
sessionId: context.sessionId,
sessionWorkspacePath: context.sessionWorkspacePath,
repositoryPath: context.repositoryPath,
Expand Down Expand Up @@ -240,7 +246,7 @@ function normalizeCommitSha(value) {
if (value == null) {
return null
}
if (typeof value !== "string" || !/^[0-9a-f]{40}$/.test(value)) {
if (typeof value !== "string" || !gitShaPattern.test(value)) {
throw new Error("commitSha must be a full lowercase Git commit SHA.")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ const base = {
}

describe("createCanvasContext()", () => {
it("keeps thread samples explicitly opted in without changing saved-note identity", async () => {
await withContext(async (fixture) => {
const input = { ...base, ...paths(fixture) }
const ordinary = await createCanvasContext(input)
const sample = await createCanvasContext({ ...input, improvementThreadsSample: true })
assert.equal(ordinary.improvementThreadsSample, undefined)
assert.equal(sample.improvementThreadsSample, true)
assert.equal(sample.stateId, ordinary.stateId)
execFileSync("git", ["checkout", "-qb", "thread-sample"], { cwd: fixture.repositoryPath })
assert.equal((await currentCanvasContext(sample)).improvementThreadsSample, true)
await assert.rejects(
createCanvasContext({ ...input, improvementThreadsSample: "true" }),
/must be a boolean/,
)
})
})

it("normalizes the URL and retains explicit target metadata", async () => {
await withContext(async (fixture) => {
const input = { ...base, ...paths(fixture) }
Expand Down Expand Up @@ -49,8 +66,13 @@ describe("createCanvasContext()", () => {
await withContext(async (fixture) => {
const input = { ...base, ...paths(fixture) }
const original = await createCanvasContext(input)
execFileSync("git", ["checkout", "-qb", "another"], { cwd: fixture.repositoryPath })
const changed = await createCanvasContext({ ...input, branch: "another" })
execFileSync("git", ["checkout", "-qb", "another"], {
cwd: fixture.repositoryPath,
})
const changed = await createCanvasContext({
...input,
branch: "another",
})
assert.notEqual(original.stateId, changed.stateId)
})
})
Expand Down Expand Up @@ -79,7 +101,10 @@ describe("createCanvasContext()", () => {
cwd: fixture.repositoryPath,
})
const input = { ...base, ...paths(fixture) }
const supplied = await createCanvasContext({ ...input, repository: "GitHub/Replica" })
const supplied = await createCanvasContext({
...input,
repository: "GitHub/Replica",
})
assert.equal(supplied.target.repository, "github/replica")
const discovered = await createCanvasContext({
...input,
Expand Down Expand Up @@ -157,7 +182,9 @@ describe("createCanvasContext()", () => {
const repositoryPath = path.join(workspace, ".local/prototypes/example")
await mkdir(repositoryPath, { recursive: true })
try {
execFileSync("git", ["init", "-q", "-b", "main"], { cwd: repositoryPath })
execFileSync("git", ["init", "-q", "-b", "main"], {
cwd: repositoryPath,
})
execFileSync("git", ["remote", "add", "origin", "https://github.com/github/replica.git"], {
cwd: repositoryPath,
})
Expand Down Expand Up @@ -196,7 +223,10 @@ describe("createCanvasContext()", () => {
createCanvasContext(input),
/Cannot verify repository hint: The origin remote is not a GitHub repository/,
)
const withoutHint = await createCanvasContext({ ...input, repository: undefined })
const withoutHint = await createCanvasContext({
...input,
repository: undefined,
})
assert.equal(withoutHint.target.repository, null)
execFileSync(
"git",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import { createHubClient } from "./index.mjs"
it("verifies repository authorization independently of branch discovery and never trusts a bare 404", async (t) => {
const root = await mkdtemp(path.join(os.tmpdir(), "replica-authorization-"))
t.after(() => rm(root, { recursive: true, force: true }))
execFileSync("git", ["init", "-b", "prototype/new", root], { stdio: "ignore" })
execFileSync("git", ["init", "-b", "prototype/new", root], {
stdio: "ignore",
})
execFileSync("git", [
"-C",
root,
Expand All @@ -23,6 +25,7 @@ it("verifies repository authorization independently of branch discovery and neve
let status = 200
let repository = "github/prototype"
let canPublish = false
let viewer = { viewerId: 7, viewerLogin: "mona" }
const server = createServer((request, response) => {
response.setHeader("X-Replica-Canvas-Version", "1")
if (request.url.startsWith("/api/canvas/prototype")) {
Expand All @@ -32,7 +35,7 @@ it("verifies repository authorization independently of branch discovery and neve
assert.equal(request.url, "/api/canvas/connection")
assert.equal(request.headers.authorization, "Bearer fixture-token")
response.writeHead(status, { "Content-Type": "application/json" })
response.end(JSON.stringify({ repository, canPublish }))
response.end(JSON.stringify({ repository, canPublish, ...viewer }))
}
})
await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve))
Expand All @@ -57,6 +60,28 @@ it("verifies repository authorization independently of branch discovery and neve
assert.equal(authorized.repositoryAuthorized, true)
assert.equal(authorized.branch, "prototype/new")
assert.equal(authorized.canPublish, false)
assert.equal(authorized.viewerId, 7)
assert.equal(authorized.viewerLogin, "mona")
const expectedTarget = { repository, branch: "prototype/new" }
const scope = {}
const readOnly = await client.status({
register: false,
expectedTarget,
connectionScope: scope,
})
assert.equal(readOnly.viewerId, 7)
assert.ok(scope.identity)
viewer = { viewerLogin: "mona" }
assert.equal((await client.status({ register: false })).viewerId, undefined)
for (const invalid of [
{ viewerId: "7", viewerLogin: "mona" },
{ viewerId: 0, viewerLogin: "mona" },
{ viewerId: 7, viewerLogin: "../user" },
]) {
viewer = invalid
assert.equal((await client.status({ register: false })).state, "unavailable")
}
viewer = { viewerId: 7, viewerLogin: "mona" }
canPublish = true
assert.equal((await client.status()).canPublish, true)
for (const code of [401, 403, 404, 500]) {
Expand Down
46 changes: 30 additions & 16 deletions .github/extensions/replica-prototype/create-hub-client/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { readCanvasRepository } from "../read-canvas-repository/index.mjs"
import { runCommand } from "../run-command/index.mjs"
import { createPrivateStore } from "../private-store/index.mjs"
import { normalizeHubOrigin } from "../normalize-hub-origin/index.mjs"
import { githubLoginPattern } from "../identity-patterns/index.mjs"

function registrationReason(value) {
if (value == null) return undefined
Expand All @@ -14,6 +15,18 @@ function registrationReason(value) {
return value
}

function connectionViewer(connection) {
if (connection.viewerId == null) return {}
Comment thread
lukasoppermann marked this conversation as resolved.
if (
!Number.isSafeInteger(connection.viewerId) ||
connection.viewerId < 1 ||
typeof connection.viewerLogin !== "string" ||
!githubLoginPattern.test(connection.viewerLogin)
)
throw new Error("Hub returned an invalid connected author.")
return { viewerId: connection.viewerId, viewerLogin: connection.viewerLogin }
}

export function createHubClient(context, options = {}) {
const origin = new URL(normalizeHubOrigin(options.origin || process.env.REPLICA_HUB_ORIGIN))
const directory = options.directory || path.join(os.homedir(), ".config", "replica", "canvas")
Expand Down Expand Up @@ -269,7 +282,7 @@ export function createHubClient(context, options = {}) {
const credential = await credentialFor(repository)
return credential ? connectionIdentity(credential) : null
},
async status() {
async status({ register = true, expectedTarget, connectionScope } = {}) {
const target = await checkout()
if (!target.repository) {
return {
Expand All @@ -278,12 +291,20 @@ export function createHubClient(context, options = {}) {
}
}
try {
const credential = await credentialFor(target.repository)
const credential = await credentialFor(target.repository, undefined, connectionScope)
const lookup = (route) =>
register
? authorizedRequest(target.repository, credential, route)
: this.request(route, {
expectedTarget: expectedTarget ?? target,
connectionScope,
})
if (!credential) {
const pending = await readPending(target.repository)
if (pending?.refreshToken && Date.parse(pending.expiresAt) <= Date.now()) {
const result = await this.poll()
if (result.status === "approved") return this.status()
if (result.status === "approved")
return this.status({ register, expectedTarget, connectionScope })
}
const pairing = pending && Date.parse(pending.expiresAt) > Date.now()
const previous = await readCredential(target.repository)
Expand All @@ -300,11 +321,7 @@ export function createHubClient(context, options = {}) {
}
}
if (!target.branch) {
const connection = await authorizedRequest(
target.repository,
credential,
"/api/canvas/connection",
)
const connection = await lookup("/api/canvas/connection")
if (connection.repository?.toLowerCase() !== target.repository.toLowerCase())
throw new Error("Hub did not verify this repository.")
return {
Expand All @@ -313,25 +330,20 @@ export function createHubClient(context, options = {}) {
reviewConnectionId: connectionIdentity(credential),
canPublish: connection.canPublish,
viewerLogin: connection.viewerLogin,
...connectionViewer(connection),
repository: target.repository,
commitSha: target.commitSha,
message: "Connected to this repository. Open a named branch for Builds and reviews.",
}
}
let prototype
try {
prototype = await authorizedRequest(
target.repository,
credential,
prototype = await lookup(
`/api/canvas/prototype?branch=${encodeURIComponent(target.branch)}`,
)
} catch (error) {
if (error.statusCode !== 404) throw error
const connection = await authorizedRequest(
target.repository,
credential,
"/api/canvas/connection",
)
const connection = await lookup("/api/canvas/connection")
if (connection.repository?.toLowerCase() !== target.repository.toLowerCase())
throw new Error("Hub did not verify this repository.", {
cause: error,
Expand All @@ -342,6 +354,7 @@ export function createHubClient(context, options = {}) {
reviewConnectionId: connectionIdentity(credential),
canPublish: connection.canPublish,
viewerLogin: connection.viewerLogin,
...connectionViewer(connection),
repository: target.repository,
branch: target.branch,
commitSha: target.commitSha,
Expand All @@ -351,6 +364,7 @@ export function createHubClient(context, options = {}) {
let registrationError
let reason = registrationReason(prototype.registrationReason)
if (
register &&
!prototype.registered &&
prototype.canEdit &&
prototype.canRegister &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,16 @@ describe("createHubClient()", () => {
status = 401
} else if (mode === "change-checkout") {
process.env.TEST_BRANCH = "prototype/other"
} else if (request.url.startsWith("/api/canvas/prototype?")) {
result = {
id: 4,
repositoryId: 5,
branch: "prototype/first",
registered: false,
canEdit: true,
canRegister: true,
build: { commitSha: "1".padStart(40, "0") },
}
}
response.writeHead(status, { "Content-Type": "application/json" })
response.end(JSON.stringify(result))
Expand Down Expand Up @@ -172,6 +182,13 @@ esac
connectionScope: {},
}
await hub.request("/api/canvas/prototype?branch=prototype/first", options)
const status = await hub.status({ register: false, ...options })
assert.equal(status.state, "connected")
assert.equal(status.prototype.registered, false)
assert.equal(
calls.some(({ method }) => method === "PATCH"),
false,
)
mode = "unversioned"
assert.deepEqual(await hub.request("/api/canvas/comments", options), {})
mode = "incompatible"
Expand Down
Loading
Loading