-
Notifications
You must be signed in to change notification settings - Fork 1
Check cross-template dependencies using liquid test YAML files #235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…specific liquid test files (RT templates only for now)
WalkthroughThe changes add two new utility functions to fsUtils.js for analyzing liquid test dependencies: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/utils/fsUtils.js (2)
418-476: Consider renaming to camelCase for consistency.The function
check_liquid_test_dependenciesuses snake_case naming, which is inconsistent with the rest of the codebase where camelCase is used (e.g.,findTemplatesWithLiquidTests,getAllTemplatesOfAType,listSharedPartsUsedInTemplate).Apply this diff to rename the function:
-function check_liquid_test_dependencies(target_handle) { +function checkLiquidTestDependencies(targetHandle) { const dependentHandles = []; const allHandlesWithTests = findTemplatesWithLiquidTests(); // Recursively check if target_handle appears in the data subtree const containsHandle = (obj, handle) => {And update the export and parameter name:
- check_liquid_test_dependencies, + checkLiquidTestDependencies,
453-453: Add documentation explaining the increased maxAliasCount setting.The
maxAliasCount: 10000is intentionally set higher than the default (100) to support parsing of legitimate liquid test YAML files—this was increased from 1000 in a prior fix when real test files exceeded that limit. While the security concern regarding YAML bombs is valid, the risk is mitigated here since these files are internal tests, not untrusted input.Consider adding an inline comment explaining this trade-off (e.g.,
// maxAliasCount increased to 10000 to support liquid test files with many reusable YAML anchors).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/utils/fsUtils.js(3 hunks)tests/lib/utils/checkLiquidTestDependencies.test.js(1 hunks)tests/lib/utils/findTemplatesWithLiquidTests.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/lib/utils/findTemplatesWithLiquidTests.test.js (1)
lib/utils/fsUtils.js (6)
fs(1-1)require(4-4)path(2-2)templateDir(373-373)handle(239-239)handle(267-267)
tests/lib/utils/checkLiquidTestDependencies.test.js (1)
lib/utils/fsUtils.js (4)
fs(1-1)require(4-4)path(2-2)templateDir(373-373)
🔇 Additional comments (3)
tests/lib/utils/findTemplatesWithLiquidTests.test.js (1)
1-196: LGTM! Comprehensive test coverage.The test suite thoroughly validates the
findTemplatesWithLiquidTestsfunction across multiple scenarios including empty directories, variant file exclusion, directory filtering, and edge cases. The setup and teardown logic properly isolates each test using temporary directories.lib/utils/fsUtils.js (1)
359-410: LGTM! Well-implemented template discovery.The
findTemplatesWithLiquidTestsfunction correctly identifies templates with liquid test files, properly excludes variant suffixes (e.g.,_TY21,_AB123), and handles edge cases like non-directory entries. The regex patterns are appropriate for the use case.tests/lib/utils/checkLiquidTestDependencies.test.js (1)
1-403: LGTM! Excellent test coverage for dependency checking.The test suite comprehensively validates
check_liquid_test_dependenciesacross numerous scenarios including:
- Detection of handle references in data as both keys and values
- Proper scoping to only the data subtree (excluding context and expectation)
- Handling of nested structures and arrays
- Graceful error handling for invalid YAML
- Multiple test cases and uniqueness validation
The tests provide strong confidence in the implementation's correctness.
Note: If the function is renamed to
checkLiquidTestDependencies(as suggested in the implementation review), remember to update the test imports and calls accordingly.
| @@ -1,5 +1,6 @@ | |||
| const fs = require("fs"); | |||
| const path = require("path"); | |||
| const yaml = require("yaml"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the yaml package to version 2.8.2 or later to address known vulnerabilities.
The yaml library version 2.2.1 is outdated and contains known security vulnerabilities. Upgrade to version 2.8.2 or later, as 2.8.2 is the latest non-vulnerable version.
🤖 Prompt for AI Agents
In lib/utils/fsUtils.js at line 3 the project imports the outdated yaml package
(currently 2.2.1); update the dependency in package.json to "yaml": ">=2.8.2",
run npm install (or yarn install) to refresh node_modules and the lockfile
(package-lock.json or yarn.lock), run the test suite and lint to ensure no
breaking API changes, and commit the updated package.json and lockfile so the
project uses yaml v2.8.2 or later to address the security vulnerability.
Description
Check cross-template dependencies using liquid test YAML files.
Currently only implemented for RTs.
Testing Instructions
Steps:
reconciliation_textsfolder into the repo for testing purposescheck_liquid_test_dependenciesin the terminal (Cursor can assist with this)Author Checklist
Reviewer Checklist