feat(editor): wet zoeken in centraal corpus, promoten naar traject en harvest-fallback via taken#952
Conversation
…w-enrich Nieuw jobtype traject_harvest: haal een wet uit BWB op voor één traject via het taken-mechanisme, zonder de centrale corpus-repo aan te raken. De harvest-worker draait deze jobs vóór de corpus-brede queue, valideert de geharveste YAML met dezelfde validator als law-convert en ketent er expliciet een taak-flow-enrich aan (new_law: het law_convert-patroon) — een directe harvest chain't immers geen enrich. Het kettingstuk is uit finish_law_convert_job getrokken naar chain_enrich_and_complete zodat beide producenten dezelfde keten delen. Terminale fouten en gereapte jobs leveren een job_failed-taak; lopende aanvragen zijn zichtbaar via list_running_task_jobs_for_account.
POST /api/trajects/{ref}/corpus/laws/{law_id}/promote kopieert een wet
uit de centrale-corpus-seed van het traject naar de traject-repo: alle
versie-YAML's (inclusief machine_readable) plus de scenarios/*.feature-
bestanden, in één commit via het bestaande traject-schrijfpad (zelfde
autorisatie en commit-gedrag als save_law). Staat de wet al in de
traject-repo, dan 409 — geen dubbele bestanden. De bron is bewust het
traject-gefedereerde corpus: dat dekt de volledige seed, waar de globale
state in productie alleen favorieten materialiseert.
POST /api/trajects/{ref}/corpus/harvest start met een BWB-id een
traject-scoped harvest via het taken-mechanisme (prioriteit 80, per
(traject, wet) gededupliceerd, onder de bestaande per-account-taakcap
die nu ook traject_harvest meetelt).
…back De "+" onder de wettenlijst wordt een menu met twee routes: zoeken in het centrale corpus (nieuw AddLawPopover) en de bestaande document- upload. Het popover hergebruikt de traject-scoped corpus-zoek-API die ook de bibliotheekzoeker voedt (geen tweede zoekpad): treffers uit de centrale seed krijgen "Toevoegen aan traject" (promote), wetten die al in de traject-repo staan (source_priority 0) zijn uitgeschakeld met "Al in dit traject", en zonder treffer start een BWB-id (direct getypt of via de wetten.overheid.nl-zoeker) een traject-scoped harvest via het taken-mechanisme. Het takenpaneel toont de lopende aanvraag als "Wet ophalen loopt"; het resultaat komt als law_create-review-taak terug langs het bestaande reviewpad.
…liceert de BWB-rij Review-fixes op de wet-toevoegen-flow: - promote_corpus_law overschreef een scenario-file die al in de traject-repo stond stilletjes met de seed-versie. save_scenario routeert scenario's van gefedereerde wetten naar de writable-own zonder dat er een wet-YAML in de traject-repo staat, dus zo'n traject-edit ontsnapte aan beide 409-checks. Bestaande bestanden worden nu overgeslagen (traject-edit wint); voor wet-YAML's blijft het een 409. Regressietest toegevoegd. - AddLawPopover toonde twee rijen voor hetzelfde BWB-id zodra de externe zoeker het getypte id ook vond; de directe rij verdwijnt nu zodra het externe resultaat er is. De begeleidende tekst claimde bovendien 'Niet in het centrale corpus' terwijl de corpus-zoeker niet op BWB-id matcht — neutraal geformuleerd.
|
Review-ronde afgerond (volledige diff + omliggende code, checks lokaal gedraaid). Fixes gepusht in 0675278. Gevonden → gefixt
Beoordeeld en bewust gelaten
Checks: clippy alle packages groen; |
|
Ship-pipeline afgerond: PR uit draft gehaald, de CI-getriggerde claude-review draaide op HEAD 0675278 en leverde geen bevindingen op, en alle CI-checks op de laatste commit zijn groen. Klaar voor beoordeling. |
…taten Een zoektreffer die wél in het centrale corpus zit maar nog niet in de eigen traject-repo krijgt nu direct een "Toevoegen aan traject"-knop in de gewone zoeker (SearchPopover) — de aparte "Wet toevoegen"-flow via het plusmenu is niet langer de enige route. De promote-logica (POST /promote, per-wet busy-state, al-in-traject-set op source_priority 0, 409-afhandeling) is uit AddLawPopover losgetrokken naar een gedeelde composable useLawPromote; beide flows gebruiken dezelfde implementatie. Na een geslaagde promote sluit de popover en emit hij "promoted" met dezelfde deferral als select-law; LibraryView ververst de bronnenlijst en opent de wet, EditorView ververst de gedeelde corpus-lijst en navigeert naar de bibliotheek. Buiten een traject (globale corpus- weergave) verschijnt de knop niet.
|
Revisie n.a.v. feedback op het ticket: de "Toevoegen aan traject"-knop zit nu ook direct in de gewone zoekresultaten — een treffer die wél in het centrale corpus zit maar nog niet in de eigen traject-repo krijgt de knop meteen, zonder eerst de "Wet toevoegen"-flow via het plusmenu te openen (die flow blijft bestaan voor de harvest-fallback). Wat er wijzigde (commit 352ebc4):
Geen custom CSS toegevoegd: uitsluitend bestaande ndd-componenten ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Reviewed the diff end-to-end: promote_corpus_law + collect_promote_files (editor-api), request_traject_harvest (task_requests.rs), the new traject_harvest pipeline module + worker integration, migration 0031, and the frontend (AddLawPopover.vue, useLawPromote.js, SearchPopover.vue, TasksPane.vue).
No blocking issues found. Specifically checked and confirmed sound:
- Auth: both new routes (
/promote,/corpus/harvest) sit under the existingaccount_middleware+require_role("editor-writer")route_layer group inmain.rs, matching the PR description. - Race safety on promote:
resolve_traject_documents_writertakes an owned lock on the writable-own backend before the per-file existence check, and that lock is held through the write+persist. Two concurrent promotes for the same law correctly serialize — the second sees the just-written files and 409s.collect_promote_files(which reads from seed backends) intentionally runs before the lock is acquired, per the documented writable-own → seed lock-order invariant. - "Already in traject" guard correctness:
source_map's single-entryget_law()always prefers the lowestsource_priority(own repo = 0 beats seed), so checkingget_law(law_id).source_id == writable_own_source_idcorrectly detects "already promoted" regardless of which source has the in-force version — verified againstSourceMap::insert's conflict resolution. - Version/file collection:
get_law_versionsis documented and implemented as newest-first, soversions[0](used for both the scenario directory andrecord_save's "newest" pick) is correct. - Path safety: seed-sourced
relative_pathvalues are checked for..and absoluteness before being reused as traject-repo write paths. - Dedup/cap SQL: the new
traject_harvestadvisory-lock + dedup query and the job-cap query are parameterized (no injection), and mirror the existingharvest_request/enforce_task_job_cappattern exactly. - Exhaustiveness: all
match JobType { ... }sites (law_status.rs,tasks.rs) got explicit new arms forTrajectHarvest; no wildcard arm silently swallows it. String-literal job-type IN-lists were checked individually —document_convert.rs's upload-GC list correctly excludestraject_harvest(it has no upload row), while the task-list/job-cap queries correctly include it. - Deterministic-vs-transient error classification in
process_next_traject_harvest_jobuses ad hoc string matching (msg.contains("valideert niet tegen het schema"),"er loopt al een verrijking") rather than the existingis_deterministic_content_failurehelper — but that helper targets a different error domain (enrich/Yaml errors), so this isn't duplicated logic, and the matched substrings do correspond to the actualPipelineError::Enrich/harvester error text. - Frontend: no
v-html/raw HTML injection surface (all custom-element bindings via:text), no custom CSS added (consistent with the PR's own criterion-7 claim), icon aliases (harvest) used correctly perCLAUDE.md.
Test coverage (Rust integration tests for promote 409/404/scenario-preservation, harvest dedup/400, Vitest for both popovers) looks proportionate to the risk surface, including the trickier case (a traject-edited scenario surviving a promote of the parent law).
Nothing else stood out as in-scope for this diff.
…ens de promote-POST Sluit de gebruiker de zoekpopover terwijl de promote-POST nog loopt, dan resolvet die daarna alsnog 'done' en bleef pendingPromotedLawId hangen: close() op een al gesloten nldd-popover is een no-op (geen 'close'-event), dus de eerstvolgende ongerelateerde close emitte een stale 'promoted' en verdrong daarbij ook een legitieme select-law. onPopoverOpen reset nu de pending emits van een vorige sessie; regressietest toegevoegd. Daarnaast focust LibraryView na een promote uit de gewone zoekresultaten nu ook het sidebar-item (focusAfter), net als select-law uit dezelfde popover - dat is precies waarvoor de 'promoted'-deferral bestaat. De AddLawPopover-route blijft ongewijzigd (geen focus).
There was a problem hiding this comment.
Correctness / Security
🔴 Critical — unsanitized traject_ref reaches remove_dir_all/create_dir_all on the harvest worker
packages/pipeline/src/traject_harvest.rs, execute_traject_harvest:
let work_dir = std::env::temp_dir().join(format!(
"trajectharvest-{}-{}",
payload.traject_ref, payload.bwb_id
));
let _ = tokio::fs::remove_dir_all(&work_dir).await;
tokio::fs::create_dir_all(&work_dir).await?;
...
let _ = tokio::fs::remove_dir_all(&work_dir).await;payload.traject_ref is the raw {traject_ref} URL path segment from POST /api/trajects/{traject_ref}/corpus/harvest, carried unmodified from task_requests::request_traject_harvest into the job payload and from there into this worker path. It is not validated for path-unsafe characters before being embedded in a filesystem path:
resolve_traject_ref(packages/editor-api/src/trajects.rs) only validates the trailing 8-hex-char suffix of the ref (used for the DB lookup); the "slug" prefix is explicitly documented as "cosmetic" and accepts any ASCII content, including/and...bwb_idis tightly validated (normalize_bwb_id:BWB+ alnum only), so that half is safe — buttraject_refgets no equivalent treatment.- axum's
Path<String>extractor percent-decodes the matched segment after routing has split on the raw (undecoded) path, so a percent-encoded slash (%2F) in the URL survives routing and becomes a literal/in the extractedtraject_refstring, at which point it is a real path-traversal-capable value.
Since a valid ref only needs to end in the 8 hex characters of a traject the caller is a member of, an authenticated member can freely choose the rest of the string. Combined with remove_dir_all being called on the resulting path (both before and after the harvest), this lets an authenticated traject member cause the worker to recursively delete a directory of their choosing on the harvest-worker host (any directory the worker process can write to), not just its own temp scratch dir.
The sibling implementations this code explicitly mirrors (document_convert, law_convert) avoid this entirely by keying their temp directory off a server-generated UUID (upload_id), not user/URL-supplied text:
std::env::temp_dir().join(format!("docconvert-{}", payload.upload_id))
std::env::temp_dir().join(format!("lawconvert-{}", payload.upload_id))TrajectHarvestPayload already carries traject_id: Uuid — swapping payload.traject_ref for payload.traject_id in the work_dir format string removes the vulnerability with no behavior change, consistent with the existing pattern.
Code quality
🟡 Minor — fragile string-matching for retry-vs-terminal classification
packages/pipeline/src/worker.rs, process_next_traject_harvest_job:
let deterministic = matches!(
e,
PipelineError::Harvester(regelrecht_harvester::HarvesterError::NoConsolidatedText { .. })
) || msg.contains("valideert niet tegen het schema")
|| msg.contains("er loopt al een verrijking");Unlike the NoConsolidatedText arm (matched on the typed error variant), the other two branches classify by matching substrings of the formatted, Dutch, user-facing error message produced elsewhere (traject_harvest.rs's schema-validation error and law_convert::chain_enrich_and_complete's dedup error). A future wording change to either message silently breaks this classification (a deterministic failure would then retry 3× instead of failing fast, or vice versa) with no compiler signal. Not incorrect today, but worth a typed error variant or error code instead of string matching if this is expected to be maintained going forward.
No other blocking issues found. The promote endpoint's write path is correctly serialized through the writable-own backend's mutex (resolve_traject_documents_writer → lock_owned()), so I don't think concurrent promote requests for the same law are actually racy despite there being no explicit advisory lock (unlike the harvest-request dedup) — the per-file existence check plus the held mutex through persist() covers it.
…niet op de ref De work-dir van execute_traject_harvest gebruikte payload.traject_ref - een URL-pad-segment waarvan alleen het 8-hex-suffix gevalideerd wordt. Het slug-deel is vrije ASCII-tekst en een percent-encoded '/' overleeft axum's routing, waarna de ref als path-traversal-waarde in een pad terechtkwam dat door remove_dir_all/create_dir_all gaat. Nu keyt de work-dir op het server-gegenereerde payload.traject_id (Uuid), hetzelfde patroon als docconvert/lawconvert (upload_id). Bijvangst uit dezelfde review-ronde: de deterministisch-vs-transiënt- classificatie in process_next_traject_harvest_job matchte op losse string-literals van foutteksten elders; die markers zijn nu gedeelde consts (SCHEMA_MISMATCH_MARKER, ENRICH_IN_PROGRESS_MARKER) zodat een herformulering de classificatie niet stilletjes kan breken.
De snapshot-refresh is stale-while-revalidate: een request na de TTL serveert de stale snapshot en start de her-enumeratie in een gespawnde background-task. De test asserteerde in één shot direct na de upstream-writes, maar een refresh die door een eerdere request gestart is kan de bronnen net vóór die writes enumereren - dan serveert de volgende request een verse snapshot zónder de nieuwe wet en faalde de test op task-scheduling (flaky; zo op CI gefaald op een commit die editor-api niet raakt). Poll nu bounded (max 5s) tot een request de post-write snapshot geserveerd krijgt; zelfde aanpak voor de convergentie-assert. De asserties zelf zijn ongewijzigd.
|
Review-ronde over de revise-commit 352ebc4 afgerond (diff in volle context, frontend-suite lokaal gedraaid, CI-review verwerkt). Gevonden → gefixt
Gecheckt, geen bevinding: gedragsbehoud van de Frontend 586/586 groen, pipeline/editor-api-tests lokaal groen, CI volledig groen op 68837e1. |
…ice-token — promote 403-fix
Op de pr952-preview faalde POST …/promote met 403 "Source is read-only"
(traject duo-46921d4d, git-backed met subpath). Rootcause, bevestigd in de
deployment-logs ("traject writable-own source resolved NO token — push
will fail"): het traject is aangemaakt op een user-gekozen repo, dus er
bestaat bewust géén CORPUS_AUTH_*-service-token voor die repo
(fail-closed). De writability-gate keek uitsluitend naar het rest-token
(BackendEntry.writable), terwijl de deployment in de
user-token-schrijfmodus staat (github.user_oauth aan): elke write hoort
daar via het gekoppelde GitHub-token van de acterende gebruiker te lopen
(WriteContext::token_override). Elke traject-write — promote én
save_law/save_scenario/documents — 403'de dus op deze config; de lokale
tests dekten alleen local-source writable-owns.
- corpus_handlers: één gedeelde gate (require_traject_backend_writable):
read-only-at-rest mag door wanneer de backend token_override
ondersteunt én de deployment in de user-token-modus staat; daarna
handhaaft user_write_token_for_backend het gekoppelde token (428).
- github_oauth: user_token_write_mode helper (write_requires_user_token,
tolerant voor een niet-geconfigureerde OAuth-integratie).
- GitHubApiBackend: write_file/delete_file bufferen zonder rest-token
(de token-guard leeft nu in persist: ReadOnly zonder énig token);
persist bootstrapt de traject-branch lazy met het effectieve token
(ensure_branch gedeeld met ensure_ready) — zonder rest-token maakt
ensure_ready de branch immers niet aan.
- GitHubFetcher: GITHUB_API_BASE-override (test-seam voor wiremock diep
in build_traject_corpus; tevens GHES-seam). Productie laat 'm ongezet.
- Integratietest promote_user_token_write_test: deployed-achtige config
(lokale read-seed + GitHub writable-own met gh_path-subpath en zonder
token-env, user-token-modus aan, wiremock als GitHub). Zonder de fix
faalt hij met exact deze 403; met de fix: 428 zonder gekoppeld token,
promote kopieert de wet-map met het user-token onder de subpath, en
een law-save volgt aantoonbaar hetzelfde schrijfpad.
…d-route in de popover
Tims feedback: de plus in het linkermenu opende eerst een dropdown-menu —
die tussenstap is weg. De "+" triggert nu direct de AddLawPopover
(zoeken in het centrale corpus, promoten, harvest-fallback). Het tweede
menu-item ("PDF of DOCX uploaden…", de conversie-naar-wet-keten) is
niet stilletjes verdwenen maar verhuisd naar de popover als
secundaire-knop onder de zoekresultaten; de popover sluit vóór de emit
zodat de file-picker (die in LibraryView leeft) niet achter het popover
opent. Vitest dekt de nieuwe route (emit + sluiten).
|
Rework verwerkt (2 punten): 1. Promote-403 op de preview gefixt (f5765f1). Rootcause — bevestigd in de pr952-deployment-logs ( 2. Plus-knop zonder dropdown (5012573). De "+" in het linkermenu opent nu direct de "Wet toevoegen"-zoeker. Het tweede menu-item (PDF/DOCX-upload → conversieketen) is verhuisd naar de popover als secundaire knop onder de zoekresultaten. Checks: Kanttekening voor de preview: het ontbrekende service-token betekent ook dat reads van de private traject-repo daar blind blijven (index/werkdocumenten/dedup-check zien de repo niet — pre-existing, ook op main). Schrijven werkt nu wél, als de gebruiker; voor volledige functionaliteit blijft een read-token ( |
ReviewWent through the full diff: the No Critical or Significant issues found. Specifically checked and found sound:
Minor: none worth flagging beyond the above — the tricky bits (409 dedup, deferred-emit-after-close focus handling, stale-pending-emit reset, TOCTOU on branch creation) all have direct test coverage. |
…can-falen per source (#953) * fix(editor): traject-eigen indexscan resolvet token strikt en meldt scan-falen per source De promote-flow (#952) schrijft een wet met het user-OAuth-token naar de traject-repo, maar de server-side indexscan van de writable-own source las met een ánder token-resolutiepad: 'resolve_token_for_source' mét legacy 'CORPUS_GIT_TOKEN'-fallback, waar push/backend strikt resolven. Zonder geconfigureerd per-repo token scant de server een privé-repo dan unauthenticated, krijgt 404 van de Trees-API, en valt de wet stil terug op het centrale corpus — de traject-source toont alleen 'law_count: 0'. - 'Source.strict_auth' (gezet voor writable-own rows) laat élk token-resolutiepad — indexscan, favorites-fetch, backendconstructie — dezelfde strikte regels volgen via 'auth::resolve_source_token'. Dit sluit ook het leespad-lek waarbij de legacy-fallback het centrale token naar een user-gekozen repo zou sturen. - 'index_all_sources_async' geeft per gefaalde source de fout terug ('SourceIndexFailure'); 'build_traject_corpus' bewaart die in 'CorpusState.index_failures' en GET /api/trajects/{ref}/sources (en /api/sources) tonen ze als 'index_error' per source — scan-falen is niet langer stil. - error!-log bij een gefaalde writable-own-scan draagt nu de fout mee; de bestaande 'NO token'-diagnostiek benoemt naast pushes ook reads. - Integratietest reproduceert het prod-symptoom (promote slaagt, onleesbare repo → fallback naar seed met priority 2 + index_error) en pint het happy path: gepromote wet zichtbaar op source_priority 0. * build(deps): bump brace-expansion naar 2.1.2 voor GHSA-3jxr-9vmj-r5cp
1 participant
Closes tdjager/development#4
Wat
Vanuit een traject een wet toevoegen in één flow:
AddLawPopover) dat de bestaande traject-scoped corpus-zoek-API hergebruikt (dezelfde die de bibliotheekzoeker voedt — geen tweede zoekpad). Een BWB-id mag ook, direct getypt of via de wetten.overheid.nl-fallback.POST /api/trajects/{ref}/corpus/laws/{law_id}/promotekopieert de volledige wet-map uit de centrale-corpus-seed naar de traject-repo: alle versie-YAML's inclusiefmachine_readable, plus descenarios/*.feature-bestanden, in één commit via het bestaande traject-schrijfpad (zelfde autorisatie/commit-gedrag alssave_law, inclusief user-token-override). Staat de wet al in de traject-repo (source_priority 0), dan is de knop uitgeschakeld en weigert de backend met 409 — geen dubbele bestanden.POST /api/trajects/{ref}/corpus/harveststart met een BWB-id een traject-scoped harvest via het taken-mechanisme: nieuw jobtypetraject_harvest(migratie 0031) op de harvest-worker, dat de geharveste basis-wet valideert en expliciet een taak-flow-enrich ketent via het uitlaw_convertgedeeldechain_enrich_and_complete(een directe harvest chain't immers geen enrich). De aanvraag is direct zichtbaar in het takenpaneel ("Wet ophalen loopt – BWBR…"); het resultaat komt terug alslaw_create-review-taak en goedkeuren landt via het gewone law-create-pad (create_traject_law).Waarom de traject-seed en niet de globale corpus-state
De promote leest uit het traject-gefedereerde corpus (de
minbzk-central-seed): die index dekt de volledige centrale corpus (metadata-only, lazy bodies), terwijl de globale state in productie alleen favorieten materialiseert (load_favorites_async). Dit is ook exact de write-routing diesave_lawvoor gefedereerde wetten gebruikt, dus paden blijven per constructie consistent.Guards
editor-writer-rol op beide endpoints (bestaande route-middleware).traject_harvestnu mee.traject_harvest-job levert alsnog eenjob_failed-taak.Tests
packages/editor-api/tests/promote_law_test.rs— kopieert versies + scenario's byte-voor-byte, 409 bij herhaalde promote, 404 bij onbekende wet (testcontainers).packages/editor-api/tests/traject_harvest_request_test.rs— 202 + jobinhoud (deliver=task, prio 80), dedup-409, 400 op ongeldig id.TrajectHarvestPayload-serde, taakcap/reaper-titels.AddLawPopover.test.js(zoekpad, sortering, al-in-traject, promote/409, BWB-fallback, harvest/409, direct BWB-id) enTasksPane.test.js(nieuw running-label). Volledige suite: 57 files / 579 tests groen.just checkis groen (de drieuntranslatables-lib-tests vereisen lokaalTESTCONTAINERS_HOST_OVERRIDE, zoals gedocumenteerd intest_utils.rs); ookeditor-api-testenpipeline-integration-testdraaien groen.Custom CSS (criterium 7)
Geen.
AddLawPopoveris volledig opgebouwd uitnldd-*-componenten (zelfde popover/listbox-patroon alsSearchPopover) en heeft geen<style>-blok; ook inLibraryViewis geen styling toegevoegd.Buiten scope (conform ticket)
Geen sync/versie-drift van gepromote wetten, geen recursieve harvest van gedelegeerde regelingen, geen bulk-promote, geen harvester-admin-wijzigingen.