DO NOT MERGE Add Open Knowledge Format (OKF) example implementation on Agent Kernel - #536
DO NOT MERGE Add Open Knowledge Format (OKF) example implementation on Agent Kernel#536induwara-yaala wants to merge 21 commits into
Conversation
…t and improved README documentation
amithad
left a comment
There was a problem hiding this comment.
Design review (Stage 1 — design.md only). A strong, genuinely point-form design that fits Agent Kernel conventions (example-only, no library changes; storage classes take explicit constructor params mirroring the shared-driver rule; tool-subset-as-permission-model). Correctly staged: design.md alone, no premature spec.md. Every factual citation I checked verifies against develop. Two design-intent questions and some PR hygiene, no blockers.
Findings
[question]×2 (inline):index.mdauto-regeneration vs. its "curated listing" role; body-only sync idempotency ignoring source frontmatter.[suggestion](PR hygiene, un-anchorable): this is a design-spec-only PR (design.md+ drawio), but the title/body call it an "example implementation" and the latest commit isfeat:. Perak-dev-write-spec's PR guidance, spec-only PRs should use adocs:title, tick Documentation update / Other: design spec, and state that implementation follows in a separate PR. All Type-of-Change and checklist boxes are currently blank — mark the ones that apply and note the rest as N/A for a spec-only PR.[nit](optional, un-anchorable): a few sub-sections lean intospec.md-altitude detail (per-tool behavior in Agent-facing tools, the Write guardrails rejection list, the link-resolution normalization mechanics). Design-review altitude favors brevity — consider deferring the mechanics tospec.mdand keeping only the decisions here. Not blocking.
Verified against develop
KnowledgeBaseABC is query-shaped —read(query, limit=3)/write(records)atknowledgebase/base.py:7,84,74— so the "doesn't map onto OKF's path/navigation surface" motivation holds.- Example layout claims all check out:
examples/cli/openai/{demo.py,demo_test.py,README.md,pyproject.toml,build.sh,uv.lock}, theexamples/cli/knowledgebase/openai/<backend>/precedent,OpenAIModule/OpenAIToolBuilder.bind/CLI.main()/Test("demo.py"), and the interface-as-top-level-dir convention (cli,api,aws-serverless, …).
Nice: one Mermaid diagram (compliant), a deliberate Decisions (previously settled) section plus a single genuine Open question, and thorough edge-case coverage (content-based idempotency, source-wins conflict policy, cache scoped to the bundle only, and the index.md invariant vs. log.md best-effort split). CI is green.
…potency Resolve the two design-intent questions from PR #536 review: - index.md regeneration: state explicitly that write_concept regenerates a flat mechanical listing (not a curated merge), name the curation tradeoff in the invariant note, and add a matching Non-goals entry. - Sync idempotency: compare the full source-derived document (frontmatter and body), excluding only the derived timestamp, so metadata-only source edits re-sync instead of being silently skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Source documents live in a read-only S3 location. Clarify that only the Curator reaches the source (the Producer never does), and that access is enforced read-only at two layers: only list/read source tools are ever bound (the permission model), and the source S3Storage needs read-only IAM (s3:GetObject + s3:ListBucket) — a separate grant from the read-write bundle bucket. Documented in Storage abstraction, Agents, and README requirements. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…deployment documentation
…e on Agent Kernel
… dependencies accordingly
…ure and sync process
amithad
left a comment
There was a problem hiding this comment.
Implementation review. This PR has grown well beyond its description — it now ships the full OKF example (okf/ package, three agents, offline + smoke tests, deploy/ Terraform, CI matrix entry, docs index) alongside the reviewed design.md and a new plan.md. The implementation tracks the design closely and the offline test suite covers the design's test list well. One verified blocker (path traversal via .. in tool paths), a few design-conformance suggestions, and PR hygiene.
Findings
[blocker]×1 (inline, verified by running the code): agent-supplied..paths escape the bundle root onFileSystemStorage.[suggestion]×4 (inline):get_relatedraises instead of returning an error string; tools bound via the SDK'sfunction_toolinstead of the design'sOpenAIToolBuilder.bind; ancestor-cascadeindex.mdregeneration not reflected indesign.md; unrelatedlint-fix.ymlchange bundled in.[question]×1 (inline):plan.mdskips thespec.mdstage of the design→spec→plan flow.[nit]×1 (inline):terraform fmtondeploy/main.tf.
Un-anchorable
[suggestion]PR title/body are stale. The body still says "This is a design-spec-only PR — it adds design.md and the architecture drawio only", but the PR now contains the complete implementation,plan.md, a CI test-matrix entry, and a workflow change. Please rewrite the description for what actually ships (and tick New feature in Type of Change; the Testing/checklist items marked "N/A (spec-only)" now do apply).[suggestion]uv.lockis not committed. design.md's Example package section listsuv.lockin the layout ("Each is a self-contained project withdemo.py, ...,uv.lock"), and the siblingexamples/cli/openai/commits one. Commit the lock so the example builds reproducibly.
Spec conformance (checked against design.md + plan.md; all factual citations in both verify): storage ABC + both backends, knowledge cache, tool surface, write guardrails, the three agents and tool-subset permission model, timestamp-based sync (source_timestamp, source-wins, synced/ subtree, None-mtime re-sync), offline tests, the three-role smoke test, deploy/ (RW/RO policy split, no IAM users, app never creates buckets), README, and the docs example index are all implemented as designed. Deviations are the inline findings above.
Nice: the offline suite asserts the index.md invariant directly (not via prompt), pins mtimes for reproducible sync-idempotency tests, and asserts read-only source access; the Terraform adds public-access blocks and prefix-scoped ListBucket conditions beyond what the design asked; the new e2e CI job (e2e-tests (7, cli, examples/cli/okf/openai)) is green along with the rest of CI.
amithad
left a comment
There was a problem hiding this comment.
Re-review of 34dc3ecf..2eebc099. The three fix commits address the code findings from the previous review, and I verified each against the new head by running the code:
- Path traversal (was
[blocker]) — fixed and verified.write_concept("../escaped.md", …)andread_concept("../secret.txt")now return "escapes the bundle root" errors and nothing lands outside the root; the guard coverslist/read/search/get_related/read_source_fileandapply_write, withstorage._normalizeraising as defence in depth. Parametrized regression tests added for all of it — nice. - Tools-never-raise — fixed and verified.
get_relatedandappend_lognow returnError: …strings on unparseable documents, with tests. function_toolvsOpenAIToolBuilder.bind— resolved via the design, as offered: design.md and plan.md now document the directfunction_toolwrapping as deliberate. Fine.- Ancestor index cascade — design.md/plan.md updated to state the cascade and its per-write cost. Matches the implementation.
main.tfformatting — fixed.uv.lock— committed, but see the new blocker below.
New finding (1 inline [blocker]): the committed uv.lock resolves langchain-community 0.4.2, and the okf e2e CI job is now red — demo_test.py fails at collection with ModuleNotFoundError: No module named 'langchain_community.chat_models.vertexai' (ragas imports it; removed in 0.4.2). Details and fix on the inline comment.
Still open from the previous review (not restated inline):
- The PR title/body still describe a design-spec-only PR; please update them for the implementation that now ships.
- The unrelated
lint-fix.ymlchange is still bundled in. - The
[question]on skipping thespec.mdstage is unanswered on its thread.
There was a problem hiding this comment.
Pull request overview
Adds a new self-contained Open Knowledge Format (OKF) CLI example under examples/cli/okf/openai/, including an OKF mini-runtime (storage/cache/format/validation/tools), a 3-agent demo (Consumer/Producer/Curator), unit + e2e tests, S3 provisioning Terraform, and accompanying docs/spec updates. The PR also updates CI configuration to run the new example and adjusts the lint-fix workflow.
Changes:
- Add an OKF implementation package (
okf/) plus a runnable multi-agent CLI demo (demo.py) and tests (test_okf.py,demo_test.py). - Add Terraform module + deploy script for the S3-backed run path (
deploy/). - Update docs/specs and docs-site examples overview; add the OKF example to CI e2e test matrix; adjust
.github/workflows/lint-fix.yml.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/cli/okf/openai/test_okf.py | Offline unit tests for format/validation/index/cache/sync behaviors. |
| examples/cli/okf/openai/sample_source/009_ASH_07_Blue_Carbuncle.md | Sample source markdown content used by the curator sync. |
| examples/cli/okf/openai/sample_source/007_ASH_05_Five_Orange_Pips.md | Sample source markdown content used by the curator sync. |
| examples/cli/okf/openai/sample_source/005_ASH_03_Case_Of_Identity.md | Sample source markdown content used by the curator sync. |
| examples/cli/okf/openai/README.md | Example documentation, run instructions, and behavior notes. |
| examples/cli/okf/openai/pyproject.toml | Example project dependencies + tooling config (black/isort/mypy). |
| examples/cli/okf/openai/okf/validation.py | OKF write-path validation (frontmatter + field/link checks). |
| examples/cli/okf/openai/okf/tools.py | Agent-facing tool surface + index/log invariants and tool binding. |
| examples/cli/okf/openai/okf/sync.py | Source→bundle timestamp-based idempotent sync into synced/. |
| examples/cli/okf/openai/okf/storage.py | Storage abstraction + filesystem/S3 backends. |
| examples/cli/okf/openai/okf/format.py | Frontmatter parsing/serialization + link extraction/resolution. |
| examples/cli/okf/openai/okf/cache.py | In-memory read-through/write-through cache for bundle storage. |
| examples/cli/okf/openai/okf/bundle.py | Bundle façade wiring storage + cache + validation + optional source. |
| examples/cli/okf/openai/okf/init.py | Package exports for the example’s OKF components. |
| examples/cli/okf/openai/deploy/variables.tf | Terraform inputs for buckets/prefixes/region/tags. |
| examples/cli/okf/openai/deploy/terraform.tfvars.example | Example tfvars template for bucket names/prefixes. |
| examples/cli/okf/openai/deploy/outputs.tf | Terraform outputs for demo environment variables + policy ARNs. |
| examples/cli/okf/openai/deploy/main.tf | Terraform S3 buckets + IAM managed policies (bundle RW, source RO). |
| examples/cli/okf/openai/deploy/deploy.sh | Helper script to init/apply/destroy the Terraform module. |
| examples/cli/okf/openai/deploy/backend.tf | Terraform remote-state backend config (currently placeholder). |
| examples/cli/okf/openai/demo.py | CLI demo wiring 3 agents + backend selection + OKF logging config. |
| examples/cli/okf/openai/demo_test.py | Live-model e2e test driving the CLI via the Test harness. |
| examples/cli/okf/openai/config.yaml | Example logging config for Agent Kernel CLI. |
| examples/cli/okf/openai/build.sh | Example environment setup using uv (mirrors other CLI examples). |
| examples/cli/okf/openai/.gitignore | Ignore venv, caches, terraform state, and generated sample bundle. |
| docs/specs/499-OKF-exploration/plan.md | Implementation plan for the OKF example/spec set. |
| docs/specs/499-OKF-exploration/design.md | Design doc describing OKF example goals/requirements/non-goals. |
| docs/docs/examples/overview.md | Docs-site examples list updated to include okf/openai. |
| .github/workflows/lint-fix.yml | Workflow updated to lint/commit against github.ref_name. |
| .github/test-config.yaml | CI e2e matrix updated to include the OKF CLI example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
amithad
left a comment
There was a problem hiding this comment.
Re-review of a51ac8ef..b8d85d6b. The lock blocker is fixed and CI is fully green again, including the okf e2e job. One minor new [suggestion] inline; two items from earlier reviews remain open.
Resolved since last review
uv.lock/ langchain-community (was[blocker]) — fixed.constraint-dependencies = ["langchain-community<0.4.2"]added under[tool.uv], the lock regenerated (the constraint is recorded inuv.lock, resolution back on 0.4.1), ande2e-tests (7, cli, examples/cli/okf/openai)passes.- PR title/body — fixed. The description now accurately describes the implementation PR.
Nice improvements beyond the asks
S3Storage.exists/last_modifiednow swallow only 404-familyClientErrors and re-raise real failures (AccessDenied etc.) instead of masking them as "missing" — a hardening I had considered flagging and didn't; good catch.sample_source/swapped from the Sherlock Holmes texts to four short original synthetic stories, documented as such in the README.demo_test.py's grounding facts were updated to match, and I verified the asserted facts ("the Magpie" = Coral Deveraux, Inspector Merrow) do appear in001_The_Lighthouse_Cipher.md. Also drops ~3,400 lines of committed prose.- Remote Terraform state is now opt-in (
backend.tf.example+OKF_REMOTE_STATE=1 ./deploy.sh, copy git-ignored), with correct first-run-exit semantics soterraform initnever sees the placeholder bucket. - BOM literal in
format.pyreplaced with the escaped\ufeff.
Still open (raised previously, not restated inline):
- The unrelated
.github/workflows/lint-fix.ymlchange is still bundled into this PR (no reply on the thread). - The
[question]on skipping thespec.mdstage is still unanswered on its thread.
…raform state handling
Description
Adds a self-contained Open Knowledge Format (OKF) example on Agent Kernel — OKF is a
markdown knowledge bundle that agents navigate like a file system — under
examples/cli/okf/openai/, together with its design spec and the CI wiring to run it.The example ships:
FileSystemStoragefor the no-AWS run path,S3Storagefor ans3://backend),demo.py, tests, and adeploy/Terraform module that provisions the buckets and IAM policies.
The
docs/specs/499-OKF-exploration/design spec (Stage 1) and implementation plan areincluded alongside the code.
Type of Change
Related Issues
Closes #499
Changes Made
examples/cli/okf/openai/— full runnable OKF example:okf/core(
format,storage,bundle,sync,validation,cache,tools),demo.py,tests,
config.yaml,pyproject.toml,build.sh, and adeploy/Terraform moduledocs/specs/499-OKF-exploration/design.md+plan.md— design spec and implementation plandocs/specs/499-OKF-exploration/Open Knowledge Format.drawio.xml— architecture sketchdocs/docs/examples/overview.md— lists the new example.github/test-config.yaml,.github/workflows/lint-fix.yml— CI wiring for the exampleTesting
Checklist