Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,17 @@ jobs:
# contract: ~90 `pitfall #N` references point at these headings.
run: python scripts/check_pitfalls_index.py

- name: Check runbook commands do not hardcode a prefixed resource name
working-directory: ${{ github.workspace }}
# Lambda/topic/DLQ/rule/alarm names derive from ResourcePrefix, and the runbook
# tells operators NOT to deploy at the `cdr` default (a live askkaifbot service
# owns those names in ap-southeast-1). A command hardcoding `cdr-<suffix>` sends
# the reader at a resource they do not have — or at somebody else's production.
# That shipped twice for the EventBridge rule. Suffixes are derived from the IaC
# at runtime, never hand-listed (pitfall #61); deliberate literals declare
# themselves with a reasoned `<!-- prefix-literal-ok: … -->` marker.
run: python scripts/check_runbook_prefix.py

- name: Sweep OPC declaration mechanisms for unscrubbed parts
working-directory: ${{ github.workspace }}
# Regression harness for the bug class that produced pitfalls #54 and #55: OPC
Expand Down
12 changes: 12 additions & 0 deletions docs/claude/pitfalls.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ it is listed once, under the group whose code you would be editing.
- [#59 — A resource cap that `return`s hands the attacker the sweep's coverage — bound the work, but fail closed](#59-a-resource-cap-that-returns-hands-the-attacker-the-sweeps-coverage-bound-the-work-but-fail-closed)
- [#60 — Auditing every cap's failure direction, and making "fail closed" a CI guard instead of a habit](#60-auditing-every-caps-failure-direction-and-making-fail-closed-a-ci-guard-instead-of-a-habit)
- [#61 — The #60 audit enumerated caps it already knew about — `_MAX_WALK_NODES` was never on the list, and truncated in two sweeps](#61-the-60-audit-enumerated-caps-it-already-knew-about-_max_walk_nodes-was-never-on-the-list-and-truncated-in-two-sweeps)
- [#62 — A runbook command that hardcodes a prefixed resource name points the operator at somebody else's production](#62-a-runbook-command-that-hardcodes-a-prefixed-resource-name-points-the-operator-at-somebody-elses-production)

**Multi-bug audit batches**

Expand Down Expand Up @@ -411,3 +412,14 @@ An external review (Codex, 2026-08-17) found the exact bug class #59 documents a
**Why #60 missed it, and the lesson that is not "audit harder".** #60's table is a *hand-written enumeration* — ten constants someone remembered. `_MAX_WALK_NODES` is a *shared* cap used by two functions and named nothing like `_PDF_WALK_MAX_NODES`, so it fell out of recall. `check_fail_closed.py` could not catch it either: that guard mutates rejections that **exist** and asks whether a test notices; it is structurally blind to a cap that never raises in the first place. **A guard that verifies existing invariants cannot find a missing one.** The complement to `check_fail_closed.py` is enumeration from the source, not from memory: `grep -n '_MAX_[A-Z_]*\|_BUDGET' src/lambda_function.py` yields the real list, and every hit must be traced to an explicit failure direction before the audit counts as complete.

**General rule: when a review concludes "no second instance", state how the instances were enumerated.** "I checked the caps" and "I listed every `_MAX_`/budget constant in the module and traced each" are different claims with the same wording, and only the second one is falsifiable. Related: #59 (the bug class), #60 (the audit that missed this), #57 (a check that cannot fail is not a check).

### 62. A runbook command that hardcodes a prefixed resource name points the operator at somebody else's production
The two Low findings from the same external review as #61, plus the guard that makes the class non-recurring.

`docs/deployment-runbook.md` told operators to inspect `--rule cdr-s3-object-created`, but Terraform names it `${var.resource_prefix}-s3-object-created` (`terraform/main.tf:363`). The same runbook, two sections earlier, tells the reader **not** to deploy at the `cdr` default — a live askkaifbot service already owns those names in ap-southeast-1 (see the deploy notes). So the documented command is wrong for exactly the readers who followed the documented advice: best case `ResourceNotFoundException` during an incident, worst case they inspect the unrelated production service and draw conclusions from it. Also fixed alongside: `MaxMemoryUsed` is **not** an `AWS/Lambda` CloudWatch metric — the query returns an empty `Datapoints` list, which reads as "no memory pressure" but means "no such metric"; `docs/benchmark.py` had it right all along, parsing the REPORT log line via Logs Insights.

**Why a guard, and why this one is narrow.** Both bugs are IaC/doc drift, the class `check_iac_parity.py` and `check_cap_defaults.py` already exist for — but neither looks at *command literals*. `scripts/check_runbook_prefix.py` closes that: it derives the prefixed-name suffixes **from `src/template.yaml` and `terraform/main.tf` at runtime** (13 of them) rather than hand-listing, which is #61's lesson applied one commit later, and flags a `cdr-<suffix>` only where it is *passed to a command* — an option-value run or a `/aws/lambda/…` path. Prose, parameter tables and S3 tag names (`cdr-status=sanitised`) are deliberately untouched: the runbook must be able to say "do not leave `ResourcePrefix` at the `cdr` default", and a guard that flags its own advice gets silenced rather than fixed.

**The exemption is the interesting part.** Section 8's `ResourceExistenceCheck` block investigates the *foreign* `cdr-lambda` the reader did not deploy — there the literal is correct and `$PREFIX-lambda` would be actively wrong. No syntactic rule separates that from a genuine bug, so the doc declares it inline with a reasoned `<!-- prefix-literal-ok: … -->` marker, and the guard prints every exemption in its summary so a growing pile is visible rather than quietly normal. **A marker with no reason is itself a failure**, as is one placed *inside* a fenced block — an HTML comment there is not a comment, it renders literally and lands mid-command when the command is line-continued (found by writing it that way first).

**Two guard bugs caught by its own negative controls, which is the point of running them.** (a) The first regex matched only a name sitting immediately after `--opt`, so an injected `--dimensions Name=FunctionName,Value=cdr-lambda` — the form the runbook's own CloudWatch commands use — passed clean. Matching the whole argument run fixed it. (b) The pass/fail harness initially counted `grep -c 'hardcodes'`, which also matched the help text, so a failing control *looked* like it fired. Five controls now run: the pre-fix runbook, an empty exemption reason, a marker inside a fence, a newly-injected literal, and the current file as the false-positive canary. **A guard never shown failing is an assumption (#57); a guard shown failing by a broken harness is worse, because it looks like evidence.** Related: #61 (enumerate from source), #57 (instrument discipline), #60 (making a rule a CI guard rather than a habit).
1 change: 1 addition & 0 deletions docs/deployment-runbook.md
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ fires when another stack — or another IaC tool entirely — holds them.
it is unmanaged or unused: it may be a live service managed by Terraform/CDK from a
different repo. Identify the owner before doing anything:

<!-- prefix-literal-ok: this whole block investigates the FOREIGN cdr-lambda that already owns the name — $PREFIX would point at the reader's own stack and defeat the check -->
```bash
# Is it serving traffic? Recent log activity is the fastest tell.
aws logs tail /aws/lambda/cdr-lambda --since 30d --format short | tail -20
Expand Down
186 changes: 186 additions & 0 deletions scripts/check_runbook_prefix.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
#!/usr/bin/env python3
"""Fail if a runbook CLI command hardcodes a `cdr-`-prefixed resource name.

Lambda, SNS topic, DLQ, IAM role/policy, EventBridge rule and alarm names all derive
from `ResourcePrefix` / `var.resource_prefix`. The runbook tells operators NOT to deploy
at the `cdr` default (a live askkaifbot service already owns those names in
ap-southeast-1), then exports `PREFIX` in section 1 for every command to use. A command
that hardcodes `cdr-<suffix>` therefore inspects a resource the reader does not have —
they see `ResourceNotFoundException`, or worse, somebody else's production resource.

That shipped twice: `aws events list-targets-by-rule --rule cdr-s3-object-created` and
`aws events describe-rule --name cdr-s3-object-created`, while Terraform actually names
the rule `${var.resource_prefix}-s3-object-created`. An operator following the runbook's
own advice to use a non-default prefix would misdiagnose a deployment or an incident.

WHAT IS CHECKED, AND WHY THE SCOPE IS NARROW

Only `cdr-<suffix>` where <suffix> is a suffix the IaC actually derives from the prefix
(read from src/template.yaml and terraform/main.tf at runtime — never hand-listed here,
see pitfall #61), and only where the name is being *passed to a command*:

--rule cdr-s3-object-created an argument value
--function-name cdr-lambda an argument value
--dimensions Name=FunctionName,Value=cdr-lambda a Key=Value argument
--alarm-names cdr-lambda-errors cdr-lambda-p99 later items in a multi-value argument
/aws/lambda/cdr-lambda a log-group path

The `Value=` and multi-value forms are not decoration: CloudWatch commands here pass the
function name as `Name=FunctionName,Value=…`, and a first cut of this guard matched only a
name sitting immediately after `--opt`, so an injected `Value=cdr-lambda` passed clean. The
guard's own negative control caught that (#57) — hence matching the whole argument run.

Prose, table cells and parameter-value examples are deliberately NOT flagged. The runbook
must be able to say "do not leave ResourcePrefix at the `cdr` default" and show
`cdr-staging-source-<alias>` as a suggested value; flagging those would make the guard
unusable and it would be silenced rather than fixed.

THE EXEMPTION, AND WHY IT MUST EXIST

Section 8's `AWS::EarlyValidation::ResourceExistenceCheck` entry is about investigating a
*pre-existing* `cdr-lambda` the reader did NOT deploy — the real askkaifbot function. There
`cdr-lambda` is correct and `$PREFIX-lambda` would be wrong. A syntactic rule cannot tell
that apart from a genuine bug, so the doc declares it inline:

<!-- prefix-literal-ok: investigating the foreign cdr-lambda, not the reader's stack -->

The marker must carry a reason and is counted in the summary, so a growing pile of them is
visible rather than silently normal. It must sit OUTSIDE the fence, immediately before the
```bash line — an HTML comment inside a fenced block is not a comment, it renders literally
and breaks copy-paste (and lands mid-command if the command is line-continued). Placed
there it covers the whole block; placed before an ordinary line it covers that line only.

Run: python scripts/check_runbook_prefix.py
"""

from __future__ import annotations

import re
import sys
from pathlib import Path

REPO = Path(__file__).resolve().parent.parent
RUNBOOK = REPO / "docs" / "deployment-runbook.md"
TEMPLATE = REPO / "src" / "template.yaml"
TERRAFORM = REPO / "terraform" / "main.tf"

EXEMPT_MARKER = "prefix-literal-ok:"

# Everything after an option up to the next option / end of line is that option's value
# run: `--rule cdr-x`, `--function-name=cdr-x`, `Name=FunctionName,Value=cdr-x`, and
# multi-value forms like `--alarm-names cdr-a cdr-b`. Names are then picked out of the run.
ARG_RUN = re.compile(r"--[a-z][a-z-]*(?:[ =])((?:(?!\s--)[^\n])*)")
NAME_IN_RUN = re.compile(r"(?<![\w./-])(cdr-[a-z0-9-]+)")
LOG_GROUP = re.compile(r"/aws/lambda/(cdr-[a-z0-9-]+)")


def prefix_suffixes() -> set[str]:
"""Suffixes the IaC derives from the resource prefix, read from the IaC itself.

Hand-listing these is the mistake pitfall #61 records: the #60 cap audit enumerated
from memory, missed `_MAX_WALK_NODES`, and shipped a fail-open. Deriving the list
means a resource added to either IaC file is covered without touching this script.
"""
suffixes: set[str] = set()
for path, pattern in (
(TEMPLATE, r"\$\{ResourcePrefix\}-([a-z0-9-]+)"),
(TERRAFORM, r"\$\{var\.resource_prefix\}-([a-z0-9-]+)"),
):
if not path.exists():
sys.exit(f"missing {path.relative_to(REPO)} — cannot derive prefixed names")
suffixes.update(re.findall(pattern, path.read_text(encoding="utf-8")))
if not suffixes:
sys.exit("derived zero prefixed resource names — the IaC patterns must have changed")
return suffixes


def main() -> int:
if not RUNBOOK.exists():
sys.exit(f"missing {RUNBOOK.relative_to(REPO)}")

suffixes = prefix_suffixes()
lines = RUNBOOK.read_text(encoding="utf-8").splitlines()

problems: list[str] = []
exempted: list[str] = []
exempt_armed = False # marker seen, not yet consumed
exempt_reason = ""
exempt_block = False # marker armed a fenced block; holds until the fence closes
in_fence = False

for lineno, line in enumerate(lines, 1):
stripped = line.strip()

if EXEMPT_MARKER in line:
if in_fence:
problems.append(
f"{RUNBOOK.name}:{lineno}: {EXEMPT_MARKER} inside a fenced block — an "
"HTML comment there renders literally and can split a line-continued "
"command. Move it above the opening ``` line."
)
continue
exempt_armed = True
exempt_reason = line.split(EXEMPT_MARKER, 1)[1].strip()
if exempt_reason.endswith("-->"):
exempt_reason = exempt_reason[:-3].strip()
if not exempt_reason:
problems.append(
f"{RUNBOOK.name}:{lineno}: {EXEMPT_MARKER} with no reason given"
)
continue

if stripped.startswith("```"):
if in_fence:
in_fence, exempt_block = False, False
else:
in_fence = True
exempt_block = exempt_armed
exempt_armed = False
continue

if not stripped:
continue

candidates = [
name for run in ARG_RUN.findall(line) for name in NAME_IN_RUN.findall(run)
] + LOG_GROUP.findall(line)
hits = {n for n in candidates if n[len("cdr-"):] in suffixes}

if hits and (exempt_block or exempt_armed):
exempted.append(f"line {lineno} ({', '.join(sorted(hits))}): {exempt_reason}")
elif hits:
for name in sorted(hits):
problems.append(
f"{RUNBOOK.name}:{lineno}: command hardcodes `{name}`; "
f"use `$PREFIX-{name[len('cdr-'):]}`\n {line.strip()[:120]}"
)
if not in_fence:
exempt_armed = False

if problems:
print("::error::runbook command hardcodes a prefix-derived resource name")
print("\nHardcoded resource names in runbook commands:\n", file=sys.stderr)
for p in problems:
print(f" {p}\n", file=sys.stderr)
print(
"These names derive from ResourcePrefix, and the runbook tells operators not\n"
"to deploy at the `cdr` default — so the command inspects a resource the\n"
"reader does not have. Use $PREFIX, or, if the literal is deliberate (e.g.\n"
f"investigating a foreign resource), mark it:\n"
f" <!-- {EXEMPT_MARKER} why this literal is correct -->",
file=sys.stderr,
)
return 1

print(
f"{RUNBOOK.name}: no command hardcodes a prefix-derived name "
f"({len(suffixes)} suffixes derived from the IaC, "
f"{len(exempted)} deliberate literal(s) exempted)."
)
for e in exempted:
print(f" exempt: {e}")
return 0


if __name__ == "__main__":
sys.exit(main())