feat(core): add governed Organization hierarchy-change review - #96
feat(core): add governed Organization hierarchy-change review#96seonghobae wants to merge 22 commits into
Conversation
📝 WalkthroughWalkthrough조직 계층 부모 변경을 위한 Changes조직 계층 변경 검토
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The Organization hierarchy review boundary is covered by passing checks, but its process-local evidence binding can make conflicting-reference decisions depend on object lifetime, and one test reuses a shared default reference. The PR is mergeable with explicit owner awareness and follow-up to make this behavior deterministic and tests isolated. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if self.decision_authority != _DECISION_AUTHORITY: | ||
| raise ValueError("decision_authority must remain human_review_only") | ||
| if type(self.next_action) is not str or self.next_action != _NEXT_ACTION: | ||
| raise ValueError("next_action must remain the governed hierarchy-change instruction") | ||
|
|
||
| payload_json = _canonical_payload_json(_payload(self)) | ||
| creation_digest = sha256(payload_json.encode("utf-8")).hexdigest() | ||
| live_key = (self.tenant_record_id, self.organization_hierarchy_change_reference) | ||
| with _REGISTRY_LOCK: | ||
| binding = _LIVE_REFERENCE_BINDINGS.get(live_key) | ||
| if binding is None: | ||
| binding = _LiveReferenceBinding(creation_digest) |
There was a problem hiding this comment.
📝 Info: Live-reference uniqueness is process-local and GC-dependent
The conflicting-reissuance guard in __post_init__ (review.py:274-285) holds only while some packet sharing the reference stays alive, since _LIVE_REFERENCE_BINDINGS is weak-valued and kept alive only by _PACKET_BINDINGS. Once all such packets are collected, a conflicting reference is accepted again. Documented as intentional defense-in-depth, not durable uniqueness.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| def _validate_issuance_timestamp(value: object) -> None: | ||
| """Require structurally valid system-recorded evidence that has already occurred.""" | ||
| _canonical_timestamp(value) | ||
| if value > datetime.now(timezone.utc): | ||
| raise ValueError("recorded_at must not be in the future") |
There was a problem hiding this comment.
📝 Info: Freshness enforced at construction only
_validate_issuance_timestamp rejects a future recorded_at only at construction; canonical_json revalidates structure and the creation digest but never re-checks the wall clock. This matches the documented design so backward clock movement cannot make issued evidence unreadable.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@opencode-agent Please review the current unchanged head against protected |
1 participant
Scope
Adds an Orgmetra-only, value-minimized pre-mutation review boundary for an Organization Unit parent change. It binds tenant/Organization Unit/current-parent/proposed-parent scope, business
effective_on, systemrecorded_at, reviewed snapshot digests, controlled reason, distinct requester/reviewer evidence and explicit evidence versioning. It carries no Person PII, worker values, compensation, ratings, free-form personal text or employment-decision authority, performs no cross-service application-table SQL and writes no foreign CWL repository.Repair history
Initial contract head
d455631ff23db843ad9da3c4476a991397b144bedeliberately imported a missing production module; its hosted run was later cancelled after implementation superseded it, so cancelled evidence is not claimed as terminal RED. Root implementation46629a26c9bbdf78b32940355af613dac2a74c18added the boundary.Fresh self-review then found conflicting live reissuance under one tenant-qualified hierarchy-change reference. Regression
9af73947b910a5e6ac43c86e9bae17313f84902fand repair2d16a1b486ae380cd67a63f21d621f8abd3e4958bind all still-live idempotent duplicates to one shared evidence digest while leaving durable uniqueness to authoritative persistence/audit.A later review found future system-recorded issuance was accepted. Regression
73d1bdb42bd1f284fc1fbe5c22f652afe795d487and root repair9ce512bf4db5968780c1435007541b7b6e367889makerecorded_atissuance-only fresh: exact fixed-offset temporal shape is required and a future instant fails closed, while later evidence export does not re-enter wall-clock freshness. CHANGELOG/README/traceability were aligned.CodeRabbit then identified test-state coupling in root attach/detach coverage because the attach case reused a live reference with a different payload. Exact current head
b9f8e3d291c4bdcd2f0aa5f9d0378dea09e5e7cdgives that case a distinct UUIDv4 change reference. The corresponding thread was resolved only after exact-current-head GREEN.Governed contract
Root attach/detach uses
None, not sentinel parents. Self-parenting and no-op reparenting fail closed. HRIS-owned operational UUID evolution is preserved while packet-owned change/actor correlations remain UUIDv4. Caller-defined trust primitive subclasses are rejected. Canonical JSON/SHA-256 is deterministic, routine repr is redacted, and post-construction evidence mutation is detected. Governance remains fixed torequires_human_review,requires_authoritative_resolution,not_authorized_to_apply, andhuman_review_only.Before mutation, the authoritative Orgmetra HRIS boundary must re-resolve same-tenant Organization Unit/current-parent/proposed-parent/hierarchy truth at
effective_onand the current system-recorded cutoff, reject stale current-parent evidence, self-parenting, cycles and multiple visible parents, re-establish actor separation, verify reviewed evidence, and write immutable audit/outbox evidence atomically with the mutation.Exact-current-head evidence
Base:
develop@9e3e4847510e1e612b48474ba42b177b8ed824dfHead:
b9f8e3d291c4bdcd2f0aa5f9d0378dea09e5e7cdGitHub reports open, non-draft, mergeable. Every applicable exact-current-head hosted workflow is terminal GREEN:
32627448936— success; focused job97164863086checked out the exact candidate, built/installed the exact package artifact, passed exact statement/branch coverage and clean-checkout steps.32627448972— success, including Foundation validation and PostgreSQL integrity matrix on the exact candidate.32627448969— success.32627448992— success.32627448956— success.Fresh review-thread state has the future-time BUG and root-transition test-isolation finding resolved. The remaining weak live-reference/GC observation is informational and matches the documented process-local defense-in-depth boundary. No qualifying independent non-author
APPROVEorCHANGES_REQUESTEDis present.Merge governance
This exact head is ready for independent review, not merge. Keep unmerged until a qualifying independent non-author approval exists and issue #89's enforceable
developprotection defect is actually repaired. Immediately before any merge, refetch exact head, live base, reviews, threads, rules/protection and checks and use expected-head protection only if every live gate is satisfied. Do not self-approve, weaken gates, simulate protection in workflow code or transfer predecessor evidence.