Skip to content

feat(#333): role sources + pluggable role→holder resolution (phases 2 and 3) - #726

Merged
Weegy merged 2 commits into
mainfrom
feat/333-role-attribute-sources
Aug 18, 2026
Merged

feat(#333): role sources + pluggable role→holder resolution (phases 2 and 3)#726
Weegy merged 2 commits into
mainfrom
feat/333-role-attribute-sources

Conversation

@Weegy

@Weegy Weegy commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Phase 1 (#724) answered who a turn's caller is. This answers the other half of the question the Phase-0 spec (§6) assigns to #333"and what are they entitled to?" — as the pluggable registry over role/attribute sources (Entra groups, Odoo HR) that the spec's artifact table calls for, generalizing auth/providerRegistry.ts.

Nothing here evaluates a permission. These are facts; #575 turns them into decisions. The boundary stays exactly where the spec drew it:

#333 produces Principals. #575 consumes Principals and produces decisions.

Conductor's RoleResolver becoming external is phase 3 and is deliberately not in this cut — 4 files, reviewable.

The property everything else hangs on: absence is a type

rolesFor never returns a bare array:

Outcome Means
resolved, roles: [] the source knows this principal and confers nothing — a real, trustworthy answer
unavailable we could not find out

Collapsing the two is an authorization bug in whichever direction the caller guesses:

  • read as "this user has no roles", an unreachable Entra tenant quietly strips every entitlement — a self-inflicted outage that looks like policy;
  • read as "unknown, so allow", it is a silent full grant.

AggregateRoleLookup.partial carries that distinction through the merge, so a half-answered lookup cannot pass for a complete one. The dangerous case is not the empty result — it is a non-empty result that is silently missing a source, which a caller glancing at roles alone would accept. Same reasoning that made ScopeId's unscoped and Principal's undefined types rather than values.

Three further deliberate behaviours

The catalog/registry split is mirrored from ProviderRegistry — and matters more here. That file's rationale is that "a compromised admin can never enable a provider the operator didn't intend". An auth provider decides whether you get in; a role source decides what you are once inside. Registering one silently is privilege escalation with no login event to notice, so the two-tier gate is mandatory rather than decorative.

A throwing source degrades to unavailable instead of rejecting. One misbehaving directory must not fail a turn — but it must not vanish either, so it still lands in bySource and still sets partial. Sources are queried concurrently; they are independent network reads on a turn's hot path, and serialising them would add every directory's latency together.

A role: principal short-circuits with no source consulted. Asking what roles a role has is a category error — a role is an indirection over holders, not a subject with entitlements — and answering it would invite a source to invent role nesting that #575 has not specified. It resolves partial: false: a complete answer, not a failed one.

Role keys are canonicalized with phase 1's role rule (trim only, case preserved). Lowercasing would stop matching the mixed-case keys createRole writes verbatim.

Blast radius

Surface Effect
Published plugin contract none — additive exports only
Runtime behaviour none — nothing constructs a registry yet; this is the seam, its consumers come with #575
Database none
Existing call sites none touched

Verification

Mutation checks

Mutation Result
an unavailable source no longer sets partial killed — 3 tests
remove the role: short-circuit killed — 1 test
activate admits an uncatalogued id killed — 1 test

Refs #333


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Weegy added 2 commits August 17, 2026 15:14
… to (phase 2)

Phase 1 answered WHO a turn's caller is. This answers the other half of the
question the Phase-0 spec (§6) assigns to #333 — "and what are they entitled
to?" — as a pluggable registry over role/attribute sources (Entra groups, Odoo
HR), generalizing `auth/providerRegistry.ts` exactly as the spec's artifact
table calls for.

Nothing here evaluates a permission. These are FACTS; #575 turns them into
decisions. Conductor's RoleResolver going external is phase 3 and is not in this
cut.

The property everything else hangs on: ABSENCE IS A TYPE.

  `rolesFor` never returns a bare array. `resolved` with an empty set means "the
  source knows this principal and confers nothing"; `unavailable` means "we
  could not find out". Collapsing them is an authorization bug in whichever
  direction the caller guesses — read as "no roles", an unreachable Entra tenant
  quietly strips every entitlement and looks like policy; read as "unknown, so
  allow", it is a silent full grant. `AggregateRoleLookup.partial` carries that
  distinction through the merge, so a half-answered lookup cannot pass for a
  complete one. Same reasoning as `ScopeId`'s `unscoped` and `Principal`'s
  `undefined`.

Three further deliberate behaviours:

  - The catalog/registry split from `ProviderRegistry` is mirrored, and matters
    MORE here. An auth provider decides whether you get in; a role source
    decides what you ARE once inside, so registering one silently is privilege
    escalation with no login event to notice.
  - A throwing source becomes `unavailable` rather than rejecting — one
    misbehaving directory must not fail a turn — but it still lands in
    `bySource` and still sets `partial`, so it cannot vanish.
  - A `role:` principal short-circuits with no source consulted. Asking what
    roles a role has is a category error; answering would invite a source to
    invent role nesting #575 has not specified. It is `resolved`, NOT `partial`
    — a complete answer, not a failed one.

Role keys are canonicalized with the ROLE rule (trim only, case preserved),
reusing phase 1's `canonicalizePrincipalRef`. Lowercasing would stop matching
the mixed-case keys `createRole` writes verbatim.

Mutation-checked: dropping `partial = true` on an unavailable source kills 3
tests; removing the `role:` short-circuit kills 1; letting `activate` admit an
uncatalogued id kills 1.

Full suite 6590 tests / 0 fail / 0 cancelled. Typecheck, lint, the #470
decoupling ratchet (3294) and the #573 test-typecheck ratchet (406/406) green.

Refs #333
…ths it exposed (phase 3)

Completes the artifact the Phase-0 spec (§6) assigns to #333: Conductor's
RoleResolver becoming external. `roleStore.ts:22` has called it "a follow-up"
since US5.

Holders used to come only from `conductor_role_assignments`, so a PARTIALLY
KNOWN holder list could not exist. Sourcing them from an Entra group or an Odoo
HR reporting line makes it possible — and two decisions built on that list turn
out to fail OPEN once it can be incomplete:

1. quorum='all' completed with too few approvals.

   Every holder still visible may have answered while the people the
   unreachable source knows about were never asked, so the quorum looks
   satisfied. A four-eyes approval silently becomes two-eyes. The pre-existing
   `required.length > 0` guard covers only the EMPTY case; the partial case is
   the dangerous one precisely because it looks legitimate. It now refuses to
   complete, and the run stalls to its deadline fallback — the same well-trodden
   path an unanswered await already takes.

2. openHumanAwait could skip the human step entirely.

   "Role has no holder → take the fallback" is correct (FR-024) when we KNOW
   there are none. An empty list from a failed lookup means "we could not ask",
   and taking the fallback there bypasses an approval because a directory was
   down. It now parks the await instead.

The authorization gate is deliberately NOT changed: a shrunken list there
rejects a genuine holder, which fails closed.

Shape mirrors phase 2 — `HolderLookup` is `resolved | unavailable`, the
aggregate carries `partial`, sources run concurrently, a throwing source becomes
`unavailable` but stays visible in `bySource`. Holder ids canonicalize with the
USER rule (trim + lowercase, matching the case-sensitive binding match); role
KEYS keep their case. Mixing those two rules up is a silent routing miss.

The local assignment table is registered as an ordinary source rather than
special-cased: one merge path, and the local store gets the same failure
handling as any remote directory. An external source may not claim the reserved
`conductor-local` id — that would substitute its own approver list, so it is a
registration collision at boot.

With no external source configured, behaviour is byte-for-byte unchanged: one
source, never partial. Existing executor test fixtures were updated to the
richer resolver shape; they had been injecting bare arrays.

Mutation-checked: removing the partial guard from the quorum check kills exactly
the new "REFUSES to close when the holder list is partial" test, and nothing
else — its `partial: false` control twin still closes.

Full suite 6606 tests / 0 fail / 0 cancelled. Typecheck, lint, the #470
decoupling ratchet (3294) and the #573 test-typecheck ratchet (406/406) green.

Refs #333
@Weegy Weegy changed the title feat(#333): role and attribute sources — what a Principal is entitled to (phase 2) feat(#333): role sources + pluggable role→holder resolution (phases 2 and 3) Aug 17, 2026
@Weegy

Weegy commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Phase 3 pushed onto this branch

The PR now carries both remaining #333 phases. Phase 2 (role sources) is described above; phase 3 completes the artifact the spec assigns to #333 — Conductor's RoleResolver becoming external, which roleStore.ts:22 has called "a follow-up" since US5.

Phase 3 is a security fix, not just a seam

Holders used to come only from conductor_role_assignments, so a partially known holder list could not exist. Making holders pluggable creates that state — and two decisions built on the list turn out to fail open once it can be incomplete:

Decision With a shrunken list
authorization gate (is this responder a holder?) rejects a genuine holder fail-closed, left unchanged
quorum='all' completeness completes with too few approvals fail-open → fixed
"no holder → take the fallback" skips the human step entirely fail-open → fixed

The quorum case is the sharp one: every holder still visible may have answered while the people an unreachable directory knows about were never asked, so a four-eyes approval silently becomes two-eyes. The pre-existing required.length > 0 guard covers only the empty list — the partial one is dangerous precisely because it looks legitimate.

Both now fail closed: the quorum refuses to complete and the run stalls to its deadline fallback, and openHumanAwait parks the await rather than bypassing the approval.

No behaviour change until someone opts in

With no external source configured there is exactly one source and partial is never true, so today's deployments are byte-for-byte unchanged. The local assignment table is registered as an ordinary RoleHolderSource rather than special-cased — one merge path, and it gets the same throw-becomes-unavailable handling as any remote directory. An external source may not claim the reserved conductor-local id; that would substitute its own approver list, so it is a boot-time collision.

Verification

Existing executor fixtures were updated to the richer resolver shape; they had been injecting bare arrays.

@Weegy
Weegy merged commit 72207d4 into main Aug 18, 2026
9 checks passed
Weegy added a commit that referenced this pull request Aug 20, 2026
* feat(#577): skill ownership + lifecycle model (P1)

Adds the model layer for scope-owned, tamper-evident skills:

- migrations/0040_skill_ownership_lifecycle.sql: owner_scope,
  lifecycle_status (draft/reviewed/published/archived), manifest_signature,
  manifest_signed_at columns on the existing skills table (0003).
- src/services/skillLifecycle.ts: pure model — SkillOwnerScope (ScopeId
  restricted to personal/group/org), the lifecycle transition matrix,
  requiredCapabilities parsing from frontmatter (throws SkillManifestError
  with a field-naming message on malformed input, never silently drops —
  #690 guard), canonical manifest serialization + HMAC-SHA256 sign/verify.
- src/services/skillLifecycleStore.ts: thin Postgres store over the new
  columns (assignPersonalOwner, transition), standalone from
  AgentGraphStore to keep this phase's surface to src/services/skill*.
- test/skillLifecycle.test.ts (30 tests): exhaustive 4x4 transition matrix,
  byte-exact canonical manifest lock, capability-order independence,
  capability case-sensitivity (mirrors #724's role-key precedent), tamper
  detection, malformed-signature handling.
- test/skillOwnershipLifecycleStore.pg.test.ts (6 tests, pg-gated): full
  draft->reviewed->published->archived flow against real Postgres,
  publish-gate rejection, re-signing on every transition.

* feat(#577): scope-ordered skill resolution with shadowing (P2)

Adds resolveSkillByName in src/services/skillResolver.ts: given a skill
name and a requester's scope, pick the single winning skill row across
personal -> shared -> team -> org, in that strict order (#577 Kernkonzept
#4). Pure and synchronous, layered on P1's ScopeId/lifecycle model
(#767) without touching GrantStore directly — membership/sharing are
passed in pre-resolved (SkillResolutionContext), same seam P1 used for
capability grants.

Resolves by `name`, not `slug`: skills.slug is globally UNIQUE (0003), so
no schema change is needed for two scopes to own same-named skills.

Two invariants get explicit, separately-mutation-tested coverage:
- precedence order (personal beats org even at equal names -- the
  'non-empty result from the wrong level' danger named in the phase
  spec, parallel to the quorum='all' fail-open lesson from #726)
- the lifecycle gate: only 'published' skills are eligible candidates,
  filtered BEFORE bucketing, so a draft at a higher-precedence level can
  never outrank a published skill lower down.

A third case is guarded structurally: two candidates tying within one
bucket return an explicit 'ambiguous' result (level + full candidate
list) rather than an arbitrary pick -- same 'absence/uncertainty is a
type' posture as resolveCapabilities (#575) and RoleSourceRegistry
(#333).

15 tests in test/skillResolver.test.ts, all pure (no pg gate needed).
Mutation-tested: reversing bucket precedence order fails 3 tests;
dropping the published-status filter fails 3 tests.
Weegy added a commit that referenced this pull request Aug 20, 2026
…te-guard (P3) (#771)

* feat(#577): skill ownership + lifecycle model (P1)

Adds the model layer for scope-owned, tamper-evident skills:

- migrations/0040_skill_ownership_lifecycle.sql: owner_scope,
  lifecycle_status (draft/reviewed/published/archived), manifest_signature,
  manifest_signed_at columns on the existing skills table (0003).
- src/services/skillLifecycle.ts: pure model — SkillOwnerScope (ScopeId
  restricted to personal/group/org), the lifecycle transition matrix,
  requiredCapabilities parsing from frontmatter (throws SkillManifestError
  with a field-naming message on malformed input, never silently drops —
  #690 guard), canonical manifest serialization + HMAC-SHA256 sign/verify.
- src/services/skillLifecycleStore.ts: thin Postgres store over the new
  columns (assignPersonalOwner, transition), standalone from
  AgentGraphStore to keep this phase's surface to src/services/skill*.
- test/skillLifecycle.test.ts (30 tests): exhaustive 4x4 transition matrix,
  byte-exact canonical manifest lock, capability-order independence,
  capability case-sensitivity (mirrors #724's role-key precedent), tamper
  detection, malformed-signature handling.
- test/skillOwnershipLifecycleStore.pg.test.ts (6 tests, pg-gated): full
  draft->reviewed->published->archived flow against real Postgres,
  publish-gate rejection, re-signing on every transition.

* feat(#577): scope-ordered skill resolution with shadowing (P2)

Adds resolveSkillByName in src/services/skillResolver.ts: given a skill
name and a requester's scope, pick the single winning skill row across
personal -> shared -> team -> org, in that strict order (#577 Kernkonzept
#4). Pure and synchronous, layered on P1's ScopeId/lifecycle model
(#767) without touching GrantStore directly — membership/sharing are
passed in pre-resolved (SkillResolutionContext), same seam P1 used for
capability grants.

Resolves by `name`, not `slug`: skills.slug is globally UNIQUE (0003), so
no schema change is needed for two scopes to own same-named skills.

Two invariants get explicit, separately-mutation-tested coverage:
- precedence order (personal beats org even at equal names -- the
  'non-empty result from the wrong level' danger named in the phase
  spec, parallel to the quorum='all' fail-open lesson from #726)
- the lifecycle gate: only 'published' skills are eligible candidates,
  filtered BEFORE bucketing, so a draft at a higher-precedence level can
  never outrank a published skill lower down.

A third case is guarded structurally: two candidates tying within one
bucket return an explicit 'ambiguous' result (level + full candidate
list) rather than an arbitrary pick -- same 'absence/uncertainty is a
type' posture as resolveCapabilities (#575) and RoleSourceRegistry
(#333).

15 tests in test/skillResolver.test.ts, all pure (no pg gate needed).
Mutation-tested: reversing bucket precedence order fails 3 tests;
dropping the published-status filter fails 3 tests.

* feat(#577): sharing via GrantStore + admin-gated promotion + cron write-guard (P3)

Adds the sharing/promotion/write-guard layer on top of P1 (skill
ownership/lifecycle) and P2 (scope-ordered resolution):

- src/services/skillLifecycle.ts: assertHumanActor / SkillAutomationWriteBlocked
  -- the enforced cron write-guard (#577 Kernkonzept #6). Checks
  ScopeId.kind === 'system' (scopeId.ts's own documented boundary: "no
  human is present in any of them" -- routine/schedule/conductor/
  conductor-builder), so it needs no new taxonomy. Threaded as the first
  check in every mutating store method -- a blocked write never reaches
  the database (asserted in the pg tests, not just at the pure-function
  level).

- src/services/skillLifecycleStore.ts:
  - assignPersonalOwner and transition now take an actorScope and call
    assertHumanActor before any query.
  - New promoteSkillOwnerScope(skillId, targetScope, opts): the ONLY way
    a skill reaches team/org ownership (#577 Kernkonzept #5 -- no direct
    creation there). Requires the skill be already 'published', re-signs
    the manifest at the NEW ownerScope + SAME status (promotion is a
    signature-changing event, since ownerScope is a signed field).
    Admin-gating itself (an authenticated-session check) is left to the
    route layer -- this method enforces the cron guard and the
    published-only invariant, nothing about roles.

- src/services/skillSharing.ts: sharing = a grant over GrantStore
  (#575), not a parallel ACL (#577 Kernkonzept #5). Encodes "skill X is
  shared with principal Y" as a Capability string (skill:read:<id>) and
  resolveSharedSkillIds(principal, roles, grants) turns a resolved
  capability set back into the ReadonlySet<string> P2's
  resolveSkillByName needs for its 'shared' bucket. Denials subtract
  from grants (same rule as the audience floor). Deliberately does NOT
  collapse "unresolved" (partial role lookup) to empty at this layer --
  SharedSkillIdsResult keeps the fact visible; toSharedSkillIdsSet is
  the explicit fail-closed adapter for callers who just want the
  resolver input. Consumes GrantStore/resolveCapabilities only -- no
  edits to grants.ts.

Tests: 15 new pure tests (skillSharing.test.ts) covering direct grants,
role-grant union, denial subtraction, unresolved-vs-empty, and the
fail-closed adapter; 2 new pure tests for assertHumanActor (all 4 system
origins blocked, every other ScopeId kind passes); 6 new pg-gated tests
covering the cron guard on all three mutating methods and the full
promotion flow (org target, team target, draft-refusal).

Mutation-tested: disabling assertHumanActor fails 4 tests across both
the pure and pg-gated suites; dropping the published-only gate on
promoteSkillOwnerScope fails 1 pg-gated test. Both reverted; working
tree clean afterward.

Not in this PR: the admin-gated HTTP route itself. Wiring a
session-authenticated Express route touches src/index.ts (the shared
app-bootstrap file, ~3800 lines, high concurrent-edit traffic across
this repo's parallel issue-harness sessions) and needs to correctly
replicate the existing session/auth middleware chain -- a promotion
endpoint with a subtly wrong auth check is a real security regression,
not a place to move fast. The service-layer method
(promoteSkillOwnerScope) is complete and fully tested; mounting it
behind route + session auth is left as a follow-up (naturally lands
with P4's admin UI, which needs a concrete endpoint contract anyway).
Weegy added a commit that referenced this pull request Aug 21, 2026
…r resolver must never reintroduce (#819)

#333 Phase 3 (#726) already delivered the full seam this issue describes: RoleHolderSource /
RoleHolderRegistry (harness-channel-sdk/src/roleHolderSource.ts), the conductor-local wiring
(src/conductor/roleHolderResolver.ts), and index.ts building the registry and threading it
into both the executor (resolveRoleHolders) and the await worker (holdersOnly). RunExecutor
already refuses quorum='all' completion on a partial holder list and already withholds the
no-holder fallback on a partial-empty lookup (runExecutor.ts resolveAwait / openHumanAwait).

What was still missing was the issue's second mandatory regression test:

- quorum='all' partial/unavailable non-completion already had a test
  (conductorQuorumAndTimeout.test.ts:127, from #726).
- 'role has no holder -> fallback' firing ONLY on a resolved-empty lookup, never on
  unavailable, had no test at all — openHumanAwait's fail-closed branch was unexercised.

Added that missing pair via ConductorRunExecutor.startRun with a minimal in-memory
workflow/run/await-store harness (same pattern as the existing quorum tests):
- a RESOLVED empty holder list takes the fallback path (records the step 'failed' with
  actor.noHolder=true) and never opens an await.
- an UNAVAILABLE (partial) holder lookup does NOT take the fallback -- it parks (opens an
  await, calls runStore.park) exactly as if a real holder existed but had not answered yet.

Mutation-verified: reverting either fail-closed guard in runExecutor.ts (the partial branch
in openHumanAwait, or the !holdersPartial conjunct in the quorum='all' completeness check)
turns the corresponding test red; both were confirmed against a clean tsc rebuild before and
after.

Also updated the roleStore.ts:22 comment (issue requirement #5) -- it still described the
external-resolver seam as future work after #726 had already filled it.
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.

1 participant