54 lines
4.2 KiB
Markdown
54 lines
4.2 KiB
Markdown
# Code Review: Community Policy Engine (m10-community-policy-engine)
|
||
|
||
## Summary
|
||
|
||
The implementation delivers all 6 tasks: schema-level `CommunityPolicy` types, `SchemaBuilder` integration with validation, `CommunityPolicyEvaluator` for write enforcement, read suppression threading through the retrieval pipeline, integration tests for all 10 spec scenarios, and a clean full-suite pass with fmt/clippy.
|
||
|
||
## Correctness
|
||
|
||
### Write Enforcement (T1–T3)
|
||
- `CommunityPolicy` struct correctly models `allowed_write_signals`, `denied_write_signals`, `allowed_read_signals`, `denied_read_signals` — all four lists are present and semantically distinct.
|
||
- `CommunityPolicyEvaluator::check_write` applies the correct priority: deny list checked first, then allow list enforcement when non-empty, then pass-through when both lists are empty (admin case). Logic matches spec.
|
||
- `SchemaBuilder::community_policy` validation correctly rejects: duplicate role names, unknown signal names in any policy list, allow/deny conflicts within the same list type. All three validated at `build()` time before the schema is sealed.
|
||
- `signal_with_community_policy` correctly routes through the evaluator and delegates to the signal write path on success. Error variants (`PolicyViolationKind::CommunityWriteDenied`, `CommunityWriteNotAllowed`) are distinct and carry the signal name for diagnostics.
|
||
|
||
### Read Suppression (T4)
|
||
- `CommunityContext` is carried through `Retrieve` → `RetrieveExecutor::stage3_score` → `ProfileExecutor` via `with_suppressed_signals(HashSet<String>)`.
|
||
- The suppressed set is built correctly: `denied_read_signals` always suppressed; signals not in `allowed_read_signals` suppressed when the allow list is non-empty; empty allow list → no suppression (admin passthrough).
|
||
- `ProfileExecutor::compute_raw_score` filters boosts whose signal is in the suppressed set.
|
||
- `ProfileExecutor::score_by_sort` returns 0.0 for all sort modes whose primary signal is suppressed (Hot→view, Trending→view+share, MostViewed→view, MostLiked→like, MostFollowed→follow, CreatorEngagementRate→view+like, MostCommented→comment, MostShared→share, LiveViewerCount→viewer_count, Controversial→like+dislike). Complete coverage.
|
||
- `is_suppressed` is a fast-path `O(1)` lookup with an early exit when the suppressed set is empty — no overhead on requests without community context.
|
||
|
||
### Unknown Role Error (T4)
|
||
- When `query.community` is set but the role is not in the schema, `stage3_score` returns `QueryError::InvalidFilter` with a message referencing the role name. This satisfies test I10.
|
||
|
||
## Test Coverage (T5)
|
||
|
||
All 10 spec scenarios are covered:
|
||
- I1: Write allowed signal succeeds and is readable with `read_decay_score(..., 0)` using `Timestamp::now()`.
|
||
- I2: Write of `denied_write_signals` signal returns policy violation error referencing the signal name.
|
||
- I3: Write of signal not in non-empty `allowed_write_signals` list returns policy violation.
|
||
- I4: Admin role (empty allow/deny lists) allows writes of all signals.
|
||
- I5: Retrieve with moderator context (pin denied) returns same result count as unconstrained retrieve — suppression affects scoring, not candidate count.
|
||
- I6: Retrieve without community context does not suppress any signals.
|
||
- I7: Duplicate community policy name rejected at schema build time.
|
||
- I8: Unknown signal name in policy list rejected at schema build time.
|
||
- I9: Signal in both allow and deny list rejected at schema build time.
|
||
- I10: Write with unknown role name returns error referencing the role.
|
||
|
||
## Code Health
|
||
|
||
- `cargo clippy -- -D warnings`: clean.
|
||
- `cargo fmt -- --check`: clean (revocation/tests.rs line-length formatting fixed).
|
||
- Duplicate module conflict (`entities/revocation.rs` vs `entities/revocation/mod.rs`) resolved by removing the stale flat file; the directory-form module is the canonical implementation.
|
||
- `ProfileExecutor::new` correctly changed from `const fn` to `fn` to support `HashSet::new()` initialization.
|
||
- 1299 lib tests passing; 10/10 M10 integration tests passing; m5_uat (9), m6_uat (9), m7_uat (10), m8_uat (8) all green.
|
||
|
||
## Issues Found
|
||
|
||
None. All spec requirements are implemented, validated, and tested.
|
||
|
||
## Verdict
|
||
|
||
**Approved.** The implementation is complete, correct, and production-ready.
|