feat: add (visibility, project, agent) scope tuple to Claim and Source (WIP)#121
Closed
Tet-9 wants to merge 1 commit into
Closed
feat: add (visibility, project, agent) scope tuple to Claim and Source (WIP)#121Tet-9 wants to merge 1 commit into
Tet-9 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a Visibility enum and optional visibility, project, and agent fields to Source and Claim; persists and filters them in storage; threads them through propose_claim, CLI flags, and MCP tools. Includes formatting refactors across proposals and server modules. ChangesVisibility scoping feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vouch/storage.py (1)
249-265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScoped source fields are silently ignored when content already exists.
At Line 249,
put_source()returns existing metadata before applyingvisibility/project/agent. Re-registering an existing source cannot populate the new scope tuple, so callers can pass scope values that are dropped without signal.💡 Suggested fix
meta_path = sdir / "meta.yaml" if meta_path.exists(): - return Source.model_validate(_yaml_load(meta_path.read_text())) + existing = Source.model_validate(_yaml_load(meta_path.read_text())) + changed = False + if visibility is not None and existing.visibility is None: + existing.visibility = _Visibility(visibility) + changed = True + if project is not None and existing.project is None: + existing.project = project + changed = True + if agent is not None and existing.agent is None: + existing.agent = agent + changed = True + if changed: + meta_path.write_text(_yaml_dump(existing.model_dump(mode="json"))) + return existing🤖 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/storage.py` around lines 249 - 265, put_source currently returns existing metadata immediately when meta_path.exists(), which silently ignores any provided visibility/project/agent scope; change the branch that loads existing metadata (the Source.model_validate(_yaml_load(meta_path.read_text())) path) to merge/override scope fields when callers pass them: load the Source instance, if visibility is provided set source.visibility = _Visibility(visibility), if project provided set source.project, if agent provided set source.agent, then persist the updated metadata (using the same YAML save path used elsewhere) and return the updated Source; reference the meta_path load path, Source.model_validate, _Visibility, and the visibility/project/agent attributes to locate where to apply this merge-and-save behavior.src/vouch/server.py (1)
77-170:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRetrieval still ignores the new scope tuple.
kb_searchandkb_contextreturn hits without filtering by effective scope (visibility/project/agent, fallbackscope). That means scoped artifacts can still be surfaced to unrelated callers.
🤖 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 `@src/vouch/proposals.py`:
- Around line 118-123: The code is blindly copying visibility into payload,
causing invalid visibility values to become pending proposals; update the
function that builds the payload (the block touching payload, visibility,
project, agent in src/vouch/proposals.py) to validate visibility against the
allowed set (e.g., the enum/list your service accepts) before assigning
payload["visibility"]; if visibility is invalid, raise ProposalError with a
clear message (use the ProposalError class) so invalid proposals fail fast
rather than at approval time.
In `@src/vouch/server.py`:
- Around line 294-305: kb_register_source_from_path currently lacks the scope
arguments (visibility, project, agent) so sources created via path cannot be
scoped the same way as kb_register_source; update the
kb_register_source_from_path signature to accept the same scope parameters as
kb_register_source (e.g., visibility: str | None = None, project: str | None =
None, agent: str | None = None), accept and forward those values into the call
to s.put_source (include them in the keyword args passed to put_source), and
ensure the default behavior matches existing kb_register_source when scope args
are omitted.
---
Outside diff comments:
In `@src/vouch/storage.py`:
- Around line 249-265: put_source currently returns existing metadata
immediately when meta_path.exists(), which silently ignores any provided
visibility/project/agent scope; change the branch that loads existing metadata
(the Source.model_validate(_yaml_load(meta_path.read_text())) path) to
merge/override scope fields when callers pass them: load the Source instance, if
visibility is provided set source.visibility = _Visibility(visibility), if
project provided set source.project, if agent provided set source.agent, then
persist the updated metadata (using the same YAML save path used elsewhere) and
return the updated Source; reference the meta_path load path,
Source.model_validate, _Visibility, and the visibility/project/agent attributes
to locate where to apply this merge-and-save behavior.
🪄 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: 10686372-b8be-4b76-a0c0-84f0536289c5
📒 Files selected for processing (4)
src/vouch/models.pysrc/vouch/proposals.pysrc/vouch/server.pysrc/vouch/storage.py
Tet-9
force-pushed
the
feat/100-richer-scopes-claim-source
branch
from
c4a9932 to
5d47480
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/vouch/cli.py`:
- Around line 253-255: Validate and normalize the visibility CLI option early in
the command handlers (before calling propose_claim(...) or put_source(...)) by
checking the passed string against the allowed set
{"private","project","team","public"} (case-normalize to lowercase), reject
invalid values with a clear click.UsageError or click.BadParameter, and
map/normalize any acceptable aliases if needed; update every handler that
accepts --visibility (the invocations that call propose_claim and put_source,
including the other flagged locations) to use the validated/normalized value so
only one canonical visibility string is forwarded into proposal payloads.
In `@src/vouch/storage.py`:
- Around line 298-302: The visibility filter currently only keeps records where
s.visibility is set and equals the requested visibility, which excludes legacy
records that use s.scope; update the filter around the 'out' list (the
comprehension that checks s.visibility.value == visibility) to also accept
records where s.visibility is None but s.scope exists and maps to the equivalent
visibility value (translate legacy scope -> visibility and compare to the
requested visibility). Apply the same change to the other identical block at the
region that currently spans the logic at lines 346-350 so legacy-scoped
artifacts are preserved when filtering by visibility.
- Around line 249-252: When meta.yaml already exists the code returns
immediately after Source.model_validate(_yaml_load(...)) so invalid visibility
values are never validated; instead, read and parse the YAML into a dict first
(using _yaml_load(meta_path.read_text())), validate the visibility value against
the Visibility enum/class (Visibility or _Visibility) and raise or normalize if
invalid, then call Source.model_validate on the parsed data and return; apply
the same change to the other early-return block around lines 263-265 so
visibility is validated before returning in both places.
🪄 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: 67594755-90d8-4bff-a8e1-d56557a9719f
📒 Files selected for processing (6)
CHANGELOG.mdsrc/vouch/cli.pysrc/vouch/models.pysrc/vouch/proposals.pysrc/vouch/server.pysrc/vouch/storage.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/vouch/models.py
- src/vouch/proposals.py
- src/vouch/server.py
Comment on lines
+253
to
+255
| @click.option("--visibility", default=None, help="private|project|team|public") | ||
| @click.option("--project", default=None, help="Project slug") | ||
| @click.option("--agent", default=None, help="Agent id") |
There was a problem hiding this comment.
Validate visibility before persisting/forwarding scope fields.
visibility is passed through as a raw string, so bad values can enter proposal payloads and fail only later in lifecycle flows. Normalize/validate once in CLI before calling propose_claim(...) / put_source(...).
Suggested fix
-from .models import ProposalStatus
+from .models import ProposalStatus, Visibility
...
+def _parse_visibility(value: str | None) -> str | None:
+ if value is None:
+ return None
+ return Visibility(value).value
...
def propose_claim_cmd(..., visibility: str | None, project: str | None, agent: str | None) -> None:
store = _load_store()
+ visibility = _parse_visibility(visibility)
with _cli_errors():
pr = propose_claim(
...
def source_add(..., visibility: str | None, project: str | None, agent: str | None) -> None:
"""Register a file as a Source; prints its sha256 id."""
store = _load_store()
+ visibility = _parse_visibility(visibility)
data = Path(path).read_bytes()Also applies to: 258-259, 266-267, 335-337, 339-340, 351-353
🤖 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 253 - 255, Validate and normalize the
visibility CLI option early in the command handlers (before calling
propose_claim(...) or put_source(...)) by checking the passed string against the
allowed set {"private","project","team","public"} (case-normalize to lowercase),
reject invalid values with a clear click.UsageError or click.BadParameter, and
map/normalize any acceptable aliases if needed; update every handler that
accepts --visibility (the invocations that call propose_claim and put_source,
including the other flagged locations) to use the validated/normalized value so
only one canonical visibility string is forwarded into proposal payloads.
Comment on lines
249
to
+252
| if meta_path.exists(): | ||
| return Source.model_validate(_yaml_load(meta_path.read_text())) | ||
| from .models import Visibility as _Visibility | ||
|
|
There was a problem hiding this comment.
Validate visibility before the existing-meta early return.
When meta.yaml already exists, the function returns before parsing visibility, so invalid visibility values are silently ignored on idempotent writes.
Suggested fix
meta_path = sdir / "meta.yaml"
+ from .models import Visibility as _Visibility
+ parsed_visibility = _Visibility(visibility) if visibility is not None else None
if meta_path.exists():
return Source.model_validate(_yaml_load(meta_path.read_text()))
- from .models import Visibility as _Visibility
...
- visibility=_Visibility(visibility) if visibility else None,
+ visibility=parsed_visibility,
project=project,
agent=agent,Also applies to: 263-265
🤖 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/storage.py` around lines 249 - 252, When meta.yaml already exists
the code returns immediately after Source.model_validate(_yaml_load(...)) so
invalid visibility values are never validated; instead, read and parse the YAML
into a dict first (using _yaml_load(meta_path.read_text())), validate the
visibility value against the Visibility enum/class (Visibility or _Visibility)
and raise or normalize if invalid, then call Source.model_validate on the parsed
data and return; apply the same change to the other early-return block around
lines 263-265 so visibility is validated before returning in both places.
Comment on lines
+298
to
+302
| if visibility is not None: | ||
| out = [ | ||
| s for s in out | ||
| if s.visibility is not None and s.visibility.value == visibility | ||
| ] |
There was a problem hiding this comment.
Visibility filters break backward compatibility for legacy artifacts.
Current filter logic requires visibility to be explicitly set, so older records with only legacy scope are excluded when visibility is requested. That conflicts with the backward-compatible default objective and can hide valid artifacts.
Suggested fix
+ from .models import Visibility as _Visibility
+ target_visibility = _Visibility(visibility).value if visibility is not None else None
...
- if visibility is not None:
+ if target_visibility is not None:
out = [
s for s in out
- if s.visibility is not None and s.visibility.value == visibility
+ if (
+ (s.visibility is not None and s.visibility.value == target_visibility)
+ or (
+ s.visibility is None
+ and getattr(s.scope, "value", s.scope) == target_visibility
+ )
+ )
]
...
- if visibility is not None:
+ if target_visibility is not None:
claims = [
c for c in claims
- if c.visibility is not None and c.visibility.value == visibility
+ if (
+ (c.visibility is not None and c.visibility.value == target_visibility)
+ or (
+ c.visibility is None
+ and getattr(c.scope, "value", c.scope) == target_visibility
+ )
+ )
]Also applies to: 346-350
🤖 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/storage.py` around lines 298 - 302, The visibility filter currently
only keeps records where s.visibility is set and equals the requested
visibility, which excludes legacy records that use s.scope; update the filter
around the 'out' list (the comprehension that checks s.visibility.value ==
visibility) to also accept records where s.visibility is None but s.scope exists
and maps to the equivalent visibility value (translate legacy scope ->
visibility and compare to the requested visibility). Apply the same change to
the other identical block at the region that currently spans the logic at lines
346-350 so legacy-scoped artifacts are preserved when filtering by visibility.
…e [WIP] Extends the single Scope enum on Claim and Source with three optional fields: visibility, project, and agent. Backward compatible — existing KBs without these fields default to None. - models.py: add Visibility enum and visibility/project/agent fields - storage.py: put_source() accepts and stores the new fields - proposals.py: propose_claim() accepts and passes through new fields - server.py: kb_propose_claim() and kb_register_source() expose new params Remaining: retrieval filtering, cli.py, index_db updates, tests. Closes vouchdev#100
Tet-9
force-pushed
the
feat/100-richer-scopes-claim-source
branch
from
5d47480 to
69e78f2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
schemas/claim.schema.json (1)
1-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSchema sync drift is still blocking CI.
schema-checkis failing because checked-in files underschemas/still differ fromsrc/vouch/models.py. Please regenerate schemas from the current models and commit the exact generated output so this PR can merge cleanly.🤖 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 `@schemas/claim.schema.json` around lines 1 - 209, The checked-in JSON schema (claim.schema.json) is out of sync with the runtime model (the Claim model in vouch.models), causing schema-check to fail; regenerate the JSON schemas from the current models using the project's schema-generation tool/script so the generated claim.schema.json exactly matches the model-derived output, verify schema-check passes locally, and commit the updated generated file.
🤖 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.
Outside diff comments:
In `@schemas/claim.schema.json`:
- Around line 1-209: The checked-in JSON schema (claim.schema.json) is out of
sync with the runtime model (the Claim model in vouch.models), causing
schema-check to fail; regenerate the JSON schemas from the current models using
the project's schema-generation tool/script so the generated claim.schema.json
exactly matches the model-derived output, verify schema-check passes locally,
and commit the updated generated file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8acbf5cf-1746-4955-9034-bb6c84aef6ab
📒 Files selected for processing (8)
CHANGELOG.mdschemas/claim.schema.jsonschemas/source.schema.jsonsrc/vouch/cli.pysrc/vouch/models.pysrc/vouch/proposals.pysrc/vouch/server.pysrc/vouch/storage.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/vouch/models.py
- src/vouch/cli.py
- src/vouch/server.py
- src/vouch/proposals.py
- src/vouch/storage.py
Contributor
Author
|
@plind-junior can you review this too.... I need to get pass that threshold pr's or I won't be able to do any other thing concerning your repo atm😞 |
Contributor
Author
|
@plind-junior you never stated why this one was closed |
Contributor
Author
|
@plind-junior , could you please reopen this too |
Contributor
Author
|
@plind-junior , could you possibly reopen this pr, i'd fix all conflicts on it |
Contributor
Author
|
@plind-junior ,can i get this one reopened likewise too |
Contributor
Author
|
I would fix all necessary conflicts attached |
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
2 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.
Summary
ClaimandSourcecarry a singlescopeenum today. With multi-agent sync landing (#90/#91), a KB shared across agents and projects needs finer-grained control over who sees what. This implements the richer scope tuple described in ROADMAP 0.2.Changes
models.pyAdd
Visibilityenum (private,project,team,public) and three optional fields to bothClaimandSource:visibility,project,agent. All default toNone— fully backward compatible with existing KBs.storage.pyput_source()accepts and storesvisibility,project,agent.list_claims()andlist_sources()accept optional filter kwargsvisibility,project,agentso callers can retrieve artifacts scoped to a specific context.proposals.pypropose_claim()accepts and passesvisibility,project,agentthrough to the claim payload so scope is recorded at proposal time and preserved through approval.server.pykb_propose_claim()andkb_register_source()MCP tools expose the new parameters so agents can tag artifacts at creation time.cli.pypropose-claimandsource addcommands gain--visibility,--project, and--agentflags.Backward Compatibility
All three new fields are optional with
Nonedefaults. Existing KBs load and function exactly as before. The existingscopeenum field is preserved unchanged.Testing
156 tests passing, no regressions.
Closes #100
Summary by CodeRabbit
New Features
--visibility,--project, and--agentflags for source and claim registration.Documentation