code/cl8y-ecosystem-legal
4
0
Fork 0
Code Issues 4 Pull requests 3 Projects Releases Packages Wiki Activity

Bot: fail-closed compliance when Legal API errors (replace unwrap_or(false)) #5

New issue
Closed
opened 2026-08-11 01:02:43 +00:00 by PlasticDigits · 7 comments
PlasticDigits commented 2026-08-11 01:02:43 +00:00 (Migrated from gitlab.com)
Copy link

Summary

Replace fail-open unwrap_or(false) on Legal API status checks in the Telegram terms bot so API outages/rate-limits do not mark signed users non-compliant or kick them.

Tracked from internal audit audits/INTERNAL_COMPOSER_1786408744.md (H1) and gaps/GAP_1786322222.md.

Current codebase

The bot decides compliance by calling LegalApi::is_signed_latest (HTTP GET /api/v1/signatures/status). On any Err (timeout, 5xx, 429, parse failure), several paths treat the user as unsigned:

// bot/src/enforcement.rs — pattern appears in multiple places
api.is_signed_latest(chat_id, user_id).await.unwrap_or(false)
Call site Behavior today on API error
handle_group_message mark_non_compliant (starts/resets grace clock)
announce_terms_update member loop same
run_kicks proceeds toward ban_chat_member if still “unsigned”

LegalApi (bot/src/api_client.rs) has no retries, circuit breaker, or authenticated channel to the API. There are no bot enforcement unit/integration tests in CI for these failure paths.

Related (in scope of this issue as guardrails, not separate products):

  • Kicks must not run when the last successful status check is stale / API is unhealthy.
  • Logging/metrics should make API-error fail-closed decisions visible to ops.

Out of scope for this issue (file separately if desired): grace reset on rejoin, admin/creator kick exemption, lurker tracking, property upsert, Telegram crypto HMAC.

Why the new implementation is needed

Fail-open into punishment is unsafe:

  1. Transient Legal API outage or rate-limit → signed members marked non-compliant.
  2. Enforcement ticker (run_kicks) can wrongfully kick users who already signed latest terms.
  3. Reputational and operational harm in production Telegram groups; trust in the terms gate collapses under infra blips.

Correct posture for enforcement: fail-closed — when status is unknown, do not escalate (no new non-compliance marks that advance kicks; never kick on unknown).

Constraints / guardrails

  1. Do not kick when is_signed_latest returns Err or when the bot has no fresh successful status for that user.
  2. Do not call mark_non_compliant solely because of an API error (that starts grace and leads to kicks).
  3. Prefer preserving last known good compliance state (or leaving the row untouched) over inventing “unsigned”.
  4. Optionally clear non-compliance only on explicit Ok(true); on Ok(false) keep current mark/clear semantics.
  5. Log at warn/error with chat_id, user_id, and error cause when failing closed.
  6. No new public API surface required unless you introduce a bot-only authenticated status path (not required for MVP).
  7. Keep allowlisted-chat checks unchanged; do not broaden kick privileges.
  8. Document the invariant in a short comment near the helper (and optionally skills/ if one exists for the bot later).

Relevant files

Path Role
bot/src/enforcement.rs handle_group_message, announce_terms_update, run_kicks
bot/src/api_client.rs status, is_signed_latest
bot/src/scheduler.rs enforcement / terms poll loops
bot/src/handlers.rs per-message → enforcement
bot/src/db.rs mark_non_compliant, clear_compliant, overdue queries
bot/Cargo.toml add tests / deps if needed
.gitlab-ci.yml ensure cargo test runs for bot/

Recommended direction

  1. Introduce a small helper, e.g. ComplianceCheck::{SignedLatest, NotSignedLatest, Unknown}, mapping Result<bool, _> → enum (never coerce Err → false).
  2. Centralize all three call sites to use the helper.
  3. Policy matrix:
    • SignedLatest → clear_compliant
    • NotSignedLatest → mark_non_compliant (existing semantics)
    • Unknown → no DB escalation; skip kick; increment/log metric
  4. For run_kicks: only kick when status is explicitly NotSignedLatest (and member still active). On Unknown, continue without ban.
  5. Consider a short in-memory “API healthy” flag or backoff so a storm of errors does not spam Telegram/logs.
  6. Add bot cargo test job in CI (lint alone is insufficient).

Acceptance criteria

  • No unwrap_or(false) (or equivalent) on compliance status in enforcement/kick paths.
  • API error during handle_group_message does not insert/update bot_member_compliance as non-compliant.
  • API error during run_kicks does not ban/kick the user.
  • API error during terms-announce member loop does not mass-mark members non-compliant.
  • Ok(true) still clears compliance; Ok(false) still marks non-compliant.
  • Structured warning logs on Unknown.
  • Unit tests cover the three outcomes; CI runs cargo test for bot/.
  • README or code comment documents fail-closed invariant.

Test plan — functional paths

Path Expectation
Happy: API returns signed_latest: true clear compliance; no kick
Happy: API returns signed_latest: false mark non-compliant; kick only after grace via existing overdue logic
API 500 / timeout on group message no mark; no kick
API 429 on kick pass skip that user; no ban
API recovers next tick correct mark/clear resumes
Mixed: some users Ok, some Err in one kick pass only explicit NotSigned + overdue kicked

Test plan — attack / abuse / failure vectors

Vector Expectation
Sustained API outage during enforcement hour zero wrongful kicks
Attacker / load causes API rate-limit bot fails closed; does not amplify by marking everyone
Flapping API (intermittent errors) no oscillation into kicks without explicit NotSigned
Malicious or empty JSON from API (parse error) Unknown → fail closed (same as transport error)
Hostile LEGAL_API_BASE_URL returning errors fail closed (note: false signed_latest from hostile API is a separate trust issue, out of scope)

Verification criteria

  1. cd bot && cargo test passes locally and in CI.
  2. Manual or integration simulation: stop Legal API → run enforcement once → confirm no ban_chat_member for previously signed fixture users; compliance rows unchanged for error path.
  3. Code search: no unwrap_or(false) on is_signed_latest / status in bot/src.
  4. Reviewer confirms policy matrix matches acceptance criteria.
## Summary Replace fail-open `unwrap_or(false)` on Legal API status checks in the Telegram terms bot so API outages/rate-limits do **not** mark signed users non-compliant or kick them. Tracked from internal audit `audits/INTERNAL_COMPOSER_1786408744.md` (H1) and `gaps/GAP_1786322222.md`. --- ## Current codebase The bot decides compliance by calling `LegalApi::is_signed_latest` (HTTP `GET /api/v1/signatures/status`). On **any** `Err` (timeout, 5xx, 429, parse failure), several paths treat the user as **unsigned**: ```rust // bot/src/enforcement.rs — pattern appears in multiple places api.is_signed_latest(chat_id, user_id).await.unwrap_or(false) ``` | Call site | Behavior today on API error | |-----------|-----------------------------| | `handle_group_message` | `mark_non_compliant` (starts/resets grace clock) | | `announce_terms_update` member loop | same | | `run_kicks` | proceeds toward `ban_chat_member` if still “unsigned” | `LegalApi` (`bot/src/api_client.rs`) has no retries, circuit breaker, or authenticated channel to the API. There are **no** bot enforcement unit/integration tests in CI for these failure paths. Related (in scope of this issue as guardrails, not separate products): - Kicks must not run when the last successful status check is stale / API is unhealthy. - Logging/metrics should make API-error fail-closed decisions visible to ops. Out of scope for this issue (file separately if desired): grace reset on rejoin, admin/creator kick exemption, lurker tracking, property upsert, Telegram crypto HMAC. --- ## Why the new implementation is needed Fail-open into punishment is unsafe: 1. Transient Legal API outage or rate-limit → signed members marked non-compliant. 2. Enforcement ticker (`run_kicks`) can **wrongfully kick** users who already signed latest terms. 3. Reputational and operational harm in production Telegram groups; trust in the terms gate collapses under infra blips. Correct posture for enforcement: **fail-closed** — when status is unknown, do not escalate (no new non-compliance marks that advance kicks; never kick on unknown). --- ## Constraints / guardrails 1. **Do not kick** when `is_signed_latest` returns `Err` or when the bot has no fresh successful status for that user. 2. **Do not** call `mark_non_compliant` solely because of an API error (that starts grace and leads to kicks). 3. Prefer preserving last known good compliance state (or leaving the row untouched) over inventing “unsigned”. 4. Optionally clear non-compliance only on explicit `Ok(true)`; on `Ok(false)` keep current mark/clear semantics. 5. Log at `warn`/`error` with `chat_id`, `user_id`, and error cause when failing closed. 6. No new public API surface required unless you introduce a bot-only authenticated status path (not required for MVP). 7. Keep allowlisted-chat checks unchanged; do not broaden kick privileges. 8. Document the invariant in a short comment near the helper (and optionally `skills/` if one exists for the bot later). --- ## Relevant files | Path | Role | |------|------| | `bot/src/enforcement.rs` | `handle_group_message`, `announce_terms_update`, `run_kicks` | | `bot/src/api_client.rs` | `status`, `is_signed_latest` | | `bot/src/scheduler.rs` | enforcement / terms poll loops | | `bot/src/handlers.rs` | per-message → enforcement | | `bot/src/db.rs` | `mark_non_compliant`, `clear_compliant`, overdue queries | | `bot/Cargo.toml` | add tests / deps if needed | | `.gitlab-ci.yml` | ensure `cargo test` runs for `bot/` | --- ## Recommended direction 1. Introduce a small helper, e.g. `ComplianceCheck::{SignedLatest, NotSignedLatest, Unknown}`, mapping `Result<bool, _>` → enum (never coerce `Err` → false). 2. Centralize all three call sites to use the helper. 3. Policy matrix: - `SignedLatest` → `clear_compliant` - `NotSignedLatest` → `mark_non_compliant` (existing semantics) - `Unknown` → **no DB escalation**; **skip kick**; increment/log metric 4. For `run_kicks`: only kick when status is explicitly `NotSignedLatest` (and member still active). On `Unknown`, `continue` without ban. 5. Consider a short in-memory “API healthy” flag or backoff so a storm of errors does not spam Telegram/logs. 6. Add `bot` `cargo test` job in CI (lint alone is insufficient). --- ## Acceptance criteria - [ ] No `unwrap_or(false)` (or equivalent) on compliance status in enforcement/kick paths. - [ ] API error during `handle_group_message` does **not** insert/update `bot_member_compliance` as non-compliant. - [ ] API error during `run_kicks` does **not** ban/kick the user. - [ ] API error during terms-announce member loop does **not** mass-mark members non-compliant. - [ ] `Ok(true)` still clears compliance; `Ok(false)` still marks non-compliant. - [ ] Structured warning logs on Unknown. - [ ] Unit tests cover the three outcomes; CI runs `cargo test` for `bot/`. - [ ] README or code comment documents fail-closed invariant. --- ## Test plan — functional paths | Path | Expectation | |------|-------------| | Happy: API returns `signed_latest: true` | clear compliance; no kick | | Happy: API returns `signed_latest: false` | mark non-compliant; kick only after grace via existing overdue logic | | API 500 / timeout on group message | no mark; no kick | | API 429 on kick pass | skip that user; no ban | | API recovers next tick | correct mark/clear resumes | | Mixed: some users Ok, some Err in one kick pass | only explicit NotSigned + overdue kicked | --- ## Test plan — attack / abuse / failure vectors | Vector | Expectation | |--------|-------------| | Sustained API outage during enforcement hour | **zero** wrongful kicks | | Attacker / load causes API rate-limit | bot fails closed; does not amplify by marking everyone | | Flapping API (intermittent errors) | no oscillation into kicks without explicit NotSigned | | Malicious or empty JSON from API (parse error) | Unknown → fail closed (same as transport error) | | Hostile `LEGAL_API_BASE_URL` returning errors | fail closed (note: false `signed_latest` from hostile API is a separate trust issue, out of scope) | --- ## Verification criteria 1. `cd bot && cargo test` passes locally and in CI. 2. Manual or integration simulation: stop Legal API → run enforcement once → confirm no `ban_chat_member` for previously signed fixture users; compliance rows unchanged for error path. 3. Code search: no `unwrap_or(false)` on `is_signed_latest` / status in `bot/src`. 4. Reviewer confirms policy matrix matches acceptance criteria.
PlasticDigits commented 2026-08-11 01:10:07 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in commit 90b3b79786

mentioned in commit 90b3b79786e67058b5b1a8cbbdab223ab8f3b186
PlasticDigits commented 2026-08-11 01:10:20 +00:00 (Migrated from gitlab.com)
Copy link

Implemented fail-closed compliance on branch fix/issue-5-fail-closed-compliance (MR linked).

Done vs acceptance criteria

  • No unwrap_or(false) on status in enforcement/kick paths — centralized in bot/src/compliance.rs
  • API error in handle_group_message → Hold (no mark_non_compliant)
  • API error in run_kicks → skip ban
  • API error in announce_terms_update loop → Hold per member
  • Ok(true) clears; Ok(false) marks
  • Structured warn on Unknown (chat_id, user_id, error, context)
  • Unit tests for three outcomes; CI job test:rust-bot
  • Docs: module invariant, README Telegram section, skills/bot-enforcement/SKILL.md

Not done (explicitly out of scope for #5)

  • In-memory API-healthy / circuit breaker / status TTL cache
  • Grace reset on rejoin, admin/creator kick exemption, lurker tracking, property upsert, Telegram crypto HMAC
  • Live Telegram + stopped-API integration simulation (policy covered by unit tests)

Verify locally

cd bot && cargo test
cd bot && cargo clippy --all-targets -- -D warnings
rg 'unwrap_or\(false\)' bot/src  # should only appear in comments
Implemented fail-closed compliance on branch `fix/issue-5-fail-closed-compliance` (MR linked). ### Done vs acceptance criteria - [x] No `unwrap_or(false)` on status in enforcement/kick paths — centralized in `bot/src/compliance.rs` - [x] API error in `handle_group_message` → Hold (no `mark_non_compliant`) - [x] API error in `run_kicks` → skip ban - [x] API error in `announce_terms_update` loop → Hold per member - [x] `Ok(true)` clears; `Ok(false)` marks - [x] Structured `warn` on Unknown (`chat_id`, `user_id`, `error`, `context`) - [x] Unit tests for three outcomes; CI job `test:rust-bot` - [x] Docs: module invariant, README Telegram section, `skills/bot-enforcement/SKILL.md` ### Not done (explicitly out of scope for #5) - In-memory API-healthy / circuit breaker / status TTL cache - Grace reset on rejoin, admin/creator kick exemption, lurker tracking, property upsert, Telegram crypto HMAC - Live Telegram + stopped-API integration simulation (policy covered by unit tests) ### Verify locally ```bash cd bot && cargo test cd bot && cargo clippy --all-targets -- -D warnings rg 'unwrap_or\(false\)' bot/src # should only appear in comments ```
PlasticDigits commented 2026-08-11 01:10:22 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in merge request !27

mentioned in merge request !27
PlasticDigits commented 2026-08-11 01:11:02 +00:00 (Migrated from gitlab.com)
Copy link

MR !27 now has the fix commits (source branch updated). Local tests green; CI blocked by project ci_quota_exceeded — re-run when minutes available.

MR !27 now has the fix commits (source branch updated). Local tests green; CI blocked by project `ci_quota_exceeded` — re-run when minutes available.
PlasticDigits commented 2026-08-11 01:25:41 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in commit 6470432a8e

mentioned in commit 6470432a8e4cb2df985b7c67743cb5cb3dd03f4e
PlasticDigits commented 2026-08-11 01:25:55 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in commit 9f45ac6e58

mentioned in commit 9f45ac6e586f7df5e52e9147a58a954a5edd310d
PlasticDigits (Migrated from gitlab.com) closed this issue 2026-08-11 01:25:55 +00:00
PlasticDigits commented 2026-08-11 01:26:14 +00:00 (Migrated from gitlab.com)
Copy link

Merge verification (!27 → main)

Verdict: PASS_WITH_NOTES — all issue #5 acceptance criteria met; MR !27 merged without waiting for CI / without auto-merge.

Conflict fix

After !26 landed, merging main into the !27 branch conflicted only in skills/security-ops/SKILL.md (out-of-scope paragraph). Resolved by keeping both the #5 bot-enforcement cross-link and the #6 SSRF out-of-scope note. Push: 6470432.

Acceptance criteria

  • No unwrap_or(false) on compliance status in enforcement/kick/announce paths
  • API error during handle_group_message does not mark non-compliant
  • API error during run_kicks does not ban/kick
  • API error during terms-announce member loop does not mass-mark
  • Ok(true) clears; Ok(false) marks
  • Structured warning logs on Unknown (chat_id/user_id/context)
  • Unit tests cover three outcomes; test:rust-bot CI job present
  • README / code comment / skill document fail-closed invariant

Local checks (post-conflict resolution)

  • cd bot && cargo test — PASS (8/8)
  • Earlier review also: cargo fmt --check + cargo clippy --all-targets -- -D warnings clean

Problems / residual risks (non-blocking)

  1. CI still not green on the MR — previous pipelines failed with ci_quota_exceeded; test:rust-bot / lint:rust-bot were not re-run before merge (per instruction). Re-run when minutes available.
  2. No enforcement-path integration tests with a mock Legal API (unit-level matrix only; acknowledged out of scope in the MR).
  3. lint:rust-bot CI uses cargo clippy without --all-targets (local used --all-targets); minor inconsistency with api/ lint job.
  4. Manual verification not run here: stop Legal API → confirm no ban_chat_member / unchanged compliance rows for previously signed users.

No acceptance-criteria defects found; closing via !27.

## Merge verification (!27 → main) **Verdict:** PASS_WITH_NOTES — all issue #5 acceptance criteria met; MR !27 merged without waiting for CI / without auto-merge. ### Conflict fix After !26 landed, merging `main` into the !27 branch conflicted only in `skills/security-ops/SKILL.md` (out-of-scope paragraph). Resolved by keeping both the #5 `bot-enforcement` cross-link and the #6 SSRF out-of-scope note. Push: `6470432`. ### Acceptance criteria - [x] No `unwrap_or(false)` on compliance status in enforcement/kick/announce paths - [x] API error during `handle_group_message` does not mark non-compliant - [x] API error during `run_kicks` does not ban/kick - [x] API error during terms-announce member loop does not mass-mark - [x] `Ok(true)` clears; `Ok(false)` marks - [x] Structured warning logs on `Unknown` (`chat_id`/`user_id`/`context`) - [x] Unit tests cover three outcomes; `test:rust-bot` CI job present - [x] README / code comment / skill document fail-closed invariant ### Local checks (post-conflict resolution) - `cd bot && cargo test` — PASS (8/8) - Earlier review also: `cargo fmt --check` + `cargo clippy --all-targets -- -D warnings` clean ### Problems / residual risks (non-blocking) 1. **CI still not green on the MR** — previous pipelines failed with `ci_quota_exceeded`; `test:rust-bot` / `lint:rust-bot` were not re-run before merge (per instruction). Re-run when minutes available. 2. **No enforcement-path integration tests** with a mock Legal API (unit-level matrix only; acknowledged out of scope in the MR). 3. **`lint:rust-bot` CI** uses `cargo clippy` without `--all-targets` (local used `--all-targets`); minor inconsistency with `api/` lint job. 4. **Manual verification not run here:** stop Legal API → confirm no `ban_chat_member` / unchanged compliance rows for previously signed users. No acceptance-criteria defects found; closing via !27.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
issue/34
issue/37
renovate/configure
No results found.
Labels
Clear labels   
agent:implement
agent-control

  
agent:ready
agent-control

  
api

   
bot

   
bug

   
ci

   
enhancement

   
ready
agent-control

  
security

   
terra-classic

   
testing

   
ux

   
web
No labels
agent:implement
agent:ready
api
bot
bug
ci
enhancement
ready
security
terra-classic
testing
ux
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
code/cl8y-ecosystem-legal#5
No description provided.