forked from molecule-ai/molecule-core
fix(sop-tier-check): clause splitter strips newlines, OR-set collapses to one token (#229)
PR #225 introduced the AND-composition clause evaluator. PR #231 patched the per-team case-pattern matching but did NOT fix the underlying clause-splitter bug. This PR fixes the actual root cause behind issue #229. Root cause (.gitea/scripts/sop-tier-check.sh ~line 289): _clause=$(echo "$_raw_clause" \ | tr -d '()' \ | tr ',' '\n' \ | tr -d '[:space:]' \ | grep -v '^$') `tr -d '[:space:]'` strips the newlines that `tr ',' '\n'` just inserted. For tier:low (expression "engineers,managers,ceo") the intermediate value is: engineers\nmanagers\nceo then `tr -d '[:space:]'` flattens it to: engineersmanagersceo The for-loop iterates ONCE over this single bogus token. The case pattern `*engineersmanagersceo*` never matches APPROVER_TEAMS values like " managers ", so EVERY tier:low PR fails: ::error::clause [engineers/managers/ceo]: FAIL — no approving reviewer belongs to any of these teamsengineersmanagersceo ::error::sop-tier-check FAILED for tier:low (Note: the missing separators in the error string `teamsengineersmanagersceo` were a SECOND, masked bug — `_clause_names="${_clause_names:+, }${_t}"` overwrites the variable on every iteration instead of appending. With the splitter bug, the inner loop only ran once so the overwrite was invisible. Fixing the splitter unmasks the accumulator bug, so we fix both atomically.) Fix: _no_parens=${_raw_clause//[()]/} _clause=${_no_parens//,/ } # comma -> space, bash word-split iterates # Append, don't overwrite: _clause_names="${_clause_names}${_clause_names:+, }${_t}" _passed_clauses="${_passed_clauses}${_passed_clauses:+, }$_label" _failed_clauses="${_failed_clauses}${_failed_clauses:+, }$_label" Per-tier policy is UNCHANGED — this is a parser fix, not a policy relaxation: tier:low — engineers,managers,ceo (OR-set, ANY ONE suffices) tier:medium — managers AND engineers AND qa???,security??? tier:high — ceo Test: .gitea/scripts/tests/test_sop_tier_check_clause_split.sh asserts the splitter, accumulators, and end-to-end OR-gate matching against APPROVER_TEAMS=" managers " (the exact shape PRs #233-238 hit). 7/7 pass on the new logic. Refs: #229, supersedes attempted fix in #231 for the same root cause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
dev-lead
parent
9cb5f43140
commit
b75187d11c
@ -285,12 +285,26 @@ _passed_clauses=""
|
||||
_failed_clauses=""
|
||||
|
||||
for _raw_clause in $EXPR; do
|
||||
# Normalise: strip parens, split on comma, trim whitespace.
|
||||
_clause=$(echo "$_raw_clause" | tr -d '()' | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
|
||||
# Normalise: strip parens, replace commas with spaces so bash word-split
|
||||
# can iterate the OR-set members. The previous form
|
||||
# _clause=$(echo ... | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
|
||||
# collapsed every member into one concatenated token because
|
||||
# `tr -d '[:space:]'` strips the very newlines that just separated them
|
||||
# ("engineers,managers,ceo" -> "engineersmanagersceo"), so the OR-clause
|
||||
# only ever evaluated as a single nonsense team name and never matched
|
||||
# APPROVER_TEAMS. Fixed in #229: leave the comma-separated members as
|
||||
# space-separated tokens for `for _t in $_clause`.
|
||||
_no_parens=${_raw_clause//[()]/}
|
||||
_clause=${_no_parens//,/ }
|
||||
_clause_passed="no"
|
||||
_clause_names=""
|
||||
for _t in $_clause; do
|
||||
_clause_names="${_clause_names:+, }${_t}"
|
||||
# Append (don't overwrite) team name to the human-readable accumulator.
|
||||
# The previous form `_clause_names="${_clause_names:+, }${_t}"`
|
||||
# rewrote the variable on every iteration, so the FAIL message only
|
||||
# ever showed the LAST team. Fixed: prepend prior value before the
|
||||
# comma-separator, then append the new team name.
|
||||
_clause_names="${_clause_names}${_clause_names:+, }${_t}"
|
||||
# Skip teams not yet in Gitea (qa??? / security??? placeholders).
|
||||
[[ "$_t" == *"???" ]] && debug "clause \"$_t\": skipped (team pending creation)" && continue
|
||||
[ -z "${TEAM_ID[$_t]:-}" ] && debug "clause \"$_t\": no ID resolved, skipping" && continue
|
||||
@ -311,11 +325,12 @@ for _raw_clause in $EXPR; do
|
||||
_label=$(echo "$_raw_clause" | tr -d '()' | tr ',' '/' | tr -d '[:space:]' | sed 's/???//g')
|
||||
|
||||
if [ "$_clause_passed" = "yes" ]; then
|
||||
_passed_clauses="${_passed_clauses:+, }$_label"
|
||||
# Append (don't overwrite) — same accumulator bug as _clause_names above.
|
||||
_passed_clauses="${_passed_clauses}${_passed_clauses:+, }$_label"
|
||||
echo "::notice::clause [$_label]: PASS — satisfied by approving reviewer(s)"
|
||||
else
|
||||
_failed_clauses="${_failed_clauses:+, }$_label"
|
||||
echo "::error::clause [$_label]: FAIL — no approving reviewer belongs to any of these teams${_clause_names}. Set SOP_DEBUG=1 to see per-team probe results."
|
||||
_failed_clauses="${_failed_clauses}${_failed_clauses:+, }$_label"
|
||||
echo "::error::clause [$_label]: FAIL — no approving reviewer belongs to any of these teams (${_clause_names}). Set SOP_DEBUG=1 to see per-team probe results."
|
||||
fi
|
||||
done
|
||||
|
||||
|
||||
101
.gitea/scripts/tests/test_sop_tier_check_clause_split.sh
Executable file
101
.gitea/scripts/tests/test_sop_tier_check_clause_split.sh
Executable file
@ -0,0 +1,101 @@
|
||||
#!/usr/bin/env bash
|
||||
# Regression test for #229 — sop-tier-check tier:low OR-clause splitter.
|
||||
#
|
||||
# Bug (PR #225 → still broken after PR #231):
|
||||
# Line ~289 of sop-tier-check.sh used:
|
||||
# _clause=$(echo "$_raw_clause" | tr -d '()' | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
|
||||
# `tr -d '[:space:]'` strips the newlines that `tr ',' '\n'` just
|
||||
# inserted, collapsing "engineers,managers,ceo" into a single token
|
||||
# "engineersmanagersceo". The for-loop then iterates ONCE on a name
|
||||
# that matches no team, so every tier:low PR fails:
|
||||
# ::error::clause [engineers/managers/ceo]: FAIL — no approving
|
||||
# reviewer belongs to any of these teamsengineersmanagersceo
|
||||
# (note also: missing separators in the error string is bug #2 —
|
||||
# `_clause_names` used "${var:+, }$x" which OVERWRITES per iteration).
|
||||
#
|
||||
# Fix shape (this PR):
|
||||
# _no_parens=${_raw_clause//[()]/}
|
||||
# _clause=${_no_parens//,/ } # comma -> space, bash word-split iterates
|
||||
# _clause_names="${_clause_names}${_clause_names:+, }${_t}" # APPEND, not overwrite
|
||||
#
|
||||
# This test extracts the splitter logic and asserts it produces the right
|
||||
# token list for each of the three tier expressions live in the script.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
PASS=0
|
||||
FAIL=0
|
||||
|
||||
assert_eq() {
|
||||
local label="$1"
|
||||
local expected="$2"
|
||||
local got="$3"
|
||||
if [ "$expected" = "$got" ]; then
|
||||
echo " PASS $label"
|
||||
PASS=$((PASS + 1))
|
||||
else
|
||||
echo " FAIL $label"
|
||||
echo " expected: <$expected>"
|
||||
echo " got: <$got>"
|
||||
FAIL=$((FAIL + 1))
|
||||
fi
|
||||
}
|
||||
|
||||
# ----- Splitter under test (mirrors the fixed sop-tier-check.sh block) -----
|
||||
split_clause() {
|
||||
local raw="$1"
|
||||
local no_parens=${raw//[()]/}
|
||||
local clause=${no_parens//,/ }
|
||||
local out=""
|
||||
for _t in $clause; do
|
||||
out="${out}${out:+|}$_t"
|
||||
done
|
||||
echo "$out"
|
||||
}
|
||||
|
||||
echo "test: tier:low OR-clause splits to 3 tokens"
|
||||
assert_eq "tier:low" "engineers|managers|ceo" "$(split_clause "engineers,managers,ceo")"
|
||||
|
||||
echo "test: tier:medium AND-expression — bash word-split on \$EXPR yields 5 tokens"
|
||||
EXPR="managers AND engineers AND qa???,security???"
|
||||
out=""
|
||||
for _raw in $EXPR; do
|
||||
out="${out}${out:+ ; }$(split_clause "$_raw")"
|
||||
done
|
||||
assert_eq "tier:medium" "managers ; AND ; engineers ; AND ; qa???|security???" "$out"
|
||||
|
||||
echo "test: tier:high single-team OR-clause"
|
||||
assert_eq "tier:high" "ceo" "$(split_clause "ceo")"
|
||||
|
||||
echo "test: paren-wrapped OR-set unwraps + splits"
|
||||
assert_eq "paren OR" "managers|ceo" "$(split_clause "(managers,ceo)")"
|
||||
|
||||
# ----- _clause_names accumulator (was overwriting per iteration) -----
|
||||
acc=""
|
||||
for t in engineers managers ceo; do
|
||||
acc="${acc}${acc:+, }${t}"
|
||||
done
|
||||
assert_eq "_clause_names append" "engineers, managers, ceo" "$acc"
|
||||
|
||||
# ----- _failed_clauses / _passed_clauses accumulator across raw clauses -----
|
||||
acc=""
|
||||
for c in clauseA clauseB clauseC; do
|
||||
acc="${acc}${acc:+, }${c}"
|
||||
done
|
||||
assert_eq "_failed_clauses append" "clauseA, clauseB, clauseC" "$acc"
|
||||
|
||||
# ----- End-to-end OR-gate: simulate APPROVER_TEAMS[core-lead]=' managers ' -----
|
||||
# The script's case pattern is *${_t}* with a space-padded value.
|
||||
APPROVER_TEAMS_VAL=" managers "
|
||||
matched=""
|
||||
for _t in $(split_clause "engineers,managers,ceo" | tr '|' ' '); do
|
||||
case "$APPROVER_TEAMS_VAL" in
|
||||
*${_t}*) matched="$_t"; break ;;
|
||||
esac
|
||||
done
|
||||
assert_eq "OR-gate matches managers" "managers" "$matched"
|
||||
|
||||
echo
|
||||
echo "------"
|
||||
echo "PASS=$PASS FAIL=$FAIL"
|
||||
[ "$FAIL" -eq 0 ]
|
||||
Loading…
Reference in New Issue
Block a user