chore: remove dead intrinsic surface and unify the ABI version default

[heifner]

Summary

Three related cleanups that surfaced while writing the migration guide (#111), plus the matcher defects found while auditing them.

1. Remove dead intrinsic surface — a security-group API and the legacy db_* declarations.
2. Unify the ABI version default behind one set of constants, and gate version-dependent sections consistently.
3. Make header staging prune, so a deleted header stops shipping.

1 — dead intrinsic surface

CDT declared intrinsics the chain does not export. A contract calling one compiles, links, and fails at deploy against the import allowlist.

Removed: the four security-group intrinsics, 60 stale db_* entries from the import allowlist, and — found in the last review round — set_kv_parameters_packed. (The db_* declarations were already gone from <sysio/db.h> on master; what survived was the allowlist.) The last one was found by comparing every sysio_wasm_import declaration under libraries/sysiolib/capi/sysio/ against the chain's 116 intrinsics in intrinsic_mapping.hpp: master carries 108 declarations, of which 5 have no chain counterpart; after this PR there are 103 and none is dead. wire-sysio mentions set_kv_parameters_packed only in a CHANGELOG.

imports/cdt.imports.in goes from 152 entries to 87 — exactly the 60 db_*, the four security-group names, and set_kv_parameters_packed. No removed name is exported anywhere in wire-sysio, so there are no false positives. Verified dead: zero security_group hits anywhere in wire-sysio; zero db_* names in the chain's genesis_intrinsics.cpp.

2 — ABI version handling

Four places derived the ABI version independently, and one of them read the version by taking the string's last three characters — so "sysio::abi/1.10" parsed as .10 → 1, and the version-gated diffs in cdt-abidiff silently skipped the variant and action-result sections for any two-digit minor.

abi_version::parse_version_string is now the single reader, comparing (major, minor) components. In cdt-abidiff those gates are gone entirely rather than fixed: every section is diffed unconditionally, because a section one document carries and the other does not is exactly the difference the tool exists to report, whatever version either side declares. The parse survives there for two other jobs — the version string is itself compared, and an unreadable one stops the run.

Section gating is consistent: variants from 1.1 and action_results from 1.2, both consulting the merged version. variants was previously emitted unconditionally, so a 1.0 document merged at 1.0 produced a 1.0 ABI carrying a variants array — contradicting the rule declared a few lines below it.

Each threshold is an explicit introduction pair of its own (variants_major/minor, action_results_major/minor). An earlier revision derived them from max_supported_major, which ties a fixed point in the format's history to the highest major this toolchain happens to accept: raising that maximum would have moved every introduction with it, so supports_variants(1, 10) would have turned false while parse() still accepted major 1 — letting valid 1.x documents omit sections they require. abigen reads the rule through supports_action_results; ABIMerger reads the same constants directly, because it needs the pair itself to promote a merged version.

cdt-codegen's protobuf branch stamped the CLI version unconditionally, downgrading a merged document whose descriptors declared something newer. It takes the newer of the two now. An assert beside it — added by an earlier commit on this branch, not inherited from master — was tautological and compiled away under the default Release TOOLS_BUILD_TYPE; it is gone.

No emitted ABI changes for existing contracts

All 16 tests/toolchain/abigen-pass/*.abi fixtures pin sysio::abi/1.2 and none is modified. No abigen-pass/*.json sets -abi-version, and add_contract never passes one, so wire-sysio's contracts stay on the default. The "four disagreeing defaults" all collapsed to 1.2 on master anyway — the float round-trip turned cdt-codegen's nominal 1.3 into 1.2 before it reached the plugin — so unifying on 1.2 is a no-op.

Two behaviour changes, both for versions that were previously emitted wrong: explicit -abi-version 1.3/1.4/1.10, and a populated version-gated section now promotes the emitted version.

Correcting an earlier version of this description, which said master discarded the variant definition at 1.0. It did not — master emitted variants unconditionally, so a contract with a std::variant parameter built at -abi-version 1.0 produced a 1.0 ABI carrying a 1.1 section: a version stamp contradicting its own content, not a dangling type reference. The section now promotes the document instead:

$ cdt-cpp -abigen -contract=vartest -abi-version 1.0 -o v.wasm v.cpp
version : sysio::abi/1.1
variants: [{"name": "variant_uint64_string", "types": ["uint64", "string"]}]

3 — matcher correctness

Auditing the version gates turned up six defects in the code they gate, tabulated below. All six are pre-existing, and several repeat the shape of one already fixed in the same file — which is why they were missed more than once.

Function	Defect
cdt-abidiff find_structs	Kept a success flag across the field loop and break-ed out without clearing it, so only a first-field change was ever reported. Seeded false and set only inside the loop, so two identical zero-field structs compared as different — and every parameterless action generates one, so the tool false-positived on essentially every real contract.
cdt-abidiff find_tables	Compared only name and type. index_type, key_names, key_types and table_id could all change with no difference reported — the metadata a contract upgrade turns on.
ABIMerger variant_is_same	Subset containment, no length check, so ["uint64"] and ["uint64","string"] compared equal. Merging them dropped the string alternative silently, or failed the build, depending on sorted .desc filename order.
ABIMerger struct_is_same	Matched fields by set membership plus size, so a struct declared with reordered fields merged as identical and the alphabetically-first descriptor won. ABI field order is serialization order.
cdt-abidiff print_clause	Read abi["clauses"] while find_clauses iterates abi["ricardian_clauses"], so the tool threw the moment it had a clause difference to report — it has never been able to report one.
cdt-abidiff find_enums / protobuf_types	Did not exist. An enum's values or an entire protobuf descriptor could change and the tool reported no difference.

table_is_same also never compared key_types, table_id or secondary_indexes; it now does, with the same empty-array tolerance already documented for key_names. tables_match likewise gained secondary_indexes — each secondary index carries its own table_id.

Optional keys. table_id and secondary_indexes are the Wire additions to table_def — index_type, key_names and key_types are standard Antelope fields, and an earlier version of this description wrongly grouped index_type with the extensions. A stock Antelope ABI carries neither addition; reading an absent key through jsoncons' const operator[] throws. Every optional field is read through a field_or_null accessor, so comparing against an upstream ABI works instead of aborting.

This corrects a diagnosis I gave in an earlier thread. I said the version parse was why cdt-abidiff reported no difference between ABIs whose table metadata had changed. It was not — find_tables never compared those fields.

4 — staging

Configure-time file(COPY) is additive and the ExternalProject's configure step is stamped, so a header deleted from a source tree stayed staged in <build>/include forever — shipped by install/CPack, and visible to compiles whose view then disagreed with the rebuilt library.

cmake/stage_cdt_tree.cmake runs as a build step and prunes first. Two corrections from review:

• It covered two of six staged trees. libc, libcxx, boost/preprocessor and bluegrass were still configure-time copies, keeping the exact bug the rework exists to fix. All six are pruned and recopied together now — verified by planting a file in each staged tree and confirming the staging step removes all five vendored ones plus sysiolib.
• Consumer ordering was a hand-maintained three-name allowlist around a step that REMOVE_RECURSEs a directory, missing sysio_malloc, sysio_dsm, sysio_cmem, c, c++, rt, sf and the native_* variants. It enumerates the directory tree instead.

Packaging: InstallCDT.cmake and package.cmake do change, for the native-component gating described below — but no archive moves between components. An earlier revision of this PR excluded libsf.a from the base install on the belief that it was native-only. It is not — it is the WebAssembly softfloat archive that cdt-ld links via -lsf for --use-rt and --fquery* (compiler_options.hpp.in:578), and it matches neither install component once excluded, so that would have shipped a sysroot unable to link a --use-rt contract. Reverted.

Tests

tests/unit/abidiff_tests.sh (60 assertions), tests/unit/abi_version_tests.sh (61) and tests/unit/staged_headers_tests.sh (28), all registered in tests/CMakeLists.txt; CLAUDE.md's list of shell-script tests is updated to match. Those counts grew across review: the three suites started at 36/53/5.

Every assertion pinning a fix was confirmed to fail against the pre-fix code. Some of the others are deliberate controls that pass on both sides — an earlier version of this description claimed every added assertion had been confirmed to fail first, which was not true, and the distinction is now stated rather than glossed:

• Reverting find_structs and find_tables fails 6 of the new cases — the non-first-field change, the identical zero-field struct, and all four table-metadata fields.
• Reverting variant_is_same and struct_is_same fails 3, and reproduces the order-dependence exactly: the variant case merged in one descriptor order and refused in the other.

Full validation at the current head

CLAUDE.md requires all three suites, with -DENABLE_INTEGRATION_TESTS=ON when a change can affect generated code, the ABI, or the runtime libraries. This one does, so that sweep was run and is recorded here rather than inferred from CI — which enables neither the integration option nor the sysio_DIR it needs, and so runs only the 30 default tests.

Run	Result
ctest — all labels, integration ON	31/31 (27 unit_tests, 3 toolchain_tests, 1 integration_tests)
ctest --test-dir tests/integration — per Boost suite	16/16
abidiff_tests.sh	60/60
abi_version_tests.sh	61/61
staged_headers_tests.sh	28/28

Built with clang-18, CMAKE_BUILD_TYPE=Debug, x64-linux-release triplets, against wire-sysio fix/opp-outpost-registry-followups @ 55e20cd4a7. Without ENABLE_INTEGRATION_TESTS the same tree is 30/30, which is the number CI reports; master is 28, and the delta is exactly the new shell tests.

The 16 integration suites are action_results, bls_primitives, capi, codegen, crypto_primitives, get_code_hash, hash_id, instant_finality, kv_cached, kv_global, kv_indexed_table, kv_scoped_table, kv_singleton, memory, multi_index and name_pk — i.e. the ABI, codegen and KV-runtime paths this PR touches, exercised against the real chain runtime.

Other behaviour changes worth knowing

• -abi-version is now validated. Master accepted 0.1 (→1.3), 1x (→1.0) and 1.2.3 (→1.2), and crashed on 2.0 with an uncaught jsoncons::key_not_found. All four are now refused with a diagnostic.
• The allowlist trim tightens the link, not just deploy. A hand-declared extern "C" db_store_i64 links on master and now fails with wasm-ld: undefined symbol: db_store_i64. A declaration carrying sysio_wasm_import emits an explicit import and still links either way.
• An empty variants array is omitted below 1.1, where master emitted "variants": []. No contract in this repo or wire-sysio is affected — all 199 emitted artifacts are byte-identical to master's.

Downstream verification

CLAUDE.md requires a change to the CMake package templates or the install layout to be verified against wire-sysio before it is called done. This PR changes InstallCDT.cmake, package.cmake, stage_cdt_tree.cmake and libraries/CMakeLists.txt, so that was done through the real consumer path rather than from the build tree:

1. cmake --install this branch to a clean prefix. The installed sysroot carries everything a contract links against — libsysio.a, libc.a, libc++.a, librt.a and libsf.a (the archive an earlier revision of this PR wrongly excluded), plus the staged include/sysiolib, include/libcxx (extensionless headers included) and include/libc trees.
2. Configure wire-sysio origin/master's contracts/ with find_package(cdt) + -DCMAKE_TOOLCHAIN_FILE=<prefix>/lib/cmake/cdt/CDTWasmToolchain.cmake and -DBUILD_SYSTEM_CONTRACTS=ON, exactly as cmake/contract-tools.cmake:19 does.
3. Build, and compare every artifact against the .wasm/.abi committed in wire-sysio, which were produced by a master-era CDT.

Result: 19/19 contracts byte-identical, wasm and abi. (Four contracts — sysio.authex, sysio.dclaim, sysio.system, sysio.tokens — could not be built in this harness because they include wire-sysio's own fc-lite/, generated types.pb.hpp and sysio/protocol/ headers, which live outside contracts/; that is a limitation of building that directory standalone, not a toolchain failure.)

CI's Verify packages job also passes, which exercises the deb/rpm layout this PR alters.

Not in this PR

The .gitignore hunk was byte-identical to #113's and conflicted with it; it lands once, on #113.

[huangminghuang]

I found three correctness gaps that the clean CI run does not exercise. Details are inline; the intrinsic/header removals themselves otherwise match the current host surface.

[huangminghuang]

[P2] Refresh the staged headers when removing these entries. libraries/native/CMakeLists.txt:367-369 and libraries/sysiolib/CMakeLists.txt:51 copy headers into build/include only during the stamped CDTWasmLibraries configure step; the inner reconfigure dependencies do not include these headers. Reusing a pre-PR build tree therefore rebuilds libnative.a from this shorter tuple and refreshes cdt.imports, but leaves the old four-entry-longer intrinsic table and both deleted security-group headers staged. Native consumers then compile against a tuple/enum layout that disagrees with the rebuilt library, and install/CPack still ships the removed API. Please make staging/cleanup a build dependency and cover upgrading an existing build tree.

[heifner]

Confirmed, and fixed here in 705b29f.

You were right about more than the mechanism: the two deleted headers were still staged in my own build tree after the rebuild that produced the "29/29 green" in the PR body. So the green run was against a partially stale staging area — thanks for catching it.

Staging moves out of the three configure-time file(COPY) calls into cmake/stage_headers.cmake, run from a stage_cdt_headers build target that prunes before it copies.

One thing worth flagging from doing it: the destinations overlap. sysiolib owns include/sysiolib, and native's second copy lands in include/sysiolib/native, inside it. Two independent prune+copy steps would race to delete each other's output, so it is one ordered script rather than a step per source dir. file(COPY) preserves source timestamps and skips files already current, so re-running it every build causes no downstream rebuild churn.

Verified the upgrade case specifically, since a green ctest does not demonstrate it — I planted both removed headers back into the existing tree and rebuilt without reconfiguring:

[3/17] Performing build step for 'CDTWasmLibraries'
[1/1] Staging CDT headers into .../cmake-build-debug-vcpkg/include
...
pruned: include/sysiolib/capi/sysio/security_group.h
pruned: include/sysiolib/contracts/sysio/security_group.hpp

CDTWasmLibraries-stamp/CDTWasmLibraries-configure was untouched throughout (dated 11 days earlier), so the configure step never re-ran — which is the case you described.

New tests/unit/staged_headers_tests.sh pins the invariant that every staged header has a source counterpart, so this catches any future stale staging, not just this deletion. It fails on the pre-fix tree and passes after.

[huangminghuang]

[P2] Finish replacing float-based version handling for every spelling accepted here. This parser accepts 1.10, but sysio_abigen_frontend_action::ParseArgs still uses stof/modf (plugins/sysio/abigen.hpp:1462-1467) and ABIMerger still applies stod to the final three characters (tools/include/sysio/abimerge.hpp:51-67). I reproduced HEAD parsing 1.10 as (1,10) and then merging it into sysio::abi/1.1, with action_results omitted. Please either restrict the grammar to one-digit minors or use structured integer parsing/comparison at those sites too, with a 1.10 regression test.

[heifner]

Confirmed and fixed in 705b29f — I reproduced your 1.10 trace exactly, and took the structured-parsing option rather than restricting the grammar.

Tracing it end to end, the mangling happened three times: the driver's parse() gave (1,10), the plugin's stof/modf turned the forwarded string back into (1,1) so the .desc said sysio::abi/1.1, and then ABIMerger read ".10" off the end as 0.10 — losing to 1.1 in merge_version and failing the >= 12 gate, which is where action_results disappeared.

The suffix trick also mis-read any major >= 10: "sysio::abi/10.2" -> "0.2". That is why I fixed the parsing rather than fencing the input.

• plugins/sysio/abigen.hpp uses abi_version::parse.
• ABIMerger stores the version it was constructed with and compares parsed components, via a new abi_version::parse_version_string that ignores the namespace prefix — so an inherited eosio::abi/1.2 descriptor parses like a sysio one.
• The gate is now abi_version::action_results_minor rather than a bare 12.
• The single-argument ABIMerger(ojson) ctor was dead (only the three-arg form is called, cdt-codegen.cpp:560), so it is removed — a merger can no longer exist without a known version.

Regression tests as requested: -abi-version 1.10 round-trips to sysio::abi/1.10, and a separate assertion that the emitted ABI still contains action_results — that second one is the case that fails on the old code.

No stof/modf/stod remains in the version path.

[huangminghuang]

[P2] Reject or correctly propagate major zero. parse() accepts 0.1, but cdt-cpp records and forwards a version only when opts.abi_version.first > 0 (tools/cc/cdt-cpp.cpp.in:69,153). Thus cdt-cpp -abi-version 0.1 silently falls back to 1.2 while standalone cdt-codegen --abi-version 0.1 uses 0.1, preserving the default divergence this change is intended to remove. Please reject major zero or track explicit option presence separately, and add a round-trip/rejection test.

[heifner]

Confirmed and fixed in 705b29f. parse() now rejects a zero major.

Your diagnosis was exactly right: first > 0 was doing double duty as "was the option given?", so -abi-version 0.1 took the fallback in the driver while cdt-codegen --abi-version 0.1 honoured it — the same divergence this PR set out to remove, just through a different door.

There is no ABI 0.x, so rejecting it at the parser is the honest fix, and it retires the sentinel: with a non-zero major guaranteed and a non-zero default, the two > 0 guards in cdt-cpp.cpp.in were dead and are gone. The version is now always recorded and always forwarded.

That last part does change sidecar content — abi_version is now always written, and cdt-ld requires sidecars to agree() on it. Same driver invocation writes them all, so they agree, and multidir_contract_tests (which is what would catch it otherwise) passes.

Rejection tests added for 0.1, plus 1.2.3 and 1x while I was there.

[huangminghuang]

[P3] Avoid /* inside this block comment. The abigen-pass/*.abi text triggers Clang's default -Wcomment in every translation unit that includes this header; spelling it as abigen-pass/<fixture>.abi avoids the new warning.

[heifner]

Fixed in 705b29f — respelled as tests/toolchain/abigen-pass/<fixture>.abi. Good catch; a warning in every TU that includes the header is not a fair trade for a glob in prose.

[huangminghuang]

[P2] Prune the native tree when native mode is disabled

Because include/sysio/native is removed only inside this truthy branch, reusing a build directory and switching the supported ENABLE_NATIVE_COMPILER option from ON to OFF leaves the old native headers behind. include/sysiolib/native happens to disappear when its parent is pruned, but InstallCDT.cmake installs the entire include tree, so an OFF build/package can still ship include/sysio/native. I reproduced the ON-to-OFF transition at this head, and the new staging test still passes because those files have source counterparts. Move the removal before the conditional and add transition/absence coverage.

[heifner]

Confirmed and fixed in 2377cb5. Both trees are now pruned unconditionally, before either is repopulated — include/sysio/native no longer depends on STAGE_NATIVE being on to be cleaned.

You were also right that the staging test could not catch it: its invariant is "no staged file without a source counterpart", and these files have counterparts — they simply should not be staged in that configuration. Those are orthogonal properties, so it needed a separate assertion rather than a stronger version of the same one. staged_headers_tests.sh now takes ENABLE_NATIVE_COMPILER and asserts the native trees are absent when it is off (and present when on).

[huangminghuang]

[P2] Make the standalone tarball target run staging

package-tgz in cmake/package.cmake is a standalone custom target with no dependency on CDTWasmLibraries or all. Now that pruning occurs only during this nested libraries build, cmake --build . --target package-tgz on a reused tree after a header deletion can invoke CPack without running stage_cdt_headers and ship the prior staged copy. The generated package and install targets are ordered after all, but this documented convenience target is not. Add a dependency on CDTWasmLibraries or explicitly stage before CPack, and cover the packaging-only path.

[heifner]

Confirmed and fixed in 2377cb5 — add_dependencies(package-tgz CDTWasmLibraries), with a comment recording why this target needs it when package and install do not.

This one only became reachable because of my own change: before, staging ran at configure time, so a standalone CPack run over a configured tree saw current headers. Moving it into the build step to fix the pruning is exactly what left this target behind. Good catch.

[huangminghuang]

[P2] Gate on the version that the merge actually emits

ret["version"] selects the maximum of the two documents, but this gate examines only the left-hand abi. A probe with ABIMerger(ojson{}, 1, 1) and a sysio::abi/1.10 descriptor emitted version 1.10 while dropping the descriptor action results; 1.1 plus 1.3 behaves the same way. Compute the merged/max version once and use it for both the emitted version and the capability gate, with a mixed-version regression.

[heifner]

Confirmed and fixed in 2377cb5. merge() computes merged_version once as the max of both sides and uses it for the capability gate, so it can no longer disagree with the version ret["version"] actually emits.

Worth noting the reachability, since it changes how much the regression is worth: after the protobuf-promotion fix below, the driver constructs ABIMerger with the same version it hands the plugin, and the accumulator starts empty and is seeded from it — so the two sides agree on every path cdt-cpp and cdt-ld drive, including multi-TU finalize. The divergence needs a merger constructed at one version and fed a descriptor stamped at another, which is your direct ABIMerger(ojson{}, 1, 1) probe rather than anything the CLI produces. I fixed it as a correctness invariant rather than a live bug, and did not add a shell regression for it because the driver cannot express the case; if you want it covered, it needs a host-side unit target for tools/include, which this repo does not currently have. Happy to add one if you think it is worth the scaffolding.

[huangminghuang]

[P2] Align accepted major versions with action-result serialization

This parser accepts every nonzero major, for example 2.0 or 10.2, and the changed merger treats those versions as newer than 1.2, but to_json() still serializes action_results only when version_major == 1. A non-void action built with -abi-version 10.2 therefore loses its result metadata. Either reject unsupported major versions here or use one shared supports_action_results(major, minor) predicate in the producer and merger, and assert the actual result entry in a higher-major test.

[heifner]

Confirmed and fixed in 2377cb5, taking the shared-predicate option and also bounding the major.

abi_version::supports_action_results(major, minor) is now the single rule, used by to_json, ABIMerger and cdt-abidiff. Alongside it, parse() rejects anything above max_supported_major (1): to_json only knows the 1.x shape, so accepting 2.0 or 10.2 meant stamping a version whose implied sections the toolchain would then omit. Rejecting is more honest than emitting a version we cannot honour, and it makes the predicate total rather than something each call site has to remember to pair with a major check.

-abi-version 2.0 and 10.2 are now in the rejection tests.

[huangminghuang]

[P2] Promote the effective protobuf version before descriptor generation

The abigen plugin has already run by the time this promotion happens. With --abi-version 1.1 plus protobuf files, the plugin suppresses action_results under its 1.2 capability gate; this code then stamps the incomplete output as 1.3 and cannot recover the omitted entries. Compute the effective protobuf version before gen_actions passes the version to the plugin, and add a 1.1 plus protobuf regression that checks a non-void action result.

[heifner]

Confirmed and fixed in 2377cb5, and the fix was one step further out than your comment describes — worth recording.

Computing the effective version before gen_actions is right, but that is not sufficient: gen_actions is only called when !finalize_mode, and cdt-ld runs the merge/stamp as a separate cdt-codegen --finalize invocation that forwards both --protobuf-files and --abi-version. Putting the promotion at the gen_actions call site left the finalize pass unpromoted, which the assertion I had added caught immediately on pb_tests. It now sits in main() right after argument parsing, so both passes settle on the same version.

Verified end to end rather than by reasoning:

$ cdt-cpp -abigen -abi-version 1.1 -contract pb_tests \
    -protobuf-dir ... -protobuf-files test.proto ... pb_tests.cpp
version: sysio::abi/1.3
action_results: [{"name": "hiproto", "result_type": "protobuf::test.ActResult"}]

Before the fix the plugin suppressed the section under its 1.1 gate, so the key was absent from an ABI stamped 1.3. Both assertions are now in abi_version_tests.sh.

[huangminghuang]

[P2] Route cdt-abidiff through the structured parser too

cdt-abidiff still parses abi["version"] with stod(ver.substr(ver.size() - 3)) * 10. For the newly supported and emitted sysio::abi/1.10, that returns 1, so its >= 11 and >= 12 guards skip variant and action-result diffs. Use this helper and compare (major, minor) components there as well, with a 1.10 regression.

[heifner]

Confirmed and fixed in 2377cb5. cdt-abidiff now uses abi_version::parse_version_string and the shared supports_variants / supports_action_results predicates instead of stod(ver.substr(ver.size() - 3)) * 10.

I had flagged this one myself while replying on #111 — the same suffix read was making cdt-abidiff report no difference between ABIs whose table metadata had changed — but scoped it out of this PR at the time. With 1.10 now a version the toolchain actually emits, it stopped being theoretical, so it is in.

New abidiff_tests.sh covers it: two ABIs differing by one action_results entry must report the difference at 1.2, 1.3 and 1.10 — the last fails on the old parser, which scored it 1 and skipped both gates — plus a version-only difference and a no-difference case.

[huangminghuang]

The four findings from my first pass are fixed, but this revision still has several version-capability and incremental-staging gaps; details are inline.

Please also refresh the PR description as required by this repository review-follow-up rules. It still describes the pre-follow-up state (19 parser checks and 29/29 overall) and omits the staged-header cleanup, the 1.10 and 0.1 behavior, the new staging test, and the current 30/30 validation.

[huangminghuang]

[P2] Do not turn failed document-version parses into ABI 1.2

parse_version_string() leaves its outputs untouched on failure, but this return value is ignored after initializing them to the 1.2 defaults. Because the shared parser rejects majors above 1, cdt-abidiff now treats sysio::abi/2.0 (and malformed or missing versions) as 1.2; I confirmed that comparing otherwise-identical 2.0 and 1.2 documents produces no version difference. Please diagnose a failed parse instead of laundering it to the emission default, and apply the same fix to ABIMerger::version_of().

[heifner]

Confirmed and fixed in 7a10f46. This was fallout from adding max_supported_major last round: parse_version_string leaves its outputs untouched on failure, and both call sites seeded them with the 1.2 defaults and then discarded the return value, so a version the parser now rejects read as the emission default.

cdt-abidiff diagnoses and exits non-zero — refusing to diff is right when every gate below keys off the value.

ABIMerger::version_of needed the two cases separated rather than the same treatment: a document with no version legitimately inherits the merger's own (that is how an empty accumulator is seeded), while a version that is present but unparsable is a malformed descriptor and now throws, consistent with add_object_to_array.

New abidiff_tests.sh case, and I checked it has teeth rather than assuming: reverting this fix plus the matcher below takes the suite from 7/7 to 5 passed, 2 failed.

[huangminghuang]

[P2] Normalize the CMake boolean passed to the staging test

The stage target canonicalizes this option with $<BOOL:...>, but this test passes the raw cache spelling and the shell recognizes only ON, on, and 1. With the valid setting -DENABLE_NATIVE_COMPILER=TRUE, staging correctly creates both native trees while this test takes its OFF branch; I reproduced the resulting 2-pass/1-fail run. Please pass $<BOOL:${ENABLE_NATIVE_COMPILER}> here (or implement the full CMake boolean vocabulary in the script).

[heifner]

Confirmed and fixed in 7a10f46 — the test now takes $<BOOL:${ENABLE_NATIVE_COMPILER}>, the same canonicalization the stage target uses, and the script compares against 1.

Verified the expression is actually evaluated rather than passed through: the generated CTestTestfile.cmake now shows "1" where it previously showed the raw ON, so TRUE/YES/Y all reduce the same way by $<BOOL:> semantics.

Teaching the shell CMake's boolean vocabulary was the other option; matching the producer's canonicalization seemed better than maintaining a second copy of that vocabulary.

[huangminghuang]

[P2] Treat missing versioned sections as empty during mixed-version merges

The new merged-version gate now calls merge_action_results() for a 1.1 accumulator plus a 1.10 descriptor, but a valid 1.1 document omits action_results and add_object_to_array() indexes both documents unconditionally. I reproduced ABIMerger(old_1_1_without_results, 1, 1).merge(new_1_10) throwing Key 'action_results' not found, so the exact newer-right-hand-side case this change intends to fix fails instead of retaining the result. Please normalize a missing capability section to an empty array in both merge directions and add a sequential 1.1→1.10 regression.

[heifner]

Confirmed and fixed in 7a10f46 — and you are right that this failed the exact case the round-2 gate change was meant to enable.

Reproduced end to end before fixing, through cdt-codegen --finalize with a hand-written 1.1 descriptor (no action_results) and a 1.10 one:

terminate called after throwing an instance of 'jsoncons::key_not_found'
  what():  Key 'action_results' not found

no ABI emitted. With the fix: version sysio::abi/1.10, and the newer side's result entry retained.

Fixed in add_object_to_array via a section() helper rather than at the call site, so it covers every versioned section in both merge directions rather than just action_results. The 1.1 → 1.10 regression is now in abi_version_tests.sh, driven through --finalize — the real ABIMerger entry point, since no cdt-cpp invocation can produce mixed-version descriptors.

[huangminghuang]

[P2] Fix result matching before routing 1.10 through it

This new gate sends 1.10 documents into find_action_results(), but that matcher compares the matched entry against abi2["action_results"].at(i) rather than .at(j). Compiling the current source and comparing two semantically identical 1.10 ABIs whose two result entries are merely reversed reports all four </> entries as changes; unequal lengths can also throw. Please fix the matching index and add a reordered-input regression so enabling the 1.10 path does not create false diffs.

[heifner]

Confirmed and fixed in 7a10f46 — .at(i) → .at(j), with a comment recording why.

One clarification on scope: this is pre-existing, and it is not limited to the newly enabled path. The matcher has always compared against the wrong index, so reordered results have been mis-reported at 1.2 and 1.3 as well; what this PR changed is only that 1.10 now reaches it too. I fixed it here rather than splitting it out, since routing more traffic through a broken matcher without fixing it would be worse.

Regression added, and confirmed to fail first: two 1.10 ABIs whose two identical results are merely reversed report all four entries as changed on the pre-fix binary, and report nothing after. My existing cases used differing single entries and could never have caught it.

[huangminghuang]

[P2] Remove stale native archives before packaging an OFF rebuild

This dependency runs the nested build, but reconfiguring a reused tree from native ON to OFF only removes the native targets; it does not delete the archives they previously copied into ${CMAKE_BINARY_DIR}/lib. InstallCDT.cmake still picks up stale libnative*.a for dev/TGZ and stale libsf.a for base/TGZ. In an existing tree, libnative.a still contains all four security-group symbols removed by this PR, so the documented upgrade/package path can ship the old implementation even after this dependency runs. Please prune those outputs when native mode is off (or make installation configuration-aware) and cover the ON→OFF package payload.

[heifner]

Confirmed and fixed in 7a10f46, and I agree this one mattered most — it meant the documented upgrade path could ship the very API this PR deletes.

Two layers:

• stage_cdt_tree (renamed from stage_headers, since it no longer stages only headers) prunes lib/libnative* and lib/libsf.a when native mode is off. That target already runs at build time and already prunes the header trees for exactly this reason, so it is the same mechanism rather than a new one.
• The libnative* install in InstallCDT.cmake is now guarded on ENABLE_NATIVE_COMPILER, so a stale file cannot be packaged even if something reintroduces one.

Verified the prune against a scratch tree seeded the way an earlier ON build leaves one:

before: libc.a libnative.a libnative_sysio.a libsf.a
after:  libc.a
native header trees: 0

libsf.a was the easy one to miss — it goes to the base component, not dev, so it would have shipped in the plain package too.

staged_headers_tests.sh now asserts those archives are absent when native is off, alongside the header trees.

[huangminghuang]

[P2] Assert the action-result entry, not the action name

"name": "hiproto" is present in the top-level actions array even when the old late-promotion path suppresses action_results, so this assertion passes on the exact regression it is meant to catch. Please scope the check to .action_results[] and verify both name == "hiproto" and result_type == "protobuf::test.ActResult" (or at least match that unique result_type).

[heifner]

You are right, and this is the worst of the six: the assertion passed on the exact regression it was written to catch, so my "verified end to end" claim last round was worth less than I said.

Confirmed against the built ABI — hiproto appears in both arrays:

actions:        [{"name": "hiproto", "type": "protobuf::test.ActData", ...}]
action_results: [{"name": "hiproto", "result_type": "protobuf::test.ActResult"}]

Fixed in 7a10f46 to assert "result_type": "protobuf::test.ActResult", which occurs only under action_results.

I applied the same check to the new mixed-version merge test while writing it — its first draft asserted "name": "actb", which is in the actions array too, so it would have had the identical hole. That one now asserts result_type as well.

[huangminghuang]

Re-reviewed current head 55e77c0. The six issues from the prior round appear substantively fixed, and all required Ubuntu/macOS/package checks are green.

I am still requesting changes for the six line-level issues posted here: mixed-version ABI merging throws when the older side lacks action_results; failed document-version parses are silently coerced to 1.2; the protobuf promotion regression assertion is a false positive; ON→OFF builds can still package stale native archives (including the removed symbols); valid CMake true spellings make the staging test fail; and the newly enabled 1.10 action-result diff path produces false differences for reordered equivalent entries.

Local validation included git diff --check, shell syntax checks, a direct STAGE_NATIVE=TRUE reproduction, and compiled probes against the current ABIMerger and cdt-abidiff sources.

[huangminghuang]

All six findings from the previous pass are addressed, and the current head is green on Ubuntu, macOS, and package verification. This pass found two reproducible ABI correctness regressions plus one unexercised state-transition fix, detailed inline.

Please also refresh the PR description before the next review. It still says the follow-up covers two review rounds and reports 28 abi_version_tests.sh assertions; the current branch includes a third review round and 30 assertions, plus the strict abidiff parse rejection, mixed-version merge handling, reordered action-result fix, ON-to-OFF archive pruning, canonical CMake boolean, and corrected protobuf assertion.

[huangminghuang]

[P2] Keep required descriptor sections mandatory

section() is used by the generic add_object_to_array(), so this now treats types, structs, actions, tables, and ricardian_clauses as optional too. Those baseline arrays are always emitted by abigen; I reproduced a 1.2 descriptor missing actions being accepted and emitting "actions": [], whereas the old indexing failed with Key 'actions' not found. A truncated descriptor can therefore silently lose contract interface content. Limit the empty fallback to explicitly optional/version-gated sections (such as action_results) and add a missing-required-section regression. Please exercise both merge orders as well: cdt-codegen sorts the descriptor paths, so the current new.desc/old.desc fixture actually processes the newer file first.

[heifner]

Confirmed and fixed in dd9ccdf. You are right that section() over-reached: it backs the generic add_object_to_array, which handles eight sections, so making the fallback unconditional turned the baseline arrays optional along with the version-gated ones.

section() now takes a section_kind, defaulting to required; only action_results, variants and enums — the sections that entered the format at a version — pass version_gated. A descriptor missing a required section throws again, with a clearer message than the raw jsoncons one.

Good catch on the merge order too: cdt-codegen.cpp:553 does std::sort(desc_files...), so filenames rather than --desc-file order decide the accumulator, and my new.desc/old.desc fixture always processed the newer first. The test now copies the descriptors to explicit a_first/b_second names and runs both directions.

Both new assertions confirmed to fail first: reverting the change accepts the truncated descriptor and emits "actions": [].

[huangminghuang]

[P2] Fix variant matching before enabling this path

This gate now routes 1.10 documents through find_variants(), but that matcher unconditionally sets found = true after breaking on a type mismatch and does not compare the list lengths. At this head, two 1.10 ABIs differing only by types: ["uint64"] versus ["string"] produce no output and exit 0; ["uint64", "string"] versus ["uint64"] throws Invalid array subscript. Compare the complete arrays (including size) before marking a same-name variant as found, and add non-empty 1.10 variant regressions.

[heifner]

Confirmed and fixed in dd9ccdf — and this one is on me beyond the finding itself. I fixed find_action_results last round and did not check its sibling, which had the same defect in a slightly different shape.

find_variants broke out of the element loop on a type mismatch and then set found = true unconditionally, so a same-named variant counted as unchanged however its types differed; and with no length comparison, at(k) threw on a shorter right-hand side. It now compares the whole list, length first.

Three regressions added, all confirmed to fail before the fix:

FAIL: 1.10 variants differing by type report a difference          (reported nothing)
FAIL: 1.10 variants of differing length are compared without throwing
PASS: identical 1.10 variants report no difference

Pre-existing, as with the action-result matcher — it affects 1.1 upward — but this PR is what routes 1.10 into the path, so it belongs here.

[huangminghuang]

[P2] Exercise the reused-tree OFF cleanup in CI

This registers the staging test only for the current configure mode. ENABLE_NATIVE_COMPILER defaults to ON and neither CI workflow overrides it, so the new OFF assertions for native headers and archives never execute. Even adding a clean OFF build would not prove the ON-to-OFF prune, because that tree contains no stale native outputs. Seed an isolated staging tree with native header/archive sentinels, invoke stage_cdt_tree.cmake with STAGE_NATIVE=0, and assert they are removed (plus ON population if useful); otherwise deleting the new OFF cleanup still leaves every current check green.

[heifner]

Confirmed and fixed in dd9ccdf, and your point about a clean OFF build is the part I had missed: it would not prove the prune either, because it has no stale outputs to remove.

The test now does exactly what you describe, and what I had only done by hand last round — seed an isolated tree with native archive and header sentinels the way a previous ON build leaves one, invoke stage_cdt_tree.cmake directly with STAGE_NATIVE=0, and assert they are gone. It also asserts an unrelated libc.a survives, so the prune is targeted rather than a wipe.

Being mode-independent, it runs in every configuration rather than only when the build happens to be configured OFF:

-- ON -> OFF prune (isolated tree) --
  PASS: STAGE_NATIVE=0 prunes stale native archives and header trees
  PASS: the prune leaves unrelated archives alone

The mode-specific assertions stay as well, so an OFF build still checks its own staged tree.

[huangminghuang]

The three findings from the previous pass are fixed: required baseline sections are rejected again, the 1.10 variant matcher handles type and length differences, and the mode-independent ON-to-OFF staging probe exercises the real cleanup. The current Ubuntu, macOS, and package checks are green.

Four remaining blockers are detailed inline. Please also clean up the documentation before the next review: the PR introduction still says two review rounds, Review round 1 names the superseded stage_headers.cmake / stage_cdt_headers, and the footer refers to a nonexistent emplace guard. Separately, cmake/CDTMacros.cmake.in still uses the removed db_store_i64 surface as its example of a native-module intrinsic; use a current kv_* intrinsic or a generic description.

[huangminghuang]

[P2] Reject versionless descriptors instead of defaulting them

The constructor already seeds an actually empty accumulator with version at lines 22–30, so a nonempty document reaching version_of() without this key is an external malformed descriptor, not the accumulator case described above. At this head, merging a descriptor containing every array but no version succeeds and emits sysio::abi/1.2; the base implementation rejected it when merge_version() indexed the missing key. Throw for a missing document version and add a merger regression, rather than silently stamping the command default.

[heifner]

Confirmed and fixed in bdbb798. You are right that the accumulator justification does not hold: the constructor seeds an empty accumulator with a version (lines 22-30) and merge() always stamps ret["version"], so every document reaching version_of() legitimately carries one. The fallback was unreachable for valid input and fired only on a malformed external descriptor — where it stamped the emission default onto input the base implementation rejected.

It now throws Error, ABI is missing its version, alongside the existing rejection for a version that is present but unparsable.

Worth naming the shape of my mistake, since it repeats in the next thread: I was fixing a too-strict failure (the Key not found throw) and over-corrected into a too-permissive one, in both cases by defaulting instead of rejecting.

[huangminghuang]

[P2] Require gated sections once the document version supports them

version_gated currently means “optional at every version.” A malformed 1.10 descriptor missing action_results or variants is therefore accepted and normalized to an empty array, even though supports_action_results(1, 10) / supports_variants(1, 10) say that version carries the section and abigen emits both keys. I reproduced both cases at this head. Permit absence only when that specific document predates the section (before 1.2 for action results, before 1.1 for variants); keep independently optional enums separate, and add supported-version missing-section rejection tests.

[heifner]

Confirmed and fixed in bdbb798. version_gated meaning "optional at every version" was too weak, exactly as you say.

I checked which sections abigen actually emits before choosing the thresholds:

Section	abigen	Rule now
variants	unconditional (abigen.hpp:1070)	required at >= 1.1
action_results	only when the version supports it (:1076)	required at >= 1.2
enums	only when non-empty (:1108)	optional everywhere — no threshold

So sections now carry the version they entered at, and absence is permitted only when the document's own version predates it. enums is kept separate as independently optional, per your note. A 1.10 descriptor missing action_results or variants is rejected with the version named in the message.

Because the two sides of a merge can declare different versions, the check is made per document rather than once for the pair — section() takes that document's parsed version.

[huangminghuang]

[P2] Seed the older-first merge at ABI 1.1

These two filename orders still do not exercise a missing section on the accumulator side because neither invocation passes --abi-version 1.1. The empty merger is seeded at the 1.2 default, so after processing old.desc first its intermediate ABI is already 1.2 with an empty action_results key. In both named cases the only missing key encountered is therefore the old descriptor on the right; a one-sided RHS fallback passes both while the original old-LHS failure remains. Run these finalize cases with a 1.1 seed (or add a direct host test) so older-first genuinely creates a 1.1 accumulator without the section.

[heifner]

Confirmed and fixed in bdbb798 — and this is the one I would have missed, because the test looked like it covered both orders.

You are exactly right about the mechanism: without --abi-version the merger is seeded at the 1.2 default and the constructor gives it an empty action_results, so the only absent key was ever the descriptor on the right. Both named cases would have passed a right-side-only fallback.

Both cases now seed at 1.1, which makes the first merge emit an intermediate carrying no action_results at all, so the second merge meets the missing section on the left.

Verified rather than assumed — with the left-side fallback removed:

FAIL: older-first: descriptors merge
PASS: newer-first: emits the newer version
PASS: newer-first: retains the newer side's action_result

That is the asymmetry the old fixture could not see.

[huangminghuang]

[P2] Do not mask failures in valid-input abidiff cases

cdt-abidiff returns 0 after reporting ordinary differences; the comment at lines 52–54 claiming otherwise is incorrect. The repeated || true discards real process failures, so this identical-variant case—and the quiet/equivalent cases at lines 90 and 110—can count a crash or generic runtime error as PASS whenever its text lacks the searched keyword. Assert exit status 0 for every valid-input invocation before checking output, and reserve expected nonzero handling for the unsupported-version case.

[heifner]

Confirmed and fixed in bdbb798, and the comment you flagged was simply wrong — I checked:

$ cdt-abidiff a.abi b.abi   # ABIs differing in action_results
exit code with differences = 0

So || true was not redundant-but-harmless, it was hiding real failures: any crash or rejection whose output lacked the searched keyword counted as a pass.

Valid-input invocations now go through a helper that fails the case outright on a non-zero exit before any output matching, and only the unsupported-version case expects non-zero. That also simplified the differing-length variant case, which had been sniffing for Invalid array subscript in the text — a throw now surfaces as a non-zero exit and is reported as such.

[huangminghuang]

The foreign-namespace and abi_extensions regression cases are now pinned, and current-head CI is green. I still cannot approve this revision: the protobuf normalizer introduces a reproducible false-negative, and the native-disabled probe does not yet prove that the archive is built and staged. Details are inline.

Please also refresh the full PR description and contradictory source comments, as required by this repository's review-follow-up rules. The body still describes the removed abidiff capability gates, claims thresholds key off max_supported_major, repeats the corrected discarded-variant history, calls standard index_type Wire-only, and reports 36/53/5 assertions instead of 50/59/15; it also omits the latest softfloat and protobuf changes. Stale source claims remain in abimerge.hpp:100-104, abi.hpp:63-66, the abidiff test preamble, stage_cdt_tree.cmake:65-69, staged_headers_tests.sh:303-306, and the removed db_store_i64 example in CDTMacros.cmake.in:185.

[huangminghuang]

[P2] Canonicalize with FileDescriptorSet semantics

Generic JSON parsing accepts roots that Wire's protobuf parser rejects and does not normalize protobuf-JSON equivalents. With this head, cdt-abidiff is silent for an absent protobuf_types field versus the nonempty string "null"; the chain retains that string and JsonStringToMessage rejects it with "Root element must be a message", while absence is valid. A raw array versus a string containing that array is similarly collapsed despite different chain behavior. Conversely, numeric and symbolic protobuf-JSON spellings that deserialize to the same FileDescriptorSet still report a difference. Canonicalize through FileDescriptorSet using the chain's parse options and compare a deterministic protobuf representation; at minimum, only adopt a parsed string when its root is an object and add invalid-root controls.

[heifner]

Reproduced both:

absent vs "null"                 : collapsed (wrong)
raw [1,2] vs string "[1,2]"      : collapsed (wrong)

c3be9d7a adopts a parsed string only when its root is an object, which is what JsonStringToMessage requires. "null" and "[1,2]" stay the strings they are and therefore differ from absence and from a raw array respectively. Controls for both; reverting the restriction fails them.

On the rest: full canonicalisation through FileDescriptorSet would mean linking protobuf into cdt-abidiff, which only cdt-codegen does today. Rather than imply an equivalence the tool does not implement, the comment now states the limit — two spellings that decode to the same FileDescriptorSet but differ as JSON are still reported as different. It errs toward reporting a difference that is not one, never toward hiding one. Happy to do the protobuf-level version if you would rather take the dependency.

[huangminghuang]

[P2] Execute the native-disabled target, not just list it

Ninja's target inventory proves only that libsf.a is declared. It still lists a target or subdirectory marked EXCLUDE_FROM_ALL, and it does not execute the POST_BUILD copy into out/lib. Either change would leave this probe green while a default native-disabled build/package still lacks the staged archive. There is no OFF matrix leg despite the comment above. Build sf or the default graph in this scratch configuration and assert the resulting out/lib/libsf.a exists (and is Wasm), alongside the absence of native-host archives.

[heifner]

Fair — listing proves declaration, not reachability, and never runs the POST_BUILD copy.

c3be9d7a builds the default graph in the scratch OFF tree and asserts out/lib/libsf.a exists and holds WebAssembly objects. It costs about five seconds, since those objects are already in the compiler cache from the main build, so a full OFF matrix leg is not needed for this property. Re-gating add_subdirectory(native) fails it.

Two harness bugs surfaced while doing it, both mine: the probe did not create out/lib, so cmake -E copy wrote a file named lib; and find | head -1 took SIGPIPE, which under set -o pipefail exited the suite mid-run with 141 rather than failing a case. -print -quit now.

[huangminghuang]

[P2] Match the native C++ archive too

This character class excludes plus signs, so it does not match the real libnative_c++.a target. Accidentally ungating only the native libc++ target therefore passes the claimed no-native assertion. Match the complete libnative* archive surface rather than only lowercase letters and underscores.

[heifner]

Correct — [a-z_]* excludes +, so libnative_c++.a never matched. Confirmed against the real build output:

libnative.a  libnative_c++.a  libnative_c.a  libnative_rt.a  libnative_sysio.a

c3be9d7a matches the full archive surface.

[huangminghuang]

[P2] Require staged files in both native trees

The new loop verifies only that each directory exists. If a staging-pattern regression creates the destinations but copies no headers, the package has no native API while this assertion passes; removing every file from both directories still leaves the suite green. Require a nonzero file count in each destination, as the test already does for the other staged trees.

[heifner]

Right. c3be9d7a requires a nonzero file count in each destination rather than the directory existing, matching what the other staged trees already assert.

[huangminghuang]

The four findings from the previous pass now reproduce as fixed: the abidiff suite passes 52/52, the staging suite passes 17/17, the clean native-OFF build stages a WebAssembly libsf.a without native archives, and current-head CI is green. Two remaining coverage/correctness issues and one diagnostic issue are inline.

Before the next review, please also complete the repository's required follow-up validation and documentation:

• Run and record all three suites at the current head with ENABLE_INTEGRATION_TESTS=ON. CLAUDE.md requires that sweep for ABI/runtime changes; the workflows currently run 31 default tests without enabling integration tests, and the documented downstream run excludes four contracts.
• Refresh the PR description and stale source comments to describe the current implementation. The body still documents the removed capability gates and max_supported_major behavior and reports old test totals (36/53/5 rather than 52/59/17). The staging comments at tests/unit/staged_headers_tests.sh:230-231,343-346 and cmake/stage_cdt_tree.cmake:65-69 likewise still say the OFF configuration does not rebuild libsf.a, although this head now builds it.

[huangminghuang]

[P2] Preserve protobuf differences hidden by duplicate JSON members

ojson discards duplicate object members while parsing both the outer ABI and this embedded object. As a result, current-head cdt-abidiff reports no difference between a raw protobuf_types value containing "file":[{"name":"a.proto"}],"file":[{"name":"b.proto"}] and a string value containing only "file":[{"name":"b.proto"}]. Wire's FC parser preserves both members, and protobuf 33.4 merges duplicate repeated fields, so the runtime sees valid descriptor sets [a.proto,b.proto] and [b.proto]. This contradicts the guarantee above that this normalization can only create false positives. Please either reject duplicate members explicitly or canonicalize with the runtime's FileDescriptorSet semantics, and add this pair as a regression test.

[heifner]

Reproduced, and you are right that it falsifies the guarantee I wrote — this is an under-report, not an over-report.

The vendored jsoncons has no option to reject duplicates at parse time, and canonicalising through FileDescriptorSet means linking protobuf into cdt-abidiff, which only cdt-codegen carries. So fc675ede takes the other option you offered: a document with duplicate object members is refused at load, via a SAX pass tracking the keys of each open object.

$ cdt-abidiff dup.abi onlyb.abi
Error: file1 has a duplicate object member 'file'. jsoncons keeps only the last, while the
chain preserves both and protobuf merges duplicate repeated fields -- so any
comparison here would describe a value the runtime never sees.
exit=1

It applies to the whole document, not just protobuf_types, since the blindness was never specific to that section. All 44 real ABIs in the tree are still accepted. The comment now states this exception instead of claiming a guarantee the tool cannot make.

It also immediately found two malformed fixtures of mine — mk_legacy already emits "types", so passing it again duplicated that member.

[huangminghuang]

[P2] Assert native headers are absent in this clean OFF build

This probe checks only ${OFFDIR}/out/lib, so it does not pin the CMake-to-staging wiring for native headers. I forced the stage command in libraries/CMakeLists.txt to pass STAGE_NATIVE=1; all 17 assertions still passed while the clean OFF output contained four files under each of out/include/sysio/native and out/include/sysiolib/native. Those trees would be packaged because InstallCDT.cmake installs the entire include tree. Please assert both native include directories are absent after this nested OFF build.

[heifner]

Confirmed by forcing STAGE_NATIVE=1 exactly as you describe — 17/17 green with four files under each native tree.

fc675ede asserts both out/include/sysio/native and out/include/sysiolib/native are absent after the nested OFF build. With the flag forced it now fails, naming both:

FAIL: an OFF build stages no native header tree
      staged: .../out/include/sysio/native (4 files)
      staged: .../out/include/sysiolib/native (4 files)

[huangminghuang]

[P3] Do not let a missing directory abort the test harness

With set -euo pipefail, find returns nonzero when $d is absent, so this assignment terminates the script before missing_native is populated, the named failure is printed, later probes run, or the Results line appears. Removing include/sysiolib/native reproduces that silent early exit. Check -d first or otherwise make the pipeline safe; the same pattern at line 105 should be fixed as well.

[heifner]

Correct, and the irony is that the abort happened precisely in the case the check exists to report — a missing directory produced no failure line and no Results line at all.

fc675ede adds a count_files helper returning 0 for an absent path, used at both sites. Removing include/sysiolib/native now yields:

FAIL: both native header trees are staged and non-empty (native enabled)
      empty or absent: .../include/sysiolib/native (0 files)
Results: 17 passed, 1 failed

exit 1 rather than a silent 141. That is the third set -e pipeline hazard in this test file; I have stopped using find | head anywhere in it.

[huangminghuang]

fc675ede fixes the raw-object duplicate case and both staging findings from the previous review. I reproduced 53/53 abidiff assertions and 18/18 staging assertions, and current-head CI is green. One supported protobuf spelling still bypasses the duplicate check, and two packaging regressions remain unpinned; details are inline.

The two previously requested follow-ups also remain outstanding:

• Please run and record the current-head sweep with ENABLE_INTEGRATION_TESTS=ON. CLAUDE.md requires all three suites for ABI/runtime changes; CI still runs only the 31 default tests, and the recorded integration runs predate the later ABI, codegen, runtime, and staging changes.
• Please refresh the PR description and contradictory source comments after this follow-up commit. The body still describes removed capability gates and max_supported_major behavior and reports 36/53/5 tests rather than the current 53/59/18. Staging comments still say the OFF configuration does not rebuild libsf.a, although it now does.

[huangminghuang]

[P1] Scan duplicate members inside the supported string spelling

load_checked() scans only the outer ABI JSON, so duplicate members inside a string-valued protobuf_types are opaque to its SAX handler. This parse then drops the earlier member: on the current head, a string containing {"file":[{"name":"a.proto"}],"file":[{"name":"b.proto"}]} compares equal to a string containing only b.proto (no output, exit 0). JsonStringToMessage accepts the first as two descriptors, a.proto and b.proto, so this is the same runtime-visible false negative the new detector is meant to prevent. Run duplicate detection on text before parsing/returning it (or canonicalize through FileDescriptorSet) and add the string-form regression.

[heifner]

Confirmed and fixed in 5578e17. Reproduced against the built binary before touching anything:

$ cdt-abidiff pb_dup_str.abi pb_onlyb.abi   # duplicate inside the STRING
exit=0                                       # no output

$ cdt-abidiff pb_dup_obj.abi pb_onlyb.abi   # the same duplicate as an object
Error: file1 has a duplicate object member 'file'. ...
exit=1

Exactly the false negative the detector exists to prevent, in the one place its walk cannot see: to the SAX reader that spelling is a single opaque string value.

Fixed at load, not at the comparison, so a document is refused on the same terms whichever spelling it uses — load_checked parses, and when protobuf_types is a string, runs first_duplicate_member on the string's own text as well:

if (parsed.is_object() && parsed.count(protobuf_types_key)) {
   const ojson& pb = parsed.at(protobuf_types_key);
   if (pb.is_string()) {
      const std::string nested = first_duplicate_member(pb.as<std::string>());
      if (!nested.empty())
         refuse_duplicate(which, nested, " inside protobuf_types");
   }
}

Not canonicalising through FileDescriptorSet: the tool does not link protobuf, which is the same reason canonical_protobuf stops at JSON-level canonicalisation. The refusal message and the protobuf_types key moved into file-local helpers so both paths share one wording, and the note in canonical_protobuf now says the refusal covers either spelling.

Three regressions in abidiff_tests.sh, via a shared expect_refused (an inline copy per spelling is how one of them stops being checked): the object form; your string form; and a duplicate-free protobuf_types string still comparing equal to its object counterpart — so the fix cannot be "refuse every string". Deleting the nested check fails only the new row (54/55). 55/55 with it.

[heifner]

Correcting one line of the reply above: "refuses the document on the same terms as the object spelling" was not true of what I pushed, and 482e2ed fixes it.

A pre-push audit caught that the nested check refused on any duplicate in the string, which is stricter than the comparison it protects. canonical_protobuf adopts the parsed value only for an object root — anything else is returned as the string node and compared verbatim, which is what the pre-existing "a raw array differs from a string containing that array" assertion pins. So for a non-object root jsoncons drops nothing from the compared value, and the refusal's own reason does not hold:

# before 482e2edbb
$ cdt-abidiff arrdup.abi arrother.abi     # protobuf_types: "[{\"a\":1,\"a\":2}]"
Error: file1 has a duplicate object member 'a' inside protobuf_types. ... rc=1

# after -- compared verbatim, as it was before this PR
< protobuf_types
"[{\"a\":1,\"a\":2}]"
> protobuf_types
"[{\"a\":9}]"
rc=0

The gate asks canonical_protobuf whether it would adopt rather than restating the rule, so the refusal rule and the adoption rule cannot drift apart:

if (pb.is_string() && canonical_protobuf(parsed).is_object()) {

The object-root string is still refused, unchanged. The regression I had added alongside it — a duplicate-free string against its object counterpart — turned out to duplicate an assertion four lines up, so it is replaced by the case that earns its place: a duplicate inside a non-object-root string still compares, and reports the difference. Removing the gate fails it (54/55); 55/55 with it.

[huangminghuang]

[P2] Assert the required non-native headers exist in the OFF output

This clean OFF build now proves native headers are absent, but it never proves the required header trees were staged. Fault-injecting removal of the OFF sysiolib, libc, libcxx, boost/preprocessor, and bluegrass outputs still lets the default build and all 18 assertions pass, yielding a package without its public headers. Please require each non-native destination under ${OFFDIR}/out/include to be nonempty here, so the OFF CMake-to-staging path is covered positively as well as negatively.

[heifner]

Agreed — that probe was all negatives. Fixed in 5578e17.

The per-destination check is now a helper, require_non_native_dests <include root> <label>, run against both the live build tree and ${OFFDIR}/out/include, over one named list:

readonly NON_NATIVE_DESTS=(sysiolib libc libcxx boost/preprocessor bluegrass)

which the sentinel-planting loop also walks, so the hardcoded expected 5 is gone with it — a tree added to stage_cdt_tree.cmake and missed in one of the three places is now the kind of gap the others can report.

It asserts a non-zero file count rather than directory existence, since a staging regression that creates the destinations and copies nothing would otherwise stay green.

Fault-injected exactly as you describe: wrapping the sysiolib/libc/libcxx/boost/preprocessor/bluegrass copies in if(STAGE_NATIVE) — a regression gated on the OFF branch, invisible to the ON-configured live tree — gives

FAIL: sysiolib is staged (native disabled)
FAIL: libc is staged (native disabled)
FAIL: libcxx is staged (native disabled)
FAIL: boost/preprocessor is staged (native disabled)
FAIL: bluegrass is staged (native disabled)
Results: 18 passed, 5 failed

with the original 18 all still green — so it is the OFF staging path those rows pin and nothing else. 23/23 unmutated.

[huangminghuang]

[P2] Seed every archive the ON-to-OFF prune must remove

A real native-enabled build stages five host archives: libnative.a, libnative_sysio.a, libnative_c.a, libnative_c++.a, and libnative_rt.a. This probe seeds and checks only the first two. Restricting stage_cdt_tree.cmake to remove exactly those two still leaves all 18 assertions green while the other three stale archives would survive and be packaged after an ON-to-OFF reconfigure. Seed and assert removal of the complete libnative* surface, especially the +-bearing name.

[heifner]

Correct — five archives, and the probe seeded two. Fixed in 5578e17.

One list now drives both the seeding and the assertion:

readonly NATIVE_ARCHIVES=(libnative.a libnative_sysio.a libnative_c.a libnative_c++.a libnative_rt.a)

matching the POST_BUILD copies in libraries/{native,sysiolib,libc,libc++,rt}/CMakeLists.txt. Seeding and asserting from the same list is what stops them drifting apart again.

Narrowing stage_cdt_tree.cmake to remove exactly the two old names now fails, and names the three you predicted:

FAIL: STAGE_NATIVE=0 prunes stale native archives and header trees
      survived: /tmp/tmp.eDGP6D86fv/lib/libnative_c.a
      survived: /tmp/tmp.eDGP6D86fv/lib/libnative_c++.a
      survived: /tmp/tmp.eDGP6D86fv/lib/libnative_rt.a
Results: 22 passed, 1 failed

libnative_c++.a is seeded and asserted literally, so the plus signs are exercised end to end rather than only by the OFF build's ^libnative[^[:space:]]*\.a$ scan.

[huangminghuang]

The three inline findings from the previous pass are fixed. I reran the focused suites (55/55 abidiff and 23/23 staging assertions); current-head CI is green and the merge tree is clean. Two remaining correctness/coverage findings are inline.

The required current-head integration sweep and follow-up documentation refresh also remain outstanding: CLAUDE.md requires all three suites with ENABLE_INTEGRATION_TESTS=ON for ABI/runtime changes, while the workflows still run the default 31 tests and the recorded integration run predates the later codegen, ABI, and staging changes. The PR body also still describes removed gate/max-major behavior and reports 36/53/5 rather than the current 55/59/23 assertion counts; please refresh it and the corresponding stale source comments to describe the final implementation.

[huangminghuang]

[P1] Use strict JSON parsing before deciding equality. ojson::parse uses jsoncons's default handler, which silently accepts and removes C/C++ comments. On this head, cdt-abidiff reports no difference between a commented outer ABI and its uncommented form even though wire-sysio's FC parser rejects the former. The same false equality occurs for string-valued protobuf_types containing {"file":[]/*comment*/} versus the equivalent object value, although protobuf 6.33.4 rejects the string and accepts the object. Please use jsoncons::strict_parse_error_handler for this parse and the nested parse in canonical_protobuf, and add regressions, so a chain-rejected ABI cannot compare equal to an accepted one.

[heifner]

Confirmed and fixed in 511b1ad. default_parse_error_handler swallows exactly one error and it is this one:

if (code == illegal_comment) { return false; }   // jsoncons/parse_error_handler.hpp:118

So a commented document — which fc will not load — compared equal to the uncommented one it does load. Same class of false equality as a dropped duplicate member, and invisible for the same reason: the difference is gone before anything compares it.

Both parses now go through one helper:

inline ojson parse_strict(const std::string& text) {
   jsoncons::strict_parse_error_handler strict;
   return ojson::parse(text, strict);
}

• the outer parse in load_checked refuses the document, naming the file: Error: file1 is not strict JSON: .... That also improves the diagnostic for ordinary malformed input, which previously surfaced as a bare e.what() from main with no indication of which file.
• the nested parse in canonical_protobuf simply does not adopt, so a commented string stays the string it is and therefore differs from the object — which matches the chain, protobuf rejecting the string and accepting the object.

Three regressions: a commented document is refused; a commented protobuf_types string differs from the equivalent object; and — the direction that keeps the fix from being "reject every string" — an uncommented string still equals its object. Reverting both parses to ojson::parse fails the first two and nothing else (56/58). 58/58 with the fix.

expect_refused now takes the needle, since there are two distinct refusal reasons to tell apart.

I left the duplicate-member SAX walk on the default handler deliberately: its job is duplicates, not strictness, and it must not throw on input the parse is about to diagnose. A document with both a comment and a duplicate is reported as the duplicate — refused either way.

[huangminghuang]

[P2] Verify the non-native sysiolib tree itself. This recursive count includes ${root}/sysiolib/native, so native headers can satisfy the purported non-native assertion while every regular sysiolib header is missing. I gated the main sysiolib copy behind NOT STAGE_NATIVE; the native-ON output retained only four sysiolib/native headers and lost all 63 regular headers, yet this suite still passed 23/23. Please exclude the native subtree from this count or verify the expected non-native headers/source set explicitly.

[heifner]

Confirmed and fixed in 511b1ad — and your fault injection found a second gap behind the first, so it took two changes rather than one.

The count. count_non_native_files now prunes the nested native subtree:

find "$1" -type f -not -path '*/sysiolib/native/*' 2>/dev/null | wc -l

so the four sysiolib/native headers can no longer stand in for the 63 regular ones.

That alone did not catch it. Running your injection against the fixed count still passed 23/23, because nothing re-ran the staging script in ON mode: the live ${INCLUDE_DIR} this suite inspects is a snapshot of whatever the last real build left, so a staging regression is invisible there until someone rebuilds — and the OFF probe is unaffected by a NOT STAGE_NATIVE gate by construction.

The isolated fresh staging already runs the script with STAGE_NATIVE=${NATIVE_ENABLED}, so it is now checked per destination too, before any sentinel is planted:

require_non_native_dests "${PRUNE_SCRATCH}/include" " (fresh staging)"

Both halves are load-bearing, confirmed separately:

variant	result
both fixes	28/28
+ main sysiolib copy gated on NOT STAGE_NATIVE	FAIL: sysiolib is staged (fresh staging)
same injection, but count reverted to plain recursive	28/28 — invisible again

Baseline is now 28 assertions, and sysiolib reports 63 files in all three roots rather than 67 in the ON one.

[huangminghuang]

The two inline defects from the previous pass are fixed. I compiled this head and reproduced 58/58 abidiff assertions; a freshly staged native-ON tree plus the isolated native-OFF build passes 28/28 staging assertions. Ubuntu's 30/30 default suite and package checks are green, the merge with current master is clean, and macOS was still building during this pass. One new parser-policy mismatch is inline.

Two previously noted merge requirements also remain unresolved:

• There is still no passing current-head ENABLE_INTEGRATION_TESTS=ON sweep. This PR changes ABI generation and runtime-library staging, so the CLAUDE.md pre-PR requirement applies; both workflows omit the option and the current Linux job runs only the 30 non-integration tests. The integration claims in early commit messages predate the later ABI/codegen/staging changes.
• The PR description and source comments still describe superseded implementations. The body says cdt-abidiff uses capability predicates/gates, derives thresholds from max_supported_major, reports 36/53/5 assertions and 31 ctests, and calls index_type Wire-only; current behavior is unconditional diffing, explicit introduction-version pairs, 58/59/28 focused assertions, and 30 default ctests. Related stale comments remain in abi.hpp, abimerge.hpp, cdt-abidiff.cpp.in, tests/CMakeLists.txt, abidiff_tests.sh, stage_cdt_tree.cmake, and staged_headers_tests.sh. Please refresh the final PR description and those comments after the implementation settles.

[huangminghuang]

[P2] Do not claim strict JSON matches the chain parser's accepted syntax. This catch handles every strict-jsoncons error and says FC rejects the document, but FC's legacy parser accepts trailing commas. I tested otherwise-valid ABIs with a trailing comma at the top level, inside an object, and inside an array: clio convert pack_hex ... --type abi_def packs all three with exit 0, while this head's cdt-abidiff file file exits 1 with Extra comma followed by this incorrect claim. The outer ABI and string-valued protobuf_types also pass through different runtime parsers (FC versus protobuf), so one shared strict policy cannot be described as matching both. Either make the outer parse mirror FC's accepted recoveries (jsoncons can recover extra_comma while still rejecting illegal_comment, with other FC leniencies audited) or explicitly document strictness as a cdt-abidiff policy and correct this diagnostic/comment; add a trailing-comma regression for the chosen contract.

[heifner]

You are right about the claim, and I checked fc rather than taking it on faith — its array loop consumes any comma it meets:

while( in.peek() != ']' ) {
   if( in.peek() == ',' ) { in.get(); continue; }     // libfc/src/io/json.cpp
   ...

so a trailing comma (a leading or repeated one too) loads fine. The diagnostic blamed the chain for a rule the chain does not have. Corrected in 97756f9.

I went with the second option — strictness as a documented cdt-abidiff policy — rather than mirroring fc, for three reasons:

It is not a behaviour change. default_parse_error_handler returns false for illegal_comment and true for everything else, so extra_comma was already propagating before this PR. I rebuilt with the parse reverted to ojson::parse(text) to be sure, and it rejects your input identically:

Error: file1 is not strict JSON: Extra comma at line 1 and column 55

The only behaviour this PR changed is the comment case. Making trailing commas parse would be a new leniency, not a restoration.

"Recover extra_comma" would not actually mirror fc. fc swallows commas anywhere in the loop, including leading and repeated ones; jsoncons's recovery is a different shape. Matching it properly means auditing fc's leniencies as a whole, which is its own change with its own risk — a lenient parse rewrites the document it then compares, which is the failure this parse exists to prevent.

Your parser point cuts against mirroring anything. The outer document is fc's to read; a string-valued protobuf_types is JsonStringToMessage's. One policy cannot be described as matching both, so it is now described as matching neither, deliberately.

The diagnostic now reads:

Error: file1 is not strict JSON: Extra comma at line 1 and column 55
cdt-abidiff refuses what it cannot read strictly, rather than let a
lenient parse normalise a difference away before anything compares
it. The runtime's parsers are laxer in places -- fc tolerates a stray
comma -- so this is this tool's policy, not a verdict on whether the
chain would load the file.

and the comment on parse_strict separates the two halves explicitly: the comment case is the bug (jsoncons accepts, fc rejects, false equality follows); everything else is policy, conservative in the direction that never answers "equal" about text it had to rewrite.

Two regressions, per your ask: the trailing comma is refused, and the diagnostic names whose rule it is. Restoring the old wording fails the second (59/60). 60/60 as it stands; full unit label 27/27.

[huangminghuang]

97756f94 resolves the previous inline finding: strictness is now correctly described as cdt-abidiff's policy rather than a claim about FC, and the new trailing-comma regression pins that contract. I compiled this head and the focused abidiff suite passes 60/60; the latest delta also passes git diff --check, the merge with current master is clean, and Ubuntu/package checks are green. I found no new code defect in this follow-up.

I still cannot approve because the two PR-wide requirements from the previous review remain unresolved:

• No passing current-head ENABLE_INTEGRATION_TESTS=ON sweep is recorded. This PR changes ABI generation and runtime-library staging, so the repository's pre-PR requirement applies. Both workflows omit the option and the Linux job runs only the 30 non-integration tests; the integration claims in early commits predate the later ABI/codegen/staging changes.
• The PR description and related source comments still describe superseded implementations. The body still says cdt-abidiff uses capability gates, derives thresholds from max_supported_major, reports 36/53/5 focused assertions and 31 ctests, and calls index_type Wire-only. Current totals are 60/59/28 and 30 default ctests, and cdt-abidiff now diffs the sections unconditionally. The stale comments previously identified in abi.hpp, abimerge.hpp, cdt-abidiff.cpp.in, tests/CMakeLists.txt, abidiff_tests.sh, stage_cdt_tree.cmake, and staged_headers_tests.sh also remain. Please refresh the full PR description and those comments before the next review.

[heifner]

Both PR-wide requirements are done — 5f7d0c62 for the comments, and the PR body is rewritten.

Integration sweep at the current head

Run here rather than inferred from CI, which enables neither the option nor the sysio_DIR it needs:

Run	Result
ctest — all labels, ENABLE_INTEGRATION_TESTS=ON	31/31 (27 unit_tests, 3 toolchain_tests, 1 integration_tests)
ctest --test-dir tests/integration — per Boost suite	16/16
abidiff_tests.sh / abi_version_tests.sh / staged_headers_tests.sh	60 / 61 / 28

clang-18, Debug, x64-linux-release, against wire-sysio @ 55e20cd4a7. The 16 suites are action_results, bls_primitives, capi, codegen, crypto_primitives, get_code_hash, hash_id, instant_finality, kv_cached, kv_global, kv_indexed_table, kv_scoped_table, kv_singleton, memory, multi_index, name_pk — the ABI, codegen and KV-runtime paths this PR touches, against the real chain runtime. I reran the whole sweep after the comment commit, not just before it.

One number differs from yours: I measure abi_version_tests.sh at 61, not 59. It takes three arguments (<build> <source> <vcpkg-include>) and silently under-runs if the third is wrong — that may be the difference.

Comments

Each was checked against the code rather than reworded, and two turned out to be wrong in a way the summary had not captured:

• abi.hpp — supports_action_results claimed abigen, ABIMerger and cdt-abidiff all decide with it. Only abigen calls it. ABIMerger reads the introduction constants directly (it needs the pair itself to promote a merged version), and cdt-abidiff does not gate at all. While checking, supports_variants turned out to have no caller anywhere — I left it in place and documented that rather than deleting a shared-header API unasked, but say the word and it goes.
• abimerge.hpp:100-104 — as you said, contradicted by merge_version, which canonicalises at the top of merge(). Corrected to say ordering ignores the prefix so a foreign descriptor can be ingested, while the output never carries one.
• cdt-abidiff.cpp.in — "every gate below keys off this value" and "right for deciding capability"; there are no gates below, and what the parse is right for is ordering.
• tests/CMakeLists.txt, abidiff_tests.sh — the same capability framing in their preambles.
• stage_cdt_tree.cmake, staged_headers_tests.sh — both said an OFF configure never rebuilds libsf.a. It is built in every configuration since the sf target moved outside the ENABLE_NATIVE_COMPILER guard (libraries/native/CMakeLists.txt:355, with its POST_BUILD copy at :370, both unindented), which is exactly what the clean OFF probe asserts.
• CDTMacros.cmake.in:185 — db_store_i64 → kv_set/kv_get.

PR description

Rewritten for the four claims you named, each verified first:

• capability gates → the gates are gone; every section is diffed unconditionally.
• max_supported_major → explicit introduction pairs, with the reason deriving them was wrong.
• index_type called Wire-only → it is a standard table_def field (abi.hpp:196-205); only table_id and secondary_indexes are the Wire additions. Marked as a correction rather than quietly edited.
• 36/53/5 and 31 ctests → 60/61/28 and 30 default / 31 with integration.

Two more the body still had wrong, which you had flagged in earlier rounds:

• It said master discarded the 1.0 variant definition. It did not — master emitted variants unconditionally, so -abi-version 1.0 produced a 1.0 ABI carrying a 1.1 section, a version stamp contradicting its own content rather than a dangling type reference. Verified on this head, which now promotes to 1.1 instead.
• The matcher section said "three defects" above a six-row table.

[huangminghuang]

5f7d0c62 fixes the earlier parser-policy wording, refreshes the PR description, and records the required current-head integration sweep. I independently confirmed the ABI-version suite at 61/61; the prior abidiff 60/60 and staging 28/28 results remain applicable because this delta changes comments only. The merge tree is clean, Ubuntu/package checks are green, and macOS is still running. One stale schema statement remains inline; please also correct its mirrored wording in tests/unit/abidiff_tests.sh:326.

[huangminghuang]

[P2] Keep the schema comment aligned with the actual ABI fields

index_type is a standard Antelope table_def field; only table_id and secondary_indexes are Wire additions. The refreshed PR description now distinguishes these correctly, but this comment—and the mirrored test preamble at tests/unit/abidiff_tests.sh:326—still says a stock Antelope ABI carries no index_type. Please correct both so the implementation and regression test no longer document the superseded schema model.

[heifner]

Right — I corrected that claim in the description and left it standing in the two places that document the code. Fixed in 3cc7752, both sites.

The test comment was contradicted by its own fixture ten lines below it:

"tables": [ { "name": "t", "type": "row", "index_type": "i64",
              "key_names": ["id"], "key_types": ["uint64"] } ]

t_antelope.abi carries index_type, key_names and key_types and omits only table_id and secondary_indexes — which is exactly what makes it a stock ABI rather than an empty one. Both comments now say which two are Wire's additions to table_def and point at that fixture as the evidence, so the next reader can check the claim without leaving the file.

One thing fell out while correcting it. The block above section_or_empty was really two comments: an optional-field paragraph ending "Every optional field is read through this", sitting on the function that reads whole sections. field_or_null is the one that sentence is true of, so the paragraph moved there. That is why the diff touches a little more than the one sentence you flagged.

abidiff_tests.sh 60/60, unit_tests 27/27 after the change; comments only, no behaviour touched.

Thanks for confirming 61/61 independently on the ABI-version suite — that matches what I measured, so the 59 in the earlier round was almost certainly the third-argument path.

[huangminghuang]

3cc7752c correctly fixes the index_type classification and moves the optional-field explanation beside field_or_null. The latest delta changes comments only, its shell test remains syntactically valid, the merge tree is clean, and all current GitHub checks are green. A cumulative reread found one other current-tense comment that still describes the zero-major sentinel removed by this PR; details inline.

[huangminghuang]

[P2] Describe the retired zero-major sentinel in past tense

This paragraph says cdt-cpp currently interprets a zero major as "the option was never given", but this PR removes both > 0 guards: cdt-cpp.cpp.in now always records the parsed version in the manifest and always forwards it to codegen. Because parse() itself rejects zero, the described fallback is neither current nor reachable, and it directly contradicts the new driver comment that says there is no unset state. Please preserve the useful historical rationale in past tense (or explicitly say the sentinel was retired) so the shared parser documentation matches the implementation.

[heifner]

Confirmed and fixed in 0729b54. The contradiction was two files apart:

// tools/cc/cdt-cpp.cpp.in:69
// Always recorded: abi_version::parse rejects a zero major and the default is
// non-zero, so there is no "unset" state left for a >0 test to stand in for.

and there are no surviving > 0 guards on abi_version anywhere in tools/ or plugins/, so the fallback the paragraph described is unreachable as well as retired.

I kept the rationale rather than deleting it, in past tense, because it is the reason the driver can be as simple as it now is — the sentinel is retired precisely because zero never survives the parse, so the rejection is what stops it being needed again:

 * A zero major is rejected: there is no ABI 0.x.
 *
 * That rule once carried a second job... cdt-cpp USED to read a zero major as "the option
 * was never given"... That sentinel is retired: the driver now records and forwards the
 * parsed version unconditionally, which it can do precisely BECAUSE zero never survives
 * this parse. Keeping the rejection is what stops the sentinel being needed again.

A third copy you did not flag carried the same claim — tests/unit/abi_version_tests.sh:150, on its own 0.1 case: "cdt-cpp reads a zero major as option absent -- accepting one would reopen the driver/codegen divergence." Corrected with it. The case still guards that divergence, now by keeping the sentinel unnecessary rather than by feeding it. I grepped never given|option was never|zero major|unset state across tools/, plugins/ and tests/ to be sure those three were all of them.

Rebuilt and reran: ctest 31/31 with integration on, abi_version_tests 61/61.

[huangminghuang]

0729b548 resolves the outstanding zero-major documentation issue in both the shared parser and its regression-test preamble. The latest delta is comments only; its diff and shell syntax are clean. I rechecked the cumulative PR, commit list, description, merge tree, and current checks. The merge is clean and all required GitHub checks pass. No further blocking issues found.