Repository navigation
feat: cross-project workflow rules (closes discussion #235) - #238
ahmadoqsrawi wants to merge 9 commits into
Conversation
Companion doc for Platform-Collective/platform discussion Platform-Collective#235. Describes: - WorkflowRule data model (per-project rule attached to a linked project) - forwardMap + allowedLoops + autoCreate shape - Server-side Tx trigger pattern reusing TxApplyIf.scope for echo-prevention - UI tab under Project Settings with a matrix editor - Explicitly deferred to a later feature: fan-out, field mirror, delete propagation, JSON import/export
…rules Adds @hcengineering/workflow-rules — the type-declaration plugin for the feature in discussion Platform-Collective#235. Compiles cleanly against the existing monorepo with no changes to tracker, task or core. Shape: - WorkflowRule (space-attached Doc, one direction per rule) - forwardMap (Ref<IssueStatus> -> Ref<IssueStatus>) - allowedLoops (explicit backward transitions on the target) - autoCreate (triggerStatuses + componentMap + inherit whitelist) - linkKind ('subIssue' | 'parentIssue' for the MVP, extension point) - enabled soft-off switch See docs/workflow-rules/DESIGN.md for the full rationale. Model registration, server trigger, and UI follow in subsequent commits.
Adds the four packages that let the Transactor run the cross-project status
mirror and wires them into the existing bootstrap:
- models/workflow-rules — TWorkflowRule class, DOMAIN_WORKFLOW_RULES
- server-plugins/workflow-rules — OnWorkflowRuleMirror trigger descriptor
- server-plugins/workflow-rules-resources — trigger implementation:
* TxUpdateDoc<Issue> listener with status-change short-circuit
* resolveCounterpart() for subIssue + parentIssue link kinds
* checkRegressionGuard() consulting TaskType.statuses as rank
* echo-prevention via "no change needed" counterpart.status === target
- models/server-workflow-rules — createDoc(serverCore.class.Trigger) binding
- models/all + server/server-pipeline — new workflow-rules modules in the
model aggregator and the resource location registry
Jest unit tests on the pure logic helpers:
Test Suites: 1 passed, 1 total
Tests: 9 passed, 9 total
Auto-create path is a logged no-op for this PR; the sequence allocation
needed to emit TxCreateDoc<Issue> wants its own review and is tracked for
the follow-up. UI tab and i18n assets also follow in later commits.
Replaces em-dash and en-dash characters with plain "-" across the files authored for this feature. Matches the house style used elsewhere in the tree and avoids rendering surprises in editors with non-UTF8 defaults. No logic changes.
Adds @hcengineering/workflow-rules-assets with: - lang/en.json: English strings for the matrix editor UI - assets/icons.svg: single workflow-rule icon - src/index.ts: loadMetadata(workflow-rules.icon) - src/lang.ts: dynamic Loader Wires the loader into: - dev/prod/src/platform.ts (webpack client bundle) - server/server-pipeline/src/internationalization.ts (server i18n) Follows the shape used by every other *-assets package in the tree, so rush-build-cache and locale-expansion by translators work the same way.
Three fixes to make the whole monorepo pass "rush validate": - plugins/workflow-rules: WorkflowRule class ref typed as Ref<Class<WorkflowRule>> so @model() accepts it. The earlier Ref<Doc> was too wide and tripped TS2345 on the decorator call site. - server-plugins/workflow-rules-resources: narrow the findAll result via `as unknown as WorkflowRule[]` so the strict overlap check in TS2352 is satisfied without widening the public call signature. - plugins/workflow-rules-assets: extend profiles/assets instead of profiles/default (unlocks `require` from @types/node) and switch the eslint config to the matching assets profile. Matches tracker-assets.
MichaelUray
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I'd address the model registration, cross-project authorization, cascade termination and transition correctness before enabling mirroring. I've included focused suggestions where the replacement is self-contained and sketches for the broader changes. Auto-create currently only logs, so I haven't treated it as an implemented copy path.
| import { serverTemplatesId, createModel as serverTemplatesModel } from '@hcengineering/model-server-templates' | ||
| import { serverTrackerId, createModel as serverTrackerModel } from '@hcengineering/model-server-tracker' | ||
| import { serverViewId, createModel as serverViewModel } from '@hcengineering/model-server-view' | ||
| import { serverWorkflowRulesId, createModel as serverWorkflowRulesModel } from '@hcengineering/model-server-workflow-rules' |
There was a problem hiding this comment.
[P1] Import the rule model as well as its trigger model
I only see the server trigger model imported here. The package dependency does not execute models/workflow-rules's createModel, so WorkflowRule and its domain never enter the hierarchy. Rule creation then fails, and status updates query an unknown class. Please apply this import together with the builder entry suggested below, and cover fresh model construction and workspace upgrades.
| import { serverWorkflowRulesId, createModel as serverWorkflowRulesModel } from '@hcengineering/model-server-workflow-rules' | |
| import { serverWorkflowRulesId, createModel as serverWorkflowRulesModel } from '@hcengineering/model-server-workflow-rules' | |
| import { workflowRulesId, createModel as workflowRulesModel } from '@hcengineering/model-workflow-rules' |
There was a problem hiding this comment.
Already applied in f4bd93a right before you reviewed. Caught the same bug end to end while running the stack locally. The import is there now; mind re-checking that commit?
| [serverTagsModel, serverTagsId], | ||
| [serverTaskModel, serverTaskId], | ||
| [serverTrackerModel, serverTrackerId], | ||
| [serverWorkflowRulesModel, serverWorkflowRulesId], |
There was a problem hiding this comment.
[P1] Execute the rule model during model construction
This is the builder entry paired with the import suggestion above. Registering the trigger alone leaves its queried class undefined. Both suggestions are needed; the rule model should be built before its server trigger model.
| [serverWorkflowRulesModel, serverWorkflowRulesId], | |
| [workflowRulesModel, workflowRulesId], | |
| [serverWorkflowRulesModel, serverWorkflowRulesId], |
There was a problem hiding this comment.
Same here, the builder entry landed in f4bd93a alongside the import. Please take a look when you get a sec.
| result.push( | ||
| control.txFactory.createTxUpdateDoc( | ||
| counterpart._class, | ||
| counterpart.space, | ||
| counterpart._id, | ||
| { status: target }, | ||
| false, | ||
| updateTx.modifiedOn | ||
| ) |
There was a problem hiding this comment.
[P1] Enforce rule-management permissions and target-project authorization
Once the model is registered, an ordinary writable rule in project A can select project B and cause this privileged derived write. Keeping the source actor in modifiedBy does not authorize that actor to write B: trigger reads and derived writes do not pass through the ordinary space-permission checks in the same way as a client write. With an existing cross-project parent/child link, an A-only member can therefore change B's issue status. An existing rule can also be activated by an actor permitted to change the source issue.
Please enforce the intended owner/admin rule-management policy on create, update and removal, including edits to the linked project and mapping. Before execution, require explicit target-project approval for the rule and verify that the approved delegation covers this status write. Source-project membership alone is insufficient. This needs server permission/model changes, so the following is an integration sketch, not an applicable suggestion; authorizeWorkflowRule is a new server policy function to implement:
const authorization = await authorizeWorkflowRule(control, updateTx, rule, counterpart, target)
if (!authorization.ruleManagementAuthorized ||
!authorization.targetProjectApproved ||
!authorization.statusWriteAuthorized) {
continue
}Please add permission-pipeline coverage for an A-only member, an unapproved target, mapping/target edits, and activation by a source-issue guest.
There was a problem hiding this comment.
Fair concern and I agree the current trigger lets an A-only member drive a privileged write on B. This one needs a new server policy (authorizeWorkflowRule) plus permission tests, and it changes the model a bit (target consent). I'd rather do it in a follow up PR so the review stays focused. Want me to open it as #239 right after this one lands, or do you prefer it in this PR?
| export async function OnWorkflowRuleMirror (txes: Tx[], control: TriggerControl): Promise<Tx[]> { | ||
| const result: Tx[] = [] | ||
| for (const actualTx of txes) { | ||
| if (actualTx._class !== core.class.TxUpdateDoc) continue |
There was a problem hiding this comment.
[P1] Bound mirror cascades independently of status equality
Equality only stops mappings that converge. For two linked issues, A→B mapping 0→1, 1→0 and B→A mapping 0→0, 1→1, with 1→0 allowed on both targets, repeats B=1 → A=1 → B=0 → A=0. Every equality check passes and derived writes invoke this trigger again.
For a one-hop MVP, this guard prevents derived status updates from starting another mirror. It deliberately also skips status updates produced by other server triggers. If multi-hop chains are required, use a cascade-scoped visited rule/issue set and a hard depth bound instead. Please update the equality-only JSDoc and test non-inverse mappings with allowed regressions.
| if (actualTx._class !== core.class.TxUpdateDoc) continue | |
| if (actualTx._class !== core.class.TxUpdateDoc || actualTx.space === core.space.DerivedTx) continue |
There was a problem hiding this comment.
Done in d3a865a. Added the DerivedTx check so a mirror cannot start another mirror. One hop by design for v1. Multi hop would want a visited set and a depth bound as you noted; left a jsdoc about it and about the fact we deliberately skip other triggers' status writes.
| if (tt === undefined) return false | ||
|
|
||
| const statuses: Array<Ref<IssueStatus>> = (tt.statuses as Array<Ref<IssueStatus>>) ?? [] | ||
| return isRegression(counterpart.status, target, statuses, rule.allowedLoops) |
There was a problem hiding this comment.
[P2] Reject statuses outside the target TaskType
A missing TaskType currently returns false, and an unknown status is also allowed by isRegression. This lets a stale, deleted or foreign status reference be written onto the target issue. Check membership before consulting loop exceptions; an allowedLoops entry must not authorize an invalid status. This replacement makes the runtime guard fail closed. Please also change the helper's unknown-status behavior and the existing test that expects it to pass.
| if (tt === undefined) return false | |
| const statuses: Array<Ref<IssueStatus>> = (tt.statuses as Array<Ref<IssueStatus>>) ?? [] | |
| return isRegression(counterpart.status, target, statuses, rule.allowedLoops) | |
| if (tt === undefined || !Array.isArray(tt.statuses)) return true | |
| const statuses = tt.statuses as Array<Ref<IssueStatus>> | |
| if (!statuses.includes(counterpart.status) || !statuses.includes(target)) return true | |
| if (!Array.isArray(rule.allowedLoops)) return true | |
| if (rule.allowedLoops.some((loop) => | |
| loop == null || typeof loop.from !== 'string' || typeof loop.to !== 'string' | |
| )) return true | |
| return isRegression(counterpart.status, target, statuses, rule.allowedLoops) |
There was a problem hiding this comment.
Done in d3a865a. checkRegressionGuard now fails closed when the TaskType is missing, when either status is not a member of the TaskType's statuses, or when allowedLoops is malformed. isRegression matches the same contract. Updated the test to assert the new behavior.
| for (const rule of rules) { | ||
| const counterpart = await resolveCounterpart(control, source, rule) |
There was a problem hiding this comment.
[P2] Validate persisted rule JSON before executing each rule
The TypeScript interface does not validate stored documents. A missing forwardMap throws during lookup, a missing allowedLoops throws in the helper, and malformed autoCreate.triggerStatuses can throw before the mirror path. One malformed rule then aborts this invocation and discards updates already accumulated for other rules. This read-side check covers the fields currently consumed by the trigger and skips malformed rules individually. Please also validate project references and the complete rule shape on server-side creation/edits; read validation does not replace the authorization checks above.
| for (const rule of rules) { | |
| const counterpart = await resolveCounterpart(control, source, rule) | |
| for (const rule of rules) { | |
| if ( | |
| typeof rule.linkedProject !== 'string' || rule.linkedProject.length === 0 || | |
| (rule.linkKind !== 'subIssue' && rule.linkKind !== 'parentIssue') || | |
| rule.forwardMap == null || typeof rule.forwardMap !== 'object' || Array.isArray(rule.forwardMap) || | |
| Object.entries(rule.forwardMap).some(([from, to]) => | |
| from.length === 0 || typeof to !== 'string' || to.length === 0 | |
| ) || | |
| !Array.isArray(rule.allowedLoops) || | |
| rule.allowedLoops.some((loop) => | |
| loop == null || typeof loop.from !== 'string' || typeof loop.to !== 'string' || | |
| loop.from.length === 0 || loop.to.length === 0 | |
| ) || | |
| (rule.autoCreate != null && ( | |
| !Array.isArray(rule.autoCreate.triggerStatuses) || | |
| rule.autoCreate.triggerStatuses.some((status) => typeof status !== 'string' || status.length === 0) | |
| )) | |
| ) { | |
| control.ctx.warn('[workflow-rules] invalid rule skipped', { ruleId: rule._id }) | |
| continue | |
| } | |
| const counterpart = await resolveCounterpart(control, source, rule) |
There was a problem hiding this comment.
Done in d3a865a. Added isValidRule covering linkedProject, linkKind, forwardMap entries, allowedLoops entries, and autoCreate.triggerStatuses. Malformed rules log a warn and the loop continues for the other rules on the same source. I kept the write path (create/edit) out of this commit because that is more invasive; happy to add server side create/edit validation in the follow up PR with the authorization work.
| const target = rule.forwardMap[newStatus] | ||
| if (target == null) continue |
There was a problem hiding this comment.
[P2] Require an own, valid status-map entry
This lookup can also read inherited object properties, and persisted JSON can contain non-string mapping values. For an unmapped source value such as constructor, an ordinary object lookup can produce a non-status value. Please require an own entry and a non-empty string before resolving a transition. This is independent of validating membership in the target TaskType.
| const target = rule.forwardMap[newStatus] | |
| if (target == null) continue | |
| if (!Object.prototype.hasOwnProperty.call(rule.forwardMap, newStatus)) continue | |
| const target = rule.forwardMap[newStatus] | |
| if (typeof target !== 'string' || target.length === 0) continue |
There was a problem hiding this comment.
Done in d3a865a. hasOwnProperty check first, then a typeof string and length check on the mapped value. constructor and other prototype keys do not resolve to junk anymore.
| if (counterpart.status === target) continue // echo short-circuit | ||
|
|
||
| const regressed = await checkRegressionGuard(control, counterpart, target, rule) |
There was a problem hiding this comment.
[P2] Evaluate regression rules against the effective pending status
Updates are queued until this function returns. If two rules or source transactions target the same issue, both read its original status: from Backlog, queued moves to Done and then In Progress both pass, but applying them performs a forbidden regression. Please define conflict ordering, retain the pending status per target across the whole batch, and use it for both equality and regression checks. For example, the following is a larger-change sketch, with the map initialized once before the transaction loop:
const pendingStatuses = new Map<Ref<Issue>, Ref<IssueStatus>>()
// Inside the rule loop, after resolving the counterpart and mapping:
const effectiveCounterpart: Issue = {
...counterpart,
status: pendingStatuses.get(counterpart._id) ?? counterpart.status
}
if (effectiveCounterpart.status === target) continue
if (await checkRegressionGuard(control, effectiveCounterpart, target, rule)) continue
// After enqueueing the accepted update:
pendingStatuses.set(counterpart._id, target)Match the conflict order to the transaction application order, and guard the persisted write against intervening edits with a conditional update/recheck. The pending map alone does not solve concurrent writers. Please test multiple rules and multiple source transactions targeting one issue.
There was a problem hiding this comment.
Good catch. I'd like to do this one in the follow up PR with the authorization work since both need the same conflict ordering story and tests. For v1 the trigger produces at most one update per rule per source transaction, and the DerivedTx skip prevents it chaining into itself; the race you describe needs multiple source rules targeting the same counterpart. Does that split work for you?
| case 'subIssue': { | ||
| const [found] = await control.findAll( | ||
| control.ctx, | ||
| tracker.class.Issue, | ||
| { | ||
| attachedTo: source._id, | ||
| attachedToClass: tracker.class.Issue, | ||
| space: rule.linkedProject | ||
| }, | ||
| { limit: 1 } | ||
| ) | ||
| return found | ||
| } |
There was a problem hiding this comment.
[P2] Refuse an ambiguous child counterpart
A parent can have multiple children in the linked project. limit: 1 silently selects whichever child the database returns first, so reversing query order changes the issue that is updated. Please require a unique match (or add an explicit pairing identifier). This replacement also includes the documented subIssues collection constraint. It skips ambiguous matches; any future auto-create implementation must distinguish ambiguity from absence rather than creating another child.
| case 'subIssue': { | |
| const [found] = await control.findAll( | |
| control.ctx, | |
| tracker.class.Issue, | |
| { | |
| attachedTo: source._id, | |
| attachedToClass: tracker.class.Issue, | |
| space: rule.linkedProject | |
| }, | |
| { limit: 1 } | |
| ) | |
| return found | |
| } | |
| case 'subIssue': { | |
| const found = await control.findAll( | |
| control.ctx, | |
| tracker.class.Issue, | |
| { | |
| attachedTo: source._id, | |
| attachedToClass: tracker.class.Issue, | |
| collection: 'subIssues', | |
| space: rule.linkedProject | |
| }, | |
| { limit: 2 } | |
| ) | |
| if (found.length !== 1) return undefined | |
| return found[0] | |
| } |
There was a problem hiding this comment.
Done in d3a865a. subIssue resolver now queries with limit 2 and requires a unique match. Also added collection: 'subIssues' to the filter so the shape matches the documented link. If a future auto-create implementation needs to pick, it will have to distinguish ambiguity from absence as you flagged.
The earlier commit wired model-server-workflow-rules into the server plugin list but missed the matching client-side @hcengineering/model-workflow-rules entry. Without it the TWorkflowRule class is never built into the workspace model, so client findAll on tracker.class.WorkflowRule throws "domain not found: workflow-rules:class:WorkflowRule" and the trigger has nothing to look up. Caught by running the stack locally end-to-end and querying the loaded hierarchy.
…, strict guards Addresses the inline review on PR Platform-Collective#238: - Skip derived writes (TxUpdateDoc.space === core.space.DerivedTx) so a mirror update cannot itself trigger another mirror. One hop semantics, independent of status equality. The equality short-circuit alone did not catch non-inverse mapping pairs across two linked projects. - Validate a persisted WorkflowRule doc on read (shape, linkKind, forwardMap entries, allowedLoops entries, autoCreate.triggerStatuses). A malformed rule is skipped with a warn log; the batch keeps going for the other rules on the same source. - Resolve forwardMap[newStatus] via Object.prototype.hasOwnProperty.call and require a non-empty string value. Avoids prototype chain hits and non-string stored values resolving to junk targets. - Make checkRegressionGuard fail closed: a missing TaskType, a status that is not a member of the target TaskType's statuses, or malformed allowedLoops now blocks the move. An allowedLoops entry can never authorize a status that does not exist in the target. - Match isRegression to the new fail-closed contract. Unknown statuses now return true (block). The jsdoc explains the trigger validates TaskType membership upstream, so the helper is defense in depth. - Require a unique child in the subIssue counterpart resolver. If a parent has two or more children in the linked project, return undefined (treat as absence) rather than letting the database pick one at its own ordering. Tests updated for the fail-closed isRegression behavior. All 9 tests pass locally; rush validate for the resources package passes. Cross-project authorization and the batch pending-status map are being tracked as a follow-up PR because they need new server policy and multi-rule conflict ordering, which deserve their own review surface.
|
Thanks for the review, this was really useful. Pushed d3a865a that addresses six of the nine findings:
The two P1 model wiring findings landed in f4bd93a earlier this evening while I was running the stack locally, so those are already in. Please take another look when you get time. Two findings I'd like to split into a follow up PR:
Does that split work for you? |
MichaelUray
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I went through d3a865a and f4bd93a against my nine comments:
| # | Comment | Status |
|---|---|---|
| 1, 2 | Import and execute the rule model | Resolved (models/all/src/index.ts:74, :325, :565) |
| 4 | Bound mirror cascades | Resolved: trigger output is DerivedTx and skipped (index.ts:44); a test with non-inverse/non-convergent mappings is still missing |
| 5 | Statuses outside the target TaskType | Resolved in the trigger (index.ts:239-248); the exported isRegression still checks allowedLoops before rank membership (logic.ts:47-50) |
| 6 | Validate persisted rule JSON | Read-side validation done (index.ts:136-157); write-side validation deferred, fine for now |
| 7 | Own, valid status-map entry | Resolved in the trigger (index.ts:93-95); the unused applyRuleToUpdate (logic.ts:24) still has the old lookup and is what the tests exercise |
| 9 | Ambiguous child counterpart | Resolved (index.ts:179-190) |
| 3 | Rule management and target authorization | Deferred, see below |
| 8 | Effective pending status | Deferred, see below |
Blocking
- The authorization gap is live by default, so it can't wait for a follow-up (comment 3). The plugin is registered with
enabled: trueand the server trigger has no feature gate, so it runs in every workspace.WorkflowRulehas no permission of its own, so anyone who can write to project A can create a rule there and then drive status changes on the linked issue in project B; the derived update enters the pipeline after the space/role checks (server/server-pipeline/src/pipeline.ts:154-158,derivedEntry.ts:30), so B's role permissions never apply, and withallowedLoopsthis can also force backward moves. Restricting only who creates rules is not enough, because an ordinary source editor still activates the write. Either add a minimal gate in this PR (rule management limited to owners/maintainers and a check at execution time that the acting user may write the target, or explicit target-side approval), or ship the feature disabled behind a switch the server trigger actually checks. - One invocation can still apply an unapproved regression (comment 8). The regression check uses the persisted target status for every queued update (
index.ts:96-98, unconditional update at:113). With target ranks[Backlog, InProgress, Done], the target at Backlog and no loop exceptions, two rules mapping the same source event to Done and then InProgress both pass against Backlog, and applying them regresses Done → InProgress. One rule with two source transactions in the same request (e.g. a bulk status change on two children of one parent) is enough as well. Derived transactions are applied sorted bymodifiedOn(foundations/server/packages/middleware/src/triggers.ts:268), so a pending-status map has to follow that order.
Should fix (from f4bd93a)
- The "Workflow Rules" toggle in Settings › Configure does not stop mirroring. It only filters UI contributions; the rule class and the server trigger (registered under
server-workflow-rules,models/all/src/index.ts:565) keep running, and execution only checks each rule's ownenabled(index.ts:62). - The configuration card uses raw strings as label/description and
icon: undefined(models/all/src/index.ts:329-332). Those are not valid IntlString ids, so the Configure page reports a translation error and shows the literal text. UseworkflowRules.string.WorkflowRules, a proper description key and the existing icon.
Tests and docs
- The new trigger behaviour has no tests: the derived-write skip,
isValidRule, the own-property lookup, missing TaskType, ambiguous children, invalid status plus matching loop exception, reciprocal non-convergent rules, multiple rules/source transactions, and unauthorized activation. Three of the nine tests exerciseapplyRuleToUpdate, which production never calls; I'd either share a hardened helper with the trigger or drop it. docs/workflow-rules/DESIGN.md:129-133and:158still describe aTxApplyIfscope token, and the PR body still credits the equality check for echo prevention.
…ig, tests
Addresses the remaining blockers and polish from the second review pass.
Authorization at execution (comment 3)
The derived mirror update bypasses the ordinary space permission
middleware, so the trigger now stands in for it. Before emitting a
write it resolves the acting account from
control.ctx.contextData.socialStringsToUsers.get(tx.modifiedBy) and
requires one of:
- system account,
- AccountRole.Admin,
- member or owner of the target Space.
Source-project membership alone is not sufficient. Results are
memoized per target space for one invocation. An unresolvable acting
account fails closed.
Batch pending-status map (comment 8)
Transactions in one invocation are now sorted by modifiedOn, matching
the derived-tx sort in triggers.ts. A pendingStatuses map retains the
queued target status per counterpart across the whole batch, and
equality + regression checks run against the effective pending status
instead of the stale persisted one. Two rules mapping the same source
event to different targets, or two source transactions on children of
one parent, can no longer slip a forbidden regression past the guard.
checkRegressionGuard now takes an explicit effectiveFrom.
Configure toggle actually stops mirroring
Settings > Configure writes a PluginConfiguration row for the plugin.
The trigger now reads it via control.modelDb.findAllSync on every
invocation and returns [] when the row says enabled: false. Missing
or unreadable config is treated as enabled, matching the default
registration in models/all.
Configure card label/description/icon
models/all now binds workflowRules.string.ConfigLabel +
ConfigDescription IntlStrings and workflowRules.icon.WorkflowRule,
replacing the raw strings and 'icon: undefined as any' from the
previous pass. en.json gains the two new strings.
Tests + helper cleanup
- Dropped applyRuleToUpdate. The trigger does the hardened lookup
inline (own-property guard, non-empty string check). The tests that
exercised it are removed; the file no longer re-exports it.
- New trigger.spec.ts with 17 cases covering: mirror emission,
DerivedTx skip, isValidRule shape enforcement, prototype pollution
guard on forwardMap, missing TaskType as absence, ambiguous
subIssue counterpart as absence, allowedLoops entry allowing a
specific backward move, batch pending-status map blocking a
second-rule regression against a queued target, target-side
authorization refusal for a non-member, admin bypass, and the
plugin kill switch.
- logic.spec.ts trimmed to the isRegression cases.
Docs
docs/workflow-rules/DESIGN.md: dropped the TxApplyIf cascade-token
description, replaced with the DerivedTx-space check we actually
ship. Added authorization and batch pending-status steps so the file
matches the code.
|
Thanks for the second pass, especially the authorization walk-through on the derived-tx pipeline. You were right that the gap could not wait for a follow-up. Pushed ba6b344 with the two blockers and the polish: Blockers
Polish
Tests + helper (your point 5)
Docs (your point 6)
The PR body was from the first iteration and I will update it to credit the DerivedTx skip, not equality, for echo prevention. Please take another look when you have time. |
MichaelUray
left a comment
There was a problem hiding this comment.
Thanks, this round is a big step forward. I checked ba6b344 against my previous points:
| Point | Status |
|---|---|
| Target authorization when a rule fires | Works for members, owners and Admins; regular users cannot spoof modifiedBy (the identity middleware binds it before triggers run). The system-account branch is unreachable, see 2. |
| Batch ordering / pending status | Fixed within one trigger call (sorted by modifiedOn, regression check against the effective pending status), see 3 for the remaining gap |
| Configure toggle | Fixed: isPluginEnabled reads the PluginConfiguration on every invocation |
| IntlStrings / icon | Fixed |
| Tests | The new trigger.spec.ts calls the real trigger, good. Missing: two source changes in one batch (the "batches by modifiedOn" test passes a single transaction), an unresolvable actor, a system-authored change, a backward move without a loop exception |
| Docs | The cascade wording is fixed; see 5 |
Still blocking
- Rule management is still unrestricted while the feature is on by default (
models/workflow-rules/src/index.ts:74-76,models/all/src/index.ts:331). There is no permission forWorkflowRule, so anyone who can create documents in project A can create an enabled rule pointing at B, with anyallowedLoops. The execution check now stops that person's own status changes from mirroring, but the rule still fires when a member of both projects or an Admin moves the linked issue, and then writes into B under their name, including backward moves the rule author chose. Since the trigger checks the acting user and never the rule's author, I'd still ask for one of: ship it off by default (enabled: false, so an Owner opts in via Configure), or gate rule creation/editing (e.g. a dedicated permission or owner/maintainer only, plus a check that the author can write the target project).
Should fix
- The system-account allowance can never match (
server-plugins/workflow-rules-resources/src/index.ts:243-251). The helper first needs an entry insocialStringsToUsers, but the session map builder skips system-account sessions (foundations/server/packages/server/src/sessionManager.ts:1360-1364), so system-authored status changes (integrations) never mirror and only log a warning. Either resolve the system account explicitly or drop the branch and document it. - The pending-status map only spans one trigger call.
Triggers.applyTriggergroups bymodifiedByand calls the trigger once per group before applying the writes (foundations/server/packages/core/src/triggers.ts:96,113-118), so a request with changes under two social IDs (e.g. a service acting for two users) can still queue Done and then a backward move against the same persisted status. Rare for normal clients, but worth a note or a guard; relatedly, the authorization cache keyed by space alone is only safe because of that grouping, keying it by actor + space would make it independent. - Membership is not quite the platform's write rule for the target: it is stricter for public projects and looser for restricted spaces or roles with forbid permissions, and archived target projects are not checked. Fine as a first step, but worth stating.
- Docs/PR body: DESIGN.md still describes auto-create emitting a create (
:145-149, the code only logs), lists the system account in the authorization step, and describes a Project Settings editor / "only members can create rules" (:182-192) that don't match the code. The PR body still credits the equality check for echo prevention.
Nits
isRegressionstill checks the loop exceptions before checking that both statuses exist, so its "fails closed" comment is not accurate (production is safe because the trigger validates first).- A few lines exceed 120 columns (
trigger.spec.ts:200,254,283,301,models/all/src/index.ts:73).
Adds cross-project workflow rules. An admin declares, on a project, that a
status change on an Issue should mirror onto a linked counterpart Issue in
another project. The mirror runs server-side so neither client needs to
poll, there is no outbound dependency on an external worker, and the rule
lives in the same backup as the project.
This retires the "polling daemon + long-lived API token" pattern that
operators run today for exactly this handoff case; see the discussion for
the write-up and a reference implementation.
Shape of a rule:
space: project the rule attaches to.linkedProject: project the mirror lands in.linkKind:subIssueorparentIssue, resolver dispatch for the counterpart.forwardMap:Record<Ref<IssueStatus>, Ref<IssueStatus>>of source -> target status.allowedLoops: pairs that may move backward on the target against TaskType ordering.autoCreate(optional): trigger statuses, source-component to target-componentmapping, default component, inheritable field whitelist.
enabled: soft-off switch, row stays for audit.Server trigger (
OnWorkflowRuleMirror, registered againstTxUpdateDoc/tracker.class.Issue):operations.status === undefinedso the hot path stays cheap.tx.spaceiscore.space.DerivedTx.The trigger's own writes are emitted via
control.txFactory, which marksderived updates with that space, so a mirror update cannot start another
mirror. For v1 this is the deliberate one-hop semantics; it also means
status writes emitted by other server triggers are not mirrored. Multi-hop
cascades would want a cascade-scoped visited set and a hard depth bound
instead.
JSON (
isValidRule), resolves the counterpart vialinkKindwith anexplicit
collection: 'subIssues'constraint andlimit: 2to refuseambiguous child matches, consults
forwardMapwith an own-property +non-empty-string guard, and emits a
TxUpdateDoc<Issue>for thecounterpart.
TaskType.statusesas the authoritative ranklist, so reorder/rename of a status re-ranks for free. The guard fails
closed when the TaskType is missing, when either status is not a member
of the rank table, or when
allowedLoopsis malformed.allowedLoopsare a per-rule allow-list of specific backward transitions.
ordinary space permission middleware, so the trigger stands in for it:
the acting account (resolved from
tx.modifiedByviasocialStringsToUsers) must be the system account, anAccountRole.Admin, or a member or owner of the target Space.Source-project membership alone is not sufficient.
in
modifiedOnorder, matching the derived-tx sort downstream intriggers.ts. A pending-status map retains the queued target statusper counterpart across the whole batch, and equality + regression
checks run against the effective pending status instead of the stale
persisted one. Two rules or two source transactions targeting one
counterpart in one request cannot slip a forbidden regression past the
guard.
PluginConfigurationrow for this pluginhas
enabled: false, the trigger returns[]without doing any work.Checked on every invocation via
control.modelDb.findAllSync, so theSettings > Configure toggle takes effect without a server restart.
TxCreateDoc<Issue>path (sequence allocation, rank, parent/sub-issuecount backfill) deserves its own review surface and will follow in a
separate PR on top of this one.
A matrix editor UI is in #244 as a dependent PR, stacked on this branch.
New packages
plugins/workflow-rules- plugin descriptor +WorkflowRuleinterface + related types.plugins/workflow-rules-assets- lang/en.json, icon, string loader.models/workflow-rules-TWorkflowRuleclass +DOMAIN_WORKFLOW_RULES.server-plugins/workflow-rules- trigger descriptor.server-plugins/workflow-rules-resources- trigger body + pure helpers + unit tests.models/server-workflow-rules- trigger registration viacreateDoc(serverCore.class.Trigger, ...).Changed files
docs/workflow-rules/DESIGN.md(new)rush.json(register six new projects)models/all/{src/index.ts,package.json}(registerworkflowRulesModel+serverWorkflowRulesModel, bind proper IntlStrings and icon)server/server-pipeline/{src/serverPlugins.ts,src/internationalization.ts,package.json}(
addLocationfor resources module + English strings loader)dev/prod/{src/platform.ts,package.json}(webpack bundle picks up assets package)Testing
server-plugins/workflow-rules-resources/src/__tests__/logic.spec.ts:6 cases on
isRegression(forward, blocked backward, allowedLoops entrymatch, allowedLoops direction strictness, same-rank no-op, fail-closed
on unknown status).
server-plugins/workflow-rules-resources/src/__tests__/trigger.spec.ts:17 cases on
OnWorkflowRuleMirrorandisValidRulecovering mirroremission,
DerivedTxskip, shape enforcement, prototype pollutionguard on
forwardMap, missing TaskType as absence, ambiguoussubIssuecounterpart as absence,allowedLoopsentry allowing aspecific backward move, batch pending-status map blocking a
second-rule regression against a queued target, target-side
authorization refusal for a non-member, admin bypass, and the plugin
kill switch.
rush check,rush build,rush validate,rush bundle,rush test. UI (Playwright) tests and thedocker build were not run locally.
self-hosted instance for roughly a month; this PR is the server-side
replacement I would retire that daemon for.
Reviewed independently before submission.