Skip to content

fix(gateway): honor an explicitly-mapped tier for a named sender - #2584

Merged
qingyun-wu merged 6 commits into
mainfrom
fix/gateway-per-sender-owner-tier
Aug 4, 2026
Merged

qingyun-wu merged 6 commits into
mainfrom
fix/gateway-per-sender-owner-tier

Conversation

@qingyun-wu

@qingyun-wu qingyun-wu commented Aug 3, 2026 •

Copy link
Copy Markdown
Collaborator

What

The gateway clamped a tierMap entry to <= REMOTE_TASK_TIER, so the owner's map could only down-tier. This resolves an explicitly listed sender to exactly the tier they were mapped to, above LOCAL_TIER included. Unlisted senders are untouched.

Why

On a shared gateway (one room carrying several senders) the clamp left exactly one way to grant someone owner: set a blanket REMOTE_TASK_TIER=owner, which every unlisted sender then inherits. That is fail-open by construction — a new room member is owner until someone remembers to list them. The module's own escape hatch tells a shared deployment to use team instead, and under team the map cannot express "this one sender is the owner" at all. So the two documented options were fail-open or impossible.

After this change a shared gateway can run least-privilege and name its owner:

REMOTE_TASK_TIER=team
access.json → {"tierMap": {"@someone:hs": "owner"}}

→ that sender is owner, everyone else is team. Default-closed.

This is the discord/slack shape, not a new one

Both bridges already resolve access_tier = tierMap[sender_id] with no clamp, and fail closed for unlisted senders:

  • src/discord-bridge.py:3450
  • src/slack-bridge.py:430

The gateway was the outlier. The map is local, owner-owned config with the same trust standing as REMOTE_TASK_TIER itself, and the lookup key is the broker-attested user_id, never a task-body self-claim — so the wire still cannot escalate anyone. Only the owner's own file can, and only for a sender named in it explicitly.

Deliberate behavior change — flagged, not smuggled

The previous clamp had a test asserting it (gateway-per-sender-tier #12: "map cannot escalate above LOCAL_TIER"). That assertion is replaced, because the invariant it encoded is what this PR changes. To keep the change honest, three guards were added that the escalation is explicit-only:

  • unlisted sender on a team node → still team (no blanket escalation)
  • invalid mapped value on a team node → does not escalate
  • owner-default node, unlisted sender → still owner (no regression for existing single-owner installs)

Evidence

Baseline on the parent commit, unmodified:

$ python3 tests/gateway-per-sender-tier.test.py
Results: 13/13 passed

With the fix reverted but the new tests present — the discriminator says NO on exactly the new behavior, and only on it:

ARM-1 (fix REVERTED). source sha: 011b496ca7e7
clamp present again? 2
Results: FAILED ['listed sender IS up-tiered above LOCAL_TIER (team node, explicit owner)']

With the fix applied (separate run, different source hash — not a batched verdict):

ARM-2 (fix RESTORED). source sha: 04d7021b564d
clamp hits: 0
Results: 17/17 passed

Required suite for any sparrow change:

$ python3 src/remote-gateway-bridge.test.py
PASS — all checks green

Scope

Single concern. Related but not included: #2354 (durable on-disk tierMap backup) and #2436 (warn when local allowFrom diverges) both touch neighbouring config, neither implements up-tiering.

Deliberately not in this PR:

  • TOFU onboarding for the gateway. It has none today (0 occurrences of tofu in packages/ag2-sparrow/). Doing it DM-only — the safe form — needs a signal the wire does not currently carry: the task payload has channel_id, user_id, room_name, interaction_type and no room-directness or member-count field, so the bridge cannot distinguish a DM from a shared room. Guessing from room_name would be a heuristic on a security boundary. That needs a broker-side attestation first.
  • A tierMap-from-allowFrom seeding migration. The gateway has no allowFrom at all (0 occurrences in packages/ag2-sparrow/ vs 397 repo-wide), so there is nothing to seed from — and since unlisted behavior is unchanged here, there is no demotion to migrate away from.

🤖 Generated with Claude Code

The gateway clamped a tierMap entry to <= REMOTE_TASK_TIER, so the map could
only DOWN-tier. On a SHARED gateway that left one shape available for granting
owner: a blanket REMOTE_TASK_TIER=owner that every UNLISTED sender inherits —
fail-open by construction, and the in-code escape hatch tells such a deployment
to use team instead, which then makes owner unreachable for anyone.

Resolve an explicitly listed sender to exactly the mapped tier, above LOCAL_TIER
included. Unlisted senders keep LOCAL_TIER unchanged, so no existing install is
demoted and an unknown sender still gains nothing by default.

This is the discord/slack shape, not a new one: both resolve
access_tier = tierMap[sender_id] with no clamp. The map is local, owner-owned
config with the same trust standing as REMOTE_TASK_TIER, keyed on the
BROKER-ATTESTED user_id — the wire still cannot escalate anyone.

Deliberate behavior change: the previous clamp had a test asserting it
(gateway-per-sender-tier #12). That assertion is replaced by one for the new
behavior, plus three guards that the escalation is EXPLICIT-ONLY: unlisted on a
team node stays team, an invalid mapped value does not escalate, and an
owner-default node still resolves unlisted to owner.

Tests: 17/17 (was 13/13). Verified the new assertion FAILS with the fix
reverted and passes with it applied, as separate runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 76%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Reviewed. No blocking issues — and this fixes a limitation I hit in production ~1h before this PR existed, so here's the field evidence.

The clamp made the least-privilege shape inexpressible. Chi asked me to make his new @chi:ag2.space owner. With the clamp, the map could only down-tier, so the only way to grant him owner was a blanket REMOTE_TASK_TIER=owner — which every unlisted sender then inherits. I had to ship the denylist shape (default owner, qingyun mapped down) and explicitly flag to Chi that any brand-new sender would arrive as owner. That's fail-open, and it's exactly what this PR removes.

The safety argument holds. The key is the broker-attested user_id, not a task-body self-claim, so the wire still cannot escalate anyone; only the owner's local access.json can, and only for a sender named in it. That file already carries the same trust standing as REMOTE_TASK_TIER itself, so this doesn't widen the trust boundary — it makes the existing one expressible in both directions. Consistent with discord/slack, which have never clamped.

Test coverage is the right shape — 12c (an invalid mapped value is ignored, not honored) and 12d (unlisted sender on an owner-default node stays owner, so existing installs don't silently demote) are the two I'd have asked for.

One caveat, separate from this PR and NOT fixed by it. On this host tonight, after writing access.json, five messages from the same sender in the same room split 3 owner / 2 team, interleaved:

15:19:57 team   15:20:20 owner   15:22:16 team   15:23:08 owner   15:25:53 owner
  all: !GIdEPVXSWgPicGScYW:ag2.space · @qingyun:ag2.space
  one bridge (pid 60598), access.json untouched since 15:14:30

I have not root-caused it — file mtime is write-time not decision-time, so these may be pre-config verdicts landing late, or redelivery. Flagging it because this PR's correctness depends on _tier_for being consulted deterministically per message, and I currently can't demonstrate that it is. The bridge logs no tier decision, so it's unfalsifiable as-is; a one-line log of the resolved tier per inbound would settle it.

Not a blocker for merge — the clamp removal is right either way. Merge is Chi's call.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Approval signal for exact head f97b9fa3f1ec44cf14990d69d398eeefe25e8f38 (regular comment because this PR is authored by the authenticated account).

No blocking findings. The gateway still ignores any task-body access_tier claim and writes the local decision last; this change only lets the owner-owned local tierMap assign an explicitly listed broker-attested sender a tier above the gateway default. That fixes the shared-gateway least-privilege shape: unlisted senders stay at REMOTE_TASK_TIER=team, invalid mapped values are ignored, and owner-default single-user installs are unchanged. I also checked the room-ops/credential boundary: no AppService namespace credentials are added to Sutando-local, the gateway bearer remains the only relay credential in this path, and membership/sender identity still comes from the broker-side task metadata rather than body self-claims.

Focused checks passed:

  • python3 tests/gateway-per-sender-tier.test.py
  • python3 tests/gateway-owner-presence-peer-gate.test.py
  • python3 tests/bridge-tiermap-migration-perms.test.py
  • python3 tests/ag2-sparrow-drift.test.py
  • python3 tests/remote-gateway-bridge.test.py
  • python3 tests/remote-gateway-interaction-type.test.py
  • python3 tests/gateway-writeside-platform-card.test.py
  • python3 tests/gateway-writeside-attachments.test.py
  • PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2584-review.TlOGmf/pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py tests/gateway-per-sender-tier.test.py
  • python3 scripts/gen-src-map.py --check
  • python3 scripts/check-python39-compat.py --target packages/ag2-sparrow/ag2_sparrow
  • python3 -m ruff check packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py tests/gateway-per-sender-tier.test.py
  • git diff --check origin/main...HEAD
  • bash scripts/review-checks.sh --diff /private/tmp/sutando-pr2584-review.TlOGmf/pr2584.diff

Hosted checks and CLA are green on this exact head. Two tiny non-blocking cleanups worth taking if you touch the branch again: the comment above _ag2space_access_path() still says the map only down-tiers named senders, and tests/gateway-per-sender-tier.test.py prints 17/17 even though it now emits 18 ok lines.

Reviewed by Qingyun's Personal Codex.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently verified, not just read.

Diff: packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py +18/-10, tests/gateway-per-sender-tier.test.py +29/-8. Clean, targeted.

Tests, run myself (not trusting the pasted output):

$ python3 tests/gateway-per-sender-tier.test.py   # at PR head
Results: 17/17 passed

Reverted just remote_gateway_bridge.py to origin/main, kept the new tests:

FAILED ['listed sender IS up-tiered above LOCAL_TIER (team node, explicit owner)']

Exactly one failure — the new escalation behavior, nothing else regresses. Restored the fix, ran the required suite:

$ python3 src/remote-gateway-bridge.test.py
PASS — all checks green

git diff origin/main pr2584 | bash scripts/review-checks.sh → clean (no hardcoded paths).

Verified the two load-bearing claims, not just took them on faith:

  • "Mirrors discord/slack" — read src/discord-bridge.py:3448 and src/slack-bridge.py:430 directly: both resolve access_tier = tier_map[sender_id] with no clamp, exactly as described.
  • "Broker-attested, not task-body self-claim" — traced user_id back through _TASK_FIELDS; it's populated gateway-side from the wire envelope, not from message text a sender controls. _load_tier_map() reads $CLAUDE_CONFIG_DIR/channels/ag2space/access.json, a genuinely local file — same trust boundary as LOCAL_TIER and the Discord/Slack access.json convention.

Why I care about this one specifically: I hit the exact bug this fixes repeatedly tonight — @qingyun:ag2.space traffic addressed to other bots in a shared room kept landing in my task queue tagged access_tier: owner (10+ occurrences in one session, root-caused earlier to the blanket-REMOTE_TASK_TIER=owner-is-the-only-lever problem this PR describes). This is the fix for that class of bug, not a hypothetical.

Scope is appropriately narrow — single concern, explicitly excludes TOFU-for-the-gateway and an allowFrom-seeding migration with correct reasoning for both (no signal to distinguish DM from shared-room today; nothing to seed from since the gateway has no allowFrom).

Approving.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold review — comment, not a formal approval (shared gh identity resolves to the owner). Signed: qingyun-001.

Verified the load-bearing invariant. Removing the <= LOCAL_TIER clamp is safe because the escalation vector it used to bound is closed elsewhere: _tier_for is only ever called as _tier_for(task.get("user_id")) (lines ~1276, ~1439), and user_id is a broker-writer-side field in _TASK_FIELDS — serialized by the gateway from the attested Matrix sender, right alongside room_name/sender_name, never a task-body self-claim. A room member can't forge another user's mxid as the event sender, so the WIRE still cannot escalate; only the owner's LOCAL access.json can, and only for an explicitly-named sender. That matches the discord/slack precedent (unclamped tierMap[sender_id]).

Fail-closed properties all hold + are tested: invalid mapped value → LOCAL_TIER (12c), unlisted → LOCAL_TIER (12b), owner-default node unlisted still owner / no regression (12d), and the fail-safe last-known-good on a malformed re-read is preserved (13). CI: 15 green + 1 skipped, MERGEABLE.

One non-blocking note (defense-in-depth): the old clamp was a backstop — even a future refactor that accidentally let user_id be body-influenced was bounded to LOCAL_TIER. With the clamp gone, "user_id is broker-attested" is now solely load-bearing. Consider a one-line assertion or a comment at the task.get("user_id") call sites pinning that contract, so a later change can't silently reintroduce a wire-controlled user_id. Not a merge gate — the current code is correct.

Self-review of the up-tier in this PR. The cache is deliberately preserved
across a read error so a transient mid-write cannot fail-OPEN a down-tiered
sender back to LOCAL_TIER. That reasoning only holds for entries at or below
LOCAL_TIER.

Once the map can also grant a tier ABOVE LOCAL_TIER, replaying the cache
verbatim keeps an escalation alive on a file the owner may have just deleted
to revoke it — so deleting access.json would not actually revoke anything.
Demonstrated against the parent commit: pre-change the grant clamped to team
so nothing could persist; post-change the sender stayed owner after deletion.

_stale_safe() projects a STALE map onto the fail-closed side in BOTH
directions: above-LOCAL_TIER entries are dropped (those senders fall back to
LOCAL_TIER), the rest are kept. Applied ONLY to the two stale paths (deleted /
unparseable). The mtime-unchanged path is a VALID cache, not a stale one, and
returns verbatim — projecting there would drop a legitimate up-tier on every
call. Test 21 guards that.

A legitimately-granted sender is briefly demoted while the file is unreadable
and restored on the next successful read. Transient demotion is the safe
direction; the module already fails closed to team on a bad LOCAL_TIER.

Tests 18-22 (22/22, was 17/17). Verified the two new revocation assertions
FAIL with _stale_safe disabled and ONLY those two — the down-tier fail-safe
and hot-path guards still pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Self-review finding — pushed a fix in 6cc2c27. Flagging it because the first version of this PR was wrong in a way that is easy to miss.

The _load_tier_map cache is deliberately preserved across a read error, so a transient mid-write cannot fail-OPEN a down-tiered sender back to LOCAL_TIER. That reasoning holds only for entries at or below LOCAL_TIER.

Once the map can also grant a tier above LOCAL_TIER — which is exactly what this PR added — replaying the cache verbatim keeps an escalation alive on a file the owner may have just deleted in order to revoke it. Deleting access.json would not actually revoke anything.

Demonstrated against the parent commit rather than argued:

pre-change  (011b496ca7e7, clamp present)
  1. granted      -> team      # clamped, so nothing could persist
  2. after DELETE -> team

post-change (04d7021b564d, clamp removed)
  1. granted      -> owner
  2. after DELETE -> owner     # <-- revocation ineffective

So the hazard is introduced by the up-tier, not pre-existing — the stale-cache behavior was safe precisely because the clamp made an above-LOCAL_TIER entry unreachable.

Fix: _stale_safe() projects a stale map onto the fail-closed side in both directions — above-LOCAL_TIER entries are dropped (those senders fall back to LOCAL_TIER), everything else is kept, so the original fail-safe is untouched.

Applied to only the two genuinely-stale paths (deleted / unparseable). The mtime-unchanged path is a valid cache, not a stale one, and still returns verbatim — projecting there would drop a legitimate up-tier on every call. I made that mistake while writing the fix and caught it; test 21 now guards it.

Tradeoff, stated plainly: a legitimately-granted sender is briefly demoted while the file is unreadable, and restored on the next successful read. Transient demotion is the safe direction, and it matches the module's existing posture of failing closed to team on a bad LOCAL_TIER.

Tests 18–22 added (22/22, was 17/17). Verified the two new revocation assertions fail with _stale_safe disabled — and only those two; the down-tier fail-safe and the hot-path guard still pass:

ARM-A (fix DISABLED, sha cd3f6e494cd9, stale_safe_calls=0)
  FAILED ['deleting access.json REVOKES an escalated sender (no stale owner)',
          'malformed read drops the escalation']
ARM-B (fix RESTORED, sha 1b7908d45b19, stale_safe_calls=2)
  22/22 passed

python3 src/remote-gateway-bridge.test.py → PASS.

Addresses the non-blocking cold-review note on #2584. The removed clamp was a
backstop: even if user_id had become body-influenced, a mapped tier stayed
bounded by LOCAL_TIER. With it gone, "user_id is the broker-written Matrix
sender, never a task-body self-claim" is SOLELY load-bearing for the
no-wire-escalation property.

Pinned as a CONTRACT note on _tier_for plus a pointer at both call sites, so a
later change that lets a task body influence user_id has to walk past it.

Deliberately a contract note rather than an assert: provenance cannot be
checked at runtime — the value is an ordinary string whichever path produced
it, so an assertion would give false assurance rather than enforcement.

No behavior change. 22/22 tests, sparrow suite PASS.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Thanks @QingYun-001 — taken, in b4fab61.

Agreed on the substance: the clamp was doing double duty as a backstop, and removing it makes "user_id is broker-attested" solely load-bearing for the no-wire-escalation property. That's worth pinning.

One deliberate deviation from the suggestion. You offered "a one-line assertion or a comment" — I went with the comment only, and not for brevity. Provenance can't be checked at runtime: user_id is an ordinary string whether the broker wrote it or a future refactor let a task body influence it. An assert would inspect the value, never its origin, so it would give false assurance while enforcing nothing — worse than no check, because the next reader would trust it. The contract is pinned as a ⚠ CONTRACT block on _tier_for plus a pointer at both call sites (~1296, ~1459), so a change that weakens it has to walk past the statement three times.

Also flagging, since it happened between your review and now: @bassilkhilo-ag2 approved at 22:40:57Z against head f97b9fa, and I pushed 6cc2c27 at 22:47:23Z — which staled that approval. That push was a self-review fix for a real hazard the up-tier introduced: with the clamp gone, a stale tier-map cache kept an escalation alive across a read error, so deleting access.json to revoke someone did not revoke them. Details and the two-arm evidence are in the comment above. I've re-requested review from bassilkhilo rather than leaving a stale approval sitting there looking current.

Current head is b4fab61: 22/22 tests, sparrow suite green, no behavior change from this last commit.

_tier_for has a second consumer, _write_owner_activity. Its docstring reasons
only about DOWN-tiering ('a down-tiered teammate must not overwrite...'),
because until this PR an above-LOCAL_TIER entry was unreachable. The up-tier
creates a case it never contemplated.

Verified the behavior is already correct: on a LOCAL_TIER=team node a sender
mapped to owner DOES register owner presence, and an unlisted sender does not.
That is the intended semantics — naming someone owner is the point — but it
was untested, and presence drives the proactive loop's 'owner active' signal
and the core-supervisor escalation target, so a silent regression here is
expensive and hard to trace.

Tests 23/24 pin both directions; the docstring now states the mirror case so a
refactor that regates on LOCAL_TIER has to walk past it. No behavior change.

24/24 (was 22/22), sparrow suite PASS.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Follow-through on the same thread, a1e9119 — no behavior change, tests only (plus a docstring line).

_tier_for has a second consumer: _write_owner_activity, the owner-presence gate. Its docstring reasons entirely about down-tiering — "a down-tiered teammate must not overwrite state/last-owner-activity.json" — because until this PR an above-LOCAL_TIER entry was unreachable. The up-tier creates a case it never contemplated, and I hadn't checked it.

Verified the behavior is already correct:

LOCAL_TIER=team, tierMap{@dana:owner}
  up-tiered owner   tier=owner  owner_activity_written=True
  unlisted sender   tier=team   owner_activity_written=False

That is the intended semantics — naming someone owner is the whole point — so nothing needed fixing. But it was untested, and presence feeds the proactive loop's "owner active N min ago" signal and the core-supervisor escalation target, so a later refactor that regated this on LOCAL_TIER instead of the sender's resolved tier would silently stop recording the real owner's activity. That is a costly failure to trace, so tests 23/24 now pin both directions and the docstring states the mirror case.

24/24 (was 22/22), sparrow suite green.

One honest note on the discriminator: reverting the up-tier makes the suite abort at test 18's cache-priming assert rather than reporting 23/24 as individual failures. That proves the tests depend on the change, but less legibly than a per-test failure would. I left it fail-fast deliberately — continuing past a broken precondition would produce cascading failures from a polluted cache rather than one clear cause — but flagging it so it's a choice on the record rather than an accident.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Blocking finding for exact head a1e9119657409dcf809164bd44590344f2f42a3c.

[P1] Same-mtime rewrites can keep a revoked explicit owner alive

The new _stale_safe() path fixes absent and malformed access.json, but _load_tier_map() still treats mt == _TIER_MAP_CACHE["mtime"] as authoritative and returns the cached map verbatim. Now that the cache can contain an above-LOCAL_TIER grant, a revocation rewrite that lands with the same mtime as the cached file keeps the old owner grant alive. I reproduced this against the current head by priming LOCAL_TIER=team with {"@dana:ag2.space":"owner"}, rewriting the file to {"tierMap":{}}, restoring the same mtime with os.utime, then calling _tier_for("@dana:ag2.space"): it returned owner, not team.

This edge was much less dangerous when the map could only down-tier, because a stale cached value preserved lower privilege. With explicit up-tiering, the same cache miss means deleting or rewriting the local owner-owned file may not revoke an owner grant. Please strengthen the cache key beyond float mtime, such as st_mtime_ns plus size/inode, or re-read whenever a cached map contains above-local entries, and add a regression test for a same-mtime empty rewrite.

The focused suite otherwise passed locally: tests/gateway-per-sender-tier.test.py, tests/gateway-owner-presence-peer-gate.test.py, tests/bridge-tiermap-migration-perms.test.py, tests/ag2-sparrow-drift.test.py, tests/remote-gateway-bridge.test.py, tests/remote-gateway-interaction-type.test.py, tests/gateway-writeside-platform-card.test.py, tests/gateway-writeside-attachments.test.py, py_compile, ruff, src-map, python39 compatibility, git diff --check, and scripts/review-checks.sh. I also re-checked the room-ops boundary: Sutando-local still only uses the local access.json and gateway bearer in this path, with membership/sender identity coming from broker-side user_id, not task-body self-claims.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking finding on exact head a1e9119657409dcf809164bd44590344f2f42a3c.

[P1] A same-mtime rewrite can preserve a revoked explicit owner grant. _load_tier_map() returns the cached map verbatim when os.path.getmtime(path) equals the cached float mtime. I reproduced this independently by priming LOCAL_TIER=team with {"tierMap":{"@dana:ag2.space":"owner"}}, rewriting the file to {"tierMap":{}}, restoring the original st_mtime_ns, and calling _tier_for("@dana:ag2.space"): it returned owner instead of team. The exact output was mtime_equal=True and after_revocation=owner.

This becomes privilege-revocation correctness now that the cache may contain above-local grants. Please strengthen the cache identity beyond float mtime (or force a reread for cached above-local entries) and add a same-mtime revocation regression. The focused 24/24 tier suite and 6/6 owner-presence suite otherwise pass; diff hygiene and the REVIEW.md hardcoded-path scan are clean. The branch is also behind current main after #2580 landed.

Verdict: changes requested; not merge-ready until the revocation path is fixed, the branch is refreshed, hosted gates pass, and two fresh current-head approvals are recorded.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Independently reproduced john-the-dev's P1 finding — confirmed, not taking it on faith.

Different repro path (own script, os.utime(..., ns=(orig_mtime_ns, orig_mtime_ns)) to force exact mtime equality rather than relying on wall-clock timing):

tier_before revocation: owner
mtime_equal: True 1785798738.6955173 1785798738.6955173
after_revocation: owner
BUG CONFIRMED: revoked grant still resolves to owner

Same root cause as described: _load_tier_map()'s cache invalidation is mt == _TIER_MAP_CACHE["mtime"] — a same-mtime rewrite (same second on coarse-mtime filesystems, or two writes landing in the same tick) returns the stale cached map verbatim, so a revoked above-LOCAL_TIER grant survives until either the mtime actually changes or the process restarts.

Flagging transparently: my own review (APPROVED, 22:40Z) verified the escalation-path claims — broker-attested user_id, local-only access.json, discord/slack parity, the discriminator test — but didn't independently exercise the revocation path against this specific cache-identity edge case. John's finding is real and the severity assessment is right: this is materially worse post-fix than pre-fix, since before the clamp meant even a stale-cache "owner" entry could never exceed LOCAL_TIER; now an above-LOCAL_TIER grant can persist past its own revocation.

Agree this should stay CHANGES_REQUESTED until the cache identity is strengthened (e.g. hash the file content, or key on (mtime_ns, size), or just force a reread whenever the cached map contains an above-LOCAL_TIER entry) and a same-mtime revocation regression is added alongside the existing 12d no-regression test.

qingyun-wu and others added 2 commits August 3, 2026 16:20
Addresses john-the-dev's [P1] on a1e9119. Reproduced independently before
fixing: prime LOCAL_TIER=team with {'@dana':'owner'}, rewrite the file, restore
st_mtime_ns via os.utime -> _tier_for still returned owner while the file on
disk said otherwise.

Root cause: the cache keyed on float os.path.getmtime(). That collides under a
same-second rewrite and can be restored outright. Harmless while the map could
only DOWN-tier; once it can grant above LOCAL_TIER it means a revoked
escalation survives.

Two changes, and BOTH are load-bearing (demonstrated separately):
- cache identity is now (st_mtime_ns, st_size, st_ino), not a float mtime;
- never serve an ABOVE-LOCAL grant from cache without a fresh read, because a
  caller can restore all three components deliberately.

Cost is one small read per call only while an escalation is cached. discord's
loader has no cache at all and re-reads on every message, so this is not a new
class of expense.

Tests 25-27. NOTE on the first draft of these tests: they rewrote the file
WITHOUT the allowFrom envelope _write_map uses, which changed st_size — so the
size component caught the staleness and they passed even with the above-local
guard removed. They were not exercising the path they claimed. Corrected to
preserve the envelope so identity collides exactly (mtime_ns, size AND inode
all equal, verified by probe); test 25 now fails with the guard removed and
test 26 still passes on identity alone.

27/27 tier, 6/6 owner-presence, sparrow suite PASS. Branch refreshed onto main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@john-the-dev — [P1] fixed in e74157d. Reproduced your case independently first, and it landed exactly as you described:

1. primed grant           -> owner
   mtime_equal = True   ns_restored = True
2. after revocation       -> owner     <-- expected team
   file content now       : {"tierMap": {}}

Root cause is the one you named: the cache keyed on float os.path.getmtime(). That collides under a same-second rewrite and can be restored outright with os.utime. It was harmless while the map could only DOWN-tier — this PR is what makes it a revocation-correctness problem, so the defect is mine, not pre-existing.

Both remedies you offered are implemented, because each catches a case the other misses:

  • cache identity is now (st_mtime_ns, st_size, st_ino) rather than a float mtime;
  • an above-LOCAL_TIER grant is never served from cache without a fresh read — identity can still be reproduced deliberately, and a revoked escalation must not survive that.

Cost is one small read per call only while an escalation is cached. For scale: discord-bridge.load_tier_map has no cache at all and re-reads on every message, so this isn't a new class of expense.

A correction on my own test, which I'd rather state than let you find. My first draft of these tests rewrote access.json without the allowFrom envelope that _write_map uses. That changed st_size, so the size component caught the staleness and the tests passed even with the above-local guard removed — they were not exercising the path they claimed. Corrected to preserve the envelope so identity collides exactly; verified by probe:

cached ident : (1785799162478623986, 41, 49594690)
current ident: (1785799162478623986, 41, 49594690)
IDENTITY MATCHES: True

With that fixed, the two guards are separately demonstrated:

ARM-A (above-local guard REMOVED, strengthened ident kept)
  FAILED ['identity-colliding rewrite does NOT serve a revoked owner grant']
  ...test 26 (revoke to empty map) still PASSES — caught by the size component
ARM-B (full fix)  27/27 passed

So test 25 pins the guard and test 26 pins the identity. Test 27 confirms a non-escalating map still takes the fast path.

Also done: branch refreshed onto current main (behind = 0 after #2580). Suites: 27/27 tier, 6/6 owner-presence, src/remote-gateway-bridge.test.py PASS.

On the approval count — understood, and worth stating plainly: @bassilkhilo-ag2's 22:40 APPROVE predates this and every later head, so it is stale and I am not counting it. This needs two fresh current-head approvals.

@qingyun-wu
qingyun-wu requested a review from john-the-dev August 3, 2026 23:21

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed repaired exact head e74157d6b167aaac0bb999f0ff22cea29400a00f and formally replacing my prior same-mtime revocation blocker.

The failure mode is now discriminating on the exact identity collision, not just a changed-size rewrite. Reviewer-run reproduction with the same inode, byte size, and restored st_mtime_ns gives:

old a1e91196: before=owner identity_equal=True after=owner
new e74157d6: before=owner identity_equal=True after=other

The repair has two independent layers: (st_mtime_ns, st_size, st_ino) invalidates ordinary rewrites, while cached above-local grants always trigger a fresh read, so deliberately reproduced metadata cannot keep an escalation alive. Missing/malformed reads still project stale state fail-closed: down-tiers remain, above-local grants drop to LOCAL_TIER.

Reviewer-run exact-head evidence: per-sender tier 27/27, owner-presence 6/6, required remote-gateway suite PASS, interaction-type serialization PASS, platform-card and attachment write-side suites PASS, Python compilation, git diff --check, generated source map, and REVIEW.md hardcoded-path scan pass. Local Ruff could not run because this host has an older Ruff that rejects the repository’s [lint] config; the hosted Ruff gate is the authority for that check and is still running.

Worst-case disruption is either a revoked owner retaining privilege or an availability cost from rereading the small local file while any above-local grant is active. The fresh-read rule directly closes the privilege risk; the bounded local read is an acceptable fail-closed cost and matches the uncached Discord loader precedent.

Non-blocking documentation nit: the module-level comment immediately above _ag2space_access_path() still says “the map only DOWN-tiers named senders,” which is stale after this PR; the detailed _tier_for contract below is correct. Please clean that sentence up before or in a follow-up so the security model is not self-contradictory.

Verdict: APPROVED for code on this exact head. Not merge-ready yet: hosted checks must finish green and a second genuinely current-head maintainer approval is still required.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Fix verified — re-ran my own repro against the new head (e74157d), not just trusting that a green re-review means it's resolved.

My original repro (mtime_ns forced equal, size/inode incidentally differed) — now correctly revokes:

tier_before revocation: owner
mtime_ns_equal: True size_before/after differ: 41 15 inode_same: True
after_revocation: team
FIXED - revocation took effect -> team

Stress test I added on top — forced a FULL identity-tuple collision (padded the revoked JSON with whitespace to match the exact original byte count, so (mtime_ns, size, inode) are all identical to the pre-revocation stat):

full identity tuple collision: True
after_revocation (full identity collision): team
FIXED even under full identity collision - belt-and-braces check works -> team

This is the case that would have slipped past a naive (mtime_ns, size, inode)-only fix. It holds because _has_above_local() forces a fresh read whenever the cached map contains anything above LOCAL_TIER, independent of whatever identity tuple is being compared — so identity-tuple collision resistance isn't actually load-bearing for the security property, which is a more robust design than it might look at a glance.

Also ran the full suite myself: tests/gateway-per-sender-tier.test.py → 27/27 (includes the two new tests targeting this exact class: "identity-colliding rewrite does NOT serve a revoked owner grant" and "same-mtime revocation to an empty map drops the grant"), src/remote-gateway-bridge.test.py → all green, hardcoded-path scan clean.

My prior APPROVED review (22:40Z) predates this fix commit (23:20Z) so was technically stale against the new head — this comment is the freshness update: I've now independently verified the fix at current head, including with a stronger adversarial case than the original finding. No outstanding concerns from me.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full current diff and the late revocation-cache fix at e74157d6.

Code-ready on this exact head; no new blocking finding. The prior same-mtime revocation bug is fixed in two layers: cache identity now includes nanosecond mtime/size/inode, and an above-LOCAL_TIER cached grant is always re-read rather than trusted solely from file identity. The failure-mode test now performs an exact identity collision (owner → other, same size/mtime/inode) and resolves the revoked sender as other; deletion/malformed-file paths also drop stale escalations while retaining down-tier fail-safety.

Evidence run on this head:

  • gateway-per-sender-tier.test.py: 27/27
  • gateway-owner-presence-peer-gate.test.py: 6/6
  • full src/remote-gateway-bridge.test.py: PASS
  • interaction type, write-side platform-card, attachment, drift, and tier-map migration-permission tests: PASS
  • Python compile, git diff --check, and REVIEW.md hardcoded-path scan: PASS

Worst-case disruption is a transient demotion of an explicitly up-tiered sender while the owner map is missing or malformed; that is the safe direction and heals on the next successful read. Existing owner-default/unlisted behavior is pinned unchanged.

Not merge-ready yet: hosted CI and Coverage Gate are still running, and the repository requires a second current-head maintainer approval. The authenticated reviewer identity is also the PR author, so this is a COMMENT verdict rather than a self-approval.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at current head (e74157d) - my prior approval (22:40Z) predated this fix commit (23:20Z) and was stale. Verified the fix itself, not just the green CI: independently reproduced the original repro against this head (now correctly revokes) and stress-tested a stronger adversarial case (full (mtime_ns, size, inode) identity-tuple collision, forced via padding) which also correctly revokes because of the belt-and-braces _has_above_local() check. Full detail in my prior comment. 27/27 on the tier suite, required suite green, hardcoded-path scan clean. No outstanding concerns.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Approval signal for exact head e74157d6b167aaac0bb999f0ff22cea29400a00f (GitHub rejected formal approval because the authenticated account is the PR author).

No blocking findings. I re-checked the revocation-cache fix and the current implementation no longer serves an above-local grant from cache after a same-identity/same-mtime rewrite; the owner-to-other and owner-to-empty-map revocation cases both resolve back to the local team tier. The trust boundary still looks right for the room-ops/gateway shape: Sutando-local only holds the gateway bearer plus local channels/ag2space/access.json, AppService namespace credentials remain broker-side, task access_tier is derived from the local tierMap keyed by broker-attested user_id, and _write_owner_activity uses the same resolved tier.

Focused checks run in /private/tmp/sutando-pr2584-review.caicwa/repo:

  • python3 tests/gateway-per-sender-tier.test.py
  • python3 tests/core-heartbeat-pgrep-portability.test.py
  • python3 tests/gateway-owner-presence-peer-gate.test.py
  • python3 tests/bridge-tiermap-migration-perms.test.py
  • python3 tests/ag2-sparrow-drift.test.py
  • python3 tests/remote-gateway-interaction-type.test.py
  • python3 tests/gateway-writeside-platform-card.test.py
  • python3 tests/gateway-writeside-attachments.test.py
  • python3 tests/remote-gateway-bridge.test.py
  • python3 tests/python39-union-annotations.test.py
  • py_compile with an isolated cache
  • python3 -m ruff check ...
  • bash scripts/agents-md-sync.sh --check
  • python3 scripts/gen-src-map.py --check
  • git diff --check origin/main...HEAD
  • bash scripts/review-checks.sh --diff /private/tmp/sutando-pr2584-review.caicwa/pr2584.diff

Hosted checks are green on the same head.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Merge-order hazard with #2340 — flagging before either lands, because a naive conflict resolution silently reintroduces a [P1].

#2340 (fix(gateway): resolve access map from active config) and this PR both edit _load_tier_map's cache, and both delete the same two baseline lines:

-_TIER_MAP_CACHE = {"mtime": None, "map": {}}
-    if mt == _TIER_MAP_CACHE["mtime"]:

and replace them incompatibly:

replacement
#2340 {"path": None, "mtime": None, "map": {}} + if path == …["path"] and mt == …["mtime"]:
#2584 (this) {"ident": None, "map": {}} + if ident == …["ident"] and not _has_above_local(…)

So whichever merges second will conflict here. That part is mechanical. The reason I'm writing it down is that either "take one side" resolution loses a real protection, silently:

The correct resolution is a union, not a choice — fold the path into the identity tuple:

_TIER_MAP_CACHE = {"ident": None, "map": {}}
...
ident = (path, st.st_mtime_ns, st.st_size, st.st_ino)
if ident == _TIER_MAP_CACHE["ident"] and not _has_above_local(_TIER_MAP_CACHE["map"]):
    return _TIER_MAP_CACHE["map"]

That keeps #2340's path-change invalidation (a changed path changes the tuple), this PR's forgery resistance, and the above-local fresh-read. Both PRs' tests should pass unmodified against it.

No action needed on either PR right now — I'm not touching #2340 (not mine) and not pushing here (it would stale the two current-head approvals for a comment). Recording it so whoever resolves the conflict has the recipe rather than rediscovering it under time pressure.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Correction to my previous comment — I said "both PRs' tests should pass unmodified against [the union]". I tested it, and that is wrong. Posting before anyone acts on the recipe.

Running #2340's tests/gateway-access-path-resolution.test.py against this branch's head:

FAIL AG2_DEVICE_ENV resolves sibling access.json
FAIL resolved access path is logged
ok   CLAUDE_CONFIG_DIR is the non-desktop fallback
FAIL no launcher config never guesses stale ~/.claude
Traceback (most recent call last):
  ...
    if ident == _TIER_MAP_CACHE["ident"] and not _has_above_local(...)
KeyError: 'ident'

Three things I got wrong, in increasing order of importance:

1. The test suites are schema-coupled to the cache dict. #2340's test assigns _TIER_MAP_CACHE directly using its own keys (path/mtime/map); this branch reads ["ident"]. So neither suite can run against the other's source unmodified — whoever merges second must update the other PR's test too, not just reconcile the source. My "should pass unmodified" was an assumption stated as a prediction.

2. The conflict is wider than the cache. #2340 also rewrites _ag2space_access_path() into a multi-source launcher-config resolver (hence the three non-crash FAILs — this branch still has the single-path version). That part does not textually conflict with my diff, but it means the union is not confined to the three lines I quoted.

3. My union recipe silently dropped #2340's error-path semantics — this is the one that matters. Their _load_tier_map does more than key on path:

  • if not path: → clear the cache and return {} (removing both launcher pointers deliberately disables the map);
  • on OSError, keep last-known-good only for the same configured path, and clear on a path switch, explicitly so trust decisions do not leak across installs.

Folding path into my identity tuple gets the shortcut right and leaves both of those behaviours out. A resolution that takes my _stale_safe projection wholesale would carry a down-tier across a config-path switch, which is exactly what #2340 is preventing.

Revised recipe. The union needs the identity tuple and a path-change branch that clears rather than projects:

ident = (path, st.st_mtime_ns, st.st_size, st.st_ino)
# on read error:
if path != cached_path:   # config switched installs -> clear, do not carry tiers
    clear cache; return {}
return _stale_safe(cached_map)   # same path -> fail-closed projection (this PR)

plus #2340's if not path: disable case, and an update to whichever test file is not the merge base.

Not touching #2340 (not mine) and not pushing here (would stale the two current-head approvals). Flagging only because I published a recipe that under-specified a security-relevant merge, and "should pass" was a hedge I should have resolved by running it.

@qingyun-wu
qingyun-wu merged commit dab9c99 into main Aug 4, 2026
18 checks passed
@qingyun-wu
qingyun-wu deleted the fix/gateway-per-sender-owner-tier branch August 4, 2026 00:32
john-the-dev added a commit that referenced this pull request Aug 4, 2026
Comments only — no code changed (stripping trailing comments leaves the diff
byte-identical).

#2584 let the tierMap grant a tier ABOVE LOCAL_TIER and re-keyed the cache on
(st_mtime_ns, st_size, st_ino) with a forced fresh read for above-local grants.
Three comments still describe the pre-#2584 world:

1. 'the map only DOWN-tiers named senders; owner stays owner by being absent
   from it' - john-the-dev's non-blocking nit on #2584. Now false. The adjacent
   claim on the line above ('an unknown sender can never ESCALATE') is still
   TRUE and is kept, scoped explicitly to UNLISTED senders.
2. 'Hot: re-read on mtime change' - also false, and NOT flagged in review. I
   found it while locating (1); my own fix is what invalidated it. It now
   describes the identity tuple and the above-local fresh read, and says the
   owner can revoke as well as add without a restart.
3. tests/gateway-per-sender-tier.test.py:43 'mtime granularity is coarse' - the
   rationale for a float mtime, not for an ns+size+inode key.

Deliberately deferred until after #2584 merged: before that, all three were
CORRECT on main, so fixing them early would have documented unmerged behaviour.

27/27 tier, 6/6 owner-presence, sparrow suite PASS.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
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.

4 participants