fix: address 3 critical PR review issues
1. threads.yaml race condition: reload threads index after agent subprocess completes before updating head pointer (cli-uwf/commands/thread.ts) 2. evaluateJsonata not awaited: jsonata evaluate() returns Promise for async expressions — now properly awaited (uwf-moderator/evaluate.ts) 3. resolveWorkflowHash dead code: function always returns a value, removed impossible null return type and dead null-check branches at call sites (cli-uwf/store.ts, commands/thread.ts, commands/workflow.ts)
This commit is contained in:
+3
-1
@@ -2,7 +2,9 @@
|
||||
"name": "@uncaged/workflow-monorepo",
|
||||
"private": true,
|
||||
"workspaces": [
|
||||
"packages/*"
|
||||
"packages/*",
|
||||
"../json-cas/packages/json-cas",
|
||||
"../json-cas/packages/json-cas-fs"
|
||||
],
|
||||
"scripts": {
|
||||
"build": "bunx tsc --build",
|
||||
|
||||
@@ -61,9 +61,6 @@ async function resolveWorkflowCasRef(
|
||||
): Promise<CasRef> {
|
||||
const registry = await loadWorkflowRegistry(storageRoot);
|
||||
const hash = resolveWorkflowHash(registry, workflowId);
|
||||
if (hash === null) {
|
||||
fail(`workflow not found: ${workflowId}`);
|
||||
}
|
||||
if (!isCasRef(hash)) {
|
||||
fail(`workflow not found: ${workflowId}`);
|
||||
}
|
||||
@@ -386,7 +383,7 @@ export async function cmdThreadStep(
|
||||
const workflow = loadWorkflowPayload(uwf, workflowHash);
|
||||
const context = buildModeratorContext(uwf, chain);
|
||||
|
||||
const nextResult = evaluate(workflow, context);
|
||||
const nextResult = await evaluate(workflow, context);
|
||||
if (!nextResult.ok) {
|
||||
fail(nextResult.error.message);
|
||||
}
|
||||
@@ -415,12 +412,14 @@ export async function cmdThreadStep(
|
||||
fail(`agent returned hash that is not a StepNode: ${newHead}`);
|
||||
}
|
||||
|
||||
index[threadId] = newHead;
|
||||
await saveThreadsIndex(storageRoot, index);
|
||||
// Reload threads index to avoid overwriting changes made by the agent subprocess
|
||||
const freshIndex = await loadThreadsIndex(storageRoot);
|
||||
freshIndex[threadId] = newHead;
|
||||
await saveThreadsIndex(storageRoot, freshIndex);
|
||||
|
||||
const chainAfter = walkChain(uwfAfter, newHead);
|
||||
const contextAfter = buildModeratorContext(uwfAfter, chainAfter);
|
||||
const afterResult = evaluate(workflow, contextAfter);
|
||||
const afterResult = await evaluate(workflow, contextAfter);
|
||||
if (!afterResult.ok) {
|
||||
fail(afterResult.error.message);
|
||||
}
|
||||
|
||||
@@ -132,9 +132,6 @@ export async function cmdWorkflowShow(
|
||||
const uwf = await createUwfStore(storageRoot);
|
||||
const registry = await loadWorkflowRegistry(storageRoot);
|
||||
const hash = resolveWorkflowHash(registry, id);
|
||||
if (hash === null) {
|
||||
fail(`workflow not found: ${id}`);
|
||||
}
|
||||
|
||||
const node = uwf.store.get(hash);
|
||||
if (node === null) {
|
||||
|
||||
@@ -100,11 +100,8 @@ export async function saveWorkflowRegistry(
|
||||
await writeFile(path, text, "utf8");
|
||||
}
|
||||
|
||||
export function resolveWorkflowHash(registry: WorkflowRegistry, id: string): CasRef | null {
|
||||
if (registry[id] !== undefined) {
|
||||
return registry[id];
|
||||
}
|
||||
return id;
|
||||
export function resolveWorkflowHash(registry: WorkflowRegistry, id: string): CasRef {
|
||||
return registry[id] !== undefined ? registry[id] : id;
|
||||
}
|
||||
|
||||
export function findRegistryName(registry: WorkflowRegistry, hash: Hash): string | null {
|
||||
|
||||
@@ -58,12 +58,12 @@ function makeContext(steps: ModeratorContext["steps"]): ModeratorContext {
|
||||
}
|
||||
|
||||
describe("evaluate", () => {
|
||||
test("$START → first role (fallback)", () => {
|
||||
const result = evaluate(solveIssueWorkflow, makeContext([]));
|
||||
test("$START → first role (fallback)", async () => {
|
||||
const result = await evaluate(solveIssueWorkflow, makeContext([]));
|
||||
expect(result).toEqual({ ok: true, value: "planner" });
|
||||
});
|
||||
|
||||
test("condition match (notApproved → developer)", () => {
|
||||
test("condition match (notApproved → developer)", async () => {
|
||||
const context = makeContext([
|
||||
{
|
||||
role: "reviewer",
|
||||
@@ -72,11 +72,11 @@ describe("evaluate", () => {
|
||||
agent: "uwf-hermes",
|
||||
},
|
||||
]);
|
||||
const result = evaluate(solveIssueWorkflow, context);
|
||||
const result = await evaluate(solveIssueWorkflow, context);
|
||||
expect(result).toEqual({ ok: true, value: "developer" });
|
||||
});
|
||||
|
||||
test("fallback when condition does not match → $END", () => {
|
||||
test("fallback when condition does not match → $END", async () => {
|
||||
const context = makeContext([
|
||||
{
|
||||
role: "reviewer",
|
||||
@@ -85,11 +85,11 @@ describe("evaluate", () => {
|
||||
agent: "uwf-hermes",
|
||||
},
|
||||
]);
|
||||
const result = evaluate(solveIssueWorkflow, context);
|
||||
const result = await evaluate(solveIssueWorkflow, context);
|
||||
expect(result).toEqual({ ok: true, value: "$END" });
|
||||
});
|
||||
|
||||
test("missing role in graph → error", () => {
|
||||
test("missing role in graph → error", async () => {
|
||||
const context = makeContext([
|
||||
{
|
||||
role: "unknown-role",
|
||||
@@ -98,14 +98,14 @@ describe("evaluate", () => {
|
||||
agent: "uwf-hermes",
|
||||
},
|
||||
]);
|
||||
const result = evaluate(solveIssueWorkflow, context);
|
||||
const result = await evaluate(solveIssueWorkflow, context);
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.error.message).toBe('no transitions defined for role "unknown-role"');
|
||||
}
|
||||
});
|
||||
|
||||
test("output expansion in context works with JSONata", () => {
|
||||
test("output expansion in context works with JSONata", async () => {
|
||||
const context = makeContext([
|
||||
{
|
||||
role: "planner",
|
||||
@@ -114,7 +114,7 @@ describe("evaluate", () => {
|
||||
agent: "uwf-hermes",
|
||||
},
|
||||
]);
|
||||
const result = evaluate(solveIssueWorkflow, context);
|
||||
const result = await evaluate(solveIssueWorkflow, context);
|
||||
expect(result).toEqual({ ok: true, value: "developer" });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -21,9 +21,9 @@ function isTruthy(value: unknown): boolean {
|
||||
return true;
|
||||
}
|
||||
|
||||
function evaluateJsonata(expression: string, context: ModeratorContext): Result<unknown, Error> {
|
||||
async function evaluateJsonata(expression: string, context: ModeratorContext): Promise<Result<unknown, Error>> {
|
||||
try {
|
||||
const result = jsonata(expression).evaluate(context);
|
||||
const result = await jsonata(expression).evaluate(context);
|
||||
return { ok: true, value: result };
|
||||
} catch (error) {
|
||||
return {
|
||||
@@ -40,10 +40,10 @@ function currentRole(context: ModeratorContext): string {
|
||||
return context.steps[context.steps.length - 1].role;
|
||||
}
|
||||
|
||||
export function evaluate(
|
||||
export async function evaluate(
|
||||
workflow: WorkflowPayload,
|
||||
context: ModeratorContext,
|
||||
): Result<string, Error> {
|
||||
): Promise<Result<string, Error>> {
|
||||
const role = currentRole(context);
|
||||
const transitions = workflow.graph[role];
|
||||
if (transitions === undefined) {
|
||||
@@ -66,7 +66,7 @@ export function evaluate(
|
||||
};
|
||||
}
|
||||
|
||||
const evalResult = evaluateJsonata(conditionDef.expression, context);
|
||||
const evalResult = await evaluateJsonata(conditionDef.expression, context);
|
||||
if (!evalResult.ok) {
|
||||
return evalResult;
|
||||
}
|
||||
|
||||
+6
-1
@@ -32,6 +32,11 @@
|
||||
{ "path": "packages/workflow-agent-react" },
|
||||
{ "path": "packages/cli-workflow" },
|
||||
{ "path": "packages/workflow-template-solve-issue" },
|
||||
{ "path": "packages/workflow-template-develop" }
|
||||
{ "path": "packages/workflow-template-develop" },
|
||||
{ "path": "packages/uwf-protocol" },
|
||||
{ "path": "packages/uwf-moderator" },
|
||||
{ "path": "packages/cli-uwf" },
|
||||
{ "path": "packages/uwf-agent-kit" },
|
||||
{ "path": "packages/uwf-agent-hermes" }
|
||||
]
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user