fix(agent-os): report a non-integer supervisor level instead of governing it

[LHMQ878]

Problem

#3510 merged at a1ba0eb5, one commit before the correction I pushed for it. Two things were left behind.

1. The merged commit message states something that was disproved during review. ec3f7661 in main says:

Previously accepted configurations are unchanged: level 0 deterministic, and agent-based supervisors at levels 1..N.

That was my claim and it is wrong. liamcrumm tested it instead of reading it and found the counterexample: validate_supervisor accepted level=0.0, is_agent=False on the old code and rejects it now, because isinstance(level, int) excludes floats. Checking after the merge, it is broader than the one case he named — every float level was accepted before:

BEFORE #3510, validate_supervisor:      AFTER (now in main):
  True   level=0.0, is_agent=False        False   <-- liamcrumm's case
  True   level=1.0, is_agent=True         False
  True   level=2.5, is_agent=True         False
  True   level=-1                         False   (the vulnerability #3510 closed)
  True   level='0'                        False
  True   level=True                       False

I can't rewrite merged history, so this PR is the place that record gets corrected.

2. Nothing in the suite records why that narrowing is acceptable. It is acceptable — but the reason is not obvious, and the next person to read isinstance(level, int) will see a type check that rejects 0.0 and reasonably wonder whether a YAML config that writes level: 0.0 used to work.

Why it never worked

A float level could never reach a working hierarchy. validate_hierarchy's gap scan computes range(1, max_level + 1), which does not accept a float:

$ # against main as merged, a lone level=0.0 root, is_agent=False
level=0.0: TypeError: 'float' object cannot be interpreted as an integer
level=1.0: TypeError: 'float' object cannot be interpreted as an integer
level=2.5: TypeError: 'float' object cannot be interpreted as an integer

So before #3510 a float got past validate_supervisor and then crashed at the next step; it was never governed. #3510 converted an unhandled TypeError into a False, which is a strictly better failure for the same input. It did not narrow the set of hierarchies that ever validated — which is the accurate version of the claim my commit message made.

Change

This started as a test-only PR. MohammadHaroonAbuomar's review turned it into a source fix, because the gap he named is wider than a missing type check — and it disproved this PR's own original premise.

range(1, max_level + 1) only raises when the float is the maximum, which was the only shape the first version's test covered. Register any higher integer level and the gap scan never sees the float, and no other rule reaches it either: level == 0 is False for 0.0's neighbours, level < 0 is False for 0.5, and the level is absent from range. Measured against the pre-fix method:

root=0 (det) + drifted=1.0 + top=2   -> violations == []
root=0 (det) + drifted=0.5 + top=1   -> violations == []
root=False   (bool)                  -> violations == []
root='0'     (str)                   -> TypeError out of `level < 0`

The 0.5 case is the one that matters: a supervisor sits between the root and level 1, is governed by neither, and the hierarchy reports as valid.

So validate_hierarchy now checks the level type first and returns early, which also stops the later rules from comparing or ranging over a value they cannot handle. bool is excluded explicitly, matching the reasoning already in TrustRoot.validate_supervisor — it subclasses int and False == 0, so it would otherwise be read as the deterministic root.

agent_os/supervisor.py +22, tests/test_trust_root.py rewritten (+80/-14). The docstring that claimed a float "crashed at the next step rather than being governed" was generalising from the lone-float case, so the test now asserts the violation and a second parametrized case covers the shape that claim missed — the record is the measurement rather than the inference.

If a YAML 0.0 should keep working

I'd argue the right place is coercion at the config-loading boundary — int(level) when float(level).is_integer(), rejecting otherwise — not a wider type check in the invariant, since the invariant's job is to name the malformed input and range() will refuse a float regardless. That is a different layer from #3510 and I have not done it here. Happy to open it separately if you want it.

Verification

Pre-fix, 10 of the 11 new assertions fail. The eleventh is an all-integer hierarchy asserting violations == [], which passes either way on purpose: the check must not add a violation to a hierarchy that already validated.

tests/test_trust_root.py cannot be collected on my machine — ImportError: cannot import name '_native' from partially initialized module 'agent_control_specification', a compiled extension I do not have. It fails identically on main with this branch's changes stashed, so it is environmental, but it does mean I have not run the file as a suite. The assertions above were measured by binding the real validate_hierarchy (before and after) to a stand-in holding the same _supervisors list.

ruff check --select E,F,W --ignore E501 and ruff format --check clean on both files; cspell over the added lines reports 0 issues.

Thanks again to liamcrumm for checking the claim rather than taking it.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[github-actions]

🔴 Contributor Check: HIGH

Check	Result
Profile	HIGH
Credential	LOW
Overall	HIGH

Automated check by AGT Contributor Check.

[Copilot]

Pull request overview

TL;DR: 0 blockers, 0 warnings. No issues found. Clean change.

This PR adds a targeted regression test in the agent-os test suite to document (and make executable) the rationale that float level values never produced a working SupervisorHierarchy due to validate_hierarchy()’s gap scan relying on range(...), which rejects floats—supporting the stricter TrustRoot.validate_supervisor type check introduced in #3510.

Changes:

• Add a parametrized test asserting that float hierarchy levels trigger a TypeError when validating the hierarchy.
• Add an explanatory docstring capturing why rejecting float level values is not a behavioral regression for “working” hierarchies.

[MohammadHaroonAbuomar]

Minor:

• underlying gap, fine as a follow-up: register_supervisor never validates and validate_hierarchy has no level-type check, so non-integer levels under an integer max are silently governed with violations == []. Inconsistent with #3510's integer-level invariant.

[MohammadHaroonAbuomar]

BLOCKER: the claim this test makes executable is false. The TypeError in validate_hierarchy's gap scan only fires when the float is the MAXIMUM registered level, and the test only ever registers a lone float supervisor. Counterexamples (verified by running both PR head and pre-#3510 ec3f766~1): {level=0.0 root, level=1 agent} passed validate_supervisor AND validate_hierarchy() == [] before #3510, and validate_hierarchy() is still [] at head; {0 root, 0.5 agent, 1 agent} also yields [] at head. So a float level DID reach a working hierarchy, and #3510 DID narrow a previously-working config, the exact YAML 0.0-root case the docstring dismisses. Either rescope honestly (rename to the lone-float-root / float-max case and rewrite the docstring) or add the multi-level counterexample and state that #3510 narrowed it.

[MohammadHaroonAbuomar]

the test never calls validate_supervisor (register_supervisor appends without validating), so it does not regression-guard #3510: reverting the fix leaves these 3 cases green (verified by mutation). Add an assertion that validate_supervisor rejects the float level so the test pins the fix it cites.

[LHMQ878]

MohammadHaroonAbuomar you're right, and I'd tested the wrong shape. 38f5dc11 fixes it.

Registering the float alone is the only case my test covered, and that is exactly the case where it is the maximum and range(1, max_level + 1) raises. Put any higher integer level in the hierarchy and the gap scan never sees the float. Measured against the pre-fix method:

root=0 (det) + drifted=1.0 + top=2   -> violations == []
root=0 (det) + drifted=0.5 + top=1   -> violations == []
root=0.0(det) + top=1                -> ["Level 0 supervisor 'r' must be deterministic..."]  (wrong reason)
root=False (bool)                    -> violations == []
root='0'   (str)                     -> TypeError out of `level < 0`

0.5 is the one that bothers me: a supervisor sits between the root and level 1, level == 0 is False, level < 0 is False, 0.5 not in range(1, 2) — so it is governed by nothing and the hierarchy reports as valid. That is not a "silently governed with violations == []" edge case in the abstract; it is a supervisor outside the chain that get_authority_chain still returns.

So this is no longer a follow-up and no longer a test-only PR. validate_hierarchy now checks the level type first and returns early:

for s in self._supervisors:
    if isinstance(s.level, bool) or not isinstance(s.level, int):
        violations.append(
            f"Supervisor '{s.name}' has non-integer level {s.level!r}; "
            "levels must be integers"
        )
if violations:
    return violations

Returning early is deliberate: the rules below compare and range over the level, so letting a str through means raising instead of reporting. bool is excluded explicitly for the reason already written into TrustRoot.validate_supervisor — it subclasses int and False == 0, so it would otherwise be read as the deterministic root.

I did not touch register_supervisor. It appends to a list and has no other validation, so raising there would be a new failure mode for callers that currently build a hierarchy and then validate it; validate_hierarchy is the method whose contract is "return the violations". Happy to add a declaration-time check too if you'd rather it fail earlier.

This also corrects the PR's own claim

The docstring I wrote said a float "crashed at the next step rather than being governed". That generalised from the lone-float case and is wrong for the shape you named. The test is rewritten to assert the violation, and a second parametrized case covers what the old docstring missed — so the record is a measurement now, not an inference from one example.

Discrimination

Pre-fix, 10 of the 11 new assertions fail. The eleventh asserts violations == [] for an all-integer hierarchy and passes either way on purpose: the check must not add a violation to a hierarchy that already validated.

One caveat I want to be explicit about: the agent-os suite does not run on my machine — tests/test_trust_root.py imports agent_control_specification, whose _native module is a build artifact not in the tree. The numbers above were measured by binding the real validate_hierarchy (at HEAD and after) to a stand-in holding the same _supervisors list, since that method never touches self.trust_root. CI is the authority on the pytest run.

ruff check --select E,F,W --ignore E501 and ruff format --check clean on both files; cspell over the added lines reports 0 issues.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

agent-governance-python/agent-os/src/agent_os/supervisor.py:66

• The PR description states "Tests only — no source change", but this diff modifies production code by adding a new non-integer level rule to SupervisorHierarchy.validate_hierarchy. Please update the PR description (and/or title) to reflect that this is a behavior change in agent_os/supervisor.py, not just a test addition.

    def validate_hierarchy(self) -> list[str]:
        """Check hierarchy rules and return a list of violations (empty = valid).

        Rules:
        - Every level MUST be a real integer.
        - No supervisor may sit above the root: levels MUST NOT be negative.
        - Level 0 MUST exist and MUST be deterministic (not an LLM agent).

[LHMQ878]

Status update, since #3510 merging changed what this PR is:

This branch was stacked on #3510, which landed as ec3f7661. The diff here is now standalone — 2 files, +93/-0, both agent-os:

agent-governance-python/agent-os/src/agent_os/supervisor.py   +22/-0
agent-governance-python/agent-os/tests/test_trust_root.py     +71/-0

All 11 checks green on 38f5dc11, mergeable: true.

MohammadHaroonAbuomar the CHANGES_REQUESTED on this PR is against 28aca3f3, which is no longer the head — the type check you asked for went in as 38f5dc11, along with the correction to the test docstring that had overclaimed. It needs a dismissal or a re-review to unblock. No action needed from me that I can see, but say the word if you'd like the declaration-time check in register_supervisor as well and I'll add it here rather than as a follow-up.

[LHMQ878]

Copilot is right and that was my miss — the description still said "Tests only — no source change" from before MohammadHaroonAbuomar's review turned this into a source fix in 38f5dc11. Description updated to describe the actual diff (supervisor.py +22, test_trust_root.py +80/-14), including the pre-fix measurement and the discrimination count.

Worth flagging that its "low confidence" note was the accurate one here: a stale description on a PR that grew a source change is exactly the kind of thing worth surfacing, and suppressing it meant it nearly went unsaid.

MohammadHaroonAbuomar — to close the loop on the follow-up you offered to defer: register_supervisor still accepts anything, so the declaration-time check remains unbuilt. I left it out deliberately rather than by omission. validate_hierarchy is the place the invariant is stated, so a value that cannot satisfy it should be named there first; adding a second gate at registration is a real improvement but it changes when callers see the error, which is a behaviour change for anyone who registers-then-validates. Happy to open it separately if you'd like it — the shape would be a TypeError at register_supervisor with validate_hierarchy's check kept as the invariant's own statement.

Status: 11 checks green, mergeable: true. Your CHANGES_REQUESTED is on 28aca3f3, two commits back — the head is 38f5dc11.

[LHMQ878]

MohammadHaroonAbuomar Thanks. Half of that is already closed at this head (38f5dc11) and half I agree is a follow-up.

validate_hierarchy does have a level-type check now — it's the first rule, and it returns early so the later rules never compare or range over a value they can't handle:

        for s in self._supervisors:
            if isinstance(s.level, bool) or not isinstance(s.level, int):
                violations.append(
                    f"Supervisor '{s.name}' has non-integer level {s.level!r}; "
                    "levels must be integers"
                )
        if violations:
            return violations

So the exact scenario you describe — a non-integer level under an integer max — no longer yields violations == []. That was the whole point of the PR: the comment above the check records why the gap scan couldn't see it (range(1, max_level + 1) raises on a float that is the max, but a float below an integer max was simply never visited).

bool is excluded explicitly since bool subclasses int and False == 0 would otherwise read as the root level — matching TrustRoot.validate_supervisor, which is the consistency with #3510's integer-level invariant you're asking for.

register_supervisor never validating — agreed, and it's a genuine follow-up. Validation is deferred to validate_hierarchy today, so a bad level is accepted at registration and only reported later, which means anything reading self._supervisors before validation still sees it. I'd rather not fold that in here: making register_supervisor raise is an API behaviour change for every caller, and this PR is a reporting fix. Happy to open a tracking issue for it, or to add it here if you'd prefer it in one change — your call.

One note on verification: I can't run test_trust_root.py locally on this machine, because agent_control_specification needs the compiled _native extension (ImportError: cannot import name '_native' ... partially initialized module). So I'm relying on CI for this one, which is 12 success / 0 failures on 38f5dc11, Test Coverage included.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[LHMQ878]

You were right on the first point, and the rescope you asked for landed in
38f5dc1 — pushed after this review, so it may not have been visible yet.

The test that made the false claim is now
test_a_float_level_is_reported_when_it_is_the_maximum, named for the case it
actually covers, and the docstring no longer dismisses the multi-level shape. The
counterexample you supplied is a test of its own,
test_a_float_level_under_an_integer_maximum_is_reported, parametrized over
(0.0, 1), (1.0, 2), (0.5, 1), (1.5, 2), (-1.0, 1) — a float supervisor
plus a higher integer level, which is exactly the shape where
range(1, max_level + 1) never sees the float. Its docstring states plainly that
these returned violations == [] before the check, i.e. that #3510 narrowed a
previously-working config.

The -1.0 row is there for the reason you'd expect: the negative-level rule did
fire for it, but on a value the hierarchy cannot order, so naming the type is the
more accurate violation.

Your second point is the more interesting one, and it stands even at that head:
nothing here called validate_supervisor, so the two layers were never asserted
to agree. register_supervisor sits between them and calls neither. I've added
test_the_two_layers_agree_about_a_non_integer_level (f928519), over the same
parametrize list the existing validate_supervisor test uses
('0', '1', 0.0, 1.5, [0], True, False), asserting both answers for
each value: validate_supervisor returns False, and after register_supervisor
the hierarchy reports a non-integer violation naming that supervisor.

Mutation-checked in both directions rather than assumed:

revert this PR's check in validate_hierarchy   -> 7 of 7 cases fail
revert #3510's check in validate_supervisor    -> 5 of 7 cases fail

(True and False still fail the determinism rule by other means, which is why
that number is 5 and not 7.) Either layer regressing alone is now caught, which
is the regression-guard you were asking for.

37 passed for the level tests in tests/test_trust_root.py.

On the underlying gap you flagged as a fine follow-up — register_supervisor
never validating — I've left that alone deliberately. Making it reject would
change the API from "record then validate" to "reject at registration", which is a
behaviour change beyond this PR's subject and worth its own discussion. The
invariant test above at least ensures the two existing checks can't diverge in the
meantime. Happy to open an issue for it.

Disclosure: I could not run the full CI matrix locally — the
agent_control_specification wheel needs maturin and a Rust toolchain I don't
have here. I ran the trust-root level tests with a minimal local stand-in for the
three ACS symbols the module imports; the 7 tests that exercise the native runtime
are excluded by that limitation, and CI covers them.

[LHMQ878]

MohammadHaroonAbuomar could you please re-review the current head (f928519)? The non-integer hierarchy check and the cross-layer regression test now cover the case from your changes-requested review; all current checks are green. GitHub does not allow me, as a fork contributor, to submit a formal review request.

[LHMQ878]

Addressed this in the current branch: the regression now includes floats below a higher integer level, which is the case that previously returned a valid hierarchy without raising, and the TrustRoot tests explicitly assert that non-integer levels are rejected. I also kept the maximum-float case to cover the old TypeError path.

[MohammadHaroonAbuomar]

Closing per maintainer decision; this repository is not accepting submissions from this account.