Code review · P3 Task 5, the Snakemake mapping¶
review-code on autopilot/P3-t05, base 85bc086, 2026-10-30. The step is P1 Task 5. It adds:
spectio_server.projector.snakemake: the Snakemake branch ofingest, andhead_exit, which ends a Snakemake launch on its head's exit code;- in
spectio_server.projector.runs: the engine's time on run start and end (Projection.event_time,set_started), andtask_idandpin_engine_version, moved out ofnextflow.pyto be shared; - in
apply_head_report: a known Snakemake exit code wins over ahead_lostorengine_faultfailure; - migration
0003: thesnakemake_jobstable (launch,jobid→ task); - in
spectio_sdk.events:SnakemakeJobInfocuts theParams have changeddetail fromreasonand dropsslurm_extrafromresources; packages/spectio-server/tests/test_projector_snakemake.py, 26 tests; one more test in each oftest_projector_nextflow.pyandtest_spool_events.py.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. What the mapping does is owned by api §7, §7.1, §7.2 and §14.1; the rules decided here are in inbox #81.
| Round | Head or range | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 2fb85fa (the step) |
changes (0 blockers, 1 major, 2 minors) | approve (0 blockers, 0 majors, 3 minors) |
| 2 | ff79703 (the round-1 fix) |
changes (0 blockers, 2 majors, 5 minors) | approve (0 blockers, 0 majors, 4 minors) |
| 3 | 5b59f37 (the round-2 fixes) |
changes (0 blockers, 1 major, 3 minors) | approve (0 blockers, 0 majors, 5 minors) |
| 4 | 0f9fcf9 (the round-3 docs fix) |
approve (0 blockers, 0 majors, 1 minor) | approve (no findings) |
Minors include the simplify: findings, as in the earlier records.
Rounds. Three fix rounds before both approved in round 4. The third (0f9fcf9) answered a code-reviewer major about the docs (CR-P3T05-04 (r3)) while no security blocker or major was open. review-code step 4 and .claude/workflows/p3-task.js allow two fix rounds in that case, then set the step blocked. Recorded in inbox #81.
IDs repeat across rounds. Each round numbered its findings again, so each ID below carries its round: CR-P3T05-02 (r1) is not CR-P3T05-02 (r2). The commit messages use the bare IDs.
Test-first. The worker's record: each code fix began with a failing test; 0f9fcf9 changed docs only. Test command, passing at 0f9fcf9 (26 tests in this file, 214 in the workspace):
uv run pytest packages/spectio-server/tests/test_projector_snakemake.py -v
CODE step=P3-t05 review=approve b/M/m=0/4/11 security=approve b/M/m=0/0/12 rounds=4 simplify=nochange -0/+0 tests=pass
The counts are per reviewer over the four rounds, each finding counted in the round that raised it; a problem raised again counts again. code-simplifier ran after round 4 and changed nothing (HEAD stayed 0f9fcf9); it left CR-P3T05-07 (r3) because the fold would change behaviour (below). Its baseline: pytest 214 passed; ruff, mypy, lint-imports and uv lock --check clean.
Findings¶
The finding texts are summarised from the round summaries and the fix commits' messages. A pair such as CR-P3T05-05 (r2) / SEC-P3T05-03 (r2) is one problem found by both reviewers.
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T05-01 (r1) | major | The HeadReport contract told the runner to set failure whenever a head ends with no terminal engine event, which is every Snakemake head; apply_head_report checked failure first, so a Snakemake exit 0 became failed / engine_fault and a tool error lost its class and culprit. |
fixed in ff79703: a known exit code wins over head_lost / engine_fault; launch_error still fails (test_exit_code_wins_over_a_head_failure, test_exit_code_with_a_head_failure_keeps_the_tool_error, test_launch_error_with_an_exit_code_still_fails); the contract is in the HeadReport docstring and api §7.1 |
| CR-P3T05-02 (r1) | minor | The culprit is the launch's last failed job, so under --keep-going with retries a real tool error is classed engine_fault (job A fails for good, then job B fails and its retry succeeds). |
open, assumed: inbox #81 rule 3, option (a); the gap stays open |
| CR-P3T05-03 (r1) | minor | slurm_extra is dropped by a second, single-key mechanism next to DENIED_KEYS, so api §14.1's never-stored list and the code's one deny-list differ. |
kept by design: api §14.1 now names the drop as scoped to snakemake.job_info resources; see CR-P3T05-07 (r3) |
| SEC-P3T05-01 (r1) | minor | Engine time sets runs.started_at and ended_at with no bound against the ingest clock, and set_started overwrote started_at on every begin event, even after the run ended. |
partly fixed in 5b59f37 (only launch 1 sets started_at). Open: the 1970–9999 bound only; inbox #81 rule 5 |
| SEC-P3T05-02 (r1) | minor | SEC-P3T1-08 closed more widely than the evidence allows: format_error() embeds str(ex) of any exception, and a SyntaxError's source line. |
open: inbox #81 rule 7, follow-up FU-P3-05 |
| SEC-P3T05-03 (r1) | minor | The slurm_extra drop is complete for the executor plugins pinned today (slurm 2.8.0, slurm-jobstep 0.6.1, kubernetes 0.5.1); nothing fails if a plugin with a raw *_extra resource is added. |
open, [to be validated] in api §14.1 |
| CR-P3T05-02 (r2) | major | set_started moved runs.started_at to each launch's begin, so a Continue or resume moved the run's start (api §7: the first queued to running, the duration covers all launches). |
fixed in 5b59f37: only launch 1 sets started_at (test_a_later_launch_keeps_the_first_start) |
| CR-P3T05-03 (r2) | major | SEC-P3T1-08 was deferred here and not closed; after parse_data, job_info.reason of the tool_error_rerun fixture still held a param's shell command. |
fixed in 5b59f37: reason is cut after the label (SnakemakeJobInfo._cut_params_detail); outcome below |
| CR-P3T05-04 (r2) | minor | The code departs from P1 Assumption 16 in three places (attempt opened by job_info; exit 0 succeeds despite error records; the culprit overwritten) and records none of them. |
fixed by this step's docs: api §7.2, api §7, a dated note on P1 Assumption 16, inbox #81 rules 1 to 3 |
| CR-P3T05-05 (r2) / SEC-P3T05-03 (r2) | minor | head_exit and _failure read the run's events, not the launch's: a later launch that writes nothing would take an earlier launch's end time and error text. |
open: no 0.0.1 run has a second launch (a terminal run never moves); before Continue ships. Inbox #81 rule 5 |
| CR-P3T05-06 (r2) | minor | The runner contract the projector now depends on is not where Tasks 9 and 10 will read it: Assumption 15 still fails every Snakemake head, and a head report applied before the last spool batch would leave the last attempts aborted in a succeeded run. |
fixed by this step's docs: api §7.1 and a dated note on P1 Assumption 15 |
| CR-P3T05-07 (r2) | minor | The second deny mechanism, and §14.1 still says "left to P1 Task 5". | §14.1 fixed in 0f9fcf9; the mechanism kept, as CR-P3T05-03 (r1) |
| CR-P3T05-08 (r2) | minor | New external facts are sourced only in code comments: submit_string.py:206-207, job_scheduler.py:333-338, executors/base.py. |
fixed by this step's docs: linked at their tags in api §7.2 and §14.1, read 2026-10-30 |
| SEC-P3T05-01 (r2) | minor | SEC-P3T1-08 still open, and more visible: runs.failure.message is now the last snakemake.error text. |
open, as SEC-P3T05-02 (r1) |
| SEC-P3T05-02 (r2) | minor | Engine time is trusted without bounds; set_started runs before the terminal check; a Nextflow head_lost end uses the ingest clock against an engine-clock start; a late progress still writes tasks_expected after the end. |
partly fixed in 5b59f37 (launch 1 only). Open: the bound, the mixed clocks (now said in api §7 runs), the late progress; inbox #81 rule 5 |
| SEC-P3T05-04 (r2) | minor | The deferred findings are not recorded as closed: §14.1 still says "left to P1 Task 5", SEC-P3T1-08 has no disposition. | fixed in 0f9fcf9 and by this step's docs (the SEC-P3T1-08 outcome below) |
| CR-P3T05-04 (r3) | major | SEC-P3T1-11 is handled in code, but api §14.1 said the opposite, the P3 Task 1 record still said "deferred to Task 5", this record did not name it, and the plugin fact was only in a code comment. | fixed in 0f9fcf9; the source link added by this step's docs |
| CR-P3T05-05 (r3) | minor | The Progress page still lists SEC-P3T1-08 as going to Task 5, with no outcome. | fixed by this step's docs (P3-M1) |
| CR-P3T05-06 (r3) | minor | head_exit takes the end time from the run's last engine record across launches, which contradicts the launch-1 guard of set_started. |
open, as CR-P3T05-05 (r2) |
| CR-P3T05-07 (r3) | minor | simplify: _drop_slurm_extra could be one more entry in DENIED_KEYS. |
not applied: code-simplifier found the fold would also drop slurm_extra at any depth and in every event type, a behaviour change, and the spec names the scoped rule |
| SEC-P3T05-01 (r3) | minor | The docs still call SEC-P3T1-11 open. | fixed in 0f9fcf9 |
| SEC-P3T05-02 (r3) | minor | SEC-P3T1-08 is marked closed, but Snakemake 9.27.1's print_exception logs format_error with source lines, or a full traceback, and it is stored in full up to 4096 characters. |
open: the P3 Task 1 record now says "partly fixed"; inbox #81 rule 7, FU-P3-05 |
| SEC-P3T05-03 (r3) | minor | The reason cut matches the exact English label of 9.27.1 (jobs.py:1968-1970); a reworded label would store param values again with no test failing. |
open: inbox #81, a test for the next Snakemake pin |
| SEC-P3T05-04 (r3) | minor | Nothing in the spec tells later retention or staleness code that started_at and ended_at are asserted by the engine. |
fixed by this step's docs: api §7 runs |
| SEC-P3T05-05 (r3) | minor | Events go to a projector by the prefix of their type, not by the run's engine: a snakemake.workflow_started line pins engine_version on a Nextflow run, and snakemake.progress sets tasks_expected there. |
open, follow-up FU-P3-06: project only events of the run's engine |
| CR-P3T05-04 (r4) | minor | The slurm_extra fact is cited "checked against the 2.8.0 wheel", with no link and no dated audit. |
fixed by this step's docs: linked at tag v2.8.0 (91f51af), read 2026-10-30, in api §14.1 |
Closed from Task 1's review (record):
| Id | Outcome |
|---|---|
| SEC-P3T1-11 | closed: slurm_extra is never stored. SnakemakeJobInfo._drop_slurm_extra drops it in 2fb85fa, since snakemake-executor-plugin-slurm 2.8.0 appends it raw to the sbatch call (submit_string.py:206-207, tag v2.8.0, read 2026-10-30); test test_slurm_extra_never_kept. Other executors' *_extra keys [to be validated] |
| SEC-P3T1-08 | partly fixed in 5b59f37, see the outcome below; the rest open (FU-P3-05) |
SEC-P3T1-08, outcome. The free-text Snakemake fields of the three T2 fixtures (snakemake-9.27.1_{checkpoint_flaky_retry,tool_error,tool_error_rerun}.spool.jsonl) were gone through after parse_data:
| Field | Outcome |
|---|---|
snakemake.error msg |
kept, quoted command hidden; the fixtures hold only the Command '…' form, which is hidden. Other exception texts (a traceback, a Snakefile source line) are not: open, FU-P3-05 |
snakemake.job_info reason |
was a leak: Params have changed since last execution: … before: 'python3 …/synth.py fail exit $SPECTIO_ATTEMPT 2' now: 'true'. Now cut after the label (SnakemakeJobInfo._cut_params_detail) |
snakemake.job_info msg, snakemake.job_error msg, snakemake.log msg, snakemake.shellcmd msg |
not in the allow-lists: dropped at parse |
Tests: test_no_parsed_record_holds_the_fixture_shell_text (each fixture: no parsed record holds synth.py or SPECTIO_ATTEMPT) and test_params_changed_reason_keeps_the_label_only. The rule is in api §14.1.