Skip to content

fix(source-control): skip the sandbox's /dev/null dotfile masks in feedback-thread record - #9551

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/feedback-thread-sandbox-devnull
Oct 24, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
fix/feedback-thread-sandbox-devnull

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: root-caused from the address-feedback stops on #9536, #9479 and #9483 today; this is the fix.

Summary

pr-unblock's address-feedback fixed VALID findings, then feedback-thread.sh record refused to record the reply: "the working tree changes .bash_profile, which is outside every kept thread's scope". The lane's Bash sandbox masks dotfiles in the checkout with /dev/null, git ls-files --others lists that device as untracked, and is_stub excuses only empty regular files. With no reply recorded, the reply job stopped feedback-invalid (no replies beside a patch, publish-feedback.mjs:404-406) or feedback-incomplete (:422-425), and a person had to finish every lane PR with a review finding.

PR run stop
#9536 40050576425 feedback-invalid
#9479 40005930064 feedback-invalid
#9483 r1 40014113360 feedback-incomplete (2 of 3)
#9483 r3 40048712108 feedback-incomplete (1 of 4)

Each run's reply quotes the .bash_profile refusal. The feedback-uncertain stops (#9483 r2, #9418) were correct escalations and are unchanged.

Fix

  • plugins/source-control/skills/pull-request/scripts/feedback-thread.sh: the untracked loop skips a character device that is not a symlink, the same rule scripts/check-affected.sh:130-133 uses since fix(ci): skip device nodes when check-affected stages uncommitted work #8520. -L is tested first because -c follows symlinks, so an untracked symlink to /dev/null is still scope-checked and refused.
  • feedback-thread.test.sh: a bwrap case with /dev/null mounted over .bash_profile (fails before the fix, passes after) and a symlink case that must still refuse.
  • .changes/source-control/: patch fragment.

Verification

  • bash plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh: 0 failures; case 79 fails on origin/main's script (exit 1) and passes with the fix.
  • shellcheck on both files: clean.

Related

#9536, #9479, #9483, #8520

🤖 Generated with Claude Code

…edback-thread record

The lane's Bash sandbox masks dotfiles in the checkout with /dev/null, so git
ls-files --others lists .bash_profile as untracked and record refused every
VALID reply as outside the kept threads' scope. address-feedback then stopped
feedback-invalid or feedback-incomplete (#9536, #9479, #9483). Skip a
character device that is not a symlink, as check-affected.sh does since #8520;
a symlink to /dev/null is still refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T02:46:34.554867Z 42f9633 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 42f9633eb7fdeb0809afcb0f514d752b6f7f7027: 0 finding(s), 0 as inline comments.

No security issues found in PR #9551.

The change makes the untracked-file scope check in feedback-thread.sh record skip character devices that are not symlinks. The skip is [[ -L $workspace/$path || ! -c $workspace/$path ]] || continue at plugins/source-control/skills/pull-request/scripts/feedback-thread.sh:183.

  • Scope-check bypass: this is the only control touched. Writing a payload into a path that evades the check would need a character device in the checkout, and creating one needs root (mknod) or a sandbox-level bind mount. An agent or PR author working with file-write access can't do either. A device node also carries no file content, so skipping it can't hide a code change.
  • Symlink handling: -L is tested before -c, and -c follows symlinks. An untracked symlink to /dev/null, or to any other device, is still scope-checked and refused. The new test covers this case.
  • Path handling: $path comes from git ls-files -z and is always quoted. No new injection, path-traversal or secret-handling surface is added.
  • Other files: the test file and the .changes fragment are inert for this review.
  • Instruction surface and manifests: no standing instruction was deleted or softened, and no plugin manifest changed, so there are no manifest <field> lines.
  • PR context: the PR description contains no embedded directives.

critical-findings: 0

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed 42f9633eb7fdeb0809afcb0f514d752b6f7f7027: 0 finding(s), 0 as inline comments.

No findings on this pull request.

What I checked

  • feedback-thread.sh:183 skips an untracked path only when it is a character device and not a symlink. The -L test runs first, which matters because -c follows symlinks, so an untracked symlink to /dev/null is still scope-checked and refused. The new line sits before is_stub, so stub handling is unchanged.
  • The skip applies only in the untracked-file loop of the VALID path. Tracked changes from git diff --name-only are still checked. Creating a character device takes root, so a reply can't use the skip to hide a real change.
  • workspace is set and required at the top of the script (line 42), so the $workspace/$path reference is safe. Paths come from ls-files -z, so quoting is correct.
  • The tests cover both sides. One bwrap case mounts /dev/null over .bash_profile and expects exit 0. It is skipped when bwrap is unavailable. A second case uses an untracked symlink to /dev/null and expects exit 1.
  • The changelog fragment uses bump: patch, which fits a bug fix with no interface change.
  • There is no closing issue (the PR says so), no manifest change, and no hold class: no cost, no trust-gate files, and no CRITICAL security finding.

I read the diff and the surrounding script. I did not run the tests, so the author's claims of 0 failures and a clean shellcheck are unverified by me.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit e486adc Oct 24, 2026
35 checks passed
@kyle-sexton
kyle-sexton deleted the fix/feedback-thread-sandbox-devnull branch October 24, 2026 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant