Skip to content

fix(worker): restore registered lesson submissions - #1293

Open
hjqcan wants to merge 1 commit into
Ikalus1988:mainfrom
hjqcan:codex/fix-kv-token-verification
Open

fix(worker): restore registered lesson submissions#1293
hjqcan wants to merge 1 commit into
Ikalus1988:mainfrom
hjqcan:codex/fix-kv-token-verification

Conversation

@hjqcan

@hjqcan hjqcan commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • restore the GitHub endpoint constants removed during the Worker module split
  • cover the full register -> KV lookup -> authenticated write_lesson -> mocked GitHub submission path
  • document that write_lesson needs the registered token in both the Bearer header and tool argument
  • update the mixed-invalid-load test to use a protected tool now that tools/list is intentionally public

Root cause

A live synthetic registration confirmed that a fresh mcp_ token passes both the top-level Bearer check and the write_lesson KV lookup immediately. The request then failed later because commit fd5a541 removed GITHUB_API, REPO, and PUBLIC_DATA_BASE from register-proxy-sw.js without importing or redefining them. The resulting ReferenceError made every complete write_lesson submission fail even though token verification succeeded.

No real token or lesson was submitted during the live smoke test.

Validation

  • node --check workers/register-proxy-sw.js
  • node --expose-gc --test workers/mcp-endpoint.stress.test.mjs (5 passed)
  • node --test workers/.test.mjs workers/email-register/.test.mjs (69 passed, 1 skipped)
  • git diff --check

Node ID: hjqcan/goodmemory-maintainer

Closes #1240

Signed-off-by: hjqcan <hjqcan@163.com>
@hjqcan
hjqcan requested a review from Ikalus1988 as a code owner August 25, 2026 02:47
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to MisakaNet!

Thanks for your first PR!

Fix DCO First

Every commit needs Signed-off-by:. If DCO fails:

git commit --amend --signoff --no-edit
git push --force-with-lease

Quick Links

Optional: Local Pre-commit Hook

For instant DCO validation before pushing:

pip install pre-commit
pre-commit install --hook-type commit-msg

For Agents & Crawlers

Found a missing lesson? Submit via MCP (no account needed):

curl -sS https://misakanet.org/mcp \
  -H 'Content-Type: application/json' \
  -H 'MCP-Protocol-Version: 2025-06-18' \
  -d '{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"misakanet_submit_intake","arguments":{"problem":"YOUR PROBLEM","source":"your-agent"}}}'

CI runs automatically once DCO passes.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@hjqcan

hjqcan commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

CI triage: the only red check is the full-repository audit job. It fails in 7 existing Python tests (tests/test_mcp_deepseek_adapter.py and tests/test_mcp_fallback.py) after 750 tests pass; this PR changes only the remote MCP documentation and two Worker JavaScript files. The dedicated Worker stress check, build, DCO, isolation, PR review, and all 8 macOS/Linux/Windows matrix jobs pass. I have kept the unrelated SAG/BM25 baseline failures out of this focused fix.

@hjqcan

hjqcan commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

The unrelated audit baseline failures are now fixed in #1294. Its full audit and every cross-platform check are green. Once #1294 merges, I will update this branch onto the repaired main baseline and rerun #1293.

@Ikalus1988

Copy link
Copy Markdown
Owner

@hjqcan This PR has merge conflicts with main. Please rebase:

The fix itself (restoring GITHUB_API constants) looks correct. Once conflicts are resolved, this should be good to merge.

@Ikalus1988

Copy link
Copy Markdown
Owner

/review

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 317d9e7)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1240 - Partially compliant

Compliant requirements:

  • misakanet_write_lesson works with registered token
  • KV lookup returns correct token data
  • No regression on other tools (mixed-invalid-load test still asserts 50/50 split)
  • Documentation updated with auth requirements (docs/integrations/mcp-remote.md)

Non-compliant requirements:

  • None

Requires further human verification:

  • Live Worker deploy validation that the ReferenceError is gone end-to-end (PR description reports a synthetic smoke test, but no production deploy confirmation)
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

fetch mock leaks across tests

The new "accepts a newly registered KV token for write_lesson" test installs t.mock.method(globalThis, 'fetch', …) on the global object. If any assertion above the mocked fetch call throws, the mock may remain installed for subsequent tests in the same file, potentially breaking other tests that rely on real fetch (e.g., tests that touch the network indirectly or that re-import modules capturing the original fetch). Confirm the assertion order is robust and that Node's t.mock.method is guaranteed to restore on early throws. If not, wrap the mock setup in t.after/explicit restore or move it into a narrower scope.

test('accepts a newly registered KV token for write_lesson', async (t) => {
  const kv = new MemoryKv();
  const testEnv = { MISAKANET_KV: kv, REGISTER_TOKEN: 'github-test-token' };
  const registerResponse = await worker.fetch(toolCall(
    1,
    'misakanet_register',
    { agent_type: 'node-test' },
  ), testEnv);
  const registration = toolResult(await registerResponse.json());

  t.mock.method(globalThis, 'fetch', async (input, init) => {
    assert.equal(String(input), 'https://api.github.com/repos/Ikalus1988/MisakaNet/issues');
    assert.equal(init.headers.Authorization, 'Bearer github-test-token');
    return new Response(JSON.stringify({
      number: 1240,
      html_url: 'https://github.com/Ikalus1988/MisakaNet/issues/1240',
    }), {
      status: 201,
      headers: { 'Content-Type': 'application/json' },
    });
  });

  const writeResponse = await worker.fetch(toolCall(
    2,
    'misakanet_write_lesson',
    {
      title: 'KV token verification regression',
      domain: 'mcp',
      problem: 'A newly registered token was rejected by the remote MCP endpoint.',
      root_cause: 'The write path did not complete after validating the KV token record.',
      fix: 'Verify the stored token and submit the reviewed lesson through the GitHub API.',
      verification: 'Register, write with the same token, and observe a pending-review receipt.',
      token: registration.token,
      source: 'node-test',
    },
    registration.token,
  ), testEnv);
  const result = toolResult(await writeResponse.json());

  assert.equal(writeResponse.status, 200);
  assert.equal(result.submitted, true);
  assert.equal(result.lesson_id, 'issue-1240');
  assert.ok(kv.readKeys.includes(`mcp_token:${registration.token}`));
});
Test depends on auth required for unauthenticated tools/call

The "returns stable errors under mixed invalid load" test now sends tools/call with misakanet_search (no Bearer header) and expects 50× 401 responses. The ticket explicitly states misakanet_search has "None (rate-limited)" auth requirement. If the auth check is gated on tools/call generally, this test works, but if the implementation later decides misakanet_search is truly public, these assertions will silently fail and the test name ("stable errors under mixed invalid load") will no longer reflect what it covers. Consider asserting against a tool whose auth posture is stable and documented (or commenting why the public tool returns 401).

test('returns stable errors under mixed invalid load', async () => {
  const requests = Array.from({ length: 100 }, (_, id) => {
    if (id % 2 === 0) return mcpRequest('{not-json');
    return new Request('https://misakanet.org/mcp', {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
        jsonrpc: '2.0',
        id,
        method: 'tools/call',
        params: { name: 'misakanet_search', arguments: { query: 'auth test' } },
      }),
    });
  });
  const responses = await Promise.all(requests.map((request) => worker.fetch(request, env)));

  assert.equal(responses.filter((response) => response.status === 400).length, 50);
  assert.equal(responses.filter((response) => response.status === 401).length, 50);
});
Token duplication in docs example

The documentation tells clients to send the same MCP token in both the Authorization: Bearer header and inside the JSON-RPC arguments.token field. Duplicating a bearer token in the request body widens the surface area for accidental logging/replay and contradicts typical "bearer-only" MCP patterns. If dual submission is intentional (e.g., to bind the call to a specific token in a multi-tenant KV), document the threat model; otherwise prefer validating the token from the header alone.

`misakanet_write_lesson` validates the registered token twice: send the same
token in the Bearer header and in the tool's `token` argument. Submissions are
created as `pending-review` issues rather than published directly.

```bash
curl -sS https://misakanet.org/mcp \
  -H "Authorization: Bearer $MISAKANET_MCP_TOKEN" \
  -H "Content-Type: application/json" \
  -d "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/call\",\"params\":{\"name\":\"misakanet_write_lesson\",\"arguments\":{\"title\":\"Example failure\",\"domain\":\"mcp\",\"problem\":\"Describe what failed and the observed behavior.\",\"root_cause\":\"Describe the verified cause of the failure.\",\"fix\":\"Describe the concrete change that resolved it.\",\"verification\":\"Describe how the fix was verified.\",\"token\":\"$MISAKANET_MCP_TOKEN\"}}}"

</details>

</td></tr>
</table>

@Ikalus1988

Copy link
Copy Markdown
Owner

⚠️ This PR has merge conflicts with main. Please rebase:

git fetch origin
git rebase origin/main
# resolve conflicts
git push --force-with-lease

@Ikalus1988

Copy link
Copy Markdown
Owner

Thanks — the stress-test additions are useful. One correction needed before merge: on current main, misakanet_write_lesson no longer accepts an args.token argument — the Bearer header is the canonical auth path (the token arg was deprecated; args.token is explicitly ignored). The doc snippet added here (sending the token in both Bearer and the token argument) would mislead users. Please rebase and update the docs to Bearer-only, keeping the stress test.

@Ikalus1988

Copy link
Copy Markdown
Owner

Heads-up: main has moved since this branch (Coogen Phase 2 touched workers/register-proxy-sw.js heavily). When you update the docs to Bearer-only (the token arg is deprecated on main) and rebase, the stress-test additions should apply cleanly — please re-run node --test workers/mcp-endpoint.stress.test.mjs.

@Ikalus1988

Copy link
Copy Markdown
Owner

Maintainer check-in (2026-08-30): this PR currently shows mergeable=false / dirty — main has moved ahead since your branch (recent merges: data-pipeline fixes, intake E2E tests, Coogen borrowings). Could you rebase onto current main and re-push? All CI checks were green on your head commit, so once rebased it should be ready to merge. Happy to help if you run into rebase conflicts.

@Ikalus1988

Copy link
Copy Markdown
Owner

/review

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 317d9e7

@Ikalus1988

Copy link
Copy Markdown
Owner

Automated review (pr-agent) flagged: the new KV-token test installs t.mock.method(globalThis, "fetch", …) without restoring it — if an earlier assertion throws, the mock leaks into subsequent tests. Use try/finally or t.mock.restoreAll().

Also: PR is still mergeable=false (dirty) — main has moved ahead. Please rebase onto current main so we can merge (all CI checks were green on your head commit).

@Ikalus1988

Copy link
Copy Markdown
Owner

Maintainer rebase check (2026-08-31): I rebased your branch locally onto current main — it rebases cleanly.

The only conflict was in workers/register-proxy-sw.js: your branch inlines REPO/GITHUB_API/PUBLIC_DATA_BASE constants, but main now imports them from ./lib/handlers.js (same values). Keeping main's import resolves it; your docs + stress-test changes (the actual fix value) carry over intact.

To update the PR: rebase your fork branch onto main and force-push:

git fetch upstream main
git rebase upstream/main
git push --force-with-lease origin <your-branch>

Worker tests pass (95/96) after the rebase. Once pushed, the PR becomes mergeable.

@Ikalus1988

Copy link
Copy Markdown
Owner

Audit失败:多个测试失败

问题

MCP fallback相关测试失败,共7个测试失败。

失败测试

解决方案

需要检查MCP fallback逻辑是否正确。可能的原因:

  1. Fallback逻辑实现与测试预期不符
  2. 搜索结果格式不符合预期
  3. Domain过滤逻辑有问题

验证

修复后可以本地运行以下命令验证:
============================= test session starts =============================
platform win32 -- Python 3.11.9, pytest-9.0.3, pluggy-1.6.0 -- C:\Users\hp\AppData\Local\Programs\Python\Python311\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\hp\MisakaNet
configfile: pyproject.toml
plugins: anyio-4.12.1, cov-7.1.0
collecting ... collected 8 items

tests/test_mcp_fallback.py::test_fallback_returns_results_instead_of_error PASSED [ 12%]
tests/test_mcp_fallback.py::test_fallback_source_is_distinct PASSED [ 25%]
tests/test_mcp_fallback.py::test_fallback_results_have_lesson_shape PASSED [ 37%]
tests/test_mcp_fallback.py::test_fallback_matches_real_content PASSED [ 50%]
tests/test_mcp_fallback.py::test_fallback_respects_top_limit PASSED [ 62%]
tests/test_mcp_fallback.py::test_fallback_empty_query_is_rejected PASSED [ 75%]
tests/test_mcp_fallback.py::test_fallback_domain_filter PASSED [ 87%]
tests/test_mcp_fallback.py::test_sag_still_preferred_when_available PASSED [100%]

============================== 8 passed in 0.23s ==============================
============================= test session starts =============================
platform win32 -- Python 3.11.9, pytest-9.0.3, pluggy-1.6.0 -- C:\Users\hp\AppData\Local\Programs\Python\Python311\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\hp\MisakaNet
configfile: pyproject.toml
plugins: anyio-4.12.1, cov-7.1.0
collecting ... collected 1 item

tests/test_mcp_deepseek_adapter.py::test_deepseek_recovery_smoke_passes PASSED [100%]

============================== 1 passed in 0.10s ==============================

修复后重新提交,audit应该会通过。

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MCP] Fix KV token verification for write_lesson

2 participants