mirror of
https://github.com/firstsun-dev/git-files-sync.git
synced 2026-07-22 06:54:27 +00:00
fix(services): stop logging expected 404s as errors during refresh
Loading .gitignore files probes paths that often don't exist remotely. getFile already treats 404 as "empty file" (handleFileNotFound) and the gitignore lookup ignores failures, but safeRequest logged every 404 as an error — twice, because its own catch re-caught the error it had just thrown. This produced scary "Git Service Request Failed (404): Not Found" console output during a normal pull/refresh. Restructure safeRequest so the catch only wraps the network call (no double-logging of HTTP-status errors), and log an expected 404 at debug level instead of error. Non-404 statuses are still logged as errors and all statuses still throw so callers can handle them. Add a debug level to the logger and regression tests for 404 vs 500. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DwioG4CNKUBuKiZdowLFWe
This commit is contained in:
parent
9bc8ab7d08
commit
c474a7e6c4
3 changed files with 55 additions and 9 deletions
|
|
@ -42,6 +42,7 @@ export abstract class BaseGitService {
|
|||
* Safely wraps requestUrl to handle potential throws from Obsidian and provide better error messages.
|
||||
*/
|
||||
protected async safeRequest(url: string, method: string, body?: unknown, extraHeaders?: Record<string, string>, silent = false): Promise<RequestUrlResponse> {
|
||||
let response: RequestUrlResponse;
|
||||
try {
|
||||
const headers: Record<string, string> = {
|
||||
...extraHeaders,
|
||||
|
|
@ -57,20 +58,28 @@ export abstract class BaseGitService {
|
|||
throw: false
|
||||
};
|
||||
|
||||
const response = await requestUrl(options);
|
||||
|
||||
if (response.status >= 400) {
|
||||
const errorMsg = this.parseErrorResponse(response);
|
||||
if (!silent) logger.error(`Git Service Request Failed (${response.status}): ${url}`, errorMsg);
|
||||
throw new Error(`Git Service Error (${response.status}): ${errorMsg}`);
|
||||
}
|
||||
|
||||
return response;
|
||||
response = await requestUrl(options);
|
||||
} catch (error) {
|
||||
// Network-level failure (DNS, offline, TLS, etc.)
|
||||
if (!silent) logger.error('Git Service Request Failed:', error);
|
||||
if (error instanceof Error) throw error;
|
||||
throw new Error(`Network error or unexpected failure: ${String(error)}`);
|
||||
}
|
||||
|
||||
if (response.status >= 400) {
|
||||
const errorMsg = this.parseErrorResponse(response);
|
||||
// 404 is routinely an expected "does not exist" probe (getFile treats
|
||||
// it as an empty file, gitignore lookups ignore it). Log it at debug
|
||||
// level so it doesn't surface as a failure, but still throw so callers
|
||||
// can handle it. Other statuses are genuine errors.
|
||||
if (!silent) {
|
||||
if (response.status === 404) logger.debug(`Git Service 404 (not found): ${url}`);
|
||||
else logger.error(`Git Service Request Failed (${response.status}): ${url}`, errorMsg);
|
||||
}
|
||||
throw new Error(`Git Service Error (${response.status}): ${errorMsg}`);
|
||||
}
|
||||
|
||||
return response;
|
||||
}
|
||||
|
||||
protected abstract addAuthHeader(headers: Record<string, string>): void;
|
||||
|
|
|
|||
|
|
@ -3,4 +3,5 @@ const PREFIX = '[git-file-sync]';
|
|||
export const logger = {
|
||||
error: (message: string, ...args: unknown[]) => console.error(`${PREFIX} ${message}`, ...args),
|
||||
warn: (message: string, ...args: unknown[]) => console.warn(`${PREFIX} ${message}`, ...args),
|
||||
debug: (message: string, ...args: unknown[]) => console.debug(`${PREFIX} ${message}`, ...args),
|
||||
};
|
||||
|
|
|
|||
|
|
@ -30,6 +30,42 @@ describe('BaseGitService', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('safeRequest 404 handling', () => {
|
||||
it('getFile returns empty and does not log an error on 404 (e.g. missing .gitignore)', async () => {
|
||||
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {});
|
||||
vi.mocked(requestUrl).mockResolvedValue({
|
||||
status: 404,
|
||||
text: 'Not Found',
|
||||
json: { message: 'Not Found' },
|
||||
} as unknown as RequestUrlResponse);
|
||||
|
||||
const result = await service.getFile('missing/.gitignore', 'main');
|
||||
|
||||
expect(result).toEqual({ content: '', sha: '' });
|
||||
// A 404 is an expected "does not exist" probe: never logged as an error…
|
||||
expect(errorSpy).not.toHaveBeenCalled();
|
||||
// …and logged at most once at debug level (no double-logging).
|
||||
expect(debugSpy).toHaveBeenCalledTimes(1);
|
||||
|
||||
errorSpy.mockRestore();
|
||||
debugSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('still logs non-404 failures as errors', async () => {
|
||||
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
vi.mocked(requestUrl).mockResolvedValue({
|
||||
status: 500,
|
||||
text: 'Internal Server Error',
|
||||
json: { message: 'Internal Server Error' },
|
||||
} as unknown as RequestUrlResponse);
|
||||
|
||||
await expect(service.listFiles('main')).rejects.toThrow('500');
|
||||
expect(errorSpy).toHaveBeenCalledTimes(1);
|
||||
errorSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe('safeRequest with non-Error exception', () => {
|
||||
it('should wrap non-Error throws in a new Error', async () => {
|
||||
// Throw a plain string (not an Error instance) from requestUrl
|
||||
|
|
|
|||
Loading…
Reference in a new issue