fix(serenity): remove dormant Semrush AI allocator, §10.1/§10.2 (SITES-49206) - #2995
fix(serenity): remove dormant Semrush AI allocator, §10.1/§10.2 (SITES-49206)#2995aliciadriani wants to merge 12 commits into
Conversation
Semrush no longer enforces AI project/prompt limits for proxy-routed LLMO workspaces (confirmed 2026-07-28), so the just-in-time (JIT) allocation machinery only works around limits that no longer apply. This removes the dormant allocator and unpicks its wiring. Removed: - Modules: dynamic-allocation-active.js, resource-manager.js, resource-lock.js (+ their test files, and the dynamic-allocation-fronting test). - allocation-metrics.js: dropped the six allocator-only recorders; kept recordRejection + recordMeteredQuotaClassifier (load-bearing for the §10.6 quota-exhaustion handling that is deleted LAST). - Wiring: controllers/serenity.js, brand-provisioning.js, handlers/prompts-subworkspace.js, handlers/markets-subworkspace.js (headroom guards, ensure/retryOnQuota, releaseAiSurplus, modelChangeUnits). - rest-transport.js transferWorkspaceResources (allocator-only method). - errors.js: isPoolExhausted / isWorkspaceNotReady predicates and the ORG_POOL_EXHAUSTED / BRAND_AI_LIMIT / WORKSPACE_BUSY codes. - Flags SERENITY_DYNAMIC_ALLOCATION / SERENITY_BRAND_AI_CEILING_PROJECTS / SERENITY_BRAND_AI_CEILING_PROMPTS (in-code references only). - scripts/serenity-rightsizing-sweep.mjs — pure allocator tooling that hard-imports the deleted releaseAiSurplus; could not compile without it. §10.2 (flat pre-carve) was already absent — createSubworkspace posts no resources body — so only the stale header comment was updated. Kept (behaviour byte-for-byte, since the allocator flag was OFF in every deployed environment): §10.3 publish best-effort/quota handling, §10.6 quota-exhaustion handling, and the §10.7 metered-405 canary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…(§10.7) Addresses PR-review doc-drift: the dormant JIT allocator was removed in this PR but the docs still described it as live/dormant. - ADR-007 (cross-container resource lock): marked SUPERSEDED — the race and its mitigation no longer exist. - ADR-008 (no-subworkspace-carve): amendment note — the no-carve decision now unconditional; the "retained behind the flag" fallback is gone. - docs/serenity.md: dropped "when to turn the JIT allocator on" + the "Dynamic AI resource allocation — operations" section (they instructed operators to enable now-deleted code); kept the still-valid no-carve rationale and pointed the premise re-check at the metered-405 canary. - Deleted docs/runbooks/serenity-zombie-workspace-recovery.md — its condition (a partially-applied allocation transfer) is now unreachable. - .gitignore: dropped the stale !scripts/serenity-rightsizing-sweep.mjs negation (the script was deleted with §10.1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger no release when merged. |
| # ADR-007: Cross-container serialization for the dynamic-allocation absolute-set race | ||
|
|
||
| > **Applies only when `SERENITY_DYNAMIC_ALLOCATION` is on**, which it is not in any deployed | ||
| > **SUPERSEDED (2026-08, SITES-49206).** The dynamic (JIT) Semrush AI allocator this ADR concerns — |
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): this PR deletes resource-lock.js/resource-manager.js, so the race this ADR documents no longer exists. Marked SUPERSEDED per §10.7 (kept as a decision record, not deleted).
| @@ -1,5 +1,11 @@ | |||
| # ADR-008: A brand sub-workspace carries no resource allocation | |||
|
|
|||
| > **Update (2026-08, SITES-49206).** This decision stands and is now **unconditional**: the dormant | |||
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): the no-carve decision here is now unconditional — the dormant JIT allocator it referenced as a flag-gated fallback is deleted in this PR. Amendment note added; core decision retained.
| future re-tightening upstream surfaces as a failed canary rather than a silent customer-facing | ||
| error. Run it per environment when validating the premise. | ||
|
|
||
| > **Removed (SITES-49206):** the just-in-time (JIT) top-up allocator |
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): removed the "when to turn the JIT allocator on" + "Dynamic AI resource allocation — operations" sections (they instructed operators to enable now-deleted code). Kept the valid no-carve rationale; premise re-check now points at the retained metered-405 canary.
|
Self-review pass (round 1) — applied fixes Code review found the removal itself clean: no dangling references to removed symbols anywhere in
Re-review: nothing further to address. One known nit deliberately left: |
aliciadriani
changed the title
There was a problem hiding this comment.
Full manual review
Summary
Removes the dormant JIT Semrush AI allocator (§10.1) and the flat pre-carve (§10.2): ~4.5k lines of source+tests deleted, plus wiring unpicked across the controller, both metered handlers, brand-provisioning, workspace-lifecycle, rest-transport, and errors. The allocator was OFF in every deployed environment and Semrush stopped enforcing AI limits for proxy-routed LLMO workspaces (confirmed 2026-07-28), so this is a pure dead-code removal. Overall quality: high — consistent, well-scoped, green.
Must Fix (blocks merge)
None.
Should Fix
None. (The doc-drift found in review — ADR-007/008, docs/serenity.md, the zombie runbook, .gitignore — was fixed in commit 582d42e with inline comments.)
Nits (optional)
quota-alerts.jsQuotaCasetypedef still listsorgPoolExhausted/brandAiLimit— cases the now-removed allocator was the only producer of. Harmless (kept §10.6 file, never enabled) and retires with §10.6; no action here.- The future §4 settle-poll removal planned to reuse
isWorkspaceNotReady; it's removed here, so that later work re-introduces a bounded not-ready retry. Not a bug — just a forward note.
What's Good
- Verified no dangling references:
isDynamicAllocationEnabled,resolveBrandAiCeiling,ensureAiHeadroom,releaseAiSurplus,withResourceLock,createHeadroomGuard,transferWorkspaceResources,isPoolExhausted,isWorkspaceNotReady,bodyText,countPublishedPrompts— zero remaining callers insrc/test. - Correct partial trim of
allocation-metrics.js:recordRejection+recordMeteredQuotaClassifierare still imported by the deliberately-deferred §10.6 code (errors.js,markets-subworkspace.js) — deleting the whole module would have broken it. Verified. - Metered handlers:
retryOnQuota/headroom.ensurewere no-op passthroughs with the flag off (always), so the direct calls are byte-for-byte prod behaviour. ci / build(incl. tests) green,type-checkbase+strict green, 16,362 tests pass.
Verdict: approve-worthy. Needs a non-author CODEOWNER approval to clear branch protection.
…0.1) The `Serenity API — dynamic allocation ON (metered JIT via the live UM mock)` block asserted the JIT allocator's behaviour — a live /resources top-up transfer and a brandAiLimit 409 — which this PR removed. With the allocator gone the writes correctly return 200/201, so the block failed `expected 200 to equal 409` in ci/it-postgres. Deleted the block and its now-unused SERENITY_MOCK_WORKSPACE_ID / SERENITY_ORG_PARENT_WS_ID imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
# Conflicts: # src/controllers/serenity.js # src/support/serenity/resource-manager.js
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
The removal itself is faithful to §10.1/§10.2 and behaviour-preserving. I checked the flag-OFF equivalence rather than taking it on trust: on main, createHeadroomGuard with enabled: false returns ensure: async () => ({ toppedUp: false }) and retryOnQuota: (fn) => fn(), and both publish seams already default to an identity wrapper (handlers/prompts.js:402, handlers/markets.js:1209), so every deleted wrapper was a literal passthrough. The positional parentWorkspaceId that ensureSubworkspace/assertNotParent need survives; only the allocator's copy in the options bags went.
Four things to change before this lands.
1. The rightsizing sweep is not a deviation — the description has it backwards
§10.7 prescribes exactly what this PR did:
scripts/serenity-rightsizing-sweep.mjs— [...] It goes with §10.1, which it cannot compile without — it hard-importsreleaseAiSurplusand the block constants.
"Delete last" in §10.7 applies to the metered-405 canary, not the sweep. Deviation 2 should come out of the description; deleting the sweep is compliance, not divergence.
Deviation 1 (trimming allocation-metrics.js instead of deleting it) is correct and the spec is what's inconsistent: recordRejection is called by toQuotaExceededError (src/support/serenity/errors.js:173) and recordMeteredQuotaClassifier by isMeteredQuota (errors.js:112), both §10.6 keeps, while §10.6 itself names recordMeteredQuotaClassifier in its own delete set. Keeping the two is the only resolution that compiles — worth saying that plainly rather than as a deviation needing forgiveness.
2. Dead code the removal creates but does not clear
Each of these had exactly one production caller, and this PR deletes it:
countPublishedPrompts(src/support/serenity/handlers/markets.js:1095) — called only by the net-model-delta sizing inhandleUpdateModelsSubworkspace. It now has no caller insrc/; only its own unit test (test/support/serenity/handlers/markets.test.js:1886-1926) keeps it alive.alertPoolFreeThreshold(src/support/serenity/quota-alerts.js:264) — called only fromresource-manager.js:445onmain. It is the allocator's org-pool early-warning, so it is §10.1 in substance even though §10.6 filesquota-alerts.jswholesale for later. Delete it here, or state in the follow-up list that §10.6 inherits a function that can no longer fire.- The two
wrapPublishseams are now caller-less identity wrappers, and their JSDoc still describes the machinery this PR removes:handlers/prompts.js:381-386("The subworkspace create-prompts caller passesheadroom.retryOnQuota(LLMO-6190 item 4)") andhandlers/markets.js:1184-1196(same). §10.3 revisits this seam anyway, so keeping the parameter is defensible — the doc text pointing at deleted code is not.
3. Test and IT-harness leftovers
transferWorkspaceResourcesis still stubbed and assertedto.not.have.been.calledagainst a transport method that no longer exists:test/support/serenity/workspace-lifecycle.test.js(17 sites),test/support/serenity/brand-provisioning.test.js(7),test/support/serenity/handlers/markets-subworkspace.test.js:93,1021. Those assertions can no longer fail. The latter two files are already touched here.setUmMockQuota(test/it/postgres/setup.js:165) has no consumer left once the flag-ON IT block goes, but is still exported and threaded intomockControls(test/it/postgres/serenity.test.js:16,21). Keeping the__quotacontrol route until the spacecat-shared §10.5 change lands is reasonable — say so at the seam instead of leaving it looking wired.- Comments that still justify their subject by the removed guard:
test/it/postgres/setup.js:158("Used by the dynamic-allocation flag-ON IT"),test/it/shared/seed-ids.js:69-71("the condition the dynamic-allocation JIT guard needs to engage"),test/it/postgres/seed-data/organizations.js:30,test/it/README.md:206, andtest/support/serenity/handlers/markets.test.js:1916("resource-manager'smodelChangeUnitssizing").
4. ADR-008 keeps two pointers that no longer resolve
docs/serenity.md and ADR-007 are handled well — the replacement paragraph naming the canary as the premise re-check is exactly what §10.7 asked for. ADR-008 is the gap: the new header marks what follows as historical, but two of those lines are live operator instructions, not history.
docs/decisions/008-no-subworkspace-resource-carve.md:70— "scripts/serenity-rightsizing-sweep.mjscan reclaim them if a pool ever does matter" points at a file this PR deletes. The consequence is now that stranded carves have no remedy; the line should say that.:76— points atdocs/serenity.md§ When to turn the JIT allocator on, a section this PR removes.:72-73and:85now describe the inverse of reality: the alternative recorded as "Delete the JIT allocator alongside the carve. Rejected" is the decision being taken here. An ADR whose rejected alternative becomes the adopted one normally gets a superseding record rather than a header note.
On the §10 precondition
§10 requires verifying once per environment that a metered write into a zero-allocation sub-workspace succeeds, and names scripts/serenity-metered-405-canary.mjs as the instrument. It is ordered as a hard gate before §10.6, so this is not a blocker for §10.1 — but §10.1 deletes the mitigation that the removed "when to turn the JIT allocator on" procedure pointed at, on a premise currently resting on the vendor statement plus the single GM-migration observation. The canary stays in the tree and needs one IMS token; running it on dev, stage and prod and recording the results in the follow-up list would turn the premise into per-environment evidence, and it is the same run §10.6 will need regardless. The soak requirement ("not the same week") is met.
Description
Drop the Verification block — lint, type-check and test counts are what the checks report. Everything else in the description is worth keeping.
One observation for the record, no action needed here: §10.1's "wiring to unpick" list omits handlers/markets-subworkspace.js, which carried the largest share of the wiring. Good catch finding it — the inventory under-scoped.
…e ADRs (SITES-49206) Follows Rainer's review on #2995: - delete dead countPublishedPrompts (markets.js) and alertPoolFreeThreshold (quota-alerts.js) — both had the removed allocator as their only caller — with their tests. - keep the two wrapPublish injection seams but rewrite their JSDoc to stop pointing at the removed headroom.retryOnQuota machinery (§10.3 seam). - drop the now-vacuous transferWorkspaceResources stubs/assertions from the workspace-lifecycle, brand-provisioning and markets-subworkspace tests (the transport method no longer exists). - document the deliberately-retained setUmMockQuota / __quota IT seam at test/it/postgres/setup.js and serenity.test.js (kept for spacecat-shared §10.5); reconcile the stale guard-justifying comments in seed-ids.js, organizations.js and test/it/README.md. - add ADR-009 as the superseding record for the allocator removal (ADR-008's rejected "delete the allocator" alternative is now adopted); reconcile the ADR-007/008 headers + inline pointers and docs/serenity.md (which had wrongly listed the trimmed allocation-metrics.js as deleted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Inline notes marking each change made to address the review — one per fix, grouped by location.
| * @param {any} [log] | ||
| * @returns {Promise<number>} number of published prompts on the project. | ||
| */ | ||
| export async function countPublishedPrompts(transport, semrushWorkspaceId, projectId, log) { |
There was a problem hiding this comment.
Fix (dead code cleared): deleted countPublishedPrompts. Its only production caller was the net-model-delta sizing in handleUpdateModelsSubworkspace, which this PR removed — so the function had no caller left in src/ and only its own unit test kept it alive. Removed here with that test.
| * `headroom.retryOnQuota` (LLMO-6190 item 4) so a disguised metered-405 gets ONE bounded | ||
| * top-up+retry; flat-mode callers omit this param, so flat mode is untouched. | ||
| * the inner `publishProject` call. Retained as an injection seam for a future publish-retry | ||
| * wrapper (§10.3); no caller passes a non-identity wrapper today, so every publish is a plain call. |
There was a problem hiding this comment.
Fix (stale JSDoc): the wrapPublish parameter is kept as an injection seam for a future publish-retry wrapper (§10.3), but its JSDoc used to describe the removed headroom.retryOnQuota (LLMO-6190) machinery. Rewritten to describe the seam as it now is: identity default, no non-identity caller today.
| * item 4) so a disguised metered-405 gets ONE bounded top-up+retry per project BEFORE it is | ||
| * recorded as a failure; flat-mode callers omit this param, so flat mode is untouched. | ||
| * behavior). Retained as a per-project injection seam for a future publish-retry wrapper | ||
| * (§10.3); no caller passes a non-identity wrapper today, so every publish is a plain call. |
There was a problem hiding this comment.
Fix (stale JSDoc): same wrapPublish seam as in markets.js — parameter retained (§10.3), JSDoc rewritten to stop pointing at the removed headroom.retryOnQuota machinery.
| * @param {object} [log] | ||
| * @returns {Promise<void>} | ||
| */ | ||
| export async function alertPoolFreeThreshold({ |
There was a problem hiding this comment.
Fix (dead code cleared): deleted alertPoolFreeThreshold, the org-pool early-warning alert. Its only caller was resource-manager.js (the deleted allocator). It is §10.1 in substance even though §10.6 files quota-alerts.js wholesale for later, so removing it here avoids §10.6 inheriting a function that can never fire. Its tests were removed too.
| * Global kill-switch (`SERENITY_QUOTA_ALERTS_ENABLED`, default OFF) mirrors the existing | ||
| * `SERENITY_*` env-flag pattern (dynamic-allocation-active.js, rest-transport.js) — adding this | ||
| * capability is not the same as turning it on; ops configure the channel/token and flip it. | ||
| * `SERENITY_*` env-flag pattern (rest-transport.js) — adding this capability is not the same as |
There was a problem hiding this comment.
Fix (stale reference): the module header cited dynamic-allocation-active.js (deleted by this PR) as an example of the SERENITY_* env-flag pattern. Trimmed to reference only a file that still exists.
| - **Transport target (`env.js`):** `SEMRUSH_PROJECTS_BASE_URL` → PE mock, `SEMRUSH_USERS_BASE_URL` → UM mock (the User-Manager origin split landed in api-service#2656; it falls back to the projects host when unset, so production needs no new config). `NODE_TLS_REJECT_UNAUTHORIZED=0` trusts the self-signed certs (IT process only). **None of these need Vault / deployed-env config.** | ||
| - **Auth (`SERENITY_ALLOW_NON_IMS_AUTH=true`, test-only):** the serenity controller normally forwards only IMS-typed tokens. This flag lets the harness's non-IMS JWT through. It's sound only against the mocks (which ignore the forwarded bearer); in production the real Semrush gateway validates the token end to end, so this never weakens deployed auth. The flag is never set in any deployed environment. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering); the mutating tests reach them through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering — retained and still threaded through `mockControls`, but consumed by no test today after the allocator removal in SITES-49206; kept for the spacecat-shared §10.5 metered-write change); the mutating tests reach the others through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. |
There was a problem hiding this comment.
Fix (stale doc): the README presented setUmMockQuota() as reached by mutating tests. Noted it is retained-but-unconsumed after the allocator removal, kept for the spacecat-shared §10.5 change.
| > `resource-lock.js`) and its operational surface — the "when to turn it on" flip, the | ||
| > zombie-workspace runbook, and the rightsizing sweep — have been deleted. Semrush no longer enforces | ||
| > AI limits for proxy-routed LLMO workspaces, so there is no allocation to top up, exhaust, or | ||
| > reclaim. `allocation-metrics.js` is **trimmed, not deleted** — `recordRejection` and |
There was a problem hiding this comment.
Fix (incorrect doc): this block listed allocation-metrics.js among the deleted modules, but it was trimmed (kept recordRejection / recordMeteredQuotaClassifier for the §10.6 classifier). Corrected, and added the ADR-009 reference for the removal.
| @@ -0,0 +1,85 @@ | |||
| # ADR-009: Remove the dormant JIT Semrush AI allocator | |||
There was a problem hiding this comment.
Fix (superseding record): new ADR-009 — ADR-008's rejected alternative "Delete the JIT allocator alongside the carve" is the decision this PR actually takes, so it gets a superseding record rather than only a header note. It documents the deletion, why the earlier rejection resolved, and the metered-405-canary as the standing per-env check.
| @@ -1,5 +1,14 @@ | |||
| # ADR-008: A brand sub-workspace carries no resource allocation | |||
|
|
|||
| > **Update (2026-08, SITES-49206).** The **no-carve decision stands and is now unconditional.** Its | |||
There was a problem hiding this comment.
Fix (stale pointers / reversed decision): ADR-008's no-carve decision stands, but its secondary "retain the allocator dormant" decision is reversed. Upgraded the header to say so and point at ADR-009; fixed the two dead body pointers (the deleted rightsizing-sweep line → "no in-tree remedy"; the removed docs/serenity.md flip-procedure section) and marked the reversed Decision/Alternatives inline.
| # ADR-007: Cross-container serialization for the dynamic-allocation absolute-set race | ||
|
|
||
| > **Applies only when `SERENITY_DYNAMIC_ALLOCATION` is on**, which it is not in any deployed | ||
| > **SUPERSEDED (2026-08, SITES-49206; see [ADR-009](009-remove-dormant-jit-allocator.md)).** The |
There was a problem hiding this comment.
Fix (cross-reference): added a pointer to ADR-009 (the record of the allocator removal) in this already-superseded ADR's header, for navigability.
|
@rainer-friederich thanks for the thorough review — all of it is addressed in a1abe03 (inline notes on each change above). Summary: 2 — dead code the removal stranded
3 — test / IT-harness leftovers
4 — ADR-008 / superseding record
1 & description
Not done, by design: the §10 metered-405 canary run (dev/stage/prod, live IMS token) stays a human step on the follow-up checklist. Verification: lint clean, |
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Request changes - every fix from the last round landed and verifies clean, but the same commit deletes a transport method that the retained premise-check script still calls.
Changes: removes the dormant JIT Semrush AI allocator with its wiring, tests, allocator-era script and docs, and adds ADR-009 as the superseding decision record (43 files).
Must fix before merge
- [Critical] The retained metered-405 canary calls
transferWorkspaceResources, which this PR removes from the transport -src/support/serenity/rest-transport.js:1057(details inline) - [Important] ADR-009 records the canary as a standing signal, but section 10.7 makes it a manual probe that is itself deleted alongside the classifier -
docs/decisions/009-remove-dormant-jit-allocator.md:54(details inline)
Non-blocking (8): stale references the sweep did not reach
Each of these was accurate on main and became stale because of this PR, so they are removal residue rather than pre-existing debt.
- nit:
resource-lock.js's in-process lock cited as an existing peer cache, andclearResourceLockscited as a sibling reset -src/support/serenity/quota-alerts.js:34and:68(the same header block whose line 25 this commit corrected) - nit: the
QuotaCaseunion still admitsorgPoolExhaustedandbrandAiLimit; at head every producer passesbrandCarveExhausted, and their only former producer wasresource-manager.js-src/support/serenity/quota-alerts.js:40 - nit: cleanup guidance tells the operator to re-run
ensureAiHeadroom-scripts/serenity-metered-405-canary.mjs:54 - nit: the positional parent is documented as "the units pool when JIT allocation is on"; there is no JIT allocation at head -
src/support/serenity/handlers/markets-subworkspace.js:480 - nit: "// ensure was skipped: no settle/transfer was performed for this call" no longer has an assertion behind it (the test itself is fine - it still asserts the pre-resolved workspace) -
test/support/serenity/handlers/markets-subworkspace.test.js:1019 - nit:
dumpUmMockis now consumer-less exactly likesetUmMockQuota, but did not get the retention note this commit added to its sibling -test/it/postgres/setup.js:203, threaded attest/it/postgres/serenity.test.js:25, no longer destructured attest/it/shared/tests/serenity.js:843 - nit: "the mutating tests reach the others through the factory's
mockControlsargument" - "the others" is now onlydumpPeMock-test/it/README.md:206 - suggestion: the retained seam gets two different forward owners - the code says section 10.3, ADR-009 says section 10.6 -
docs/decisions/009-remove-dormant-jit-allocator.md:44vssrc/support/serenity/handlers/markets.js:1132andhandlers/prompts.js:383
Previously flagged, now resolved
- Description no longer frames the sweep deletion or the
allocation-metrics.jstrim as deviations, and the Verification block is gone. countPublishedPromptsandalertPoolFreeThresholddeleted with their tests; neither has a caller left anywhere at head.- Both
wrapPublishJSDoc blocks rewritten; no non-identity caller exists at head, so the text is accurate. - All 26 vacuous
transferWorkspaceResourcesstubs and assertions removed, and no survivingit()lost its only meaningful assertion. setUmMockQuota/__quotaretention documented at the seam and in the IT README; the guard-justifying comments now describe the survivingparentWorkspaceIdrequirement, whichworkspace-lifecycle.js:410still enforces.- ADR-008's two dead pointers fixed and its reversed decision marked, ADR-009 added as the superseding record, ADR-007 cross-referenced, and
docs/serenity.mdcorrected to stop listing the trimmedallocation-metrics.jsas deleted.
| * | ||
| * NOTE (SITES-49206): the just-in-time allocator that read this before a metered op was removed | ||
| * once Semrush stopped enforcing AI limits for proxy-routed LLMO workspaces; this read has no | ||
| * in-repo caller today but is kept as part of the transport's workspace-resource surface. |
There was a problem hiding this comment.
issue (blocking): scripts/serenity-metered-405-canary.mjs:122 calls transport.transferWorkspaceResources(...), and this PR deletes that method just below this note. The canary dies with a TypeError at step 2 (drain prompts.total to used) - after the getWorkspaceResources read on line 109, and before the publish that produces the evidence.
I checked every other transport method the script uses - listLanguages, createProject, createProjectTags, createPromptsByIds, publishProject, getWorkspaceResources. All six survive at head. transferWorkspaceResources is the only missing one, so the script is one method away from working.
Why this is more than a broken dev script:
- Section 10.7 keeps the canary deliberately and makes it the instrument for section 10's precondition: "Run it per env, record the result, and remove it only once section 10.6's classifier is gone."
- The description says the canary "only imports the transport" and is therefore safe to keep. It does only import the transport - the transport is what lost the method.
- ADR-009, added in this commit, names the canary twice as the standing per-environment premise check and as the replacement for the removed flip procedure.
docs/decisions/008-no-subworkspace-resource-carve.md:86and:104anddocs/serenity.md:457say the same. Four documented claims currently describe a script that cannot run. - ADR-009's own Alternatives section rejects keeping
serenity-rightsizing-sweep.mjsbecause it "hard-importsreleaseAiSurplus... so it cannot compile once the allocator is gone". That is the same dependency argument, applied to the transport rather than toresource-manager.js, and it was not carried across to the canary. - Nothing in CI can catch this.
scripts/sits outside thetsconfig.jsoninclude list, the call is a dynamic property access so lint does not resolve it, and the script never executes in CI. The green checks are not evidence here.
There is also an asymmetry in this very note: getWorkspaceResources is kept as having "no in-repo caller today but kept as part of the transport's workspace-resource surface", while its pair is deleted. The canary's step 1 is getWorkspaceResources and its step 2 is transferWorkspaceResources - one consumer, both methods.
Smallest fix: keep transferWorkspaceResources under the same rationale as the method above it, until sections 10.6 and 10.7 retire the canary together - that is the same "delete last" sequencing section 10.7 already grants the canary. If it has to go now, rewrite the drain step against a surviving call and re-run the script end to end, then correct the "it only imports the transport" line in the description and record the dependency in ADR-009 so the later sections sequence it knowingly.
There was a problem hiding this comment.
Fixed (now at branch head, commit 594dbd13). Restored transferWorkspaceResources on the transport with its unit test — you're right that it isn't allocator-only: it and getWorkspaceResources are the canary's read/drain pair. Both methods now carry a NOTE naming the canary as their sole remaining consumer and marking them delete-last with §10.6/§10.7. I also corrected the description's "removed transferWorkspaceResources" and "canary only imports the transport" claims, and recorded the dependency in ADR-009's Decision so §10.6/§10.7 sequence it knowingly.
Verified: node --check on the canary + a live typeof transport.transferWorkspaceResources === 'function' probe, type-check (base + strict) green, and the restored transport unit test passing.
|
|
||
| 1. *"The evidence supports 'our parents are unmetered', not 'no tenant is ever metered'."* Semrush's | ||
| confirmation plus the soak now support the stronger claim. The standing check that it still holds | ||
| is the `scripts/serenity-metered-405-canary.mjs` probe (serenity-docs#72 §10), run per |
There was a problem hiding this comment.
issue (blocking): this line calls the canary "the standing check", and line 70 calls it "the replacement signal" for the removed flip procedure. Section 10.7 describes something narrower on both counts. It "needs a live IMS token and a real sub-workspace id, so a human runs it" - nothing schedules it, and this PR adds no scheduling. And it is marked "delete last": removed once section 10.6's classifier is gone.
So the only re-check mechanism this ADR records is itself queued for deletion inside the same cleanup. Once section 10.6 lands, this line, line 70, docs/decisions/008-no-subworkspace-resource-carve.md:86 and :104, and docs/serenity.md:457 all become dead pointers - the same defect class flagged in ADR-008 last round and repaired in this commit, reintroduced one step further out. Section 10.7 anticipates exactly this: "Whatever replaces this paragraph must say how the limits-unenforced premise gets re-checked, rather than pointing at a token that no longer exists."
This is the durable record for a 5,000-line deletion resting on a vendor behaviour change confirmed 2026-07-28, so a reader arriving cold after section 10.6 needs the successor named, not the interim probe.
Fix, prose only and on this branch: in Consequences, say that the canary is a manual per-environment probe which section 10.7 retires together with the section 10.6 classifier, and that the durable re-check beyond that point is an open question section 10.6 must settle before deleting both. Align the ADR-008 bullet at line 86 to the same wording so the two records do not diverge.
There was a problem hiding this comment.
Fixed. Reframed the canary as an interim, manual per-environment probe that §10.7 retires together with the §10.6 classifier, and called out the durable re-check as an open question §10.6 must settle before deleting both. Applied to ADR-009 (both the rejection-grounds item and the Consequences bullet), the matching ADR-008 bullet (:86) and its Alternatives note (:104), and docs/serenity.md:457. ADR-009's Consequences now states explicitly that leaving a canary pointer past §10.6 would reintroduce the same dead-pointer defect class ADR-008 was repaired for last round.
…frame canary as interim probe (SITES-49206) Rainer's re-review on #2995: - [Critical] Restore transferWorkspaceResources on the transport (+ its unit test). The retained metered-405 canary (scripts/serenity-metered-405-canary.mjs:122) calls it; the PR had removed it, so the canary would TypeError at step 2. Neither it nor getWorkspaceResources is allocator-only any more — both are the canary's read/drain pair, retired with it under §10.6/§10.7 delete-last. Updated both methods' notes to name the canary as their consumer. - [Important] Reframe the canary as an interim, MANUAL per-env probe that §10.7 retires together with the §10.6 classifier — the durable re-check beyond that is an open question §10.6 must settle. Fixed ADR-009, ADR-008 and docs/serenity.md, which had called it a "standing signal" / "replacement signal". - Nits: quota-alerts.js header dropped resource-lock.js / clearResourceLocks refs; narrowed QuotaCase to brandCarveExhausted; canary cleanup note no longer says re-run ensureAiHeadroom; markets-subworkspace.js parent doc no longer cites JIT allocation; markets-subworkspace.test.js comment no longer claims a removed assertion; dumpUmMock got the same retention note as setUmMockQuota (both threaded-but-unconsumed; only dumpPeMock has a live consumer); README "the others" corrected; reconciled the wrapPublish seam owner (§10.3 in both code and ADR-009). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Re-review round: inline notes on each new fix. The two blocking items are answered in-thread; these mark the code/doc changes.
| * NOTE (SITES-49206): the just-in-time allocator that used to call this — `transferOnce` / | ||
| * `transferAndSettle` in the removed `resource-manager.js` — is gone. The sole remaining caller | ||
| * is the retained metered-405 canary (`scripts/serenity-metered-405-canary.mjs`, step 2), which | ||
| * drains a throwaway sub-workspace to zero prompt headroom to provoke the disguised 405. Kept — |
There was a problem hiding this comment.
Re-review fix [Critical]: restored transferWorkspaceResources (with its unit test). The retained metered-405 canary drains a throwaway sub-workspace through this method, so it is not allocator-only — it and getWorkspaceResources (its NOTE updated too) are the canary's read/drain pair, retired together under §10.6/§10.7 delete-last.
| /** @typedef {'brandCarveExhausted'|'orgPoolExhausted'|'brandAiLimit'} QuotaCase */ | ||
| // `orgPoolExhausted` / `brandAiLimit` were produced only by the removed allocator (SITES-49206); | ||
| // every producer at head passes `brandCarveExhausted`. | ||
| /** @typedef {'brandCarveExhausted'} QuotaCase */ |
There was a problem hiding this comment.
Re-review nit: narrowed QuotaCase — orgPoolExhausted / brandAiLimit were produced only by the removed allocator; every producer at head passes brandCarveExhausted.
| * five times is one sales signal, not five pages." This is warm-Lambda-container-scoped state | ||
| * (no DynamoDB/persistent store), the same shape as this codebase's existing per-container caches | ||
| * (markets.js's language cache, resource-lock.js's in-process lock) — sufficient for collapsing a | ||
| * (markets.js's language cache) — sufficient for collapsing a |
There was a problem hiding this comment.
Re-review nit: header no longer cites resource-lock.js's in-process lock (deleted) as a peer cache; the clearResourceLocks sibling-reset mention on clearQuotaAlertDedup was dropped too.
| * @param {object} brand - brand record/stub being provisioned. | ||
| * @param {string} parentWorkspaceId - the org parent workspace: the `assertNotParent` guard | ||
| * input, and the units pool when JIT allocation is on. Nothing is carved from it. | ||
| * @param {string} parentWorkspaceId - the org parent workspace, used as the `assertNotParent` guard |
There was a problem hiding this comment.
Re-review nit: the positional parentWorkspaceId doc no longer says it's "the units pool when JIT allocation is on" — there is no JIT allocation at head; it's only the assertNotParent guard input now.
| * to run against a disposable dev/throwaway sub-workspace. Re-run `ensureAiHeadroom`/an ordinary | ||
| * API top-up (or just re-activate the brand) afterwards if the workspace needs to keep working, or | ||
| * decommission the throwaway workspace entirely. | ||
| * to run against a disposable dev/throwaway sub-workspace. To restore headroom afterwards, transfer |
There was a problem hiding this comment.
Re-review nit: cleanup guidance no longer tells the operator to re-run the removed ensureAiHeadroom; it now points at the inverse transferWorkspaceResources transfer (or re-activating the brand).
| - **Transport target (`env.js`):** `SEMRUSH_PROJECTS_BASE_URL` → PE mock, `SEMRUSH_USERS_BASE_URL` → UM mock (the User-Manager origin split landed in api-service#2656; it falls back to the projects host when unset, so production needs no new config). `NODE_TLS_REJECT_UNAUTHORIZED=0` trusts the self-signed certs (IT process only). **None of these need Vault / deployed-env config.** | ||
| - **Auth (`SERENITY_ALLOW_NON_IMS_AUTH=true`, test-only):** the serenity controller normally forwards only IMS-typed tokens. This flag lets the harness's non-IMS JWT through. It's sound only against the mocks (which ignore the forwarded bearer); in production the real Semrush gateway validates the token end to end, so this never weakens deployed auth. The flag is never set in any deployed environment. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering); the mutating tests reach them through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` (PE store snapshot — the one control a mutating test reaches today, through the factory's `mockControls` argument). `dumpUmMock()` (UM store snapshot) and `setUmMockQuota()` (UM resource metering) are also threaded through `mockControls` but are consumed by no test today after the allocator removal in SITES-49206 — retained for the spacecat-shared §10.5 metered-write change. Readiness is polled via `waitForSemrushMocks()`. |
There was a problem hiding this comment.
Re-review nit: "the others" was inaccurate — only dumpPeMock has a live consumer; dumpUmMock / setUmMockQuota are retained-but-unconsumed.
| re-meter sizing, both of which had the allocator as their only caller; | ||
| - the flag-ON integration path. | ||
|
|
||
| Three things at the metered-write boundary are **kept**, none belonging to the allocator any more: |
There was a problem hiding this comment.
Re-review fix: records that getWorkspaceResources / transferWorkspaceResources are kept for the canary (delete-last), and splits the seam owners — wrapPublish → §10.3, classifier → §10.6 (aligning ADR with the code JSDoc).
| procedure.** ADR-008 pointed operators at a "when to turn the JIT allocator on" runbook; that | ||
| section and the allocator it armed are both removed. *For now* the `serenity-metered-405-canary.mjs` | ||
| probe is the re-check — a **manual** per-environment run whose failure is the trigger to | ||
| re-introduce the allocator from history, not to re-enable a flag. It is **not** a standing signal: |
There was a problem hiding this comment.
Re-review fix [Important]: the canary is reframed as an interim manual probe that §10.7 retires with the §10.6 classifier; the durable re-check is called out as an open question §10.6 must settle.
| - **A tenant whose parent enforces limits would get no sizing at all.** _(SITES-49206: this was the | ||
| risk the retained allocator covered. The allocator, and the "when to turn the JIT allocator on" | ||
| flip procedure this line pointed at in `docs/serenity.md`, are both removed — see ADR-009. The | ||
| re-check, for now, is the `scripts/serenity-metered-405-canary.mjs` probe — a **manual** |
There was a problem hiding this comment.
Re-review fix [Important]: aligned ADR-008's bullet to the same "interim manual probe, retired with §10.6" wording as ADR-009.
| The parent-pool premise is re-checked, not assumed: `scripts/serenity-metered-405-canary.mjs` | ||
| drives the real transport against a throwaway sub-workspace and publishes into zero headroom, so a | ||
| future re-tightening upstream surfaces as a failed canary rather than a silent customer-facing | ||
| error. It is a **manual** per-environment run (live IMS token + a real sub-workspace id; nothing |
There was a problem hiding this comment.
Re-review fix [Important]: the canary paragraph now says it's a manual per-env probe retired with §10.6, not the durable re-check.
|
@rainer-friederich re-review addressed — branch head [Critical] canary's transport dependency — restored [Important] canary over-described — reframed as an interim, manual per-env probe that §10.7 retires with the §10.6 classifier, with the durable re-check named as an open question §10.6 must settle. Applied across ADR-009, ADR-008 (:86, :104) and Nits (all 8) — Verified: lint + |
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Request changes - the restore landed and every nit from the last round is clean, but the note that justifies it names the wrong caller, and the type guard that pinned the restored method did not come back with it.
Changes: removes the dormant JIT Semrush AI allocator with its wiring, tests, allocator-era script and docs, and adds ADR-009 as the superseding decision record (43 files).
Must fix before merge
- [Important]
getWorkspaceResourcesis documented as canary-only, butelements.jscheckAccesscalls it in production - and ADR-009 plus the §10.6 checklist schedule it for deletion -src/support/serenity/rest-transport.js:1055(details inline) - [Important] The
@ts-expect-errorguard pinningtransferWorkspaceResourceswas deleted with the method and not restored with it -test/types/base/serenity-transport.types.js:55(details inline) - [Important] The records make the canary the premise re-check but describe its outcome backwards, and it has no failure signal -
docs/decisions/009-remove-dormant-jit-allocator.md:78(details inline)
Non-blocking (3): minor issues and suggestions
- nit:
caseType: 'orgPoolExhausted'is still the caseType-varying arm of the dedup-key test, after this commit narrowedQuotaCasetobrandCarveExhaustedalone -test/support/serenity/quota-alerts.test.js:116 - suggestion: the strict tier is the only one that reports an unknown member, so adding
getWorkspaceResourcesandtransferWorkspaceResourcescall lines totest/types/strict/serenity-transport-strict.types.jswould give the canary's call site the compile-time gate it has never had - a rename would otherwise stay green acrossrest-transport.jsand its unit test and break the canary again - nit: the delete-last coupling is documented one way only - the transport notes point at the canary, but the canary's own header says nothing about what its deletion strands -
scripts/serenity-metered-405-canary.mjs:16
Previously flagged, now resolved
transferWorkspaceResourcesrestored with a unit test whose payload shape matches both the canary's call site and the user-manager v2 transfer contract.- Canary reframed as an interim manual probe across ADR-009, ADR-008 and
docs/serenity.md, with the durable re-check named as an open question for §10.6. - All eight nits landed: the
resource-lock.js/clearResourceLocksreferences dropped,QuotaCasenarrowed, canary cleanup no longer citingensureAiHeadroom, the positional parent doc de-JIT'd, the assertion-less test comment reworded,dumpUmMockgiven the retention note, the IT README corrected, and thewrapPublishseam owner reconciled to §10.3 in both places. - The three test-file edits in this commit are comment-only; no assertion was weakened.
| * NOTE: use this on the master id for the pool — `/parent/resources` returns the workspace's | ||
| * OWN allocation, not the master pool (live-verified 2026-07-02). | ||
| * | ||
| * NOTE (SITES-49206): the just-in-time allocator that read this before a metered op was removed |
There was a problem hiding this comment.
issue (blocking): this NOTE says the canary is getWorkspaceResources's "one remaining caller". It is not. src/controllers/elements.js:703 calls it from checkAccess, the handler for GET /v2/orgs/:spaceCatId/brands/:brandId/serenity/brand-presence/access (LLMO-6747). That endpoint is the whole "Semrush workspace access needed" banner: it probes the resource-allowance endpoint with the caller's own IMS token and reads a 401/403 as hasAccess: false. It is live on main, untouched by this PR, and covered by test/controllers/elements.test.js.
The claim on the sibling below is correct - I checked transferWorkspaceResources at head and the canary really is its only caller outside the transport itself and the new unit test. So the two methods have two different lifetimes, and this commit binds them to one.
Why this is more than a comment being wrong: the same claim is now written into two places that outlive the comment.
docs/decisions/009-remove-dormant-jit-allocator.md:47says both methods "are therefore retired with the canary under the same delete-last sequencing".- The description's §10.6 checklist item says that step "also retires the canary itself and its two retained transport methods (
getWorkspaceResources/transferWorkspaceResources)".
That checklist is the instruction a future engineer executes. Following it deletes a method a production endpoint depends on, and the failure is quiet rather than loud: checkAccess documents every non-401/403 error as INDETERMINATE, which the UI treats as "assume access". A TypeError there maps to a 5xx, so the banner silently stops firing for users who genuinely lack workspace access, with nothing surfaced to them.
This is also last round's defect class, inverted. Then, a method was deleted because its surviving caller was invisible to CI. The fix for that records a caller inventory by hand, and the inventory under-counts - in the direction that looks safe.
Fix: name elements.js checkAccess here as the production caller and drop this method from the canary-coupled sequencing; only transferWorkspaceResources below is canary-scoped. Then split the pair in ADR-009:47 and in the description's §10.6 item so both record two methods with two lifetimes. While correcting that inventory: ADR-009's "Three things at the metered-write boundary are kept" at :44 is also missing the fourth retained surface - the IT __quota controls (setUmMockQuota / dumpUmMock, §10.5), which today exist only in the test files and not in the record a later section will read.
There was a problem hiding this comment.
Fixed (commit d96c01c8). You're right — elements.js checkAccess (GET .../brand-presence/access, LLMO-6747) is a live production caller, so getWorkspaceResources is not canary-scoped. Decoupled it everywhere:
- The transport NOTE now names
elements.js checkAccessas the production caller and says the method outlives the canary; onlytransferWorkspaceResourcescarries the canary-scoped/delete-last NOTE. - ADR-009's "kept surfaces" list is split into different lifetimes (canary-scoped
transferWorkspaceResourcesvs. production-scopedgetWorkspaceResources), and I added the fourth retained surface you flagged — the IT__quotacontrols (setUmMockQuota/dumpUmMock, §10.5). - The description's §10.6 checklist now retires only
transferWorkspaceResources, explicitly notgetWorkspaceResources.
The under-counted inventory was exactly the last-round defect inverted — thanks for catching the safe-looking direction.
| // @ts-expect-error - string has no properties in common with { projects?, prompts? }. | ||
| await transport.transferWorkspaceResources(parentWorkspaceId, { ai: 'unlimited' }); | ||
|
|
||
| // Argument types. |
There was a problem hiding this comment.
issue (blocking): the assertion that pinned transferWorkspaceResources was removed here along with the method, and did not come back with it. On main this block reads:
// Body shape, derived from the generated contract: `ai` carries unit counts.
// @ts-expect-error - string has no properties in common with { projects?, prompts? }.
await transport.transferWorkspaceResources(parentWorkspaceId, { ai: 'unlimited' });Everything else about the method came back in this commit - the implementation, the JSDoc with both @param tags, the WorkspaceResources typedef at rest-transport.js:77, the unit test, the ADR entry. This one artefact did not, and its absence is silent: a missing @ts-expect-error fails nothing.
src/support/serenity/CLAUDE.md names this file as the enforcement, in rule 6: "A new transport method documents its own parameters ... test/types/base/serenity-transport.types.js pins both halves." docs/decisions/005-opt-in-type-checking.md says the same. This file's own header explains why both halves are load-bearing - dropping a transport method's @param tags makes its assertions legal again, so the assertion is what keeps the tags honest.
With it gone, a later change that strips those tags or widens payload back to {object} compiles clean. That matters more for this method than for its neighbours, because its only consumer is the canary, and this PR's own argument is that scripts/ sits outside tsconfig and never runs in CI - so nothing else is watching.
Fix: re-add the three lines after the createSubworkspace arity block. The signature and the typedef are unchanged from main, so the expected error should be unchanged; if it is not, tsc tells you immediately as TS2578.
There was a problem hiding this comment.
Fixed (commit d96c01c8). Re-added the three lines after the createSubworkspace arity block, verbatim from main:
// Body shape, derived from the generated contract: `ai` carries unit counts.
// @ts-expect-error - string has no properties in common with { projects?, prompts? }.
await transport.transferWorkspaceResources(parentWorkspaceId, { ai: 'unlimited' });
The signature and WorkspaceResources typedef are unchanged from main, so the expected error is unchanged — type-check (base + strict) is green, i.e. no TS2578. I also took your suggestion and pinned both getWorkspaceResources and transferWorkspaceResources in test/types/strict/serenity-transport-strict.types.js, so a rename now fails as TS2339 under strict rather than only breaking the canary at runtime.
| - **A tenant whose parent enforces limits would get no sizing, and there is no longer a flip | ||
| procedure.** ADR-008 pointed operators at a "when to turn the JIT allocator on" runbook; that | ||
| section and the allocator it armed are both removed. *For now* the `serenity-metered-405-canary.mjs` | ||
| probe is the re-check — a **manual** per-environment run whose failure is the trigger to |
There was a problem hiding this comment.
issue (blocking): "whose failure is the trigger to re-introduce the allocator" - the canary has no failure to observe, and its on-screen labels read the other way round.
The script was written for LLMO-6190 to capture the disguised-405 fixture, and it still speaks that language at head:
scripts/serenity-metered-405-canary.mjs:175- "Publishing with zero prompt headroom - expecting the disguised metered-quota 405...":177- "UNEXPECTED: publish succeeded. The workspace may not actually be at zero headroom...":179- prints the 405 under "this is what isMeteredQuota must match"
Under this ADR's premise those meanings invert. A 405 at zero headroom means Semrush is enforcing again: the premise is broken and the allocator has to come back from history. A publish that succeeds at zero headroom is the confirmation the ADR wants. The script calls the first "expected" and the second "UNEXPECTED". And main() returns normally in both branches - exit(1) fires only on a usage error or an unexpected top-level throw - so "a failed canary" names nothing the script actually emits.
The labels themselves predate this PR. What this PR creates is the reliance on them: before it, ADR-008 pointed operators at the "when to turn the JIT allocator on" procedure in docs/serenity.md, and this commit replaces that pointer with the canary in three records at once - here at :78, at docs/decisions/008-no-subworkspace-resource-carve.md:87, and at docs/serenity.md:459. The description's §10.7 item then asks a human to run it on dev, stage and prod and record each result before §10.6. That operator gets UNEXPECTED: publish succeeded in the case where everything is fine, and will reasonably read it as a botched run.
It matters because this ADR accepts a one-way door - "re-introduced from history rather than re-enabled by a dead flag" - and names exactly one safeguard for it. A safeguard whose healthy result is printed as an anomaly is a nominal one.
Fix, prose only and on this branch: drop "failure" as the trigger vocabulary here, at ADR-008:87 and at docs/serenity.md:459, and name the observable outcome instead - the disguised 405 is the signal to re-introduce the allocator, a publish that succeeds against zero headroom confirms the premise holds. Then add two lines to the script header at scripts/serenity-metered-405-canary.mjs:16 saying it now has a second reading under ADR-009, and that its "expected" / "UNEXPECTED" labels refer to fixture capture rather than to the premise.
There was a problem hiding this comment.
Fixed (commit d96c01c8), prose-only across all three records plus the script header. Dropped "failure" as the trigger vocabulary and named the observable outcome instead: the canary exits 0 either way, so it's read by what the publish does — a publish that succeeds at zero headroom confirms the premise, and the disguised 405 means Semrush is enforcing again (the trigger to re-introduce the allocator). Applied at ADR-009:88-ish (Consequences), ADR-008:86, and docs/serenity.md:457. I also added a "SECOND READING" block to the script header (serenity-metered-405-canary.mjs:16) spelling out that its expected/UNEXPECTED labels are LLMO-6190 fixture-capture language and read the opposite way under this ADR's premise, plus the delete-last coupling to transferWorkspaceResources.
…pe guard, correct canary outcome semantics (SITES-49206) Rainer's third re-review on #2995: - [Important] getWorkspaceResources is NOT canary-only: elements.js checkAccess (GET .../brand-presence/access, LLMO-6747) calls it in production. Decoupled it from the canary in the transport NOTE, ADR-009 (split the two methods into different lifetimes + added the IT __quota controls as a retained surface), and the PR description's §10.6 checklist. Only transferWorkspaceResources is canary-scoped/delete-last. - [Important] Restored the transferWorkspaceResources @ts-expect-error body-shape guard in test/types/base/serenity-transport.types.js (deleted with the method, not restored with it). Also pinned both methods in the strict-tier types file so a rename is caught as TS2339 rather than only breaking the canary at runtime. - [Important] Corrected the canary outcome vocabulary in ADR-009/ADR-008/serenity.md: the canary exits 0 either way; a publish that SUCCEEDS at zero headroom confirms the premise, a disguised 405 means Semrush is enforcing again. Documented the script's inverted expected/UNEXPECTED (fixture-capture) labels and the delete-last coupling in the canary header. - Nit: quota-alerts dedup-key test no longer varies caseType with the removed orgPoolExhausted (uses rollbackFailed, a caseType the module still keys on). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Third re-review round: inline notes on each new fix (the three blocking items are also answered in-thread).
| * | ||
| * NOTE (SITES-49206): the just-in-time allocator that read this before a metered op was removed | ||
| * once Semrush stopped enforcing AI limits for proxy-routed LLMO workspaces, but this read has | ||
| * a live production caller — `elements.js` `checkAccess` (GET .../brand-presence/access, |
There was a problem hiding this comment.
Re-review fix [B1]: getWorkspaceResources is NOT canary-only — elements.js checkAccess (LLMO-6747 brand-presence access banner) calls it in production. This NOTE now names that caller and states the method outlives the canary; only transferWorkspaceResources (below) is canary-scoped/delete-last.
| * NOTE (SITES-49206): the just-in-time allocator that used to call this — `transferOnce` / | ||
| * `transferAndSettle` in the removed `resource-manager.js` — is gone. The sole remaining caller | ||
| * is the retained metered-405 canary (`scripts/serenity-metered-405-canary.mjs`, step 2), which | ||
| * drains a throwaway sub-workspace to zero prompt headroom to provoke the disguised 405. Unlike |
There was a problem hiding this comment.
Re-review fix [B1]: the transferWorkspaceResources NOTE no longer calls getWorkspaceResources its co-retired "read pair" — the two methods have different lifetimes; only this one is retired with the canary.
| // whole value went untyped. | ||
| await transport.listLanguages(); | ||
|
|
||
| // Compile-time gate for the workspace-resource surface (SITES-49206). Both methods are consumed |
There was a problem hiding this comment.
Re-review suggestion (taken): pin both transport methods in the strict tier so a rename/removal is reported as TS2339 rather than only breaking the canary at runtime (the canary sits outside tsconfig/CI).
| re-meter sizing, both of which had the allocator as their only caller; | ||
| - the flag-ON integration path. | ||
|
|
||
| Several surfaces are **kept**, none belonging to the allocator any more, and they have *different* |
There was a problem hiding this comment.
Re-review fix [B1]: split the retained surfaces by lifetime — transferWorkspaceResources is canary-scoped, getWorkspaceResources has a production caller (elements.js), plus the fourth surface you flagged: the IT __quota controls (§10.5).
| procedure.** ADR-008 pointed operators at a "when to turn the JIT allocator on" runbook; that | ||
| section and the allocator it armed are both removed. *For now* the `serenity-metered-405-canary.mjs` | ||
| probe is the re-check — a **manual** per-environment run. Read its outcome by what the publish does | ||
| at zero headroom, not by an exit code (it exits 0 either way): a **publish that succeeds** against |
There was a problem hiding this comment.
Re-review fix [B3]: the canary exits 0 either way; a publish that succeeds confirms the premise and a disguised 405 means Semrush is enforcing again. Also flags that the script's on-screen labels read the opposite way (fixture-capture language).
| risk the retained allocator covered. The allocator, and the "when to turn the JIT allocator on" | ||
| flip procedure this line pointed at in `docs/serenity.md`, are both removed — see ADR-009. The | ||
| re-check, for now, is the `scripts/serenity-metered-405-canary.mjs` probe — a **manual** | ||
| per-environment run read by what the publish does at zero headroom (it exits 0 either way): a |
There was a problem hiding this comment.
Re-review fix [B3]: aligned ADR-008's canary bullet to the corrected outcome semantics (405 = enforcing again = re-introduce allocator; success = premise holds).
| The parent-pool premise is re-checked, not assumed: `scripts/serenity-metered-405-canary.mjs` | ||
| drives the real transport against a throwaway sub-workspace and publishes into zero headroom. Read | ||
| the outcome by what the publish does, not by an exit code (it exits 0 either way): a publish that | ||
| **succeeds** at zero headroom confirms the premise holds, whereas the disguised **405** the script |
There was a problem hiding this comment.
Re-review fix [B3]: corrected the canary outcome description here too, and noted the inverted on-screen labels.
| * design doc says 405s as a disguised quota rejection (as opposed to a genuine Method-Not-Allowed). | ||
| * It prints the raw response status, headers, and body so a human can re-confirm the shape. | ||
| * | ||
| * SECOND READING (SITES-49206 / ADR-009): after the JIT allocator's removal this script is also the |
There was a problem hiding this comment.
Re-review fix [B3] + coupling nit: added a SECOND READING block — the script's expected/UNEXPECTED labels are LLMO-6190 fixture-capture language and invert under the ADR-009 premise; plus a DELETE-LAST COUPLING note (this script ⇄ transferWorkspaceResources; getWorkspaceResources is NOT coupled).
| orgId: 'org-1', brandId: 'brand-1', caseType: 'orgPoolExhausted', dimension: 'prompts', | ||
| // A distinct caseType proves it participates in the dedup key. `brandCarveExhausted` is | ||
| // the only case `alertQuotaRejection` emits now (SITES-49206 narrowed QuotaCase), so this | ||
| // arm uses `rollbackFailed` — another caseType the module still keys on |
There was a problem hiding this comment.
Re-review nit: the dedup-key test no longer varies caseType with the removed orgPoolExhausted; it uses rollbackFailed, a caseType the module still keys on (alertRollbackFailure).
|
@rainer-friederich third re-review addressed — branch head [Important] [Important] [Important] canary outcome semantics — dropped "failure" as the trigger vocabulary across ADR-009, ADR-008 and Nit — the dedup-key test no longer varies Verified: lint + |
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all three blocking items are fixed and verify clean, and the two nits landed with them.
Changes: removes the dormant JIT Semrush AI allocator with its wiring, tests, allocator-era script and docs, and adds ADR-009 as the superseding decision record (43 files).
Non-blocking (1): minor issues and suggestions
- suggestion: the SECOND READING block sends the operator to the success branch as the premise-confirming result, but that branch still reads
UNEXPECTED: publish succeeded. The workspace may not actually be at zero headroom, ..., and the script never re-reads the workspace after the drain - so "did the drain land" is an open confounder on exactly the reading this PR adds. OnegetWorkspaceResourcescall after the transfer, printing the post-drain totals, closes it and makes the header's existing "Prints the before/after totals" claim true -scripts/serenity-metered-405-canary.mjs:61and:138
Out of scope, worth tracking: docs/specs/2026-06-15-serenity-subworkspace-dual-mode-implementation-plan.md:149,171,214 still describes transferWorkspaceResources as part of decommissionBrandWorkspace and the de-resourced re-grant path. Those paths predate this PR and no longer exist, so a section-10.6 author greps up a phantom production caller - worth a line on that follow-up rather than a change here.
Previously flagged, now resolved
- The
getWorkspaceResourcescaller inventory is corrected everywhere it was wrong: both transport NOTEs, ADR-009's kept-surfaces list (now per-item lifetimes, including the section-10.5 IT controls), the PR description's three references, and a new DELETE-LAST COUPLING block in the canary header that states the exclusion from the other direction. - The base-tier
@ts-expect-errorpin is restored character-identical to what was removed, and a strict-tier pin now names both transport methods so a rename reports TS2339 instead of only breaking the canary at runtime. - Canary outcome semantics corrected in ADR-008, ADR-009,
docs/serenity.mdand the script header: each names the publish outcome rather than a failure, records that the script exits 0 either way, and flags that its own labels read inverted. quota-alerts.test.js's dedup arm now varies onrollbackFailed, a caseType the module genuinely emits.
…se-confirm confounder (SITES-49206) Rainer's approval + non-blocking suggestion on #2995: the metered-405 canary never re-read the workspace after the drain, so "did the drain land?" was an open confounder on exactly the premise-confirming reading this PR added (a publish that succeeds only confirms Semrush isn't enforcing if headroom was GENUINELY zero). Add one getWorkspaceResources call after the transfer, print the post-drain totals with a zero-headroom check, and reword the success/405 branches to read against it — which also makes the header's existing "prints the before/after totals" claim true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Non-blocking suggestion addressed (post-approval): the canary now re-reads and prints the post-drain totals. Out-of-scope stale-spec-doc item tracked on the follow-up checklist.
| ai: { projects: before.product_resources.ai.resources.projects.total, prompts: prompts.used }, | ||
| }); | ||
|
|
||
| // Re-read AFTER the drain and print the post-drain totals. Without this, "did the drain land?" |
There was a problem hiding this comment.
Non-blocking suggestion (taken): the canary now re-reads the workspace with getWorkspaceResources right after the drain and prints the post-drain totals with a zero-headroom check. This closes the "did the drain land?" confounder on the premise-confirming reading — a publish that succeeds only confirms Semrush isn't enforcing if headroom was genuinely zero — and makes the header's existing "prints the before/after totals" claim true. The success/405 branch messages now tell the operator to read against these post-drain totals.
|
Thanks @rainer-friederich for the approval. Both items from the approval round handled in Non-blocking suggestion (the drain confounder) — taken. The canary now issues one Out-of-scope stale spec doc — tracked, not changed here (it's a planning artefact). Added a follow-up checklist item: Verified: lint + |
2 participants
Premise
Semrush no longer enforces AI project/prompt limits for LLMO workspaces reached via the Adobe proxy (confirmed 2026-07-28). The just-in-time (JIT) allocation machinery this PR removes only ever worked around limits that no longer apply, and its global kill-switch (
SERENITY_DYNAMIC_ALLOCATION) was OFF in every deployed environment, so the removed code paths were dormant. Behaviour on the flag-OFF path is byte-for-byte identical before/after — a no-op headroom guard is simply replaced by the direct call it wrapped.Part of the Serenity Post-GA cleanup (story SITES-49196). Inventory: https://github.com/adobe/serenity-docs/pull/62
Jira: https://jira.corp.adobe.com/browse/SITES-49206
What was removed (§10.1 / §10.2)
Modules deleted (+ their tests):
src/support/serenity/dynamic-allocation-active.js(kill-switch, ceiling resolver,createHeadroomGuard,retryOnQuota)src/support/serenity/resource-manager.js(whole allocator:ensureAiHeadroom,releaseAiSurplus,readAiTotals,roundUpToBlock,modelChangeUnits,transferOnce/transferAndSettle,requireWorkspaceId, blocks/ceiling constants, the org-pool/brand-limit/workspace-busy error factories)src/support/serenity/resource-lock.js(withResourceLock,clearResourceLocks)scripts/serenity-rightsizing-sweep.mjs— deleted per §10.7: it hard-imports the now-deletedreleaseAiSurplusand block constants fromresource-manager.js, so it cannot compile or lint once the allocator is gone. It is pure allocator tooling with no life after the allocator. The metered-405 canary is kept (scripts/serenity-metered-405-canary.mjs); §10.7's "delete last" applies to it, not to the sweep. It readsgetWorkspaceResourcesand drains viatransferWorkspaceResources; only the latter is canary-scoped (the former has a liveelements.jscaller) — see therest-transport.jsnote below.resource-manager.test.js,resource-lock.test.js,dynamic-allocation-active.test.js,dynamic-allocation-fronting.test.jsWiring unpicked:
controllers/serenity.js— removed thedynamicAllocationEnabled/brandAiCeilinghelpers and thedynamicAllocation/parentWorkspaceId/ceilingoptions from all five subworkspace handler call sites.brand-provisioning.js— dropped the flag/ceiling resolution and options.handlers/prompts-subworkspace.js— removed the headroom guard,ensure, andretryOnQuotawrapping (direct calls restored).handlers/markets-subworkspace.js— not in the original file list, but it carried the largest share of the allocator wiring (headroom guards in create-market / update-models / generate-prompts,releaseAiSurpluson delete and net-model-removal,modelChangeUnits). Unpicked here because the modules can't be deleted while it imports them.rest-transport.js—transferWorkspaceResourcesandgetWorkspaceResourcesare both kept, but on different lifetimes.getWorkspaceResourceshas a live production caller —elements.jscheckAccess(GET.../brand-presence/access, LLMO-6747, the "workspace access needed" banner) — so it is not canary-scoped and stays regardless.transferWorkspaceResourcesis canary-scoped: the retained §10.7 metered-405 canary (which drains a throwaway sub-workspace through it) is its only caller, so it is retired with the canary under §10.6/§10.7 "delete last". A strict-tier type pin (test/types/strict/serenity-transport-strict.types.js) now guards both against a silent rename (the canary sits outsidetsconfig/CI). Recorded in ADR-009. (Earlier revisions of this PR removedtransferWorkspaceResourcesoutright, and then wrongly coupledgetWorkspaceResourcesto the canary — both corrected; thanks @rainer-friederich.)errors.js— removedisPoolExhausted/isWorkspaceNotReady(+ orphanedbodyText) and theORG_POOL_EXHAUSTED/BRAND_AI_LIMIT/WORKSPACE_BUSYcodes.isWorkspaceBusyErrordoes not exist in this repo (its elmo-side detector is out of scope).handlers/markets.jscountPublishedPrompts(allocator was its only caller) andquota-alerts.jsalertPoolFreeThreshold(called only fromresource-manager.js), plus their tests. The twowrapPublishpublish-retry seams are kept as identity-default injection points for §10.3, with their JSDoc updated to stop describing the removedheadroom.retryOnQuotamachinery.SERENITY_DYNAMIC_ALLOCATION/SERENITY_BRAND_AI_CEILING_PROJECTS/SERENITY_BRAND_AI_CEILING_PROMPTSremoved.§10.2 (flat pre-carve): already absent —
createSubworkspaceposts noresourcesbody. Only the stale module-header comment inworkspace-lifecycle.jswas updated.Spec inconsistency, resolved the only way that compiles
allocation-metrics.jsis trimmed, not deleted — and this is compliance, not a deviation. §10.6 keepstoQuotaExceededError/isMeteredQuota, which callrecordRejection(errors.js:173) andrecordMeteredQuotaClassifier(errors.js:112) respectively — yet §10.6's own delete set also namesrecordMeteredQuotaClassifier. Those two instructions cannot both be honoured: deleting the recorder (or the whole module) fails to compile against the §10.6-kept callers. So the six allocator-only recorders were removed and these two kept, with the test trimmed to match. Keeping them is the only resolution consistent with §10.6, and the §10.6 delete-LAST boundary is where they finally go.FOLLOW-UP checklist (deferred)
publishMode:'best-effort'rejection is a distinct upstream check that survives the limits change; only then removerepublishBestEffort's 405 swallow and the competitor/alias/brand-URL best-effort paths. Left fully in place here (the twowrapPublishseams remain wired at identity for this).setUmMockQuota/__quotaseam is deliberately retained (unconsumed today) for this change — seetest/it/postgres/setup.js.ERROR_CODES.QUOTA_EXCEEDED,toQuotaExceededError,isMeteredQuota,quota-alerts.js, the two keptallocation-metrics.jsrecorders, and the elmo half) — delete LAST, only after the §10.7 canary records the new upstream behaviour per env. This step also retires the canary itself and the one canary-scoped transport method (transferWorkspaceResources) — NOTgetWorkspaceResources, whichelements.jscheckAccesskeeps alive — and must settle the durable re-check of the limits-unenforced premise (the manual canary is only the interim one) — see ADR-009.scripts/serenity-metered-405-canary.mjson dev, stage and prod with a live IMS token and a real throwaway sub-workspace id (manual HUMAN step — nothing schedules it) and record each result here before §10.6. This turns the no-carve premise from "vendor statement + single GM-migration observation" into per-env evidence, and it is the same run §10.6 needs regardless. Soak ("not the same week") is already met.SERENITY_DYNAMIC_ALLOCATION/SERENITY_BRAND_AI_CEILING_*from Vault (dx_mysticat/{env}/api-service) — not touched here.docs/specs/2026-06-15-serenity-subworkspace-dual-mode-implementation-plan.md:149,171,214still describestransferWorkspaceResourcesas part ofdecommissionBrandWorkspaceand the de-resourced re-grant path. Those paths predate this PR and no longer exist, so a §10.6 author greps up a phantom production caller for the method. Correct or annotate that planning doc before §10.6 acts on the caller inventory (left as tracking, not changed here, since it is a planning artefact outside this PR's scope).docs/decisions/009-remove-dormant-jit-allocator.mdas the superseding record for the removal (ADR-008's rejected "delete the allocator" alternative is now the adopted decision); reconciled the ADR-007 / ADR-008 headers and inline pointers anddocs/serenity.md(including its incorrect listing of the trimmedallocation-metrics.jsas deleted).docs/runbooks/serenity-zombie-workspace-recovery.mdwas deleted with the allocator.