feat: add BYO filesystem e2e test and supporting infrastructure#1461
feat: add BYO filesystem e2e test and supporting infrastructure#1461padmak30 wants to merge 2 commits into
Conversation
padmak30
commented
Jun 4, 2026
padmak30
commented
- Add strands-bedrock-byo-filesystem.test.ts: e2e test for EFS and S3 Files mounts with unique file content assertion
- Add e2e-tests/fixtures/filesystem/setup_byo_filesystem.py: one-time setup script to provision VPC, EFS, S3 Files access points in AWS
- Extend E2EConfig with apiKeyEnvVar, requiredEnvVars, efsAccessPoints, s3AccessPoints, networkConfig, invokePrompt, invokeResponseCheck
- Fix VPC warning in invoke action to suppress when --json is passed
- Add filesystem env vars to e2e-tests.yml env blocks
- Add filesystem resources fixture file
- Add strands-bedrock-byo-filesystem.test.ts: e2e test for EFS and S3 Files mounts with unique file content assertion - Add e2e-tests/fixtures/filesystem/setup_byo_filesystem.py: one-time setup script to provision VPC, EFS, S3 Files access points in AWS - Extend E2EConfig with apiKeyEnvVar, requiredEnvVars, efsAccessPoints, s3AccessPoints, networkConfig, invokePrompt, invokeResponseCheck - Fix VPC warning in invoke action to suppress when --json is passed - Add filesystem env vars to e2e-tests.yml env blocks - Add filesystem resources fixture file
github-actions
Bot
added
the
agentcore-harness-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
Package TarballHow to installgh release download pr-1461-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.17.0.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-cli-automation
left a comment
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Nice work setting up filesystem e2e coverage end-to-end. A few issues to address before merging:
- The S3 Files IAM trust policy looks wrong — it allows
elasticfilesystem.amazonaws.comto assume the role, which is the EFS service principal, not S3 Files. This will likely make the S3 Files mount unable to read the bucket. - The new
invokeResponseCheckis silently skipped whenjson.responseis falsy, which weakens the assertion. The whole point of this check is to fail when the response is wrong (or missing). - The test prompt only exercises the EFS mount; the S3 Files mount is configured but never read or written from at runtime, so a misconfigured S3 mount would still pass.
Inline comments below.
| "Version": "2012-10-17", | ||
| "Statement": [{ | ||
| "Effect": "Allow", | ||
| "Principal": {"Service": "elasticfilesystem.amazonaws.com"}, |
There was a problem hiding this comment.
Wrong service principal. This is the trust policy for the S3 Files role (the role passed as roleArn to s3files:CreateFileSystem), but the principal here is elasticfilesystem.amazonaws.com — that's the EFS service. With this trust policy, S3 Files won't be able to assume the role and the mount will fail to access the bucket at runtime.
This should be the S3 Files service principal (most likely s3files.amazonaws.com — please verify against the S3 Files docs / what the CDK construct expects).
Also, since this is for S3 Files, consider scoping the aws:SourceAccount condition to the S3 Files file system ARN via aws:SourceArn once known, but at minimum the principal needs to change.
| const json = parseJsonOutput(result.stdout) as { success: boolean }; | ||
| const json = parseJsonOutput(result.stdout) as { success: boolean; response?: string }; | ||
| expect(json.success, 'Invoke should report success').toBe(true); | ||
| if (cfg.invokeResponseCheck && json.response) { |
There was a problem hiding this comment.
This silently skips the configured assertion when json.response is empty/undefined, which defeats the purpose of invokeResponseCheck — a response that's missing the expected token (or missing entirely) would still let the test pass.
A couple of options:
- Drop the
&& json.responseguard and let the check run unconditionally — the assertion inside (expect(...).toContain(...)) will fail loudly onundefined/empty, which is what we want. - Add an explicit
expect(json.response, 'Invoke should return a response').toBeTruthy()before invoking the check.
Either way, an e2e test that's specifically asserting on response content shouldn't quietly become a no-op.
| }, | ||
| efsAccessPoints: [{ accessPointArn: process.env.E2E_EFS_ACCESS_POINT_ARN!, mountPath: '/mnt/efs' }], | ||
| s3AccessPoints: [{ accessPointArn: process.env.E2E_S3_ACCESS_POINT_ARN!, mountPath: '/mnt/s3' }], | ||
| invokePrompt: `Write the text "${uniqueFileContent}" to /mnt/efs/test.txt using file_write, then read it back using file_read and return the contents.`, |
There was a problem hiding this comment.
The prompt only writes to and reads back from /mnt/efs/test.txt, so the S3 Files mount (/mnt/s3) is provisioned and configured on the runtime but is never actually exercised at runtime. A broken S3 Files mount target / access point / IAM policy would still let this test pass.
Consider either:
- Extending the prompt to also write+read a unique token under
/mnt/s3/(e.g. a second UUID, asserted on as well), so both mounts are exercised end-to-end. - Splitting into two tests (one EFS, one S3) so failures localize cleanly.
Given the cost of standing up the VPC/EFS/S3 infra in CI, exercising both in a single invocation seems like the better tradeoff.
There was a problem hiding this comment.
updated to also test s3 mounts
github-actions
Bot
removed
the
agentcore-harness-reviewing
Coverage Report
|
- Fix invokeResponseCheck to assert response is truthy before running the check — previously silently skipped on empty/undefined response - Extend filesystem test prompt to write and read unique tokens on both EFS (/mnt/efs) and S3 Files (/mnt/s3) mounts, asserting both appear in the response so a misconfigured S3 mount is caught
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
2 participants
