fix(browseros): product-aware release CLI with bounded listing#1505
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 20s —— View job Code Review
Overall this is a well-structured refactor. The subcommand conversion is clean, the golden test for Bugs1. Hardcoded "BrowserOS" in
return f"""<item>
<title>BrowserOS - {version}</title>This title is hardcoded regardless of the Same issue in notes = f"""## BrowserOS v{version}Both functions predate this PR, but since the PR is threading product-awareness through the whole CLI, these two spots now become incorrect for BrowserClaw. 2. Silent partial result on mid-pagination R2 failure
try:
response = client.list_objects_v2(**kwargs)
except Exception as e:
log_warning(f"R2 listing failed for {prefix}: {e}")
breakIf the listing fails mid-pagination (e.g., on page 3 of 5), the function silently returns only the versions collected so far. The warning says the listing failed, but the caller can't distinguish "empty" from "partial." Worse, Minor fix: return Design Observations3.
products = [_resolve_product(product)] if product else list(PRODUCTS.values())
release_ctx = create_release_context("", product=product) # calls _resolve_product again internally
resolved = _resolve_product(product) if product else None
products = [resolved] if resolved else list(PRODUCTS.values())
release_ctx = create_release_context("", product=resolved.id if resolved else None)4.
release_ctx = create_release_context("", product=product)The context's Test Gaps5.
def list_objects_v2(self, **kwargs):
return pages[int(kwargs.get("ContinuationToken", 0))]This works because the fake returns 6. No test for the multi-product listing output
Minor7.
8.
What's working well
|
Greptile SummaryThis PR threads
Confidence Score: 3/5The CLI restructuring and listing logic are solid, but the appcast and GitHub release-note commands now silently emit wrong product names for BrowserClaw. The product threading through fetch_all_release_metadata is correct everywhere, and the listing, download, and publish paths work properly. However, generate_appcast_item and generate_release_notes both hardcode 'BrowserOS' in their output — now that --product browserclaw is a reachable CLI path for both appcast and github create, operators using those commands for BrowserClaw will get XML with the wrong title/link and GitHub release notes with the wrong header. packages/browseros/bos_build/release/common.py — generate_appcast_item (line ~194) and generate_release_notes (line ~214) need a product parameter before BrowserClaw appcast/release-notes are usable. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["browseros release"] --> LIST["list [VERSION]"]
CLI --> APPCAST["appcast --version"]
CLI --> PUBLISH["publish --version"]
CLI --> DOWNLOAD["download --version"]
CLI --> GITHUB["github create --version"]
LIST -- "VERSION given" --> CTX1["create_release_context(version, product)"]
LIST -- "no VERSION" --> RESOLVE["_resolve_product(product)"]
RESOLVE --> PRODUCTS_ALL["PRODUCTS.values() or single product"]
PRODUCTS_ALL --> LIST_MODULE["ListModule(products, limit)"]
LIST_MODULE --> COLLECT["collect_product_rows(product, env)"]
COLLECT --> ALL_VER["list_all_versions(release_prefix, env)\nreleases/prefix/"]
COLLECT -- "default product only" --> LEG["list_legacy_versions(env)\nreleases/ filter digits-and-dots"]
ALL_VER & LEG --> MERGE["merge_product_versions()\ndedupe + sort desc"]
MERGE --> LIMIT["apply_list_limit(rows, limit)"]
LIMIT --> PRINT["_print_product_section()"]
CTX1 --> DETAIL["ListModule._list_version_details()\nfetch_all_release_metadata(version, env, product.id)"]
APPCAST --> CTX2["create_release_context(version, product)"]
CTX2 --> APPM["AppcastModule\nfetch_all_release_metadata(version, env, product.id)"]
APPM --> GEN["generate_appcast_item()\n hardcodes BrowserOS title/link"]
GITHUB --> CTX3["create_release_context(version, repo, product)"]
CTX3 --> GHM["GithubModule\ngenerate_release_notes()\n hardcodes BrowserOS header"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
CLI["browseros release"] --> LIST["list [VERSION]"]
CLI --> APPCAST["appcast --version"]
CLI --> PUBLISH["publish --version"]
CLI --> DOWNLOAD["download --version"]
CLI --> GITHUB["github create --version"]
LIST -- "VERSION given" --> CTX1["create_release_context(version, product)"]
LIST -- "no VERSION" --> RESOLVE["_resolve_product(product)"]
RESOLVE --> PRODUCTS_ALL["PRODUCTS.values() or single product"]
PRODUCTS_ALL --> LIST_MODULE["ListModule(products, limit)"]
LIST_MODULE --> COLLECT["collect_product_rows(product, env)"]
COLLECT --> ALL_VER["list_all_versions(release_prefix, env)\nreleases/prefix/"]
COLLECT -- "default product only" --> LEG["list_legacy_versions(env)\nreleases/ filter digits-and-dots"]
ALL_VER & LEG --> MERGE["merge_product_versions()\ndedupe + sort desc"]
MERGE --> LIMIT["apply_list_limit(rows, limit)"]
LIMIT --> PRINT["_print_product_section()"]
CTX1 --> DETAIL["ListModule._list_version_details()\nfetch_all_release_metadata(version, env, product.id)"]
APPCAST --> CTX2["create_release_context(version, product)"]
CTX2 --> APPM["AppcastModule\nfetch_all_release_metadata(version, env, product.id)"]
APPM --> GEN["generate_appcast_item()\n hardcodes BrowserOS title/link"]
GITHUB --> CTX3["create_release_context(version, repo, product)"]
CTX3 --> GHM["GithubModule\ngenerate_release_notes()\n hardcodes BrowserOS header"]
|
Greptile SummaryThis PR replaces a flag-soup callback (
Confidence Score: 4/5Safe to merge — all call sites that fetch release metadata now pass the product ID, the removed constant is verified byte-identical via a golden test, and 30 new tests cover listing logic, pagination, merge/dedupe, and the CLI surface. The core change is well-tested and logically sound. The only issues are cosmetic: an inaccurate comment in the exception handler, a double _resolve_product call in the listing path, an or-based None check that conflates empty-list with unset, and no warning when --all silently discards a -n value. None of these affect production behavior today. No files require special attention; the minor style observations are concentrated in cli/release.py, release/list.py, and release/common.py. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[browseros release cmd] --> B{subcommand?}
B --> C[list]
B --> D[appcast]
B --> E[publish]
B --> F[download]
B --> G[github create]
C --> H{version arg?}
H -- yes --> I[create_release_context version + product]
H -- no --> J[resolve products list]
J --> K[create_release_context empty version]
I --> L[ListModule no products arg]
K --> M[ListModule products + limit]
L --> N[_list_version_details]
M --> O[_list_all_versions]
O --> P[list_all_versions]
O --> Q{default product?}
Q -- yes --> R[list_legacy_versions]
Q -- no --> S[skip legacy]
P & R --> T[merge_product_versions]
T --> U[apply_list_limit]
D & E & F & G --> V[create_release_context version + product]
V --> W[Module.execute fetch_all_release_metadata ctx.product.id]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[browseros release cmd] --> B{subcommand?}
B --> C[list]
B --> D[appcast]
B --> E[publish]
B --> F[download]
B --> G[github create]
C --> H{version arg?}
H -- yes --> I[create_release_context version + product]
H -- no --> J[resolve products list]
J --> K[create_release_context empty version]
I --> L[ListModule no products arg]
K --> M[ListModule products + limit]
L --> N[_list_version_details]
M --> O[_list_all_versions]
O --> P[list_all_versions]
O --> Q{default product?}
Q -- yes --> R[list_legacy_versions]
Q -- no --> S[skip legacy]
P & R --> T[merge_product_versions]
T --> U[apply_list_limit]
D & E & F & G --> V[create_release_context version + product]
V --> W[Module.execute fetch_all_release_metadata ctx.product.id]
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/browseros/bos_build/release/common.py:104-106
The comment "A partial listing renders as '(no releases found)'" is inaccurate. If names were already collected before the exception fires on a continuation page, those entries are returned — not an empty list. The comment would only hold if the first page itself raises.
```suggestion
except Exception as e:
# Partial listing: entries collected so far are returned; caller
# may see fewer versions than exist in R2.
log_warning(f"R2 listing failed for {prefix}: {e}")
```
### Issue 2 of 4
packages/browseros/bos_build/cli/release.py:189-195
**Redundant `_resolve_product` call and unused `ctx.product` in listing path**
When `product` is not `None`, `_resolve_product(product)` is called once to build the `products` list, and then a second time inside `create_release_context("", product=product)`. The resulting `release_ctx.product` is never accessed in the non-version listing path — `_list_all_versions` uses `self.products` from the `ListModule` argument, while `ctx.env` is the only field actually consumed from the context.
### Issue 3 of 4
packages/browseros/bos_build/release/list.py:98
`self.products or list(PRODUCTS.values())` treats an explicit empty list the same as `None` (unset). If a caller ever passes `ListModule(products=[])` intending to list no products, it silently falls back to all products. Using `if self.products is not None` is more explicit and guards the default correctly.
```suggestion
products = self.products if self.products is not None else list(PRODUCTS.values())
```
### Issue 4 of 4
packages/browseros/bos_build/cli/release.py:153-154
**`-n` is silently ignored when `--all` is combined with it**
`limit=None if show_all else limit` means `browseros release list --all -n 3` behaves identically to `--all` alone, with `-n 3` having no effect and no warning. Consider emitting a warning or making the combination an error, similar to how conflicting `version_arg`/`--version` is handled.
Reviews (2): Last reviewed commit: "fix(browseros): address review findings ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| """List all available versions""" | ||
| versions = list_all_versions(ctx.env) | ||
| """Print a bounded newest-first section per product""" | ||
| products = self.products or list(PRODUCTS.values()) |
There was a problem hiding this comment.
self.products or list(PRODUCTS.values()) treats an explicit empty list the same as None (unset). If a caller ever passes ListModule(products=[]) intending to list no products, it silently falls back to all products. Using if self.products is not None is more explicit and guards the default correctly.
| products = self.products or list(PRODUCTS.values()) | |
| products = self.products if self.products is not None else list(PRODUCTS.values()) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/release/list.py
Line: 98
Comment:
`self.products or list(PRODUCTS.values())` treats an explicit empty list the same as `None` (unset). If a caller ever passes `ListModule(products=[])` intending to list no products, it silently falls back to all products. Using `if self.products is not None` is more explicit and guards the default correctly.
```suggestion
products = self.products if self.products is not None else list(PRODUCTS.values())
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
--productthrough the whole release CLI (default browseros):create_release_contextnow resolves the product via the registry, and everyfetch_all_release_metadatacall site (list/appcast/download, joining publish/github) passesctx.product.id— BrowserClaw releases are no longer invisible.list_all_versions(release_prefix, env)listsreleases/<prefix>/, andlist_legacy_versionssurfaces pre-product barereleases/<version>/entries under browseros tagged(legacy)(dedupe prefers productized).browseros release listto the newest 5 versions per product (both products by default, in short sections) with-n/--limitoverride and--allfor everything;release list <version>keeps the product-aware artifact detail view.--list/--appcast/--publish/--download) into real typer subcommands:release list|appcast|publish|download, keepingrelease github create(+--product, long-only since-pstays--publishthere).if product.id == "browseros"branch inget_download_path_mapping— the alias map is built purely fromproduct.artifact_prefix, with a golden test pinning the browseros output byte-identical to the old constant.Design
Thin subcommands over product-parameterized helpers: the Step-module architecture stays (clean merge surface for the in-flight feed-publisher branch — appcast still only prints snippets, no feed publishing here). Listing splits into two composable R2 helpers sharing one paginated CommonPrefixes walker; merge/limit logic is pure and unit-tested without boto3. Legacy bare releases attach to the default product because they predate the registry; their detail view already works via
get_release_json's existing browseros fallback (untouched).Test plan
uv run python -m unittest discover -s bos_build -t . -p "*_test.py"— 384 tests OK (30 new: golden alias maps, prefix/delimiter/pagination listing, legacy shape filtering, merge/dedupe/limit, CLI surface incl. conflict + unknown-product errors)uv run ruff check bos_build— cleanuv run browseros release list --help/release --help/build --list/product doctor— all green… 112 more (use --all)hint, browserclaw section,(legacy)tags, legacy version detail fallback🤖 Generated with Claude Code