fix(agent): detect conflicting ic_env cookies instead of taking the first

[marc0olo]

Closes #1384.

Problem

getCanisterEnv resolved the environment with:

document.cookie.split(';').find(cookie => cookie.trim().startsWith(`${cookieName}=`));

Several ic_env cookies can coexist on one origin — a cookie is keyed on (name, domain, path), and partitioned cookies live in their own jar. The realistic trigger is a frontend canister upgraded between asset canister implementations that disagree on Partitioned: dfinity/sdk's ic-certified-assets writes SameSite=Lax, dfinity/certified-assets writes Secure; SameSite=None; Partitioned (confirmed against the live header on skills.internetcomputer.org). The two land in different jars on the same host and both stay readable.

The issue framed this as "whichever the browser happens to list first", but the order is specified: RFC 6265 §5.4 lists longer paths first and, among equal paths, earlier creation-times first. So first-match isn't arbitrary — it systematically prefers the stalest copy. The app is then configured against a canister that may no longer exist, and the resulting call rejection names nothing about cookies, so it reads as a broken replica.

Change

Collect every copy. Identical copies are common and stay silent; when they disagree, throw a new ConflictingCanisterEnvErrorCode naming the conflict. safeGetCanisterEnv keeps returning undefined.

No precedence rule is invented, deliberately. Sorting by Path or Partitioned isn't possible here — document.cookie exposes only name=value, and the attributes aren't there. cookieStore.getAll() would have them but is Chromium-only. And "prefer the non-partitioned copy" would be backwards: the authoritative mainnet writer is the partitioned one. Conflicting boot config is a broken environment; guessing is what produced the bug.

Copies are compared on the environment they resolve to, not on their text, because writers differ in ways that carry no meaning. Decoding handles percent-encoding — encodeURIComponent leaves _ alone while the asset canister encodes it as %5F. Resolving repeated variables last-wins (as parseEnvVars does) and then sorting handles emission order. Comparing raw or merely-decoded strings would report false conflicts between an asset canister and a dev server serving the same environment, which would be worse than the bug being fixed; sorting raw variables without resolving repeats would do the opposite and mask a real difference.

A copy whose percent-encoding is malformed can't be decoded at all. It carries no environment, so it isn't a second candidate to choose between — it's dropped, letting a valid sibling still be used, and MalformedCookieErrorCode is reported only when no copy survives. This also keeps decodeURIComponent's URIError from escaping an API that documents InputError.

Also replaces cookie.split('=')[1], which truncated any value containing an unencoded =, with the same first-separator slice parseEnvVars already used. It failed closed (MissingRootKeyErrorCode), so this is robustness rather than a fix.

Scope

Deliberately minimal. The investigation on #1384 also considered aligning cookie attributes across dfinity/certified-assets, dfinity/candid, dfinity/sdk and ~38 dev-server configs. That's dropped: local frontends are served at http://<canister-id>.localhost:8000/ and these are host-only cookies, so the dev-server/asset-canister collision that would have justified it doesn't occur. This reader change is also the only one that reaches canisters and projects already deployed — every writer change is upgrade-gated.

No new public functions — the only additions to the surface are the two error codes. getAllCanisterEnvs was considered as an escape hatch and left out until someone needs it.

Testing

Ten tests added: identical copies; copies differing only in encoding or variable order; disagreeing copies; three cookies holding two environments (pinning the count in the message); copies whose difference only repeated variables reveal; repeated variables in a consistent order resolving last-wins as one environment; a cookie whose name merely shares a prefix (ic_env_backup); a malformed copy alongside a valid one; no decodable copy at all; and the safeGetCanisterEnv path. Full suite green (646 passed, 36 snapshots), typecheck and lint clean.

The module is @experimental, so the behaviour change — throwing where it previously returned a guess — is in budget. It strictly dominates silently building an actor against a dead canister.

🤖 Generated with Claude Code

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 2ea4433.

Security Overview

• 🔎 Scanned files: 3 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/d57f9d06-f73e-44d7-87e6-48ac7e591b20?scanId=56bd03f1-3cbf-4290-a34f-a9f63ab4168f&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► packages/core/src/agent/canister-env/index.ts - Add handling for multiple identical cookies and conflict resolution between cookie copies - Introduce decodeCookieValue, getCookieValues, distinctEnvValues, canonicalEnvForm, splitEnvVars helpers - Improve error handling for malformed cookies and conflicting environments - Add documentation comments for new helpers
Bug Fix	► packages/core/src/agent/errors.ts - Add MalformedCookieErrorCode and ConflictingCanisterEnvErrorCode with corresponding messages

[github-actions]

size-limit report 📦

Path	Size
@icp-sdk/core	91 B (0%)
@icp-sdk/core/agent	53.07 KB (+0.07% 🔺)
@icp-sdk/core/candid	13.47 KB (0%)
@icp-sdk/core/identity	21.38 KB (0%)
@icp-sdk/core/identity/secp256k1	33.73 KB (0%)
@icp-sdk/core/principal	4.38 KB (0%)

[Copilot]

Pull request overview

This PR updates the experimental canister-environment cookie reader to detect and surface ambiguous configuration when multiple ic_env cookies exist on the same origin, avoiding silent selection of a potentially stale value.

Changes:

• Collect all matching cookie values, canonicalize them, and throw a new ConflictingCanisterEnvErrorCode when distinct environments are detected.
• Improve cookie value parsing by slicing on the first name= prefix rather than split('=')[1].
• Add test coverage for duplicate-cookie scenarios, including ordering/encoding normalization and safeGetCanisterEnv behavior.

Reviewed changes

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

File	Description
packages/core/src/agent/errors.ts	Adds a new error code to report conflicting canister-env cookie values.
packages/core/src/agent/canister-env/index.ts	Reads all cookie copies, deduplicates by canonicalized env-var ordering, and throws on disagreement.
packages/core/src/agent/canister-env/index.test.ts	Adds tests covering duplicate cookies (identical, normalized-equal, conflicting, prefix collisions) and safeGetCanisterEnv.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[marc0olo]

All three valid — applied in 1aae9f2. Notes on each, since two of them changed my reasoning and not just the wording.

URIError on malformed encoding — correct, and it's a regression this PR introduced rather than a pre-existing wart. Before, only the first match was decoded; decoding all copies means a corrupt stale duplicate can now break a call that a valid sibling would have served, and URIError isn't the InputError the docstring promises.

Decoding defensively now. On the question of what an undecodable copy means: I didn't surface it as a conflict as suggested, because a copy that can't be decoded carries no environment at all — it isn't a second candidate to choose between, so treating it as one would break working setups on corrupt data. It's dropped, and MalformedCookieErrorCode (new) is reported only when no copy survives. That keeps the "never guess between two environments" property while still naming the corruption when corruption is all there is.

Conflict message count — right, and it undercut the point of the PR. Three cookies holding two environments reported "Found 2 ic_env cookies". Now counts values: "Found 2 conflicting values for the 'ic_env' cookie". Test added pinning the 3-cookies/2-environments case specifically.

distinctEnvValues rationale — right, and the comment was simply wrong. Decoding is what normalises _ vs %5F; by the time values reach that function the encoding difference is gone, so sorting is justified by variable order alone. The comparison was already correct — the explanation for it wasn't. Reworded, and I've fixed the same claim in the PR description above, which had inherited it.

Full suite green (647 passed, 36 snapshots), typecheck and lint clean.

[Copilot]

Pull request overview

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

Suppressed comments (1)

packages/core/src/agent/canister-env/index.ts:232

• distinctEnvValues canonicalizes by sorting the raw key=value segments. If a cookie contains duplicate keys (e.g. A=1&A=2), the resulting environment depends on segment order because parseEnvVars ultimately uses Object.fromEntries (last value wins). Sorting makes A=1&A=2 and A=2&A=1 look identical, which can incorrectly suppress a real conflict between two cookies whose effective parsed env differs.

function distinctEnvValues(decodedValues: string[]): string[] {
  const byCanonicalForm = new Map<string, string>();

  for (const value of decodedValues) {
    const canonicalForm = value.split(ENV_VAR_SEPARATOR).sort().join(ENV_VAR_SEPARATOR);
    if (!byCanonicalForm.has(canonicalForm)) {
      byCanonicalForm.set(canonicalForm, value);
    }
  }

  return Array.from(byCanonicalForm.values());

[marc0olo]

The suppressed comment on distinctEnvValues is correct. Verified it directly:

canonical equal?  true
effective A in a: 2 | effective A in b: 1
=> genuinely different envs treated as identical: true

Sorting the raw key=value segments made A=1&A=2 and A=2&A=1 compare equal, while Object.fromEntries resolves them last-wins to different values for A. A genuine conflict, silently suppressed — which is exactly the failure class this PR exists to remove.

Fixed in 2ea4433 by resolving repeated variables before sorting, so the canonical form reflects the environment a cookie actually produces rather than its raw text. The entry splitting is now shared with parseEnvVars instead of duplicated, so the two can't drift apart on how they read the same format.

Worth being clear about reachability, since Copilot suppressed this for a reason and I agree with its confidence assessment: no writer emits repeated variables. certified-assets builds from a BTreeMap, the dfx-era canister from a prefixed map, and the Vite templates from fixed literal keys — all unique by construction. Triggering this would need two cookies each carrying a repeated variable in different orders. I fixed it regardless, because a canonical form that can mask a real difference undercuts the argument the rest of this PR rests on, and the fix removes duplicated parsing rather than adding a special case.

Two tests added: one asserting the conflict is now detected, one asserting that repeated variables in a consistent order still count as a single environment and resolve last-wins.

Full suite green (649 total, 646 passed, 36 snapshots), typecheck and lint clean.