Give generic_title an automatic consumer, so the nightly stops re-deriving it forever (#765) - #766
Merged
Merged
Conversation
…iving it forever (#765) The consolidation pass reports two live classes and applied only one. generic_title was report-only, so every nightly run re-derived the same finding and nothing was on the other end - a queue whose only consumer would have been a human, which is the shape loopctl#765 and KB 837daaa0 say is a leak rather than a safety gate. Measured in production 2026-08-27: exactly one article matches, id 1dbc8227, titled Untitled, published, 3639 bytes of body. It has been proposed on every report since 2026-08-05 and acted on never. Retitle earns automatic application the way the confirmed-duplicate unpublish does - by being REVERSIBLE, not by being confident. It runs on the SAME gate: two consecutive reports must propose the same article, and the live row is then re-checked rather than trusted, because the corpus moves between the scan and the write. It ABSTAINS rather than invents. A provider error, a reply that is itself a placeholder, an empty body, or a generated title that collides with an existing active title all leave the placeholder alone. A wrong title is worse than no title, and abstention is counted rather than silent. One skip is worth naming because it was found while implementing rather than specced: a CURATED article is left alone. Any title change clears curated_at and curated_by, and putting the title back does not put the governed marker back - re-curation has to go through mark_curated. Reversibility is what licenses this whole step, so where it does not hold the step does not run. BOUNDS, in the worker that just died of getting this wrong. The step costs an outbound call per item, so it is bounded by a wall clock and not by its count cap, and the budget is carved OUT OF the existing job timeout rather than added beside it: the reserve is now prelude plus retitle, the one clamp lowers the judge's ceiling by exactly what this step may spend, and timeout/1 does not move. Raising the job timeout to pay for a new step is the move #761 makes fatal, because the real ceiling is the Lifeline window. The night reports what it was OFFERED and whether the clock truncated it, in the log line and in the knowledge.lint_completed audit event. A truncated night and a night with nothing to do must not read the same, which is how the silent truncation at exactly 100 went unnoticed for three weeks. Every new guard is mutation-verified. Removing the placeholder re-check reddens 8 tests, the curated skip 1, and the two-run agreement reddens the new suite AND the pre-existing duplicate_capture suite - which is the check that the shared-helper refactor did not weaken the class that already worked.
…correct the derived judge figures Re-validate the live row AFTER the provider call, not only before it. The placeholder/curated/visibility check ran against a snapshot taken before an outbound call the provider may hold for 25 s, and the write then landed unconditionally: a human retitling inside that window had their edit completed for them by a machine title, a curation that landed there was silently cleared, the recorded previous title named a placeholder that was already gone, and the pre-call metadata snapshot was written back over anything concurrent - including an agent flipping the article to private. The re-check is now made again against the row as it is, and the write is built from that row. Refuse a generated title that collides with another live article under the pass's own normalization. The unique index is on the raw tenant/title pair, so a case or punctuation variant stores fine and the two are then one duplicate_capture group that the sibling drain unpublishes two nights later. Abstain on a reply naming several articles. The extractor splits content into up to ten articles and saw only the opening 4 KB, so candidate one names the article's first section rather than the article. Say once per run when a tenant has no extraction key. That abstention is permanent, self-inflicted and operator-fixable, and it was folded into the same tag a transient model refusal gets. Check the retitle deadline BEFORE each item. A post-item check is unconditionally committed to item one, so a budget of exactly zero still bought one outbound call and one write out of a reserve already spent. Size the overshoot allowance for receive_timeout times attempts plus backoff, not for a single attempt. Correct two claims the code no longer supports: the previous title on metadata is a convenience an ordinary PATCH erases, not a record outliving the audit log's retention; and the judge is now the last of two steps with a per-item outbound call, not the only one. Recompute every figure derived from the judge's budget, which the new retitle reserve drops from 20 to 18 minutes.
… from skip, and check the judge's clock before the call Six findings from the round-2 review, all in the nightly knowledge-lint pass. Scope the new title-collision guard the way the grouping it prevents is scoped. title_key_taken?/3 filtered only on the raw unique index's active statuses, so a DRAFT row or an agent-private article - neither of which can ever join a title-drift group - permanently vetoed a retitle, and the placeholder was re-offered and re-generated at one provider call a night forever. It now builds on published_base/1 (published composed with shared_only/1), which is the same set title_drift_groups/1 groups over, and runs on heavy_all/2 like every other whole-corpus scan here rather than on the 3-connection AdminRepo pool. Keep abstain and skip distinct across the provider call. revalidate_and_write/3 funnelled both outcomes of the post-call re-read into the skip tally, so an empty body found after the call was counted in a different bucket than the same condition found before it - and moved out of the counter the keyless-tenant warning reads. title_key_taken also gets its own log line: the skip line asserts the article is no longer the confirmed candidate, which is the one thing that skip is not. Check the judge's deadline at the head of each task, not after each result. That is the fix round 1 applied to the retitle twin and left undone here, and it is worse in the judge: async_stream starts judge_concurrency tests before the reducer sees anything, so a zero budget bought several concurrent outbound calls and their terminal dismiss resolutions. The two budget tests move with it. Derive the overshoot allowance instead of writing it down. Both inputs are live-tunable SystemConfig rows with no upper clamp, so a literal stops covering the call it was sized for the moment an operator raises either one. Correct the derived figures the round-1 commit left stale in config.exs, and ceil the burst-recovery night counts as the figures they replaced did - 15,246 over ~1,260/night is thirteen nights, not twelve. The same stale figures in article_linking_worker.ex are on their own branch.
#765) CLAUDE.md's KB-content carve-out described the nightly pass as an allowed system-actor unpublisher and closed with "unpublish is the one write it can undo in code". That sentence was true when it was written and this branch makes it incomplete: the pass now also retitles a confirmed generic_title. Restated so the criterion is the thing a reader carries away rather than the verb. Undoability is what licenses an unattended write - which is also why the retitle declines on a CURATED article, the one case where putting the title back does not put the governed marker back.
…lug with it (#765) Three findings from this branch's own enhanced review, all consequences of the retitle it adds, so they belong in this PR rather than after it. THE UNDO RECORD WAS ERASABLE. write_title recorded the replaced title on metadata, and metadata is cast and whole-map-REPLACED by PATCH on an article - so one ordinary agent request destroyed the record while leaving the retitle standing. Reversibility is the only thing licensing an unattended write, so an erasable record is not good enough. This is the stories.lifecycle_entered_at lesson reached a second time by a second subsystem, and the remedy is the same: a column, written programmatically, castable from nowhere. previous_title is now a nullable text column, written only by Article.retitle_changeset/2 through Knowledge.retitle_article/4. The retitle gets its own changeset and context function rather than a flag on update_article, following the mark_curated and curation_changeset precedent: a field unreachable from the agent-facing edit path gets its own writer, not an option that makes the shared path behave two ways. The shared multi - audit event, webhook, embedding enqueue - is unchanged. The migration backfills from the metadata key and leaves it in place, so a re-run cannot destroy the record it is rescuing. AND THE COLUMN WOULD HAVE BEEN INVISIBLE. article_json serializes a narrow field set that excludes slug and curated_at, while metadata IS serialized - so moving the record to a column would have made it unreadable through the API. A durable record nobody can read is not an undo. previous_title is now on the full read and documented in the show operation, and deliberately not on the list path, where it is null on nearly every article. THE SLUG OUTLIVED THE TITLE. maybe_generate_slug fires only on create, so a retitled article kept its placeholder slug forever - unattended, at scale, on exactly the articles whose slug is a placeholder. Scoped to retitle_changeset and not to update_changeset, because a slug is a URL: rotating it on every human title edit would break standing wiki links to buy nothing. A slug collision inherits the existing unique constraints and comes back as a changeset error, which the caller already treats as a skip. A slug collision also reported the wrong field. unique_constraint on the composite index attributed the violation to tenant_id, its first field, which is the same misleading attribution the title index was already corrected for. Fixed with an explicit error_key on both changesets. THE DRAIN FIGURES this branch invalidated. Carving the retitle reserve out of the same job timeout lowered the judge budget from 20 to 18 minutes, so article_linking_worker's cited 1,400 gross and 900 net are now 1,260 and 760. The threshold on line 79 is the load-bearing one - it is what an operator sizes the unbounded-inflow decision against. Both sites now point at the worker's own budget note as the single place the arithmetic lives. Eight mutations run and killed. The one this branch turns on: adding :previous_title to either cast list reddens "castable from NOWHERE".
This was referenced Aug 27, 2026
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
1 participant
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.
First of loopctl#765. Closes item 2 of that spec, which stands exactly as written.
Why this is a leak and not a safety gate
The consolidation pass reports two live classes (
@live_classes [:duplicate_capture, :generic_title]) and applied only one.generic_titlewas report-only, so every nightly run re-derived the same finding and nothing was on the other end.Measured in production 2026-08-27: exactly one article matches —
1dbc8227, titled "Untitled", published, 3,639 bytes of body. It has been proposed on every report since 2026-08-05 and acted on never. Per the governing constraint (owner decision, KB837daaa0), a queue whose only exit is a human is a defect in the design.What earns it automatic application
Reversibility, not confidence — the same rule the confirmed-duplicate unpublish runs under, and the same gate:
confirmed_proposals/3, extracted from the existing duplicate path rather than copied)metadata.consolidation_previous_titleand the new one markedconsolidation_title_generated, so the write is undoable in practice and a later run can tell its own work from a person'sIt abstains rather than invents. A provider error, a reply that is itself a placeholder, an empty body, or a title colliding with an existing active title all leave the placeholder alone — counted as an abstention, never silent. A wrong title is worse than no title.
One skip was found while implementing, not specced: a curated article is left alone. Any title change clears
curated_at/curated_by(Article.update_changeset/2) and putting the title back does not restore the governed marker — re-curation has to go throughmark_curated/3. Reversibility is what licenses this whole step, so where it does not hold the step does not run.Bounds — in the worker that just died of getting this exactly wrong
The step costs an outbound call per item, so it is bounded by a wall clock, not its count cap. The budget is carved out of the existing job timeout rather than added beside it:
@job_reserve_mssplits into@prelude_reserve_ms(5 min, measured) +@retitle_reserve_ms(2 min, budgeted)timeout/1does not move. Raising the job timeout to pay for a new step is the move KnowledgeLintWorker times out at 600s every night - consolidation pass has been dead since 2026-08-20 #761 makes fatal, because the real ceiling is Oban Lifeline's 30-minuterescue_afterknowledge_consolidation_max_retitles(default 25) resolves from aSystemConfigrow first, so setting it to0pauses the drain fleet-wide without a deploy — the lever to reach for first, since this is the one nightly step that spends a tenant's own provider budget.The night reports what it was offered and whether the clock truncated it, in both the log line and the
knowledge.lint_completedaudit event.Verification
7,905 tests green via the pre-commit hook (credo --strict, dialyzer included). Every new guard mutation-verified by me, not by the implementer's report:
duplicate_capturesuiteThat last row is the one that matters: it proves extracting the shared helper did not weaken the class that already worked.
No new env vars, so
mix loopctl.check_env_docsis unaffected (it still reports the same 60 documented variables). One config key is added,knowledge_consolidation_max_retitles, resolved through the existing SystemConfig-row -> app-config -> module-default ladder; it is a DB key, not an environment variable.No CHANGELOG entry, deliberately. An earlier draft of this branch had one and it was wrong: that file's rule enumerates five categories with "ONLY" — env vars, deploy ordering, migrations with manual steps, breaking API changes, security-relevant storage changes — and this is none of them. The response shape of
GET /knowledge/consolidationis unchanged, and writing a title plus two metadata keys is not security-relevant storage. The strongest case for including it was "new unattended provider spend plus an operator pause lever", which is a behavioural change, and the same rule routes those togit log --first-parent. The operator-relevant facts live in theconfig/config.exscomment directly above the key, which is where someone changing that number will actually be reading.Enhanced review: 15 confirmed, 15 fixed, 0 deferred — then 4 more fixed in-branch
Two review rounds (
f5dd3c7,f91718a) plus266f6ccfor the findings that landed outside the fixer's write scope but are consequences of this change, so they belong here rather than after it.The one that mattered most — a TOCTOU I had read past and praised.
classify_live/1validated a snapshot taken BEFORE a provider call the model may hold for 25s × 2 attempts, then wrote unconditionally against that snapshot. So the curated skip — the guard whose reasoning I singled out as correct — was checked at the wrong moment: amark_curated/3or a human retitle landing in that window was silently clobbered, and the recorded previous title was the wrong one. Now re-validated immediately after the call, with the write built from the live row.Others worth naming:
:duplicate_capturegroup the sibling drain would unpublish two nights laterbudget_ms: 0still bought one full provider call, because the deadline was checked after the item rather than at its headtitle_key_taken?/3carried two distinct defects in one function: it scoped to the raw index instead ofpublished_base/1, so a draft or agent-private row permanently vetoed a shared retitle; and it issued an unindexed whole-corpus scan on AdminRepo's 3-connection pool instead ofheavy_all/2The reversibility hole, fixed in
266f6ccThe undo record lived on
metadata, whichPATCHcasts and whole-map-replaces — so one ordinary agent request erased it while leaving the retitle standing. Reversibility is the only thing licensing this write. That is thestories.lifecycle_entered_atlesson reached a second time by a second subsystem, and it takes the same remedy:previous_titleis now a column, written only byArticle.retitle_changeset/2, castable from nowhere (mutation-verified independently: adding it to either cast list reddens the guard test).And it would have been invisible.
article_json.exserializes a narrow field set that excludesslugandcurated_at, whilemetadatais serialized — so moving the record to a column would have made it unreadable through the API. A durable record nobody can read is not an undo. It is now on the full read and in the:showOpenAPI spec, and deliberately off the list path where it is null on nearly every article.Also in that commit: the slug no longer outlives the title (scoped to the retitle changeset, not
update_changeset/2— a slug is a URL, and rotating it on every human edit would break standing wiki links to buy nothing); a slug collision reported:tenant_idinstead of:slug, the same misleading attribution the title index was already corrected for; andarticle_linking_worker.ex's drain figures, which this branch invalidated by carving the retitle reserve out of the same job timeout (1,400/900 → 1,260/760).publish.inSync: true, 7,922 tests green, migration runs clean on dev and test.