Code review · P3 Task 1, workspace and the spool event contract¶
review-code on autopilot/P3-t01, base 9e08a3e, 2026-10-29. The step is P1 Task 1. It adds:
- the uv workspace (
pyproject.toml,uv.lock, three packages); spectio_sdk.events(the spool envelope, the allow-list models, the caps);- the converted T2 fixtures;
- the CI job
product; - api §14.1.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md.
| Round | Head | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 9fb1d97 |
changes (0 blockers, 2 majors, 4 minors, 2 of them simplify:) |
changes (0 blockers, 1 major, 5 minors) |
| 2 | 3be01c1 (the round-1 fixes) |
approve (0 blockers, 0 majors, 2 minors, plus the 2 open simplify: findings) |
approve (0 blockers, 0 majors, 4 minors) |
| 3 | 7106465 (delta 3e5342b..7106465: the doc-review fix DR-P3T1-01) |
not run (the change is a security fix, reviewed by security-reviewer) |
approve (0 blockers, 0 majors, 2 minors: SEC-P3T1-11 deferred to Task 5 with a §14.1 note, SEC-P3T1-12 fixed in f456646) |
The round-2 minors CR-07, CR-08, SEC-07 and SEC-10 were fixed in c69bccb. Each fix was test-first: the new tests failed before the change and pass after it. Round 3 reviewed the fix of the doc review's finding DR-P3T1-01 (security only). Test command:
uv run pytest packages/spectio-sdk/tests/test_spool_events.py -v
CODE step=P3-t01 review=approve b/M/m=0/2/6 security=approve b/M/m=0/1/11 rounds=3 simplify=changed -12/+3 tests=pass
code-simplifier ran on c69bccb (commit 3e5342b); 38 tests pass after it.
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T1-01 | major | seq scope: §14.1 said "per (run, launch, producer)", while api §8.2 and architecture §13.1 said "per producer and run". A second launch restarts at 1. |
fixed in 3be01c1: §8.2 and §13.1 now say per (run, launch, producer), with dated notes |
| CR-P3T1-02 / SEC-P3T1-01 | major | snakemake.error.msg stored the failed shell command, export values included (5 fixture events). |
fixed in 3be01c1: the command is hidden, the check fails closed, and the text is capped at 4096 characters; a test over every fixture value now checks it |
| CR-P3T1-03 | minor | time accepted epoch numbers. |
fixed in 3be01c1: time must be an RFC 3339 string, in UTC years 1970–9999 |
| CR-P3T1-04 | minor | rule_msg was dropped, but no test pinned it. |
fixed in 3be01c1: added to DENYLIST, and §14.1 names it |
| CR-P3T1-05 | minor | simplify: NextflowTrace.model_config repeats _Data's config |
fixed by code-simplifier in 3e5342b: the override is removed |
| CR-P3T1-06 | minor | simplify: two tests walk the same fixture lines | fixed by code-simplifier in 3e5342b: folded into test_t2_fixtures_validate |
| CR-P3T1-07 | minor | The poison-line test also accepted UnknownEventType. |
fixed in c69bccb: the test expects ValidationError only |
| CR-P3T1-08 | minor | A signal kill lost its cause, and other ways of quoting a command are not hidden. | fixed in c69bccb: died with <…> is kept; §14.1 says other quoting forms are not hidden [to be validated] |
| SEC-P3T1-02 | minor | Values passed validation but could not be stored: unbounded integers, epoch time, coerced data integers, NaN, NUL. |
fixed in 3be01c1: Int64 bounds, strict data models, a recursive check on free-form JSON, and 15 poison-line tests |
| SEC-P3T1-03 | minor | The fixture credential guard did not cover the engine-event fixtures. | fixed in 3be01c1: 24 files are scanned, and convert_t2.py refuses to write on a match |
| SEC-P3T1-04 | minor | The reader's rules were missing: names that must match the grammar, wrong_launch, a cap on partial lines, a cap on the number of files. |
deferred to Task 3, which builds the reader; handed to its implementer with this record |
| SEC-P3T1-05 | minor | The §14.1 wording on event paths was wrong, and req. 6 was stale. | fixed in 3be01c1 |
| SEC-P3T1-06 | minor | The import contracts are static only. | partly fixed in 3be01c1 and c69bccb: the sdk forbids fastapi and sqlalchemy, ruff TID251 bans dynamic imports in packages/spectio, and an AST test refuses __import__. The fresh-environment CI check is deferred to Task 3 |
| SEC-P3T1-07 | minor | Hiding the command took quadratic time, and a quoted terminator let the command through. | fixed in c69bccb: the text is cut before the regex, which is now greedy; resolved, verified in round 3 |
| SEC-P3T1-08 | minor | Hiding is a deny-pattern; other Snakemake error forms would pass through. | deferred to Task 5 [to be validated] |
| SEC-P3T1-09 | minor | Nextflow's errorMessage is capped but not hidden. |
deferred to Task 6 (nf-spectio sends errorMessage, never errorReport) |
| SEC-P3T1-10 | minor | The AST guard missed importlib.__import__; other loaders were not banned. |
fixed in c69bccb; resolved, verified in round 3 |
| SEC-P3T1-11 | minor | resources values are stored as the rule declares them (deny-listing is by exact key name); the Snakemake SLURM plugin's slurm_extra may carry raw sbatch flags [to be validated]. |
deferred to Task 5, with a note in §14.1 (f456646) |
| SEC-P3T1-12 | minor | The at-any-depth drop of 6d50d3c also removed wildcards named env or cmd, which are the workflow's own labels. |
fixed in f456646: wildcards kept as declared, test-first (1 test) |
| DR-P3T1-01 | block (doc review) | api §14.1 said the denylisted keys are never stored, but free-form resources, rulegraph and wildcards kept them nested. |
fixed in 6d50d3c: dropped at any depth, 5 tests (wildcards then excepted by SEC-P3T1-12) |