feat(qa-rig): L0 T0 #6 PR-OPAQUE-6 — Centrifugo opaque-ticket E2E flows (happy path + single-use rejection) #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/l0-t0-opaque-6-e2e-flow"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Header
feat/l0-t0-opaque-6-e2e-flow→mainflows/30-33)TL;DR
CONDITIONAL_APPROVE— ships the two PR-OPAQUE-6 ticket-lifecycle E2E flows (happy path + single-use rejection) as BPMN-first scaffolds with full TOML acceptance contracts, 6 new pytest cases, and a drift-detector that keeps the committed.bpmnsidecars in sync with the TOML source-of-truth.Summary
Adds the two flows the PR-OPAQUE-6 spec requires plus a regression-resistant drift-detector tying the BPMN sidecars back to the TOML via
im2be-qa-rig graphround-trip. Flows are scaffolds (matching the established L-130-33failure-mode pattern) — they use existing action types with detailed# TODO (real-execution):comments documenting the real HTTP/gRPC/Redis/Prometheus assertions verbatim, so a later runner-extension tier can wire them without re-deriving the contract. No scope reduction per rule 52: the assertion contract lives in source.Both flows target realtime-service
/centrifugo/connect(the HTTP-to-gRPC bridge), NOT Centrifugo itself — Centrifugo isn't in the spike target yet, and per ADR-0002 §3a the bridge is the conformance contract. Persona: adult-1 (Anna Kowalska, deterministic UUIDv5b1089187-810a-5a62-89da-75004a4608e3) reusing the existing mock-data fixture per rule 55.Reviewed commit:
ef842ac(single-commit PR).Findings
No new findings this round. This is R0 (initial open) — the autonomous reviewer will surface findings on the first round.
Bookkeeping anchors for future rounds:
flows/21-…toml:1-130flows/22-…toml:1-138tests/test_opaque_ticket_flows.py:test_committed_bpmn_matches_regeneratedflows/21-…toml:18-22+flows/22-…toml:21-22test_storage_state_resolves_to_adult_1Verdict
CONDITIONAL_APPROVEpending autonomous-reviewer R1. Conditions to track:im2be-qa-rig graphafter any TOML step edit).# TODO (real-execution):comment blocks define the contract a later carve-out will wire; do not strip them.Footer
ef842ac~/code/vioxen/qa-rigunchanged🤖 Authored autonomously per L0 T0 #6 plan; review-cycle handoff to the autonomous reviewer.
Adds the two end-to-end flow scaffolds locking the PR-OPAQUE-6 ticket lifecycle as BPMN-first artefacts (rule 57.1): - flows/21-centrifugo-opaque-ticket-happy-path.toml login → mint (POST /token/centrifugo) → connect (POST /centrifugo/connect on realtime-service) → validate-success → single-use Redis-key consumed. Targets the HTTP-to-gRPC bridge directly because Centrifugo itself is NOT in the spike target yet (per ADR-0002 §3a — the bridge is the conformance contract; L2.5 notification-only pilot lands later). - flows/22-centrifugo-opaque-ticket-single-use-rejection.toml login → mint → first-connect (200, consumes ticket) → second- connect with SAME ticket (401 + Disconnect{4401}) → asserts realtime_centrifugo_connect_total{outcome="invalid"} +1. Exercises the ADR-0002 §4 replay-attack defence (GETDEL atomic in identity-service ValidateRpc). Both flows ship as SCAFFOLD (matching flows/30-33 L-1 pattern) — TOMLs use existing action types with detailed `# TODO (real-execution):` comments documenting the real HTTP/gRPC/Redis/Prometheus assertions a later carve-out will wire when the runner gains http+metric step kinds (no scope reduction per rule 52 — the assertion contract is captured in source, not deferred to a future memory). Persona: adult-1 (Anna Kowalska, UUIDv5 b1089187-810a-5a62-89da-75004a4608e3) — reuses the existing mock-data fixture per rule 55. BPMN sidecars generated via `im2be-qa-rig graph` (M3 emitter) and committed next to the TOMLs. test_opaque_ticket_flows.py verifies: - both flows parse cleanly via spec.load_flow_spec - storage-state resolves to adult-1 + UUIDv5 anchor present - committed .bpmn sidecars match what `graph` would regenerate (drift-detector for rule 57.1) - BPMN task count == TOML step count - BPMN documentation surfaces action= attributes (bpmn-js readability) README "Active flows" section added — first explicit per-flow index in this repo, classifying every committed flow EXECUTABLE vs SCAFFOLD. Verification: - uv run pytest -q → 138 passed - uv run ruff check tests/test_opaque_ticket_flows.py flows/21-… flows/22-… → All checks passed - xmllint --noout flows/21-…bpmn flows/22-…bpmn → clean - ( cd ~/code/vioxen/qa-rig && git status -sb ) → unchanged (rule 53)Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-qa-rig)
Round 1 — head
ef842acc9611, basemain, triggeropenedTL;DR: BLOCKED — kept 3 findings (1 blocking agreed by both reviewers, 1 minor verified A-only, 1 info verified A-only); test_storage_state_resolves_to_adult_1 will error the CI test suite on every runner without the im2be-mono co-checkout.
Summary
Reconciliation Summary
No prior run history found in Memora for this PR — first arbitration.
Step 1 — Agreed finding (A ∩ B)
Both A (line 83, severity=blocking) and B (line 86, severity=major) independently flag
test_storage_state_resolves_to_adult_1. Verification confirms:tests/test_opaque_ticket_flows.py:83is the function definition; line 86 callsresolve_storage_state(spec, toml_path).src/im2be_qa_rig/spec.py:59raisesFlowSpecError(f'storage_state.path resolves to nonexistent file: {storage_path}')when the path is absent.path = "../../mock-data/pwa/playwright-storage/adult-1.json", resolving to<repo-parent>/mock-data/…(the adjacentim2be-monocheckout)..forgejo/workflows/qa-rig-ci.yml:53shows thetestjob runs onlyactions/checkout@v4— no monorepo co-checkout. Thesmokejob (lines 87–90) adds theim2be-monocheckout;testdoes not.im2be-monoco-located).Step 2 — Unique-to-A findings (verified)
Minor — byte-for-byte BPMN comparison:
tests/test_opaque_ticket_flows.py:123confirmed:assert regenerated.read_bytes() == bpmn_path.read_bytes().bpmn.py:166usesET.tostring(root)directly afterET.indent. Namespace declaration ordering is dict-insertion-order-stable in CPython 3.7+ but is not guaranteed across minor Python releases, meaning a Python upgrade can silently invalidate this assertion even when the BPMN content is structurally identical. Finding verified and kept as minor.Info — missing Prometheus baseline step:
flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:101confirmed. Lines 111–117 of the TOML TODO describe a pre/post delta assertion requiring a baseline scrape captured before step 4, but the scaffold has no such step (total: 6 steps, 6 BPMN tasks). When themetric_scrapecarve-out inserts a baseline step (→ 7 steps),test_committed_bpmn_has_one_task_per_step(line 134) assertslen(tasks) == len(spec["steps"])and will fail until the BPMN sidecar is regenerated. Finding verified and kept as info with defer note.Disposition
Blast Radius
The diff is entirely additive: two new TOML flow specs, two BPMN sidecars, one new test module, and README documentation. No existing modules are modified. Impact is confined to the qa-rig test suite and flow scaffold corpus; no shared library surface or protocol contracts are touched.
BLAST_SCORE: 2/10
Risk Indicators
CI status (head
ef842acc9611)Overall: ✗ failure
3 checks: 3 pending
Findings (3)
[BLOCKING]
test_storage_state_resolves_to_adult_1will error in CI — monorepo mock-data absent fromtestjobtests/test_opaque_ticket_flows.py:83
Agreed by both A and B.
resolve_storage_stateis called at line 86.src/im2be_qa_rig/spec.py:59raisesFlowSpecError(f'storage_state.path resolves to nonexistent file: {storage_path}')when the resolved path does not exist.Both TOMLs embed
path = "../../mock-data/pwa/playwright-storage/adult-1.json". Resolved from<repo>/flows/, that is one directory above the qa-rig checkout — i.e. inside the adjacentim2be-monorepo..forgejo/workflows/qa-rig-ci.yml:53shows thetestjob runs a plainactions/checkout@v4only. Thesmokejob (lines 87–90) explicitly addsaffinity-intelligence-rework/im2be-monoas a second checkout; thetestjob does not. On any runner withoutim2be-monoco-located the test exits with an error (not a skip, not a soft failure), breaking the green gate.Minimum-impact fix (no CI change): add a skip guard:
Stronger fix: add the monorepo checkout step used by the
smokejob to thetestjob in.forgejo/workflows/qa-rig-ci.ymlso the assertion runs on every CI push.[MINOR] Byte-for-byte BPMN comparison is fragile across Python upgrades
tests/test_opaque_ticket_flows.py:123
write_bpmn(bpmn.py:166) callsET.tostring(root)afterET.indent. CPython 3.7+ serialises element attributes in dict-insertion order, which is stable within a release but not guaranteed to remain identical across minor Python upgrades — especially for namespace declarations that ElementTree adds implicitly. A Python version bump would silently break this test even when the BPMN content is structurally unchanged.Prefer a semantic comparison using
ET.canonicalize(stdlib since Python 3.8):This normalises attribute order and whitespace, making the comparison robust to serialisation differences while still catching structural BPMN drift.
[INFO] Scaffold missing pre-baseline capture step for Prometheus delta assertion — carve-out will break
test_committed_bpmn_has_one_task_per_stepflows/22-centrifugo-opaque-ticket-single-use-rejection.toml:101
Step 5's TODO (lines 101–117) describes a delta comparison (post − pre = +1), which requires a baseline metrics scrape before step 4 (the second connect). No such step exists in the scaffold — the flow currently has 6 steps and the BPMN sidecar has 6
<bpmn:task>elements.When the
metric_scrapestep kind lands, the carve-out must insert acapture_metrics_baselinestep between the current steps 3 and 4 (→ 7 total).test_committed_bpmn_has_one_task_per_step(test_opaque_ticket_flows.py:134) assertslen(tasks) == len(spec["steps"])and will fail until the.bpmnsidecar is regenerated.Suggested addition to the step 5 TODO comment:
Verdict
BLOCKED
hib-pr-reviewer • round 1 • 3 findings (1b/1m/1i) • 2026-05-25T00:18:53.350Z → 2026-05-25T00:21:46.198Z • posted-as: pr-reviewer-bot
R1 verdict findings (1 BLOCKING + 1 MINOR + 1 INFO; kept=3): (1) BLOCKING tests/test_opaque_ticket_flows.py:83 — test_storage_state_resolves_to_adult_1 dereferenced ../../mock-data/pwa/playwright-storage/adult-1.json which lives in the adjacent im2be-mono repo. The `test` Forgejo Actions job (.forgejo/workflows/qa-rig-ci.yml) uses plain actions/checkout@v4 only — no monorepo co-checkout — and would ERROR (not fail-clean) on lean runners. Fix: resolve the fixture path at module import time + gate the parametrised test with @pytest.mark.skipif(not _ADULT1_STORAGE_PATH.exists(), reason=...). Reason string cites the `smoke` vs `test` job asymmetry so a future reader knows why the skip exists. Lean `test`-job runners now collect-and-skip instead of error; meta-repo-coupled runs continue to enforce the persona-drift invariant. (2) MINOR tests/test_opaque_ticket_flows.py:123 — byte-for-byte BPMN comparison via .read_bytes() is fragile across Python upgrades (CPython element-attribute serialisation order is stable WITHIN a release but NOT GUARANTEED across minor versions). Fix: replace with semantic comparison via stdlib xml.etree.ElementTree.canonicalize (C14N XML Canonicalisation, available Python 3.8+). New helper _canonical_bpmn(path) invokes ET.canonicalize(..., strip_text=True) so insignificant inter-element whitespace (pretty-print indent) doesn't trip the comparison. The contract-level invariant — same XML information set → same canonical form — is what we actually want to assert, and is forward-compatible with future ET writer changes. (3) INFO flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:101 — step 5's TODO describes a delta assertion (post − pre = +1) but no baseline-capture step exists before step 4. The carve-out PR that introduces the metric_scrape step kind must INSERT a baseline-capture step between current steps 3 and 4 (6 → 7 steps), which will require regenerating the BPMN sidecar. Fix: add a NOTE block to step 5's TODO comment block documenting (a) the required insertion point + (b) the exact `im2be-qa-rig graph` command to regenerate the BPMN + (c) which tests would break if the carve-out forgets the regeneration (test_committed_bpmn_has_one_task_per_step + test_committed_bpmn_matches_regenerated). Comment-only change in the TOML — flow steps unchanged, BPMN sidecar still matches. Verification: - uv run pytest -q → 138 passed (10 from this file) - uv run pytest tests/test_opaque_ticket_flows.py -v → 10 passed (skipif gate runs) - uv run ruff check tests/test_opaque_ticket_flows.py flows/21-… flows/22-… → All checks passed - test_committed_bpmn_matches_regenerated[happy-path/single-use] both pass under ET.canonicalize comparison — confirms the canonical-form invariant holds against both committed sidecars at HEAD. - ( cd ~/code/vioxen/qa-rig && git status -sb ) → unchanged (rule 53).Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-qa-rig)
Round 2 — head
112583751eb6, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept all 4 findings (1 info, 3 minor; 1 deferred); all verified by direct file read; no blocking issues.
Summary
Round 2 arbitration — no prior Memora history found (create-new path, review_count=2).
Kept all 4 findings after direct file verification. No findings were dropped.
A-unique (3 findings):
tests/test_opaque_ticket_flows.py:161(info):path.read_text()confirmed without encoding argument —ET.canonicalize(path.read_text(), strip_text=True). KEPT.</definitions>with no trailing\n; the diff\ No newline at end of filemarker corroborates. KEPT.B-unique (1 finding):
flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:130(minor): Confirmedname = "prometheus counter realtime_centrifugo_connect_total{outcome=invalid} +1"— unquoted label value; line 107 TODO comment correctly uses{outcome="invalid"}. Grep on the BPMN confirmed the bad syntax propagates totask_5name. KEPT.Memora run summary persisted as memory #238 (new record, review_count=2).
Blast Radius
All 6 touched files are net-new additions (2 BPMN sidecars, 2 TOML flow specs, 1 test module, 1 README update). No existing production code is modified. The BPMN artifacts serve as specification anchors for future carve-out PRs, giving them modest forward-blast radius if the schema conventions are misread.
BLAST_SCORE: 3/10
Risk Indicators
CI status (head
112583751eb6)Overall: ✗ failure
3 checks: 3 pending
Findings (4)
[INFO]
path.read_text()lacks explicitencoding="utf-8"in_canonical_bpmntests/test_opaque_ticket_flows.py:161
Line 161:
return ET.canonicalize(path.read_text(), strip_text=True)Path.read_text()with no encoding argument falls back tolocale.getpreferredencoding(False). The current BPMN files are pure ASCII (non-ASCII characters like→are entity-escaped as→), so this is harmless today. However, any future flow with a literal non-ASCII byte in a task name (e.g. a Unicode em-dash or a persona name in Cyrillic) written by the generator will be mis-decoded on a non-UTF-8 CI locale, producing a different canonical form from the regenerated file and breakingtest_committed_bpmn_matches_regeneratedwith a confusing failure.Fix:
path.read_text(encoding="utf-8")— one-character change, zero risk.[MINOR] Both committed BPMN sidecars are missing a trailing newline
flows/21-centrifugo-opaque-ticket-happy-path.bpmn:77
Line 77 of
flows/21-centrifugo-opaque-ticket-happy-path.bpmnand line 88 offlows/22-centrifugo-opaque-ticket-single-use-rejection.bpmnboth end with</definitions>and no trailing\n(confirmed by direct Read of both files and the\ No newline at end of filediff marker).POSIX defines a text file as a sequence of lines each terminated by
\n; without it,git diff --check, many pre-commit hooks, and some XML processors emit warnings. Thetest_committed_bpmn_matches_regeneratedtest is unaffected because C14N canonicalization strips trailing whitespace from both sides.Fix: add a single
\nat EOF in both files and fix theim2be-qa-rig graphemitter to write a trailing newline so re-generated sidecars stay clean.[MINOR] Carve-out must insert baseline-capture step AND regenerate BPMN or
test_committed_bpmn_has_one_task_per_stepwill failflows/22-centrifugo-opaque-ticket-single-use-rejection.toml:101
The TODO block (lines 119-129, verified) correctly documents that wiring the Prometheus delta assertion requires inserting a new baseline-capture step between the current steps 3 and 4, raising the total from 6 to 7 steps. If the carve-out PR adds that TOML step but omits
im2be-qa-rig graphregeneration,test_committed_bpmn_has_one_task_per_stepwill fail (BPMN has 6 tasks, TOML has 7 steps) andtest_committed_bpmn_matches_regeneratedwill also fail.Deferred — out of scope for this scaffold PR. Must be tracked as a tech-debt issue so it is not lost when the carve-out is scoped.
[MINOR] Step name uses
{outcome=invalid}— missing quotes makes it invalid Prometheus label-selector syntaxflows/22-centrifugo-opaque-ticket-single-use-rejection.toml:130
Line 130 (verified):
The label value is unquoted. Valid PromQL / OpenMetrics label-selector syntax requires double-quoted values:
{outcome="invalid"}. The step name propagates verbatim into the BPMN task name (confirmed by Grep:flows/22-...bpmnline 17 showstask_5with the same unquoted form), so the diagram — the primary specification artifact — shows invalid syntax. The TODO comment at line 107 already uses the correct form:realtime_centrifugo_connect_total{outcome="invalid"}.Fix: change the step name to use
{outcome="invalid"}then re-runim2be-qa-rig graph --spec flows/22-....toml --output flows/22-....bpmnand commit the regenerated sidecar.test_committed_bpmn_matches_regeneratedwill guard the sidecar stays in sync.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 4 findings (3m/1i) • 2026-05-25T00:30:12.942Z → 2026-05-25T00:32:25.449Z • posted-as: pr-reviewer-bot
R2 verdict findings (kept=4): (1) INFO tests/test_opaque_ticket_flows.py:161 — _canonical_bpmn used path.read_text() without explicit encoding (locale-dependent fallback breaks non-UTF-8 CI locale once future flows add non-ASCII bytes). (2) MINOR flows/{21,22}-…bpmn — both committed sidecars missing EOF \n (POSIX text-file convention; git diff/cat warning surface). Emitter write_bpmn() also lacked the trailing newline. (3) MINOR — step-5 NOTE already adequate from R2; no further wording tightening needed. (4) MINOR flows/22-…toml:130 — step name used unquoted PromQL label `{outcome=invalid}`; invalid OpenMetrics syntax. The TODO comment at line 107 already showed the correct `{outcome="invalid"}` form. Fixes: (1) tests/test_opaque_ticket_flows.py:169 path.read_text() → path.read_text(encoding="utf-8"); docstring expanded with rationale. (2) src/im2be_qa_rig/bpmn.py write_bpmn() — output.write_bytes(...) now appends b"\n" after ET.tostring(root); docstring updated. Both flows/{21,22}-…bpmn regenerated with terminating \n (xxd confirms 0x0a at EOF). (3) No change. (4) flows/22-…toml:130 step-5 name uses single-quoted TOML string so the inner `outcome="invalid"` double-quotes survive. BPMN sidecar regenerated for flow 22. Verification: - uv run pytest -q: 138 passed (unchanged from R2; new code paths exercised by existing test_committed_bpmn_matches_regenerated). - tail -c1 flows/{21,22}-…bpmn: both 0x0a.Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-qa-rig)
Round 3 — head
146f7e9fcf85, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed minor (encoding at line 111), 1 verified unique-to-A minor (deferred baseline-capture placeholder), 1 verified unique-to-B info (dangling docstring PR reference); no blocking issues.
Summary
Reconciliation — Round 3
Memora: No prior run history found in store (first-time create at ID 239,
review_count: 3). Both reviewer findings sets A and B are treated fresh against round 3 diff.Agreed finding (rules 1 + 3): Both A and B independently flag
tests/test_opaque_ticket_flows.pyline 111 forcfg.path.read_text()missingencoding="utf-8". Verified by Read: line 111 isstorage_text = cfg.path.read_text()(bare call). Contrast at line 169 confirmed:path.read_text(encoding="utf-8")— same file, same PR. Kept.Unique-to-A (rule 2):
flows/22-centrifugo-opaque-ticket-single-use-rejection.tomlline 101 — no baseline-capture placeholder step in scaffold. Verified by Read: lines 101–133 show the Prometheus counter step as step 6 with TODO at lines 119–129 explicitly warning that wiring the delta assertion requires inserting a step between current steps 3 and 4 (raising total from 6→7), followed by mandatory BPMN sidecar regeneration. No placeholder exists. Kept with deferred disposition.Unique-to-B (rule 2):
src/im2be_qa_rig/bpmn.pylines 165–167 —write_bpmndocstring hard-codesPR #5 R2provenance reference. Verified by Read: lines 165–167 contain "Reviewer-observed regression in PR #5 R2: original emitter omitted EOF\\n…" — opaque to readers after merge. Functional rationale (POSIX text-file convention) is self-contained in lines 163–164; the PR reference adds no durable value. Kept at info severity.All 5 prior-round items (R1/R2) confirmed resolved per both reviewers' R3 summaries; no prior-round findings re-raised.
Blast Radius
PR adds two new scaffold flow files (TOML + BPMN pairs) and a small fix to the BPMN emitter helper plus a new test module. Changes are self-contained in the QA-rig layer with no shared library or auth-boundary modifications. The only cross-module surface is
src/im2be_qa_rig/bpmn.py, which is internal to the rig.BLAST_SCORE: 3/10
Risk Indicators
CI status (head
146f7e9fcf85)Overall: ✗ failure
3 checks: 3 pending
Findings (3)
[MINOR]
cfg.path.read_text()missing explicitencoding="utf-8"tests/test_opaque_ticket_flows.py:111
Line 111 reads
adult-1.jsonwithout a pinned encoding:storage_text = cfg.path.read_text(). The same file fixed this pattern in this PR at line 169 (path.read_text(encoding="utf-8")inside_canonical_bpmn). The immediate assertion (ADULT_1_UUID in storage_text) is ASCII-only and safe today, butread_text()without encoding falls back tolocale.getpreferredencoding(False)— on a non-UTF-8 CI locale, a futureadult-1.jsonfixture with non-ASCII bytes (display name, locale field, token payload) raisesUnicodeDecodeErrorinstead of a clean assertion failure, masking persona-drift issues.Fix (one-line, identical pattern to the round-2 fix at line 169):
[MINOR] No baseline-capture placeholder step in scaffold — carve-out PR must perform two operations atomically
flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:101
Flow 22 currently has 6 TOML steps and 6 BPMN tasks (in sync, verified). The TODO comment at lines 119–129 correctly warns that wiring the Prometheus delta assertion requires inserting a separate baseline-capture step between current steps 3 and 4, raising the total to 7, then re-running
im2be-qa-rig graphto regenerate and recommit the.bpmnsidecar. Without a placeholder step today, the carve-out PR cannot add just the step logic — it must add the TOML step AND regenerate the BPMN atomically; if either half is missed,test_committed_bpmn_has_one_task_per_stepfails (7 TOML steps vs 6 BPMN tasks).Recommended fix for the carve-out: insert between current steps 3 and 4:
Then re-run
im2be-qa-rig graph --spec flows/22-centrifugo-opaque-ticket-single-use-rejection.toml --output flows/22-centrifugo-opaque-ticket-single-use-rejection.bpmnand recommit the sidecar.Deferred — adequately documented in the TOML TODO; filing as tech-debt so the reminder survives merge.
[INFO]
write_bpmndocstring hard-codesPR #5 R2— dangling reference after mergesrc/im2be_qa_rig/bpmn.py:165
Lines 165–167 of the
write_bpmndocstring read:This PR-review provenance is opaque after merge — a reader must retrieve this specific review round to interpret the note. The functional rationale (POSIX text-file convention, linter/git-diff/cat cleanliness) is already fully stated in lines 163–164 and is self-contained. Dropping the
PR #5 R2clause produces a cleaner docstring without loss of information:Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 3 findings (2m/1i) • 2026-05-25T00:44:04.960Z → 2026-05-25T00:46:14.039Z • posted-as: pr-reviewer-bot
R3 verdict findings (kept=3; 2 fixed in this commit, 1 deferred): (1) MINOR tests/test_opaque_ticket_flows.py:111 — `cfg.path.read_text()` in test_storage_state_resolves_to_adult_1 missed the encoding="utf-8" fix that was applied to `_canonical_bpmn` at line 169 in R2. Same failure mode (locale-dependent decode on non-UTF-8 CI; UnicodeDecode- Error masks persona-drift assertion). (2) MINOR — Carve-out atomic-operation reminder. Reviewer agreed the TOML TODO at lines 119-129 already documents this adequately. No code change needed; deferred to the PR-OPAQUE-6-FOLLOWUP scope. (3) INFO src/im2be_qa_rig/bpmn.py:165-167 — write_bpmn docstring hard-coded `PR #5 R2` reference. PR provenance is opaque after merge; functional rationale (POSIX text-file convention) is already stated in the preceding sentence and is self-contained. Fixes: (1) Added `encoding="utf-8"` to the `cfg.path.read_text()` call. (3) Dropped the `Reviewer-observed regression in PR #5 R2: original emitter omitted EOF \\n, causing both committed BPMN sidecars to lack a trailing newline.` clause from the docstring. Preserved the functional rationale (POSIX convention + linter/git-diff/cat cleanliness). Verification: - `uv run pytest -q`: 138 passing (unchanged). - Two surgical text changes; no behaviour delta.hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-qa-rig)
Round 4 — head
be401bd07600, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 unique-to-B minor/deferred finding (verified); no blocking issues; all R1–R3 findings confirmed resolved.
Summary
Round 4 Arbitration — A=0 findings, B=1 finding (minor/deferred)
Reconciliation actions:
flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:119— baseline-capture placeholder absent; atomicity constraint documented in NOTE but not structurally enforced.Readat lines 110–137. Confirmed: lines 119–129 contain exactly the NOTE comment B describes — it documents the 6→7 step atomicity requirement and the BPMN regeneration command, but no placeholder[[steps]]entry exists in the current scaffold.test_committed_bpmn_has_one_task_per_stepandtest_committed_bpmn_matches_regenerated.Kept=1 (minor, deferred). Dropped=0.
Blast Radius
The diff adds 7 files spanning flows/, src/, and tests/. All changes are new QA spec artifacts (TOML flow definitions, BPMN sidecars) and accompanying test infrastructure — no shared library surface or protocol contracts modified. Impact is scoped to the qa-rig's own test suite and the two new flow specs.
BLAST_SCORE: 3/10
Risk Indicators
CI status (head
be401bd07600)Overall: ✗ failure
3 checks: 3 pending
Related PRs
Findings (1)
[MINOR] Baseline-capture placeholder step absent — atomicity constraint documented in NOTE but not enforced structurally
flows/22-centrifugo-opaque-ticket-single-use-rejection.toml:119
Lines 119–129 carry a NOTE that the carve-out PR must insert a baseline-capture step between current steps 3 and 4, growing the flow from 6 to 7 steps, AND must atomically regenerate the committed BPMN sidecar. The current scaffold is self-consistent (6 TOML steps, 6 BPMN tasks;
test_committed_bpmn_has_one_task_per_steppasses today). The structural risk: a carve-out author who adds the[[steps]]entry but omits the BPMN regeneration will silently break bothtest_committed_bpmn_has_one_task_per_stepandtest_committed_bpmn_matches_regenerated. Mitigation is documentation-only. Recommend the carve-out PR checklist explicitly mandate: ① add baseline-capture[[steps]]between steps 3 and 4, ② runim2be-qa-rig graph --spec flows/22-centrifugo-opaque-ticket-single-use-rejection.toml --output flows/22-centrifugo-opaque-ticket-single-use-rejection.bpmn, ③ commit both files atomically. Deferred to carve-out PR to own.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 4 • 1 finding (1m) • 2026-05-25T00:52:31.006Z → 2026-05-25T00:54:22.168Z • posted-as: pr-reviewer-bot