Skip to content

feat(cost): add org and team monthly budgets with alerting - #775

Open
ayushtr-aws wants to merge 4 commits into
mainfrom
feat/471-org-team-budgets
Open

feat(cost): add org and team monthly budgets with alerting#775
ayushtr-aws wants to merge 4 commits into
mainfrom
feat/471-org-team-budgets

Conversation

@ayushtr-aws

Copy link
Copy Markdown
Contributor

Adds recurring monthly USD guardrails for Cognito users and groups, with admission enforcement, threshold alerts, operator controls, and read-only personal visibility.

Area

  • cdk — infrastructure, handlers, constructs
  • agent — Python runtime / Docker image
  • clibgagent client
  • docs — guides or design sources (docs/guides/, docs/design/)
  • tooling — root mise.toml, scripts, CI workflows

Related

Closes #471

Changes

  • Add recurring user and Cognito-group monthly budget configuration and UTC-month spend rollups.
  • Check personal and applicable team hard-stop budgets at the shared task-admission boundary, returning 429 BUDGET_EXCEEDED without interrupting running work.
  • Roll terminal task cost_usd into user/team scopes idempotently through the existing TaskTable stream reconciler.
  • Emit one-shot 80% and 100% CloudWatch threshold metrics and notify the shared operational SNS topic.
  • Add operator bgagent budget status|set commands and authenticated, read-only bgagent budget status --me personal visibility.
  • Document organization-group setup, onboarding overhead, enforcement behavior, estimated AWS operating cost, and the distinction from authoritative AWS billing.
  • Keep the change scoped to CDK, CLI, and documentation; no agent runtime or agent-registry changes are included.

Validation

  • CDK lint and TypeScript compilation
  • CDK: 203 suites, 4,117 tests passed
  • CLI lint and TypeScript compilation
  • CLI: 58 suites, 769 tests passed
  • Documentation Astro check, link check, and 78-page static build
  • Drift-prevention and whitespace checks
  • Deployed smoke test: personal status started at $0.00; a $0.0573 task stopped by its per-task limit and a $0.0270 completed task both rolled up, producing $0.08 monthly spend

mise was unavailable in the implementation shell, so the package-native equivalents were used.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@ayushtr-aws
ayushtr-aws requested review from a team as code owners August 21, 2026 18:24
@ayushtr-aws

Copy link
Copy Markdown
Contributor Author

Addressed the self-review in a41a06f.

  • B1 fixed: orchestration reconciliation and budget rollup are now attempted independently for every TaskTable stream record. A failure in either still requests an isolated retry, but cannot prevent the other subsystem from progressing.
  • N1 fixed: added coverage for a single rollup crossing both 80% and 100%.
  • N2 fixed: threshold metrics now emit before the conditional claim. This removes the permanent alert-loss window; rare concurrent/crash retries may emit a harmless duplicate, which is documented.
  • N3 fixed: operator status now rejects corrupt non-positive configured limits consistently with admission and personal status.
  • N4 retained intentionally: duplicate deliveries still reload state and retry alert work. Returning early after the task dedupe marker would permanently lose an alert when spend committed but alert delivery failed. A code comment and recovery test now make this explicit.
  • N5 deferred: reliably recording the modifying AWS principal would require a new STS dependency/request and is better handled as a dedicated auditability enhancement.
  • N6 addressed: native Cognito array claims preserve exact names; docs recommend avoiding whitespace/commas because API Gateway's string fallback treats them as delimiters.
  • N7 fixed: added admission and rollup coverage for the 99-scope transaction boundary.

The failed build (agentcore) check was unrelated to agent registry. AgentCore built successfully; the mutation guard caught a stale root yarn.lock after the new Cognito SDK dependency. This commit includes the regenerated lockfile, verified with yarn install --frozen-lockfile.

Validation on the final source changes:

  • CDK: 203 suites, 4,124 tests
  • CLI: 58 suites, 770 tests
  • CDK/CLI lint and TypeScript compilation
  • Docs check/build: 78 pages
  • Docs link check and all drift-prevention checks

@isadeks

isadeks commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Heads up before review: this branch is conflicting with main, so it can't merge as-is. Given the size (57 files, +3573/-94) I'd rather review the post-rebase diff than one that's going to shift — a rebase can meaningfully change what a cost/budgeting change touches.

Worth noting your CI is fully green (8/8) at the current head, so this is purely a merge-base issue, not a code problem.

Ping me once it's rebased and I'll do a full review pass.

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Verdict

Request changes — the design is right and the implementation is unusually careful (idempotent stream rollup, one-shot alert claims, emit-before-claim, fail-closed admission, real failure-path tests), but the branch is CONFLICTING against main, three doc comments describe the opposite execution order from the code they document, the new Cognito AdminListGroupsForUser call sits unconditionally in the task-admission hot path of five ingest channels, and the new user-facing docs link resolves to the wrong published page.

Review method disclosure: this was a static review. I did not run mise run build, jest, pytest, tsc, or cdk synth — this environment has no node_modules/ or agent/.venv/. Every claim below is derived from reading the code at a41a06f. The one thing I did execute is the docs mirror check (see §5), in a throwaway copy, because docs/scripts/sync-starlight.mjs needs only node:fs/node:path.

Governance: clean. Issue #471 is OPEN and carries approved; the branch name feat/471-org-team-budgets matches the ADR-003 convention; the delivered scope maps 1:1 onto the issue's five acceptance criteria (budget store, admission in createTaskCore, 80/100% CloudWatch alarms + SNS, bgagent budget status, bgagent budget set). Two nits: the issue has no priority label (P0/P1) and the PR has no labels at all.

2. Vision alignment

Squarely on the bounded blast radius & cost tenet (docs/design/VISION.md). Until now the only cost bound was per-task (max_budget_usd); a fleet operator had no recurring, human-scoped ceiling. This adds one at the right layer — admission, not mid-flight termination — which also preserves fire-and-forget: nothing here turns a task into an interactive session, and the refusal is a synchronous 429 BUDGET_EXCEEDED at submit time rather than a surprise mid-run kill.

Two deliberate trade-offs, both of which I think are correct but only one of which is stated clearly enough:

  • Alert-only by default; hard stop opt-in per scope. This matches #471's "optional hard stop" wording, so it is not an undocumented tenet trade. Worth stating explicitly in COST_ATTRIBUTION.md that the default posture is observe-only, so an operator does not read "budgets" as "enforcement".
  • In-flight work is never terminated at 100%. DEVELOPER_GUIDE.md:206 says so plainly ("without terminating in-flight work"). Good — that is the honest framing, and it means a single scope can overshoot its limit by the cost of whatever is already running.

Reviewable outcomes: the EMF metric + alarm + bgagent budget status triad keeps the control inspectable. The one gap is attribution of the control itself: see N-5.

3. Blocking issues

B-1. Branch is CONFLICTING with main — cannot merge, and the green CI is against a stale base

gh pr view 775 reports mergeable: CONFLICTING, mergeStateStatus: DIRTY. Merge-base is 2cee8800; main has moved on since (including today's 4da0a1f browserslist re-resolve). 17 files are touched by both this PR and main-since-merge-base, and they are not incidental:

cdk/src/stacks/agent.ts
cdk/src/handlers/shared/types.ts
cli/src/types.ts
cdk/package.json
yarn.lock
cli/README.md
+ 5 generated docs mirror files, + 6 others

All 8 checks are SUCCESS, but they ran against a base that no longer exists — a re-run post-rebase is not a formality here, because agent.ts and both types.ts files are exactly the kind of overlap that produces a semantically-clean-but-wrong merge. This also confirms @isadeks' request on the PR thread. Fix: merge/rebase origin/main, then per AGENTS.md run mise //cdk:eslint + mise //cli:eslint, commit any autofixes ("Fail build on mutation" rejects uncommitted lint output), re-run mise //docs:sync, then mise run build.

B-2. cdk/src/handlers/shared/budgets.ts:281 — unconditional Cognito AdminListGroupsForUser on every headless task creation, fail-closed, even when zero budgets are configured

const teamIds = suppliedTeamIds === undefined
  ? (budgetTableName ? await resolveTeamIds(userId) : [])   // ← line 281
  : [...new Set(suppliedTeamIds)].sort();

Only create-task.ts:54 supplies teamIds (from the verified cognito:groups JWT claim — good, see §7). Every other creator passes undefined: webhook-create-task.ts:150, slack-command-processor.ts:256, jira-webhook-processor.ts:770,986, linear-webhook-processor.ts (5 call sites), and orchestration-release.ts:502 / orchestration-reconciler.ts:1394 — i.e. the DAG child-release path. Each of those now makes a Cognito control-plane call before it can create a task.

The gate is budgetTableName, and cdk/src/stacks/agent.ts:127 provisions BudgetTable unconditionally and wires BUDGET_TABLE_NAME into all of them — so this is not opt-in. A deployment that has never run bgagent budget set pays the call on every single headless submission, and resolveTeamIds (lines 124-144) has no try/catch: any AdminListGroupsForUser failure (throttle, UserNotFoundException for a mapping row whose Cognito user was deleted, a transient 5xx) propagates to create-task-core.ts, which fail-closes to 503 SERVICE_UNAVAILABLE. Team resolution is ordered before any budget row is read, so the call happens even when the answer cannot possibly change the outcome.

I want to be precise about the blast radius: orchestration-release.ts treats 5xx as transient, so a DAG release retries rather than strands, and the IAM grants are all correctly scoped (§7). This is not data loss. But it is a new hard cross-service dependency in the admission path of every non-Cognito ingest channel, added for a feature that is inert until an operator configures it, and it is the kind of coupling that is very hard to remove once six call sites depend on it.

Fix (pick one):

  • Resolve group membership lazily: read the USER# config/spend rows first, and only call Cognito when at least one TEAM# CONFIG row exists (the record_type-scope_key-index GSI you already build makes "are any team budgets configured?" a single cheap query, cacheable in module scope across warm invocations); or
  • Keep the call but scope the fail-closed behaviour to the case where enforcement is actually possible — catch the Cognito error, and if the caller's own scope has no hard-stop config and no team budgets exist, log and continue with teamIds: [] rather than refusing the task; or
  • If the unconditional call is intentional, say so in docs/design/ (or a short ADR note) with the reasoning — an undocumented widening of the admission path's failure surface is the thing Stage 2 of our review bar treats as blocking.

B-3. cdk/src/handlers/orchestration-reconciler.ts:23-24 — the docstring states the reverse of what the code does, and the test enshrines the code

 * Consumes the **TaskTable DynamoDB stream**. For each terminal task it first
 * applies an idempotent user/team monthly cost rollup. For graph tasks it then:

The code (lines 1499-1520) does the opposite: the orchestration try block (parseTerminalTaskRecordcascadeRestack / reconcileTerminalChild) runs first, and await rollupTaskCost(record) runs second. cdk/src/constructs/orchestration-reconciler.ts:56-58 repeats the same inversion ("It rolls up every positive task cost into monthly user/team budgets, then drives parent/sub-issue orchestration"), and cdk/test/handlers/orchestration-reconciler.test.ts:340 locks in the actual order by name: 'releases orchestration dependents before reporting a budget-rollup retry'.

Two reasons this is blocking rather than a nit:

  1. It is a behavioural claim, not prose. Within one stream batch, a parent task's terminal record releases its unblocked children before the parent's own cost_usd has been added to the monthly rollup. So each released child is admitted against pre-rollup spend, and a hard-stopped scope can overshoot by one wave of children. That is a defensible design (it is also the ordering that keeps a rollup failure from stranding a DAG), but the two docstrings promise the safer ordering and the code delivers the looser one — a future reader trusting the comment will reason wrongly about the enforcement boundary.
  2. The next person "fixes" the wrong side. With three artefacts disagreeing (two comments vs. code vs. a test name), whoever notices will pick one at random.

Fix: decide and make all four agree. Either swap the two try blocks (they are independent — each captures its own error and orchestrationError ?? budgetError already prefers the orchestration failure, so swapping changes only the admission snapshot the children see), or keep the current order and rewrite both docstrings to say "orchestration first, then rollup" and note the one-wave overshoot explicitly.

B-4. docs/guides/COST_ATTRIBUTION.md:26 and docs/guides/DEVELOPER_GUIDE.md:206 — the new cross-reference resolves to the wrong published page

Both files link [Monthly user and team budgets](./USER_GUIDE.md#monthly-user-and-team-budgets). In-repo that resolves fine, which is why CI is green. On the published site it does not:

  • docs/scripts/sync-starlight.mjs:72-84 (userGuideAnchorRoutes) has no entry for monthly-user-and-team-budgets, so the link falls through to explicitGuideRoutes.USER_GUIDE/using/overview#monthly-user-and-team-budgets (verified in the committed mirror at docs/src/content/docs/getting-started/Cost-attribution.md:30 and docs/src/content/docs/developer-guide/Model-configuration.md:92).
  • But USER_GUIDE.md:272 adds the section as ### Monthly user and team budgets nested under ## Per-repo overrides (line 216), and splitGuide splits on ## — so the content renders at /customizing/per-repo-overrides#monthly-user-and-team-budgets (confirmed: Monthly user and team budgets appears in src/content/docs/customizing/Per-repo-overrides.md, not in using/Overview.md).

Net effect: both links land readers on the Overview page at an anchor that does not exist there — the two most likely entry points to the new feature's documentation dead-end. CI cannot catch this: docs/scripts/link-check.sh:7 scans only guides/, design/, decisions/ and root *.md, never the generated mirror, and the relative source link is genuinely valid. sync-starlight.mjs already carries a comment describing this exact 404 mode for COST_ATTRIBUTION, so the trap is known.

Fix (one line): add 'monthly-user-and-team-budgets': '/customizing/per-repo-overrides#monthly-user-and-team-budgets', to userGuideAnchorRoutes and re-run mise //docs:sync. Alternatively — and better for a feature this prominent — promote the section to a top-level ## in USER_GUIDE.md so it gets its own page, then map it.

4. Non-blocking suggestions / nits

N-1. cdk/src/handlers/shared/budgets.ts:115 and cli/src/budget-store.ts:67 — private numeric() fails open on garbage. Both return 0 for NaN, Infinity, objects, and unparseable strings. A corrupted spend_usd therefore reads as "no spend" and admits work, silently, with no log line. The repo already has the right helper: cdk/src/handlers/shared/numeric.ts coerceNumericOrNull (used at get-task-replay.ts:190 and fanout-task-events.ts:842,847), which logs the coercion. Its docstring says a third call site is the signal to widen the type — this PR adds two. Note the asymmetry with loadBudgetStates/loadPersonalBudgetStatus, which do fail closed by throwing on monthly_limit_usd <= 0; the limit side is strict and the spend side is lenient.

N-2. Duplicated cross-package contracts with no parity test. USER#/TEAM#/CONFIG (budgets.ts:29-32 vs budget-store.ts:28-32), ROLLUP_RETENTION_DAYS = 400 (budget-rollup.ts:43 vs budget-store.ts:34), the index name (budget-table.ts:24 exports BUDGET_CONFIG_INDEX_NAME, and budget-store.ts:30 re-declares the same literal instead of importing it), the 100% hard-stop threshold, and — most fragile — the alerted_80_at/alerted_100_at marker names, which budget-rollup.ts claims with attribute_not_exists(...) and budget-store.ts:141-142 REMOVEs to re-arm. Rename a marker on the CDK side and the CLI's re-arm becomes a silent no-op: REMOVE of a non-existent attribute succeeds, and alerts stay permanently suppressed after a limit raise. contracts/constants.json already exists for exactly this (it holds max_budget_usd); at minimum add a test asserting the two sets of literals match.

N-3. cdk/src/constructs/budget-table.ts:24BUDGET_CONFIG_INDEX_NAME is exported but never imported. Dead export; the one consumer that needs it (the CLI) hardcodes the string. Either import it or drop the export.

N-4. cli/src/budget-store.ts:114,163,198 — bypasses cli/src/dynamo-clients.ts and builds a new client per call. documentClient(region) (dynamo-clients.ts:25) is the established seam; three raw makeDocClient({ region }) calls both diverge from it and forgo connection reuse across the three calls a single budget status makes. Attribution is correct either way (both go through cli/src/ua.ts), so this is style + coherence, not #319.

N-5. budget set has no record of who changed the limit. setMonthlyBudget writes updated_at but not an updated_by. Since the CLI writes straight to DynamoDB with the operator's own credentials, the only trail is CloudTrail data events on the table, which are off by default. For a control that gates other people's work, updated_by is a cheap addition. (I see this is your deferred N-5 from the self-review — agreed it is not a merge blocker, but it is the one deferral I would file as a follow-up issue rather than leave implicit.)

N-6. cdk/src/handlers/list-tasks.ts:55GET /tasks?view=budget overloads a collection endpoint with a singleton resource, and unknown view values are silently ignored. A caller who typos view=budgets gets a task list with a 200, not a 400. A sibling route (GET /budget, or /users/me/budget) would be more honest; failing that, validate the view enum and 400 on anything unrecognized. Authorization is correct — loadPersonalBudgetStatus(userId) uses the authorizer-derived id, so there is no IDOR here.

N-7. cdk/src/handlers/shared/budgets.ts:287-292 — the >99-scope guard surfaces as a misleading 503. A user in 99+ Cognito groups throws, and create-task-core maps the throw to 503 SERVICE_UNAVAILABLE — "try again later" for a permanent configuration condition that retrying will never clear. Consider a distinct 4xx (or at least a distinguishable error code) so the operator learns the real cause.

N-8. Alarm granularity. budget-alerts.ts aggregates BudgetThresholdCrossed by Threshold only, so the alarm fires without naming the breaching scope. The docs correctly point the responder at the logs, and adding scope_key as an EMF dimension would be a cardinality footgun, so I think the design is right — worth one sentence in the runbook prose making the "alarm tells you that, logs tell you who" contract explicit.

N-9. No budget unset. budget set can only raise/lower a limit, never remove it (--monthly-usd is validated >= MIN_MONTHLY_BUDGET_USD = 0.01, and the loaders throw on <= 0, so "set it to zero to disable" is not available either). Deleting the CONFIG row currently requires the AWS console or CLI. Follow-up, not a blocker.

N-10. cdk/test/constructs/budget-table.test.ts:26,61new App() + Template.fromStack() per test (#366). Only two tests and one small construct, so the cost is negligible, but the repo's stated pattern is one synth in beforeAll. Cheap to align now, before this file grows.

5. Documentation

Genuinely good, and better than the median for a feature this size. Reviewed: docs/guides/COST_ATTRIBUTION.md (new budget section, the cost-of-controls table, and — refreshingly — the caveat that rollups are estimates that will not tie out to the invoice), docs/guides/USER_GUIDE.md:272 (operator walkthrough), docs/guides/DEVELOPER_GUIDE.md:206 (admission-control framing, and the explicit "does not terminate in-flight work"), cli/README.md (both subcommands, flags, and output shapes). Env vars (BUDGET_TABLE_NAME, USER_POOL_ID) and the new BudgetTableName output are covered.

Mirror sync: verified in sync. I copied the worktree to a scratch directory, ran node docs/scripts/sync-starlight.mjs there, and diff -r against the committed docs/src/content/docs/ produced no output. The "Fail build on mutation" step will not trip on the mirror. (This ran in a throwaway copy and touched neither the PR worktree nor any build tooling.)

Missing: only the B-4 anchor routing. No ADR is required in my view — this implements an existing tenet with existing primitives rather than making a new architectural choice — though if you take the B-2 "document the rationale" branch, that note is the place for it.

Issue tracking: #471 is approved and OPEN and accurately describes the delivered work. Add a priority label to it and some labels to this PR.

6. Tests & CI

Bootstrap synth-coverage: not applicable — verified, not assumed. I traced every resource type this PR introduces. AWS::DynamoDB::Table and AWS::CloudWatch::Alarm are both already present in cdk/src/bootstrap/resource-action-map.ts; application.ts already grants dynamodb:CreateTable/UpdateTable/UpdateTimeToLive on arn:aws:dynamodb:*:*:table/backgroundagent-dev-* (the new table's physical name matches that pattern), and observability.ts already grants cloudwatch:PutMetricAlarm. The new cognito-idp:AdminListGroupsForUser is a runtime task-role permission granted in-stack, not a deploy-time CFN action, so it does not touch the bootstrap bundle. No BOOTSTRAP_VERSION bump and no artifact regeneration required — correct as submitted. (Static reasoning; I could not run mise //cdk:test -- test/bootstrap/synth-coverage.)

API type sync: OK. PersonalBudgetStatus is byte-identical in cdk/src/handlers/shared/types.ts and cli/src/types.ts. TaskRecord.team_ids?: readonly string[] is added on the CDK side only, which is right — it is a persisted-record field, not part of the wire contract.

Coverage is a strength. The tests go after the paths that actually break in production, not the happy path: idempotent replay of an already-rolled-up task (transaction cancelled + marker exists), re-emission when the alert claim write fails after the metric was emitted, both thresholds crossing inside a single rollup, the 99-scope boundary, fail-closed 503 when admission throws, and 429 with no write. The emit-before-claim ordering in budget-rollup.ts is the right call and is the kind of thing that only gets built when someone has thought about a crash between the two writes — it has a comment explaining why, and a test proving it.

Gaps I would like to see closed: (a) a test pinning the reconciler ordering to whatever B-3 resolves to, asserted on behaviour (child admitted against post-rollup spend, or not) rather than encoded in a test name; (b) a resolveTeamIds failure test — today there is no coverage of what happens when Cognito throws, which is precisely the B-2 path; (c) a cross-package parity test for the N-2 literals.

CI: 8/8 SUCCESS, but see B-1 — that verdict is against a base main no longer resembles. reviewDecision: REVIEW_REQUIRED, no labels.

7. Review agents run

Nested agent dispatch was unavailable in this run. This review was executed as a subagent inside a batch fan-out, and that harness permits only one level of nesting, so I could not dispatch the pr-review-toolkit agents (code-reviewer, silent-failure-hunter, type-design-analyzer, comment-analyzer, pr-test-analyzer) that Stage 3 of our review bar names. I am not claiming to have invoked them. Instead I applied each rubric dimension myself, explicitly and one at a time:

  • code-reviewer (applied inline). AGENTS.md routing is correct (API/Lambdas in cdk/, CLI in cli/, nothing misplaced into agent/); solution-UA attribution is correct everywhere (makeClient/makeDocClient throughout — no naked new XxxClient({}), so #319 holds); L2 constructs, PAY_PER_REQUEST, PITR, TTL, no hardcoded ARNs or account IDs. Findings: N-3, N-4, N-10.
  • silent-failure-hunter (applied inline). Traced every catch and every default. Findings: N-1 (garbage → 0, admits work, no log), N-6 (unknown view silently ignored), N-7 (permanent condition reported as transient), and it is what made B-2 visible (an uncaught Cognito throw converted to a 503 refusal). Credit where due: the admission path fails closed, loadBudgetStates throws on a non-positive limit, and the UnprocessedKeys loop in batchGetItems is handled rather than dropped.
  • type-design-analyzer (applied inline). PersonalBudgetStatus, BudgetState, BudgetAdmission, BudgetScope, TaskCostEvent are all narrow, readonly, and discriminated where they need to be; parseBudgetScopeKey returns a proper union rather than string-sniffing at call sites. null-vs-undefined is used consistently ("configured but absent" vs "not configured"). No finding beyond N-2's stringly-typed duplicated literals.
  • comment-analyzer (applied inline). This is where B-3 came from — two docstrings asserting the opposite execution order from the code. Everything else I spot-checked was accurate and several comments were load-bearing in a good way (the emit-before-claim rationale in budget-rollup.ts, the "reserve one action for the task marker" note on MAX_BUDGET_SCOPES_PER_TASK = 99, the stream-contention rationale on the reconciler construct, the 429-is-deterministic reasoning in orchestration-release.ts).
  • pr-test-analyzer (applied inline). See §6 — strong failure-path coverage, three named gaps.
  • security-review (Skill invoked; harness diff was empty, so analysis performed inline). I invoked the Skill because the diff adds IAM statements and touches the input gateway. Its harness resolved the diff against the wrong working tree and returned nothing, and its methodology requires parallel sub-tasks I cannot dispatch here, so I ran the analysis myself against the PR tree. No HIGH or MEDIUM exploitable finding. Specifically verified: (a) extractUserGroups (gateway.ts:47) reads only authorizer.claims['cognito:groups'] — the authorizer-verified JWT — never a header or request body, so team attribution is not client-forgeable, and POST /tasks is Cognito-only (task-api.ts:868), not reachable through the API-key/webhook authorizer that lacks claims; (b) no IDOR on the read path — list-tasks.ts:56 passes the authorizer-derived userId, and there is no ?user= override; (c) IAM least privilege is clean and consistent — all six cognito-idp:AdminListGroupsForUser grants (agent.ts:1211,1308, task-api.ts:1301, jira-integration.ts:329, slack-integration.ts:317, linear-integration.ts:328) are scoped to userPool.userPoolArn, never *, and the budget table grants are correctly asymmetric: grantReadData for the six admission-only consumers, grantReadWriteData only for the stream reconciler that actually writes rollups (constructs/orchestration-reconciler.ts:129), with the stranded reconciler correctly read-only (agent.ts:1304) because it never calls rollupTaskCost; (d) no NoSQL-injection surface — all DynamoDB access uses parameterized ExpressionAttributeValues; (e) no secrets, no new network egress, no deserialization. The N-5 audit gap is a defence-in-depth item, not a vulnerability.
  • Genuinely out of scope, omitted: Cedar/HITL policy review — the diff contains no Cedar policy or approval-gate change (budget admission is a hard-coded numeric check, deliberately not a Cedar decision). Agent-runtime review — nothing under agent/ changed, and the runtime never reads the budget table. Cedar engine parity (cedarpy / @cedar-policy/cedar-wasm) — no pin moved.

8. Human heuristics

  • Proportionality — pass, with one note. The complexity is earned: a two-key table with a GSI, one stream consumer, one admission check, two alarms, two CLI subcommands. No speculative abstraction, no "engine", no factory-for-one. Largest new file is 325 lines. The 100-action transaction limit driving MAX_BUDGET_SCOPES_PER_TASK = 99 is a real constraint handled at the right place. The one disproportion is B-2: an unconditional cross-service call to support a feature that is inert until configured.
  • Coherence — concern (N-2, B-3). Same concept, three homes: the scope-key/TTL/threshold/alert-marker contracts are re-declared in cli/ rather than shared, while contracts/constants.json already exists for precisely this and already holds max_budget_usd. And "same concept, same story" fails in orchestration-reconciler where the comments and the code describe different orderings. Otherwise the vocabulary is consistent and matches the rest of the repo (scope_key, period, hard_stop, utilization_percent).
  • Clarity — concern (N-1, N-6, N-7). Names are good and intent-revealing (hard_stop_active vs hard_stop is a genuinely useful distinction; budgetPeriod/budgetResetAt say what they do). The concerns are all error-surfacing: a plausible default (0) hiding corrupt data, a silently-ignored query parameter, and a permanent condition dressed as a transient one (AI004).
  • Appropriateness — pass, with one AI005 caution. The DynamoDB semantics are used correctly against real API behaviour, not mock-shaped behaviour: TransactWriteCommand cancellation reasons are inspected properly via isTransactionCanceled + markerExists, ADD is used for the counters instead of read-modify-write, UnprocessedKeys is looped, ConsistentRead is set where correctness needs it, and TTL is set on rollup rows but not on CONFIG rows (a config that quietly expired after 400 days would have been a nasty bug — good catch on your part). The AI005 caution is cdk/test/handlers/orchestration-reconciler.test.ts:340, whose name asserts what the code does while the docstring says the code should do the opposite — the test is currently ratifying the behaviour rather than specifying it.

Overall: this is careful, well-tested work on the right problem, and I expect to approve it quickly. B-1 you have to do anyway; B-3 and B-4 are small; B-2 is the one that deserves a real answer rather than a patch — a lazy resolution or a documented rationale both work for me.

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.

feat(cost): org and team monthly budgets with alerting

3 participants