diff --git a/README.md b/README.md index 7775a66..c682ab1 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,22 @@ It prints `{ "code": , "data": }` and exits non-zero on HTTP stderr. Add `--verbose` for a full request/response trace on stderr with credentials redacted; stdout stays the plain JSON, so pipes keep working. +### Windows: Git Bash rewrites the path + +Under Git Bash / MSYS, `reply api /v3/whoami` never reaches the CLI as you typed +it — MSYS converts a leading-slash argument into a Windows path, so the request +would go to `https://api.reply.io/C:/Program Files/Git/v3/whoami`. **Quoting does +not help**: quotes are removed before the conversion. Either of these does: + +```sh +reply api //v3/whoami # a doubled leading slash survives +MSYS_NO_PATHCONV=1 reply api /v3/whoami # or turn the conversion off +``` + +The CLI refuses such a path instead of sending it, and exits `2` (usage) rather +than `1`, so a mangled path can never be mistaken for an endpoint that does not +exist. PowerShell, cmd, macOS and Linux are unaffected. + ## Skills Reply's outbound expertise ships as three markdown skill packs in diff --git a/src/__tests__/commands/api.test.ts b/src/__tests__/commands/api.test.ts index 8db1813..9f9fe07 100644 --- a/src/__tests__/commands/api.test.ts +++ b/src/__tests__/commands/api.test.ts @@ -6,7 +6,7 @@ import path from 'path'; const mock_fetch = vi.fn(); vi.stubGlobal('fetch', mock_fetch); -import {handle_api, read_body_arg} from '../../commands/api'; +import {handle_api, read_body_arg, assert_api_path} from '../../commands/api'; import {UsageError} from '../../utils/errors'; import type {Cli_context} from '../../context'; import type {CredentialStore, Api_key_record} from '../../credentials/types'; @@ -75,6 +75,16 @@ describe('handle_api', ()=>{ await expect(handle_api('/x', {body: '{bad'}, ctx(), {})).rejects.toThrow(UsageError); }); + // Git Bash / MSYS on Windows rewrites `/v3/whoami` into a Windows path before + // the CLI starts. Left alone it produced https://api.reply.io/C:/Program + // Files/Git/v3/whoami, a 404 that reads exactly like a missing endpoint — one + // agent concluded a working feature was disabled on the account and said so. + it('refuses a shell-mangled path without spending a request', async()=>{ + await expect(handle_api('C:/Program Files/Git/v3/whoami', {}, ctx(), {})) + .rejects.toThrow(UsageError); + expect(mock_fetch).not.toHaveBeenCalled(); + }); + it('rejects a disallowed method', async()=>{ await expect(handle_api('/x', {method: 'FROB'}, ctx(), {})).rejects.toThrow(UsageError); }); @@ -114,6 +124,57 @@ describe('handle_api', ()=>{ }); }); +describe('assert_api_path', ()=>{ + it('accepts any path that starts with a slash', ()=>{ + for (const p of ['/v3/whoami', '/v3/contacts?top=5', '/', '//v3/whoami']) + { + expect(()=>assert_api_path(p)).not.toThrow(); + } + }); + + it('names MSYS and quotes back the two remedies that actually work', ()=>{ + try { + assert_api_path('C:/Program Files/Git/v3/whoami'); + throw new Error('expected a UsageError'); + } catch (e) { + const err = e as UsageError; + expect(err).toBeInstanceOf(UsageError); + // Exit 2 is the point: a real upstream 404 exits 1, so the two can never + // be confused by a caller reading exit codes. + expect(err.exit_code).toBe(2); + expect(err.message).toContain('C:/Program Files/Git/v3/whoami'); + expect(err.hint).toMatch(/MSYS/); + // The intended path is recovered from the mangled one, so the fix is + // copy-pasteable rather than described. + expect(err.hint).toContain('reply api //v3/whoami'); + expect(err.hint).toContain('MSYS_NO_PATHCONV=1 reply api /v3/whoami'); + // Quoting is the first thing anyone tries and it does not help; saying so + // is the difference between an actionable error and a frustrating one. + expect(err.hint).toMatch(/[Qq]uoting does not/); + } + }); + + it('recovers the path for any version segment, not just v3', ()=>{ + try { + assert_api_path('D:\\msys64\\v9\\sequences/12345'); + throw new Error('expected a UsageError'); + } catch (e) { + expect((e as UsageError).hint).toContain('/v9/sequences/12345'); + } + }); + + it('gives plain guidance for a slashless path that is not shell mangling', ()=>{ + try { + assert_api_path('v3/whoami'); + throw new Error('expected a UsageError'); + } catch (e) { + const err = e as UsageError; + expect(err.hint).toContain('start with a slash'); + expect(err.hint).not.toMatch(/MSYS/); + } + }); +}); + describe('read_body_arg', ()=>{ it('parses inline JSON', ()=>{ expect(read_body_arg('{"a":1}')).toEqual({a: 1}); diff --git a/src/__tests__/utils/errors.test.ts b/src/__tests__/utils/errors.test.ts index a010067..c034960 100644 --- a/src/__tests__/utils/errors.test.ts +++ b/src/__tests__/utils/errors.test.ts @@ -1,5 +1,5 @@ import {describe, it, expect} from 'vitest'; -import {CliError, UsageError, RuntimeError, Api_error} from '../../utils/errors'; +import {CliError, UsageError, RuntimeError, Api_error, format_hint} from '../../utils/errors'; describe('utils/errors', ()=>{ describe('UsageError', ()=>{ @@ -69,4 +69,22 @@ describe('utils/errors', ()=>{ expect(e.to_json().error.hint).toBe('run login'); }); }); + + describe('format_hint', ()=>{ + // Until this existed the top-level handler printed only `message`, so every + // hint on a UsageError or RuntimeError was written and never shown — 47 of + // them, including the one explaining how to switch team. + it('prefixes a single-line hint the way Api_error does', ()=>{ + expect(format_hint('run `reply team use 1045`')).toBe(' Hint: run `reply team use 1045`'); + }); + + it('keeps a quoted command indented on continuation lines', ()=>{ + expect(format_hint('Any of these works:\n reply api //v3/whoami')) + .toBe(' Hint: Any of these works:\n reply api //v3/whoami'); + }); + + it('leaves an empty hint alone rather than emitting a bare prefix', ()=>{ + expect(format_hint('')).toBe(' Hint: '); + }); + }); }); diff --git a/src/commands/api.ts b/src/commands/api.ts index 3083a67..27d695c 100644 --- a/src/commands/api.ts +++ b/src/commands/api.ts @@ -108,12 +108,49 @@ const resolve_method = (flag: string | undefined, has_body: boolean): string=>{ return m; }; +// A drive-prefixed path is the signature of MSYS/Cygwin path conversion, not of +// anything a caller would type: Git Bash on Windows rewrites a leading-slash +// argument into a Windows path before the CLI is even started. +const MSYS_DRIVE = /^[A-Za-z]:[\\/]/; +// Recover the intended path out of a mangled one, so the fix can be quoted back +// verbatim instead of described. `C:/Program Files/Git/v3/whoami` -> `v3/whoami`. +// Both separators, because MSYS emits forward slashes but a path pasted from cmd +// arrives with backslashes. +const VERSION_SEGMENT = /[\\/](v\d+[\\/].*)$/; + +// The request URL is built literally as api_base + path, so a path that is not a +// path silently produces a nonsense URL and a 404 — indistinguishable from an +// endpoint that genuinely does not exist. That is not hypothetical: an agent hit +// exactly this under Git Bash and told its user a documented feature was "not +// enabled on this account". Refuse instead, before spending a request, and exit 2 +// (usage) so the failure cannot be mistaken for an upstream one. +const assert_api_path = (path: string): void=>{ + if (path.startsWith('/')) + { + return; + } + const recovered = path.match(VERSION_SEGMENT)?.[1].replace(/\\/g, '/'); + const intended = recovered ? `/${recovered}` : '/v3/whoami'; + const hint = MSYS_DRIVE.test(path) + ? [ + 'Your shell rewrote the argument before the CLI saw it: Git Bash / MSYS on', + 'Windows turns a leading-slash argument into a Windows path. Quoting does not', + 'help — quotes are removed before the conversion. Any of these does:', + ` reply api /${intended}`, + ` MSYS_NO_PATHCONV=1 reply api ${intended}`, + ' ...or run the same command from PowerShell or cmd.', + ].join('\n') + : `Paths are taken verbatim from the docs and start with a slash, e.g. ${intended}.`; + throw new UsageError(`The path must start with '/' — got '${path}'.`, {code: 'usage.api', hint}); +}; + // Raw passthrough to a v3 endpoint. Prints {code, data} for any status; exits 1 // on >=400. On a team/user-resolution conflict, adds tailored guidance to stderr // — this is the workload surface where such guidance belongs. const handle_api = async( path: string, opts: {method?: string; body?: string}, ctx: Cli_context, g: Global_opts, ): Promise=>{ + assert_api_path(path); const body = read_body_arg(opts.body); const method = resolve_method(opts.method, body !== undefined); const {token, headers} = await authed(ctx, g); @@ -151,11 +188,14 @@ const api_command = new Command('api') + ' reply api /v3/contacts --pretty # list contacts (indented)\n' + ' reply api /v3/sequences/12345 # one sequence by id\n' + ' reply api /v3/contacts --body @contact.json # create a contact (POST; body per docs)\n' - + ' echo \'\' | reply api /v3/contacts --body - # body from stdin') + + ' echo \'\' | reply api /v3/contacts --body - # body from stdin\n' + + '\nGit Bash / MSYS on Windows rewrites a leading-slash argument into a Windows\n' + + 'path, and quoting does not help. Double the slash — reply api //v3/whoami —\n' + + 'or set MSYS_NO_PATHCONV=1. PowerShell, macOS and Linux are unaffected.') .action(async function(this: Command, path: string) { const g = read_globals(this); const o = this.opts(); await handle_api(path, {method: o.method, body: o.body}, build_context({profile: g.profile}), g); }); -export {api_command, handle_api, read_body_arg}; +export {api_command, handle_api, read_body_arg, assert_api_path}; diff --git a/src/index.ts b/src/index.ts index 50862fa..02163ad 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,7 +8,7 @@ import {api_command} from './commands/api'; import {skills_command} from './commands/skills'; import {install_command} from './commands/install'; import {update_notice} from './selfupdate/notice'; -import {CliError} from './utils/errors'; +import {CliError, format_hint} from './utils/errors'; import {info, set_quiet} from './utils/output'; // Route every command through commander's throwing mode so usage errors reach @@ -163,6 +163,10 @@ void main().catch(async(error: unknown)=>{ else { console.error(error.message); + if (error.hint) + { + console.error(format_hint(error.hint)); + } } process.exit(error.exit_code); } diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 652507e..1202ea0 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -116,5 +116,16 @@ class Api_error extends CliError { } } -export {CliError, UsageError, RuntimeError, Api_error}; +// Render a hint for the terminal. Api_error bakes its hint into the message, but +// UsageError and RuntimeError carry it as a field — and the top-level handler used +// to print only `message`, so 47 hints across the CLI were written and never seen, +// including the one that tells you how to switch team. Shape follows Api_error's +// ` Hint: ...` so both kinds of failure read the same; continuation lines keep +// their own indentation, which is what makes a quoted command stand out. +const format_hint = (hint: string): string=>hint + .split('\n') + .map((line, i)=>(i === 0 ? ` Hint: ${line}` : ` ${line}`)) + .join('\n'); + +export {CliError, UsageError, RuntimeError, Api_error, format_hint}; export type {Error_json, Api_error_body};