diff --git a/CHANGELOG.md b/CHANGELOG.md index 8672bd3..0901d6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,11 @@ The monorepo ships **one shared train version** across all eleven packages, so u ### Added -- Manual Cursor Cloud Agent reviews can use the same Greptile-like `caeliq-reviewer` harness as the Cursor provider: isolated PR checks, structured preview approval, and optional `--token-env` publication. +- Manual Cursor Cloud Agent reviews can use the same Greptile-like `caeliq-reviewer` harness as the Cursor provider: isolated PR checks, structured preview approval, and optional `--token-env` on `post`, `threads`, and `complete-resolutions.py`. ### Fixed +- `post`, `threads`, and `complete-resolutions.py` send one GitHub credential on every `gh` call: the `--token-env` personal access token when that option is set, otherwise the App installation token. The named token wins over an ambient `GH_TOKEN` or `GITHUB_TOKEN`. - Cloud Agent setup copies `bun` and `bunx` onto `/usr/bin`, so non-interactive shells and the isolated-check sandbox can run them without `~/.bashrc`. - The isolated-check sandbox names the program it could not start. - `ocp-dev` DSH local wiring generates Host Typert declarations before host `tsc`, so package `./remote` exports resolve and leftover empty package dirs no longer block a full reshim. diff --git a/docs/pr-review.md b/docs/pr-review.md index 5a3c13c..ff1cf18 100644 --- a/docs/pr-review.md +++ b/docs/pr-review.md @@ -22,7 +22,7 @@ Contributor pull requests use `scripts/isolated-check.sh` below. Interactive sto ## Manual review -GitHub authentication for posting is a GitHub App installation token. `post` reads `REVIEW_APP_ID` and `REVIEW_APP_PRIVATE_KEY`, or `REVIEW_APP_PRIVATE_KEY_FILE` instead of the inline key. It signs a short-lived JWT and exchanges it for that app's installation token. Set `REVIEW_APP_INSTALLATION_ID` to the installation, or leave it unset to use the installation on the pull request's repository. The review author is the app's bot account. A client secret and the Cloud Agent `gh` login are not used for the review request. Pass `--token-env NAME` to `post` or `complete-resolutions.py` to use a GitHub personal access token (`ghp_` or `github_pat_`) from that environment variable instead of the App; the token is not a command-line argument. The same token is also used for the pull and review reads when it is the only GitHub credential. Without that option the App installation token remains the only publication credential. The private key stays out of the check boundary below. +GitHub authentication for posting is a GitHub App installation token. `post` reads `REVIEW_APP_ID` and `REVIEW_APP_PRIVATE_KEY`, or `REVIEW_APP_PRIVATE_KEY_FILE` instead of the inline key. It signs a short-lived JWT and exchanges it for that app's installation token. Set `REVIEW_APP_INSTALLATION_ID` to the installation, or leave it unset to use the installation on the pull request's repository. The review author is the app's bot account. A client secret and the Cloud Agent `gh` login are not used for the review request. Pass `--token-env NAME` to `post`, `threads`, or `complete-resolutions.py` to use a GitHub personal access token (`ghp_` or `github_pat_`) from that environment variable instead of the App; the token is not a command-line argument. That credential is sent on every `gh` call the command makes, including thread listing and the reads before publication, and it wins over an ambient `GH_TOKEN` or `GITHUB_TOKEN`. Without that option the App installation token is the credential for those calls. The private key stays out of the check boundary below. Launch a Cursor Cloud Agent on this repository with this instruction, substituting the pull request: @@ -176,7 +176,7 @@ URL is not accepted as a review receipt; the send remains unconfirmed and is not automatically repeated. Every publication still requires the approved preview. Readback checks do not authorize additional GitHub mutations. -`approve` records the preview digest, base, head, and runtime locally, and only when the file and the frozen preview are still those bytes. `approve` and `post` take the rendered payload, the same file `stamp` and `show` froze. `post` takes the publication lock and then reads the approval and the payload again. It reads them again after each read-only GitHub lookup. Each read must be the same pull request, base, head, runtime, identity, preview digest, and approval generation as the approval that was current when the lock was acquired. A denial or a different approval is not sent, including a replacement that would be valid on its own. After the reviews lookup, and immediately before it sends, `post` reads the pull request again and then reads the pinned approval and payload again. It mints the GitHub App installation token and reads that approval once more. A different base or head stops the send and requires a refreshed review and a new approval. A push that lands after that read can still be published on the approved commit. A request that was already sent is not cancelled. Two posts of the same payload share one lock. When a send does not confirm delivery, a later `post` checks whether the review is visible and does not send a second request. A response that shows GitHub rejected the request can be retried. A returned review URL has to name the approved pull request; otherwise the send stays unconfirmed and is not repeated. A review id that was already confirmed is not posted again. A later `post` returns that review and its comment URLs, or stops when the id is no longer listed, even if the summary no longer contains the marker. The checkpoint keeps the review id and comment ids. The review is a `COMMENT` on the approved commit. Comment URLs come from the pull request's review-comments list. `deny` records a refusal and `post` then makes no GitHub call: +`approve` records the preview digest, base, head, and runtime locally, and only when the file and the frozen preview are still those bytes. `approve` and `post` take the rendered payload, the same file `stamp` and `show` froze. `post` takes the publication lock and then reads the approval and the payload again. It reads them again after each read-only GitHub lookup. Each read must be the same pull request, base, head, runtime, identity, preview digest, and approval generation as the approval that was current when the lock was acquired. A denial or a different approval is not sent, including a replacement that would be valid on its own. Before the first GitHub lookup, `post` selects one credential, the `--token-env` token or one GitHub App installation token, and sends that credential on every GitHub call. After the reviews lookup, and immediately before it sends, `post` reads the pull request again and then reads the pinned approval and payload again. A different base or head stops the send and requires a refreshed review and a new approval. A push that lands after that read can still be published on the approved commit. A request that was already sent is not cancelled. Two posts of the same payload share one lock. When a send does not confirm delivery, a later `post` checks whether the review is visible and does not send a second request. A response that shows GitHub rejected the request can be retried. A returned review URL has to name the approved pull request; otherwise the send stays unconfirmed and is not repeated. A review id that was already confirmed is not posted again. A later `post` returns that review and its comment URLs, or stops when the id is no longer listed, even if the summary no longer contains the marker. The checkpoint keeps the review id and comment ids. The review is a `COMMENT` on the approved commit. Comment URLs come from the pull request's review-comments list. `deny` records a refusal and `post` then makes no GitHub call: ```bash python3 scripts/review-publish.py deny "$review_dir/approval.json" @@ -309,7 +309,7 @@ Keep the existing `REVIEW_APP_ID`, `REVIEW_APP_PRIVATE_KEY` (or owner-only configuration. Store the inline PEM as a Cloud Agent Runtime Secret. Install the App on this repository with **Pull requests: Read and write** and disable webhooks for this manually initiated workflow. A client secret or ambient -user token is not a publishing credential unless `post` or +user token is not a publishing credential unless `post`, `threads`, or `complete-resolutions.py` is given `--token-env NAME` for a personal access token in that environment variable. diff --git a/scripts/complete-resolutions.py b/scripts/complete-resolutions.py index 1cbd423..32525d4 100755 --- a/scripts/complete-resolutions.py +++ b/scripts/complete-resolutions.py @@ -13,10 +13,11 @@ python3 scripts/complete-resolutions.py python3 scripts/complete-resolutions.py --token-env NAME -Without --token-env, resolution uses the GitHub App installation token. +Without --token-env, every gh call uses one GitHub App installation token. --token-env NAME reads a GitHub personal access token (ghp_ or github_pat_) -from that environment variable and uses it for resolveReviewThread. The token -value is never taken from the command line. +from that environment variable and sends it as GH_TOKEN on every gh call, +including the review lookup and resolveReviewThread. The token value is never +taken from the command line. An ambient GH_TOKEN does not replace it. """ from __future__ import annotations @@ -52,9 +53,9 @@ def main() -> None: metavar="NAME", help=( "read a GitHub personal access token (ghp_ or github_pat_) from environment " - "variable NAME and use it for resolveReviewThread. NAME is the variable's name; " - "do not pass the token on the command line. Omit this option to mint a GitHub App " - "installation token instead." + "variable NAME and send it as GH_TOKEN on every gh call, including the review " + "lookup and resolveReviewThread. NAME is the variable's name; do not pass the token " + "on the command line. Omit this option to mint a GitHub App installation token instead." ), ) arguments = parser.parse_args() @@ -78,8 +79,7 @@ def main() -> None: lock = lock_path.open("a+") try: fcntl.flock(lock, fcntl.LOCK_EX) - if arguments.token_env is not None: - publisher.personal_access_token(arguments.token_env) + publisher.begin_publication(str(owner), str(repo), arguments.token_env) marker = f"" existing = publisher.matching_review(publisher.reviews_of(owner, repo, pull), marker) journal = publisher.read_json_object(checkpoint_path) diff --git a/scripts/review-publish.py b/scripts/review-publish.py index daf2e52..74f17ef 100755 --- a/scripts/review-publish.py +++ b/scripts/review-publish.py @@ -11,13 +11,13 @@ different approval is not sent, even when the replacement is valid by itself. After the reviews lookup, and immediately before it sends, post reads the pull request again and then re-reads that pinned operation. -Without --token-env, it mints a GitHub App installation token and reads -the approval once more. The review author is that app's bot. A client -secret and the ambient gh login are not used. +Before the first GitHub lookup, it selects one GitHub App installation +token unless --token-env is set. The review author is that app's bot +when using the App. A client secret and the ambient gh login are not used. --token-env NAME instead reads a GitHub personal access token (ghp_ or github_pat_) from the environment variable NAME and uses that token for -the review POST and resolveReviewThread. The token value is never taken -from the command line. A moved base or head is not sent; refresh the +every gh call, including the review POST and resolveReviewThread. The +token value is never taken from the command line. A moved base or head is not sent; refresh the review and approve it again. A push after that read can still land on the approved commit. A request that was already sent is not cancelled. A send that does not confirm @@ -643,10 +643,14 @@ def redact_secret(text: str) -> str: # Names passed to --token-env. The named variable is removed from gh's -# environment and its value is redacted from printed errors. A read that has -# no other GitHub credential receives the value back as GH_TOKEN. The set is -# process-local; post is a short-lived command. +# environment and its value is redacted from printed errors. The publication +# credential, once chosen, is what gh receives. The set is process-local; +# post, threads, and complete-resolutions are short-lived commands. _hidden_env: set[str] = set() +# One credential for every gh call in this process: the --token-env PAT, or +# the App installation token. None until begin_publication selects it. +_publication_token: str | None = None +_publication_required = False ENV_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") PERSONAL_ACCESS_TOKEN = re.compile(r"^(?:ghp_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+)$") @@ -791,15 +795,18 @@ def publisher_env(token: str | None) -> dict[str, str]: env.pop("GH_DEBUG", None) env["GH_HOST"] = "github.com" env["GH_PROMPT_DISABLED"] = "1" - # Reads call gh without an explicit token and rely on GH_TOKEN / GITHUB_TOKEN. - # Hiding the selected PAT removes that credential when it is the only one, - # including when the named variable is GH_TOKEN or GITHUB_TOKEN itself. - if token is None and not any(env.get(name, "").strip() for name in ("GH_TOKEN", "GITHUB_TOKEN")): + # The publication credential and a --token-env PAT both win over ambient + # GH_TOKEN / GITHUB_TOKEN. An empty result is not an anonymous gh login. + if token is None: + token = _publication_token + if token is None: for name in _hidden_env: value = os.environ.get(name, "").strip() if value: token = value break + if token is None and _publication_required: + refuse_publication("No GitHub credential is available. GitHub was not called.") if token is not None: env["GH_TOKEN"] = token env["GITHUB_TOKEN"] = token @@ -1070,6 +1077,14 @@ def publication_token(owner: str, repo: str, token_env: str | None) -> str: return personal_access_token(token_env) +def begin_publication(owner: str, repo: str, token_env: str | None) -> str: + """Select the one credential for every later gh call in this process.""" + global _publication_token, _publication_required + _publication_required = True + _publication_token = publication_token(owner, repo, token_env) + return _publication_token + + def resolve_threads( payload_path: Path, approval_path: Path, @@ -1083,7 +1098,9 @@ def resolve_threads( return require_same_operation(payload_path, approval_path, pinned) owner, repo, pull = payload["owner"], payload["repo"], payload["pull"] - token = publication_token(str(owner), str(repo), token_env) + token = _publication_token + if token is None: + token = publication_token(str(owner), str(repo), token_env) require_same_operation(payload_path, approval_path, pinned) for action in actions: thread = checked_thread(payload, action, token) @@ -1705,8 +1722,7 @@ def post(payload_path: Path, approval_path: Path, token_env: str | None = None) print("The approval record was replaced. GitHub was not called.", file=sys.stderr) raise SystemExit(5) pinned = operation_key(payload, approval, digest) - if token_env is not None: - personal_access_token(token_env) + begin_publication(str(owner), str(repo), token_env) if isinstance(payload.get("result"), dict): inventory = thread_inventory(owner, repo, pull) require_same_operation(payload_path, approval_path, pinned) @@ -1747,10 +1763,9 @@ def post_locked( token_env: str | None = None, ) -> None: owner, repo, pull = payload["owner"], payload["repo"], payload["pull"] - if token_env is None: - app_id, pem = load_app_material() - else: - personal_access_token(token_env) + token = _publication_token + if not token: + refuse_publication("No GitHub credential is available. GitHub was not called.") approved_head = payload["head"] approved_base = payload["base"] marker = f"" @@ -1782,18 +1797,12 @@ def post_locked( ) try: # The reviews lookup is the last preparatory read. Fetch the remote - # revision, then take the publication credential and re-read the pinned - # approval. Without --token-env that credential is a new installation - # token. A push after this fetch is not covered. A changed revision or - # approval is not sent. + # revision again and re-read the pinned approval. The credential was + # selected once, before the first gh call. A push after this fetch is + # not covered. A changed revision or approval is not sent. fetched_head, fetched_base, readable = read_pull(owner, repo, pull) reject_moved_revision(fetched_head, fetched_base, readable, approved_head, approved_base) require_same_operation(payload_path, approval_path, pinned) - if token_env is None: - token = installation_token(str(owner), str(repo), app_id, pem) - else: - token = personal_access_token(token_env) - require_same_operation(payload_path, approval_path, pinned) except SystemExit: # The request was not sent. Drop the checkpoint so it is not an unsettled delivery. checkpoint_path.unlink(missing_ok=True) @@ -1945,17 +1954,17 @@ def main() -> None: "--token-env", metavar="NAME", help=( - "post only: read a GitHub personal access token (ghp_ or github_pat_) from " - "environment variable NAME and use it for the review POST and resolveReviewThread. " - "NAME is the variable's name; do not pass the token on the command line. " + "post and threads: read a GitHub personal access token (ghp_ or github_pat_) from " + "environment variable NAME and send it as GH_TOKEN on every gh call, including " + "thread reads. NAME is the variable's name; do not pass the token on the command line. " "Omit this option to mint a GitHub App installation token instead." ), ) args = parser.parse_args() if args.discard_instruction is not None and args.command != "cleanup": parser.error("--discard-instruction is only valid with cleanup") - if args.token_env is not None and args.command != "post": - parser.error("--token-env is only valid with post") + if args.token_env is not None and args.command not in {"post", "threads"}: + parser.error("--token-env is only valid with post and threads") if args.command == "auth-info": auth_info() elif args.command == "cleanup": @@ -1992,6 +2001,7 @@ def main() -> None: if args.command == "start": start_review(str(args.payload), str(args.approval), pull) elif args.command == "threads": + begin_publication(str(args.payload), str(args.approval), args.token_env) json.dump(thread_inventory(str(args.payload), str(args.approval), pull), sys.stdout, indent=2) sys.stdout.write("\n") else: diff --git a/test/review-publish.test.ts b/test/review-publish.test.ts index a7a126e..1de05a0 100644 --- a/test/review-publish.test.ts +++ b/test/review-publish.test.ts @@ -227,6 +227,8 @@ note_key() with (state / "calls").open("a", encoding="utf-8") as handle: handle.write(method + " " + endpoint + "\\n") token = os.environ.get("GH_TOKEN", "") +with (state / "call-auth").open("a", encoding="utf-8") as handle: + handle.write((hashlib.sha256(token.encode()).hexdigest() if token else "missing") + "\\n") if method == "GET": digest = hashlib.sha256(token.encode()).hexdigest() if token else "missing" with (state / "read-auth").open("a", encoding="utf-8") as handle: @@ -566,6 +568,25 @@ function tokenSha(value: string): string { return createHash("sha256").update(value).digest("hex") } +function callAuth(state: string): string[] { + const file = path.join(state, "call-auth") + return fs.existsSync(file) ? fs.readFileSync(file, "utf8").split("\n").filter(Boolean) : [] +} + +function credentialRows(state: string, stdout: string, stderr: string, secret: string) { + const calls = callsOf(state) + const auth = callAuth(state) + const data = calls.flatMap((call, index) => ( + call.includes("access_tokens") || call.endsWith("/installation") ? [] : [auth[index] ?? "missing"] + )) + return { + paired: auth.length === calls.length && calls.length > 0, + missing: auth.includes("missing"), + data, + leaked: `${stdout}${stderr}`.includes(secret), + } +} + function sleep(ms: number): void { Bun.sleepSync(ms) } @@ -1347,7 +1368,7 @@ describe("review publication", () => { expect(result.stderr).toContain("The approval record was replaced.") expect(postsOf(state)).toBe(0) expect(postBodies(state)).toBe("") - expect(callsOf(state).filter((line) => line.startsWith("POST "))).toEqual([]) + expect(callsOf(state).filter((line) => line.startsWith("POST ") && !line.includes("access_tokens"))).toEqual([]) } finally { stopProcess(child) fs.rmSync(directory, { recursive: true, force: true }) @@ -1824,8 +1845,8 @@ describe("review publication", () => { expect(stored[0]?.request.comments[0]?.path).toBe("scripts/review-publish.py") expect(stored[0]?.request.comments[0]?.body).toContain("High: Replacement approval is sent to the locked pull request") expect(callsOf(state).filter((line) => line.startsWith("POST "))).toEqual([ - "POST graphql", "POST app/installations/67890/access_tokens", + "POST graphql", "POST repos/oakimov/cursor-opencode-provider/pulls/156/reviews", ]) expect(fs.readFileSync(path.join(state, "review-auth"), "utf8")).toBe("app") @@ -1960,10 +1981,10 @@ describe("review publication", () => { expect(help.stdout).toContain("--token-env") expect(help.stdout).toContain("ghp_") expect(help.stdout).toContain("github_pat_") - expect(help.stdout).toContain("do not pass the token") + expect(help.stdout.replaceAll(/\s+/g, " ")).toContain("do not pass the token") const elsewhere = run(["show", "--token-env", "REVIEW_PUBLISH_TOKEN"], { REVIEW_APP_UNCONFIGURED: "1" }) expect(elsewhere.status).not.toBe(0) - expect(elsewhere.stderr).toContain("--token-env is only valid with post") + expect(elsewhere.stderr).toContain("--token-env is only valid with post and threads") expect(elsewhere.stdout + elsewhere.stderr).not.toContain("REVIEW_PUBLISH_TOKEN=") }) @@ -2058,6 +2079,187 @@ describe("review publication", () => { } }) + it("sends the publication credential on every threads and pre-publish gh call", () => { + const pat = "ghp_threadreadtokenvalue123456789" + const ambient = "ghp_ambientthreadtokenvalue12345" + const installation = "ghs_testinstallationtoken" + const thread = { + id: "PRRT_fixture0", + isResolved: false, + pullRequest: { number: 156, repository: { nameWithOwner: "oakimov/cursor-opencode-provider" } }, + comments: [{ + id: "original", + url: "https://github.com/oakimov/cursor-opencode-provider/pull/156#discussion_r10", + body: "Earlier review finding", + updatedAt: "2026-01-01T00:00:00Z", + }], + } + const directories: string[] = [] + const observed: Array> = [] + try { + const appFixture = freshReview("review-thread-app-") + directories.push(appFixture.directory) + fs.writeFileSync(path.join(appFixture.state, "threads.json"), JSON.stringify([thread])) + const appEnv = { PATH: `${appFixture.bin}:/usr/bin:/bin`, GH_STATE: appFixture.state, GH_TOKEN: ambient, GITHUB_TOKEN: ambient } + const appThreads = run(["threads", "oakimov", "cursor-opencode-provider", "156"], appEnv) + const appRows = credentialRows(appFixture.state, appThreads.stdout, appThreads.stderr, installation) + observed.push({ + name: "threads-app", + status: appThreads.status, + graphql: callsOf(appFixture.state).includes("POST graphql"), + paired: appRows.paired, + missing: appRows.missing, + authed: appRows.data.length > 0 && appRows.data.every(line => line === tokenSha(installation)), + leaked: appRows.leaked || `${appThreads.stdout}${appThreads.stderr}`.includes(ambient), + }) + + const patFixture = freshReview("review-thread-pat-") + directories.push(patFixture.directory) + fs.writeFileSync(path.join(patFixture.state, "threads.json"), JSON.stringify([thread])) + const patThreads = run( + ["threads", "oakimov", "cursor-opencode-provider", "156", "--token-env", "REVIEW_PUBLISH_TOKEN"], + { + PATH: `${patFixture.bin}:/usr/bin:/bin`, + GH_STATE: patFixture.state, + REVIEW_APP_UNCONFIGURED: "1", + REVIEW_PUBLISH_TOKEN: pat, + GH_TOKEN: ambient, + GITHUB_TOKEN: ambient, + }, + ) + const patRows = credentialRows(patFixture.state, patThreads.stdout, patThreads.stderr, pat) + observed.push({ + name: "threads-pat", + status: patThreads.status, + graphql: callsOf(patFixture.state).includes("POST graphql"), + minted: callsOf(patFixture.state).some(line => line.includes("access_tokens") || line.endsWith("/installation")), + paired: patRows.paired, + missing: patRows.missing, + authed: patRows.data.length > 0 && patRows.data.every(line => line === tokenSha(pat)), + leaked: patRows.leaked || `${patThreads.stdout}${patThreads.stderr}`.includes(ambient), + }) + + const missingFixture = freshReview("review-thread-missing-") + directories.push(missingFixture.directory) + const missing = run(["threads", "oakimov", "cursor-opencode-provider", "156"], { + PATH: `${missingFixture.bin}:/usr/bin:/bin`, + GH_STATE: missingFixture.state, + REVIEW_APP_UNCONFIGURED: "1", + GH_TOKEN: ambient, + GITHUB_TOKEN: ambient, + }) + observed.push({ + name: "threads-missing", + status: missing.status, + told: missing.stderr.includes("GitHub was not called.") && missing.stderr.includes("REVIEW_APP_ID"), + calls: callsOf(missingFixture.state).join("\n"), + leaked: `${missing.stdout}${missing.stderr}`.includes(ambient), + }) + } finally { + for (const directory of directories) fs.rmSync(directory, { recursive: true, force: true }) + } + expect(observed).toEqual([ + { name: "threads-app", status: 0, graphql: true, paired: true, missing: false, authed: true, leaked: false }, + { name: "threads-pat", status: 0, graphql: true, minted: false, paired: true, missing: false, authed: true, leaked: false }, + { name: "threads-missing", status: 4, told: true, calls: "", leaked: false }, + ]) + }) + + it("sends that same credential on the pre-publish thread lookup and resolution reads", () => { + const pat = "ghp_prepublishreadtokenvalue12345" + const ambient = "ghp_ambientprepublishtokenvalue1" + const installation = "ghs_testinstallationtoken" + const directories: string[] = [] + const observed: Array> = [] + try { + const appFixture = freshReview("review-prepublish-app-") + directories.push(appFixture.directory) + const { env } = resolutionDraft(appFixture, ["fixed"]) + fs.rmSync(path.join(appFixture.state, "calls"), { force: true }) + fs.rmSync(path.join(appFixture.state, "call-auth"), { force: true }) + expect(approve(appFixture.payload, appFixture.approval, env).status).toBe(0) + const posted = run(["post", appFixture.payload, appFixture.approval], { ...env, GH_TOKEN: ambient, GITHUB_TOKEN: ambient }) + const appRows = credentialRows(appFixture.state, posted.stdout, posted.stderr, installation) + observed.push({ + name: "post-app", + status: posted.status, + graphql: callsOf(appFixture.state).includes("POST graphql"), + paired: appRows.paired, + missing: appRows.missing, + authed: appRows.data.length > 0 && appRows.data.every(line => line === tokenSha(installation)), + leaked: appRows.leaked || `${posted.stdout}${posted.stderr}`.includes(ambient), + }) + + fs.rmSync(path.join(appFixture.state, "calls"), { force: true }) + fs.rmSync(path.join(appFixture.state, "call-auth"), { force: true }) + const appFinished = runComplete({ ...env, GH_TOKEN: ambient, GITHUB_TOKEN: ambient }, appFixture.payload, appFixture.approval) + const appCompleteRows = credentialRows(appFixture.state, appFinished.stdout, appFinished.stderr, installation) + observed.push({ + name: "complete-app", + status: appFinished.status, + reviews: callsOf(appFixture.state).some(line => line.endsWith("/reviews")), + paired: appCompleteRows.paired, + missing: appCompleteRows.missing, + authed: appCompleteRows.data.length > 0 && appCompleteRows.data.every(line => line === tokenSha(installation)), + leaked: appCompleteRows.leaked || `${appFinished.stdout}${appFinished.stderr}`.includes(ambient), + }) + + const patFixture = freshReview("review-prepublish-pat-") + directories.push(patFixture.directory) + const drafted = resolutionDraft(patFixture, ["fixed"]) + fs.rmSync(path.join(patFixture.state, "calls"), { force: true }) + fs.rmSync(path.join(patFixture.state, "call-auth"), { force: true }) + expect(approve(patFixture.payload, patFixture.approval, drafted.env).status).toBe(0) + const patPosted = run(["post", patFixture.payload, patFixture.approval, "--token-env", "REVIEW_PUBLISH_TOKEN"], { + ...drafted.env, + REVIEW_APP_UNCONFIGURED: "1", + REVIEW_PUBLISH_TOKEN: pat, + GH_TOKEN: ambient, + GITHUB_TOKEN: ambient, + }) + const patRows = credentialRows(patFixture.state, patPosted.stdout, patPosted.stderr, pat) + observed.push({ + name: "post-pat", + status: patPosted.status, + graphql: callsOf(patFixture.state).includes("POST graphql"), + minted: callsOf(patFixture.state).some(line => line.includes("access_tokens") || line.endsWith("/installation")), + paired: patRows.paired, + missing: patRows.missing, + authed: patRows.data.length > 0 && patRows.data.every(line => line === tokenSha(pat)), + leaked: patRows.leaked || `${patPosted.stdout}${patPosted.stderr}`.includes(ambient), + }) + + fs.rmSync(path.join(patFixture.state, "calls"), { force: true }) + fs.rmSync(path.join(patFixture.state, "call-auth"), { force: true }) + const finished = runComplete({ + ...drafted.env, + REVIEW_APP_UNCONFIGURED: "1", + REVIEW_PUBLISH_TOKEN: pat, + GH_TOKEN: ambient, + GITHUB_TOKEN: ambient, + }, "--token-env", "REVIEW_PUBLISH_TOKEN", patFixture.payload, patFixture.approval) + const completeRows = credentialRows(patFixture.state, finished.stdout, finished.stderr, pat) + observed.push({ + name: "complete-pat", + status: finished.status, + reviews: callsOf(patFixture.state).some(line => line.endsWith("/reviews")), + minted: callsOf(patFixture.state).some(line => line.includes("access_tokens") || line.endsWith("/installation")), + paired: completeRows.paired, + missing: completeRows.missing, + authed: completeRows.data.length > 0 && completeRows.data.every(line => line === tokenSha(pat)), + leaked: completeRows.leaked || `${finished.stdout}${finished.stderr}`.includes(ambient), + }) + } finally { + for (const directory of directories) fs.rmSync(directory, { recursive: true, force: true }) + } + expect(observed).toEqual([ + { name: "post-app", status: 0, graphql: true, paired: true, missing: false, authed: true, leaked: false }, + { name: "complete-app", status: 0, reviews: true, paired: true, missing: false, authed: true, leaked: false }, + { name: "post-pat", status: 0, graphql: true, minted: false, paired: true, missing: false, authed: true, leaked: false }, + { name: "complete-pat", status: 0, reviews: true, minted: false, paired: true, missing: false, authed: true, leaked: false }, + ]) + }) + it("resolves threads with the named personal access token", () => { const fixture = freshReview("review-pat-resolve-") const { directory, payload, approval, state } = fixture @@ -2065,6 +2267,8 @@ describe("review publication", () => { try { const { env } = resolutionDraft(fixture, ["fixed"]) expect(approve(payload, approval, env).status).toBe(0) + fs.rmSync(path.join(state, "calls"), { force: true }) + fs.rmSync(path.join(state, "call-auth"), { force: true }) const posted = run(["post", payload, approval, "--token-env", "GITHUB_CC"], { ...env, GITHUB_CC: token }) expect(posted.status, `${posted.stdout}\n${posted.stderr}`).toBe(0) expect(posted.stdout).toContain("THREAD_RESOLVED=PRRT_fixture0") @@ -2169,8 +2373,7 @@ describe("review publication", () => { expect(finished.stdout).toContain("THREAD_RESOLVED=PRRT_fixture0") expect(finished.stdout + finished.stderr).not.toContain(token) expect(fs.readFileSync(path.join(state, "resolve-token-sha256"), "utf8")).toBe(tokenSha(token)) - // The App post mints for the review and for its rejected resolution. The - // completion uses the named PAT and does not mint again. + // threads and the App post each mint once. The completion uses the named PAT. expect(callsOf(state).filter((line) => line.includes("access_tokens"))).toEqual([ "POST app/installations/67890/access_tokens", "POST app/installations/67890/access_tokens", @@ -2204,7 +2407,7 @@ async function expectNoPublishWhenRevisionMoves(kind: "head" | "base"): Promise< const duringLookup = callsOf(state) expect(duringLookup.filter((line) => line === pull)).toHaveLength(1) expect(duringLookup.some((line) => line.endsWith("/reviews"))).toBe(true) - expect(duringLookup.some((line) => line.startsWith("POST "))).toBe(false) + expect(duringLookup.some((line) => line.startsWith("POST ") && line.endsWith("/reviews"))).toBe(false) if (kind === "head") fs.writeFileSync(path.join(state, "head"), `${otherHead}\n`) else fs.writeFileSync(path.join(state, "base"), `${"d".repeat(40)}\n`) fs.writeFileSync(path.join(state, "release"), "go\n") @@ -2221,7 +2424,7 @@ async function expectNoPublishWhenRevisionMoves(kind: "head" | "base"): Promise< expect(pullGets).toHaveLength(2) expect(pullGets[0]!.index).toBeLessThan(reviewsAt) expect(pullGets[1]!.index).toBeGreaterThan(reviewsAt) - expect(calls.some((line) => line.startsWith("POST "))).toBe(false) + expect(calls.some((line) => line.startsWith("POST ") && line.endsWith("/reviews"))).toBe(false) const journalDir = path.join(directory, ".review-publish") const journals = fs.existsSync(journalDir) ? fs.readdirSync(journalDir).filter((name) => name.endsWith(".json")) : [] expect(journals).toEqual([]) diff --git a/test/review-scripts.test.ts b/test/review-scripts.test.ts index 99b07b8..889ddf5 100644 --- a/test/review-scripts.test.ts +++ b/test/review-scripts.test.ts @@ -1124,7 +1124,8 @@ print("SAW=" + str(found)) const result = run("bash", ["scripts/isolate-run.sh", "--src", src, "--", "not-a-real-program-a977"]) if (result.status === 2 && /ISOLATION=unavailable/.test(result.stderr)) return expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(127) - expect(result.stderr).toContain("cannot run not-a-real-program-a977:") + const diagnostic = process.platform === "darwin" ? "env: not-a-real-program-a977:" : "cannot run not-a-real-program-a977:" + expect(result.stderr).toContain(diagnostic) expect(result.stderr).not.toContain("Traceback") } finally { fs.rmSync(src, { recursive: true, force: true })