From ebf2402cdea780f39f4b9d6fd8cdfa15375e24af Mon Sep 17 00:00:00 2001 From: Artem Kosolap Date: Mon, 3 Aug 2026 23:59:36 -0700 Subject: [PATCH] fix(api): refuse a shell-mangled path instead of sending it as a 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under Git Bash on Windows, `reply api /v3/whoami` never reached the CLI as typed. MSYS converts a leading-slash argument into a Windows path, so the request went to https://api.reply.io/C:/Program Files/Git/v3/whoami and came back 404 — indistinguishable from an endpoint that does not exist. Quoting does not help; quotes are removed before the conversion. That cost real damage, not just a detour. An agent exploring the product hit it on the prospect-search endpoints and told its user the feature "returns 404 on this account, so the beta isn't enabled for team 18185". The feature works on that account. An environment quirk became a confident, wrong statement about someone's own entitlements. One cause, not two. A dummy query string appeared to fix it only because a `?` stops MSYS treating the argument as a path. The path is now checked before anything is spent: it must start with `/`. A drive-prefixed path is recognised as shell mangling, the intended path is recovered from it so the fix can be quoted back verbatim rather than described, and the error names MSYS, says that quoting will not help, and gives the two remedies that do. Exit is 2 (usage), so a mangled path can never be mistaken for an upstream 404, which exits 1. PowerShell, cmd, macOS and Linux see no change. Found while fixing it: the top-level handler printed only `error.message` and dropped `hint` entirely, so every hint on a UsageError or RuntimeError was written and never seen — 47 of them, including the one that explains how to switch team. Api_error was unaffected because it bakes its hint into the message. Hints now print for all three, in Api_error's existing ` Hint: ...` shape, with continuation lines keeping their indentation so a quoted command stands out. `--json` output is unchanged; the hint was already in `to_json`. --- README.md | 16 ++++++++ src/__tests__/commands/api.test.ts | 63 +++++++++++++++++++++++++++++- src/__tests__/utils/errors.test.ts | 20 +++++++++- src/commands/api.ts | 44 ++++++++++++++++++++- src/index.ts | 6 ++- src/utils/errors.ts | 13 +++++- 6 files changed, 156 insertions(+), 6 deletions(-) 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};