Repository navigation
rejected: generic multi-origin convergence does not belong in UCNS - #236
erinepshovel-code wants to merge 11 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b75b4f86f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| class InvariantObservation: | ||
| invariant_id: str | ||
| source_value: str | ||
| target_value: str | ||
| preserved: bool |
There was a problem hiding this comment.
For any caller, this schema accepts arbitrary domain values plus a caller-asserted preserved judgment without identifying a geometric carrier, scale boundary, or operation. The accompanying fixture uses agents, hosts, and literary provenance, so its digest certifies a generic cross-domain comparison rather than UCNS mathematical evidence; move this adjudication to EDCM/METAPAT or bind it to actually constructed UCNS geometry.
AGENTS.md reference: AGENTS.md:L3-L9
Useful? React with 👍 / 👎.
| if self.receipt_sha256 and self.receipt_sha256 != expected: | ||
| raise ValueError("receipt_sha256 mismatch") | ||
| object.__setattr__(self, "receipt_sha256", expected) |
There was a problem hiding this comment.
When a serialized witness is modified and its receipt_sha256 is set to "", this truthiness guard skips the mismatch check and then replaces the blank value with a newly computed digest. Thus from_dict() accepts and silently re-receipts precisely the tampered payload that convergence_digest_tamper_rejected promises to reject; deserialization must require a nonempty matching receipt rather than treating blank as fresh construction.
Useful? React with 👍 / 👎.
| return StructuralConvergenceWitness(**data) | ||
|
|
||
|
|
||
| def test_witness_preserves_distinct_paths_and_independence(): |
There was a problem hiding this comment.
None of the five executable tests has a CHECKS declaration linking it to the four new contracts. Running python3 tools/verify_skill_lib_contracts.py . reports every contract as unproved and every test as unresolved, so this commit cannot pass the repository's required outcome gate until a resolving CHECKS block is added.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| mapping_complete: Optional[bool] | ||
| replay_passed: Optional[bool] |
There was a problem hiding this comment.
Dataclass annotations do not enforce runtime types, and __post_init__ never validates these fields, so direct construction and from_dict() accept values such as replay_passed=0, replay_passed="yes", or mapping_complete=[]. Consumers can consequently interpret an invalid evidence status as false or truthy success despite the contract requiring bool | None; reject every value whose exact type is neither bool nor None.
Useful? React with 👍 / 👎.
| @property | ||
| def independent(self) -> bool: | ||
| return self.paths_distinct and not self.shared_ancestry |
There was a problem hiding this comment.
When the source and target have distinct path IDs but share the same provenance_ids entry, leaving shared_ancestry at its default makes this property return True even though the record itself proves common provenance. This can label dependent evidence as independent; check the two provenance sets as well, or represent independence as unresolved unless ancestry was explicitly evaluated.
Useful? React with 👍 / 👎.
| mappings: tuple[StructuralMapping, ...] | ||
| invariants: tuple[InvariantObservation, ...] |
There was a problem hiding this comment.
The public constructor accepts lists for mappings and invariants despite the tuple annotations, and the frozen dataclass retains those lists by reference. If the caller mutates either list after construction, to_dict() emits changed evidence while receipt_sha256 remains the digest of the original payload, defeating the witness's integrity guarantee; reject non-tuples or defensively convert all collection fields before computing the receipt.
Useful? React with 👍 / 👎.
| def __post_init__(self) -> None: | ||
| if self.source == self.target: | ||
| raise ValueError("source and target paths must be distinct records") |
There was a problem hiding this comment.
The equality check compares every field of the two records, so a source and target with the same origin_id and path_id but different structure_id or provenance are accepted even though paths_distinct is then false. This permits constructing the path-collapse case that the witness contract is intended to exclude; validate the identity fields themselves rather than only whole-record inequality.
Useful? React with 👍 / 👎.
|
Audit rejected this placement. The generic witness let callers assert cross-domain invariant preservation without constructing a UCNS carrier, operation, or scale boundary. That violates UCNS geometry authority. The implementation, tests, facade export, and graph projection have been removed. Replacement path:
This preserves the convergence concept while rejecting the generic UCNS schema. |
erinepshovel-code
changed the title
1 participant
Architectural audit falsified this placement. Generic cross-domain invariant judgments are not UCNS geometry.
All proposed implementation changes have been removed. The concept continues in Stack + METAPAT; UCNS re-enters only through constructed geometric evidence.