feat: enhance generate:metadata-payload with mode filtering, path targeting, and CI/lint-staged integration#3788
Open
itsarijitray wants to merge 14 commits into
Open
feat: enhance generate:metadata-payload with mode filtering, path targeting, and CI/lint-staged integration#3788itsarijitray wants to merge 14 commits into
itsarijitray wants to merge 14 commits into
Conversation
…ate:metadata-payload Adds a --mode flag to filter metadata generation by destination mode (cloud/device), and discovers unregistered cloud destinations from the filesystem so they can also have their metadata.json generated without being registered in index.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Enhances the CLI generate:metadata-payload command to better support generating metadata.json during development/CI by adding mode-based filtering and discovering cloud destinations that exist on disk but aren’t yet registered in the destination-actions manifest.
Changes:
- Added a
--mode/-mflag to filter generated metadata by destination mode (e.g.,cloud,device). - Added filesystem discovery for unregistered cloud destinations under
packages/destination-actions/src/destinationsand includes them in the generation run. - Updated command examples to document the new flag.
Comment on lines
+312
to
+316
| // Discover unregistered cloud destinations from the filesystem | ||
| const cloudDestDir = path.join(process.cwd(), 'packages', 'destination-actions', 'src', 'destinations') | ||
| const registeredDirs = new Set(Object.values(manifest).map((entry: any) => entry.directory as string)) | ||
|
|
||
| if (await fs.pathExists(cloudDestDir)) { |
Discovers warehouse destinations from packages/warehouse-destinations/src/destinations/ and generates metadata.json for them. Handles the warehouse-specific settings field and resolveSourceDir patterns. Updates --mode flag to accept 'warehouse'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
itsarijitray
force-pushed
the
feat/metadata-payload-cli-enhancements
branch
from
e783ec8 to
1c246dd
Compare
2 tasks
Adds convenience scripts for generating metadata by destination mode: - generate:metadata-payload:cloud - generate:metadata-payload:browser - generate:metadata-payload:warehouse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/cli/src/commands/generate/metadata-payload.ts:400
- When
--modeis provided and nothing matches, the command currently exits successfully after generating 0 files. It would be more consistent with the existing--slugbehavior to throw an error whenfilterModesis set butgenerated === 0, so invalid/unsupported mode values are surfaced in CI.
if (filterModes && filterModes.length > 0) {
const definitionMode = ((definition as any).mode ?? 'cloud') as string
if (!filterModes.includes(definitionMode)) {
skipped++
continue
Comment on lines
+338
to
+343
| if (await fs.pathExists(cloudDestDir)) { | ||
| const dirs = await fs.readdir(cloudDestDir) | ||
| for (const dir of dirs) { | ||
| if (registeredDirs.has(dir)) continue | ||
| const indexPath = path.join(cloudDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue |
…nceConfig details Adds missing fields to generated metadata.json that the bot service needs: - syncMode label/description/choices on actions - hooks inputFields/outputFields - displayMode, additionalProperties on action fields - audienceConfig.supportsAudienceFunctions - multiple/depends_on on auth fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use flags.enum for --mode to validate values and fail fast on typos - Filter registeredDirs to only cloud-manifest entries to avoid false-positive skips when browser/cloud dir names collide - Log skipped destination errors via this.debug() instead of silently swallowing them - Update test mocks to include fs.pathExists and fs.readdir - Add filesystem discovery tests covering: discovering unregistered destinations, skipping invalid folders, and ensuring discovered entries are written correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Deep Code ReviewBug Findings
Test Coverage Gaps
Architecture Notes
🤖 Generated with Claude Code deep review (3 parallel reviewers) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/cli/src/commands/generate/metadata-payload.ts:461
- New --mode filtering logic is added here, but the test suite only exercises --slug filtering. Add command-level tests to verify that --mode=cloud/device/warehouse includes only matching destinations (and that multiple --mode values behave as expected).
if (filterModes && filterModes.length > 0) {
const definitionMode = ((definition as any).mode ?? 'cloud') as string
if (!filterModes.includes(definitionMode)) {
skipped++
continue
}
}
Comment on lines
+389
to
+427
| // Discover unregistered cloud destinations from the filesystem | ||
| const cloudDestDir = path.join(process.cwd(), 'packages', 'destination-actions', 'src', 'destinations') | ||
| const registeredDirs = new Set( | ||
| Object.values(manifest) | ||
| .filter((entry: any) => entry.path?.includes('packages/destination-actions')) | ||
| .map((entry: any) => entry.directory as string) | ||
| ) | ||
|
|
||
| if (await fs.pathExists(cloudDestDir)) { | ||
| const dirs = await fs.readdir(cloudDestDir) | ||
| for (const dir of dirs) { | ||
| if (registeredDirs.has(dir)) continue | ||
| const indexPath = path.join(cloudDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue | ||
| try { | ||
| const definition = await loadDestination(indexPath) | ||
| if (!definition) continue | ||
| manifest[`unregistered:${dir}`] = { | ||
| definition, | ||
| directory: dir, | ||
| path: indexPath | ||
| } | ||
| } catch (err) { | ||
| this.debug(`Skipping unregistered destination "${dir}": ${(err as Error).message}`) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Discover warehouse destinations from the filesystem | ||
| const warehouseDestDir = path.join(process.cwd(), 'packages', 'warehouse-destinations', 'src', 'destinations') | ||
| if (await fs.pathExists(warehouseDestDir)) { | ||
| const dirs = await fs.readdir(warehouseDestDir) | ||
| for (const dir of dirs) { | ||
| if (dir === 'index.ts') continue | ||
| const indexPath = path.join(warehouseDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue | ||
| try { | ||
| const definition = await loadDestination(indexPath) | ||
| if (!definition) continue |
Comment on lines
+57
to
+71
| syncMode: { | ||
| default: string | ||
| label: string | ||
| description: string | ||
| choices: Array<{ label: string; value: string }> | ||
| } | null | ||
| hooks: Record< | ||
| string, | ||
| { | ||
| label: string | ||
| description: string | ||
| inputFields: Record<string, PublicActionField> | ||
| outputFields: Record<string, PublicActionField> | ||
| } | ||
| > | null |
Comment on lines
+323
to
+337
| // Compiled warehouse: .../packages/warehouse-destinations/dist/destinations/<name>/index.js | ||
| const warehouseDistMatch = entryPath.match( | ||
| /^(.+\/packages\/warehouse-destinations)\/dist\/destinations\/([^/]+)\/index\.js$/ | ||
| ) | ||
| if (warehouseDistMatch) { | ||
| return path.join(warehouseDistMatch[1], 'src', 'destinations', warehouseDistMatch[2]) | ||
| } | ||
|
|
||
| // Source warehouse: .../packages/warehouse-destinations/src/destinations/<name>/index.ts | ||
| const warehouseSrcMatch = entryPath.match( | ||
| /^(.+\/packages\/warehouse-destinations\/src\/destinations\/[^/]+)\/index\.ts$/ | ||
| ) | ||
| if (warehouseSrcMatch) { | ||
| return warehouseSrcMatch[1] | ||
| } |
Bug fixes:
- Use readdir({ withFileTypes: true }) to filter directories only,
avoiding wasteful pathExists calls on non-directory entries
- Add fallback for hook.outputTypes → hook.outputFields naming
- Only include full_audience_sync in audienceConfig when mode is
'synced' (proper discriminated union)
- Use path.startsWith check for registeredDirs filtering
Test improvements:
- Add --mode=cloud and --mode=device filtering tests
- Add warehouse resolveSourceDir path tests
- Add displayMode/additionalProperties action field tests
- Add multiple/depends_on auth field tests
- Add test for skipping non-directory entries
- Update discovery test mocks to use Dirent objects
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ta.json Adds targeted metadata regeneration via --path flag so lint-staged can efficiently regenerate only affected destinations' metadata.json on commit. Also adds a CI validate step to assert metadata payloads stay up-to-date. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
itsarijitray
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
packages/cli/src/commands/generate/metadata-payload.ts:515
- Filtering only against
entry.directorymisses merged cloud/browser manifest entries. For example the browser manifest registerspackages/browser-destinations/destinations/amplitude-pluginsunder the same metadata ID as the cloudamplitudedestination, andgetManifest()keeps the cloud entry directory (amplitude) while merging the browser action into it. A--pathfromamplitude-pluginstherefore skips the merged destination and won't regenerate the metadata file that contains that web action.
if (filterDirs && filterDirs.size > 0) {
const entryDir: string = entry.directory?.split('/').pop() ?? ''
if (!filterDirs.has(entryDir)) {
skipped++
continue
packages/cli/src/commands/generate/metadata-payload.ts:515
- The path filter compares only the destination directory basename, so paths from one package can match unrelated destinations in another package that happen to share the same directory name (for example browser
brazeand cloudbraze). This makes--pathover-generate metadata outside the changed destination; the filter needs to preserve package/mode context or match against source paths rather than just basenames.
if (filterDirs && filterDirs.size > 0) {
const entryDir: string = entry.directory?.split('/').pop() ?? ''
if (!filterDirs.has(entryDir)) {
skipped++
continue
Comment on lines
+57
to
+71
| syncMode: { | ||
| default: string | ||
| label: string | ||
| description: string | ||
| choices: Array<{ label: string; value: string }> | ||
| } | null | ||
| hooks: Record< | ||
| string, | ||
| { | ||
| label: string | ||
| description: string | ||
| inputFields: Record<string, PublicActionField> | ||
| outputFields: Record<string, PublicActionField> | ||
| } | ||
| > | null |
Comment on lines
+511
to
+517
| if (filterDirs && filterDirs.size > 0) { | ||
| const entryDir: string = entry.directory?.split('/').pop() ?? '' | ||
| if (!filterDirs.has(entryDir)) { | ||
| skipped++ | ||
| continue | ||
| } | ||
| } |
Comment on lines
+464
to
+485
| // Discover warehouse destinations from the filesystem | ||
| const warehouseDestDir = path.join(process.cwd(), 'packages', 'warehouse-destinations', 'src', 'destinations') | ||
| if (await fs.pathExists(warehouseDestDir)) { | ||
| const entries = await fs.readdir(warehouseDestDir, { withFileTypes: true }) | ||
| for (const entry of entries) { | ||
| if (!entry.isDirectory()) continue | ||
| const dir = entry.name | ||
| const indexPath = path.join(warehouseDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue | ||
| try { | ||
| const definition = await loadDestination(indexPath) | ||
| if (!definition) continue | ||
| manifest[`warehouse:${dir}`] = { | ||
| definition, | ||
| directory: dir, | ||
| path: indexPath | ||
| } | ||
| } catch (err) { | ||
| this.debug(`Skipping warehouse destination "${dir}": ${(err as Error).message}`) | ||
| } | ||
| } | ||
| } |
Comment on lines
+133
to
+134
| - name: Assert metadata payloads are up-to-date | ||
| run: bash scripts/assert-metadata-payload.sh |
3 tasks
…warehouse tests - --path with unrecognized paths now exits early instead of generating all - CI assert script uses git diff (not git status) to ignore untracked files - Added warehouse filesystem discovery tests with --mode filtering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
All Copilot review comments addressedHere's a summary of how each concern was resolved:
|
Browser destinations registered in the manifest have entry paths under node_modules/@segment/analytics-browser-*, which didn't match any resolveSourceDir pattern. Added a lazy package.json lookup map that maps package names back to source directories under browser-destinations/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (5)
packages/cli/src/commands/generate/metadata-payload.ts:552
- The path filter only compares the extracted directory name to
entry.directory, which misses merged cloud+browser destinations. For example, a staged browser file underpackages/browser-destinations/destinations/amplitude-plugins/...extractsamplitude-plugins, butgetManifest()merges that browser action into the cloud manifest entry whose directory isamplitude, so--pathgenerates nothing and leaves the cloud metadata stale.
if (filterDirs && filterDirs.size > 0) {
const entryDir: string = entry.directory?.split('/').pop() ?? ''
if (!filterDirs.has(entryDir)) {
packages/cli/src/commands/generate/metadata-payload.ts:490
- This passes a source
.tsfile toloadDestination, but the normal CLI entrypoint only registerstsconfig-pathsand does not registerts-node;loadDestinationusesrequire()directly. In CI/lint-staged this will fail to load TypeScript source files and silently skip unregistered destinations, so the new discovery path will not generate their metadata.
const definition = await loadDestination(indexPath)
packages/cli/src/commands/generate/metadata-payload.ts:513
- Warehouse discovery has the same source-loading problem: it passes
index.tstoloadDestination, which uses plainrequire()withoutts-noderegistered bybin/run. As a result, the CI command will skip warehouse destinations instead of generating their metadata.
const definition = await loadDestination(indexPath)
packages/cli/src/commands/generate/metadata-payload.ts:552
- The filter collapses paths to a bare destination directory name, so it cannot distinguish packages with the same basename. This repo has overlaps such as cloud
brazeand browserbraze; passing a path for one will also generate and potentially stage metadata for the other, which defeats targeted regeneration and can add unrelated metadata files.
if (filterDirs && filterDirs.size > 0) {
const entryDir: string = entry.directory?.split('/').pop() ?? ''
if (!filterDirs.has(entryDir)) {
packages/cli/src/commands/generate/metadata-payload.ts:544
- Filtering solely on
definition.modedoes not work for destinations whose browser actions are merged into a cloud manifest entry bygetManifest(). Those entries keep the cloud definition mode, so--mode=devicewill skip merged browser actions (for example Amplitude plugins), while--mode=cloudcan still emit metadata containing web actions.
if (filterModes && filterModes.length > 0) {
const definitionMode = ((definition as any).mode ?? 'cloud') as string
if (!filterModes.includes(definitionMode)) {
Comment on lines
+8
to
12
| # Only check already-tracked metadata.json files for modifications (ignore untracked) | ||
| STALE=$(git diff --name-only -- 'packages/destination-actions/src/destinations/*/metadata.json' 'packages/browser-destinations/destinations/*/metadata.json' 'packages/warehouse-destinations/src/destinations/*/metadata.json') | ||
|
|
||
| if [ -n "$STALE" ]; then | ||
| echo "The following metadata files are out of date:" |
Comment on lines
+117
to
+120
| multiple: schema.multiple ?? false, | ||
| choices: normalizeChoices(schema.choices), | ||
| default: schema.default ?? null | ||
| default: schema.default ?? null, | ||
| depends_on: schema.depends_on ?? null |
Comment on lines
+470
to
+471
| // Discover unregistered cloud destinations from the filesystem | ||
| const cloudDestDir = path.join(process.cwd(), 'packages', 'destination-actions', 'src', 'destinations') |
Comment on lines
+162
to
163
| additionalProperties: field.additionalProperties ?? false | ||
| } |
Ensures generated metadata.json always conforms to the repo's prettier config by running prettier --write on all generated files after the generation loop completes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
packages/cli/src/commands/generate/metadata-payload.ts:189
- This changes
hooksfrom the previously emitted string array to an object/null shape without preserving the old list of hook names or versioning the metadata payload. Any consumer that currently checkshooks.includes(...)will fail after regeneration; consider retaining a compatible hook-name list while adding the detailed hook metadata.
const hooks: PublicAction['hooks'] = {}
if (action.hooks) {
for (const [hookKey, hook] of Object.entries(action.hooks as Record<string, any>)) {
if (!hookTypeStrings.includes(hookKey as any)) continue
packages/cli/src/commands/generate/metadata-payload.ts:179
- This changes the serialized
syncModecontract by removing the existingsupportedModesarray and replacing it withchoices. Sincemetadata.jsonis a generated public payload and there is no schema version bump or compatibility field, existing consumers that readsyncMode.supportedModeswill break; keep the old field alongside the new metadata or introduce a versioned format.
syncMode = {
default: action.syncMode.default,
label: action.syncMode.label ?? 'Sync Mode',
description: action.syncMode.description ?? '',
choices: (action.syncMode.choices ?? []).map((c: { value: string; label: string }) => ({
Comment on lines
+8
to
+9
| # Only check already-tracked metadata.json files for modifications (ignore untracked) | ||
| STALE=$(git diff --name-only -- 'packages/destination-actions/src/destinations/*/metadata.json' 'packages/browser-destinations/destinations/*/metadata.json' 'packages/warehouse-destinations/src/destinations/*/metadata.json') |
Comment on lines
+471
to
+472
| // Discover unregistered cloud destinations from the filesystem | ||
| const cloudDestDir = path.join(process.cwd(), 'packages', 'destination-actions', 'src', 'destinations') |
Comment on lines
+511
to
+514
| const indexPath = path.join(warehouseDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue | ||
| try { | ||
| const definition = await loadDestination(indexPath) |
Comment on lines
+488
to
+491
| const indexPath = path.join(cloudDestDir, dir, 'index.ts') | ||
| if (!(await fs.pathExists(indexPath))) continue | ||
| try { | ||
| const definition = await loadDestination(indexPath) |
| label: 'Traits', | ||
| description: 'User traits.', | ||
| type: 'object', | ||
| displayMode: 'key-value', |
Comment on lines
+544
to
+546
| if (filterModes && filterModes.length > 0) { | ||
| const definitionMode = ((definition as any).mode ?? 'cloud') as string | ||
| if (!filterModes.includes(definitionMode)) { |
Comment on lines
+552
to
+554
| if (filterDirs && filterDirs.size > 0) { | ||
| const entryDir: string = entry.directory?.split('/').pop() ?? '' | ||
| if (!filterDirs.has(entryDir)) { |
Comment on lines
+156
to
+157
| readOnly: field.readOnly ?? null, | ||
| hidden: field.unsafe_hidden ?? null, |
Instead of flattening conditional required fields to false, retain the full conditions object so downstream consumers can evaluate them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When merging browser and cloud manifests, copy browser destination settings into the cloud definition's authentication fields so metadata generation captures both. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+8
to
+9
| # Only check already-tracked metadata.json files for modifications (ignore untracked) | ||
| STALE=$(git diff --name-only -- 'packages/destination-actions/src/destinations/*/metadata.json' 'packages/browser-destinations/destinations/*/metadata.json' 'packages/warehouse-destinations/src/destinations/*/metadata.json') |
Comment on lines
+552
to
+554
| if (filterDirs && filterDirs.size > 0) { | ||
| const entryDir: string = entry.directory?.split('/').pop() ?? '' | ||
| if (!filterDirs.has(entryDir)) { |
Comment on lines
+156
to
+157
| readOnly: field.readOnly ?? null, | ||
| hidden: field.unsafe_hidden ?? null, |
| hasPerformBatch: typeof action.performBatch === 'function', | ||
| syncMode, | ||
| hooks, | ||
| hooks: Object.keys(hooks).length > 0 ? hooks : null, |
Browser "plugin" destinations (algolia-plugins, amplitude-plugins, etc.) share metadata IDs with their cloud counterparts and get merged by getManifest(), losing their standalone identity. This adds filesystem scanning to discover them independently and generate their metadata.json. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
Comprehensive enhancements to the
generate:metadata-payloadCLI command:--modeflag (-m): Filter metadata generation by destination mode (cloud, device, warehouse)--pathflag (-p): Generate metadata only for destinations matching given file paths — enables efficient targeted regeneration from lint-stagedindex.ts, so metadata.json can be generated for WIP destinationspackages/warehouse-destinations/metadata.jsonwhen destination source files are modifiedmetadata.jsonis stale (runs full generation and checks for uncommitted diffs)outputTypes ?? outputFieldsfallback for hooks, discriminated union for audienceConfig mode, robusthookTypeStringsfilteringNew files
scripts/update-metadata-payload.sh— called by lint-staged to regenerate affected destinations' metadatascripts/assert-metadata-payload.sh— CI script that asserts no stale metadata.json filesModified files
packages/cli/src/commands/generate/metadata-payload.ts—--mode,--pathflags,extractDestinationDirs(), filesystem discovery.github/workflows/ci.yml— new validate steppackage.json— lint-staged config for metadata regenerationpackages/cli/src/__tests__/generate-metadata-payload.test.ts— 63+ test cases covering all featuresTest plan
./bin/run generate:metadata-payload --mode=cloudgenerates only cloud destinations./bin/run generate:metadata-payload --path packages/destination-actions/src/destinations/amplitudegenerates only amplitude--slugflag still worksupdate-metadata-payload.shand stages metadata.jsonassert-metadata-payload.shchecks cloud, browser, and warehouse metadata staleness🤖 Generated with Claude Code