Sara's cycle-2 S2: clarify the latent (but not actual) cross-test state
risk on the two metrics tests that hit the global REGISTRY instead of
the per-test fresh_metrics fixture. Migrating them would actually break
them — the /metrics endpoint is served by prometheus-fastapi-instrumentator
which binds to the default REGISTRY at app-construction time, and the
http_requests_total assertion only finds counters on that global
registry. Both tests already assert response shape only (status code,
content-type substring, body substrings), not numeric values, so the
shared-registry caveat is documented for future readers rather than
treated as a bug to fix.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sara's cycle-2 S1: the wall-clock assertion at < 0.05s could trip on a
slow CI runner under load even when the timer correctly excludes
spell-check. Sara's preferred structural fix (patch main.time.monotonic
with a deterministic sequence) proved awkward — the patched attribute is
the *global* time.monotonic which httpx and asyncio consume, exhausting
the sequence before the request reaches the engine loop.
Take the documented fallback: widen the bound to 0.09s and explain why.
The failure mode the test guards against (spell-check inside the timer)
would add 0.1s (2 × 0.05s sleep), so 0.09s catches the bug while leaving
~90ms of headroom for slow CI runners. Verified red→green by temporarily
moving correct_text inside the timer block: bound trips at 0.101s; the
fixed code reads ~0.001s.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ADR-023 captures why prometheus-fastapi-instrumentator was chosen,
the build_metrics(registry) factory pattern, and the test rebinding
seam. The glossary gains four ops-aligned terms — illegible word,
models-ready gauge, recognition vs segmentation accuracy — so the
metrics documentation in OBSERVABILITY.md has a vocabulary to lean on.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- L2 container diagram now shows the Prometheus -> ocr:8000 scrape edge
(plus the previously-undrawn Prometheus -> backend edge for symmetry).
- OBSERVABILITY.md gains a full ocr_* metrics table with labels, units,
and the canonical example queries from issue #652.
- New "Internal-only endpoints" subsection captures the unauthenticated
/metrics caveat and provides the Caddy block snippet for the case
where the service ever gets a host port.
- Explicit note that MetricsPathFilter only quiets uvicorn stdout, and
the OCR metrics must never carry PII or document content.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three small drops that pay back later:
- Note that main.metrics is import-time bound and tests must
monkeypatch `main.metrics`, not the registry.
- Flag the /metrics endpoint as unauthenticated and cross-link the
Caddy-block snippet in docs/OBSERVABILITY.md.
- Pin prometheus-client to the exact 0.25.0 patch version already
resolved by prometheus-fastapi-instrumentator 7.0.0, so an upstream
bump cannot silently slip in.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Each metrics test was repeating the same five-line block — patch
kraken_engine.load_models, patch load_spell_checker, instantiate the
AsyncClient, force _models_ready True, restore it. Lift the lot into a
single async context manager so each test body shrinks to its real
arrange / act / assert intent.
Tests that drive the lifespan directly (models_ready gauge) or stub
asyncio.to_thread for /train (which already patches _models_ready) stay
unchanged.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The /train, /train-sender, and /segtrain endpoints each duplicated the
same eight-line try/except + counter + gauge block around the
asyncio.to_thread call. Lift it into _record_training(runner, kind),
which accepts a sync- or async-returning callable for flexibility.
Each endpoint now ends with a single return line. Behaviour preserved —
status codes, error propagation, and metric labels stay identical.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The two block-iteration loops (/ocr and /ocr/stream's standard generator)
both ran the same word-total and illegible-word increments. Lift them
into a single helper so each call site becomes one line and the counter
intent reads cleanly. Pure refactor — no behavior change, tests stay green.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The asyncio.to_thread patch stubbed out the entire _run_training call,
hiding the real error path. Replacing it with a failing CompletedProcess
from subprocess.run exercises the actual ketos-failed branch and keeps
the test's intent — error counter bumps, 500 surfaces — intact.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If uvicorn's access log format ever changes (args=None, or shorter
than 3 elements), the filter must keep forwarding records rather than
silently dropping them. Two extra LogRecords cover both edge cases.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two regression tests:
- /train-sender hitting the success path bumps the recognition counter
(previously only /train and /segtrain were covered).
- A successful run whose result.accuracy is None must not call set() on
ocr_model_accuracy — the gauge stays at its default 0.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the guided generator's page_started timer wrapped the entire
region loop including the synchronous correct_text() call, inflating
ocr_processing_seconds with spell-check latency. Sum the per-region
engine.extract_region_text durations instead so the histogram matches
the unguided stream's "engine only" semantic.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Locks in the post-download placement of the counter increment so a
regression that moves it back above _download_and_convert_pdf would fail.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The TODO was a placeholder for this work — the OCR service now exposes
/metrics so the target will flip from DOWN to UP on next image rebuild.
Refs #652
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a logging.Filter on uvicorn.access that drops records whose request
path is /metrics or /health. Each is hit on a tight schedule (Prometheus
scrape interval and Docker healthcheck), so unfiltered they dominate the
access log without carrying any information about real traffic.
Refs #652 (Nora's recommendation)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors the existing _models_ready bool so Prometheus has a time-series
liveness/readiness signal for future alerting rules (e.g.
ocr_models_ready < 1 for 2m).
Refs #652 (AC7)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hits /train then /segtrain through the same test, each with a distinct
mocked accuracy, and asserts the labelled gauges reflect the two values.
Locks down the kind-label separation between recognition and segmentation
accuracy (decision #2).
Refs #652 (AC6)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wraps the await asyncio.to_thread(_run_*) calls in /train, /train-sender,
and /segtrain with try/except. Recognition training (/train, /train-sender)
shares kind="recognition"; /segtrain uses kind="segmentation". The
ocr_model_accuracy gauge is set per kind on success.
Refs #652 (AC6, decision #2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wraps every asyncio.to_thread(engine.extract_*) call with time.monotonic()
deltas in /ocr (per document) and in both /ocr/stream generators (per page).
Streaming buckets are the useful operational signal; the non-streaming
observation is a bonus.
Refs #652 (AC5)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Walks block["words"] before apply_confidence_markers strips the list, then
increments ocr_words_total by len(words) and ocr_illegible_words_total by
the count below threshold. Same pattern in both /ocr and /ocr/stream so the
ratio illegible/words is a faithful quality signal across endpoints.
Refs #652 (AC4)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bumps the counter in both /ocr/stream except blocks (standard and guided
generators) so the existing skipped_pages local variable now also flows
into Prometheus.
Refs #652 (AC3b)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bumps the counter inside both the standard and guided /ocr/stream
generators after a page yields its blocks, before the per-page json line is
emitted. Also moves the ocr_jobs_total increment for /ocr/stream right after
engine selection so the counter still fires when a page later errors out.
Refs #652 (AC3a)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Locks down AC2 for the non-Kurrent path. The same code branch in /ocr that
sets engine_name from script_type now has explicit coverage for both
HANDWRITING_KURRENT → kraken and TYPEWRITER → surya.
Refs #652 (AC2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pick engine="kraken" for HANDWRITING_KURRENT, engine="surya" otherwise,
then increment after the blocks have been extracted.
Refs #652 (AC2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Encapsulates every custom OCR metric in an OcrMetrics frozen dataclass and
exposes a `build_metrics(registry)` factory. Production main.py binds against
the default REGISTRY; tests construct a fresh CollectorRegistry per case and
monkeypatch main.metrics, so counter values stay isolated between tests
(decision #3 on issue #652, Option A).
Refs #652
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Locks down AC1: prometheus-fastapi-instrumentator must keep auto-exposing
http_requests_total and http_request_duration_seconds for application
traffic, not just register the /metrics endpoint.
Refs #652
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mount the instrumentator immediately after FastAPI app creation, excluding
/health and /metrics from request metrics to keep http_requests_total focused
on real application traffic.
Refs #652
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A symlink placed inside importDir pointing to a file outside it would pass
isValidImportFilename (no forbidden chars in the symlink name) and be found
by Files.walk. Now checks candidate.getCanonicalPath() against
baseDir.getCanonicalPath() — if the resolved path escapes importDir,
throws DomainException.internal and aborts the import. Adds regression
test using @TempDir + Files.createSymbolicLink.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces MassImportService.SkipReason with all five values —
INVALID_FILENAME_PATH_TRAVERSAL, INVALID_PDF_SIGNATURE, FILE_READ_ERROR,
ALREADY_EXISTS, S3_UPLOAD_FAILED — making the full set of reasons greppable
and type-safe. SkippedFile.reason changes from String to SkipReason;
importSingleDocument return type updated accordingly. JSON serialisation
is unchanged (Jackson serialises enums by name). All tests updated.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents that .hidden.pdf and "Brief an Oma.pdf" correctly pass the
isValidImportFilename guard — both are valid basenames common in the archive.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds checks for U+2215 DIVISION SLASH (∕), U+FF0F FULLWIDTH SOLIDUS (／),
and U+29F5 REVERSE SOLIDUS OPERATOR (⧵) — all of which bypass the existing
ASCII separator checks on Linux path resolution. Adds a clarifying comment on
the Paths.get().isAbsolute() call explaining its InvalidPathException safety
boundary. Adds 3 regression tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rejects path-traversal filenames before findFileRecursive runs.
Guard runs on the derived filename (after the ternary) as specified.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codifies the path-traversal constraint that was previously safe by
accident (findFileRecursive's getFileName() strip) but had no explicit
guard or test coverage. Fixes issue #530.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All route spec files that mock $lib/shared/api.server were missing
extractErrorCode from the mock factory, causing a vitest "No export defined"
error after the refactor introduced the new export.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Round-4 polish from Felix (#1): SEARCH_RESULT_LIMIT only has one consumer
today (PersonMentionEditor), so it risked masquerading as shared. Add a
one-line rationale that the symmetry with MAX_QUERY_LENGTH and
SEARCH_DEBOUNCE_MS — keeping all @mention knobs in one file — is the
intentional motivation, not a missed inlining.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-4 polish from Leonie (S-2), Felix (#3), Sara (#4):
- Add aria-hidden="true" to the visible empty-state <p> so VoiceOver does
not double-announce — the persistent sr-only live region is now the
sole AT source of truth (NVDA already de-duped, VoiceOver did not).
- Extract `searchQuery.trim() === ''` into an `isQueryEmpty` $derived;
both the announcer branch and the visible empty-state branch now read
from the single intent-named alias.
- Cover the singular branch of the persistent live region (1 item ->
"1 Person gefunden" / "1 person found" / "1 persona encontrada").
Plural was already covered; this closes the missing-branch gap.
- Extend the existing "no aria-live on visible <p>" test to also assert
aria-hidden="true" so a regression on the AT-source-of-truth contract
goes red immediately.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round-4 polish from Sara (#11199) and Felix (#11186):
- Replace setTimeout(50) in stale-response race with tick() — matches
round-3 pattern Sara verified in the sticky-takeover test.
- Add intent comment above the "clear input" wait — it is a negative
assertion that must not be optimised away.
- Tighten displayName-clip assert from <=100 to ===100 so the test
discriminates "clip works" from "clip works AND nothing weakened it".
- JSDoc POST_DEBOUNCE_SLACK_MS with the calibration rationale.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add ESLint no-restricted-imports rule banning *.test-fixture.svelte from
non-test files. Tree-shaking already keeps test fixtures out of the
production bundle, but making the boundary lint-enforced catches an
accidental autocomplete-driven import in a route or component. Test
files and the fixtures themselves are exempt. Nora #2 on PR #629
round 3.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The aria-live region previously lived inside {#if items.length === 0} so
it remounted whenever items transitioned between empty and populated —
VoiceOver in particular swallows announcements from freshly-mounted live
regions, and the "N persons found" announcement was missing entirely on
the populated branch. Move the live region above the conditional so the
element persists, and announce a localized "1 person found" / "N persons
found" count on the populated branch. The visible empty-state <p> stays
as a visual cue (no aria-live). Leonie #3 on PR #629 round 3.
Adds person_mention_results_count_singular / _plural in de/en/es.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
de + es already use singular ("Person suchen", "Buscar persona"); en
was plural ("Search persons"). Switch en to "Search for a person" so
all three locales announce a singular search control to screen-reader
users — cross-locale parity polish. Leonie #1 on PR #629 round 3.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wrap all four onKeyDown unit tests (ArrowDown/ArrowUp/Enter/Escape) in
flushSync uniformly so the next reader doesn't have to figure out why
some are wrapped and others aren't. Felix #1 on PR #629 round 3.
Also add a comment above the describe block calling out that these unit
tests do NOT exercise the Tiptap forwarding chain — that is covered by
the 'ArrowDown moves the highlight' integration test. Sara #3 on PR #629
round 3.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round 2 renamed only MentionDropdown's fixture; three siblings retained
the old suffix. Rename PersonMentionEditor, confirm, and TranscriptionBlock
test hosts to the .test-fixture suffix and update the three importers so
the boundary is uniform across the repo. Felix #1 / Tobi #1 on PR #629.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sara on PR #629 round 3: the magic 50 ms in the @mention sticky-takeover
test was anchored to nothing and read as a race-fix it wasn't. Replace
with await tick() so the intent ("flush pending Svelte reactivity") is
explicit. The expect.element polling already covers timing drift.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sara on PR #629 round 3: the round-2 fix captured the fetch count AFTER
typing '@', so a regression that re-introduced the legacy per-keystroke
items() callback would have its '@'-keystroke fetch silently absorbed
into the baseline. Drop the baseline subtraction and count every
/api/persons fetch since render — typing '@' + fill('Walter') must
total exactly one fetch.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The dropdown's editor-mirror clips at 100 chars (CWE-400, Nora #1), but
the host editor previously fed renderProps.query directly to displayName
on selection — so a 200-char @-suffix would search the first 100 chars
but insert 200 chars. Clip once in updateState and use the clipped value
for both the inserted displayName and the dropdown's editorQuery mirror,
keeping "what I searched" and "what got inserted" in sync. Felix #3 on
PR #629.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Single source of truth for MAX_QUERY_LENGTH, SEARCH_DEBOUNCE_MS, and
SEARCH_RESULT_LIMIT — MentionDropdown imports MAX_QUERY_LENGTH;
PersonMentionEditor imports the debounce + result-limit; the spec's
mirror now imports SEARCH_DEBOUNCE_MS so it can never drift. Unblocks
the displayName length-cap fix (Felix #3 on PR #629).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
