diff --git a/.changes/source-control/fix-feedback-thread-sandbox-devnull-5dcba90c.md b/.changes/source-control/fix-feedback-thread-sandbox-devnull-5dcba90c.md new file mode 100644 index 00000000000..41b035e536d --- /dev/null +++ b/.changes/source-control/fix-feedback-thread-sandbox-devnull-5dcba90c.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `feedback-thread.sh record` no longer refuses a VALID reply because the lane's sandbox masks a dotfile in the checkout (such as `.bash_profile`) with `/dev/null`. It skips a character device that is not a symlink, as `check-affected.sh` does. An untracked symlink to `/dev/null` is still refused. This was why address-feedback stopped with `feedback-invalid` or `feedback-incomplete` on lane PRs whose findings it had fixed. diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh index 28b510e8688..2d220c10bf2 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh @@ -179,6 +179,8 @@ cmd_record() { mapfile -d '' -t untracked < <(git_tree ls-files --others --exclude-standard -z) wait "$!" || die 1 "cannot read the working tree's untracked files" for path in "${untracked[@]}"; do + # The lane's sandbox masks a dotfile with /dev/null, a device git will not add. + [[ -L $workspace/$path || ! -c $workspace/$path ]] || continue is_stub "$path" || paths+=("$path") done ((${#paths[@]} < 300)) || die 1 "the working tree changes too many files to check" diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh index 53510e1cff6..cecafc47301 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh @@ -228,6 +228,25 @@ echo 'guarded' >"$STUB/ws/src/a.js" mkdir -p "$STUB/ws/src/nested" && : >"$STUB/ws/src/nested/package.json" run record PRRT_a "$STUB/scratch/d.json" >/dev/null 2>&1 assert_exit "an empty stub name below the top of the checkout refuses the VALID record" 1 "$?" +# The lane's sandbox masks a dotfile in the checkout with /dev/null, a device git +# will not add: it is no fix, while a symlink to /dev/null still is one. +if bwrap --bind / / --dev /dev true 2>/dev/null; then + setup + echo 'guarded' >"$STUB/ws/src/a.js" + : >"$STUB/ws/.bash_profile" + disposition '{"disposition":"VALID","evidence":["src/a.js:3"],"fix_sha":"","note":"fixed"}' + PATH="$TEST_TMPDIR/bin:$PATH" FEEDBACK_THREADS="$STUB/threads.json" FEEDBACK_LOG="$STUB/replies.jsonl" \ + FEEDBACK_SCRATCH="$STUB/scratch" FEEDBACK_WORKSPACE="$STUB/ws" \ + bwrap --bind / / --dev /dev --ro-bind /dev/null "$STUB/ws/.bash_profile" \ + bash "$SCRIPT" record PRRT_a "$STUB/scratch/d.json" >/dev/null 2>&1 + assert_exit "a sandbox /dev/null mask over a dotfile does not refuse the VALID record" 0 "$?" +fi +setup +echo 'guarded' >"$STUB/ws/src/a.js" +ln -s /dev/null "$STUB/ws/.bash_profile" +disposition '{"disposition":"VALID","evidence":["src/a.js:3"],"fix_sha":"","note":"fixed"}' +run record PRRT_a "$STUB/scratch/d.json" >/dev/null 2>&1 +assert_exit "an untracked symlink to /dev/null refuses the VALID record" 1 "$?" setup disposition '{"disposition":"VALID","evidence":["src/a.js:3"],"note":"fix_sha left out"}'