Repository navigation
fix: scope webhook promotion to rollout cohort - #334
Conversation
Filter coverage and exceptions to webhookCanaryRepos, report out-of-scope actionable traffic separately, and allow an explicit auditable observation epoch. Refs #299 Co-authored-by: Eduard Carreras <ecarreras@gisce.net>
pilipilisbot
left a comment
There was a problem hiding this comment.
The canary scoping and explicit observation epoch are sensible, but the scope currently persists into primary mode and would make the second observation window misleading.
_webhook_promotion_scope() always returns webhookCanaryRepos whenever a policy is configured (backend.py:160-163), and both coverage and exceptions use it. In primary, webhook ingestion deliberately stops applying that list (backend.py:879; docs/ingestion.md, “Primary webhook with stable IMAP fallback”). After promotion, an actionable IMAP-only event or action/intent mismatch in any newly enabled repository outside the old canary would be excluded from the gate and exceptions; the dashboard could still report ready. Please scope to the canary list only in canary mode, and use the actual global eligibility scope (enabledRepos, or all when unrestricted) in primary. A regression with an out-of-canary actionable mismatch in primary would pin this down. The dashboard/docs wording should distinguish the two modes too.
Checked the exact PR head f229f5d, the #299 rollout contract, SQL coverage/exception queries, policy/ingestion behavior, and the focused Python suites: PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=src .../python -m pytest -q -p no:cacheprovider tests/test_webhook.py tests/test_persistence_query_plans.py — 69 passed. CI dashboard and Python 3.11/3.12 checks are green. I did not change the branch.
|
Confirmed independently against exact head f229f5d and current main 3eb5001. I reproduced primary mode with enabledRepos = [gisce/in-scope, gisce/out-of-scope], webhookCanaryRepos = [gisce/in-scope], a clean paired event inside the former canary, and an actionable action mismatch in the newly primary-eligible repository. The summary returned scope_repositories = [gisce/in-scope], decision_mismatches = 0, outside-scope counts = 1/1, and promotion_state = ready. So the second observation gate can produce a false positive exactly as the review describes. The fix needs one mode-resolved scope shared by summary and exceptions:
Please add the primary regression so the out-of-canary mismatch both blocks promotion and appears in exceptions; align dashboard and docs wording to the active mode. Important edge: passing an empty enabledRepos set into the current SQL helper means AND 0, but policy semantics say empty is unrestricted, so it must become no scope filter. Validation: focused webhook/query-plan suite 69 passed. Worktree stayed clean; I made no branch changes. Changes-requested remains the correct verdict. |
|
@giscebot avanaça en solucionar això |
Resolve promotion coverage and exceptions from the active webhook mode so primary observation cannot ignore repositories outside the former canary.\n\nCo-authored-by: ecarreras <ecarreras@users.noreply.github.com>
|
Solucionat al commit
Validació: 676 tests Python, 79 tests del dashboard, build de producció i No s'ha activat |
|
@pilipilisbot revisa aquesta pull-request |
pilipilisbot
left a comment
There was a problem hiding this comment.
Reviewed exact head c3cbda0 against current main. The previous blocking scope issue is resolved: primary uses enabledRepos, or an unrestricted scope when that set is empty, for both the promotion summary and exceptions. The new regression shows an actionable mismatch outside the former canary blocks promotion and appears in exceptions; dashboard and docs distinguish the mode-specific scopes.
Focused validation: PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=src .../python -m pytest -q -p no:cacheprovider tests/test_webhook.py tests/test_persistence_query_plans.py — 70 passed. git diff --check passed; the review worktree stayed clean. CI dashboard and Python 3.11/3.12 passed on this head. This approves the code change, not an operational switch to primary; the observation windows and rollout prerequisites in #299 remain separate.
|
Post-merge follow-up: el worktree dedicat de la PR s’ha trobat net i s’ha retirat correctament. La implementació queda mergejada a |
|
Post-merge sync (nova comprovació): el worktree dedicat de #334 havia tornat a aparèixer després de la nota de neteja anterior. Era net i apuntava al head fusionat La PR continua fusionada a |
TASK-85718
Refs #299
What changed
shadowandcanaryuse the intended rollout cohort inwebhookCanaryReposprimaryuses globalenabledRepos, or all repositories when that allowlist is emptyGITHUB_AGENT_BRIDGE_WEBHOOK_OBSERVATION_STARTED_ATas an auditable rollout epochValidation
pytest -q— 676 passednpm test -- --run— 79 passednpm run build— passedgit diff --check— passedRollout
No webhook mode change. After merge/release, set the observation timestamp to the deployment instant, restart all runtimes, and begin a new seven-day canary window. After promotion, the second
primarywindow evaluates the full global eligibility scope rather than the former canary only.