cmake: forward SEQUANT_ASSERT_BEHAVIOR to TiledArray and BTAS built from source

[evaleev]

Problem

SEQUANT_ASSERT_BEHAVIOR did not reach the dependencies SeQuant builds from source, so TiledArray and BTAS picked their own defaults (BTAS: throw whenever BUILD_TESTING=ON, regardless of NDEBUG) and the three libraries could disagree on what a tripped assertion does. Also, SeQuant's own default was THROW for every build type, unlike assert() and unlike TiledArray.

Change (three commits: BTAS pin, TiledArray pin, feature)

• SEQUANT_ASSERT_BEHAVIOR defaults to ABORT except for Release/MinSizeRel (IGNORE), the partition TA_ASSERT_POLICY uses.
• On the first configure of a build directory, FindOrFetchTiledArray.cmake seeds TA_ASSERT_POLICY from SEQUANT_ASSERT_BEHAVIOR if it is not in the cache yet (THROW/ABORT/IGNORE map one-to-one); TiledArray seeds BTAS_ASSERT_POLICY of the BTAS it builds the same way (cmake: seed BTAS_ASSERT_POLICY once from TA_ASSERT_POLICY tiledarray#588). FindOrFetchBTAS.cmake seeds BTAS_ASSERT_POLICY when SeQuant builds BTAS itself (SEQUANT_TILEDARRAY=OFF).
• This is the plain CMake idiom for a cached option: an explicit -DTA_ASSERT_POLICY=.../-DBTAS_ASSERT_POLICY=... (from the user or a parent project) is already in the cache and wins; a later change of SEQUANT_ASSERT_BEHAVIOR does not re-seed, exactly as a change of CMAKE_BUILD_TYPE does not re-default it (set the option explicitly, or use a fresh build directory). CMake cannot distinguish an explicit -D from a stale cache entry, so anything smarter (re-deriving on reconfigure, opt-out options, acknowledgment markers) can only approximate that and has corner cases; the review rounds on this PR explored that path and it was dropped in favor of seeding once.
• Pins: BTAS 52aadfe54 (BTAS_ASSERT_POLICY: real THROW / ABORT / IGNORE modes BTAS#188: BTAS_ASSERT_POLICY with THROW/ABORT/IGNORE, none NDEBUG-dependent); TiledArray 1654b57b7 (tiledarray#588: BTAS_ASSERT_POLICY seeded once from TA_ASSERT_POLICY). A BTAS older than Improve missing space instance error #188 ignores BTAS_ASSERT_POLICY and only knows the boolean BTAS_ASSERT_THROWS, which SeQuant does not touch.
• installing.rst and AGENTS.md describe the above in one paragraph each.

Verification (configure-only, TiledArray + BTAS from source, RelWithDebInfo, BUILD_TESTING=ON)

tree	step	extra -D	SEQUANT_ASSERT_BEHAVIOR	TA_ASSERT_POLICY	BTAS_ASSERT_POLICY
TA on	1	—	ABORT	ABORT (seeded)	ABORT (seeded by TA)
TA on	2	SEQUANT_ASSERT_BEHAVIOR=IGNORE	IGNORE	ABORT (cached, not re-seeded, as documented)	ABORT
TA on	3	TA_ASSERT_POLICY=TA_ASSERT_THROW	IGNORE	THROW (explicit)	THROW
TA on, fresh	1	SEQUANT_ASSERT_BEHAVIOR=IGNORE	IGNORE	IGNORE	IGNORE
TA on, fresh	1	TA_ASSERT_POLICY=TA_ASSERT_IGNORE	ABORT	IGNORE (explicit wins)	IGNORE
TA on, fresh	1	BTAS_ASSERT_POLICY=BTAS_ASSERT_IGNORE	ABORT	ABORT	IGNORE (explicit wins)
TA off	1	—	ABORT	—	ABORT (seeded)
TA off	2	SEQUANT_ASSERT_BEHAVIOR=IGNORE	IGNORE	—	ABORT (cached, as documented)
TA off	3	BTAS_ASSERT_POLICY=BTAS_ASSERT_IGNORE	IGNORE	—	IGNORE (explicit)
TA off, fresh	1	SEQUANT_ASSERT_BEHAVIOR=THROW	THROW	—	THROW

The "TA on" rows were measured against TiledArray 02d03c42c (tiledarray#587, whose BTAS forwarding still re-derived on reconfigure); with the pinned tiledarray#588 BTAS is seeded once and its column behaves like TA_ASSERT_POLICY's (row 3 would keep ABORT). No SeQuant-owned marker or option entries remain in any cache.

[Copilot]

🟡 Changes recommended

Moderate assertion-mapping and override-handling issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Aligns SeQuant assertion defaults with source-built TiledArray and BTAS, while updating configuration documentation.

Changes:

• Updates assertion defaults and dependency forwarding.
• Preserves explicit dependency overrides.
• Updates installation and contributor guidance.

File summaries

File	Summary
doc/user/getting_started/installing.rst	Documents assertion configuration behavior.
CMakeLists.txt	Updates assertion defaults.
cmake/modules/FindOrFetchTiledArray.cmake	Adds forwarding; moderate findings remain regarding IGNORE mapping and explicit policy overrides.
cmake/modules/FindOrFetchBTAS.cmake	Adds BTAS forwarding; a moderate finding remains regarding IGNORE mapping.
AGENTS.md	Updates assertion guidance.

Review details

Suppressed comments (1)

cmake/modules/FindOrFetchTiledArray.cmake:25

• When a parent project or user explicitly sets TA_ASSERT_POLICY, this block still derives BTAS from SEQUANT_ASSERT_BEHAVIOR. For example, TA_ASSERT_POLICY=TA_ASSERT_IGNORE with SEQUANT_ASSERT_BEHAVIOR=THROW leaves TA assertions disabled but makes BTAS assertions throw, so the documented override and TA/BTAS alignment are broken. Derive this value from TA_ASSERT_POLICY here, while retaining the independent BTAS_ASSERT_THROWS override guard.

        if (SEQUANT_ASSERT_BEHAVIOR STREQUAL THROW)
            set(BTAS_ASSERT_THROWS ON CACHE BOOL "Whether BTAS_ASSERT should throw")
        else ()
            set(BTAS_ASSERT_THROWS OFF CACHE BOOL "Whether BTAS_ASSERT should throw")

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues remain in assertion-policy forwarding and override handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

cmake/modules/FindOrFetchBTAS.cmake:48

• If only the documented deprecated override BTAS_ASSERT_THROWS is set, this condition turns following off but the message interpolates only BTAS_ASSERT_POLICY, which may be empty. Report both variables (or the variable that triggered the override) so the status message explains why following was disabled.

      message(STATUS "BTAS's assertion behavior was set explicitly (BTAS_ASSERT_POLICY=${BTAS_ASSERT_POLICY}): SEQUANT_BTAS_ASSERT_POLICY_FOLLOWS_SEQUANT turned OFF, it will no longer follow SeQuant's")

cmake/modules/FindOrFetchTiledArray.cmake:44

• The phrase “one an overridden ... points at” is ungrammatical; rephrase it so the source-selection caveat is clear.

    # forwarded as well, for a BTAS older than that (e.g. one an overridden
    # TA_TRACKED_BTAS_TAG or FETCHCONTENT_SOURCE_DIR_BTAS points at).

cmake/modules/FindOrFetchTiledArray.cmake:27

• FetchContent_MakeAvailable(TiledArray) below is allowed to satisfy FIND_PACKAGE_ARGS from an installed TiledArray, but these forwarding assignments run before that choice and are unconditional. In that case no source dependency consumes the values, while the FORCEd TA/BTAS cache entries still alter the parent project and the later ..._SEEN state. Restrict the forwarding and acknowledgement to the FetchContent source-build path so installed targets retain their own configured policy.

            set(TA_ASSERT_POLICY TA_ASSERT_${SEQUANT_ASSERT_BEHAVIOR} CACHE STRING "Controls the behavior of TA_ASSERT" FORCE)

cmake/modules/FindOrFetchTiledArray.cmake:62

• If only the documented deprecated override BTAS_ASSERT_THROWS is set, this condition turns following off but the message interpolates only BTAS_ASSERT_POLICY, which may be empty. Report both variables (or the variable that triggered the override) so the status message explains why following was disabled.

            message(STATUS "BTAS's assertion behavior was set explicitly (BTAS_ASSERT_POLICY=${BTAS_ASSERT_POLICY}): SEQUANT_BTAS_ASSERT_POLICY_FOLLOWS_SEQUANT turned OFF, it will no longer follow SeQuant's")

doc/user/getting_started/installing.rst:116

• The sentence uses there where the definite article the is required.

       N.B. a BTAS older than ``BTAS_ASSERT_POLICY`` (ValeevGroup/BTAS#188) has neither an abort nor an ignore mode -- there

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Lite

[evaleev]

Re the five suppressed comments of the last Copilot round: the two status-message ones and the shim-vs-policy one are resolved by dropping the BTAS_ASSERT_THROWS shim (32967a6); the two wording nits are gone with the rewritten docs; the installed-TiledArray one is addressed by probing find_package(TiledArray CONFIG QUIET) the way FetchContent's FIND_PACKAGE_ARGS will and skipping the forwarding (and the _SEEN marker) when it succeeds. PR body has the updated verification tables.

[Copilot]

🔵 Needs a closer look

The TiledArray discovery logic must preserve parent-selected FetchContent sources.

Review details

Suppressed comments (1)

cmake/modules/FindOrFetchTiledArray.cmake:32

• This pre-probe can bypass a parent project's FetchContent declaration. A parent may declare TiledArray with a custom SOURCE_DIR, GIT_TAG, or FIND_PACKAGE_ARGS without setting FETCHCONTENT_SOURCE_DIR_TILEDARRAY; if an installed package is found here, FetchContent_MakeAvailable later sees the target and never consumes that declaration. Please inspect existing FetchContent properties/declarations before probing, or only probe when this module owns the declaration, so a parent-selected source cannot be silently replaced by an installed TiledArray.

    if (NOT DEFINED FETCHCONTENT_SOURCE_DIR_TILEDARRAY AND NOT FETCHCONTENT_TRY_FIND_PACKAGE_MODE STREQUAL NEVER)
        find_package(TiledArray CONFIG QUIET COMPONENTS tiledarray)
        if (TiledArray_FOUND)
            set(_sequant_ta_from_source FALSE)
        endif()

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[evaleev]

Re the suppressed comment on the find_package pre-probe in FindOrFetchTiledArray.cmake: agreed, a probe ahead of FetchContent_MakeAvailable could hijack a parent project's own TiledArray declaration. c303f2d drops the probe and decides afterwards instead: TA_ASSERT_POLICY is forwarded unconditionally, and once FetchContent_MakeAvailable has run, tiledarray_SOURCE_DIR (set only when TiledArray was populated from source) selects between recording the acknowledged value and undoing the forwarding (TA_ASSERT_POLICY and the FOLLOWS option restored to their pre-forwarding cache state, marker dropped) when an installed TiledArray was used. Source-path verification (trees A/B in the description) re-run and unchanged; there is no installed TiledArray on this machine, so the restore branch is verified by reading, not by a configure.

[Copilot]

🔵 Needs a closer look

Two moderate cache-handling issues remain in the TiledArray integration.

Review details

Suppressed comments (2)

cmake/modules/FindOrFetchTiledArray.cmake:46

• On a build directory configured with the previous SeQuant, TA_ASSERT_POLICY is already cached by TiledArray, but SEQUANT_TA_ASSERT_POLICY_SEEN does not exist. The first configure after this change therefore takes this old dependency default as an explicit override and forces SEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT=OFF; for example, an existing RelWithDebInfo cache can remain TA_ASSERT_THROW instead of adopting the new SeQuant default TA_ASSERT_ABORT. Please add a migration path, or document a required cache reset/explicit policy, so upgrades do not silently retain the old mismatched default.

        if (DEFINED TA_ASSERT_POLICY AND NOT (DEFINED SEQUANT_TA_ASSERT_POLICY_SEEN AND TA_ASSERT_POLICY STREQUAL SEQUANT_TA_ASSERT_POLICY_SEEN))
            # explicit user value (on the first configure, or changed since SeQuant last saw it): honor it, stop following
            set(SEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT OFF CACHE BOOL "${_sequant_ta_follow_doc}" FORCE)
            message(STATUS "TA_ASSERT_POLICY=${TA_ASSERT_POLICY} was set explicitly: SEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT turned OFF, TA_ASSERT_POLICY will no longer follow SEQUANT_ASSERT_BEHAVIOR")

cmake/modules/FindOrFetchTiledArray.cmake:88

• When TiledArray is satisfied by an installed package, this branch restores the prior follow option only if it existed. On a fresh installed-package configure, option() above has already created SEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT in the cache, but _sequant_ta_follows_before is undefined, so the source-only option is left behind even though it has no effect for the installed target (and can leak into a later source reconfiguration). Unset it in the else branch so the installed path leaves the cache unchanged.

        if (DEFINED _sequant_ta_follows_before)
            set(SEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT "${_sequant_ta_follows_before}" CACHE BOOL "Derive TA_ASSERT_POLICY from SEQUANT_ASSERT_BEHAVIOR when TiledArray is built from source; OFF leaves TA_ASSERT_POLICY to the user (or to TiledArray's default)" FORCE)
        endif()
        unset(SEQUANT_TA_ASSERT_POLICY_SEEN CACHE)

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[evaleev]

Re the two suppressed comments of the last round (61a2f6e): (1) upgrade of an existing build directory: the cached pre-forwarding default is indeed taken as an explicit value on the first reconfigure (CMake cannot tell them apart, and keeping the cached value is the conservative choice); the status message now says so and names the fix, a single reconfigure with -DSEQUANT_TA_ASSERT_POLICY_FOLLOWS_SEQUANT=ON (same for BTAS), and installing.rst documents it. Simulated by stripping the marker from a configured cache and setting TA_ASSERT_POLICY=TA_ASSERT_THROW: the reconfigure reports it and turns following OFF (TA stays THROW), and the reconfigure with the option ON adopts TA_ASSERT_ABORT. (2) installed-TiledArray path: the FOLLOWS option that option() created is now unset in that branch, so the cache is left as it was.

[Copilot]

🔵 Needs a closer look

Address stale-cache handoff issues when switching between direct-BTAS and TiledArray modes.

Review details

Suppressed comments (2)

cmake/modules/FindOrFetchBTAS.cmake:31

• The reverse transition has the same stale-cache failure: after a TiledArray build, BTAS_ASSERT_POLICY remains cached but this module has no SEQUANT_BTAS_ASSERT_POLICY_SEEN marker because TiledArray records TA_BTAS_ASSERT_POLICY_SEEN. Switching to SEQUANT_TILEDARRAY=OFF therefore classifies TiledArray's derived value as an explicit BTAS override, turns this follows option OFF, and subsequent changes to SEQUANT_ASSERT_BEHAVIOR no longer update direct BTAS. Please recognize the TiledArray-derived marker or reset the value when taking ownership here, without clobbering a real user override.

  # SEQUANT_BTAS_ASSERT_POLICY_FOLLOWS_SEQUANT is the opt-out: while ON,
  # BTAS_ASSERT_POLICY is (re)derived from SEQUANT_ASSERT_BEHAVIOR on every
  # configure; an explicit BTAS_ASSERT_POLICY that differs from the value SeQuant
  # last acknowledged (recorded in SEQUANT_BTAS_ASSERT_POLICY_SEEN) is honored
  # and turns the option OFF. Turning it back ON resumes following.

cmake/modules/FindOrFetchTiledArray.cmake:16

• When an existing build directory is reconfigured from the direct-BTAS mode to SEQUANT_TILEDARRAY=ON, the previous direct path leaves BTAS_ASSERT_POLICY and SEQUANT_BTAS_ASSERT_POLICY_SEEN in the cache. TiledArray uses its separate TA_BTAS_ASSERT_POLICY_SEEN, so it treats that inherited policy as a new explicit override, disables TA_BTAS_ASSERT_POLICY_FOLLOWS_TA, and a later change to SEQUANT_ASSERT_BEHAVIOR can leave TA and BTAS on different policies. Please hand off or clear SeQuant-derived BTAS state when entering this path, while preserving an actual user override.

    # BTAS is not handled here: the tracked TiledArray builds BTAS itself and
    # forwards the effective TA_ASSERT_POLICY to it as BTAS_ASSERT_POLICY
    # (ValeevGroup/tiledarray#587, with its own TA_BTAS_ASSERT_POLICY_FOLLOWS_TA
    # opt-out), so an explicit or forwarded TA_ASSERT_POLICY reaches BTAS too.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[evaleev]

Re the two suppressed comments on switching SEQUANT_TILEDARRAY in an existing build directory (6a23c0c): each BTAS-building path now adopts the other's acknowledged value as its own marker when the cached BTAS_ASSERT_POLICY equals it, and drops the foreign marker, so following continues across the switch; a real override differs from the marker and is honored as before. Verified configure-only in one tree: TA on (BTAS derived ABORT) -> TA off (adopted, SEQUANT_ASSERT_BEHAVIOR=IGNORE then moves BTAS to IGNORE) -> TA on with THROW (TiledArray adopts, TA and BTAS both THROW; ABORT next) -> TA off with an explicit BTAS_ASSERT_POLICY=BTAS_ASSERT_THROW (honored, following OFF) -> IGNORE (BTAS stays THROW).

[Copilot]

🟡 Changes recommended

Two moderate issues remain in cross-configuration policy provenance handling.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[evaleev]

27ccc22 replaces all of the forwarding machinery (FOLLOWS options, acknowledgment markers, mode-switch handoff, installed-package restore) with the plain cached-option idiom: seed TA_ASSERT_POLICY / BTAS_ASSERT_POLICY from SEQUANT_ASSERT_BEHAVIOR only if not in the cache yet, no FORCE. An explicit -D wins and a later change of SEQUANT_ASSERT_BEHAVIOR does not re-seed, like any cached option; CMake cannot distinguish an explicit -D from a stale cache entry, so the removed machinery could only approximate that, and every approximation had a corner case. Description rewritten accordingly; the same reduction of TiledArray's BTAS forwarding follows in a separate TiledArray PR.

[Copilot]

🟢 Approval recommended

No unresolved review comments remain.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite