fix(publish): address Claude review on PR #1425

- AbortError → TimeoutError: AbortSignal.timeout() throws a
  DOMException with name 'TimeoutError', not Error{name:'AbortError'}.
  Match the pattern used in core/embeddings/http-client.ts so the
  user-facing "timed out after 15000ms" message actually fires. Update
  the regression test to throw a real DOMException — the previous fake
  was a false-green.
- isValidOwnerRepo: forbid trailing hyphen in the owner segment.
  GitHub rejects this at account-creation time; allowing it here meant
  hand-typed --id values like 'my-org-/repo' would pass our regex and
  422 from GitHub.
- Add publish-command coverage to cli-index-help.test.ts (asserts on
  --id, --skip-git, the registry name, and the token env var) and
  cli-commands.test.ts (asserts publishCommand is exported as a
  function). Catches accidental command-registration deletion.
This commit is contained in:
Gergo Magyar 2026-05-09 08:54:00 +01:00
parent ae16a99d06
commit 7d05f03add
5 changed files with 45 additions and 9 deletions

View file

@ -65,13 +65,15 @@ export function buildUqDispatchPayload(id: string): UqDispatchPayload {
* naming rules are looser, but we want to catch local paths
* (`/Users/...`), bare slugs (`my-repo`), and accidental whitespace.
*
* Tightened (LOW 8) to match GitHub's published slug rules:
* owner: starts with alnum, then alnum/hyphen only — no underscore,
* no dot. Length cap 39.
* Matches GitHub's published slug rules:
* owner: starts with alnum, then alnum/hyphen only, must end with
* alnum (no trailing hyphen — GitHub rejects this at account
* creation, so a `my-org-/repo` input would otherwise pass us
* and 422 from GitHub). No underscore, no dot. Length cap 39.
* repo: any of alnum/dot/hyphen/underscore. Length cap 100.
*/
export function isValidOwnerRepo(id: string): boolean {
return /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})\/[A-Za-z0-9._-]{1,100}$/.test(id);
return /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?\/[A-Za-z0-9._-]{1,100}$/.test(id);
}
/**

View file

@ -134,7 +134,14 @@ export const publishCommand = async (
signal: AbortSignal.timeout(DISPATCH_TIMEOUT_MS),
});
} catch (err) {
if (err instanceof Error && err.name === 'AbortError') {
// `AbortSignal.timeout()` throws a `DOMException` with `name ===
// 'TimeoutError'` on Node 18.14+ (and on browsers/Bun). It is NOT
// a plain `AbortError`. Match the pattern used in
// gitnexus/src/core/embeddings/http-client.ts so the user sees the
// targeted "timed out" message instead of a generic "operation
// was aborted".
const isTimeout = err instanceof DOMException && err.name === 'TimeoutError';
if (isTimeout) {
cliError(
`[understand-quickly] dispatch timed out after ${DISPATCH_TIMEOUT_MS}ms. ` +
`Check network access to api.github.com and retry.`,

View file

@ -10,6 +10,9 @@ vi.mock('../../src/cli/mcp.js', () => ({
vi.mock('../../src/cli/setup.js', () => ({
setupCommand: vi.fn(),
}));
vi.mock('../../src/cli/publish.js', () => ({
publishCommand: vi.fn(),
}));
describe('CLI commands', () => {
describe('version', () => {
@ -84,4 +87,11 @@ describe('CLI commands', () => {
expect(typeof setupCommand).toBe('function');
});
});
describe('publishCommand', () => {
it('is a function', async () => {
const { publishCommand } = await import('../../src/cli/publish.js');
expect(typeof publishCommand).toBe('function');
});
});
});

View file

@ -63,4 +63,17 @@ describe('CLI help surface', () => {
expect(result.stdout).toContain('--model <model>');
expect(result.stdout).toContain('--gist');
});
it('publish help names the registry, the token env var, and the opt-out behaviour', () => {
const result = runHelp('publish');
expect(result.status).toBe(0);
expect(result.stdout).toContain('--id <owner/repo>');
expect(result.stdout).toContain('--skip-git');
// Discoverability contract: a contributor scanning `--help` must see
// (a) which registry this dispatches to, and (b) the env var that
// gates the opt-in. Both are part of the no-token contract.
expect(result.stdout).toContain('understand-quickly');
expect(result.stdout).toContain('UNDERSTAND_QUICKLY_TOKEN');
});
});

View file

@ -26,7 +26,7 @@ describe('understand-quickly helpers (gitnexus-shared)', () => {
// LOW 8 additions:
['some_org/repo', false], // underscore in owner — invalid
['-org/repo', false], // leading hyphen — invalid
['org-/repo', true], // trailing hyphen — GitHub actually allows this
['org-/repo', false], // trailing hyphen — GitHub rejects at account creation; we mirror that here
['org/repo_with_underscore', true],
['org/.dotfile', true], // repos may start with dot
])('returns %s for %j', (id, expected) => {
@ -280,9 +280,13 @@ describe('publishCommand response branches (MEDIUM 5)', () => {
expect(process.exitCode).toBe(1);
});
it('AbortError (HIGH 4 — fetch timeout) → exit 1 with timed-out message', async () => {
const abort = new Error('aborted');
abort.name = 'AbortError';
it('TimeoutError (HIGH 4 — fetch timeout) → exit 1 with timed-out message', async () => {
// `AbortSignal.timeout()` throws a real `DOMException` with
// `name === 'TimeoutError'`. Faking it as `Error{name:'AbortError'}`
// (the previous shape of this test) hid a mismatch in publish.ts —
// the catch branch only matched 'AbortError' and the user-facing
// "timed out" message never fired in production.
const abort = new DOMException('The operation was aborted due to timeout', 'TimeoutError');
fetchSpy.mockRejectedValueOnce(abort);
const errSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true);
const { publishCommand } = await import('../../src/cli/publish.js');