Modernize testlead Lambda: Node 24, SDK v3, osls deploy (sc-112520)#15
Merged
Conversation
Add mocha/chai/nock tests covering submitlead, submitfeedback, and the lambda handler to lock in current behavior before modernizing dependencies. Export generateLead and fakeByLCName so they can be unit tested directly. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate the S3 config reads in the lambda handler from the end-of-support aws-sdk v2 to the modular @aws-sdk/client-s3 v3 client. Convert the handler to async/await so it awaits both reads, and drop the hardcoded region so the SDK uses AWS_REGION from the Lambda runtime. Update the index test to mock the v3 S3Client.send. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Swap the deprecated request module for the request-capture compat shim, which exposes the same callback API backed by native node:http/node:https. Only the require specifier changes; all existing call sites are unchanged. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Swap the unmaintained faker package for the community @faker-js/faker fork (pinned to 8.4.1, the last CJS-compatible release). Update all generator calls to the v8 namespaces (person/location/string/helpers) and the object form of date.past so no deprecated APIs are hit on the per-minute schedule. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
The only moment usage was a 7-day lookback date formatted as YYYY-MM-DD. Replace it with native Date arithmetic and toISOString().slice(0, 10), dropping the moment dependency entirely. Lambda runs in UTC so the formatted date is unchanged. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Codify the test-sales-and-dev-leads Lambda runtime (nodejs24.x) and its schedule, S3 IAM, and packaging in serverless.yml, modeled on the automated-retries service. The function keeps its existing name so osls takes over the current function rather than creating a duplicate. Add a workflow_dispatch deploy workflow (osls deploy --stage staging) and a test workflow that runs the unit tests and validates that the serverless template packages on every push/PR. Both materialize demoConfig/keys.json (real keys from a secret on deploy, a placeholder for packaging validation) because the bundling plugin statically resolves that require. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Bump the npm publish workflow to Node 24 to match the Lambda runtime, declare
"engines": { "node": ">=24" } in package.json, and mark deploy.sh as deprecated
in favor of the osls deploy workflow.
Refs sc-112520
Co-authored-by: Cursor <cursoragent@cursor.com>
An async handler completes as soon as its promise settles, which on Lambda freezes the execution environment before the callback-based HTTP posts in submitLead/submitFeedback finish. Use .then() chains and keep lambda synchronous so the default callbackWaitsForEmptyEventLoop drains the event loop, matching the original aws-sdk v2 behavior. Poll for side effects in the test since the handler no longer returns a promise. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
faker v4's name.title() produced job titles; the v8 equivalent is person.jobTitle(), not person.prefix() (which yields honorifics like "Mr."). Map the LeadConduit title field to jobTitle to preserve the original semantics. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Promisify submitLead and submitFeedback (wrapping the request-capture callback in a small requestAsync helper) and have demoLeads/demoFeedbacks await every submission via Promise.all. The lambda handler is async again and awaits both the S3 reads and all downstream posts, so the handler promise only settles once the work is complete rather than relying on event-loop draining. Update tests to await the promises instead of polling. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
- Separate the S3 config-load and submission-processing phases in the lambda handler so a failed read is logged distinctly from a failure while processing a successfully fetched config (previously a submission error was mislabeled as an S3 load failure). - Guard verbose response logging in submitLead against a missing response object, which on a request error (err set, response undefined) would throw a TypeError and reject Promise.all, skipping the remaining submissions. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the deploy.sh reference in the README with the osls/GitHub Actions workflow, and spell out the one-time cutover (delete the manual EventBridge rule and existing function) required before the first CloudFormation-managed deploy. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Modernizes the test-sales-and-dev-leads Lambda and its tooling by upgrading to Node 24, moving AWS SDK usage to v3, codifying deployment via Serverless/osls, and adding characterization tests to lock in existing behavior during the async refactor.
Changes:
- Upgrade runtime/tooling to Node 24 and update dependencies (AWS SDK v3, request wrapper, faker, remove moment).
- Add
serverless.yml+ GitHub Actions workflows for CI testing, serverless packaging validation, and manual staging deploy via osls. - Refactor handler + submission flows to
async/awaitand add mocha/chai/nock characterization tests.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/submitlead.test.js |
Adds characterization tests for fake field generation, lead generation, and lead submission behavior. |
test/submitfeedback.test.js |
Adds characterization tests for feedback query/post flow and staging host selection. |
test/index.test.js |
Adds tests for Lambda S3 config reads and basic processing logging. |
serverless.yml |
Codifies Lambda runtime (nodejs24.x), schedule, and S3 read IAM permissions. |
README.md |
Documents new osls-based deployment workflow and one-time cutover steps. |
package.json |
Pins Node engine to >=24, adds test script, modernizes dependencies and adds devDependencies for tests. |
package-lock.json |
Updates lockfile to v3 and captures the new dependency graph. |
lib/submitlead.js |
Replaces deprecated deps, updates faker API usage, and converts submission flow to async/await. |
lib/submitfeedback.js |
Removes moment, converts flow to async/await, and updates HTTP dependency. |
index.js |
Migrates S3 access to AWS SDK v3 and awaits downstream submission work to completion. |
deploy.sh |
Marks legacy deploy script as deprecated with guidance to use the new workflow. |
.gitignore |
Ignores Serverless build output (.serverless). |
.github/workflows/test.yml |
Adds CI for tests and serverless template packaging validation. |
.github/workflows/release.yml |
Updates release workflow Node version to 24. |
.github/workflows/deploy-staging.yml |
Adds manual staging deploy workflow using osls + injected keys secret. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Now that submitLead is async, drop the trailing callback parameter and resolve with the response body on success (undefined when skipped or failed). The CLI consumes the returned body via .then() to optionally open the lead in a browser. Update tests to assert on the resolved value. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
- Guard the status-code checks in submitLead and submitFeedback against a
missing response object (defensive; previously safe only via || short-circuit
on err).
- Format the feedback lookback startDate from local date parts to preserve the
original moment().format('YYYY-MM-DD') local-time behavior on the CLI (the
Lambda runs in UTC regardless).
- Form-url-encode the feedback POST body with URLSearchParams so reasons with
spaces or special characters are sent correctly; add a test that asserts the
encoding round-trips.
- Document why excludeDevDependencies is intentionally false (the
include-dependencies plugin already trims to required deps).
Refs sc-112520
Co-authored-by: Cursor <cursoragent@cursor.com>
Adopt leadconduit-api's ESLint setup, extending semistandard instead of standard, with complexity capped at 10. Apply `eslint --fix` across the repo and hand-fix the non-autofixable findings: - refactor submitlead's fakeByLCName switch to a lookup map - scope `complexity` disables on submitLead/submitFeedback (sequential flows with response guards) - fix a latent implicit-global `fields` assignment in generateLead - replace a top-level `return` in testfeedback with process.exit(3) - drop dead `let url, fields, data, browser` in testlead Enforce `npm run lint` in CI (test job). Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the bundled, gitignored demoConfig/keys.json with a configurable
resolver (lib/demokeys.js): DEMO_KEYS env (inline JSON) -> local file ->
SSM SecureString, with a "log in to AWS" fallback that prints actionable
guidance on a credentials error. Behavior is tunable via DEMO_KEYS,
DEMO_KEYS_FILE, DEMO_KEYS_SSM_PARAM, DEMO_KEYS_SOURCE and
DEMO_KEYS_DISABLE_SSM. The @aws-sdk/client-ssm dep is lazy-required so the
env/file paths never load it.
- index.js: demoFeedbacks awaits getDemoKeys()
- manualdemo.js: await both runners and surface resolver errors cleanly
- index.test.js: guard feedback processing so tests never hit live SSM
serverless.yml injects DEMO_KEYS as ${env:DEMO_KEYS, ssm:/test-sales-and-dev-leads/DEMO_KEYS, '{}'}.
The env fallback (over the plan's bare ssm form) lets `serverless package`
run credential-free, since osls errors on invalid/missing creds rather than
falling back on auth errors; a real deploy leaves DEMO_KEYS unset so the
value comes from SSM. Precedence mirrors lib/demokeys.js.
Refs sc-112520
Co-authored-by: Cursor <cursoragent@cursor.com>
Switch deploy-staging.yml from static AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY
secrets to GitHub OIDC role assumption (aws-actions/configure-aws-credentials
assuming a dedicated least-privilege testlead-deployer role), and remove the
demoConfig/keys.json write step now that keys come from SSM. NPM_TOKEN remains
the only secret the workflow needs.
test.yml: drop the placeholder keys.json step (index.js no longer requires the
file) and set DEMO_KEYS='{}' so `serverless package` resolves credential-free.
Refs sc-112520
Co-authored-by: Cursor <cursoragent@cursor.com>
Delete the retired deploy.sh (it bundled demoConfig/keys.json and could not set the SSM-injected DEMO_KEYS env var). Rewrite the README keys/deployment sections to cover the SSM SecureString (Doppler-sourced) keys, the getDemoKeys() local resolution precedence and config knobs, OIDC deploy with no AWS key secrets, the local osls break-glass path, and a correct keys.json example shape (account name -> API key). Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
CodeQL flagged clear-text logging of sensitive information: the verbose "query options" log stringified the request options, which include auth.pass (the LeadConduit API key). Redact auth.pass before logging and add a regression test asserting the key never appears in verbose output. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- index.js: skip feedback (log) when no API key maps to the account instead of firing an unauthenticated request that would just 401/403; add a regression test. - submitlead.js: log the resolved `body` in verbose mode rather than `response.body` (the request-capture shim doesn't set response.body, so verbose output was misleading). - testfeedback.js: read commander's camelCase feedbackType/feedbackReason (the snake_case reads were always undefined, so the conversion default reason and unknown-type guard never fired) and handle the submitFeedback promise. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
The previous redaction overwrote auth.pass after spreading options, which is safe at runtime but still trips CodeQL's js/clear-text-logging: its taint analysis follows apiKey through `...options.auth` into the logged object and doesn't model the override. Destructure auth out and rebuild it from only the non-sensitive user field so the key never enters the logged value. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
submitLead returns a promise but the CLI only attached .then(), so a rejection (e.g. a synchronous throw from the request library) would surface as an unhandled rejection. Add a .catch() that logs and exits non-zero, matching the testfeedback CLI. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Per DevOps guidance, stop injecting the demo API keys into the Lambda
environment (readable by anyone with function-view access) and drop SSM
Parameter Store. The Lambda now fetches the keys from AWS Secrets Manager at
runtime via its own function role; only the non-sensitive secret id is exposed
as an env var. Doppler remains the source of truth via a Doppler -> Secrets
Manager sync (project leadconduit-lambdas).
- package.json: swap @aws-sdk/client-ssm for @aws-sdk/client-secrets-manager.
- lib/demokeys.js: replace fromSsm() with fromSecretsManager() (GetSecretValue,
parse SecretString as the account -> API key JSON map). Rename knobs to
DEMO_KEYS_SECRET_ID / DEMO_KEYS_DISABLE_AWS and DEMO_KEYS_SOURCE=secretsmanager.
Precedence is unchanged (env -> file -> AWS); the deployed Lambda has neither
env nor file, so Secrets Manager is the operative runtime source.
- serverless.yml: drop the SSM-injected DEMO_KEYS env var; add the
DEMO_KEYS_SECRET_ID env var and a scoped secretsmanager:GetSecretValue IAM
statement on the secret ARN.
- workflows: packaging needs no deploy-time variable resolution now, so drop
DEMO_KEYS='{}' from the validate job; fix the deploy-role comment.
- tests: mock SecretsManagerClient instead of SSMClient; rename the disable knob.
Refs sc-112520
Co-authored-by: Cursor <cursoragent@cursor.com>
Rewrite the README keys, local-resolution, and deployment sections to describe the runtime Secrets Manager fetch (Doppler -> SM sync, project leadconduit-lambdas), the new resolution precedence and config knobs (DEMO_KEYS_SECRET_ID, DEMO_KEYS_SOURCE=env|file|secretsmanager, DEMO_KEYS_DISABLE_AWS), and the function role's secretsmanager:GetSecretValue permission. The secret is never placed in the Lambda environment. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Serverless does not honor .gitignore, so a break-glass local `osls deploy` from a checkout that still has demoConfig/keys.json would bundle those keys into the Lambda zip. With the demo keys now resolved at runtime (env -> file -> Secrets Manager) and no longer masked by a deploy-injected DEMO_KEYS env var, a bundled file would take precedence over Secrets Manager and leak stale keys. Exclude demoConfig/** from packaging so the deployed function always reads Secrets Manager. Verified the artifact omits keys.json even when the file exists locally. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Sanitize every error/guidance string in lib/demokeys.js so no process.env.DEMO_KEYS* read (secret id, file path, source selector) or raw JSON.parse detail can reach a log/throw sink. This closes a narrow real leak (the parse error could echo a fragment of a malformed DEMO_KEYS value) and severs the taint CodeQL tracks into the index.js and manualdemo.js log sinks, resolving the three js/clear-text-logging alerts on PR #15. Add a regression test asserting a malformed DEMO_KEYS value is never echoed in the thrown error. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
Guard the two JSON.parse calls in submitFeedback so a 200/201 with a non-JSON body (gateway error page, truncated response) logs and skips that account instead of rejecting the promise and aborting the rest of the per-minute feedback batch. The post-success parse is also guarded so an unexpected body never marks an already-accepted (201) feedback as a failed run. Also fail with a clear message when a Secrets Manager secret has no SecretString payload (e.g. stored as binary), rather than a misleading "not valid JSON" error from parsing undefined. Add tests covering both non-JSON feedback response paths. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lib/testlead.js:35
commander.program.arguments()should describe only the arguments (not include the command name). Includingtestleadhere makes the generated help usage read liketestlead testlead <posting-url> ..., which is misleading.
program
.arguments('testlead <posting-url> [fields...]')
.option('-d, --data <parameters>', 'Defined data to send as-is. Provide in curl -d style ("a=42&foo=bar")', '')
.option('-p, --probability <percentage>', 'Probability % of sending anything', 100)
testfeedback declared positional args (testfeedback <api_key> <recipient_id>) that it never reads - the required inputs come via -a/--apiKey and -i/--recipientId - and repeated the command name, so --help implied an unsupported positional invocation. Drop the .arguments() line so usage matches the documented options. testlead's .arguments() likewise duplicated the command name; trim it to '<posting-url> [fields...]' (its positionals are real and still consumed in .action). Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
DevOps provisioned the demo keys as a Doppler -> Secrets Manager single-secret sync (leadconduit-lambdas/staging_testlead) rather than the placeholder name/shape the PR assumed. Update the secret id to leadconduit-lambdas-staging-testlead-doppler in serverless.yml (env + IAM ARN), the getDemoKeys default, tests, and README. Single-secret sync stores the whole config as one JSON object with each value serialized as a string, so the account -> API key map arrives nested (and double-encoded) under a DEMO_KEYS key alongside DOPPLER_* metadata. fromSecretsManager() now unwraps and parses that DEMO_KEYS entry, falling back to treating the whole object as the bare map so a hand-created secret or test stub still works. Add a test for the Doppler-wrapped payload. Refs sc-112520 Co-authored-by: Cursor <cursoragent@cursor.com>
johnrb2
marked this pull request as ready for review
July 15, 2026 16:00
Code review — Josh Evensen — 2026-07-15Engineer's assessment Jeremy's analysis Reviewed high-risk files: No blockers. Secret handling is solid — redaction in verbose logs, lazy-required Secrets Manager SDK, static (non-interpolated) error/guidance strings, and the deploy artifact excludes Two notes (non-blocking):
Verdict: Approved with comments High-risk files reviewed:
Files skipped (low risk):
|
joshevensen-ap
approved these changes
Jul 15, 2026
joshevensen-ap
left a comment
joshevensen-ap
left a comment
There was a problem hiding this comment.
Node 24 / SDK v3 / OIDC-deploy modernization — clean async refactor with solid secret-handling and good characterization test coverage. Two non-blocking notes left inline (S3 region, request-capture smoke test).
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
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.
Description of the change
Brings the
test-sales-and-dev-leadsLambda up to date (it was flagged as a Node 20 straggler) and modernizes the surrounding tooling, dependencies, secret handling, and deploy authentication.Runtime & deployment
nodejs20.xtonodejs24.xand pinsengines.node >= 24.serverless.ymland aworkflow_dispatchGitHub Action (deploy-staging.yml) that deploys viaosls, codifying the runtime, S3 read permission,secretsmanager:GetSecretValueon the demo-keys secret, every-minute schedule, and theDEMO_KEYS_SECRET_IDenv var (the Secrets Manager secret id only; the keys are fetched at runtime).test.yml) that lints, runs the test suite, and validates the serverless template (serverless package).release.ymlto Node 24.Dependency modernization
aws-sdkv2 →@aws-sdk/client-s3v3.request→@activeprospect/request-capture/request(callback API wrapped in a smallrequestAsyncpromise helper).faker→@faker-js/faker(pinned to 8.4.1, the last CJS-compatible release) with the corresponding API renames.momentremoved in favor of nativeDate.Behavior / refactor
async/await. The handler awaits both the S3 reads and every downstream post (viaPromise.all), so its promise only settles once all work is complete — correct for an async Lambda handler, which freezes the execution environment as soon as the promise resolves.try/catchblocks so a failed read is reported distinctly from a processing failure.Linting (ESLint)
semistandard(semicolons required) withcomplexitycapped at 10. Applieseslint --fixacross the repo and enforcesnpm run lintin CI. Hand-fixes include refactoringfakeByLCNameto a lookup map, fixing a latent implicit-globalfieldsassignment, and replacing a top-levelreturnintestfeedback.Demo keys → Secrets Manager (Doppler-sourced, runtime fetch)
demoConfig/keys.jsonwithlib/demokeys.jsgetDemoKeys(), resolving in precedence order:DEMO_KEYSenv (inline JSON) → local file → AWS Secrets Manager, with a "log in to AWS" fallback that prints actionable guidance on a credentials error. Tunable viaDEMO_KEYS,DEMO_KEYS_FILE,DEMO_KEYS_SECRET_ID,DEMO_KEYS_SOURCE,DEMO_KEYS_DISABLE_AWS.secretsmanager:GetSecretValue); the secret is never placed in the Lambda environment (only the non-sensitiveDEMO_KEYS_SECRET_ID). Doppler is the source of truth via a Doppler → Secrets Manager sync (projectleadconduit-lambdas).@aws-sdk/client-secrets-manageris lazy-required so the env/file paths never load it, anddemoConfig/**is excluded from the deploy artifact so a stray localkeys.jsoncan't shadow Secrets Manager.serverless.ymlno longer resolves any deploy-time variable, soserverless packageruns credential-free in CI and a real deploy carries no secret material.Deploy auth → GitHub OIDC
deploy-staging.ymlnow assumes a dedicated least-privilegetestlead-deployerrole via GitHub OIDC (aws-actions/configure-aws-credentials), removing the staticAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYsecrets and theTESTLEAD_DEMO_KEYSkeys.json write step.NPM_TOKENis the only secret the workflow needs.Cleanup
deploy.sh. README documents the Secrets Manager / Doppler keys model, localgetDemoKeys()resolution, OIDC deploy, and the localosls deploybreak-glass path.Tests
submitlead,submitfeedback, and theindexhandler, plustest/demokeys.test.jsfor the resolver (env/file/Secrets Manager/auth-failure/forcing). 47 passing.Type of change
Related tickets
sc-112520
Deployment note — prerequisites (DevOps)
This PR is gated on DevOps provisioning (tracked in sc-112598):
NPM_TOKENGitHub Actions secret — required by CI (npm ciof the private@activeprospect/request-capture). CI stays red until this is set.leadconduit-lambdas/staging/test-sales-and-dev-leads(SecretString = account name → API key JSON), Doppler-sourced via a Doppler → Secrets Manager sync, in the staging account (371005981288,us-east-1). If the sync encrypts with a customer-managed KMS key, the function role also needskms:Decrypton that key. The secret id is overridable via theDEMO_KEYS_SECRET_IDenv var inserverless.yml.testlead-deployerOIDC role + trust policy in the staging account (371005981288), with permission to create the function role'ssecretsmanager:GetSecretValuestatement.Deployment note — one-time cutover
The existing function and its every-minute EventBridge rule (
test-sales-and-staging-leads, single target = this Lambda) were created outside CloudFormation, so the firstosls deploycannot adopt them. Before the first deploy, in the LeadConduit staging account (us-east-1):test-sales-and-staging-leads.test-sales-and-dev-leadsfunction.Residual risks (accepted)
@aws-sdk/client-secrets-manageris traced into the Lambda bundle by the include-dependencies plugin. Unlike the previous SSM client, it is actually used at runtime, so this is expected; the only cost is bundle size.GetSecretValueon every invocation (once per minute, ~43k/month) with no in-process caching — negligible against Secrets Manager cost/throttle limits.workflow_dispatch-only, so merging is safe; CI (lint/test) is independent onceNPM_TOKENis set.Checklists
Development and Testing
Code Review
Tracking
QA
Made with Cursor