Skip to content

feat: wire pilot guardrails into agent runtime + close approval bypass#4

Open
mtmtian wants to merge 4 commits into
oratis:mainfrom
mtmtian:feat/pilot-guardrails-runtime
Open

feat: wire pilot guardrails into agent runtime + close approval bypass#4
mtmtian wants to merge 4 commits into
oratis:mainfrom
mtmtian:feat/pilot-guardrails-runtime

Conversation

@mtmtian

@mtmtian mtmtian commented Jul 7, 2026

Copy link
Copy Markdown

Summary

Wires the four P21 pilot disciplines (docs/growth/00 §7.3) into the agent's actual runtime — they previously existed only as pure functions in src/lib/growth/{pilot-gates,budget-guard}.ts with zero act-time enforcement — and closes the guardrail bypass in executeApprovedDecision.

1. Three new guardrail evaluators (guardrails.ts + schemas)

  • pilot_budget_cap (fail-closed) — inert until an org sets pilotStartDate in the rule config, so existing orgs are unaffected. Sums Report.spend since pilot start; ≥95% of cap blocks spend-increasing tools, ≥80% warns.
  • skan_maturity (fail-closed) — meta/tiktok app_install campaigns: <72h hard-reject automated adjustments; learning phase (≤7d) warns only, unless daily spend >200% of budget. Missing startDate/budget data rejects.
  • tier_cac_ceiling (fail-open to avoid cold-start lockout) — blocks bid/budget increases when the channel's latest CohortSnapshot.cac exceeds tierCacCeiling(firstMonthNet); decreases always pass.

2. Approval-path bypass closed (act.ts)

executeApprovedDecision now re-runs evaluateGuardrails against fresh state immediately before each tool.execute — approvals can sit pending for hours, so the stored report can't be trusted. All five callers (approvals, bulk approvals, rollback, bulk rollback, Slack interactive) inherit from this single choke point. Rollbacks (detected via existing DecisionStep.rollbackOf) are exempt from the budget cap and CAC ceiling so a bad state can't get stranded; SKAN warnings surface but don't block.

3. Growth snapshot now reaches the plan LLM

perceive() computed funnel/gate/budget signals but plan.v1.md had no growth placeholder — the data was dropped before prompting. Added a GROWTH_JSON block (after the prompt-cache marker, with a regression test pinning that position) plus explicit rules: gate=kill → reduce/stop only; budget non-ok → no spend-increasing actions.

4. Docs: MMP ingestion prerequisites

docs/growth/06-mmp-ingest.md — why plugging Adjust/AppsFlyer into ConversionEvent next to GA4 double-counts installs (idempotency key includes source; cohort aggregation is source-blind), splits channels, and zeroes retention/LTV; plus an executable S2S-callback integration checklist. Pointer comments on the legacy Report-only clients.

Test plan

  • npx vitest run — 275/275 across 27 files (new: evaluator boundaries at 95%/72h/7d/ceiling, fail-closed paths, decrease pass-through; act.test.ts covers block-at-execution, rollback exemptions, and a control case where rollback still blocks on unrelated rules)
  • npx tsc --noEmit clean; npm run lint 0 errors (35 pre-existing warnings)

🤖 Generated with Claude Code

mtmtian and others added 3 commits July 7, 2026 16:35
WHY: docs/growth/00 §7.3 disciplines existed only as pure functions in
src/lib/growth/{pilot-gates,budget-guard}.ts — zero act-time enforcement.
A $5K pilot could overspend and SKAN-immature campaigns could be
auto-adjusted; executeApprovedDecision also skipped all guardrails.

WHAT: register pilot_budget_cap (fail-closed, activates via pilotStartDate
config), skan_maturity (fail-closed, <72h reject / learning-phase warn),
tier_cac_ceiling (fail-open, increase-only) in the evaluator registry +
schemas; re-evaluate guardrails at execution time inside
executeApprovedDecision (all five caller routes inherit), with rollback
exemption via existing DecisionStep.rollbackOf. Tests for boundaries,
fail-closed paths, and the bypass fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY: perceive() attached GrowthSnapshot to the snapshot but plan.v1.md had
no growth placeholder — funnel/gate/budget signals were computed then
dropped before reaching the LLM.

WHAT: add GROWTH_JSON block to plan.v1.md (after the cache-control marker)
with explicit rules — gate=kill means reduce/stop only, budget non-ok
forbids any spend-increasing action; render it in plan.ts; regression test
pins the placeholder position relative to the cache split.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY: wiring an MMP into ConversionEvent alongside GA4 double-counts
installs (source is part of the idempotency key, cohort aggregation is
source-blind), splits channels (network names all fall through to
organic), and zeroes retention/LTV (adid vs pseudo_id namespaces).

WHAT: docs/growth/06-mmp-ingest.md — the three holes, two canon decisions
to make first, and an executable S2S-callback integration checklist;
pointer comments on the legacy Report-only clients.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mtmtian added a commit to mtmtian/adex that referenced this pull request Jul 7, 2026
WHY: 04-status.md is the build-status handoff doc but stopped at the
2026-07-04 upstream snapshot (PR oratis#1oratis#3); anyone reading it would miss
the guardrail wiring, Adjust ingest, BI dashboard, and campaign-name
canon now in review.

WHAT: append the PR oratis#4oratis#7 table with scopes and ops prerequisites;
normalize the two pre-existing table separators to the padded style the
linter enforces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@oratis

oratis commented Jul 8, 2026

Copy link
Copy Markdown
Owner

🤖 Code review — Claude Code

Verdict: Request changes — the core approval-bypass fix is correctly implemented, but a High double-counting bug breaks the arithmetic of this PR's headline guardrail.

Scope note: This is the base of the stacked series #4#5#6#7 (all target main). Its diff is its own; it should merge first.

Summary

Closes a real approval-bypass: executeApprovedDecision (src/lib/agent/act.ts) previously executed a Decision's steps using only the guardrail report computed once at proposal time, so a stale approval sitting in the queue could execute against outdated state. This PR adds a fresh evaluateGuardrails() re-check immediately before each step's tool.execute() — applied to all five callers, which funnel through this one function — plus three new pilot evaluators (pilot_budget_cap, skan_maturity, tier_cac_ceiling) with a narrowly-scoped rollback exemption.

Findings

  • [High] src/lib/agent/guardrails.ts:339-348pilot_budget_cap double-counts spend. It sums Report.spend with no level filter (report.findMany({ where: { orgId, date: { gte: since } }, select: { spend: true } })). For adaptable platforms (google/meta/tiktok), sync writes both an account-level row (writeAccountReport, level:'account') and per-campaign rows (writeCampaignReports, level:'campaign') for the same spend (src/lib/sync/report-writer.ts). Summing all rows roughly doubles real spend — so the $5K pilot cap warns/auto-pauses at ~half (or less) of actual spend. The correct pattern already exists one function above: pause_only_with_conversions (guardrails.ts:238) scopes with level:'campaign'. This fails restrictive (not a security bypass) but breaks the headline feature's math. → Filter by level the same way pause_only_with_conversions does.

  • [Medium] src/lib/agent/act.ts:217-276fresh guardrail result is discarded on the success path. The re-check (freshResults, act.ts:217) is persisted to DecisionStep.guardrailReport only on the blocked branch (act.ts:242). When a step passes and executes, the success-path decisionStep.update (act.ts:265-276) writes status/toolOutput/executedAt but not guardrailReport — so the row keeps only the stale proposal-time report, and any fresh warn-level signal (skan_maturity "learning phase", pilot_budget_cap 80%+ warn) is silently dropped. This contradicts the inline comment at act.ts:225-231 claiming the warning "is kept in the report." Audit-trail gap, not a security issue. → Add guardrailReport: JSON.stringify(freshResults) to the success-path update.

Core fix verified sound: traced every caller of executeApprovedDecision (approvals/[id], approvals/bulk, decisions/[id]/rollback, decisions/bulk-rollback, slack/interactive) — tool.execute() is reached from exactly one place, inside this function, so no path skips the fresh re-check. Multi-tenant scoping checks out (orgId flows through decision.findFirst({ where:{ id, orgId } }); the new evaluators' campaignId-only queries are safe because campaignId is validated against ctx.orgId earlier in the same evaluator). logAudit() correctly called on the new blocked_at_execution path. No schema.prisma change → no migration needed (correctly absent).

Test coverage

Adequate for the core logic — act.test.ts / guardrails.test.ts (Vitest) cover fresh-recheck block-vs-pass, the rollback exemption (incl. "still blocks on an unrelated rule"), and each new guardrail's pass/warn/block/fail-closed branches. Gap: no Playwright e2e exercises the bypass end-to-end (approve → mutate state so a guardrail now fails → hit /api/agent/approvals/[id] → confirm blocked) — worth adding given this is a security-relevant behavior change.

Aside: the code ships Vitest unit tests and they're clearly established here, but AGENTS.md still says "No unit tests yet — only Playwright e2e." Worth updating that doc.

Strengths

  • The re-check sits at the one truly shared choke point, verified by tracing all five callers rather than assuming — closes the bypass for real.
  • Rollback exemption is scoped to three named rules (not a blanket bypass) and justified by the tool.inverse() contract.
  • Scenario-rich unit tests for both the new guardrails and the re-check/rollback behavior.

Independent automated review. Findings are advisory — use judgment.

…success

WHY (review oratis#4): sync writes both an account-level row and per-campaign
rows for the same spend, so the unfiltered sum double-counted and
tripped the $5K cap at ~half real spend. The execution-time guardrail
re-check was also discarded on the success path, silently dropping
warn-level signals from the audit trail.

WHAT: pilot_budget_cap filters level:'account' (the one-per-platform-
per-day contract row); success-path decisionStep.update persists
guardrailReport. Regression tests for both.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mtmtian

mtmtian commented Jul 9, 2026

Copy link
Copy Markdown
Author

✅ Changes applied (fixup pushed)

Both review findings addressed:

  • [High] pilot_budget_cap double-count — the report.findMany now filters level: 'account', matching the pause_only_with_conversions pattern one function above. Account-level rows are the one-per-platform-per-day contract row; the per-campaign rows that duplicated the same spend are no longer summed, so the $5K cap trips at real spend rather than ~half. Added a regression test asserting the level: 'account' filter is present in the query.
  • [Medium] fresh report discarded on success path — the success-path decisionStep.update now writes guardrailReport: JSON.stringify(freshResults), so warn-level signals (learning-phase, 80%-of-cap) survive in the audit trail instead of leaving only the stale proposal-time report. Regression test added.

Commit f14b638. Full stack re-verified: 389/389 vitest, tsc clean, build passes.

Re the aside on AGENTS.md still claiming "no unit tests" — noted; that doc predates the Growth OS vitest suite. Leaving it for a separate docs pass rather than widening this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants