Repository navigation
chore(bos_build): remove stale product identity residue - #1537
Conversation
|
Claude (@claude) review |
|
Greptile (@greptileai) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 3s —— View job Code Review
Overall: LGTM with one minor observation. The PR is a clean cleanup — the deleted file is confirmed dead code, the test assertions match the actual patch contents, and the renames improve clarity. Deleted
|
| Test | Assertion | Verified |
|---|---|---|
test_mac_profile_root_comes_from_browseros_product_gn_arg |
BUILD.gn has BROWSEROS_PRODUCT_DIR_NAME=... define |
✓ line 28 |
app-Info.plist patch sets CrProductDirName to ${BROWSEROS_PRODUCT_DIR_NAME} |
✓ lines 19–20 | |
buildflags.gni if/else assigns BrowserClaw/BrowserOS |
✓ lines 37–40 | |
test_linux_profile_roots_are_product_specific |
#elif BUILDFLAG, browser-claw, browser-os in Linux paths patch |
✓ lines 17–21 |
test_windows_profile_roots_are_product_specific |
#if BUILDFLAG, both kProductPathName values in install modes |
✓ lines 21–24 |
Minor nitpick — the Linux and Windows tests use assertIn for each value independently, while the macOS buildflags test uses assertRegex to verify the full if/else structure. The assertIn approach proves the strings are present but not that they're in separate #if/#elif/#else branches. Not a bug (the patch file is what it is), but if a future patch accidentally duplicated a value outside the conditional, these tests would still pass. Consider using assertRegex with the surrounding conditional context for those two tests — similar to what's already done for buildflags.gni. Low priority.
Test renames
| Old name | New name | Assessment |
|---|---|---|
test_release_gn_args_bake_product_identity |
test_release_gn_args_select_product_for_baked_identity |
✓ accurate — describes the GN arg selection, not a removed stamping step |
test_browserclaw_helpers_use_product_identity |
test_browserclaw_helpers_use_product_package_names |
✓ accurate — the test checks package name values, not a product_identity abstraction |
Residue check
rg confirms zero remaining references to product_identity, product_dir_name, user_data_dir_name, product_path_name, or Info.plist.additions in bos_build/resources (the new test file itself only contains the new browseros_product_dir_name identifier, which is correct). ✓
Greptile SummaryThis PR cleans up residual
Confidence Score: 5/5Safe to merge — the deletion of Info.plist.additions is confirmed to have no remaining callers, and the new regression tests match the actual patch file content. The deleted plist file has zero remaining references in the repository, the new test file's assertions were verified against the actual chromium patch files on disk, and the two test renames are purely cosmetic. No logic paths are altered. No files require special attention. The new product_user_data_dir_test.py is the only substantive addition and its assertions are well-grounded in the existing patch files. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GN build arg\nbrowseros_product_dir_name] --> B{Product?}
B -->|browserclaw| C["BrowserClaw\n(buildflags.gni)"]
B -->|browseros| D["BrowserOS\n(buildflags.gni)"]
C --> E[macOS: app-Info.plist\nCrProductDirName = BrowserClaw]
D --> F[macOS: app-Info.plist\nCrProductDirName = BrowserOS]
C --> G[Linux: chrome_paths_linux.cc\ndata_dir_basename = browser-claw]
D --> H[Linux: chrome_paths_linux.cc\ndata_dir_basename = browser-os]
C --> I[Windows: chromium_install_modes.h\nkProductPathName = BrowserClaw]
D --> J[Windows: chromium_install_modes.h\nkProductPathName = BrowserOS]
K["❌ Info.plist.additions DELETED\n(was hardcoding CrProductDirName=BrowserOS)"] -.->|superseded by| E
K -.->|superseded by| F
style K fill:#ffcccc,stroke:#cc0000
%%{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[GN build arg\nbrowseros_product_dir_name] --> B{Product?}
B -->|browserclaw| C["BrowserClaw\n(buildflags.gni)"]
B -->|browseros| D["BrowserOS\n(buildflags.gni)"]
C --> E[macOS: app-Info.plist\nCrProductDirName = BrowserClaw]
D --> F[macOS: app-Info.plist\nCrProductDirName = BrowserOS]
C --> G[Linux: chrome_paths_linux.cc\ndata_dir_basename = browser-claw]
D --> H[Linux: chrome_paths_linux.cc\ndata_dir_basename = browser-os]
C --> I[Windows: chromium_install_modes.h\nkProductPathName = BrowserClaw]
D --> J[Windows: chromium_install_modes.h\nkProductPathName = BrowserOS]
K["❌ Info.plist.additions DELETED\n(was hardcoding CrProductDirName=BrowserOS)"] -.->|superseded by| E
K -.->|superseded by| F
style K fill:#ffcccc,stroke:#cc0000
Reviews (1): Last reviewed commit: "test(bos_build): normalize colored CLI h..." | Re-trigger Greptile |
Greptile SummaryThis PR removes stale
Confidence Score: 5/5Safe to merge — removes a dead file and adds regression test coverage with no logic changes to production code. The deleted Info.plist.additions has zero remaining references in the repository, confirming it was truly orphaned. The new patch-stack regression tests read live patch files and assert against content that is verified to exist. Test renames in the two existing files are purely cosmetic. No production logic is touched. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bos_build release] --> B{product?}
B -->|browseros| C[browseros_product_dir_name = BrowserOS]
B -->|browserclaw| D[browseros_product_dir_name = BrowserClaw]
C --> E[macOS: CrProductDirName via app-Info.plist]
D --> E
C --> F[Linux: data_dir_basename = browser-os]
D --> G[Linux: data_dir_basename = browser-claw]
C --> H[Windows: kProductPathName = BrowserOS]
D --> I[Windows: kProductPathName = BrowserClaw]
J[resources/entitlements/Info.plist.additions\nCrProductDirName = BrowserOS HARDCODED] -->|deleted - superseded| E
style J fill:#f88,stroke:#c00
style E fill:#8f8,stroke:#080
style F fill:#8f8,stroke:#080
style G fill:#8f8,stroke:#080
style H fill:#8f8,stroke:#080
style I fill:#8f8,stroke:#080
%%{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[bos_build release] --> B{product?}
B -->|browseros| C[browseros_product_dir_name = BrowserOS]
B -->|browserclaw| D[browseros_product_dir_name = BrowserClaw]
C --> E[macOS: CrProductDirName via app-Info.plist]
D --> E
C --> F[Linux: data_dir_basename = browser-os]
D --> G[Linux: data_dir_basename = browser-claw]
C --> H[Windows: kProductPathName = BrowserOS]
D --> I[Windows: kProductPathName = BrowserClaw]
J[resources/entitlements/Info.plist.additions\nCrProductDirName = BrowserOS HARDCODED] -->|deleted - superseded| E
style J fill:#f88,stroke:#c00
style E fill:#8f8,stroke:#080
style F fill:#8f8,stroke:#080
style G fill:#8f8,stroke:#080
style H fill:#8f8,stroke:#080
style I fill:#8f8,stroke:#080
Reviews (2): Last reviewed commit: "test(bos_build): normalize colored CLI h..." | Re-trigger Greptile |
…and stop advising a rebase that would destroy work (#369) For four rounds `why` warned that all remaining accepted branches conflict, that nothing can land and that the swarm will starve behind them. Mechanically true and useless: those branches will never land. Their base has moved past them, replaying one would delete work that landed since, and there was nothing in the loop that could say so. `salvage` has now measured what each still owes and filed it as a brief against today's base - Trinity-S-AI-DNA/trios#1537, browseros-ai#1538, browseros-ai#1539, browseros-ai#1540. So the branches are recorded as REPLACED, with the issue that replaces each: queen-1302 -> browseros-ai#1537 billing mode and quota authority queen-1303 -> browseros-ai#1538 the started_running state queen-1387 -> browseros-ai#1539 the heading-parity checker queen-1422 -> browseros-ai#1540 the noise filter that deleted its answer Five conflicting branches become one, and the one that remains - queen-1484 - is not debt either: it is an OPEN issue whose own criterion fails against the base today, which the swarm can pick up as ordinary work. RECORDED, NEVER INFERRED. Nothing here guesses that a branch is superseded. Someone says so and says by what, in a file, with a timestamp. The branches themselves are untouched: no deletion, no force, nothing irreversible. The selftest fails if `--force`, `branch -D` or `push --delete` ever appear in this file. AND THE ADVICE CHANGED, because it was wrong. For four rounds this tool told a reader that a conflicting branch "needs a rebase, or an honest closure as superseded". Rebasing browseros-ai#1302 would have deleted browseros-ai#1308's landed code and reintroduced a non-ASCII ellipsis into a redaction the base already performs in ASCII. It now says what was learned: A rebase is usually the WRONG remedy once the base has moved: replaying an old branch can delete work that landed since, and applying cleanly proves nothing, because a semantic conflict carries no markers. tri salvage <N> measures what the branch would still add, asks the issue its own criterion against the base, and writes the remainder as a brief A tool that recommends the destructive option in one line, every round, for four rounds, is worse than one that says nothing. selftest 160 pass 0 fail. Co-authored-by: Dmitrii Vasilev <trackgmedernj@hotmail.com>
…) (#372) The public research page shows the same paid worker slots /queen/status explains, but never said which quota gate they answer to, so an idle subscription swarm read as broken capacity. Add a billing block built from the exact resolver the status page publishes: billingMode plus quotaAuthority, one of two closed words - provider_quota under a Coding Plan, estimated_usd_gate under metered API work. An explanation, not a decision: queend still owns whether a Bee starts, and neither word can be read as an activity claim. Written against today's base: the capacity contract moved to the worker breakdown authority (browseros-ai#1308) and the tree loader learned failure states since the specification branch, so the capability is re-expressed for the deps that exist now, with TRIOS_SWARM_BILLING_MODE joining the cleared environment keys the default-resolver cases require. Co-authored-by: Trinity Bee <bee@trinity.local>
…pped connection, and I had seen it before (#373) THE SALVAGE LOOP CLOSED, and looking at it found the defect. Last round `salvage` measured what four stale branches still owed and wrote each as a brief. This round the swarm executed three of them. The audit: browseros-ai#1537 8 promised identifiers absent at the fork point, present on the branch, 6 in a declaration browseros-ai#1538 4 of 4, 3 in a declaration browseros-ai#1539 10 of 10, 9 in a declaration browseros-ai#1484 its own criterion failed at the fork point and passes on the branch Four for four, every one proven fail-to-pass. Measurement -> brief -> work -> audit -> land works end to end, and #370, #371 and #372 carry it into the base. BUT THEY HAD BEEN FINISHED FOR A ROUND AND NOBODY COULD SEE THEM. The Queen's own board said "completed" for browseros-ai#1538, browseros-ai#1539 and browseros-ai#1484 - dispatched, run for 9 to 15 minutes, 23 to 35 thousand output tokens each, reviewed and ACCEPTED - while no branch existed on the remote and no comment existed on the issue. `push-work` is the only way a bee's work leaves the container. It had died on `Operation timed out (os error 60)` from `railway ssh`, which is a network hop failing the way network hops fail. A retry thirty seconds later pushed thirteen branches. AND I HAD SEEN IT BEFORE. Two rounds ago the chain printed `push-work=FAILED`, I re-ran it by hand, it worked, and I wrote it off as transient without changing anything. "It worked when I tried again" is not a diagnosis. It is the observation that a retry belongs in the code, and skipping that step cost a round of invisible work. So the channel retries with backoff, and only for channel-shaped failures - "Operation timed out", "Connection reset", "SendRequest", ETIMEDOUT, 502. A git answer is an answer: `! [rejected] ... (non-fast-forward)` is not retried, because a partly-rejected push is a normal outcome the reporting already knows how to describe. The selftest pins both directions. AND A CHANNEL FAILURE MUST NOT LOOK LIKE "NOTHING TO PUSH". Both end without pushing anything and only one of them is fine. The chain reads this step's words, so the words differ: COULD NOT REACH THE CONTAINER after retrying. could not reach the container after 3 attempts: ... Nothing was pushed and nothing was inspected. This is NOT "nothing to push": this step is the only way a bee's work leaves the container, and it did not run. Exit 3, and never a stack trace. selftest 162 pass 0 fail. Co-authored-by: Dmitrii Vasilev <trackgmedernj@hotmail.com>
…ad been failing two thirds of the time (#374) I came to watch four salvage briefs through the swarm. They passed - and looking at them found something much larger. THE SALVAGE LOOP CLOSED. browseros-ai#1537, browseros-ai#1538 and browseros-ai#1539 were executed by bees and every promised identifier is absent at the fork point and present on the branch: 8 of 8, 4 of 4, 10 of 10, six to nine of them in a declaration. browseros-ai#1484's own criterion failed at the fork point and passes on the branch. Landed as #370, #371, #372. Measurement -> brief -> work -> audit -> land, proven end to end. BUT THEY HAD BEEN FINISHED FOR A ROUND AND INVISIBLE. The Queen's board said "completed" for three of them - dispatched, run 9 to 15 minutes, 23 to 35 thousand output tokens each, reviewed and ACCEPTED - with no branch on the remote and no comment on the issue. `push-work` had died on `Operation timed out (os error 60)`. A retry pushed thirteen branches. I HAD SEEN IT BEFORE AND FIXED NOTHING. Two rounds ago the chain printed `push-work=FAILED`, I re-ran it by hand, it worked, and I called it transient. "It worked when I tried again" is not a diagnosis; it is the observation that a retry belongs in the code. SO I ASKED THE LEDGER, for the first time in the whole run. Over 23 hours: reap 45 of 63 runs failed 71% lease 43 of 63 68% push-work 46 of 70 66% close-done 32 of 70 46% Those are exactly the four steps that FREE the swarm. All four reach the container through `railway ssh` and each carried its own six-line copy of the call. None of them retried. They had been failing about two thirds of the time for as long as the ledger goes back, and it was invisible because every run only ever reported itself - a single failure looks like bad luck, and only the record shows it is the system. `channel.mjs` is that call, once. It retries a CHANNEL failure - "Operation timed out", "Connection reset", "SendRequest", ETIMEDOUT, 502 - and never retries an ANSWER: `git push` exits non-zero when a ref is rejected, and a partly-rejected push is a normal outcome its caller already describes. Retrying an answer would be as wrong as crashing on a dropped connection, and the selftest pins both directions. Copying the push-work fix into three more files would have been L2 again: a rule transcribed four times is four rules that agree until someone edits one. It also marks the failure, so a caller can tell "I looked and found nothing" from "I never looked" - the same silence, completely different facts. AND THE LEDGER RECORDED NO REASON FOR ANY OF THEM. `line` is set only when a SUMMARY pattern matches, and no pattern matches a failure, so every one of the 46 FAILED entries carries an empty summary. The console had the reason. The record did not, and those failures can never now be diagnosed. Both chains keep the last six lines of a failing step from here on. `tri failures` reads the ledger backwards; `tri channel` says whether the one way in is up. Four `tri` commands were also passing their own name through as an argument, which is why `tri failures --since 1` looked for a step called "failures". selftest 166 pass 0 fail. Co-authored-by: Dmitrii Vasilev <trackgmedernj@hotmail.com>
Summary
product_identityoverride; the Python stamping step and descriptor fields are already goneresources/entitlements/Info.plist.additionsfile that still hardcodedCrProductDirName = BrowserOSproduct_identitystepTest plan
uv run python -m unittest bos_build.steps.patches.product_user_data_dir_test bos_build.steps.package.linux_test bos_build.core.context_test bos_build.core.planner_test bos_build.core.step_test bos_build.products.products_testuv run python -m unittest bos_build.cli.build_test.GnArgOptionTest.test_help_documents_repeatable bos_build.cli.dev_test.DoctorCliTest.test_help_documents_flags_and_read_only_guarantee bos_build.cli.release_test.ReleaseHelpTest.test_list_help_shows_bounding_optionsuv run python -m unittest discover -s bos_build -t . -p '*_test.py'uv run ruff check bos_buildrg -n "product_identity|product_dir_name|user_data_dir_name|product_path_name|Info\.plist\.additions" packages/browseros/bos_build packages/browseros/resources