Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds path observers for exact files and directory descendants. Validates observer configuration and CLI arguments. Updates settings descriptions to reference the default property. Adds a CI settings-synchronization check and related tests. ChangesPath observer shard selection
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR restores generated settings checks and expands affected-test selection. It is mergeable with owner awareness of a bounded validation issue: non-canonical observer paths with duplicate separators are still accepted and should be corrected or explicitly accepted before merge. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
WalkthroughBefore this PR, release-time CI had no dedicated guard for settings-docs/schema drift, and the affected-test-shard selector lacked explicit boundary coverage for path-prefix rules, shard-selection behavior, and settings-sync correctness. That left a gap where a release could ship with mismatched settings documentation or schema without an unconditional CI failure, and where selector edge cases around prefix boundaries and affected shards were only implicitly covered. After this PR, CI always runs a settings-sync correctness gate, the selector’s path-observer rules are explicitly enforced so settings-schema source changes select the proper scripts test shard, and new behavioral tests lock in prefix-boundary safety, affected-shard selection, and the settings-sync gate behavior. In addition, the restore command now sorts checkpoint filenames deterministically, making restore suggestions and listings stable and predictable. Release NotesNew Features
Bug Fixes
Tests
Documentation
Refactor
Chore
Changes
Magnitude🎯 2 (M) RelatedNo related items found. Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-affected-test-shards.ts`:
- Around line 580-600: Update the argument parsing for --root and --data to
reject a following recognized option token as a missing value, rather than
resolving it as the path. Preserve existing missing-value errors and defaults,
and add regression coverage for both --root --data file.json and --data --root
repo.
- Around line 398-405: Update isValidCanonicalPrefix to reject Windows
drive-qualified prefixes matching a letter, colon, and slash before the existing
canonical-prefix checks; preserve validation for other prefixes. Add a
regression test asserting that values such as “C:/” are invalid.
In `@scripts/tests/affected-test-shards.test.ts`:
- Around line 100-109: Update expectPathReasonKeywords so each keyword is
lowercased before the case-insensitive containment check, matching the existing
lowercasing of pr!.reason and preserving the helper’s contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4a43f59-f642-4e92-9b93-2352d81b1cb6
⛔ Files ignored due to path filters (1)
project-plans/issue3212/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (8)
docs/cli/configuration.mdschemas/settings.schema.jsonscripts/affected-test-shards.data.jsonscripts/affected-test-shards.tsscripts/check-affected-test-shards.tsscripts/tests/affected-test-shards-prefix-boundary.test.tsscripts/tests/affected-test-shards.test.tsscripts/tests/check-affected-test-shards.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
|
Addressed the option-as-value finding in commit 7683ede. The checker now treats recognized option tokens as missing values for both --root and --data, with subprocess coverage for both reported forms. |
|
Addressed the Windows drive-prefix finding in commit 7683ede. Canonical prefixes now reject drive-qualified forward-slash paths such as C:/, with Bun regression coverage. |
|
Addressed the filesystem race finding in commit 7683ede. Prefix validation performs one narrowly scoped stat operation and reports missing, non-directory, or raced paths as drift. Direct tests cover nonexistent and existing-file prefixes. |
|
Addressed the case-insensitive helper finding in commit 7683ede. Both the path reason and expected keyword are normalized, and the selector test now uses mixed-case keywords. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check-affected-test-shards.ts (1)
405-407: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty path segments in observer paths.
packages//cli/andpackages//cli/file.tspass these validators.join()normalizes the duplicate separator beforestatSync, so the checker accepts a non-canonical observer rule. Reject empty segments so path observers use the same canonical path form as changed-file paths.Proposed fix
export function isValidDirectoryPrefix(prefix: string): boolean { if (!prefix.endsWith('/')) return false; if (prefix.startsWith('/')) return false; if (prefix.includes('\\')) return false; if (/^[a-z]:\//i.test(prefix)) return false; - return !prefix + return !prefix.slice(0, -1) .split('/') - .some((segment) => segment === '..' || segment === '.'); + .some( + (segment) => + segment.length === 0 || segment === '..' || segment === '.', + ); } export function isValidExactPath(path: string): boolean { // ... return !path .split('/') - .some((segment) => segment === '..' || segment === '.'); + .some( + (segment) => + segment.length === 0 || segment === '..' || segment === '.', + ); }Also applies to: 428-430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-affected-test-shards.ts` around lines 405 - 407, Update the observer-path validation around the prefix segment check to reject empty path segments in addition to "." and "..". Ensure paths such as packages//cli/ and packages//cli/file.ts are rejected before join/stat normalization, while preserving acceptance of canonical relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/check-affected-test-shards.ts`:
- Around line 405-407: Update the observer-path validation around the prefix
segment check to reject empty path segments in addition to "." and "..". Ensure
paths such as packages//cli/ and packages//cli/file.ts are rejected before
join/stat normalization, while preserving acceptance of canonical relative
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a43ad9a-ac59-434c-a598-a03a97701e24
⛔ Files ignored due to path filters (1)
project-plans/issue3212/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (5)
.github/workflows/ci.ymlpackage.jsonscripts/check-affected-test-shards.tsscripts/tests/check-affected-test-shards.test.tsscripts/tests/settings-sync-gate.test.ts
|
Addressed the duplicate-separator validation gap in commit ca262c2. Exact paths and directory prefixes now reject empty interior path segments, with Bun regression tests covering both forms. |
1 participant
TLDR
Fixes the nightly release preflight failure by restoring the generated settings schema and CLI configuration documentation, then closes the PR CI gap that allowed stale settings artifacts to merge.
Settings-schema facade and modular-source changes now select the scripts test shard in addition to normal CLI coverage, ensuring the existing generated-settings synchronization test runs before merge.
Dive Deeper
The August 11 and August 12 scheduled releases failed during Run Preflight Checks, before publication. The scripts shard reported that schemas/settings.schema.json and docs/cli/configuration.md were stale relative to the canonical CLI settings schema.
The exact stale description originated in the modular settings-schema source merged by PR 3201. A later facade-level settings-schema change in PR 3207 also escaped the scripts shard, confirming that both schema source surfaces were missing from affected-test selection.
This PR:
No release bypass or settings runtime behavior was added.
Reviewer Test Plan
Run the focused behavioral checks:
Validate the checked-in selector graph:
Verify generated settings artifacts are synchronized:
Confirm selector behavior:
Run the standard lint, typecheck, format, build, and smoke gates.
Local focused tests, lint, typecheck, format, build, and the stepfun-37 smoke test pass. The full local macOS test command still reports existing PowerShell grammar and Homebrew ripgrep-path failures in unchanged packages/core tests; PR CI provides the authoritative Linux test signal.
Testing Matrix
Linked issues / bugs
Fixes #3212
Summary by CodeRabbit
Documentation
Improvements
Tests