From 75221a29a462f66fecde29a0cb8401b1dd825d7d Mon Sep 17 00:00:00 2001 From: Thomas Luizon Rodrigues Gregorio Date: Fri, 24 Jul 2026 16:55:55 -0300 Subject: [PATCH 1/3] chore(lessons): promote both pending lessons, one as a gate and one as a rule Clears `.claude/pending-lessons.md` before Phase 7. Lesson 1 (checkable) becomes a gate in the merge sweeps. `gh pr update-branch` rewrites the head SHA and re-triggers the `review` check, but `reviewDecision` is PR-level and keeps the pre-update APPROVED while that re-review runs, so both sweeps could squash-merge on a snapshot that predated the head they were merging. That is how a HIGH backend-contract finding reached main on orbit-api#403 and the fix landed on the orphaned head branch instead. Both scripts now: - discover whether the repo runs `.github/workflows/claude-review.yml`, and if it does, refuse every merge path until the `review` check on the CURRENT head SHA is terminal, then re-read reviewDecision; - record each merged PR's head branch and re-check it at end of sweep, printing ORPHANED-HEAD and exiting 1 when a post-merge push re-created it; - gain `--help`, documented exit codes (0/1/2), and a timeout line that names the actual blocker instead of a generic "waiting for CLEAN". `merge-sweep.sh` carried the same race in a stronger form (it never waited for checks at all), so it is fixed in the same pass rather than left as known-broken. Lesson 2 (judgment) becomes standing guidance in the orchestrate skill's delegation-discipline section, which is the one place in the repo that carries a delegation contract template and a wait-on-CI loop. Both halves land: subagents poll in the foreground inside their own turn, and the parent treats a "standing by / monitor armed" completion as a nudge trigger, not as progress. --- .claude/pending-lessons.md | 11 +- .claude/skills/orchestrate/SKILL.md | 16 +++ tools/README.md | 4 +- tools/dash-baseline.json | 4 +- tools/merge-sweep-cov.sh | 152 ++++++++++++++++++++++++---- tools/merge-sweep.sh | 138 +++++++++++++++++++++---- 6 files changed, 277 insertions(+), 48 deletions(-) diff --git a/.claude/pending-lessons.md b/.claude/pending-lessons.md index ae9878480..092a5b56e 100644 --- a/.claude/pending-lessons.md +++ b/.claude/pending-lessons.md @@ -1,15 +1,12 @@ -# Pending lessons (staging — not loaded into context) +# Pending lessons (staging, not loaded into context) Reviewed and promoted via `/lesson`. Delete each entry once promoted to a rule/hook or dropped. -## 2026-07-14 — sweep-merge can race a re-triggered review on a BEHIND PR → merges past CHANGES_REQUESTED -- Trigger: `tools/merge-sweep-cov.sh` merging any PR that is BEHIND main (require-up-to-date) and therefore needs an update-branch. Hit once on orbit-api #403 (a HIGH backend-contract finding shipped to main + deployed before the re-review landed; the fix went to the orphaned head branch, not main). -- Type: checkable (the sweep script can enforce this deterministically). -- Proposed home: a guard inside `tools/merge-sweep-cov.sh` — after its update-branch step, re-poll `gh pr view --json reviewDecision` until the re-triggered `review` check reaches a terminal state, and BLOCK the merge unless it re-settles to APPROVED (never merge on the pre-update APPROVED snapshot). Secondary signal to detect a past occurrence: the PR's head branch survives deletion (a post-merge push re-created it) = an orphaned fix that never reached main — scan for surviving head branches after a sweep. -- Draft: In the sweep, sequence = update-branch → wait-for-checks-terminal (INCLUDING `review`) → re-read reviewDecision → if APPROVED and required checks green (or coverage-only), merge; else abort + report. Do not read reviewDecision once before the update-branch and reuse it. -- Interim operational guard (until promoted): for every BEHIND-PR sweep tonight, after merge re-check `reviewDecision` + whether the head branch still exists; if flipped/orphaned, fix-forward onto main. +Queue is empty. ## Graduated - 2026-07-08 "don't offer optional next-steps" + 2026-07-09 proactivity failures (assume/ask/optional/improvise) → merged as one class and graduated to the global **proactivity guard** (`~/.claude/hooks/proactivity-reminder.mjs` UserPromptSubmit re-injection + `~/.claude/hooks/proactivity-guard.mjs` Stop class-gate). See `project_proactivity_guard` memory. Cleared 2026-07-09. - 2026-07-14 "opencode + Zen" is the opencode Zen gateway, NOT Z.ai → promoted to the `feedback_opencode_zen_not_zai` memory and to the `OpenCode Go plus Zen over OpenRouter` ADR in the brain vault, which carries the full naming trap + the pricing rationale. Cleared 2026-07-16. +- 2026-07-14 sweep-merge races a re-triggered review on a BEHIND PR and merges past CHANGES_REQUESTED (orbit-api #403) → graduated to a **gate**, not prose: `tools/merge-sweep-cov.sh` and `tools/merge-sweep.sh` now block every merge path until the `review` check on the CURRENT head SHA settles, re-read `reviewDecision` after it does, and scan merged PRs' head branches at end of sweep, exiting 1 on a re-created (orphaned) branch. Cleared 2026-07-24. +- 2026-07-24 background subagents idle on phantom "background waiters" while babysitting CI → promoted to `.claude/skills/orchestrate/SKILL.md`, "Delegation discipline → Waiting is foreground work, on both sides": the subagent-side foreground-poll contract plus the parent-side rule that "standing by" is not progress. Cleared 2026-07-24. diff --git a/.claude/skills/orchestrate/SKILL.md b/.claude/skills/orchestrate/SKILL.md index a516a0f48..25592d3dc 100644 --- a/.claude/skills/orchestrate/SKILL.md +++ b/.claude/skills/orchestrate/SKILL.md @@ -92,3 +92,19 @@ landed clean. So: (SendMessage), never into the main session. - The main session keeps only: decisions, small verification reads, user checkpoints, and cross-repo sequencing. + +### Waiting is foreground work, on both sides + +A stopped agent receives no notifications, so a background waiter it armed can never wake +it: ending a turn with the goal unmet records the task as idle, and only a human nudge +restarts it. Measured 2026-07-24: three agents in one day (the ui merge-chain, the Phase 3 +deletion, the orchestrate-skill fix) each ended their turn on "the monitor will notify me" +with no live background child. Two of the three prompts already carried a warning against +exactly that, so the subagent-side half alone does not hold. Both halves are the rule: + +- **In a prompt whose task includes waiting on CI or a review:** poll in the FOREGROUND, + sleep 60 to 120s per loop, inside your own turn. End the turn only on the goal state or a + genuinely unfixable blocker, and say which one. +- **On any completion notification whose result reads "waiting", "standing by", or "monitor + armed":** read the real PR/CI state yourself and send the agent back to work with it. + Standing by is not progress. diff --git a/tools/README.md b/tools/README.md index cf38a4e8e..014ce3e37 100644 --- a/tools/README.md +++ b/tools/README.md @@ -15,8 +15,8 @@ Read `CONVENTIONS.md` before adding one. Use the `/make-tool` skill to scaffold | Tool | What it does | Usage | |---|---|---| | `agent-review.sh` / `agent-review.ps1` | Cross-model second opinion (GLM-5.2 via opencode) on one claim or review finding. Thin wrapper over `.claude/skills/second-opinion/second-opinion.mjs`; prints one line of JSON (`AGREE` / `DISAGREE` / `UNSURE`, or a graceful `UNAVAILABLE`). | `agent-review --claim ""` or `agent-review < dossier.txt`; `--help` for options | -| `merge-sweep.sh` | Require-up-to-date server-side merge sweep: per PR, update-branch then poll `mergeStateStatus` until it is decidable and squash-merge. Skips on a failed required check or timeout. | `bash merge-sweep.sh ` | -| `merge-sweep-cov.sh` | Coverage-aware merge sweep: like `merge-sweep.sh`, but admin-overrides a SonarCloud failure that is solely new-code coverage (verified from the check-run summary), and skips anything more. | `bash merge-sweep-cov.sh ` | +| `merge-sweep.sh` | Require-up-to-date server-side merge sweep: per PR, update-branch then poll `mergeStateStatus` until it is decidable and squash-merge. Skips on a failed required check or timeout. Blocks every merge until the `review` check on the CURRENT head SHA settles, so an update-branch cannot merge on a stale APPROVED, and exits 1 on a merged PR whose head branch was re-created. | `bash merge-sweep.sh ` (or `--help`) | +| `merge-sweep-cov.sh` | Coverage-aware merge sweep: like `merge-sweep.sh` (same review-staleness guard and orphaned-head scan), but admin-overrides a SonarCloud failure that is solely new-code coverage (verified from the check-run summary), and skips anything more. | `bash merge-sweep-cov.sh ` (or `--help`) | | `rollup.sh` | Thin cross-repo CI/nightly health roll-up: reads the latest `main` run of each tracked quality gate across all three Orbit repos and prints ONE consolidated verdict (exit `0` green / `1` red / `2` tool-error). Reads run conclusions only; runs and audits nothing. Backs the `/rollup` skill and `.github/workflows/rollup.yml`. | `bash rollup.sh` (or `--help`) | | `surface-manifest.mjs` | Derives the visual-surface inventory from the codebase (web routes, the multi-view Today root, overlays) into `.claude/manifests/surfaces.json`, expanded to one cell per surface x theme x locale. Emits **no status field** on purpose. Survives until the #539 Linear project completes (REBUILD.md D39), then folds into `arch-map.mjs`. | `npm run surfaces:manifest` | | `capture-surfaces.mjs` | Playwright capture of one screenshot per manifest cell into `.artifacts/surfaces/`, against a running local stack. Reports every surface it cannot reach generically instead of skipping it. The evidence-gate screenshot mechanism (D7). | `ORBIT_AUTH_TOKEN=... npm run surfaces:capture` | diff --git a/tools/dash-baseline.json b/tools/dash-baseline.json index 8d5773a84..0b6923304 100644 --- a/tools/dash-baseline.json +++ b/tools/dash-baseline.json @@ -160,7 +160,5 @@ "packages/shared/src/utils/plural.ts": 1, "sonar-project.properties": 13, "TESTING.md": 7, - "tools/CONVENTIONS.md": 2, - "tools/merge-sweep-cov.sh": 3, - "tools/merge-sweep.sh": 1 + "tools/CONVENTIONS.md": 2 } diff --git a/tools/merge-sweep-cov.sh b/tools/merge-sweep-cov.sh index f8fb35da6..1c0bf576e 100755 --- a/tools/merge-sweep-cov.sh +++ b/tools/merge-sweep-cov.sh @@ -9,9 +9,63 @@ # never a Bug/Vuln/Hotspot/Smell/Duplication/rating drop) -> admin squash-merge # (coverage debt repaid in the Sonar burn-down; rubber-stamp tests are banned). # * Sonar FAILURE on anything more -> SKIP (needs a real fix). -# Never touches the local working tree. Usage: merge-sweep-cov.sh -repo="$1"; shift -gate() { # prints MS \t REVIEW \t NONSONAR_FAILED \t SONARSTATE \t SHA +# WHY the review-staleness guard below: an update-branch rewrites the head SHA and re-triggers the +# `review` check, but GitHub keeps the PRE-update APPROVED reviewDecision while that re-review runs, +# so a sweep that merges on a decidable merge state can ship past a CHANGES_REQUESTED that lands +# seconds later. That happened on https://github.com/thomasluizon/orbit-api/pull/403: a HIGH +# backend-contract finding reached main and deployed, and the fix went to the orphaned head branch. +# Never touches the local working tree. +set -u + +REVIEW_WORKFLOW_PATH=".github/workflows/claude-review.yml" +REVIEW_CHECK_NAME="review" + +usage() { + cat < ... + merge-sweep-cov.sh --help + +Per PR it update-branches, polls until the merge state is decidable, then merges: + Sonar SUCCESS or absent -> squash merge + Sonar FAILURE, new-code coverage -> admin squash merge (coverage-only override) + Sonar FAILURE, anything else -> SKIP + +It refuses to merge while the \`$REVIEW_CHECK_NAME\` check for the CURRENT head SHA is still +running, and re-reads reviewDecision after that check settles, so a pre-update APPROVED can +never carry a merge. Repos without $REVIEW_WORKFLOW_PATH skip that wait. + +After the sweep it re-checks every merged PR's head branch: a branch that exists again was +re-created by a post-merge push, so its commits never reached main. + +Output (stdout): one MERGED/SKIP/FAIL-ADMIN line per PR, then any ORPHANED-HEAD lines, then +COV-SWEEP-DONE. +Exit codes: 0 no orphaned head branch; 1 at least one orphaned head branch; 2 bad usage. +EOF +} + +case "${1:-}" in + -h | --help) + usage + exit 0 + ;; +esac +if [ "$#" -lt 2 ]; then + usage >&2 + exit 2 +fi +repo="$1" +shift + +review_required="" +if gh api "repos/$repo/actions/workflows" --jq '.workflows[].path' 2>/dev/null | grep -qx "$REVIEW_WORKFLOW_PATH"; then + review_required=1 +fi + +merged_heads="" + +gate() { # prints MS \t REVIEW \t NONSONAR_FAILED \t SONARSTATE \t SHA \t REVIEWCHECK gh pr view "$1" --repo "$repo" --json mergeStateStatus,reviewDecision,statusCheckRollup,headRefOid 2>/dev/null | node -e " let s='';process.stdin.on('data',d=>s+=d).on('end',()=>{ try{ @@ -22,37 +76,97 @@ gate() { # prints MS \t REVIEW \t NONSONAR_FAILED \t SONARSTATE \t SHA const nonSonar=failed.filter(c=>(c.name||c.context)!=='SonarCloud Code Analysis').map(c=>c.name||c.context); const sonar=rows.find(c=>(c.name||c.context)==='SonarCloud Code Analysis')||{}; const sonarState=(sonar.conclusion||sonar.state||'NONE').toUpperCase(); - process.stdout.write([(d.mergeStateStatus||'?'),(d.reviewDecision||'?'),(nonSonar.join(',')||'NONE'),sonarState,(d.headRefOid||'')].join('\t')); - }catch(e){process.stdout.write('ERR\tERR\tERR\tERR\t');} + const review=rows.find(c=>(c.name||c.context)==='$REVIEW_CHECK_NAME'); + const reviewSettled=!!review&&(!!review.conclusion||(review.status||'').toUpperCase()==='COMPLETED'); + const reviewCheck=!review?'ABSENT':(reviewSettled?'SETTLED':'RUNNING'); + process.stdout.write([(d.mergeStateStatus||'?'),(d.reviewDecision||'?'),(nonSonar.join(',')||'NONE'),sonarState,(d.headRefOid||''),reviewCheck].join('\t')); + }catch(e){process.stdout.write('ERR\tERR\tERR\tERR\t\tERR');} })" } + +squash_merge() { #