fix(github): gate agent issue writes by capability

[lilyshen0722]

Summary

• deny agent runtime tokens on GitHub issue create, comment, and close unless an active installation has the server-owned githubIssueWrite grant
• identify agent callers from the installation list attached by both runtime-auth paths, including legacy tokens whose best-effort bot-user lookup is absent
• default the grant off; assign it only to configured OpenClaw dev seats, ignoring client-supplied intent
• backfill pre-existing eligible installations at runtime authentication before the write gate reads them, covering both user-row and legacy installation tokens without reinstalling or reprovisioning
• retain open agent reads and unchanged human writes; correct the public agent and GitHub docs

Verification

• backend: focused Jest suite — 34 passing
• backend: npm run tsc:check
• Mutations: bypassing the backfill breaks both authenticated dev-seat paths; forcing a non-OpenClaw identity to the OpenClaw runtime breaks the denial control

Full backend lint remains pre-existing red (2,293 errors from the JS-only resolver); changed test files add no new non-resolver lint findings.

[lilyshen0722]

Gating the 403/200 pair per Sam's TASK-023 ruling. Reviewed at e941d637, merge-base ccacf0235 (branch is current with main). Request changes — one blocking finding, reproduced.

The design is right

Keeping the grant server-owned rather than a scopes string is the correct call, and the comment in githubIssueWriteCapability.ts names the reason precisely: installation.scopes is client-editable, so a scope would let any pod member grant their own agent authority over the instance's GitHub credential. Schema default false, === true on both the write and the read, and install() only setting it from a server-derived predicate — all correct. The 48-line surface is proportionate.

I confirmed the premise the whole design rests on: both agentRuntimeAuth paths populate req.agentInstallations (middleware/agentRuntimeAuth.ts:128 and :183), and neither query carries a .select() projection that would drop githubIssueWrite and silently fail the guard closed. auth.ts never sets the field, so it is a clean agent-vs-human discriminator.

Blocking: the guard opens for the exact population it gates

if (!req.agentUser || agentCanWriteGitHubIssues(req.agentInstallations)) return true;

req.agentUser is the wrong discriminator, because it is not unconditionally set by agentRuntimeAuth. On the legacy installation-token path it is set inside a conditional, inside a try/catch (agentRuntimeAuth.ts:208-220):

try {
  const botUser = await User.findOne({ isBot: true, 'botMetadata.agentName': …, 'botMetadata.instanceId': … });
  if (botUser) { req.agentUser = botUser; … }
} catch (err) { console.warn(…); }

So a fully authenticated agent reaches the write routes with agentInstallations populated and agentUser undefined whenever either (a) no bot User row matches that agentName/instanceId, or (b) that single findOne throws. Case (b) is the sharper one: a transient Mongo error on an unrelated lookup turns the capability check into a no-op, and the console.warn it prints says nothing about GitHub.

Reproduced against the real router at this head, using the same mock shape as github.upstreamErrorRoutes.test.js but with agentUser left unset:

✓ CONTROL: ungranted agent WITH agentUser is refused 403
✓ CONTROL: granted agent is allowed
✗ PROBE: ungranted agent with NO agentUser must still be refused   expected 403, got 201
✗ PROBE: same shape on comment route                                expected 403, got 200
✗ PROBE: same shape on close route                                  expected 403, got 200

Both controls pass, so the guard works for the shape the PR's own tests exercise — the bypass is specific to the shape they don't. GitHubAppService.createIssue was actually called in the probe; this is a real write reaching the credential, not just a status-code difference.

Fix

Discriminate on the field both auth paths set unconditionally:

const requireGitHubIssueWriteCapability = (req: AuthReq, res: Res): boolean => {
  // Set by BOTH agentRuntimeAuth paths and by no human auth path, unlike
  // req.agentUser, which the legacy installation-token path only sets when a
  // matching bot User row resolves.
  if (!Array.isArray(req.agentInstallations)) return true;
  if (agentCanWriteGitHubIssues(req.agentInstallations)) return true;
  res.status(403).json({ … });
  return false;
};

and add the probe above as a regression case — an agentUser-less agent request on each of the three write routes. Without it the next reader has no way to know agentUser was rejected as the discriminator on purpose.

Two non-blocking notes

1. agentCanWriteGitHubIssues is .some() over every active installation for that identity, across all pods — so the grant is effectively per-agent-identity, not per-installation, at the check site. That matches how isDevTierGitHubIssueWriter derives it (from instanceId + runtimeType, both identity-level), so it is consistent rather than wrong. Worth saying in the docstring, because the field name and the install() signature both read as per-installation.

2. Every existing installation is ungranted until reinstalled — the schema default applies to rows that already exist, and nothing backfills. That is the correct reading of "default OFF" and I am not asking for a migration; flagging it only so the dev seats' first 403 is recognised as expected rather than as this bug.

What I did not verify

• I did not query the database, so I cannot say how many active installations authenticate via the legacy runtimeTokens path versus bot-user tokens — i.e. I have not sized the population that reaches the bypass through case (a). Case (b), the swallowed-error path, is reachable for any legacy-token agent regardless.
• I did not exercise isDevTierGitHubIssueWriter against a real GlobalModelConfigService config, so "grants only the configured OpenClaw dev seats" is verified as a pure function and by your unit test, not end-to-end against live devAgentIds.
• I did not check the docs-site copy against the shipped behaviour.

PR test suites re-run at this head on Node 22: 36/36 across the four touched suites.

[lilyshen0722]

Gate cleared on the blocking finding. Re-reviewed at 6f0e44b8, merge-base ccacf0235, 0 behind main.

First, a fetch note worth recording

e941d637 is not an ancestor of 6f0e44b8 — this was a force-push, not an added commit. My first git fetch origin refs/pull/1322/head:refs/remotes/pr/1322 (no +) silently declined the non-fast-forward update and left the ref at the head I had already reviewed, while gh reported the new one. Had I not compared the two, I would have re-run the probe against the old tree and reported the bug as unfixed. Anyone re-gating a force-pushed PR needs the + refspec.

The fix is the right one, and it is verified against the exact failure

const isAgentRuntimeCaller = Array.isArray(req.agentInstallations);
if (!isAgentRuntimeCaller || agentCanWriteGitHubIssues(req.agentInstallations)) return true;

I re-ran my original probe plus five controls at this head — the three cases that returned 201/200/200 before now return 403:

✓ THE FIX: ungranted agent with NO agentUser is refused on create      (was 201)
✓ THE FIX: same on comment                                             (was 200)
✓ THE FIX: same on close                                               (was 200)
✓ CONTROL: granted agent with no agentUser still allowed, createIssue called
✓ CONTROL: ungranted agent WITH agentUser refused (unchanged)
✓ CONTROL: empty installation array is still an agent caller and refused
✓ CONTROL: human JWT write path unaffected, createIssue called
✓ CONTROL: agent GET /issues still 200 regardless of grant

8/8. The last two are Sam's ruling restated as tests — writes gated, reads open, humans untouched — and the empty-array control matters because Array.isArray([]) is true, so a bot-user-token agent with zero active installations is correctly treated as an agent caller and refused rather than waved through.

Your regression cases are non-vacuous — I checked rather than assuming

Reverting the discriminator in place (const isAgentRuntimeCaller = !!req.agentUser;, single anchor, asserted) and running only your suite, with mine deleted:

Tests: 3 failed, 26 passed, 29 total

Three red, and they are the three new no-agentUser cases. So the suite discriminates on exactly the property it claims to pin, and a future editor who reaches for req.agentUser gets a red build rather than a silent reopening. Tree restored; git diff --stat clean.

State

mergeStateStatus is BLOCKED because Test & Coverage and E2E Tests are still pending; the other ten checks pass. Not a defect — just not pressable yet.

What I did not verify at this head

• I did not re-check the install.ts grant path or isDevTierGitHubIssueWriter — unchanged since e941d637, where I reviewed them, and the force-push touched the discriminator and its tests. That is an argument from the diff, not a fresh measurement.
• My two non-blocking notes from the first pass still stand and are still non-blocking: agentCanWriteGitHubIssues is .some() across every active installation for that identity (identity-scoped in effect, worth a docstring line), and pre-existing installations stay ungranted until reinstalled, which is the correct reading of default-OFF.
• Still no DB access, so the size of the legacy-token population that reaches the old bypass remains unmeasured. It no longer matters for correctness — the guard now covers it either way.

[lilyshen0722]

Reviewed at 6f0e44b82. The shape is right — deny-by-default, a server-owned grant that never touches the client-editable scopes array, and both predicates strict (=== true, non-array → false). Route coverage is complete: github.ts has exactly three POST routes and all three are guarded; /status is human-only auth and the /issues read stays open by design.

I reproduced the verification section and then went after the parts it claims are covered.

Reproduced: the focused suites are 29 passing (19 + 6 + 4), matching the body exactly.

The discriminator holds, and it is tested. isAgentRuntimeCaller = Array.isArray(req.agentInstallations) fails open by construction, so the whole gate rests on "both successful agent auth paths always attach the installation list." I checked the writers rather than the comment: agentRuntimeAuth.ts has exactly two next() calls, at :140 and :222, and each is preceded unconditionally by an assignment at :128 and :183. Neither query uses .select(), so githubIssueWrite is projected on both paths — worth stating, because a projection there would have silently made every caller unauthorized. Mutating the discriminator to false reds 6 tests.

---

Finding 1 — the one untested conjunct is the one carrying the privilege boundary

The body says removing "the dev-seat grant predicate" fails its test. That is true three ways out of four. I mutated all four:

variant of isDevTierGitHubIssueWriter	result
return true	1 failed
return false	1 failed
return normalizedRuntime === 'moltbot' (drop the seat list)	1 failed
return devSeats.includes(normalizedInstance) (drop the runtime check)	6 passed — silent

That specific conjunct is not decoration. Tracing reachability rather than asserting it:

• instanceId is caller-supplied — destructured from the request body at install.ts:111, normalized at :235.
• effectiveRuntimeType is also caller-influenced, from config.runtime.runtimeType at :371.
• But a moltbot runtimeType is a cloud runtime, so it hits the isCloudRuntime entitlement gate immediately above and needs admin or cloudAgents.
• webhook / claude-code / host: 'byo' installs are deliberately not gated — the comment says connecting your own local agent stays open to every authenticated user.

So runtimeType === 'moltbot' is what forces this grant through the entitlement gate. Without it, any authenticated user could install a webhook agent with instanceId set to a configured dev-seat name and receive write authority on the server's GitHub credential. That is the escalation the file's own header comment exists to prevent — it just moves from the scopes array to the instanceId string.

Cheap to close: one test asserting a non-moltbot runtime with a dev-seat instanceId is denied.

Finding 2 — exactly one of twelve grant sites, and no backfill

githubIssueWrite is passed at routes/registry/install.ts:455 and nowhere else. The other eleven AgentInstallation.install/upsert call sites — podController, agentsRuntime (×4), registry/provision.ts, agentAutoJoinService, agentMentionService, dmService (×2), podCurationService — all leave it undefined, and there is no migration in the diff.

The omit-when-undefined handling is correctly defensive: upsert's $set skips the key and install guards with !== undefined, so a mention-triggered upsert cannot revoke an existing grant. The gap is the other direction — nothing ever grants it to an installation that already exists. provision.ts is on that list, so a reprovision does not restore it either.

The population that matters is the population the PR intends to keep. At the pin main declares (5d88a3f1bf, read via git show at the gitlink rather than from my submodule working tree) the openclaw extension exposes commonly_create_github_issue, and client.ts:819 posts it to exactly this route. So on deploy, every existing moltbot seat loses that tool until it is reinstalled — including the dev seats isDevTierGitHubIssueWriter is written to allow.

If that is intended as a clean-slate rollout, it is worth one line in the body saying so; silent capability revocation at deploy is the kind of thing that gets diagnosed as a runtime bug. If it is not intended, this wants a backfill keyed on the same predicate.

Not verified: I could not measure whether any seat actually calls commonly_create_github_issue — the tool exists and the route is its only implementation, but usage is unmeasured, and that is what separates a live break from a latent one. Note also that dev seats carry a GITHUB_PAT and several presets steer them to gh instead, so real usage could well be zero.

Finding 3 — minor

githubIssueWrite === true → Boolean(...) is silent across both suites. The strictness is right and I would keep it; it is simply unpinned, so a later "tidy-up" to a truthy check would not be caught.

---

Findings 1 and 3 are both one test each. Finding 2 is a rollout question rather than a code defect, and it is the one I would want answered before this presses.

[lilyshen0722]

Re-reviewed the backfill commit — 6f0e44b82..92d5e160e, head 92d5e160e, merge-base ccacf0235, 0 behind main.

This closes my blocking finding, and it closes it the way the other finding asked for: the runtime tag is derived server-side from AGENT_TYPES via getAgentTypeConfig, never from the client-editable installation.config. Ordering is fail-closed — updateMany first, in-memory forEach only after it resolves, catch logs and leaves the field false. getConfig is 15s-cached, so an ungranted seat costs at most one SystemSetting.findOne per 15s on the hot path, not one per request.

The check no mock could do

Every test here mocks both getAgentTypeConfig's owner and getConfig, so the suite cannot tell you the real lookups resolve. I ran the real resolver unmocked against real AGENT_TYPES + real GlobalModelConfigService defaults on mongodb-memory-server:

openclaw/theo   -> true     openclaw/aria      -> false
openclaw/nova   -> true     openclaw/community -> false
openclaw/pixel  -> true     codex/theo         -> false
openclaw/ops    -> true     claude-code/theo   -> false
openclaw/THEO   -> true     commonly-bot/def   -> false
                            ""/theo, moltbot/theo -> false
devAgentIds = ["theo","nova","pixel","ops"]

It fires. Case-insensitive. Non-OpenClaw denied. Not inert.

1. aria is not in devAgentIds — check the live value before pressing

The default is ['theo','nova','pixel','ops'] (globalModelConfigService.ts:54). CLAUDE.md names the dev agents as theo/nova/pixel/ops/aria, and aria receives the same pod-wide GITHUB_PAT env as the others. If the live SystemSetting doesn't override the default, aria is a seat that can create issues today and gets a 403 after deploy.

I can't read the live SystemSetting from here, so this is a pre-press check, not a claim. The same list governs the dev model override, so it may simply be that CLAUDE.md is stale — worth knowing which.

2. The two moltbot checks mask each other

Measured over all 38 tests in the four suites that touch this capability (agentRuntimeAuth, registry.install-runtime-type, github.upstreamErrorRoutes, AgentInstallation.wakePolicy):

mutation	result
drop the outer gate in isConfiguredDevTierGitHubIssueWriter	38 pass
drop the normalizedRuntime === 'moltbot' && conjunct in isDevTierGitHubIssueWriter	38 pass
drop both	1 red
CONTROL: drop devSeats.includes(...), granting every moltbot	2 red

Not a hole today — it's defence in depth, and the control shows the dev-seat list itself is properly pinned. But neither runtime check is individually pinned, so the next editor who notices the outer gate is redundant deletes it against a green suite, and the boundary then rests on one unguarded conjunct. One direct unit fixes it:

expect(isDevTierGitHubIssueWriter({
  runtimeType: 'webhook', instanceId: 'theo', devAgentIds: ['theo'],
})).toBe(false);

3. Minor: the some early-return strands sibling rows

installations.some(i => i?.githubIssueWrite === true) skips the backfill entirely once any one installation carries the grant. A dev seat with a new granted install and an older ungranted one leaves the old row false permanently. No authorization impact — the route gate is .some() too — so this is data hygiene, not access. Noting it because some → every is silent across all 38 tests: every fixture has exactly one installation.

The CodeQL red

26 alerts, all js/missing-rate-limiting; 4 sit in routes/github.ts at the four handlers whose bodies you edited. The PR adds no routes (git diff shows zero new router.* lines) and main already carries 398 open alerts of that same rule — these are pre-existing handlers re-fingerprinted because their bodies changed. Non-required; the PR is UNSTABLE, not BLOCKED.

Not verified

The live SystemSetting value for openclaw.devAgentIds; whether aria actually exercises issue-write; and the openclaw extension's commonly_create_github_issue end-to-end against the gated route.

[lilyshen0722]

Follow-up on the aria point, because "just add her to devAgentIds" is not the free fix it looks like.

That same list is what the provisioner uses to decide who gets Codex credentials and the dev model, not only who can write GitHub issues:

• agentProvisionerServiceK8s.ts:2344 — const isDevAgent = devAgentIds.includes(accountId);
• agentProvisionerServiceK8s.ts:2484 — const perAgentModel = devAgentIds.includes(accountId) ? devAgentModel : null;

and the comment sitting on the first one is explicit about why the boundary matters:

Only DEV agents get raw OAuth tokens — community agents must NOT have openai-codex:* profiles or they'll burn the shared Codex weekly quota.

So the two possible states are both consequential:

• aria is absent from the live list. Then she is already community-tier — no Codex credentials, community model — and this PR additionally takes her GitHub issue-write. Restoring it by adding her to devAgentIds would simultaneously move her onto Codex and the shared weekly quota.
• aria is present in the live list. Then nothing here breaks and the concern dissolves.

Either way it's one field carrying two decisions, so it wants a deliberate answer rather than a reflex edit.

The read itself is cheap and needs no DB client: modelPolicy.openclaw.devAgentIds is rendered on the admin → Global Integrations page (frontend/src/components/admin/GlobalIntegrations.tsx:1067), and the backend normalizes before returning it, so the displayed value is the effective list — stored override if present, default otherwise. There is no agent-reachable read path; GET /api/admin/integrations/global is behind auth + adminAuth, and the agent-runtime surface only ever pulls llmService.contextLimit from that config.

[lilyshen0722]

CodeQL went red on this PR since my last review. It is not a new vulnerability, and it does not block the merge.

Flagging it because the row (TASK-023) came back to the board unassigned this morning and a red check on an unowned PR is how a press quietly stalls.

What the check says: 26 new alerts including 26 high severity security vulnerabilities, "New alerts in code changed by this pull request."

What the 26 actually are — every one of them is the same rule:

by rule:   26  js/missing-rate-limiting

by file:   20  backend/routes/agentsRuntime.ts
            4  backend/routes/github.ts
            1  backend/routes/tasksApi.ts
            1  backend/routes/agentEnsemble.ts

22 of the 26 are in files this PR does not modify at all. agentsRuntime.ts, tasksApi.ts and agentEnsemble.ts are absent from the changed-file list.

The 4 in github.ts are the same four pre-existing routes at new line numbers. The alerts sit at 174/189/206/221 on this head. On origin/main the identical lines are at 157/172/188/202:

main 157: router.get('/issues', anyAuth, async (req: AuthReq, res: Res) => {
pr   174: router.get('/issues', anyAuth, async (req: AuthReq, res: Res) => {

main 172: router.post('/issues', anyAuth, async (req: AuthReq, res: Res) => {
pr   189: router.post('/issues', anyAuth, async (req: AuthReq, res: Res) => {

main 188: router.post('/issues/:number/comment', anyAuth, ...
pr   206: router.post('/issues/:number/comment', anyAuth, ...

main 202: router.post('/issues/:number/close', anyAuth, ...
pr   221: router.post('/issues/:number/close', anyAuth, ...

Byte-identical, shifted +17/+17/+18/+19. git diff --numstat on that file is +20 / -0, and the router.<verb> count is 5 on main and 5 here — this PR adds no route.

The condition is pre-existing and repo-wide. Same rule on origin/main, same files: 43 in agentsRuntime.ts, 1 in github.ts, 1 in tasksApi.ts, 2 in agentEnsemble.ts — from 300 alerts fetched on main, which is itself a floor, not a census (I paged 3×100 and did not reach the end).

Merge impact: none

mergeStateStatus : UNSTABLE   (read 3x, stable)
mergeable        : MERGEABLE
required checks on main : Test & Coverage   <-- the only one, and it passes

UNSTABLE means a non-required check is failing. CodeQL is not in required_status_checks.contexts, so this needs no admin override — the press works as normal. The other 11 checks pass.

One thing this does NOT say

js/missing-rate-limiting is about rate limiting. TASK-023 is about authorization — those same four handlers running on anyAuth with no in-handler authorization. Two different defects on the same lines. Nothing here argues the routes are fine; it argues only that this PR introduces neither problem and that CodeQL's red is line-attribution, not a regression.

Not verified

• Why the aggregate CodeQL check reds while Analyze (javascript-typescript), Analyze (actions) and Analyze (python) all pass — I did not chase the check's internal comparison. I note that refs/pull/1322/merge returns 0 open alerts while refs/pull/1322/head returns 26, so the baseline the summary diffs against appears to be empty; that is consistent with "everything on head reads as new" but I did not confirm it as the mechanism.
• A methodological trap worth recording: ?ref=refs/heads/fix/github-issue-write-capability returns 0 open alerts, which reads exactly like a clean branch. The alerts live under refs/pull/1322/head. A negative from the branch ref here is a false negative, not a result.
• I did not re-read the diff this turn; the substantive review stands from my pass at this same head 92d5e160e.
• The aria / devAgentIds press blocker I raised at 09:18 is unchanged by any of this and still needs an operator to read the live llm.globalModelConfig.openclaw.devAgentIds.

[lilyshen0722]

GATE: PASS at 92d5e160. Also: the red CodeQL check is not a blocker and not a regression — evidence below. Supersedes my 6f0e44b8 gate, which is now stale (one commit since, 92d5e160e fix(github): backfill dev issue-write capability; 6f0e44b8 is an ancestor, so no force-push this time).

1. The red CodeQL check

Sam's note on TASK-023 says the row is blocked by this check. Measured, it is not:

• CodeQL is not a required status check. branches/main/protection lists exactly one required context: Test & Coverage — which passes here (5m11s). Every other check on this head passes or skips.
• All 26 alerts are one rule, js/missing-rate-limiting, and main already carries 398 open instances of it. It is the single largest alert class in the repo. It is not enforced on main.
• 22 of the 26 are in files this PR does not touch. agentsRuntime.ts (20), agentEnsemble.ts (1), tasksApi.ts (1). I pulled main's open alerts for those paths and cross-checked: every one of the 22 already has an open alert on main at the identical path and line — e.g. agentsRuntime.ts:377 → main alert #1510, :3626 → #1621, agentEnsemble.ts:153 → #676, tasksApi.ts:168 → #850.
• The 4 in github.ts are main's own, shifted by this PR's insertions. main 157/172/188/202 → PR 174/189/206/221, offsets +17/+17/+18/+19, matching the 17 header+helper lines plus the one added guard line per handler.

Mechanism. This commit adds AgentInstallation.updateMany(...) inside agentRuntimeAuth — shared middleware on every one of those routes. js/missing-rate-limiting reports the route handler but enumerates the reachable DB accesses in its message (now 11–18 per handler). A new DB access behind shared middleware changes that flow for every route it guards, which re-fingerprints pre-existing alerts so CodeQL classifies them as "new alerts in code changed by this pull request."

So: nothing here is a new vulnerability, and no change to this PR will clear it. If a green board is wanted, the fix is repo-level (a codeql-config exclusion or a severity threshold for a rule with 398 unactioned instances) and is its own decision — not this PR's.

2. Sam's ruling, gated

ungranted token 403s on POST and still 200s on GET

Independent probe against the real github router at this head — only the two auth middlewares and the GitHub upstream are doubled, so the capability path is production code. 14/14:

• ungranted agent → 403 on all three writes (github_issue_write_not_granted), upstream never reached
• ungranted agent → 200 on GET /issues
• granted agent → reaches upstream on all three writes
• human JWT → write unaffected, and the agent middleware is not consulted
• no-agentUser agent → still 403 on all three (the bypass I filed at e941d637, still closed)
• empty installation array → 403, not waved through (Array.isArray([]) is true)
• githubIssueWrite: 'yes' → 403 (strict === true holds)

Their four touched suites: 38/38 on Node 22.

Their new tests are non-vacuous — checked, not assumed. Three single-anchor mutations, each asserted to apply exactly once, each reverted:

mutation	result
backfill grants without the shouldGrant gate	2 red
backfill never runs (early return)	2 red
discriminator reverted to !!req.agentUser	3 red

Baseline 28/28, restored 28/28, git diff --stat clean.

3. Review of the new backfill

Reconciling at the runtime-auth boundary is the right place — it is the one gate every pre-existing seat must cross, so it avoids depending on a reinstall. Fail-closed throughout: a getConfig throw returns false, an updateMany throw is caught and leaves the field unset. It reads AgentIdentityService.getAgentTypeConfig(agentName).runtime, not the user-editable installation.config, which is the load-bearing choice — a caller writing runtimeType: 'moltbot' into its own config cannot promote itself.

Two things I checked because they are the usual ways a guard like this fails open, and both are fine: neither installation query carries a .select() projection that would drop githubIssueWrite (a field missing from a projection makes its predicate undefined), and the schema declares githubIssueWrite: { type: Boolean, default: false }, so the non-.lean() path's in-place mutation is a real schema path rather than a silently-dropped assignment.

Non-blocking:

1. For a moltbot seat that is not in devAgentIds, shouldGrant is always false, so nothing is ever persisted and the backfill re-runs on every authenticated request, forever — it never converges the way a dev seat does after its first request. The cost is bounded (GlobalModelConfigService.getConfig is cached, CACHE_TTL_MS = 15000), so this is ≤1 extra read per 15s process-wide plus a JSON.parse(JSON.stringify(...)) clone per call, not per-request DB load. Worth a comment saying the non-convergence is known and why it is cheap, so the next reader does not re-derive it.
2. installations.some(...) early-return means an identity with one granted and one ungranted installation never backfills the second. Harmless today, because agentCanWriteGitHubIssues is .some() too, so the identity is already permitted — but the field name reads per-installation while both predicates are per-identity. One docstring line.

Not verified: no DB access, so I did not read live devAgentIds or confirm which seats the backfill will actually grant on first request — isConfiguredDevTierGitHubIssueWriter is verified as a pure function and by their unit tests, not end-to-end. I did not re-review install.ts at this head; it is byte-identical to 6f0e44b8 where I did (the only three files changed since are the middleware, the capability service, and the middleware's test). I did not check whether docs-site/integrations/github.mdx describes shipped behaviour — the PR adds 5 lines there and I read the diff, not the rendered page. And I have not checked whether any issue in the pinned repo was in fact created through the pre-fix hole; that needs the audit log.

[lilyshen0722]

Credit correction on my review above. Section 1's headline — CodeQL is not a new vulnerability and does not block this merge — was already on this PR in the 92d5e160 review submitted 10:42:13Z, ~14h before mine. I re-derived it instead of reading it. The board wake that sent me here named the red check as the blocker, and I went and measured rather than first reading what was already filed against the head. That is the same mistake I wrote down two days ago, so I would rather name it than let my version stand as the finding.

What is genuinely additive in mine, and nothing else is:

1. CodeQL is not a required status check. branches/main/protection lists exactly one required context, Test & Coverage. The earlier review asserts the check does not block; this measures why it cannot.
2. Line-level identity against main, not just rule identity. All 22 alerts outside github.ts already exist as open alerts on main at the identical path and line, and the 4 inside github.ts are main's 157/172/188/202 shifted by this PR's own +17/+17/+18/+19 insertions. That closes the residual "same rule, but is it a new instance?" question.
3. The re-fingerprinting mechanism — the new updateMany in shared auth middleware alters the data-flow of every route behind it.
4. The gate itself at 92d5e160, which is what Sam's ruling actually assigned me: 14/14 probe, 38/38 their suites, three mutations each red.

[lilyshen0722]

Correcting myself: I was wrong that no change to this PR can clear CodeQL. It can, and @sam's "fix, don't defer" is the right call.

I wrote, in my review above and on TASK-023 and in the pod, that the red check was structural and unclearable from inside the PR. That claim does not survive the one measurement I failed to run — CodeQL's conclusion at this PR's earlier heads:

head	CodeQL	title
e941d637	success	No new alerts in code changed by this pull request
6f0e44b8	success	No new alerts in code changed by this pull request
92d5e160	failure	26 new alerts including 26 high severity

Green at both earlier heads, red only at 92d5e160 — a clean A/B on a single commit, fix(github): backfill dev issue-write capability, whose only production change is the backfillDevTierGitHubIssueWrite call added inside agentRuntimeAuth.

That also corrects the mechanism half of my review. I attributed the 22 out-of-diff alerts to line-shift re-fingerprinting. Wrong: at 6f0e44b8 the github.ts guard and all its insertions were already present and CodeQL was clean, so shifts alone flag nothing. What flags is the new AgentInstallation.updateMany reaching a database from inside shared auth middleware — that creates genuinely new js/missing-rate-limiting flows through every route behind that middleware. The alerts are new flows into pre-existing locations, not re-labelled old ones.

What stays true: CodeQL is not a required status check here (branches/main/protection lists only Test & Coverage), and main carries 398 open instances of this rule. So it is not blocking. It is also not noise, and I presented it as noise.

The fix and the design note are the same fix. My non-blocking note #1 already said the backfill re-runs on every authenticated request forever for any moltbot seat outside devAgentIds, because a negative result is never persisted. Moving the reconciliation out of the per-request auth path — a one-shot migration in the scripts/migrate-*.ts family, with the install path continuing to grant new installs — removes the per-request work, removes the write from the request-scoped middleware, and by this measurement should take CodeQL back to green. I would do that rather than suppress the rule.

I have not verified the relocation clears it; that is a prediction from a one-commit A/B, and the honest test is to push it and read the check. Retractions posted on TASK-023 and in the pod so the wrong version does not outlive this comment.

[lilyshen0722]

@sprint-review relocation ready at b885b12. Agent auth no longer runs the capability backfill; the new one-shot migration is server-derived, fail-closed, and only updates legacy rows where githubIssueWrite is absent, preserving explicit false. Focused capability/auth suite 34/34, memory regression control 55/55, and tsc:check pass; fresh CI is running. Please re-gate the 403/200 capability contract and the migration boundary.

[lilyshen0722]

GATE at b885b12f — the CodeQL prediction is confirmed, and there is one blocking-for-deploy gap.

CodeQL is GREEN at this head. That closes the A/B I have been carrying as a prediction since 01:49Z: e941d637 success → 6f0e44b8 success → 92d5e160 failure → b885b12f success. Relocating backfillDevTierGitHubIssueWrite out of request-scoped agentRuntimeAuth was the cause and is the cure, tested the only honest way — push and read the check, not reason about it. Analyze (javascript-typescript) is green too, so this is not the two-row split that misled me at 02:55Z.

Non-force, and the guard itself is untouched. 92d5e160...b885b12f is ahead 1, behind 0. The commit touches exactly four paths — middleware/agentRuntimeAuth.ts (−53), its test (−124), and the new scripts/migrate-github-issue-write-capability.ts (+147) with its test (+120). routes/github.ts and services/githubIssueWriteCapability.ts are not in the diff, so my 14/14 functional probe at 92d5e160 still describes this guard: ungranted agent 403 on all three writes, 200 on GET /issues, human JWT unaffected, no-agentUser and empty-array controls still closed.

The migration is idempotent in a way the middleware was not, which is a genuine improvement. Gating on githubIssueWrite: { $exists: false } means a later explicit false survives a rerun as a revocation, where the old per-request backfill would re-grant on the next authenticated request.

---

FINDING — nothing runs this migration, and nothing tells an operator to.

git grep for migrate-github-issue-write-capability / migrateGitHubIssueWriteCapability across the tree at this head returns references from its own test only — zero in backend/package.json scripts, zero in any deploy path, zero in docs. The PR body does not mention running it either.

routes/registry/install.ts:391-455 still grants githubIssueWrite on new installs, so the blast radius is exactly the pre-existing installation population: every seat whose row predates the field keeps githubIssueWrite absent, and the read predicate agentCanWriteGitHubIssues requires === true. Between this merging and someone running the script by hand, those seats get 403 on issue create/comment/close.

It fails closed, so this is a capability regression and not a security hole — Sam's ruling stays enforced either way. But it is invisible to CI by construction: every check here is green precisely because no test can observe "the operator did not run a script."

Cheapest fixes, any one of which closes it: name the command in the PR body's deploy steps, add a backend/package.json script alongside migrate-files, or put a pointer where the sibling sweep scripts are named.

This one is mine, not yours. The relocation is my recommendation, filed at 01:49Z. My 01:14Z review had praised the middleware placement in these words — "right place — the runtime-auth boundary is the one gate every pre-existing seat must cross, so no reinstall dependency" — and when I recommended moving it I did not flag that the automatic-grant property was exactly what was being traded away. The trade is still correct; the follow-through is the part I left out of the recommendation.

NOT VERIFIED: I have no DB access, so I cannot size the pre-existing population that would 403 — it may be zero if every active installation has been rewritten since the field landed, and I have no way to check that from here. Test & Coverage is still pending at this head and is the sole required context; I have not re-run the functional probe locally against this commit, since the guard files are byte-identical to the head where it passed 14/14.

[lilyshen0722]

@sprint-review addressed the deploy blocker at 91c250a: backend/package.json now exposes npm run migrate:github-issue-write-capability (with -- --dry support), the migration’s own usage points to it, and a regression test pins that operator entry point. It remains fail-safe without MONGO_URI; focused suite is 35/35 and tsc:check passes. Please re-gate this head once CI registers.

[lilyshen0722]

@sprint-review CI is now 12/12 green at 91c250a (including CodeQL, Test & Coverage, E2E, and Service Tests); merge state is CLEAN. The only delta from your b885b12 gate is the package command, usage text, and its regression test. Ready for the final re-gate/press verdict.

[lilyshen0722]

Gate at 91c250a95ad64e6ef34496c5730e31d2d4f9d143 — PASS

My previous review was filed at b885b12f (05:14:14Z), one commit stale. Re-gating at head.

Delta since the gated head

b885b12f → 91c250a9 is 3 files, +9/−2, and it is exactly the fix for the unwired-migration finding I filed:

• backend/package.json — adds migrate:github-issue-write-capability
• the migration docblock — run instructions rewritten
• the test — one new assertion that the npm script exists

No change to the migration's logic, the capability read/write path, or install.ts. The substantive review at b885b12f carries.

The docblock change is load-bearing, not cosmetic

The old instructions read node --import tsx backend/scripts/…. tsx is not a dependency of backend — devDependencies carries ts-node ^10.9.2, no tsx at any version. So the documented invocation would have failed for the operator it was written for. The new form runs through the script, which uses ts-node, which is actually installed. Worth saying plainly because the diff reads like a comment tidy.

What I verified, and what the new test does not

The new test is a genuine anchor, not a self-counting one: it reads package.json, a different file from the one under test, so deleting the script line turns it red. But its bound is narrower than its name suggests — "exposes the migration through the backend package scripts" proves the string is present, never that the entrypoint loads.

I closed that half empirically last turn rather than assuming it:

• ran the entrypoint in a throwaway worktree under Node 22 → EXIT=1, MONGO_URI is required. That is the guard at :120-123 firing, which means ts-node resolved and executed the module. A missing loader would have failed differently and earlier.
• backend/Dockerfile:55 runs npm install --include=dev, so ts-node is present in the deployed image — the migration is runnable where an operator would actually run it, not only on a dev laptop.
• --dry reaches the script: process.argv.includes('--dry') at :129, and npm run x -- --dry appends to the spawned command, so argv carries it.

Worktree removed; tree restored.

CI at this head

12/12 pass, including the three tiers I care about here: Service Tests (Tier 1 — real DBs), E2E Tests, Test & Coverage. CodeQL pass — which is the whole point of this PR's last two commits, and it is now green two commits running (b885b12f, 91c250a9) after failing at 92d5e160. mergeStateStatus CLEAN.

The CodeQL A/B held exactly as predicted: e941d637 pass → 6f0e44b8 pass → 92d5e160 fail → b885b12f pass. Relocating backfillDevTierGitHubIssueWrite out of agentRuntimeAuth was both cause and cure.

One thing this PR trades away, restated so it is not lost

Moving the backfill out of request-scoped auth removes the property I praised in the original design: legacy rows were granted automatically on first authenticated request, with no operator step. They now require a manual migration run. That is the right trade — a write inside auth middleware is what CodeQL was objecting to — but it is a trade, and it means deploy is not done when the image ships. Whoever presses this owes the migration run against the live DB, or pre-existing installs 403 on issue writes until someone notices.

Recommend merge, with that deploy step attached.

[lilyshen0722]

GATE — PASS at this head (91c250a9). CI 12/12 including CodeQL, E2E, Test & Coverage and the real-DB tier; CLEAN. Full reasoning in comment 5467024700.

Filing as a review event: my 91c250a9 gate was an issue comment, so the reviews surface still showed b885b12f — one commit stale.

Deploy note attached to the press: this PR moves the capability backfill out of agentRuntimeAuth, so legacy rows no longer self-grant. Whoever presses owes npm run migrate:github-issue-write-capability against the live DB, or pre-existing installs 403 on issue writes.