fix(artifacts): isolate in-memory composite keys

[fallintoplace]

Summary

Encode in-memory artifact keys as scoped tuples instead of joining components with slashes.

The previous key format could map distinct artifacts to the same array when a nested filename overlapped with a separator in another key component. That combined version histories and allowed loads or deletes through one logical key to affect another.

The explicit scope marker also keeps user-scoped artifacts separate while preserving existing listing behavior.

Tests

• Added regression coverage for a nested filename and overlapping session ID
• npx vitest run --project unit:core
• Prettier and ESLint checks for changed files
• Workspace build

Fixes #574

[Varun-S10]

Hi @kalenkevich, I have checked the issue and validated the proposed changes. I was able to reproduce the issue. Could you please review this PR?

[AmaadMartin]

The key encoding is correct as far as I can tell: JSON.stringify over a tagged tuple is injective for these components (a sessionId containing "," is escaped to \",\" inside the quotes, so it can't forge a field boundary), the 'session'/'user' discriminant closes the sessionId === 'user' ambiguity the flat key had, and listArtifactKeys still emits user-scoped names with their user: prefix, so artifact_service_test_utils.ts:225 keeps passing. I grepped for every place the old flat key was built or split: all six reads go through artifactPath, the store is private, and nothing outside this file (dev API server, runner tests) depends on the key format, so there is no migration surface for an in-memory store. I also type-checked the tuple narrowing (key[0] === 'session' -> key[4]) against TS 5.9 strict in a scratch file — it compiles. Three comments below: one uncovered half of the fix, one readability nit, and a heads-up that GcsArtifactService still has this bug.

[AmaadMartin]

Not a nit, but out of scope for this PR — worth a follow-up issue rather than a change here.

The encoding on this line is right, but GcsArtifactService still builds its object names the way this file used to, so the exact repro from #574 conflates artifacts there too:

// core/src/artifacts/gcs_artifact_service.ts:283-285
const prefix = isUser
  ? `${appName}/${userId}/user/${cleanFilename}`
  : `${appName}/${userId}/${sessionId}/${cleanFilename}`;

{sessionId: 'session', filename: 'nested/report.txt'} and {sessionId: 'session/nested', filename: 'report.txt'} both resolve to app/user/session/nested/report.txt/<version>.

It is actually worse there than it was here. GCS strips the user: prefix before writing (cleanFilename, line 281), so a session literally named user collides with the user namespace outright: {sessionId: 'user', filename: 'foo.txt'} and {filename: 'user:foo.txt'} land on the same blob. The in-memory service never had that particular collision, because it kept user: inside the stored filename.

For contrast, FileArtifactService is the sibling that already gets this right — assertSafeSegment (file_artifact_service.ts:411) rejects separators in userId/sessionId, and user- vs session-scoped artifacts live under structurally distinct roots.

I am not asking you to fix GCS in this PR; flagging it so #574 doesn't get closed as fully resolved when only one of the two implementations is fixed.

[AmaadMartin]

Nit, optional.

The declaration is positional, so the reads in listArtifactKeys (key[3] for user scope, key[4] for session scope) are hard to check without scrolling back here.

type ArtifactStorageKey =
  | ['session', string, string, string, string]
  | ['user', string, string, string];

Labelled tuple members are erased at runtime and make the indices self-explanatory:

type ArtifactStorageKey =
  | [
      'session',
      appName: string,
      userId: string,
      sessionId: string,
      filename: string,
    ]
  | ['user', appName: string, userId: string, filename: string];

Genuine trade-off: prettier will wrap the session arm as shown, so 3 lines become 9. Take it or leave it — I checked the labels don't affect the key[0] === 'session' narrowing.

Separately, while you're in this area: the JSDoc above artifactPath (lines 201-209) still reads "Constructs the path to the artifact" / "@return The path to the artifact", and every caller still names the result path. The point of this change is that the value is no longer a path, and leaving the wording invites someone to "simplify" it back to slashes later. Renaming the function would touch six call sites, so a one-line doc tweak is probably the right size of fix.

[AmaadMartin]

Not a nit.

This pins the separator collision from #574, but not the other half of the change. The scope marker also fixes a cross-session listing leak, and nothing exercises it.

Under the old key scheme:

const usernamespacePrefix = `${appName}/${userId}/user/`;
...
} else if (path.startsWith(usernamespacePrefix)) {

an artifact saved into a session literally named user was stored at app/<user>/user/foo.txt, which matches usernamespacePrefix — so listArtifactKeys for any other session of the same user returned foo.txt. The 'session' / 'user' discriminant is what stops that, and it is the part the PR description calls out ("the explicit scope marker also keeps user-scoped artifacts separate"), but a regression would go unnoticed:

it('does not leak a session named "user" into other sessions', async () => {
  const service = new InMemoryArtifactService();

  await service.saveArtifact({
    appName: 'app',
    userId: 'user',
    sessionId: 'user',
    filename: 'foo.txt',
    artifact: {text: 'session-scoped'},
  });

  const keys = await service.listArtifactKeys({
    appName: 'app',
    userId: 'user',
    sessionId: 'other',
  });

  expect(keys).toEqual([]);
});

I traced this by hand rather than running it: on the pre-change code the save lands at app/user/user/foo.txt and the usernamespacePrefix branch pushes foo.txt, so it fails; on your code key[0] === 'session' with key[3] === 'user' !== 'other' skips it, so it passes.

Also worth one more line in the test above — loading {sessionId: 'session/nested', filename: 'report.txt'} and asserting artifact-b — so the assertion pins both sides of the split instead of only proving the first key won.

[kalenkevich]

Can we just encode/decode every part of the path (appName, userId, sessionId, filename) to not to have the delimiter (/)?

No need to create an array as a key. I think this is antipatern and better to keep keys as numbers or strings but not as objects.

[AmaadMartin]

Re-checked at d0ff1087. The tuple key is gone in favour of Alexey's per-segment encodeURIComponent, which I think is the better answer — the key stays a string and the ambiguity is removed at the source rather than encoded in a shape.

I re-derived the collisions by hand against the new scheme rather than trusting the tests: {sessionId: 'session', filename: 'nested/report.txt'} -> session/app/user/session/nested%2Freport.txt vs {sessionId: 'session/nested', filename: 'report.txt'} -> session/app/user/session%2Fnested/report.txt, distinct. / now only ever appears as a separator, so the startsWith(prefix) scans in listArtifactKeys can't over-match a sibling segment (.../s/ no longer prefixes .../s2/...), and the session/user discriminant keeps a session literally named user out of the user namespace — which is what the test I asked for pins. Both sides of the split are asserted now, not just the winner.

The doc wording is fixed ("storage key", not "path"). My GCS note stands as a follow-up, not a blocker for this PR — worth keeping #574 open until gcs_artifact_service.ts:283 gets the same treatment, since that one is strictly worse (it strips user: before writing, so a session named user collides outright).

CI green on all three platforms after a re-run. LGTM.