Post-merge follow-up to #339: two protocol rules, and the two policies it left open - #340
Merged
Conversation
…k gate Two rules #339 paid for. A rename is a whole-tree fact. #339 renamed a test double, #281 landed mid-flight with a fixture annotated on the old name, git merged both without a conflict because they are different lines, and every targeted pytest module passed — an annotation is never evaluated. CI answered F821. So: after a rebase that brings in commits you did not write, `ruff check .` over everything, not over what you edited. And the local gate now has a stated fallback rather than an unstated one. A green `check.sh` is still what a merge requires; when another session demonstrably owns the machine, the substitute is every static gate, the full frontend suite and the affected pytest modules, with green CI as arbiter — declared in the PR body, because a merge that lets a reader infer a local run that never happened is a protocol violation rather than a shortcut.
The CHANGELOG already recorded the breaking change; this names the shape it replaced (`FileResponse`) and the route the archive moved to, so the entry reads without the PR beside it. The two open review items are closed as decisions rather than left as questions somebody re-opens: `exports/` has no retention policy, and that is the policy. No TTL, no cap, no sweeper, no DELETE route — the disk is the user's, and a local-first tool deleting an exported training set on a schedule nobody chose takes an action nobody can undo. It is the posture blobs and uploads already have; what it costs is that a workspace only grows, and `rm` is the remedy for the two directories that are reproducible. The `job` table has no `project_id` and no foreign key at all. A job is workspace-scoped plumbing and its subject lives in the payload, so the column would be null for some types and wrong for others — and the *key* would cascade away the record of work that already happened. Also corrects drift #339 introduced: `docs/workspaces.md` still credited the export route with clearing the output directory, which the handler does now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge follow-up to #339. Docs, one skill file, and the CHANGELOG — no source changes.
What changed
refactor-protocolgains two rules, both of which #339 paid for.api: batch membership editing is not on the wire, and the gallery is now the caller #29 was waiting for #281 landed mid-flight with a fixture annotated on the old name; git merged both without a
conflict because they are different lines, and every targeted pytest module passed, because an
annotation is never evaluated. CI answered
F821. So: after a rebase that brings in commitsyou did not write,
ruff check .over everything.bash scripts/check.shis still what a mergerequires. When another session demonstrably owns the machine — with evidence — the substitute
is every static gate, the full frontend build and suite, and every affected pytest module, with
green CI on clean runners as arbiter. It must be declared in the PR body before merging; a
merge that lets a reader infer a local run that never happened is a protocol violation rather
than a shortcut.
The two open review items on #339 are closed as decisions, written where the next person will
look rather than left as questions somebody re-opens:
exports/has no retention policy, and that is the policy —docs/releases.md(where exportbehaviour lives) and
docs/workspaces.md(where the layout does). No TTL, no size cap, nosweeper, no
DELETEroute. The disk is the user's; a local-first tool deleting an exportedtraining set on a schedule nobody chose takes an action nobody can undo. Same posture blobs and
staged uploads already have. What it costs is stated — a workspace only grows — and so is the
remedy,
rm, which is safe foruploads/andexports/because both are reproducible and isemphatically not safe for
blobs/.jobtable has noproject_id, and no foreign key at all —docs/background-jobs.md,in "The decisions, and why" plus a one-line pointer in "Limits, stated". Scoping lives in the
payload, because different job types are about different things and the column would be null
for some and wrong for others. The keyless half is the load-bearing one: a key means a
cascade, and a cascade would delete the record of work that already happened.
The CHANGELOG entry is sharpened, not added. #339 already recorded the breaking change; this
names the shape it replaced (
FileResponse) and the route the archive moved to, so the entryreads without the PR open beside it.
Which doc, and why not a new one
AGENTS.mdsays to update the doc that covers the topic and create one only if none does. #339created
docs/background-jobs.md, so this extends it. Export behaviour is documented indocs/releases.mdand the layout indocs/workspaces.md, so the retention policy is stated inboth and cross-referenced from the jobs doc rather than restated a third time.
Found, and fixed because it is the same paragraph
docs/workspaces.mdstill credited the export route with clearing the output directory.#339 moved that into the handler, so the sentence had been wrong since
d39eeef— in the exactparagraph this change annotates. Corrected rather than left, and called out here rather than
slipped in.
Found, not fixed
../visionset-rename-sweepwith uncommitted edits to this same skillfile (
refactor-protocol/SKILL.md), adding three bullets about rename sweeps. Their edit isin Scope discipline; mine is in Testing requirements, so the two hunks do not overlap and
whichever lands second should merge cleanly. Flagging it because a clean merge is not a
guarantee — see the very rule this PR adds. I did not touch their worktree. cf. feat(cli): rename server start command from
visionset uitovisionset server#333.docs/ingest.mddescribes ingest progress as living on theingest_jobrow, which is true andnow carries the "two rows for one run" caveat An embedded job system: a JobQueue port, a SQLite queue, and a spawn dispatcher (#328) #339 gave it. Collapsing the two rows is the real
fix and remains a migration with its own wire-contract discussion. Untouched here.
Test plan
No source changed, so nothing executable moved.
ruff format --check .is the gate with realcoverage here — it formats Python blocks inside Markdown, and
docs/background-jobs.mdcontainsone.
ruff check .— passruff format --check .— pass, 299 filesmypy src/visionset— pass, 131 fileslint-imports— 3 contracts keptnode --test "tests/scripts/*.test.mjs"— 33 pass, 0 failpnpm -r build— passpnpm -r test— 1289 pass (annotator 769, ui-core 520)uv run pytest tests/architecture tests/packaging— pass (the only Python modules a docschange can reach; both read
git ls-files)bash scripts/check.shdid not complete. It was started and abandoned; everything above isthe sanctioned substitute. Evidence, as the rule requires:
Four other sessions own this machine right now. At that rate the Python suite alone was hours
away, and a timing-sensitive suite under load 45 would not have been worth trusting if it had
finished. Full green CI on clean runners is the arbiter for this PR. No source changed, so
the executable risk is close to nil — but "close to nil" is exactly the reasoning the new rule
exists to stop being silent about, so it is stated rather than assumed.
Found, not fixed (second)
Running the node gates in a fresh worktree failed 9 of 33 — every one of them
node_modulesmissing (
Cannot find package 'openapi-typescript', and the annotator boundary gates needingtsc/eslint).pnpm installfixed all nine. That is #336 — check.sh stepping over amissing
node_modulesinstead of failing loudly — reproduced here from the other direction.Untouched; it has its own issue. cf. #336.