diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index 1630d677..87217caa 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -48,14 +48,15 @@ project_dir="${CLAUDE_PROJECT_DIR:-.}" # Like the push check it reads the commands the line runs, so a gh command that # is only mentioned, in a quoted string, a heredoc body or a comment # (`python3 - <<'EOF'` with `# gh pr ready` in it), doesn't count, unless a -# shell runs it: a $(…) (in an argument, a ${…} default, arithmetic or an -# unquoted heredoc's body), `bash -c '…'`, `eval`, or a heredoc or here-string -# a shell reads, directly or through a pipe; that code, as written, quotes and -# heredoc bodies included, is tokenized and judged the same way. Each -# gh command is judged on its own words: a gh api call by its endpoint (its -# first argument that isn't a flag) and its fields, never by text elsewhere on -# the line or inside a field's value, so a comment body that names an endpoint -# is just a body. +# shell runs it: a $(…) (in an argument, a ${…} default, arithmetic, $[ … ] +# included, or an unquoted heredoc's body), `bash -c '…'`, `eval`, or a +# heredoc or here-string a shell reads, directly or through a pipe; that code, +# as written, quotes and heredoc bodies included, is tokenized and judged the +# same way. Each gh command is judged on its own words: a gh api call by its +# endpoint (its first argument that isn't a flag) and its fields, never by text +# inside a field's value or elsewhere on the line (unless its GraphQL query +# reads a shell variable, which may be set there), so a comment body that names +# an endpoint is just a body. # # gh pr merge is not checked here: it is denied in .claude/settings.json for # Claude's own commands, and a person merges with it in shell mode (`!`), which @@ -79,6 +80,7 @@ msg_reviewers="Adding/removing reviewers is humans-only. Re-trigger bot reviewer msg_reviewers_api="Reviewer-write requests are humans-only. Re-trigger bot reviewers via PR comment mention." msg_approve="Only humans approve PRs." msg_changes="Agents post inline review comments instead of --request-changes." +msg_event_dyn="A review event the shell computes can't be checked for APPROVE or REQUEST_CHANGES. Write it literally (event=COMMENT)." merge_msg="Agents don't merge PRs; a person does. Ask the user." # truthy : the value turns a boolean flag on, as gh's flag parser reads it. @@ -180,6 +182,19 @@ pr_flag() { return 0 } +# reads_var : the text reads a shell variable: it has a ${ or a $name that +# isn't a GraphQL variable the text declares ($name: Type). +reads_var() { + local v re + while IFS= read -r v; do + v=${v#?} + [ "$v" != '{' ] || return 0 + re='[$]'$v'[[:space:]]*:' + [[ $1 =~ $re ]] || return 0 + done < <(grep -oE "$var_re" <<<"$1" | sort -u) + return 1 +} + # gh_api : judge `gh api …`, its words from token up to # , in the simple command that starts at token . The endpoint is # its first word that isn't a flag or a flag's value; a field is the value of @@ -189,10 +204,11 @@ pr_flag() { # reads from stdin (`-F key=@-`, `--input -`) is read from the heredoc or # here-string the command itself reads. gh_api() { - local i=$1 e=$2 j w f ep="" method="" fields=0 write=0 path typed stdin="" input=0 - local draft="" title="" event="" query="" qvar=0 values="" + local i=$1 e=$2 j w f d ep="" method="" fields=0 write=0 path typed stdin="" sdyn=0 input=0 + local draft="" title="" event="" edyn=0 query="" qvar=0 values="" vset=0 vdyn=0 k + local fn=() fv=() fd=() for ((j = 0; j < ${#TK_IN[@]}; j++)); do - [ "${TK_IN_AT[j]}" != "$3" ] || stdin=${TK_IN[j]%$'\n'} + [ "${TK_IN_AT[j]}" != "$3" ] || { stdin=${TK_IN[j]%$'\n'}; sdyn=${TK_IN_DYN[j]}; } done for ((j = i; j < e; j++)); do w=${TK_VAL[j]} @@ -216,26 +232,28 @@ gh_api() { *) [ -n "$ep" ] || ep=$w ;; esac [ -n "$f" ] || continue + d=${TK_DYN[j]} if [ "$typed" = 1 ]; then case ${f#*=} in - @-) f=${f%%=*}=$stdin ;; + @-) f=${f%%=*}=$stdin d=$sdyn ;; @*) continue ;; # a file's contents esac fi - values+=" ${f#*=}" + # Which field holds the review's event is known once the query is; one + # other than the query that the shell computes may hold it, unread. + if [ "${f%%=*}" != query ]; then + fn+=("${f%%=*}") fv+=("${f#*=}") fd+=("$d") + [ "$d" = 0 ] || vdyn=1 + fi case $f in draft=*) draft=${f#draft=} ;; - title=*) [ "${TK_DYN[j]}" = 0 ] && title=${f#title=} ;; + title=*) [ "$d" = 0 ] && title=${f#title=} ;; event=*) event=${f#event=} ;; query=*) query=${f#query=} - [ "${TK_DYN[j]}" = 0 ] || case $query in *'$'[A-Za-z_'{']*) qvar=1 ;; esac ;; + [ "$d" = 1 ] && reads_var "$query" && qvar=1 ;; esac done - # A computed query is matched against the code that computes it: its own - # word, heredoc bodies included (`-f query="$(cat <<'EOF' … EOF)"`), and, - # when it reads a variable, the rest of the code, where that may be set. - [ "$qvar" = 0 ] || query+=" $GH_CODE" if [ -n "$method" ]; then case $method in [gG][eE][tT]) ;; *) write=1 ;; esac else @@ -245,25 +263,71 @@ gh_api() { path=${path#/} path=${path%%\?*} path=${path%/} + local fq=$query # the query its fields give # A body sent with --input - is the request's JSON: a GraphQL one is matched # as a query is, and a REST one's draft, event and title are read from it. if [ "$input" = 1 ] && [ -n "$stdin" ]; then if [ "$path" = graphql ]; then query+=" $stdin" - else + # One the shell expands may hold the review's event, read from its text. + if [ "$sdyn" = 1 ]; then + vdyn=1 + [[ $stdin =~ \"event$json_str_re ]] || { [[ $stdin =~ \"event\"[[:space:]]*: ]] && edyn=1; } + fi + [ "$sdyn" = 1 ] && reads_var "$stdin" && qvar=1 + elif [ "$sdyn" = 0 ] && jq . <<<"$stdin" >/dev/null 2>&1; then { read -r draft; read -r event; IFS= read -r title; } < <(jq -r \ '(.draft | tostring), (.event // "" | tostring), (.title // "" | tostring | gsub("\n"; " "))' \ <<<"$stdin" 2>/dev/null) + else + # One the shell expands, or that isn't JSON until it does + # (`"body": $(jq -Rs . notes.md)`), is read from its text; an event that + # isn't a plain string there is computed. + [[ $stdin =~ $json_draft_re ]] && draft=true + [[ $stdin =~ \"title$json_str_re ]] && title=${BASH_REMATCH[1]} + if [[ $stdin =~ \"event$json_str_re ]]; then + event=${BASH_REMATCH[1]} + elif [[ $stdin =~ \"event\"[[:space:]]*: ]]; then + edyn=1 + fi fi fi + # A computed query is matched against the code that computes it: its own + # word, heredoc bodies included (`-f query="$(cat <<'EOF' … EOF)"`), and, + # when it reads a variable, the whole line, where that may be set. + [ "$qvar" = 0 ] || query+=" $cmd" + # The review's event is a field named event or …[event], or one the query's + # event: takes (`event: $e`, `\$e` in an unquoted heredoc) in any mutation the + # request runs; a body that is the word APPROVE or COMMENT isn't. + local rest=$query evnames=" " + while [[ $rest =~ event[[:space:]]*:[[:space:]]*\\?[$]([A-Za-z_][A-Za-z0-9_]*) ]]; do + evnames+="${BASH_REMATCH[1]} " + rest=${rest#*"${BASH_REMATCH[0]}"} + done + for ((k = 0; k < ${#fn[@]}; k++)); do + case ${fn[k]} in event | *'[event]') ;; *) [[ $evnames == *" ${fn[k]} "* ]] || continue ;; esac + case ${fv[k]} in + APPROVE | REQUEST_CHANGES) values+=" ${fv[k]}" vset=1 ;; + COMMENT | DISMISS) vset=1 ;; + *) [ "${fd[k]}" = 0 ] || edyn=1 ;; + esac + done if [ "$path" = graphql ]; then case $query in *markPullRequestReadyForReview*) block "$msg_ready" ;; esac case $query in *mergePullRequest* | *enablePullRequestAutoMerge*) block "$merge_msg" ;; esac case $query in *requestReviews*) block "$msg_reviewers_api" ;; esac + # Not addPullRequestReviewThreadReply or …Comment, which take no event. case $query in - *addPullRequestReview* | *submitPullRequestReview*) + *addPullRequestReview | *addPullRequestReview[!A-Za-z0-9_]* | *submitPullRequestReview*) case "$query$values" in *APPROVE*) block "$msg_approve" ;; esac - case "$query$values" in *REQUEST_CHANGES*) block "$msg_changes" ;; esac ;; + case "$query$values" in *REQUEST_CHANGES*) block "$msg_changes" ;; esac + # A computed event field, or the event in a GraphQL variable no field + # gives literally. + [ "$edyn" = 0 ] || block "$msg_event_dyn" + # A query from a field may spell it \$e, as an unquoted heredoc must. + [[ $vset == 0 && ($vdyn == 1 || $qvar == 1) && + ($query =~ event[[:space:]]*:[[:space:]]*[$] || $fq =~ event[[:space:]]*:[[:space:]]*\\[$]) ]] && + block "$msg_event_dyn" ;; esac case $query in *createPullRequest*) @@ -287,6 +351,7 @@ gh_api() { fi case $path in repos/*/pulls/*/reviews | repos/*/pulls/*/reviews/*/events) + [ "$edyn" = 0 ] || block "$msg_event_dyn" case $event in [aA][pP][pP][rR][oO][vV][eE]) block "$msg_approve" ;; [rR][eE][qQ][uU][eE][sS][tT]_[cC][hH][aA][nN][gG][eE][sS]) block "$msg_changes" ;; @@ -462,14 +527,15 @@ reviewers_script="scripts/pre-push-reviewers.sh" # redirections, heredocs, arithmetic, $(…) and `…`) without expanding or # running anything. A word that carries an expansion is flagged dynamic: its # value is unknowable here, so a push that depends on one fails closed. Fills -# TK_VAL / TK_DYN / TK_OP, TK_IN / TK_IN_AT with each heredoc body and -# here-string outside a $(…) and the token index of the simple command that -# reads it, and TK_SUBS with the text of every $(…) and `…`, wherever it went -# (an argument, a here-string, a redirect target, an unquoted heredoc's body, -# arithmetic), each quoted part and heredoc body inside a $(…) wrapped in -# \002…\003, and TK_CODE with the code inside each one exactly as written, -# quotes and heredoc bodies included, for the gh checks to tokenize in turn; -# returns 1 on an unterminated quote or substitution. +# TK_VAL / TK_DYN / TK_OP, TK_IN / TK_IN_AT / TK_IN_DYN with each heredoc body +# and here-string outside a $(…), the token index of the simple command that +# reads it and whether the shell expands it, and TK_SUBS with the text of +# every $(…) and `…`, wherever it went (an argument, a here-string, a redirect +# target, an unquoted heredoc's body, arithmetic), each quoted part and +# heredoc body inside a $(…) wrapped in \002…\003, and TK_CODE with the code +# inside each one exactly as written, quotes and heredoc bodies included, for +# the gh checks to tokenize in turn; returns 1 on an unterminated quote or +# substitution. tk_flush() { if [ "$_inw" = 1 ]; then @@ -477,7 +543,7 @@ tk_flush() { case $_w in *$'\002'* | *$'\003'*) _w=$(printf '%s.' "$_w" | tr -d '\002\003'); _w=${_w%.} ;; esac case $_skip in 0) TK_VAL+=("$_w"); TK_DYN+=("$_dyn"); TK_OP+=(0) ;; - 2) TK_IN+=("$_w"); TK_IN_AT+=("$_cmd0") ;; + 2) TK_IN+=("$_w"); TK_IN_AT+=("$_cmd0"); TK_IN_DYN+=("$_dyn") ;; esac _skip=0 fi @@ -527,7 +593,7 @@ tk_dquote() { c2=${_s:_i+1:1} case $c2 in '$' | '`' | '"' | \\) _w+=$c2 ;; $'\n') ;; *) _w+=$c$c2 ;; esac _i=$((_i + 2)) ;; - '$') tk_dollar || return 1 ;; + '$') tk_dollar q || return 1 ;; '`') tk_backtick || return 1 ;; *) # Everything up to the next character with a meaning here, in one step: @@ -540,6 +606,8 @@ tk_dquote() { return 1 } +# tk_dollar [q]: at a "$"; q when the text is expanded as in double quotes (a +# double-quoted string, an unquoted heredoc's body, arithmetic). tk_dollar() { local rest part s0=${#_w} start=$_i _inw=1; _dyn=1 @@ -550,38 +618,48 @@ tk_dollar() { tk_paren || return 1 _insub=$((_insub - 1)); TK_SUBS+=("${_w:s0}") TK_CODE+=("${_s:start+2:_i-start-3}") ;; + '[') tk_arith || return 1 ;; '{') rest=${_s:_i} case $rest in *'}'*) ;; *) return 1 ;; esac part=${rest%%'}'*} # A $(…) in a default (`${x:-$(…)}`) runs: the text inside the braces is # queued as code, for its substitution to be found when it's tokenized. + # Expanded as in double quotes, a '…' in it is literal and its $(…) runs + # (`"${x:-'$(…)'}"`), so it's queued double-quoted. # shellcheck disable=SC2016 # a literal $( or backtick in ${…:-…} - case $part in *'$('* | *'`'*) TK_SUBS+=("$part}"); TK_CODE+=("${part:2}") ;; esac + case $part in + *'$('* | *'`'*) + TK_SUBS+=("$part}") + if [ -n "${1:-}" ]; then TK_CODE+=("\"${part:2}\""); else TK_CODE+=("${part:2}"); fi ;; + esac _w+="$part}"; _i=$((_i + ${#part} + 1)) ;; *) _w+='$'; _i=$((_i + 1)) ;; esac } -# tk_arith: at "$((" or "((" whose parens close as "))", consume the -# arithmetic into the word, so a "<<" in it is a shift, not a heredoc, and -# record the substitutions in it. Returns 1, consuming nothing, when the -# parens close otherwise (a subshell in a substitution or subshell, as bash -# reads it) or follow "<" or ">" (a process substitution). +# tk_arith: at "$((" or "((" whose parens close as "))", or at bash's older +# "$[ … ]", consume the arithmetic into the word, so a "<<" in it is a shift, +# not a heredoc, and record the substitutions in it. Returns 1, consuming +# nothing, when they don't close so (a "((" that is a subshell in a +# substitution or subshell, as bash reads it) or "((" follows "<" or ">" (a +# process substitution). tk_arith() { - local p=$_i from depth=0 c rest part - if [ "${_s:p:1}" = '$' ]; then + local p=$_i from end depth=0 o='(' x=')' c rest part + if [ "${_s:p:2}" = '$[' ]; then + o='[' x=']' + elif [ "${_s:p:1}" = '$' ]; then p=$((p + 1)) elif [ "$p" -gt 0 ]; then case ${_s:p-1:1} in '<' | '>') return 1 ;; esac fi - [ "${_s:p:2}" = '((' ] || return 1 + [ "$o" = '[' ] || [ "${_s:p:2}" = '((' ] || return 1 from=$((p + 2)) for ((p = from; p < _n; p++)); do c=${_s:p:1} case $c in - '(') depth=$((depth + 1)) ;; - ')') + "$o") depth=$((depth + 1)) ;; + "$x") [ "$depth" -gt 0 ] || break depth=$((depth - 1)) ;; \\) p=$((p + 1)) ;; @@ -591,8 +669,14 @@ tk_arith() { part=${rest%%"$c"*}; p=$((p + ${#part} + 1)) ;; esac done - [ "${_s:p:2}" = '))' ] || return 1 - _w+=${_s:_i:p+2-_i}; _inw=1; _dyn=1; _i=$((p + 2)) + if [ "$o" = '(' ]; then + [ "${_s:p:2}" = '))' ] || return 1 + end=$((p + 2)) + else + [ "$p" -lt "$_n" ] || return 1 + end=$((p + 1)) + fi + _w+=${_s:_i:end-_i}; _inw=1; _dyn=1; _i=$end tk_expand_subs "$from" "$p" || true # one that doesn't close: bash runs nothing } @@ -628,7 +712,7 @@ tk_expand_subs() { if ! tk_scan_subs; then tk_hd_restore _s=$ss; _n=$2; _i=$((off + _i)); _w="" - if [ "${_s:_i:1}" = '$' ]; then tk_dollar; else tk_backtick; fi || { rc=1; break; } + if [ "${_s:_i:1}" = '$' ]; then tk_dollar q; else tk_backtick; fi || { rc=1; break; } next=$_i fi tk_hd_restore ;; @@ -652,7 +736,7 @@ tk_scan_subs() { from=$_i; _w="" case ${_s:_i:1} in \\) _i=$((_i + 2)) ;; - '$') tk_dollar || { _i=$from; return 1; } ;; + '$') tk_dollar q || { _i=$from; return 1; } ;; *) tk_backtick || { _i=$from; return 1; } ;; esac done @@ -769,7 +853,7 @@ tk_heredoc_bodies() { done [ "$end" -le "$_n" ] || end=$_n if [ "${at[k]}" -ge 0 ]; then - TK_IN+=("${_s:start:end-start}"); TK_IN_AT+=("${at[k]}") + TK_IN+=("${_s:start:end-start}"); TK_IN_AT+=("${at[k]}"); TK_IN_DYN+=("$((1 - quoted[k]))") elif [ "$_insub" -gt 0 ]; then _w+=$'\002'${_s:start:end-start}$'\003' fi @@ -797,7 +881,7 @@ tokenize() { local LC_ALL=C c c2 rest line _s=$1; _n=${#1}; _i=0 _w=""; _inw=0; _dyn=0; _skip=0; _cmd0=0; _insub=0 - TK_VAL=(); TK_DYN=(); TK_OP=(); TK_IN=(); TK_IN_AT=(); TK_SUBS=(); TK_CODE=() + TK_VAL=(); TK_DYN=(); TK_OP=(); TK_IN=(); TK_IN_AT=(); TK_IN_DYN=(); TK_SUBS=(); TK_CODE=() _hd_delim=(); _hd_strip=(); _hd_at=(); _hd_quoted=() while [ "$_i" -lt "$_n" ]; do c=${_s:_i:1} @@ -1161,6 +1245,11 @@ push_re=${git_re}push$end_re # doesn't reach, where it can't tell a mention from a command. gh_cmd_re='(^|[^[:alnum:]_.-])gh[[:space:]]+((-R|--repo)(=|[[:space:]]*)[^[:space:]]+[[:space:]]+)*(pr|api)([^[:alnum:]_-]|$)' head_re=${git_re}'(commit|merge|rebase|reset|checkout|switch|cherry-pick|revert|am|pull)'$end_re +var_re='[$]([A-Za-z_][A-Za-z0-9_]*|[{])' # a $name or ${ +# A JSON key's value read from a body's text: a plain string, or the key alone. +# shellcheck disable=SC2016 +json_str_re='"[[:space:]]*:[[:space:]]*"([^"$`\\]*)"' +json_draft_re='"draft"[[:space:]]*:[[:space:]]*true' # shellcheck disable=SC2016 shell_re='(^|[[:space:];|&(`])(eval|([^[:space:];|&]*/)?(ba|z|da|k)?sh)([[:space:]<]|$)' cant_follow="this command runs \`git push\` in a form the gate can't follow (behind a wrapper such as timeout, sudo or env -C, in a command substitution, or in code handed to a shell or eval). Run \`git push\` directly, or as \`git -C push …\`." diff --git a/.claude/hooks/review-gate.test.sh b/.claude/hooks/review-gate.test.sh index 1a8823fb..f76776be 100755 --- a/.claude/hooks/review-gate.test.sh +++ b/.claude/hooks/review-gate.test.sh @@ -691,6 +691,13 @@ expect_block "a quoted gh pr ready past the 64 the gate follows is refused" "$re expect_block "a gh pr ready in a \${x:-\$(…)} default" "$repo" 'echo "${x:-$(gh pr ready 12)}"' "ready-for-review" # shellcheck disable=SC2016 expect_block "…unquoted" "$repo" 'echo ${x:-$(gh pr ready 12)}' "ready-for-review" +# Expanded as in double quotes, a '…' in a default is literal, so its $(…) runs. +expect_block "a gh pr ready in '…' in a double-quoted \${x:-…} default" "$repo" "echo \"\${x:-'\$(gh pr ready 12)'}\"" "ready-for-review" +expect_block "…in an unquoted heredoc body" "$repo" "cat </reviews` with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked. Through GraphQL, `createPullRequest` without `draft: true`, `markPullRequestReadyForReview`, and `addPullRequestReview` or `submitPullRequestReview` with `APPROVE` or `REQUEST_CHANGES` are blocked. It also blocks a merge through the API (`PUT …/pulls//merge`, `mergePullRequest`, `enablePullRequestAutoMerge`). `gh pr merge` itself is denied for Claude's commands in `.claude/settings.json` and left out of the gate, because a person merges with it from shell mode (`!`), which the Claude Code docs don't say whether this hook sees. The gate reads the commands a line runs, like the push check: a gh command only mentioned in a quoted string, a heredoc body or a comment doesn't count, unless a shell runs it: a `$(…)` where the shell expands one (a `${…}` default; arithmetic; the body of an unquoted heredoc, `< pr …` and `gh pr -R …` (or `--repo`) count as `gh pr …`, a shell given options before `-c` (`bash -euo pipefail -c '…'`) still hands its code over, and a GraphQL query computed by a heredoc inside `$(…)` is read from that heredoc, not from the rest of the line, unless the query's text has a `$` followed by a letter, `_` or `{`, which is a shell variable or a GraphQL `$var` (the gate can't tell them apart); that query is matched against the whole line too, where a shell variable may be set. Flags are read as gh reads them: each letter of a short-flag cluster is a flag (`-fr alice` is `--fill --reviewer alice`), and a flag's value is never read as a flag (`-b "-a thing"` is a body). It reads the command line only: a field or body gh reads from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command itself reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen. +The gate checks these rules in their other forms too. `gh pr new` is an alias of `gh pr create`. Through the REST API, `gh api …/pulls` that opens a PR must send a `draft=true` field (`-F draft=true` sends it as a boolean), and `…/pulls//reviews` with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked. Through GraphQL, `createPullRequest` without `draft: true`, `markPullRequestReadyForReview`, and `addPullRequestReview` or `submitPullRequestReview` with `APPROVE` or `REQUEST_CHANGES`, in the query or as the whole value of the event's own field (`event`, `…[event]`, or the variable each `event:` in the query takes), are blocked; a body that mentions `APPROVE` or is just the word, or a mutation such as `addPullRequestReviewThreadReply` that takes no event, isn't. A review event the shell computes (an `event` or `input[event]` field, an `"event"` key in a `--input -` body the shell expands, or a GraphQL `event: $e` whose variable no field gives literally) is refused, because the gate can't read it; such a body is read from its text, so it needn't be valid JSON until the shell expands it. An event built into a body any other way (the whole body from a variable or `$(jq …)`, or, in a `--input -` body, a computed `variables` entry for the query's escaped `\$e`) isn't followed. It also blocks a merge through the API (`PUT …/pulls//merge`, `mergePullRequest`, `enablePullRequestAutoMerge`). `gh pr merge` itself is denied for Claude's commands in `.claude/settings.json` and left out of the gate, because a person merges with it from shell mode (`!`), which the Claude Code docs don't say whether this hook sees. The gate reads the commands a line runs, like the push check: a gh command only mentioned in a quoted string, a heredoc body or a comment doesn't count, unless a shell runs it: a `$(…)` where the shell expands one (a `${…}` default, inside single quotes too when it is double-quoted, as in `"${x:-'$(…)'}"`; arithmetic, `$[ … ]` included; the body of an unquoted heredoc, `< pr …` and `gh pr -R …` (or `--repo`) count as `gh pr …`, a shell given options before `-c` (`bash -euo pipefail -c '…'`) still hands its code over, and a GraphQL query computed by a heredoc inside `$(…)` is read from that heredoc, not from the rest of the line, unless the query reads a shell variable: a `$name` or `${` that isn't a GraphQL variable the query declares (`$name: Type`). Such a query, sent as a field or from stdin (`<<< "$q"`, or an unquoted heredoc), is matched against the whole line too, where the variable may be set. Flags are read as gh reads them: each letter of a short-flag cluster is a flag (`-fr alice` is `--fill --reviewer alice`), and a flag's value is never read as a flag (`-b "-a thing"` is a body). It reads the command line only: a field or body gh reads from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command itself reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen. **PR title format.** The title becomes the squash-merge subject on `main` and is gated by the required `CI` check's `PR title` job — a bad title blocks merge, so don't discover it from CI. It must be Conventional Commits — `(optional-scope)(optional-!): ` — **≤ 72 chars**, subject **lowercase-first** with **no trailing period**. Types: `feat fix docs refactor test chore ci deps build perf revert style`. Validate before creating: `scripts/lint-pr-title.sh ""` (exit 0 = valid; it prints the reason on failure). `.claude/hooks/agent-bash-gate.sh` runs the same check on `gh pr create` / `gh pr new` / `gh pr edit --title`, and on the `title=` field of a `gh api` call that opens or edits a PR, so a malformed title is caught locally before the PR exists. The rule has a single source of truth — `scripts/lint-pr-title.sh` — called by this local gate, by the `PR title` job in `ci.yml` (from a trusted `main` checkout), and by `housekeeping.yml`'s advisory sticky-comment mirror, so local and CI never drift. Fixing a title after the fact needs no new push: editing it triggers housekeeping, which re-runs the failed `PR title` job (the job re-reads the title from the API). @@ -274,7 +274,7 @@ Today the list holds two reviewers; it's designed to grow (security is the obvio Each subagent's verdict is one of `ship_it`, `iterate`, or `block`. **`ship_it` requires zero findings at any severity** (`[MUST]`, `[SHOULD]`, `[MAY]` sections all empty). Anything in the findings list — including `[MAY]` — forces `iterate`. The rule is: if there's anything left to do, the PR isn't shippable. "Ship it, just do this one thing first" is iteration, not shipping. **Every** listed reviewer must reach `ship_it`, and you address findings from all of them (§Review Response) — not just the ones in whichever reviewer's lane you expected. -When a reviewer you ran ends with the parseable lines `REVIEWED: <sha>` (the commit it pinned before reading anything) and `VERDICT: ship_it`, `.claude/hooks/review-marker.sh` writes its marker — `tmp/<name>-passed-<sha>`, derived from the reviewer's name (so `pre-push-reviewer` → `tmp/pre-push-reviewer-passed-…`, `docs-reviewer` → `tmp/docs-reviewer-passed-…`) — into the worktree checked out at that commit. The marker attests to exactly the commit the reviewer read, so the hook writes none if that worktree's HEAD moved while the reviewer ran: **don't commit, check out or reset in a worktree while its reviewers run**. A report delivered through the subagent hand-back counts the same as one in the final message. Each decision, including why a `ship_it` wrote no marker, goes to `tmp/review-marker.log`, which the push gate prints when it blocks. A reviewer you **skip** instead earns the same marker via `scripts/skip-pre-push-review.sh <name> "<reason>"`, run in the worktree you're pushing, which also appends the reason to `tmp/review-skips-<HEAD>.log` (the push gate echoes these skips for the record). `git push` succeeds only when the commit each refspec publishes (its tip, or HEAD with no refspec) has a marker from **every** listed reviewer — run *or* skipped. The gate judges the worktree the push runs in (the shell's directory, a `cd` or `pushd` before it, `git -C`) and accepts a marker from any worktree of the repository. It blocks a push when it can't tell where it runs or what it publishes: a computed path or refspec, `GIT_DIR` or `GIT_WORK_TREE` set earlier on the line, `--all` or a glob refspec, a push behind a wrapper such as `timeout` or in a command substitution (one in the body of an unquoted heredoc included), or code handed to a shell (`bash -c`, `eval`, a heredoc or here-string read by a shell, inside a `$(…)` or not). It also blocks a push that follows a HEAD-moving git command on the same line, run directly or not (`git commit … && git push`, `out=$(git commit …) && git push`), because it runs before the line does and would judge the commit before the move: push in a separate command. The gate reads only the command line, so it catches the common accidental forms, not deliberate evasion. It doesn't follow a script that pushes, a shell behind a wrapper (`sudo bash -c`, `/usr/bin/env bash -c`) or one it doesn't know (`fish -c`), code piped into a shell, `git push --tags`, git run by path or through an alias, a `$(` split by a backslash-newline inside an unquoted heredoc body, or a `$(…)` inside single quotes within a double-quoted `${x:-…}` default such as `"${x:-'$(…)'}"` (the shell still runs it). A line it can't parse, such as one with an apostrophe in a comment inside `$(…)`, is refused: run the push as its own command. On `VERDICT: iterate` or `VERDICT: block`, that reviewer writes no marker — the orchestrator agent **loops**: address every finding, commit, re-invoke the reviewer(s) in fresh context, repeat until all say `ship_it`. Never push with open findings. +When a reviewer you ran ends with the parseable lines `REVIEWED: <sha>` (the commit it pinned before reading anything) and `VERDICT: ship_it`, `.claude/hooks/review-marker.sh` writes its marker — `tmp/<name>-passed-<sha>`, derived from the reviewer's name (so `pre-push-reviewer` → `tmp/pre-push-reviewer-passed-…`, `docs-reviewer` → `tmp/docs-reviewer-passed-…`) — into the worktree checked out at that commit. The marker attests to exactly the commit the reviewer read, so the hook writes none if that worktree's HEAD moved while the reviewer ran: **don't commit, check out or reset in a worktree while its reviewers run**. A report delivered through the subagent hand-back counts the same as one in the final message. Each decision, including why a `ship_it` wrote no marker, goes to `tmp/review-marker.log`, which the push gate prints when it blocks. A reviewer you **skip** instead earns the same marker via `scripts/skip-pre-push-review.sh <name> "<reason>"`, run in the worktree you're pushing, which also appends the reason to `tmp/review-skips-<HEAD>.log` (the push gate echoes these skips for the record). `git push` succeeds only when the commit each refspec publishes (its tip, or HEAD with no refspec) has a marker from **every** listed reviewer — run *or* skipped. The gate judges the worktree the push runs in (the shell's directory, a `cd` or `pushd` before it, `git -C`) and accepts a marker from any worktree of the repository. It blocks a push when it can't tell where it runs or what it publishes: a computed path or refspec, `GIT_DIR` or `GIT_WORK_TREE` set earlier on the line, `--all` or a glob refspec, a push behind a wrapper such as `timeout` or in a command substitution (one in the body of an unquoted heredoc included), or code handed to a shell (`bash -c`, `eval`, a heredoc or here-string read by a shell, inside a `$(…)` or not). It also blocks a push that follows a HEAD-moving git command on the same line, run directly or not (`git commit … && git push`, `out=$(git commit …) && git push`), because it runs before the line does and would judge the commit before the move: push in a separate command. The gate reads only the command line, so it catches the common accidental forms, not deliberate evasion. It doesn't follow a script that pushes, a shell behind a wrapper (`sudo bash -c`, `/usr/bin/env bash -c`) or one it doesn't know (`fish -c`), code piped into a shell, `git push --tags`, git run by path or through an alias, or a `$(` split by a backslash-newline inside an unquoted heredoc body. A line it can't parse, such as one with an apostrophe in a comment inside `$(…)`, is refused: run the push as its own command. On `VERDICT: iterate` or `VERDICT: block`, that reviewer writes no marker — the orchestrator agent **loops**: address every finding, commit, re-invoke the reviewer(s) in fresh context, repeat until all say `ship_it`. Never push with open findings. The orchestrator agent cannot override any subagent's system prompt (the fixed file content of `.claude/agents/<name>.md`), and each runs in a clean conversation context, so they don't share the orchestrator's bias toward its own work. diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b0b0add..9234347f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - **`TestClaims_DifferingExtrasLeaveConfiguredUnitsAlone` no longer fails when a member's view lags the split** (`internal/ingest/claims_test.go`): the test settled on the owner map alone, which could look split while one process had yet to read the last member and would give units up on its next tick, so the "nothing moves once settled" check saw that handover (7 of 400 runs locally). Settled now also means every process has read every member and three ticks pass with no claim event (0 of 400). - **The agent gate checks the alias and API forms of the PR actions it guards, and no longer blocks a command that only mentions one** (`.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/{review-gate,hook-commands}.test.sh`, `scripts/githooks.test.sh`, `AGENTS.md`, `docs/src/content/docs/claude-code.md`), closing [#790](https://github.com/Wave-RF/WaveHouse/issues/790). `agent-bash-gate.sh` matched only the `gh pr` spellings, so each guarded action went through in another form. `gh pr new`, an alias of `gh pr create`, skipped the `--draft` check and the title lint. Through the REST API, `gh api …/pulls/<N>/reviews -f event=APPROVE` approved a PR, `gh api …/pulls -f head=… -f title=…` opened a non-draft one, `gh api …/requested_reviewers -f 'reviewers[]=…'` requested a reviewer (the check wanted `-X POST` or a literal `reviewers=`, though a field already makes `gh api` POST), and `gh api -X PUT …/pulls/<N>/merge` merged. Through GraphQL, `markPullRequestReadyForReview` marked a draft ready. The gate now gives each of these the same answer as its `gh pr` form. A `gh api` call counts as a write when it names a method other than GET or, naming none, sends fields. Opening a PR (`…/pulls`) needs `draft=true` and a `title=` that passes the PR-title lint, as does a retitle (`PATCH …/pulls/<N>`). A review with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked. A write to `requested_reviewers`, `DELETE` included, is blocked. A merge (`PUT …/pulls/<N>/merge`, or GraphQL `mergePullRequest` or `enablePullRequestAutoMerge`) is blocked. In GraphQL, so are `markPullRequestReadyForReview`, `requestReviews`, an approving or changes-requesting `addPullRequestReview` or `submitPullRequestReview`, and a `createPullRequest` without `draft: true`. `gh pr merge` itself stays with the `deny` rule in `.claude/settings.json` and out of the gate, because a person merges with it from shell mode (`!`), and the Claude Code docs don't say whether a PreToolUse hook sees those commands. The `gh` checks used to read the line with its quoted strings removed and everything else kept, so they blocked a command that only mentioned a guarded one in a heredoc body or a comment (a `python3 - <<'EOF'` script with `# gh pr ready` in it), and missed one in a `$(…)` or handed to `bash -c`. They now read the commands the line runs through the gate's tokenizer, the view the push check already uses: a mention in a quoted string, heredoc body or comment doesn't count, while the code a shell runs, a `$(…)`, `bash -c '…'` (or `-lc` and the like), `eval`, or a heredoc a shell reads directly or through a pipe, is tokenized and judged the same way, read as written, quotes and heredoc bodies included, so a heredoc a shell reads inside a `$(…)` counts too. Each `gh` command is judged on its own words. A `gh api` call is judged by its endpoint, its first argument that isn't a flag, and by its fields, so a comment or review-thread reply whose body names an endpoint, a field or a mutation, or another call on the same line, doesn't count: `draft=`, `title=` and `event=` are read only from `-f`, `-F`, `--field` or `--raw-field` values, and GraphQL mutation names only from the `query=` field. A boolean flag set with `=` is read as gh reads it, so `--draft=true` counts as `--draft` and `--approve=false` doesn't approve. A line the tokenizer can't parse is refused when it mentions a `gh pr` or `gh api` command, since what it runs can't be told, and so is such a command left beyond the 64 pieces of nested code the gate follows. `gh -R <repo> pr …` (or `--repo`, before the command) counts as `gh pr …`; a shell given options before `-c` (`bash -euo pipefail -c '…'`, `bash --noprofile --norc -c '…'`) still hands its code over; and a GraphQL `query=` computed by a heredoc inside `$(…)` is matched against that heredoc. `gh pr create` and `gh pr new` are blocked when they name `--reviewer` or `--assignee` (or `-r`, `-a`), as `gh pr edit --add-reviewer` is. The `--title` of `gh pr create` and `gh pr edit` is now read from the tokens, so an unquoted title is linted too. The tokenizer now steps over plain text in a double-quoted string in one move rather than a character at a time: measured on an earlier revision of this change (a 100 KB inline `--body "…"`: 11 s with the old character loop, under 1 s after); not re-measured after the per-call rewrite. The gate still reads only the command line: fields sent through `--input` or `-F field=@file` go unseen. `test-review-gate`, `test-hook-commands` and `test-githooks` run the hooks a second time with `/bin/bash` first on `PATH` when that is a different bash, as macOS's bash 3.2 is, so a construct bash 3.2 rejects fails a test rather than leaving the gate to exit 1, which Claude Code doesn't treat as a block. - **The agent gate judges the `gh` and push forms it still missed, and a `<<` in arithmetic no longer hides the lines after it** (`.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/review-gate.test.sh`, `AGENTS.md`, `docs/src/content/docs/claude-code.md`), closing [#797](https://github.com/Wave-RF/WaveHouse/issues/797). The gate's tokenizer read the `<<` of an arithmetic shift (`n=$(( 1 << 3 ))`, `(( n <<= 1 ))`) as a heredoc, whose delimiter then swallowed every later line, so a `gh pr ready` or a `git push` on the next line went unjudged; the regex gate that [#790](https://github.com/Wave-RF/WaveHouse/issues/790)'s rework replaced had blocked the `gh` form. Arithmetic is now read as arithmetic, and a `$(…)` inside it is judged. The push check now follows a heredoc a shell reads inside `$(…)` (`out=$(bash <<'EOF'` … `EOF` `)`), as the `gh` checks already did: a heredoc body inside a `$(…)` is kept with the substitution's text as a quoted part, an argument unless the substitution hands it to a shell. The body of an unquoted heredoc (`<<EOF`, not `<<'EOF'`) runs its `$(…)` and backticks, and both checks now judge them, a backtick code span in a PR body included; a quoted heredoc's body stays text. Nested escaped backticks (`` \` `` inside a backtick substitution) are followed too: there a backslash quotes only `$`, a backtick and a backslash, so the code that runs has a backtick. `gh pr -R <repo> <subcommand>` (or `--repo`) counts as `gh pr <subcommand>`; gh accepts the flag there (measured with `gh pr --repo cli/cli list`, gh 2.102.0). A `$(…)` in a `${…}` default (`${x:-$(…)}`) is judged, and a `gh pr` or `gh api` command left beyond the 64 pieces of nested code the gate follows is refused when it's quoted too (`bash -c 'gh pr ready 12'`). A field `gh api` reads from stdin, `-F key=@-` or `--input -`, is read from the heredoc or here-string the command itself reads: a GraphQL body is matched as a query is, and a REST body's `draft`, `event` and `title` are read from its JSON. A raw field (`-f`) that starts with `@` is now read as the literal value gh sends. A computed GraphQL `query=` is matched against its own code, heredoc bodies included, rather than the whole line, so another field's text naming a mutation no longer blocks the call; a query that reads a variable is still matched against the whole line, where the variable may be set. `gh pr` flags are read as gh's parser reads them: each letter of a short-flag cluster is a flag, so `-fr alice` and `-fa alice` on `gh pr create` are blocked, `-df` counts as `--draft` and `gh pr review -ab "lgtm"` as an approval; and a flag's value is never read as a flag, so `-b "-a thing"` is a body and `--title "-rx"` a title, not a reviewer, and `gh pr review --comment -b "--approve"` is a comment. An unquoted heredoc's body is scanned a line at a time, each line as a string of its own, because bash measures the whole string on every `${s:i:n}`: tokenizing a 100 KB unquoted body with 1,800 backtick spans takes 0.7 s, against 0.4–0.6 s for the same body quoted and 2.2–2.9 s with a character loop over it (measured, bash 5.3 and 3.2). A `gh pr` or `gh api` command left past the 64 pieces is looked for in one `grep` rather than one per piece. A heredoc opened inside a `$(…)` in an unquoted heredoc body no longer leaves its delimiter pending, which had swallowed every later line as a body (`cat <<EOF` / `$(cat <<X)` / `EOF`, then a `gh pr ready` or a `git push`, was allowed) or refused a valid nested heredoc as unparseable; the heredoc's body can only be inside the text being expanded. The push check's cheap filter joined backslash-newlines with a pattern bash 3.2 misread inside `$(…)`, dropping text and taking about 105 s on a 60 KB command (a hook timeout does not block, so the check was skipped); it now takes under a second (measured, `/bin/bash` 3.2.57, 60 KB of this changelog). +- **The agent gate judges `$[ … ]` arithmetic, a variable GraphQL query sent on stdin and a single-quoted `$(…)` in a double-quoted default, and no longer refuses a query with GraphQL variables or a reply that mentions `APPROVE`** (`.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/review-gate.test.sh`, `AGENTS.md`, `docs/src/content/docs/claude-code.md`), closing [#809](https://github.com/Wave-RF/WaveHouse/issues/809). The tokenizer read the `<<` in bash's older arithmetic syntax, `x=$[1<<3]`, as a heredoc whose delimiter swallowed every later line, so a `gh pr ready` or a `git push` on the next line went unjudged; `$[ … ]` is now read as arithmetic, like `$(( … ))`, and a `$(…)` in it is judged. A GraphQL query that reads a shell variable was matched against the whole line only when sent as a field (`-f query="$q"`); one sent on stdin from a here-string or an unquoted heredoc (`-F query=@- <<< "$q"`, `--input - <<< "$b"`) now is too, and the line it is matched against is the whole command, so a variable set outside the `$(…)` the call runs in (`q=…; resp=$(gh api graphql -f query="$q")`) counts. Inside double quotes, and in an unquoted heredoc's body, a `'…'` in a `${x:-…}` default is literal and bash runs the `$(…)` in it (`"${x:-'$(gh pr ready 12)'}"`, measured, bash 5.3.20 and 3.2.57); the gate read the default as unquoted code, where `'…'` quotes it, and now reads it as double-quoted, so that form leaves the documented limits. A query's GraphQL variables (`mutation($id: ID!) { … $id … }`) were taken for shell variables, which matched the query against the whole line, where another field's text naming a mutation refused it; a `$name` the query declares (`$name: Type`) no longer counts as a shell variable. `addPullRequestReview` matched `addPullRequestReviewThreadReply` as a substring, and any field whose value contained `APPROVE` counted as the review's event, so a GraphQL thread reply, or a `COMMENT` review whose body (`-F body=@-`) mentioned `APPROVE`, was refused; the event now counts in the query or as the whole value of the event's own field (`event`, `…[event]`, or the variable each `event:` in the query takes), so a body that is the word `APPROVE` or `COMMENT` is a body, and only `addPullRequestReview` itself and `submitPullRequestReview` are checked for it. A review event the shell computes (`-f event="$ev"`, `-f "input[event]=$ev"`, an `"event": "$ev"` in a `--input -` body the shell expands, or a GraphQL `event: $e` whose variable no field gives literally) went unread and passed; it is now refused, while a literal event beside a computed body still passes. A `--input -` body the shell expands is read from its text, so in one that is JSON only once expanded (`"body": $(jq -Rs . notes.md)`) a literal `"event": "APPROVE"` is now seen and a draft PR is no longer refused as not a draft. An event built into a body any other way (the whole body from a variable or `$(jq …)`, or, in a `--input -` body, a computed `variables` entry for the query's escaped `\$e`) isn't followed. The 60 KB timed cases are unchanged, 0.07–0.12 s a call before and after (measured, `/bin/bash` 3.2.57 and bash 5.3.20). - **The Claude Code hooks no longer fail open when their scripts are missing** (`.claude/settings.json`, `.claude/hooks/hook-commands.test.sh` (new), `Makefile`, `AGENTS.md`, `docs/src/content/docs/{claude-code,development}.md`), closing [#787](https://github.com/Wave-RF/WaveHouse/issues/787): each hook ran `$CLAUDE_PROJECT_DIR/.claude/hooks/<script>.sh`, and `CLAUDE_PROJECT_DIR` is, in the Claude Code docs' words, ["the project root where the session started"](https://code.claude.com/docs/en/hooks#reference-scripts-by-path). Once that checkout was removed, or a script went missing or lost its executable bit, the shell exited 127 or 126, which Claude Code treats as a non-blocking error, so `agent-bash-gate.sh` stopped gating pushes and PR changes and `review-marker.sh` stopped writing markers, and nothing said so. Each command is now a short guard that `exec`s its script, unchanged, when the script is there and executable, and otherwise acts by event. The PreToolUse guard blocks (exit 2) any command that contains the words `git` and `push`; `gh` and `pr` with one of `create`, `new`, `edit`, `ready`, `review` or `merge`; or `gh` and `api` with `pulls` or `graphql`, which covers every call to the pull-request or GraphQL API, reads included. It also blocks every command when there is no `jq` to read it with. Other commands go through, so the session can still work. The SubagentStop guard reports the missing script with exit 1, since exit 2 there keeps the subagent running, and the push gate then finds no marker. The PostToolUse guards report it with exit 2, which shows Claude the message after an edit that has already happened. Each message says to restart Claude Code from a live checkout. The match is crude on purpose, a guard against accidents like the gate it stands in for. A project directory whose path has a space in it now works too: the bare path was split. `make verify` gains `test-hook-commands`, which reads every command from the real `settings.json` and runs it under `sh -c` (and `dash` and `bash` when present) with its script present, missing and not executable. - **A worktree whose branch has no Makefile can commit, and the git hooks say when `scripts/ci-marker.sh` is missing** (`.githooks/{pre-commit,pre-push}`, `scripts/githooks.test.sh` (new), `Makefile`, `AGENTS.md`, `docs/src/content/docs/{claude-code,development}.md`), closing [#678](https://github.com/Wave-RF/WaveHouse/issues/678): with an absolute `core.hooksPath`, git runs one checkout's `.githooks/` in every worktree of the repository, including one on an orphan branch that keeps a tool's state and no code. There `pre-commit` couldn't find `scripts/ci-marker.sh`, hid the error behind `2>/dev/null`, and ran `make verify` with no Makefile, which failed with `No rule to make target 'verify'`, so nothing could be committed. A worktree with no Makefile now has nothing to verify and commits; a missing or failing `scripts/ci-marker.sh` is reported instead of hidden, and `make verify` runs as before. `pre-push` blocked the same branches with a misleading "can't resolve tree"; it now lets through a pushed commit that has no Makefile, which holds no code to validate, and blocks one that has a Makefile but is pushed from a worktree without `scripts/ci-marker.sh`, saying so. The relative `.githooks` that `make tools` sets is resolved in each worktree, so there a worktree whose branch has no `.githooks/` runs no hooks at all; the hooks now behave correctly under either setting. `make verify` gains `test-githooks`, which commits and pushes through the real hooks from scratch worktrees with and without code, and `make lint-sh` now shellchecks `.githooks/`. - **`scripts/skip-pre-push-review.sh` no longer refuses, now and then, to skip the first reviewer the manifest lists** (`scripts/skip-pre-push-review.sh`, `.claude/hooks/review-gate.test.sh`), closing [#767](https://github.com/Wave-RF/WaveHouse/issues/767): it checked the name with `bash scripts/pre-push-reviewers.sh | grep -Fxq` under `pipefail`. Bash writes each name in its own `write()`, so `grep -q` could match `pre-push-reviewer` on the first line and exit before the second write, which then died of `SIGPIPE`; `pipefail` reported that as no match, and the script refused with "'pre-push-reviewer' is not a reviewer". Whether it happened was up to the scheduler, so `make test-review-gate` failed intermittently in CI, more often on a busy runner, and its log showed only that no marker was written, because the test discarded the script's stderr. The script now reads the whole list before matching, and does the same for its advisory `git diff --name-only | grep -qv` check; that check also resolves its base the way the push gate does (`origin/main`, else `main`), which its comment already claimed. The test now prints the script's stderr when a skip fails, and adds a case whose manifest pauses after each name, which the old script failed every time. diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index 67a0f233..aa55fe3e 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -52,7 +52,7 @@ Both markers are tree-keyed so commit-then-push works without a re-run when the | `.claude/settings.json` | Team-wide: `deny` permissions (force-push / git reset --hard / filter-branch / update-ref -d, gh pr merge / ready / approve / request-changes, gh repo/release delete, gh secret delete, gh workflow disable, rm -rf / sudo rm), all four hooks wired, each through a guard that runs its script when it's there and fails closed when it isn't (see [When a hook script is missing](#when-a-hook-script-is-missing)) | | `.claude/hooks/gofumpt-on-save.sh` | PostToolUse Edit/Write/MultiEdit: auto-formats `.go` files | | `.claude/hooks/markdown-on-save.sh` | PostToolUse Edit/Write/MultiEdit: repairs MDX fence structure in `.mdx`, applies markdownlint `--fix` (WH001 unwrapping) in `.md` only, and runs misspell over docs prose in both | -| `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: catches accidental Agent PR Discipline violations (drafts only, no human reviewer adds, a marker (from a review or a logged skip) from every reviewer in `scripts/pre-push-reviewers.sh` required for the commit each refspec publishes (its tip, or HEAD with no refspec) when `origin/main` (local `main` if there's no `origin/main`) doesn't have it yet; PR title linted via `scripts/lint-pr-title.sh` on `gh pr create` / `gh pr new` / `gh pr edit --title`; the `gh api` REST and GraphQL forms of these, and of a merge, checked too, reading the command line only: a field or body read from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen). A gh command only mentioned in a quoted string, a heredoc body or a comment doesn't count, unless a shell runs it; a `$(…)` counts where the shell expands one (a `${…}` default, arithmetic and the body of an unquoted heredoc included). A push is judged where it runs (the tool call's `cwd`, a `cd` or `pushd` before it, `git -C`), and a marker counts from any worktree of the repository. It blocks a push it can't follow (a computed path or refspec, `GIT_DIR` or `GIT_WORK_TREE` set earlier on the line, `--all` or a glob refspec, a push behind a wrapper or in a command substitution, one in an unquoted heredoc's body included, code handed to `bash -c`, `eval` or a heredoc or here-string read by a shell, inside a `$(…)` or not) and one that follows a HEAD-moving git command on the same line, run directly or not. It reads only the command line, so it catches the common accidental forms, not deliberate evasion; it doesn't follow a script that pushes, a shell behind a wrapper or one it doesn't know (`fish -c`), code piped into a shell, `git push --tags`, git run by path or through an alias, a `$(` split by a backslash-newline inside an unquoted heredoc body, or a `$(…)` inside single quotes within a double-quoted `${x:-…}` default (the shell still runs it). A line it can't parse, such as one with an apostrophe in a comment inside `$(…)`, is refused when it mentions a push or a `gh pr` / `gh api` command. It follows 64 pieces of code a shell runs; a `gh pr` / `gh api` command left beyond them, quoted or not, is refused unjudged | +| `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: catches accidental Agent PR Discipline violations (drafts only, no human reviewer adds, a marker (from a review or a logged skip) from every reviewer in `scripts/pre-push-reviewers.sh` required for the commit each refspec publishes (its tip, or HEAD with no refspec) when `origin/main` (local `main` if there's no `origin/main`) doesn't have it yet; PR title linted via `scripts/lint-pr-title.sh` on `gh pr create` / `gh pr new` / `gh pr edit --title`; the `gh api` REST and GraphQL forms of these, and of a merge, checked too, reading the command line only: a field or body read from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen). A gh command only mentioned in a quoted string, a heredoc body or a comment doesn't count, unless a shell runs it; a `$(…)` counts where the shell expands one (a `${…}` default, inside single quotes too when it is double-quoted; arithmetic, `$[ … ]` included; the body of an unquoted heredoc). A push is judged where it runs (the tool call's `cwd`, a `cd` or `pushd` before it, `git -C`), and a marker counts from any worktree of the repository. It blocks a push it can't follow (a computed path or refspec, `GIT_DIR` or `GIT_WORK_TREE` set earlier on the line, `--all` or a glob refspec, a push behind a wrapper or in a command substitution, one in an unquoted heredoc's body included, code handed to `bash -c`, `eval` or a heredoc or here-string read by a shell, inside a `$(…)` or not) and one that follows a HEAD-moving git command on the same line, run directly or not. It reads only the command line, so it catches the common accidental forms, not deliberate evasion; it doesn't follow a script that pushes, a shell behind a wrapper or one it doesn't know (`fish -c`), code piped into a shell, `git push --tags`, git run by path or through an alias, or a `$(` split by a backslash-newline inside an unquoted heredoc body. A line it can't parse, such as one with an apostrophe in a comment inside `$(…)`, is refused when it mentions a push or a `gh pr` / `gh api` command. It follows 64 pieces of code a shell runs; a `gh pr` / `gh api` command left beyond them, quoted or not, is refused unjudged | | `.claude/hooks/review-marker.sh` | SubagentStop: on a reviewer's `VERDICT: ship_it`, writes its `tmp/<name>-passed-<sha>` marker for the commit named on the report's `REVIEWED:` line, into the worktree on that commit, and only if that worktree's HEAD stayed on it for the whole review (its reflog against the subagent transcript's first timestamp). The reviewer set comes from `scripts/pre-push-reviewers.sh`; filters by `agent_type` in-script. Reads the report from `.last_assistant_message`, or, when that has no `VERDICT:` line or says `ship_it` without `REVIEWED:`, from the hand-back message in the subagent's transcript; a final `iterate` or `block` stands. An abbreviated sha counts if it names exactly one commit. Logs every decision to `tmp/review-marker.log` | | `.claude/hooks/review-gate.test.sh` | Test for the two review hooks: feeds them synthetic events in a scratch repository with sibling worktrees (`make test-review-gate`, part of `make verify`) | | `.claude/hooks/hook-commands.test.sh` | Test for the hook commands in `.claude/settings.json`: runs each one under `sh -c` as Claude Code does, with its script present, missing and not executable (`make test-hook-commands`, part of `make verify`) | @@ -107,7 +107,7 @@ To add a skill: create `.claude/skills/<name>/SKILL.md` with frontmatter `name` Agents (Claude Code etc.) have additional gating beyond what humans face — enforced by `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) + deny rules in `.claude/settings.json`. Humans keep full git/gh affordances; agents have these extra constraints: - **Drafts only.** `gh pr create` (or its alias `gh pr new`) must include `--draft`. Only humans transition draft → ready (`gh pr ready` is blocked), approve (`gh pr review --approve` is blocked), or request changes (`gh pr review --request-changes` is blocked). The gate also lints the PR title on `gh pr create` / `gh pr new` / `gh pr edit --title` via `scripts/lint-pr-title.sh` — the same rule as the required `CI` check's `PR title` job, so a malformed title is caught before the PR exists (fail-open when the title is missing or computed). -- **The API forms count too.** Through `gh api`, opening a PR (`…/pulls`) needs a `draft=true` field (`-F draft=true` sends it as a boolean) and a title that passes the lint, and a review with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked. Through GraphQL, `createPullRequest` needs `draft: true`, and `markPullRequestReadyForReview`, an approving or changes-requesting `addPullRequestReview` / `submitPullRequestReview`, and `requestReviews` are blocked. A merge through the API (`PUT …/pulls/<N>/merge`, `mergePullRequest`, `enablePullRequestAutoMerge`) is blocked. `gh pr merge` itself is left to the `deny` rule in `.claude/settings.json`, because a person merges with it from shell mode (`!`). Each `gh api` call is judged by its own endpoint (its first argument that isn't a flag) and its own fields, so a comment or review-thread reply whose body names an endpoint doesn't count, and neither does another call on the same line. Code a shell runs (a `$(…)` where the shell expands one, the body of an unquoted heredoc included, `bash -c '…'` with or without options before `-c`, `eval`, a heredoc a shell reads) is judged the same way, except the forms the `agent-bash-gate.sh` row above says it doesn't follow, and so is a GraphQL query computed by a heredoc inside `$(…)`. `gh -R <repo> pr …` and `gh pr -R <repo> …` (or `--repo`) count as `gh pr …`. Flags are read as gh reads them, so a short-flag cluster counts (`-fr alice` requests a reviewer) and a flag's value doesn't (`-b "-a thing"` is a body). The gate reads the command line only: a field or body gh reads from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command itself reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen. +- **The API forms count too.** Through `gh api`, opening a PR (`…/pulls`) needs a `draft=true` field (`-F draft=true` sends it as a boolean) and a title that passes the lint, and a review with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked. Through GraphQL, `createPullRequest` needs `draft: true`, and `markPullRequestReadyForReview`, an approving or changes-requesting `addPullRequestReview` / `submitPullRequestReview` (the event in the query, or the whole value of the event's own field: `event`, `…[event]`, or the variable each `event:` in the query takes), and `requestReviews` are blocked; a body that mentions `APPROVE` or is just the word, or a thread reply (`addPullRequestReviewThreadReply`), isn't. A review event the shell computes (an `event` or `input[event]` field, an `"event"` key in a `--input -` body the shell expands, or a GraphQL `event: $e` whose variable no field gives literally) is refused, because the gate can't read it; such a body is read from its text, so it needn't be valid JSON until the shell expands it. An event built into a body any other way (the whole body from a variable or `$(jq …)`, or, in a `--input -` body, a computed `variables` entry for the query's escaped `\$e`) isn't followed. A merge through the API (`PUT …/pulls/<N>/merge`, `mergePullRequest`, `enablePullRequestAutoMerge`) is blocked. `gh pr merge` itself is left to the `deny` rule in `.claude/settings.json`, because a person merges with it from shell mode (`!`). Each `gh api` call is judged by its own endpoint (its first argument that isn't a flag) and its own fields, so a comment or review-thread reply whose body names an endpoint doesn't count, and neither does another call on the same line. Code a shell runs (a `$(…)` where the shell expands one, the body of an unquoted heredoc included, `bash -c '…'` with or without options before `-c`, `eval`, a heredoc a shell reads) is judged the same way, except the forms the `agent-bash-gate.sh` row above says it doesn't follow, and so is a GraphQL query computed by a heredoc inside `$(…)`. A query that reads a shell variable (a `$name` or `${` that isn't a GraphQL variable the query declares, `$name: Type`), sent as a field or from stdin, is matched against the whole line, where the variable may be set. `gh -R <repo> pr …` and `gh pr -R <repo> …` (or `--repo`) count as `gh pr …`. Flags are read as gh reads them, so a short-flag cluster counts (`-fr alice` requests a reviewer) and a flag's value doesn't (`-b "-a thing"` is a body). The gate reads the command line only: a field or body gh reads from stdin (`-F field=@-`, `--input -`) is read from a heredoc or here-string the `gh` command itself reads, and one read from a file (`-F field=@file`, `--input file`) or piped in goes unseen. - **No human reviewer assignment.** `gh pr edit --add-reviewer / --add-assignee`, `--reviewer` / `--assignee` (or `-r` / `-a`, alone or in a short-flag cluster such as `-fr`) on `gh pr create` / `gh pr new`, and any `gh api` write to `…/requested_reviewers` are blocked. GitHub assigns reviewers natively — the `required_reviewers` ruleset rule requests the `@Wave-RF/wavehouse-admins` team and the team's code-review assignment picks the member; humans handle the rest. - **Bot re-triggers via comments.** Agents CAN mention bots in PR comments to re-trigger reviews — `@coderabbitai review`, etc. This goes through `gh pr comment` (allowed), not the reviewer API. - **Pre-push review required on PR branches.** Before `git push` to a branch with commits ahead of `main`, the agent runs the reviewers in `scripts/pre-push-reviewers.sh` (today: `pre-push-reviewer` for code, `docs-reviewer` for docs prose + code↔docs sync) that the change needs — in fresh context, in parallel — and **skips** any with nothing to do via `scripts/skip-pre-push-review.sh <name> "<reason>"` (a logged skip that satisfies the marker, so a docs typo doesn't pay for a full code review). `/prepush` does all of this. `ship_it` requires zero findings at any severity — any `[MUST]` / `[SHOULD]` / `[MAY]` forces iterate; fix and re-invoke (always fresh context) until clean. The push gate requires a marker — from a `ship_it` or a logged skip — from every listed reviewer for the commit each refspec publishes (its tip, or HEAD with no refspec), judged in the worktree the push runs in, and echoes the skips at push time. A `ship_it` marks only the commit the reviewer read, so don't commit in a worktree while its reviewers run.