feat(skills): add optional owners on skills for foolproof scout reviewer routing

[andrewm4894]

Problem

Reports that a signals scout emits about a scout skill (self-improvement especially) were routing suggested_reviewers to the wrong person. The model picked the reviewer, and its notion of "who owns this skill" came from reconstructing authorship out of the skill's version rows — creator-first, but degrading to the first surviving editor whenever the true creator was a system-authored v1 or had lost access. So editing a shared scout silently transferred reviewer ownership to whoever last touched it (traced from inbox report 019f8b1c-8a41-7119-bf68-bc740be4517d, which suggested an editor rather than the v1 author).

Root cause: ownership was derived from mutable, per-version created_by. Any prompt tweak or better reconstruction is still best-effort.

Changes

Adds a first-class, optional set of owner users on a skill — a general skills primitive that also fixes scout reviewer routing (scouts are skills).

• New LLMSkillOwner model, keyed on the logical skill (team, skill_name, user) — not a version row — so editing a body can never change ownership. Fail-closed (TeamScopedRootMixin); FKs to the hot posthog_team/posthog_user tables use db_constraint=False, so the migration is a plain CREATE TABLE with no lock on either parent.
• Owners on the API: surfaced (read) on skill-get / skill-list, writable on skill-create / skill-update (a list of project-member user UUIDs). The creator is seeded as owner by default on every create path (REST create, import, duplicate). Non-member UUIDs are rejected fail-loud. Owners are keyed on the logical skill, so a plain body edit never touches them; passing an empty list clears them. OpenAPI + MCP tool schemas regenerated.
• Reviewer wiring prefers owners: resolve_skill_authors returns the explicit owner set (role="owner", seed-creator first) when present, falling back to version-history reconstruction only when a skill has no owners; the prompt renders a "skill owners" line from it. The deterministic backstop is a guardrail in tools/report.py._build_suggested_reviewers — on emit it resolves the running scout's owners and places them first, ahead of the model's picks (deduped, capped, owners never dropped), so a report always routes to whoever owns the scout even if the model picks the wrong person or none at all. On edit, owners are only enforced when the caller is actually setting reviewers, so an unrelated edit can't clobber a human's chosen routing.

Rejected alternatives (documented in the tracking issue): an M2M on LLMSkill (attaches to a version row → same drift trap) and a metadata.owners JSON blob (no FK integrity, not queryable, stored on a version row → rots).

No frontend/UI in this PR — the Skills page owners editor is a fast follow-up.

How did you test this code?

Automated tests I (the agent) added and ran locally (all green):

• products/skills/backend/api/test/test_skills.py::TestLLMSkillOwners (6 cases) — creator is seeded on create; explicit owners on create don't add the creator; a non-member UUID is rejected (400); update replaces then clears owners; an update without owners leaves them untouched; owners survive a version publish by a different user (the core durability property — version-history reconstruction would return the editor here).
• products/signals/backend/test/test_scout_report_api.py::TestBuildSuggestedReviewers (4 new cases) — owners are enforced first and deduped against the model's picks; an owner is injected even when the scout supplies no reviewers; owners are not injected when the enforce flag is off (edit path); an owner with no linked GitHub identity is skipped rather than failing the whole resolution.
• products/signals/backend/test/test_scout_harness.py::TestSkillLoader::test_authors_prefer_explicit_owners_over_version_history — an explicit owner wins over the latest editor in author resolution.

Full suites re-run green: test_skills.py, test_scout_report_api.py, test_scout_harness.py (211 tests). Repo-wide uv run mypy --cache-fine-grained . clean; ruff clean; hogli ci:preflight 0 failures; migration risk analysis reports the new table Safe (0 blocked, no hot-table flag). Not tested manually / no UI exercised.

Automatic notifications

• Publish to changelog?
• Alert Sales and Marketing teams?

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Built with Claude Code (Opus 4.8) from the /phs scouts-work issue 06 spec (agreed design). Skills invoked while producing this PR: /django-migrations (model + migration safety — drove the db_constraint=False hot-table-FK choice and the fail-closed mixin), /improving-drf-endpoints (owners field typing + help_text so the OpenAPI/MCP/zod types generate cleanly), /writing-tests (value-gated the test set — each case names a distinct regression, no coverage padding).

Key decisions: keyed owners on the logical skill rather than a version row or a metadata blob (durability by construction); routed all owner reads/writes through the fail-closed manager's for_team() so the service works both inside and outside a request; made the reviewer guardrail a union (owners first + the model's valid picks) rather than a hard replace, so code-owner evidence the model surfaces isn't discarded while ownership is still guaranteed represented.

🤖 Generated with Claude Code

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[github-actions]

🦔 Hogbox preview · ✅ ready

▶ Open the preview

🔑 Login	test@posthog.com / 12345678 (demo data)
🧩 Running	this PR's backend and frontend, on the PostHog :master base
🔗 Link	stable across rebuilds — a re-push swaps the box underneath, the URL stays
🔒 Access	tailnet only (PostHog VPN)
🛠️ Admin	inspect & debug state in hogland
💤 Idle	sleeps after ~30 min idle (snapshot to S3, zero node cost) and wakes on your next visit in ~30s, behind a brief "waking up" screen

_{commit b048b69 · box box-ba948e9c1797 · ready in 866s (push → usable) · build log · rebuilds on every push, torn down on close}

[github-actions]

🤖 CI report

✅ Bundle size — 🟢 -2.6 KiB (-0.0%)

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 64.66 MiB · 🟢 -2.6 KiB (-0.0%)

File	Size	Δ vs base
posthog-app/_parent/products/replay_vision/frontend/replay_scanners/ReplayScanner.js	94.6 KiB	🟢 -2.6 KiB (-2.6%)

_{Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report}

✅ Eager graph — within budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root	Eager (shipped)	Δ vs base	Budget
entry (logged-out pages, app bootstrap) src/index.tsx	1.24 MiB · 22 files	no change	███░░░░░░░ 27.5% of 4.51 MiB
authenticated shell (every logged-in page) src/scenes/AuthenticatedShell.tsx	8.21 MiB · 3,000 files	no change	████████░░ 84.5% of 9.71 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx

Size	File
126.8 KiB	../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB	../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB	../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB	../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB	../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB	../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB	src/RootErrorBoundary.tsx
912 B	../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
789 B	src/scenes/ChunkLoadErrorBoundary.tsx
762 B	src/index.tsx

Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx

Size	File
281.3 KiB	../node_modules/.pnpm/posthog-js@1.406.2/node_modules/posthog-js/dist/rrweb.js
267.7 KiB	../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
236.0 KiB	src/taxonomy/core-filter-definitions-by-group.json
224.7 KiB	../node_modules/.pnpm/posthog-js@1.406.2/node_modules/posthog-js/dist/module.js
167.1 KiB	src/queries/validators.js
154.3 KiB	../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB	../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
105.8 KiB	src/lib/api.ts
94.0 KiB	../packages/quill/packages/quill/dist/index.js
93.3 KiB	../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js

_{Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479}

✅ Toolbar bundle — eager 2.18 MiB within budget

What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.

Metric	Size	Δ vs base	Budget
Eager (shipped) entry + static imports	2.18 MiB · 17 files	no change	████░░░░░░ 38.1% of 5.72 MiB
Deferred (lazy)	2.07 MiB · 33 files	no change	n/a — loads on demand
Loader dist/toolbar.js	1.1 KiB	no change	█░░░░░░░░░ 5.8% of 19.5 KiB

Largest eagerly-shipped chunks

Size	File
713.8 KiB	dist/toolbar/toolbar-app-HY7HJI4V.css
543.6 KiB	dist/toolbar/chunk-chunk-UG3THN3N.js
484.2 KiB	dist/toolbar/chunk-chunk-QS5AHYGW.js
133.6 KiB	dist/toolbar/chunk-chunk-MCXISDMN.js
131.8 KiB	dist/toolbar/chunk-chunk-T5KY5WYR.js
71.0 KiB	dist/toolbar/toolbar-app-UDDFB6JG.js
69.0 KiB	dist/toolbar/chunk-chunk-27JL52RE.js
35.6 KiB	dist/toolbar/chunk-chunk-XVKSNBZ7.js
20.9 KiB	dist/toolbar/chunk-chunk-CS7W2KTV.js
12.2 KiB	dist/toolbar/chunk-chunk-PIK3PADE.js

_{Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile}

✅ Dist folder size — 🟢 -9.0 KiB (-0.0%)

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 1357.17 MiB · 🟢 -9.0 KiB (-0.0%)

ℹ️ MCP UI apps size — 32 app(s), 17064.9 KB JS

Built size of each MCP UI app (main.js + styles.css).

App	JS	CSS
debug	599.5 KB	187.7 KB
action	457.8 KB	187.7 KB
action-list	564.3 KB	187.7 KB
cohort	456.8 KB	187.7 KB
cohort-list	563.3 KB	187.7 KB
email-template	456.6 KB	187.7 KB
error-details	472.4 KB	187.7 KB
error-issue	457.5 KB	187.7 KB
error-issue-list	564.2 KB	187.7 KB
experiment	561.5 KB	187.7 KB
experiment-list	565.1 KB	187.7 KB
experiment-results	563.2 KB	187.7 KB
feature-flag	567.1 KB	187.7 KB
feature-flag-list	570.9 KB	187.7 KB
feature-flag-testing	461.0 KB	187.7 KB
insight-actors	562.1 KB	187.7 KB
invite-email-preview	456.0 KB	187.7 KB
llm-costs	559.5 KB	187.7 KB
session-recording	458.6 KB	187.7 KB
session-summary	463.9 KB	187.7 KB
survey	458.4 KB	187.7 KB
survey-global-stats	562.2 KB	187.7 KB
survey-list	565.0 KB	187.7 KB
survey-stats	562.2 KB	187.7 KB
trace-span	457.2 KB	187.7 KB
trace-span-list	564.2 KB	187.7 KB
workflow	457.1 KB	187.7 KB
workflow-list	563.7 KB	187.7 KB
loops-review	460.6 KB	187.7 KB
query-results	745.5 KB	187.7 KB
render-ui	826.2 KB	187.7 KB
visual-review-snapshots	461.6 KB	187.7 KB

⚠️ Backend coverage — 96.0% of changed backend lines covered — 12 uncovered

🧪 Backend test coverage

Patch coverage — changed backend lines (products + core): ███████████████████░ 96.0% (306 / 318)

File	Patch	Uncovered changed lines
products/signals/backend/scout_harness/prompt.py	50.0%	455–456
products/skills/backend/api/skills.py	77.3%	396–397, 1112–1114
products/skills/backend/api/skill_services.py	92.2%	657, 690, 699, 728
products/signals/backend/scout_harness/tools/report.py	97.9%	333

🤖 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 29961455358 -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
batch_exports	████████░░░░░░░░░░░░ 39.6%	8,414 / 21,250
demo	███████████░░░░░░░░░ 56.2%	1,497 / 2,663
warehouse_sources_queue	████████████░░░░░░░░ 59.2%	148 / 250
tasks	██████████████░░░░░░ 69.1%	29,366 / 42,488
data_tools	██████████████░░░░░░ 70.0%	63 / 90
ai_gateway	███████████████░░░░░ 75.0%	9 / 12
signals	████████████████░░░░ 79.1%	20,479 / 25,883
cdp	████████████████░░░░ 80.7%	3,117 / 3,863
data_modeling	█████████████████░░░ 82.8%	5,529 / 6,675
wizard	█████████████████░░░ 84.8%	935 / 1,102
notebooks	█████████████████░░░ 85.3%	7,266 / 8,522
agent_platform	█████████████████░░░ 86.4%	3,807 / 4,405
actions	█████████████████░░░ 86.6%	717 / 828
cohorts	██████████████████░░ 87.8%	4,488 / 5,114
product_tours	██████████████████░░ 87.9%	1,303 / 1,482
exports	██████████████████░░ 88.4%	6,949 / 7,861
data_warehouse	██████████████████░░ 88.8%	11,867 / 13,358
business_knowledge	██████████████████░░ 89.0%	4,391 / 4,936
engineering_analytics	██████████████████░░ 89.2%	5,641 / 6,323
conversations	██████████████████░░ 89.3%	16,820 / 18,834
dashboards	██████████████████░░ 89.4%	5,983 / 6,693
visual_review	██████████████████░░ 89.4%	5,818 / 6,505
error_tracking	██████████████████░░ 89.7%	10,174 / 11,342
alerts	██████████████████░░ 89.9%	4,054 / 4,508
early_access_features	██████████████████░░ 90.1%	1,031 / 1,144
mcp_analytics	██████████████████░░ 90.1%	2,763 / 3,065
streamlit_apps	██████████████████░░ 90.4%	2,501 / 2,767
links	██████████████████░░ 90.6%	183 / 202
slack_app	██████████████████░░ 90.7%	9,025 / 9,948
marketing_analytics	██████████████████░░ 91.0%	11,792 / 12,964
stamphog	██████████████████░░ 91.1%	4,056 / 4,450
product_analytics	██████████████████░░ 91.3%	5,811 / 6,362
mcp_store	██████████████████░░ 91.9%	4,257 / 4,634
notifications	███████████████████░ 92.6%	1,011 / 1,092
ai_observability	███████████████████░ 92.7%	14,944 / 16,117
surveys	███████████████████░ 93.0%	5,730 / 6,163
web_analytics	███████████████████░ 93.0%	14,147 / 15,206
managed_migrations	███████████████████░ 93.1%	1,424 / 1,530
posthog_ai	███████████████████░ 93.2%	1,326 / 1,422
approvals	███████████████████░ 93.3%	3,437 / 3,682
reminders	███████████████████░ 93.4%	468 / 501
workflows	███████████████████░ 93.5%	6,337 / 6,775
legal_documents	███████████████████░ 94.1%	1,568 / 1,667
endpoints	███████████████████░ 94.1%	8,640 / 9,177
messaging	███████████████████░ 94.3%	2,682 / 2,845
revenue_analytics	███████████████████░ 94.5%	3,598 / 3,809
tracing	███████████████████░ 94.5%	2,670 / 2,826
skills	███████████████████░ 94.6%	3,059 / 3,234
review_hog	███████████████████░ 94.6%	6,808 / 7,193
logs	███████████████████░ 95.4%	9,935 / 10,416
experiments	███████████████████░ 95.7%	24,882 / 25,996
growth	███████████████████░ 96.1%	3,245 / 3,376
replay_vision	███████████████████░ 96.2%	14,973 / 15,572
annotations	███████████████████░ 96.2%	732 / 761
feature_flags	███████████████████░ 96.5%	17,110 / 17,734
user_interviews	███████████████████░ 96.5%	2,638 / 2,734
access_control	███████████████████░ 96.8%	851 / 879
warehouse_sources	███████████████████░ 97.1%	327,769 / 337,448
customer_analytics	███████████████████░ 97.3%	8,380 / 8,610
data_catalog	███████████████████░ 97.4%	2,367 / 2,429
analytics_platform	████████████████████ 98.0%	2,153 / 2,197
metrics	████████████████████ 98.2%	2,491 / 2,536
pulse	████████████████████ 98.4%	2,017 / 2,049
live_debugger	████████████████████ 99.2%	613 / 618
field_notes	████████████████████ 99.4%	158 / 159

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.

⚠️ MCP snapshots — 4 updated (4 modified, 0 added, 0 deleted)

Snapshots: MCP unit test snapshots updated

Changes: 4 snapshots (4 modified, 0 added, 0 deleted)

What this means:

• Snapshots have been automatically updated to match current output

Next steps:

• Review the changes to ensure they're intentional
• If unexpected, investigate what caused the output to change

Review snapshot changes →

⚠️ Django migration SQL — 1 new migration to review

We've detected new migrations on this PR. Review the SQL output for each migration:

products/skills/backend/migrations/0004_llmskillowner.py

BEGIN;
--
-- Create model LLMSkillOwner
--
CREATE TABLE "llm_analytics_llmskillowner" ("id" uuid NOT NULL PRIMARY KEY, "skill_name" varchar(64) NOT NULL, "created_at" timestamp with time zone NOT NULL, "team_id" integer NOT NULL, "user_id" integer NOT NULL, CONSTRAINT "unique_llm_skill_owner" UNIQUE ("team_id", "skill_name", "user_id"));
CREATE INDEX "llm_analytics_llmskillowner_team_id_8c99ec65" ON "llm_analytics_llmskillowner" ("team_id");
CREATE INDEX "llm_analytics_llmskillowner_user_id_50a0e86e" ON "llm_analytics_llmskillowner" ("user_id");
COMMIT;

Last updated: 2026-07-22 22:06 UTC (b048b69)

✅ Django migration risk — migration analysis complete

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

skills.0004_llmskillowner
  └─ #1 ✅ CreateModel
     Creating new table is safe
     model: LLMSkillOwner
  │
  └──> ℹ️  INFO:
       ℹ️  Skipped operations on newly created tables (empty tables
       don't cause lock contention).

Last updated: 2026-07-22 22:06 UTC (b048b69)

[greptile-apps]

Security Review

Archived owner rows can attach to a later skill that reuses the same name. This can expose stale owner identities and route reports to users unrelated to the new skill.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
products/skills/backend/api/skill_serializers.py:612-623
**Empty Owner List Seeds Creator**

When create receives `owners: []`, this truthiness check treats it like an omitted field and seeds the creator. The API therefore silently ignores an explicit empty owner set, unlike update where an empty list clears ownership.

```suggestion
            if owner_uuids is not None:
                try:
                    owner_users = resolve_owner_users(team, [str(u) for u in owner_uuids])
                except LLMSkillOwnerNotFoundError as err:
                    raise serializers.ValidationError(
                        {"owners": f"User '{err.user_uuid}' is not a member of this project."},
                        code="invalid_owner",
                    )
                set_skill_owners(team, skill.name, owner_users)
            else:
                # Creator owns by default — durable, not reconstructed from version history.
                seed_skill_owner(team, skill.name, request.user)
```

### Issue 2 of 3
products/skills/backend/api/skill_services.py:386
**Archived Owners Attach To Recreated Skill**

When an archived skill name is reused, its owner rows still exist because archive does not remove them. This `get_or_create` only adds the new creator, so the new skill also inherits the previous skill's owners and can expose or route reports to unrelated users.

### Issue 3 of 3
products/skills/backend/api/skills.py:452-453
**Publish Commits Before Owner Replacement**

The version publish commits before this separate owner transaction starts. If owner replacement fails, the request returns an error after the new body or files are already live, while ownership remains unchanged; retrying with the original `base_version` then conflicts. Both writes need one transaction so the update cannot partially succeed.

_{Reviews (1): Last reviewed commit: "feat(skills): add optional owners on ski..." | Re-trigger Greptile}

[posthog]

🦔 ReviewHog reviewed this pull request

Found 1 must fix, 1 should fix, 0 consider.

Published 2 findings (view the review).

[veria-ai]

PR overview

This pull request adds optional owner metadata to skills to support more reliable scout reviewer routing. The backend skill service changes include reading and serializing skill owner information for API consumers.

There is one low-severity issue still open: skill readers may be able to see profile details for owners who no longer have project or organization access if stale owner rows remain. One issue has already been addressed, so the review is trending down, but the remaining gap should be closed by filtering returned owners to users who still have access.

Open issues (1)

• Low: Revoked owner profiles remain readable — products/skills/backend/api/skill_services.py:657

Fixed/addressed: 1 · PR risk: 4/10

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 530a7bfd79

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[hex-security-app]

Yes, that fix direction addresses the escalation. Make the provenance a persisted field on SuggestedReviewerEntry (rather than deriving it from ordering or reason), set it only in _owner_reviewer_entries, and preserve it through every reviewer parse/merge/enrichment path. _resolve_autostart_assignee should ignore entries with that owner provenance while keeping them in the artefact for inbox routing.

The regression should cover an owner listed first plus a later normal reviewer: autostart must select only the normal reviewer, never the owner. Also cover an owner-only list so it cannot mint a token as the owner (whether the existing safe fallback is used or autostart is skipped).

[andrewm4894]

Pushed fixes for the review findings in a210c825fa4. Mapping to the comments above:

Security

• Owners as autostart identities (hex) — added an is_skill_owner provenance flag to the suggested_reviewers artefact; _resolve_autostart_assignee now excludes owner-provenance entries from task-identity selection (they still route the report, they just can't be the runner). An owner-only list resolves to nobody → the existing signals-enabler fallback applies. Regression test: test_resolve_autostart_assignee_never_runs_as_a_skill_owner.
• Cross-environment ownership collision (veria) — LLMSkillOwner is now scoped to the exact environment team (not TeamScopedRootMixin's canonical parent), matching LLMSkill; reads/writes go through for_team(..., canonical=True). Kept fail-closed via TeamScopedManager and added to the IDOR semgrep rule (a pre-existing coverage gap). Regression test: test_owners_are_scoped_to_the_exact_environment.

Correctness / privacy

• Archived owners attach to recreated skill (greptile / codex) — archive_skill now clears owner rows. Test: test_recreated_skill_does_not_inherit_archived_owners.
• Revoked owner profiles readable (veria / codex) — read resolvers filter to team.all_users_with_access(). Test: test_skill_get_excludes_owner_who_lost_access.
• Fallback when owners lose access (codex) — an owned-but-unroutable skill returns no authors instead of drifting to the version-history heuristic. Test: test_owned_skill_with_all_owners_revoked_returns_no_authors.
• Empty owner list seeds creator (greptile) — create now uses is not None, so owners: [] creates with no owners. Test: test_create_with_empty_owners_creates_with_no_owners.
• Publish before owner replacement (greptile) — publish + owner replace now share one transaction.

API contract

• Create response schema (codex) — pinned to LLMSkillSerializer (responses={201: ...}); regenerated types now return owners as UserBasic[].
• Owner list cap (codex) — max_length=25 on both owners fields.

Repo-wide mypy clean, ruff clean, IDOR coverage passes, OpenAPI + MCP snapshots regenerated. New/updated suites green.

[posthog]

ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

[posthog]

ReviewHog Report

Feature

Issues: 1 issue

Files (14)

• products/skills/backend/models/skills.py
• products/skills/backend/models/__init__.py
• products/skills/backend/migrations/0004_llmskillowner.py
• products/skills/backend/api/skill_services.py
• products/skills/backend/api/skill_serializers.py
• products/skills/backend/api/skills.py
• products/skills/mcp/tools.yaml
• products/skills/frontend/generated/api.schemas.ts
• products/skills/frontend/generated/api.zod.ts
• services/mcp/schema/generated-tool-definitions.json
• services/mcp/schema/tool-definitions-all.json
• services/mcp/src/api/generated.ts
• services/mcp/src/generated/skills/api.ts
• services/mcp/src/tools/generated/skills.ts

What were the main changes

• New LLMSkillOwner model keyed on the logical skill (team, skill_name, user) — not a version row — so a body edit can never change ownership; fail-closed via TeamScopedRootMixin, FKs to hot posthog_team/posthog_user use db_constraint=False so the migration is a plain CREATE TABLE with no parent lock
• Migration 0004_llmskillowner adds the table with a unique (team, skill_name, user) constraint
• New service functions in skill_services.py: resolve_owner_users (fail-loud on non-member UUIDs), seed_skill_owner, set_skill_owners (replace/clear), resolve_skill_owners/_for_names (batched for list views)
• Owners exposed as read-only on skill-get/skill-list (SerializerMethodField, N+1-safe via list context) and writable on skill-create/skill-update as a list of project-member UUIDs; creator seeded as owner by default on create
• skills.py viewset validates/resolves owner UUIDs before publish and applies them after the version publish only when 'owners' was passed, so a plain body edit never touches ownership
• Review findings flagged: empty owners:[] on create is silently treated as omitted and still seeds the creator (should clear) [greptile P1]; archived skill name reuse via get_or_create inherits the previous skill's stale owner rows [greptile P1/security]; owner replacement runs in a separate transaction from the version publish, risking partial success on failure [greptile P1]
• Regenerated OpenAPI TS types and MCP tool schemas/zod/generated clients to surface the new 'owners' field on skill-create/skill-update

Bugfix

Issues: 1 issue

Files (4)

• products/signals/backend/scout_harness/skill_loader.py
• products/signals/backend/scout_harness/prompt.py
• products/signals/backend/scout_harness/tools/report.py
• products/signals/backend/scout_harness/AGENTS.md

What were the main changes

• skill_loader.py: new resolve_skill_owner_user_uuids and _resolve_owner_authors; resolve_skill_authors now prefers the skill's explicit LLMSkillOwner set (role="owner", seed-creator first) and only falls back to reconstructing authorship from mutable version-row history when no explicit owners exist
• prompt.py: _skill_authors_line renders a 'skill owners' guidance line when explicit owners are present, else falls back to the legacy creator/editor line
• tools/report.py: _build_suggested_reviewers reworked to take (team, reviewers, skill_name, enforce_owners); resolves the running scout's owners and places them first ahead of the model's picks (deduped by GitHub login, capped, owners never dropped) — the deterministic backstop for reviewer routing
• Owner enforcement wired in on the emit path (always) and the edit path (only when the caller is actually setting reviewers, so an unrelated edit can't clobber a human's chosen routing)
• AGENTS.md updated to document the owner-first routing design and the fallback to version-history reconstruction
• Flagged by automated security review: owners are writable by any skill editor yet become trusted suggested_reviewers used to select the auto-start task identity, letting a skill editor mint an OAuth session as an arbitrary higher-privileged owner — not addressed in this PR

[posthog]

Owner-as-suggested-reviewer guardrail also fires from edit_report, which accepts an arbitrary report_id — widening the already-flagged autostart-identity escalation beyond self-authored reports

Why we think it's a valid issue

• Checked: the full edit path at the PR head (fetched report.py@530a7bf), the owner-guardrail wiring, the service scoping in scout_report/persistence.py, and the autostart identity resolution in auto_start.py.
• Found (arbitrary report_id): _validate_edit_inputs (report.py:1044-1049) only runs _assert_team_owns_run(team, run) — it validates the run belongs to the team, never that report_id is attributable to this run/skill. update_scout_report (persistence.py:269) and set_scout_report_reviewers (persistence.py:397) are team-scoped fail-closed only, and their docstrings confirm edit_report "can target ANY inbox report (pipeline-authored included)" as an intentional decision. So a run of skill S can mutate any team report.
• Found (owner injection + autostart re-fire): _do_edit_report (report.py:964-966) passes enforce_owners=bool(suggested_reviewers) + skill_name=run.skill_name, so _owner_reviewer_entries force-prepends the running scout's LLMSkillOwner set on any edit that sets reviewers; reviewers_set then re-fires _maybe_autostart_report (report.py:1001-1002).
• Found (identity escalation confirmed): _maybe_autostart_report (report.py:430-440) calls maybe_autostart_from_report_artefacts with no triggering_user_id, so auto_start.py:500 takes the _resolve_autostart_assignee(reviewers_content) branch (not _resolve_triggering_user), returning the first reviewer — owners lead the list — and mints an OAuth token as that user. That function's own docstring (auto_start.py:292-295) states it is "Only used for trusted (pipeline / custom-agent / scout) reviewer lists, which are derived from commit authorship rather than user input" — precisely the assumption the writable-owner set breaks.
• Impact: A skill editor who names a higher-privileged teammate as owner of skill S can, without S authoring any new self-report, have any run of S that edits an existing inbox report with reviewer input silently prepend that owner and re-trigger autostart on an already-triaged report — minting a full-scope OAuth implementation task under the named owner's identity. This is a distinct attack surface (arbitrary pre-existing reports) beyond the already-flagged emit-path parent, and it directly contradicts the PR's stated safety property that edits "can't clobber a human's chosen routing" (that holds only when the edit sets no reviewers). Verified auth/permission-gap + privilege-escalation, worth surfacing as a fix-completeness requirement on the parent security finding.

Issue description

This builds on the existing high-severity hex-security-app finding on this file (anchored at _owner_reviewer_entries, line 254) about LLMSkillOwner rows — writable by any skill editor, naming any project member — being injected as trusted suggested_reviewers that auto_start.py._resolve_autostart_assignee uses to pick who an autonomous implementation task mints a full-scope OAuth session as. That comment reads as scoped to reports the scout emits about itself. Reading _do_edit_report (lines 887-946) shows the blast radius is broader: edit_report takes a caller-supplied report_id with no check that it belongs to the current run or even the current skill (update_scout_report(team_id=team.id, report_id=report_id, ...) / set_scout_report_reviewers(team_id=team.id, report_id=report_id, ...) operate on whatever report_id was passed). The new wiring at lines 960-966 enforces the same owner-guardrail (enforce_owners=bool(suggested_reviewers)) whenever this edit sets any reviewer, and a reviewers_set edit re-triggers _maybe_autostart_report (line 946). So a member who can edit skill S — and who names a higher-privileged teammate as S's owner — doesn't need S to author a new report at all: any run of S that calls edit_report on any existing report in the team's inbox with any reviewer input will forcibly prepend S's owner into that unrelated report's suggested_reviewers and re-run autostart, potentially causing an already-triaged report to spin up an autonomous PR run under the named owner's identity. resolve_reviewers.py's own docstring for _resolve_autostart_assignee states this path is "only used for trusted (pipeline / custom-agent / scout) reviewer lists, which are derived from commit authorship rather than user input" — that trust assumption is what this PR breaks, and the edit path shows it isn't limited to the emitting scout's own reports.

Suggested fix

In addition to whatever fix lands for the emit-path finding, don't let edit_report inject a skill's owners (or re-run autostart) onto a report that isn't actually attributable to the running skill — e.g. only enforce owners / re-trigger autostart when report_id is already tracked against this run or skill (SignalScoutRun.emitted_report_ids / edited_report_ids), or decouple 'may be named a suggested reviewer' from 'may be minted an OAuth session by autostart' entirely so a skill's self-declared owner set can never by itself select the autostart identity.

Prompt to fix with AI (copy-paste)

## Context
@products/signals/backend/scout_harness/tools/report.py#L887-946
@products/signals/backend/scout_harness/tools/report.py#L957-966

<issue_description>
This builds on the existing high-severity hex-security-app finding on this file (anchored at `_owner_reviewer_entries`, line 254) about `LLMSkillOwner` rows — writable by any skill editor, naming any project member — being injected as trusted `suggested_reviewers` that `auto_start.py._resolve_autostart_assignee` uses to pick who an autonomous implementation task mints a full-scope OAuth session as. That comment reads as scoped to reports the scout emits about itself. Reading `_do_edit_report` (lines 887-946) shows the blast radius is broader: `edit_report` takes a caller-supplied `report_id` with no check that it belongs to the current run or even the current skill (`update_scout_report(team_id=team.id, report_id=report_id, ...)` / `set_scout_report_reviewers(team_id=team.id, report_id=report_id, ...)` operate on whatever `report_id` was passed). The new wiring at lines 960-966 enforces the same owner-guardrail (`enforce_owners=bool(suggested_reviewers)`) whenever this edit sets any reviewer, and a `reviewers_set` edit re-triggers `_maybe_autostart_report` (line 946). So a member who can edit skill S — and who names a higher-privileged teammate as S's owner — doesn't need S to author a new report at all: any run of S that calls `edit_report` on *any* existing report in the team's inbox with any reviewer input will forcibly prepend S's owner into that unrelated report's `suggested_reviewers` and re-run autostart, potentially causing an already-triaged report to spin up an autonomous PR run under the named owner's identity. `resolve_reviewers.py`'s own docstring for `_resolve_autostart_assignee` states this path is "only used for trusted (pipeline / custom-agent / scout) reviewer lists, which are derived from commit authorship rather than user input" — that trust assumption is what this PR breaks, and the edit path shows it isn't limited to the emitting scout's own reports.
</issue_description>

<issue_validation>
- **Checked:** the full edit path at the PR head (fetched `report.py`@530a7bf), the owner-guardrail wiring, the service scoping in `scout_report/persistence.py`, and the autostart identity resolution in `auto_start.py`.
- **Found (arbitrary report_id):** `_validate_edit_inputs` (report.py:1044-1049) only runs `_assert_team_owns_run(team, run)` — it validates the *run* belongs to the team, never that `report_id` is attributable to this run/skill. `update_scout_report` (persistence.py:269) and `set_scout_report_reviewers` (persistence.py:397) are team-scoped fail-closed only, and their docstrings confirm `edit_report` "can target ANY inbox report (pipeline-authored included)" as an intentional decision. So a run of skill S can mutate any team report.
- **Found (owner injection + autostart re-fire):** `_do_edit_report` (report.py:964-966) passes `enforce_owners=bool(suggested_reviewers)` + `skill_name=run.skill_name`, so `_owner_reviewer_entries` force-prepends the *running scout's* `LLMSkillOwner` set on any edit that sets reviewers; `reviewers_set` then re-fires `_maybe_autostart_report` (report.py:1001-1002).
- **Found (identity escalation confirmed):** `_maybe_autostart_report` (report.py:430-440) calls `maybe_autostart_from_report_artefacts` with no `triggering_user_id`, so `auto_start.py:500` takes the `_resolve_autostart_assignee(reviewers_content)` branch (not `_resolve_triggering_user`), returning the first reviewer — owners lead the list — and mints an OAuth token as that user. That function's own docstring (auto_start.py:292-295) states it is "Only used for trusted (pipeline / custom-agent / scout) reviewer lists, which are derived from commit authorship rather than user input" — precisely the assumption the writable-owner set breaks.
- **Impact:** A skill editor who names a higher-privileged teammate as owner of skill S can, without S authoring any new self-report, have any run of S that edits an existing inbox report with reviewer input silently prepend that owner and re-trigger autostart on an already-triaged report — minting a full-scope OAuth implementation task under the named owner's identity. This is a distinct attack surface (arbitrary pre-existing reports) beyond the already-flagged emit-path parent, and it directly contradicts the PR's stated safety property that edits "can't clobber a human's chosen routing" (that holds only when the edit sets no reviewers). Verified auth/permission-gap + privilege-escalation, worth surfacing as a fix-completeness requirement on the parent security finding.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
In addition to whatever fix lands for the emit-path finding, don't let `edit_report` inject a skill's owners (or re-run autostart) onto a report that isn't actually attributable to the running skill — e.g. only enforce owners / re-trigger autostart when `report_id` is already tracked against this run or skill (`SignalScoutRun.emitted_report_ids` / `edited_report_ids`), or decouple 'may be named a suggested reviewer' from 'may be minted an OAuth session by autostart' entirely so a skill's self-declared owner set can never by itself select the autostart identity.
</potential_solution>

[posthog]

duplicate_skill() reusing an archived skill name inherits stale owners too

Why we think it's a valid issue

• Checked: archive_skill, duplicate_skill, create_skill, and seed_skill_owner on the PR head, plus the LLMSkill uniqueness constraints and the LLMSkillOwner model keying.
• Found: archive_skill (skill_services.py:603-621) sets deleted=True on LLMSkill rows only; it never deletes LLMSkillOwner rows. LLMSkillOwner is keyed on the string (team, skill_name) with no FK to LLMSkill (models/skills.py), so archiving fires no cascade and the owner rows persist.
• Found: Name reuse after archive is a supported, documented flow — the LLMSkill unique constraints are conditioned on deleted=False, and the version-limit error tells users to 'Archive and recreate the skill' (skills.py:419). duplicate_skill's only collision guard is filter(team=team, name=new_name, deleted=False).exists(), so new_name can be an archived name; its seed_skill_owner(team, new_name, user) is a get_or_create that adds only the duplicating user and leaves the archived skill's owners attached. create_skill (skill_services.py:387) is identical.
• Impact: A brand-new, unrelated skill created or duplicated under a reused (archived) name silently inherits the archived skill's owner set. Because owners drive reviewer routing ('who should review reports about this scout'), reports on the new skill route to people who owned a different, archived skill — the exact misattribution this feature exists to prevent. Concrete trigger + concrete consequence on a realistic path; clears the correctness/data-integrity bar. The suggested root-cause fix in archive_skill correctly covers all creation paths.

Issue description

This is the same root-cause bug already flagged on create_skill (line 386-388: reusing an archived skill's name inherits that name's pre-existing LLMSkillOwner rows because archive_skill never removes them), but it also reproduces here via a different call path. duplicate_skill's only existence check is LLMSkill.objects.filter(team=team, name=new_name, deleted=False).exists() (active skills only), so new_name can silently be an archived skill's name. seed_skill_owner(team, new_name, user) then only adds the duplicating user via get_or_create — any owners left over from the archived skill under that same (team, skill_name) remain attached to the new, unrelated duplicate.

Suggested fix

The real fix belongs in archive_skill (products/skills/backend/api/skill_services.py, unchanged in this PR): delete LLMSkillOwner rows for (team, skill_name) when a skill is archived, so no creation path (create_skill, duplicate_skill, or any future one) has to individually guard against inheriting stale owners from a reused name.

Prompt to fix with AI (copy-paste)

## Context
@products/skills/backend/api/skill_services.py#L458-461

<issue_description>
This is the same root-cause bug already flagged on `create_skill` (line 386-388: reusing an archived skill's name inherits that name's pre-existing `LLMSkillOwner` rows because `archive_skill` never removes them), but it also reproduces here via a different call path. `duplicate_skill`'s only existence check is `LLMSkill.objects.filter(team=team, name=new_name, deleted=False).exists()` (active skills only), so `new_name` can silently be an archived skill's name. `seed_skill_owner(team, new_name, user)` then only adds the duplicating user via `get_or_create` — any owners left over from the archived skill under that same `(team, skill_name)` remain attached to the new, unrelated duplicate.
</issue_description>

<issue_validation>
- **Checked:** `archive_skill`, `duplicate_skill`, `create_skill`, and `seed_skill_owner` on the PR head, plus the `LLMSkill` uniqueness constraints and the `LLMSkillOwner` model keying.
- **Found:** `archive_skill` (`skill_services.py:603-621`) sets `deleted=True` on `LLMSkill` rows only; it never deletes `LLMSkillOwner` rows. `LLMSkillOwner` is keyed on the string `(team, skill_name)` with no FK to `LLMSkill` (`models/skills.py`), so archiving fires no cascade and the owner rows persist.
- **Found:** Name reuse after archive is a supported, documented flow — the `LLMSkill` unique constraints are conditioned on `deleted=False`, and the version-limit error tells users to 'Archive and recreate the skill' (`skills.py:419`). `duplicate_skill`'s only collision guard is `filter(team=team, name=new_name, deleted=False).exists()`, so `new_name` can be an archived name; its `seed_skill_owner(team, new_name, user)` is a `get_or_create` that adds only the duplicating user and leaves the archived skill's owners attached. `create_skill` (`skill_services.py:387`) is identical.
- **Impact:** A brand-new, unrelated skill created or duplicated under a reused (archived) name silently inherits the archived skill's owner set. Because owners drive reviewer routing ('who should review reports about this scout'), reports on the new skill route to people who owned a different, archived skill — the exact misattribution this feature exists to prevent. Concrete trigger + concrete consequence on a realistic path; clears the correctness/data-integrity bar. The suggested root-cause fix in `archive_skill` correctly covers all creation paths.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
The real fix belongs in `archive_skill` (products/skills/backend/api/skill_services.py, unchanged in this PR): delete `LLMSkillOwner` rows for `(team, skill_name)` when a skill is archived, so no creation path (`create_skill`, `duplicate_skill`, or any future one) has to individually guard against inheriting stale owners from a reused name.
</potential_solution>

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a210c825fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid letting owner injection signal PR intent

When a report-channel scout emits an immediately-actionable report with a priority, no explicit repository, and no scout-supplied reviewers, this new enforce_owners=True call can still produce reviewer entries from skill owners who have linked GitHub accounts. _wants_repo_selection() then treats priority + reviewers as PR intent, runs free-form repo selection, and autostart later filters those owner entries out before falling back to the team's default runner, so a routing-only guardrail can unexpectedly start a draft implementation task even though the scout did not provide a repo or code-derived reviewer.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid publishing versions for owner-only updates

When a caller sends an owner-only PATCH, for example {"owners": [...], "base_version": 7}, the serializer accepts it but this path still publishes a full new skill version before replacing LLMSkillOwner rows. That creates an identical LLMSkill version, changes version-history authorship and marketplace/update timestamps, and can fail at MAX_SKILL_VERSION, even though owners are stored independently and do not need a content/version publish.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve explicit empty owner sets

When a caller passes owners: [] to intentionally clear a skill's owners, this deletes every LLMSkillOwner row and leaves no marker that ownership was explicitly configured as empty. In the scout prompt path, resolve_skill_authors() then sees no owner rows and falls back to version-history authors, so custom scouts whose owners were cleared can still route self-improvement reports to historical editors even though the API reports owners: []; keep an explicit empty state or make the fallback distinguish never configured from cleared.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid carrying stale owner provenance

When a skill owner is removed and a later scout edit re-adds the same GitHub login as a normal reviewer, this OR preserves the previous is_skill_owner=true even though _build_suggested_reviewers() no longer marked that login as a current owner. Autostart then filters the reviewer out of identity selection and can run under the team fallback instead of the newly supplied reviewer; preserve evidence fields from the prior artefact, but let the new entry's owner provenance replace the old value.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Backfill owners for existing skills

When this migration is applied to an environment that already has active LLMSkill rows, it creates an empty owner table and does not seed any LLMSkillOwner rows. Those existing custom scouts will keep returning owners: [], and the owner-aware prompt/guardrail paths fall back to version-history routing until someone manually edits ownership, so the production skills that motivated this change are not protected by the new default owner behavior; add a data migration or lazy backfill for active skills with known authors.

Useful? React with 👍 / 👎.