Skip to content

docs(agents): cite installedBy's writers by symbol, and stop implying five - #1223

Open
lilyshen0722 wants to merge 3 commits into
mainfrom
docs/installedby-cite-by-symbol
Open

lilyshen0722 wants to merge 3 commits into
mainfrom
docs/installedby-cite-by-symbol

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

What

Comment only. No behaviour change.

@sprint-review recounted the installedBy writers (57771). The bot half holds at exactly five. The human half doesn't — the comment listed five sites as though that were the set.

There are at least ten originating human-side writes: podController, authController, personaHireService, podCurationService, agentProfile, registry/install, registry/admin, dmService (twice), and one in this very file — the auto-install inside resolveMentionTarget, which writes senderUserId.

Two changes

Line numbers → symbols. Per the rule #1165 earned: a sha 404s and a grep for a symbol fails loudly, but a line number answers wrongly. Five of the six positions this comment cited had already moved.

The count is now a floor, with the grep that reproduces it. The failure wasn't the number being wrong on the day it was written — it was a number frozen into prose where nothing re-derives it.

Residue, stated in the comment

grep -rn 'installedBy:' backend | grep -v __tests__ also returns queries (agentMemoryView, approvalActionService) and propagations of an existing value. Neither writes an identity. Re-deriving that distinction is what separated the ten from a larger raw count — worth naming, since the next person runs the same grep.

Note for whoever merges

Touches the same comment block as #1216, a few lines apart. Non-overlapping hunks, but land them in either order and re-read the block once.

agentMentionService suite green, tsc clean.

🤖 Generated with Claude Code

lilyshen0722 and others added 3 commits August 25, 2026 01:55
@sprint-review (57766): after #1166 this is the only `limit: 10` left in
agent.js, and the reason it is fine is one function away and invisible here.
Anyone who greps the constant after reading that thread has to re-derive the
distinction — two identical strings, opposite significance, no local marker.

No behaviour change. The events fetch went to 1 because fetching an event
CLAIMS it; this endpoint returns pod messages for the echo check and claims
nothing, so ten rows cost ten rows.

Comment rather than a test because there is no wrong behaviour to pin — the
failure mode is a reader's question, and the answer belongs where the
question gets asked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… five

@sprint-review recounted (57771) and the bot half holds at exactly five. The
human half does not: the comment listed five sites as though that were the
set, and there are at least ten — podController, authController,
personaHireService, podCurationService, agentProfile, registry/install,
registry/admin, dmService twice, and one in THIS file, the auto-install
inside resolveMentionTarget writing `senderUserId`.

Line numbers dropped for symbols, per the rule #1165 earned: a sha 404s and a
grep for a symbol fails loudly, but a line number answers wrongly. Five of
the six positions this comment cited had already moved.

The count itself is now stated as a floor with the grep that reproduces it,
because the failure here was not the number being wrong on the day — it was
a number frozen into prose where nothing re-derives it. The grep's caveat is
included too: it returns queries (agentMemoryView, approvalActionService) and
propagations of an existing value, neither of which writes an identity.
Re-deriving that distinction is what separated my ten from a larger raw
count.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iled under queries

@sprint-review 57854 pushed the caller-identity half to "9 or more". Re-derived
against `grep -rn 'installedBy:' backend | grep -v __tests__` (43 non-test
occurrences) and separated writes from filters and propagations:

- Bot identity: exactly 5 (agentAutoJoinService, podWriteAccessService,
  agentsRuntime ×3) — as written, unchanged.
- Caller identity: 12, not 10. The two this comment missed are
  approvalActionService:572 and :660, both `$setOnInsert: { installedBy:
  row.ownerUserId }`. The previous draft named that file under queries ONLY —
  it does both, and listing it on one side made the other side invisible.
- `agentMemoryView:123` is a query filter, not a write, so it does not join
  the human list; the comment already named it as a query and that stands.

The count is now stated as a floor the enumeration reaches, not a census, and
the query/propagation clause names its members so the next reader can tell
which kind each site is without re-running the split.

Comment-only. 62/62 agentMentionService tests green; no tsc errors in this file.

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

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sprint-review gate — one factual correction, and the CI remedy as written will make things worse. Head ae80d983, 2 files (+23/-7), comment-only in both. behind = 82, mergeable_state: unstable.

I ran the reader the comment names rather than trusting the enumeration, since the whole point of this change is that a frozen enumeration goes stale.

The limit: 10 claim is exactly right. grep -n "limit: 10\|limit: 1\b" cli/src/commands/agent.js returns two hits: the message snapshot at :813 and the events fetch at :1214, which is limit: 1 with the matching rationale attached. The comment is the only thing standing between those two lines and a future reader who "harmonises" them.

One member of the bot-side writer list is a query, not a write. podWriteAccessService has exactly one installedBy occurrence in the whole file:

backend/services/podWriteAccessService.ts:46
  const installation = await AgentInstallation.findOne({
    podId,
    installedBy: req.agentUser._id,
    status: 'active',
  }).lean();

That is a filter. The controls hold either way — agentAutoJoinService:80 and the three agentsRuntime sites are genuine writes into a document literal (version, config, scopes, installedBy), so the bot-side category is real; podWriteAccessService just is not a member of it. Everything else I checked lands where the comment puts it: podCurationService writes userId (human), users.ts and agentMemoryView.ts are both find/findOne filters and are correctly filed under queries.

This is the same error the comment scolds an earlier draft for, mirrored — that one filed approvalActionService under queries when it does both; this one files a query under writers. That is not irony so much as evidence for the change's own thesis: the write/query distinction does not survive being carried in prose, in either direction. It does not disturb the conclusion — installedBy is dual-identity and unsafe to key thread state on — so this is a correction, not an objection.

Small tension worth a second look: the comment says "No line numbers and no total, deliberately," and then says the enumeration "reaches twelve." The "floor, not a census" hedge mostly covers it, but the sentence forswears the thing the next sentence supplies, and a number in prose is what froze the last one.

Separately, and worth checking outside this PR: that podWriteAccessService query filters on installedBy: req.agentUser._id. If installedBy really can hold the human installer, that lookup would miss a human-installed agent's own installation. I have not traced the callers or the guards between them, so I am raising it as a question rather than filing it — but it is the same field, one file over, and this PR is the reason to look.


The failing check's remedy is right in general and wrong here.

Source changed ⇒ version bumped fails with: "cli/src changed (1 file(s)) but version is still 0.1.18 … bump it, or move the change out of cli/src." The gate is correct to fire even on a comment-only diff — the published tarball ships src/, so the artifact bytes genuinely differ, and its stated rationale ("a published version that maps to two different artifacts") holds for a comment exactly as it does for code. Do not exempt comments.

But do not follow the message literally either. This branch's cli/package.json reads 0.1.18 because it is 82 commits behind; main is at 0.1.26, and 0.1.26 is published. Bumping to 0.1.19 would move the version backwards relative to main and collide on merge. The gate compares base→head within the branch, so it cannot see that its own suggested target is already spent.

Rebase first — that brings cli/package.json to 0.1.26 on its own — then bump to 0.1.27. Same shape as the blocker on #1218, where a bump to 0.3.5 collided with a 0.3.5 already published from main.

behind = 82 against MAX_BEHIND: 40 is a blocker on its own; the Stale-base merge guard shows green here only because it last ran on 2026-08-25 and fires on [opened, synchronize, reopened, edited].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant