Repository navigation
Clean up issues brought up by Copilot reviews #13
Description
Assisted-by: copilot:gpt-5.6-luna
Review findings
🟡 Medium — Missing secret produces the wrong error and retries unnecessarily
data_validation.py:11 indexes dotenv_values(...) before the None check:
If TILED_API_KEY is absent from /srv/container.secret, this raises KeyError; therefore the intended ValueError("TILED_API_KEY must have a value") at line 13 is unreachable for the most likely misconfiguration. Because this occurs inside the retried get_run/get_run_migration tasks, the workflow will retry a permanent configuration error twice before failing.
Use .get("TILED_API_KEY") and validate both missing and empty values:
🟡 Medium — Removing api_key is a breaking workflow-interface change
The change removes api_key from the deployed flow signature at end_of_run_workflow.py:79 and from data_validation at data_validation.py:43.
Any existing Prefect trigger, deployment invocation, or external caller that still sends api_key will now fail with an unexpected keyword argument instead of ignoring or using it. The repository has no migration note or compatibility handling for this interface change. Confirm that all event producers have been updated, or retain a deprecated parameter temporarily while ignoring/rejecting it explicitly.
Activity
JunAishima commentedon Sep 23, 2026
also raised during NSLS2/ucal-workflows#6 -
The earlier review identified potential Tiled credential exposure. That concern remains visible in the code:
The decorator wrapper accepts api_key at end_of_run_workflow.py:31.
The Prefect flow also exposes it as a parameter at end_of_run_workflow.py:125.
Prefect generally records flow parameters in flow-run metadata. If a real API key is supplied this way, it may be persisted and displayed in the Prefect UI or API. The preferred design is to remove credentials from the flow signature and load them at runtime from a Prefect Secret/block or another protected secret source.
The code already has a runtime fallback in get_api_key_from_env, so callers may not need to pass the key through the flow at all.
JunAishima commentedon Sep 23, 2026
from tst-workflows:
TILED_API_KEY remains exposed in the worker environment
load_dotenv mutates os.environ, and this function leaves TILED_API_KEY there after returning it. The credential therefore remains available to subsequent tasks and child processes for the lifetime of the worker, which undermines the secret-file-only handling; read it with dotenv_values or remove/restore the environment entry immediately after reading it.
JunAishima commentedon Sep 23, 2026
from tst-workflows:
Moderate:
Early Tiled failures bypass failure notifications
The Tiled lookup (and the preceding block/context setup) runs before the try block. If get_run cannot read the secret or connect to Tiled, the flow fails without sending either failure notification, defeating the error-reporting wrapper for one of the first operations it performs. Move failures that should be reported inside the protected block, while handling notification setup separately.
Edit: note that we did have the Slack handling updated in ucal - I believe this solved the same problem with that repo