Verify FEP-044f quote authorization stamps#33
Conversation
Add validation for FEP-044f quote authorization stamps when materializing received messages, exposing the result through Message.quoteApproved. Route quote authorization Delete activities through the received stamp reference index, strip revoked quotes after forwarding the Delete, and fire Bot.onQuoteRevoked after the Update is sent. Add reverse lookup support across the in-memory, KV, SQLite, and PostgreSQL repositories, plus tests and documentation for the new behavior. Fixes fedify-dev#30 Refs fedify-dev#27 Assisted-by: Codex:gpt-5.5
|
@codex review |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds quote approval verification, reverse-index lookup for quote authorization references, quote revocation handling, and documentation updates. It computes ChangesQuote Approval and Revocation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b21b1f11ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for verifying and revoking FEP-044f quote authorization stamps. It adds a new onQuoteRevoked event handler, introduces the Message.quoteApproved property, and implements the findQuoteAuthorizationReferenceIdentifiers method across database repositories to trace which bots reference a given stamp. The review feedback highlights a critical bug where the BotImpl instance is incorrectly passed to ctx.forwardActivity instead of the Delete activity, potential runtime TypeError crashes due to missing null checks on actor.id and targetActorId, and a redundant call to #hasValidQuoteAuthorization that can be safely removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/botkit/src/repository.ts (1)
1017-1101: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
migrate()doesn't rebuild the new quote-authorization reference index for legacy data.
migrate()special-cases only the"followees"category to rebuild its reverse index via#addToFolloweeIndex(lines 1084-1098), but"quoteAuthorizationRefs"(included incategoriesat line 1063) is copied as raw KV entries without a corresponding call to#addToQuoteAuthorizationReferenceIndex. SincefindQuoteAuthorizationReferenceIdentifiersonly reads from this materialized index (unlike the Postgres/SQLite backends, which query the table directly), legacy-migrated quote-authorization references become permanently invisible to reverse lookup, breaking Delete-triggered quote revocation for bots that adopted BotKit 0.4 data.🛠️ Proposed fix
if (category === "followees" && rest.length === 1) { let followeeId: URL; try { followeeId = new URL(rest[0]); } catch (error) { logger.warn( "Skipping the malformed legacy followee key {followeeId}.", { followeeId: rest[0], error }, ); continue; } await this.#addToFolloweeIndex(identifier, followeeId); } + if (category === "quoteAuthorizationRefs" && rest.length === 1) { + let authorizationUrl: URL; + try { + authorizationUrl = new URL(rest[0]); + } catch (error) { + logger.warn( + "Skipping the malformed legacy quote authorization reference key {authorization}.", + { authorization: rest[0], error }, + ); + continue; + } + await this.#addToQuoteAuthorizationReferenceIndex( + identifier, + authorizationUrl, + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/repository.ts` around lines 1017 - 1101, migrate() is copying legacy quote-authorization reference rows but never rebuilds the reverse lookup index used by findQuoteAuthorizationReferenceIdentifiers. Update the migration loop in KvRepository.migrate to mirror the followees special case for the "quoteAuthorizationRefs" category by parsing each legacy reference key and calling `#addToQuoteAuthorizationReferenceIndex` after copying the raw entry, while preserving the existing retry/no-op behavior and letting malformed keys or storage errors behave consistently with the followees handling.
🧹 Nitpick comments (4)
packages/botkit/src/message-impl.test.ts (1)
271-283: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a test for a fetched authorization whose
iddoesn't match the requestedquoteAuthorizationId.
validateQuoteAuthorizationseparately checksauthorization.id.href === options.authorizationId.href(distinct from the cross-origin check already tested), guarding against a server returning a different authorization object than the one referenced. This path isn't currently exercised.🧪 Proposed additional test
+ await t.test("mismatched authorization id", async () => { + const message = await materialize({ + quote: targetId, + quoteAuthorization: authorizationId, + authorization: new QuoteAuthorization({ + id: new URL("https://author.example/stamps/other"), + attribution: author.id, + interactingObject: quoteId, + interactionTarget: targetId, + }), + }); + assert.deepStrictEqual(message.quoteApproved, false); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/message-impl.test.ts` around lines 271 - 283, Add a test in message-impl.test.ts that exercises validateQuoteAuthorization with a fetched QuoteAuthorization whose id does not match the requested quoteAuthorizationId. Reuse the existing materialize/setup pattern from the valid stamp test, but make the returned authorization object have a different id than the one passed in quoteAuthorization, and assert the failure/false result that proves validateQuoteAuthorization checks authorization.id.href against options.authorizationId.href in addition to the cross-origin validation.packages/botkit/src/message-impl.ts (1)
984-992: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the already-computed
documentLoaderinstead of recreating it.
createMessagealready buildsdocumentLoaderat line 793 for use throughout the function, butverifyQuoteApprovalindependently callssession.context.getDocumentLoader(session.bot)again, duplicating that work on every quote verification.♻️ Proposed fix: pass the existing loader down
async function verifyQuoteApproval<TContextData>( raw: MessageClass, quoteTarget: Message<MessageClass, TContextData>, session: SessionImpl<TContextData>, + documentLoader: DocumentLoader, ): Promise<boolean> { ... : await session.context.lookupObject( raw.quoteAuthorizationId, { contextLoader: session.context.contextLoader, - documentLoader: await session.context.getDocumentLoader( - session.bot, - ), + documentLoader, }, );And update the call site to pass the existing
documentLoadervariable fromcreateMessage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/message-impl.ts` around lines 984 - 992, In createMessage, verifyQuoteApproval is redundantly calling session.context.getDocumentLoader(session.bot) instead of using the already-built documentLoader from earlier in the function. Update the verifyQuoteApproval call site to pass the existing documentLoader variable through, and adjust verifyQuoteApproval’s parameter usage so it reuses that loader rather than recreating it.packages/botkit/src/bot-impl.test.ts (2)
4590-4646: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame boolean-assertion anti-pattern here.
revokedis a plain boolean flag; Line 4640'sassert.deepStrictEqual(revoked, false)should beassert.ok(!revoked).As per coding guidelines: "avoid
assert.equal(..., true)orassert.equal(..., false)in favor ofassert.ok(...)/assert.ok(!...)."♻️ Proposed fix
- assert.deepStrictEqual(revoked, false); + assert.ok(!revoked);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/bot-impl.test.ts` around lines 4590 - 4646, The test in BotImpl.onDeleted() is using a boolean anti-pattern for the revoked flag. Update the assertion after the Delete handling to use assert.ok(!revoked) instead of a deep-equality false check, keeping the rest of the quote revocation behavior verification in this test unchanged.Source: Coding guidelines
4437-4496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assert.ok(!...)for the boolean flag instead ofdeepStrictEqual.
rejectedCalledis a plain boolean, soassert.deepStrictEqual(rejectedCalled, false)at Line 4490 is the same anti-pattern the guideline calls out forassert.equal(..., false).As per coding guidelines: "avoid
assert.equal(..., true)orassert.equal(..., false)in favor ofassert.ok(...)/assert.ok(!...)."♻️ Proposed fix
- assert.deepStrictEqual(rejectedCalled, false); + assert.ok(!rejectedCalled);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/bot-impl.test.ts` around lines 4437 - 4496, The test in BotImpl.onDeleted() is asserting a plain boolean with deepStrictEqual, which should follow the boolean assertion guideline instead. Update the rejectedCalled check near the quote revocation assertions to use assert.ok(!rejectedCalled) rather than comparing it to false, keeping the surrounding revocation checks for revoked and revoker unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/botkit/src/message-impl.ts`:
- Around line 960-1003: verifyQuoteApproval currently performs
session.context.lookupObject without any cancellation, so a slow quote
authorization fetch can hang message processing. Thread an AbortSignal from
createMessage through verifyQuoteApproval and into the lookupObject call (or add
a timeout wrapper there), keeping the existing repository path and validation
flow intact.
In `@packages/botkit/src/quote-authorization.ts`:
- Around line 18-37: Add JSDoc to the exported API in quote-authorization.ts:
document QuoteAuthorizationValidationOptions and validateQuoteAuthorization with
a short purpose summary, parameter descriptions, and the return type/meaning for
the type guard. Include a `@since` tag with the appropriate version number for
both exported symbols, and keep the comments attached to the identifiers so they
remain discoverable even if code moves.
---
Outside diff comments:
In `@packages/botkit/src/repository.ts`:
- Around line 1017-1101: migrate() is copying legacy quote-authorization
reference rows but never rebuilds the reverse lookup index used by
findQuoteAuthorizationReferenceIdentifiers. Update the migration loop in
KvRepository.migrate to mirror the followees special case for the
"quoteAuthorizationRefs" category by parsing each legacy reference key and
calling `#addToQuoteAuthorizationReferenceIndex` after copying the raw entry,
while preserving the existing retry/no-op behavior and letting malformed keys or
storage errors behave consistently with the followees handling.
---
Nitpick comments:
In `@packages/botkit/src/bot-impl.test.ts`:
- Around line 4590-4646: The test in BotImpl.onDeleted() is using a boolean
anti-pattern for the revoked flag. Update the assertion after the Delete
handling to use assert.ok(!revoked) instead of a deep-equality false check,
keeping the rest of the quote revocation behavior verification in this test
unchanged.
- Around line 4437-4496: The test in BotImpl.onDeleted() is asserting a plain
boolean with deepStrictEqual, which should follow the boolean assertion
guideline instead. Update the rejectedCalled check near the quote revocation
assertions to use assert.ok(!rejectedCalled) rather than comparing it to false,
keeping the surrounding revocation checks for revoked and revoker unchanged.
In `@packages/botkit/src/message-impl.test.ts`:
- Around line 271-283: Add a test in message-impl.test.ts that exercises
validateQuoteAuthorization with a fetched QuoteAuthorization whose id does not
match the requested quoteAuthorizationId. Reuse the existing materialize/setup
pattern from the valid stamp test, but make the returned authorization object
have a different id than the one passed in quoteAuthorization, and assert the
failure/false result that proves validateQuoteAuthorization checks
authorization.id.href against options.authorizationId.href in addition to the
cross-origin validation.
In `@packages/botkit/src/message-impl.ts`:
- Around line 984-992: In createMessage, verifyQuoteApproval is redundantly
calling session.context.getDocumentLoader(session.bot) instead of using the
already-built documentLoader from earlier in the function. Update the
verifyQuoteApproval call site to pass the existing documentLoader variable
through, and adjust verifyQuoteApproval’s parameter usage so it reuses that
loader rather than recreating it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4feb207-6c24-4774-bc63-c4f14a131e7f
📒 Files selected for processing (20)
CHANGES.mddocs/concepts/events.mddocs/concepts/message.mdpackages/botkit-postgres/src/mod.test.tspackages/botkit-postgres/src/mod.tspackages/botkit-sqlite/src/mod.test.tspackages/botkit-sqlite/src/mod.tspackages/botkit/src/bot-impl.test.tspackages/botkit/src/bot-impl.tspackages/botkit/src/bot.test.tspackages/botkit/src/bot.tspackages/botkit/src/events.tspackages/botkit/src/instance-impl.tspackages/botkit/src/instance-routing.test.tspackages/botkit/src/message-impl.test.tspackages/botkit/src/message-impl.tspackages/botkit/src/message.tspackages/botkit/src/quote-authorization.tspackages/botkit/src/repository.test.tspackages/botkit/src/repository.ts
Legacy quote authorization reference rows were copied into the scoped key space, but their reverse lookup index also has to be rebuilt while the adopting bot owns the migration. Rebuilding it in the same pass keeps lookup behavior consistent after migration. Malformed legacy keys are skipped like malformed followees, while storage failures still leave the done marker unwritten so a later migration attempt can retry the index write. Addresses review feedback from: fedify-dev#33 (review) Assisted-by: Codex:gpt-5.5
Quote authorization revocations now require the recorded attribution instead of trusting every actor on the stamp origin. Remote Delete forwarding is limited to quotes whose audience is already shared with followers, which avoids exposing private quote stamps to unrelated followers. Remote quote authorization lookups now receive the createMessage abort signal, and the validation helper documents its exported API while rejecting missing target actors explicitly. fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for FEP-044f quote authorization stamp revocations and verification of quote approvals on received third-party quote posts. It adds the onQuoteRevoked event handler, the Message.quoteApproved property, and new repository methods to find bot actors referencing a stamp. However, a critical security issue was identified in bot-impl.ts where the return value of this.#hasValidQuoteAuthorization is ignored, bypassing the authorization requirement. Additionally, several potential runtime crashes were found where calling new repository methods on custom repositories could throw a TypeError if they are not implemented; defensive checks should be added to ensure backward compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Local FEP-044f quote handling now stops before user handlers when the stored authorization is missing, invalid, or revoked during the audience check. The cleanup path still runs first so widened local quote stamps are revoked before the activity is dropped. Older custom repository implementations may not yet expose the new quote authorization reverse-index methods. Treat those methods as optional at runtime and fall back to empty routing or missing attribution instead of crashing. fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for verifying FEP-044f quote authorization stamps on received third-party quote posts and handling their revocation. It adds the Message.quoteApproved property to check for valid FEP-044f approval, introduces the QuoteRevokedEventHandler type and Bot.onQuoteRevoked event handler, and implements the findQuoteAuthorizationReferenceIdentifiers method across Postgres, SQLite, KV, and Memory repositories to track which bots depend on a given authorization stamp. When an authorization stamp is deleted, BotKit now forwards the Delete activity, strips the quote, and triggers the onQuoteRevoked event. As there are no review comments, no additional feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/botkit/src/bot-impl.ts (2)
2456-2482: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilent fail-closed for repositories without the new reverse-index methods.
When the underlying repository doesn't implement
findQuoteAuthorizationReferenceAttribution, this delegate returnsundefinedwith no logging. Downstream,#validateQuoteAuthorizationDeletion()(Line 1033) treatsundefinedattribution as a mismatch and always returnsundefined— meaning any repository that hasn't been upgraded with this optional method will silently and permanently ignore every incoming quote-authorization revocation, with no signal to the operator that the feature is inert. The same applies tofindQuoteAuthorizationReferenceIdentifiers(Lines 2460-2463), used by migration/cleanup.Since this is a deliberate compatibility fallback (per the PR's commit notes) rather than a bug, the main gap is observability: operators running a custom/legacy
Repositoryimplementation have no way to discover that revocation handling is silently disabled.📝 Suggested fix: log once when the fallback is hit
async findQuoteAuthorizationReferenceAttribution( identifier: string, authorization: URL, ): Promise<URL | undefined> { await this.#migration; if ( typeof this.#repository.findQuoteAuthorizationReferenceAttribution !== "function" - ) return undefined; + ) { + logger.warn( + "The repository does not implement " + + "findQuoteAuthorizationReferenceAttribution(); quote " + + "authorization revocations will be silently ignored until it " + + "is upgraded.", + ); + return undefined; + } return await this.#repository.findQuoteAuthorizationReferenceAttribution( identifier, authorization, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/bot-impl.ts` around lines 2456 - 2482, The fallback in bot-impl.ts silently returns when the repository lacks the new reverse-index methods, so add a one-time warning for observability in both findQuoteAuthorizationReferenceAttribution and findQuoteAuthorizationReferenceIdentifiers. Use the existing BotImpl methods and the `#repository` capability checks to log once when these optional methods are missing, making it clear that quote-authorization revocation/cleanup is disabled for this repository implementation. Keep the compatibility behavior unchanged, but ensure the operator can detect the fallback through the logger.
882-887: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard against stripping a newer quote authorization.
onDeleted()uses a staleobjectsnapshot, but#stripRejectedQuote()only checksquoteId. If the message is re-authorized with a newquoteAuthorizationIdfor the same target beforeupdateMessage()runs, the old delete can still strip the newer authorization. CompareexistingObject.quoteAuthorizationId?.hrefwith the authorization being revoked before updating.Proposed fix
if ( !isMessageObject(existingObject) || existingObject.id == null || - (quoteId != null && existingObject.quoteId?.href !== quoteId.href) + (quoteId != null && existingObject.quoteId?.href !== quoteId.href) || + existingObject.quoteAuthorizationId?.href !== + object.quoteAuthorizationId?.href ) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/bot-impl.ts` around lines 882 - 887, Guard the quote revocation path in onDeleted()/#stripRejectedQuote so it does not strip a newer authorization when the stale object snapshot is older than the current message state. Update `#stripRejectedQuote` to load or compare the latest existingObject.quoteAuthorizationId?.href against the quote authorization being revoked before calling updateMessage, and only strip when they match. Use the existing `#stripRejectedQuote` and onDeleted flow to locate the fix and ensure the deletion handler ignores outdated quoteId-only matches.
🧹 Nitpick comments (3)
packages/botkit/src/quote-authorization.test.ts (1)
21-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThin coverage: only the null-
targetActorIdshort-circuit is tested.The test only exercises the branch where
options.targetActorId == null, which short-circuitsvalidateQuoteAuthorization()before any of the URL/origin comparisons run. Given this function is the core FEP-044f gate (blocking unapproved quotes, gating revocations), it would be valuable to also cover: the accepted/true path, anattributionIdmismatch, aninteractingObjectId/interactionTargetIdmismatch, anauthorizationIdmismatch, and a non-QuoteAuthorizationinput.✅ Example additional cases
test("validateQuoteAuthorization() accepts a matching stamp", () => { const targetActorId = new URL("https://example.com/users/alice"); const authorization = new QuoteAuthorization({ id: new URL("https://example.com/stamps/1"), attribution: targetActorId, interactingObject: new URL("https://quote.example/notes/1"), interactionTarget: new URL("https://example.com/notes/1"), }); assert.ok( validateQuoteAuthorization(authorization, { quoteId: new URL("https://quote.example/notes/1"), targetId: new URL("https://example.com/notes/1"), targetActorId, }), ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/quote-authorization.test.ts` around lines 21 - 37, The current quote authorization test only covers the null targetActorId short-circuit in validateQuoteAuthorization(), so add coverage for the full decision path using QuoteAuthorization. Extend quote-authorization.test.ts with cases for a valid matching authorization that returns true, mismatches on attributionId, interactingObjectId/interactionTargetId, and authorizationId that return false, plus a non-QuoteAuthorization input case. Use the existing validateQuoteAuthorization() helper and QuoteAuthorization constructor to keep the tests anchored to the same symbols.packages/botkit/src/bot-impl.test.ts (1)
2865-2879: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
findQuoteAuthorizationReferenceAttributionfallback.This test only covers
MigrationGatedRepositorytolerating a missingfindQuoteAuthorizationReferenceIdentifiers. It doesn't cover the sibling fallback forfindQuoteAuthorizationReferenceAttribution, which is the one that actually gates whetheronDeleted()can validate/process a revocation (see the related comment inbot-impl.ts). A regression test asserting it returnsundefinedwhen the underlying method is absent would help lock in that fail-closed behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/bot-impl.test.ts` around lines 2865 - 2879, Add missing coverage for the sibling fallback in MigrationGatedRepository: the current test only verifies findQuoteAuthorizationReferenceIdentifiers is tolerated when absent, but it should also exercise findQuoteAuthorizationReferenceAttribution on the same wrapped repository. Update bot-impl.test.ts by extending the existing MigrationGatedRepository test (or adding a parallel one) to hide findQuoteAuthorizationReferenceAttribution on the underlying MemoryRepository and assert the repository returns undefined for a quote stamp URL. Use the MigrationGatedRepository and onDeleted-related fallback behavior as the target symbols to keep the regression anchored.packages/botkit/src/repository.test.ts (1)
60-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
hideRepositoryMethodshelper across test files.The same implementation of
hideRepositoryMethodsis defined inpackages/botkit/src/bot-impl.test.tsandpackages/botkit/src/instance-routing.test.ts. Since this helper directly encodes the "missing reverse-index method" contract used to test critical fallback behavior (revocation-related), keeping three independent copies risks silent drift if the detection semantics ever need to change.♻️ Suggested consolidation
-function hideRepositoryMethods( - repository: Repository, - hiddenMethods: readonly PropertyKey[], -): Repository { - return new Proxy(repository, { - get(target, prop, receiver) { - if (hiddenMethods.includes(prop)) return undefined; - const value = Reflect.get(target, prop, receiver); - return typeof value === "function" ? value.bind(target) : value; - }, - }); -} +import { hideRepositoryMethods } from "./test-utils.ts"; // shared helper🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/repository.test.ts` around lines 60 - 72, The `hideRepositoryMethods` helper is duplicated across the botkit test files, so consolidate it into a shared test utility and import it from `repository.test.ts`, `bot-impl.test.ts`, and `instance-routing.test.ts`. Keep the existing `Proxy`/`get` behavior and `PropertyKey[]` contract unchanged, and update the affected tests to use the single shared helper so the reverse-index fallback semantics stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/botkit/src/bot-impl.ts`:
- Around line 2456-2482: The fallback in bot-impl.ts silently returns when the
repository lacks the new reverse-index methods, so add a one-time warning for
observability in both findQuoteAuthorizationReferenceAttribution and
findQuoteAuthorizationReferenceIdentifiers. Use the existing BotImpl methods and
the `#repository` capability checks to log once when these optional methods are
missing, making it clear that quote-authorization revocation/cleanup is disabled
for this repository implementation. Keep the compatibility behavior unchanged,
but ensure the operator can detect the fallback through the logger.
- Around line 882-887: Guard the quote revocation path in
onDeleted()/#stripRejectedQuote so it does not strip a newer authorization when
the stale object snapshot is older than the current message state. Update
`#stripRejectedQuote` to load or compare the latest
existingObject.quoteAuthorizationId?.href against the quote authorization being
revoked before calling updateMessage, and only strip when they match. Use the
existing `#stripRejectedQuote` and onDeleted flow to locate the fix and ensure the
deletion handler ignores outdated quoteId-only matches.
---
Nitpick comments:
In `@packages/botkit/src/bot-impl.test.ts`:
- Around line 2865-2879: Add missing coverage for the sibling fallback in
MigrationGatedRepository: the current test only verifies
findQuoteAuthorizationReferenceIdentifiers is tolerated when absent, but it
should also exercise findQuoteAuthorizationReferenceAttribution on the same
wrapped repository. Update bot-impl.test.ts by extending the existing
MigrationGatedRepository test (or adding a parallel one) to hide
findQuoteAuthorizationReferenceAttribution on the underlying MemoryRepository
and assert the repository returns undefined for a quote stamp URL. Use the
MigrationGatedRepository and onDeleted-related fallback behavior as the target
symbols to keep the regression anchored.
In `@packages/botkit/src/quote-authorization.test.ts`:
- Around line 21-37: The current quote authorization test only covers the null
targetActorId short-circuit in validateQuoteAuthorization(), so add coverage for
the full decision path using QuoteAuthorization. Extend
quote-authorization.test.ts with cases for a valid matching authorization that
returns true, mismatches on attributionId,
interactingObjectId/interactionTargetId, and authorizationId that return false,
plus a non-QuoteAuthorization input case. Use the existing
validateQuoteAuthorization() helper and QuoteAuthorization constructor to keep
the tests anchored to the same symbols.
In `@packages/botkit/src/repository.test.ts`:
- Around line 60-72: The `hideRepositoryMethods` helper is duplicated across the
botkit test files, so consolidate it into a shared test utility and import it
from `repository.test.ts`, `bot-impl.test.ts`, and `instance-routing.test.ts`.
Keep the existing `Proxy`/`get` behavior and `PropertyKey[]` contract unchanged,
and update the affected tests to use the single shared helper so the
reverse-index fallback semantics stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 381efd78-c38f-407f-820f-aa6eeeebfa2c
📒 Files selected for processing (10)
packages/botkit/src/bot-impl.test.tspackages/botkit/src/bot-impl.tspackages/botkit/src/instance-impl.tspackages/botkit/src/instance-routing.test.tspackages/botkit/src/message-impl.test.tspackages/botkit/src/message-impl.tspackages/botkit/src/quote-authorization.test.tspackages/botkit/src/quote-authorization.tspackages/botkit/src/repository.test.tspackages/botkit/src/repository.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/botkit/src/instance-impl.ts
- packages/botkit/src/quote-authorization.ts
- packages/botkit/src/message-impl.test.ts
- packages/botkit/src/instance-routing.test.ts
- packages/botkit/src/message-impl.ts
- packages/botkit/src/repository.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ecb6aa95d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Stale quote authorization deletes can arrive after a message has already recorded a newer stamp. Compare the current stored authorization before stripping quote metadata so an old delete only removes the old reference. The migration-gated repository now warns once when a custom repository omits the optional quote reference index methods, preserving compatibility while making disabled cleanup visible. The tests cover the sibling fallback path, the full quote authorization validator decision table, and the stale delete race. The shared repository method-hiding helper keeps the fallback tests aligned. fedify-dev#33 (review) Assisted-by: Codex:gpt-5.5
The revoked stamp Delete used to go only to followers, so direct quotes and mention-expanded audiences could miss the revocation even though they received the quoted message. The forwarding path now rebuilds concrete recipients from the stored message audience and mention tags, while keeping followers as a shared inbox fan-out when the original audience included them. fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
Remote quote stamp lookups can reject because the caller aborted the message materialization. Treating that as an ordinary failed stamp lookup lets cancelled work continue with a false approval result. The verification path now rethrows abort failures while still suppressing ordinary lookup and validation failures as an unapproved quote. fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/botkit/src/quote-authorization.test.ts`:
- Around line 53-98: Add a test case in validateQuoteAuthorization() that
specifically exercises the origin guard in validateQuoteAuthorization by keeping
all other bindings valid but giving the authorization.id a different origin than
targetActorId. Use the existing createAuthorization helper and the
validateQuoteAuthorization test table in quote-authorization.test.ts to forge a
cross-origin stamp, then assert the function returns false for that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cff2a86-b2a0-4abf-b82f-35623d1a9e19
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/botkit/src/bot-impl.test.tspackages/botkit/src/bot-impl.tspackages/botkit/src/helpers.tspackages/botkit/src/instance-routing.test.tspackages/botkit/src/message-impl.test.tspackages/botkit/src/message-impl.tspackages/botkit/src/quote-authorization.test.tspackages/botkit/src/repository.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/botkit/src/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/botkit/src/message-impl.test.ts
- packages/botkit/src/repository.test.ts
- packages/botkit/src/message-impl.ts
- packages/botkit/src/bot-impl.test.ts
- packages/botkit/src/bot-impl.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b45faf7cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces support for verifying FEP-044f quote authorization stamps on received third-party quote posts and handling revoked quote authorizations, adding the onQuoteRevoked event handler and the Message.quoteApproved property. It also updates Postgres, SQLite, and KV repositories to track quote authorization references. The review feedback suggests guarding against a null or undefined fepQuoteUrl when validating quote authorizations to prevent runtime errors, and optimizing sequential network lookups of mentioned actors in a loop by executing them in parallel using Promise.all.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The revocation forwarder now resolves message addressees in parallel, keeps the original recipient order, and includes the reply target actor before the quoted actor. That matches the audience used for quote updates so every server that saw the quote can also see the stamp Delete. The validation tests also cover cross-origin stamp IDs directly, which locks down the origin binding separately from the authorization ID match. fedify-dev#33 (comment) fedify-dev#33 (comment) fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements support for verifying FEP-044f quote authorization stamps on received third-party quote posts and handling revoked stamps. It introduces the onQuoteRevoked event handler, the Message.quoteApproved property, and new repository methods to track and query quote authorization references across bot instances. When a quote authorization is deleted, BotKit now forwards the deletion to the quote's audience and strips the quote target. Feedback on the changes suggests simplifying the abortion check in verifyQuoteApproval to robustly propagate any error when signal?.aborted is true, rather than checking for specific error types.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6000a26c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Abort handling should not depend on the exact error type thrown by the remote lookup path. Once the caller's signal is aborted, quote stamp verification now propagates the error instead of treating it as an ordinary failed validation. Quote rejection and revocation also no longer materialize the stripped message unless a handler is registered. The stored message is still updated and the audience update is still sent, but handler-only lookup work is skipped when there is no callback to consume it. fedify-dev#33 (comment) fedify-dev#33 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request implements support for verifying and revoking quote authorization stamps under FEP-044f. It introduces the Bot.onQuoteRevoked event handler, the Message.quoteApproved property, and updates the Postgres, SQLite, and KV repositories with reverse indexing methods to track quote authorization references. When a quote authorization is deleted, BotKit now forwards the deletion activity and strips the quote from the bot's post. The review feedback highlights two critical issues in packages/botkit/src/bot-impl.ts where missing null checks on fepQuoteUrl and actor.id could lead to TypeScript compilation errors or runtime TypeErrors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #30.
Why this shape
FEP-044f approval has to be checked at the point where a received message is materialized, not only when the activity first arrives. A stamp can disappear or change after the quote was stored, so packages/botkit/src/message-impl.ts dereferences and validates the stamp each time it computes
Message.quoteApproved. The helper in packages/botkit/src/quote-authorization.ts keeps the validation rules shared between materialization and incoming approval handling, so the origin, attribution, quote object, and target object checks cannot drift apart.Self-quotes are treated as approved because they do not need a remote authorization stamp. Everything else is conservative: a missing stamp, a failed fetch, a mismatched
interactingObject, a mismatchedinteractionTarget, a mismatchedattributedTo, or a stamp from the wrong origin becomesfalserather than an exception. That gives event handlers a clear signal without making timeline ingestion brittle.How revocation is routed
A deleted quote authorization stamp may be delivered to the shared inbox without addressing the local bot that used it. Routing only through
toandccwould miss that Delete, especially for dynamic bot groups. packages/botkit/src/instance-impl.ts therefore asks the repository for every bot identifier that references the deleted stamp URI and resolves those identifiers through the normal instance routing path.Once a bot receives the Delete, packages/botkit/src/bot-impl.ts validates the revoker by origin instead of by exact actor URI. That matches how servers often send administrative Deletes from a same-origin service actor. The original Delete is forwarded before the local quote is stripped, preserving the remote revocation activity for followers first; the local message is then updated without the quote target, and
Bot.onQuoteRevokedfires after the Update has been sent.How storage supports routing
The received-stamp reference index is intentionally repository-level rather than actor-scoped. Shared-inbox routing starts before the instance knows which bot should handle the Delete, so the lookup has to answer “which identifiers reference this stamp?” rather than “which message does this one bot reference?”
packages/botkit/src/repository.ts adds that reverse lookup for the KV, memory, and memory-cached repositories. packages/botkit-sqlite/src/mod.ts and packages/botkit-postgres/src/mod.ts add database indexes on the authorization URI so the same query stays cheap for persistent repositories. The existing actor-scoped reference methods still handle per-bot message lookup and cleanup after the route has been resolved.
Validation
The tests exercise the failure modes that matter for federation rather than only the happy path: invalid stamps, legacy quotes without stamps, failed stamp fetches, same-origin service revocations, wrong-origin Deletes, unaddressed shared-inbox Deletes, and dynamic bot group routing. The repository tests cover the new reverse lookup across the in-memory, KV, SQLite, and PostgreSQL implementations.
I ran
mise run testandmise run docs:build. The commit hook also randeno check,deno lint,deno fmt --check,hongdown --check, anddeno publish --dry-run.Summary by CodeRabbit
Message.quoteApprovedto report whether an incoming quote target is approved (or not applicable).onQuoteRevokedso bots can react when a quote authorization is revoked.