Commit Β·
411e20a
1
Parent(s): 90e80f9
[NOTICKET] docs: update PROGRESS.md tracker with TAB review items (T1 done, R11 TAB slice, T2 blocked)
Browse filesLog the TAB-owned review work in the fix tracker:
- R11 [ ]->[~]: canonical DATA_ACCESS_TOOLS now lives once in tools/data_access.py, invoker.py imports it (90e80f9). Agent remainder (planner registry derive + _PLACEHOLDER_RE dedup) noted.
- T1 [x]: input_schema clarified as presence-only, not type-checked (90e80f9).
- T2 [blocked]: Python embed path likely redundant (verified Go is a full writer to langchain_pg_embedding for all 5 file types) but blocked on frontend routing fact; do not delete a live ingestion path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- PROGRESS.md +3 -1
PROGRESS.md
CHANGED
|
@@ -50,9 +50,11 @@ Verified against code before logging. Severity: **critical** / important / nice-
|
|
| 50 |
| R8 | **Catalog upsert race** β per-user advisory lock around read-merge-upsert (`store.py`); concurrent uploads can drop a source. | important | DB | `[ ]` |
|
| 51 |
| R9 | **`extra="ignore"`** in `settings.py:15` (currently `allow` β typo'd env vars silently swallowed); require Azure keys in prod. | nice-to-have | B | `[ ]` |
|
| 52 |
| R10 | **Read-only enforcement is session-state, not a server role.** `REPO_CONTEXT.md` counts "read-only DB credentials" as a defense layer but nothing requests/verifies a read-only role. Either request read-only creds at registration (verify via `SELECT current_setting(...)`) or drop the claim. | important | DB | `[ ]` |
|
| 53 |
-
| R11 | **De-duplicate** `_PLACEHOLDER_RE` (`task_runner.py:31` vs validator) and `_DATA_ACCESS_TOOLS` (invoker vs planner registry) β import one from the other; comments aren't a sync mechanism. | nice-to-have | agent/tool | `[
|
| 54 |
| R12 | **Doc/process hygiene** β some code docstrings cite internal design specs that are not committed to the repo (design docs are kept out of version control), so the references dangle for anyone but the author; `CLAUDE.md` lists deleted modules (enricher, `pipeline/orchestrator.py`); `main` is 38 commits behind on a dead architecture. | nice-to-have | agent | `[ ]` |
|
| 55 |
| R13 | **Pre-existing test failure** (found during R2, NOT caused by it): `tests/query/planner/test_prompt.py::test_render_catalog_with_sources` fails β `query/planner/prompt.py::render_catalog` now renders stable IDs (`src_test_db`) the test asserts are absent. Old query-planner path; confirmed failing on a clean tree. | nice-to-have | DB | `[ ]` |
|
|
|
|
|
|
|
| 56 |
|
| 57 |
**Architecture verdict:** fundamentally sound (catalog-driven IR + deterministic compiler
|
| 58 |
+ static plan is the right call). Debt is transitional duplication (two planners/registries/
|
|
|
|
| 50 |
| R8 | **Catalog upsert race** β per-user advisory lock around read-merge-upsert (`store.py`); concurrent uploads can drop a source. | important | DB | `[ ]` |
|
| 51 |
| R9 | **`extra="ignore"`** in `settings.py:15` (currently `allow` β typo'd env vars silently swallowed); require Azure keys in prod. | nice-to-have | B | `[ ]` |
|
| 52 |
| R10 | **Read-only enforcement is session-state, not a server role.** `REPO_CONTEXT.md` counts "read-only DB credentials" as a defense layer but nothing requests/verifies a read-only role. Either request read-only creds at registration (verify via `SELECT current_setting(...)`) or drop the claim. | important | DB | `[ ]` |
|
| 53 |
+
| R11 | **De-duplicate** `_PLACEHOLDER_RE` (`task_runner.py:31` vs validator) and `_DATA_ACCESS_TOOLS` (invoker vs planner registry) β import one from the other; comments aren't a sync mechanism. **TAB slice done (90e80f9):** canonical `DATA_ACCESS_TOOLS` now lives once in `tools/data_access.py`; `invoker.py` imports it (was a duplicated frozenset synced by comment). **Agent remainder:** `planner/registry.py` should derive its data-access spec names from that constant, and `_PLACEHOLDER_RE` (validator β task_runner) still needs dedup β both agent-side. | nice-to-have | agent/tool | `[~]` |
|
| 54 |
| R12 | **Doc/process hygiene** β some code docstrings cite internal design specs that are not committed to the repo (design docs are kept out of version control), so the references dangle for anyone but the author; `CLAUDE.md` lists deleted modules (enricher, `pipeline/orchestrator.py`); `main` is 38 commits behind on a dead architecture. | nice-to-have | agent | `[ ]` |
|
| 55 |
| R13 | **Pre-existing test failure** (found during R2, NOT caused by it): `tests/query/planner/test_prompt.py::test_render_catalog_with_sources` fails β `query/planner/prompt.py::render_catalog` now renders stable IDs (`src_test_db`) the test asserts are absent. Old query-planner path; confirmed failing on a clean tree. | nice-to-have | DB | `[ ]` |
|
| 56 |
+
| T1 | **`input_schema` is presence-only, not type-checked** β `ToolSpec.input_schema` comment said "validates ToolCall.args", but `TaskRunner._validate_args` only enforces `required` presence; the `properties` types are documentation, never validated at runtime. Clarified the contract in `tools/contracts.py` so nobody assumes type-safety (a wrong-typed arg passes validation, surfaces only inside the compute fn). Doc-only, no behavior change (90e80f9). | nice-to-have | TAB | `[x]` |
|
| 57 |
+
| T2 | **Dead Python embed path?** β `document_pipeline.process()` β `knowledge_processor` β `vector_store.aadd_documents()` still writes PDF/DOCX/TXT embeddings to `langchain_pg_embedding`, contradicting CLAUDE.md's "Go is sole writer, Python reads only". Verified the Go service (`Orchestrator-Agent-Service/internal/documents`) IS a complete ingestion writer to the same tables for all 5 file types (OCR + chunk + embed) β the Python embed branch is very likely redundant. **Blocked on one operational fact:** does the frontend still upload to `/document/process` (Python) or to Go? Park until confirmed β deleting a live ingestion path would break unstructured RAG. The csv/xlsx parquet branch stays regardless (feeds the catalog/tabular path). | nice-to-have | TAB | `[blocked]` |
|
| 58 |
|
| 59 |
**Architecture verdict:** fundamentally sound (catalog-driven IR + deterministic compiler
|
| 60 |
+ static plan is the right call). Debt is transitional duplication (two planners/registries/
|