Skip to content

feat(tenants): dual-control export/delete requests + safer status transitions#19497

Closed
BrianCLong wants to merge 1 commit intomainfrom
codex/implement-multi-tenant-isolation-features
Closed

feat(tenants): dual-control export/delete requests + safer status transitions#19497
BrianCLong wants to merge 1 commit intomainfrom
codex/implement-multi-tenant-isolation-features

Conversation

@BrianCLong
Copy link
Copy Markdown
Owner

@BrianCLong BrianCLong commented Mar 7, 2026

Motivation

  • Harden tenant lifecycle controls by preventing unsafe status transitions (e.g., reactivating a disabled tenant via the generic PATCH flow) and by gating destructive or high-sensitivity operations behind dual-control approvals.
  • Make all sensitive operations auditable and governed by provenance entries and bounded per-tenant governance request records.
  • Surface enforcement and tests so CI and governance artifacts reflect the expanded Sprint +3 hosted SaaS hardening scope.

Description

  • Add stricter status transition checks in TenantService.updateStatus to reject transitions from disabled and other unsupported states and to persist a capped lifecycle.statusHistory and emit TENANT_STATUS_UPDATED provenance entries.
  • Introduce dual-control sensitive-operation workflows with new endpoints POST /api/tenants/:id/export-requests and POST /api/tenants/:id/delete-requests in server/src/routes/tenants.ts, including request validation, approval normalization, and dual-control validation via middleware/dual-control.
  • Implement TenantService.createSensitiveOperationRequest to persist a bounded governance requests array on the tenant config and to emit TENANT_EXPORT_REQUESTED / TENANT_DELETE_REQUESTED provenance entries.
  • Add route-level Jest tests in server/src/routes/__tests__/tenants.test.ts to cover status transition conflict handling, successful export request when dual-control is satisfied, and rejected delete requests when dual-control fails.
  • Update governance/readiness artifacts: docs/roadmap/STATUS.json, prompts/tenants/tenant-status-hardening@v1.md, prompts/registry.yaml, and agents/examples/TENANT_STATUS_HARDENING_20260206.json to reflect expanded scope and new success criteria.

Testing

  • Ran node scripts/check-boundaries.cjs which succeeded (no parallelization/boundary violations).
  • Added and exercised route-level tests in server/src/routes/__tests__/tenants.test.ts (status patch, export/delete request flows) but running the local Jest invocation (pnpm --filter intelgraph-server exec jest --config jest.config.ts -- tenants.test.ts) failed in this environment because the jest binary was not available; the test file is included and expected to run in CI where the test runtime is provisioned.
  • Mocked dual-control middleware in tests and verified behavior for both satisfied and unsatisfied dual-control scenarios (test assertions added; local execution of jest must be performed in CI/dev environment with jest installed).

Codex Task

Summary by CodeRabbit

  • New Features

    • Tenant status lifecycle management: update tenant states between active and suspended
    • Dual-approval gated export and delete request capabilities with comprehensive audit logging
  • Documentation

    • Added design guidance for tenant status hardening
  • Tests

    • Added test coverage for status transitions and dual-approval workflows

@BrianCLong BrianCLong added the codex Codex-owned implementation work label Mar 7, 2026 — with ChatGPT Codex Connector
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

@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 significantly hardens tenant lifecycle controls by preventing unsafe status transitions and gating high-sensitivity operations behind a dual-control approval mechanism. The changes ensure that all sensitive actions are auditable, governed by provenance entries, and recorded within bounded per-tenant governance request records. This enhancement improves the security posture and compliance readiness of the hosted SaaS platform by enforcing stricter controls and providing clear visibility into critical tenant-related operations.

Highlights

  • Enhanced Tenant Status Transitions: Implemented stricter status transition checks in TenantService.updateStatus to prevent unsafe state changes (e.g., reactivating a disabled tenant via generic PATCH) and to maintain a capped lifecycle.statusHistory with TENANT_STATUS_UPDATED provenance entries.
  • Dual-Control Sensitive Operations: Introduced new endpoints POST /api/tenants/:id/export-requests and POST /api/tenants/:id/delete-requests for sensitive operations, enforcing dual-control approvals via middleware/dual-control for enhanced security and auditability.
  • Persistent Governance Requests: Added TenantService.createSensitiveOperationRequest to persist a bounded governance requests array on the tenant config and to emit TENANT_EXPORT_REQUESTED / TENANT_DELETE_REQUESTED provenance entries for all sensitive operations.
  • Comprehensive Test Coverage: Included new route-level Jest tests in server/src/routes/__tests__/tenants.test.ts to cover status transition conflict handling, successful export requests with satisfied dual-control, and rejected delete requests when dual-control fails.
  • Updated Governance Artifacts: Updated docs/roadmap/STATUS.json, prompts/tenants/tenant-status-hardening@v1.md, prompts/registry.yaml, and agents/examples/TENANT_STATUS_HARDENING_20260206.json to reflect the expanded scope and new success criteria for hosted SaaS hardening.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • agents/examples/TENANT_STATUS_HARDENING_20260206.json
    • Added a new JSON file defining a task for tenant status hardening, including its scope, allowed operations, verification requirements, and success criteria.
  • docs/roadmap/STATUS.json
    • Updated the last_updated timestamp and revision_note to reflect the expanded scope of hosted SaaS hardening.
    • Added a new initiative with id: "sprint-plus3-hosted-saas-hardening" to the roadmap.
    • Corrected unicode characters in existing notes.
  • prompts/registry.yaml
    • Added a new prompt entry for tenant-status-hardening with its path, SHA256, description, scope, verification requirements, and allowed operations.
  • prompts/tenants/tenant-status-hardening@v1.md
    • Created a new markdown file outlining the objective, constraints, and evidence for tenant status hardening, including dual-control for export/delete requests.
  • server/src/routes/tests/tenants.test.ts
    • Refactored Jest imports for conciseness.
    • Added mocks for TenantService.updateStatus, TenantService.createSensitiveOperationRequest, dual-control.normalizeApprovalActors, and dual-control.validateDualControlRequirement.
    • Simplified mock implementations for createTenantSchema and createTenantBaseSchema.
    • Removed database import.
    • Updated beforeEach to include mocks for new service and dual-control functions.
    • Added tests for PATCH /api/tenants/:id to update tenant status, including conflict handling for blocked transitions.
    • Added tests for POST /api/tenants/:id/export-requests to verify export request creation when dual-control is satisfied.
    • Added tests for POST /api/tenants/:id/delete-requests to verify rejection when dual-control fails.
  • server/src/routes/tenants.ts
    • Imported normalizeApprovalActors and validateDualControlRequirement from ../middleware/dual-control.js.
    • Defined new Zod schemas: statusSchema for tenant status updates and sensitiveOperationSchema for export/delete requests, including approvalActorSchema.
    • Implemented a new PATCH /:id endpoint to update tenant status, including validation, error handling for blocked transitions, and provenance logging.
    • Implemented a new POST /:id/export-requests endpoint to handle tenant export requests, enforcing dual-control validation and logging provenance.
    • Implemented a new POST /:id/delete-requests endpoint to handle tenant delete requests, enforcing dual-control validation and logging provenance.
  • server/src/services/TenantService.ts
    • Defined the TenantSensitiveRequest interface to model sensitive operation requests.
    • Implemented updateStatus method to handle tenant status transitions, including validation for unsafe transitions, maintaining a capped status history in config, and emitting TENANT_STATUS_UPDATED provenance entries.
    • Implemented createSensitiveOperationRequest method to persist sensitive operation requests (export/delete) on the tenant's config (capped to 20 entries) and emit TENANT_EXPORT_REQUESTED or TENANT_DELETE_REQUESTED provenance entries.
Activity
  • Ran node scripts/check-boundaries.cjs which succeeded, indicating no parallelization or boundary violations.
  • Added and exercised route-level tests in server/src/routes/__tests__/tenants.test.ts covering status patch, export, and delete request flows. Local Jest execution failed due to environment setup, but the tests are expected to run in CI.
  • Mocked dual-control middleware in tests and verified behavior for both satisfied and unsatisfied dual-control scenarios with added assertions.
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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 7, 2026

Walkthrough

Implements tenant status hardening (Sprint +3) with new lifecycle controls via PATCH endpoint, dual-control-gated sensitive operation requests (export/delete), transactional status transitions, provenance logging, and corresponding task specification, documentation, prompts, and test coverage across routes, services, and governance configurations.

Changes

Cohort / File(s) Summary
Task Specification & Prompts
agents/examples/TENANT_STATUS_HARDENING_20260206.json, prompts/registry.yaml, prompts/tenants/tenant-status-hardening@v1.md
New task descriptor and prompt entry defining tenant status hardening requirements, including scope (server routes, governance, tenancy, compliance), allowed operations, verification tiers, and success criteria focused on API behavior, provenance, dual-control approvals, and roadmap alignment.
Roadmap Update
docs/roadmap/STATUS.json
Added new initiative (sprint-plus3-hosted-saas-hardening) with in_progress status; updated last_updated timestamp and revision_note; normalized en dashes in existing initiative notes.
API Routes Implementation
server/src/routes/tenants.ts
Three new routes: PATCH /:id for status updates with lifecycle history, POST /:id/export-requests and POST /:id/delete-requests for sensitive operations with dual-control enforcement; includes schema validation (statusSchema, sensitiveOperationSchema, approvalActorSchema) and error handling.
Service Layer
server/src/services/TenantService.ts
Added updateStatus method for transactional status transitions with history tracking and provenance logging; added createSensitiveOperationRequest method for sensitive operation persistence with approval metadata and governance updates; introduced TenantSensitiveRequest interface.
Test Coverage
server/src/routes/__tests__/tenants.test.ts
Extended mocks for tenantService.updateStatus and tenantService.createSensitiveOperationRequest; added dual-control middleware mocks (normalizeApprovalActors, validateDualControlRequirement); added test scenarios for status updates, blocked transitions, export-requests with dual-control satisfaction, and delete-requests rejection when dual-control unsatisfied.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant API as PATCH /:id<br/>Endpoint
    participant Schema as Zod<br/>Validator
    participant Service as TenantService
    participant ProvenanceLedger
    participant Database

    Client->>API: PATCH /api/tenants/{id}<br/>(status, reason, actor)
    API->>Schema: Validate status schema
    Schema-->>API: Valid
    API->>Service: updateStatus(tenantId,<br/>status, actorId, reason)
    Service->>Database: BEGIN TRANSACTION
    Service->>Service: Validate transition<br/>(active ↔ suspended)
    Service->>Database: Update tenant.lifecycle<br/>.statusHistory
    Service->>ProvenanceLedger: Log provenance event<br/>(actor, status, reason)
    ProvenanceLedger-->>Service: Event recorded
    Service->>Database: COMMIT
    Database-->>Service: Transaction complete
    Service-->>API: Updated Tenant +<br/>Receipt
    API->>Schema: Format response
    API-->>Client: 200 OK<br/>{data, receipt}
Loading
sequenceDiagram
    actor Client
    participant API as POST /:id/<br/>export-requests
    participant Schema as Zod<br/>Validator
    participant DualControl as Dual-Control<br/>Middleware
    participant Service as TenantService
    participant ProvenanceLedger
    participant Database

    Client->>API: POST /api/tenants/{id}<br/>/export-requests<br/>(reason, approvals)
    API->>Schema: Validate sensitive<br/>operation schema
    Schema-->>API: Valid
    API->>DualControl: normalizeApprovalActors
    DualControl-->>API: Normalized approvals
    API->>DualControl: validateDualControlRequirement<br/>(approvals, roles,<br/>action: export)
    alt Dual-Control Satisfied
        DualControl-->>API: Valid (2 approvals,<br/>compliance-officer)
        API->>Service: createSensitiveOperationRequest<br/>(tenantId, export,<br/>reason, actorId, approvals)
        Service->>Database: BEGIN TRANSACTION
        Service->>Database: Update tenant.config<br/>.governance.requests
        Service->>ProvenanceLedger: Log operation with<br/>approvals metadata
        Service->>Database: COMMIT
        Service-->>API: TenantSensitiveRequest
        API-->>Client: 202 Accepted<br/>{data, receipt}
    else Dual-Control Not Satisfied
        DualControl-->>API: Violations found
        API-->>Client: 403 Forbidden<br/>{violations}
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Hop, hop, the tenant status hops along,
With dual-control guards keeping governance strong!
Patches and exports, delete requests too,
Provenance ledgers log all that we do!
Sprint +3 hardening—secure and serene,

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers motivation, implementation details, testing approach, and governance artifact updates. However, critical sections from the template are missing: Risk & Surface (risk level and surface area checkboxes), Assumption Ledger, Execution Governor & Customer Impact, Evidence Bundle checklist, Security Impact, and Green CI Contract Checklist. Add all required template sections including Risk Level selection, Surface Area checkboxes, Assumption Ledger details, Execution Governor compliance, Evidence Bundle verification, Security Impact assessment, and Green CI Contract Checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main changes: dual-control export/delete requests and safer status transitions for tenants, which aligns perfectly with the PR's core objectives and changeset.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/implement-multi-tenant-isolation-features

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.

❤️ Share

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

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 introduces significant enhancements to tenant lifecycle management, including dual-control for export and delete operations and safer status transitions. However, a high-severity security vulnerability exists where the dual-control mechanism in the routes can be bypassed, as the current design allows a requester to spoof approvals in the request body. This requires a multi-step, server-side approval workflow. Additionally, the review identifies opportunities to improve maintainability by reducing code duplication in route handlers, make error handling more robust using custom error types, and enhance type safety and provenance data accuracy.

Comment on lines +381 to +392
const parsed = sensitiveOperationSchema.parse(req.body);
const approvals = normalizeApprovalActors(parsed.approvals);

const dualControl = await validateDualControlRequirement({
requestId: parsed.requestId || randomUUID(),
action: 'tenant_delete_request',
tenantId,
requesterId: actorId,
requiredApprovals: 2,
requiredRoles: ['compliance-officer', 'security-admin'],
approvals,
});
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.

security-high high

The delete-requests endpoint has a high-severity security flaw: it allows the requester to provide approvals in the request body, bypassing dual-control. The server does not verify these approvals, enabling a single user to spoof approval data. A server-side, state-based approval workflow is required where each approver logs in independently. Furthermore, the route handlers for export-requests and delete-requests exhibit significant code duplication, which increases maintenance overhead. Refactoring this shared logic into a single, parameterized helper function would improve maintainability.

Comment on lines +322 to +333
const parsed = sensitiveOperationSchema.parse(req.body);
const approvals = normalizeApprovalActors(parsed.approvals);

const dualControl = await validateDualControlRequirement({
requestId: parsed.requestId || randomUUID(),
action: 'tenant_export_request',
tenantId,
requesterId: actorId,
requiredApprovals: 2,
requiredRoles: ['compliance-officer'],
approvals,
});
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.

security-high high

The dual-control implementation for sensitive operations (export/delete requests) is insecure because it allows the requester to provide the list of approvals directly in the request body. The validateDualControlRequirement function is called with these user-supplied approvals without any verification of their authenticity (e.g., cryptographic signatures or independent server-side approval records). An attacker can bypass the dual-control requirement by providing fake approval entries in the JSON payload, allowing a single user to authorize sensitive operations that are intended to require multiple independent approvals. To remediate this, implement a multi-step approval process where approvers must independently authenticate and approve a pending request record stored on the server.

Comment on lines +296 to +304
if (
error instanceof Error
&& (
error.message.includes('Disabled tenants require dedicated reactivation workflow')
|| error.message.includes('Unsupported current status transition')
)
) {
return res.status(409).json({ success: false, error: error.message });
}
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

Relying on string matching for error messages is fragile. If the error message in TenantService is modified, this logic will fail silently. A more robust approach is to use custom error classes. You can define a specific error class (e.g., InvalidTenantStateTransitionError) in your service layer, throw it, and then check for it in the route handler using instanceof. This makes the contract between the service and route layers explicit and less prone to breaking.


await provenanceLedger.appendEntry({
action: 'TENANT_STATUS_UPDATED',
actor: { id: actorId, role: 'admin' },
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 actor's role is hardcoded as 'admin' when creating the provenance entry. This reduces the value of the audit log, as it doesn't capture the actual role of the user performing the action. The user's role is available in the route handler and should be passed to this service method.

Consider changing the method signature to accept an actor object with both id and role:
async updateStatus(tenantId: string, status: 'active' | 'suspended', actor: { id: string; role: string }, reason?: string)

This will allow you to record the correct role, making the provenance data more accurate and useful.

Suggested change
actor: { id: actorId, role: 'admin' },
actor: { id: actor.id, role: actor.role },

Comment on lines +549 to +551
const governance = Array.isArray((current.config as any)?.governance?.requests)
? (current.config as any).governance.requests
: [];
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

Using as any to access nested properties on current.config bypasses TypeScript's type safety and can lead to runtime errors if the object structure changes. To improve type safety and code clarity, it's better to define a more specific type for the Tenant['config'] object that includes expected structures like governance and lifecycle.

For example:

interface TenantConfig {
  lifecycle?: {
    statusHistory: any[];
  };
  governance?: {
    requests: TenantSensitiveRequest[];
  };
}

export interface Tenant {
  // ...
  config: TenantConfig;
}

With a properly typed config object, you can safely access its properties without type casting.

      const governance = current.config?.governance?.requests ?? [];


await provenanceLedger.appendEntry({
action: action === 'export' ? 'TENANT_EXPORT_REQUESTED' : 'TENANT_DELETE_REQUESTED',
actor: { id: actorId, role: 'admin' },
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

Similar to the updateStatus method, the actor's role is hardcoded as 'admin'. For more accurate and meaningful audit trails, the actual user role should be passed in from the route handler and used here.

Consider modifying the function signature to accept an actor object:
async createSensitiveOperationRequest(..., actor: { id: string; role: string }, ...)

This will ensure the provenance ledger contains precise information about who performed the action.

        actor: { id: actor.id, role: actor.role },

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/src/routes/tenants.ts`:
- Around line 325-350: The code generates different request IDs for validation
and persistence when parsed.requestId is missing; instead, create a single
requestId variable (e.g., const requestId = parsed.requestId || randomUUID())
and pass that same requestId into validateDualControlRequirement (requestId:
requestId) and tenantService.createSensitiveOperationRequest (last arg) so both
use the identical ID; make the same change in the delete-request route where the
same pattern occurs.
- Around line 322-333: Do not trust approval.role from the request body: instead
of passing parsed.approvals (and the caller-supplied role strings) into
validateDualControlRequirement, resolve each approver identity and their roles
server-side and pass those verified values. Concretely, in the tenant export
endpoints (where sensitiveOperationSchema.parse and normalizeApprovalActors are
used) ignore any approval.role from req.body, use the approver actorId(s) from
normalized approvals to fetch the canonical actor record/roles (e.g., via your
existing user/actor lookup like getActorById or a roles service), build a new
approvals array containing the server-resolved actorId and role(s)/claims, and
pass that verified approvals array into validateDualControlRequirement (same in
the other endpoint noted). Ensure validateDualControlRequirement receives only
server-validated roles so callers cannot fabricate required roles like
"compliance-officer".

In `@server/src/services/TenantService.ts`:
- Around line 464-467: The current branch in TenantService that detects
no-change status (the if (current.status === status) block) should return an
explicit no-op result rather than the same tenant object so callers (e.g., the
PATCH handler that emits TENANT_STATUS_UPDATED) can distinguish true updates
from no-ops; modify the updateStatus/updateTenantStatus method to return a
structured response indicating noOp (for example { noOp: true, tenant: current }
or a result enum with NO_OP vs UPDATED) and keep existing behavior for actual
updates (e.g., { noOp: false, tenant: updated }), then update callers to check
noOp before emitting TENANT_STATUS_UPDATED.
- Around line 450-493: The tenant row is read and then rewritten without any
lock, allowing concurrent transactions to clobber lifecycle fields; modify the
mutator (e.g., updateStatus in TenantService) to SELECT the tenant using SELECT
* FROM tenants WHERE id = $1 FOR UPDATE inside the same transaction (client)
before mapping with mapRowToTenant and before computing nextConfig, then perform
the UPDATE and RETURNING based on that locked row; apply the same FOR UPDATE
locking (or equivalent optimistic version check using a version/updated_at WHERE
clause) to the other mutator that updates tenant config/status (the similar
block around the governance.requests change) so both paths serialize reads and
writes to prevent lost updates.
- Around line 497-509: The provenance append is executed outside the tenant DB
transaction because provenanceLedger.appendEntry(...) performs its own commit,
so move the provenance write into the same transaction by either (A) changing
appendEntry to accept a client/transaction parameter and call it with the
current transaction client in TenantService (so the ledger write uses the same
connection and does not commit independently), or (B) instead of calling
provenanceLedger.appendEntry(...) inside TenantService, insert a staged outbox
row (e.g. provenance_outbox) within the same transaction alongside the tenant
update and commit both together, letting a background worker/process pick up the
outbox and call provenanceLedger.appendEntry() afterwards; update calls at both
the shown block around client.query('COMMIT') and the other occurrence (lines
~566-579) to use the chosen approach.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4bd389d0-d3f9-4334-af0a-fe54da2b2a1e

📥 Commits

Reviewing files that changed from the base of the PR and between 68c99c3 and 872348e.

📒 Files selected for processing (7)
  • agents/examples/TENANT_STATUS_HARDENING_20260206.json
  • docs/roadmap/STATUS.json
  • prompts/registry.yaml
  • prompts/tenants/tenant-status-hardening@v1.md
  • server/src/routes/__tests__/tenants.test.ts
  • server/src/routes/tenants.ts
  • server/src/services/TenantService.ts

Comment on lines +322 to +333
const parsed = sensitiveOperationSchema.parse(req.body);
const approvals = normalizeApprovalActors(parsed.approvals);

const dualControl = await validateDualControlRequirement({
requestId: parsed.requestId || randomUUID(),
action: 'tenant_export_request',
tenantId,
requesterId: actorId,
requiredApprovals: 2,
requiredRoles: ['compliance-officer'],
approvals,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Do not trust approval.role from the request body.

These endpoints feed client-supplied role strings straight into dual-control validation. Since the validator only checks whether the required role names are present, a caller can invent compliance-officer / security-admin approvals and satisfy the gate without any real approver. Resolve approver identities and roles server-side before evaluating the requirement.

Also applies to: 384-392

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/routes/tenants.ts` around lines 322 - 333, Do not trust
approval.role from the request body: instead of passing parsed.approvals (and
the caller-supplied role strings) into validateDualControlRequirement, resolve
each approver identity and their roles server-side and pass those verified
values. Concretely, in the tenant export endpoints (where
sensitiveOperationSchema.parse and normalizeApprovalActors are used) ignore any
approval.role from req.body, use the approver actorId(s) from normalized
approvals to fetch the canonical actor record/roles (e.g., via your existing
user/actor lookup like getActorById or a roles service), build a new approvals
array containing the server-resolved actorId and role(s)/claims, and pass that
verified approvals array into validateDualControlRequirement (same in the other
endpoint noted). Ensure validateDualControlRequirement receives only
server-validated roles so callers cannot fabricate required roles like
"compliance-officer".

Comment on lines +325 to +350
const dualControl = await validateDualControlRequirement({
requestId: parsed.requestId || randomUUID(),
action: 'tenant_export_request',
tenantId,
requesterId: actorId,
requiredApprovals: 2,
requiredRoles: ['compliance-officer'],
approvals,
});

if (!dualControl.satisfied) {
return res.status(403).json({
success: false,
error: 'Dual-control requirement not satisfied',
violations: dualControl.violations,
});
}

const requestRecord = await tenantService.createSensitiveOperationRequest(
tenantId,
'export',
parsed.reason,
actorId,
approvals,
parsed.requestId,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use one requestId through validation and persistence.

When the body omits requestId, the dual-control check gets a fresh UUID, but createSensitiveOperationRequest() gets undefined and generates a different ID. That breaks audit correlation and any request-level dedupe keyed by the validator's ID.

🔁 Proposed fix
+      const requestId = parsed.requestId ?? randomUUID();

       const dualControl = await validateDualControlRequirement({
-        requestId: parsed.requestId || randomUUID(),
+        requestId,
         action: 'tenant_export_request',
         tenantId,
         requesterId: actorId,
         requiredApprovals: 2,
         requiredRoles: ['compliance-officer'],
         approvals,
       });

       const requestRecord = await tenantService.createSensitiveOperationRequest(
         tenantId,
         'export',
         parsed.reason,
         actorId,
         approvals,
-        parsed.requestId,
+        requestId,
       );

Apply the same change in the delete-request route.

Also applies to: 384-409

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/routes/tenants.ts` around lines 325 - 350, The code generates
different request IDs for validation and persistence when parsed.requestId is
missing; instead, create a single requestId variable (e.g., const requestId =
parsed.requestId || randomUUID()) and pass that same requestId into
validateDualControlRequirement (requestId: requestId) and
tenantService.createSensitiveOperationRequest (last arg) so both use the
identical ID; make the same change in the delete-request route where the same
pattern occurs.

Comment on lines +450 to +493
const existing = await client.query('SELECT * FROM tenants WHERE id = $1', [tenantId]);
if (!existing.rowCount) {
throw new Error('Tenant not found');
}

const current = this.mapRowToTenant(existing.rows[0]);
if (current.status === 'disabled') {
throw new Error('Disabled tenants require dedicated reactivation workflow');
}

if (!['active', 'suspended'].includes(current.status)) {
throw new Error(`Unsupported current status transition from '${current.status}'`);
}

if (current.status === status) {
await client.query('ROLLBACK');
return current;
}

const statusHistory = Array.isArray(current.config?.lifecycle?.statusHistory)
? current.config.lifecycle.statusHistory
: [];

const nextConfig = {
...(current.config || {}),
lifecycle: {
...(current.config?.lifecycle || {}),
statusHistory: [
{
previousStatus: current.status,
nextStatus: status,
actorId,
reason,
changedAt: new Date().toISOString(),
},
...statusHistory,
].slice(0, 10),
},
};

const result = await client.query(
'UPDATE tenants SET status = $1, config = $2, updated_at = NOW() WHERE id = $3 RETURNING *',
[status, nextConfig, tenantId],
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Lock the tenant row before rewriting status and config.

Both methods read the tenant row, derive new state from that snapshot, and then write it back without any lock or version guard. A concurrent disableTenant, second status update, or another sensitive-request insert can overwrite lifecycle.statusHistory / governance.requests, and updateStatus() can still commit after another transaction has already disabled the tenant. Use SELECT ... FOR UPDATE or optimistic concurrency consistently across tenant mutators before calculating the next state.

Also applies to: 533-564

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/services/TenantService.ts` around lines 450 - 493, The tenant row
is read and then rewritten without any lock, allowing concurrent transactions to
clobber lifecycle fields; modify the mutator (e.g., updateStatus in
TenantService) to SELECT the tenant using SELECT * FROM tenants WHERE id = $1
FOR UPDATE inside the same transaction (client) before mapping with
mapRowToTenant and before computing nextConfig, then perform the UPDATE and
RETURNING based on that locked row; apply the same FOR UPDATE locking (or
equivalent optimistic version check using a version/updated_at WHERE clause) to
the other mutator that updates tenant config/status (the similar block around
the governance.requests change) so both paths serialize reads and writes to
prevent lost updates.

Comment on lines +464 to +467
if (current.status === status) {
await client.query('ROLLBACK');
return current;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Surface no-op status requests explicitly.

This branch returns the current tenant as if the update succeeded. The PATCH route in server/src/routes/tenants.ts Lines 284-287 always emits a TENANT_STATUS_UPDATED receipt on success, so callers can get an "updated" receipt even though no row write or provenance entry happened. Return an explicit no-op result or separate status so the route can avoid issuing a false audit record.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/services/TenantService.ts` around lines 464 - 467, The current
branch in TenantService that detects no-change status (the if (current.status
=== status) block) should return an explicit no-op result rather than the same
tenant object so callers (e.g., the PATCH handler that emits
TENANT_STATUS_UPDATED) can distinguish true updates from no-ops; modify the
updateStatus/updateTenantStatus method to return a structured response
indicating noOp (for example { noOp: true, tenant: current } or a result enum
with NO_OP vs UPDATED) and keep existing behavior for actual updates (e.g., {
noOp: false, tenant: updated }), then update callers to check noOp before
emitting TENANT_STATUS_UPDATED.

Comment on lines +497 to +509
await provenanceLedger.appendEntry({
action: 'TENANT_STATUS_UPDATED',
actor: { id: actorId, role: 'admin' },
metadata: {
tenantId,
previousStatus: current.status,
nextStatus: status,
reason,
},
artifacts: [],
});

await client.query('COMMIT');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

The provenance write and tenant write are not atomic.

provenanceLedger.appendEntry() commits on its own database transaction in server/src/provenance/ledger.ts Lines 230-335, so a successful ledger append followed by an outer COMMIT failure leaves an audit record for a status/request change that never persisted. For governance flows, write the event in the same transaction or stage it in an outbox row that is committed with the tenant change.

Also applies to: 566-579

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/services/TenantService.ts` around lines 497 - 509, The provenance
append is executed outside the tenant DB transaction because
provenanceLedger.appendEntry(...) performs its own commit, so move the
provenance write into the same transaction by either (A) changing appendEntry to
accept a client/transaction parameter and call it with the current transaction
client in TenantService (so the ledger write uses the same connection and does
not commit independently), or (B) instead of calling
provenanceLedger.appendEntry(...) inside TenantService, insert a staged outbox
row (e.g. provenance_outbox) within the same transaction alongside the tenant
update and commit both together, letting a background worker/process pick up the
outbox and call provenanceLedger.appendEntry() afterwards; update calls at both
the shown block around client.query('COMMIT') and the other occurrence (lines
~566-579) to use the chosen approach.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🤖 Auto-approved by Mega Merge Orchestrator

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🤖 Auto-approved by Mega Merge Orchestrator

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🤖 Auto-approved by Mega Merge Orchestrator

@BrianCLong
Copy link
Copy Markdown
Owner Author

Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges.

1 similar comment
@BrianCLong
Copy link
Copy Markdown
Owner Author

Temporarily closing to reduce Actions queue saturation and unblock #22241. Reopen after the golden-main convergence PR merges.

@BrianCLong BrianCLong closed this Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex Codex-owned implementation work queue:blocked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant