fix: batch embedding generation + dev tooling (SLOPSCAN 9000) #287
Conversation
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
…eanup
The embedding worker returns {"embedding": [...], "model": ...} but
batch_service checked result.get("success"), which was always None —
silently discarding every generated embedding. Fix both call sites
(concept embeddings and ontology embeddings) to check for the actual
"embedding" key.
The batch route constructed BatchService directly instead of using the
get_batch_service() factory, so test mocks patching that factory had
no effect. Fix route to use the factory, and align mock fixtures to
match the real embedding worker return contract.
Add shutdown() to the JobQueue ABC and implement it on PostgreSQLJobQueue
to cleanly stop the ThreadPoolExecutor and close the connection pool.
Call it during app shutdown and detach stream handlers afterward so
draining worker threads don't crash on closed stderr during test teardown.
Fix stale coverage paths in pytest.ini (src -> api/app).
…ctor New tool at scripts/development/lint/todo_inventory.py scans the project for TODO/FIXME/HACK/XXX markers with optional git blame age, JSON output, and marker filtering. The --slop flag enables SLOPSCAN 9000: detection of agent antipatterns including truncated output (...rest of the code), placeholder stubs, AI attribution comments, dummy credentials, and rubber-stamp approvals. Add --json output to docstring_coverage.py for pipeline use with jq and other tools. JSON mode auto-includes staleness data with all status categories (current, stale, unverified, unknown). New Makefile targets: todos, todos-verbose, todos-age, slopscan.
Two-tier severity for staged code files only: - Hard fail (blocks commit): TRUNCATED, STUB - Soft warn (prints, allows): ATTRIB, DUMMY, LGTM Install: git config core.hooksPath .githooks
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.
Summary
batch_service.pycheckedresult.get("success")but the embedding worker never returns that key, silently discarding every generated embeddingBatchServicedirectly instead of usingget_batch_service(), so test patches had no effectJobQueue.shutdown()on the ABC withPostgreSQLJobQueueimplementation that stops the ThreadPoolExecutor and closes the connection pool; stream handler detachment prevents I/O errors during test teardownmake todosscans for TODO/FIXME/HACK/XXX with git blame age, JSON output, and filteringmake slopscandetects agent antipatterns (truncated output, stubs, attribution noise, dummy creds, rubber-stamp approvals)--jsonto docstring coverage — machine-readable output for pipeline use withjq,claude-code, etc.Bug details
The embedding worker (
embedding_worker.py) returns:{"embedding": [...], "model": "...", "provider": "...", "dimensions": 384, "tokens": N}But
batch_service.pycheckedresult.get("success")— a key that never existed. This meant every batch embedding generation silently reported failure while the embedding was actually computed and thrown away.The route at
graph.py:76constructedBatchService(client)inline, so tests patchingget_batch_service()were patching dead code. The mock fixture also used the wrong return format ({"success": True, ...}), which mirrored the production bug — tests passed even though production was broken.Code review findings addressed
shutdown()not onJobQueueABCtimeoutparamunknownstatus in JSONasdictimporttypingimportsTest plan
make test— 978 passed, 23 skipped, 0 failuresmake todos— 42 markers foundmake slopscan— agent antipattern scan runs cleandocstring_coverage.py --json | jq .staleness— includes all status categories