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.
…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
|
Introduces a per-app encryption opt-out so apps can declare specific fields
they don't want encrypted on write, while keeping decrypt-on-read for legacy
encrypted data. Touched rows naturally migrate to plain JSON on the next
save — no data migration required.
API: `appDefinition.encryption.disable = { ModelName: ['field.path'] }`
Implementation:
- `encryption-schema-registry`: new `registerEncryptionOptOut`,
`validateOptOut`, `getFieldsToEncryptOnWrite` (respects opt-out),
`getFieldsToDecryptOnRead` (ignores opt-out — preserves legacy reads),
`resetEncryptionOptOut`. `loadCustomEncryptionSchema` now also loads
`appDefinition.encryption.disable`.
- `FieldEncryptionService`: splits write-side and read-side field
resolution. Falls back to `getEncryptedFields` when the new methods
aren't present, preserving backwards compatibility for older schema
adapters.
- `prisma-encryption-extension`: wires the new lookups through to the
service.
Motivation: apps storing non-sensitive data in encrypted fields (e.g.
`IntegrationMapping.mapping`) need to query it directly in Postgres for
features like failure tracking and dashboards. Encrypted blobs block
`WHERE` clauses on jsonb paths. This is per-app — encrypt-by-default
behavior is unchanged for everyone else.
Tests: 15 new for the registry opt-out API; 4 new for the
FieldEncryptionService write/read split + legacy fallback. All existing
encryption-related tests continue to pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e-opt-out feat(core): allow apps to opt out of write-side field encryption
…n 401
Three coordinated changes implement the contract per 401:
- token NOT refreshable → ERROR
- refresh FAILS → ERROR (refreshAuth's own catch)
- refresh SUCCEEDS → no status change; if status was
ERROR, heal to ENABLED
1. Requester: split the 401 handler into three explicit branches.
Drops the `|| refreshCount > 0` clause that speculatively marked
ERROR on any second 401, including transient ones after a successful
refresh and concurrent sibling-requester races.
2. Module: register DLGT_CREDENTIAL_VALIDATED and emit it from
onTokenUpdate() after the refreshed credential is persisted.
Best-effort try/catch — a delegate hiccup must not throw out of
the token-update path since the original 401'd request is about
to be retried.
3. IntegrationBase.receiveNotification: handle CREDENTIAL_VALIDATED
as a narrow self-heal — only flips ERROR → ENABLED, leaves
NEEDS_CONFIG / DISABLED / other intentional states untouched.
Tests (TDD red-green):
- requester.test.js: 4 contract cases (not-refreshable notifies;
refresh-succeeds no notify + retries; refresh-fails no notify
from requester; second-401 after successful refresh no notify)
- oauth-2.test.js: updated consecutive-401 test to assert the new
contract (no notify on second 401)
- module-on-token-update.test.js: 4 cases for CREDENTIAL_VALIDATED
emission, delegate-throw tolerance, no-id no-op
- integration-base-receive-notification.test.js: 5 self-heal cases
(ERROR→ENABLED, ENABLED/NEEDS_CONFIG/DISABLED no-ops, no-id no-op)
187 tests pass across the affected suites. Pre-existing failures
in encryption/Cryptor, webhook-flow.integration, queuer-util, and
adopter-jwt are unrelated (verified by re-running on the stashed
branch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…efresh Follow-up to 134ef28 addressing the Codex review on PR #587. Issue: `refreshCount` was incremented but never reset. After one successful refresh in a Requester's lifetime, any later 401 on the same instance fell through silently — no second refresh attempt, no INVALID_AUTH. If a token got rotated mid-session, the integration stayed ENABLED while every request kept 401-ing. Fix: reset `this.refreshCount = 0` on every successful 2xx response in `_request`. Each request that flows normally restores the per-instance refresh budget, so a later 401 gets a fresh refresh attempt. Preserves the original intent: a 401 immediately after a same-instance refresh (no successful traffic in between) still falls through without speculative INVALID_AUTH — the race-condition guard the previous commit established. Test: requester.test.js gains a case asserting that 401 → refresh → 200 → 401 → refresh → 200 fires refreshAuth twice on a single Requester instance without ever firing INVALID_AUTH. 188 tests pass across the affected suites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n-854c69 fix(core): respect refresh outcome before marking integration ERROR on 401
There was a problem hiding this comment.
SonarCloud found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
UpdateProcessMetrics.execute now accepts a `skipped` field on the
metrics-update object and routes it into the atomic phase as
`results.aggregateData.totalSkipped`. Mirrors the existing handling of
`success` and `errors`: omitted from the increment map when zero, and
counts as atomic work so a skipped-only batch isn't short-circuited
through findById.
Why
---
`processed = totalSynced + totalFailed + totalSkipped` is the identity
the UI relies on to render counts. Before this change, integrations
that called `recordBatchMetrics({ processed: 1, skipped: 1 })` only
incremented `context.processedRecords`; the skipped count was dropped
on the atomic path and at best left a `kind: 'skipped_count'`
breadcrumb in the bounded errorDetails array (capped at 100).
The downstream HubSpot ↔ Clockwork integration was hash-skipping
~574 records per initial sync (legitimate loop-protection on records
that originated in the destination system) and the UI reported
`Skipped: 0`, making the gap between `processed` and `synced`
inexplicable.
Compatibility
-------------
- Pure addition. Existing callers that don't pass `skipped` see no
behaviour change.
- WebSocket broadcasts now also expose `skippedCount` in the progress
payload — additive, no removed fields.
Tests
-----
3 new tests:
- routes `skipped` into the increment map as totalSkipped
- omits totalSkipped from the increment map when skipped is zero
- a skipped-only batch is treated as atomic work (doesn't short-circuit
through findById)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feat(core): increment totalSkipped counter on UpdateProcessMetrics
Defines a three-tier model for what's pluggable in Frigg, distinguishing Core Plugins (required infrastructure with defaults — provider, queue, encryption), Application Extensions (optional cross-cutting features on appDefinition.extensions), and Integration Extensions (off-the-shelf handler bundles on IntegrationBase.Definition.extensions). Grounds the model in existing prototypes: the packages/core/extensions system on claude/frigg-netlify-exploration-aY2Bh for Tier 2, and the this.extensions pattern in the frigg-2.0 downstream repos for Tier 3. Calls out current shape mismatches (array vs object, constructor vs static Definition, vocabulary collision) and proposes resolutions. Co-Authored-By: Claude Opus 4.7 (1M context) <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)
Treat reporting as a sibling of the Admin Script Runner (ADR-005) on shared primitives, replacing PR #607's standalone reporting silo. Reports register via `reports: []` in the app definition (plus `admin.includeBuiltinReports` for core built-ins), extend `ReportBase`, and run through the same runner, admin API key, async/SQS execution, and EventBridge scheduling as admin scripts. Run modes: live (compute inline, persist nothing), recorded (persist an execution record), snapshot (recorded run tagged into a named series). All three use the isolated `AdminScriptExecution` store (type: 'REPORT', no user/ integration FK), so a user-scoped query can never return a report record. Highlights: - ReportBase + built-in `integrations` report (PR #607 aggregation preserved, schemaVersion 1) reading via the admin command bundle, not a repository. - Fold the retired reporting repository triad into the canonical integration / integration-mapping repositories (findAllForReport, countByIntegrationIds); hoist the DocumentDB cursor-drain into documentdb-utils (findManyDrained / aggregateDrained) so deployment-wide scans never truncate at the first batch. - report-commands (type:'REPORT', guarded findExecutionById, findSnapshotSeries), report-runner, report-router (live + async + snapshots + executions + schedule), report-executor-handler, bootstrap wiring with a script/report name-collision guard. - devtools AdminScriptBuilder: dedicated ReportQueue + report executor Lambda, report router, artifact bucket + scheduler resources gated on reports; remove the always-on standalone reporting function. - Artifact storage (S3 + signed URL, private + SSE) for non-JSON report output. - Credential-refresh generic (countActiveByType) reading only a non-secret projection — never decrypts credential secrets. Retire the silo: delete reporting-router, reporting/repositories/*, use-cases/list-integrations-report, handlers/routers/reporting; collapse the dedicated reporting API key into the admin API key. Reviewed across three adversarial passes; confirmed findings fixed (artifact dev-stage selection, DocumentDB truncation, execution-record compensation on enqueue/requeue failure). NOTE: adds @aws-sdk/client-s3 and @aws-sdk/s3-request-presigner to packages/core/package.json — package-lock.json must be regenerated with the repo's canonical npm 10 before merge (do not commit an npm 11 lockfile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Reports (ADR-010) section to the frigg skill covering the reports registry, ReportBase, run modes (live/recorded/snapshot), endpoints, artifact output, and scheduling, with two examples: a snapshot+scheduled JSON report reading via the frigg command bundle, and a recorded CSV report writing to artifact storage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…isolation) Follow-up to ADR-010 reporting, resolving verified code-review findings: - Artifact retrieval: report execution + snapshot reads now mint a signed download URL (report-commands) so stored non-JSON artifacts are reachable via the API; snapshot artifactUrl is a real URL, not the raw storage ref. - Async run validation: recorded/snapshot runs validate mode + inputSchema before persisting/enqueueing, returning 400 up front instead of a 202 that fails later in the worker. - Isolation: admin-script findExecutionById now rejects REPORT rows, symmetric with report findExecutionById rejecting non-REPORT rows. - Report stays protocol-agnostic: integrations-report throws a coded INVALID_INPUT error instead of Boom (no HTTP in the application layer). - Add zip artifact content type; remove orphan findMany import; correct the local artifact adapter comment. - Docs: clarify that a Definition-declared schedule only supplies the default mode; the recurring trigger is activated via PUT /:name/schedule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…unwrap helper Reduce PR-added code comments across the reporting stack so the code speaks for itself (~700 -> ~290 comment lines), keeping only non-obvious "why": invariants, ordering/consistency reasons, and gotchas. Removed route-path JSDoc banners, architecture-tag headers, and param/flow restatements. Comment-only changes; no runtime behavior altered. Also drop the unwrap() helper in integrations-report: read commands directly so the report body reads plainly. A failed read still fails the report (the runner records FAILED), just without unwrap's synthesized message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ns-plan-5b607f feat(core): implement ADR-010 reporting as an admin operation
PR #628 added @aws-sdk/client-s3 and @aws-sdk/s3-request-presigner to packages/core for report artifact storage, but the root package-lock.json was never regenerated. The Release workflow's `npm ci` fails with EUSAGE ("Missing: @aws-sdk/s3-request-presigner ... from lock file"), so no 2.0.0-next.105 has published since #628 merged. Regenerated with npm 10 (npm 11 lockfiles break `npm ci` in this repo's CI). Verified: `npm ci --dry-run` now resolves cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ws-sdk fix: sync package-lock.json with @aws-sdk artifact-storage deps
Migrate and redesign the friggframework.org marketing site under website/, add the on-site "Ask Freya" assistant running on a vendored Freya runtime with a Frigg-domain ontology and roadmap-retrieval tools, apply Frigg brand green, and exclude the vendored bundle from SonarCloud.
✅ Deploy Preview for friggframework-org ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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
5 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