Conversation
fd7ba73 to
a13bedb
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a13bedb. Configure here.
| if (operationFailed) { | ||
| throw operationError; | ||
| } | ||
| return result as T; |
There was a problem hiding this comment.
Cleanup errors delete successful products
High Severity
Source-based tests nest withManagedTestProductsReader inside withManagedTestProductsOutput. After the test already finished, the reader still takes the shared lifecycle lock and prunes. A lock timeout or prune error then bubbles out as an operation failure, so isSuccessful is skipped and withManagedTestProductsOutput deletes the bundle it just built. Callers lose a completed test result and its multi-gigabyte products.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit a13bedb. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const preparedSourcePath = params.testProductsPath ?? params.xctestrunPath; | ||
| return preparedSourcePath | ||
| ? withManagedTestProductsReader(resolvePathFromCwd(preparedSourcePath), execute) | ||
| : execute(); |
There was a problem hiding this comment.
Bug: The resolveFromLayers calls for testProductsMaxCount and testProductsMaxAgeDays are missing the fileConfig parameter, preventing configuration from project files.
Severity: MEDIUM
Suggested Fix
Add the fileConfig: opts.fileConfig parameter to the resolveFromLayers calls for both testProductsMaxCount and testProductsMaxAgeDays in src/utils/config-store.ts to allow configuration from project files.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/utils/test-common.ts#L228-L231
Potential issue: The configuration values for `testProductsMaxCount` and
`testProductsMaxAgeDays` are resolved using the `resolveFromLayers` function but are
missing the `fileConfig: opts.fileConfig` parameter. This is inconsistent with how other
configuration values are handled. As a result, users cannot configure these test product
retention settings through project configuration files like `.xcodebuildmcp.json`. The
settings can only be configured through environment variables or direct code overrides,
which breaks the intended configuration mechanism for this new feature.
Did we get this right? 👍 / 👎 to inform future reviews.
This branch has not been deployed
1 participant
What changed
.xctestproductsretention configurable throughXCODEBUILDMCP_TEST_PRODUCTS_MAX_COUNTandXCODEBUILDMCP_TEST_PRODUCTS_MAX_AGE_DAYSWhy
Source-based test runs stage full
.xctestproductsbundles in workspace storage. Multi-gigabyte products made the previous 100-bundle default capable of retaining hundreds of gigabytes, while concurrent readers and writers require cleanup to remain process-safe.Validation
npm run typechecknpm run lint(0 errors; existing warnings remain)npm run format:checknpm run buildnpm test: 2,611 passed; one unchanged environment-sensitiveinit --dest ~/...test failed because this machine already has the CLI skill installed in the real home directorygit diff --checkSnapshot and smoke suites were not run because repository policy requires explicit permission.
Fixes #524.
AI assistance: Heavy AI use. Human skimmed, but I don't have deep experience with TS.