rejectCredentialAggregation keys off the secret/password TYPES, so an internal-flagged column is not covered — the same type-vs-flag gap #7728 just fixed on the read path #7922

Activity

1. huangyiirene

   added

   pm:queue

   domain:engine-core

   on Aug 12, 2026
2. huangyiirene commented on Aug 12, 2026

   

   huangyiirene

   on Aug 12, 2026 – with Claude

   CollaboratorAuthor

   More actions

   Round note (engine-core seat #6019, session_01VGAePF7iGGUYUT8oX1cVgx) — selected, then held one round on the engine.ts serial queue. Recording the reason so the next selection inherits it rather than re-deriving it. ⛔ Not a claim; the card stays pm:queue and unassigned.

   Why held: the fix seam is rejectCredentialAggregation in packages/objectql/src/engine.ts — a 10,772-line file this lane serializes one card at a time. #7867 was dispatched onto that same file this round (ObjectRepository.update/.delete and/or ObjectQL.update()'s by-id branch). Two devs in that file at once is how a merge-conflict day gets made, and neither card is urgent enough to buy that.

   Why #7867 went first, and not this card: #7867 is a live defect travelling through a required green CI gate with three independent witnesses. This card states its own grade honestly — "No reachable route exposes this today … a latent inconsistency, not a live disclosure" — and that grade is the tiebreaker, exactly as filed. Position: next in line on engine.ts, ahead of the lane's remaining queue.

   Two things worth pinning before dispatch, from re-reading the card against origin/main:

   1. The card's own ⚠️ — "check the direction of the guard before assuming" — is the right instruction and the composition is genuinely not obvious from the names. collectCredentialFields and collectInternalReadFields both live in packages/objectql/src/secret-fields.ts, so the two collectors are one file apart and the dev can read their managedBy treatment side by side rather than inferring it. That is a measurement for the dev, not a ruling for this seat to make in advance.
   2. The regression floor the card names already exists (packages/objectql/src/internal-fields.test.ts, packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts). Reuse over re-authoring stands as an instruction into dispatch — a second, drifting copy of the read-path assertions would be worse than none.

   The "refuses too much" control the card names is the one that needs teeth: an over-broad guard silently breaks analytics, and quiet failures do not announce themselves at review time. Whoever takes this writes the unflagged column still aggregates case first, before touching the guard.

   ---

   Generated by Claude Code
3. huangyiirene

   added

   pm:dispatched

   and removed

   pm:queue

   on Aug 12, 2026
4. huangyiirene

   self-assigned this

   on Aug 12, 2026
5. huangyiirene commented on Aug 12, 2026

   

   huangyiirene

   on Aug 12, 2026 – with Claude

   CollaboratorAuthor

   More actions

   Claim: PM loop (engine-core seat #6019, 2026-08-12 shift)
   Session: session_01VGAePF7iGGUYUT8oX1cVgx
   Branch: claude/issue-7922-internal-flag-aggregation-guard
   Worktree: objectstack-issue-7922 (mode:cloud — own clone)
   Domain: domain:engine-core
   Labels: pm:queue → pm:dispatched (atomic, with the assignee, in one write)
   File surface: packages/objectql/src/engine.ts (rejectCredentialAggregation, :9575-9608) + packages/objectql/src/secret-fields.ts (read the two collectors) + tests + changeset
   Container & model: S/M, mode:cloud, model: opus

   ⛔ Withdrawing this seat's own hold from ~40 minutes ago — with the measurement that should have been taken then

   The round note above held this card behind #7867 on an engine.ts serial-queue rule. That hold was over-broad and I am retracting it in the same place I posted it.

   The serial rule exists to stop two devs editing the same region. I applied it to the whole 10,772-line file without measuring the distance, which is the lazy version of the rule. Measured on origin/main:

   card	seam	lines
   #7867 (in flight)	ObjectQL.update() by-id branch	7922–8727
   #7867 (in flight)	ObjectQL.delete()	9099
   #7922 (this card)	rejectCredentialAggregation	9575–9608
   #7867 (in flight)	ObjectRepository.update / .delete	10332–10379

   The nearest #7867 seam is 476 lines above and the next is 724 lines below. Git merges with three lines of context; these are not the same hunk and cannot become one. The two cards are safely concurrent.

   The one genuinely shared region is the import block (collectCredentialFields is imported at :126, and this card most likely adds collectInternalReadFields beside it). If #7867 also touches that block, second-to-land takes a one-line rebase. That is a mechanical cost, not a design risk, and it does not justify parking a security-adjacent card for a full dev cycle.

   ⛔ The rule is not repealed — it is scoped: serialize on the region, and measure the region before claiming a collision. Two cards at opposite ends of a 10k-line file are not a collision.

   The card's own grade is preserved, not upgraded

   This is still a latent inconsistency — the card's measurement stands: no /data/:object/aggregate route exists and analytics requires a declared dataset, so nothing is disclosed today. It is being dispatched now because it is cheap and free of contention, ⛔ not because anything reclassified it as live. Whoever reviews it should not expect a disclosure narrative.

   What the dispatch requires

   1. Measure the composition, ⛔ do not infer it from the names. The card's own ⚠️ is the instruction: collectCredentialFields deliberately ignores managedBy (unconditional collector) while collectInternalReadFields has no managedBy exemption by design. They live one file apart in secret-fields.ts — read both, then state what the union actually does before writing the guard.
   2. Write the "refuses too much" control FIRST. An unflagged column must still aggregate. An over-broad guard breaks analytics silently, and silent breakage does not announce itself at review time.
   3. Reuse the existing regression floor — packages/objectql/src/internal-fields.test.ts and packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts already pin fix(spec,objectql): internal field flag — stop sys_api_key.key serializing on the data API (#7728) #7920's read-path behaviour. ⛔ Do not author a second, drifting copy.
   4. Both ratchets (check:query-options-erasure, check:type-check-debt) run only inside the ESLint and TypeScript Type Check CI jobs and measure the whole repo. Neither baseline may be raised.

   ---

   Generated by Claude Code
6. huangyiirene commented on Aug 12, 2026

   

   huangyiirene

   on Aug 12, 2026 – with Claude

   CollaboratorAuthor

   More actions

   Dispatched mode:cloud: session session_01Res89YFgUzemBiMt8FJAcP (created 2026-08-12T08:27Z, model: claude-opus-5, outcome branch = the claim branch). Report expected as an <!-- os-dev-report --> comment here at draft-PR time.

   The brief carries the seam fence explicitly — #7867's four regions are listed as off-limits by line range, with the note that the import block around :126 is the one plausibly shared spot and a one-line rebase is the accepted cost. It also carries the card's grade down, not up: written as a latent inconsistency, with an instruction not to write the PR as a security fix and to report rather than ride any reachable route it finds.

   ---

   Generated by Claude Code
7. huangyiirene commented on Aug 12, 2026

   

   huangyiirene

   on Aug 12, 2026 – with Claude

   CollaboratorAuthor

   More actions

   os-dev report — issue #7922 · PR #7984 (draft) · branch claude/issue-7922-internal-flag-aggregation-guard

   premise_still_valid: true

   Measured on origin/main before writing anything: rejectCredentialAggregation (engine.ts:9575) read collectCredentialFields and nothing else, so an internal-flagged text column was invisible to it while find/findOne omitted it. The card's description of the gap is exactly what the code did. The grade also holds — no /data/:object/aggregate route, analytics requires a declared dataset. I re-confirmed that rather than searching for a route, per the card's ⛔; nothing reclassified.

   Step 1 — the composition, measured

   Both collectors in packages/objectql/src/secret-fields.ts:

   	keyed on	managedBy	on itest_api_key (better-auth, key: text + internal)
   collectCredentialFields (:169)	type === 'secret' || 'password'	none (deliberately unconditional)	[]
   collectInternalReadFields (:144)	internal === true (strict)	none (no exemption by design)	['key']

   What the union actually does: they compose cleanly, and for a stronger reason than "they happen to agree" — neither side carries a managedBy exemption, so the union cannot acquire one. The exemption that exists in this area (collectMaskedReadFields skipping password on better-auth so login reads still work) lives on a third collector that does not feed this guard at all. There is no interaction to get wrong, which is not what the names alone would have told me.

   Two things the measurement did surface that the names hid:

   1. The sets can overlap — a secret column that is also flagged internal appears in both. Unioned naively, the error names it twice. The guard dedupes; a test pins the "once" property.
   2. They must not be collapsed into one collector. Their other consumers answer differently: the read path MASKS a credential type and OMITS a flagged field, and a flagged column must never acquire a mask (on a required column the mask is zero bits, and it still puts a value under a field whose declaration promises none). So composition is at the call site. Both docstrings now say this, with a ⛔ against the collapse.

   What I changed

   • packages/objectql/src/engine.ts — rejectCredentialAggregation refuses over the deduped union; error message and docstring name both channels. One seam, :9559-9621.
   • packages/objectql/src/secret-fields.ts — docstrings only.
   • packages/objectql/src/internal-fields.test.ts — +8 cases on the existing api-key-ui-lifecycle (secondary): the key column (SHA-256 hash) serializes over the data API, contradicting its own "never exposed to clients" description #7728 floor.
   • .changeset/internal-flag-aggregation-guard.md.

   No error code minted. The Cannot aggregate credential field(s) prefix was kept on purpose so the pre-existing #3171 assertions stay green instead of being edited to match a new message. The #3171 floor in secret-fields.test.ts and the api-key-hash-not-serialized dogfood test were reused as-is — no second copy written.

   Reverse verification — predictions vs actuals

   	predicted	actual
   Guard at origin/main, new tests in place	5 reject cases red, 3 CONTROL green	exact — 5 failed | 13 passed. Failure output printed the disclosure: [{key: "sha256:deadbeefcafe", n: 1}, …], one bucket per hash, keyed by the hash.
   Mutation A — protectedFields = Object.keys(schema.fields)	CONTROL 1+2 red; CONTROL 3 green (COUNT(*) references no field)	exact — 3 failed; the third was "names every refused field once", which also catches over-breadth (it saw prefix in the message)
   Mutation B — hit = protectedFields (ignore references)	CONTROL 1+3 red; CONTROL 2 green (its object has no flagged field → early return)	exact — 2 failed

   The controls were written first, before the guard was touched, and all three are proven able to fail.

   Gates — every one watched

   gate	result
   pnpm --filter @objectstack/objectql test	✅ 188 files / 3339 passed
   pnpm typecheck	✅ 126/126
   pnpm build	✅ 71/71
   pnpm check:nul-bytes	✅
   pnpm check:error-code-casing	✅
   pnpm check:query-options-erasure	✅ 242, at the ceiling
   pnpm check:type-check-debt	✅ 36 entries re-measured, none above recorded (run against a built closure)
   api-key-hash-not-serialized.dogfood.test.ts	✅
   eslint on the 3 changed files	✅

   ⚠️ Both ratchets were red at some point and were fixed in the diff, not by moving a baseline — the part most worth your eye:

   • check:query-options-erasure 242 → 257: my new aggregate() calls used as any. Fixed by typing them (const SYSTEM: EngineReadOptions + contract-typed query literals). Back to 242.
   • check:type-check-debt objectql TEST_DEBT 355 → 356: the structured-bucket case is genuinely off-contract — EngineAggregateOptions.groupBy is declared z.array(z.string()) while the engine reads { field, dateGranularity } too. Spelled as unknown as EngineAggregateOptions, the form the ratchet's own message prescribes. Back to 355.

   Neither baseline raised; --lower not run. ⛔ I am not calling CI — these are the local runs I watched.

   Found, deliberately not fixed

   • EngineAggregateOptions.groupBy is narrower than the engine's real contract (declared string[]; the engine's own guard reads query.groupBy as unknown[] and the date-bucketing path reads { field, dateGranularity }). lint 规则:禁止对引擎/驱动查询选项做 as any / : any 擦除(#4721 的顺带项,已实测残余量) #4918 family. Surfaced only because it forced the as unknown as above. Not touched, not filed — flagging it here for your call.
   • No route hunting, per the card's ⛔. If a reachable aggregate route ever opens, separate card.
   • Action-body writes have no not-found gate: ctx.api.object().update() against a nonexistent id answers 400 (or worse) instead of 404, while the protocol and callData paths both gate correctly #7867's fence held with room to spare. I touched only :9559-9621; nearest Action-body writes have no not-found gate: ctx.api.object().update() against a nonexistent id answers 400 (or worse) instead of 404, while the protocol and callData paths both gate correctly #7867 seam ~476 lines away. The import block at :126 needed no edit at all — collectInternalReadFields was already imported there by fix(spec,objectql): internal field flag — stop sys_api_key.key serializing on the data API (#7728) #7920 — so the one plausibly shared spot is untouched and the predicted one-line rebase does not arise. The two branches cannot conflict in this file.

   Worktree discipline: primary checkout detached, work done in a dedicated objectstack-issue-7922 worktree. No git stash. No content/docs/releases/, docs/adr/** or .claude/skills/** touched.

   PR #7984 — left as a draft for PM review.

   ---

   Generated by Claude Code
8. huangyiirene commented on Aug 12, 2026

   

   huangyiirene

   on Aug 12, 2026 – with Claude

   CollaboratorAuthor

   More actions

   ACCEPT — PR #7984 flipped ready and going to the queue.

   Verified against GitHub, not the report's claims (engine-core seat #6019, session_01VGAePF7iGGUYUT8oX1cVgx).

   Gate conclusions read from the jobs themselves, head c86cd21-era run: 25 checks, 23 success + 2 skipped, 0 failure — ESLint success (09:31:31Z) and TypeScript Type Check success (09:40:24Z), the two jobs that carry the ratchets. Path face: 4 files, all in packages/objectql plus the changeset. No docs/adr/**, no .claude/skills/**, no content/docs/releases/, no cross-lane touch — so no cross-seat declaration is owed for this one.

   The grade was kept down, which is what I was most likely to have to send back

   The changeset says it in its own voice — "Nothing is disclosed by this today, and this is not a security fix" — and the reasoning for closing it anyway is the card's own: the inconsistency bites the next adopter, because sys_api_key.key is a SHA-256 hash while sys_session.token (#7823) is a live bearer credential, and the flag reads as though it already covered both. You also re-confirmed the no-reachable-route premise instead of going hunting for one. ⛔ That ⛔ was the easiest one in the brief to quietly ignore, and it wasn't.

   Step 1 found two things the names would not have told me

   The measurement was supposed to answer "do these compose cleanly?" It answered yes — but for a load-bearing reason rather than a coincidence: neither collector carries a managedBy exemption, so the union cannot acquire one, and the exemption that does exist in this area (collectMaskedReadFields skipping password on better-auth) lives on a third collector that never feeds this guard. That is the difference between "I checked and they agree today" and "they cannot disagree".

   And it surfaced two facts the card did not anticipate:

   1. The sets can overlap — a secret column that is also flagged internal is in both, and a naive union names it twice in the error. Deduped, with a test pinning the "once" property (match(/…\.key/g) has length 1). Nobody asked for that; it is the kind of thing that ships as a papercut otherwise.
   2. They must not be collapsed into one collector, because their other consumers answer differently — the read path MASKS a credential type and OMITS a flagged field, and a flagged column must never acquire a mask (on a required column the mask is zero bits of information and still puts a value under a field whose declaration promises none). The ⛔ against collapsing is now written into both docstrings, where the next person to "simplify" this will actually read it.

   The controls do their job, and one of them is subtler than it looks

   Written first, before the guard was touched, and all three proven able to fail via two mutations:

   • protectedFields = Object.keys(schema.fields) (refuse everything) → the two aggregate controls go red plus "names every refused field once" catches the over-breadth by seeing prefix in the message.
   • hit = protectedFields (ignore what the query references) → the fast-path control stays green exactly as predicted, because its object has no flagged field at all.

   ⭐ The control I did not ask for and should have: itest_plain.key shares its NAME with the flagged column on the other object. A guard that collected field names globally rather than per-schema passes every other test in this file and fails only that one.

   Both ratchets were red mid-flight and were fixed in the diff, not by moving a baseline

   This is the part worth recording, because the sibling card shows the counterfactual:

   • check:query-options-erasure 242 → 257, caused by as any on the new aggregate() calls. Fixed by typing them (const SYSTEM: EngineReadOptions). Back to 242, at the ceiling.
   • check:type-check-debt objectql TEST_DEBT 355 → 356. Fixed by spelling the deliberately off-contract structured groupBy as as unknown as EngineAggregateOptions — the form the ratchet's own message prescribes — with a comment naming the contract being bypassed and why (EngineAggregateOptions.groupBy is declared string[] while the engine also reads { field, dateGranularity }). Back to 355.

   #7867's dev hit the identical 355 → 356 and did not see it, because its gate table — 17 suites, repo-wide typecheck and build, five check:* scripts — did not include either ratchet, and pnpm typecheck passing says nothing about them. Same defect class, same package, same hour; the difference is entirely that this card's brief named both ratchets as a deliverable and that one buried them in a paragraph. PR #7989 is in a patch round for it, and the brief template is changing.

   Kept restraint worth naming: the error message was extended without dropping the Cannot aggregate credential field(s) prefix, so #3171's existing assertions stay green rather than being edited to match a new message. Editing other cards' pins to fit your own wording is how a floor quietly stops being one.

   Carried forward, not blocking

   EngineAggregateOptions.groupBy is declared narrower than the engine's real contract (#4918 family) — surfaced only because it forced the as unknown as. You flagged it for my call rather than filing or fixing it: I'll file it, since it now has a second witness. ⛔ Correctly not ridden into this PR.

   Auto-merge armed SQUASH. ⚠️ The receipt's empty fields carry no information — membership gets read from the added_to_merge_queue event or from main moving. On merge the container is archived in the same action.

   ---

   Generated by Claude Code
9. huangyiirene

   closed this as completedin #7984on Aug 12, 2026
10. akarma-synetal

    added a commit that references this issue on Aug 17, 2026

    fix(objectql): fold `internal: true` into the aggregate guard (object…

    ...

    fce8e49