feat(quality): add behavior ownership catalogue foundation - #297
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a behavior-ownership catalogue foundation. It defines a JSON Schema, partition manifest, deterministic tooling, validation rules, candidate generation, reviewed-test execution, comprehensive tests, and repository documentation. Existing mutation and test-lane behavior remains unchanged except for registering the new test module. ChangesBehavior ownership catalogue
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant behavior_ownership.py
participant partition.v1.json
participant catalogue_records
participant pytest
CLI->>behavior_ownership.py: run inventory, candidate, validation, or partition command
behavior_ownership.py->>partition.v1.json: load and verify assignments
behavior_ownership.py->>catalogue_records: validate record schema and ownership
behavior_ownership.py->>pytest: collect or run reviewed test nodes
pytest-->>behavior_ownership.py: return collection or execution status
behavior_ownership.py-->>CLI: emit JSON result or validation error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
backend/scripts/behavior_ownership.py (2)
397-403: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRe-reading the partition creates a second source of truth.
validate_partitionalready parses and digest-verifies the partition at line 360. Line 397 reads the same file again with no shape or digest check and usesprotected_base_commitfrom that second read. The file can change between the two reads, and aKeyErrorescapes as an untyped error if the second read returns a JSON object without that key.Return the protected base commit from
validate_partition, or extract a singleload_partitionhelper that both call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/behavior_ownership.py` around lines 397 - 403, Update validate_partition to return the validated protected base commit, then have the caller near _validate_remaps reuse that returned value instead of calling _read_json again. Remove the second partition read and preserve the existing validation and digest-verification path.
98-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDomain modules fall into
sharedbecause grouping matches directories and a fixed token list.group_for_targetrecognizes a domain only when the path contains a directory segment such as/authorization/or/artifacts/, or one of four substrings. A module named after its domain but located elsewhere falls through toshared. The partition is generated output, so both sites change together.
backend/scripts/behavior_ownership.py#L98-L111: add a/auth/directory rule and match a module namedartifacts.pyorartifact_*.py, so auth adapters and artifact workers and interfaces reach their own group..ci/behavior-ownership/partition.v1.json#L627-L642: regenerate the file withbehavior_ownership partitionafter the rule change, soauthority_digestand the affected assignments stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/behavior_ownership.py` around lines 98 - 111, The domain grouping logic in group_for_target must recognize additional auth and artifact module paths. In backend/scripts/behavior_ownership.py#L98-L111, add the /auth/ directory rule and match artifacts.py or artifact_* module names; then regenerate .ci/behavior-ownership/partition.v1.json#L627-L642 with behavior_ownership partition so authority_digest and affected assignments remain consistent..ci/behavior-ownership/README.md (1)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
partitionsubcommand.
_mainregisters four subcommands:inventory,generate,validate, andpartition. The command block lists only three.partitionis the command that regeneratespartition.v1.json, so a maintainer needs it after any change togroup_for_targetor to the eligible target set.📝 Proposed documentation addition
.venv/bin/python -m scripts.behavior_ownership validate +.venv/bin/python -m scripts.behavior_ownership partition --base-commit <sha></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In @.ci/behavior-ownership/README.md around lines 30 - 36, Update the command
examples in the behavior-ownership README to include the registered partition
subcommand, showing that it regenerates partition.v1.json for maintainers after
changes to group_for_target or the eligible target set. Keep the existing
inventory, generate, and validate examples unchanged.</details> <!-- cr-comment:v1:37230b389bfdea47b619148c --> </blockquote></details> <details> <summary>backend/tests/test_behavior_ownership.py (1)</summary><blockquote> `449-468`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Both subprocess fakes replace `run` on the shared stdlib module.** `ownership.subprocess` is the `subprocess` module object, so each `monkeypatch.setattr` call swaps `subprocess.run` process-wide for the duration of the test. `monkeypatch` restores the attribute, so the current suite is safe, but the isolation is weaker than it appears. - `backend/tests/test_behavior_ownership.py#L449-L468`: patch a module-local indirection in `backend/scripts/behavior_ownership.py` instead of the stdlib attribute. - `backend/tests/test_behavior_ownership.py#L794-L809`: apply the identical change to `test_collect_only_runner_adds_collection_flag`. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@backend/tests/test_behavior_ownership.pyaround lines 449 - 468, Replace the
process-wide subprocess.run patches in test_owned_tests_runs_only_reviewed_nodes
and test_collect_only_runner_adds_collection_flag
(backend/tests/test_behavior_ownership.py:449-468 and :794-809) with patches to
the module-local subprocess runner indirection used by
backend/scripts/behavior_ownership.py, preserving each test’s existing fake
behavior and assertions.</details> <!-- cr-comment:v1:b1ae68bc8cc638882cf9dfaa --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.ci/behavior-ownership/partition.v1.json:
- Around line 627-642: Update group_for_target to classify artifact-related
module filenames such as artifacts.py and artifact_operations.py, not only
targets within an /artifacts/ directory or the existing fixed token list.
Preserve the intended artifacts grouping for checkers.py and related modules,
then regenerate partition.v1.json.In
@backend/scripts/behavior_ownership.py:
- Around line 241-261: The structural-only validation is missing executable AST
node types. In backend/scripts/behavior_ownership.py lines 241-261, update
_is_strictly_structural’s forbidden tuple to include ast.Lambda, ast.IfExp,
ast.comprehension, and ast.Assert. In backend/tests/test_behavior_ownership.py
lines 522-540, add one parametrized source case for each newly forbidden node
type to ensure structural_only rejects them.- Around line 301-307: Update the record-loading logic around
_catalogue_at_revision so any successfully parsed JSON value that is not a dict
raises BehaviorOwnershipError("invalid_protected_record") instead of being
skipped. Preserve the existing JSONDecodeError handling and append only
validated object records, ensuring malformed protected data fails closed.- Around line 196-209: Update the custody validation around _git_show_optional
so a missing trusted revision is distinguished from a missing PARTITION_PATH at
an existing revision. When trusted_revision is provided but the revision cannot
be resolved, raise BehaviorOwnershipError("invalid_partition_ancestry") instead
of falling back to the merge-base check; retain the existing behavior only when
the trusted revision exists and the file is absent, while trusted_revision=None
continues to opt out explicitly.In
@backend/tests/test_behavior_ownership.py:
- Around line 522-540: Extend _is_strictly_structural’s forbidden node tuple to
reject lambda expressions, conditional expressions, comprehensions, and assert
statements as runtime behavior. Add each corresponding source case to
test_structural_only_rejects_runtime_side_effects, preserving the existing
BehaviorOwnershipError assertion and matching executable_structural_only.- Around line 682-709: Extend test_catalogue_reports_owned_test_failure coverage
in test_behavior_ownership.py by setting up a valid catalogue fixture, patching
_run_test_nodes to return 0 for collect_only=True and a non-zero status for
collect_only=False, and invoking validate_catalogue with run_tests=True. Assert
BehaviorOwnershipError matches "owned_test_failure" and verify calls occurred in
the order [True, False].- Around line 87-107: Extend test_partition_tampering_fails_closed and related
ownership tests to cover validate_partition’s invalid_partition_assignments,
invalid_partition_assignment, missing_partition_base_commit,
invalid_partition_ancestry, and invalid_trusted_partition branches. Recompute
the partition digest after each payload mutation so validation reaches the
intended branch, and adjust the _mock_partition_git behavior or targeted mocks
to exercise missing base commits and merge-base failures while preserving the
expected BehaviorOwnershipError messages.- Around line 41-48: Update
test_repository_partition_is_exact_deterministic_and_digest_bound to avoid
requiring unavailable local Git refs: either skip when protected_base_commit or
origin/main cannot be resolved, or call
validate_partition(trusted_revision=None). Preserve assertions for
authority_digest, eligible-target ordering, and deterministic assignment content
without depending on trusted-revision validation.
Nitpick comments:
In @.ci/behavior-ownership/README.md:
- Around line 30-36: Update the command examples in the behavior-ownership
README to include the registered partition subcommand, showing that it
regenerates partition.v1.json for maintainers after changes to group_for_target
or the eligible target set. Keep the existing inventory, generate, and validate
examples unchanged.In
@backend/scripts/behavior_ownership.py:
- Around line 397-403: Update validate_partition to return the validated
protected base commit, then have the caller near _validate_remaps reuse that
returned value instead of calling _read_json again. Remove the second partition
read and preserve the existing validation and digest-verification path.- Around line 98-111: The domain grouping logic in group_for_target must
recognize additional auth and artifact module paths. In
backend/scripts/behavior_ownership.py#L98-L111, add the /auth/ directory rule
and match artifacts.py or artifact_* module names; then regenerate
.ci/behavior-ownership/partition.v1.json#L627-L642 with behavior_ownership
partition so authority_digest and affected assignments remain consistent.In
@backend/tests/test_behavior_ownership.py:
- Around line 449-468: Replace the process-wide subprocess.run patches in
test_owned_tests_runs_only_reviewed_nodes and
test_collect_only_runner_adds_collection_flag
(backend/tests/test_behavior_ownership.py:449-468 and :794-809) with patches to
the module-local subprocess runner indirection used by
backend/scripts/behavior_ownership.py, preserving each test’s existing fake
behavior and assertions.</details> <details> <summary>🪄 Autofix</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `010a9a5d-2609-40b9-afbb-8d499d3f7ce2` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7676ce4347db0c9694962a9b587a20765e16eac6 and e9a2c7d9c5730bf8d8301bee67aba12adfeffed0. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `.agent-loop/initiatives/WS-QUAL-002-behavior-ownership-catalogue/STATUS.md` * `.agent-loop/initiatives/WS-QUAL-002-behavior-ownership-catalogue/chunks/WS-QUAL-002-01-catalogue-foundation.md` * `.agent-loop/initiatives/WS-QUAL-002-behavior-ownership-catalogue/reviews/WS-QUAL-002-01-pr-trust-bundle.md` * `.ci/behavior-ownership/README.md` * `.ci/behavior-ownership/examples/reviewed.example.json` * `.ci/behavior-ownership/partition.v1.json` * `CONTRIBUTING.md` * `backend/scripts/behavior_ownership.py` * `backend/scripts/run_test_lanes.py` * `backend/tests/test_behavior_ownership.py` * `docs/operations_backend_testing.md` * `scripts/behavior-ownership.schema.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| { | ||
| "group": "shared", | ||
| "target": "backend/app/workers/artifacts.py" | ||
| }, | ||
| { | ||
| "group": "shared", | ||
| "target": "backend/app/workers/async_runner.py" | ||
| }, | ||
| { | ||
| "group": "shared", | ||
| "target": "backend/app/workers/celery_app.py" | ||
| }, | ||
| { | ||
| "group": "artifacts", | ||
| "target": "backend/app/workers/checkers.py" | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Artifact modules assigned to shared.
backend/app/workers/artifacts.py is assigned to shared, while backend/app/workers/checkers.py on the adjacent line is assigned to artifacts. The cause is in group_for_target: the rule tests for the directory segment /artifacts/ and for a fixed token list, so a module named artifacts.py outside an artifacts/ directory falls through. backend/app/interfaces/artifacts.py at line 116 and backend/app/interfaces/artifact_operations.py at line 112 are affected in the same way.
Fix the rule in group_for_target, then regenerate this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ci/behavior-ownership/partition.v1.json around lines 627 - 642, Update
group_for_target to classify artifact-related module filenames such as
artifacts.py and artifact_operations.py, not only targets within an /artifacts/
directory or the existing fixed token list. Preserve the intended artifacts
grouping for checkers.py and related modules, then regenerate partition.v1.json.
| if trusted_revision is not None: | ||
| trusted = _git_show_optional(root, trusted_revision, PARTITION_PATH) | ||
| if trusted is not None: | ||
| try: | ||
| trusted_value = json.loads(trusted) | ||
| except json.JSONDecodeError as exc: | ||
| raise BehaviorOwnershipError("invalid_trusted_partition") from exc | ||
| if trusted_value != value: | ||
| raise BehaviorOwnershipError("untrusted_partition_change") | ||
| else: | ||
| try: | ||
| _git(root, "merge-base", "--is-ancestor", protected_base, "HEAD") | ||
| except BehaviorOwnershipError as exc: | ||
| raise BehaviorOwnershipError("invalid_partition_ancestry") from exc |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Custody check degrades silently when the trusted revision is unavailable.
_git_show_optional returns None for any git show failure. This includes the case where origin/main does not exist in the clone, for example after a shallow fetch or in a fork without the remote ref. Validation then skips the comparison against the protected copy and accepts a branch-local partition that only needs the protected base commit to be an ancestor of HEAD. This is the exact substitution that the README says must fail.
Distinguish "trusted revision does not exist" from "file absent at that revision". Fail closed when the revision itself is missing, unless the caller passes trusted_revision=None explicitly.
🔒 Proposed fail-closed handling
if trusted_revision is not None:
+ _git(root, "rev-parse", "--verify", f"{trusted_revision}^{{commit}}")
trusted = _git_show_optional(root, trusted_revision, PARTITION_PATH)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if trusted_revision is not None: | |
| trusted = _git_show_optional(root, trusted_revision, PARTITION_PATH) | |
| if trusted is not None: | |
| try: | |
| trusted_value = json.loads(trusted) | |
| except json.JSONDecodeError as exc: | |
| raise BehaviorOwnershipError("invalid_trusted_partition") from exc | |
| if trusted_value != value: | |
| raise BehaviorOwnershipError("untrusted_partition_change") | |
| else: | |
| try: | |
| _git(root, "merge-base", "--is-ancestor", protected_base, "HEAD") | |
| except BehaviorOwnershipError as exc: | |
| raise BehaviorOwnershipError("invalid_partition_ancestry") from exc | |
| if trusted_revision is not None: | |
| _git(root, "rev-parse", "--verify", f"{trusted_revision}^{{commit}}") | |
| trusted = _git_show_optional(root, trusted_revision, PARTITION_PATH) | |
| if trusted is not None: | |
| try: | |
| trusted_value = json.loads(trusted) | |
| except json.JSONDecodeError as exc: | |
| raise BehaviorOwnershipError("invalid_trusted_partition") from exc | |
| if trusted_value != value: | |
| raise BehaviorOwnershipError("untrusted_partition_change") | |
| else: | |
| try: | |
| _git(root, "merge-base", "--is-ancestor", protected_base, "HEAD") | |
| except BehaviorOwnershipError as exc: | |
| raise BehaviorOwnershipError("invalid_partition_ancestry") from exc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/behavior_ownership.py` around lines 196 - 209, Update the
custody validation around _git_show_optional so a missing trusted revision is
distinguished from a missing PARTITION_PATH at an existing revision. When
trusted_revision is provided but the revision cannot be resolved, raise
BehaviorOwnershipError("invalid_partition_ancestry") instead of falling back to
the merge-base check; retain the existing behavior only when the trusted
revision exists and the file is absent, while trusted_revision=None continues to
opt out explicitly.
| forbidden = ( | ||
| ast.FunctionDef, | ||
| ast.AsyncFunctionDef, | ||
| ast.Call, | ||
| ast.Await, | ||
| ast.Yield, | ||
| ast.YieldFrom, | ||
| ast.If, | ||
| ast.For, | ||
| ast.AsyncFor, | ||
| ast.While, | ||
| ast.With, | ||
| ast.AsyncWith, | ||
| ast.Try, | ||
| ast.Raise, | ||
| ast.Match, | ||
| ast.NamedExpr, | ||
| ast.AugAssign, | ||
| ast.Delete, | ||
| ) | ||
| return not any(isinstance(node, forbidden) for node in ast.walk(tree)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The forbidden node tuple is incomplete, so structural_only accepts executable modules. _is_strictly_structural omits ast.Lambda, ast.IfExp, ast.comprehension, and ast.Assert. A module-level lambda, conditional expression, comprehension, or assert therefore passes as structural, although the README and the chunk contract state that branches, loops, and raises fail validation. The test parametrization mirrors the same gap, so no test detects it.
backend/scripts/behavior_ownership.py#L241-L261: addast.Lambda,ast.IfExp,ast.comprehension, andast.Assertto theforbiddentuple.backend/tests/test_behavior_ownership.py#L522-L540: add one parametrized source per added node type, so the gap cannot reopen.
📍 Affects 2 files
backend/scripts/behavior_ownership.py#L241-L261(this comment)backend/tests/test_behavior_ownership.py#L522-L540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/behavior_ownership.py` around lines 241 - 261, The
structural-only validation is missing executable AST node types. In
backend/scripts/behavior_ownership.py lines 241-261, update
_is_strictly_structural’s forbidden tuple to include ast.Lambda, ast.IfExp,
ast.comprehension, and ast.Assert. In backend/tests/test_behavior_ownership.py
lines 522-540, add one parametrized source case for each newly forbidden node
type to ensure structural_only rejects them.
| try: | ||
| value = json.loads(source) | ||
| except json.JSONDecodeError as exc: | ||
| raise BehaviorOwnershipError("invalid_protected_record") from exc | ||
| if isinstance(value, dict): | ||
| records.append(value) | ||
| return records |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Non-object protected records are skipped instead of rejected.
Line 305 appends only dict values. If a protected record at the base revision is replaced by a JSON list or scalar, _catalogue_at_revision drops it. The protected owner then disappears from protected_by_id, and the ownership, replacement, and effective-owner checks never run for it. Every other malformed-input path in this module fails closed. Make this one consistent.
🐛 Proposed fail-closed handling
- if isinstance(value, dict):
- records.append(value)
+ if not isinstance(value, dict):
+ raise BehaviorOwnershipError("invalid_protected_record")
+ records.append(value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| value = json.loads(source) | |
| except json.JSONDecodeError as exc: | |
| raise BehaviorOwnershipError("invalid_protected_record") from exc | |
| if isinstance(value, dict): | |
| records.append(value) | |
| return records | |
| try: | |
| value = json.loads(source) | |
| except json.JSONDecodeError as exc: | |
| raise BehaviorOwnershipError("invalid_protected_record") from exc | |
| if not isinstance(value, dict): | |
| raise BehaviorOwnershipError("invalid_protected_record") | |
| records.append(value) | |
| return records |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/behavior_ownership.py` around lines 301 - 307, Update the
record-loading logic around _catalogue_at_revision so any successfully parsed
JSON value that is not a dict raises
BehaviorOwnershipError("invalid_protected_record") instead of being skipped.
Preserve the existing JSONDecodeError handling and append only validated object
records, ensuring malformed protected data fails closed.
| def test_repository_partition_is_exact_deterministic_and_digest_bound() -> None: | ||
| value = json.loads((ownership.ROOT / ownership.PARTITION_PATH).read_text()) | ||
| mapping = ownership.validate_partition() | ||
| assert len(mapping) == len(ownership.eligible_targets()) | ||
| assert list(mapping) == ownership.eligible_targets() | ||
| authority = {key: value[key] for key in value if key != "authority_digest"} | ||
| assert value["authority_digest"] == hashlib.sha256(ownership._json_bytes(authority)).hexdigest() | ||
| assert ownership.build_partition(base_commit=value["protected_base_commit"]) == value |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
This test depends on local git state.
validate_partition() runs with the default trusted_revision="origin/main". It also runs git rev-parse --verify on protected_base_commit at line 170 of backend/scripts/behavior_ownership.py. Both steps fail in a shallow clone and in a clone without the origin/main ref. The test then fails for an environment reason, not a code reason.
Skip the test when the base commit or the trusted ref is unavailable, or pass trusted_revision=None and assert the digest and assignment content only.
💚 Proposed guard
def test_repository_partition_is_exact_deterministic_and_digest_bound() -> None:
value = json.loads((ownership.ROOT / ownership.PARTITION_PATH).read_text())
+ if subprocess.run(
+ ["git", "cat-file", "-e", f"{value['protected_base_commit']}^{{commit}}"],
+ cwd=ownership.ROOT,
+ capture_output=True,
+ ).returncode:
+ pytest.skip("protected base commit is not present in this clone")
mapping = ownership.validate_partition()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_behavior_ownership.py` around lines 41 - 48, Update
test_repository_partition_is_exact_deterministic_and_digest_bound to avoid
requiring unavailable local Git refs: either skip when protected_base_commit or
origin/main cannot be resolved, or call
validate_partition(trusted_revision=None). Preserve assertions for
authority_digest, eligible-target ordering, and deterministic assignment content
without depending on trusted-revision validation.
| @pytest.mark.parametrize( | ||
| ("mutator", "error"), | ||
| [ | ||
| (lambda value: value.update(schema="wrong"), "unsupported_partition_schema"), | ||
| (lambda value: value.update(authority_digest="0" * 64), "partition_digest_mismatch"), | ||
| (lambda value: value.update(assignments="wrong"), "partition_digest_mismatch"), | ||
| (lambda value: value["assignments"].append(value["assignments"][0]), "partition_digest_mismatch"), | ||
| ], | ||
| ) | ||
| def test_partition_tampering_fails_closed( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch, mutator, error: str | ||
| ) -> None: | ||
| target = "backend/scripts/example.py" | ||
| value = _partition([target]) | ||
| mutator(value) | ||
| _write_json(tmp_path / ownership.PARTITION_PATH, value) | ||
| monkeypatch.setattr(ownership, "eligible_targets", lambda root=ownership.ROOT: [target]) | ||
| _mock_partition_git(monkeypatch) | ||
| with pytest.raises(ownership.BehaviorOwnershipError, match=error): | ||
| ownership.validate_partition(tmp_path, trusted_revision=None) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Several partition failure branches have no test.
The parametrized cases all fail at the digest check or earlier. The following error paths in validate_partition are never reached by this module:
invalid_partition_assignments(line 174) —assignmentsis not a list, with a matching digest.invalid_partition_assignment(line 184) — an assignment with an unknown group, an extra key, or an ineligible target, with a matching digest.missing_partition_base_commit(line 171) —_mock_partition_gitalways returns the same value thatrev-parseis compared against.invalid_partition_ancestry(line 209) — the_gitmock never raises formerge-base.invalid_trusted_partition(line 202) — the trusted copy is never malformed JSON.
The chunk contract requires negative tests for malformed records and invalid ancestry. Add cases that recompute the digest after each mutation, so the check under test is the one that fires.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_behavior_ownership.py` around lines 87 - 107, Extend
test_partition_tampering_fails_closed and related ownership tests to cover
validate_partition’s invalid_partition_assignments,
invalid_partition_assignment, missing_partition_base_commit,
invalid_partition_ancestry, and invalid_trusted_partition branches. Recompute
the partition digest after each payload mutation so validation reaches the
intended branch, and adjust the _mock_partition_git behavior or targeted mocks
to exercise missing base commits and merge-base failures while preserving the
expected BehaviorOwnershipError messages.
| @pytest.mark.parametrize( | ||
| "source", | ||
| [ | ||
| "open('value.txt')\n", | ||
| "if FLAG:\n VALUE = 1\n", | ||
| "VALUES = []\nVALUES.append(1)\n", | ||
| "for item in VALUES:\n VALUE = item\n", | ||
| "raise RuntimeError('side effect')\n", | ||
| ], | ||
| ) | ||
| def test_structural_only_rejects_runtime_side_effects(tmp_path: Path, source: str) -> None: | ||
| target = "backend/scripts/structural.py" | ||
| path = tmp_path / target | ||
| path.parent.mkdir(parents=True) | ||
| path.write_text(source, encoding="utf-8") | ||
| with pytest.raises(ownership.BehaviorOwnershipError, match="executable_structural_only"): | ||
| ownership._validate_record_semantics( | ||
| tmp_path, {"status": "structural_only", "target": target} | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add structural cases for lambdas, conditional expressions, comprehensions, and asserts.
The parametrized sources cover a call, an if, a method call, a for, and a raise. They do not cover the node types that _is_strictly_structural omits from its forbidden tuple. Add these sources after you extend that tuple, so the gap cannot reopen:
SCALE = lambda value: value * 2LIMIT = 10 if FLAG else 100NAMES = [item for item in RAW]assert LIMIT > 0
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_behavior_ownership.py` around lines 522 - 540, Extend
_is_strictly_structural’s forbidden node tuple to reject lambda expressions,
conditional expressions, comprehensions, and assert statements as runtime
behavior. Add each corresponding source case to
test_structural_only_rejects_runtime_side_effects, preserving the existing
BehaviorOwnershipError assertion and matching executable_structural_only.
| def test_catalogue_fails_when_exact_test_collection_fails( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| target = "backend/scripts/example.py" | ||
| path = tmp_path / target | ||
| path.parent.mkdir(parents=True) | ||
| path.write_text("def run():\n return 1\n", encoding="utf-8") | ||
| test_file = tmp_path / "backend/tests/test_example.py" | ||
| test_file.parent.mkdir(parents=True) | ||
| test_file.write_text("def test_run():\n pass\n", encoding="utf-8") | ||
| (tmp_path / "scripts").mkdir() | ||
| (tmp_path / ownership.SCHEMA_PATH).write_text( | ||
| (ownership.ROOT / ownership.SCHEMA_PATH).read_text(), encoding="utf-8" | ||
| ) | ||
| _write_json( | ||
| tmp_path / ".ci/behavior-ownership/shared/example.json", | ||
| _catalogue_record("reviewed", target, "behavior:example"), | ||
| ) | ||
| _write_json(tmp_path / ownership.PARTITION_PATH, _partition([target])) | ||
| monkeypatch.setattr( | ||
| ownership, | ||
| "validate_partition", | ||
| lambda root=ownership.ROOT, **kwargs: {target: "shared"}, | ||
| ) | ||
| monkeypatch.setattr(ownership, "_validate_remaps", lambda *args, **kwargs: None) | ||
| monkeypatch.setattr(ownership, "_run_test_nodes", lambda *args, **kwargs: 1) | ||
| with pytest.raises(ownership.BehaviorOwnershipError, match="stale_catalogue_test"): | ||
| ownership.validate_catalogue(tmp_path) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
No test covers owned_test_failure.
test_catalogue_fails_when_exact_test_collection_fails patches _run_test_nodes to return 1, so the collection branch at line 408 of backend/scripts/behavior_ownership.py raises first. The run_tests=True branch at lines 410-411 is never executed. That branch is the --run-owned-tests behavior named in the chunk contract acceptance criteria.
Add a case where the collect-only call returns 0 and the execution call returns non-zero, then assert owned_test_failure.
💚 Proposed test
def test_catalogue_reports_owned_test_failure(tmp_path, monkeypatch):
calls = []
def fake_nodes(root, records, *, collect_only):
calls.append(collect_only)
return 0 if collect_only else 3
monkeypatch.setattr(ownership, "_run_test_nodes", fake_nodes)
# reuse the fixture setup from test_catalogue_fails_when_exact_test_collection_fails
with pytest.raises(ownership.BehaviorOwnershipError, match="owned_test_failure"):
ownership.validate_catalogue(tmp_path, run_tests=True)
assert calls == [True, False]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_behavior_ownership.py` around lines 682 - 709, Extend
test_catalogue_reports_owned_test_failure coverage in test_behavior_ownership.py
by setting up a valid catalogue fixture, patching _run_test_nodes to return 0
for collect_only=True and a non-zero status for collect_only=False, and invoking
validate_catalogue with run_tests=True. Assert BehaviorOwnershipError matches
"owned_test_failure" and verify calls occurred in the order [True, False].
2 participants
PR Trust Bundle: WS-QUAL-002-01
Chunk
WS-QUAL-002-01— Behavior Ownership Catalogue Foundation.Goal And Human-Approved Intent
Add one versioned catalogue contract, exact eligible-target partition, and
deterministic read-only generator/validator without activating mutation CI or
changing Workstream product behavior. The human separately approved the narrow
contract correction that assigns the new focused test module to the existing
shared_foundationslane.What Changed And Why
candidate,reviewed, and strictstructural_onlyrecords.exactly one population group.
validation, exact pytest collection, and optional exact owned-test execution.
carry-forward, structural-side-effect, identity, and effective-owner checks.
example.
Design Chosen
The tooling delegates eligibility, safe paths, callable spans, changed-callable
derivation, outcomes, boundaries, and test-node syntax to
backend/scripts/mutation_policy.py. Candidate inference is structurallynon-authoritative. Protected records remain byte-identical or resolve through
exactly one reviewed, evidence-preserving remap. The initial empty catalogue is
reported as incomplete rather than promoted or blocked.
Alternatives Rejected
No wildcard/group inference authority, branch-local partition replacement,
callable-wide mutation activation, inferred reviewed ownership, free-form
structural exemption, parallel AST implementation, or workflow change.
Scope Control And Product Behavior
All files are within the approved contract plus the human-approved single lane
assignment. No
.github/workflows/**, backend application module, migration,coverage floor, timeout, skip, deselection, product review decision,
authorization rule, payment, reputation, or
ContributionRecordbehaviorchanged.
Acceptance Proof And Tests
scripts.behavior_ownershipfocused coverage is 91.30 percent, above 90.complete: false.authoritative: false, emits no empty-callablecandidate, and separates structural-review targets.
Test Delta And CI Integrity
One focused test module was added and assigned to
shared_foundations. No testwas removed, skipped, deselected, weakened, or moved between existing lanes. No
workflow, coverage threshold, package configuration, or required-check behavior
changed.
Reviewer Results
group-directory enforcement.
structural exclusions.
External Review
CodeRabbit and exact-head GitHub checks are pending after PR creation. They
supplement, but do not replace, the internal reviews above.
Remaining Risks And Follow-Up
The catalogue is intentionally incomplete until population chunks
03Athrough03Dmerge. Context evidence (02), completeness integration (04), and anyfuture changed-line mutation reactivation (
05) remain separate approvedchunks. Mutation enforcement remains retired.
Human Review Focus
Review candidate-versus-reviewed authority, protected partition bootstrap and
future trusted-base custody, remap carry-forward/effective-owner rules, strict
structural-only behavior, and the single lane assignment.
Human Merge Ownership
Summary by CodeRabbit
New Features
Documentation
Tests