Repository navigation
Conversation
Signed-off-by: zengchuang <zengchuang3@huawei.com> Agent-assisted-by: Codex (author)
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base c2f463f066921a15120a3c86506c4f97b1909acb and head 6b7f5d3c1f9d90124c4c1894c7f78ce0f177144a.
What I checked:
src/inferrsi/apps/knowledge.py:open_knowledge_storeis a thin wrapper overKnowledgeStore(store_root, skill_root=..., read_only=not writable), and it defaults to read-only. A non-booleanwritableis rejected before the store is touched.knowledge_readernow goes through the same read-only entry, and its behavior is unchanged.- Candidate authority, transactions, expected-revision conflicts and the read-only write guard all stay in
KnowledgeStore(knowledge_store.pyL44-62, L118-143). Apps adds no write logic of its own. tests/test_apps_knowledge.py: a read-only open of an empty directory fails with "requires an existing database" and leaves the directory empty. The test also covers create/update, stale-writeRevisionConflict, parent lineage inhistory, rejection of writes through a read-only viewer, and reading back both revisions throughknowledge_reader.- The
docs/operations.mdrow and section match the entry's actual read-only default and the explicitwritable=Trueopt-in. - All 8 checks on this head are green (full-check 3.10/3.11, installed-package 3.10/3.11, declarations x2, pr-references, dashboard-browser).
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base c2f463f066921a15120a3c86506c4f97b1909acb and head 6b7f5d3c1f9d90124c4c1894c7f78ce0f177144a.
Governing: Refs #371 step 4 (explicit candidate create/update and read-only history entry). Independent first-pass as cursor[bot] (steward/Dong-bot standing delegation; not author or accountable parent for this slice).
Verified:
src/inferrsi/apps/knowledge.py:open_knowledge_storedefaultswritable=False→KnowledgeStore(..., read_only=True); rejects non-booleanwritablebefore touching the store;knowledge_readercomposes the same read-only entry.- Write/CAS/history authority stays in Knowledge (
KnowledgeStore.create/update/history); Apps adds no write logic. tests/test_apps_knowledge.py: RO miss does not initialize; non-boolean write flag rejected; create/update; stale-writeRevisionConflict; parent lineage viahistory; RO viewer rejects writes; pinned and current revisions readable throughknowledge_reader.- Local:
pytest tests/test_apps_knowledge.py→ 3 passed;ruff checkon changed Python → pass. - Ready CI on this head: declarations, pr-references, full-check 3.10/3.11, installed-package 3.10/3.11, dashboard-browser — all success.
docs/operations.mddocuments the entry, RO default, and explicitwritable=Trueopt-in.
Non-blockers: PR is one commit behind current main (36440109, #425) with a clean auto-merge of docs/operations.md; Amy already APPROVED this same head (5909004107). Not merged by this review.
Governing issue and delivery slice
Refs #371, step 4. Expose the Apps Python entry for an explicitly authorized Knowledge candidate store. It opens existing stores read-only by default for history. When the caller expressly enables writes, the returned Knowledge port provides create and expected-revision update. Existing Apps reader composition uses the same read-only entry. Knowledge retains its candidate, adapter authority, transaction and conflict rules.
The regression covers no initialization on a read-only miss, rejection of a nonboolean write flag, candidate create/update, stale-write rejection, parent lineage and reopening the original revision. The public entry is Python, with write validation and publication owned by Knowledge.
Validation
tar. The same failure reproduced at the base. The full rerun usedTAPE=-sotarreads the archive from stdin.