Fix/orion bugs fix#2051
Merged
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 261fec5488
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 347c384f9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d39abfa1eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0e37727ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2801f8eca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 945e1aca4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+572
to
+576
| "deleted" => { | ||
| tracing::warn!( | ||
| job_id = job_id, | ||
| repo = repo, | ||
| cl_link = cl_link, |
There was a problem hiding this comment.
populate_cl_overlay_dir now handles cl builds by downloading changed files, but the "deleted" branch only logs and continues. In this direct-mount path, not writing a whiteout means deleted files remain visible from the lower Dicfuse layer, so CLs that remove source/BUCK files are built against stale filesystem state and can yield incorrect target impact/build results.
Useful? React with 👍 / 👎.
Comment on lines
+89
to
+93
| let secure = server_addr.starts_with("wss://"); | ||
| let preferred = if secure { "HTTPS_PROXY" } else { "HTTP_PROXY" }; | ||
| std::env::var(preferred) | ||
| .ok() | ||
| .filter(|value| !value.trim().is_empty()) |
There was a problem hiding this comment.
proxy_url_for always returns HTTP(S)_PROXY/ALL_PROXY when set, but it never checks NO_PROXY. In common deployments with global proxy env vars and internal Orion endpoints listed in NO_PROXY, this forces websocket traffic through the proxy and can cause repeated handshake failures/reconnect loops where direct connection should have been used.
Useful? React with 👍 / 👎.
benjamin-747
approved these changes
Apr 22, 2026
Add test cases to verify that configuration files in the root directory (BUCK, Cargo.toml, .buckconfig) are correctly resolved when root cell is configured as ".". Tests added: - cells.rs: test_unresolve_root_cell_with_dot_prefix * Verifies BUCK, Cargo.toml, .buckconfig resolution * Includes regression test for src/main.rs (CL OB6W18SK) * Tests empty path edge case - cells.rs: test_unresolve_multi_cell_priority * Verifies correct cell matching priority in multi-cell setup * Tests that more specific paths match first - changes.rs: test_changes_with_root_directory_files * Integration test for Changes::new() with root directory files * Verifies all paths are resolved correctly - changes.rs: test_changes_preserves_status * Verifies that file status (Modified/Added/Removed) is preserved - changes.rs: test_regression_cl_ob6w18sk * Ensures CL OB6W18SK scenario continues to work All tests pass, indicating current implementation handles these cases. This establishes baseline before investigating production environment differences. Related to: CL 2NY0WW96, CL 9TNTRBBQ, CL OB6W18SK Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
This commit enhances the path matching logic to explicitly handle root cells (prefix = "" or ".") with proper semantics. Problem: In production environments, configuration files in the root directory (BUCK, Cargo.toml) were not triggering incremental builds when modified. Analysis of production logs showed: - CL 2NY0WW96 (BUCK modified): 0 targets discovered - CL 9TNTRBBQ (Cargo.toml modified): 0 targets discovered - CL OB6W18SK (src/main.rs modified): 1 target discovered Root Cause: The original implementation relied on strip_prefix() which works when parse_cells_data() converts "." to "", but the logic was implicit and fragile. The semantics of root cell matching were not clearly expressed. Solution: Explicitly handle root cell (empty or "." prefix) with clear semantics: - Root cell matches "all paths without a more specific prefix" - Check for more specific matches first (longest prefix priority) - Only assign to root cell if no better match exists - Non-root cells use standard prefix matching Benefits: 1. Explicit semantics: Code clearly expresses the intent 2. Robust: Works regardless of how parse_cells_data() normalizes paths 3. Maintainable: Future developers understand the root cell logic 4. Correct priority: Ensures longest-prefix-first matching The fix ensures all root directory files (BUCK, Cargo.toml, .buckconfig) are correctly resolved to the root cell, enabling proper incremental build detection. Related: CL 2NY0WW96, CL 9TNTRBBQ, CL OB6W18SK Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Problem:
When BUCK files are modified, the changes contain paths like "root//BUCK",
but contains_package("root//") only checked for the exact package path,
causing BUCK file changes to be missed.
Solution:
Modified contains_package to also check if any BUCK or BUCK.v2 files
within the package directory are in the changes. This ensures that
build file modifications are properly detected as package changes.
Tests:
- test_contains_package_with_buck_file: Verifies BUCK detection
- test_contains_package_with_buck_v2_file: Verifies BUCK.v2 detection
- test_contains_package_with_subdirectory_buck_file: Ensures subdirectory isolation
- test_contains_package_without_buck_file: Ensures non-BUCK files don't trigger
All 53 tests pass.
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
These tests simulate the ACTUAL production flow that was failing: Git change → ProjectRelativePath → unresolve() → CellPath → contains_package() The previous tests used Changes::from_paths() which bypassed the unresolve() step, so they passed even though production was failing. Test cases added: - test_real_world_buck_file_change_with_unresolve: Simulates CL 2NY0WW96 - test_real_world_cargo_toml_change_with_unresolve: Simulates CL 9TNTRBBQ - test_real_world_subdirectory_file_with_unresolve: Simulates CL OB6W18SK These tests verify the complete fix: 1. unresolve() correctly handles root cell files (BUCK, Cargo.toml) 2. Changes::new() successfully resolves paths (not skipped with warnings) 3. contains_package() correctly detects BUCK file changes Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Added tests for additional scenarios mentioned in orion.md: - .buckconfig file changes (root config file) - BUCK.v2 variant support - Multi-cell environment with priority matching - Mixed changes (multiple files in one commit) All tests simulate the complete production flow: Git change → ProjectRelativePath → unresolve() → CellPath → contains_package() Test coverage now includes: ✅ Root directory files (BUCK, Cargo.toml, .buckconfig) ✅ BUCK file variants (BUCK, BUCK.v2) ✅ Subdirectory files (src/main.rs) ✅ Multi-cell priority (toolchains cell vs root cell) ✅ Mixed changes (multiple files at once) All 60 tests passing. Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Apply rustfmt to ensure consistent code style across the codebase. Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
The contains_package method now correctly handles both package formats:
- "cell//dir/" (with trailing slash)
- "cell//dir" (without trailing slash, the real-world format)
When strip_prefix("cell//dir") is applied to "cell//dir/BUCK", it yields
"/BUCK" (with leading slash). The fix strips this leading slash before
checking if the relative path is a BUCK file.
This ensures BUCK file changes in non-root packages correctly trigger
package-level impact detection in immediate_target_changes.
Added test: test_contains_package_without_trailing_slash
Signed-off-by: jackie <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
When cell paths in JSON have trailing slashes (e.g., "toolchains": "./toolchains/"), the unresolve() method would fail to match paths correctly because it would test against "toolchains//" instead of "toolchains/". This fix normalizes prefix strings by trimming trailing slashes before matching, ensuring consistent path resolution regardless of whether the cell configuration includes trailing slashes. Test: test_unresolve_cell_with_trailing_slash Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
The contains_package() method was incorrectly matching files inside BUCK.* directories (e.g., BUCK.gen/subdir/file.rs) as BUCK file changes. This caused non-build-file changes to be misclassified as package changes, triggering incorrect recursive impact propagation. This fix adds a check to ensure the relative path contains no '/' character, restricting matches to actual BUCK files (BUCK, BUCK.v2, BUCK.*) in the package root directory only. Test: test_contains_package_rejects_buck_directory Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Extend test_real_world_cargo_toml_change_with_unresolve to verify that Cargo.toml changes are detected as package changes by contains_package(). This test currently fails because contains_package() only checks for BUCK files, not Cargo.toml. The test verifies the complete flow: 1. Git detects Cargo.toml change (ProjectRelativePath) 2. unresolve() converts to CellPath (root//Cargo.toml) ✅ 3. contains_package() should detect it as package change ❌ FAILS Related: CL GIVWFTLA (Cargo.toml changes not triggering builds) Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Extend contains_package() to recognize Cargo.toml as a package-level
configuration file, similar to BUCK files.
Root Cause:
The previous fix only added BUCK file detection to contains_package(),
but didn't consider other package-level manifest files like Cargo.toml.
Why This Matters:
Cargo.toml defines dependencies and build configuration for Rust
projects. Changes to it should trigger rebuilds just like BUCK file
changes do.
Implementation:
- Add Cargo.toml check alongside BUCK file checks
- Use same pattern: check if file is in package root (no '/' in relative path)
- Add trace logging for debugging
Flow After Fix:
1. unresolve("Cargo.toml") → root//Cargo.toml ✅
2. contains_package("root//") → detects root//Cargo.toml ✅
3. Returns RootImpactKind::Package ✅
4. Marks targets as impacted ✅
5. Triggers incremental build ✅
Fixes: CL GIVWFTLA (Cargo.toml changes not triggering builds)
Maintains: CL BUYRPBLK (BUCK changes continue to work)
Signed-off-by: jackie <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
…l file detection Replace the hardcoded list of package-level files (BUCK, Cargo.toml) with a comprehensive, pattern-based approach that supports all common manifest and build files. Root Cause: The previous implementation hardcoded specific file names, requiring code changes for every new file type. This is not scalable or maintainable. Solution: Introduce is_package_level_file() function that recognizes: - Build system files: BUCK, BUILD, CMakeLists.txt, Makefile - Rust manifests: Cargo.toml, Cargo.lock - JavaScript/Node: package.json, package-lock.json, yarn.lock, pnpm-lock.yaml - Python: requirements.txt, setup.py, pyproject.toml, Pipfile - Go: go.mod, go.sum - Java/Maven/Gradle: pom.xml, build.gradle, settings.gradle - Configuration: .buckconfig, .bazelrc, .buckversion Benefits: - One-time fix for all manifest file types - No code changes needed for new file types - Centralized, maintainable logic - Comprehensive test coverage (36 tests passing) Fixes: CL 8OU2UOEX (Cargo.lock now supported) Fixes: Future manifest files automatically supported Maintains: All existing functionality (BUCK, Cargo.toml, etc.) Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Enhance the implementation with: 1. Comprehensive function documentation listing all supported file types 2. Additional unit tests for CMakeLists.txt and Makefile 3. Integration tests for CMake, Makefile, and .buckconfig files This ensures all supported file types are properly documented and tested, making the codebase more maintainable and easier to understand. Test coverage: 40 tests passing (4 new tests added) Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Add support for .mega_cedar.json as a package-level configuration file that triggers full package rebuilds when modified. Changes: - Add .mega_cedar.json detection in is_package_level_file() - Update function documentation to include Mega configuration - Add unit test for .mega_cedar.json detection - Add integration test for contains_package() behavior This ensures that changes to .mega_cedar.json properly trigger incremental compilation for affected packages. Signed-off-by: jackie <jackieismpc@gmail.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Problem:
When modifying files in non-root cells (e.g., toolchains/BUCK), Orion
reported 'No impacted Buck targets detected'. This was because buck2
targets command only queried the root cell ('//...'), missing targets
in other cells like toolchains, prelude, etc.
Root Cause:
The targets_arguments() function hardcoded '//...' pattern, which only
queries the root cell. When toolchains/BUCK was modified:
1. Path was correctly resolved to 'toolchains//BUCK' ✓
2. changes.contains_package(&toolchains_package) returned true ✓
3. But diff.targets() had NO toolchains cell targets ✗
Solution:
1. Add CellInfo::get_all_cell_patterns() to generate patterns for all
cells (e.g., ['root//...', 'toolchains//...', 'prelude//...'])
2. Modify get_repo_targets() to accept optional CellInfo parameter
3. When CellInfo is provided, query all cells; otherwise default to
root cell only (for backward compatibility with tests)
4. Remove hardcoded '//...' from targets_arguments()
Impact:
- Fixes toolchains/BUCK not triggering incremental builds
- Fixes any other non-root cell file changes not being detected
- Maintains backward compatibility for tests without cell info
Test: Added test_get_all_cell_patterns() and test_contains_package_toolchains_cell()
Related: CL HM7ZXVEV (toolchains/BUCK), CL M0YGTOEO (.mega_cedar.json)
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
…errors The prelude cell contains build rule definitions and may have parsing issues when queried directly (e.g., "Object of type 'struct' has no attribute 'go_unittest'"). Since prelude is a special cell that doesn't contain user targets, we exclude it from the target query patterns. This allows us to successfully query targets from all user-defined cells (root, toolchains, etc.) without hitting prelude parsing errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
When buck2 targets command fails, stderr was not captured. More critically, stderr was not consumed even on success, which could cause pipe deadlock if buck2 writes more than 64KB to stderr. This change: - Spawns background thread to consume stderr, preventing pipe deadlock - Logs stderr on failure for diagnostics - Logs stderr on success (debug level) if non-empty Fixes issues like: - Cell configuration errors when CL modifies .buckconfig or toolchains/BUCK - FUSE mount issues - Buck2 daemon startup failures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Problem: After commit fa81285 (query targets from all cells), buck2 targets command started failing with: Error resolving recursive spec `none///...` This happened because the buck2_test project uses a "none" placeholder cell for cell aliases (fbcode, fbsource, etc.), and buck2 tried to query targets from this non-existent directory. Root Cause: The get_all_cell_patterns() method included ALL cells from the configuration, including placeholder cells like "none" that don't contain actual build targets. When buck2 tried to query "none//...", it failed because the directory doesn't exist or is just a placeholder. Solution: Extend the filter in get_all_cell_patterns() to exclude both: - "prelude": Special cell with build rule definitions (already excluded) - "none": Placeholder cell used for cell aliases (newly excluded) This is a targeted fix that uses a simple hardcoded filter, which is appropriate given: 1. Only 2 known special cells need exclusion 2. These are well-known Buck2 conventions 3. Simple and maintainable Impact: - Fixes all CL builds that were failing with "none//..." error - Allows Cargo.lock, BUCK, and other file modifications to work again - No impact on normal cells (root, toolchains, etc.) Test: Added test_get_all_cell_patterns_excludes_none_placeholder() Related CLs: FTB5APYJ, RTXQJQS8, OPJ2PRVU (all were failing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
- Changed /Users/jackie/work/project/buck2_test to /repo in all test cases - Ensures tests are portable across different development environments - Follows best practice of using generic paths in unit tests Signed-off-by: jackieismpc <jackieismpc@gmail.com>
…tory errors The toolchains cell is defined in .buckconfig but the directory doesn't exist in the main project (only in test fixtures). When querying targets from all cells, buck2 tries to access the non-existent toolchains directory and fails with 'No such file or directory' error. This fix excludes the toolchains cell from target queries, similar to how we exclude prelude and none cells. Test fixtures with their own .buckconfig are not affected. Fixes the error: Error resolving recursive spec `toolchains///...` Caused by: read_dir(/home/jackie/mega/toolchains) No such file or directory (os error 2) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
When buck2 cells are defined in .buckconfig but their directories don't exist, querying them causes "No such file or directory" errors. This happens in monorepo setups where root .buckconfig defines cells that only exist in subprojects. Changes: - Modified get_all_cell_patterns() to accept repo_root parameter - Added directory existence check before including cells in queries - Updated buck_controller.rs to pass repo_path to get_all_cell_patterns() - Added test coverage for directory existence validation This fixes the issue where modifying project/buck2_test/toolchains/BUCK failed because root .buckconfig defined "toolchains = toolchains" but the directory didn't exist at root level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Buck2 has two layers of caching that can prevent syntax errors from being detected: 1. Remote cache (already disabled via --no-remote-cache) 2. Local daemon cache (still active, causing issues) This commit implements a comprehensive cache prevention strategy: - Kill buck2 daemon before each build to clear local action cache - Use unique isolation-dir per build to prevent cache sharing - Keep --no-remote-cache to prevent remote cache usage This ensures that syntax errors are always detected immediately in incremental builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Add cleanup logic after build completes to remove .buck-isolation directories, preventing disk space accumulation from multiple builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Prevent buck2 isolation directories from being tracked in git. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Buck2 requires --isolation-dir to be a simple directory name without path
separators. Changed from '.buck-isolation/{id}' to 'buck-isolation-{id}' to
comply with this requirement.
This fixes the issue where buck2 commands would fail with invalid isolation-dir
format when the parameter contained '/' characters.
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
auto-merge was automatically disabled
April 22, 2026 01:20
Head branch was pushed to by a user without write access
jackieismpc
force-pushed
the
fix/Orion-bugs-fix
branch
from
945e1ac to
c4c7b4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4c7b4f7cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.