Enable signal-scoped entity enrichment producers - #854
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 4 Skipped Deployments
|
|
Warning Review limit reached
More reviews will be available in 3 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthrough Changes Signal Enrichment Pipeline
Sequence Diagram(s) sequenceDiagram
participant Signal as SignalProducer
participant Classifier as classifySignalInputLocally
participant Indexer as classify-signal workflow
participant Enricher as enrichSignalEntities workflow
participant Provider as ProviderFetchers (X/GitHub)
participant Resolver as runEntityResolution
Signal->>Classifier: classify input (input, signalId)
Classifier-->>Indexer: classification result (routing, visibility)
Indexer->>Indexer: persist classification & resolved links
Indexer->>Enricher: enqueue app/signal.entity-enrichment.requested
Enricher->>Provider: fetchSignalEntityProfiles(targets)
Provider-->>Enricher: payloads (xPayloads, githubPayloads, diagnostics)
Enricher->>Enricher: convert to observations, compute IDs
Enricher->>Resolver: emit app/connector.profile.observed (observations + source)
Resolver->>Resolver: project graph people & reconcile links (if source=signal)
Resolver-->>Indexer: emit app/entity.graph.persisted (projectedPeople, entityLinksResolved)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Review issues (categorize: [blocker]/[warning]/[nit]) [warning] app event schema and persisted counters changed — verify backwards compatibility in consumers (file: api/app/src/inngest/schemas/app.ts). Why: adding optional [warning] Env gating for local classification/deterministic-only linking uses VERCEL_ENV + absence of process.env.AI_GATEWAY_API_KEY (files: api/app/src/inngest/workflow/classify-signal.ts, api/app/src/inngest/workflow/index-signal-entities.ts). Why: accidental use in CI or atypical dev setups could trigger local paths; confirm environment semantics and CI test coverage. [nit] Exporting getFreshXConnectorAccessToken (file: api/app/src/services/connectors/x-mcp-bridge.ts) is small API surface increase — ensure intended public usage and add brief docstring where consumed. Why: minor maintenance clarity. [nit] Ensure new enum expansion ( End. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bfee3bad1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inArray(orgEntityPeople.primarySourceIdentityId, sourceIdentityIds), | ||
| ...input.sourceIdentities.map( | ||
| (identity) => | ||
| sql`${orgEntityPeople.canonicalKey} like ${`%${escapeLikePattern(identity.identityKey)}%`} escape '\\\\'` |
There was a problem hiding this comment.
When an enriched handle is a prefix of another handle, this substring LIKE can load the wrong canonical graph person; for example x:handle:ava matches a canonical key like person:x:handle:ava_ai, and graphPersonContainsSourceIdentity also uses .includes, so projection can create/resolve a People row for @ava using the @ava_ai graph metadata. This can incorrectly resolve signal links and is avoidable by matching delimited canonical-key tokens or joining through exact source identity ids instead of substring matching.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
api/app/src/inngest/workflow/index-signal-entities.ts (1)
33-35: ⚡ Quick win[warning] Centralize the dev keyless fallback gate used by both workflows
shouldUseDeterministicOnlyEntityLinking()duplicates the same condition used inapi/app/src/inngest/workflow/classify-signal.ts(Line 123-125). If one branch changes later, classification and indexing can diverge in non-obvious ways. Extract a shared helper and reuse it in both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/app/src/inngest/workflow/index-signal-entities.ts` around lines 33 - 35, The dev keyless fallback condition is duplicated in shouldUseDeterministicOnlyEntityLinking; extract that condition into a single exported helper (e.g., isDevKeylessFallback or useDeterministicOnlyEntityLinkingGate) in a shared utilities module and replace the local implementation in index-signal-entities.ts and the duplicate in classify-signal.ts to import and call the new helper; ensure the helper returns the same boolean expression (env.VERCEL_ENV === "development" && !process.env.AI_GATEWAY_API_KEY) and update both files to remove their local functions and use the shared export.db/app/src/__tests__/signal-entity-links.test.ts (1)
460-548: ⚡ Quick win[warning] Cap-path behavior is not actually exercised
This test name says “capped provider targets,” but the fixture never exceeds
MAX_ENRICHMENT_TARGETS_PER_PROVIDER, soover_capis untested. Add >10 uniquextargets and assert only 10 remain plusskipped.reason === "over_cap"for the rest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/app/src/__tests__/signal-entity-links.test.ts` around lines 460 - 548, The test "derives capped provider targets from unresolved persisted links" never exceeds MAX_ENRICHMENT_TARGETS_PER_PROVIDER so the over_cap path is untested; update the fixture passed to makeTargetDb (used by listSignalEntityEnrichmentTargets) to include more than MAX_ENRICHMENT_TARGETS_PER_PROVIDER unique x/profile targets (e.g., create >10 distinct links with provider "x"/normalizedValue "ava_ai_<n>") so the cap logic runs, then assert the returned x array contains only MAX_ENRICHMENT_TARGETS_PER_PROVIDER items and that the extra links appear in skipped with reason "over_cap"; keep other assertions (github target and unsupported skips) and the spies.where / spies.orderBy call-count checks.packages/github-app-node/src/__tests__/user.test.ts (1)
53-102: ⚡ Quick win[nit] Add one failure-path behavior test for profile validation
Line 53-102 only validates the success path. Add one reject case (e.g.,
type: "Organization"or malformed profile payload) so theGITHUB_API_RESPONSE_INVALIDguard stays locked by behavior, not just implementation.Suggested test addition
describe("getGitHubUserByLogin", () => { + it("rejects non-user profiles", async () => { + const fetchMock = vi.fn(async () => + Response.json({ id: 1, login: "acme", type: "Organization" }) + ); + + await expect( + getGitHubUserByLogin({ + fetch: fetchMock, + login: "acme", + token: "installation_token", + }) + ).rejects.toMatchObject({ code: "GITHUB_API_RESPONSE_INVALID" }); + }); + it("returns a rich user profile from GET /users/{login}", async () => {As per coding guidelines, "Prefer behavior-based assertions over implementation details".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/github-app-node/src/__tests__/user.test.ts` around lines 53 - 102, Add a failure-path test for getGitHubUserByLogin that exercises the GITHUB_API_RESPONSE_INVALID guard: create a fetch mock that returns a GitHub profile payload which is invalid (e.g., type: "Organization" or missing required fields like login/id) and assert that getGitHubUserByLogin rejects (or throws) with the expected error (or error code/message) instead of resolving; reference getGitHubUserByLogin and the GITHUB_API_RESPONSE_INVALID guard when locating where to add the test and ensure the test also verifies the fetch call was invoked with the same request headers as the success test.api/app/src/__tests__/connectors-x-mcp-bridge.test.ts (1)
227-251: ⚡ Quick win[nit] Assert refresh is skipped for fresh access tokens
Line 227-251 validates decrypt behavior, but it does not explicitly lock the “no refresh” branch. Add a
refreshXOAuthTokenMocknegative assertion to catch unnecessary refresh regressions.Suggested assertion
it("exposes the shared fresh access-token helper", async () => { await expect( getFreshXConnectorAccessToken({ config: { @@ expect(decryptMock).toHaveBeenCalledWith( "encrypted_x_access", envMock.ENCRYPTION_KEY ); + expect(refreshXOAuthTokenMock).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/app/src/__tests__/connectors-x-mcp-bridge.test.ts` around lines 227 - 251, The test for getFreshXConnectorAccessToken should also assert that the refresh path was not taken: after the existing expect(...).resolves.toBe("x_access_token") and the decryptMock assertion, add a negative assertion verifying refreshXOAuthTokenMock was not called (e.g. expect(refreshXOAuthTokenMock).not.toHaveBeenCalled()) so regressions that trigger unnecessary token refreshes are caught; place this in the "exposes the shared fresh access-token helper" test alongside decryptMock assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/app/src/inngest/workflow/enrich-signal-entities.ts`:
- Around line 44-52: The timestamp and observation conversion are done outside a
step, breaking determinism on retries; move creation of observedAt and the
mapping/filtering (the block using observedAt, xUserPayloadToObservation,
githubUserPayloadToObservation, and isNonNullish) into a single step.run call
(e.g., step.run("convert observations", async () => { ... })) immediately after
fetching profiles so the timestamp is stable and the conversion benefits from
step-level retries.
In `@db/app/src/utils/people.ts`:
- Around line 381-399: The canonical-key matching currently uses substring LIKE
fragments (see orgEntityPeople.canonicalKey with escapeLikePattern and
input.sourceIdentities) which causes false positives; replace these raw SQL LIKE
fragments with delimiter-aware exact token membership checks using the Drizzle
query builder (e.g., split canonicalKey by the delimiter and use equality or
array membership operators provided by Drizzle instead of sql`` fragments), and
also ensure the primarySourceIdentityId match
(orgEntityPeople.primarySourceIdentityId) is preserved; remove all raw sql
string construction (escapeLikePattern/sql`` usage) and build the equivalent
safe query using Drizzle functions/operators so tokens are matched exactly
rather than via '%...%'.
In `@packages/github-app-node/src/index.ts`:
- Around line 36-41: The package entrypoint is missing the exported input type
for getGitHubUserByLogin: add GetGitHubUserByLoginInput to the re-exports in
packages/github-app-node/src/index.ts (alongside getGitHubUserByLogin and the
other types) so the public API surfaces the new input type; alternatively if the
input should be internal, remove its export from
packages/github-app-node/src/user.ts, but ensure consistency by either
re-exporting GetGitHubUserByLoginInput or making it non-exported in user.ts.
---
Nitpick comments:
In `@api/app/src/__tests__/connectors-x-mcp-bridge.test.ts`:
- Around line 227-251: The test for getFreshXConnectorAccessToken should also
assert that the refresh path was not taken: after the existing
expect(...).resolves.toBe("x_access_token") and the decryptMock assertion, add a
negative assertion verifying refreshXOAuthTokenMock was not called (e.g.
expect(refreshXOAuthTokenMock).not.toHaveBeenCalled()) so regressions that
trigger unnecessary token refreshes are caught; place this in the "exposes the
shared fresh access-token helper" test alongside decryptMock assertions.
In `@api/app/src/inngest/workflow/index-signal-entities.ts`:
- Around line 33-35: The dev keyless fallback condition is duplicated in
shouldUseDeterministicOnlyEntityLinking; extract that condition into a single
exported helper (e.g., isDevKeylessFallback or
useDeterministicOnlyEntityLinkingGate) in a shared utilities module and replace
the local implementation in index-signal-entities.ts and the duplicate in
classify-signal.ts to import and call the new helper; ensure the helper returns
the same boolean expression (env.VERCEL_ENV === "development" &&
!process.env.AI_GATEWAY_API_KEY) and update both files to remove their local
functions and use the shared export.
In `@db/app/src/__tests__/signal-entity-links.test.ts`:
- Around line 460-548: The test "derives capped provider targets from unresolved
persisted links" never exceeds MAX_ENRICHMENT_TARGETS_PER_PROVIDER so the
over_cap path is untested; update the fixture passed to makeTargetDb (used by
listSignalEntityEnrichmentTargets) to include more than
MAX_ENRICHMENT_TARGETS_PER_PROVIDER unique x/profile targets (e.g., create >10
distinct links with provider "x"/normalizedValue "ava_ai_<n>") so the cap logic
runs, then assert the returned x array contains only
MAX_ENRICHMENT_TARGETS_PER_PROVIDER items and that the extra links appear in
skipped with reason "over_cap"; keep other assertions (github target and
unsupported skips) and the spies.where / spies.orderBy call-count checks.
In `@packages/github-app-node/src/__tests__/user.test.ts`:
- Around line 53-102: Add a failure-path test for getGitHubUserByLogin that
exercises the GITHUB_API_RESPONSE_INVALID guard: create a fetch mock that
returns a GitHub profile payload which is invalid (e.g., type: "Organization" or
missing required fields like login/id) and assert that getGitHubUserByLogin
rejects (or throws) with the expected error (or error code/message) instead of
resolving; reference getGitHubUserByLogin and the GITHUB_API_RESPONSE_INVALID
guard when locating where to add the test and ensure the test also verifies the
fetch call was invoked with the same request headers as the success test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 297e40a0-1d83-4c07-b5ed-6543e08f0493
⛔ Files ignored due to path filters (3)
docs/superpowers/plans/2026-06-07-signal-scoped-entity-enrichment.mdis excluded by!docs/**docs/superpowers/runbooks/2026-06-07-signal-enrichment-agent-browser.mdis excluded by!docs/**docs/superpowers/specs/2026-06-07-real-entity-enrichment-producers-design.mdis excluded by!docs/**
📒 Files selected for processing (42)
ai/src/__tests__/signal-classifier/classify.test.tsai/src/signal-classifier/index.tsai/src/signal-classifier/local.tsapi/app/src/__tests__/connectors-x-mcp-bridge.test.tsapi/app/src/__tests__/entity-enrichment-adapters.test.tsapi/app/src/__tests__/entity-index-workflow.test.tsapi/app/src/__tests__/entity-resolution-workflow.test.tsapi/app/src/__tests__/inngest-route.test.tsapi/app/src/__tests__/signal-entity-enrichment-fetchers.test.tsapi/app/src/__tests__/signal-entity-enrichment-workflow.test.tsapi/app/src/__tests__/signal-workflow.test.tsapi/app/src/__tests__/workspace-entity-graph-router.test.tsapi/app/src/inngest/index.tsapi/app/src/inngest/schemas/app.tsapi/app/src/inngest/workflow/classify-signal.tsapi/app/src/inngest/workflow/enrich-signal-entities.tsapi/app/src/inngest/workflow/index-signal-entities.tsapi/app/src/inngest/workflow/run-entity-resolution.tsapi/app/src/router/(pending-not-allowed)/workspace-entity-graph.tsapi/app/src/services/connectors/x-mcp-bridge.tsapi/app/src/services/entity-enrichment/adapters.tsapi/app/src/services/entity-enrichment/ids.tsapi/app/src/services/entity-enrichment/index.tsapi/app/src/services/entity-enrichment/provider-fetchers.tsdb/app/src/__tests__/people.test.tsdb/app/src/__tests__/signal-entity-links.test.tsdb/app/src/index.tsdb/app/src/utils/people.tsdb/app/src/utils/signal-entity-links.tsemulators/github/src/__tests__/server.test.tsemulators/github/src/fixtures.tsemulators/x/src/__tests__/server.test.tsemulators/x/src/fixtures.tsemulators/x/src/plugin/index.tsemulators/x/src/plugin/users.tspackages/app-validation/src/__tests__/people.test.tspackages/app-validation/src/schemas/people.tspackages/github-app-node/src/__tests__/user.test.tspackages/github-app-node/src/index.tspackages/github-app-node/src/user.tspackages/x-app-node/src/__tests__/tools.test.tspackages/x-app-node/src/tools.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/app/src/utils/people.ts`:
- Around line 406-447: The fallback selection uses sourceIdentities[0] from an
unordered set, causing non-deterministic flips; make selection deterministic by
ordering the source identities before returning/using them: in
loadCanonicalSourceIdentitiesForGraphPeople ensure the returned array is sorted
by a stable key (e.g., identityKey or a concatenation of provider/type/value or
id) so callers (and the primarySourceIdentityId fallback logic) always pick the
same provider; apply the same deterministic sort where sourceIdentities is
consumed (the fallback that reads primarySourceIdentityId) so fallback selection
is stable across runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 041dde81-d23b-44b5-86f2-cbbd6969bbcf
📒 Files selected for processing (6)
api/app/src/inngest/workflow/enrich-signal-entities.tsdb/app/src/__tests__/people.test.tsdb/app/src/__tests__/signal-entity-links.test.tsdb/app/src/utils/people.tsdb/app/src/utils/signal-entity-links.tspackages/github-app-node/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/github-app-node/src/index.ts
- api/app/src/inngest/workflow/enrich-signal-entities.ts
- db/app/src/tests/signal-entity-links.test.ts
- db/app/src/utils/signal-entity-links.ts
Fixes Applied SuccessfullyFixed 2 file(s) based on 1 CodeRabbit feedback item(s). Files modified:
Commit: The latest autofix changes are on the |
…chment-producers # Conflicts: # api/app/src/__tests__/connectors-x-mcp-bridge.test.ts # packages/x-app-node/src/__tests__/tools.test.ts # packages/x-app-node/src/tools.ts
1 participant
Summary
Scope notes
@ava_ai+https://github.com/avachen.Test Plan
pnpm --filter @repo/ai exec vitest run src/__tests__/signal-classifier/classify.test.tspnpm --filter @api/app exec vitest run --passWithNoTests entity-index-workflow.test.ts signal-workflow.test.ts signal-entity-enrichment-fetchers.test.ts entity-resolution-workflow.test.tspnpm --filter @db/app exec vitest run src/__tests__/people.test.ts src/__tests__/signal-entity-links.test.tspnpm --filter @repo/x-emulator test -- src/__tests__/server.test.tspnpm --filter @repo/github-emulator test -- src/__tests__/server.test.tspnpm checkpnpm typecheckLocal verification
@ava_aiandhttps://github.com/avachen.Summary by CodeRabbit