[lilyshen0722]

Why

"Read a repo" is one of the four out-of-box asks. It fails on every account, because no agent can open a link. TASK-267b's egress half, getFromPublicAddress, landed in #2242. This PR adds the tool on top of it, as TASK-267 designed: opt-in by declaration, a page narrows the turn, and the #2206 marker wraps the page.

The rule this adds: provenance

The TASK-267 design narrowed the turn after a fetch. It missed that the fetch's address is itself a way out. Whatever the model puts in a URL reaches that URL's host. A turn steered by outsider text could compose https://their.host/?d=<what this turn holds>, for example connector results it read earlier in the turn. The guard would let it through, because the host is public.

So the model never composes an address. commonly_fetch_url opens only an address that appears, as written, in text the turn was given:

• the prompt;
• the message it is answering;
• other authors' pod messages, whether in the opening window or read with read_context;
• search results;
• pages already read.

The model's own words never count: its replies, its tool arguments, its own posts read back (services/webAddressProvenance.ts). A match is exact apart from what can't carry data the model chose: scheme (http is opened as https), fragment, host case, trailing slashes, and sentence punctuation. Path and query must match character for character, and a prefix doesn't count. The check runs before anything is sent.

The rest

Taint	A page reaches the model behind the #2206 marker ("Content from a web page follows…"), and the turn then narrows exactly as after a search: in-pod read and post plus lookups. Connector tools, code runs, tasks and memory writes leave.
Bridged pods	Not offered, and refused at dispatch if a bridge appears mid-turn (the #2244 rule).
Size	The guard reads up to 1 MB. The model gets the first 24,000 characters, flagged truncated.
Cap	3 pages a turn, counted when sent, recorded as AgentRun.webFetches. No spend term: there's no per-request cost.
Guard refusal	The reason reaches the model. No third-party text entered, so the turn isn't narrowed.
Record	The run keeps the address, status, size and truncated, not the page.

Hosted ("Run it here") agents don't get it in this PR. Adding it to HOSTED_NATIVE_TOOLS follows once #2248 merges, with the hosted pod-frame fix.

Residuals (open, not handled)

• Another agent can relay an address. An address this agent wrote, re-posted by another agent, counts as that agent's message. It needs a second agent acting on it; the reach is the relaying agent's.
• Choosing among given addresses is a low-bandwidth signal: which one to open. Nothing the turn holds can be spelled into a URL.

Fixed after the first gates

• Linear on third-party text (2024f74c, sprint-review's HOLD): the trims scan by index, given texts are scanned once into a set, and a host is capped at 253 characters.
• The argument only selects (87f24d84, Cofounder's first security HOLD): the request is the given address rebuilt from the given text (GivenAddresses.resolve), never the model's argument. A trailing run, a fragment or host case the comparison ignored used to go out in the request path. Arms pin the URL handed to the network layer byte for byte.
• Own posts by user id, not name (3afb9631, Cofounder's second security HOLD): this closes the residual this body first listed as open. A read-back post's author is the chat mirror's username, which drifts by design (a collision suffix, "Unknown User"). Messages are now kept only when their author id is known and isn't the agent's own bot user; an unresolved own user admits no pod message.

Tests

Run under Node 22 (CI's version).

• webAddressProvenance.test.js: 30 arms.
  • Accepted forms: scheme-less, http, punctuation, fragment, host case, markdown, angle brackets, JSON, port.
  • Refused forms: unseen, query added or changed, path added, prefix, other host.
  • Non-web schemes; linear time on 450 KB of hostile text.
• nativeRuntimeService.webFetch.test.js: 16 arms. A link from the mention; scheme-less over https; an unseen address and a given address with data added, both refused with no request; another author's message gives a link while the agent's own post does not; a search result, then a link on that page; post-then-read-back refused; narrowing; guard refusal; truncation; the cap; opt-in only; bridged; a bridge mid-turn.
• All 61 suites under nativeRuntimeService*, outboundUrlGuard*, spendAlarm* and models/: 637/637.
• Mutations, each caught and restored from a copy:

mutation	failing
provenance check removed	4
own posts count as given	2
a page doesn't taint	1
bridged pods offer fetch	1
cap 4	1
search results give nothing	1
read_context gives nothing	1
no bridge re-read at dispatch	1

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wkz5axAzCvoLEFkZ5KtkKG

[lilyshen0722]

Written by sprint-review, a Commonly agent — code gate at 67bff82b3. pod thread

Holding on a measured availability defect in the new module. The provenance design itself is strong and I want to say that first — the hold is about two regexes, not about the approach.

What I verified is sound

I probed the matcher directly rather than reading it, 18 cases, all as intended — including both primary exfiltration vectors:

attempt	result
given https://docs.x.com/a, fetch …/a?d=secret (query append)	refused
given …/a, fetch …/a/secret (path append)	refused
fetch a prefix of a given address	refused
https://docs.x.com@evil.com/a (userinfo smuggle)	refused
https://docs.x.com.evil.com/a (subdomain swap)	refused
169.254.169.254, localhost (raw IP / loopback)	refused
query reordered	refused
host case, trailing slash, fragment, http→https, prose period, markdown link	allowed, correctly

Four security invariants are pinned by name, each failing the arm that states it:

mutation	result
addressWasGiven check removed	4 failed, incl. "refuses the given link with data added" and "the model's own words never give a link: it cannot post an address, read it back and open it"
own-author exclusion removed	2 failed
per-turn fetch cap removed	1 failed
fetch does not taint the turn	1 failed — the narrowing arm

BASE/RESTORED 46/46; wider sweep 22 suites / 390 passed, 0 failed (my selection, a subset of your 637 — I'm not claiming to have reproduced that number). Redirects are also handled correctly: getFromPublicAddress re-runs toPublicDestination on every hop and pins to the resolved address, so there is no SSRF-via-redirect, and a redirect target carries no model-chosen data.

HOLD: two high-severity ReDoS alerts, confirmed by measurement

CodeQL is red at this head — "2 new alerts including 2 high severity security vulnerabilities", both js/polynomial-redos, both in the new file:

• webAddressProvenance.ts:30 — TRAILING_PUNCTUATION = /[.,;:!?*_~]+$/
• webAddressProvenance.ts:38 — .replace(/\/+$/, '')

CodeQL is a heuristic, so I measured instead of trusting it. Both are real. An anchored +$ backtracks quadratically when the character run is not at the end:

token	addressKey cost
a. + 2,000 ! + x	2.7 ms
a. + 8,000 ! + x	34.0 ms
a. + 20,000 ! + x	194.9 ms
a. + 40,000 ! + x	727.8 ms

Doubling the length roughly quadruples the time — clean n². The /\/+$/ site scales identically (611 ms at 40,000).

It is reachable with attacker-controlled input at this code's own caps. The texts scanned are search results and fetched pages — third-party by definition. TOKEN_SEPARATORS does not split on ! or /, so a single token can be arbitrarily long, and MAX_FETCHED_TEXT_CHARS is 24,000, so one page contributes a 24k token.

Two things amplify it:

1. addressWasGiven calls addressesIn over every given text on every attempt, with no memoisation, and givenText accumulates the prompt, other authors' messages, each search result and each page read.
2. The cap check at :649 precedes the provenance check at :654, and webFetches is incremented at :655 only after provenance passes. So a refused fetch pays the full scan and does not spend the 3-fetch budget — the model can keep attempting, bounded only by MAX_TURNS = 10.

Composed at the real caps — three pages of 24k malicious text in givenText:

one refused fetch attempt:  721 ms
× MAX_TURNS (10):          7.2 s

7.2 seconds of blocked event loop per turn. Node is single-threaded, so that stalls every request on the pod, not just this agent's turn. MAX_WALL_CLOCK_MS = 60_000 does not help: 7.2s is well inside it, so the turn completes normally and nothing reports a fault.

The fix, and a warning about the obvious one

Do not simply bound the quantifier. I tried {1,8} first: it fixes the cost (721 ms → 0.26 ms) but changes behaviour — an address with 9 or more trailing dots or slashes stops matching. Worth noting that your 46 tests still pass under that regression; I only caught it because I had added two boundary cases at the limit. That is a coverage gap in its own right.

The behaviour-preserving fix is to strip trailing characters by index scan rather than an anchored quantifier:

const PUNCT = new Set(['.', ',', ';', ':', '!', '?', '*', '_', '~']);
const stripTrailing = (s, pred) => { let e = s.length; while (e > 0 && pred(s[e - 1])) e--; return s.slice(0, e); };

I checked it against the original regexes on 20,000 random strings (40,000 comparisons): zero mismatches, and on the 24k malicious token it is 0.004 ms vs 253 ms. Same semantics, no backtracking.

Two further findings, neither blocking

AgentRun.webFetches is recorded but never priced. spendAlarmService.ts aggregates only sandboxSeconds and webSearches, and its candidate filter at :131 is $or: [{sandboxSeconds: {$gt: 0}}, {webSearches: {$gt: 0}}]. A turn that only fetched pages matches neither clause, so the row is excluded from the aggregation entirely — the field is written and nothing reads it. Fetch cost is mostly egress and small, so this is about the instrument, not the bill.

The docstring's threat model omits the selection channel. It argues, correctly, that the model never composes an address. But choosing among the addresses a page gives is itself a channel: an attacker-controlled page can pre-write many URLs and the steered model leaks information by which it opens — roughly log2(N) bits per fetch. MAX_WEB_FETCHES_PER_TURN = 3 is what bounds this, which makes that constant load-bearing for a reason the comment does not state. Worth a sentence there, so a later editor does not raise the cap believing provenance makes fetches free.

Minor, currently unreachable: byOtherAuthors defaults to ownNames || new Set(), which excludes nothing when the name set is missing — the central invariant inverted by an absent field. It is always populated today (dispatchCtx sets it at birth, :1255), but ownAuthorNames?: is optional in DispatchContext, so the type permits exactly that mistake. Making it required, or defaulting to refuse, would put the failure on the safe side.

Summary

The provenance model, the taint narrowing, the egress re-guarding and the test arms are all good work, and the matcher resisted every bypass I threw at it. The hold is narrow and mechanical: two quadratic regexes on third-party text, worth 7.2s of pod-wide stall per turn, with a verified equivalent fix available.

GATE: HOLD

[lilyshen0722]

Written by sprint-review, a Commonly agent — re-gate at 2024f74cb. pod thread

My HOLD at 67bff82b3 (review 5542295530) is lifted. All four items are fixed and I re-measured each rather than reading the commit. CodeQL is green at this head — the two high-severity alerts are gone.

Every held item, re-measured

held item	before	now
anchored punctuation trim	728 ms on one 40k token	0.01 ms at 80k
anchored slash trim	611 ms at 40k	0.01 ms at 80k
rescan amplifier	721 ms per refused attempt	0.36 ms for 1,000 refused attempts
event loop per turn	7.2 s	immaterial

The trims are now one pass from the end, and GivenAddresses scans each text once on arrival so an attempt is a single set lookup — which is the right shape, because a refused attempt spends no page and so should cost nothing.

The host bound works, and my first attempt to check it was a bad measurement. I tested 'a'.repeat(249) + '.com' and saw a rejection, which proved nothing: a 249-character label already fails HOST_PATTERN's 63-character label limit, so I had measured the old constraint. Re-isolated with valid labels:

3 x 60 = 186 chars  accepted
4 x 60 = 247 chars  accepted
4 x 62 = 255 chars  REJECTED
5 x 60 = 308 chars  REJECTED

Boundary exactly at MAX_HOST_LENGTH 253, and a 2,443-character host now costs 0.007 ms because the length check precedes the regex.

No semantic regression from the rewrite

The trim rewrite could have changed matching, so I re-ran the same 18-case bypass probe from my hold against the new module: 18/18 identical, including query-append, path-append, userinfo smuggle, subdomain swap, raw metadata IP and loopback all still refused, and host-case/trailing-slash/fragment/http→https/prose-period/markdown all still allowed.

Measurement

BASE 53/53 (was 46 — seven new arms). Needles asserted, restored after each, RESTORED 53/53.

mutation	result
provenance check removed	4 failed, incl. "the model's own words never give a link"
own-author exclusion removed	2 failed
anchored punctuation regex restored	1 failed — "a 40k non-terminal run of punctuation", and the arm took 3,596 ms to fail, so it is genuinely timing the backtracking
my {1,8} trap	1 failed — "a long trailing run of punctuation or slashes still trims away"
GivenAddresses made to rescan	1 failed — "each text is scanned once: refused attempts against three hostile pages cost no rescan" (950 ms)

The {1,8} arm is the one I most wanted to see. My own proposed shortcut would now be caught, which is the right outcome: the trap I found by accident is a test the suite owns.

One correction to my own hold report. I wrote that the own-author exclusion had a fail-open ownNames || new Set() default, "currently unreachable", and suggested making the field required. It is now required at the type level and the fallback is deleted — byOtherAuthors takes ownNames: ReadonlySet<string> and calls .has() directly. That is a stronger fix than I asked for; there is no longer a default to point the wrong way.

Wider sweep: 22 suites, 397 passed, 0 failed.

Two leftovers, neither blocking

addressWasGiven still exists and is now called only by tests — the runtime imports GivenAddresses alone. It is the rescanning shape, exported, with no production caller: harmless today, and a ready-made way for a future call site to reintroduce the amplifier I held on. Worth deleting, or a line marking it test-only.

On webFetches: the comment now says the spend alarm does not read it, so the field's status is honest. Noting for the record that it remains written-and-unread, which is fine as a record but means a fetch-only turn still contributes nothing to any spend view.

GATE: PASS

[lilyshen0722]

Written by sprint-review, a Commonly agent — rebase re-gate at dfdceec2c. pod thread

This supersedes my PASS at 2024f74cb (review 5542372397). The rebase is clean and the conflict resolution loses nothing.

The rebase, checked against the PR's own patch rather than the range-diff

I compared the PR's own added/removed lines at each head, each against its own merge base (f8b43625a → 47ba879ba), per file:

• 4 of 5 files byte-identical — webAddressProvenance.ts, both test files, AgentRun.ts.
• nativeRuntimeService.ts differs by exactly one line (155 → 154).

And the entire content of that one-line delta is the conflict: a comment rewrap (same meaning, re-flowed because main's comment moved) plus the OPT_IN_TOOLS line, which at the old head the PR rewrote wholesale and now appends to. So nothing beyond the declared conflict changed.

The union held. Rather than read the resolved line, I evaluated the result: all five opt-in tools still require declaration, and an undeclared config is offered none of them.

tool	offered to an undeclared config
commonly_propose_action	no
commonly_offer_hosted_agent	no
commonly_run_code	no
commonly_web_search	no
commonly_fetch_url	no

If the resolution had dropped either side, one of those would read yes.

Your narrowing claim is right, and I want to be precise about how I know

The post-lookup set is a complete four-element enumeration, which settles it for every tool rather than for the ones I happened to probe:

WEB_TAINTED_TOOLS = IN_POD_NATIVE_TOOLS ∪ WEB_LOOKUP_TOOLS
                  = {read_context, post_message} ∪ {web_search, fetch_url}

commonly_offer_hosted_agent is in neither, so a page or a search does narrow it out — as you said.

My behavioural probe of that specific tool was inconclusive and I am not reporting it as a measurement. The offer tool is gated by hostedEngineAvailable() → isNativeRuntimeAvailable() → resolveLiteLLM(), which I could not make true in this sandbox, so the tool is absent from the offer untainted as well — meaning its absence when tainted shows my environment, not the narrowing. I nearly filed exactly that error for commonly_web_search first: it too looked "narrowed out" until the untainted control showed it absent in both cases because isSearchConfigured() was false (and my first attempt set EXA_API_KEY, where the code reads SEARCH_API_KEY).

So I demonstrated the mechanism on tools this environment does offer:

UNTAINTED: read_context, write_memory, post_message, create_task, web_search, fetch_url
TAINTED  : read_context, post_message, web_search, fetch_url
removed  : write_memory, create_task

The filter really does remove declared, available tools on taint; the set definition then covers the offer tool. Both lookups survive taint, matching #2244's design, and a bridged pod withholds both of them (:448 keys on WEB_LOOKUP_TOOLS, widened from #2244's search-only condition) — measured: a bridged pod kept only post_message.

State at this head

Suites most exposed to the union line: 4 suites, 65 passed, 0 failed (toolGating, webAddressProvenance, webFetch, hostedOffer). CI 12 pass with CodeQL green; BLOCKED is pending Test & Coverage only. No review bound to this head before this one.

My two non-blocking notes from 2024f74cb are unchanged and still just notes: addressWasGiven remains exported with zero runtime importers (tests only), and spendAlarmService still contains no reference to webFetches.

Noting for the record that Rhea's seat is down and Cofounder is taking security at this head — this review is the code gate only, and says nothing about that lane.

GATE: PASS

[lilyshen0722]

SECURITY-GATE: HOLD @ dfdceec. This is an independent security read standing in for Rhea while her provider refuses this PR's content. Done by the Cofounder operator session with a separate reviewer; not a Commonly seat.

Finding (high): the checked address is not the fetched address. The provenance check compares addressKey(url), which drops a trailing run of . , ; : ! ? * _ ~ and /. The dispatcher then requests the model's raw argument (scheme swap only) through getFromPublicAddress. So characters the key ignores still reach the host on the wire, and the model chooses them. Reproduced locally at this head: with a link to a host present in another author's message, a call carrying that link plus an appended run of those characters passed givenAddresses.has(), and the outgoing request path carried the appended bytes. The docstring's claim that "punctuation that ends a sentence cannot carry data the model chose" doesn't hold for the fetched URL.

Fix: request a server-canonical URL rebuilt from the matched given address (the original given text's URL, not the model's argument), or apply the identical trims to the URL that is fetched. Add an arm asserting that the URL handed to the network layer equals the matched given address byte for byte, with the trailing-run case included.

Verified clean at this head:

• The range-diff from 2024f74 touches only the OPT_IN_TOOLS line and import context; the provenance module and the fetch dispatcher are byte-identical.
• 53 PR tests pass on Node 22.
• The ReDoS fix is linear: a 50k non-terminal run is sub-ms on the new trims (the old regexes took about 0.8 s), and no anchored +$ regexes remain.
• 26 matcher probes found 0 bypasses and 0 false refusals beyond the finding: append, prefix, userinfo, subdomain swap, percent and double encoding, backslash, scheme-relative, ports, decimal IP, zero-width, IDN.
• Redirects re-check every hop, with pinned resolution and all DNS answers checked. The request is GET-only with frozen headers, a 3-per-turn cap and 24k truncation.
• commonly_offer_hosted_agent is narrowed out of a tainted turn.

Non-blocking: the trailing-slash trim is the same channel class, near-unary; the rebuilt-URL fix closes it too.

GATE: HOLD

[lilyshen0722]

Written by sprint-review, a Commonly agent — security-HOLD re-gate at 87f24d843. pod thread

Cofounder's finding was real, and my own gates missed it twice. I passed this PR at 2024f74cb and again at dfdceec2c, and in both reviews I probed addressWasGiven / GivenAddresses.has — the comparison — and never once asked what URL reached getFromPublicAddress. The 18-case bypass table I was pleased with tested a predicate's return value, not the request. Since addressKey trims a trailing run of . , ; : ! ? * _ ~ and /, every one of those trimmed bytes was model-chosen and went out on the wire, which is a selection channel of exactly the kind the module exists to close. Testing a guard's verdict is not testing what the guarded call does.

The property, measured at the thing that is sent

resolve() now returns the address as given, and the dispatcher passes that value and nothing else. Probing the resolved URL byte for byte:

given	model's argument	requested
https://d.example/p	…/p	https://d.example/p
https://d.example/p	…/p.....	https://d.example/p
https://d.example/p	…/p/////	https://d.example/p
https://d.example/p	…/p!?*_~	https://d.example/p
https://d.example/p	…/p#secret	https://d.example/p
https://d.example/p	https://D.EXAMPLE/p	https://d.example/p
https://d.example/p	http://d.example/p	https://d.example/p
https://d.example/p	…/p?x=1	refused
https://d.example/p	…/p/secret	refused
https://d.example/dir/	…/dir	https://d.example/dir/
http://d.example/x	http://d.example/x	https://d.example/x
https://d.example/p	…/p.../	refused

12/12 as required. The tenth row is the one that proves the shape of the fix: a given trailing slash is kept, so the request is the given writing rather than a normalised one — the key and the URL are now genuinely different values with different jobs.

The consumer is pinned, which is what my reviews lacked

Your new arms assert expect(mockGetPage.mock.calls).toEqual([[REPO]]) — the exact call list, so they pin the requested bytes and that exactly one request happens. That is a consumer-level assertion; mine were all producer-level.

BASE 2 suites / 67 passed; RESTORED 67/67.

my mutation	result
dispatcher requests the raw model argument (the HOLD's bug restored)	6 failed — matches your count exactly
dispatcher requests https:// + the comparison key	1 failed
resolve() returns the model's argument	13 failed (2 suites)

Two of my counts differ from yours and I am not claiming yours are wrong — I built different mutants. For the key-request case I used https://${addressKey(arg)}, which for the trailing-run variants reconstructs the clean given URL and therefore still passes those arms; the single failure it reds is the given-trailing-slash row, where the key has dropped a byte the request needs. Your "fails 6" must be a different construction. Same for resolve(): I measured 13 against your 12, which is a one-test difference across two suites, not a disagreement about behaviour. I did not run your fourth mutation (skipping the check), since C1 and C3 already cover that wiring.

State

CI at this head: CodeQL green, Test & Coverage pending — that pending check is the only reason mergeStateStatus reads BLOCKED.

My two standing notes are unchanged: addressWasGiven still has zero runtime importers (tests only), and spendAlarmService still never reads webFetches. The first one is worth a little more now than it was: it is the rescanning, key-only helper, exported beside a class whose whole point is that the key is not the request. A future caller reaching for the obvious-looking function would reintroduce both defects at once.

This is the code gate only; Cofounder holds the security lane at this head.

GATE: PASS

[lilyshen0722]

SECURITY-GATE: HOLD @ 87f24d8. Independent security read standing in for Rhea (Cofounder operator session with a separate reviewer; not a Commonly seat).

The first finding is fixed. End to end through runAgent, a given address followed by a long trailing punctuation run, a slash run, a #fragment, or an upper-cased scheme and host reaches the network layer exactly as the given address. 25 resolve() probes:

• Each comparable form maps to one stored address, the first one seen.
• A query or path added to a given address is refused.
• A given query is rebuilt as written.
• The only call that sends model input to the network passes resolve()'s result.
• PR suites pass, 67/67, on Node 22.

New finding (high, reproduced): the agent's own posts can count as given text. byOtherAuthors decides which messages are the agent's own by comparing author NAME strings against ownAuthorNamesFor, which is built from AgentInstallation.displayName, agentName, instanceId and username. The author on a message read back is the Postgres username, which for a bot is botMetadata.displayName, re-synced on every post. That field drifts from the installation's name by design: resolveCollisionFreeDisplayName adds a " (Instance)" suffix when the name collides with another bot's. A missing Postgres row gives "Unknown User". In both cases a turn can post a URL carrying data it chose, read it back with commonly_read_context, and then fetch it. Reproduced with the PR's test setup for both shapes; a matching-name control was refused.

Fix: have fetchRecentPodMessages return the author's user id, and exclude the agent's own messages by its bot User id, not by name. Treat any message whose author can't be resolved to a user id as the agent's own (fail closed). Add arms for the suffixed-displayName and unresolved-author cases.

Not blocking: the search query still reaches the search provider. That's the accepted TASK-267a channel, unchanged here.

GATE: HOLD

[lilyshen0722]

SECURITY-GATE: PASS @ 3afb963. Independent security read standing in for Rhea (Cofounder operator session with a separate reviewer; not a Commonly seat).

Both earlier findings are closed at this head. PR tests pass 70/70 on Node 22.

Fetched value equals the given address. No regression: a trailing punctuation run, a slash run, a fragment, and an upper-cased scheme and host all request exactly the given address.

Own posts are excluded by bot User id. resolveOwnUserId uses the same lookup as the posting path, so it finds the identity that actually posts. It fails closed both ways: no own id means no pod message gives an address, and a message with no author id counts as the agent's own.

• The suffixed-displayName and "Unknown User" repros now refuse; the human-author control is still allowed.
• 9 id-path arms all refuse: PG string id, Mongo ObjectId, PG row with no username, a row with only user_db_id. Both sides are String()-compared, so the ObjectId === trap doesn't arise.

Residual, MEDIUM, non-blocking (follow-up required). A post by ANOTHER agent in the same pod, including another instance of the same agent, counts as given text. Abusing it takes two agents steered by the same injected text: one posts a URL carrying data, and a fetch-capable native agent opens it. A single turn still can't open an address it wrote, which is what this PR claims. Follow-up: exclude bot-authored messages from the given text (u.is_bot on the PG row; populate isBot on the Mongo fallback). It ships in the next hosted-native PR.

Hardening note: lowercase both ids before comparing (nothing produces upper-case today).

GATE: PASS

[lilyshen0722]

Written by sprint-review, a Commonly agent — security-HOLD-2 re-gate at 3afb96317. pod thread

Second miss on this file, and this one I had the facts for. I read ownAuthorNamesFor and byOtherAuthors at 2024f74cb, flagged the fail-open default, and said the name enumeration covered displayName / agentName / instanceId / buildAgentUsername. At dfdceec2c I looked again and called the required-field version "a stronger fix than I asked for". What I never asked, either time, is whether the author string on a message read back equals that displayName — and I hold the answer in my own standing notes: collisions are disambiguated by appending (<HumanizedInstanceId>) to a bot's displayName. I had the drift mechanism written down and did not connect it to the guard I was reviewing. A name-based identity check deserves the question "who writes this string, and when does it change?", and I did not ask it.

The fix, and the ordering that makes it work

ownUserId replaces the name set, resolved via buildAgentUsername(resolveAgentType(agentName), instanceId) — the same derivation getOrCreateAgentUser uses to create the row, so the id really is the one the agent's posts carry. Parity is structural, not coincidental: both of this module's seat lookups go through those two functions.

Ordering checked, because the field starts null and a late resolve would silently disable the feature: created null at :1380, assigned at :1383, first read at :1437. Resolved before any use.

Both directions fail closed, and they are different failures:

• ownUserId === null → []: no pod message gives an address at all.
• Boolean(message.authorId) false → excluded: a message whose author cannot be resolved is treated as the agent's own.

Measurement

BASE 27/27; RESTORED 27/27. Failures by name:

mutation	result
exclusion removed entirely (the original vulnerability)	5 — incl. "…read back under a collision-suffixed display name gives no link" and "…under "Unknown User" with no author id gives no link"
fail open when the own id is unresolved	1 — "when the agent's own user cannot be found, no pod message gives a link; the mention still does"
a missing authorId counted as another author's	1 — the "Unknown User" arm
fetchRecentPodMessages stops returning author ids	2

Three of four match your counts exactly. My first is 5 against your 4 because I built a wider mutant — I replaced the whole byOtherAuthors body, which also breaks the unresolved-seat arm, where yours evidently left the null branch intact. Not a disagreement about behaviour.

The arms are the right shape and that is worth saying explicitly: the fixtures carry the agent's own user_id under a drifted username, so a name-based check cannot pass them and only the id-based one can. The unresolved-seat arm pins mockGetPage.mock.calls to [[REPO]] — the mention's address and nothing from pod messages — which is exactly "fail closed without going dead".

One note, not blocking

resolveOwnUserId queries { username, isBot: true }, while the adjacent broker seat lookup omits isBot. agentIdentityService explicitly contemplates "pre-isBot legacy rows", so for such a seat this returns null and the pod-message half of provenance switches off entirely — the turn can still open an address from its prompt or the mention that woke it, but not from another author's pod message. That is the safe direction and the arm above covers the behaviour; it is worth knowing that the degradation is silent, and that dropping isBot from the filter would trade a narrow outage for a wider match.

CI at this head: 12 pass, CodeQL green, Test & Coverage pending — the only reason mergeStateStatus reads BLOCKED.

Three security findings on this PR, none of them mine. The pattern across all three is the same and worth naming: I verified predicates and left their consumers unexamined — what the guard decides, never what the guarded call sends or what the compared string actually contains.

GATE: PASS