Repository navigation
Design: nodes declare an id, a deploy finds its Project by logicalId only, and asks before deleting data - #417
wmadden-electric wants to merge 3 commits into
Conversation
…nsent Two Drive projects that replace #355. deployment-identity: a deploy finds its Project by logicalId only, refuses to create one without --create, Console creates the Project at connect, and a platform script backfills logicalId on old Projects. node-identity-and-consent: written for handoff. Every node gets a required logicalId and an optional name, data-losing deploys use the ORM's consent model, and the platform scopes CI tokens to one Project. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
✅ Gizmo reviewed 4e4a43f — posted 1 inline comment(s) this pass. Open findings: 🟠 1 major · 🟡 7 minor Change walkthroughThis delta rewrites the node-identity-and-consent project documentation for handoff: the previous three-document arrangement ( How it fits the codebase. The design is anchored to the repo's recorded decisions: the four ADRs it amends or supersedes (ADR-0006, ADR-0024, ADR-0050, ADR-0057) all exist at the linked paths, the consent rules defer to the ORM's CLI Style Guide, and the Consistency across the project docs. The spec's anchor links into |
Pre-merge checks:
|
| - [ ] Releasing a setup ("Remove setup") or closing the setup PR unmerged deletes the Project the connect created, and only that one. | ||
| - [ ] Every linked Project in production has a `logicalId`; the backfill reports zero remaining. | ||
| - [ ] `prisma deploy --create` creates a Project from a laptop; without the flag the same deploy refuses. | ||
| - [ ] The two Composer decision records named above are amended. The deploying guide and the core-concepts skill say that a deploy finds its Project by `logicalId` and needs `--create` to make one. The guide's `prisma deploy module.ts --name demo-42` example still works, with `--create` on its first run. |
There was a problem hiding this comment.
🟠 Major · consistency — DoD amends the superseded ADR-0051 and never names ADR-0057, the live record for Project resolution
"The two Composer decision records named above" are ADR-0024 and ADR-0051 ("a node's address is its logical ID on the platform", referenced at line 26 and in the references at line 61). ADR-0057 supersedes ADR-0051, and it is ADR-0057's Consequences that record the behavior this project deletes: "container resolution falls back to matching a Project by display name … a compatibility path" (and ADR-0057 also rejects "Match on display names"). If slice 3 amends only the records named here, the repository's binding decision record keeps prescribing the fallback the new resolver no longer implements. The same omission repeats in plan.md slice 3 ("the decision records on stage resolution and node addresses amended") and in the node-identity-and-consent spec and plan.
Recommended fix
| - [ ] The two Composer decision records named above are amended. The deploying guide and the core-concepts skill say that a deploy finds its Project by `logicalId` and needs `--create` to make one. The guide's `prisma deploy module.ts --name demo-42` example still works, with `--create` on its first run. | |
| - [ ] The two Composer decision records named above are amended, together with ADR-0057 ("a node's address is its full path from the app name"), which supersedes ADR-0051 and is the live record that carries the display-name fallback. The deploying guide and the core-concepts skill say that a deploy finds its Project by `logicalId` and needs `--create` to make one. The guide's `prisma deploy module.ts --name demo-42` example still works, with `--create` on its first run. |
|
|
||
| ## The model | ||
|
|
||
| The Project's `logicalId` is the app name (ADR-0051). Container resolution matches on it and on nothing else. If no Project matches, the deploy stops unless `--create` was passed. The platform creates the Project when a repo is connected, so a connected repo's CI run always finds its Project and never needs `--create`. |
There was a problem hiding this comment.
🟡 Minor · consistency — Model statement cites ADR-0051, the superseded record, instead of ADR-0057
ADR-0051 was superseded by ADR-0057, which is the live record for "the address is the node's logicalId" (for the root, the address is the app name, so the claim here still holds). Citing only the superseded ADR — here, and in the references lists of both projects' specs — sends the reader to a dead record. The same stale citation appears in .drive/projects/deployment-identity/spec.md (references) and .drive/projects/node-identity-and-consent/spec.md and its design-notes D2.
Recommended fix
| The Project's `logicalId` is the app name (ADR-0051). Container resolution matches on it and on nothing else. If no Project matches, the deploy stops unless `--create` was passed. The platform creates the Project when a repo is connected, so a connected repo's CI run always finds its Project and never needs `--create`. | |
| The Project's `logicalId` is the app name (ADR-0051, superseded by ADR-0057, which keeps this: the root's address is the app name, and the address is the node's `logicalId`). |
| const auth = provision(authModule, { logicalId: 'auth', secrets: { signingKey: envSecret('AUTH_SIGNING_SECRET') } }); | ||
| provision(web, { logicalId: 'web', name: 'Web', deps: { db: catalog, auth: auth.rpc } }); | ||
| provision(api, { logicalId: 'api', deps: { db: catalog } }); |
There was a problem hiding this comment.
🟡 Minor · consistency — Walkthrough's proposed API puts logicalId/name in provision() options, contradicting settled decision D1
The example passes logicalId: 'web', logicalId: 'api' and logicalId: 'auth' inside the provision() options object, but design-notes D1 settles that "logicalId lives on the node definition (postgres({ logicalId }), module({ logicalId, name }, …)), not on the provision() call" — the provision-time override stays id, for provisioning the same node twice or a packaged module. The glossary likewise defines the provision-time override as id. A handoff implementer reading this asset as the worked example could build the wrong constructor surface.
Recommended fix
| const auth = provision(authModule, { logicalId: 'auth', secrets: { signingKey: envSecret('AUTH_SIGNING_SECRET') } }); | |
| provision(web, { logicalId: 'web', name: 'Web', deps: { db: catalog, auth: auth.rpc } }); | |
| provision(api, { logicalId: 'api', deps: { db: catalog } }); | |
| const auth = provision(authModule, { secrets: { signingKey: envSecret('AUTH_SIGNING_SECRET') } }); | |
| provision(web({ logicalId: 'web', name: 'Web' }), { deps: { db: catalog, auth: auth.rpc } }); | |
| provision(api({ logicalId: 'api' }), { deps: { db: catalog } }); |
|
|
||
| ``` | ||
| Project "billing" (proj_…) exists and holds a production deploy from | ||
| another source. Continue? (--yes) |
There was a problem hiding this comment.
🟡 Minor · consistency — Takeover prompt in 5b offers (--yes), which the design's consent model forbids
The scenario shows Continue? (--yes) for deploying into a Project that holds another source's production deploy. D8 and the spec settle that --yes never consents to a data-losing action — the consent flags are per-subject --delete <subject> / --rename <old>:<new>. Setting logicalId: 'billing' over store is exactly a root re-identification, so the consistent answer mechanism is --rename store:billing. The surrounding text hedges ("Whether Composer asks, or just logs, is a separate decision"), but the illustrated mechanism contradicts the consent model the design hands off.
Recommended fix
| another source. Continue? (--yes) | |
| another source. Continue? (--rename store:billing) |
|
|
||
| | # | Scenario | Expected | Covered by | | ||
| | --- | --- | --- | --- | | ||
| | B1 | Author changes the root `name` from "Store" to "Storefront". | Console label changes. Same Project, same stages, same data. | D1, D3 | |
There was a problem hiding this comment.
🟡 Minor · consistency — Scenarios use the retired word "stage" for what ADR-0059 renames to "environment"
B1 says "same stages" and D5 (line 57) "User destroys a stage, or production". ADR-0059 replaces "stage" with "environment" on every Composer surface (guides, errors, agent skill), and these handoff docs seed exactly those surfaces. The same slip appears in the walkthrough's §2 table ("overwrites its production stage") and in node-identity design-notes ("The hash differs per stage"). Quoting ADR-0024's title is fine; describing behavior is not.
Recommended fix
| | B1 | Author changes the root `name` from "Store" to "Storefront". | Console label changes. Same Project, same stages, same data. | D1, D3 | | |
| | B1 | Author changes the root `name` from "Store" to "Storefront". | Console label changes. Same Project, same environments, same data. | D1, D3 | |
| 2. A6: migration rule for existing single-string modules. | ||
| 3. A7: backfill of `logicalId` on old Projects, or a one-time Console edit. | ||
| 4. C3 and B5: plan output for identity changes below the root, and whether it needs consent. | ||
| 5. E3: the fate of `--name`. |
There was a problem hiding this comment.
🟡 Minor · consistency — E3 and open item 5 still list the fate of --name as unsettled, but the design settles it
Row E3 (line 66) marks --name as "Open" and item 5 here repeats "the fate of --name" as an open item, but the settled design removes it: design-notes "Smaller consequences" ("--name on prisma deploy goes away"), the spec's phase 2 ("--name on prisma deploy goes away") and plan phase 2 ("--name is removed from prisma deploy"). A reader reconciling the asset against the spec gets two different answers.
Recommended fix
Delete open item 5 and change the E3 row's "Open" to the settled answer: --name is removed in phase 2 and re-identifying the root is --rename.
| ### D10. Backfill `logicalId` on old Projects once | ||
|
|
||
| A platform script sets `logicalId` from the link's reserved module name on | ||
| every linked Project that has none. A Project with neither is set by hand in |
There was a problem hiding this comment.
🟡 Minor · correctness — D10 claims leftover Projects are "set by hand in Console", which deployment-identity's spec defers as an open question
D2's safe-removal story assumes D10 covers old Projects, but D10 only backfills linked Projects from the link's reserved module name. For the remaining population — Projects old enough to predate logicalId that also have no repo link — D10 says they are "set by hand in Console", yet the deployment-identity spec's open questions (line 55) defer "whether Console should also allow editing a Project's logicalId by hand for Projects with no link" to the follow-on project "if it comes up". Both cannot hold: either the Console capability exists (then the spec's open question is already answered) or it does not (then this population loses display-name resolution with no remediation, and --create would build them a second, empty Project). The cross-cutting requirement "every repo connected through Console today keeps deploying" is unaffected — linked Projects are backfilled — but the docs disagree about the unlinked tail.
Recommended fix
Reconcile the two documents: either state in D10 that Console editing of a Project's logicalId ships with the backfill slice, or scope D10 to linked Projects and record that unlinked Projects without a logicalId have no automatic remediation and their deploys stop with the --create message.
…nd environments Cite the decision record that makes a node's address its full path from the app name, which superseded the one the designs cited and is the record that keeps the display-name fallback. Use full-path addresses and the word environment in examples. Record how the scenarios' open items were settled, and put logicalId on the node definition in the worked example. Add the open question this raises: on a child, the value an author writes is one segment of the logicalId. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
There was a problem hiding this comment.
New findings: none · trace
Still open from previous reviews: 🟠 1 major · 🟡 6 minor
Replace the design notes, walkthrough and scenario list with a single design document for the team: the decision first, then the design in full, the requirements it meets, the records it changes, delivery, open questions and alternatives. Nodes now declare an id, the last segment of their address. Align both projects' specs and plans. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
There was a problem hiding this comment.
New findings: 🟡 1 minor · trace
Still open from previous reviews: 🟠 1 major · 🟡 6 minor
| export default appRoot('shop', ({ provision }) => { | ||
| const catalog = provision(postgres({ name: 'catalog', contract, config })); | ||
| provision(web, { deps: { db: catalog } }); // web.ts: compute({ name: 'web', … }) | ||
| }); | ||
|
|
||
| // After: identity comes from `id`, and says so. | ||
| export default appRoot({ id: 'shop' }, ({ provision }) => { |
There was a problem hiding this comment.
🟡 Minor · consistency — Design examples use an appRoot constructor that does not exist; the root constructor is module
The "Today" example (line 15), the "After" example (line 21), the address table (line 29) and the "Declaring a node" section (line 51, which lists appRoot and module as separate constructors) all use appRoot, but no such constructor exists anywhere in packages or docs — grep -rn appRoot packages docs returns nothing. The root module is declared module('auth-example', …) in examples/auth, ADR-0006 names the root a module, and the PR description itself uses module('store', …) for today's syntax. In a document written for handoff, presenting a nonexistent constructor as both the current and the proposed API will send part 2's implementers looking for an API that isn't there, or have them invent one.
Recommended fix
Use module in the examples (export default module('shop', ({ provision }) => { … }) today, module({ id: 'shop' }, …) after), drop appRoot from the line-51 list and the address table — or, if a dedicated root constructor is genuinely intended, state its introduction explicitly in "The design" and in part 2 of the delivery order.
Today a cosmetic-looking rename of a Composer app's root can deploy into another repository's Project:
The root's
nameis the app's identity, and Composer falls back to matching Projects by display name, so the next push from repo A reconciles into repo B's production environment. The same defect one level down deletes data: renaming a database'snamefromcatalogtocataloguedeletes the database and creates an empty one.This pull request records the design that fixes both, as two Drive projects. The design document for the team is
.drive/projects/node-identity-and-consent/design.md. It replaces #355, which checked the GitHub repository id inside Composer.The decision
id, the last segment of its address.postgres({ id: 'catalog' })insideappRoot({ id: 'shop' })has the address and platformlogicalIdshop.catalog, the address format that already ships.nameno longer gives identity.logicalIdonly, and creates one only with--create. The display-name fallback goes. Console creates the Project when a repository is connected, and a one-time script gives older connected Projects alogicalId.--delete shop.catalog, or--rename shop.catalog:shop.catalogueto keep the data under the new address.--yesnever answers, and CI fails closed.prisma deploy --nameis removed.What is in this pull request
.drive/projects/node-identity-and-consent/design.md: the whole design, the requirements it meets, the decision records it will change, the delivery order, open questions and alternatives..drive/projects/deployment-identity/: spec, plan and notes for part 1 (find bylogicalIdonly,--create, Console creates the Project, the backfill), which is ready to execute..drive/projects/node-identity-and-consent/spec.mdandplan.md: the contract and plan for parts 2 to 4.Nothing under
docs/design/changes yet; the decision records are amended when each part lands.Open question for review
Whether a node keeps
nameas a Console label besideid. The document recommends removing it, so every platform name stays the node's address.Alternatives considered
--name. Hides the identity from the repository, so a local and a CI deploy of the same file can reach different Projects.logicalIdfield. The platformlogicalIdis the full address, so the same word would mean two different strings.movedblocks). The ORM rejected this: the code keeps describing a system that no longer exists.Agent: brunhilde-44
🤖 Generated with Claude Code