From e44ea80e382fb7f0cbf36c9c84f24ff7d5a4a852 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:21:42 +0000 Subject: [PATCH 1/8] ref(egress): simplify host installation credential cache Keep sandbox isolation via host injection, but stop minting and caching installation tokens per sandbox context or repository lease scope. Share installation leases on the host, refresh before expiry, and remint once on upstream 403 so mid-PR pushes no longer look like lost App write access. Co-Authored-By: David Cramer --- TELEMETRY.md | 3 +- packages/junior-github/SETUP.md | 12 +- .../junior-github/skills/github-code/SKILL.md | 2 +- .../github-code/references/api-surface.md | 6 +- .../junior-github/src/credential-support.ts | 36 +- packages/junior-github/src/egress-policy.ts | 27 +- packages/junior-github/src/plugin.ts | 9 +- .../resolve-pull-request-review-thread.ts | 4 +- .../junior-github/tests/github-plugin.test.ts | 31 +- .../tests/webhook-outcomes.test.ts | 2 +- .../junior/src/chat/egress/credentialed.ts | 346 ++++++++++-------- .../src/chat/sandbox/egress/credentials.ts | 6 +- .../junior/src/chat/sandbox/egress/session.ts | 54 ++- .../handlers/sandbox-egress-proxy.test.ts | 89 +++-- .../integration/sandbox-egress-proxy.test.ts | 8 +- .../sandbox-egress-credentials.test.ts | 68 +++- 16 files changed, 386 insertions(+), 317 deletions(-) diff --git a/TELEMETRY.md b/TELEMETRY.md index f27e546e1a..8e74dc2896 100644 --- a/TELEMETRY.md +++ b/TELEMETRY.md @@ -248,7 +248,8 @@ conversation, use `app.dispatch.id` or `agent-dispatch:` as A turn parked for auth, resumed late, or failed after callback. Events: `sandbox.egress.credential.needed`, -`sandbox.egress.credential.unavailable`, `plugin.credential.rejected`, +`sandbox.egress.credential.unavailable`, `sandbox.egress.upstream_auth.rejected`, +`sandbox.egress.upstream_auth.retrying`, `plugin.credential.rejected`, `subscribed_message.authorization.required`, `agent.continue.schedule.failed`, `agent.continue.lock.busy`, `agent.continue.lock.retrying`, `oauth.callback.resume.completed`, `oauth.callback.resume.busy`, diff --git a/packages/junior-github/SETUP.md b/packages/junior-github/SETUP.md index d94b92c6ad..37f5508715 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -148,16 +148,16 @@ githubPlugin({ }); ``` -Use `additionalUserScopes` only when a human-identity integration flow requires specific GitHub OAuth scope parameters in the authorization URL. Do not rely on it to authorize Junior-owned repository or workflow writes — those use repository-scoped installation tokens and the permissions approved on the GitHub App installation. +Use `additionalUserScopes` only when a human-identity integration flow requires specific GitHub OAuth scope parameters in the authorization URL. Do not rely on it to authorize Junior-owned repository or workflow writes — those use installation tokens and the permissions approved on the GitHub App installation. ## 3) Runtime behavior - When either GitHub skill is active, authenticated `gh` and `git` commands cause the runtime to inject GitHub credentials automatically for the current turn. -- The plugin classifies GitHub traffic from the forwarded HTTP request. Reads use `installation-read`, while `GET /user` uses `user-read`. Allowlisted App-owned mutations and Git smart-HTTP pushes use repository-scoped `installation-write`. User-attachment uploads to `uploads.github.com/user-attachments/assets` use `user-write`. Unknown REST writes and GraphQL mutations are denied. +- The plugin classifies GitHub traffic from the forwarded HTTP request. Reads use `installation-read`, while `GET /user` uses `user-read`. Allowlisted App-owned mutations and Git smart-HTTP pushes use `installation-write`. User-attachment uploads to `uploads.github.com/user-attachments/assets` use `user-write`. Unknown REST writes and GraphQL mutations are denied. - `user-read` and explicitly human `user-write` operations require the actor, or an explicitly delegated user subject, to authorize the GitHub App through the private OAuth flow. Junior-owned issue, pull request, review, inline review comment, and branch operations do not fall back to user OAuth. -- Headless resource-event turns use the `resource-event` system actor and may receive the same repository-scoped installation grants. This lets Junior respond to subscribed pull request events by committing and pushing fixes without inheriting a subscriber's OAuth credential. +- Headless resource-event turns use the `resource-event` system actor and may receive the same installation grants. This lets Junior respond to subscribed pull request events by committing and pushing fixes without inheriting a subscriber's OAuth credential. - Git commits use Junior as author and committer. Resolvable human run actors are credited once with `Co-Authored-By` trailers. -- Issued credentials are reused only within the current turn, credential leases are cached by plugin grant and repository lease scope, and upstream 403 permission denials clear the cached lease before the next retry. +- Installation credential leases are cached on the host by grant name and reused across sandboxes until near expiry. User grants stay actor-scoped. Upstream 403 after injection clears the cached lease, remints once, and retries the hop once before recording permission denied. - Sandbox does not receive raw tokens via env; host applies Authorization header transforms for GitHub API and upload calls. ## 4) CLI usage @@ -188,14 +188,14 @@ The plugin uses installation credentials for read-only GitHub traffic, workflow Committing and pushing code uses more than one GitHub surface: - Creating the local Git commit does not call GitHub. Junior sets the GitHub App bot as author and committer and credits resolvable human actors with `Co-Authored-By` trailers. -- Pushing a branch with Git smart HTTP (`git push`) uses the repository-scoped `installation-write` grant and requires the App installation to have `Contents: write`. Workflow-file changes also require the installation to have `Workflows: write`. +- Pushing a branch with Git smart HTTP (`git push`) uses the `installation-write` grant and requires the App installation to have `Contents: write`. Workflow-file changes also require the installation to have `Workflows: write`. - The smart-HTTP classifier does not distinguish Junior-managed branches or independently detect force updates or ref deletion. Use GitHub branch protection and limit the App installation to repositories where Junior may push. - REST Git database and ref writes are denied by the current write allowlist. Use Git smart HTTP (`git push`) for branch updates instead. - Opening the PR after the branch exists is separate: `github_createPullRequest` needs pull-request write permission, but it should not create or push commits itself. Fork creation is not part of the default PR path and is denied by the current write allowlist. Do not grant `Administration: write` for routine PR creation; push a branch explicitly and create the PR with `github_createPullRequest` instead. -Repository scoping and the egress allowlist are the write boundaries. Credential injection is provider-domain scoped for sandbox traffic to `api.github.com` and `github.com` during turns with a signed credential context. Keep repo context explicit, and let the plugin choose the grant for the outbound request. +The egress allowlist and App installation repositories are the write boundaries. Credential injection is provider-domain scoped for sandbox traffic to `api.github.com` and `github.com` during turns with a signed credential context. Keep repo context explicit, and let the plugin choose the grant for the outbound request. Be careful with mixed-surface PR commands. Use the allowlisted REST endpoints rather than GraphQL-backed `gh pr` mutation commands. PR-native title, body, diff --git a/packages/junior-github/skills/github-code/SKILL.md b/packages/junior-github/skills/github-code/SKILL.md index 6bb5af339e..8085779f1d 100644 --- a/packages/junior-github/skills/github-code/SKILL.md +++ b/packages/junior-github/skills/github-code/SKILL.md @@ -21,7 +21,7 @@ Use `git` and `gh` for repository work. Use `github_createPullRequest`, not `gh - Read applicable `AGENTS.md` files before editing. Narrower repo/task instructions win. - Preserve unrelated work. Never force-push, delete refs, or perform destructive merges. - Base conclusions on repository evidence. Do not claim a check ran unless it did. -- For Junior-owned pull requests, push the branch before creating the PR. The runtime supplies repository-scoped GitHub App credentials for both; try the operations before requesting remediation and never ask for a user token. +- For Junior-owned pull requests, push the branch before creating the PR. The runtime supplies GitHub App installation credentials for both; try the operations before requesting remediation and never ask for a user token. - Use `github_cloneRepository` instead of shelling out to `git clone` when a repository is not already available in the sandbox. - If `github_cloneRepository` returns a tool input error about matching Workspaces, call `switchWorkspace`. The checkout is already present after a successful switch. Pass `allowAdHoc=true` only for an intentional ad-hoc checkout. - A tool-routing denial requires the named tool; only an upstream denial justifies permission remediation. diff --git a/packages/junior-github/skills/github-code/references/api-surface.md b/packages/junior-github/skills/github-code/references/api-surface.md index 90f26856dc..2641ca6fd7 100644 --- a/packages/junior-github/skills/github-code/references/api-surface.md +++ b/packages/junior-github/skills/github-code/references/api-surface.md @@ -72,15 +72,15 @@ jr-rpc config set github.repo owner/repo - Prefer `--json` output for machine-readable parsing where available. - Pass extra `git clone` flags after `--` (e.g. `gh repo clone owner/repo -- --depth=1`). -- A local `git commit` does not call GitHub. Pushing that commit uses Junior's repository-scoped installation credential and requires `github.contents.write` on the target repo. +- A local `git commit` does not call GitHub. Pushing that commit uses Junior's installation credential and requires `github.contents.write` on the target repo. - If the commit changes workflow files under `.github/workflows`, the App installation needs Workflows write in addition to Contents write. - Before rebasing, merge-base analysis, blame/history inspection, or a base comparison, check whether the repository is shallow. Fetch a bounded depth of the base into `refs/remotes/origin/BASE`, deepen incrementally until the needed ancestry is present, and compare against `origin/BASE`; use `--unshallow` only when bounded deepening is insufficient. Never force-push to work around missing ancestry. - Before `github_createPullRequest`, push the head branch explicitly and resolve the target repo's default branch for `base`. That push requires GitHub write access to the remote. - Use `github_updatePullRequest` for title, body, base, or open/closed state changes. Do not raw-`PATCH` `/repos/.../pulls/NUMBER`; that path is denied so Junior can keep the conversation footer. - Merge, fork creation, REST contents/Git database writes, and repository administration are outside the current write allowlist. -- Pull request reviews and inline review comments use the same repository-scoped `installation-write` credential as other bot-owned PR writes, so they post as Junior even on headless turns. Merge remains denied. +- Pull request reviews and inline review comments use the same `installation-write` credential as other bot-owned PR writes, so they post as Junior even on headless turns. Merge remains denied. - Resolve review threads with `github_resolvePullRequestReviewThread`. That tool is the Junior equivalent of `gh api graphql` `resolveReviewThread`; raw GraphQL mutations stay denied, and the tool only succeeds on Junior-authored PRs. - If the explicit `git push` fails with 401/403 or another access/permission error, verify the repo context and retry once. If it still fails, load troubleshooting guidance and report the exact command failure. -- PR comments, labels, and assignees use GitHub's issue endpoints; use the `github-issues` REST guidance for those operations. All allowlisted bot writes share the same repository-scoped `installation-write` credential. +- PR comments, labels, and assignees use GitHub's issue endpoints; use the `github-issues` REST guidance for those operations. All allowlisted bot writes share the same `installation-write` credential. - To embed a local image in a GitHub issue, pull request, review, or comment, call `publishImage` first. That tool returns a durable public URL. The published image is public to anyone on the internet who has the URL. Embed the URL with normal GitHub Markdown. Do not use private Slack file links or conversation attachment URLs. - Return actionable errors for access, permission, not-found, and validation failures. diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 69c01647cf..02a94f0034 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -2,7 +2,7 @@ * GitHub credential issuance and provider request support. * * This module owns OAuth refresh, installation tokens, credential leases, and - * repository-scoped credential parsing. + * installation and user credential parsing. */ import { createPrivateKey, createSign } from "node:crypto"; import type { @@ -87,14 +87,10 @@ interface InstallationCredentialBaseOptions { type InstallationCredentialOptions = InstallationCredentialBaseOptions & ( | { + // Optional downscope. Omit both for the full installation envelope. loadPermissions?: never; permissions?: GitHubAppPermissions; - repositories: string[]; - } - | { - loadPermissions?: never; - permissions: GitHubAppPermissions; - repositories?: never; + repositories?: string[]; } | { loadPermissions: LoadInstallationReadPermissions; @@ -571,26 +567,6 @@ export function githubRepositoryFromUrl( return owner && name ? { owner, name } : undefined; } -/** Build the stable lease scope for a GitHub repository. */ -export function githubRepositoryLeaseScope( - repository: GitHubRepository, -): string { - return `repository:${repository.owner.toLowerCase()}/${repository.name.toLowerCase()}`; -} - -/** Parse the repository bound to an installation-write lease. */ -export function githubRepositoryFromLeaseScope( - leaseScope: string | undefined, -): GitHubRepository { - const match = /^repository:([^/]+)\/([^/]+)$/.exec(leaseScope ?? ""); - if (!match?.[1] || !match[2]) { - throw new GitHubPluginSetupError( - "GitHub installation write grant is missing a repository lease scope.", - ); - } - return { owner: match[1], name: match[2] }; -} - /** Resolve the GitHub account associated with stored user tokens. */ export async function resolveUserAccount( tokens: PluginStoredTokens, @@ -827,11 +803,11 @@ export async function issueInstallationToken( : typeof options.loadPermissions === "function" ? await options.loadPermissions({ appJwt, installationId }) : undefined; + const repositories = + "repositories" in options ? options.repositories : undefined; const body = { ...(permissions ? { permissions } : undefined), - ...("repositories" in options - ? { repositories: options.repositories } - : undefined), + ...(repositories ? { repositories } : undefined), }; const accessTokenResponse = await githubRequest( "https://api.github.com", diff --git a/packages/junior-github/src/egress-policy.ts b/packages/junior-github/src/egress-policy.ts index 4105b6c793..c048b4fb24 100644 --- a/packages/junior-github/src/egress-policy.ts +++ b/packages/junior-github/src/egress-policy.ts @@ -17,7 +17,6 @@ import { HTTP_READ_METHODS, USER_WRITE_REQUIREMENTS, githubRepositoryFromUrl, - githubRepositoryLeaseScope, isRecord, type GitHubGrant, type GitHubGrantName, @@ -395,14 +394,13 @@ function reviewThreadResolveRepository( return repository; } -function repositoryLeaseScopeFromRef(repository: string): string { +function requireRepositoryRef(repository: string): void { const [owner, name] = repository.split("/"); if (!owner || !name) { throw new EgressPolicyDenied( "GitHub review thread resolution does not identify a target repository.", ); } - return githubRepositoryLeaseScope({ owner, name }); } function isGitHubGraphqlMutation( @@ -463,25 +461,22 @@ function grantForAccess( access: PluginGrantAccess, reason: GitHubGrantReason, name: GitHubGrantName, - leaseScope?: string, ): GitHubGrant { return { name, access, - ...(leaseScope ? { leaseScope } : undefined), reason, ...(name === "user-write" ? { requirements: USER_WRITE_REQUIREMENTS } : undefined), }; } -function repositoryLeaseScope(upstreamUrl: URL): string { - const repository = githubRepositoryFromUrl(upstreamUrl); - if (!repository) { - throw new EgressPolicyDenied( - "GitHub write request does not identify a target repository.", - ); +function requireRepositoryTarget(upstreamUrl: URL): void { + if (githubRepositoryFromUrl(upstreamUrl)) { + return; } - return githubRepositoryLeaseScope(repository); + throw new EgressPolicyDenied( + "GitHub write request does not identify a target repository.", + ); } export async function githubGrantForEgress( @@ -504,11 +499,11 @@ export async function githubGrantForEgress( const smartHttpAccess = githubSmartHttpAccess(upstreamUrl); if (smartHttpAccess) { if (smartHttpAccess === "write") { + requireRepositoryTarget(upstreamUrl); return grantForAccess( "write", "github.installation-write", "installation-write", - repositoryLeaseScope(upstreamUrl), ); } return grantForAccess( @@ -525,13 +520,15 @@ export async function githubGrantForEgress( const writeGrantName = githubApiWriteGrantName(method, upstreamUrl); if (writeGrantName) { + if (writeGrantName === "installation-write") { + requireRepositoryTarget(upstreamUrl); + } return grantForAccess( "write", writeGrantName === "user-write" ? "github.user-write" : "github.installation-write", writeGrantName, - repositoryLeaseScope(upstreamUrl), ); } @@ -542,11 +539,11 @@ export async function githubGrantForEgress( ctx.request.bodyText, ); if (reviewThreadRepository) { + requireRepositoryRef(reviewThreadRepository); return grantForAccess( "write", "github.installation-write", "installation-write", - repositoryLeaseScopeFromRef(reviewThreadRepository), ); } diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index dc7204e369..0f708b4dc8 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -63,7 +63,6 @@ import { GitHubPluginSetupError, createPermissionCache, credentialUnavailable, - githubRepositoryFromLeaseScope, githubRequest, issueInstallationCredential, issueInstallationToken, @@ -383,16 +382,12 @@ export function githubPlugin( }); } if (ctx.grant.name === "installation-write") { - const repository = githubRepositoryFromLeaseScope( - ctx.grant.leaseScope, - ); + // Installation write uses the full installed App envelope. Repo + // allowlisting stays in egress policy, not in per-hop token minting. return await issueInstallationCredential({ appIdEnv, privateKeyEnv, installationIdEnv, - // This repository-only variant cannot downscope the installed - // App envelope with an operation-specific permission body. - repositories: [repository.name], }); } if (USER_TOKEN_GRANTS.has(ctx.grant.name)) { diff --git a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts index dfb34cb546..c375c43527 100644 --- a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts +++ b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts @@ -18,7 +18,7 @@ import { botUserIdFromEmail } from "../webhooks/ownership.js"; * -F id=THREAD_ID * ``` * - * `repo` is required so Junior can issue a repository-scoped installation + * `repo` is required so Junior can bind the GraphQL operation to a repository * credential; GraphQL has no repo path to derive that from. */ const inputSchema = z @@ -26,7 +26,7 @@ const inputSchema = z repo: z .string() .describe( - 'Repository in "owner/name" format. Required for repository-scoped credentials (GraphQL has no repo path).', + 'Repository in "owner/name" format. Required because GraphQL has no repo path.', ), threadId: z .string() diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index 4b895a197a..cc1be59601 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -419,7 +419,6 @@ function githubIssueCredentialContext(input: { }; grant: { access: "read" | "write"; - leaseScope?: string; name: string; reason?: string; }; @@ -642,7 +641,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -654,7 +652,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -666,7 +663,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -678,7 +674,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -724,7 +719,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -735,7 +729,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -746,7 +739,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -757,7 +749,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -768,7 +759,6 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); }); @@ -1807,7 +1797,6 @@ Conversation: \`local:test:old-conversation\` ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); expect( @@ -1818,12 +1807,11 @@ Conversation: \`local:test:old-conversation\` ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); }); - it("selects repository-scoped installation identity for Git push discovery", async () => { + it("selects installation identity for Git push discovery", async () => { expect( await grantForEgress({ method: "GET", @@ -1832,7 +1820,6 @@ Conversation: \`local:test:old-conversation\` ).toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); }); @@ -1971,7 +1958,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2120,7 +2106,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2132,7 +2117,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2144,7 +2128,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2155,7 +2138,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2166,7 +2148,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2177,7 +2158,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2188,7 +2168,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2199,7 +2178,6 @@ Conversation: \`local:test:old-conversation\` ).resolves.toMatchObject({ name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }); await expect( @@ -2253,7 +2231,7 @@ Conversation: \`local:test:old-conversation\` ); }); - it("preserves installed App permissions on repository-scoped write credentials", async () => { + it("issues full-installation write credentials without repository downscope", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); @@ -2272,7 +2250,6 @@ Conversation: \`local:test:old-conversation\` grant: { name: "installation-write", access: "write", - leaseScope: "repository:getsentry/junior", reason: "github.installation-write", }, db, @@ -2286,9 +2263,7 @@ Conversation: \`local:test:old-conversation\` expect(requests[0]).toEqual({ url: "https://api.github.com/app/installations/456/access_tokens", method: "POST", - body: { - repositories: ["junior"], - }, + body: {}, headers: expect.any(Object), }); }); diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index 77227d4df2..f92e7cb3c4 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -1721,7 +1721,7 @@ describe("GitHub-owned pull request outcomes", () => { } }); - it("classifies commits through repository-scoped production plugin wiring", async () => { + it("classifies commits through production plugin wiring", async () => { const fixture = await createGitHubFixture(); const tokenBodies: unknown[] = []; const commitRequests: Request[] = []; diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index 78c129d692..bd95741d05 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -648,56 +648,69 @@ export async function executeCredentialedEgressRequest(input: { const recordPermissionDenied = deps.recordPermissionDenied ?? recordSandboxPermissionDenied; - let lease: SandboxEgressCredentialLease; - try { - lease = await issueCredentialLease( - provider, - grantSelection, - credentialContext, - ); - } catch (error) { - if (error instanceof SandboxEgressCredentialError) { - await recordAuthRequired({ + const resolveLease = async (): Promise< + SandboxEgressCredentialLease | Response + > => { + try { + return await issueCredentialLease( + provider, + grantSelection, credentialContext, - provider: error.provider, - grant: error.grant, - kind: error.kind, - authorization: error.authorization, - message: error.message, - }); - const isAuthRequired = error.kind === "auth_required"; - logWarn( - isAuthRequired - ? "sandbox.egress.credential.needed" - : "sandbox.egress.credential.unavailable", - { - ...egressAttributes({ - egressId: activeEgressId, - grantAccess: error.grant.access, - grantName: error.grant.name, - grantReason: error.grant.reason, - host: upstreamUrl.hostname, - method: request.method, - path: upstreamUrl.pathname, - provider: error.provider, - status: 401, - }), - ...routingAttributes(request, upstreamUrl), - }, ); - return authRequiredResponse({ - provider: error.provider, - grant: error.grant, - message: error.message, - }); + } catch (error) { + if (error instanceof SandboxEgressCredentialError) { + await recordAuthRequired({ + credentialContext, + provider: error.provider, + grant: error.grant, + kind: error.kind, + authorization: error.authorization, + message: error.message, + }); + const isAuthRequired = error.kind === "auth_required"; + logWarn( + isAuthRequired + ? "sandbox.egress.credential.needed" + : "sandbox.egress.credential.unavailable", + { + ...egressAttributes({ + egressId: activeEgressId, + grantAccess: error.grant.access, + grantName: error.grant.name, + grantReason: error.grant.reason, + host: upstreamUrl.hostname, + method: request.method, + path: upstreamUrl.pathname, + provider: error.provider, + status: 401, + }), + ...routingAttributes(request, upstreamUrl), + }, + ); + return authRequiredResponse({ + provider: error.provider, + grant: error.grant, + message: error.message, + }); + } + throw error; } - throw error; + }; + + let leaseOrResponse = await resolveLease(); + if (leaseOrResponse instanceof Response) { + return leaseOrResponse; } + let lease = leaseOrResponse; - const attributes = (status: number, upstream?: Response) => + const attributes = ( + activeLease: SandboxEgressCredentialLease, + status: number, + upstream?: Response, + ) => leaseLogAttributes({ egressId: activeEgressId, - lease, + lease: activeLease, provider, request, status, @@ -707,7 +720,7 @@ export async function executeCredentialedEgressRequest(input: { if (!hasSandboxEgressLeaseTransformForHost(lease, upstreamUrl.hostname)) { logWarn("sandbox.egress.transform.missing", { - ...attributes(403), + ...attributes(lease, 403), "app.sandbox.egress.transform_domains": lease.headerTransforms.map( (transform) => transform.domain, ), @@ -719,111 +732,100 @@ export async function executeCredentialedEgressRequest(input: { } const fetchImpl = deps.fetch ?? fetch; - const headers = requestHeaders( - request, - lease, - upstreamUrl.hostname, - deps.tracePropagation ?? {}, - ); const body = bodyForGrantSelection ?? (await requestBodyBytes(request)); - const intercepted = await deps.interceptHttp?.({ - provider, - request: new Request(upstreamUrl, { - method: request.method, - headers, - ...(body !== undefined ? { body } : undefined), - }), - upstreamUrl, - }); - if (intercepted) { - return intercepted; - } + // One remint retry for upstream 403 after credential injection. Intermittent + // provider denials (for example GitHub git receive-pack) should not fail the + // command before Junior replaces the cached lease and tries once more. + const maxAttempts = 2; - const upstream = await fetchImpl(upstreamUrl, { - method: request.method, - headers, - ...(body !== undefined ? { body } : undefined), - redirect: "manual", - }); - try { - const effects = await onPluginEgressResponse({ + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + const headers = requestHeaders( + request, + lease, + upstreamUrl.hostname, + deps.tracePropagation ?? {}, + ); + const intercepted = await deps.interceptHttp?.({ provider, - grant: lease.grant, - method: request.method, - ...(operation ? { operation } : undefined), + request: new Request(upstreamUrl, { + method: request.method, + headers, + ...(body !== undefined ? { body } : undefined), + }), upstreamUrl, - response: { - headers: new Headers(upstream.headers), - readText: async (maxBytes) => - await responseTextWithinLimit(upstream, maxBytes), - status: upstream.status, - }, }); - if (effects.permissionDenied) { - await recordPermissionDenied({ - credentialContext, + if (intercepted) { + return intercepted; + } + + const requestBody = + body instanceof ArrayBuffer ? body.slice(0) : body; + const upstream = await fetchImpl(upstreamUrl, { + method: request.method, + headers, + ...(requestBody !== undefined ? { body: requestBody } : undefined), + redirect: "manual", + }); + let pluginPermissionDenied: { message: string } | undefined; + try { + const effects = await onPluginEgressResponse({ provider, - lease, - message: effects.permissionDenied.message, - upstream, + grant: lease.grant, + method: request.method, + ...(operation ? { operation } : undefined), upstreamUrl, + response: { + headers: new Headers(upstream.headers), + readText: async (maxBytes) => + await responseTextWithinLimit(upstream, maxBytes), + status: upstream.status, + }, }); - logWarn("sandbox.egress.upstream_permission.classified", { - ...attributes(upstream.status, upstream), + pluginPermissionDenied = effects.permissionDenied; + } catch (error) { + if (!isEgressAuthRequired(error)) { + throw error; + } + await clearCredentialLease(provider, lease.grant, credentialContext); + await recordAuthRequired({ + credentialContext, + provider, + grant: lease.grant, + authorization: error.authorization ?? lease.authorization, + message: error.message, + }); + logWarn("sandbox.egress.upstream_auth_requirement.classified", { + ...attributes(lease, upstream.status, upstream), + }); + await upstream.body?.cancel().catch(() => undefined); + return authRequiredResponse({ + provider, + grant: lease.grant, + message: error.message, }); } - } catch (error) { - if (!isEgressAuthRequired(error)) { - throw error; - } - await clearCredentialLease(provider, lease.grant, credentialContext); - await recordAuthRequired({ - credentialContext, - provider, - grant: lease.grant, - authorization: error.authorization ?? lease.authorization, - message: error.message, - }); - logWarn("sandbox.egress.upstream_auth_requirement.classified", { - ...attributes(upstream.status, upstream), - }); - await upstream.body?.cancel().catch(() => undefined); - return authRequiredResponse({ + logSandboxEgressUpstreamRequest({ + egressId: activeEgressId, + grantAccess: lease.grant.access, + grantName: lease.grant.name, + grantReason: lease.grant.reason, provider, - grant: lease.grant, - message: error.message, - }); - } - logSandboxEgressUpstreamRequest({ - egressId: activeEgressId, - grantAccess: lease.grant.access, - grantName: lease.grant.name, - grantReason: lease.grant.reason, - provider, - request, - upstream, - upstreamUrl, - }); - if (upstream.status >= 400) { - logWarn("sandbox.egress.upstream_response.failed", { - ...attributes(upstream.status, upstream), - "error.type": `http_${upstream.status}`, - }); - } - if ( - upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS || - upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS - ) { - logWarn("sandbox.egress.upstream_auth.rejected", { - ...attributes(upstream.status, upstream), - ...(upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS - ? { - "app.sandbox.egress.www_authenticate": - upstream.headers.get("www-authenticate") ?? undefined, - } - : undefined), + request, + upstream, + upstreamUrl, }); + if (upstream.status >= 400) { + logWarn("sandbox.egress.upstream_response.failed", { + ...attributes(lease, upstream.status, upstream), + "error.type": `http_${upstream.status}`, + }); + } if (upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS) { + logWarn("sandbox.egress.upstream_auth.rejected", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.www_authenticate": + upstream.headers.get("www-authenticate") ?? undefined, + }); await clearCredentialLease(provider, lease.grant, credentialContext); await recordAuthRequired({ credentialContext, @@ -838,22 +840,80 @@ export async function executeCredentialedEgressRequest(input: { grant: lease.grant, message: `Provider rejected the injected ${provider} credential.\n`, }); - } else { + } + if (upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS) { + logWarn("sandbox.egress.upstream_auth.rejected", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.auth_attempt": attempt, + }); await clearCredentialLease(provider, lease.grant, credentialContext); + if (attempt < maxAttempts) { + await upstream.body?.cancel().catch(() => undefined); + logWarn("sandbox.egress.upstream_auth.retrying", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.auth_attempt": attempt, + }); + leaseOrResponse = await resolveLease(); + if (leaseOrResponse instanceof Response) { + return leaseOrResponse; + } + lease = leaseOrResponse; + if (!hasSandboxEgressLeaseTransformForHost(lease, upstreamUrl.hostname)) { + logWarn("sandbox.egress.transform.missing", { + ...attributes(lease, 403), + "app.sandbox.egress.transform_domains": lease.headerTransforms.map( + (transform) => transform.domain, + ), + }); + return Response.json( + { error: "Credential lease does not cover forwarded host" }, + { status: 403 }, + ); + } + continue; + } + await recordPermissionDenied({ + credentialContext, + provider, + lease, + message: + pluginPermissionDenied?.message ?? + permissionDeniedMessage(provider, lease.grant), + upstream, + upstreamUrl, + }); + if (pluginPermissionDenied) { + logWarn("sandbox.egress.upstream_permission.classified", { + ...attributes(lease, upstream.status, upstream), + }); + } + return new Response(upstream.body, { + status: upstream.status, + statusText: upstream.statusText, + headers: responseHeaders(upstream), + }); + } + + if (pluginPermissionDenied) { await recordPermissionDenied({ credentialContext, provider, lease, - message: permissionDeniedMessage(provider, lease.grant), + message: pluginPermissionDenied.message, upstream, upstreamUrl, }); + logWarn("sandbox.egress.upstream_permission.classified", { + ...attributes(lease, upstream.status, upstream), + }); } + + return new Response(upstream.body, { + status: upstream.status, + statusText: upstream.statusText, + headers: responseHeaders(upstream), + }); } - return new Response(upstream.body, { - status: upstream.status, - statusText: upstream.statusText, - headers: responseHeaders(upstream), - }); + throw new Error("Credentialed egress exhausted auth attempts without a response"); } diff --git a/packages/junior/src/chat/sandbox/egress/credentials.ts b/packages/junior/src/chat/sandbox/egress/credentials.ts index c30b3100ab..cc173cf06d 100644 --- a/packages/junior/src/chat/sandbox/egress/credentials.ts +++ b/packages/junior/src/chat/sandbox/egress/credentials.ts @@ -178,9 +178,9 @@ export function authorizationForSandboxEgressGrant( /** * Return cached or newly issued credential header transforms for a selected grant. * - * Leases are cached per actor/context/grant, validated against provider-owned - * domains, and reused only while both the provider lease and sandbox context are - * still valid. + * Leases are cached on the host by provider grant (and actor for user grants), + * validated against provider-owned domains, and reused until the provider lease + * is near expiry. Sandbox context only authorizes the hop. */ export async function sandboxEgressCredentialLease( provider: string, diff --git a/packages/junior/src/chat/sandbox/egress/session.ts b/packages/junior/src/chat/sandbox/egress/session.ts index 6fbd1db9b9..a484e10213 100644 --- a/packages/junior/src/chat/sandbox/egress/session.ts +++ b/packages/junior/src/chat/sandbox/egress/session.ts @@ -17,10 +17,11 @@ import { getStateAdapter } from "@/chat/state/adapter"; // // The sandbox gets a signed context token in its network policy URL; the proxy // verifies that token on every forwarded request. Credential leases are cached -// per actor, grant, VM id, and token id so repeated provider calls in the same -// command do not reissue credentials. Auth-required and permission-denied -// signals are written here so the sandbox command runner can translate host -// egress failures into the same user-facing auth flow as direct tool calls. +// on the host by provider grant (and actor only for user-owned grants) so the +// same installation token can be reused across sandboxes until expiry. Auth- +// required and permission-denied signals are written here so the sandbox +// command runner can translate host egress failures into the same user-facing +// auth flow as direct tool calls. export const SANDBOX_EGRESS_PROXY_PATH = "/api/internal/sandbox-egress"; @@ -31,6 +32,8 @@ const SANDBOX_EGRESS_PERMISSION_SIGNAL_PREFIX = "sandbox-egress-permission-denied"; const SANDBOX_EGRESS_LEASE_PREFIX = "sandbox-egress-lease"; const DEFAULT_SESSION_TTL_MS = 30 * 60 * 1000; +/** Treat a cached lease as expired this long before its provider expiry. */ +const LEASE_REFRESH_BUFFER_MS = 5 * 60 * 1000; export type { SandboxEgressAuthRequiredSignal, @@ -40,7 +43,12 @@ export type { }; /** - * Build the lease cache key for one provider grant, actor, sandbox VM, and token. + * Build the host lease cache key for one provider grant. + * + * Installation/bot grants are shared across sandboxes. User grants stay bound + * to the actor so one human's OAuth token cannot be reused for another. + * Sandbox egress id and context token id are not part of the key: those only + * authorize the hop; they do not change which host credential to inject. */ function leaseKey( provider: string, @@ -48,12 +56,15 @@ function leaseKey( context: SandboxEgressCredentialContext, ): string { const actor = context.credentials.actor; - const actorKey = - "type" in actor ? `user:${actor.userId}` : `system:${actor.name}`; - const grantKey = grant.leaseScope - ? `${grant.name}:${grant.leaseScope}` - : grant.name; - return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grantKey}:${actorKey}:${context.egressId}:${context.contextId}`; + // Only installation/bot grants are host-shared. User and broker-default + // grants stay actor-bound so one human's token cannot serve another. + const isSharedInstallationGrant = grant.name.startsWith("installation-"); + const actorKey = isSharedInstallationGrant + ? "shared" + : "type" in actor + ? `user:${actor.userId}` + : `system:${actor.name}`; + return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:${actorKey}`; } /** @@ -124,7 +135,12 @@ function parseLease(value: unknown): SandboxEgressCredentialLease | undefined { return undefined; } const expiresAtMs = Date.parse(result.data.expiresAt); - if (!Number.isFinite(expiresAtMs) || expiresAtMs <= Date.now()) { + // Refresh before hard expiry so a hop near the edge does not send a token + // that dies between cache read and upstream use. + if ( + !Number.isFinite(expiresAtMs) || + expiresAtMs <= Date.now() + LEASE_REFRESH_BUFFER_MS + ) { return undefined; } return result.data; @@ -197,10 +213,11 @@ export function parseSandboxEgressCredentialToken( } /** - * Cache credential header transforms for one actor, VM, context token, and grant. + * Cache credential header transforms for one host-owned provider grant. * - * The cache TTL is capped by both the provider lease expiry and the signed - * context expiry so a stale sandbox URL cannot keep using old credentials. + * TTL follows the provider lease expiry. Sandbox context expiry still bounds + * whether a hop may use the cache; it does not shorten a shared installation + * lease for every other sandbox. */ export async function setSandboxEgressCredentialLease( context: SandboxEgressCredentialContext, @@ -210,17 +227,14 @@ export async function setSandboxEgressCredentialLease( if (!Number.isFinite(leaseExpiresAtMs) || leaseExpiresAtMs <= Date.now()) { return; } - const ttlMs = Math.max( - 1, - Math.min(leaseExpiresAtMs, context.expiresAtMs) - Date.now(), - ); + const ttlMs = Math.max(1, leaseExpiresAtMs - Date.now()); const state = getStateAdapter(); await state.connect(); await state.set(leaseKey(lease.provider, lease.grant, context), lease, ttlMs); } /** - * Load cached credential header transforms for the exact actor/context/grant. + * Load cached credential header transforms for the host-owned provider grant. */ export async function getSandboxEgressCredentialLease( provider: string, diff --git a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts index fcbd7ad4e6..fb37a75fef 100644 --- a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts @@ -178,7 +178,7 @@ function mockSentryLease(domain = "sentry.io", token = "sentry-token"): void { headers: { Authorization: `Bearer ${token}` }, }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }); } @@ -627,7 +627,7 @@ describe("sandbox egress proxy composition", () => { headers: { Authorization: "Bearer token-u123" }, }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }) .mockResolvedValueOnce({ id: "lease-2", @@ -639,7 +639,7 @@ describe("sandbox egress proxy composition", () => { headers: { Authorization: "Bearer token-u456" }, }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }); const fetchMock = vi.fn(async (_url: URL | string, init?: RequestInit) => { @@ -674,7 +674,7 @@ describe("sandbox egress proxy composition", () => { }); }); - it("does not reuse cached credential leases across renewed credential contexts", async () => { + it("reuses host-cached credential leases across renewed credential contexts for the same actor", async () => { setSandboxEgressUserActor(); issueProviderCredentialLeaseMock .mockResolvedValueOnce({ @@ -687,7 +687,7 @@ describe("sandbox egress proxy composition", () => { headers: { Authorization: "Bearer token-first-session" }, }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }) .mockResolvedValueOnce({ id: "lease-2", @@ -699,7 +699,7 @@ describe("sandbox egress proxy composition", () => { headers: { Authorization: "Bearer token-second-session" }, }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }); const fetchMock = vi.fn(async (_url: URL | string, init?: RequestInit) => { @@ -714,68 +714,85 @@ describe("sandbox egress proxy composition", () => { "Bearer token-first-session", ); + // New signed context/session for the same actor should reuse the host lease. setSandboxEgressUserActor(); const secondResponse = await proxy( egressRequest({ path: "/api/0/issues/2" }), fetchMock as typeof fetch, ); await expect(secondResponse.text()).resolves.toBe( - "Bearer token-second-session", + "Bearer token-first-session", ); - expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); + expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(1); }); - it("passes through upstream 403 responses without overriding the body", async () => { + it("remints once on upstream 403 and recovers or records permission denied", async () => { setSandboxEgressUserActor(); - issueProviderCredentialLeaseMock.mockResolvedValue({ - id: "lease-1", + const lease = (token: string) => ({ + id: `lease-${token}`, provider: "sentry", env: { SENTRY_AUTH_TOKEN: "host_managed_credential" }, headerTransforms: [ - { domain: "sentry.io", headers: { Authorization: "Bearer token" } }, + { domain: "sentry.io", headers: { Authorization: `Bearer ${token}` } }, ], - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), }); - - const fetchMock = vi.fn().mockImplementation( - async () => - new Response("Permission denied for this organization", { - status: 403, - }), - ); - - const response = await proxy( + issueProviderCredentialLeaseMock + .mockResolvedValueOnce(lease("token-1")) + .mockResolvedValueOnce(lease("token-2")) + .mockResolvedValueOnce(lease("token-3")) + .mockResolvedValueOnce(lease("token-4")); + const denied = () => + new Response("Permission denied for this organization", { status: 403 }); + const fetchMock = vi + .fn() + .mockResolvedValueOnce(denied()) + .mockResolvedValueOnce(new Response("ok", { status: 200 })) + .mockResolvedValueOnce(denied()) + .mockResolvedValueOnce(denied()); + + const recovered = await proxy( egressRequest({ path: "/api/0/issues/1" }), fetchMock as typeof fetch, ); + expect(recovered.status).toBe(200); + await expect(recovered.text()).resolves.toBe("ok"); + expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect( + new Headers(fetchMock.mock.calls[0]?.[1]?.headers).get("authorization"), + ).toBe("Bearer token-1"); + expect( + new Headers(fetchMock.mock.calls[1]?.[1]?.headers).get("authorization"), + ).toBe("Bearer token-2"); + await expect( + consumeSandboxEgressPermissionDeniedSignal(EGRESS_ID), + ).resolves.toBeUndefined(); - expect(response.status).toBe(403); - const body = await response.text(); + const persistent = await proxy( + egressRequest({ path: "/api/0/issues/2" }), + fetchMock as typeof fetch, + ); + expect(persistent.status).toBe(403); + const body = await persistent.text(); expect(body).toBe("Permission denied for this organization"); expect(body).not.toContain("junior-auth-required"); + // Second hop reuses the recovered lease, then remints once after 403. + expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(3); + expect(fetchMock).toHaveBeenCalledTimes(4); await expect( consumeSandboxEgressPermissionDeniedSignal(EGRESS_ID), ).resolves.toMatchObject({ provider: "sentry", - grant: { - name: "default", - access: "read", - }, + grant: { name: "default", access: "read" }, message: "sentry returned HTTP 403 after Junior injected the default grant. Junior forwarded the request; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "sentry.io", - upstreamPath: "/api/0/issues/1", + upstreamPath: "/api/0/issues/2", }); - - const secondResponse = await proxy( - egressRequest({ path: "/api/0/issues/2" }), - fetchMock as typeof fetch, - ); - expect(secondResponse.status).toBe(403); - expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); }); it("does not apply subdomain transforms to the apex host", async () => { diff --git a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts index c9f6b1eca2..26e28f61a4 100644 --- a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts @@ -1002,7 +1002,7 @@ describe("sandbox egress proxy integration", () => { expect(upstreamFetch).toHaveBeenCalledTimes(1); }); - it("uses repository-scoped GitHub App credentials for workflow dispatch", async () => { + it("uses GitHub App installation credentials for workflow dispatch", async () => { configureGitHubAppEnv(); const tokenRequests = mockGitHubInstallationToken(); await registerGitHubPlugin({ @@ -1049,11 +1049,7 @@ describe("sandbox egress proxy integration", () => { expect(response.status).toBe(204); expect(upstreamFetch).toHaveBeenCalledTimes(1); - expect(tokenRequests).toEqual([ - { - repositories: ["junior"], - }, - ]); + expect(tokenRequests).toEqual([{}]); }); it("records GitHub GraphQL repository access errors without rewriting the response", async () => { diff --git a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts index e3bb1ce988..9c5495d264 100644 --- a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts +++ b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts @@ -192,7 +192,7 @@ describe("sandboxEgressCredentialLease — credential error normalization", () = ); }); - it("isolates cached plugin leases by opaque lease scope", async () => { + it("reuses host-shared installation leases across contexts and scopes", async () => { hasEgressCredentialHooks.mockReturnValue(true); issuePluginCredential.mockClear(); const state = new Map(); @@ -206,7 +206,7 @@ describe("sandboxEgressCredentialLease — credential error normalization", () = issuePluginCredential.mockResolvedValue({ type: "lease", lease: { - expiresAt: new Date(Date.now() + 60_000).toISOString(), + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), headerTransforms: [ { domain: "sentry.io", @@ -219,31 +219,69 @@ describe("sandboxEgressCredentialLease — credential error normalization", () = grant: { name: "installation-write", access: "write" as const, - leaseScope: "repository:getsentry/junior", }, source: "plugin" as const, }; - const second = { + const secondContext = { + ...credentialContext(), + egressId: "other-egress", + contextId: "ctx-other", + }; + + await sandboxEgressCredentialLease(PROVIDER, first, credentialContext()); + await sandboxEgressCredentialLease(PROVIDER, first, secondContext); + await sandboxEgressCredentialLease(PROVIDER, first, credentialContext()); + + expect(issuePluginCredential).toHaveBeenCalledTimes(1); + expect(stateStub.set.mock.calls.map(([key]) => key)).toEqual([ + "sandbox-egress-lease:sentry:installation-write:shared", + ]); + }); + + it("keeps user grants isolated by actor", async () => { + hasEgressCredentialHooks.mockReturnValue(true); + issuePluginCredential.mockClear(); + const state = new Map(); + const stateStub = { + connect: vi.fn(), + get: vi.fn((key: string) => state.get(key)), + set: vi.fn((key: string, value: unknown) => state.set(key, value)), + delete: vi.fn((key: string) => state.delete(key)), + }; + getStateAdapter.mockReturnValue(stateStub); + issuePluginCredential.mockResolvedValue({ + type: "lease", + lease: { + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), + headerTransforms: [ + { + domain: "sentry.io", + headers: { Authorization: "Bearer user-token" }, + }, + ], + }, + }); + const grant = { grant: { - name: "installation-write", + name: "user-write", access: "write" as const, - leaseScope: "repository:getsentry/sentry", }, source: "plugin" as const, }; + const otherActor = { + credentials: { actor: { type: "user" as const, userId: "U999" } }, + egressId: EGRESS_ID, + expiresAtMs: Date.now() + 60_000, + contextId: "ctx-other-user", + }; - await sandboxEgressCredentialLease(PROVIDER, first, credentialContext()); - await sandboxEgressCredentialLease(PROVIDER, second, credentialContext()); - await sandboxEgressCredentialLease(PROVIDER, first, credentialContext()); + await sandboxEgressCredentialLease(PROVIDER, grant, credentialContext()); + await sandboxEgressCredentialLease(PROVIDER, grant, otherActor); expect(issuePluginCredential).toHaveBeenCalledTimes(2); expect(stateStub.set.mock.calls.map(([key]) => key)).toEqual([ - expect.stringContaining( - ":installation-write:repository:getsentry/junior:", - ), - expect.stringContaining( - ":installation-write:repository:getsentry/sentry:", - ), + "sandbox-egress-lease:sentry:user-write:user:U123", + "sandbox-egress-lease:sentry:user-write:user:U999", ]); }); }); From 9eddddac41dddb5c6edb5c55000b7d6ee453dc69 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:30:13 +0000 Subject: [PATCH 2/8] ref(egress): clean jargon in credential simplification Rewrite comments, docs, and test titles in plain language. Drop leftover repository-scoped wording and other internal slang from the simplification. Co-Authored-By: David Cramer --- packages/junior-github/SETUP.md | 4 +-- .../junior-github/src/credential-support.ts | 2 +- packages/junior-github/src/plugin.ts | 4 +-- .../resolve-pull-request-review-thread.ts | 4 +-- .../junior-github/tests/github-plugin.test.ts | 2 +- .../junior/src/chat/egress/credentialed.ts | 5 ++- .../src/chat/sandbox/egress/credentials.ts | 6 ++-- .../junior/src/chat/sandbox/egress/session.ts | 32 ++++++++----------- .../handlers/sandbox-egress-proxy.test.ts | 6 ++-- .../sandbox-egress-credentials.test.ts | 2 +- 10 files changed, 30 insertions(+), 37 deletions(-) diff --git a/packages/junior-github/SETUP.md b/packages/junior-github/SETUP.md index 37f5508715..8451919563 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -138,7 +138,7 @@ githubPlugin({ Installation-read token requests remain read-only by requesting read-capable configured permissions at `read` level and omitting GitHub permission fields that have no `read` value. Installation-write token requests intentionally omit the `permissions` field, so GitHub applies the complete permission envelope approved on the App installation. GitHub remains the source of truth for whether a permission name or level exists. -GitHub App user-to-server tokens do not use OAuth scopes as their permission model. Their effective access is limited by the GitHub App's installed permissions, the app installation's repository access, and the requesting user's own GitHub access. Repository-scoped installation tokens instead use the App permission envelope and installation repository access without borrowing the requesting user's authority. GitHub returns an empty `scope` value for user-to-server tokens, so Junior cannot verify granted scopes from the token response. +GitHub App user-to-server tokens do not use OAuth scopes as their permission model. Their effective access is limited by the GitHub App's installed permissions, the app installation's repository access, and the requesting user's own GitHub access. Installation tokens use the App permissions and installation repository access without borrowing the requesting user's authority. GitHub returns an empty `scope` value for user-to-server tokens, so Junior cannot verify granted scopes from the token response. If you pass `additionalUserScopes`, Junior includes those values in the authorization URL and records the requested scope string as a local reauthorization contract. This does not expand or prove GitHub API permissions. Configure provider-enforced access in the GitHub App settings; `appPermissions` only controls read-token downscoping: @@ -157,7 +157,7 @@ Use `additionalUserScopes` only when a human-identity integration flow requires - `user-read` and explicitly human `user-write` operations require the actor, or an explicitly delegated user subject, to authorize the GitHub App through the private OAuth flow. Junior-owned issue, pull request, review, inline review comment, and branch operations do not fall back to user OAuth. - Headless resource-event turns use the `resource-event` system actor and may receive the same installation grants. This lets Junior respond to subscribed pull request events by committing and pushing fixes without inheriting a subscriber's OAuth credential. - Git commits use Junior as author and committer. Resolvable human run actors are credited once with `Co-Authored-By` trailers. -- Installation credential leases are cached on the host by grant name and reused across sandboxes until near expiry. User grants stay actor-scoped. Upstream 403 after injection clears the cached lease, remints once, and retries the hop once before recording permission denied. +- Installation credential leases are cached on the host by grant name and reused across sandboxes until near expiry. User grants stay actor-scoped. Upstream 403 after injection clears the cached lease, issues a new token, and retries the hop once before recording permission denied. - Sandbox does not receive raw tokens via env; host applies Authorization header transforms for GitHub API and upload calls. ## 4) CLI usage diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 02a94f0034..5333425bdd 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -87,7 +87,7 @@ interface InstallationCredentialBaseOptions { type InstallationCredentialOptions = InstallationCredentialBaseOptions & ( | { - // Optional downscope. Omit both for the full installation envelope. + // Omit both to use the full installed App permissions and repos. loadPermissions?: never; permissions?: GitHubAppPermissions; repositories?: string[]; diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 0f708b4dc8..5cfc3de5a5 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -382,8 +382,8 @@ export function githubPlugin( }); } if (ctx.grant.name === "installation-write") { - // Installation write uses the full installed App envelope. Repo - // allowlisting stays in egress policy, not in per-hop token minting. + // Use the full installed App permissions. Repo allowlisting stays in + // egress policy, not in per-request token minting. return await issueInstallationCredential({ appIdEnv, privateKeyEnv, diff --git a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts index c375c43527..b95e2f610e 100644 --- a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts +++ b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts @@ -18,8 +18,8 @@ import { botUserIdFromEmail } from "../webhooks/ownership.js"; * -F id=THREAD_ID * ``` * - * `repo` is required so Junior can bind the GraphQL operation to a repository - * credential; GraphQL has no repo path to derive that from. + * `repo` is required so Junior can bind the GraphQL operation to a repository; + * GraphQL has no repo path to derive that from. */ const inputSchema = z .object({ diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index cc1be59601..e1160b8d6e 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -2231,7 +2231,7 @@ Conversation: \`local:test:old-conversation\` ); }); - it("issues full-installation write credentials without repository downscope", async () => { + it("issues installation-write credentials without repository filter", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index bd95741d05..0b1cfe3708 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -733,9 +733,8 @@ export async function executeCredentialedEgressRequest(input: { const fetchImpl = deps.fetch ?? fetch; const body = bodyForGrantSelection ?? (await requestBodyBytes(request)); - // One remint retry for upstream 403 after credential injection. Intermittent - // provider denials (for example GitHub git receive-pack) should not fail the - // command before Junior replaces the cached lease and tries once more. + // Retry once on upstream 403 after credential injection. Replace the cached + // lease first so a single intermittent provider denial does not fail the hop. const maxAttempts = 2; for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { diff --git a/packages/junior/src/chat/sandbox/egress/credentials.ts b/packages/junior/src/chat/sandbox/egress/credentials.ts index cc173cf06d..1dc71bb41a 100644 --- a/packages/junior/src/chat/sandbox/egress/credentials.ts +++ b/packages/junior/src/chat/sandbox/egress/credentials.ts @@ -178,9 +178,9 @@ export function authorizationForSandboxEgressGrant( /** * Return cached or newly issued credential header transforms for a selected grant. * - * Leases are cached on the host by provider grant (and actor for user grants), - * validated against provider-owned domains, and reused until the provider lease - * is near expiry. Sandbox context only authorizes the hop. + * Leases are cached on the host by provider grant, validated against + * provider-owned domains, and reused until the provider lease is near expiry. + * Sandbox context only authorizes the hop. */ export async function sandboxEgressCredentialLease( provider: string, diff --git a/packages/junior/src/chat/sandbox/egress/session.ts b/packages/junior/src/chat/sandbox/egress/session.ts index a484e10213..6c47768ed3 100644 --- a/packages/junior/src/chat/sandbox/egress/session.ts +++ b/packages/junior/src/chat/sandbox/egress/session.ts @@ -17,11 +17,11 @@ import { getStateAdapter } from "@/chat/state/adapter"; // // The sandbox gets a signed context token in its network policy URL; the proxy // verifies that token on every forwarded request. Credential leases are cached -// on the host by provider grant (and actor only for user-owned grants) so the -// same installation token can be reused across sandboxes until expiry. Auth- -// required and permission-denied signals are written here so the sandbox -// command runner can translate host egress failures into the same user-facing -// auth flow as direct tool calls. +// on the host by provider grant. Installation grants are shared across +// sandboxes. Other grants stay bound to the actor. Auth-required and +// permission-denied signals are written here so the sandbox command runner can +// translate host egress failures into the same user-facing auth flow as direct +// tool calls. export const SANDBOX_EGRESS_PROXY_PATH = "/api/internal/sandbox-egress"; @@ -45,10 +45,9 @@ export type { /** * Build the host lease cache key for one provider grant. * - * Installation/bot grants are shared across sandboxes. User grants stay bound - * to the actor so one human's OAuth token cannot be reused for another. - * Sandbox egress id and context token id are not part of the key: those only - * authorize the hop; they do not change which host credential to inject. + * Installation grants are shared across sandboxes. Other grants stay bound to + * the actor. Sandbox egress id and context token id authorize the hop only; + * they do not change which host credential to inject. */ function leaseKey( provider: string, @@ -56,10 +55,7 @@ function leaseKey( context: SandboxEgressCredentialContext, ): string { const actor = context.credentials.actor; - // Only installation/bot grants are host-shared. User and broker-default - // grants stay actor-bound so one human's token cannot serve another. - const isSharedInstallationGrant = grant.name.startsWith("installation-"); - const actorKey = isSharedInstallationGrant + const actorKey = grant.name.startsWith("installation-") ? "shared" : "type" in actor ? `user:${actor.userId}` @@ -213,11 +209,11 @@ export function parseSandboxEgressCredentialToken( } /** - * Cache credential header transforms for one host-owned provider grant. + * Cache credential header transforms for one provider grant. * - * TTL follows the provider lease expiry. Sandbox context expiry still bounds + * TTL follows the provider lease expiry. Sandbox context expiry still decides * whether a hop may use the cache; it does not shorten a shared installation - * lease for every other sandbox. + * lease for other sandboxes. */ export async function setSandboxEgressCredentialLease( context: SandboxEgressCredentialContext, @@ -233,9 +229,7 @@ export async function setSandboxEgressCredentialLease( await state.set(leaseKey(lease.provider, lease.grant, context), lease, ttlMs); } -/** - * Load cached credential header transforms for the host-owned provider grant. - */ +/** Load cached credential header transforms for one provider grant. */ export async function getSandboxEgressCredentialLease( provider: string, grant: SandboxEgressCredentialLease["grant"], diff --git a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts index fb37a75fef..f0f768f90e 100644 --- a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts @@ -674,7 +674,7 @@ describe("sandbox egress proxy composition", () => { }); }); - it("reuses host-cached credential leases across renewed credential contexts for the same actor", async () => { + it("reuses cached credential leases across renewed contexts for the same actor", async () => { setSandboxEgressUserActor(); issueProviderCredentialLeaseMock .mockResolvedValueOnce({ @@ -727,7 +727,7 @@ describe("sandbox egress proxy composition", () => { expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(1); }); - it("remints once on upstream 403 and recovers or records permission denied", async () => { + it("retries once on upstream 403 and recovers or records permission denied", async () => { setSandboxEgressUserActor(); const lease = (token: string) => ({ id: `lease-${token}`, @@ -778,7 +778,7 @@ describe("sandbox egress proxy composition", () => { const body = await persistent.text(); expect(body).toBe("Permission denied for this organization"); expect(body).not.toContain("junior-auth-required"); - // Second hop reuses the recovered lease, then remints once after 403. + // Second hop reuses the recovered lease, then retries once after 403. expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(3); expect(fetchMock).toHaveBeenCalledTimes(4); await expect( diff --git a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts index 9c5495d264..bcf5e52cee 100644 --- a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts +++ b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts @@ -192,7 +192,7 @@ describe("sandboxEgressCredentialLease — credential error normalization", () = ); }); - it("reuses host-shared installation leases across contexts and scopes", async () => { + it("reuses installation leases across sandboxes", async () => { hasEgressCredentialHooks.mockReturnValue(true); issuePluginCredential.mockClear(); const state = new Map(); From 53412dd13495017a9065ee0edd1b5f63aa1f1a04 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:35:23 +0000 Subject: [PATCH 3/8] ref(github-code): slim skill to always-on router Keep hard rules and tool routing in SKILL.md. Move edit/PR packaging, command matrices, and troubleshooting into references so every run does not load the full handbook. Co-Authored-By: David Cramer --- .../junior-github/skills/github-code/SKILL.md | 94 +++--------- .../junior-github/skills/github-code/SPEC.md | 34 ++--- .../github-code/references/api-surface.md | 142 +++++++----------- .../references/troubleshooting-workarounds.md | 64 ++++---- .../skills/github-code/references/workflow.md | 47 ++++++ 5 files changed, 172 insertions(+), 209 deletions(-) create mode 100644 packages/junior-github/skills/github-code/references/workflow.md diff --git a/packages/junior-github/skills/github-code/SKILL.md b/packages/junior-github/skills/github-code/SKILL.md index 8085779f1d..1c3eb378f6 100644 --- a/packages/junior-github/skills/github-code/SKILL.md +++ b/packages/junior-github/skills/github-code/SKILL.md @@ -5,80 +5,34 @@ description: Work with GitHub repositories, source code, branches, commits, pull # GitHub Code Operations -Use `git` and `gh` for repository work. Use `github_createPullRequest`, not `gh pr create`, for new PRs. Use `github_updatePullRequest`, not raw `gh api`/`gh pr edit`, when changing PR title, body, base, or open/closed state. Use `github_resolvePullRequestReviewThread`, not raw `gh api graphql` `resolveReviewThread`, when resolving review threads on Junior-authored PRs. +Use `git` and `gh` for repository work. -## References +| Action | Tool / command | +| --- | --- | +| Create PR | `github_createPullRequest` (not `gh pr create`) | +| Update PR title/body/base/state | `github_updatePullRequest` (not raw PATCH / `gh pr edit`) | +| Resolve review thread | `github_resolvePullRequestReviewThread` (not raw GraphQL) | +| Clone missing repo | `github_cloneRepository`; on Workspace match error use `switchWorkspace` | -| Open when you need | Read | -| -------------------------------------- | -------------------------------------------------------------------------------------- | -| Command syntax, permissions, config | [references/api-surface.md](references/api-surface.md) | -| Failed commands or permission recovery | [references/troubleshooting-workarounds.md](references/troubleshooting-workarounds.md) | +## Open when needed -## Non-negotiable rules +| Need | Read | +| --- | --- | +| Commands, permissions, allowlist | [references/api-surface.md](references/api-surface.md) | +| Edit → verify → PR packaging | [references/workflow.md](references/workflow.md) | +| Failed command or permission recovery | [references/troubleshooting-workarounds.md](references/troubleshooting-workarounds.md) | -- Resolve the repo from the explicit request, then `github.repo`. Run `jr-rpc config get github.repo` standalone. +## Always + +- Resolve repo from the request, then `github.repo`. Run `jr-rpc config get github.repo` standalone. - Keep `--repo owner/repo` explicit on `gh`; use `git -C PATH` for local repos. -- Read applicable `AGENTS.md` files before editing. Narrower repo/task instructions win. -- Preserve unrelated work. Never force-push, delete refs, or perform destructive merges. +- Read applicable `AGENTS.md` before editing. Narrower repo/task instructions win. +- Preserve unrelated work. Never force-push, delete refs, or do destructive merges. - Base conclusions on repository evidence. Do not claim a check ran unless it did. -- For Junior-owned pull requests, push the branch before creating the PR. The runtime supplies GitHub App installation credentials for both; try the operations before requesting remediation and never ask for a user token. -- Use `github_cloneRepository` instead of shelling out to `git clone` when a repository is not already available in the sandbox. -- If `github_cloneRepository` returns a tool input error about matching Workspaces, call `switchWorkspace`. The checkout is already present after a successful switch. Pass `allowAdHoc=true` only for an intentional ad-hoc checkout. -- A tool-routing denial requires the named tool; only an upstream denial justifies permission remediation. -- Stop for ambiguous targets, missing access, destructive operations, or unresolved upstream permission failures. - -## Workflow - -### 1. Resolve and inspect - -Identify the repo, checkout, default/current branches, worktree state, repo instructions, package manager, and relevant checks. Prefer an existing checkout or matching Workspace; otherwise clone shallowly. If clone returns a Workspace tool input error, switch Workspace instead of cloning again, or pass `allowAdHoc=true`. - -A shallow clone is for fast inspection, not history rewriting. Before rebasing, merge-base analysis, blame/history work, or comparing against a base absent locally, fetch the needed refs and deepen incrementally. Use `--unshallow` only when bounded deepening is insufficient. Never use a force push to compensate for incomplete history. - -For edits, choose the smallest credible validation path before changing files. Capture a baseline when a failure may be pre-existing. - -### 2. Investigate - -Establish where the behavior lives, current versus requested behavior, root cause or gap, and the smallest proof of correctness. Read linked issues, PRs, specs, and failing output when provided. For pull requests, inspect conversation comments, inline review comments, reviews, the diff, and checks. If the request is investigation-only, report evidence without editing. - -### 3. Edit - -Make the smallest coherent change. Follow local patterns and avoid speculative cleanup. After a failed attempt, re-check the root cause before patching again. - -Before running repo checks, ensure project dependencies are available: - -1. Detect the package manager and lockfile from repo evidence. -2. If dependencies are missing or the check reports missing packages, run the repo-native frozen/immutable install (`pnpm install --frozen-lockfile`, `npm ci`, `yarn install --immutable`, `bun install --frozen-lockfile`, or the documented equivalent). -3. Do not regenerate or modify a lockfile merely to make verification run. If the locked install fails, report the exact failure unless dependency changes are part of the task. - -Do not install or repair the GitHub plugin runtime itself; that is manifest-owned setup. - -### 4. Verify and review - -Run targeted changed-file/package checks before broad suites. Separate regressions from baseline failures. For instruction-only changes, run available structural checks and perform a content-consistency review. - -### 5. Package every completed edit - -Unless the user explicitly says not to create a PR, every completed repository edit must end in a pushed branch and PR. Default to draft; honor an explicit user or repo instruction to open it ready for review. Do not stop at local changes or a commit. - -1. Reuse the current non-default branch or create a focused branch. -2. Commit using repo conventions; otherwise use `(): ` in imperative present tense, with no agent branding. -3. Push explicitly with `git push -u origin BRANCH`. -4. Resolve the actual default branch. -5. Reuse and update an existing PR for the branch with `github_updatePullRequest`; otherwise call `github_createPullRequest` with explicit repo, head, base, title, body, and `draft: true` unless the user or repo explicitly requires ready-for-review. - -PR titles use the same conventional form as commits: `(): ` or `: `. Match the current dominant change, not the latest commit or a stale title. - -Write the PR body for a reviewer who knows the product but not this change. Use ASD-STE100 English: short sentences, common words, active voice, and one idea per sentence. Avoid dense academic prose and unnecessary jargon. - -Explain what this PR changes and why it matters. Add only context the diff cannot show. Keep the body short by default; add structure only when it helps. Omit empty or `N/A` sections, file-by-file narration, copied commit logs, and redundant diff summaries. Do not put `Checks`, `Verification`, `Test plan`, or similar validation sections in the PR body; put local check results only in the final user report. - -Treat the current title, body, and commit messages as fallible context. After material follow-up commits, re-check the title and rewrite the body against the current diff with `github_updatePullRequest`. Never include customer data, PII, secrets, or sensitive thread context, especially in public repositories. Resolve requested assignee/reviewer handles from evidence; skip unconfirmed identities. - -If PR creation or update is blocked, report the exact failed command/tool call and leave the committed branch intact. - -### 6. Follow and report - -When PR creation returns a subscribable resource hint, subscribe to suggested review/CI events. Report only actionable feedback addressed, build failures fixed, fully green/ready state, or merge. +- Push the branch before creating a Junior-owned PR. Runtime injects installation credentials; never ask for a user token for bot pushes. +- Tool-routing denials need the named tool. Only upstream denials justify permission remediation. +- Stop for ambiguous targets, missing access, destructive ops, or unresolved upstream permission failures. +- Unless the user opts out, finish completed edits with a pushed branch and PR (draft by default). +- Report to the user: repo, branch, PR URL/number, local check results, and anything not run. -Return to the user (not the PR body): repo, branch, PR URL/number, local check results, pre-existing failures, and anything not run with the reason. +Do not install or repair the GitHub plugin runtime from this skill. The plugin manifest owns that. diff --git a/packages/junior-github/skills/github-code/SPEC.md b/packages/junior-github/skills/github-code/SPEC.md index 610474084d..12a06861ce 100644 --- a/packages/junior-github/skills/github-code/SPEC.md +++ b/packages/junior-github/skills/github-code/SPEC.md @@ -2,44 +2,40 @@ ## Intent -Guide evidence-first GitHub repository work from inspection through a reviewable result without duplicating command and troubleshooting detail in runtime context. +Guide evidence-first GitHub repository work from inspection through a reviewable result without loading command and packaging detail on every run. ## Behavioral contract - Resolve and inspect the repository before acting. -- Preserve unrelated work and reject destructive Git operations. -- Treat shallow clones as inspection checkouts; fetch/deepen before history-dependent operations and never force-push around missing ancestry. -- Install repository dependencies with the detected package manager's locked/frozen mode before verification when dependencies are absent. -- For every completed repository edit, create or update a pushed PR unless the user explicitly opts out; default new PRs to draft while honoring explicit ready-for-review instructions. -- Write conventional PR titles that match the current dominant change. -- Write short reviewer-facing PR bodies in ASD-STE100 English; explain what changed and why, add only context the diff cannot show, omit empty ceremony or fixed templates, and keep Checks/Verification/Test plan style sections out of the PR body (report local checks to the user instead). -- Treat existing PR metadata and commit messages as fallible context, and refresh the title/body against the current diff after material changes. -- Report exact validation and permission failures without claiming partial work is complete. +- Preserve unrelated work; reject destructive Git operations. +- 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 completed edits with a pushed PR unless the user opts out; default draft. +- Write conventional PR titles and short plain-English bodies; keep check results out of the PR body. +- Report exact validation and permission failures. ## Runtime architecture -- `SKILL.md`: compact workflow and decision rules. +- `SKILL.md`: always-on rules and reference router. - `references/api-surface.md`: command and permission lookup. +- `references/workflow.md`: edit, verify, and PR packaging. - `references/troubleshooting-workarounds.md`: failure recovery. -Do not move provider runtime installation, OAuth, or environment setup into this skill; the GitHub plugin manifest owns those concerns. +The GitHub plugin manifest owns runtime install, OAuth, and env setup. Do not move those into this skill. -## Trigger expectations +## Triggers Should trigger for implementation, source inspection, clone/fetch/branch work, commits, PRs, reviews, CI, and repository credential failures. -Should not trigger for GitHub issue-only operations, non-GitHub ticketing, product telemetry, or general product documentation with no repository task. +Should not trigger for issue-only ops, non-GitHub ticketing, product telemetry, or docs with no repository task. ## Validation -After material edits: - 1. Run the repository skill validator. -2. Run formatting or package checks applicable to changed Markdown. -3. Confirm all referenced files exist. +2. Confirm all referenced files exist. +3. Confirm `SKILL.md` stays a router (workflow detail lives in references). 4. Confirm code-edit completion defaults to a draft PR. -5. Confirm dependency installation and shallow-history recovery do not permit lockfile mutation or force-push shortcuts. ## Maintenance -Keep workflow policy in `SKILL.md`; move syntax matrices and failure details to the routed references. Remove duplicated rules rather than restating them across sections. +Keep always-on policy in `SKILL.md`. Move syntax matrices, packaging steps, and failure tables to routed references. Delete duplicates instead of restating them. diff --git a/packages/junior-github/skills/github-code/references/api-surface.md b/packages/junior-github/skills/github-code/references/api-surface.md index 2641ca6fd7..3191dec9d1 100644 --- a/packages/junior-github/skills/github-code/references/api-surface.md +++ b/packages/junior-github/skills/github-code/references/api-surface.md @@ -1,86 +1,56 @@ -# GitHub API Surface — code & pull requests - -PR creation uses Junior's `github_createPullRequest` tool. PR title, body, base, and open/closed state updates use `github_updatePullRequest` so Junior keeps requester attribution and the conversation footer. Review-thread resolve uses `github_resolvePullRequestReviewThread` because GitHub exposes only the GraphQL `resolveReviewThread` mutation (no REST endpoint and no first-class `gh pr` subcommand). Other supported mutations use allowlisted REST endpoints through `gh api`; generic GraphQL-backed `gh pr` mutations are not supported. - -## Repo scoping - -When the user omits `owner/repo`, resolve `github.repo` first with `jr-rpc config get github.repo`, then pass the resolved repo explicitly on the actual `gh` or `git` command. -Run `jr-rpc config get github.repo` as a standalone bash command. Never chain it with `cd`, `&&`, pipes, or a provider command. -Treat explicit repo flags as command-targeting safety rails, not as a credential-scoping mechanism. - -## GitHub App permission guidance - -| Permission capability | Commands | -| ---------------------------- | ------------------------------------------------------------------------------------ | -| `github.actions.read` | `gh run list`, `gh run view`, `gh run watch`, `gh workflow list`, `gh workflow view` | -| `github.actions.write` | `gh workflow run`, `gh run rerun`, `gh run cancel` | -| `github.contents.read` | `gh repo clone`, `git fetch` | -| `github.contents.write` | Git smart-HTTP `git push` only | -| `github.workflows.write` | Workflow-file changes carried by Git smart-HTTP push | -| `github.pull-requests.read` | `gh pr view`, `gh pr list`, `gh pr diff`, `gh pr checks` | -| `github.pull-requests.write` | Typed PR creation and allowlisted REST PR lifecycle endpoints | - -## Command matrix - -| Operation | Command | -| ---------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | -| Clone repository (default shallow) | `gh repo clone owner/repo [DIRECTORY] -- --depth=1` | -| Fetch bounded base history | `git -C DIRECTORY fetch --depth=N origin BASE:refs/remotes/origin/BASE` | -| Deepen base history | `git -C DIRECTORY fetch --deepen=N origin BASE:refs/remotes/origin/BASE` | -| Convert shallow clone to full | `git -C DIRECTORY fetch --unshallow origin` | -| Check shallow state | `git -C DIRECTORY rev-parse --is-shallow-repository` | -| Check branch | `git -C DIRECTORY branch --show-current` | -| Check worktree state | `git -C DIRECTORY status --short --branch` | -| View commit log against base | `git -C DIRECTORY log origin/BASE..HEAD --oneline` | -| Diff against base | `git -C DIRECTORY diff origin/BASE...HEAD` | -| Resolve default branch | `gh repo view owner/repo --json defaultBranchRef --jq .defaultBranchRef.name` | -| Create branch | `git -C DIRECTORY checkout -b BRANCH` | -| Stage and commit | `git -C DIRECTORY add -A && git -C DIRECTORY commit -m "message"` | -| Push branch before PR creation | `git -C DIRECTORY push -u origin BRANCH` | -| Dispatch workflow | `gh workflow run WORKFLOW --repo owner/repo --ref REF [-f key=value]` | -| Rerun workflow run | `gh run rerun RUN_ID -R owner/repo [--failed]` | -| Rerun workflow job | `gh run rerun --job JOB_ID -R owner/repo` | -| Cancel workflow run | `gh run cancel RUN_ID -R owner/repo` | -| Create pull request (draft) | `github_createPullRequest({ repo: "owner/repo", head: "BRANCH", base: "BASE", title: "...", body: "...", draft: true })` | -| Update pull request | `github_updatePullRequest({ repo: "owner/repo", number: NUMBER, title?: "...", body?: "...", base?: "BASE", state?: "open" \| "closed" })` | -| Mark ready for review | `gh api repos/owner/repo/pulls/NUMBER/ready_for_review --method POST` | -| Request reviewers | `gh api repos/owner/repo/pulls/NUMBER/requested_reviewers --method POST --input reviewers.json` | -| Remove requested reviewers | `gh api repos/owner/repo/pulls/NUMBER/requested_reviewers --method DELETE --input reviewers.json` | -| Close pull request | `github_updatePullRequest({ repo: "owner/repo", number: NUMBER, state: "closed" })` | -| Submit pull request review | `gh api repos/owner/repo/pulls/NUMBER/reviews --method POST --input review.json` | -| Post inline review comment | `gh api repos/owner/repo/pulls/NUMBER/comments --method POST --input comment.json` | -| Reply to inline review comment | `gh api repos/owner/repo/pulls/NUMBER/comments/COMMENT_ID/replies --method POST --input reply.json` | -| Resolve review thread | `github_resolvePullRequestReviewThread({ repo: "owner/repo", threadId: "PRRT_..." })` (GraphQL `resolveReviewThread` substitute; Junior-authored PRs only) | -| View pull request | `gh pr view NUMBER --repo owner/repo [--json ...]` | -| List pull requests | `gh pr list --repo owner/repo [--state open \| closed \| merged]` | -| Diff pull request | `gh pr diff NUMBER --repo owner/repo` | -| Check pull request status | `gh pr checks NUMBER --repo owner/repo` | -| View PR review comments | `gh api repos/{owner}/{repo}/pulls/{number}/comments` | -| View PR reviews | `gh api repos/{owner}/{repo}/pulls/{number}/reviews` | -| List workflow runs | `gh run list -R owner/repo --workflow WORKFLOW [--limit N] [--json ...]` | -| View workflow run | `gh run view RUN_ID -R owner/repo [--json ...] [--log-failed]` | -| Watch workflow run | `gh run watch RUN_ID -R owner/repo --exit-status` | - -## Config helpers - -```bash -jr-rpc config get github.repo -jr-rpc config set github.repo owner/repo -``` - -## Behavior notes - -- Prefer `--json` output for machine-readable parsing where available. -- Pass extra `git clone` flags after `--` (e.g. `gh repo clone owner/repo -- --depth=1`). -- A local `git commit` does not call GitHub. Pushing that commit uses Junior's installation credential and requires `github.contents.write` on the target repo. -- If the commit changes workflow files under `.github/workflows`, the App installation needs Workflows write in addition to Contents write. -- Before rebasing, merge-base analysis, blame/history inspection, or a base comparison, check whether the repository is shallow. Fetch a bounded depth of the base into `refs/remotes/origin/BASE`, deepen incrementally until the needed ancestry is present, and compare against `origin/BASE`; use `--unshallow` only when bounded deepening is insufficient. Never force-push to work around missing ancestry. -- Before `github_createPullRequest`, push the head branch explicitly and resolve the target repo's default branch for `base`. That push requires GitHub write access to the remote. -- Use `github_updatePullRequest` for title, body, base, or open/closed state changes. Do not raw-`PATCH` `/repos/.../pulls/NUMBER`; that path is denied so Junior can keep the conversation footer. -- Merge, fork creation, REST contents/Git database writes, and repository administration are outside the current write allowlist. -- Pull request reviews and inline review comments use the same `installation-write` credential as other bot-owned PR writes, so they post as Junior even on headless turns. Merge remains denied. -- Resolve review threads with `github_resolvePullRequestReviewThread`. That tool is the Junior equivalent of `gh api graphql` `resolveReviewThread`; raw GraphQL mutations stay denied, and the tool only succeeds on Junior-authored PRs. -- If the explicit `git push` fails with 401/403 or another access/permission error, verify the repo context and retry once. If it still fails, load troubleshooting guidance and report the exact command failure. -- PR comments, labels, and assignees use GitHub's issue endpoints; use the `github-issues` REST guidance for those operations. All allowlisted bot writes share the same `installation-write` credential. -- To embed a local image in a GitHub issue, pull request, review, or comment, call `publishImage` first. That tool returns a durable public URL. The published image is public to anyone on the internet who has the URL. Embed the URL with normal GitHub Markdown. Do not use private Slack file links or conversation attachment URLs. -- Return actionable errors for access, permission, not-found, and validation failures. +# GitHub API surface — code and pull requests + +PR create/update and review-thread resolve use the Junior tools named in `SKILL.md`. Other supported mutations use allowlisted REST through `gh api`. Generic GraphQL-backed `gh pr` mutations are not supported. + +## Repo targeting + +When the user omits `owner/repo`, resolve with standalone `jr-rpc config get github.repo`, then pass `--repo owner/repo` on the next `gh`/`git` command. Explicit repo flags target the command; they are not a credential scope. + +## Permissions + +| Capability | Commands | +| --- | --- | +| `github.actions.read` | `gh run list`, `gh run view`, `gh run watch`, `gh workflow list`, `gh workflow view` | +| `github.actions.write` | `gh workflow run`, `gh run rerun`, `gh run cancel` | +| `github.contents.read` | `gh repo clone`, `git fetch` | +| `github.contents.write` | Git smart-HTTP `git push` only | +| `github.workflows.write` | Workflow-file changes on push | +| `github.pull-requests.read` | `gh pr view`, `gh pr list`, `gh pr diff`, `gh pr checks` | +| `github.pull-requests.write` | Typed PR create and allowlisted REST PR lifecycle | + +## Commands + +| Operation | Command | +| --- | --- | +| Clone (default shallow) | `gh repo clone owner/repo [DIR] -- --depth=1` | +| Fetch bounded base | `git -C DIR fetch --depth=N origin BASE:refs/remotes/origin/BASE` | +| Deepen base | `git -C DIR fetch --deepen=N origin BASE:refs/remotes/origin/BASE` | +| Unshallow | `git -C DIR fetch --unshallow origin` | +| Shallow check | `git -C DIR rev-parse --is-shallow-repository` | +| Branch / status | `git -C DIR branch --show-current` / `git -C DIR status --short --branch` | +| Log / diff vs base | `git -C DIR log origin/BASE..HEAD --oneline` / `git -C DIR diff origin/BASE...HEAD` | +| Default branch | `gh repo view owner/repo --json defaultBranchRef --jq .defaultBranchRef.name` | +| Create branch | `git -C DIR checkout -b BRANCH` | +| Commit | `git -C DIR add -A && git -C DIR commit -m "message"` | +| Push | `git -C DIR push -u origin BRANCH` | +| Workflow dispatch | `gh workflow run WORKFLOW --repo owner/repo --ref REF [-f key=value]` | +| Rerun / cancel | `gh run rerun RUN_ID -R owner/repo [--failed]` / `gh run cancel RUN_ID -R owner/repo` | +| Ready for review | `gh api repos/owner/repo/pulls/NUMBER/ready_for_review --method POST` | +| Request reviewers | `gh api repos/owner/repo/pulls/NUMBER/requested_reviewers --method POST --input reviewers.json` | +| Submit review | `gh api repos/owner/repo/pulls/NUMBER/reviews --method POST --input review.json` | +| Inline review comment | `gh api repos/owner/repo/pulls/NUMBER/comments --method POST --input comment.json` | +| View PR / checks | `gh pr view NUMBER --repo owner/repo` / `gh pr checks NUMBER --repo owner/repo` | +| Diff PR | `gh pr diff NUMBER --repo owner/repo` | +| List runs | `gh run list -R owner/repo --workflow WORKFLOW` | +| View / watch run | `gh run view RUN_ID -R owner/repo` / `gh run watch RUN_ID -R owner/repo --exit-status` | + +## Notes + +- Prefer `--json` where available. Pass clone flags after `--`. +- Local commit does not call GitHub. Push uses installation credentials (`contents.write`; workflow files also need `workflows.write`). +- Before history-dependent git work, deepen shallow clones; never force-push around missing ancestry. +- Push head and resolve default `base` before `github_createPullRequest`. +- Denied: merge, forks, REST contents/Git database writes, repo admin, raw PR PATCH, raw GraphQL mutations. +- Reviews and inline comments post as the App bot via `installation-write`. +- PR comments/labels/assignees use issue endpoints; load `github-issues` for those. +- Embed local images with `publishImage` first (public URL). Do not use private Slack file links. diff --git a/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md b/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md index 5d14720753..d102670410 100644 --- a/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md +++ b/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md @@ -1,37 +1,33 @@ -# GitHub CLI Troubleshooting — code & pull requests +# GitHub CLI troubleshooting — code and pull requests -Use this table to recover quickly while keeping operations deterministic. +| Symptom | Likely cause | Fix | +| --- | --- | --- | +| `unknown command` from `gh` | Runtime `gh` missing or too old | Report GitHub plugin runtime dependency unavailable | +| `unknown flag: --depth` on clone | Clone flags before `--` | `gh repo clone owner/repo -- --depth=1` | +| Missing `--repo` | No explicit target | Resolve `github.repo`, pass `--repo owner/repo` | +| Wrong repo authenticated | Stale default | Pass `--repo owner/repo` or update `github.repo` | +| GraphQL: could not resolve repository | Bad slug or no access | Validate `owner/repo` and App install | +| 401 Unauthorized | Credential rejected | Confirm target; distinguish user OAuth vs installation setup | +| `junior-auth-required` `user-write` | Missing/stale user OAuth | Follow private OAuth prompt; never ask for pasted tokens | +| `git push` 401/403 | Install scope, remote, or permissions | Verify remote/repo, retry once, then report install scope | +| `permission_denied` `source: "upstream"` | GitHub 403 after inject | Not a Junior runtime block; use grant/account/SSO fields | +| 403 without upstream `permission_denied` | Local policy denial | Read body; follow required-tool guidance | +| `Token scopes: none` on `gh auth status` | Normal for App user tokens | Use App permissions / accepted-permissions headers | +| `github_createPullRequest` 401/403 | Install/repo lacks write | Report install scope; do not fall back to user OAuth | +| Create PR 422 on `head` | Branch not pushed | Push branch; retry with explicit head/base | +| Create/update PR 422 on `base` | Base missing | Resolve default branch; retry | +| 403 names `github_updatePullRequest` | Raw PR PATCH blocked | Use `github_updatePullRequest` | +| GraphQL mutations not enabled | Raw resolve blocked | Use `github_resolvePullRequestReviewThread` (Junior PRs only) | +| Missing blame/old history | Shallow clone | Deepen needed refs; `--unshallow` only if required | +| Odd ancestry / rebase fails | Base ref missing locally | Fetch `BASE:refs/remotes/origin/BASE`, deepen, use `origin/BASE` | +| Missing deps in tests | Not installed | Frozen/immutable install for the lockfile; do not rewrite lockfile | +| Frozen install fails | Drift or registry | Report exact failure | +| `dnf install gh failed` | Plugin bootstrap | Report runtime bootstrap failure; do not repair from skill | -| Symptom | Likely cause | Fix | -| ---------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `unknown command "..."` from `gh` | CLI version too old or wrong binary in the plugin runtime. | Verify `gh --version`; if it is unavailable or too old, report that the GitHub plugin runtime dependency is not available. | -| `unknown flag: --depth` from `gh repo clone` | `git clone` flags were passed before `--`. | Pass clone flags after `--`, for example `gh repo clone owner/repo -- --depth=1`. | -| `Missing required option --repo` | Repo not passed and no default was resolved. | Resolve with `jr-rpc config get github.repo`; pass `--repo owner/repo` explicitly when missing. | -| Command affects or authenticates against the wrong repo | Stale `github.repo` default or authenticated command missing explicit repo. | Pass `--repo owner/repo` for the target repository, or update `github.repo` before retrying. | -| `GraphQL: Could not resolve to a Repository` | Repo slug is wrong or inaccessible. | Validate `owner/repo` and confirm app installation on target repository. | -| 401 Unauthorized | Issued GitHub credentials were rejected upstream. | Verify the target repo, then use the grant/auth signal to distinguish stale user OAuth from app installation or host env setup. | -| `junior-auth-required provider=github grant=user-write` | User-to-server OAuth is missing or stale for a human-identity operation. | Follow the private OAuth prompt; do not ask the user to paste or manage tokens manually. | -| `git push` fails with 401/403 or auth/permission output | Write permission is missing, app installation is too narrow, or remote is wrong. | Verify the remote and repo context, retry once, then confirm app permissions and installation scope if it still fails. | -| Bash result includes `permission_denied` with `source: "upstream"` | GitHub returned 403 after Junior injected the named grant. | Do not call this a Junior runtime block. Use the message, connected account, upstream target, grant requirements, accepted-permissions, and SSO fields to explain the GitHub denial. | -| 403 without `permission_denied` where `source: "upstream"` | Junior may have rejected an unsupported route before contacting GitHub. | Read the response body. Follow any required-tool instruction; do not ask for GitHub permissions unless the failure is confirmed upstream. | -| `gh auth status` shows `Token scopes: none` | Expected for GitHub App user-to-server tokens. | Do not treat this as read-only proof. Use the failed command, `permission_denied.acceptedPermissions`, and GitHub App permissions instead. | -| `github_createPullRequest` returns upstream 401/403 | The App installation or target repository does not permit the operation. | Use the structured upstream denial to verify installation scope and accepted permissions; do not request user OAuth for this bot-owned operation. | -| `github_createPullRequest` returns 422 for `head` | The head branch was not pushed or the explicit head ref is wrong. | Push the branch, then retry with explicit `repo`, `head`, and `base` values. | -| `github_createPullRequest` fails with 422 validation on `base` | The `base` branch does not exist in the target repo. | Resolve the default branch with `gh repo view owner/repo --json defaultBranchRef --jq .defaultBranchRef.name`, then retry with that value as `base`. | -| `403` names `github_updatePullRequest` | Raw PR title/body/base/state PATCH was blocked so Junior can own the footer. | Retry with `github_updatePullRequest`; do not use `gh api .../pulls/NUMBER --method PATCH` or `gh pr edit`. | -| `GraphQL mutations are not enabled` / resolve thread denied | Raw `gh api graphql` `resolveReviewThread` is blocked. | Retry with `github_resolvePullRequestReviewThread({ repo, threadId })`. Only Junior-authored PRs are allowed. | -| `github_updatePullRequest` returns upstream 401/403 | The App installation or target repository does not permit the operation. | Use the structured upstream denial to verify installation scope and accepted permissions; do not request user OAuth for this bot-owned operation. | -| `git blame`, long log history, or old commits are missing after clone | Repo was cloned shallow by design. | Fetch the required ref and deepen it incrementally; use `--unshallow` only when bounded deepening is insufficient. | -| Rebase, merge-base, or `origin/BASE...HEAD` comparison fails or gives odd ancestry | Required ancestry or the remote-tracking base ref is absent from the shallow clone. | Fetch a bounded depth into `BASE:refs/remotes/origin/BASE`, deepen that base ref until the merge base exists, and compare or rebase against `origin/BASE`. Never force-push around incomplete history. | -| Tests fail because dependencies or executables are missing | Repository dependencies were not installed. | Detect the lockfile and run the repo-native frozen/immutable install. Do not rewrite the lockfile unless dependency changes are part of the task. | -| Frozen/immutable dependency install fails | Lockfile drift, unavailable registry, incompatible runtime, or environment issue. | Report the exact install failure. Do not fall back to a lockfile-updating install unless the requested work intentionally changes dependencies. | -| `sandbox setup failed (dnf install gh failed ...)` | `gh` package not available from the plugin runtime dependency bootstrap. | Report the plugin runtime bootstrap failure; do not try to repair package installation from the skill workflow. | +## Retry rules -## Retry guidance - -- Retry once for transient transport failures after verifying repo context. -- Do not loop retries on repeated 401/403/404 validation errors. -- Treat missing or stale `user-read`/`user-write` grants as private GitHub App OAuth work. Treat all `installation-*` failures as App permission, installation scope, or host environment setup; they do not fall back to user OAuth. -- Do not describe `permission_denied` with `source: "upstream"` as Junior blocking the request. It means the egress proxy injected a credential, forwarded the request, and recorded GitHub's upstream 403. Prefer its `account` and `grant.requirements` fields over inference when explaining what to fix. -- Do not infer permission level from OAuth scopes. GitHub App user tokens report no OAuth scopes; GitHub App permissions and accepted-permissions headers are the useful evidence. -- For persistent permission problems, return explicit remediation and stop. +- Retry once for transient transport after verifying repo context. +- Do not loop on repeated 401/403/404 validation errors. +- `user-read`/`user-write` gaps → private App OAuth. `installation-*` failures → App permission/install/host setup only. +- Prefer `permission_denied` structured fields over guessing. +- Persistent permission problems: report remediation and stop. diff --git a/packages/junior-github/skills/github-code/references/workflow.md b/packages/junior-github/skills/github-code/references/workflow.md new file mode 100644 index 0000000000..5204ede7fa --- /dev/null +++ b/packages/junior-github/skills/github-code/references/workflow.md @@ -0,0 +1,47 @@ +# Edit and PR packaging + +Open this when making repository edits, not for read-only inspection. + +## Resolve and inspect + +1. Identify repo, checkout, branches, worktree, package manager, and relevant checks. +2. Prefer an existing checkout or matching Workspace; otherwise clone shallowly. +3. If clone returns a Workspace match error, switch Workspace or pass `allowAdHoc=true`. +4. Shallow clones are for fast inspection. Before rebase, merge-base, blame, or base comparison, fetch/deepen the needed refs. Never force-push around missing history. +5. For edits, pick the smallest credible validation path. Capture a baseline when a failure may be pre-existing. + +## Investigate + +1. Find where the behavior lives, current vs requested, root cause or gap, and the smallest proof. +2. Read linked issues, PRs, specs, and failing output when provided. +3. For PRs, inspect conversation, inline comments, reviews, diff, and checks. +4. Investigation-only requests: report evidence; do not edit. + +## Edit + +1. Make the smallest coherent change. Follow local patterns. Avoid speculative cleanup. +2. After a failed attempt, re-check root cause before patching again. +3. Before repo checks, ensure dependencies with the lockfile-native frozen install. Do not rewrite the lockfile unless dependency changes are part of the task. + +## Verify + +1. Run targeted changed-file/package checks before broad suites. +2. Separate regressions from baseline failures. +3. Instruction-only changes: structural checks plus content review. + +## Package + +1. Reuse the current non-default branch or create a focused branch. +2. Commit with repo conventions, else `(): ` imperative present, no agent branding. +3. Push with `git push -u origin BRANCH`. +4. Resolve the actual default branch. +5. Update an existing PR for the branch with `github_updatePullRequest`, or create with `github_createPullRequest` (`draft: true` unless ready-for-review is required). +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. Put local check results in the user report only. +8. After material follow-up commits, refresh title and body against the current diff. +9. Never put customer data, PII, secrets, or sensitive thread context in public PR text. +10. If PR create/update is blocked, report the exact failure and leave the committed branch intact. + +## Follow + +When PR creation returns a subscribable hint, subscribe to suggested review/CI events. Report only actionable feedback fixed, build failures fixed, green/ready, or merge. From da9730734af77ab8143fcf846260423fd21304b9 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:39:40 +0000 Subject: [PATCH 4/8] ref(skills): drop agent brand name from github skill prose Agent display name is configurable. Prefer runtime/bot wording in skill text, and keep protocol tokens like junior-auth-required unchanged. Co-Authored-By: David Cramer --- packages/junior-github/skills/github-code/SKILL.md | 2 +- .../skills/github-code/references/api-surface.md | 2 +- .../github-code/references/troubleshooting-workarounds.md | 4 ++-- packages/junior-github/skills/github-issues/SKILL.md | 4 ++-- .../skills/github-issues/references/api-surface.md | 2 +- .../github-issues/references/troubleshooting-workarounds.md | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/junior-github/skills/github-code/SKILL.md b/packages/junior-github/skills/github-code/SKILL.md index 1c3eb378f6..e39131e4ec 100644 --- a/packages/junior-github/skills/github-code/SKILL.md +++ b/packages/junior-github/skills/github-code/SKILL.md @@ -29,7 +29,7 @@ Use `git` and `gh` for repository work. - Read applicable `AGENTS.md` before editing. Narrower repo/task instructions win. - Preserve unrelated work. Never force-push, delete refs, or do destructive merges. - Base conclusions on repository evidence. Do not claim a check ran unless it did. -- Push the branch before creating a Junior-owned PR. Runtime injects installation credentials; never ask for a user token for bot pushes. +- Push the branch before creating a bot-owned PR. Runtime injects installation credentials; never ask for a user token for bot pushes. - Tool-routing denials need the named tool. Only upstream denials justify permission remediation. - Stop for ambiguous targets, missing access, destructive ops, or unresolved upstream permission failures. - Unless the user opts out, finish completed edits with a pushed branch and PR (draft by default). diff --git a/packages/junior-github/skills/github-code/references/api-surface.md b/packages/junior-github/skills/github-code/references/api-surface.md index 3191dec9d1..584a60162b 100644 --- a/packages/junior-github/skills/github-code/references/api-surface.md +++ b/packages/junior-github/skills/github-code/references/api-surface.md @@ -1,6 +1,6 @@ # GitHub API surface — code and pull requests -PR create/update and review-thread resolve use the Junior tools named in `SKILL.md`. Other supported mutations use allowlisted REST through `gh api`. Generic GraphQL-backed `gh pr` mutations are not supported. +PR create/update and review-thread resolve use the tools named in `SKILL.md`. Other supported mutations use allowlisted REST through `gh api`. Generic GraphQL-backed `gh pr` mutations are not supported. ## Repo targeting diff --git a/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md b/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md index d102670410..0029f60662 100644 --- a/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md +++ b/packages/junior-github/skills/github-code/references/troubleshooting-workarounds.md @@ -10,14 +10,14 @@ | 401 Unauthorized | Credential rejected | Confirm target; distinguish user OAuth vs installation setup | | `junior-auth-required` `user-write` | Missing/stale user OAuth | Follow private OAuth prompt; never ask for pasted tokens | | `git push` 401/403 | Install scope, remote, or permissions | Verify remote/repo, retry once, then report install scope | -| `permission_denied` `source: "upstream"` | GitHub 403 after inject | Not a Junior runtime block; use grant/account/SSO fields | +| `permission_denied` `source: "upstream"` | GitHub 403 after inject | Not a local runtime block; use grant/account/SSO fields | | 403 without upstream `permission_denied` | Local policy denial | Read body; follow required-tool guidance | | `Token scopes: none` on `gh auth status` | Normal for App user tokens | Use App permissions / accepted-permissions headers | | `github_createPullRequest` 401/403 | Install/repo lacks write | Report install scope; do not fall back to user OAuth | | Create PR 422 on `head` | Branch not pushed | Push branch; retry with explicit head/base | | Create/update PR 422 on `base` | Base missing | Resolve default branch; retry | | 403 names `github_updatePullRequest` | Raw PR PATCH blocked | Use `github_updatePullRequest` | -| GraphQL mutations not enabled | Raw resolve blocked | Use `github_resolvePullRequestReviewThread` (Junior PRs only) | +| GraphQL mutations not enabled | Raw resolve blocked | Use `github_resolvePullRequestReviewThread` (bot-authored PRs only) | | Missing blame/old history | Shallow clone | Deepen needed refs; `--unshallow` only if required | | Odd ancestry / rebase fails | Base ref missing locally | Fetch `BASE:refs/remotes/origin/BASE`, deepen, use `origin/BASE` | | Missing deps in tests | Not installed | Frozen/immutable install for the lockfile; do not rewrite lockfile | diff --git a/packages/junior-github/skills/github-issues/SKILL.md b/packages/junior-github/skills/github-issues/SKILL.md index ce74331096..afa22055a3 100644 --- a/packages/junior-github/skills/github-issues/SKILL.md +++ b/packages/junior-github/skills/github-issues/SKILL.md @@ -92,8 +92,8 @@ Run [references/issue-quality-checklist.md](references/issue-quality-checklist.m ### 5. Execute -- Use `github_createIssue` for new issues so Junior owns idempotency and session-link footers. -- Use `github_updateIssue` for issue title, body, or state changes so Junior preserves requester attribution and the session footer. +- Use `github_createIssue` for new issues so the runtime owns idempotency and session-link footers. +- Use `github_updateIssue` for issue title, body, or state changes so the runtime preserves requester attribution and the session footer. - Use `gh` commands from [references/api-surface.md](references/api-surface.md) for comments, labels, assignees, and read-only operations. - For issue listing or other read-only inspection, prefer `--json` output so empty results still produce deterministic stdout. - Check duplicates silently before creating a new issue. Do not mention this check in the final reply unless a duplicate blocks creation. diff --git a/packages/junior-github/skills/github-issues/references/api-surface.md b/packages/junior-github/skills/github-issues/references/api-surface.md index c3ea672468..74a0e2a1e8 100644 --- a/packages/junior-github/skills/github-issues/references/api-surface.md +++ b/packages/junior-github/skills/github-issues/references/api-surface.md @@ -1,6 +1,6 @@ # GitHub Issue API Surface -Issue creation uses `github_createIssue`. Issue title, body, and state updates use `github_updateIssue` so Junior keeps requester attribution and the conversation footer. Comments, labels, assignees, and reads use allowlisted REST endpoints through `gh api`; generic GraphQL-backed `gh issue` mutations are not supported. +Issue creation uses `github_createIssue`. Issue title, body, and state updates use `github_updateIssue` so the runtime keeps requester attribution and the conversation footer. Comments, labels, assignees, and reads use allowlisted REST endpoints through `gh api`; generic GraphQL-backed `gh issue` mutations are not supported. ## Repo scoping diff --git a/packages/junior-github/skills/github-issues/references/troubleshooting-workarounds.md b/packages/junior-github/skills/github-issues/references/troubleshooting-workarounds.md index 0106c6e95f..8513d2e45c 100644 --- a/packages/junior-github/skills/github-issues/references/troubleshooting-workarounds.md +++ b/packages/junior-github/skills/github-issues/references/troubleshooting-workarounds.md @@ -10,7 +10,7 @@ Use this table to recover quickly while keeping operations deterministic. | `GraphQL: Could not resolve to a Repository` | Repo slug is wrong or inaccessible. | Validate `owner/repo` and confirm app installation on target repository. | | 401 Unauthorized | Issued GitHub credentials were rejected upstream. | Verify the target repo, then use the grant/auth signal to distinguish stale user OAuth from app installation or host env setup. | | `junior-auth-required provider=github grant=user-write` | User-to-server OAuth is missing or stale for a human-identity operation. | Follow the private OAuth prompt; do not ask the user to paste or manage tokens manually. | -| 403 without `permission_denied` where `source: "upstream"` | Junior may have rejected an unsupported route before contacting GitHub. | Read the response body. Follow any required-tool instruction; do not ask for GitHub permissions unless the failure is confirmed upstream. | +| 403 without `permission_denied` where `source: "upstream"` | The runtime may have rejected an unsupported route before contacting GitHub. | Read the response body. Follow any required-tool instruction; do not ask for GitHub permissions unless the failure is confirmed upstream. | | `permission_denied` with `source: "upstream"` | GitHub rejected the injected installation credential. | Verify the target, accepted permissions, and App installation scope; do not request user OAuth for a bot-owned issue operation. | | 404 Not Found | Issue number or repo is wrong. | Validate repo + issue ID with `gh issue view NUMBER --repo owner/repo`. | | Issue label mutation fails | Wrong REST payload or wrong repo context. | Use the allowlisted issue labels endpoint with an explicit `owner/repo` path and valid JSON input. | From 7eaeb4cdadeeb1df7257e057848ff8a5b4207d79 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:40:44 +0000 Subject: [PATCH 5/8] ref(github): stop using agent brand name in agent-facing copy Agent display name is configurable. Prefer runtime/bot wording in skills and tool descriptions/errors. Keep protocol tokens and session footer product links unchanged. Co-Authored-By: David Cramer --- packages/junior-github/src/tools/create-issue.ts | 8 ++++---- .../junior-github/src/tools/create-pull-request.ts | 10 +++++----- packages/junior-github/src/tools/footer.ts | 6 +++--- .../src/tools/resolve-pull-request-review-thread.ts | 10 +++++----- packages/junior-github/src/tools/update-issue.ts | 4 ++-- .../junior-github/src/tools/update-pull-request.ts | 4 ++-- packages/junior-github/tests/github-plugin.test.ts | 2 +- .../tests/resolve-pull-request-review-thread.test.ts | 2 +- packages/junior-github/tests/update-issue.test.ts | 2 +- .../junior-github/tests/update-pull-request.test.ts | 2 +- 10 files changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/junior-github/src/tools/create-issue.ts b/packages/junior-github/src/tools/create-issue.ts index c6a28e4f39..b81c19e486 100644 --- a/packages/junior-github/src/tools/create-issue.ts +++ b/packages/junior-github/src/tools/create-issue.ts @@ -38,7 +38,7 @@ const createIssueInputSchema = Type.Object( }), body: Type.Optional( Type.String({ - description: "Issue body. Junior appends the conversation footer.", + description: "Issue body. The runtime appends the conversation footer.", }), ), labels: Type.Optional( @@ -57,7 +57,7 @@ const createIssueToolInputSchema = z title: z.string().describe("Issue title."), body: z .string() - .describe("Issue body. Junior appends the conversation footer.") + .describe("Issue body. The runtime appends the conversation footer.") .optional(), labels: z .array(z.string()) @@ -311,7 +311,7 @@ export function createGitHubIssueTool(ctx: ToolRegistrationHookContext) { readOnlyHint: false, }, description: - "Create a GitHub issue with a runtime-owned Junior conversation footer. Use this instead of shelling out to gh issue create when creating issues.", + "Create a GitHub issue with a runtime-owned conversation footer. Use this instead of shelling out to gh issue create when creating issues.", inputSchema: createIssueToolInputSchema, outputSchema: gitHubIssueOutputSchema, async execute( @@ -375,7 +375,7 @@ export function createGitHubIssueTool(ctx: ToolRegistrationHookContext) { ); } catch (error) { throw new Error( - "GitHub issue was created, but Junior could not persist the completed issue state.", + "GitHub issue was created, but the runtime could not persist the completed issue state.", { cause: error }, ); } diff --git a/packages/junior-github/src/tools/create-pull-request.ts b/packages/junior-github/src/tools/create-pull-request.ts index 990dd8d4a3..8ca7f632b7 100644 --- a/packages/junior-github/src/tools/create-pull-request.ts +++ b/packages/junior-github/src/tools/create-pull-request.ts @@ -50,7 +50,7 @@ const createPullRequestInputSchema = Type.Object( body: Type.Optional( Type.String({ description: - "Pull request body. Junior appends the conversation footer.", + "Pull request body. The runtime appends the conversation footer.", }), ), draft: Type.Optional( @@ -71,7 +71,7 @@ const createPullRequestToolInputSchema = z base: z.string().describe("Base branch."), body: z .string() - .describe("Pull request body. Junior appends the conversation footer.") + .describe("Pull request body. The runtime appends the conversation footer.") .optional(), draft: z .boolean() @@ -250,7 +250,7 @@ function isDefinitiveGitHubPullRequestCreateRejection( return [400, 401, 404, 410, 422].includes(error.status); } -/** Build the GitHub REST create-PR request after Junior owns body/footer shaping. */ +/** Build the GitHub REST create-PR request after the runtime owns body/footer shaping. */ async function createGitHubPullRequestRequest( conversationId: string, input: CreateGitHubPullRequestInput, @@ -417,7 +417,7 @@ export function createGitHubPullRequestTool( readOnlyHint: false, }, description: - "Create a GitHub pull request with a runtime-owned Junior conversation footer. Use this instead of shelling out to gh pr create when creating pull requests.", + "Create a GitHub pull request with a runtime-owned conversation footer. Use this instead of shelling out to gh pr create when creating pull requests.", inputSchema: createPullRequestToolInputSchema, outputSchema: gitHubPullRequestOutputSchema, async execute( @@ -482,7 +482,7 @@ export function createGitHubPullRequestTool( ); } catch (error) { throw new Error( - "GitHub pull request was created, but Junior could not persist the completed pull request state.", + "GitHub pull request was created, but the runtime could not persist the completed pull request state.", { cause: error }, ); } diff --git a/packages/junior-github/src/tools/footer.ts b/packages/junior-github/src/tools/footer.ts index 887dc02de3..3b28085014 100644 --- a/packages/junior-github/src/tools/footer.ts +++ b/packages/junior-github/src/tools/footer.ts @@ -52,7 +52,7 @@ export function sentryConversationUrl( return `${parsed.protocol}//${parsed.hostname}${port}/organizations/${orgSlug}/${path}`; } -/** Build the Junior session footer, preferring a host-provided dashboard link. */ +/** Build the conversation session footer, preferring a host-provided dashboard link. */ export function githubConversationFooter( conversationId: string, dashboardUrl?: string, @@ -72,7 +72,7 @@ export function githubConversationFooter( return `${GITHUB_SESSION_FOOTER_START}\n${conversationMarker}\n\n--\n\n${sessionLinks}\n\n${GITHUB_SESSION_FOOTER_END}`; } -/** Read opaque native conversation ids from Junior-owned GitHub footers. */ +/** Read opaque native conversation ids from runtime-owned GitHub footers. */ export function githubConversationIds( body: string | null | undefined, ): string[] { @@ -134,7 +134,7 @@ export function githubLinkedIssues( } /** - * Append (or replace an existing) Junior session footer to a GitHub body string. + * Append (or replace an existing) conversation session footer to a GitHub body string. * Without a dashboard or Sentry link, returns the body unchanged (existing footer stripped). */ export function appendGitHubFooter( diff --git a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts index b95e2f610e..9077d1e1ff 100644 --- a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts +++ b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts @@ -10,7 +10,7 @@ import { botUserIdFromEmail } from "../webhooks/ownership.js"; /** * GraphQL-only GitHub mutation. There is no REST endpoint and no first-class - * `gh pr` subcommand yet, so this tool is the Junior substitute for: + * `gh pr` subcommand yet, so this tool is the runtime substitute for: * * ``` * gh api graphql \ @@ -18,7 +18,7 @@ import { botUserIdFromEmail } from "../webhooks/ownership.js"; * -F id=THREAD_ID * ``` * - * `repo` is required so Junior can bind the GraphQL operation to a repository; + * `repo` is required so the runtime can bind the GraphQL operation to a repository; * GraphQL has no repo path to derive that from. */ const inputSchema = z @@ -79,7 +79,7 @@ function githubError(payload: unknown): string { return "GitHub request failed"; } -/** Resolve one review thread after GitHub proves it belongs to a Junior-authored PR. */ +/** Resolve one review thread after GitHub proves it belongs to a bot-authored PR. */ export function createGitHubResolvePullRequestReviewThreadTool( ctx: { egress: PluginEgress }, botEmail: string | undefined, @@ -92,7 +92,7 @@ export function createGitHubResolvePullRequestReviewThreadTool( readOnlyHint: false, }, description: - "Resolve a GitHub pull request review thread. Use this instead of shelling out to `gh api graphql` for resolveReviewThread (GraphQL-only; no REST or `gh pr` equivalent). Only works on pull requests Junior authored.", + "Resolve a GitHub pull request review thread. Use this instead of shelling out to `gh api graphql` for resolveReviewThread (GraphQL-only; no REST or `gh pr` equivalent). Only works on pull requests the bot authored.", inputSchema, outputSchema, async execute(input): Promise { @@ -171,7 +171,7 @@ export function createGitHubResolvePullRequestReviewThreadTool( pullRequest.author?.databaseId === botUserId; if (!ownsPullRequest) { throw new PluginToolInputError( - "Junior can only resolve review threads on pull requests it authored.", + "This bot can only resolve review threads on pull requests it authored.", ); } if (thread.isResolved) { diff --git a/packages/junior-github/src/tools/update-issue.ts b/packages/junior-github/src/tools/update-issue.ts index 7be1aade51..e8a8940b87 100644 --- a/packages/junior-github/src/tools/update-issue.ts +++ b/packages/junior-github/src/tools/update-issue.ts @@ -30,7 +30,7 @@ const inputSchema = z .string() .optional() .describe( - "Replacement issue body. Junior appends requester attribution and the conversation footer.", + "Replacement issue body. The runtime appends requester attribution and the conversation footer.", ), state: z .enum(["open", "closed"]) @@ -95,7 +95,7 @@ function githubApiErrorMessage(payload: unknown): string { return "GitHub request failed"; } -/** Update mutable issue metadata while preserving Junior-owned body attribution. */ +/** Update mutable issue metadata while preserving runtime-owned body attribution. */ export function createGitHubUpdateIssueTool(ctx: { actor?: Actor; conversationId?: string; diff --git a/packages/junior-github/src/tools/update-pull-request.ts b/packages/junior-github/src/tools/update-pull-request.ts index cbbb60cf0d..7d3d550459 100644 --- a/packages/junior-github/src/tools/update-pull-request.ts +++ b/packages/junior-github/src/tools/update-pull-request.ts @@ -30,7 +30,7 @@ const inputSchema = z .string() .optional() .describe( - "Replacement pull request body. Junior appends requester attribution and the conversation footer.", + "Replacement pull request body. The runtime appends requester attribution and the conversation footer.", ), base: z .string() @@ -108,7 +108,7 @@ function githubApiErrorMessage(payload: unknown): string { return "GitHub request failed"; } -/** Update mutable PR metadata while preserving Junior-owned body attribution. */ +/** Update mutable PR metadata while preserving runtime-owned body attribution. */ export function createGitHubUpdatePullRequestTool(ctx: { actor?: Actor; conversationId?: string; diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index e1160b8d6e..9c70f656c3 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -1164,7 +1164,7 @@ Conversation: \`local:test:old-conversation\` { toolCallId: "call-completed-storage-fails" }, ), ).rejects.toThrow( - "GitHub issue was created, but Junior could not persist the completed issue state.", + "GitHub issue was created, but the runtime could not persist the completed issue state.", ); await expect( tool?.execute?.( diff --git a/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts b/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts index 20abfa1c55..2274ac7e72 100644 --- a/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts +++ b/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts @@ -105,7 +105,7 @@ describe("resolvePullRequestReviewThread", () => { ).rejects.toMatchObject({ name: "PluginToolInputError", message: - "Junior can only resolve review threads on pull requests it authored.", + "This bot can only resolve review threads on pull requests it authored.", }); expect(fetch).toHaveBeenCalledTimes(1); }); diff --git a/packages/junior-github/tests/update-issue.test.ts b/packages/junior-github/tests/update-issue.test.ts index 6ac55d573f..d4786f1055 100644 --- a/packages/junior-github/tests/update-issue.test.ts +++ b/packages/junior-github/tests/update-issue.test.ts @@ -47,7 +47,7 @@ describe("updateIssue", () => { } }); - it("updates issue metadata and preserves Junior-owned body metadata", async () => { + it("updates issue metadata and preserves runtime-owned body metadata", async () => { process.env.GITHUB_WEBHOOK_SECRET = "test-secret"; const { fetch, tool } = toolContext(); diff --git a/packages/junior-github/tests/update-pull-request.test.ts b/packages/junior-github/tests/update-pull-request.test.ts index 7654457f38..47a2b5a914 100644 --- a/packages/junior-github/tests/update-pull-request.test.ts +++ b/packages/junior-github/tests/update-pull-request.test.ts @@ -49,7 +49,7 @@ describe("updatePullRequest", () => { } }); - it("updates mutable pull request fields and preserves Junior-owned body metadata", async () => { + it("updates mutable pull request fields and preserves runtime-owned body metadata", async () => { process.env.GITHUB_WEBHOOK_SECRET = "test-secret"; const { fetch, tool } = toolContext(); From 390bfb16b2bb0a0b0d3a1872a3e531aaaae14da7 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:45:26 +0000 Subject: [PATCH 6/8] fix(lint): update tool error baseline after copy rename Keep the tool-error classification baseline in sync with the agent-name neutral create issue/PR failure messages. --- .../junior/scripts/tool-error-classification-baseline.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/junior/scripts/tool-error-classification-baseline.txt b/packages/junior/scripts/tool-error-classification-baseline.txt index 55ec6d844c..2657d23756 100644 --- a/packages/junior/scripts/tool-error-classification-baseline.txt +++ b/packages/junior/scripts/tool-error-classification-baseline.txt @@ -3,12 +3,12 @@ # Only keep true system/config/integrity failures here. # Format: relativePathnormalized throw statement packages/junior-github/src/tools/create-issue.ts throw new Error( "GitHub issue creation for this tool call has an uncertain pending result; refusing to create a duplicate issue.", ); -packages/junior-github/src/tools/create-issue.ts throw new Error( "GitHub issue was created, but Junior could not persist the completed issue state.", { cause: error }, ); +packages/junior-github/src/tools/create-issue.ts throw new Error( "GitHub issue was created, but the runtime could not persist the completed issue state.", { cause: error }, ); packages/junior-github/src/tools/create-issue.ts throw new Error("GitHub issue creation returned an invalid response."); packages/junior-github/src/tools/create-issue.ts throw new Error("Invalid GitHub createIssue idempotency state.", { cause: error, }); packages/junior-github/src/tools/create-pull-request.ts throw new Error( "GitHub pull request creation for this tool call has an uncertain pending result; refusing to create a duplicate pull request.", ); packages/junior-github/src/tools/create-pull-request.ts throw new Error( "GitHub pull request creation returned an invalid response.", ); -packages/junior-github/src/tools/create-pull-request.ts throw new Error( "GitHub pull request was created, but Junior could not persist the completed pull request state.", { cause: error }, ); +packages/junior-github/src/tools/create-pull-request.ts throw new Error( "GitHub pull request was created, but the runtime could not persist the completed pull request state.", { cause: error }, ); packages/junior-github/src/tools/create-pull-request.ts throw new Error("Invalid GitHub createPullRequest idempotency state.", { cause: error, }); packages/junior-github/src/tools/get-deployment.ts throw new Error(message); packages/junior-github/src/tools/get-pull-request.ts throw new Error(message); From 9885da81ef8dbbf708447b7c6468e3a98695819c Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:48:52 +0000 Subject: [PATCH 7/8] ref(egress): drop leftover complexity from credential simplification Remove dead review-thread repo-ref validation, skip the second-pass host transform recheck on 403 retry, and neutralize remaining agent-branded permission-denied copy. Co-Authored-By: David Cramer --- packages/junior-github/src/egress-policy.ts | 12 +- .../junior/src/chat/egress/credentialed.ts | 133 ++++++++---------- .../handlers/sandbox-egress-proxy.test.ts | 2 +- .../component/misc/sandbox-executor.test.ts | 6 +- .../tool-support/normalize-result.test.ts | 4 +- 5 files changed, 68 insertions(+), 89 deletions(-) diff --git a/packages/junior-github/src/egress-policy.ts b/packages/junior-github/src/egress-policy.ts index c048b4fb24..fd25f9dd24 100644 --- a/packages/junior-github/src/egress-policy.ts +++ b/packages/junior-github/src/egress-policy.ts @@ -394,15 +394,6 @@ function reviewThreadResolveRepository( return repository; } -function requireRepositoryRef(repository: string): void { - const [owner, name] = repository.split("/"); - if (!owner || !name) { - throw new EgressPolicyDenied( - "GitHub review thread resolution does not identify a target repository.", - ); - } -} - function isGitHubGraphqlMutation( method: string, upstreamUrl: URL, @@ -539,7 +530,6 @@ export async function githubGrantForEgress( ctx.request.bodyText, ); if (reviewThreadRepository) { - requireRepositoryRef(reviewThreadRepository); return grantForAccess( "write", "github.installation-write", @@ -555,7 +545,7 @@ export async function githubGrantForEgress( if (graphqlAccess) { if (graphqlAccess === "write") { throw new EgressPolicyDenied( - "GitHub GraphQL mutations are not enabled for Junior credentials.", + "GitHub GraphQL mutations are not enabled for runtime credentials.", ); } return grantForAccess( diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index 0b1cfe3708..899f578e8c 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -253,7 +253,7 @@ function permissionDeniedMessage( provider: string, grant: SandboxEgressCredentialLease["grant"], ): string { - return `${provider} returned HTTP 403 after Junior injected the ${grant.name} grant. Junior forwarded the request; this is not a local runtime block.`; + return `${provider} returned HTTP 403 after the runtime injected the ${grant.name} grant. The request was forwarded; this is not a local runtime block.`; } function isEgressAuthRequired(error: unknown): error is EgressAuthRequired { @@ -648,9 +648,7 @@ export async function executeCredentialedEgressRequest(input: { const recordPermissionDenied = deps.recordPermissionDenied ?? recordSandboxPermissionDenied; - const resolveLease = async (): Promise< - SandboxEgressCredentialLease | Response - > => { + async function resolveLease(): Promise { try { return await issueCredentialLease( provider, @@ -658,44 +656,44 @@ export async function executeCredentialedEgressRequest(input: { credentialContext, ); } catch (error) { - if (error instanceof SandboxEgressCredentialError) { - await recordAuthRequired({ - credentialContext, - provider: error.provider, - grant: error.grant, - kind: error.kind, - authorization: error.authorization, - message: error.message, - }); - const isAuthRequired = error.kind === "auth_required"; - logWarn( - isAuthRequired - ? "sandbox.egress.credential.needed" - : "sandbox.egress.credential.unavailable", - { - ...egressAttributes({ - egressId: activeEgressId, - grantAccess: error.grant.access, - grantName: error.grant.name, - grantReason: error.grant.reason, - host: upstreamUrl.hostname, - method: request.method, - path: upstreamUrl.pathname, - provider: error.provider, - status: 401, - }), - ...routingAttributes(request, upstreamUrl), - }, - ); - return authRequiredResponse({ - provider: error.provider, - grant: error.grant, - message: error.message, - }); + if (!(error instanceof SandboxEgressCredentialError)) { + throw error; } - throw error; + await recordAuthRequired({ + credentialContext, + provider: error.provider, + grant: error.grant, + kind: error.kind, + authorization: error.authorization, + message: error.message, + }); + const isAuthRequired = error.kind === "auth_required"; + logWarn( + isAuthRequired + ? "sandbox.egress.credential.needed" + : "sandbox.egress.credential.unavailable", + { + ...egressAttributes({ + egressId: activeEgressId, + grantAccess: error.grant.access, + grantName: error.grant.name, + grantReason: error.grant.reason, + host: upstreamUrl.hostname, + method: request.method, + path: upstreamUrl.pathname, + provider: error.provider, + status: 401, + }), + ...routingAttributes(request, upstreamUrl), + }, + ); + return authRequiredResponse({ + provider: error.provider, + grant: error.grant, + message: error.message, + }); } - }; + } let leaseOrResponse = await resolveLease(); if (leaseOrResponse instanceof Response) { @@ -733,11 +731,10 @@ export async function executeCredentialedEgressRequest(input: { const fetchImpl = deps.fetch ?? fetch; const body = bodyForGrantSelection ?? (await requestBodyBytes(request)); - // Retry once on upstream 403 after credential injection. Replace the cached - // lease first so a single intermittent provider denial does not fail the hop. - const maxAttempts = 2; + // One retry after upstream 403: clear/replace the cached lease, then try again. + let retriedAfter403 = false; - for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + while (true) { const headers = requestHeaders( request, lease, @@ -840,37 +837,31 @@ export async function executeCredentialedEgressRequest(input: { message: `Provider rejected the injected ${provider} credential.\n`, }); } - if (upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS) { + if ( + upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS && + !retriedAfter403 + ) { logWarn("sandbox.egress.upstream_auth.rejected", { ...attributes(lease, upstream.status, upstream), - "app.sandbox.egress.auth_attempt": attempt, }); await clearCredentialLease(provider, lease.grant, credentialContext); - if (attempt < maxAttempts) { - await upstream.body?.cancel().catch(() => undefined); - logWarn("sandbox.egress.upstream_auth.retrying", { - ...attributes(lease, upstream.status, upstream), - "app.sandbox.egress.auth_attempt": attempt, - }); - leaseOrResponse = await resolveLease(); - if (leaseOrResponse instanceof Response) { - return leaseOrResponse; - } - lease = leaseOrResponse; - if (!hasSandboxEgressLeaseTransformForHost(lease, upstreamUrl.hostname)) { - logWarn("sandbox.egress.transform.missing", { - ...attributes(lease, 403), - "app.sandbox.egress.transform_domains": lease.headerTransforms.map( - (transform) => transform.domain, - ), - }); - return Response.json( - { error: "Credential lease does not cover forwarded host" }, - { status: 403 }, - ); - } - continue; + await upstream.body?.cancel().catch(() => undefined); + logWarn("sandbox.egress.upstream_auth.retrying", { + ...attributes(lease, upstream.status, upstream), + }); + leaseOrResponse = await resolveLease(); + if (leaseOrResponse instanceof Response) { + return leaseOrResponse; } + lease = leaseOrResponse; + retriedAfter403 = true; + continue; + } + if (upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS) { + logWarn("sandbox.egress.upstream_auth.rejected", { + ...attributes(lease, upstream.status, upstream), + }); + await clearCredentialLease(provider, lease.grant, credentialContext); await recordPermissionDenied({ credentialContext, provider, @@ -913,6 +904,4 @@ export async function executeCredentialedEgressRequest(input: { headers: responseHeaders(upstream), }); } - - throw new Error("Credentialed egress exhausted auth attempts without a response"); } diff --git a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts index f0f768f90e..05bcf8b95d 100644 --- a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts @@ -787,7 +787,7 @@ describe("sandbox egress proxy composition", () => { provider: "sentry", grant: { name: "default", access: "read" }, message: - "sentry returned HTTP 403 after Junior injected the default grant. Junior forwarded the request; this is not a local runtime block.", + "sentry returned HTTP 403 after the runtime injected the default grant. The request was forwarded; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "sentry.io", diff --git a/packages/junior/tests/component/misc/sandbox-executor.test.ts b/packages/junior/tests/component/misc/sandbox-executor.test.ts index 87a60628f5..b37fa27f5d 100644 --- a/packages/junior/tests/component/misc/sandbox-executor.test.ts +++ b/packages/junior/tests/component/misc/sandbox-executor.test.ts @@ -1755,7 +1755,7 @@ describe("createTestSandbox", () => { access: "write", }, message: - "github returned HTTP 403 after Junior injected the user-write grant. Junior forwarded the request; this is not a local runtime block.", + "github returned HTTP 403 after the runtime injected the user-write grant. The request was forwarded; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "github.com", @@ -1904,7 +1904,7 @@ describe("createTestSandbox", () => { reason: "github.installation-write", }, message: - "github returned HTTP 403 after Junior injected the user-write grant. Junior forwarded the request; this is not a local runtime block.", + "github returned HTTP 403 after the runtime injected the user-write grant. The request was forwarded; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "github.com", @@ -1941,7 +1941,7 @@ describe("createTestSandbox", () => { reason: "github.installation-write", }, message: - "github returned HTTP 403 after Junior injected the user-write grant. Junior forwarded the request; this is not a local runtime block.", + "github returned HTTP 403 after the runtime injected the user-write grant. The request was forwarded; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "github.com", diff --git a/packages/junior/tests/unit/tool-support/normalize-result.test.ts b/packages/junior/tests/unit/tool-support/normalize-result.test.ts index 2244a9d69f..dcdf2d0240 100644 --- a/packages/junior/tests/unit/tool-support/normalize-result.test.ts +++ b/packages/junior/tests/unit/tool-support/normalize-result.test.ts @@ -123,7 +123,7 @@ describe("normalizeToolResult", () => { requirements: ["GitHub App Contents: write on the target repository"], }, message: - "github returned HTTP 403 after Junior injected the user-write grant. Junior forwarded the request; this is not a local runtime block.", + "github returned HTTP 403 after the runtime injected the user-write grant. The request was forwarded; this is not a local runtime block.", provider: "github", source: "upstream", status: 403, @@ -177,7 +177,7 @@ describe("normalizeToolResult", () => { requirements: ["GitHub App Contents: write on the target repository"], }, message: - "github returned HTTP 403 after Junior injected the user-write grant.", + "github returned HTTP 403 after the runtime injected the user-write grant.", provider: "github", source: "upstream", status: 403, From f2f88459074487c994ecc4171aa09829cbe6b06b Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:58:41 +0000 Subject: [PATCH 8/8] fix(egress): bind non-installation leases to delegated subject System runs with a delegated user subject must not share one user/broker lease cache entry. Key those grants by subject user id so a later hop cannot reuse another user's host-held credential. --- .../junior/src/chat/sandbox/egress/session.ts | 17 ++-- .../sandbox-egress-credentials.test.ts | 78 +++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/packages/junior/src/chat/sandbox/egress/session.ts b/packages/junior/src/chat/sandbox/egress/session.ts index 6c47768ed3..479f8572ad 100644 --- a/packages/junior/src/chat/sandbox/egress/session.ts +++ b/packages/junior/src/chat/sandbox/egress/session.ts @@ -46,21 +46,28 @@ export type { * Build the host lease cache key for one provider grant. * * Installation grants are shared across sandboxes. Other grants stay bound to - * the actor. Sandbox egress id and context token id authorize the hop only; - * they do not change which host credential to inject. + * the credential owner: the actor for normal user runs, or the delegated + * subject when a system run injects that user's token. Sandbox egress id and + * context token id authorize the hop only; they do not change which host + * credential to inject. */ function leaseKey( provider: string, grant: SandboxEgressCredentialLease["grant"], context: SandboxEgressCredentialContext, ): string { + if (grant.name.startsWith("installation-")) { + return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:shared`; + } const actor = context.credentials.actor; - const actorKey = grant.name.startsWith("installation-") - ? "shared" + const subject = + "subject" in context.credentials ? context.credentials.subject : undefined; + const ownerKey = subject + ? `subject:${subject.userId}` : "type" in actor ? `user:${actor.userId}` : `system:${actor.name}`; - return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:${actorKey}`; + return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:${ownerKey}`; } /** diff --git a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts index bcf5e52cee..8dfc16bc38 100644 --- a/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts +++ b/packages/junior/tests/unit/handlers/sandbox-egress-credentials.test.ts @@ -284,4 +284,82 @@ describe("sandboxEgressCredentialLease — credential error normalization", () = "sandbox-egress-lease:sentry:user-write:user:U999", ]); }); + + it("keeps user grants isolated by delegated subject on system runs", async () => { + hasEgressCredentialHooks.mockReturnValue(true); + issuePluginCredential.mockClear(); + const state = new Map(); + const stateStub = { + connect: vi.fn(), + get: vi.fn((key: string) => state.get(key)), + set: vi.fn((key: string, value: unknown) => state.set(key, value)), + delete: vi.fn((key: string) => state.delete(key)), + }; + getStateAdapter.mockReturnValue(stateStub); + issuePluginCredential.mockResolvedValue({ + type: "lease", + lease: { + expiresAt: new Date(Date.now() + 30 * 60_000).toISOString(), + headerTransforms: [ + { + domain: "sentry.io", + headers: { Authorization: "Bearer user-token" }, + }, + ], + }, + }); + const grant = { + grant: { + name: "user-write", + access: "write" as const, + }, + source: "plugin" as const, + }; + const firstSubject = { + credentials: { + actor: { platform: "system" as const, name: "resource-event" }, + subject: { + type: "user" as const, + userId: "U123", + allowedWhen: "event-task" as const, + taskId: "task-1", + binding: { + type: "event-task" as const, + plugin: "github", + taskId: "task-1", + signature: "sig-1", + }, + }, + }, + egressId: EGRESS_ID, + expiresAtMs: Date.now() + 60_000, + contextId: "ctx-subject-1", + }; + const secondSubject = { + ...firstSubject, + credentials: { + ...firstSubject.credentials, + subject: { + ...firstSubject.credentials.subject, + userId: "U999", + taskId: "task-2", + binding: { + ...firstSubject.credentials.subject.binding, + taskId: "task-2", + signature: "sig-2", + }, + }, + }, + contextId: "ctx-subject-2", + }; + + await sandboxEgressCredentialLease(PROVIDER, grant, firstSubject); + await sandboxEgressCredentialLease(PROVIDER, grant, secondSubject); + + expect(issuePluginCredential).toHaveBeenCalledTimes(2); + expect(stateStub.set.mock.calls.map(([key]) => key)).toEqual([ + "sandbox-egress-lease:sentry:user-write:subject:U123", + "sandbox-egress-lease:sentry:user-write:subject:U999", + ]); + }); });