Skip to content

fix(v2): preserve permission request wire shapes - #40

Merged
alderpath merged 7 commits into
Reliary:mainfrom
jrzmurray:fix/v2-permission-shape
Nov 14, 2026
Merged

alderpath merged 7 commits into
Reliary:mainfrom
jrzmurray:fix/v2-permission-shape

Conversation

@jrzmurray

Copy link
Copy Markdown
Contributor

What this changes

The v1 and v2 HTTP surfaces share the permission gate, but the gate stores different field names for each API family. Requests and events now project into the frozen v1 or v2 shape at the boundary. The shared always-grant logic accepts both field vocabularies, and v2 permission creation rejects malformed fields and values before storing a request.

Issue

No linked issue. Serves K-V2-PERM-SHAPE.

Verification

  • cargo fmt --check — pass
  • cargo clippy --workspace -- -D warnings — pass
  • cargo test -p ocserve-core permission — pass
  • cargo test -p ocserve-http --test v2_surface — pass (17 tests)
  • cargo test -p ocserve-http --lib v2_permission_event_projects_the_permission_request_shape — pass
  • uv run --with jsonschema bash ./scripts/check-guards.sh — pass
  • ./scripts/check-matrix.sh — pass
  • cargo test --workspace — macOS run fails in Linux /proc and cgroup-dependent tests; the subsequent broad run also hit shutdown::sigterm_logs_the_drain_line before reaching the HTTP suite. Linux PR CI must confirm the workspace gate.

Checklist

  • A bug fix has a test that fails without the fix (negative control)
  • A crates/**/*.rs change stages a TRACEABILITY.md row
  • New behaviour is named in CHANGELOG.md
  • No personal names, email addresses or absolute home paths
  • Commits are conventional (feat:, fix:, docs:, test:, chore:)

jrzmurray and others added 4 commits November 13, 2026 01:10
The v1 and v2 APIs share a permission gate, but its stored request shape was being serialized directly at both boundaries. Project requests and events into each frozen schema, accept either field vocabulary for shared always-grants, and reject malformed v2 payloads before persistence.

Serves K-V2-PERM-SHAPE. The malformed-field regression was observed red before validation was tightened; tests pin the v1 and v2 read shapes, event shape, and always-grant cascade.

Local verification: fmt, clippy, permission-focused tests, guard script, and traceability matrix pass. Full workspace tests on macOS hit existing Linux /proc and cgroup assumptions; Linux PR CI must confirm the workspace gate.
V2 permission creation must store its own action/resources/save/source shape. Boundary projectors own the v1 and v2 translations; storing duplicate v1 fields made the gate and always-grant paths ambiguous. The session detail route also needs the session id to enforce request ownership.

Serves K-V2-PERM-SHAPE. The v2 surface regressions cover create, both read grammars, event projection, and the shared always-grant cascade.
The permission PR included the adjacent drain acceptance row because both additions shared one patch hunk. Remove that row here so the permission PR documents only the permission behavior; the drain branch adds it with its implementation and tests.
Amendment to the contributor's branch (credited @jrzmurray), reviewed
before merge on the evidence each arm provides:

  * DELETE the `v2_twin("permission.asked")` arm. permission.asked is emitted
    only through emit_durable/emit_durable_http, and neither calls v2_twin, so
    the arm was unreachable and its test proved a path production never takes.
    The real v2 projection is the /api/event encoder (encode_event_frame),
    which the envelope test already covers. A new assertion keeps the dead arm
    from returning; the encoder test carries a proven negative control
    (remove the projector -> "left: Null, right: \"bash\"").
  * add D-PERM-SOURCE-KEY and D-PERM-EVENT-NAME to the ledger. The two frozen
    contracts disagree (v1 PermissionV2Source is callID, v2 Permission.Source is
    id; event name permission.v2.asked at 1.19.5 vs permission.asked at 2.2.5),
    and both are additionalProperties:false, so no single body satisfies both.
    The row also records why the event-name split is currently unreachable:
    the 1.19.5/2.2.5 web app probes /global/health and treats {healthy:true} as
    v1, so it consumes the v1 payload.
  * add v2_created_ask_is_readable_in_both_grammars: drives the real v2 create
    handler and reads the stored ask back through BOTH grammars, asserting
    source.id on v2 and tool.callID on v1. The existing test seeded the gate
    directly, so it never exercised the create path.

Requirement: K-V2-PERM-SHAPE. Verified: v2_surface 18/18, core lib 162/162,
fmt, matrix.
@alderpath

Copy link
Copy Markdown
Collaborator

Thanks — this is a careful piece of work and the shape reasoning is right.

I approved the workflows (fork runs had never executed; CI was also red on main for an unrelated compiler-roll reason, now fixed in #45) and then pushed two commits onto your branch via maintainer edit. Say the word if you would rather have them as a separate PR and I will split them out.

What I changed and why

  1. Removed the v2_twin("permission.asked") arm. permission.asked is emitted only through emit_durable/emit_durable_http, and neither calls v2_twin — so that branch is unreachable and its test proves a path production never takes. The live projection is your encode_event_frame change, which stays and is what the envelope test covers. I added an assertion that v2_twin("permission.asked", …) stays None so the dead arm cannot come back.

  2. Added two ledger rows, no behaviour change. D-PERM-SOURCE-KEY: 1.19.5's PermissionV2Source is {type,messageID,callID} and 2.2.5's Permission.Source is {type,messageID,id}, both additionalProperties:false, so no single body satisfies both — your per-family projection is the correct answer and now has a name. D-PERM-EVENT-NAME: permission.v2.asked (1.19.5) vs permission.asked (2.2.5); I recorded that it is currently unreachable because the 1.19.5/2.2.5 web app probes /global/health and treats {healthy:true} as v1.

  3. Added a test that drives the real create path. The existing test seeded the gate directly, so it could not catch a create-path regression. v2_created_ask_is_readable_in_both_grammars POSTs to /api/session/{id}/permission, then reads the same ask back on both routes: source.id on v2, tool.callID on v1.

One correction to my own earlier review, for the record: I had claimed this PR wrongly rejects message on create. It does not — the create body schema is additionalProperties:false and message is a read-side field. Your handling is correct.

The event-name matrix is now a named divergence rather than an unproven claim. If you want the 1.19.5-era event name emitted as well, that is a new decision, not a bug fix, and I would want the live consumer first.

Comment-only: rustdoc's doc_lazy_continuation (new in 1.100) rejects the
paragraph without a blank line. No behaviour change, no requirement change.
@alderpath
alderpath merged commit 258cfff into Reliary:main Nov 14, 2026
15 checks passed
alderpath added a commit to jrzmurray/ocserve that referenced this pull request Nov 14, 2026
Kept both sides in CHANGELOG, PLAN section 18 and TESTING 1.6: the permission
rows from Reliary#40 and the drain/delete rows from this branch are independent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants