fix: address HIGH and MEDIUM code quality audit findings by RobertLD · Pull Request #280 · RobertLD/libscope

RobertLD · 2026-03-03T13:35:46Z

Summary

Systematic fixes for 28 HIGH and MEDIUM code quality issues identified in comprehensive audit.

Error Handling (5 files, 14 raw `Error()` replaced)

rag.ts: 6 throw new Error() → ConfigError / FetchError
workspace.ts: 5 throw new Error() → ValidationError
ollama.ts: 2 throw new Error() → EmbeddingError
openai.ts: 1 throw new Error() → EmbeddingError
packs.ts: Network errors now throw FetchError instead of ValidationError

API Layer (2 files)

middleware.ts: Added PATCH to CORS allowed methods
routes.ts:
- Bounds validation on limit (1–1000), offset (≥0), maxChunksPerDocument (1–100)
- Default limit of 100 for GET /documents
- All 4 DELETE endpoints return 204 No Content (REST convention)
- Explicit 400 for invalid bulk operation names
- Removed method+pathname from 404 errors (info leak)

Bug Fixes (3 files)

http-utils.ts: Off-by-one in retry loop (<=maxRetries → <maxRetries)
url-fetcher.ts: Off-by-one in redirect loop (<=maxRedirects → <maxRedirects)
search.ts: Aligned keyword fallback word filter to match FTS5 (length > 2 → length > 0)

Reliability (5 files)

packs.ts: Added 30s fetch timeouts to registry calls
scheduler.ts: Added 30s shutdown timeout to stop()
notion.ts: Added MAX_PAGES = 10,000 pagination cap
confluence.ts: Added MAX_PAGES = 10,000 pagination cap
cli/index.ts: Refactored signal handlers to named function

Observability (1 file)

dashboard.ts: Added console.error logging to 4 empty catch {} blocks

Semantic Fix (1 file)

webhooks.ts: getWebhook/deleteWebhook throw ValidationError instead of DocumentNotFoundError

Tests Updated (6 files)

api.test.ts: DELETE assertions expect 204, CORS expects PATCH
http-utils.test.ts: Retry count matches off-by-one fix
url-fetcher.test.ts: Redirect count matches off-by-one fix
search.test.ts: Updated for new word filter behavior
webhooks.test.ts: Updated error type assertions
repl.test.ts: Added spy cleanup, proper MockInstance types
update-document.test.ts: Hardened timestamp assertion

Verification

✅ All 926 tests pass
✅ Zero lint errors
✅ Prettier clean

Systematic fixes for 28 code quality issues identified in audit: Error handling: - Replace 14 raw Error() throws with typed errors (ConfigError, FetchError, ValidationError, EmbeddingError) across rag.ts, workspace.ts, ollama.ts, openai.ts, packs.ts - Fix webhook error types (DocumentNotFoundError → ValidationError) API layer: - Add PATCH to CORS allowed methods - Add bounds validation on limit (1-1000), offset (≥0), maxChunksPerDocument (1-100) - Add default limit of 100 for document listing - Change all 4 DELETE endpoints to return 204 No Content - Return 400 for invalid bulk operation names - Remove method+pathname from 404 error messages (info leak) Bug fixes: - Fix off-by-one in retry loop (http-utils.ts: attempt<=maxRetries → <) - Fix off-by-one in redirect loop (url-fetcher.ts: i<=maxRedirects → <) - Align keyword fallback word filter to match FTS5 behavior (length>2 → >0) Reliability: - Add 30-second fetch timeouts to pack registry calls - Add 30-second shutdown timeout to scheduler stop() - Add MAX_PAGES=10,000 pagination caps to Notion and Confluence connectors - Refactor CLI signal handlers to named function Observability: - Add console.error logging to 4 empty catch blocks in dashboard Tests: - Update DELETE test assertions to expect 204 - Update retry/redirect count assertions for off-by-one fixes - Update search test for new word filter behavior - Update CORS test for PATCH method - Add spy cleanup in repl tests - Harden timestamp assertion in update-document test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

vercel · 2026-03-03T13:35:51Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
libscope	Ready	Preview, Comment	Mar 3, 2026 1:35pm

Copilot

Pull request overview

This PR applies broad code-quality audit remediations across LibScope, focusing on typed error handling, tighter API behavior, and reliability/observability improvements, plus accompanying test and documentation updates.

Changes:

Replace many raw Error throws with typed LibScopeError variants (e.g., ConfigError, FetchError, EmbeddingError, ValidationError).
Adjust REST API behavior (CORS PATCH, parameter bounds/defaults, 204 responses for DELETEs, safer 404 messaging).
Add reliability safeguards (timeouts, pagination caps, scheduler shutdown timeout) and update tests/docs accordingly.

Reviewed changes

Copilot reviewed 40 out of 41 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
tests/unit/webhooks.test.ts	Updates expected error types for webhook not-found cases
tests/unit/url-fetcher.test.ts	Updates redirect-limit assertions
tests/unit/update-document.test.ts	Hardens timestamp assertions for SQLite second-resolution
tests/unit/search.test.ts	Updates expectations for LIKE fallback behavior with short words
tests/unit/repl.test.ts	Adds spy cleanup and improves mock typing
tests/unit/http-utils.test.ts	Updates retry-count assertions
tests/unit/api.test.ts	Updates CORS method expectations and DELETE status expectations (204)
src/web/dashboard.ts	Adds logging in previously silent catch blocks
src/providers/openai.ts	Uses `EmbeddingError` for empty embedding response
src/providers/ollama.ts	Uses `EmbeddingError` for non-OK/empty embedding responses
src/core/workspace.ts	Uses `ValidationError` for workspace input/state errors
src/core/webhooks.ts	Changes not-found behavior to throw `ValidationError`
src/core/url-fetcher.ts	Changes redirect loop bounds for max-redirect enforcement
src/core/search.ts	Adjusts keyword fallback word filtering to include short words
src/core/scheduler.ts	Adds shutdown timeout when waiting for in-flight connector syncs
src/core/rag.ts	Uses typed errors for missing config and LLM fetch error cases
src/core/packs.ts	Adds registry fetch timeouts and uses `FetchError` for network failures
src/connectors/notion.ts	Adds pagination cap to avoid unbounded looping
src/connectors/http-utils.ts	Changes retry loop bounds/error message formatting
src/connectors/confluence.ts	Adds pagination cap to avoid unbounded looping
src/cli/index.ts	Refactors SIGINT/SIGTERM shutdown handlers to a named function
src/api/routes.ts	Adds bounds/defaults for query params, returns 204 for DELETEs, safer 404, invalid bulk op 400
src/api/middleware.ts	Adds `PATCH` to CORS allowed methods
sdk/python/README.md	Table formatting updates
sdk/go/README.md	Table formatting updates
eslint.config.js	Minor formatting normalization for eslint rule config
docs/reference/rest-api.md	Table formatting updates
docs/reference/mcp-tools.md	Table formatting updates + minor phrasing tweak
docs/reference/configuration.md	Table formatting updates
docs/reference/cli.md	Table formatting updates
docs/guide/configuration.md	Table formatting updates
docs/contributing.md	Table formatting updates + spacing tweak
docs/changelog.md	List formatting updates
docker-compose.yml	Formatting cleanup
agents.md	Table formatting updates + spacing tweaks
README.md	Table formatting updates
CONTRIBUTING.md	Table formatting updates + spacing tweak
CHANGELOG.md	List formatting updates
.github/workflows/sdk-python.yml	Quote-style change in `paths`/matrix config
.github/workflows/sdk-go.yml	Quote-style change in `paths`/matrix config
.github/workflows/docker.yml	Quote-style change in tag/path globs

Copilot · 2026-03-03T13:42:45Z

tests/unit/url-fetcher.test.ts

    // 1 original + 2 redirects = 3 calls
-    expect(mockFetch).toHaveBeenCalledTimes(3);
+    expect(mockFetch).toHaveBeenCalledTimes(2);
  });


This test’s comment says “1 original + 2 redirects = 3 calls”, but the assertion expects 2 calls. If maxRedirects is the number of redirects (per FetchOptions docs), the fetcher should make 3 requests here (initial + 2 redirects) before throwing.

Copilot · 2026-03-03T13:42:45Z

src/core/scheduler.ts

+      const timeout = new Promise<void>((resolve) => {
+        const timer = setTimeout(() => {
+          log.warn("Shutdown timeout reached, proceeding without waiting for in-flight syncs");
+          resolve();
+        }, SHUTDOWN_TIMEOUT_MS);
+        timer.unref();
+      });
+      await Promise.race([Promise.allSettled(inFlight), timeout]);


The shutdown timeout timer isn’t cleared if Promise.allSettled(inFlight) wins the race, so 30s later it can still log "Shutdown timeout reached" even though shutdown already completed. Consider storing the timer handle and clearTimeout() it in a finally once the race resolves.

Suggested change

const timeout = new Promise<void>((resolve) => {

const timer = setTimeout(() => {

log.warn("Shutdown timeout reached, proceeding without waiting for in-flight syncs");

resolve();

}, SHUTDOWN_TIMEOUT_MS);

timer.unref();

});

await Promise.race([Promise.allSettled(inFlight), timeout]);

let shutdownTimer: NodeJS.Timeout | undefined;

const timeout = new Promise<void>((resolve) => {

shutdownTimer = setTimeout(() => {

log.warn("Shutdown timeout reached, proceeding without waiting for in-flight syncs");

resolve();

}, SHUTDOWN_TIMEOUT_MS);

shutdownTimer.unref();

});

try {

await Promise.race([Promise.allSettled(inFlight), timeout]);

} finally {

if (shutdownTimer !== undefined) {

clearTimeout(shutdownTimer);

}

}

Copilot · 2026-03-03T13:42:45Z

src/core/webhooks.ts

 export function getWebhook(db: Database.Database, id: string): Webhook {
  const row = db.prepare("SELECT * FROM webhooks WHERE id = ?").get(id) as WebhookRow | undefined;
  if (!row) {
-    throw new DocumentNotFoundError(id);
+    throw new ValidationError(`Webhook not found: ${id}`);
  }
  return rowToWebhook(row);


getWebhook now throws ValidationError for a missing row. In the REST API handler, ValidationError is translated to HTTP 400, so a non-existent webhook id will become a 400 instead of a 404. Consider introducing a dedicated not-found error for webhooks (or mapping this case to 404) to keep resource-not-found behavior consistent.

Copilot · 2026-03-03T13:42:46Z

src/core/webhooks.ts

 export function deleteWebhook(db: Database.Database, id: string): void {
  const result = db.prepare("DELETE FROM webhooks WHERE id = ?").run(id);
  if (result.changes === 0) {
-    throw new DocumentNotFoundError(id);
+    throw new ValidationError(`Webhook not found: ${id}`);
  }


deleteWebhook uses ValidationError when no rows were deleted. This is effectively a not-found condition, and will surface as HTTP 400 in the API layer. Consider using a not-found error type (or otherwise translating to 404) for missing webhook IDs.

Copilot · 2026-03-03T13:42:46Z

.github/workflows/sdk-python.yml

  push:
-    paths: ['sdk-python/**']
+    paths: ["sdk-python/**"]
  pull_request:
-    paths: ['sdk-python/**']
+    paths: ["sdk-python/**"]


The workflow trigger paths reference sdk-python/**, but the repository directory is sdk/python/ (and similarly Go is sdk/go/). As written, this workflow won’t run when the SDK code changes. Update the paths globs to match the actual directories.

Copilot · 2026-03-03T13:42:46Z

src/connectors/http-utils.ts

+    for (let attempt = 0; attempt < maxRetries; attempt++) {
      const response = await fetch(url, options);

      if (response.status === 429 || (response.status >= 500 && response.status < 600)) {
-        if (attempt >= maxRetries) {
+        if (attempt >= maxRetries - 1) {
          const body = await response.text().catch(() => "");
-          throw new FetchError(`HTTP ${response.status} after ${maxRetries + 1} attempts: ${body}`);
+          throw new FetchError(`HTTP ${response.status} after ${maxRetries} attempts: ${body}`);
        }


maxRetries is named/used as a retry count (default 3) in the public RetryConfig, but the new loop attempt < maxRetries turns it into a max-attempts value and also breaks maxRetries=0 (it falls through and throws fetchWithRetry: unexpected code path). If the intent is “retry N times after the first attempt”, the loop should allow maxRetries + 1 total attempts and the terminal condition/error message should match that.

Copilot · 2026-03-03T13:42:47Z

tests/unit/http-utils.test.ts

    await expect(
      fetchWithRetry("https://example.com/api", undefined, {
        maxRetries: 2,
        baseDelay: 10,
      }),
    ).rejects.toThrow(FetchError);
-    expect(mockFetch).toHaveBeenCalledTimes(3);
+    expect(mockFetch).toHaveBeenCalledTimes(2);
    vi.useFakeTimers();


The updated expectation (toHaveBeenCalledTimes(2)) matches the new implementation, but it no longer aligns with the parameter name maxRetries. With maxRetries: 2, typical semantics are 1 initial attempt + 2 retries = 3 total fetch calls. If you keep maxRetries as “retries”, this assertion should be 3.

Copilot · 2026-03-03T13:42:47Z

src/core/rag.ts

      } catch (err) {
        if (err instanceof Error && err.name === "AbortError") {
-          throw new Error(`OpenAI LLM request timed out after ${timeoutMs}ms`);
+          throw new FetchError(`OpenAI LLM request timed out after ${timeoutMs}ms`);
        }
        throw err;
      } finally {


In the non-timeout error path, this rethrows err directly. That can leak raw TypeError/DOMException from fetch() and violates the project convention of throwing typed errors from src/errors.ts. Consider wrapping unknown fetch failures in FetchError (while still passing through existing LibScopeErrors) so callers get consistent error types.

Copilot · 2026-03-03T13:42:47Z

src/core/rag.ts

      } catch (err) {
        if (err instanceof Error && err.name === "AbortError") {
-          throw new Error(`Ollama LLM request timed out after ${timeoutMs}ms`);
+          throw new FetchError(`Ollama LLM request timed out after ${timeoutMs}ms`);
        }
        throw err;
      } finally {


Same issue here: non-timeout errors are rethrown as-is, which can propagate non-LibScope error types from fetch(). Consider wrapping unexpected errors in FetchError (or at least normalizing to LibScopeError) for consistent error handling upstream.

Copilot · 2026-03-03T13:42:48Z

.github/workflows/sdk-go.yml

  push:
-    paths: ['sdk-go/**']
+    paths: ["sdk-go/**"]
  pull_request:
-    paths: ['sdk-go/**']
+    paths: ["sdk-go/**"]


The workflow trigger paths reference sdk-go/**, but the SDK lives under sdk/go/ in this repo. This means the workflow likely won’t run on Go SDK changes unless the paths globs are corrected.

Copilot AI review requested due to automatic review settings March 3, 2026 13:35

Copilot started reviewing on behalf of RobertLD March 3, 2026 13:36 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

RobertLD merged commit 1e93987 into main Mar 3, 2026
14 checks passed

github-actions bot mentioned this pull request Mar 3, 2026

chore(main): release 1.3.0 #238

Closed

RobertLD deleted the fix/audit-high-medium branch March 3, 2026 13:44

This was referenced Mar 5, 2026

chore(development): release 1.3.0 #346

Closed

chore(development): release 1.3.0 #355

Closed

chore(main): release 1.3.0 #359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address HIGH and MEDIUM code quality audit findings#280

fix: address HIGH and MEDIUM code quality audit findings#280
RobertLD merged 1 commit intomainfrom
fix/audit-high-medium

RobertLD commented Mar 3, 2026

Uh oh!

vercel bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

RobertLD commented Mar 3, 2026

Summary

Error Handling (5 files, 14 raw Error() replaced)

API Layer (2 files)

Bug Fixes (3 files)

Reliability (5 files)

Observability (1 file)

Semantic Fix (1 file)

Tests Updated (6 files)

Verification

Uh oh!

vercel bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Error Handling (5 files, 14 raw `Error()` replaced)