Record code runs with pull, commit and push

[krystophny]

Closes #2

Problem

A scientist running a recorded code run needs commands to manage their
work across a local working directory (RUN) and long-term storage (DATA).
Issue #2 specifies a workflow where the current directory is RUN:

1. dat pull <path> — pull required inputs/templates from DATA to RUN
2. run the job (e.g. slurm <run_code>)
3. dat commit -m <message> — record the started run in the database with
   status running
4. dat push — set status finished, update the database, and push
   results from RUN to DATA

Changes

• dat pull <path>: unidirectional copy of DATA/<path> into the
  current directory via rclone copy.
• dat commit -m <message>: writes the title (and optional body after
  a newline) plus automatically retrieved information (status running,
  start time) to metadata.json, and creates a database entry with status
  running through the eLabFTW API.
• dat push: sets status finished, records the finish time, updates
  the eLabFTW database entry, and unidirectionally copies the current
  directory from RUN to DATA via rclone.
• Metadata is stored in metadata.json using the eLabFTW standard
  (elabftw.extra_fields). DATA location is configurable via DAT_DATA
  (file system path or rclone remote); eLabFTW credentials come from
  ELAB_URL/ELAB_KEY environment variables.
• Added pytest coverage for pull, commit, and push, including message
  parsing, the running/finished status transition, and the rclone syncs.

Tests

python -m pytest tests/ -q — 7 tests pass.

[krystophny]

Review verdict: Request changes

Summary: The workflow is broadly implemented, but the eLabFTW metadata layout and push failure handling contain release-blocking correctness issues. No CI runs were available, and local pytest could not run because pytest is not installed.

Findings:

1. [major] src/dat/journal.py:39 — extra_fields is nested under metadata.elabftw, but eLabFTW requires it directly under metadata (with elabftw as a sibling). The status, message, and timestamps will therefore not be processed as custom fields. Move extra_fields to the required level.
2. [major] src/dat/dat.py:91 — push marks the run finished locally and in eLabFTW before rclone succeeds. A failed transfer leaves metadata claiming completion although results were not stored. Sync first, then finalize status, or implement explicit rollback/retry state handling.
3. [major] src/dat/dat.py:123 — pull accepts absolute and .. paths; os.path.join can escape both DATA_ROOT and RUN, potentially copying unintended data or overwriting outside the run directory. Require a normalized relative path and reject traversal.

Verdict: Request changes — the core metadata persistence and push consistency guarantees are not currently reliable.

[krystophny]

Review verdict: Request changes

Summary: The workflow is mostly coherent, but push leaves DATA with stale run metadata.

Findings:

1. [major] src/dat/dat.py:96-102 — rclone_copy() runs before the finished status and timestamp are saved to metadata.json. Consequently, DATA receives the old metadata (or none), while RUN and eLabFTW report finished. Save and sync the final metadata as part of the push, with appropriate failure handling.

Verdict: Request changes — push does not reliably persist the completed run state to DATA.

[krystophny]

Review verdict: Request changes

Summary: The workflow is implemented and path/metadata fixes are present, but push still leaves run state inconsistent when the DATA transfer fails. No CI checks were available; local tests could not collect because elabapy is unavailable.

Findings:

1. [major] src/dat/dat.py:97-104 — push saves status=finished locally before rclone_copy succeeds. If the transfer fails, RUN/eLabFTW can report a finished run while DATA lacks the results, and retrying sees no running run. Sync with failure-aware state handling, or roll back/retry the completion state when the transfer fails.

Verdict: Request changes — failed pushes can falsely finalize runs and lose synchronization with DATA.

[krystophny]

Review verdict: Request changes

Summary: The PR adds the requested commands and basic tests, but several core acceptance and data-safety requirements remain unmet. GitHub reports no CI checks, and local tests cannot collect without elabapy.

Findings:

1. [major] src/dat/dat.py:45 — rclone copy omits --ignore-existing and --immutable, so pull can overwrite local edits and push can overwrite existing DATA files. Add both flags and test the exact invocation.

2. [major] src/dat/dat.py:138 — dat pull <path> copies into RUN/<path>, producing ./templates/input.h5 instead of copying the requested DATA subdirectory’s contents into RUN. Use RUN as the destination.

3. [major] src/dat/dat.py:29 — globals() still exposes imported modules as commands; dat os raises a traceback instead of a clean unknown-command error. Use an explicit command map and exit status 2.

4. [major] src/dat/dat.py:98 — DATA receives metadata.json while its status is still running, then local metadata is changed to finished; successful archival data therefore permanently reports the wrong status. Ensure the final metadata is copied while preserving retry-safe failure handling.

5. [major] pyproject.toml:17, src/dat/journal.py:6 — the implementation depends on deprecated API-v1 elabapy, so the eLabFTW integration is not viable against the supported API. Migrate to elabapi-python and update the storage implementation accordingly.

Verdict: Request changes — the current implementation can overwrite data and persist incorrect run status, while the eLabFTW dependency is obsolete.

[slopqueue]

Review verdict: Request changes

Summary: This PR replaces the experimental elabapy interface with the official elabapi-python v2 client and turns dat into a pull/commit/push workflow: pull copies a path from DATA, commit -m records a running run, and push syncs results and marks the run finished. It replaces the unsafe globals()-lookup dispatch with an explicit COMMANDS whitelist, hardens pull against path traversal, adds rclone --ignore-existing/--immutable flags, and adds a 12-case test suite (all pass) with faked eLab storage and rclone. The design and tests are solid; the main defect is in the real eLabFTW authentication setup, invisible to the faked tests.

Findings:

1. [major] src/dat/journal.py:83-86 — ElabStorage.__init__ sets configuration.api_key["Authorization"] = ELAB_KEY but never sets configuration.api_key_prefix["Authorization"] = "Bearer". The library's get_api_key_with_prefix (confirmed in installed client) emits only the raw key without a prefix, so requests go out as Authorization: <token> instead of Authorization: Bearer <token>. The official elabapi-python usage requires the Bearer prefix for eLabFTW v2; without it the server rejects (401) every request. Since commit/push swallow eLab exceptions into a Warning:, the tool silently never mirrors to eLabFTW while reporting success. Fix: add configuration.api_key_prefix["Authorization"] = "Bearer" and add a test that the config is wired correctly.

2. [minor] src/dat/dat.py:71-92 (commit) — If elab.create() succeeds but the subsequent elab.save() fails, the id is never persisted locally, and a retry of commit calls create() again, leaving an orphaned/duplicate experiment on the server. Persist the id locally immediately after create().

3. [minor] src/dat/journal.py:104-107 — Patching with metadata=entry.get("metadata") replaces the whole server metadata body, overwriting eLabFTW-managed fields. Using metadatamerge on EntityEditable to merge only extra_fields would be non-destructive.

4. [minor] tests/test_dat.py:207-213 — test_unknown_command_exits_2 is not robust: dat.main() always sys.exits, so dat.COMMANDS["os"] is dead code that never runs; the test passes only incidentally because sys.argv[1] during pytest is an unrecognized token. Call main() with a controlled sys.argv (["dat", "os"]) and assert the exit code, or drop the dead line.

Verdict: Request changes — the data-sync design, path hardening, and dispatch refactor are good, but the missing Bearer auth prefix means the eLabFTW mirroring (a core goal of this PR) silently fails against a real server, and the tests cannot catch it because ElabStorage is faked.