feat: integration builder POC [CM-1372] - #4463
Conversation
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
PR SummaryMedium Risk Overview Adds GitHub is wired as the first connector scaffold (app JWT/installation tokens, repo discovery, GraphQL helpers, activity schemas/mappers) but Reviewed by Cursor Bugbot for commit 266690b. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
|
mbani01
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a proof-of-concept persistence and scheduling layer for integration sync units.
Changes:
- Defines sync-unit types and statuses.
- Adds claiming, rescheduling, and run-recording queries.
- Creates the sync-unit table and due-work index.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
services/libs/data-access-layer/src/integrationBuilder/types.ts |
Defines sync-unit data contracts. |
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts |
Implements sync-unit database operations. |
backend/src/database/migrations/V1786442761__createSyncUnitsTable.sql |
Adds sync-unit storage and indexing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:48
- Soft-deleting an integration does not deactivate these rows:
IntegrationRepository.destroyuses the paranoid integration model, while this claim only checks the sync-unit status. As a result, disconnected integrations remain claimable and continue syncing indefinitely. Filter candidates to integrations whosedeletedAtis null (and separately decommission their units if retention requires it).
WHERE status = 'active'
AND "nextRunAt" <= now()
AND ("lockedAt" IS NULL OR "lockedAt" < now() - $(leaseMinutes) * interval '1 minute')
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:42
- This lease can expire and be claimed by a second worker, but the returned
lockedAtis not used as an ownership token byrescheduleUnit,recordRunSuccess, orrecordRunFailure; all three update byidalone. A slow first worker can therefore overwrite the newer run's watermark/counters and even clear its lock. Pass the claimed lease value (or a generated claim token) to every completion update, include it in theWHEREclause, and reject a zero-row update.
SET "lockedAt" = now(), "updatedAt" = now()
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:9
- These new data-access functions are not exported from the package entry point:
@crowd/data-access-layerresolves tosrc/index.ts, which has nointegrationBuilderexport, and this directory has no index module. Consumers therefore cannot use the normal package API and must rely on an internal/src/...deep import. Add anintegrationBuilder/index.tsbarrel and export it from the root index.
export async function upsertSyncUnits(qx: QueryExecutor, units: SyncUnitUpsert[]): Promise<number> {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:26
- This only inserts/renames discovered units. A channel or sync omitted by a later discovery remains
activeand continues being scheduled, while a previouslydecommissionedunit that is rediscovered remains decommissioned. Reconcile the complete discovered set transactionally: decommission missing units and reactivate rediscovered ones (while preserving intentionally paused/dead-letter units).
ON CONFLICT ("integrationId", "channelId", "syncName")
DO UPDATE SET "channelName" = EXCLUDED."channelName", "updatedAt" = now()`,
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:53
- Filtering soft-deleted integrations only at claim time leaves their units
activewith permanently overduenextRunAtvalues. Those rows stay at the front ofix_sync_units_due, so every scheduler poll must scan past an ever-growing set of unclaimable units. Decommission sync units as part of integration deletion (or add equivalent cleanup) so they leave the partial due index.
AND EXISTS (
SELECT 1
FROM public.integrations i
WHERE i.id = su2."integrationId" AND i."deletedAt" IS NULL
)
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
services/libs/integration-builder/src/credentials.ts:31
- These lines document a future implementation change rather than a required invariant. Remove them; the credential-loading function and environment variable names are self-explanatory.
// POC only: secrets come from env; secret-manager adoption (OCI Vault) swaps this
// body without touching callers — getCredential stays the single entry point
services/libs/integration-builder/package.json:19
zodis not used anywhere in the new package, but adding it also introduces a separate Zod 3 installation in the lockfile. Remove the dependency until validation is implemented.
"zod": "^3.22.0"
services/libs/integration-builder/src/types.ts:14
- These POC/future-design notes describe the current change rather than a non-obvious invariant. Remove them; the fixed
kindtype already makes the temporary single-variant constraint clear.
// POC only: single variant; becomes a discriminated union (token, oauth2, ...)
// as more connectors land
services/libs/integration-builder/src/credentials.ts:20
- This change note only restates the temporary switch design and does not document an invariant. Remove it rather than retaining POC commentary in the implementation.
This issue also appears on line 30 of the same file.
// POC scope: GitHub only; each migrated connector adds its platform case here
services/libs/integration-builder/package.json:16
@crowd/commonis not imported anywhere in this new package. Remove the unused dependency so the package declares only its actual runtime requirements.
This issue also appears on line 19 of the same file.
"@crowd/common": "workspace:*",
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
services/libs/integration-builder/src/credentials.ts:12
- This lookup discards the integration-specific identity and returns the same global app credential for every GitHub integration. GitHub integrations are scoped by an installation ID (
integrationIdentifier), whilegithub-nangointegrations use mapped connection IDs; becauseManifest.discoverreceives only this credential, it cannot restrict discovery to the requested integration and can associate another installation's channels with it. Include the relevant installation/connection identity in the credential (or pass the integration identity into discovery) and handle the two platform credential models separately.
const integration: { platform: string } | null = await qx.selectOneOrNone(
`SELECT platform
FROM integrations
WHERE id = $(integrationId) AND "deletedAt" IS NULL`,
services/libs/integration-builder/src/credentials.ts:31
- This comment describes the implementation and an unticketed future replacement rather than an allowed invariant or external quirk. Remove it; the helper name and environment-variable reads are self-explanatory.
// POC only: secrets come from env; secret-manager adoption (OCI Vault) swaps this
// body without touching callers — getCredential stays the single entry point
services/libs/integration-builder/src/types.ts:15
- These POC/future-design notes do not document an external quirk, invariant, constraint, legacy complexity, or ticketed TODO. Remove them and let the credential type express the currently supported variant.
// POC only: single variant; becomes a discriminated union (token, oauth2, ...)
// as more connectors land
services/libs/integration-builder/src/credentials.ts:21
- This scope note describes the current implementation and future work without a ticket, which is not an allowed code-comment case. Remove it; the switch already makes the supported platforms clear.
This issue also appears on line 30 of the same file.
// POC scope: GitHub only; each migrated connector adds its platform case here
services/libs/data-access-layer/src/integrationBuilder/syncUnits.ts:40
- The new atomic-claim behavior has no database integration test. Add coverage that runs concurrent claims and verifies an ID is returned once, while active/due, deleted-integration, and expired-lease filtering behave as intended; comparable data-access SQL is exercised in
services/libs/data-access-layer/src/packages/*.integration.test.ts.
export async function claimDueUnits(qx: QueryExecutor, limit: number): Promise<ISyncUnit[]> {
return qx.select(
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
services/libs/connectors/src/pool/tokenPool.ts:53
- This new token state machine has no automated coverage for parking, quarantine, LRU selection, stale budget probing, or pool exhaustion. These branches control credential reuse and dispatch admission; add focused Vitest tests with a fake Redis client, consistent with the repository's existing HTTP/retry tests such as
services/apps/packages_worker/src/go/__tests__/proxyClient.test.ts.
export function createTokenPool(
redis: RedisClient,
platform: string,
connectionId: string,
options?: TokenPoolOptions,
): TokenPool {
services/libs/connectors/src/http/client.ts:127
- Every rejection from
axios.requestis classified asprovider.unavailable, including deterministic request/setup failures such as an invalid URL or bad Axios option. Those connector defects are then retried three times and persisted as provider outages, while the newconnector.codeclass is never used here. Distinguish transport failures (timeout/DNS/socket) from request-construction errors and wrap the latter asConnectorCodeError.
return await axios.request<T>(authenticatedConfig)
} catch (err) {
throw new ProviderUnavailableError('no response from provider', { cause: err })
pnpm-lock.yaml:20569
- Adding the new workspace unexpectedly downgrades the existing
tsx@4.23.12dependency fromesbuild@0.28.2to0.28.0across the lockfile. This is unrelated to the connector POC and changes tooling for every workspace usingtsx, contrary to the repository rule to avoid out-of-scope changes (CLAUDE.md:70). Regenerate the lockfile while preserving the existing esbuild resolution, or include the dependency change separately with its rationale.
services/libs/data-access-layer/src/connectors/syncUnits.ts:138 - The shared DAL rules require selecting known columns rather than touching columns blindly (
CLAUDE.md:56-57).SELECT *also returnscreatedAt/updatedAt, which are not part ofISyncUnit, and silently broadens this activity payload whenever the table changes. Project only the fields declared byISyncUnit.
`SELECT *
…e interpretation Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/libs/connectors/src/emit.ts:48
triggerResultProcessingexpects the activity'ssourceIdas its second argument (the existing activity path passesactivity.sourceIdinintegrationDataService.ts:164-167), but this uses the newly generated result ID. That gives every duplicate a different Kafka key, so re-emitted activities are no longer ordered with the same source and can race through sink deduplication. Pass the schema-validated activity source ID instead; this also requires the emitted activity contract to exposesourceId.
await deps.sinkEmitter.triggerResultProcessing(resultId, resultId, false)
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
services/apps/connectors_worker/src/workflows/dispatcher.ts:22
workflow.startreturns after creating the child, so the child can hit the rate-limit handler and persist itsresumeAtbefore this next activity runs. This cadence reschedule then overwrites that provider-required resume time, allowing the unit to run again before the limit resets. Establish the cadence schedule before starting the child, or make this update conditional so it cannot replace scheduling written by the run.
await activity.startRun(unit)
await activity.reschedule(unit.id, unit.platform, unit.syncName)
| '@esbuild/aix-ppc64@0.28.0': | ||
| resolution: {integrity: sha512-lhRUCeuOyJQURhTxl4WkpFTjIsbDayJHih5kZC1giwE+MhIzAb7mEsQMqMf18rHLsrb5qI1tafG20mLxEWcWlA==} |
There was a problem hiding this comment.
Verified against the actual commit diff: no resolved versions changed. The lockfile delta is only (1) the two new workspace deps (@crowd/integrations link + jsonwebtoken/@types/jsonwebtoken), (2) refreshed deprecation message text for the already-present old glob versions, and (3) AWS SDK peer-dependency annotation reshuffling ((@aws-sdk/client-sts@…) suffixes moving between snapshot keys) with identical versions. There is no esbuild downgrade and no follow-redirects/debug version change in the diff.
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
services/apps/connectors_worker/src/workflows/dispatcher.ts:22
workflow.startresolves once Temporal accepts the run, not whenexecuteSyncfinishes. The child can therefore persist a rate-limitresumeAtbefore this next activity runs, after whichrescheduleoverwrites that provider reset with the ordinary cadence. Make cadence scheduling and rate-limit scheduling ordered (for example, persist cadence before launch with failure recovery, or let the sync workflow own the next schedule).
await activity.startRun(unit)
await activity.reschedule(unit.id, unit.platform, unit.syncName)
services/libs/connectors/src/connectors/github/appToken.ts:47
executeSynccallsseedTokensonce per sync unit, and this always mints a new installation token even though its expiry is returned. Many repository units for one integration will concurrently call the same GitHub endpoint and overwrite the same pool entry. Cache the token withexpiresAtand single-flight refresh only near expiry.
export async function seedGithubTokens(credential: Credential, pool: TokenPool): Promise<void> {
const installationId = requireInstallationId()
const { token } = await mintInstallationToken(credential, installationId)
await pool.seed(`install-${installationId}`, token)
…k error class Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
services/libs/connectors/src/connectors/github/mappers/member.ts:61
- These flat attribute values do not match the data-sink contract.
ActivityService.memberAttValuereadsmember.attributes[attributeName][platform], so values such asisHireable: falseandurl: "..."are ignored and GitHub profile attributes are silently lost. Return{ github: value }for each attribute and update the Zod schema to validate that shape.
return {
isHireable: user.isHireable ?? false,
url: `https://github.com/${user.login ?? ''}`,
bio: user.bio ?? '',
location: user.location ?? '',
services/apps/connectors_worker/src/workflows/dispatcher.ts:22
startRunreturns once Temporal accepts the workflow, soexecuteSynccan already persist a rate-limitresumeAtbefore this next activity runs. The unconditional cadence reschedule can then overwrite that later reset time and dispatch the unit while the provider is still limiting it. Move cadence scheduling into the successful sync completion path, or use a conditional state update that cannot overwrite run-owned scheduling decisions.
await activity.startRun(unit)
await activity.reschedule(unit.id, unit.platform, unit.syncName)
services/apps/connectors_worker/src/activities/syncRunActivities.ts:52
- The interval reports liveness but the activity never observes
activityContext.cancelledor passescancellationSignalto connector requests. Temporal heartbeats enable delivery of cancellation; they do not stop application work by themselves, so a cancelled execution can continue emitting results untilsync.runreturns. Propagate the cancellation signal through HTTP/sync execution and rethrow cancellation after cleanup.
const heartbeat = setInterval(() => {
try {
activityContext.heartbeat()
} catch (err) {
log.warn({ errMsg: (err as Error).message }, 'heartbeat failed')
| const installationId = process.env.CROWD_GITHUB_INSTALLATION_ID | ||
| if (!installationId) { | ||
| throw new Error('missing CROWD_GITHUB_INSTALLATION_ID environment variable') |
| interface GraphqlErrorEnvelope { | ||
| errors?: { type?: string }[] | ||
| } | ||
|
|
||
| export const interpretGithubResponse: ResponseInterpreter = (response) => { | ||
| if (response.status !== 200) { | ||
| return null | ||
| } | ||
| const body = response.data as GraphqlErrorEnvelope | null | ||
| if (body?.errors?.some((e) => e.type === 'RATE_LIMITED')) { | ||
| return new RateLimitError('github graphql rate limited') | ||
| } | ||
| return null | ||
| } |
| for (const unit of units) { | ||
| const manifest = findManifest(unit.platform) | ||
| const pool = createTokenPool(svc.redis, unit.platform, unit.integrationId, { | ||
| probeBudget: manifest?.probeBudget, | ||
| }) |
| const MAX_INITIAL_DELAY_SECONDS = 900 | ||
| const CLAIM_LEASE_MINUTES = 5 | ||
|
|
||
| export async function upsertSyncUnits(qx: QueryExecutor, units: SyncUnitUpsert[]): Promise<number> { |
|
|
||
| export const githubConnector: Manifest = { | ||
| platform: 'github', | ||
| syncs: [], |
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (10)
Previously missed (2) — in code that hasn't changed since the last review.
services/libs/connectors/src/connectors/github/mappers/member.ts:136
- Regular users always lose their GitHub profile name here even though it is available; the existing GitHub processor uses the trimmed name with login as fallback (
processData.ts:123). Preserve that behavior to avoid degrading member display names.
displayName: user.login,
services/libs/connectors/src/credentials.ts:11
- This duplicates an integration lookup outside the DAL even though
fetchIntegrationByIdalready returnsplatformand applies the same soft-delete filter. Reuse that function so integration lookup behavior has one implementation, as required by the repository's DAL convention (CLAUDE.md:32-34).
const integration: { platform: string } | null = await qx.selectOneOrNone(
`SELECT platform
FROM integrations
WHERE id = $(integrationId) AND "deletedAt" IS NULL`,
{ integrationId },
)
services/apps/connectors_worker/src/workflows/dispatcher.ts:22
- Rescheduling immediately after starting the detached workflow races with the rate-limit reschedule in
executeSync(syncRunActivities.ts:115-126). A fast run can persist its provider resume time first and then have it overwritten by the normal cadence, causing requests before the provider reset. Give one code path ownership ofnextRunAt(for example, schedule on sync completion and have dispatch only release the claim).
await activity.reschedule(unit.id, unit.platform, unit.syncName)
services/libs/connectors/src/http/client.ts:89
- This boolean allows only one replacement token. If the first two tokens are rate-limited (or fail authentication in the equivalent branch below), a healthy third token is never attempted even though the pool supports multiple entries. Continue rotation until acquisition reports exhaustion, while tracking attempted token IDs to keep it bounded.
if (allowTokenRotation) {
services/apps/connectors_worker/src/activities/dispatcherActivities.ts:30
hasHeadroomonly reads the cached remaining budget; this loop never reservesDEFAULT_RUN_ESTIMATE. Therefore every claimed unit for the same integration sees the same snapshot and all can be admitted, defeating budget admission and creating a quota burst. Aggregate units per pool and atomically reserve capacity, or cap admissions from one snapshot before starting runs.
if (await pool.hasHeadroom(DEFAULT_RUN_ESTIMATE)) {
pnpm-lock.yaml:6909
- Adding the two workspace importers also rewrites unrelated existing resolutions, including downgrading
esbuildfrom 0.28.2 to 0.28.0 and changing AWS/debug/follow-redirects entries. This broadens the dependency blast radius contrary to the focused-change convention (CLAUDE.md:70). Regenerate with the repository's pinned pnpm version while preserving unrelated resolutions.
services/libs/connectors/src/types.ts:19 - This future-plan note will become stale as credential variants are added. The type already expresses the current supported variant; track the planned union expansion in the ticket rather than source commentary.
services/libs/connectors/src/credentials.ts:17 - This scope/future-work note does not explain an invariant needed to use the switch and will become stale as cases are added. Keep the supported platforms self-evident in the cases and track migration scope in the ticket.
// POC scope: GitHub only; each migrated connector adds its platform case here
services/libs/connectors/src/credentials.ts:28
- This comment documents the temporary implementation and a future replacement rather than an invariant callers must respect. Track the secret-manager migration in a ticket and let the function name/configuration describe the current behavior.
// POC only: secrets come from env; secret-manager adoption (OCI Vault) swaps this
// body without touching callers — getCredential stays the single entry point
services/apps/connectors_worker/src/main.ts:36
- This note describes why the dummy connector was added, not a non-obvious runtime invariant, and will stale when the POC evolves. The
IS_DEV_ENVguard already makes the behavior clear.
// POC only: dummy connector drives the control-plane end-to-end in dev
| const memberAttributesSchema = z.object({ | ||
| isHireable: z.boolean(), | ||
| url: z.string(), | ||
| bio: z.string(), | ||
| location: z.string(), |
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 266690b. Configure here.
| if (installations.length === 0) { | ||
| throw new Error('github app has no installations') | ||
| } | ||
| return String(installations[0].id) |
There was a problem hiding this comment.
Silent pick of first GitHub installation
Medium Severity
resolveInstallationId falls back to installations[0] when CROWD_GITHUB_INSTALLATION_ID is unset. A GitHub App with multiple installations then mints a token for an arbitrary org, so runtime seedGithubTokens and discoverRepos can target a different installation than the seed script's --installation-id. That used to fail closed; it now syncs the wrong org or 404s.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 266690b. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (9)
Previously missed (1) — in code that hasn't changed since the last review.
services/libs/connectors/src/http/client.ts:52
- The new retry and token-rotation state machine has no automated coverage, despite similar provider clients being covered with Vitest. Add tests for unavailable retries/backoff, rate-limit parking and one-time rotation, auth quarantine, custom interpretation, and retry exhaustion; regressions here can multiply provider traffic or disable valid credentials.
services/apps/connectors_worker/src/workflows/dispatcher.ts:22
startRunreturns as soon as Temporal accepts the child workflow, soexecuteSynccan concurrently write a rate-limitresumeAtwhile this activity writes the normal cadence. The last update wins, which can overwrite the provider reset time and run the unit too early. Make one code path ownnextRunAt, or combine dispatch/rescheduling into an atomic state transition.
await activity.startRun(unit)
await activity.reschedule(unit.id, unit.platform, unit.syncName)
services/apps/connectors_worker/src/activities/dispatcherActivities.ts:25
- This serial loop can perform one 30-second GitHub budget probe per distinct integration, while the dispatcher activity has a one-minute start-to-close timeout. A few stale or slow pools can therefore make admission repeatedly time out before any units are dispatched. Probe with bounded concurrency and heartbeat progress, or split/extend the activity timeout accordingly.
for (const unit of units) {
services/apps/connectors_worker/src/activities/dispatcherActivities.ts:30
hasHeadroomonly reads the current bucket; it does not reserveDEFAULT_RUN_ESTIMATE. Because admission completes before any workflow consumes budget, 100 units sharing a pool can all be admitted when only 50 requests remain. Reserve the estimate atomically (or track remaining capacity per pool during this pass) so admission actually caps concurrent work.
if (await pool.hasHeadroom(DEFAULT_RUN_ESTIMATE)) {
services/libs/connectors/src/types.ts:19
- This describes a future implementation plan rather than a non-obvious invariant. Remove it or track the discriminated-union work with a
TODO(CM-XXX):ticket, as required by the repository's comment policy.
services/libs/connectors/src/credentials.ts:17 - This comment restates the switch's current scope and future direction. The code is self-explanatory; remove the comment or use a ticketed
TODO(CM-XXX):if follow-up work must be tracked.
// POC scope: GitHub only; each migrated connector adds its platform case here
services/libs/connectors/src/credentials.ts:28
- This is an unticketed note about a planned secret-manager migration, which the repository comment policy disallows. Remove it or replace it with a concrete
TODO(CM-XXX):linked to the follow-up ticket.
// POC only: secrets come from env; secret-manager adoption (OCI Vault) swaps this
// body without touching callers — getCredential stays the single entry point
services/apps/connectors_worker/src/main.ts:36
- This comment only narrates the immediately following development-only condition. Remove it so the code remains self-explanatory under the repository's comment policy.
// POC only: dummy connector drives the control-plane end-to-end in dev
services/libs/connectors/src/testing/dummyConnector.ts:15
- These records are published as
IntegrationResultType.ACTIVITY, but{ tick }has neither a member/username nor the required activity fields. The data-sink worker consequently marks every dummy result as an unrepeatable failure, so the development connector cannot exercise the pipeline successfully. Emit a contract-valid activity or route dummy records to an appropriate result type.
WIP