Skip to content

[PM-35128] Refine manifest shape and content#7

Merged
jprusik merged 5 commits into
mainfrom
pm-35128
Apr 15, 2026
Merged

[PM-35128] Refine manifest shape and content#7
jprusik merged 5 commits into
mainfrom
pm-35128

Conversation

@jprusik

@jprusik jprusik commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-35128

📔 Objective

  • The build file hierarchy has been flattened
  • webtests.dev entries have been added to the forms map
  • Some adjustments have been made to the manifest shape to improve consumption ergonomics
 {
   "buildId": "v20260410.1",
   "timestamp": "2026-04-10T13:31:28.913Z",
   "gitSha": "8145b8f7ce91a574d05e2936eeb49b5e6a9f53ab",
   "maps": {
-    "forms": [
-      {
-        "schemaVersion": "1.0.0",
-        "files": [
-          "maps/forms/forms.v1.json",
-          "maps/forms/forms.v1.schema.json"
-        ]
-      }
-    ]
+    "forms": {
+      "v1": {
+        "filename": "forms.v1.json",
+        "cid": "sha256:abcdef...",
+        "schema": "forms.v1.schema.json"
+      }
+    }
   }
 }

@jprusik jprusik self-assigned this Apr 14, 2026
@jprusik jprusik requested a review from MGibson1 April 14, 2026 15:51
@jprusik jprusik added the ai-review Request a Claude code review label Apr 14, 2026
@jprusik jprusik marked this pull request as ready for review April 14, 2026 15:55
@jprusik jprusik requested a review from a team as a code owner April 14, 2026 15:56
@jprusik jprusik requested a review from audreyality April 14, 2026 15:56
@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR restructures the build manifest from an array-based format to a version-keyed object, adds a content hash (cid) field per map entry, flattens the build output directory so all files live at dist/ root, improves local development by detecting the git SHA (with dirty flag) when GITHUB_SHA is not set, and adds two new webtests.dev form entries for login test scenarios. The release workflow glob was updated to match the new flattened output structure. All changes are internally consistent and aligned with the stated objectives.

Code Review Details

No findings. All changes reviewed for security, correctness, breaking changes, and performance concerns.

@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown

Logo
Checkmarx One – Scan Summary & Details4fd9c255-f8db-4ff6-a4c0-1ed467e8eed6

Great job! No new security vulnerabilities introduced in this pull request

Comment thread scripts/build.mjs Outdated
files: [relative(DIST, outDataFile), relative(DIST, outSchemaFile)],
});
manifest.maps[map.name][`v${majorVersion}`] = {
name: basename(outDataFile),

@jprusik jprusik Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the key name here be filename (or file. or something else)? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any of the above are all fine, just need to pick one 🤷

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll stick with name unless a strong opinion emerges.

@audreyality audreyality Apr 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 It's not clear to me what name is naming. I see where it's listed in the output, and it clearly contains a file name. What isn't clear to me is why that's important.

❓ Some interesting questions are:

  • Where is the manifest located relative to the files it catalogues?
  • What is the purpose of the SHA?
  • What is is the purpose of the manifest?
  • What is the purpose of including the major version in the manifest?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What isn't clear to me is why that's important.

The filename allows:

  • Us to change the names of the built resource files in the future
  • Private providers to do the same

The manifest name/location/structure becomes the important part of the consumer contract, not the individual build files files

For the other questions:

  • The manifest lives at the root of the build path. This PR moves the rest of the files into that same root path, to keep relative file expectations consistent with release build path structure (in case private providers want to serve their own builds internally without an extra steps to move files around).
  • The SHA gives consumers the ability to check if the file content has changed (as opposed to a release diff). This allows consumer clients to see if the map for their schema version actually changed before they pay the cost of pulling it down. For example, a data update might only affect schema v2 concerns, and when the map files are built and released, v2 has changes, but v1 does not.
  • The manifest provides information that allows consumers to: understand what maps are available, in what formats, and verify the data they have does not differ from a given release's expectations. This enables consumers to make informed decisions about what map data they want to retrieve as well as how and when they should do so.
  • Keying off map schema major versions allows consumers to "pin" their maps to maintain compatibility in their client. If a map schema has breaking changes, it would constitute a major version bump (and build migration to the previous schema version for future releases). This gives consumers time to update their code to consume new schema builds

(see also: Versioning)

We're still at a stage where much of this is flexible however, so revisiting any of these points before project release is acceptable (and indeed, likely).

@audreyality audreyality Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ It would be useful to have a description of the manifest format that outlines how it works and expectations on its use. In particular, calling out that the SHA is serving a similar purpose to HTTP ETag is an important use-case that should be specified.

⛏️ file or filename is more appropriate than name, since it's intent is to refer to a specific file from the manifest.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file or filename is more appropriate than name, since it's intent is to refer to a specific file from the manifest.

I'm convinced 34219fb

@audreyality audreyality left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My questions are non-blocking.

@jprusik jprusik merged commit 6863020 into main Apr 15, 2026
14 checks passed
@jprusik jprusik deleted the pm-35128 branch April 15, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants