Repository navigation
feat: read the abgen registry from the AB status commands - #397
Conversation
The abgen generator publishes to asset-bundle-registry-abgen.decentraland. <org|zone>, which nothing in the CLI could reach: pointer-consistency and world-ab-status both hardcoded the Unity registry URL, so checking an abgen conversion meant curling /entities/active by hand and trimming a ~160 KB entity payload down to the handful of fields that get reported. Both commands now take --registry unity|abgen, defaulting to unity so existing invocations are unchanged, and abgen on today is rejected up front (no abgen queue there, and the hostname does not resolve). Adds ab-queue-status for the same reason: /queues/status was another raw curl, and it prints per-platform counts plus the 1-indexed position of the entities you asked about instead of the whole queue. The registry URLs, the shared statusLabel, the reported-platform list (which is what keeps decommissioned webgl out of the output) and the separate-version- series note now live in helpers/ab-registry.ts.
The registry stores the version already prefixed ("v49"), so both status
commands printed "vv49". Share one formatVersion helper, print the value as
it comes, and treat an empty version string as N/A — a platform key with an
empty version means it never built.
The [abgen] label in the header already says which registry is being read; the extra line was documentation printed on every run.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: read the abgen registry from the AB status commands
PR: #397 · Branch: feat/abgen-registry-status → main
Files changed: 6 (+175/−55) · CI: pending (build)
Summary
Adds --registry unity|abgen to pointer-consistency and world-ab-status, creates a new ab-queue-status command, and extracts shared helpers (statusLabel, REPORTED_PLATFORMS, formatVersion, parseRegistry, registryUrlFor) into src/helpers/ab-registry.ts — eliminating the duplicated statusLabel function and hardcoded platform lists.
The refactoring is clean: the shared module is well-scoped, the new command follows existing patterns exactly, and the --registry flag defaults to unity so existing invocations are unaffected.
Findings
No P0 or P1 issues.
[P2] Unconsumed response body on error path — ab-queue-status.ts:23-25
When !response.ok, the response body is not consumed before throwing. For a CLI that exits immediately this is benign (process termination cleans up the socket), but adding await response.body?.cancel() before the throw would set the right pattern for new code. Same pre-existing pattern in pointer-consistency.ts:62-64 and world-ab-status.ts:79-82.
[P2] formatVersion drops the manual v prefix — intentional bug fix
Old code: v${version.version} → "vv49" if the registry stores "v49".
New code: ${version.version} → "v49" as-is.
The inline comment at ab-registry.ts:43 confirms the registry already stores the prefix, and the PR body confirms live verification. This is a fix, not a regression. The secondary improvement (version?.version guard instead of just truthy object check) is strictly more robust.
[P2] Dead code in world-ab-status.ts — pre-existing
Two maps (registryByEntityId at line 92 and entityIdByPointer at line 57) are populated but never read. The actual lookup uses registryEntities.find(...). Good cleanup opportunity if you're touching the file.
[P2] RegistryKind imported as value, used only as type — pointer-consistency.ts:7
import { type RegistryKind, ... } would be clearer intent, though no runtime impact since verbatimModuleSyntax is not enabled.
What looks good
- Type safety:
RegistryKindunion +isRegistryKindtype guard +parseRegistryvalidation is well-structured. TheQueueStatusrecord type is appropriate for the dynamic${platform}PendingJobskey pattern. - Input validation:
parseRegistryrejects unknown registry values and the invalidabgen+todaycombination up front with clear error messages. - URL safety:
encodeURIComponent(world)used in both URL paths and query strings inworld-ab-status.ts.registryUrlForconstructs URLs from a validatedRegistryKind— no injection risk. - Fetch bodies consumed: All happy-path fetches consume bodies via
.json(). - Pattern consistency: The new
ab-queue-statuscommand follows the exact same structure as existing commands (arg parsing, CliError, console output). - Deduplication: Extracting the shared module eliminates real duplication without over-abstracting.
- README: Thorough documentation including the registry table, version series caveat, and
today/abgen rejection.
Consumer Impact
Backward-compatible. --registry defaults to unity; ab-queue-status is additive. Output labels now include [unity]/[abgen], which the agent-server ab-status skill accounts for in stacked PR #266. No public API surface changed.
Git Conventions (ADR-6)
- PR title:
feat: read the abgen registry from the AB status commands✅ - Branch:
feat/abgen-registry-status✅
Security
- Read-only endpoints, no credentials or authentication involved
- No new dependencies added
- URL construction from validated inputs only
DEPENDENCY_REVIEW: PASS
Verdict: ✅ APPROVE
No P0 or P1 findings. The code is clean, well-structured, and backward-compatible. The P2s are minor — the dead-code cleanup is the most actionable if you want to tidy up while you're in the files.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
entityIdByPointer and registryByEntityId were populated and then ignored; the scene/registry match goes through registryEntities.find().
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review: feat: read the abgen registry from the AB status commands
PR: #397 · Branch: feat/abgen-registry-status → main
Files changed: 6 (+175/−55) · CI: build pending
Commits: 5 (feat + fix + 2 chores + style)
Head: 4ee1869
Summary
Adds --registry unity|abgen to pointer-consistency and world-ab-status, creates a new ab-queue-status command, and extracts shared helpers (statusLabel, REPORTED_PLATFORMS, formatVersion, parseRegistry, registryUrlFor) into src/helpers/ab-registry.ts.
Since the first review, the PR picked up four follow-up commits:
fix: stop double-prefixing the bundle version— thevv49bug fix cleanly separatedchore: drop the version-series reminder line— the[abgen]label already signals which registrystyle: collapse the import prettier can now fit on one linechore: drop two maps world-ab-status never read— removesentityIdByPointerandregistryByEntityId(addressed from the earlier P2)
Findings
No P0 or P1 issues.
[P2] Unconsumed response body on error path — ab-queue-status.ts:23-24
When !response.ok, the body is not consumed before throwing CliError. For a CLI that exits on throw this is benign, but await response.body?.cancel() before the throw would be clean. Same pre-existing pattern in pointer-consistency.ts:62-64 and world-ab-status.ts:79-81.
[P2] RegistryKind imported as value, used only as type — pointer-consistency.ts:7
RegistryKind is only used as the type annotation for the kind parameter at line 50. Using import { type RegistryKind, ... } would be clearer. No runtime impact.
What looks good
- Shared helper module (
ab-registry.ts): Well-scoped extraction —RegistryKindunion +isRegistryKindtype guard +parseRegistryvalidation is solid.parseRegistryrejects invalid kinds and theabgen+todaycombination up front with clear error messages. - New
ab-queue-statuscommand: Follows the exact same structure as existing commands.QueueStatustype is appropriate for the dynamic${platform}PendingJobskeys. Jobs presence is checked before accessing.length. - Dead code cleaned up: Both unused maps (
registryByEntityId,entityIdByPointer) removed in a dedicated commit. The pointer collection simplified toallPointers.push(...scene.parcels). formatVersionfix: Correctly drops the manualvprefix (registry already stores"v49"), and the guard is nowversion?.version(truthy string) instead of just truthy object — handles empty-string edge case.- URL safety:
encodeURIComponent(world)in both URL paths and query strings.registryUrlForbuilds URLs from a validatedRegistryKind. - Fetch bodies consumed: All happy-path fetches consume bodies via
.json(). - Backward compatible:
--registrydefaults tounity;ab-queue-statusis additive. Existing invocations unchanged. - Commit structure: Clean semantic separation — feature, fix, chores, style. Each commit is self-contained.
- README: Thorough documentation including the registry table, version series caveat, and
today/abgen rejection.
Consumer Impact
Backward-compatible. The agent-server ab-status skill (PR #266) accounts for the new output labels and ab-queue-status command. No other known consumers of the output format.
Git Conventions (ADR-6)
- PR title:
feat: read the abgen registry from the AB status commands✅ - Branch:
feat/abgen-registry-status✅ - Individual commits follow semantic format ✅
Security
- Read-only endpoints, no credentials or authentication
- No new dependencies
- URL construction from validated inputs only
- No injection vectors
DEPENDENCY_REVIEW: PASS
Verdict: ✅ APPROVE
No P0 or P1 findings. The code is clean, well-structured, and backward-compatible. The follow-up commits addressed the dead-code and version-prefix findings from the first pass. Only two minor P2s remain.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
3 participants
--platform abgen(#396) can queue an abgen conversion, but nothing in the CLI could read the result:pointer-consistencyandworld-ab-statusboth hardcodehttps://asset-bundle-registry.decentraland.<env>, while abgen publishes toasset-bundle-registry-abgen.decentraland.<org|zone>.--registry unity|abgenon both status commands, defaulting tounity— existing invocations are unchanged.abgen+todayis rejected up front: no abgen queue there, and the hostname does not resolve.ab-queue-statuscommand: per-platform pending counts and the 1-indexed position of any--cidpassed, instead of curling the full queue.statusLabel,formatVersionand the reported-platform list (what keeps decommissionedwebglout of the output) move tohelpers/ab-registry.ts.world-ab-statusthat were populated and never read.vv49: the registry already stores the version prefixed, and both commands added anotherv.Tested against
organdzonewith a scene, a world and a base wearable — same payload shape from both registries,todayguard exits in 0.3s with no network call.🤖 Generated with Claude Code