Repository navigation
prototype: reduce knowledge projection and declaration churn - #359
Conversation
Keep owner projections content-local, resolve the legacy catalog shape at read time, and derive complete declaration paths from trusted-base policy. Include strict provenance validation, compatibility tests and an explicit protected-main squash recovery workflow. Canonical knowledge is unchanged. This is a design-review prototype, not accepted architecture or knowledge-policy admission. Actual author model and effort are unknown and are not inferred. Signed-off-by: Codex <codex@openai.com> Agent-assisted-by: dot / unknown [effort=unknown] (author)
Skill evaluationPLANNED: not executed; there are no task outcomes yet.
Author verification steps (nonpaid; not yet run)
python tools/build_skill_index.py --source-revision 56c500367d5495fdd9f0e35030c9cd957f70eac5 --checkThe declared source pin matches this head's canonical library bytes. The command has not run: retain its exit code and full drift/success output. It checks the generated projection, not claim truth or skill efficacy. Evidence and logs · Retention: 30 days. Plan bindings, budget, frozen exam and advisory detailsAdvisory: obtain missing evidenceRetained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
Model/settings:
Automatic execution / cumulative PR budget: {"reason":"Trusted configuration or pricing missing, invalid or expired","remaining_usd":"unknown","reserved_or_unresolved_usd":"unknown","spent_or_legacy_usd":"unknown","state":"blocked"}Projection check selection (not an executed result): {"declared_in":"src/inferrsi/data/repo_packs/inferrsi/knowledge/_provenance.json","source_revision":"56c500367d5495fdd9f0e35030c9cd957f70eac5","status":"ready"}Provider/settings: Uncovered / unsupported paths (94)
|
Review (independent; COMMENT only — not an approval)Reviewed range: VerdictDo not Approve / merge while Draft, #355 design acceptance, and protected-path admission remain unresolved. Blockers
Code / design observations (non-blocking if design is accepted later)
What would unblock Approve later
No approval implied. Canonical skill bodies appear unchanged; this is storage/tooling/migration. |
Dong1017
added
the
area:knowledge
The #355 design decision accepted the shared source-snapshot manifest. Remove the prototype so the replacement implementation is authored and declared on its own; the prototype stays in branch history. Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
Dong1017
added
kind:rfc
…shot Each generated record repeated the repository-wide source commit and registry hash, so editing one skill rewrote every record. Keep those two values in one _provenance.json snapshot and store the records as schema 2 with only their own fields and resource hashes. The catalog resolves them into the unchanged public schema-1 records, and the generator refuses to write a record that does not resolve back to what it rendered. Migrate the 80 records from the already-pinned source revision; every resolved record is byte-identical to the previous stored record. Add a read-only helper that lists a PR's protected, policy and generated paths for the authorship declaration without supplying any author fact, and document the workflow. Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
The migrated snapshot named the pre-squash branch commit of #325. Point it at the squash commit on main, which has identical skill-library bytes, so the pin stays an ancestor after this change lands. Only _provenance.json changes. Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
Re-review (independent; COMMENT only — not an approval)Reviewed range: Earlier findings
Blocking on this head
Code observations (non-blocking, questions)
What would unblock Approve
No approval implied. |
Hosted CI failed two tests. The divergent-edit merge ran without the fixture's Git identity, so on a runner without a global identity it stopped before any conflict; every fixture Git call now uses the same identity. The coverage-view test rendered the index with the placeholder revision "test-source", which the strict snapshot no longer accepts; it now passes a full-length revision. Also state in the workflow docs that the helper's path lists overlap. Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR review verdict — DO NOT PASS
Reviewed base 56c500367d5495fdd9f0e35030c9cd957f70eac5 and head b9851bf93d762a991c31320e4c27449f176338eb (full base..head diff; pr/prior-reviews.md is empty and no previous head was recorded).
This review does not approve and does not change the issue #355 scope. I will re-read after you push.
Blocker
Public source pin disagrees with the migration contract. resolve() writes sources[0].commit and sources[0].url from the shared snapshot revision (src/inferrsi/_source_snapshot.py, the git source object built in resolve). _provenance.json sets that revision to 56c500367d5495fdd9f0e35030c9cd957f70eac5. On base, every visible record and the generated README pin d648f25732dee1c974da62a94c46a642ad52cef8 (example: knowledge/ascend/skills/ascend-aclgraph-capture-replay.json sources[0].commit; README source-snapshot line removed in this diff).
docs/knowledge-contribution-workflow.md (Stored format, the catalog paragraph) says the migration regenerated all 80 records from the already-pinned source revision, that each resolved record is byte-identical to the previous stored record, and that search, knowledge show, Collect, and installed wheels see no change. With this snapshot, catalog consumers observe commit 56c50036… and blob URLs under that commit. Per-file content digests in the visible records are unchanged, and the registry sha256 7904275c3383380db179287171e1430617aaa9ac4f439291eff8a71426987fbb is only relocated into _provenance.json.
Trigger: catalog.knowledge_records() on any schema-2 skill record.
Correction, either one:
- Put
source_revisionback tod648f25732dee1c974da62a94c46a642ad52cef8and regenerate so resolvedsourcesmatch the previous public records, or - Keep
56c50036…(it is an ancestor of this head, which the old squash pin is documented not to be) and correct the workflow text so it records the pin move, states thatsources.commitandsources.urlchange, and points at a generator--checkagainst that revision for the stored digests.
Verified in the seeded diff
compactdropssourcesand the registry digest;resolverebuilds one git source, inserts it afterorigin, and merges the registry digest back into sortedsource_files. Snapshot validation requires schema 1, the InferRSI repository, a 40-hex revision,skill-index.json, and a 64-hex digest.catalog.knowledge_recordsresolves only dicts withschema_version == 2, skipsknowledge/_provenance.json, and raises if a schema-2 record has no snapshot.- Visible skill JSON keeps its own resource hashes. The shared commit and registry hash leave the per-record files.
- Docs that describe the new path (
docs/contributing.md,docs/knowledge-base.md,docs/skill-eval-pr.md,knowledge/README.md) point at_provenance.jsonfor the pin.
Not verified here
- Seeded
pr/diff.patchstops athost-overhead-optimization.json(…[truncated]). Generator, coverage builder,tools/knowledge_declaration_paths.py, SkillEval planner, tests, and package-data rules were not in the reviewed text. - Tests and
build_skill_index.py --checkwere not run. CI was not inspected. - PR body was not seeded, so the knowledge authorship declaration for head
b9851bf…was not checked. - Records after the truncation point were not checked for
origin,skill_documents.original, and a matchingsource_filesentry. The visible ones have that shape, whichresolverequires.
Optional
knowledge/README.mdno longer embeds a concrete revision; the check example is the placeholder<committed-source-revision>. Pointing at_provenance.jsonis enough if the pin and the workflow text agree.
Incremental re-review (COMMENT only, not an approval)Reviewed range: What this commit changes
These look like the right fixes for the two Still blocking
What would unblock Approve
No approval implied. |
The migration regenerated the records at the previous pin d648f25, where every resolved record was byte-identical, and then moved the pin to the squash commit 56c5003 on main. State that resolved records therefore differ only in sources[0].commit and sources[0].url, and how to check the stored hashes against the recorded revision. Agent-assisted-by: Claude Code / claude-opus-5-5 [effort=xhigh] (author) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base 56c500367d5495fdd9f0e35030c9cd957f70eac5 and head dfae5ed5cc45294b8e31229c2f991b843f61a412 (incremental b9851bf93d762a991c31320e4c27449f176338eb..dfae5ed5cc45294b8e31229c2f991b843f61a412).
Earlier finding
Resolved: the public source pin now matches the migration contract in docs/knowledge-contribution-workflow.md (Stored format, catalog paragraph). The paragraph states that all 80 records were regenerated at d648f25732dee1c974da62a94c46a642ad52cef8 and that each resolved record was byte-identical to the previous stored record, then the shared pin moved to 56c500367d5495fdd9f0e35030c9cd957f70eac5. It states that resolved records differ in sources[0].commit and sources[0].url, while every content hash and the registry hash stay the same. That matches resolve() writing those two source fields from the _provenance.json snapshot revision (src/inferrsi/_source_snapshot.py).
Schema 1, field set, and field order stay in place for search, knowledge show, Collect, and installed wheels. Hash checks are python tools/build_skill_index.py --source-revision <source_revision> --check with the source_revision from _provenance.json.
Verified
- Incremental diff contains only
docs/knowledge-contribution-workflow.md. - The replacement contract names both revisions and limits the public delta to the two source fields traced in the previous review.
Not verified here
tools/build_skill_index.py --checkwas not run.- Ancestry of
d648f25732dee1c974da62a94c46a642ad52cef8and byte-equality of the skill-library trees against56c500367d5495fdd9f0e35030c9cd957f70eac5were not re-checked in this pass. - CI was not inspected.
No outstanding blocker. This review stays on the issue #355 workflow contract.
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
Incremental range: b9851bf93d762a991c31320e4c27449f176338eb..dfae5ed5cc45294b8e31229c2f991b843f61a412. The net PR was reviewed earlier against base 56c500367d5495fdd9f0e35030c9cd957f70eac5.
docs/knowledge-contribution-workflow.md(Stored format) now records the pin move. Records were regenerated atd648f257…, where they were byte-identical, and then re-pinned to56c50036…, an ancestor ofmain. As a result, onlysources[0].commitandsources[0].urldiffer, and it shows how to--checkagainst_provenance.json. This resolves the remaining doc blocker from my last review.- CI on this head:
full-check (3.10)andfull-check (3.11)pass, as doinstalled-package(both),lint-and-smoke,pr-referencesand the latestdeclarationsrun. An earlierdeclarationsrun on the same head failed and a later one passed. - The PR is out of Draft.
The manual repair path after future squash merges, already listed under Limits, stays as a known follow-up.
Refs #355
What this changes
This implements the representation accepted in the #355 design decision, after the reader landed in #360.
knowledge/_provenance.jsonstores the full source revision and the registry hash. Generated skill records are stored as schema 2 and keep only their own fields and resource hashes.inferrsi.catalogresolves each stored record into a public schema-1 record with the same fields and order: thesourcesentry with the full commit URL, andsource_filesincluding the registry hash. Search,knowledge show, Collect and installed wheels keep reading schema 1. The generator refuses to write a record that does not resolve back to the record it rendered.tools/knowledge_declaration_paths.py(read-only) lists the head SHA and the protected, policy, generated and other paths under the target branch's policy, including deletions and both rename endpoints. It never supplies an author, model, effort or exception.contributing.md,knowledge-base.mdandskill-eval-pr.mdpoint to it.No canonical skill, matrix, coverage view, ranking, policy or workflow changes.
Commits
4260fc75d1d64da1; the tree equals its base. The prototype stays in branch historye9a4b69a56c50036; the tree equals mainf6762d7fd648f2570241800e56c50036, the squash commit on main with identical skill-library bytes. Only_provenance.jsonchangesb9851bf9full-check: the divergent-edit merge now uses the fixture's Git identity (runners have none), and the coverage-view test passes a full-length revision to the strict snapshot. States in the docs that the helper's path lists overlapdfae5ed5d648f257to56c50036and that onlysources[0].commitandsources[0].urlchange in resolved records0241800eis itself the smallest example of the new cost: changing the snapshot revision now touches 1 file instead of 82.Measured contribution cost
One appended line in
performance-ceiling-analysis/SKILL.md, on main56c50036, regenerated with each generator:_provenance.json, coverage viewThe coverage view changes in both cases because it hashes the edited
SKILL.md. These are file counts, not authoring-time or efficacy claims.Verification
f6762d7f(pind648f257), all 80 resolved records are byte-identical to the records on main.0241800ethen changes onlysource_revisionin_provenance.json, so at the head the resolved records differ from main only insources[0].commitandsources[0].url(now56c50036); every content hash and the registry hash are unchanged.build_skill_index.py --check(80 records) andbuild_skill_coverage.py --checkpass at the head.tests/test_source_snapshot.py,tests/test_knowledge_declaration_paths.py) cover: stored schema and resolution for all 80 records; field order; 8 invalid-snapshot and 5 invalid-record cases; a one-skill edit changing only its record and the snapshot; two skills edited on parallel branches and integrated in both orders, keeping both contributions with the third skill's record unchanged; divergent edits to one skill remaining a conflict;--checkfailing on a tampered or missing snapshot and a tampered record; and the helper's protected, renamed, deleted and policy paths, with the base branch's policy deciding scope.ruff check src tests toolsandgit diff --checkpass. The two tests that failed on hostedfull-checkat0241800epass locally atb9851bf9with no global or system Git identity. Hosted Python 3.10/3.11 full and installed-package checks are the full-suite evidence.readywith source revision56c50036declared in_provenance.json; the declaration helper reports 84 protected paths (82 generated,catalog.py,knowledge-base.md); andcheck_knowledge_model_policy.assessreturns declaration compliance for this body.Limits
inferrsi.catalogorinferrsi._source_snapshot.resolve. No such reader exists insrcortoolson main.0241800e.Knowledge authorship declaration
The design came from the earlier prototype and the accepted #355 decision. That prototype's commit is reverted here, and the code, tests, docs and regenerated records in this PR's diff were written for this rewrite.