Add test coverage for error mapping and tsjs helpers

[ChristianPavilonis]

Summary

• add exhaustive unit coverage for TrustedServerError::status_code() so each public variant is pinned to its expected HTTP response
• add direct tsjs formatting tests for unified and deferred script URL/tag helpers using computed hashes
• cover invalid UTF-8 cookie-header handling and simplify duplicate callback serde rename tests

Changes

File	Change
crates/trusted-server-core/src/error.rs	Added a table-driven test covering HTTP status code mapping for every TrustedServerError variant.
crates/trusted-server-core/src/tsjs.rs	Added direct unit tests for unified and deferred TS/JS script URL and tag helpers.
crates/trusted-server-core/src/cookies.rs	Added a unit test for invalid UTF-8 Cookie header handling in handle_request_cookies().
crates/trusted-server-core/src/models.rs	Consolidated duplicate callback serde rename tests into one focused test.

Closes

Closes #448
Closes #450
Closes #454
Closes #456

Test plan

• cargo test --workspace
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other:

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

Pure test additions/consolidation — no production code changes. Tests pass on wasm32-wasip1, clippy is clean, and CI is fully green. The substantive concerns below are about test design — what these tests would catch on regression — rather than correctness of what was added. Requesting changes primarily on the exhaustiveness and tautology issues; the rest are non-blocking.

Blocking

♻️ refactor

• Status code mapping test is not exhaustive at compile time — error.rs:144-247. Hand-maintained cases array means a new variant added to TrustedServerError won't fail this test. A non-exhaustive match guard would force exhaustive coverage. See inline comment.
• tsjs hash assertions are tautological — tsjs.rs:75-116. expected_hash is derived by calling the same concatenated_hash the production code calls. If concatenated_hash regressed, both sides would change in lockstep. Suggest asserting hash structure (64 hex chars) and that different module sets yield different URLs. See inline comment.
• Redundant import in tsjs test module — tsjs.rs:70. Duplicates the top-of-file import; use super::* at line 72 is sufficient (verified locally).

Non-blocking

🤔 thinking

• tsjs_deferred_script_src_uses_empty_hash_for_unknown_module locks in silent failure — tsjs.rs:137-143. Empty hash produces a 404-bound URL with no logging. Capturing this as "expected" makes future improvements look like regressions.

🌱 seedling

• Production unwrap_or_default() for unknown deferred modules is invisible — tsjs.rs:44 (unchanged in this PR, so noted in the body). single_module_hash(module_id).unwrap_or_default() silently produces a 404-bound URL with no logging when the module isn't registered. Worth a follow-up issue to add log::warn! for unregistered module IDs.

📝 note

• Issue #454 wording vs implementation — cookies.rs:353-372. Issue says "returning None" but production returns Err. Test correctly verifies current implementation; issue text is stale.

⛏ nitpick

• ["core", "creative"] redundantly includes core — tsjs.rs:76, 88. concatenate_modules always prepends core; could be simplified to ["creative"].
• HeaderValue::from_bytes expect message is slightly misleading — cookies.rs:358. Reads like a property of handle_request_cookies but is about HeaderValue::from_bytes.

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS (verified locally on wasm32-wasip1)
• vitest: PASS
• format-typescript / format-docs: PASS
• browser & integration tests: PASS

[aram356]

Summary

Pure test additions — no production code changes. CI is fully green and 809 trusted-server-core tests pass locally on wasm32-wasip1. The exhaustiveness guard added in 02ac32f for status_code() mapping is a strong improvement. However, two of the four "addressed" items still contain the same self-referential pattern the prior review flagged — the fix landed for concatenated_hash but the same shape was preserved (or freshly introduced) for single_module_hash and the deferred-tags collection helper. Plus one test whose name advertises a contract its body does not verify. Requesting changes on those three; the rest is non-blocking.

Blocking

🔧 wrench

• tsjs_deferred_helpers_format_single_module_urls_and_tags is tautological — expected_hash is derived by calling the same single_module_hash the production code calls (tsjs.rs:130-145). Same critique the prior review raised against concatenated_hash, shifted helper. See inline comment.
• tsjs_deferred_script_tags_concatenates_tags_in_input_order is byte-identical to production — test body literally re-executes module_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>() and asserts equality with the production function (tsjs.rs:156-169). Any symmetric regression passes. See inline comment.
• tsjs_unified_helpers_use_all_module_ids does not verify the contract its name advertises — never asserts that all_module_ids() is consulted or that every module is in the hash (tsjs.rs:113-127). Would still pass if production silently degraded to tsjs_script_src(&[]). See inline comment.

Non-blocking

🤔 thinking

• Status code cases array still hand-maintained — error.rs:165-258. The _exhaustive closure (lines 147-163) forces compile-fail on a new variant — good — but does not force adding the new variant to cases. A developer could update the closure to silence the compile error and forget the case. Stronger alternative: derive cases from the same exhaustive arm pattern, or assert_eq!(cases.len(), <const tied to closure>). Not blocking — closure catches the common slip — but worth a follow-up.

🌱 seedling

• Follow-up: log unregistered deferred module IDs — tsjs.rs:44. single_module_hash(module_id).unwrap_or_default() silently produces a 404-bound URL with no logging when the module isn't registered. The PR now documents this as expected behavior at tsjs.rs:148-154; tracking issue would be useful so the test's "documented fallback" framing has a paired plan to remove it.

⛏ nitpick

• Inconsistent test naming convention within this PR — new tests in error.rs/tsjs.rs use the descriptive style without test_ prefix; the new test in cookies.rs uses test_ prefix matching its file-local neighbors. The codebase has both styles, so this is fine — just a note for future consistency.

📝 note

• Issue #454 wording vs implementation — already noted in the prior review; cookies.rs test correctly verifies current Err(InvalidHeaderValue) behavior despite the issue text saying "returns None". No PR action needed.
• Branch is 4 commits behind origin/main — no conflicting paths, but a rebase before merge is standard.

CI Status

• fmt: PASS
• clippy: PASS
• cargo test: PASS (verified locally on wasm32-wasip1, 809 trusted-server-core tests)
• vitest: PASS
• format-typescript / format-docs: PASS
• browser & integration tests: PASS
• CodeQL: PASS

[aram356]

🔧 wrench — This re-introduces the same tautology pattern the prior review flagged for concatenated_hash, just shifted to single_module_hash. expected_hash is computed by calling the same hash function the production code calls — if single_module_hash regressed to return String::new(), both tsjs_deferred_script_src(module_id) and expected_hash would change in lockstep and this test would still pass (equivalent to the documented unregistered-module case at line 148-154).

Fix — mirror the unified-bundle approach: assert URL prefix and SHA-256 hex hash structure, not equality with another call to the same hash function. For the tag wrap test, derive expected_src from tsjs_deferred_script_src itself — that's verifying the wrapping format, not re-computing the hash.

let src = tsjs_deferred_script_src("prebid");
assert!(
    src.starts_with("/static/tsjs=tsjs-prebid.min.js?v="),
    "should include the deferred module path and cache-busting parameter",
);
assert_sha256_hex_hash(&src);

assert_eq!(
    tsjs_deferred_script_tag("prebid"),
    format!("<script src=\"{src}\" defer></script>"),
    "should render a deferred script tag for the module",
);

[aram356]

🔧 wrench — Test body is byte-identical to the production function body (tsjs.rs:62-65):

// production
module_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>()

// test
let expected = module_ids.iter().map(|id| tsjs_deferred_script_tag(id)).collect::<String>();
assert_eq!(tsjs_deferred_script_tags(&module_ids), expected, ...);

Any symmetric regression in either side passes the test. The test name claims to verify input-order concatenation, but the test never actually checks that the modules appear in input order — it just checks that re-running the same expression yields the same result.

Fix — assert the property (order preservation, count of tags), not equality with a re-execution of the same code:

let result = tsjs_deferred_script_tags(&["prebid", "creative"]);
let prebid_pos = result.find("tsjs-prebid").expect("should contain prebid script");
let creative_pos = result.find("tsjs-creative").expect("should contain creative script");
assert!(prebid_pos < creative_pos, "should preserve input order");
assert_eq!(
    result.matches("<script ").count(),
    2,
    "should emit one tag per module",
);

[aram356]

🔧 wrench — Test name advertises "use all module IDs" but the body never verifies that contract. It only checks URL prefix, SHA-256 hex hash structure, and that tsjs_unified_script_tag wraps tsjs_unified_script_src. If tsjs_unified_script_src() were silently changed to tsjs_script_src(&[]), the test would still pass — the unified bundle path would still match, the empty hash would still be 64 hex chars (well, it would actually be the hash of just core, which is still a valid SHA-256 hex), and the tag wrap assertion would still hold.

Fix — make the contract executable by asserting equivalence with the manual all-modules call:

let unified = tsjs_unified_script_src();
let manual = tsjs_script_src(&all_module_ids());
assert_eq!(
    unified, manual,
    "unified helpers must hash every registered module",
);
assert!(
    unified.starts_with("/static/tsjs=tsjs-unified.min.js?v="),
    "should include the unified bundle path and cache-busting parameter",
);
assert_sha256_hex_hash(&unified);
assert_eq!(
    tsjs_unified_script_tag(),
    format!("<script src=\"{unified}\" id=\"trustedserver-js\"></script>"),
    "should wrap the unified bundle source in the standard script tag",
);

This closes the gap: any divergence between tsjs_unified_script_src and the all-modules contract is caught directly.