ci: make required checks actually gate auto-merge - #186
Merged
Conversation
dizhaky
marked this pull request as ready for review
August 12, 2026 15:34
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dizhaky
marked this pull request as draft
August 12, 2026 16:04
9 tasks
dizhaky
added
the
ci-reviewed
Claude
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 12, 2026
* fix: repair two NameError crashes in the Python suite Both are environment-independent source bugs — code paths that could never have run — surfaced while investigating why main's Python suite is red (six of eight CI slices), which blocks the aggregate gate in #186. plugins/memory/mem0: _start_prefetch(self, query) computed `effective_user_id = user_id or self._user_id`, but user_id is not one of its parameters, so any call raised NameError. It is a copy of the identical line in sync_turn fifty lines below, where user_id genuinely is a parameter. The value was never read — the closure filters via self._read_filters() — so the line is deleted rather than the parameter added. hermes_cli/gateway: the launchd plist builder assembled `core_args`, computed a `use_wrapper` flag, then called `prog_args.append(...)` — a name never assigned. All three were mutually inconsistent, so the function could not run to completion. The surrounding comment is unambiguous about intent (the wrapper execs the python + module args passed after it; fall back to direct-python when it is missing) and the three failing tests assert nothing about wrapper shape, so the comment is the spec: the wrapper path goes in front of core_args when usable, core_args alone otherwise. Deliberately not included: the tests importing _expand_value_from_environ and _redact_command_for_display, neither of which exists. Implementing them means inventing semantics in a security-adjacent config loader, and whether they were removed or never written is not answerable from a shallow clone. Also records a correction in the system log: my earlier "69 failing tests" figure was this sandbox, not the repo. Most of it is missing optional extras and a different aiohttp than uv.lock, and some of those failures would be wrong to "fix" — the lazy-deps pin test is correctly detecting local drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix: unshadow the gateway command-line matcher, repair two test harnesses Second instalment of the Python-suite repair. CI confirmed the first: slice 6 went 5 files/20 tests -> 4 files/17 tests with test_mem0_v3.py gone. This clears the remaining four files in that slice. gateway/status.py had TWO definitions of looks_like_gateway_command_line — the canonical one at line 444 delegating to _gateway_command_subcommand (shlex tokenization, profile-selector stripping, case-folding), and a hand-rolled substring scan at line 569. Python keeps the last, so every caller silently got the naive one. looks_like_gateway_runtime_command_line was duplicated the same way. The substring version fails precisely the cases the canonical one exists for: `--profile work gateway run` (the flag splits the substring), `--profile gateway gateway run` (the profile value shadows the subcommand), bare `gateway`, and `GATEWAY RUN`. _scan_venv_blockers' docstring warns about this in as many words, so the regression it describes had reappeared one module over. Deleting the duplicates restores the canonical behaviour unchanged — 14 tests across two files. test_kanban_db referenced bare `hermes_state` at line 907 while line 922 does a plain `import hermes_state`, making the name function-local for the whole body, so the earlier reference could never resolve. It was redundant anyway: line 903 already clears that same set via the _hermes_state alias. test_modal_snapshot_isolation died before reaching its subject: modal.py calls lazy_deps.ensure("terminal.modal") first, and that asks whether the distribution is installed, which a sys.modules stub cannot satisfy. The test already stubs hermes_cli, tools, tools.environments and modal, so lazy_deps is one more stub, not the thing under test. The production check is deliberately untouched — it is security-adjacent, and loosening it to accept pre-imported modules is not a change to make for a test's convenience. Also corrects a claim in the log: I said test_gateway_command_line_matcher fails in CI but passes locally, and used it to argue the environments disagree. It fails locally too — verified by stashing the fix. It was missing from my inventory because the background capture kept only the tail of the full-suite output, so the file list was truncated. No regressions: tests/gateway/ + tests/hermes_cli/ run 9,102 passing / 24 failing across 12 files, none of them files touched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix: define the cron staleness check, stop provider labels collapsing to ids cron/scheduler: gateway/run.py's cron-ticker supervisor imports get_ticker_heartbeat_age and ticker_heartbeat_is_stale from cron.scheduler. Neither existed there — the age reader lives in cron/jobs.py and the staleness predicate was defined nowhere in the repo. The import sits inside the supervisor loop, so the watchdog meant to restart a wedged ticker would itself have died on ImportError the first time it checked. Added the predicate and re-exported the age reader beside it, since the supervisor and its tests both treat cron.scheduler as the ticker's public surface. The rule is taken from the existing docstrings rather than invented: age > interval * stale_multiplier, and an unknown age is NOT stale — get_ticker_heartbeat_age returns None on a missing or torn read and documents that callers treat that as "cannot determine", not "dead". Restarting on a torn read would turn a filesystem hiccup into a restart loop. hermes_cli/providers: get_label("xai") returned "xai", which collapsed the xAI API-key entry onto its own id and made it indistinguishable from the OAuth sibling in the model picker. My first fix was wrong and the test caught it — I added a fallback at the end of get_label, but that line is unreachable for this case: get_provider returns a synthesized def for Hermes-only providers, with name=_LABEL_OVERRIDES.get(canonical, canonical), so the id was already baked in. Fixed at that source via one _display_name() helper shared by both call sites: overrides, then the registry in hermes_cli.models (which already carries ProviderEntry("xai", "xAI", ...)), then the id. Not fixed by adding an _LABEL_OVERRIDES entry: that duplicates a name the registry owns, which is exactly the duplication behind the gateway/status.py bug in the previous commit. models.dev is a remote catalog, so this path is also what every provider hits whenever it is unreachable. No regressions: 774 tests across 90 provider/model files pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * docs(system-log): record the missing config-integrity cluster Five names are imported from hermes_cli.config and none exist; two of them by production code in hermes_cli/backup.py. The restore path swallows the ImportError, so a config restore silently never reseals the integrity baseline and the watchdog reads an authorized restore as tampering. The machinery exists unwired in skills/devops/config-integrity-watchdog/, whose seal(quiet=True) parameter is documented for exactly this caller. Reconstructing it is four functions of security-adjacent behaviour, so it is reported for a decision rather than folded into a make-CI-green pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(slack): assign as_yaml, add the channels:join bot scope hermes_cli/slack_cli.py used `as_yaml` in two places — the render call and the file extension — without ever assigning it, so `hermes slack manifest` raised NameError on every invocation. Every other option in that function is read via getattr(args, ..., default); as_yaml now is too, defaulting to False (JSON), which is what the default write target (slack-manifest.json) and the test's json.loads(stdout) both assume. The generated app manifest also omitted the channels:join bot scope, so `hermes slack invite --all` would fail per-channel with missing_scope on an otherwise valid install — conversations.join requires it. Not fixed here: test_gateway_platform_gating asserts _builtin_setup_fn returns bespoke setup functions for telegram/slack/matrix. Those return None by design — all three moved into plugins/platforms/*/adapter.py, and I verified each registers setup_fn=interactive_setup, so the invariant the test documents (they must not fall through to the generic wizard, which would break Matrix's empty-token password login) still holds. The test asserts the removed route rather than the invariant; replacing it properly needs the plugin registry's lookup API, so it is documented rather than deleted to make CI green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(config): restore the missing config-integrity cluster Eight functions were referenced by tests and production but did not exist in hermes_cli/config.py: get_config_lock_path, get_config_seal_path, config_write_lock, config_read_lock, seal_config, verify_config_integrity, _reseal_git_backed_integrity_baseline and restore_config. Three test files failed at collection because of it, and hermes_cli/backup.py imported two of them behind `except ImportError: pass` — so a config restore silently never resealed and the watchdog read it as tampering. They were deleted, not never written: config.py still imports `fcntl as _fcntl` and `msvcrt as _msvcrt` at module scope and uses neither, which is exactly what the lock helpers needed, and hermes_cli/config_integrity_cli.py still provides the _find_core_module bridge the tests import. save_config() now seals both baselines after a successful write — the local config.yaml.sha256 sidecar and the git-backed JSONL log in the operator's dotfiles repo. A write through save_config is authorized by definition, so leaving those stale is what made the watchdog flag the model scanner. Decisions worth calling out. A missing config and a missing seal both verify OK: absence of a baseline is not a mismatch, and failing there would flag every pre-existing install as compromised. Sealing never fails a write — the git-backed half shells into a repo outside Hermes' control that can be absent, non-git, mid-rebase or read-only. That reseal is a no-op unless the dotfiles directory already exists, since the skill's seal() would otherwise mkdir a baseline on machines that never opted in. Locks degrade to no-lock rather than raising, and Windows readers go unlocked because msvcrt has no shared mode. restore_config always writes a pre-restore backup, including when the live file is unreadable — a corrupt config is the usual reason a restore is happening. _expand_value_from_environ leaves an unset reference verbatim rather than blanking it, so a typo'd variable stays legible instead of becoming an empty API key. That one rule also resolves the apparent contradiction in its tests: $TEST_VAR expands and literal-$var does not, purely because one name is exported and the other is not. It is not wired into config loading — _expand_env_vars walks whole documents and resolves ${env:NAME} SecretRefs with different warning behaviour, so delegating would change semantics rather than restore a helper. Verified against the same baseline command as before: tests/gateway/ + tests/hermes_cli/ go from 9,102 passing / 24 failing across 12 files to 9,139 passing / 11 failing across 8, with collection errors down from 3 to 1. Strictly better on every axis, so the new sealing side effect regressed nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(config): open the lock file in binary mode Ruff (PLW1514) and the Windows-footgun checker both flagged the new `open(lock_path, "a+")`: text mode without an explicit encoding, which on Windows also brings newline translation. The file is only ever a lock handle — never read or written as text — so binary sidesteps both concerns rather than papering over them with an encoding argument. Both checks pass locally: `ruff check hermes_cli/ cron/ gateway/` is clean and `scripts/check-windows-footguns.py --all` reports no footguns across 927 files. Unrelated, for the record: the `gitleaks (diff)` failure on 25b74f9 is not a secret. Its install step died on `curl: (22) ... error: 503` fetching the release tarball, so the scanner never ran — but the job's failure-notice step prints "gitleaks flagged a secret in this diff" unconditionally, making a transient CDN failure look identical to a real detection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(model-metadata): define the Ollama /api/show probe gate Ninth instance of the same defect class: _should_probe_ollama_api_show was referenced by tests but defined nowhere, so the file raised NameError. Regression guard for NousResearch#31555 — /api/show is an Ollama-server endpoint, so firing it at a hosted provider costs a doomed HTTP roundtrip (up to the 3s probe timeout) on a path that runs during agent startup. The rule comes from the three tests: skip for a known hosted provider, infer the provider from the base URL when none is named (which is how the OpenRouter case reached the probe at all), otherwise allow. The first attempt gated on _PROVIDER_PREFIXES and was wrong — that set contains plain `ollama`, because it answers "can this appear as a model prefix?", not "is this hosted?". test_allows_ollama_provider caught it. Local providers are now checked first via an explicit _LOCAL_OLLAMA_PROVIDERS; ollama-cloud stays out of it, being the hosted service that serves no /api/show. Found by reading CI's slice list rather than my sandbox: tests/agent/ — 351 files — had never been in any of my local sweeps, so this bug was absent from every inventory I had built. That also corrects my earlier claim that the remaining failures were all environmental: the environmental ones are real (root defeats chmod-based permission tests, no IPv6 for bind assertions), but that was a statement about a set I had not measured. tests/agent/: 3,926 passing / 12 failing across 7 files, none of them this one. ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * docs(system-log): complete local inventory and two corrections First full-suite run with a repaired venv — the first complete enumeration this session has had. 62 failures across 28 files, roughly half of them this sandbox's missing optional extras rather than repo bugs. Two of the real ones are features that do not exist rather than bugs: _redact_command_for_display (tools/approval) and per-model credential lockout (CredentialPool.select/_mark_exhausted take no model kwarg and the module has zero lockout machinery, against a 185-line spec). Both left for a decision. The contrast with the config-integrity cluster is the reason: that one had orphaned fcntl/msvcrt imports, a surviving bridge module and a skill implementing the machinery, so restoring it was recovery. There is no such trace here, and inventing cooldown semantics beside a DEAD/EXHAUSTED state machine that governs billing-bearing credentials is a feature decision. Also records two corrections. My first regression check was vacuous — git stash takes only uncommitted work, so with everything committed I compared the branch against itself and read the identical numbers as evidence. A worktree at origin/main gives the real answer: the tests/agent/ failures are pre-existing. And 'same compromised run' was wrong; runs are mixed, and log length (~180 vs ~1000+ lines) discriminates a setup death from a real test run, not the run id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(deps): stop lazy installs downgrading aiohttp below its CVE patch pyproject.toml and uv.lock both pin aiohttp==3.14.3; tools/lazy_deps.py still pinned 3.14.1 in four places, and cbor2==6.1.2 against a uv.lock of 6.1.3. The consequence is in the failing test's own message: pyproject extras pins must match LAZY_DEPS pins for every shared package, otherwise `hermes update` downgrades the package. Every one of those lines carries a CVE list including an RCE (CVE-2026-34993), so installing any lazy feature — a Discord, Teams or Matrix platform, or the Modal terminal — would have pulled aiohttp back below the patched floor on a machine that already had the fix. Bumped the four aiohttp pins to 3.14.3 and cbor2 to 6.1.3, matching uv.lock exactly. Also corrected the comments that named the old version beside the new pin (`# aiohttp 3.14.1: CVE-…` next to `aiohttp==3.14.3`, once here and three times in pyproject.toml) — a version string that disagrees with the pin it annotates is the drift that produced this. Correction to my own triage: I had filed these two tests under "sandbox drift" because the message mentions an aiohttp version, assuming it reflected my stale venv. It does not — they compare declarations across pyproject/lazy_deps/uv.lock and never inspect what is installed, and CI fails them identically. test_project_metadata + test_packaging_metadata: 13 tests, 0 failures. pyproject.toml parses; ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * feat(credential-pool): restore per-model credential lockout A 429 for one model says nothing about the others, but the pool could only bench a credential globally — so one rate-limited model removed the key from rotation everywhere. Correction to my own earlier triage: I reported that credential_pool.py had "zero model-lockout machinery" and deferred this as inventing a feature. That was wrong — I grepped for model_lockout/locked_models/model_locks; the feature is called exhausted_models, and two pieces were already shipping. _model_exhausted_until() lives in credential_pool.py and reads exactly the record shape the tests specify, and hermes_cli/auth_commands.py renders entry.extra["exhausted_models"] in auth status output. The display side has been live against a field nothing ever wrote — the same partial-deletion signature as the config-integrity cluster, consumers and helpers surviving without the producer. _mark_exhausted(model=...) now records the bench in extra["exhausted_models"] and leaves the credential-wide status alone. Terminal auth failures are deliberately excluded from that path: a revoked or invalid token is dead for every model, and recording it per-model would keep the credential in rotation for the rest, failing instantly on each. The existing _is_terminal_auth_failure classifier makes that call, so the DEAD/EXHAUSTED distinction is unchanged. select(model=...) filters entries benched for that model, keyword-only so existing no-argument callers are untouched. _prune_expired_model_lockouts() drops lapsed records, mirroring what _available_entries(clear_expired=True) does for the credential-wide status, and runs before filtering so a just-expired lockout frees its credential in the same call. reset_statuses() now counts a per-model lockout as status — a credential can be benched for a model with every credential-wide field clear, and reset must free that too rather than reporting "0 reset". tests/agent/test_model_lockout.py 4/4. Credential, pool and auth suites: 477 tests across 67 files, 0 failures. tests/agent/ overall 3,930 passing / 8 failing, up from 3,926 / 12 — no regressions in a module governing billing-bearing credential rotation. ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(approval): redact commands shown for approval; fold single-component homes Two fixes in tools/approval.py, five tests. _redact_command_for_display was missing. Searching for consumers first — the lesson from the credential-pool commit — showed agent/redact.py already carries the Slack xapp-/xox[baprs]- patterns, URL-userinfo handling and redact_sensitive_text(), and that it satisfies every assertion in the spec unchanged. So the missing piece was a thin wrapper, not a redaction engine. It earns its own name from the contract in the test docstring: the original command still runs, this is display-only. Motivated by the 2026-07-13 incident — a heredoc appending a Slack app-level token to ~/.hermes/.env, where the approval prompt reproduced the command in full, so asking permission published the credential more widely than running it would have. The variable name survives so the reviewer can see which secret is being written; only the value is masked. force=True and redact_url_credentials=True come from redact_sensitive_text's own docstring — force is documented for "safety boundaries that must never return raw secrets regardless of the user's global logging redaction preference", and an approval echo into Slack is an egress boundary rather than the tool flow the URL default protects. The fifth failure was TestSensitiveRedirectPattern, and it corrects me twice. I first checked /home/alice/.ssh/authorized_keys, saw False, and called it a general security hole — wrong, that is not the current user's home and the fold is deliberately scoped to the running user's. The real issue is narrower: _home_prefix_fold_regex required two components below the root, which correctly rejects / and /home (folding /home would rewrite every user's path to ~), but /root is a legitimate home with one component. So on any root-run deployment — the Docker image, CI containers, sudo — ~/.ssh/authorized_keys was guarded while the identical /root/.ssh/authorized_keys was not. Replaced the count with the property the guard wants: reject a bare root, and reject a single component only when it contains other homes (home, users, var, mnt, …). Verified both directions: /root/.ssh/authorized_keys now flags, ~ still flags, /home/other/.ssh/authorized_keys is still left unfolded. test_approval.py 101 passing / 0 failing (was 96/5). tests/tools/ 5,375 passing / 18 failing across 3 files, all pre-existing: daytona (optional extra absent), execution-flag detection, and the chmod-under-root artifact. ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(cron,state): re-enable the privileged-model gate and the PASSIVE WAL checkpoint Three defects of the same shape: the mechanism survived, the thing that uses it did not. cron/model_policy.py is a complete, documented guardrail — it refuses to route a legal/finance cron job to a denylisted or non-allowlisted provider/model. It had zero callers. `enforce_privileged_model_route()` now runs in create_job() on the assembled record and in update_job() on the *merged* record (the privileged tags come from the stored prompt/skills while the offending route arrives in the update, so neither half is conclusive alone). Both raise before save_jobs(), so a rejected job never reaches disk. hermes_state._try_wal_checkpoint() executed `PRAGMA wal_checkpoint(TRUNCATE)` under a 16-line docstring explaining that TRUNCATE is what caused B-tree corruption on large databases (NousResearch#45383) and that the periodic path must be PASSIVE. The warning string and the tests still said PASSIVE; only the pragma had reverted. Now PASSIVE again; close() and the pre-VACUUM path keep TRUNCATE, as documented. test_gateway_platform_gating asserted the retired lookup: telegram/slack/ matrix moved to plugin-registered `setup_fn=interactive_setup` in NousResearch#41112, and _configure_platform() tries the registry entry first. The invariant — these three never fall through to _setup_standard_platform(), whose mandatory-first-var behaviour would break Matrix's password-login path — is now asserted against the mechanism actually in use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(config): drop the resurrected mcpServers block from .claude/settings.json The block was removed deliberately on 2026-07-28 (see docs/system-log) in favour of .mcp.json.example: Claude Code documents ${VAR} expansion for .mcp.json and ~/.claude.json but not for settings.json, so a server declared there needs a literal absolute path and starts on exactly one machine, failing silently everywhere else. An unrelated dependabot npm bump (7e38fa5) re-added a copy carrying a hardcoded /Users/<name>/ path. The `memory` server moves into .mcp.json.example so #134's @modelcontextprotocol/server-memory@0.6.2 pin survives the removal (0.6.3 does not exist on npm). The `hooks` block is untouched — AGENTS.md is explicit that settings.json keeps hooks only, and $CLAUDE_PROJECT_DIR *does* expand there, which is how the tracked .claude/hooks/session-start.sh is reached. test_no_repository_local_claude_permissions_file asserted the file must not exist at all, which would orphan that hook. It now asserts what AGENTS.md actually requires: no `permissions` and no `mcpServers` key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * docs(system-log): ninth instalment — CI's failure list and the last five Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix(cron): anchor privileged-workload keywords so ordinary jobs aren't refused Found by the E2E validation AGENTS.md requires before wiring a dead module into a live path — and it would have shipped a regression. detect_privileged_workload_tags() scanned for its keywords as plain substrings. Several of them are acronyms: "je" (journal entry) matches "project", "subject", "rejected" and "Jenkins", and "books" matches "notebooks". Harmless while the detector had no callers. Now that the route gate runs on create_job()/update_job(), an over-broad match doesn't just mislabel a job — a privileged tag plus any pinned provider/model outside the six-entry allowlist refuses the job outright. "Summarize the project changelog" with a model override would have been rejected. Keywords now match at a token start, so inflections still tag ("reconciliation" -> "reconciliations", "attorney" -> "attorneys") while a keyword buried inside an unrelated word does not. Acronyms are anchored at both ends. The over-tagging the module documents as deliberate is intact: every keyword that matched a real word before still matches. E2E against a temp HERMES_HOME through the real create_job/update_job chain: an ordinary pinned job saying "project" lands, a denied route is refused without persisting, an allowlisted privileged route lands, a bad update leaves the stored job untouched, and ordinary updates still work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * docs(system-log): tenth instalment — E2E validation caught a regression Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * test(moa): remove the interrupt-ordering race in the reference fan-out test test_references_parallel_interrupt_aborts_wait raised the interrupt flag from inside the "fast" stub, before that stub's response reached its future. A poll landing in the gap saw an interrupt with the reference still in flight, so the slot was marked skipped and the assertion failed — rare locally, hit on a loaded CI runner. The production path is not at fault: _run_references_parallel() records completed futures before it checks the flag, and the interrupted sweep preserves any result that lands in between. The test simply wasn't creating the scenario it described. The interrupt now comes from the progress_callback the fan-out invokes right after recording a completed reference — "the interrupt arrives right after the fast reference finishes", ordered by the production code instead of by thread scheduling. Verified in both directions: forcing the old ordering with a 150ms linger reproduces the exact CI failure string, and the new ordering under the same delay returns the real output. agent/moa_loop.py is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 --------- Co-authored-by: Claude <noreply@anthropic.com>
auto-merge-prs.yml triggered on pull_request [opened, ready_for_review] and called `gh pr merge --auto` immediately. `--auto` does not wait for CI — it waits for whatever branch protection requires, and this repo has no required status checks configured, so "wait for the required checks" resolved to "wait for nothing" and PRs merged seconds after being opened. Observed, not theorised: #183 merged with `All required checks pass` RED, while web/check and apps/desktop/check:test:ui were both failing on the React version mismatch. #185 merged 11 seconds after it was created, before CI finished. The repo had already half-learned this one level down — ci.yml's all-checks-pass job carries a comment that desktop-install-windows "merged red three times before being listed here, because nothing was waiting on it." Nothing was waiting on the aggregator either. The trigger is now the completion of the CI workflow, and the decision is read from the `All required checks pass` check run on the head commit. Keyed on that check run rather than workflow_run.conclusion, and the difference is load-bearing: CI as a whole can conclude failure because of a lane nobody requires — the Docker build is explicitly outside the gate's needs list — so gating on the overall conclusion would block merges on lanes the repo has already decided are non-blocking. The aggregator is the contract, and it already counts `skipped` as passing, so Python-only PRs aren't held up by frontend lanes. One re-run, then stop. A red gate blocks the merge; if the run hasn't been retried, its failed jobs re-run once, which fires a fresh workflow_run completion that re-enters at attempt 2 where no further retry is offered. A genuinely broken PR fails twice and stays blocked. This is what keeps the known ui-tui/check flake (ink-resize.test.ts) from wedging a good PR permanently. Fork PRs never reach the merge path, since a workflow_run job holds a write token in the base repo's context. Drafts are skipped, a `do-not-merge` label is an escape hatch, and a PR whose head moved since CI ran is left to the newer run. Actions can't be executed here, so the step's shell was extracted and run against a mock gh that logs every merge/rerun requested. Nine cases: success merges; failure/cancelled at attempt 1 re-runs without merging; failure at attempt 2 neither re-runs nor merges; a missing gate, a draft, a do-not-merge label, a moved head and an already-merged PR all decline. The only path that merges is success. Separately verified that the jq name filter picks the aggregator out of a noisy check-run list, and that a renamed gate yields an empty conclusion — it fails closed, not open. This is not branch protection: it stops this automation from merging red, not a human merging by hand. Requiring the aggregator in the branch rules is still the real fence and composes with this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
Owner's explicit instruction, once the gate in the previous commit made the aggregator actually block merges. The consequence, stated rather than left to be discovered: a PR that touches CI-sensitive files (workflows, actions, eslint config), changes the MCP catalog, or trips a critical supply-chain finding can now merge with no human having looked at it. That gate asks for the `ci-reviewed` label, and a label is by definition something a person adds, so requiring it would mean every such PR waits for a human — the opposite of what this repo's automation is for. To be precise about what changed: review-labels never blocked anything before this PR either, because nothing was waiting on the aggregator at all — #183 merged with the label gate red. So this is a change in intent, not in effective behaviour. The previous commit would have started enforcing it for the first time; that enforcement is declined up front rather than discovered as friction later. The job still runs and still reports red on the PR, so the signal is intact; it just doesn't block. Restoring it is one line. ci.yml parses, the review-labels job is still defined, and comment-live still lists it in needs — the review comment reads its status, so dropping it from the gate must not drop it from the comment. The gate's evaluate step iterates toJSON(needs) generically, so no other edit is required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
dizhaky
force-pushed
the
claude/daily-repo-scan-v8fvqs
branch
from
August 12, 2026 19:00
c18971c to
6e024be
Compare
The "What to do on a hit" step was gated on a bare `if: failure()`, which is job-scoped — it fires when checkout or the pinned-gitleaks download fails too. A registry 503 then prints "gitleaks flagged a secret in this diff" over an infrastructure blip, which sends someone hunting a credential that was never there and teaches them to discount the message on the day it is real. Observed on this PR's own CI: a slice failed with four `curl: (22) ... error: 503` retries and exit 22 before any scan ran. Now `if: failure() && steps.scan.outcome == 'failure'`. When an earlier step fails the scan never runs, so its outcome is empty and the advice stays quiet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
dizhaky
marked this pull request as ready for review
August 12, 2026 19:12
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
2 participants
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.
What does this PR do?
Two commits: make the aggregate gate actually block auto-merge, then decide (deliberately) that the human review-label gate is not part of it.
1. The hole
auto-merge-prs.ymltriggered onpull_request: [opened, ready_for_review]and calledgh pr merge --autoimmediately.--autodoes not wait for CI. It waits for whatever branch protection requires — and this repo has no required status checks configured, so "wait for the required checks" resolved to "wait for nothing," and PRs merged seconds after being opened.Observed, not theorised:
All required checks passRED —web / checkandapps/desktop / check:test:uiwere both failing on the React version mismatch at the time.github-actions[bot], long before CI could finish.The repo had already half-learned this one level down:
ci.yml'sall-checks-passjob carries a comment thatdesktop-install-windows"merged red three times before being listed here, because nothing was waiting on it." Nothing was waiting on the aggregator either.The fix. The trigger is now the completion of the CI workflow, and the merge decision is read from the
All required checks passcheck run on the head commit.Keyed on that check run rather than
workflow_run.conclusion, and the difference is load-bearing: CI as a whole can concludefailurebecause of a lane nobody requires — the Docker build is explicitly outside the gate'sneedslist, with a comment saying so — so gating on the overall conclusion would block merges the repo has already decided shouldn't be blocked. The aggregator is the contract, and it already countsskippedas passing, so a Python-only PR isn't held up by frontend lanes.One re-run, then stop. A red gate blocks the merge; if the run hasn't been retried, its failed jobs re-run once. That fires a fresh
workflow_runcompletion which re-enters at attempt 2, where no further retry is offered. A genuinely broken PR fails twice and stays blocked. This is what keeps the knownui-tui / checkflake (ink-resize.test.ts) from permanently wedging a good PR.Other guards: fork PRs never reach the merge path (a
workflow_runjob holds a write token in the base repo's context), drafts are skipped, ado-not-mergelabel is an escape hatch, and a PR whose head moved since CI ran is left for the newer run.2.
review-labelsdropped from the aggregatorRepo owner's explicit instruction, once the gate above made the aggregator actually block.
The consequence, stated rather than left to be discovered: a PR that touches CI-sensitive files (workflows, actions, eslint config), changes the MCP catalog, or trips a critical supply-chain finding can now merge with no human having looked at it. That gate asks for the
ci-reviewedlabel, and a label is by definition something a person adds — so requiring it would mean every such PR waits for a human, which is the opposite of what this repo's automation is for.Worth being precise about what actually changed:
review-labelsnever blocked anything before this PR either, because nothing was waiting on the aggregator at all — #183 merged with the label gate red. So this is a change in intent, not in effective behaviour. Commit 1 would have started enforcing it for the first time; that enforcement is declined up front rather than discovered as friction later.The job still runs and still reports red on the PR, so the signal is intact — it just doesn't block. Restoring it is one line.
Related Issue
No issue — follows directly from #183 and #185 merging without a passing gate.
Type of Change
Changes Made
.github/workflows/auto-merge-prs.yml— rewritten:workflow_runon CI completion instead ofpull_request: opened; merge only on asuccessaggregator check run; bounded single re-run; fork/draft/label/stale-head guards..github/workflows/ci.yml—review-labelsremoved fromall-checks-pass'sneeds, with the reasoning inline.docs/system-log/2026-08-12.md— full write-up of both.How to Test
GitHub Actions can't be executed locally, so the step's shell was extracted and run against a mock
ghthat logs every merge/rerun it is asked to perform. See the decision table below.Checklist
Code
pytest tests/ -q— N/A, no Python touchedDocumentation & Housekeeping
docs/system-log/2026-08-12.md, plus header comments in both workflows)cli-config.yaml.example,CONTRIBUTING.md/AGENTS.md, tool schemasScreenshots / Logs
Decision table, executed against a mock
gh:successfailure, attempt 1failure, attempt 2cancelled, attempt 1do-not-mergelabelThe only path that merges is
success. Separately verified that the jq name filter picks the aggregator out of a noisy check-run list, and that a renamed gate yields an empty conclusion — it fails closed, declining to merge, rather than failing open. YAML parses,bash -nis clean, and the gate string is asserted equal toci.yml'sall-checks-passjob name.For commit 2:
ci.ymlparses, thereview-labelsjob is still defined, andcomment-livestill lists it inneeds— the review comment reads its status, so dropping it from the gate must not drop it from the comment. The gate's evaluate step iteratestoJSON(needs)generically, so no other edit was required.Two honest limits
This is not branch protection. It stops this automation from merging red — the hole that actually bit us three times. It does not stop a human merging by hand or pushing straight to
main. RequiringAll required checks passin the branch rules is the real fence and composes with this. I couldn't do that from here: this environment has noghCLI or direct GitHub API, and the MCP GitHub toolset exposes no branch-protection or ruleset endpoint. If it's ever wanted: Settings → Branches → add a rule formain→ "Require status checks to pass before merging" → search forAll required checks pass.This PR is itself merged by the old, ungated path, because the version of the workflow on
mainis what runs at the time. The gate applies from the next PR onward.