feat(cli): programmatic control API — @prisma/composer/control (TML-3174) - #205
Open
wmadden-electric wants to merge 21 commits into
Open
feat(cli): programmatic control API — @prisma/composer/control (TML-3174)#205wmadden-electric wants to merge 21 commits into
wmadden-electric wants to merge 21 commits into
Conversation
deploymentReport now also writes a JSON DeploymentSummary — the serializable projection of DeploymentResult (address + entities, no in-process node) — to the file named by PRISMA_COMPOSER_DEPLOYMENT_RESULT_FILE when that env var is set. The printed report is unchanged, and nothing is written without the env var, so the generated stack file stays byte-identical. This is the writer half of the cross-process result contract the programmatic deploy operation reads back (TML-3174 design §3.4). Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
deploy() and destroy() are now programmatic operations (operations/): typed inputs, structured results, no argv, no console, no process.exit. The pipeline orchestration (main.ts steps 0-9.75) moved verbatim into execute-deploy-destroy.ts, reached only by dynamic import after a structured effect-resolution preflight (TML-3158) so the operations entry stays import-safe in a broken effect tree. main.ts run() becomes a thin renderer: flag combinations validated with the same CliError texts, the destroy no-state warning rendered from the operation event, alchemy-failure hints and passthrough exit codes unchanged. run.test.ts passes unmodified — the extraction proof. A successful deploy now also reads back the DeploymentSummary the alchemy child writes via PRISMA_COMPOSER_DEPLOYMENT_RESULT_FILE (absent or malformed file = undefined summary, never a failure). Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
dev() returns a DevSession (endpoints, stop(), closed) and reports lifecycle through onEvent — the operation never touches process signal handlers; the CLI adapter keeps the removeAllListeners + single-listener signal ownership and renders each event with the exact console lines it always printed. log() returns the running services plus a merged, address-filtered AsyncIterable of lines, ended by the caller-owned AbortSignal; per-stream failures surface as stream-failed events without ending the other streams. run-dev.ts and run-log.ts become thin parse-shaped adapters over the operations; their suites pass unchanged. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
New `./control` subpath on @internal/cli (generated exports map committed) and @prisma/composer (hand-maintained map, per the published-package exception): the deploy/destroy/dev/log operations, their input/result types, and the DEPLOYMENT_RESULT_FILE_ENV cross-process contract. Depcruise aliases and the architecture per-file entry for the 9-public shim keep every edge visible to the cruiser; lint:deps is green. Named `/control` after the plane the CLI sources already occupy — the shim doc-comment distinguishes it from an extension's ADR-0017 `/control` entry. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ublished tree operations.test.ts drives deploy/destroy/log with the run.test.ts fakes: the result-file round trip (env var passed, stale file removed pre-spawn, malformed file = undefined summary), structured invalid-input/pipeline/execution failures with the exact CLI messages, destroy target discrimination and teardown-before-remove order, the pre-pipeline no-local-deploy-state event, the merged/ filtered/abortable log stream with stream-failed events, and a seeded effect-mismatch tree returning an effect-resolution result. Every operation call runs inside silently(), which fails on any console output. control.deploy.test.ts is the slice done-condition: a consumer outside the CLI imports @prisma/composer/control and runs deploy over the real integration fixture, reaching the same missing-built-entry terminal point the binary test pins — as a structured result. check-npm-effect-resolution.mjs now also probes the adversarial package-manager tree in-process: importing the control surface must not crash, and deploy() must return the effect-resolution failure with exit 0. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
ADR-0043 records the ./control surface and the PRISMA_COMPOSER_DEPLOYMENT_RESULT_FILE cross-process contract (plus the index entry). The deploying guide gains a "Driving deploys from code" section, mirrored tersely into skills/prisma-composer/SKILL.md (user-facing-surface-changes: both in the same PR), and deploy-cli.md § Contracts now names @internal/assemble's second consumer as shipped rather than future. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The endpoint merge and watch setup ran outside any try/catch, so a throw
there (e.g. withEmulatorRetry exhausting its attempts) rejected the dev()
promise instead of producing { outcome: "failed", kind: "pipeline" } as
design § 3.5 requires. The attach try block now extends through the
endpoint merge, watch setup, and session construction; the startServices
rollback is unchanged.
Refs: TML-3174
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
The CLI adapter printed unwatchable notices before the `[dev] logs:` hint; the shipped order was front door, hint, then unwatchable lines. Notices received before the session is returned are now buffered and flushed right after the hint; later ones print immediately. Every string is unchanged. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Summary by CodeRabbit
WalkthroughAdded the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/0-framework/3-tooling/cli/src/dev/run-dev.ts`:
- Around line 70-72: Update the `converge-failed` handler in the dev watch-loop
switch to read the emitted `stackFilePath`, `reproduceCommand`, and `cwd`
payload and print the stack path and reproduce command, matching the initial
converge-failure output. Preserve the existing message that the running app is
untouched and still watching, and use `cwd` when formatting or executing the
reproduction command as established by the startup failure path.
- Around line 109-128: Update runDev around the signal registration and
session.closed wait to use try/finally, removing the finish listener for both
SIGINT and SIGTERM in the finally block after session.closed settles or exits
early. Preserve the existing finish behavior while ensuring each runDev
invocation does not leave stale listeners behind.
In `@packages/0-framework/3-tooling/cli/src/main.ts`:
- Line 229: Guard the rethrow in the OperationFailure handling path the same way
as the existing line-231 logic: only throw failure.cause when it is an Error,
otherwise create or propagate an Error using failure.message so the formatter
always receives an Error. Keep the surrounding failure handling unchanged.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts`:
- Around line 254-257: Update the deployment flow around resultFilePath and
readDeploymentSummary to use a unique per-run result filename, preventing
concurrent deploy() calls sharing or deleting each other’s reports. Wrap the
execution and summary-read lifecycle in a try/finally so the unique result file
is removed on success and every execution failure path, including returns before
readDeploymentSummary.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts`:
- Around line 266-271: Update the executeDev startup error handler to clean up
all partially started resources before returning the failed outcome. Declare a
watchHandle before the try, assign it from startWatch, then in the catch stop
the watcher when present and call stopServices() for every attachment,
preserving the existing failure response afterward.
- Around line 138-172: Update the converge helper to accept a pipeline argument
and use that argument for writeDevStackFile instead of the outer pipeline. Pass
the initial pipeline to the first converge call, and replace the watch-loop’s
duplicated stack-writing and runAlchemy logic with converge(rePipeline),
preserving the existing return and failure handling.
- Around line 206-240: Update the rebuild callback in startWatch to maintain a
single in-flight rebuild promise, preventing overlapping runPipeline,
writeDevStackFile, and deploy operations; queue or skip subsequent triggers
until the current rebuild completes. Track the dev session’s stopping/stopped
state and cancel or skip callbacks that begin after shutdown starts, including
already scheduled work. Before each writeDevStackFile, deploy/converge action,
and onEvent call, recheck that state so no stack writes, deploys, or events
occur after stopping begins.
- Around line 248-262: Update the shutdown logic in stop so host-supplied
onEvent callbacks cannot interrupt cleanup or leave closed unresolved: isolate
both stopping and stopped event calls from thrown exceptions, and place
resolveClosed() in a finally block that always executes after
attachment.stopServices() processing. Preserve the existing stopping guard and
closed return behavior.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-log.ts`:
- Around line 38-56: Bound the shared queue used by the attachment pumps so it
cannot grow without limit when the consumer is slow. Update the queue production
and consumption flow around the attachment `logs` pumps and `notify` to apply
backpressure once the configured capacity is reached, ensuring producers resume
as items are consumed; alternatively, explicitly drop oldest entries and report
each drop through `onEvent`.
- Line 98: Update the local services declaration in the execute-log operation to
use readonly DevEndpoint[] instead of an inline object shape, reusing the
existing DevEndpoint symbol so changes to that type are enforced here.
- Around line 16-24: Remove toCliError and update the execute operation’s inner
catch blocks to rethrow the original error instead of wrapping it, so the outer
failure retains the original value as failure.cause. Keep failureMessage for
deriving the failure message only, preserving the existing message behavior.
- Around line 67-83: Update the async log generator’s cleanup around the
`finally` block to derive an internal abort signal from `input.signal`,
preserving propagation of the caller’s abort state. Abort the internal signal
before awaiting `Promise.all(pumps)` so early termination through `break`,
`return`, or `lines.return()` causes all pumps to exit and cleanup to resolve.
- Around line 113-148: Ensure every attachment created in executeLog is cleaned
up on pipeline failures, empty-service early returns, and invalid input returns;
retain them through normal log consumption. Update the attachment lifecycle
around target.attach and the services validation branches to invoke the existing
cleanup API, or add a close/dispose/detach mechanism (alternatively propagate
input.signal when creating subscriptions) so abandoned LocalTargetAttachment
followers cannot remain active.
In `@packages/0-framework/3-tooling/cli/src/render-deployment.ts`:
- Around line 149-152: Update deploymentReport’s summary-writing block to create
the parent directory for DEPLOYMENT_RESULT_FILE_ENV before writing, using
node:path to derive it, and wrap directory creation and fs.writeFileSync in
error handling that swallows failures. Preserve successful summary serialization
while ensuring report-hook write errors never escape.
In `@skills/prisma-composer/SKILL.md`:
- Around line 693-697: Update the dev and log result documentation in SKILL.md
to describe their outcome discriminants, matching the existing deploy
documentation. Identify the success and failure branches of DevStartResult and
LogResult, and state that session/services/lines are available only for the
successful outcome so callers handle failures before destructuring.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 435a7704-0c0d-4927-9cc6-6cbaeb5474fa
📒 Files selected for processing (26)
architecture.config.jsondocs/design/10-domains/deploy-cli.mddocs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.mddocs/design/90-decisions/README.mddocs/guides/deploying.mdpackages/0-framework/3-tooling/cli/package.jsonpackages/0-framework/3-tooling/cli/src/__tests__/render-deployment.test.tspackages/0-framework/3-tooling/cli/src/dev/run-dev.tspackages/0-framework/3-tooling/cli/src/exports/control.tspackages/0-framework/3-tooling/cli/src/log/run-log.tspackages/0-framework/3-tooling/cli/src/main.tspackages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.tspackages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.tspackages/0-framework/3-tooling/cli/src/operations/execute-dev.tspackages/0-framework/3-tooling/cli/src/operations/execute-log.tspackages/0-framework/3-tooling/cli/src/operations/operations.tspackages/0-framework/3-tooling/cli/src/operations/results.tspackages/0-framework/3-tooling/cli/src/render-deployment.tspackages/0-framework/3-tooling/cli/tsdown.config.tspackages/9-public/composer/package.jsonpackages/9-public/composer/src/exports/control.tspackages/9-public/composer/tsdown.config.tsscripts/check-npm-effect-resolution.mjsskills/prisma-composer/SKILL.mdtest/integration/test/control.deploy.test.tstsconfig.depcruise.json
Comment on lines
+109
to
+128
| const finish = (): void => { | ||
| void session.stop(); | ||
| }; | ||
|
|
||
| // alchemy's own library code (imported transitively while loading the | ||
| // app's config/providers) registers its own process-level SIGINT/SIGTERM | ||
| // listeners for ITS OWN in-process resource bookkeeping — irrelevant | ||
| // here, since the actual converge runs in a separate spawned `alchemy` | ||
| // child process (run-alchemy.ts), never in this one. Left in place, | ||
| // whichever of its listeners runs first can call process.exit() | ||
| // synchronously and tear this process down before the watch loop's own | ||
| // async cleanup (stopping the app's services) ever gets a turn. This is | ||
| // this process's OWN signal handling from here on: strip whatever else | ||
| // is registered and become the only listener. | ||
| process.removeAllListeners('SIGINT'); | ||
| process.removeAllListeners('SIGTERM'); | ||
| process.on('SIGINT', finish); | ||
| process.on('SIGTERM', finish); | ||
|
|
||
| await session.closed; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the signal listeners after the session closes.
runDev registers finish for SIGINT and SIGTERM and never removes it. It also calls removeAllListeners for both signals first, which strips the host's own handlers.
For a one-shot CLI process this is harmless, because the process exits. The PR objective states that these operations enable in-process use by the unified prisma CLI. In a long-lived host, two effects follow. Each runDev call leaves a listener bound to a session that already closed. The host also loses its own signal handling permanently, because removeAllListeners deleted it and nothing restores it.
Remove finish after session.closed settles. Use try/finally so an early return or a throw still cleans up.
🛡️ Proposed fix
process.removeAllListeners('SIGINT');
process.removeAllListeners('SIGTERM');
process.on('SIGINT', finish);
process.on('SIGTERM', finish);
- await session.closed;
- return 0;
+ try {
+ await session.closed;
+ } finally {
+ process.removeListener('SIGINT', finish);
+ process.removeListener('SIGTERM', finish);
+ }
+ return 0;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/dev/run-dev.ts` around lines 109 -
128, Update runDev around the signal registration and session.closed wait to use
try/finally, removing the finish listener for both SIGINT and SIGTERM in the
finally block after session.closed settles or exits early. Preserve the existing
finish behavior while ensuring each runDev invocation does not leave stale
listeners behind.
Comment on lines
+254
to
+257
| // Stale-result guard: remove any previous run's result file so a summary is | ||
| // only ever read from THIS child's report hook. | ||
| const resultFilePath = path.join(cwd, '.prisma-composer', 'deployment-result.json'); | ||
| fs.rmSync(resultFilePath, { force: true }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the deployment-result file unique per run and delete it after the read.
resultFilePath is a single fixed path per cwd. The CLI ran one deploy per process, so this was safe. The control surface is a library API now, and a host can call deploy() twice concurrently against the same cwd. Then the second call's rmSync deletes the first child's file, both children write the same target, and readDeploymentSummary at line 336 can return the other deploy's summary.
The file also survives the read. Deployed resource ids and public URLs stay on disk under the project directory.
Use a unique name per run and remove it in a finally block.
🔧 Proposed fix
+import { randomUUID } from 'node:crypto'; // Stale-result guard: remove any previous run's result file so a summary is
// only ever read from THIS child's report hook.
- const resultFilePath = path.join(cwd, '.prisma-composer', 'deployment-result.json');
- fs.rmSync(resultFilePath, { force: true });
+ // The name is unique per run, so concurrent in-process operations against the
+ // same cwd cannot read or delete each other's summary.
+ const resultFilePath = path.join(
+ cwd,
+ '.prisma-composer',
+ `deployment-result.${randomUUID()}.json`,
+ ); if (action === 'deploy') {
- return { summary: readDeploymentSummary(resultFilePath) };
+ try {
+ return { summary: readDeploymentSummary(resultFilePath) };
+ } finally {
+ fs.rmSync(resultFilePath, { force: true });
+ }
}
+ fs.rmSync(resultFilePath, { force: true });
return {};Both execution failure paths at lines 270-294 also return before the read, so remove the file there as well, or wrap lines 259-338 in a single try/finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts`
around lines 254 - 257, Update the deployment flow around resultFilePath and
readDeploymentSummary to use a unique per-run result filename, preventing
concurrent deploy() calls sharing or deleting each other’s reports. Wrap the
execution and summary-read lifecycle in a try/finally so the unique result file
is removed on success and every execution failure path, including returns before
readDeploymentSummary.
Comment on lines
+138
to
+172
| const reproduceCommand = `alchemy deploy ${DEV_STACK_RELATIVE_PATH} --yes --stage dev`; | ||
|
|
||
| const converge = (): { status: number; stackPath: string } => { | ||
| const stackPath = writeDevStackFile({ | ||
| entryPath: pipeline.entryModule.path, | ||
| cwd, | ||
| configPath: pipeline.configPath, | ||
| name: pipeline.name, | ||
| assembled: pipeline.assembled, | ||
| }); | ||
| const status = (deps?.alchemy ?? runAlchemy)({ | ||
| command: 'deploy', | ||
| stackFileRelativePath: DEV_STACK_RELATIVE_PATH, | ||
| cwd, | ||
| stage: 'dev', | ||
| containerEnv: containerEnv(containers), | ||
| }); | ||
| return { status, stackPath }; | ||
| }; | ||
|
|
||
| // 7. Write the dev stack file and converge. | ||
| const first = converge(); | ||
| if (first.status !== 0) { | ||
| return { | ||
| outcome: 'failed', | ||
| failure: { | ||
| kind: 'execution', | ||
| message: `alchemy deploy exited with status ${first.status}.`, | ||
| exitCode: first.status, | ||
| stackFilePath: first.stackPath, | ||
| reproduceCommand, | ||
| cwd, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract one converge helper parameterized by the pipeline.
converge() at lines 140-156 and the watch-loop body at lines 214-227 repeat the same writeDevStackFile plus runAlchemy call with identical arguments. The only difference is the pipeline object. converge() is also called exactly once. Accept a pipeline argument so both call sites share one implementation.
♻️ Proposed refactor
- const converge = (): { status: number; stackPath: string } => {
+ type ConvergeInput = Awaited<ReturnType<typeof runPipeline>>;
+ const converge = (p: ConvergeInput): { status: number; stackPath: string } => {
const stackPath = writeDevStackFile({
- entryPath: pipeline.entryModule.path,
+ entryPath: p.entryModule.path,
cwd,
- configPath: pipeline.configPath,
- name: pipeline.name,
- assembled: pipeline.assembled,
+ configPath: p.configPath,
+ name: p.name,
+ assembled: p.assembled,
});
const status = (deps?.alchemy ?? runAlchemy)({
command: 'deploy',
stackFileRelativePath: DEV_STACK_RELATIVE_PATH,
cwd,
stage: 'dev',
containerEnv: containerEnv(containers),
});
return { status, stackPath };
};
// 7. Write the dev stack file and converge.
- const first = converge();
+ const first = converge(pipeline);Then the watch loop calls converge(rePipeline).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-dev.ts` around
lines 138 - 172, Update the converge helper to accept a pipeline argument and
use that argument for writeDevStackFile instead of the outer pipeline. Pass the
initial pipeline to the first converge call, and replace the watch-loop’s
duplicated stack-writing and runAlchemy logic with converge(rePipeline),
preserving the existing return and failure handling.
|
|
||
| let name: string; | ||
| const attachments: LocalTargetAttachment[] = []; | ||
| let services: readonly { readonly address: string; readonly url: string }[]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the DevEndpoint declaration.
rg -nP --type=ts -C6 '\b(interface|type)\s+DevEndpoint\b'Repository: prisma/composer
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files matching execute-log/results:"
fd -a '(.*(execute-log|results)\.ts$)' . | sed 's#^\./##'
echo
echo "Search LogResult and services declarations/usages:"
rg -n --type=ts 'LogResult|DevEndpoint|services:' .
echo
echo "Show relevant execute-log slices:"
for f in $(fd 'execute-log\.ts$' .); do
echo "--- $f"
nl -ba "$f" | sed -n '80,120p;140,160p'
done
echo
echo "Show relevant results slices:"
for f in $(fd 'results\.ts$' .); do
echo "--- $f"
nl -ba "$f" | sed -n '150,190p'
doneRepository: prisma/composer
Length of output: 3858
🏁 Script executed:
#!/bin/bash
set -u
echo "--- packages/0-framework/3-tooling/cli/src/operations/results.ts lines 80-185"
awk 'NR>=80 && NR<=185 {printf "%7d %s\n", NR, $0}' packages/0-framework/3-tooling/cli/src/operations/results.ts
echo
echo "--- packages/0-framework/3-tooling/cli/src/operations/execute-log.ts lines 80-165"
awk 'NR>=80 && NR<=165 {printf "%7d %s\n", NR, $0}' packages/0-framework/3-tooling/cli/src/operations/execute-log.ts
echo
echo "--- imports/usages of DevEndpoint"
rg -n --type=ts '\bDevEndpoint\b' packages/0-framework/3-tooling/cli/srcRepository: prisma/composer
Length of output: 8641
Use readonly DevEndpoint[] for local services.
The local shape currently matches DevEndpoint only by having address and url. Type this as readonly DevEndpoint[] so future changes to DevEndpoint fail here instead of drifting past the compile-time contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-log.ts` at line 98,
Update the local services declaration in the execute-log operation to use
readonly DevEndpoint[] instead of an inline object shape, reusing the existing
DevEndpoint symbol so changes to that type are enforced here.
Comment on lines
+113
to
+148
| for (const target of resolved.values()) { | ||
| try { | ||
| const container = await target.container.ensure({ appName: name, stage: undefined }); | ||
| attachments.push(await target.attach({ container, devDir })); | ||
| } catch (error) { | ||
| throw toCliError(error); | ||
| } | ||
| } | ||
|
|
||
| services = (await Promise.all(attachments.map((a) => a.endpoints()))).flat(); | ||
| } catch (error) { | ||
| return { | ||
| outcome: 'failed', | ||
| failure: { kind: 'pipeline', message: failureMessage(error), cause: error }, | ||
| }; | ||
| } | ||
|
|
||
| if (services.length === 0) { | ||
| return { | ||
| outcome: 'attached', | ||
| appName: name, | ||
| services: [], | ||
| lines: mergeLogStreams([], input), | ||
| }; | ||
| } | ||
| if (input.address !== undefined && !services.some((s) => s.address === input.address)) { | ||
| return { | ||
| outcome: 'failed', | ||
| failure: { | ||
| kind: 'invalid-input', | ||
| message: `no service "${input.address}" in "${name}" — running services: ${services | ||
| .map((s) => s.address) | ||
| .join(', ')}.`, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the LocalTargetAttachment contract and any disposal member.
fd -t f 'local-target' -x echo {}
ast-grep run --pattern 'interface LocalTargetAttachment { $$$ }' --lang typescript .
rg -nP --type=ts -C3 '\b(close|dispose|detach|\[Symbol\.asyncDispose\])\s*[:(]' -g '**/local-target/**'Repository: prisma/composer
Length of output: 2403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execute-log outline =="
ast-grep outline packages/0-framework/3-tooling/cli/src/operations/execute-log.ts --view expanded || true
echo "== execute-log relevant lines =="
sed -n '1,190p' packages/0-framework/3-tooling/cli/src/operations/execute-log.ts | cat -n
echo "== LocalTargetAttachment usages =="
rg -n --type=ts -C3 'LocalTargetAttachment|attachments|\\.logs\\(|attach\\(' packages/0-framework/3-tooling/cli/src packages/0-framework/1-core/core/src/control packages/1-prisma-cloud/1-extensions/target/src || true
echo "== LogEndpoint / DevEndpoint definitions =="
rg -n --type=ts -C4 'interface .*DevEndpoint|type .*DevEndpoint|readonly .*DevEndpoint|service: string|AsyncIterable' packages/0-framework packages/1-prisma-cloud || trueRepository: prisma/composer
Length of output: 20796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg 'execute-log|results|operations\.ts|attach|local-target'
echo "== results.ts log result =="
sed -n '135,185p' packages/0-framework/3-tooling/cli/src/operations/results.ts | cat -n
echo "== operations execute-log caller =="
rg -n --type=ts -C8 'executeLog|LogResult|lines:' packages/0-framework/3-tooling/cli/src/operations/operations.ts packages/0-framework/3-tooling/cli/src packages/1-prisma-cloud || true
echo "== local-target export contracts =="
for f in packages/0-framework/1-core/core/src/exports/local-target.ts packages/1-prisma-cloud/1-extensions/target/src/exports/local-target.ts packages/1-prisma-cloud/1-extensions/target/src/local-target-entry.d.ts packages/0-framework/1-core/core/src/control/local-target.ts; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,220p' "$f" | cat -n
fi
done
echo "== attach implementation =="
sed -n '1,260p' packages/1-prisma-cloud/1-extensions/target/src/local-target/attach.ts | cat -n
echo "== target implementation attach export usages =="
rg -n --type=ts -C8 'attach\\(|LocalTargetAttachment|logs\\(' packages/0-framework/1-core/core/src/control packages/1-prisma-cloud/1-extensions/target/src || trueRepository: prisma/composer
Length of output: 44269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== descriptor attach export =="
sed -n '1,220p' packages/1-prisma-cloud/1-extensions/target/src/local-target/descriptor.ts | cat -n
echo "== compute client followLogs implementation =="
rg -n --type=ts -C8 'followLogs|listServices|startApp|stopApp|request|fetch' packages/1-prisma-cloud/0-lowering/dev-emulators/src packages/1-prisma-cloud/1-extensions/target/src.local-target || true
echo "== all attach/localTarget call sites across TypeScript files =="
python3 - <<'PY'
import pathlib, re
paths = [p for p in pathlib.Path('.').rglob('*.ts') if '.git' not in p.parts]
hits = []
for p in paths:
try:
text = p.read_text(encoding='utf-8', errors='ignore')
except UnicodeDecodeError:
continue
for i,line in enumerate(text.splitlines(),1):
if re.search(r'\.attach\s*\(|attach\(\s*\{', line) or re.search(r'\.logs\s*\(|logs\(', line) or 'LocalTargetAttachment' in line:
hits.append((p, i, line.strip()))
for h in hits:
print(f"{h[0]}:{h[1]}: {h[2]}")
PYRepository: prisma/composer
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local-target descriptor attach implementation =="
rg -n --type=ts -C5 'export async function devAttach|attach:' packages/1-prisma-cloud/1-extensions/target/src/local-target/descriptor.ts
echo "== dev-emulators followLogs definitions =="
rg -n --type=ts -C10 'export async function followLogs|async function followLogs|followLogs\\(' packages/1-prisma-cloud/0-lowering/dev-emulators/src packages/1-prisma-cloud/1-extensions/target/src/local-target packages/1-prisma-cloud/1-extensions/target/src/exports || trueRepository: prisma/composer
Length of output: 662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local-target descriptor attach implementation =="
rg -n --type=ts -C5 'export async function devAttach|attach:' packages/1-prisma-cloud/1-extensions/target/src/local-target/descriptor.ts
echo "== dev-emulators followLogs definitions =="
rg -n --type=ts -C10 'export async function followLogs|async function followLogs|followLogs(' packages/1-prisma-cloud/0-lowering/dev-emulators/src packages/1-prisma-cloud/1-extensions/target/src/local-target packages/1-prisma-cloud/1-extensions/target/src/exports || true
echo "== cleanup/dispose/close methods around attachment lifecycle =="
rg -n --type=ts -C3 'close\\(|dispose\\(|detach\\(|Symbol\\.asyncDispose|stopServices\\(|endServices|stopApp\\(' packages/0-framework packages/1-prisma-cloud --glob '!**/dist/**' --glob '!**/node_modules/**' || trueRepository: prisma/composer
Length of output: 943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local-target descriptor attach implementation =="
rg -n -C5 'export async function devAttach|attach:' packages/1-prisma-cloud/1-extensions/target/src/local-target/descriptor.ts || true
echo "== dev-emulators followLogs definitions =="
rg -n -C10 'export async function followLogs|async function followLogs|followLogs\(' packages/1-prisma-cloud/0-lowering/dev-emulators/src packages/1-prisma-cloud/1-extensions/target/src/local-target packages/1-prisma-cloud/1-extensions/target/src/exports || true
echo "== cleanup/dispose/close methods around attachment lifecycle =="
rg -n -C3 'close\(|dispose\(|detach\(|Symbol\.asyncDispose|stopServices\(|endServices|stopApp\(' packages/0-framework packages/1-prisma-cloud --glob '!**/dist/**' --glob '!**/node_modules/**' || trueRepository: prisma/composer
Length of output: 48599
Abort or clean up the attachments returned by an error/early-return path.
attach creates a long-lived log follower for each created LocalTargetAttachment, and executeLog now creates attachments before validating services or input.address. When the function returns pipeline, an empty attached result, or invalid-input, those attachments are never read and no cleanup is applied. If Stop cannot stop an attachment’s specific follower, add close(), dispose(), detach(), or make the attachment subscription pass input.signal from creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-log.ts` around
lines 113 - 148, Ensure every attachment created in executeLog is cleaned up on
pipeline failures, empty-service early returns, and invalid input returns;
retain them through normal log consumption. Update the attachment lifecycle
around target.attach and the services validation branches to invoke the existing
cleanup API, or add a close/dispose/detach mechanism (alternatively propagate
input.signal when creating subscriptions) so abandoned LocalTargetAttachment
followers cannot remain active.
Ground the decision in a usage example, build the narrative up from motivation through import safety and the process-boundary contract, and strip refactor-history framing and ticket references. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.md`:
- Line 5: Update the control subpath contract near the description of the four
operations to distinguish CLI rendering from inherited Alchemy output: state
that operations do not render CLI output themselves but may expose Alchemy
output when configured with stdio: 'inherit'. Keep the existing
structured-input/output and no-argv/process.exit guarantees unchanged.
- Line 44: Update ADR-0043 to explicitly define the programmatic surface’s cwd
contract: deploy(), destroy(), and dev() must use the target app directory,
including when cwd is omitted, before calling checkEffectResolution(cwd). Align
the examples and result-type description with this target-directory behavior,
including where .prisma-composer/ and .alchemy state are located.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efd36683-40fa-44dc-902e-e6c94d398a78
📒 Files selected for processing (1)
docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.md
| The `./control` entry defends against this structurally: | ||
|
|
||
| 1. Its **static import graph contains no alchemy-reachable module** — only types, the resolution checker, and the result definitions. Importing the subpath executes nothing dangerous, even inside a broken tree. An adversarial fixture in `scripts/check-npm-effect-resolution.mjs` pins this against a real package-manager install: importing `@prisma/composer/control` from a tree with a seeded `effect` mismatch must succeed. | ||
| 2. Each operation first runs `checkEffectResolution(cwd)` against the **target app's** directory (not the host's own), and reports a mismatch as a `{ kind: 'effect-resolution' }` failure result. Only after the check passes does it dynamically `import()` the executor that reaches the pipeline and alchemy. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'input\.cwd \?\? process\.cwd\(\)|runEffectPreflight\(cwd\)|checkEffectResolution' \
packages/0-framework/3-tooling/cli/src/operations
rg -n -C 4 \
'entry:|cwd:' \
packages/0-framework/3-tooling/cli/src/operations/__tests__ \
docsRepository: prisma/composer
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- ADR relevant lines ---\n'
sed -n '1,80p' docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.md
printf '\n--- main.ts call sites using operations.ts functions ---\n'
rg -n -C 6 '\b(deploy|destroy|dev|log)\s*\(' packages/0-framework/3-tooling/cli/src/main.ts packages/0-framework/3-tooling/cli/src -g 'packages/0-framework/3-tooling/cli/src/*.ts'
printf '\n--- operations type definitions ---\n'
rg -n -C 8 'interface (DeployInput|DestroyInput|DevInput|LogInput)|type (DeployInput|DestroyInput|DevInput|LogInput)|DeployInput|DestroyInput|DevInput|LogInput' packages/0-framework/3-tooling/cli/src/operations/operations.ts packages/0-framework/3-tooling/cli/src/operations/types.ts packages/0-framework/3-tooling/cli/src/operations/__tests__
printf '\n--- deploy docs explicit input ---\n'
sed -n '290,320p' docs/guides/deploying.md
printf '\n--- check-effect-resolution semantics ---\n'
sed -n '1,140p' packages/0-framework/3-tooling/cli/src/check-effect-resolution.tsRepository: prisma/composer
Length of output: 244
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- ADR relevant lines ---'
sed -n '1,80p' docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.md
printf '%s\n' ''
printf '%s\n' '--- main.ts call sites using operations.ts functions ---'
rg -n -C 6 '\b(deploy|destroy|dev|log)\s*\(' packages/0-framework/3-tooling/cli/src/main.ts packages/0-framework/3-tooling/cli/src -g 'packages/0-framework/3-tooling/cli/src/*.ts' || true
printf '%s\n' ''
printf '%s\n' '--- operations type definitions ---'
rg -n -C 8 'interface (DeployInput|DestroyInput|DevInput|LogInput)|type (DeployInput|DestroyInput|DevInput|LogInput)|DeployInput|DestroyInput|DevInput|LogInput' packages/0-framework/3-tooling/cli/src/operations/operations.ts packages/0-framework/3-tooling/cli/src/operations packages/0-framework/3-tooling/cli/src/operations/__tests__ || true
printf '%s\n' ''
printf '%s\n' '--- deploy docs explicit input ---'
sed -n '290,320p' docs/guides/deploying.md
printf '%s\n' ''
printf '%s\n' '--- check-effect-resolution semantics ---'
sed -n '1,160p' packages/0-framework/3-tooling/cli/src/check-effect-resolution.tsRepository: prisma/composer
Length of output: 50373
🌐 Web query:
Docs: deploy({ entry: 'module.ts', stage: 'feat-auth' }) cwd programmatic control deploy
💡 Result:
The syntax deploy({ entry: 'module.ts', stage: 'feat-auth' }) does not correspond to a single, universally recognized standard across deployment platforms. However, similar programmatic deployment patterns are common in JavaScript/TypeScript-based automation tools and CI/CD libraries. Based on technical documentation and common practices for programmatic control, here is how the specified components are typically handled: 1. Entry Point (entry): In most programmatic interfaces (e.g., Deno Deploy, Catapult, or custom build scripts), the entry parameter defines the main file (e.g., 'module.ts') used to bootstrap the application [1][2][3][4]. 2. Stage/Environment (stage): This parameter is often used to specify target environments (e.g., 'feat-auth' as a branch, environment name, or deployment target) to distinguish between production, preview, or feature-specific builds [3][5][4]. 3. Current Working Directory (cwd) Control: - When executing commands programmatically, the cwd (Current Working Directory) is a standard option used to isolate the process execution [6][7][8]. - If using a tool that supports an options object, cwd is typically passed as a property alongside your other configuration parameters [6][8]. - Example structure: deploy({ entry: 'module.ts', stage: 'feat-auth', cwd: '/path/to/project' }); For specific platforms like Deployer (PHP), the cwd can be passed as an argument to control the working directory for a specific task or command, overriding global defaults [9][10][11]. If you are working within a Node.js/TypeScript environment using a custom deployment class, ensure your deploy function is implemented to accept a cwd string and pass it to the underlying process execution (e.g., child_process.exec or spawn options) [6][7].
Citations:
- 1: https://docs.deno.com/runtime/reference/cli/deploy/
- 2: https://docs.deno.com/deploy/classic/deployctl/
- 3: https://docs.deno.com/deploy/getting_started/
- 4: https://catapultjs.com/guide/programmatic
- 5: https://neon.com/docs/compute/functions/deploy.md
- 6: https://agentcmd.dev/docs/reference/workflow-steps/cli-step
- 7: https://docs.controller.im/advanced-usage/available-tools/execute-command
- 8: https://docs.metaflow.org/api/deployer
- 9: https://github.com/deployphp/deployer/blob/master/docs/api.md
- 10: https://deployer.org/docs/8.x/cli
- 11: https://deployer.org/docs/8.x/UPGRADE
Document the cwd contract for the programmatic surface.
deploy(), destroy(), and dev() use input.cwd ?? process.cwd() before checkEffectResolution(), but the examples show omitting cwd. The result type describes cwd as where .prisma-composer/ and .alchemy state live, while the ADR says the preflight must run against the target app's directory. Make cwd the same target-directory contract in both places, or derive it before preflight.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.md`
at line 44, Update ADR-0043 to explicitly define the programmatic surface’s cwd
contract: deploy(), destroy(), and dev() must use the target app directory,
including when cwd is omitted, before calling checkEffectResolution(cwd). Align
the examples and result-type description with this target-directory behavior,
including where .prisma-composer/ and .alchemy state are located.
wmadden
requested changes
Aug 6, 2026
| let alchemyStage: string; | ||
|
|
||
| try { | ||
| // 1–6. The shared prefix (pipeline.ts): config discovery/load, entry load, |
| readonly summary?: DeploymentSummary | undefined; | ||
| } | ||
|
|
||
| async function executeDeployOrDestroy( |
Collaborator
There was a problem hiding this comment.
This is a bad name. It describes the callers not the function
Collaborator
There was a problem hiding this comment.
I don't love that we're collecting things by type. This file is all the result types of the operations. operations.ts is all the operation functions.
This is a well known bad practice. Why not have one file per operation instead and group logically related things, input types, return types, and the function that uses them, together?
| | { readonly outcome: 'started'; readonly session: DevSession } | ||
| | { readonly outcome: 'failed'; readonly failure: OperationFailure }; | ||
|
|
||
| // ---- log ---- |
Collaborator
There was a problem hiding this comment.
you wouldn't need this stupid ascii art if you grouped the operation and its types together
…catch path The per-operation checkEffectResolution preflight and the dedicated effect-resolution failure kind were oversized for what is now a transient upstream condition. Each operation instead wraps the lazy import of its executor in a try/catch: on a load failure it diagnoses the target tree with checkEffectResolution and returns a pipeline failure carrying the fix-naming message (or the original error message when the tree is healthy), with the import error as cause. The entry stays import-light as a general no-import-side-effects property; bin.ts keeps its own start-up check. Reverts the control-surface probe added to scripts/check-npm-effect-resolution.mjs back to main. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…l property ADR-0043 no longer frames import-safety as an effect-specific contract with its own failure kind: the entry is import-light with lazily loaded executors, and a tree that cannot load the deploy stack surfaces as a structured pipeline failure with a diagnostic message. Update the ADR index line, the deploying guide, and the composer skill to the reduced failure taxonomy. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
operations/results.ts and operations/operations.ts grouped every input and
result type by category, with ascii-art-free-but-monolithic files. Each
operation now owns one module — operations/{deploy,destroy,dev,log}.ts —
holding its input types, result types, and the operation function, each
lazily importing its executor so the control entry stays import-light.
OperationFailure/OperationDeps and the executor-load diagnosis live in
operations/shared.ts.
Also from the round-2 review: executeDeployOrDestroy is renamed
runStackPipeline and its internal return is a proper discriminated union
(succeeded/failed) instead of field-presence encoding; the executors drop
the step-number comments inherited from the deleted main.ts sequence; the
per-extension container maps name their key (ExtensionId); deps seams are
marked @internal test seams; the log deps shape is named once (LogDeps).
Refs: TML-3174
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
- 'unsupported' said too little — it means exactly one thing, the host platform is Windows — so the failure kind is now 'unsupported-platform', renamed before any external consumer can switch on the old literal. - DevEndpoint meant "a running service's address + URL", nothing dev-specific, and log's services were typed with it. It is now ServiceEndpoint, defined once in operations/shared.ts. - DevRunDeps re-declared OperationDeps structurally and LogRunDeps re-declared the log deps shape; both are now aliases, so the shapes cannot drift. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ract The exported execution failure required exitCode/stackFilePath/ reproduceCommand/cwd — all facts about the CURRENT mechanism (a spawned alchemy child driving a generated stack file), frozen into the surface's types. They now live in an optional 'diagnostics' object documented as mechanism-detail with no stability promise; message/cause are the durable fields. The CLI adapters read diagnostics and print exactly what they always printed, and their rethrow is guarded: a non-Error cause becomes a CliError from the failure message instead of a raw throw. The deployment-summary protocol also gets one named home: deployment-summary.ts holds the shape, the env var, the writer (now best-effort — a write failure cannot fail a converged deploy), and the reader validation. render-deployment.ts is presentation-only again; its report hook calls the writer. The env var's value export is gone from ./control — no host has a use for it, both halves reach it by direct import. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Three code paths sat outside every try: writeStackFile and the stale-result rmSync in the deploy/destroy executor, and the first converge (writeDevStackFile + spawn) in the dev executor. A stray .prisma-composer FILE, a read-only or full disk, or a permissions problem made deploy()/dev() reject — breaking the surface headline contract that failures come back as values. All three now map to pipeline failures, pinned by tests that reproduce the review probe (.prisma-composer as a file must yield a failure result and never reach alchemy). Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… after end Four defects in mergeLogStreams, all host-facing: - A consumer that stopped iterating without aborting hung forever: the generator's finally awaited pumps whose sources never end. The merge now runs on an internal AbortController linked to the caller's signal; the finally aborts it and does NOT await the pumps, so break/lines.return() terminate promptly even against a source that ignores the signal. - The queue was unbounded. It is now capped at 10k lines with a drop-oldest policy; the consumer learns how many lines it lost through a new lines-dropped LogEvent member, coalesced per delivery. The CLI adapter ignores it (its console drain never fell behind before either). - Events could fire into torn-down host state after the iterable ended; a done flag plus the abort now silence them. - log skipped the emulator retry dev has, so a transient loopback refusal right after a converge — precisely when the CLI's own hint says to run log — became a hard failure. attach() and endpoints() now retry through the shared withEmulatorRetry, moved to operations/emulator-retry.ts. CliError also accepts ErrorOptions so the executors' toCliError wrappers preserve the original error as cause instead of flattening it to a string. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
- Every event emission in the dev executor goes through a guard: a host onEvent that throws is the host's bug, and it can no longer prevent closed from settling or become an unhandled rejection out of the fire-and-forget watch callback. - A failure between attach and session hand-over (partial startServices, endpoint merge, watch setup) now rolls everything back — watcher stopped, every started service stopped — before the failure result returns. - Watch errors no longer print from inside the operation: startWatch takes an onError callback, the executor maps it to a new watch-error DevEvent, and the CLI adapter prints today's exact '[dev] watch error:' line. That was the last console write reachable from an operation. - stop() no longer swallows teardown errors: a service that refuses to stop becomes a stop-error DevEvent; teardown continues, stopped still fires, closed still settles. The CLI adapter ignores it, matching the shipped behavior where these were silently dropped. - The CLI adapter also renders the converge-failed event's hint fields (stack file path + reproduce command) and removes its SIGINT/SIGTERM listeners once the session closes. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…inputs Per operator ruling: either it's public API or it isn't — we don't ship production code just for tests. DeployInput/DestroyInput/DevInput/LogInput no longer carry deps, and OperationDeps/LogDeps are gone from the ./control shims. Each per-operation module now pairs the clean public function with an in-package *WithDeps variant (deployWithDeps, ...) that takes the seam as a separate parameter; the executors take (input, deps, cwd). The CLI adapters and unit tests thread RunDeps through the WithDeps variants — run.test.ts unchanged — and the published integration consumer (test/integration control.deploy.test.ts) keeps driving the real pipeline with no seam at all. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…nd-trip The result file was a fixed path under cwd, so two deploys sharing a working directory (two stages from one checkout, a host mid-deploy while the CLI runs) silently corrupted each other: run B's stale-guard deleted run A's summary and both children raced one file. The path now carries pid + a UUID, the pre-spawn stale-guard is gone (nothing can be stale on a unique path), and the file is deleted right after the operation reads it. The summary protocol also gains its first covering test through a REAL child process: the injected alchemy spawns a bun child that calls writeDeploymentSummaryFile with the env var the operation set, and the operation reads back exactly what the writer wrote — previously every regression in the pair (env-var drift, report unwired) presented as a normal deploy with summary: undefined. Refs: TML-3174 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… graph
Two ADR-level guarantees had no test:
- DevSession: closed settles only via stop(), stop() is idempotent
(stopping/stopped fire once, both calls resolve), and a dev() run leaves
process.listenerCount('SIGINT'/'SIGTERM') untouched — the operation
never registers signal handlers, so the host can own signals.
- Import-lightness: a fresh bun process imports src/exports/control.ts
with every heavy module poisoned (executors, pipeline, run-alchemy,
stack generators, watch, adapters); if the entry's static graph ever
reaches one, the import throws. Replaces the reverted CI probe with an
in-repo structural check.
Refs: TML-3174
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
…th round 2
- ADR heading softened to what the body defends ('Importing the subpath
executes nothing'), and the body now cites the structural test pinning
the entry's static graph.
- The stdio consequence stops promising a mechanism: the spawned child and
'stdio: inherit' are how composer deploys today, not part of the
contract — matching the demotion of the spawn-shaped failure fields into
the optional diagnostics object, which all three documents now describe.
- The ADR names the accepted structural cost: @internal/cli now contains a
surface that is not a CLI.
- The summary-protocol section points at deployment-summary.ts, the
unique per-run result file, the best-effort writer, and the env var's
removal from the public exports.
- The guide and SKILL document dev/log outcome discriminants
({outcome:'started', session} / {outcome:'attached', ...}), the
lines-dropped and watch-error events, and clean early termination of the
log stream.
Refs: TML-3174
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Contributor
Author
|
Round 2 pushed (12 commits since the last review): a full second-round review (architect + principal-engineer passes, CodeRabbit triage, and @wmadden's comments) drove this batch. @wmadden's review comments:
Contract neutralization (keeping the execution mechanism out of the exported types): spawn-shaped fields ( Probe-confirmed contract breaks fixed: CodeRabbit items: 10 accepted and fixed across Also: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts (1)
217-249: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a per-run generated stack file for concurrent operations.
writeStackFilestill writes only.prisma-composer/alchemy.run.ts, andstackFileRelativePath: GENERATED_STACK_RELATIVE_PATHpasses that fixed path to the Alchemy child. If two deployments/destroys target the samecwd, a later run can overwrite the generated stack before an earlier child reads it, so the earlier command can act on another run’s stack program. Generate a per-run stack path as well, and pass that path asstackFileRelativePath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts` around lines 217 - 249, The deployment flow currently uses the shared GENERATED_STACK_RELATIVE_PATH, allowing concurrent runs to overwrite each other’s stack files. Update the stack-generation call around writeStackFile to create a unique per-run relative stack path, use it when writing the stack, and pass the same path as stackFileRelativePath in the deps.alchemy/runAlchemy invocation; keep result-file isolation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/0-framework/3-tooling/cli/src/dev/run-dev.ts`:
- Around line 54-87: Update the event switch in the onEvent handler to add a
stop-error case that logs event.message to the console with the existing [dev]
prefix, alongside the stopping and stopped cases. Preserve the current
stopped-event output while ensuring service stop failures are surfaced.
In `@packages/0-framework/3-tooling/cli/src/log/run-log.ts`:
- Around line 41-45: Update the onEvent callback to handle events with kind
`lines-dropped` by emitting a console.error warning, alongside the existing
`stream-failed` handling. Include the event’s available message or dropped-line
details in the warning.
In
`@packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts`:
- Around line 1158-1186: Update the test around the async generator in “no event
is delivered after the merged iterable has ended” to assert that failLate is
defined before invoking it, ensuring the late-failure path is actually
exercised. If breaking after the first line prevents the generator from reaching
that assignment, advance the generator once more before breaking while
preserving the test’s expected empty events result.
- Around line 1125-1156: Update the test around logWithDeps to derive TOTAL from
the log module’s exported queue-bound constant, using a value comfortably above
that bound so the 20 ms stall reliably causes drops; preserve the existing
droppedTotal and conservation assertions. Export the bound from the log module
if it is not currently public.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts`:
- Around line 314-323: Ensure every execution path in the operation that creates
resultFilePath removes the result file, including execution failures and
teardown failures, not only the successful deploy path. Reuse the existing
cleanup helper or centralize cleanup in a finally block spanning the result-file
assignment through all returns, while preserving the returned success and
failure values.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-log.ts`:
- Around line 66-68: Update the emit callback in the log streaming operation to
catch and isolate errors thrown by input.onEvent?. Ensure handler failures do
not propagate through pump() or reject the public lines iterable, while
preserving the existing done guard and event delivery behavior.
---
Outside diff comments:
In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts`:
- Around line 217-249: The deployment flow currently uses the shared
GENERATED_STACK_RELATIVE_PATH, allowing concurrent runs to overwrite each
other’s stack files. Update the stack-generation call around writeStackFile to
create a unique per-run relative stack path, use it when writing the stack, and
pass the same path as stackFileRelativePath in the deps.alchemy/runAlchemy
invocation; keep result-file isolation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6608894c-11a7-47a9-a096-52bbee179c57
📒 Files selected for processing (24)
docs/design/90-decisions/ADR-0043-the-control-subpath-is-the-programmatic-deploy-surface.mddocs/design/90-decisions/README.mddocs/guides/deploying.mdpackages/0-framework/3-tooling/cli/src/__tests__/render-deployment.test.tspackages/0-framework/3-tooling/cli/src/cli-error.tspackages/0-framework/3-tooling/cli/src/deployment-summary.tspackages/0-framework/3-tooling/cli/src/dev/run-dev.tspackages/0-framework/3-tooling/cli/src/dev/watch.tspackages/0-framework/3-tooling/cli/src/exports/__tests__/control-import.test.tspackages/0-framework/3-tooling/cli/src/exports/control.tspackages/0-framework/3-tooling/cli/src/log/run-log.tspackages/0-framework/3-tooling/cli/src/main.tspackages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.tspackages/0-framework/3-tooling/cli/src/operations/deploy.tspackages/0-framework/3-tooling/cli/src/operations/destroy.tspackages/0-framework/3-tooling/cli/src/operations/dev.tspackages/0-framework/3-tooling/cli/src/operations/emulator-retry.tspackages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.tspackages/0-framework/3-tooling/cli/src/operations/execute-dev.tspackages/0-framework/3-tooling/cli/src/operations/execute-log.tspackages/0-framework/3-tooling/cli/src/operations/log.tspackages/0-framework/3-tooling/cli/src/operations/shared.tspackages/0-framework/3-tooling/cli/src/render-deployment.tsskills/prisma-composer/SKILL.md
Comment on lines
+54
to
+87
| onEvent: (event) => { | ||
| switch (event.kind) { | ||
| case 'ready': | ||
| printFrontDoor(event.endpoints); | ||
| break; | ||
| case 'unwatchable': { | ||
| const line = `[dev] ${event.address} has no watchable inputs`; | ||
| if (hintPrinted) console.log(line); | ||
| else pendingUnwatchable.push(line); | ||
| break; | ||
| } | ||
| case 'converge-failed': | ||
| console.error('[dev] converge failed — the running app is untouched; still watching.'); | ||
| console.error(`\nGenerated stack file: ${event.stackFilePath}`); | ||
| console.error( | ||
| `Run \`${event.reproduceCommand}\` from ${event.cwd} to reproduce this directly.`, | ||
| ); | ||
| break; | ||
| case 'rebuild-failed': | ||
| console.error(`[dev] rebuild failed: ${event.message}`); | ||
| break; | ||
| case 'watch-error': | ||
| console.error(`[dev] watch error: ${event.message}`); | ||
| break; | ||
| case 'stopping': | ||
| console.log( | ||
| "[dev] stopping — the app's services are stopping; emulators and data stay up.", | ||
| ); | ||
| break; | ||
| case 'stopped': | ||
| console.log('[dev] stopped.'); | ||
| break; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Render the stop-error event.
DevEvent in packages/0-framework/3-tooling/cli/src/operations/dev.ts (lines 17-34) includes { kind: 'stop-error'; message: string }, and execute-dev.ts emits it at line 280 when a service refuses to stop. This switch has no stop-error case. The user then sees only [dev] stopped. while a service process is still running.
Add the case so the failure reaches the console.
🐛 Proposed fix
case 'stopping':
console.log(
"[dev] stopping — the app's services are stopping; emulators and data stay up.",
);
break;
+ case 'stop-error':
+ console.error(`[dev] a service failed to stop: ${event.message}`);
+ break;
case 'stopped':
console.log('[dev] stopped.');
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onEvent: (event) => { | |
| switch (event.kind) { | |
| case 'ready': | |
| printFrontDoor(event.endpoints); | |
| break; | |
| case 'unwatchable': { | |
| const line = `[dev] ${event.address} has no watchable inputs`; | |
| if (hintPrinted) console.log(line); | |
| else pendingUnwatchable.push(line); | |
| break; | |
| } | |
| case 'converge-failed': | |
| console.error('[dev] converge failed — the running app is untouched; still watching.'); | |
| console.error(`\nGenerated stack file: ${event.stackFilePath}`); | |
| console.error( | |
| `Run \`${event.reproduceCommand}\` from ${event.cwd} to reproduce this directly.`, | |
| ); | |
| break; | |
| case 'rebuild-failed': | |
| console.error(`[dev] rebuild failed: ${event.message}`); | |
| break; | |
| case 'watch-error': | |
| console.error(`[dev] watch error: ${event.message}`); | |
| break; | |
| case 'stopping': | |
| console.log( | |
| "[dev] stopping — the app's services are stopping; emulators and data stay up.", | |
| ); | |
| break; | |
| case 'stopped': | |
| console.log('[dev] stopped.'); | |
| break; | |
| } | |
| }, | |
| onEvent: (event) => { | |
| switch (event.kind) { | |
| case 'ready': | |
| printFrontDoor(event.endpoints); | |
| break; | |
| case 'unwatchable': { | |
| const line = `[dev] ${event.address} has no watchable inputs`; | |
| if (hintPrinted) console.log(line); | |
| else pendingUnwatchable.push(line); | |
| break; | |
| } | |
| case 'converge-failed': | |
| console.error('[dev] converge failed — the running app is untouched; still watching.'); | |
| console.error(`\nGenerated stack file: ${event.stackFilePath}`); | |
| console.error( | |
| `Run \`${event.reproduceCommand}\` from ${event.cwd} to reproduce this directly.`, | |
| ); | |
| break; | |
| case 'rebuild-failed': | |
| console.error(`[dev] rebuild failed: ${event.message}`); | |
| break; | |
| case 'watch-error': | |
| console.error(`[dev] watch error: ${event.message}`); | |
| break; | |
| case 'stopping': | |
| console.log( | |
| "[dev] stopping — the app's services are stopping; emulators and data stay up.", | |
| ); | |
| break; | |
| case 'stop-error': | |
| console.error(`[dev] a service failed to stop: ${event.message}`); | |
| break; | |
| case 'stopped': | |
| console.log('[dev] stopped.'); | |
| break; | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/dev/run-dev.ts` around lines 54 - 87,
Update the event switch in the onEvent handler to add a stop-error case that
logs event.message to the console with the existing [dev] prefix, alongside the
stopping and stopped cases. Preserve the current stopped-event output while
ensuring service stop failures are surfaced.
Comment on lines
+41
to
+45
| onEvent: (event) => { | ||
| if (event.kind === 'stream-failed') { | ||
| console.error(`[log] stream failed: ${event.message}`); | ||
| } | ||
| } | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render lines-dropped events in the CLI.
The operation reports dropped log lines through lines-dropped, but this callback ignores that event. If stdout cannot keep up, the CLI loses output without a warning.
Add a console.error branch for lines-dropped.
Proposed fix
onEvent: (event) => {
if (event.kind === 'stream-failed') {
console.error(`[log] stream failed: ${event.message}`);
+ } else {
+ console.error(`[log] dropped ${event.count} lines because output could not keep up.`);
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onEvent: (event) => { | |
| if (event.kind === 'stream-failed') { | |
| console.error(`[log] stream failed: ${event.message}`); | |
| } | |
| } | |
| }), | |
| }, | |
| onEvent: (event) => { | |
| if (event.kind === 'stream-failed') { | |
| console.error(`[log] stream failed: ${event.message}`); | |
| } else { | |
| console.error(`[log] dropped ${event.count} lines because output could not keep up.`); | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/log/run-log.ts` around lines 41 - 45,
Update the onEvent callback to handle events with kind `lines-dropped` by
emitting a console.error warning, alongside the existing `stream-failed`
handling. Include the event’s available message or dropped-line details in the
warning.
Comment on lines
+1125
to
+1156
| test('a consumer that falls behind gets a bounded queue: oldest lines drop, a lines-dropped event says how many', async () => { | ||
| const TOTAL = 10_150; | ||
| const flood = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | ||
| for (let i = 0; i < TOTAL; i += 1) yield { service: 'a', line: String(i) }; | ||
| }); | ||
| const droppedCounts: number[] = []; | ||
|
|
||
| const result = await silently(() => | ||
| logWithDeps( | ||
| { | ||
| entry: 'service.ts', | ||
| onEvent: (event) => { | ||
| if (event.kind === 'lines-dropped') droppedCounts.push(event.count); | ||
| }, | ||
| }, | ||
| { identity: identityFor([flood]) }, | ||
| ), | ||
| ); | ||
|
|
||
| if (result.outcome !== 'attached') throw new Error('expected attached'); | ||
| const seen: LogLine[] = []; | ||
| for await (const line of result.lines) { | ||
| seen.push(line); | ||
| if (seen.length === 1) { | ||
| // Stall once so the pump floods the queue past its bound. | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| } | ||
| } | ||
| const droppedTotal = droppedCounts.reduce((sum, count) => sum + count, 0); | ||
| expect(droppedTotal).toBeGreaterThan(0); | ||
| expect(seen.length + droppedTotal).toBe(TOTAL); | ||
| }, 15_000); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Give the drop assertion more headroom above the queue bound.
TOTAL is 10_150. The bounded queue holds on the order of 10_000 lines. The test therefore needs a single 20 ms stall to let the pump enqueue more than the whole bound plus a 1.5% margin. If the bound is raised, or if the pump ever yields to the event loop per line, droppedTotal becomes 0 and line 1154 fails intermittently in CI.
Export the queue bound from the log module and derive TOTAL from it, or raise TOTAL to a large multiple of the bound. The conservation assertion on line 1155 stays valid either way.
💚 Proposed change to remove the marginal dependency
- const TOTAL = 10_150;
+ // Far above the bounded queue's capacity, so a single stall guarantees drops
+ // regardless of the exact bound.
+ const TOTAL = 50_000;Deriving TOTAL from an exported bound constant is the stronger form, if the log module can export it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('a consumer that falls behind gets a bounded queue: oldest lines drop, a lines-dropped event says how many', async () => { | |
| const TOTAL = 10_150; | |
| const flood = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | |
| for (let i = 0; i < TOTAL; i += 1) yield { service: 'a', line: String(i) }; | |
| }); | |
| const droppedCounts: number[] = []; | |
| const result = await silently(() => | |
| logWithDeps( | |
| { | |
| entry: 'service.ts', | |
| onEvent: (event) => { | |
| if (event.kind === 'lines-dropped') droppedCounts.push(event.count); | |
| }, | |
| }, | |
| { identity: identityFor([flood]) }, | |
| ), | |
| ); | |
| if (result.outcome !== 'attached') throw new Error('expected attached'); | |
| const seen: LogLine[] = []; | |
| for await (const line of result.lines) { | |
| seen.push(line); | |
| if (seen.length === 1) { | |
| // Stall once so the pump floods the queue past its bound. | |
| await new Promise((resolve) => setTimeout(resolve, 20)); | |
| } | |
| } | |
| const droppedTotal = droppedCounts.reduce((sum, count) => sum + count, 0); | |
| expect(droppedTotal).toBeGreaterThan(0); | |
| expect(seen.length + droppedTotal).toBe(TOTAL); | |
| }, 15_000); | |
| test('a consumer that falls behind gets a bounded queue: oldest lines drop, a lines-dropped event says how many', async () => { | |
| // Far above the bounded queue's capacity, so a single stall guarantees drops | |
| // regardless of the exact bound. | |
| const TOTAL = 50_000; | |
| const flood = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | |
| for (let i = 0; i < TOTAL; i += 1) yield { service: 'a', line: String(i) }; | |
| }); | |
| const droppedCounts: number[] = []; | |
| const result = await silently(() => | |
| logWithDeps( | |
| { | |
| entry: 'service.ts', | |
| onEvent: (event) => { | |
| if (event.kind === 'lines-dropped') droppedCounts.push(event.count); | |
| }, | |
| }, | |
| { identity: identityFor([flood]) }, | |
| ), | |
| ); | |
| if (result.outcome !== 'attached') throw new Error('expected attached'); | |
| const seen: LogLine[] = []; | |
| for await (const line of result.lines) { | |
| seen.push(line); | |
| if (seen.length === 1) { | |
| // Stall once so the pump floods the queue past its bound. | |
| await new Promise((resolve) => setTimeout(resolve, 20)); | |
| } | |
| } | |
| const droppedTotal = droppedCounts.reduce((sum, count) => sum + count, 0); | |
| expect(droppedTotal).toBeGreaterThan(0); | |
| expect(seen.length + droppedTotal).toBe(TOTAL); | |
| }, 15_000); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts`
around lines 1125 - 1156, Update the test around logWithDeps to derive TOTAL
from the log module’s exported queue-bound constant, using a value comfortably
above that bound so the 20 ms stall reliably causes drops; preserve the existing
droppedTotal and conservation assertions. Export the bound from the log module
if it is not currently public.
Comment on lines
+1158
to
+1186
| test('no event is delivered after the merged iterable has ended', async () => { | ||
| let failLate: (() => void) | undefined; | ||
| const lateFailer = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | ||
| yield { service: 'a', line: 'one' }; | ||
| await new Promise<void>((_resolve, reject) => { | ||
| failLate = () => reject(new Error('daemon went away late')); | ||
| }); | ||
| }); | ||
| const events: string[] = []; | ||
|
|
||
| const result = await silently(() => | ||
| logWithDeps( | ||
| { | ||
| entry: 'service.ts', | ||
| onEvent: (event) => void events.push(event.kind), | ||
| }, | ||
| { identity: identityFor([lateFailer]) }, | ||
| ), | ||
| ); | ||
|
|
||
| if (result.outcome !== 'attached') throw new Error('expected attached'); | ||
| for await (const line of result.lines) { | ||
| void line; | ||
| break; | ||
| } | ||
| failLate?.(); | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| expect(events).toEqual([]); | ||
| }, 5_000); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pin that the late failure actually fired.
failLate is assigned only after the generator resumes past its first yield. The consumer takes one line and breaks on line 1181, which finalizes the source generator through return(). If the generator never resumes past the yield, failLate stays undefined, line 1183 is a no-op, and line 1185 passes without exercising the late-failure path the test names.
Assert that failLate is defined before calling it.
💚 Proposed change to make the test exercise its named path
failLate?.();
+ expect(failLate).toBeDefined();
await new Promise((resolve) => setTimeout(resolve, 20));
expect(events).toEqual([]);If failLate is genuinely unreachable after break, drive the generator one step further before breaking so the late rejection is observable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('no event is delivered after the merged iterable has ended', async () => { | |
| let failLate: (() => void) | undefined; | |
| const lateFailer = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | |
| yield { service: 'a', line: 'one' }; | |
| await new Promise<void>((_resolve, reject) => { | |
| failLate = () => reject(new Error('daemon went away late')); | |
| }); | |
| }); | |
| const events: string[] = []; | |
| const result = await silently(() => | |
| logWithDeps( | |
| { | |
| entry: 'service.ts', | |
| onEvent: (event) => void events.push(event.kind), | |
| }, | |
| { identity: identityFor([lateFailer]) }, | |
| ), | |
| ); | |
| if (result.outcome !== 'attached') throw new Error('expected attached'); | |
| for await (const line of result.lines) { | |
| void line; | |
| break; | |
| } | |
| failLate?.(); | |
| await new Promise((resolve) => setTimeout(resolve, 20)); | |
| expect(events).toEqual([]); | |
| }, 5_000); | |
| test('no event is delivered after the merged iterable has ended', async () => { | |
| let failLate: (() => void) | undefined; | |
| const lateFailer = fakeAttachment([{ address: 'a', url: 'http://a' }], async function* () { | |
| yield { service: 'a', line: 'one' }; | |
| await new Promise<void>((_resolve, reject) => { | |
| failLate = () => reject(new Error('daemon went away late')); | |
| }); | |
| }); | |
| const events: string[] = []; | |
| const result = await silently(() => | |
| logWithDeps( | |
| { | |
| entry: 'service.ts', | |
| onEvent: (event) => void events.push(event.kind), | |
| }, | |
| { identity: identityFor([lateFailer]) }, | |
| ), | |
| ); | |
| if (result.outcome !== 'attached') throw new Error('expected attached'); | |
| for await (const line of result.lines) { | |
| void line; | |
| break; | |
| } | |
| failLate?.(); | |
| expect(failLate).toBeDefined(); | |
| await new Promise((resolve) => setTimeout(resolve, 20)); | |
| expect(events).toEqual([]); | |
| }, 5_000); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/0-framework/3-tooling/cli/src/operations/__tests__/operations.test.ts`
around lines 1158 - 1186, Update the test around the async generator in “no
event is delivered after the merged iterable has ended” to assert that failLate
is defined before invoking it, ensuring the late-failure path is actually
exercised. If breaking after the first line prevents the generator from reaching
that assignment, advance the generator once more before breaking while
preserving the test’s expected empty events result.
Comment on lines
+314
to
+323
| if (action === 'deploy') { | ||
| const summary = readDeploymentSummary(resultFilePath); | ||
| try { | ||
| fs.rmSync(resultFilePath, { force: true }); | ||
| } catch { | ||
| // Best-effort cleanup — the summary is already in hand. | ||
| } | ||
| return { kind: 'succeeded', summary }; | ||
| } | ||
| return { kind: 'succeeded', summary: undefined }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Delete the result file on the failure paths too.
The cleanup at lines 316-320 runs only after a successful deploy. Both execution failure returns (lines 250-260 and 261-270) and the teardown failure return (lines 307-312) leave the file in place. The alchemy child writes the file through its report hook before a later step fails, so a failed deploy can leave deployed resource ids and public URLs on disk under .prisma-composer/.
The per-run name makes this worse than the previous fixed name. Every failed run now adds another deployment-result-<pid>-<uuid>.json file, and nothing ever removes it.
Wrap the region from the resultFilePath assignment to the returns in a single try/finally that removes the file, or remove it explicitly on each failure return.
🔧 Proposed fix sketch
+function removeResultFile(resultFilePath: string): void {
+ try {
+ fs.rmSync(resultFilePath, { force: true });
+ } catch {
+ // Best-effort cleanup.
+ }
+} if (action === 'deploy') {
const summary = readDeploymentSummary(resultFilePath);
- try {
- fs.rmSync(resultFilePath, { force: true });
- } catch {
- // Best-effort cleanup — the summary is already in hand.
- }
+ removeResultFile(resultFilePath);
return { kind: 'succeeded', summary };
}
+ removeResultFile(resultFilePath);
return { kind: 'succeeded', summary: undefined };Call removeResultFile(resultFilePath) on the failure returns at lines 250-270 and 307-312 as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-deploy-destroy.ts`
around lines 314 - 323, Ensure every execution path in the operation that
creates resultFilePath removes the result file, including execution failures and
teardown failures, not only the successful deploy path. Reuse the existing
cleanup helper or centralize cleanup in a finally block spanning the result-file
assignment through all returns, while preserving the returned success and
failure values.
Comment on lines
+66
to
+68
| const emit = (event: LogEvent): void => { | ||
| if (!done) input.onEvent?.(event); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate errors from input.onEvent.
If input.onEvent throws during a pump's stream-failed event, pump() rejects after its catch block. Because Line 95 discards that promise, this creates an unhandled rejection. If it throws for lines-dropped, the public lines iterable rejects.
Catch callback errors inside emit. Do not let host event handlers stop log streaming or reject an unobserved pump.
Proposed fix
const emit = (event: LogEvent): void => {
- if (!done) input.onEvent?.(event);
+ if (done) return;
+ try {
+ input.onEvent?.(event);
+ } catch {
+ // A host event handler must not stop the log stream.
+ }
};Also applies to: 86-89, 101-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/0-framework/3-tooling/cli/src/operations/execute-log.ts` around
lines 66 - 68, Update the emit callback in the log streaming operation to catch
and isolate errors thrown by input.onEvent?. Ensure handler failures do not
propagate through pump() or reject the public lines iterable, while preserving
the existing done guard and event delivery behavior.
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
2 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.
Adds the typed programmatic deploy surface that composer's docs promised (
@internal/assemble's "second consumer"):deploy/destroy/dev/logoperations with structured inputs and results, published as@prisma/composer/control. Theprisma-composerCLI commands are now thin renderers over these operations — this is the seam the unifiedprismaCLI (consolidate-clis project) will pilot in-process forproject deploy/project dev.Design decisions
src/__tests__/run.test.ts(the 1,089-line behavior pin) is byte-untouched and green; every user-visible string relocated verbatim. The alchemy spawn stays the execution mechanism.effect-resolution,invalid-input,unsupported,pipeline,execution) instead of throwing; the TML-3158 effect preflight becomes a structured failure and runs inside each operation. The published./controlentry is import-safe in a broken effect tree (verified at the built-artifact level).DeploymentSummarywhenPRISMA_COMPOSER_DEPLOYMENT_RESULT_FILEis set; the generated stack file is byte-identical. Recorded in ADR-0043.dev()returns a session handle (endpoints,stop(),closed, events); no executor touches process signal handlers.Changes
packages/0-framework/3-tooling/cli/src/operations/(results, entry functions, deploy/destroy, dev, log executors); CLI adapters re-pointed (main.ts,run-dev.ts,run-log.ts).@internal/cli/control→@prisma/composer/control; depcruise aliases; architecture.config entry.test/integration/test/control.deploy.test.ts) drivingdeploywith zero argv/console/exit-code handling,check-npm-effect-resolution.mjsin-process probe.docs/guides/deploying.md§ Driving deploys from code, SKILL mirror,deploy-cli.md§ Contracts amendment, ADR-0043.Known issues (pre-existing, not this PR)
check-npm-effect-resolutionis red onmain: alchemy beta.67's floating ranges now resolveeffect@4.0.0-beta.104against the repo's beta.103 pin, failing the healthy-shape assertion before this PR's probe runs. Needs a pin bump/constraint in a separate change.dev()'s executor body is covered by one structured-failure test plus the type checker; broader dev-session tests are follow-up work (noted in review).Independently reviewed against the accepted design (verdict: clean with nits; the one should-fix — structured failure for post-attach dev throws — is fixed here, with the shipped output ordering restored).
Refs: TML-3174
🤖 Generated with Claude Code