Repository navigation
docs(eventing): fix doc/code inconsistencies from the agentdocs review - #900
Conversation
Six verified inconsistencies between the eventing docs and the tree,
each checked against the code before editing (the repo's own §8.5/§8.6
discipline):
1. SIGNED_ATTRS drift: DESIGN_PHASE3's header, §2.6, §7.7, §9 step 4 and
§11 T4 still described T4 as adding two attributes ("userkey, depth"),
contradicting §8.3's rule, §2.6's own opening, Report3's T4 row and
shared/signing.py, which all say three (userkey, depth, agent) in ONE
change. All five sites now agree.
2. ER_USERKEY false positive (code fix): the runner refused to start
whenever REQUEST_TOPIC != "requests" and ER_USERKEY was unset, which
false-positives every Phase 1 single-tenant deployment — the shipped
k8s/base/configmap.yaml sets REQUEST_TOPIC: kev1-requests and boots no
runner at all. The refusal now keys on the per-user topic shape
({prefix}-{userkey}-requests, §3.1) via tenancy.userkey_in_topic(), so
a renamed default boots and a per-user topic still demands its
matching key — mismatched too, not just missing. DESIGN_PHASE3 §3.3
and §8.2 updated to match; 10 new tests (round-trip, each suffix,
renamed-default regression, mismatch refusal, non-userkey shapes).
3. EB_TOPIC_PREFIX default: §3.1 claimed "{prefix} defaults to the
namespace (kev1)"; the code has a literal "kev1" default and §8.1's
row agrees. Reworded to the literal default and why.
4. README_PHASE1: "submitter is not signed" — it joined SIGNED_ATTRS in
Phase 2 (DESIGN_PHASE2 §4.2), kept the honest-limit framing (Kafka
still plaintext, the claim is about EventBridge's assertion).
"roughly 100 ms per sign/verify" — Report1 §9 measured 222/227 ms;
replaced with the measured figures and the 190-230 ms re-run band.
5. README_PHASE0: pinned the suite count to Report0's record (58 passed
in 7.27s, not "20 passed in ~0.6s").
6. KubeCon §9 changelog rows reordered chronologically (10-05, 10-05,
10-06); Report1's §3 table now cites §9 for the sign/verify figures
(the section they live in); DESIGN_PHASE0's CLI skeleton --watch uses
argparse.BooleanOptionalAction like the shipped CLI, not
store_true+default=True, a combination that can never be false;
DESIGN_PHASE2 §5 gains the missing ER_VERIFY_KEY_PATH row that the
keyset row already referenced; the ~150-200 ms estimates in four code
docstrings replaced with the measured 222/227 ms (Report1 §9), which
KubeCon M4 flags as estimates-vs-measured.
eventing suite: 907 passed, 7 skipped (baseline 897 + 10 new).
ruff check / format --check: clean, CI-exact invocation.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
The §3.3 refusal covers two distinct faults but reported both as "ER_USERKEY is required": an unset variable, and one that disagrees with the userkey embedded in REQUEST_TOPIC. For the second the message is wrong — the variable *was* supplied, it just names another tenant — and an operator told a variable they set is missing looks in the wrong place. The exit now names which fault it is and, on a mismatch, prints both the supplied key and the embedded one, so a mis-rendered deployment is diagnosable from the exit line without reading the chart. Behaviour is unchanged: the same four inputs boot or refuse exactly as before, only the text differs. The mismatch test asserts the new wording, that "is required" is absent, and that both keys appear. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Summary
I verified every documentation correction in this PR against the code at head, and all of
them hold: submitter really is in SIGNED_ATTRS (shared/signing.py:195), the three
Phase 3 attributes really do land in one change (signing.py:196-210), 222 ms / 227 ms
really is in IMPLEMENTATION_REPORT1 §9 and not §6 (line 827), Report0 really records
58 passed in 7.27s, the shipped CLI really uses argparse.BooleanOptionalAction
(skills/eventbridge/eventbridge-cli.py:659), ER_VERIFY_KEY_PATH really defaults to empty
with the keyset falling back to it (eventrunner/config.py:100-105), and the 190-230 ms
band is sourced rather than invented (KUBECON_NA_2026.md §2.4, "re-running it here gave
190/198 ms median"). README_PHASE0's "four tests in tests/test_router.py" still reads
correctly after the count change — test_router.py has exactly 4. The §2 bug is real and
worth the code fix: k8s/base/configmap.yaml:18 does ship REQUEST_TOPIC: kev1-requests
with no ER_USERKEY, so the old condition did refuse to boot the shipped Phase 1 deployment.
One must-fix in the new code. PER_USER_TOPIC_RE tries to recover {userkey} from
{prefix}-{userkey}-{suffix}, but both prefix and the userkey's slug may contain -,
so that grammar is ambiguous and no quantifier choice resolves it. The shipped non-greedy
{1,32}? misparses a legal dashed prefix and reintroduces exactly the false-positive boot
refusal this PR set out to remove; flipping it to greedy only moves the failure onto
multi-dash slugs, which are ordinary (the module's own docstring example is
oi-alice-example-com-9e8f7a6b). Details and a tested alternative are inline.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Python (shared/tenancy.py, eventrunner/config.py, shared/signing.py, eventrunner/emit.py, eventbridge/kafka_out.py), tests, Docs (9 agentdocs files)
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: passing (11/11 — eventing-test, lint, codeql, trivy-scan, hadolint, dependency-review, test-startup, DCO)
Review: #900, three comments — one must-fix, one suggestion, one nit. - must-fix: `userkey_in_topic`'s split is ambiguous, and the non-greedy quantifier picked the wrong end — a legal dashed prefix (`prod-eu-…`) made a correctly-configured runner refuse to boot; the greedy flip moves the failure to multi-dash slugs, which userkey()'s own docstring shows are ordinary. The grammar cannot be fixed by quantifier, so the guard no longer decomposes the name at all: `userkey_in_topic` keeps only the yes/no per-user question (every valid split means per-user), its docstring now forbids using the captured key for identity, and a new `tenancy.topic_names_userkey(topic, userkey)` anchors on the key the runner already holds — `-{userkey}-{suffix}`, no split. All eight of the reviewer's cases pass, plus dashed-prefix and multi-dash-slug boot tests, and the dashed-prefix case he asked for in test_userkey_in_topic_*. - suggestion (taken): the inverse misconfiguration — ER_USERKEY set while REQUEST_TOPIC is a shared topic — now refuses too. emit.py keys its stamp on userkey presence, not tenancy mode, so that runner would have consumed the shared topic and filed every response as one tenant's: the harm §3.3 names, from the other direction. ER_USERKEY is also now shape-checked (`is_valid_userkey`) at boot — a typo'd key names nobody and StoreRegistry counts every stamped response `unattributed`, and a topic rendered from the same typo'd key anchors against it consistently, so only the shape check catches it. - nit: §3.1's EB_TOPIC_PREFIX paragraph reflowed to the file's width. Rebased onto 370ef6b, which split the refusal message by fault ("is required" for unset, "must match" for a wrong key). That split is preserved and carried into the anchored check: the message names the fault and the supplied key, and REQUEST_TOPIC rides in the message because the anchoring check does not extract an embedded key to print. DESIGN_PHASE3 §3.3 and §8.2 describe the two-directional refusal and the anchoring rationale; the Cfg docstring matches. eventing suite: 915 passed, 7 skipped (897 baseline; +18 across the three commits). ruff check / format --check: clean, CI-exact invocation. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
|
@aslom all three of your comments are addressed in 2e94bcb — replies with details are on each thread:
One wrinkle: this rebased onto 370ef6b (the fault-naming refusal messages), which landed between my original commit and the review — your message split is preserved and carried into the anchored check. Suite: 915 passed, 7 skipped; ruff clean; all CI green. Since your review was filed as a comment rather than an approval, the branch protection gate is still asking for one — mind converting when you're satisfied? Thanks! |
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Summary
Re-review of 2e94bcb0 against my three findings on 370ef6b9. All three are addressed,
and I re-derived the fix rather than taking the reply's word for it.
The must-fix is genuinely fixed. topic_names_userkey anchors on -{userkey}-{suffix}
with no split, and I re-ran the full scenario matrix against the new tenancy.py driving
load()'s three checks in order: 10/10 correct, including the two that failed before —
prod-eu-gh-alice-ed66acf0-requests with the correct key now boots, and the multi-dash-slug
case (oi-alice-ex-ample-com-1f806a21) that a greedy quantifier would have broken also
boots. The renamed-default case still boots and every mismatch still refuses. I also
confirmed userkey_in_topic has exactly one production caller (config.py:208) which
uses only the is not None answer — so the docstring's "never compared for identity" is
enforced in practice, not just documented.
The suggestion was taken in both halves, and the reasoning in the reply about the shape
check is correct and checks out: store_registry.py:167 does say "An INVALID key counts as
unattributed too, not just a missing one", so a typo'd key really is invisible to the
topic-name checks and only is_valid_userkey can catch it. I verified the new shape check
cannot break an existing deployment — ER_USERKEY is set nowhere in k8s/, the overlays,
or scripts/. Not making the inverse refusal conditional on tenancy mode is the right call
for the reason given: emit.py:77-78 keys the stamp on key presence alone.
The nit is fixed — §3.1's paragraph is now uniform at 83-86 columns with no short line.
One residual I chased and am satisfied is a non-issue, recorded so it is not rediscovered as
a surprise: the suffix anchor can match a different tenant's topic when one valid userkey
is a dash-boundary suffix of another, e.g. topic_names_userkey("kev1-oi-alice-gh-bob-1234abcd-requests", "gh-bob-1234abcd") is True. That requires the two keys to share a trailing digest — a
32-bit collision between two real identities, which is precisely the property userkey()'s
digest exists to prevent, and a fabricated key is operator-controlled anyway. No change wanted.
One suggestion left, inline: two of the older userkey_in_topic tests still assert the split
for identity, which is now the one thing its docstring says you may not do.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Python (shared/tenancy.py, eventrunner/config.py), tests, Docs (DESIGN_PHASE3 §3.1/§3.3/§11)
Agent/IDE config (.claude/.vscode): none
Commits: 3 commits, all signed-off: yes
CI status: passing (11/11 on 2e94bcb0)
… split Review: #900, the remaining suggestion on the approval review — the two tests that still pinned the split for identity, the use the new userkey_in_topic docstring rules out. test_userkey_in_topic_round_trips_topicset asserted that whatever TopicSet derives, userkey_in_topic recognises the key back out of it — a round-trip contract the module disowns: it holds only because the fixture prefix "kev1" has no dash, and the ambiguity test twelve lines below asserts the opposite ("prod-eu-…" splits shortest-first). So the file asserted both "the split round-trips" and "the split is arbitrary", and a future narrowing (return None on several valid splits, or a quantifier flip) would fail these two as if regressions when they actually encode a contract the fix repudiated. Narrowed both to the claim that survives, per the reviewer's shape: - renamed to test_userkey_in_topic_recognises_topicset_names_as_per_user; recognition (is not None) plus the anchored round-trip topic_names_userkey(method(uk), uk), which holds for every prefix and slug rather than only the dash-free ones. - test_userkey_in_topic_recognises_each_suffix likewise asserts recognition plus the anchored check, with a docstring pointing at the ambiguity test. The captured key is now asserted nowhere, matching the docstring. eventing suite: 915 passed, 7 skipped. ruff check / format --check: clean, CI-exact invocation. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
2 participants
From the review of all nine
eventing/agentdocs/files. Every fix below was verified against the code before the doc was edited — the repo's own §8.5/§8.6 discipline — and the code fix is reproduced by a test. This PR covers the review's inconsistencies section only; gaps and improvements are separate conversations.1. SIGNED_ATTRS drift (5 sites in DESIGN_PHASE3)
§2.6's opening,§8.3's rule,IMPLEMENTATION_REPORT3's T4 row andshared/signing.py:185-219all say Phase 3 added three attributes toSIGNED_ATTRSin ONE change:userkey,depth,agent. But five other sites in the same document still said two:depthin one change rather than two" sentence)SIGNED_ATTRS+=userkey,depth")All five now name three attributes and the one-change rule.
§8.3itself was already correct and untouched.2. ER_USERKEY false positive — code fix
The runner refused to boot whenever
REQUEST_TOPIC != "requests"andER_USERKEYwas unset. That false-positives every Phase 1 single-tenant deployment that renames the default — and the shippedk8s/base/configmap.yaml:18does exactly that (REQUEST_TOPIC: kev1-requests, noER_USERKEY): a fresh Phase 1 cluster boots no runner at all. Reproduced by executingercfg.load()with the configmap's env.The design's own text (§3.3) says the refusal guards per-user runners. The condition now keys on the per-user topic shape —
{prefix}-{userkey}-requests(§3.1) — via a newtenancy.userkey_in_topic():kev1-requests(renamed default) → boots, single-tenant.kev1-gh-mrsabath-4c1d9e07-requestswith noER_USERKEY→ still refuses.ER_USERKEY→ now also refuses (previously it booted and mis-attributed).EventBridge'sStoreRegistry.for_eventremains the actual filing control, so a false negative in the shape check is counted (unattributed), never guessed.userkey_in_topiclives inshared/tenancy.pynext tois_valid_userkeybecause that module is "the ONLY place an identity becomes a name" — recognising one inside a topic name belongs there. TheUSERKEY_REcore is shared so the recognizer can't drift from the producer.DESIGN_PHASE3.md§3.3 and §8.2 updated to describe the shape-keyed refusal. Tests: 10 new (TopicSet round-trip, each of the four suffixes, the renamed-default regression, mismatch refusal, non-userkey shapes).3.
EB_TOPIC_PREFIXdefault§3.1 said "
{prefix}defaults to the namespace (kev1)" — implying derivation from the namespace. The code has a literal"kev1"default (eventbridge/config.py:91), and §8.1's table row agrees. Reworded to the literal default and why a literal is right (names stay identical to Phase 1's shippedkev1-topics in any namespace).4. README_PHASE1 — two stale claims
submitteris not signed" — false since Phase 2:submitter,submitteriss,groupidjoinedSIGNED_ATTRS(DESIGN_PHASE2 §4.2). Corrected, keeping the honest-limit framing: Kafka is still plaintext; the claim is EventBridge's assertion, not proof of who typed.5. README_PHASE0 — test count
"20 passed in ~0.6s" → Report0's record: 58 passed in 7.27s.
6. Assorted smaller ones
--watchusedaction="store_true", default=True— a combination that can never be false. Nowargparse.BooleanOptionalActionlike the shipped CLI (skills/eventbridge/eventbridge-cli.py:659).ER_VERIFY_KEY_PATHrow — the keyset row already referenced it as the fallback, but no row defined it.kafka_out.py,emit.pyandsigning.py(×2) replaced with the measured 222/227 ms (Report1 §9). KubeCon M4 explicitly flags these as estimates-vs-measured; the convention there is docstrings get corrected in place.Verification
eventingsuite: 907 passed, 7 skipped (baseline 897 + 10 new), Python 3.14.ruff check .andruff format --check .from the repo root, ruff 0.11.4 (CI-exact): clean.🤖 Generated with Claude Code