feat(timeline): kb.timeline — entity chronological trajectory (#313)#354
Closed
devmixa702 wants to merge 15 commits into
Closed
feat(timeline): kb.timeline — entity chronological trajectory (#313)#354devmixa702 wants to merge 15 commits into
devmixa702 wants to merge 15 commits into
Conversation
scaffold typed page and entity proposals from the page-kind registry so authors get correctly-shaped drafts through the normal review gate. Co-authored-by: Cursor <cursoragent@cursor.com>
catch invalid yaml in --field and interactive prompts, gate citation-required kinds before filing, unify dry-run json id key, and move new command tests into test_cli.py. Co-authored-by: Cursor <cursoragent@cursor.com>
the old openclaw.plugin.json spoke a manifest dialect current openclaw no longer parses: installs of the linked repo failed before load (the loader hard-requires id + configSchema, routes any dir containing an openclaw.plugin.json away from the bundle path, and needs a package.json openclaw.extensions pointer to find the entry module). every old-dialect field the manifest relied on — mcpServers, contracts, family, shared_deps, openclaw.* — is silently ignored by the current parser. rewrite the packaging for the current contract, verified against a real openclaw 2026.6.11 install: * openclaw.plugin.json: id + json-schema configSchema + kind: context-engine + version (synced with pyproject) + skills as SKILL.md directories under adapters/openclaw/skills/ (mirrored from the claude-code commands; a sync test keeps them identical). the kb.* mcp server moves to deployment config (openclaw mcp add). * package.json (new, loader-facing only): openclaw.extensions entry pointer + openclaw.compat.pluginApi floor. * engine id renamed vouch-context -> vouch to equal the plugin id: the installer auto-binds plugins.slots.contextEngine to the plugin id while resolveContextEngine looks the slot value up by engine id, so distinct ids quarantined the engine and silently fell back to openclaw's legacy engine. * vouch openclaw-rpc: json-serialize datetimes in wire payloads. contextPack.generated_at crashed json.dumps on every assemble that found a kb — found by running a live agent turn against the linked plugin, where openclaw quarantined the engine for the process. * tests: manifest sync tests rewritten for the new dialect (id alignment, three-way version sync, publishable skill dirs, dead dialect fields rejected); new tier-2 e2e (tests/test_openclaw_plugin_load_real.py, skipped without the openclaw cli) links the repo into an isolated profile and asserts import, engine registration, slot auto-bind, skill publication, and a clean plugins doctor; new stdio round-trip regression test for the datetime fix. verified live: plugins install --link of this repo, inspect --runtime (imported, loaded, contextEngineIds ["vouch"]), four skills ready, plugins doctor clean, and a full embedded agent turn where assemble() ran against a real kb with zero context-engine quarantines.
93f5836 made storage.list_sources() skip artifacts it cannot parse so one corrupt file no longer crashes bulk listings, and pinned utf-8 so a non-utf-8 locale no longer mis-decodes healthy files into mojibake at read time. the skip leaves a gap for files that really are corrupt on disk: a hand edit or an external writer under a mismatched locale can land a raw control character pyyaml rejects, and such a meta then vanishes from the lint sweep with only a log warning while any claim citing the source is misreported as broken_citation. lint now loads source metas per-file like _load_claims_for_lint does for claims: a meta that fails to parse becomes an unreadable_source error finding with a repair hint, and the source id (its directory name) still counts as present so citation checks point at the actual problem instead of a phantom missing source.
# Conflicts: # CHANGELOG.md # openclaw.plugin.json
…emplate-refs Docs/remove gittensor template refs
…ifest fix(openclaw): repackage plugin for the 2026.6 loader dialect
fix(health): surface unreadable source metas as lint findings
new remember-across-sessions tutorial told through prompts typed into the claude code window, with real captured transcripts: monday's session fixes a staging bug and proposes what it learned over mcp; the sessionstart hook injects the approved claims into tuesday's fresh session, which answers a pr-review question in 2 turns citing the incident and its regression test, while the same question without vouch is slower and generic. includes a measured a/b benchmark on the fix task (4 verified headless runs per arm, claude-sonnet-5): 17% faster, 32% fewer turns, 31% fewer output tokens, 18% cheaper with the ~260-token recall digest injected. adds the tutorial to the index.
feat(cli): add vouch new scaffold command
1. Add 8 new CLI commands for reading and listing approved artifacts
(read-{claim,page,entity,relation}, list-{claims,pages,entities,relations})
2. Document self-approval prevention in getting-started guide with
config option for local testing (approver_role: trusted-agent)
3. Parameterize jsonl-quickstart test to support dynamic method count
4. Complete decision-log example with 5 approved decisions and full KB
Fixes discovered during end-to-end installation testing.
the relation model uses source/relation/target but the list-relations command was incorrectly trying to access from_id/relation_type/to_id. this caused mypy errors that failed the ci type-check step.
…ifest fix: add CLI read/list, docs, fix test, complete example
…ev#313) read_entity returns an entity's claims as a flat set; neighbors expands graph adjacency. neither answers "what did the kb learn about this entity, in what order?". kb.timeline orders an entity's approved claims and relations along a time axis, oldest first. - order=effective uses the artifact's own created_at (accrual time); order=decided recovers approval time from the audit log's approve events (object_ids[1] -> created_at), falling back to created_at for artifacts written outside the proposal path. - since/until/types/limit filters; limit keeps the most recent n while preserving chronological order. - superseded/archived claims still appear, flagged by current status; relations carry status=null; pending proposals never appear. pure read: no propose_*, no approve, no write path is reachable — a viewport over already-reviewed artifacts. all ordering logic lives in a new timeline.py, not storage.py. registered at all four surfaces (mcp/jsonl/capabilities/cli); attaches _meta.vouch_salience when a session_id is passed, per the per-tool convention.
github-actions
Bot
added
docs
📝 WalkthroughWalkthroughThis PR adds a read-only Changeskb.timeline feature
Approved-artifact reads and vouch new scaffolding
OpenClaw plugin packaging alignment
Lint source-metadata validation fix
Example decision-log fixture directory
Tutorial and getting-started documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
timeline recovered approval time from the audit log by importing metrics' private `_APPROVE_RE`. promote it to a public `APPROVE_RE` so the cross-module use is intentional rather than reaching into another module's private symbol — a rename in metrics would otherwise silently break the decided-order axis, with every artifact falling back to created_at and no compile-time signal. `_CREATE_RE` / `_REJECT_RE` stay private; only the approve regex is shared.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/vouch/health.py (1)
124-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid implementation; matches upstream
Source/storage contracts.Logic correctly surfaces corrupt
meta.yamlasunreadable_sourcewhile still counting the id as "present" for citation checks (avoiding a misleadingbroken_citation), consistent with theSourcemodel andsources/<sha>/meta.yamlon-disk layout used elsewhere in storage.py.One optional nit: this function structurally mirrors
_load_claims_for_lint(iterate → parse → validate → catch → appendFinding). Not urgent, since the iteration mechanics differ (glob vs.iterdir+ explicit meta check), but a shared helper could reduce duplication if a third similar scanner (e.g. entities/relations) is added later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/health.py` around lines 124 - 165, The implementation is correct, and no functional change is required; this mirrors `_load_claims_for_lint`’s iterate/parse/validate/catch/appended Finding pattern in `_collect_sources_for_lint` and fits the `Source`/`meta.yaml` contract. If you touch this area later, consider extracting the shared scan-and-validate flow into a helper used by `_load_claims_for_lint` and `_collect_sources_for_lint` to reduce duplication, especially if additional scanners are added.src/vouch/cli.py (1)
1395-1416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
citation_reminderderivation.The dry-run guard re-derives
requires_citations and not (claims or sources), which is exactlycitation_remindercomputed a few lines above. Reuse the variable for clarity.♻️ Proposed fix
if dry_run: - if not missing and not (requires_citations and not (claims or sources)): + if not missing and not citation_reminder:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/cli.py` around lines 1395 - 1416, The dry-run branch in cli.py is recalculating the citation reminder condition instead of reusing the existing citation_reminder variable. Update the logic in the page draft flow around page_draft and the dry_run check to reference citation_reminder directly so the condition is defined once and stays consistent.src/vouch/timeline.py (2)
49-63: 🚀 Performance & Scalability | 🔵 TrivialFull audit-log scan on every
order=decidedcall.
_decided_mapre-reads the entireaudit.log.jsonlon every invocation ofbuild_timeline(order="decided"). Fine today, but worth keeping in mind as a scaling limit for KBs with long-lived, heavily-audited histories (repeatedkb.timelinecalls become O(events) each).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/timeline.py` around lines 49 - 63, The _decided_map helper in timeline.py rescans the full audit stream every time build_timeline(order="decided") runs, which makes repeated timeline calls scale poorly. Update _decided_map (and any caller path in build_timeline) to avoid re-reading all events on each invocation by caching the decided mapping per KBStore/kb_dir or otherwise deriving it from a maintained index, while keeping the fallback behavior for artifacts without an approve event.
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport a public approval-regex helper
timeline.pyimportsmetrics._APPROVE_RE, butmetrics.pyonly defines it as a private module constant. Expose a public alias/helper there and use that instead so_decided_mapdoesn’t couple to an internal name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/timeline.py` around lines 22 - 23, The approval-regex is being imported in timeline.py through the private metrics._APPROVE_RE constant, which creates an internal dependency. In metrics.py, add a public alias or helper for the approval regex and update _decided_map in timeline.py to import and use that सार्वजनिक symbol instead of the underscored one. Keep the existing regex behavior the same, but route access through the new public name so the timeline code no longer depends on a private module constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tutorials/README.md`:
- Around line 30-40: Update the tutorial README intro to stay consistent with
the “one session” framing by explicitly marking this walkthrough as a
two-session exception or softening the claim in the opening copy. Locate the
affected tutorial blurb around the “Carry what one Claude Code session learned
into the next” entry and adjust the surrounding language so it clearly signals
that this example spans two sessions while preserving the rest of the page’s
messaging.
In `@docs/tutorials/remember-across-sessions.md`:
- Around line 13-17: The tutorial text overstates that the 30-second review is
the only vouch command ever typed, which conflicts with the setup steps that use
vouch init and vouch install-mcp. Update the wording in the
remember-across-sessions section to qualify the claim as applying after setup or
once installed, so the description matches the commands shown in the tutorial
and avoids contradicting itself.
In `@src/vouch/jsonl_server.py`:
- Around line 185-204: The `handle_request` exception mapping is treating
`ArtifactNotFoundError` from `kb.timeline` as `missing_param` because it
subclasses `KeyError`. Update the `except` ordering in
`src/vouch/jsonl_server.py::handle_request` so the more specific unknown-entity
path from `_h_timeline`/`build_timeline` is caught before the generic `KeyError`
branch and returned as `invalid_request`.
In `@src/vouch/timeline.py`:
- Around line 93-109: In the timeline filtering logic, `types` can be a plain
string and currently gets treated like an iterable of characters in the `want =
{t.strip() ...}` path. Update the input validation in the function that builds
`want` so `types` must be a list/sequence of strings, and raise `TimelineError`
with a clear message when a string or other invalid type is passed. Keep the
rest of the `store.get_entity`, `_as_utc`, and `ORDER_DECIDED` flow unchanged.
In `@tests/test_timeline.py`:
- Around line 189-195: The kb.timeline test currently calls the handler directly
and misses the JSONL request envelope shape that handle_request returns. Update
test_jsonl_handler_runs to exercise the JSONL path via the request wrapper used
by vouch.jsonl_server.handle_request, and assert the full {id, ok, result}
success envelope (or {id, ok: false, error} on failure) instead of only checking
HANDLERS["kb.timeline"] output. Keep the existing kb.timeline and HANDLERS
symbols as the locator for the handler under test.
---
Nitpick comments:
In `@src/vouch/cli.py`:
- Around line 1395-1416: The dry-run branch in cli.py is recalculating the
citation reminder condition instead of reusing the existing citation_reminder
variable. Update the logic in the page draft flow around page_draft and the
dry_run check to reference citation_reminder directly so the condition is
defined once and stays consistent.
In `@src/vouch/health.py`:
- Around line 124-165: The implementation is correct, and no functional change
is required; this mirrors `_load_claims_for_lint`’s
iterate/parse/validate/catch/appended Finding pattern in
`_collect_sources_for_lint` and fits the `Source`/`meta.yaml` contract. If you
touch this area later, consider extracting the shared scan-and-validate flow
into a helper used by `_load_claims_for_lint` and `_collect_sources_for_lint` to
reduce duplication, especially if additional scanners are added.
In `@src/vouch/timeline.py`:
- Around line 49-63: The _decided_map helper in timeline.py rescans the full
audit stream every time build_timeline(order="decided") runs, which makes
repeated timeline calls scale poorly. Update _decided_map (and any caller path
in build_timeline) to avoid re-reading all events on each invocation by caching
the decided mapping per KBStore/kb_dir or otherwise deriving it from a
maintained index, while keeping the fallback behavior for artifacts without an
approve event.
- Around line 22-23: The approval-regex is being imported in timeline.py through
the private metrics._APPROVE_RE constant, which creates an internal dependency.
In metrics.py, add a public alias or helper for the approval regex and update
_decided_map in timeline.py to import and use that सार्वजनिक symbol instead of
the underscored one. Keep the existing regex behavior the same, but route access
through the new public name so the timeline code no longer depends on a private
module constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6b376fd-2d08-440d-8a71-69b00cf8ee23
📒 Files selected for processing (53)
CHANGELOG.mdCLAUDE.mdREADME.mdadapters/openclaw/README.mdadapters/openclaw/skills/vouch-propose-from-pr/SKILL.mdadapters/openclaw/skills/vouch-recall/SKILL.mdadapters/openclaw/skills/vouch-resolve-issue/SKILL.mdadapters/openclaw/skills/vouch-status/SKILL.mdadapters/openclaw/vouch-context-engine.mjsdocs/getting-started.mddocs/tutorials/README.mddocs/tutorials/remember-across-sessions.mdexamples/decision-log/vouch/.gitignoreexamples/decision-log/vouch/audit.log.jsonlexamples/decision-log/vouch/claims/billing-data-requires-acid-guarantees-postgresql-is-the-prim.yamlexamples/decision-log/vouch/claims/critical-system-incidents-must-have-a-15-minute-response-sla.yamlexamples/decision-log/vouch/claims/free-tier-100-req-superseded.yamlexamples/decision-log/vouch/claims/free-tier-500-req.yamlexamples/decision-log/vouch/claims/free-tier-gets-100-requests-per-day-as-baseline.yamlexamples/decision-log/vouch/claims/incident-response-15min-sla.yamlexamples/decision-log/vouch/claims/use-postgres-for-billing.yamlexamples/decision-log/vouch/claims/use-postgresql-15-for-enhanced-replication-and-monitoring-fe.yamlexamples/decision-log/vouch/claims/vouch-starter-reviewed-knowledge.yamlexamples/decision-log/vouch/config.yamlexamples/decision-log/vouch/decided/20260703-091452-063145f6.yamlexamples/decision-log/vouch/decided/20260703-091452-4ffb9687.yamlexamples/decision-log/vouch/decided/20260703-091452-976471f3.yamlexamples/decision-log/vouch/decided/20260703-091453-4ca258b5.yamlexamples/decision-log/vouch/pages/edit-in-obsidian.mdexamples/decision-log/vouch/schema_versionexamples/decision-log/vouch/sources/1f8ef21996a0f59eeb5174071f5293fc3b864defe928b9ce7879022a8230dbc8/contentexamples/decision-log/vouch/sources/1f8ef21996a0f59eeb5174071f5293fc3b864defe928b9ce7879022a8230dbc8/meta.yamlexamples/decision-log/vouch/sources/3131367abbcefab105fa2fcaa36617bfdda15511cc8880145af113731e95ad6d/contentexamples/decision-log/vouch/sources/3131367abbcefab105fa2fcaa36617bfdda15511cc8880145af113731e95ad6d/meta.yamlexamples/decision-log/vouch/sources/be7aec64b0fc803a33cb3d610f67ae95e636877db20231ef72440a7cbe6b69d2/contentexamples/decision-log/vouch/sources/be7aec64b0fc803a33cb3d610f67ae95e636877db20231ef72440a7cbe6b69d2/meta.yamlexamples/jsonl-quickstart/run.shopenclaw.plugin.jsonpackage.jsonsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/health.pysrc/vouch/jsonl_server.pysrc/vouch/openclaw/context_engine.pysrc/vouch/openclaw/rpc.pysrc/vouch/server.pysrc/vouch/timeline.pytests/test_cli.pytests/test_health.pytests/test_openclaw_context_engine.pytests/test_openclaw_plugin_load_real.pytests/test_openclaw_plugin_manifest.pytests/test_timeline.py
💤 Files with no reviewable changes (4)
- examples/decision-log/vouch/claims/use-postgres-for-billing.yaml
- examples/decision-log/vouch/claims/free-tier-500-req.yaml
- examples/decision-log/vouch/claims/free-tier-100-req-superseded.yaml
- examples/decision-log/vouch/claims/incident-response-15min-sla.yaml
Comment on lines
+30
to
+40
| - [**Carry what one Claude Code session learned into the next**](remember-across-sessions.md) | ||
| — the two-session story, told entirely through prompts typed into the Claude | ||
| Code window (real captured transcripts). Monday you type a bug report; | ||
| Claude fixes it and proposes what it learned to vouch on its own (MCP + | ||
| hooks — you never type a vouch command except the 30-second approve). | ||
| Tuesday you type a PR-review question into a brand-new session: the | ||
| SessionStart hook has already injected the approved claims, and Claude | ||
| answers in 2 turns and 15 seconds citing the incident and the regression | ||
| test — while the same question without vouch is 33% slower and generic. | ||
| Includes a measured A/B benchmark (~17% faster, ~32% fewer turns, ~18% | ||
| cheaper on the same bug-fix task). About 15 minutes. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
call out the two-session exception.
this entry is explicitly a two-session walkthrough, but the intro above still says tutorials get from nothing to a working result "in one session." either relax that framing or label this one as the exception so the page stays consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tutorials/README.md` around lines 30 - 40, Update the tutorial README
intro to stay consistent with the “one session” framing by explicitly marking
this walkthrough as a two-session exception or softening the claim in the
opening copy. Locate the affected tutorial blurb around the “Carry what one
Claude Code session learned into the next” entry and adjust the surrounding
language so it clearly signals that this example spans two sessions while
preserving the rest of the page’s messaging.
Comment on lines
+13
to
+17
| You never drive vouch yourself. The agent proposes knowledge over MCP | ||
| while it works, hooks auto-capture the session, and every new session | ||
| gets the approved claims injected before your first message. The only | ||
| vouch command you ever type is the 30-second review — which is the point: | ||
| nothing becomes "what the team knows" without a human approving it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
qualify the no-vouch-command claim.
this says the only vouch command you ever type is the 30-second review, but the setup immediately below still has vouch init and vouch install-mcp. narrow the claim to "after setup" or "once installed" so the tutorial does not contradict itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tutorials/remember-across-sessions.md` around lines 13 - 17, The
tutorial text overstates that the 30-second review is the only vouch command
ever typed, which conflicts with the setup steps that use vouch init and vouch
install-mcp. Update the wording in the remember-across-sessions section to
qualify the claim as applying after setup or once installed, so the description
matches the commands shown in the tutorial and avoids contradicting itself.
Comment on lines
+185
to
+204
| def _h_timeline(p: dict) -> dict: | ||
| from .metrics import parse_since | ||
| from .timeline import build_timeline | ||
|
|
||
| store = _store() | ||
| cfg = _load_cfg(store) | ||
| session_id = p.get("session_id") | ||
| limit = p.get("limit") | ||
| result = build_timeline( | ||
| store, | ||
| p["entity_id"], | ||
| since=parse_since(p.get("since")), | ||
| until=parse_since(p.get("until")), | ||
| order=p.get("order", "effective"), | ||
| types=p.get("types"), | ||
| limit=int(limit) if limit is not None else None, | ||
| ) | ||
| return salience_mod.attach_salience(result, store, session_id, cfg) | ||
|
|
||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files of interest\n'
git ls-files 'src/vouch/jsonl_server.py' 'src/vouch/storage.py' 'src/vouch/*' | sed -n '1,120p'
printf '\n## Relevant symbols in src/vouch/jsonl_server.py\n'
ast-grep outline src/vouch/jsonl_server.py --view expanded | sed -n '1,220p'
printf '\n## Relevant symbols in src/vouch/storage.py\n'
ast-grep outline src/vouch/storage.py --view expanded | sed -n '1,220p'
printf '\n## Find handle_request and ArtifactNotFoundError definitions/usages\n'
rg -n "def handle_request|class ArtifactNotFoundError|except KeyError|except \\(ValueError, ProposalError, ArtifactNotFoundError\\)|kb.timeline|timeline" src/vouch -n -A 4 -B 4Repository: vouchdev/vouch
Length of output: 25538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## build_timeline exception path\n'
sed -n '74,170p' src/vouch/timeline.py | cat -n
printf '\n## jsonl_server handle_request exception ordering\n'
sed -n '757,784p' src/vouch/jsonl_server.py | cat -n
printf '\n## any tests covering jsonl handle_request or kb.timeline errors\n'
rg -n "kb.timeline|handle_request\\(|missing_param|invalid_request|ArtifactNotFoundError" tests src/vouch -g '!src/vouch/web/**' -A 3 -B 3Repository: vouchdev/vouch
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## timeline tests around JSONL coverage\n'
sed -n '180,220p' tests/test_timeline.py | cat -n
printf '\n## jsonl server tests around error mapping\n'
sed -n '1,120p' tests/test_jsonl_server.py | cat -nRepository: vouchdev/vouch
Length of output: 7201
Map missing entity_id to invalid_request in kb.timeline.
ArtifactNotFoundError from build_timeline() subclasses KeyError, so the JSONL envelope currently reports an unknown entity as missing_param. Reorder the except blocks in src/vouch/jsonl_server.py::handle_request so this path surfaces as invalid_request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/jsonl_server.py` around lines 185 - 204, The `handle_request`
exception mapping is treating `ArtifactNotFoundError` from `kb.timeline` as
`missing_param` because it subclasses `KeyError`. Update the `except` ordering
in `src/vouch/jsonl_server.py::handle_request` so the more specific
unknown-entity path from `_h_timeline`/`build_timeline` is caught before the
generic `KeyError` branch and returned as `invalid_request`.
Comment on lines
+93
to
+109
| if order not in ORDERS: | ||
| raise TimelineError(f"order must be one of {ORDERS}, got {order!r}") | ||
| if limit is not None and limit < 0: | ||
| raise TimelineError("limit must be >= 0") | ||
| since = _as_utc(since) | ||
| until = _as_utc(until) | ||
|
|
||
| entity = store.get_entity(entity_id) # raises ArtifactNotFoundError | ||
|
|
||
| want = {t.strip() for t in types if t.strip()} if types else None | ||
| decided = _decided_map(store) if order == ORDER_DECIDED else {} | ||
|
|
||
| def when_for(artifact_id: str, created: datetime | None) -> datetime | None: | ||
| eff = _as_utc(created) | ||
| if order == ORDER_DECIDED: | ||
| return decided.get(artifact_id) or eff | ||
| return eff |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
types silently mis-filters when passed a string instead of a list.
If types arrives as a plain string (plausible from JSONL/MCP callers, since nothing enforces the list type at runtime), {t.strip() for t in types if t.strip()} iterates over individual characters rather than raising a clear error, producing a confusing empty/garbage filter instead of a validation error.
🐛 Proposed fix
want = {t.strip() for t in types if t.strip()} if types else None
+ if types is not None and isinstance(types, str):
+ raise TimelineError("types must be a list of strings, not a single string")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/timeline.py` around lines 93 - 109, In the timeline filtering
logic, `types` can be a plain string and currently gets treated like an iterable
of characters in the `want = {t.strip() ...}` path. Update the input validation
in the function that builds `want` so `types` must be a list/sequence of
strings, and raise `TimelineError` with a clear message when a string or other
invalid type is passed. Keep the rest of the `store.get_entity`, `_as_utc`, and
`ORDER_DECIDED` flow unchanged.
Comment on lines
+189
to
+195
| def test_jsonl_handler_runs(store: KBStore, monkeypatch) -> None: | ||
| _seed(store) | ||
| monkeypatch.chdir(store.root) | ||
| from vouch.jsonl_server import HANDLERS | ||
|
|
||
| out = HANDLERS["kb.timeline"]({"entity_id": "acme", "order": "effective"}) | ||
| assert [e["id"] for e in out["entries"]] == ["c1", "c2", "r1", "c3"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'handle_request' tests/test_jsonl_server.py -A3 -B3Repository: vouchdev/vouch
Length of output: 5850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the timeline test and related JSONL server tests/handlers.
sed -n '160,220p' tests/test_timeline.py
printf '\n---\n'
sed -n '1,260p' tests/test_jsonl_server.py
printf '\n---\n'
sed -n '1,220p' vouch/jsonl_server.pyRepository: vouchdev/vouch
Length of output: 11665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'jsonl_server.py' .
printf '\n---\n'
rg -n 'kb\.timeline|def handle_request|HANDLERS' -S .Repository: vouchdev/vouch
Length of output: 13196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the JSONL server dispatch/envelope logic and the tests that might cover it generically.
sed -n '680,790p' src/vouch/jsonl_server.py
printf '\n---\n'
sed -n '1,200p' tests/test_capabilities.py
printf '\n---\n'
sed -n '1,220p' tests/test_provenance.py
printf '\n---\n'
sed -n '1,180p' tests/test_synthesize.pyRepository: vouchdev/vouch
Length of output: 16814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '680,790p' src/vouch/jsonl_server.py
printf '\n---\n'
sed -n '1,200p' tests/test_capabilities.py
printf '\n---\n'
sed -n '1,220p' tests/test_provenance.py
printf '\n---\n'
sed -n '1,180p' tests/test_synthesize.pyRepository: vouchdev/vouch
Length of output: 16814
Add a JSONL envelope assertion for kb.timeline. test_jsonl_handler_runs only calls HANDLERS["kb.timeline"] directly, so it skips the {id, ok, result} / {id, ok: false, error} shape that handle_request returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_timeline.py` around lines 189 - 195, The kb.timeline test
currently calls the handler directly and misses the JSONL request envelope shape
that handle_request returns. Update test_jsonl_handler_runs to exercise the
JSONL path via the request wrapper used by vouch.jsonl_server.handle_request,
and assert the full {id, ok, result} success envelope (or {id, ok: false, error}
on failure) instead of only checking HANDLERS["kb.timeline"] output. Keep the
existing kb.timeline and HANDLERS symbols as the locator for the handler under
test.
Source: Coding guidelines
devmixa702
changed the title
Author
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
parallels #352 (dhgoal's original
kb.timeline); opened from a fork so it cancarry a small review follow-up on top of the feature. credit for the feature
itself to @dhgoal.
kb.read_entityreturns an entity and its claims as a flat set;kb.neighborsexpands graph adjacency at a point in time. neither answers "what did the kb
learn about this entity, in what order?" — a trajectory. this orders an
entity's approved claims and relations along a time axis, oldest-first.
effectiveuses artifactcreated_at(when the fact entered thekb);
decidedrecovers approval time from the append-only audit log.--since/--until/--types/--limit;--jsonfor themachine shape.
carry
status = null; pending proposals never appear.surfaces (mcp/jsonl/capabilities/cli), and attaches
_meta.vouch_saliencewhen a
session_idis passed.on top of the feature, one review follow-up:
metrics._APPROVE_REis promotedto a public
metrics.APPROVE_RE, sotimelinereuses it intentionally ratherthan reaching into another module's private symbol — a rename in metrics would
otherwise silently break the decided-order axis with no compile-time signal.