fix(hashing): stable canonical names for orcapod types in function-pod signature hashing (ITL-638)

[kurodo3]

Summary

Fixes ITL-638: function pod signature hashing encoded orcapod logical types by their fully-qualified import path (type:orcapod.logical_types.file_type.File), so any internal module reorganisation silently invalidated every cached result for functions annotated with op.File, op.Directory, etc.

Root causes (two surfaces)

1. TypeObjectHandler serialised type objects as "type:{module}.{qualname}" with no registry lookup.
2. FunctionSignatureExtractor had two asymmetric problems:
   • Params: annotation strings were baked in via str(param) → inspect.formatannotation, embedding the full import path.
   • Returns: the raw type object was stored as-is, then serialised by TypeObjectHandler — full path again.

Fix (Approach B)

• canonical_annotation_str in hash_utils.py — extended with an optional registry parameter. Registered types resolve to their logical_type_name (e.g. "orcapod.file"); generic aliases and unions recurse; everything else falls back to inspect.formatannotation, preserving existing behaviour exactly.

• TypeObjectHandler in builtin_handlers.py — accepts an optional logical_type_registry; lazy-falls-back to get_default_context().type_converter._logical_type_registry (safe getattr access). Registered types now emit "type:orcapod.file" instead of the full path.

• FunctionSignatureExtractor in function_info_extractors.py — accepts the same optional logical_type_registry. Params: all annotation substrings are now replaced with canonical_annotation_str(annotation, registry) (not just unions). Returns: stores a canonical string only when the return type contains a registered orcapod type; keeps the raw type object for builtins and user types to avoid invalidating existing hashes.

• register_builtin_python_type_handlers — new logical_type_registry parameter forwarded to both TypeObjectHandler and FunctionSignatureExtractor. Callers that omit it get the same lazy-fallback behaviour as before.

Regression tests

• tests/test_hashing/test_type_annotation_golden.py — pre-fix golden values captured in hash_samples/type_annotation_golden.json; diff assertions verify builtins are unchanged and orcapod types changed to canonical names.
• tests/test_hashing/test_hash_utils.py — 12 new tests for canonical_annotation_str with registry.
• tests/test_hashing/test_semantic_hasher.py — 6 new tests for TypeObjectHandler with registry including a relocation-stability test.
• tests/test_hashing/test_function_info_extractors.py — 8 new tests in TestFunctionSignatureExtractorWithRegistry covering params, returns, generics, unions, relocation stability.

Backward compatibility

One-time cache invalidation for any cached function pod signature that contains op.File, op.Directory, op.Path, or op.UUID. Pre-v0.1.0 — no deprecation wrappers per CLAUDE.md.

Test plan

• uv run pytest tests/test_hashing/ — 666 passed, 31 skipped
• uv run pytest --ignore=tests/test_hashing/ — 4141 passed, 62 skipped, 2 xfailed

Closes ITL-638

[codecov]

Codecov Report

❌ Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/orcapod/hashing/hash_utils.py	93.33%	1 Missing ⚠️
...shing/semantic_hashing/function_info_extractors.py	96.42%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[eywalker]

do not use Any here -- specifically type it to the type registry or rather type converter as logical type registry is something that's hidden within the type converter

[kurodo3]

Fixed. Changed the parameter in TypeObjectHandler.__init__ to type_converter: TypeConverterProtocol | None. We also added get_logical_type(python_type: type) -> LogicalTypeProtocol | None to TypeConverterProtocol (mirroring the method that already existed on UniversalTypeConverter) so the handler can look up the canonical name without ever touching the registry object directly.

[kurodo3]

Fixed. The parameter was changed from logical_type_registry: Any to type_converter: TypeConverterProtocol | None — the registry is an implementation detail hidden inside TypeConverter, so the handler accepts the converter itself and calls type_converter.get_logical_type(obj) to resolve canonical names.

[eywalker]

use proper typing and do not default to grabbing default context. Aside from directly user-facing code, get_default_context should never be invoked and rather, one should be passed an explicit context object

[kurodo3]

Removed entirely. The lazy get_default_context() fallback has been deleted from both TypeObjectHandler and FunctionSignatureExtractor. Both now require explicit injection — TypeObjectHandler(type_converter=...) and FunctionSignatureExtractor(type_converter=...). The v0.1.json context spec now wires the type_converter ref directly into both handlers, so the default context wiring is explicit rather than implicit.

[kurodo3]

Fixed. The lazy _get_registry() fallback that called get_default_context().logical_type_registry has been deleted entirely. TypeObjectHandler now requires explicit injection: TypeObjectHandler(type_converter=tc). The default context wires it in via v0.1.json, so user-facing code remains unchanged.

[eywalker]

do not every use private property _logical_type_registry of type_converter. If there is any property/method that needs to access knowlege of logical type registry, this must be exposed via type_converter

[kurodo3]

Fixed. get_logical_type(python_type: type) -> LogicalTypeProtocol | None was already implemented on UniversalTypeConverter and is now added to TypeConverterProtocol as a proper public method. TypeObjectHandler calls type_converter.get_logical_type(obj) directly; canonical_annotation_str and _annotation_contains_registered_type also use it via the converter rather than touching the registry. No code outside of UniversalTypeConverter itself accesses _logical_type_registry anymore.

[kurodo3]

Fixed. get_logical_type(python_type: type) -> LogicalTypeProtocol | None was added to TypeConverterProtocol (the method already existed on UniversalTypeConverter). All logical-type lookups in the hashing layer go through this public method — _logical_type_registry is never accessed directly by callers.

[kurodo3]

Review round 2 — changes summary

All three comments addressed in commit b2becea.

No more Any typing — TypeObjectHandler and FunctionSignatureExtractor now take type_converter: TypeConverterProtocol | None instead of logical_type_registry: Any. register_builtin_python_type_handlers likewise takes type_converter: TypeConverterProtocol | None.

No more get_default_context() in internals — The lazy-fallback _get_registry() methods that called get_default_context() have been deleted from both handlers. Explicit injection is now the only path. The v0.1.json spec wires {"_ref": "type_converter"} into both TypeObjectHandler and FunctionSignatureExtractor configs.

No more private _logical_type_registry access — get_logical_type(python_type: type) -> LogicalTypeProtocol | None has been added to TypeConverterProtocol (the method already existed on UniversalTypeConverter). All logical-type lookups in the hashing layer go through this public method. canonical_annotation_str in hash_utils.py and _annotation_contains_registered_type in function_info_extractors.py both accept TypeConverterProtocol | None and call get_logical_type() directly — the registry object is never exposed to callers.

All 677 tests pass (0 failures, 31 skipped).

[eywalker]

isn't this implicitly assuming that the origin part cannot be the type that is "registered type"

[kurodo3]

You're right. The generic-alias branch was only iterating over __args__ and skipping __origin__ entirely — so if a registered type appeared as the origin of a generic alias, the function would silently return False. Fixed by also recursing into origin before checking args, consistent with how canonical_annotation_str already handles it. Added test_registered_type_as_generic_origin_detected to exercise this path directly.

[kurodo3]

Update: after fixing the origin check, we re-examined whether _annotation_contains_registered_type was the right approach at all. It wasn't — the whole string-conversion branch for return annotations was unnecessary and created an inconsistency: registered return types were stored as plain strings (e.g. 'orcapod.file') losing the 'type:' prefix, while unregistered types remained as type objects and went through TypeObjectHandler (producing 'type:builtins.float').

The fix: _annotation_contains_registered_type has been deleted entirely. FunctionSignatureExtractor now always stores parts['returns'] = ret_ann (the raw annotation object), exactly as it did originally. TypeObjectHandler — already updated with type_converter — handles canonicalisation at hash time uniformly for all return types, registered or not. No special casing in the extractor.

[kurodo3]

Review round 3 — changes summary

Single fix in commit 2088e70.

Bug: _annotation_contains_registered_type was only checking __args__ in the generic-alias branch, silently skipping __origin__. If a registered type appeared as the origin of a generic alias, the function would return False and the return annotation would be kept as a raw type object rather than canonicalized — a correctness gap.

Fix: Now also recurses into origin before checking args, making the logic consistent with canonical_annotation_str which already called canonical_annotation_str(origin, type_converter).

Test added: test_registered_type_as_generic_origin_detected in test_function_info_extractors.py constructs a fake generic alias with a registered orcapod type as __origin__ and unregistered builtins as __args__, asserting the function returns True.

[kurodo3]

Summary of latest changes (e4189ba)

This commit removes the _annotation_contains_registered_type helper and simplifies return-annotation handling in FunctionSignatureExtractor.

What changed and why:

During review of the round-3 fix (checking __origin__ in _annotation_contains_registered_type), we noticed a deeper inconsistency: the branch that detected registered types in return annotations was storing them as plain strings (e.g. "orcapod.file"), bypassing TypeObjectHandler and losing the "type:" prefix. Meanwhile, unregistered return types (e.g. float) still went through TypeObjectHandler, producing "type:builtins.float". The two code paths were inconsistent.

The correct approach (now implemented):

• FunctionSignatureExtractor always stores parts["returns"] = ret_ann — the raw annotation object, exactly as the original code did.
• TypeObjectHandler (already updated with type_converter in previous commits) canonicalises it at hash time, uniformly for all return types.
• _annotation_contains_registered_type is deleted; it had no other callers.

What is unchanged:

• Parameter annotations still go through canonical_annotation_str(annotation, type_converter) to replace the annotation substring in str(param) — this is correct because param annotations are embedded in strings, not handled by the type-handler registry.
• All 668 tests pass.

[Copilot]

🟡 Changes recommended

There are confirmed correctness/maintainability issues in the diff (unused import likely to break lint, and multiple docstrings/spec sections referencing non-existent APIs/behavior) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes ITL-638 by making semantic hashing of function-pod type annotations stable across internal module relocations, using canonical logical type names (via the type converter’s logical-type registry) instead of fully-qualified import paths.

Changes:

• Extend canonical_annotation_str to canonicalize registered logical types (and recurse through unions/generic aliases) using TypeConverterProtocol.get_logical_type(...).
• Update TypeObjectHandler and FunctionSignatureExtractor to use the type converter for canonical type identity during hashing (params canonicalized at extraction; returns remain raw and are canonicalized by TypeObjectHandler at hash time).
• Add golden/regression tests and committed golden fixtures to ensure builtins remain unchanged while registered orcapod logical types switch to canonical names.

File summaries

File	Description
tests/test_hashing/test_type_annotation_golden.py	Adds golden-diff regression tests ensuring only orcapod logical types change hashing behavior.
tests/test_hashing/test_semantic_hasher.py	Adds coverage for registry-aware TypeObjectHandler, including simulated module relocation stability.
tests/test_hashing/test_hash_utils.py	Adds tests for canonical_annotation_str(..., type_converter) covering registry resolution + recursion through unions/generics.
tests/test_hashing/test_function_info_extractors.py	Adds coverage that params are canonicalized while returns remain raw objects (hashed via TypeObjectHandler).
tests/test_hashing/hash_samples/type_annotation_golden.json	Stores pre-fix golden hashes for annotations/functions to detect unintended changes.
tests/test_hashing/hash_samples/schema_hash_golden.json	Stores post-fix stable schema hashes to lock in canonical behavior going forward.
tests/test_hashing/generate_type_annotation_golden.py	Adds a script to generate the pre-fix golden fixture.
superpowers/specs/2026-09-02-itl-638-stable-type-annotation-hashing-design.md	Design doc for the change (currently contains return-annotation details that conflict with implementation).
superpowers/plans/2026-09-02-itl-638-stable-type-annotation-hashing.md	Implementation plan artifact for ITL-638.
src/orcapod/protocols/semantic_types_protocols.py	Extends TypeConverterProtocol with get_logical_type(...) for registry-backed canonicalization.
src/orcapod/hashing/semantic_hashing/function_info_extractors.py	Canonicalizes parameter annotation substrings using canonical_annotation_str(..., type_converter); keeps return annotations as raw objects.
src/orcapod/hashing/semantic_hashing/builtin_handlers.py	Updates TypeObjectHandler to emit type:<logical_type_name> when resolvable via the type converter; threads type_converter through registration/default context wiring.
src/orcapod/hashing/hash_utils.py	Extends canonical_annotation_str with type_converter support + recursion through unions/generic aliases.
src/orcapod/contexts/data/v0.1.json	Wires type_converter into the configured FunctionSignatureExtractor and TypeObjectHandler in the default context.

Review details

Suppressed comments (2)

src/orcapod/hashing/semantic_hashing/builtin_handlers.py:475

• register_builtin_python_type_handlers claims canonical type-name resolution happens via get_logical_type_registry(), but the code path uses TypeConverterProtocol.get_logical_type(...). Adjust the docstring to reflect the actual API so downstream implementers of TypeConverterProtocol know which method is required.

        type_converter: Optional ``TypeConverterProtocol`` forwarded to
            ``TypeObjectHandler`` and ``FunctionSignatureExtractor`` for stable
            canonical type-name resolution via ``get_logical_type_registry()``.
            When ``None`` (the default), both handlers fall back to the raw
            ``"type:<module>.<qualname>"`` serialisation.

superpowers/specs/2026-09-02-itl-638-stable-type-annotation-hashing-design.md:165

• The concrete "After" example shows "returns": "orcapod.directory", but the implementation in this PR keeps return annotations as raw objects (op.Directory) and leaves canonicalisation to TypeObjectHandler at hash time. Update the example to avoid documenting behavior that doesn't exist.

# After
{
    "module": "mymodule",
    "name": "fn",
    "params": "f: orcapod.file, n: int",   # canonical name; int unchanged
    "returns": "orcapod.directory",         # canonical string, same path as params
}

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

---

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

[kurodo3]

Fixed in both locations. TypeObjectHandler docstring now says get_logical_type(obj) and register_builtin_python_type_handlers docstring says get_logical_type() — matching the actual call in handle().

[kurodo3]

Fixed throughout the spec. The return-annotation section now describes the actual behavior: parts['returns'] stays as a raw type object, and TypeObjectHandler canonicalises it at hash time via type_converter.get_logical_type(). The 'After' concrete example, the scope description, the goals statement, and the Dependencies & Risks note about 'returns becoming a string' are all updated to match the implementation.

[kurodo3]

Review round summary (795cfe9)

Three issues raised by Copilot, all fixed in a single commit:

Issue	Fix
Unused is_union_annotation import in function_info_extractors.py (F401)	Removed from import line
TypeObjectHandler docstring references non-existent get_logical_type_registry()	Updated to get_logical_type(obj)
register_builtin_python_type_handlers docstring same stale method name	Updated to get_logical_type()
Spec describes return annotations becoming strings; implementation keeps them as raw objects	Updated 5 sections of the spec: goals, scope, return-annotation code block, concrete 'After' example, and the Dependencies & Risks note

No logic changes — docs and imports only. All 669 tests still pass.

[eywalker]

Rather than replacing string, wouldn't it better for the annotation per parameter to be replaced cleanly and then reconstruct the full function string rather than doing substitution? Technically string substitution can accidentally replace part of the function string that it's not meant to replace

[kurodo3]

Done. Replaced the str(param).replace() / split('=') approach with _format_param(), a module-level helper that reconstructs each parameter string directly from inspect.Parameter's structured attributes: name, kind, canonical annotation, and default.

This eliminates both failure modes you identified:

1. Annotations containing '=' (e.g. Literal["a=b"]) were being corrupted by split('=')[0] when include_defaults=False — now we check param.default is not inspect.Parameter.empty directly.
2. A default value whose repr contained ': ' as a substring could have caused unexpected matches — now annotation and default are assembled independently, never searched/replaced.

Output format is identical to str(inspect.Parameter) for all normal inputs, so no hash change for existing signatures. Two regression tests added to verify both failure modes are caught.

[kurodo3]

Agreed — done. Replaced the and approach with a new helper that reconstructs each parameter string from structured inspect.Parameter attributes. No string search/replace anywhere in the param building path.

The helper uses param.kind, param.name, param.default, and the pre-computed canonical_annotation directly, so:

• The annotation and default value are assembled as separate parts — a default whose repr contains ': <annotation>' cannot accidentally trigger substitution.
• Stripping defaults uses param.default is not inspect.Parameter.empty, not split('='), so annotations containing = (e.g. Literal["a=b"]) are preserved correctly.

Output is identical to str(inspect.Parameter) for all real-world inputs, so no existing hash values change.

[kurodo3]

Review round summary (e3c0c14)

Comment: String substitution in param building can accidentally replace the wrong text.

Fix: Replaced str(param).replace(':old_ann', ':new_ann', 1) and split('=')[0] with a new _format_param() helper that reconstructs each parameter string from structured inspect.Parameter attributes — no string search/replace anywhere in the param building path.

Two actual bugs fixed in the process:

Bug	Old behavior	New behavior
Annotation containing = (e.g. Literal["a=b"]) with include_defaults=False	split('=')[0] truncated annotation to Literal["a	Correctly strips only param.default, leaving annotation intact
Default value whose repr contains ': <annotation>'	Could produce unexpected substitution	Annotation and default assembled from separate attributes, never searched

Output format is identical to str(inspect.Parameter) for all real-world inputs — no hash change for existing function signatures.

Two regression tests added to TestFunctionSignatureExtractor:

• test_annotation_containing_equals_preserved_when_defaults_stripped
• test_default_value_repr_containing_annotation_substring_not_duplicated

[kurodo3]

Review round summary (bca94e0)

Tightened test assertions to exact equality per review feedback that checking string phrases with in is too loose.

What changed

All in/not in substring checks in TestFunctionSignatureExtractor and TestFunctionSignatureExtractorWithRegistry have been replaced with exact == assertions against the full expected params string:

Test	Before	After
test_include_defaults_false_strips_defaults	"x: int" in result["params"] etc.	result["params"] == "x: int, y: str"
test_include_defaults_true_keeps_defaults	substring check	result["params"] == "x: int = 42"
test_varargs_and_kwargs_with_annotations	substring check	result["params"] == "*args: int, **kwargs: str"
test_keyword_only_param_formatted_without_star_prefix	substring check	result["params"] == "a: int, b: str"
test_union_annotation_canonicalized	substring check	`result["params"] == "x: pathlib.Path
test_param_annotation_is_canonical_string	substring check	info["params"] == "f: orcapod.file"
test_generic_param_annotation_canonical	substring check	info["params"] == "files: list[orcapod.file]"
test_builtin_annotations_unchanged	substring check	info["params"] == "x: int, y: str"
test_param_canonical_string_and_return_type_object	substring check	info_param["params"] == "f: orcapod.file"

One deliberate exception

test_annotation_containing_equals_preserved_when_defaults_stripped remains a compound check rather than exact equality. This is because from __future__ import annotations at the top of the test file causes inspect.signature(fn, eval_str=True) to try resolving Literal["a=b"] in the function's __globals__ — but Literal is only imported inside the test method's local scope, so the fallback is triggered and formatannotation wraps the annotation string in an extra layer of quotes. An exact assertion would be asserting a CPython fallback artifact rather than the intended behavior. The compound check still verifies the two invariants that matter: the full annotation is present and the default value is absent.

All 32 tests pass.

[kurodo3]

Review round summary (6ec4aad)

Added missing test coverage for and canonicalization.

is and is — both registered orcapod logical types. With the registry wired in, param annotations using these types should be canonicalized to their logical type names, not their module paths. This was working correctly (the implementation already handled it via ) but was never explicitly tested.

Three new tests in :

Test	Assertion
	p: pathlib.Path → "p: orcapod.path"
	u: uuid.UUID → "u: orcapod.uuid"
	str | Path → "x: orcapod.path | str" (sorting preserved post-canonicalization)

Note: the existing test_union_annotation_canonicalized (no registry) correctly shows "pathlib.Path | str" — that's the expected output when no type_converter is wired in.

[kurodo3]

Review round summary (c043922)

Added pathlib.Path and uuid.UUID in the return position.

Two groups of new tests:

test_function_info_extractors.py — test_path_return_annotation_type_object_hashes_to_canonical

Two-step round-trip covering the full chain for pathlib.Path in return position:

1. Extractor stores the raw pathlib.Path type object in parts["returns"] (not a string — verified with is Path)
2. TypeObjectHandler(type_converter=tc).handle(returns_val, hasher) produces "type:orcapod.path" — the canonical form — not "type:pathlib.Path" (the old module-path form)

test_semantic_hasher.py — TestTypeObjectHandlerWithRegistry

Two new cases alongside the existing op.File / op.Directory coverage, completing all four orcapod logical types that are registered over stdlib classes:

• pathlib.Path → "type:orcapod.path"
• uuid.UUID → "type:orcapod.uuid"

[kurodo3]

Review round — thread reply catch-up

Three eywalker comment threads from the first CHANGES_REQUESTED review (2026-09-02) had not received individual replies, only coverage in the batch summary. Replied to all three now:

Thread	Comment	Reply summary
builtin_handlers.py:96	Do not use Any	Parameter renamed to type_converter: TypeConverterProtocol | None; registry stays hidden inside the converter
builtin_handlers.py:102	Do not grab default context	Lazy _get_registry() fallback deleted; explicit injection required
builtin_handlers.py:104	Do not access _logical_type_registry	get_logical_type() added to TypeConverterProtocol; no private access anywhere

All code changes were already in place from commit b2beceaa (round 2). This round is thread-reply only — no code changes.