Skip to content

fix(settings): fail closed on uncertain targets - #121

Merged
Teakowa merged 3 commits into
mainfrom
codex/issue-111-review-remediation
Aug 28, 2026
Merged

fix(settings): fail closed on uncertain targets#121
Teakowa merged 3 commits into
mainfrom
codex/issue-111-review-remediation

Conversation

@Teakowa

@Teakowa Teakowa commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Refs #111. This review-remediation PR tightens the typed settings API around evidence-insufficient applicability and source-safe writes.

Current implementation:

  • removes hero applicability inference from localized presentation/export data;
  • keeps topology-valid wildcard-to-concrete hero and hero-ability targets Unknown when no independent Workshop applicability evidence exists;
  • allows typed reads of existing Unknown occurrences for inspection;
  • rejects typed writes when applicability is Unknown, before any mutation;
  • retains Applicable only 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 Settings currently 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 NumericBounds example demonstrates the API only and is not catalog evidence.

Verification

Current head af74d1c6c5177b2be00a7a4cf176eeaa19669cf4:

  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace --all-targets (261 passed)
  • cargo run -p workshop-rs --bin workshop-catalog-gen -- check
  • git diff --check

@Teakowa

Teakowa commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Independent QA audit

Baseline/treatment harness was run outside the committed tests:

  • Baseline a7fd6cc failed to compile against the contract because SettingOperationError::ApplicabilityUnknown did not exist.
  • Treatment 6ea9749 passed: writing a hero setting for an unknown/future hero returns the structured ApplicabilityUnknown error.
  • Focused settings tests and Rust 1.85 clippy pass.

This confirms the fail-closed target behavior, but does not close #111. The original-source patch/edit requirement remains open because Settings currently carries a tree and spans, not the source buffer plus an applyable edit representation.

@Teakowa Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full QA review

Blocker

  1. 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() returns Applicable for a fixed key list whenever gameplay topology says the slot exists;
  • hero_setting_applicability() hard-codes ability1EnemyKb% 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

  1. read() now fails on Applicability::Unknown even though no edit occurs. Both read() and write() call the same ensure_target(), which now turns Unknown into ApplicabilityUnknown. 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 as Unknown, 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;
  • Unknown writes 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 6ea9749 is 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 Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The remaining wildcard Team/TeamAbility -> concrete Hero/HeroAbility applicability still upgrades Unknown to Applicable using 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 { .. } returns Applicable whenever the team matches;
  • TargetPattern::TeamAbility { .. } -> SettingTarget::HeroAbility { .. } returns Applicable whenever the team/slot match and hero_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 read and write reject Unknown. At this head, hero-specific applicability no longer uses that key list and read() intentionally accepts Unknown. Update the description before merge so the review/delivery record matches the actual contract.

Verified fixes

  • GENERATED_HERO_SETTING_NAMES is 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 Unknown without evidence;
  • read() accepts Applicability::Unknown while write() fails closed with ApplicabilityUnknown;
  • 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 12f467d3 is 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 Teakowa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Unknown when only team/topology structure is known; topology is no longer promoted to applicability evidence.
  • Hero/HeroAbility applicability remains Unknown without 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, while write() rejects Unknown before mutation with ApplicabilityUnknown.
  • The previous broader-path write hazard remains closed because narrowed wildcard targets are now Unknown and therefore fail the write gate before concrete_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.

@Teakowa
Teakowa merged commit 6af5f80 into main Aug 28, 2026
4 checks passed
@Teakowa
Teakowa deleted the codex/issue-111-review-remediation branch August 28, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant