One address per plan: the readable URL is the only URL - #196
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80451fcebe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| params.delete(:page) | ||
| result = resolve(slug_path: [ params[:slug_path], *page_tail ].compact_blank.join("/")) |
There was a problem hiding this comment.
page
When a plan's leaf slug is edit or history beneath a folder, the corresponding sub-page route first resolves only the folder and enters this fallback. Deleting params[:page] before calling page_tail makes page_tail return [], so the request resolves the folder again instead of the plan; the plan's canonical URL therefore renders the wrong page. The same ordering also drops the requested sub-page when resolving an aliased document path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Confirmed and fixed in 803feba — good catch, and the reason it survived is that my own spec was too weak to see it.
page_tail derives the tail from params[:page], so deleting :page first left nothing to put back and the folder resolved a second time. The spec that was supposed to cover this asserted the response mentioned "Edit" — which the folder listing containing that plan does too, so it passed on the wrong page. I verified the bug directly before fixing: /hampton/liveorder/edit returned 200 with no rel=canonical at all, i.e. the library view.
Reassembly now happens before the delete, and the spec asserts through rel=canonical plus the document body, neither of which the folder page can produce.
Your second point was right too and was a separate bug: the stale-path 301 dropped the sub-page, so asking for a moved document's editor landed on the document. page_tail rides through the redirect now, with specs for /edit and /history/2/diff.
| library.unfiled_plans | ||
| end | ||
|
|
||
| scope.where(slug: slug, slug_suffix: nil) | ||
| scope.where(slug: slug, slug_suffix: nil).lock |
There was a problem hiding this comment.
On MySQL's default repeatable-read isolation, a folder save can perform validation reads, wait for the library lock, and then execute this locking outer query while the placement subquery still reads the earlier snapshot. If a concurrent move just placed an unsuffixed same-slug plan at this level, the sweep misses it; the folder wins resolution and leaves the plan unreachable. AssignSlug#siblings already avoids this exact issue by replacing placement subqueries with locking joins, and this query needs the same treatment.
AGENTS.md reference: AGENTS.md:L30-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Agreed, and this was already fixed before your review landed — you reviewed 80451fc, and the force-push to b7b5432 had changed it.
shadowed_plans now joins:
Plan.joins(:placement).where(coplan_plan_placements: { folder_id: parent_id })
It got fixed for a second reason as well, which is worth recording next to your point. The first shape I pushed used left_joins(:placement) for the unfiled sibling read, and PostgreSQL rejects that outright: FOR UPDATE cannot be applied to the nullable side of an outer join. So the shape rule is now two-sided — coplan_plans stays in the outer query (your snapshot concern), and no outer join sits under FOR UPDATE (the PG constraint) — and there's a spec pinning it, since the suite normally runs on one database at a time and each constraint is invisible to the other's CI job.
Verified on both: 1798 examples, 0 failures on MySQL and on a freshly migrated PostgreSQL.
| if plan["slug"].present? | ||
| # Only an un-suffixed slug blocks the segment; a suffixed one has | ||
| # already moved out of the way. | ||
| (taken[key] ||= Set.new) << plan["slug"] if plan["slug_suffix"].blank? | ||
| next |
There was a problem hiding this comment.
When an already-slugged sibling has a slug_suffix, this pass records nothing for it, but unique_suffix checks only the seen set. If an unslugged duplicate's deterministic candidate equals that existing suffix, the migration assigns both plans the same leaf path; there is no database uniqueness constraint to reject it, so one document becomes unreachable. Add existing suffixed leaves to seen while still reserving only unsuffixed base slugs for the initial contest check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Real bug, fixed in 803feba. The consequence is worse than the odds suggest: the suffix is derived from the plan id precisely so a re-run produces the same URLs, which means it can't be re-rolled on a collision, and there's no unique index to reject the duplicate. Two documents, one address, silently — on a one-shot migration with no second run to correct it.
Existing suffixed leaves go into seen now. The bare slug still doesn't, since a suffixed plan has already moved out of the way and shouldn't block the base segment.
I also gave the migration a spec (spec/migrations/backfill_plan_slugs_spec.rb), following the pattern of the two existing ones. It builds the pre-migration state with the NOT NULL constraint lifted, which is the only way to produce a plan the model no longer can. I checked the collision example fails without the fix rather than assuming it:
expected: value != ["cart-roadmap", "5sa4"]
got: ["cart-roadmap", "5sa4"]
Writing it turned up something adjacent: coplan_plan_events was missing from PLAN_TABLES, so filing a plan in a non-transactional spec leaked events into every later spec that counts them. Added, and the suite now passes twice in a row.
/_/plans was still the page I actually read my own work on, which is the tell that the browsable scheme hadn't finished landing. A document that has two addresses has one people paste and one people see, and the id form kept winning because it was the one every internal link produced. So the id form stops existing as a destination. - Every plan has a slug. BackfillPlanSlugs gives one to every plan that predates the scheme and then makes `slug` NOT NULL, so "no readable address yet" is no longer a state the app has to have an answer for. The rules are inlined in the migration, like the folder and handle backfills before it: a migration has to keep producing the same URLs years after the app's rules move on. - The id fallback dies. plan_browse_path, Urls::Canonical.plan_path and plan_browse_url always return the readable path, and rel=canonical always renders rather than only when a slug happened to exist. - A document's pages hang off the document. /hampton/liveorder/cart-roadmap gains /edit, /history, /history/2 and /history/2/diff, so trimming the tail walks back to the thing being edited — the same property every other prefix in this scheme has. Versions are addressed by the revision number already on screen, not by id. When the tail turns out not to hang off a document (/sam/notes/history, where "notes" is a folder) the path is put back together and resolved as a place, so a plan someone titled "History" keeps its address. - /_/plans is gone. /plans and /plans/<uuid> stay in the frozen legacy block as 301s, so every old link converges on the readable form — query string and all — instead of rendering a second copy of the page. The id-based `_` scope keeps only the mutations, which don't need readable URLs because a button doesn't. - "All plans" was a scope toggle on a page that no longer exists; the answer to "where is someone else's work" is now their library, so the specs that exercised scope=all visit /theirhandle instead. - The API says where a plan is: plan_json carries `url`, and the agent instructions mention it, so an agent that creates a plan can hand back the address a human will actually use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now that a plan's readable path is its only path, two plans landing on one address isn't a cosmetic collision — it's a document with no way to be reached. AssignSlug decided contention by reading the siblings and then writing, with nothing holding the two together, so two saves a millisecond apart both read "free" and both took it. No unique index can close this. The contest crosses tables (the slug is on coplan_plans, the level is on coplan_plan_placements) and it crosses *models* — a folder and a plan want the same word, and the folder wins, which is why Folder#disambiguate_shadowed_plans exists. Nothing spans coplan_folders and coplan_plans. The write path is the only place "one segment, one thing" can be decided, so the read it decides from has to be authoritative. Library#lock_namespace! is that: a row-level FOR UPDATE on the library, taken inside the caller's transaction and released on commit. Row-level rather than an advisory lock so MySQL and PostgreSQL behave the same. A library is one person's, so nothing waits on it in practice, and one lock per transaction — always the library being written to — means writers can't deadlock against each other. Calling it outside a transaction raises, since there it would release on the next statement and guard nothing while looking like it does. - AssignSlug takes the destination library's lock before reading. - Folder takes it in before_save, but only when the slug or parent actually changes: a description edit contests nothing and shouldn't queue behind anything. Held through the shadowed-plan sweep. - Plans::Place is now one transaction with the lock taken first, because a move is one fact — the placement row, the slug that follows from it, the alias that keeps the old address working, and both audit trails either all happen or none do. The lock alone did not fix the race, and the reason is worth writing down. Under MySQL's REPEATABLE READ, FOR UPDATE reads latest-committed only for the tables the query actually scans. The sibling read was `Plan.where(id: <subquery over coplan_plans>)`, and MySQL evaluates that IN (SELECT …) as a plain consistent read off the snapshot this transaction took at its *first* read — which happened before the lock. So the writer we had just finished waiting for was filtered out before the outer locking read ever touched its row: the lock worked perfectly and guarded a stale set. Both sibling reads now keep coplan_plans in the outer query, where the locking scan can see it. Which table the placement lives in then splits the two cases. The filed read joins, because the placement row is what decides membership and so has to be read fresh too. The unfiled read leaves placements in a subquery — a plan created a moment ago has no placement in either the snapshot or the present, so NOT IN answers the same from both — and that is deliberate rather than incidental: PostgreSQL refuses FOR UPDATE on the nullable side of an outer join outright, so the left join this briefly used is a hard error on a host we support. A spec pins the shape, since the suite normally runs on one database at a time. segment_claims_spec covers which writes take the lock and which don't, that it refuses to run outside a transaction, and two real two-connection races: one asserting two same-titled plans get two addresses, one asserting each address resolves back to its own plan. Before the fix the race produced two identical paths; after it, one plan takes a suffix and either thread can be the one that moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
80451fc to
b7b5432
Compare
Three from the Codex review on #196, the first of which was a live bug my own spec was too weak to catch. `/hampton/liveorder/edit`, where "edit" is a plan someone titled Edit, served the *folder* — `params.delete(:page)` ran before `page_tail`, which derives the tail from `params[:page]`, so there was nothing left to put back and the folder simply resolved a second time. The spec passed because it asserted the page mentioned "Edit", and the folder listing containing that plan mentions it too. Reassemble first, and assert through rel=canonical, which can only name one of the two. The same `page_tail` is now carried through the stale-path 301. Asking for a moved document's editor and landing on the document is a silent demotion of what was asked for, so /edit and /history/2/diff ride along. The backfill registered nothing for a sibling that already had a suffixed slug, while its own suffix is derived from the plan id and so can't be re-rolled on a collision. A backfilled plan whose first candidate matched that sibling got the same leaf, with no unique index to reject it — two documents, one address, silently. Existing suffixed leaves go into the seen set now (the bare slug still doesn't, since a suffixed plan has already moved out of the way). That migration now has a spec, this being a one-shot that hands real documents permanent addresses: there is no second run to correct a mistake. It builds the pre-migration state with the NOT NULL constraint lifted, which is the only way to get a plan the model can no longer produce, and the collision example fails without the fix above. Folder#shadowed_plans was the third, and was already fixed while making the locking reads portable — it joins rather than reaching through a placement subquery. Writing those specs surfaced a fourth thing: coplan_plan_events was missing from PLAN_TABLES, so filing a plan in a non-transactional spec leaked events into every later spec that counts them. The full suite now passes twice in a row, which is what proves it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review pass over the whole PR. One real defect, one gap. `url` in plan_json reaches for the plan's whole location — the folder's ancestors for the path, the library for the handle — and reached for it through the associations, once per row. GET /api/v1/plans went from 12 queries to 21 for three plans and from 33 to 98 for twenty-four: roughly three extra per plan, on the list endpoint agents page through. The placement was already being preloaded into a hash for `folder_path`, but `url` went around it via plan.placement. The location is preloaded on the query itself now, down to the library and two levels of folder parent, which is what both fields needed. Twelve location queries for thirty plans instead of ninety-three, and the hash and its placement_for indirection are gone — the association preload serves both fields, so it was two ways of loading the same rows. Asserted as a shape rather than a number: the location-table query count for thirty plans has to equal the count for three. Counting *all* queries would fail on tag_names, which plucks per plan for reasons of its own and predates this branch, and the number would then get bumped to whatever made it pass rather than fixed. Home's `?tag=` had no coverage at all. It arrived in this branch as the replacement for the workspace's scope=all — a tag spans libraries, so it isn't a place inside one — which makes it the only cross-library tag view there is, reached from every row's tag chip. Specs now cover narrowing, the empty state, the clear control, and that the chips point at it. Everything else I checked came out clean and is recorded here so the next reader doesn't have to re-derive it: the sub-page actions authorize themselves (edit_content asks for :edit_content?, not the :show? that got it there); url_path is total, since slug is NOT NULL and User#library is an invariant rather than an association; the lock is taken only on writes that claim a segment, both for plans (before_save ... if url_slug_stale?) and folders; and a plan can only ever be placed in its author's library, which is what keeps Place's one-lock-per-transaction claim true — worth revisiting when team libraries land, since two libraries in one transaction is where lock ordering starts to matter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last thing #196 left on the table. Its location preload had to count only the location tables, because counting everything would have failed on `tag_names` — which was doing a query a row — and the number would then have been bumped to whatever made it pass. That note called the pluck the cause and left it alone. The pluck isn't the cause. `pluck` on a relation that's already loaded reads the loaded records instead of querying; it has since Rails 6. So `tag_names` is free on a plan whose tags are loaded and one query on a plan whose tags aren't, and the whole story is that the API index never eager-loaded `:tags`. Five plans, five `Tag Pluck` queries; add `:tags` to the includes and there are none. Every other list endpoint that reads tags — the libraries organization API, the workspace index, home, search — already preloads them, which is why this only ever showed up here. So: `:tags` joins the index's eager loads, and the comment on tag_names now says which of its two costs a caller is paying, so the next reader doesn't re-derive this the way I just did. The spec is the location one's twin — tag-table query count for thirty plans equals the count for three, plus the assertion that each plan still gets its own tags rather than a neighbour's, since a preload can be cheap and wrong. Both now share the counter as a helper. It fails without the includes: thirty queries against three. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
With the browsable scheme shipped, I was still reading my own work on
/_/plansinstead of the nice URLs. That's the tell: a document with two addresses has one people paste and one people see, and the id form kept winning because it was the one every internal link produced.This finishes the job. A plan lives in one library, at one address, and that address is the readable one.
The readable URL is the only URL
Every plan has a slug.
BackfillPlanSlugsgives one to every plan that predates the scheme, then makes the columnNOT NULL— so "no readable address yet" stops being a state the app has to have an answer for. The slug rules are inlined in the migration, like the folder and handle backfills before it: a migration has to keep producing the same URLs years after the app's rules move on.The id fallback dies.
plan_browse_path,Urls::Canonical.plan_pathandplan_browse_urlalways return the readable path, andrel=canonicalalways renders instead of only when a slug happened to exist.A document's pages hang off the document.
Trimming the tail walks back to the thing being edited — the same property every other prefix in this scheme has. Versions are addressed by the revision number already on screen, not by id. When the tail turns out not to hang off a document —
/sam/notes/history, where "notes" is a folder — the path is put back together and resolved as a place, so a plan someone titled "History" keeps its address./_/plansis gone./plansand/plans/<uuid>stay in the frozen legacy block as 301s, so old links converge on the readable form, query string and all, rather than rendering a second copy of the page. The id-based_scope keeps only the mutations — a button doesn't need a readable URL."All plans" was a scope toggle on a page that no longer exists. The answer to "where is someone else's work" is their library now, so the specs that exercised
scope=allvisit/theirhandleinstead.The API says where a plan is.
plan_jsoncarriesurland the agent instructions mention it, so an agent that creates a plan can hand back the address a human will actually use.A namespace lock per library
Once the readable path is the only path, two plans landing on one address isn't a cosmetic collision — it's a document with no way to be reached.
AssignSlugdecided contention by reading the siblings and then writing, with nothing holding those two together, so two saves a millisecond apart both read "free" and both took it.No unique index can close this. The contest crosses tables (the slug is on
coplan_plans, the level is oncoplan_plan_placements) and it crosses models — a folder and a plan want the same word, and the folder wins, which is whyFolder#disambiguate_shadowed_plansexists. Nothing spanscoplan_foldersandcoplan_plans. The write path is the only place "one segment, one thing" can be decided, so the read it decides from has to be authoritative.Library#lock_namespace!is a row-levelFOR UPDATEon the library row, taken inside the caller's transaction and released on commit. Row-level rather than an advisory lock so MySQL and PostgreSQL behave the same. A library is one person's, so nothing waits on it in practice, and one lock per transaction — always the library being written to — means writers can't deadlock against each other. Calling it outside a transaction raises: there it would release on the next statement and guard nothing while looking like it does.AssignSlugtakes the destination library's lock before reading.Foldertakes it inbefore_save, but only when the slug or parent actually changes — a description edit contests nothing and shouldn't queue behind anything. Held through the shadowed-plan sweep.Plans::Placeis now one transaction with the lock taken first, because a move is one fact: the placement row, the slug that follows from it, the alias that keeps the old address working, and both audit trails either all happen or none do.The part that nearly shipped broken
Adding the lock did not fix the race, and the reason is the interesting part.
Under MySQL's REPEATABLE READ,
FOR UPDATEreads latest-committed only for the tables the query actually scans. The sibling read wasPlan.where(id: <subquery over coplan_plans>), and MySQL evaluates thatIN (SELECT …)as a plain consistent read off the snapshot this transaction took at its first read — which happened before the lock. A full SQL trace with connection ids showed thread B'sFOR UPDATEwaiting ~9ms and returning immediately after thread A's COMMIT, and B still couldn't see A's plan: the lock worked perfectly and guarded a set that was never refreshed.Both sibling reads now keep
coplan_plansin the outer query, where the locking scan can see it. Where the placement goes then splits into two cases, and the split is deliberate:The first shape I pushed used
left_joins(:placement)for the unfiled case, and the PostgreSQL CI job rejected it:FOR UPDATE cannot be applied to the nullable side of an outer join— a hard error, not a slow path. Fixed, and a spec pins the shape, since the suite normally runs on one database at a time and each constraint here is invisible to the other's job.Testing
spec/services/coplan/plans/segment_claims_spec.rbcovers which writes take the lock and which don't, that it refuses to run outside a transaction, the PostgreSQL-safe read shape, and two real two-connection races: one asserting two same-titled plans get two addresses, one asserting each address resolves back to its own plan. Before the fix the race produced two identical paths; after it, one plan takes a suffix and either thread can be the one that moves.spec/requests/browse_spec.rbgains the document sub-pages, including the "a plan titled Edit still owns that segment" case.Full suite: 1790 examples, 0 failures — verified on both MySQL and a locally migrated PostgreSQL, since the locking reads answer to both. RuboCop is unchanged from
main(190 offenses, all in generated schema files).spec/system/deck_ux_spec.rbarrived from #195 mid-review and usedvisit plan_path(plan), which is now the PATCH-only mutation helper; it visits the readable path instead.Note on what isn't here
The original sketch promised a DB unique index on the address. It isn't possible without collapsing a plan's location onto
coplan_plansand dropping the join table — and even then it wouldn't cover the folder-vs-plan half of the contest. The lock is the enforcement; saying so plainly rather than leaving a comment implying an index is coming.🤖 Generated with Claude Code