fix(scripts): fail closed on relative invocations the exec-bit guard cannot resolve - #4058
Conversation
…cannot resolve A step's working-directory, or a cd earlier in its run block, changes what a relative path names: `./foo.sh` run from `scripts` execs the tracked scripts/foo.sh. That text matched no path the guard models, so it produced no occurrence, and with another invocation satisfying anti-vacuity the guard reported success over a script the runner genuinely execs. A directly execed `./x.sh` or `../x.sh` outside ./scripts/ and ./.github/ is now reported by name, with the paved-road fix, whenever it could name a tracked script. Handing it to an interpreter still passes, and a relative path that names no tracked script stays out of scope, exactly as an untracked modelled path does. The committed tree resolves the identical directly-invoked set and reports nothing new. Fixes #3688 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Aut24ni8XbKPYyY5mYrbb
Evidence for head This is the fail-closed increment #3688 names as the smaller first step. Full resolution of RED: new fixtures against
|
| fixture | main | want |
|---|---|---|
working-directory: scripts + run: ./fixture-target.sh |
rc=0 (exec-bit guard OK) |
1 |
working-directory: scripts/tests + run: ../fixture-target.sh |
rc=0 | 1 |
cd scripts && ./fixture-target.sh |
rc=0 | 1 |
working-directory: scripts + run: bash ./fixture-target.sh |
rc=0 | 0 (interpreter control) |
GREEN: this head
- Full suite: 34 assertions pass. The 29 pre-existing ones are unmodified, and 5 were added: the four above, plus the scope control
./not-a-tracked-script.sh, which stays accepted. - The failure names the invocation and the fix:
'./fixture-target.sh' is invoked directly, but this guard cannot resolve './fixture-target.sh' to a tracked path … Invoke it from the repository root by its tracked path … or hand it to an interpreter (bash ./fixture-target.sh).
Conservation (issue acceptance: same directly-invoked set)
- I dumped the resolved set from the old and new guards over the
maintree and over this branch's tree. The sets are identical in both cases (36 tracked + 2 untracked fixture paths), and the new guard reports no unresolved path on the real tree:exec-bit guard OK; 36 directly-invoked script(s) are tracked 100755. - Why the "could name a tracked script" filter is needed: with it ablated, the real tree fails, because the guard flags the test file's own fixture strings (
'./fixture-target.sh','../fixture-target.sh', …). Those are strings a test builds, exactly like the untracked./scripts/fixture paths the guard already skips. With the filter on, the fixture repositories, where the target is tracked, still fail closed. The filter therefore does not swallow real cases.
Interaction with #4056 (same guard, #3693)
Both PRs change the same file, in different hunks. A trial merge of this branch onto #4056's head applied cleanly, and the merged result passes 46 assertions. The real tree passes (36 … 100755) and shellcheck is clean. The trial merge was discarded afterwards. Whichever lands second should still re-run the suite after rebasing.
Validation
shellcheck over the guard and its test ✅ · bash scripts/guard-tracked-exec-bit.sh . ✅ · the suite ✅. The suite also passes with goawk (a strict POSIX awk) substituted for awk.
No runtime surface beyond CI: the guard only runs in CI, and the fixtures drive that exact path.
Generated by Claude Code
@coderabbitai review Generated by Claude Code |
|
|
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 20ee57d210e8765b6a5c550a466ebce10917c0d9
- CodeRabbit: requested at this head 2026-09-23T05:15:0xZ (comment 5789481453). It replied "Review limit reached — next included review available in 53 minutes" at 2026-09-23T05:15:21Z, and its command reply reads "Action not completed — Review rate limited". No review was delivered at this head.
- Codex: usage limit reached (measured 2026-09-22T20:53:06Z on monorepo#3507; maintainer-only, not re-requested)
- Cursor Bugbot: usage limit reached (measured 2026-09-22T20:53:35Z on monorepo#3507; maintainer-only, not re-requested)
What I checked (independent reviewer, not the author):
- Direction: the change only adds refusals. An unresolved relative path is diverted to a named
::error::, and a modelled./scripts/or./.github/path still reaches the existing mode check. A modelled path can never be misrouted into the unresolved list:RELATIVE_PATH_REcannot cross whitespace, and every occurrence ends at its path. - Tests: the suite passes locally (34 assertions) and
shellcheckis clean on the guard and its test. The three fail-closed fixtures (working-directory, parent-relative, andcd … &&) plus the interpreter and untracked-name controls discriminate as described in the evidence comment. - Real tree:
exec-bit guard OK; 36 directly-invoked script(s) are tracked 100755. I instrumented the unresolved list before filtering. Its only candidates are the test file's own fixture strings (./fixture-target.sh,../fixture-target.sh,./not-a-tracked-script.sh). No tracked script carries those names, socould_name_tracked_scriptdrops them. The resolved direct set is byte-identical tomain's guard (38 lines). - Injection: the
::error::line interpolates a repository-derived path, but that path is constrained to[A-Za-z0-9_./-]+\.sh, so it cannot carry a workflow command. - CI: the guard and suite run unconditionally in
🔍 Detect Changes. That job succeeded at this head, and so didCI - Required Checks(28 checks: 17 success, 11 skipped). - Interaction with #4056 (same file): a trial merge of this head with #4056's current head
0d194063applies cleanly with no conflicts. The merged suite passes (50 assertions), the real tree passes (36), andshellcheckis clean. Whichever lands second should still mergemainand re-run the suite. - Nits (not blocking): the untracked-name filter is a suffix match against every tracked
*.sh, so a common basename (install.sh) could someday make an unrelated relative call fail closed. That is loud and self-explanatory. The greedy-match gap for./a.sh ./b.sh-shaped lines is pre-existing onmainand reported separately.
Verdict: no P0/P1 findings
Generated by Claude Code
2 participants
Why
A CI step can run a script from a different directory, so a short relative call such as
./foo.shcan run one of the repository's own scripts. The exec-bit guard, which makes sure such scripts are committed as executable, could not tell what file that short path named. It therefore reported success without checking it. Nothing is affected today, but the first step written this way would pass silently and could then fail in CI.What
The guard now fails with a clear message naming any such call it cannot resolve, and says how to fix it. The same path handed to an interpreter is still accepted, and the repository's current scripts give exactly the same result as before.
Fixes #3688
🤖 Generated with Claude Code
https://claude.ai/code/session_011Aut24ni8XbKPYyY5mYrbb
Generated by Claude Code