8c583b5fd59e83a301d039927c470f727515f90e
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  Defame1297andClaude Opus 5
|
8cfd54f925 |
fix(gates): close the review findings in the gates and their docs
Two reproduced bugs in check-skill-version-bump: - The origin/main-tip check fired even when the pushed skill was byte-identical to main's tip, so a cherry-pick or backport failed a push that ships nothing. The merge-base intersection |
||
|
Defame1297andClaude Opus 5
|
ffcbed6c41 |
fix(tests): replace pipefail-racy echo | grep -q with here-strings
Why Two suites failed intermittently — tests/test-vale-wrap.sh case 21 and tests/test-check-release-needed.sh cases 4 and 15 — on correct output, and never when run alone. The cause is the `echo "$OUT" | grep -q P` idiom under `set -o pipefail`: grep -q exits as soon as it has an answer, bash's echo can hand a multi-line value to the pipe one line at a time, and a write after the reader is gone kills echo with SIGPIPE. pipefail then reports the writer's death, so output that DID match reads as "no match". Every observed failure had lines after its match; case 15's match is on line 1 of 6, the widest window in that file. Forced with a pause before the writer's last line, the pipe form failed 50 of 50 runs; a here-string, a match on the last line, and the same pipe without pipefail each passed 50 of 50. Unforced the rate is about 1 per 670 suite runs, which is why it read as a flaky gate rather than a bug. The failures at review time are consistent with this, but were not proven to be it: the suite was running while agents edited live config files in place, and a brief change to .vale.ini or .pre-commit-hooks.yaml would produce the same two failures. The race is real and fixed either way. Implementation Notes `grep -q P <<< "$VAR"` has no separate writer process, so there is nothing to race. It is not a retry or a sleep. 121 sites converted across 9 files, three of them scripts rather than tests: new-agent.sh, new-skill.sh and check-executables-allow-sync.sh. None ships via .pre-commit-hooks.yaml, so no external consumer pins them, and all three are single-pipeline checks whose verdict cannot change. Left alone deliberately: 14 sites whose writer is a command, not a shell builtin — they either absorb the writer's status with `|| true` or are python3 and awk, which write once at exit — and one file with no pipefail. `printf '%s'` sites differ from a here-string only by a trailing newline, which no -q verdict on a non-empty pattern depends on. tests/test-no-pipefail-early-exit-grep.sh is a static guard against new occurrences, discovered automatically by run-tests.sh. It only scans files that set pipefail, joins continuation lines, skips comments, and flags only echo/printf writers. Its first case proves the scanner can fail before its second trusts a clean verdict on the tree. A guard covers exactly the spellings its regex models, so the miss surface was measured rather than assumed. Four were found and closed: pipefail declared as `set -o errexit -o pipefail` (where the old pattern required pipefail to follow the FIRST -o, and a file-level miss skips every site in that file); a writer separated from grep by an intermediate stage; a pipeline wrapped on a trailing `|` rather than a backslash; and readers spelled egrep, fgrep, /bin/grep, `command grep` or with an env-var prefix. Segment characters exclude a bare `&` so `echo ok && other | grep -q x`, whose writer is `other`, does not false-fire. Widening surfaced 5 live sites invisible to the original scanner, all in tests/test-apm-current-hook.sh, all `echo "$out" | json_field ... | grep -q`; they are safe today only because json_field is python3, which reads to EOF and writes once. Fixtures go 4 to 12 vulnerable spellings plus near-miss negatives. Two `grep ... | head -1` sites (test-vale-wrap.sh) are the same race with a different early-exiting reader, and are fixed by absorbing the writer. The scanner deliberately does not model `head`, `sed -n 1p` or a bare `read`: most legitimate uses in this tree are already absorbed with `|| true` and the scanner cannot see absorption from pipeline text, so a high false-positive rate would be how this guard gets weakened. Heredoc bodies are scanned as code; none in the tree trips it today. Impact The bug predates the factory-audit merge: every converted site in check-release-needed and case 21 dates to |
||
|
Defame1297andClaude Sonnet 5
|
1b01e25f3b |
refactor(hooks): shrink check-executables-allow-sync (finding 2)
Trim the two comment blocks that re-derived ADR-0019's argument in full (apm's exact-dict-lookup key matching, and why the PyYAML fallback is not a hard requirement) down to a short summary plus a pointer at ADR-0019, which already carries that reasoning verbatim. 231 -> 222 lines. The hook is kept, not deleted, per the audit's own corrected scope: the "or drop it" option in SIMPLIFICATION-AUDIT.md finding #2 is off the table because ADR-0019's Consequences section and the script's own header both call this failure mode silent, and the ADR says a silent-staleness failure here is strictly worse than the duplication this repo's other gates catch. The dual-reader design (PyYAML preferred, hand-rolled shape-scan fallback) is also kept as-is: it exists specifically so a missing python3/PyYAML can't silently skip the check or block every push, which is exactly the loud-failure guarantee this finding must not weaken. No genuine redundancy was found in the parsing logic, the per-branch Why/Fix error messages (each tied to a specific test), or the test matrix (which verifies the two readers agree across every failure mode) without cutting something load-bearing -- so those are untouched, and tests/test-check-executables-allow-sync.sh needed no changes since script behavior and output are byte-identical. All 23 tests in tests/test-check-executables-allow-sync.sh pass, and `pre-commit run check-executables-allow-sync --all-files --hook-stage pre-push` passes against the real repo state. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD |
||
|
Defame1297andClaude Opus 5
|
099cf5846c |
ci: gate the executables.allow key against kyberforge's version
apm approves a package's hooks and bin by an exact dictionary lookup on a composed `name#version` key (apm_cli/security/executables.py, is_package_approved). There is no wildcard and no version-less form, so bumping plugins/kyberforge/apm.yml without editing root apm.yml's `kyberforge#<version>` key errors nowhere: the entry stops matching, the SessionStart hook stops deploying, and the install goes quietly stale. ADR-0019 already named that as a live failure mode, mitigated only by a comment in the executables block. This repo gates generated-content drift, marketplace mirror drift and vale style drift deterministically, and a silent-staleness failure is worse than any of them — a comment does not survive the release that breaks it. check-executables-allow-sync.sh parses the version out of the plugin manifest and asserts the matching key exists in the root manifest. It uses PyYAML where importable and falls back to a two-shape scan otherwise, so a missing pip package cannot become the thing that blocks every push; the test asserts both readers agree. 23 assertions. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT |