Skip to content

feat(cli): unblock arkor dev; add --oauth and prompt to login#65

Merged
k-taro56 merged 11 commits intomainfrom
eng-314
Apr 29, 2026
Merged

feat(cli): unblock arkor dev; add --oauth and prompt to login#65
k-taro56 merged 11 commits intomainfrom
eng-314

Conversation

@k-taro56
Copy link
Copy Markdown
Contributor

Summary

  • arkor dev previously handed off to the interactive Auth0 flow whenever no credentials existed and the deployment advertised OAuth. That blocked Studio launch on a browser callback the user might not want right now — especially first-time npx create-arkor users who just want to see Studio open. Now arkor dev always bootstraps an anonymous session and, when OAuth is available, surfaces a one-line hint pointing at arkor login so users can upgrade to a real account whenever they choose.
  • arkor login gains a --oauth flag (vendor-neutral counterpart to --anonymous) and, when neither flag is passed in an interactive terminal, prompts the user to pick the flow. CI / non-TTY contexts default to anonymous since OAuth's PKCE loopback can't be satisfied without a browser; automation that wants OAuth must opt in explicitly with --oauth.
  • --oauth against a deployment that doesn't advertise Auth0 now throws instead of silently falling back to anonymous — the user is explicitly asking us to surface the misconfiguration.
  • --oauth and --anonymous together are rejected at both the CLI argument layer (main.ts) and inside runLogin itself, so programmatic callers can't bypass the guard.

Why "oauth" and not "auth0" or "sso"

Auth0 is an implementation detail; the user-facing flag describes the protocol. If we swap IdPs later the flag stays meaningful, and --oauth / --anonymous reads cleanly as a pair in --help.

Test plan

  • pnpm --filter arkor test (115 passing — 3 new runLogin tests, 1 new ensureCredentialsForStudio test for the OAuth-configured anon path)
  • pnpm --filter arkor exec tsc --noEmit
  • Manual on a deployment with OAuth configured:
    • arkor login (no flags, interactive) shows the prompt with anon as the highlighted default and OAuth above it
    • arkor login --oauth opens the PKCE flow
    • arkor login --anonymous issues an anon token
    • arkor login --oauth --anonymous errors before any I/O
    • arkor dev on a clean ~/.arkor prints the "run arkor login to sign in" hint and boots Studio without detouring through the browser
  • Manual on a deployment without OAuth: arkor login --oauth fails with the "OAuth is not configured" error

…ous credential handling

- Updated the login command to include an `--oauth` option for signing in via OAuth.
- Added validation to prevent using both `--oauth` and `--anonymous` flags simultaneously.
- Modified the `ensureCredentialsForStudio` function to bootstrap anonymous credentials when OAuth is available, providing a hint to use `arkor login` for a full session.
- Updated tests to cover new behavior for anonymous credential bootstrapping in OAuth-configured deployments.
- Introduced a new test file for the login command to validate the behavior of the `runLogin` function.
- Added tests to ensure proper handling of conflicting `--oauth` and `--anonymous` flags.
- Verified that the function throws an error when OAuth is requested without proper configuration.
- Ensured that the default behavior in non-interactive contexts correctly defaults to anonymous login.
- Modified the message in `ensureCredentialsForStudio` to instruct users to run `arkor login --oauth` for signing in, enhancing clarity on the OAuth login process.
Copilot AI review requested due to automatic review settings April 29, 2026 13:28
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ec614e813

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
Comment on lines 64 to 65
try {
const anon = await requestAnonymousToken(baseUrl, "cli");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Handle OAuth-only deployments in arkor dev

ensureCredentialsForStudio() now always calls requestAnonymousToken() even when OAuth is advertised, so first-run arkor dev fails on deployments that expose /v1/auth/cli/config but reject /v1/auth/anonymous (the Auth0-only case already documented in dev.test.ts). Before this change, that branch delegated to runLogin() and could succeed via PKCE; after this commit, affected users cannot start Studio unless they manually pre-login, which is a regression in startup behavior for those deployments.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the CLI authentication UX to prevent arkor dev from blocking on an OAuth browser callback, while adding an explicit --oauth login path and an interactive mode picker for arkor login.

Changes:

  • arkor dev now always bootstraps an anonymous session and, when OAuth is available, prints a hint about upgrading via arkor login.
  • arkor login adds --oauth and prompts users to choose OAuth vs anonymous when run interactively without flags; non-interactive contexts default to anonymous.
  • Adds/updates Vitest coverage for the new runLogin selection/guard behavior and the new ensureCredentialsForStudio OAuth-configured anon path.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/arkor/src/cli/main.ts Adds --oauth flag to login and rejects --oauth + --anonymous together at the CLI layer.
packages/arkor/src/cli/commands/login.ts Implements OAuth availability detection, interactive mode selection, and explicit failure when --oauth is requested but unavailable.
packages/arkor/src/cli/commands/login.test.ts New tests for mutual-exclusion guard, --oauth misconfiguration failure, and non-interactive default-to-anon behavior.
packages/arkor/src/cli/commands/dev.ts Changes Studio credential bootstrap to always attempt anonymous, with an informational hint when OAuth is advertised.
packages/arkor/src/cli/commands/dev.test.ts Adds coverage ensuring arkor dev bootstraps anonymous even when OAuth is configured.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

import { promptSelect, ui } from "../prompts";

export interface LoginOptions {
/** Force the OAuth (Auth0) flow even if `--anonymous` would otherwise be selected interactively. */
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc for LoginOptions.oauth still refers to “Auth0” even though the user-facing flag and messaging are now “OAuth”. Consider updating this comment to avoid mixing terminology (e.g., “Force the OAuth browser flow…”).

Suggested change
/** Force the OAuth (Auth0) flow even if `--anonymous` would otherwise be selected interactively. */
/** Force the OAuth browser flow even if `--anonymous` would otherwise be selected interactively. */

Copilot uses AI. Check for mistakes.
{
value: "oauth",
label: "OAuth (browser)",
hint: "Sign in to your account (requires an arkor.ai web account)",
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prompt hint hardcodes “requires an arkor.ai web account”. Since the CLI supports overriding the cloud API base URL (e.g. self-hosted / different domain), this could be misleading. Suggest making the hint deployment-neutral (e.g. “requires a web account” or similar).

Suggested change
hint: "Sign in to your account (requires an arkor.ai web account)",
hint: "Sign in to your account (requires a web account)",

Copilot uses AI. Check for mistakes.
Comment on lines +56 to +58
ui.log.info(
"No credentials on file — bootstrapping an anonymous session. Run `arkor login --oauth` to sign in to your account instead.",
);
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This arkor dev hint tells users to run arkor login --oauth, but the PR description says the intended upgrade path is arkor login (which now prompts interactively). Consider changing the message to point at arkor login (optionally mentioning --oauth as an advanced shortcut) to match the new UX and avoid over-prescribing flags.

Copilot uses AI. Check for mistakes.
Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
Comment on lines +76 to +88
// instead of being routed back to the interactive `runLogin`. Fail
// fast so the user sees the real cause and can re-run once
// connectivity is back.
// Studio when we couldn't reach the cloud-api at all. If
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch-block comment still says we only swallow transport failures when we confirmed the deployment “doesn't require Auth0”, but the code no longer performs that check (it only checks deploymentModeKnown). Please update the comment (and/or the condition) so the documentation matches the actual behavior and current “OAuth” terminology.

Copilot uses AI. Check for mistakes.
});
});

it("bootstraps anonymous credentials when Auth0 isn't configured", async () => {
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name still refers to “Auth0” (“when Auth0 isn't configured”) but the CLI/user-facing terminology is now “OAuth”. Renaming the test to “OAuth isn't configured” would keep terminology consistent with the code and flags.

Suggested change
it("bootstraps anonymous credentials when Auth0 isn't configured", async () => {
it("bootstraps anonymous credentials when OAuth isn't configured", async () => {

Copilot uses AI. Check for mistakes.
});

// `--oauth` is an explicit opt-in. If the deployment doesn't advertise
// Auth0, silently falling back to anon would mask a misconfiguration the
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments refer to “Auth0”, but the behavior being tested is now described externally as “OAuth” (and the flag is --oauth). Consider updating the comments to “OAuth” to keep terminology consistent and reduce confusion.

Suggested change
// Auth0, silently falling back to anon would mask a misconfiguration the
// OAuth, silently falling back to anon would mask a misconfiguration the

Copilot uses AI. Check for mistakes.
@soleil-colza soleil-colza self-requested a review April 29, 2026 13:34
Copy link
Copy Markdown
Contributor

@soleil-colza soleil-colza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMeow

…ents

- Updated the `ensureCredentialsForStudio` function to provide clearer guidance for users on OAuth-only deployments, suggesting the use of `arkor login --oauth` when anonymous token acquisition fails.
- Enhanced the error handling logic to differentiate between transport failures and OAuth-related issues, ensuring users receive appropriate feedback based on their deployment configuration.
- Adjusted test cases to validate the new behavior and messaging for anonymous credential handling in OAuth scenarios.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8cffe6c44f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
Comment on lines +114 to +118
if (!isTransportFailure && oauthAvailable) {
const cause = err instanceof Error ? err.message : String(err);
throw new Error(
`Failed to bootstrap an anonymous session (${cause}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
{ cause: err },
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve local write failures instead of OAuth-only hint

This branch rewrites all non-transport errors to an OAuth guidance message whenever oauthAvailable is true, but the same catch also handles local failures after token issuance (for example writeCredentials failing on read-only or permission-restricted $HOME). In that case arkor dev now reports a misleading "deployment may require sign-in" remediation instead of surfacing the actionable filesystem error, which makes recovery harder for users on locked-down environments.

Useful? React with 👍 / 👎.

…tudio

- Introduced a new test to ensure that filesystem errors from `writeCredentials` are not misrepresented as OAuth hints during the anonymous token acquisition process.
- Updated the `ensureCredentialsForStudio` function to clarify error handling, ensuring local filesystem errors are surfaced with their original messages.
- Enhanced the test suite to validate the behavior when `credentials.json` is incorrectly set as a directory, confirming that the appropriate error is thrown.
Copilot AI review requested due to automatic review settings April 29, 2026 18:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +247 to +257
// Codex P2 review on PR #65 — the OAuth-only wrap used to span the whole
// anon bootstrap, so fs errors from `writeCredentials` were also rewritten
// as "deployment may require sign-in", hiding the actionable fs cause.
// We force `writeCredentials` to fail by pre-creating
// `~/.arkor/credentials.json` as a directory (so `writeFile` raises
// EISDIR) and assert the fs error propagates intact.
it("does not rewrite fs errors from writeCredentials as OAuth hints", async () => {
mkdirSync(join(fakeHome, ".arkor", "credentials.json"), {
recursive: true,
});
globalThis.fetch = vi.fn(async (input) => {
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test intends to force writeCredentials to fail (EISDIR) by creating ~/.arkor/credentials.json as a directory, but that setup causes readCredentials() to throw EISDIR first (since it tries to read the same path) and the code never reaches writeCredentials. The test currently doesn’t validate the behavior described in the comment. Consider adjusting the setup to let readCredentials() succeed (no file) and then force writeCredentials() to fail (e.g., make ~/.arkor unwritable, or mock writeCredentials to throw).

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +76
// Interactive choice: when neither flag was passed, ask which mode to use.
// Non-interactive contexts (CI, piped stdout) default to anonymous via
// `initialValue` because OAuth requires a browser callback that CI can't
// satisfy — silently falling back to anon is safer than hanging on the
// PKCE loopback. Automation that wants OAuth must opt in with `--oauth`.
const mode = options.oauth
? "oauth"
: await promptSelect<"oauth" | "anonymous">({
message: "How would you like to sign in?",
options: [
{
value: "oauth",
label: "OAuth (browser)",
hint: "Sign in to your account (requires an arkor.ai web account)",
},
{
value: "anonymous",
label: "Anonymous",
hint: "Throwaway token, no account",
},
],
initialValue: "anonymous",
});
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runLogin({ oauth: true }) will attempt the PKCE loopback flow even in non-interactive/CI contexts, and startLoopbackServer().waitForCallback has no timeout. In CI this can hang indefinitely if --oauth is passed by mistake. Consider adding an explicit guard (e.g., when process.env.CI is set) to throw a clear error instructing users to omit --oauth or use --anonymous, rather than waiting forever.

Copilot uses AI. Check for mistakes.
Comment on lines +103 to +116
// OAuth-only deployments (`/v1/auth/cli/config` advertises Auth0 but
// `/v1/auth/anonymous` is disabled) used to be handled by delegating to
// `runLogin()` here. The new flow always tries anon first, so a
// permanent rejection of `/v1/auth/anonymous` would leave the user with
// a bare "Failed to acquire anonymous token (4xx)" error and no way
// forward. Wrap the error with an explicit pointer at `arkor login
// --oauth` so first-run users on those deployments still have a
// discoverable next step.
if (!isTransportFailure && oauthAvailable) {
const cause = err instanceof Error ? err.message : String(err);
throw new Error(
`Failed to bootstrap an anonymous session (${cause}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
{ cause: err },
);
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OAuth-only error wrapping triggers for any non-transport failure when oauthAvailable is true, including schema-validation failures (e.g., ZodError) or other backend bugs. That can mask the real root cause and mislead users into thinking they just need to run arkor login --oauth. Consider narrowing the wrap condition to only known “anon endpoint rejected” cases (e.g., based on HTTP status / a dedicated error type from requestAnonymousToken).

Copilot uses AI. Check for mistakes.
…o and login command

- Updated the `ensureCredentialsForStudio` test to validate that filesystem errors are correctly propagated and not misrepresented as OAuth hints.
- Introduced a new test in the `login` command to ensure that the `--oauth` option fails gracefully in non-interactive contexts, providing clear feedback to users.
- Enhanced error handling in the `ensureCredentialsForStudio` function to differentiate between anonymous token rejection and other errors, improving user guidance on OAuth usage.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
Comment on lines +117 to +121
if (err instanceof AnonymousTokenRejectedError && oauthAvailable) {
throw new Error(
`Failed to bootstrap an anonymous session (${err.message}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
{ cause: err },
);
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensureCredentialsForStudio wraps any AnonymousTokenRejectedError with an "deployment may require sign-in" hint when OAuth is configured. Since requestAnonymousToken throws this error for all non-2xx responses (including 5xx), a transient server error (e.g. 500) would be misreported as a sign-in requirement. Gate this wrap on status codes that actually indicate anonymous is disabled (typically 401/403/404 or broadly 4xx), and let 5xx surface unchanged.

Copilot uses AI. Check for mistakes.
Comment thread packages/arkor/src/core/credentials.ts Outdated
Comment on lines +31 to +37
* non-2xx status (i.e. the deployment specifically rejected our anon
* request — typically because anonymous tokens are disabled there).
*
* Distinct from generic Errors / ZodErrors so callers can react to
* "anon endpoint says no" without misclassifying transport failures,
* schema mismatches, or local fs errors. `arkor dev` uses this to
* decide whether to surface the `arkor login --oauth` upgrade hint.
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc for AnonymousTokenRejectedError says it represents a deployment "specifically rejected" anon requests, but requestAnonymousToken throws it for any non-2xx (including 5xx). Either narrow the throw condition (e.g. only 4xx) or adjust the docs/name so callers don’t treat server-side failures as a policy rejection.

Suggested change
* non-2xx status (i.e. the deployment specifically rejected our anon
* request typically because anonymous tokens are disabled there).
*
* Distinct from generic Errors / ZodErrors so callers can react to
* "anon endpoint says no" without misclassifying transport failures,
* schema mismatches, or local fs errors. `arkor dev` uses this to
* decide whether to surface the `arkor login --oauth` upgrade hint.
* non-2xx status while requesting an anonymous token.
*
* This covers explicit deployment-side rejection (for example, anonymous
* tokens being disabled) as well as other unsuccessful HTTP responses
* such as transient server-side failures. It remains distinct from
* transport failures, schema mismatches, or local fs errors so callers
* can handle "the anon token endpoint returned an HTTP error" separately.

Copilot uses AI. Check for mistakes.
});

afterEach(() => {
process.env.HOME = ORIG_HOME;
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In afterEach, restoring process.env.HOME via assignment can accidentally set it to the string "undefined" when the original value was unset. Mirror the pattern used for CI/ARKOR_CLOUD_API_URL (set when defined, otherwise delete process.env.HOME) to avoid leaking env state across tests/runs.

Suggested change
process.env.HOME = ORIG_HOME;
if (ORIG_HOME !== undefined) process.env.HOME = ORIG_HOME;
else delete process.env.HOME;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c944eba92

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 60 to 63
if (options.oauth && !isInteractive()) {
throw new Error(
"--oauth requires an interactive terminal (browser callback). Use --anonymous in CI / non-TTY contexts.",
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow explicit OAuth login in non-TTY sessions

The new if (options.oauth && !isInteractive()) guard blocks all explicit OAuth logins whenever stdout is not a TTY, including cases like arkor login --oauth --no-browser | tee ... where the user can still complete the browser callback manually. Because isInteractive() treats piped output as non-interactive, this is a regression from the previous behavior (OAuth path still ran) and it contradicts the noBrowser headless-use intent by failing before any OAuth URL is emitted.

Useful? React with 👍 / 👎.

… and OAuth interactions

- Updated the `ensureCredentialsForStudio` function to only trigger OAuth hints for genuine 4xx errors, preventing misleading hints for transient 5xx failures.
- Improved tests for `ensureCredentialsForStudio` and `login` commands to validate the new error handling logic, ensuring clarity in non-interactive contexts and proper feedback for users.
- Adjusted environment variable handling in tests to ensure proper cleanup after each test run.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d67636e39

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +120 to +127
err instanceof AnonymousTokenRejectedError &&
err.status >= 400 &&
err.status < 500 &&
oauthAvailable
) {
throw new Error(
`Failed to bootstrap an anonymous session (${err.message}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
{ cause: err },
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fall back to OAuth login when anonymous bootstrap is rejected

On deployments that advertise OAuth but reject /v1/auth/anonymous (for example 401/403), ensureCredentialsForStudio now throws instead of delegating to runLogin, so arkor dev cannot start from a clean machine even though OAuth auth is available. Fresh evidence in this commit is the new oauthAvailable rejection path that wraps AnonymousTokenRejectedError and rethrows, plus the added test that asserts rejection for this case; this codifies the startup regression rather than recovering via PKCE.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
oauthAvailable
) {
throw new Error(
`Failed to bootstrap an anonymous session (${err.message}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapped error message here embeds the full inner err.message, which already includes the "Failed to acquire anonymous token (...)" prefix and a body snippet. This leads to a redundant/verbose message like "Failed to bootstrap... (Failed to acquire...)" and may surface noisy response bodies. Consider constructing a cleaner top-level message (e.g., mention just the status code) and rely on cause for the detailed underlying failure.

Suggested change
`Failed to bootstrap an anonymous session (${err.message}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,
`Failed to bootstrap an anonymous session (HTTP ${err.status}). This deployment may require sign-in — run \`arkor login --oauth\` and try again.`,

Copilot uses AI. Check for mistakes.
- Updated the error handling in `ensureCredentialsForStudio` to surface only the HTTP status code, avoiding redundant messaging that could leak sensitive information.
- Enhanced the test for `ensureCredentialsForStudio` to validate the new error messaging behavior, ensuring clarity and security in error reporting.
@k-taro56 k-taro56 self-assigned this Apr 29, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2459c4c2ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

it("does not rewrite fs errors from writeCredentials as OAuth hints", async () => {
const arkorDir = join(fakeHome, ".arkor");
mkdirSync(arkorDir, { recursive: true });
chmodSync(arkorDir, 0o555);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make fs-permission test independent of effective UID

This test assumes chmodSync(arkorDir, 0o555) will make writeCredentials fail with EACCES, but that assumption breaks when tests run as UID 0 (root), where writes still succeed and ensureCredentialsForStudio() resolves. In root-based CI/container environments this causes a false failure (rejects.toThrow(/EACCES/) never triggers), so the new test is not portable and can block merges despite correct runtime behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/arkor/src/cli/commands/dev.ts Outdated
* enabled, so it will retry on the first `/api/credentials` hit. This
* keeps `arkor dev` usable when the cloud-api is momentarily down.
* - Otherwise → always acquire an anonymous token. When the deployment
* advertises OAuth, surface a hint pointing at `arkor login` so the user
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function doc comment says the OAuth-configured case surfaces a hint pointing at arkor login, but the actual hint in the implementation points at arkor login --oauth. Suggest updating the comment to match the behavior (and the rationale a few lines below).

Suggested change
* advertises OAuth, surface a hint pointing at `arkor login` so the user
* advertises OAuth, surface a hint pointing at `arkor login --oauth` so the user

Copilot uses AI. Check for mistakes.
…tudio

- Updated the test for `ensureCredentialsForStudio` to skip execution for root users, as they bypass permission checks on Linux, which could lead to misleading assertions.
- Added a conditional check to determine if the process is running as root, ensuring the test suite remains robust across different environments.
- Modified the documentation in `dev.ts` to specify the correct command for upgrading to a real session, changing `arkor login` to `arkor login --oauth`.
- This change enhances clarity for users regarding the OAuth upgrade process during anonymous token acquisition.
@k-taro56 k-taro56 merged commit a4c499a into main Apr 29, 2026
11 checks passed
@k-taro56 k-taro56 deleted the eng-314 branch April 29, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants