fix(settings): fail closed on uncertain targets - #121
Conversation
Independent QA auditBaseline/treatment harness was run outside the committed tests:
This confirms the fail-closed target behavior, but does not close #111. The original-source patch/edit requirement remains open because |
Teakowa
left a comment
There was a problem hiding this comment.
Full QA review
Blocker
- Applicability is still being upgraded without reviewed semantic evidence.
This PR correctly removes the direct dependency on GENERATED_HERO_SETTING_NAMES presentation data. However, it replaces that inference with two hard-coded semantic claims:
is_common_hero_setting()returnsApplicablefor a fixed key list whenever gameplay topology says the slot exists;hero_setting_applicability()hard-codesability1EnemyKb%as applicable only to Ashe.
Neither claim is accompanied by new Workshop semantic evidence or provenance in this PR. ADR-0006 at this exact head still says that the current projection has no complete independent applicability matrix, that a topology-valid target without explicit applicability evidence remains Unknown, and that locale/presentation quality never changes applicability. The ability1EnemyKb% generated entry is still a wildcard hero path with the presentation label Coach Gun Knockback Scalar Enemy; that presentation fact is not independent evidence that the setting is semantically Ashe-only.
So the previous presentation-derived overclaim has changed form, but the #109 contract is still violated. Either keep these targets Unknown, or add owner-reviewed semantic evidence/provenance that independently establishes the common controls and the exceptional hero applicability.
Major
read()now fails onApplicability::Unknowneven though no edit occurs. Bothread()andwrite()call the sameensure_target(), which now turnsUnknownintoApplicabilityUnknown. Failing closed is required for writes, but #111 also owns inspection/query behavior and explicitly preserves evidence-insufficient occurrences when they are not being edited. A typed read of an existing authored occurrence should not need to pretend applicability is known; it can return the authored value while leaving applicability queryable asUnknown, or otherwise expose that state without preventing inspection. Consider separating the read and write target gates rather than using one mutation-oriented rule for both.
Verified fixes
- locale presentation data is no longer used directly as applicability evidence;
Unknownwrites now fail closed with a structured error;- Team -> Hero and TeamAbility -> HeroAbility writes are rejected before mutating a broader source path;
- #111 remains open and the PR correctly does not claim the original-source patch/edit contract or downstream dogfood complete;
- CI on
6ea9749is green across stable/MSRV Rust quality, catalog check, and the five real-project scenarios.
Because the authenticated account is also the PR author, this is a COMMENT review rather than a formal request-changes state. The PR is not ready to merge while the Blocker above remains.
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up full QA review
Reviewed head 12f467d32af39df379d2e84b305c66d203857a6f against the full #109/#111 applicability and typed-read/write contract, not only the two prior findings.
Blocker
- The remaining wildcard Team/TeamAbility -> concrete Hero/HeroAbility applicability still upgrades
UnknowntoApplicableusing topology/team matching alone.
The new commit correctly restores Unknown for TargetPattern::HeroAbility -> SettingTarget::HeroAbility, but two earlier #119 changes remain:
TargetPattern::Team(_) -> SettingTarget::Hero { .. }returnsApplicablewhenever the team matches;TargetPattern::TeamAbility { .. } -> SettingTarget::HeroAbility { .. }returnsApplicablewhenever the team/slot match andhero_ability_exists(...) == Some(true).
Before #119, both concrete-narrowing cases intentionally returned Unknown when structural/topology checks passed. That matches ADR-0006: a topology-valid concrete hero target without explicit applicability evidence remains Unknown; topology proves that the hero has the slot, not that a wildcard Workshop setting is semantically available for that hero.
This matters beyond writes: applicability() is itself public semantic query output, and read() now correctly permits evidence-insufficient occurrences. Returning Applicable here therefore still overstates the owner-side semantic contract even though write_target_shape_matches() prevents the broader-path mutation.
Restore the pre-#119 evidence boundary for concrete narrowing (or add independently reviewed Workshop applicability evidence/provenance). If those cases return Unknown, consider evaluating TargetShapeMismatch before the write applicability gate if the API still wants that more specific write error; otherwise ApplicabilityUnknown is sufficient and the shape guard may be redundant for those branches.
Minor
- The PR body is now stale: it still says common hero controls use explicit topology/key evidence and that both
readandwriterejectUnknown. At this head, hero-specific applicability no longer uses that key list andread()intentionally acceptsUnknown. Update the description before merge so the review/delivery record matches the actual contract.
Verified fixes
GENERATED_HERO_SETTING_NAMESis no longer used as applicability evidence;- the hard-coded Ashe/common-key applicability introduced in the prior remediation is removed;
- hero-specific topology-valid targets now remain
Unknownwithout evidence; read()acceptsApplicability::Unknownwhilewrite()fails closed withApplicabilityUnknown;- Team -> Hero / TeamAbility -> HeroAbility writes still cannot mutate a broader source path;
- #111 remains correctly open for the original-source patch/edit contract and downstream consumer dogfood;
- CI on
12f467d3is green for stable Rust, Rust 1.85.0, Catalog check, and five real-project scenarios.
Because the authenticated account is also the PR author, this is a COMMENT review rather than a formal request-changes state. The PR is not ready to merge while the Blocker above remains.
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up QA — pass for this remediation
Reviewed exact head af74d1c6c5177b2be00a7a4cf176eeaa19669cf4 against the full prior findings and the applicable #109/#111 contract.
Findings
No Blocker, Major, or Minor findings in this head.
Verified
- Team -> Hero and TeamAbility -> HeroAbility concrete narrowing now remains
Unknownwhen only team/topology structure is known; topology is no longer promoted to applicability evidence. - Hero/HeroAbility applicability remains
Unknownwithout independent Workshop applicability evidence; the previous locale/presentation and hard-coded hero/key inference paths are gone. read()permits existing evidence-insufficient occurrences for inspection, whilewrite()rejectsUnknownbefore mutation withApplicabilityUnknown.- The previous broader-path write hazard remains closed because narrowed wildcard targets are now
Unknownand therefore fail the write gate beforeconcrete_path()/mutation. - Direct Team/TeamAbility targets with matching structural targets remain writable, so the fail-closed change does not unnecessarily disable known broad occurrences.
- The PR body now accurately states that this is remediation only: #111 remains open for the original-source patch/edit contract and real downstream consumer dogfood.
- CI for this exact head is green on stable Rust, Rust 1.85.0, Catalog check, and the five real-project scenarios.
Scope note
This pass applies to PR #121 only. The separate reopened #110 catalog-reconciliation issue is still unresolved on main: the duplicate projection validator does not yet compare enum-domain identity/presentation strongly enough. That is not introduced by this PR and should remain a release/Goal blocker until fixed and independently reviewed.
Because the authenticated account is also the PR author, this is recorded as a COMMENT review rather than a formal GitHub approval.
Summary
Refs #111. This review-remediation PR tightens the typed settings API around evidence-insufficient applicability and source-safe writes.
Current implementation:
Unknownwhen no independent Workshop applicability evidence exists;Unknownoccurrences for inspection;Unknown, before any mutation;Applicableonly for directly targeted Team and TeamAbility settings with matching structural targets.Scope boundary
This PR does not complete #111.
The original-source patch/edit contract remains open because
Settingscurrently carries a mutable settings tree plus spans, not the original source buffer and an applyable validated edit representation. Real downstream consumer dogfood is also still required before #111 can be closed.Numeric bounds remain unknown where the canonical catalog has no reviewed bounds. The documentation's synthetic
NumericBoundsexample demonstrates the API only and is not catalog evidence.Verification
Current head
af74d1c6c5177b2be00a7a4cf176eeaa19669cf4:cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace --all-targets(261 passed)cargo run -p workshop-rs --bin workshop-catalog-gen -- checkgit diff --check