Repository navigation
fix(canary): wire CANARY_ENABLED/_SLACK_WEBHOOK_URL into prod backend env - #1876
Merged
Merged
Conversation
… env The canary invariant harness could not be enabled on any deployed instance. `architecture.md` documents it as "Disabled by default; CANARY_ENABLED=1 on staging/dev" — but every deploy path uses docker-compose.prod.yml (.github/workflows/deploy-dev.yml, scripts/deploy/gcp-deploy.sh), and that file never passed the two knobs into backend.environment. Prod compose launches standalone (no base merge, no env_file: on any service), so the explicit environment list is the only path in: a CANARY_ENABLED=1 in the remote .env was interpolated into nothing and silently dropped. Net effect: epic #411 was closed COMPLETED on 2026-07-28 at 53/53 sub-issues, while the harness it delivered was un-turnable-on on the very instance it exists to watch — and its Slack alert sink was unconfigurable, so even a hand-enabled canary would have been a silent sink. config/canary-fleet.yaml targets a running instance's /api/systems/deploy, so this was never a laptop-only path. Verified with a positive control, before and after: $ CANARY_ENABLED=1 docker compose -f docker-compose.prod.yml config before: CANARY_ENABLED -> None VOIP_ENABLED -> 'true' after: CANARY_ENABLED -> '1' VOIP_ENABLED -> 'true' Additive-safe: with the vars unset the resolved config is CANARY_ENABLED='0' and CANARY_SLACK_WEBHOOK_URL='', so existing installs are unaffected. Both docker-compose.prod.yml and the prod+enterprise overlay still validate. Found by the inert-lever audit on #1258 (the #1039/#1056 packaging-gap class), written while fixing the same class in #1871. Refs #1258 Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
webmixgamer
force-pushed
the
fix/canary-env-prod-parity
branch
from
July 29, 2026 14:11
8da1ddd to
961143e
Compare
4 tasks done
The fix in the previous commit is two compose lines that nothing asserts —
delete them and every test still passes, which is exactly how the original
omission survived ~2.5 months and an epic close-out.
Guards both compose files for both knobs:
* the injection line exists (absence = the lever is inert on every deploy)
* it defaults to OFF (presence must not mean enabled — .env.example promises
"Production stays 0", and a future `:-1` would start a 5-min watcher loop
plus its synthetic fleet on every install)
* dev and prod agree (the drift IS the bug)
Plus a meta-test proving the matcher fails on pre-fix content, so a typo can't
make the assertions vacuously true and rot the guard into a no-op.
Shape mirrors tests/unit/test_1076_voice_model_config.py, the existing
precedent for guarding a compose injection against a silent revert.
Refs #1878
Refs #1258
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
obasilakis
approved these changes
Jul 29, 2026
obasilakis
left a comment
obasilakis
left a comment
Contributor
There was a problem hiding this comment.
Reviewed via /review + /cso — no critical or security findings.
Independently verified (not taken from the PR body):
- Backend genuinely reads both vars —
canary_service.py:130,canary_alerts.py:134 - Prod compose has no
env_file:on any service, so the explicitenvironment:list really was the only path in - Post-fix resolution:
CANARY_ENABLED -> '1', webhook URL reaches the container - Inert by default: unset resolves to
'0'/''— no behaviour change for existing installs - prod and prod+enterprise overlay both validate
- Guard bites: reverting the two compose lines fails 3 tests;
backend-unit-test.yml:116collectstests/unit/
Security note. This makes a Slack webhook URL — a bearer credential — settable in production for the first time, so the leak path matters now. It is already hardened: slack_service.py:346-351 returns type(e).__name__ rather than str(e) specifically because httpx embeds the request URL in exception messages, and the URL appears in no log line or API response. Given Vector persists all logs to disk, that guard is load-bearing and intact. Payload contents (agent names, execution ids, counts) carry no credentials or user content; the backlog_metadata-touching invariants (E-04/G-04) have no _render_forensic branch and fall through to a generic message — a completeness gap that fails in the safe direction.
Non-blocking nits:
_injection_linesscans the whole file while the failure message claims "under backend.environment" — a line moved to the wrong service would pass. Narrow case; worth tightening if this file is touched again.- Scope is correctly limited to the canary vars, with the broader inert-lever sweep tracked on #1258.
LGTM.
5 of 6 tasks
8 tasks done
obasilakis
pushed a commit
that referenced
this pull request
Jul 30, 2026
…eeps it (#1894) Five registry invariants (E-03, E-04, E-06, G-03, G-04) shipped with no entry in the per-invariant Slack alert surfaces, so a green→red page rendered: 🚨 canary G-04 G-04 (critical): G-04 fired 1 violation(s). No name, no evidence, no next step, and the id stuttered — on the *critical* credential-leak check. Default-OFF is why it hadn't bitten; #1876 wired CANARY_ENABLED into prod compose two days ago, so it is now one env var away. Mechanism, not diligence: #882 populated every alert surface for its invariants, #1497 and #1472 populated none. Two hand-maintained lists with nothing tying them together (learnings.md 2026-07-16). There are FOUR id-keyed surfaces, not the two the issue named ------------------------------------------------------------ _INVARIANT_NAMES, _INVARIANT_RUNBOOKS, and the branch chains inside _render_message and _render_forensic — all four independently covering the same 10 of 15 ids. The two the issue missed carry the per-row evidence, so fixing only the named ones would have given G-04 a name, a runbook, a count, and no rows. The guard has to run where CI looks ----------------------------------- tests/test_canary_invariants.py (3,726 lines, the only file exercising this code) is executed by NO workflow — every gating job is `cd tests && pytest unit/`. The guard therefore lives in tests/unit/, is AST-based (importing services/ drags docker_service→pydantic, and `services` is a known stub-leak target under pytest-randomly), and is bidirectional so a typo'd or stale id fails too. Mutation-verified against six regression shapes. Names come from each invariant module's docstring title, NOT the catalog ----------------------------------------------------------------------- Catalog ids are not registry ids: catalog E-06 is the unimplemented #129 check while registry E-06 is "no overdue next_run_at" (#1472, semantically SCH-03). G-04's catalog title also promises "/ logs" coverage that does not exist. A catalog-sourced name would confidently mislabel a live alert, which is worse than the honest bare-id fallback. Both stale catalog cross-refs corrected. The render fallbacks are a security property, not debt ------------------------------------------------------ _render_forensic's `return None` and _render_message's count-only string are why an un-rendered invariant cannot echo observed_state — E-04 and G-04 scrub at the *check*, reporting a reason code / pattern name only. A parity gate creates standing pressure toward the one-line generic observed_state dumper that satisfies it forever, which is invisible in review because it removes a special case. Both fallbacks are kept, commented, and pinned by a negative test. Also in scope ------------- * _mrkdwn_safe() at the render boundary (56 sites), collapsing the two inconsistent `?`-defaulting idioms into one. Escapes Slack's documented &/</> set so a name cannot forge a live link or an @channel mention; injection safety is now local instead of transitive across ~8 sanitize-at-write paths (retention_guard.py already writes a name sanitize_agent_name could never produce). * A NULL agent_name no longer raises TypeError out of sorted() into canary_service's swallowing except — which dropped the alert entirely while still advancing the green→red cursor, so nothing retried. Swept all ten pre-existing sites, not just the five new branches. * requirements/infrastructure.md §31 said registering an invariant leaves "the service and API surface unchanged" — after this change that produces a red CI. Corrected, along with the architecture.md alert-sink block, which now also records that a rendered alert points at where a credential sits and so belongs in a restricted channel. No schema change, no migration, no new endpoint, no config, no dependency. 107 new tests; 139 existing canary tests unaffected. Fixes #1880 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 4, 2026
obasilakis
added a commit
that referenced
this pull request
Aug 6, 2026
…ent#335/336/337) (#2022) * fix(canary): stop three false-positive classes drowning the harness (ent#335/336/337) eu2 (e6df5bf, PostgreSQL) is the first fleet instance to run the canary under prod compose after #1876 wired CANARY_ENABLED in. In ~13h it recorded 6,605 violations. 6,598 of them (99.9%) were the harness misreading correct runtime behaviour. In each case the platform is right and the invariant's predicate is wrong. E-06 — schedules of soft-deleted agents (ent#335), 6,220 violations (94%) _collect_enabled_schedules filtered only on the schedule's own deleted_at, so schedules of a soft-deleted AGENT stayed "enabled" for the whole 180-day window (#834 Phase 1a preserves child rows by design) with a frozen next_run_at. The projection did not freeze because the scheduler failed to advance it — it froze because the scheduler correctly stopped registering a deleted agent's jobs. This is not a new predicate. db/schedules/crud.py::list_all_enabled_schedules — the list the scheduler actually arms — is exactly the proposed join and applies BOTH #834 filters; the collector's docstring already claimed to mirror it and had copied half. Now inner-joins agent_ownership with deleted_at IS NULL, the matched pair to _collect_known_agents' deliberate INclusion of soft-deleted agents (which keeps L-03 from reporting those same preserved rows as orphans). The join also drops schedules whose owner row is gone — L-03's orphans, and its agent_schedules scan is unfiltered, so the deferral is real. S-03 — TTL floor from the agent cap (ent#336), 378 violations acquire_slot sets the TTL from THIS execution's timeout, which since #929 is legitimately below the agent cap (an explicit shorter schedule timeout, a 900s public turn, a loop's timeout_per_run). Building the floor from the cap paged critical for the entire runtime of every normally-configured scheduled run — and unlike E-06 it re-fires per execution, so it produced a fresh green->red page on essentially every one. The runtime had already reached the opposite conclusion: _cleanup_stale_slots_for_agent reads timeout_seconds back off the metadata HASH for exactly this reason (#869). Cleanup was per-slot-aware and the canary was not. The floor now comes from that same stored field, read in one pipeline with the TTL. An unobservable timeout SKIPS the slot rather than falling back to the cap — the fallback re-arms this same false positive in a narrower window. -1/-2 stay untouched as the load-bearing #226 coverage. Honest about the cost: acquire_slot derives the EXPIRE and the HSET from one local three lines apart and is the sole writer, so below_floor is now an internal-coherence check and no longer catches a caller passing the wrong timeout (the #913 class). The docstring's worked example claiming that coverage is deleted rather than left lying, and architecture.md says so too. Tracked: ent#339. R-01 — no persistence gate (ent#337), 6 critical pages Fired on any single positive sample, so it flagged the sampling window rather than the #407 leak: every page was zombie_count 1 on a container with 13h uptime that was never restarted — reaped by the ordinary parent wait() within one cycle. The harm model is cumulative; a zombie that clears in one cycle exhausts nothing. Now gated on a dwell, and the dwell is per-PID rather than per-count. A count cannot support one: three cycles catching three DIFFERENT transients are indistinguishable from one stuck zombie, and "clear when the count returns to 0" needs a zero to be observed, which under sustained load never happens — on exactly the busy agents that filed this. ps -eo stat,pid,comm gives the identity, and a pid present at T and T+600 has demonstrably dwelled. Elapsed wall-clock from snapshot.snapshot_time, never time.time(): multi-worker- safe (a "seen twice" rule self-confirms in <1s across two workers — trinity#1881 part 2 is visible in the eu2 data as rows 0.8s apart) and structurally testable at the boundary (#1909). Marker is first-write-wins; rewriting first_seen each cycle would reset the clock and leave a permanently blind critical invariant. Keyed agent:canary_zombie:{name} and registered in CLEARED_KEYSPACES — the first name-keyed canary key, so it inherits the #1560 recycled-name hazard (E-02's canary:e02:* are global, hence legitimately unregistered). A last_seen gap over two intervals restarts the dwell, so a docker-exec outage is not counted as dwell time; unparseable ps output is sources_unavailable, never a silent green. The issue's etimes alternative was evaluated and rejected: etimes is time since process START, not since it zombified, so a process that ran 45min and zombified 200ms ago reads as 45-min-old and fires immediately — the exact transient the gate exists to stop flagging. Deviations from the issues' acceptance criteria - ent#336 AC #3 (fire when stored timeout > agent cap) is DROPPED: loops are not clamped to the agent cap (timeout_per_run is bounded only by MAX_TIMEOUT_PER_RUN=7200), so it would page critical on a legal loop, and S-03 cannot tell a loop slot from a schedule slot. Replaced by a build-time test pinning the two ceilings against each other — same regression, caught on the PR that introduces it instead of 5 minutes after deploy. Gap filed as ent#338. - ent#337 AC #2's growth arm is satisfied by the dwell rather than separately: a count that grows across cycles has been >0 continuously, so the dwell elapses. A separate immediate-fire growth arm would reopen the multi-worker race the issue itself warns about. Trend rides in observed_state for triage. Both deviations are commented on their issues rather than silently omitted. Tests New tests live in tests/unit/ because CI runs `cd tests && pytest unit/` only — tests/test_canary_invariants.py, which holds every existing S-03/R-01/E-06 test, is executed by no workflow (the finding test_1880_canary_alert_parity.py already records). 41 new tests there; the existing suite updated in place and extended (FakeRedis gains pipeline/hgetall/expire; the pipeline double buffers rather than executing eagerly, so a test cannot pass against code that issues the reads separately). Full unit suite: 7514 passed, 18 skipped. Refs Abilityai/trinity-enterprise#335, Abilityai/trinity-enterprise#336, Abilityai/trinity-enterprise#337 * fix(canary): restart R-01's zombie dwell when the container's pid namespace does (ent#337 review) A PID is only an identity within one PID namespace. The dwell marker was keyed by (agent_name, pid), and `clear_agent_breakers` only covers backend-mediated lifecycle events — a container that restarts on its own (`docker restart`, a restart policy after an OOM kill or a crash) keeps its marker. A fresh namespace hands out low PIDs immediately, and a zombie `claude` in a just-restarted agent is exactly the low-PID case, so a brand-new transient landing on a PID still in the marker inherited a dwell-old `first_seen` and paged critical on its first sample — the precise false positive this PR removes. The module docstring dismissed PID reuse on `pid_max` wrap-around grounds, which is the right argument for a running container and the wrong one for a restarted one. The `last_seen` continuity check cannot cover it either: a restart inside one cycle leaves no observation gap at all. The snapshot now carries each container's `State.StartedAt` and R-01 stores it in a reserved `__started_at` field on the marker HASH, dropping the whole marker when it moves. Free: docker-py's `containers.list()` defaults to `sparse=False`, which already issues a full inspect per container, so the field is in `attrs` before we ask. Deliberately not `Created` — a restart does not create a new container. Only an OBSERVED mismatch invalidates. An unreadable `StartedAt` leaves the dwell untouched: restarting on a non-signal every cycle would blind the invariant the same way rewriting `first_seen` each cycle would. 8 new tests, including the review's scenario end to end (restart, then an immediate transient on a reused PID, must be silent) and the differential that makes it a real guard — the same fixture with the generation unobservable still fires at the dwell.
5 tasks done
vybe
pushed a commit
that referenced
this pull request
Aug 6, 2026
…2047) * fix(canary): one cycle per fleet, not one per uvicorn worker (#1881) #1881 describes two chained defects and says they must land together, because "shipping the packaging fix alone converts a dormant bug into a live one". Part 1 shipped in #1876 — docker-compose.prod.yml now forwards CANARY_ENABLED and CANARY_SLACK_WEBHOOK_URL. Part 2 did not, so the warned-about state has been live since. Verified on eu2 today. The backend command is `uvicorn main:app ... --workers 2`; the canary cycle log lines interleave at ~2m15s / ~2m47s alternating gaps, which is the signature of two independent 5-minute loops running offset from each other; and canary_violations holds 11,942 rows for the last 24h, double-persisted. Why there was nothing stopping it The FastAPI lifespan calls canary_service.start(), so each worker process starts its own loop. The only mutual exclusion in the service is `self._lock = asyncio.Lock()`, which guards re-entrancy inside ONE process and says nothing about cross-worker exclusion. Reading an asyncio.Lock in a service class as evidence of single-instance-ness is exactly the mistake learnings.md 2026-07-29 records; it is a prompt to ask where the leader lease is, not proof that there is one. What that costs, concretely: R-01 `docker exec`s into every running agent container each cycle, so the fleet is probed twice per 5 min per agent by the subsystem that is supposed to observe it unobtrusively. Every violation is inserted twice. And every shared cross-cycle marker — canary:last_cycle_at, canary:last_cycle_red, E-02's canary:e02:terminal_seen and H-01's canary:h01:suspect_since — has two independent writers. The fix The scheduled loop now runs only when it holds a Redis `canary:leader` lease, mirroring monitoring:leader (#1464) and opqueue:leader (#1632): SET NX to pick a single winner atomically, TTL refresh only when the stored id is our own (so a worker can never steal or clobber a sibling's lease), best-effort release on stop() for an instant handoff. Every worker still runs its loop and re-evaluates leadership each cycle rather than once at startup, so a dead leader's lease expires and a sibling takes over with no restart. Two places it deliberately departs from the precedents, both for the same underlying reason — a duplicate canary cycle is not inert the way a doubled breaker feed (#1464) or an on-conflict create (#1632) is. TTL carries a floor. `interval * 3` is the right scaling for a leader whose work scales with its interval. A canary cycle's cost does not: it is dominated by R-01's container.exec_run sweep across every running agent container, which is bounded by no timeout and scales with FLEET SIZE, not with how often we look. The TTL is refreshed once at the top of a cycle, so it must outlast one worst-case cycle plus the inter-cycle sleep — otherwise the lease lapses mid-cycle, a sibling grabs it, and leadership flaps, restoring the exact concurrent probing the lease exists to remove. The floor is set to what `interval * 3` already yields at the default 300s interval, so the default is unchanged and it only binds if the service is constructed with a shorter one. Fail-open is kept, but not on the precedents' reasoning. monitoring_service can fail open because a doubled record_failure() lands in a breaker that is itself fail-open; operator_queue_service can because duplicate creates dedupe on conflict. Neither argument transfers — here the duplicate re-runs the sweep and double-persists rows. We fail open anyway, because the alternative is strictly worse: this is the one subsystem whose purpose is noticing that something went quiet, H-01 exists because a blind collector reports green, and a canary that stops running is that same silent-green one level up, where no invariant can catch it because invariants only run inside the thing that isn't running. A fail-closed lease would let a Redis blip stop the watcher on every worker at once with nothing saying so. Duplicated probes are noisy and visible; silence is not. A Redis outage is also already a degraded state the harness is built to announce (sources_unavailable; H-01 fires unconfirmed on an unreadable marker), and failing closed would suppress precisely those paths. What the lease does not buy, and what therefore did not change It is best-effort, and the fail-open above re-opens the multi-writer window exactly when Redis is down. H-01's CONFIRMATION_MIN_SECONDS and R-01's DWELL_SECONDS elapsed-wall-clock gates are therefore untouched, and their docstrings — which asserted, correctly at the time, that the service holds no lease — are corrected rather than deleted, with the reason they remain load-bearing spelled out so the next reader does not "simplify" either back to a cycle count. There is a second, more fundamental reason beyond the fail-open window: both gates ride out a REAL-TIME transient (a container finishing teardown, a claude child awaiting its parent's wait()), which is a single-worker property that a cycle count never expressed correctly at any worker count. One knock-on is recorded at R-01's _MAX_OBSERVATION_GAP_SECONDS: a leader failover leaves up to ~1200s (TTL + interval) with nobody cycling, which exceeds that 600s window and restarts the dwell. That is correct — a crashed leader is a genuine observation outage by that constant's own rule — and restarting is the fail-safe direction, so the constant is documented, not widened. run_cycle() is deliberately NOT gated. The lease belongs to the scheduled path, which is the one that runs unattended in every worker. POST /api/canary/run-cycle lands on whichever worker uvicorn routes it to, so gating there would make an explicit admin request return an empty result about half the time under --workers 2 — structurally identical to a green cycle, which is the exact ambiguity the skipped/409 contract was added to remove. Non-leaders log on the leadership transition only; narrating idleness every 5 minutes is how real canary output gets tuned out. Tests go in tests/unit/ (#2037: the 3,700-line canary suite at tests/ root is collected by no CI workflow, so a guard beside it would never go red). 23 tests covering the lease mechanics, failover, both fail-open arms, transition-only logging, that a non-leader never reaches collect_snapshot — with a leader=True positive control, since _loop swallows cycle exceptions and a raise-on-call probe would pass vacuously — that run_cycle stays ungated, and that the two confirmation gates survive. No schema, endpoint, config or compose change. * fix(canary): heartbeat the leader lease, and make every lease write atomic Review findings (dolho, #2047), plus the mid-cycle residual raised in review. **1. Non-atomic release could delete a SUCCESSOR's lease.** `_release_leadership` was GET-then-DELETE: if our lease expired between the two and a sibling won SET NX in that window, we deleted theirs. A third worker could then acquire while the successor still had `_is_leader = True` — two concurrent cycles, the exact state this PR removes. Both writes are now Lua compare-and-{delete,expire} via the existing `ScriptCache`, so no path can touch a lease that is not ours. #1464/#1632 share the non-atomic shape; we depart for the same reason this service departs on the TTL — a duplicated canary cycle is not inert. **2 + the mid-cycle residual: one TTL was answering two questions.** "How long may a cycle run" wants LARGE (R-01's exec sweep has no timeout and scales with fleet size); "how long before a dead leader is noticed" wants SMALL. The single `max(interval*3, 900)` TTL bought a ~1200s blind failover to pay for (a) — and did not even close (a): a cycle overrunning 900s lapsed its lease mid-flight and a sibling acquired anyway. `_heartbeat_loop` re-arms the lease on its own 60s timer, independent of where the cycle is, so the TTL only has to survive a couple of missed beats: `_LEADER_TTL_SECONDS` is 3x the heartbeat (the precedents' rule), and worst-case failover drops ~1200s → ~780s, owned by `_max_failover_seconds()` so R-01's comment and architecture.md quote one number instead of three prose copies. `_MAX_CYCLE_LEASE_SECONDS` (900s) caps how long the heartbeat will keep a running cycle's lease alive. Without it a WEDGED leader holds the lease forever and nobody cycles — trading "two workers probing" for "no worker watching", the wrong direction here. Past the cap we choose the duplicate over the silence, at ERROR. Same direction as the fail-open. **3.** Documented why two Redis clients coexist and must not be unified — including that `decode_responses=True` on the breaker client is what makes the ownership comparison valid, so a client built without it turns every check silently False. **4.** `stop()` is async and awaits the cancelled task before releasing, matching `monitoring_service`; its one caller in `main.py`'s lifespan already awaits. **5.** Confirmed and documented: the manual `POST /api/canary/run-cycle` path DOES drive the alert sink (`run_cycle` → `_run_cycle_inner` → `CanaryAlerts.emit_transition`), so a transition caught by an admin cycle alerts and the leader's next cycle correctly sees continuing-red. No lost alert. Tests: `FakeRedis` grows a client-aware `register_script`. That is load-bearing, not plumbing — every lease mutation is Lua now, and `_try_acquire_leadership` fails OPEN, so a fake that raised on `register_script` made every non-leader return True (the double-running under test) while the suite still read green. It caught 8 failures here. The two TTL tests asserting the retired single-TTL design are replaced by ones pinning the new split, including that `ttl < interval` is deliberate so nobody "fixes" it back.
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.
Fixes #1878
Summary
The canary invariant harness could not be enabled on any deployed instance.
architecture.mddocuments it as "Disabled by default;CANARY_ENABLED=1on staging/dev" — but every deploy path usesdocker-compose.prod.yml(.github/workflows/deploy-dev.yml,scripts/deploy/gcp-deploy.sh), and that file never passed the two knobs intobackend.environment:.Prod compose launches standalone — no base-compose merge, and no
env_file:on any service in either file — so the explicitenvironment:list is the only path in. ACANARY_ENABLED=1in the remote.env(whichgcp-deploy.shdoes write) was interpolated into nothing and silently dropped.Net effect: epic #411 was closed
COMPLETEDon 2026-07-28 at 53/53 sub-issues, while the harness it delivered was un-turnable-on on the very instance it exists to watch — and its Slack alert sink was unconfigurable, so even a hand-enabled canary would have been a silent sink.config/canary-fleet.yamltargets a running instance's/api/systems/deploy, so this was never a laptop-only path.Change
Two lines in
docker-compose.prod.ymlbackend.environment:, mirroringdocker-compose.ymlverbatim. No code change.Verification
Same test that proved the bug, with a positive control so the method itself is validated:
CANARY_ENABLED='0'/CANARY_SLACK_WEBHOOK_URL='', so existing installs see no behaviour changedocker compose -f docker-compose.prod.yml configvalidatesdocker compose -f docker-compose.prod.yml -f docker-compose.prod.enterprise.yml configvalidates (the enterprise overlay layers on prod, so it inherits the fix)CANARY_*vars the backend reads.gitea.ymlis an unrelated overlay;.override*.ymland.sibling.ymldefine nobackendserviceProvenance
Found by an inert-lever audit posted on #1258, written while fixing the same class in #1871 (where
/validate-pr§4.9 caughtAGENT_LOG_MAX_*before merge). This is the #1039/#1056 packaging-gap class: a documented.envlever that silently does nothing.That audit initially flagged 11 candidates; hand-verification retracted 2 as false positives (
TRINITY_GIT_*, correctly handled by thedocker-compose.gitea.ymloverlay). This is the only one that turned out to be a genuine, user-visible defect — the remaining items are dev-compose parity and a docs deletion, both noted on #1258 rather than filed.Deliberately no closing keyword: #1258 is an epic and #411 is already closed, so neither should be auto-closed by this PR.
Not doing here
No CI guard. A naive "every
os.getenvmust be in compose" check fails on 56 pre-existing vars, most legitimately code-default-only. The tractable predicate — read and uncommented in.env.exampleand not forwarded by any compose file including overlays — is viable once #1258's short list is burned down; see the audit comment for why the overlay and comment-line cases must both be handled.Refs #1258
Refs #411
🤖 Generated with Claude Code
"Was the omission deliberate?" — investigated, no
Fair challenge, since the originating commit's own wording sounds like intent.
.env.example(added by the same commita4eec13) says:Four independent lines of evidence say the omission from prod compose was an oversight, not enforcement:
1. The intent was unachievable from day one.
.github/workflows/deploy-dev.ymlwas created 2026-04-28 already usingdocker-compose.prod.yml. The canary landed 2026-05-10 — 12 days later. So "set to 1 on staging/dev" could never work on the deployed staging/dev instance; the deploy path did not change underneath it. Reading the omission as deliberate enforcement would mean the feature was dead on arrival everywhere, which is incoherent for an epic that also shipped a load-generator fleet manifest and a Slack alert sink.2. The same commit shipped another packaging gap. #751 — "backend Dockerfile missing COPY for canary/ package (a4eec13 breaks startup)" — the very same commit omitted a required
COPY, crashing the backend on boot. That PR demonstrably had packaging blind spots; two is unsurprising.3. "Production stays 0" is about the default, and the fix preserves it. The variable being present with an OFF default is not the same as enabled. With both unset, the resolved prod config is still
CANARY_ENABLED='0'andCANARY_SLACK_WEBHOOK_URL=''— verified above. Production users see no canary activity unless they deliberately opt in, exactly as written.4. Codebase convention is unambiguous.
docker-compose.prod.ymlalready carries eight default-OFF opt-in flags:WORKSPACE_ENABLED,VOIP_ENABLED,MCP_AGENT_CHAT_PULL_ENABLED,REDELIVERY_GOVERNOR_ENABLED,OTEL_ENABLED,PUBLIC_ACCESS_REQUESTS_ENABLED,DO_NOT_TRACK,DISPATCH_ASYNC. A default-OFF flag belongs in prod compose with its OFF default.And this exact question has already been adjudicated in this file. The VOIP block, two lines above where the canary vars now sit:
Identical situation, resolved as a packaging gap. This PR applies the same resolution.
Added after
/validate-prTracking (W2) — filed #1878 as a sub-issue of #1258 and switched to a closing keyword. Without it nothing transitioned
status-in-progress → status-in-dev, so/release(which builds notes from in-dev issues) would have shipped a user-visible fix silently.Regression guard (W1) —
tests/unit/test_canary_env_prod_parity.py. The fix is two compose lines that nothing asserted: delete them and every test still passed, which is precisely how the original omission survived ~2.5 months and an epic close-out. The guard pins, for both compose files and both knobs::-1would start a 5-min watcher loop and its synthetic agent fleet on every install)Verified it actually bites: with the fix reverted in-memory, the matcher drops from 1 injection line to 0 and the guard fails. Shape mirrors
test_1076_voice_model_config.py, the existing precedent for guarding a compose injection against a silent revert.