Reject a recorded type byte that names no type

include/pup/core/types.hpp

@@ -144,6 +144,8 @@ enum class LinkRole : std::uint8_t {
 
 /// The one place a new LinkType must be classified: no `default`, so -Wswitch makes
 /// omitting one a build error here instead of a silent exclusion from every mask.
+/// The recovery read consumes this across the whole readable window: retiring an enumerator without
+/// a format bump would need the retired set NodeFlags has and this does not (#399).
 [[nodiscard]]
 constexpr auto link_role(LinkType type) -> LinkRole
 {
@@ -161,6 +163,35 @@ constexpr auto link_role(LinkType type) -> LinkRole
     return LinkRole::Unknown;
 }
 
+/// A persisted type byte outside its enum is damage, not a value from a future version (#399).
+/// The recovery read consumes this across the whole readable window: retiring an enumerator without
+/// a format bump would need the retired set NodeFlags has and this does not (#399).
+[[nodiscard]]
+constexpr auto names_node_type(std::uint8_t value) -> bool
+{
+    switch (static_cast<NodeType>(value)) {
+    case NodeType::File:
+    case NodeType::Command:
+    case NodeType::Directory:
+    case NodeType::Variable:
+    case NodeType::Generated:
+    case NodeType::Ghost:
+    case NodeType::Group:
+    case NodeType::GeneratedDir:
+    case NodeType::Root:
+    case NodeType::Condition:
+    case NodeType::Phi:
+        return true;
+    }
+    return false;
+}
+
+[[nodiscard]]
+constexpr auto names_link_type(std::uint8_t value) -> bool
+{
+    return link_role(static_cast<LinkType>(value)) != LinkRole::Unknown;
+}
+
 /// One command that must run before another, on evidence no edge in the graph carries. A
 /// discovered dependency is recorded only in the index, so both the router that decides who runs
 /// and the scheduler that decides in what order have to be told about it separately (#276, #277).

include/pup/index/entry.hpp

@@ -6,6 +6,7 @@
 #include "format.hpp"
 #include "pup/core/arena.hpp"
 #include "pup/core/node_id_map.hpp"
+#include "pup/core/result.hpp"
 #include "pup/core/string_id.hpp"
 #include "pup/core/types.hpp"
 #include "pup/core/vec.hpp"
@@ -35,12 +36,14 @@ struct FileEntry {
 
     /// Create from raw format (path must be computed separately from parent chain)
     /// @param array_index 0-based position in file array (ID = array_index + 1)
+    /// @return IndexDamaged if the recorded type byte names no NodeType this build knows, or the
+    ///         recorded flags word carries a bit no version in the readable window names
     [[nodiscard]]
     static auto from_raw(
         RawFileEntry const& raw,
         std::string_view name_str,
         std::size_t array_index
-    ) -> FileEntry;
+    ) -> Result<FileEntry>;
 };
 
 /// In-memory command entry (v8)
@@ -73,6 +76,7 @@ struct CommandEntry {
 
     /// Create from raw format
     /// @param array_index 0-based position in command array (ID = node_id::make_command(array_index + 1))
+    /// @return IndexDamaged if the recorded flags word carries a bit no CommandFlag names
     [[nodiscard]]
     static auto from_raw(
         RawCommandEntry const& raw,
@@ -82,7 +86,7 @@ struct CommandEntry {
         Vec<NodeId> inputs,
         Vec<NodeId> outputs,
         std::size_t array_index
-    ) -> CommandEntry;
+    ) -> Result<CommandEntry>;
 };
 
 /// In-memory edge
@@ -96,8 +100,9 @@ struct EdgeEntry {
     auto to_raw() const -> RawEdge;
 
     /// Create from raw format
+    /// @return IndexDamaged if the recorded type byte names no LinkType this build knows
     [[nodiscard]]
-    static auto from_raw(RawEdge const& raw) -> EdgeEntry;
+    static auto from_raw(RawEdge const& raw) -> Result<EdgeEntry>;
 };
 
 /// Complete in-memory index

include/pup/index/format.hpp

@@ -180,9 +180,22 @@ constexpr auto flag_category(CommandFlag flag) -> std::string_view
     return {};
 }
 
-// Nothing calls flag_category — the switch is the point, so this is what keeps it compiled.
 static_assert(!flag_category(CommandFlag::MustRerun).empty(), "a flag names a category or it is not a category");
 
+/// Which bits a recorded command flags word may carry. Derived from `flag_category` rather than
+/// written out, so a new flag widens what the reader accepts by the same answer that names it.
+/// No retired term, unlike the node word: commands are read only at exact version (#399).
+inline constexpr auto RECORDED_COMMAND_FLAGS_MASK = [] {
+    auto mask = std::uint32_t { 0 };
+    for (auto bit = 0; bit < 32; ++bit) {
+        auto const flag = static_cast<CommandFlag>(std::uint32_t { 1 } << bit);
+        if (!flag_category(flag).empty()) {
+            mask |= static_cast<std::uint32_t>(flag);
+        }
+    }
+    return mask;
+}();
+
 [[nodiscard]]
 constexpr auto to_underlying(CommandFlag flag) -> std::uint32_t
 {
@@ -228,6 +241,62 @@ struct alignas(8) RawFooter {
 
 static_assert(sizeof(RawFooter) == 32, "RawFooter must be 32 bytes");
 
+/// The one place a new NodeFlags bit must be answered for: -Wswitch makes an unlisted enumerator a
+/// build error, and the reader's validity mask is derived from this switch, so a bit cannot reach
+/// the record without joining it (#399). Single-bit enumerators only: a composite convenience
+/// spelling, if one is ever added, classifies false. The mask derivation below is what keeps this
+/// switch compiled, so it needs no keep-compiled static_assert of its own like `flag_category`'s.
+[[nodiscard]]
+constexpr auto is_recorded_node_flag(NodeFlags flag) -> bool
+{
+    switch (flag) {
+    case NodeFlags::Modified:
+    case NodeFlags::Created:
+    case NodeFlags::AbsenceRouted:
+    case NodeFlags::ConfigDep:
+    case NodeFlags::Transient:
+        return true;
+    case NodeFlags::None:
+        return false;
+    }
+    return false;
+}
+
+inline constexpr auto RECORDED_NODE_FLAGS_MASK = [] {
+    auto mask = std::uint16_t { 0 };
+    for (auto bit = 0; bit < 16; ++bit) {
+        auto const flag = static_cast<NodeFlags>(std::uint16_t { 1 } << bit);
+        if (is_recorded_node_flag(flag)) {
+            mask |= static_cast<std::uint16_t>(flag);
+        }
+    }
+    return mask;
+}();
+
+static_assert(
+    RECORDED_NODE_FLAGS_MASK == 0x1F,
+    "update deliberately with the enum; this is the on-disk vocabulary, not a checksum — and a flag "
+    "retired without a format bump MOVES its bit to RETIRED_NODE_FLAGS_MASK rather than vanishing: "
+    "the recovery read reaches back to INDEX_LAYOUT_FLOOR, so records that carried the bit are still "
+    "readable (#399, fa5deb220)"
+);
+
+/// Bit 5 was NodeFlags::Inactive until it was retired without a format change (fa5deb220, index
+/// version 13), and the recovery read reaches back to INDEX_LAYOUT_FLOOR, so a record that carries
+/// it is old rather than damaged. A retired bit is history and cannot grow by being forgotten.
+inline constexpr auto RETIRED_NODE_FLAGS_MASK = std::uint16_t { 1 << 5 };
+
+static_assert(
+    (RECORDED_NODE_FLAGS_MASK & RETIRED_NODE_FLAGS_MASK) == 0,
+    "a retired bit cannot be reassigned while INDEX_LAYOUT_FLOOR admits records that carried it: an "
+    "old record's stale bit would be read as the new flag's meaning. Reuse the bit only after raising "
+    "the floor past the retirement."
+);
+
+/// Which bits a recorded node flags word may carry, over every version the readers accept.
+inline constexpr auto READABLE_NODE_FLAGS_MASK
+    = static_cast<std::uint16_t>(RECORDED_NODE_FLAGS_MASK | RETIRED_NODE_FLAGS_MASK);
+
 /// Helper to get NodeFlags from entry
 [[nodiscard]]
 inline auto get_node_flags(RawFileEntry const& entry) -> NodeFlags

spec/requirements/record-read.ears.md

@@ -17,11 +17,24 @@ downstream code acts on — the first feeds change detection, the second the pat
 so a validation failure makes the record unreadable rather than weaker. `DESIGN.md`'s "What a
 record claims" carries the rule and its display-side counterpart.
 
-Two boundaries keep that from over-reaching. Rejection follows a failed check, never a value: the
-string table's offset 0 is a legitimate empty string and reads as one. And each reader validates
-what it reads, so the recovery read of the file table (`read_prior_paths`, issue #291) is unaffected
-by damage in sections it never looks at — which is what makes whole-record rejection safe rather
-than a wider outage than the damage warrants.
+A recorded value naming nothing in its enum or flag vocabulary — an entry or edge type byte, an
+entry or command flags word — is the same class of failure. Tolerating it has no disposition a
+reader can implement: dropping the entry is the silent-nothing state — an edge that joins no mask
+routes nothing — and substituting a classification is the substitution this area already forbids.
+Nor is a value the reader merely ignores harmless, because a read word is written back into a fresh
+record and thereafter looks authentic.
+
+Three boundaries keep that from over-reaching. Rejection follows a failed check, never a value: the
+string table's offset 0 is a legitimate empty string and reads as one. Each reader validates what it
+reads, so the recovery read of the file table (`read_prior_paths`, issue #291) is unaffected by
+damage in sections it never looks at — which is what makes whole-record rejection safe rather than a
+wider outage than the damage warrants. And the vocabulary a value is judged against spans every
+version the readable window admits, not today's alone: type vocabularies only ever grew, so the
+floor admits only prefixes of this one's, but a flag bit retired without a format change
+(`NodeFlags` bit 5, index version 13) was legitimate when written and stays readable. That span is
+uniform rather than per-version, because a read word is carried whole into the record the next
+build writes — so a current-version record holding a retired bit is an honest descendant of one
+that predates the retirement, and judging it by today's vocabulary alone would reject its lineage.
 
 Upstream tup keeps its state in a database and delegates this class of decision to SQLite, so it
 has no counterpart and every requirement here is `putup-only`.
@@ -42,6 +55,20 @@ What a failed validation does to the record.
 If a semantics-bearing field's declared position fails its bounds check, then putup shall report the
 record as unreadable rather than returning an empty value in that field's place.
 
+### REQ-READ-REJECT-UNKNOWN-VALUE
+
+- conformance: putup-only
+- discharge: test "A record whose entry carries a type this putup cannot name is unreadable"
+- discharge: test "A record whose edge carries a link type this putup cannot name is unreadable"
+- discharge: test "A record whose edge carries link type zero is unreadable"
+- discharge: test "A record whose entry carries a flag bit this putup cannot name is unreadable"
+- discharge: test "A record whose command carries a flag bit this putup cannot name is unreadable"
+- discharge: test "A record carrying a flag bit retired since it was written stays readable"
+
+If a recorded entry, edge, or command carries a value no version in the readable window defines,
+then putup shall report the record as unreadable rather than admitting a value no observer
+classifies.
+
 ### REQ-READ-REJECT-SELF-CONTRADICTION
 
 - conformance: putup-only

src/index/entry.cpp

@@ -40,8 +40,15 @@ auto FileEntry::from_raw(
     RawFileEntry const& raw,
     std::string_view name_str,
     std::size_t array_index
-) -> FileEntry
+) -> Result<FileEntry>
 {
+    if (!names_node_type(raw.type)) {
+        return make_error<FileEntry>(ErrorCode::IndexDamaged, "Recorded entry type names no node type");
+    }
+    auto const raw_flags = static_cast<std::uint16_t>(get_node_flags(raw));
+    if ((raw_flags & ~READABLE_NODE_FLAGS_MASK) != 0) {
+        return make_error<FileEntry>(ErrorCode::IndexDamaged, "Recorded entry flags carry a bit no flag names");
+    }
     return FileEntry {
         .id = static_cast<NodeId>(array_index + 1),
         .parent_id = raw.parent_id,
@@ -81,8 +88,11 @@ auto CommandEntry::from_raw(
     Vec<NodeId> inputs,
     Vec<NodeId> outputs,
     std::size_t array_index
-) -> CommandEntry
+) -> Result<CommandEntry>
 {
+    if ((raw.flags & ~RECORDED_COMMAND_FLAGS_MASK) != 0U) {
+        return make_error<CommandEntry>(ErrorCode::IndexDamaged, "Recorded command flags carry a bit no flag names");
+    }
     return CommandEntry {
         .id = node_id::make_command(array_index + 1),
         .dir_id = raw.dir_id,
@@ -107,8 +117,11 @@ auto EdgeEntry::to_raw() const -> RawEdge
     };
 }
 
-auto EdgeEntry::from_raw(RawEdge const& raw) -> EdgeEntry
+auto EdgeEntry::from_raw(RawEdge const& raw) -> Result<EdgeEntry>
 {
+    if (!names_link_type(raw.type)) {
+        return make_error<EdgeEntry>(ErrorCode::IndexDamaged, "Recorded edge type names no link type");
+    }
     return EdgeEntry {
         .from = raw.from_id,
         .to = raw.to_id,

src/index/reader.cpp

@@ -146,7 +146,11 @@ auto read_index(IndexFile const& f) -> Result<Index>
         if (!name) {
             return pup::unexpected<Error>(name.error());
         }
-        index.add_file(FileEntry::from_raw(raw, *name, i));
+        auto entry = FileEntry::from_raw(raw, *name, i);
+        if (!entry) {
+            return pup::unexpected<Error>(entry.error());
+        }
+        index.add_file(*entry);
     }
 
     // Compute paths from parent chain (after all files loaded)
@@ -166,15 +170,23 @@ auto read_index(IndexFile const& f) -> Result<Index>
         if (!operands) {
             return pup::unexpected<Error>(operands.error());
         }
-        index.add_command(CommandEntry::from_raw(
+        auto command = CommandEntry::from_raw(
             raw, instruction_pattern, display, *env, std::move(operands->first), std::move(operands->second), i
-        ));
+        );
+        if (!command) {
+            return pup::unexpected<Error>(command.error());
+        }
+        index.add_command(std::move(*command));
     }
 
     // Read edges
     auto edges = index_raw_edges(f);
     for (auto const& raw : edges) {
-        index.add_edge(EdgeEntry::from_raw(raw));
+        auto edge = EdgeEntry::from_raw(raw);
+        if (!edge) {
+            return pup::unexpected<Error>(edge.error());
+        }
+        index.add_edge(*edge);
     }
 
     // Build edge indices for O(1) lookup
@@ -282,7 +294,11 @@ auto read_prior_paths(std::string_view path) -> PriorPaths
         if (!name) {
             return lost;
         }
-        recorded.add_file(FileEntry::from_raw(raw[i], *name, i));
+        auto entry = FileEntry::from_raw(raw[i], *name, i);
+        if (!entry) {
+            return lost;
+        }
+        recorded.add_file(*entry);
     }
     recorded.compute_paths();