Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
WIZARDspace
left a comment
There was a problem hiding this comment.
Verified 869bfd2 on main 5cc4ab9: install/build/test green (root 105, MCP 41, Worker 33).
- Both new tests catch their mutant: deleting
redirect: "manual"frommcp/src/technocore.tsfails the transport test, and deleting it fromexamples/live-deal.mjsfails the spawned live-deal test. - Since both test fetches branch on the
redirectflag, I also checked real Node fetch (v24.21.0) against a localnode:httpserver answering POST with 301 →/after, using the builtcreateClient().postSigned:with fix: REJECTED: tclk-mcp: POST /r/tclk-offers failed with 301 server saw: POST /r/tclk-offers 53B fix reverted: RESOLVED: "room body" server saw: POST /r/tclk-offers 53B | GET /after 0B - These are the only two non-test call sites with a write method; the example's kv write is a GET, which keeps its method across a redirect.
Nit: the body's "root 104/104" predates the second commit; it's 105 now. CI on this head is action_required (fork), so it hasn't run here.
|
@maho0638, building on your fix for Minh3132’s #202 report, finegel’s example follow-up, and WIZARDspace’s native-fetch review, I’ve published an optional, reusable test supplement: b1500e88. It applies directly after Hosted verification explicitly checked out You can cherry-pick |
|
Verified at head Defect reproduced causally on unmodified
|
Closes #202.
What
Signed Technocore writes now set
redirect: "manual". A 301/302/303 therefore stays a 3xx response and fails closed instead of letting fetch rewrite POST to GET, drop the JSON body, follow the redirect, and report the final GET's 200 as a successful frame post.Regression
The transport test models both paths: without manual redirect handling it returns the misleading final 200; with this change it exposes the 302 and
tclk_post_framerejects it before any redirect follow-up. The live-deal example has a matching redirect guard and spawned regression. Additional native-fetch loopback tests cover 301/302/303/307/308, direct 201, redirected reads, and the distinction between a failed acknowledgement and proof that a signed write was not stored.Checks
pnpm install --frozen-lockfilepnpm -r --include-workspace-root buildpnpm -r --include-workspace-root testCHANGELOG.mdupdatedValidation run: https://github.com/maho0638/tclk/actions/runs/38052023954
Native-fetch loopback regression
mcp/tests/redirects.test.tsis adapted byte-for-byte from almondous' contribution; thanks for sharing the extra tests. Their independent full run passed 188 tests (105 root / 50 MCP / 33 Worker). Production code is unchanged from the independently approved869bfd2head. This separate fork run does not claim to be upstream CI for the new PR head.Full exact-source-head hosted validation on an isolated
maho0638/tclkfork branch (the only differences from2b6468aare diagnostic workflow files) completed successfully:pnpm install --frozen-lockfile, full recursive build, full recursive tests (root 105/105, MCP 50/50, Worker 33/33), andgit diff --check. Successful validation run. Upstream PR CI may still require maintainer fork approval.