Skip to content

Add /v1/* API route aliases while preserving legacy endpoints#63

Open
Copilot wants to merge 3 commits into
masterfrom
copilot/change-api-endpoints-to-v1
Open

Add /v1/* API route aliases while preserving legacy endpoints#63
Copilot wants to merge 3 commits into
masterfrom
copilot/change-api-endpoints-to-v1

Conversation

Copilot AI commented Apr 11, 2026

Copy link
Copy Markdown
Contributor
  • Surveyed all paths in index.ts, NotificationManager.ts, Process.ts
  • Added tests for all uncovered paths: /, /v1/, /GetAddOnScript, /v1/GetAddOnScript, /GetImage, /v1/GetImage, legacy /GetStd C++ processing
  • Validated with Code Review and CodeQL Security Scan — no issues found

Summary by Sourcery

Add support for versioned /v1/* API routes while preserving behavior of existing legacy endpoints.

New Features:

  • Introduce a path normalization layer that maps /v1 and /v1/* routes to their corresponding legacy handlers, with / and /v1[/] mapped to GetNotice.
  • Expose versioned WebSocket and notification endpoints under /v1/ws/notifications and /v1/notify that behave identically to the legacy paths.

Tests:

  • Add comprehensive tests covering legacy and /v1 aliases for notice, addon script, image, and GetStd processing endpoints, as well as the versioned notification endpoint.

Agent-Logs-Url: https://github.com/XMOJ-Script-dev/XMOJ-bbs/sessions/3f0e67ba-63e5-4ab3-99b6-85ed8c48cfd7

Co-authored-by: PythonSmall-Q <106425289+PythonSmall-Q@users.noreply.github.com>
@sourcery-ai

sourcery-ai Bot commented Apr 11, 2026

Copy link
Copy Markdown

Reviewer's Guide

Adds versioned /v1/* aliases for existing HTTP endpoints while keeping legacy routes intact, centralizing path normalization in Process and extending tests to cover both legacy and versioned behaviors.

Sequence diagram for normalized v1 API request handling

sequenceDiagram
  actor Client
  participant IndexHandler
  participant Process
  participant URLParser as URL
  participant KV as KVNamespace
  participant DB as DatabaseSession
  participant HttpResponse as Response

  Client->>IndexHandler: HTTP GET /v1/GetImage
  IndexHandler->>Process: new Process(RequestData)
  IndexHandler->>Process: Process()
  Process->>URLParser: new URL(RequestData.url)
  URLParser-->>Process: pathname /v1/GetImage
  Process->>Process: NormalizePathName("/v1/GetImage")
  Process-->>Process: "/GetImage"
  Process->>Process: strip leading slash ("GetImage")
  Process->>KV: get("noticeboard") when needed
  KV-->>Process: notice data
  Process->>DB: query(...) when needed
  DB-->>Process: result rows
  Process->>HttpResponse: new Response(JSON.stringify(ResponseData))
  HttpResponse-->>IndexHandler: Response
  IndexHandler-->>Client: HTTP 200 with JSON body
Loading

Updated class diagram for Process and NotificationManager routing

classDiagram
  class Process {
    +RequestData: Request
    -RawDatabase
    -kv
    +constructor(RequestData)
    -NormalizePathName(pathname: string) string
    +Process() Promise~Response~
    -processCppString(input: string) string
  }

  class NotificationManager {
    -pushToken: string
    +constructor(pushToken: string)
    +handleRequest(request: Request) Promise~Response~
  }

  class IndexHandler {
    +handleRequest(RequestData: Request) Promise~Response~
  }

  IndexHandler --> Process : creates
  IndexHandler --> NotificationManager : uses

  class URL {
    +pathname: string
  }

  Process --> URL : parses
  NotificationManager --> URL : parses

  class KVNamespace {
    +get(key: string) Promise~string~
  }

  Process --> KVNamespace : uses kv

  class DatabaseSession {
    +query(sql: string) Promise~any~
  }

  Process --> DatabaseSession : uses RawDatabase

  class Response {
    +headers
    +status: number
  }

  Process --> Response
  NotificationManager --> Response
  IndexHandler --> Response
Loading

File-Level Changes

Change Details Files
Centralized request path normalization to support /v1 aliases and default routing to GetNotice.
  • Introduced a NormalizePathName helper that maps /, /v1, and /v1/ to /GetNotice and strips the /v1 prefix from other versioned paths.
  • Updated the main request handler to use normalized path names for routing logic, including the GetNotice special case.
  • Ensured response construction for JSON and C++-processed GetStd output uses the normalized path name to preserve existing behavior while supporting /v1/GetStd.
Source/Process.ts
Exposed /v1/notify as an alias for the internal /notify push endpoint.
  • Extended notification routing condition to treat /v1/notify identically to /notify.
  • Reused existing authentication with X-Notification-Token and WebSocket push logic for the versioned path.
Source/NotificationManager.ts
Exposed /v1/ws/notifications as an alias for the WebSocket notifications endpoint.
  • Updated the WebSocket routing check to accept both /ws/notifications and /v1/ws/notifications.
  • Left existing SessionID validation and 401 responses unchanged for the versioned route.
Source/index.ts
Expanded test coverage to validate behavior of both legacy and new /v1-prefixed endpoints, including default routing and binary/image responses.
  • Added tests for /GetNotice, /v1/GetNotice, /, /v1, and /v1/ to confirm they all return notices from KV storage.
  • Added tests for legacy and versioned GetAddOnScript and GetImage endpoints, including verifying content-type and parameters passed to GetImage.
  • Added tests to ensure /v1/GetStd and legacy /GetStd both use C++ string processing via processCppString.
  • Added a generic routing test confirming that an arbitrary /v1/<Name> endpoint (e.g., /v1/GetUserSettings) is dispatched to the corresponding legacy handler.
  • Added a notification manager test to assert /v1/notify behaves identically to /notify, sending payloads over the WebSocket session.
test/process.test.js
test/notification_manager.test.js

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@PythonSmall-Q

Copy link
Copy Markdown
Member

@copilot add /v1/* for all endpoints, not just the two you created

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Apr 11, 2026

Copilot AI commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add /v1/* for all endpoints, not just the two you created

Implemented in commit f5a3c68: /v1 alias coverage now extends beyond the two earlier paths by adding /v1/notify support and tests confirming generic /v1/<endpoint> routing (e.g. /v1/GetUserSettings) maps to existing handlers while legacy endpoints remain unchanged.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • registry.npmmirror.com
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/_temp/ghcca-node/node/bin/npm install (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@PythonSmall-Q PythonSmall-Q marked this pull request as ready for review April 11, 2026 02:32
Copilot AI review requested due to automatic review settings April 11, 2026 02:32

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 4 issues, and left some high level feedback:

  • Consider reusing the NormalizePathName logic (or extracting it to a shared utility) for the /notify and /ws/notifications handlers so that versioned and unversioned routing behavior stays consistent across the codebase.
  • In NormalizePathName, you special-case /, /v1, and /v1/ but not other trailing-slash or double-slash variants (e.g. /v1//GetImage); if those are possible in practice, it may be worth normalizing redundant slashes as well to avoid subtle routing differences.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider reusing the `NormalizePathName` logic (or extracting it to a shared utility) for the `/notify` and `/ws/notifications` handlers so that versioned and unversioned routing behavior stays consistent across the codebase.
- In `NormalizePathName`, you special-case `/`, `/v1`, and `/v1/` but not other trailing-slash or double-slash variants (e.g. `/v1//GetImage`); if those are possible in practice, it may be worth normalizing redundant slashes as well to avoid subtle routing differences.

## Individual Comments

### Comment 1
<location path="test/process.test.js" line_range="101-102" />
<code_context>
+        }
+    });
+
+    const response = await proc.Process();
+    const payload = await response.json();
+    assert.strictEqual(payload.Success, true);
+    assert.strictEqual(payload.Data.Notice, 'test notice');
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions on HTTP status (and possibly headers) to more fully validate the behavior of the new aliased endpoints.

For these new tests (e.g. `/GetNotice`, `/v1/GetNotice`, `/`, `/v1/`, `/GetAddOnScript`, `/v1/GetAddOnScript`), we only check the JSON body. Please also assert `response.status` (e.g. `assert.strictEqual(response.status, 200)`) and any key headers like `content-type` so we verify the aliased endpoints remain fully equivalent, not just in payload.

Suggested implementation:

```javascript
    const response = await proc.Process();
    assert.strictEqual(response.status, 200);
    assert.match(response.headers.get('content-type') ?? '', /^application\/json\b/i);

    const payload = await response.json();
    assert.strictEqual(payload.Success, true);
    assert.strictEqual(payload.Data.Notice, 'test notice');

```

1. For the other aliased endpoint tests mentioned (`/v1/GetNotice`, `/`, `/v1/`, `/GetAddOnScript`, `/v1/GetAddOnScript`), add the same pair of assertions immediately after `const response = await proc.Process();`:
   - `assert.strictEqual(response.status, 200);`
   - `assert.match(response.headers.get('content-type') ?? '', /^application\/json\b/i);` (or an equivalent check appropriate to the endpoint, e.g., for script/text responses you may want `text/javascript` or `text/html` instead of `application/json`).
2. Ensure that these header/status assertions are added before reading the body with `response.json()` or `response.text()` so failures are surfaced early and consistently across tests.
</issue_to_address>

### Comment 2
<location path="test/process.test.js" line_range="262-271" />
<code_context>
+test('Process serves image on legacy GET /GetImage endpoint', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the GetImage tests by verifying the response body, not just headers and arguments.

Right now the test only checks that `GetImage` is called with `{ ImageID }` and that `content-type` is `image/png`. To fully validate the behavior, also read the response body (e.g. via `await response.arrayBuffer()` or `await response.blob()`) and assert its length or bytes match the `imageData` you set up. This confirms the Blob from `ProcessFunctions.GetImage` is actually streamed to the client.

Suggested implementation:

```javascript
test('Process serves image on legacy GET /GetImage endpoint', async () => {
    const imageData = new Uint8Array([137, 80, 78, 71]).buffer;
    const proc = createProcess({
        req: new Request('https://example.com/GetImage?ImageID=test.png', {
            method: 'GET',
            headers: { 'CF-Connecting-IP': '127.0.0.1' }
        })
    });

    proc.ProcessFunctions.GetImage = test.mock.fn(
        async () => new Blob([imageData], { type: 'image/png' })
    );

    const response = await proc.Process();

    // Validate the handler was invoked with the expected arguments
    assert.strictEqual(proc.ProcessFunctions.GetImage.mock.calls.length, 1);
    assert.deepStrictEqual(proc.ProcessFunctions.GetImage.mock.calls[0][0], {
        ImageID: 'test.png'
    });

    // Validate response headers
    assert.strictEqual(response.headers.get('content-type'), 'image/png');

    // Validate that the response body matches the image data we set up
    const bodyBuffer = await response.arrayBuffer();
    assert.strictEqual(bodyBuffer.byteLength, imageData.byteLength);
    assert.deepStrictEqual(
        new Uint8Array(bodyBuffer),
        new Uint8Array(imageData)
    );
});

```

If your test framework exposes mock call metadata differently than `mock.calls`, adjust the assertions on `proc.ProcessFunctions.GetImage.mock.calls` accordingly (e.g., `mock.calls[0].arguments` or similar), but keep the body validation via `response.arrayBuffer()` and `Uint8Array` comparison as implemented.
</issue_to_address>

### Comment 3
<location path="test/process.test.js" line_range="141-150" />
<code_context>
+test('Process uses GetStd C++ string processing on /v1/GetStd', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative/edge-case test for versioned GetStd to ensure aliasing doesn’t bypass error handling.

You’re already verifying that `/GetStd` and `/v1/GetStd` share the same happy-path C++ processing. Please also add a failing case (e.g., `CheckToken` failure or `ProcessFunctions.GetStd` error) for `/v1/GetStd` to confirm the alias follows the same error path and still returns C++-processed errors rather than plain JSON.

Suggested implementation:

```javascript
    const payload = await response.json();
    assert.strictEqual(payload.Success, true);
    assert.strictEqual(payload.Data.Notice, 'test notice');
});

test('Process uses GetStd C++ error handling on /v1/GetStd for failing C++ processing', async () => {
    const req = new Request('https://example.com/v1/GetStd', {
        method: 'POST',
        headers: {
            "CF-Connecting-IP": "127.0.0.1",
            "content-type": "application/json"
        },
        body: JSON.stringify({
            Authentication: { SessionID: 'testsession', Username: 'testuser' },
            Data: {},
            Version: 'test',
        }),
    });

    const proc = new Process(req, {
        CheckToken: async () => ({
            Success: true,
            Data: { Username: 'testuser' },
        }),
        ProcessFunctions: {
            GetStd: {
                // Simulate a C++-side error path for GetStd
                post: async () => {
                    return {
                        ok: false,
                        status: 500,
                        json: async () => ({
                            Success: false,
                            Error: 'forced C++ GetStd failure',
                        }),
                    };
                },
            },
        },
    });

    const response = await proc.Process();
    const payload = await response.json();

    // Ensure the alias /v1/GetStd follows the same C++ error path as /GetStd
    assert.strictEqual(payload.Success, false);
    assert.strictEqual(payload.Error, 'forced C++ GetStd failure');
});

test('Process uses GetStd C++ string processing on /v1/GetStd', async () => {

```

You may need to adjust the new test to match your existing testing conventions and error shapes:

1. **Shape of `CheckToken` result**:  
   - If other tests return a differently shaped object (e.g. `{ Success: false, ErrorCode: '...' }`), align this mock with that structure.

2. **Shape of `ProcessFunctions.GetStd.post` result**:  
   - I assumed it returns a `Response`-like object with `ok`, `status`, and `json()`.  
   - If your existing GetStd tests use a different mock (e.g. directly returning `{ Success: false, ... }`), copy that pattern here and keep the failure semantics identical.

3. **Error assertions**:  
   - If your C++-processed errors use different fields (`ErrorMessage`, `ErrorCode`, `Data.Error`, etc.), update the assertions to mirror the expectations from your existing failing `/GetStd` test, so that `/v1/GetStd` is verified against the same error contract.
</issue_to_address>

### Comment 4
<location path="test/notification_manager.test.js" line_range="90-99" />
<code_context>
   assert.deepStrictEqual(socket.getSent(), []);
 });
+
+test('versioned notify endpoint works the same as legacy notify endpoint', async () => {
+  const manager = createManager();
+  const socket = createFakeWebSocket('alice');
+  manager.addSession('alice', socket);
+
+  const payload = { type: 'bbs_mention', data: { PostID: 11 } };
+  const response = await manager.fetch(new Request('https://dummy/v1/notify', {
+    method: 'POST',
+    headers: { 'X-Notification-Token': 'test-push-token' },
+    body: JSON.stringify({ userId: 'alice', notification: payload }),
+  }));
+
+  assert.strictEqual(response.status, 200);
+  assert.deepStrictEqual(socket.getSent(), [JSON.stringify(payload)]);
+});
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test for the versioned /v1/notify path to mirror the legacy unauthorized-case coverage.

To fully mirror the legacy behavior and protect against regressions in the aliasing, please also add a test where `/v1/notify` is called with a missing or invalid `X-Notification-Token`, asserting it returns `401` and does not send anything on the socket, matching the existing `/notify` unauthorized test.

Suggested implementation:

```javascript
test('versioned notify endpoint works the same as legacy notify endpoint', async () => {
  const manager = createManager();
  const socket = createFakeWebSocket('alice');
  manager.addSession('alice', socket);

  const payload = { type: 'bbs_mention', data: { PostID: 11 } };
  const response = await manager.fetch(new Request('https://dummy/v1/notify', {
    method: 'POST',
    headers: { 'X-Notification-Token': 'test-push-token' },
    body: JSON.stringify({ userId: 'alice', notification: payload }),
  }));

  assert.strictEqual(response.status, 200);
  assert.deepStrictEqual(socket.getSent(), [JSON.stringify(payload)]);
});

test('versioned notify endpoint rejects requests without a valid notification token', async () => {
  const manager = createManager();
  const socket = createFakeWebSocket('alice');
  manager.addSession('alice', socket);

  const payload = { type: 'bbs_mention', data: { PostID: 11 } };
  const response = await manager.fetch(new Request('https://dummy/v1/notify', {
    method: 'POST',
    // No X-Notification-Token header, mirroring the legacy unauthorized test
    body: JSON.stringify({ userId: 'alice', notification: payload }),
  }));

  assert.strictEqual(response.status, 401);
  assert.deepStrictEqual(socket.getSent(), []);

```

1. Ensure that the inserted positive test body (the `body: JSON.stringify(...)`, `assert.strictEqual(...)`, and `assert.deepStrictEqual(...)` lines) exactly matches what is already in your file; adjust indentation or trailing commas as needed.
2. If the legacy `/notify` unauthorized test uses a different payload shape or additional assertions, you may want to mirror those details in the new `/v1/notify` unauthorized test for stricter parity.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread test/process.test.js
Comment on lines +101 to +102
const response = await proc.Process();
const payload = await response.json();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add assertions on HTTP status (and possibly headers) to more fully validate the behavior of the new aliased endpoints.

For these new tests (e.g. /GetNotice, /v1/GetNotice, /, /v1/, /GetAddOnScript, /v1/GetAddOnScript), we only check the JSON body. Please also assert response.status (e.g. assert.strictEqual(response.status, 200)) and any key headers like content-type so we verify the aliased endpoints remain fully equivalent, not just in payload.

Suggested implementation:

    const response = await proc.Process();
    assert.strictEqual(response.status, 200);
    assert.match(response.headers.get('content-type') ?? '', /^application\/json\b/i);

    const payload = await response.json();
    assert.strictEqual(payload.Success, true);
    assert.strictEqual(payload.Data.Notice, 'test notice');
  1. For the other aliased endpoint tests mentioned (/v1/GetNotice, /, /v1/, /GetAddOnScript, /v1/GetAddOnScript), add the same pair of assertions immediately after const response = await proc.Process();:
    • assert.strictEqual(response.status, 200);
    • assert.match(response.headers.get('content-type') ?? '', /^application\/json\b/i); (or an equivalent check appropriate to the endpoint, e.g., for script/text responses you may want text/javascript or text/html instead of application/json).
  2. Ensure that these header/status assertions are added before reading the body with response.json() or response.text() so failures are surfaced early and consistently across tests.

Comment thread test/process.test.js
Comment on lines +262 to +271
test('Process serves image on legacy GET /GetImage endpoint', async () => {
const imageData = new Uint8Array([137, 80, 78, 71]).buffer;
const proc = createProcess({
req: new Request('https://example.com/GetImage?ImageID=test.png', {
method: 'GET',
headers: { "CF-Connecting-IP": "127.0.0.1" }
})
});

proc.ProcessFunctions.GetImage = test.mock.fn(async () => new Blob([imageData], { type: 'image/png' }));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Strengthen the GetImage tests by verifying the response body, not just headers and arguments.

Right now the test only checks that GetImage is called with { ImageID } and that content-type is image/png. To fully validate the behavior, also read the response body (e.g. via await response.arrayBuffer() or await response.blob()) and assert its length or bytes match the imageData you set up. This confirms the Blob from ProcessFunctions.GetImage is actually streamed to the client.

Suggested implementation:

test('Process serves image on legacy GET /GetImage endpoint', async () => {
    const imageData = new Uint8Array([137, 80, 78, 71]).buffer;
    const proc = createProcess({
        req: new Request('https://example.com/GetImage?ImageID=test.png', {
            method: 'GET',
            headers: { 'CF-Connecting-IP': '127.0.0.1' }
        })
    });

    proc.ProcessFunctions.GetImage = test.mock.fn(
        async () => new Blob([imageData], { type: 'image/png' })
    );

    const response = await proc.Process();

    // Validate the handler was invoked with the expected arguments
    assert.strictEqual(proc.ProcessFunctions.GetImage.mock.calls.length, 1);
    assert.deepStrictEqual(proc.ProcessFunctions.GetImage.mock.calls[0][0], {
        ImageID: 'test.png'
    });

    // Validate response headers
    assert.strictEqual(response.headers.get('content-type'), 'image/png');

    // Validate that the response body matches the image data we set up
    const bodyBuffer = await response.arrayBuffer();
    assert.strictEqual(bodyBuffer.byteLength, imageData.byteLength);
    assert.deepStrictEqual(
        new Uint8Array(bodyBuffer),
        new Uint8Array(imageData)
    );
});

If your test framework exposes mock call metadata differently than mock.calls, adjust the assertions on proc.ProcessFunctions.GetImage.mock.calls accordingly (e.g., mock.calls[0].arguments or similar), but keep the body validation via response.arrayBuffer() and Uint8Array comparison as implemented.

Comment thread test/process.test.js
Comment on lines +141 to +150
test('Process uses GetStd C++ string processing on /v1/GetStd', async () => {
const req = new Request('https://example.com/v1/GetStd', {
method: 'POST',
headers: {
"CF-Connecting-IP": "127.0.0.1",
"content-type": "application/json"
},
body: JSON.stringify({
Authentication: { SessionID: 'testsession', Username: 'testuser' },
Data: {},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding a negative/edge-case test for versioned GetStd to ensure aliasing doesn’t bypass error handling.

You’re already verifying that /GetStd and /v1/GetStd share the same happy-path C++ processing. Please also add a failing case (e.g., CheckToken failure or ProcessFunctions.GetStd error) for /v1/GetStd to confirm the alias follows the same error path and still returns C++-processed errors rather than plain JSON.

Suggested implementation:

    const payload = await response.json();
    assert.strictEqual(payload.Success, true);
    assert.strictEqual(payload.Data.Notice, 'test notice');
});

test('Process uses GetStd C++ error handling on /v1/GetStd for failing C++ processing', async () => {
    const req = new Request('https://example.com/v1/GetStd', {
        method: 'POST',
        headers: {
            "CF-Connecting-IP": "127.0.0.1",
            "content-type": "application/json"
        },
        body: JSON.stringify({
            Authentication: { SessionID: 'testsession', Username: 'testuser' },
            Data: {},
            Version: 'test',
        }),
    });

    const proc = new Process(req, {
        CheckToken: async () => ({
            Success: true,
            Data: { Username: 'testuser' },
        }),
        ProcessFunctions: {
            GetStd: {
                // Simulate a C++-side error path for GetStd
                post: async () => {
                    return {
                        ok: false,
                        status: 500,
                        json: async () => ({
                            Success: false,
                            Error: 'forced C++ GetStd failure',
                        }),
                    };
                },
            },
        },
    });

    const response = await proc.Process();
    const payload = await response.json();

    // Ensure the alias /v1/GetStd follows the same C++ error path as /GetStd
    assert.strictEqual(payload.Success, false);
    assert.strictEqual(payload.Error, 'forced C++ GetStd failure');
});

test('Process uses GetStd C++ string processing on /v1/GetStd', async () => {

You may need to adjust the new test to match your existing testing conventions and error shapes:

  1. Shape of CheckToken result:

    • If other tests return a differently shaped object (e.g. { Success: false, ErrorCode: '...' }), align this mock with that structure.
  2. Shape of ProcessFunctions.GetStd.post result:

    • I assumed it returns a Response-like object with ok, status, and json().
    • If your existing GetStd tests use a different mock (e.g. directly returning { Success: false, ... }), copy that pattern here and keep the failure semantics identical.
  3. Error assertions:

    • If your C++-processed errors use different fields (ErrorMessage, ErrorCode, Data.Error, etc.), update the assertions to mirror the expectations from your existing failing /GetStd test, so that /v1/GetStd is verified against the same error contract.

Comment on lines +90 to +99
test('versioned notify endpoint works the same as legacy notify endpoint', async () => {
const manager = createManager();
const socket = createFakeWebSocket('alice');
manager.addSession('alice', socket);

const payload = { type: 'bbs_mention', data: { PostID: 11 } };
const response = await manager.fetch(new Request('https://dummy/v1/notify', {
method: 'POST',
headers: { 'X-Notification-Token': 'test-push-token' },
body: JSON.stringify({ userId: 'alice', notification: payload }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a negative test for the versioned /v1/notify path to mirror the legacy unauthorized-case coverage.

To fully mirror the legacy behavior and protect against regressions in the aliasing, please also add a test where /v1/notify is called with a missing or invalid X-Notification-Token, asserting it returns 401 and does not send anything on the socket, matching the existing /notify unauthorized test.

Suggested implementation:

test('versioned notify endpoint works the same as legacy notify endpoint', async () => {
  const manager = createManager();
  const socket = createFakeWebSocket('alice');
  manager.addSession('alice', socket);

  const payload = { type: 'bbs_mention', data: { PostID: 11 } };
  const response = await manager.fetch(new Request('https://dummy/v1/notify', {
    method: 'POST',
    headers: { 'X-Notification-Token': 'test-push-token' },
    body: JSON.stringify({ userId: 'alice', notification: payload }),
  }));

  assert.strictEqual(response.status, 200);
  assert.deepStrictEqual(socket.getSent(), [JSON.stringify(payload)]);
});

test('versioned notify endpoint rejects requests without a valid notification token', async () => {
  const manager = createManager();
  const socket = createFakeWebSocket('alice');
  manager.addSession('alice', socket);

  const payload = { type: 'bbs_mention', data: { PostID: 11 } };
  const response = await manager.fetch(new Request('https://dummy/v1/notify', {
    method: 'POST',
    // No X-Notification-Token header, mirroring the legacy unauthorized test
    body: JSON.stringify({ userId: 'alice', notification: payload }),
  }));

  assert.strictEqual(response.status, 401);
  assert.deepStrictEqual(socket.getSent(), []);
  1. Ensure that the inserted positive test body (the body: JSON.stringify(...), assert.strictEqual(...), and assert.deepStrictEqual(...) lines) exactly matches what is already in your file; adjust indentation or trailing commas as needed.
  2. If the legacy /notify unauthorized test uses a different payload shape or additional assertions, you may want to mirror those details in the new /v1/notify unauthorized test for stricter parity.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces /v1/* aliases for existing HTTP API routes (and a /v1 default) while keeping legacy endpoints working, improving forward compatibility without breaking current clients.

Changes:

  • Added centralized path normalization in Process to map / and /v1[/] to GetNotice, and to route /v1/* to the corresponding legacy handler.
  • Extended notification and websocket routing to accept /v1/notify and /v1/ws/notifications.
  • Added/expanded tests covering both legacy and versioned routes, including GetStd C++ string processing behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Source/Process.ts Normalizes incoming pathnames to support /v1/* aliases and ensures /v1/GetStd retains the special C++ string response handling.
Source/NotificationManager.ts Accepts /v1/notify in addition to /notify for internal push delivery.
Source/index.ts Accepts /v1/ws/notifications in addition to /ws/notifications for websocket notification upgrades.
test/process.test.js Adds coverage for legacy + versioned endpoints (/, /v1, /v1/, GetNotice, GetAddOnScript, GetImage, GetStd, and generic /v1/* routing).
test/notification_manager.test.js Adds coverage ensuring /v1/notify behaves identically to /notify.

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

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

@boomzero boomzero left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I don't see why a migration would benefit us, but I hardly maintain anymore,so your decision

@PythonSmall-Q

PythonSmall-Q commented Apr 22, 2026 via email

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants