- Add skipFilters to Reports/Get.php (was the only endpoint still
triggering the full N+1 subquery cascade)
- Scale CTA batch limit dynamically (insightCount * MAX_CTA_COUNT)
instead of fixed APP_LIMIT_SUBQUERY to prevent silent truncation
- Revert deleteReport to callback-based pagination so CTAs are not
orphaned when a report has more than APP_LIMIT_SUBQUERY insights
- Add explicit prefix lengths (700) to _key_project_resource and
_key_project_parent_resource indexes to stay under InnoDB 3072-byte limit
- Validate CTA service/method against ADVISOR_CTA_SERVICES and
ADVISOR_CTA_METHODS enums in the CTAs validator
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The module's namespace and directory now match the top-level service
name (`advisor`) instead of one of its resource names (`insights`):
- src/Appwrite/Platform/Modules/Insights -> .../Modules/Advisor
- src/Appwrite/Insights -> src/Appwrite/Advisor
- tests/unit/Insights -> tests/unit/Advisor
- Route group label flipped from `'insights'` to `'advisor'`
- Section-header comments aligned
Resource names (`insights`, `reports`, `insightCTAs`) and the
`Insight*`/`Report` response models stay — those are the resources the
service exposes, not the service itself.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`key` was a leftover from when CTAs were embedded JSON — there's no
remaining reason to require analyzers to invent a within-insight
identifier. The execution layer is gone (no `cta.key` event format),
insights are immutable from the user side (analyzers re-ingest by
delete + recreate, so idempotent matching never happens), and `label`
already covers human-facing identification. The console can group/sort
CTAs by `service`+`method` if needed.
- Schema: drop `key` attribute and the UNIQUE
`(insightInternalId, key)` index from insightCTAs. Required fields
are now `label`, `service`, `method` (+ optional `params`).
- Validator no longer requires `key`. Drop the dup-key normalization
loop in the manager Create endpoint — there's no semantic
uniqueness to enforce.
- Response model: `InsightCTA` keeps `$id` + standard headers,
`insightId` backref, and the four functional fields.
- E2E: drop sampleCTA's `$key` parameter, drop the
testCreateRejectsDuplicateCTAIds test entirely, rename empty-fields
test to testCreateRejectsCTAWithEmptyLabel and update the missing-
fields tests to drop `key` from their payloads.
- Unit tests rewritten to drop `key`.
- Comment on the `insights.ctas` virtual attribute updated to
reference the renamed `insightCTAs` collection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Embedding CTAs as a 16384-byte JSON blob on `insights` was the wrong
shape — they're real documents with their own lifecycle. Move them out.
Schema:
- New platform `ctas` collection. Each row carries `projectInternalId`,
`projectId`, `insightInternalId`, `insightId` (backref), plus the
CTA fields: `key`, `label`, `service`, `method`, `params`.
- Indexes: `(projectInternalId, insightInternalId)` for the subquery
lookup and a UNIQUE `(insightInternalId, key)` so the per-insight
uniqueness invariant lives at the DB layer (not just in PHP).
- The `ctas` field on `insights` becomes a virtual attribute backed by
a new `subQueryInsightCTAs` filter that joins child docs at read
time. Consumers still get CTAs embedded on the insight response —
one round-trip from their perspective.
- The CTA descriptor's within-insight identifier renamed `id` → `key`
(clashed with the document `$id`). Validator updated.
Endpoints:
- Manager Create now persists CTAs as separate `ctas` documents after
the parent insight, then re-fetches the insight so the response
carries the freshly-joined CTA list.
- User Update trimmed to user-controlled state only (`severity`,
`status`). `title`, `summary`, `payload`, `ctas`, and `analyzedAt`
are analyzer-controlled — analyzers re-ingest by deleting and
POSTing again to the manager endpoint.
- Insight Delete cascades to CTAs.
- Report Delete cascades through Insights → CTAs.
Response model:
- InsightCTA gains the standard document headers (`$id`,
`$createdAt`, `$updatedAt`) and an `insightId` backref. The
caller-supplied identifier is now `key`.
Tests:
- E2E sampleCTA factory uses `key` everywhere; testCreate asserts the
freshly-created CTA carries `$id`, `$createdAt`, `insightId`, and
the right shape.
- Dropped the testUpdate*CTA* tests — user Update no longer accepts
CTAs. testDismissViaUpdate now depends on testUpdate directly.
- Unit tests rewritten to validate `key` instead of `id`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Locks the bug-fix invariants from PR #12195's last review pass and rounds out
worker channel coverage:
C1 testEmailSendFailureDoesNotPersistAlert — SMTP throw must NOT leave a dedup
row behind, retry must deliver and persist exactly once.
C2 testNotificationEventResetClearsAllState — reset() drops every state-bearing
field including preview (regression: missed in original reset body).
C3 testWebhookSendAlertResetsBetweenCalls — Webhooks::sendAlert must reset()
the DI-shared Notification event so two paused-webhook alerts in one worker
pass do not bleed recipients/subject/body into each other.
C4 testConsoleAdapterTreatsDuplicateAsDelivered — Duplicate on createDocument
must surface as a successful idempotent send, not a per-recipient error.
M7 testTrackingPixelRejectsJwtWithoutPurposeClaim — Track endpoint silently
ignores JWTs missing or with the wrong purpose claim (defends against
replaying session/reset JWTs to mark alerts read).
Worker happy-path tests: testEmailChannelHappyPath, testConsoleChannelHappyPath,
testWebhookChannelHappyPath cover the full per-channel dispatch contract end
to end, including HMAC signing for webhooks and the tracking pixel injection
+ post-send persistence for email.
Also extracts CapturingWebhook into its own PSR-4 file so reused across tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- InsightCTA model now exposes `service` (SDK namespace) and `method`
(function name) instead of a single `action` string. Drops the
string-splitting burden on every consumer and lets the console reach
the right SDK method directly.
- Validator requires both `service` and `method` non-empty; same
16-entry max still enforced.
- Endpoint normalization (Create + Update) splits the new shape into
the persisted CTA descriptor.
- Constants split: INSIGHT_CTA_SERVICE_* (databases / tablesDB /
documentsDB / vectorsDB) and INSIGHT_CTA_METHOD_* (createIndex).
- Insight model + InsightCTA model docs updated with the new field
semantics and per-engine examples.
- E2E factory `sampleCTA($id, $engine)` emits the correct service and
engine-appropriate params keys (tableId/columns for tablesDB;
collectionId/attributes everywhere else). Engine matrix asserts
`service` and `method` independently.
- Added e2e + unit coverage for the new failure modes (missing
service, missing method, empty service, empty method).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Restructure InsightsBase trait with explicit helper methods
(createInsight/getInsight/listInsights/updateInsight/deleteInsight,
createReport/getReport/listReports/updateReport/deleteReport, plus
sampleInsight/sampleCTA factories) — same shape ProxyBase uses.
- Add coverage for: report CRUD + duplicate-id rejection, invalid type
rejection, list filtering by all allowed attributes, cursor
pagination + missing-cursor, update preserving untouched fields,
CTA validation edge cases (duplicate ids, empty fields, count > 16),
dismissal round-trip + status filter, report cascade delete,
unauthorized access (no server key), empty-result list.
- Engine-specific insight types (tablesDBIndex, documentsDBIndex,
vectorsDBIndex, plus the legacy databaseIndex) so the CTA's `action`
can map to the matching public API: databases.createIndex,
tablesDB.createIndex, documentsDB.createIndex,
vectorsDB.createIndex. dataProvider drives the engine matrix and
asserts the right action lands in the persisted CTA. Constants for
each action name live in app/init/constants.php.
- InsightCTA model docs spell out which action belongs to which engine
and that the params keys differ between APIs (tableId/columns for
tablesDB vs collectionId/attributes for the legacy / DocumentsDB /
VectorsDB APIs).
- Insight model `type` description now lists every engine variant.
- CTAsTest gains coverage for object-shaped params, empty-action and
empty-label rejection, and the default 16-entry cap.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Account/Alerts/Track endpoint is scope:public (email clients have no
session); SDK Method now declares auth: [] so generators do not require
an auth header for an unauthenticated endpoint.
- NotificationsTest setUp now creates the production `_key_recipient`
UNIQUE composite index on (messageId, channel, userId, teamId), and
testPersistAlertReturnsExistingAlertIdOnDuplicate exercises the
DuplicateException → return-existing-alertId branch end-to-end (both
primary-key collision and unique-index collision via a sibling $id).
- Tracking-pixel e2e now asserts _APP_OPENSSL_KEY_V1 is set instead of
silently falling back to the .env.example placeholder, so a missing
CI secret fails loudly rather than passing against the wrong key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `alerts` collection lives in the platform database, but the Notifications
worker was injecting `dbForProject` and writing alerts there. Webhooks
dispatches with the user's project context, so alerts were being written to
the user's project DB (which has no `alerts` collection), and `/v1/account/alerts`
(which reads from `dbForPlatform`) returned nothing.
Switch the worker to inject `dbForPlatform` instead. All alert reads (dedup
lookup) and writes (persistAlert + ConsoleAdapter) now target the platform DB,
independent of which project the dispatching event belongs to.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover the new worker behaviors introduced in waves 1-3:
- testDedupQueriesByAttributeNotById: prove alreadyDelivered() queries the
messageId attribute, not getDocument($messageId), by seeding a row with
a non-matching $id but matching messageId.
- testConsoleChannelSkipsPersistAlert: confirm the action loop does NOT
call persistAlert for console recipients (the adapter persists).
- testConsoleZeroDeliveryThrows: surface adapter failures via the worker.
- testMultiRecipientFanoutNoCollision: same messageId across recipients
must produce distinct $id values.
- testRecipientStructRoundtripsUserIdAndTeamId: persisted alert carries
recipient userId/teamId.
- testTrackingPixelInjectedIntoEmailHtml: verify the tracking pixel is
spliced before the last </body> tag with the expected URL shape.
- testPersistAlertReturnsAlertIdAndStoresUserId: dispatchEmail's
persistAlert returns a resolvable id and stores userId + read=false.
ConsoleTest: add testMultiRecipientWithSameMessageIdGeneratesDistinctIds
and update existing tests to use the post-ST2 compound `$id` form
(messageId + 8-hex md5 suffix).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the four Greptile P1 fixes to the Notifications worker and
extend it for C3 email read tracking and ST4's stripped SMTP plumbing.
P1 #1: alreadyDelivered() now queries the indexed messageId attribute
instead of getDocument($messageId). The action loop and Console
adapter both write compound `$id`s (messageId + recipient hash), so
the previous direct-id lookup always missed.
P1 #3: action() no longer calls persistAlert after dispatchConsole;
ConsoleAdapter persists internally. Email persists inside
dispatchEmail BEFORE the adapter send so the alertId is available for
the tracking pixel; webhook persists in the action loop after a
successful HTTP send.
P1 #4: dispatchConsole now throws when the adapter reports
`deliveredTo === 0`, surfacing the per-recipient error.
Recipient threading: dispatch() now takes the full recipient map and
returns the alertId (or null when persistence is the caller's
responsibility). persistAlert() reads userId/teamId from the
recipient and grants per-user / per-team-owner CRUD permissions,
falling back to payload permissions only when neither is set. The
returned alertId lets dispatchEmail splice a 1x1 tracking pixel
before the last `</body>` tag, signed with a 30-day HS256 JWT
(_APP_OPENSSL_KEY_V1) carrying {alertId, userId}.
SMTP resolution: ST4 stripped `smtp` and `customMailOptions` from
the Notification event payload, so the worker now resolves SMTP
from the injected project Document (mirroring Mails.php /
Memberships/Create.php), falling back to the env-driven cloud SMTP
adapter when the project has no enabled override.
Tests updated: SpyNotifications.dispatch() matches the new
signature and emulates per-channel persistence so existing routing
assertions keep their semantics. Memory `alerts` collection adds
the `read` boolean attribute to mirror platform.php.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review feedback on PR #12194:
- Pivot CTAs to pure descriptors (id/label/action/params). Drop the
server-side execution layer: Action interface, registry, the
databases.indexes.create CTA action, the params validator, the
/v1/insights/:id/ctas/:id/executions endpoint, the InsightCTAExecution
model, the INSIGHT_CTA_* errors, and the corresponding events. The
console invokes the existing public API directly with the descriptor's
action + params.
- Restore Databases\Indexes\Action.php to its pre-CTA shape and inline
the index-create body back into Create.php (the createIndex helper
was added solely for CTA reuse).
- Move insights collection from project DB to platform DB and add a
parent reports collection alongside it. Insights carry projectId /
projectInternalId for tenant scoping and an optional reportId for
grouping. List endpoints filter by projectInternalId; Get/Update/
Delete also enforce project ownership before touching the document.
- New Reports module with full CRUD (Create/Get/XList/Update/Delete),
Report response model, Reports query validator, REPORT_NOT_FOUND /
REPORT_ALREADY_EXISTS errors, reports.read / reports.write scopes,
and reports.* event tree. Delete cascades to child insights.
- Update.php now mutates the loaded document via setAttribute (instead
of passing a partial new Document), reuses CTAsValidator (instead of
the looser ArrayList<JSON> + isset check), and rejects duplicate CTA
ids.
- Create.php enforces unique CTA ids during normalization.
- CTAsValidator gained a configurable maxCount (default 16) so the
Create path matches the Update path and the DB column size, and
oversized payloads return a clean 400.
- Validator\Queries\Insights adds status and reportId to
ALLOWED_ATTRIBUTES so dismissal / report workflows are filterable.
- Realtime channel parser guards $parts[1] for both insights and
reports event names.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves Validator/CTAParams/DatabasesCreateIndex to
Validator/CTA/Databases/Index/Create so the validator hierarchy mirrors
the action hierarchy (Insights/CTA/Action/Databases/Indexes/Create).
Also tightens the Update and CTA execute endpoint descriptions to call
out the dismissal-via-status flow and what the execution result carries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Jake Barnby
Chirag Aggarwal
Damodar Lohani
ArnabChatterjee20k
Matej Bačo