fix(lint): resolve BES files via path_prefix; skip unreadable outputs gracefully#1135
Draft
gregmagolan wants to merge 2 commits into
Draft
fix(lint): resolve BES files via path_prefix; skip unreadable outputs gracefully#1135gregmagolan wants to merge 2 commits into
gregmagolan wants to merge 2 commits into
Conversation
✨ Aspect Workflows Tasks📅 Mon May 25 19:10:55 UTC 2026
|
![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: 4fc8bcb6e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| """ | ||
| if file.path_prefix: | ||
| rel = "/".join(list(file.path_prefix) + [file.name]) | ||
| local = ctx.std.env.current_dir() + "/" + rel |
There was a problem hiding this comment.
resolve_bes_file_path builds local from current_dir, but BES file.path_prefix is rooted at the Bazel workspace/output tree, not the caller’s working directory. When aspect lint is run from a subdirectory (or when Aspect root differs from Bazel root), this computes a non-existent path, so .report/.patch/.exit_code files are treated as unreadable and skipped; that can drop diagnostics/patches and leave linter_exit_code at 0 even when a linter failed. Use ctx.std.env.bazel_root_dir() (or equivalent workspace root) for this join.
Useful? React with 👍 / 👎.
The BES `named_set_of_files` stream emits `bytestream://` URIs for build outputs. `file_uri_to_path` resolves these to the bb_clientd FUSE mount under `_BB_CLIENTD_ROOT`, which only exists on Aspect Workflows runners. On any other CI host (e.g. GitHub Actions in bazel-examples) the resolved path doesn't exist locally, so the first `ctx.std.fs.read_to_string(filepath)` inside the drain loop crashes the entire `lint` task with `os error 2`. Narrow the loop to the four rules_lint extensions before any read, then check `fs.exists(filepath)`; on miss, increment a counter and continue. After the drain, emit one `warn()` summarising the skip count so users know results may be incomplete. The lint aspect's tool name still lands in `lint_tools_run` (extracted from `file.name`, not from the unreadable file content) so the zero-aspect safeguard isn't falsely tripped. Repros: https://github.com/aspect-build/bazel-examples/actions/runs/26376140872/job/77636608034 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The BES `File` proto carries the workspace-relative materialization path in `path_prefix + [name]` alongside the canonical content URI in `file`. Bazel writes downloaded outputs to that workspace path on every host (and `--remote_download_regex='.*AspectRulesLint.*'` ensures lint outputs are downloaded), so it's the right local path regardless of whether bb_clientd is mounted. Add `resolve_bes_file_path(ctx, file)` that tries the workspace-relative path first and falls back to the existing bytestream → bb_clientd resolution. Only when neither exists do we hit the skip-and-warn safety net added in the previous commit. This restores lint findings on non-Workflows CI hosts (GHA, GitLab, etc.) where BES emits `bytestream://` for downloaded outputs and bb_clientd doesn't exist — previously we'd skip those, now we read them from `bazel-out/<config>/bin/<pkg>/<target>.AspectRulesLint<Tool>.<ext>` directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gregmagolan
force-pushed
the
fix-lint-skip-unreadable-bes-outputs
branch
from
4fc8bcb to
8e7ce50
Compare
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
1 participant
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.
Summary
aspect lintcrashed withNo such file or directory (os error 2)partway through the BES drain on non-Aspect-Workflows CI hosts (example: bazel-examples GHA run). Two issues in the existing BES file handling:file_uri_to_pathwhich only understands the canonicalbytestream://URI form and resolves it to a bb_clientd FUSE path (/mnt/ephemeral/buildbarn/bb_clientd/...) that only exists on Workflows runners.read_to_stringhad no guard against a missing path, so a single miss tanked the whole task mid-drain.This PR teaches the lint task that the BES
Fileproto carries the workspace-relative materialization path inpath_prefix + [name]alongside the canonical content URI. Bazel writes downloaded outputs to that workspace path on every host, and the lint task's own--remote_download_regex='.*AspectRulesLint.*'flag guarantees lint outputs are downloaded — so on GHA / GitLab / CircleCI / local dev that path resolves correctly tobazel-out/<config>/bin/<pkg>/<target>.AspectRulesLint<Tool>.<ext>.New
resolve_bes_file_path(ctx, file)tries thepath_prefixmaterialization first, falls back to the existing bytestream → bb_clientd resolution for Workflows runners where outputs may stay remote-only, and returns""when neither resolves. Callers skip empty results, increment anunreadable_outputscounter, and the post-drain code emits one summarywarn()so the user knows results may be incomplete instead of a crash mid-stream.The BES loop is also narrowed to the four rules_lint extensions (
.report,.patch,.exit_code,.out) before any read, so non-lint outputs innamed_set_of_filesdon't inflate the unreadable counter or cost a needlessfs.existscall.Changes are visible to end-users: yes
Suggested release notes:
aspect lintcrashing withNo such file or directory (os error 2)on non-Aspect-Workflows CI hosts (GHA, GitLab, CircleCI). The lint task now resolves BES file outputs via the workspace-relative materialization path (bazel-out/...) before falling back to bb_clientd, and gracefully skips + summarises any output that still can't be read locally.Test plan
test-lint-template-snapshots,test-lint-annotation-plan,test-lint-linter-rows, andtest-lint-detect-toolall still pass; the resolution helper is pure data flow into the existing readers and doesn't change the data shape.cd bazel-examples && aspect lint --aspect=//tools/lint:linters.bzl%shellcheck -- //...) — the run still exits cleanly with the zero-aspect ERROR line; the new resolver doesn't regress the no-output path.aspect lintno longer crashes; lint outputs are read frombazel-out/...(Bazel downloads them per--remote_download_regex), and any output that still can't be resolved is counted into a single trailingWARNING: lint: N BES output(s) were unavailable locally and skipped; lint results may be incomplete.line.