feat: [HIMMEL-1596][HIMMEL-1573][HIMMEL-1617] wave 2k — orchestration wave 1: worker checkpointing, arm-resume integrity, GLM lane liveness, dispatch-guard precision (23 tickets) - #555
Conversation
… wave 1: worker checkpointing, arm-resume integrity, GLM lane liveness, dispatch-guard precision (23 tickets) The orchestration-MVP wave-1 batch plus its follow-ups, 17 private merges: - Worker lifecycle: checkpoint a worker's uncommitted work + reap stale checkpoints (HIMMEL-1596); await-glm-worker prefix-sibling resolver and liveness that does not lie (HIMMEL-1347); a confirmed-alive pid is never STALLED and the mtime leg walks the worker's own worktree (HIMMEL-1573/1616); portable mtime leg with loud degradation (HIMMEL-1614); retry the glm critic on timeout (HIMMEL-1569). - arm-resume integrity: no arm without a registry record (HIMMEL-1603), captured arm time (HIMMEL-1579), T24 isolated-tree rot (HIMMEL-1607), refuse re-arming shipped work + bounded probes (HIMMEL-1331), refuse arming a real task at a temp target (HIMMEL-1365/1606), SCHTASKS_CMD seam (HIMMEL-1610). - Dispatch guard: funded-bank lane refusal (HIMMEL-1513); descriptive fix mentions and plan-shaped briefs no longer read as implementation intent, imperative shapes still refuse (HIMMEL-1617). - Harness/test hygiene: version identity VERSION + himmelctl --version (HIMMEL-1598); squash-aware branch classifier (HIMMEL-1600); jira list pagination past the silent 100 cap (HIMMEL-1597); adopter fixes (HIMMEL-1586); skip propagated tests lacking a private fixture (HIMMEL-1590); fsync-off test fixtures + conditional filter (HIMMEL-1589); bun test cwd is per-suite (HIMMEL-1615); gitignore CorsairLink.log (HIMMEL-1611).
📝 WalkthroughWalkthroughThis PR adds conditional shell-suite filtering, worker checkpoint durability, handover scheduling safeguards, dispatch controls, Jira pagination, branch classification, worker liveness handling, and supporting repository tooling. ChangesTest and repository tooling
Worker checkpoints
Handover scheduling safeguards
Dispatch and hook controls
Branch classification
Jira listing and worker liveness
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WorkerSpawner
participant WorkerWorktree
participant CheckpointRef
participant MetaJson
WorkerSpawner->>WorkerWorktree: execute worker run
WorkerSpawner->>CheckpointRef: snapshot allowed changes with private index
CheckpointRef-->>WorkerSpawner: return checkpoint commit or failure reason
WorkerSpawner->>MetaJson: merge checkpoint metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
scripts/hooks/guard-implementor-dispatch.sh (2)
272-289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
lane_fundedre-runs the whole probe for each lane, doubling the worst-case blocking latency.
bank-status.tsprints one line for every lane in a single invocation. Line 346 and line 348 each calllane_funded, so the probe spawnsbuntwice and re-reads every bank twice whenever claudex is skipped and glm is evaluated.This runs on a PreToolUse path, which blocks the tool call. With the default 4s budget the worst case is about 8s of added latency instead of 4s.
Memoize the probe output on first use.
The fail-open contract itself is correct. Checking
rcbefore parsing the output is the right ordering, and test RC35B pins it.♻️ Proposed memoization of the probe output
+_bank_status_out="" +_bank_status_rc="" + lane_funded() { local id="$1" cmd budget out rc state lid lstate budget="${IMPL_GUARD_BANK_BUDGET_SECS:-4}" + if [ -n "$_bank_status_rc" ]; then + out="$_bank_status_out" + rc="$_bank_status_rc" + else if [ -n "${IMPL_GUARD_BANK_STATUS_CMD:-}" ]; then cmd="$IMPL_GUARD_BANK_STATUS_CMD" else @@ rc=0 out=$(_run_bounded "$budget" "$cmd") || rc=$? + _bank_status_out="$out" + _bank_status_rc="$rc" + fiNote: the early
return 0preconditions (bun missing, helper missing) must also record a memoized state, or set a separate "probe unavailable" flag, so the second lane does not repeat the same warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/guard-implementor-dispatch.sh` around lines 272 - 289, Update lane_funded to memoize the bank-status probe output and return cached lane results on subsequent calls, so bank-status.ts runs at most once per dispatch. Preserve checking the probe return code before parsing output, and record the unavailable/fail-open state for missing bun or bank-status.ts so those preconditions do not repeat warnings or probes.
137-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe generic determiner strip removes imperative "fix" for any verb outside the mask list.
Line 139 masks only
apply|commit|push|land|merge|ship. Line 140 then strips the generic(a|an|the)[[:space:]]+fixarm. An imperative that uses any other verb loses its signal.Examples that now classify as non-implementation:
revert the fixdeploy the fixbackport the fixrebase the fix onto mainThe descriptive determiners are already enumerated separately (
this|that|its|prior|previous|earlier|existing, pluscommitted a/the fix). The generic(a|an|the)arm is the one that over-strips. Consider dropping the generic arm and relying on the enumerated descriptive forms, or requiring the generic arm to be clause-initial.This is a recall gap in a fail-open direction: a missed refusal lets the in-process agent do the work. It does not create a hard block. Treat it as a follow-up rather than a merge blocker.
♻️ Proposed narrowing of the generic arm
implementation_text=$(printf '%s' "$text" | tr '[:upper:]' '[:lower:]' | sed -E ' s/how[[:space:]]+to[[:space:]]+(implement|fix|land)/ /g s/(apply|commit|push|land|merge|ship)[[:space:]]+(the[[:space:]]+|a[[:space:]]+)?fix/applyprotected/g - s/((this|that|its|prior|previous|earlier|existing)[[:space:]]+|committed[[:space:]]+(a|the)[[:space:]]+|(a|an|the)[[:space:]]+)fix(ed)?/ /g + s/((this|that|its|prior|previous|earlier|existing|proposed|original|attempted)[[:space:]]+|committed[[:space:]]+(a|the)[[:space:]]+)fix(ed)?/ /g + s/(^|[.!?;][[:space:]]*)(a|an|the)[[:space:]]+fix(ed)?/\1 /g s/fixed/ /g s/applyprotected/apply the fix/g ')Note: test RC49 (
The proposed fix is large.) currently passes through the read-only declaration gate, not the strip. Addingproposedto the enumerated list keeps it passing on the strip path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/guard-implementor-dispatch.sh` around lines 137 - 146, Narrow the generic determiner handling in the sed rule within implementation_text so imperative phrases such as “revert the fix,” “deploy the fix,” “backport the fix,” and “rebase the fix” retain their implementation signal. Remove the broad `(a|an|the) fix` alternative or constrain it to clause-initial usage, while preserving the existing descriptive determiner patterns and the intended handling of “proposed fix.”scripts/hooks/narrow-allow.mjs (3)
214-224: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an
fsyncbefore the rename, or soften the durability claim in the comment.The header at lines 198-202 states that an interruption between stage and swap can never leave
settings.jsontruncated or empty, and thatpermissions.deny/askcannot be dropped mid-write.That holds for a process crash.
renameSyncis atomic with respect to visibility, so no reader ever observes a partial file.It does not hold for a power loss or a kernel panic.
writeFileSyncreturns after the data reaches the page cache, not the disk. The directory entry created by the rename can be committed before the data blocks. The result is a zero-lengthsettings.json, which drops the entire permission policy includingdenyandask. ext4 withdata=orderedhappens to order this correctly; other filesystems do not guarantee it.Flush the staged temp before the rename.
♻️ Proposed fsync before rename
-import { readFileSync, renameSync, unlinkSync, writeFileSync } from 'node:fs'; +import { closeSync, fsyncSync, openSync, renameSync, readFileSync, unlinkSync, writeSync } from 'node:fs';export function writeSettingsAtomic(settingsPath, output, tmpPath = tempPathFor(settingsPath)) { let staged = false; try { - writeFileSync(tmpPath, output, { encoding: 'utf8', flag: 'wx' }); - staged = true; + // 'wx' => O_CREAT|O_EXCL: a pre-existing temp raises EEXIST, never a clobber. + const fd = openSync(tmpPath, 'wx'); + staged = true; + try { + writeSync(fd, output, null, 'utf8'); + // Force the bytes to disk BEFORE the rename publishes the new inode, so a + // power loss cannot commit the directory entry ahead of the data blocks. + fsyncSync(fd); + } finally { + closeSync(fd); + } renameSync(tmpPath, settingsPath); } catch (error) { if (staged) { try { unlinkSync(tmpPath); } catch { /* best-effort cleanup of our own staged temp */ } } throw error; } }Note:
stagednow becomes true once the exclusive open succeeds, which keeps the existing EEXIST behaviour — a foreign temp still fails beforestagedis set, so cleanup never deletes it. TestwriteSettingsAtomic opens the temp exclusivelystays green.If you prefer not to take the
fsynccost, narrow the comment to say the guarantee covers process crashes rather than all interruptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/narrow-allow.mjs` around lines 214 - 224, Update writeSettingsAtomic to flush the staged temporary file to durable storage with fsync before renameSync, while preserving exclusive creation and cleanup behavior. Set staged immediately after the temporary file is successfully opened so cleanup only removes files owned by this operation; use the existing file-descriptor lifecycle and ensure it is closed after syncing.
187-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnique temp names remove the collision bug but make crash leftovers unbounded.
The fixed name
.narrow-allow.tmpwas reused by the next run, so at most one stale file existed. A pid+uuid name leaks one file per crash. If the process is killed between the exclusive write at line 217 and the rename at line 219, the staged temp stays in the.claude/directory and nothing removes it.The window is two adjacent syscalls, so the probability is low. The atomicity gain is worth the trade. Consider a cheap sweep of stale siblings while the settings lock is already held, so the directory cannot accumulate temps indefinitely.
Only remove a temp that is clearly abandoned. Match the
.narrow-allow.tmp.prefix and require an mtime older than a few minutes, so a concurrent run's in-flight temp is never deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/narrow-allow.mjs` around lines 187 - 196, Update the locked settings-write flow around writeSettingsAtomic to sweep stale sibling temp files before or after writing. Remove only files sharing the .narrow-allow.tmp. prefix whose modification time is older than a few minutes, preserving recent in-flight temps and leaving unrelated files untouched.
226-256: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse atomic replacement in
wire-hook-bash.mjs
wire-hook-bash.mjsstill writessettings.jsonwithwriteFileSync, which can leave a truncated file if the process stops during the write. ReusewriteSettingsAtomicfor all sanctioned settings writers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/narrow-allow.mjs` around lines 226 - 256, Update wire-hook-bash.mjs to replace its direct writeFileSync settings update with the existing writeSettingsAtomic helper, matching the atomic write path used by narrow-allow and ensuring all sanctioned settings writers use atomic replacement.scripts/hooks/test-guard-implementor-dispatch.sh (1)
557-581: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
RC56andRC57are each assigned twice.
RC56is set at line 557 forreadonly-please-fix, then reset at line 572 forprobe-descriptive-allows.RC57is set at line 560 forreadonly-now-implement, then reset at line 580 forstandalone-implement-blocks.Each assertion runs on the line directly after its assignment, so the suite is correct today. The numbering scheme is unique everywhere else, which makes these two reuses look accidental. If a later edit separates an assignment from its assertion, the assertion reads the wrong run's exit code and the failure is silent.
Renumber the second pair to
RC59andRC60.♻️ Proposed renumbering
-RC56=$(run_hook probe-descriptive-allows "$REG_CLAUDEX" "$(payload general-purpose sonnet 'Judge doc placement' 'Analysis only. Do not edit any file. Decide whether this doc section belongs in the always-loaded file or a reference. The parent already committed a fix that expanded the bullet. Return a recommendation as text.')") -assert_rc "descriptive analysis brief with a committed fix allows" 0 "$RC56" +RC59=$(run_hook probe-descriptive-allows "$REG_CLAUDEX" "$(payload general-purpose sonnet 'Judge doc placement' 'Analysis only. Do not edit any file. Decide whether this doc section belongs in the always-loaded file or a reference. The parent already committed a fix that expanded the bullet. Return a recommendation as text.')") +assert_rc "descriptive analysis brief with a committed fix allows" 0 "$RC59"-RC57=$(run_hook standalone-implement-blocks "$REG_CLAUDEX" "$(payload general-purpose sonnet 'Do the work' 'Analysis only. Do not edit any file. Implement the new handler and report.')") -assert_rc "analysis-only declaration does not rescue a standalone implement" 2 "$RC57" +RC60=$(run_hook standalone-implement-blocks "$REG_CLAUDEX" "$(payload general-purpose sonnet 'Do the work' 'Analysis only. Do not edit any file. Implement the new handler and report.')") +assert_rc "analysis-only declaration does not rescue a standalone implement" 2 "$RC60"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/test-guard-implementor-dispatch.sh` around lines 557 - 581, In the test sequence around probe-descriptive-allows and standalone-implement-blocks, rename the second assignments currently using RC56 and RC57 to RC59 and RC60, and update their corresponding assert_rc references so each test retains its own exit code.scripts/telegram/spawn-glm.ts (1)
1416-1418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe no-checkpoint line prints on every clean run.
A worker that commits its work leaves a clean worktree. The
finallyblock then printsno checkpoint (clean)on stderr for every well-behaved dispatch. This adds noise to the normal path. Consider printing the reason only when it is notclean, or downgrading it to a verbose-only line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/telegram/spawn-glm.ts` around lines 1416 - 1418, Update the no-checkpoint handling around cp.reason so clean completions do not print to stderr; retain the existing message for non-clean reasons, or emit the clean case only through verbose logging.scripts/handover/test-arm-resume.sh (1)
2277-2278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture
future_timeonce in the verify tests, as T23 does.Each of these sites calls
$(future_time)twice: once to build the stub's expected epoch and once as the--timeargument. The cache keeps the two calls equal in almost every run. If a refresh lands between them, the values differ by about 30 minutes, which exceeds the 120s tolerance in the post-arm verify at Line 3389 ofscripts/handover/arm-resume.sh, and the test fails for a reason unrelated to what it checks.This is the same window the comment at Lines 735-740 already documents for T23. Applying the same fix here removes the last instances.
♻️ Proposed pattern, shown for V1
+V1_TIME=$(future_time) ... -' "$(future_time)") -out=$(win_env "$V1BIN" bash "$ARM" --time "$(future_time)" --handover "$V1_HO" --dry-run 2>&1) +' "$V1_TIME") +out=$(win_env "$V1BIN" bash "$ARM" --time "$V1_TIME" --handover "$V1_HO" --dry-run 2>&1)Also applies to: 2302-2303, 2383-2387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/handover/test-arm-resume.sh` around lines 2277 - 2278, Update the affected verify tests around the V1 handover invocations to call future_time once, store the result in a local variable, and reuse that variable both when constructing the stub’s expected epoch and when passing the --time argument. Apply the same pattern to the sites around lines 2302-2303 and 2383-2387, matching the existing T23 handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/git/classify-branches.sh`:
- Around line 97-99: Update the PR-state collection and `--pr-map` handling in
`classify-branches.sh` to include each merged PR’s `headRefOid` alongside
`headRefName` and state. Gate the `LANDED` shortcut on the local branch tip
matching that OID; otherwise call `classify_by_content`, including for fork-name
collisions or recreated branches. Add a regression case covering a merged PR
name with a divergent local branch.
- Line 70: Validate the --limit value before generating the report, requiring a
positive decimal integer and returning exit status 2 for invalid input. Update
BASE validation to require verification as a commit object rather than accepting
arbitrary Git objects, likewise returning status 2 on failure. Add test cases
asserting rc=2 for both invalid limit and non-commit BASE inputs.
In `@scripts/handover/arm-resume.sh`:
- Around line 2891-2892: Guard the stale launcher cleanup around the find
command that deletes himmel-resume.*.bat files so it is skipped whenever DRY_RUN
is enabled. Preserve pruning during normal execution, while ensuring the
--dry-run path remains completely side-effect-free.
- Around line 2472-2484: Ensure the PR-processing loop in _arm_shipped_preflight
always returns success after evaluating rows, including when the final OPEN row
is not MERGEABLE. Neutralize the conditional test’s nonzero status or add an
explicit successful final command so the unguarded _arm_shipped_preflight call
cannot abort under set -e.
- Around line 2449-2454: Update the status case in the ticket probe flow around
_arm_probe and _arm_shipped_note so it matches only configured Jira status
names, removing the "wont do" and "wont fix" resolution values. Keep Done and
Closed only unless resolution-based detection is explicitly implemented by
fetching the resolution separately.
In `@scripts/hooks/check-trust-suites.sh`:
- Around line 4-13: Update the comments in check-trust-suites.sh to remove the
inaccurate claims that no pipeline runs the trust suites and that pre-commit is
the only execution surface. Explain instead that the hook provides path-filtered
local pre-commit coverage, while preserving the existing rationale about the
hook’s distinct role and repository/fixture constraints.
In `@scripts/hooks/guard-implementor-dispatch.sh`:
- Around line 241-264: Update _run_bounded to accept the bank-status path as a
separate argument and pass it safely to the spawned command, rather than
embedding it in cmd. Adjust the caller around the bank-status dispatch so the
path is supplied as an argument, preventing bash -c reparsing from expanding or
executing path contents.
In `@scripts/jira/src/commands/list.ts`:
- Around line 91-117: The MCP list handler must paginate Jira search results
instead of issuing a single request and ignoring nextPageToken. Update the MCP
handler around its `/search/jql` request to reuse searchAllIssues via an
appropriate request adapter or implement equivalent cursor traversal, preserving
the requested limit and formatting all returned issues. Add coverage verifying a
limit of 150 triggers two requests and produces 150 formatted issues.
In `@scripts/lanes/await-glm-worker.sh`:
- Around line 199-204: Update the BSD capability probe near MTIME_TOOL selection
to capture the output of stat -f %m / and verify it is numeric before assigning
MTIME_TOOL=bsd. If the output is non-numeric, continue to MTIME_TOOL=none so the
existing degradation notice is emitted.
In `@scripts/lanes/tests/test-await-liveness.sh`:
- Around line 44-45: Update scripts/lanes/tests/test-await-liveness.sh at lines
44-45 to replace GNU-only touch -d "$STALE_ISO" with portable touch -t
202001010000; at lines 189-199, update the fake stat implementation to use GNU
-c %Y when supported and BSD -f %m otherwise, preserving cross-platform liveness
behavior.
In `@scripts/setup.sh`:
- Around line 226-234: Do not ignore failures from install-cr-pre-push-legacy.sh
in either setup path: update scripts/setup.sh lines 226-234 and
scripts/setup-hooks.sh lines 124-129 to propagate the installer failure, or
establish an explicit safe hook chain before reporting setup success. Ensure
both paths prevent claiming successful installation when a foreign
pre-push.legacy hook blocks the Himmel hook.
In `@scripts/telegram/spawn-glm.ts`:
- Line 898: Update the checkpoint logic around the read-tree guard to explicitly
detect whether HEAD exists before calling `git read-tree HEAD`; skip the
seed/read-tree step for an unborn HEAD so the existing unborn-HEAD handling and
checkpoint behavior can execute. Ensure normal repositories still require a
successful `read-tree HEAD`, and keep the comments aligned with the resulting
behavior.
---
Nitpick comments:
In `@scripts/handover/test-arm-resume.sh`:
- Around line 2277-2278: Update the affected verify tests around the V1 handover
invocations to call future_time once, store the result in a local variable, and
reuse that variable both when constructing the stub’s expected epoch and when
passing the --time argument. Apply the same pattern to the sites around lines
2302-2303 and 2383-2387, matching the existing T23 handling.
In `@scripts/hooks/guard-implementor-dispatch.sh`:
- Around line 272-289: Update lane_funded to memoize the bank-status probe
output and return cached lane results on subsequent calls, so bank-status.ts
runs at most once per dispatch. Preserve checking the probe return code before
parsing output, and record the unavailable/fail-open state for missing bun or
bank-status.ts so those preconditions do not repeat warnings or probes.
- Around line 137-146: Narrow the generic determiner handling in the sed rule
within implementation_text so imperative phrases such as “revert the fix,”
“deploy the fix,” “backport the fix,” and “rebase the fix” retain their
implementation signal. Remove the broad `(a|an|the) fix` alternative or
constrain it to clause-initial usage, while preserving the existing descriptive
determiner patterns and the intended handling of “proposed fix.”
In `@scripts/hooks/narrow-allow.mjs`:
- Around line 214-224: Update writeSettingsAtomic to flush the staged temporary
file to durable storage with fsync before renameSync, while preserving exclusive
creation and cleanup behavior. Set staged immediately after the temporary file
is successfully opened so cleanup only removes files owned by this operation;
use the existing file-descriptor lifecycle and ensure it is closed after
syncing.
- Around line 187-196: Update the locked settings-write flow around
writeSettingsAtomic to sweep stale sibling temp files before or after writing.
Remove only files sharing the .narrow-allow.tmp. prefix whose modification time
is older than a few minutes, preserving recent in-flight temps and leaving
unrelated files untouched.
- Around line 226-256: Update wire-hook-bash.mjs to replace its direct
writeFileSync settings update with the existing writeSettingsAtomic helper,
matching the atomic write path used by narrow-allow and ensuring all sanctioned
settings writers use atomic replacement.
In `@scripts/hooks/test-guard-implementor-dispatch.sh`:
- Around line 557-581: In the test sequence around probe-descriptive-allows and
standalone-implement-blocks, rename the second assignments currently using RC56
and RC57 to RC59 and RC60, and update their corresponding assert_rc references
so each test retains its own exit code.
In `@scripts/telegram/spawn-glm.ts`:
- Around line 1416-1418: Update the no-checkpoint handling around cp.reason so
clean completions do not print to stderr; retain the existing message for
non-clean reasons, or emit the clean case only through verbose logging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2255c5e4-e253-4fd6-92ab-323b76ff7d04
📒 Files selected for processing (39)
.github/workflows/ci.yml.gitignore.pre-commit-config.yamlVERSIONdocs/internals/environment-gotchas.mdscripts/ci/run-shell-tests.shscripts/ci/test-run-shell-tests.shscripts/clean-garden.shscripts/cr/critics.jsonscripts/cr/test-critic-panel-fallback.shscripts/git/classify-branches.shscripts/git/test-classify-branches.shscripts/handover/arm-resume.shscripts/handover/test-arm-resume.shscripts/himmelctl/bin.jsscripts/himmelctl/test/test-version.shscripts/hooks/check-trust-suites.shscripts/hooks/guard-implementor-dispatch.shscripts/hooks/narrow-allow.mjsscripts/hooks/narrow-allow.test.mjsscripts/hooks/test-guard-implementor-dispatch.shscripts/hooks/wire-hook-bash.test.mjsscripts/jira/src/commands/list-pagination.test.tsscripts/jira/src/commands/list.tsscripts/jira/src/types.tsscripts/lanes/await-glm-worker.shscripts/lanes/bank-status.tsscripts/lanes/tests/test-await-glm-worker.shscripts/lanes/tests/test-await-liveness.shscripts/lib/git-test-env.shscripts/lib/scheduler-backend.shscripts/lib/test-detect-hook-dup.shscripts/setup-hooks.shscripts/setup.shscripts/telegram/spawn-glm.test.tsscripts/telegram/spawn-glm.tsscripts/test-clean-garden-accounting.shscripts/upstreams/test-resync-fork.shscripts/voice/speak.sh
| --base) [ $# -ge 2 ] || { echo "classify-branches: --base needs a value" >&2; exit 2; }; BASE="$2"; shift 2 ;; | ||
| --pattern) [ $# -ge 2 ] || { echo "classify-branches: --pattern needs a value" >&2; exit 2; }; PATTERN="$2"; shift 2 ;; | ||
| --pr-map) [ $# -ge 2 ] || { echo "classify-branches: --pr-map needs a value" >&2; exit 2; }; PR_MAP="$2"; shift 2 ;; | ||
| --limit) [ $# -ge 2 ] || { echo "classify-branches: --limit needs a value" >&2; exit 2; }; LIMIT="$2"; shift 2 ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid option values before producing the report.
A non-numeric --limit makes gh pr list fail and silently changes the report to content-only mode. A non-commit value such as HEAD:path passes git rev-parse --verify, then makes every non-PR branch UNKNOWN while the script exits 0.
Validate LIMIT as a positive decimal integer. Verify BASE^{commit} instead of accepting any Git object. Add rc=2 test cases for both inputs.
Proposed validation
done
+case "$LIMIT" in
+ ''|*[!0-9]*|0)
+ echo "classify-branches: --limit must be a positive integer" >&2
+ exit 2
+ ;;
+esac
+
git rev-parse --git-dir >/dev/null 2>&1 || {
echo "classify-branches: not inside a git repository" >&2; exit 2; }
-git rev-parse --verify --quiet "$BASE" >/dev/null || {
+git rev-parse --verify --quiet "${BASE}^{commit}" >/dev/null || {
echo "classify-branches: base ref '$BASE' does not exist" >&2; exit 2; }As per PR objectives, the classifier must validate inputs.
Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/git/classify-branches.sh` at line 70, Validate the --limit value
before generating the report, requiring a positive decimal integer and returning
exit status 2 for invalid input. Update BASE validation to require verification
as a commit object rather than accepting arbitrary Git objects, likewise
returning status 2 on failure. Add test cases asserting rc=2 for both invalid
limit and non-commit BASE inputs.
| if gh pr list --state all --limit "$LIMIT" \ | ||
| --json headRefName,state \ | ||
| --jq '.[] | [.headRefName, .state] | @tsv' > "$PR_STATE_FILE" 2>/dev/null; then |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind the merged PR state to the local branch tip.
headRefName is not a unique branch identity. A recreated local branch, or a local branch with the same name as a fork PR head, matches this map and bypasses content comparison as LANDED.
Fetch headRefOid with the PR data. Apply the merged-PR shortcut only when the local branch tip equals that OID. Otherwise, run classify_by_content. Update --pr-map and add a regression case with a merged PR name and a divergent local branch.
Also applies to: 242-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/git/classify-branches.sh` around lines 97 - 99, Update the PR-state
collection and `--pr-map` handling in `classify-branches.sh` to include each
merged PR’s `headRefOid` alongside `headRefName` and state. Gate the `LANDED`
shortcut on the local branch tip matching that OID; otherwise call
`classify_by_content`, including for fork-name collisions or recreated branches.
Add a regression case covering a merged PR name with a divergent local branch.
| if [ -n "$_ticket" ] && [ -f "$_jira" ] && command -v node >/dev/null 2>&1; then | ||
| _status=$(_arm_probe node "$_jira" get "$_ticket" | head -1 | awk -F'\t' '{print $3}') || _status="" | ||
| case "$_status" in | ||
| Done|Closed|"wont do"|"wont fix") | ||
| _arm_shipped_note "ticket $_ticket is '$_status'" ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the `get` command's printed columns and the status vocabulary used elsewhere.
set -euo pipefail
fd -t f 'get.ts' scripts/jira/src | while IFS= read -r f; do
echo "=== $f"
cat -n "$f"
done
# Any other place that compares a Jira status string.
rg -n -C3 --type=ts --type=sh "Won't|wont do|wont fix|'Done'|\"Done\"" scripts | head -80Repository: yotamleo/Himmel
Length of output: 9242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Jira formatter symbols'
ast-grep outline scripts/jira/src/output.ts --match 'formatIssue' --view expanded
printf '%s\n' '=== Jira formatter source'
cat -n scripts/jira/src/output.ts
printf '%s\n' '=== Formatter and get tests'
rg -n -C5 --type=ts 'formatIssue|formatIssueWithDescription|short|status' scripts/jira/src --glob '*test.ts'
printf '%s\n' '=== arm probe definitions and call sites'
rg -n -C8 --type=sh '_arm_probe|node .*jira.*get|_arm_shipped_note' scripts/handover/arm-resume.shRepository: yotamleo/Himmel
Length of output: 36049
🌐 Web query:
Atlassian Jira official documentation status names "Won't Do" "Won't Fix"
💡 Result:
In Atlassian Jira, "Won't Do" and "Won't Fix" are typically resolution values, not statuses [1][2]. Key distinctions: - Resolution: This field explains how an issue was completed (e.g., "Fixed," "Won't Fix," "Won't Do," "Duplicate") [1][2]. It is a system field that indicates the issue's final state when it reaches a "Done" status [3][2]. - Status: This field describes where an issue is within its workflow (e.g., "To Do," "In Progress," "Done") [4][5]. "Won't Do" and "Won't Fix" are common resolution values provided by default in many Jira project configurations [6][2]. For example, Jira introduced "Won't Do" as a resolution option specifically for software development projects, intended to function similarly to "Won't Fix" [6]. Because Jira is highly customizable, administrators can define, edit, or remove both status names and resolution values to fit their team's specific requirements [7][4][1]. Consequently, while these terms are common defaults, they are not hardcoded system-wide statuses that cannot be changed [6][2]. If you want to use "Won't Do" or "Won't Fix" in your workflow, the standard practice is to move the issue to a "Done" status category and configure a workflow transition or post-function to automatically set the appropriate resolution field [1][8][9][2].
Citations:
- 1: https://community.atlassian.com/forums/Jira-articles/What-Makes-an-Issue-Done-Resolved-or-Completed/ba-p/2861518
- 2: https://drivtu.com/resolution-field-values/
- 3: https://community.atlassian.com/forums/Jira-questions/Are-there-definitions-for-the-various-default-statuses-in/qaq-p/46148
- 4: https://support.atlassian.com/jira-cloud-administration/docs/what-are-issue-statuses-priorities-and-resolutions/
- 5: https://confluence.atlassian.com/servicemanagementserver0517/advanced-searching-fields-reference-1431243384.html
- 6: https://confluence.atlassian.com/jira/jira-6-4-eap-13-m13-700088986.html
- 7: https://confluence.atlassian.com/spaces/ADMINJIRASERVER/pages/938847116/Issue+fields+and+statuses
- 8: https://community.atlassian.com/forums/Jira-questions/Status-Done-and-Resolution-Won-t-Fix/qaq-p/785619
- 9: https://community.atlassian.com/forums/Jira-questions/Marking-issues-as-closed-or-won-t-do-in-the-next-gen-backlog/qaq-p/923451
Remove the resolution values from the status match. Field 3 is the status. Won't Do and Won't Fix are Jira resolutions, not standard statuses, and the get output does not include resolution. Match configured status names, or fetch resolution separately if resolution-based detection is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/handover/arm-resume.sh` around lines 2449 - 2454, Update the status
case in the ticket probe flow around _arm_probe and _arm_shipped_note so it
matches only configured Jira status names, removing the "wont do" and "wont fix"
resolution values. Keep Done and Closed only unless resolution-based detection
is explicitly implemented by fetching the resolution separately.
| while IFS=$'\t' read -r _num _state _mergeable; do | ||
| [ -n "$_state" ] || continue | ||
| case "$_state" in | ||
| MERGED) _arm_shipped_note "PR #$_num for branch '$_branch' is MERGED" ;; | ||
| OPEN) | ||
| [ "$_mergeable" = "MERGEABLE" ] && \ | ||
| _arm_shipped_note "PR #$_num for branch '$_branch' is OPEN and MERGEABLE" ;; | ||
| esac | ||
| done <<EOF | ||
| $_prs | ||
| EOF | ||
| fi | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the PR loop return 0 so the unguarded call at Line 2493 cannot abort the arm.
Line 2477 is the last command of the OPEN) case arm. If $_mergeable is not MERGEABLE, that test returns 1, the case returns 1, and the while loop body returns 1. A while loop reports the status of the last body execution, so the loop — and therefore _arm_shipped_preflight — returns 1 whenever the final PR row is OPEN and not mergeable.
_arm_shipped_preflight "${_ho_ticket:-}" at Line 2493 is an unguarded simple command under this file's set -e. The script then exits 1 instead of arming. This is the opposite of the fail-open contract stated at Lines 2395-2400.
Test case (c) in scripts/handover/test-arm-resume.sh (1331 conflicting PR does not trip the preflight) uses assert_not_11, which accepts any rc except 11, so it does not catch this.
🐛 Proposed fix
while IFS=$'\t' read -r _num _state _mergeable; do
[ -n "$_state" ] || continue
case "$_state" in
MERGED) _arm_shipped_note "PR #$_num for branch '$_branch' is MERGED" ;;
OPEN)
[ "$_mergeable" = "MERGEABLE" ] && \
_arm_shipped_note "PR #$_num for branch '$_branch' is OPEN and MERGEABLE" ;;
esac
+ : # keep the loop body status 0 -- a non-MERGEABLE OPEN row must
+ # not make this function return 1 into the unguarded call below
done <<EOF
$_prs
EOF
fi
+ return 0
}Based on learnings: "Only flag when the resulting status can propagate to an unguarded set -e-sensitive context (such as a final command position of a function call)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while IFS=$'\t' read -r _num _state _mergeable; do | |
| [ -n "$_state" ] || continue | |
| case "$_state" in | |
| MERGED) _arm_shipped_note "PR #$_num for branch '$_branch' is MERGED" ;; | |
| OPEN) | |
| [ "$_mergeable" = "MERGEABLE" ] && \ | |
| _arm_shipped_note "PR #$_num for branch '$_branch' is OPEN and MERGEABLE" ;; | |
| esac | |
| done <<EOF | |
| $_prs | |
| EOF | |
| fi | |
| } | |
| while IFS=$'\t' read -r _num _state _mergeable; do | |
| [ -n "$_state" ] || continue | |
| case "$_state" in | |
| MERGED) _arm_shipped_note "PR #$_num for branch '$_branch' is MERGED" ;; | |
| OPEN) | |
| [ "$_mergeable" = "MERGEABLE" ] && \ | |
| _arm_shipped_note "PR #$_num for branch '$_branch' is OPEN and MERGEABLE" ;; | |
| esac | |
| : # keep the loop body status 0 -- a non-MERGEABLE OPEN row must | |
| # not make this function return 1 into the unguarded call below | |
| done <<EOF | |
| $_prs | |
| EOF | |
| fi | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/handover/arm-resume.sh` around lines 2472 - 2484, Ensure the
PR-processing loop in _arm_shipped_preflight always returns success after
evaluating rows, including when the final OPEN row is not MERGEABLE. Neutralize
the conditional test’s nonzero status or add an explicit successful final
command so the unguarded _arm_shipped_preflight call cannot abort under set -e.
Source: Learnings
| find "$(dirname "$bat_path")" -maxdepth 1 -type f \ | ||
| -name 'himmel-resume.*.bat' -mtime +7 -delete 2>/dev/null || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip the prune under --dry-run.
The prune runs before the DRY_RUN early return at Line 3221, so --dry-run deletes files. The script states the opposite contract at Line 2490: "--dry-run touches nothing and must stay side-effect-free".
The blast radius is small, because only leaked launchers older than seven days are removed. The inconsistency is still worth closing.
🐛 Proposed fix
- find "$(dirname "$bat_path")" -maxdepth 1 -type f \
- -name 'himmel-resume.*.bat' -mtime +7 -delete 2>/dev/null || true
+ if [ "$DRY_RUN" -ne 1 ]; then
+ find "$(dirname "$bat_path")" -maxdepth 1 -type f \
+ -name 'himmel-resume.*.bat' -mtime +7 -delete 2>/dev/null || true
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find "$(dirname "$bat_path")" -maxdepth 1 -type f \ | |
| -name 'himmel-resume.*.bat' -mtime +7 -delete 2>/dev/null || true | |
| if [ "$DRY_RUN" -ne 1 ]; then | |
| find "$(dirname "$bat_path")" -maxdepth 1 -type f \ | |
| -name 'himmel-resume.*.bat' -mtime +7 -delete 2>/dev/null || true | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/handover/arm-resume.sh` around lines 2891 - 2892, Guard the stale
launcher cleanup around the find command that deletes himmel-resume.*.bat files
so it is skipped whenever DRY_RUN is enabled. Preserve pruning during normal
execution, while ensuring the --dry-run path remains completely
side-effect-free.
| export async function searchAllIssues( | ||
| jql: string, | ||
| limit: string, | ||
| req: typeof request, | ||
| ): Promise<JiraIssue[]> { | ||
| const want = Number.parseInt(limit, 10); | ||
| const target = Number.isFinite(want) && want > 0 ? want : 25; | ||
|
|
||
| const issues: JiraIssue[] = []; | ||
| let token: string | undefined; | ||
|
|
||
| do { | ||
| const page = Math.min(PAGE_MAX, target - issues.length); | ||
| const cursor = token === undefined ? '' : `&nextPageToken=${encodeURIComponent(token)}`; | ||
| const result = await req<JiraSearchResult>( | ||
| 'GET', | ||
| `/search/jql?jql=${encodeURIComponent(jql)}&fields=summary,status,issuetype&maxResults=${page}${cursor}`, | ||
| ); | ||
| issues.push(...result.issues); | ||
| token = result.nextPageToken; | ||
| // A page that returns nothing ends the walk even if a token came back — | ||
| // without this an empty-but-tokened page would spin forever. | ||
| if (result.issues.length === 0) break; | ||
| } while (token !== undefined && issues.length < target); | ||
|
|
||
| return issues.slice(0, target); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline scripts/jira/src/mcp.ts --items all --type function
rg -n -C 12 '/search/jql|nextPageToken|JiraSearchResult|searchAllIssues' \
scripts/jira/src/mcp.ts scripts/jira/src/commands/list.tsRepository: yotamleo/Himmel
Length of output: 10889
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- MCP dependencies and list tool ---'
sed -n '1,230p' scripts/jira/src/mcp.ts
printf '%s\n' '--- search result type and related tests ---'
rg -n -C 8 'interface JiraSearchResult|type JiraSearchResult|nextPageToken|createTools|name: .list.|limit' \
scripts/jira/src scripts/jira/test scripts/jira/tests 2>/dev/null || true
printf '%s\n' '--- repository test/config files ---'
git ls-files scripts/jira | rg '(^|/)(package.json|tsconfig.*|.*test.*|.*spec.*)$' || trueRepository: yotamleo/Himmel
Length of output: 46690
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
mcp = Path("scripts/jira/src/mcp.ts").read_text()
tests = Path("scripts/jira/src/mcp.test.ts").read_text()
handler = re.search(
r"name: 'list'.*?async handler\(args, deps\) \{(.*?)\n \},\n \},",
mcp,
re.S,
)
assert handler, "MCP list handler not found"
body = handler.group(1)
assert body.count("deps.request") == 1, "MCP list handler no longer has exactly one request"
assert "nextPageToken" not in body, "MCP list handler already handles pagination"
assert "searchAllIssues" not in body, "MCP list handler already uses the shared paginator"
assert "maxResults=25" in tests, "default MCP list coverage is missing"
assert not re.search(r"list.*?150|limit:\s*['\"](?:1[1-9]\d|[2-9]\d{2,})['\"]", tests, re.S), \
"MCP pagination coverage already exists"
print("MCP list handler makes one /search/jql request and has no cursor handling.")
print("MCP tests cover default/small limits but no limit above 100.")
print("A limit of 150 therefore remains untested and can return only the first server-capped page.")
PYRepository: yotamleo/Himmel
Length of output: 381
Paginate the MCP list handler.
scripts/jira/src/mcp.ts:177-190 sends one /search/jql request and ignores nextPageToken. For limit=150, it can return only the first 100 issues. Reuse searchAllIssues through an adapter, or add equivalent cursor handling. Add MCP coverage that asserts two requests and 150 formatted issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/jira/src/commands/list.ts` around lines 91 - 117, The MCP list
handler must paginate Jira search results instead of issuing a single request
and ignoring nextPageToken. Update the MCP handler around its `/search/jql`
request to reuse searchAllIssues via an appropriate request adapter or implement
equivalent cursor traversal, preserving the requested limit and formatting all
returned issues. Add coverage verifying a limit of 150 triggers two requests and
produces 150 formatted issues.
| if find / -maxdepth 0 -printf '%T@\n' >/dev/null 2>&1; then | ||
| MTIME_TOOL=gnu | ||
| elif stat -f %m / >/dev/null 2>&1; then | ||
| MTIME_TOOL=bsd | ||
| else | ||
| MTIME_TOOL=none |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target script context ---'
sed -n '1,260p' scripts/lanes/await-glm-worker.sh
printf '%s\n' '--- related mtime logic and notices ---'
rg -n -C 4 'MTIME_TOOL|mtime|degrad|stat -f|find /' scripts/lanes scripts 2>/dev/null | head -n 240
printf '%s\n' '--- available command behavior ---'
command -v find || true
command -v stat || true
find --version 2>/dev/null | head -n 2 || true
stat --version 2>/dev/null | head -n 2 || true
stat -f %m / 2>&1 || true
stat -f %m / 2>/dev/null | awk '{ print "output=[" $0 "], numeric=" ($0 ~ /^[0-9]+$/ ? "yes" : "no") }'Repository: yotamleo/Himmel
Length of output: 35412
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
from pathlib import Path
p = Path("scripts/lanes/await-glm-worker.sh")
lines = p.read_text().splitlines()
for n in range(170, 241):
if n <= len(lines):
print(f"{n}: {lines[n-1]}")
PY
printf '%s\n' '--- repository references ---'
rg -n 'MTIME_TOOL|mtime|degradation|degrad|GNU stat|BSD stat' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 300Repository: yotamleo/Himmel
Length of output: 48908
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import subprocess
def run(*args):
p = subprocess.run(args, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print("cmd:", " ".join(args))
print("status:", p.returncode)
print("stdout:", repr(p.stdout))
print("stderr:", repr(p.stderr))
run("find", "/", "-maxdepth", "0", "-printf", "%T@")
run("stat", "-f", "%m", "/")
run("stat", "-c", "%Y", "/")
PY
printf '%s\n' '--- script syntax and command shape ---'
bash -n scripts/lanes/await-glm-worker.sh
rg -n -C 8 'MTIME_TOOL|case .*MTIME|grep.*mtime|NOTICE|warning|WARN|echo.*mtime|printf.*mtime' scripts/lanes/await-glm-worker.shRepository: yotamleo/Himmel
Length of output: 794
Validate the BSD stat capability probe.
When GNU stat handles stat -f %m /, it can return a mount path with exit code 0. Check that the output is numeric before setting MTIME_TOOL=bsd; otherwise, non-numeric mtimes are discarded without the degradation notice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lanes/await-glm-worker.sh` around lines 199 - 204, Update the BSD
capability probe near MTIME_TOOL selection to capture the output of stat -f %m /
and verify it is numeric before assigning MTIME_TOOL=bsd. If the output is
non-numeric, continue to MTIME_TOOL=none so the existing degradation notice is
emitted.
| # Backdate every file so no fixture is accidentally "live" via mtime. | ||
| find "$wt" -exec touch -d "$STALE_ISO" {} + 2>/dev/null || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove GNU-only commands from the cross-platform liveness suite.
The test suite must run on Windows Git Bash, macOS, and Linux. GNU-only fixture commands make macOS report false test failures.
scripts/lanes/tests/test-await-liveness.sh#L44-L45: replacetouch -d "$STALE_ISO"with a portabletouch -t 202001010000command.scripts/lanes/tests/test-await-liveness.sh#L189-L199: make the fakestatuse GNU-c %Yonly when supported, and otherwise use BSD-f %m.
📍 Affects 1 file
scripts/lanes/tests/test-await-liveness.sh#L44-L45(this comment)scripts/lanes/tests/test-await-liveness.sh#L189-L199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lanes/tests/test-await-liveness.sh` around lines 44 - 45, Update
scripts/lanes/tests/test-await-liveness.sh at lines 44-45 to replace GNU-only
touch -d "$STALE_ISO" with portable touch -t 202001010000; at lines 189-199,
update the fake stat implementation to use GNU -c %Y when supported and BSD -f
%m otherwise, preserving cross-platform liveness behavior.
Source: Path instructions
| # Non-fatal (HIMMEL-1586): the installer refuses (exit 2) to overwrite a | ||
| # foreign pre-push hook that the `pre-commit install --hook-type pre-push` | ||
| # above may have just left behind. That refusal must not abort the remaining | ||
| # setup steps [3/9]..[9/9] — the push-time self-heal | ||
| # (scripts/hooks/check-cr-before-push.sh) reinstalls a missing legacy hook on | ||
| # demand, so nothing is lost by continuing. | ||
| if ! bash "$REPO_ROOT/scripts/hooks/install-cr-pre-push-legacy.sh"; then | ||
| echo " WARNING: CR pre-push legacy hook not installed (see above). Setup continues; the push-time self-heal retries this install on first push." >&2 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,260p' scripts/hooks/check-cr-before-push.sh
rg -n -C 5 'install-cr-pre-push-legacy|pre-push|self-heal|exit' \
scripts/hooks/check-cr-before-push.sh \
scripts/hooks/install-cr-pre-push-legacy.sh \
.pre-commit-config.yamlRepository: yotamleo/Himmel
Length of output: 43635
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- setup.sh ---'
sed -n '190,250p' scripts/setup.sh
printf '%s\n' '--- setup-hooks.sh ---'
sed -n '90,145p' scripts/setup-hooks.sh
printf '%s\n' '--- all hook installation references ---'
rg -n -C 6 'pre-push\.legacy|install-cr-pre-push-legacy|pre-commit install|code-review-before-push' \
scripts .pre-commit-config.yaml docs README.md 2>/dev/null || true
printf '%s\n' '--- tests and fixtures for the recovery path ---'
rg -n -C 5 'foreign|non-Himmel|self-heal|pre-push\.legacy|CR ref-stream|check-cr-before-push' \
--glob '*test*' --glob '*spec*' --glob '*.bats' --glob '*.sh' . 2>/dev/null || trueRepository: yotamleo/Himmel
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- setup.sh target ---'
nl -ba scripts/setup.sh | sed -n '218,240p'
printf '%s\n' '--- setup-hooks.sh target ---'
nl -ba scripts/setup-hooks.sh | sed -n '116,136p'
printf '%s\n' '--- pre-commit CR hook ---'
nl -ba .pre-commit-config.yaml | sed -n '244,258p'
printf '%s\n' '--- legacy installer ---'
nl -ba scripts/hooks/install-cr-pre-push-legacy.sh | sed -n '18,52p'
printf '%s\n' '--- self-heal branch ---'
nl -ba scripts/hooks/check-cr-before-push.sh | sed -n '344,420p'
printf '%s\n' '--- hook installation order ---'
rg -n -C 4 'pre-commit install|install-cr-pre-push-legacy|setup-hooks|setup\.sh' \
scripts/setup.sh scripts/setup-hooks.sh scripts/hooks .pre-commit-config.yamlRepository: yotamleo/Himmel
Length of output: 217
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- setup.sh target ---'
sed -n '218,240p' scripts/setup.sh
printf '%s\n' '--- setup-hooks.sh target ---'
sed -n '116,136p' scripts/setup-hooks.sh
printf '%s\n' '--- pre-commit CR hook ---'
sed -n '244,258p' .pre-commit-config.yaml
printf '%s\n' '--- legacy installer ---'
sed -n '18,52p' scripts/hooks/install-cr-pre-push-legacy.sh
printf '%s\n' '--- self-heal branch ---'
sed -n '344,420p' scripts/hooks/check-cr-before-push.sh
printf '%s\n' '--- hook installation order ---'
rg -n -C 4 'pre-commit install|install-cr-pre-push-legacy|setup-hooks|setup\.sh' \
scripts/setup.sh scripts/setup-hooks.sh scripts/hooks .pre-commit-config.yaml
printf '%s\n' '--- static return-path verifier ---'
python3 - <<'PY'
from pathlib import Path
installer = Path("scripts/hooks/install-cr-pre-push-legacy.sh").read_text()
checker = Path("scripts/hooks/check-cr-before-push.sh").read_text()
config = Path(".pre-commit-config.yaml").read_text()
assert "refusing to overwrite existing non-Himmel hook" in installer
assert 'exit 2' in installer
assert 'entry: bash scripts/hooks/check-cr-before-push.sh' in config
assert 'if ! bash "$SCRIPT_DIR/install-cr-pre-push-legacy.sh"' in checker
assert 'retry_hint="automatic install failed' in checker
assert 'exit 2' in checker[checker.index('retry_hint="automatic install failed'):]
print("foreign pre-push.legacy => installer refuses; pre-commit still invokes check-cr-before-push.sh; self-heal reports failure and refuses the push")
PYRepository: yotamleo/Himmel
Length of output: 33823
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- pre-commit recovery test outline ---'
rg -n -C 8 'foreign|non-Himmel|legacy|self-heal|automatic install|refuses|retry' \
scripts/hooks/test-check-cr-before-push-precommit.sh
printf '%s\n' '--- relevant test sections ---'
sed -n '1,240p' scripts/hooks/test-check-cr-before-push-precommit.sh
printf '%s\n' '--- both setup blocks and failure handling ---'
python3 - <<'PY'
from pathlib import Path
import re
for name, start, end in [
("scripts/setup.sh", "if ! bash \"$REPO_ROOT/scripts/hooks/install-cr-pre-push-legacy.sh\";", "fi"),
("scripts/setup-hooks.sh", "if ! bash \"$SCRIPT_DIR/hooks/install-cr-pre-push-legacy.sh\";", "fi"),
]:
text = Path(name).read_text()
match = re.search(re.escape(start) + r".*?" + re.escape(end), text, re.S)
assert match, f"missing failure-handling block in {name}"
block = match.group(0)
assert "Setup continues" in block or "setup continues" in block
print(f"{name}: installer failure is explicitly swallowed")
PYRepository: yotamleo/Himmel
Length of output: 22324
Do not swallow legacy-hook installation failures.
When a foreign pre-push.legacy hook exists, the installer exits 2 and cannot install the Himmel hook. The next push reaches check-cr-before-push.sh, but its retry fails again and the push remains rejected until manual hook merging. Propagate this failure from both setup paths, or install an explicit safe hook chain before reporting success.
📍 Affects 2 files
scripts/setup.sh#L226-L234(this comment)scripts/setup-hooks.sh#L124-L129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/setup.sh` around lines 226 - 234, Do not ignore failures from
install-cr-pre-push-legacy.sh in either setup path: update scripts/setup.sh
lines 226-234 and scripts/setup-hooks.sh lines 124-129 to propagate the
installer failure, or establish an explicit safe hook chain before reporting
setup success. Ensure both paths prevent claiming successful installation when a
foreign pre-push.legacy hook blocks the Himmel hook.
| // read-tree makes the snapshot HEAD-plus-changes, which is what "checkpoint" | ||
| // means. The mock suite cannot catch this (canned SHAs) — the real-git test | ||
| // asserting no `D` lines is the gate. | ||
| if (safe(["read-tree", "HEAD"], env).code !== 0) return { committed: false, reason: "read-tree failed" }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The unborn-HEAD path is unreachable, so the comments at Lines 916-917 and 925-927 describe behavior the code cannot produce.
git read-tree HEAD exits non-zero when HEAD is unborn. The guard at Line 898 then returns read-tree failed and aborts. The unborn-HEAD handling at Lines 918-921 and Lines 925-927 never runs, and an unborn repository never checkpoints.
If unborn HEAD must checkpoint, detect it and skip the seed. If unborn HEAD is out of scope, remove the unborn-HEAD claims from the comments so the stated contract matches the code.
🐛 Proposed fix to seed only when HEAD exists
- if (safe(["read-tree", "HEAD"], env).code !== 0) return { committed: false, reason: "read-tree failed" };
+ // An unborn HEAD has no tree to seed from; an empty index is then correct.
+ const hasHead = safe(["rev-parse", "--verify", "HEAD"]).code === 0;
+ if (hasHead && safe(["read-tree", "HEAD"], env).code !== 0) {
+ return { committed: false, reason: "read-tree failed" };
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/telegram/spawn-glm.ts` at line 898, Update the checkpoint logic
around the read-tree guard to explicitly detect whether HEAD exists before
calling `git read-tree HEAD`; skip the seed/read-tree step for an unborn HEAD so
the existing unborn-HEAD handling and checkpoint behavior can execute. Ensure
normal repositories still require a successful `read-tree HEAD`, and keep the
comments aligned with the resulting behavior.
1 participant
Wave 2k — orchestration wave 1 (23 tickets, 17 private merges)
Private range
e62aeb4d..dbdb9486, propagated with the fail-closed leak scan + byte-verify helper.Worker lifecycle / GLM lane
refs/checkpoints/*) + reap stale checkpoints.await-glm-worker --slugmust not resolve to a prefix-sibling; max-of-three liveness legs.arm-resume integrity
SCHTASKS_CMDseam.Dispatch guard
Harness + test hygiene
VERSION(0.1.0) +himmelctl --versionso orchestration measurements are attributable.jira listpast the server's silent 100 cap.CorsairLink.log..gitignoreitself is carved out of propagation (HIMMEL-1587) — the public copy redacts the state-repo name.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
himmelctl --versionreporting.Bug Fixes
Documentation