Skip to content

Test: pin filename edge cases in the shared artifact-service conformance suite - #765

Open
AmaadMartin wants to merge 1 commit into
mainfrom
feat/artifact-service-filename-edge-cases
Open

Test: pin filename edge cases in the shared artifact-service conformance suite#765
AmaadMartin wants to merge 1 commit into
mainfrom
feat/artifact-service-filename-edge-cases

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: runArtifactServiceTests() is the conformance suite that every BaseArtifactService backend must satisfy, but it only exercises plain filenames. The three backends store a filename very differently: InMemoryArtifactService escapes it into a Record key, GcsArtifactService makes it a segment of an object name, and FileArtifactService maps it onto a real directory. They can therefore diverge on a legal-but-unusual name and no test notices.

Solution: I added one filename edge cases block to the shared suite. It pins a save -> load round trip and the exact listArtifactKeys string for four names, plus an exact-set assertion that the four stay distinct in one session. This is a test-only change; no file under core/src/ is touched.

The four names and why each was chosen:

Filename What it pins
.hidden.txt a leading dot, one character away from the .. prefix the file backend's containment guard rejects
my report.txt an interior space, which must survive encodeURIComponent in memory and become a real directory name on disk
trailing.dot. a trailing dot, legal on POSIX and in GCS; Win32 strips it from on-disk names
nested/dir/report.txt path separators, which become three directories on disk, three object-name segments in GCS, and %2F in memory

Two cases are deliberately excluded because the backends genuinely disagree on them today, so a shared test cannot pin them:

  • ..-prefixed names such as ..hidden.txt. FileArtifactService.getArtifactDir() rejects them while the other two accept them. That over-rejection is a source fix, tracked separately.
  • Leading or trailing whitespace. cleanFilename.trim() collapses 'a.txt', ' a.txt' and 'a.txt ' onto one artifact directory, while the other two backends keep them apart. That needs a product decision, not a test.

The set assertion sorts both sides ([...keys].sort()). GcsArtifactService sorts with localeCompare and the other two use the default sort, so collation is not part of the contract, but the exact set is.

Collision check: I listed every open PR on the fork and diffed the artifact-adjacent ones (#760, #658, #661, #520). None touches core/test/artifacts/artifact_service_test_utils.ts, so there is no overlap and this branch is based on main.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

npx vitest run --project unit:core core/test/artifacts
   Test Files  5 passed (5)
        Tests  128 passed (128)

The new block runs three times, once per backend: 15 new test cases in total.

Proof the tests can fail. There is no source fix to revert here, so I broke each backend in turn, confirmed the new block went red, then reverted. Every backend is caught by at least one case.

Backend Mutation Failure
InMemoryArtifactService drop decodeURIComponent in listArtifactKeys expected [ 'my%20report.txt' ] to include 'my report.txt' (4 failed)
GcsArtifactService getFileNameFromPath returns parts[0] expected [ 'nested' ] to include 'nested/dir/report.txt' (3 failed)
FileArtifactService containment guard tests relative.startsWith('.') Error: Artifact filename .hidden.txt escapes storage directory. (2 failed)

The FileArtifactService mutation is the sharpest signal: it fails only the new block, and every pre-existing test in that file still passes.

Other gates, run on the pushed commit:

npm run build          # ok
npm run lint           # clean
npm run format:check   # All matched files use Prettier code style!

npm run ts:check reports 287 errors in 42 files both with and without this change. Those errors are pre-existing and unrelated.

CI. The three artifact suites are green on all three runner platforms. Every leg of every attempt reports core/test/artifacts/file_artifact_service_test.ts (47 tests), gcs_artifact_service_test.ts (38 tests) and in_memory_artifact_service_test.ts (32 tests) passing. The trailing-dot case therefore passes on Windows, which is where it had platform exposure.

The run-tests matrix is nonetheless red, on a test this change does not touch. tests/integration/app_loader/app_loader_test.ts:82 times out at its 40000ms budget. It is the only failing test in every red run. I re-ran the matrix twice: it failed on macOS, then on macOS again, then on Windows while macOS passed. ubuntu-latest passed every time.

This flake is pre-existing. The identical test fails the same way on upstream main with no PR changes, in the macOS leg of validation run 31061656732 (commit 13f9995c). Its beforeAll runs npm install in a fixture and the test body then compiles TypeScript through AgentLoader.listApps(), which is much slower on the Windows and macOS runners. I did not change it, because it is unrelated breakage. It is filed as separate work.

Manual End-to-End (E2E) Tests:
Not applicable. This change adds no runtime behaviour a user can exercise. To see the new cases, run npx vitest run --project unit:core core/test/artifacts --reporter=verbose and look for filename edge cases under each of the three backends.

Cross-platform note: CI runs this suite on Linux, Windows and macOS. The trailing-dot case is the one with platform exposure, because Win32 strips a trailing dot from an on-disk path component. The round trip still holds, since the write and the read share the same normalization and listArtifactKeys returns the original metadata.fileName. The four base names stay distinct after stripping, so no Windows collision is possible between them.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

…ce suite

The shared conformance suite only exercised plain filenames, so the three
BaseArtifactService backends could diverge on legal-but-unusual names
without any test noticing. Add a 'filename edge cases' block covering a
leading dot, an interior space, a trailing dot and a nested path.

Test-only change. No file under core/src is touched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant