Skip to content

Commit 3143ebb

Browse files
author
bgagent
committed
fix(event-governance): address review findings
- rethrow retryable infra errors (throttle/5xx) from task load and high-water update so the stream retries instead of silently under-counting a cost/turn ceiling - seed the monotonic cost mark from authoritative task.cost_usd so a ceiling can trip on a non-cost event and the first eval of an already-costed task - normalize aggregate metadata aliases on both evaluators; drop the duplicate cumulative_* / turn_count aliases the progress writer emitted - default gate_on_event_async ts_module to the real task_state so a missing module pauses for approval instead of crashing - key get-event-rules cache on repo + workflow_ref (rules depend on the workflow's eventRulePack) - add coverage for the new retry/seed paths
1 parent 607fb61 commit 3143ebb

11 files changed

Lines changed: 335 additions & 69 deletions

File tree

agent/src/event_governance/evaluator.py

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,31 @@ def _fields_match(rule_when: dict[str, Any], metadata: dict[str, Any]) -> bool:
4848
return True
4949

5050

51+
# Canonical aggregate metadata fields ↔ accepted aliases (base field a producer
52+
# emits ↔ cumulative field the rule reads). Parity with the CDK evaluator's
53+
# AGGREGATE_FIELDS: the evaluator normalizes so producers emit ONE name. See #230.
54+
_AGGREGATE_ALIASES = {
55+
"cost": ("cumulative_cost_usd", "cost_usd"),
56+
"turns": ("turn_count", "turn"),
57+
}
58+
59+
60+
def _read_aggregate(
61+
metadata: dict[str, Any],
62+
aliases: tuple[str, ...],
63+
) -> float | None:
64+
"""First value among ``aliases`` that coerces to a finite float."""
65+
for key in aliases:
66+
raw = metadata.get(key)
67+
if raw is None:
68+
continue
69+
try:
70+
return float(raw)
71+
except (TypeError, ValueError):
72+
continue
73+
return None
74+
75+
5176
def _aggregate_match(
5277
rule_when: dict[str, Any],
5378
metadata: dict[str, Any],
@@ -62,7 +87,7 @@ def _aggregate_match(
6287
if cost_gte is not None:
6388
cumulative = aggregate_state.get("cumulative_cost_usd") if aggregate_state else None
6489
if cumulative is None:
65-
cumulative = metadata.get("cumulative_cost_usd")
90+
cumulative = _read_aggregate(metadata, _AGGREGATE_ALIASES["cost"])
6691
if cumulative is None:
6792
return False
6893
try:
@@ -74,7 +99,7 @@ def _aggregate_match(
7499
if turn_gte is not None:
75100
turns = aggregate_state.get("turn_count") if aggregate_state else None
76101
if turns is None:
77-
turns = metadata.get("turn_count")
102+
turns = _read_aggregate(metadata, _AGGREGATE_ALIASES["turns"])
78103
if turns is None:
79104
return False
80105
try:

agent/src/event_governance/gate.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
if TYPE_CHECKING:
99
from event_governance.evaluator import EventRule
1010

11+
import task_state
1112
from hooks import _handle_require_approval
1213
from policy import PolicyDecision
1314

@@ -32,6 +33,11 @@ async def gate_on_event_async(
3233
ts_module: Any = None,
3334
) -> EventGateResult:
3435
"""Block on human approval for an event rule (enforce + require_approval)."""
36+
# Default to the real task_state module. _handle_require_approval dereferences
37+
# ``ts`` unconditionally (increment_approval_gate_count_in_ddb,
38+
# transact_write_approval_request); a None here crashes the gate instead of
39+
# pausing for approval. The parameter stays overridable for tests. See #230.
40+
ts_module = ts_module if ts_module is not None else task_state
3541
checkpoint = metadata.get("checkpoint") or rule.on
3642
tool_input = {"checkpoint": checkpoint, "event_type": event_type, **metadata}
3743
decision = PolicyDecision.require_approval(

agent/src/progress_writer.py

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -612,10 +612,6 @@ def write_agent_turn(
612612
"agent_turn",
613613
{
614614
"turn": turn,
615-
# Cumulative alias read by event-governance aggregate rules
616-
# (turn_count_gte). ``turn`` is the SDK's running turn index, so
617-
# it is already the per-session cumulative count. See #230.
618-
"turn_count": turn,
619615
"model": model,
620616
"thinking_preview": self._preview(thinking),
621617
"text_preview": self._preview(text),
@@ -690,14 +686,9 @@ def write_agent_cost_update(
690686
"agent_cost_update",
691687
{
692688
"cost_usd": cost_usd,
693-
# Cumulative alias read by event-governance aggregate rules
694-
# (cost_usd_gte). SDK ``total_cost_usd`` is the running
695-
# session total, so it is already cumulative. See #230.
696-
"cumulative_cost_usd": cost_usd,
697689
"input_tokens": input_tokens,
698690
"output_tokens": output_tokens,
699691
"turn": turn,
700-
"turn_count": turn,
701692
},
702693
)
703694

cdk/src/handlers/fanout-task-events.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ import type {
4747
} from 'aws-lambda';
4848
import { clearTokenCache, resolveGitHubToken } from './shared/context-hydration';
4949
import { classifyError } from './shared/error-classifier';
50-
import { evaluateAsyncEventRules, loadTaskForGovernance } from './shared/event-governance-async';
50+
import { evaluateAsyncEventRules, isRetryableInfraError, loadTaskForGovernance } from './shared/event-governance-async';
5151
import { renderCommentBody, upsertTaskComment } from './shared/github-comment';
5252
import { postIssueComment } from './shared/linear-feedback';
5353
import { logger } from './shared/logger';
@@ -1284,6 +1284,11 @@ export const handler = async (
12841284
governanceForceFanOut = govResult.forceFanOut || governanceForcedChannels.length > 0;
12851285
}
12861286
} catch (govErr) {
1287+
// Infra errors (DDB throttle / 5xx) must retry the record — otherwise an
1288+
// enforce cancel_task / require_approval is silently skipped and the
1289+
// cost/turn high-water mark under-counts (#230). Benign failures degrade
1290+
// to fanout-only, as before, to preserve poison-pill isolation.
1291+
if (isRetryableInfraError(govErr)) throw govErr;
12871292
logger.warn('[fanout] event governance evaluation failed — continuing fanout', {
12881293
task_id: ev.task_id,
12891294
event_id: ev.event_id,

cdk/src/handlers/get-event-rules.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
2121
import { DynamoDBDocumentClient, UpdateCommand } from '@aws-sdk/lib-dynamodb';
2222
import type { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda';
2323
import { ulid } from 'ulid';
24-
import { listBuiltinEventRulePacks, resolveEventRules } from './shared/event-rule-pack-resolver';
24+
import { listBuiltinEventRulePacks, resolveEventRules, UnknownEventRulePackError } from './shared/event-rule-pack-resolver';
2525
import { extractUserId } from './shared/gateway';
2626
import { logger } from './shared/logger';
2727
import { formatMinuteBucket, RATE_LIMIT_ROW_TTL_SECONDS } from './shared/rate-limit';
@@ -107,7 +107,12 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
107107
}
108108
}
109109

110-
const cached = cache.get(repoId);
110+
// Cache key includes workflow_ref: resolved rules depend on the workflow's
111+
// eventRulePack, so keying on repo alone would serve one workflow's rules
112+
// for another within the TTL window (#230).
113+
const workflowRef = event.queryStringParameters?.workflow_ref;
114+
const cacheKey = `${repoId}\n${workflowRef ?? ''}`;
115+
const cached = cache.get(cacheKey);
111116
if (cached && cached.expiresAt > Date.now()) {
112117
return successResponse(200, cached.response, requestId);
113118
}
@@ -123,13 +128,20 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
123128
}
124129

125130
const repoConfig = await loadRepoConfig(repoId);
126-
const workflowRef = event.queryStringParameters?.workflow_ref;
127131
const workflow = workflowRef ? getWorkflowDescriptor(workflowRef) : undefined;
128132
const packRef = repoConfig?.event_rule_pack ?? workflow?.eventRulePack;
129-
const resolved = resolveEventRules({
130-
inlineRules: repoConfig?.event_rules,
131-
packRef,
132-
});
133+
let resolved: EventRule[];
134+
try {
135+
resolved = resolveEventRules({
136+
inlineRules: repoConfig?.event_rules,
137+
packRef,
138+
});
139+
} catch (err) {
140+
if (err instanceof UnknownEventRulePackError) {
141+
return errorResponse(422, ErrorCode.VALIDATION_ERROR, err.message, requestId);
142+
}
143+
throw err;
144+
}
133145

134146
const response: GetEventRulesResponse = {
135147
repo_id: repoId,
@@ -138,7 +150,7 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
138150
registry_packs: listBuiltinEventRulePacks(),
139151
};
140152

141-
cache.set(repoId, { response, expiresAt: Date.now() + CACHE_TTL_MS });
153+
cache.set(cacheKey, { response, expiresAt: Date.now() + CACHE_TTL_MS });
142154
return successResponse(200, response, requestId);
143155
} catch (err) {
144156
logger.error('get-event-rules failed', {

cdk/src/handlers/shared/create-task-core.ts

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import type { APIGatewayProxyResult } from 'aws-lambda';
3131
import { ulid } from 'ulid';
3232
import { isDegeneratePattern, parseApprovalScope } from './approval-scope';
3333
import { screenImage, screenTextFile, AttachmentScreeningError, type ScreeningConfig } from './attachment-screening';
34-
import { resolveEventRules } from './event-rule-pack-resolver';
34+
import { resolveEventRules, UnknownEventRulePackError } from './event-rule-pack-resolver';
3535
import { generateBranchName } from './gateway';
3636
import { estimateImageTokensFromBuffer } from './image-tokens';
3737
import { logger } from './logger';
@@ -96,6 +96,24 @@ function describeRequiredInputs(requiredInputs: { allOf?: readonly string[]; one
9696
return parts.length > 0 ? parts.join(', plus ') : 'a task specification';
9797
}
9898

99+
/** 503 for a blueprint/workflow that pins an unresolvable event-rule-pack —
100+
* a platform misconfiguration, same class as an out-of-bounds approval cap. */
101+
function unknownPackResponse(repo: string | undefined, err: UnknownEventRulePackError, requestId: string): APIGatewayProxyResult {
102+
logger.error('Blueprint misconfiguration — unknown event-rule-pack pin', {
103+
repo,
104+
pack_id: err.packRef.id,
105+
pack_version: err.packRef.version,
106+
request_id: requestId,
107+
});
108+
return errorResponse(
109+
503,
110+
ErrorCode.SERVICE_UNAVAILABLE,
111+
`Blueprint misconfiguration: event-rule-pack '${err.packRef.id}@${err.packRef.version}' `
112+
+ `for '${repo}' does not resolve. Ask the platform admin to re-deploy the blueprint with a valid pack pin.`,
113+
requestId,
114+
);
115+
}
116+
99117
/**
100118
* Core task creation logic shared by the Cognito create-task handler
101119
* and the webhook create-task handler.
@@ -212,19 +230,33 @@ export async function createTaskCore(
212230
resolvedApprovalGateCap = blueprintCap;
213231
capFromBlueprint = true;
214232
}
215-
resolvedEventRules = resolveEventRules({
216-
inlineRules: repoConfig?.event_rules,
217-
packRef: repoConfig?.event_rule_pack ?? workflow.eventRulePack,
218-
});
219233
const packRef = repoConfig?.event_rule_pack ?? workflow.eventRulePack;
234+
try {
235+
resolvedEventRules = resolveEventRules({
236+
inlineRules: repoConfig?.event_rules,
237+
packRef,
238+
});
239+
} catch (err) {
240+
if (err instanceof UnknownEventRulePackError) {
241+
return unknownPackResponse(body.repo, err, requestId);
242+
}
243+
throw err;
244+
}
220245
if (packRef) {
221246
eventRulePackId = packRef.id;
222247
eventRulePackVersion = packRef.version;
223248
}
224249
} else {
225-
resolvedEventRules = resolveEventRules({
226-
packRef: workflow.eventRulePack,
227-
});
250+
try {
251+
resolvedEventRules = resolveEventRules({
252+
packRef: workflow.eventRulePack,
253+
});
254+
} catch (err) {
255+
if (err instanceof UnknownEventRulePackError) {
256+
return unknownPackResponse(body.repo, err, requestId);
257+
}
258+
throw err;
259+
}
228260
if (workflow.eventRulePack) {
229261
eventRulePackId = workflow.eventRulePack.id;
230262
eventRulePackVersion = workflow.eventRulePack.version;

0 commit comments

Comments
 (0)