Skip to content

Commit 568f5e2

Browse files
rustyconoverclaude
andcommitted
codegen: generate the protocol wire names, and drift-guard them
The names were hand-written on both sides of the C++ boundary, and a rename proved why they should not be. A version bump that misses the C++ tree is a red build, because the versions are generated and drift-guarded. A rename that missed it was a silent misroute instead: every request the DuckDB extension sent named a protocol the server no longer hosted, and nothing failed until integration. Same wire contract, two spellings, two very different costs. `vgi.codegen.cpp_protocol_name` emits both names into one header, `vgi_protocol_names.hpp`. One artifact rather than one per protocol, unlike the versions: these are not two independent facts but the wire-name contract, decided together and — as dc6f82d shows — changed together. One artifact means one regen entry, one drift test, and no way to update one name while leaving the other stale. The name is resolved through vgi-rpc's own `_protocol_wire_name` rather than reimplemented. Its rule has a subtlety worth not duplicating: it reads `vars(protocol)`, not `getattr`, so a Protocol that subclasses another and forgets to redeclare gets its own class name rather than silently inheriting its parent's routing key. A generator that reimplemented that with `getattr` would disagree with the dispatcher it exists to serve — precisely the drift it is for. The name is also run through vgi-rpc's `validate_protocol_name` (without `allow_reserved`, since nothing here may claim the framework's `vgi_rpc.` prefix), so an unroutable name fails at generation rather than becoming a literal every client sends and every server rejects. The drift test asserts against `_protocol_wire_name`, not against `Protocol.protocol_name` — a test written against `getattr` would pass while the client addressed a protocol the server does not host. All three guards were mutation-checked rather than assumed: a doctored header fails the checked-in comparison, an upstream rename with a stale header fails it too, and a reserved-prefix name aborts generation. Two fixes found while in here: `scripts/regen_generated.py` never listed the secret protocol's three artifacts, so `--check` answered "no drift" over a stale secret header — false confidence, which is worse than no script. Listed now; all three were in fact current. `cpp_secret_protocol_version.emit()` parsed `--namespace` and then dropped it on the floor, never forwarding it to `emit_version_header`. Harmless for the default namespace and silently wrong for the standalone-SDK case the flag exists to serve. Suite: 2724 passed, 103 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent dc6f82d commit 568f5e2

4 files changed

Lines changed: 352 additions & 0 deletions

File tree

‎scripts/regen_generated.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,22 @@
3434
("vgi.codegen.cpp_request_builders", "src/generated/vgi_request_builders.hpp", "vgi"),
3535
("vgi.codegen.cpp_constants", "src/generated/vgi_protocol_constants.hpp", "vgi"),
3636
("vgi.codegen.cpp_protocol_version", "src/generated/vgi_protocol_version.hpp", "vgi"),
37+
("vgi.codegen.cpp_protocol_name", "src/generated/vgi_protocol_names.hpp", "vgi"),
38+
# The secret protocol's three artifacts were checked in but never listed here,
39+
# so `--check` reported "no drift" over a stale secret header. Listed now: a
40+
# regen script that silently skips an artifact is worse than no script, because
41+
# it answers the drift question with false confidence.
42+
(
43+
"vgi.codegen.cpp_secret_protocol_version",
44+
"src/generated/vgi_secret_protocol_version.hpp",
45+
"vgi",
46+
),
47+
("vgi.codegen.cpp_secret_schemas", "src/generated/vgi_secret_protocol_schemas.hpp", "vgi"),
48+
(
49+
"vgi.codegen.cpp_secret_request_builders",
50+
"src/generated/vgi_secret_request_builders.hpp",
51+
"vgi",
52+
),
3753
("vgi.codegen.go_schemas", "vgi/generated/protocol_schemas.go", "vgi-go"),
3854
("vgi.codegen.rust_schemas", "vgi-protocol/src/generated/protocol_schemas.rs", "vgi-rust"),
3955
("vgi.codegen.rust_request_builders", "vgi-protocol/src/generated/request_params.rs", "vgi-rust"),
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
# Copyright 2025, 2026 Query Farm LLC - https://query.farm
2+
3+
"""Drift test for ``vgi.codegen.cpp_protocol_name``.
4+
5+
This test exists because its absence already cost a wire break. The protocol
6+
*versions* are generated and drift-guarded, so a version bump that misses the
7+
C++ tree is a red build. The protocol *names* were hand-written on both sides,
8+
so a rename was a silent misroute instead: every request the DuckDB extension
9+
sent named a protocol the server no longer hosted, and nothing failed until
10+
integration.
11+
12+
So the assertions below are not "the file looks plausible". They are, in order:
13+
the generator agrees with the dispatcher about what the name IS, the name is
14+
routable, and the checked-in header says the same thing.
15+
16+
Skipped when the ``vgi`` repo isn't checked out next to ``vgi-python``; the
17+
other tests still run.
18+
"""
19+
20+
from __future__ import annotations
21+
22+
import io
23+
import os
24+
import re
25+
from pathlib import Path
26+
27+
import pytest
28+
from vgi_rpc.rpc._types import _protocol_wire_name, validate_protocol_name
29+
30+
from vgi.codegen.cpp_protocol_name import (
31+
current_protocol_name,
32+
current_secret_protocol_name,
33+
emit,
34+
)
35+
from vgi.protocol import VgiProtocol
36+
from vgi.secret_protocol import VgiSecretProtocol
37+
38+
_CONSTANTS = {
39+
"VGI_PROTOCOL_NAME": (VgiProtocol, current_protocol_name),
40+
"VGI_SECRET_PROTOCOL_NAME": (VgiSecretProtocol, current_secret_protocol_name),
41+
}
42+
43+
44+
def _vgi_generated_path() -> Path:
45+
"""Locate ``vgi/src/generated/vgi_protocol_names.hpp``."""
46+
override = os.environ.get("VGI_GENERATED_PROTOCOL_NAMES_HPP")
47+
if override:
48+
return Path(override)
49+
return Path(__file__).resolve().parents[2] / "vgi" / "src" / "generated" / "vgi_protocol_names.hpp"
50+
51+
52+
_REGEN_HINT = (
53+
"To regenerate, run:\n"
54+
" uv run --project ~/Development/vgi-python python scripts/regen_generated.py\n"
55+
"\n"
56+
"Do not redirect a generator into its destination with `>`: the shell\n"
57+
"truncates the file before the generator runs, so any failure destroys\n"
58+
"the checked-in artifact. The script renders to memory first."
59+
)
60+
61+
62+
def _emitted() -> str:
63+
out = io.StringIO()
64+
emit(out)
65+
return out.getvalue()
66+
67+
68+
def _emitted_literal(constant: str) -> str:
69+
match = re.search(rf'inline constexpr std::string_view {constant} = "([^"]*)";', _emitted())
70+
assert match is not None, f"generator did not emit {constant}"
71+
return match.group(1)
72+
73+
74+
def test_generator_is_deterministic() -> None:
75+
"""Running the generator twice produces byte-identical output."""
76+
assert _emitted() == _emitted()
77+
78+
79+
@pytest.mark.parametrize("constant", sorted(_CONSTANTS))
80+
def test_emitted_value_matches_the_dispatcher(constant: str) -> None:
81+
"""The emitted literal is what vgi-rpc will actually route on.
82+
83+
Compared against ``_protocol_wire_name`` — the function the server itself
84+
uses — rather than against ``Protocol.protocol_name``. Those differ in a way
85+
that matters: the wire name is read from ``vars(protocol)``, so a Protocol
86+
that subclasses another and forgets to redeclare gets its own class name,
87+
NOT its parent's routing key. A test written against ``getattr`` would pass
88+
while the client addressed a protocol the server does not host.
89+
"""
90+
protocol, _accessor = _CONSTANTS[constant]
91+
assert _emitted_literal(constant) == _protocol_wire_name(protocol)
92+
93+
94+
@pytest.mark.parametrize("constant", sorted(_CONSTANTS))
95+
def test_emitted_value_is_routable(constant: str) -> None:
96+
"""A name the server would refuse to route must never become a client constant."""
97+
validate_protocol_name(_emitted_literal(constant))
98+
99+
100+
def test_the_two_protocols_do_not_share_a_routing_key() -> None:
101+
"""Distinct protocols must be distinctly addressable.
102+
103+
They are co-hostable by construction, and dispatch resolves (protocol,
104+
method). Were both to answer to one key, a co-hosting server would route
105+
``secret_lookup`` by whichever binding registered last.
106+
"""
107+
assert current_protocol_name() != current_secret_protocol_name()
108+
109+
110+
def test_checked_in_generated_hpp_matches_generator() -> None:
111+
"""The .hpp checked into the vgi repo must match the current generator output."""
112+
path = _vgi_generated_path()
113+
if not path.exists():
114+
pytest.skip(
115+
f"{path} not found; set VGI_GENERATED_PROTOCOL_NAMES_HPP or check out the vgi repo next to vgi-python"
116+
)
117+
118+
checked_in = path.read_text()
119+
expected = _emitted()
120+
121+
if checked_in != expected:
122+
for i, (actual_ch, expected_ch) in enumerate(zip(checked_in, expected, strict=False)):
123+
if actual_ch != expected_ch:
124+
window_start = max(0, i - 40)
125+
window_end = i + 40
126+
raise AssertionError(
127+
f"checked-in {path} differs from generator output at offset {i}.\n"
128+
f" checked-in: {checked_in[window_start:window_end]!r}\n"
129+
f" expected: {expected[window_start:window_end]!r}\n"
130+
f"{_REGEN_HINT}"
131+
)
132+
raise AssertionError(
133+
f"checked-in {path} is shorter/longer than generator output "
134+
f"({len(checked_in)} vs {len(expected)} chars)\n{_REGEN_HINT}"
135+
)

‎vgi/codegen/cpp_protocol_name.py‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
# Copyright 2025, 2026 Query Farm LLC - https://query.farm
2+
3+
r"""Emit ``vgi_protocol_names.hpp`` — the C++ side's view of the wire routing keys.
4+
5+
Third sibling of :mod:`vgi.codegen.cpp_constants` (byte keys from
6+
``vgi_rpc.metadata``) and :mod:`vgi.codegen.cpp_protocol_version` (the surface
7+
versions). This one emits the ``vgi_rpc.protocol`` *values*: the names a client
8+
must send to reach each protocol this repo declares.
9+
10+
Why this generator exists
11+
-------------------------
12+
13+
It was added after a real break, and the break is the specification. Neither
14+
Protocol declared a ``protocol_name``, so every implementation's wire name
15+
defaulted to whatever its local class happened to be called — six
16+
implementations, four different answers. That was invisible until the
17+
transports made ``vgi_rpc.protocol`` a required routing key, at which point
18+
there was no single string the DuckDB extension could send.
19+
20+
The names were then decided (``vgi.v2``, ``vgi.secret.v1``) and hand-written on
21+
both sides. That is the same shape of hazard one step removed: the version is
22+
generated *and* drift-guarded, so a version bump that misses the C++ tree is a
23+
red build, while a rename that missed it was a silent misroute. A wire contract
24+
that two repos spell independently will eventually be spelled differently.
25+
26+
Both names live in ONE header, unlike the versions, which have one file each.
27+
They are not two independent facts: they are the wire-name contract, decided
28+
together and — as the commit that introduced them shows — changed together. One
29+
artifact means one regen entry and one drift test, and makes it impossible to
30+
update one name while leaving the other stale.
31+
32+
The name is read through vgi-rpc's own ``_protocol_wire_name`` rather than
33+
reimplementing its rule here. The rule has a subtlety worth not duplicating:
34+
it reads ``vars(protocol)`` rather than ``getattr``, so a Protocol that
35+
subclasses another and forgets to redeclare does NOT silently inherit its
36+
parent's routing key. Reimplementing that with ``getattr`` would make this
37+
generator disagree with the dispatcher it exists to serve — which is precisely
38+
the class of drift the generator is for.
39+
40+
Workflow:
41+
42+
uv run --project ~/Development/vgi-python python scripts/regen_generated.py
43+
44+
``tests/test_generated_cpp_protocol_name.py`` enforces drift detection at PR time.
45+
"""
46+
47+
from __future__ import annotations
48+
49+
import argparse
50+
import io
51+
import sys
52+
from typing import TYPE_CHECKING
53+
54+
from vgi_rpc.rpc._types import (
55+
_protocol_wire_name,
56+
validate_protocol_name,
57+
)
58+
59+
from vgi.codegen._common import (
60+
DEFAULT_CPP_NAMESPACE,
61+
GeneratorError,
62+
close_namespace,
63+
open_namespace,
64+
parse_cpp_namespace,
65+
provenance_comment,
66+
)
67+
from vgi.protocol import VgiProtocol
68+
from vgi.secret_protocol import VgiSecretProtocol
69+
70+
if TYPE_CHECKING:
71+
from typing import TextIO
72+
73+
74+
GENERATOR_VERSION = "1"
75+
76+
77+
def current_protocol_name() -> str:
78+
"""Return the wire routing key for :class:`vgi.protocol.VgiProtocol`."""
79+
return _wire_name(VgiProtocol)
80+
81+
82+
def current_secret_protocol_name() -> str:
83+
"""Return the wire routing key for :class:`vgi.secret_protocol.VgiSecretProtocol`."""
84+
return _wire_name(VgiSecretProtocol)
85+
86+
87+
def _wire_name(protocol: type) -> str:
88+
"""Resolve and validate one Protocol's wire name.
89+
90+
Resolution is delegated to vgi-rpc so this never disagrees with the
91+
dispatcher. Validation is vgi-rpc's too, and runs here so a name that the
92+
server would refuse to route fails at generation rather than becoming a
93+
string literal that every client dutifully sends and every server rejects.
94+
95+
``allow_reserved`` is deliberately NOT set: the ``vgi_rpc.`` prefix belongs
96+
to protocols the framework itself defines, and nothing this repo declares
97+
may claim it.
98+
"""
99+
name = _protocol_wire_name(protocol)
100+
try:
101+
validate_protocol_name(name)
102+
except ValueError as exc:
103+
raise GeneratorError(f"{protocol.__name__} has an unroutable wire name: {exc}") from exc
104+
105+
# The name is emitted into a C++ string literal AND into an HTTP URL path
106+
# segment. validate_protocol_name's charset already excludes everything that
107+
# would need escaping in either; this asserts that rather than assuming it,
108+
# so a future loosening upstream surfaces here instead of as a malformed
109+
# header or a percent-encoding mismatch at some proxy.
110+
if not all(0x20 <= ord(c) < 0x7F for c in name):
111+
raise GeneratorError(f"non-printable byte in protocol_name {name!r}; this is a bug")
112+
return name
113+
114+
115+
def emit(out: TextIO, namespace: list[str] | None = None) -> None:
116+
"""Emit ``vgi_protocol_names.hpp`` to *out*."""
117+
if namespace is None:
118+
namespace = parse_cpp_namespace(DEFAULT_CPP_NAMESPACE)
119+
120+
entries = [
121+
(
122+
"VGI_PROTOCOL_NAME",
123+
current_protocol_name(),
124+
"VgiProtocol",
125+
"the worker/catalog protocol: bind, init, catalog_*, aggregates, table_buffering_*",
126+
),
127+
(
128+
"VGI_SECRET_PROTOCOL_NAME",
129+
current_secret_protocol_name(),
130+
"VgiSecretProtocol",
131+
"the standalone secret service: the single unary secret_lookup",
132+
),
133+
]
134+
135+
body = io.StringIO()
136+
body.write("#pragma once\n\n")
137+
body.write("#include <string_view>\n\n")
138+
body.write(open_namespace(namespace))
139+
body.write("\n")
140+
body.write("// Wire routing keys. A vgi-rpc server dispatches on the pair\n")
141+
body.write("// (protocol, method): one server may co-host several protocols and their\n")
142+
body.write("// method names may collide, so every request must name the protocol it\n")
143+
body.write("// addresses under `vgi_rpc.protocol`. On the raw transports that metadata\n")
144+
body.write("// key is the only carrier; over HTTP the same value is also the protocol\n")
145+
body.write("// path segment, and the server rejects a request whose two disagree.\n")
146+
body.write("//\n")
147+
body.write("// The major version is part of the name, so an incompatible major is a\n")
148+
body.write("// DIFFERENT protocol and an unroutable request 404s -- an answer any proxy\n")
149+
body.write("// understands without an Arrow parser.\n")
150+
for constant_name, value, source, purpose in entries:
151+
body.write("\n")
152+
body.write(f"// {source} -- {purpose}.\n")
153+
body.write(f"// Sourced from {source}.protocol_name (vgi-python).\n")
154+
body.write(f'inline constexpr std::string_view {constant_name} = "{value}";\n')
155+
body.write("\n")
156+
body.write(close_namespace(namespace))
157+
158+
out.write("// ============================================================================\n")
159+
out.write(
160+
provenance_comment(
161+
generator_module="vgi.codegen.cpp_protocol_name",
162+
generator_command="python -m vgi.codegen.cpp_protocol_name",
163+
generator_version=GENERATOR_VERSION,
164+
regen_command_lines=[
165+
"uv run --project ~/Development/vgi-python python scripts/regen_generated.py",
166+
],
167+
body=body.getvalue(),
168+
)
169+
)
170+
out.write("// ============================================================================\n")
171+
out.write("\n")
172+
out.write(body.getvalue())
173+
174+
175+
def main() -> None:
176+
"""Console-script entry point."""
177+
parser = argparse.ArgumentParser(description=__doc__)
178+
parser.add_argument(
179+
"--namespace",
180+
default=DEFAULT_CPP_NAMESPACE,
181+
help=(
182+
"C++ namespace to emit into, `::`-separated "
183+
f"(default: {DEFAULT_CPP_NAMESPACE}). VGI is not DuckDB-only: a "
184+
"standalone worker SDK wants something like `vgi::generated`."
185+
),
186+
)
187+
args = parser.parse_args()
188+
try:
189+
namespace = parse_cpp_namespace(args.namespace)
190+
except GeneratorError as e:
191+
print(f"\nerror: {e}\n", file=sys.stderr)
192+
sys.exit(2)
193+
emit(sys.stdout, namespace)
194+
195+
196+
if __name__ == "__main__":
197+
main()

‎vgi/codegen/cpp_secret_protocol_version.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ def emit(out: TextIO, namespace: list[str] | None = None) -> None:
5858
"uv run --project ~/Development/vgi-python python -m vgi.codegen.cpp_secret_protocol_version \\",
5959
" > ~/Development/vgi/src/generated/vgi_secret_protocol_version.hpp",
6060
],
61+
# Was dropped on the floor: `main` parsed --namespace, passed it to
62+
# emit(), and emit() never forwarded it. Harmless for the default, and
63+
# silently wrong for the standalone-SDK case the flag exists to serve.
64+
namespace=namespace,
6165
)
6266

6367

0 commit comments

Comments
 (0)