schaefera/utStudyBIble
Watch
1
0
Fork
You've already forked utStudyBIble
 0
Code Issues 62 Pull requests Projects Releases Packages Wiki Activity Actions

Install: a prefix module owns its whole folder, so StrongsHebrew installs (#77) #78

Merged
claude-bot merged 1 commit from fix/strongs-datapath into main 2026-10-23 16:36:54 +00:00
Conversation 5 Commits 1 Files changed 2 +121 -4
claude-bot commented 2026-10-23 15:57:29 +00:00
Collaborator

Fixes #77

What

  • A file-prefix DataPath now owns every file in its folder when that folder is the module's own: at least 4 components deep and named as the stem, ASCII case-insensitive. StrongsHebrew's strongs.doc no longer makes check() refuse the package.
  • For such a prefix, remove_data deletes the whole folder, so #10 part 2b removes the extra files too. A prefix in a shared folder keeps the strict stem rule and the stem-file deletion.
  • DataPath::owns_its_folder() holds the rule. Its doc notes that a conf can already claim any folder via a Dir DataPath, so a hostile conf can name nothing new.

Blast radius / rollback

Trunk (sword/src/install*). To undo, revert this merge: no migration and no data format change.

Proof

  • Tests first: with a stub owns_its_folder() -> false, a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them and which_prefix_folders_are_the_modules_own failed. Both pass now.
  • Regression guards: another module's folder is still refused (3 stray shapes, nothing installed). Shared-folder prefixes stay strict, both directly under the driver and in a differently named folder.
  • Renamed test: remove_deletes_the_files_of_a_prefix_data_path moved to a shared folder, because in an own folder removal now takes everything.
  • Gates: cargo make is green. CI is green on dcca5e0: rust, preset-check, audit, app, e2e and click.
  • Not done: no e2e StrongsHebrew install, because there is no lexdict fixture. A unit test with the real package layout covers it.
  • Survey: real install() over 70 CrossWire rawzip packages (ignored test survey_refusals, UTSB_SURVEY_ZIPS=<dir>).
    • 68 install; 2 are refused, neither by the data-path rule.
    • The 2 refused are KLVen_iklingon and KLViklingon_en, which fail the separate conf-name rule. Neither is in a preset.
    • Now installing despite extra files: StrongsHebrew, Webster1913, PorLitCal2020, ZhEnglish, ZhHanzi, ZhPinyin and RusCARSDict.

Phone (docs/09 "On the phone")

Build dcca5e0 installed over WiFi. The owner confirmed that StrongsHebrew installs from "+" and shows as installed (comment 1848). The app can't use it yet; that comes with F2 (#42).

Resource budgets (docs/07)

No effect on memory or start-up work: the change is an install-time check only.

Review / audit

claude-review APPROVED dcca5e0 (review 260). Review-Confidence: 100 (comment 1852).

🤖 Generated with Claude Code

Fixes #77 ## What - A file-prefix `DataPath` now owns every file in its folder when that folder is the module's own: at least 4 components deep and named as the stem, ASCII case-insensitive. StrongsHebrew's `strongs.doc` no longer makes `check()` refuse the package. - For such a prefix, `remove_data` deletes the whole folder, so #10 part 2b removes the extra files too. A prefix in a shared folder keeps the strict stem rule and the stem-file deletion. - `DataPath::owns_its_folder()` holds the rule. Its doc notes that a conf can already claim any folder via a `Dir` DataPath, so a hostile conf can name nothing new. ## Blast radius / rollback Trunk (`sword/src/install*`). To undo, revert this merge: no migration and no data format change. ## Proof - **Tests first:** with a stub `owns_its_folder() -> false`, `a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them` and `which_prefix_folders_are_the_modules_own` failed. Both pass now. - **Regression guards:** another module's folder is still refused (3 stray shapes, nothing installed). Shared-folder prefixes stay strict, both directly under the driver and in a differently named folder. - **Renamed test:** `remove_deletes_the_files_of_a_prefix_data_path` moved to a shared folder, because in an own folder removal now takes everything. - **Gates:** `cargo make` is green. CI is green on dcca5e0: rust, preset-check, audit, app, e2e and click. - **Not done:** no e2e StrongsHebrew install, because there is no lexdict fixture. A unit test with the real package layout covers it. - **Survey:** real `install()` over 70 CrossWire rawzip packages (ignored test `survey_refusals`, `UTSB_SURVEY_ZIPS=<dir>`). - 68 install; 2 are refused, neither by the data-path rule. - The 2 refused are `KLVen_iklingon` and `KLViklingon_en`, which fail the separate conf-name rule. Neither is in a preset. - Now installing despite extra files: StrongsHebrew, Webster1913, PorLitCal2020, ZhEnglish, ZhHanzi, ZhPinyin and RusCARSDict. ## Phone (docs/09 "On the phone") Build dcca5e0 installed over WiFi. The owner confirmed that StrongsHebrew installs from "+" and shows as installed (comment 1848). The app can't use it yet; that comes with F2 (#42). ## Resource budgets (docs/07) No effect on memory or start-up work: the change is an install-time check only. ## Review / audit claude-review APPROVED dcca5e0 (review 260). Review-Confidence: 100 (comment 1852). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Install: a prefix module owns its whole folder (#77)
All checks were successful
CI / rust (pull_request) Successful in 8m7s
Details
Presets / preset-check (pull_request) Successful in 3m50s
Details
CI / audit (pull_request) Successful in 18s
Details
CI / app (pull_request) Successful in 3m54s
Details
CI / e2e (pull_request) Successful in 6m57s
Details
CI / click (pull_request) Successful in 2m38s
Details
dcca5e02f6 
StrongsHebrew's package ships strongs.doc beside its data, so check()
refused it. A file-prefix DataPath whose folder is the module's own
(modules/<cat>/<driver>/<name>/<stem>, folder named as the stem) now owns
every file under that folder, and remove deletes the folder. A prefix in a
shared folder keeps the strict stem rule. Adds an ignored survey test over
downloaded CrossWire packages.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review approved these changes 2026-10-23 15:58:20 +00:00
claude-review left a comment

Fresh-context review, round 1, trunk depth (sword/src/install*). Commit dcca5e02f6, range origin/main...HEAD (base 8e2ece7, head is a descendant of origin/main).
Files reviewed: sword/src/install/package.rs, sword/src/install/package/tests.rs

Verdict: APPROVED. No high or medium findings.

Ownership (install side):

  • DataPath::parse still rejects .., absolute paths and anything outside modules/ (Component filter), and requires at least 4 path parts. owns_its_folder() additionally needs the prefix dir to have 4 or more components and its last component to equal the stem (ASCII case-insensitive). So a prefix can only claim modules////, a folder a Dir DataPath could already claim at the same depth. Nothing is widened beyond what a conf could already name.
  • Shared-folder prefixes (directly under the driver, or a folder named otherwise) keep the strict parent + stem rule. Shared-folder case verified by a_prefix_in_a_shared_folder_keeps_the_strict_stem_rule.
  • Lookalike folders (strongshebrewx/) and sibling folders (other/) are refused because owns() uses Path::starts_with (component-wise) against the exact dir. Unpack only creates regular files (no symlinks), so remove_dir_all cannot follow a link out of the folder.

remove_data safety:

  • remove_dir_all runs only for Dir or an own-folder prefix; a shared driver folder (3 components, or a folder not named as the stem) always goes down the stem-file branch, so it is never deleted. remove_deletes_only_the_stem_files_of_a_prefix_in_a_shared_folder asserts the lookalike file is kept. The existing remove_refuses_a_conf_that_names_more_than_its_own_data still covers hostile confs.
  • Residual and accepted: a hostile conf for module A with DataPath .../rawld/b/b could delete module B's folder on removal. A Dir DataPath of the same shape already could, so this is not a new capability, and the doc comment says so.

Tests:

  • a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them and which_prefix_folders_are_the_modules_own fail without the change (the first on check() refusing strongs.doc, the second on the missing method). The refusal and shared-folder tests are regression guards that also pass on base, which is the right shape for them. I ran cargo test -p sword --lib install::package: 17 passed, 1 ignored.

Issue #77 items:

  • Survey: reported in the PR body (70 packages, 68 installed, 2 refused for the unrelated conf-name rule, neither in presets), and the repeatable ignored test survey_refusals is committed.
  • Unit proof, other-module refusal, removal of extras: met.
  • Golden/e2e StrongsHebrew install: not done, justified (no lexdict fixture in the e2e repo; the unit test uses the real layout).

Non-blocking notes (not defects):

  • CI on this head is still pending and the PR is draft. The merge gate still needs CI green on head.
  • The docs/09 phone check (StrongsHebrew from "+") is still to do before leaving draft, as the PR body states.
Fresh-context review, round 1, trunk depth (sword/src/install*). Commit dcca5e02f636f73a2967c80ca21ac7c173dabfe1, range origin/main...HEAD (base 8e2ece7, head is a descendant of origin/main). Files reviewed: sword/src/install/package.rs, sword/src/install/package/tests.rs Verdict: APPROVED. No high or medium findings. Ownership (install side): - DataPath::parse still rejects `..`, absolute paths and anything outside modules/ (Component filter), and requires at least 4 path parts. owns_its_folder() additionally needs the prefix dir to have 4 or more components and its last component to equal the stem (ASCII case-insensitive). So a prefix can only claim modules/<cat>/<driver>/<stem>/, a folder a Dir DataPath could already claim at the same depth. Nothing is widened beyond what a conf could already name. - Shared-folder prefixes (directly under the driver, or a folder named otherwise) keep the strict parent + stem rule. Shared-folder case verified by `a_prefix_in_a_shared_folder_keeps_the_strict_stem_rule`. - Lookalike folders (`strongshebrewx/`) and sibling folders (`other/`) are refused because owns() uses Path::starts_with (component-wise) against the exact dir. Unpack only creates regular files (no symlinks), so remove_dir_all cannot follow a link out of the folder. remove_data safety: - remove_dir_all runs only for Dir or an own-folder prefix; a shared driver folder (3 components, or a folder not named as the stem) always goes down the stem-file branch, so it is never deleted. `remove_deletes_only_the_stem_files_of_a_prefix_in_a_shared_folder` asserts the lookalike file is kept. The existing `remove_refuses_a_conf_that_names_more_than_its_own_data` still covers hostile confs. - Residual and accepted: a hostile conf for module A with DataPath .../rawld/b/b could delete module B's folder on removal. A Dir DataPath of the same shape already could, so this is not a new capability, and the doc comment says so. Tests: - `a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them` and `which_prefix_folders_are_the_modules_own` fail without the change (the first on check() refusing strongs.doc, the second on the missing method). The refusal and shared-folder tests are regression guards that also pass on base, which is the right shape for them. I ran `cargo test -p sword --lib install::package`: 17 passed, 1 ignored. Issue #77 items: - Survey: reported in the PR body (70 packages, 68 installed, 2 refused for the unrelated conf-name rule, neither in presets), and the repeatable ignored test `survey_refusals` is committed. - Unit proof, other-module refusal, removal of extras: met. - Golden/e2e StrongsHebrew install: not done, justified (no lexdict fixture in the e2e repo; the unit test uses the real layout). Non-blocking notes (not defects): - CI on this head is still pending and the PR is draft. The merge gate still needs CI green on head. - The docs/09 phone check (StrongsHebrew from "+") is still to do before leaving draft, as the PR body states.
claude-review commented 2026-10-23 15:58:49 +00:00
Collaborator

Review audit (fresh agent) for head dcca5e02f6, Review-Tier: trunk.

Review-Confidence: 80
Deductions:

  • −20 CI not green on the final commit: all 6 statuses (rust, audit, app, e2e, click, preset-check) are pending on dcca5e0.
    Missed defect: none found (riskiest hunk, remove_data/owns_its_folder in sword/src/install/package.rs, re-checked: the folder must be 4+ components deep and named as the stem; shared folders still go through the stem-file branch).
    Reason: the review is sound; re-run this audit once CI is green on head (the score should then be 100), and the docs/09 phone check is still open.
Review audit (fresh agent) for head dcca5e02f636f73a2967c80ca21ac7c173dabfe1, Review-Tier: trunk. Review-Confidence: 80 Deductions: - −20 CI not green on the final commit: all 6 statuses (rust, audit, app, e2e, click, preset-check) are pending on dcca5e0. Missed defect: none found (riskiest hunk, remove_data/owns_its_folder in sword/src/install/package.rs, re-checked: the folder must be 4+ components deep and named as the stem; shared folders still go through the stem-file branch). Reason: the review is sound; re-run this audit once CI is green on head (the score should then be 100), and the docs/09 phone check is still open.
claude-bot commented 2026-10-23 16:01:36 +00:00
Author
Collaborator

On the phone: build dcca5e0, installed over WiFi at 17:59. Please try it before this PR leaves draft (docs/09):

  • Open "+" and install StrongsHebrew. It finishes with no "not a usable module … outside the module's DataPath" error, and its row shows a tick.
  • StrongsGreek, Robinson and TSK still install, or still show as installed if you already have them.
  • A Bible still opens and reads as before.

There is no in-app removal yet (#10 part 2b). Removing the extra files is covered by the unit test a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them.

Gate: claude-review APPROVED dcca5e0. CI pending. I merge once CI is green and you have ticked the list above.

🤖 Generated with Claude Code

**On the phone:** build dcca5e0, installed over WiFi at 17:59. Please try it before this PR leaves draft (docs/09): - [ ] Open "+" and install **StrongsHebrew**. It finishes with no "not a usable module … outside the module's DataPath" error, and its row shows a tick. - [ ] StrongsGreek, Robinson and TSK still install, or still show as installed if you already have them. - [ ] A Bible still opens and reads as before. There is no in-app removal yet (#10 part 2b). Removing the extra files is covered by the unit test `a_prefix_module_may_ship_other_files_in_its_own_folder_and_removal_takes_them`. Gate: claude-review APPROVED dcca5e0. CI pending. I merge once CI is green and you have ticked the list above. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude-bot commented 2026-10-23 16:02:53 +00:00
Author
Collaborator

Phone check by the owner on dcca5e0: StrongsHebrew installs from "+" and shows as installed. The app can't use it yet; that comes with F2, the lexicon sheet in #42. I'll take this PR out of draft and merge it once CI is green.

🤖 Generated with Claude Code

Phone check by the owner on dcca5e0: StrongsHebrew installs from "+" and shows as installed. The app can't use it yet; that comes with F2, the lexicon sheet in #42. I'll take this PR out of draft and merge it once CI is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude-review commented 2026-10-23 16:36:15 +00:00
Collaborator

Review audit (fresh agent, re-run) for head dcca5e02f6, Review-Tier: trunk. Supersedes comment 1845.

Review-Confidence: 100
Deductions:

  • none. The only earlier deduction (−20, CI pending) is cleared: get_status on dcca5e0 shows state success, all 6 jobs green (CI / rust, Presets / preset-check, CI / audit, CI / app, CI / e2e, CI / click).
    Missed defect: none found (riskiest hunk, remove_data/owns_its_folder in sword/src/install/package.rs, unchanged since the earlier audit: the folder must be 4+ components deep and named as the stem; shared folders still use the stem-file branch). The reviewed commit equals head, and no code changed since.
    Reason: the review is sound and CI is green on head; the owner's phone check of StrongsHebrew is recorded in comment 1848.
Review audit (fresh agent, re-run) for head dcca5e02f636f73a2967c80ca21ac7c173dabfe1, Review-Tier: trunk. Supersedes comment 1845. Review-Confidence: 100 Deductions: - none. The only earlier deduction (−20, CI pending) is cleared: get_status on dcca5e0 shows state success, all 6 jobs green (CI / rust, Presets / preset-check, CI / audit, CI / app, CI / e2e, CI / click). Missed defect: none found (riskiest hunk, remove_data/owns_its_folder in sword/src/install/package.rs, unchanged since the earlier audit: the folder must be 4+ components deep and named as the stem; shared folders still use the stem-file branch). The reviewed commit equals head, and no code changed since. Reason: the review is sound and CI is green on head; the owner's phone check of StrongsHebrew is recorded in comment 1848.
claude-bot changed title from WIP: Install: a prefix module owns its whole folder, so StrongsHebrew installs (#77) to Install: a prefix module owns its whole folder, so StrongsHebrew installs (#77) 2026-10-23 16:36:37 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
claude-review
Labels
Clear labels   
priority
Could
 MoSCoW: nice to have, done when time allows

  
priority
Must
 MoSCoW: the release isn't viable without it

  
priority
Should
 MoSCoW: important, but the release works without it

  
priority
Won't
 MoSCoW: not this release (kept for later)

  
type
Bug

No labels
priority
Could
priority
Must
priority
Should
priority
Won't
type
Bug
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
schaefera/utStudyBIble!78
No description provided.