Skip to content

fix: use subordinate_id/supervisor_id in HierarchyResolver#751

Merged
Aureliolo merged 4 commits intomainfrom
feat/implement-746
Mar 22, 2026
Merged

fix: use subordinate_id/supervisor_id in HierarchyResolver#751
Aureliolo merged 4 commits intomainfrom
feat/implement-746

Conversation

@Aureliolo
Copy link
Copy Markdown
Owner

Summary

  • Add subordinate_key/supervisor_key @computed_field properties to ReportingLine that return subordinate_id/supervisor_id when set, falling back to the role name
  • Update HierarchyResolver.__init__ to use these properties as dict keys in the explicit reporting lines loop, so agents sharing a role name but with different IDs are properly distinguished
  • Add 11 tests: 4 for computed properties, 7 for hierarchy resolution with IDs (disambiguation, chain-walking, cycle detection, override, backward compatibility)

Test plan

  • uv run python -m pytest tests/unit/core/test_company_reporting.py tests/unit/communication/delegation/test_hierarchy.py -n auto -v -- all 91 tests pass
  • uv run mypy src/ tests/ -- clean
  • uv run ruff check src/ tests/ -- clean
  • Full test suite: 10,382 passed, 93.68% coverage
  • Pre-reviewed by 9 agents (code-reviewer, python-reviewer, test-analyzer, type-design, logging-audit, resilience-audit, conventions-enforcer, docs-consistency, issue-verifier), 4 findings addressed

🤖 Generated with Claude Code

Closes #746

Aureliolo and others added 2 commits March 22, 2026 21:29
HierarchyResolver used bare role names as dict keys when processing
explicit reporting lines, collapsing agents that share a role name
into a single entry. Add subordinate_key/supervisor_key computed
properties to ReportingLine and use them as dict keys so agents
with different IDs are properly distinguished.

Closes #746

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Update HierarchyResolver docstring to reflect subordinate_key/supervisor_key
usage. Strengthen override test with distinct member name vs ID. Add
chain-walking test with ID keys and cycle detection test with ID keys.

Pre-reviewed by 9 agents, 4 findings addressed

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 22, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: df2e4aac-0ca6-4a73-adbc-c30e9ae26587

📥 Commits

Reviewing files that changed from the base of the PR and between 3d3e958 and 9b66400.

📒 Files selected for processing (4)
  • src/synthorg/communication/delegation/hierarchy.py
  • src/synthorg/core/company.py
  • tests/unit/communication/delegation/test_hierarchy.py
  • tests/unit/core/test_company_reporting.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Test (Python 3.14)
  • GitHub Check: Build Sandbox
  • GitHub Check: Build Backend
  • GitHub Check: Build Web
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: No from __future__ import annotations -- Python 3.14 has PEP 649 native lazy annotations.
Use PEP 758 except syntax: except A, B: (no parentheses) -- ruff enforces this on Python 3.14.
Type hints: all public functions, mypy strict mode.
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules).
Immutability: create new objects, never mutate existing ones. For non-Pydantic internal collections, use copy.deepcopy() at construction + MappingProxyType wrapping for read-only enforcement. For dict/list fields in frozen Pydantic models, rely on frozen=True and copy.deepcopy() at system boundaries.
Config vs runtime state: frozen Pydantic models for config/identity; separate mutable-via-copy models for runtime state that evolves. Never mix static config fields with mutable runtime fields.
Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use @computed_field for derived values. Use NotBlankStr for all identifier/name fields instead of manual whitespace validators.
Async concurrency: prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code. Prefer structured concurrency over bare create_task.
Line length: 88 characters (ruff).
Functions: < 50 lines, files < 800 lines.
Errors: handle explicitly, never silently swallow.
Validate: at system boundaries (user input, external APIs, config files).
Variable name: always logger (not _logger, not log).

Files:

  • tests/unit/core/test_company_reporting.py
  • src/synthorg/communication/delegation/hierarchy.py
  • src/synthorg/core/company.py
  • tests/unit/communication/delegation/test_hierarchy.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.e2e, @pytest.mark.slow
Parametrize: Prefer @pytest.mark.parametrize for testing similar cases.
Tests must use test-provider, test-small-001, etc. for vendor-agnostic test code.
Property-based testing: Python uses Hypothesis (@given + @settings). Hypothesis profiles: ci (50 examples, default) and dev (1000 examples), controlled via HYPOTHESIS_PROFILE env var.
Flaky tests: NEVER skip, dismiss, or ignore -- always fix them fully and fundamentally. For timing-sensitive tests, mock time.monotonic() and asyncio.sleep() to make them deterministic. For tasks that must block indefinitely until cancelled, use asyncio.Event().wait() instead of asyncio.sleep(large_number).

Files:

  • tests/unit/core/test_company_reporting.py
  • tests/unit/communication/delegation/test_hierarchy.py
src/synthorg/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/synthorg/**/*.py: Every module with business logic MUST have: from synthorg.observability import get_logger then logger = get_logger(__name__)
Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT
Structured kwargs: always logger.info(EVENT, key=value) -- never logger.info("msg %s", val)
All error paths must log at WARNING or ERROR with context before raising.
All state transitions must log at INFO.
DEBUG for object creation, internal flow, entry/exit of key functions.
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names: example-provider, example-large-001, example-medium-001, example-small-001, large/medium/small as aliases.
Pure data models, enums, and re-exports do NOT need logging.
Use NotBlankStr from core.types for all identifier/name fields -- including optional (NotBlankStr | None) and tuple variants -- instead of manual whitespace validators.

Files:

  • src/synthorg/communication/delegation/hierarchy.py
  • src/synthorg/core/company.py
src/synthorg/!(observability)/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Never use import logging / logging.getLogger() / print() in application code (exception: observability/setup.py and observability/sinks.py may use stdlib logging and print for bootstrap).

Files:

  • src/synthorg/communication/delegation/hierarchy.py
  • src/synthorg/core/company.py
🧠 Learnings (9)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 BaseModel, model_validator, computed_field, ConfigDict.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-15T18:42:17.990Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:42:17.990Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-22T19:37:56.694Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T19:37:56.694Z
Learning: Applies to **/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values. Use `NotBlankStr` for all identifier/name fields instead of manual whitespace validators.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state

Applied to files:

  • src/synthorg/core/company.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.

Applied to files:

  • src/synthorg/core/company.py
🧬 Code graph analysis (3)
tests/unit/core/test_company_reporting.py (1)
src/synthorg/core/company.py (3)
  • ReportingLine (40-126)
  • subordinate_key (71-79)
  • supervisor_key (83-91)
src/synthorg/communication/delegation/hierarchy.py (1)
src/synthorg/core/company.py (2)
  • subordinate_key (71-79)
  • supervisor_key (83-91)
tests/unit/communication/delegation/test_hierarchy.py (2)
src/synthorg/core/company.py (3)
  • Department (340-445)
  • ReportingLine (40-126)
  • Team (306-337)
src/synthorg/communication/delegation/hierarchy.py (1)
  • HierarchyResolver (16-285)
🔇 Additional comments (6)
src/synthorg/core/company.py (1)

69-91: Computed identity keys are implemented correctly and align with resolver usage.

The new subordinate_key / supervisor_key properties cleanly prefer IDs with a safe fallback to names, and the implementation is consistent with the model’s validation behavior.

src/synthorg/communication/delegation/hierarchy.py (2)

67-77: Key-based hierarchy indexing and override flow look correct.

Switching to subordinate_key / supervisor_key here fixes the ID-collision problem at the resolver root while preserving explicit-line override precedence.


145-276: Public API docs now correctly reflect name-or-ID inputs.

The updated docstrings are consistent with the resolver’s new key semantics and reduce ambiguity for callers.

tests/unit/core/test_company_reporting.py (1)

119-163: Great coverage for computed key semantics and serialization round-trip.

These tests exercise both ID-preferred and fallback behavior, and they verify the computed fields remain stable across dump/validate flow.

tests/unit/communication/delegation/test_hierarchy.py (2)

422-643: ID-based hierarchy behavior is thoroughly validated.

The new suite covers the critical disambiguation and override paths introduced by the resolver key change, including backward-compatible name-only behavior.


659-682: Good addition: cycle detection is now explicitly tested for ID-key graphs.

This closes an important regression gap for the new identity-key path.


Walkthrough

Hierarchy resolution now keys reporting relationships by identity rather than by name. ReportingLine gains computed properties subordinate_key and supervisor_key that return *_id when present, else fall back to subordinate/supervisor. HierarchyResolver was changed to consume these keys when building and updating the supervisor/direct-reports mappings and when performing overrides. Unit tests were added/extended to verify ID-based distinctness, override semantics, cycle detection with IDs, and backward compatibility when IDs are absent.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: use subordinate_id/supervisor_id in HierarchyResolver' accurately summarizes the main change, which is updating HierarchyResolver to use the new ID fields for proper agent identification.
Description check ✅ Passed The description is related to the changeset, providing a clear summary of changes (computed properties, HierarchyResolver updates, test additions) and test plan results.
Linked Issues check ✅ Passed The PR fully addresses issue #746: adds subordinate_key/supervisor_key computed properties to ReportingLine [746], updates HierarchyResolver to use these properties as dictionary keys [746], and includes comprehensive tests validating ID-based disambiguation and backward compatibility [746].
Out of Scope Changes check ✅ Passed All changes are directly within scope: adding computed properties to ReportingLine, updating HierarchyResolver to use them, and adding tests for the new functionality. No unrelated refactoring or feature additions are present.
Docstring Coverage ✅ Passed Docstring coverage is 92.86% which is sufficient. The required threshold is 40.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the hierarchy resolution logic by incorporating subordinate and supervisor IDs in addition to role names. This change ensures that agents with the same role name but different IDs are correctly distinguished in the reporting hierarchy. The update includes new computed properties and corresponding tests to validate the new functionality and maintain backward compatibility.

Highlights

  • Reporting Line Enhancement: Added subordinate_key and supervisor_key computed properties to the ReportingLine model, enabling the use of IDs instead of names for identifying subordinates and supervisors.
  • Hierarchy Resolution Update: Updated the HierarchyResolver to utilize the new subordinate_key and supervisor_key properties when processing explicit reporting lines, ensuring accurate disambiguation of agents with shared role names but distinct IDs.
  • Comprehensive Testing: Introduced 11 new tests covering the computed properties and hierarchy resolution with IDs, including disambiguation, chain-walking, cycle detection, override, and backward compatibility scenarios.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 22, 2026 20:57 — with GitHub Actions Inactive
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses the issue of distinguishing agents with identical role names by leveraging subordinate_id and supervisor_id. The introduction of subordinate_key and supervisor_key as computed properties on the ReportingLine model is a clean and appropriate solution. The HierarchyResolver is correctly updated to use these new keys, and the accompanying tests are comprehensive, covering various scenarios including disambiguation, overrides, and cycle detection. I have one minor suggestion to improve the clarity and efficiency of the code in HierarchyResolver.

Comment on lines 73 to +74
old_reports = reports_of.get(old_sup, [])
reports_of[old_sup] = [
r for r in old_reports if r != line.subordinate
]
supervisor_of[line.subordinate] = line.supervisor
reports_of.setdefault(line.supervisor, []).append(
line.subordinate,
)
reports_of[old_sup] = [r for r in old_reports if r != sub_key]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The list comprehension used to remove the subordinate from their old supervisor's list of reports is functional but can be made more direct and readable. Since the logic ensures that sub_key will be present in reports_of[old_sup], you can use list.remove() to modify the list in-place. This more clearly expresses the intent of removing an element and avoids creating a new list.

Suggested change
old_reports = reports_of.get(old_sup, [])
reports_of[old_sup] = [
r for r in old_reports if r != line.subordinate
]
supervisor_of[line.subordinate] = line.supervisor
reports_of.setdefault(line.supervisor, []).append(
line.subordinate,
)
reports_of[old_sup] = [r for r in old_reports if r != sub_key]
reports_of[old_sup].remove(sub_key)

coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 22, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.29%. Comparing base (b7dd7de) to head (9b66400).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   92.28%   92.29%   +0.01%     
==========================================
  Files         573      573              
  Lines       29690    29704      +14     
  Branches     2877     2879       +2     
==========================================
+ Hits        27398    27414      +16     
+ Misses       1811     1810       -1     
+ Partials      481      480       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Document normalization asymmetry between _identity_key and computed
  keys in subordinate_key/supervisor_key docstrings
- Fix HierarchyResolver class docstring: "resolve to" instead of
  "prefer"
- Update all public method Args docs to say "name or ID"
- Fix misleading comment in override test (alice is not "distinct")
- Add assertions for old supervisor reports removal in override test
- Add negative assertion for raw role name when IDs are used
- Add get_lowest_common_manager test with ID-based keys
- Add head_id-not-used-by-resolver documentation test
- Add serialization round-trip test for computed fields

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@Aureliolo Aureliolo merged commit 118235b into main Mar 22, 2026
33 checks passed
@Aureliolo Aureliolo deleted the feat/implement-746 branch March 22, 2026 21:35
@Aureliolo Aureliolo temporarily deployed to cloudflare-preview March 22, 2026 21:35 — with GitHub Actions Inactive
Aureliolo added a commit that referenced this pull request Mar 22, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.4.8](v0.4.7...v0.4.8)
(2026-03-22)


### Features

* add auto_cleanup config and improve update UX
([#741](#741))
([289638f](289638f))
* add reporting lines, escalation paths, and workflow handoffs to
templates ([#745](#745))
([c374cc9](c374cc9))
* differentiate template operational configs
([#742](#742))
([9b48345](9b48345))
* diversify personality preset assignments across templates
([#743](#743))
([15487a5](15487a5))
* improve template metadata -- skill taxonomy, descriptions, tags, and
display names ([#752](#752))
([f333f24](f333f24))


### Bug Fixes

* resolve log analysis findings (Ollama prefix, logging, init)
([#748](#748))
([8f871a4](8f871a4))
* use git tag for dev release container image tags
([#749](#749))
([f30d071](f30d071))
* use subordinate_id/supervisor_id in HierarchyResolver
([#751](#751))
([118235b](118235b))


### Performance

* add long-lived cache headers for content-hashed static assets
([#747](#747))
([4d350b5](4d350b5))
* use worksteal distribution for pytest-xdist
([#750](#750))
([b7dd7de](b7dd7de))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: HierarchyResolver does not consume subordinate_id/supervisor_id from ReportingLine

1 participant