feat(signals): share the scout creation check between endpoint and facade - #106430
Conversation
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/facade/task_run_signals.py |
75.0% | 25 |
products/tasks/backend/facade/api.py |
83.5% | 709–710, 725, 727–732, 734–738, 4542–4544, 4549, 4552–4553, 4593, 4640 |
products/posthog_ai/backend/exec_commands.py |
87.2% | 51, 60, 69, 73, 81–82 |
products/tasks/backend/models.py |
92.6% | 981, 985 |
products/signals/backend/facade/api.py |
94.4% | 1192 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 36153199991 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.1% |
92 / 316 |
demo |
████████████░░░░░░░░ 57.8% |
1,545 / 2,673 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
aeo |
██████████████░░░░░░ 70.5% |
467 / 662 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 81.2% |
21,451 / 26,424 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
ml_inference |
█████████████████░░░ 86.7% |
451 / 520 |
cdp |
██████████████████░░ 88.2% |
4,548 / 5,155 |
mcp_analytics |
██████████████████░░ 88.6% |
4,704 / 5,308 |
signals |
██████████████████░░ 88.7% |
52,451 / 59,133 |
product_tours |
██████████████████░░ 89.3% |
1,331 / 1,491 |
dashboards |
██████████████████░░ 89.8% |
7,234 / 8,056 |
data_warehouse |
██████████████████░░ 89.9% |
13,738 / 15,285 |
cohorts |
██████████████████░░ 90.2% |
8,238 / 9,138 |
notebooks |
██████████████████░░ 90.2% |
15,287 / 16,945 |
streamlit_apps |
██████████████████░░ 90.7% |
2,625 / 2,895 |
managed_warehouse |
██████████████████░░ 90.9% |
10,215 / 11,234 |
tasks |
██████████████████░░ 91.1% |
73,926 / 81,124 |
data_modeling |
██████████████████░░ 91.4% |
10,491 / 11,483 |
business_knowledge |
██████████████████░░ 91.6% |
6,899 / 7,528 |
engineering_analytics |
██████████████████░░ 91.7% |
11,002 / 11,999 |
exports |
██████████████████░░ 91.8% |
9,685 / 10,555 |
ai_training |
██████████████████░░ 92.2% |
356 / 386 |
conversations |
███████████████████░ 92.5% |
28,705 / 31,025 |
early_access_features |
███████████████████░ 92.6% |
1,341 / 1,448 |
managed_migrations |
███████████████████░ 92.7% |
1,581 / 1,705 |
visual_review |
███████████████████░ 92.8% |
9,244 / 9,966 |
canvas |
███████████████████░ 92.8% |
6,877 / 7,409 |
approvals |
███████████████████░ 93.0% |
3,919 / 4,214 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
error_tracking |
███████████████████░ 93.1% |
15,708 / 16,874 |
notifications |
███████████████████░ 93.2% |
1,145 / 1,229 |
slack_app |
███████████████████░ 93.2% |
13,677 / 14,674 |
surveys |
███████████████████░ 93.3% |
6,571 / 7,040 |
stamphog |
███████████████████░ 93.4% |
7,716 / 8,262 |
context_layer |
███████████████████░ 93.8% |
3,373 / 3,595 |
web_analytics |
███████████████████░ 93.9% |
21,653 / 23,051 |
ai_observability |
███████████████████░ 94.0% |
20,588 / 21,892 |
alerts |
███████████████████░ 94.1% |
8,553 / 9,094 |
billing_alerts |
███████████████████░ 94.1% |
2,094 / 2,226 |
mcp_store |
███████████████████░ 94.4% |
8,940 / 9,472 |
wizard |
███████████████████░ 94.4% |
5,791 / 6,134 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
workflows |
███████████████████░ 94.8% |
13,438 / 14,170 |
review_hog |
███████████████████░ 94.9% |
11,490 / 12,109 |
annotations |
███████████████████░ 95.1% |
817 / 859 |
customer_analytics |
███████████████████░ 95.1% |
24,669 / 25,932 |
endpoints |
███████████████████░ 95.1% |
9,211 / 9,681 |
legal_documents |
███████████████████░ 95.2% |
2,311 / 2,427 |
marketing_analytics |
███████████████████░ 95.3% |
19,047 / 19,991 |
posthog_ai |
███████████████████░ 95.4% |
2,489 / 2,610 |
logs |
███████████████████░ 95.4% |
15,235 / 15,967 |
tracing |
███████████████████░ 95.4% |
3,483 / 3,650 |
growth |
███████████████████░ 95.4% |
9,812 / 10,282 |
actions |
███████████████████░ 95.5% |
756 / 792 |
messaging |
███████████████████░ 95.9% |
3,766 / 3,927 |
skills |
███████████████████░ 95.9% |
6,649 / 6,932 |
replay_vision |
███████████████████░ 96.0% |
26,750 / 27,878 |
autoresearch |
███████████████████░ 96.0% |
7,716 / 8,037 |
product_analytics |
███████████████████░ 96.2% |
28,495 / 29,617 |
revenue_analytics |
███████████████████░ 96.4% |
1,876 / 1,946 |
access_control |
███████████████████░ 96.4% |
7,122 / 7,386 |
user_interviews |
███████████████████░ 96.5% |
2,859 / 2,963 |
feature_flags |
███████████████████░ 96.5% |
25,146 / 26,046 |
experiments |
███████████████████░ 96.6% |
33,963 / 35,146 |
warehouse_sources |
███████████████████░ 97.2% |
443,964 / 456,678 |
data_quality |
████████████████████ 97.7% |
7,592 / 7,774 |
links |
████████████████████ 97.9% |
234 / 239 |
security |
████████████████████ 98.1% |
1,258 / 1,283 |
metrics |
████████████████████ 98.1% |
4,085 / 4,166 |
analytics_platform |
████████████████████ 98.3% |
2,778 / 2,827 |
data_catalog |
████████████████████ 98.3% |
3,931 / 3,999 |
pulse |
████████████████████ 98.5% |
2,043 / 2,075 |
live_debugger |
████████████████████ 99.2% |
626 / 631 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
[Medium risk] Refactors scout creation permission check into a shared function. No blocking runtime failure was established, but the facade contract must satisfy the repository's explicit requirement before merging. Reviews (1) · Last reviewed commit: "feat(signals): share the scout creation ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
🚫 This stack was removed from the merge queue because the GitHub stack changed. Please re-submit it in order to merge. See more details here. |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Greptile's unresolved inline comment flags a real architectural violation — the new public facade function accepts raw Team/User ORM models instead of the frozen-contract inputs the repo's own AGENTS.md requires at facade boundaries — and the diff doesn't address it; the discussion summary reiterates it must be satisfied before merging.
- 👍 on the PR from greptile-apps[bot].
- Unresolved Greptile comment: scout_creation_available facade function takes Team/User ORM instances instead of stable identifiers/frozen contracts, violating documented facade-boundary rules (AGENTS.MD).
- Unresolved Greptile comment: new facade tests only cover root-team cases, missing a child-environment case that would catch a regression checking the wrong project.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 37L, 4F substantive, 70L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (70L, 6F, single-area, feat) |
| stamphog 2.1.0 | .stamphog/policy.yml @ ec508ef · reviewed head ec508ef |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe facade now reports scout-creation availability by checking project access to the requested team, enrollment on the canonical team, and the user’s editor access. The creation endpoint uses the same access helper. Tests cover enrollment cases, denied access, and the existing access-denial expectation. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to For some child-environment users, the new availability check can say scout creation is available even though the create request will be denied. This is a bounded eligibility mismatch; add the canonical-parent membership check before relying on the API. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Comment |
ec508ef to
43fb7d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/signals/backend/facade/api.py-1174-1174 (1)
1174-1174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck project access on the requested team.
scout_creation_availableuses the canonical team for skill access, butAccessControlPermissionchecks project access on the requested child team. Canonical-team access can therefore coexist with denial of the endpoint’s project-access check. Checkhas_project_accesson the requested team, notcanonical_team.🐛 Suggested fix
+from products.access_control.backend.facade.user_access_control import UserAccessControl ... canonical_team = team.parent_team or team if not team_is_enrolled(canonical_team.id): return False + if not UserAccessControl(user=user, team=team).has_project_access: + return False return can_create_scout(user, canonical_team)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 5d13fc62-20fa-4450-8499-501c85c7b2b3
📒 Files selected for processing (2)
products/signals/backend/facade/api.pyproducts/signals/backend/test/test_facade_scout_creation_available.py
Included review availability: Your plan provides up to 12 included reviews per hour; 3 remain after this review.
43fb7d9 to
c2b31fc
Compare
|
@coderabbitai review-body comment:
|
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/signals/backend/facade/api.py-1169-1178 (1)
1169-1178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMirror the child-team skill gate in
scout_creation_available.For a child environment, the facade checks project access on the child but checks skill-editor access on the canonical parent. The creation endpoint also requires
llm_skilleditor access on the requested child throughAccessControlPermission. A user can therefore receiveTruehere and then receive a permission denial when creating the scout.Suggested fix
- if not UserAccessControl(user=user, team=team).has_project_access: + access = UserAccessControl(user=user, team=team) + if not access.has_project_access: + return False + if not access.check_access_level_for_resource("llm_skill", "editor"): return False
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8f27c569-9142-4f1f-a683-9e008cc541ad
📒 Files selected for processing (2)
products/signals/backend/facade/api.pyproducts/signals/backend/test/test_facade_scout_creation_available.py
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline products/signals/backend/facade/api.py --match scout_creation_available
rg -n -C 10 'def scout_creation_available|check_project_access|project_access|requested_team' products/signals/backend/facade/api.py products/signals/backend/test/test_facade_scout_creation_available.py
git show --format='%h %s' --stat c2b31fc0af5 -- products/signals/backend/facade/api.py products/signals/backend/test/test_facade_scout_creation_available.py | head -65Length of output: 5702
You are interacting with an AI system. |
c2b31fc to
0432a8e
Compare
|
@coderabbitai review-body comment:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
products/signals/backend/test/test_facade_scout_creation_available.py (1)
41-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest access denial on a child environment.
Both denial cases pass
self.team, so the requested and canonical teams are identical. A regression that checks access on the canonical parent would still pass. Extend the existing parameterization with a child environment and assert thatUserAccessControlreceives the child team. The child fixture in the enrollment test covers enrollment only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 581541ae-b532-4c3a-96e5-7bc5a2671b88
📒 Files selected for processing (2)
products/signals/backend/facade/api.pyproducts/signals/backend/test/test_facade_scout_creation_available.py
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline products/signals/backend/facade/api.py --match scout_creation_available
ast-grep outline products/signals/backend/scout_harness/create_access.py --match can_create_scout
ast-grep outline products/signals/backend/scout_harness/views.py --match _assert_can_create_scout
git show --format='%h %s' --stat 0432a8e8daf -- products/signals/backend/facade/api.py products/signals/backend/scout_harness/create_access.py products/signals/backend/scout_harness/views.py products/signals/backend/test/test_facade_scout_creation_available.py
git show --format='' 0432a8e8daf -- products/signals/backend/facade/api.py products/signals/backend/scout_harness/create_access.py products/signals/backend/scout_harness/views.py products/signals/backend/test/test_facade_scout_creation_available.py
rg -n -C 12 'def scout_creation_available|def can_create_scout|def _assert_can_create_scout|child|editor|llm_skill' products/signals/backend/facade/api.py products/signals/backend/scout_harness/create_access.py products/signals/backend/scout_harness/views.py products/signals/backend/test/test_facade_scout_creation_available.py | head -260Length of output: 33106
You are interacting with an AI system. |
…d the facade can_create_scout holds the create endpoint's skill editor check, and the new scout_creation_available facade read uses it after the enrollment check, so a caller can decide whether to offer scout creation with the same rule the endpoint enforces. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
scout_creation_available takes a team id and a user id and resolves the models inside Signals, so no ORM instance crosses the facade. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ing scouts Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…offering scouts Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
0432a8e to
a477eda
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/signals/backend/facade/api.py-1193-1201 (1)
1193-1201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate availability on canonical-parent membership.
scout_creation_availablechecks project access on the requested child team, but the create endpoint checks membership oncanonical_team. For a user with child-team access and no canonical-parent membership, the facade can returnTrue, whileScoutCanonicalTeamAccessPermissionrejects the create request.Use the same
effective_membership_levelcheck as the endpoint.Suggested fix
+from posthog.user_permissions import UserPermissions + ... canonical_team = team.parent_team or team + if UserPermissions(user).team(canonical_team).effective_membership_level is None: + return False if not team_is_enrolled(canonical_team.id): return False
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 64c667d7-14d2-43a0-a947-77dd008a694b
📒 Files selected for processing (1)
products/signals/backend/facade/api.py
Included review availability: Your plan provides up to 12 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
CodeRabbit's review on the current head (findings at facade/api.py:1193-1201) flags that scout_creation_available checks project access on the requested team but skips the canonical-parent membership check the real create endpoint enforces, so the facade can report availability the endpoint would then reject — the earlier two rounds of CodeRabbit findings were fixed and confirmed by the bot, but this third, current-head finding on permission logic has no reply or follow-up commit addressing it.
- coderabbitai[bot] reviewed the current head.
- Unresolved current-head CodeRabbit finding: scout_creation_available omits the canonical-team membership check (effective_membership_level) that ScoutCanonicalTeamAccessPermission enforces on the actual create endpoint, so the facade can return True for a user the endpoint would reject — exactly the drift this PR is meant to prevent.
- This touches authorization logic for scout creation (auth-sensitive surface); author is not on the owning team (@PostHog/team-self-driving) and there is no approving/no-concerns review on the current head to serve as independent assurance.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 48L, 4F substantive, 100L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (100L, 6F, single-area, feat) |
| stamphog 2.1.0 | .stamphog/policy.yml @ a477eda · reviewed head a477eda |
|
This pull request was merged into |
2 participants
Problem
Changes
Nothing is user-visible. The endpoint keeps the same check.
can_create_scoutholds the create endpoint's rule: editor access to skills, because the skill body is the prompt the scout runs.can_create_scout.scout_creation_available(team_id, user_id)checks that the canonical project runs scouts (thesignals-scoutflag payload), then callscan_create_scout.How did you test this code?
test_facade_scout_creation_available.pycatches an offer to a project that does not run scouts, or to a user without skill editor access.Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Fable 5.1 wrote the original #101991. Claude Opus 5.5 split it into this stack.
🤖 Generated with Claude Code