feat: [HIMMEL-1596][HIMMEL-1573][HIMMEL-1617] wave 2k — worker checkpointing, liveness that does not lie, dispatch-guard precision (19 PRs) - #554
Merged
Conversation
…ointing, liveness that does not lie, dispatch-guard precision (19 PRs) Propagates the 2026-08-06 wave-1 ship batch plus the 2026-08-07 mandated follow-ups: worker checkpoint + stale-checkpoint reaping (1596), the prefix-sibling await resolver + max-of-signals liveness (1347), process liveness as a fourth non-write signal so a confirmed-alive pid never reads STALLED (1573) with the mtime walk scoped to the worker's own worktree (1616) and a portable BSD/macOS mtime leg that degrades loudly (1614), the GLM critic timeout self-retry (1569), the funded-bank lane guard (1513), arm-registry integrity (1603/1579/1607/1331/1365/1606), the SCHTASKS_CMD seam (1610), squash-aware branch classifier (1600), jira list pagination past the silent 100 cap (1597), harness versioning (1598), shell-suite perf fixtures (1589), propagated-test fixture skips (1590), adopter fixes (1586), trust-case skew skip (1588), and dispatch-guard classifier precision — descriptive "fix" mentions and plan-shaped briefs no longer read as implementation intent while clause-head imperatives still block (1617). Docs: the bun-test cwd is per-suite (1615).
📝 WalkthroughWalkthroughThis PR adds conditional shell-suite filtering, checkpoint durability and cleanup, handover safety checks, lane observability, branch classification, Jira pagination, CLI version reporting, critic fallback configuration, atomic settings writes, and repository validation updates. ChangesCI and test infrastructure
Checkpoint lifecycle
Handover safety and scheduling
Dispatch guarding and atomic settings
Lane observability and liveness
Branch classification
Jira pagination and CLI versioning
Critic fallback configuration
Runtime portability
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
Owner
Author
|
@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: 19
🧹 Nitpick comments (15)
scripts/ci/test-run-shell-tests.sh (1)
517-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a matching untracked-path case.
The runner includes
git ls-files --others --exclude-standardoutput inchanged_set. Cases 12b and 12c only setGIT_FAKE_DIFF. Add a case that sets a matchingGIT_FAKE_UNTRACKEDpath and asserts thattest-propagate-public.shruns. This protects the untracked-path contract inscripts/ci/run-shell-tests.shLines 798-803.🤖 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/ci/test-run-shell-tests.sh` around lines 517 - 540, Add a new conditional-suite test near cases 12b/12c that sets GIT_FAKE_UNTRACKED to a path matching the propagation pattern, invokes RUNNER, and verifies success, the propagation sentinel, and absence of the “conditional: no changed path matches” message. Keep the setup and cleanup consistent with the existing sandbox cases.scripts/cr/test-critic-panel-fallback.sh (1)
716-724: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete shipped fallback contract.
The shipped row in
scripts/cr/critics.json, Line 5, setsfallback_modelsandfallback_provider. These checks only prove that the chain is non-empty and thatfallback_triggerisany. A wrong model or provider would still pass.Add exact assertions for
fallback_models[0] == "glm-5.2"andfallback_provider == "glm".Proposed assertions
+check "22: shipped glm row uses glm-5.2 as fallback model" \ + "$(node -e 'const r=require(process.argv[1]).panel.find(x=>x.slug==="glm");process.stdout.write(String(r&&r.fallback_models&&r.fallback_models[0]))' "$SHIPPED")" "glm-5.2" +check "22: shipped glm row uses glm fallback provider" \ + "$(node -e 'const r=require(process.argv[1]).panel.find(x=>x.slug==="glm");process.stdout.write(String(r&&r.fallback_provider))' "$SHIPPED")" "glm"Based on the supplied
scripts/cr/critic-panel.shcontext,fallback_provideris passed explicitly to each fallback attempt.🤖 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/cr/test-critic-panel-fallback.sh` around lines 716 - 724, Extend the shipped GLM fallback contract checks in the test script to assert that fallback_models[0] is exactly "glm-5.2" and fallback_provider is exactly "glm". Keep the existing non-empty chain and fallback_trigger=any assertions unchanged, using the shipped critics.json GLM row lookup already present in the checks.scripts/himmelctl/test/test-version.sh (1)
26-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the documented fallback.
The implementation returns
himmel unknownwith exit code 0 whenVERSIONis absent or unreadable. This test covers only a validVERSION. Add a temporary root withoutVERSIONand assert the fallback output and exit code.🤖 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/himmelctl/test/test-version.sh` around lines 26 - 31, Add a test case alongside the existing version check that uses a temporary HIMMELCTL_REPO_ROOT without a VERSION file, invokes bin.js --version, and asserts exit code 0 with output containing exactly the documented “himmel unknown” fallback. Keep the existing valid-VERSION assertions unchanged and clean up the temporary root afterward.scripts/hooks/guard-implementor-dispatch.sh (3)
346-348: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider running the bank-status probe once for both lanes.
lane_fundedruns the full probe per lane. The probe output already contains a line for every lane, and the command is the same for claudex and glm. When claudex is spent or the probe hangs, the hook pays the budget twice, up to 8 seconds by default, on a synchronous dispatch path.Cache the first successful probe output in a script-level variable and parse it for each lane.
🤖 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 346 - 348, Update the lane selection logic around lane_funded to execute the bank-status probe only once, cache its successful output in a script-level variable, and reuse that cached data when evaluating both claudex and glm. Adjust lane_funded or its callers to parse the cached per-lane status without rerunning the probe, while preserving the existing funded checks and fallback behavior.
303-309: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider stripping carriage returns in the probe parse loop.
read -r lid lstatekeeps a trailing\rwhen the probe writes CRLF. On Windows Git Bash,spent\rthen falls to the unrecognised-state branch and the lane is treated as funded. The direction is fail-open, so the guard stays safe, but the funded check stops working on that platform.The shipped
scripts/lanes/bank-status.tswrites LF, so this only affects a CRLF-producing override ofIMPL_GUARD_BANK_STATUS_CMD.🧹 Proposed CR tolerance
while IFS=' ' read -r lid lstate; do [ -n "$lid" ] || continue + lstate="${lstate%$'\r'}" if [ "$lid" = "$id" ]; then🤖 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 303 - 309, Update the probe parse loop in the guard script around `read -r lid lstate` to strip a trailing carriage return from the parsed lane state before comparing or branching on it. Preserve the existing lane ID matching and fail-open behavior while ensuring CRLF output still recognizes the funded state.
274-274: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueValidate
IMPL_GUARD_BANK_BUDGET_SECSbefore you use it in the loop test.Line 274 accepts the env value without a numeric check. Line 250 passes it to
[ ... -lt "$budget_secs" ]. If an operator sets a non-numeric or empty value,testwritesinteger expression expectedto stderr and returns non-zero. The loop then exits at once and the probe reports rc=124.The result stays fail-open, so the lane is still treated as funded. The stderr noise is the visible cost.
🧹 Proposed guard
- budget="${IMPL_GUARD_BANK_BUDGET_SECS:-4}" + budget="${IMPL_GUARD_BANK_BUDGET_SECS:-4}" + case "$budget" in + ''|*[!0-9]*) budget=4 ;; + esacAlso applies to: 250-250
🤖 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` at line 274, Validate IMPL_GUARD_BANK_BUDGET_SECS as a non-empty numeric value before assigning or using it as budget in the loop test around the budget assignment and the [ ... -lt "$budget_secs" ] check. When invalid or empty, fall back to the existing default budget so the test never receives a non-numeric operand and the funded fail-open behavior remains unchanged.scripts/hooks/test-guard-implementor-dispatch.sh (1)
572-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the duplicated
RC56andRC57result variables.Line 557 assigns
RC56and line 572 assigns it again. Line 560 assignsRC57and line 580 assigns it again. Each assertion runs directly after its assignment, so the current results are correct.The reuse breaks the unique-identifier convention that the rest of the suite follows. It also becomes a wrong-result trap if a later change inserts a case between an assignment and its assertion. Use the next free numbers, for example
RC59andRC60.🧹 Proposed rename
-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"Also applies to: 580-580
🤖 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` at line 572, Rename the duplicated result variables in the probe cases around `probe-descriptive-allows`: change the later `RC56` assignment to the next unused unique identifier (such as `RC59`) and the later `RC57` assignment to another unique identifier (such as `RC60`), updating their corresponding assertions while leaving the earlier cases unchanged.scripts/test-clean-garden-accounting.sh (1)
249-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree near-identical clean-garden runners now exist.
run_clean(Lines 140-151),run_clean_ttl, andrun_clean_race(Lines 361-371) repeat the same body: the stubPATHexport, theGH_ROWS_*exports,cd "$REPO", andbash "$CLEAN_GARDEN" --prune-only. They differ only in the extraPATHprefix and theCHECKPOINT_TTL_DAYShandling. A single runner that takes an extraPATHprefix and an optional TTL would remove the duplication and keep theshellcheck disablerationale in one place.This is test-only cleanup and can be deferred.
🤖 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/test-clean-garden-accounting.sh` around lines 249 - 262, Consolidate run_clean, run_clean_ttl, and run_clean_race into one shared runner that accepts an optional PATH prefix and CHECKPOINT_TTL_DAYS value. Centralize the stub PATH and GH_ROWS_* exports, repository change, prune-only invocation, and shellcheck suppression rationale, while preserving each caller’s existing environment behavior.scripts/telegram/spawn-glm.ts (2)
945-954: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a write-then-rename for the checkpoint merge.
mergeMetaCheckpointtruncatesmeta.jsonin place.await-glm-worker.shpolls this file, and the comment at Lines 990-994 states that a read landing mid-truncate can be misread. The terminal writes inexecuteRunalready use plainwriteFileSync, so this matches existing style and the run has ended by this point. Reusing the rename path would remove the last polling window.♻️ Proposed change
- const prior = JSON.parse(readFileSync(metaPath, "utf8")); - writeFileSync(metaPath, JSON.stringify({ ...prior, checkpoint }, null, 2)); + const prior = JSON.parse(readFileSync(metaPath, "utf8")); + const tmpPath = `${metaPath}.tmp-ckpt-${process.pid}`; + writeFileSync(tmpPath, JSON.stringify({ ...prior, checkpoint }, null, 2)); + renameSync(tmpPath, metaPath);🤖 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 945 - 954, Update mergeMetaCheckpoint to write the merged JSON to a temporary file and then rename it over metaPath, avoiding in-place truncation while await-glm-worker.sh may poll the file. Preserve the existing merge content, success return value, and non-fatal error handling.
854-860: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider distinguishing a signal-killed git from an unspawnable git.
gitCapturemapsexitCode == nullto-1.checkpointWorktree.safealso uses-1for a thrown spawn. Astatuscall killed by a signal therefore reportsreason: "git unavailable", which is inaccurate for diagnostics.gitCapturealso dropsstderr, so no failure detail reaches the reason strings.Both are diagnostic-only. The checkpoint stays best-effort and the exit code is unaffected.
🤖 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 854 - 860, Update gitCapture to distinguish a signal-terminated git process from a spawn failure instead of mapping both to -1, and preserve the relevant stderr or termination detail in its returned diagnostic data. Adjust checkpointWorktree.safe’s reason mapping to report “git unavailable” only for genuine spawn failures and use an accurate signal/failure reason otherwise, without changing checkpoint best-effort behavior or exit-code handling.scripts/telegram/spawn-glm.test.ts (3)
2009-2030: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
githelper in this test ignores exit codes, so two assertions can pass vacuously.Unlike the helpers at Lines 1959-1963 and 2048-2052, the helper at Line 2009 returns
stdoutwithout checkingexitCode. Ifgit branch --listorgit for-each-reffails, both return an empty string, and the containment assertions at Lines 2029-2030 pass with no ref data examined. The setup calls (init,config,commit) also fail silently and surface later as a confusingsha2 === sha1mismatch.Reuse the checking helper shape used by the other two real-git tests.
♻️ Proposed change
- const git = (...a: string[]) => Bun.spawnSync(["git", "-C", tmp, ...a], { stdout: "pipe", stderr: "pipe" }).stdout.toString(); + const git = (...a: string[]) => { + const r = Bun.spawnSync(["git", "-C", tmp, ...a], { stdout: "pipe", stderr: "pipe" }); + if (r.exitCode !== 0) throw new Error(`git ${a.join(" ")}: ${r.stderr.toString()}`); + return r.stdout.toString(); + };🤖 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.test.ts` around lines 2009 - 2030, Update the local git helper used by the test around checkpointWorktree to validate Bun.spawnSync exitCode before returning stdout, matching the checking helper shape used by the neighboring real-git tests. Ensure setup commands and branch/ref inspection fail explicitly on git errors instead of returning empty output.
252-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelax the source-text pin and use one reader alias.
The regex at Line 254 matches an exact source line, including spacing and the trailing comma. A reformat breaks the test with no behavior change. Two independent key assertions keep the same intent and survive formatting.
This test uses
readFileSync, while the wiring pin at Line 2089 uses_rffor the same file. Pick one alias for consistency.♻️ Proposed change
- const src = readFileSync("scripts/telegram/spawn-glm.ts", "utf8"); - expect(/worktree: absCwd, branch, worker_worktree: worktree,/.test(src)).toBe(true); + const src = _rf("scripts/telegram/spawn-glm.ts", "utf8"); + expect(src).toMatch(/worktree:\s*absCwd\b/); + expect(src).toMatch(/worker_worktree:\s*worktree\b/);🤖 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.test.ts` around lines 252 - 255, Update the wiring-pin test around the worker_worktree assertion to avoid matching the exact formatted source line: assert the relevant worktree and worker_worktree mappings independently so harmless reformatting does not break the test. Use the same readFileSync alias as the wiring pin near the existing _rf usage, consistently replacing the separate reader alias.
1964-1974: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider isolating the fixture repositories from ambient Git configuration.
The fixtures set
user.nameanduser.email, but a globalcommit.gpgsign=true,core.hooksPath, orinit.templateDirstill applies togit commit -qm "base". That makes the three real-git tests fail on some operator machines for reasons unrelated tocheckpointWorktree. Addinggit config commit.gpgsign falseandgit config core.hooksPath /dev/nullto each fixture removes that dependence.
git init -q -b mainalso requires Git 2.28 or later. Confirm that matches the supported toolchain.🤖 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.test.ts` around lines 1964 - 1974, Isolate each fixture repository in the real-Git tests from ambient configuration by configuring commit.gpgsign=false and core.hooksPath=/dev/null before the base commit, alongside the existing user settings. Apply this consistently to all three fixture setup blocks, and verify that the project’s supported Git toolchain includes the 2.28+ requirement imposed by git init -b main.scripts/clean-garden.sh (1)
859-872: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe TTL warning prints even when pruning is disabled.
The
caseguard runs at script top level.prune_checkpoint_refsruns only whenNO_PRUNEis0(Line 911). WithNO_PRUNE=1and an invalidCHECKPOINT_TTL_DAYS, the script still prints the warning for a knob it never uses.Move the validation into
prune_checkpoint_refsso the warning tracks the code path that reads the value.♻️ Proposed change
prune_checkpoint_refs() { local cutoff now ref ts oid pruned=0 kept=0 + case "$CHECKPOINT_TTL_DAYS" in + ''|*[!0-9]*) + echo "WARN clean-garden: CHECKPOINT_TTL_DAYS=\"$CHECKPOINT_TTL_DAYS\" is not a non-negative decimal — using 14" >&2 + CHECKPOINT_TTL_DAYS=14 + ;; + esac now=$(date +%s)🤖 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/clean-garden.sh` around lines 859 - 872, Move the CHECKPOINT_TTL_DAYS case validation and fallback warning from top-level script execution into prune_checkpoint_refs, before that function performs TTL arithmetic or reads the value. Keep the existing validation behavior unchanged, and ensure no warning is emitted when pruning is disabled via NO_PRUNE.scripts/handover/test-arm-resume.sh (1)
2951-2956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
unregistered-armtelemetry event.The test sets
SKILL_TELEMETRY_DIRbut checks only stderr. Assert thatskill-usage.jsonlcontains"event":"unregistered-arm"so removal of thetelemetry_emitcall fails the test.🤖 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 2951 - 2956, Extend the unresolvable-root test around the ARM invocation to inspect the telemetry file under the T1603 directory, and assert it contains the event value "unregistered-arm". Keep the existing stderr assertions, ensuring the test fails if the telemetry_emit call is removed.
🤖 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/cr/test-critic-panel-fallback.sh`:
- Around line 675-678: Update the timeout setup near the critic fallback test to
select a command that supports -k, preferring timeout and falling back to
gtimeout when necessary. Reuse this resolved command in all timeout invocations
and test guards, including the critic execution path, so execution remains
bounded and fails clearly when neither compatible backend is available.
In `@scripts/git/classify-branches.sh`:
- Around line 154-160: Update the blob classification logic around the
current-tree lookup and the historical search in
scripts/git/classify-branches.sh lines 154-160 so a historical-only match cannot
produce LANDED; only a matching blob in the current BASE tree or evidence from a
merged PR may do so, otherwise return UNKNOWN. Add a regression fixture in
scripts/git/test-classify-branches.sh lines 128-135 where a branch reverts a
post-merge-base change to an older blob, and assert the verdict is not LANDED.
- Around line 233-240: Resolve the base ref to its short local branch name
before the filtering loop compares branches against BASE, so --base
refs/heads/main excludes main correctly; update scripts/git/classify-branches.sh
lines 233-240 around the branch filter. Add a regression case in
scripts/git/test-classify-branches.sh lines 147-153 that invokes --base
refs/heads/main and asserts main is absent.
In `@scripts/handover/arm-resume.sh`:
- Around line 2871-2892: Gate the stale .bat pruning command near the temp-file
creation so it runs only for a real arm, not when DRY_RUN is enabled. Preserve
the existing age, filename, and best-effort conditions, while ensuring the
--dry-run path performs no deletion before its later early return.
- Around line 2449-2455: Preserve the handover ticket value before it is unset
near the preflight setup, so the shipped-work check can use it when evaluating
`${_ho_ticket:-}`. Update the status cases in the `_arm_probe` block to match
Jira’s exact names, including `Won't Do` and `Won't Fix`, while retaining the
existing Done and Closed handling.
In `@scripts/handover/test-arm-resume.sh`:
- Around line 126-139: Prime the future_time cache in the parent shell
immediately after defining future_time by invoking it without command
substitution. At every call site that compares or otherwise requires two
future_time results to match, especially the V4, V4B, and V6 identity/verify
tests, capture one returned value in a variable and reuse it instead of invoking
future_time separately; preserve the existing T23_TIME pattern.
- Around line 2896-2912: Update the test cases around _a1331 so environment
overrides such as POSIX_CORRECT, ARM_SHIPPED_OK, and GHC_1331 are restored after
each case, including when VAR=value _a1331 leaves them set under Bash 3.2.
Preserve any pre-existing values, restore them after cases (g) and (h), and
avoid unconditional unsetting so subsequent cases exercise their intended paths.
In `@scripts/himmelctl/test/test-version.sh`:
- Around line 35-37: Update the --help check around the node invocation to
capture its exit status before running grep, then require that status to be zero
in addition to verifying the output contains “usage.” Keep the existing failure
message or provide equivalent failures, matching the status-checking pattern
used by the version check.
In `@scripts/hooks/guard-implementor-dispatch.sh`:
- Line 141: Update the normalization substitutions in the guard script: make the
fixed-word strip match only the standalone word fixed, and make the infinitive
verb strip require a word boundary before to while preserving its existing verb
alternatives. Apply these changes to the substitutions near the fixed cleanup
and imperative_verb extraction, without altering unrelated classification
behavior.
In `@scripts/hooks/narrow-allow.mjs`:
- Line 14: Update the atomic replacement flow in the hook to read the existing
target file mode while holding the lock, apply that mode to the staged file
before renameSync, and preserve current behavior when the target is absent. Add
a regression test covering replacement of a 0600 settings file and verifying the
resulting mode remains 0600.
In `@scripts/jira/src/commands/list.ts`:
- Around line 99-114: Update the pagination loop around the request in the list
command to track every requested continuation token and fail immediately when a
token is reused, preventing duplicate-page requests. Preserve the existing
empty-page guard and normal token-based termination, and add coverage for a
repeated token returned alongside non-empty issues.
- Around line 91-117: Update the MCP list command to use the existing
searchAllIssues helper instead of making a single /search/jql request,
preserving the requested limit for cursor-based pagination beyond 100 results.
Add MCP coverage verifying that limits above 100 walk multiple pages and return
the requested results.
- Around line 96-97: Update the limit parsing around want and target to validate
the complete trimmed input rather than using Number.parseInt, accepting only
safe positive integers; otherwise retain the default of 25. Add tests covering
malformed values, decimals, and unsafe integers.
In `@scripts/lanes/bank-status.ts`:
- Around line 83-94: Update the bankCache handling in the lane loop to cache
failed readBank attempts as an explicit failure/unknown sentinel. Reuse that
cached sentinel for subsequent lanes sharing the same bank, ensuring readBank
executes at most once per bank while failed reads still produce the existing
unknown state.
- Around line 67-72: Update the bank source-path resolution around claudeCache,
codexSessions, and glmLedger to honor configured quota_sources overrides for
claude_cache_path, codex_sessions_dir, and glm_ledger_path, matching
flow-exporter’s effective resolver. Reuse or share that resolver, or pass the
configured paths into this command, and add a test covering non-default source
paths so both components read the same files.
- Around line 58-61: Update the MAX_PCT initialization to accept only finite
LANЕ_FUNDED_MAX_PCT values within the valid quota range of 0 through 100; reject
negative or greater-than-100 values and fall back to 99. Preserve the existing
environment parsing and default behavior.
- Line 82: Validate the result of readLaneQuotaTargets before iterating over its
lanes so a non-iterable lanes value is handled as an unknown status rather than
throwing during startup. Update the initialization or surrounding startup flow
in bank-status.ts to detect the invalid registry shape and preserve the
documented unknown behavior, while leaving valid per-bank processing unchanged.
In `@scripts/lanes/tests/test-await-liveness.sh`:
- Line 45: Update the fixture timestamp setup around find to use portable POSIX
touch -t with a fixed timestamp instead of GNU-only touch -d, and remove the
failure suppression so timestamp-update errors are propagated rather than
leaving files fresh. Preserve the recursive update of all files under wt used by
the idle liveness cases.
In `@scripts/lib/scheduler-backend.sh`:
- Line 39: Update scheduler_backend_remediation for the windows backend to
handle missing SCHTASKS_CMD, returning a remediation message that names the
probed scheduler binary and indicates it came from SCHTASKS_CMD, matching the
diagnostic wording used by arm-resume.sh. Preserve the existing no-op behavior
when the Windows scheduler is available.
---
Nitpick comments:
In `@scripts/ci/test-run-shell-tests.sh`:
- Around line 517-540: Add a new conditional-suite test near cases 12b/12c that
sets GIT_FAKE_UNTRACKED to a path matching the propagation pattern, invokes
RUNNER, and verifies success, the propagation sentinel, and absence of the
“conditional: no changed path matches” message. Keep the setup and cleanup
consistent with the existing sandbox cases.
In `@scripts/clean-garden.sh`:
- Around line 859-872: Move the CHECKPOINT_TTL_DAYS case validation and fallback
warning from top-level script execution into prune_checkpoint_refs, before that
function performs TTL arithmetic or reads the value. Keep the existing
validation behavior unchanged, and ensure no warning is emitted when pruning is
disabled via NO_PRUNE.
In `@scripts/cr/test-critic-panel-fallback.sh`:
- Around line 716-724: Extend the shipped GLM fallback contract checks in the
test script to assert that fallback_models[0] is exactly "glm-5.2" and
fallback_provider is exactly "glm". Keep the existing non-empty chain and
fallback_trigger=any assertions unchanged, using the shipped critics.json GLM
row lookup already present in the checks.
In `@scripts/handover/test-arm-resume.sh`:
- Around line 2951-2956: Extend the unresolvable-root test around the ARM
invocation to inspect the telemetry file under the T1603 directory, and assert
it contains the event value "unregistered-arm". Keep the existing stderr
assertions, ensuring the test fails if the telemetry_emit call is removed.
In `@scripts/himmelctl/test/test-version.sh`:
- Around line 26-31: Add a test case alongside the existing version check that
uses a temporary HIMMELCTL_REPO_ROOT without a VERSION file, invokes bin.js
--version, and asserts exit code 0 with output containing exactly the documented
“himmel unknown” fallback. Keep the existing valid-VERSION assertions unchanged
and clean up the temporary root afterward.
In `@scripts/hooks/guard-implementor-dispatch.sh`:
- Around line 346-348: Update the lane selection logic around lane_funded to
execute the bank-status probe only once, cache its successful output in a
script-level variable, and reuse that cached data when evaluating both claudex
and glm. Adjust lane_funded or its callers to parse the cached per-lane status
without rerunning the probe, while preserving the existing funded checks and
fallback behavior.
- Around line 303-309: Update the probe parse loop in the guard script around
`read -r lid lstate` to strip a trailing carriage return from the parsed lane
state before comparing or branching on it. Preserve the existing lane ID
matching and fail-open behavior while ensuring CRLF output still recognizes the
funded state.
- Line 274: Validate IMPL_GUARD_BANK_BUDGET_SECS as a non-empty numeric value
before assigning or using it as budget in the loop test around the budget
assignment and the [ ... -lt "$budget_secs" ] check. When invalid or empty, fall
back to the existing default budget so the test never receives a non-numeric
operand and the funded fail-open behavior remains unchanged.
In `@scripts/hooks/test-guard-implementor-dispatch.sh`:
- Line 572: Rename the duplicated result variables in the probe cases around
`probe-descriptive-allows`: change the later `RC56` assignment to the next
unused unique identifier (such as `RC59`) and the later `RC57` assignment to
another unique identifier (such as `RC60`), updating their corresponding
assertions while leaving the earlier cases unchanged.
In `@scripts/telegram/spawn-glm.test.ts`:
- Around line 2009-2030: Update the local git helper used by the test around
checkpointWorktree to validate Bun.spawnSync exitCode before returning stdout,
matching the checking helper shape used by the neighboring real-git tests.
Ensure setup commands and branch/ref inspection fail explicitly on git errors
instead of returning empty output.
- Around line 252-255: Update the wiring-pin test around the worker_worktree
assertion to avoid matching the exact formatted source line: assert the relevant
worktree and worker_worktree mappings independently so harmless reformatting
does not break the test. Use the same readFileSync alias as the wiring pin near
the existing _rf usage, consistently replacing the separate reader alias.
- Around line 1964-1974: Isolate each fixture repository in the real-Git tests
from ambient configuration by configuring commit.gpgsign=false and
core.hooksPath=/dev/null before the base commit, alongside the existing user
settings. Apply this consistently to all three fixture setup blocks, and verify
that the project’s supported Git toolchain includes the 2.28+ requirement
imposed by git init -b main.
In `@scripts/telegram/spawn-glm.ts`:
- Around line 945-954: Update mergeMetaCheckpoint to write the merged JSON to a
temporary file and then rename it over metaPath, avoiding in-place truncation
while await-glm-worker.sh may poll the file. Preserve the existing merge
content, success return value, and non-fatal error handling.
- Around line 854-860: Update gitCapture to distinguish a signal-terminated git
process from a spawn failure instead of mapping both to -1, and preserve the
relevant stderr or termination detail in its returned diagnostic data. Adjust
checkpointWorktree.safe’s reason mapping to report “git unavailable” only for
genuine spawn failures and use an accurate signal/failure reason otherwise,
without changing checkpoint best-effort behavior or exit-code handling.
In `@scripts/test-clean-garden-accounting.sh`:
- Around line 249-262: Consolidate run_clean, run_clean_ttl, and run_clean_race
into one shared runner that accepts an optional PATH prefix and
CHECKPOINT_TTL_DAYS value. Centralize the stub PATH and GH_ROWS_* exports,
repository change, prune-only invocation, and shellcheck suppression rationale,
while preserving each caller’s existing environment behavior.
🪄 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: 006602b0-a772-4842-9405-ebcbdff22d04
📒 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
…MP_CWD_OK shield The HIMMEL-1365 temp-target refusal (exit 12) fires on the suites' own mktemp fixtures; test-arm-resume.sh declared the opt-out, these two were never updated — 30 + 7 failures, first caught by this PR's shell-unit run (the private repo runs no Actions by design). Same one-line declared shield in each, matching the main suite's pattern.
…k suite shield, verify-return instrument, sort -V retirement Carries the private merges since this PR's last reship: the seven-fix HIMMEL-1624 batch answering this PR's own CodeRabbit findings (guard strip word-boundaries, portable fixture backdating, funded-pct full-string parsing, jira repeated-token guard, dry-run prune gate, --help rc assert, fail-closed settings-mode preservation), the HIMMEL-1623 ARM_TEMP_CWD_OK shield for the queue-lock arm suite (same class as HIMMEL-1622), HIMMEL-1621's standalone lane-return verifier, and HIMMEL-1054's sort -V retirement.
…eld (unblocks this PR's shell-unit) The shell-unit red on this PR is not a wave-2k regression: the suite fails 59/65 on clean private main too. Root cause is a fixture gap — the HIMMEL-1365 guard in arm-resume.sh refuses to arm (exit 12) when RESUME_CWD resolves under a TEMP path unless ARM_TEMP_CWD_OK=1 is exported; this suite's fixture points resume_cwd at a mktemp -d repo but never exported the shield, unlike all four sibling arm suites (HIMMEL-1622/1623). T2/T6/T9 are exactly the three cases that reach a full arm-resume invocation. Fix: the same 6-line export block the siblings carry. Suite 65/65 (was 59/65); refusal classes intact. Private: PR #1620, merged (008f5238 squashed as 0e37cac4).
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/check-plugin-drift.sh`:
- Line 192: Update the version-parsing flow using highest_version so its Python
interpreter matches the command provided by scripts/setup.ps1 on Windows:
resolve python3 or fall back to python once, then reuse that selected command
for all parser calls and avoid reporting INCOMPLETE when only python is
available.
In `@scripts/lanes/verify-return.mjs`:
- Around line 41-43: Update the rev-list count parsing in the verification flow
to validate that the complete trimmed output is a decimal count before
converting it and evaluating n > 0; reject partially numeric values such as
“3junk” through the existing cannot-evaluate error path. Add a test covering
partially numeric rev-list output.
- Around line 140-143: Validate the parsed branch value in the argument-handling
flow before verification and before it reaches the stdout write at Line 158,
rejecting any branch containing ASCII whitespace or control characters with the
existing exit-code-2 error path. Preserve valid branch names and the existing
missing-branch and unexpected-argument handling.
In `@scripts/test-check-plugin-drift.sh`:
- Around line 104-105: Remove the eval-based extraction of highest_version in
the test script; move highest_version into a side-effect-free sourceable
library, then source and reuse that library from both scripts while preserving
the existing pick and version-selection behavior.
🪄 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: e00dcdf9-e35f-495d-82d5-a26d32ec72ef
📒 Files selected for processing (21)
scripts/check-plugin-drift.shscripts/handover/arm-resume.shscripts/handover/test-arm-resume-identity.shscripts/handover/test-arm-resume-long-gap.shscripts/handover/test-arm-resume-queue-lock.shscripts/handover/test-arm-resume.shscripts/handover/test-worker-lifecycle.shscripts/himmelctl/test/test-version.shscripts/hooks/guard-implementor-dispatch.shscripts/hooks/narrow-allow.mjsscripts/hooks/narrow-allow.test.mjsscripts/hooks/test-guard-implementor-dispatch.shscripts/jira/src/commands/list-pagination.test.tsscripts/jira/src/commands/list.tsscripts/lanes/bank-status.tsscripts/lanes/funded-max-pct.mjsscripts/lanes/tests/funded-max-pct.test.mjsscripts/lanes/tests/test-await-liveness.shscripts/lanes/tests/verify-return.test.mjsscripts/lanes/verify-return.mjsscripts/test-check-plugin-drift.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/lanes/bank-status.ts
- scripts/hooks/guard-implementor-dispatch.sh
- scripts/hooks/narrow-allow.mjs
- scripts/himmelctl/test/test-version.sh
- scripts/hooks/test-guard-implementor-dispatch.sh
- scripts/handover/arm-resume.sh
yotamleo
added a commit
that referenced
this pull request
Aug 7, 2026
…, session telemetry, alert rules, review-freshness gate, PHI fence hardening (8 PRs) (#556) ## HIMMEL-654 leg-3 batch (8 private PRs) Propagates the 2026-08-07 dispatch-session leg-3 merges (private #1612, #1614–#1620) onto the freshly-merged wave-2k base: | Ticket | What lands here | |---|---| | HIMMEL-1575 | GLM startup-hang fail-fast: transcript-growth watchdog + `failure_class="startup-hang"` accounting (`GLM_STARTUP_FAILFAST_MINS`, 0=off) | | HIMMEL-1626 | Guard lane-READINESS measured from the verify-return ledger: `scripts/lanes/lane-readiness.mjs`, `lanes.json` `readiness.passesRequired` gates (glm 10 / claudex 5), `lane_ready()` in the dispatch guard — a ruled-down lane is skipped like a spent bank; fail-open on every probe failure | | HIMMEL-1629 | cli-proxy-api pin 7.2.115 → 7.2.122 (mechanical fork-drift bump) | | HIMMEL-1052 | Live session + subagent telemetry: session-runs ledger + hook entrypoint, six exporter families, `/sessions.json`, war-room Sessions row (exporter stays a pure reader) | | HIMMEL-845 | Cross-drive `X:tail` fail-open closed FAIL-CLOSED in both PHI fence twins + lockstep regression tests | | HIMMEL-924 | Nine alert rules + Telegram delivery (both Prometheus and Grafana-provisioning dialects), HIMMEL-918 coverage invariant, promtool suite | | HIMMEL-1181 | Review-freshness gate: stale bot review anchor hard-blocks both check-ci modes (exit 4) + the merge gate (rc 2); body-only findings surfaced with counts, never blocking | | HIMMEL-1631 | Already on public main via the #554 reship — carried as a fail-closed verified `--skip-path` | Every item passed the private gate (critic panel, per-suite runs, check-ci) and merged to private main today. Public notes: the HIMMEL-1632 hardening batch (from #554's public CR pass) is tracked privately and will follow the same private-first path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added review-freshness checks to CI and merge validation. * Added lane readiness checks before dispatching work. * Added session and subagent monitoring, dashboards, and a sessions endpoint. * Added Prometheus and Grafana alerts with Telegram notifications. * Added startup-hang detection for inactive workers. * **Bug Fixes** * Guardrails now safely reject ambiguous Windows cross-drive relative paths. * **Documentation** * Updated CI, enforcement, observability, and configuration guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 2k
Propagates private main
9891d5ea..dbdb9486to the public mirror — the2026-08-06 wave-1 ship batch (13 PRs), two precursor fixes (#1588/#1589), and
the 2026-08-07 mandated follow-ups (4 PRs) — 19 squash-merged PRs in one
conventional commit.
Highlights
refs/checkpoints/*on run end + stale-checkpoint reaping--slugresolve; max-of-signals; a CONFIRMED-alive pid never reads STALLED (write-side signals cannot see a READ/PLAN phase); mtime walk scoped to the worker's own worktree, not the dispatch CWD; portable BSD/macOS mtime leg that degrades loudlySCHTASKS_CMDseamVERSION+himmelctl --version; fsync-off test fixtures; fixture-gated propagated tests; adopter fixesscripts/telegramruns from the repo rootVerification
Every PR in the range carried the private CR gate: cross-model critic panel
(+ CodeRabbit App threads resolved), suite evidence in each PR body, and the
HIMMEL-1573 fix additionally carries a live §2.8 acceptance re-run (a real
31.5-minute dispatch at the default stall window, zero false STALLED).
Carve-outs
.gitignoreis excluded from propagation (HIMMEL-1587 — the public copyredacts the state-repo name), so the
.gitignore-only private PR #1590 has nopublic payload beyond its ticket appearing here.
Summary by CodeRabbit
New Features
himmelctl --version.Bug Fixes
Documentation