forked from bchanot/claude
fix(effort): harden lib/effort-pins.sh (security gate, 4 LOW)
Last map line without newline read; unclosed frontmatter skipped with an err; level re-read after write, mismatch counted as failed; mktemp + cp -p + mv, temp removed on failure; rc 1 on any rejected or failed entry. Cases T11-T14 in the fixture suite; contract criteria 8-9.
This commit is contained in:
bastien
@@ -12,6 +12,7 @@
|
||||
- Defect found in passing, fixed here: `update-all.sh` re-fetched the vendored skills but never re-applied the pins (lost until the next `make plugin`).
|
||||
- Defect found in passing, surfaced not fixed: ~94 % of sub-agent usage records carry no `output_tokens_details`, so `lib/effort-audit.py` read zero thinking on sub-agents; the script now prints coverage and a CAVEAT; EVAL-037's "executors stay cheap" is a measurement gap (registry correction pending user approval).
|
||||
- lib/tests/effort-routing.test.sh line 4 widens its shellcheck directive from SC2015 to SC2015,SC2016: the new `has … '$REPO'` locks are literal source text, the `$REPO` must NOT expand (authorized; a test file, informational). [verifier 2026-09-29 gap 3]
|
||||
- Hardening round (criteria 8-9) added after the security gate on user go; the fixture suite may `chmod` its own mktemp directory (555 then back to 755 for the trap cleanup), never `-R`, never outside the fixture.
|
||||
- Frontmatter placement of the inserted `effort:` line (after `name:`, else before the closing `---`) has no harness effect; locked by the fixture suite only.
|
||||
|
||||
## ACCEPTANCE CRITERIA
|
||||
@@ -44,6 +45,15 @@
|
||||
EXPECT: LINT_OK
|
||||
EVIDENCE: MET exit=0 marker-found :: LINT_OK
|
||||
|
||||
8. Hardening (security gate 2026-09-29, 4 LOW, user go): (a) a map whose last line has no trailing newline still applies that line; (b) a SKILL.md whose frontmatter has no closing `---` is skipped with an err line, file byte-identical; (c) a CRLF SKILL.md (`---\r`) is never counted as applied: the helper re-reads the level after the write and reports a mismatch as err, counted as failed (rc 1); (d) a write failure (read-only skill directory) is reported as err, counted as failed, and leaves no temporary file behind. Cases T11-T14 in lib/tests/effort-pins.test.sh, header comment of the helper updated.
|
||||
CHECK: out=$(make test suite=lib/tests/effort-pins.test.sh 2>&1); echo "$out" | grep -q 'effort-pins: [0-9]* pass, 0 fail' || { echo "$out" | grep FAIL; exit 1; }; for k in T11-last-line-no-newline T12-unterminated-frontmatter-skipped T13-crlf-not-counted-applied T14-write-failure-no-temp; do echo "$out" | grep -q "PASS $k" || { echo "missing PASS $k"; exit 1; }; done; echo HARDEN_GREEN
|
||||
EXPECT: HARDEN_GREEN
|
||||
EVIDENCE: MET exit=0 marker-found :: HARDEN_GREEN
|
||||
9. Hardening keeps everything else green: shellcheck clean on the helper and its suite, effort-routing census green, live tree still idempotent (0 applied).
|
||||
CHECK: shellcheck lib/effort-pins.sh lib/tests/effort-pins.test.sh && make test suite=lib/tests/effort-routing.test.sh 2>&1 | grep -q 'census: [0-9]* pass, 0 fail' && bash lib/effort-pins.sh 2>&1 | grep -q ' 0 applied, [0-9]* already at level' && echo HARDEN_STABLE
|
||||
EXPECT: HARDEN_STABLE
|
||||
EVIDENCE: MET exit=0 marker-found :: HARDEN_STABLE
|
||||
|
||||
## FILE SCOPE
|
||||
- lib/effort-pins.txt, lib/effort-pins.sh (new); lib/tests/effort-pins.test.sh (new); lib/tests/effort-routing.test.sh
|
||||
- install-plugins.sh, update-all.sh (re-apply call), lib/effort-audit.py (coverage)
|
||||
|
||||
+47
-14
@@ -6,12 +6,16 @@
|
||||
# it back after the last vendoring step of install-plugins.sh and
|
||||
# update-all.sh. Idempotent: same level → untouched, other level →
|
||||
# replaced inside the frontmatter only, skill not vendored → skipped,
|
||||
# malformed map line → rejected loudly, never applied. Placement inside the
|
||||
# malformed map line → rejected loudly, never applied. Four hardenings:
|
||||
# a map whose last line lacks a newline is still read; a SKILL.md whose
|
||||
# frontmatter never closes is skipped untouched; the level is re-read after
|
||||
# every write and a mismatch (CRLF, malformed) counts as failed; the write
|
||||
# goes through a mktemp sibling removed on any failure. Placement inside the
|
||||
# frontmatter has no effect on the harness, which reads the key anywhere.
|
||||
#
|
||||
# Usage: source it, then `apply_effort_pins [repo-root]`
|
||||
# or standalone: bash lib/effort-pins.sh [repo-root]
|
||||
# Exit 1 when at least one map line was rejected.
|
||||
# Exit 1 when at least one map line was rejected or a skill failed.
|
||||
|
||||
EFFORT_PINS_REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
||||
EFFORT_PIN_LEVEL_RE='^(low|medium|high|xhigh|max)$'
|
||||
@@ -29,12 +33,19 @@ _effort_pin_current() {
|
||||
| sed -n 's/^effort: //p' | head -1
|
||||
}
|
||||
|
||||
# _effort_pin_closed <skill-file> → rc 0 when the frontmatter has a closing ---
|
||||
_effort_pin_closed() {
|
||||
awk 'NR==1&&/^---$/{p=1;next} p&&/^---$/{f=1;exit} END{exit !f}' "$1"
|
||||
}
|
||||
|
||||
# _effort_pin_write <skill-file> <name> <level> — replace the frontmatter
|
||||
# `effort:` line, or insert one after `name: <name>` (before the closing
|
||||
# `---` when the frontmatter has no name line). Body lines never change.
|
||||
# Writes a mktemp sibling then renames; any failure leaves no temp behind.
|
||||
_effort_pin_write() {
|
||||
local file="$1" name="$2" level="$3"
|
||||
awk -v n="$name" -v lvl="$level" '
|
||||
local file="$1" name="$2" level="$3" tmp
|
||||
tmp="$(mktemp "$file.XXXXXX")" || return 1
|
||||
cp -p "$file" "$tmp" && awk -v n="$name" -v lvl="$level" '
|
||||
NR==1 && /^---$/ { fm=1; print; next }
|
||||
fm && /^---$/ {
|
||||
if (!done) { print "effort: " lvl; done=1 }
|
||||
@@ -43,16 +54,36 @@ _effort_pin_write() {
|
||||
fm && /^effort: / { if (!done) { print "effort: " lvl; done=1 }; next }
|
||||
fm && $0 == "name: " n { print; if (!done) { print "effort: " lvl; done=1 }; next }
|
||||
{ print }
|
||||
' "$file" > "$file.tmp" && mv "$file.tmp" "$file"
|
||||
' "$file" > "$tmp" && mv "$tmp" "$file" && return 0
|
||||
rm -f "$tmp"
|
||||
return 1
|
||||
}
|
||||
|
||||
# _effort_pin_apply_one <file> <name> <level> → rc 0 applied, 2 already at
|
||||
# level, 1 failed (err line printed, file untouched or write rolled back)
|
||||
_effort_pin_apply_one() {
|
||||
local file="$1" name="$2" level="$3"
|
||||
if ! _effort_pin_closed "$file"; then
|
||||
err "effort-pins: $file: frontmatter never closed — skipped"; return 1
|
||||
fi
|
||||
[ "$(_effort_pin_current "$file")" = "$level" ] && return 2
|
||||
if ! _effort_pin_write "$file" "$name" "$level"; then
|
||||
err "effort-pins: $file: write failed"; return 1
|
||||
fi
|
||||
if [ "$(_effort_pin_current "$file")" != "$level" ]; then
|
||||
err "effort-pins: $file: level not applied (CRLF or malformed frontmatter?)"
|
||||
return 1
|
||||
fi
|
||||
return 0
|
||||
}
|
||||
|
||||
# apply_effort_pins [repo-root] — walk the map, pin every vendored skill
|
||||
apply_effort_pins() {
|
||||
local repo="${1:-$EFFORT_PINS_REPO}" map name level rest file
|
||||
local applied=0 kept=0 rejected=0
|
||||
local repo="${1:-$EFFORT_PINS_REPO}" map name level rest file rc
|
||||
local applied=0 kept=0 rejected=0 failed=0
|
||||
map="$repo/lib/effort-pins.txt"
|
||||
[ -f "$map" ] || { err "effort-pins: map missing: $map"; return 1; }
|
||||
while read -r name level rest; do
|
||||
while read -r name level rest || [ -n "$name" ]; do
|
||||
case "$name" in ''|'#'*) continue ;; esac
|
||||
if [ -n "$rest" ] || ! [[ "$name" =~ $EFFORT_PIN_NAME_RE ]] \
|
||||
|| ! [[ "$level" =~ $EFFORT_PIN_LEVEL_RE ]]; then
|
||||
@@ -61,13 +92,15 @@ apply_effort_pins() {
|
||||
fi
|
||||
file="$repo/skills-external/$name/SKILL.md"
|
||||
[ -f "$file" ] || continue
|
||||
if [ "$(_effort_pin_current "$file")" = "$level" ]; then
|
||||
kept=$((kept + 1)); continue
|
||||
fi
|
||||
_effort_pin_write "$file" "$name" "$level" && applied=$((applied + 1))
|
||||
_effort_pin_apply_one "$file" "$name" "$level"; rc=$?
|
||||
case "$rc" in
|
||||
0) applied=$((applied + 1)) ;;
|
||||
2) kept=$((kept + 1)) ;;
|
||||
*) failed=$((failed + 1)) ;;
|
||||
esac
|
||||
done < "$map"
|
||||
ok "effort-pins: $applied applied, $kept already at level"
|
||||
[ "$rejected" -eq 0 ]
|
||||
ok "effort-pins: $applied applied, $kept already at level, $failed failed"
|
||||
[ "$rejected" -eq 0 ] && [ "$failed" -eq 0 ]
|
||||
}
|
||||
|
||||
if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then
|
||||
|
||||
@@ -5,7 +5,8 @@
|
||||
# skip a skill not vendored, insert before the closing `---` when the
|
||||
# frontmatter has no name line, run idempotently, reject a bad level, a
|
||||
# traversal name and a three-field line before writing anything, and
|
||||
# parse the real map without error. All on a throwaway fixture repo.
|
||||
# parse the real map without error; hardening: last map line without a
|
||||
# newline, unterminated frontmatter, CRLF file and read-only directory. All on a throwaway fixture repo.
|
||||
set -u
|
||||
ROOT="$(cd "$(dirname "$0")/../.." && pwd)"
|
||||
LIB="$ROOT/lib/effort-pins.sh"
|
||||
@@ -56,5 +57,35 @@ out="$(bash "$LIB" "$WORK/real" 2>&1)"; check T9-real-map-parses "$?" 0
|
||||
check T9b-real-map-nothing-applied "$(printf '%s' "$out" | grep -c '0 applied, 0 already')" 1
|
||||
check T10-missing-map-rc "$(bash "$LIB" "$WORK/nowhere" >/dev/null 2>&1; echo $?)" 1
|
||||
|
||||
# hardening: each case in its own fixture repo
|
||||
mkrepo() { R="$WORK/$1"; mkdir -p "$R/lib" "$R/skills-external/$2"; }
|
||||
mkrepo h11 alpha; mkdir "$WORK/h11/skills-external/beta"
|
||||
printf -- '---\nname: alpha\n---\nb\n' > "$WORK/h11/skills-external/alpha/SKILL.md"
|
||||
printf -- '---\nname: beta\n---\nb\n' > "$WORK/h11/skills-external/beta/SKILL.md"
|
||||
printf 'alpha high\nbeta low' > "$WORK/h11/lib/effort-pins.txt"
|
||||
bash "$LIB" "$WORK/h11" >/dev/null 2>&1
|
||||
check T11-last-line-no-newline "$(fm_effort "$WORK/h11/skills-external/beta/SKILL.md")" low
|
||||
|
||||
mkrepo h12 open; f12="$WORK/h12/skills-external/open/SKILL.md"
|
||||
printf -- '---\nname: open\nbody effort: max\n' > "$f12"; b12="$(cat "$f12")"
|
||||
printf 'open high\n' > "$WORK/h12/lib/effort-pins.txt"
|
||||
out="$(bash "$LIB" "$WORK/h12" 2>&1)"; rc=$?
|
||||
check T12-unterminated-frontmatter-skipped \
|
||||
"$rc|$(cat "$f12" | cmp -s - <(printf '%s\n' "$b12") && echo same)|$(printf '%s' "$out" | grep -c "ERR .*$f12")" "1|same|1"
|
||||
|
||||
mkrepo h13 crlf
|
||||
printf -- '---\r\nname: crlf\r\n---\r\nbody\r\n' > "$WORK/h13/skills-external/crlf/SKILL.md"
|
||||
printf 'crlf high\n' > "$WORK/h13/lib/effort-pins.txt"
|
||||
out="$(bash "$LIB" "$WORK/h13" 2>&1)"; rc=$?
|
||||
check T13-crlf-not-counted-applied \
|
||||
"$rc|$(printf '%s' "$out" | grep -c 'ERR ')|$(printf '%s' "$out" | grep -c ' 0 applied, ')" "1|1|1"
|
||||
|
||||
mkrepo h14 ro; d14="$WORK/h14/skills-external/ro"
|
||||
printf -- '---\nname: ro\n---\nb\n' > "$d14/SKILL.md"
|
||||
printf 'ro high\n' > "$WORK/h14/lib/effort-pins.txt"
|
||||
chmod 555 "$d14"; out="$(bash "$LIB" "$WORK/h14" 2>&1)"; rc=$?; chmod 755 "$d14"
|
||||
check T14-write-failure-no-temp \
|
||||
"$rc|$(printf '%s' "$out" | grep -c 'ERR ')|$(find "$d14" -name 'SKILL.md.*' | wc -l)" "1|1|0"
|
||||
|
||||
echo "effort-pins: $pass pass, $fail fail"
|
||||
[ "$fail" -eq 0 ]
|
||||
|
||||
Reference in New Issue
Block a user