Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const config: SignalScoutConfigApi = {
description: 'General scout',
scout_origin: 'canonical',
enabled: true,
status: 'active',
pause_reason: null,
emit: true,
run_interval_minutes: 1440,
run_cron_schedule: '0 9 * * *',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ const CREATED_SCOUT: SignalScoutCreateResponseApi = {
description: 'Investigates recurring checkout failures.',
scout_origin: 'custom',
enabled: false,
status: 'paused_by_user',
pause_reason: null,
emit: false,
run_interval_minutes: 60,
run_cron_schedule: null,
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/scenes/inbox/logics/scoutFleetLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const BASE_CONFIG: SignalScoutConfigApi = {
description: 'Finds error trends.',
scout_origin: 'canonical',
enabled: true,
status: 'active',
pause_reason: null,
emit: true,
run_interval_minutes: 1440,
run_cron_schedule: null,
Expand Down
5 changes: 5 additions & 0 deletions posthog/models/activity_logging/activity_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ class Meta:
"SignalScoutConfig": {
"run_interval_minutes": "run interval (minutes)",
"emit": "emit findings",
"pause_reason": "pause reason",
},
"OAuthApplication": {
"_provisioning_config": "provisioning config",
Expand Down Expand Up @@ -759,6 +760,10 @@ class Meta:
# Run bookkeeping, not user intent — keep it out of change detection even when it
# rides along with a real change (belt-and-suspenders with signal_exclusions above).
"last_run_at",
# Companion bookkeeping that rides along with every logged `status` change; the
# activity log entry itself already carries who and when.
"status_changed_at",
"status_changed_by",
# Reverse relations auto-managed by FK creates, not user-initiated config changes.
"runs",
],
Expand Down
2 changes: 2 additions & 0 deletions posthog/settings/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,8 @@ def static_varies_origin(headers, path, url):
# path (drf-spectacular generates the x-spec-enum-id from the same tuples).
# --- Model class paths (ChoiceField x-spec-enum-id hashes) ---
"SignalReportRefundReasonEnum": "products.signals.backend.models.SignalReportRefund.Reason",
"ScoutConfigStatusEnum": "products.signals.backend.models.SignalScoutConfig.Status",
"ScoutConfigPauseReasonEnum": "products.signals.backend.models.SignalScoutConfig.PauseReason",
"EngineeringAnalyticsPRStateEnum": "products.engineering_analytics.backend.facade.contracts.PRState",
"QuarantineModeEnum": "products.engineering_analytics.backend.facade.contracts.QuarantineMode",
"CITestRunnerEnum": "products.engineering_analytics.backend.facade.contracts.CITestRunner",
Expand Down
6 changes: 5 additions & 1 deletion products/signals/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,11 @@ Per-scout binding for the headless **Signals agent**: one row per `(team, skill_
| ---------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `team` | FK → Team | Owning team (`related_name="signal_scout_configs"`). `unique_together(team, skill_name)`. |
| `skill_name` | CharField | The `signals-scout-*` skill this row controls. Auto-registered by the coordinator when it finds the skill on a participating team. |
| `enabled` | Boolean | Per-scout switch; defaults `True`. `False` pauses just this scout. |
| `enabled` | Boolean | Per-scout switch; defaults `True`. Derived from `status` (`enabled = status in (active, pending_pause)`) but kept as a real column for SQL filtering and the warehouse mirror; `save()` reconciles the pair for writers that set only one side. |
| `status` | Char (choices) | Lifecycle source of truth: `active` / `pending_pause` / `paused_by_system` / `paused_by_user`. Who paused is the state itself: the system may resume its own pauses but must never touch a human's. `pending_pause` still runs (a warning, not a pause) and is cleared by any human config edit. |
| `pause_reason` | Char (nullable) | Why the system paused or warned: `no_output` / `ignored` / `repeated_failures`. Also names the writer that owns the pause: a system writer may only clear or overwrite a pause carrying its own reason (`transition_status_by_system()`). Null outside `pending_pause` / `paused_by_system`. |
| `status_changed_at` | DateTime (nullable) | When `status` last changed. Anchors the cold-start grace window (`in_cold_start_grace()`): any move back to `active` re-anchors it, deliberately independent of the actor so the window survives account deletion. |
| `status_changed_by` | FK → User (nullable) | The human behind the last status change; null on system transitions, so a human re-enable and a system resume are distinguishable and a system pause never reads as a person's action. |
| `emit` | Boolean | Dry-run vs emit. Defaults `True`: a freshly authored scout is live from its first tick. Flip to `False` for dry-run — the scout runs and logs but `emit_finding` writes nothing — to validate it on a team before its findings reach the inbox. |
| `run_interval_minutes` | PositiveInteger | Minutes between runs. The coordinator dispatches rolling schedules when `last_run_at is None or now - last_run_at >= run_interval_minutes`. Default `1440` (daily). Validated `30 <= N <= 43200`. |
| `run_cron_schedule` | Char (nullable) | Optional five-field cron expression anchoring runs to wall-clock slots; takes precedence over `run_interval_minutes` when set. Interpreted in the project's timezone so daylight-saving changes are applied automatically. Occurrences must be ≥ 30 minutes apart. Null keeps the rolling interval behavior. |
Expand Down
19 changes: 17 additions & 2 deletions products/signals/backend/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,32 @@ class SignalScoutConfigAdmin(admin.ModelAdmin):
"team_link",
"skill_name",
"enabled",
"status",
"emit",
"run_interval_minutes",
"run_cron_schedule",
"last_run_at",
"updated_at",
)
list_display_links = ("id",)
list_filter = ("enabled", "emit")
list_filter = ("enabled", "status", "emit")
search_fields = ("id", "skill_name", "team__name", "team__organization__name")
raw_id_fields = ("team", "created_by", "enabled_by")
readonly_fields = ("id", "created_at", "updated_at", "last_run_at")
# The status cluster is read-only here: admin's lifecycle control stays the `enabled`
# checkbox (the model's save() derives the status pair from it), and a hand-edited status
# or attribution stamp would bypass the transition rules the API and system writers
Comment on lines +105 to +107

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Route admin saves through the human lifecycle path

When staff use the retained enabled checkbox to re-enable a paused scout, the default admin save reaches the model's actorless reconciliation path, which clears status_changed_by; in_cold_start_grace() therefore does not grant the documented grace period for this human re-enable. Similarly, editing an active field such as the schedule or emit while the scout is pending_pause does not clear that warning as the API human-write path does. Add lifecycle-aware admin save handling that stamps request.user and applies the same transition semantics as the update serializer.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Considered, leaving as is. Admin is a break-glass surface, not the product write path, and both divergences fail in the conservative direction: no grace re-anchor means a staff-touched scout gets evaluated sooner (not pause-exempt longer), and an uncleaned pending_pause means the warning stays visible rather than being silently dismissed. Duplicating the serializer's human-write semantics in save_model (or weakening the rule that the actorless reconciliation clears attribution) buys little for the risk; if staff lifecycle operations become a real flow they should go through the API, which carries the full semantics.

# enforce. Read-only also keeps `status_changed_by` off the default user `<select>`,
# which would load the whole user table.
readonly_fields = (
"id",
"created_at",
"updated_at",
"last_run_at",
"status",
"pause_reason",
"status_changed_at",
"status_changed_by",
)
list_select_related = ("team", "team__organization")
show_full_result_count = False

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import django.db.models.deletion
from django.conf import settings
from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("signals", "0076_signalscoutnote_discussion_origin"),
]

operations = [
migrations.AddField(
model_name="signalscoutconfig",
name="status",
field=models.CharField(
choices=[
("active", "Active"),
("pending_pause", "Pending pause"),
("paused_by_system", "Paused by system"),
("paused_by_user", "Paused by user"),
],
db_default="active",
default="active",
max_length=20,
),
),
migrations.AddField(
model_name="signalscoutconfig",
name="pause_reason",
field=models.CharField(
blank=True,
choices=[
("no_output", "No output"),
("ignored", "Ignored"),
("repeated_failures", "Repeated failures"),
],
max_length=20,
null=True,
),
),
migrations.AddField(
model_name="signalscoutconfig",
name="status_changed_at",
field=models.DateTimeField(blank=True, null=True),
),
migrations.AddField(
model_name="signalscoutconfig",
name="status_changed_by",
field=models.ForeignKey(
blank=True,
db_constraint=False,
Comment on lines +49 to +51

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Create the attribution index without blocking writes

Although db_constraint=False avoids taking a lock on the hot user table for a foreign-key constraint, ForeignKey still defaults to db_index=True. This AddField therefore creates a regular index on the existing scout-config table inside the atomic migration, blocking inserts and updates while PostgreSQL builds it. Set db_index=False if this attribution column does not need an index, or add the index separately with SafeAddIndexConcurrently.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f33bc52: db_index=False on the field - nothing queries by attribution (it's read per-row), so the index isn't worth having at all, which also removes the non-concurrent build inside the migration. sqlmigrate now shows a bare nullable ADD COLUMN and no CREATE INDEX.

db_index=False,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
related_name="+",
to=settings.AUTH_USER_MODEL,
),
),
]
22 changes: 22 additions & 0 deletions products/signals/backend/migrations/0078_backfill_scout_status.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from django.db import migrations


def backfill_paused_by_user(apps, schema_editor):
# Rows disabled before `status` existed can only have been switched off by a human:
# nothing system-driven ever wrote `enabled` until this field shipped. One UPDATE over a
# small table (scout configs, not events); kept separate from 0077's AddFields so the
# data write never shares a transaction with schema locks.
SignalScoutConfig = apps.get_model("signals", "SignalScoutConfig")
# `_default_manager`, not `objects`: the model's Meta routes the default manager to the
# unscoped `all_teams`, so the historical model carries no `objects` attribute.
SignalScoutConfig._default_manager.filter(enabled=False).update(status="paused_by_user")


class Migration(migrations.Migration):
dependencies = [
("signals", "0077_signalscoutconfig_status"),
]

operations = [
migrations.RunPython(backfill_paused_by_user, reverse_code=migrations.RunPython.noop),
]
2 changes: 1 addition & 1 deletion products/signals/backend/migrations/max_migration.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0076_signalscoutnote_discussion_origin
0078_backfill_scout_status
Loading
Loading