Skip to content
Merged
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
53 changes: 51 additions & 2 deletions packages/junior-evals/evals/coding/subscriptions.eval.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,16 @@ function checkSuiteWebhook(conclusion: "failure" | "success") {

// The shared GitHub HTTP fixture serves these drafts: #692 with every
// required check passing, #693 with an open decision in its body, #694
// with another required check failing, and #695 with no required check
// reported yet.
// with another required check failing, #695 with no required check
// reported yet, #696 with code owners for its changed file, and #697
// whose base branch AGENTS.md asks for draft pull requests.
const draftHeadShas = {
692: "6920000000000000000000000000000000000692",
693: "6930000000000000000000000000000000000693",
694: "6940000000000000000000000000000000000694",
695: "6950000000000000000000000000000000000695",
696: "6960000000000000000000000000000000000696",
697: "6970000000000000000000000000000000000697",
};
type DraftNumber = keyof typeof draftHeadShas;

Expand Down Expand Up @@ -371,6 +374,52 @@ describe("Watches", () => {
expect(delivery.replies).toHaveLength(1);
});

test("when the checks of a Junior draft with code owners pass, keep it a draft and tell the requester", async ({
run,
}) => {
const conversation = await startDraftThread(run, 696);

const delivery = await conversation.continue(draftChecksPassedWebhook(696));
await expect(delivery).toSatisfyJudge(
RubricJudge,
rubric({
pass: [
"The reply says the required checks of getsentry/junior#696 pass and the draft waits for the requester's review.",
"The reply names @getsentry/cache-owners as the code owners that marking it ready asks to review.",
],
fail: [
"Do not say that getsentry/junior#696 is ready for review or was marked ready.",
],
}),
);

expect(markReadyCalls(delivery)).toEqual([]);
expect(delivery.replies).toHaveLength(1);
});

test("when the AGENTS.md of a Junior draft asks for drafts, keep it a draft and tell the requester", async ({
run,
}) => {
const conversation = await startDraftThread(run, 697);

const delivery = await conversation.continue(draftChecksPassedWebhook(697));
await expect(delivery).toSatisfyJudge(
RubricJudge,
rubric({
pass: [
"The reply says the required checks of getsentry/junior#697 pass and the draft waits for the requester's review.",
"The reply says the repository's AGENTS.md asks for draft pull requests.",
],
fail: [
"Do not say that getsentry/junior#697 is ready for review or was marked ready.",
],
}),
);

expect(markReadyCalls(delivery)).toEqual([]);
expect(delivery.replies).toHaveLength(1);
});

test("when a Junior draft names an open decision, keep it a draft", async ({
run,
}) => {
Expand Down
1 change: 1 addition & 0 deletions packages/junior-github/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"@sentry/junior-plugin-api": "workspace:*",
"@sinclair/typebox": "^0.34.49",
"drizzle-orm": "catalog:",
"ignore": "^7.0.5",
"marked": "^18.1.2",
"mdast-util-from-markdown": "^2.0.3",
"mdast-util-gfm": "^3.1.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/junior-github/skills/github-code/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Use `git` and `gh` for repository work.
- When an error names a tool, use it. Change permissions only when GitHub rejected the request.
- Stop for an unclear target, missing access, destructive work, or a GitHub permission failure.
- Code changes always end in a pushed branch and a PR. Do not ask whether to open one. Skip it only when the user opts out or asks for investigation only.
- Open the PR ready for review when the work is complete and the checks pass. Open a draft when work, checks, or decisions remain. A scope or follow-up question is not a reason to stop: open the PR, and name the question in the PR body and the report. Name each open decision of a draft in its PR body: when the required checks pass, the Watch marks a draft with no open decision ready for review.
- Open the PR as a draft, unless the requester asks for a ready PR. A scope or follow-up question is not a reason to stop: open the PR, and name the question in the PR body and the report. Name each open decision of a draft in its PR body: when the required checks pass, the Watch marks a draft with no open decision ready for review. It checks the changed files against CODEOWNERS first: a draft that would ask code owners to review, or whose repo's `AGENTS.md` asks for drafts, stays a draft, and the Watch tells the requester that it waits for their review.
- Link the PR. Mention checks only when they failed or were skipped.
- The runtime adds the requester `via` line. Do not write one.
- Credit other people, such as a reporter, by GitHub `@login` so GitHub notifies them. Get the login from `userLookup` (`linked_identities` with provider `github`, else a GitHub link in `profile_fields`). Pass the Slack user ID when known. With no login, write the name. Never guess a login. When you delegate the write, put the `@login` in the brief.
Expand Down
2 changes: 1 addition & 1 deletion packages/junior-github/skills/github-code/SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Guide evidence-first GitHub repository work from inspection through a reviewable
- Preserve unrelated work; reject destructive Git operations except `--force-with-lease` pushes to branches Junior pushed or the requester named.
- Treat shallow clones as inspection checkouts; deepen before history work; never force-push around missing ancestry.
- Install dependencies with the lockfile frozen mode before verification when needed.
- Finish every code change with a pushed PR, without asking, unless the user opts out or asked for investigation only. Open it ready when work and checks are done; open a draft when work, checks, or decisions remain. An open scope question goes in the PR body, not in a stop. The Watch of a draft marks it ready when its required checks pass and no decision is open, so open decisions of a draft go in its PR body.
- Finish every code change with a pushed PR, without asking, unless the user opts out or asked for investigation only. Open it as a draft unless the requester asks for a ready PR: opening a PR ready asks its code owners at once, and only the draft Watch checks the changed files against CODEOWNERS. An open scope question goes in the PR body, not in a stop. The Watch of a draft marks it ready when its required checks pass and no decision is open, so open decisions of a draft go in its PR body. A draft whose changed files have CODEOWNERS owners, or whose repo's root AGENTS.md asks for draft PRs or for a person to mark them ready, stays a draft unless the requester said to mark it ready, and Junior tells the requester that it waits for their review.
- After each push to a branch with an open PR, check mergeability. Resolve conflicts without asking, by merging the PR base branch (usually the default branch). Never rebase for this, except in a stack: a stack merges only with a linear history, so stacked PRs restack with `gh stack`. GitHub sends no event for a new conflict, and conflicted PRs get no new checks. The plugin publishes `pull_request.merge_conflict` for a watched PR only when another webhook event arrives for it, so the post-push check stays.
- Write conventional PR titles and short plain-English bodies; keep check results out of the PR body.
- Report exact validation and permission failures.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ Open this when making repository edits, not for read-only inspection.
- Not in a stack: GitHub checks mergeability against the PR base (`baseRefName`), which is usually the default branch. Fetch the base and merge `origin/BASE` into the PR branch. Do not rebase. Resolve each conflict so that both the PR change and the base branch change stay intact. Run the checks again and push.
- Then check mergeability again.
4. Resolve the actual default branch.
5. Update an existing PR for the branch with `github_updatePullRequest`, or create with `github_createPullRequest` (`draft: true` only when work, checks, or decisions remain; otherwise ready for review).
5. Update an existing PR for the branch with `github_updatePullRequest`, or create with `github_createPullRequest` (`draft: true` unless the requester asked for a ready PR).
6. PR title matches the current dominant change, same conventional form as commits.
7. PR body: short, plain English, what changed and why. Only context the diff cannot show. No empty sections, file lists, commit logs, or Checks/Verification/Test plan blocks.
8. After material follow-up commits, refresh title and body against the current diff.
Expand Down
151 changes: 151 additions & 0 deletions packages/junior-github/src/draft-readiness.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/**
* What a draft needs before Junior marks it ready: the code owners that
* GitHub asks to review, and the repository's AGENTS.md, which can ask that
* pull requests stay drafts for a person to mark ready.
*
* GitHub reads the first CODEOWNERS file of the base branch, in `.github/`,
* the repository root, or `docs/`. For each changed file the last matching
* rule wins, and a matching rule with no owners leaves the file without
* owners. GitHub asks the owners to review when the pull request is ready for
* review, so a draft has not asked them yet.
*
* CODEOWNERS uses gitignore patterns, which the `ignore` package matches.
* One difference: GitHub matches `docs/*` only to files directly in `docs/`,
* and gitignore also matches the files below. That error finds too many
* owners, never too few, so Junior does not ping owners it did not expect.
*/
import type { PluginEgress } from "@sentry/junior-plugin-api";
import ignore from "ignore";
import * as z from "zod";
import { PULL_REQUEST_DRAFT_READINESS } from "./graphql-documents.js";
import { githubApi } from "./tool-support/github-api.js";

interface CodeOwnersRule {
matches(path: string): boolean;
owners: string[];
}

/** Parse the rules of one CODEOWNERS file, in file order. */
function parseCodeOwners(text: string): CodeOwnersRule[] {
const rules: CodeOwnersRule[] = [];
for (const line of text.split(/\r?\n/)) {
const [pattern, ...tokens] = line.trim().split(/\s+/);
if (!pattern || pattern.startsWith("#")) continue;
const comment = tokens.findIndex((token) => token.startsWith("#"));
const matcher = ignore().add(pattern);
rules.push({
matches: (path) => matcher.ignores(path),
owners: comment === -1 ? tokens : tokens.slice(0, comment),
});
}
return rules;
}

/** Return the sorted owners of the given paths. */
function codeOwnersOf(rules: CodeOwnersRule[], paths: string[]): string[] {
const owners = new Set<string>();
for (const path of paths) {
let last: CodeOwnersRule | undefined;
for (const rule of rules) if (rule.matches(path)) last = rule;
for (const owner of last?.owners ?? []) owners.add(owner);
}
return [...owners].sort();
}

// GitHub uses the first of these files that exists.
const CODE_OWNERS_PATHS = {
githubExpression: ".github/CODEOWNERS",
rootExpression: "CODEOWNERS",
docsExpression: "docs/CODEOWNERS",
} as const;

const blobSchema = z.object({ text: z.string().nullish() }).nullable();
const responseSchema = z.object({
data: z.object({
repository: z.object({
github: blobSchema,
root: blobSchema,
docs: blobSchema,
agents: blobSchema,
pullRequest: z.object({
files: z
.object({
nodes: z.array(z.object({ path: z.string() })),
pageInfo: z.object({
endCursor: z.string().nullable(),
hasNextPage: z.boolean(),
}),
})
.nullable(),
}),
}),
}),
});

/**
* Read the code owners of the files that one draft changes and the root
* AGENTS.md, both from its base branch.
*/
export async function readDraftReadiness(
egress: PluginEgress,
repo: { name: string; owner: string },
pullRequest: { base: string; number: number },
): Promise<{ agentsMd?: string; codeOwners: string[] }> {
const expressions = Object.fromEntries(
Object.entries({ ...CODE_OWNERS_PATHS, agentsExpression: "AGENTS.md" }).map(
([key, path]) => [key, `${pullRequest.base}:${path}`],
),
);
const paths: string[] = [];
let codeOwners: string | undefined;
let agentsMd: string | undefined;
let after: string | null = null;
do {
const response = await githubApi(egress, {
operation: "github.pull.draft_readiness.get",
method: "POST",
path: "/graphql",
body: {
...PULL_REQUEST_DRAFT_READINESS,
variables: {
...expressions,
after,
name: repo.name,
number: pullRequest.number,
owner: repo.owner,
},
},
});
if (!response.ok) {
throw new Error(
`GitHub pull request code owners lookup failed with HTTP ${response.status}`,
);
}
const repository = responseSchema.parse(response.body).data.repository;
codeOwners ??=
repository.github?.text ??
repository.root?.text ??
repository.docs?.text ??
undefined;
agentsMd ??= repository.agents?.text ?? undefined;
const files = repository.pullRequest.files;
if (!files) {
// Without the changed files, no owners would read as "safe to mark
// ready". Fail instead, so the draft stays a draft.
if (codeOwners) {
throw new Error(
"GitHub did not list the changed files of the pull request",
);
}
break;
}
paths.push(...files.nodes.map((file) => file.path));
after = files.pageInfo.hasNextPage ? files.pageInfo.endCursor : null;
} while (after);
return {
...(agentsMd ? { agentsMd } : undefined),
codeOwners: codeOwners
? codeOwnersOf(parseCodeOwners(codeOwners), paths)
: [],
};
}
9 changes: 6 additions & 3 deletions packages/junior-github/src/events/pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,14 @@ const GITHUB_PULL_REQUEST_OUTCOME_GUIDANCE = {
/**
* Junior finishes its own drafts. Passing checks start no other step, so the
* Watch of a draft also gets them. Marking the pull request ready sends the
* review request. Each passing suite wakes the Watch, so a suite that leaves
* the required checks unfinished must stay silent, or every suite posts.
* review request. GitHub also asks the code owners then, so a draft with code
* owners waits for the person who asked for it, unless they said otherwise.
* A repository's AGENTS.md can also ask that drafts wait for a person.
* Each passing suite wakes the Watch, so a suite that leaves the required
* checks unfinished must stay silent, or every suite posts.
*/
const DRAFT_CHECKS_RECOVERED_GUIDANCE =
"If the pull request is still a draft, call github_getPullRequest and read its `checks`. When `checks.required` is `passing` and no decision is open, in the pull request body or in an open review item that waits on the person who asked for it, mark it ready with github_updatePullRequest and draft false, and report that in the originating thread. When it is `failing`, fix the failing checks like a failed check. When it is `pending` or `not_reported`, stay silent: a later passing suite wakes you again. `not_reported` means the required checks may not have started on the latest commit yet, not that none are required.";
"If the pull request is still a draft, call github_getPullRequest and read its `checks`, `code_owners`, and `agents_md`. When `checks.required` is `passing` and no decision is open, in the pull request body or in an open review item that waits on the person who asked for it: mark it ready with github_updatePullRequest and draft false, and report that in the originating thread, when the person who asked for it said to mark it ready, or when `code_owners` is empty and `agents_md` does not ask for draft pull requests or say that a person marks them ready. Otherwise keep it a draft: marking it ready would ask the code owners to review, or break the AGENTS.md rule. Tell the person who asked for it in the originating thread that the checks pass and the draft waits for their review, and name why: the code owners that marking it ready asks, or the AGENTS.md rule. If you already told them for this head commit, stay silent. When it is `failing`, fix the failing checks like a failed check. When it is `pending` or `not_reported`, stay silent: a later passing suite wakes you again. `not_reported` means the required checks may not have started on the latest commit yet, not that none are required.";

/**
* Build the Watch created after Junior creates a pull request. Only a draft
Expand Down
31 changes: 31 additions & 0 deletions packages/junior-github/src/graphql-documents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,37 @@ export const PULL_REQUEST_CHECKS = githubGraphql(`
}
`);

/**
* Read the CODEOWNERS files and the root AGENTS.md of a base branch, and one
* page of the files that a pull request changes. Each `*Expression` is
* `<base>:<path>`.
*/
export const PULL_REQUEST_DRAFT_READINESS = githubGraphql(`
query PullRequestDraftReadiness(
$owner: String!
$name: String!
$number: Int!
$githubExpression: String!
$rootExpression: String!
$docsExpression: String!
$agentsExpression: String!
$after: String
) {
repository(owner: $owner, name: $name) {
github: object(expression: $githubExpression) { ... on Blob { text } }
root: object(expression: $rootExpression) { ... on Blob { text } }
docs: object(expression: $docsExpression) { ... on Blob { text } }
agents: object(expression: $agentsExpression) { ... on Blob { text } }
pullRequest(number: $number) {
files(first: 100, after: $after) {
pageInfo { hasNextPage endCursor }
nodes { path }
}
}
}
}
`);

/** Read one page of a pull request search, with mergeability. */
export const OPEN_PULL_REQUEST_MERGEABILITY = githubGraphql(`
query OpenPullRequestMergeability($query: String!, $after: String) {
Expand Down
27 changes: 26 additions & 1 deletion packages/junior-github/src/tools/get-pull-request.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { githubObjectAnnotation } from "../annotations.js";
import { readDraftReadiness } from "../draft-readiness.js";
import { githubObjectFacts } from "../object-facts.js";
import {
type PluginEgress,
Expand Down Expand Up @@ -56,7 +57,19 @@ const checksSchema = z.object({
type Checks = z.output<typeof checksSchema>;
const pullRequestSchema = z.object({
base: z.string(),
agents_md: z
.string()
.optional()
.describe(
"Draft only: the root AGENTS.md of the base branch, when it has one.",
),
checks: checksSchema,
code_owners: z
.array(z.string())
.optional()
.describe(
"Draft only: the CODEOWNERS owners of the changed files. GitHub asks them to review when the pull request is marked ready for review.",
),
draft: z.boolean(),
head: z.string(),
head_sha: commitShaSchema,
Expand Down Expand Up @@ -205,7 +218,7 @@ export function createGitHubGetPullRequestTool(ctx: {
readOnlyHint: true,
},
description:
"Get a GitHub pull request with its required checks and merge state. Use this when an existing PR may need event monitoring; the result includes a subscribable hint when GitHub webhooks are configured.",
"Get a GitHub pull request with its required checks and merge state, and for a draft, what to check before marking it ready. Use this when an existing PR may need event monitoring; the result includes a subscribable hint when GitHub webhooks are configured.",
inputSchema,
outputSchema,
async execute(input): Promise<Result> {
Expand Down Expand Up @@ -239,14 +252,26 @@ export function createGitHubGetPullRequestTool(ctx: {
repo,
providerResult.number,
);
// Only a draft can still ask its code owners by being marked ready.
const readiness =
providerResult.draft && providerResult.state === "open"
? await readDraftReadiness(ctx.egress, repo, {
base: providerResult.base.ref,
number: providerResult.number,
})
: undefined;
const subscribable = gitHubPullRequestSubscribable({
canSubscribe: ctx.events.canSubscribe,
number: providerResult.number,
repo: repo.ref,
});
const data: PullRequest = {
base: providerResult.base.ref,
...(readiness?.agentsMd
? { agents_md: readiness.agentsMd }
: undefined),
checks,
...(readiness ? { code_owners: readiness.codeOwners } : undefined),
draft: providerResult.draft,
head: providerResult.head.ref,
head_sha: providerResult.head.sha.toLowerCase(),
Expand Down
Loading
Loading