fix(bin): fix BSD sed -i inbox bug and record known test failures - #2851
fix(bin): fix BSD sed -i inbox bug and record known test failures#2851pramendra wants to merge 5 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (4): Last reviewed commit: "no-mistakes(review): Untrack accidentall..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Final whole-repo confirmationIndependently of the pipeline's targeted test step, I ran the complete The 4 remaining failures are exactly the ones this PR documents and files issues for:
Net result: of the original 9 failures reported, 5 are now fixed on this branch (2 actionlint self-skips already on |
9113451 to
98c16c1
Compare
… absent Real workflow-YAML linting can't be faked with a stub binary, so 9 tests across fm-lint-workflows.test.sh and fm-lint.test.sh invoke the genuine actionlint on the ambient PATH. Without the pinned 1.7.12 installed locally, fm-lint-workflows.sh fails closed and these tests failed with it, even though CI always installs the pin first and was never at risk. Mirror the existing pinned_ready guard already used for ShellCheck in fm-lint.test.sh: add an equivalent actionlint_ready check and self-skip the affected tests when the pin isn't resolved, instead of failing closed. Coverage is unchanged wherever the pin is present (CI, or after running bin/fm-install-actionlint.sh locally).
Whole-repo suite triage found 4 failures that reproduce identically on a fresh, unmodified origin/main checkout and are not caused by any branch: herdr 0.8.2 and Pi 0.84.2 version drift against e2e mitigations verified on older releases, a real PATH-ordering discrepancy in fm-on.sh's remote dispatch path, and a load-sensitive flake in fm-watcher-lock.test.sh that only reproduces under full-suite concurrent load. Each is filed as its own issue (kunchenguid#2841-kunchenguid#2844) rather than folded into this triage, and recorded here so a contributor can tell their own branch's failures apart from these without re-deriving the triage from scratch.
…TRIBUTING A local --all walk was already documented as not being no-mistakes Test, but nothing told a contributor that a non-clean result there might be pre-existing rather than caused by their branch.
…stale facts found
98c16c1 to
a33e2b0
Compare
1 participant
Intent
Triage and reduce firstmate's whole-repo pre-existing test-suite failures on main: verify the actionlint-absence premise, fix real portability bugs found along the way (BSD sed -i incompatibility in bin/fm-inbox.sh, BSD wc -l padding in two test files' string comparisons), confirm the remaining failures (herdr/pi version drift, an fm-on.sh PATH-ordering discrepancy, a load-sensitive watcher-lock flake) reproduce identically on pristine main and are not caused by any branch, file each as its own GitHub issue, and record them durably in docs so contributors stop re-triaging the same failures.
What Changed
sed -iincompatibility inbin/fm-inbox.sh'squeue_note: instead of relying on GNU-only in-place editing, it now writes the rewritten note to a temp file (withchmod 600) and renames it into place.wc -lwhitespace-padding in string comparisons intests/fm-tool-update-check.test.shandtests/fm-voice-relay.test.sh(piping throughtr -d ' '), and added actionlint-pin self-skip guards to the actionlint-dependent cases intests/fm-lint-workflows.test.shandtests/fm-lint.test.sh, mirroring the existing ShellCheckpinned_readyguard.docs/verification/known-test-failures.md, cataloguing pre-existingmaintest failures reproduced independently of any branch (herdr/Pi version drift, anfm-on.shPATH-ordering discrepancy, and a load-sensitivefm-watcher-lockflake) with linked GitHub issues, and wired it intoCONTRIBUTING.md,docs/documentation-audiences.json, and.gitignore(.serena/).Risk Assessment
✅ Low: Both prior review findings (accidental .serena commit, dropped 0600 permission on inbox notes) are correctly fixed in the current HEAD with no regressions; the remaining diff (actionlint self-skip guards, BSD wc -l portability, known-test-failures doc, CONTRIBUTING pointer) is well-scoped, matches the stated intent, and all four referenced GitHub issues (#2841-#2844) exist and match their doc descriptions.
Testing
All targeted tests pass on a genuine BSD (macOS) environment: the fm-inbox.sh sed rewrite and the wc -l padding fixes in fm-tool-update-check.test.sh/fm-voice-relay.test.sh are exercised and pass, and the actionlint self-skip guard was verified in both states (actionlint present → runs real checks; actionlint hidden from PATH → self-skips cleanly instead of failing). One unrelated timing-sensitive flake ('the spoken round trip did not hold') appeared on a first run of fm-voice-relay.test.sh and passed cleanly on immediate re-run with no code changes, confirming it's an environmental flake unrelated to this diff (the diff touches only fm-inbox.sh and wc -l assertions, not the voice timing path) and not one of the three failures this change documents, so no action was taken on it. No leftover artifacts; worktree is clean.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
.serena/project.yml:1- Commit 0d0ad7f ("Verify test-failure docs already accurate; no stale facts found") adds .serena/.gitignore and .serena/project.yml — an auto-generated local Serena MCP tool config with a session-specific project_name ("01M0PW11N8HWG78YANJX18G325") — despite the commit message claiming no changes were needed. This directory does not exist on main and is unrelated to the stated verification task. Git history on this same repo shows this exact accidental-commit pattern recurring and being reverted in follow-up commits (e.g. "chore: drop accidentally committed .serena local tool config", "no-mistakes(review): Drop accidentally committed .serena local tool config"), confirming it is tool-session noise rather than an intentional deliverable.🔧 Fix: Untrack accidentally committed .serena config; ignore .serena/
1 warning still open:
bin/fm-inbox.sh:187- The BSD-portability fix for the id rewrite silently drops the note file's permissions from 0600 to the process umask (typically 0644, world-readable).mktemp "$INBOX/.staging-XXXXXX"on line 172 creates$tmpat mode 0600 regardless of umask, but the replacementsed "..." "$tmp" >"$tmp.rewritten"on line 187 creates$tmp.rewrittenas a fresh file subject to umask, and the followingmv "$tmp.rewritten" "$tmp"(line 188) thenmv "$tmp" "$INBOX/$id.note"(line 189) propagate that looser mode to the final note. Verified concretely:mktempyields-rw-------, and aftersed ... > f.rewritten && mv f.rewritten funder umask 022 the file becomes-rw-r--r--. Every captain note/voice-transcribed body written byfm-inbox.sh note/sayis now world-readable on a shared machine, whereas before this fix (on Linux, where GNUsed -iactually worked pre-portability-fix) the original 0600 mode was preserved by GNU sed's in-place edit. The codebase treats 0600 as a deliberate security baseline for exactly this kind of artifact (see the explicitchmod 0600calls in bin/fm-wake-lib.sh:510,619). Fix: addchmod 600 "$tmp.rewritten"before the firstmv.🔧 Fix: Restore 0600 perms on rewritten inbox note before rename
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash bin/fm-test-run.sh tests/fm-voice-relay.test.sh — exercises bin/fm-inbox.sh's queue_note (BSD sed -i fix) and the wc -l padding fix; all cases passbash bin/fm-test-run.sh tests/fm-tool-update-check.test.sh — exercises the wc -l padding fix in the one-line-report assertions; all cases passbash bin/fm-test-run.sh tests/fm-lint.test.sh tests/fm-lint-workflows.test.sh — with actionlint 1.7.12 present on PATH, all actionlint-dependent regression checks run for real and passenv PATH=<actionlint removed> bash bin/fm-test-run.sh tests/fm-lint-workflows.test.sh — confirms the self-skip guard actually engages (SKIP lines) instead of failing closed when the pinned actionlint is absent from PATHgit status --porcelain after test runs — worktree clean, no leftover test artifacts✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.