feat: multi-bucket routing (pkg_bucket_map) and auto_packaging toggle#393
feat: multi-bucket routing (pkg_bucket_map) and auto_packaging toggle#393QuiltSimon wants to merge 5 commits into
Conversation
Two new optional secret parameters, both runtime-configurable in AWS Secrets Manager (no redeploy needed): - pkg_bucket_map: JSON object mapping Benchling folder IDs (lib_...) or project IDs (src_...) to S3 bucket names. Entries route by folder first, then project; unmapped entries fall back to user_bucket. Routing applies to exports, SQS packaging messages, canvas package links, and package-event canvas refreshes. The package-event consumer accepts events from all mapped buckets. - auto_packaging (default "true"): when "false", entry lifecycle events are acknowledged without triggering packaging, and canvas creation renders the canvas without starting an export. Packaging runs only from the explicit "Update Package" canvas button. Both are also settable via the CLI profile config (packages.bucketMap / packages.autoPackaging) and synced by sync-secrets. Co-authored-by: Cursor <cursoragent@cursor.com>
…kager) Co-authored-by: Cursor <cursoragent@cursor.com>
…nt routing) Co-authored-by: Cursor <cursoragent@cursor.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not config.auto_packaging: | ||
| # Auto-packaging disabled: render the canvas (package links + | ||
| # "Update Package" button) but do not enqueue a packaging | ||
| # workflow. Packaging runs only from the explicit | ||
| # "Update Package" button (handled above). | ||
| _render_canvas_without_packaging(payload) | ||
|
|
||
| logger.info( | ||
| "Canvas rendered without packaging (auto-packaging disabled)", | ||
| canvas_id=payload.canvas_id, | ||
| entry_id=payload.entry_id, | ||
| event_type=payload.event_type, | ||
| ) | ||
|
|
||
| return JSONResponse( | ||
| {"status": "ACCEPTED", "message": "Canvas rendered (auto-packaging disabled)"}, | ||
| status_code=202, | ||
| ) |
There was a problem hiding this comment.
auto_packaging=False silently re-renders canvas for unknown button interactions
The if not config.auto_packaging: check is reached not only for v2.canvas.initialized but also whenever a v2.canvas.userInteracted event arrives with an unrecognized button_id (none of the startswith guards return early). When auto_packaging=True that falls through to publish_packaging_request — a pre-existing issue. But with auto_packaging=False, the unknown button now silently re-renders the canvas instead. This may be acceptable as a safe fallback, but it's worth confirming the intent since it means any typo'd or future buttonId value silently re-renders rather than logging "unknown" and returning.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Is re-rendering the canvas the desired behavior for unrecognized button IDs when auto_packaging=False, or should the unknown-button warning still be returned?
There was a problem hiding this comment.
Valid — fixed in 5a8be45. Added a return immediately after the unknown-button warning so the auto_packaging gate (and the publish_packaging_request path above it) is only reachable by canvas lifecycle events (initialized, created, etc.), not by unrecognized button interactions. Also cleans up the pre-existing issue where an unknown button with auto_packaging=True would trigger packaging.
…cess_export Closes the race Greptile flagged: previously execute_workflow resolved the target bucket twice — once inside _process_export (from its own entry fetch for S3 writes) and once at Step 5 (from the Step 1 fetch for the SQS message). If an entry was re-foldered between those two Benchling API calls, S3 files would land in bucket-A while the SQS source_prefix pointed at bucket-B. Now target_bucket is resolved once from the Step 1 entry_data and passed into _process_export as an optional argument, so S3 writes and the SQS registry field are always derived from the same snapshot. Co-authored-by: Cursor <cursoragent@cursor.com>
… through With auto_packaging=False an unknown button_id was silently re-rendering the canvas; with auto_packaging=True it was incorrectly triggering packaging. Neither is the right behavior for an unrecognized button. Return 200/ignored immediately after the warning so the auto_packaging gate is only reached by canvas lifecycle events (initialized, created, etc.). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Superseded by #395. After discussion in the #engineering thread, we agreed that all configurable capability should live in the platform (Benchling App Config Items), not in AWS Secrets Manager. PR #395 adds the Closing this PR. The Secrets Manager routing code ( |
2 participants
Context for reviewers
Sergey and Ernie — you wrote the original benchling-webhook. This PR adds two new optional
BenchlingSecretparameters requested during the leadership call for the Pioneering Medicines multi-bucket workflow. Everything is backward-compatible: existing secrets without the new keys behave exactly as today.What changed and why
pkg_bucket_map— multi-bucket routingWhy: PM needs different experiment types stored in different S3 buckets. The current code hard-codes
user_bucketfor everything.How it works:
BenchlingSecretgains an optionalpkg_bucket_mapkey — a JSON object mapping Benchling folder IDs (lib_...) or project IDs (src_...) to S3 bucket names, e.g.{"lib_abc123": "bucket-a", "src_def456": "bucket-b"}.user_bucketfallback. Logic lives in the new moduledocker/src/bucket_router.py._process_export,input.jsonregistry metadata, SQS packaging messages, canvas package links/QuiltSync URIs/footer, and package-event canvas refreshes. The SQS consumer also accepts events from all mapped buckets, not justuser_bucket.Constraint to be aware of (Sergey): Mapped buckets must already be registered with the Quilt stack — the task role's
BucketWritePolicyand the per-bucket Athena<bucket>_packages-viewonly cover buckets the stack knows about. No new IAM or CloudFormation changes are needed in this PR; that's an operator concern when they configure the map.auto_packaging— disable automatic packagingWhy: PM also wants a mode where entry events don't immediately kick off packaging (they're high-frequency and not all represent a "finished" state). Instead, a lab scientist manually triggers packaging via the canvas "Update Package" button when they're ready.
How it works:
BenchlingSecretgains an optionalauto_packagingkey (string"true"/"false", default"true")."false": entry lifecycle events (v2.entry.created,v2.entry.updated.fields,v2.entry.updated.reviewRecord) returnstatus: skippedwithout touching SQS. Canvas init/creation events render the full canvas (package links, Browse/Update buttons) without starting an export.Architecture decisions you should weigh in on
Ernie — reversibility and blast radius:
auto_packaging=falseis a positive opt-in, not a default-change. No existing deployment is affected unless the operator explicitly adds the key.pkg_bucket_mapvalues that name non-stack buckets fail silently at write time (IAMAccessDeniedon S3 put) rather than at parse time. Worth discussing whether we want a validation step at secret-load time that checks bucket reachability.Sergey — AWS/infra specifics:
BucketWritePolicy(a managed IAM policy attached at deploy time) already covers all stack-registered buckets, so multi-bucket writes work without an IAM change if the buckets are stack-registered.{user_bucket} ∪ {all pkg_bucket_map values}instead of justuser_bucket. The EventBridge pattern itself is unchanged — filtering happens in the consumer at runtime, not at the SQS/EventBridge level.pkg_bucket_mapchange takes effect within ~60s without a restart.Bug found and fixed during review
Greptile flagged (and we fixed before requesting review):
execute_workflowpreviously resolvedtarget_buckettwice from two independent Benchling API calls — once inside_process_exportfor S3 writes, and once at Step 5 for the SQS message. If an entry was re-foldered between those calls, files would land in bucket-A while the SQSsource_prefixpointed at bucket-B. Fixed in the last commit: bucket is now resolved once from the Step 1entry_dataand threaded into_process_exportas a parameter.Files to focus your review on
docker/src/bucket_router.pydocker/src/entry_packager.pyexecute_workflow→_process_export→_send_to_sqsdocker/src/sqs_consumer.pybucketparam threaded torefresh_canvas_for_package_eventdocker/src/secrets_manager.pydocker/src/app.pyauto_packaginggates in_handle_event_impland_handle_canvas_impldocker/src/canvas.pypkg_bucket_mapin__init__; propagates toPackageQueryandPackageFileFetcherTest coverage
Not yet tested (needs a live dev stack)
user_bucket, canvas links point at the right catalog URLauto_packaging=false: entry events skipped, canvas renders with buttons, Update Package still packages