fix: replace empty catch blocks with meaningful error handling by qorexdev · Pull Request #90 · linearis-oss/linearis

qorexdev · 2026-04-07T23:48:02Z

The main fix is in token-storage.ts — a corrupted token file used to silently return null, which made users think no token existed. Now it warns to stderr so they know the file is there but broken.

Also:

file-service uploadFile now distinguishes "file not found" from "permission denied"
added explanatory comments to all other intentional try-parse catch blocks across auth, documents, embed-parser, and identifier

All 215 tests passing.

Closes #60

- token-storage: warn to stderr when stored token is corrupted instead of silently returning null - file-service: distinguish ENOENT from permission errors in upload - auth, documents, embed-parser, identifier: add explanatory comments to intentional try-parse catch blocks Closes linearis-oss#60

Copilot

Pull request overview

This PR removes silent error swallowing by adding explanatory comments to intentional parse-fallback catch {} blocks and introducing user-visible error handling where silently returning null previously obscured real problems (notably corrupted stored tokens).

Changes:

getStoredToken() now emits stderr warnings when stored token decryption fails instead of silently returning null.
FileService.uploadFile() now differentiates missing-file vs other access() failures and returns more specific error messages.
Added inline comments explaining intentionally swallowed exceptions in auth/documents/embed parsing/identifier helpers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/unit/services/file-service.test.ts	Updates the missing-file test to simulate a real `ENOENT`-shaped error (with `code`).
src/services/file-service.ts	Improves handling of `access()` failures during upload to distinguish not-found from other access errors.
src/common/token-storage.ts	Adds stderr warnings when stored token files exist but cannot be decrypted.
src/common/identifier.ts	Documents the intentional try-parse swallow behavior.
src/common/embed-parser.ts	Documents URL parse failure fallbacks.
src/commands/documents.ts	Documents malformed document URL fallback behavior.
src/commands/auth.ts	Documents expected exceptions in token resolution/validation flows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-07T23:50:37Z

src/common/token-storage.ts

  try {
    const encrypted = fs.readFileSync(tokenPath, "utf8").trim();
    return decryptToken(encrypted);
-  } catch {
+  } catch (err) {
+    const detail = err instanceof Error ? err.message : String(err);
+    console.error(
+      `Warning: stored token at ${tokenPath} is corrupted: ${detail}`,
+    );
    return null;


getStoredToken() now logs "... is corrupted" for any error thrown by readFileSync() or decryptToken(). That can produce misleading warnings for non-corruption cases (e.g., EACCES/EPERM permission issues, or ENOENT if the file is deleted between existsSync() and readFileSync()). Consider splitting the read/decrypt steps (or checking NodeJS.ErrnoException.code) so that ENOENT still returns null silently, permission errors report as access problems, and only decryption/parse failures are labeled as corruption.

Copilot · 2026-04-07T23:50:37Z

src/services/file-service.ts

+    } catch (err) {
+      // access() fails with ENOENT for missing files, EACCES for permission issues
+      const detail =
+        err instanceof Error && "code" in err
+          ? (err as NodeJS.ErrnoException).code
+          : undefined;
      return {
        success: false,
-        error: `File not found: ${filePath}`,
+        error:
+          detail === "ENOENT"
+            ? `File not found: ${filePath}`
+            : `Cannot access file: ${filePath} (${detail || (err instanceof Error ? err.message : String(err))})`,
      };


uploadFile() intends to surface the underlying access error, but for errors that do have a .code the message currently only includes the code (e.g., "(EACCES)") and drops the actual error message/path details. Consider including both code and the original err.message when available so the returned error is consistently actionable (especially for permission issues).

Copilot · 2026-04-07T23:50:38Z

src/services/file-service.ts

+    } catch (err) {
+      // access() fails with ENOENT for missing files, EACCES for permission issues
+      const detail =
+        err instanceof Error && "code" in err
+          ? (err as NodeJS.ErrnoException).code
+          : undefined;
      return {
        success: false,
-        error: `File not found: ${filePath}`,
+        error:
+          detail === "ENOENT"
+            ? `File not found: ${filePath}`
+            : `Cannot access file: ${filePath} (${detail || (err instanceof Error ? err.message : String(err))})`,
      };


New behavior distinguishes ENOENT vs other access() failures, but tests only cover ENOENT. Add a unit test for a non-ENOENT failure (e.g., EACCES) to assert the new "Cannot access file" branch and message formatting.

Copilot · 2026-04-07T23:50:38Z

src/common/token-storage.ts

        try {
          const encrypted = fs.readFileSync(legacy, "utf8").trim();
          return decryptToken(encrypted);
-        } catch {
+        } catch (err) {
+          // file exists but can't be decrypted — warn instead of silently returning null
+          const detail = err instanceof Error ? err.message : String(err);
+          console.error(
+            `Warning: stored token at ${legacy} is corrupted: ${detail}`,
+          );
          return null;
        }


Legacy token fallback catch block also logs "... is corrupted" for any readFileSync()/decryptToken() error. If the file exists but is unreadable (EACCES/EPERM) this message is misleading; consider checking errno codes and using a different warning for access errors, reserving the corruption warning for decryption failures.

iamfj

Clean, well-scoped fix. The corrupted token file silently returning null was a real user pain point — good catch surfacing the warning to stderr while keeping the return behavior unchanged.

Comment quality across the board is solid — they explain why the catch is empty, not what the code does, which is exactly the right approach. The file-service ENOENT/EACCES distinction is a nice improvement too.

One suggestion: since uploadFile now branches on code === "ENOENT" vs other errors, it'd be worth adding a test case for the non-ENOENT path (e.g., EACCES) to cover both sides of that branch. Not a blocker — the primary error case is covered — but good to have since that's new behavior.

LGTM 👍

iamfj · 2026-04-08T08:28:43Z

tests/unit/services/file-service.test.ts

+      code: "ENOENT",
+    });
+    vi.mocked(access).mockRejectedValue(err);



Nice — the mock now properly simulates a real Node.js filesystem error with code. Since the implementation branches on code === "ENOENT", consider adding a parallel test for the other branch:

it("returns descriptive error on permission denied", async () => { const err = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES", }); vi.mocked(access).mockRejectedValue(err); const service = new FileService(TEST_TOKEN); const result = await service.uploadFile("/path/to/restricted.png"); expect(result.success).toBe(false); expect(result.error).toContain("Cannot access file"); });

Copilot AI review requested due to automatic review settings April 7, 2026 23:48

qorexdev requested a review from iamfj as a code owner April 7, 2026 23:48

Copilot started reviewing on behalf of qorexdev April 7, 2026 23:48 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

iamfj approved these changes Apr 8, 2026

View reviewed changes

This was referenced Apr 8, 2026

fix(#60): fix: replace empty catch blocks with meaningful error handling #88

Closed

fix: replace empty catch blocks with handling #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace empty catch blocks with meaningful error handling#90

fix: replace empty catch blocks with meaningful error handling#90
qorexdev wants to merge 1 commit intolinearis-oss:mainfrom
qorexdev:fix/empty-catch-blocks

qorexdev commented Apr 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

iamfj left a comment

Uh oh!

iamfj Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

qorexdev commented Apr 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

iamfj left a comment

Choose a reason for hiding this comment

Uh oh!

iamfj Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants