[No QA] Migrate ProposalPolice to Responses + Conversations API - #97199
Conversation
Replaces the deprecated Assistants thread/run polling helpers (promptAssistant, parseAssistantResponse) with createConversation, addConversationItems, and a generic parseJSONResponse<T>, so callers can use the Responses API with persistent Conversations instead.
ProposalUtils holds the proposal/bot-detection helpers previously private to proposalPoliceComment.ts, so the new conversation-tracking utils can share them without a reverse import. GithubUtils.pinIssue is a best-effort GraphQL pin (the REST API has no equivalent) used to surface ProposalPolice's per-issue duplicate-check tracking comment; failures are swallowed since pinning is a convenience, not something the caller depends on.
Splits the dashboard-configured Assistant prompt into small, focused fragments (template definition, template/edit examples, decision tree, bot actions, duplicate detection) with per-call-type assemblers, so each Responses API call only gets the instructions it actually needs instead of the whole prompt every time. Also drops a stale "re-state the problem" section from the examples that no longer matches the proposal template. Adds JSON-schema definitions and type guards for the three response shapes (template-check, edit-check, duplicate-check), replacing the "respond with JSON" instructions previously baked into the prompt text.
Pure, independently-testable helpers for the duplicate-check Conversation flow: finding a tracked Conversation ID from a hidden marker on a bot-authored comment, building the tracking comment body, seeding items for prior proposals, and chunking items to OpenAI's 20-per-call Conversation limit.
Swaps promptAssistant for promptResponses on gpt-5.6-luna (replacing the Assistant's GPT-4o) for template-check and edit-check, and replaces the per-prior-proposal Assistants loop with a single Responses call against a persistent per-issue Conversation for duplicate-check. Removes the now-unused PROPOSAL_POLICE_ASSISTANT_ID input/secret, and exports `run` (guarded behind a JEST_WORKER_ID check on the auto-invocation) so it can be unit tested directly. Fixes #72725
Covers OpenAIUtils (promptResponses, createConversation, addConversationItems, parseJSONResponse), the ProposalPoliceConversation tracking helpers, and proposalPoliceComment's run() end-to-end (NO_ACTION, ACTION_REQUIRED, ACTION_EDIT, duplicate withdrawal, bot-author skip, and the Conversation create/reuse flow). No tests previously existed for either of these.
Regenerates every action's ncc bundle via npm run gh-actions-build. Most of these only pick up the new GithubUtils.pinIssue method (a shared lib bundled into every action); proposalPoliceComment's bundle reflects its full migration off the Assistants API.
- Require the model's action to be ACTION_HIDE_DUPLICATE, not just a high similarity score, before withdrawing a proposal as a duplicate (guards against the two fields disagreeing). - Skip the duplicate-check Responses call entirely when an issue has no prior proposals to compare against, instead of always spending an API call on a comparison that can't find anything. - Add a concurrency group (scoped by issue number) to the workflow so two comments posted close together on the same issue can't each create their own tracking Conversation. - Add tests for both behavior changes.
Round-2 branch-reviewer catch: the duplicate-check Responses call was the only mechanism appending items to a Conversation (via its `conversation` param's auto-append behavior), so skipping that call for an issue's first proposal (introduced in the previous commit) also skipped ever recording it - permanently hiding it from every future duplicate check on that issue. Now the proposal is recorded directly via addConversationItems when the call is skipped. Adds a regression test that runs the action twice in sequence (first proposal, then a near-duplicate second one) to prove the first proposal is actually comparable.
- Post the tracking comment (and pin) immediately after creating a Conversation, before sending any remaining seed-item batches, so a failure mid-seeding can't leave the Conversation permanently untracked and fragment duplicate-detection history across issues with a large pre-existing proposal backlog. - Validate the model-reported duplicateCommentId actually matches a real proposal comment before using it to build the withdrawal notice's link. - Escape angle brackets in untrusted comment/proposal text before interpolating it into our XML-style wrapper tags, so a comment containing a literal closing tag can't be mistaken by the model for the end of our own wrapper. - Switch the auto-invocation guard from checking JEST_WORKER_ID to the require.main === module pattern already used by every other action in .github/actions/javascript/*. - Add tests: multi-batch seeding (>20 prior proposals) with an assertion on tracking-comment-before-remaining-seed ordering, and escaping/tagging coverage for all four prompt input builders.
- Exclude the new proposal's own comment ID from the duplicate-check originalProposal lookup, guarding against a model self-match hallucination linking the withdrawal notice to itself. - Add tests for GithubUtils.pinIssue (correct GraphQL call, and that errors are swallowed rather than thrown), the one piece of new logic from this migration that didn't yet have coverage.
Round-5 branch-reviewer note: no test asserted the actual model value passed to promptResponses, so a future accidental edit to PROPOSAL_POLICE_MODEL wouldn't be caught by the suite. Exports the constant and asserts it's used for all three call types (duplicate, template, and edit check). (The model ID itself, gpt-5.6-luna, was independently confirmed to be a real, GA OpenAI model as of 2026-07-09 before this migration began.)
Pinning is a scarce, repo-wide resource (max 3 pinned issues for the entire repo), used by maintainers for things like contributing guides or roadmap items. ProposalPolice would have attempted to consume one of those slots on the first proposal of every "Help Wanted" issue, which could fire constantly on an active repo and conflict with real pins - for zero functional benefit, since the hidden marker text in the tracking comment (not pin status) is the actual mechanism used to find a tracked Conversation. Reverts the corresponding +1 eslint-seatbelt allowance bump for GithubUtilsTest.ts back to its original value now that the pinIssue test (and its one unsafe-type-assertion) is gone too.
Codecov Report✅ All modified and coverable lines are covered by tests. |
…ctions one getIsBotAuthor duplicated GitHub-specific bot detection that already exists (and is better) in the GitHub-Actions repo, so port it in as isBotUser instead of reimplementing it under ProposalUtils.
getIsProposal, getDuplicateCheckWithdrawMessage, and getDuplicateCheckNoticeMessage aren't property getters, so name them like the boolean/builder functions they are (isProposal, buildDuplicateCheckWithdrawMessage, buildDuplicateCheckNoticeMessage), consistent with the other build* helpers in this flow.
It took no arguments and always returned the same string, so a function call was unnecessary indirection.
Testing hasPriorProposals instead of conversationID left conversationID as string | undefined at the addConversationItems call, failing typecheck. The two conditions are equivalent here, since hasPriorProposals is assigned !!conversationID on the line above.
The prompts stated the same rules several times over: - decisionTree restated NEW_COMMENT_ACTIONS step for step, and its "is it actually a proposal?" step was already covered by the identification examples, so delete it outright. - duplicateDetection stated its one scoring rule four times, across Instructions / IMPORTANT / EXAMPLES / Summary. State it once. - templateCheckExamples repeated an identical example verbatim across its validation and identification sections. Merge them. Also define the ROOT CAUSE / SOLUTION / ALTERNATIVES shorthand once in templateDefinition, rather than respelling the full section headings in every file that refers to them.
The model was made to echo fixed message text back to us verbatim,
including a long template URL and {user} / {updated_timestamp}
placeholders that JS then substituted. Any drift in reproducing that
text silently changed what contributors saw.
Drop `message` from both schemas: the model now returns only an action,
and messages.ts owns every byte we post. The "already flagged" guard
also stops matching a hardcoded string and uses the exported
SUBSTANTIVE_EDIT_MESSAGE_PREFIX, so the message and the guard that
detects it can no longer drift apart.
The cutoff lived in two places that disagreed. The prompt told the model to set ACTION_HIDE_DUPLICATE only at similarity >= 90, and JS gated on that action *and* its own >= 85 check, so a proposal scoring 87 came back as NO_ACTION and was never withdrawn. Lowering the threshold to 85 in c24faa1 therefore had no effect at all. Drop `action` from the duplicate-check schema. The model reports only a similarity score, and DUPLICATE_SIMILARITY_THRESHOLD decides, so the cutoff is tunable in one place without the model having to reproduce it. ACTION_HIDE_DUPLICATE has no remaining callers, so remove it too.
PR #98402 moved every test whose import graph reaches @actions/* or @octokit/* out of Jest, so that those dependencies can be upgraded to their pure-ESM versions. These suites import the action, so they reach both and belong there too — left in Jest they would break on that upgrade. Three things did not survive the move as written: - The suite relied on Jest's globally-faked timers to skip retryWithBackoff's delay, and bun:test has no global fake timers and no async timer advance to install. It now waits out the real second, which exercises the actual backoff. - jest.mock('@scripts/utils/OpenAIUtils') auto-mocked the class. Its replacement is a real class rather than a mock constructor, because several tests call clearAllMocks partway through and that would strip a mock constructor's implementation. - resetAllMocks does not clear these module-level mocks' queued mockResolvedValueOnce values, so one test's leftovers were being consumed by the next. They are reset by name instead.
It has no @actions import of its own, but leaving one ProposalPolice suite behind in Jest splits them across two runners for no reason.
|
npm has a |
|
Measured it and added the figures to the description. Over the 7 days to 2026-08-15, across the 61 open |
|
All ten are done in One correction on the framing: these weren't lost. None of them were posted in round 1 — I checked every comment on the PR, and each of these appears for the first time in the comment above. Not a complaint, just so you're not left thinking a reply went missing.
|
On N1There's no N1 on the PR. The findings go N2, N3, N4, N5 — it's referenced only in your approval comment, so I think it's sitting in an unsubmitted pending review that only you can see. Going from your one-line description ("the team-slug 404 ambiguity in the workflow, which silently makes every employee spam-eligible if the slug or token scope ever changes"): the ambiguity is real, and I checked rather than assumed — So the three-way check separates errors from 404s, but can't separate those two 404s. I'm not fixing it. It takes a deliberate config change to trigger — a team rename or the token losing Worth naming: "fail loudly" isn't available here as an alternative. A broken slug doesn't produce an error to be loud about, it produces a successful 404 meaning "not a member". Detecting it is the extra call. If you think that's the wrong call, submit N1 and I'll take another look — you may have a failure mode in mind that I'm not seeing from the summary line. |
The merge brought a new lockfile that I hadn't installed, so the bundles were built against 2.0.197 and were missing three validate-code actions that CI's clean install has.
bun:test's jest.fn() has no type without one, so every .mock.calls read and every argument these were handed came through as any. Jest's jest.mocked() had been supplying that for free.
| // Bypass the real JSON-schema validators here; OpenAIUtils.test.ts already covers parseJSONResponse itself. | ||
| mockParseJSONResponse.mockImplementation((text) => JSON.parse(text)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 The verdict-pruning loop has no integration test
ProposalPoliceConversation.test.ts covers findVerdictItemIDs in isolation, and OpenAIUtils.test.ts covers deleteConversationItem.
But every action test's mock returns user messages only:
function conversationMessage(itemID: string, commentID: number): ConversationItem {
return {id: itemID, type: 'message', role: 'user', status: 'completed', content: [...]};
}So findVerdictItemIDs returns [] in all 27 tests and the loop body in run() never executes. The pieces are tested, the wiring is not. Cheapest fix is inside the test that already exercises the delete path:
mockListConversationItems.mockResolvedValue([
conversationMessage('item_42', 42),
conversationMessage('item_99', 99),
{id: 'item_verdict', type: 'message', role: 'assistant', status: 'completed', content: [{type: 'output_text', text: '{"similarity":95,"duplicateCommentID":42}'}]},
]);then assert both item_verdict and item_99 are deleted. The existing toHaveBeenCalledTimes(1) becomes 2, which is the assertion that would have caught a prune loop that silently does nothing.
🟢 Worth one check during live testing
If reasoning items do show up, widening the filter is a one-line change. |
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
Two new notes above ☝️ , neither blocking
All 14 items from rounds 1 and 2 are resolved, N1 was my error (never posted, your counter-argument on the merits is sound). The only real blocker left is an ESLint failure inherited from the main merge, in src/ files this PR never touches.
…sponsesConversations
Five violations in four src/ files are unbaselined on main and block any PR that busts the ESLint cache. This one does, because it adds an evals project to config/eslint/eslint.config.mjs and the cache key hashes that directory, so it re-lints the whole repo instead of only changed files. The files are byte-identical to main. Three of the casts became redundant in f2d5991, which gave getCardFeedWithDomainID overloads that already return the asserted type, and the fourth is a react-navigation deprecation. Seatbelt already covered these files for no-unsafe-type-assertion, so recording the new rule also tightens the old counts against the same casts rather than adding debt. Auto-tighten will drop the entries once the owners fix them properly.
…sponsesConversations
These recorded five violations that #98943 has since fixed at the source, so the entries describe casts and a deprecated call that no longer exist.
|
🚧 chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.4.57-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.57-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
ProposalPolice currently runs on the OpenAI Assistants API: the system prompt is configured manually in the OpenAI dashboard (not checked into code), and duplicate-proposal detection makes one Assistants thread/run per prior proposal on the issue (each with up to 90s of polling).
Assistants are being deprecated in favor of the Responses API. They will be shut off on 2026-08-26, so we're up against a real deadline soon here.
This migrates ProposalPolice to:
OpenAIUtils.promptResponses) on gpt-5.6-luna (replacing the Assistant's GPT-4o) for the comment-intent and edit-check calls — no more thread/run polling.prompts/proposalPolice/*) split into small, focused instruction fragments per call type (comment-intent / edit-check / duplicate-check), replacing the single dashboard-configured prompt blob — each call only gets the instructions it actually needs.Behaviour changes beyond the migration
The system prompt configured in the OpenAI dashboard is stale against
contributingGuides/PROPOSAL_TEMPLATE.md: it still requires two sections that were removed ined87b140dc6and0ced4a8d38c. Onmainthat means the bot nags contributors whose proposals correctly follow the current template. The checked-intemplateDefinition.tsmatches the real template, which fixes that.The created-comment path is reworked. The case-sensitive
Proposalkeyword gate is gone, because it meant an entire class of comment never reached any check at all — a bid for the job that never uses the word, like this one, returned at that gate before anything ran. Instead:author_associationisn't sufficient (it only reportsMEMBERfor publicly visible org members), so this readsExpensify/expensify-expensifyteam membership, which needs aread:orgtoken. A check that errors skips the run rather than treating the author as an outside contributor.isProposal). It's a definition, not a judgment, and an LLM checking it can drift from the template — which is exactly themainbug above.SPAM(a claim on the job with no technical content of its own),GENUINE_ATTEMPT(real technical content, wrong format), orNOT_AN_ATTEMPT(feedback, retests, questions, discussion).SPAMis collapsed via GraphQLminimizeComment, which leaves the text intact and is reversible — deliberately not the duplicate path's body overwrite.GENUINE_ATTEMPTstays visible. Both are pointed at the template.The model reports intent and the code decides the action, so the enforcement policy stays in one reviewable, testable place.
Call volume. Measured over the 7 days to 2026-08-15 across the 61 open
Help Wantedissues: 303 non-bot comments, of which 98 already follow the template and so cost nothing. That leaves ~29 intent calls/day (~880/month), an upper bound since Expensify employees are filtered out before the action runs and aren't excluded from that count. Duplicate-check volume is unchanged at ~14/day. The instruction prefix is cached viapromptCacheKey, so the marginal cost of each call is the comment body plus a one-word response.Two edit-check gates are new rather than ported, both fixing live
mainbugs: edits that didn't change the body are skipped (onmainthese reached the model asPrevious comment content: undefined, which reads as a full rewrite and could produce a spurious banner), and edits are skipped unless either side of the edit is a proposal (so a heavily reworded discussion comment that merely mentions "Proposal" isn't bannered).Duplicate detection never matches a proposal against its own author's. The old prompt asked for this but neither implementation honoured it, and the Conversation makes it more likely to bite:
maincompared pairwise and stopped at the first hit, whereas the model now sees every prior proposal at once and reports a single highest score, making a contributor's own earlier draft a real candidate.No tests previously existed for ProposalPolice or
OpenAIUtils; this PR adds coverage for all of the new logic (OpenAIUtilsTest,ProposalPoliceConversationTest,ProposalPoliceInputTest,ProposalPoliceSchemaTest,ProposalUtilsTest,proposalPoliceCommentTest).Fixed Issues
$ #72725
Tests
Outside of the automated tests we added, this will be live-tested.
Offline tests
None — this is a GitHub Actions bot with no client-side/offline behavior.
QA Steps
None — see
[No QA]in the PR title. This is a backend-only GitHub Actions change with no user-facing app behavior.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
n/a