Skip to content

Safety fixes: parent-done guard, verification nullsafe, metadata inheritance, role leak #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
krandder wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Safety fixes: parent-done guard, verification nullsafe, metadata inheritance, role leak #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions core/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,18 +262,19 @@ function applyTaskCreated(state: SystemState, event: Extract<Event, { type: "Tas
}

function applyLeaseGranted(state: SystemState, event: Extract<Event, { type: "LeaseGranted" }>, task: Task): void {
const ts = event.ts ?? Date.now();
const t = deepCloneTask(task);
t.currentFenceToken = event.fenceToken;
t.leasedTo = event.agentId;
t.leaseExpiresAt = event.ts + event.leaseTimeout;
t.leaseExpiresAt = ts + event.leaseTimeout;
t.phase = event.phase;
t.condition = "active";
t.retryAfter = null;
t.waitState = null;
t.currentSessionId = event.agentContext.sessionId;
t.contextBudget = event.contextBudget;
t.attempts[event.phase].used += 1;
t.updatedAt = event.ts;
t.updatedAt = ts;
state.tasks[t.id] = t;
}

Expand Down
6 changes: 5 additions & 1 deletion core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ export interface ReviewPolicyMet extends BaseEvent {
// Completion verification
// ---------------------------------------------------------------------------

export type ArtifactKind = "journal" | "code" | "pr";
export type ArtifactKind = "journal" | "code" | "pr" | "file";

export interface ArtifactEvidence {
kind: ArtifactKind;
Expand All @@ -473,6 +473,8 @@ export interface ArtifactEvidence {
aheadCount?: number | null;
changedFiles?: string[];
prUrl?: string | null;
path?: string;
sizeBytes?: number;
}

export interface CompletionVerification {
Expand All @@ -490,13 +492,15 @@ export interface CompletionVerificationRecorded extends BaseEvent {
export interface TaskCompleted extends BaseEvent {
type: "TaskCompleted";
stateRef: StateRef;
source?: EventSource;
}

export interface TaskFailed extends BaseEvent {
type: "TaskFailed";
reason: "budget_exhausted" | "cost_exhausted" | "review_rejected";
phase: Phase;
summary: FailureSummary;
source?: EventSource;
}

export interface TaskExhausted extends BaseEvent {
Expand Down
98 changes: 92 additions & 6 deletions middle/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,12 +745,12 @@ function handleClaimTask(
if (task.phase === "review") {
const reviewer = task.metadata["reviewer"] as string | undefined;
if (reviewer && agentRole(reviewer) !== claimRole) {
return { status: 403, body: { error: "role_mismatch", message: `Task ${taskId} reviewer is "${reviewer}", not "${agentId}". Use --force to override.` } };
return { status: 403, body: { error: "role_mismatch", message: `Task ${taskId} is assigned to a different agent for review. Use --force to override.` } };
}
} else {
const assignee = task.metadata["assignee"] as string | undefined;
if (assignee && agentRole(assignee) !== claimRole) {
return { status: 403, body: { error: "role_mismatch", message: `Task ${taskId} assignee is "${assignee}", not "${agentId}". Use --force to override.` } };
return { status: 403, body: { error: "role_mismatch", message: `Task ${taskId} is assigned to a different agent. Use --force to override.` } };
}
}
}
Expand Down Expand Up @@ -1722,6 +1722,74 @@ function maybeCreatePr(config: Config, task: Task, core: Core): void {
}
}

function isZeroedStateRef(ref: StateRef): boolean {
return ref.commit === "0000000" && ref.parentCommit === "0000000";
}

/**
* Pre-flight checks before marking a task done.
* Layer 1: Reject if any children are still non-terminal.
* Layer 2: If completionRule === "and", all terminal children must be "done".
* Layer 3: If metadata.repo is set, require a non-zeroed stateRef.
*/
function verifyCompletion(
core: Core,
task: Task,
stateRef?: StateRef,
): RouteResult | null {
// (a) Task has metadata.repo but stateRef is missing or zeroed
if (task.metadata["repo"]) {
const ref = stateRef ?? task.stateRef;
if (!ref || isZeroedStateRef(ref)) {
return {
status: 422,
body: {
error: "missing_state_ref",
message:
"Task has metadata.repo but no valid stateRef was provided. " +
"Submit a stateRef with a real commit before marking done.",
},
};
}
}

// (b) Layer 1: Block if any children are still non-terminal
if (task.children.length > 0) {
const children = core.getChildren(task.id);
const pending = children.filter((c) => c.terminal === null);
if (pending.length > 0) {
return {
status: 422,
body: {
error: "children_pending",
message:
`Cannot mark done: ${pending.length} child(ren) still pending: ` +
pending.map((c) => c.id).join(", ") +
". Cancel or complete them first.",
},
};
}

// (c) Layer 2: completionRule="and" requires all terminal children to be "done"
if (task.completionRule === "and") {
const notDone = children.filter((c) => c.terminal !== "done");
if (notDone.length > 0) {
return {
status: 422,
body: {
error: "children_not_done",
message:
`Task has completionRule='and' but ${notDone.length} child(ren) are not done: ` +
notDone.map((c) => c.id).join(", "),
},
};
}
}
}

return null;
}

function applyDoneTransition(
core: Core,
config: Config,
Expand All @@ -1732,6 +1800,10 @@ function applyDoneTransition(
evidence?: string,
stateRef?: StateRef,
): RouteResult {
// Pre-flight completion checks (children guard, stateRef guard)
const completionErr = verifyCompletion(core, task, stateRef);
if (completionErr) return completionErr;

if (task.phase === "execution" && task.condition === "active" && task.reviewConfig === null) {
// Verify artifacts before allowing completion
const verification = verifyArtifacts(task, config);
Expand Down Expand Up @@ -2535,10 +2607,17 @@ function handleDecomposeCommit(
}
}

const parentPriority = (task.metadata["priority"] as string | undefined) ?? "medium";
const metadata: Record<string, unknown> = {};
// Inherit all parent metadata, then override with child-specific values
const metadata: Record<string, unknown> = { ...task.metadata };
// Remove parent-specific bookkeeping that shouldn't propagate
delete metadata["createdBy"]; delete metadata["createdAt"];
delete metadata["claimedAt"]; delete metadata["claimedBy"];
delete metadata["claimSessionId"]; delete metadata["claimSource"];
delete metadata["last_update"]; delete metadata["last_update_at"];
// Apply child overrides
if (child.assignee) metadata["assignee"] = child.assignee;
if (child.reviewer) metadata["reviewer"] = child.reviewer;
const parentPriority = (task.metadata["priority"] as string | undefined) ?? "medium";
const childPriority = ("priority" in child ? (child as { priority?: string }).priority : undefined) ?? "medium";
metadata["priority"] = maxPriority(parentPriority, childPriority);

Expand Down Expand Up @@ -2779,10 +2858,17 @@ function handleDecompose(
}
}

const parentPriority = (task.metadata["priority"] as string | undefined) ?? "medium";
const metadata: Record<string, unknown> = {};
// Inherit all parent metadata, then override with child-specific values
const metadata: Record<string, unknown> = { ...task.metadata };
// Remove parent-specific bookkeeping that shouldn't propagate
delete metadata["createdBy"]; delete metadata["createdAt"];
delete metadata["claimedAt"]; delete metadata["claimedBy"];
delete metadata["claimSessionId"]; delete metadata["claimSource"];
delete metadata["last_update"]; delete metadata["last_update_at"];
// Apply child overrides
if (child.assignee) metadata["assignee"] = child.assignee;
if (child.reviewer) metadata["reviewer"] = child.reviewer;
const parentPriority = (task.metadata["priority"] as string | undefined) ?? "medium";
const childPriority = ("priority" in child ? (child as { priority?: string }).priority : undefined) ?? "medium";
metadata["priority"] = maxPriority(parentPriority, childPriority);

Expand Down