Full update opencode big pickle

[Jun Aishima (JunAishima)]

Update to similar ways of doing things as the https://github.com/NSLS2/nsls2-workflows-template repo, such as standardizing Tiled API key handling, adding Slack messaging, and adding .env file for Prefect and Tiled config parameters.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect TES processing, credential exposure, notifications, and container configuration.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates UCAL workflows to centralize Tiled access, add Slack notifications, and configure container runtime settings.

Changes:

• Adds shared Tiled helpers, API-key handling, and validation.
• Adds Slack notification and dry-run support.
• Updates Prefect mounts, Docker configuration, environment settings, and ignore rules.

File summaries

File	Summary
process_tes.py	Uses shared Tiled helpers for TES processing.
prefect.yaml	Adds the worker application mount.
export_tools.py	Removes legacy Tiled initialization.
end_of_run_workflow.py	Adds notifications, dry-run handling, and API-key parameters.
end_of_run_export.py	Uses shared run retrieval.
Dockerfile	Updates Pixi configuration and copies .env.
data_validation.py	Adds Tiled access and stream-validation tasks.
.gitignore	Refreshes Python ignore patterns.
.env	Defines Prefect and Tiled settings.

Review details

Suppressed comments (1)

Dockerfile:10

• Copying .env into the image does not load it. No code calls load_dotenv on this file (the only call reads /srv/container.secret), and Docker does not source copied files, so PREFECT_LOGGING_EXTRA_LOGGERS and TILED_RETRY_ATTEMPTS are absent from the container. Set these as image/runtime environment variables or explicitly load .env before the affected libraries initialize.

COPY .env .

• Files reviewed: 8/9 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved issues affect Slack notifications, API-key propagation, beamline selection, and environment loading.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

end_of_run_export.py:34

• export_all_streams still accepts beamline_acronym, and general_data_export still forwards it, but this lookup ignores it because get_run hard-codes ucal/raw. Non-ucal callers therefore export data from the wrong catalog; propagate the beamline or remove the unused parameter.

Dockerfile:10

• Copying .env into the image does not load it into the process environment. The only dotenv call in the changed code parses /srv/container.secret, so PREFECT_LOGGING_EXTRA_LOGGERS and TILED_RETRY_ATTEMPTS from this file are ignored unless they are separately injected by the deployment.

COPY .env .

end_of_run_workflow.py:88

• The slack decorator is defined but never applied, so none of the new Slack notifications run and failures/successes are not reported. Apply it to this flow (with @flow outermost, e.g. @flow followed by @slack) so the deployment still receives a Prefect flow.

@flow
def end_of_run_workflow(stop_doc, api_key=None, dry_run=False, reprocess_tes=False):

end_of_run_workflow.py:103

• The same explicit api_key is also dropped before export. general_data_export and export_all_streams call get_run without it, so an invocation with a supplied key can validate one Tiled client but export through the secret-file fallback; propagate the key through the export path.

        exit_status = stop_doc.get("exit_status", "No Status")
        if exit_status == "success":
            general_data_export(uid)

end_of_run_workflow.py:100

• The flow accepts an explicit api_key and uses it for validation and lookup, but this call drops it. process_tes consequently falls back to /srv/container.secret, so callers that provide a key can use different credentials or fail during TES processing; thread the key through this path.

    if not dry_run:
        process_tes(uid, reprocess=reprocess_tes)
        # Here is where exporters could be added

end_of_run_workflow.py:103

• The top-level api_key is used for validation and the initial lookup, but it is not passed to either process_tes or general_data_export. Supplying a key therefore makes only part of the workflow use it; those downstream calls fall back to /srv/container.secret and can fail or access a different Tiled account. Thread the key through both downstream flow APIs and their Tiled tasks.

    if not dry_run:
        process_tes(uid, reprocess=reprocess_tes)
        # Here is where exporters could be added
        exit_status = stop_doc.get("exit_status", "No Status")
        if exit_status == "success":
            general_data_export(uid)

end_of_run_workflow.py:45

• The failure handler starts after the Tiled lookup, so an authentication/network/UID error from get_run escapes without any Slack failure notification. Include this lookup in the guarded path and use a fallback when scan metadata cannot be fetched, so the notifier still reports the original failure.

        run = get_run(uid, api_key=api_key)
        scan_id = run.start["scan_id"]

process_tes.py:32

• The flow accepts an explicit api_key and passes it through validation, but this nested flow calls get_catalog() and get_run() without that key, forcing both tasks to fall back to /srv/container.secret. A caller supplying a valid key can therefore still fail during TES processing when that mounted secret is unavailable or differs; add an api_key parameter to process_tes and propagate it to both tasks and this call.

    catalog = get_catalog()
    run = get_run(uid)

• Files reviewed: 8/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved export signature mismatches and API-key/error-handling issues block safe approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

end_of_run_export.py:34

• get_run is defined with only (uid, api_key=None), so every invocation of this task raises TypeError: got an unexpected keyword argument 'dry_run' before any export can run. Remove this unsupported argument; dry-run handling already belongs to the export operations below.

    run = get_run(uid, api_key=api_key, dry_run=dry_run)

end_of_run_export.py:47

• The new dry-run branches skip the XDI/HDF5 writes, but create_export_path(base_export_path) above still creates a directory before reaching them. Consequently dry_run=True mutates the proposal filesystem; guard base-directory creation as well if dry-run is intended to perform no writes.

    if not dry_run:
        create_export_path(xdi_export_path)
        exportToXDI(xdi_export_path, run)
    else:
        logger.info(f"dry_run: not exporting to {xdi_export_path}")

end_of_run_workflow.py:63

• traceback.format_exception_only is called with only the exception instance; the supported Python runtimes require the exception type and value, so this error handler raises another TypeError and suppresses the original failure/Slack notifications. Pass both type(e) and e.

            tb = traceback.format_exception_only(e)

end_of_run_workflow.py:45

• This Tiled lookup is outside the try that emits failure notifications. If authentication, connectivity, or UID lookup fails, the flow raises before any mon-prefect/mon-prefect-ucal failure message is sent, which defeats the new failure-alerting path for a common failure mode. Include this setup in the guarded error-handling path (while preserving the scan ID fallback).

        run = get_run(uid, api_key=api_key)
        scan_id = run.start["scan_id"]

process_tes.py:32

• process_tes still exposes beamline_acronym, but these calls now use get_catalog/get_run, whose catalog path is hard-coded to ucal; passing any other beamline silently processes the wrong catalog. Either propagate the beamline through the data-access helpers or remove this parameter from the public flow.

    catalog = get_catalog()
    run = get_run(uid)

process_tes.py:32

• The parent flow accepts an explicit api_key, but TES processing now calls both Tiled helpers without it; they therefore fall back to /srv/container.secret. A caller using an explicit key, or a worker without that mounted secret, can pass validation/export setup and then fail here. Add api_key to process_tes and pass it to both helper calls, then forward it from the parent flow.

    catalog = get_catalog()
    run = get_run(uid)

• Files reviewed: 8/9 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved issues affect beamline selection, failure handling, and credential safety.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 4

Open (5)

• general_data_export still accepts only (uid, beamline_acronym), so this call always raises…
• Uninitialized scan_id masks get_run failures · New
• Explicit API key not forwarded to catalog lookup · New
• The flow accepts an explicit api_key and passes it to validation/export, but TES processing drops…
• beamline_acronym remains a public parameter, but this replacement no longer uses it:…

Resolved since last review (2)

• Adding api_key and dry_run before the existing beamline_acronym parameter breaks the…
• The flow accepts and forwards api_key for validation and the initial lookup, but this path drops…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues affect configuration, beamline selection, credentials, notifications, API compatibility, and dry-run behavior.

Review effort: Lite
Findings: 1 · 1

Open (2)

• general_data_export still accepts only (uid, beamline_acronym), so this call always raises…
• beamline_acronym remains a public parameter, but this replacement no longer uses it:…

Resolved since last review (3)

• Explicit API key not forwarded to catalog lookup
• Uninitialized scan_id masks get_run failures
• The flow accepts an explicit api_key and passes it to validation/export, but TES processing drops…

Previously missed (1)

In code that hasn't changed since last review

Slack notification failures incorrectly fail workflow

end_of_run_workflow.py:60

This success notification is inside the same try block as the workflow. If Slack is unavailable after processing/export succeeds, notify raises, the handler sends failure notifications, and the flow is marked failed; a failure-notification error can also mask the original exception. Treat Slack sends as isolated best-effort operations so notification outages cannot change workflow status or hide the underlying error.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Moderate issues remain in flow credential handling and dry-run behavior.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Slack alert failures mask the original workflow exception

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve API-key persistence exposure and ensure dry runs do not create processing artifacts.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the undeclared python-dotenv dependency and prevent Tiled API keys from being persisted as flow parameters.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

Four unresolved moderate findings must be addressed before approval.

Review effort: Lite
Findings: None

[Jun Aishima (JunAishima)]

fixes #5

[Abby Giles (AbbyGi)]

Looks good. Simple comment removal and a general comment

[Abby Giles (AbbyGi)]

I think I like how all these variables are set up at the top. Maybe we can include this in the template and generalize SLACK_UCAL to something like SLACK_BL_ES? This would make it easier to check if the slack channels are set up correctly when adding new beamlines.

[Jun Aishima (JunAishima)]

I think this general method of handling the exceptions that occur within the Slack messaging is good, but I want to go over this before the end of the cycle, move into template, and propagate to the other workflow repos.

Do you mean we should just keep SLACK_UCAL should be replaced with SLACK_BL_ES for all repos? that does make sense.

[Abby Giles (AbbyGi)]

Sounds good with the exception handling. Up to you if we want to start replacing SLACK_UCAL now or wait until we roll out the exception handling, but the template should definitely pull these variables out and use something like SLACK_BL_ES

[Copilot]

Copilot review overview

🔵 Needs a closer look

Resolve the Docker environment loading issue and prevent Tiled credentials from being serialized as Prefect flow parameters.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the API-key exposure risk and ensure .env settings are loaded or provided through the runtime environment.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Copied .env is not loaded into the runtime environment

Dockerfile:10

Copying .env into the image does not load its values into the process environment. The only load_dotenv call in this PR reads /srv/container.secret, so PREFECT_LOGGING_EXTRA_LOGGERS and TILED_RETRY_ATTEMPTS from this file will not be applied at runtime (and Tiled is imported before that call). Set these through the deployment/container environment or explicitly load .env before importing the consumers.