fix: address github scope review feedback

Author: rikohomeless

packages/lib/src/usecases/auth-github.ts

@@ -21,7 +21,7 @@ import {
   normalizeGithubScopes
 } from "./github-scope-policy.js"
 import type { GithubTokenValidationResult } from "./github-token-validation.js"
-import { validateGithubToken } from "./github-token-validation.js"
+import { githubInvalidTokenMessage, validateGithubToken } from "./github-token-validation.js"
 import { resolvePathFromCwd } from "./path-helpers.js"
 import { withFsPathContext } from "./runtime.js"
 import { ensureStateDotDockerGitRepo } from "./state-repo-github.js"
@@ -204,7 +204,10 @@ export const runGithubRemoveDeleteRepoScope = (
 export const rejectGithubTokenWithRepositoryDeleteScope = (token: string): Effect.Effect<void, AuthError> =>
   validateGithubToken(token).pipe(
     Effect.flatMap((validation) => {
-      if (validation.status !== "valid" || validation.oauthScopes === null) {
+      if (validation.status === "invalid") {
+        return Effect.fail(new AuthError({ message: githubInvalidTokenMessage }))
+      }
+      if (validation.status === "unknown" || validation.oauthScopes === null) {
         return Effect.fail(new AuthError({ message: githubUnverifiedTokenScopesMessage }))
       }
       return hasGithubRepositoryDeleteScope(validation.oauthScopes)

packages/lib/src/usecases/github-scope-policy.ts

@@ -1,4 +1,4 @@
-export const defaultGithubScopes: ReadonlyArray<string> = ["repo", "workflow", "read:org"]
+export const defaultGithubScopes: ReadonlyArray<string> = Object.freeze(["repo", "workflow", "read:org"])
 export const githubRepositoryDeleteScope = "delete_repo"
 export const githubForbiddenDeleteRepoScopeMessage = [
   "GitHub auth token includes forbidden OAuth scope: delete_repo.",

packages/lib/tests/usecases/auth-container-paths.test.ts

@@ -190,7 +190,7 @@ describe("auth container paths", () => {
         const recorded: Array<RecordedCommand> = []
         const executor = makeFakeExecutor(recorded)
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse("repo, workflow, read:org"))
+          Effect.runPromise(Effect.succeed(githubUserResponse("repo, workflow, read:org")))
         )
 
         yield* _(
@@ -287,7 +287,7 @@ describe("auth container paths", () => {
         const recorded: Array<RecordedCommand> = []
         const executor = makeFakeExecutor(recorded)
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse("repo, workflow"))
+          Effect.runPromise(Effect.succeed(githubUserResponse("repo, workflow")))
         )
 
         yield* _(
@@ -329,7 +329,7 @@ describe("auth container paths", () => {
         const recorded: Array<RecordedCommand> = []
         const executor = makeFakeExecutor(recorded)
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse("repo, delete_repo"))
+          Effect.runPromise(Effect.succeed(githubUserResponse("repo, delete_repo")))
         )
 
         const failure = yield* _(
@@ -371,7 +371,7 @@ describe("auth container paths", () => {
         const recorded: Array<RecordedCommand> = []
         const executor = makeFakeExecutor(recorded)
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse(null))
+          Effect.runPromise(Effect.succeed(githubUserResponse(null)))
         )
 
         const failure = yield* _(
@@ -411,7 +411,7 @@ describe("auth container paths", () => {
         const fs = yield* _(FileSystem.FileSystem)
         const envPath = `${root}/.docker-git/.orch/env/global.env`
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse("repo, delete_repo"))
+          Effect.runPromise(Effect.succeed(githubUserResponse("repo, delete_repo")))
         )
 
         const failure = yield* _(
@@ -448,7 +448,7 @@ describe("auth container paths", () => {
         const fs = yield* _(FileSystem.FileSystem)
         const envPath = `${root}/.docker-git/.orch/env/global.env`
         const fetchMock = vi.fn<typeof globalThis.fetch>(() =>
-          Promise.resolve(githubUserResponse(null))
+          Effect.runPromise(Effect.succeed(githubUserResponse(null)))
         )
 
         const failure = yield* _(

packages/lib/tests/usecases/github-scope-policy.test.ts

@@ -1,3 +1,4 @@
+import fc from "fast-check"
 import { describe, expect, it } from "vitest"
 
 import {
@@ -8,6 +9,21 @@ import {
 } from "../../src/usecases/github-scope-policy.js"
 
 describe("github scope policy", () => {
+  const deleteRepoScopeArbitrary = fc.constantFrom("delete_repo", "DELETE_REPO", "Delete_Repo", " delete_repo ")
+  const separatorArbitrary = fc.constantFrom(",", " ", "\n", "\t", ", ", " , ")
+  const scopeTokenArbitrary = fc.oneof(fc.string(), deleteRepoScopeArbitrary)
+  const scopeListInputArbitrary = fc.array(scopeTokenArbitrary, { maxLength: 20 }).chain((scopes) =>
+    fc.array(separatorArbitrary, { minLength: scopes.length, maxLength: scopes.length }).map((separators) =>
+      scopes.map((scope, index) => `${scope}${separators[index] ?? ""}`).join("")
+    )
+  )
+  const nullableScopeInputArbitrary = fc.oneof(fc.constant(null), fc.constant(undefined), fc.string(), scopeListInputArbitrary)
+  const forbiddenOnlyInputArbitrary = fc.array(deleteRepoScopeArbitrary, { minLength: 1, maxLength: 20 }).chain((scopes) =>
+    fc.array(separatorArbitrary, { minLength: scopes.length, maxLength: scopes.length }).map((separators) =>
+      scopes.map((scope, index) => `${scope}${separators[index] ?? ""}`).join("")
+    )
+  )
+
   it("preserves default safe scopes", () => {
     expect(normalizeGithubScopes(null)).toEqual(defaultGithubScopes)
     expect(normalizeGithubScopes("")).toEqual(defaultGithubScopes)
@@ -25,6 +41,21 @@ describe("github scope policy", () => {
     expect(normalizeGithubScopes("delete_repo DELETE_REPO")).toEqual(defaultGithubScopes)
   })
 
+  it("preserves the no-delete-repo invariant for arbitrary scope inputs", () => {
+    fc.assert(
+      fc.property(nullableScopeInputArbitrary, (input) => {
+        expect(
+          normalizeGithubScopes(input).some((scope) => scope.trim().toLowerCase() === "delete_repo")
+        ).toBe(false)
+      })
+    )
+    fc.assert(
+      fc.property(forbiddenOnlyInputArbitrary, (input) => {
+        expect(normalizeGithubScopes(input)).toEqual(defaultGithubScopes)
+      })
+    )
+  })
+
   it("parses GitHub OAuth scope headers and detects repository deletion", () => {
     expect(parseGithubOauthScopesHeader(null)).toBe(null)
     expect(parseGithubOauthScopesHeader(undefined)).toBe(null)