Skip to content

fix: scope webhook promotion to rollout cohort - #334

Merged
ecarreras merged 2 commits into
mainfrom
fix/issue-299-promotion-window
Nov 2, 2026
Merged

ecarreras merged 2 commits into
mainfrom
fix/issue-299-promotion-window

Conversation

@giscebot

@giscebot giscebot commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

TASK-85718

Refs #299

What changed

  • resolve promotion coverage and exceptions from the active webhook mode:
    • shadow and canary use the intended rollout cohort in webhookCanaryRepos
    • primary uses global enabledRepos, or all repositories when that allowlist is empty
  • report actionable IMAP/webhook traffic outside a constrained scope separately
  • add GITHUB_AGENT_BRIDGE_WEBHOOK_OBSERVATION_STARTED_AT as an auditable rollout epoch
  • show the mode-specific cohort or eligibility scope in the dashboard and document the rollout control

Validation

  • pytest -q — 676 passed
  • dashboard npm test -- --run — 79 passed
  • dashboard npm run build — passed
  • git diff --check — passed

Rollout

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 primary window evaluates the full global eligibility scope rather than the former canary only.

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 pilipilisbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@giscebot

giscebot commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

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:

  • canary: webhookCanaryRepos
  • primary: enabledRepos, or unrestricted when enabledRepos is empty
  • shadow: keep and document the intended rollout cohort

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.

@ecarreras

Copy link
Copy Markdown
Member

@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>
@giscebot

giscebot commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

Solucionat al commit c3cbda0.

  • shadow i canary mantenen webhookCanaryRepos com a cohort d'observació.
  • primary usa enabledRepos; si és buit, l'scope és global i no es converteix en AND 0.
  • La regressió reprodueix un mismatch accionable fora de l'antic canary: ara bloqueja promotion_state i apareix a /exceptions.
  • Dashboard i documentació identifiquen explícitament cohort canary/shadow versus eligibilitat global de primary.

Validació: 676 tests Python, 79 tests del dashboard, build de producció i git diff --check. La CI del head c3cbda0 és verda a dashboard, Python 3.11 i Python 3.12; la PR continua MERGEABLE.

No s'ha activat primary ni s'ha tocat producció. El canvi només evita el fals positiu durant la segona finestra d'observació.

@ecarreras

Copy link
Copy Markdown
Member

@pilipilisbot revisa aquesta pull-request

@pilipilisbot pilipilisbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ecarreras
ecarreras merged commit 969635a into main Nov 2, 2026
5 checks passed
@ecarreras
ecarreras deleted the fix/issue-299-promotion-window branch November 2, 2026 16:25
@giscebot

giscebot commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

Post-merge follow-up: el worktree dedicat de la PR s’ha trobat net i s’ha retirat correctament.

La implementació queda mergejada a 969635a amb els checks de dashboard, Python 3.11 i Python 3.12 en verd. Aquesta neteja és independent del rollout: no s’ha activat primary; cal mantenir els prerequisits operatius i la finestra d’observació documentats a la PR i a #299.

@pilipilisbot

Copy link
Copy Markdown
Collaborator

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 c3cbda0; l'he retirat i he verificat que ja no figura a git worktree list ni existeix al path /home/openclaw/projects/gisce-ti/worktrees/github-agent-bridge-pr-334.

La PR continua fusionada a 969635a. Els checks de dashboard i Python 3.11/3.12 del head van passar. Aquesta neteja local no canvia l'estat del rollout ni activa primary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants