Conversation
d-klotz
commented
Jun 5, 2025
| // Removes "Bearer " and trims | ||
| const token = authorizationHeader.split(' ')[1].trim(); | ||
| // Load user for later middleware/routes to use | ||
| req.user = await User.newUser({ token }); |
Contributor
Author
There was a problem hiding this comment.
User.newUser is misleading, it does not create a new user, instead it retrieves a user based on the JWT token.
Contributor
Author
There was a problem hiding this comment.
User.fromToken({ token }) or User.getByToken({ token }) would be a better fit.
Contributor
There was a problem hiding this comment.
Yup agreed. I was using existing methods though.
d-klotz
commented
Jun 6, 2025
| const appDefinition = backendJsFile.Definition; | ||
|
|
||
| const backend = createFriggBackend(appDefinition); | ||
| const loadRouterFromObject = (IntegrationClass, routerObject) => { |
Contributor
Author
There was a problem hiding this comment.
"Object" is too vague, I have no clue what can be in there or what that means
Contributor
Author
There was a problem hiding this comment.
I think we could call it loadIntegrationRoutes
Contributor
Author
There was a problem hiding this comment.
Also the routerObject is already inside of IntegrationClass isn't it?
Contributor
There was a problem hiding this comment.
The overall method of loadIntegrationRoutes makes sense, or integration defined routes.
The load from object method was intended to load from a... route object? Dunno what to call it. The "method", "path", "event" object. I'm not even sure where I got that concept from except it's a common way to represent an http endpoint. Minus the event thing. That I want to be an easy way for a dev to reference when creating event handlers.
|
|
||
| for (const routeDef of IntegrationClass.Definition.routes) { | ||
| if (typeof routeDef === 'function') { | ||
| router.use(basePath, routeDef(IntegrationClass)); |
Contributor
Author
There was a problem hiding this comment.
I don't understand this.
You're looping through IntegrationClass.Definition.routes and from every routeDef you're passing the same integration as parameter?
Contributor
Author
There was a problem hiding this comment.
Do you have an example of a place where we declare routes as a function and need it's own integration class as parameter?
Contributor
Author
There was a problem hiding this comment.
Why not stick to one type of route definition? Either function or object
Contributor
There was a problem hiding this comment.
I think the rationale was that in some cases, you need to have some logic that runs outside of express route generation. https://github.com/lefthookhq/frigg-2.0-prototyping/blob/460ad99b85b5a53bac5a859de8b8d3780b85937d/backend/src/testRouter.js#L8
In other cases, relying on the normal instantiation is fine and for those you can reach for either the straight express route or the static object.
I wanted to provide a "quick and easy", "moderate complexity", "high complexity" set of options.
That said I'm not sure what I did was the way to do it.
| router[method.toLowerCase()](path, async (req, res, next) => { | ||
| try { | ||
| const integration = new IntegrationClass({}); | ||
| await integration.loadModules(); |
Contributor
Author
There was a problem hiding this comment.
We already loadModules in the IntegrationBase constructor, we could registerEventHandlers in the constructor as well.
Contributor
There was a problem hiding this comment.
I think I had an issue of not wanting to load event handlers yet because of some dynamic issues? Or something else. But, happy to not duplicate invoking!
| await integration.loadModules(); | ||
| await integration.registerEventHandlers(); | ||
| const result = await integration.send(event, {req, res, next}); | ||
| res.json(result); |
Contributor
Author
There was a problem hiding this comment.
Do we really have to return the result here?
Developers will by instict call the http response when it's available to them. I was not aware that this existed until now and probably other developers won't either.
Contributor
There was a problem hiding this comment.
Well, the intent was to remove the need to think about express inside the handler function. Just, do what you need, then if you need to return something, return it.
Contributor
Author
There was a problem hiding this comment.
But if someones sees a "res" object, one would not simply return and ignore "res".
BTW, one of the reasons I don't like ruby on rails is because it does too much magic and I don't know why and how 🫠
| for (const [entityId, key] of Object.entries( | ||
| integrationRecord.entityReference | ||
| )) { | ||
| const moduleInstance = |
Contributor
Author
There was a problem hiding this comment.
Are the modules not already instantiated in IntegrationBase -> loadModules() when we do:
const instance = new integrationClass({
userId,
integrationId: params.integrationId,
});
Contributor
Author
There was a problem hiding this comment.
OK, looks like loadModules doesn't actualy loads modules but it simply instantiates the module api.
Contributor
There was a problem hiding this comment.
There's a difference between "load module definitions" and "load module entities into the module instance"
| integrationRecord.config.type | ||
| ); | ||
|
|
||
| const instance = new integrationClass({ |
Contributor
Author
There was a problem hiding this comment.
rename to integrationInstance
| // for each entity, get the moduleinstance and load them according to their keys | ||
| // If it's the first entity, load the moduleinstance into primary as well | ||
| // If it's the second entity, load the moduleinstance into target as well | ||
| const moduleTypesAndKeys = |
Contributor
Author
There was a problem hiding this comment.
I think this comment also does not make sense anymore, right?
Contributor
There was a problem hiding this comment.
The first line does make sense. The second and third lines are for backwards compatibility, where we enforced a "primary" and "target" concept via naming conventions.
Contributor
There was a problem hiding this comment.
I don't think we need to maintain that though. Let things error and people correct the errors.
| moduleClass && | ||
| typeof moduleClass.definition.getName === 'function' | ||
| ) { | ||
| const moduleType = moduleClass.definition.getName(); |
Contributor
Author
There was a problem hiding this comment.
moduleType also comes from the name?
Contributor
There was a problem hiding this comment.
Yes, though we can debate that
| const integrationClassIndex = this.integrationTypes.indexOf(type); | ||
| return this.integrationClasses[integrationClassIndex]; | ||
| } | ||
| getModuleTypesAndKeys(integrationClass) { |
Contributor
Author
There was a problem hiding this comment.
I dont get what this does
Contributor
There was a problem hiding this comment.
I don't think this implementation is fully what I intended. But anywho, the goal is that we grab an integration definition, and from it we can determine which api modules are part of it, which allows us to get the required module entity instances in order to create a complete integration record (TODO allow for a module to be optional and not required on creation of an integration record, potentially make them required on a per event basis, ie we throw an error/force a user to assign a connection or create a new connection if they go to use a feature that is only available if they have a specific module instance added).
The direct use case is during the management inside the frontend experience. The ui should see "user wants to create a HubSpot integration. The HubSpot integration requires two modules. The user has one module connection (entity) available to use, but needs the other one. I'll run the auth flow for the other one and then ask them to confirm the use of that new connection."
The reason I say this implementation may not be what I intended is that we should allow multiple modules of the same type to be assigned different module names so you have something like "slackUser" and "slackApp" both pointing to the slack api module.
seanspeaks
reviewed
Jun 7, 2025
seanspeaks
left a comment
seanspeaks
left a comment
Contributor
There was a problem hiding this comment.
Did my comments to your comments and a few added comments in there
| { | ||
| "$schema": "node_modules/lerna/schemas/lerna-schema.json", | ||
| "version": "1.2.2", | ||
| "version": "2.0.0-next.0", |
Contributor
There was a problem hiding this comment.
I have no idea if this should be committed 😅
| const appDefinition = backendJsFile.Definition; | ||
|
|
||
| const backend = createFriggBackend(appDefinition); | ||
| const loadRouterFromObject = (IntegrationClass, routerObject) => { |
Contributor
There was a problem hiding this comment.
The overall method of loadIntegrationRoutes makes sense, or integration defined routes.
The load from object method was intended to load from a... route object? Dunno what to call it. The "method", "path", "event" object. I'm not even sure where I got that concept from except it's a common way to represent an http endpoint. Minus the event thing. That I want to be an easy way for a dev to reference when creating event handlers.
| router[method.toLowerCase()](path, async (req, res, next) => { | ||
| try { | ||
| const integration = new IntegrationClass({}); | ||
| await integration.loadModules(); |
Contributor
There was a problem hiding this comment.
I think I had an issue of not wanting to load event handlers yet because of some dynamic issues? Or something else. But, happy to not duplicate invoking!
|
|
||
| getIntegrationById: async function(id) { | ||
| return IntegrationModel.findById(id); | ||
| getIntegrationById: async function (id) { |
Contributor
There was a problem hiding this comment.
I really need to lock this concept into my brain. Git repo takes the entire definition of "repository" in my head.
| const integration = | ||
| await integrationFactory.getInstanceFromIntegrationId({ | ||
| integrationId: integrationRecord.id, | ||
| userId: getUserId(req), |
Contributor
There was a problem hiding this comment.
I think we discussed this on our call. Likely just was throwing things that stuck, for context on why this is here.
| @@ -349,9 +375,7 @@ function setEntityRoutes(router, factory, getUserId) { | |||
| throw Boom.forbidden('Credential does not belong to user'); | |||
Contributor
There was a problem hiding this comment.
I think there were moments where this failed? Dunno what those moments were/are though.
| const {ModuleConstants} = require("./ModuleConstants"); | ||
| const { ModuleConstants } = require('./ModuleConstants'); | ||
|
|
||
| class Auther extends Delegate { |
Contributor
There was a problem hiding this comment.
All for it. At one point @MichaelRyanWebber and I were debating both naming and intent of the class (hi Michael! Not to rope you in but, you likely can either find the note somewhere or comment on the future improvements you/we had in mind).
| this.EntityModel = definition.Entity || this.getEntityModel(); | ||
| } | ||
|
|
||
| static async getInstance(params) { |
Contributor
There was a problem hiding this comment.
Async construction.
It's a debate in nodejs world. Might be a resolved debate in node 20? But basically "if you need to await/promise something in order to instantiate a class, do you make it an async constructor, or do you create a static async instantiation method?" Aka the get
That's the root of this decision at any rate.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23374074 | Triggered | Generic Password | 5c1d197 | .claude/skills/frigg-canary-test/assets/harness/docker-compose.yml | View secret |
| 23374074 | Triggered | Generic Password | 8eb6309 | .claude/skills/frigg-canary-test/assets/harness/docker-compose.yml | View secret |
| 23374074 | Triggered | Generic Password | 9b7e917 | .claude/skills/frigg-canary-test/assets/harness/docker-compose.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
SonarQube Cloud flagged three style issues on PR #579: - javascript:S1186 (MAJOR) × 2: `hangingFetch` and `okFetch` were declared inside the `describe('AbortController timeout behavior')` block; hoist them to module scope so each describe can reuse them without the nested-function cost. - javascript:S3799 (MINOR): `okFetch(body = { ok: true })` uses an object literal as a default parameter. Replace with an `undefined`-check that picks up the default inside the body, avoiding the fresh-object-per-call allocation pattern the rule guards against. No functional change; all 15 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lStack
LocalStack queues were being created with only `QueueName`, dropping
every CloudFormation `Properties` field. AWS (via CloudFormation)
applies the full Properties block — notably `VisibilityTimeout: 1800`
that integration-builder emits for integration queues — so local
emulation diverges from production.
Observed impact: the HubspotQueue queue worker's handler runs for
minutes (initial sync of a real firm), but SQS's default 30 s
VisibilityTimeout makes the message re-appear as visible every 30 s.
The same message gets delivered to a second worker invocation while
the first is still processing, which breaks the "one worker per
message" invariant Frigg assumes.
Change
- `extractQueueDefinitions` now looks up each queue's matching
`AWS::SQS::Queue` resource in `serverless.service.resources.Resources`
and attaches its `Properties` to the queue definition. If the
resource is missing, non-SQS, or has no Properties, the behavior is
unchanged (back-compat).
- `LocalStackQueueService.createQueue` accepts an optional
`attributes` argument and forwards it to SQS `CreateQueue`. When
`attributes` is missing/empty the call shape matches the previous
`{QueueName}`-only behavior.
- `LocalStackQueueService._propertiesToAttributes` whitelists the 15
CloudFormation Properties that map 1:1 onto SQS CreateQueue
Attributes and drops everything else (Tags, QueueName, etc.). Object
values like `RedrivePolicy` get JSON-encoded as AWS expects.
- `createQueues` wires the two together.
Tests: 39/39 pass (9 new cases covering the Properties-forwarding path,
back-compat for definitions without resources, and the property-to-
attribute mapping).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rom queue attributes Addresses Codex review P1 on PR #580. Serializing every CloudFormation `Properties` value with `JSON.stringify` would forward unresolved CloudFormation intrinsics (`Fn::GetAtt`, `Ref`, `Fn::Sub`, …) into LocalStack's `CreateQueue` call. The concrete case: integration-builder.js emits RedrivePolicy: { maxReceiveCount: 3, deadLetterTargetArn: { 'Fn::GetAtt': ['InternalErrorQueue', 'Arn'] } } CloudFormation resolves the Fn::GetAtt to a real ARN in AWS, but at plugin-time the value is still the raw intrinsic object. SQS's `RedrivePolicy` requires `deadLetterTargetArn` to be a valid ARN string — passing the JSON blob either fails the CreateQueue call or produces a queue with malformed DLQ config. Change - Added `LocalStackQueueService._containsUnresolvedIntrinsic(value)`: recursively detects `Fn::*` and `Ref` keys in CloudFormation Property values. - `_propertiesToAttributes` now checks each attribute's value for unresolved intrinsics and DROPS the attribute (with a console.warn) instead of stringifying it. The rest of the attributes pass through untouched — so `VisibilityTimeout` (the main win of this PR) still gets applied locally; the DLQ association is skipped until the plugin gains a proper intrinsic resolver. Tests - 4 new unit tests covering the intrinsic-filter behavior, `Ref` handling, nested intrinsic detection, and the happy path where a resolved ARN string passes through untouched. - 43/43 plugin tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lags
`UpdateProcessMetrics` and `UpdateProcessState` both did read-modify-
write against `Process.context` / `Process.results` JSON blobs. Under
concurrent queue-worker invocations (any integration with
`reservedConcurrency > 1`) the spread-and-save pattern clobbers later
writes with stale snapshots: counters drift low and context flags such
as `fetchDone` silently revert to undefined, leaving syncs stuck in
non-terminal states.
Adds `applyProcessUpdate(processId, ops)` to the ProcessRepository
interface and implements it natively in each adapter:
- PostgreSQL: one UPDATE ... RETURNING * with nested `jsonb_set`
expressions for increments, sets, and bounded-array pushes. Row
locking inside the single statement serializes concurrent callers.
- MongoDB: findAndModify via `$runCommandRaw` with `$inc` / `$set`
/ `$push` + `$slice`. Server-side atomic.
- DocumentDB: same operator set as Mongo; adds compat note for
`$slice` (requires DocumentDB 4.0+).
Validation (regex-checked dot-paths rooted at `context|results`,
whitelisted op kinds) lives in a shared `process-update-ops-shared`
module so every adapter fails fast before touching the DB.
Refactors `UpdateProcessMetrics` to split into an atomic phase
(counters + bounded error history) and a best-effort non-atomic
follow-up for derived fields (duration, recordsPerSecond,
estimatedCompletion) so existing consumers of those fields see no
contract change. Derived-field write failures are logged and do NOT
fail the overall call — the atomic counters already landed in phase 1.
Refactors `UpdateProcessState` to route context updates through
`applyProcessUpdate` when the repo supports it, falling back to the
original read-merge-write path for custom repos that don't. Existing
"shallow top-level merge" semantics are preserved.
Removes the long-standing TODO banner on `update-process-metrics.js`
documenting this exact race. Adds 57 unit tests covering validation,
op shapes, race simulation, empty-batch short-circuit, derived-field
non-fatal failures, and the legacy fallback in `UpdateProcessState`.
This commit is additive: `repository.update(id, patch)` is unchanged,
no schema migration, no behavior change for callers that don't opt in
to `applyProcessUpdate`.
Note on branch scoping: this change lands on `claude/plugin-queue-
attributes` per downstream consumer request. The branch now mixes a
serverless-plugin fix (earlier commits) with a core fix; consumers
reviewing this PR should treat the two concerns as separable for
bisection purposes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses two correctness issues flagged on PR #580 review of c0487bc: B1 (Postgres pushSlice ordering): `jsonb_agg(elem)` without an explicit `ORDER BY` clause produces implementation-defined order even over a `WITH ORDINALITY` subquery. Added `ORDER BY idx` inside the aggregate so insertion order is preserved deterministically across Postgres versions. While there: wrapped the combined-array expression in a CTE so `${newArr}` is evaluated once on the server rather than reconstructed three times in the outer expression. B2 (intermediate-path silent no-op): `jsonb_set(target, path, value, create_missing=true)` only creates the LEAF segment if missing — intermediate segments that don't exist as objects cause the call to return `target` unchanged. For depth-1 paths (our current only callers) this was fine, but the interface JSDoc advertises arbitrary depth (`context.pagination.cursor` is in the example). Added an `ensureParents` helper that wraps each intermediate prefix path in a `jsonb_set(..., COALESCE(cur #> path, '{}'::jsonb), true)` call, preserving existing content and synthesizing `{}` where missing. Known artifact documented in the generated-SQL test: the string- substitution nature of `ensureParents` causes SQL size to grow as O(2^N) in path depth. Fine for realistic depths (≤3 segments); a future optimization could materialize `cur` once per iteration via CTE to make it O(N). No runtime correctness impact — Postgres still executes the whole expression as a single UPDATE under a single row lock. Adds `process-repository-postgres.test.js` with 10 SQL-generation tests covering: - state-only UPDATE (no jsonb_set chains) - depth-1 increment (single jsonb_set wrap, no parent synthesis) - depth-2 set (1 parent synthesis + leaf) - depth-3 set (ensureParents substitution artifact documented) - pushSlice emits `ORDER BY idx` and binds values exactly once - parameter binding order is stable left-to-right - combined increment/set/pushSlice/newState in one UPDATE - null return when UPDATE hits zero rows - validateOps rejects invalid paths before DB call - depth-1 set skips parent synthesis (no wasted jsonb_set) Test coverage gap remaining (follow-up): no real-Postgres race test — the Frigg monorepo has MongoDB-memory-server for Mongo, but no equivalent for Postgres. Adding one requires a team decision on pg-mem vs testcontainers vs CI-provisioned. The SQL-generation tests here + Postgres's documented single-UPDATE atomicity provide the correctness argument without the infrastructure cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Postgres schema at `packages/core/prisma-postgresql/schema.prisma` declares two artifacts that no migration in `prisma-postgresql/migrations/` ever creates. `prisma migrate deploy` runs to completion (there's nothing to apply for those declarations) and the generated client is happy, so the drift only surfaces as P2021 / P2022 when a downstream app actually writes to the affected paths. Missing pieces: 1. `Entity.data Json @default("{}")` was added to the schema but never migrated. ModuleRepositoryPostgres.createEntity / findEntity persist and rehydrate any `identifiers`/`details` fields that fall outside the six named columns through this JSONB blob — so the moment an integration's `getEntityDetails` returns a field like `firm_subdomain`, prisma.entity.create throws `Unknown argument 'userId'. Did you mean 'user'?` (the first argument Prisma can't place is the one it names in the error — confusingly unrelated to the real cause). Added `20260422120000_add_entity_data_column` — a single `ALTER TABLE "Entity" ADD COLUMN IF NOT EXISTS "data" JSONB NOT NULL DEFAULT '{}'` so the migration is safe to re-apply on environments that may have worked around the drift locally. 2. The entire `Process` model is declared in the schema but no migration creates the table, its six indexes, or the three FK constraints (user, integration, self-referential parent). ProcessRepositoryPostgres and FriggProcessManager depend on this for long-running-job state tracking — fan-out sync progress, batched state machines — so any app that uses the new process primitive hits P2021 on first `prisma.process.create`. Added `20260422120001_create_process_table` with the table definition, all declared indexes, and the three FK constraints matching the relations in schema.prisma (userId→User CASCADE, integrationId→Integration CASCADE, parentProcessId→Process SET NULL). Downstream apps currently working around these with local patch migrations (e.g. lefthookhq/clockwork-recruiting--frigg's `scripts/patch-core-migrations.js`) can delete that workaround after bumping to the release that includes this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Under heavy concurrent fan-out sync load, worker Lambdas were hanging the
full 900s timeout with no logs or errors. Root cause: three compounding
infrastructure defaults.
1. Prisma engineType="binary" — schema.prisma forced the binary engine,
which forks a separate Rust query-engine child and communicates over
an HTTP/IPC pipe that has NO client-side timeout on the Node side. A
zombied child wedges the calling process until Lambda kills it.
Switch both postgres and mongodb schemas to engineType="library" (the
Prisma 3.x+ default), which loads the Rust engine as a Node-API addon
in-process. Native crashes now surface as Runtime.ExitError instead of
15-min hangs.
2. Aurora Serverless v2 MaxCapacity=1 ACU — at 0.5–1 ACU Aurora is CPU-
bound under 20-way concurrent writes, inflating query latency and
compounding pool pressure. Bump the default to 4 ACU; apps can still
override via dbConfig.maxCapacity. Scales to MinCapacity when idle so
cost impact is minimal.
3. DATABASE_URL had no pool or timeout params — a blocked query or row
lock would wait forever. Append Prisma + libpq params at URL build
time:
connection_limit=1 (one pg conn per Lambda container; rely on Lambda
concurrency for parallelism — AWS best practice)
pool_timeout=10 (throw P2024 after 10s instead of hanging)
connect_timeout=10 (bound TCP/TLS handshake)
socket_timeout=60 (kill dead sockets when server never responds)
options=-c statement_timeout=30000 -c lock_timeout=10000
(Postgres-side 30s query cap / 10s lock-wait cap
— aborts with SQLSTATE 57014 / 55P03 rather than
silently sitting in pg_stat_activity)
Any combination of these defaults could mask the other two; all three
had to ship together to make workers fail loud and fast instead of
silently hanging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to f1cb41c, addressing code-review concerns: 1. connection_limit 1 → 2, pool_timeout 10 → 20 core has multiple in-handler Promise.all patterns against Prisma: get-process.executeMany (parallel fetch), field-encryption-service (batched decrypt). With connection_limit=1 these would serialize behind a single pool slot, and under Aurora pressure each later query gets less remaining budget before P2024. Bumping to 2 removes the cliff while staying safe against max_connections (200 Lambdas × 2 = 400 conns, fits aurora-pg 15 at 4 ACU). pool_timeout=20 still fails fast relative to Lambda's 900s cap but gives fan-outs headroom. 2. Cover the use-existing and discover-no-secret runtime-construction paths Those paths export DATABASE_HOST / DATABASE_PORT / DATABASE_NAME / DATABASE_USER / DATABASE_PASSWORD and expect consumer code to assemble DATABASE_URL at runtime. Previously got NONE of the new timeouts — a real gap. Extract LAMBDA_DATABASE_URL_QUERY_PARAMS to a module-scoped constant and export it as a `DATABASE_URL_PARAMS` env var on those two paths, with a log hint telling consumers to append `?${DATABASE_URL_PARAMS}` when building the URL. Single source of truth for the params; no drift between paths. 3. Tests now assert all five param components (connection_limit, pool_timeout, connect_timeout, socket_timeout, statement_timeout, lock_timeout) so accidental removal of any is caught. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-logs feat(core): add observability logs to queue workers
Previously the GET /api/authorize handler dropped req.query.state on the
floor: it called getModuleInstanceFromType.execute(userId, entityType)
without forwarding any extra params, so the Module constructor's apiParams
never received state. As a result, the OAuth2Requester base — which
already accepts `state` via get(params, 'state', null) — always saw
state = null, and downstream API modules that hardcode &state=${this.state}
in their authorize URL (e.g. api-module-hubspot) interpolated state=null.
This commit threads state through:
- integration-router.js: GET /api/authorize passes
{ state: req.query.state } as the third arg.
- get-module-instance-from-type.js: execute(userId, type, options = {})
forwards options.state to the Module constructor.
- module.js: Module constructor accepts state and merges { state } into
apiParams when truthy, so OAuth2Requester picks it up.
Backwards-compatible: state defaults to null, no callers need to change.
Modules that conditionally append state (e.g. api-module-clockwork-recruiting
which uses `if (this.state) params.append('state', ...)`) continue to omit
the param when no state is provided.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…butes fix(serverless-plugin): apply CloudFormation queue Properties to LocalStack
…eout fix(core): add per-request timeout to Requester to catch silent fetch hangs
ProcessAuthorizationCallback relied solely on the DLGT_TOKEN_UPDATE notification chain (setTokens -> notify -> Module.onTokenUpdate -> upsertCredential) to persist freshly-issued OAuth tokens. In production the chain has been observed to silently no-op: callers complete OAuth on the provider side (issuing new tokens and invalidating the prior refresh_token), but the new tokens never land in the DB. The user- visible /api/authorize call appears to succeed, the existing credential is reused with stale token data, and downstream API calls fail with 401 once the provider rotates the old refresh_token. Changes: - Bootstrap the Module with the existing entity (looked up via findEntitiesByUserIdAndModuleName) so the API requester is preloaded with prior tokens. Replaces the `entity = null` TODO. - Belt-and-suspenders: explicitly call onTokenUpdate after getToken on the OAuth2 path. The notification chain remains in place; this ensures persistence happens even when the chain doesn't fire. - Add diagnostic logging at entry, after getToken, after persistence, and after restore so future investigations are not blind. - Wrap POST /api/authorize handler with try/catch + console.error so failures surface in CloudWatch instead of returning silently. - restoreIntegrationsForEntity returns the count of flipped integrations for logging. Tests: existing re-auth status restoration tests pass; new test locks in that upsertCredential is called once on the OAuth2 path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reinstates the `{ state: req.query.state }` third argument to
getModuleInstanceFromType.execute in the GET /api/authorize handler.
Was inadvertently dropped when applying this branch's changes from a
working tree based on a commit predating f660549 (feat(core): forward
OAuth state from /api/authorize to module API).
Without this, modules that hardcode &state=${this.state} in their
authorize URL (e.g. api-module-hubspot) interpolate state=null again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n-oauth-reauth fix(core): explicitly persist tokens on OAuth2 re-auth
…ent credential
When a user re-authenticates against a different workspace/account of the
same provider (e.g. switching between two Pipedrive workspaces),
upsertCredential matches/creates a credential keyed by externalId from
getCredentialDetails — but findEntity matches the entity by its own
externalId from getEntityDetails. These keys can resolve to different
credential rows when the user has multiple workspaces, leaving the
entity still linked to the prior workspace's credential after re-auth.
Without this fix, the OAuth callback reports
{credentialId, entityId} where credentialId is the freshly-persisted
credential, but the entity in the DB still references a different
credential. Downstream integration creation reads the stale link and
runs against the prior workspace's tokens, while the just-issued tokens
sit on a different credential row no entity points to.
Repoint the entity's credentialId via moduleRepository.updateEntity
when findOrCreateEntity returns an existing entity whose credentialId
differs from the just-upserted credential.
Tests: two new cases — repoints when upsert produces a different
credential, no-ops when the entity already points at the upserted one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n-oauth-reauth fix(core): repoint entity credentialId when re-auth produces a different credential
…ce-ef760e # Conflicts: # packages/core/integrations/integration-base.js # packages/core/modules/requester/requester.js
) * docs(adr): mark ADR-010 and ADR-011 as Accepted Both ADRs were merged to next in #612; flip Status Proposed -> Accepted (dated 2026-07-03) in the ADRs and the index so a merged decision no longer reads as under-discussion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o * docs(adr): ADR-012 database schema migrations + ADR-013 integration version migrations Add concept ADRs for the two migration types that lacked one, each opening with the three-type taxonomy so "migration" stops being conflated (project scaffold = ADR-004; db schema = 012; integration version = 013). ADR-012 ratifies the in-VPC Prisma migrator (Lambda + router, S3 status, multi- engine, credential-safe) as deploy-time infra, separate from the admin runner. ADR-013 frames integration version migration as an admin operation on the ADR-010/ 005 runner (persisted, resumable, isolated), and explicitly defers the integration versioning contract to a separate forthcoming ADR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o * docs(adr): retitle ADR-004 to Project Structure Migration Tool + disambiguate Rename the ambiguous "Migration Tool Design" to make clear it is the project- scaffold migration (create-frigg-app -> frigg init), add the three-type migration taxonomy + cross-links to ADR-012 (db schema) and ADR-013 (integration version), and note it is a build-time CLI that never touches the database or integration records. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o * docs(adr): ADR-014 propose one numbered ADR register Proposal to reconcile the two ADR collections (numbered docs/architecture- decisions/ + 12 name-slugged docs/architecture/ADR-*.md) into a single numbered register with one structure: NNN-kebab-title.md, "# ADR-NNN: Title" heading (number AND name), Status/Date/Deciders block, standard sections, one README index. Includes a git-mv fold-in plan and proposed 015-026 assignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o * docs(adr): accept ADR-014 and document the ADR conventions in the README Flip ADR-014 (One Numbered ADR Register) to Accepted, and add a Conventions section to the register README (location, NNN-kebab filename, "# ADR-NNN: Title" heading, Status/Date/Deciders block, section set, single index, stable numbers) so contributors see the rules without opening the ADR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o --------- Co-authored-by: Claude <noreply@anthropic.com>
#621) Pure restructure, no content changes (per ADR-014). git mv the 12 name-slugged docs/architecture/ADR-*.md into docs/architecture-decisions/ as 015-026, rewrite each H1 to "# ADR-NNN: Title", map **Author** -> **Deciders**, and repoint all intra-cluster cross-links to the new NNN-slug.md paths. Add index rows. Also repoints stale links that pointed at the previously-deleted ADR-EXTENSIONS.md (now superseded by 015-extensions-taxonomy.md) in EXTENSIONS.md, the frigg- extensions skill, and the global-entities guide. Non-ADR files under docs/architecture/ are left in place. Statuses preserved. Claude-Session: https://claude.ai/code/session_019BQcoqYva5TXM2DQAyEG5o Co-authored-by: Claude <noreply@anthropic.com>
…, lambda.scopedEnvironment Allows environment values to be boolean | 'ssm' (offload to Parameter Store), adds ssm.parameterPrefix / ssm.kmsKeyArn / ssm.restrictIamToPrefix, and a top-level lambda.scopedEnvironment flag for per-function env scoping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Core needs it for the runtime parameter loader; devtools already lazy-required it in the AWS provider adapter and management-ui without declaring it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ence to match implementation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fetches SSM-offloaded env vars at cold start (GetParameters, batched, decrypted) and hydrates process.env before handlers run. Real env vars always win; loader-owned keys refresh on a TTL (default 300s); a failed refresh keeps stale values instead of erroring warm containers; missing declared parameters fail fast by name. Wired into createHandler after secretsToEnv. Part of ADR-027. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in deployment IAM New 'frigg ssm push' writes offloaded values from CI env/.env to Parameter Store (SecureString via ssm.parameters, size/tier validation, missing-value errors with --allow-empty escape). frigg deploy runs the push before the serverless deploy so parameters exist before new code cold-starts; push failure aborts the deploy. Deployment IAM templates gain ssm:PutParameter/DeleteParameter/AddTagsToResource within the existing *frigg* scope. Part of ADR-027. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ment) Builders can emit function-scoped env maps (functionEnvironments) that the composer applies after all functions exist — unknown targets are a hard build error, and builder-set function env keys are never clobbered. Behind lambda.scopedEnvironment and skipped in local mode (LocalStack queue URLs are provider-level). Scoped consumer sets (verified against core): - <NAME>_QUEUE_URL -> auth + admin-script functions + owning integration's router/webhook/extension handlers/queue worker - scheduler vars -> same union except adminScriptRouter (keeps its own admin-scheduler role) - migration bucket/queue vars -> dbMigrationRouter/Worker; DB_TYPE stays global - ADMIN_SCRIPT_QUEUE_URL -> admin functions only Part of ADR-027. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ts + flag-off lock Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…inters/IAM, SSM VPC endpoint
environment-builder excludes 'ssm'-marked keys from provider.environment
when offload is active (plain ${env:KEY} fallback locally). SsmBuilder
emits SSM_PARAMETER_PREFIX + FRIGG_SSM_OFFLOADED_KEYS, a prefix-scoped
read grant (broad parameter/* grant kept unless ssm.restrictIamToPrefix),
and kms:Decrypt when ssm.kmsKeyArn is set. FriggSSMVPCEndpoint mirrors the
KMS interface endpoint, gated on offload + vpc. Rewrites stale composer/
integration test assertions that asserted the never-implemented extension
layer and two-segment prefix. Part of ADR-027.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… KMS-via-SSM, external migration scoping Addresses review findings on the ADR-027 branch: - ssm.parameters[KEY].tier (standard|advanced) so large offloaded values deploy via 'frigg deploy' without a deploy-only --tier flag; per-key oversize validation; --tier remains a CLI-wide override - frigg ssm push --region + resolution options.region || AWS_REGION || 'us-east-1' matching the composed stack default (was profile default) - deployment IAM grants kms:Encrypt/GenerateDataKey/Decrypt via ssm service (scoped to ssm.kmsKeyArn when set) so SecureString push with a CMK works - external migration-resources path now honors lambda.scopedEnvironment (was still broadcasting migration vars app-wide) - schema: ssm.parameters key pattern tightened to env-var names; tier field - docs: --allow-empty wording (skips, not stores empty) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, push retry/honesty, local-dev ssm.parameters fallback
Resolves 7 findings from the Sonnet review of the ADR-027 branch:
- generate-iam / generate now thread appDefinition.ssm.kmsKeyArn into the
IAM generator (was dead wiring -> account-wide kms wildcard); getFeatureSummary
surfaces it; integration-level test added
- frigg-deployment-iam-stack.yaml: SSM-via-KMS grant moved to its own
FriggSSMParameterKMSEncryption statement under CreateSSMPermissions (was
wrongly gated on CreateKMSPermissions)
- iam-policy-full.json: add kms:Encrypt to the KMS statement (standard-tier
SecureString PutParameter needs it; aligns flat policy with generator/yaml)
- frigg ssm push: PutParameter retry/backoff on throttling; a mid-batch
failure attaches pushed-so-far to the error and deploy's abort message
reflects real partial-push state instead of 'no resources changed'
- frigg-cli module.exports now includes ssmPushCommand
- environment-builder: ssm.parameters-only keys get the local ${env:KEY}
fallback (were invisible to frigg start), excluded when offload active,
no double-processing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…equire api-modules capture OAuth client credentials in a top-level Definition.env evaluated at cold-start module-require — before the handler-time loader (parametersToEnv) runs. Offloaded creds therefore snapshotted undefined and OAuth token exchange 401'd (broke all OAuth integrations). Fix (ADR-027 option 2, --import variant): ship ssm-preload.mjs in core and set NODE_OPTIONS=--import (appended to any app value) when offload is active. The ESM preload's top-level await fetches params and populates process.env BEFORE the entry module / api-modules load, so module-load credential reads see real values. Runs in the runtime's own process (same module resolution as the handler; no wrapper, no second process) vs the AWS_LAMBDA_EXEC_WRAPPER alternative. parametersToEnv stays as a lazy-read/TTL-refresh fallback. Shared fetch extracted to fetchOffloadedParameters(). NODE_OPTIONS emitted only when the offload set is non-empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eys for TTL refresh
Opus review of the INIT preload found (confirmed 2-3/3):
- BLOCKER-adjacent: NODE_OPTIONS=--import was emitted into provider.environment
(global). esbuild-bundled functions (defaultWebsocket, adopter custom fns)
do not package the preload .mjs, and a missing --import target fatally aborts
Node INIT -> those functions were bricked whenever offload was enabled (a
regression). Now applied per-function on skipEsbuild handlers only, in the
composer where function defs are visible. Also dropped the
'${env:NODE_OPTIONS}' prefix (it resolved the deploy host's shell at package
time, leaking into every Lambda).
- TTL refresh was silently defeated: the preload set process.env without
ownership, so the handler loader treated offloaded keys like console
overrides and never refreshed them. The preload now adopts the keys it set
into the shared in-process loader ownership (adoptPreloadedKeys) so the TTL
path re-fetches them; real-env values it left untouched are not adopted.
- docs/reference/ssm-configuration.md: added the INIT-phase preload section,
corrected precedence, documented the esbuild-function packaging caveat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- iam: match regional kms:ViaService with StringLike — StringEquals never
matches ssm.<region>.amazonaws.com, so the CMK-offload deploy path would
hit KMS access-denied (iam-generator + both IAM templates; the full-policy
StringLike also fixes the pre-existing lambda.*/s3.* wildcards it shares)
- vpc-discovery: populate ssmVpcEndpointId so an existing SSM interface
endpoint on a shared VPC is reused, not duplicated; endsWith('.ssm')
disambiguates from .ssmmessages
- offload-utils: explicit sort comparator (Sonar S2871)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- preload: honor real-env-wins when deciding WHAT to fetch, not just when setting. A key overridden directly on the Lambda whose SSM parameter is absent no longer fails INIT (defeated the documented console-override escape hatch). Logic moved into a tested CJS preloadOffloadedParameters(); the .mjs is now a thin shim over it. - ssm push: --allow-empty now verifies each skipped key already exists in Parameter Store and aborts otherwise — a green deploy can no longer leave a key in FRIGG_SSM_OFFLOADED_KEYS with no parameter and brick every function at cold start. - docs: correct precedence to real env > Secrets Manager > SSM (ssm-configuration ranked SSM above Secrets Manager; ADR-027 line 78 contradicted its own line 197 and the code). Document the advanced tier / --tier / --region and drop the absolute >4KB-rejected claim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…intness
- composer: applySsmPreloadNodeOptions now APPENDS the --import preload to any
NODE_OPTIONS already set on the function or (folded in) the provider, instead
of overwriting — so app flags (OTel auto-instrumentation, source maps, memory
tuning) survive. ADR-027 promised append; the code was clobbering. Only a
value already in the definition is appended, never a synthesized
${env:NODE_OPTIONS} (which would leak the deploy host's shell). Exported +
unit-tested with pre-existing function- and provider-level values.
- docs: correct ADR-027 ("the composer sets", appended) and document that
offloaded keys and SECRET_ARN bundle keys must be disjoint — an overlapping
key has undefined precedence (module-load sees SSM, handler sees Secrets
Manager, TTL refresh flips back). Stays disjoint in practice because
framework secrets are on the offload blocklist.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it-49ad1f feat: SSM parameter offload + per-function env scoping (Lambda 4KB env limit)
…ce-ef760e # Conflicts: # packages/core/core/create-handler.js # packages/core/package.json # packages/devtools/infrastructure/domains/shared/environment-builder.js # packages/devtools/infrastructure/domains/shared/environment-builder.test.js
When an OTLP-family telemetry exporter is configured, the telemetry
passthrough added OTEL_EXPORTER_OTLP_ENDPOINT/HEADERS as direct
`${env:KEY, ''}` Lambda vars unconditionally, before the environment
loop processed the `'ssm'` offload marker. For a key marked both as an
OTEL passthrough var and `'ssm'` (with offload active), the offload
branch only skipped adding a *second* entry via `continue`; it never
removed the passthrough entry. The var was therefore deployed as an
empty-string direct env var, and at runtime `'' !== undefined` blocked
the SSM loader from ever populating it, silently killing telemetry
export.
Skip the passthrough for any OTEL var that is in the active SSM offload
set (covering both `environment: 'ssm'` and `ssm.parameters`) so it
lives only in FRIGG_SSM_OFFLOADED_KEYS and the runtime loader can
populate it. Local mode / ssm-disabled still get the passthrough
fallback, and `true`-marked OTEL vars are unaffected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cut the explanation down to the one non-obvious fact (empty string blocks the SSM loader); dropped the lines restating what the code already says. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…f760e feat(core): ADR-011 — integration telemetry, eventing & usage tracking
The core migration router registers POST /admin/db-migrate/resolve
(db-migration.js) to recover from a Prisma P3009 (failed migration), but
migration-builder only emitted API Gateway routes for status, POST migrate,
and GET {processId}. So the resolve endpoint 404'd on every deployed app —
a single failed migration blocked all subsequent migrations for that stage
with no admin-API path to resolve it (recovery required direct DB writes).
Adds the missing httpApi event so the generated routes match the router.
Fixes #626.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exposing the resolve route (previous commit) wasn't enough: the route handler ran runPrismaMigrateResolve() synchronously in the ROUTER Lambda, which is packaged without Prisma (no layer, node_modules/prisma + WASM excluded). So a real call would 500 at runtime instead of resolving. Delegate to the worker Lambda (which has the Prisma CLI), mirroring the existing checkStatus direct-invoke pattern (GetDatabaseStateViaWorkerUseCase): - New ResolveMigrationViaWorkerUseCase invokes the worker with a resolve payload. - Worker gains a `resolve` action that runs runPrismaMigrateResolve + returns its result (validates migrationName + applied|rolled-back). - Router resolve route delegates via the use case; stays Prisma-free and synchronous (200 body from the worker, 500 on failure). Addresses the P1 review on #627. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- prisma-runner: capture resolve stderr/stdout and return it in the error (was stdio:'inherit' → Prisma's real failure, e.g. P3011 for a mistyped migration, went only to CloudWatch; the HTTP client got "exited with code 1"). The point of #626 is HTTP recovery without console access. - Validate migrationName shape (^\d{14}_[a-z0-9_]+$) in router + worker so a value like "--schema" can't be handed to the Prisma CLI as a flag. - Router maps a worker-side 400 (LambdaInvocationError.statusCode===400) to a client 400 instead of a generic 500. - Worker guards dbType !== 'postgresql' → 400 (resolve is postgres-only) instead of dying on a schema-not-found error. - Tests: worker resolve branch (malformed name, non-postgres dbType, error passthrough) + router resolve route registration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rationale lives in the commit history / PR; the code is self-explanatory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olve-route fix(devtools): expose POST /admin/db-migrate/resolve route (fixes #626)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Version
Published prerelease version:
v2.0.0-next.104Changelog
🚀 Enhancement
@friggframework/admin-scripts,@friggframework/core,@friggframework/devtools,@friggframework/eslint-config,@friggframework/prettier-config,@friggframework/schemas,@friggframework/serverless-plugin,@friggframework/test,@friggframework/ui🐛 Bug Fix
@friggframework/core,@friggframework/devtoolsAuthors: 1