Repository navigation
Attest upstream facts in the gate and re-check them before merging (design v8 8.2) - #232
Conversation
…esign v8 8.2)
Rules state facts about their upstream: the PRs they cite were merged, and
the files and symbols they name exist. Until now nothing checked them. The
service signed facts=[], and the verifier refused any verdict that carried
facts.
- New knowledge_service/facts.py, shared by both sides. It extracts the
claims that can be resolved from a rule:
- ^[PR #N] citations;
- backticked paths whose first segment is a top-level entry of the
upstream tree;
- path::Symbol, where every dotted part must be defined by a def, class
or assignment.
Fragments such as config/x.py, and paths with .. or ... segments, are not
claims. An active rule's claims must hold. A retired rule's claims are
only recorded, but the PR named as retirement evidence must be merged.
attest() observes the claims on the upstream head and returns the pinned
{repository, sha}, the facts and the problems. recheck() observes every
signed fact again and compares it field by field. An unreadable upstream
raises FactsError.
- Service: run_gate(facts=observer) attests right after L1, so a false
claim fails before any paid L2 call and goes to refine. An unreadable
upstream sends the change to people. The facts and the pinned upstream
go into the decision and are signed into the verdict. The observer
(kb_service/upstream_facts.MirrorObserver) uses the service's own bare
mirror, shared with the release sweep, and the read-only GitHub reader.
KbRuntime.upstream_facts supplies it. A repository without an upstream,
or one that publishes nothing, attests nothing.
- Publisher: after the local gate passes and before the final control
re-read, it re-checks every fact from independent sources: its own
mirror and `gh api`, using upstreams from its own adapter config. Any
difference, a missing public upstream, or an unreadable upstream means
no merge this round.
- The service treats such a refusal as transient and signs again next
round, recording facts_failing_since. After 24 consecutive hours the
change goes to gate_failed and people are told. Any other answer ends
the run of failures.
- gate_verifier no longer refuses verdicts that carry facts: the
publisher re-checks them.
Measured on the live vllm-omni knowledge against upstream main: 710 active
rules, 68 path/symbol claims, 7 rules (1%) cite a path that no longer
exists. Those rules fail the gate when they are next edited, which is the
intent.
Tests: test_kb_upstream_facts.py covers claim extraction, symbol
definitions, and attest/recheck when the upstream agrees, disagrees, is
unreachable, has an unknown SHA, is another repository, or the facts are
forged. It also covers a real git mirror and a gate that fails before L2.
For the publisher: it merges when the upstream agrees; it does not merge
when the upstream disagrees or is unreachable, and escalates after 24
hours; it does not merge without a configured public upstream.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6bf5a9566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if symbol: | ||
| if SYMBOL.fullmatch(symbol) and not path.endswith("/"): | ||
| out.append(Claim("symbol", active, path=path, symbol=symbol)) | ||
| continue |
There was a problem hiding this comment.
When a changed rule uses common call notation such as path.py::func(), SYMBOL.fullmatch fails and the unconditional continue drops the token entirely, so neither the path nor the symbol is attested and an invalid upstream assertion can pass the gate. This is already used by the active EXEC-10c rule in knowledge/repos/vllm-omni/components/model-executor/rules-platform-backends.md:32; editing that rule would therefore bypass the new check. Normalize supported call notation (and similar grouped notation), reject it explicitly, or at minimum preserve the path claim.
Useful? React with 👍 / 👎.
| try: | ||
| proc = subprocess.run(["git", "--git-dir", str(self.git_dir), *args], capture_output=True, | ||
| text=True, check=False, timeout=600) | ||
| except (OSError, subprocess.TimeoutExpired) as exc: | ||
| raise FactsError(f"git {args[0]}: {exc}") from exc |
There was a problem hiding this comment.
When a referenced upstream file contains non-UTF-8 bytes—including valid Python source with a non-UTF-8 encoding declaration—subprocess.run(..., text=True) raises UnicodeDecodeError while decoding stdout. _git catches only OSError and TimeoutExpired, whereas run_gate handles only FactsError, so the exception escapes and aborts the gate instead of producing the intended human result for an unreadable upstream. Decode explicitly with a defined policy or translate decoding failures to FactsError.
Useful? React with 👍 / 👎.
Design v8 §8.2: upstream fact attestations. The service signs them and the publisher re-checks them independently before merging. Until now nothing checked what rules claim about their upstream. The service signed
facts=[], and the verifier refused any verdict carrying facts.What counts as a claim (
knowledge_service/facts.py)^[PR #N]citations: the PR must be merged.path::Symbol(including root files such assetup.py::main): the symbol must be defined.ast. Only realdef/class, bound assignment targets and imports count; text in strings or comments,d[k] = vandobj.attr = vdo not.config/x.py, bare words, and../...segments.Service
run_gate(facts=...)attests right after L1:{repository, sha}and the facts are signed into the verdict.Publisher
gh api, using upstreams from its own adapter config.gate_failedand tells people.Measured
On the live vllm-omni knowledge against upstream
main:benchmarks/serve.pyandvllm_omni/core/prefix_cache.py. Those rules fail the gate when next edited.Tests
test_kb_upstream_facts.py:test_mcp.pyenvironment failures, which fail identically onorigin/main.🤖 Generated with Claude Code