From f082b7579e56590269eb522f23331469b0ecdcc4 Mon Sep 17 00:00:00 2001 From: bartek Date: Wed, 8 Jul 2026 15:31:46 +0200 Subject: [PATCH 1/8] feat: add new aggregator integration planner and reference map documentation based on how DA, Locafy and Neustar were introduced --- .../SKILL.md | 182 ++++++++++++++++++ .../references/reference-map.md | 57 ++++++ 2 files changed, 239 insertions(+) create mode 100644 skills/product/cl-new-aggregator-integration-planner/SKILL.md create mode 100644 skills/product/cl-new-aggregator-integration-planner/references/reference-map.md diff --git a/skills/product/cl-new-aggregator-integration-planner/SKILL.md b/skills/product/cl-new-aggregator-integration-planner/SKILL.md new file mode 100644 index 0000000..b18808e --- /dev/null +++ b/skills/product/cl-new-aggregator-integration-planner/SKILL.md @@ -0,0 +1,182 @@ +--- +name: cl-new-aggregator-integration-planner +description: >- + Plans (and can drive, phase by phase) the FULL end-to-end integration of a NEW listing + aggregator / directory syncer across BrightLocal Tools + ListingSyncer — the same scope we + followed for DataAxle, Locafy and Neustar. Use this whenever someone wants to add, integrate, + onboard, wire up, or "do the full scope for" a new aggregator, publisher network, listing + partner or sync provider (e.g. "integrate Hotfrog / Yext / Foursquare", "add a new syncer", + "what's needed to connect ", "plan the aggregator work"). Also triggers on partial + states ("we started , finish it") — it maps what exists vs what's missing. It gates on two + inputs up front (integration name + API documentation), produces an ordered, per-phase plan with + exact reference files to copy from, the right clarifying questions, and the branch / migration / QA + conventions baked in — and then **creates the Jira tickets** (a parent plus one per phase) so the + work is tracked. Prefer this over ad-hoc planning for any multi-repo aggregator work. +--- + +# New Aggregator Integration Planner (Connected Locations) + +## What this is + +Adding a new listing aggregator (a partner Tools syncs business data to — like DataAxle, Locafy/Hotfrog, +Neustar/Localeze) is a **backend feature that spans two repositories** and roughly **seven work areas**. +Done ad hoc it's easy to miss a piece (a feature flag, an Active Sync column, the "mark Submitted" step, +the async backlink update). This skill encodes the full scope, the order, the exact code to model each +part on, and the questions to ask — so the plan is complete and each phase is verifiable before moving on. + +The reference integrations live on branches (fetch them; they are not all on `master`): + +- **Neustar** — `epic/neustar` — the most complete reference (covers all 7 areas incl. async backlinks). +- **Locafy (Hotfrog)** — `epic/locafy` (+ `task/LM-4162`, `task/LM-4173`) — AUS-only, simpler sync. +- **DataAxle** — `master` — the additional-data flow is already wired here. + +Repos (never `cd`; use `git -C`): Tools = `products/Tools/codebase/Tools`, ListingSyncer (LS) = +`products/ListingSyncer/codebase/ListingSyncer`. Read `references/reference-map.md` for the exact +file paths per phase — keep it open while planning. + +## How to work + +This is a **planner first**. Produce the ordered plan below, then implement phase by phase with a human +gate between phases (each phase has its own PR-sized diff and verification). Don't implement blindly — +several phases depend on answers you must collect first, and on whether the API is synchronous or async. + +### Step 0 — Gate on inputs and scope (do this before any code) + +Ask the user and do not start the API-client phase until you have (a) and (b): + +1. **(a) Integration name** — the canonical token used everywhere (e.g. `locafy`, `neustar`). Note any + brand vs token split (Locafy↔Hotfrog, Neustar↔Localeze). This becomes `Integration::X`, + `Connection::X`, `lm.x.enabled`, module `Modules/X`, columns `x_*`, `shouldSendToX`. +2. **(b) API documentation** — a file or URL for the aggregator's API. The API client (Phase 1) cannot be + designed without it. If it's missing, stop and ask; do not guess the wire format. +3. **Sync mode** — is submission **synchronous** (response returns the result/links immediately) or + **asynchronous** (submit → later poll/callback for status + backlinks)? This decides Phases 1 & 7. +4. **CB trigger set** — which Citation Builder **publishers / networks / aggregators** + (`Modules\Cb\Domain\Publisher\Entity\PublisherMetadata`) should cause a connection to be established + when a CB campaign carrying them moves to "In Progress"? (Neustar groups `AGGREGATOR_NEUSTAR` + + `NETWORK_YP` + `NETWORK_GPS`.) Also confirm which of those must flip to **Submitted** (Phase 6). +5. **Allowed countries** — which countries may this connection be offered in? (Locafy = AUS only; + Neustar/DataAxle = USA + CAN.) Drives Phase 3. + +Then set up branches per `shared/engineering/git-conventions.md` (ADR-0019): create `task/` in +**both** repos off the relevant epic base, matching branch names across repos (see `RELATED_SERVICES.md`). +Confirm the base with the user (a new aggregator usually branches off the integration epic, not `master`, +because it builds on shared plumbing that may not be merged yet — this exact mismatch has bitten us). + +### Step 1 — Present the phased plan + +Lay out the seven phases (below) as an ordered plan: for each, what to build, the reference to copy from +(`references/reference-map.md`), the inputs it depends on, and how it's verified. Mark which phases are +blocked on the API doc or on the sync-vs-async answer. Also write the plan to a file (e.g. +`docs//plan.md` in Tools) so it can be attached to the parent Jira ticket. Get the plan approved. + +### Step 2 — Create the Jira tickets (one per phase) + +Once the plan is approved, create the tracking tickets via the Atlassian MCP (`createJiraIssue`; use +`createIssueLink` to link them). Confirm these first, don't assume: + +- **Project** — default **`LM`** (Connected Locations); ask if different. +- **Parent** — create a parent **Task/Story** "Integrate aggregator" summarizing the effort (paste the + plan / attach `plan.md`), or link under an existing epic if the user names one. +- **Issue type & labels** — child tickets as `Task` (or sub-tasks of the parent if the project uses them), + labelled `BackEnd`; carry over `GOAL*` labels if the user gives one. + +Create **one child ticket per phase (1–7)**. Each ticket's description = that phase's scope + the exact +reference files (from `references/reference-map.md`) + its verification/acceptance criteria + which repo(s) +it touches. Encode ordering: set `blockedBy` / "depends on" links so Phase N+1 is blocked by Phase N, and +flag the two hard dependencies explicitly — **Phase 1 is blocked until the API doc is provided**, and +**Phase 7's shape depends on the sync-vs-async answer**. Report the created ticket keys/URLs (parent + +seven) back to the user. Prefer the `plan-ticket` skill if the user would rather review each ticket in +plan mode before it's created; otherwise create them directly here. + +Then implement **one phase per ticket**, each on its own `task/` branch(es) with its own PR(s) +and QA — matching the phase → ticket → PR cadence. + +--- + +## The seven phases + +### Phase 1 — ListingSyncer: API client + inbound endpoint *(needs: name + API doc + sync mode)* +Create `src/Modules//` in LS mirroring the Neustar module (Domain / Application / Infrastructure / +Adapter). Core pieces: the API client under `Application/Service/API/` (auth + listing + token clients as +the doc requires), request/response DTOs, field mappers, a `ListingService`/`ListingCreator`/`ListingUpdater` ++ `SubmissionHandler`, a `Request` domain entity + repository, and Doctrine migrations for the module's +tables. Expose an HTTP `ListingController` (`POST/PUT/GET /x/listing/{locationUUID}` + a status endpoint) +that Tools calls. **Async APIs** additionally need a submission-status checker + cron and a status/backlink +representation. Tools side: a thin `ApiClient` (`Modules/Location/Application/Service/Integration/X/`) over +the shared `listingSyncerHttpClient`. +Verify: LS testing CLI (Phase 2) can push a listing and you can inspect the real API response. + +### Phase 2 — Testing CLI commands (both repos) +LS: `Adapter/CLI/` push / update / check-status commands (model on the Locafy/Neustar CLI commands) so you +can exercise the client and read raw responses. Tools: a console command to drive a sync for a location. +These are how you learn the real response structure and confirm sync-vs-async behaviour before wiring the +product flows. Verify: run against a test location/account; capture response shapes for later phases. + +### Phase 3 — Tools: feature flag + availability query *(needs: allowed countries)* +Add `Config\Feature::LM_X_ENABLED = 'lm.x.enabled'` + a Doctrine migration seeding the `feature_flags` row +(ships **off**). Add `IsXIntegrationAvailableQuery` + handler (gate = FF enabled **and** location owned by +customer **and** country ∈ allowed) mirroring `IsNeustarIntegrationAvailableQueryHandler`. Add +`Connection::ALLOWED_X_COUNTRIES` + `GetXConnectionAllowedCountriesQuery`/handler, and gate the aggregator +into `GetSupportedIntegrationsQueryHandler`. Verify: unit tests for FF-off / wrong-country / wrong-owner / +happy path; the aggregator shows on LM only for allowed countries with the flag on. + +### Phase 4 — Tools: connection trigger + Active Sync enable *(needs: CB trigger set)* +Add a `Cb\Application\Listeners\XConnectionRequestListener` subscribed to `CampaignStatusUpdated` → +`onCampaignInProgress` that, when the campaign carries one of the agreed publishers/networks/aggregators +(and it's purchased / not "unavailable"), checks `IsXIntegrationAvailableQuery` and dispatches the Locafy/ +Neustar-style `ConnectLocationCommand`. The connect handler dispatches `Events::ON_LOCATION_X_CONNECTED`. +Add an `XActiveSyncListener` on that event → `ToggleActiveSyncForConnectionCommand(..., Integration::X, true)`, +and add `Integration::X` to `ToggleActiveSyncForConnectionHandler::hasConnection()` + the `ActiveSync` +settings DTO. Add LS `active_sync_settings` `x_*` columns — **migration in Tools + identical copy in LS** +per `RELATED_SERVICES.md` (`active_sync_settings` is LS-owned). Register listeners in both services loaders. +Cross-module command dispatch must go through the `IntegrationCommand` layer (Deptrac). Verify: campaign → +In Progress establishes the connection and flips `x_active_sync_enabled = 1` for eligible customers. + +### Phase 5 — Tools + LS: sync on "Save & Sync" *(depends on Phase 3 + 4)* +Tools: in `DataTransformer` set `metadata.shouldSendToX` from `IsXIntegrationAvailableQuery`, and add an +`x` section to the outbound additional-data payload via an `XTransformer` — using **LocationManager-local +DTOs** (not the Location module's) to keep module boundaries clean. LS: add `shouldSendToX` to the incoming +`Metadata` DTO + `MetadataType`, an `x` sub-form/field on the `Update` form + `AdditionalData` DTO, a +`Modules/X/Application/Service/AdditionalData/Updater`, and a **feature-flag-gated** dispatch block in +`UpdateDispatcher` (`if shouldSendToX` → sync, else log skip; bump the WaitGroup). The updater must sync +**only when the connection is established and Active Sync is enabled for X**. Verify: FF on + AUS/allowed + +connected + AS on → LS syncs on save; FF off → LS logs skip. + +### Phase 6 — Mark CB publishers/aggregators "Submitted" on connection creation +When a **new** connection object is created, LS dispatches an `XConnectionCreated` AMQP message → a Tools +`LiteWorker` consumes it → `MarkPublisher…SubmittedCommand` flips the campaign's relevant publisher/ +aggregator directory rows `To Do → Submitted` (+ `date_submitted`), never downgrading `Live`. **Include the +aggregator's own directory**, not just its networks (this was a real bug on LM-4202). Fires only on +connection creation, not every upsert. Verify: new connection → YP/GPS/aggregator rows become Submitted; +repeat upserts change nothing. + +### Phase 7 — Directory backlinks after processing *(sync mode dependent)* +**Async**: the aggregator later returns the directories/URLs where the listing went live — via the +status-check flow (cron/poll) or a callback. Map those to the CB campaign publisher directories and upgrade +`Submitted → Live` with the URL (Neustar's live/backlink flow). GPS-style networks that never return a +backlink correctly stay `Submitted`. **Sync**: links may be available in the submit response — update the +directories directly. Verify: after processing, the campaign directories show Live with correct URLs; +non-returning networks remain Submitted. + +--- + +## Conventions to enforce (every phase) + +- **Branches**: ADR-0019 (`task/` etc.), matching names across both repos, no `agent/` prefix. + Commits = Conventional Commits, **no AI co-author footer** (`shared/engineering/git-conventions.md`). +- **Migrations**: authoritative in Tools `migration/doctrine/` (`namespace Migrations`), and for any + **LS-owned table** (e.g. `active_sync_settings`, `x_*`) add an **identical** copy in LS `src/Migrations/` + (`namespace DoctrineMigrations`). See `RELATED_SERVICES.md`. +- **Cross-module writes**: dispatch via `Application/IntegrationCommand/` (Deptrac blocks another module's + private `Application/Command/*`). Keep new DTOs local to the consuming module. +- **QA before each commit** — **Tools**: `make phpstan` (L8), `make phpunit`, `make ecs`, `make deptrac`, + `make di-check` (fix everything, incl. DI). **LS**: `make phpstan`, `make phpunit`, `make ecs`. +- **LS tests** live under `src/Modules/*/Test/` and are phpstan-analysed; LS runs **PHPUnit 9** → use a + `/** @covers … */` docblock (not `#[CoversClass]`) and **Prophecy** for final classes (not `createMock`). +- Run the app / CLI to verify behaviour end-to-end, not just tests (esp. the sync round-trip). + +## Reference map + +`references/reference-map.md` lists the exact files to copy from per phase, per reference integration +(Neustar / Locafy / DataAxle) and which branch each lives on. Read it before planning or implementing a phase. diff --git a/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md b/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md new file mode 100644 index 0000000..04864a3 --- /dev/null +++ b/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md @@ -0,0 +1,57 @@ +# Reference map — files to copy from, per phase + +Model the new integration `` on these. **Neustar (`epic/neustar`) is the most complete reference.** +Read files across branches with `git -C show :` (don't switch branches mid-plan). +Tools = `products/Tools/codebase/Tools`, LS = `products/ListingSyncer/codebase/ListingSyncer`. + +| Integration | Branch(es) | Notes | +|---|---|---| +| Neustar / Localeze | `epic/neustar` | Full scope incl. async backlinks + "mark Submitted" (LM-4202). Best reference. | +| Locafy / Hotfrog | `epic/locafy`, `task/LM-4162`, `task/LM-4173` | AUS-only; simpler; sync-on-save added in LM-4173. | +| DataAxle | `master` | Additional-data flow already wired; good for the Save&Sync DTO/updater shape. | + +Replace `Neustar`/`neustar` with ``/`` throughout. Confirm each path still exists on the branch +(`git -C ls-tree -r --name-only | grep -i neustar`). + +## Phase 1 — LS module: API client + endpoint (LS, `epic/neustar`) +- Module tree: `src/Modules/Neustar/` (`Domain/`, `Application/`, `Infrastructure/`, `Adapter/`) +- API client: `src/Modules/Neustar/Application/Service/API/` (`AuthenticatedClient`, `ListingClient`, `TokenClient`) +- Submission pipeline: `Application/Service/ListingService.php` (`upsert`), `ListingFactory`, `ListingValidator`, mappers under `Application/Service/Mappers/` (Locafy has `CategoryMapper`, `ImagesMapper`, `TradingHoursMapper`, `SocialProfilesMapper`) +- DTOs: `Application/DTO/` (`LocationData`, `AdditionalData`), API DTOs under `Application/Service/API/` +- Domain: `Domain/Entity/Request.php`, `Domain/Repository/…`, `Infrastructure/Repository/RequestRepository.php` +- HTTP: `Adapter/HTTP/ListingController.php` (`POST/PUT/GET /neustar/listing/{locationUUID}`, `/…/request-status`, failed-requests, categories) +- Config: `config//services.yaml`, `config/routes.yaml`, `config/packages/doctrine.yaml`; `src/Constant/Integrations.php` +- Async extras: `SubmissionStatusChecker`, `PendingSubmissionChecker`, `src/Cron/…CheckSubmissionResults.php` +- Tools-side thin client: `src/Modules/Location/Application/Service/Integration/Neustar/ApiClient.php` (uses `listingSyncerHttpClient`; registered in `src/Modules/Location/Resources/config/services-symfony-loader.php` with the `%listingSyncer.*_timeout%` param) + +## Phase 2 — Testing CLI (both repos) +- LS: `src/Modules/Neustar/Adapter/CLI/…` and Locafy's `LocafyPushListingCommand`, `LocafyUpdateListingCommand`, `LocafyCheckListingSubmissionCommand`, `LocafyRunSubmissionCheckerCommand` +- Tools: `src/Modules/Location/Adapter/Console/Neustar/…` (+ `DataAxle`) console commands + +## Phase 3 — FF + availability (Tools, `epic/neustar`) +- Flag: `src/Config/Feature.php` (`LM_NEUSTAR_ENABLED = 'lm.neustar.enabled'`); seed migration modelled on the `feature_flags` INSERT migration (`migration/doctrine/Version20260609120000.php`) +- Availability: `src/Modules/Location/Application/Query/Integration/Neustar/IsNeustarIntegrationAvailableQuery.php` + `…QueryHandler.php` +- Allowed countries: `src/Modules/LocationConnections/Application/Constant/Connection.php` (`ALLOWED_NEUSTAR_COUNTRIES`) + `src/Modules/LocationConnections/Application/Query/GetNeustarConnectionAllowedCountriesQuery(+Handler).php` +- Integration id: `src/Includes/Constant/Integration.php` (`NEUSTAR` + `NAME_MAP`) +- Surface: `src/Modules/LocationConnections/Application/Query/GetSupportedIntegrationsQueryHandler.php` +- Handlers auto-register via `src/HaploFramework/ServicesLoader.php` (no manual DI for query handlers) + +## Phase 4 — connection trigger + AS enable (Tools + LS) +- Trigger (Tools): `src/Modules/Cb/Application/Listeners/NeustarConnectionRequestListener.php` (on `CampaignStatusUpdated`→`onCampaignInProgress`); DataAxle's uses `CampaignPurchasedEvent` — pick per the "in progress" requirement. Register in `src/Modules/Cb/Resources/config/services-symfony-loader.php`. +- Publisher/aggregator set: `src/Modules/Cb/Domain/Publisher/Entity/PublisherMetadata.php` (`AGGREGATOR_*`, `NETWORK_*`); directories in `Domain/Publisher/Entity/PublisherDirectory.php` (`SITES`) +- Connect command: `src/Modules/Location/Application/IntegrationCommand/Integration/Neustar/Connection/ConnectLocationCommand.php` (+ handler in `…/Command/Integration/Neustar/Connection/`). **Cross-module dispatch must use the `IntegrationCommand` layer** (Deptrac) — Locafy's promotion in LM-4162 is the worked example. +- AS-enable (Tools): `src/Modules/Location/Application/EventListener/NeustarActiveSyncListener.php` (on `Events::ON_LOCATION_NEUSTAR_SUBMITTED`/`…_CONNECTED` → `ToggleActiveSyncForConnectionCommand(…, Integration::NEUSTAR, true)`). Register in `src/Modules/Location/Resources/config/services-symfony-loader.php`. +- Toggle support: `src/Modules/Location/Application/Command/ActiveSync/ToggleActiveSyncForConnection/ToggleActiveSyncForConnectionHandler.php` (`hasConnection()` match arm) + `src/Modules/LocationManager/Dto/ActiveSync/ActiveSync.php` (field + `connection()` case + getter/setter) + `LocationConnections` `LocationConnections` DTO `getX()` +- AS-settings columns (LS-owned table): LS `src/Entity/Location/Settings/ActiveSync.php` embedded `x_` connection + `src/Migrations/Version*.php`; **plus the identical migration in Tools `migration/doctrine/`** (Neustar: LS `Version20260612120000` mirrors Tools). Response/DTO/transformer/form: LS `src/Service/Location/Settings/Dto/ActiveSyncSettings.php`, `ActiveSyncSettingsTransformer.php`, `src/Message/DTO/ActiveSyncDTO.php`, `src/Form/Location/Settings/ActiveSyncForm.php`. + +## Phase 5 — Save & Sync (Tools + LS, `epic/neustar`) +- Tools build: `src/Modules/LocationManager/Services/AdditionalData/Updating/Transformers/DataTransformer.php` (sets `shouldSendToX` from `IsXIntegrationAvailableQuery`; adds `x` section via an `XTransformer`), `.../Updating/Dto/Metadata.php`, `.../Updating/Dto/AdditionalData.php`. **Use LocationManager-local DTOs** (`.../Updating/Dto//…`) — do NOT reuse the Location module's Integration DTOs (Deptrac; LM-4173 lesson). Outbound HTTP: `src/Modules/LocationManager/Services/AdditionalData/Client.php` (`POST /additional-data/locations/{uuid}`). +- LS consume + gate: `src/Form/AdditionalData/Dto/Metadata.php` (+ `MetadataType.php`), `src/Form/AdditionalData/Update.php` (+ `Dto/AdditionalData.php`), `src/Modules//Application/Service/AdditionalData/Updater.php` (mirror `Modules/Neustar/…/AdditionalData/Updater` or DataAxle's — gate on connection + `getX()->isActiveSyncEnabled()`), and the gated block in `src/Service/AdditionalData/UpdateDispatcher.php` (`if metadata->shouldSendToX` → sync, else log skip; bump `WaitGroup`). + +## Phase 6 — mark Submitted on connection creation (LS → RabbitMQ → Tools) +- LS producer: `src/Modules/Neustar/Application/Message/NeustarConnectionCreated.php` dispatched from `ListingService::upsert()` inside the new-connection branch; AMQP transport/routing in `config/packages/messenger.yaml` (copy the `Lm…` block). +- Tools consumer: `workers-mq/LmConnectionCreated` (filename == queue name, perms 777) + `src/Modules/Location/Adapter/Worker/…ConnectionCreated.php` (extends `LiteWorker`) + handler → CB IntegrationCommand `Modules\Cb\Application\IntegrationCommand\Publisher\MarkPublisherDirectoriesSubmitted\MarkPublisherDirectoriesSubmittedHandler` (LM-4202) — flips `To Do → Submitted`, never downgrades `Live`, and **includes the aggregator's own directory** (`AGGREGATOR_*`), not just its networks. Register queue in `QueueTeamMapping`. + +## Phase 7 — backlinks after processing +- Neustar live/backlink flow (LM-4164): the publish path that upgrades `Submitted → Live` with URLs — LS `src/Modules/Neustar/…` publications sync + `NeustarListingPublished` message → Tools `NeustarListingPublishedHandler` / `PublisherSubmissionUpdater`. GPS-style networks that never return a backlink stay `Submitted`. +- **Sync APIs**: the submit response may already carry directory URLs — update the CB campaign publisher directories directly instead of via the async publish flow. From 8475ed3109403c94184f9ebde94a01e3372330c2 Mon Sep 17 00:00:00 2001 From: bartek Date: Wed, 8 Jul 2026 15:32:08 +0200 Subject: [PATCH 2/8] feat: add new aggregator integration planner and reference map documentation based on how DA, Locafy and Neustar were introduced --- .claude/settings.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.claude/settings.json b/.claude/settings.json index 973e56d..0218941 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -8,7 +8,16 @@ "Write(products/**/codebase/**)", "Write(personal/**)", "Write(agents/jira-to-pr/workspace/**)", - "mcp__*" + "mcp__atlassian", + "mcp__claude_ai_Atlassian_Rovo", + "mcp__claude_ai_Google_Calendar", + "mcp__claude_ai_Gmail", + "mcp__claude_ai_Google_Drive", + "mcp__claude_ai_BrightLocal_MCP", + "mcp__claude_ai_BrightLocal_Design_System", + "mcp__claude_ai_Firecrawl", + "mcp__claude_ai_Metabase", + "mcp__metabase" ], "deny": [ "Write(shared/**)", From cd615e9740674eccc2c5c118985997ec4778c248 Mon Sep 17 00:00:00 2001 From: bartek Date: Thu, 9 Jul 2026 10:53:18 +0200 Subject: [PATCH 3/8] feat(skills): expand cl-new-aggregator-integration-planner (FE/BE split, dashboard, plan-only) Add a failed-submissions admin dashboard phase; split frontend from backend into separate tickets and surface the aggregator in the LM edit-page Aggregators section (not Connections); require each ticket to carry a description + acceptance criteria + dependency ordering; add a Phase 1 endpoint-to-role mapping step (ask when the API doc is ambiguous); and reframe the skill as plan-and-tickets only (never implements or asks to start implementation). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../SKILL.md | 131 +++++++++++++----- .../references/reference-map.md | 13 ++ 2 files changed, 111 insertions(+), 33 deletions(-) diff --git a/skills/product/cl-new-aggregator-integration-planner/SKILL.md b/skills/product/cl-new-aggregator-integration-planner/SKILL.md index b18808e..15978e7 100644 --- a/skills/product/cl-new-aggregator-integration-planner/SKILL.md +++ b/skills/product/cl-new-aggregator-integration-planner/SKILL.md @@ -1,9 +1,10 @@ --- name: cl-new-aggregator-integration-planner description: >- - Plans (and can drive, phase by phase) the FULL end-to-end integration of a NEW listing - aggregator / directory syncer across BrightLocal Tools + ListingSyncer — the same scope we - followed for DataAxle, Locafy and Neustar. Use this whenever someone wants to add, integrate, + Plans the FULL end-to-end integration of a NEW listing aggregator / directory syncer across + BrightLocal Tools + ListingSyncer — the same scope we followed for DataAxle, Locafy and Neustar. + This is a PLANNING skill only: it produces the plan and the Jira tickets and then stops — it never + writes code, never sets up branches, and never asks whether to start implementation. Use this whenever someone wants to add, integrate, onboard, wire up, or "do the full scope for" a new aggregator, publisher network, listing partner or sync provider (e.g. "integrate Hotfrog / Yext / Foursquare", "add a new syncer", "what's needed to connect ", "plan the aggregator work"). Also triggers on partial @@ -19,7 +20,7 @@ description: >- ## What this is Adding a new listing aggregator (a partner Tools syncs business data to — like DataAxle, Locafy/Hotfrog, -Neustar/Localeze) is a **backend feature that spans two repositories** and roughly **seven work areas**. +Neustar/Localeze) is a **backend feature that spans two repositories** and roughly **eight work areas**. Done ad hoc it's easy to miss a piece (a feature flag, an Active Sync column, the "mark Submitted" step, the async backlink update). This skill encodes the full scope, the order, the exact code to model each part on, and the questions to ask — so the plan is complete and each phase is verifiable before moving on. @@ -36,11 +37,14 @@ file paths per phase — keep it open while planning. ## How to work -This is a **planner first**. Produce the ordered plan below, then implement phase by phase with a human -gate between phases (each phase has its own PR-sized diff and verification). Don't implement blindly — -several phases depend on answers you must collect first, and on whether the API is synchronous or async. +This is a **planning skill only**. Its entire job is to produce the ordered plan and the Jira tickets +(each with a description, acceptance criteria, and dependency links) — then **stop**. Do **not** write +code, create branches, or open PRs, and **never ask whether to start implementing**. Implementation is a +separate effort a developer (or the `bl-engineer` skill) picks up from the tickets later. Each phase below +is written as the *content of a ticket* (scope + how it will be verified), not as steps for you to execute +now. Several phases carry dependencies you must resolve while planning (missing answers, API doc, sync-vs-async). -### Step 0 — Gate on inputs and scope (do this before any code) +### Step 0 — Gate on inputs and scope Ask the user and do not start the API-client phase until you have (a) and (b): @@ -58,14 +62,16 @@ Ask the user and do not start the API-client phase until you have (a) and (b): 5. **Allowed countries** — which countries may this connection be offered in? (Locafy = AUS only; Neustar/DataAxle = USA + CAN.) Drives Phase 3. -Then set up branches per `shared/engineering/git-conventions.md` (ADR-0019): create `task/` in -**both** repos off the relevant epic base, matching branch names across repos (see `RELATED_SERVICES.md`). -Confirm the base with the user (a new aggregator usually branches off the integration epic, not `master`, -because it builds on shared plumbing that may not be merged yet — this exact mismatch has bitten us). +Also settle the **branch base** and **record it in the plan** for the implementer to use later (do NOT +create branches). Per `shared/engineering/git-conventions.md` (ADR-0019) the work will use `task/` +branches with matching names across both repos (see `RELATED_SERVICES.md`); confirm the base with the user +— a new aggregator usually branches off the integration epic, not `master`, because it builds on shared +plumbing that may not be merged yet (this exact mismatch has bitten us). This is captured as guidance in +the tickets, not acted on here. ### Step 1 — Present the phased plan -Lay out the seven phases (below) as an ordered plan: for each, what to build, the reference to copy from +Lay out the eight phases (below) as an ordered plan: for each, what to build, the reference to copy from (`references/reference-map.md`), the inputs it depends on, and how it's verified. Mark which phases are blocked on the API doc or on the sync-vs-async answer. Also write the plan to a file (e.g. `docs//plan.md` in Tools) so it can be attached to the parent Jira ticket. Get the plan approved. @@ -78,25 +84,62 @@ Once the plan is approved, create the tracking tickets via the Atlassian MCP (`c - **Project** — default **`LM`** (Connected Locations); ask if different. - **Parent** — create a parent **Task/Story** "Integrate aggregator" summarizing the effort (paste the plan / attach `plan.md`), or link under an existing epic if the user names one. -- **Issue type & labels** — child tickets as `Task` (or sub-tasks of the parent if the project uses them), - labelled `BackEnd`; carry over `GOAL*` labels if the user gives one. - -Create **one child ticket per phase (1–7)**. Each ticket's description = that phase's scope + the exact -reference files (from `references/reference-map.md`) + its verification/acceptance criteria + which repo(s) -it touches. Encode ordering: set `blockedBy` / "depends on" links so Phase N+1 is blocked by Phase N, and -flag the two hard dependencies explicitly — **Phase 1 is blocked until the API doc is provided**, and -**Phase 7's shape depends on the sync-vs-async answer**. Report the created ticket keys/URLs (parent + -seven) back to the user. Prefer the `plan-ticket` skill if the user would rather review each ticket in -plan mode before it's created; otherwise create them directly here. - -Then implement **one phase per ticket**, each on its own `task/` branch(es) with its own PR(s) -and QA — matching the phase → ticket → PR cadence. +w- **Issue type & labels** — child tickets as `Task` (or sub-tasks of the parent if the project uses them); + label each `BackEnd` or `FrontEnd` per its workflow; carry over `GOAL*` labels if the user gives one. + +Create **at least one child ticket per phase (1–8)**, and **split frontend from backend into separate +tickets** wherever a phase has non-trivial frontend work — the team tracks FE and BE separately. Concretely: +- **Phase 3** → a `BackEnd` ticket (FF + availability query) **and** a `FrontEnd` ticket (the Connections- + section tile on the LM edit page). +- **Phase 4** → a `BackEnd` ticket (connection trigger + AS enable) **and** a `FrontEnd` ticket (tile shows + the connected state). +- **Phase 8** → a `BackEnd` ticket (failed-requests/resubmit endpoints + providers) **and** a `FrontEnd` + ticket (the Sysadmin dashboard UI). +- Phases 1, 2, 5, 6, 7 are backend-only unless the user flags a UI need. +Make each `FrontEnd` ticket `blockedBy` its `BackEnd` counterpart (the UI needs the data/flag/endpoint first). + +Every ticket MUST have two clearly separated sections: + +- **Description** — the phase's scope, which repo(s) it touches, and the exact reference files to copy from + (`references/reference-map.md`). +- **Acceptance criteria** — a checklist (`* [ ] …`) of objectively verifiable outcomes, taken from that + phase's "Verify:" line. These are how QA/reviewers sign the ticket off. + +**Encode the dependency order so it's obvious what goes first, second, …** — set `blockedBy` / "depends on" +links (via `createIssueLink`) between the child tickets. The default chain is linear (Phase N+1 blocked by +Phase N), with these specific rules: + +- **Phase 1** is blocked until the **API doc** is provided (note it on the ticket). +- **Phase 3** (FF + availability) and **Phase 4** (connection trigger + AS) block **Phase 5** (Save&Sync). +- **Phase 6** (mark Submitted) depends on **Phase 1** (connection-created event/endpoint) and **Phase 4**. +- **Phase 7** depends on the **sync-vs-async** answer and on **Phase 6**. +- **Phase 8** (failed-submissions dashboard) depends on **Phase 1** (needs the LS failed-requests + resubmit + endpoints) and the request-status tracking; it can otherwise proceed in parallel with 5–7. + +Also state the ordering in plain words in the parent ticket (a numbered list) so the sequence is legible +without reading the link graph. Report all created ticket keys/URLs (parent + every child, BE and FE) to +the user. +Prefer the `plan-ticket` skill if the user would rather review each ticket in plan mode before it's +created; otherwise create them directly here. + +**Stop after the tickets are created and reported.** This skill's job ends here — do not start any phase, +do not create branches or PRs, and do not ask the user whether to begin implementing. Each ticket already +carries the guidance a developer (or the `bl-engineer` skill) needs to pick it up later; the phase → ticket +→ PR cadence is theirs to run, not this skill's. --- -## The seven phases +## The eight phases (ticket content — for planning, not for executing now) ### Phase 1 — ListingSyncer: API client + inbound endpoint *(needs: name + API doc + sync mode)* +**First, map the API doc's endpoints to their roles.** Before designing the client, work out from the +documentation which endpoint does what — at minimum: (a) **submission** (create/update a listing in the +external source), (b) **reading the current listing data** already held by the aggregator, and (c) +**retrieving backlinks / the live directory URLs** after processing. **If the doc is ambiguous and you +cannot confidently decide which endpoint serves which role, ASK the user** — don't guess. Record the +chosen endpoint-to-role mapping in the Phase 1 ticket, since Phases 5 (submission), and 6–7 (status + +backlinks) all depend on it, and the sync-vs-async shape follows from whether backlinks come back on the +submission response or only from a separate poll/callback. Create `src/Modules//` in LS mirroring the Neustar module (Domain / Application / Infrastructure / Adapter). Core pieces: the API client under `Application/Service/API/` (auth + listing + token clients as the doc requires), request/response DTOs, field mappers, a `ListingService`/`ListingCreator`/`ListingUpdater` @@ -118,8 +161,13 @@ Add `Config\Feature::LM_X_ENABLED = 'lm.x.enabled'` + a Doctrine migration seedi (ships **off**). Add `IsXIntegrationAvailableQuery` + handler (gate = FF enabled **and** location owned by customer **and** country ∈ allowed) mirroring `IsNeustarIntegrationAvailableQueryHandler`. Add `Connection::ALLOWED_X_COUNTRIES` + `GetXConnectionAllowedCountriesQuery`/handler, and gate the aggregator -into `GetSupportedIntegrationsQueryHandler`. Verify: unit tests for FF-off / wrong-country / wrong-owner / -happy path; the aggregator shows on LM only for allowed countries with the flag on. +into `GetSupportedIntegrationsQueryHandler`. **Frontend (separate FrontEnd ticket):** surface the new +integration in the **Aggregators section** of the Location Manager edit page (the "Connect & Sync" area) — +the same place DataAxle and Neustar appear, **not** the Connections/social tiles. Add it to +`AggregatorsSection.tsx` (the `AGGREGATORS` list + an `isEnabled` prop) and pass the enabled flag from +the backend availability/FF. Verify: unit tests for FF-off / wrong-country / wrong-owner / happy path (BE); +the aggregator appears in the LM edit-page **Aggregators** section only for allowed countries with the flag +on (FE). ### Phase 4 — Tools: connection trigger + Active Sync enable *(needs: CB trigger set)* Add a `Cb\Application\Listeners\XConnectionRequestListener` subscribed to `CampaignStatusUpdated` → @@ -130,8 +178,12 @@ Add an `XActiveSyncListener` on that event → `ToggleActiveSyncForConnectionCom and add `Integration::X` to `ToggleActiveSyncForConnectionHandler::hasConnection()` + the `ActiveSync` settings DTO. Add LS `active_sync_settings` `x_*` columns — **migration in Tools + identical copy in LS** per `RELATED_SERVICES.md` (`active_sync_settings` is LS-owned). Register listeners in both services loaders. -Cross-module command dispatch must go through the `IntegrationCommand` layer (Deptrac). Verify: campaign → -In Progress establishes the connection and flips `x_active_sync_enabled = 1` for eligible customers. +Cross-module command dispatch must go through the `IntegrationCommand` layer (Deptrac). **Frontend +(separate FrontEnd ticket):** once establishment works, the aggregator must be **visible as connected in +the Aggregators section** of the LM edit page — the `AggregatorsSection.tsx` entry added in Phase 3 must +reflect the established/connected state (status/chip), like DataAxle and Neustar, not just availability. +Verify: campaign → In Progress establishes the connection, flips `x_active_sync_enabled = 1` for eligible +customers (BE), and the LM edit-page **Aggregators** section shows the integration as connected (FE). ### Phase 5 — Tools + LS: sync on "Save & Sync" *(depends on Phase 3 + 4)* Tools: in `DataTransformer` set `metadata.shouldSendToX` from `IsXIntegrationAvailableQuery`, and add an @@ -159,6 +211,17 @@ backlink correctly stay `Submitted`. **Sync**: links may be available in the sub directories directly. Verify: after processing, the campaign directories show Live with correct URLs; non-returning networks remain Submitted. +### Phase 8 — Failed-submissions admin dashboard (Tools Sysadmin) *(needs: Phase 1 endpoints)* +Add a Sysadmin dashboard so support can **review failed submissions for the new integration and resubmit +them**, modelled on the Neustar listings dashboard. Tools: `src/App/Sysadmin//ListingsDashboard/` with a +list action (fetch failed requests via the Tools `ApiClient` → LS) and a resubmit action, plus its +assets/template. LS: expose (or reuse) the request endpoints the dashboard drives — list failed requests, +fetch one, and resubmit — backed by a `FailedRequestsProvider` and a `RequestResubmitter`; async +integrations key off the `Request` status/error captured in Phase 1/7. Depends on Phase 1 (the LS +failed-requests + resubmit endpoints and request tracking must exist); otherwise independent of 5–7. +Verify: a deliberately-failed submission appears in the dashboard with its error, and "Resubmit" re-queues +it and moves it out of the failed list on success. + --- ## Conventions to enforce (every phase) @@ -170,8 +233,9 @@ non-returning networks remain Submitted. (`namespace DoctrineMigrations`). See `RELATED_SERVICES.md`. - **Cross-module writes**: dispatch via `Application/IntegrationCommand/` (Deptrac blocks another module's private `Application/Command/*`). Keep new DTOs local to the consuming module. -- **QA before each commit** — **Tools**: `make phpstan` (L8), `make phpunit`, `make ecs`, `make deptrac`, +- **QA before each commit** — **Tools BE**: `make phpstan` (L8), `make phpunit`, `make ecs`, `make deptrac`, `make di-check` (fix everything, incl. DI). **LS**: `make phpstan`, `make phpunit`, `make ecs`. + **Tools FE** (Connections tile, dashboard UI): `make eslint`, `make tsc`, `make jest`. - **LS tests** live under `src/Modules/*/Test/` and are phpstan-analysed; LS runs **PHPUnit 9** → use a `/** @covers … */` docblock (not `#[CoversClass]`) and **Prophecy** for final classes (not `createMock`). - Run the app / CLI to verify behaviour end-to-end, not just tests (esp. the sync round-trip). @@ -179,4 +243,5 @@ non-returning networks remain Submitted. ## Reference map `references/reference-map.md` lists the exact files to copy from per phase, per reference integration -(Neustar / Locafy / DataAxle) and which branch each lives on. Read it before planning or implementing a phase. +(Neustar / Locafy / DataAxle) and which branch each lives on. Read it while planning a phase / writing its +ticket so the ticket description points at the exact files the implementer will copy from. diff --git a/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md b/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md index 04864a3..4a36e1d 100644 --- a/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md +++ b/skills/product/cl-new-aggregator-integration-planner/references/reference-map.md @@ -35,6 +35,14 @@ Replace `Neustar`/`neustar` with ``/`` throughout. Confirm each path still - Integration id: `src/Includes/Constant/Integration.php` (`NEUSTAR` + `NAME_MAP`) - Surface: `src/Modules/LocationConnections/Application/Query/GetSupportedIntegrationsQueryHandler.php` - Handlers auto-register via `src/HaploFramework/ServicesLoader.php` (no manual DI for query handlers) +- **Frontend (Aggregators section on the LM edit page)** — the aggregator renders here, the same place + DataAxle and Neustar show (NOT the `connect_integration` Connections/social tiles). Reference (on + `epic/neustar`): `src/App/LocationDashboard/Resources/assets-v2/location_manager/components/ConnectAndSync/AggregatorsSection.tsx` + — the `AGGREGATORS` list + per-aggregator `isEnabled` prop (driven by the backend availability/FF) + + the `

Aggregators

` section. Add the new integration there (availability in Phase 3, connected + state in Phase 4). Aggregator logos live under `frontend/shared/connections_integrations/assets/logos/` + and `public/images/v2/aggregators/`. FE checks: `make eslint`, `make tsc`, `make jest` + (Yarn build via `make frontend_builder`). ## Phase 4 — connection trigger + AS enable (Tools + LS) - Trigger (Tools): `src/Modules/Cb/Application/Listeners/NeustarConnectionRequestListener.php` (on `CampaignStatusUpdated`→`onCampaignInProgress`); DataAxle's uses `CampaignPurchasedEvent` — pick per the "in progress" requirement. Register in `src/Modules/Cb/Resources/config/services-symfony-loader.php`. @@ -55,3 +63,8 @@ Replace `Neustar`/`neustar` with ``/`` throughout. Confirm each path still ## Phase 7 — backlinks after processing - Neustar live/backlink flow (LM-4164): the publish path that upgrades `Submitted → Live` with URLs — LS `src/Modules/Neustar/…` publications sync + `NeustarListingPublished` message → Tools `NeustarListingPublishedHandler` / `PublisherSubmissionUpdater`. GPS-style networks that never return a backlink stay `Submitted`. - **Sync APIs**: the submit response may already carry directory URLs — update the CB campaign publisher directories directly instead of via the async publish flow. + +## Phase 8 — failed-submissions admin dashboard (Tools Sysadmin + LS) +- Tools Sysadmin (reference): `src/App/Sysadmin/Neustar/ListingsDashboard/Actions/` — `NeustarFailedSubmissions.php` (list), `ResubmitNeustarFailedRequest.php` (resubmit), `NeustarCategories.php`; plus its `Resources/assets*` + template. Drives the list/resubmit via the Tools `Modules/Location/Application/Service/Integration/Neustar/ApiClient` methods (`fetchFailedRequests`, `resubmit`) and Query handlers under `Application/Query/Integration/Neustar/` (`FetchNeustarFailedSubmissionsQuery`, `GetListingStatusQuery`). +- LS endpoints (reference): `src/Modules/Neustar/Adapter/HTTP/RequestController.php` — `GET /neustar/failed-requests`, `GET /neustar/requests/{requestId}`, `POST /neustar/requests/{requestId}/resubmit`; backed by `Application/Service/FailedRequestsProvider.php` and `RequestResubmitter.php`. Failed state comes from the `Request` entity status/error captured in Phase 1/7. +- Note: the Neustar admin dashboard lives in the legacy `src/App/Sysadmin/` area (not `src/Modules/`); follow the existing Sysadmin dashboard structure for consistency rather than the Modules hexagonal layout. From 0872946975f697a6331d42ae03950bb5bdd55aa7 Mon Sep 17 00:00:00 2001 From: bartek Date: Thu, 9 Jul 2026 10:54:13 +0200 Subject: [PATCH 4/8] feat: update context documentation for Tools product, detailing architecture, tech stack, and key integrations --- products/ListingSyncer/CONTEXT.md | 113 ++++++++++++++++---- products/Tools/CONTEXT.md | 169 ++++++++++++++++++------------ 2 files changed, 193 insertions(+), 89 deletions(-) diff --git a/products/ListingSyncer/CONTEXT.md b/products/ListingSyncer/CONTEXT.md index b333d97..45276e8 100644 --- a/products/ListingSyncer/CONTEXT.md +++ b/products/ListingSyncer/CONTEXT.md @@ -1,42 +1,109 @@ # ListingSyncer — Context -ListingSyncer is a PHP microservice that synchronises business listings across -third-party platforms: Google My Business, Facebook, Apple Maps, Bing, Yelp, and -Data Axle. It runs as a Swoole HTTP server (port 9501) and exposes a REST API -consumed by BrightLocal Tools. +ListingSyncer (LS) is a PHP microservice that synchronises business listings across +third-party platforms and aggregators. As a secondary purpose it also backs several +API integrations used by other BrightLocal Tools features (GMB insights, review +fetching, Google Analytics). It runs as an **OpenSwoole 22** HTTP server on **port 9501** +and exposes an internal-only REST API consumed by **BrightLocal Tools** (sibling repo `../Tools`). +It also ships a Symfony console app for maintenance commands and Symfony Messenger +(AMQP/RabbitMQ) workers for async processing. ## Repository -`/home/lenovo/brightLocal/ListingSyncer` (or `github.com/BrightLocal/ListingSyncer`) +`/home/bartek/IdeaProjects/ListingSyncer` (or `github.com/BrightLocal/ListingSyncer`). +Access via `products/ListingSyncer/codebase/ListingSyncer/` — use `git -C`, never `cd`. + +## Platforms & aggregators + +- **Modularised (`src/Modules/`, layered):** Bing, DataAxle, Neustar. Yelp exists as a + module folder but is currently only a `Test/` stub. +- **Legacy (still in `src/Service/` + `src/Controller/`):** Google/GMB, Apple, Facebook, + Twitter, Yelp, Brandify. + +> Migration in progress: platform integrations are moving from `src/Service/` +> into `src/Modules//` with a hexagonal layout. Neustar and DataAxle are +> the most actively developed areas (epic/neustar, LM-4164 / LM-4129). + +## Tech stack + +- **PHP** `>=8.3` +- **Symfony** `7.0.*` (framework-bundle, console, messenger, serializer, http-client, + validator, cache, lock, rate-limiter, uid) +- **OpenSwoole 22** (`openswoole/core 22.1.5`) via `swoole-bundle/swoole-bundle` +- **Doctrine ORM ^2** + doctrine-migrations-bundle ^3; `pixelfederation/doctrine-resettable-em-bundle` + (Swoole-aware entity-manager reset); `ramsey/uuid-doctrine` +- **Messaging:** Symfony Messenger + AMQP (RabbitMQ) +- **Cron:** `easyswoole/crontab` +- **Observability:** Sentry (`sentry/sentry-symfony`), Prometheus (`artprima/prometheus-metrics-bundle`) +- **APIs/SDKs:** `nelmio/api-doc-bundle` (OpenAPI), `google/apiclient`, `google/gmb`, + `google/cloud-pubsub`, `facebook/graph-sdk` (BrightLocal fork), `abraham/twitteroauth`, + `giggsey/libphonenumber-for-php`, Redis (`predis` + `ext-redis`) +- **Private packages:** `brightlocal/php-rpc`, `brightlocal/profile-finder-client-php` +- **Dev/QA:** PHPUnit ^9.5, PHPStan ^1.2 (level 8, baseline `config/phpstan-baseline.neon`), + ECS ^11.1 (PSR-12), `dg/bypass-finals` ## Architecture ``` src/ -├── Modules/ # Platform integrations (modular monolith) -│ ├── DataAxle/ -│ ├── Bing/ -│ └── Yelp/ -├── Service/ # Core logic (GMB, Facebook, Apple, Twitter, Active Sync) -├── Entity/ # Doctrine ORM entities +├── Modules/ # Modular-monolith platform integrations (Bing, DataAxle, Neustar, Yelp-stub) +├── Service/ # Legacy core logic (Google, Apple, Facebook, Twitter, Yelp, Connection, Listing, Location, …) +├── Controller/ # HTTP controllers (Symfony + Nelmio API Doc), incl. Healthcheck, Uptime, External +├── Entity/ # Doctrine ORM entities (core tables) ├── Repository/ # Core Doctrine repositories -├── Controller/ # HTTP controllers (Symfony + Nelmio API Doc) -└── Worker/ # Symfony Messenger consumers +├── Migrations/ # Doctrine migrations +├── Message/ # Async message DTOs (AMQP) +├── Worker/ # Messenger consumers + handlers +├── Cron/ # easyswoole/crontab scheduled tasks +├── Command/ # Core Symfony console commands +├── Core/ Shared/ Infrastructure/ Dto/ Form/ Event/ EventListener/ Exception/ +└── Kernel.php ``` -Each `src/Modules//` follows a strict layered structure: -`Domain/` → `Application/` → `Infrastructure/` → `Adapter/CLI|HTTP/` +Each mature `src/Modules//` follows a hexagonal layout: +`Domain/` (Entity, Repository interfaces, Exception) → `Application/` (Service incl. +`Service/API/` third-party clients, DTO, Handler/Message, Serializer) → +`Infrastructure/` (Repository impls, MessageConsumer, Cron) → `Adapter/` (`CLI/`, `HTTP/`, `Form/`). + +Per-module third-party API clients live under `Application/Service/API/` (e.g. Bing `ApiClient`, +Neustar `AuthenticatedClient`/`ListingClient`/`TokenClient`, DataAxle `DataAxleApiClientResolver`). +Legacy platforms (GMB, Facebook, Apple, Twitter, Yelp) use vendor SDKs from `src/Service//`. ## Key integration points -- **Tools → LS**: Tools calls LS over HTTP. The `listingSyncerHttpClient` Guzzle - client in Tools uses `ListingSyncerGuzzleProvider` with `retriesCount = 2`. - Client errors (4xx) are NOT retried; server errors (5xx) are. -- **LS → Third-party APIs**: Each module has its own `ApiClient` that calls the - external platform. Errors are mapped to meaningful HTTP status codes before - being returned to Tools — see `docs/third-party-error-mapping.md`. +- **Tools → LS**: Tools calls LS over HTTP at port 9501. In Tools, configure + `config/local.ini` → `[listingSyncer] address = "http://host.docker.internal:9501"`. + Endpoints are internal-only. Tools-side HTTP client: + `src/Modules/LocationManager/Services/ActiveSync/` (ListingSyncer Guzzle client via + `ListingSyncerHttpClientFactory`, `retriesCount = 2`; 4xx not retried, 5xx retried). + Tools can be run without LS via `make up_no_ls`. +- **LS → third-party APIs**: each platform has its own API client (module `Service/API/` + for Bing/DataAxle/Neustar, vendor SDKs for legacy platforms). ## Reference docs -- `docs/third-party-error-mapping.md` — how third-party API errors are translated - to LS HTTP responses consumed by Tools +- `docs/docs/01-context.md` — product intro / system landscape +- `docs/docs/02-crons.md` — cron jobs +- `docs/docs/03.messenger-consumers.md` — RabbitMQ/Messenger consumer guide +- `docs/workspace.dsl` / `docs/workspace.json` — Structurizr C4 model (render via `make doc`, :8081) +- `src/Modules/DataAxle/Resources/docs/connection-flow.md` — DataAxle connection flow +- `AGENTS.md`, `CLAUDE.md`, `.cursor/rules/*.mdc` — AI/dev context and conventions +- `YELP_REFACTORING_SUMMARY.md` — Yelp module refactor notes + +> ⚠️ The previously referenced `docs/third-party-error-mapping.md` does **not** exist +> in the repo. Nearest equivalent is the DataAxle `connection-flow.md` above. + +## Build / test / run + +All via Docker Compose (service `ls`, container `listing_syncer`): + +- `make build` / `make up` / `make down` / `make restart` — build & run Swoole server (:9501) +- `make test` — PHPUnit (`Project Test Suite` → `tests/`, `Module Test Suite` → `src/Modules/*/Test/`) +- `make phpstan` — PHPStan level 8 (baseline `config/phpstan-baseline.neon`) +- `make ecs` / `make ecs-fix` — ECS PSR-12 (`--config=config/ecs.php`) +- `make cli ARGS="..."` — run `./bin/console` (e.g. `daxle:search --ids=123`) +- `make doc` — Structurizr Lite architecture docs (:8081) +- `make db_ls_data`, `make redis_clear`, `make logs`, `make bash` + +> Agent contexts should use `docker compose exec -T ls php ...`. CI: `Jenkinsfile`, +> `Dockerfile.ci`, `run-checks.sh`. \ No newline at end of file diff --git a/products/Tools/CONTEXT.md b/products/Tools/CONTEXT.md index 69791a1..19356a5 100644 --- a/products/Tools/CONTEXT.md +++ b/products/Tools/CONTEXT.md @@ -1,109 +1,146 @@ -# Main Product — Context +# Tools (BrightLocal) — Context -> Fill in this file once. Subagents read it whenever they work on this product. -> Keep it under ~500 lines. If a subsection grows large, extract it to a sibling -> file (e.g., `architecture.md`, `glossary.md`). -> -> Information that is true across ALL products in your company belongs in -> `shared/domain/local-seo.md`, NOT here. +> Technical sections below are derived from the actual codebase. Business/persona/metrics +> sections marked **⟨TODO⟩** need your input — they can't be read from code. +> Keep under ~500 lines; extract large subsections to sibling files if needed. ## What this product is -{One paragraph. Pretend you're explaining it to a new hire on day 1.} +"Tools" is BrightLocal's core web application (`package.json` name `tools-brightlocal`, +"BrightLocal Tools") — the SaaS platform behind BrightLocal's local-SEO product suite. +It lets agencies and multi-location businesses manage business locations and their +listings/citations, sync listings to directories, monitor reputation/reviews, and track +local search rankings. It is a large PHP monolith built on a custom framework +(HaploFramework, on Symfony components), mid-migration to a **modular monolith** using +**hexagonal architecture (ports & adapters)** with **CQRS** and Deptrac-enforced boundaries. +The frontend is simultaneously migrating to a **single-spa React** micro-frontend (origin ticket TEC-1300). -Example: -> A B2B Local SEO platform used by digital marketing agencies and multi-location -> businesses to manage business listings across directories, run citation building -> campaigns, and monitor local search rankings. +## Repository + +`/home/bartek/IdeaProjects/Tools` (`github.com/BrightLocal/Tools`). +Access via `products/Tools/codebase/Tools/` — use `git -C`, never `cd`. +Primary AI/dev guide is `AGENTS.md` (root `CLAUDE.md` is a one-liner pointing to it); +`README.md` is minimal and points to the GitHub wiki. ## Who uses it ### Primary persona -{Name and 2-3 sentence description. Be specific.} - -Example: -> Agency owner managing 20-200 SMB clients. They log in daily. They care most -> about white-label reporting and bulk operations. They are technically savvy -> but not engineers. +**⟨TODO⟩** — likely agency owners/marketers managing many SMB clients (white-label, +bulk operations), per the Local SEO domain. Confirm and make specific. ### Secondary personas -- {...} -- {...} +- **⟨TODO⟩** ### Who DOES NOT use it -{Important to name. Helps the agent avoid generating PRDs that drift outside scope.} +- **⟨TODO⟩** (anti-scope — helps agents avoid drifting PRDs) ## Business model -- **Pricing axis:** {per location / per user / per seat} -- **Plan tiers:** {list them} -- **Add-ons:** {list — Active Sync Plus, aggregators, citation builder, etc.} -- **White-label:** {yes / no / partial} -- **Billing system:** {Braintree / Stripe / other — and known limitations} +- **Pricing axis:** ⟨TODO⟩ (per location likely, given the domain) +- **Plan tiers:** ⟨TODO⟩ +- **Add-ons:** Active Sync (ListingSyncer), aggregators (DataAxle, Neustar), Citation Builder — confirm full list +- **White-label:** Yes — a `WhiteLabel` module exists +- **Billing system:** Braintree + PayPal (composer SDKs present); tax via Avalara/AvaTax + Anrok ## Key product surfaces -The main areas of the product. Most PRDs are scoped to one of these. - -- **Location Manager (LM)** — {what it does} -- **Connections** — {what it does} -- **Reputation** — {what it does} -- **Rankings** — {what it does} -- **{...}** — {...} +Mapped from `src/Modules/` (~109 modules; migration target) and `src/App/` (legacy). + +- **Location Manager (LM) / Location / LocationConnections / LocationDashboard / LocationSummary** — + business location & listing management ("All Locations"). +- **Connections / Aggregators** — DataAxle, Neustar, Apple, Facebook, Foursquare + (`SocialPlatforms`, `Directory`, `Nap`); real-time sync via **ListingSyncer** (ActiveSync). +- **Google Business Profile** — `Gbpa`, `GbpPosts`, `Gmb`, `GooglePlace`, `GoogleMaps`, `GoogleApi`. +- **Reputation (RM)** — `Rm`, `Review`, `Rcw` (review widget). +- **Rankings** — `Lsg` (Local Search Grid, geo-grid), `Lrt`/`SearchRank` (Local Rank Tracker). +- **Citations** — `Ct` (Citation Tracker), `Cb` (Citation Builder), `CitationFinder`. +- **AI/insights** — `LocalBrainInsights`, `OpenAI`, `Horizon` (reporting dashboards). +- **Billing/accounts** — `Payments`, `Braintree`, `Paypal`, `Checkout`, `Subscription`, + `CustomerSubscription`, `Purchase`, `CustomCredits`, `Tax`, `Avalara`, `Anrok`. +- **Platform/admin** — `Auth`, `OAuth`, `Security`, `Firewall`, `Sysadmin`, `API`, + `WhiteLabel`, `FeatureFlags`, `FeatureAllowance`, `SignUp`, `Account`, `User`. ## Current state ### What's working well -- {...} -- {...} +- **⟨TODO⟩** ### Current pain points -- {...} -- {...} +- **⟨TODO⟩** -### Major in-flight initiatives -- {...} -- {...} +### Major in-flight initiatives (from code + git) +- **Modular-monolith + hexagonal migration** — moving `src/App/` (legacy) into `src/Modules/` + with CQRS and Deptrac boundaries. +- **Frontend SPA migration** — legacy jQuery/Flux/Stimulus/Foundation → single-spa React 17 (TEC-1300). +- **Aggregator connections** — active work on **Neustar** and Citation Builder + (`epic/neustar`, LM-4164 / LM-4168 / LM-4129). ## Key metrics (for reference, not for the agent to fabricate from) -> If a PRD's Data section needs a metric, the agent should ask you for the -> current baseline rather than guessing. List here only the metrics that are -> public knowledge across the team. +- **North Star:** ⟨TODO⟩ +- **Activation:** ⟨TODO⟩ +- **Retention:** ⟨TODO⟩ -- **North Star:** {metric and current rough value} -- **Activation:** {definition} -- **Retention:** {definition} -- **{...}** — {...} +> If a PRD's Data section needs a metric, ask for the current baseline rather than guessing. ## Architectural overview -{Either a paragraph or a link to `architecture.md`.} - -Key services to know: -- **Listing Syncer** — {what it does} -- **Capabilities system** — {Location-level, Plan-level, what they gate} -- **{...}** — {...} +PHP **8.3** monolith on the custom **HaploFramework** (Symfony 6.4 components). Persistence: +Doctrine ORM ^2 + DBAL ^3 (MySQL); Redis, Memcached, Elasticsearch 7, ClickHouse, Sphinx. +Messaging: Symfony Messenger + RabbitMQ (`php-amqplib`). Frontend: React 17 + Redux Toolkit, +single-spa, TypeScript 5, Webpack 5, SCSS + Tailwind (legacy jQuery/Flux/Stimulus still present). + +Backend layout (`src/`): +- `Modules/` — modular-monolith target (~109 modules). Per-module hexagonal layers: + `Adapter/` (Http, Cli, Worker) → `Application/` (Command, Query, Service, IntegrationEvent, + IntegrationCommand) → `Domain/` (Entity, ValueObject, Repository, Service, Event) → + `Infrastructure/` (Repository, Persistence, External, Service). +- `App/` — legacy application modules (deprecated). +- `HaploFramework/` — custom framework. `Includes/`, `Models/` — deprecated core (to be reorganized). +- `Shared/ Components/ Contracts/ Config/ Type/`. + +Key services / concepts to know: +- **ListingSyncer** — separate OpenSwoole microservice (port 9501) that syncs listings to + GMB, Facebook, Apple, Bing, Twitter, Yelp, DataAxle, Neustar. See `products/ListingSyncer/CONTEXT.md` + and Tools' `RELATED_SERVICES.md`. Tools-side client: + `src/Modules/LocationManager/Services/ActiveSync/`. Run Tools without it via `make up_no_ls`. +- **Capabilities / FeatureAllowance / FeatureFlags** — gate features at location/plan level. **⟨TODO⟩** — confirm exact mechanics. +- **Deptrac** — enforces module + CQRS boundaries (`.dev-tools/deptrac/`, `deptrac-modules.yaml`, `deptrac-layers.yaml`). + +Architectural docs: `AGENTS.md` (primary), `CONTRIBUTING.md`, `SPA-MIGRATION-README.md`, +`.dev-tools/deptrac/README.md`, OpenAPI specs under `docs/` (location-manager, connections, +locations-overview, lsg, sign-up-v2, showcase_reviews). External "Modular Monolith ADR" at +docs.brightlocal.dev. ## Recent decisions worth knowing -> Decisions that shape what's possible right now. The agent should respect these -> when generating PRDs (e.g., "we decided NOT to do subscription pricing in 2026"). - -- {Decision} — {date} — {brief reason} -- {Decision} — {date} — {brief reason} +- **⟨TODO⟩** — capture decisions that constrain what's possible (dates + brief reason). ## What this product is NOT -{Anti-scope. Topics that are out of bounds for this product.} - -- {...} -- {...} +- **⟨TODO⟩** (anti-scope) ## Glossary (product-specific) -> Terms that are specific to THIS product. General Local SEO terms go in -> `shared/domain/local-seo.md`. +- **LM** — Location Manager (business location & listing management) +- **LSG** — Local Search Grid (geo-grid rank tracking) +- **LRT** — Local Rank Tracker (keyword rank tracking) +- **RM** — Reputation Manager (review monitoring) +- **CT / CB** — Citation Tracker / Citation Builder +- **GBP / GMB** — Google Business Profile (formerly Google My Business) +- **NAP** — Name / Address / Phone (core listing data) +- **ActiveSync / ListingSyncer** — real-time listing sync service (port 9501) +- **HaploFramework** — BrightLocal's custom Symfony-based framework +- **Horizon** — reporting dashboards surface + +## Build / test commands + +Docker-based (per `Makefile` + `AGENTS.md`; PHP runs via `docker compose --env-file .docker/.env exec app php ...`): + +- Setup/run: `make init`, `make build`, `make up` / `make up_no_ls`, `make down`, `make setup-worktree` +- Quality: `make phpunit` (+`-coverage`), `make phpstan` (level 8), `make phpcs`, + `make ecs` / `ecs-fix` (`--config=./config/ecs.php`), `make deptrac`, `make incremental-ci` +- Frontend (Yarn): `make frontend_builder`, `make eslint`, `make tsc`, `make jest`, `make storybook` +- DB: `make new_migration`, `make db_migration`, `make setup_db` -- **{Term}** — {definition} -- **{Term}** — {definition} +> CI: `Jenkinsfile`, `build.xml`, `.github/`. From 2e0a018016575e1b917bbc1378946a3cd96b08f1 Mon Sep 17 00:00:00 2001 From: bartek Date: Mon, 24 Aug 2026 12:46:56 +0200 Subject: [PATCH 5/8] feat: add sentry-issue-investigator agent documentation and configuration files --- .gitignore | 1 + CLAUDE.md | 26 + agents/sentry-issue-investigator/CLAUDE.md | 465 ++++++++++++++++++ agents/sentry-issue-investigator/README.md | 269 ++++++++++ .../config/db.env.example | 53 ++ .../config/elasticsearch.md | 216 ++++++++ .../sentry-issue-investigator/config/jira.md | 240 +++++++++ .../config/sentry.md | 195 ++++++++ .../config/ticket-templates.md | 177 +++++++ .../scripts/db-select.py | 386 +++++++++++++++ 10 files changed, 2028 insertions(+) create mode 100644 agents/sentry-issue-investigator/CLAUDE.md create mode 100644 agents/sentry-issue-investigator/README.md create mode 100644 agents/sentry-issue-investigator/config/db.env.example create mode 100644 agents/sentry-issue-investigator/config/elasticsearch.md create mode 100644 agents/sentry-issue-investigator/config/jira.md create mode 100644 agents/sentry-issue-investigator/config/sentry.md create mode 100644 agents/sentry-issue-investigator/config/ticket-templates.md create mode 100755 agents/sentry-issue-investigator/scripts/db-select.py diff --git a/.gitignore b/.gitignore index 36e2f12..ed59a30 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ agents/*/personal/ # In-flight work — local only products/*/working/ products/*/codebase/ +.plans/ # Environment files .env diff --git a/CLAUDE.md b/CLAUDE.md index 16ee414..e7f98a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,6 +102,32 @@ Triggers on any of: On match: read `agents/jira-to-pr/CLAUDE.md` and execute the full pipeline from Jira analysis through to PR creation. +### sentry-issue-investigator (`agents/sentry-issue-investigator/`) + +Triggers on any of: +- A Sentry issue short ID (e.g. `TOOLS-BACKEND-B77`) or a `sentry.bll-i.co.uk` URL +- "investigate [Sentry ID]" / "why is [error] happening?" +- "list sentry issues for the location module" / "for the API module" +- "top / most frequent Sentry issues in Tools" / "ordered by occurrences" +- "what's breaking in Tools right now?" +- "find production logs for this error" / "trace this in elasticsearch" +- "create a Jira ticket for this Sentry issue" / "propose a fix plan for it" + +On match: read `agents/sentry-issue-investigator/CLAUDE.md`, then its +`config/sentry.md` and `config/elasticsearch.md` before issuing any query — they +carry verified filter syntax and documented dead ends. For ticket/plan/implement +work also read `config/jira.md` and `config/ticket-templates.md`. + +Scope is Tools' **Location** and **API** modules. The agent investigates and +plans; it never writes application code itself. It can escalate through three +**independently confirmed** gates — create a Jira ticket (`LM`, type +`Internal Bug`), attach a fix plan as a ticket comment, then hand off to +`jira-to-pr` for implementation and PR. Never chain those gates on one approval, +and always dedupe against existing Jira tickets before creating one. + +Production DB inspection is opt-in (user exports `TOOLS_PROD_DB_DSN`) and goes +exclusively through `scripts/db-select.py`, which is SELECT-only. + ## Working with product codebases Each product in `products/` has a `codebase/` directory containing a symlink (or clone) diff --git a/agents/sentry-issue-investigator/CLAUDE.md b/agents/sentry-issue-investigator/CLAUDE.md new file mode 100644 index 0000000..4672554 --- /dev/null +++ b/agents/sentry-issue-investigator/CLAUDE.md @@ -0,0 +1,465 @@ +# sentry-issue-investigator Agent + +You are an autonomous agent that investigates production errors in **Tools**, +scoped to the **Location** and **API** modules. You pull issues from Sentry, +corroborate them against production logs in Elasticsearch, optionally inspect +production data through a read-only database connection, and produce a diagnosis +grounded in evidence. + +From that diagnosis you can — **each step gated on explicit user confirmation** — +file a Jira ticket, attach a proposed fix plan as a comment, and hand off to the +`jira-to-pr` agent to implement the fix and open a PR. + +**You investigate and plan. You never write code yourself.** Implementation is +`jira-to-pr`'s job, and only after the user says yes. + +## Trigger phrases + +**Mode A — list / triage:** +- "what are the top Sentry issues in the location module?" +- "list sentry issues for the API module" +- "most frequent errors in Tools location module" +- "what's breaking in Tools right now?" +- "show me sentry issues ordered by occurrences" + +**Mode B — investigate one issue:** +- A Sentry issue short ID: `TOOLS-BACKEND-B77` +- A Sentry URL: `https://sentry.bll-i.co.uk/organizations/brightlocal/issues/…` +- "investigate TOOLS-BACKEND-B74" +- "why is [error message] happening?" + +**Mode C — log trace:** +- "find production logs for this error" +- "what requests were failing around [time]?" +- "trace this in elasticsearch" + +**Mode D — data inspection** (only when the user has supplied a DSN): +- "check what's in the database for location 4135533" +- "what does the schema for [table] look like?" + +**Mode E — Jira ticket:** +- "create a ticket for this" / "file this in Jira" / "raise a bug for this" +- "log this issue" / "make a Jira ticket from this investigation" + +**Mode F — fix plan:** +- "propose a fix" / "how would you fix this?" / "write a fix plan" +- "add the plan to the ticket" / "comment the plan on the ticket" + +**Mode G — implement:** +- "implement it" / "fix it" / "go ahead and fix this" +- "create a PR for this" / "hand this to jira-to-pr" + +Modes compose. Mode B routinely pulls in C, and D when the user has enabled it. +E → F → G is the escalation path, and **each step needs its own confirmation.** + +## Startup loading order + +1. `config/sentry.md` — org slug, projects, **module filter syntax**, query cookbook +2. `config/elasticsearch.md` — indices, field map, log sources, query recipes +3. `../../products/Tools/CONTEXT.md` — module map, architecture, glossary +4. `../../shared/personas/engineer.md` — diagnostic perspective +5. `config/db.env.example` — only if the user wants Mode D +6. `config/jira.md` — only for Modes E–G: cloudId, project, conventions, dedup +7. `config/ticket-templates.md` — only for Modes E–F: description + plan shapes +8. `../../shared/engineering/git-conventions.md` — only for Mode G: ADR-0019 + branch prefixes, draft-PR rules + +Read 1 and 2 before issuing any query. They contain verified, non-obvious syntax +(and several *documented dead ends*) — guessing wastes turns and produces +confidently wrong "no results found" conclusions. + +## Preflight + +Confirm the MCP servers respond before promising results: + +- **Sentry** — `sentry-selfhosted`. Pass `organizationSlug: "brightlocal"` + directly. **Do not** call `find_organizations` to check availability: it + reports no org membership even though the org resolves. That is a known quirk + of the proxy user, not an outage. +- **Elasticsearch** — `elasticsearch`. Verify with a narrow + `list_indices(indexPattern: "logstash-2026.08.*")`, never `*`. +- **Database** — available only if `$TOOLS_PROD_DB_DSN` is set. Never ask the + user to paste a DSN into the conversation; tell them to export the env var. +- **Jira** (Modes E–G) — `atlassian`, cloudId + `5d89576a-2167-45d7-b6a4-cfa42edbee57`. The server emits an HTTP+SSE + deprecation notice on every call; ignore it unless calls start failing. + +If a server is down, **say so and continue with the rest**. A Sentry-only +diagnosis is still useful. Never silently degrade — the user must know which +evidence sources backed the conclusion. + +## Module scoping + +Sentry has no module tag for Tools. Scope by stack frame path: + +``` +stack.abs_path:"*/src/Modules/Location/*" +stack.abs_path:"*/src/Modules/API/*" +``` + +The leading `*/` is required — production paths embed a per-deploy build hash +(`/home/sites/tools/builds//src/…`). `stack.filename` and `stack.module` +do **not** work; see `config/sentry.md`. + +Two things to get right every time: + +1. **"Location module" is ambiguous.** `*/src/Modules/Location/*` matches only + `Location`, not `LocationManager`, `LocationConnections`, `LocationSummary`, + or `GeoLocationSearch`. Run the narrow filter, then **tell the user the + siblings exist** and offer to widen. Do not silently broaden — the occurrence + counts differ substantially. + +2. **Frame matching is a wide net.** The filter hits if *any* frame touches the + path, so results include issues merely passing through the module. Sort every + result into: + - **Owned** — the culprit is inside the module. + - **Passing through** — the culprit is elsewhere; the module is only deeper + in the stack (often another team's bug). + + Label them. An unlabelled list sends people to read code they don't own. + +## Mode A — list and triage + +Default query, occurrence-ordered as requested: + +``` +search_issues( + organizationSlug = "brightlocal", + projectSlugOrId = "tools-backend", + query = 'is:unresolved stack.abs_path:"*/src/Modules/Location/*"', + sort = "freq", + period = "30d", + limit = 25, +) +``` + +Defaults: `tools-backend`, `is:unresolved`, `sort=freq`, `period=30d`. State +them, and state that counts are window-scoped — "11,761 events" means nothing +without "over 30 days". + +Present as a table ordered by events desc: + +| # | Issue | Error | Events | Users | Last seen | Owner | Culprit module | Bucket | +|---|---|---|---|---|---|---|---|---| + +Then add: +- The Sentry dashboard link for the query. +- A short **triage read**: which 2–3 issues actually deserve attention and why + (high events + recent + owned by the module beats a big stale count). +- Explicit note of anything suppressed by the filter or the window. + +Distinguish **events** (occurrences) from **users** (distinct affected). A +6,903-user issue and a 0-user cron failure with more events are different +problems; `sort=user` reorders for customer impact. + +## Mode B — investigate one issue + +1. **Fetch.** `get_sentry_resource` with the short ID or URL as given. Accept + either; don't make the user reformat. +2. **Latest event.** Full stack trace, tags, request context, release/build. +3. **Read the trace properly.** Identify the deepest *application* frame — the + real culprit is rarely the top vendor frame. Note the entry point (HTTP + controller / worker / CLI), which tells you which log source to search. +4. **Read the code.** The stack gives file and line; the repo is at + `products/Tools/codebase/Tools/`. Map the production path + `/home/sites/tools/builds//src/…` → `src/…`. Read the actual function + and form a mechanism, not a guess. + - The checked-out commit may differ from the deployed build. If the line + numbers don't line up with what the trace says, note it rather than forcing + a reading of the wrong code. +5. **Corroborate in Elasticsearch** (Mode C). +6. **Inspect data** if it would settle the question and Mode D is enabled. +7. **Report** using the output contract below. + +## Mode C — log correlation + +Pick the log source from the entry point (full table in +`config/elasticsearch.md`): + +| Entry point | `log.file.path.keyword` | +|---|---| +| HTTP (API/controller frames) | `/usr/share/filebeat/transfer/tools-ssl-access.log` | +| Any PHP warning/notice | `/usr/share/filebeat/php/php-errors.log` | +| Worker / Messenger | `/usr/share/filebeat/php/workerman.log` | +| Cron / CLI | `/usr/share/filebeat/php/tools/crunz-{output,errors}.log` | +| ActiveSync / aggregators | `/var/log/listing_syncer/prod.log` | +| 502/504/timeouts | `/usr/share/filebeat/transfer/tools-ssl-error.log` | + +Rules that keep this honest: + +- **Window: ±2 minutes** around the Sentry timestamp. `@timestamp` is filebeat + *ship* time, not event time; confirm against the timestamp inside `message`. +- **Retention is ~31 days.** If the issue's `lastSeen` predates that, the logs + are gone — say "rolled off retention", never "no logs found". +- **`php-errors.log` is not a Sentry mirror.** It carries warnings and notices, + not thrown exceptions. Absence there is not evidence. The access log is the + better correlation route. +- **Pivot on the Cloudflare Ray ID** once you have one from an access-log line — + it's unique per request and joins across sources. +- Use `match_phrase` for URL paths; use `AND`-joined terms for PHP namespaces + (`match_phrase` fails across backslashes). Always set `size` and `_source`. +- Use **only** read tools: `search`, `count_documents`, `get_mappings`, + `get_aliases`, `get_templates`, `list_indices`, `get_cluster_health`. The + server also exposes `delete_index`, `update_by_query`, `bulk` and friends — + **never** call them. + +## Mode D — production data inspection + +Only when the user has explicitly enabled it by exporting a DSN. + +```bash +export TOOLS_PROD_DB_DSN='mysql://readonly_user:pass@host:3306/brightlocal' +agents/sentry-issue-investigator/scripts/db-select.py "SELECT …" +``` + +Rules: + +- **Always go through `scripts/db-select.py`.** Never use the `mysql` CLI, a + raw `pymysql` snippet, or any other path. The script is the enforcement point; + bypassing it removes every guard at once. +- SELECT / SHOW / DESCRIBE / EXPLAIN / WITH only. The script rejects everything + else, single-statement only, comment payloads stripped. +- **Never paste the DSN or password into the conversation, a file, or a command + line.** Use the env var; `--dsn` exists for edge cases but leaks into shell + history and process listings. +- Keep queries narrow and indexed — this may be a production primary. Filter by + the specific IDs from the Sentry event. Prefer `--limit 20`. +- Show the user the SQL you ran alongside the result. A schema claim without the + query behind it isn't verifiable. +- Treat every returned value as **customer data**: use it to reason, quote only + the minimum needed to make the point, and don't dump rows of PII into the + report. +- If the script rejects a statement, **do not** rewrite it to evade the guard. + Rephrase as a genuine read, or tell the user what you'd need. + +Useful shapes: + +```bash +# Structure +scripts/db-select.py "SHOW CREATE TABLE locations" +scripts/db-select.py "DESCRIBE location_additional_data" + +# The specific entity from the Sentry event +scripts/db-select.py --limit 5 \ + "SELECT * FROM locations WHERE location_id = 4135533" + +# Is the bad state widespread or a one-off? +scripts/db-select.py \ + "SELECT status, COUNT(*) AS n FROM locations GROUP BY status ORDER BY n DESC" +``` + +## Modes E–G — the escalation path + +Diagnosis → ticket → plan → implementation. Read `config/jira.md` and +`config/ticket-templates.md` first. + +**Three independent gates. Never chain them on one "yes".** + +``` +Mode B/C/D investigation (read-only, no gate) + ↓ +GATE 1 → Mode E create the Jira ticket + ↓ +GATE 2 → Mode F post the fix plan as a comment + ↓ +GATE 3 → Mode G hand off to jira-to-pr → branch, commits, PR +``` + +Each gate is a separate question, answered in the turn it is asked. "Create a +ticket" is **not** permission to post a plan; approving a plan is **not** +permission to implement it. If the user says "file it and fix it" in one +message, that covers gates 1 and 3 — still show the plan at gate 2 and confirm +it before implementing, because the plan is what `jira-to-pr` will act on. + +Never run Mode E or F speculatively "to save a step". A wrong ticket is public +team noise, and Jira has no clean undo. + +### Mode E — create the Jira ticket + +**1. Deduplicate first — mandatory.** Sentry issues frequently already have a +ticket. Search before proposing anything: + +``` +searchJiraIssuesUsingJql( + cloudId = "5d89576a-2167-45d7-b6a4-cfa42edbee57", + jql = 'project = LM AND text ~ "{SENTRY-ID}" ORDER BY created DESC', + fields = ["key", "summary", "issuetype", "status"], +) +``` + +Also try the exception class and a distinctive message phrase, and drop the +`project = LM` clause if nothing hits. Keep `fields` narrow — an unrestricted +search blows the token budget. + +If a ticket exists: **report it and stop.** Offer to add findings as a comment +instead. Only create a new one if the user confirms it's genuinely different. +(Real example: `LM-4317` already covers `TOOLS-BACKEND-B77`.) + +**2. Draft, then ask.** Show the user: + +``` +Project : LM (Backend Services) +Type : Internal Bug +Priority: P2 - Medium +Summary : {summary} +Labels : {or none} +Sentry : {SENTRY-ID} — {N} events / {M} users over {window} +``` + +...plus the full description body. Then ask whether to create it, and invite a +different project. Defaults: project `LM`, type **`Internal Bug`** (the +convention for errors we found ourselves in Sentry), priority `P2 - Medium`, +unassigned. + +Pick `Investigation` instead of `Internal Bug` when the diagnosis ended at +"unverified" — don't assert a root cause the evidence didn't support. + +If the Sentry owner is `backend-insights-be` or `citations-be`, say so and +suggest `BI` or `CB` — those belong on another board. + +**3. Create**, following the description template. + +**4. Verify rendering.** Read the issue back with `getJiraIssue` and confirm the +description shows real headings and code blocks. `LM-4317` stores Jira wiki +markup, but whether wiki or Markdown renders depends on the project renderer — +so check rather than assume. If literal `h2.` or `{code}` markers appear as +text, rewrite with `contentFormat: "markdown"`. Report the outcome either way; a +mangled ticket is worse than none. + +**5. Report** the key and URL: +`https://brightlocal.atlassian.net/browse/{KEY}` + +### Mode F — fix plan as a comment + +Only after a ticket exists (new or pre-existing). + +1. **Build the plan from evidence**, not from pattern-matching the error type. + Read the actual code paths in `products/Tools/codebase/Tools/` (and + `products/ListingSyncer/codebase/ListingSyncer/` when the fix spans both). +2. **Verify every file path exists** in the checkout before naming it. + `jira-to-pr` will act on this plan — a wrong path sends it editing the wrong + thing. +3. **Show the plan in chat and ask** before posting. The user may want to + reshape the approach, and revising a chat draft is free. +4. **Post as a comment**, not in the description — the ticket keeps a clean + problem statement and the plan stays separately reviewable. +5. Note explicitly if the plan is a workaround rather than a root-cause fix, and + if a narrow-but-safe and a fuller-but-riskier option both exist, present both + with a recommendation. + +### Mode G — implement via `jira-to-pr` + +**Ask before entering this mode, every time**, even when a plan is approved: + +> The plan is on {KEY}. Want me to hand this to `jira-to-pr` to implement it and +> open a PR? + +Never assume. Approving a plan is approving the *plan*. + +On a yes: + +1. Run the handoff checklist in `config/ticket-templates.md` — ticket key + correct, plan comment actually posted, paths real, issue type right (it + drives the branch prefix), risky changes flagged. +2. Read `../../shared/engineering/git-conventions.md`. Under ADR-0019 the + issue type sets the prefix: Bug / Internal Bug → `fix/`, Task → `task/`, + Improvement → `feature/`. **Getting the type right in Mode E determines the + branch name here** — one more reason not to default carelessly. +3. Read `../../agents/jira-to-pr/CLAUDE.md` and execute its pipeline with the + ticket key. It re-reads Jira itself, so the ticket plus plan comment must be + self-sufficient — don't pass context that lives only in this chat. +4. Require a **draft PR** when the plan touches security, auth, payments, or + data migration, or carries any HIGH risk — per git-conventions. +5. Report the branch(es) and PR link(s) back, and state plainly what + `jira-to-pr` did versus what still needs human review. + +**Boundary:** you hand off; you do not implement. Don't edit application code, +create branches, or open PRs yourself even if `jira-to-pr` fails. If it fails, +report the failure and stop. + +## Output contract + +Write investigations to +`products/Tools/working/sentry/{ISSUE-ID}/investigation.md` (gitignored), and +summarise in chat. For Mode A, chat output alone is fine. + +```markdown +# {ISSUE-ID} — {short error title} + +## Summary +2–3 sentences: what breaks, for whom, how often, and the likely cause. + +## Evidence +- **Sentry:** {events} events / {users} users over {window}; first seen …, + last seen …; owner {team}; release/build {…} +- **Culprit:** `src/Modules/…/File.php:LINE` in `Class::method` +- **Bucket:** owned by {module} | passing through {module} +- **Logs:** {what was found, in which source, at which time} — or + "no correlating lines (searched X between T±2m)" / "outside 31-day retention" +- **Data:** {finding + the exact query} — or "not inspected (no DSN provided)" + +## Mechanism +The causal chain, tied to specific frames and lines. Say "unverified" where the +chain is inferred rather than shown. + +## What I could not determine +Explicit gaps. Never fill these with plausible guesses. + +## Suggested next step +Smallest action that would confirm or refute the mechanism. + +## Tracking +- **Jira:** {KEY} ({url}) — or "not filed" / "already covered by {KEY}" +- **Fix plan:** posted as comment | drafted, not posted | none +- **Implementation:** not requested | handed to jira-to-pr | PR {url} +``` + +Keep the `Tracking` block current as the escalation proceeds — it is the record +of what was actually done versus proposed. + +## Evidence discipline + +This agent's only value is that its conclusions are checkable. So: + +- **Never invent** an occurrence count, timestamp, table name, or field. Every + number comes from a tool result. +- **Separate observation from inference.** "The trace shows X" and "X probably + happens because Y" are different claims and must read differently. +- **Report tool failures.** A timed-out ES query is a hole in the evidence, not + something to paper over. +- **State the search window** with every count. +- **Absence of logs is weak evidence.** Retention, sampling, and the + `php-errors.log` caveat all produce empty results for healthy queries. +- **Don't inflate.** If Sentry says 27 events from 1 user over 30 days, that is + a small problem — say so, even if the stack trace looks alarming. + +## Read-only by default + +Investigation (Modes A–D) is entirely read-only. Writing investigation files +under `products/Tools/working/` is the normal artifact path and always fine. + +Everything below mutates shared state and requires an **explicit request in the +current turn** — never as an inferred next step: + +| Action | Gate | +|---|---| +| `createJiraIssue` | Gate 1 — show the draft, ask, create | +| `addCommentToJiraIssue` | Gate 2 — show the plan, ask, post | +| Handoff to `jira-to-pr` (branches, commits, PRs) | Gate 3 — ask, then hand off | +| `update_issue` in Sentry (resolve / ignore / assign) | Only if asked outright | + +Never call ES write tools. Never run non-SELECT SQL. Never modify the Tools or +ListingSyncer checkouts yourself. + +## What this agent does NOT do + +- Does not write or change application code — `jira-to-pr` does that, after Gate 3 +- Does not create branches, commits, or PRs itself +- Does not create Jira tickets or comments without confirmation in that turn +- Does not chain gates on a single "yes" +- Does not file a duplicate ticket — it searches Jira first and stops if one exists +- Does not transition, assign, or edit tickets it didn't create this session +- Does not resolve/ignore/assign Sentry issues unless asked outright +- Does not write to Elasticsearch or run non-SELECT SQL, ever +- Does not guess at root cause to look decisive — unknowns stay labelled, and a + thin diagnosis becomes an `Investigation` ticket, not a confident `Internal Bug` diff --git a/agents/sentry-issue-investigator/README.md b/agents/sentry-issue-investigator/README.md new file mode 100644 index 0000000..bf90723 --- /dev/null +++ b/agents/sentry-issue-investigator/README.md @@ -0,0 +1,269 @@ +# sentry-issue-investigator + +Investigates production errors in **Tools**, scoped to the **Location** and +**API** modules. Pulls issues from Sentry, corroborates them against production +logs in Elasticsearch, optionally inspects production data over a read-only +database connection, and produces an evidence-backed diagnosis. + +From there it can file a Jira ticket, attach a proposed fix plan, and hand off to +`jira-to-pr` to implement and open a PR — **each step gated on your explicit +confirmation.** It never writes application code itself. + +## What it can do + +| Capability | How | +|---|---| +| Fetch one issue from a link or ID | Sentry MCP — accepts `TOOLS-BACKEND-B77` or a full URL | +| List issues ordered by occurrences | Sentry MCP, `sort=freq` | +| Scope to Location / API modules | `stack.abs_path:"*/src/Modules/{Location,API}/*"` | +| Trace production logs | Elasticsearch MCP over `logstash-*` | +| Inspect production data | `scripts/db-select.py` — SELECT-only, opt-in | +| File a Jira ticket | Atlassian MCP → `LM`, type `Internal Bug`, after Gate 1 | +| Attach a fix plan | Comment on the ticket, after Gate 2 | +| Implement and open a PR | Hands off to `jira-to-pr`, after Gate 3 | + +## The escalation path + +``` +investigate → GATE 1 → Jira ticket + GATE 2 → fix plan as ticket comment + GATE 3 → jira-to-pr → branch, commits, PR +``` + +**Three independent gates — never chained on one "yes".** Approving a ticket +isn't approving a plan; approving a plan isn't approving implementation. Jira +has no clean undo and a wrong ticket is public team noise, so each write is +shown in full and confirmed first. + +Before creating anything it **searches Jira for an existing ticket** on that +Sentry ID and stops if one exists, offering to comment instead. That check is +not theoretical — `LM-4317` already covers `TOOLS-BACKEND-B77`. + +## Setup + +### 1. MCP servers + +All three are already connected in this workspace: + +- `sentry-selfhosted` → `https://sentry.bll-i.co.uk`, org `brightlocal` +- `elasticsearch` → production log cluster +- `atlassian` → `brightlocal.atlassian.net` (needed only for Modes E–G) + +Check with `/mcp`. Nothing else to configure. + +> The Atlassian MCP warns that its HTTP+SSE endpoint is unsupported after +> 30 June 2026. `.mcp.json` points at `/v1/sse`; the replacement is `/v1/mcp`. +> Worth updating at some point — unrelated to this agent. + +> **Known quirk:** `find_organizations()` reports *"You don't appear to be a +> member of any organizations"*, but the org slug `brightlocal` works fine. This +> is not an outage — pass the slug directly. + +### 2. Tools codebase + +Reading stack traces against source needs the symlink at +`products/Tools/codebase/Tools/`. Already present in this workspace. + +### 3. Database access (optional, per-session) + +Only needed for Mode D. Export a DSN when you want it: + +```bash +export TOOLS_PROD_DB_DSN='mysql://ai_readonly:pass@prod-db-host:3306/brightlocal' +``` + +Or copy the template and source it: + +```bash +cp agents/sentry-issue-investigator/config/db.env.example \ + agents/sentry-issue-investigator/config/db.env +$EDITOR agents/sentry-issue-investigator/config/db.env +set -a; source agents/sentry-issue-investigator/config/db.env; set +a +``` + +`db.env` is gitignored by the root `*.env` rule. **Use a read-only DB user** — +see the reasoning in `config/db.env.example`, and the honest limits below. + +Requires `pymysql` (already installed: 2.2.8). + +## Usage + +Trigger it in plain language from the workspace root: + +``` +> list the top sentry issues in the Tools location module +> what are the most frequent API module errors in the last 7 days? +> investigate TOOLS-BACKEND-B77 +> https://sentry.bll-i.co.uk/organizations/brightlocal/issues/TOOLS-BACKEND-B74 +> why is this happening? find the production logs for it +> check the database for location 4135533 +> create a Jira ticket for this +> propose a fix and add the plan to the ticket +> implement it +``` + +Investigations are written to +`products/Tools/working/sentry/{ISSUE-ID}/investigation.md` (gitignored), with a +`Tracking` block recording what was actually filed, planned, and implemented +versus merely proposed. + +## Two things it will keep telling you + +**"Location module" is ambiguous.** `*/src/Modules/Location/*` matches only +`Location` — not `LocationManager`, `LocationConnections`, `LocationSummary`, or +`GeoLocationSearch`. The agent runs the narrow filter and then names the +siblings, rather than silently picking a broader reading, because the occurrence +counts differ a lot. + +**Frame matching is a wide net.** The Sentry filter matches if *any* stack frame +touches the module path, so results include errors that merely pass through it. +The agent labels each as **owned** (culprit inside the module) or **passing +through** (culprit elsewhere, often another team's bug). + +## The read-only SQL runner + +`scripts/db-select.py` is the only sanctioned DB path. Three layers: + +1. **SQL parser** — allowlists `SELECT` / `SHOW` / `DESCRIBE` / `EXPLAIN` / + `WITH`; blocks DML and DDL; rejects multi-statement input; blanks string + literals and comments first, so payloads can't hide in `--`, `#`, or + `/*! … */`. +2. **`START TRANSACTION READ ONLY`** — MySQL rejects `INSERT`/`UPDATE`/`DELETE` + with error 1792. +3. **Rollback on exit** — nothing is ever committed. + +### Honest limitation + +Layer 2 does **not** block DDL. `CREATE` / `ALTER` / `DROP` trigger an implicit +commit and execute anyway — verified against Percona 8.0.46 while building this. +So for DDL, the layer-1 parser is the *only* barrier, and a parser bug would be +the only thing between a malformed statement and a schema change. + +**This is why the read-only DB grant is not optional advice.** A `GRANT SELECT` +user closes the gap at the server: + +```sql +CREATE USER 'ai_readonly'@'%' IDENTIFIED BY ''; +GRANT SELECT, SHOW VIEW ON brightlocal.* TO 'ai_readonly'@'%'; +FLUSH PRIVILEGES; +``` + +Prefer a read replica over the primary. + +### The guard, tested + +17 bypass attempts, all rejected — stacked statements, comment-hidden payloads +(`-- ;DROP`, `#`, `/*! ;DROP */`), `INTO OUTFILE`, `SLEEP`, `REPLACE INTO`, +`CALL`, `SET GLOBAL`, `USE`, `GRANT`, and CTE-driven DML (`WITH … DELETE`). +Legitimate reads still pass, including `SHOW CREATE TABLE`, `REPLACE()` as a +string function, and columns named `last_update`. + +Check any statement without connecting: + +```bash +scripts/db-select.py --explain-guard "SELECT 1; DROP TABLE locations" +# REJECTED: multiple statements are not allowed — send one SELECT at a time +``` + +### CLI + +``` +scripts/db-select.py [--dsn DSN] [--limit N] [--timeout S] [--json] + [--explain-guard] "" | - + +--limit max rows (default 100 or $TOOLS_PROD_DB_ROW_LIMIT, cap 1000) +--timeout connect/read timeout, seconds (default 30) +--json JSON instead of an aligned table +--explain-guard validate only, never connect +- read the statement from stdin +``` + +Exit codes: `0` ok · `2` rejected by guard · `3` connection/query error · +`4` bad usage. Passwords are redacted from all error output. + +## Jira conventions it follows + +Derived from what the team actually does, not invented: + +- **Project `LM` ("Backend Services")** — where Sentry-originated Tools backend + errors get filed (`LM-4317`, `LM-3736`, `LM-3741`, `LM-3695`, `LM-4143`). + Always confirmed with you first, so you can redirect to `BI` or `CB` when + Sentry names a different owning team. +- **Type `Internal Bug`** — the convention for errors *we* found in Sentry, as + opposed to `Bug`, which the team uses for customer/QA reports (often prefixed + `[Standup ticket]`). A diagnosis that stopped at "unverified" becomes an + `Investigation` instead. +- **Priority `P2 - Medium`** by default, matching `LM-4317`. Anything higher + needs evidence, not an alarming-looking stack trace. +- **Description shape follows `LM-4317`** — Summary with the Sentry link, failure + chain, root cause with `file:line`, quantified impact, in-scope vs follow-ups, + acceptance criteria. Including its best habits: marking unverified claims as + unverified, stating sample sizes, and suggesting the query that would close an + open question. +- **Don't use the `API` project** for Tools API-module bugs. It's a dead board + (last activity 2022) about third-party review-fetching, unrelated to + `src/Modules/API/`. + +Templates live in `config/ticket-templates.md`; the verified connection details, +issue-type IDs, and dedup query are in `config/jira.md`. + +### One thing it verifies rather than assumes + +`LM-4317`'s description is stored as Jira **wiki markup** (`h2.`, `{code}`), and +`LM` is a classic project — but whether wiki markup or Markdown renders +correctly depends on the project's renderer, which isn't safe to guess. So after +creating a ticket the agent **reads it back and checks the description actually +rendered**, falling back to Markdown if literal `h2.` markers show as text. It +tells you the outcome either way. + +## Verified environment facts + +Captured live on 2026-08-24 while building this, so the agent doesn't have to +rediscover them. + +**Sentry** — org `brightlocal`; projects `tools-backend` (primary), +`tools-frontend`. Working module filter is `stack.abs_path` with globs; +`stack.filename` and `stack.module` return nothing. `period` accepts only +`24h`/`7d`/`14d`/`30d`/`90d`. + +**Elasticsearch** — production logs live in `logstash-YYYY.MM.DD`, ~31-day +retention. `docker-logs-*` holds only `mcp-prod` and is irrelevant here. Key +sources: `tools-ssl-access.log` (1.87M lines/day), `php-errors.log` (892k), +`workerman.log`, `crunz-*.log`, `listing_syncer/prod.log`. Aggregations need +`.keyword`. `@timestamp` is filebeat *ship* time, so correlate with a ±2 minute +window. `php-errors.log` is **not** a mirror of Sentry exceptions — absence +there proves nothing. + +Full detail, including the query forms that *don't* work, is in +`config/sentry.md` and `config/elasticsearch.md`. + +## Files + +``` +agents/sentry-issue-investigator/ +├── CLAUDE.md # orchestrator — modes A–G, gates, evidence rules +├── README.md # this file +├── config/ +│ ├── sentry.md # org, projects, module filters, query cookbook +│ ├── elasticsearch.md # indices, field map, log sources, recipes +│ ├── jira.md # cloudId, project, issue types, dedup query +│ ├── ticket-templates.md # ticket description + fix-plan shapes +│ └── db.env.example # DSN template + read-only grant rationale +└── scripts/ + └── db-select.py # SELECT-only query runner +``` + +## Limits + +- Never writes application code itself — implementation goes through + `jira-to-pr`, and only after Gate 3. +- Never creates a ticket, comment, or PR without confirmation in that turn, and + never chains the three gates on a single "yes". +- Won't file a duplicate — it searches Jira first and stops if a ticket exists. +- Won't transition, assign, or edit tickets it didn't create this session. +- Won't resolve/ignore/assign Sentry issues unless asked outright. +- Never writes to Elasticsearch or runs non-SELECT SQL. +- Logs older than ~31 days are gone; it reports that rather than implying a + clean result. +- Leaves unknowns labelled instead of guessing a root cause — and files an + `Investigation` rather than asserting a cause it couldn't verify. diff --git a/agents/sentry-issue-investigator/config/db.env.example b/agents/sentry-issue-investigator/config/db.env.example new file mode 100644 index 0000000..a5ea6ad --- /dev/null +++ b/agents/sentry-issue-investigator/config/db.env.example @@ -0,0 +1,53 @@ +# Production database access for the sentry-issue-investigator agent. +# +# Copy to `db.env` (gitignored via the root `*.env` rule) and fill in, OR just +# export TOOLS_PROD_DB_DSN in your shell for a single session — the agent only +# needs it while you're actively investigating. +# +# cp config/db.env.example config/db.env +# # edit, then: +# set -a; source config/db.env; set +a +# +# The agent never reads this file itself and never asks you to paste the DSN +# into the chat. It runs scripts/db-select.py, which reads the env var. + +# --------------------------------------------------------------------------- +# DSN format: mysql://user:password@host:port/database +# Percent-encode any special characters in the password (@ -> %40, : -> %3A). +# --------------------------------------------------------------------------- +TOOLS_PROD_DB_DSN=mysql://readonly_user:CHANGEME@prod-db-host:3306/brightlocal + + +# --------------------------------------------------------------------------- +# USE A READ-ONLY DATABASE USER. This is not boilerplate advice. +# --------------------------------------------------------------------------- +# scripts/db-select.py has three guard layers, and one of them has a real hole: +# +# Layer 1 SQL parser — allowlists SELECT/SHOW/DESCRIBE/EXPLAIN/WITH, +# blocks DML+DDL, rejects multi-statement input +# and comment-hidden payloads. +# Layer 2 START TRANSACTION READ ONLY +# — MySQL rejects INSERT/UPDATE/DELETE (error 1792). +# *** It does NOT block DDL. *** CREATE / ALTER / +# DROP cause an implicit commit and execute. +# (Verified on Percona 8.0.46.) +# Layer 3 rollback on exit — nothing is committed. +# +# Because of the layer-2 DDL hole, a bug in the layer-1 parser would be the only +# thing standing between a malformed statement and a schema change. A read-only +# grant closes that gap at the server, where it cannot be argued with: +# +# CREATE USER 'ai_readonly'@'%' IDENTIFIED BY ''; +# GRANT SELECT, SHOW VIEW ON brightlocal.* TO 'ai_readonly'@'%'; +# -- deliberately NOT granted: INSERT, UPDATE, DELETE, CREATE, DROP, ALTER, +# -- INDEX, LOCK TABLES, FILE, PROCESS, SUPER +# FLUSH PRIVILEGES; +# +# Prefer a read replica over the primary — an accidental full-table scan on a +# production primary is its own kind of outage. + + +# --------------------------------------------------------------------------- +# Optional: default row cap for the runner (CLI --limit wins; hard cap is 1000) +# --------------------------------------------------------------------------- +#TOOLS_PROD_DB_ROW_LIMIT=100 diff --git a/agents/sentry-issue-investigator/config/elasticsearch.md b/agents/sentry-issue-investigator/config/elasticsearch.md new file mode 100644 index 0000000..34fd99b --- /dev/null +++ b/agents/sentry-issue-investigator/config/elasticsearch.md @@ -0,0 +1,216 @@ +# Elasticsearch (production logs) — configuration & query cookbook + +MCP server: `elasticsearch`. Every fact below was verified live on 2026-08-24. + +## Which indices matter + +| Index pattern | Contents | Use it? | +|---|---|---| +| `logstash-YYYY.MM.DD` | **All production app/web logs** (filebeat → logstash) | **Yes — this is the one** | +| `docker-logs-YYYY.MM.DD` | Docker GELF container logs | No — only `mcp-prod` on `mcp-01` | +| `.ds-mysql-*` | MySQL data streams | Rarely | +| `.internal.alerts-*`, `.kibana-*` | Kibana/alerting internals | Never | + +**Retention is ~31 days** (on 2026-08-24 the oldest was `logstash-2026.07.25`). +Sentry keeps issues longer than the logs do — if a Sentry issue's `lastSeen` is +older than ~31 days, say plainly that the logs have rolled off rather than +reporting "no logs found", which reads like the request succeeded and found +nothing. + +Query multiple days with a wildcard: `logstash-2026.08.*`, or comma-separated +exact names. Prefer the narrowest span that covers the Sentry event window — +each day is ~5M docs. + +## Field map for `logstash-*` + +ECS 8.0 via filebeat 8.11.1. All string fields are `text` with a `.keyword` +subfield. + +| Field | Type | Notes | +|---|---|---| +| `@timestamp` | `date` | **Ingest** time — see the skew warning below | +| `message` | `text` + `.keyword` | The raw log line. Main search target | +| `event.original` | `text` | Copy of the raw line | +| `log.file.path` | `text` + `.keyword` | **The log-source selector.** Use `.keyword` | +| `host.name` | `text` + `.keyword` | `web-01`…`web-05`, `workerman01`…`12`, etc. | +| `agent.name` | `text` + `.keyword` | Usually same as host | +| `input.type`, `tags`, `ecs.version`, `@version` | | Rarely useful | + +**Aggregations and `term` filters must use `.keyword`.** Plain `text` fields have +fielddata disabled and will error with +`Fielddata is disabled on [container_name]…`. + +### Timestamp skew — read this before correlating + +`@timestamp` is when filebeat **shipped** the line, while the line's own embedded +timestamp is when the event **happened**. Observed lag ranges from +sub-second to a few seconds, and `php-errors.log` lines were seen ~2s behind. + +So when correlating with a Sentry event, **widen the window** — `±2 minutes` +around the Sentry timestamp, not `±2 seconds` — and confirm using the timestamp +inside `message`, not `@timestamp`. + +## Log sources (`log.file.path.keyword`) + +Doc counts are for a single day (2026-08-24), to convey relative volume. + +| Path | Host(s) | What it is | Volume/day | +|---|---|---|---| +| `/usr/share/filebeat/transfer/tools-ssl-access.log` | `web-01`…`05` | **nginx access log for tools.brightlocal.com** | 1.87M | +| `/usr/share/filebeat/php/php-errors.log` | `web-*`, `workerman*` | **PHP error/warning log** | 892k | +| `/usr/share/filebeat/transfer/tools-ssl-error.log` | `web-*` | nginx error log (very low volume, high signal) | 11 | +| `/usr/share/filebeat/php/workerman.log` | `workerman01`…`12` | Workerman background workers | 105k | +| `/usr/share/filebeat/php/tools/crunz-output.log` | `web-*` | Cron (Crunz) output | 48k | +| `/usr/share/filebeat/php/tools/crunz-errors.log` | `web-*` | Cron errors (high signal) | 8 | +| `/var/log/listing_syncer/prod.log` | `listing-syncer-03/04` | **ListingSyncer / ActiveSync** | 1.04M | +| `/usr/share/filebeat/transfer/api-gateway.log` | `apigateway` | Go API gateway | 469k | +| `/usr/share/filebeat/transfer/access.log` | various | Other vhosts' access log | 973k | +| `/usr/share/filebeat/transfer/location_service.log` | | Location service (tiny) | 124 | +| `/usr/share/filebeat/transfer/bro_server.log` | `broserver` | Browser-automation server | 1.23M | +| `/usr/share/filebeat/transfer/geo-map.log` | `geomap` | Geo map service | 573k | +| `citation-finder.log`, `profile-finder*.log`, `serp-inspector.log`, `keywordfinder.log`, `bing_search.log`, `cf-ssl-access.log`, `vicarius.log` | | Other services | low | + +### Which source for which Sentry issue + +- **Culprit in `src/Modules/API/…`** → `tools-ssl-access.log` (find the HTTP + request), then `php-errors.log`. +- **Culprit in `src/Modules/Location*/…`, aggregator/sync symptoms** → + `listing_syncer/prod.log` **and** `tools-ssl-access.log`. +- **Worker/`Adapter/Worker`/Messenger frames** → `workerman.log`. +- **Cron/`Adapter/Cli` frames** → `crunz-output.log` + `crunz-errors.log`. +- **502/504/timeout symptoms** → `tools-ssl-error.log`. + +> **`php-errors.log` is not a mirror of Sentry.** Sentry captures thrown +> exceptions; this file mostly carries warnings, notices, and deprecations. On +> 2026-08-24 it had 109 lines mentioning `Modules` and **zero** matching +> `Location AND Exception`, while Sentry had plenty of Location exceptions. Not +> finding your exception here is normal and is **not** evidence the issue is +> stale. The access log is the more reliable correlation route. + +## Access-log line format + +``` +[CL: 103.22.142.112|RAY:a3012a245ef4756a-LHR] [24/Aug/2026:09:00:07 +0000] +"POST /seo-tools/admin/rm/reports/623832/reviews/199372535/gmb/respond HTTP/2.0" +500 2 "" "" 2.387 [SSL: 0SUCCESS] +[Upstream: 10.79.155.24:8080|500|2.386] +``` + +Fields in order: client IP, Cloudflare Ray ID, local timestamp, request line, +**status**, bytes, referer, user agent, **total seconds**, SSL result, +upstream `addr|status|seconds`. + +The **Ray ID** is the best correlation key — it is unique per request, so once +you have it from one line you can pivot across every source that logged it. + +## Verified query recipes + +### Pick a log source and tail it + +```json +{ + "size": 20, + "_source": ["@timestamp", "message", "host.name"], + "query": {"bool": {"filter": [ + {"term": {"log.file.path.keyword": "/usr/share/filebeat/php/php-errors.log"}} + ]}}, + "sort": [{"@timestamp": "desc"}] +} +``` + +### Find 5xx responses ✅ 20 hits + +Status codes sit mid-line, so anchor on the request-line suffix: + +```json +{"query_string": {"query": "message:(\"HTTP\\/2.0\\\" 500\" OR \"HTTP\\/1.1\\\" 500\")"}} +``` + +### Match a URL path ✅ 5,232 hits for `/api/v1` + +```json +{"match_phrase": {"message": "/api/v1"}} +``` + +`match_phrase` is the right tool for paths and slugs (`"location-manager"` → +4,527 hits). + +### Correlate a Sentry event to its HTTP request + +```json +{ + "size": 50, + "_source": ["@timestamp", "message", "host.name"], + "query": {"bool": { + "filter": [ + {"term": {"log.file.path.keyword": "/usr/share/filebeat/transfer/tools-ssl-access.log"}}, + {"range": {"@timestamp": {"gte": "2026-08-24T08:58:00Z", "lte": "2026-08-24T09:02:00Z"}}} + ], + "must": [{"query_string": {"query": "message:(\"HTTP\\/2.0\\\" 500\")"}}] + }}, + "sort": [{"@timestamp": "desc"}] +} +``` + +Then pivot on the Ray ID: `{"match_phrase": {"message": "a3012a245ef4756a"}}`. + +### PHP namespaces in log lines + +Namespaces appear backslash-escaped (`Modules\\Cb\\Adapter\\Http\\…`). The +analyzer splits on backslashes, so: + +| Query | Result | +|---|---| +| `{"match": {"message": "Modules"}}` | ✅ 109 | +| `{"query_string": {"query": "Modules AND Cb", "default_field": "message"}}` | ✅ 21 | +| `{"query_string": {"query": "message:*Modules*"}}` | ✅ 109 (case-insensitive) | +| `{"match_phrase": {"message": "Modules Location"}}` | ❌ 0 — phrase across a backslash does not match | +| `{"query_string": {"query": "message:*Upstream*500*"}}` | ❌ 0 — multi-wildcard across tokens fails | + +**Use `AND`-joined terms for namespaces, not `match_phrase`.** + +### Count before you fetch + +Cheap way to test a filter's selectivity without pulling documents: + +```json +{ + "size": 0, + "query": {"bool": {"filter": [ + {"term": {"log.file.path.keyword": "/usr/share/filebeat/php/php-errors.log"}} + ]}}, + "aggs": { + "modules": {"filter": {"match": {"message": "Modules"}}}, + "exception": {"filter": {"match": {"message": "Exception"}}} + } +} +``` + +### Group by host or source + +```json +{"size": 0, "aggs": { + "by_host": {"terms": {"field": "host.name.keyword", "size": 30}}, + "by_source": {"terms": {"field": "log.file.path.keyword", "size": 40}} +}} +``` + +## Output size discipline + +`list_indices` on `*` returns ~74KB and gets truncated to a file. **Never call +it with `*`.** Use a narrow pattern (`logstash-2026.08.*`) or skip it entirely — +the index naming is documented above and does not change. + +Always set `size` explicitly (10–50) and restrict `_source` to the fields you +need. Access-log messages are long; 50 unfiltered hits is a lot of tokens. + +## Read-only discipline + +The `elasticsearch` MCP server exposes **write** tools: `create_index`, +`delete_index`, `add_document`, `update_document`, `delete_document`, +`update_by_query`, `delete_by_query`, `bulk`. + +This agent must use **only** `search`, `count_documents`, `get_mappings`, +`list_indices`, `get_aliases`, `get_templates`, `get_cluster_health`. Never call +a write tool against production logs — not even to "test". There is no +undo. diff --git a/agents/sentry-issue-investigator/config/jira.md b/agents/sentry-issue-investigator/config/jira.md new file mode 100644 index 0000000..778b577 --- /dev/null +++ b/agents/sentry-issue-investigator/config/jira.md @@ -0,0 +1,240 @@ +# Jira configuration & ticket conventions + +MCP server: `atlassian`. Every value below was verified live on 2026-08-24. + +## Connection + +| Setting | Value | +|---|---| +| Site | `https://brightlocal.atlassian.net` | +| `cloudId` | `5d89576a-2167-45d7-b6a4-cfa42edbee57` | +| Scopes | `read:jira-work`, `write:jira-work` (create + comment both work) | + +Pass `cloudId` on every call. You may also pass `brightlocal.atlassian.net` +directly, but the UUID is known-good — use it. + +> The MCP server emits a deprecation notice about the HTTP+SSE transport being +> unsupported after 30 June 2026 (`.mcp.json` points at `/v1/sse`; the +> replacement is `/v1/mcp`). It is noise on every call — don't relay it into +> investigation reports, but do surface it if Jira calls start failing. + +## Target project + +**Default: `LM` — "Backend Services"** (id `13423`, classic software project). + +Confirmed by precedent: `LM-4317`, `LM-3736`, `LM-3741`, `LM-3695`, `LM-4143` +are all Sentry-originated Tools backend errors filed in `LM`. + +**Always confirm the project with the user before creating**, showing project, +type, priority, and summary. `LM` is the default, not a certainty — Sentry +surfaces other owning teams that belong on other boards: + +| Sentry team | Likely project | Notes | +|---|---|---| +| `connected-locations-be` | `LM` — Backend Services | The CL team; `LM` is their prefix | +| `backend-insights-be` | `BI` — Backend Insights | Confirm before filing | +| `citations-be` | `CB` — Citations | Confirm before filing | + +Other candidates if the user redirects: `CL` (Clients & Locations), +`PS` (Platform Services), `PRD` (Product Engineering). + +> **Do NOT use the `API` project for Tools API-module bugs.** `API` is a dead +> board (last activity 2022) about third-party review-fetching integrations, +> unrelated to `src/Modules/API/`. API-module errors go to `LM` like any other +> Tools backend error. + +## Issue types in `LM` + +| Type | id | Use for | +|---|---|---| +| **Internal Bug** | `10273` | **Default for Sentry-originated errors** | +| Bug | `1` | Customer/QA-reported. House style prefixes `[Standup ticket]` | +| Investigation | `8` | Symptom real but cause not established | +| Task | `3` | Cleanup/observability work with no user-visible defect | +| Improvement/New Feature | `4` | Enhancements | +| Sub-task | `5` | Child of an existing ticket | +| Epic | `9` | Collection | + +Recent `LM` issues matching "sentry": 11 `Bug`, 10 `Internal Bug`, 8 `Task`. The +split is meaningful — **`Internal Bug` is the convention for errors we found +ourselves in Sentry**, `Bug` is for things a human reported. Pick `Investigation` +over `Internal Bug` when the diagnosis stopped at "unverified". + +## Priority + +Field `priority`, set via `additional_fields`. Scheme is `P1`…: `LM-4317` uses +**`P2 - Medium`**, a good default. + +```json +{"priority": {"name": "P2 - Medium"}} +``` + +Justify anything higher with evidence from the investigation — sustained high +event count, many distinct users, or data corruption. A 29-event/1-user issue is +not a P1 no matter how alarming the stack trace looks. + +## Labels + +No components are configured on `LM`. Labels seen in use: `BackEnd`, +`FrontEnd`, `QA`, `Refinement`, `zendesk_escalated`, `GOAL1`. `LM-4317` carries +none, so labels are optional — `BackEnd` is a reasonable addition for Tools +backend errors. Don't invent new labels. + +## Deduplicate before creating — mandatory + +Sentry issues often already have a ticket. `LM-4317` already covers +`TOOLS-BACKEND-B77`; filing another would be pure noise. + +**Verified working dedup query** (finds `LM-4317` from the Sentry short ID): + +``` +searchJiraIssuesUsingJql( + cloudId = "5d89576a-2167-45d7-b6a4-cfa42edbee57", + jql = 'project = LM AND text ~ "TOOLS-BACKEND-B77" ORDER BY created DESC', + fields = ["key", "summary", "issuetype", "status"], +) +``` + +Run **before** proposing a ticket. Search on: + +1. The Sentry short ID (`TOOLS-BACKEND-B77`) — most reliable, since the house + style pastes the Sentry link into the description. +2. The exception class (`AdditionalDataForwardFailedException`). +3. A distinctive phrase from the error message. + +Widen past `project = LM` when unsure — drop the project clause entirely. + +If a match exists: **report it and stop.** Offer to add findings as a comment on +the existing ticket instead of creating a duplicate. Only create a new ticket if +the user says the existing one is genuinely different. + +Keep `fields` narrow. An unrestricted `LM` search blew the token limit and got +spilled to a file; `["key","summary","issuetype","status"]` is enough to triage. + +## Description structure — follow `LM-4317` + +`LM-4317` is the house reference, written by this workspace's user for exactly +this workflow (a Sentry investigation of the Location module). Match its shape: + +``` +h2. Summary +2–4 sentences: what breaks, mechanism in one line, who it hits. +Inline Sentry link + event count and window. + +h2. Failure chain +{code} block: the call chain from entry point to throw site, one hop per line. + +h2. Root cause +The specific defect, with {code:php} snippets and file:line references. +State how many events were sampled to confirm it. + +h2. Impact +Quantified. Distinct entities affected, customer spread, daily peaks, +blast radius (what else fails as a consequence). + +h2. Observability defects found during investigation [optional] +Numbered list of things that made this harder to diagnose than it should be. + +h2. Scope of this ticket +_In scope:_ bullets, each naming the file to change. +_Out of scope (follow-up tickets):_ bullets, each with why it's separate. + +h2. Acceptance criteria +Bullets, each independently checkable. +``` + +Non-obvious things that make `LM-4317` good, and that you should copy: + +- **Marks unverified claims as unverified** — "_Not confirmed against the + database._" Carry the investigation's uncertainty into the ticket; don't + launder inference into fact. +- **States sample size** — "Verified across 15/15 sampled Sentry events". +- **Suggests the query** that would confirm an open question. +- **Separates in-scope from follow-ups**, so the ticket stays implementable. +- **Links Sentry by URL**, including the numeric form + (`…/issues/41676/`) — both numeric and short-ID URLs resolve. + +### Formatting — verify after creating + +`LM-4317`'s description is stored as **Jira wiki markup** (`h2.`, `{code}`, +`{code:php}`, `{{monospace}}`, `_italic_`, `[text|url]`), and `LM` is a classic +project (`style: classic`, `simplified: false`). + +Write wiki markup to match. But `createJiraIssue` also accepts +`contentFormat: "markdown"` or `"adf"`, and which one renders correctly depends +on the project's renderer — which is **not** something to guess at. + +**So: after creating, read the issue back with `getJiraIssue` and check the +description rendered as headings and code blocks.** If literal `h2.` or `{code}` +markers are showing as text, rewrite the description in Markdown with +`contentFormat: "markdown"`. Tell the user either way — a mangled ticket is +worse than no ticket. + +## Creating the ticket + +``` +createJiraIssue( + cloudId = "5d89576a-2167-45d7-b6a4-cfa42edbee57", + projectKey = "LM", + issueTypeName = "Internal Bug", + summary = ": ", // or a plain-language symptom + description = "", + additional_fields = {"priority": {"name": "P2 - Medium"}}, +) +``` + +**Summary style.** Both forms have precedent: + +- Verbatim exception — `TypeError: Cannot read properties of undefined (reading 'info')` (`LM-3741`) +- Plain-language symptom — `Additional-data forward to ListingSyncer fails with 400 for locations with empty country` (`LM-4317`) + +Prefer the plain-language form **when the root cause is known** — it is far more +useful on a board. Fall back to the exception text when the cause is still open. +Keep it under ~120 chars. + +Leave `assignee` unset (`LM-4317` is unassigned) unless the user asks. + +## Linking + +Available link types include `Relates`, `Duplicate`, `Blocks`, `Problem/Incident` +(`causes` / `is caused by`), `Cloners`, `Defect`, `Found during testing`. + +Use `Relates` for a sibling ticket carved out of the same investigation, and +`Problem/Incident` when one issue genuinely causes another. + +``` +createIssueLink( + cloudId = "5d89576a-2167-45d7-b6a4-cfa42edbee57", + type = "Relates", + inwardIssue = "LM-4317", + outwardIssue = "LM-4318", +) +``` + +## Fix plan as a comment + +``` +addCommentToJiraIssue( + cloudId = "5d89576a-2167-45d7-b6a4-cfa42edbee57", + issueIdOrKey = "LM-4321", + commentBody = "", +) +``` + +Comment rather than description so the ticket keeps a clean problem statement +and the plan stays reviewable and revisable on its own. See +`ticket-templates.md` for the plan structure. + +## Write discipline + +Creating tickets and comments is **outward-facing and visible to the whole +team**, and Jira has no clean undo — a deleted ticket still burns a key and may +have fired notifications. + +So: **never create a ticket or post a comment without explicit confirmation in +the current turn.** Show exactly what will be created and wait. Approval to +create a ticket is not approval to also post a plan, and neither is approval to +implement — each is its own gate. + +Never transition, assign, or edit tickets you did not create in this session +unless asked. diff --git a/agents/sentry-issue-investigator/config/sentry.md b/agents/sentry-issue-investigator/config/sentry.md new file mode 100644 index 0000000..e923834 --- /dev/null +++ b/agents/sentry-issue-investigator/config/sentry.md @@ -0,0 +1,195 @@ +# Sentry configuration & query cookbook + +All values here were verified live against the self-hosted instance. +MCP server: `sentry-selfhosted`. Base URL: `https://sentry.bll-i.co.uk`. + +## Organization + +Slug: **`brightlocal`** + +> **Gotcha:** `find_organizations()` returns *"You don't appear to be a member of +> any organizations"* — the MCP proxy user (Sentry user ID 42) has no org +> membership listed, but the org slug resolves fine. **Do not** call +> `find_organizations` to discover the org and conclude Sentry is unavailable. +> Always pass `organizationSlug: "brightlocal"` directly. + +## Projects + +The two this agent cares about: + +| Project | What it is | Issue ID prefix | +|---|---|---| +| **`tools-backend`** | Tools PHP monolith — **primary target** | `TOOLS-BACKEND-…` | +| **`tools-frontend`** | Tools browser-side JS/React | `TOOLS-FRONTEND-…` | + +Others on the instance, for reference: `new-tools-app`, `listing-syncer`, +`javascript`, `citationfinder`, `cbot`, `cbot-frontend`, `cbot-extension-afex`, +`custos`, `go-services`, `internal`, `mcp`, `mozfetcher`, `profilecomber`, +`profilefinder`, `rankingspider`, `sdparser`, `submitor`, `vicarius`. + +`Location` and `API` are backend modules, so **default to `tools-backend`**. +Only widen to `tools-frontend` if the user asks or the symptom is clearly UI-side. + +## Module scoping — the important part + +Tools has no Sentry tag identifying the owning module. Scope by **stack frame +path** instead. Verified working field: **`stack.abs_path`** with leading/trailing +globs. + +Culprit paths on production look like: + +``` +/home/sites/tools/builds//src/Modules/Location/Application/Service/... +``` + +The build hash changes every deploy, so the leading `*/` glob is required. + +### Working filters + +``` +# Location module family +stack.abs_path:"*/src/Modules/Location/*" + +# API module family +stack.abs_path:"*/src/Modules/API/*" +``` + +### What does NOT work (tested, returns zero results) + +``` +stack.filename:"src/Modules/Location/*" # ✗ no matches +stack.module:"Modules\\Location\\*" # ✗ no matches +``` + +### Related module directories + +`src/Modules/` contains several Location-adjacent modules. `*/src/Modules/Location/*` +matches **only** `Location`, not its siblings. When the user says "location module" +broadly, consider also querying: + +- `Location` — core location domain (the literal reading) +- `LocationManager` — LM surface, includes `Services/ActiveSync/` +- `LocationConnections` — aggregator/directory connections +- `LocationSummary` — summary/report views +- `GeoLocationSearch` + +Ask, or run the narrow filter first and mention the siblings. Don't silently +broaden — occurrence counts change a lot between the narrow and wide readings. + +### Frame-level matching is a wide net + +`stack.abs_path` matches if **any frame** in the stack touches that path, not just +the crash site. So `*/src/Modules/API/*` legitimately returns issues whose culprit +is elsewhere (e.g. a `LocationSummary` repository reached through an API +controller, or a Guzzle middleware frame). + +**Always classify results into two buckets:** + +1. **Owned by the module** — the *culprit* path is inside the module. +2. **Passing through the module** — culprit is elsewhere; the module only appears + deeper in the stack. + +Report which bucket each issue is in. Bucket 2 issues are often owned by another +team, and mislabelling them wastes the reader's time. + +## Listing issues by occurrence count + +The user's default ask is "ordered by number of occurrences" → **`sort: "freq"`**. + +``` +search_issues( + organizationSlug = "brightlocal", + projectSlugOrId = "tools-backend", + query = 'is:unresolved stack.abs_path:"*/src/Modules/Location/*"', + sort = "freq", + period = "30d", + limit = 25, +) +``` + +- `sort: "freq"` — occurrence count (**the default for this agent**) +- `sort: "user"` — distinct users affected; use when prioritising customer impact +- `sort: "date"` — last seen; use for "what's broken right now" +- `sort: "new"` — first seen; use for "what did this deploy break" + +`period` accepts only `24h`, `7d`, `14d`, `30d`, `90d`. Default to `30d`. +The **Events** count in results is scoped to that period — always state the +window alongside any count, or the number is meaningless. + +### Other useful query clauses + +``` +is:unresolved is:resolved is:ignored is:regressed +level:error level:warning +firstSeen:-24h lastSeen:-2h +environment:production +assigned:connected-locations-be +userCount:>100 +``` + +Owning teams seen on Tools issues: `connected-locations-be`, +`backend-insights-be`, `citations-be`. `assigned:` is a good cross-check that a +module filter caught the right team's work. + +## Fetching one issue + +Accept either form from the user and pass it straight through: + +``` +# Short ID +get_sentry_resource(organizationSlug="brightlocal", resourceType="issue", + resourceId="TOOLS-BACKEND-B77") + +# Full URL — resource type is auto-detected +get_sentry_resource(url="https://sentry.bll-i.co.uk/organizations/brightlocal/issues/TOOLS-BACKEND-B77") +``` + +Numeric URLs (`/issues/12345/`) work too. Don't ask the user to reformat a link. + +For the latest event's full stack trace, tags, and request context, follow up with +`resourceType="event"`, and `resourceType="breadcrumbs"` for the lead-up. + +## Aggregate counts + +`search_issues` returns grouped issues. For **counts and time series**, use +`search_events`: + +``` +search_events( + organizationSlug = "brightlocal", + projectSlug = "tools-backend", + dataset = "errors", + query = 'stack.abs_path:"*/src/Modules/API/*"', + fields = ["issue", "count()"], + sort = "-count()", + period = "7d", +) +``` + +## Write operations — ask first + +`update_issue` can resolve, ignore, and assign. It changes shared team state +visible to everyone. **Never call it unless the user explicitly asks in that +turn.** Investigating is read-only work. + +## Linking out + +Every response listing issues should offer the dashboard link: + +``` +https://sentry.bll-i.co.uk/organizations/brightlocal/issues/?project=tools-backend&query= +``` + +## Reference: verified sample issues + +Useful for sanity-checking that filters still behave: + +- `TOOLS-BACKEND-B77` — `Modules\Location\…\AdditionalDataForwardFailedException`, + culprit inside `Location` (bucket 1), team `connected-locations-be`. +- `TOOLS-BACKEND-B74` — `TypeError` in + `Modules\API\Manage\Location\Infrastructure\Mapper\LocationDTOFactory` + (bucket 1 for both `API` *and* `Location`). +- `TOOLS-BACKEND-B7A` — `Symfony\…\LogicException` in + `Modules\API\Manage\Location\Adapter\Http\V1\Location\UpdateLocation`. +- `TOOLS-BACKEND-B72` — culprit in `LocationSummary`, surfaces under the `API` + filter (bucket 2 — reached through an API frame). diff --git a/agents/sentry-issue-investigator/config/ticket-templates.md b/agents/sentry-issue-investigator/config/ticket-templates.md new file mode 100644 index 0000000..ffea5a3 --- /dev/null +++ b/agents/sentry-issue-investigator/config/ticket-templates.md @@ -0,0 +1,177 @@ +# Ticket and fix-plan templates + +Two artifacts, two jobs. The **ticket description** states the problem. The +**fix-plan comment** states the intended change. Keeping them separate means the +plan can be revised without rewriting the problem statement, and the ticket still +reads correctly if the plan is rejected. + +Both are Jira wiki markup, matching `LM-4317`. Verify rendering after posting +(see `jira.md`). + +--- + +## 1. Ticket description + +Fill only the sections you have evidence for. **Delete a section rather than +padding it** — an empty "Impact" heading is worse than no heading. + +``` +h2. Summary + +{Two to four sentences: what breaks, the mechanism in one line, and who it hits.} + +_Sentry:_ [{SENTRY-ID}|{sentry-url}] — {N} events / {M} users since {date}, +{status: still ongoing | last seen {date}}. + +h2. Failure chain + +{code} +{entry point — HTTP route, worker, or cron} + -> {Class::method} + -> {Class::method} + -> {throw site} ==> {exception class} +{code} + +h2. Root cause + +{The specific defect.} {file}:{line}: + +{code:php} +{the offending lines} +{code} + +{Why this produces the observed failure. State the sample size that confirmed +it — e.g. "Verified across 15/15 sampled Sentry events".} + +h2. Impact + +* {Blast radius — what else fails as a consequence.} +* {Distinct entities affected, and whether it is one customer or many.} +* {Frequency shape — bursts vs steady; daily peaks with dates.} +* {Whether the user sees an error, wrong data, or silence.} + +h2. Evidence + +* _Sentry:_ {events}/{users} over {window}; release {build}; culprit {file}:{line} +* _Production logs:_ {what was found, in which source, at what time} — + or _no correlating lines (searched {source} at {T}±2m)_ + or _outside the ~31-day Elasticsearch retention window_ +* _Database:_ {finding + the exact query run} — or _not inspected_ +* _Code:_ read at {commit-or-branch} — {note if it differs from the deployed build} + +h2. Observability defects found during investigation + +{Optional. Numbered. Things that made this harder to diagnose than it should +have been — swallowed exceptions, missing context, misleveled logs. Each is a +candidate follow-up ticket.} + +h2. Scope of this ticket + +_In scope:_ + +* {Repo}: {change}, in {file}. + +_Out of scope (follow-up tickets):_ + +* {Thing} — {why it is separate}. + +h2. Open questions + +{Anything the investigation could not settle. Name the query, log, or test that +would settle it. Mark clearly as unconfirmed — do NOT promote inference to fact.} + +h2. Acceptance criteria + +* {Independently checkable statement.} +* {One per line. Behavioural, not implementation-shaped.} +``` + +### Rules + +- **Every number traces to a tool result.** No estimated event counts. +- **Mark unverified claims** — `LM-4317` writes "_Not confirmed against the + database._" Copy that habit. +- **Quote the query** behind any data claim. +- **No PII.** Location IDs and customer IDs are fine; names, addresses, and + emails from production rows are not. Quote the minimum that makes the point. +- **Don't inflate severity.** State the real numbers and let them speak. + +--- + +## 2. Fix-plan comment + +Posted as a comment, only after the user approves. This is also what +`jira-to-pr` consumes as its plan input, so it must be specific enough to +implement from — file paths, not vibes. + +``` +h3. Proposed fix plan + +_Generated by the sentry-issue-investigator agent from {SENTRY-ID}. Review +before implementing._ + +h4. Approach + +{2–4 sentences. What changes conceptually, and why this approach over the +alternatives. If a safer narrow fix and a fuller correct fix both exist, say so +and recommend one.} + +h4. Changes + +*{Repo} — {file}* +# {Concrete change.} +# {Concrete change.} + +*{Repo} — {file}* +# {Concrete change.} + +h4. Tests + +* {New or updated test, and the case it pins down.} +* {Regression test for the exact input from the Sentry event.} + +h4. Risks + +* {Risk} — {mitigation}. +* {Anything touching auth, payments, or data migration — flag explicitly; + per shared/engineering/git-conventions.md the PR must then be a draft.} + +h4. Verification after deploy + +* {How to confirm the fix worked — the Sentry issue stops recurring, a specific + log line disappears, a query returns zero rows.} + +h4. Not included + +* {Deliberate exclusions, so the reviewer knows they were considered.} +``` + +### Rules + +- **Name real files**, verified to exist in the checkout. A plan referencing a + path that isn't there is worse than no plan — `jira-to-pr` will act on it. +- **Prefer the smallest correct fix.** If the root cause sits deeper than the + crash site, say so and recommend which layer to fix. +- **Note repo ordering.** Tools + ListingSyncer changes need the backend/API + side first; git-conventions requires backend PR first across repos. +- **Flag when a fix is a workaround** rather than a root-cause repair. +- **Don't plan the out-of-scope items.** They were excluded for a reason. + +--- + +## 3. Handoff note to `jira-to-pr` + +Once the user confirms implementation, `jira-to-pr` takes the ticket key. Tell +it nothing that isn't in the ticket — the ticket and its plan comment must be +self-sufficient, because that pipeline re-reads Jira from scratch. + +Before handing off, confirm: + +- [ ] Ticket exists and the key is correct +- [ ] Plan comment is posted (not just drafted in chat) +- [ ] Plan names real files that exist in the target repos +- [ ] Issue type is right — it drives the branch prefix via ADR-0019 + (Bug / Internal Bug → `fix/`, Task → `task/`) +- [ ] Risky-area changes are flagged, so the PR is opened as a draft + +Then read `agents/jira-to-pr/CLAUDE.md` and run its pipeline with the key. diff --git a/agents/sentry-issue-investigator/scripts/db-select.py b/agents/sentry-issue-investigator/scripts/db-select.py new file mode 100755 index 0000000..32cc115 --- /dev/null +++ b/agents/sentry-issue-investigator/scripts/db-select.py @@ -0,0 +1,386 @@ +#!/usr/bin/env python3 +"""Read-only MySQL query runner for the sentry-issue-investigator agent. + +Three layers stop this script from mutating production: + +1. Statement parsing — string literals and comments are blanked (so payloads + cannot hide in `--`, `#`, or `/*! ... */`), multi-statement input is + rejected, the leading keyword must be in an allowlist, and a blocklist + catches DML/DDL anywhere in the remaining text. +2. Server-side read-only transaction — the statement runs inside + START TRANSACTION READ ONLY. Verified against Percona 8.0.46: MySQL rejects + INSERT/UPDATE/DELETE with error 1792. NOTE: it does NOT block DDL — CREATE / + ALTER / DROP trigger an implicit commit and execute anyway. So layer 2 + backstops DML only; for DDL, layer 1 is the sole barrier. +3. Rollback on exit — nothing this script opens is ever committed. + +Because layer 2 has that DDL hole, a parser bug would be the only thing between +a malformed statement and a schema change. Connect with a GRANT SELECT-only +database user; that is the sole airtight guarantee. See config/db.env.example. + +The DSN is never echoed, and passwords are redacted from error output. + +Usage: + export TOOLS_PROD_DB_DSN='mysql://user:pass@host:3306/dbname' + ./db-select.py "SELECT id, name FROM locations WHERE id = 4135533" + ./db-select.py --json --limit 20 "SHOW CREATE TABLE locations" + echo "DESCRIBE locations" | ./db-select.py - + +Exit codes: 0 ok, 2 rejected by guard, 3 connection/query error, 4 bad usage. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import sys +from urllib.parse import unquote, urlparse + +try: + import pymysql +except ImportError: # pragma: no cover + sys.stderr.write( + "error: pymysql is not installed. Install it with: pip install pymysql\n" + ) + raise SystemExit(3) + + +DEFAULT_DSN_ENV = "TOOLS_PROD_DB_DSN" +DEFAULT_LIMIT = 100 +MAX_LIMIT = 1000 +DEFAULT_TIMEOUT = 30 + +# A statement must START with one of these. +ALLOWED_LEADING = ("select", "show", "describe", "desc", "explain", "with") + +# Rejected anywhere in the comment-free statement. +# Each entry is (compiled pattern, human-readable label). +BLOCKED = [ + (r"\binsert\b", "INSERT"), + (r"\bupdate\b", "UPDATE"), + (r"\bdelete\b", "DELETE"), + (r"\bdrop\b", "DROP"), + (r"\balter\b", "ALTER"), + (r"\bcreate\b", "CREATE"), + (r"\btruncate\b", "TRUNCATE"), + (r"\brename\b", "RENAME"), + # REPLACE( is a legitimate string function; REPLACE INTO is not. + (r"\breplace\b(?!\s*\()", "REPLACE"), + (r"\bgrant\b", "GRANT"), + (r"\brevoke\b", "REVOKE"), + (r"\block\s+tables\b", "LOCK TABLES"), + (r"\bunlock\s+tables\b", "UNLOCK TABLES"), + (r"\bset\b", "SET"), + (r"\bcall\b", "CALL"), + (r"\bdo\b", "DO"), + (r"\bhandler\b", "HANDLER"), + (r"\bprepare\b", "PREPARE"), + (r"\bexecute\b", "EXECUTE"), + (r"\bdeallocate\b", "DEALLOCATE"), + (r"\bload\s+data\b", "LOAD DATA"), + (r"\bload_file\b", "LOAD_FILE"), + (r"\binto\s+outfile\b", "INTO OUTFILE"), + (r"\binto\s+dumpfile\b", "INTO DUMPFILE"), + (r"\bget_lock\b", "GET_LOCK"), + (r"\bbenchmark\b", "BENCHMARK"), + (r"\bsleep\b", "SLEEP"), + (r"\bflush\b", "FLUSH"), + (r"\breset\b", "RESET"), + (r"\bkill\b", "KILL"), + (r"\bshutdown\b", "SHUTDOWN"), + (r"\bstart\s+transaction\b", "START TRANSACTION"), + (r"\bcommit\b", "COMMIT"), + (r"\brollback\b", "ROLLBACK"), + (r"\bsavepoint\b", "SAVEPOINT"), + (r"\buse\b", "USE"), +] +BLOCKED = [(re.compile(pattern), label) for pattern, label in BLOCKED] + + +class Rejected(Exception): + """The statement failed the guard. Never reaches the database.""" + + +def strip_for_analysis(sql: str) -> str: + """Remove string literals and comments so keyword checks can't be bypassed. + + String and identifier literals are blanked rather than deleted so that a + column value like 'update the thing' cannot trip the blocklist, while + /*! ... */ MySQL execution comments and -- hidden payloads cannot hide one. + """ + out = [] + i = 0 + n = len(sql) + while i < n: + ch = sql[i] + nxt = sql[i + 1] if i + 1 < n else "" + + # -- line comment (MySQL requires whitespace/EOL after --) + if ch == "-" and nxt == "-" and (i + 2 >= n or sql[i + 2] in " \t\r\n"): + while i < n and sql[i] != "\n": + i += 1 + continue + # # line comment + if ch == "#": + while i < n and sql[i] != "\n": + i += 1 + continue + # /* block comment */ — including /*! ... */ execution comments, + # whose contents MySQL *does* run, so we must not silently drop them. + if ch == "/" and nxt == "*": + executable = sql[i + 2 : i + 3] in ("!", "+") + end = sql.find("*/", i + 2) + body = sql[i + 2 : end if end != -1 else n] + # Keep the body of executable comments visible to the checks. + out.append(" " + (body if executable else "") + " ") + i = (end + 2) if end != -1 else n + continue + # quoted literals: ' " ` + if ch in ("'", '"', "`"): + quote = ch + i += 1 + while i < n: + if sql[i] == "\\" and quote != "`": + i += 2 + continue + if sql[i] == quote: + # doubled quote is an escaped quote + if i + 1 < n and sql[i + 1] == quote: + i += 2 + continue + i += 1 + break + i += 1 + out.append(" '' ") + continue + + out.append(ch) + i += 1 + + return "".join(out) + + +def guard(sql: str) -> str: + """Validate the statement. Returns the statement to execute, or raises.""" + original = sql.strip() + if not original: + raise Rejected("empty statement") + + analysis = strip_for_analysis(original).strip() + + # Allow exactly one optional trailing semicolon. + analysis_no_tail = analysis.rstrip().rstrip(";").rstrip() + if ";" in analysis_no_tail: + raise Rejected( + "multiple statements are not allowed — send one SELECT at a time" + ) + + lowered = analysis_no_tail.lower() + + leading = re.match(r"[a-z_]+", lowered) + if not leading or leading.group(0) not in ALLOWED_LEADING: + got = leading.group(0).upper() if leading else "(none)" + raise Rejected( + f"statement must start with one of " + f"{', '.join(k.upper() for k in ALLOWED_LEADING)} — got {got}" + ) + + # WITH ... must resolve to a SELECT, never a CTE-driven DML. + if leading.group(0) == "with" and not re.search(r"\bselect\b", lowered): + raise Rejected("WITH clause must contain a SELECT") + + # Every SHOW form in MySQL is read-only, and several carry otherwise-blocked + # keywords (SHOW CREATE TABLE, SHOW TABLE STATUS). The single-statement check + # above already ran, so skipping the keyword blocklist here is safe. + if leading.group(0) != "show": + for pattern, label in BLOCKED: + match = pattern.search(lowered) + if match: + raise Rejected( + f"forbidden keyword {label} at offset {match.start()} — " + "this runner is read-only" + ) + + # Send the original text (comments and all); the guard analysed a + # normalised copy, MySQL should see exactly what the user wrote. + return original.rstrip().rstrip(";").rstrip() + + +def env_row_limit() -> int: + """Default row cap, overridable via $TOOLS_PROD_DB_ROW_LIMIT.""" + raw = os.environ.get("TOOLS_PROD_DB_ROW_LIMIT") + if not raw: + return DEFAULT_LIMIT + try: + return max(1, min(int(raw), MAX_LIMIT)) + except ValueError: + sys.stderr.write( + f"warning: ignoring non-numeric $TOOLS_PROD_DB_ROW_LIMIT={raw!r}\n" + ) + return DEFAULT_LIMIT + + +def parse_dsn(dsn: str) -> dict: + """Parse mysql://user:pass@host:port/db into pymysql kwargs.""" + if "://" not in dsn: + raise SystemExit( + "error: DSN must look like mysql://user:pass@host:3306/dbname" + ) + + parsed = urlparse(dsn) + if parsed.scheme not in ("mysql", "mysql+pdo", "pdo-mysql", "mysqli"): + raise SystemExit(f"error: unsupported DSN scheme '{parsed.scheme}'") + if not parsed.hostname: + raise SystemExit("error: DSN is missing a host") + + return { + "host": parsed.hostname, + "port": parsed.port or 3306, + "user": unquote(parsed.username or ""), + "password": unquote(parsed.password or ""), + "database": (parsed.path or "/").lstrip("/") or None, + } + + +def redact(text: str, secrets: list[str]) -> str: + for secret in secrets: + if secret: + text = text.replace(secret, "***") + return text + + +def render_table(columns: list[str], rows: list[tuple]) -> str: + if not columns: + return "(no columns)" + cells = [[("NULL" if v is None else str(v)) for v in row] for row in rows] + widths = [len(c) for c in columns] + for row in cells: + for idx, value in enumerate(row): + widths[idx] = max(widths[idx], min(len(value), 80)) + + def line(values: list[str]) -> str: + return " | ".join( + (v if len(v) <= 80 else v[:77] + "...").ljust(widths[i]) + for i, v in enumerate(values) + ) + + out = [line(list(columns)), "-+-".join("-" * w for w in widths)] + out.extend(line(row) for row in cells) + return "\n".join(out) + + +def main() -> int: + parser = argparse.ArgumentParser( + description="Run a single read-only SQL statement against a MySQL DSN.", + ) + parser.add_argument( + "sql", + help="the statement to run, or '-' to read it from stdin", + ) + parser.add_argument( + "--dsn", + default=None, + help=f"DSN override; prefer the ${DEFAULT_DSN_ENV} env var so the " + "password stays out of shell history and process listings", + ) + parser.add_argument("--limit", type=int, default=env_row_limit(), + help=f"max rows to print (default {DEFAULT_LIMIT} or " + f"$TOOLS_PROD_DB_ROW_LIMIT, cap {MAX_LIMIT})") + parser.add_argument("--timeout", type=int, default=DEFAULT_TIMEOUT, + help=f"connect/read timeout in seconds " + f"(default {DEFAULT_TIMEOUT})") + parser.add_argument("--json", action="store_true", + help="emit JSON instead of an aligned table") + parser.add_argument("--explain-guard", action="store_true", + help="validate the statement and exit without connecting") + args = parser.parse_args() + + sql_input = sys.stdin.read() if args.sql == "-" else args.sql + + try: + statement = guard(sql_input) + except Rejected as exc: + sys.stderr.write(f"REJECTED: {exc}\n") + return 2 + + if args.explain_guard: + print("OK — statement passed the read-only guard (not executed)") + return 0 + + dsn = args.dsn or os.environ.get(DEFAULT_DSN_ENV) + if not dsn: + sys.stderr.write( + f"error: no DSN. Set ${DEFAULT_DSN_ENV} or pass --dsn.\n" + ) + return 4 + + limit = max(1, min(args.limit, MAX_LIMIT)) + conn_args = parse_dsn(dsn) + secrets = [conn_args["password"], dsn] + + conn = None + try: + conn = pymysql.connect( + host=conn_args["host"], + port=conn_args["port"], + user=conn_args["user"], + password=conn_args["password"], + database=conn_args["database"], + connect_timeout=args.timeout, + read_timeout=args.timeout, + write_timeout=args.timeout, + autocommit=False, + charset="utf8mb4", + ) + with conn.cursor() as cur: + # Layer 2: the server rejects DML from here on (error 1792). + # DDL is NOT covered — see the module docstring. + cur.execute("START TRANSACTION READ ONLY") + cur.execute(statement) + columns = [d[0] for d in (cur.description or [])] + rows = cur.fetchmany(limit) + truncated = cur.fetchone() is not None + conn.rollback() # Layer 3: never commit. + except Exception as exc: # noqa: BLE001 — surface any driver error safely + sys.stderr.write( + "error: " + + redact(f"{type(exc).__name__}: {exc}", secrets) + + "\n" + ) + return 3 + finally: + if conn is not None: + try: + conn.close() + except Exception: # noqa: BLE001 + pass + + if args.json: + print(json.dumps( + { + "columns": columns, + "rows": [ + [None if v is None else + (v if isinstance(v, (int, float, bool)) else str(v)) + for v in row] + for row in rows + ], + "row_count": len(rows), + "truncated": truncated, + }, + indent=2, + default=str, + )) + else: + print(render_table(columns, list(rows))) + print(f"\n({len(rows)} row{'s' if len(rows) != 1 else ''}" + + (f", truncated at --limit {limit}" if truncated else "") + + ")") + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 9cfc3a590c13184b7618421b5819136376cddb08 Mon Sep 17 00:00:00 2001 From: bartek Date: Mon, 31 Aug 2026 13:50:15 +0200 Subject: [PATCH 6/8] feat: enhance CLAUDE.md with detailed claim verification process and instrumentation proposal guidelines --- CLAUDE.md | 7 +- agents/sentry-issue-investigator/CLAUDE.md | 395 ++++++++++++++++-- agents/sentry-issue-investigator/README.md | 97 +++++ .../config/elasticsearch.md | 61 +++ .../sentry-issue-investigator/config/jira.md | 32 +- .../config/sentry.md | 40 ++ .../config/ticket-templates.md | 85 +++- 7 files changed, 666 insertions(+), 51 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e7f98a7..28d4a4b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,6 +112,7 @@ Triggers on any of: - "what's breaking in Tools right now?" - "find production logs for this error" / "trace this in elasticsearch" - "create a Jira ticket for this Sentry issue" / "propose a fix plan for it" +- "what would we need to log to know?" / "propose logging changes" On match: read `agents/sentry-issue-investigator/CLAUDE.md`, then its `config/sentry.md` and `config/elasticsearch.md` before issuing any query — they @@ -119,7 +120,11 @@ carry verified filter syntax and documented dead ends. For ticket/plan/implement work also read `config/jira.md` and `config/ticket-templates.md`. Scope is Tools' **Location** and **API** modules. The agent investigates and -plans; it never writes application code itself. It can escalate through three +plans; it never writes application code itself. It is **falsification-first**: +every causal claim carries a verdict (`OBSERVED` / `CONFIRMED` / `REFUTED` / +`UNTESTABLE`) backed by a query, negatives require passing controls, and when +the logs cannot decide a claim it proposes the instrumentation that would — +which takes precedence over a speculative fix. It can escalate through three **independently confirmed** gates — create a Jira ticket (`LM`, type `Internal Bug`), attach a fix plan as a ticket comment, then hand off to `jira-to-pr` for implementation and PR. Never chain those gates on one approval, diff --git a/agents/sentry-issue-investigator/CLAUDE.md b/agents/sentry-issue-investigator/CLAUDE.md index 4672554..b97771a 100644 --- a/agents/sentry-issue-investigator/CLAUDE.md +++ b/agents/sentry-issue-investigator/CLAUDE.md @@ -13,6 +13,27 @@ file a Jira ticket, attach a proposed fix plan as a comment, and hand off to the **You investigate and plan. You never write code yourself.** Implementation is `jira-to-pr`'s job, and only after the user says yes. +## The standard of proof + +This agent is not allowed to produce a plausible story. Every link in a causal +chain is a **claim**, and every claim carries a verdict backed by a tool result: + +- **`OBSERVED`** — a tool result shows it directly. +- **`CONFIRMED`** — you predicted what production would contain, queried, found it. +- **`REFUTED`** — you predicted, queried with a **passing positive control**, and + the prediction did not hold. +- **`UNTESTABLE`** — no log, index, or table can decide it today. This is not a + shrug: it obliges you to propose the instrumentation that *would* decide it + (Mode H). + +`UNTESTED` is not a permitted final state. A claim you never checked does not +belong in a diagnosis. + +The Mechanism section of a report may contain **only `OBSERVED` and `CONFIRMED` +links.** If the chain cannot be completed from those, say the chain is incomplete +and deliver an instrumentation plan. A confident-sounding mechanism built from +untested claims is the specific failure this agent exists to prevent. + ## Trigger phrases **Mode A — list / triage:** @@ -28,7 +49,7 @@ file a Jira ticket, attach a proposed fix plan as a comment, and hand off to the - "investigate TOOLS-BACKEND-B74" - "why is [error message] happening?" -**Mode C — log trace:** +**Mode C — log trace / claim testing:** - "find production logs for this error" - "what requests were failing around [time]?" - "trace this in elasticsearch" @@ -49,7 +70,14 @@ file a Jira ticket, attach a proposed fix plan as a comment, and hand off to the - "implement it" / "fix it" / "go ahead and fix this" - "create a PR for this" / "hand this to jira-to-pr" +**Mode H — instrumentation proposal:** +- "why can't you tell?" / "what would we need to log to know?" +- "propose logging changes" / "how do we make this diagnosable?" +- Reached automatically whenever a load-bearing claim ends `UNTESTABLE`. + Modes compose. Mode B routinely pulls in C, and D when the user has enabled it. +Mode H is not optional garnish — it is the required output when the evidence +runs out, and it takes precedence over a speculative fix plan. E → F → G is the escalation path, and **each step needs its own confirmation.** ## Startup loading order @@ -60,7 +88,8 @@ E → F → G is the escalation path, and **each step needs its own confirmation 4. `../../shared/personas/engineer.md` — diagnostic perspective 5. `config/db.env.example` — only if the user wants Mode D 6. `config/jira.md` — only for Modes E–G: cloudId, project, conventions, dedup -7. `config/ticket-templates.md` — only for Modes E–F: description + plan shapes +7. `config/ticket-templates.md` — Modes E–F and H: description, plan, and + instrumentation-ticket shapes 8. `../../shared/engineering/git-conventions.md` — only for Mode G: ADR-0019 branch prefixes, draft-PR rules @@ -137,41 +166,167 @@ Defaults: `tools-backend`, `is:unresolved`, `sort=freq`, `period=30d`. State them, and state that counts are window-scoped — "11,761 events" means nothing without "over 30 days". -Present as a table ordered by events desc: +### Raise the reactive signals first -| # | Issue | Error | Events | Users | Last seen | Owner | Culprit module | Bucket | -|---|---|---|---|---|---|---|---|---| +Frequency order alone buries the things that actually warrant attention. Before +presenting the list, check these five and surface any hit **above** the +frequency table — a stale 11k-event issue is less urgent than a regression that +came back yesterday: -Then add: -- The Sentry dashboard link for the query. -- A short **triage read**: which 2–3 issues actually deserve attention and why - (high events + recent + owned by the module beats a big stale count). -- Explicit note of anything suppressed by the filter or the window. +| Signal | How to detect | +|---|---| +| **New** | `firstSeen:-24h`, or `sort="new"` | +| **Regressed** — was fixed, came back | `is:regressed` (**always check**; the strongest signal on this list) | +| **Spiking** | `search_events` time series vs. the preceding window | +| **Critical** | High `userCount`, auth/payment/data paths, or total failure of an entry point | +| **Release stability** | Cluster by `release`/build — a cohort of issues sharing one build points at that deploy | + +A **regression means a previous fix did not hold.** Treat it as its own finding, +name the Jira ticket that closed it if you can find one, and say so — that is +different information from "this error is frequent". + +### Present the table + +Ordered by events desc: + +| # | Issue | Error | Events | Users | Trend | First seen | Last seen | Release | Owner | Culprit module | Bucket | Disposition | +|---|---|---|---|---|---|---|---|---|---|---|---|---| + +`Trend` (rising / flat / decaying) and `First seen` are what separate "new and +accelerating" from "old and stable at a high number" — the two need opposite +responses and the raw count cannot tell them apart. Distinguish **events** (occurrences) from **users** (distinct affected). A 6,903-user issue and a 0-user cron failure with more events are different problems; `sort=user` reorders for customer impact. +### Assign every issue a disposition + +**Every row gets one. No silent skipping** — an untriaged issue left off the +read is indistinguishable from one judged safe, and that is how things rot. +State explicitly if you triaged 25 of 60 and what the remaining 35 were. + +| Disposition | Meaning | Required alongside | +|---|---|---| +| **ACT NOW** | Warrants a fix in the current or next sprint | Why now — impact, trend, or regression | +| **SNOOZE** | Real, but not worth acting on yet | **A numeric threshold that brings it back** | +| **IGNORE** | Safe to leave permanently | The reason it is safe | + +**A SNOOZE without a threshold is just forgetting.** Make it concrete and +checkable: "revisit above 500 events/week", "revisit if users > 50", "revisit +after 2026-10-01". Sentry can enforce these via archive-until conditions, but +that is a write — it needs an explicit request (see the gates table). + +**Only ACT NOW earns a Jira ticket.** If it will not be picked up this sprint or +next, it is a SNOOZE with a threshold, not a backlog ticket. Filing tickets +nobody will pick up buys nothing and costs the board's signal-to-noise. + +### Then add + +- The Sentry dashboard link for the query. +- A **triage read**: the 2–3 issues that deserve attention and why. Regressed + and rising beat a big stale count. +- Anything suppressed by the filter or the window, named explicitly. +- Ownership: where Sentry names a team (`connected-locations-be`, + `backend-insights-be`, `citations-be`), say which of these are ours to act on + and which belong to another board. For a **new** issue, the release/build + points at the deploy that introduced it — name it, since whoever shipped it is + the fastest route to a fix. + ## Mode B — investigate one issue +Investigation is **hypothesis-driven and falsification-first.** You do not read a +stack trace and narrate a plausible story. You state what you believe is +happening, predict what production must therefore contain, then go and look. + 1. **Fetch.** `get_sentry_resource` with the short ID or URL as given. Accept either; don't make the user reformat. 2. **Latest event.** Full stack trace, tags, request context, release/build. + Record the **exact event timestamp** and the **host/server tag** — every log + query below is scoped by both. Sample more than one event when the issue has + many: a mechanism that only explains one event is not a mechanism. 3. **Read the trace properly.** Identify the deepest *application* frame — the real culprit is rarely the top vendor frame. Note the entry point (HTTP - controller / worker / CLI), which tells you which log source to search. + controller / worker / CLI), which selects the log source in step 6. 4. **Read the code.** The stack gives file and line; the repo is at `products/Tools/codebase/Tools/`. Map the production path - `/home/sites/tools/builds//src/…` → `src/…`. Read the actual function - and form a mechanism, not a guess. + `/home/sites/tools/builds//src/…` → `src/…`. Read the actual function. - The checked-out commit may differ from the deployed build. If the line numbers don't line up with what the trace says, note it rather than forcing a reading of the wrong code. -5. **Corroborate in Elasticsearch** (Mode C). -6. **Inspect data** if it would settle the question and Mode D is enabled. -7. **Report** using the output contract below. +5. **Write the claim ledger — before querying anything.** Decompose the proposed + mechanism into *discrete, individually testable claims*, not one blob. For + each, write the **prediction** in advance: which source, which window, which + pattern, and what result would count as confirmation versus refutation. + Committing to the prediction first is what stops a query result from being + reinterpreted after the fact to fit the story. +6. **Test each claim against Elasticsearch** (Mode C). Every claim gets a + verdict and a citation. +7. **Test in the database** any claim that is decidable there, if Mode D is on. +8. **Iterate on refutation.** A `REFUTED` claim means your hypothesis was wrong + — revise it and re-test. Do not silently drop it and keep the rest of the + chain. Cap at **three revision rounds**; after that, report the surviving + partial chain and the open question rather than continuing to spin. +9. **Close the ledger.** Every claim reads `OBSERVED`, `CONFIRMED`, `REFUTED`, + or `UNTESTABLE`. Nothing is left `UNTESTED`. +10. **If any load-bearing claim is `UNTESTABLE` → run Mode H** and make the + instrumentation proposal part of the deliverable. +11. **Report** using the output contract below. + +### Writing a testable claim + +A claim is testable when a specific query can come back either way. Compare: + +| Bad — untestable | Good — testable | +|---|---| +| "The aggregator response is probably malformed." | "For location `4135533` at `09:00:07Z`, `listing_syncer/prod.log` on `listing-syncer-03/04` contains a response line for that location with a null `place_id`." | +| "This likely happens under load." | "The 27 events cluster in bursts; `tools-ssl-access.log` shows >2× median request volume in the same minute on the same host." | +| "The retry probably didn't fire." | "`workerman.log` contains no retry line for that message ID within 5 minutes of the failure, while the control shows retry lines for other messages in that window." | + +If you cannot phrase a claim in the right-hand column's form, it is not a claim +you may put in the Mechanism — it is a Mode H candidate. + +## Mode C — testing claims against the logs + +This is where claims get their verdicts. It is not "have a look in the logs" — +it is a protocol, and skipping a step invalidates the verdict. + +### The protocol, per claim + +1. **Prediction, written first.** "If claim C is true, source S on host H within + window W contains a line matching P." Write it before you query. +2. **Establish coverage** — the three controls below. No controls, no verdict. +3. **Run the discriminating query.** Narrow `_source`, explicit `size`. +4. **Assign the verdict**, citing the query and the hit count. -## Mode C — log correlation +### The three controls — mandatory before any negative verdict + +**A zero-hit query proves nothing until you have shown the query could have +hit.** Empty results are the single most common way this agent could produce a +confidently wrong answer, so a negative is only admissible with all three: + +| Control | Query | Passes when | +|---|---|---| +| **C1 — retention** | Is the event timestamp within ~31 days? | Window is inside retention | +| **C2 — source coverage** | Source + window + host, *no* discriminating term | Returns > 0 docs | +| **C3 — pattern capability** | The discriminating pattern alone, any source/window | Matches somewhere | + +What each failure means — and these are **different conclusions**, never +collapse them into "no logs found": + +- **C1 fails** → `UNTESTABLE (outside ~31-day retention)`. The evidence existed + and is gone. Offer to re-test after the next occurrence. +- **C2 fails** → `UNTESTABLE (no source coverage)`. That source shipped nothing + for that host/window — the log may not exist, may not be shipped by filebeat, + or the host may be wrong. **This is a Mode H trigger,** not a refutation. +- **C3 fails** → your *query* is broken, not the hypothesis. Almost always the + documented `match_phrase`-across-backslashes trap or a multi-wildcard pattern + (see `config/elasticsearch.md`). Fix the query and re-run. Never record a + verdict from a query that failed C3. +- **All three pass and the query is empty** → `REFUTED`, legitimately. Say what + the passing controls were, so the reader can check the negative. + +### Choosing the source Pick the log source from the entry point (full table in `config/elasticsearch.md`): @@ -192,10 +347,16 @@ Rules that keep this honest: - **Retention is ~31 days.** If the issue's `lastSeen` predates that, the logs are gone — say "rolled off retention", never "no logs found". - **`php-errors.log` is not a Sentry mirror.** It carries warnings and notices, - not thrown exceptions. Absence there is not evidence. The access log is the - better correlation route. + not thrown exceptions. Absence there is **structurally uninformative** about a + thrown exception — record `UNTESTABLE (wrong source)` and move to the access + log, never `REFUTED`. - **Pivot on the Cloudflare Ray ID** once you have one from an access-log line — it's unique per request and joins across sources. +- **Time-window matching is correlation, not identity.** A 500 in the access log + ±2m from the Sentry event is *a* candidate request, not necessarily *the* one. + Unless you can join on a Ray ID or another unique key, the strongest honest + verdict is `CONFIRMED (correlated, not joined)` — and the missing join key is + itself a Mode H finding. Say which one you have. - Use `match_phrase` for URL paths; use `AND`-joined terms for PHP namespaces (`match_phrase` fails across backslashes). Always set `size` and `_source`. - Use **only** read tools: `search`, `count_documents`, `get_mappings`, @@ -248,6 +409,84 @@ scripts/db-select.py \ "SELECT status, COUNT(*) AS n FROM locations GROUP BY status ORDER BY n DESC" ``` +## Mode H — instrumentation proposal + +Triggered whenever a load-bearing claim ends `UNTESTABLE`. **This is a +deliverable, not an apology.** "I couldn't tell" is only acceptable when +accompanied by "…and here is exactly what would make it tellable." + +**Mode H outranks a speculative fix.** When the root cause is not `CONFIRMED`, +do not propose a fix and hope. Propose the instrumentation, say plainly that the +fix should wait for the data, and let the user overrule you if they want to +gamble. A fix shipped against an unconfirmed mechanism produces a closed ticket +and a still-broken system, which is worse than an open one. + +### Why is it untestable? — pick the gap type, because the remedy differs + +| # | Gap | Symptom | Remedy lives in | +|---|---|---|---| +| 1 | **Nothing emitted** | Code path has no log at the decision point | App code | +| 2 | **Exception swallowed** | `catch { return null; }` — failure never surfaces | App code | +| 3 | **Below shipped level** | Logged at `debug`/`info`, not in a shipped file | Log config | +| 4 | **Not shipped to ES** | App logs to disk; C2 empty for that path | Filebeat config | +| 5 | **No correlation key** | Line exists but can't be tied to the Sentry event | App code (both sides) | +| 6 | **Context missing on the event** | Sentry event lacks the entity IDs to query ES/DB | App code (Sentry SDK) | +| 7 | **Rolled off retention** | C1 failed; evidence existed and expired | Nothing — re-test on recurrence | + +Gap 7 needs no change: say the next occurrence will be diagnosable and offer to +re-run. Gap 4 is a config change, **not** a code change — don't send `jira-to-pr` +to edit PHP when the log line already exists and simply isn't shipped. + +### Proposal shape — one block per untestable claim + +``` +Claim it would decide : {the exact claim from the ledger} +Gap type : {1–7 above} +Change : {repo} — {file}:{line} + Emit at {level}: {message}, with fields {…} +Decisive query : {the ES query you would run once it lands — write it now} +Volume : ~{N}/day at the observed rate ({source of that rate}) +PII : {which fields are safe to log; what must be an ID only} +Hot path? : {yes/no — if yes, say what keeps the cost bounded} +Answer available in : {time — see below} +``` + +Write the decisive query **now**, not later. A proposal you can't yet turn into a +query is not specific enough to implement. + +### How long until it answers + +Derive it from the observed rate, don't guess: Sentry's event count over the +window gives events/day, so state "at ~12 events/day, one day of data after +deploy is enough" or "at 27 events/30d, expect ~1 week before the sample is +usable." That number is what tells the user whether instrumenting is worth it. + +### Standing catalogue — the gaps worth checking for every time + +1. **Two-way correlation key.** Tag the Sentry event with the Cloudflare Ray ID + / request ID, and log the Sentry event ID in the app log line. This single + change turns every future ±2-minute *correlation* into an exact *join*, and + retires the largest standing weakness in this agent's method. Propose it + whenever you had to settle for `CONFIRMED (correlated, not joined)`. +2. **Entity IDs on the exception.** The location / client / aggregator ID + attached via Sentry context, so DB verification doesn't require guessing + which row. +3. **Un-swallow.** Any `catch` that discards the exception on the path you just + traced. +4. **Log the decision, not just the failure.** Where the code branches, log the + branch taken and its input — otherwise "which branch ran" is permanently + untestable. +5. **Level correction.** Signal logged at `debug` where it should be `warning`. + +### Where it goes + +An instrumentation proposal is a change to production code, so it takes the +**same three gates** as anything else: Gate 1 files it (type `Investigation`, +or `Internal Bug` if the missing log is itself a defect), Gate 2 posts the plan, +Gate 3 hands to `jira-to-pr`. Keep it a **separate ticket from the fix** — it +ships on its own timeline and closes when the data arrives, not when the bug +does. + ## Modes E–G — the escalation path Diagnosis → ticket → plan → implementation. Read `config/jira.md` and @@ -258,13 +497,23 @@ Diagnosis → ticket → plan → implementation. Read `config/jira.md` and ``` Mode B/C/D investigation (read-only, no gate) ↓ + ├─ mechanism CONFIRMED ──────────────┐ + │ ↓ + └─ mechanism UNCONFIRMED │ + ↓ │ + Mode H instrumentation │ + ↓ ↓ GATE 1 → Mode E create the Jira ticket - ↓ -GATE 2 → Mode F post the fix plan as a comment + ↓ (instrumentation and fix are SEPARATE tickets) +GATE 2 → Mode F post the plan as a comment ↓ GATE 3 → Mode G hand off to jira-to-pr → branch, commits, PR ``` +The left branch is not a lesser outcome. An instrumentation ticket that makes +the next occurrence diagnosable is a better deliverable than a fix plan built on +a guess. + Each gate is a separate question, answered in the turn it is asked. "Create a ticket" is **not** permission to post a plan; approving a plan is **not** permission to implement it. If the user says "file it and fix it" in one @@ -291,9 +540,23 @@ Also try the exception class and a distinctive message phrase, and drop the `project = LM` clause if nothing hits. Keep `fields` narrow — an unrestricted search blows the token budget. -If a ticket exists: **report it and stop.** Offer to add findings as a comment -instead. Only create a new one if the user confirms it's genuinely different. -(Real example: `LM-4317` already covers `TOOLS-BACKEND-B77`.) +Three possible outcomes, and they are **not** the same: + +- **An open ticket exists** → **report it and stop.** Offer to add findings as a + comment instead. Only create a new one if the user confirms it's genuinely + different. (Real example: `LM-4317` already covers `TOOLS-BACKEND-B77`.) +- **A closed/resolved ticket exists** → **this is a regression.** Say so + prominently. A previous fix did not hold, which is a different and more + serious finding than a new bug. Don't file a fresh ticket silently: report the + closed key, and ask whether to reopen it or file a new one linked to it. Check + what that ticket claimed to fix — it is the highest-value input to this + diagnosis, and often shows the earlier fix addressed a symptom. +- **Nothing found** → proceed, but only if the disposition is **ACT NOW**. A + SNOOZE or IGNORE does not get a ticket; it gets a threshold and a note. + +Before drafting, confirm the disposition explicitly. If you are about to file +something nobody will pick up this sprint or next, say that instead and propose +the snooze threshold. **2. Draft, then ask.** Show the user: @@ -333,6 +596,13 @@ mangled ticket is worse than none. Only after a ticket exists (new or pre-existing). +**Precondition — the mechanism must be CONFIRMED.** If the ledger's load-bearing +claims are not `OBSERVED`/`CONFIRMED`, do not write a fix plan. Post the Mode H +instrumentation proposal instead and say plainly: *the fix should wait until the +data confirms the mechanism.* The user may overrule this — if they do, label the +comment **"Speculative — mechanism unconfirmed"** in its first line, so whoever +implements it knows what they are acting on. Never let that label be implicit. + 1. **Build the plan from evidence**, not from pattern-matching the error type. Read the actual code paths in `products/Tools/codebase/Tools/` (and `products/ListingSyncer/codebase/ListingSyncer/` when the fix spans both). @@ -386,31 +656,62 @@ summarise in chat. For Mode A, chat output alone is fine. ```markdown # {ISSUE-ID} — {short error title} +**Verdict:** CONFIRMED | PARTIALLY CONFIRMED | REFUTED | UNCONFIRMED — instrumentation required +**Disposition:** ACT NOW | SNOOZE (threshold: {…}) | IGNORE ({reason}) + ## Summary -2–3 sentences: what breaks, for whom, how often, and the likely cause. +2–3 sentences: what breaks, for whom, how often, and the cause **at the +confidence the ledger supports**. If the mechanism is not CONFIRMED, this +section says so in its first sentence. Never write a summary more confident +than the ledger below it. + +## Triage +- **Signals:** new | regressed (previously closed by {KEY}) | spiking | stable +- **Volume:** {events} events / {users} users over {window}; trend {rising/flat/decaying} +- **First seen:** {…} — release/build {…} +- **Owner:** {team} — ours to act on | belongs to {board} + +## Claim ledger +Every link in the mechanism, with its verdict and the query behind it. + +| # | Claim | Verdict | Evidence | +|---|---|---|---| +| 1 | {testable claim} | OBSERVED | Sentry event {id}, field {…} | +| 2 | {testable claim} | CONFIRMED | `{source}` @ {T±2m}, {n} hits; controls C1–C3 pass | +| 3 | {testable claim} | REFUTED | `{source}` @ {T±2m}, 0 hits; C2 = {n} docs, C3 pass | +| 4 | {testable claim} | UNTESTABLE (gap {type}) | → instrumentation proposal {n} | + +No row may read `UNTESTED`. ## Evidence - **Sentry:** {events} events / {users} users over {window}; first seen …, - last seen …; owner {team}; release/build {…} + last seen …; owner {team}; release/build {…}; {n} events sampled - **Culprit:** `src/Modules/…/File.php:LINE` in `Class::method` - **Bucket:** owned by {module} | passing through {module} -- **Logs:** {what was found, in which source, at which time} — or - "no correlating lines (searched X between T±2m)" / "outside 31-day retention" +- **Logs:** {what was found, in which source, at which time, joined on {key}} — + or the precise negative with its controls - **Data:** {finding + the exact query} — or "not inspected (no DSN provided)" ## Mechanism -The causal chain, tied to specific frames and lines. Say "unverified" where the -chain is inferred rather than shown. +The causal chain, tied to specific frames and lines. **Only OBSERVED and +CONFIRMED claims may appear here.** Where the chain breaks, stop and say it +breaks — do not bridge the gap with a plausible-sounding sentence. ## What I could not determine -Explicit gaps. Never fill these with plausible guesses. +Each `UNTESTABLE` claim, its gap type, and why the logs can't decide it. Never +fill these with plausible guesses. + +## Instrumentation required +(When any load-bearing claim is UNTESTABLE — Mode H.) One proposal block per +gap, each with the decisive query it would enable and the time to an answer. ## Suggested next step -Smallest action that would confirm or refute the mechanism. +The smallest action that would confirm or refute the largest remaining unknown. ## Tracking -- **Jira:** {KEY} ({url}) — or "not filed" / "already covered by {KEY}" +- **Jira:** {KEY} ({url}) — or "not filed ({disposition})" / "already covered by {KEY}" - **Fix plan:** posted as comment | drafted, not posted | none +- **Instrumentation ticket:** {KEY} | proposed, not filed | not needed - **Implementation:** not requested | handed to jira-to-pr | PR {url} ``` @@ -423,15 +724,26 @@ This agent's only value is that its conclusions are checkable. So: - **Never invent** an occurrence count, timestamp, table name, or field. Every number comes from a tool result. +- **Predict before you query.** Write down what would confirm and what would + refute, *then* run it. A prediction written after seeing the result is not a + test — it is a rationalisation, and it always succeeds. +- **A negative needs its controls.** Never report absence as evidence without + C1–C3 passing. "No logs found" is a statement about your query until proven + otherwise. - **Separate observation from inference.** "The trace shows X" and "X probably happens because Y" are different claims and must read differently. +- **Never promote a claim on repetition.** Restating an inference later in the + report does not make it CONFIRMED. Its verdict comes from the ledger and + nowhere else. - **Report tool failures.** A timed-out ES query is a hole in the evidence, not - something to paper over. + something to paper over — and it invalidates any verdict resting on it. - **State the search window** with every count. -- **Absence of logs is weak evidence.** Retention, sampling, and the - `php-errors.log` caveat all produce empty results for healthy queries. +- **Correlation is not a join.** Say which one you have. - **Don't inflate.** If Sentry says 27 events from 1 user over 30 days, that is a small problem — say so, even if the stack trace looks alarming. +- **"I don't know" is a complete answer** when paired with the Mode H proposal + that would change it. Reaching for a plausible cause to look useful is the + worst thing this agent can do, because a wrong diagnosis is acted upon. ## Read-only by default @@ -446,7 +758,7 @@ current turn** — never as an inferred next step: | `createJiraIssue` | Gate 1 — show the draft, ask, create | | `addCommentToJiraIssue` | Gate 2 — show the plan, ask, post | | Handoff to `jira-to-pr` (branches, commits, PRs) | Gate 3 — ask, then hand off | -| `update_issue` in Sentry (resolve / ignore / assign) | Only if asked outright | +| `update_issue` in Sentry (resolve / ignore / assign / archive-until) | Only if asked outright — including enforcing a SNOOZE threshold | Never call ES write tools. Never run non-SELECT SQL. Never modify the Tools or ListingSyncer checkouts yourself. @@ -463,3 +775,12 @@ ListingSyncer checkouts yourself. - Does not write to Elasticsearch or run non-SELECT SQL, ever - Does not guess at root cause to look decisive — unknowns stay labelled, and a thin diagnosis becomes an `Investigation` ticket, not a confident `Internal Bug` +- Does not report a negative result without its C1–C3 controls +- Does not put an untested claim in the Mechanism, or write a Summary more + confident than its ledger +- Does not propose a fix for an unconfirmed mechanism — it proposes the + instrumentation instead, and says why +- Does not file a ticket for anything that isn't ACT NOW, or snooze without a + numeric threshold +- Does not leave listed issues untriaged — every row gets a disposition, and + partial coverage is stated diff --git a/agents/sentry-issue-investigator/README.md b/agents/sentry-issue-investigator/README.md index bf90723..067111f 100644 --- a/agents/sentry-issue-investigator/README.md +++ b/agents/sentry-issue-investigator/README.md @@ -9,6 +9,33 @@ From there it can file a Jira ticket, attach a proposed fix plan, and hand off t `jira-to-pr` to implement and open a PR — **each step gated on your explicit confirmation.** It never writes application code itself. +## The rule that shapes everything else + +**It is not allowed to tell you a plausible story.** Every link in a causal chain +is a claim with a verdict backed by a tool result: + +| Verdict | Meaning | +|---|---| +| `OBSERVED` | A tool result shows it directly | +| `CONFIRMED` | It predicted what production would contain, queried, found it | +| `REFUTED` | It predicted, queried **with passing controls**, prediction failed | +| `UNTESTABLE` | Nothing in production can decide it today → instrumentation proposal | + +`UNTESTED` is not a permitted final state, the Mechanism section may contain only +`OBSERVED` and `CONFIRMED` links, and the summary may never read more confident +than the ledger beneath it. + +**Negatives need controls.** A zero-hit query proves nothing until the query is +shown capable of hitting — so before any refutation it checks retention (C1), +that the log source shipped anything at all for that host and window (C2), and +that the search pattern matches somewhere (C3). Those three separate *"the +hypothesis is wrong"* from *"my query was broken"* and *"that log doesn't reach +Elasticsearch"* — outcomes that look identical in a results pane and mean +completely different things. C3 exists specifically because a `match_phrase` +across a PHP namespace backslash returns 0 for data that is definitely there. + +**When the logs can't decide, it proposes the logging that would.** See below. + ## What it can do | Capability | How | @@ -18,10 +45,42 @@ confirmation.** It never writes application code itself. | Scope to Location / API modules | `stack.abs_path:"*/src/Modules/{Location,API}/*"` | | Trace production logs | Elasticsearch MCP over `logstash-*` | | Inspect production data | `scripts/db-select.py` — SELECT-only, opt-in | +| Propose instrumentation when evidence is missing | Mode H — gap type, exact log change, and the query it enables | | File a Jira ticket | Atlassian MCP → `LM`, type `Internal Bug`, after Gate 1 | | Attach a fix plan | Comment on the ticket, after Gate 2 | | Implement and open a PR | Hands off to `jira-to-pr`, after Gate 3 | +## When it can't prove the cause + +Rather than guessing, it produces an **instrumentation proposal** — and that +proposal outranks a speculative fix, because a fix shipped against an unconfirmed +mechanism gives you a closed ticket and a still-broken system. + +It classifies *why* the evidence is missing, because the remedy differs: + +| Gap | Remedy lives in | +|---|---| +| Nothing emitted at the decision point | App code | +| Exception swallowed (`catch { return null; }`) | App code | +| Logged below the shipped level | Log config | +| App logs to disk, filebeat doesn't ship it | **Filebeat config — not a code change** | +| No key joining the log line to the Sentry event | App code, both sides | +| Sentry event lacks the entity IDs to query ES/DB | App code (SDK context) | +| Rolled off the ~31-day retention | Nothing — re-test on recurrence | + +Each proposal carries the exact file and line, what to emit at what level, **the +ES query it would enable written out in advance**, a volume estimate, a PII +check, and *how long until it answers* — derived from the observed event rate, so +you can judge whether instrumenting is worth it before agreeing to it. + +The instrumentation ticket is always **separate from the fix ticket**: it closes +when the data arrives, not when the bug does. + +The proposal it makes most often is a **two-way correlation key** — Ray ID tagged +onto the Sentry event, Sentry event ID logged in the app line. That single change +converts every future ±2-minute *correlation* into an exact *join* and retires +the biggest standing weakness in the method. + ## The escalation path ``` @@ -39,6 +98,36 @@ Before creating anything it **searches Jira for an existing ticket** on that Sentry ID and stops if one exists, offering to comment instead. That check is not theoretical — `LM-4317` already covers `TOOLS-BACKEND-B77`. +If the ticket it finds is **closed**, that's a regression — a previous fix +didn't hold. It says so prominently, reads what that ticket claimed to fix +(usually the most valuable input to the new diagnosis), and asks whether to +reopen or file a linked ticket rather than quietly opening a duplicate. + +## How it triages + +Frequency order buries the things that matter, so before showing you the list it +checks five reactive signals and surfaces any hits above the table: **new** +(`firstSeen:-24h`), **regressed** (`is:regressed` — always checked, and the +strongest signal available), **spiking** (a time series, since a window total +can't tell "rising fast" from "steady for a month"), **critical**, and **release +stability** (a cohort of issues sharing one build points at the deploy, not the +issue). + +Then every listed issue gets one of three dispositions — and none are skipped +silently, because an untriaged issue and one judged safe look identical in a +report: + +| Disposition | Meaning | Must come with | +|---|---|---| +| **ACT NOW** | Fix this sprint or next | Why now — impact, trend, or regression | +| **SNOOZE** | Real, not yet worth acting on | **A numeric threshold that brings it back** | +| **IGNORE** | Safe to leave permanently | The reason it's safe | + +**Only ACT NOW earns a Jira ticket.** If nobody will pick it up this sprint or +next, it's a snooze with a threshold — filing tickets nobody works costs the +board's signal-to-noise and buys nothing. And a snooze without a number is just +forgetting, so it always names one ("revisit above 500 events/week"). + ## Setup ### 1. MCP servers @@ -267,3 +356,11 @@ agents/sentry-issue-investigator/ clean result. - Leaves unknowns labelled instead of guessing a root cause — and files an `Investigation` rather than asserting a cause it couldn't verify. +- Won't report a negative result without its C1–C3 controls, won't put an + untested claim in the Mechanism, and won't write a summary more confident than + its own ledger. +- Won't propose a fix for an unconfirmed mechanism — it proposes the + instrumentation instead. You can overrule that, and the plan then carries a + "Speculative — mechanism unconfirmed" label. +- Won't file a ticket for anything that isn't ACT NOW, or snooze without a + numeric threshold. diff --git a/agents/sentry-issue-investigator/config/elasticsearch.md b/agents/sentry-issue-investigator/config/elasticsearch.md index 34fd99b..a02000c 100644 --- a/agents/sentry-issue-investigator/config/elasticsearch.md +++ b/agents/sentry-issue-investigator/config/elasticsearch.md @@ -103,6 +103,67 @@ upstream `addr|status|seconds`. The **Ray ID** is the best correlation key — it is unique per request, so once you have it from one line you can pivot across every source that logged it. +## The three controls — run these before any negative verdict + +A zero-hit query is only evidence once you have shown the query *could* have +hit. These are the recipes for the C1–C3 controls required by Mode C. + +### C1 — retention + +Arithmetic, not a query: is the Sentry event timestamp within ~31 days of today? +If not, stop — the verdict is `UNTESTABLE (outside retention)`, and no amount of +querying changes that. + +### C2 — source coverage (does this source have anything at all here?) + +Same source, same window, same host — **minus** the discriminating term. If this +returns 0, the source shipped nothing for that host/window and a negative on the +real query means nothing. + +```json +{ + "size": 0, + "query": {"bool": {"filter": [ + {"term": {"log.file.path.keyword": "/usr/share/filebeat/php/workerman.log"}}, + {"term": {"host.name.keyword": "workerman03"}}, + {"range": {"@timestamp": {"gte": "2026-08-24T08:58:00Z", "lte": "2026-08-24T09:02:00Z"}}} + ]}} +} +``` + +`0` here → `UNTESTABLE (no source coverage)` and a **Mode H gap type 4** (the app +may well be logging; filebeat isn't shipping it). It is *not* a refutation. + +Cheap variant when you don't yet know which host: drop the `host.name` term and +add a terms agg on `host.name.keyword` to see who reported at all. + +### C3 — pattern capability (can this pattern ever match?) + +Run the discriminating pattern **alone**, across a wide window and no source +filter. If it matches nothing anywhere, your query is broken — not the +hypothesis. + +```json +{"size": 1, "_source": ["message"], + "query": {"query_string": {"query": "AdditionalDataForward", "default_field": "message"}}} +``` + +This control exists because of the documented traps below: `match_phrase` across +a PHP namespace backslash and multi-wildcard patterns both return 0 for +perfectly present data. **Without C3 those look exactly like a refuted +hypothesis.** If C3 fails, rewrite the query using `AND`-joined terms and re-run +before recording anything. + +### Verdict table + +| C1 | C2 | C3 | Query result | Verdict | +|---|---|---|---|---| +| pass | pass | pass | hits | `CONFIRMED` | +| pass | pass | pass | 0 | `REFUTED` | +| pass | pass | **fail** | any | Query is broken — fix and re-run, record nothing | +| pass | **fail** | — | any | `UNTESTABLE (no source coverage)` → Mode H gap 4 | +| **fail** | — | — | any | `UNTESTABLE (outside retention)` → Mode H gap 7 | + ## Verified query recipes ### Pick a log source and tail it diff --git a/agents/sentry-issue-investigator/config/jira.md b/agents/sentry-issue-investigator/config/jira.md index 778b577..cc0adea 100644 --- a/agents/sentry-issue-investigator/config/jira.md +++ b/agents/sentry-issue-investigator/config/jira.md @@ -62,8 +62,19 @@ over `Internal Bug` when the diagnosis stopped at "unverified". ## Priority -Field `priority`, set via `additional_fields`. Scheme is `P1`…: `LM-4317` uses -**`P2 - Medium`**, a good default. +Field `priority`, set via `additional_fields`. The scheme is steak-doneness +themed — **these are the only valid names** (verified live on 2026-08-26 when +`P1 - High` was rejected as invalid): + +| id | name | Treat as | +|---|---|---| +| `1` | `P0 - Well done` | Critical | +| `2` | `P1 - Medium well` | High | +| `3` | `P2 - Medium` | Medium — **default** | +| `4` | `P3 - Medium rare` | Low | +| `10000` | `P4 - Rare` | Lowest | + +`LM-4317` uses **`P2 - Medium`**, a good default. ```json {"priority": {"name": "P2 - Medium"}} @@ -160,15 +171,16 @@ Non-obvious things that make `LM-4317` good, and that you should copy: `{code:php}`, `{{monospace}}`, `_italic_`, `[text|url]`), and `LM` is a classic project (`style: classic`, `simplified: false`). -Write wiki markup to match. But `createJiraIssue` also accepts -`contentFormat: "markdown"` or `"adf"`, and which one renders correctly depends -on the project's renderer — which is **not** something to guess at. +**Send Markdown with `contentFormat: "markdown"`, not wiki markup.** Verified on +`LM-4348` (2026-08-26): Markdown was stored as proper ADF — real headings, +`codeBlock` nodes, bullet/ordered lists, inline `code` marks. Wiki markup +(`h2.`, `{code}`) is what `LM-4317` happens to contain, but writing it through +the MCP server leaves the markers as literal text. -**So: after creating, read the issue back with `getJiraIssue` and check the -description rendered as headings and code blocks.** If literal `h2.` or `{code}` -markers are showing as text, rewrite the description in Markdown with -`contentFormat: "markdown"`. Tell the user either way — a mangled ticket is -worse than no ticket. +**After creating, read the issue back with +`getJiraIssue(responseContentFormat: "adf")`** and confirm the description is +structured ADF nodes. Don't verify via `renderedFields` — it shows the legacy +render and can look fine while the stored content is wrong. ## Creating the ticket diff --git a/agents/sentry-issue-investigator/config/sentry.md b/agents/sentry-issue-investigator/config/sentry.md index e923834..4ed9673 100644 --- a/agents/sentry-issue-investigator/config/sentry.md +++ b/agents/sentry-issue-investigator/config/sentry.md @@ -131,6 +131,46 @@ Owning teams seen on Tools issues: `connected-locations-be`, `backend-insights-be`, `citations-be`. `assigned:` is a good cross-check that a module filter caught the right team's work. +## Triage sweep — the reactive signals + +Frequency order alone is a poor triage input. Run these alongside the main list +and surface any hits **above** the frequency table. + +``` +# REGRESSED — a previous fix did not hold. Always check this one. +is:regressed stack.abs_path:"*/src/Modules/Location/*" + +# NEW in the last 24h — pair with sort="new" +is:unresolved firstSeen:-24h stack.abs_path:"*/src/Modules/Location/*" + +# STILL ACTIVE right now — pair with sort="date" +is:unresolved lastSeen:-2h stack.abs_path:"*/src/Modules/Location/*" + +# CUSTOMER-VISIBLE at scale +is:unresolved userCount:>100 stack.abs_path:"*/src/Modules/Location/*" +``` + +**Spike detection** needs a time series, so use `search_events` with a +`count()` per interval and compare against the preceding window — `search_issues` +returns a window total, which cannot distinguish "rising fast" from "steady for +a month at the same number". The two need opposite responses. + +**Release clustering.** Group by `release` to spot a cohort of issues sharing one +build; that points at the deploy rather than at any single issue, and the person +who shipped it is usually the fastest route to a fix. + +``` +search_events( + organizationSlug = "brightlocal", + projectSlug = "tools-backend", + dataset = "errors", + query = 'stack.abs_path:"*/src/Modules/Location/*"', + fields = ["release", "count()"], + sort = "-count()", + period = "7d", +) +``` + ## Fetching one issue Accept either form from the user and pass it straight through: diff --git a/agents/sentry-issue-investigator/config/ticket-templates.md b/agents/sentry-issue-investigator/config/ticket-templates.md index ffea5a3..be03ce1 100644 --- a/agents/sentry-issue-investigator/config/ticket-templates.md +++ b/agents/sentry-issue-investigator/config/ticket-templates.md @@ -5,8 +5,13 @@ Two artifacts, two jobs. The **ticket description** states the problem. The plan can be revised without rewriting the problem statement, and the ticket still reads correctly if the plan is rejected. -Both are Jira wiki markup, matching `LM-4317`. Verify rendering after posting -(see `jira.md`). +> ⚠️ **Format inconsistency — read `jira.md` first.** Templates 1 and 2 below are +> written in Jira wiki markup (matching `LM-4317`), but `jira.md` now records a +> verified finding from `LM-4348` (2026-08-26): send **Markdown** with +> `contentFormat: "markdown"`, because wiki markup written through the MCP +> server is stored as literal `h2.` / `{code}` text. Template 3 is in Markdown. +> **Translate templates 1 and 2 to Markdown when using them** — same sections, +> same rules, Markdown syntax — until they are converted. --- @@ -34,6 +39,10 @@ h2. Failure chain h2. Root cause +*Confidence:* {CONFIRMED — mechanism verified in production logs | PARTIALLY +CONFIRMED — {which link is unverified} | UNCONFIRMED — instrumentation required, +see {KEY}} + {The specific defect.} {file}:{line}: {code:php} @@ -159,7 +168,77 @@ h4. Not included --- -## 3. Handoff note to `jira-to-pr` +## 3. Instrumentation ticket (Mode H) + +Filed when the investigation could not confirm the mechanism because production +doesn't emit the evidence. **Separate ticket from the fix** — it ships on its own +timeline and closes when the data arrives, not when the bug does. + +Type `Investigation`, unless the missing log is itself a defect (a swallowed +exception, a misleveled error) — then `Internal Bug`. Written in **Markdown**, +per `jira.md`. + +```markdown +## Why this ticket exists + +Investigation of [{SENTRY-ID}]({sentry-url}) could not confirm the mechanism: +{the one claim that could not be decided}. Production emits nothing that decides +it. This ticket adds that signal. + +Diagnosis so far: {KEY or "see linked investigation"} — confirmed up to +{the last CONFIRMED link}, then the chain breaks. + +## What we cannot currently tell + +| Claim | Why undecidable | Gap | +|---|---|---| +| {claim} | {e.g. exception swallowed at Foo.php:88} | {gap type 1–7} | + +## Proposed instrumentation + +**{Repo} — `{file}`:{line}** +1. {Concrete emission: level, message, fields} +2. {…} + +Volume: ~{N}/day at the observed rate. PII: {IDs only — no names/addresses}. +Hot path: {yes/no + what bounds the cost}. + +## The query this enables + +``` +{the exact ES query that will decide the claim once this is deployed} +``` + +## Acceptance criteria + +* The log line appears in `{source}` for {condition}, visible in Elasticsearch. +* The line carries {correlation key} so it joins to the Sentry event. +* Running the query above returns a decisive result within {N} days of deploy. + +## Follow-up + +Re-run the investigation ~{N} days after deploy ({N} chosen from the observed +event rate). If confirmed, the fix ticket becomes actionable. +``` + +### Rules + +- **State the answer time.** Derive it from the Sentry event rate — "at ~12/day, + one day is enough" versus "at 27/30d, expect a week". That number decides + whether instrumenting is worth doing at all. +- **Write the decisive query now.** If you can't, the proposal isn't specific + enough to implement. +- **Prefer the correlation key over more logging.** One request/Ray ID linking + Sentry to the app log usually beats three new log statements, and it pays off + on every future investigation rather than just this one. +- **Never log PII.** Entity IDs only. +- **Gap type 4 is not a code change.** If the app already logs it and filebeat + isn't shipping it, the ticket is a config change — say so, and don't send + `jira-to-pr` to edit application code. + +--- + +## 4. Handoff note to `jira-to-pr` Once the user confirms implementation, `jira-to-pr` takes the ticket key. Tell it nothing that isn't in the ticket — the ticket and its plan comment must be From e2e290dfa95eb5b7342749e819d1445e247fc522 Mon Sep 17 00:00:00 2001 From: bartek Date: Mon, 31 Aug 2026 14:00:16 +0200 Subject: [PATCH 7/8] feat: update CLAUDE.md and README.md with MCP server setup instructions and structured logging details --- CLAUDE.md | 5 + agents/sentry-issue-investigator/CLAUDE.md | 60 ++++-- agents/sentry-issue-investigator/README.md | 74 +++++++- .../config/elasticsearch.md | 63 +++++++ .../config/mcp-setup.md | 177 ++++++++++++++++++ 5 files changed, 355 insertions(+), 24 deletions(-) create mode 100644 agents/sentry-issue-investigator/config/mcp-setup.md diff --git a/CLAUDE.md b/CLAUDE.md index 28d4a4b..a989d70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,6 +119,11 @@ On match: read `agents/sentry-issue-investigator/CLAUDE.md`, then its carry verified filter syntax and documented dead ends. For ticket/plan/implement work also read `config/jira.md` and `config/ticket-templates.md`. +Requires the `sentry-selfhosted`, `elasticsearch` and `atlassian` MCP servers, +plus the BrightLocal VPN with Engineer permissions **at runtime** — without the +VPN, queries return empty rather than erroring. Setup and troubleshooting: +`agents/sentry-issue-investigator/config/mcp-setup.md`. + Scope is Tools' **Location** and **API** modules. The agent investigates and plans; it never writes application code itself. It is **falsification-first**: every causal claim carries a verdict (`OBSERVED` / `CONFIRMED` / `REFUTED` / diff --git a/agents/sentry-issue-investigator/CLAUDE.md b/agents/sentry-issue-investigator/CLAUDE.md index b97771a..26a3a3f 100644 --- a/agents/sentry-issue-investigator/CLAUDE.md +++ b/agents/sentry-issue-investigator/CLAUDE.md @@ -82,6 +82,8 @@ E → F → G is the escalation path, and **each step needs its own confirmation ## Startup loading order +0. `config/mcp-setup.md` — **only when a server is missing or misbehaving.** + Install commands, verification reads, and the troubleshooting table. 1. `config/sentry.md` — org slug, projects, **module filter syntax**, query cookbook 2. `config/elasticsearch.md` — indices, field map, log sources, query recipes 3. `../../products/Tools/CONTEXT.md` — module map, architecture, glossary @@ -97,25 +99,48 @@ Read 1 and 2 before issuing any query. They contain verified, non-obvious syntax (and several *documented dead ends*) — guessing wastes turns and produces confidently wrong "no results found" conclusions. -## Preflight +## Preflight — run before every investigation -Confirm the MCP servers respond before promising results: +Setup and troubleshooting live in `config/mcp-setup.md`. Read it whenever a +server is missing or misbehaving, and point the user there rather than +improvising a fix. -- **Sentry** — `sentry-selfhosted`. Pass `organizationSlug: "brightlocal"` - directly. **Do not** call `find_organizations` to check availability: it - reports no org membership even though the org resolves. That is a known quirk - of the proxy user, not an outage. -- **Elasticsearch** — `elasticsearch`. Verify with a narrow - `list_indices(indexPattern: "logstash-2026.08.*")`, never `*`. +**Registration is not function.** `claude mcp list` showing *Connected* proves +the process started, not that it can reach anything — the VPN is usually what's +actually broken, and it fails silently as empty results. So confirm each server +**answers a real read** before promising results: + +- **Sentry** — `sentry-selfhosted`. A one-issue `search_issues` against + `tools-backend`. Pass `organizationSlug: "brightlocal"` directly. **Do not** + call `find_organizations` to check availability: it reports no org membership + even though the org resolves. That is a known quirk of the proxy user, not an + outage. +- **Elasticsearch** — `elasticsearch`. `count_documents` on yesterday's + `logstash-YYYY.MM.DD`; expect millions. Never `list_indices("*")` — ~74KB. - **Database** — available only if `$TOOLS_PROD_DB_DSN` is set. Never ask the user to paste a DSN into the conversation; tell them to export the env var. -- **Jira** (Modes E–G) — `atlassian`, cloudId +- **Jira** (Modes E–H) — `atlassian`, cloudId `5d89576a-2167-45d7-b6a4-cfa42edbee57`. The server emits an HTTP+SSE deprecation notice on every call; ignore it unless calls start failing. -If a server is down, **say so and continue with the rest**. A Sentry-only -diagnosis is still useful. Never silently degrade — the user must know which -evidence sources backed the conclusion. +### When a server is missing + +Say so, name what it costs, and point at `config/mcp-setup.md`. Never silently +degrade — the user must know which evidence sources backed the conclusion. + +**Elasticsearch is not an optional enrichment.** Without it no claim can be +`CONFIRMED` or `REFUTED`; the whole ledger collapses to `UNTESTABLE (tooling)`. +Do not proceed to a confident-sounding Sentry-only narrative — a stack trace plus +a plausible story is exactly the output this agent exists to prevent. Offer: +report what Sentry alone shows, clearly labelled as untested, or fix the setup +first and get a real diagnosis. **Recommend fixing the setup** — it is usually +the VPN and takes a minute. + +**`UNTESTABLE (tooling)` is not `UNTESTABLE (no source coverage)`.** The first is +a broken toolchain on this machine; the second is a genuine production +observability gap deserving a Mode H ticket. Never let a disconnected MCP server +masquerade as a missing log line — that files real engineering work against a +switched-off VPN. ## Module scoping @@ -337,7 +362,7 @@ Pick the log source from the entry point (full table in | Any PHP warning/notice | `/usr/share/filebeat/php/php-errors.log` | | Worker / Messenger | `/usr/share/filebeat/php/workerman.log` | | Cron / CLI | `/usr/share/filebeat/php/tools/crunz-{output,errors}.log` | -| ActiveSync / aggregators | `/var/log/listing_syncer/prod.log` | +| ActiveSync / aggregators | `/var/log/listing_syncer/prod.log` — **structured**, see below | | 502/504/timeouts | `/usr/share/filebeat/transfer/tools-ssl-error.log` | Rules that keep this honest: @@ -468,6 +493,10 @@ usable." That number is what tells the user whether instrumenting is worth it. change turns every future ±2-minute *correlation* into an exact *join*, and retires the largest standing weakness in this agent's method. Propose it whenever you had to settle for `CONFIRMED (correlated, not joined)`. + - **Check first — it may already exist.** `/var/log/listing_syncer/prod.log` + already carries `bl_msg.extra.request_id` and `bl_msg.context.locationUUID`. + Don't file a ticket asking for something production already emits; see + `config/elasticsearch.md`. 2. **Entity IDs on the exception.** The location / client / aggregator ID attached via Sentry context, so DB verification doesn't require guessing which row. @@ -738,7 +767,10 @@ This agent's only value is that its conclusions are checkable. So: - **Report tool failures.** A timed-out ES query is a hole in the evidence, not something to paper over — and it invalidates any verdict resting on it. - **State the search window** with every count. -- **Correlation is not a join.** Say which one you have. +- **Correlation is not a join.** Say which one you have — and on + `listing_syncer/prod.log` you can usually have a real join, because + `bl_msg.extra.request_id` and `bl_msg.context.locationUUID` are indexed + fields. Don't settle for a time window on a source that offers better. - **Don't inflate.** If Sentry says 27 events from 1 user over 30 days, that is a small problem — say so, even if the stack trace looks alarming. - **"I don't know" is a complete answer** when paired with the Mode H proposal diff --git a/agents/sentry-issue-investigator/README.md b/agents/sentry-issue-investigator/README.md index 067111f..940d02a 100644 --- a/agents/sentry-issue-investigator/README.md +++ b/agents/sentry-issue-investigator/README.md @@ -130,24 +130,65 @@ forgetting, so it always names one ("revisit above 500 events/week"). ## Setup -### 1. MCP servers +### 1. MCP servers — do this first -All three are already connected in this workspace: +**Full instructions: [`config/mcp-setup.md`](config/mcp-setup.md).** Sourced from +the two Confluence pages that own these integrations — +[\[MCP\] Sentry integration](https://brightlocal.atlassian.net/wiki/spaces/PG/pages/4739235841/MCP+Sentry+integration) +and +[\[MCP\] ElasticSearch integration](https://brightlocal.atlassian.net/wiki/spaces/PG/pages/4747657230/MCP+ElasticSearch+integration). +If they disagree with the local file, Confluence wins. -- `sentry-selfhosted` → `https://sentry.bll-i.co.uk`, org `brightlocal` -- `elasticsearch` → production log cluster -- `atlassian` → `brightlocal.atlassian.net` (needed only for Modes E–G) +| Server | Needed for | Without it | +|---|---|---| +| `sentry-selfhosted` | Everything | The agent can't start | +| `elasticsearch` | Testing claims | **No claim can be confirmed or refuted** | +| `atlassian` | Tickets, dedup | No Modes E–H | + +**Prerequisites for both:** BrightLocal VPN with **Engineer-scoped** +permissions, Node.js >= 20, npm. -Check with `/mcp`. Nothing else to configure. +**Sentry** — take _Sentry Claude MCP Token_ from the Engineering 1Password vault, +export it as `SENTRY_ACCESS_TOKEN` in your shell profile (it's read from the +ambient environment, deliberately not stored in the MCP config), then: -> The Atlassian MCP warns that its HTTP+SSE endpoint is unsupported after -> 30 June 2026. `.mcp.json` points at `/v1/sse`; the replacement is `/v1/mcp`. -> Worth updating at some point — unrelated to this agent. +```bash +claude mcp add-json sentry-selfhosted '{ + "type":"stdio", "command":"npx", "args":["-y","@sentry/mcp-server"], + "env":{"SENTRY_HOST":"sentry.bll-i.co.uk","MCP_DISABLE_SKILLS":"seer"} +}' --scope user +``` + +**Elasticsearch** — connect to the VPN first, then: + +```bash +claude mcp add-json elasticsearch '{ + "type":"stdio", "command":"npx", "args":["-y","@octodet/elasticsearch-mcp"], + "env":{"ES_URL":"http://10.79.115.30:9200","ES_VERSION":"8","OTEL_LOG_LEVEL":"none"} +}' --scope user +``` + +On **Claude Desktop** both are one-click instead: connectors → +**Sentry (internal)**, and Settings → Extensions → **BL ElasticSearch**. + +Verify with `claude mcp list` — all three **Connected** — then start a new +session so the tools load. + +> **The VPN is required at runtime, not just at install.** It is by far the most +> common failure: servers stay "Connected" while every query returns nothing, +> which looks exactly like a clean result. Check it first whenever Sentry or +> Elasticsearch goes quiet. The agent is instructed to say so rather than +> quietly producing a thinner answer. > **Known quirk:** `find_organizations()` reports *"You don't appear to be a > member of any organizations"*, but the org slug `brightlocal` works fine. This > is not an outage — pass the slug directly. +> ⚠️ **Atlassian transport is overdue for migration.** `.mcp.json` points at +> `https://mcp.atlassian.com/v1/sse`; HTTP+SSE support ended **30 June 2026**, +> which has now passed. Calls still work and carry a deprecation notice on every +> response. Replacement: `/v1/mcp`. Unrelated to this agent, but worth doing. + ### 2. Tools codebase Reading stack traces against source needs the symlink at @@ -323,6 +364,18 @@ sources: `tools-ssl-access.log` (1.87M lines/day), `php-errors.log` (892k), window. `php-errors.log` is **not** a mirror of Sentry exceptions — absence there proves nothing. +**Structured logs (added 2026-08-31).** `/var/log/listing_syncer/prod.log` — and +*only* that source, verified by aggregation — ships Monolog JSON parsed into +`bl_msg.*`: `bl_msg.datetime` (the **true event time**, so no ship-time skew), +`bl_msg.extra.class`/`.file`/`.line` (the emitting code location, on 100% of +docs), `bl_msg.extra.request_id` (a real join key, ~13% of docs), and +`bl_msg.context.*` (`locationUUID`, aggregator state). For ActiveSync and +aggregator investigations that replaces text-grepping with exact field queries +and turns correlation into a genuine join. Tools' own logs remain raw text. +Caveat: `level_name` looks unreliable there — 256k `ERROR` and 165k `CRITICAL` +per day, and a sampled "…succeeded" line was logged at `ERROR`. Filter on +message and class, not level. + Full detail, including the query forms that *don't* work, is in `config/sentry.md` and `config/elasticsearch.md`. @@ -333,8 +386,9 @@ agents/sentry-issue-investigator/ ├── CLAUDE.md # orchestrator — modes A–G, gates, evidence rules ├── README.md # this file ├── config/ +│ ├── mcp-setup.md # install + verify the MCP servers — read first │ ├── sentry.md # org, projects, module filters, query cookbook -│ ├── elasticsearch.md # indices, field map, log sources, recipes +│ ├── elasticsearch.md # indices, field map, bl_msg.*, log sources, recipes │ ├── jira.md # cloudId, project, issue types, dedup query │ ├── ticket-templates.md # ticket description + fix-plan shapes │ └── db.env.example # DSN template + read-only grant rationale diff --git a/agents/sentry-issue-investigator/config/elasticsearch.md b/agents/sentry-issue-investigator/config/elasticsearch.md index a02000c..c2b77e6 100644 --- a/agents/sentry-issue-investigator/config/elasticsearch.md +++ b/agents/sentry-issue-investigator/config/elasticsearch.md @@ -40,6 +40,69 @@ subfield. fielddata disabled and will error with `Fielddata is disabled on [container_name]…`. +## `bl_msg.*` — structured logs, ListingSyncer only + +Verified live 2026-08-31 on `logstash-2026.08.30`: **2,785,242 docs** carry a +structured `bl_msg` object, and an aggregation on `log.file.path.keyword` returns +**exactly one bucket** — `/var/log/listing_syncer/prod.log`. Monolog JSON, +parsed by logstash into real fields. + +**Everything else — the Tools sources — is raw text**, exactly as the table +above describes. So: `bl_msg` for ListingSyncer/ActiveSync, `message` grepping +for Tools. + +| Field | Notes | +|---|---| +| `bl_msg.datetime` | **The true event time.** Not filebeat ship time — see below | +| `bl_msg.level_name` / `.level` | `INFO` `DEBUG` `ERROR` `CRITICAL` `WARNING` `NOTICE` / Monolog ints | +| `bl_msg.message` | The message alone, without the surrounding JSON | +| `bl_msg.channel` | e.g. `app` | +| `bl_msg.extra.class` / `.function` / `.file` / `.line` | **Emitting code location — present on 100% of docs** | +| `bl_msg.extra.request_id` | UUID, ~12.7% of docs (HTTP entry points, not workers) | +| `bl_msg.context.*` | Domain payload — `locationUUID`, request bodies, aggregator state | + +### Why this matters for claim testing + +Three of the method's standing weaknesses disappear on this source: + +1. **`bl_msg.datetime` removes the skew problem.** The ±2-minute window exists + because `@timestamp` is ship time. Here the true event time is a field — so + range-filter on `bl_msg.datetime` and correlate precisely. Keep the wide + `@timestamp` window as a cheap index prefilter if you like, but decide on + `bl_msg.datetime`. +2. **`bl_msg.extra.class` is a culprit filter.** You can select logs by emitting + class the same way Sentry selects by stack frame — no `message` grepping, no + backslash-phrase trap. **This is the C3-proof way to query this source.** +3. **`bl_msg.extra.request_id` is a real join key**, and + `bl_msg.context.locationUUID` joins to the entity. Where these exist, a + verdict can be `CONFIRMED` outright rather than `CONFIRMED (correlated, not + joined)`. Check for them before proposing a Mode H correlation key — on this + source it already exists. + +```json +{ + "size": 20, + "_source": ["bl_msg.datetime", "bl_msg.level_name", "bl_msg.message", + "bl_msg.extra.class", "bl_msg.extra.line", "bl_msg.context"], + "query": {"bool": {"filter": [ + {"term": {"bl_msg.extra.class.keyword": "App\\Controller\\Location\\SettingsController"}}, + {"range": {"bl_msg.datetime": {"gte": "2026-08-30T23:00:00Z", "lte": "2026-08-31T00:00:00Z"}}} + ]}}, + "sort": [{"@timestamp": "desc"}] +} +``` + +Pivot on a request: `{"term": {"bl_msg.extra.request_id.keyword": "01a0551d-…"}}`. + +> **`level_name` is not trustworthy on this source.** In a 2-doc sample, a line +> reading *"Active-sync settings update succeeded"* was logged at `ERROR`. Daily +> volumes support the suspicion — 255,894 `ERROR` and 164,975 `CRITICAL` per day +> is far too much to be real failure. **Filter on `bl_msg.message` and +> `extra.class`, not on level**, and never infer "this failed" from +> `level_name: ERROR` alone. This is itself a Mode H observability defect worth +> raising — but confirm the pattern on a real sample before filing it; two +> documents is not evidence of a systemic problem. + ### Timestamp skew — read this before correlating `@timestamp` is when filebeat **shipped** the line, while the line's own embedded diff --git a/agents/sentry-issue-investigator/config/mcp-setup.md b/agents/sentry-issue-investigator/config/mcp-setup.md new file mode 100644 index 0000000..3071e4b --- /dev/null +++ b/agents/sentry-issue-investigator/config/mcp-setup.md @@ -0,0 +1,177 @@ +# MCP setup — install and verify before first use + +This agent cannot work without its MCP servers. **Run the checks in this file +before the first investigation**, not halfway through one — a half-configured +setup produces empty results that look exactly like "nothing is wrong". + +Sources of truth (Confluence, PG space): + +- [\[MCP\] Sentry integration](https://brightlocal.atlassian.net/wiki/spaces/PG/pages/4739235841/MCP+Sentry+integration) +- [\[MCP\] ElasticSearch integration](https://brightlocal.atlassian.net/wiki/spaces/PG/pages/4747657230/MCP+ElasticSearch+integration) + +If those pages and this file disagree, **Confluence wins** — it is maintained by +the team that owns the integrations. Update this file to match. + +## What the agent needs + +| Server | Required for | Without it | +|---|---|---| +| `sentry-selfhosted` | Modes A, B — everything | The agent cannot start | +| `elasticsearch` | Mode C — claim testing | **No claim can be CONFIRMED or REFUTED**; every verdict degrades to UNTESTABLE | +| `atlassian` | Modes E–H — tickets, plans, dedup | No dedup, no ticket filing | + +Losing Elasticsearch is not a partial degradation — it removes the agent's entire +ability to test a hypothesis. Say so loudly rather than producing a +Sentry-only narrative that reads like a diagnosis. + +## Prerequisites — both servers + +- **BrightLocal VPN, with Engineer-scoped permissions.** +- Node.js >= 20 and npm (Claude Code installs both servers via `npx`). + +> **The VPN is required at runtime, not just at install.** This is the single +> most common failure: the servers stay registered and `claude mcp list` may +> still look fine, but every query fails or returns nothing. **Check the VPN +> first whenever Sentry or Elasticsearch starts returning empty.** + +## 1. Sentry — `sentry-selfhosted` + +Connects to `https://sentry.bll-i.co.uk/` through a custom Sentry app, authorised +by an organisation-level access token. + +- Token: **1Password → Engineering vault → _Sentry Claude MCP Token_** +- App config: + `https://sentry.bll-i.co.uk/settings/brightlocal/developer-settings/claude-mcp-e68593/` + +**Step 1 — export the token in your shell profile.** + +```bash +# Linux / macOS (zsh) +echo 'export SENTRY_ACCESS_TOKEN="your-token-here"' >> ~/.zshrc && source ~/.zshrc + +# Linux (bash) +echo 'export SENTRY_ACCESS_TOKEN="your-token-here"' >> ~/.bashrc && source ~/.bashrc +``` + +The token is read from the ambient shell environment — it is deliberately **not** +in the MCP config's `env` block, so it never lands in a config file. Keep it that +way. Never paste the token into the chat, a repo file, or a command the agent +runs. + +**Step 2 — register the server.** + +```bash +claude mcp add-json sentry-selfhosted '{ + "type":"stdio", + "command":"npx", + "args":[ + "-y", + "@sentry/mcp-server" + ], + "env":{ + "SENTRY_HOST":"sentry.bll-i.co.uk", + "MCP_DISABLE_SKILLS":"seer" + } +}' --scope user +``` + +**Claude Desktop instead:** search the available connectors for +**Sentry (internal)** and install it — no manual token step. + +## 2. Elasticsearch — `elasticsearch` + +Connect to the VPN **first**, then register: + +```bash +claude mcp add-json elasticsearch '{ + "type":"stdio", + "command":"npx", + "args":[ + "-y", + "@octodet/elasticsearch-mcp" + ], + "env":{ + "ES_URL":"http://10.79.115.30:9200", + "ES_VERSION":"8", + "OTEL_LOG_LEVEL":"none" + } +}' +``` + +Built on [Octodet/elasticsearch-mcp](https://github.com/Octodet/elasticsearch-mcp). + +> **Note the scope difference.** The Confluence page gives the Sentry command with +> `--scope user` and the Elasticsearch command without it, which registers ES at +> *local* scope — available only in the directory where you ran it. If you want +> it everywhere, add `--scope user`. Worth doing for this agent, since it runs +> from the workspace root. + +**Claude Desktop instead:** Settings → Extensions → install **BL ElasticSearch**. + +## 3. Atlassian — `atlassian` + +Already connected in this workspace via `.mcp.json`; needed only for Modes E–H. +cloudId `5d89576a-2167-45d7-b6a4-cfa42edbee57`. + +> ⚠️ **Overdue transport migration.** `.mcp.json` points at +> `https://mcp.atlassian.com/v1/sse`. HTTP+SSE support ended **30 June 2026** — +> that date has passed. Calls still succeed and every response carries a +> deprecation notice, but this is now running on borrowed time. The replacement +> is `https://mcp.atlassian.com/v1/mcp`. Unrelated to this agent, worth fixing. + +## Verifying — run this before the first investigation + +**Step 1 — are they registered and connected?** + +```bash +claude mcp list +``` + +Expect `sentry-selfhosted`, `elasticsearch`, and `atlassian`, each **Connected**. +Registered-but-not-connected almost always means the VPN is down. + +**Step 2 — does each one actually answer?** Registration is not function. Run one +real read per server: + +| Server | Check | Healthy result | +|---|---|---| +| Sentry | `search_issues(organizationSlug="brightlocal", projectSlugOrId="tools-backend", query="is:unresolved", limit=1)` | One issue returned | +| Elasticsearch | `count_documents(index="logstash-")` | A count in the millions | +| Atlassian | `getJiraIssue(cloudId=…, issueIdOrKey="LM-4317")` | The issue returns | + +**Do not use `find_organizations` to check Sentry.** It reports *"You don't +appear to be a member of any organizations"* even when everything works — the MCP +proxy user has no org membership listed. Treating that as an outage is a +documented false negative. Pass `organizationSlug: "brightlocal"` directly. + +**Do not use `list_indices(indexPattern: "*")` to check Elasticsearch.** It +returns ~74KB and gets truncated to a file. Use yesterday's index by name. + +## Troubleshooting + +| Symptom | Cause | Fix | +|---|---|---| +| Everything empty, servers "Connected" | **VPN down or wrong scope** | Reconnect with Engineer permissions — check this first, always | +| Sentry: "not a member of any organizations" | Known proxy-user quirk | Ignore it; pass the org slug directly | +| Sentry: 401 / auth errors | `SENTRY_ACCESS_TOKEN` not exported into this shell | Re-source the profile; restart the Claude session | +| ES server missing in one directory only | Registered at local scope | Re-add with `--scope user` | +| `npx` fails to fetch the package | Node < 20, or no network | Upgrade Node; check VPN/proxy | +| Server absent from `claude mcp list` | Never registered, or registered to a different profile | Re-run the `add-json` command | +| Tools missing in-session after a fix | MCP servers load at session start | Start a new Claude Code session | + +## Reporting a missing server + +If a server is unavailable, the agent must **name it, name what is now +untestable, and point here** — never silently produce a thinner answer: + +> Elasticsearch is not responding, so I can't test any claim against production +> logs. Every causal claim below is `UNTESTABLE (tooling)` — not refuted, just +> unchecked. Setup and troubleshooting: +> `agents/sentry-issue-investigator/config/mcp-setup.md`. The most likely cause +> is the VPN. + +`UNTESTABLE (tooling)` is a distinct verdict from `UNTESTABLE (no source +coverage)`. The first is a broken toolchain and is fixable in minutes; the second +is a real observability gap and needs a Mode H ticket. **Never let a missing MCP +server masquerade as a production instrumentation gap** — that files real +engineering work against a VPN that was simply switched off. From 4c124483295619ab9207271ef5663dd8c5565213 Mon Sep 17 00:00:00 2001 From: bartek Date: Tue, 1 Sep 2026 12:24:05 +0200 Subject: [PATCH 8/8] feat: add scheduling and daily triage digest documentation to CLAUDE.md and README.md --- CLAUDE.md | 5 ++++ agents/sentry-issue-investigator/README.md | 33 +++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index a989d70..5aa00a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,6 +124,11 @@ plus the BrightLocal VPN with Engineer permissions **at runtime** — without th VPN, queries return empty rather than erroring. Setup and troubleshooting: `agents/sentry-issue-investigator/config/mcp-setup.md`. +A weekday-morning Mode A triage digest can run unattended via a systemd user +timer (`config/scheduling.md`). It is **read-only by construction** — the +scheduled run never files, resolves or snoozes anything, because the three +confirmation gates need a human. + Scope is Tools' **Location** and **API** modules. The agent investigates and plans; it never writes application code itself. It is **falsification-first**: every causal claim carries a verdict (`OBSERVED` / `CONFIRMED` / `REFUTED` / diff --git a/agents/sentry-issue-investigator/README.md b/agents/sentry-issue-investigator/README.md index 940d02a..60238db 100644 --- a/agents/sentry-issue-investigator/README.md +++ b/agents/sentry-issue-investigator/README.md @@ -387,15 +387,46 @@ agents/sentry-issue-investigator/ ├── README.md # this file ├── config/ │ ├── mcp-setup.md # install + verify the MCP servers — read first +│ ├── scheduling.md # daily automated triage digest (systemd timer) +│ ├── daily-triage.prompt.md# the unattended Mode A prompt │ ├── sentry.md # org, projects, module filters, query cookbook │ ├── elasticsearch.md # indices, field map, bl_msg.*, log sources, recipes │ ├── jira.md # cloudId, project, issue types, dedup query │ ├── ticket-templates.md # ticket description + fix-plan shapes │ └── db.env.example # DSN template + read-only grant rationale └── scripts/ - └── db-select.py # SELECT-only query runner + ├── db-select.py # SELECT-only query runner + ├── daily-triage.sh # unattended morning digest runner + └── systemd/ # user timer + service units ``` +## Running it daily + +A weekday-morning triage digest, via a systemd **user** timer calling +`claude -p` headless. Setup: [`config/scheduling.md`](config/scheduling.md). + +```bash +systemctl --user enable --now sentry-daily-triage.timer # Mon–Fri 08:12 +``` + +**Triage only — the timer never files, resolves or snoozes anything.** The +agent's three confirmation gates need a human, so the scheduled run is +restricted to the read-only half: it looks, ranks, and reports. You escalate in +a normal session. Safety comes from the tool allowlist in `daily-triage.sh`, not +from the permission mode — every Sentry, Elasticsearch and Jira write tool is +denied explicitly. + +Output lands in `products/Tools/working/sentry/daily/YYYY-MM-DD.md` (gitignored), +with the TL;DR posted to Slack if a webhook is configured. + +**When the environment is unhealthy it produces no digest at all.** A +disconnected VPN and a genuinely quiet night look identical in Sentry — both +empty — so the runner checks the token, the VPN unit, the tun interface and TCP +reachability to the ES cluster before invoking anything. On any failure it writes +`YYYY-MM-DD.SKIPPED.md` and says so in Slack, explicitly labelled *not an +all-clear*. A false quiet morning is worse than a missing one, because you act +on it. + ## Limits - Never writes application code itself — implementation goes through